feat: scope hooks, MCP servers and env variables by logical project - #700
Conversation
|
52344a0 to
bb117c2
Compare
|
Addressed all four points. Branch rebased and force-pushed ( P1 Nothing deliverable and no P1 namespace validation is unrelated. Agreed. The commit is dropped from this branch ( P1 Test Plan. Re-run on the pushed head, all green: The three Providers: stated plainly in the body now. A P2 CHANGELOG "restricts nothing". Reworded: with no |
|
|
Addressed in P1 env scoping skipped on the revision fast path. Correct, and env was the only axis with that hole: hooks and MCP reconcile outside Because that delivery now runs on every session start, P1 testing record. Re-run on The body no longer carries the earlier record with the three On providers, the body says what was and was not done rather than claiming a matrix that was not run: Claude, Codex, CodeBuddy and OpenCode through the compiled CLI against a P2 P2 PR description claims namespace path hardening. The description does not intend to claim it. The hardening was removed from this branch in the previous round and is #710; the two remaining mentions said so, but one could be read as describing this diff. Reworded at the top of the body: this PR does not touch how resource namespace strings are validated in either manifest. |
|
Adds src/membership.ts: one check covering an entry's optional roles: and projects: keys, so a delivery path cannot filter one axis and forget the other. The two axes compose as AND, which is how tools: and roles: already compose. activeProjectIds lives in projects.ts beside the manifest it reads, mirroring activeRoleIds: an absent or empty projects list collapses to null, meaning no project filter, so nothing about today's delivery changes until a maintainer adds a projects: key. No call site uses it yet.
An mcp.yaml server accepts an optional projects: list beside roles:, and reaches a directory only when that directory is bound to one of them. Both axes AND, so 'roles: [frontend] projects: [checkout]' reaches frontend members of checkout. This is what the issue's five-projects-three-servers case costs today: every member of a role starts fifteen server processes and carries fifteen tool lists in every session. desiredMcpForTarget now takes both axes as one membership value, so the filter cannot be applied on one axis and forgotten on the other. teamai doctor inherits the filter through buildDesiredMcpContext, and 'teamai mcp list' prints the restriction next to the roles one. A non-matching server is skipped silently with no change record, exactly as the roles and tools filters already do.
A hooks.yaml hook accepts an optional projects: list beside roles:, filtered in resolveTeamHooks before the security gates so the transparency print keeps listing only the hooks this member will actually run. resolveTeamHooks now takes both axes as one membership value instead of activeRoles. With that, matchesRoles and warnUnknownRoleIds have no callers left, so they are deleted rather than kept alive by their own tests; their truth table moved to membership.test.ts as the roles-axis rows beside the new projects-axis and AND rows. 'teamai hooks list' prints the projects restriction after the roles one.
env.yaml variables accept optional roles: and projects: lists, the axes env delivery had neither of. A variable lands in the member's shell profile, so an unscoped one reaches every member of the team. resolveDeliverableEnvVariables is the single filter: pullItem writes env.sh and the KEY=value backup from it, and doctor diffs env.sh against it. Without the doctor half, a project-scoped variable a pull correctly withheld would be reported as undelivered. countEnvVars stays UNFILTERED on purpose. It gates the Tencent#662 "no top-level variables: key" warning, where a count of 0 means the file may be malformed; filtering it would fire that warning at a member scoped out of every variable, on a valid file. A member scoped out of everything still gets an env.sh written — an empty one — because that is what removes the variables an earlier pull gave them. 'teamai env list' prints both restrictions. 'teamai env add' has no flags for them (env.yaml is hand-edited for scoping, as hooks.yaml and mcp.yaml are) but round-trips them when updating a variable's value. pull-skip-sync.test.ts partially mocks ../roles.js; env delivery now reads activeRoleIds through it, so the mock carries it.
…t env count
Adds src/__tests__/e2e/project-scoped-delivery.test.ts: a directory bound to
one project receives that project's MCP server, hook and env variable and not
the other's; an entry scoping both axes reaches only a member matching both;
and 'projects set' to another project REMOVES what the first delivered. Both
MCP render paths are covered — Claude's JSON in project scope, Codex's TOML in
user scope, since Codex has no project-scope MCP location.
The e2e run found a reporting bug this branch introduced: pull printed the
DECLARED count ('Synced 2 env variable(s)') while delivering one. Adds
countDeliverableEnvVars and reports 'Synced 1 of 2 env variable(s)' when the
two differ, so a member who expected a variable can see it was scoped away
rather than lost. countEnvVars stays unfiltered for the Tencent#662 probe.
The resource table moved out of the five READMEs into docs/product-overview.md (EN + zh) when Tencent#722 slimmed the README to a landing page, so its Env, Hooks and MCP rows extend their existing one-line note there rather than gaining rows. usage-guide (EN + zh) gains the projects: line in the mcp and hooks snippets, the hooks field-table row, a projects paragraph mirroring the canonical roles one, and an explicit note that the two axes AND — which readers would otherwise assume mirrors the role-project union that resource namespaces take. env.yaml's schema was documented in no language before this: its section only showed 'teamai env add'. It now carries the first env.yaml snippet, the AND rule, the removal-on-rebind behaviour, and the last-pull-wins consequence of a shell profile that holds one block pointing at one env.sh. docs/designs/multi-project-management.md listed hooks/mcp/env in neither its affected surface nor its out-of-scope section; both now name Tencent#668, and the out-of-scope note records what stays unscoped (packages, docs, culture.md).
The warning said a projects: key with no manifest 'restricts nothing — they are delivered to every member'. The real-CLI run disproved it in its own output: the pull printed that warning next to 'Synced 2 of 4 env variable(s)'. A directory's active projects come from its own config.yaml, not from the manifest, so a directory bound to billing still filters out a projects: [checkout] entry with the manifest missing. What a missing manifest actually means is that no id can be validated, and that a directory bound to no project receives every entry. Says that instead. Adds the case that disproved the claim as a test.
…y-run Four findings from the review pass. loadProjectsManifest returns null only when the file is ABSENT; it throws for bad YAML, a bad shape, a duplicate id or an unsafe namespace. The catch-to-null conflated the two, so a team whose projects.yaml fails validation was told it 'defines no projects. Define the projects there, or drop the key' — false, and it discarded the loader's own message naming the fault. Each axis now reports three outcomes: ids to check against, no manifest, or the real load error. The roles axis had the same failure handled differently twelve lines away, silently. Both axes now read alike, via a new loadRolesManifestIfPresent that mirrors loadProjectsManifest's contract: absent is a value, invalid is an error. A team with no roles.yaml is ordinary and stays silent; a broken one is named. The env unknown-id warning moved from EnvHandler.pullItem to pullForScope. --dry-run skips pullItem, so the one command a maintainer runs to check a scoping edit was the one that never warned, while hooks and MCP warned there already. countDeliverableEnvVars folded into that call site and is gone. SAFE_SEGMENT_MESSAGE was over-escaped and printed a doubled backslash. Also: the two axis loops were one shape, now one loop; MembershipScope reads as EntryScope, the mirror of Membership rather than a near-synonym; and the pull-skip-sync mock takes activeRoleIds from vi.importActual instead of restating its body.
Two claims the spec review caught, both inherited from the roles docs rather than introduced here. "`roles: []` ships to nobody" has never been the whole rule. matchesRoles returns true when the member's axis is null, and roles.test.ts asserts exactly that on main, so an entry scoped `[]` still reaches a member who has not configured that axis. The guide now says the rule holds among members who use the axis, and points at `tools: []` for reaching no one at all. The behaviour is untouched: changing it would alter shipped Tencent#563 semantics, which is a call for the maintainers rather than a detail of this issue. The missing-manifest sentence claimed the key stops restricting. It does not. A directory's active projects come from its own config.yaml, so a directory bound to billing still filters out a projects: [checkout] entry with no manifest present. What the manifest provides is id validation.
After 'teamai projects set' or a role change, env.sh keeps exporting the previous project's variables until the next pull rewrites it. doctor returned success before reading the file whenever the member was scoped out of every variable, so those secrets stayed live in every new shell behind a passing 'Env variables injected in shell profile'. The check now parses env.sh whenever it exists and reports every variable env.yaml declares but no longer delivers to this directory, beside the missing and stale ones. Nothing deliverable and no env.sh is still a pass: nothing is owed and nothing was left behind. CHANGELOG: a projects: key with no projects manifest still filters against the directory's config.yaml ids; only their validation is lost.
…fest that cannot be read `pullForScope` returns early when the team repo revision matches the last pull, and env is delivered inside the loop that return skips. Hooks and MCP reconcile outside `pullForScope`, so a scoping change reaches them on every pull; env did not. A machine upgrading from a CLI that ignored `roles:` and `projects:` on env variables kept the withheld variables exported until `--force` or a repo change. The fast path now runs the env delivery beside the env.yaml shape check it already ran, rewriting `env.sh` from the filtered set. That delivery now runs on every session start, so `injectShellProfile` leaves an unchanged shell profile alone instead of rewriting it each time. `loadRolesManifestIfPresent` and `loadProjectsManifest` read through `readFileSafe`, which folds a permission or I/O failure into "no manifest". Both read through a new `readFileIfExists`, which returns null on ENOENT alone and throws otherwise, so a manifest that cannot be read is reported rather than treated as a team without one. Rows: the fast path in pull-skip-sync and the project-scoped-delivery e2e through the compiled CLI, the read failure in roles.test and projects.test, the untouched profile in env-handler.test.
The fast-path env delivery caught every failure and logged it at debug level, after "Already synced" had already printed. That delivery is what REMOVES a variable the member is no longer scoped to, so a write that fails left the withheld variable exported with nothing on screen to say so. The catch now warns, naming the env.sh that may still be stale and the way out (`teamai pull --force`, then a new shell). Still not rethrown: the pull it runs beside has already succeeded, and taking that down would trade one silent failure for a louder one. `teamai doctor` reports the same leftover on its own. The test makes env.sh a directory so the write throws on every platform and as root, unlike a permission bit, and asserts the warning carries the path and the command.
9a78a66 to
9a7b147
Compare
|
Addressed both findings. Branch rebased onto P1 It is still not rethrown: the pull it runs beside has already succeeded, and taking that down would trade one silent failure for a louder one that hides the rest of the sync. P1 provider coverage. The The failing file is The same fixture then drove the scoping walkthrough through the compiled CLI:
Rebase. #722 slimmed all five READMEs to a landing page and moved the resource table into One unrelated unit failure, so the record stays honest. |
|
2 participants
Summary
TeamAI resolves delivery on two membership axes, roles and projects. Four resource types read both. The three whose delivery costs something on every session read neither or only one, so a project's hook, MCP server or env variable reaches every member of the role.
One check reads both axes, so a delivery path cannot filter on one and forget the other:
Matching is an intersection on both sides. A member holds several roles and a directory is bound to several projects, so neither side is a single value to compare against.
The axes AND, the way
tools:androles:already do. Aroles: [frontend] projects: [checkout]server reaches frontend members of checkout, not everyone on either. That is deliberately not the union role and project resource namespaces take, which answers the different question of which directories to sync.Nothing changes for anyone until a maintainer adds a
projects:key. A directory bound to no project has a null projects axis and keeps receiving every entry, the same fallback a member with no role already gets.Three call sites share the one check:
matchesRolesandwarnUnknownRoleIdshad no callers left afterwards, so they are deleted rather than kept alive by their own tests. Their truth table moved tomembership.test.tsbeside the new projects and AND rows.Type of Change
Three fixes ride along, described below: a swallowed manifest load error, env delivery skipped by the revision fast path, and a failure in that fast-path delivery that was logged at debug level under
Already synced. This PR does not touch how resource namespace strings are validated inmanifest/roles.yamlormanifest/projects.yaml; that hardening was removed from this branch at review request and is #710.Test Plan
npm run buildpassesnpx tsc --noEmitpassesnpx vitest runpasses — 3834 passed, 1 pre-existing flake unrelated to this branch (local-agent.test.ts, reproduced on cleanorigin/main; see Suites)npm run test:e2epasses — run against a real GitHub-hosted team repo withTEAMAI_TEST_PROVIDER=github, 197 passed / 0 failed, because thee2eCI job skips on a fork. Details and the real-CLI walkthrough in Agents and providerProviders exercised end to end through the compiled CLI:
git(local team repo) andgithub(private fixture repo, full e2e surface plus the scoping walkthrough).gitlabis not — no credentials on this machine, the same reason this repo's owngitlab-provider-live.test.tsis opt-in onGITLAB_TOKEN. The filter runs on the local clone afterpullReporeturns, so no provider or transport code is involved on any of the three paths.Before and after, same team repo, real CLI
A directory bound to project
checkout, memberprimaryRole: frontend. The team declares four MCP servers, three hooks and four env variables, scoped acrosscheckoutandbilling.Before, on
origin/mainat 2c0ae96:DEVOPS_ONLYcarriesroles: [devops]and still reaches a frontend member, because env had no role axis either.After, this branch:
fe-checkout-apicarries bothroles: [frontend]andprojects: [checkout]and arrives, since both match.Rebinding removes what the previous project delivered
teamai projects set billingthenteamai pull --force:fe-checkout-apileaves although the role still matches, which is the AND. An OR would have kept it.Upgrade with the team repo unchanged, no
--forceHooks and MCP reconcile outside
pullForScope, so the "Already synced" fast path never hides a scoping change from them. Env was delivered inside the loop that path skips, so a machine upgrading from a CLI that ignoredroles:/projects:on env kept a withheld variable exported until--forceor a repo change. The fast path now delivers env too. Real CLI, a frontend member,env.yamlwithSHARED_URLand aroles: [devops]variable:The shell profile's mtime is unchanged across that second pull:
injectShellProfilenow skips an unchanged write, since this delivery runs on every session start. The same step is asserted in theproject-scoped-deliverye2e and in apull-skip-syncunit row.That delivery can fail — a read-only
env.sh, a full disk — and it runs afterAlready syncedhas printed, so a failure caught at debug level would leave the withheld variable exported with nothing on screen. It warns instead, naming the file and the way out:It is not rethrown: the pull it runs beside has already succeeded, and taking that down would trade one silent failure for a louder one.
teamai doctorreports the same leftover independently. The unit row makesenv.sha directory, so the write throws on every platform and as root, unlike a permission bit.Restrictions are visible
Warnings, including on
--dry-rundoctor
A variable the pull correctly withheld is not reported as undelivered, because doctor applies the same filter.
Between
teamai projects set billingand the next pull,env.shstill exports checkout's variable. doctor no longer passes on "nothing owed" there:That sequence is asserted in
project-scoped-delivery.test.tsthrough the compiled CLI, and the unit rows indoctor-env-delivery.test.tscover a leftover beside delivered variables, a leftover when nothing is deliverable, and nothing deliverable with noenv.sh(still a pass).Suites
Both suites were run with
SHELLunset, because threeshell-profile.test.tsWindows rows do not stubSHELLand resolve the host's zsh profile otherwise; that is a test-hygiene fix for its own PR.The unit failure is
local-agent.test.ts > emits hint only once per sessionId, and it is not this branch. The full suite was run four times on a cleanorigin/mainworktree at9ce8a0eand failed there in two of the four runs — once on this same row, once on it plus its siblingoutputs hookSpecificOutput with choices when project is unbound. Both pass in isolation on both trees. The two rows drivereportAndSyncLocalAgentthrough a five-second fetch path and share a fixed marker file inos.tmpdir(), so a call that outlives its test is the likely mechanism; that is a hypothesis, not something I chased down. Worth its own issue.opencode-recall.test.tsfails before its one test runs: it executesnode_modules/opencode-ai/postinstall.mjs, which the installedopencode-ai@1.18.23package does not ship on this machine. That is the test environment, not this branch; the file touches nothing this PR changes.Agents and provider
Driven through the compiled CLI against a local
gitteam repo, for Claude, Codex, CodeBuddy and OpenCode. Each received the filtered set and each dropped the other project's entries on rebind. Both MCP renderers are covered: Claude's JSON in project scope, and Codex's TOML in user scope, since Codex has no project-scope MCP location.githubis exercised end to end. Thee2eCI job skips on this fork (it is gated onvars.TEAMAI_TEST_REPO_URL), so its setup was reproduced locally: a throwaway private GitHub repo seeded as a team repo with this PR'smcp.yaml,hooks.yaml,env.yamland both manifests, cloned into an isolatedHOME, and the fullvitest.e2e.config.tssurface run against it withTEAMAI_TEST_PROVIDER=github. 197 passed, 0 failed. The same fixture then drove the scoping walkthrough through the compiled CLI withprovider: github:gitlabis still not exercised: there are no GitLab credentials on this machine, and the repository's owngitlab-provider-live.test.tsis opt-in onGITLAB_TOKENfor the same reason. This change adds no provider or transport code — the filter runs on the local clone afterpullReporeturns, so every provider reaches it through the identical path, which is why thegitandgithubruns exercise the same lines. A maintainer with GitLab credentials can repeat the walkthrough above against a GitLab-hosted repo with the same seed files; say the word and I will run it against any test group you can point me at.Related Issues
Closes #668
Notes for Reviewers
Merge danger
Two-way door. Every commit is additive behind an optional key, and reverting restores the previous delivery on the next pull, since hooks, MCP and env are reconciled from scratch each time rather than migrated.
Blast radius: delivery. What could go wrong is an entry reaching fewer members than intended, which a maintainer sees the moment they add the key and which
teamai mcp list,hooks listandenv listexplain. Nothing is deleted from a team repo and no member data is rewritten.The manifest namespace hardening, the one change that could make something that parses today stop parsing, is now #710.
One question for maintainers, not settled here
roles: []has never meant "nobody" in the way the docs say.matchesRolesreturns true when the member's axis is null, androles.test.tson main asserts exactly that, so an entry scoped[]still reaches every member who has not configured that axis. The three list commands printnobodyfor such an entry, and the docs say "ships to nobody".I kept the behaviour and made the docs precise instead. Changing it is three lines, but it would alter shipped #563 semantics for role-less members, which is a maintainer decision rather than a detail of this issue. Say the word and I will flip it in a follow-up, or here.
Four things the real CLI and the review caught, not the unit tests
The first
teamai pullon this branch printedSynced 2 env variable(s)while writing one.countEnvVarsgates the #662 malformed-env.yaml probe, where a count of zero means the file may be broken, so filtering it would fire that warning at a member scoped out of every variable on a good file. It stays unfiltered, and the summary line now readsSynced 1 of 3 env variable(s)when the two differ.The no-projects-manifest warning claimed the key "restricts nothing" and that entries "are delivered to every member". The pull printed that next to
Synced 2 of 4 env variable(s), disproving it in its own output. A directory's active projects come from its ownconfig.yaml, so a directory bound tobillingstill filters out aprojects: [checkout]entry with the manifest missing. The warning now says what is true. The case that disproved it is a test.loadProjectsManifestreturns null only when the file is absent and throws for every invalid one, so catching to null told a team with a broken manifest to "define the projects there" and threw away the loader's own message. Each axis now reports three outcomes, and the roles axis no longer handles the same failure silently twelve lines away. Both loaders read throughreadFileIfExists, which returns null onENOENTalone and throws on anything else, so a manifest that exists but cannot be read is one of the reported outcomes rather than "no manifest";readFileSafefolded every error into null.The env unknown-id warning lived in
EnvHandler.pullItem, which--dry-runnever reaches. Checking a scoping edit is what a maintainer runs--dry-runfor, and hooks and MCP warned there already. It moved topullForScope.Decisions worth disagreeing with
countEnvVarsstays unfiltered while the summary line is filtered. Two counts answering two questions, which is a seam a reader can trip on. The alternative was a false #662 warning.A member scoped out of every variable still gets an
env.shwritten, an empty one. That is what removes variables an earlier pull gave them, so the write is the point rather than a wasted step.Env keeps its pre-existing behaviour where one shell profile block points at one
env.sh, so a machine that pulls in several project-scoped directories ends up with the last-pulled directory's variables in new shells. Each directory's ownenv.shis correct. Fixing the shell profile is a wider change than this issue, and the usage guide now states the consequence.teamai env addgains no--rolesor--projectsflags. Those keys are hand-edited inenv.yaml, as they are inhooks.yamlandmcp.yaml.env adddoes preserve them when updating an existing key, with a test.Hooks gain no
doctordelivery check, because there is none to extend.doctorchecks hook plumbing only. Building a hooks delivery check is its own issue.Follow-ups, not in this PR
src/status.tsparsesenv.yamlby hand in two places rather than going throughEnvHandler, so it ignores any new per-variable key, and itsstatuscounts for hooks and MCP are unfiltered in the same waypull's env count was. It only prints and counts, so nothing misdelivers, but it is a second reader that will drift again.src/env-commands.ts:36writes a Chinese warning to stderr in production code, against the rule inCLAUDE.md.