Repository navigation
fix(pull): do not deliver an env, hook or MCP entry with a mistyped key (#822) - #833
Merged
Merged
Conversation
SaulMoro
force-pushed
the
fix-822-namespace-followups
branch
from
September 25, 2026 17:07
b90ed73 to
92f2e05
Compare
SaulMoro
changed the title
The PR description includes sufficient real-CLI/E2E verification, so no testing-related blocking finding. Previously reported wording, doctor-title, upgrade-note, and duplication issues are resolved. |
…ey (Tencent#822) Item 1. Env, hook and MCP entry schemas are plain z.object, which strips unknown keys, so a mistyped scoping key (`role:` for `roles:`) vanished and the entry reached every member. Each reader now reports the keys an entry was written with that its schema does not know (known keys come from the schema's own shape), and keepScopedEntry does not deliver such an entry and warns once, naming the file, the entry and the key, the same path the removed `projects:` key takes. doctor's per-entry-key check is retitled to cover it. `env add`/`env remove` and `remove mcp` keep such a key when they rewrite the file; `remove mcp` edits the YAML document instead of re-serializing the parsed servers. Item 4. recall ended every result with a Chinese line; it is English now. Item 2 is not a bug: tags reaching a tagged skill in an inactive namespace is the behavior Tencent#337 added and roles-tags-pull tests. The design doc's Known gaps entry now says so. Item 3 (pull --dry-run warnings) is left to Tencent#832.
…encent#822) Updating a variable that carries an unknown key keeps the key, so the variable stays undelivered; env add now says so instead of only reporting 'Updated env variable'.
SaulMoro
force-pushed
the
fix-822-namespace-followups
branch
from
September 26, 2026 07:43
92f2e05 to
206c0a1
Compare
|
No findings.
|
…o known top-level key (Tencent#822) A hooks or MCP file with `server:` for `servers:` parsed as empty and removed every installed team server or hook for every member, silently. Such a file now fails like one that does not parse, naming the keys found and the key expected. An extra key beside a known one is still ignored.
The previously reported |
jeff-r2026
approved these changes
Sep 26, 2026
jeff-r2026
pushed a commit
that referenced
this pull request
Sep 29, 2026
) (#851) * fix(env,hooks,mcp,status): name the entries that are not delivered (#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' — #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. * test(e2e): match the delivered DEVOPS_ONLY form, not its name in the notice env list now reports the withheld per-entry `roles:` variable by name (#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. * fix(env): point env add at the namespace file, not a key drop The review of #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. * fix(entries): report notices alongside resolution failures * fix(entries): scope list warnings and document env updates * docs(entries): align changelog with list and env warnings --------- Co-authored-by: ydflow <ydflow@users.noreply.github.com> Co-authored-by: ydflow <314143294+ydflow@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A mistyped scoping key sent an env variable, hook or MCP server to every member, and a mistyped top-level key (
server:forservers:) uninstalled every team MCP server or hook for every member, silently. This fixes items 1 and 4 of #822 and the top-level-key follow-up; item 2 turns out to be intended behavior, and item 3 is #832.Not asked for in #822 but needed for it:
env add,env removeandremove mcpkeep an unknown key when they rewrite a file, so an edit does not turn a blocked entry back into one sent to everyone.remove mcpnow edits the YAML document, so it also keeps comments.env addon such a variable warns that pull does not deliver it.A hooks or MCP file whose mapping has none of its schema's top-level keys (
servers;hooks,builtin) now fails the resolution like a file that does not parse, viamissingTopLevelKeyReasoninnamespaced-entries.ts, the same checkenv.yamlhas had since #662. The existing keep-on-failure paths then hold:reconcileMcpForConfigreturnsunresolvedand changes nothing, and hooks reconcile only the built-ins. An extra top-level key beside a known one is still ignored; the schemas are not.strict().Rebased onto
origin/main(fe03787, #832). Onlydocs/designs/multi-project-management.mdconflicted: both PRs rewrote the same "Known gaps" bullet. Resolution: the mistyped-key gap is dropped (fixed here), and #832'spull --dry-runsentence describes shipped behavior rather than a gap, so it moved into the per-entry-key paragraph.src/resources/hooks.tsauto-merged; a dry run now reports the unknown-key warnings too (run below).Type of Change
Breaking only for a team that added an extra key to an entry by hand (
notes:): that entry is no longer delivered, and pull anddoctorname it. The same holds for an entry key a later release adds, on members still on this version, so every member upgrades before a team uses one (CHANGELOG, design doc).Test Plan
npx tsc --noEmitpassesnpx vitest runpasses at 9b542ac: 327 files, 5068 passed, 1 skippedNew tests, red on
origin/main(7c834ce):hooks-handler.test.ts"skips the whole file on malformed yaml" expected[]for:::not yaml:::\n - broken, which YAML parses as a mapping with nohooks:key: that expectation was this bug (read as no hooks, remove every installed one). It now expectsnull(failed), as its sibling "not YAML at all" does.Ablation: each part (the keepScopedEntry branch, passing the keys into the scope, each reader, the schema-derived key list, each writer, the recall line) was reverted alone, and at least one of the tests above failed.
Build and E2E:
npm run buildok. E2E with--retry 0: 6 files (project-scoped-delivery,e2e,opencode-recall,remove-mcp-namespaces,namespaced-entries,roles-tags-pull), 30 passed, 22 skipped (remote-only). At 9b542ac, every e2e file that writes an MCP or hooks file (copilot-mcp,codebuddy-mcp,doctor-delivery-cli,hooks-project-isolation-issue373,mcp-uninstall,namespaced-entries,project-scoped-delivery,remove-mcp-namespaces,scope-isolation-issue85): 9 files, 39 passed. The full e2e suite was not run.Before / After (real CLI, provider
giton a local bare repo, project scope, Claude):origin/main7c834ce)typo-apiwithrole: [frontend].mcp.json, no warningDB_URLwithrole: [frontend]env.sh, no warningteamai recall ...last linemcp/mcp.yamlserver:,hooks/hooks.yamlhook:(typo pushed after a good pull).mcp.jsonand team hook emptied, no warningReal CLI, top-level typo: before (206c0a1) and after (9b542ac)
Team repo:
servers: [team-api]andhooks: [team-guard], pulled once (both installed), thenserver:/hook:pushed. Same script, two builds.Real CLI, after the rebase onto #832 (206c0a1)
Team repo:
typo-api(MCP),typo-guard(hook) andDB_URL(env) each carryrole: [frontend].Real CLI, before the rebase (92f2e05)
Before, on
origin/main:pull --forceinstalledtypo-apiand exportedDB_URLwith no warning; recall ended with以上内容来自团队知识库，仅供参考。….Related Issues
Part of #822: fixes items 1 and 4, and the top-level-key follow-up (a hooks or MCP file with
server:forservers:now fails and keeps what is installed), added to this PR because it touches the same readers; item 2 is docs-only (see Notes). Item 3 (pull --dry-runhooks/MCP warnings) is #832, now on main; this branch is rebased onto it. Item 5 (pull keeps local edits) follows separately.Notes for Reviewers
e2e/roles-tags-pull.test.tstests it.docs/usage-guide.mddocuments it too. Only the design doc's "Known gaps" called it a gap; it now describes it as intended. No code change..strict()and fail the file, so they are unchanged.server:forservers:) fails the file and keeps what is installed. Not covered:env list/mcp list/hooks list/statusdo not warn about a dropped entry, as forprojects:today.version: 1, now fails and keeps what is installed where it used to remove everything. An empty file or{}is unchanged.