diff --git a/docs/designs/team-secrets.md b/docs/designs/team-secrets.md index 7e0f08e3..2c76e239 100644 --- a/docs/designs/team-secrets.md +++ b/docs/designs/team-secrets.md @@ -144,7 +144,7 @@ the member's value for this team teamai env set KEY [--from-env VAR] **Still reachable.** The resolved value is written in plaintext to each tool's MCP config, as before. A config that holds a resolved `${VAR}` value is written `0600`, an existing wider one (`.mcp.json` is often `0644`) included, and a pull that changes nothing in it still tightens it to `0600` without rewriting it; one without such a value keeps its mode, and a new one is created `0600`. A command run under `env exec` gets it in its environment, and so does every process it starts: an agent that runs `teamai env exec -- env` can read it. The agent skills forbid that, but nothing enforces it. This keeps secrets out of git, not away from the member's machine or the agent running on it. -**Out of git.** A project-scope MCP config that holds a resolved value is listed in the clone's `.git/info/exclude` (#882), which stops `git add` but not a file git already tracks. So a declared secret's value is never written into a project config `git ls-files` tracks: that server is skipped for that tool, an entry an earlier pull wrote there stays as it is, and `pull` (a warning), `teamai mcp list` (`withheld:`) and `teamai doctor` (`MCP servers delivered to ` fails) name the file and the fix: `git rm --cached `, and rotate the token if it was ever committed. A variable the team does not declare as a secret is written as before. A repository git cannot answer for is not treated as tracking the file: a commit fails there too. +**Out of git.** A resolved value lands in a project-scope MCP config only once the clone's `.git/info/exclude` lists the file (#882). An exclude rule does not stop a file git already tracks, so no resolved value, declared secret or not, is written into a project config `git ls-files` tracks: pull leaves that file as it was (an entry an earlier pull wrote stays), and `pull` (a warning), `teamai mcp list` (`withheld:`) and `teamai doctor` (`MCP servers delivered to ` fails) name the file and the fix: `git rm --cached `, and rotate the token if it was ever committed. An exclusion that fails for another reason (`.git/info` or the exclude file not writable, another teamai command holding it, a git error) leaves the file as it was the same way, with that reason and its fix. ## A missing secret keeps the MCP entry diff --git a/docs/designs/team-secrets.zh-CN.md b/docs/designs/team-secrets.zh-CN.md index 8cdd15ea..3173393a 100644 --- a/docs/designs/team-secrets.zh-CN.md +++ b/docs/designs/team-secrets.zh-CN.md @@ -144,7 +144,7 @@ teamai env unset GITHUB_TOKEN [--global] **仍可访问。** 解析后的值仍以明文写入各工具的 MCP 配置。含有已解析 `${VAR}` 值的配置以 `0600` 写入,已有的更宽权限文件(`.mcp.json` 常为 `0644`)也会收紧,即使 pull 没有改动其中任何内容,也会在不重写文件的情况下收紧为 `0600`;不含这类值的配置保持原权限,新文件以 `0600` 创建。在 `env exec` 下运行的命令会在环境变量中拿到它,它启动的每个进程也一样:agent 运行 `teamai env exec -- env` 就能读到。agent skill 禁止这样做,但没有任何机制强制。这让密钥不进入 git,而不是让它远离成员的机器或在上面运行的 agent。 -**不进入 git。** 含有已解析值的项目级 MCP 配置会被写入本地克隆的 `.git/info/exclude`(#882),这能阻止 `git add`,但挡不住 git 已跟踪的文件。因此已声明密钥的值永远不会写入 `git ls-files` 已跟踪的项目配置:该工具跳过这个 server,之前 pull 写入的条目保持不变,`pull`(警告)、`teamai mcp list`(`withheld:`)和 `teamai doctor`(`MCP servers delivered to ` 失败)会指出该文件和修复方法:`git rm --cached `,如果它曾随 token 一起提交过,还要轮换 token。团队未声明为密钥的变量照旧写入。git 无法判断的仓库不视为跟踪了该文件:在那里提交同样会失败。 +**不进入 git。** 只有在本地克隆的 `.git/info/exclude` 列出某个项目级 MCP 配置之后,解析后的值才会写入该文件(#882)。exclude 规则挡不住 git 已跟踪的文件,因此无论是否为已声明密钥,解析后的值都不会写入 `git ls-files` 已跟踪的项目配置:pull 保持该文件原样(之前 pull 写入的条目保留),`pull`(警告)、`teamai mcp list`(`withheld:`)和 `teamai doctor`(`MCP servers delivered to ` 失败)会指出该文件和修复方法:`git rm --cached `,如果它曾随 token 一起提交过,还要轮换 token。因其他原因无法排除时(`.git/info` 或 exclude 文件不可写、另一个 teamai 命令占用它、git 出错),同样保持该文件原样,并给出对应的原因与修复方法。 ## 缺少密钥时保留 MCP 条目 diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 11a7881f..b5693dcd 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -1244,7 +1244,7 @@ Copilot uses its native `mcpServers` schema: `stdio` becomes `type: "local"`, re teamai **resolves every `${VAR}` to its value and writes it verbatim** into each tool's config, which is then written `0600`, an existing `0644` one included (a config without a resolved value keeps its mode; new files are created `0600`). It does not rely on any tool's own env-var expansion: that expansion is fragile — most decisively, IDEs launched from the GUI (Dock/Launchpad) never inherit your shell's exported variables, so a `${VAR}` placeholder expands to empty and the server 401s. Resolving to plaintext makes the token present no matter how the tool is started. -> ⚠️ **The resolved token lands on disk.** Project-scope MCP configs (`.mcp.json`, `.github/mcp.json`, `.cursor/mcp.json`, `.codex/config.toml`, `opencode.json`) then contain the literal secret. Whenever such a file holds a value teamai resolved and git would track it, teamai lists the path in the clone's `.git/info/exclude`, inside a `# [teamai:mcp-exclude:start]` block (the worktrees of a repo share it). That covers a file this pull did not write: one written earlier for a tool since disabled, or dropped from `toolPaths` (at the tool's built-in location), or one still holding a server since removed from `mcp.yaml`. A path git cannot answer for is listed all the same, or teamai warns; so does a pull that finds another teamai command holding the exclude file past a short wait, which writes nothing (run `teamai pull` again). The committed `.gitignore` is left alone, a path git already ignores adds nothing, and `teamai uninstall` removes a path from the block (the block with its last path) once that file, in every worktree of its repository, is gone, holds no MCP server, or holds none of: a team server with a resolved value, an entry of teamai's that cleanup left, or the value (8+ characters) of a variable still set in the environment, with teamai's record of what it wrote there (`managed-mcp.json`) still present. Otherwise, or for a file it cannot check (for example one that does not parse), it keeps the path and warns, naming the file and why: remove teamai's servers from it, then delete that line yourself (with its last line, the block's markers). `teamai doctor` reports such a file git would still commit or cannot answer for — for example one already tracked: `git rm --cached` it and rotate the token. A config git already tracks never gets a declared secret's value (`env/secrets.yaml`): teamai skips that server for that tool, keeps the entry an earlier pull wrote as it is, and `pull`, `teamai mcp list` and `teamai doctor` name the file and the fix (`git rm --cached `, then rotate the token). +> ⚠️ **The resolved token lands on disk.** Project-scope MCP configs (`.mcp.json`, `.github/mcp.json`, `.cursor/mcp.json`, `.codex/config.toml`, `opencode.json`) then contain the literal secret. Whenever such a file would hold a value teamai resolved and git would track it, teamai lists the path in the clone's `.git/info/exclude`, inside a `# [teamai:mcp-exclude:start]` block (the worktrees of a repo share it), before it writes the value. That covers a file this pull did not write: one written earlier for a tool since disabled, or dropped from `toolPaths` (at the tool's built-in location), or one still holding a server since removed from `mcp.yaml`. A path git cannot answer for is listed all the same. When it cannot — `.git/info` or the exclude file is not writable, another teamai command holds the exclude file past a short wait, git already tracks the file, or git fails — it leaves that file as it was (an entry an earlier pull wrote stays), warns with the reason and the fix, and `teamai mcp list` and `teamai doctor` report the server as withheld; make the file writable (or `git rm --cached` the tracked file and rotate the token) and run `teamai pull` again. The committed `.gitignore` is left alone, a path git already ignores adds nothing, and `teamai uninstall` removes a path from the block (the block with its last path) once that file, in every worktree of its repository, is gone, holds no MCP server, or holds none of: a team server with a resolved value, an entry of teamai's that cleanup left, or the value (8+ characters) of a variable still set in the environment, with teamai's record of what it wrote there (`managed-mcp.json`) still present. Otherwise, or for a file it cannot check (for example one that does not parse), it keeps the path and warns, naming the file and why: remove teamai's servers from it, then delete that line yourself (with its last line, the block's markers). `teamai doctor` reports such a file git would still commit or cannot answer for — for example one already tracked: `git rm --cached` it and rotate the token. Claude Code may show project `.mcp.json` servers as pending approval until you accept them once in an interactive session. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index e98260dc..329a8063 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -1126,7 +1126,7 @@ Copilot 使用原生 `mcpServers` 结构:`stdio` 写成 `type: "local"`,远 teamai 会**把每个 `${VAR}` 解析成取值后原样写入**各工具的配置文件,并以 `0600` 写入该文件,已有的 `0644` 文件也会收紧(不含已解析值的配置保持原权限;新建文件权限为 `0600`)。它不依赖任何工具自身的环境变量展开——因为那种展开很脆弱:最典型的是,以 GUI 方式(Dock/Launchpad)启动的 IDE 不会继承你 shell 中 `export` 的变量,`${VAR}` 占位符会展开为空、导致服务端 401。解析成明文可以保证无论工具如何启动,token 都在。 -> ⚠️ **解析后的 token 会落盘。** 项目级 MCP 配置(`.mcp.json`、`.github/mcp.json`、`.cursor/mcp.json`、`.codex/config.toml`、`opencode.json`)因此含有明文密钥。只要这类文件含有 teamai 解析出的值且 git 会跟踪它,teamai 就会把路径写入本地克隆的 `.git/info/exclude`,放在 `# [teamai:mcp-exclude:start]` 块中(同一仓库的各 worktree 共用该文件)。本次 pull 未写入的文件同样适用:之前为某个现已禁用、或已从 `toolPaths` 移除的工具写入的文件(按该工具的内置位置查找),或仍含已从 `mcp.yaml` 删除的 server 的文件。git 无法判断的路径也会照样写入,否则 teamai 会给出警告;若 pull 在短暂等待后仍发现另一个 teamai 命令占用该 exclude 文件,则不写入并给出警告(请再次运行 `teamai pull`)。不会改动已提交的 `.gitignore`,git 已忽略的路径不会重复添加,`teamai uninstall` 会从块中移除某个路径(移除最后一个路径时连同整个块),前提是该文件(在其仓库的每个 worktree 中)已不存在、不含任何 MCP server,或在 teamai 的写入记录(`managed-mcp.json`)仍在的情况下不含以下任何一项:带解析值的团队 server、清理后仍残留的 teamai 条目、仍在环境中设置的变量的值(8 个字符以上)。否则,或对无法检查的文件(例如无法解析),会保留该路径并给出警告,说明文件及原因:请先从中移除 teamai 的 server,再自行删除那一行(删到最后一行时连同块的首尾标记)。`teamai doctor` 会报告 git 仍会提交或无法判断的这类文件——例如已被跟踪的文件:请 `git rm --cached` 并轮换 token。git 已跟踪的配置永远不会写入已声明密钥(`env/secrets.yaml`)的值:teamai 对该工具跳过这个 server,保留之前 pull 写入的条目不变,`pull`、`teamai mcp list` 和 `teamai doctor` 会指出该文件和修复方法(`git rm --cached `,然后轮换 token)。 +> ⚠️ **解析后的 token 会落盘。** 项目级 MCP 配置(`.mcp.json`、`.github/mcp.json`、`.cursor/mcp.json`、`.codex/config.toml`、`opencode.json`)因此含有明文密钥。只要这类文件将含有 teamai 解析出的值且 git 会跟踪它,teamai 就会在写入该值之前把路径写入本地克隆的 `.git/info/exclude`,放在 `# [teamai:mcp-exclude:start]` 块中(同一仓库的各 worktree 共用该文件)。本次 pull 未写入的文件同样适用:之前为某个现已禁用、或已从 `toolPaths` 移除的工具写入的文件(按该工具的内置位置查找),或仍含已从 `mcp.yaml` 删除的 server 的文件。git 无法判断的路径也会照样写入。若无法写入——`.git/info` 或 exclude 文件不可写、另一个 teamai 命令在短暂等待后仍占用 exclude 文件、git 已跟踪该文件,或 git 出错——teamai 会保持该文件原样(之前 pull 写入的条目保留),给出原因与修复方法的警告,`teamai mcp list` 和 `teamai doctor` 也会把该 server 报告为未写入(withheld);请让文件可写(或对已跟踪的文件执行 `git rm --cached` 并轮换 token),再运行 `teamai pull`。不会改动已提交的 `.gitignore`,git 已忽略的路径不会重复添加,`teamai uninstall` 会从块中移除某个路径(移除最后一个路径时连同整个块),前提是该文件(在其仓库的每个 worktree 中)已不存在、不含任何 MCP server,或在 teamai 的写入记录(`managed-mcp.json`)仍在的情况下不含以下任何一项:带解析值的团队 server、清理后仍残留的 teamai 条目、仍在环境中设置的变量的值(8 个字符以上)。否则,或对无法检查的文件(例如无法解析),会保留该路径并给出警告,说明文件及原因:请先从中移除 teamai 的 server,再自行删除那一行(删到最后一行时连同块的首尾标记)。`teamai doctor` 会报告 git 仍会提交或无法判断的这类文件——例如已被跟踪的文件:请 `git rm --cached` 并轮换 token。 Claude Code 可能把来自仓库的 `.mcp.json` 标为待批准,需在交互式会话中确认一次。 diff --git a/skill-data/core/references/troubleshooting.md b/skill-data/core/references/troubleshooting.md index 1bece545..6fb169cc 100644 --- a/skill-data/core/references/troubleshooting.md +++ b/skill-data/core/references/troubleshooting.md @@ -82,14 +82,17 @@ reference to VAR (`--from-env`) and VAR is unset in this environment. Ask the user whether to set VAR in their shell or replace the reference with the command in the line; do not choose for them. -## "MCP server X not written: is tracked by git" +## "Did not write 's MCP servers to " / `withheld:` -`pull`, `teamai mcp list` (`withheld:`) and `teamai doctor` print this when a -project MCP config git already tracks would get a declared secret's value. -That server is skipped for that tool, and an entry an earlier pull wrote -stays as it is. Tell the user: `git rm --cached ` (the file stays on -disk), commit that, and rotate the token if the file was ever committed with -it; then `teamai pull`. Do not run `git rm` or commit for them. +`pull` prints this, and `teamai mcp list` (`withheld:`) and `teamai doctor` +report it, when a project MCP config would get a resolved `${VAR}` value that +git would commit: the file could not be kept out of git first. It is left as +it was, and an entry an earlier pull wrote stays. The line names the reason and the +fix. For `git already tracks `, tell the user: `git rm --cached ` +(the file stays on disk), commit that, and rotate the token if the file was +ever committed with it; then `teamai pull`. Do not run `git rm` or commit for +them. For an exclude file that is not writable, one another teamai command +held, or a git error, relay the fix the line gives. ## Permission / access denied diff --git a/src/__tests__/doctor-mcp-delivery.test.ts b/src/__tests__/doctor-mcp-delivery.test.ts index 5ad0d1a4..48727e85 100644 --- a/src/__tests__/doctor-mcp-delivery.test.ts +++ b/src/__tests__/doctor-mcp-delivery.test.ts @@ -344,22 +344,39 @@ describe('doctor — MCP servers delivered on disk', () => { expect(await excludeCheck()).toBeUndefined(); }); + it.skipIf(process.getuid?.() === 0)('says a server was withheld because its file cannot be kept out of git, and the fix', async () => { + await fse.remove(path.join(projectRoot, '.mcp.json')); + vi.stubEnv('JIRA_TOKEN', 'long-t0ken-value-7c1'); + const excludeFile = path.join(projectRoot, '.git', 'info', 'exclude'); + await fse.chmod(excludeFile, 0o444); + + try { + const check = await mcpCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toMatch(/\.git\/info\/exclude is not writable/); + expect(check.fix).toContain('teamai pull'); + expect(check.fix).not.toContain('again..'); + } finally { + await fse.chmod(excludeFile, 0o644); + } + }); + it('emits no check when the installed servers carry no resolved value', async () => { await writeTeamMcp('servers:\n - name: jira\n transport: http\n url: https://jira.example/mcp\n'); expect(await excludeCheck()).toBeUndefined(); }); - it('fails the delivery check for a declared secret withheld from a file git tracks, naming the file and the fix (#879)', async () => { - await fse.outputFile(path.join(repoPath, 'env', 'secrets.yaml'), 'secrets:\n - key: JIRA_TOKEN\n'); + it('fails the delivery check for a server withheld from a file git tracks, naming the file and the fix once (#879)', async () => { vi.stubEnv('JIRA_TOKEN', 'fixture-jira-token'); execFileSync('git', ['add', '.mcp.json'], { cwd: projectRoot }); const check = await mcpCheck(); expect(await check.check()).toBe(false); const file = path.join(projectRoot, '.mcp.json'); - expect(check.fix).toContain(`jira not written: ${file} is tracked by git, so the value of JIRA_TOKEN would be committed.`); - expect(check.fix).toContain(`git rm --cached ${file}`); + expect(check.fix).toContain(`In ${file}, withheld: jira, as git would commit the file: git already tracks ${file}.`); + expect(check.fix).toContain(`git rm --cached ${file}\` (rotate any value a commit of it holds)`); + expect(check.fix).not.toContain('not the team\'s definition'); expect(check.fix).not.toContain('pull --force'); }); }); diff --git a/src/__tests__/env-advisories.test.ts b/src/__tests__/env-advisories.test.ts index 350f6c40..22094a5f 100644 --- a/src/__tests__/env-advisories.test.ts +++ b/src/__tests__/env-advisories.test.ts @@ -327,8 +327,8 @@ describe('a missing declared secret tells the member what to run', () => { await mcpList({}); const out = spy.mock.calls.map(([line]) => String(line)).join('\n'); const file = path.join(projectRoot, '.mcp.json'); - expect(out).toContain(`withheld: claude — ${file} is tracked by git, so the value of GITHUB_TOKEN would be committed.`); - expect(out).toContain(`git rm --cached ${file}`); + expect(out).toContain(`withheld: claude — git already tracks ${file}. Run \`git rm --cached ${file}\` (rotate any value a commit of it holds)`); + expect(out.match(/withheld:/g)).toHaveLength(1); } finally { spy.mockRestore(); } diff --git a/src/__tests__/mcp-cmd.test.ts b/src/__tests__/mcp-cmd.test.ts index bdc16dd7..97903d70 100644 --- a/src/__tests__/mcp-cmd.test.ts +++ b/src/__tests__/mcp-cmd.test.ts @@ -11,8 +11,11 @@ vi.mock('../namespaced-entries.js', async (importOriginal) => ({ vi.mock('../mcp-reconcile.js', () => ({ reconcileMcpForConfig: vi.fn(), resolveMcpTargets: vi.fn().mockResolvedValue([]), - buildDesiredMcpContext: vi.fn().mockResolvedValue({ vars: {} }), - desiredMcpForTarget: vi.fn(), + buildVarTable: vi.fn().mockResolvedValue({}), +})); +vi.mock('../mcp-git-exclude.js', async (importOriginal) => ({ + ...(await importOriginal()), + ensureExcludedFromGit: vi.fn(), })); vi.mock('../utils/fs.js', () => ({ readJson: vi.fn().mockResolvedValue(null), @@ -26,7 +29,8 @@ vi.mock('../utils/logger.js', () => ({ import { autoDetectInit } from '../config.js'; import { entryLayout, resolveEntriesFor } from '../namespaced-entries.js'; import { mcpInject, mcpList } from '../mcp-cmd.js'; -import { reconcileMcpForConfig } from '../mcp-reconcile.js'; +import { reconcileMcpForConfig, resolveMcpTargets } from '../mcp-reconcile.js'; +import { ensureExcludedFromGit } from '../mcp-git-exclude.js'; const mockedAutoDetectInit = autoDetectInit as Mock; const mockedResolve = resolveEntriesFor as Mock; @@ -90,6 +94,25 @@ describe('mcpList', () => { expect(text.match(/roles:/g)).toHaveLength(1); }); + it('says where a server needing a resolved value is withheld because git would commit the file, and the fix (#882)', async () => { + mockedResolve.mockResolvedValue(resolved([ + [{ name: 'jira', transport: 'http', url: 'https://jira.example/mcp', headers: { Authorization: 'Bearer ${JIRA_TOKEN}' } }, 'mcp/mcp.yaml', null], + ])); + (resolveMcpTargets as Mock).mockResolvedValueOnce([ + { tool: 'claude', format: 'claude', file: '/work/app/.mcp.json', projectScope: true }, + ]); + (ensureExcludedFromGit as Mock).mockResolvedValueOnce({ + kind: 'failed', + reason: '/work/app/.git/info/exclude is not writable', + fix: 'Make it writable, then run `teamai pull` again.', + }); + + const text = await listOutput(); + + expect(ensureExcludedFromGit).toHaveBeenCalledWith('/work/app/.mcp.json', { dryRun: true }); + expect(text).toContain('withheld: claude — /work/app/.git/info/exclude is not writable. Make it writable, then run `teamai pull` again.'); + }); + it('reports a set that cannot be resolved instead of listing part of it', async () => { mockedResolve.mockResolvedValue({ kind: 'failed', diff --git a/src/__tests__/mcp-reconcile.test.ts b/src/__tests__/mcp-reconcile.test.ts index 6d5a85b2..287bd0d1 100644 --- a/src/__tests__/mcp-reconcile.test.ts +++ b/src/__tests__/mcp-reconcile.test.ts @@ -21,7 +21,25 @@ vi.mock('../utils/logger.js', () => ({ })), })); +// What .git/info/exclude held at the moment each JSON config was written (#882). +const excludeAtWrite = vi.hoisted(() => new Map()); +vi.mock('../utils/fs.js', async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + writeJsonAtomic: async (...args: Parameters) => { + const [file] = args; + const gitDir = path.join(path.dirname(String(file)), '.git'); + if (await fse.pathExists(gitDir)) { + excludeAtWrite.set(String(file), await actual.readFileSafe(path.join(gitDir, 'info', 'exclude'))); + } + return actual.writeJsonAtomic(...args); + }, + }; +}); + import { reconcileMcpForConfig, resolveMcpTargets, spliceCodexBlock, codexServerNames, writeCodexAtomic } from '../mcp-reconcile.js'; +import { acquireLock, releaseLock } from '../update.js'; import { log } from '../utils/logger.js'; import { resetWarnOnce } from '../utils/warn-once.js'; import { TeamaiConfigSchema, type TeamaiConfig, type LocalConfig } from '../types.js'; @@ -1015,54 +1033,116 @@ servers: }); }); - describe('a config git already tracks never gets a declared secret (#879)', () => { - const claudeFile = (): string => path.join(projectRoot, '.mcp.json'); + it('lists the config in .git/info/exclude before writing the value into it', async () => { + await writeMcpYaml(withSecret); - beforeEach(async () => { - await fse.outputFile(path.join(repoPath, 'env', 'secrets.yaml'), 'secrets:\n - key: SECRET_TOKEN\n'); - await writeMcpYaml(withSecret); + await reconcileMcpForConfig(teamConfig, projectConfig); + + expect(excludeAtWrite.get(path.join(projectRoot, '.mcp.json'))).toMatch(/^\/\.mcp\.json$/m); + expect(await fse.readFile(path.join(projectRoot, '.mcp.json'), 'utf-8')).toContain('super-secret-value'); + }); + + describe('when the config cannot be kept out of git first', () => { + const mcpJson = (): string => path.join(projectRoot, '.mcp.json'); + const infoDir = (): string => path.join(projectRoot, '.git', 'info'); + const claudeOnly = (): LocalConfig => ({ ...projectConfig, disabledAgents: ['cursor'] } as LocalConfig); + + beforeEach(() => { vi.mocked(log.warn).mockClear(); }); - it('skips the server there, names the file and the fix, and still writes and excludes an untracked config', async () => { - await fse.writeJson(claudeFile(), { mcpServers: {} }); + afterEach(async () => { + await fse.chmod(infoDir(), 0o755); + await fse.chmod(path.join(infoDir(), 'exclude'), 0o644); + }); + + it.skipIf(process.getuid?.() === 0).each([ + ['.git/info/exclude is read-only', () => fse.chmod(path.join(infoDir(), 'exclude'), 0o444)], + ['.git/info is read-only', () => fse.chmod(infoDir(), 0o555)], + ])('writes no value when %s, and warns with the fix', async (_label, lockDown) => { + await writeMcpYaml(withSecret); + await lockDown(); + + const { changes } = await reconcileMcpForConfig(teamConfig, claudeOnly()); + + expect(await fse.pathExists(mcpJson())).toBe(false); + expect(changes).toContainEqual(expect.objectContaining({ tool: 'claude', server: 'with-secret', action: 'skipped' })); + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining(mcpJson())); + expect(log.warn).toHaveBeenCalledWith(expect.stringMatching(/not writable[\s\S]*teamai pull/)); + }); + + it.skipIf(process.getuid?.() === 0)('keeps an earlier entry as it was', async () => { + await writeMcpYaml(withSecret); + await reconcileMcpForConfig(teamConfig, claudeOnly()); + await fse.writeFile(path.join(infoDir(), 'exclude'), ''); + const before = await fse.readFile(mcpJson(), 'utf-8'); + await writeMcpYaml(withSecret.replace('https://example.com/mcp', 'https://example.com/v2')); + vi.stubEnv('SECRET_TOKEN', 'rotated-secret-value'); + await fse.chmod(infoDir(), 0o555); + + await reconcileMcpForConfig(teamConfig, claudeOnly()); + + expect(await fse.readFile(mcpJson(), 'utf-8')).toBe(before); + }); + + it('writes no value while another command holds the exclude file\'s lock', async () => { + const lock = path.join(infoDir(), 'exclude.teamai-lock'); + expect(await acquireLock(lock)).toBe(true); + await writeMcpYaml(withSecret); + + try { + await reconcileMcpForConfig(teamConfig, claudeOnly()); + } finally { + await releaseLock(lock); + } + + expect(await fse.pathExists(mcpJson())).toBe(false); + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining(mcpJson())); + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('teamai pull')); + }); + + it('writes no value into a file git already tracks, names the fix once, and still writes an untracked config', async () => { + await fse.writeJson(mcpJson(), { mcpServers: {} }); git(projectRoot, 'add', '.mcp.json'); + await writeMcpYaml(withSecret); const { changes } = await reconcileMcpForConfig(teamConfig, projectConfig); - expect(await fse.readFile(claudeFile(), 'utf-8')).not.toContain('super-secret-value'); + expect(await fse.readFile(mcpJson(), 'utf-8')).not.toContain('super-secret-value'); expect(changes).toContainEqual(expect.objectContaining({ - tool: 'claude', server: 'with-secret', action: 'skipped', reason: expect.stringContaining(`git rm --cached ${claudeFile()}`), + tool: 'claude', server: 'with-secret', action: 'skipped', reason: expect.stringContaining(`git already tracks ${mcpJson()}`), })); - expect(log.warn).toHaveBeenCalledWith(expect.stringMatching(/tracked by git.*git rm --cached .*rotate/)); + const warnings = vi.mocked(log.warn).mock.calls.map(([line]) => String(line)).filter((line) => line.includes(mcpJson())); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toMatch(new RegExp(`git already tracks[\\s\\S]*git rm --cached ${mcpJson()}[\\s\\S]*rotate`)); expect((await fse.readJson(path.join(projectRoot, '.cursor', 'mcp.json'))).mcpServers['with-secret'].headers.Authorization) .toBe('Bearer super-secret-value'); expect(await excludeOf(projectRoot)).toMatch(/^\/\.cursor\/mcp\.json$/m); }); - it('keeps the entry an earlier pull wrote as it is, and updates it once the file is untracked', async () => { - await reconcileMcpForConfig(teamConfig, projectConfig); + it('keeps the entry an earlier pull wrote to a file git now tracks, and updates it once the file is untracked (#879)', async () => { + await writeMcpYaml(withSecret); + await reconcileMcpForConfig(teamConfig, claudeOnly()); git(projectRoot, 'add', '-f', '.mcp.json'); - const before = await fse.readFile(claudeFile(), 'utf-8'); + const before = await fse.readFile(mcpJson(), 'utf-8'); vi.stubEnv('SECRET_TOKEN', 'rotated-secret-value'); - await reconcileMcpForConfig(teamConfig, projectConfig); - expect(await fse.readFile(claudeFile(), 'utf-8')).toBe(before); + await reconcileMcpForConfig(teamConfig, claudeOnly()); + expect(await fse.readFile(mcpJson(), 'utf-8')).toBe(before); git(projectRoot, 'rm', '-q', '--cached', '.mcp.json'); - const { changes } = await reconcileMcpForConfig(teamConfig, projectConfig); + const { changes } = await reconcileMcpForConfig(teamConfig, claudeOnly()); expect(changes).toContainEqual({ tool: 'claude', server: 'with-secret', action: 'updated' }); - expect(await fse.readFile(claudeFile(), 'utf-8')).toContain('rotated-secret-value'); + expect(await fse.readFile(mcpJson(), 'utf-8')).toContain('rotated-secret-value'); }); - it('writes a variable the team does not declare as a secret, as before', async () => { - await fse.remove(path.join(repoPath, 'env', 'secrets.yaml')); - await fse.writeJson(claudeFile(), { mcpServers: {} }); - git(projectRoot, 'add', '.mcp.json'); + it('still writes a config that carries no resolved value', async () => { + await fse.chmod(infoDir(), 0o555); + await writeMcpYaml('servers:\n - name: open\n transport: http\n url: https://example.com/open\n'); - await reconcileMcpForConfig(teamConfig, projectConfig); + await reconcileMcpForConfig(teamConfig, claudeOnly()); - expect(await fse.readFile(claudeFile(), 'utf-8')).toContain('super-secret-value'); + expect(await fse.readFile(mcpJson(), 'utf-8')).toContain('https://example.com/open'); }); }); diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts index d480f912..e36206e4 100644 --- a/src/doctor-delivery.ts +++ b/src/doctor-delivery.ts @@ -430,6 +430,7 @@ export async function buildMcpDeliveryChecks(ctx: DoctorContext): Promise !excludedByUser.has(change.server) && !kept.has(change.server) && !withheld.has(change.server)) + .filter((change) => !excludedByUser.has(change.server) && !kept.has(change.server)) .map((change) => `${change.server} (${change.reason ?? 'skipped'})`); - // Its reason says what fixes it (#879). - const withheldNotes = skipped.filter((change) => withheld.has(change.server)).map((change) => `${change.server} not written: ${change.reason}.`); const problems: string[] = []; + // Its fix is the exclusion's own, not another pull (#882). + let withheld: string | undefined; const installed = await installedMcpEntries(target); if (installed === null) { problems.push(`${target.file} could not be parsed, so no server was injected`); @@ -481,12 +482,21 @@ export async function buildMcpDeliveryChecks(ctx: DoctorContext): Promise 0) problems.push(`not injected: ${nameList(absent)}`); - if (foreign.length > 0) problems.push(`not the team's definition: ${nameList(foreign)}`); + // Pull writes a resolved value only into a file git leaves out of a + // commit (#882), and otherwise leaves the whole file as it was. + const exclusion = carriesResolvedValue(target, teamDefs, [...absent, ...foreign]) + ? await ensureExcludedFromGit(target.file, { dryRun: true }) + : undefined; + if (exclusion?.kind === 'failed') { + withheld = `In ${target.file}, withheld: ${nameList([...absent, ...foreign])}, as git would commit the file: ${exclusion.reason}. ${exclusion.fix}`; + } else { + if (absent.length > 0) problems.push(`not injected: ${nameList(absent)}`); + if (foreign.length > 0) problems.push(`not the team's definition: ${nameList(foreign)}`); + } } if (blocked.length > 0) problems.push(`skipped: ${nameList(blocked)}`); - if (problems.length === 0 && withheldNotes.length === 0 && desired.size === 0) continue; + if (problems.length === 0 && !withheld && desired.size === 0) continue; const delivery = problems.length === 0 ? [] : [`In ${target.file}, ${problems.join('; ')}. A server needing a variable reads it from ` + '`env/env.yaml` or an active `env//env.yaml`, whose top-level key is `variables:` — a plain `KEY: value` mapping ' @@ -496,8 +506,8 @@ export async function buildMcpDeliveryChecks(ctx: DoctorContext): Promise problems.length === 0 && withheldNotes.length === 0, - fix: [...withheldNotes, ...delivery].join(' '), + check: async () => problems.length === 0 && !withheld, + fix: [...withheld ? [withheld] : [], ...delivery].join(' '), }); } diff --git a/src/mcp-cmd.ts b/src/mcp-cmd.ts index 5e155fb4..f7e9bf6b 100644 --- a/src/mcp-cmd.ts +++ b/src/mcp-cmd.ts @@ -5,13 +5,14 @@ import { describeEntryFailure, describeOrigin, resolveEntriesFor } from './names import { reconcileMcpForConfig, resolveMcpTargets, - buildDesiredMcpContext, - desiredMcpForTarget, + buildVarTable, type McpChange, + type McpTarget, } from './mcp-reconcile.js'; import { referencedVars } from './resources/mcp-format.js'; import { reportMissingSecrets } from './env-advisories.js'; import { resolveTeamEnv } from './env-resolution.js'; +import { carriesResolvedValue, ensureExcludedFromGit } from './mcp-git-exclude.js'; import { log } from './utils/logger.js'; import type { GlobalOptions } from './types.js'; import { managedMcpManifestPath, managedMcpManifestKey, getDataHome } from './types.js'; @@ -53,16 +54,7 @@ export async function mcpList(_options: GlobalOptions): Promise { } const targets = await resolveMcpTargets(teamConfig, localConfig); - const context = await buildDesiredMcpContext(teamConfig, localConfig, { teamEnv }, targets); - const { vars } = context; - // Why each server is not written to a tool's config git tracks (#879), by server. - const withheld = new Map(); - for (const target of targets) { - const { skipped, withheld: names } = desiredMcpForTarget(target, servers.map((s) => teamMcpToDef(s.entry)), context); - for (const change of skipped) { - if (names.has(change.server)) withheld.set(change.server, [...withheld.get(change.server) ?? [], `${target.tool} — ${change.reason}`]); - } - } + const vars = await buildVarTable(localConfig, teamEnv); // Project scope reads THIS worktree's own per-worktree manifest; user the global file. const manifest = (await readJson( managedMcpManifestPath( @@ -90,11 +82,17 @@ export async function mcpList(_options: GlobalOptions): Promise { console.log(` secrets: ${needed.join(', ')} (${state})`); } - const installedIn = targets - .filter((t) => (manifest[managedMcpManifestKey(t.tool, t.projectScope)] ?? []).some((r) => r.name === s.name)) - .map((t) => t.tool); + const installed = (t: McpTarget): boolean => + (manifest[managedMcpManifestKey(t.tool, t.projectScope)] ?? []).some((r) => r.name === s.name); + const installedIn = targets.filter(installed).map((t) => t.tool); console.log(` installed: ${installedIn.length > 0 ? installedIn.join(', ') : '(none)'}`); - for (const reason of withheld.get(s.name) ?? []) console.log(` withheld: ${reason}`); + // Pull writes a resolved value only into a file git leaves out of a commit + // (#882); an entry an earlier pull wrote there stays as it was. + for (const t of targets) { + if (!carriesResolvedValue(t, [s], [s.name])) continue; + const exclusion = await ensureExcludedFromGit(t.file, { dryRun: true }); + if (exclusion.kind === 'failed') console.log(` withheld: ${t.tool} — ${exclusion.reason}. ${exclusion.fix}`); + } console.log(''); } diff --git a/src/mcp-git-exclude.ts b/src/mcp-git-exclude.ts index b9b08ee7..5ce56572 100644 --- a/src/mcp-git-exclude.ts +++ b/src/mcp-git-exclude.ts @@ -75,6 +75,13 @@ async function gitExcludeFile(dir: string): Promise<{ excludeFile: string; root: return { excludeFile: path.resolve(base, gitPath), root, prefix }; } +/** The closest directory above `file` that exists. */ +async function existingAncestor(file: string): Promise { + let dir = path.dirname(path.resolve(file)); + while (!await pathExists(dir) && path.dirname(dir) !== dir) dir = path.dirname(dir); + return dir; +} + /** * Whether git would put a file in a commit. `unknown` is a repository git could * not answer for (unsafe ownership, a bad config): never read it as safe. @@ -87,8 +94,8 @@ export type GitTracking = /** Whether git would put `file` in a commit: tracked, or untracked without an ignore rule. Read-only. */ export async function gitTracking(file: string): Promise { - const dir = path.dirname(file); - const result = await execCommand('git', ['check-ignore', '-q', '--', path.basename(file)], { cwd: dir, timeoutMs: 10_000 }) + const dir = await existingAncestor(file); + const result = await execCommand('git', ['check-ignore', '-q', '--', path.relative(dir, file)], { cwd: dir, timeoutMs: 10_000 }) .catch((e: unknown) => ({ code: -1, stdout: '', stderr: e instanceof Error ? e.message : String(e) })); if (result.code === 0) return { kind: 'ignored' }; if (result.code === 1) return { kind: 'would-commit' }; @@ -105,10 +112,9 @@ export async function gitTracking(file: string): Promise { * is not tracked; nor is one in a repository git cannot answer for, where a * commit fails too. */ -export async function gitTracks(file: string): Promise { +async function gitTracks(file: string): Promise { // The file, or even its directory, may be gone from disk and still be in the index. - let dir = path.dirname(file); - while (!await pathExists(dir) && path.dirname(dir) !== dir) dir = path.dirname(dir); + const dir = await existingAncestor(file); const result = await execCommand('git', ['--literal-pathspecs', 'ls-files', '--error-unmatch', '--', path.relative(dir, file)], { cwd: dir, timeoutMs: 10_000 }) .catch(() => null); return result?.code === 0; @@ -129,49 +135,96 @@ function splitBlock(content: string): { before: string; patterns: string[]; afte return { before: content.slice(0, start), patterns, after }; } +/** + * Whether `file` is kept out of git, or why teamai could not keep it out and + * what the member does about it. `pending`: a dry run found nothing in the way + * of listing it. + */ +export type GitExclusion = + | { kind: 'excluded' } + | { kind: 'pending' } + | { kind: 'failed'; reason: string; fix: string }; + /** * Add `file` to its repository's `.git/info/exclude` unless git ignores it - * already. Idempotent; a path already ignored, or outside any repository, adds - * nothing, and one git cannot answer for is added all the same. A failure warns + * already, and whether git now leaves it out of a commit. Idempotent; a path + * already ignored, or outside any repository, adds nothing, and one git cannot + * answer for is added all the same. `file` need not exist yet: pull calls this + * before writing a resolved value into it. `dryRun` writes nothing and reports + * what would stop the write. + */ +export async function ensureExcludedFromGit(file: string, options: { dryRun?: boolean } = {}): Promise { + const tracking = await gitTracking(file); + if (tracking.kind === 'ignored' || tracking.kind === 'outside-repo') return { kind: 'excluded' }; + // `file` and its directory need not exist yet: git is asked from the nearest one that does. + const dir = await existingAncestor(file); + const location = await gitExcludeFile(dir); + if (!location) { + return { + kind: 'failed', + reason: tracking.kind === 'unknown' ? tracking.error : 'git could not locate .git/info/exclude', + fix: 'Fix the repository, or add the file to its .git/info/exclude yourself, then run `teamai pull` again.', + }; + } + const { excludeFile } = location; + // Anchored at the working tree root, glob characters escaped. + const rel = path.relative(dir, file).split(path.sep).join('/'); + const pattern = `/${location.prefix}${rel}`.replace(/[\\*?[\]!#]/g, '\\$&'); + const retry = `Make it writable, or add \`${pattern}\` to it yourself, then run \`teamai pull\` again.`; + // A read-only exclude file is the member's choice; the atomic write would replace it all the same. + for (const writable of [path.dirname(excludeFile), ...(await pathExists(excludeFile) ? [excludeFile] : [])]) { + const denied = await fse.access(writable, fse.constants.W_OK).then(() => false, () => true); + if (denied) return { kind: 'failed', reason: `${writable} is not writable`, fix: retry }; + } + const add = (content: string): string | null => { + const block = splitBlock(content); + if (block?.patterns.includes(pattern)) return null; + const head = block ? block.before : content; + const patterns = [...(block?.patterns ?? []), pattern]; + const body = [MCP_EXCLUDE_START, ...patterns, MCP_EXCLUDE_END].join('\n'); + const sep = head === '' || head.endsWith('\n') ? '' : '\n'; + return `${head}${sep}${body}\n${block?.after ?? ''}`; + }; + // An exclude rule does not apply to a file git tracks already. + const tracked: GitExclusion = { + kind: 'failed', + reason: `git already tracks ${file}`, + fix: `Run \`git rm --cached ${file}\` (rotate any value a commit of it holds), then \`teamai pull\` again.`, + }; + let result: ExcludeUpdate; + try { + if (options.dryRun) { + // Nothing listed yet: only a tracked file would still stop the write. + if (add((await readFileSafe(excludeFile)) ?? '') !== null) return await gitTracks(file) ? tracked : { kind: 'pending' }; + result = 'unchanged'; + } else { + result = await updateExclude(excludeFile, add); + } + } catch (e) { + return { kind: 'failed', reason: `adding it to ${excludeFile} failed: ${e instanceof Error ? e.message : String(e)}`, fix: retry }; + } + if (result === 'locked') { + return { + kind: 'failed', + reason: `another teamai command held ${excludeFile} past the wait`, + fix: 'Run `teamai pull` again.', + }; + } + if (result === 'written') log.debug(`Added ${pattern} to ${excludeFile}`); + return (await gitTracking(file)).kind === 'would-commit' ? tracked : { kind: 'excluded' }; +} + +/** + * `ensureExcludedFromGit` for a file already on disk, warning when it fails * rather than failing the sync that wrote the file. */ export async function excludeFromGit(file: string): Promise { if (!await pathExists(file)) return; - const tracking = await gitTracking(file); - if (tracking.kind === 'ignored' || tracking.kind === 'outside-repo') return; - const location = await gitExcludeFile(path.dirname(file)); - if (!location) { - const reason = tracking.kind === 'unknown' ? tracking.error : 'git could not locate .git/info/exclude'; + const exclusion = await ensureExcludedFromGit(file); + if (exclusion.kind === 'failed') { log.warn( - `${file} holds a resolved MCP variable, and teamai could not keep it out of git: ${reason}. ` - + 'Fix the repository, or add the file to its .git/info/exclude yourself, so git does not commit the value.', - ); - return; - } - const { excludeFile } = location; - // Anchored at the working tree root, glob characters escaped. - const pattern = `/${location.prefix}${path.basename(file)}`.replace(/[\\*?[\]!#]/g, '\\$&'); - try { - const result = await updateExclude(excludeFile, (content) => { - const block = splitBlock(content); - if (block?.patterns.includes(pattern)) return null; - const head = block ? block.before : content; - const patterns = [...(block?.patterns ?? []), pattern]; - const body = [MCP_EXCLUDE_START, ...patterns, MCP_EXCLUDE_END].join('\n'); - const sep = head === '' || head.endsWith('\n') ? '' : '\n'; - return `${head}${sep}${body}\n${block?.after ?? ''}`; - }); - if (result === 'written') log.debug(`Added ${pattern} to ${excludeFile}`); - if (result === 'locked') { - log.warn( - `${file} holds a resolved MCP variable and is not excluded from git yet: another teamai command held ${excludeFile} past the wait. ` - + 'Run `teamai pull` again, and do not commit the file meanwhile.', - ); - } - } catch (e) { - log.warn( - `${file} holds a resolved MCP variable, and adding it to ${excludeFile} failed: ${e instanceof Error ? e.message : String(e)}. ` - + `Add \`${pattern}\` to that file yourself so git does not commit the value.`, + `${file} holds a resolved MCP variable, and teamai could not keep it out of git: ${exclusion.reason}. ` + + `${exclusion.fix} Do not commit the file meanwhile.`, ); } } diff --git a/src/mcp-reconcile.ts b/src/mcp-reconcile.ts index 78aad607..e9183683 100644 --- a/src/mcp-reconcile.ts +++ b/src/mcp-reconcile.ts @@ -49,7 +49,7 @@ import { log } from './utils/logger.js'; import { warnOnce } from './utils/warn-once.js'; import { loadProjectMcpManifest } from './utils/mcp-manifest.js'; import { isOnPath, SAFE_BIN_RE, type LookPathOptions } from './utils/lookpath.js'; -import { carriesResolvedValue, excludeFromGit, gitTracks, resolvedVariableIn } from './mcp-git-exclude.js'; +import { carriesResolvedValue, ensureExcludedFromGit, excludeFromGit, resolvedVariableIn, type GitExclusion } from './mcp-git-exclude.js'; // ─── Reconcile engine ──────────────────────────────────────── // @@ -418,38 +418,21 @@ export interface DesiredMcpContext { vars: Record; /** Which `${VAR}` names are declared secrets, whose missing value keeps an entry (#875). */ secrets: SecretDeclarations; - /** The project MCP configs git tracks, which never get a declared secret's value (#879). */ - tracked: ReadonlySet; lookPath?: McpReconcileOptions['lookPath']; } -/** Why a server is not written to `file`, a config git tracks, and what fixes it (#879). */ -export function trackedConfigReason(file: string, secrets: readonly string[]): string { - return `${file} is tracked by git, so the value of ${secrets.join(', ')} would be committed. ` - + `Run \`git rm --cached ${file}\` and rotate the token, then \`teamai pull\``; -} - -/** `targets` are the ones the caller will render for: which of them git tracks is asked here, once. */ export async function buildDesiredMcpContext( teamConfig: TeamaiConfig, localConfig: LocalConfig, options: McpReconcileOptions = {}, - targets: readonly McpTarget[] = [], ): Promise { // HTTP mode has no repo tree to declare secrets in. const teamEnv = localConfig.repo.kind === 'http' ? undefined : options.teamEnv ?? await resolveTeamEnv(localConfig); - const secrets: SecretDeclarations = teamEnv?.declarations ?? { kind: 'absent' }; - // Declarations that failed may name any variable, so they are asked about too. - const mayHoldSecret = declaredSecretKeys(secrets)?.size !== 0; - const tracked = mayHoldSecret - ? await Promise.all(targets.filter((t) => t.projectScope).map(async (t) => (await gitTracks(t.file) ? [t.file] : []))) - : []; return { sharing: getMcpSharing(teamConfig), excluded: new Set(localConfig.excludedSkills ?? []), vars: await buildVarTable(localConfig, teamEnv), - secrets, - tracked: new Set(tracked.flat()), + secrets: teamEnv?.declarations ?? { kind: 'absent' }, lookPath: options.lookPath, }; } @@ -468,20 +451,15 @@ export async function buildDesiredMcpContext( * a secret that lives in the member's shell is there for one pull and gone for * the next, and an entry an earlier pull wrote stays as it is. With * declarations that failed, every skipped server is kept. - * - * `withheld` names the servers skipped because `target` is a project config - * git tracks and the entry would hold a declared secret's value (#879): git - * would commit it. An entry an earlier pull wrote there stays as it is. */ export function desiredMcpForTarget( target: McpTarget, teamDefs: McpServerDef[], ctx: DesiredMcpContext, -): { desired: Map; skipped: McpChange[]; kept: Set; withheld: Set } { +): { desired: Map; skipped: McpChange[]; kept: Set } { const desired = new Map(); const skipped: McpChange[] = []; const kept = new Set(); - const withheld = new Set(); // Declarations that failed can't say which variables are secrets, so every // missing one may be: pull keeps every installed entry then. const declared = declaredSecretKeys(ctx.secrets); @@ -515,8 +493,7 @@ export function desiredMcpForTarget( // Pass ${VAR} through where the tool expands it itself, so the secret // never lands on disk; otherwise resolve and require every var to exist. // A resolved value is written verbatim into the target file; a project - // file holding one is kept out of git (#882), and one git already tracks - // never gets a declared secret's value. + // file gets one only once it is kept out of git (#882, reconcileTargets). const passthrough = supportsEnvExpansion(target.format, target.projectScope, raw); let def = raw; if (!passthrough) { @@ -531,12 +508,6 @@ export function desiredMcpForTarget( if (missing.every((key) => declared?.has(key) ?? true)) kept.add(raw.name); continue; } - const secrets = referencedVars(raw).filter((key) => declared?.has(key) ?? true); - if (secrets.length > 0 && ctx.tracked.has(target.file)) { - skipped.push({ tool: target.tool, server: raw.name, action: 'skipped', reason: trackedConfigReason(target.file, secrets) }); - withheld.add(raw.name); - continue; - } def = resolved; } else if (referencedVars(raw).length > 0) { log.debug(`${raw.name}: passing ${referencedVars(raw).join(', ')} through to ${target.tool}`); @@ -552,7 +523,7 @@ export function desiredMcpForTarget( } } - return { desired, skipped, kept, withheld }; + return { desired, skipped, kept }; } /** @@ -676,11 +647,13 @@ export async function reconcileMcpForConfig( localConfig: LocalConfig, options: McpReconcileOptions = {}, ): Promise { + // Each project config's exclusion from git, established before a resolved value is written into it. + const exclusions = new Map(); try { - return await reconcileTargets(teamConfig, localConfig, options); + return await reconcileTargets(teamConfig, localConfig, options, exclusions); } finally { // Also after a failed write: what earlier pulls wrote is on disk either way. - if (!options.removeAll && !options.dryRun) await protectResolvedMcpConfigs(teamConfig, localConfig); + if (!options.removeAll && !options.dryRun) await protectResolvedMcpConfigs(teamConfig, localConfig, exclusions); } } @@ -690,11 +663,15 @@ export async function reconcileMcpForConfig( * run delivered to it: the file of a disabled or undetected tool, or one * written before the team turned delivery off, still holds what a pull wrote. */ -async function protectResolvedMcpConfigs(teamConfig: TeamaiConfig, localConfig: LocalConfig): Promise { +async function protectResolvedMcpConfigs( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + exclusions: Map, +): Promise { const { projectRoot } = localConfig; if (localConfig.scope !== 'project' || !projectRoot || localConfig.repo.kind === 'http') return; try { - await protectProjectMcpConfigs(teamConfig, localConfig, projectRoot); + await protectProjectMcpConfigs(teamConfig, localConfig, projectRoot, exclusions); } catch (e) { log.warn( `Could not check this project's MCP configs for resolved values to keep out of git: ${e instanceof Error ? e.message : String(e)}. ` @@ -703,12 +680,19 @@ async function protectResolvedMcpConfigs(teamConfig: TeamaiConfig, localConfig: } } -async function protectProjectMcpConfigs(teamConfig: TeamaiConfig, localConfig: LocalConfig, projectRoot: string): Promise { +async function protectProjectMcpConfigs( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + projectRoot: string, + exclusions: Map, +): Promise { const resolution = await resolveEntriesFor(mcpEntryReader, localConfig); const teamDefs = resolution.kind === 'failed' ? null : resolution.entries.map((entry) => teamMcpToDef(entry.entry)); const { manifest } = await loadProjectMcpManifest(getDataHome(localConfig), projectRoot, { dryRun: true }); const vars = await buildVarTable(localConfig); for (const target of await resolveMcpTargets(teamConfig, localConfig, { includeUndetected: true })) { + // Tried before its write this run, and reported there when it failed. + if (exclusions.has(target.file)) continue; const owned = (manifest[managedMcpManifestKey(target.tool, true)] ?? []).map((record) => record.name); if (await resolvedValueEvidence(target, teamDefs, owned, vars)) await excludeFromGit(target.file); } @@ -718,6 +702,7 @@ async function reconcileTargets( teamConfig: TeamaiConfig, localConfig: LocalConfig, options: McpReconcileOptions, + exclusions: Map, ): Promise { const changes: McpChange[] = []; let wrote = false; @@ -758,7 +743,7 @@ async function reconcileTargets( const nothingOwned = Object.values(manifest).every((r) => r.length === 0); if (teamDefs.length === 0 && nothingOwned) return { changes, wrote }; - const desiredContext = await buildDesiredMcpContext(teamConfig, localConfig, options, targets); + const desiredContext = await buildDesiredMcpContext(teamConfig, localConfig, options); // A failed declaration is not "no secrets": read as none, every server whose // secret the member left in their shell would be removed. Keep what is // installed rather than guess which variables are secrets. `removeAll` @@ -779,11 +764,26 @@ async function reconcileTargets( const nextRecords: ManagedMcpRecord[] = []; // Which of this team's servers apply to this tool, and in what rendered form. - const { desired, skipped, kept, withheld } = desiredMcpForTarget(target, teamDefs, desiredContext); + const { desired, skipped, kept } = desiredMcpForTarget(target, teamDefs, desiredContext); changes.push(...skipped); - for (const change of skipped) if (withheld.has(change.server)) log.warn(`MCP server ${change.server} not written: ${change.reason}.`); // Their old records, so a manifest this run writes still claims them. - const keep = new Map(owned.filter((r) => kept.has(r.name) || withheld.has(r.name)).map((r) => [r.name, r])); + const keep = new Map(owned.filter((r) => kept.has(r.name)).map((r) => [r.name, r])); + + // A resolved value lands only in a file git leaves out of a commit (#882). + // Otherwise the file stays as it was, its manifest entry with it. + if (carriesResolvedValue(target, teamDefs, desired.keys())) { + const exclusion = exclusions.get(target.file) ?? await ensureExcludedFromGit(target.file, { dryRun: options.dryRun }); + exclusions.set(target.file, exclusion); + if (exclusion.kind === 'failed') { + const reason = `${target.file} is not kept out of git: ${exclusion.reason}`; + for (const server of desired.keys()) changes.push({ tool: target.tool, server, action: 'skipped', reason }); + log.warn( + `Did not write ${target.tool}'s MCP servers to ${target.file}: it would hold resolved values, and teamai could not ` + + `keep it out of git first: ${exclusion.reason}. The file is left as it was. ${exclusion.fix}`, + ); + continue; + } + } if (target.format === 'codex') { wrote = await applyCodex(target, desired, keep, ownedNames, nextRecords, changes, options) || wrote;