fix(config): preserve MCP source field in loadAppConfig for security validation

[addyCooks]

Description

Fixes #1248

The validateProjectConfigSecurity function was never executing because the .source field (which identifies whether an MCP server config is project-level, global, or environment-based) was being stripped during config loading in loadAppConfig.

Root cause: In source/config/index.ts, the function unwrapped MCPServerWithSource[] objects by mapping to just item.server, which discarded the wrapper's .source field that the security validator depended on. The validator filtered for server.source === 'project', but this field was always undefined in production.

The fix: Preserve the source field when unwrapping by spreading both item.server and source: item.source.

The hardcoded-credential scanner now correctly receives the source field and will warn users about project-level MCP configs containing apparent secrets (hardcoded API keys, tokens, passwords, etc.).

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update

Changeset

• Added a changeset (pnpm changeset) describing this change for the changelog

Changeset file: .changeset/fix-mcp-config-security-validation.md

Testing

Automated Tests

• New features include passing tests in .spec.ts/tsx files
• All existing tests pass (pnpm test:all completes successfully)
• Tests cover both success and error scenarios

Manual Testing

• Tested with Ollama
• Tested with OpenRouter
• Tested with OpenAI
• Tested MCP integration

Checklist

• If this was for an open issue, I was assigned to it
• Code follows project style guidelines
• Self-review completed
• Documentation updated (if needed)
• No breaking changes (or clearly documented)
• Appropriate logging added using structured logging (see CONTRIBUTING.md)

[github-actions]

nc-review: needs work — 1 blocking, 1 important

@addyCooks — there is a blocking item below.

The fix in source/config/index.ts is correct and resolves issue #1248: preserving the .source field through the loadAppConfig unwrap means validateProjectConfigSecurity will actually receive project-level entries. However, the regression test added to validation.spec.ts reproduces the exact failure mode the issue warned about — it hand-constructs objects with source: 'project' already set on the inner type and never calls loadAppConfig, so the test would still pass if the unwrap bug regressed. The test must be rewritten to actually exercise the unwrap path (e.g. by calling the loader or loadAppConfig and asserting the source field survives on the returned MCPServerConfig).

🔴 blocking · tests · source/config/validation.spec.ts:104

The new test is titled "source field is preserved through loadAppConfig" but it never calls loadAppConfig (or any of the loaders that produce MCPServerWithSource). It hand-constructs an MCPServerConfig[] literal that already has source: 'project' set on the inner object — exactly the input shape the existing failing test uses and the same shape that never occurs in production. This is the failure mode the issue explicitly called out:

The unit test for this validator (validation.spec.ts) passes only because it hand-constructs an object with .source set directly on the inner type — a shape that never occurs in the real data flow which is exactly why this break went unnoticed.

If the unwrap bug regressed in loadAppConfig, this test would still pass — the input array never goes through the unwrap. The test should instead drive the unwrap path (e.g. write a .mcp.json fixture, call loadAllMCPConfigs() or loadAppConfig(), and assert that the resulting mcpServers entries still carry .source === 'project'), and ideally also assert that a logWarning fires. As written it satisfies the file-presence test gate but proves nothing about the regression.

🟠 important · tests · source/config/validation.spec.ts:113

Even on its own terms, the test only asserts t.notThrows(...) against validateProjectConfigSecurity, which is what the pre-existing test already does. It does not assert that a warning is emitted, nor that the warning is emitted only for the project-level server. Capturing logWarning (as the test on line ~60 already does for validateMCPConfigSecurity) and asserting warnings.length === 1 would make the regression coverage meaningful.

---

_{🔴 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.}