Repository navigation
feat(doctor): surface toolPaths keys a team entry drops (#1115) - #1274
Conversation
A team toolPaths entry replaces that tool's default whole, so naming a tool while omitting a key the default provides silently disables the surface behind the key — hooks skip the tool, no skills arrive — and nothing surfaced the drop. One doctor note per affected tool names every dropped key with its default path and what its absence does, keeping omission a valid opt-out: a note, never a failing check, so a team that disables a surface on purpose keeps a green doctor. The guide's FAQ gains the matching one-liner: keys you omit are gone, not inherited. Verified on the real CLI: a zcode entry with skills and rules but no settings/agents/mcp produces toolPaths.zcode replaces the tool's default entry whole and drops keys the default provides: `agents` (...), `settings` (...), `mcp` (...) with each default path inline.
The PR description includes a representative real-CLI verification record, satisfying the testing requirement. No confident runtime bugs found in the scoped diff. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@kittimzhe Thanks. The zcode case works, but the note also fires for tools the team never named. We built this head and main (d60e68f5) and ran init, pull and doctor against a local team repo in an isolated HOME.
- Workspace scope, team
toolPaths.zcodewith onlyskillsandrules: this head prints the zcode note from the PR body, word for word, anddoctorkeeps its exit code.mainprints nothing. - User scope,
--agent claude,joycode, teamteamai.yamlwith notoolPathsat all: this head printstoolPaths.joycode replaces the tool's default entry whole and drops keys the default provides: `rules` (.joycode/rules — no rules are delivered to it).mainprints nothing.
[P1 blocking] src/doctor-delivery.ts:260-287 compares ctx.toolPaths with DEFAULT_TOOL_PATHS. ctx.toolPaths is the scoped result from scopedToolPaths (src/doctor.ts:443-448): defaults merged in, then userScope applied in user scope, where joycode's rules: null (src/types.ts:2747-2748) removes rules. So every user-scope member with joycode enabled, or with no enabledAgents, gets a note blaming a team entry that does not exist. The note also tells them to add back a rules path joycode does not read in user scope. That contradicts the PR's "nested userScope is skipped". The unit tests don't catch it because ctxWith passes the team's toolPaths before scoping. Reading the declared entry, ctx.teamConfig?.toolPaths[tool], and skipping tools whose entry is still the default object would fix it. A user-scope test case would cover it.
[P2 non-blocking] The new FAQ entry has no counterpart in docs/guide/zh-CN/faq.md. The doc layout rule in AGENTS.md wants both. The note is self-explanatory, so dropping the FAQ entry also works.
[P3 nit] The two as unknown as casts in the new function go away with typed keys.
#1252 adds a line to the same notes list in src/doctor.ts. Expect a trivial rebase, nothing more. This PR owns the dropped-key note.
The English FAQ entry added in this PR has no Chinese counterpart — the repo keeps behavior documentation in both languages. Mirrors the same Q before the Hooks one, matching the English placement.
|
P2 taken — the zh-CN counterpart is in (same Q, same placement before the Hooks entry). Pushed. |
|
No new findings in the scoped diff.
|
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
@kittimzhe This re-reviews cf52f755 against our comment at 515a18b3 (#1274 (review)). The Request changes stays.
Resolved
- [P2 non-blocking] zh-CN FAQ.
docs/guide/zh-CN/faq.mdnow has the entry, and it matches the English one.
Still open
- [P1 blocking] User-scope false note. This commit only changes docs, so
src/doctor-delivery.tsstill compares the scopedctx.toolPathswithDEFAULT_TOOL_PATHS. A user-scope member with joycode still gets a dropped-rulesnote for a team entry that doesn't exist. The fix suggested last time still applies: read the declared entry,ctx.teamConfig?.toolPaths[tool], and add a user-scope test case. - The branch also conflicts with main in
src/doctor.ts, after the #1221 locate changes. Please rebase onto current main.
|
@kittimzhe To get this in sooner, we pushed two commits to your branch:
With the built CLI, your zcode case still prints its note, and a user-scope member with joycode and no team |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Approving f512e85. The [P1 blocking] user-scope false note is fixed. A new unit test failed before the fix and passes after it, and with the built CLI a user-scope member with joycode and no team toolPaths gets no note, while the zcode case still prints it. The merge of main keeps both lines in src/doctor.ts. tsc, lint and the unit suite pass locally. We'll merge once CI is green.
|
No new findings in the scoped diff.
|
Resolve the doctor-delivery.ts import conflict with Tencent#1274 and update its dropped-keys fixture to the zcode default this branch sets (mcp in .zcode/cli/config.json, mcpProject in .zcode/config.json).
Fixes #1115
What
A team
toolPathsentry replaces that tool's default whole, so naming a tool while omitting a key the default provides silently disables the surface behind the key — hooks skip the tool, no skills arrive — and nothing surfaced the drop. This adds onedoctornote per affected tool naming every dropped key with its default path and what its absence does, plus the guide FAQ one-liner (keys you omit are gone, not inherited).Why a note, not a finding
Omission is a documented opt-out (a team can disable a tool's rules by leaving the path out), so the surfaced information must not fail
doctorfor teams that opt out on purpose. The note names the mechanism and the exact keys; a failing check would make the opt-out permanently red.Only keys the default table provides for that tool can be dropped — codex has no
rulesby design, so a codex entry withoutrulesproduces no note. Custom tools (no default entry) produce none. NesteduserScopeis skipped: the #938 finding already covers its observable case.Verification
Real-CLI record on the seeded fixture (
zcodeentry withskills/rulesbut nosettings/agents/mcp):Six red-first unit tests (naming + defaults + consequence, non-applicable keys, kept keys, custom tools, full restatement, no team config); full suite unchanged (the eight known local-env failures aside).