fix(env,hooks,mcp,status): name the entries that are not delivered (#822) - #851
Conversation
…encent#822) env list, mcp list, hooks list, status and list <env|hooks|mcp> resolve the entry types to show what reaches this directory, but dropped the resolution notices: an entry an unknown key (a mistyped role:) or a removed key (roles: on env, projects:) takes out of the delivered set was silently missing from the list, and status counted around it. pull and doctor report these; now the list commands do too, via reportEntryResolution. env add on a variable carrying a removed per-entry key kept reporting plain 'Updated' — Tencent#833 taught it to warn for keys the schema does not know, but a removed key is in the shape on purpose (so it can be detected), so it stayed silent. Warn the same way for those.
…notice env list now reports the withheld per-entry `roles:` variable by name (Tencent#822), so the whole-output not.toContain('DEVOPS_ONLY') assertion tripped on the delivery notice itself. The variable stays out of the delivered list; match the listed form `DEVOPS_ONLY=` instead.
The PR description includes sufficient real-CLI verification. |
The review of Tencent#851 found the update-path warning told users to remove a per-entry `roles:`/`projects:` key in place, which delivers a root-scoped secret to the whole team. The remediation now reuses `moveTo`, the same remedy pull's notice names, so it points at the namespace file to move the entry into (with the manifest declaration to add when nothing declares it). `moveTo` and `TargetFiles` move from module-private to exported for this. The review also found skill-data/setup/references/manage-admin.md still said only pull and doctor report undelivered entries, while this branch made the list commands and status report them too. It now names them, as docs/usage-guide.md does.
|
Both findings addressed in [P1 blocking] The update-path warning reused the unknown-key wording ("Remove it in env/env.yaml"), which for a root-scoped variable tells the reader to drop the key in place — exactly what turns a scoped secret into a team-wide one. It now reuses the same remedy When nothing declares the id, it names the namespace file together with the declaration to add, as
[P1 blocking] Both bullets said "Pull and
Tests. Verification (on Real-CLI record ( The second block above was also run twice more, with the role declaring a different namespace ( |
The previously reported unsafe |
|
With #865 merged, this PR is the last open item of #822: every other item there is resolved. Could you add The two blocking points from the Codex review are still open after the last commit:
|
Previously reported unsafe remediation, missing status/design documentation, and notices hidden by resolution failures are resolved. The PR description includes sufficient real-CLI verification. |
All previously reported runtime and documentation findings are resolved. The PR description includes sufficient real-CLI verification. |
|
No findings. All previously reported runtime and documentation issues are resolved in the current diff. The PR description includes sufficient real-CLI verification for the runtime behavior changes. |
Fixes the two entry-delivery follow-ups noticed in #822 (the block "Follow-ups noticed, not planned here"): the list commands that never name an undelivered entry, and
env addreportingUpdatedwithout saying it does not deliver.Closes #822
Problem
env list,mcp list,hooks list,statusandlist <env|hooks|mcp> --source reporesolve the entry types to show what reaches this directory, but drop the resolution notices. An entry that an unknown key (role:forroles:) or a removed per-entry key (roles:on env,projects:everywhere) takes out of the delivered set silently vanishes from every list, andstatuscounts around it — whilepullanddoctordo report exactly these (#833). Real CLI, fixture with one deliverable variable and two undeliverable ones (roles: [legacy]and a mistypedrole: [frontend]), onmain@e0bf2e9:Two of the three declared variables are gone and nothing says why. Same for
mcp list(aprojects:server vanishes) andhooks list(a mistyped-key hook vanishes).env addhas the mirror gap on the write path: updating a variable that carriesroles:prints✔ Updated env variable: DB_URL=…with no warning, though pull will not deliver it. #833 taught the update path to warn for keys the schema does not know — but a removed key is in the shape on purpose (kept so it can be detected), so it stayed silent.Fix
env list,mcp list,hooks list, thestatuscounts and the matchinglistsections report only unknown-key and removed-key notices, including those found before a separate resolution failure. Pull continues to report every notice. Warnings land once per run (warnOnce) and in debug.log; list commands keep their dedicated failure handling without reporting it twice.env add's update path warns for removed per-entry keys, reusing the same remedypull's notice names (moveTo), so it points at the namespace file to move the entry into rather than telling the reader to drop the key in place — which for a root-scoped variable would deliver the secret to the whole team.Docs updated where the namespace section already described who reports these keys (
docs/usage-guide.md,.zh-CN.md, andskill-data/setup/references/manage-admin.md, which the same repository rule covers);skill-data/core/references/commands.mdis generated from command flags, which did not change.Tests
env-commands.test.ts:env listnames a variable an unknown key takes out (and still lists only the delivered one), names a removed-key variable, andenv addwarns while still reportingUpdated. Theenv addcases cover both remedy branches: a role that declares the namespace (names the file alone) and nothing declaring it (names the file plus the declaration to add).mcp-cmd.test.ts:mcp listnames aprojects:-scoped server and does not list it.hooks-cmd.test.ts:hooks listnames a hook an unknown key takes out (and does not list it).npx tsc --noEmitclean;npm run lint0 warnings under--deny-warnings.Real-CLI verification (
b2b3618)npm run build, thennode dist/index.jsagainst a sandboxHOMEand a local team-repo fixture whoseenv/env.yamlholdsGOOD_URLplus aroles: [legacy]variable and a mistypedrole:variable,mcp/mcp.yamla good server plus aprojects: [web]one, andhooks/hooks.yamla good hook plus a mistypedrole:one — before/after with the fixture unchanged:Before — every command silently shows only the deliverable entries (above).
After — each names what is missing, with the remedy:
The
env addrun above was repeated twice more — with the role declaring a different namespace (legacy-ns) and with noenv:declaration at all — to cover bothmoveTobranches.Review follow-ups (
b2b3618)env add's removed-key warning reused the unknown-key wording ("Remove it in env/env.yaml"), which for a root-scoped variable tells the reader to drop the key in place and delivers the secret to the whole team. It now names the namespace file, as above.moveToandTargetFilesmoved from module-private to exported so the write path resolves the same target files the resolver does.skill-data/setup/references/manage-admin.mdstill said only pull and doctor report these keys; it now names the list commands andstatus, matchingdocs/usage-guide.md.Latest review follow-up
Second review follow-up
roles:remain documented and reported by pull and doctor.teamai env addupdates a variable carrying removed per-entry keys.