diff --git a/CHANGELOG.md b/CHANGELOG.md index 3a3307356..e0f7326aa 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ All notable changes to this project will be documented in this file. See [standa ### 💥 Breaking Changes +- An env variable, hook or MCP server with a key its schema does not know, such as a misspelled `role:` or a hand-added `notes:`, is no longer delivered to anyone: the key used to be dropped silently, so a misspelled restriction shipped the entry to every member. `teamai pull` and `teamai doctor` name the file, the entry and the key. `teamai env add`, `teamai env remove` and `teamai remove mcp` keep the key when they rewrite the file. A key that a later version adds to these entries is unknown to this one too, so an entry that uses it is not delivered to a member still on this version: upgrade every member before the team uses a new entry key, as for a new `resources:` key (for [#822](https://github.com/Tencent/teamai-cli/issues/822)). - `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, as a project id already had to be (the id keeps its own narrower ASCII rule). A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil`, `a/b`, `C:evil`, a bare `..`, any name with a trailing `.` or space — which Win32 strips, making `.. ` arrive as `..` and `frontend.` as `frontend` — and a Windows device name such as `CON` or `COM1` under `resources:` no longer parse; the error names the offending entry. Nothing else is rejected: a namespace that is a plain directory name still parses, non-ASCII names and names with a space included. A manifest that fails to parse now reports the offending entry on one line (`Invalid projects manifest: projects.0.resources.skills.1: ...`) instead of dumping a raw validation object. Two namespaces of one resource type that differ only by case (`frontend`, `Frontend`) are rejected too, within a manifest and between the two, since they name one directory on Windows and macOS. A manifest that ships any of these — a device name, a trailing `.`, a case-only pair — parsed before and fails every pull now; rename the directory and the entry together. - **Upgrade every member before a team declares a new axis.** `resources:` in `manifest/roles.yaml` and `manifest/projects.yaml` accepts `env`, `hooks`, `mcp`, `models` and `docs`, but teamai 0.25.0 and the 0.26.0 betas reject a `resources:` key they do not know, so a team that declares one breaks pull for every member still on those versions. From this version on, an unknown `resources:` key prints one warning naming the role or project and the key, and the scope syncs as if the key were absent; `teamai roles` and `teamai projects` keep the key when they save the manifest. `teamai roles|projects add/update --namespaces` never write the new keys, so nothing declares them until an admin does by hand (for [#707](https://github.com/Tencent/teamai-cli/issues/707)). - Per-entry scoping of env variables, hooks and MCP servers gives way to namespace files (see Features). `projects:` on an `env/env.yaml` variable, a `hooks/hooks.yaml` hook or an `mcp/mcp.yaml` server, and `roles:` on an env variable, existed only in the 0.26.0 betas and are removed: such an entry now reaches nobody, and each pull warns with the namespace file to move it to, one per listed id, so a project-only value never falls through to the whole team. `roles:` on hooks and MCP servers, which 0.25.0 shipped, is deprecated: it keeps filtering for one more minor release as 0.25.0 did, a name repeated in one file under different `roles:` included, pull warns once per run and `teamai doctor` has a check, both naming every target file. There is no automatic migration; move each entry into the file the warning names and drop the key. A member with no role in a team with `roles.yaml` received every `roles:`-scoped hook and server; once they move into `hooks//` or `mcp//`, that member no longer does (for [#707](https://github.com/Tencent/teamai-cli/issues/707)). @@ -31,6 +32,8 @@ All notable changes to this project will be documented in this file. See [standa ### 🐛 Bug Fixes +- A misspelled top-level key in `mcp/mcp.yaml` or `hooks/hooks.yaml` (`server:` for `servers:`, `hook:` for `hooks:`) no longer removes every installed team MCP server or hook: such a file read as empty. It now fails like a file that does not parse, so pull keeps what is installed, and pull and `teamai doctor` name the file, the keys found and the key expected. An extra top-level key beside `servers:` or `hooks:` is still ignored (for [#822](https://github.com/Tencent/teamai-cli/issues/822)). +- The closing line of `teamai recall` output is in English (for [#822](https://github.com/Tencent/teamai-cli/issues/822)). - A broken team file no longer wipes or downgrades what a member has installed. An `mcp/mcp.yaml` or `hooks/hooks.yaml` that did not parse reconciled to an empty set and removed every team MCP server or hook from every tool, and two active namespaces defining one skill or agent aborted the pull for the whole scope, skipping rules, env, docs and cleanup. Now a file in the active set that does not parse or cannot be read, a name repeated inside one file, or one name in two active namespaces stops only that resource type for the run: env keeps `env.sh`, hooks and MCP keep their entries (the built-in hooks, with the session-start pull, are still installed where missing: with the root hooks file's `builtin:` overrides when it parses, and otherwise with their defaults only in a tool that has none yet; `teamai init` says the team hooks were not installed), model profiles leave switched agents alone, skills and agents keep what is installed, and every other type still syncs. The warning names the file or both files and the fix, and for env, hooks, MCP and models is also written to `~/.teamai/debug.log` for session-start pulls (for [#707](https://github.com/Tencent/teamai-cli/issues/707)). - In single-repo (`kind: self`) mode, every checkout of the business repo now publishes learnings and reports. The `teamai-learnings` and `teamai-reports` checkouts and the queue of unpublished learnings lived in each checkout's own `.teamai/`, and git checks a branch out in one worktree only, so the first checkout to create them locked every other one out (`'teamai-learnings' is already used by worktree`), and a learning queued in a linked worktree was deleted with it by a plain `git worktree remove`. They now live in the project partition that every checkout shares, and the search index is kept per checkout, so `recall` no longer serves another checkout's index with paths into it. On upgrade, `init`, `pull`, `push`, `contribute` and `import --from-mr` migrate a checkout's old install, moving its queue into the partition without overwriting anything (or into `pending-learnings.self` when another checkout has since switched the project to another kind; an old git-mode install beside the knowledge of a switch to single-repo mode goes to `.teamai.bak`, its queue to `pending-learnings.git`); `contribute` and `import --from-mr` stop, saving nothing, while that data cannot move out of the checkout; and the first command that needs a side-branch checkout removes the old one from `.teamai/`; one with uncommitted changes is kept, a warning names it and says what to do, and `recall maintenance` and `recall promote` stop until then, while another command holds the learnings or reports lock, and when a checkout cannot be created, naming the cause. A git-mode install of the same project uses the same partition paths, so a checkout that belongs to the other repository is refused, never reused or removed, with the command that clears it, and so is one whose repository was moved or deleted; its votes are not counted, `init` deletes the search indexes built for the old repository, learnings the old install still had queued are set aside in `pending-learnings.` rather than published by the new one (and in `pending-learnings.-` when `init` points an install at another team repository of the same kind, which on `main` published them to the new one; the same repository written another way keeps them, [#823](https://github.com/Tencent/teamai-cli/issues/823) item 13), and `uninstall` lists every unpublished queue before it asks. (for [#808](https://github.com/Tencent/teamai-cli/issues/808)). - A scope's usage file no longer grows without bound where it is never reported: an http source, a team with `usageReport: false`, or a remote that rejects every push. `teamai pull` keeps its newest 5,000 events, so `teamai stats` still shows recent usage there, the local file being its only source. In a reporting scope whose report does not complete while it holds more than 5,000 events, the dropped events were never reported. The cap runs after the report has removed the events it sent, so it cannot shift that removal onto events not yet reported, and a file at or below the cap is not rewritten. Hook appends, the report's truncate and the cap now share one lock beside the usage file, and a rewrite goes through a temp file that keeps the file's mode, so a rewrite no longer loses an event recorded while it runs, nor the file on a kill or a full disk. A hook that cannot take the lock within ~250 ms records its event in a `*.pending-.jsonl` file beside it, which the next lock holder appends; a rewrite that cannot take it within ~5 s leaves the file as it is. A pending file gets no wider mode than the usage file (owner-only while there is none), and an in-workspace `.teamai/.gitignore` ignores the lock, a rewrite's temp copy and the pending files; `pull` and `push` add those entries to an existing single-repo one, and the first pending file or rewrite adds them to an existing project-scope one (for [#788](https://github.com/Tencent/teamai-cli/issues/788)). diff --git a/docs/designs/multi-project-management.md b/docs/designs/multi-project-management.md index 82217dfd4..3cbdb6f5d 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -271,7 +271,9 @@ resolved env set. Skills keep one difference: in role/project mode the root `skills/` stays the tag catalog and is not delivered by default. A root skill that arrives through a subscribed tag is replaced by an active namespace skill of the same name, and -among tag matches the root skill wins over one in an inactive namespace. Installing +among tag matches the root skill wins over one in an inactive namespace. A tagged +skill that exists only in an inactive namespace still reaches a subscribed member: +tags cross namespaces by design (#337). Installing a skill removes the files that another team version of that skill (root or any namespace) has and the new one lacks, when they match that version byte for byte, so switching versions leaves no team file behind; a file the member added @@ -408,7 +410,20 @@ and pull warns with the namespace file to move it to, one per listed id. `roles:` on hooks and MCP shipped in 0.25.0 and keeps filtering for one more minor release; pull warns once per run and `doctor` has an informational check, both naming every target file. Model profiles are strict, so a per-entry key -fails the file. There is no automatic migration. +fails the file. An env, hook or MCP entry with any other key its schema does not +know, such as a mistyped `role:`, reaches nobody too, and pull and `doctor` name +the file, the entry and the key (#822); `env add`, `env remove` and `remove mcp` +keep such a key when they rewrite the file. A key that a later version adds is +unknown to this one as well, so an entry that uses it is not delivered to a member +still on this version: every member has to upgrade before the team uses a new +entry key, as for a new `resources:` key. A hooks or MCP file with none of its +top-level keys (`server:` for `servers:`, `hook:` for `hooks:`) used to read as +empty and remove every installed server or hook; it now fails like a file that +does not parse, naming the keys it found, as `env.yaml` has since #662. An extra +top-level key beside a known one is still ignored. `pull --dry-run` resolves the hooks +and MCP entries and reports their warnings (an unknown id, a per-entry key, a file +that does not parse) without writing, so a maintainer can see them before a real +pull applies them. There is no automatic migration. ### Push and commands @@ -434,15 +449,8 @@ entry comes from. ### Known gaps -- The tag channel matches skills by name across every namespace, so a tagged - skill that exists only in an inactive namespace still reaches the member. - No pull protects a local edit from being overwritten, override transitions included. -- A mistyped per-entry key (`role:`) is stripped by the schema and the entry - reaches everyone. `pull --dry-run` resolves the hooks and MCP entries and - reports their warnings (an unknown id, a deprecated per-entry `roles:`, a file - that does not parse) without writing, so a maintainer can see them before a - real pull applies them. ## Backward compatibility diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 94477309d..dab8f2416 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -976,6 +976,17 @@ The per-entry keys these files replace: There is no automatic migration: move each entry into the namespace file the warning names, and drop the key. +An entry with any other key its schema does not know, such as a mistyped `role:`, +reaches nobody as well, and pull and `teamai doctor` name the file, the entry and +the key. Correct the key or remove it. A key that a later teamai version adds is +unknown to an older one too, so upgrade every member before the team uses a new +entry key. + +A hooks or MCP file that has none of its top-level keys, such as `server:` for +`servers:`, is treated like a file that does not parse: pull keeps the installed +servers or hooks, and pull and `teamai doctor` name the file, the keys found and +the key expected. An extra top-level key next to `servers:` or `hooks:` is ignored. + ### Env (environment variables) ```bash diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 846d761a6..367ceeda1 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -895,6 +895,15 @@ projects: 没有自动迁移：把每个条目移到警告给出的 namespace 文件中，并删掉该 key。 +条目若带有其 schema 不认识的其他 key（例如拼错的 `role:`），同样不会下发给任何人； +pull 与 `teamai doctor` 会指出文件、条目和该 key。请改正或删除这个 key。 +较新版本 teamai 新增的 key 对旧版本同样是未知 key，因此团队使用新的条目 key 之前， +请先让所有成员升级。 + +hooks 或 MCP 文件若没有任何一个应有的顶层 key（例如把 `servers:` 写成 `server:`）， +按无法解析的文件处理：pull 保留已安装的 server 或 hook，pull 与 `teamai doctor` +会指出文件、实际找到的 key 和应有的 key。`servers:` 或 `hooks:` 旁多出的顶层 key 会被忽略。 + ### Env（环境变量） ```bash diff --git a/skill-data/setup/references/manage-admin.md b/skill-data/setup/references/manage-admin.md index 654309ca6..0f476c8b9 100644 --- a/skill-data/setup/references/manage-admin.md +++ b/skill-data/setup/references/manage-admin.md @@ -168,10 +168,15 @@ and push it with git. `teamai doctor` lists each override. - A name twice in one file, in two active namespaces, or an active file that does not parse: that type is not applied for affected members and their installed - state is kept. Fix the file the warning names. + state is kept. Fix the file the warning names. A hooks or MCP file with none of + its top-level keys (`server:` for `servers:`) counts as one that does not parse. - Per-entry `projects:` (and `roles:` on env) no longer works: such an entry reaches nobody. `roles:` on hooks and MCP still filters for one more minor release. Pull and `teamai doctor` name the namespace file each entry belongs in; move it there. +- An env, hook or MCP entry with a key its schema does not know (a mistyped `role:`) + also reaches nobody. Pull and `teamai doctor` name the file, entry and key; correct + the key or remove it. A key a later teamai version adds is unknown to an older one, + so upgrade every member before the team uses a new entry key. - Team model profiles work the same way: `models//models.yaml`, declared under `resources.models`, replaces the root profile with the same `id` for members who have `` active. A member's API key is bound to the profile's gateway origin: diff --git a/src/__tests__/doctor-entry-namespaces.test.ts b/src/__tests__/doctor-entry-namespaces.test.ts index 935e849af..9c326f810 100644 --- a/src/__tests__/doctor-entry-namespaces.test.ts +++ b/src/__tests__/doctor-entry-namespaces.test.ts @@ -103,6 +103,18 @@ describe('doctor — env, hooks and MCP namespaces', () => { expect(check.fix).toContain('env/env.yaml: variable "A" is scoped with per-entry `projects:`'); }); + it('names an entry with a key its schema does not know in the same check (#822)', async () => { + await fse.outputFile(path.join(repoPath, 'mcp', 'mcp.yaml'), [ + 'servers:', + ' - { name: db, transport: http, url: https://a.example.com, role: [frontend] }', + '', + ].join('\n')); + + const [check] = await buildEntryScopeKeyCheck(ctx()); + if (!check) throw new Error('expected the scope-key check'); + expect(check.fix).toContain('mcp/mcp.yaml: server "db" has unknown key `role:`, so this entry is not delivered'); + }); + it('adds no check when no entry carries a per-entry key', async () => { await fse.outputFile(path.join(repoPath, 'hooks', 'hooks.yaml'), 'hooks:\n - { id: lint, description: x, event: Stop, command: echo }\n'); expect(await buildEntryScopeKeyCheck(ctx())).toEqual([]); diff --git a/src/__tests__/e2e/project-scoped-delivery.test.ts b/src/__tests__/e2e/project-scoped-delivery.test.ts index 818aff070..2adf04113 100644 --- a/src/__tests__/e2e/project-scoped-delivery.test.ts +++ b/src/__tests__/e2e/project-scoped-delivery.test.ts @@ -363,6 +363,28 @@ describe('project-scoped hooks, MCP servers and env variables via the real CLI ( expect(pull.output).toContain('Move it to mcp/billing/mcp.yaml and drop the key.'); expect(readClaudeMcp()).not.toContain('legacy-api'); }, 60_000); + + // A typo of a scoping key (#822) must not widen who gets the server. + it('installs no server that carries an unknown key, and warns naming the file, server and key', async () => { + const teamRepo = path.join(projectRoot, '.teamai', 'team-repo'); + fs.writeFileSync(path.join(teamRepo, 'mcp', 'mcp.yaml'), [ + 'servers:', + ' - name: typo-api', + ' transport: http', + ' url: https://typo.example.com/mcp', + ' role: [frontend]', + ' - name: shared-api', + ' transport: http', + ' url: https://shared.example.com/mcp', + '', + ].join('\n')); + + const pull = await runCLI(['pull', '--force'], projectRoot, home); + expect(pull.code, pull.output).toBe(0); + expect(pull.output).toMatch(/mcp\/mcp\.yaml: server "typo-api".*\brole\b/); + expect(readClaudeMcp()).not.toContain('typo-api'); + expect(readClaudeMcp()).toContain('shared-api'); + }, 60_000); }); // Codex has no project-scope MCP location (no `mcpProject` in toolPaths), so its diff --git a/src/__tests__/env-commands.test.ts b/src/__tests__/env-commands.test.ts index 207dc9853..d0ff6f792 100644 --- a/src/__tests__/env-commands.test.ts +++ b/src/__tests__/env-commands.test.ts @@ -280,6 +280,24 @@ scope: 'user', }); }); + // A variable with a misspelled `roles:` reaches nobody (#822); a rewrite + // that drops the key would deliver it to the whole team. + it('preserves a key env does not know on a variable it updates', async () => { + await fse.writeFile( + path.join(repoPath, 'env', 'env.yaml'), + YAML.stringify({ variables: [{ key: 'DB_URL', value: 'old', role: ['frontend'] }] }), + ); + + await envAdd('DB_URL', 'new', {}); + + const parsed = YAML.parse(await fse.readFile(path.join(repoPath, 'env', 'env.yaml'), 'utf-8')); + expect(parsed.variables).toEqual([{ key: 'DB_URL', value: 'new', role: ['frontend'] }]); + expect(log.warn).toHaveBeenCalledWith( + 'env/env.yaml: variable "DB_URL" has unknown key `role:`, so pull does not deliver it. ' + + 'Correct the key or remove it in env/env.yaml.', + ); + }); + it('preserves the scope of other variables when adding a new one', async () => { await fse.writeFile( path.join(repoPath, 'env', 'env.yaml'), diff --git a/src/__tests__/env-handler.test.ts b/src/__tests__/env-handler.test.ts index 63b63b8d9..0db8b14bc 100644 --- a/src/__tests__/env-handler.test.ts +++ b/src/__tests__/env-handler.test.ts @@ -690,6 +690,60 @@ scope: 'user', )); }); + // A typo of a scoping key (#822) must not widen who gets the variable. + it('delivers no variable that carries a key env does not know, and names the file, variable and key', async () => { + await fse.writeFile(path.join(repoPath, 'env', 'env.yaml'), YAML.stringify({ + variables: [ + { key: 'DB_URL', value: 'db', role: ['frontend'] }, + { key: 'SHARED', value: 's' }, + ], + })); + const { log } = await import('../utils/logger.js'); + vi.mocked(log.warn).mockClear(); + + await handler.pullItem(item, teamConfig, localConfig); + + const content = await envSh(); + expect(content).toContain('export SHARED='); + expect(content).not.toContain('DB_URL'); + const warnings = vi.mocked(log.warn).mock.calls.map(([m]) => String(m)) + .filter((m) => m.includes('env/env.yaml') && m.includes('"DB_URL"')); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toMatch(/\brole\b/); + }); + + it('keeps the root variable when the active namespace copy of it carries an unknown key', async () => { + await writeCheckoutProject(); + await fse.outputFile(path.join(repoPath, 'env', 'checkout', 'env.yaml'), YAML.stringify({ + variables: [{ key: 'MODEL_ENDPOINT', value: 'https://checkout.example.com', role: ['frontend'] }], + })); + const { log } = await import('../utils/logger.js'); + vi.mocked(log.warn).mockClear(); + + await handler.pullItem(item, teamConfig, { ...localConfig, projects: ['checkout'] }); + + const content = await envSh(); + expect(content).toContain("export MODEL_ENDPOINT='https://api.example.com'"); + expect(content).not.toContain('https://checkout.example.com'); + const warnings = vi.mocked(log.warn).mock.calls.map(([m]) => String(m)) + .filter((m) => m.includes('env/checkout/env.yaml') && m.includes('"MODEL_ENDPOINT"')); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toMatch(/\brole\b/); + }); + + it('delivers a variable with every key env knows, without a warning about it', async () => { + await fse.writeFile(path.join(repoPath, 'env', 'env.yaml'), YAML.stringify({ + variables: [{ key: 'FULL', value: 'f', description: 'every known key' }], + })); + const { log } = await import('../utils/logger.js'); + vi.mocked(log.warn).mockClear(); + + await handler.pullItem(item, teamConfig, localConfig); + + expect(await envSh()).toContain("export FULL='f'"); + expect(vi.mocked(log.warn).mock.calls.map(([m]) => String(m)).filter((m) => m.includes('"FULL"'))).toEqual([]); + }); + it('should handle invalid env.yaml gracefully', async () => { await fse.writeFile(path.join(repoPath, 'env', 'env.yaml'), ':::bad yaml'); diff --git a/src/__tests__/hooks-handler.test.ts b/src/__tests__/hooks-handler.test.ts index 606d130c4..279311f52 100644 --- a/src/__tests__/hooks-handler.test.ts +++ b/src/__tests__/hooks-handler.test.ts @@ -148,9 +148,11 @@ hooks: expect(await parseTeamHooks(repo)).toBeNull(); }); - it('skips the whole file on malformed yaml', async () => { + // It parses as a mapping with no `hooks:` key, which read as no hooks and + // removed every installed one (#822). + it('fails the whole file on malformed yaml that parses as a mapping without hooks:', async () => { await writeHooksYaml(':::not yaml:::\n - broken'); - expect(await parseTeamHooks(repo)).toEqual([]); + expect(await parseTeamHooks(repo)).toBeNull(); }); it('fails the whole file when it is not YAML at all', async () => { diff --git a/src/__tests__/hooks-reconcile-scope.test.ts b/src/__tests__/hooks-reconcile-scope.test.ts index be015e1a0..c4a400ec1 100644 --- a/src/__tests__/hooks-reconcile-scope.test.ts +++ b/src/__tests__/hooks-reconcile-scope.test.ts @@ -248,6 +248,47 @@ hooks: expect((await manifest()).claude.map((r) => r.id)).toEqual(['lint']); }); + // #822: `hook:` for `hooks:` parsed as "no hooks" and removed every installed + // team hook, with no warning. + it('keeps the installed team hooks when hooks.yaml has no top-level hooks: key, and names the file and key', async () => { + const lint = '\n - id: lint\n description: lint\n event: Stop\n command: npm run lint\n'; + await writeYaml(`hooks:${lint}`); + await reconcileTeamHooksForConfig(teamConfig, localConfig()); + const before = await claudeSettings(); + const { log } = await import('../utils/logger.js'); + vi.mocked(log.warn).mockClear(); + + await writeYaml(`hook:${lint}`); + const applied = await reconcileTeamHooksForConfig(teamConfig, localConfig()); + + expect(applied).toEqual({ ok: false, builtins: 'defaults-where-none' }); + expect(await claudeSettings()).toEqual(before); + expect((await manifest()).claude.map((r) => r.id)).toEqual(['lint']); + const warnings = vi.mocked(log.warn).mock.calls.map(([m]) => String(m)); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('hooks/hooks.yaml'); + expect(warnings[0]).toContain('`hook`'); + expect(warnings[0]).toContain('`hooks:`'); + }); + + it('still applies the hooks of a hooks.yaml that carries an extra top-level key', async () => { + await writeYaml('version: 1\nhooks:\n - id: lint\n description: lint\n event: Stop\n command: npm run lint\n'); + + const applied = await reconcileTeamHooksForConfig(teamConfig, localConfig()); + + expect(applied.ok).toBe(true); + expect((await manifest()).claude.map((r) => r.id)).toEqual(['lint']); + }); + + it('still reads a hooks.yaml that declares only builtin: overrides', async () => { + await writeYaml('builtin:\n overrides:\n Hook dispatch stop: { timeout: 99 }\n'); + + const applied = await reconcileTeamHooksForConfig(teamConfig, localConfig()); + + expect(applied.ok).toBe(true); + expect((await codexSettings()).hooks.Stop[0].hooks[0].timeout).toBe(99); + }); + // #707: a first install whose team hooks do not resolve still gets the // built-in hooks, above all the session-start pull that heals the member. it('installs the built-in hooks with the root overrides on a first install whose namespaces clash', async () => { diff --git a/src/__tests__/hooks-security.test.ts b/src/__tests__/hooks-security.test.ts index 2ac15f0ab..cc7141792 100644 --- a/src/__tests__/hooks-security.test.ts +++ b/src/__tests__/hooks-security.test.ts @@ -309,3 +309,79 @@ projects: )); }); }); + +// A typo of a scoping key (#822) must not widen who runs the hook: the hook +// reaches nobody and the warning names the file, the hook and the key. +describe('resolveTeamHooks — unknown entry keys', () => { + const warningsAbout = (file: string, id: string): string[] => + logWarn.mock.calls.map(([m]) => String(m)).filter((m) => m.includes(file) && m.includes(`"${id}"`)); + + it('delivers no hook that carries a key hooks do not know, and names the file, hook and key', async () => { + await writeYaml(` +hooks: + - id: fe-lint + description: frontend only + event: Stop + command: 'bash -lc "~/.teamai/team-scripts/ok.sh" || true' + role: [frontend] + - id: everyone + description: for all + event: Stop + command: 'bash -lc "~/.teamai/team-scripts/ok.sh" || true' +`); + const defs = await defsFor(teamConfig(), member(), { auto: true }); + expect(defs.map((d) => d.key)).toEqual(['everyone']); + const warnings = warningsAbout('hooks/hooks.yaml', 'fe-lint'); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toMatch(/\brole\b/); + }); + + it('keeps the root hook when the active namespace copy of it carries an unknown key', async () => { + await writeYaml(` +hooks: + - id: lint + description: root + event: Stop + command: 'bash -lc "~/.teamai/team-scripts/root-lint.sh" || true' +`); + await fse.outputFile(path.join(repo, 'hooks', 'checkout', 'hooks.yaml'), ` +hooks: + - id: lint + description: checkout + event: Stop + command: 'bash -lc "~/.teamai/team-scripts/checkout-lint.sh" || true' + role: [frontend] +`); + await fse.outputFile(path.join(repo, 'manifest', 'projects.yaml'), ` +version: 1 +projects: + - id: checkout + resources: { hooks: [checkout] } +`); + const defs = await defsFor(teamConfig(), member({ projects: ['checkout'] }), { auto: true }); + expect(defs.map((d) => d.command)).toEqual(['bash -lc "~/.teamai/team-scripts/root-lint.sh" || true']); + const warnings = warningsAbout('hooks/checkout/hooks.yaml', 'lint'); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toMatch(/\brole\b/); + }); + + it('delivers a hook with every key hooks know, with no warning beyond the roles: deprecation', async () => { + await writeYaml(` +hooks: + - id: full + description: every known key + event: PreToolUse + matcher: Bash + command: 'bash -lc "~/.teamai/team-scripts/ok.sh" || true' + timeout: 30 + tools: [claude] + roles: [frontend] +`); + await writeRolesYaml(); + const defs = await defsFor(teamConfig(), member({ primaryRole: 'frontend' }), { auto: true }); + expect(defs.map((d) => d.key)).toEqual(['full']); + const warnings = warningsAbout('hooks/hooks.yaml', 'full'); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('is scoped with per-entry `roles:`, which is deprecated'); + }); +}); diff --git a/src/__tests__/mcp-handler.test.ts b/src/__tests__/mcp-handler.test.ts index 23fafb97b..6703269dd 100644 --- a/src/__tests__/mcp-handler.test.ts +++ b/src/__tests__/mcp-handler.test.ts @@ -7,9 +7,10 @@ vi.mock('../utils/logger.js', () => ({ log: { info: vi.fn(), success: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn(), persist: vi.fn() }, })); -import { mcpEntryReader } from '../resources/mcp.js'; +import YAML from 'yaml'; +import { McpHandler, mcpEntryReader } from '../resources/mcp.js'; import { resolveEntriesFor } from '../namespaced-entries.js'; -import type { LocalConfig } from '../types.js'; +import type { LocalConfig, TeamaiConfig } from '../types.js'; let repo: string; @@ -85,3 +86,35 @@ servers: expect((await serversFor(member({ projects: ['checkout'] }))).map((s) => s.name)).toEqual(['shared']); }); }); + +describe('McpHandler.removeItem', () => { + // A server with a misspelled `roles:` reaches nobody (#822); a rewrite that + // drops the key would install it for the whole team. + it('keeps a key MCP does not know on the servers it leaves', async () => { + await writeMcpYaml(` +servers: + - name: gone + transport: http + url: https://example.com/gone + - name: fe-db + transport: http + url: https://example.com/fe-db + role: [frontend] +`); + const teamConfig: TeamaiConfig = { + team: 'test', + description: '', + repo: 'owner/repo', + provider: 'github', + reviewers: [], + sharing: { skills: {}, rules: { enforced: [] }, docs: { localDir: '' }, env: { injectShellProfile: true } }, + toolPaths: {}, + }; + await new McpHandler().removeItem('gone', teamConfig, member()); + + const written = YAML.parse(await fse.readFile(path.join(repo, 'mcp', 'mcp.yaml'), 'utf-8')); + expect(written.servers).toEqual([ + { name: 'fe-db', transport: 'http', url: 'https://example.com/fe-db', role: ['frontend'] }, + ]); + }); +}); diff --git a/src/__tests__/mcp-reconcile.test.ts b/src/__tests__/mcp-reconcile.test.ts index 7e92ae9cb..9ad27eb4f 100644 --- a/src/__tests__/mcp-reconcile.test.ts +++ b/src/__tests__/mcp-reconcile.test.ts @@ -277,6 +277,46 @@ servers: expect(after.mcpServers.temp).toBeUndefined(); }); + // #822: `server:` for `servers:` parsed as "no servers" and uninstalled every + // team server for every member, with no warning. + it('keeps every installed server when mcp.yaml has no top-level servers: key, and names the file and key', async () => { + const temp = ` + - name: temp + transport: http + url: https://example.com/mcp + tools: [claude] +`; + await writeMcpYaml(`servers:${temp}`); + await reconcileMcpForConfig(teamConfig, localConfig); + const { log } = await import('../utils/logger.js'); + vi.mocked(log.warn).mockClear(); + + await writeMcpYaml(`server:${temp}`); + const { changes } = await reconcileMcpForConfig(teamConfig, localConfig); + + expect(changes).toEqual([]); + expect((await fse.readJson(path.join(homeDir, '.claude.json'))).mcpServers.temp).toBeDefined(); + const warnings = vi.mocked(log.warn).mock.calls.map(([m]) => String(m)); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('mcp/mcp.yaml'); + expect(warnings[0]).toContain('`server`'); + expect(warnings[0]).toContain('`servers:`'); + }); + + it('still installs the servers of an mcp.yaml that carries an extra top-level key', async () => { + await writeMcpYaml(` +version: 1 +servers: + - name: temp + transport: http + url: https://example.com/mcp + tools: [claude] +`); + await reconcileMcpForConfig(teamConfig, localConfig); + + expect((await fse.readJson(path.join(homeDir, '.claude.json'))).mcpServers.temp).toBeDefined(); + }); + // roles: on a server shipped in 0.25.0 and keeps filtering for one minor // release (#707); the warning names the namespace file it belongs in. describe('deprecated roles filter', () => { @@ -534,6 +574,84 @@ servers: + 'so it reaches nobody. Move it to mcp/checkout/mcp.yaml and drop the key.', )); }); + + // A typo of a scoping key (#822) must not widen who gets the server. + const warningsAbout = async (file: string, server: string): Promise => { + const { log } = await import('../utils/logger.js'); + return vi.mocked(log.warn).mock.calls.map(([m]) => String(m)) + .filter((m) => m.includes(file) && m.includes(`"${server}"`)); + }; + + it('installs no server that carries a key MCP does not know, and names the file, server and key', async () => { + await writeMcpYaml(` +servers: + - name: fe-db + transport: http + url: https://example.com/fe-db + role: [frontend] + - name: shared + transport: http + url: https://example.com/shared +`); + const { log } = await import('../utils/logger.js'); + vi.mocked(log.warn).mockClear(); + resetWarnOnce(); + + await reconcileMcpForConfig(teamConfig, { ...localConfig, projects: ['billing'] }); + + expect(Object.keys(await claudeServers()).sort()).toEqual(['shared']); + const warnings = await warningsAbout('mcp/mcp.yaml', 'fe-db'); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toMatch(/\brole\b/); + }); + + it('keeps the root server when the active namespace copy of it carries an unknown key', async () => { + await writeNamespaceMcp('checkout', ` +servers: + - name: db + transport: http + url: https://checkout.example.com/db + role: [frontend] +`); + const { log } = await import('../utils/logger.js'); + vi.mocked(log.warn).mockClear(); + resetWarnOnce(); + + await reconcileMcpForConfig(teamConfig, { ...localConfig, projects: ['checkout'] }); + + expect((await claudeServers()).db?.url).toBe('https://example.com/db'); + const warnings = await warningsAbout('mcp/checkout/mcp.yaml', 'db'); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toMatch(/\brole\b/); + }); + + it('installs a server with every key MCP knows, with no warning beyond the roles: deprecation', async () => { + await writeMcpYaml(` +servers: + - name: full + description: every known key + transport: stdio + command: node + args: [server.js] + url: https://example.com/unused + headers: { X-Team: t } + env: { LEVEL: debug } + timeout: 30 + requires: [node] + tools: [claude] + roles: [frontend] +`); + const { log } = await import('../utils/logger.js'); + vi.mocked(log.warn).mockClear(); + resetWarnOnce(); + + await reconcileMcpForConfig(teamConfig, { ...localConfig, projects: ['billing'] }); + + expect(Object.keys(await claudeServers())).toEqual(['full']); + const warnings = await warningsAbout('mcp/mcp.yaml', 'full'); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('is scoped with per-entry `roles:`, which is deprecated'); + }); }); it('does not prune managed servers in http mode (install_mcp survives second sync)', async () => { diff --git a/src/__tests__/recall-format.test.ts b/src/__tests__/recall-format.test.ts index db81c236f..d0a42aebf 100644 --- a/src/__tests__/recall-format.test.ts +++ b/src/__tests__/recall-format.test.ts @@ -172,3 +172,21 @@ describe('formatResults — a path that no longer exists', () => { } }); }); + +describe('formatResults — closing line', () => { + // CLI output is English only (#822). + it('ends with the English reference note and prints no CJK text', () => { + const output = formatResults([ + { + entry: makeEntry({ title: 'Regular Learning' }), + score: 6.0, + scope: 'user', + }, + ]); + + expect(output.split('\n').at(-1)).toBe( + 'The above comes from the team knowledge base and is for reference only. Use the Read tool to open the listed files for details.', + ); + expect(output).not.toMatch(/[\u3400-\u9fff]/); + }); +}); diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts index 18b2b6bbc..885f142b2 100644 --- a/src/doctor-delivery.ts +++ b/src/doctor-delivery.ts @@ -500,11 +500,12 @@ export async function buildMcpDeliveryChecks(ctx: DoctorContext): Promise { const messages = (await resolveEntryTypes(ctx.localConfig)) @@ -513,7 +514,7 @@ export async function buildEntryScopeKeyCheck(ctx: DoctorContext): Promise notice.message); if (messages.length === 0) return []; return [{ - name: 'Team env, hooks and MCP are scoped by namespace files, not per-entry keys', + name: 'Team env, hooks and MCP entries have no per-entry key to fix', source: 'local', informational: true, check: async () => false, diff --git a/src/env-commands.ts b/src/env-commands.ts index 65cf0c2b3..b3c3432a6 100644 --- a/src/env-commands.ts +++ b/src/env-commands.ts @@ -2,7 +2,7 @@ import { requireInit, detectProjectConfig } from './config.js'; import { pullRepo } from './utils/git.js'; import { pathExists } from './utils/fs.js'; import { log, spinner } from './utils/logger.js'; -import { EnvHandler, maskEnvValue, ENV_KEY_RE, envEntryReader, type EnvYaml } from './resources/env.js'; +import { EnvHandler, maskEnvValue, ENV_KEY_RE, envEntryReader, unknownEnvVariableKeys, type EnvYaml } from './resources/env.js'; import { describeEntryFailure, describeOrigin, entryFileAbsolutePath, entryFilePath, entryNamespaceFromFlags, resolveEntriesFor } from './namespaced-entries.js'; import type { GlobalOptions, LocalConfig } from './types.js'; import { isSelfMode } from './types.js'; @@ -77,7 +77,7 @@ export async function envAdd( const target = await envFileFromFlags(repoPath, options); if (!target) return; - const { envYamlPath, where } = target; + const { envYamlPath, relativePath, where } = target; // The target env.yaml, or a new one when it does not exist. const envConfig = await readEnvFileForEdit(envYamlPath); @@ -92,6 +92,16 @@ export async function envAdd( if (options.description) { envConfig.variables[existingIdx].description = options.description; } + // The update keeps an unknown key, so the variable stays undelivered. + const unknown = unknownEnvVariableKeys(envConfig.variables[existingIdx]); + if (unknown.length > 0) { + const one = unknown.length === 1; + log.warn( + `${relativePath}: variable "${key}" has unknown ${one ? 'key' : 'keys'} ` + + `${unknown.map((k) => `\`${k}:\``).join(', ')}, so pull does not deliver it. ` + + `Correct the ${one ? 'key' : 'keys'} or remove ${one ? 'it' : 'them'} in ${relativePath}.`, + ); + } } else { const newVar: { key: string; value: string; description?: string } = { key, value }; if (options.description) { diff --git a/src/namespaced-entries.ts b/src/namespaced-entries.ts index 505882076..bc99c6622 100644 --- a/src/namespaced-entries.ts +++ b/src/namespaced-entries.ts @@ -78,7 +78,13 @@ export async function listEntryFiles(repoPath: string, type: EntryType): Promise /** One parsed file, or why it cannot be used; the reason names the file. */ export type EntryFileRead = - | { readonly ok: true; readonly entries: readonly E[]; readonly notes?: readonly string[] } + | { + readonly ok: true; + readonly entries: readonly E[]; + readonly notes?: readonly string[]; + /** The keys an entry carries that its schema does not know, for the entries that carry any. */ + readonly unknownKeys?: ReadonlyMap; + } | { readonly ok: false; readonly reason: string }; /** The per-entry scoping keys the namespaces replace. */ @@ -87,6 +93,54 @@ export interface EntryScopeKeys { readonly projects?: readonly string[]; } +/** The list under `listKey` of a parsed file, as written, before its schema drops any key. */ +export function writtenList(document: unknown, listKey: string): unknown { + if (document === null || typeof document !== 'object') return undefined; + const fields: [string, unknown][] = Object.entries(document); + return fields.find(([key]) => key === listKey)?.[1]; +} + +/** + * The keys each entry was written with that `schema` does not know, for the + * entries that have any. zod drops such a key without a word, so a misspelled + * `roles:` would send the entry to every member (#822). `entries` is the + * `listKey` list of `document`, parsed, in the same order. + */ +export function unknownEntryKeys( + document: unknown, + listKey: string, + entries: readonly E[], + schema: { readonly shape: object }, +): Map { + const byEntry = new Map(); + const raw = writtenList(document, listKey); + if (!Array.isArray(raw)) return byEntry; + const known = Object.keys(schema.shape); + entries.forEach((entry, index) => { + const written: unknown = raw[index]; + if (written === null || typeof written !== 'object') return; + const unknown = Object.keys(written).filter((key) => !known.includes(key)); + if (unknown.length > 0) byEntry.set(entry, unknown); + }); + return byEntry; +} + +/** + * Why a file that has none of its schema's top-level keys cannot be read, or + * null when it has one (or is not a non-empty mapping). zod defaults the + * missing list to empty and drops the key it does not know, so `server:` for + * `servers:` read as "no entries" and removed every installed one (#822). An + * extra key beside a known one stays permitted, as for env.yaml (#662). + */ +export function missingTopLevelKeyReason(document: unknown, schema: { readonly shape: object }): string | null { + if (document === null || typeof document !== 'object' || Array.isArray(document)) return null; + const found = Object.keys(document); + const expected = Object.keys(schema.shape); + if (found.length === 0 || found.some((key) => expected.includes(key))) return null; + const quote = (keys: string[], suffix = ''): string => keys.map((key) => `\`${key}${suffix}\``).join(', '); + return `it has no top-level ${quote(expected, ':').replace(/, (?=[^,]*$)/, ' or ')} key, only ${quote(found)}`; +} + /** How one type's files are read. */ /** * The text of one type file, null when it does not exist. Any other read error @@ -141,7 +195,7 @@ export type EntryFailure = /** A warning about an entry that still resolves, worded for the admin who can fix it. */ export interface EntryNotice { - readonly kind: 'removed-key' | 'deprecated-roles' | 'file-note'; + readonly kind: 'unknown-key' | 'removed-key' | 'deprecated-roles' | 'file-note'; readonly message: string; } @@ -220,7 +274,8 @@ export async function resolveEntries( for (const entry of read.entries) { const name = reader.nameOf(entry); const scope = reader.scopeOf(entry); - if (!await keepScopedEntry(type, name, source, scope, localConfig, targets, notices)) continue; + const unknownKeys = read.unknownKeys?.get(entry) ?? []; + if (!await keepScopedEntry(type, name, source, scope, unknownKeys, localConfig, targets, notices)) continue; const candidate = { name, source, namespace, value: entry }; candidates.push(candidate); if (scope.roles !== undefined) roleScoped.add(candidate); @@ -312,6 +367,9 @@ export async function resolveEntriesFor( * Whether an entry's per-entry keys let it through, recording a notice when it * carries one. * + * A key the schema does not know (`unknownKeys`) may be a misspelled scoping + * key, so such an entry is not delivered, as with a removed key. + * * `projects:` (every type) and `roles:` on env exist only in the 0.26.0 betas * and are removed: such an entry reaches nobody, which is the direction that * cannot leak a project's value to the whole team. `roles:` on hooks and MCP @@ -322,11 +380,23 @@ async function keepScopedEntry( name: string, source: string, scope: EntryScopeKeys, + unknownKeys: readonly string[], localConfig: LocalConfig, targets: TargetFiles, notices: EntryNotice[], ): Promise { const label = `${source}: ${ENTRY_NOUN[type]} "${name}"`; + if (unknownKeys.length > 0) { + const one = unknownKeys.length === 1; + const keys = unknownKeys.map((key) => `\`${key}:\``).join(', '); + notices.push({ + kind: 'unknown-key', + message: `${label} has unknown ${one ? 'key' : 'keys'} ${keys}, so this entry is not delivered. ` + + `Correct the ${one ? 'key' : 'keys'} or remove ${one ? 'it' : 'them'}.`, + }); + return false; + } + const removedKeys: ('projects' | 'roles')[] = []; if (scope.projects !== undefined) removedKeys.push('projects'); if (type === 'env' && scope.roles !== undefined) removedKeys.push('roles'); diff --git a/src/recall.ts b/src/recall.ts index da5338477..415d3a0ba 100644 --- a/src/recall.ts +++ b/src/recall.ts @@ -233,7 +233,7 @@ export function formatResults(results: ScopedSearchResult[]): string { lines.push('--- [teamai:recall:end] ---'); lines.push(''); - lines.push('以上内容来自团队知识库，仅供参考。如需详细信息，请用 Read 工具读取对应文件。'); + lines.push('The above comes from the team knowledge base and is for reference only. Use the Read tool to open the listed files for details.'); return lines.join('\n'); } diff --git a/src/resources/env.ts b/src/resources/env.ts index 2657150be..2b386e133 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -8,7 +8,7 @@ import { pathExists, readFileSafe, writeFile, ensureDir, fileContentEqual } from import { log } from '../utils/logger.js'; import { entryFileAbsolutePath, listEntryFiles, readEntryFileText, reportEntryResolution, resolveEntriesFor, - type EntryReader, + unknownEntryKeys, writtenList, type EntryReader, } from '../namespaced-entries.js'; import { resolveActiveShellProfile, @@ -37,6 +37,11 @@ const EnvYamlSchema = z.object({ export type EnvVariable = z.infer; export type EnvYaml = z.infer; +/** The keys `variable` was written with that env.yaml does not know: pull does not deliver it (#822). */ +export function unknownEnvVariableKeys(variable: object): string[] { + return Object.keys(variable).filter((key) => !Object.hasOwn(EnvVariableSchema.shape, key)); +} + /** A parsed env.yaml, or the reason it declares nothing. See `readEnvYaml`. */ export type EnvYamlRead = | { ok: true; variables: EnvVariable[] } @@ -63,7 +68,8 @@ export const envEntryReader: EntryReader = { const shapeProblem = describeEnvYamlShapeProblem(raw); if (shapeProblem) return { ok: false, reason: `${relativePath} declares no variables: ${shapeProblem}` }; const read = parseEnvYamlDocument(raw, relativePath); - return read.ok ? { ok: true, entries: read.variables } : read; + if (!read.ok) return read; + return { ok: true, entries: read.variables, unknownKeys: unknownEntryKeys(raw, 'variables', read.variables, EnvVariableSchema) }; }, nameOf: (variable) => variable.key, scopeOf: (variable) => variable, @@ -362,7 +368,18 @@ export class EnvHandler extends ResourceHandler { } catch (e) { return { ok: false, reason: `${filePath} is not valid YAML: ${(e as Error).message}` }; } - return parseEnvYamlDocument(raw, filePath); + const read = parseEnvYamlDocument(raw, filePath); + if (!read.ok) return read; + // A rewrite keeps every key a variable was written with: dropping a + // misspelled `roles:` would deliver the variable to every member (#822). + const written = writtenList(raw, 'variables'); + return { + ok: true, + variables: read.variables.map((variable, index) => { + const entry: unknown = Array.isArray(written) ? written[index] : undefined; + return entry !== null && typeof entry === 'object' ? { ...entry, ...variable } : variable; + }), + }; } /** diff --git a/src/resources/hooks.ts b/src/resources/hooks.ts index 3444af26c..c26839883 100644 --- a/src/resources/hooks.ts +++ b/src/resources/hooks.ts @@ -7,7 +7,7 @@ import { TEAMAI_CUSTOM_HOOK_PREFIX, areTeamHooksDisabled, getHooksSharing } from import { pathExists } from '../utils/fs.js'; import { log } from '../utils/logger.js'; import { - entryFilePath, readEntryFileText, reportEntryResolution, resolveEntriesFor, + entryFilePath, missingTopLevelKeyReason, readEntryFileText, reportEntryResolution, resolveEntriesFor, unknownEntryKeys, type EntryReader, type EntryResolution, } from '../namespaced-entries.js'; @@ -60,7 +60,9 @@ export function teamHooksYamlPath(repoPath: string): string { } /** One hooks file, parsed, or why it cannot be used. */ -type HooksFileRead = { ok: true; yaml: HooksYaml; declaresBuiltin: boolean } | { ok: false; reason: string }; +type HooksFileRead = + | { ok: true; yaml: HooksYaml; declaresBuiltin: boolean; unknownKeys: ReadonlyMap } + | { ok: false; reason: string }; /** Read one hooks file; null when it does not exist. */ async function readHooksFile(absolutePath: string, relativePath: string): Promise { @@ -70,8 +72,11 @@ async function readHooksFile(absolutePath: string, relativePath: string): Promis if (content === null) return null; try { const raw: unknown = YAML.parse(content); + const shapeProblem = missingTopLevelKeyReason(raw, HooksYamlSchema); + if (shapeProblem) return { ok: false, reason: `${relativePath} does not parse: ${shapeProblem}` }; const declaresBuiltin = !!raw && typeof raw === 'object' && 'builtin' in raw; - return { ok: true, yaml: HooksYamlSchema.parse(raw ?? {}), declaresBuiltin }; + const yaml = HooksYamlSchema.parse(raw ?? {}); + return { ok: true, yaml, declaresBuiltin, unknownKeys: unknownEntryKeys(raw, 'hooks', yaml.hooks, TeamHookSchema) }; } catch (e) { return { ok: false, reason: `${relativePath} does not parse: ${e instanceof Error ? e.message : String(e)}` }; } @@ -89,7 +94,7 @@ export const hooksEntryReader: EntryReader = { const notes = read.declaresBuiltin && relativePath !== entryFilePath('hooks', null) ? [`${relativePath}: \`builtin:\` is ignored outside hooks/hooks.yaml; built-in hook overrides apply to the whole team. Move it there.`] : []; - return { ok: true, entries: read.yaml.hooks, notes }; + return { ok: true, entries: read.yaml.hooks, notes, unknownKeys: read.unknownKeys }; }, nameOf: (hook) => hook.id, scopeOf: (hook) => hook, diff --git a/src/resources/mcp.ts b/src/resources/mcp.ts index 6dbe49a25..6c63d471f 100644 --- a/src/resources/mcp.ts +++ b/src/resources/mcp.ts @@ -5,7 +5,7 @@ import type { ResourceItem, TeamaiConfig, LocalConfig, McpServerDef } from '../t import { readFileSafe, writeFile } from '../utils/fs.js'; import { log } from '../utils/logger.js'; import { - entryFileAbsolutePath, entryFilePath, listEntryFiles, readEntryFileText, + entryFileAbsolutePath, entryFilePath, listEntryFiles, missingTopLevelKeyReason, readEntryFileText, unknownEntryKeys, type EntryReader, } from '../namespaced-entries.js'; @@ -15,7 +15,7 @@ import { // names; each tool's own spelling (the claude/cursor/codebuddy `type` field, // Codex's TOML table) is applied at render time by mcp-format.ts. -const TeamMcpServerSchema = z +const TeamMcpServerFields = z .object({ name: z.string().regex(/^[A-Za-z0-9_-]+$/, 'name must be alphanumeric with - or _'), description: z.string().optional(), @@ -32,7 +32,9 @@ const TeamMcpServerSchema = z roles: z.array(z.string()).optional(), /** Removed (0.26.0 betas only): kept so it is detected; such a server reaches nobody. */ projects: z.array(z.string()).optional(), - }) + }); + +const TeamMcpServerSchema = TeamMcpServerFields .refine((s) => (s.transport === 'stdio' ? !!s.command : true), { message: 'stdio transport requires `command`', }) @@ -54,7 +56,7 @@ export type McpYaml = z.infer; * whose MCP is entirely broken (#624 review). */ type McpYamlRead = - | { ok: true; yaml: McpYaml | null } + | { ok: true; yaml: McpYaml | null; unknownKeys: ReadonlyMap } | { ok: false; reason: string }; /** Read one MCP file, keeping why it cannot be used; `yaml: null` when absent. */ @@ -64,9 +66,13 @@ async function readMcpFile(absolutePath: string): Promise { /** Parse one MCP file's text; `yaml: null` when it is absent or empty. */ function parseMcpContent(content: string | null): McpYamlRead { - if (!content) return { ok: true, yaml: null }; + if (!content) return { ok: true, yaml: null, unknownKeys: new Map() }; try { - return { ok: true, yaml: McpYamlSchema.parse(YAML.parse(content)) }; + const raw: unknown = YAML.parse(content); + const shapeProblem = missingTopLevelKeyReason(raw, McpYamlSchema); + if (shapeProblem) return { ok: false, reason: shapeProblem }; + const yaml = McpYamlSchema.parse(raw); + return { ok: true, yaml, unknownKeys: unknownEntryKeys(raw, 'servers', yaml.servers, TeamMcpServerFields) }; } catch (e) { return { ok: false, reason: (e as Error).message }; } @@ -80,7 +86,7 @@ export const mcpEntryReader: EntryReader = { if (!file.ok) return file; const read = parseMcpContent(file.text); if (!read.ok) return { ok: false, reason: `${relativePath} does not parse: ${read.reason}` }; - return read.yaml === null ? null : { ok: true, entries: read.yaml.servers }; + return read.yaml === null ? null : { ok: true, entries: read.yaml.servers, unknownKeys: read.unknownKeys }; }, nameOf: (server) => server.name, scopeOf: (server) => server, @@ -164,16 +170,22 @@ export class McpHandler extends ResourceHandler { const namespace = slash === -1 ? null : name.slice(0, slash); const serverName = slash === -1 ? name : name.slice(slash + 1); const yamlPath = entryFileAbsolutePath(localConfig.repo.localPath, 'mcp', namespace); - const read = await readMcpFile(yamlPath); + const content = await readFileSafe(yamlPath); + const read = parseMcpContent(content); if (!read.ok) { log.warn(`${entryFilePath('mcp', namespace)} does not parse, so "${serverName}" was not removed: ${read.reason}`); return []; } const servers = read.yaml?.servers ?? []; - const remaining = servers.filter((s) => s.name !== serverName); - if (remaining.length === servers.length) return []; - - await writeFile(yamlPath, YAML.stringify({ servers: remaining })); + const removed = servers.flatMap((s, index) => (s.name === serverName ? [index] : [])); + if (content === null || removed.length === 0) return []; + + // Edit the document rather than write the parsed servers back: the schema + // drops a key it does not know, and dropping a misspelled `roles:` would + // install that server for every member (#822). + const doc = YAML.parseDocument(content); + for (const index of removed.reverse()) doc.deleteIn(['servers', index]); + await writeFile(yamlPath, doc.toString()); await this.addTombstone(serverName, localConfig); return [yamlPath]; }