feat(mcp): keep project MCP configs with resolved tokens out of git (#882) - #886
Merged
Merged
Conversation
…encent#882) A project-scope MCP config that carries a resolved ${VAR} sat untracked and unignored in the business repo, one `git add -A` from committing the token. After the reconcile writes such a file and git would track it, teamai lists its path in the clone's .git/info/exclude inside a marked block (resolved via `git rev-parse --git-path`, so linked worktrees and submodules work). The committed .gitignore is never touched; an ignored path or a config with no resolved value adds nothing; dry runs write nothing. Project-scope uninstall removes only teamai's block, and doctor reports such a file git would still commit. The hook sits after the appliers in reconcileMcpForConfig, outside desiredMcpForTarget/applyJson/applyCodex, so it merges cleanly with Tencent#880.
…Tencent#882) The plan now records whether the project's .git/info/exclude holds teamai's MCP config block (gitExcludeBlock). It counts toward isPlanEmpty, is listed in the summary and dry run, and gates the removal, so a plan whose only teamai leftover is the block removes it instead of reporting "Nothing to uninstall".
|
Findings
The PR description includes sufficient unit, e2e, and representative real-CLI testing. |
…cent#882) - uninstall keeps a repository's .git/info/exclude block while a config in it could not be parsed and still holds teamai servers, and warns - uninstall finds and removes the block in nested repositories holding an MCP config, across every worktree - the block opens at the last start marker, so an orphaned start never pairs with a later block's end and takes the member's lines - doctor counts only servers the ownership manifest records, not a member's own server under a team name
|
Findings
The four earlier findings are resolved in the current diff. The PR description includes sufficient unit, e2e, and representative real-CLI testing. |
…en clean (Tencent#882) A missing or unreadable managed-mcp.json made the MCP cleanup return early without reporting anything, so uninstall removed the block while .mcp.json still held the resolved token. Uninstall now inspects every path the block protects after the cleanup. The block goes only when each one is missing, or parses and holds none of the team's servers that need a resolved ${VAR}. Anything it cannot check keeps the block, with a warning naming the file. This replaces the leftInPlace report from the reconcile, which the check subsumes.
|
Findings
The four earlier findings are resolved in the current diff. The PR description includes sufficient unit, e2e, and representative real-CLI testing. |
…encent#882) Pull, doctor and uninstall each skipped a case they had not inspected and treated it as safe. Now: - pull lists a config in .git/info/exclude whether or not it delivered to it this run: a disabled or undetected tool's file, a team with automatic delivery off, an unreadable mcp.yaml (any teamai entry counts), a failed write to another tool's config, and a lost ownership manifest (the resolved value found in the file) - doctor checks the same files, including one that does not parse, and counts a git error as a failure - git check-ignore failing inside a repository is no longer read as "not tracked": the path is excluded anyway, or teamai warns with git's error - uninstall also keeps the block while a file contains the value (8+ characters, not a path or the login name) of a variable still set in the environment, which finds a server since dropped from mcp.yaml
|
Findings
The previously reported findings are resolved in the current diff. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
…encent#882) Pull, doctor and uninstall still decided "clean" from the current team config in places. Now one function, resolvedValueEvidence, decides for all three: - a teamai-owned entry still in the file counts when its server has left mcp.yaml, as well as when it needs a resolved ${VAR} or mcp.yaml cannot be read (doctor no longer skips that case) - targets include the built-in location of a tool the team dropped from toolPaths or moved - doctor names a file two tools share once - exclude updates take the existing acquireLock helper, re-read the file and write it atomically, so concurrent commands keep each other's paths - uninstall inspects every worktree of each repository owning a block, including a nested repository's linked worktrees, and applies the manifest rule per worktree - the kept-block warning names each file and why, such as the variable whose value matched
|
Findings
The previously reported findings are resolved in the current diff. The PR description includes sufficient unit, e2e, and representative real-CLI testing. |
6 of 9 tasks
SaulMoro
added a commit
to SaulMoro/teamai-cli
that referenced
this pull request
Sep 28, 2026
…esolved tokens out of git # Conflicts: # src/doctor-delivery.ts # src/mcp-reconcile.ts
…s its lock (Tencent#882) After the 2.5 s wait for the exclude file's lock, updateExclude wrote without it, so two writers could drop each other's pattern and leave a plaintext MCP config committable. It now writes nothing and reports 'locked': pull warns that the file is not excluded yet and to run `teamai pull` again (doctor's exclude check keeps reporting it meanwhile), and uninstall keeps the block and warns.
… free of teamai's servers (Tencent#882) Uninstall judged a protected file clean from the current mcp.yaml, manifest and resolvable values, so with the manifest lost, the server gone from mcp.yaml and its value unset, a plaintext token looked like the member's own server and the exclusion went. It now fails closed and works per entry: a pattern goes only when its file is gone, holds no server, or holds none of teamai's servers with managed-mcp.json still there to say what teamai wrote. A kept entry is named with its file, why, and how to clean it by hand, since a rerun of uninstall finds no config after a full uninstall.
|
Findings
The other earlier findings are resolved. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
SaulMoro
added a commit
to SaulMoro/teamai-cli
that referenced
this pull request
Sep 29, 2026
…proof, skip unlocked writes # Conflicts: # docs/usage-guide.md # docs/usage-guide.zh-CN.md
…lved value into it (Tencent#882) Pull listed the file in .git/info/exclude only after writing the plaintext, and a failed exclusion only warned, so the secret-bearing file stayed eligible for git add -A. The exclusion now comes first; when it cannot be established (exclude file or .git/info not writable, lock held past the wait, file already tracked, git error) the file is left as it was and the warning names the reason and the fix.
…p list and doctor (Tencent#882)
|
Findings
The earlier nested-repository linked-worktree issue and the other previously resolved findings remain fixed. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
SaulMoro
added a commit
to SaulMoro/teamai-cli
that referenced
this pull request
Sep 29, 2026
…ed value Tencent#882's pre-write exclusion is now the single gate for a project MCP config git would commit. Tencent#880's separate tracked-file check (gitTracks in buildDesiredMcpContext, `withheld` in desiredMcpForTarget, its warning, mcp list lines and doctor note) is removed: a tracked file is one more way the exclusion fails, reported once with `git rm --cached <file>` and rotate. A dry run (mcp list, doctor) now also names a tracked file before any pull has listed it, mcp list reports a withheld file with an entry already installed, and doctor reports withheld servers without the pull --force text. A tracked file now gets no resolved value, declared secret or not.
…ready installed (Tencent#882) Backports Tencent#880's merge 0d9f7fa: a dry run (doctor, mcp list) names a tracked file before any pull has listed it, mcp list reports withheld for a server an earlier pull installed, and doctor's withheld note carries the exclusion's own fix instead of the pull --force advice.
…clude (Tencent#882) A tracked file needs `git rm --cached` whatever else is wrong, so ensureExcludedFromGit checks gitTracks before the writability check, on a pull and a dry run alike, and lists nothing for it.
…olds no resolved value (Tencent#882) A pull that lists a config in .git/info/exclude and then writes no value into it (it does not parse, a member's server holds the team's name, the write fails) removes the line it added. After a pull or `teamai mcp remove`, a line whose configs are proven clean in every worktree, by the proof uninstall uses (moved to mcp-reconcile.ts), is removed under the lock; one not proven clean stays. A config listed before its write is listed again after it, so a concurrent uninstall that dropped the line between the check and the write does not leave the value unprotected.
…re the pull rewrote it (Tencent#882) A pull whose manifest was lost before it ran recreates managed-mcp.json while reconciling, so the clean-file proof read the new record and took the exclude line out of a file still holding a teamai server that left mcp.yaml with its variable unset. The proof now uses this worktree's manifest as read before the reconcile.
A line this pull added and took back out, because it wrote no resolved value into the file, was reported as removed although the member never saw it added. Only removing a line an earlier run added stays at info.
|
Findings
The other previously fixed findings remain resolved. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
SaulMoro
added a commit
to SaulMoro/teamai-cli
that referenced
this pull request
Sep 29, 2026
…rst, backport
…holds a server (Tencent#882) A pull or `teamai mcp remove` judged every linked worktree's MCP config with today's definitions and values. Once a server's ${VAR} became a literal, and the value was no longer set, a pull in worktree A took worktree B's stale token-bearing entry for clean and removed the shared /.mcp.json line, so `git add -A` in B staged the token. These commands now release a line only when the current worktree's file passes the full proof and every other worktree's file is missing or holds no MCP server. `teamai uninstall` keeps its full proof in each worktree.
…and command line as credentials, and scope the rebuild's claims by key (Tencent#882)
|
Findings
The previously reported findings appear resolved. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
… have doctor judge a recorded HTTP-team file with no record (Tencent#882)
|
Findings
The other previously reported findings appear resolved. The PR description includes sufficient unit, e2e, and representative real-CLI testing. |
…e Copilot copy, hold a shadowed one, and judge a partly lost HTTP record by its entries (Tencent#882)
|
Findings
The previously reported findings appear resolved. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
…al into on the next sync or pull of an HTTP-backed team (Tencent#882)
… install's credential file (Tencent#882)
…nd call the file's content a credential (Tencent#882)
|
Findings
The previously reported findings appear resolved. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
…amai too: a failed or partial one leaves what to keep out of git (Tencent#882)
|
Review Result
|
Collaborator
|
Please resolve the conflicts |
# Conflicts: # docs/designs/data-directory-layout.md # docs/usage-guide.md # docs/usage-guide.zh-CN.md # skill-data/setup/references/manage-admin.md # src/__tests__/doctor-mcp-delivery.test.ts # src/__tests__/mcp-cmd.test.ts # src/__tests__/mcp-git-exclude.test.ts # src/__tests__/mcp-reconcile.test.ts # src/doctor-delivery.ts # src/mcp-cmd.ts # src/mcp-git-exclude.ts # src/mcp-reconcile.ts
|
Findings
The previously reported findings remain resolved. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
…lds, not by the new install's resolved: false (Tencent#882) install_mcp records the new entry before writing the config. When that write fails, the older entry, credential included, stays in the file while the record says resolved: false. localAgentCredentialFiles now trusts resolved: false only while the entry on disk has the recorded hash; otherwise it checks the entry itself. The doctor fixture that used a placeholder hash now records the hash an install writes.
…another tool owns under mcpServers (Tencent#882) recordedMcpFileEvidence merged a Copilot project file's bare servers with those under mcpServers by name, so Claude owning a nested jira read as owning a bare jira Copilot wrote with a token before it moved. Bare servers now count as owned only by a Copilot record: no other tool writes there.
|
Findings
The two findings from the September 30 review are resolved. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
…efault and a bare Copilot entry apart (Tencent#882) An HTTP team has no teamai.yaml history, so localAgentCredentialFiles never visited a built-in default teamai has since changed: a credential a local agent from before 57636a2 wrote to .codebuddy/mcp.json stayed committable. It now also reads EARLIER_BUILTIN_MCP_PROJECT (earlierMappedMcpTargets with history: false), in sync and doctor alike. It also judged a Copilot project file through installedMcpEntries, which merges bare servers with mcpServers by name, the keyed one winning. A tokenized bare entry beside a credential-free mcpServers entry of its name, both under one record, went unseen. Each entry is now judged on its own (mcpEntriesByPlacement, which recordedMcpFileEvidence uses too).
|
Findings
The previously reported findings appear resolved. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
|
Findings
The previously reported findings are resolved. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
|
Findings
The other previously reported findings appear resolved. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
|
Findings
The previously reported findings appear resolved. The PR description documents sufficient unit, e2e, and representative real-CLI testing. |
|
Findings
The previously reported ownership-loss issue is resolved: failed updates retain the prior manifest record and restore the prior config. The PR description documents sufficient unit, e2e, and representative real-CLI testing. Per instruction, I did not execute tests or PR code. |
jeff-r2026
approved these changes
Sep 30, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A project MCP config that holds a resolved token is kept out of git through the clone's own
.git/info/exclude. Nothing committed changes.Most of this landed with #880: it was stacked on
2033b5behere, and its squash (7c929a99) carries the exclude block, uninstall, doctor's check andmanaged-mcp-files.json. See #880 for that design. Againstmain, this PR is the follow-up fixes after2033b5be. They close three gaps:Type of Change
Test Plan
npx tsc --noEmitpassesnpm run lintpassesnpx vitest runpasses (352 files, 6145 passed, 1 skipped atd548eaa2;CLAUDE_CONFIG_DIRunset, see fix(tests): prevent model tests from overwriting Claude config (P1) #890)9c3333d9and pass after the fix.npm run test:e2e: 391 passed, 26 skippedReal CLI after the merge (
npm run build, temporaryHOME,CLAUDE_CONFIG_DIRunset, project scope,toolPathsputs Copilot and Claude on.mcp.json,jirafor Copilot withBearer ${JIRA_TOKEN},openfor Claude):Real CLI, a moved Copilot (same setup, pre-fix build
7ffd5593against17b1a59d): Copilot on.mcp.jsonwrites a barejirawith the token; the team moves Copilot to.github/mcp.jsonand givesjirato Claude as a literal; token unset, exclude emptied, pull.A second run of the base flow (Claude, project scope) also passed: pull, a second pull, doctor ✔, then
uninstall --forceremoved the token and the block. Real CLI, local agent (hook-dispatch session-startagainst a local fake HTTP backend;install_mcpwith a Bearer header, then the files left as an agent from before this PR left them: exclude emptied, nomanaged-mcp-files.json, noresolvednote; one more sync). Pre-fix build17b1a59dagainst9c3333d9:The local agent's failed-write case (a read-only directory) is covered by its unit test only.
Review verification at
4e71c7a6npx vitest run src/__tests__/mcp-reconcile.test.ts src/__tests__/local-agent-mcp.test.ts: 260 passed. Covers an identical member entry through pull/removal and install/update/uninstall, legacy records without placement evidence, and a failed ownership write leaving the user config visible to Git.npx tsc --noEmit,npm run lint,npm run build: passed.npx vitest run: 352 files, 6087 passed, 1 skipped. The first run alongside other checks had six failures inpush-env.test.ts; its isolated run passed all 19, as did the same file on9c3333d9. Repeating the full suite without concurrent checks passed.copilot-mcp.test.tsandmcp-uninstall.test.ts: 2 passed. The earlier full e2e record above remains from9c3333d9; the full provider/agent suite was not rerun for this follow-up.hook-dispatch session-start --tool copilotagainst a fixture HTTP backend: install, update and uninstall preserve an identical member-owned barejira; a directory atmanaged-mcp.jsonmakes install fail, while the config and exclusion stay unchanged, no sidecar appears, andgit statusstill lists?? .github/mcp.json. Harness:/tmp/882-r6-cli.mjs.Review verification at
1a4ddfa14e71c7a6: pull update, dropping a server, removeAll, local-agent install/update, and local-agent uninstall. A sixth guard covers missing-secret retention. All six pass after the fix, including a repeated pull after a skipped update.npx vitest run: 352 files, 6093 passed, 1 skipped.npx vitest run src/__tests__/mcp-reconcile.test.ts src/__tests__/local-agent-mcp.test.ts: 266 passed.npm run build,npx tsc --noEmit,npm run lint: passed.copilot-mcp.test.tsandmcp-uninstall.test.ts: 2 passed. The earlier full e2e record remains from9c3333d9.hook-dispatch session-start --tool copilotagainst a fixture HTTP backend: initial bare TeamAI install succeeds; the member then adds a same-named keyed server; two update commands fail withnot managed by teamaiand preserve both entries; uninstall succeeds and removes only TeamAI's bare copy. Harness:/tmp/882-r7-cli.mjs.git merge-tree --write-tree HEAD origin/main: no conflicts with128844fe. The existing branch commits all belong to [feat] keep project MCP configs with resolved tokens out of git #882.Review verification at
c0748a9b1a4ddfa1and pass after the fix: legacy bare records and a failed post-write manifest update, each with distinct or identical keyed member content, exercised through pull/update/remove and local-agent install/uninstall; plus compatibility for a legacy keyed record with a matching hash.npx vitest run: 352 files, 6106 passed, 1 skipped.npx vitest run src/__tests__/mcp-reconcile.test.ts src/__tests__/local-agent-mcp.test.ts: 279 passed.copilot-mcp.test.tsandmcp-uninstall.test.ts: 2 passed. The earlier full e2e record remains from9c3333d9.npx tsc --noEmit,npm run lint,npm run build: passed.hook-dispatch session-start --tool copilotagainst a fixture HTTP backend: a legacy unmarked bare record and an injected failure on the second atomic ownership-manifest rename both reject repeated updates and preserve the member's keyed entry on uninstall. The failure case uses identical bare/keyed content. A genuine legacy keyed entry with a unique matching hash still updates, recordsbare: false, and uninstalls. Harness:/tmp/882-r8-cli.mjs, fault injection:/tmp/882-r8-fault.mjs.git merge-tree --write-tree HEAD origin/main: no conflicts with128844fe. Every branch commit belongs to [feat] keep project MCP configs with resolved tokens out of git #882.Latest review verification at
d548eaa2npx vitest run: 352 files, 6145 passed, 1 skipped. MCP targeted suite: 420 passed; final local-agent suite after snapshot simplification: 76 passed.npx tsc --noEmit,npm run lint,npm run build: passed.copilot-mcp.test.tsandmcp-uninstall.test.ts: 2 passed. The earlier full e2e record remains from9c3333d9.hook-dispatch session-start --tool copilot, isolated HOME/Git workspace, fixture HTTP backend. Tested a legacy bare update, a proven bare-to-keyed update and uninstall. For each, injected an atomic config-rename failure, then a post-config ownership-manifest rename failure. Both failures reported failed, preserved the exact prior config/ownership, and left the credential file absent from Git status. Retry and subsequent uninstall succeeded; the old token was removed and the member's keyed server survived. Harness:/tmp/882-r9-cli.mjs; rename-fault preload:/tmp/882-r9-fault.mjs.git merge-tree --write-tree HEAD origin/main: no conflicts with128844fe; no unrelated branch commits.Related Issues
Closes #882. Found while implementing #879 / #880.
Notes for Reviewers
Door: two-way. Only writes a marked block in a local, uncommitted file; uninstall removes it.
Blast Radius: MCP configs. HTTP-backed teams using local-agent install/uninstall, JSON/TOML reconcile failure recovery, and Copilot project configs shared with another tool.
${VAR}, so there is no definition to judge against. Only a bare stdio command is treated as credential-free.bare: true), completed keyed writes (bare: false), and unknown placement. An unknown record needs a matching hash in the keyed entry and no equal hash in the bare entry; an identical duplicate is ambiguous and left alone.mainused2033b5beas its base. One real conflict came out of it, inapplyJsonTarget: feat(env): team-declared secrets with member-local values (#875) #880'sholdsResolvedValueand this branch's bare-copy removal are both kept.Known limits
teamai uninstalltakes the line out: no pull orteamai mcp removereleases it. An older install's file is listed only in the current workspace; other workspaces are covered at their next session.mcpServers, every other top-level object reads as a bare server. An object-valued setting there reads as a server no record claims, which keeps the line.