diff --git a/CHANGELOG.md b/CHANGELOG.md index ede459704..d2abe7fdc 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 ### ✨ Features +- `teamai doctor` now checks what landed for every resource, not only skills and docs. `Rules delivered to ` and `Agents delivered to ` ask the resource handler where an item lands — a rule's filename and content change per tool, an agent's destination comes from its render and its `targets:` — and compare a delivered rule with the bytes the handler renders for that tool, so a `.mdc` whose `globs` drifted from the team rule's `paths:` is reported rather than passing on the presence of its frontmatter keys. An agent is compared with the bytes its render produces, so a copy left behind by an older spec is reported rather than counted as delivered. `Every team agent reaches a tool` names an agent that renders for no installed tool, and is reported whenever a tool is installed to receive agents, including when no agent renders anywhere. Two tools do not read a rules directory and get a check each: `Team rules are active in opencode` fails when `opencode.json` stops listing the glob that makes the delivered `.md` files load at all, and `Team rules are inlined in Hermes SOUL.md` compares the managed block of `SOUL.md` with what the team rules inline to. `MCP servers delivered to ` compares each server the team resolves for a tool with the entry in that tool's own config — the entry, not the name, since reconciliation leaves an entry teamai does not own alone, so an unrelated server under a team name holds the key while the team's definition never arrives — and names any the reconcile skipped with its reason, so an unresolved `${VAR}` is reported with the variable instead of being mentioned once during a pull and never again. An `mcp.yaml` that does not parse is reported as `Team MCP servers can be read` rather than read as a team shipping no MCP at all. `Env variables injected in shell profile` stops at the marker comment no longer: it checks that `env/env.yaml` parses and declares its variables under `variables:` (an explicit `variables: []` is an empty configuration and fails nothing), that each reached `env.sh` with the declared value — read back through the generator's own inverse, so a multiline value quoted across several lines is matched rather than reported stale — and that the injected block would actually load it. The two expensive registries, rules and agents, are built for `teamai doctor` only, so the checks at the end of a pull keep their budget (for [#624](https://github.com/Tencent/teamai-cli/issues/624)). - A manual `teamai pull` ends by running the `teamai doctor` checks and printing each one that failed, with its fix. It prints nothing when they all pass, the exit code is unchanged, and the SessionStart hook path (`--silent`) and `--dry-run` run no checks, so session startup is untouched. Provider authentication checks are left to `teamai doctor`: the pull just used the provider. So is any check that pull already reported in its own words on that run — the queued-learnings warning is not immediately repeated as a check telling you to run the pull you just ran. A check the pull stayed silent about is still printed (for [#598](https://github.com/Tencent/teamai-cli/issues/598)). - `teamai doctor` now checks what landed, not only the plumbing. `Skills delivered to ` compares the skills your roles, tag subscriptions and exclusions resolve to against each installed tool's directory, reporting a skill that never arrived separately from one that arrived unreadable (`SKILL.md` missing, unparseable frontmatter, or a `name` that does not match the directory, which keeps the agent from discovering it). `Team docs delivered` does the same for the docs bundle against `sharing.docs.localDir`. ` is installed` fails when `enabledAgents` lists a tool with no directory here, instead of skipping it silently, and reports an installed one as passing so `--json` carries an entry either way. Resolving a skill's destination without a team copy to compare against no longer warns about a Codex shared-directory conflict, so a read-only `doctor` stops reporting one for copies the pull treats as identical. The installed check asks the same resolver the sync uses, so OpenClaw is judged at its workspace directory rather than its tool root. `Team docs delivered` requires each expected document to be a readable file, not merely a name that exists. And a pull that found a scope locked by another process runs no checks at the end, since they would read a clone that process may have mid-write (for [#598](https://github.com/Tencent/teamai-cli/issues/598)). - `teamai remove` accepts `--force` to skip its confirmation prompt, spelled the same way as `teamai uninstall --force`. Without a TTY the prompt answers itself with no, so this is the only way to remove a resource from a script or a test (for [#591](https://github.com/Tencent/teamai-cli/issues/591)). diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 20d229721..3979faab4 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -1524,7 +1524,13 @@ teamai remove rules --force # Skip the prompt, for scripts and CI `teamai doctor` exits with code 0 only when every check passes, and code 1 when any check fails. Before initialization, it reports the missing configuration without assuming a Git provider. The same checks run at the end of a manual `teamai pull`, minus the provider ones and minus any check that pull already reported in its own words on that run. -Besides the provider, clone, config, hook and env checks, `doctor` verifies three things about what reached your machine. ` is installed` fails when `enabledAgents` lists a tool that nothing would be delivered to, which is the case where a pull reports success and that tool receives nothing. It asks the same resolver the sync uses, so a tool that keeps its skills somewhere other than its tool root, as OpenClaw does with its workspace directory, is judged where the sync would actually write. It reports an installed tool as passing too, so `--json` carries one entry per enabled tool either way. The checks at the end of a pull cover the scope that pull resolved from the current directory; run `teamai doctor` in another scope to check that one. `Skills delivered to ` compares the skills your role namespaces, tag subscriptions and exclusions resolve to against what is on disk for each installed tool: it reports a skill that was never delivered separately from one that arrived unreadable — `SKILL.md` missing, its frontmatter unparseable, or its `name` not matching the directory, which keeps the agent from ever discovering it. `Team docs delivered` compares the docs bundle against `sharing.docs.localDir`, which has one destination rather than one per tool; each expected document has to be a file that can be read, so a directory or a dangling link sitting on the name counts as missing. Rules, agents and MCP servers are not checked yet. +Besides the provider, clone, config and hook checks, `doctor` verifies what reached your machine. ` is installed` fails when `enabledAgents` lists a tool that nothing would be delivered to, which is the case where a pull reports success and that tool receives nothing. It asks the same resolver the sync uses, so a tool that keeps its skills somewhere other than its tool root, as OpenClaw does with its workspace directory, is judged where the sync would actually write. It reports an installed tool as passing too, so `--json` carries one entry per enabled tool either way. The checks at the end of a pull cover the scope that pull resolved from the current directory; run `teamai doctor` in another scope to check that one. `Skills delivered to ` compares the skills your role namespaces, tag subscriptions and exclusions resolve to against what is on disk for each installed tool: it reports a skill that was never delivered separately from one that arrived unreadable — `SKILL.md` missing, its frontmatter unparseable, or its `name` not matching the directory, which keeps the agent from ever discovering it. `Team docs delivered` compares the docs bundle against `sharing.docs.localDir`, which has one destination rather than one per tool; each expected document has to be a file that can be read, so a directory or a dangling link sitting on the name counts as missing. + +`Rules delivered to ` and `Agents delivered to ` do the same for the other two per-tool resources, and both ask the handler where an item lands rather than deriving a path: a rule's filename and content change per tool (`.md` verbatim, `.mdc` with derived `globs`/`alwaysApply`, `.instructions.md` with `applyTo`), and an agent's destination comes from its render, with `targets:` deciding which tools are owed a copy at all. A delivered rule is compared with the bytes the handler renders for that tool, not merely read for the keys its tool needs: a `.mdc` whose `globs` no longer match the team rule's `paths:` applies to the wrong files while carrying a perfectly legal `alwaysApply`, and that reads here as `delivered from an older copy` — the same label as a body that drifted, because both landed successfully and are still wrong. An agent is compared with the bytes its render produces, so a copy left behind by an older spec — a plain pull skips a scope whose team repo has not changed, so it can sit there indefinitely — is reported as `delivered from an older spec` rather than passing as present. `Every team agent reaches a tool` names an agent that renders for no installed tool — usually a spec that does not parse, or a `targets:` list naming only tools you do not have. These two are `doctor`-only: they read every rule per tool and parse every agent, which would spend the budget the checks at the end of a pull run under. + +Two tools do not read a rules directory, so a per-file check cannot speak for them and each gets one of its own. `Team rules are active in opencode` checks that `opencode.json` still lists the glob the pull owns under `instructions`: OpenCode does not auto-scan `.opencode/rules`, so without it every delivered `.md` is inert while the per-file check keeps passing. `Team rules are inlined in Hermes SOUL.md` compares the teamai-managed block of `SOUL.md` with what the team rules inline to, since Hermes reads standing instructions from that one file rather than from a directory — a deleted block, or one left on an older rule set, is a tool reading the wrong rules with nothing on disk to show for it. + +`MCP servers delivered to ` compares each server the team's `mcp.yaml` resolves for that tool against the entry in the tool's own config, and names any the reconcile skipped with its reason. The comparison is the entry, not the name: reconciliation leaves an entry teamai does not own alone, so a server of your own under a team name holds the key while the team's definition never arrives, and a stale copy is just as undelivered. Both are reported as `not the team's definition`, and only `teamai pull --force` replaces an entry teamai did not write. An unresolved `${VAR}` is reported here with the variable's name, which is otherwise said once during a pull and never again. An `mcp.yaml` that does not parse is not a team without MCP: it is reported as `Team MCP servers can be read` with the parse error, since it injects nothing into any tool and every run after the first is silent about it. `Env variables injected in shell profile` no longer stops at finding the marker comment: it checks that `env/env.yaml` parses and declares its variables under the `variables:` key (a plain `KEY: value` mapping parses as none, while an explicit `variables: []` is a configuration with nothing to deliver and fails nothing), that each one reached `env.sh` with the value `env.yaml` declares — a key left over from an older value exports it to every shell and MCP server until the next pull, and the comparison reads `env.sh` back through the generator's own inverse, so a multiline value quoted across several lines is matched rather than called stale — and that the injected block would actually load it — an unquoted Windows path degrades to something a POSIX shell cannot read, so `source` never runs and nothing says so. `Contributed learnings are published` fails while `teamai contribute` has notes queued that could not be pushed. A manual `teamai pull` does not repeat it at the end when the pull has already said it: the pull tries to publish the queue and reports the outcome itself, with the push error that made it fail — more than this check can tell you. If the pull never got that far, because the team repo failed to refresh, the check is printed as usual. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 57a8d99ad..aa83586e3 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -1484,7 +1484,13 @@ teamai remove rules --force # 跳过确认，用于脚本和 CI 仅当所有检查通过时，`teamai doctor` 才以状态码 0 退出；任一检查失败时以状态码 1 退出。尚未初始化时，它只报告缺少配置，不会臆测 Git 托管平台。手动执行 `teamai pull` 结束时会运行同一批检查（不含托管平台相关的检查，也不含本次 pull 已经自行报告过的检查）。 -除了托管平台、clone、配置、hook 和 env 检查之外，`doctor` 还会验证落到本机上的三件事。` is installed` 在 `enabledAgents` 列出了不会收到任何内容的工具时失败——这正是 pull 报告成功、而该工具什么都没收到的情况。它使用与同步相同的解析逻辑，因此像 OpenClaw 这样把 skills 放在 workspace 目录而非工具根目录的工具，会在同步真正写入的位置被判断。工具已安装时也会作为通过项报告，因此 `--json` 无论哪种情况都会为每个已启用工具给出一条记录。pull 结束时的检查只覆盖它从当前目录解析出的那个 scope；其他 scope 请在对应目录下运行 `teamai doctor`。`Skills delivered to ` 会把角色命名空间、标签订阅与排除规则解析出的 skill 集合，与每个已安装工具磁盘上的内容比对：从未送达的 skill 与送达但不可读的 skill 会分别报告——后者指 `SKILL.md` 缺失、frontmatter 无法解析，或其 `name` 与目录名不一致，导致 agent 永远发现不了它。`Team docs delivered` 将 docs 包与 `sharing.docs.localDir` 比对（它只有一个目标目录，而非每个工具一个）；每个应有的文档都必须是可读取的文件，因此占用了该名字的目录或断链接也算缺失。rules、agents 和 MCP server 目前尚未检查。 +除了托管平台、clone、配置和 hook 检查之外，`doctor` 还会验证落到本机上的内容。` is installed` 在 `enabledAgents` 列出了不会收到任何内容的工具时失败——这正是 pull 报告成功、而该工具什么都没收到的情况。它使用与同步相同的解析逻辑，因此像 OpenClaw 这样把 skills 放在 workspace 目录而非工具根目录的工具，会在同步真正写入的位置被判断。工具已安装时也会作为通过项报告，因此 `--json` 无论哪种情况都会为每个已启用工具给出一条记录。pull 结束时的检查只覆盖它从当前目录解析出的那个 scope；其他 scope 请在对应目录下运行 `teamai doctor`。`Skills delivered to ` 会把角色命名空间、标签订阅与排除规则解析出的 skill 集合，与每个已安装工具磁盘上的内容比对：从未送达的 skill 与送达但不可读的 skill 会分别报告——后者指 `SKILL.md` 缺失、frontmatter 无法解析，或其 `name` 与目录名不一致，导致 agent 永远发现不了它。`Team docs delivered` 将 docs 包与 `sharing.docs.localDir` 比对（它只有一个目标目录，而非每个工具一个）；每个应有的文档都必须是可读取的文件，因此占用了该名字的目录或断链接也算缺失。 + +`Rules delivered to ` 与 `Agents delivered to ` 对另外两类按工具下发的资源做同样的事，并且都向 handler 询问落点，而不是自行拼路径：rule 的文件名和内容因工具而异（`.md` 原样、`.mdc` 带派生的 `globs`/`alwaysApply`、`.instructions.md` 带 `applyTo`），agent 的落点来自渲染结果，且由 `targets:` 决定哪些工具应当收到。已送达的 rule 会与 handler 为该工具渲染出的字节逐一比对，而不只是检查该工具所需的键是否存在：`globs` 与团队 rule 的 `paths:` 不再一致的 `.mdc`，即使 `alwaysApply` 取值合法，也会作用到错误的文件上；这里会报告为 `delivered from an older copy`——正文漂移的副本同样如此，因为两者都写入成功，却都是错的。agent 会与渲染结果逐字节比对：旧版 spec 留下的副本（普通 pull 会跳过团队仓库未变化的 scope，它可能一直留在那里）报告为 `delivered from an older spec`，而不是当作已送达。`Every team agent reaches a tool` 会指出在任何已安装工具上都无法渲染的 agent，通常是 spec 解析失败，或 `targets:` 只列了本机没有的工具。这两项仅在 `doctor` 中运行：它们会按工具读取每条 rule、解析每个 agent，放进 pull 结束时的检查会耗尽其时间预算。 + +有两个工具并不读取 rules 目录，按文件比对的检查无法代表它们，因此各自单列一项。`Team rules are active in opencode` 检查 `opencode.json` 的 `instructions` 中是否仍列着 teamai 所拥有的那条 glob：OpenCode 不会自动扫描 `.opencode/rules`，缺了它，已送达的每个 `.md` 都不会生效，而按文件比对的检查依旧通过。`Team rules are inlined in Hermes SOUL.md` 把 `SOUL.md` 中 teamai 管理的代码块与团队 rule 内联后的内容比对——Hermes 的常驻指令来自这一个文件而非某个目录，因此代码块被删除或停留在旧版规则集上，都意味着该工具读到的是错误的规则，而磁盘上看不出任何异常。 + +`MCP servers delivered to ` 将团队 `mcp.yaml` 为该工具解析出的每个 server 与该工具自己配置文件中的条目逐一比对，并列出 reconcile 跳过的 server 及原因。比对的是条目内容而非名字：reconcile 不会覆盖不属于 teamai 的条目，因此你自己写的同名 server 会占住这个名字，团队的定义从未真正送达；过期的旧副本同样等于没送达。两者都报告为 `not the team's definition`，而覆盖非 teamai 写入的条目只有 `teamai pull --force` 能做到。未解析的 `${VAR}` 会在这里连同变量名一起报告——否则它只在 pull 时出现一次，之后再无提示。无法解析的 `mcp.yaml` 并不等于团队没有 MCP：它会作为 `Team MCP servers can be read` 连同解析错误一起报告，因为这种文件不会向任何工具注入内容，而且除第一次之外的每次运行都对此保持沉默。`Env variables injected in shell profile` 不再只查标记注释：它会检查 `env/env.yaml` 能否解析、以及是否在 `variables:` 键下声明了变量（写成普通的 `KEY: value` 映射等于没有声明；而显式写成 `variables: []` 属于没有内容要下发的配置，不会判为失败）、每个变量是否以 `env.yaml` 声明的值写进了 `env.sh`（残留的旧值会一直被导出到每个 shell 和 MCP server，直到下次 pull；比对时会用生成器自身的逆运算读回 `env.sh`，因此跨多行引用的多行值能够正确匹配，而不会被误判为过期），以及注入的代码块是否真的能加载它——未加引号的 Windows 路径在 POSIX shell 中会被转义破坏，`source` 从不执行，而且没有任何提示。 `Contributed learnings are published` 会在 `teamai contribute` 写下、但尚未推送成功的笔记仍在队列中时失败。当本次 pull 已经说过时，手动 `teamai pull` 结束时不会再重复它：pull 会尝试发布队列并自行报告结果，还会带上导致失败的推送错误——这是该检查本身给不出的信息。如果 pull 因为团队仓库刷新失败而根本没走到那一步，该检查会照常打印。 diff --git a/src/__tests__/agents.test.ts b/src/__tests__/agents.test.ts index 96179646a..aecafb5f9 100644 --- a/src/__tests__/agents.test.ts +++ b/src/__tests__/agents.test.ts @@ -184,6 +184,27 @@ describe('AgentsHandler — Phase 1 push/pull/remove', () => { expect(await fse.pathExists(path.join(homeDir, '.claude-internal/agents/helper.md'))).toBe(false); }); + it('pullItem leaves a member\'s same-stem file alone beside a legacy .md agent', async () => { + // A legacy `.md` is copied verbatim to one extension for every tool, so it + // can never leave a sibling of its own behind. Anything else on the stem is + // the member's file: a rendered spec sweeps its own stale extensions, this + // does not get to delete an unrelated `.toml`, `.json` or `.agent.md`. + const srcPath = path.join(repoPath, 'agents', 'helper.md'); + await fse.writeFile(srcPath, '# helper agent'); + const mine = path.join(homeDir, '.claude/agents/helper.toml'); + await fse.ensureDir(path.dirname(mine)); + await fse.writeFile(mine, 'name = "my own helper"\n'); + + await handler.pullItem( + { name: 'helper', type: 'agents', sourcePath: srcPath, relativePath: 'agents/helper.md' }, + teamConfig, + localConfig, + ); + + expect(await fse.pathExists(path.join(homeDir, '.claude/agents/helper.md'))).toBe(true); + expect(await fse.readFile(mine, 'utf8')).toBe('name = "my own helper"\n'); + }); + // ── scanLocalForPush ──────────────────────────────────── it('scanLocalForPush detects a modified agent across tool dirs as "modified"', async () => { diff --git a/src/__tests__/doctor-agents-delivery.test.ts b/src/__tests__/doctor-agents-delivery.test.ts new file mode 100644 index 000000000..5aedb3d62 --- /dev/null +++ b/src/__tests__/doctor-agents-delivery.test.ts @@ -0,0 +1,233 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import fse from 'fs-extra'; +import os from 'node:os'; +import path from 'node:path'; + +vi.mock('../config.js', () => ({ + detectProjectConfig: vi.fn().mockResolvedValue(null), + loadLocalConfig: vi.fn(), + loadTeamConfig: vi.fn(), +})); + +vi.mock('../utils/logger.js', () => ({ + log: { + debug: vi.fn(), error: vi.fn(), info: vi.fn(), success: vi.fn(), warn: vi.fn(), dim: vi.fn(), + }, + setStderrOnly: vi.fn(), +})); + +import { loadLocalConfig, loadTeamConfig } from '../config.js'; +import { AgentsHandler } from '../resources/agents.js'; +import { buildChecks, resolveDoctorContext, type Check } from '../doctor.js'; +import type { LocalConfig, TeamaiConfig } from '../types.js'; + +/** + * The agents half of the delivery check (#624). Unlike skills, the desired set + * is a relation: `targets:` decides which tools owe a copy, and each tool's + * render decides the extension — so the expected path cannot be derived from + * the agent's name. + */ +describe('doctor — agents delivered on disk', () => { + let tempDir: string; + let homeDir: string; + let repoPath: string; + let localConfig: LocalConfig; + let teamConfig: TeamaiConfig; + + const CLAUDE_AGENTS = '.claude/agents'; + const CODEX_AGENTS = '.codex/agents'; + + async function writeTeamAgent(name: string, spec: string): Promise { + const file = path.join(repoPath, 'agents', `${name}.yaml`); + await fse.ensureDir(path.dirname(file)); + await fse.writeFile(file, spec); + } + + function specFor(name: string, targets?: string[]): string { + const targetLine = targets ? `targets: [${targets.join(', ')}]\n` : ''; + return `name: ${name}\ndescription: does ${name} things\n${targetLine}instructions: |\n Do the thing.\n`; + } + + /** + * Deliver one agent to one tool the way `pullItem` does: the handler's own + * render at the handler's own path. Writing a placeholder instead would make + * every fixture a copy rendered from no spec at all. + */ + async function deliver(tool: string, name: string): Promise { + const handler = new AgentsHandler(); + const item = (await handler.scanTeamForPull(teamConfig, localConfig)).find((i) => i.name === name); + if (!item) throw new Error(`no team agent named ${name}`); + const target = (await handler.deliveryTargets(teamConfig, localConfig, item)) + .find((t) => t.tool === tool); + if (!target) throw new Error(`${name} does not render for ${tool}`); + await fse.ensureDir(path.dirname(target.dest)); + await fse.writeFile(target.dest, target.content ?? ''); + } + + async function checks(): Promise { + const ctx = await resolveDoctorContext(); + if (!ctx) throw new Error('expected a resolved doctor context'); + return buildChecks(ctx); + } + + async function agentsCheck(tool: string): Promise { + const check = (await checks()).find((c) => c.name === `Agents delivered to ${tool}`); + if (!check) throw new Error(`no agents delivery check for ${tool}`); + return check; + } + + beforeEach(async () => { + tempDir = await fse.mkdtemp(path.join(os.tmpdir(), 'teamai-agents-delivery-')); + homeDir = path.join(tempDir, 'home'); + repoPath = path.join(tempDir, 'team-repo'); + vi.stubEnv('HOME', homeDir); + + await writeTeamAgent('reviewer', specFor('reviewer')); + await fse.ensureDir(path.join(homeDir, CLAUDE_AGENTS)); + await fse.ensureDir(path.join(homeDir, CODEX_AGENTS)); + + localConfig = { + repo: { localPath: repoPath, remote: 'owner/repo' }, + username: 'tester', + scope: 'user', + additionalRoles: [], + }; + teamConfig = { + team: 'test', + description: '', + repo: 'owner/repo', + provider: 'git', + reviewers: [], + sharing: { + skills: {}, rules: { enforced: [] }, docs: { localDir: '' }, + env: { injectShellProfile: false }, + }, + toolPaths: { + claude: { agents: CLAUDE_AGENTS }, + codex: { agents: CODEX_AGENTS }, + }, + }; + + vi.mocked(loadLocalConfig).mockResolvedValue(localConfig); + vi.mocked(loadTeamConfig).mockResolvedValue(teamConfig); + }); + + afterEach(async () => { + vi.unstubAllEnvs(); + vi.clearAllMocks(); + await fse.remove(tempDir); + }); + + it('expects each tool its own render extension', async () => { + await deliver('claude', 'reviewer'); + await deliver('codex', 'reviewer'); + + expect(await (await agentsCheck('claude')).check()).toBe(true); + expect(await (await agentsCheck('codex')).check()).toBe(true); + }); + + it('fails when the tool-native render is missing, naming its directory', async () => { + await deliver('claude', 'reviewer'); + + const codex = await agentsCheck('codex'); + expect(await codex.check()).toBe(false); + expect(codex.fix).toContain('not delivered: reviewer'); + expect(codex.fix).toContain(path.join(homeDir, CODEX_AGENTS)); + + expect(await (await agentsCheck('claude')).check()).toBe(true); + }); + + it('does not ask a tool the spec does not target', async () => { + await writeTeamAgent('claude-only', specFor('claude-only', ['claude'])); + await deliver('claude', 'reviewer'); + await deliver('claude', 'claude-only'); + await deliver('codex', 'reviewer'); + + expect(await (await agentsCheck('codex')).check()).toBe(true); + expect(await (await agentsCheck('claude')).check()).toBe(true); + }); + + it('asks only LEGACY_MD_TOOLS for a legacy .md agent', async () => { + const legacy = path.join(repoPath, 'agents', 'old-hand.md'); + await fse.writeFile(legacy, '# old hand\n'); + await deliver('claude', 'reviewer'); + await deliver('claude', 'old-hand'); + await deliver('codex', 'reviewer'); + + // codex is not a legacy .md tool, so it is owed nothing for old-hand. + expect(await (await agentsCheck('codex')).check()).toBe(true); + expect(await (await agentsCheck('claude')).check()).toBe(true); + }); + + it('fails when the delivered copy was rendered from an older spec', async () => { + await deliver('claude', 'reviewer'); + await deliver('codex', 'reviewer'); + await writeTeamAgent('reviewer', specFor('reviewer').replace('Do the thing.', 'Do it differently.')); + + const claude = await agentsCheck('claude'); + expect(await claude.check()).toBe(false); + expect(claude.fix).toContain('delivered from an older spec: reviewer'); + expect(await (await agentsCheck('codex')).check()).toBe(false); + }); + + it('reports an agent whose spec renders for no installed tool', async () => { + await writeTeamAgent('broken', 'name: broken\n bad: [indent\n'); + await deliver('claude', 'reviewer'); + await deliver('codex', 'reviewer'); + + const check = (await checks()).find((c) => c.name === 'Every team agent reaches a tool'); + if (!check) throw new Error('expected the unreachable-agent check'); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('broken'); + }); + + it('reports the unreachable agents even when no agent reaches any tool', async () => { + // The tools are installed and every agent is malformed, so there is no + // per-tool check to hang the failure on. Taking the deliveries as proof a + // tool was there left this passing (#624 review). + await fse.remove(path.join(repoPath, 'agents', 'reviewer.yaml')); + await writeTeamAgent('broken', 'name: broken\n bad: [indent\n'); + + const built = await checks(); + expect(built.filter((c) => c.name.startsWith('Agents delivered to'))).toEqual([]); + + const check = built.find((c) => c.name === 'Every team agent reaches a tool'); + if (!check) throw new Error('expected the unreachable-agent check'); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('broken'); + }); + + it('stays silent about unreachable agents when no tool receives agents at all', async () => { + teamConfig.toolPaths = {}; + await writeTeamAgent('broken', 'name: broken\n bad: [indent\n'); + + const names = (await checks()).map((c) => c.name); + expect(names).not.toContain('Every team agent reaches a tool'); + }); + + it('emits no check for a tool the member disabled', async () => { + localConfig.disabledAgents = ['codex']; + await deliver('claude', 'reviewer'); + + const names = (await checks()).map((c) => c.name); + expect(names).toContain('Agents delivered to claude'); + expect(names).not.toContain('Agents delivered to codex'); + }); + + it('emits no check at all when the team repo ships no agents', async () => { + await fse.remove(path.join(repoPath, 'agents')); + + const names = (await checks()).map((c) => c.name); + expect(names.filter((n) => n.startsWith('Agents delivered to'))).toEqual([]); + }); + + it('never writes to the tool directory it inspects', async () => { + await deliver('claude', 'reviewer'); + + const before = (await fse.readdir(path.join(homeDir, CODEX_AGENTS))).sort(); + await (await agentsCheck('codex')).check(); + const after = (await fse.readdir(path.join(homeDir, CODEX_AGENTS))).sort(); + + expect(after).toEqual(before); + }); +}); diff --git a/src/__tests__/doctor-env-delivery.test.ts b/src/__tests__/doctor-env-delivery.test.ts new file mode 100644 index 000000000..bf9bdc6e5 --- /dev/null +++ b/src/__tests__/doctor-env-delivery.test.ts @@ -0,0 +1,233 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import fse from 'fs-extra'; +import os from 'node:os'; +import path from 'node:path'; + +vi.mock('../config.js', () => ({ + detectProjectConfig: vi.fn().mockResolvedValue(null), + loadLocalConfig: vi.fn(), + loadTeamConfig: vi.fn(), +})); + +vi.mock('../utils/logger.js', () => ({ + log: { + debug: vi.fn(), error: vi.fn(), info: vi.fn(), success: vi.fn(), warn: vi.fn(), dim: vi.fn(), + }, + setStderrOnly: vi.fn(), +})); + +import { loadLocalConfig, loadTeamConfig } from '../config.js'; +import { buildChecks, resolveDoctorContext, type Check } from '../doctor.js'; +import type { LocalConfig, TeamaiConfig } from '../types.js'; + +/** + * The env half of the delivery check (#624). The plumbing version asked only + * whether the marker comment was in the profile, which is true of a block that + * cannot load (#661) and of a run that delivered nothing (#662) — both of which + * surface three layers away as MCP servers skipped for unresolved variables. + */ +describe('doctor — env variables reach a shell', () => { + let tempDir: string; + let homeDir: string; + let repoPath: string; + let localConfig: LocalConfig; + let teamConfig: TeamaiConfig; + let envShPath: string; + let profilePath: string; + + async function writeEnvYaml(body: string): Promise { + await fse.ensureDir(path.join(repoPath, 'env')); + await fse.writeFile(path.join(repoPath, 'env', 'env.yaml'), body); + } + + async function writeEnvSh(body: string): Promise { + await fse.ensureDir(path.dirname(envShPath)); + await fse.writeFile(envShPath, body); + } + + async function writeProfile(sourceLine: string): Promise { + await fse.writeFile( + profilePath, + `# [teamai:env:start]\n# DO NOT EDIT: This section is auto-managed by teamai\n${sourceLine}\n# [teamai:env:end]\n`, + ); + } + + async function envCheck(): Promise { + const ctx = await resolveDoctorContext(); + if (!ctx) throw new Error('expected a resolved doctor context'); + const check = (await buildChecks(ctx)).find((c) => c.name === 'Env variables injected in shell profile'); + if (!check) throw new Error('no env check'); + return check; + } + + beforeEach(async () => { + tempDir = await fse.mkdtemp(path.join(os.tmpdir(), 'teamai-env-delivery-')); + homeDir = path.join(tempDir, 'home'); + repoPath = path.join(tempDir, 'team-repo'); + envShPath = path.join(homeDir, '.teamai', 'env.sh'); + profilePath = path.join(homeDir, '.bashrc'); + await fse.ensureDir(homeDir); + vi.stubEnv('HOME', homeDir); + vi.stubEnv('SHELL', '/bin/bash'); + + await writeEnvYaml('variables:\n - key: JIRA_PASSWORD\n value: "s3cret"\n'); + + localConfig = { + repo: { localPath: repoPath, remote: 'owner/repo' }, + username: 'tester', + scope: 'user', + additionalRoles: [], + }; + teamConfig = { + team: 'test', + description: '', + repo: 'owner/repo', + provider: 'git', + reviewers: [], + sharing: { + skills: {}, rules: { enforced: [] }, docs: { localDir: '' }, + env: { injectShellProfile: true }, + }, + toolPaths: {}, + }; + + vi.mocked(loadLocalConfig).mockResolvedValue(localConfig); + vi.mocked(loadTeamConfig).mockResolvedValue(teamConfig); + }); + + afterEach(async () => { + vi.unstubAllEnvs(); + vi.clearAllMocks(); + await fse.remove(tempDir); + }); + + it('passes when the block loads an env.sh carrying every declared variable', async () => { + await writeEnvSh("export JIRA_PASSWORD='s3cret'\n"); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + expect(await (await envCheck()).check()).toBe(true); + }); + + it('fails when the block points at a path a POSIX shell cannot read (#661)', async () => { + await writeEnvSh("export JIRA_PASSWORD='s3cret'\n"); + const windowsStyle = envShPath.replace(/\//g, '\\'); + await writeProfile(`[ -f ${windowsStyle} ] && source ${windowsStyle}`); + + const check = await envCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('does not load'); + expect(check.fix).toContain(envShPath); + }); + + it('fails and names `variables:` for the shorthand env.yaml form (#662)', async () => { + await writeEnvYaml('JIRA_PASSWORD: "s3cret"\n'); + await writeEnvSh(''); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + const check = await envCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('declares no variables'); + expect(check.fix).toContain('variables:'); + }); + + it('passes for a multiline value the shell quotes across several lines', async () => { + // A YAML block scalar is a legal env value, and single-quoting one spans + // physical lines. A reader that scans env.sh line by line can never match + // that export, so it called a correct delivery stale. + await writeEnvYaml('variables:\n - key: TEAM_KEY\n value: |\n line one\n line two\n'); + await writeEnvSh("export TEAM_KEY='line one\nline two\n'\n"); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + expect(await (await envCheck()).check()).toBe(true); + }); + + it('still reports a multiline value that drifted from env.yaml', async () => { + await writeEnvYaml('variables:\n - key: TEAM_KEY\n value: |\n line one\n line two\n'); + await writeEnvSh("export TEAM_KEY='line one\nsomething else\n'\n"); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + const check = await envCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('stale value'); + }); + + it('passes for a value carrying a single quote, which the generator escapes', async () => { + await writeEnvYaml("variables:\n - key: TEAM_KEY\n value: \"it's here\"\n"); + await writeEnvSh("export TEAM_KEY='it'\\''s here'\n"); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + expect(await (await envCheck()).check()).toBe(true); + }); + + it('passes for an env.yaml that declares `variables: []` on purpose', async () => { + // Nothing is owed, so nothing can be undelivered. This parses correctly + // and is a deliberately empty configuration, not the shorthand form. + await writeEnvYaml('variables: []\n'); + + expect(await (await envCheck()).check()).toBe(true); + }); + + it('passes for an empty env.yaml', async () => { + await writeEnvYaml(''); + + expect(await (await envCheck()).check()).toBe(true); + }); + + it('fails and names the file when env.yaml is not valid YAML', async () => { + await writeEnvYaml('variables:\n - key: A\n value: bad indent\n'); + + const check = await envCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain(path.join(repoPath, 'env', 'env.yaml')); + }); + + it('fails when env.sh still exports the value env.yaml replaced', async () => { + await writeEnvSh("export JIRA_PASSWORD='rotated-away'\n"); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + const check = await envCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('JIRA_PASSWORD'); + expect(check.fix).toContain('stale value'); + // The value is a secret: naming the key is the whole diagnosis. + expect(check.fix).not.toContain('s3cret'); + expect(check.fix).not.toContain('rotated-away'); + }); + + it('fails when a declared variable never reached env.sh', async () => { + await writeEnvSh("export OTHER='x'\n"); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + const check = await envCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('JIRA_PASSWORD'); + }); + + it('fails when the profile carries no TeamAI block at all', async () => { + await writeEnvSh("export JIRA_PASSWORD='s3cret'\n"); + await fse.writeFile(profilePath, '# nothing here\n'); + + const check = await envCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('carries no TeamAI env block'); + }); + + it('passes when the team opted out of shell-profile injection', async () => { + teamConfig.sharing.env = { injectShellProfile: false }; + + expect(await (await envCheck()).check()).toBe(true); + }); + + it('passes when the team ships no env.yaml', async () => { + await fse.remove(path.join(repoPath, 'env')); + + expect(await (await envCheck()).check()).toBe(true); + }); + + it('accepts a quoted path containing whitespace', async () => { + await writeEnvSh("export JIRA_PASSWORD='s3cret'\n"); + await writeProfile(`[ -f "${envShPath}" ] && source "${envShPath}"`); + + expect(await (await envCheck()).check()).toBe(true); + }); +}); diff --git a/src/__tests__/doctor-mcp-delivery.test.ts b/src/__tests__/doctor-mcp-delivery.test.ts new file mode 100644 index 000000000..bda4fc2c5 --- /dev/null +++ b/src/__tests__/doctor-mcp-delivery.test.ts @@ -0,0 +1,231 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import fse from 'fs-extra'; +import os from 'node:os'; +import path from 'node:path'; + +vi.mock('../config.js', () => ({ + detectProjectConfig: vi.fn().mockResolvedValue(null), + loadLocalConfig: vi.fn(), + loadTeamConfig: vi.fn(), +})); + +vi.mock('../utils/logger.js', () => ({ + log: { + debug: vi.fn(), error: vi.fn(), info: vi.fn(), success: vi.fn(), warn: vi.fn(), dim: vi.fn(), + }, + setStderrOnly: vi.fn(), +})); + +import { loadLocalConfig, loadTeamConfig } from '../config.js'; +import { buildChecks, resolveDoctorContext, type Check } from '../doctor.js'; +import type { LocalConfig, TeamaiConfig } from '../types.js'; + +/** + * The MCP half of the delivery check (#624). A server lands as an entry inside + * the tool's own config, so "delivered" is a key being present — and a server + * the reconcile skipped is reported with its reason, which is the only place + * an unresolved `${VAR}` is ever named again (#662). + */ +describe('doctor — MCP servers delivered on disk', () => { + let tempDir: string; + let homeDir: string; + let repoPath: string; + let localConfig: LocalConfig; + let teamConfig: TeamaiConfig; + + async function writeTeamMcp(yaml: string): Promise { + await fse.ensureDir(path.join(repoPath, 'mcp')); + await fse.writeFile(path.join(repoPath, 'mcp', 'mcp.yaml'), yaml); + } + + async function writeClaudeConfig(servers: Record): Promise { + const file = path.join(homeDir, '.claude.json'); + await fse.writeJson(file, { mcpServers: servers }, { spaces: 2 }); + } + + async function checks(): Promise { + const ctx = await resolveDoctorContext(); + if (!ctx) throw new Error('expected a resolved doctor context'); + return buildChecks(ctx); + } + + async function mcpCheck(tool = 'claude'): Promise { + const check = (await checks()).find((c) => c.name === `MCP servers delivered to ${tool}`); + if (!check) throw new Error(`no MCP delivery check for ${tool}`); + return check; + } + + beforeEach(async () => { + tempDir = await fse.mkdtemp(path.join(os.tmpdir(), 'teamai-mcp-delivery-')); + homeDir = path.join(tempDir, 'home'); + repoPath = path.join(tempDir, 'team-repo'); + vi.stubEnv('HOME', homeDir); + + await fse.ensureDir(path.join(homeDir, '.claude')); + await writeTeamMcp('servers:\n - name: docs\n transport: stdio\n command: docs-server\n'); + + localConfig = { + repo: { localPath: repoPath, remote: 'owner/repo' }, + username: 'tester', + scope: 'user', + additionalRoles: [], + }; + teamConfig = { + team: 'test', + description: '', + repo: 'owner/repo', + provider: 'git', + reviewers: [], + sharing: { + skills: {}, rules: { enforced: [] }, docs: { localDir: '' }, + env: { injectShellProfile: false }, + }, + toolPaths: { claude: { skills: '.claude/skills', mcp: '.claude.json' } }, + }; + + vi.mocked(loadLocalConfig).mockResolvedValue(localConfig); + vi.mocked(loadTeamConfig).mockResolvedValue(teamConfig); + }); + + afterEach(async () => { + vi.unstubAllEnvs(); + vi.clearAllMocks(); + await fse.remove(tempDir); + }); + + it('passes when every desired server is installed as teamai renders it', async () => { + await writeClaudeConfig({ docs: { type: 'stdio', command: 'docs-server' } }); + + expect(await (await mcpCheck()).check()).toBe(true); + }); + + it('passes when the installed entry differs only in key order', async () => { + await writeClaudeConfig({ docs: { command: 'docs-server', type: 'stdio' } }); + + expect(await (await mcpCheck()).check()).toBe(true); + }); + + it('fails when the name is held by a server teamai did not write', async () => { + // Exactly what reconciliation refuses to overwrite: the key is there, the + // team's server is not, and a plain pull skips it rather than clobber it. + await writeClaudeConfig({ docs: { type: 'stdio', command: 'my-own-docs-server' } }); + + const check = await mcpCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain("not the team's definition: docs"); + expect(check.fix).toContain('--force'); + }); + + it('fails when the installed entry is a stale copy of the team definition', async () => { + await writeClaudeConfig({ docs: { type: 'stdio', command: 'docs-server' } }); + await writeTeamMcp('servers:\n - name: docs\n transport: stdio\n command: docs-server-v2\n'); + + const check = await mcpCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain("not the team's definition: docs"); + }); + + it('fails and names a desired server with no entry', async () => { + await writeClaudeConfig({ other: { command: 'x' } }); + + const check = await mcpCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('not injected: docs'); + expect(check.fix).toContain(path.join(homeDir, '.claude.json')); + }); + + it('names the variable a server was skipped for, and points at env.yaml', async () => { + await writeTeamMcp( + 'servers:\n - name: jira\n transport: stdio\n command: jira-server\n' + + ' env:\n TOKEN: "${JIRA_PASSWORD}"\n', + ); + await writeClaudeConfig({}); + + const check = await mcpCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('jira'); + expect(check.fix).toContain('JIRA_PASSWORD'); + expect(check.fix).toContain('variables:'); + }); + + it('stays silent about a server the member excluded on purpose', async () => { + localConfig.excludedSkills = ['docs']; + await writeClaudeConfig({}); + + const names = (await checks()).map((c) => c.name); + expect(names).not.toContain('MCP servers delivered to claude'); + }); + + it('compares codex blocks by their text, whatever spacing the file has', async () => { + teamConfig.toolPaths!.codex = { skills: '.codex/skills', mcp: '.codex/config.toml' }; + await fse.ensureDir(path.join(homeDir, '.codex')); + const configToml = path.join(homeDir, '.codex', 'config.toml'); + await fse.writeFile( + configToml, + '[mcp_servers.docs]\ncommand = "docs-server"\nargs = []\n\n\n[other]\nx = 1\n', + ); + + expect(await (await mcpCheck('codex')).check()).toBe(true); + + await fse.writeFile(configToml, '[mcp_servers.docs]\ncommand = "someone-elses"\nargs = []\n'); + const check = await mcpCheck('codex'); + expect(await check.check()).toBe(false); + expect(check.fix).toContain("not the team's definition: docs"); + }); + + it('reports a tool config that cannot be parsed', async () => { + await fse.writeFile(path.join(homeDir, '.claude.json'), '{ not json'); + + const check = await mcpCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('could not be parsed'); + }); + + it('fails when mcp.yaml is present but does not parse', async () => { + // The parse yields no servers, exactly as an absent file does. Reading + // that as "this team ships no MCP" is what let `doctor --json` answer + // ok: true over a team whose every server reaches no tool at all. + await writeTeamMcp('servers:\n - name: docs\n transport: stdio\n bad indent\n'); + + const check = (await checks()).find((c) => c.name === 'Team MCP servers can be read'); + if (!check) throw new Error('no MCP parse check'); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('does not parse'); + expect(check.fix).toContain(path.join(repoPath, 'mcp', 'mcp.yaml')); + }); + + it('fails when mcp.yaml parses as YAML but breaks the server schema', async () => { + // `stdio` without `command`: zod refuses it, so the desired set is empty + // for a reason a member has to be told rather than shown as success. + await writeTeamMcp('servers:\n - name: docs\n transport: stdio\n'); + + const check = (await checks()).find((c) => c.name === 'Team MCP servers can be read'); + if (!check) throw new Error('no MCP parse check'); + expect(await check.check()).toBe(false); + }); + + it('emits no check when the team ships no MCP servers', async () => { + await fse.remove(path.join(repoPath, 'mcp')); + + const names = (await checks()).map((c) => c.name); + expect(names.filter((n) => n.startsWith('MCP servers delivered to'))).toEqual([]); + }); + + it('emits no check while the team has not opted into auto-apply', async () => { + teamConfig.sharing.mcp = { autoApply: false, allowedCommands: [], allowedHosts: [] }; + await writeClaudeConfig({}); + + const names = (await checks()).map((c) => c.name); + expect(names.filter((n) => n.startsWith('MCP servers delivered to'))).toEqual([]); + }); + + it('never writes to the tool config it inspects', async () => { + await writeClaudeConfig({ docs: { type: 'stdio', command: 'docs-server' } }); + const file = path.join(homeDir, '.claude.json'); + const before = await fse.readFile(file, 'utf8'); + + await (await mcpCheck()).check(); + + expect(await fse.readFile(file, 'utf8')).toBe(before); + }); +}); diff --git a/src/__tests__/doctor-rules-delivery.test.ts b/src/__tests__/doctor-rules-delivery.test.ts new file mode 100644 index 000000000..429b57805 --- /dev/null +++ b/src/__tests__/doctor-rules-delivery.test.ts @@ -0,0 +1,368 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import fse from 'fs-extra'; +import os from 'node:os'; +import path from 'node:path'; + +vi.mock('../config.js', () => ({ + detectProjectConfig: vi.fn().mockResolvedValue(null), + loadLocalConfig: vi.fn(), + loadTeamConfig: vi.fn(), +})); + +vi.mock('../utils/logger.js', () => ({ + log: { + debug: vi.fn(), error: vi.fn(), info: vi.fn(), success: vi.fn(), warn: vi.fn(), dim: vi.fn(), + }, + setStderrOnly: vi.fn(), +})); + +import { loadLocalConfig, loadTeamConfig } from '../config.js'; +import { buildChecks, resolveDoctorContext, type Check } from '../doctor.js'; +import type { LocalConfig, TeamaiConfig } from '../types.js'; + +/** + * The rules half of the delivery check (#624). A rule changes both its filename + * and its bytes per tool, so "it synced" and "the tool can read it" are two + * different questions — and only the second one is the one that matters. + */ +describe('doctor — rules delivered on disk', () => { + let tempDir: string; + let homeDir: string; + let repoPath: string; + let localConfig: LocalConfig; + let teamConfig: TeamaiConfig; + + const CLAUDE_RULES = '.claude/rules'; + const CURSOR_RULES = '.cursor/rules'; + const OPENCODE_RULES = '.config/opencode/rules'; + const OPENCODE_CONFIG = '.config/opencode/opencode.json'; + + /** Make OpenCode an installed tool that receives rules in this scope. */ + async function installOpencode(): Promise { + teamConfig.toolPaths.opencode = { + rules: '.opencode/rules', + mcp: OPENCODE_CONFIG, + mcpProject: 'opencode.json', + userScope: { rules: OPENCODE_RULES }, + }; + await fse.ensureDir(path.join(homeDir, OPENCODE_RULES)); + for (const name of ['coding-style', 'reviews']) { + await fse.writeFile(path.join(homeDir, OPENCODE_RULES, `${name}.md`), `Body of ${name}\n`); + } + } + + async function writeOpencodeConfig(data: unknown): Promise { + const file = path.join(homeDir, OPENCODE_CONFIG); + await fse.ensureDir(path.dirname(file)); + await fse.writeFile(file, typeof data === 'string' ? data : JSON.stringify(data, null, 2)); + } + + async function namedCheck(name: string): Promise { + return (await checks()).find((c) => c.name === name); + } + + async function writeTeamRule(name: string, frontmatter = ''): Promise { + const file = path.join(repoPath, 'rules', `${name}.md`); + await fse.ensureDir(path.dirname(file)); + await fse.writeFile(file, `${frontmatter}Body of ${name}\n`); + } + + /** A correctly delivered copy, the way pullItem leaves one. */ + async function deliverPlain(toolPath: string, name: string): Promise { + const file = path.join(homeDir, toolPath, `${name}.md`); + await fse.ensureDir(path.dirname(file)); + await fse.writeFile(file, `Body of ${name}\n`); + } + + async function deliverMdc(name: string, frontmatter = '---\nalwaysApply: true\n---\n\n'): Promise { + const file = path.join(homeDir, CURSOR_RULES, `${name}.mdc`); + await fse.ensureDir(path.dirname(file)); + await fse.writeFile(file, `${frontmatter}Body of ${name}\n`); + } + + async function checks(): Promise { + const ctx = await resolveDoctorContext(); + if (!ctx) throw new Error('expected a resolved doctor context'); + return buildChecks(ctx); + } + + async function rulesCheck(tool: string): Promise { + const check = (await checks()).find((c) => c.name === `Rules delivered to ${tool}`); + if (!check) throw new Error(`no rules delivery check for ${tool}`); + return check; + } + + beforeEach(async () => { + tempDir = await fse.mkdtemp(path.join(os.tmpdir(), 'teamai-rules-delivery-')); + homeDir = path.join(tempDir, 'home'); + repoPath = path.join(tempDir, 'team-repo'); + vi.stubEnv('HOME', homeDir); + + await writeTeamRule('coding-style'); + await writeTeamRule('reviews'); + await fse.ensureDir(path.join(homeDir, CLAUDE_RULES)); + await fse.ensureDir(path.join(homeDir, CURSOR_RULES)); + + localConfig = { + repo: { localPath: repoPath, remote: 'owner/repo' }, + username: 'tester', + scope: 'user', + additionalRoles: [], + }; + teamConfig = { + team: 'test', + description: '', + repo: 'owner/repo', + provider: 'git', + reviewers: [], + sharing: { + skills: {}, rules: { enforced: [] }, docs: { localDir: '' }, + env: { injectShellProfile: false }, + }, + toolPaths: { + claude: { rules: CLAUDE_RULES }, + cursor: { rules: CURSOR_RULES }, + }, + }; + + vi.mocked(loadLocalConfig).mockResolvedValue(localConfig); + vi.mocked(loadTeamConfig).mockResolvedValue(teamConfig); + }); + + afterEach(async () => { + vi.unstubAllEnvs(); + vi.clearAllMocks(); + await fse.remove(tempDir); + }); + + it('passes when every desired rule reached both tools in its own format', async () => { + await deliverPlain(CLAUDE_RULES, 'coding-style'); + await deliverPlain(CLAUDE_RULES, 'reviews'); + await deliverMdc('coding-style'); + await deliverMdc('reviews'); + + expect(await (await rulesCheck('claude')).check()).toBe(true); + expect(await (await rulesCheck('cursor')).check()).toBe(true); + }); + + it('fails for the tool whose .mdc copy is missing, and names its directory', async () => { + await deliverPlain(CLAUDE_RULES, 'coding-style'); + await deliverPlain(CLAUDE_RULES, 'reviews'); + await deliverMdc('coding-style'); + + const cursor = await rulesCheck('cursor'); + expect(await cursor.check()).toBe(false); + expect(cursor.fix).toContain('not delivered: reviews'); + expect(cursor.fix).toContain(path.join(homeDir, CURSOR_RULES)); + + // Per tool, independently: claude received both. + expect(await (await rulesCheck('claude')).check()).toBe(true); + }); + + it('reports a .mdc that landed without the frontmatter Cursor reads', async () => { + await deliverPlain(CLAUDE_RULES, 'coding-style'); + await deliverPlain(CLAUDE_RULES, 'reviews'); + await deliverMdc('coding-style'); + await deliverMdc('reviews', ''); + + const cursor = await rulesCheck('cursor'); + expect(await cursor.check()).toBe(false); + expect(cursor.fix).toContain('delivered from an older copy: reviews'); + }); + + it('reports a .mdc whose globs no longer match the team rule', async () => { + // The frontmatter fields are all present and `alwaysApply` is a legal + // value, so checking that the keys exist calls this delivered. Cursor + // applies it to `**/*.py` while the team rule says `**/*.ts`. + await writeTeamRule('reviews', '---\npaths:\n - "**/*.ts"\n---\n'); + await deliverPlain(CLAUDE_RULES, 'coding-style'); + await deliverPlain(CLAUDE_RULES, 'reviews'); + await deliverMdc('coding-style'); + await deliverMdc('reviews', '---\nglobs: "**/*.py"\nalwaysApply: false\n---\n\n'); + + const cursor = await rulesCheck('cursor'); + expect(await cursor.check()).toBe(false); + expect(cursor.fix).toContain('delivered from an older copy: reviews'); + }); + + it('reports a .md copy whose body drifted from the team rule', async () => { + await deliverPlain(CLAUDE_RULES, 'coding-style'); + const drifted = path.join(homeDir, CLAUDE_RULES, 'reviews.md'); + await fse.ensureDir(path.dirname(drifted)); + await fse.writeFile(drifted, 'Something else entirely\n'); + await deliverMdc('coding-style'); + await deliverMdc('reviews'); + + const claude = await rulesCheck('claude'); + expect(await claude.check()).toBe(false); + expect(claude.fix).toContain('delivered from an older copy: reviews'); + }); + + it('treats a plain .md rule as applicable without frontmatter', async () => { + await deliverPlain(CLAUDE_RULES, 'coding-style'); + await deliverPlain(CLAUDE_RULES, 'reviews'); + + expect(await (await rulesCheck('claude')).check()).toBe(true); + }); + + it('fails when opencode.json does not reference the rules glob', async () => { + // Every `.md` is delivered byte for byte and OpenCode reads none of them: + // it does not scan a rules directory, so the files are inert until the + // glob in `instructions` points at them. + await installOpencode(); + await writeOpencodeConfig({ instructions: [] }); + await deliverPlain(CLAUDE_RULES, 'coding-style'); + await deliverPlain(CLAUDE_RULES, 'reviews'); + await deliverMdc('coding-style'); + await deliverMdc('reviews'); + + expect(await (await rulesCheck('opencode')).check()).toBe(true); + + const active = await namedCheck('Team rules are active in opencode'); + expect(active).toBeDefined(); + expect(await active!.check()).toBe(false); + expect(active!.fix).toContain('rules/*.md'); + expect(active!.fix).toContain('inert'); + }); + + it('passes when opencode.json lists the rules glob beside the user\'s own', async () => { + await installOpencode(); + await writeOpencodeConfig({ instructions: ['CONVENTIONS.md', 'rules/*.md'] }); + + expect(await (await namedCheck('Team rules are active in opencode'))!.check()).toBe(true); + }); + + it('fails when opencode.json cannot be parsed, which is when the pull skipped it', async () => { + await installOpencode(); + await writeOpencodeConfig('{ not json'); + + const active = await namedCheck('Team rules are active in opencode'); + expect(await active!.check()).toBe(false); + expect(active!.fix).toContain('could not be read'); + }); + + it('emits no opencode activation check while opencode is not installed here', async () => { + expect(await namedCheck('Team rules are active in opencode')).toBeUndefined(); + }); + + it('fails when the Hermes SOUL.md block is gone', async () => { + // Hermes has no rules directory: its rules are the contents of a managed + // block, so a deleted block is a tool reading none of the team's rules. + const hermesHome = path.join(tempDir, 'hermes'); + await fse.ensureDir(hermesHome); + vi.stubEnv('HERMES_HOME', hermesHome); + await fse.writeFile(path.join(hermesHome, 'SOUL.md'), 'My own standing instructions\n'); + + const soul = await namedCheck('Team rules are inlined in Hermes SOUL.md'); + expect(soul).toBeDefined(); + expect(await soul!.check()).toBe(false); + expect(soul!.fix).toContain('carries no teamai rules block'); + }); + + it('fails when the Hermes block holds a stale rule set', async () => { + const hermesHome = path.join(tempDir, 'hermes'); + await fse.ensureDir(hermesHome); + vi.stubEnv('HERMES_HOME', hermesHome); + await fse.writeFile( + path.join(hermesHome, 'SOUL.md'), + '\nBody of coding-style\n\n', + ); + + const soul = await namedCheck('Team rules are inlined in Hermes SOUL.md'); + expect(await soul!.check()).toBe(false); + expect(soul!.fix).toContain('not what the team rules inline to'); + }); + + it('passes when the Hermes block holds every team rule body', async () => { + const hermesHome = path.join(tempDir, 'hermes'); + await fse.ensureDir(hermesHome); + vi.stubEnv('HERMES_HOME', hermesHome); + await fse.writeFile( + path.join(hermesHome, 'SOUL.md'), + '\nBody of coding-style\n\nBody of reviews\n\n', + ); + + expect(await (await namedCheck('Team rules are inlined in Hermes SOUL.md'))!.check()).toBe(true); + }); + + it('emits no Hermes check while Hermes is not installed here', async () => { + vi.stubEnv('HERMES_HOME', path.join(tempDir, 'no-hermes')); + + expect(await namedCheck('Team rules are inlined in Hermes SOUL.md')).toBeUndefined(); + }); + + it('emits no check for a tool configured without a rules path', async () => { + teamConfig.toolPaths = { claude: { rules: CLAUDE_RULES }, codex: { skills: '.codex/skills' } }; + await deliverPlain(CLAUDE_RULES, 'coding-style'); + await deliverPlain(CLAUDE_RULES, 'reviews'); + + const names = (await checks()).map((c) => c.name); + expect(names).toContain('Rules delivered to claude'); + expect(names).not.toContain('Rules delivered to codex'); + }); + + it('emits no check for a tool the member disabled', async () => { + localConfig.disabledAgents = ['cursor']; + await deliverPlain(CLAUDE_RULES, 'coding-style'); + await deliverPlain(CLAUDE_RULES, 'reviews'); + + const names = (await checks()).map((c) => c.name); + expect(names).not.toContain('Rules delivered to cursor'); + }); + + it('emits no check at all when the team repo ships no rules', async () => { + await fse.remove(path.join(repoPath, 'rules')); + + const names = (await checks()).map((c) => c.name); + expect(names.filter((n) => n.startsWith('Rules delivered to'))).toEqual([]); + }); + + it('resolves a namespaced rule to its nested destination', async () => { + await writeTeamRule('frontend/scoped'); + await deliverPlain(CLAUDE_RULES, 'coding-style'); + await deliverPlain(CLAUDE_RULES, 'reviews'); + await deliverPlain(CLAUDE_RULES, 'frontend/scoped'); + await deliverMdc('coding-style'); + await deliverMdc('reviews'); + + expect(await (await rulesCheck('claude')).check()).toBe(true); + + const cursor = await rulesCheck('cursor'); + expect(await cursor.check()).toBe(false); + expect(cursor.fix).toContain('not delivered: frontend/scoped'); + }); + + it('leaves the rules and agents checks out of the post-pull stage', async () => { + await fse.ensureDir(path.join(repoPath, 'agents')); + await fse.writeFile( + path.join(repoPath, 'agents', 'reviewer.yaml'), + 'name: reviewer\ndescription: reviews\ninstructions: |\n Review.\n', + ); + teamConfig.toolPaths.claude = { rules: CLAUDE_RULES, agents: '.claude/agents' }; + await fse.ensureDir(path.join(homeDir, '.claude/agents')); + + const ctx = await resolveDoctorContext(); + if (!ctx) throw new Error('expected a resolved doctor context'); + + const forDoctor = (await buildChecks(ctx, 'doctor')).map((c) => c.name); + const forPull = (await buildChecks(ctx, 'pull')).map((c) => c.name); + + expect(forDoctor.filter((n) => !forPull.includes(n)).sort()).toEqual([ + 'Agents delivered to claude', + 'Rules delivered to claude', + 'Rules delivered to cursor', + ]); + // Everything the pull stage keeps is also in the doctor stage. + expect(forPull.filter((n) => !forDoctor.includes(n))).toEqual([]); + }); + + it('never writes to the tool directory it inspects', async () => { + await deliverPlain(CLAUDE_RULES, 'coding-style'); + + const before = (await fse.readdir(path.join(homeDir, CURSOR_RULES))).sort(); + await (await rulesCheck('cursor')).check(); + const after = (await fse.readdir(path.join(homeDir, CURSOR_RULES))).sort(); + + expect(after).toEqual(before); + }); +}); diff --git a/src/__tests__/doctor.test.ts b/src/__tests__/doctor.test.ts index 41b08d977..b93c4f0e4 100644 --- a/src/__tests__/doctor.test.ts +++ b/src/__tests__/doctor.test.ts @@ -12,10 +12,13 @@ vi.mock('../config.js', () => ({ vi.mock('../utils/fs.js', () => ({ pathExists: vi.fn(), readFileSafe: vi.fn(), - // The delivery checks walk the team repo through resolveDesiredSkills and - // DocsHandler. This machine has neither skills nor docs; delivery on a real - // disk is covered by doctor-delivery.test.ts. + // The delivery checks walk the team repo through resolveDesiredSkills, + // resolveDesiredRules, resolveDesiredAgents and DocsHandler. This machine + // has none of those; delivery on a real disk is covered by + // doctor-delivery.test.ts, doctor-rules-delivery.test.ts and + // doctor-agents-delivery.test.ts. listDirs: vi.fn().mockResolvedValue([]), + listFiles: vi.fn().mockResolvedValue([]), listFilesRecursive: vi.fn().mockResolvedValue([]), expandHome: vi.fn((p: string) => p), })); diff --git a/src/__tests__/e2e/doctor-delivery-cli.test.ts b/src/__tests__/e2e/doctor-delivery-cli.test.ts new file mode 100644 index 000000000..6f7ecf704 --- /dev/null +++ b/src/__tests__/e2e/doctor-delivery-cli.test.ts @@ -0,0 +1,303 @@ +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { spawnSync } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..', '..'); +const CLI = path.join(ROOT, 'dist', 'index.js'); + +/** The `agents/reviewer.yaml` fixture as each tool's renderer writes it. */ +const CLAUDE_AGENT_MD = '---\nname: reviewer\ndescription: reviews\n---\nReview.\n'; +const CODEX_AGENT_TOML = 'name = "reviewer"\ndescription = "reviews"\ndeveloper_instructions = "Review.\\n"\n'; + +interface CheckResult { name: string; ok: boolean; fix?: string } +interface DoctorReport { ok: boolean; checks: CheckResult[] } + +/** + * The delivery checks through the real built CLI (#624): a team repo that ships + * one of every per-tool resource, a HOME where nothing was delivered, and the + * same HOME once each file is in place. + */ +describe('teamai doctor delivery checks (e2e)', () => { + let sandbox: string; + let home: string; + let repo: string; + + function runDoctor(): DoctorReport { + const result = spawnSync(process.execPath, [CLI, 'doctor', '--json'], { + cwd: home, + env: { ...process.env, HOME: home, USERPROFILE: home, SHELL: '/bin/bash', FORCE_COLOR: '0' }, + encoding: 'utf8', + }); + return JSON.parse(result.stdout) as DoctorReport; + } + + function check(report: DoctorReport, name: string): CheckResult { + const found = report.checks.find((c) => c.name === name); + if (!found) throw new Error(`no check named ${name} in: ${report.checks.map((c) => c.name).join(', ')}`); + return found; + } + + function write(file: string, content: string): void { + fs.mkdirSync(path.dirname(file), { recursive: true }); + fs.writeFileSync(file, content); + } + + beforeAll(() => { + if (!fs.existsSync(CLI)) throw new Error('Run npm run build before the E2E test.'); + + sandbox = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-delivery-e2e-')); + home = path.join(sandbox, 'home'); + repo = path.join(sandbox, 'team-repo'); + + const toolDirs = [ + '.claude/skills', '.claude/rules', '.claude/agents', + '.cursor/rules', + '.codex/agents', + '.codebuddy/rules', '.codebuddy/agents', + '.config/opencode/rules', + '.teamai', + ]; + for (const dir of toolDirs) fs.mkdirSync(path.join(home, dir), { recursive: true }); + + write(path.join(repo, 'teamai.yaml'), [ + 'team: e2e', + 'repo: team/repo', + 'provider: git', + 'toolPaths:', + ' claude:', + ' settings: .claude/settings.json', + ' skills: .claude/skills', + ' rules: .claude/rules', + ' agents: .claude/agents', + ' mcp: .claude.json', + ' cursor:', + ' rules: .cursor/rules', + ' codex:', + ' agents: .codex/agents', + ' codebuddy:', + ' rules: .codebuddy/rules', + ' agents: .codebuddy/agents', + ' opencode:', + ' rules: .opencode/rules', + ' mcp: .config/opencode/opencode.json', + ' mcpProject: opencode.json', + ' userScope:', + ' rules: .config/opencode/rules', + ].join('\n')); + + write(path.join(repo, 'skills', 'alpha', 'SKILL.md'), '---\nname: alpha\ndescription: d\n---\n'); + write(path.join(repo, 'rules', 'coding-style.md'), 'Coding style body\n'); + write(path.join(repo, 'agents', 'reviewer.yaml'), 'name: reviewer\ndescription: reviews\ninstructions: |\n Review.\n'); + write(path.join(repo, 'mcp', 'mcp.yaml'), [ + 'servers:', + ' - name: jira', + ' transport: stdio', + ' command: jira-server', + ' env:', + ' TOKEN: "${JIRA_PASSWORD}"', + ].join('\n')); + // Deliberately the shorthand form #662 is about. + write(path.join(repo, 'env', 'env.yaml'), 'JIRA_PASSWORD: "s3cret"\n'); + + // OpenCode reads its rules through this glob, not by scanning the directory. + write(path.join(home, '.config', 'opencode', 'opencode.json'), JSON.stringify({ instructions: [] })); + + write(path.join(home, '.teamai', 'config.yaml'), [ + 'repo:', + ` localPath: ${JSON.stringify(repo)}`, + ' remote: https://example.invalid/team/repo.git', + ' kind: git', + 'username: e2e-user', + 'updatePolicy: skip', + 'scope: user', + 'enabledAgents:', + ' - claude', + ' - cursor', + ' - codex', + ' - codebuddy', + ' - opencode', + ].join('\n')); + write( + path.join(home, '.claude', 'settings.json'), + JSON.stringify({ hooks: { SessionStart: [{ hooks: [{ type: 'command', command: 'teamai hook-dispatch' }] }] } }), + ); + }); + + afterAll(() => { + fs.rmSync(sandbox, { recursive: true, force: true }); + }); + + it('names every resource that never reached its tool', () => { + const report = runDoctor(); + + expect(report.ok).toBe(false); + expect(check(report, 'Skills delivered to claude').ok).toBe(false); + expect(check(report, 'Rules delivered to claude').fix).toContain('coding-style'); + expect(check(report, 'Rules delivered to cursor').fix).toContain('.cursor/rules'); + expect(check(report, 'Agents delivered to codex').fix).toContain('reviewer'); + expect(check(report, 'Rules delivered to codebuddy').ok).toBe(false); + expect(check(report, 'Agents delivered to codebuddy').ok).toBe(false); + // OpenCode's user scope reads a different prefix than its project paths. + expect(check(report, 'Rules delivered to opencode').fix).toContain('.config/opencode/rules'); + }); + + it('names the variable an MCP server was skipped for, and points at env.yaml', () => { + const mcp = check(runDoctor(), 'MCP servers delivered to claude'); + + expect(mcp.ok).toBe(false); + expect(mcp.fix).toContain('JIRA_PASSWORD'); + expect(mcp.fix).toContain('variables:'); + }); + + it('reports the shorthand env.yaml that parses to no variables at all', () => { + const env = check(runDoctor(), 'Env variables injected in shell profile'); + + expect(env.ok).toBe(false); + expect(env.fix).toContain('declares no variables'); + }); + + it('passes every check once each file is where its tool reads it', () => { + write(path.join(home, '.claude/skills/alpha/SKILL.md'), '---\nname: alpha\ndescription: d\n---\n'); + write(path.join(home, '.claude/rules/coding-style.md'), 'Coding style body\n'); + write(path.join(home, '.cursor/rules/coding-style.mdc'), '---\nalwaysApply: true\n---\n\nCoding style body\n'); + // What `pullItem` writes for each tool, byte for byte: the check compares + // the delivered copy with the render, so a placeholder is a stale copy. + write(path.join(home, '.claude/agents/reviewer.md'), CLAUDE_AGENT_MD); + write(path.join(home, '.codex/agents/reviewer.toml'), CODEX_AGENT_TOML); + write(path.join(home, '.codebuddy/rules/coding-style.md'), 'Coding style body\n'); + write(path.join(home, '.codebuddy/agents/reviewer.md'), CLAUDE_AGENT_MD); + write(path.join(home, '.config/opencode/rules/coding-style.md'), 'Coding style body\n'); + // The glob the pull adds; without it every .md above is inert. + write(path.join(home, '.config', 'opencode', 'opencode.json'), JSON.stringify({ instructions: ['rules/*.md'] })); + // The entry teamai renders for claude, placeholder resolved — the check + // compares the value, so a hand-shaped entry of the same name is not it. + write(path.join(home, '.claude.json'), JSON.stringify({ + mcpServers: { jira: { type: 'stdio', command: 'jira-server', env: { TOKEN: 's3cret' } } }, + })); + write(path.join(repo, 'env', 'env.yaml'), 'variables:\n - key: JIRA_PASSWORD\n value: "s3cret"\n'); + write(path.join(home, '.teamai', 'env.sh'), "export JIRA_PASSWORD='s3cret'\n"); + // The machine-local KEY=VALUE backup the env channel writes beside env.sh; + // it is what the MCP placeholder resolution reads. + write(path.join(home, '.teamai', 'env'), 'JIRA_PASSWORD=s3cret\n'); + const envSh = path.join(home, '.teamai', 'env.sh'); + write(path.join(home, '.bashrc'), [ + '# [teamai:env:start]', + `[ -f ${envSh} ] && source ${envSh}`, + '# [teamai:env:end]', + ].join('\n')); + + const report = runDoctor(); + const failed = report.checks.filter((c) => !c.ok).map((c) => c.name); + + expect(failed).toEqual([]); + expect(report.ok).toBe(true); + }); + + it('reports a server of your own holding a team name, which a pull will not overwrite', () => { + write(path.join(home, '.claude.json'), JSON.stringify({ + mcpServers: { jira: { type: 'stdio', command: 'my-own-jira' } }, + })); + + const mcp = check(runDoctor(), 'MCP servers delivered to claude'); + + expect(mcp.ok).toBe(false); + expect(mcp.fix).toContain("not the team's definition: jira"); + expect(mcp.fix).toContain('--force'); + }); + + it('reports an agent copy left behind by an older spec', () => { + write(path.join(home, '.claude/agents/reviewer.md'), CLAUDE_AGENT_MD.replace('Review.', 'Review it the old way.')); + + const agents = check(runDoctor(), 'Agents delivered to claude'); + + expect(agents.ok).toBe(false); + expect(agents.fix).toContain('delivered from an older spec: reviewer'); + }); + + it('reports an env.sh left on the value env.yaml replaced', () => { + write(path.join(home, '.teamai', 'env.sh'), "export JIRA_PASSWORD='rotated-away'\n"); + + const env = check(runDoctor(), 'Env variables injected in shell profile'); + + expect(env.ok).toBe(false); + expect(env.fix).toContain('JIRA_PASSWORD'); + expect(env.fix).toContain('stale value'); + }); + + it('reports a Cursor rule whose globs no longer match the team rule', () => { + // Legal frontmatter, wrong scope: Cursor applies it to `**/*.py` while the + // team rule scopes it to `**/*.ts`. Reading the keys for presence calls + // this delivered; comparing against the render does not. + write(path.join(repo, 'rules', 'coding-style.md'), '---\npaths:\n - "**/*.ts"\n---\nCoding style body\n'); + write( + path.join(home, '.cursor/rules/coding-style.mdc'), + '---\nglobs: "**/*.py"\nalwaysApply: false\n---\n\nCoding style body\n', + ); + + const cursor = check(runDoctor(), 'Rules delivered to cursor'); + + expect(cursor.ok).toBe(false); + expect(cursor.fix).toContain('delivered from an older copy: coding-style'); + + write(path.join(repo, 'rules', 'coding-style.md'), 'Coding style body\n'); + write(path.join(home, '.cursor/rules/coding-style.mdc'), '---\nalwaysApply: true\n---\n\nCoding style body\n'); + }); + + it('reports an mcp.yaml that does not parse instead of reading it as no MCP', () => { + const mcpYaml = path.join(repo, 'mcp', 'mcp.yaml'); + const original = fs.readFileSync(mcpYaml, 'utf8'); + write(mcpYaml, 'servers:\n - name: jira\n transport: stdio\n'); + + const report = runDoctor(); + + expect(report.ok).toBe(false); + expect(check(report, 'Team MCP servers can be read').ok).toBe(false); + // The per-tool checks cannot say anything: there is no desired set. + expect(report.checks.filter((c) => c.name.startsWith('MCP servers delivered to'))).toEqual([]); + + write(mcpYaml, original); + }); + + it('reports rules OpenCode reads none of, though every file is delivered', () => { + // Drop the glob, leave the files. The per-file check keeps passing: the + // failure is that OpenCode does not scan a rules directory. + write(path.join(home, '.config', 'opencode', 'opencode.json'), JSON.stringify({ instructions: [] })); + + const report = runDoctor(); + + expect(check(report, 'Rules delivered to opencode').ok).toBe(true); + const active = check(report, 'Team rules are active in opencode'); + expect(active.ok).toBe(false); + expect(active.fix).toContain('inert'); + + write(path.join(home, '.config', 'opencode', 'opencode.json'), JSON.stringify({ instructions: ['rules/*.md'] })); + }); + + it('passes a multiline env value the shell quotes across several lines', () => { + const envYaml = path.join(repo, 'env', 'env.yaml'); + const envSh = path.join(home, '.teamai', 'env.sh'); + const originalYaml = fs.readFileSync(envYaml, 'utf8'); + const originalSh = fs.readFileSync(envSh, 'utf8'); + + write(envYaml, 'variables:\n - key: JIRA_PASSWORD\n value: |\n one\n two\n'); + write(envSh, "export JIRA_PASSWORD='one\ntwo\n'\n"); + + expect(check(runDoctor(), 'Env variables injected in shell profile').ok).toBe(true); + + write(envYaml, originalYaml); + write(envSh, originalSh); + }); + + it('passes an env.yaml that declares `variables: []` on purpose', () => { + const envYaml = path.join(repo, 'env', 'env.yaml'); + const original = fs.readFileSync(envYaml, 'utf8'); + write(envYaml, 'variables: []\n'); + + expect(check(runDoctor(), 'Env variables injected in shell profile').ok).toBe(true); + + write(envYaml, original); + }); +}); diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts new file mode 100644 index 000000000..d2e4701c9 --- /dev/null +++ b/src/doctor-delivery.ts @@ -0,0 +1,644 @@ +import path from 'node:path'; +import fs from 'node:fs'; +import { isDeepStrictEqual } from 'node:util'; +import { expandHome, listFilesRecursive, pathExists, readFileSafe } from './utils/fs.js'; +import { getDataHome, getMcpSharing, isAgentExcluded, TEAMAI_ENV_START, TEAMAI_ENV_END } from './types.js'; +import type { DeliveryTarget, ResourceItem } from './types.js'; +import { splitFrontmatter } from './utils/frontmatter.js'; +import type { ResourceHandler } from './resources/base.js'; +import type { Check, DoctorContext } from './doctor.js'; + +/** + * The checks that verify the payload rather than the plumbing: what each tool + * was owed, against what is on its disk (#598, #624). + * + * They live beside `doctor.ts` rather than inside it because every one of them + * is domain logic — where a rule lands for Cursor, which tools an agent's spec + * targets, whether a shell block would load — and `doctor.ts` is the registry + * that runs them. + * + * Every check here is read-only by contract. `doctor-delivery.test.ts` asserts + * it directly: resolving a destination must never write, or the command whose + * job is to describe the machine would change it. + */ + +/** + * Whether a delivered skill directory is one an agent can actually discover: + * SKILL.md present, frontmatter parses, and its `name` is the directory's own. + * A copy that fails this landed successfully — no write-time gate can see it. + */ +async function skillIsDiscoverable(skillDir: string, skillName: string): Promise { + const content = await readFileSafe(path.join(skillDir, 'SKILL.md')); + if (!content) return false; + + const { data, valid } = splitFrontmatter(content); + if (!valid) return false; + return data.name === skillName; +} + +/** + * Whether `filePath` is a file something can actually read. `pathExists` + * follows symlinks but says yes to a directory too, so on its own it cannot + * tell a delivered document from a name occupied by something else. + */ +async function isReadableFile(filePath: string): Promise { + try { + return (await fs.promises.stat(expandHome(filePath))).isFile(); + } catch { + return false; + } +} + +/** At most this many names in a fix string; the rest are counted. */ +const MAX_NAMED_IN_FIX = 5; + +/** Group item names under the tool that did not receive them. */ +function appendTo(buckets: Map, tool: string, name: string): void { + const names = buckets.get(tool); + if (names) names.push(name); + else buckets.set(tool, [name]); +} + +/** What one tool was owed, and which of it did not arrive intact. */ +interface ToolDelivery { + /** Where its items land — the fix names it when the filename is derived. */ + dir: string; + /** Item names grouped by the problem label `classify` gave them. */ + problems: Map; +} + +/** + * Walk every desired item across the tools that receive it, letting `classify` + * name what is wrong with each delivered path, or return null when it arrived + * intact. A tool absent from every item's targets receives nothing, so nothing + * is owed: it is either uninstalled — caught by its own ` is installed` + * check — or configured without a path for this resource. + */ +async function walkDelivery( + handler: ResourceHandler, + ctx: DoctorContext, + items: ResourceItem[], + classify: (target: DeliveryTarget, item: ResourceItem) => Promise, +): Promise<{ byTool: Map; unreceived: string[] }> { + const { localConfig, teamConfig } = ctx; + if (!teamConfig) return { byTool: new Map(), unreceived: [] }; + + const byTool = new Map(); + const unreceived: string[] = []; + + for (const item of items) { + const targets = await handler.deliveryTargets(teamConfig, localConfig, item); + if (targets.length === 0) unreceived.push(item.name); + + for (const target of targets) { + let delivery = byTool.get(target.tool); + if (!delivery) { + delivery = { dir: path.dirname(target.dest), problems: new Map() }; + byTool.set(target.tool, delivery); + } + const problem = await classify(target, item); + if (problem !== null) appendTo(delivery.problems, problem, item.name); + } + } + + return { byTool, unreceived }; +} + +/** + * `not delivered: a, b; unreadable: c`, with the labels in the order the caller + * lists them rather than the order the failures happened, so the same broken + * machine reads the same way twice. + */ +function describeProblems(problems: Map, labels: readonly string[]): string { + return labels + .filter((label) => (problems.get(label)?.length ?? 0) > 0) + .map((label) => `${label}: ${nameList(problems.get(label) ?? [])}`) + .join('; '); +} + +/** `a, b, c and 4 more` — a fix a human reads, not a wall of paths. */ +function nameList(names: string[]): string { + if (names.length <= MAX_NAMED_IN_FIX) return names.join(', '); + const shown = names.slice(0, MAX_NAMED_IN_FIX).join(', '); + return `${shown} and ${names.length - MAX_NAMED_IN_FIX} more`; +} + +/** + * Build one delivery check per installed tool: every skill the member should + * have, against what is actually on disk for that tool. + * + * This is the only check that looks at the payload rather than the plumbing. A + * write-time gate cannot cover it — `SkillsHandler.pullItem` skips each + * uninstalled tool on its own, and a directory deleted by hand after a correct + * pull leaves every gate happy (#598). + * + * The scan runs here rather than inside `check()` because the fix names the + * skills that are missing, and a `Check`'s fix is read as it was built. + */ +export async function buildDeliveryChecks(ctx: DoctorContext): Promise { + const { localConfig, teamConfig } = ctx; + if (!teamConfig) return []; + + // Dynamic: pull.ts imports this module for its post-pull pass, and the desired + // set is policy that must not be restated here. + const { buildRolePullContext, resolveDesiredSkills } = await import('./pull.js'); + const { getHandler } = await import('./resources/index.js'); + + let items: ResourceItem[]; + try { + const roleContext = await buildRolePullContext(localConfig); + ({ items } = await resolveDesiredSkills(teamConfig, localConfig, roleContext)); + } catch (e) { + // A team repo whose active namespaces collide cannot say what should be + // delivered — `pull` aborts the scope with this same message. The command + // whose job is explaining bad state must report it, not stack-trace on it. + return [{ + name: 'Skills to deliver can be resolved', + source: 'local', + check: async () => false, + fix: `${(e as Error).message}. Until the team repo is fixed, ` + + 'pull cannot sync skills for this role.', + }]; + } + if (items.length === 0) return []; + + const labels = ['not delivered', 'delivered but unreadable'] as const; + const { byTool } = await walkDelivery(getHandler('skills'), ctx, items, async ({ dest }, item) => { + if (!await pathExists(dest)) return labels[0]; + return await skillIsDiscoverable(dest, item.name) ? null : labels[1]; + }); + + return [...byTool].map(([tool, delivery]) => ({ + name: `Skills delivered to ${tool}`, + source: 'local', + check: async () => delivery.problems.size === 0, + fix: `In ${tool}, ${describeProblems(delivery.problems, labels)}. Run \`teamai pull --force\`: ` + + 'a plain pull skips a scope whose team repo has not changed, so it cannot restore this. ' + + 'If a skill stays unreadable, fix its SKILL.md in the team repo — the ' + + 'frontmatter needs a `name` matching the directory, or the agent never ' + + 'discovers it.', + })); +} + +/** + * Build one delivery check per tool that receives rules: every rule the member + * should have, against what is on disk for that tool. + * + * Rules change filename *and* content per tool, so only the handler can say + * where one lands. Asking it here is what keeps the check from growing its own + * copy of the extension table (#624). + */ +export async function buildRulesDeliveryChecks(ctx: DoctorContext): Promise { + const { localConfig, teamConfig } = ctx; + if (!teamConfig) return []; + + const { buildRolePullContext, resolveDesiredRules } = await import('./pull.js'); + const { getHandler } = await import('./resources/index.js'); + + const roleContext = await buildRolePullContext(localConfig); + const { items } = await resolveDesiredRules(teamConfig, localConfig, roleContext); + if (items.length === 0) return []; + + const activation = await buildRulesActivationChecks(ctx, items); + + // `pullItem` writes the handler's render byte for byte, so anything else at + // that path is a stale or hand-edited copy. Cursor reads `globs` and + // `alwaysApply` and Copilot reads `applyTo`; comparing against the render + // catches a wrong value there, which checking the keys were present did not. + const ruleLabels = ['not delivered', 'delivered from an older copy'] as const; + const perTool: Check[] = [...(await walkDelivery( + getHandler('rules'), + ctx, + items, + async ({ dest, content }) => { + // readFileSafe answers both questions at once: a directory or a dangling + // link on the name reads as null, the same as nothing being there. + const delivered = await readFileSafe(dest); + if (delivered === null) return ruleLabels[0]; + return content === undefined || delivered === content ? null : ruleLabels[1]; + }, + )).byTool].map(([tool, delivery]) => ({ + name: `Rules delivered to ${tool}`, + source: 'local', + check: async () => delivery.problems.size === 0, + // The fix names the directory rather than the tool: a rule's delivered + // filename carries a per-tool extension the reader would have to derive. + fix: `In ${delivery.dir}, ${describeProblems(delivery.problems, ruleLabels)}. ` + + 'Run `teamai pull --force`: a plain pull skips a scope whose team repo has not changed, ' + + 'so it cannot restore this. An older copy is one whose bytes are no longer what teamai ' + + `renders for ${tool}, frontmatter included: a \`.mdc\` or \`.instructions.md\` whose ` + + '`globs`, `alwaysApply` or `applyTo` drifted from the team `.md` applies to the wrong ' + + 'files while looking perfectly well-formed.', + })); + + return [...activation, ...perTool]; +} + +/** + * The two rule destinations that are not a file per tool. + * + * OpenCode does not auto-scan its rules directory: a `.md` copied there is + * inert until `opencode.json` references it through the glob the pull owns. + * Hermes has no rules directory at all — its rules are inlined into a managed + * block of SOUL.md. Both are delivered by `pullAllRules` rather than by + * `pullItem`, so `deliveryTargets` cannot see them, and a per-file check + * passes over a tool that reads none of what it was given. + * + * They take the shape of the hook and MCP checks — one destination, not one + * per tool — rather than an invented entry in `deliveryTargets`. + */ +async function buildRulesActivationChecks(ctx: DoctorContext, items: ResourceItem[]): Promise { + const { localConfig, teamConfig } = ctx; + if (!teamConfig) return []; + + const { RulesHandler, hermesRulesText } = await import('./resources/rules.js'); + const handler = new RulesHandler(); + const checks: Check[] = []; + + const opencode = await handler.opencodeInstructionsTarget(teamConfig, localConfig); + if (opencode !== null) { + const instructions = await readOpencodeInstructions(opencode.configFile); + const active = instructions !== null && instructions.includes(opencode.glob); + checks.push({ + name: 'Team rules are active in opencode', + source: 'local', + check: async () => active, + fix: instructions === null + ? `${opencode.configFile} could not be read as a JSON object, so the pull left it alone ` + + `and never added \`${opencode.glob}\` to \`instructions\`. Fix the file, then run ` + + '`teamai pull --force`.' + : `${opencode.configFile} does not list \`${opencode.glob}\` under \`instructions\`. ` + + 'OpenCode does not scan a rules directory, so every team rule delivered there is ' + + 'inert until this glob references it. Run `teamai pull --force`: a plain pull skips ' + + 'a scope whose team repo has not changed, so it cannot restore this.', + }); + } + + const { getHermesHome } = await import('./hermes-home.js'); + const hermesHome = getHermesHome(); + if (!isAgentExcluded(localConfig, 'hermes') && await pathExists(hermesHome)) { + const { getHermesSoulPath, readSoulRules } = await import('./hermes-config.js'); + const expected = await hermesRulesText(items); + const delivered = await readSoulRules(); + checks.push({ + name: 'Team rules are inlined in Hermes SOUL.md', + source: 'local', + check: async () => delivered !== null && delivered === expected.trim(), + fix: delivered === null + ? `${getHermesSoulPath()} carries no teamai rules block, so Hermes reads none of the ` + + 'team rules. Run `teamai pull --force`: a plain pull skips a scope whose team repo ' + + 'has not changed, so it cannot restore this.' + : `The teamai block in ${getHermesSoulPath()} is not what the team rules inline to: ` + + 'Hermes reads standing instructions from this file rather than a rules directory, ' + + 'so a stale block is a stale rule set. Run `teamai pull --force` to rewrite it.', + }); + } + + return checks; +} + +/** + * The `instructions` entries of an opencode.json, or null when the file is + * missing or is not a JSON object — the two cases in which the pull leaves it + * strictly alone and the glob never lands. + */ +async function readOpencodeInstructions(configFile: string): Promise { + const raw = await readFileSafe(configFile); + if (raw === null) return null; + if (raw.trim() === '') return []; + try { + const parsed: unknown = JSON.parse(raw); + if (typeof parsed !== 'object' || parsed === null || Array.isArray(parsed)) return null; + const { instructions } = parsed as { instructions?: unknown }; + return Array.isArray(instructions) ? instructions : []; + } catch { + return null; + } +} + +/** + * Build one delivery check per tool that receives agents. + * + * An agent's desired set is a relation rather than a product: `spec.targets` + * names the tools it is for, and each renders into its own format, so the + * handler is the only thing that can say which tools owe what file (#624). + */ +export async function buildAgentsDeliveryChecks(ctx: DoctorContext): Promise { + const { localConfig, teamConfig } = ctx; + if (!teamConfig) return []; + + const { buildRolePullContext, resolveDesiredAgents } = await import('./pull.js'); + const { AgentsHandler } = await import('./resources/agents.js'); + const handler = new AgentsHandler(); + + let items: ResourceItem[]; + try { + const roleContext = await buildRolePullContext(localConfig); + items = await resolveDesiredAgents(teamConfig, localConfig, roleContext); + } catch (e) { + // Two active namespaces claiming one agent name: `pull` aborts the scope + // with this message rather than picking one, so `doctor` reports it. + return [{ + name: 'Agents to deliver can be resolved', + source: 'local', + check: async () => false, + fix: `${(e as Error).message}. Until the team repo is fixed, ` + + 'pull cannot sync agents for this role.', + }]; + } + if (items.length === 0) return []; + + // An agent whose spec reaches no tool at all is not a per-tool failure: the + // file is in the team repo and nothing renders it anywhere. + const agentLabels = ['not delivered', 'delivered from an older spec'] as const; + const { byTool, unreceived: unreachable } = await walkDelivery( + handler, + ctx, + items, + // `pullItem` writes `content` verbatim, so anything else at that path is a + // render of an older spec — a copy that landed and is still wrong, the + // same class as a rule whose delivered copy no longer matches its render. + async ({ dest, content }) => { + // readFileSafe answers both questions at once: a directory or a dangling + // link on the name reads as null, the same as nothing being there. + const delivered = await readFileSafe(dest); + if (delivered === null) return agentLabels[0]; + return content === undefined || delivered === content ? null : agentLabels[1]; + }, + ); + + const checks: Check[] = [...byTool].map(([tool, delivery]) => ({ + name: `Agents delivered to ${tool}`, + source: 'local', + check: async () => delivery.problems.size === 0, + fix: `In ${delivery.dir}, ${describeProblems(delivery.problems, agentLabels)}. ` + + 'Run `teamai pull --force`: a plain pull skips a scope whose team repo has not changed, ' + + 'so it cannot restore this.', + })); + + // Only worth reporting once a tool is there to receive agents: with none + // installed, "reaches no tool" is the machine, not the team repo. The gate is + // the installed tools rather than the deliveries, or a set of agents that all + // fail to render would report nothing at all. + const agentTools = await handler.agentToolDirs(teamConfig, localConfig); + if (unreachable.length > 0 && agentTools.length > 0) { + checks.push({ + name: 'Every team agent reaches a tool', + source: 'local', + check: async () => false, + fix: `${nameList(unreachable)} render for no installed tool. Either the spec does not ` + + 'parse — `teamai pull` names the reason — or its `targets:` lists only tools that ' + + 'are not installed here.', + }); + } + + return checks; +} + +/** + * Build one check per tool that receives MCP servers. + * + * An MCP server is an entry inside the tool's own config file, not a file of + * its own, so this takes the shape of the hook check rather than of + * `deliveryTargets`. It reports two things a pull says once and never again: + * a desired server whose entry is not there, and a server the reconcile + * skipped — an unresolved `${VAR}` is the reason behind "MCP does not work" + * that no other output points at (#662). + */ +export async function buildMcpDeliveryChecks(ctx: DoctorContext): Promise { + const { localConfig, teamConfig } = ctx; + if (!teamConfig) return []; + // HTTP-backed teams have no repo tree: servers arrive through the local-agent + // install channel, and the desired set here would always be empty. + if (localConfig.repo.kind === 'http') return []; + + const sharing = getMcpSharing(teamConfig); + // Nothing was promised automatically, so nothing is owed until the member + // runs `teamai mcp inject`. + if (!sharing.autoApply) return []; + + const { + resolveMcpTargets, buildDesiredMcpContext, desiredMcpForTarget, + mcpTargetExcluded, installedMcpEntries, + } = await import('./mcp-reconcile.js'); + const { readMcpYaml, teamMcpToDef, teamMcpYamlPath } = await import('./resources/mcp.js'); + + // A file that does not parse is not a team without MCP: the pull logs the + // reason once and injects nothing anywhere, and every later run is silent. + // Flattening it to an empty desired set is what let `doctor --json` answer + // `ok: true` over a team whose MCP reaches no tool at all. + const read = await readMcpYaml(localConfig.repo.localPath); + if (!read.ok) { + const yamlPath = teamMcpYamlPath(localConfig.repo.localPath); + return [{ + name: 'Team MCP servers can be read', + source: 'local', + check: async () => false, + fix: `${yamlPath} does not parse: ${read.reason}. No server is injected into any tool ` + + 'until it is fixed in the team repo and pushed.', + }]; + } + + const teamDefs = (read.yaml?.servers ?? []).map(teamMcpToDef); + if (teamDefs.length === 0) return []; + + const targets = await resolveMcpTargets(teamConfig, localConfig); + const desiredContext = await buildDesiredMcpContext(teamConfig, localConfig); + const excludedByUser = new Set(localConfig.excludedSkills ?? []); + + const checks: Check[] = []; + for (const target of targets) { + if (mcpTargetExcluded(localConfig, target)) continue; + + const { desired, skipped } = desiredMcpForTarget(target, teamDefs, desiredContext); + const blocked = skipped + .filter((change) => !excludedByUser.has(change.server)) + .map((change) => `${change.server} (${change.reason ?? 'skipped'})`); + + const problems: string[] = []; + const installed = await installedMcpEntries(target); + if (installed === null) { + problems.push(`${target.file} could not be parsed, so no server was injected`); + } else { + const absent: string[] = []; + const foreign: string[] = []; + for (const [name, { entry }] of desired) { + if (!installed.has(name)) absent.push(name); + // An entry that is not the one teamai renders is not this server: the + // appliers leave an entry they do not own alone, so the name can be + // held by something else entirely, and a stale copy is equally undelivered. + else if (!isDeepStrictEqual(installed.get(name), entry)) foreign.push(name); + } + 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 && desired.size === 0) continue; + + checks.push({ + name: `MCP servers delivered to ${target.tool}`, + source: 'local', + check: async () => problems.length === 0, + fix: `In ${target.file}, ${problems.join('; ')}. A server needing a variable reads it from ` + + '`env/env.yaml`, whose top-level key is `variables:` — a plain `KEY: value` mapping ' + + 'parses as no variables at all. Then run `teamai pull --force`: a pull leaves an entry ' + + 'teamai does not own untouched, so a server of your own under a team name only gives ' + + 'way to `--force`.', + }); + } + + return checks; +} + +/** The TeamAI-managed block of a shell profile, or null when it is absent. */ +function envBlockIn(profileContent: string): string | null { + const start = profileContent.indexOf(TEAMAI_ENV_START); + if (start === -1) return null; + const end = profileContent.indexOf(TEAMAI_ENV_END, start); + return end === -1 ? profileContent.slice(start) : profileContent.slice(start, end); +} + +/** + * Whether the injected block would actually load `env.sh` when a POSIX shell + * reads it. + * + * The block is generated by joining paths with the platform separator, so on + * Windows it carries backslashes. An unquoted `\` is an escape character there, + * so `[ -f C:\Users\me\.teamai/env.sh ]` tests a path that cannot exist, `&&` + * short-circuits and `source` never runs — silently, because a failed `[` test + * in a profile prints nothing (#661). A path containing whitespace needs quotes + * for the same reason. + */ +function envBlockLoads(block: string, envShPath: string): boolean { + const posixPath = envShPath.split(path.sep).join('/'); + if (!block.includes(posixPath)) return false; + if (!/\s/.test(posixPath)) return true; + return block.includes(`"${posixPath}"`) || block.includes(`'${posixPath}'`); +} + +/** + * Check that the env variables the team declares actually reach a shell. + * + * The plumbing version of this check asked only whether the marker comment was + * in the profile, which is true of a block that cannot load and of a run that + * delivered nothing. Both failures surface three layers away, as MCP servers + * skipped for `unresolved variable(s)`, with nothing pointing back here. + */ +export async function buildEnvDeliveryCheck(ctx: DoctorContext): Promise { + const problems = await envDeliveryProblems(ctx); + return [{ + name: 'Env variables injected in shell profile', + source: 'local', + check: async () => problems.length === 0, + fix: problems.length === 0 + ? 'Run `teamai pull` to inject env variables into shell profile' + : `${problems.join('; ')}. Run \`teamai pull\` after fixing the cause, then open a new shell.`, + }]; +} + +/** Every reason the team's env variables are not reaching a shell. */ +async function envDeliveryProblems(ctx: DoctorContext): Promise { + const { localConfig, teamConfig } = ctx; + if (teamConfig?.sharing?.env?.injectShellProfile === false) return []; + + const envYamlPath = path.join(localConfig.repo.localPath, 'env', 'env.yaml'); + if (!await pathExists(envYamlPath)) return []; + + const { EnvHandler } = await import('./resources/env.js'); + const envHandler = new EnvHandler(); + + // The handler distinguishes a file that parses from one that does not, so a + // shorthand `KEY: value` mapping is reported (#662) while a deliberate + // `variables: []` is not. Counting the variables alone cannot tell them apart. + const read = await envHandler.readEnvYaml(envYamlPath); + if (!read.ok) return [read.reason]; + + const declared = read.variables; + const problems: string[] = []; + + // Nothing declared and nothing malformed: there is nothing to deliver. + if (declared.length === 0) return problems; + + // env.sh lives under teamaiHome, which is /.teamai in project + // scope and ~/.teamai in user scope — mirror the path that `teamai pull` + // actually writes to, not a hardcoded user-home path. + const envShPath = path.join(getDataHome(localConfig), 'env.sh'); + const envSh = await readFileSafe(envShPath); + if (envSh === null) { + problems.push(`${envShPath} is missing`); + } else { + // Read the file back through the generator's own inverse, value included: + // a key whose value changed in env.yaml exports the old one until the next + // pull rewrites the file, and every shell and MCP server reads that. It is + // a parse rather than a line scan because a value may be multiline — a + // YAML block scalar quotes into an export spanning several lines. + const { parseEnvFile } = await import('./resources/env.js'); + const delivered = parseEnvFile(envSh); + const undelivered: string[] = []; + const stale: string[] = []; + for (const variable of declared) { + const value = delivered.get(variable.key); + if (value === undefined) undelivered.push(variable.key); + else if (value !== variable.value) stale.push(variable.key); + } + if (undelivered.length > 0) problems.push(`${envShPath} is missing ${nameList(undelivered)}`); + if (stale.length > 0) { + problems.push( + `${envShPath} has a stale value for ${nameList(stale)}: env.yaml declares a different one`, + ); + } + } + + // Same resolution the injection runs, not a second copy of it. + const profilePath = teamConfig?.sharing?.env?.shellProfilePath ?? envHandler.detectShellProfile(); + const profile = await readFileSafe(profilePath); + const block = profile === null ? null : envBlockIn(profile); + + if (block === null) { + problems.push(`${profilePath} carries no TeamAI env block`); + } else if (envSh !== null && !envBlockLoads(block, envShPath)) { + problems.push( + `the block in ${profilePath} does not load ${envShPath}: a POSIX shell reads an unquoted ` + + 'backslash as an escape, so the `[ -f ... ]` test fails and `source` never runs', + ); + } + + return problems; +} + +/** + * The docs bundle has one destination rather than one per tool: `DocsHandler` + * copies the whole `docs/` tree into `sharing.docs.localDir`. So this check + * compares the two trees, file by file, rather than asking each tool. + */ +export async function buildDocsCheck(ctx: DoctorContext): Promise { + const { localConfig, teamConfig } = ctx; + if (!teamConfig) return []; + + const { DocsHandler, resolveDocsDestination } = await import('./resources/docs.js'); + const handler = new DocsHandler(); + const [item] = await handler.scanTeamForPull(teamConfig, localConfig); + if (!item) return []; + + const dest = resolveDocsDestination(teamConfig, localConfig); + const teamFiles = (await listFilesRecursive(item.sourcePath)) + // Same filter DocsHandler.pullItem copies with: dotfiles never travel. + .filter((file) => file.split('/').every((segment) => !segment.startsWith('.'))); + + // isFile, not merely "something is there": a directory sitting on the + // expected name, or a symlink with nothing behind it, would satisfy a plain + // existence check while the doc is no more readable than a missing one. + const missing: string[] = []; + for (const file of teamFiles) { + if (!await isReadableFile(path.join(dest, file))) missing.push(file); + } + + return [{ + name: 'Team docs delivered', + source: 'local', + check: async () => missing.length === 0, + fix: `Missing from ${dest}: ${nameList(missing)}. Run \`teamai pull --force\`: a plain ` + + 'pull skips a scope whose team repo has not changed, so it cannot restore these.', + }]; +} diff --git a/src/doctor.ts b/src/doctor.ts index 8ec76efa4..231fe9c42 100644 --- a/src/doctor.ts +++ b/src/doctor.ts @@ -1,25 +1,28 @@ import path from 'node:path'; import { detectProjectConfig, loadLocalConfig, loadTeamConfig } from './config.js'; -import fs from 'node:fs'; -import { expandHome, listFilesRecursive, pathExists, readFileSafe } from './utils/fs.js'; +import { pathExists, readFileSafe } from './utils/fs.js'; import { log, setStderrOnly } from './utils/logger.js'; -import type { GlobalOptions, ResourceItem } from './types.js'; +import type { GlobalOptions } from './types.js'; import { COPILOT_TOOL_ID, - TEAMAI_ENV_START, resolveHookScope, resolveToolBaseDir, - getDataHome, isAgentExcluded, scopedToolPaths, type LocalConfig, type TeamaiConfig, } from './types.js'; import { isToolInstalledForConfig } from './resources/base.js'; -import { skillsReachTool } from './resources/skills.js'; -import { splitFrontmatter } from './utils/frontmatter.js'; +import { skillsDirForTool } from './resources/skills.js'; import { TEAMAI_HOOK_SUBCOMMANDS, isCodexTrustGatedTool, codexTrustReminder } from './hooks.js'; -import { getUserHome } from './utils/home.js'; +import { + buildDeliveryChecks, + buildRulesDeliveryChecks, + buildAgentsDeliveryChecks, + buildMcpDeliveryChecks, + buildEnvDeliveryCheck, + buildDocsCheck, +} from './doctor-delivery.js'; /** * Where a check gets its answer. `provider` checks shell out to a provider CLI @@ -125,7 +128,7 @@ async function buildEnabledToolChecks(ctx: DoctorContext): Promise { // such resolver, so it keeps the generic probe. const skillsPath = paths.skills; const isInstalled = skillsPath - ? (): Promise => skillsReachTool(tool, skillsPath, localConfig) + ? async (): Promise => await skillsDirForTool(tool, skillsPath, localConfig) !== null : (): Promise => isToolInstalledForConfig(tool, probePath, localConfig); // Pushed whether or not it passes. Every other check in the registry @@ -189,157 +192,6 @@ async function buildHookChecks( return checks; } -/** - * Whether a delivered skill directory is one an agent can actually discover: - * SKILL.md present, frontmatter parses, and its `name` is the directory's own. - * A copy that fails this landed successfully — no write-time gate can see it. - */ -async function skillIsDiscoverable(skillDir: string, skillName: string): Promise { - const content = await readFileSafe(path.join(skillDir, 'SKILL.md')); - if (!content) return false; - - const { data, valid } = splitFrontmatter(content); - if (!valid) return false; - return data.name === skillName; -} - -/** - * Whether `filePath` is a file something can actually read. `pathExists` - * follows symlinks but says yes to a directory too, so on its own it cannot - * tell a delivered document from a name occupied by something else. - */ -async function isReadableFile(filePath: string): Promise { - try { - return (await fs.promises.stat(expandHome(filePath))).isFile(); - } catch { - return false; - } -} - -/** At most this many names in a fix string; the rest are counted. */ -const MAX_NAMED_IN_FIX = 5; - -/** `a, b, c and 4 more` — a fix a human reads, not a wall of paths. */ -function nameList(names: string[]): string { - if (names.length <= MAX_NAMED_IN_FIX) return names.join(', '); - const shown = names.slice(0, MAX_NAMED_IN_FIX).join(', '); - return `${shown} and ${names.length - MAX_NAMED_IN_FIX} more`; -} - -/** - * Build one delivery check per installed tool: every skill the member should - * have, against what is actually on disk for that tool. - * - * This is the only check that looks at the payload rather than the plumbing. A - * write-time gate cannot cover it — `SkillsHandler.pullItem` skips each - * uninstalled tool on its own, and a directory deleted by hand after a correct - * pull leaves every gate happy (#598). - * - * The scan runs here rather than inside `check()` because the fix names the - * skills that are missing, and a `Check`'s fix is read as it was built. - */ -async function buildDeliveryChecks(ctx: DoctorContext): Promise { - const { localConfig, teamConfig, toolPaths } = ctx; - if (!teamConfig) return []; - - // Dynamic: pull.ts imports this module for its post-pull pass, and the desired - // set is policy that must not be restated here. - const { buildRolePullContext, resolveDesiredSkills } = await import('./pull.js'); - const { skillTargetForTool } = await import('./resources/skills.js'); - - let items: ResourceItem[]; - try { - const roleContext = await buildRolePullContext(localConfig); - ({ items } = await resolveDesiredSkills(teamConfig, localConfig, roleContext)); - } catch (e) { - // A team repo whose active namespaces collide cannot say what should be - // delivered — `pull` aborts the scope with this same message. The command - // whose job is explaining bad state must report it, not stack-trace on it. - return [{ - name: 'Skills to deliver can be resolved', - source: 'local', - check: async () => false, - fix: `${(e as Error).message}. Until the team repo is fixed, ` - + 'pull cannot sync skills for this role.', - }]; - } - if (items.length === 0) return []; - - const checks: Check[] = []; - for (const [tool, paths] of Object.entries(toolPaths)) { - const skillsPath = paths.skills; - if (!skillsPath) continue; - - const missing: string[] = []; - const unreadable: string[] = []; - let installed = true; - for (const item of items) { - const dest = await skillTargetForTool(tool, skillsPath, localConfig, item.name); - if (!dest) { - // Not installed. Nothing was promised to this tool, so nothing is owed; - // a tool the user listed in enabledAgents is caught by its own check. - installed = false; - break; - } - if (!await pathExists(dest)) missing.push(item.name); - else if (!await skillIsDiscoverable(dest, item.name)) unreadable.push(item.name); - } - if (!installed) continue; - - const problems: string[] = []; - if (missing.length > 0) problems.push(`not delivered: ${nameList(missing)}`); - if (unreadable.length > 0) problems.push(`delivered but unreadable: ${nameList(unreadable)}`); - - checks.push({ - name: `Skills delivered to ${tool}`, - source: 'local', - check: async () => problems.length === 0, - fix: `In ${tool}, ${problems.join('; ')}. Run \`teamai pull --force\`: a plain pull ` - + 'skips a scope whose team repo has not changed, so it cannot restore this. ' - + 'If a skill stays unreadable, fix its SKILL.md in the team repo — the ' - + 'frontmatter needs a `name` matching the directory, or the agent never ' - + 'discovers it.', - }); - } - - return checks; -} - -/** - * The docs bundle has one destination rather than one per tool: `DocsHandler` - * copies the whole `docs/` tree into `sharing.docs.localDir`. So this check - * compares the two trees, file by file, rather than asking each tool. - */ -async function buildDocsCheck(ctx: DoctorContext): Promise { - const { localConfig, teamConfig } = ctx; - if (!teamConfig) return []; - - const { DocsHandler, resolveDocsDestination } = await import('./resources/docs.js'); - const handler = new DocsHandler(); - const [item] = await handler.scanTeamForPull(teamConfig, localConfig); - if (!item) return []; - - const dest = resolveDocsDestination(teamConfig, localConfig); - const teamFiles = (await listFilesRecursive(item.sourcePath)) - // Same filter DocsHandler.pullItem copies with: dotfiles never travel. - .filter((file) => file.split('/').every((segment) => !segment.startsWith('.'))); - - // isFile, not merely "something is there": a directory sitting on the - // expected name, or a symlink with nothing behind it, would satisfy a plain - // existence check while the doc is no more readable than a missing one. - const missing: string[] = []; - for (const file of teamFiles) { - if (!await isReadableFile(path.join(dest, file))) missing.push(file); - } - - return [{ - name: 'Team docs delivered', - source: 'local', - check: async () => missing.length === 0, - fix: `Missing from ${dest}: ${nameList(missing)}. Run \`teamai pull --force\`: a plain ` - + 'pull skips a scope whose team repo has not changed, so it cannot restore these.', - }]; -} /** * True if a trust-gated Codex tool (the public `codex`) already has teamai hooks @@ -384,11 +236,26 @@ export async function resolveDoctorContext(): Promise { return { localConfig, teamConfig, toolPaths, baseDir }; } +/** + * Which caller the registry is being built for. + * + * `pull` runs the registry again at the end of an interactive sync, under a + * budget that covers building it as well as running it. Skills and docs cost a + * stat per item; rules cost a read per rule per tool and agents parse every + * spec. Spending the budget on those loses the cheap checks that catch the bug + * this whole line of work exists for, so they are `doctor`-only. + * + * The stage is a property of the caller, not of a check, which is why it is an + * argument here rather than a third optional flag on `Check` beside `source` + * and `reportedByPull`. + */ +export type CheckStage = 'pull' | 'doctor'; + /** * The check registry. Exported so callers other than `teamai doctor` can run * the same diagnostics and act on the result. */ -export async function buildChecks(ctx: DoctorContext): Promise { +export async function buildChecks(ctx: DoctorContext, stage: CheckStage = 'doctor'): Promise { const { localConfig, teamConfig, toolPaths, baseDir } = ctx; const providerName = teamConfig?.provider; const checks: Check[] = []; @@ -485,37 +352,13 @@ export async function buildChecks(ctx: DoctorContext): Promise { ...await buildEnabledToolChecks(ctx), ...await buildHookChecks(toolPaths, baseDir, localConfig), ...await buildDeliveryChecks(ctx), + // Built only for `doctor`: the work is in building these, not in running + // them, so skipping them post-pull is what keeps the budget for the rest. + ...(stage === 'doctor' ? await buildRulesDeliveryChecks(ctx) : []), + ...(stage === 'doctor' ? await buildAgentsDeliveryChecks(ctx) : []), + ...await buildMcpDeliveryChecks(ctx), ...await buildDocsCheck(ctx), - { - name: 'Env variables injected in shell profile', - source: 'local', - check: async () => { - if (teamConfig?.sharing?.env?.injectShellProfile === false) return true; - - const envYamlPath = path.join(localConfig.repo.localPath, 'env', 'env.yaml'); - if (!await pathExists(envYamlPath)) return true; - - const home = getUserHome(); - - // env.sh lives under teamaiHome, which is /.teamai in - // project scope and ~/.teamai in user scope — mirror the path that - // `teamai pull` actually writes to, not a hardcoded user-home path. - const envShPath = path.join( - getDataHome(localConfig), - 'env.sh', - ); - if (!await pathExists(envShPath)) return false; - - const shell = process.env.SHELL ?? ''; - const profilePath = shell.includes('zsh') - ? path.join(home, '.zshrc') - : path.join(home, '.bashrc'); - if (!await pathExists(profilePath)) return false; - const content = await readFileSafe(profilePath); - return content?.includes(TEAMAI_ENV_START) ?? false; - }, - fix: 'Run `teamai pull` to inject env variables into shell profile', - }, + ...await buildEnvDeliveryCheck(ctx), ); return checks; diff --git a/src/hermes-config.ts b/src/hermes-config.ts index eab540994..0e1552e82 100644 --- a/src/hermes-config.ts +++ b/src/hermes-config.ts @@ -127,6 +127,22 @@ export async function upsertSoulRules(rulesText: string): Promise { await writeFile(filePath, merged + '\n'); } +/** + * The text inside the teamai-managed block of SOUL.md, or null when the block + * is not there. Read-only: `doctor` has to compare what Hermes actually reads + * with what `upsertSoulRules` would write, and reusing the writer to find out + * would edit the file the command is only meant to describe. + */ +export async function readSoulRules(): Promise { + const content = await readFileSafe(getHermesSoulPath()); + if (content === null) return null; + + const startIdx = content.indexOf(RULES_BLOCK_START); + const endIdx = content.indexOf(RULES_BLOCK_END); + if (startIdx === -1 || endIdx === -1 || endIdx < startIdx) return null; + return content.slice(startIdx + RULES_BLOCK_START.length, endIdx).trim(); +} + /** * Remove the teamai-managed rules block from Hermes SOUL.md, leaving user * content intact. No-op when nothing is present. diff --git a/src/mcp-reconcile.ts b/src/mcp-reconcile.ts index 2fce08357..e2cd2d76a 100644 --- a/src/mcp-reconcile.ts +++ b/src/mcp-reconcile.ts @@ -280,15 +280,7 @@ export async function writeJsonDoc( * rest of config.toml byte-identical (comments included). */ export function spliceCodexBlock(source: string, name: string, block: string | null): string { - const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); - // The block runs from its header to the next table header that is not one of - // its own sub-tables (e.g. [mcp_servers..env]), or to end-of-input. - // End-of-input must be spelled `(?![\s\S])`: JS has no \z, and under the `m` - // flag `$` only means end-of-line, which would truncate the match early. - const re = new RegExp( - String.raw`^\[mcp_servers\.${escaped}\]\s*$[\s\S]*?(?=^\[(?!mcp_servers\.${escaped}[.\]])|(?![\s\S]))`, - 'm', - ); + const re = codexBlockRe(name); const match = source.match(re); if (match) { @@ -304,6 +296,32 @@ export function spliceCodexBlock(source: string, name: string, block: string | n return source + sep + block; } +/** + * Matches one `[mcp_servers.]` block, from its header to the next table + * header that is not one of its own sub-tables (e.g. [mcp_servers..env]), + * or to end-of-input. End-of-input must be spelled `(?![\s\S])`: JS has no `\z`, + * and under the `m` flag `$` only means end-of-line, which would truncate the + * match early. + */ +function codexBlockRe(name: string): RegExp { + const escaped = name.replace(/[.*+?^${}()|[\]\\]/g, '\\$&'); + return new RegExp( + String.raw`^\[mcp_servers\.${escaped}\]\s*$[\s\S]*?(?=^\[(?!mcp_servers\.${escaped}[.\]])|(?![\s\S]))`, + 'm', + ); +} + +/** + * The text of one `[mcp_servers.]` block, trimmed to the single trailing + * newline `renderCodexBlock` emits so the two forms compare directly — the + * splice pads a written block with a blank line to separate it from the next + * table. + */ +export function codexBlockIn(source: string, name: string): string | null { + const match = source.match(codexBlockRe(name)); + return match === null ? null : match[0].trimEnd() + '\n'; +} + /** Extract the names of all `[mcp_servers.X]` tables present in a config.toml. */ export function codexServerNames(source: string): string[] { const names = new Set(); @@ -311,9 +329,148 @@ export function codexServerNames(source: string): string[] { return [...names]; } +// ─── Desired set ───────────────────────────────────────────── + +/** One team server in the rendered form that lands in a tool's own config. */ +export interface DesiredMcpEntry { + entry: unknown; + hash: string; + /** Codex alone stores a TOML block rather than a JSON value. */ + block?: string; +} + +/** Everything the per-server filters need, resolved once per run. */ +export interface DesiredMcpContext { + sharing: ReturnType; + excluded: Set; + /** null when the member has no role: every `roles:` entry then applies. */ + activeRoles: string[] | null; + vars: Record; + lookPath?: McpReconcileOptions['lookPath']; +} + +export async function buildDesiredMcpContext( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + options: McpReconcileOptions = {}, +): Promise { + return { + sharing: getMcpSharing(teamConfig), + excluded: new Set(localConfig.excludedSkills ?? []), + activeRoles: activeRoleIds(localConfig), + vars: await buildVarTable(localConfig), + lookPath: options.lookPath, + }; +} + +/** + * Which of `teamDefs` apply to `target`, rendered the way they land in the + * tool's config, and a skip entry naming why each of the rest does not. + * + * Exported so `doctor` can check what should have arrived without restating + * the filters (#624). A second copy of them is how an MCP server ends up + * skipped for `unresolved variable(s)` during one pull and reported as + * correctly delivered forever after. + */ +export function desiredMcpForTarget( + target: McpTarget, + teamDefs: McpServerDef[], + ctx: DesiredMcpContext, +): { desired: Map; skipped: McpChange[] } { + const desired = new Map(); + const skipped: McpChange[] = []; + + for (const raw of teamDefs) { + if (raw.tools && !raw.tools.includes(target.tool)) continue; + if (!matchesRoles(raw.roles, ctx.activeRoles)) continue; + if (ctx.excluded.has(raw.name)) { + skipped.push({ tool: target.tool, server: raw.name, action: 'skipped', reason: 'excluded by user' }); + continue; + } + if (!supportsTransport(target.format, raw.transport)) { + skipped.push({ + tool: target.tool, + server: raw.name, + action: 'skipped', + reason: `${target.tool} does not support ${raw.transport} transport`, + }); + continue; + } + const violation = policyViolation(raw, ctx.sharing); + if (violation) { + skipped.push({ tool: target.tool, server: raw.name, action: 'skipped', reason: violation }); + continue; + } + const missingBin = requirementsMet(raw, ctx.lookPath); + if (missingBin) { + skipped.push({ tool: target.tool, server: raw.name, action: 'skipped', reason: missingBin }); + continue; + } + + // 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, including + // project-scope files that get committed — the team has opted into that + // by declaring the server with a ${VAR} a tool cannot expand itself. + const passthrough = supportsEnvExpansion(target.format, target.projectScope, raw); + let def = raw; + if (!passthrough) { + const { def: resolved, missing } = resolvePlaceholders(raw, ctx.vars); + if (missing.length > 0) { + skipped.push({ + tool: target.tool, + server: raw.name, + action: 'skipped', + reason: `unresolved variable(s): ${missing.join(', ')}`, + }); + continue; + } + def = resolved; + } else if (referencedVars(raw).length > 0) { + log.debug(`${raw.name}: passing ${referencedVars(raw).join(', ')} through to ${target.tool}`); + } + + if (target.format === 'codex') { + const block = renderCodexBlock(def); + desired.set(raw.name, { entry: block, hash: entryHash(block), block }); + } else { + const entry = renderJsonEntry(target.format, def); + desired.set(raw.name, { entry, hash: entryHash(entry) }); + } + } + + return { desired, skipped }; +} + +/** + * The MCP server entries already present in `target`'s own config file, in the + * same rendered form `desiredMcpForTarget` produces, or null when the file + * exists and cannot be parsed — the same condition that makes the write path + * abandon the injection rather than clobber a file it does not understand. + * + * Entries rather than names, because a name being present does not mean the + * team's server arrived: the appliers refuse to overwrite an entry teamai does + * not own, so an unrelated server of the same name leaves the key there and the + * team's definition undelivered. Only the value tells those two apart. + * + * Read-only. An MCP server is an entry inside a tool's config rather than a + * file of its own, so this, not a destination path, is what "delivered" means. + */ +export async function installedMcpEntries(target: McpTarget): Promise | null> { + if (target.format === 'codex') { + const raw = await readFileSafe(target.file); + if (raw === null) return new Map(); + return new Map(codexServerNames(raw).map((name) => [name, codexBlockIn(raw, name)])); + } + const serverKey = MCP_SERVER_KEY[target.format as Exclude]; + const allowBare = target.format === 'copilot' && target.projectScope; + const doc = await readJsonDoc(target.file, serverKey, allowBare); + return doc === null ? null : new Map(Object.entries(doc.servers)); +} + // ─── Main entry ────────────────────────────────────────────── -function mcpTargetExcluded(localConfig: LocalConfig, target: McpTarget): boolean { +export function mcpTargetExcluded(localConfig: LocalConfig, target: McpTarget): boolean { if (!isAgentExcluded(localConfig, target.tool)) return false; // tclaude has no project-scope MCP file: it reads the /.mcp.json the // claude target writes, so that target stays live while tclaude is enabled. @@ -351,9 +508,6 @@ export async function reconcileMcpForConfig( return { changes, wrote }; } - const excluded = new Set(localConfig.excludedSkills ?? []); - // Role filter (mcp.yaml `roles:`): same shape as `tools:`, applied per member. - const activeRoles = activeRoleIds(localConfig); if (!removeAll) { await warnUnknownRoleIds( localConfig.repo.localPath, @@ -383,7 +537,7 @@ export async function reconcileMcpForConfig( const nothingOwned = Object.values(manifest).every((r) => r.length === 0); if (teamDefs.length === 0 && nothingOwned) return { changes, wrote }; - const vars = await buildVarTable(localConfig); + const desiredContext = await buildDesiredMcpContext(teamConfig, localConfig, options); for (const target of targets) { // Same enabledAgents / disabledAgents gate as the other resource syncs. The @@ -396,66 +550,8 @@ export async function reconcileMcpForConfig( const nextRecords: ManagedMcpRecord[] = []; // Which of this team's servers apply to this tool, and in what rendered form. - const desired = new Map(); - - for (const raw of teamDefs) { - if (raw.tools && !raw.tools.includes(target.tool)) continue; - if (!matchesRoles(raw.roles, activeRoles)) continue; - if (excluded.has(raw.name)) { - changes.push({ tool: target.tool, server: raw.name, action: 'skipped', reason: 'excluded by user' }); - continue; - } - if (!supportsTransport(target.format, raw.transport)) { - changes.push({ - tool: target.tool, - server: raw.name, - action: 'skipped', - reason: `${target.tool} does not support ${raw.transport} transport`, - }); - continue; - } - const violation = policyViolation(raw, sharing); - if (violation) { - changes.push({ tool: target.tool, server: raw.name, action: 'skipped', reason: violation }); - continue; - } - const missingBin = requirementsMet(raw, options.lookPath); - if (missingBin) { - changes.push({ tool: target.tool, server: raw.name, action: 'skipped', reason: missingBin }); - continue; - } - - // 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, including - // project-scope files that get committed — the team has opted into that - // by declaring the server with a ${VAR} a tool cannot expand itself. - const passthrough = supportsEnvExpansion(target.format, target.projectScope, raw); - let def = raw; - if (!passthrough) { - const { def: resolved, missing } = resolvePlaceholders(raw, vars); - if (missing.length > 0) { - changes.push({ - tool: target.tool, - server: raw.name, - action: 'skipped', - reason: `unresolved variable(s): ${missing.join(', ')}`, - }); - continue; - } - def = resolved; - } else if (referencedVars(raw).length > 0) { - log.debug(`${raw.name}: passing ${referencedVars(raw).join(', ')} through to ${target.tool}`); - } - - if (target.format === 'codex') { - const block = renderCodexBlock(def); - desired.set(raw.name, { entry: block, hash: entryHash(block), block }); - } else { - const entry = renderJsonEntry(target.format, def); - desired.set(raw.name, { entry, hash: entryHash(entry) }); - } - } + const { desired, skipped } = desiredMcpForTarget(target, teamDefs, desiredContext); + changes.push(...skipped); if (target.format === 'codex') { wrote = await applyCodex(target, desired, ownedNames, nextRecords, changes, options) || wrote; diff --git a/src/pull.ts b/src/pull.ts index 63e95ff73..b7bcfcb42 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -11,6 +11,7 @@ import { pathExists, remove, listFiles, listDirs, listFilesRecursive, readFileSa import { injectClaudeMdSection, removeClaudeMdSection } from './utils/claudemd.js'; import { getHandler, RulesHandler, DocsHandler, EnvHandler, AgentsHandler } from './resources/index.js'; import { isToolInstalledForConfig, ResourceHandler } from './resources/base.js'; +import { skillsDirForTool } from './resources/skills.js'; import { ruleFileExtensionForTool } from './resources/rule-format.js'; import { AGENT_FILE_EXTENSIONS } from './resources/agent-format.js'; import { loadTagsConfig, filterByTags } from './utils/tags.js'; @@ -383,6 +384,47 @@ export async function resolveDesiredSkills( return { items, teamItems, skippedByTags }; } +export interface DesiredRules { + /** The rules this member should have: active knowledge namespaces ∩ tag subscriptions. */ + items: ResourceItem[]; + /** How many rules the tag channel left out, for the sync line. */ + skippedByTags: number; +} + +/** + * Resolve the rules this member should have. Same contract as + * `resolveDesiredSkills`, and for the same reason: `pull` calls it to decide + * what to install and `doctor` calls it to check what landed (#624), so the + * namespace convention and the tag channel are stated once. + */ +export async function resolveDesiredRules( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + roleContext: RolePullContext | null, +): Promise { + const handler = getHandler('rules'); + const tagsConfig = await loadTagsConfig(localConfig.repo.localPath); + const allItems = await handler.scanTeamForPull(teamConfig, localConfig); + const knowledgeNs = roleContext ? roleContext.activeNamespaces.knowledge : null; + const roleFiltered = filterRulesByKnowledgeNamespaces(allItems, knowledgeNs); + const { included, skipped } = filterByTags(roleFiltered, tagsConfig, localConfig.subscribedTags, 'rules'); + return { items: included, skippedByTags: skipped.length }; +} + +/** + * Resolve the agents this member should have. Throws on a stem collision + * between two active namespaces, the same way `pull` aborts the scope: a + * caller that cannot say what should be delivered must not guess. + */ +export async function resolveDesiredAgents( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + roleContext: RolePullContext | null, +): Promise { + const items = await getHandler('agents').scanTeamForPull(teamConfig, localConfig); + return filterAgentsByNamespaces(items, roleContext ? roleContext.activeNamespaces.agents : null); +} + // Deployment adds a CONTRIBUTORS file that the team source may not have; ignore it // when checking whether a deployed skill still matches its source (same file as // resources/skills.ts and pre-push-sync.ts use for modification detection). @@ -427,21 +469,22 @@ export async function cleanupInactiveNamespaceSkills( inactiveSkillNames: Set, inactiveSkillSources?: Map, ): Promise { - const baseDir = resolveBaseDir(localConfig); - for (const [tool, toolPath] of Object.entries(scopedToolPaths(teamConfig, localConfig))) { if (isAgentExcluded(localConfig, tool)) continue; - if (!toolPath.skills) continue; - if (!await ResourceHandler.isToolInstalled(toolPath.skills, baseDir)) continue; - if (!await pathExists(path.join(baseDir, toolPath.skills))) continue; - - const localSkillNames = await listDirs(path.join(baseDir, toolPath.skills)); + // Ask where delivery writes, not where the tool root sits: OpenClaw keeps + // its skills under a workspace directory, so the generic probe sweeps a + // directory a pull never wrote to and leaves the real one untouched (#624). + const skillsDir = await skillsDirForTool(tool, toolPath.skills, localConfig); + if (skillsDir === null) continue; + if (!await pathExists(skillsDir)) continue; + + const localSkillNames = await listDirs(skillsDir); for (const skillName of localSkillNames) { if (BUILTIN_SKILL_NAMES.has(skillName)) continue; if (retainedSkillNames.has(skillName)) continue; if (!inactiveSkillNames.has(skillName)) continue; - const localSkillDir = path.join(baseDir, toolPath.skills, skillName); + const localSkillDir = path.join(skillsDir, skillName); // Data-safety guard: only delete a deployed skill when it is byte-identical // to its team-repo source. If the user modified SKILL.md or added unpushed @@ -759,9 +802,6 @@ async function pullForScope( } } - // Load tags config for filtering - const tagsConfig = await loadTagsConfig(localConfig.repo.localPath); - const subscribedTags = localConfig.subscribedTags; const excludedSkills = new Set(localConfig.excludedSkills ?? []); // Step 2: Sync each resource type @@ -778,14 +818,10 @@ async function pullForScope( if (type === 'rules') { const rulesHandler = handler as RulesHandler; - const allItems = await rulesHandler.scanTeamForPull(freshConfig, localConfig); - // Filter by role knowledge namespaces first, then by tags - const knowledgeNs = roleContext ? roleContext.activeNamespaces.knowledge : null; - const roleFiltered = filterRulesByKnowledgeNamespaces(allItems, knowledgeNs); - const { included: items, skipped } = filterByTags(roleFiltered, tagsConfig, subscribedTags, 'rules'); + const { items, skippedByTags } = await resolveDesiredRules(freshConfig, localConfig, roleContext); if (options.dryRun) { if (items.length > 0) { - log.info(`[${scopeLabel}] [dry-run] Would sync ${items.length} rule(s)${skipped.length > 0 ? ` (skipped ${skipped.length} by tags)` : ''}`); + log.info(`[${scopeLabel}] [dry-run] Would sync ${items.length} rule(s)${skippedByTags > 0 ? ` (skipped ${skippedByTags} by tags)` : ''}`); } } else { // Always call pullAllRules, even with an empty set: it also cleans up @@ -794,7 +830,7 @@ async function pullForScope( // would leak those artifacts on the machine after upstream deletion. await rulesHandler.pullAllRules(freshConfig, localConfig, items); if (items.length > 0) { - log.success(`[${scopeLabel}] Synced ${items.length} rule(s)${skipped.length > 0 ? ` (skipped ${skipped.length} by tags)` : ''}`); + log.success(`[${scopeLabel}] Synced ${items.length} rule(s)${skippedByTags > 0 ? ` (skipped ${skippedByTags} by tags)` : ''}`); } } totalSynced += items.length; @@ -812,12 +848,9 @@ async function pullForScope( knownRepoSkillNames = new Set(desired.teamItems.map((i) => i.name)); knownRepoSkillSources = new Map(desired.teamItems.map((i) => [i.name, i.sourcePath])); } else if (type === 'agents') { - // Role/project namespace filter (root = everyone), same as rules. Throws - // on a stem collision; the caller's try/catch logs it and aborts the scope. - items = filterAgentsByNamespaces( - await handler.scanTeamForPull(freshConfig, localConfig), - roleContext ? roleContext.activeNamespaces.agents : null, - ); + // Throws on a stem collision; the caller's try/catch logs it and aborts + // the scope. + items = await resolveDesiredAgents(freshConfig, localConfig, roleContext); } else { items = await handler.scanTeamForPull(freshConfig, localConfig); } @@ -1927,10 +1960,12 @@ async function reportPostPullChecks( // The budget covers building the registry as well as running it: the // delivery checks stat every desired skill for every tool while the - // registry is built, which is where the I/O actually is. + // registry is built, which is where the I/O actually is. The `'pull'` + // stage leaves out the two that would spend it — rules read every file per + // tool, agents parse every spec — so the cheap ones still get to run. const results = await withTimeout( (async () => { - const local = (await buildChecks(ctx)) + const local = (await buildChecks(ctx, 'pull')) .filter((c) => c.source === 'local') .filter((c) => !c.reportedByPull || !reported.has(c.reportedByPull)); return runChecks(local); diff --git a/src/resources/agents.ts b/src/resources/agents.ts index de2da94c2..33058a1e0 100644 --- a/src/resources/agents.ts +++ b/src/resources/agents.ts @@ -2,7 +2,7 @@ import path from 'node:path'; import { isDeepStrictEqual } from 'node:util'; import { parse as parseYaml } from 'yaml'; import { isToolInstalledForConfig, ResourceHandler } from './base.js'; -import type { ResourceItem, ResourceItemStatus, TeamaiConfig, LocalConfig } from '../types.js'; +import type { ResourceItem, ResourceItemStatus, DeliveryTarget, TeamaiConfig, LocalConfig } from '../types.js'; import { listFiles, listDirs, pathExists, copyFile, ensureDir, remove, fileContentEqual, getFileMtime, writeFile, readFileSafe } from '../utils/fs.js'; import { log } from '../utils/logger.js'; import { resolveToolBaseDir, isAgentExcluded, isSelfMode, scopedToolPaths } from '../types.js'; @@ -385,53 +385,36 @@ export class AgentsHandler extends ResourceHandler { const agentItem = item as AgentResourceItem; // Determine format: explicit flag takes precedence; fall back to extension detection - const isLegacy = agentItem.legacy === true || (!agentItem.legacy && !item.sourcePath.endsWith('.yaml')); - - if (isLegacy) { - // Legacy: copy .md to tools that support agents - await this.pullLegacyMd(item, teamConfig, localConfig); - return; - } - - // New YAML format: parse + render per-tool const content = await readFileSafe(item.sourcePath); - if (!content) { + if (content === null) { log.warn(`agents: cannot read ${item.sourcePath}`); return; } - let spec: AgentSpec; - const parseResult: ParseResult = parseAgentYaml(content, item.name + '.yaml'); - if (!parseResult.ok) { - console.warn(`[agents] 解析失败 ${item.name}.yaml: ${parseResult.reason}, 已跳过`); - return; - } - spec = parseResult.spec; - - const targets = spec.targets ?? ALL_SUPPORTED_TOOLS; - const scoped = scopedToolPaths(teamConfig, localConfig); - - for (const tool of targets) { - const toolPath = scoped[tool]; - if (!toolPath?.agents) { - log.debug(`Skipping agent sync for ${tool}: no agents path configured`); - continue; - } - if (!await isToolInstalledForConfig(tool, toolPath.agents, localConfig)) { - log.debug(`Skipping agent sync for ${tool}: tool not installed`); - continue; + // Say why an agent reaches nothing before the loop silently delivers + // nowhere: `resolveRenders` skips an unparsable spec for every tool alike. + if (!isLegacyAgent(agentItem)) { + const parseResult: ParseResult = parseAgentYaml(content, `${item.name}.yaml`); + if (!parseResult.ok) { + log.warn(`[agents] Skipped ${item.name}.yaml: ${parseResult.reason}`); + return; } - if (isAgentExcluded(localConfig, tool)) continue; + } - const baseDir = resolveToolBaseDir(tool, localConfig); - const destDir = path.join(baseDir, toolPath.agents); + for (const { tool, dest, render } of await this.resolveRenders(teamConfig, localConfig, item)) { + const destDir = path.dirname(dest); try { await ensureDir(destDir); - const { ext, content: rendered } = renderForTool(spec, tool); - await removeStaleAgentSiblings(destDir, item.name, ext); - const dest = path.join(destDir, `${item.name}${ext}`); - await writeFile(dest, rendered); - log.debug(`Rendered agent ${item.name} → ${tool} (${ext})`); + // Only a rendered spec can leave a sibling behind: its extension follows + // the tool's format and changes when `targets` does. A legacy `.md` is + // copied verbatim to one extension for every tool, so a same-stem + // `.toml`, `.json` or `.agent.md` beside it is the member's own file + // and not ours to delete (#624 review). + if (!isLegacyAgent(agentItem)) { + await removeStaleAgentSiblings(destDir, item.name, render.ext); + } + await writeFile(dest, render.content); + log.debug(`Rendered agent ${item.name} → ${tool} (${render.ext})`); } catch (e) { log.warn(`Failed to sync agent ${item.name} to ${tool}: ${(e as Error).message}`); } @@ -496,12 +479,7 @@ export class AgentsHandler extends ResourceHandler { const inactive = items.filter((item) => !isActive(item) && !BUILTIN_AGENT_NAMES.has(item.name)); if (inactive.length === 0) return; - for (const [tool, toolPath] of Object.entries(scopedToolPaths(teamConfig, localConfig))) { - if (!toolPath.agents || !isKnownTool(tool) || isAgentExcluded(localConfig, tool)) continue; - if (!await isToolInstalledForConfig(tool, toolPath.agents, localConfig)) continue; - const baseDir = resolveToolBaseDir(tool, localConfig); - const destDir = path.join(baseDir, toolPath.agents); - + for (const { tool, dir: destDir } of await this.agentToolDirs(teamConfig, localConfig)) { const activeDestinations = new Set(); for (const item of active) { const rendered = await this.renderedForTool(item, tool); @@ -523,6 +501,69 @@ export class AgentsHandler extends ResourceHandler { } } + /** + * Every tool that receives `item`, with the path and the bytes `pullItem` + * writes there. + * + * An agent's desired set is a relation, not a product: a YAML spec carries + * `targets`, a legacy `.md` only reaches LEGACY_MD_TOOLS, and the filename + * extension comes from the render rather than the item. So this is the only + * place that can answer where an agent lands. + */ + private async resolveRenders( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + item: ResourceItem, + ): Promise<{ tool: ToolName; dest: string; render: RenderResult }[]> { + const agentItem = item as AgentResourceItem; + const renders: { tool: ToolName; dest: string; render: RenderResult }[] = []; + + for (const { tool, dir } of await this.agentToolDirs(teamConfig, localConfig)) { + const render = await this.renderedForTool(agentItem, tool); + if (!render) continue; + + renders.push({ tool, dest: path.join(dir, `${item.name}${render.ext}`), render }); + } + + return renders; + } + + /** + * Every installed tool that receives agents at all, with the directory its + * copies land in — the gate, without asking any agent to render. + * + * `doctor` needs this on its own. "This agent reaches no tool" is a team-repo + * problem only once some tool was there to receive it, and taking the + * successful renders as proof of that hides the case where every agent is + * malformed: no render, no tool, no failure reported (#624). + */ + async agentToolDirs( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + ): Promise<{ tool: ToolName; dir: string }[]> { + const dirs: { tool: ToolName; dir: string }[] = []; + + for (const [tool, toolPath] of Object.entries(scopedToolPaths(teamConfig, localConfig))) { + if (!toolPath.agents || !isKnownTool(tool) || isAgentExcluded(localConfig, tool)) continue; + if (!await isToolInstalledForConfig(tool, toolPath.agents, localConfig)) { + log.debug(`Skipping agent sync for ${tool}: tool not installed`); + continue; + } + dirs.push({ tool, dir: path.join(resolveToolBaseDir(tool, localConfig), toolPath.agents) }); + } + + return dirs; + } + + async deliveryTargets( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + item: ResourceItem, + ): Promise { + return (await this.resolveRenders(teamConfig, localConfig, item)) + .map(({ tool, dest, render }) => ({ tool, dest, content: render.content })); + } + /** * What `pullItem` writes for this agent on this tool, or null when the tool * is not a target (legacy `.md` only reaches LEGACY_MD_TOOLS, a YAML spec @@ -531,7 +572,7 @@ export class AgentsHandler extends ResourceHandler { private async renderedForTool(item: AgentResourceItem, tool: ToolName): Promise { const content = await readFileSafe(item.sourcePath); if (content === null) return null; - if (item.legacy) { + if (isLegacyAgent(item)) { return LEGACY_MD_TOOLS.has(tool) ? { ext: '.md', content } : null; } const parsed = parseAgentYaml(content, `${item.name}.yaml`); @@ -542,38 +583,6 @@ export class AgentsHandler extends ResourceHandler { // ─── Private helpers ────────────────────────────────────────────────────── - /** - * Legacy pull: copies .md as-is to Claude-compatible tools, including JoyCode. - */ - private async pullLegacyMd( - item: ResourceItem, - teamConfig: TeamaiConfig, - localConfig: LocalConfig, - ): Promise { - for (const [tool, toolPath] of Object.entries(scopedToolPaths(teamConfig, localConfig))) { - if (!LEGACY_MD_TOOLS.has(tool)) continue; - if (!toolPath.agents) { - log.debug(`Skipping legacy agent sync for ${tool}: no agents path configured`); - continue; - } - if (!await isToolInstalledForConfig(tool, toolPath.agents, localConfig)) { - log.debug(`Skipping legacy agent sync for ${tool}: tool not installed`); - continue; - } - if (isAgentExcluded(localConfig, tool)) continue; - - const baseDir = resolveToolBaseDir(tool, localConfig); - const destDir = path.join(baseDir, toolPath.agents); - try { - await ensureDir(destDir); - const dest = path.join(destDir, `${item.name}.md`); - await copyFile(item.sourcePath, dest); - log.debug(`Synced legacy agent ${item.name} → ${tool}`); - } catch (e) { - log.warn(`Failed to sync legacy agent ${item.name} to ${tool}: ${(e as Error).message}`); - } - } - } } // ─── Module-level helpers ────────────────────────────────────────────────── @@ -675,6 +684,17 @@ async function removeStaleAgentSiblings(agentsDir: string, stem: string, targetE } } +/** + * Whether an agent is the legacy `.md` kind, copied verbatim to Claude-shaped + * tools rather than rendered from a spec. `scanTeamForPull` sets the flag; a + * caller that builds an item by hand may not, so the source extension decides + * when it is absent. Pull and the delivery check must agree on this, or one + * renders a `.md` body as YAML while the other copies it. + */ +function isLegacyAgent(item: AgentResourceItem): boolean { + return item.legacy === true || !item.sourcePath.endsWith('.yaml'); +} + /** * Check if a tool name is a known agent-capable tool. */ diff --git a/src/resources/base.ts b/src/resources/base.ts index 656bdb617..0280d997d 100644 --- a/src/resources/base.ts +++ b/src/resources/base.ts @@ -1,6 +1,6 @@ import path from 'node:path'; import { COPILOT_TOOL_ID, getCopilotHome, resolveToolBaseDir } from '../types.js'; -import type { ResourceType, ResourceItem, ResourceDiff, TeamaiConfig, LocalConfig } from '../types.js'; +import type { ResourceType, ResourceItem, ResourceDiff, DeliveryTarget, TeamaiConfig, LocalConfig } from '../types.js'; import { readFileSafe, writeFile, ensureDir, pathExists } from '../utils/fs.js'; import { getUserHome } from '../utils/home.js'; @@ -94,6 +94,30 @@ export abstract class ResourceHandler { localConfig: LocalConfig, ): Promise; + /** + * Where `item` lands for each tool that can receive it on this machine. + * + * Read-only by contract: resolving a destination must never write, so the + * same answer serves the sync and the checks that verify it. Two resolvers + * for one destination is how "Synced N skills" ends up true while a tool + * receives nothing (#598, #624). + * + * A tool that cannot receive the item is absent from the result: not + * installed, no configured path, or outside the item's own targets. + * + * The default is empty, which also covers a resource with no per-tool file + * destination at all. Docs land in one directory, env in one shell profile, + * and hooks and MCP are entries inside a tool's own config file, so those + * keep checks of their own instead of a sentinel tool. + */ + async deliveryTargets( + _teamConfig: TeamaiConfig, + _localConfig: LocalConfig, + _item: ResourceItem, + ): Promise { + return []; + } + /** * Check if an AI tool is installed by verifying its root directory exists. * e.g. for toolPath ".codebuddy/skills", checks if ~/.codebuddy/ exists. diff --git a/src/resources/env.ts b/src/resources/env.ts index 133b88c25..7ba3a2f5a 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -23,6 +23,11 @@ const EnvYamlSchema = z.object({ export type EnvVariable = z.infer; export type EnvYaml = z.infer; +/** A parsed env.yaml, or the reason it declares nothing. See `readEnvYaml`. */ +export type EnvYamlRead = + | { ok: true; variables: EnvVariable[] } + | { ok: false; reason: string }; + /** * Mask an env variable value for display. * Shows first 2 chars + "****", or "****" for very short values. @@ -42,6 +47,58 @@ function shellQuoteValue(value: string): string { return `'${value.replace(/'/g, "'\\''")}'`; } +/** + * Read back the assignments `generateEnvFile` writes, as key → value. + * + * The inverse of the generator, and it has to be: a YAML block scalar is a + * legal env value, and single-quoting one spans several physical lines. A + * reader that splits env.sh on newlines can never match such an export, so it + * reports a correctly delivered value as stale (#624 review). Lines that are + * not an `export KEY='...'` we wrote are skipped rather than guessed at. + */ +export function parseEnvFile(content: string): Map { + const PREFIX = 'export '; + const KEY = /^[A-Za-z_][A-Za-z0-9_]*$/; + const assignments = new Map(); + + let i = 0; + while (i < content.length) { + const eq = content.startsWith(PREFIX, i) ? content.indexOf('=', i + PREFIX.length) : -1; + const key = eq === -1 ? '' : content.slice(i + PREFIX.length, eq); + if (eq === -1 || !KEY.test(key) || content[eq + 1] !== "'") { + const nl = content.indexOf('\n', i); + if (nl === -1) break; + i = nl + 1; + continue; + } + + let j = eq + 2; + let value = ''; + let closed = false; + while (j < content.length) { + if (content[j] !== "'") { + value += content[j]; + j++; + } else if (content.startsWith("'\\''", j)) { + // The generator's encoding of a literal quote: close, escape, reopen. + value += "'"; + j += 4; + } else { + closed = true; + j++; + break; + } + } + // An unterminated quote means the rest of the file is not ours to read. + if (!closed) break; + + assignments.set(key, value); + i = content[j] === '\n' ? j + 1 : j; + } + + return assignments; +} + // ─── Handler ───────────────────────────────────────────── export class EnvHandler extends ResourceHandler { @@ -198,15 +255,45 @@ export class EnvHandler extends ResourceHandler { * Parse the env.yaml file and return variables. */ async parseEnvYaml(filePath: string): Promise { + const read = await this.readEnvYaml(filePath); + return { variables: read.ok ? read.variables : [] }; + } + + /** + * Parse env/env.yaml, keeping the reason a file yielded no variables. + * + * `parseEnvYaml` answers `[]` to four different files: absent, empty, + * `variables: []`, and a shorthand `KEY: value` mapping whose unknown + * top-level key zod drops (#662). Only the last is broken, so a caller that + * reports on the count alone either misses the bug or calls a deliberately + * empty configuration malformed (#624 review). + */ + async readEnvYaml(filePath: string): Promise { const content = await readFileSafe(filePath); - if (!content) return { variables: [] }; + if (content === null) return { ok: true, variables: [] }; + let raw: unknown; try { - const raw = YAML.parse(content); - return EnvYamlSchema.parse(raw); - } catch { - return { variables: [] }; + raw = YAML.parse(content); + } catch (e) { + return { ok: false, reason: `${filePath} is not valid YAML: ${(e as Error).message}` }; + } + // An empty document is a file with nothing to deliver, not a broken one. + if (raw === null || raw === undefined) return { ok: true, variables: [] }; + + if (typeof raw !== 'object' || Array.isArray(raw) || !('variables' in raw)) { + return { + ok: false, + reason: `${filePath} declares no variables. Its top-level key must be \`variables:\`, a list ` + + 'of `key`/`value` entries — a plain `KEY: value` mapping parses as an empty list', + }; } + + const parsed = EnvYamlSchema.safeParse(raw); + if (!parsed.success) { + return { ok: false, reason: `${filePath} does not match the env.yaml schema: ${parsed.error.message}` }; + } + return { ok: true, variables: parsed.data.variables }; } /** @@ -246,8 +333,12 @@ export class EnvHandler extends ResourceHandler { /** * Detect the user's shell profile path. + * + * Public because `doctor` has to check the same file the injection writes: + * a second spelling of this choice would check `.bashrc` while the pull + * wrote `.zshrc`, and report a correct install as broken. */ - private detectShellProfile(): string { + detectShellProfile(): string { const home = getUserHome(); const shell = process.env.SHELL ?? ''; diff --git a/src/resources/mcp.ts b/src/resources/mcp.ts index 188de638b..2d99ebc11 100644 --- a/src/resources/mcp.ts +++ b/src/resources/mcp.ts @@ -51,13 +51,31 @@ export function teamMcpYamlPath(repoPath: string): string { * absent or fails validation, so callers never act on a half-broken server set. */ export async function parseMcpYaml(repoPath: string): Promise { + const read = await readMcpYaml(repoPath); + if (read.ok) return read.yaml; + log.warn(`Invalid mcp.yaml format: ${read.reason} — skipping team MCP servers this run`); + return null; +} + +/** An mcp.yaml that is absent (`yaml: null`), parsed, or refused with a reason. */ +export type McpYamlRead = + | { ok: true; yaml: McpYaml | null } + | { ok: false; reason: string }; + +/** + * Read the team repo's mcp/mcp.yaml, keeping the reason a bad file yielded no + * servers. `parseMcpYaml` flattens both to `null`, which is right for a pull + * that can only skip the run — but a file that does not parse injects nothing + * into any tool, and a check that cannot tell it from a repo with no MCP at + * all reports `ok: true` over a team whose MCP is entirely broken (#624 review). + */ +export async function readMcpYaml(repoPath: string): Promise { const content = await readFileSafe(teamMcpYamlPath(repoPath)); - if (!content) return null; + if (!content) return { ok: true, yaml: null }; try { - return McpYamlSchema.parse(YAML.parse(content)); + return { ok: true, yaml: McpYamlSchema.parse(YAML.parse(content)) }; } catch (e) { - log.warn(`Invalid mcp.yaml format: ${(e as Error).message} — skipping team MCP servers this run`); - return null; + return { ok: false, reason: (e as Error).message }; } } diff --git a/src/resources/rules.ts b/src/resources/rules.ts index 61216bbd4..e27e775bf 100644 --- a/src/resources/rules.ts +++ b/src/resources/rules.ts @@ -1,6 +1,6 @@ import path from 'node:path'; import { isToolInstalledForConfig, ResourceHandler } from './base.js'; -import type { ResourceItem, ResourceItemStatus, TeamaiConfig, LocalConfig } from '../types.js'; +import type { ResourceItem, ResourceItemStatus, DeliveryTarget, TeamaiConfig, LocalConfig } from '../types.js'; import { listFilesRecursive, pathExists, copyFile, ensureDir, remove, fileContentEqual, getFileMtime, listDirs, readFileSafe, writeFile } from '../utils/fs.js'; import { log } from '../utils/logger.js'; import { TEAMAI_RULES_START, TEAMAI_RULES_END, resolveBaseDir, resolveToolBaseDir, isAgentExcluded, scopedToolPaths } from '../types.js'; @@ -179,9 +179,23 @@ export class RulesHandler extends ResourceHandler { } /** - * Pull a single rule file to all configured AI tool rules/ directories. + * Where `item` lands for each tool that receives rules. The filename is + * tool-dependent — `.md` verbatim, `.mdc` for Cursor-compatible tools, + * `.instructions.md` for Copilot — so a reader cannot derive it from the + * rule's name alone. */ - async pullItem(item: ResourceItem, teamConfig: TeamaiConfig, localConfig: LocalConfig): Promise { + async deliveryTargets( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + item: ResourceItem, + ): Promise { + // The bytes as well as the path: Cursor and Copilot read frontmatter this + // derives from the team `.md`, so a copy whose `globs`, `alwaysApply` or + // `applyTo` no longer match the source is inert in exactly the way a + // missing file is. Only a comparison against the render can see that, and + // the render belongs here rather than in a second copy inside `doctor`. + const source = await readFileSafe(item.sourcePath); + const targets: DeliveryTarget[] = []; for (const [tool, toolPath] of Object.entries(scopedToolPaths(teamConfig, localConfig))) { if (isAgentExcluded(localConfig, tool)) continue; if (!toolPath.rules) continue; @@ -193,29 +207,32 @@ export class RulesHandler extends ResourceHandler { } const destDir = path.join(resolveToolBaseDir(tool, localConfig), toolPath.rules); - await ensureDir(destDir); - const dest = path.join(destDir, `${item.name}${ruleFileExtensionForTool(tool)}`); + targets.push({ + tool, + dest: path.join(destDir, `${item.name}${ruleFileExtensionForTool(tool)}`), + content: source === null ? undefined : renderRuleForTool(tool, source), + }); + } + return targets; + } + + /** + * Pull a single rule file to all configured AI tool rules/ directories. + */ + async pullItem(item: ResourceItem, teamConfig: TeamaiConfig, localConfig: LocalConfig): Promise { + for (const { tool, dest, content } of await this.deliveryTargets(teamConfig, localConfig, item)) { + const destDir = path.dirname(dest); try { - if (usesCursorMdcRules(tool)) { - // Cursor-compatible tools need `.mdc` with derived frontmatter. - const raw = await readFileSafe(item.sourcePath); - if (raw === null) { - // Never write a stub always-on rule in place of an unreadable source. - throw new Error(`Cannot read rule source ${item.sourcePath}`); - } - await writeFile(dest, teamRuleToCursorMdc(raw)); - // Drop the `.md` copy left by an older layout; these tools do not read it. - await remove(path.join(destDir, `${item.name}.md`)); - } else if (usesCopilotInstructions(tool)) { - const raw = await readFileSafe(item.sourcePath); - if (raw === null) { - throw new Error(`Cannot read rule source ${item.sourcePath}`); - } - await writeFile(dest, teamRuleToCopilotInstructions(raw)); - await remove(path.join(destDir, `${item.name}.md`)); - } else { - await copyFile(item.sourcePath, dest); + if (content === undefined) { + // Never write a stub always-on rule in place of an unreadable source. + throw new Error(`Cannot read rule source ${item.sourcePath}`); } + await ensureDir(destDir); + await writeFile(dest, content); + // Drop the `.md` copy left by an older layout; a tool that reads a + // derived extension does not read it, and it would outlive the rule. + const legacyCopy = path.join(destDir, `${item.name}.md`); + if (dest !== legacyCopy) await remove(legacyCopy); log.debug(`Synced rule ${item.name} → ${tool}`); } catch (e) { log.warn(`Failed to sync rule ${item.name} to ${tool}: ${(e as Error).message}`); @@ -282,13 +299,8 @@ export class RulesHandler extends ResourceHandler { if (!isAgentExcluded(localConfig, 'hermes')) { const { getHermesHome } = await import('../hermes-home.js'); if (await pathExists(getHermesHome())) { - const bodies: string[] = []; - for (const rule of rules) { - const body = await readFileSafe(rule.sourcePath); - if (body && body.trim() !== '') bodies.push(body.trim()); - } const { upsertSoulRules } = await import('../hermes-config.js'); - await upsertSoulRules(bodies.join('\n\n')); + await upsertSoulRules(await hermesRulesText(rules)); } } @@ -400,29 +412,47 @@ export class RulesHandler extends ResourceHandler { localConfig: LocalConfig, present: boolean, ): Promise { - if (isAgentExcluded(localConfig, 'opencode')) return; - const scoped = scopedToolPaths(teamConfig, localConfig); - const paths = scoped['opencode']; - if (!paths?.rules) return; + const target = await this.opencodeInstructionsTarget(teamConfig, localConfig); + if (target === null) return; + + const { reconcileOpencodeInstructions } = await import('./opencode-config.js'); + try { + await reconcileOpencodeInstructions(target.configFile, target.glob, present); + } catch (e) { + log.warn(`Failed to update OpenCode instructions in ${target.configFile}: ${(e as Error).message}`); + } + } + + /** + * The opencode.json this scope activates rules through, and the one glob + * teamai owns inside it. Null when OpenCode receives no rules here: + * excluded, not installed, or configured without a rules or config path. + * + * Read-only, and public for the same reason `deliveryTargets` is: OpenCode + * does not auto-scan its rules directory, so a `.md` sitting there is inert + * until this glob references it. A check that derived the path a second time + * could look at a different file than the pull writes (#624). + */ + async opencodeInstructionsTarget( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + ): Promise<{ configFile: string; glob: string } | null> { + if (isAgentExcluded(localConfig, 'opencode')) return null; + const paths = scopedToolPaths(teamConfig, localConfig)['opencode']; + if (!paths?.rules) return null; const baseDir = resolveBaseDir(localConfig); // Only touch opencode.json when OpenCode is actually installed for this scope. - if (!await ResourceHandler.isToolInstalled(paths.rules, baseDir)) return; + if (!await ResourceHandler.isToolInstalled(paths.rules, baseDir)) return null; // The config file mirrors the MCP scope fields: /opencode.json in // project scope, ~/.config/opencode/opencode.json in user scope. const configRel = localConfig.scope === 'project' ? paths.mcpProject : paths.mcp; - if (!configRel) return; - const configFileAbs = path.join(baseDir, configRel); - const rulesDirAbs = path.join(baseDir, paths.rules); + if (!configRel) return null; - const { reconcileOpencodeInstructions, opencodeRulesGlob } = await import('./opencode-config.js'); - const glob = opencodeRulesGlob(configFileAbs, rulesDirAbs); - try { - await reconcileOpencodeInstructions(configFileAbs, glob, present); - } catch (e) { - log.warn(`Failed to update OpenCode instructions in ${configFileAbs}: ${(e as Error).message}`); - } + const configFile = path.join(baseDir, configRel); + const { opencodeRulesGlob } = await import('./opencode-config.js'); + return { configFile, glob: opencodeRulesGlob(configFile, path.join(baseDir, paths.rules)) }; } /** @@ -443,3 +473,35 @@ export class RulesHandler extends ResourceHandler { } } } + +/** + * The bytes a team rule becomes for one tool. `.md` is copied verbatim; + * Cursor-compatible tools and Copilot read frontmatter derived from the same + * source, so their file is a render rather than a copy. + * + * This is the single spelling of that mapping: `pullItem` writes it and + * `doctor` compares the delivered file against it, so a stale render is a + * reported failure rather than a file that merely exists. + */ +function renderRuleForTool(tool: string, source: string): string { + if (usesCursorMdcRules(tool)) return teamRuleToCursorMdc(source); + if (usesCopilotInstructions(tool)) return teamRuleToCopilotInstructions(source); + return source; +} + +/** + * The text `upsertSoulRules` inlines into the teamai block of Hermes SOUL.md. + * + * Hermes reads standing instructions from one file rather than a rules + * directory, so its rules are delivered as this block's contents. `doctor` + * compares what is in the block with this, the same way it compares a rule + * file with its render. + */ +export async function hermesRulesText(rules: ResourceItem[]): Promise { + const bodies: string[] = []; + for (const rule of rules) { + const body = await readFileSafe(rule.sourcePath); + if (body && body.trim() !== '') bodies.push(body.trim()); + } + return bodies.join('\n\n'); +} diff --git a/src/resources/skills.ts b/src/resources/skills.ts index 7b40e9a3c..e2dfb68f4 100644 --- a/src/resources/skills.ts +++ b/src/resources/skills.ts @@ -1,7 +1,7 @@ import path from 'node:path'; import YAML from 'yaml'; import { isToolInstalledForConfig, ResourceHandler } from './base.js'; -import type { ResourceItem, ResourceItemStatus, TeamaiConfig, LocalConfig } from '../types.js'; +import type { ResourceItem, ResourceItemStatus, DeliveryTarget, TeamaiConfig, LocalConfig } from '../types.js'; import { getPushignorePath, isAgentExcluded, resolveToolBaseDir, scopedToolPaths } from '../types.js'; import { listDirs, pathExists, copyDir, remove, dirContentEqual, dirTeamSubsetEqual, getDirLatestMtime, readFileSafe, writeFile } from '../utils/fs.js'; import { log } from '../utils/logger.js'; @@ -52,47 +52,19 @@ export async function resolveSkillDestination( } /** - * Name used only to ask the resolver a yes/no question. It shapes the path that - * comes back, never the installed gate, so no skill by this name need exist. - */ -const INSTALL_PROBE_SKILL = '__teamai_probe__'; - -/** - * Whether skills reach `tool` at all on this machine. + * The directory `tool` receives skills into on this machine, or null when it + * cannot receive them: no skills path configured, or the tool is not installed. * - * Asks `skillTargetForTool`, which is the gate the write path itself runs: - * OpenClaw resolves through its workspace directory, Hermes through its home, - * Copilot counts itself installed once `enabledAgents` names it, and everything - * else falls back to the tool root. A probe that answered any of those - * differently is exactly how "Synced N skills" ends up true while a tool - * receives nothing (#598), which is the failure `doctor` exists to catch. + * This is the gate on its own, asked without inventing a skill name. OpenClaw + * resolves through its workspace directory, Hermes through its home, Copilot + * counts itself installed once `enabledAgents` names it, and everything else + * falls back to the tool root. A second spelling of these gates is exactly how + * "Synced N skills" ends up true while a tool receives nothing (#598). */ -export async function skillsReachTool( - tool: string, - configuredSkillsPath: string, - localConfig: LocalConfig, -): Promise { - return await skillTargetForTool(tool, configuredSkillsPath, localConfig, INSTALL_PROBE_SKILL) !== null; -} - -/** - * Where `skillName` lands for `tool` on this machine, or null when the tool - * cannot receive it: no skills path configured, or the tool is not installed. - * - * One place answers that question, so `pull` writes and `doctor` checks the very - * same paths (#598). A second copy of these gates is how "Synced 12 skills" - * ends up true for one tool and silently false for another. - * - * `sourcePath` belongs to the write path: it lets the Codex shared-directory - * reconciliation delete a duplicate it can prove is identical. Omit it to - * resolve a destination without that side effect. - */ -export async function skillTargetForTool( +export async function skillsDirForTool( tool: string, configuredSkillsPath: string | undefined, localConfig: LocalConfig, - skillName: string, - sourcePath?: string, ): Promise { if (!configuredSkillsPath) return null; @@ -102,7 +74,7 @@ export async function skillTargetForTool( log.debug('Skipping skill sync for openclaw: workspace dir not found'); return null; } - return path.join(wsDir, 'skills', skillName); + return path.join(wsDir, 'skills'); } if (tool === 'hermes') { @@ -113,7 +85,7 @@ export async function skillTargetForTool( log.debug(`Skipping skill sync for ${tool}: tool not installed`); return null; } - return path.join(getHermesHome(), 'skills', skillName); + return path.join(getHermesHome(), 'skills'); } if (!await isToolInstalledForConfig(tool, configuredSkillsPath, localConfig)) { @@ -121,8 +93,40 @@ export async function skillTargetForTool( return null; } - const baseDir = resolveToolBaseDir(tool, localConfig); - return resolveSkillDestination(tool, configuredSkillsPath, baseDir, skillName, sourcePath); + return path.join(resolveToolBaseDir(tool, localConfig), configuredSkillsPath); +} + +/** + * Where `skillName` lands for `tool` on this machine, or null when the tool + * cannot receive it. + * + * One place answers that question, so `pull` writes and `doctor` checks the very + * same paths (#598). A second copy of these gates is how "Synced 12 skills" + * ends up true for one tool and silently false for another. + * + * `sourcePath` belongs to the write path: it lets the Codex shared-directory + * reconciliation delete a duplicate it can prove is identical. Omit it to + * resolve a destination without that side effect. + */ +export async function skillTargetForTool( + tool: string, + configuredSkillsPath: string | undefined, + localConfig: LocalConfig, + skillName: string, + sourcePath?: string, +): Promise { + const skillsDir = await skillsDirForTool(tool, configuredSkillsPath, localConfig); + if (skillsDir === null || configuredSkillsPath === undefined) return null; + + // Codex alone can redirect a skill to the shared `.agents/skills` directory, + // and only for a skill that already lives there — so the destination is + // per-skill and the gate above cannot answer it. + if (tool === CODEX_TOOL) { + const baseDir = resolveToolBaseDir(tool, localConfig); + return resolveSkillDestination(tool, configuredSkillsPath, baseDir, skillName, sourcePath); + } + + return path.join(skillsDir, skillName); } /** Add fields immediately before the closing delimiter without reformatting existing YAML. */ @@ -555,15 +559,41 @@ export class SkillsHandler extends ResourceHandler { } /** - * Pull a skill from team repo to all configured AI tool directories. + * Every tool that receives `item`, and where it lands. + * + * `sourcePath` opts into the write path's Codex shared-directory + * reconciliation, which can delete a duplicate it proves identical. A reader + * omits it and gets the same destinations without the side effect. */ - async pullItem(item: ResourceItem, teamConfig: TeamaiConfig, localConfig: LocalConfig): Promise { + private async resolveTargets( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + item: ResourceItem, + sourcePath?: string, + ): Promise { + const targets: DeliveryTarget[] = []; for (const [tool, toolPath] of Object.entries(scopedToolPaths(teamConfig, localConfig))) { if (isAgentExcluded(localConfig, tool)) continue; - const dest = await skillTargetForTool(tool, toolPath.skills, localConfig, item.name, item.sourcePath); - if (!dest) continue; + const dest = await skillTargetForTool(tool, toolPath.skills, localConfig, item.name, sourcePath); + if (dest) targets.push({ tool, dest }); + } + return targets; + } + + async deliveryTargets( + teamConfig: TeamaiConfig, + localConfig: LocalConfig, + item: ResourceItem, + ): Promise { + return this.resolveTargets(teamConfig, localConfig, item); + } + /** + * Pull a skill from team repo to all configured AI tool directories. + */ + async pullItem(item: ResourceItem, teamConfig: TeamaiConfig, localConfig: LocalConfig): Promise { + for (const { tool, dest } of await this.resolveTargets(teamConfig, localConfig, item, item.sourcePath)) { try { await copyDir(item.sourcePath, dest); await ensureSkillFrontmatter(dest, item.name); diff --git a/src/types.ts b/src/types.ts index dfd128449..c37c161d8 100644 --- a/src/types.ts +++ b/src/types.ts @@ -630,6 +630,19 @@ export interface ResourceDiff { removed: ResourceItem[]; } +/** Where one item lands for one tool. See `ResourceHandler.deliveryTargets`. */ +export interface DeliveryTarget { + tool: string; + dest: string; + /** + * The exact bytes `pullItem` writes at `dest`, for a handler that renders + * its destination rather than copying a tree there. It is what tells a copy + * rendered from an older spec from the current one; absent means the handler + * cannot say, and only the destination's existence can be judged. + */ + content?: string; +} + // ─── Hook definitions (unified model, issue #19) ───────── // // A single declarative model for both built-in operational hooks (source: