fix(config): preserve MCP source through loadAppConfig unwrap

[89799969]

Problem

validateProjectConfigSecurity is the app's defense-in-depth check for hardcoded credentials in project-level MCP configs. It filters on MCPServerConfig.source === 'project', but loadAppConfig unwrapped MCPServerWithSource to plain MCPServerConfig before validation:

const mcpServers = mcpServersWithSource.map(item => item.server);

The loader only tracks provenance on the wrapper, so every runtime object had source: undefined, the filter was always empty, and the scanner never ran. The unit test passed only because it hand-set .source on the inner type — a shape that never occurs in the real data flow.

Fix

When unwrapping, copy the wrapper's source onto the runtime object (MCPServerConfig.source is already an optional field):

const mcpServers = mcpServersWithSource.map(item => ({
  ...item.server,
  source: item.source,
}));

Both useAppInitialization and plain/initialize.ts then see real provenance.

Tests

• Existing source/config/validation.spec.ts suite still passes
• New regression test loader unwrap keeps source so project configs reach the validator models the real MCPServerWithSource[] shape and asserts:
  • the production-style unwrap keeps only project-server in the project filter
  • the old strip-wrapper path yields an empty filter (the bug)
  • validateProjectConfigSecurity accepts the preserved-source array

5 tests passed

tsc --noEmit passes.

Fixes #1248

[github-actions]

nc-review: comments — 2 nits

@89799969 — a few things worth a look, none blocking.

The change correctly fixes the integration break described in issue #1248 by copying MCPServerWithSource.source onto the runtime MCPServerConfig objects in loadAppConfig, which restores validateProjectConfigSecurity's filter and re-enables the hardcoded-credential scanner for project-level MCP configs. Both consumers (useAppInitialization and plain/initialize) share loadAppConfig and are covered by the single-site fix. The new regression test models the real wrapper shape and demonstrates both the bug and the fix; the existing test (which hand-set .source) is left in place. Changeset, package name and bump type are all correct.

⚪ nit · tests · source/config/validation.spec.ts

The new regression test demonstrates that the unwrap preserves .source and that validateProjectConfigSecurity does not throw on the resulting array, but it does not assert that a warning is actually emitted for a project-level server carrying a hardcoded credential. validateMCPConfigSecurity already has a logWarning capture pattern (see the env-server test at the top of this file). Extending the new test to also assert that validateProjectConfigSecurity(wrapped-with-preserved-source) triggers a warning would directly verify the security-relevant end-to-end behaviour the issue calls out — today the test would still pass even if the validator were silently a no-op for project sources. Not blocking, since the unwrap-preservation assertion is the load-bearing part of the regression test.

⚪ nit · completeness

Issue #1248 also flags source/config/mcp-config-loader.ts:47-63 (mapServerConfig never copies .source onto the returned MCPServerConfig) as part of the root cause. The PR's suggested fix at the consumer site in loadAppConfig is valid and is one of the two approaches the issue explicitly lists, so leaving mapServerConfig untouched is defensible — but worth noting that MCPServerConfig.source is still set only at the unwrap site, which means any other future caller of loadAllMCPConfigs() that forgets to copy .source would silently re-introduce the same bug. A defence-in-depth option would be to have loadAllMCPConfigs() return already-attributed MCPServerConfig[] directly (and retire the wrapper). Not blocking; the current fix does close the reported issue.

---

_{🔴 blocking · 🟠 a reviewer would ask for a change · ⚪ optional}

_{Automated code review — correctness, security, design, tests, plus duplicates and scope. A human still decides; this is not a substitute for review and is not exhaustive. The required status checks separately cover lint, formatting, types, unused dependencies, the test suite and the build. This bot never merges. Maintainers can rerun with /re-review.}

[will-lamerton]

Thanks for this - the root cause analysis is right, the fix in source/config/index.ts is the correct minimal change, and both consumers (useAppInitialization.tsx:210, plain/initialize.ts:163) are covered by it. Changeset is correct too.

Two things I'd like sorted before merge.

