Skip to content

fix(env,hooks,mcp,status): name the entries that are not delivered (#822) - #851

Merged
jeff-r2026 merged 6 commits into
Tencent:mainfrom
ydflow:fix/entry-delivery-notices
Sep 29, 2026
Merged

jeff-r2026 merged 6 commits into
Tencent:mainfrom
ydflow:fix/entry-delivery-notices

Conversation

@ydflow

@ydflow ydflow commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

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 add reporting Updated without saying it does not deliver.

Closes #822

Problem

env list, mcp list, hooks list, status and list <env|hooks|mcp> --source repo resolve the entry types to show what reaches this directory, but drop the resolution notices. An entry that an unknown key (role: for roles:) or a removed per-entry key (roles: on env, projects: everywhere) takes out of the delivered set silently vanishes from every list, and status counts around it — while pull and doctor do report exactly these (#833). Real CLI, fixture with one deliverable variable and two undeliverable ones (roles: [legacy] and a mistyped role: [frontend]), on main @ e0bf2e9:

Team env variables (1):

  GOOD_URL=ht****  (root)

Two of the three declared variables are gone and nothing says why. Same for mcp list (a projects: server vanishes) and hooks list (a mistyped-key hook vanishes).

env add has the mirror gap on the write path: updating a variable that carries roles: 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, the status counts and the matching list sections 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 remedy pull'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.
⚠ env/env.yaml: variable "DB_URL" is scoped with per-entry `roles:`, which this version no longer reads, so it reaches nobody. Move it to env/legacy/env.yaml (declare env: [legacy] for role legacy in manifest/roles.yaml) and drop the key.
⚠ env/env.yaml: variable "CACHE_TTL" has unknown key `role:`, so this entry is not delivered. Correct the key or remove it.

Team env variables (1):

  GOOD_URL=ht****  (root)

Docs updated where the namespace section already described who reports these keys (docs/usage-guide.md, .zh-CN.md, and skill-data/setup/references/manage-admin.md, which the same repository rule covers); skill-data/core/references/commands.md is generated from command flags, which did not change.

Tests

  • env-commands.test.ts: env list names a variable an unknown key takes out (and still lists only the delivered one), names a removed-key variable, and env add warns while still reporting Updated. The env add cases 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 list names a projects:-scoped server and does not list it.
  • hooks-cmd.test.ts: hooks list names a hook an unknown key takes out (and does not list it).

npx tsc --noEmit clean; npm run lint 0 warnings under --deny-warnings.

Real-CLI verification (b2b3618)

npm run build, then node dist/index.js against a sandbox HOME and a local team-repo fixture whose env/env.yaml holds GOOD_URL plus a roles: [legacy] variable and a mistyped role: variable, mcp/mcp.yaml a good server plus a projects: [web] one, and hooks/hooks.yaml a good hook plus a mistyped role: 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:

$ teamai status
⚠ env/env.yaml: variable "DB_URL" is scoped with per-entry `roles:`, which this version no longer reads, so it reaches nobody. Move it to env/legacy/env.yaml (declare env: [legacy] for role legacy in manifest/roles.yaml) and drop the key.
⚠ env/env.yaml: variable "CACHE_TTL" has unknown key `role:`, so this entry is not delivered. Correct the key or remove it.
⚠ hooks/hooks.yaml: hook "scoped-hook" has unknown key `role:`, so this entry is not delivered. Correct the key or remove it.
⚠ mcp/mcp.yaml: server "scoped_server" is scoped with per-entry `projects:`, which this version no longer reads, so it reaches nobody. Move it to mcp/web/mcp.yaml (declare mcp: [web] for project web in manifest/projects.yaml) and drop the key.
  env: 1
  hooks: 1
  mcp: 1

$ teamai env add DB_URL postgres://db-v2
⚠ env/env.yaml: variable "DB_URL" is scoped with per-entry `roles:`, which this version no longer reads, so pull does not deliver it. Move it to env/legacy/env.yaml (declare env: [legacy] for role legacy in manifest/roles.yaml) and drop the key.
✔ Updated env variable: DB_URL=postgres://db-v2
ℹ Run `teamai push` to sync to team repo.

The env add run above was repeated twice more — with the role declaring a different namespace (legacy-ns) and with no env: declaration at all — to cover both moveTo branches.

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. moveTo and TargetFiles moved from module-private to exported so the write path resolves the same target files the resolver does.
  • skill-data/setup/references/manage-admin.md still said only pull and doctor report these keys; it now names the list commands and status, matching docs/usage-guide.md.

Latest review follow-up

  • Added teamai status to the affected setup skill reference and updated the multi-project design document for removed and unknown per-entry keys.
  • Keep notices discovered before a separate resolution failure visible in status, the list commands and their dedicated env/MCP/hooks variants; the existing failure remains displayed once by each command.
  • Added regression coverage for an unknown-key entry alongside a namespace conflict. Focused status/MCP tests: 16 passed; npx tsc --noEmit, npm run lint and npm run build passed. Built CLI verification showed both the notice and the conflict in status, list mcp --source repo and mcp list.

Second review follow-up

  • List and status commands now report only unknown-key and removed-key notices for entries omitted from delivery. Delivered hooks/MCP entries with deprecated roles: remain documented and reported by pull and doctor.
  • The English and Chinese usage guides, setup skill reference, and multi-project design document now describe the warning when teamai env add updates a variable carrying removed per-entry keys.
  • Focused tests: 17 status/MCP tests and 5 env/hooks notice tests passed. Typecheck, lint and build passed; the built CLI showed both an undelivered-entry notice and a separate namespace conflict in status, generic list and MCP list.

ydflow and others added 2 commits September 27, 2026 11:20
…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.
@jeff-r2026 jeff-r2026 self-assigned this Sep 27, 2026
@github-actions

Copy link
Copy Markdown
  • [P1 blocking] src/env-commands.ts:119 gives unsafe remediation for root-scoped variables. If env/env.yaml contains DB_URL with roles: [legacy], following “Remove it in env/env.yaml” makes the secret deliver to everyone. The warning must direct users to move the entry into the appropriate namespace file before dropping the key, matching reportEntryResolution.
  • [P1 blocking] docs/usage-guide.md:978 documents the new list/status warnings, but skill-data/setup/references/manage-admin.md:173 still says only pull and doctor report these keys. This violates the repository rule that behavior changes must update every affected skill-data/ reference.

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.
@ydflow

ydflow commented Sep 28, 2026

Copy link
Copy Markdown
Contributor Author

Both findings addressed in b2b3618.

[P1 blocking] src/env-commands.ts:119 unsafe remediation — fixed.

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 reportEntryResolution names, so env add and pull say the same thing:

$ teamai env add DB_URL postgres://db-v2
⚠ env/env.yaml: variable "DB_URL" is scoped with per-entry `roles:`, which this version no longer reads, so pull does not deliver it. Move it to env/legacy/env.yaml and drop the key.
✔ Updated env variable: DB_URL=new

When nothing declares the id, it names the namespace file together with the declaration to add, as moveTo does:

... so pull does not deliver it. Move it to env/legacy/env.yaml (declare env: [legacy] for role legacy in manifest/roles.yaml) and drop the key.

moveTo and TargetFiles were module-private in namespaced-entries.ts; both are exported now so the write path resolves the same target files the resolver does, rather than duplicating the wording.

[P1 blocking] skill-data/setup/references/manage-admin.md:173 stale — fixed.

Both bullets said "Pull and teamai doctor" name these keys. They now name the list commands and status too, matching docs/usage-guide.md and docs/usage-guide.zh-CN.md:

Per-entry projects: (and roles: on env) no longer works: such an entry reaches nobody. roles: on hooks and MCP still filters for one more minor release. Pull, the list commands (teamai env list, teamai mcp list, teamai hooks list, teamai list <env|hooks|mcp> --source repo) and teamai doctor name the namespace file each entry belongs in; move it there.

An env, hook or MCP entry with a key its schema does not know (a mistyped role:) also reaches nobody. Pull, the list commands and teamai doctor name the file, entry and key; correct the key or remove it.

Tests. env-commands.test.ts gains two cases: one with a role that declares the namespace (names the file alone) and one with nothing declaring it (names the file plus the declaration to add). The existing case's assertion is updated to the new wording. Both new tests assert the full message string, so the two paths cannot drift apart again.

Verification (on b2b3618): npx tsc --noEmit clean, npm run lint 0 warnings under --deny-warnings, npx vitest run env-commands 34 passed / 2 failed — those two (env add/env remove refuse a namespace file that does not parse) fail on this Windows checkout before my change too: the reader reports the absolute path and the assertion expects the repo-relative one. Full-suite failure set is byte-identical to the pre-change baseline (92 failures, all Windows-environment: path separators, HOME), confirmed by diffing the two runs.

Real-CLI record (npm run build, then node dist/index.js against a sandbox HOME and a local team-repo fixture whose env/env.yaml holds GOOD_URL, a roles: [legacy] variable and a mistyped role: one):

$ teamai env list
⚠ env/env.yaml: variable "DB_URL" is scoped with per-entry `roles:`, which this version no longer reads, so it reaches nobody. Move it to env/legacy/env.yaml and drop the key.
⚠ env/env.yaml: variable "CACHE_TTL" has unknown key `role:`, so this entry is not delivered. Correct the key or remove it.

Team env variables (1):

  GOOD_URL=ht****  (root)

$ teamai env add DB_URL postgres://db-v2
⚠ env/env.yaml: variable "DB_URL" is scoped with per-entry `roles:`, which this version no longer reads, so pull does not deliver it. Move it to env/legacy/env.yaml and drop the key.
✔ Updated env variable: DB_URL=postgres://db-v2
ℹ Run `teamai push` to sync to team repo.

The second block above was also run twice more, with the role declaring a different namespace (legacy-ns) and with no env: declaration at all, to cover both moveTo branches.

@github-actions

Copy link
Copy Markdown
  • [P1 blocking] skill-data/setup/references/manage-admin.md:173 still omits teamai status from the commands that report undelivered entries. This leaves affected skill-data/ inconsistent with runtime behavior and violates the repository rule requiring behavior changes to update every affected skill reference.
  • [P1 blocking] docs/designs/multi-project-management.md:417 still states that only pull warns for removed keys, and line 424 still names only pull and doctor for unknown keys. The new list/status behavior must also be reflected here under the rule requiring every affected design document to be updated.
  • [P2 non-blocking] src/status.ts:108 reports notices only for successful resolutions. A file containing both an unknown-key entry and a separate duplicate/conflict returns kind: 'failed' with accumulated notices, so status—and similarly the dedicated and generic list paths—still hides the unknown entry. Report resolution.notices before the dedicated failure handling without reporting the failure twice.

The previously reported unsafe env add remediation is resolved. The PR description includes sufficient real-CLI verification.

@SaulMoro

SaulMoro commented Sep 29, 2026 •

Copy link
Copy Markdown
Collaborator

With #865 merged, this PR is the last open item of #822: every other item there is resolved. Could you add Closes #822 to the PR description so the issue closes when this merges? A closing keyword only takes effect in the description, not in a comment, and I can't edit it from here.

The two blocking points from the Codex review are still open after the last commit:

  • skill-data/setup/references/manage-admin.md:173 does not list teamai status among the commands that report undelivered entries.
  • docs/designs/multi-project-management.md:417 still says only pull warns for removed keys, and line 424 names only pull and doctor for unknown keys; both should include the list commands and status.

@github-actions

Copy link
Copy Markdown
  • [P1 blocking] src/hooks-cmd.ts:147, src/mcp-cmd.ts:37, and src/status.ts:108 call reportEntryResolution, which also emits deprecated-roles notices for delivered hooks/MCP entries. However, docs/designs/multi-project-management.md:420 and both usage-guide tables still say only pull and doctor report this deprecation. This violates the repository rule requiring every affected document to match runtime behavior; document list/status warnings or restrict these calls to undelivered-entry notices.
  • [P1 blocking] src/env-commands.ts:109 adds a new warning when env add updates an entry carrying removed scoping keys, but the affected command and namespace documentation still enumerates pull, list, status, and doctor without mentioning env add (docs/usage-guide.md:977, docs/usage-guide.zh-CN.md:899, skill-data/setup/references/manage-admin.md:173). Update all affected bilingual and skill documentation as required by the repository rules.

Previously reported unsafe remediation, missing status/design documentation, and notices hidden by resolution failures are resolved. The PR description includes sufficient real-CLI verification.

@github-actions

Copy link
Copy Markdown
  • [P1 blocking] CHANGELOG.md:9 and CHANGELOG.md:12 still state that only pull/doctor report unknown keys and that removed keys warn only during pull. The PR adds warnings to list/status commands and env add, so this violates the repository rule requiring every affected document to be updated and old wording to be grepped before PR.

All previously reported runtime and documentation findings are resolved. The PR description includes sufficient real-CLI verification.

@github-actions

Copy link
Copy Markdown

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.

@jeff-r2026
jeff-r2026 merged commit 3a9a24a into Tencent:main Sep 29, 2026
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bugs and follow-ups from namespace scoping (#707)

3 participants