Repository navigation
feat(roles): assign roles in manifest/members.yaml instead of per member (#1186) - #1191
Conversation
The PR description provides sufficient testing evidence: E2E results and a representative built real-CLI run. Extra provider/agent coverage is explicitly deferred to CI. No PR code was executed. |
Tencent#1186) Review of Tencent#1191 found two places the overlay did not reach. `refreshRoleAssignment` returned a new config, and only `pullForScope` held it. The hook, MCP and env stages that run after a scope reconcile the object `pull` itself holds, and they look up `teamEnvs` and `instructions` by that object's identity. So a reassignment reached the resource sync but not the reconcilers — which kept the old role's hooks, MCP servers and env — and the two maps were keyed on an object nothing looked up again. It now mutates the config in place and returns nothing. `init` builds its config in memory and hands it straight to that same reconciliation, before any load can overlay it. The assignment branch of the role prompt returned no role at all, so the config arrived role-less, and a role-less config matches every role-scoped entry: a member assigned `frontend` had `backend`'s hooks installed at init, and an unlisted member received role-scoped hooks instead of none. Every init path now applies the assignment after the role profile and before reconciliation. Nothing is persisted: the save keeps the member's own selection, which an assignment leaves empty. The e2e case added for init applies `be-stop` to a frontend-only member without the fix, so it fails for the reason it was written.
d12dbd9 to
c045288
Compare
Tencent#1186) Review of Tencent#1191 found two places the overlay did not reach. `refreshRoleAssignment` returned a new config, and only `pullForScope` held it. The hook, MCP and env stages that run after a scope reconcile the object `pull` itself holds, and they look up `teamEnvs` and `instructions` by that object's identity. So a reassignment reached the resource sync but not the reconcilers — which kept the old role's hooks, MCP servers and env — and the two maps were keyed on an object nothing looked up again. It now mutates the config in place and returns nothing. `init` builds its config in memory and hands it straight to that same reconciliation, before any load can overlay it. The assignment branch of the role prompt returned no role at all, so the config arrived role-less, and a role-less config matches every role-scoped entry: a member assigned `frontend` had `backend`'s hooks installed at init, and an unlisted member received role-scoped hooks instead of none. Every init path now applies the assignment after the role profile and before reconciliation. Nothing is persisted: the save keeps the member's own selection, which an assignment leaves empty. The e2e case added for init applies `be-stop` to a frontend-only member without the fix, so it fails for the reason it was written.
Both earlier findings are resolved: refresh now mutates the caller’s config, and all three init paths apply the assignment before reconciliation. The description supplies sufficient testing evidence, including E2E results and a representative built real-CLI run. Extra provider/agent coverage is explicitly deferred to CI. Only the specified diff was reviewed; no PR code was run, built or installed. |
|
@scpia Codex left a remaining finding on clearing stale roleUnresolved. Mind taking a look? |
All three earlier findings are resolved: refresh preserves config identity, all init paths apply assignments before reconciliation, and refresh clears stale The description documents sufficient testing: E2E results and a representative built real-CLI run. Extra provider/agent coverage is explicitly deferred to CI. Only the specified diff was reviewed; no PR code was run, built, or installed. |
|
@scpia Codex left a new finding on suppressing the migration notice in JSON mode, and the branch now conflicts with main. Mind taking a look? |
|
@SaulMoro i am on it :) |
|
No new findings in the specified diff. All four earlier findings are resolved: refresh preserves config identity, all init paths apply assignments before reconciliation, refresh clears stale The PR description documents sufficient testing, including E2E results and representative built real-CLI runs. Extra provider/agent coverage is explicitly deferred to CI; the older commit named in part of the record is not blocking. Only the specified diff was reviewed. No PR code was run, built, or installed. |
@jeff-r2026 a short take to help with your review and your decision on whether to merge this feature. Value. Yes. The PR makes a member's roles team data that changes through the forge's review, and teamai keeps no access list of its own. An unlisted member gets no roles instead of every namespace. Gap. #1186 sets a narrow aim: an agent can no longer change its own role "with one command or one file write". Two one-step paths still do it. I found both by reading the code at
Suggestions, smallest first.
Neither change needs a new command or a new config key. @scpia could you add suggestion 1 to this PR? It closes the first path, and it fits the PR's own rule that only absence of the file hands role selection back. Suggestion 2 can be a separate issue. The merge decision is Jeff's. |
|
@SaulMoro suggestion 1 is in at 009d7bd.
Decisions worth a look:
Suggestion 2 (whoami check on pull) left for a separate issue, as you proposed. |
|
No new findings in the specified diff. All four earlier findings are resolved: refresh preserves config identity, all init paths apply assignments before reconciliation, refresh clears stale The PR description documents sufficient testing, including E2E results and representative built real-CLI runs. Extra provider/agent coverage is explicitly deferred to CI; older commits named in part of the record are not blocking. Only the specified diff was reviewed against the trusted base review criteria. No PR code was run, built, or installed. |
|
@scpia thanks,
@jeff-r2026 suggestion 1 from my note above is now in at |
|
Shall I open a issue regarding the follow-up? |
|
@scpia if you want to start working on it right away, feel free to skip the issue and build the fix directly. If you finish before @jeff-r2026 reviews and merges this PR, you can push the commit right here; if this PR gets merged first, you can open a follow-up PR with it. |
@scpia this now conflicts with |
…ber (Tencent#1186) A member's roles were a local setting: `teamai roles set` wrote them into that member's own config, and nothing said who may hold which role. For a team that also uses the effective roles to bound its agents, that bound is one the agent can lift by the same means the member has. `manifest/members.yaml` makes the assignment team data, reviewed in the team repo like `roles.yaml` is, and the forge decides who may change it: version: 1 members: alice: [frontend] bob: [backend, ki] default: [] While the file exists, a member's effective roles are the ones assigned there. The roles are overlaid onto the config where it is loaded, so every reader — pull, push placement, the namespace filter, per-entry role scopes, the dashboard, child workspaces — sees the assignment unchanged. The overlay is runtime-only: a save by any other command writes the member's own selection back, so no stale copy of an assignment is left in the config file. Nothing falls back to an unfiltered sync any more: a member the file does not list holds `default` and receives shared resources only, and a file that cannot be read, or that names a role `roles.yaml` does not declare, leaves fewer roles rather than more. `roles set` refuses, naming the file; so does `--role` at init and the dashboard's role editor. `roles list --json` reports the effective roles and their source (`assigned`, `default`, `local`, `none`) for team hooks, which otherwise parse teamai's config file. `pull` re-resolves the assignment after it refreshes the team repo, so a reassignment takes effect on the pull that fetches it instead of the one after. This is not a boundary against a process running as the member; it makes a role change a reviewed team decision with a history.
…ncent#1186) The re-resolution of manifest/members.yaml sat inside the try that reports a failed team repo fetch, so anything it threw was reported as "Pull failed" and took the rest of the scope's pull with it. It belongs after that block: the refresh runs only once the fetch succeeded, and a failure of its own is not a failure to fetch.
Tencent#1186) Review of Tencent#1191 found two places the overlay did not reach. `refreshRoleAssignment` returned a new config, and only `pullForScope` held it. The hook, MCP and env stages that run after a scope reconcile the object `pull` itself holds, and they look up `teamEnvs` and `instructions` by that object's identity. So a reassignment reached the resource sync but not the reconcilers — which kept the old role's hooks, MCP servers and env — and the two maps were keyed on an object nothing looked up again. It now mutates the config in place and returns nothing. `init` builds its config in memory and hands it straight to that same reconciliation, before any load can overlay it. The assignment branch of the role prompt returned no role at all, so the config arrived role-less, and a role-less config matches every role-scoped entry: a member assigned `frontend` had `backend`'s hooks installed at init, and an unlisted member received role-scoped hooks instead of none. Every init path now applies the assignment after the role profile and before reconciliation. Nothing is persisted: the save keeps the member's own selection, which an assignment leaves empty. The e2e case added for init applies `be-stop` to a frontend-only member without the fix, so it fails for the reason it was written.
A broken manifest/roles.yaml at load marks a role-less config roleUnresolved. When pull then fetches a fixed manifest and a members.yaml, refreshRoleAssignment applied the assigned role but kept the flag, so activeRoleIds still returned [] and role-scoped hooks, MCP entries and env were skipped on that pull. The refresh now drops the flag when it resolves an assignment, the state a load reaches, where the legacy migration is skipped once an assignment exists.
…encent#1186) The dry-run load previewed the legacy role migration before the JSON, so a hook parsing the output broke. Suppress the notice for --json only.
…nt#1186) The assignments were read from the clone's working tree, so one local edit, deletion or unpushed commit changed a member's roles. Read them from origin/<default> as last fetched instead: only what the forge merged counts, and a copy only the working tree has is absent. An HTTP source has no git of its own and sits inside whatever repository holds the config, so it keeps reading its materialized files. So does a repo with no fetched default branch to prefer.
009d7bd to
34e42d3
Compare
|
@SaulMoro rebased onto current main (6ebb31c), now 34e42d3. Linear: six commits, no merge commits, force-pushed with lease. Conflicts were import lines only ( On the rebased branch: |
All four earlier findings are resolved: config identity is preserved, init applies assignments before reconciliation, refresh clears The description documents sufficient testing, including representative built real-CLI runs. Extra provider/agent coverage is explicitly deferred to CI; older commits in parts of the record are not blocking. Only the specified diff and its relevant context were read. No PR code was run, built, or installed. |
…changed (Tencent#1186) A reassignment in manifest/members.yaml arrives with the fetch. A pinned workspace reads it from origin/<default>, so its checked-out revision does not move, and the fast path reported "Already synced" with the newly assigned skills undelivered until --force. A local selection change clears lastPullRev; an assignment has no command to do that. Compare the record's own delivered selection instead. A record without one, from an older CLI, reads as unchanged.
|
Codex's fast-path finding is real and fixed at 2fa1994. Confirmed: the "Already synced" gate compared revision, team ref, children fingerprint, targets and places, never the roles. In a workspace pinned to a tag, a reassignment on main arrives with the fetch while HEAD stays put, so the pull skipped delivery. Fix: the checkout record already stores Tests: an e2e case pins a workspace to a tag, assigns the member on main, pulls once; it fails on |
The four earlier non-cache findings are resolved. The earlier pinned-workspace reassignment scenario is fixed when the role list changes; the filtering-state transition above remains uncovered. The description documents sufficient testing, including E2E and built real-CLI records. Additional provider/agent coverage is explicitly deferred to CI; older recorded commits are not blocking. Only the specified diff and its relevant context were read against the trusted base criteria. No PR code was run, built, or installed. |
SaulMoro
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Checked at 2fa1994 with the built CLI against a build of the merge-base (6ebb31c, current main). Assigned roles, unlisted members on default, reading from origin/<default> instead of the clone's working tree, and leaving the overlay out of the saved config all behave as described. The tag-pinned reassignment fixed in 2fa1994 works when the role list changes. tsc, lint and the PR's own test files pass. Three things block.
[P1 blocking] The E2E check is red, and the cause is this PR. E2E (fork-safe, no credentials, shard 1/3) fails two cases in push-active-namespaces.test.ts, both sends an edit back to the project it came from under --role, and a new skill to the role (missing-projects-manifest and unknown-active-project contexts), with ENOENT on .claude/skills/a-skill/SKILL.md at line 1173. Locally the file is 91/93 on this head and 93/93 on the merge-base. The new selection comparison at src/pull.ts:1758 now treats the project switch from svc-a to svc-b as a changed selection. The pull therefore runs a full sync and correctly withdraws the svc-a copy the test then tries to edit. The new behavior is right. The fixture should make its edit before the switch and pull, keeping the assertions that the edit goes back to its original project and that the new skill follows --role.
[P1 blocking] Adding members.yaml with default: [] does not reach a tag-pinned member who had no role. This is the CI review's latest finding, reproduced with the built CLI. The workspace is pinned to a tag, the member has no local role, and the team declares no hai role. After the first pull it holds backend-only, frontend-only and shared-only. Main then adds members.yaml with default: []. A plain pull prints Already synced and keeps all three skills, while roles list --json reports "roles": [] and "source": "default". pull --force removes them. deliverySelection() serializes the same role-less selection in both states, because it does not record that an assignment applies. Rolling out members.yaml is exactly when this happens, and the PR's point is that the member cannot keep what the team withholds. One fix: add rolesAssigned: z.boolean().optional() to DeliverySelectionSchema, set it from !!localConfig.roleAssignment, and decide how a record without the field compares.
[P1 blocking] roles list --json prints warnings on stdout ahead of the JSON. src/roles-cmd.ts:184-194 loads the overlay before writing JSON, and the overlay's warnOnce goes through the normal logger to stdout. Both cases below exit 0 with empty stderr, and stdout does not parse:
members.yaml assigns a role roles.yaml does not declare:
⚠ manifest/members.yaml assigns unknown, which manifest/roles.yaml does not declare. Ignoring it; fix one file or the other in the team repo.
{ "roles": [], "source": "assigned", ... }
members.yaml malformed (members as a list):
⚠ manifest/members.yaml could not be read: Invalid members manifest: members: Expected object, received array
⚠ Roles are assigned in manifest/members.yaml while that file exists, so this run holds none. ...
{ "roles": [], "source": "default", ... }
A hook that runs JSON.parse on stdout fails in exactly the cases the PR says should still report the no-role state. Setting setStderrOnly(true) before autoDetectInit in JSON mode and restoring it afterwards, as other JSON commands do, keeps the warnings and leaves stdout parseable. The legacy hai migration notice is already fixed. A listed member and an unlisted member both give clean JSON.
The CI review at this head reports one [P2 non-blocking], the sync-cache finding above. It says the four earlier findings are resolved, and so is the earlier fast-path finding when the role list changes. We agree with its diagnosis and with the resolutions; we rate the cache finding blocking for the reason above. It read only the diff, so it did not catch the red E2E job or the JSON output. CI jobs at this head: E2E (fork-safe, no credentials, shard 1/3) and its aggregate E2E (fork-safe, no credentials) fail as described above; the rest pass, and the GitHub-provider job is skipped.
No other open PR implements role assignment. Not checked here: the GitLab/GitHub provider fixtures, Windows and Linux runs, and the dashboard in a browser (its API was checked).
…not (Tencent#1186) The checkout record's selection now says whether manifest/members.yaml assigned the roles: a role-less member goes from every namespace to the shared root when the file appears, with no role list changing and no revision moving in a pinned checkout. Compare only the roles part of the selection. The rest changes through commands that clear lastPullRev, and comparing it all made a config edited by hand resync where it kept the fast path before, which the push-active-namespaces e2e relies on.
|
Both addressed at 11f5574. Codex, assignment presence. Confirmed: CI, One note for the record: the fixture's withdrawn skills are kept with the existing "local changes or unpushed files (or could not be verified)" warning, the same line the description's first real-CLI run showed on a plain reassignment and on a local |
|
No new findings in the specified diff. All six earlier findings are resolved: config identity is preserved, init applies assignments before reconciliation, refresh clears The PR description documents sufficient testing, including E2E results and representative built real-CLI runs. Additional provider/agent coverage is explicitly deferred to CI; older commits in parts of the record are not blocking. Only the specified diff was read against the trusted base review criteria. No PR code was run, built, or installed. |
|
@SaulMoro one CI note on 11f5574:
Could you rerun the failed job? Nothing else is open from my side. |
@scpia Thanks. Re-checked at 11f5574. Two of the three blocking findings are fixed:
[P1 blocking] still open:
The CI review at this head reports no new findings and calls all earlier findings resolved. We agree except for the JSON output above: it read only the diff, and its "JSON mode suppresses migration notices" covers the old notice, not these warnings. |
…ent#1186) The load warns about an assigned role roles.yaml does not declare, or a members.yaml it cannot read, and a hook parses stdout whole. Route every log line to stderr for --json, as doctor --json does, and put the route back afterwards. The member guide and the admin skill now say that a team assigning roles in manifest/members.yaml refuses roles set. The e2e skill fixture gains frontmatter, so a withdrawal removes the copy instead of keeping it as unverifiable, and the cases assert the removal.
|
@SaulMoro all three at 18e8cdd.
Thanks for the rerun. |
All six earlier findings are resolved, including both sync-cache cases. The description documents sufficient testing: E2E results and representative built real-CLI runs. Additional provider/agent coverage is explicitly deferred to CI; older recorded commits are not blocking. Only the specified diff was reviewed against the trusted base criteria. No PR code was run, built, or installed. |
@scpia Thanks, checked at 18e8cdd. Nothing blocks from my side now, so I'm dismissing my Request changes. Approval and merge are @jeff-r2026's call, as this adds a new data model.
The CI review at this head raises one [P2 non-blocking]. When |
The blocking findings are resolved at 18e8cdd; see the latest comment. Approval is the maintainer's call.
|
@SaulMoro Anything open from my site? |
SaulMoro
left a comment
There was a problem hiding this comment.
@jeff-r2026 Approving and merging this one.
It is opt-in. Every new path is gated on manifest/members.yaml existing in the team repo, so a team that does not add the file sees no change in pull, push, init, roles set or the dashboard; the only addition it can notice is roles list --json. It is also a two-way door: the assignment is a runtime overlay and nothing is written to member configs, so it reverts cleanly if we decide against it. It has real value for teams that bound their agents by role, as in #1186.
The open Codex P2 (deleting members.yaml upstream delivers every role namespace for one pull when the team declares hai; the next pull corrects it) can go in a follow-up.
Closes #1186.
A member's roles were a local setting, so a team that uses the effective roles to bound its agents has a bound the agent can lift by the same means the member has. This makes the assignment team data, reviewed in the team repo, and takes the local setting out of the decision while it exists.
manifest/members.yamlThe first role listed is the primary role, the rest are additional. Who may change the file is the forge's decision, as it already is for
roles.yaml: protected branch, merge request, code owners formanifest/. teamai keeps no access list of its own.How it works
applyRoleAssignmentinconfig.ts), so pull, push placement, the namespace filter, per-entry role scopes, the dashboard and child workspaces all see the assignment with no change of their own.LocalConfig.roleAssignment);serializeLocalConfigwrites the member's own on-disk selection back, so a save byprojects setor a tag subscription never freezes a copy of an assignment into the config file.default— no roles unless the team says otherwise — and receives shared resources only, instead of the unfiltered sync a role-less member gets today. Amembers.yamlthat cannot be read, or that names a roleroles.yamldoes not declare, leaves fewer roles, never more. Only absence of the file hands role selection back. The file is read from the team repo's fetched default branch (origin/<default>), not from the clone's working tree: a local edit, deletion or unpushed commit in the clone changes nothing, and a copy only the working tree has is absent. An HTTP source, or a repo with no fetched default branch, reads its working tree as before.refreshRoleAssignmentre-resolves it on a config already in memory, mutating that config — pull keysteamEnvs/instructionson config identity and reconciles hooks, MCP and env on the object its caller holds, so a copy would reach none of them.pullcalls it after it refreshes the team repo, so a reassignment takes effect on the pull that fetches it rather than being recorded as synced and skipped until--force.initcalls it after the role profile and before its own hook, MCP and env reconciliation, which would otherwise see a role-less config and install every role's hooks. The checkout record keeps the selection it delivered, and the revision fast path compares the roles in it — which ones, and whether the team assigned them: a workspace pinned to a tag does not move when main reassigns, so the revision alone would say "Already synced" and leave the new skills undelivered until--force.roles setrefuses, naming the file and the roles held, and exits non-zero. So doesinit --roleand the dashboard's role editor (409; the page shows the roles read-only). Active projects stay each workspace's own choice.roles list --jsonreports{ roles, primaryRole, additionalRoles, source, assignmentFile },sourceone ofassigned/default/local/none, so a team hook reads the effective roles instead of parsing teamai's config file.The identity is the
usernamein the member's own config, recorded byinitfrom the git provider. The docs say plainly what this is not: a boundary against a process running as the member, which can edit that config or patch teamai. What it is: a role change that is a reviewed team decision with a history.Design note:
docs/designs/assigned-roles.md. Guide:docs/guide/distribution.md+zh-CN.Testing
Unit:
npx vitest runon the branch (18e8cdd, on top of upstream/main 6ebb31c) — 15396 passed, 67 failed. The failures aresession-scoring,workflow-scoring,workflow-handoff,session-target-id(four files; judge tests that spawn theclaudeCLI, which exits 1 in this environment) and the twosubmodule-checkoutcases; the same files fail identically on bare upstream/main 6ebb31c here, so they are environmental and unchanged by this PR.npx tsc --noEmit,npm run lint,npm run buildandnpm run test:e2e -- assigned-roles-1186(7 pass) are clean on the rebased branch.New tests:
role-assignments.test.ts(19),roles-assigned-cmd.test.ts(6), 3 cases added todashboard-distribution.test.ts,e2e/assigned-roles-1186.test.ts(4).E2E:
npm run test:e2e -- assigned-roles-1186— 4 passed:roles setis refused;initapplies only the assigned role's hooks and writes no role to the config. With the init-side resolution removed, this case applies the backend hook to a frontend-only member, so it fails for the reason it was written.Real CLI, built from d12dbd9,
gitprovider, Claude.teamai init <url> --provider git --agent claude --scope project --forceagainst a team repo assigningalice: [frontend], withhooks/frontend/hooks.yamlandhooks/backend/hooks.yamlboth declared inroles.yaml:No role prompt, the backend hook not applied, and the saved config carries no
primaryRole.Workspace scope,
members.yamlassigningalice: [frontend]while the config saysprimaryRole: backend:Then, after committing
alice: [backend]in the team repo, one plainteamai pull(no--force):The "Kept skill" line is the existing withdrawal behavior for a copy whose bytes cannot be verified: a local
teamai roles set frontend→backendchange on the same fixture, with nomembers.yamlat all, prints exactly the same line.Codex follow-up, built from 1c754f0: a legacy role-less user config,
roles.yamldeclaringhai, nomembers.yaml.Nothing but the JSON on either stream.
teamai roles listwithout--jsonstill opens withℹ [dry-run] Would migrate legacy teamai config to default role profile: hai, andconfig.yamlgains noprimaryRoleeither way.Review follow-up, built from 009d7bd:
manifest/members.yamlis read fromorigin/<default>. Fixture: a team repo assigningalice: [frontend], cloned as a workspace's team repo.Then
alice: [backend]committed in the team repo and one plainteamai pull:Team repo: 1 file(s) changed, androles list --jsonreports["backend"].Unit: 8 cases added to
role-assignments.test.tsagainst a real clone (edit, deletion, unpushed commit, working-tree-only file, fetch without checkout, empty file at the ref, HTTP source nested in a clone, repo without a remote). E2E: 1 case added toassigned-roles-1186(5 pass).Fast path, built from 2fa1994:
e2e/assigned-roles-1186gained a case that pins a workspace to a tag (pull --team-ref pin-1186, unlisted member,teamaionly delivered), commitsdave: [backend]on main, and runs one plainpull. With thesrc/pull.tschange reverted and rebuilt, that case fails onnot to contain 'Already synced'; with it, the pull does a full sync and deliversbackend-only, androles list --jsonreports["backend"]. Unit:pull-skip-sync.test.tsgained the matching case (record selection differs, HEAD unchanged, full sync).Fast path, assignment presence, built from 11f5574: the selection records
rolesAssigned, and the gate compares only the roles part (roles + that flag).e2e/assigned-roles-1186gained a case that removesmembers.yamlon main, pins a workspace to a tag (every namespace delivered), restores the file with the member unlisted, and pulls once: with the flag left out of the key it fails onnot to contain 'Already synced'; with it the pull syncs, removes both role skills androles list --jsonreportsroles: [], source: default. The fixture skills carry frontmatter, so a withdrawal removes the copy rather than keeping it as unverifiable. Unit cases inpull-skip-sync.test.ts: roles differ → sync; roles became assigned → sync; onlyprojectsdiffer in a hand-edited config → still skips. The last one is what the earlier whole-selection compare broke in CI (push-active-namespaces, two cases, now passing locally).roles list --jsonwarnings, built from 18e8cdd: the load's log lines go to stderr for--json(setStderrOnly, put back afterwards). Fixture:members.yamlassigningalice: [frontend, ops]with onlyfrontenddeclared, then a malformedmembers.yamlcommitted on main and fetched.With the malformed file: stdout
{"roles":[],"primaryRole":null,...,"source":"default"}, both ⚠ lines on stderr. Both stdouts parse. Unit case inroles-assigned-cmd.test.ts: a warning emitted during the load lands onconsole.error, stdout parses, and the route is restored after the command.Not covered locally, left to CI: the
gitlab/githubproviders and the Codex / CodeBuddy / OpenCode agents.Not included, and not needed for this:
sharing.roles.requiredinteamai.yaml(#1096 — the presence ofmembers.yamlalready decides fail-closed delivery), and a cached forge whoami as a stronger binding of the member key to the token.