Repository navigation
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.
…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.
d12dbd9 to
c045288
Compare
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. |
…gnated-maintainers-assign-roles-ins # Conflicts: # docs/guide/distribution.md # docs/guide/zh-CN/distribution.md
|
@scpia Codex left a remaining finding on clearing stale roleUnresolved. Mind taking a look? |
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.
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 :) |
…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.
…gnated-maintainers-assign-roles-ins-2 # Conflicts: # src/pull.ts
|
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. |
…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.
|
@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. |
3 participants
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.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 run— 11477 passed, 2 failed. Both failures aresrc/__tests__/submodule-checkout.test.ts(hook.teamai-post-checkout.command), which fail the same way onupstream/main(0da2be7) in this environment, unchanged by this PR.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).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.