feat(doctor): run the checks after a pull, and check what actually landed - #625
Conversation
Every line a pull prints reports what it did; none reported what is on disk. That gap is the shape of Tencent#574, Tencent#525, Tencent#342 and friends: "Synced N skills" and the tool receives nothing. An explicit `teamai pull` now re-runs the doctor registry and prints only the checks that failed, with the fix each one already carries. The SessionStart hook path (`pull({ silent: true })`) and `--dry-run` run no checks at all, so session startup is unchanged. Checks now declare `source: 'local' | 'provider'`. The post-pull pass runs the local ones only: the pull just used the provider successfully, so re-probing `gh auth status` would add a subprocess to every sync and prove nothing new. `teamai doctor` still runs the full registry. For Tencent#598.
buildHookChecks skipped any tool whose settings directory was missing — the same silent skip Tencent#574 reports in pull, reproduced inside doctor. With `claude` installed and `codex` not, the report was all green while codex received nothing. A tool listed in `enabledAgents` is the user's own claim that they use it, so it now yields a failing `<tool> is installed` check with a fix that points at `teamai uninstall --agent <tool>`. Without `enabledAgents` the team's tool list is aspirational and an absent tool stays silent, so no existing install grows a new red line. For Tencent#598.
pullForScope computed the desired skill set — role namespaces union the subscribed tags, minus the exclusions — and dropped it when the run ended. The delivery check needs the same set, and re-deriving it there would put that policy in a second place that drifts on its own. The block moves to an exported, read-only resolveDesiredSkills(), called from where it stood. roleContext stays an explicit argument: pullForScope already holds one, and null means "no roles configured", not "not looked up yet". No behaviour change. For Tencent#598.
Every other check verifies plumbing — provider CLI, clone, config, hooks, env. None verified the payload, which is what Tencent#574, Tencent#525, Tencent#342 and Tencent#372 are actually about: the run reports success and the agent finds nothing. `Skills delivered to <tool>` compares the desired set (role namespaces union subscribed tags, minus exclusions) against what is on disk for each installed tool, and names the skills that are missing. It catches what a write-time gate cannot: per-tool skips, and drift after a correct pull — a directory deleted by hand, a tool reinstalled, a role changed. Destination resolution moves into skillTargetForTool(), so pull writes and doctor checks the same paths, including Codex's shared .agents/skills directory. A tool that is not installed is asked for nothing; enabledAgents covers that case with its own check. For Tencent#598.
A copy can arrive intact and still never be discovered: SKILL.md deleted, frontmatter that does not parse, or a `name` that does not match its directory (Tencent#372's class). The write succeeded, so no write-time gate has anything to report. The delivery check now separates the two causes — "not delivered" from "delivered but unreadable" — and the fix says which one `teamai pull` can repair and which one needs the team repo fixed. For Tencent#598.
Hanging "is this tool installed" off the hook registry made it invisible for exactly the tools most likely to be declared and absent: OpenCode and CodeBuddy ship skills and no hook configuration, so `buildHookChecks` returned before the question was ever asked. Found driving the real CLI: `enabledAgents: [claude, opencode]` with no OpenCode root printed nothing. The check moves to its own builder over ctx.toolPaths, and probes a resource path rather than the settings path — resources land under resolveToolBaseDir (the project root in project scope), which is the root a pull would have to write into. For Tencent#598.
Two findings from the review pass. A repo with the same skill in two active namespaces makes scanRoleAwareSkills throw. pullForScope catches it and the post-pull pass catches it, but `teamai doctor` called buildChecks unguarded: the command whose job is explaining bad state stack-traced on it. It now reports a failing "Skills to deliver can be resolved" check carrying the collision. `copilot is installed` could never fail — isToolInstalledForConfig counts Copilot as installed as soon as enabledAgents names it — so that dead check is gone. Copilot's delivery check still reports what did not arrive. Also: pull and doctor now share formatCheckResult instead of two copies of the same glyphs, the timeout message interpolates its constant, and the DesiredSkills block no longer sits between skillSafeToRemove's doc comment and its function. For Tencent#598.
Both guides, both languages, same positions: the manual-pull block, the doctor section, the exclusion and tag-subscription paragraphs, and the packages-section one-liner that enumerated what doctor checks. No README change: the `teamai doctor` row still describes it, and touching it would cost five synchronized translations. For Tencent#598.
Docs are the one payload with a single destination instead of one per tool, so the check is a tree comparison rather than a per-tool loop: every non-dot file under the team repo's `docs/` against `sharing.docs.localDir`, with the same filter the copy uses. The destination resolution moves out of DocsHandler.pullItem into resolveDocsDestination(), so pull writes and doctor checks the same directory — including the project-scope rule that a `~/` prefix means the project root, not HOME. Found while validating it: a doc deleted by hand is not restored by the next pull, because the rev fast-path skips the scope. The check is what makes that visible. For Tencent#598.
The delivery fixes said "Run `teamai pull`" — printed at the end of a `teamai pull`, and wrong besides: a scope whose team repo has not moved is skipped by the revision fast-path, so a plain pull cannot restore a resource deleted after a correct sync, which is the main case these checks exist to catch. Both fixes now say `teamai pull --force` and why. The underlying gap — that a plain pull does not heal drift — is filed as its own issue. For Tencent#598.
… said Tencent#621 landed a "Contributed learnings are published" check on the same registry this pass now runs. A pull with a stuck queue therefore said the same thing twice, and contradicted itself doing it: pullForScope warns "run `teamai doctor` for what to check", then the post-pull block answers with the check's own fix, "Run `teamai pull` to publish them" — the pull that had just run. The warning is the better of the two and has to stay: it carries the push error, which the check cannot learn without attempting a push of its own, and `doctor` is a read-only diagnostic. Rewording the fix is no good either, because in `teamai doctor` — where the queue publish runs before the revision fast-path, so a plain pull really is the retry — that advice is correct. So `Check` gains `reportedByPull`, and the post-pull pass skips a check whose topic this run reported. Evidence, not a declaration: pullForScope returns before the publish step when the team repo fails to refresh, and swallows a publish throw into a debug line. On both paths the pull says nothing about the queue, so suppressing the check unconditionally would leave a stuck queue reported by nobody. The topic is a union rather than a boolean for the same reason: the pull proves what it reported by naming it, so a second tagged check cannot be silenced by the first one's evidence. For Tencent#598.
…y does `nameList` was written for the docs check and used only there, while the delivery check — the one most likely to have a long list, since a fresh machine is missing every skill at once — joined its names unbounded. A member with forty desired skills got all forty pasted into one fix line. Both now go through the helper, which moves above its first caller. Also drops a stray blank line that a rebase left between `skillSafeToRemove`'s docstring and the function, detaching the two. For Tencent#598.
713d38b to
7cc650a
Compare
|
Rebased onto #621 put a
I also amended the commit that makes Description updated. Full test plan re-run: 248 test files and 3407 tests, e2e 34 files and 154 tests, plus the real CLI against the CI is green apart from Reviewing the docs delivery check also turned up #635, where Ready for review. |
`resolveSkillDestination` warns when a skill exists in both `.agents/skills` and `.codex/skills`, unless it can prove the two are identical. That proof needs the team copy, so the check is written as `sourcePath && ...` — and without a sourcePath the guard short-circuits into the warning instead of past it. The read-only callers are the ones that omit it. `teamai remove skills` already did on main; this branch added `buildDeliveryChecks`, so the warning now fires once per skill on every `teamai doctor` and at the end of every pull, for copies the write path silently reconciles. Omitting sourcePath now returns the shared destination before the reconciliation branch, which is what the function's own docstring already promised. The write path is untouched: with a source, an unprovable pair still warns. For Tencent#598.
… both ways Three things a review of the post-pull pass turned up. The set of what a run already said was a module-level `const` cleared at the top of `pull()`, and the topic it held was a union with one member: two pieces of machinery where one value does. `pull()` now owns the set and passes it down. It is a required parameter of `pullForScope` rather than a field on its optional `policy`, because a call site that forgot it would stop recording silently, which is the failure the mechanism exists to prevent. `PullReportedTopic` is gone and `Check.reportedByPull` is a plain string. The 5s budget wrapped `runChecks` only, while the I/O is in `buildChecks`: the delivery checks stat every desired skill for every tool as the registry is built. Both are inside it now. And the pass no longer goes quiet when it gives up — silence after spending the whole budget is the same "reported success, nothing happened" shape these checks exist to catch, so it says one line and points at `teamai doctor`. The reason stays on the debug channel. `<tool> is installed` only pushed a check when it already failed, so an installed tool had no entry at all. `doctor --json` is consumed by hooks and CI, where a missing entry cannot be told apart from one that passed, and no other check in the registry behaves that way. It now reports both ways. Docs in both languages and the CHANGELOG follow, including a note that the checks at the end of a pull cover the scope resolved from the current directory. For Tencent#598.
|
Four fixes from a review pass. The first one is the reason for this push.
Omitting The other three:
Verified again end to end: 248 test files and 3411 tests, e2e 34 files and 154 tests, and the real CLI against the I kept
|
…clone Three findings from the Codex review. A pull that found a scope's lock held by another process drops that scope from every stage that reads the shared clone, because the other process may have it on a transient branch. The post-pull checks resolve their own context from that same clone and ran anyway, so a diagnostic could report a failure about someone else's work in progress. They now stay out entirely when any scope was contended; `teamai doctor` runs them once the other process is done. `<tool> is installed` probed the tool root while skill delivery asks `skillTargetForTool`, which sends OpenClaw to its workspace directory, Hermes to its home, and Copilot through `enabledAgents`. A `~/.openclaw` with no workspace therefore passed the check while delivery skipped the tool and its delivery check vanished — "reported success, received nothing" inside the command written to catch it. The probe is now `skillsReachTool`, which asks that same resolver; a tool that configures no skills path keeps the generic one, having no such resolver to ask. `Team docs delivered` called `pathExists`, which follows symlinks and says yes to a directory, so a name occupied by something other than the document passed while the document was no more readable than a missing one. It now requires a file. Reading each one would cost more than the job needs on a bundle of hundreds of documents, so this stats rather than reads, and the guides no longer claim the docs check does everything the skills check does. For Tencent#598.
|
All three taken. Two as written, one with a smaller remedy. P1, post-pull diagnostics on a contended clone. Correct. P2, OpenClaw. Correct, and the gap is wider than the tool root. export async function skillsReachTool(tool, configuredSkillsPath, localConfig): Promise<boolean> {
return await skillTargetForTool(tool, configuredSkillsPath, localConfig, INSTALL_PROBE_SKILL) !== null;
}That covers Hermes and Copilot by construction too, which a targeted fix would have left to drift. A tool that configures no skills path has no such resolver to ask and keeps the generic probe. P3, docs readability. The gap is real and the wording was ours: the guide said the docs check "does the same" as the skills one, which distinguishes delivered from unreadable, while the code only called I did not go as far as reading each file. A bundle can hold hundreds of documents and the pass now runs inside a 5s budget on every pull, so reading them all is more than the job needs. Each fix has a test that fails without it: the OpenClaw one passes a tool root with no workspace, which the old probe accepted. Verified again: 248 test files and 3415 tests, e2e 34 files and 154 tests, and the real CLI across the On the Chinese-comment question in your notes: no new Chinese reaches production code. The repo rule is English in |
|
Findings
Testing
|
2 participants
Summary
Phases 2 and 3 of #598. An interactive
teamai pullnow ends by running thedoctorchecks and printing what failed, and the registry gained the checks that look at the payload rather than the plumbing: the skills and docs a pull reported syncing are verified to be on disk and readable.teamai pull sync() report("Synced N skills") + buildChecks() → print each failure with its fix (interactive only) + minus provider checks, and minus what the pull already said hook session-start pull({ silent: true }) unchanged, runs no checks buildChecks() + + <tool> is installed enabledAgents names it, nothing is here + + Skills delivered to <tool> per tool, per skill, SKILL.md readable + + Team docs delivered the bundle against sharing.docs.localDirThe delivery check separates three outcomes, because they need different fixes:
flowchart LR D["desired set<br>role namespaces ∪ subscribed tags − excluded"] --> R{"per enabled tool:<br>resolve destination, stat, read SKILL.md"} R -->|"directory exists, frontmatter name matches"| OK["ok"] R -->|"no directory"| MISS["not delivered<br>→ teamai pull"] R -->|"frontmatter broken or name mismatched"| BAD["delivered but unreadable<br>→ fix SKILL.md in the team repo"]Type of Change
Decisions worth a reviewer's attention
Post-pull runs the local checks only.
Checkgainedsource: 'local' | 'provider', and the post-pull pass filters out the provider ones. The pull just used the provider successfully, so re-probinggh auth statuswould add a subprocess to every sync and prove nothing new.teamai doctorstill runs the full registry — verified below ongitlabandgithub.Rebased onto
origin/main, and one collision resolved. #621 landed after this branch was opened and put aContributed learnings are publishedcheck on the registry this PR now re-runs. Keeping both as-is made oneteamai pullsay this:Two lines for one fact, pointing at each other.
Checktherefore gainedreportedByPull, and the post-pull pass skips a check whose topic this run reported.The warning is the one that stays, because it carries
lastErrorfrompublishQueuedLearnings— the push error.doctorcannot learn that without attempting a push of its own, which a read-only diagnostic must not do. And #621'sfixis left untouched, because inteamai doctorit is correct: the queue publish runs before the revision fast-path, so unlike the delivery checks, a plainteamai pullreally is the retry there.Evidence, not a declaration.
pullForScopereturns before the publish step when the team repo fails to refresh, and swallows a publish throw into a debug line. On both paths the pull says nothing about the queue, so suppressing the check unconditionally would leave a stuck queue reported by nobody.pull()records the topics it actually reported and the filter reads that; the topic is a union rather than a boolean so a second tagged check cannot be silenced by the first one's evidence. Both paths are covered by tests and by a real-CLI run below.The "enabled but not installed" check is not the one-liner the issue described. #598 proposed turning the
continueinbuildHookChecksinto a failing check. Driving the real CLI showed that never fires for OpenCode or CodeBuddy: they ship skills and no hook configuration, so the function returns before the question is asked — exactly the tools most likely to be declared and absent. It is now its own builder overctx.toolPaths, probing a resource path (resources land underresolveToolBaseDir, the project root in project scope).One resolver per destination, shared with the write path.
skillTargetForTool(src/resources/skills.ts) andresolveDocsDestination(src/resources/docs.ts) are extracted frompullItemand used by both. A second copy of those gates is how "Synced 12 skills" ends up true for one tool and silently false for another.docsis included,rules/agents/mcpare not. Phase 3 asks for a check that looks at the payload rather than the plumbing. Docs are payload delivered by the same pull, through the same kind of destination resolver this PR extracts, so leaving them out would ship the idea half done while the resolver sits right there. The line is drawn at what a desired set costs to state: docs are one fixed directory, and so are skills onceresolveDesiredSkillsexists. Rules need a per-tool destination resolver (.mdcwith derived frontmatter, Copilot instructions), and agents carryspec.targetsper item plus three render formats, so their desired set is a relation rather than a product and checking them means parsing every agent YAML on every pull. That is a different design question, and it has its own issue: #624. Reviewers who read phase 3 more narrowly should say so;Team docs deliveredis one commit and comes out cleanly.Known limits, stated rather than hidden. A delivery check's
check()closes over a scan done when the registry was built, so thefixcan name the missing items; it is consistent within a run, not re-entrant. And per-tool granularity for an uninstalled tool depends onenabledAgentsbeing set: without it, the team's tool list is aspirational and an absent tool stays silent, as it always has.Four fixes from a review pass, after the rebase.
resolveSkillDestinationwarned about a Codex shared-directory conflict whenever it was called without a team copy to compare against, which is exactly what the read-only callers do:teamai remove skillsalready did onmain, and this branch addedbuildDeliveryChecks, so a plainteamai doctorwarned once per skill about copies the pull treats as identical. OmittingsourcePathnow returns the shared destination before the reconciliation branch, which is what that function's docstring already promised.The post-pull evidence moved out of module scope into
pull(), which passes it down as a required parameter ofpullForScope;PullReportedTopicis gone andCheck.reportedByPullis a plain string. The 5s budget now coversbuildChecksas well asrunChecks, because the delivery checks do their I/O while the registry is built, and the pass prints one line instead of going quiet when it gives up. And<tool> is installedreports an installed tool as passing rather than omitting it, sodoctor --jsoncarries an entry per enabled tool either way.Test Plan
npx tsc --noEmitpassesnpm run buildpassesnpx vitest runpasses — 248 files, 3411 testsnpm run test:e2epasses — 34 files, 154 tests (3 files / 26 tests skipped, network-gated)pull-post-checks.test.ts(10),doctor-delivery.test.ts(18),desired-skills.test.ts(6), plus 6 cases indoctor.test.tssourcerequired now sets it on feat(contribute): make the pending queue the write path, and surface it #621's check too, so the commits between it and the merge resolution are not left with aCheckliteral missing a required field.Real CLI end-to-end
Throwaway HOME, local git remote, project scope, four agents declared (
claude,codex,codebuddy,opencode), run against providersgit,gitlabandgithub.A hand-deleted skill is reported, with the tool and the name
A skill that landed but is invisible is reported differently, at the end of the pull
An enabled tool that is not installed
The docs bundle
A stuck queue is reported once, by the pull, with the reason — and not repeated as a check
No
Pull finished, but N check(s) failedblock.teamai doctorstill carries it, anddoctor --jsonis unchanged — neithersourcenorreportedByPullis serialized, the report is still{ name, ok, fix }.… but a pull that never reached the publish step does print it
All four agents, once their roots exist
The hook path stays silent,
--dry-runruns nothing, and the provider is never probedRelated Issues
For #598 (phases 2 and 3; phase 1 landed in #599). Follow-up for the remaining primitives: #624.
Notes for Reviewers
This is rebased on
origin/main(currently #631). The one conflict was inbuildChecks's registry array, where #621 and this branch inserted at the same point; both sides are kept, and the resulting double-report is thereportedByPulldecision above.Still based on
origin/mainrather than #597. That PR is @Morrowga's and is conflicting withmain, so I did not block on it — as discussed in the issue. It touchessrc/pull.tsandsrc/doctor.tstoo, so #597 keeps priority and I rebase this branch again after it merges.Docs are updated in both guides and both languages (
docs/usage-guide.md,docs/usage-guide.zh-CN.md) plus a CHANGELOG entry. No README change: theteamai doctorrow still describes the command, and touching it would cost five synchronized translations.