1. Re-enabling the scanner emits false positives for correct usage

loadProjectMCPConfig runs substituteEnvVars(mcpServers) at source/config/mcp-config-loader.ts:81, before mapServerConfig. So by the time validateMCPConfigSecurity sees a server, "$API_KEY" has already been resolved to its literal value, and the !value.startsWith('$') check at source/config/validation.ts:17 is always true.

I ran the real path with a temp project .mcp.json:

--- loaded env values after substitution ---
 good-citizen: source=project API_KEY="sk-live-secret-from-env"   # was "$MY_REAL_KEY"
 unset-var:    source=project API_KEY=""                          # was "$NOT_SET_ANYWHERE"
 actually-bad: source=project API_KEY="sk-hardcoded-literal"

--- security warnings captured ---
 * good-citizen
 * unset-var
 * actually-bad
total: 3

Only actually-bad is a genuine finding. The other two are users doing the right thing, and the warning tells them to "Consider using environment variable references (e.g. $API_KEY)", which is exactly what they wrote. Every project with an env key matching token/key/secret/password/auth would get a security warning on startup.

This is likely why the dead filter went unnoticed: the check has never run against real data. Fix is to validate the pre-substitution values, either by scanning inside loadProjectMCPConfig before substituteEnvVars, or by carrying the raw env/headers through for the validator.

2. The regression test can't catch the regression

The new test builds mcpServers by reimplementing the unwrap inline; it never calls anything from source/config/index.ts. I checked out this branch, reverted source/config/index.ts to main, and re-ran:

✔ loader unwrap keeps source so project configs reach the validator
5 tests passed

Green with the production fix entirely absent. source/config/index.spec.ts already has the harness for a real one (temp dir + reloadAppConfig): write a .mcp.json into a temp cwd, call reloadAppConfig(), assert config.mcpServers[0].source === 'project'. That also covers nc-review's first nit, since asserting the warning actually fires is what surfaces issue 1 above.

nc-review's second nit (mapServerConfig still drops .source) is fair but not blocking - effective-config.ts:801 is the only other caller and it destructures source off the wrapper itself.

Also note the required pr-checks haven't run on this commit yet, so lint/types/tests/build are still unverified here. I'll get that approved.

[89799969]

Pushed f4a25ba addressing both review items.

1. False positives from env substitution
loadProjectMCPConfig / loadGlobalMCPConfig / loadEnvMCPConfigs now map first, snapshot pre-substitution env/headers onto rawEnv/rawHeaders, then substitute for runtime. collectMCPSecurityFindings prefers the raw copies, so $MY_REAL_KEY stays silent and only a literal secret warns. unset-var (empty after substitution of an unknown $VAR) also no longer warns.

2. Regression test that can fail
Replaced the inline unwrap simulation with a serial end-to-end test that writes a temp .mcp.json (good-citizen / unset-var / actually-bad), calls reloadAppConfig(), and asserts:

• every runtime server keeps source: 'project' (fails if the unwrap in source/config/index.ts is reverted)
• runtime env.API_KEY is substituted, while rawEnv.API_KEY keeps $MY_REAL_KEY
• collectMCPSecurityFindings reports only actually-bad

Also exported collectMCPSecurityFindings so the scanner is testable without monkey-patching logWarning.

Verified: validation + loader specs pass; tsc --noEmit clean; Biome clean on the touched files.

[89799969]

CI note on 2fd2104:

• All 7721 unit tests that ran include the new validation/loader coverage.
• The only red check is the pre-existing flaky UI test StatsDisplay changes range with arrow keys and closes on Escape (Timed out after 2000ms), which is unrelated to this change (source/commands/stats).
• Config Schema Freshness is green again after regenerating agents.config.schema.json.
• mcp-config-loader.ts coverage rose 88.88% → 92.06% on the previous run; validation.ts remains 100%.

I cannot re-run the workflow without admin rights. Please re-run Unit Tests & Coverage Analysis when convenient, or let me know if you want me to take a look at that stats flake separately.