From 383bbc2957a916147c766e781a2b7196169e65b6 Mon Sep 17 00:00:00 2001 From: Nils Schiller Date: Fri, 16 Oct 2026 19:42:59 +0200 Subject: [PATCH 1/6] feat(roles): assign roles in manifest/members.yaml instead of per member (#1186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/designs/README.md | 1 + docs/designs/assigned-roles.md | 70 ++++++ docs/guide/distribution.md | 36 ++- docs/guide/zh-CN/distribution.md | 36 ++- skill-data/core/references/commands.md | 3 +- src/__tests__/dashboard-distribution.test.ts | 28 +++ src/__tests__/e2e/assigned-roles-1186.test.ts | 218 ++++++++++++++++ src/__tests__/role-assignments.test.ts | 232 ++++++++++++++++++ src/__tests__/roles-assigned-cmd.test.ts | 128 ++++++++++ src/config.ts | 127 +++++++++- src/dashboard/client.ts | 13 +- src/dashboard/distribution.ts | 15 ++ src/index.ts | 7 +- src/init.ts | 27 +- src/manifest-schema.ts | 15 +- src/pull.ts | 6 +- src/resource-namespaces.ts | 17 +- src/role-assignments.ts | 126 ++++++++++ src/roles-cmd.ts | 70 +++++- src/roles.ts | 12 +- src/types.ts | 15 ++ src/workspace-children.ts | 12 +- 22 files changed, 1175 insertions(+), 39 deletions(-) create mode 100644 docs/designs/assigned-roles.md create mode 100644 src/__tests__/e2e/assigned-roles-1186.test.ts create mode 100644 src/__tests__/role-assignments.test.ts create mode 100644 src/__tests__/roles-assigned-cmd.test.ts create mode 100644 src/role-assignments.ts diff --git a/docs/designs/README.md b/docs/designs/README.md index e6af06d9c..1dd891d1f 100644 --- a/docs/designs/README.md +++ b/docs/designs/README.md @@ -9,6 +9,7 @@ One document per feature design, in English. The status column says how far each | Implemented | [GitCode provider](gitcode-provider.md) | #361 | | Implemented | [Model profiles](model-profiles.md) — `teamai models` | — | | Implemented | [Team env by directory](env-by-directory.md) — each directory's shell gets the env of the scope that governs it | #1018 | +| Implemented | [Assigned roles](assigned-roles.md) — `manifest/members.yaml` decides a member's roles | #1186 | | Implemented | [Workspace children](workspace-children.md) — a workspace also delivers what the repos set up inside it select | #913 | | Implemented | [Skill serving](skill-serving.md) — built-in skill content shipped with the CLI (`teamai skill get`) | #678 | | Implemented, living | [Team secrets](team-secrets.md) — describes only what the current version does | #875, #879 | diff --git a/docs/designs/assigned-roles.md b/docs/designs/assigned-roles.md new file mode 100644 index 000000000..53492b475 --- /dev/null +++ b/docs/designs/assigned-roles.md @@ -0,0 +1,70 @@ +# Assigned roles + +> Status: Implemented — `manifest/members.yaml` decides a member's roles when it exists (#1186). + +## Problem + +A member's roles were a local setting. `teamai roles set ` checked the id against `manifest/roles.yaml` and wrote `primaryRole`/`additionalRoles` into that member's own config; nothing said who may hold which role. A member with no role received every namespace. + +That is fine while roles only pick what gets delivered. A team that also uses the effective roles as a bound for its agents — a `PreToolUse` hook that lets the agent of a `frontend` member change `frontend/` and shared files, and nothing else — gets a different property: the agent can lift its own bound by the same means the member has. It can run `teamai roles set`, write the config file, or clear the role and remove the filter altogether. Blocking the command in a hook is string matching on shell commands, and the config is a plain YAML file in the member's home that the agent's shell can write. + +## Design + +Role assignments become team data, reviewed like any other change in the team repo, and the local setting stops deciding while they exist. + +`manifest/members.yaml`, next to `roles.yaml`: + +```yaml +version: 1 +members: + alice: [frontend] + bob: [backend, ki] +default: [] # roles of a member this file does not list +``` + +The 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 for `manifest/`. teamai keeps no access list of its own — the people allowed to assign roles are the code owners of that file. + +### One overlay, every reader + +The roles are overlaid onto the member's config where it is loaded (`applyRoleAssignment` in `config.ts`, beside the legacy role migration it precedes). Every existing reader of `primaryRole`/`additionalRoles` — pull, push placement, the namespace filter, the per-entry role scope, the dashboard, child workspaces — then sees the team's assignment without knowing it was assigned. + +The overlay is runtime-only (`LocalConfig.roleAssignment`). `serializeLocalConfig` writes the member's own on-disk selection back, so a save by any other command (`projects set`, a tag subscription) never freezes a copy of an assignment into the config file. A stale copy that outlives the assignment is the thing the file exists to prevent. + +### Fail closed + +With assignments in place, nothing falls back to an unfiltered sync: + +- `activeRoleIds` returns `[]`, not `null`: a member assigned no roles matches no role-scoped entry, where `null` means "no role filter". +- `resolveOwnResourceNamespaces` keeps its three unfiltered fallbacks closed, so such a member receives the shared root only. +- A `members.yaml` that exists but cannot be read or parsed leaves this run with no roles, loudly. Only absence of the file returns role selection to the member. +- An assigned role `roles.yaml` does not declare is dropped with a warning, rather than failing every command for that member over one stale line. If that leaves no roles — including when `roles.yaml` is missing entirely — the member holds none. + +### Takes effect on the pull that fetches it + +The assignment lives in the repo `pull` itself refreshes, and the config is loaded before that refresh. Resolved only at load time, the first pull after a reassignment would deliver the previous roles and record the new revision as synced, so the fast path would skip every pull after it until `--force`. `pullForScope` therefore re-resolves the assignment (`refreshRoleAssignment`) right after the team repo moves, which also picks up an assignments file that just arrived or just went away. + +In a workspace with children (#913) every delivered member holds the workspace's assignment: a child cannot select a role, so the selection it recorded is not one to honour. + +### What the member sees + +- `teamai roles set` refuses, naming the file and the roles held, and exits non-zero. +- `teamai roles list` prints the effective roles and says where they come from. `--json` reports `{ roles, primaryRole, additionalRoles, source, assignmentFile }` with `source` one of `assigned` (listed), `default` (not listed), `local` (no such file) or `none`. A team hook reads that instead of parsing teamai's config file, whose path and shape are teamai's own. +- `teamai init` skips the role prompt and says which roles the team assigned; `--role` is refused. +- The dashboard's Distribution page shows the roles read-only; the role save and preview APIs refuse with 409. Active projects stay each workspace's own choice. + +### Identity + +The member key is the `username` in that member's teamai config, which `teamai init` records from the git provider (`gitlabWhoami`, `ghAuthWhoami`, ...). No whoami call happens per command: it would put a network round trip on every load, with nothing useful to do when it fails. + +Two member keys that differ only by case are refused when the file loads, since a forge that folds usernames would make them one person. + +An HTTP source (`init --repo `, `username: http-consumer`) has no forge identity, so such a consumer resolves as an unlisted member and holds `default`. + +## What this is not + +A boundary against a determined process running as the member. The agent runs as that member and can edit the config the username comes from, or patch teamai itself. The narrower aim: a role change is a reviewed team decision with a history, and an agent can no longer change its own role with one command or one file write. + +## Not included + +- `sharing.roles.required` in `teamai.yaml` (#1096). The presence of `members.yaml` already decides fail-closed delivery. +- A cached forge whoami check as a stronger binding of the key to the member's token. diff --git a/docs/guide/distribution.md b/docs/guide/distribution.md index 37c44d659..77c3122a3 100644 --- a/docs/guide/distribution.md +++ b/docs/guide/distribution.md @@ -177,9 +177,43 @@ teamai projects set checkout teamai pull ``` +## Assigning roles in the team repo + +By default each member picks their own role and can change it at any time. A team that also uses the effective roles as a limit — a hook that lets a `frontend` member's agent change `frontend/` and shared files only — wants that assignment to be a reviewed change in the team repo. `manifest/members.yaml` makes it one: + +```yaml +# manifest/members.yaml +version: 1 +members: + alice: [frontend] + bob: [backend, ki] +default: [] # roles of a member this file does not list +``` + +The first role listed is the member's primary role, the rest are additional. While the file exists: + +- A member's effective roles are the ones assigned here. `teamai roles set` refuses, and so does the dashboard's Distribution page, which shows the roles instead of offering them. +- A member the file does not list holds `default` — no roles unless you say otherwise — and receives shared resources only, rather than the unfiltered sync a role-less member gets without this file. +- Who may assign roles is your forge's decision, as it already is for `roles.yaml`: a protected branch, a merge request, code owners for `manifest/`. teamai keeps no access list of its own. + +The member key is the username in the member's own teamai config, which `teamai init` records from the git provider. Deleting the file hands role selection back to each member. + +A reassignment takes effect on the next pull. + +A team hook reads the effective roles without parsing any config file: + +```bash +teamai roles list --json +# {"roles":["frontend"],"primaryRole":"frontend","additionalRoles":[],"source":"assigned","assignmentFile":"manifest/members.yaml"} +``` + +`source` is `assigned` (listed in the file), `default` (not listed), `local` (no such file, so the member's own selection) or `none`. + +This is not a boundary against a determined process running as the member: the agent runs as that member and could edit the config the username comes from, or teamai itself. What it gives you is a role change that is a reviewed team decision with a history, instead of one command on a laptop. + ## Restricting skills to some members -Namespaces only filter for members who have set a role: a member with no `primaryRole` syncs everything, so `manifest/roles.yaml` alone restricts no one. `teamai init` makes new members pick a role; members from before the manifest need `teamai roles set ` once. Their next pull withdraws teamai-delivered copies outside the new namespaces (locally modified copies are kept). +Namespaces only filter for members who have set a role: a member with no `primaryRole` syncs everything, so `manifest/roles.yaml` alone restricts no one. With `manifest/members.yaml` in place that gap is closed — an unlisted member receives shared resources only. `teamai init` makes new members pick a role; members from before the manifest need `teamai roles set ` once. Their next pull withdraws teamai-delivered copies outside the new namespaces (locally modified copies are kept). To remind members who never set a role, enforce a rule that asks the agent to check `teamai roles list` and prompt them — enforced rules cannot be deleted: diff --git a/docs/guide/zh-CN/distribution.md b/docs/guide/zh-CN/distribution.md index ae17390cb..e0154b433 100644 --- a/docs/guide/zh-CN/distribution.md +++ b/docs/guide/zh-CN/distribution.md @@ -177,9 +177,43 @@ teamai projects set checkout teamai pull ``` +## 在团队仓库里分配角色 + +默认情况下，角色由成员自己设置，也可以随时改。如果团队还把生效角色当作边界——例如用 hook 让 `frontend` 成员的 agent 只能改 `frontend/` 和共享文件——那么这个分配应当是团队仓库里一次经过评审的变更。`manifest/members.yaml` 就是这个文件： + +```yaml +# manifest/members.yaml +version: 1 +members: + alice: [frontend] + bob: [backend, ki] +default: [] # 这个文件未列出的成员所持有的角色 +``` + +列在第一位的是主角色，其余是附加角色。该文件存在时： + +- 成员的生效角色就是这里分配的角色。`teamai roles set` 会被拒绝，dashboard 的 Distribution 页面也一样，只展示角色而不提供修改。 +- 文件未列出的成员持有 `default`——除非另行声明即为无角色——并且只接收共享资源，而不是没有该文件时无角色成员得到的全量同步。 +- 谁可以分配角色由代码平台决定，和 `roles.yaml` 一样：保护分支、合并请求、`manifest/` 的 code owner。teamai 自己不维护任何权限名单。 + +成员的键是该成员本地 teamai 配置里的用户名，由 `teamai init` 从 Git 平台记录。删除该文件后，角色选择重新回到每个成员手里。 + +重新分配会在下一次 pull 时生效。 + +团队 hook 读取生效角色时不需要解析任何配置文件： + +```bash +teamai roles list --json +# {"roles":["frontend"],"primaryRole":"frontend","additionalRoles":[],"source":"assigned","assignmentFile":"manifest/members.yaml"} +``` + +`source` 取值为 `assigned`（文件中列出）、`default`（未列出）、`local`（没有该文件，成员自己的选择）或 `none`。 + +这不是对以该成员身份运行的进程的硬边界：agent 以该成员身份运行，可以改写用户名所在的配置，也可以改 teamai 自身。它带来的是：角色变更成为一次有记录、经过评审的团队决定，而不是笔记本上的一条命令。 + ## 把技能限定给部分成员 -Namespace 只对设置了角色的成员过滤：`primaryRole` 未设置的成员会同步全部技能，因此只有 `manifest/roles.yaml` 并不限定任何人。`teamai init` 会让新成员选择角色；manifest 之前入队的成员需要执行一次 `teamai roles set `。之后的 pull 会收回新 namespace 之外、由 teamai 下发的副本（本地改过的保留）。 +Namespace 只对设置了角色的成员过滤：`primaryRole` 未设置的成员会同步全部技能，因此只有 `manifest/roles.yaml` 并不限定任何人。有了 `manifest/members.yaml` 这个缺口就被补上——未列出的成员只接收共享资源。`teamai init` 会让新成员选择角色；manifest 之前入队的成员需要执行一次 `teamai roles set `。之后的 pull 会收回新 namespace 之外、由 teamai 下发的副本（本地改过的保留）。 要提醒一直没设角色的成员，可以用强制规则让 agent 检查 `teamai roles list` 并催促设置——强制规则成员不可删除： diff --git a/skill-data/core/references/commands.md b/skill-data/core/references/commands.md index 4bc051658..dd695a78c 100644 --- a/skill-data/core/references/commands.md +++ b/skill-data/core/references/commands.md @@ -109,7 +109,8 @@ Generated: do not edit by hand. Regenerate with - `teamai roles` — Manage team roles and resource namespaces - `teamai roles init` — Create a roles manifest for the team repo (admin) - `teamai roles list` — List all defined roles and your current role - - `teamai roles set ` — Set your primary role (updates local config) + - `--json` — Print your effective roles and where they come from (assigned, default, local, none) + - `teamai roles set ` — Set your primary role (updates local config; refused when manifest/members.yaml assigns roles) - `--add ` — Additional roles to include - `teamai roles add ` — Add a new role to the manifest (admin) - `--namespaces ` — Comma-separated resource namespaces (e.g. common,hai) diff --git a/src/__tests__/dashboard-distribution.test.ts b/src/__tests__/dashboard-distribution.test.ts index 650a1395f..3b13a9686 100644 --- a/src/__tests__/dashboard-distribution.test.ts +++ b/src/__tests__/dashboard-distribution.test.ts @@ -212,4 +212,32 @@ describe('dashboard distribution', () => { expect((await rejection(saveRoles(config, { revision: stale, primaryRole: 'frontend' }))).status).toBe(409); expect(await fse.pathExists(path.join(homeDir, '.teamai', 'config.yaml'))).toBe(false); }); + + // The team assigns the roles (#1186): the page shows them, and a save here + // would be overlaid away on the next load. + describe('with roles assigned in the team repo', () => { + beforeEach(() => { + config.roleAssignment = { source: 'assigned', local: { additionalRoles: [] } }; + }); + + it('says the roles are assigned, so the page can show them read-only', async () => { + expect((await readDistribution(config)).rolesAssigned) + .toEqual({ file: 'manifest/members.yaml', source: 'assigned' }); + }); + + it('refuses a role save and a role preview, naming the file', async () => { + const save = await rejection(saveRoles(config, { revision: await revision(), primaryRole: 'backend' })); + expect(save.status).toBe(409); + expect(save.message).toContain('manifest/members.yaml'); + expect((await rejection(previewDistribution(config, { primaryRole: 'backend' }))).status).toBe(409); + expect(await fse.pathExists(path.join(homeDir, '.teamai', 'config.yaml'))).toBe(false); + }); + + it('still saves the active projects, which each workspace still picks', async () => { + await saveProjects(config, { revision: await revision(), projects: ['pay'] }); + + const saved = YAML.parse(await fse.readFile(path.join(homeDir, '.teamai', 'config.yaml'), 'utf8')); + expect(saved.projects).toEqual(['pay']); + }); + }); }); diff --git a/src/__tests__/e2e/assigned-roles-1186.test.ts b/src/__tests__/e2e/assigned-roles-1186.test.ts new file mode 100644 index 000000000..006ef7f68 --- /dev/null +++ b/src/__tests__/e2e/assigned-roles-1186.test.ts @@ -0,0 +1,218 @@ +import { afterAll, beforeAll, describe, expect, it } from 'vitest'; +import { execFileSync, spawn } from 'node:child_process'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const ROOT = path.resolve(__dirname, '..', '..', '..'); +const CLI = path.join(ROOT, 'dist', 'index.js'); + +interface RunResult { + code: number | null; + output: string; +} + +function runCLI(args: string[], homeDir: string, cwd: string): Promise { + return new Promise((resolve) => { + const child = spawn('node', [CLI, ...args], { + cwd, + env: { ...process.env, FORCE_COLOR: '0', HOME: homeDir }, + stdio: ['ignore', 'pipe', 'pipe'], + }); + let output = ''; + child.stdout.on('data', (data: Buffer) => { output += data.toString(); }); + child.stderr.on('data', (data: Buffer) => { output += data.toString(); }); + child.on('close', (code) => resolve({ code, output })); + }); +} + +function git(args: string[], cwd: string): void { + execFileSync('git', args, { + cwd, + stdio: 'pipe', + env: { + ...process.env, + GIT_AUTHOR_NAME: 'TeamAI CI', + GIT_AUTHOR_EMAIL: 'ci@teamai.test', + GIT_COMMITTER_NAME: 'TeamAI CI', + GIT_COMMITTER_EMAIL: 'ci@teamai.test', + }, + }); +} + +/** + * `manifest/members.yaml` assigns roles as team data (#1186): the member's own + * `primaryRole` stops deciding, `roles set` refuses, and a member the file does + * not list receives shared resources only instead of an unfiltered sync. + */ +describe('assigned roles (#1186)', () => { + let sandbox: string; + let homeDir: string; + let remoteRepo: string; + + function writeSkill(namespace: string | null, name: string): void { + const skillDir = namespace === null + ? path.join(remoteRepo, 'skills', name) + : path.join(remoteRepo, 'skills', namespace, name); + fs.mkdirSync(skillDir, { recursive: true }); + fs.writeFileSync(path.join(skillDir, 'SKILL.md'), `# ${name}\n`); + } + + /** A project checkout of the team repo whose config names `username`. */ + function project(name: string, username: string, localRole?: string): string { + const projectRoot = path.join(sandbox, name); + const localRepo = path.join(projectRoot, '.teamai', 'team-repo'); + // Built fresh so a retry of the test starts from the same state. + fs.rmSync(projectRoot, { recursive: true, force: true }); + fs.mkdirSync(path.join(projectRoot, '.claude', 'skills'), { recursive: true }); + git(['clone', '-q', remoteRepo, localRepo], sandbox); + fs.writeFileSync(path.join(projectRoot, '.teamai', 'config.yaml'), [ + 'repo:', + ` localPath: ${localRepo}`, + ` remote: ${remoteRepo}`, + `username: ${username}`, + 'updatePolicy: auto', + 'scope: project', + `projectRoot: ${projectRoot}`, + ...(localRole ? [`primaryRole: ${localRole}`] : []), + '', + ].join('\n')); + return projectRoot; + } + + function deliveredSkills(projectRoot: string): string[] { + const skillsDir = path.join(projectRoot, '.claude', 'skills'); + return fs.existsSync(skillsDir) ? fs.readdirSync(skillsDir).sort() : []; + } + + beforeAll(() => { + if (!fs.existsSync(CLI)) { + throw new Error(`CLI binary not found at ${CLI}. Run "npm run build" first.`); + } + + sandbox = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-assigned-roles-e2e-')); + homeDir = path.join(sandbox, 'home'); + remoteRepo = path.join(sandbox, 'remote'); + fs.mkdirSync(homeDir, { recursive: true }); + fs.mkdirSync(path.join(remoteRepo, 'manifest'), { recursive: true }); + + fs.writeFileSync(path.join(remoteRepo, 'teamai.yaml'), [ + 'team: assigned-roles-e2e', + `repo: ${remoteRepo}`, + 'provider: tgit', + 'toolPaths:', + ' claude:', + ' skills: .claude/skills', + ' rules: .claude/rules', + '', + ].join('\n')); + fs.writeFileSync(path.join(remoteRepo, 'manifest', 'roles.yaml'), [ + 'version: 1', + 'roles:', + ' - id: backend', + ' resources:', + ' knowledge: [backend]', + ' skills: [backend]', + ' - id: frontend', + ' resources:', + ' knowledge: [frontend]', + ' skills: [frontend]', + '', + ].join('\n')); + fs.writeFileSync(path.join(remoteRepo, 'manifest', 'members.yaml'), [ + 'version: 1', + 'members:', + ' alice: [frontend]', + 'default: []', + '', + ].join('\n')); + writeSkill('backend', 'backend-only'); + writeSkill('frontend', 'frontend-only'); + + git(['init', '-q', '-b', 'main'], remoteRepo); + git(['add', '-A'], remoteRepo); + git(['commit', '-q', '-m', 'fixture'], remoteRepo); + }); + + afterAll(() => { + if (sandbox) fs.rmSync(sandbox, { recursive: true, force: true }); + }); + + it('delivers the assigned role, not the one the member set locally, and refuses roles set', async () => { + // The config says backend; the team assigned frontend. + const projectRoot = project('alice-project', 'alice', 'backend'); + + const list = await runCLI(['roles', 'list', '--json'], homeDir, projectRoot); + expect(list.code, list.output).toBe(0); + expect(JSON.parse(list.output)).toMatchObject({ + roles: ['frontend'], + primaryRole: 'frontend', + source: 'assigned', + assignmentFile: 'manifest/members.yaml', + }); + + const set = await runCLI(['roles', 'set', 'backend'], homeDir, projectRoot); + expect(set.code).toBe(1); + expect(set.output).toContain('manifest/members.yaml'); + + const pull = await runCLI(['pull', '--force'], homeDir, projectRoot); + expect(pull.code, pull.output).toBe(0); + // `teamai` is the CLI's own discovery skill, delivered to every member. + expect(deliveredSkills(projectRoot)).toEqual(['frontend-only', 'teamai']); + + // The refused `roles set` left the config file alone, and so did the pull. + const config = fs.readFileSync(path.join(projectRoot, '.teamai', 'config.yaml'), 'utf8'); + expect(config).toContain('primaryRole: backend'); + }); + + // The assignment lives in the repo the pull itself refreshes: resolved before + // that, it would deliver the previous roles and record the new revision as + // synced, so no later pull would fix it without --force. + it('delivers a reassignment on the pull that fetches it', async () => { + const projectRoot = project('bob-project', 'bob'); + fs.writeFileSync(path.join(remoteRepo, 'manifest', 'members.yaml'), [ + 'version: 1', + 'members:', + ' alice: [frontend]', + ' bob: [frontend]', + 'default: []', + '', + ].join('\n')); + git(['commit', '-qam', 'assign bob to frontend'], remoteRepo); + const first = await runCLI(['pull', '--force'], homeDir, projectRoot); + expect(first.code, first.output).toBe(0); + expect(deliveredSkills(projectRoot)).toEqual(['frontend-only', 'teamai']); + + fs.writeFileSync(path.join(remoteRepo, 'manifest', 'members.yaml'), [ + 'version: 1', + 'members:', + ' alice: [frontend]', + ' bob: [backend]', + 'default: []', + '', + ].join('\n')); + git(['commit', '-qam', 'reassign bob to backend'], remoteRepo); + + // A plain pull, as a session start runs it: one pull, no --force. + const second = await runCLI(['pull'], homeDir, projectRoot); + expect(second.code, second.output).toBe(0); + expect(deliveredSkills(projectRoot)).toContain('backend-only'); + const list = await runCLI(['roles', 'list', '--json'], homeDir, projectRoot); + expect(JSON.parse(list.output)).toMatchObject({ roles: ['backend'] }); + }); + + it('delivers no role namespace to a member the file does not list', async () => { + const projectRoot = project('carol-project', 'carol'); + + const list = await runCLI(['roles', 'list', '--json'], homeDir, projectRoot); + expect(JSON.parse(list.output)).toMatchObject({ roles: [], primaryRole: null, source: 'default' }); + + const pull = await runCLI(['pull', '--force'], homeDir, projectRoot); + expect(pull.code, pull.output).toBe(0); + // Without the assignments file this role-less member would receive every + // namespace — both of these skills. `default: []` closes that. + expect(deliveredSkills(projectRoot)).toEqual(['teamai']); + }); +}); diff --git a/src/__tests__/role-assignments.test.ts b/src/__tests__/role-assignments.test.ts new file mode 100644 index 000000000..8df5eef8a --- /dev/null +++ b/src/__tests__/role-assignments.test.ts @@ -0,0 +1,232 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, rmSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import YAML from 'yaml'; +import { loadLocalConfig, refreshRoleAssignment, saveLocalConfig } from '../config.js'; +import { activeRoleIds } from '../roles.js'; +import { loadRoleAssignments, loadRoleAssignmentsIfPresent, resolveAssignedRoles } from '../role-assignments.js'; +import { resolveResourceNamespaces } from '../resource-namespaces.js'; +import { log } from '../utils/logger.js'; +import { resetWarnOnce } from '../utils/warn-once.js'; + +/** + * `manifest/members.yaml` assigns roles as team data (#1186): the member's own + * config stops deciding them, and holding none is one of the answers. + */ +describe('role assignments from manifest/members.yaml', () => { + const originalHome = process.env.HOME; + let home: string; + let repoDir: string; + + beforeEach(() => { + resetWarnOnce(); + home = mkdtempSync(path.join(os.tmpdir(), 'teamai-assigned-roles-')); + process.env.HOME = home; + repoDir = path.join(home, 'team-repo'); + mkdirSync(path.join(repoDir, 'manifest'), { recursive: true }); + mkdirSync(path.join(home, '.teamai'), { recursive: true }); + writeRoles('version: 1\nroles:\n - id: frontend\n resources: { knowledge: [], skills: [frontend] }\n - id: backend\n resources: { knowledge: [], skills: [backend] }\n - id: ki\n resources: { knowledge: [], skills: [ki] }\n'); + }); + + afterEach(() => { + vi.restoreAllMocks(); + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + rmSync(home, { recursive: true, force: true }); + }); + + function writeRoles(content: string): void { + writeFileSync(path.join(repoDir, 'manifest', 'roles.yaml'), content, 'utf-8'); + } + + function writeAssignments(content: string): void { + writeFileSync(path.join(repoDir, 'manifest', 'members.yaml'), content, 'utf-8'); + } + + function writeConfig(extra = ''): string { + const configPath = path.join(home, '.teamai', 'config.yaml'); + writeFileSync( + configPath, + `repo:\n localPath: ${repoDir}\n remote: https://gitlab.com/acme/team.git\nusername: alice\n${extra}`, + 'utf-8', + ); + return configPath; + } + + // ── the manifest itself ── + + it('reads the roles a member is assigned, primary first', async () => { + writeAssignments('version: 1\nmembers:\n alice: [frontend]\n bob: [backend, ki]\ndefault: []\n'); + const manifest = await loadRoleAssignments(repoDir); + expect(resolveAssignedRoles(manifest, 'bob')).toEqual({ roles: ['backend', 'ki'], source: 'assigned' }); + }); + + it('gives a member the file does not list the declared default', async () => { + writeAssignments('version: 1\nmembers:\n alice: [frontend]\ndefault: [ki]\n'); + const manifest = await loadRoleAssignments(repoDir); + expect(resolveAssignedRoles(manifest, 'carol')).toEqual({ roles: ['ki'], source: 'default' }); + }); + + // An absent `default` must mean "no roles", never "every namespace": the + // fail-closed half of the feature (#1096). + it('gives an unlisted member no roles when the file declares no default', async () => { + writeAssignments('version: 1\nmembers:\n alice: [frontend]\n'); + const manifest = await loadRoleAssignments(repoDir); + expect(resolveAssignedRoles(manifest, 'carol')).toEqual({ roles: [], source: 'default' }); + }); + + it('is absent, not invalid, when the team assigns no roles', async () => { + expect(await loadRoleAssignmentsIfPresent(repoDir)).toBeNull(); + }); + + it('refuses two member keys that differ only by case', async () => { + writeAssignments('version: 1\nmembers:\n alice: [frontend]\n Alice: [backend]\n'); + await expect(loadRoleAssignments(repoDir)).rejects.toThrow(/differ only by case/); + }); + + // ── the overlay on the member's config ── + + it('overlays the assigned roles on the config, primary first', async () => { + writeAssignments('version: 1\nmembers:\n alice: [backend, ki]\n'); + writeConfig(); + const config = await loadLocalConfig(); + expect(config?.primaryRole).toBe('backend'); + expect(config?.additionalRoles).toEqual(['ki']); + expect(config?.roleAssignment?.source).toBe('assigned'); + }); + + it('overrides the role the member set locally', async () => { + writeAssignments('version: 1\nmembers:\n alice: [frontend]\n'); + writeConfig('primaryRole: backend\nadditionalRoles: [ki]\n'); + const config = await loadLocalConfig(); + expect(activeRoleIds(config!)).toEqual(['frontend']); + }); + + // The overlay is this run's reading of the team repo. Persisting it would + // leave a copy behind that outlives the assignment — the stale local role the + // file exists to prevent. + it('keeps the local selection in the config file when another command saves', async () => { + writeAssignments('version: 1\nmembers:\n alice: [frontend]\n'); + const configPath = writeConfig('primaryRole: backend\nadditionalRoles: [ki]\n'); + const config = await loadLocalConfig(); + + await saveLocalConfig({ ...config!, subscribedTags: ['infra'] }); + + const written = YAML.parse(readFileSync(configPath, 'utf-8')); + expect(written.primaryRole).toBe('backend'); + expect(written.additionalRoles).toEqual(['ki']); + expect(written.subscribedTags).toEqual(['infra']); + expect(written.roleAssignment).toBeUndefined(); + }); + + it('drops the local role from the file when the member has none assigned', async () => { + writeAssignments('version: 1\nmembers: {}\n'); + const configPath = writeConfig(); + const config = await loadLocalConfig(); + + await saveLocalConfig(config!); + + expect(YAML.parse(readFileSync(configPath, 'utf-8')).primaryRole).toBeUndefined(); + }); + + // ── fail closed ── + + it('matches no role-scoped entry for a member with no assigned roles', async () => { + writeAssignments('version: 1\nmembers:\n bob: [backend]\ndefault: []\n'); + writeConfig(); + const config = await loadLocalConfig(); + // Role-less normally reads as "no role filter" (null). Assigned roles make + // it "holds none", which matches nothing. + expect(activeRoleIds(config!)).toEqual([]); + }); + + it('delivers shared resources only to a member with no assigned roles', async () => { + writeAssignments('version: 1\nmembers:\n bob: [backend]\ndefault: []\n'); + writeConfig(); + const config = await loadLocalConfig(); + + const resolved = await resolveResourceNamespaces(config!); + + // Not null: null is the unfiltered sync this feature exists to close. + expect(resolved).not.toBeNull(); + expect(resolved?.activeNamespaces.skills).toEqual([]); + }); + + it('holds no roles, loudly, when the assignments file cannot be parsed', async () => { + writeAssignments('version: 1\nmembers: [alice]\n'); + writeConfig('primaryRole: backend\n'); + const warn = vi.spyOn(log, 'warn').mockImplementation(() => {}); + + const config = await loadLocalConfig(); + + expect(config?.primaryRole).toBeUndefined(); + expect(activeRoleIds(config!)).toEqual([]); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('manifest/members.yaml could not be read')); + }); + + it('ignores an assigned role the roles manifest does not declare', async () => { + writeAssignments('version: 1\nmembers:\n alice: [frontend, design]\n'); + writeConfig(); + const warn = vi.spyOn(log, 'warn').mockImplementation(() => {}); + + const config = await loadLocalConfig(); + + expect(config?.primaryRole).toBe('frontend'); + expect(config?.additionalRoles).toEqual([]); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('which manifest/roles.yaml does not declare')); + }); + + it('holds no roles when there is no roles manifest to resolve them against', async () => { + rmSync(path.join(repoDir, 'manifest', 'roles.yaml')); + writeAssignments('version: 1\nmembers:\n alice: [frontend]\n'); + writeConfig(); + const warn = vi.spyOn(log, 'warn').mockImplementation(() => {}); + + const config = await loadLocalConfig(); + + expect(config?.primaryRole).toBeUndefined(); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('manifest/roles.yaml could not be read')); + }); + + // ── refreshed after the team repo moves ── + + it('resolves the assignment again after the team repo changed', async () => { + writeAssignments('version: 1\nmembers:\n alice: [frontend]\n'); + writeConfig(); + const config = await loadLocalConfig(); + expect(config?.primaryRole).toBe('frontend'); + + // What `pull` does: the config was loaded before the repo was refreshed. + writeAssignments('version: 1\nmembers:\n alice: [backend]\n'); + const refreshed = await refreshRoleAssignment(config!); + + expect(refreshed.primaryRole).toBe('backend'); + }); + + it('hands the roles back to the member when the assignments file goes away', async () => { + writeAssignments('version: 1\nmembers:\n alice: [frontend]\n'); + writeConfig('primaryRole: backend\n'); + const config = await loadLocalConfig(); + expect(config?.primaryRole).toBe('frontend'); + + rmSync(path.join(repoDir, 'manifest', 'members.yaml')); + const refreshed = await refreshRoleAssignment(config!); + + expect(refreshed.primaryRole).toBe('backend'); + expect(refreshed.roleAssignment).toBeUndefined(); + }); + + // The legacy migration writes a `hai` role into a role-less config. A member + // the team assigned nothing must not acquire one that way. + it('does not let the legacy role migration write a role the team did not assign', async () => { + writeRoles('version: 1\nroles:\n - id: hai\n resources: { knowledge: [], skills: [hai] }\n'); + writeAssignments('version: 1\nmembers: {}\ndefault: []\n'); + const configPath = writeConfig(); + + const config = await loadLocalConfig(); + + expect(config?.primaryRole).toBeUndefined(); + expect(YAML.parse(readFileSync(configPath, 'utf-8')).primaryRole).toBeUndefined(); + }); +}); diff --git a/src/__tests__/roles-assigned-cmd.test.ts b/src/__tests__/roles-assigned-cmd.test.ts new file mode 100644 index 000000000..ba5df48c3 --- /dev/null +++ b/src/__tests__/roles-assigned-cmd.test.ts @@ -0,0 +1,128 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +vi.mock('../config.js', async (importOriginal) => ({ + ...(await importOriginal()), + autoDetectInit: vi.fn(), +})); +vi.mock('../scope-selection.js', () => ({ saveScopeSelection: vi.fn() })); + +import { rolesList, rolesSet } from '../roles-cmd.js'; +import { autoDetectInit } from '../config.js'; +import { saveScopeSelection } from '../scope-selection.js'; +import { log } from '../utils/logger.js'; +import type { LocalConfig, TeamaiConfig } from '../types.js'; + +/** `teamai roles set` and `roles list` once the team assigns roles (#1186). */ +describe('roles commands with assigned roles', () => { + let repoDir: string; + let printed: string[]; + + const teamConfig = { team: 'acme', description: '', repo: 'https://gitlab.com/acme/team.git' } as TeamaiConfig; + + function configWith(overrides: Partial): LocalConfig { + return { + repo: { localPath: repoDir, remote: 'https://gitlab.com/acme/team.git' }, + username: 'alice', + additionalRoles: [], + scope: 'user', + ...overrides, + } as LocalConfig; + } + + function assigned(roles: string[]): LocalConfig { + const [primaryRole, ...additionalRoles] = roles; + return configWith({ + ...(primaryRole ? { primaryRole } : {}), + additionalRoles, + roleAssignment: { source: 'assigned', local: { additionalRoles: [] } }, + }); + } + + function given(localConfig: LocalConfig): void { + vi.mocked(autoDetectInit).mockResolvedValue({ localConfig, teamConfig }); + } + + beforeEach(() => { + repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-assigned-cmd-')); + mkdirSync(path.join(repoDir, 'manifest'), { recursive: true }); + writeFileSync( + path.join(repoDir, 'manifest', 'roles.yaml'), + 'version: 1\nroles:\n - id: frontend\n resources: { knowledge: [], skills: [frontend] }\n - id: backend\n resources: { knowledge: [], skills: [backend] }\n', + 'utf-8', + ); + printed = []; + vi.spyOn(console, 'log').mockImplementation((line?: unknown) => { printed.push(String(line ?? '')); }); + }); + + afterEach(() => { + vi.restoreAllMocks(); + vi.mocked(saveScopeSelection).mockReset(); + rmSync(repoDir, { recursive: true, force: true }); + }); + + it('refuses to set a role, naming the file and the roles held', async () => { + given(assigned(['frontend'])); + const error = vi.spyOn(log, 'error').mockImplementation(() => {}); + + await rolesSet('backend', {}); + + expect(error).toHaveBeenCalledWith(expect.stringContaining('manifest/members.yaml')); + expect(error).toHaveBeenCalledWith(expect.stringContaining('Yours: frontend')); + expect(saveScopeSelection).not.toHaveBeenCalled(); + expect(process.exitCode).toBe(1); + process.exitCode = 0; + }); + + it('still sets a role when the team assigns none', async () => { + given(configWith({})); + + await rolesSet('backend', {}); + + expect(saveScopeSelection).toHaveBeenCalledWith( + expect.anything(), + expect.objectContaining({ primaryRole: 'backend' }), + ); + }); + + it('reports the effective roles and where they come from as JSON', async () => { + given(assigned(['frontend', 'backend'])); + + await rolesList({ json: true }); + + expect(JSON.parse(printed.join('\n'))).toEqual({ + roles: ['frontend', 'backend'], + primaryRole: 'frontend', + additionalRoles: ['backend'], + source: 'assigned', + assignmentFile: 'manifest/members.yaml', + }); + }); + + it('reports a local selection as local, with no assignment file', async () => { + given(configWith({ primaryRole: 'backend' })); + + await rolesList({ json: true }); + + expect(JSON.parse(printed.join('\n'))).toMatchObject({ source: 'local', assignmentFile: null }); + }); + + it('reports a member the team assigned no roles as holding none', async () => { + given(configWith({ roleAssignment: { source: 'default', local: { additionalRoles: [] } } })); + + await rolesList({ json: true }); + + expect(JSON.parse(printed.join('\n'))).toMatchObject({ roles: [], primaryRole: null, source: 'default' }); + }); + + it('says where the roles come from in the human listing', async () => { + given(assigned(['frontend'])); + + await rolesList(); + + expect(printed.join('\n')).toContain('Your primary role: frontend'); + expect(printed.join('\n')).toContain('Roles are assigned in manifest/members.yaml'); + }); +}); diff --git a/src/config.ts b/src/config.ts index 341e4c65d..86ab92882 100644 --- a/src/config.ts +++ b/src/config.ts @@ -23,7 +23,9 @@ import { nearestGitEntry, resolveAnchors } from './utils/git.js'; import { getUserHome } from './utils/home.js'; import { resolvePartitionDir, writeAnchorFile } from './utils/partition.js'; import { log } from './utils/logger.js'; -import { loadRolesManifest, RolesManifestNotFoundError } from './roles.js'; +import { listRoleIds, loadRolesManifest, RolesManifestNotFoundError } from './roles.js'; +import { loadRoleAssignmentsIfPresent, resolveAssignedRoles, ROLE_ASSIGNMENTS_FILE } from './role-assignments.js'; +import { warnOnce } from './utils/warn-once.js'; /** * Loads that serve a --dry-run write nothing: the legacy role migration, the @@ -45,7 +47,10 @@ async function migrateLegacyRoleConfig( configPath: string, options: LoadOptions = {}, ): Promise { - if (config.primaryRole) { + // The assignment overlay runs first, and an assignment already decided this + // member's roles — holding none is one of its answers (`default: []`). + // Migrating would persist a `hai` the team did not assign. + if (config.primaryRole || config.roleAssignment) { return config; } @@ -85,6 +90,111 @@ async function migrateLegacyRoleConfig( return migrated; } +/** + * The roles `manifest/members.yaml` assigns this member, overlaid on the config + * this machine holds (#1186). + * + * Every reader of `primaryRole`/`additionalRoles` — pull, push, the namespace + * filter, the hooks and env a role scopes — then sees the team's assignment + * without knowing it was assigned, and `teamai roles set` has nothing left to + * change. The overlay is runtime-only: `serializeLocalConfig` writes the local + * selection back, so a save by any other command never freezes a copy of the + * assignment into the config file. + * + * Absence of the file is the only case that leaves the local selection in + * charge. A file that exists but cannot be read, or that assigns a role + * `roles.yaml` does not declare, leaves this member with fewer roles — never + * with the selection the file was written to take over. + */ +async function applyRoleAssignment(config: LocalConfig): Promise { + // Idempotent: a project config passes through `readConfigFrom` and then the + // loader that migrates it, and the second pass must not read the overlaid + // roles as the local selection to keep. + if (config.roleAssignment) return config; + + let assignments; + try { + assignments = await loadRoleAssignmentsIfPresent(config.repo.localPath); + } catch (error) { + warnOnce(`${ROLE_ASSIGNMENTS_FILE} could not be read: ${(error as Error).message}`); + warnOnce(`Roles are assigned in ${ROLE_ASSIGNMENTS_FILE} while that file exists, so this run holds none. ` + + 'Fix it in the team repo, or ask a code owner of manifest/ to.'); + return withAssignedRoles(config, [], 'default'); + } + if (!assignments) return config; + + const { roles, source } = resolveAssignedRoles(assignments, config.username); + return withAssignedRoles(config, await declaredRoles(config.repo.localPath, roles), source); +} + +/** The assigned ids `manifest/roles.yaml` still declares, with a word about the rest. */ +async function declaredRoles(repoPath: string, roles: string[]): Promise { + if (roles.length === 0) return roles; + + let declared: Set; + try { + declared = new Set(listRoleIds(await loadRolesManifest(repoPath))); + } catch (error) { + // Without a roles manifest nothing maps an assigned id to its namespaces. + // The id doubles as a namespace in the legacy fallback, but guessing that + // here would deliver directories no reviewed manifest named. + warnOnce(`${ROLE_ASSIGNMENTS_FILE} assigns ${roles.join(', ')}, but manifest/roles.yaml could not be read: ` + + `${(error as Error).message}. No role resources are delivered until it can.`); + return []; + } + + const unknown = roles.filter((role) => !declared.has(role)); + if (unknown.length > 0) { + warnOnce(`${ROLE_ASSIGNMENTS_FILE} assigns ${unknown.join(', ')}, which manifest/roles.yaml does not declare. ` + + `Ignoring ${unknown.length === 1 ? 'it' : 'them'}; fix one file or the other in the team repo.`); + } + return roles.filter((role) => declared.has(role)); +} + +/** `config` with the member's own selection back in place of an overlay. */ +function withLocalRoles(config: LocalConfig): LocalConfig { + const { roleAssignment, ...local } = config; + if (!roleAssignment) return config; + const restored: LocalConfig = { ...local, additionalRoles: roleAssignment.local.additionalRoles }; + if (roleAssignment.local.primaryRole) restored.primaryRole = roleAssignment.local.primaryRole; + else delete restored.primaryRole; + return restored; +} + +/** + * Resolve the assignment again for a config that is already loaded (#1186). + * + * `pull` loads the config before it updates the team repo, so the assignment it + * read is the one the previous pull left on disk. Re-resolving after the + * refresh is what makes a reassignment take effect on the pull that fetches it, + * instead of delivering the old roles and recording the new revision as synced + * — which the fast path would then skip until `pull --force`. + */ +export async function refreshRoleAssignment(config: LocalConfig): Promise { + // Also covers an assignments file that just arrived, or just went away: the + // local selection is restored first, so absence hands the roles back to it. + return applyRoleAssignment(withLocalRoles(config)); +} + +/** `roles` as this member's effective selection, primary first. */ +function withAssignedRoles(config: LocalConfig, roles: string[], source: 'assigned' | 'default'): LocalConfig { + const [primaryRole, ...additionalRoles] = roles; + const assigned: LocalConfig = { + ...config, + additionalRoles, + roleAssignment: { + source, + local: { + ...(config.primaryRole ? { primaryRole: config.primaryRole } : {}), + additionalRoles: config.additionalRoles ?? [], + }, + }, + }; + if (primaryRole) assigned.primaryRole = primaryRole; + else delete assigned.primaryRole; + return assigned; +} + /** * Load the team config (teamai.yaml) from the team repo */ @@ -142,7 +252,7 @@ export async function loadLocalConfig(options: LoadOptions = {}): Promise

'+e(dist.effective.error)+'

') +(dist.children.length?'

'+label('Delivered with child workspaces')+':
'+dist.children.map(child=>e(child.path)+' · '+(child.primaryRole?e(child.primaryRole):label('None'))+(child.projects.length?' + '+child.projects.map(e).join(', '):'')).join('
')+'

':'') +''; + const assigned=dist.rolesAssigned; + const held=role=>sel.primaryRole===role.id||sel.additionalRoles.includes(role.id); const rolesPanel=dist.roles.state==='invalid'?'

'+e(dist.roles.error)+'

' :dist.roles.state==='missing'?'' - :'

'+label('Roles')+' '+number(dist.roles.items.length)+'

'+label('Pick one primary role and any additional roles')+'
' - +dist.roles.items.map(role=>'

'+e(role.description||'—')+'

'+namespaceChips(role.namespaces)+'
').join('')+'
'; + :'

'+label('Roles')+' '+number(dist.roles.items.length)+'

'+(assigned?label('Assigned in the team repo')+': '+e(assigned.file):label('Pick one primary role and any additional roles'))+'
' + +dist.roles.items.map(role=>'

'+(assigned + ?e(role.id)+(held(role)?' '+label('Yours')+'':'') + :' ') + +'

'+e(role.description||'—')+'

'+namespaceChips(role.namespaces)+'
').join('')+'
'; const projectsPanel=dist.projects.state==='invalid'?'

'+e(dist.projects.error)+'

' :dist.projects.state==='missing'?'' :'

'+label('Projects')+' '+number(dist.projects.items.length)+'

'+label('Saving replaces the active project list')+'
' @@ -177,8 +182,8 @@ function distribution() { +(previewError?''+e(previewError)+'':'')+''; return top+rolesPanel+projectsPanel+actions+previewPanel; } -async function previewSelection() { if(!dist||!distDraft)return; const generation=workspaceGeneration, version=distVersion; previewing=true;previewError='';saveError='';savedNote='';render(); try{ const data=await postJson('/api/distribution/preview',{primaryRole:distDraft.primary,additionalRoles:distDraft.extra,projects:distDraft.projects}); if(generation!==workspaceGeneration||version!==distVersion)return; preview=data; } catch(error){ if(generation!==workspaceGeneration||version!==distVersion)return; preview=null; previewError=error.message; } finally{ if(generation===workspaceGeneration){ previewing=false; if(page==='distribution')render(); } } } -async function saveSelection() { if(!dist||!distDraft||!preview)return; const generation=workspaceGeneration, version=distVersion; saving=true;saveError='';savedNote='';render(); try{ let revision=dist.revision; if(dist.roles.state==='ok'){ const fresh=await postJson('/api/distribution/roles',{primaryRole:distDraft.primary,additionalRoles:distDraft.extra,revision}); // A workspace switch or draft edit mid-save must stop the chain before +async function previewSelection() { if(!dist||!distDraft)return; const generation=workspaceGeneration, version=distVersion; previewing=true;previewError='';saveError='';savedNote='';render(); try{ const data=await postJson('/api/distribution/preview',dist.rolesAssigned?{projects:distDraft.projects}:{primaryRole:distDraft.primary,additionalRoles:distDraft.extra,projects:distDraft.projects}); if(generation!==workspaceGeneration||version!==distVersion)return; preview=data; } catch(error){ if(generation!==workspaceGeneration||version!==distVersion)return; preview=null; previewError=error.message; } finally{ if(generation===workspaceGeneration){ previewing=false; if(page==='distribution')render(); } } } +async function saveSelection() { if(!dist||!distDraft||!preview)return; const generation=workspaceGeneration, version=distVersion; saving=true;saveError='';savedNote='';render(); try{ let revision=dist.revision; if(dist.roles.state==='ok'&&!dist.rolesAssigned){ const fresh=await postJson('/api/distribution/roles',{primaryRole:distDraft.primary,additionalRoles:distDraft.extra,revision}); // A workspace switch or draft edit mid-save must stop the chain before // the second write: the revision belongs to the old workspace/selection. if(generation!==workspaceGeneration)return; if(version!==distVersion){ saveError='The selection changed while saving. Preview again to save the rest.'; return; } revision=fresh.revision; } if(dist.projects.state==='ok')await postJson('/api/distribution/projects',{projects:distDraft.projects,revision}); if(generation!==workspaceGeneration||version!==distVersion)return; savedNote='Saved. Takes effect on the next pull.'; } catch(error){ if(generation!==workspaceGeneration)return; saveError=error.message; } finally{ if(generation===workspaceGeneration){ saving=false; const note=savedNote, err=saveError; await loadDistribution(); savedNote=note; saveError=err; if(page==='distribution')render(); } } } // Translate static report labels only; never translate knowledge titles, authors, commands or session text. diff --git a/src/dashboard/distribution.ts b/src/dashboard/distribution.ts index 7b660f543..769fc58f6 100644 --- a/src/dashboard/distribution.ts +++ b/src/dashboard/distribution.ts @@ -15,6 +15,17 @@ import { log } from '../utils/logger.js'; import type { LocalConfig } from '../types.js'; import { diffDelivery, pickNamespaces, resolveDelivery, type Delivery } from './delivery.js'; import { HttpError } from './http.js'; +import { ROLE_ASSIGNMENTS_FILE } from '../role-assignments.js'; + +/** + * The team assigns this workspace's roles (#1186), so the page shows them and + * edits nothing: a save would be overlaid away on the next load. + */ +function assertRolesEditable(ownConfig: LocalConfig): void { + if (!ownConfig.roleAssignment) return; + throw new HttpError(409, `Roles are assigned in ${ROLE_ASSIGNMENTS_FILE} in the team repo and cannot be changed here. ` + + 'Open a merge request against that file, or ask a code owner of manifest/ to.'); +} export interface DistributionSelection { primaryRole: string | null; @@ -146,6 +157,8 @@ export async function readDistribution(ownConfig: LocalConfig) { configured: true as const, revision: revisionOf(ownConfig), selection: selectionOf(ownConfig), + // Null when this workspace still picks its own roles. + rolesAssigned: ownConfig.roleAssignment ? { file: ROLE_ASSIGNMENTS_FILE, source: ownConfig.roleAssignment.source } : null, // A workspace saves only its own selection; its children's are shown, not edited. children: (withChildren.children?.children ?? []).map((child) => ({ path: child.path, ...selectionOf(child.selection) })), roles: roles.state === 'ok' @@ -172,6 +185,7 @@ export async function previewDistribution(ownConfig: LocalConfig, body: Record): Promise { + assertRolesEditable(ownConfig); assertRevision(ownConfig, body); const { primaryRole, additionalRoles, version } = await roleChange(ownConfig.repo.localPath, body); await saveScopeSelection(ownConfig, { ...ownConfig, primaryRole, additionalRoles, resourceProfileVersion: version }); diff --git a/src/index.ts b/src/index.ts index 679802134..f18905250 100644 --- a/src/index.ts +++ b/src/index.ts @@ -366,14 +366,15 @@ rolesCmd rolesCmd .command('list') .description('List all defined roles and your current role') - .action(async () => { + .option('--json', 'Print your effective roles and where they come from (assigned, default, local, none)') + .action(async (cmdOpts) => { const { rolesList } = await import('./roles-cmd.js'); - await rolesList(); + await rolesList(cmdOpts); }); rolesCmd .command('set ') - .description('Set your primary role (updates local config)') + .description('Set your primary role (updates local config; refused when manifest/members.yaml assigns roles)') .option('--add ', 'Additional roles to include') .action(async (primary: string, cmdOpts) => { const globalOpts = program.opts() as GlobalOptions; diff --git a/src/init.ts b/src/init.ts index 6cefb5336..c90b4cd87 100644 --- a/src/init.ts +++ b/src/init.ts @@ -97,6 +97,7 @@ import { } from './types.js'; import { getUserHome } from './utils/home.js'; import { describeRoles, listRoleIds, loadRolesManifest, RolesManifestNotFoundError } from './roles.js'; +import { loadRoleAssignmentsIfPresent, resolveAssignedRoles, ROLE_ASSIGNMENTS_FILE } from './role-assignments.js'; import { loadProjectsManifest, listProjectIds, type ProjectsManifest } from './projects.js'; import { memberReadRoots, readMemberConfig, mergeMemberConfig } from './members.js'; import { noDryRunPreview } from './dry-run-guard.js'; @@ -234,8 +235,26 @@ class NoRoleSelectedError extends Error {} async function promptForRoleProfile( repoPath: string, + username: string, roleFlag?: string, ): Promise> { + // The team assigns roles (#1186): there is nothing to pick, and nothing to + // write — every load reads the assignment from the team repo. + const assignments = await loadRoleAssignmentsIfPresent(repoPath); + if (assignments) { + if (roleFlag) { + throw new Error( + `Roles are assigned in ${ROLE_ASSIGNMENTS_FILE} in the team repo, so --role cannot set one. ` + + 'Open a merge request against that file, or ask a code owner of manifest/ to.', + ); + } + const { roles } = resolveAssignedRoles(assignments, username); + log.info(roles.length > 0 + ? `Roles assigned to ${username} in ${ROLE_ASSIGNMENTS_FILE}: ${roles.join(', ')}` + : `No roles assigned to ${username} in ${ROLE_ASSIGNMENTS_FILE}; you receive shared resources only.`); + return { additionalRoles: [] }; + } + const manifest = await loadRolesManifest(repoPath); const roleLabels = describeRoles(manifest.roles); @@ -698,7 +717,7 @@ export async function initHttp( ...(inheritUserScope !== undefined ? { inheritUserScope } : {}), }; try { - Object.assign(localConfig, await promptForRoleProfile(localPath, options.role)); + Object.assign(localConfig, await promptForRoleProfile(localPath, localConfig.username, options.role)); } catch (error) { // Two cases leave the role unset on purpose: a repo with no roles manifest, // and a person who skipped the prompt. Anything else — a manifest that does @@ -1307,7 +1326,7 @@ export async function initSelfRepo(options: GlobalOptions & { ...(inheritUserScope !== undefined ? { inheritUserScope } : {}), }; try { - Object.assign(localConfig, await promptForRoleProfile(localPath, options.role)); + Object.assign(localConfig, await promptForRoleProfile(localPath, localConfig.username, options.role)); } catch (error) { // Two cases leave the role unset on purpose: a repo with no roles manifest, // and a person who skipped the prompt. Anything else — a manifest that does @@ -1936,7 +1955,7 @@ export async function init(options: GlobalOptions & { if (projectsManifest?.projects.length) { rolePromptedEarly = true; try { - roleProfile = await promptForRoleProfile(localPath, options.role); + roleProfile = await promptForRoleProfile(localPath, username, options.role); } catch (error) { const msg = (error as Error).message; if (msg.includes('Roles manifest not found')) { @@ -2123,7 +2142,7 @@ export async function init(options: GlobalOptions & { if (!rolePromptedEarly) { try { - roleProfile = await promptForRoleProfile(localPath, options.role); + roleProfile = await promptForRoleProfile(localPath, username, options.role); } catch (error) { const msg = (error as Error).message; if (msg.includes('Roles manifest not found')) { diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index 6b6af1304..e8a483017 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -43,6 +43,12 @@ const TRAILING_DOT_OR_SPACE = /[ .]$/; // accepted, and narrowing it would break manifests that parse today. const WINDOWS_DEVICE_NAME = /^(con|conin\$|conout\$|prn|aux|nul|(com|lpt)[1-9\u00b9\u00b2\u00b3])(\.|$)/i; +/** + * A manifest this CLI reads from `manifest/`. Names the file in every error a + * manifest read or parse produces, so one spelling serves all of them. + */ +export type ManifestKind = 'projects' | 'roles' | 'members'; + /** True if `seg` is safe to use as a single path segment (no separators, no `..`). */ export function isSafeNamespaceSegment(seg: string): boolean { return seg.length > 0 @@ -158,7 +164,7 @@ export function fallbackNamespaceError(ids: string[], source: string): string | * it do: one line naming the offending entry. A raw ZodError reaches the CLI as * an object dump, which tells an admin nothing about which line to edit. */ -export function parseManifest(schema: S, raw: unknown, kind: 'projects' | 'roles'): z.infer { +export function parseManifest(schema: S, raw: unknown, kind: ManifestKind): z.infer { const parsed = schema.safeParse(raw); if (parsed.success) return parsed.data; @@ -200,7 +206,7 @@ async function danglingLinkOnPath(target: string): Promise { * then drop its filtering because the file is unreadable or empty — the fail-open * direction. Absence returns `null` here; anything else throws. */ -export async function readManifestFile(manifestPath: string, kind: 'projects' | 'roles'): Promise { +export async function readManifestFile(manifestPath: string, kind: ManifestKind): Promise { // `repo.localPath` is documented as `~/.teamai/...`; the helpers this replaced // expanded it, and a path left unexpanded would be searched under the current // directory, read as absent, and relax the filtering. @@ -217,7 +223,10 @@ export async function readManifestFile(manifestPath: string, kind: 'projects' | throw new Error(`The ${kind} manifest ${resolvedPath} could not be read: ${(error as Error).message}. Make it a readable file, then retry.`); } if (content.trim() === '') { - throw new Error(`Invalid ${kind} manifest: ${resolvedPath} is empty. Give it a version and a ${kind} list; deleting it instead turns ${kind} filtering off for the whole team.`); + const consequence = kind === 'members' + ? 'deleting it instead hands role selection back to each member' + : `deleting it instead turns ${kind} filtering off for the whole team`; + throw new Error(`Invalid ${kind} manifest: ${resolvedPath} is empty. Give it a version and a ${kind} list; ${consequence}.`); } return content; } diff --git a/src/pull.ts b/src/pull.ts index 92ad23b7a..3fd1898b3 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -6,7 +6,7 @@ import { indexedSkills, resolveDesiredAgents, resolveDesiredRules, resolveDesiredSkills, type RolePullContext, } from './resources/desired.js'; import type { IndexedSkills, UnreadableFile } from './utils/search-index.js'; -import { detectProjectConfig, describeUnreadableConfig, loadLocalConfigForScope, loadTeamConfig, loadStateForScope, saveStateForScope } from './config.js'; +import { detectProjectConfig, describeUnreadableConfig, loadLocalConfigForScope, loadTeamConfig, loadStateForScope, refreshRoleAssignment, saveStateForScope } from './config.js'; import { pullRepo, getHeadRev, getHeadCommit, createGit, getDefaultBranch, completeWorktreeList, gitCommonDir, isLiveCheckout, isLiveProjectCheckout, listWorktrees, resolveAnchors } from './utils/git.js'; import { publishQueuedLearnings } from './utils/learnings-publish.js'; import { pendingLearningsDir } from './utils/pending-learnings.js'; @@ -1291,6 +1291,10 @@ async function pullForScope( const outcome = `[${scopeLabel}] Team repo: ${refresh.label}`; pullSpin.succeed(outcome); log.debug(outcome); + // manifest/members.yaml lives in the repo the refresh just moved, so the + // roles read at load time are the previous pull's (#1186). Re-resolving + // here is what lets a reassignment take effect on the pull that fetches it. + localConfig = await refreshRoleAssignment(localConfig); } catch (e) { const reason = `[${scopeLabel}] Pull failed: ${(e as Error).message}`; pullSpin.fail(reason); diff --git a/src/resource-namespaces.ts b/src/resource-namespaces.ts index 0fb4263f2..fac99b05d 100644 --- a/src/resource-namespaces.ts +++ b/src/resource-namespaces.ts @@ -48,6 +48,12 @@ async function resolveOwnResourceNamespaces(localConfig: LocalConfig) { const primaryRole = localConfig.primaryRole; const hasRole = !!primaryRole; const hasProjects = activeProjects.length > 0; + // The team assigns this member's roles (#1186). Every unfiltered fallback + // below is then closed: a member the team assigned no roles — or whose + // assignment names a role the manifest dropped — receives the shared root + // only. Falling through to "sync everything" would hand back exactly what + // assigning roles was meant to take away. + const rolesAssigned = !!localConfig.roleAssignment; // Load the projects manifest up front: its mere existence means this team uses // project partitioning, which changes the "no active filter" semantics below. @@ -83,7 +89,7 @@ async function resolveOwnResourceNamespaces(localConfig: LocalConfig) { // through to an unfiltered sync that reinstalls every project's skills/rules. // So we return a real (possibly empty-active) context and let the cleanup path // below prune the now-inactive project namespaces. - if (!hasRole && !hasProjects && !teamHasProjects) return null; + if (!rolesAssigned && !hasRole && !hasProjects && !teamHasProjects) return null; // ── Role namespaces (optional) ── let roleNamespaces: ResourceNamespaces = { knowledge: [], skills: [], learnings: [], agents: [] }; @@ -107,12 +113,13 @@ async function resolveOwnResourceNamespaces(localConfig: LocalConfig) { }); allRoleSkillNamespaces = new Set(rolesManifest.roles.flatMap((role) => role.resources.skills)); } catch { - warnOnce(`Role "${localConfig.primaryRole}" not found in manifest. Falling back to unfiltered sync.`); - warnOnce('Run `teamai roles set ` to pick a valid role.'); + warnOnce(`Role "${localConfig.primaryRole}" not found in manifest.` + + (rolesAssigned ? ' Delivering shared resources only.' : ' Falling back to unfiltered sync.')); + if (!rolesAssigned) warnOnce('Run `teamai roles set ` to pick a valid role.'); // A misconfigured role, with nothing else to scope by, can't filter safely. - if (!hasProjects && !teamHasProjects) return null; + if (!rolesAssigned && !hasProjects && !teamHasProjects) return null; } - } else if (!hasProjects && !teamHasProjects) { + } else if (!rolesAssigned && !hasProjects && !teamHasProjects) { return null; } } diff --git a/src/role-assignments.ts b/src/role-assignments.ts new file mode 100644 index 000000000..b806a0268 --- /dev/null +++ b/src/role-assignments.ts @@ -0,0 +1,126 @@ +import path from 'node:path'; +import YAML from 'yaml'; +import { z } from 'zod'; +import { caseFoldKey, parseManifest, parseManifestOnce, readManifestFile } from './manifest-schema.js'; + +/** Where a team declares who holds which role. Relative to the team repo root. */ +export const ROLE_ASSIGNMENTS_FILE = 'manifest/members.yaml'; + +/** + * `manifest/members.yaml`: the roles each member holds, as team data. + * + * A role a member sets locally (`teamai roles set`) is their own choice, which + * is fine while roles only pick what gets delivered. A team that also uses the + * effective roles to bound what its agents may touch needs the assignment to be + * a reviewed change in the team repo instead (#1186). Who may change it is the + * forge's decision, as for `roles.yaml`: protected branch, merge request, code + * owners for `manifest/`. teamai keeps no access list of its own. + * + * ```yaml + * version: 1 + * members: + * alice: [frontend] + * bob: [backend, ki] + * default: [] # roles of a member who is not listed + * ``` + * + * The first role listed is the member's primary role; the rest are additional. + */ +const RoleAssignmentsSchema = z.object({ + version: z.number(), + members: z.record(z.string().min(1), z.array(z.string().min(1))).default({}), + // A file that exists but lists nobody still governs: an unlisted member gets + // `default`, which is empty unless the team says otherwise. Absent means the + // same, never "every namespace" — that is the fail-closed half of #1096. + default: z.array(z.string().min(1)).default([]), +}); + +export type RoleAssignments = z.infer; + +/** Where a member's effective roles came from, as `roles list --json` reports it. */ +export type RoleSource = 'assigned' | 'default' | 'local' | 'none'; + +/** + * The team repo has no `manifest/members.yaml` — a team that assigns no roles, + * not a broken one. Only absence may leave the local selection in charge: + * a file that exists but cannot be read or parsed must not, or an agent could + * hand itself every role by making the file unreadable. + */ +export class RoleAssignmentsNotFoundError extends Error { + constructor(manifestPath: string) { + super(`Role assignments not found: ${manifestPath}`); + this.name = 'RoleAssignmentsNotFoundError'; + } +} + +function validateAssignments(raw: unknown): RoleAssignments { + if (!raw || typeof raw !== 'object' || Array.isArray(raw)) { + throw new Error('Invalid members manifest: expected an object'); + } + const manifest = parseManifest(RoleAssignmentsSchema, raw, 'members'); + + // Two keys that differ only by case are one person to a forge that folds + // usernames, and the member they describe would hold whichever the YAML + // parser kept last. Say so instead of picking one. + const seen = new Map(); + for (const username of Object.keys(manifest.members)) { + const key = caseFoldKey(username); + const prior = seen.get(key); + if (prior !== undefined && prior !== username) { + throw new Error( + `Invalid members manifest: "${prior}" and "${username}" differ only by case or Unicode normalization. ` + + 'Keep one entry per member', + ); + } + seen.set(key, username); + } + + return manifest; +} + +export async function loadRoleAssignments(repoPath: string): Promise { + const manifestPath = path.join(repoPath, ...ROLE_ASSIGNMENTS_FILE.split('/')); + const content = await readManifestFile(manifestPath, 'members'); + if (content === null) throw new RoleAssignmentsNotFoundError(manifestPath); + + return parseManifestOnce(manifestPath, content, (text) => { + let raw: unknown; + try { + raw = YAML.parse(text); + } catch (error) { + throw new Error(`Invalid members manifest YAML: ${(error as Error).message}`); + } + return validateAssignments(raw); + }); +} + +/** + * The role assignments, or `null` when the team has none. Absent is a value, + * invalid is an error — the same contract `loadRolesManifestIfPresent` keeps. + */ +export async function loadRoleAssignmentsIfPresent(repoPath: string): Promise { + try { + return await loadRoleAssignments(repoPath); + } catch (error) { + if (error instanceof RoleAssignmentsNotFoundError) return null; + throw error; + } +} + +/** + * The roles the team assigned to `username`, primary first. A member the file + * does not list gets `default`, which is usually no roles at all. + * + * The key is matched exactly: the manifest is checked for keys that differ only + * by case when it loads, so an exact match is the only one that can be meant. + */ +export function resolveAssignedRoles( + manifest: RoleAssignments, + username: string, +): { roles: string[]; source: 'assigned' | 'default' } { + const listed = Object.prototype.hasOwnProperty.call(manifest.members, username) + ? manifest.members[username] + : undefined; + const roles = listed ?? manifest.default; + return { roles: [...new Set(roles)], source: listed ? 'assigned' : 'default' }; +} diff --git a/src/roles-cmd.ts b/src/roles-cmd.ts index 1bf19ee06..dad89d36a 100644 --- a/src/roles-cmd.ts +++ b/src/roles-cmd.ts @@ -4,6 +4,7 @@ import { autoDetectInit } from './config.js'; import { saveScopeSelection } from './scope-selection.js'; import { loadRolesManifest, saveRolesManifest, findRole, listRoleIds } from './roles.js'; import type { RolesManifest, TeamRole } from './roles.js'; +import { ROLE_ASSIGNMENTS_FILE, type RoleSource } from './role-assignments.js'; import { pathExists } from './utils/fs.js'; import { log } from './utils/logger.js'; import { pullLatest, runManifestEdit, pushManifestChange } from './manifest-edit.js'; @@ -144,10 +145,54 @@ async function initializeRoles(options: GlobalOptions, localConfig: LocalConfig, // ─── roles list ───────────────────────────────────────── -export async function rolesList(): Promise { +/** + * This member's effective roles and where they come from. + * + * `assigned` — `manifest/members.yaml` lists this member; `default` — it exists + * but does not list them, so they hold the file's `default`; `local` — no such + * file, and the roles are this machine's own selection; `none` — neither. + * + * A team hook that bounds an agent by role reads this (`roles list --json`) + * rather than the member's config file, whose path and shape are teamai's own. + */ +export function effectiveRoles(localConfig: LocalConfig): { + roles: string[]; + primaryRole: string | null; + additionalRoles: string[]; + source: RoleSource; +} { + const primaryRole = localConfig.primaryRole ?? null; + const additionalRoles = primaryRole ? (localConfig.additionalRoles ?? []) : []; + const source: RoleSource = localConfig.roleAssignment + ? localConfig.roleAssignment.source + : primaryRole ? 'local' : 'none'; + return { + roles: primaryRole ? [...new Set([primaryRole, ...additionalRoles])] : [], + primaryRole, + additionalRoles, + source, + }; +} + +/** Said wherever a local role change is refused, and in `roles list`. */ +export function assignedRolesNotice(effective: ReturnType): string { + return `Roles are assigned in ${ROLE_ASSIGNMENTS_FILE} in the team repo. ` + + `Yours: ${effective.roles.join(', ') || 'none'}. ` + + 'To change them, open a merge request against that file, or ask a code owner of manifest/ to.'; +} + +export async function rolesList(options: { json?: boolean } = {}): Promise { // Read-only: the load never persists a migration (#893). const { localConfig } = await autoDetectInit(undefined, { dryRun: true }); const repoPath = localConfig.repo.localPath; + const effective = effectiveRoles(localConfig); + + // The effective roles are config, not manifest: report them even when the + // manifest cannot be read, so a hook reading --json is never left guessing. + if (options.json) { + console.log(JSON.stringify({ ...effective, assignmentFile: localConfig.roleAssignment ? ROLE_ASSIGNMENTS_FILE : null }, null, 2)); + return; + } let manifest; try { @@ -171,14 +216,20 @@ export async function rolesList(): Promise { } // Show current user's role - if (localConfig.primaryRole) { - console.log(`Your primary role: ${localConfig.primaryRole}`); - if (localConfig.additionalRoles && localConfig.additionalRoles.length > 0) { - console.log(`Additional roles: ${localConfig.additionalRoles.join(', ')}`); + if (effective.primaryRole) { + console.log(`Your primary role: ${effective.primaryRole}`); + if (effective.additionalRoles.length > 0) { + console.log(`Additional roles: ${effective.additionalRoles.join(', ')}`); } + } else if (localConfig.roleAssignment) { + console.log('You hold no roles, so you receive shared resources only.'); } else { console.log('You have no role configured. Run `teamai roles set ` to set one.'); } + if (localConfig.roleAssignment) { + console.log(''); + console.log(assignedRolesNotice(effective)); + } } // ─── roles set ────────────────────────────────────────── @@ -190,6 +241,15 @@ export async function rolesSet( const { localConfig } = await autoDetectInit(undefined, options); const repoPath = localConfig.repo.localPath; + // The team assigns roles (#1186): a local selection would be overlaid away + // on the next load, and letting it through would make the assignment look + // like something each member can still decide. + if (localConfig.roleAssignment) { + log.error(assignedRolesNotice(effectiveRoles(localConfig))); + process.exitCode = 1; + return; + } + let manifest; try { manifest = await loadRolesManifest(repoPath); diff --git a/src/roles.ts b/src/roles.ts index 2ce3ba9c1..121d0d5f9 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -236,11 +236,19 @@ export function resolveRoleResourceNamespaces(input: { * receiving every resource, the same fallback pull applies to skills and rules. * A config whose role could not be resolved (`roleUnresolved`) holds none: * `[]` matches no role-scoped entry. + * + * An assignment (`roleAssignment`, #1186) never reads as "no filter": a member + * the team assigned no roles holds none, which is the point of assigning them. */ export function activeRoleIds( - localConfig: { primaryRole?: string; additionalRoles?: string[]; roleUnresolved?: true }, + localConfig: { + primaryRole?: string; + additionalRoles?: string[]; + roleUnresolved?: true; + roleAssignment?: unknown; + }, ): string[] | null { if (localConfig.roleUnresolved) return []; - if (!localConfig.primaryRole) return null; + if (!localConfig.primaryRole) return localConfig.roleAssignment ? [] : null; return [...new Set([localConfig.primaryRole, ...(localConfig.additionalRoles ?? [])])]; } diff --git a/src/types.ts b/src/types.ts index 8f85cfd69..84d7cf966 100644 --- a/src/types.ts +++ b/src/types.ts @@ -902,7 +902,22 @@ export type LocalConfig = z.infer & { dataHome?: string; roleUnresolved?: true; children?: WorkspaceChildren; + roleAssignment?: RoleAssignment; }; + +/** + * Runtime-only, set when `manifest/members.yaml` governs this member's roles + * (#1186): `primaryRole` and `additionalRoles` then hold the team's assignment, + * read fresh on every load, and `local` keeps what the config file says so a + * save by any other command writes that back instead of freezing a copy of the + * assignment into the file. + */ +export interface RoleAssignment { + /** `assigned` — the file lists this member; `default` — it does not. */ + source: 'assigned' | 'default'; + /** The selection on disk, which `roles set` no longer changes. */ + local: { primaryRole?: string; additionalRoles: string[] }; +} export type LocalConfigInput = z.input; // ─── Local state (~/.teamai/state.json) ──────────────────── diff --git a/src/workspace-children.ts b/src/workspace-children.ts index 28f4bf5f0..6d4631d4a 100644 --- a/src/workspace-children.ts +++ b/src/workspace-children.ts @@ -142,11 +142,19 @@ export async function deliveredMembers(config: LocalConfig): Promise Date: Fri, 16 Oct 2026 19:50:32 +0200 Subject: [PATCH 2/6] fix(roles): keep a refresh failure out of pull's fetch error path (#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. --- src/__tests__/pull-placement-reconcile.test.ts | 3 +++ src/pull.ts | 12 ++++++++---- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/src/__tests__/pull-placement-reconcile.test.ts b/src/__tests__/pull-placement-reconcile.test.ts index 54630f96b..71cfac17f 100644 --- a/src/__tests__/pull-placement-reconcile.test.ts +++ b/src/__tests__/pull-placement-reconcile.test.ts @@ -16,6 +16,9 @@ vi.mock('../config.js', () => ({ detectProjectConfig: vi.fn().mockResolvedValue(null), loadStateForScope: vi.fn().mockResolvedValue({ lastPull: null, lastPullRev: null }), saveStateForScope: vi.fn(), + // Pull re-resolves the role assignment once the team repo has moved (#1186); + // this suite's configs carry none, so it hands the config straight back. + refreshRoleAssignment: vi.fn(async (config: unknown) => config), })); vi.mock('../utils/git.js', () => ({ diff --git a/src/pull.ts b/src/pull.ts index 3fd1898b3..bb54aae98 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -1291,10 +1291,6 @@ async function pullForScope( const outcome = `[${scopeLabel}] Team repo: ${refresh.label}`; pullSpin.succeed(outcome); log.debug(outcome); - // manifest/members.yaml lives in the repo the refresh just moved, so the - // roles read at load time are the previous pull's (#1186). Re-resolving - // here is what lets a reassignment take effect on the pull that fetches it. - localConfig = await refreshRoleAssignment(localConfig); } catch (e) { const reason = `[${scopeLabel}] Pull failed: ${(e as Error).message}`; pullSpin.fail(reason); @@ -1310,6 +1306,14 @@ async function pullForScope( return; } + // manifest/members.yaml lives in the repo the refresh above just moved, so the + // roles read when the config loaded are the previous pull's (#1186). + // Re-resolving here is what lets a reassignment take effect on the pull that + // fetches it, rather than on the one after — by which time this pull has + // recorded the new revision as synced and the fast path skips it. + // Outside the refresh's try: a failure here is not a failure to fetch. + localConfig = await refreshRoleAssignment(localConfig); + // Settle the placement records against the tree just refreshed, before // delivery reads them: a placement whose PR has merged becomes a record, one // whose file the team deleted stops being one, and in legacy mode one From c0452883a92838e8c37cf73722d672da1febf6c3 Mon Sep 17 00:00:00 2001 From: Nils Schiller Date: Sat, 17 Oct 2026 15:09:12 +0200 Subject: [PATCH 3/6] fix(roles): resolve the assignment where pull and init actually use it (#1186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review of #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. --- docs/designs/assigned-roles.md | 9 +- src/__tests__/e2e/assigned-roles-1186.test.ts | 97 +++++++++++++++++++ .../pull-placement-reconcile.test.ts | 2 +- src/__tests__/role-assignments.test.ts | 47 ++++++++- src/config.ts | 36 ++++--- src/init.ts | 15 ++- src/pull.ts | 4 +- 7 files changed, 189 insertions(+), 21 deletions(-) diff --git a/docs/designs/assigned-roles.md b/docs/designs/assigned-roles.md index 53492b475..5005d7d07 100644 --- a/docs/designs/assigned-roles.md +++ b/docs/designs/assigned-roles.md @@ -39,9 +39,14 @@ With assignments in place, nothing falls back to an unfiltered sync: - A `members.yaml` that exists but cannot be read or parsed leaves this run with no roles, loudly. Only absence of the file returns role selection to the member. - An assigned role `roles.yaml` does not declare is dropped with a warning, rather than failing every command for that member over one stale line. If that leaves no roles — including when `roles.yaml` is missing entirely — the member holds none. -### Takes effect on the pull that fetches it +### Resolved again where a load cannot reach -The assignment lives in the repo `pull` itself refreshes, and the config is loaded before that refresh. Resolved only at load time, the first pull after a reassignment would deliver the previous roles and record the new revision as synced, so the fast path would skip every pull after it until `--force`. `pullForScope` therefore re-resolves the assignment (`refreshRoleAssignment`) right after the team repo moves, which also picks up an assignments file that just arrived or just went away. +`refreshRoleAssignment` re-resolves the assignment on a config already in memory. It **mutates** that config rather than returning a copy: pull keys `teamEnvs` and `instructions` on config identity, and reconciles hooks, MCP and env on the object its caller holds, so a replacement would reach none of them. + +Two callers: + +- `pull`. The assignment lives in the repo `pull` itself refreshes, and the config is loaded before that refresh. Resolved only at load time, the first pull after a reassignment would deliver the previous roles and record the new revision as synced, so the fast path would skip every pull after it until `--force`. `pullForScope` therefore re-resolves right after the team repo moves, which also picks up an assignments file that just arrived or just went away. +- `init`. It builds the config in memory and hands it straight to the hook, MCP and env reconciliation, before any load can overlay it. Without this the config reaches them role-less, and a role-less config matches every role-scoped entry: a member assigned `frontend` would have `backend`'s hooks installed at init. The assignment is applied after the role profile and before that reconciliation; nothing of it is written, since the save keeps the member's own selection, which an assignment leaves empty. In a workspace with children (#913) every delivered member holds the workspace's assignment: a child cannot select a role, so the selection it recorded is not one to honour. diff --git a/src/__tests__/e2e/assigned-roles-1186.test.ts b/src/__tests__/e2e/assigned-roles-1186.test.ts index 006ef7f68..4d51a8a0e 100644 --- a/src/__tests__/e2e/assigned-roles-1186.test.ts +++ b/src/__tests__/e2e/assigned-roles-1186.test.ts @@ -216,3 +216,100 @@ describe('assigned roles (#1186)', () => { expect(deliveredSkills(projectRoot)).toEqual(['teamai']); }); }); + +/** + * `init` builds its config in memory and hands it to the hook, MCP and env + * reconciliation before any load can overlay it (#1186). Without the + * assignment, that config is role-less, and a role-less config matches every + * role-scoped entry — so init would install the hooks the manifest restricts. + */ +describe('assigned roles at init (#1186)', () => { + let sandbox: string; + let homeDir: string; + + afterAll(() => { + if (sandbox) fs.rmSync(sandbox, { recursive: true, force: true }); + }); + + it('applies only the assigned role\'s hooks, and writes no role to the config', async () => { + if (!fs.existsSync(CLI)) throw new Error(`CLI binary not found at ${CLI}. Run "npm run build" first.`); + + sandbox = fs.mkdtempSync(path.join(os.tmpdir(), 'teamai-assigned-init-e2e-')); + homeDir = path.join(sandbox, 'home'); + const seed = path.join(sandbox, 'seed'); + const remote = path.join(sandbox, 'remote.git'); + // The git provider wants a URL; `insteadOf` maps it to the bare repo here. + const url = 'https://git.example.com/team/assigned-init.git'; + const project = path.join(sandbox, 'project'); + fs.mkdirSync(path.join(homeDir, '.claude'), { recursive: true }); + fs.mkdirSync(seed, { recursive: true }); + fs.mkdirSync(project, { recursive: true }); + + const write = (file: string, text: string): void => { + fs.mkdirSync(path.dirname(file), { recursive: true }); + fs.writeFileSync(file, text); + }; + // The `git` provider reads the member's name from git config; alice is the + // member manifest/members.yaml lists. + execFileSync('git', ['config', '--global', 'user.name', 'alice'], { env: { ...process.env, HOME: homeDir }, stdio: 'pipe' }); + execFileSync('git', ['config', '--global', 'user.email', 'alice@teamai.test'], { env: { ...process.env, HOME: homeDir }, stdio: 'pipe' }); + + write(path.join(seed, 'teamai.yaml'), [ + 'team: assigned-init-e2e', + `repo: ${url}`, + 'provider: git', + 'usageReport: false', + 'toolPaths:', + ' claude:', + ' skills: .claude/skills', + '', + ].join('\n')); + write(path.join(seed, 'manifest/roles.yaml'), [ + 'version: 1', + 'roles:', + ' - id: frontend', + ' resources: { knowledge: [], skills: [frontend], hooks: [frontend] }', + ' - id: backend', + ' resources: { knowledge: [], skills: [backend], hooks: [backend] }', + '', + ].join('\n')); + write(path.join(seed, 'manifest/members.yaml'), 'version: 1\nmembers:\n alice: [frontend]\ndefault: []\n'); + write(path.join(seed, 'hooks/frontend/hooks.yaml'), 'hooks:\n - id: fe-stop\n event: Stop\n command: echo frontend-hook\n description: Frontend\n'); + write(path.join(seed, 'hooks/backend/hooks.yaml'), 'hooks:\n - id: be-stop\n event: Stop\n command: echo backend-hook\n description: Backend\n'); + write(path.join(seed, 'skills/frontend/frontend-only/SKILL.md'), '# frontend-only\n'); + write(path.join(seed, 'skills/backend/backend-only/SKILL.md'), '# backend-only\n'); + + git(['init', '-q', '-b', 'main'], seed); + git(['add', '-A'], seed); + git(['commit', '-qm', 'fixture'], seed); + // -b main: the bare repo's HEAD decides what init clones, and a HEAD on + // another branch reads as an empty team repo. + git(['init', '-q', '--bare', '-b', 'main', remote], sandbox); + execFileSync('git', ['config', '--global', `url.${remote}.insteadOf`, url], { env: { ...process.env, HOME: homeDir }, stdio: 'pipe' }); + git(['remote', 'add', 'origin', remote], seed); + git(['push', '-q', 'origin', 'main'], seed); + git(['init', '-q', '-b', 'main'], project); + + const init = await runCLI( + ['init', url, '--provider', 'git', '--agent', 'claude', '--scope', 'project', '--force'], + homeDir, + project, + ); + expect(init.code, init.output).toBe(0); + // No role prompt: the team assigned them, and init says which. + expect(init.output).toContain('Roles assigned to alice in manifest/members.yaml: frontend'); + // The hooks init applies are the assigned role's alone. A role-less config + // matches every role-scoped hook, so without the assignment both land here. + expect(init.output).toContain('fe-stop'); + expect(init.output).not.toContain('be-stop'); + + // Nothing of the assignment is written: the config keeps no role at all. + const saved = fs.readFileSync(path.join(homeDir, '.teamai', 'projects', fs.readdirSync(path.join(homeDir, '.teamai', 'projects'))[0], 'config.yaml'), 'utf8'); + expect(saved).not.toContain('primaryRole'); + + const pull = await runCLI(['pull', '--force'], homeDir, project); + expect(pull.code, pull.output).toBe(0); + expect(fs.existsSync(path.join(project, '.claude', 'skills', 'frontend-only', 'SKILL.md'))).toBe(true); + expect(fs.existsSync(path.join(project, '.claude', 'skills', 'backend-only'))).toBe(false); + }); +}); diff --git a/src/__tests__/pull-placement-reconcile.test.ts b/src/__tests__/pull-placement-reconcile.test.ts index 71cfac17f..00f742543 100644 --- a/src/__tests__/pull-placement-reconcile.test.ts +++ b/src/__tests__/pull-placement-reconcile.test.ts @@ -18,7 +18,7 @@ vi.mock('../config.js', () => ({ saveStateForScope: vi.fn(), // Pull re-resolves the role assignment once the team repo has moved (#1186); // this suite's configs carry none, so it hands the config straight back. - refreshRoleAssignment: vi.fn(async (config: unknown) => config), + refreshRoleAssignment: vi.fn(async () => {}), })); vi.mock('../utils/git.js', () => ({ diff --git a/src/__tests__/role-assignments.test.ts b/src/__tests__/role-assignments.test.ts index 8df5eef8a..e90721db6 100644 --- a/src/__tests__/role-assignments.test.ts +++ b/src/__tests__/role-assignments.test.ts @@ -8,6 +8,7 @@ import { activeRoleIds } from '../roles.js'; import { loadRoleAssignments, loadRoleAssignmentsIfPresent, resolveAssignedRoles } from '../role-assignments.js'; import { resolveResourceNamespaces } from '../resource-namespaces.js'; import { log } from '../utils/logger.js'; +import type { LocalConfig } from '../types.js'; import { resetWarnOnce } from '../utils/warn-once.js'; /** @@ -199,9 +200,45 @@ describe('role assignments from manifest/members.yaml', () => { // What `pull` does: the config was loaded before the repo was refreshed. writeAssignments('version: 1\nmembers:\n alice: [backend]\n'); - const refreshed = await refreshRoleAssignment(config!); + await refreshRoleAssignment(config!); - expect(refreshed.primaryRole).toBe('backend'); + expect(config!.primaryRole).toBe('backend'); + }); + + // Pull keys `teamEnvs` and `instructions` on config identity, and reconciles + // hooks, MCP and env on the object its caller holds: a refreshed copy would + // reach none of them. + it('refreshes the config in place, keeping its identity', async () => { + writeAssignments('version: 1\nmembers:\n alice: [frontend]\n'); + writeConfig(); + const config = (await loadLocalConfig())!; + const byIdentity = new Map([[config, 'env']]); + + writeAssignments('version: 1\nmembers: {}\ndefault: []\n'); + await refreshRoleAssignment(config); + + expect(byIdentity.get(config)).toBe('env'); + expect(config.primaryRole).toBeUndefined(); + expect(config.roleAssignment?.source).toBe('default'); + // Fail closed through the same object the reconcilers hold. + expect(activeRoleIds(config)).toEqual([]); + }); + + // What `init` does: it builds the config in memory and hands it to the hook, + // MCP and env reconciliation before any load can overlay it. + it('applies the assignment to a config built in memory, as init hands it on', async () => { + writeAssignments('version: 1\nmembers:\n alice: [frontend]\n'); + const built: LocalConfig = { + repo: { localPath: repoDir, remote: 'https://gitlab.com/acme/team.git' }, + username: 'alice', + additionalRoles: [], + scope: 'user', + }; + + await refreshRoleAssignment(built); + + expect(activeRoleIds(built)).toEqual(['frontend']); + expect(built.roleAssignment?.source).toBe('assigned'); }); it('hands the roles back to the member when the assignments file goes away', async () => { @@ -211,10 +248,10 @@ describe('role assignments from manifest/members.yaml', () => { expect(config?.primaryRole).toBe('frontend'); rmSync(path.join(repoDir, 'manifest', 'members.yaml')); - const refreshed = await refreshRoleAssignment(config!); + await refreshRoleAssignment(config!); - expect(refreshed.primaryRole).toBe('backend'); - expect(refreshed.roleAssignment).toBeUndefined(); + expect(config!.primaryRole).toBe('backend'); + expect(config!.roleAssignment).toBeUndefined(); }); // The legacy migration writes a `hai` role into a role-less config. A member diff --git a/src/config.ts b/src/config.ts index 86ab92882..df34713cb 100644 --- a/src/config.ts +++ b/src/config.ts @@ -162,18 +162,32 @@ function withLocalRoles(config: LocalConfig): LocalConfig { } /** - * Resolve the assignment again for a config that is already loaded (#1186). + * Resolve the assignment again on a config that is already in memory (#1186). * - * `pull` loads the config before it updates the team repo, so the assignment it - * read is the one the previous pull left on disk. Re-resolving after the - * refresh is what makes a reassignment take effect on the pull that fetches it, - * instead of delivering the old roles and recording the new revision as synced - * — which the fast path would then skip until `pull --force`. - */ -export async function refreshRoleAssignment(config: LocalConfig): Promise { - // Also covers an assignments file that just arrived, or just went away: the - // local selection is restored first, so absence hands the roles back to it. - return applyRoleAssignment(withLocalRoles(config)); + * Two callers: + * + * - `pull` loads the config before it updates the team repo, so the assignment + * it read is the one the previous pull left on disk. Re-resolving after the + * refresh is what makes a reassignment take effect on the pull that fetches + * it, instead of delivering the old roles and recording the new revision as + * synced — which the fast path would then skip until `pull --force`. + * - `init` builds the config in memory and hands it straight to the hook, MCP + * and env reconciliation, before any load can overlay it. Without this the + * config would reach them role-less, which matches every role-scoped entry. + * + * Mutates `config` rather than returning a new one. Pull keys `teamEnvs` and + * `instructions` on config identity and reconciles hooks, MCP and env on the + * object its caller holds, so a replacement would reach neither. + */ +export async function refreshRoleAssignment(config: LocalConfig): Promise { + // The local selection is restored first, so an assignments file that just + // arrived is applied over it — and one that just went away hands it back. + const resolved = await applyRoleAssignment(withLocalRoles(config)); + if (resolved.primaryRole) config.primaryRole = resolved.primaryRole; + else delete config.primaryRole; + config.additionalRoles = resolved.additionalRoles; + if (resolved.roleAssignment) config.roleAssignment = resolved.roleAssignment; + else delete config.roleAssignment; } /** `roles` as this member's effective selection, primary first. */ diff --git a/src/init.ts b/src/init.ts index c90b4cd87..b2c4a43c8 100644 --- a/src/init.ts +++ b/src/init.ts @@ -2,7 +2,7 @@ import YAML from 'yaml'; import fs from 'node:fs'; import path from 'node:path'; import { readWorkspaceChildren } from './workspace-children.js'; -import { saveLocalConfig, loadTeamConfig, saveLocalConfigForScope, loadLocalConfigForScope, loadStateForScope, saveStateForScope, resolveProjectDataHome } from './config.js'; +import { saveLocalConfig, loadTeamConfig, saveLocalConfigForScope, loadLocalConfigForScope, loadStateForScope, refreshRoleAssignment, saveStateForScope, resolveProjectDataHome } from './config.js'; import { deliveredHookFiles, describeUnappliedTeamHooks, hasTeamaiHooks, reconcileHooks, reconcileTeamHooksForConfig, reportCodexTrust, trustCodexForScope } from './hooks.js'; import { configureGitUser, initRepo, isGitRepo, getRemoteUrl, remotesMatch, redactGitCredentials, pullRepoFastForward, pushRepoDirectly, autoPushViaMR, initPushBlockTimeoutMs } from './utils/git.js'; import { withTimeout } from './utils/async.js'; @@ -727,6 +727,11 @@ export async function initHttp( const lenient = error instanceof RolesManifestNotFoundError || error instanceof NoRoleSelectedError; if (!lenient) throw error; } + // The team's assignment, if it has one (#1186). This config is handed to the + // hook, MCP and env reconciliation below before any load can overlay it, and + // a role-less config matches every role-scoped entry. Runtime-only: the save + // writes the member's own selection, which an assignment leaves empty. + await refreshRoleAssignment(localConfig); Object.assign(localConfig, await resolveProjectsForInit(localPath, options.project)); // Persist --agent into enabledAgents (additive across runs) @@ -1336,6 +1341,11 @@ export async function initSelfRepo(options: GlobalOptions & { const lenient = error instanceof RolesManifestNotFoundError || error instanceof NoRoleSelectedError; if (!lenient) throw error; } + // The team's assignment, if it has one (#1186). This config is handed to the + // hook, MCP and env reconciliation below before any load can overlay it, and + // a role-less config matches every role-scoped entry. Runtime-only: the save + // writes the member's own selection, which an assignment leaves empty. + await refreshRoleAssignment(localConfig); Object.assign(localConfig, await resolveProjectsForInit(localPath, options.project)); // Which AI tools to set up in this repo (create skills dir + inject hooks + // commit their settings.json). Resolved from --agent, else HOME detection @@ -2154,6 +2164,9 @@ export async function init(options: GlobalOptions & { } } if (roleProfile) Object.assign(localConfig, roleProfile); + // As above: the assignment reaches hook, MCP and env reconciliation through + // this config, and is never written to disk. + await refreshRoleAssignment(localConfig); // Projects were resolved before member registration so both records agree. localConfig.projects = resolvedProjects; diff --git a/src/pull.ts b/src/pull.ts index bb54aae98..a81d53e0e 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -1312,7 +1312,9 @@ async function pullForScope( // fetches it, rather than on the one after — by which time this pull has // recorded the new revision as synced and the fast path skips it. // Outside the refresh's try: a failure here is not a failure to fetch. - localConfig = await refreshRoleAssignment(localConfig); + // In place: the hooks, MCP and env stages after this scope reconcile the + // object `pull` holds, and key `teamEnvs`/`instructions` on its identity. + await refreshRoleAssignment(localConfig); // Settle the placement records against the tree just refreshed, before // delivery reads them: a placement whose PR has merged becomes a record, one From 69302b814c085b106a3b73a4e199c7debcaaba0f Mon Sep 17 00:00:00 2001 From: Nils Schiller <93717397+scpia@users.noreply.github.com> Date: Mon, 19 Oct 2026 13:48:51 +0200 Subject: [PATCH 4/6] fix(roles): clear a stale roleUnresolved on refresh (#1186) 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. --- src/__tests__/role-assignments.test.ts | 17 +++++++++++++++++ src/config.ts | 10 ++++++++-- 2 files changed, 25 insertions(+), 2 deletions(-) diff --git a/src/__tests__/role-assignments.test.ts b/src/__tests__/role-assignments.test.ts index e90721db6..3564bbb7b 100644 --- a/src/__tests__/role-assignments.test.ts +++ b/src/__tests__/role-assignments.test.ts @@ -254,6 +254,23 @@ describe('role assignments from manifest/members.yaml', () => { expect(config!.roleAssignment).toBeUndefined(); }); + // A broken roles manifest at load leaves a role-less config unresolved. The + // pull that fetches the fixed manifest and an assignment must apply it. + it('drops an unresolved role once the refreshed assignment resolves it', async () => { + writeRoles('version: 1\nroles: broken\n'); + writeConfig(); + vi.spyOn(log, 'warn').mockImplementation(() => {}); + const config = (await loadLocalConfig())!; + expect(config.roleUnresolved).toBe(true); + + writeRoles('version: 1\nroles:\n - id: frontend\n resources: { knowledge: [], skills: [frontend] }\n'); + writeAssignments('version: 1\nmembers:\n alice: [frontend]\n'); + await refreshRoleAssignment(config); + + expect(config.roleUnresolved).toBeUndefined(); + expect(activeRoleIds(config)).toEqual(['frontend']); + }); + // The legacy migration writes a `hai` role into a role-less config. A member // the team assigned nothing must not acquire one that way. it('does not let the legacy role migration write a role the team did not assign', async () => { diff --git a/src/config.ts b/src/config.ts index df34713cb..286cdbd21 100644 --- a/src/config.ts +++ b/src/config.ts @@ -186,8 +186,14 @@ export async function refreshRoleAssignment(config: LocalConfig): Promise if (resolved.primaryRole) config.primaryRole = resolved.primaryRole; else delete config.primaryRole; config.additionalRoles = resolved.additionalRoles; - if (resolved.roleAssignment) config.roleAssignment = resolved.roleAssignment; - else delete config.roleAssignment; + if (resolved.roleAssignment) { + config.roleAssignment = resolved.roleAssignment; + // The assignment decides the role the legacy migration could not, as it + // does on a load, where the migration then never runs. + delete config.roleUnresolved; + } else { + delete config.roleAssignment; + } } /** `roles` as this member's effective selection, primary first. */ From d525f64f9dc4615aa9106745c774024145c8fe78 Mon Sep 17 00:00:00 2001 From: Nils Schiller Date: Wed, 21 Oct 2026 10:14:34 +0200 Subject: [PATCH 5/6] fix(roles): load without the migration notice for roles list --json (#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. --- src/__tests__/roles-assigned-cmd.test.ts | 12 ++++++++++++ src/roles-cmd.ts | 5 +++-- 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/__tests__/roles-assigned-cmd.test.ts b/src/__tests__/roles-assigned-cmd.test.ts index ba5df48c3..6d5291855 100644 --- a/src/__tests__/roles-assigned-cmd.test.ts +++ b/src/__tests__/roles-assigned-cmd.test.ts @@ -101,6 +101,18 @@ describe('roles commands with assigned roles', () => { }); }); + it('loads without the migration notice for --json, which a hook parses', async () => { + given(assigned(['frontend'])); + + await rolesList({ json: true }); + await rolesList(); + + expect(vi.mocked(autoDetectInit).mock.calls.map(([, options]) => options)).toEqual([ + { dryRun: true, suppressMigrationNotice: true }, + { dryRun: true, suppressMigrationNotice: undefined }, + ]); + }); + it('reports a local selection as local, with no assignment file', async () => { given(configWith({ primaryRole: 'backend' })); diff --git a/src/roles-cmd.ts b/src/roles-cmd.ts index dad89d36a..204ebe0f3 100644 --- a/src/roles-cmd.ts +++ b/src/roles-cmd.ts @@ -182,8 +182,9 @@ export function assignedRolesNotice(effective: ReturnType } export async function rolesList(options: { json?: boolean } = {}): Promise { - // Read-only: the load never persists a migration (#893). - const { localConfig } = await autoDetectInit(undefined, { dryRun: true }); + // Read-only: the load never persists a migration (#893). Its preview notice + // would precede the JSON, so --json, read by hooks, omits it. + const { localConfig } = await autoDetectInit(undefined, { dryRun: true, suppressMigrationNotice: options.json }); const repoPath = localConfig.repo.localPath; const effective = effectiveRoles(localConfig); From 009d7bd1a36fc2167597151ac715049be10712fd Mon Sep 17 00:00:00 2001 From: Nils Schiller Date: Wed, 21 Oct 2026 11:19:23 +0200 Subject: [PATCH 6/6] feat(roles): read members.yaml from the fetched default branch (#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/ 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. --- docs/designs/assigned-roles.md | 1 + docs/guide/distribution.md | 2 +- docs/guide/zh-CN/distribution.md | 2 +- src/__tests__/e2e/assigned-roles-1186.test.ts | 17 +++ src/__tests__/role-assignments.test.ts | 112 ++++++++++++++++++ src/config.ts | 2 +- src/init.ts | 13 +- src/manifest-schema.ts | 15 ++- src/role-assignments.ts | 48 +++++++- 9 files changed, 191 insertions(+), 21 deletions(-) diff --git a/docs/designs/assigned-roles.md b/docs/designs/assigned-roles.md index 5005d7d07..fcc5b5ce2 100644 --- a/docs/designs/assigned-roles.md +++ b/docs/designs/assigned-roles.md @@ -37,6 +37,7 @@ With assignments in place, nothing falls back to an unfiltered sync: - `activeRoleIds` returns `[]`, not `null`: a member assigned no roles matches no role-scoped entry, where `null` means "no role filter". - `resolveOwnResourceNamespaces` keeps its three unfiltered fallbacks closed, so such a member receives the shared root only. - A `members.yaml` that exists but cannot be read or parsed leaves this run with no roles, loudly. Only absence of the file returns role selection to the member. +- The file is read from the team repo's fetched default branch (`origin/`), not from the clone's working tree. A local edit, a deletion or an unpushed commit in the clone changes nothing, and a copy only the working tree has is absent: only what the forge merged counts. Where no fetched default branch exists — an HTTP source, a repo without a remote — the working tree is all there is. - An assigned role `roles.yaml` does not declare is dropped with a warning, rather than failing every command for that member over one stale line. If that leaves no roles — including when `roles.yaml` is missing entirely — the member holds none. ### Resolved again where a load cannot reach diff --git a/docs/guide/distribution.md b/docs/guide/distribution.md index c663e237e..5234dfc19 100644 --- a/docs/guide/distribution.md +++ b/docs/guide/distribution.md @@ -215,7 +215,7 @@ The first role listed is the member's primary role, the rest are additional. Whi - A member the file does not list holds `default` — no roles unless you say otherwise — and receives shared resources only, rather than the unfiltered sync a role-less member gets without this file. - Who may assign roles is your forge's decision, as it already is for `roles.yaml`: a protected branch, a merge request, code owners for `manifest/`. teamai keeps no access list of its own. -The member key is the username in the member's own teamai config, which `teamai init` records from the git provider. Deleting the file hands role selection back to each member. +The member key is the username in the member's own teamai config, which `teamai init` records from the git provider. Deleting the file hands role selection back to each member. teamai reads it from the team repo's fetched default branch, not from the clone on the member's machine, so editing or deleting that copy changes nothing: only what your forge merged counts. A reassignment takes effect on the next pull. diff --git a/docs/guide/zh-CN/distribution.md b/docs/guide/zh-CN/distribution.md index 89f970997..6a50c7391 100644 --- a/docs/guide/zh-CN/distribution.md +++ b/docs/guide/zh-CN/distribution.md @@ -215,7 +215,7 @@ default: [] # 这个文件未列出的成员所持有的角色 - 文件未列出的成员持有 `default`——除非另行声明即为无角色——并且只接收共享资源，而不是没有该文件时无角色成员得到的全量同步。 - 谁可以分配角色由代码平台决定，和 `roles.yaml` 一样：保护分支、合并请求、`manifest/` 的 code owner。teamai 自己不维护任何权限名单。 -成员的键是该成员本地 teamai 配置里的用户名，由 `teamai init` 从 Git 平台记录。删除该文件后，角色选择重新回到每个成员手里。 +成员的键是该成员本地 teamai 配置里的用户名，由 `teamai init` 从 Git 平台记录。删除该文件后，角色选择重新回到每个成员手里。teamai 从团队仓库已拉取的默认分支读取该文件，而不是成员机器上的克隆副本，所以改写或删除本地副本不起作用：只有代码平台已合并的内容才算。 重新分配会在下一次 pull 时生效。 diff --git a/src/__tests__/e2e/assigned-roles-1186.test.ts b/src/__tests__/e2e/assigned-roles-1186.test.ts index 4d51a8a0e..3ff2279b6 100644 --- a/src/__tests__/e2e/assigned-roles-1186.test.ts +++ b/src/__tests__/e2e/assigned-roles-1186.test.ts @@ -215,6 +215,23 @@ describe('assigned roles (#1186)', () => { // namespace — both of these skills. `default: []` closes that. expect(deliveredSkills(projectRoot)).toEqual(['teamai']); }); + + // The file is read from origin/, so the clone's copy is not a way + // around the assignment: only what the forge merged counts. + it('ignores an edit to, or deletion of, the clone\'s members.yaml', async () => { + const projectRoot = project('alice-clone-edit', 'alice'); + const cloneCopy = path.join(projectRoot, '.teamai', 'team-repo', 'manifest', 'members.yaml'); + + fs.writeFileSync(cloneCopy, 'version: 1\nmembers:\n alice: [backend]\ndefault: []\n'); + const edited = await runCLI(['roles', 'list', '--json'], homeDir, projectRoot); + expect(JSON.parse(edited.output)).toMatchObject({ roles: ['frontend'], source: 'assigned' }); + + fs.rmSync(cloneCopy); + const deleted = await runCLI(['roles', 'list', '--json'], homeDir, projectRoot); + expect(JSON.parse(deleted.output)).toMatchObject({ roles: ['frontend'], source: 'assigned' }); + const set = await runCLI(['roles', 'set', 'backend'], homeDir, projectRoot); + expect(set.code, set.output).toBe(1); + }); }); /** diff --git a/src/__tests__/role-assignments.test.ts b/src/__tests__/role-assignments.test.ts index 3564bbb7b..0a9c61ee9 100644 --- a/src/__tests__/role-assignments.test.ts +++ b/src/__tests__/role-assignments.test.ts @@ -1,4 +1,5 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { execFileSync } from 'node:child_process'; import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, rmSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; @@ -284,3 +285,114 @@ describe('role assignments from manifest/members.yaml', () => { expect(YAML.parse(readFileSync(configPath, 'utf-8')).primaryRole).toBeUndefined(); }); }); + +/** + * Only what the forge merged counts: the file is read from `origin/` + * as last fetched, not from the clone's working tree, so an agent cannot lift + * the assignment by editing, deleting or committing to the clone's copy. + */ +describe('role assignments read from the fetched default branch (#1186)', () => { + let sandbox: string; + let upstream: string; + let clone: string; + + const ROLES = 'version: 1\nroles:\n - id: frontend\n resources: { knowledge: [], skills: [frontend] }\n - id: backend\n resources: { knowledge: [], skills: [backend] }\n'; + const ALICE_FRONTEND = 'version: 1\nmembers:\n alice: [frontend]\ndefault: []\n'; + const ALICE_BACKEND = 'version: 1\nmembers:\n alice: [backend]\ndefault: []\n'; + + function git(cwd: string, ...args: string[]): void { + execFileSync('git', args, { + cwd, + stdio: 'pipe', + env: { + ...process.env, + GIT_AUTHOR_NAME: 'TeamAI Test', GIT_AUTHOR_EMAIL: 'test@teamai.test', + GIT_COMMITTER_NAME: 'TeamAI Test', GIT_COMMITTER_EMAIL: 'test@teamai.test', + }, + }); + } + + function membersFile(repo: string): string { + return path.join(repo, 'manifest', 'members.yaml'); + } + + beforeEach(() => { + resetWarnOnce(); + sandbox = mkdtempSync(path.join(os.tmpdir(), 'teamai-assigned-roles-ref-')); + upstream = path.join(sandbox, 'upstream'); + clone = path.join(sandbox, 'clone'); + mkdirSync(path.join(upstream, 'manifest'), { recursive: true }); + writeFileSync(path.join(upstream, 'manifest', 'roles.yaml'), ROLES, 'utf-8'); + writeFileSync(membersFile(upstream), ALICE_FRONTEND, 'utf-8'); + git(upstream, 'init', '-q', '-b', 'main'); + git(upstream, 'add', '-A'); + git(upstream, 'commit', '-q', '-m', 'fixture'); + git(sandbox, 'clone', '-q', upstream, clone); + }); + + afterEach(() => { + rmSync(sandbox, { recursive: true, force: true }); + }); + + async function rolesOf(repo: string, username: string): Promise { + return resolveAssignedRoles(await loadRoleAssignments(repo), username).roles; + } + + it('ignores an edit to the clone\'s working tree', async () => { + writeFileSync(membersFile(clone), ALICE_BACKEND, 'utf-8'); + expect(await rolesOf(clone, 'alice')).toEqual(['frontend']); + }); + + it('ignores a deletion from the clone\'s working tree', async () => { + rmSync(membersFile(clone)); + expect(await rolesOf(clone, 'alice')).toEqual(['frontend']); + }); + + it('ignores a commit the clone has not pushed', async () => { + writeFileSync(membersFile(clone), ALICE_BACKEND, 'utf-8'); + git(clone, 'commit', '-qam', 'self-assign'); + expect(await rolesOf(clone, 'alice')).toEqual(['frontend']); + }); + + it('treats a file only the working tree has as absent', async () => { + git(upstream, 'rm', '-q', 'manifest/members.yaml'); + git(upstream, 'commit', '-qm', 'no assignments'); + git(clone, 'pull', '-q'); + writeFileSync(membersFile(clone), ALICE_BACKEND, 'utf-8'); + expect(await loadRoleAssignmentsIfPresent(clone)).toBeNull(); + }); + + it('reads what the last fetch brought, whatever the clone has checked out', async () => { + writeFileSync(membersFile(upstream), ALICE_BACKEND, 'utf-8'); + git(upstream, 'commit', '-qam', 'reassign alice'); + git(clone, 'fetch', '-q'); + expect(await rolesOf(clone, 'alice')).toEqual(['backend']); + }); + + it('refuses an empty file on the default branch as invalid, not absent', async () => { + writeFileSync(membersFile(upstream), '', 'utf-8'); + git(upstream, 'commit', '-qam', 'empty'); + git(clone, 'fetch', '-q'); + await expect(loadRoleAssignments(clone)).rejects.toThrow(/is empty/); + }); + + // An HTTP source is materialized files inside whatever directory holds the + // config — a project checkout, say. Git would answer for that repository. + it('reads the working tree of an HTTP source, whatever repository it sits in', async () => { + const http = path.join(clone, 'http-team-repo'); + mkdirSync(path.join(http, 'manifest'), { recursive: true }); + writeFileSync(membersFile(http), ALICE_BACKEND, 'utf-8'); + expect(resolveAssignedRoles(await loadRoleAssignments(http, { kind: 'http' }), 'alice').roles).toEqual(['backend']); + // The same directory probed as git-backed reads the enclosing clone's branch, where it is absent. + expect(await loadRoleAssignmentsIfPresent(http)).toBeNull(); + }); + + // A repo with no remote has nothing the forge merged to prefer. + it('reads the working tree where no fetched default branch exists', async () => { + const local = path.join(sandbox, 'local'); + mkdirSync(path.join(local, 'manifest'), { recursive: true }); + git(local, 'init', '-q', '-b', 'main'); + writeFileSync(membersFile(local), ALICE_BACKEND, 'utf-8'); + expect(await rolesOf(local, 'alice')).toEqual(['backend']); + }); +}); diff --git a/src/config.ts b/src/config.ts index 286cdbd21..69cade9bd 100644 --- a/src/config.ts +++ b/src/config.ts @@ -114,7 +114,7 @@ async function applyRoleAssignment(config: LocalConfig): Promise { let assignments; try { - assignments = await loadRoleAssignmentsIfPresent(config.repo.localPath); + assignments = await loadRoleAssignmentsIfPresent(config.repo.localPath, config.repo); } catch (error) { warnOnce(`${ROLE_ASSIGNMENTS_FILE} could not be read: ${(error as Error).message}`); warnOnce(`Roles are assigned in ${ROLE_ASSIGNMENTS_FILE} while that file exists, so this run holds none. ` diff --git a/src/init.ts b/src/init.ts index 053c915f0..33c6b92e5 100644 --- a/src/init.ts +++ b/src/init.ts @@ -235,13 +235,14 @@ function parseRoleSelection(answer: string, max: number): number[] { class NoRoleSelectedError extends Error {} async function promptForRoleProfile( - repoPath: string, + repo: { localPath: string; kind?: string }, username: string, roleFlag?: string, ): Promise> { + const repoPath = repo.localPath; // The team assigns roles (#1186): there is nothing to pick, and nothing to // write — every load reads the assignment from the team repo. - const assignments = await loadRoleAssignmentsIfPresent(repoPath); + const assignments = await loadRoleAssignmentsIfPresent(repoPath, repo); if (assignments) { if (roleFlag) { throw new Error( @@ -718,7 +719,7 @@ export async function initHttp( ...(inheritUserScope !== undefined ? { inheritUserScope } : {}), }; try { - Object.assign(localConfig, await promptForRoleProfile(localPath, localConfig.username, options.role)); + Object.assign(localConfig, await promptForRoleProfile(localConfig.repo, localConfig.username, options.role)); } catch (error) { // Two cases leave the role unset on purpose: a repo with no roles manifest, // and a person who skipped the prompt. Anything else — a manifest that does @@ -1331,7 +1332,7 @@ export async function initSelfRepo(options: GlobalOptions & { ...(inheritUserScope !== undefined ? { inheritUserScope } : {}), }; try { - Object.assign(localConfig, await promptForRoleProfile(localPath, localConfig.username, options.role)); + Object.assign(localConfig, await promptForRoleProfile(localConfig.repo, localConfig.username, options.role)); } catch (error) { // Two cases leave the role unset on purpose: a repo with no roles manifest, // and a person who skipped the prompt. Anything else — a manifest that does @@ -1969,7 +1970,7 @@ export async function init(options: GlobalOptions & { if (projectsManifest?.projects.length) { rolePromptedEarly = true; try { - roleProfile = await promptForRoleProfile(localPath, username, options.role); + roleProfile = await promptForRoleProfile({ localPath }, username, options.role); } catch (error) { const msg = (error as Error).message; if (msg.includes('Roles manifest not found')) { @@ -2156,7 +2157,7 @@ export async function init(options: GlobalOptions & { if (!rolePromptedEarly) { try { - roleProfile = await promptForRoleProfile(localPath, username, options.role); + roleProfile = await promptForRoleProfile({ localPath }, username, options.role); } catch (error) { const msg = (error as Error).message; if (msg.includes('Roles manifest not found')) { diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index e8a483017..52413d6b8 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -222,15 +222,18 @@ export async function readManifestFile(manifestPath: string, kind: ManifestKind) } throw new Error(`The ${kind} manifest ${resolvedPath} could not be read: ${(error as Error).message}. Make it a readable file, then retry.`); } - if (content.trim() === '') { - const consequence = kind === 'members' - ? 'deleting it instead hands role selection back to each member' - : `deleting it instead turns ${kind} filtering off for the whole team`; - throw new Error(`Invalid ${kind} manifest: ${resolvedPath} is empty. Give it a version and a ${kind} list; ${consequence}.`); - } + if (content.trim() === '') throw emptyManifestError(kind, resolvedPath); return content; } +/** An empty manifest is invalid, not absent: absence is the only thing that relaxes anything. */ +export function emptyManifestError(kind: ManifestKind, where: string): Error { + const consequence = kind === 'members' + ? 'deleting it instead hands role selection back to each member' + : `deleting it instead turns ${kind} filtering off for the whole team`; + return new Error(`Invalid ${kind} manifest: ${where} is empty. Give it a version and a ${kind} list; ${consequence}.`); +} + const parsedManifests = new Map(); /** diff --git a/src/role-assignments.ts b/src/role-assignments.ts index b806a0268..5297cb0a5 100644 --- a/src/role-assignments.ts +++ b/src/role-assignments.ts @@ -1,7 +1,8 @@ import path from 'node:path'; import YAML from 'yaml'; import { z } from 'zod'; -import { caseFoldKey, parseManifest, parseManifestOnce, readManifestFile } from './manifest-schema.js'; +import { caseFoldKey, emptyManifestError, parseManifest, parseManifestOnce, readManifestFile } from './manifest-schema.js'; +import { getDefaultBranch, getFileContentAtRev, getHeadCommit } from './utils/git.js'; /** Where a team declares who holds which role. Relative to the team repo root. */ export const ROLE_ASSIGNMENTS_FILE = 'manifest/members.yaml'; @@ -78,10 +79,45 @@ function validateAssignments(raw: unknown): RoleAssignments { return manifest; } -export async function loadRoleAssignments(repoPath: string): Promise { +/** + * The fetched default branch of a git-backed team repo, as a ref, or `null` + * where there is none to read: an HTTP source, a repo with no remote, a clone + * of an empty remote, a path git cannot open. + */ +async function fetchedDefaultBranch(repoPath: string): Promise { + try { + const tip = `origin/${await getDefaultBranch(repoPath)}`; + return (await getHeadCommit(repoPath, tip)) ? tip : null; + } catch { + return null; + } +} + +/** + * The assignments are read from what the forge merged — `origin/` as + * last fetched — not from the clone's working tree. A local edit, a deletion + * or an unpushed commit in the clone changes nothing, and a `members.yaml` + * that only the working tree has is absent. Where no fetched default branch + * exists, the working tree is all there is. + */ +export async function loadRoleAssignments(repoPath: string, repo: { kind?: string } = {}): Promise { const manifestPath = path.join(repoPath, ...ROLE_ASSIGNMENTS_FILE.split('/')); - const content = await readManifestFile(manifestPath, 'members'); - if (content === null) throw new RoleAssignmentsNotFoundError(manifestPath); + // An HTTP source is materialized files with no git of its own; a probe there + // would find whatever repository the directory sits in, such as the project's. + const tip = repo.kind === 'http' ? null : await fetchedDefaultBranch(repoPath); + let content: string | null; + let where: string; + if (tip) { + where = `${ROLE_ASSIGNMENTS_FILE} at ${tip}`; + // `./`: relative to the directory git runs in, which in single-repo mode + // is `.teamai/` inside the business repo rather than its root. + content = (await getFileContentAtRev(repoPath, tip, `./${ROLE_ASSIGNMENTS_FILE}`))?.toString('utf-8') ?? null; + if (content !== null && content.trim() === '') throw emptyManifestError('members', where); + } else { + where = manifestPath; + content = await readManifestFile(manifestPath, 'members'); + } + if (content === null) throw new RoleAssignmentsNotFoundError(where); return parseManifestOnce(manifestPath, content, (text) => { let raw: unknown; @@ -98,9 +134,9 @@ export async function loadRoleAssignments(repoPath: string): Promise { +export async function loadRoleAssignmentsIfPresent(repoPath: string, repo: { kind?: string } = {}): Promise { try { - return await loadRoleAssignments(repoPath); + return await loadRoleAssignments(repoPath, repo); } catch (error) { if (error instanceof RoleAssignmentsNotFoundError) return null; throw error;