mirror of
https://github.com/Tencent/teamai-cli.git
synced 2026-10-02 03:14:40 +08:00
Saul MoroandSaul Moro
52525a9402
* refactor(doctor): resolve delivery destinations through one handler seam `doctor` asked "can this tool receive skills" through `skillsReachTool`, which had to invent a skill name (`__teamai_probe__`) because `skillTargetForTool` fused two questions: whether a tool receives skills at all, and where a given skill lands. Only a comment said the invented name could not affect the first. Split the gate from the path. `skillsDirForTool` answers the gate on its own — OpenClaw's workspace, Hermes' home, Copilot's enabledAgents, else the tool root — and `skillTargetForTool` is that directory plus the skill name, with Codex's shared-directory redirect on top since only that one is per-skill. Add `ResourceHandler.deliveryTargets`, the read-only seam #624 asks for: where an item lands for each tool that receives it, `null` for a resource with no per-tool file destination. `SkillsHandler` implements it, and its `pullItem` now walks the same resolved targets, so the write path and the check cannot answer differently. `buildDeliveryChecks` consumes the seam through the handler registry instead of importing `SkillsHandler` directly; check names, failure buckets and fix text are unchanged. Also points the inactive-skill cleanup at the same gate. It probed the tool root and then swept `<base>/<skills path>`, which for OpenClaw is a directory delivery never writes to — the real workspace copy was never pruned. * feat(doctor): check that rules reached each tool in its own format A rule changes both its filename and its bytes per tool: `.md` verbatim for Claude, `.mdc` with derived `globs`/`alwaysApply` for Cursor-compatible tools, `.instructions.md` with `applyTo` for Copilot. Nothing exposed where one lands, so `doctor` could not ask — the extension table lived inside `pullItem`. `RulesHandler.deliveryTargets` answers it, and `pullItem` now walks the targets it returns rather than rebuilding the gate chain, so the check and the write path resolve the same paths. `resolveDesiredRules` joins `resolveDesiredSkills` in pull.ts: the namespace convention and the tag channel are stated once, and the check reads them rather than restating them. The check reports two buckets per tool: a rule that never arrived, and one that arrived without the frontmatter its tool reads — a `.mdc` without `alwaysApply` is inert, which no write-time gate can see because the write succeeded. The fix names the destination directory, since the filename is not the rule's name. A legacy `.md` left beside a correct `.mdc` is deliberately not reported: it is inert leftover that `pullAllRules` already sweeps, not a delivery failure. * feat(doctor): check that agents reached each tool they target Agents break the items × tools shape the skills check assumes: a spec carries `targets:`, so the desired set is a relation, and each tool renders its own format, so the filename comes from the render and not from the agent's name. `AgentsHandler.deliveryTargets` is therefore the only thing that can say where an agent lands, and `pullItem` now walks the same resolution — the YAML and legacy paths merge into one loop instead of two gate chains. `resolveDesiredAgents` joins its skills and rules siblings in pull.ts, so the namespace filter and its stem-collision throw are stated once; `doctor` reports that throw as a failing check rather than stack-tracing, as it already does for skills. Two bugs surfaced while unifying the paths: - `renderedForTool` decided "legacy" from `item.legacy` alone while `pullItem` also accepted a non-`.yaml` source. An item built without the flag was therefore parsed as a spec by the cleanup and copied verbatim by the pull. `isLegacyAgent` now answers it in one place. - The parse-failure warning was Chinese, which the repo forbids in production code, and went through `console.warn` rather than the logger. Also adds a check for an agent that renders for no installed tool at all: the file is in the team repo, `pull` names the reason once, and nothing afterwards says it is still reaching nobody. * feat(doctor): check that MCP servers reached each tool's own config An MCP server is an entry inside a tool's native config, not a file of its own, so this check takes the shape of the hook check rather than of the delivery seam: it asks which servers the reconcile would want for a tool, then whether that tool's config carries them. The desired-set pass moves out of `reconcileMcpForConfig` into `desiredMcpForTarget`, unchanged — the `tools:` and `roles:` filters, the transport and policy gates, the `requires:` PATH check and the placeholder resolution all stay in one place, and the reconcile now calls it. A second copy of those filters is precisely how a server skipped once for an unresolved variable gets reported as delivered forever after. That skip reason is the point. A server dropped for `unresolved variable(s)` prints one line during a pull and is never mentioned again, so the member sees "MCP does not work" and goes looking at MCP. The check now names the server, the variable and `env/env.yaml` — including that its top-level key must be `variables:`, since a plain `KEY: value` mapping parses as no variables at all and silently skips every injection (#662). A server the member excluded on purpose is not reported; an unparseable tool config is, because the write path abandons the injection there too. * feat(doctor): check that env variables reach a shell, not just a marker The env check asserted that `# [teamai:env:start]` appeared somewhere in the profile. That is true of a block that cannot load and of a run that delivered nothing, so both failures passed and surfaced three layers away as MCP servers skipped for `unresolved variable(s)`, with nothing pointing back at env. It now asks the three questions the marker stands in for: - Does `env.yaml` declare anything? A file with content that parses to zero variables is the shorthand `KEY: value` form, which zod strips to an empty list — the pull then writes nothing and logs nothing (#662). - Did every declared variable reach `env.sh`? - Would the injected block load it? The block is built with the platform separator, so on Windows it carries backslashes; a POSIX shell reads an unquoted `\` as an escape, the `[ -f ... ]` test fails, `&&` short-circuits and `source` never runs, silently (#661). Whitespace in the path needs quotes for the same reason. Neither underlying bug is fixed here — #661 and #662 own those. This is the row missing from the issue's table: env had no check that looks at the payload, which is why both of them reach `All checks passed!`. The check is still emitted when there is nothing to deliver, passing, since `doctor --json` consumers cannot tell an absent entry from a passing one. * feat(doctor): let the caller pick the stage instead of flagging each check The post-pull pass re-runs the registry under a 5s all-or-nothing 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. Adding those to the pass would spend the budget on the expensive checks and lose the cheap ones — and going over means the member gets no check at all. `buildChecks(ctx, stage)` takes 'pull' or 'doctor' and does not build the two expensive registries for 'pull'. The stage is a property of the caller, not of a check, so it is an argument rather than a third optional flag on `Check` beside `source` and `reportedByPull` — which the issue flags as the point where that object stops reading. Skipping is at build time, not a filter over the result: the cost is in building the registry, so filtering afterwards would save nothing. * docs(doctor): describe the rules, agents, MCP and env delivery checks * test(doctor): cover the delivery checks through the built CLI * refactor(doctor): move the delivery checks out of the command file Review findings, all three from the repo's own standards. `doctor.ts` had grown to 959 lines, most of it domain logic: where a rule lands for Cursor, which tools an agent's spec targets, whether a shell block would load. CONTRIBUTING says commands in `src/*.ts` stay thin and the heavy lifting lives elsewhere. The checks move to `doctor-delivery.ts`, and `doctor.ts` is back to being the registry that runs them — smaller now than before this branch. The three per-tool builders repeated one shape: walk items × targets, bucket the failures by tool, remember the directory, format a check. `walkDelivery` holds that walk and takes a `classify` callback for the part that genuinely differs; `describeProblems` formats the buckets in the caller's label order, so the same broken machine reads the same way twice rather than in the order its failures happened. `envDeliveryProblems` had its own copy of the `$SHELL` → `.zshrc`/`.bashrc` choice, a second spelling of what `EnvHandler.detectShellProfile` already decides — the exact failure this branch exists to prevent, one layer down: it would check `.bashrc` while the pull wrote `.zshrc` and call a correct install broken. That method is now public and the check calls it. No check name, failure bucket or fix string changes. * refactor(doctor): drop the unused null return from deliveryTargets AGENTS.md's review rules reject unused flexibility, and this was some. The seam returned `DeliveryTarget[] | null`, where `null` meant "this resource has no per-tool file destination" and `[]` meant "no installed tool receives it here". The single caller wrote `?? []` and treated them alike, so the distinction only cost a branch nobody took. The default is `[]` now, and the comment carries the meaning the type was trying to. * refactor(doctor): drop the imports the delivery move left behind Moving the checks into `doctor-delivery.ts` left nine imports in `doctor.ts` with no remaining user: `fs`, `expandHome`, `listFilesRecursive`, `TEAMAI_ENV_END`, `getMcpSharing`, `usesCursorMdcRules`, `usesCopilotInstructions`, `splitFrontmatter` and the `ResourceItem` type. `tsc --noEmit` stays green either way because `noUnusedLocals` is off, so CI could not have caught these. They make `doctor.ts` look like it still reaches into frontmatter parsing and MCP sharing config, which is the impression the move existed to remove. * fix(doctor): report unreachable agents from the tools, not from the renders `Every team agent reaches a tool` was gated on `byTool.size > 0`, using successful deliveries as the proxy for "some tool was there to receive an agent". It is the wrong proxy for exactly the case it exists to catch: when every agent is malformed or targets tools that are not installed, no agent renders anywhere, `byTool` is empty, no check is built at all, and `doctor` reports success on a machine where nothing arrived. The gate is now the installed tools themselves. `AgentsHandler.agentToolDirs` answers that on its own — the tool-path, exclusion and install gates without asking any agent to render — and `resolveRenders` and the inactive-agent cleanup, which both carried their own copy of that loop, now go through it. * fix(doctor): compare MCP entries with the team definition, not their names The check asked whether the desired server name was a key in the tool's config. Reconciliation never overwrites an entry teamai does not own, so the one case the write path deliberately skips — a server of your own under a team name — satisfied the check: the key is there, the team's server is not, and every later pull skips it again without a word. `installedMcpEntries` replaces `installedMcpServerNames` and returns the entries in the rendered form `desiredMcpForTarget` produces, so the check compares values. Structurally, via `isDeepStrictEqual`: key order in a JSON config is not meaning, and a tool that rewrites its own file should not read as a failure. Codex stores a TOML block rather than a JSON value, so `codexBlockIn` extracts the block by the same regex `spliceCodexBlock` writes with, trimmed to the single trailing newline `renderCodexBlock` emits. A stale entry and a foreign one are reported alike, as `not the team's definition` — both mean the tool is not running what the team declared — and the fix says that a pull leaves an entry teamai does not own alone, so only `--force` replaces it. * fix(doctor): compare env.sh assignments with their values, not their keys `export KEY=` as a substring is true of the value env.yaml declares and of the one it replaced. A rotated credential that never reached `env.sh` — the pull that would rewrite it skips a scope whose team repo has not changed — passed the check while every shell and every MCP server kept exporting the old value, which is the failure this check exists to name. Each declared variable is now compared against the line `generateEnvFile` would write for it, the injection's own rendering rather than a second copy of its quoting, and a key present with a different value is reported as stale rather than as missing. Neither value is printed: these are credentials, and the key is the whole diagnosis. The e2e fixture delivered an MCP entry and an `env.sh` that were not what teamai writes; it now carries the rendered forms, and covers a foreign server under a team name and a stale `env.sh` through the built CLI. * docs(doctor): say what the delivery checks compare, not just that they check The MCP and env paragraphs described a name lookup and a key lookup. Both now compare values, and the MCP one reports a server of your own holding a team name — which only `teamai pull --force` replaces — so the guide and the changelog have to say so. Both language versions. * feat(doctor): compare a delivered agent with its render, not its existence The check asked only whether something readable sat at the destination, which is the same class of gap the three review findings were: an agent rendered from an older spec passes while the tool runs instructions the team replaced. A plain pull syncs a scope only when its team repo changed, so the copy can sit there indefinitely. `DeliveryTarget` carries the bytes `pullItem` writes, which `resolveRenders` already had in hand and threw away at the seam, and the agents check compares them. It is the same equality the inactive-agent cleanup already uses to decide a deployed copy is the team's. Absent `content` means the handler renders nothing — a skill is a directory tree — and only existence is judged, so skills and rules are unchanged. `walkDelivery` passes the target to `classify` rather than its two fields. The fixtures delivered the literal string `rendered`, which the new comparison correctly rejects: the unit tests now deliver through the handler's own seam, and the e2e fixture carries each tool's render byte for byte. * fix(agents): leave a member's same-stem file alone beside a legacy .md Routing the legacy `.md` path through `resolveRenders` also gave it the stale-sibling sweep, which the old `pullLegacyMd` never ran. A team agent named `helper` then deleted a `helper.toml`, `helper.json` or `helper.agent.md` the member wrote, with no ownership or content check. 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 anything else on the stem is not ours. * fix(doctor): compare a delivered rule with its render, not its key names The check read the delivered file for the presence of `alwaysApply` or a nonempty `applyTo`. A `.mdc` whose `globs` no longer match the team rule's `paths:` passes that while Cursor applies it to the wrong files, and so does a body that drifted from the team `.md`. `RulesHandler.deliveryTargets` now carries the bytes `pullItem` writes, the way the agents handler does, and the check compares against them. That makes the render the single spelling of the mapping rather than a contract `doctor` restates in terms of the keys it happens to know about. * fix(doctor): keep the reason an mcp.yaml yielded no servers `parseTeamMcpServers` answers `[]` to an absent file and to one that does not parse alike. That is right for a pull, which can only skip the run, but it left `doctor` unable to tell a team with no MCP from a team whose every server reaches no tool: the desired set was empty, no per-tool check was emitted, and `doctor --json` reported ok: true. `readMcpYaml` returns the parse failure with its reason and the check reports it. `parseMcpYaml` keeps its old shape on top of it, so the pull path is unchanged. * fix(doctor): tell a parse failure from a deliberately empty env.yaml `parseEnvYaml` answers `[]` to four different files: absent, empty, `variables: []`, and the shorthand `KEY: value` mapping whose unknown top-level key zod drops (#662). The check equated zero variables with the shorthand form, so an intentional `variables: []` was reported as malformed. `readEnvYaml` returns the reason instead of the count, so the shorthand form and invalid YAML are both named while an empty configuration fails nothing. * docs(doctor): say what the rules, MCP and env checks compare after the review The guides and the changelog entry describe what each check compares, and three of them now compare something else: a delivered rule against its render rather than its frontmatter keys, an unparsable `mcp.yaml` as its own failing check, and an explicit `variables: []` as an empty configuration rather than a malformed file. The e2e suite covers all four cases through the built CLI. * fix(doctor): check the two rule destinations that are not a file per tool `deliveryTargets` covers what `pullItem` writes under `toolPath.rules`. `pullAllRules` delivers two more things it cannot see, and both fail silently: OpenCode does not auto-scan a rules directory. Every `.md` can be there byte for byte and be inert, because `opencode.json` no longer lists the glob the pull owns — and `Rules delivered to opencode` passes throughout. Hermes has no rules directory at all: its rules are the contents of a managed block in SOUL.md. A deleted or stale block is a tool reading the wrong rules with nothing on disk to show for it. Both take the shape of the hook and MCP checks — one destination, not one per tool. `opencodeInstructionsTarget` and `hermesRulesText` are the single spelling each, so the check reads the answer the pull writes rather than deriving a second one. * fix(doctor): match a multiline env value instead of calling it stale A YAML block scalar is a legal env value, and `generateEnvFile` single-quotes it into an export spanning several physical lines. The check split env.sh on newlines and compared each line with a whole generated export, so such a value could never match: a correct pull was reported as a stale value on every run. `parseEnvFile` is the generator's inverse — it reads the assignments back, including the `'\''` encoding of an embedded quote — and the check compares values rather than lines. * docs(doctor): describe the two rule activation checks and the env inverse Two checks are new and one comparison changed, so the guides and the changelog entry describing them change with it. The e2e suite covers both through the built CLI: OpenCode rules delivered byte for byte while the glob is gone, and a multiline env value that the old line scan called stale. --------- Co-authored-by: Saul Moro <saul.moro@darstelecom.es>