From 34c32b326dc39b2776afbd32588db945e8b61da2 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Mon, 21 Sep 2026 22:11:17 +0200 Subject: [PATCH 01/25] fix(manifest): reject namespace strings that are not safe path segments A resource namespace becomes a directory component (skills//, agents//, learnings//) exactly as a project id does, but only the project id was refined. Both manifests accepted a namespace like '../../evil', and roles.yaml had the same hole. Guarded at the manifest boundary, which is where projects.ts already claims it is enforced and the only place these strings enter the process. The existing isSafeNamespaceSegment guards in contribute.ts and resources/agents.ts stay as defence in depth. The doc comment pointed at the wrong layer: an id read from a hand-edited config.yaml resolves through getProjectOrThrow, so it can only ever name a project the manifest already validated. Corrected to say so. No fixture or e2e manifest in the repo ships a namespace containing '/' or '..', so nothing that parses today stops parsing. --- src/__tests__/projects.test.ts | 36 +++++++++++++++++++++++++++++ src/__tests__/roles.test.ts | 18 +++++++++++++++ src/projects.ts | 41 +++++++++++++++++++++++----------- src/roles.ts | 11 ++++++--- 4 files changed, 90 insertions(+), 16 deletions(-) diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index 474b40f6..740ede08 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -143,6 +143,42 @@ projects: } }); + it('rejects a resource namespace that is not a safe path segment (traversal guard)', async () => { + // A namespace becomes a directory component (skills//, agents//) just + // as a project id does, so the boundary has to guard both. + for (const type of ['knowledge', 'skills', 'learnings', 'agents']) { + for (const badNamespace of ['../../evil', 'a/b', '..', 'x\\y']) { + const repoDir = writeManifest(` +version: 1 +projects: + - id: x + resources: { ${type}: ['${badNamespace}'] } +`); + try { + await expect(loadProjectsManifest(repoDir)).rejects.toThrow(/single path segment/i); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + } + } + }); + + it('keeps accepting namespaces that differ from the project id', async () => { + const repoDir = writeManifest(` +version: 1 +projects: + - id: alpha + resources: { learnings: [alpha-notes], skills: [alpha.v2, alpha_shared] } +`); + try { + const manifest = await loadProjectsManifest(repoDir); + expect(manifest?.projects[0].resources.learnings).toEqual(['alpha-notes']); + expect(manifest?.projects[0].resources.skills).toEqual(['alpha.v2', 'alpha_shared']); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('round-trips through save', async () => { const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-projsave-')); try { diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index f6b33a2c..7153f8e4 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -123,6 +123,24 @@ roles: rmSync(repoDir, { recursive: true, force: true }); }); + it('fails when a resource namespace is not a safe path segment (traversal guard)', async () => { + // Role namespaces become directory components (skills//, agents//) + // exactly as project namespaces do, so the same boundary guard applies. + for (const badNamespace of ['../../evil', 'a/b', '..', 'x\\y']) { + const repoDir = writeManifest(` +version: 1 +roles: + - id: hai + resources: + knowledge: [] + skills: ['${badNamespace}'] +`); + + await expect(loadRolesManifest(repoDir)).rejects.toThrow(/single path segment/i); + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('fails when duplicate role ids are declared', async () => { const repoDir = writeManifest(` version: 1 diff --git a/src/projects.ts b/src/projects.ts index 4f78f9fe..ab1607ca 100644 --- a/src/projects.ts +++ b/src/projects.ts @@ -13,18 +13,17 @@ const PROJECT_RESOURCE_TYPES = ['knowledge', 'skills', 'learnings', 'agents'] as export type ProjectResourceType = typeof PROJECT_RESOURCE_TYPES[number]; -const ProjectResourceNamespacesSchema = z.object({ - knowledge: z.array(z.string().min(1)).default([]), - skills: z.array(z.string().min(1)).default([]), - learnings: z.array(z.string().min(1)).default([]), - agents: z.array(z.string().min(1)).default([]), -}); - /** - * A project id becomes a path component (skills//, learnings//), so it - * must never contain a path separator or `..`. Enforced here at the manifest - * boundary; use-sites that read ids from other sources (e.g. a hand-edited - * config.yaml `projects` field) additionally guard via `isSafeNamespaceSegment`. + * A project id and every resource namespace become path components + * (`skills//`, `learnings//`), so neither may contain a path + * separator or `..`. + * + * Both are enforced here at the manifest boundary, which is the only place they + * enter the process: an id read from elsewhere (a hand-edited config.yaml + * `projects` field) is resolved through `getProjectOrThrow`, so it can only ever + * name a project this manifest already validated. `contribute.ts` and + * `resources/agents.ts` keep their own `isSafeNamespaceSegment` guards on the + * resolved namespace as defence in depth. */ const SAFE_ID = /^[A-Za-z0-9._-]+$/; @@ -33,9 +32,25 @@ export function isSafeNamespaceSegment(seg: string): boolean { return SAFE_ID.test(seg) && seg !== '.' && seg !== '..'; } +/** Message shared by the id and namespace guards, so both read the same. */ +export const SAFE_SEGMENT_MESSAGE = + "must be a single path segment (letters, digits, '.', '_', '-'; no '/', '\\\\', or '..')"; + +/** A resource namespace: one safe path segment. */ +export const NamespaceSegmentSchema = z.string().min(1).refine(isSafeNamespaceSegment, { + message: `resource namespace ${SAFE_SEGMENT_MESSAGE}`, +}); + +const ProjectResourceNamespacesSchema = z.object({ + knowledge: z.array(NamespaceSegmentSchema).default([]), + skills: z.array(NamespaceSegmentSchema).default([]), + learnings: z.array(NamespaceSegmentSchema).default([]), + agents: z.array(NamespaceSegmentSchema).default([]), +}); + const ProjectSchema = z.object({ - id: z.string().min(1).refine((v) => isSafeNamespaceSegment(v), { - message: "project id must be a single path segment (letters, digits, '.', '_', '-'; no '/', '\\\\', or '..')", + id: z.string().min(1).refine(isSafeNamespaceSegment, { + message: `project id ${SAFE_SEGMENT_MESSAGE}`, }), name: z.string().default(''), description: z.string().default(''), diff --git a/src/roles.ts b/src/roles.ts index 143a7ab9..e3ddb18c 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -2,17 +2,22 @@ import path from 'node:path'; import YAML from 'yaml'; import { z } from 'zod'; import { readFileSafe, readFileIfExists, ensureDir, writeFile } from './utils/fs.js'; +import { log } from './utils/logger.js'; +// A role namespace is the same kind of path component a project namespace is, so +// both manifests guard it with one schema. projects.ts imports only a *type* +// from here, so this direction adds no runtime cycle. +import { NamespaceSegmentSchema } from './projects.js'; const ROLE_RESOURCE_TYPES = ['knowledge', 'skills', 'agents'] as const; export type RoleResourceType = typeof ROLE_RESOURCE_TYPES[number]; const RoleResourceNamespacesSchema = z.object({ - knowledge: z.array(z.string().min(1)), - skills: z.array(z.string().min(1)), + knowledge: z.array(NamespaceSegmentSchema), + skills: z.array(NamespaceSegmentSchema), // Optional: a role without `agents` receives root-level agents only, which // is what every manifest written before this key existed already got. - agents: z.array(z.string().min(1)).default([]), + agents: z.array(NamespaceSegmentSchema).default([]), // learnings is accepted for backward compatibility but ignored at runtime. // All learnings are shared flat across the entire team (no namespace isolation). learnings: z.array(z.string()).optional(), From af2db676370ce8817ac995e8a6ab0f333e0cdd56 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 07:29:45 +0200 Subject: [PATCH 02/25] docs(changelog): note the manifest namespace guard --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index fba2c6da..6a4a2dab 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,7 @@ All notable changes to this project will be documented in this file. See [standa ### 🐛 Bug Fixes - `teamai init` no longer hangs without a terminal. When the provider had no session it spawned `gh auth login --web` (or `gf auth login`, `cnb login`) with inherited stdio and waited for a browser device flow that nobody could complete, about five minutes for GitHub, then exited with the provider's error and no hint of the missing credential. Each login now refuses up front when the run is not interactive and names the credential to prepare (`GITHUB_TOKEN` / `GH_TOKEN`, `CNB_TOKEN`, or for TGit a prior `gf auth login`, since a `TGIT_TOKEN` PAT is REST-API-only and cannot clone). A run is non-interactive when stdin is not a TTY or when `CI` or `TEAMAI_NONINTERACTIVE` is set, so an agent sandbox with a pseudo-terminal can declare itself unattended, and every prompt in the CLI follows the same rule. `git` also runs with its prompts closed in that case — `GIT_TERMINAL_PROMPT=0`, `GIT_ASKPASS=echo` and `GCM_INTERACTIVE=never`, each only where the caller set nothing — so a missing clone credential fails at once instead of waiting on a terminal prompt or an askpass or credential-manager dialog. `ssh` keeps its own settings: its batch flag is only reachable through `GIT_SSH_COMMAND`, which would override each repository's `core.sshCommand` (for [#711](https://github.com/Tencent/teamai-cli/issues/711)). +- `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the same guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil` or `a/b` under `resources:` no longer parses; the error names the offending entry. A manifest whose namespaces are plain names is unaffected. - Cache GC now rejects partial integers such as `12abc`, decimals, zero and unsafe integers for `--max-bytes` and `--stale-days` before deleting anything. An invalid `TEAMAI_CACHE_MAX_BYTES` value falls back to the default 5 GB limit instead of using a numeric prefix. - Usage reporting scopes sessions by path on Windows too. The project/user scope filter compared an event's `cwd` against `projectRoot` with a hard-coded `/` separator, so on Windows only a session started in the project root itself matched: every session started in a subdirectory was dropped from the project team's report and counted in the user scope's instead, which is the isolation the usage guide promises. Windows paths are also compared case-insensitively, so a drive letter or a directory name spelled with different case in the two sources no longer leaks a project session into the user scope. POSIX paths keep their own rules: case-sensitive, and a `\` in a filename stays part of the name. - `teamai tags subscribe` and `teamai tags unsubscribe` now invalidate the pull revision cache, as `teamai skill exclude` already does, so the next `teamai pull` applies the new subscriptions instead of reporting "Already synced" when the team repo has not changed. From 79fb8a459fd3f80cc4efb7e77461c6d5543adcde Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 12:31:44 +0200 Subject: [PATCH 03/25] fix(manifest): guard namespaces against traversal only, and say what is wrong Review findings on #710. The first cut reused SAFE_ID (^[A-Za-z0-9._-]+$) for resource namespaces. That allowlist is right for a project id, which is also typed on the command line and split on commas, but for a namespace it rejects far more than traversal: a team whose skills live under a non-ASCII directory, or one with a space in the name, would have stopped parsing although the directory is perfectly safe. The namespace guard now tests what actually matters -- no path separator, no `:` (drive-relative on Windows), no control character, and not `.` or `..` -- while the project id keeps its narrower spelling. Both live in src/manifest-schema.ts, which is what roles.yaml and projects.yaml genuinely share; roles.ts no longer reaches into projects.ts for the schema. Running the CLI against a manifest with `../../evil` showed the second half: the zod failure escaped as a raw ZodError, so `teamai pull` printed a validation object instead of a sentence. parseManifest now reports it the way the hand-written checks beside it do, naming the entry: Invalid projects manifest: projects.0.resources.skills.1: resource namespace must be a single path segment (no '/', '\', ':' or control characters, and not '.' or '..') Docs: the namespace rule is stated where each manifest is documented, in both usage guides and in the multi-project design doc. --- CHANGELOG.md | 2 +- docs/designs/multi-project-management.md | 6 ++++ docs/usage-guide.md | 12 +++++++ docs/usage-guide.zh-CN.md | 11 +++++++ src/__tests__/projects.test.ts | 42 +++++++++++++++++++++++- src/__tests__/roles.test.ts | 18 +++++++++- src/contribute.ts | 3 +- src/manifest-schema.ts | 41 +++++++++++++++++++++++ src/projects.ts | 37 +++++++++------------ src/resources/agents.ts | 2 +- src/roles.ts | 7 ++-- 11 files changed, 149 insertions(+), 32 deletions(-) create mode 100644 src/manifest-schema.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a4a2dab..ec767eb8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ All notable changes to this project will be documented in this file. See [standa ### 🐛 Bug Fixes - `teamai init` no longer hangs without a terminal. When the provider had no session it spawned `gh auth login --web` (or `gf auth login`, `cnb login`) with inherited stdio and waited for a browser device flow that nobody could complete, about five minutes for GitHub, then exited with the provider's error and no hint of the missing credential. Each login now refuses up front when the run is not interactive and names the credential to prepare (`GITHUB_TOKEN` / `GH_TOKEN`, `CNB_TOKEN`, or for TGit a prior `gf auth login`, since a `TGIT_TOKEN` PAT is REST-API-only and cannot clone). A run is non-interactive when stdin is not a TTY or when `CI` or `TEAMAI_NONINTERACTIVE` is set, so an agent sandbox with a pseudo-terminal can declare itself unattended, and every prompt in the CLI follows the same rule. `git` also runs with its prompts closed in that case — `GIT_TERMINAL_PROMPT=0`, `GIT_ASKPASS=echo` and `GCM_INTERACTIVE=never`, each only where the caller set nothing — so a missing clone credential fails at once instead of waiting on a terminal prompt or an askpass or credential-manager dialog. `ssh` keeps its own settings: its batch flag is only reachable through `GIT_SSH_COMMAND`, which would override each repository's `core.sshCommand` (for [#711](https://github.com/Tencent/teamai-cli/issues/711)). -- `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the same guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil` or `a/b` under `resources:` no longer parses; the error names the offending entry. A manifest whose namespaces are plain names is unaffected. +- `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil`, `a/b`, `C:evil` or a bare `..` under `resources:` no longer parses; the error names the offending entry. Only traversal is rejected: a namespace that is a plain directory name still parses, non-ASCII names and names with a space included. A manifest that fails to parse now reports the offending entry on one line (`Invalid projects manifest: projects.0.resources.skills.1: ...`) instead of dumping a raw validation object. - Cache GC now rejects partial integers such as `12abc`, decimals, zero and unsafe integers for `--max-bytes` and `--stale-days` before deleting anything. An invalid `TEAMAI_CACHE_MAX_BYTES` value falls back to the default 5 GB limit instead of using a numeric prefix. - Usage reporting scopes sessions by path on Windows too. The project/user scope filter compared an event's `cwd` against `projectRoot` with a hard-coded `/` separator, so on Windows only a session started in the project root itself matched: every session started in a subdirectory was dropped from the project team's report and counted in the user scope's instead, which is the isolation the usage guide promises. Windows paths are also compared case-insensitively, so a drive letter or a directory name spelled with different case in the two sources no longer leaks a project session into the user scope. POSIX paths keep their own rules: case-sensitive, and a `\` in a filename stays part of the name. - `teamai tags subscribe` and `teamai tags unsubscribe` now invalidate the pull revision cache, as `teamai skill exclude` already does, so the next `teamai pull` applies the new subscriptions instead of reporting "Already synced" when the team repo has not changed. diff --git a/docs/designs/multi-project-management.md b/docs/designs/multi-project-management.md index 308788a7..60380498 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -81,6 +81,12 @@ projects: agents: [hai-inference] # optional; agents// scoped to this project ``` +The id and every namespace are refused at the manifest boundary unless they are a +single path segment (no `/`, `\`, `:` or control character, and not `.` or `..`), +since each becomes a directory component. The id is narrower still — letters, +digits, `.`, `_`, `-` — because it is also typed on the command line and split on +commas. The same guard applies to `manifest/roles.yaml`. + Agent push uses the same role/project namespace resolution as pull and skips ambiguous source destinations. On a role or project change, agent cleanup checks each tool destination independently, including YAML `targets` and legacy format support. Locally edited copies are preserved. Directory layout reuses the existing namespace convention, adding one learnings layer: diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 0718a285..8131f4d7 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -243,6 +243,14 @@ projects: agents: [hai-inference] # optional ``` +The project id and every namespace under `resources:` become a directory name +(`skills//`, `learnings//`, `agents//`), so each +must be a single path segment: no `/`, `\`, `:` or control character, and not `.` +or `..`. A namespace is otherwise free — a non-ASCII name or one with a space is +still a valid directory. A project id is narrower, because it is also typed on +the command line: letters, digits, `.`, `_` and `-`. A manifest that breaks +either rule fails to parse, and the error names the offending entry. + **Commands** (low-frequency correction/query, mirroring `teamai roles …`): ```bash @@ -1479,6 +1487,10 @@ roles: agents: [common, frontend] # optional; omitted = root-level agents only ``` +Every namespace listed under `resources:` becomes a directory name, so it must be +a single path segment — no `/`, `\`, `:` or control character, and not `.` or +`..` — in `manifest/roles.yaml` exactly as in `manifest/projects.yaml`. + `teamai pull` copies these into each Tier-1 tool's `agents/` directory (e.g. `~/.claude/agents/`), flattened by file name, so two active namespaces must not define the same agent name (pull reports the collision and skips the scope). `teamai pull` writes `.toml` for Codex tools, `.json` for Kiro, `.agent.md` for Copilot, and `.md` for every other tool. When a member changes role, agents of the namespaces that stopped being active are removed on the next pull, unless the deployed copy was edited locally, in which case it is kept with a warning. Without a configured role, every agent syncs. `teamai push` resolves the source using the same active role and project namespaces as pull. It writes edits to that source and skips ambiguous destinations with a warning; an agent with only inactive sources is also skipped. Skipped agents do not block other resources in the same push. A new agent lands at the root. Cleanup checks each tool separately, respecting YAML `targets` and legacy format support. An active same-named agent protects a deployed file only when it targets that tool and output file. `teamai remove agents ` records a tombstone. The next pull on every other machine deletes `.agent.md`, `.md`, `.toml` and `.json` from each synced tool's agents directory. That cleanup also runs when the pull finds the team repo unchanged. The CLI's built-in `teamai-recall` profile is deployed alongside team agents but is not uploaded by `teamai push`. ### GitHub Copilot CLI diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 5aa56d40..5860861c 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -228,6 +228,13 @@ projects: agents: [hai-inference] # 可选 ``` +项目 id 与 `resources:` 下的每个 namespace 都会成为目录名 +（`skills//`、`learnings//`、`agents//`），因此 +必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，且不能是 `.` 或 `..`。除此 +之外 namespace 不受限制 —— 非 ASCII 名称或含空格的名称仍是合法目录。项目 id 更 +严格，因为它还会在命令行中输入：只允许字母、数字、`.`、`_` 和 `-`。违反任一规则 +的 manifest 会解析失败，错误信息会指出具体条目。 + **命令**（低频的事后修正与查询，对标 `teamai roles …`）： ```bash @@ -1439,6 +1446,10 @@ roles: agents: [common, frontend] # 可选；省略 = 只同步根目录 agents ``` +`resources:` 下列出的每个 namespace 都会成为目录名，因此必须是单个路径片段 —— +不含 `/`、`\`、`:` 和控制字符，且不能是 `.` 或 `..` —— `manifest/roles.yaml` 与 +`manifest/projects.yaml` 规则一致。 + `teamai pull` 会将它们按文件名拍平复制到每个 Tier-1 工具的 `agents/` 目录（如 `~/.claude/agents/`），因此两个活跃 namespace 不能定义同名 agent（pull 会报告冲突并跳过该 scope）。`teamai pull` 为 Codex 系工具写入 `.toml`，为 Kiro 写入 `.json`，为 Copilot 写入 `.agent.md`，其余工具写入 `.md`。成员切换角色后，不再活跃的 namespace 中的 agents 会在下一次 pull 时被移除；若本地副本已被手动修改，则保留并给出警告。未配置角色时同步全部 agents。`teamai push` 使用与 pull 相同的活跃角色和项目 namespace 来确定源文件，并将修改写回该源文件；若存在多个候选目标，则跳过并给出警告。若源文件均不活跃，也会跳过。跳过的 agent 不会阻止同一次 push 中的其他资源。新 agent 落在根目录。清理会逐个工具检查 YAML 的 `targets` 和旧格式支持；只有活跃的同名 agent 会写入该工具的同一输出文件时，才保留该文件。`teamai remove agents ` 会记录 tombstone。其他机器下一次 pull 时，会从每个同步中的工具的 agents 目录删除 `.agent.md`、`.md`、`.toml` 和 `.json`。即使该次 pull 发现团队仓库没有变化，也会执行清理。CLI 内置的 `teamai-recall` 配置与团队 agents 并列部署，但不会被 `teamai push` 上传。 ### GitHub Copilot CLI diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index 740ede08..fa1ebaec 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -147,7 +147,7 @@ projects: // A namespace becomes a directory component (skills//, agents//) just // as a project id does, so the boundary has to guard both. for (const type of ['knowledge', 'skills', 'learnings', 'agents']) { - for (const badNamespace of ['../../evil', 'a/b', '..', 'x\\y']) { + for (const badNamespace of ['../../evil', 'a/b', '..', '.', 'x\\y', 'C:evil']) { const repoDir = writeManifest(` version: 1 projects: @@ -179,6 +179,46 @@ projects: } }); + it('names the offending entry instead of dumping a raw ZodError', async () => { + const repoDir = writeManifest(` +version: 1 +projects: + - id: alpha + resources: { skills: [good, '../evil'] } +`); + try { + await expect(loadProjectsManifest(repoDir)).rejects.toThrow( + /^Invalid projects manifest: projects\.0\.resources\.skills\.1: resource namespace must be a single path segment/, + ); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + + it('accepts a namespace that is an unusual but traversal-free directory name', async () => { + // The guard is about escaping the parent directory, not about spelling: a + // namespace a filesystem accepts as one directory keeps parsing, so a team + // whose namespaces are non-ASCII or hold a space is not forced to rename. + const repoDir = writeManifest(` +version: 1 +projects: + - id: alpha + resources: + skills: ["\u7814\u53d1", "team frontend", "team@frontend", "..notes"] +`); + try { + const manifest = await loadProjectsManifest(repoDir); + expect(manifest?.projects[0].resources.skills).toEqual([ + '\u7814\u53d1', + 'team frontend', + 'team@frontend', + '..notes', + ]); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('round-trips through save', async () => { const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-projsave-')); try { diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index 7153f8e4..061b36ef 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -126,7 +126,7 @@ roles: it('fails when a resource namespace is not a safe path segment (traversal guard)', async () => { // Role namespaces become directory components (skills//, agents//) // exactly as project namespaces do, so the same boundary guard applies. - for (const badNamespace of ['../../evil', 'a/b', '..', 'x\\y']) { + for (const badNamespace of ['../../evil', 'a/b', '..', '.', 'x\\y', 'C:evil']) { const repoDir = writeManifest(` version: 1 roles: @@ -141,6 +141,22 @@ roles: } }); + it('names the offending entry instead of dumping a raw ZodError', async () => { + const repoDir = writeManifest(` +version: 1 +roles: + - id: hai + resources: + knowledge: [] + skills: ['a/b'] +`); + + await expect(loadRolesManifest(repoDir)).rejects.toThrow( + /^Invalid roles manifest: roles\.0\.resources\.skills\.0: resource namespace must be a single path segment/, + ); + rmSync(repoDir, { recursive: true, force: true }); + }); + it('fails when duplicate role ids are declared', async () => { const repoDir = writeManifest(` version: 1 diff --git a/src/contribute.ts b/src/contribute.ts index c3ea10de..53a45818 100644 --- a/src/contribute.ts +++ b/src/contribute.ts @@ -8,7 +8,8 @@ import { markContributed } from './contribute-check.js'; import { pendingLearningsDir, savePendingLearning } from './utils/pending-learnings.js'; import { publishQueuedLearnings } from './utils/learnings-publish.js'; import { learningsRoots } from './utils/learnings-roots.js'; -import { isSafeNamespaceSegment, resolveActiveLearningsNamespaces } from './projects.js'; +import { resolveActiveLearningsNamespaces } from './projects.js'; +import { isSafeNamespaceSegment } from './manifest-schema.js'; import type { GlobalOptions, LocalConfig } from './types.js'; import { getDataHome, getReportsDir, isSelfMode } from './types.js'; diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts new file mode 100644 index 00000000..90027818 --- /dev/null +++ b/src/manifest-schema.ts @@ -0,0 +1,41 @@ +import { z } from 'zod'; + +/** + * What `manifest/projects.yaml` and `manifest/roles.yaml` share: the spelling of + * a resource namespace, and the shape of the error a bad manifest produces. + * + * A namespace becomes a path component (`skills//`, + * `agents//`, `learnings//`), so it may not escape the + * directory it names. Nothing else about it is constrained: it is a directory + * name, so any name a filesystem accepts — non-ASCII, or holding a space — + * stays valid. (A project id is narrower still, because it is also typed on the + * command line; that guard lives with the project schema.) + */ +// `:` is unsafe with the separators rather than merely unusual: on Windows +// `path.resolve(base, 'C:evil')` is drive-relative and lands outside `base`. +const UNSAFE_SEGMENT = /[/\\:\u0000-\u001f]/; + +/** True if `seg` is safe to use as a single path segment (no separators, no `..`). */ +export function isSafeNamespaceSegment(seg: string): boolean { + return seg.length > 0 && !UNSAFE_SEGMENT.test(seg) && seg !== '.' && seg !== '..'; +} + +/** A resource namespace: one path segment that cannot escape its parent. */ +export const NamespaceSegmentSchema = z.string().min(1).refine(isSafeNamespaceSegment, { + message: "resource namespace must be a single path segment (no '/', '\\', ':' or control characters, and not '.' or '..')", +}); + +/** + * Parse a manifest, reporting a failure the way the hand-written checks around + * it do: one line naming the offending entry. A raw ZodError reaches the CLI as + * an object dump, which tells an admin nothing about which line to edit. + */ +export function parseManifest(schema: S, raw: unknown, kind: 'projects' | 'roles'): z.infer { + const parsed = schema.safeParse(raw); + if (parsed.success) return parsed.data; + + const detail = parsed.error.issues + .map((issue) => (issue.path.length > 0 ? `${issue.path.join('.')}: ${issue.message}` : issue.message)) + .join('; '); + throw new Error(`Invalid ${kind} manifest: ${detail}`); +} diff --git a/src/projects.ts b/src/projects.ts index ab1607ca..5519e9b4 100644 --- a/src/projects.ts +++ b/src/projects.ts @@ -2,6 +2,7 @@ import path from 'node:path'; import YAML from 'yaml'; import { z } from 'zod'; import { readFileIfExists, ensureDir, writeFile } from './utils/fs.js'; +import { NamespaceSegmentSchema, parseManifest } from './manifest-schema.js'; import type { ResourceNamespaces } from './roles.js'; /** @@ -14,33 +15,25 @@ const PROJECT_RESOURCE_TYPES = ['knowledge', 'skills', 'learnings', 'agents'] as export type ProjectResourceType = typeof PROJECT_RESOURCE_TYPES[number]; /** - * A project id and every resource namespace become path components - * (`skills//`, `learnings//`), so neither may contain a path - * separator or `..`. + * A project id becomes a path component (`skills//`, `learnings//`) just + * as a resource namespace does, so it carries the same traversal guard. It is + * narrower than a namespace on purpose: an id is also typed on the command line + * and split on commas (`teamai projects set a,b`), so it keeps the ASCII + * spelling it has always had. * - * Both are enforced here at the manifest boundary, which is the only place they - * enter the process: an id read from elsewhere (a hand-edited config.yaml - * `projects` field) is resolved through `getProjectOrThrow`, so it can only ever - * name a project this manifest already validated. `contribute.ts` and + * Both are enforced here at the manifest boundary, the only place they enter the + * process: an id read from elsewhere (a hand-edited config.yaml `projects` + * field) is resolved through `getProjectOrThrow`, so it can only ever name a + * project this manifest already validated. `contribute.ts` and * `resources/agents.ts` keep their own `isSafeNamespaceSegment` guards on the * resolved namespace as defence in depth. */ const SAFE_ID = /^[A-Za-z0-9._-]+$/; -/** True if `seg` is safe to use as a single path segment (no separators, no `..`). */ -export function isSafeNamespaceSegment(seg: string): boolean { - return SAFE_ID.test(seg) && seg !== '.' && seg !== '..'; +function isSafeProjectId(id: string): boolean { + return SAFE_ID.test(id) && id !== '.' && id !== '..'; } -/** Message shared by the id and namespace guards, so both read the same. */ -export const SAFE_SEGMENT_MESSAGE = - "must be a single path segment (letters, digits, '.', '_', '-'; no '/', '\\\\', or '..')"; - -/** A resource namespace: one safe path segment. */ -export const NamespaceSegmentSchema = z.string().min(1).refine(isSafeNamespaceSegment, { - message: `resource namespace ${SAFE_SEGMENT_MESSAGE}`, -}); - const ProjectResourceNamespacesSchema = z.object({ knowledge: z.array(NamespaceSegmentSchema).default([]), skills: z.array(NamespaceSegmentSchema).default([]), @@ -49,8 +42,8 @@ const ProjectResourceNamespacesSchema = z.object({ }); const ProjectSchema = z.object({ - id: z.string().min(1).refine(isSafeNamespaceSegment, { - message: `project id ${SAFE_SEGMENT_MESSAGE}`, + id: z.string().min(1).refine(isSafeProjectId, { + message: "project id must be a single path segment (letters, digits, '.', '_', '-'; no '/', '\\\\', or '..')", }), name: z.string().default(''), description: z.string().default(''), @@ -98,7 +91,7 @@ function validateManifestShape(raw: unknown): ProjectsManifest { } } - const manifest = ProjectsManifestSchema.parse(raw); + const manifest = parseManifest(ProjectsManifestSchema, raw, 'projects'); const ids = new Set(); for (const project of manifest.projects) { if (ids.has(project.id)) { diff --git a/src/resources/agents.ts b/src/resources/agents.ts index 9c1883ec..53390262 100644 --- a/src/resources/agents.ts +++ b/src/resources/agents.ts @@ -8,7 +8,7 @@ import { log } from '../utils/logger.js'; import { resolveToolBaseDir, isAgentExcluded, isSelfMode, scopedToolPaths } from '../types.js'; import { BUILTIN_AGENT_NAMES } from '../builtin-agents.js'; import { resolveResourceNamespaces } from '../resource-namespaces.js'; -import { isSafeNamespaceSegment } from '../projects.js'; +import { isSafeNamespaceSegment } from '../manifest-schema.js'; import { assertWithinRoot } from '../utils/path-safety.js'; import { parseAgentYaml, diff --git a/src/roles.ts b/src/roles.ts index e3ddb18c..3545dbfd 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -3,10 +3,7 @@ import YAML from 'yaml'; import { z } from 'zod'; import { readFileSafe, readFileIfExists, ensureDir, writeFile } from './utils/fs.js'; import { log } from './utils/logger.js'; -// A role namespace is the same kind of path component a project namespace is, so -// both manifests guard it with one schema. projects.ts imports only a *type* -// from here, so this direction adds no runtime cycle. -import { NamespaceSegmentSchema } from './projects.js'; +import { NamespaceSegmentSchema, parseManifest } from './manifest-schema.js'; const ROLE_RESOURCE_TYPES = ['knowledge', 'skills', 'agents'] as const; @@ -84,7 +81,7 @@ function validateManifestShape(raw: unknown): RolesManifest { } } - const manifest = RolesManifestSchema.parse(raw); + const manifest = parseManifest(RolesManifestSchema, raw, 'roles'); const ids = new Set(); for (const role of manifest.roles) { if (ids.has(role.id)) { From 399eb8ed801ebccd25013f4de7a2e0d123065bb2 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 12:47:32 +0200 Subject: [PATCH 04/25] fix(manifest): reject DEL and C1 controls in a namespace too Review P2 on #710: the guard rejected only U+0000-U+001F while the error message and the docs promise every control character, so U+007F and the C1 range U+0080-U+009F still parsed. Range extended and the three ranges covered in the projects and roles fixtures. --- src/__tests__/projects.test.ts | 7 ++++++- src/__tests__/roles.test.ts | 5 ++++- src/manifest-schema.ts | 5 ++++- 3 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index fa1ebaec..44c13707 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -147,7 +147,12 @@ projects: // A namespace becomes a directory component (skills//, agents//) just // as a project id does, so the boundary has to guard both. for (const type of ['knowledge', 'skills', 'learnings', 'agents']) { - for (const badNamespace of ['../../evil', 'a/b', '..', '.', 'x\\y', 'C:evil']) { + // '\u0009' (C0), '\u007f' (DEL) and '\u0085' (C1) stand for the three control + // ranges the message promises to reject. + for (const badNamespace of [ + '../../evil', 'a/b', '..', '.', 'x\\y', 'C:evil', + 'a\u0009b', 'a\u007fb', 'a\u0085b', + ]) { const repoDir = writeManifest(` version: 1 projects: diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index 061b36ef..2a6512de 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -126,7 +126,10 @@ roles: it('fails when a resource namespace is not a safe path segment (traversal guard)', async () => { // Role namespaces become directory components (skills//, agents//) // exactly as project namespaces do, so the same boundary guard applies. - for (const badNamespace of ['../../evil', 'a/b', '..', '.', 'x\\y', 'C:evil']) { + for (const badNamespace of [ + '../../evil', 'a/b', '..', '.', 'x\\y', 'C:evil', + 'a\u0009b', 'a\u007fb', 'a\u0085b', + ]) { const repoDir = writeManifest(` version: 1 roles: diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index 90027818..01086b57 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -13,7 +13,10 @@ import { z } from 'zod'; */ // `:` is unsafe with the separators rather than merely unusual: on Windows // `path.resolve(base, 'C:evil')` is drive-relative and lands outside `base`. -const UNSAFE_SEGMENT = /[/\\:\u0000-\u001f]/; +// The control ranges are both of them, C0 with DEL and C1: a segment carrying one +// is a name no admin typed on purpose, and it renders as something other than +// what it is in a terminal that reports the path back. +const UNSAFE_SEGMENT = /[/\\:\u0000-\u001f\u007f-\u009f]/; /** True if `seg` is safe to use as a single path segment (no separators, no `..`). */ export function isSafeNamespaceSegment(seg: string): boolean { From a4f579e713812be4560a7ca39e82d4b4be156452 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 13:09:29 +0200 Subject: [PATCH 05/25] fix(manifest): reject the Win32 dot/space aliases of . and .. Review P1 on #710: the guard tested for the exact strings '.' and '..', so '.. ', '.. .' and '...' passed. Win32 strips trailing spaces and periods from a path component, so each of those reaches the filesystem as '..' and escapes the namespace directory it was supposed to name. A segment of nothing but dots and spaces is '.' or '..' in disguise and is refused as such; 'a..' keeps parsing, since it stays inside its parent. The project id, whose allowlist already excluded spaces, refuses any run of dots for the same reason. --- CHANGELOG.md | 2 +- docs/designs/multi-project-management.md | 4 +++- docs/usage-guide.md | 10 ++++++---- docs/usage-guide.zh-CN.md | 7 ++++--- src/__tests__/projects.test.ts | 3 +++ src/__tests__/roles.test.ts | 1 + src/manifest-schema.ts | 10 ++++++++-- src/projects.ts | 4 +++- 8 files changed, 29 insertions(+), 12 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ec767eb8..c8a497cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ All notable changes to this project will be documented in this file. See [standa ### 🐛 Bug Fixes - `teamai init` no longer hangs without a terminal. When the provider had no session it spawned `gh auth login --web` (or `gf auth login`, `cnb login`) with inherited stdio and waited for a browser device flow that nobody could complete, about five minutes for GitHub, then exited with the provider's error and no hint of the missing credential. Each login now refuses up front when the run is not interactive and names the credential to prepare (`GITHUB_TOKEN` / `GH_TOKEN`, `CNB_TOKEN`, or for TGit a prior `gf auth login`, since a `TGIT_TOKEN` PAT is REST-API-only and cannot clone). A run is non-interactive when stdin is not a TTY or when `CI` or `TEAMAI_NONINTERACTIVE` is set, so an agent sandbox with a pseudo-terminal can declare itself unattended, and every prompt in the CLI follows the same rule. `git` also runs with its prompts closed in that case — `GIT_TERMINAL_PROMPT=0`, `GIT_ASKPASS=echo` and `GCM_INTERACTIVE=never`, each only where the caller set nothing — so a missing clone credential fails at once instead of waiting on a terminal prompt or an askpass or credential-manager dialog. `ssh` keeps its own settings: its batch flag is only reachable through `GIT_SSH_COMMAND`, which would override each repository's `core.sshCommand` (for [#711](https://github.com/Tencent/teamai-cli/issues/711)). -- `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil`, `a/b`, `C:evil` or a bare `..` under `resources:` no longer parses; the error names the offending entry. Only traversal is rejected: a namespace that is a plain directory name still parses, non-ASCII names and names with a space included. A manifest that fails to parse now reports the offending entry on one line (`Invalid projects manifest: projects.0.resources.skills.1: ...`) instead of dumping a raw validation object. +- `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil`, `a/b`, `C:evil`, a bare `..` and its Windows aliases `.. ` and `...` under `resources:` no longer parse; the error names the offending entry. Only traversal is rejected: a namespace that is a plain directory name still parses, non-ASCII names and names with a space included. A manifest that fails to parse now reports the offending entry on one line (`Invalid projects manifest: projects.0.resources.skills.1: ...`) instead of dumping a raw validation object. - Cache GC now rejects partial integers such as `12abc`, decimals, zero and unsafe integers for `--max-bytes` and `--stale-days` before deleting anything. An invalid `TEAMAI_CACHE_MAX_BYTES` value falls back to the default 5 GB limit instead of using a numeric prefix. - Usage reporting scopes sessions by path on Windows too. The project/user scope filter compared an event's `cwd` against `projectRoot` with a hard-coded `/` separator, so on Windows only a session started in the project root itself matched: every session started in a subdirectory was dropped from the project team's report and counted in the user scope's instead, which is the isolation the usage guide promises. Windows paths are also compared case-insensitively, so a drive letter or a directory name spelled with different case in the two sources no longer leaks a project session into the user scope. POSIX paths keep their own rules: case-sensitive, and a `\` in a filename stays part of the name. - `teamai tags subscribe` and `teamai tags unsubscribe` now invalidate the pull revision cache, as `teamai skill exclude` already does, so the next `teamai pull` applies the new subscriptions instead of reporting "Already synced" when the team repo has not changed. diff --git a/docs/designs/multi-project-management.md b/docs/designs/multi-project-management.md index 60380498..1f629b4b 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -82,7 +82,9 @@ projects: ``` The id and every namespace are refused at the manifest boundary unless they are a -single path segment (no `/`, `\`, `:` or control character, and not `.` or `..`), +single path segment (no `/`, `\`, `:` or control character, and not `.`, `..` or +any other name made only of dots and spaces — Win32 strips trailing spaces and +periods, so `.. ` would arrive as `..`), since each becomes a directory component. The id is narrower still — letters, digits, `.`, `_`, `-` — because it is also typed on the command line and split on commas. The same guard applies to `manifest/roles.yaml`. diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 8131f4d7..d90ba3c4 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -245,8 +245,9 @@ projects: The project id and every namespace under `resources:` become a directory name (`skills//`, `learnings//`, `agents//`), so each -must be a single path segment: no `/`, `\`, `:` or control character, and not `.` -or `..`. A namespace is otherwise free — a non-ASCII name or one with a space is +must be a single path segment: no `/`, `\`, `:` or control character, and not `.`, +`..` or any other name made only of dots and spaces (Windows strips trailing +spaces and periods, so `.. ` would arrive as `..`). A namespace is otherwise free — a non-ASCII name or one with a space is still a valid directory. A project id is narrower, because it is also typed on the command line: letters, digits, `.`, `_` and `-`. A manifest that breaks either rule fails to parse, and the error names the offending entry. @@ -1488,8 +1489,9 @@ roles: ``` Every namespace listed under `resources:` becomes a directory name, so it must be -a single path segment — no `/`, `\`, `:` or control character, and not `.` or -`..` — in `manifest/roles.yaml` exactly as in `manifest/projects.yaml`. +a single path segment — no `/`, `\`, `:` or control character, and not `.`, `..` +or any other name made only of dots and spaces — in `manifest/roles.yaml` exactly +as in `manifest/projects.yaml`. `teamai pull` copies these into each Tier-1 tool's `agents/` directory (e.g. `~/.claude/agents/`), flattened by file name, so two active namespaces must not define the same agent name (pull reports the collision and skips the scope). `teamai pull` writes `.toml` for Codex tools, `.json` for Kiro, `.agent.md` for Copilot, and `.md` for every other tool. When a member changes role, agents of the namespaces that stopped being active are removed on the next pull, unless the deployed copy was edited locally, in which case it is kept with a warning. Without a configured role, every agent syncs. `teamai push` resolves the source using the same active role and project namespaces as pull. It writes edits to that source and skips ambiguous destinations with a warning; an agent with only inactive sources is also skipped. Skipped agents do not block other resources in the same push. A new agent lands at the root. Cleanup checks each tool separately, respecting YAML `targets` and legacy format support. An active same-named agent protects a deployed file only when it targets that tool and output file. `teamai remove agents ` records a tombstone. The next pull on every other machine deletes `.agent.md`, `.md`, `.toml` and `.json` from each synced tool's agents directory. That cleanup also runs when the pull finds the team repo unchanged. The CLI's built-in `teamai-recall` profile is deployed alongside team agents but is not uploaded by `teamai push`. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 5860861c..21618c6b 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -230,7 +230,8 @@ projects: 项目 id 与 `resources:` 下的每个 namespace 都会成为目录名 （`skills//`、`learnings//`、`agents//`），因此 -必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，且不能是 `.` 或 `..`。除此 +必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，且不能是 `.`、`..` 或任何只由 +点和空格组成的名称（Windows 会删除结尾的空格与句点，`.. ` 最终会变成 `..`）。除此 之外 namespace 不受限制 —— 非 ASCII 名称或含空格的名称仍是合法目录。项目 id 更 严格，因为它还会在命令行中输入：只允许字母、数字、`.`、`_` 和 `-`。违反任一规则 的 manifest 会解析失败，错误信息会指出具体条目。 @@ -1447,8 +1448,8 @@ roles: ``` `resources:` 下列出的每个 namespace 都会成为目录名，因此必须是单个路径片段 —— -不含 `/`、`\`、`:` 和控制字符，且不能是 `.` 或 `..` —— `manifest/roles.yaml` 与 -`manifest/projects.yaml` 规则一致。 +不含 `/`、`\`、`:` 和控制字符，且不能是 `.`、`..` 或任何只由点和空格组成的名称 +—— `manifest/roles.yaml` 与 `manifest/projects.yaml` 规则一致。 `teamai pull` 会将它们按文件名拍平复制到每个 Tier-1 工具的 `agents/` 目录（如 `~/.claude/agents/`），因此两个活跃 namespace 不能定义同名 agent（pull 会报告冲突并跳过该 scope）。`teamai pull` 为 Codex 系工具写入 `.toml`，为 Kiro 写入 `.json`，为 Copilot 写入 `.agent.md`，其余工具写入 `.md`。成员切换角色后，不再活跃的 namespace 中的 agents 会在下一次 pull 时被移除；若本地副本已被手动修改，则保留并给出警告。未配置角色时同步全部 agents。`teamai push` 使用与 pull 相同的活跃角色和项目 namespace 来确定源文件，并将修改写回该源文件；若存在多个候选目标，则跳过并给出警告。若源文件均不活跃，也会跳过。跳过的 agent 不会阻止同一次 push 中的其他资源。新 agent 落在根目录。清理会逐个工具检查 YAML 的 `targets` 和旧格式支持；只有活跃的同名 agent 会写入该工具的同一输出文件时，才保留该文件。`teamai remove agents ` 会记录 tombstone。其他机器下一次 pull 时，会从每个同步中的工具的 agents 目录删除 `.agent.md`、`.md`、`.toml` 和 `.json`。即使该次 pull 发现团队仓库没有变化，也会执行清理。CLI 内置的 `teamai-recall` 配置与团队 agents 并列部署，但不会被 `teamai push` 上传。 diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index 44c13707..d7a6ca04 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -149,9 +149,12 @@ projects: for (const type of ['knowledge', 'skills', 'learnings', 'agents']) { // '\u0009' (C0), '\u007f' (DEL) and '\u0085' (C1) stand for the three control // ranges the message promises to reject. + // Win32 strips trailing spaces and periods, so '.. ', '.. .' and '...' all + // arrive as '..'; they have to fall with the literal ones. for (const badNamespace of [ '../../evil', 'a/b', '..', '.', 'x\\y', 'C:evil', 'a\u0009b', 'a\u007fb', 'a\u0085b', + '.. ', '.. .', '...', '. ', ' ', ]) { const repoDir = writeManifest(` version: 1 diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index 2a6512de..1cc4cbe0 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -129,6 +129,7 @@ roles: for (const badNamespace of [ '../../evil', 'a/b', '..', '.', 'x\\y', 'C:evil', 'a\u0009b', 'a\u007fb', 'a\u0085b', + '.. ', '.. .', '...', '. ', ' ', ]) { const repoDir = writeManifest(` version: 1 diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index 01086b57..740a9a00 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -18,14 +18,20 @@ import { z } from 'zod'; // what it is in a terminal that reports the path back. const UNSAFE_SEGMENT = /[/\\:\u0000-\u001f\u007f-\u009f]/; +// Win32 strips trailing spaces and periods from a path component, so `.. `, `.. .` +// and `...` all reach the filesystem as `..` or as nothing at all. A segment of +// nothing but dots and spaces is therefore `.` or `..` in disguise; `a..` is not, +// it stays inside its parent, so only the whole-string form is refused. +const DOTS_AND_SPACES = /^[ .]+$/; + /** True if `seg` is safe to use as a single path segment (no separators, no `..`). */ export function isSafeNamespaceSegment(seg: string): boolean { - return seg.length > 0 && !UNSAFE_SEGMENT.test(seg) && seg !== '.' && seg !== '..'; + return seg.length > 0 && !UNSAFE_SEGMENT.test(seg) && !DOTS_AND_SPACES.test(seg); } /** A resource namespace: one path segment that cannot escape its parent. */ export const NamespaceSegmentSchema = z.string().min(1).refine(isSafeNamespaceSegment, { - message: "resource namespace must be a single path segment (no '/', '\\', ':' or control characters, and not '.' or '..')", + message: "resource namespace must be a single path segment (no '/', '\\', ':' or control characters, and not '.', '..' or any other name made only of dots and spaces)", }); /** diff --git a/src/projects.ts b/src/projects.ts index 5519e9b4..7aa4aca9 100644 --- a/src/projects.ts +++ b/src/projects.ts @@ -29,9 +29,11 @@ export type ProjectResourceType = typeof PROJECT_RESOURCE_TYPES[number]; * resolved namespace as defence in depth. */ const SAFE_ID = /^[A-Za-z0-9._-]+$/; +/** `.`, `..` and every longer run of dots: see DOTS_AND_SPACES in manifest-schema. */ +const ONLY_DOTS = /^\.+$/; function isSafeProjectId(id: string): boolean { - return SAFE_ID.test(id) && id !== '.' && id !== '..'; + return SAFE_ID.test(id) && !ONLY_DOTS.test(id); } const ProjectResourceNamespacesSchema = z.object({ From b52a3b1f500d4ccb40b42b66dbe791c9a97f67f4 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 13:15:07 +0200 Subject: [PATCH 06/25] fix(manifest): keep the project id rule untouched, scope the role docs Review on #710. The dot/space fix reached further than it needed to: tightening the id to reject every run of dots also rejected '...', a working POSIX directory name the id rule has always accepted, so a manifest that parses today would have stopped. The id is back to the exact '.'/'..' check it had before this PR, with a test that says so. Only the namespace rule moves. The role docs claimed every namespace under resources: follows the rule, but roles.yaml's learnings: is accepted for backward compatibility and ignored at runtime -- it names no directory, so holding an old manifest to the rule would reject it over a field nothing reads. Both usage guides and the design doc now name the fields that do take effect. --- docs/designs/multi-project-management.md | 4 +++- docs/usage-guide.md | 11 +++++++---- docs/usage-guide.zh-CN.md | 8 +++++--- src/__tests__/projects.test.ts | 18 ++++++++++++++++++ src/projects.ts | 4 +--- src/roles.ts | 2 ++ 6 files changed, 36 insertions(+), 11 deletions(-) diff --git a/docs/designs/multi-project-management.md b/docs/designs/multi-project-management.md index 1f629b4b..f71573a2 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -87,7 +87,9 @@ any other name made only of dots and spaces — Win32 strips trailing spaces and periods, so `.. ` would arrive as `..`), since each becomes a directory component. The id is narrower still — letters, digits, `.`, `_`, `-` — because it is also typed on the command line and split on -commas. The same guard applies to `manifest/roles.yaml`. +commas. The same guard applies to `manifest/roles.yaml`'s active namespaces +(`knowledge`, `skills`, `agents`); its `learnings:` is kept for backward +compatibility, ignored at runtime, and therefore unchecked. Agent push uses the same role/project namespace resolution as pull and skips ambiguous source destinations. On a role or project change, agent cleanup checks each tool destination independently, including YAML `targets` and legacy format support. Locally edited copies are preserved. diff --git a/docs/usage-guide.md b/docs/usage-guide.md index d90ba3c4..96313b33 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -1488,10 +1488,13 @@ roles: agents: [common, frontend] # optional; omitted = root-level agents only ``` -Every namespace listed under `resources:` becomes a directory name, so it must be -a single path segment — no `/`, `\`, `:` or control character, and not `.`, `..` -or any other name made only of dots and spaces — in `manifest/roles.yaml` exactly -as in `manifest/projects.yaml`. +Every namespace that takes effect — `knowledge`, `skills` and `agents` — becomes a +directory name, so it must be a single path segment: no `/`, `\`, `:` or control +character, and not `.`, `..` or any other name made only of dots and spaces, in +`manifest/roles.yaml` exactly as in `manifest/projects.yaml`. A role's +`learnings:` is accepted for backward compatibility and ignored at runtime +(learnings are namespaced by project, not by role), so it names no directory and +is not checked. `teamai pull` copies these into each Tier-1 tool's `agents/` directory (e.g. `~/.claude/agents/`), flattened by file name, so two active namespaces must not define the same agent name (pull reports the collision and skips the scope). `teamai pull` writes `.toml` for Codex tools, `.json` for Kiro, `.agent.md` for Copilot, and `.md` for every other tool. When a member changes role, agents of the namespaces that stopped being active are removed on the next pull, unless the deployed copy was edited locally, in which case it is kept with a warning. Without a configured role, every agent syncs. `teamai push` resolves the source using the same active role and project namespaces as pull. It writes edits to that source and skips ambiguous destinations with a warning; an agent with only inactive sources is also skipped. Skipped agents do not block other resources in the same push. A new agent lands at the root. Cleanup checks each tool separately, respecting YAML `targets` and legacy format support. An active same-named agent protects a deployed file only when it targets that tool and output file. `teamai remove agents ` records a tombstone. The next pull on every other machine deletes `.agent.md`, `.md`, `.toml` and `.json` from each synced tool's agents directory. That cleanup also runs when the pull finds the team repo unchanged. The CLI's built-in `teamai-recall` profile is deployed alongside team agents but is not uploaded by `teamai push`. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 21618c6b..51c3f6af 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -1447,9 +1447,11 @@ roles: agents: [common, frontend] # 可选；省略 = 只同步根目录 agents ``` -`resources:` 下列出的每个 namespace 都会成为目录名，因此必须是单个路径片段 —— -不含 `/`、`\`、`:` 和控制字符，且不能是 `.`、`..` 或任何只由点和空格组成的名称 -—— `manifest/roles.yaml` 与 `manifest/projects.yaml` 规则一致。 +真正生效的 namespace（`knowledge`、`skills`、`agents`）都会成为目录名，因此必须是 +单个路径片段：不含 `/`、`\`、`:` 和控制字符，且不能是 `.`、`..` 或任何只由点和空格 +组成的名称，`manifest/roles.yaml` 与 `manifest/projects.yaml` 规则一致。role 的 +`learnings:` 仅为向后兼容而保留、运行时忽略（learnings 按 project 而非 role 划分 +namespace），不会成为目录名，因此不做校验。 `teamai pull` 会将它们按文件名拍平复制到每个 Tier-1 工具的 `agents/` 目录（如 `~/.claude/agents/`），因此两个活跃 namespace 不能定义同名 agent（pull 会报告冲突并跳过该 scope）。`teamai pull` 为 Codex 系工具写入 `.toml`，为 Kiro 写入 `.json`，为 Copilot 写入 `.agent.md`，其余工具写入 `.md`。成员切换角色后，不再活跃的 namespace 中的 agents 会在下一次 pull 时被移除；若本地副本已被手动修改，则保留并给出警告。未配置角色时同步全部 agents。`teamai push` 使用与 pull 相同的活跃角色和项目 namespace 来确定源文件，并将修改写回该源文件；若存在多个候选目标，则跳过并给出警告。若源文件均不活跃，也会跳过。跳过的 agent 不会阻止同一次 push 中的其他资源。新 agent 落在根目录。清理会逐个工具检查 YAML 的 `targets` 和旧格式支持；只有活跃的同名 agent 会写入该工具的同一输出文件时，才保留该文件。`teamai remove agents ` 会记录 tombstone。其他机器下一次 pull 时，会从每个同步中的工具的 agents 目录删除 `.agent.md`、`.md`、`.toml` 和 `.json`。即使该次 pull 发现团队仓库没有变化，也会执行清理。CLI 内置的 `teamai-recall` 配置与团队 agents 并列部署，但不会被 `teamai push` 上传。 diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index d7a6ca04..9367a15d 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -187,6 +187,24 @@ projects: } }); + it('leaves the project id rule exactly where it was', async () => { + // The namespace guard tightened; the id did not. '...' is a working POSIX + // directory name that the id rule has always accepted, so a manifest using + // it must keep parsing. + const repoDir = writeManifest(` +version: 1 +projects: + - id: '...' + resources: { skills: [alpha] } +`); + try { + const manifest = await loadProjectsManifest(repoDir); + expect(manifest?.projects[0].id).toBe('...'); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('names the offending entry instead of dumping a raw ZodError', async () => { const repoDir = writeManifest(` version: 1 diff --git a/src/projects.ts b/src/projects.ts index 7aa4aca9..5519e9b4 100644 --- a/src/projects.ts +++ b/src/projects.ts @@ -29,11 +29,9 @@ export type ProjectResourceType = typeof PROJECT_RESOURCE_TYPES[number]; * resolved namespace as defence in depth. */ const SAFE_ID = /^[A-Za-z0-9._-]+$/; -/** `.`, `..` and every longer run of dots: see DOTS_AND_SPACES in manifest-schema. */ -const ONLY_DOTS = /^\.+$/; function isSafeProjectId(id: string): boolean { - return SAFE_ID.test(id) && !ONLY_DOTS.test(id); + return SAFE_ID.test(id) && id !== '.' && id !== '..'; } const ProjectResourceNamespacesSchema = z.object({ diff --git a/src/roles.ts b/src/roles.ts index 3545dbfd..b6c757f2 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -17,6 +17,8 @@ const RoleResourceNamespacesSchema = z.object({ agents: z.array(NamespaceSegmentSchema).default([]), // learnings is accepted for backward compatibility but ignored at runtime. // All learnings are shared flat across the entire team (no namespace isolation). + // It never becomes a directory here, so it stays a plain string: holding an old + // manifest to the namespace rule would reject it over a field nothing reads. learnings: z.array(z.string()).optional(), }); From 391bafd5284ce4646cc10aff916e751cfa2de008 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 13:19:30 +0200 Subject: [PATCH 07/25] docs(manifest): state the id and namespace rules separately Review P2 on #710: after the id was left on its old rule, the docs still described one rule for both, so they claimed a project id rejects any name made only of dots and spaces while '...' parses. Each rule now stands on its own in both usage guides and the design doc, and the projects.ts comment says why the id is not held to the namespace rule. --- docs/designs/multi-project-management.md | 15 ++++++++------- docs/usage-guide.md | 22 +++++++++++++++------- docs/usage-guide.zh-CN.md | 15 ++++++++++----- src/projects.ts | 9 +++++---- 4 files changed, 38 insertions(+), 23 deletions(-) diff --git a/docs/designs/multi-project-management.md b/docs/designs/multi-project-management.md index f71573a2..1cb6e1f6 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -81,13 +81,14 @@ projects: agents: [hai-inference] # optional; agents// scoped to this project ``` -The id and every namespace are refused at the manifest boundary unless they are a -single path segment (no `/`, `\`, `:` or control character, and not `.`, `..` or -any other name made only of dots and spaces — Win32 strips trailing spaces and -periods, so `.. ` would arrive as `..`), -since each becomes a directory component. The id is narrower still — letters, -digits, `.`, `_`, `-` — because it is also typed on the command line and split on -commas. The same guard applies to `manifest/roles.yaml`'s active namespaces +The id and every namespace are refused at the manifest boundary unless they can +name a directory without escaping it, since each becomes a directory component. A +namespace must be a single path segment: no `/`, `\`, `:` or control character, +and not `.`, `..` or any other name made only of dots and spaces (Win32 strips +trailing spaces and periods, so `.. ` would arrive as `..`). The id keeps the +older, narrower rule it has always had — letters, digits, `.`, `_`, `-`, and not +`.` or `..` — because it is also typed on the command line and split on commas. +The namespace guard applies to `manifest/roles.yaml`'s active namespaces (`knowledge`, `skills`, `agents`); its `learnings:` is kept for backward compatibility, ignored at runtime, and therefore unchecked. diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 96313b33..0c87bed1 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -244,13 +244,21 @@ projects: ``` The project id and every namespace under `resources:` become a directory name -(`skills//`, `learnings//`, `agents//`), so each -must be a single path segment: no `/`, `\`, `:` or control character, and not `.`, -`..` or any other name made only of dots and spaces (Windows strips trailing -spaces and periods, so `.. ` would arrive as `..`). A namespace is otherwise free — a non-ASCII name or one with a space is -still a valid directory. A project id is narrower, because it is also typed on -the command line: letters, digits, `.`, `_` and `-`. A manifest that breaks -either rule fails to parse, and the error names the offending entry. +(`skills//`, `learnings//`, `agents//`), so +neither may escape the directory it names. + +A **namespace** must be a single path segment: no `/`, `\`, `:` or control +character, and not `.`, `..` or any other name made only of dots and spaces +(Windows strips trailing spaces and periods, so `.. ` would arrive as `..`). +Anything else a filesystem accepts stays valid — a non-ASCII name, or one holding +a space. + +A **project id** keeps its own older and narrower rule, because it is also typed +on the command line and split on commas: letters, digits, `.`, `_` and `-`, and +not `.` or `..`. + +A manifest that breaks either rule fails to parse, and the error names the +offending entry. **Commands** (low-frequency correction/query, mirroring `teamai roles …`): diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 51c3f6af..8bab0f03 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -230,11 +230,16 @@ projects: 项目 id 与 `resources:` 下的每个 namespace 都会成为目录名 （`skills//`、`learnings//`、`agents//`），因此 -必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，且不能是 `.`、`..` 或任何只由 -点和空格组成的名称（Windows 会删除结尾的空格与句点，`.. ` 最终会变成 `..`）。除此 -之外 namespace 不受限制 —— 非 ASCII 名称或含空格的名称仍是合法目录。项目 id 更 -严格，因为它还会在命令行中输入：只允许字母、数字、`.`、`_` 和 `-`。违反任一规则 -的 manifest 会解析失败，错误信息会指出具体条目。 +都不能越出自己命名的目录。 + +**namespace** 必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，且不能是 `.`、 +`..` 或任何只由点和空格组成的名称（Windows 会删除结尾的空格与句点，`.. ` 最终会 +变成 `..`）。除此之外不受限制 —— 非 ASCII 名称或含空格的名称仍是合法目录。 + +**项目 id** 沿用它原有的、更严格的规则，因为它还会在命令行中输入并按逗号切分： +只允许字母、数字、`.`、`_` 和 `-`，且不能是 `.` 或 `..`。 + +违反任一规则的 manifest 会解析失败，错误信息会指出具体条目。 **命令**（低频的事后修正与查询，对标 `teamai roles …`）： diff --git a/src/projects.ts b/src/projects.ts index 5519e9b4..a6120c9c 100644 --- a/src/projects.ts +++ b/src/projects.ts @@ -16,10 +16,11 @@ export type ProjectResourceType = typeof PROJECT_RESOURCE_TYPES[number]; /** * A project id becomes a path component (`skills//`, `learnings//`) just - * as a resource namespace does, so it carries the same traversal guard. It is - * narrower than a namespace on purpose: an id is also typed on the command line - * and split on commas (`teamai projects set a,b`), so it keeps the ASCII - * spelling it has always had. + * as a resource namespace does, so it is guarded here too — but by its own older + * rule, not the namespace one. An id is also typed on the command line and split + * on commas (`teamai projects set a,b`), so its ASCII allowlist already excludes + * most of what the namespace guard has to test for, and holding it to the rest + * would reject ids that work today (`...` is a directory POSIX accepts). * * Both are enforced here at the manifest boundary, the only place they enter the * process: an id read from elsewhere (a hand-edited config.yaml `projects` From bf0d79f2c9b571032de772595669899a63738208 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 13:24:10 +0200 Subject: [PATCH 08/25] fix(manifest): reject a namespace with a trailing '.' or space Review P1 on #710: refusing only names made entirely of dots and spaces left the aliasing half open. Win32 strips trailing periods and spaces from every path component, so 'frontend.', 'frontend ' and 'frontend..' all resolve to 'frontend' -- one namespace reading and writing another's directory, which is the isolation a namespace exists to provide. The rule is now the trailing character itself, which covers the escape ('.. ' arriving as '..') and the aliasing in one test, and '.' and '..' fall out of it. A dot inside a name ('alpha.v2') is untouched. --- CHANGELOG.md | 2 +- docs/designs/multi-project-management.md | 6 ++++-- docs/usage-guide.md | 13 +++++++------ docs/usage-guide.zh-CN.md | 12 +++++++----- src/__tests__/projects.test.ts | 3 +++ src/__tests__/roles.test.ts | 1 + src/manifest-schema.ts | 15 ++++++++------- 7 files changed, 31 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c8a497cc..3c28ed95 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,7 @@ All notable changes to this project will be documented in this file. See [standa ### 🐛 Bug Fixes - `teamai init` no longer hangs without a terminal. When the provider had no session it spawned `gh auth login --web` (or `gf auth login`, `cnb login`) with inherited stdio and waited for a browser device flow that nobody could complete, about five minutes for GitHub, then exited with the provider's error and no hint of the missing credential. Each login now refuses up front when the run is not interactive and names the credential to prepare (`GITHUB_TOKEN` / `GH_TOKEN`, `CNB_TOKEN`, or for TGit a prior `gf auth login`, since a `TGIT_TOKEN` PAT is REST-API-only and cannot clone). A run is non-interactive when stdin is not a TTY or when `CI` or `TEAMAI_NONINTERACTIVE` is set, so an agent sandbox with a pseudo-terminal can declare itself unattended, and every prompt in the CLI follows the same rule. `git` also runs with its prompts closed in that case — `GIT_TERMINAL_PROMPT=0`, `GIT_ASKPASS=echo` and `GCM_INTERACTIVE=never`, each only where the caller set nothing — so a missing clone credential fails at once instead of waiting on a terminal prompt or an askpass or credential-manager dialog. `ssh` keeps its own settings: its batch flag is only reachable through `GIT_SSH_COMMAND`, which would override each repository's `core.sshCommand` (for [#711](https://github.com/Tencent/teamai-cli/issues/711)). -- `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil`, `a/b`, `C:evil`, a bare `..` and its Windows aliases `.. ` and `...` under `resources:` no longer parse; the error names the offending entry. Only traversal is rejected: a namespace that is a plain directory name still parses, non-ASCII names and names with a space included. A manifest that fails to parse now reports the offending entry on one line (`Invalid projects manifest: projects.0.resources.skills.1: ...`) instead of dumping a raw validation object. +- `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil`, `a/b`, `C:evil`, a bare `..`, and any name with a trailing `.` or space — which Win32 strips, making `.. ` arrive as `..` and `frontend.` as `frontend` — under `resources:` no longer parse; the error names the offending entry. Only traversal is rejected: a namespace that is a plain directory name still parses, non-ASCII names and names with a space included. A manifest that fails to parse now reports the offending entry on one line (`Invalid projects manifest: projects.0.resources.skills.1: ...`) instead of dumping a raw validation object. - Cache GC now rejects partial integers such as `12abc`, decimals, zero and unsafe integers for `--max-bytes` and `--stale-days` before deleting anything. An invalid `TEAMAI_CACHE_MAX_BYTES` value falls back to the default 5 GB limit instead of using a numeric prefix. - Usage reporting scopes sessions by path on Windows too. The project/user scope filter compared an event's `cwd` against `projectRoot` with a hard-coded `/` separator, so on Windows only a session started in the project root itself matched: every session started in a subdirectory was dropped from the project team's report and counted in the user scope's instead, which is the isolation the usage guide promises. Windows paths are also compared case-insensitively, so a drive letter or a directory name spelled with different case in the two sources no longer leaks a project session into the user scope. POSIX paths keep their own rules: case-sensitive, and a `\` in a filename stays part of the name. - `teamai tags subscribe` and `teamai tags unsubscribe` now invalidate the pull revision cache, as `teamai skill exclude` already does, so the next `teamai pull` applies the new subscriptions instead of reporting "Already synced" when the team repo has not changed. diff --git a/docs/designs/multi-project-management.md b/docs/designs/multi-project-management.md index 1cb6e1f6..6b247572 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -84,8 +84,10 @@ projects: The id and every namespace are refused at the manifest boundary unless they can name a directory without escaping it, since each becomes a directory component. A namespace must be a single path segment: no `/`, `\`, `:` or control character, -and not `.`, `..` or any other name made only of dots and spaces (Win32 strips -trailing spaces and periods, so `.. ` would arrive as `..`). The id keeps the +and no trailing `.` or space (Win32 strips those from every component, so `.. ` +would arrive as `..` and `frontend.` as `frontend`, escaping the parent in the +first case and another namespace's directory in the second; `.` and `..` fall out +of the same rule). The id keeps the older, narrower rule it has always had — letters, digits, `.`, `_`, `-`, and not `.` or `..` — because it is also typed on the command line and split on commas. The namespace guard applies to `manifest/roles.yaml`'s active namespaces diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 0c87bed1..772b6514 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -248,10 +248,11 @@ The project id and every namespace under `resources:` become a directory name neither may escape the directory it names. A **namespace** must be a single path segment: no `/`, `\`, `:` or control -character, and not `.`, `..` or any other name made only of dots and spaces -(Windows strips trailing spaces and periods, so `.. ` would arrive as `..`). -Anything else a filesystem accepts stays valid — a non-ASCII name, or one holding -a space. +character, and no trailing `.` or space. Windows strips those from every path +component, so `.. ` would arrive as `..` and escape the parent, and `frontend.` +would arrive as `frontend` and land in another namespace's directory; the rule +also rules out `.` and `..`. Anything else a filesystem accepts stays valid — a +non-ASCII name, or one holding a space inside it. A **project id** keeps its own older and narrower rule, because it is also typed on the command line and split on commas: letters, digits, `.`, `_` and `-`, and @@ -1498,8 +1499,8 @@ roles: Every namespace that takes effect — `knowledge`, `skills` and `agents` — becomes a directory name, so it must be a single path segment: no `/`, `\`, `:` or control -character, and not `.`, `..` or any other name made only of dots and spaces, in -`manifest/roles.yaml` exactly as in `manifest/projects.yaml`. A role's +character, and no trailing `.` or space, in `manifest/roles.yaml` exactly as in +`manifest/projects.yaml`. A role's `learnings:` is accepted for backward compatibility and ignored at runtime (learnings are namespaced by project, not by role), so it names no directory and is not checked. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 8bab0f03..dd6091fe 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -232,9 +232,11 @@ projects: （`skills//`、`learnings//`、`agents//`），因此 都不能越出自己命名的目录。 -**namespace** 必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，且不能是 `.`、 -`..` 或任何只由点和空格组成的名称（Windows 会删除结尾的空格与句点，`.. ` 最终会 -变成 `..`）。除此之外不受限制 —— 非 ASCII 名称或含空格的名称仍是合法目录。 +**namespace** 必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，且结尾不能是 +`.` 或空格。Windows 会从每个路径片段删除结尾的句点与空格，因此 `.. ` 最终变成 +`..` 越出上级目录，`frontend.` 最终变成 `frontend` 落进另一个 namespace 的目录； +该规则同时排除了 `.` 与 `..`。除此之外不受限制 —— 非 ASCII 名称、名称中间含空格 +的目录仍然合法。 **项目 id** 沿用它原有的、更严格的规则，因为它还会在命令行中输入并按逗号切分： 只允许字母、数字、`.`、`_` 和 `-`，且不能是 `.` 或 `..`。 @@ -1453,8 +1455,8 @@ roles: ``` 真正生效的 namespace（`knowledge`、`skills`、`agents`）都会成为目录名，因此必须是 -单个路径片段：不含 `/`、`\`、`:` 和控制字符，且不能是 `.`、`..` 或任何只由点和空格 -组成的名称，`manifest/roles.yaml` 与 `manifest/projects.yaml` 规则一致。role 的 +单个路径片段：不含 `/`、`\`、`:` 和控制字符，且结尾不能是 `.` 或空格， +`manifest/roles.yaml` 与 `manifest/projects.yaml` 规则一致。role 的 `learnings:` 仅为向后兼容而保留、运行时忽略（learnings 按 project 而非 role 划分 namespace），不会成为目录名，因此不做校验。 diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index 9367a15d..a637dd1a 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -155,6 +155,9 @@ projects: '../../evil', 'a/b', '..', '.', 'x\\y', 'C:evil', 'a\u0009b', 'a\u007fb', 'a\u0085b', '.. ', '.. .', '...', '. ', ' ', + // Win32 strips the trailing character here too, so each of these is + // `frontend` on that filesystem — another namespace's directory. + 'frontend.', 'frontend ', 'frontend..', ]) { const repoDir = writeManifest(` version: 1 diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index 1cc4cbe0..a80e4b9c 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -130,6 +130,7 @@ roles: '../../evil', 'a/b', '..', '.', 'x\\y', 'C:evil', 'a\u0009b', 'a\u007fb', 'a\u0085b', '.. ', '.. .', '...', '. ', ' ', + 'frontend.', 'frontend ', 'frontend..', ]) { const repoDir = writeManifest(` version: 1 diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index 740a9a00..f4d67a33 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -18,20 +18,21 @@ import { z } from 'zod'; // what it is in a terminal that reports the path back. const UNSAFE_SEGMENT = /[/\\:\u0000-\u001f\u007f-\u009f]/; -// Win32 strips trailing spaces and periods from a path component, so `.. `, `.. .` -// and `...` all reach the filesystem as `..` or as nothing at all. A segment of -// nothing but dots and spaces is therefore `.` or `..` in disguise; `a..` is not, -// it stays inside its parent, so only the whole-string form is refused. -const DOTS_AND_SPACES = /^[ .]+$/; +// Win32 strips trailing spaces and periods from every path component, so a +// namespace ending in one is not the directory the manifest names: `.. ` arrives +// as `..` and escapes the parent, `frontend.` arrives as `frontend` and lands in +// another namespace's directory, which is the isolation the namespace exists for. +// Refusing the trailing character covers both, and `.`/`..` fall out of it. +const TRAILING_DOT_OR_SPACE = /[ .]$/; /** True if `seg` is safe to use as a single path segment (no separators, no `..`). */ export function isSafeNamespaceSegment(seg: string): boolean { - return seg.length > 0 && !UNSAFE_SEGMENT.test(seg) && !DOTS_AND_SPACES.test(seg); + return seg.length > 0 && !UNSAFE_SEGMENT.test(seg) && !TRAILING_DOT_OR_SPACE.test(seg); } /** A resource namespace: one path segment that cannot escape its parent. */ export const NamespaceSegmentSchema = z.string().min(1).refine(isSafeNamespaceSegment, { - message: "resource namespace must be a single path segment (no '/', '\\', ':' or control characters, and not '.', '..' or any other name made only of dots and spaces)", + message: "resource namespace must be a single path segment (no '/', '\\', ':' or control characters, and no trailing '.' or space, which also rules out '.' and '..')", }); /** From d9c27472b2b3418445ea9554c01749ef2b9be518 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 13:32:35 +0200 Subject: [PATCH 09/25] fix(pull): a roles manifest that does not parse must not widen delivery Review P1 on #710. resolveResourceNamespaces caught every failure from loadRolesManifest and carried on with no role filter, which for a member with no active project means an unfiltered sync: making the schema stricter would have turned 'skills: [../../evil]' into 'deliver every namespace', the opposite of what the guard is for. The catch was covering two cases at once, because loadRolesManifest throws both when the file is absent and when it is invalid. Only the first is the legacy, unfiltered case, so it now throws a typed RolesManifestMissingError and the catch reacts to that alone. An invalid manifest propagates and pull fails the scope with the entry named -- exactly what an invalid projects manifest already does. Verified against the real CLI: with 'skills: [evil/nested]' pushed to the team repo, pull reports the failed 'Skills to deliver can be resolved' check and the three delivered skills are left untouched; restoring the manifest syncs them again. --- src/__tests__/pull-tombstone.test.ts | 27 ++++++++++++++++++++++++--- src/resource-namespaces.ts | 17 ++++++++++++++--- src/roles.ts | 15 ++++++++++++++- 3 files changed, 52 insertions(+), 7 deletions(-) diff --git a/src/__tests__/pull-tombstone.test.ts b/src/__tests__/pull-tombstone.test.ts index cba04722..ac55d7c8 100644 --- a/src/__tests__/pull-tombstone.test.ts +++ b/src/__tests__/pull-tombstone.test.ts @@ -93,6 +93,9 @@ vi.mock('../roles.js', () => ({ agents: [], }; }), + // The real class: resource-namespaces distinguishes an absent manifest from a + // malformed one by its type, so the mock has to carry the same identity. + RolesManifestMissingError: class RolesManifestMissingError extends Error {}, })); // Isolation: pull() takes a real ~/.teamai/.sync-lock. Parallel vitest workers @@ -524,14 +527,32 @@ describe('pull role-aware sync and cleanup', () => { expect(await fse.pathExists(path.join(sk, 'scripts', '.git', 'HEAD'))).toBe(true); }); - it('gracefully degrades when the roles manifest is malformed', async () => { + it('gracefully degrades when the roles manifest is absent', async () => { + const { loadRolesManifest, RolesManifestMissingError } = await import('../roles.js'); + vi.mocked(loadRolesManifest).mockRejectedValueOnce( + new RolesManifestMissingError('/repo/manifest/roles.yaml'), + ); + + await pull({}); + + const { log } = await import('../utils/logger.js'); + expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('Roles manifest not found')); + }); + + it('fails the scope instead of delivering everything when the roles manifest is malformed', async () => { + // A manifest that exists but does not parse cannot degrade to "no filter": + // that hands out exactly the namespaces it was written to gate. const { loadRolesManifest } = await import('../roles.js'); - vi.mocked(loadRolesManifest).mockRejectedValueOnce(new Error('Invalid roles manifest')); + vi.mocked(loadRolesManifest).mockRejectedValueOnce( + new Error("Invalid roles manifest: roles.0.resources.skills.0: resource namespace must be a single path segment"), + ); await pull({}); + // pull logs the manifest error and returns before any resource is written, + // which is what an invalid projects manifest already does. const { log } = await import('../utils/logger.js'); - expect(log.warn).toHaveBeenCalledWith(expect.stringContaining('Could not load roles manifest')); + expect(log.error).toHaveBeenCalledWith(expect.stringContaining('Invalid roles manifest')); }); it('aborts pull when the same skill exists in multiple active namespaces', async () => { diff --git a/src/resource-namespaces.ts b/src/resource-namespaces.ts index a4dccd8c..afe1f930 100644 --- a/src/resource-namespaces.ts +++ b/src/resource-namespaces.ts @@ -1,5 +1,10 @@ import type { LocalConfig } from './types.js'; -import { loadRolesManifest, resolveRoleResourceNamespaces, type ResourceNamespaces } from './roles.js'; +import { + loadRolesManifest, + resolveRoleResourceNamespaces, + RolesManifestMissingError, + type ResourceNamespaces, +} from './roles.js'; import { loadProjectsManifest, resolveProjectResourceNamespaces, mergeNamespaces } from './projects.js'; import { log } from './utils/logger.js'; @@ -34,8 +39,14 @@ export async function resolveResourceNamespaces(localConfig: LocalConfig) { let rolesManifest; try { rolesManifest = await loadRolesManifest(localConfig.repo.localPath); - } catch { - log.warn('Could not load roles manifest. Skipping role-based filtering.'); + } catch (error) { + // Only an ABSENT manifest degrades to unfiltered delivery. One that exists + // and does not parse must not: every path below this point would treat the + // roles as "no filter" and deliver the namespaces the manifest was written + // to gate. Let it fail the scope's pull, as an invalid projects manifest + // already does. + if (!(error instanceof RolesManifestMissingError)) throw error; + log.warn('Roles manifest not found. Skipping role-based filtering.'); rolesManifest = null; } if (rolesManifest) { diff --git a/src/roles.ts b/src/roles.ts index b6c757f2..dbc31b6c 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -95,11 +95,24 @@ function validateManifestShape(raw: unknown): RolesManifest { return manifest; } +/** + * The manifest file is absent: a team that does not use roles, not a broken one. + * Callers that fall back to unfiltered delivery must react to this case ONLY — + * doing the same for a manifest that exists but does not parse would hand out + * every namespace the manifest was written to gate. + */ +export class RolesManifestMissingError extends Error { + constructor(manifestPath: string) { + super(`Roles manifest not found: ${manifestPath}`); + this.name = 'RolesManifestMissingError'; + } +} + export async function loadRolesManifest(repoPath: string): Promise { const manifestPath = path.join(repoPath, 'manifest', 'roles.yaml'); const content = await readFileSafe(manifestPath); if (!content) { - throw new Error(`Roles manifest not found: ${manifestPath}`); + throw new RolesManifestMissingError(manifestPath); } let raw: unknown; From 32efce706f5961fc6366076495ab5e88356dc9e9 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 13:41:22 +0200 Subject: [PATCH 10/25] fix(manifest): narrow 'absent' to ENOENT, refuse Windows device names Review on #710, two of the three findings; the third was a stale read of the PR description, which the e2e section had already been rewritten to match and which is now updated before the push rather than after. readFileSafe returns null for every read failure and for an empty file, so an unreadable roles.yaml was indistinguishable from one that was never written -- and 'never written' is the one case allowed to relax role filtering. projects.yaml had the same hole, where a null manifest means 'this team is not partitioned'. Both loaders now read the file directly: ENOENT is absence, and a permission error, a directory or an empty file is an error that fails the pull. Windows opens a device for CON, NUL, AUX, PRN, COM0-9 and LPT0-9 in every directory, extension or not, so a namespace spelled that way cannot be the directory the manifest names. A name that merely starts like one (console, community) is untouched, and the project id stays out of this rule as it stays out of the others: it is a working POSIX name the id rule has always accepted. --- CHANGELOG.md | 3 +- docs/designs/multi-project-management.md | 11 +++--- docs/usage-guide.md | 16 +++++---- docs/usage-guide.zh-CN.md | 15 +++++---- src/__tests__/projects.test.ts | 43 ++++++++++++++++++++++++ src/__tests__/roles.test.ts | 20 +++++++++++ src/manifest-schema.ts | 37 ++++++++++++++++++-- src/projects.ts | 8 +++-- src/roles.ts | 24 ++++++++----- 9 files changed, 144 insertions(+), 33 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3c28ed95..89862bb9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,8 @@ All notable changes to this project will be documented in this file. See [standa ### 🐛 Bug Fixes - `teamai init` no longer hangs without a terminal. When the provider had no session it spawned `gh auth login --web` (or `gf auth login`, `cnb login`) with inherited stdio and waited for a browser device flow that nobody could complete, about five minutes for GitHub, then exited with the provider's error and no hint of the missing credential. Each login now refuses up front when the run is not interactive and names the credential to prepare (`GITHUB_TOKEN` / `GH_TOKEN`, `CNB_TOKEN`, or for TGit a prior `gf auth login`, since a `TGIT_TOKEN` PAT is REST-API-only and cannot clone). A run is non-interactive when stdin is not a TTY or when `CI` or `TEAMAI_NONINTERACTIVE` is set, so an agent sandbox with a pseudo-terminal can declare itself unattended, and every prompt in the CLI follows the same rule. `git` also runs with its prompts closed in that case — `GIT_TERMINAL_PROMPT=0`, `GIT_ASKPASS=echo` and `GCM_INTERACTIVE=never`, each only where the caller set nothing — so a missing clone credential fails at once instead of waiting on a terminal prompt or an askpass or credential-manager dialog. `ssh` keeps its own settings: its batch flag is only reachable through `GIT_SSH_COMMAND`, which would override each repository's `core.sshCommand` (for [#711](https://github.com/Tencent/teamai-cli/issues/711)). -- `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil`, `a/b`, `C:evil`, a bare `..`, and any name with a trailing `.` or space — which Win32 strips, making `.. ` arrive as `..` and `frontend.` as `frontend` — under `resources:` no longer parse; the error names the offending entry. Only traversal is rejected: a namespace that is a plain directory name still parses, non-ASCII names and names with a space included. A manifest that fails to parse now reports the offending entry on one line (`Invalid projects manifest: projects.0.resources.skills.1: ...`) instead of dumping a raw validation object. +- `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil`, `a/b`, `C:evil`, a bare `..`, any name with a trailing `.` or space — which Win32 strips, making `.. ` arrive as `..` and `frontend.` as `frontend` — and a Windows device name such as `CON` or `COM1` under `resources:` no longer parse; the error names the offending entry. Only traversal is rejected: a namespace that is a plain directory name still parses, non-ASCII names and names with a space included. A manifest that fails to parse now reports the offending entry on one line (`Invalid projects manifest: projects.0.resources.skills.1: ...`) instead of dumping a raw validation object. +- A `manifest/roles.yaml` that exists but does not parse now fails the pull for that scope instead of warning and syncing with no role filter at all. For a member with no active project that fallback meant an unfiltered sync, so a broken manifest delivered every namespace it was written to gate. Only an absent manifest still means "this team does not use roles"; an unreadable or empty file is an error, as it now is for `manifest/projects.yaml` too. - Cache GC now rejects partial integers such as `12abc`, decimals, zero and unsafe integers for `--max-bytes` and `--stale-days` before deleting anything. An invalid `TEAMAI_CACHE_MAX_BYTES` value falls back to the default 5 GB limit instead of using a numeric prefix. - Usage reporting scopes sessions by path on Windows too. The project/user scope filter compared an event's `cwd` against `projectRoot` with a hard-coded `/` separator, so on Windows only a session started in the project root itself matched: every session started in a subdirectory was dropped from the project team's report and counted in the user scope's instead, which is the isolation the usage guide promises. Windows paths are also compared case-insensitively, so a drive letter or a directory name spelled with different case in the two sources no longer leaks a project session into the user scope. POSIX paths keep their own rules: case-sensitive, and a `\` in a filename stays part of the name. - `teamai tags subscribe` and `teamai tags unsubscribe` now invalidate the pull revision cache, as `teamai skill exclude` already does, so the next `teamai pull` applies the new subscriptions instead of reporting "Already synced" when the team repo has not changed. diff --git a/docs/designs/multi-project-management.md b/docs/designs/multi-project-management.md index 6b247572..1e763c7e 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -84,10 +84,13 @@ projects: The id and every namespace are refused at the manifest boundary unless they can name a directory without escaping it, since each becomes a directory component. A namespace must be a single path segment: no `/`, `\`, `:` or control character, -and no trailing `.` or space (Win32 strips those from every component, so `.. ` -would arrive as `..` and `frontend.` as `frontend`, escaping the parent in the -first case and another namespace's directory in the second; `.` and `..` fall out -of the same rule). The id keeps the +no trailing `.` or space, and not a Windows device name (`CON`, `NUL`, `COM1`, …). +Win32 strips a trailing period or space from every component, so `.. ` would +arrive as `..` and `frontend.` as `frontend`, escaping the parent in the first +case and another namespace's directory in the second; `.` and `..` fall out of the +same rule. A manifest file that exists but cannot be read, or is empty, is an +error rather than an absent manifest: treating it as absent would drop the +filtering the manifest exists to apply. The id keeps the older, narrower rule it has always had — letters, digits, `.`, `_`, `-`, and not `.` or `..` — because it is also typed on the command line and split on commas. The namespace guard applies to `manifest/roles.yaml`'s active namespaces diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 772b6514..d2a27e11 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -248,11 +248,13 @@ The project id and every namespace under `resources:` become a directory name neither may escape the directory it names. A **namespace** must be a single path segment: no `/`, `\`, `:` or control -character, and no trailing `.` or space. Windows strips those from every path -component, so `.. ` would arrive as `..` and escape the parent, and `frontend.` -would arrive as `frontend` and land in another namespace's directory; the rule -also rules out `.` and `..`. Anything else a filesystem accepts stays valid — a -non-ASCII name, or one holding a space inside it. +character, no trailing `.` or space, and not a Windows device name (`CON`, `NUL`, +`AUX`, `PRN`, `COM0`–`COM9`, `LPT0`–`LPT9`, with or without an extension). +Windows strips a trailing period or space from every path component, so `.. ` +would arrive as `..` and escape the parent while `frontend.` would arrive as +`frontend` and land in another namespace's directory; the same rule rules out `.` +and `..`. Anything else a filesystem accepts stays valid — a non-ASCII name, one +holding a space inside it, or one that merely starts like a device (`console`). A **project id** keeps its own older and narrower rule, because it is also typed on the command line and split on commas: letters, digits, `.`, `_` and `-`, and @@ -1499,8 +1501,8 @@ roles: Every namespace that takes effect — `knowledge`, `skills` and `agents` — becomes a directory name, so it must be a single path segment: no `/`, `\`, `:` or control -character, and no trailing `.` or space, in `manifest/roles.yaml` exactly as in -`manifest/projects.yaml`. A role's +character, no trailing `.` or space, and not a Windows device name, in +`manifest/roles.yaml` exactly as in `manifest/projects.yaml`. A role's `learnings:` is accepted for backward compatibility and ignored at runtime (learnings are namespaced by project, not by role), so it names no directory and is not checked. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index dd6091fe..fb70d079 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -232,11 +232,12 @@ projects: （`skills//`、`learnings//`、`agents//`），因此 都不能越出自己命名的目录。 -**namespace** 必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，且结尾不能是 -`.` 或空格。Windows 会从每个路径片段删除结尾的句点与空格，因此 `.. ` 最终变成 -`..` 越出上级目录，`frontend.` 最终变成 `frontend` 落进另一个 namespace 的目录； -该规则同时排除了 `.` 与 `..`。除此之外不受限制 —— 非 ASCII 名称、名称中间含空格 -的目录仍然合法。 +**namespace** 必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，结尾不能是 `.` +或空格，也不能是 Windows 设备名（`CON`、`NUL`、`AUX`、`PRN`、`COM0`–`COM9`、 +`LPT0`–`LPT9`，带不带扩展名都算）。Windows 会从每个路径片段删除结尾的句点与空格， +因此 `.. ` 最终变成 `..` 越出上级目录，`frontend.` 最终变成 `frontend` 落进另一个 +namespace 的目录；该规则同时排除了 `.` 与 `..`。除此之外不受限制 —— 非 ASCII 名称、 +名称中间含空格的目录、以及只是以设备名开头的名称（如 `console`）仍然合法。 **项目 id** 沿用它原有的、更严格的规则，因为它还会在命令行中输入并按逗号切分： 只允许字母、数字、`.`、`_` 和 `-`，且不能是 `.` 或 `..`。 @@ -1455,8 +1456,8 @@ roles: ``` 真正生效的 namespace（`knowledge`、`skills`、`agents`）都会成为目录名，因此必须是 -单个路径片段：不含 `/`、`\`、`:` 和控制字符，且结尾不能是 `.` 或空格， -`manifest/roles.yaml` 与 `manifest/projects.yaml` 规则一致。role 的 +单个路径片段：不含 `/`、`\`、`:` 和控制字符，结尾不能是 `.` 或空格，也不能是 +Windows 设备名，`manifest/roles.yaml` 与 `manifest/projects.yaml` 规则一致。role 的 `learnings:` 仅为向后兼容而保留、运行时忽略（learnings 按 project 而非 role 划分 namespace），不会成为目录名，因此不做校验。 diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index a637dd1a..94510072 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -158,6 +158,9 @@ projects: // Win32 strips the trailing character here too, so each of these is // `frontend` on that filesystem — another namespace's directory. 'frontend.', 'frontend ', 'frontend..', + // Windows opens a device for these in any directory, with or without an + // extension, so they cannot name the directory the manifest means. + 'CON', 'con', 'NUL', 'aux', 'COM1', 'lpt9', 'CON.txt', ]) { const repoDir = writeManifest(` version: 1 @@ -208,6 +211,46 @@ projects: } }); + it('keeps a name that merely starts like a device name', async () => { + const repoDir = writeManifest(` +version: 1 +projects: + - id: alpha + resources: { skills: [console, connect, community, complex, nullable] } +`); + try { + const manifest = await loadProjectsManifest(repoDir); + expect(manifest?.projects[0].resources.skills).toEqual([ + 'console', 'connect', 'community', 'complex', 'nullable', + ]); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + + it('treats an empty manifest as broken, not as an absent one', async () => { + // `readFileSafe` returned null for an empty or unreadable file just as it did + // for a missing one, and a null manifest means "this team has no projects" — + // i.e. no project filtering at all. Only ENOENT may mean that. + const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-projempty-')); + try { + mkdirSync(path.join(repoDir, 'manifest'), { recursive: true }); + writeFileSync(path.join(repoDir, 'manifest', 'projects.yaml'), ' \n'); + await expect(loadProjectsManifest(repoDir)).rejects.toThrow(/is empty/i); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + + it('returns null only when the manifest file is absent', async () => { + const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-projnone-')); + try { + expect(await loadProjectsManifest(repoDir)).toBeNull(); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('names the offending entry instead of dumping a raw ZodError', async () => { const repoDir = writeManifest(` version: 1 diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index a80e4b9c..e6e9c6d5 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -11,6 +11,7 @@ import { resolveRoleResourceNamespaces, activeRoleIds, loadRolesManifestIfPresent, + RolesManifestMissingError, } from '../roles.js'; import type { RolesManifest } from '../roles.js'; @@ -131,6 +132,7 @@ roles: 'a\u0009b', 'a\u007fb', 'a\u0085b', '.. ', '.. .', '...', '. ', ' ', 'frontend.', 'frontend ', 'frontend..', + 'CON', 'nul', 'COM1', 'CON.txt', ]) { const repoDir = writeManifest(` version: 1 @@ -146,6 +148,24 @@ roles: } }); + it('reports an empty manifest as broken rather than missing', async () => { + // Only ENOENT may become RolesManifestMissingError: that is the one case the + // pull is allowed to treat as "this team has no roles" and stop filtering. + const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-rolesempty-')); + mkdirSync(path.join(repoDir, 'manifest'), { recursive: true }); + writeFileSync(path.join(repoDir, 'manifest', 'roles.yaml'), '\n'); + + await expect(loadRolesManifest(repoDir)).rejects.toThrow(/is empty/i); + await expect(loadRolesManifest(repoDir)).rejects.not.toBeInstanceOf(RolesManifestMissingError); + rmSync(repoDir, { recursive: true, force: true }); + }); + + it('reports an absent manifest with the typed missing error', async () => { + const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-rolesnone-')); + await expect(loadRolesManifest(repoDir)).rejects.toBeInstanceOf(RolesManifestMissingError); + rmSync(repoDir, { recursive: true, force: true }); + }); + it('names the offending entry instead of dumping a raw ZodError', async () => { const repoDir = writeManifest(` version: 1 diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index f4d67a33..4d5099f2 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -1,3 +1,4 @@ +import fs from 'node:fs/promises'; import { z } from 'zod'; /** @@ -25,14 +26,25 @@ const UNSAFE_SEGMENT = /[/\\:\u0000-\u001f\u007f-\u009f]/; // Refusing the trailing character covers both, and `.`/`..` fall out of it. const TRAILING_DOT_OR_SPACE = /[ .]$/; +// Windows reserves these names for devices in every directory, extension or not: +// `CON`, `NUL`, `COM1`, `CON.txt` all open a device rather than a file, so a +// namespace spelled that way cannot be the directory the manifest means. The +// project id is deliberately left out of this, the way it is left out of the +// rules above: it is a working POSIX directory name that the id rule has always +// accepted, and narrowing it would break manifests that parse today. +const WINDOWS_DEVICE_NAME = /^(con|prn|aux|nul|com[0-9]|lpt[0-9])(\.|$)/i; + /** True if `seg` is safe to use as a single path segment (no separators, no `..`). */ export function isSafeNamespaceSegment(seg: string): boolean { - return seg.length > 0 && !UNSAFE_SEGMENT.test(seg) && !TRAILING_DOT_OR_SPACE.test(seg); + return seg.length > 0 + && !UNSAFE_SEGMENT.test(seg) + && !TRAILING_DOT_OR_SPACE.test(seg) + && !WINDOWS_DEVICE_NAME.test(seg); } /** A resource namespace: one path segment that cannot escape its parent. */ export const NamespaceSegmentSchema = z.string().min(1).refine(isSafeNamespaceSegment, { - message: "resource namespace must be a single path segment (no '/', '\\', ':' or control characters, and no trailing '.' or space, which also rules out '.' and '..')", + message: "resource namespace must be a single path segment (no '/', '\\', ':' or control characters, no trailing '.' or space, which also rules out '.' and '..', and not a Windows device name such as 'CON' or 'COM1')", }); /** @@ -49,3 +61,24 @@ export function parseManifest(schema: S, raw: unknown, k .join('; '); throw new Error(`Invalid ${kind} manifest: ${detail}`); } + +/** + * Read a manifest file, separating "there is no such file" from every other + * reason a read can fail. `readFileSafe` collapses the two into `null`, and a + * caller that treats `null` as "this team does not use roles/projects" would + * then drop its filtering because the file is unreadable or empty — the fail-open + * direction. Absence returns `null` here; anything else throws. + */ +export async function readManifestFile(manifestPath: string, kind: 'projects' | 'roles'): Promise { + let content: string; + try { + content = await fs.readFile(manifestPath, 'utf-8'); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; + throw new Error(`Could not read ${kind} manifest ${manifestPath}: ${(error as Error).message}`); + } + if (content.trim() === '') { + throw new Error(`Invalid ${kind} manifest: ${manifestPath} is empty. Delete it, or give it a version and a ${kind} list.`); + } + return content; +} diff --git a/src/projects.ts b/src/projects.ts index a6120c9c..682124e9 100644 --- a/src/projects.ts +++ b/src/projects.ts @@ -1,8 +1,8 @@ import path from 'node:path'; import YAML from 'yaml'; import { z } from 'zod'; -import { readFileIfExists, ensureDir, writeFile } from './utils/fs.js'; -import { NamespaceSegmentSchema, parseManifest } from './manifest-schema.js'; +import { ensureDir, writeFile } from './utils/fs.js'; +import { NamespaceSegmentSchema, parseManifest, readManifestFile } from './manifest-schema.js'; import type { ResourceNamespaces } from './roles.js'; /** @@ -113,7 +113,9 @@ function validateManifestShape(raw: unknown): ProjectsManifest { */ export async function loadProjectsManifest(repoPath: string): Promise { const manifestPath = path.join(repoPath, 'manifest', 'projects.yaml'); - const content = await readFileIfExists(manifestPath); + // Only an absent file means "this team has no projects": an unreadable or empty + // one throws, so the pull fails rather than quietly syncing as if unpartitioned. + const content = await readManifestFile(manifestPath, 'projects'); if (content === null) { return null; } diff --git a/src/roles.ts b/src/roles.ts index dbc31b6c..cafa6e32 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -1,9 +1,9 @@ import path from 'node:path'; import YAML from 'yaml'; import { z } from 'zod'; -import { readFileSafe, readFileIfExists, ensureDir, writeFile } from './utils/fs.js'; +import { ensureDir, writeFile } from './utils/fs.js'; import { log } from './utils/logger.js'; -import { NamespaceSegmentSchema, parseManifest } from './manifest-schema.js'; +import { NamespaceSegmentSchema, parseManifest, readManifestFile } from './manifest-schema.js'; const ROLE_RESOURCE_TYPES = ['knowledge', 'skills', 'agents'] as const; @@ -110,8 +110,11 @@ export class RolesManifestMissingError extends Error { export async function loadRolesManifest(repoPath: string): Promise { const manifestPath = path.join(repoPath, 'manifest', 'roles.yaml'); - const content = await readFileSafe(manifestPath); - if (!content) { + // Absence is the only case that may relax filtering downstream, so it is the + // only one that becomes RolesManifestMissingError: an unreadable or empty file + // throws a plain error and fails the pull. + const content = await readManifestFile(manifestPath, 'roles'); + if (content === null) { throw new RolesManifestMissingError(manifestPath); } @@ -134,11 +137,14 @@ export async function loadRolesManifest(repoPath: string): Promise { - const manifestPath = path.join(repoPath, 'manifest', 'roles.yaml'); - // 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); + // Only absence relaxes to null: a manifest that exists but cannot be read or + // parsed is a failure to report, not a team without roles. + try { + return await loadRolesManifest(repoPath); + } catch (error) { + if (error instanceof RolesManifestMissingError) return null; + throw error; + } } export async function saveRolesManifest(repoPath: string, manifest: RolesManifest): Promise { From 83cb66cde6d253758c5ae88b04beb5a816c26a2a Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 13:49:41 +0200 Subject: [PATCH 11/25] fix(manifest): narrow every roles fallback, drop COM0/LPT0 from the device set Review on #710. The fail-closed change covered resolveResourceNamespaces but not the other callers that fall back when the loader throws, so a malformed manifest still reached an unfiltered sync by another route: bootstrap.ts left the member role-less while auto-selecting the sole role, resources/skills.ts and push.ts guessed the namespaces from the role ids, and config.ts skipped the legacy migration and left the role unset. Each now reacts to RolesManifestMissingError alone. roles-cmd.ts keeps its broad catches on purpose: those commands report the error to the person running them instead of deciding what to deliver. Windows reserves COM1-COM9 and LPT1-LPT9, not COM0/LPT0, so the guard was rejecting two ordinary directory names for no safety gain. Both are now covered by the test that pins 'console' and 'community' as valid. --- CHANGELOG.md | 2 +- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 4 ++-- src/__tests__/projects.test.ts | 8 +++++--- src/bootstrap.ts | 10 ++++++++-- src/config.ts | 7 +++++-- src/manifest-schema.ts | 9 ++++++--- src/push.ts | 7 +++++-- src/resources/skills.ts | 9 ++++++--- 9 files changed, 39 insertions(+), 19 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 89862bb9..90e3a0c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -23,7 +23,7 @@ All notable changes to this project will be documented in this file. See [standa - `teamai init` no longer hangs without a terminal. When the provider had no session it spawned `gh auth login --web` (or `gf auth login`, `cnb login`) with inherited stdio and waited for a browser device flow that nobody could complete, about five minutes for GitHub, then exited with the provider's error and no hint of the missing credential. Each login now refuses up front when the run is not interactive and names the credential to prepare (`GITHUB_TOKEN` / `GH_TOKEN`, `CNB_TOKEN`, or for TGit a prior `gf auth login`, since a `TGIT_TOKEN` PAT is REST-API-only and cannot clone). A run is non-interactive when stdin is not a TTY or when `CI` or `TEAMAI_NONINTERACTIVE` is set, so an agent sandbox with a pseudo-terminal can declare itself unattended, and every prompt in the CLI follows the same rule. `git` also runs with its prompts closed in that case — `GIT_TERMINAL_PROMPT=0`, `GIT_ASKPASS=echo` and `GCM_INTERACTIVE=never`, each only where the caller set nothing — so a missing clone credential fails at once instead of waiting on a terminal prompt or an askpass or credential-manager dialog. `ssh` keeps its own settings: its batch flag is only reachable through `GIT_SSH_COMMAND`, which would override each repository's `core.sshCommand` (for [#711](https://github.com/Tencent/teamai-cli/issues/711)). - `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil`, `a/b`, `C:evil`, a bare `..`, any name with a trailing `.` or space — which Win32 strips, making `.. ` arrive as `..` and `frontend.` as `frontend` — and a Windows device name such as `CON` or `COM1` under `resources:` no longer parse; the error names the offending entry. Only traversal is rejected: a namespace that is a plain directory name still parses, non-ASCII names and names with a space included. A manifest that fails to parse now reports the offending entry on one line (`Invalid projects manifest: projects.0.resources.skills.1: ...`) instead of dumping a raw validation object. -- A `manifest/roles.yaml` that exists but does not parse now fails the pull for that scope instead of warning and syncing with no role filter at all. For a member with no active project that fallback meant an unfiltered sync, so a broken manifest delivered every namespace it was written to gate. Only an absent manifest still means "this team does not use roles"; an unreadable or empty file is an error, as it now is for `manifest/projects.yaml` too. +- A `manifest/roles.yaml` that exists but does not parse now fails the pull for that scope instead of warning and syncing with no role filter at all. The same applies to `init`, `push` and the legacy role migration, which each fell back to a guess at the namespaces when any error came out of the loader. For a member with no active project that fallback meant an unfiltered sync, so a broken manifest delivered every namespace it was written to gate. Only an absent manifest still means "this team does not use roles"; an unreadable or empty file is an error, as it now is for `manifest/projects.yaml` too. - Cache GC now rejects partial integers such as `12abc`, decimals, zero and unsafe integers for `--max-bytes` and `--stale-days` before deleting anything. An invalid `TEAMAI_CACHE_MAX_BYTES` value falls back to the default 5 GB limit instead of using a numeric prefix. - Usage reporting scopes sessions by path on Windows too. The project/user scope filter compared an event's `cwd` against `projectRoot` with a hard-coded `/` separator, so on Windows only a session started in the project root itself matched: every session started in a subdirectory was dropped from the project team's report and counted in the user scope's instead, which is the isolation the usage guide promises. Windows paths are also compared case-insensitively, so a drive letter or a directory name spelled with different case in the two sources no longer leaks a project session into the user scope. POSIX paths keep their own rules: case-sensitive, and a `\` in a filename stays part of the name. - `teamai tags subscribe` and `teamai tags unsubscribe` now invalidate the pull revision cache, as `teamai skill exclude` already does, so the next `teamai pull` applies the new subscriptions instead of reporting "Already synced" when the team repo has not changed. diff --git a/docs/usage-guide.md b/docs/usage-guide.md index d2a27e11..098204f1 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -249,7 +249,7 @@ neither may escape the directory it names. A **namespace** must be a single path segment: no `/`, `\`, `:` or control character, no trailing `.` or space, and not a Windows device name (`CON`, `NUL`, -`AUX`, `PRN`, `COM0`–`COM9`, `LPT0`–`LPT9`, with or without an extension). +`AUX`, `PRN`, `COM1`–`COM9`, `LPT1`–`LPT9`, with or without an extension). Windows strips a trailing period or space from every path component, so `.. ` would arrive as `..` and escape the parent while `frontend.` would arrive as `frontend` and land in another namespace's directory; the same rule rules out `.` diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index fb70d079..58bde460 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -233,8 +233,8 @@ projects: 都不能越出自己命名的目录。 **namespace** 必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，结尾不能是 `.` -或空格，也不能是 Windows 设备名（`CON`、`NUL`、`AUX`、`PRN`、`COM0`–`COM9`、 -`LPT0`–`LPT9`，带不带扩展名都算）。Windows 会从每个路径片段删除结尾的句点与空格， +或空格，也不能是 Windows 设备名（`CON`、`NUL`、`AUX`、`PRN`、`COM1`–`COM9`、 +`LPT1`–`LPT9`，带不带扩展名都算）。Windows 会从每个路径片段删除结尾的句点与空格， 因此 `.. ` 最终变成 `..` 越出上级目录，`frontend.` 最终变成 `frontend` 落进另一个 namespace 的目录；该规则同时排除了 `.` 与 `..`。除此之外不受限制 —— 非 ASCII 名称、 名称中间含空格的目录、以及只是以设备名开头的名称（如 `console`）仍然合法。 diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index 94510072..6c8693d8 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -211,17 +211,19 @@ projects: } }); - it('keeps a name that merely starts like a device name', async () => { + it('keeps a name that merely starts like a device name, and the unreserved COM0/LPT0', async () => { const repoDir = writeManifest(` version: 1 projects: - id: alpha - resources: { skills: [console, connect, community, complex, nullable] } + resources: { skills: [console, connect, community, complex, nullable, COM0, LPT0] } `); try { const manifest = await loadProjectsManifest(repoDir); + // COM0 and LPT0 are ordinary names: Windows reserves COM1-COM9 and + // LPT1-LPT9 only, so rejecting them would cost compatibility for nothing. expect(manifest?.projects[0].resources.skills).toEqual([ - 'console', 'connect', 'community', 'complex', 'nullable', + 'console', 'connect', 'community', 'complex', 'nullable', 'COM0', 'LPT0', ]); } finally { rmSync(repoDir, { recursive: true, force: true }); diff --git a/src/bootstrap.ts b/src/bootstrap.ts index 885a7655..d5b064cc 100644 --- a/src/bootstrap.ts +++ b/src/bootstrap.ts @@ -172,8 +172,14 @@ export async function bootstrapSelfRepo( localConfig.primaryRole = manifest.roles[0].id; localConfig.resourceProfileVersion = manifest.version; } - } catch { - // no roles manifest — leave role unset + } catch (error) { + // No manifest: leave the role unset, as a repo without roles intends. A + // manifest that exists and does not parse is different — swallowing it + // would leave the role unset too, and a member with no role and no project + // gets an unfiltered sync, which is the opposite of what the broken + // manifest asked for. + const { RolesManifestMissingError } = await import('./roles.js'); + if (!(error instanceof RolesManifestMissingError)) throw error; } await ensureDir(localPath); diff --git a/src/config.ts b/src/config.ts index 9aeab136..8c4b0eef 100644 --- a/src/config.ts +++ b/src/config.ts @@ -19,7 +19,7 @@ import { readFileSafe, readJson, writeFile, writeJson, expandHome, pathExists } import { resolveAnchors } from './utils/git.js'; import { resolvePartitionDir, writeAnchorFile } from './utils/partition.js'; import { log } from './utils/logger.js'; -import { loadRolesManifest } from './roles.js'; +import { loadRolesManifest, RolesManifestMissingError } from './roles.js'; async function migrateLegacyRoleConfig(config: LocalConfig, configPath: string): Promise { if (config.primaryRole) { @@ -29,7 +29,10 @@ async function migrateLegacyRoleConfig(config: LocalConfig, configPath: string): let manifest; try { manifest = await loadRolesManifest(config.repo.localPath); - } catch { + } catch (error) { + // A repo with no manifest has nothing to migrate. A broken one leaves the + // config role-less, which downstream reads as "no filter", so it surfaces. + if (!(error instanceof RolesManifestMissingError)) throw error; return config; } diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index 4d5099f2..20f31cc7 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -28,11 +28,14 @@ const TRAILING_DOT_OR_SPACE = /[ .]$/; // Windows reserves these names for devices in every directory, extension or not: // `CON`, `NUL`, `COM1`, `CON.txt` all open a device rather than a file, so a -// namespace spelled that way cannot be the directory the manifest means. The -// project id is deliberately left out of this, the way it is left out of the +// namespace spelled that way cannot be the directory the manifest means. The set +// is `CON`, `PRN`, `AUX`, `NUL`, `COM1`-`COM9` and `LPT1`-`LPT9`; `COM0` and +// `LPT0` are ordinary names and keep parsing. +// +// The project id is deliberately left out of this, the way it is left out of the // rules above: it is a working POSIX directory name that the id rule has always // accepted, and narrowing it would break manifests that parse today. -const WINDOWS_DEVICE_NAME = /^(con|prn|aux|nul|com[0-9]|lpt[0-9])(\.|$)/i; +const WINDOWS_DEVICE_NAME = /^(con|prn|aux|nul|com[1-9]|lpt[1-9])(\.|$)/i; /** True if `seg` is safe to use as a single path segment (no separators, no `..`). */ export function isSafeNamespaceSegment(seg: string): boolean { diff --git a/src/push.ts b/src/push.ts index 4934a72f..57791ab8 100644 --- a/src/push.ts +++ b/src/push.ts @@ -21,7 +21,7 @@ import type { import { getDataHome, SYNC_LOCK_FILENAME } from './types.js'; import { acquireLock, releaseLock } from './update.js'; import { assertSafePath, assertSafeResourceName, defaultAllowedRoots } from './utils/path-safety.js'; -import { loadRolesManifest, resolveRoleResourceNamespaces } from './roles.js'; +import { loadRolesManifest, resolveRoleResourceNamespaces, RolesManifestMissingError } from './roles.js'; import { askQuestion, askSelection } from './utils/prompt.js'; import { pathExists, pruneEmptyDirs, readFileSafe, writeFile } from './utils/fs.js'; @@ -77,7 +77,10 @@ async function resolveSkillNamespaces( additionalRoles, }); return namespaces.skills; - } catch { + } catch (error) { + // Only an absent manifest falls back to the role id as the namespace; a + // broken one would send the push into a namespace nothing validated. + if (!(error instanceof RolesManifestMissingError)) throw error; return [primaryRole]; } } diff --git a/src/resources/skills.ts b/src/resources/skills.ts index cdde62f6..c4a8ca41 100644 --- a/src/resources/skills.ts +++ b/src/resources/skills.ts @@ -8,7 +8,7 @@ import { log } from '../utils/logger.js'; import { BUILTIN_SKILL_NAMES } from '../builtin-skills.js'; import { resolveOpenclawWorkspaceDir } from '../openclaw-hooks.js'; import { getHermesHome } from '../hermes-home.js'; -import { loadRolesManifest, resolveRoleResourceNamespaces } from '../roles.js'; +import { loadRolesManifest, resolveRoleResourceNamespaces, RolesManifestMissingError } from '../roles.js'; import { assertWithinRoot } from '../utils/path-safety.js'; import { splitFrontmatter, stringifyFrontmatter } from '../utils/frontmatter.js'; @@ -271,8 +271,11 @@ async function resolveSkillNamespaces(localConfig: LocalConfig): Promise Date: Tue, 22 Sep 2026 13:56:12 +0200 Subject: [PATCH 12/25] fix(manifest): prove absence before trusting it, add the superscript devices Review on #710. ENOENT is not proof that a manifest is absent: a committed symlink whose target is missing reads exactly the same way, and absence is the one answer that lets a caller relax its filtering. The path is now lstat-ed before absence is believed, so a dangling link is an error like any other unreadable file. Windows reads the superscript forms of 1, 2 and 3 as device numbers, so COM and LPT followed by one of those join the ASCII-digit set. The third finding, that resolveResourceNamespaces returns before reading roles.yaml, is not a fail-open and is left as it is: that branch is reached only when the member has no role, and a role-less member gets the same unfiltered sync from a perfectly valid manifest, since every role namespace below is gated on primaryRole. Reading the manifest there would only add a new way for their pull to fail. The reasoning now sits in the code beside the early return. --- docs/usage-guide.md | 3 ++- docs/usage-guide.zh-CN.md | 2 +- src/__tests__/projects.test.ts | 20 +++++++++++++++++++- src/manifest-schema.ts | 15 ++++++++++++--- src/resource-namespaces.ts | 7 +++++++ 5 files changed, 41 insertions(+), 6 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 098204f1..5c069262 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -249,7 +249,8 @@ neither may escape the directory it names. A **namespace** must be a single path segment: no `/`, `\`, `:` or control character, no trailing `.` or space, and not a Windows device name (`CON`, `NUL`, -`AUX`, `PRN`, `COM1`–`COM9`, `LPT1`–`LPT9`, with or without an extension). +`AUX`, `PRN`, `COM1`–`COM9`, `LPT1`–`LPT9`, including the superscript forms +Windows also reads as device numbers, with or without an extension). Windows strips a trailing period or space from every path component, so `.. ` would arrive as `..` and escape the parent while `frontend.` would arrive as `frontend` and land in another namespace's directory; the same rule rules out `.` diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 58bde460..39b16710 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -234,7 +234,7 @@ projects: **namespace** 必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，结尾不能是 `.` 或空格，也不能是 Windows 设备名（`CON`、`NUL`、`AUX`、`PRN`、`COM1`–`COM9`、 -`LPT1`–`LPT9`，带不带扩展名都算）。Windows 会从每个路径片段删除结尾的句点与空格， +`LPT1`–`LPT9`，含 Windows 同样识别为设备编号的上标形式，带不带扩展名都算）。Windows 会从每个路径片段删除结尾的句点与空格， 因此 `.. ` 最终变成 `..` 越出上级目录，`frontend.` 最终变成 `frontend` 落进另一个 namespace 的目录；该规则同时排除了 `.` 与 `..`。除此之外不受限制 —— 非 ASCII 名称、 名称中间含空格的目录、以及只是以设备名开头的名称（如 `console`）仍然合法。 diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index 6c8693d8..51ddec3a 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, chmodSync } from 'node:fs'; +import { mkdtempSync, writeFileSync, rmSync, mkdirSync, chmodSync, symlinkSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; import { @@ -161,6 +161,8 @@ projects: // Windows opens a device for these in any directory, with or without an // extension, so they cannot name the directory the manifest means. 'CON', 'con', 'NUL', 'aux', 'COM1', 'lpt9', 'CON.txt', + // Windows reads the superscript forms as device numbers too. + 'COM\u00b9', 'LPT\u00b3', ]) { const repoDir = writeManifest(` version: 1 @@ -230,6 +232,22 @@ projects: } }); + it('treats a symlink with no target as broken, not as an absent manifest', async () => { + // A dangling link fails to read with ENOENT exactly as a missing file does, + // and "missing" is the one answer that lets a caller drop its filtering. + const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-projlink-')); + try { + mkdirSync(path.join(repoDir, 'manifest'), { recursive: true }); + symlinkSync( + path.join(repoDir, 'manifest', 'nowhere.yaml'), + path.join(repoDir, 'manifest', 'projects.yaml'), + ); + await expect(loadProjectsManifest(repoDir)).rejects.toThrow(/symbolic link with no target/i); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('treats an empty manifest as broken, not as an absent one', async () => { // `readFileSafe` returned null for an empty or unreadable file just as it did // for a missing one, and a null manifest means "this team has no projects" — diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index 20f31cc7..9a85411d 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -30,12 +30,14 @@ const TRAILING_DOT_OR_SPACE = /[ .]$/; // `CON`, `NUL`, `COM1`, `CON.txt` all open a device rather than a file, so a // namespace spelled that way cannot be the directory the manifest means. The set // is `CON`, `PRN`, `AUX`, `NUL`, `COM1`-`COM9` and `LPT1`-`LPT9`; `COM0` and -// `LPT0` are ordinary names and keep parsing. +// `LPT0` are ordinary names and keep parsing. Windows also reads the superscript +// forms of 1, 2 and 3 (U+00B9, U+00B2, U+00B3) as device numbers, so those go in +// with the ASCII digits. // // The project id is deliberately left out of this, the way it is left out of the // rules above: it is a working POSIX directory name that the id rule has always // accepted, and narrowing it would break manifests that parse today. -const WINDOWS_DEVICE_NAME = /^(con|prn|aux|nul|com[1-9]|lpt[1-9])(\.|$)/i; +const WINDOWS_DEVICE_NAME = /^(con|prn|aux|nul|(com|lpt)[1-9\u00b9\u00b2\u00b3])(\.|$)/i; /** True if `seg` is safe to use as a single path segment (no separators, no `..`). */ export function isSafeNamespaceSegment(seg: string): boolean { @@ -77,7 +79,14 @@ export async function readManifestFile(manifestPath: string, kind: 'projects' | try { content = await fs.readFile(manifestPath, 'utf-8'); } catch (error) { - if ((error as NodeJS.ErrnoException).code === 'ENOENT') return null; + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + // ENOENT is not proof of absence: a committed symlink pointing at a file + // that is not there reads the same way. `lstat` sees the link itself, so + // only a path that resolves to nothing at all counts as "no manifest". + const present = await fs.lstat(manifestPath).then(() => true, () => false); + if (!present) return null; + throw new Error(`Could not read ${kind} manifest ${manifestPath}: it is a symbolic link with no target.`); + } throw new Error(`Could not read ${kind} manifest ${manifestPath}: ${(error as Error).message}`); } if (content.trim() === '') { diff --git a/src/resource-namespaces.ts b/src/resource-namespaces.ts index afe1f930..3118767a 100644 --- a/src/resource-namespaces.ts +++ b/src/resource-namespaces.ts @@ -30,6 +30,13 @@ export async function resolveResourceNamespaces(localConfig: LocalConfig) { // through to an unfiltered sync that reinstalls every project's skills/rules. // So we return a real (possibly empty-active) context and let the cleanup path // below prune the now-inactive project namespaces. + // + // This returns before roles.yaml is read, and deliberately so: the branch is + // reached only when the member HAS NO ROLE, and a role-less member resolves to + // the same unfiltered sync when roles.yaml is perfectly valid — every role + // namespace below is gated on `primaryRole`. The manifest gates nothing for + // them, so reading it here could only add a new way for their pull to fail, + // never close a gap. A member WITH a role never reaches this line. if (!hasRole && !hasProjects && !teamHasProjects) return null; // ── Role namespaces (optional) ── From 4ea91e25a6d38ef08191ede1709afc0231eb3cf7 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 14:11:53 +0200 Subject: [PATCH 13/25] fix(init): a broken roles manifest must stop init, a skipped prompt must not Review on #710. Both init paths swallowed every role-selection failure and carried on without a role. A role-less config matches every role when hooks are reconciled, so a manifest that does not parse installed exactly the hooks it restricts. Narrowing the catch to RolesManifestMissingError alone was too much: the same block also absorbs a person skipping the role prompt, and a non-interactive run reaches it, so init would have started failing for anyone who does not pick a role. That case is now its own type, NoRoleSelectedError, and the two lenient cases are named while a parse failure or an unknown --role propagates. Both catch blocks read the same. Also: ENOENT proves nothing about absence when the DIRECTORY is a dangling link -- readFile and lstat on the file both report ENOENT -- so the path's components are walked, and the first link that leads nowhere is reported instead of being read as 'no manifest'. Verified in init.test.ts (malformed aborts and writes nothing, absent still initializes role-less) and against the real CLI in single-repo mode: no manifest exits 0 with the role unset, '../../evil' exits 1, and a valid manifest with --role sets primaryRole: frontend. --- docs/designs/multi-project-management.md | 4 ++- src/__tests__/init.test.ts | 45 ++++++++++++++++++++++++ src/__tests__/projects.test.ts | 13 +++++++ src/init.ts | 33 +++++++++++------ src/manifest-schema.ts | 35 ++++++++++++++---- 5 files changed, 113 insertions(+), 17 deletions(-) diff --git a/docs/designs/multi-project-management.md b/docs/designs/multi-project-management.md index 1e763c7e..e1a9627b 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -90,7 +90,9 @@ arrive as `..` and `frontend.` as `frontend`, escaping the parent in the first case and another namespace's directory in the second; `.` and `..` fall out of the same rule. A manifest file that exists but cannot be read, or is empty, is an error rather than an absent manifest: treating it as absent would drop the -filtering the manifest exists to apply. The id keeps the +filtering the manifest exists to apply. Absence means the path is genuinely not +there — a dangling symlink, on the file or on `manifest/` itself, reads as ENOENT +but is an error. The id keeps the older, narrower rule it has always had — letters, digits, `.`, `_`, `-`, and not `.` or `..` — because it is also typed on the command line and split on commas. The namespace guard applies to `manifest/roles.yaml`'s active namespaces diff --git a/src/__tests__/init.test.ts b/src/__tests__/init.test.ts index 12cc057b..25abde34 100644 --- a/src/__tests__/init.test.ts +++ b/src/__tests__/init.test.ts @@ -163,6 +163,9 @@ vi.mock('../roles.js', () => ({ describeRoles: vi.fn((roles: Array<{ id: string; name: string; description?: string }>) => roles.map((role) => role.description ? `${role.id} - ${role.name}: ${role.description}` : `${role.id} - ${role.name}`), ), + // The real class: init swallows ONLY this one, so the mock must carry the + // same identity for the distinction to be exercised. + RolesManifestMissingError: class RolesManifestMissingError extends Error {}, })); // Track pathExists calls to simulate directory states @@ -759,5 +762,47 @@ describe('init', () => { process.cwd(), ); }); + + it('aborts on a malformed roles manifest instead of initializing role-less', async () => { + // A role-less config matches every role when hooks are reconciled, so + // swallowing the parse failure would install exactly the hooks the manifest + // restricts. Only an ABSENT manifest may leave the role unset. + pathExistsFn = (p: string) => p.endsWith(`${path.sep}.git`) || p.endsWith('/.git'); + mockGit.raw.mockResolvedValue('https://git.example.com/group/repo.git\n'); + + const { loadRolesManifest } = await import('../roles.js'); + vi.mocked(loadRolesManifest).mockRejectedValueOnce( + new Error('Invalid roles manifest: roles.0.resources.skills.0: resource namespace must be a single path segment'), + ); + + const { saveLocalConfigForScope } = await import('../config.js'); + vi.mocked(saveLocalConfigForScope).mockClear(); + + // The error leaves init, so the CLI prints it and exits non-zero; nothing + // is written for the scope. + await expect(init({ repo: '.', dryRun: true })).rejects.toThrow(/Invalid roles manifest/); + expect(saveLocalConfigForScope).not.toHaveBeenCalled(); + }); + + it('still initializes with the role unset when there is no roles manifest', async () => { + pathExistsFn = (p: string) => p.endsWith(`${path.sep}.git`) || p.endsWith('/.git'); + mockGit.raw.mockResolvedValue('https://git.example.com/group/repo.git\n'); + + const { loadRolesManifest, RolesManifestMissingError } = await import('../roles.js'); + vi.mocked(loadRolesManifest).mockRejectedValueOnce( + new RolesManifestMissingError('/repo/.teamai/manifest/roles.yaml'), + ); + + const { saveLocalConfigForScope } = await import('../config.js'); + vi.mocked(saveLocalConfigForScope).mockClear(); + + await init({ repo: '.', dryRun: true }); + + expect(saveLocalConfigForScope).toHaveBeenCalledWith( + expect.not.objectContaining({ primaryRole: expect.anything() }), + 'project', + process.cwd(), + ); + }); }); }); diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index 51ddec3a..7cd84f29 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -232,6 +232,19 @@ projects: } }); + it('treats a dangling manifest/ directory link as broken too', async () => { + // Both readFile and lstat on the file give ENOENT when the DIRECTORY is the + // dangling link, so the whole path has to be walked before absence is + // believed — otherwise the team looks unpartitioned and filtering falls open. + const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-projdirlink-')); + try { + symlinkSync(path.join(repoDir, 'nowhere'), path.join(repoDir, 'manifest')); + await expect(loadProjectsManifest(repoDir)).rejects.toThrow(/symbolic link with no target/i); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('treats a symlink with no target as broken, not as an absent manifest', async () => { // A dangling link fails to read with ENOENT exactly as a missing file does, // and "missing" is the one answer that lets a caller drop its filtering. diff --git a/src/init.ts b/src/init.ts index 92831d2e..8eb02738 100644 --- a/src/init.ts +++ b/src/init.ts @@ -20,7 +20,7 @@ import { isRecallEnabled, } from './types.js'; import { getUserHome } from './utils/home.js'; -import { describeRoles, listRoleIds, loadRolesManifest } from './roles.js'; +import { describeRoles, listRoleIds, loadRolesManifest, RolesManifestMissingError } from './roles.js'; import { loadProjectsManifest, listProjectIds } from './projects.js'; import { getMemberConfig, mergeMemberConfig } from './members.js'; import { askQuestion, askConfirmation, askSelection, closePrompt, isInteractive } from './utils/prompt.js'; @@ -62,6 +62,13 @@ function parseRoleSelection(answer: string, max: number): number[] { return [...new Set(selections)]; } +/** + * The person did not pick a role at the prompt. Single-repo `init` treats this + * like a repo with no manifest — the role can be set later with `teamai roles + * set` — while every other caller lets it abort, as it always has. + */ +class NoRoleSelectedError extends Error {} + async function promptForRoleProfile( repoPath: string, roleFlag?: string, @@ -108,7 +115,7 @@ async function promptForRoleProfile( }); const [primaryIndex] = parseRoleSelection(primaryAnswer, manifest.roles.length); if (!primaryIndex) { - throw new Error('A primary role is required.'); + throw new NoRoleSelectedError('A primary role is required.'); } const primaryRole = manifest.roles[primaryIndex - 1]; @@ -433,10 +440,13 @@ export async function initHttp( try { Object.assign(localConfig, await promptForRoleProfile(localPath, options.role)); } catch (error) { - const msg = (error as Error).message; - if (!msg.includes('Roles manifest not found')) { - log.debug(`Role selection skipped: ${msg}`); - } + // Two cases leave the role unset on purpose: a repo with no roles manifest, + // and a person who skipped the prompt. Anything else — a manifest that does + // not parse, an unknown `--role` — must not be swallowed: a role-less config + // matches every role when hooks are reconciled, so it would install exactly + // the hooks the manifest restricts. + const lenient = error instanceof RolesManifestMissingError || error instanceof NoRoleSelectedError; + if (!lenient) throw error; } Object.assign(localConfig, await resolveActiveProjects(localPath, options.project)); @@ -876,10 +886,13 @@ export async function initSelfRepo(options: GlobalOptions & { try { Object.assign(localConfig, await promptForRoleProfile(localPath, options.role)); } catch (error) { - const msg = (error as Error).message; - if (!msg.includes('Roles manifest not found')) { - log.debug(`Role selection skipped: ${msg}`); - } + // Two cases leave the role unset on purpose: a repo with no roles manifest, + // and a person who skipped the prompt. Anything else — a manifest that does + // not parse, an unknown `--role` — must not be swallowed: a role-less config + // matches every role when hooks are reconciled, so it would install exactly + // the hooks the manifest restricts. + const lenient = error instanceof RolesManifestMissingError || error instanceof NoRoleSelectedError; + if (!lenient) throw error; } Object.assign(localConfig, await resolveActiveProjects(localPath, options.project)); // Which AI tools to set up in this repo (create skills dir + inject hooks + diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index 9a85411d..46525a90 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -1,4 +1,5 @@ import fs from 'node:fs/promises'; +import path from 'node:path'; import { z } from 'zod'; /** @@ -67,6 +68,31 @@ export function parseManifest(schema: S, raw: unknown, k throw new Error(`Invalid ${kind} manifest: ${detail}`); } +/** + * The first component of `target` that exists as a symbolic link pointing at + * nothing, or `null` when the path is simply not there. + * + * ENOENT is not proof of absence: a dangling link anywhere on the path — the + * file itself, or the `manifest/` directory — reads exactly like a file that was + * never written. Absence is the one answer that lets a caller drop its + * filtering, so it has to be the true one. `lstat` sees each link itself, and + * `stat` says whether it leads anywhere. + */ +async function danglingLinkOnPath(target: string): Promise { + let current = target; + for (;;) { + const link = await fs.lstat(current).catch(() => null); + if (link) { + if (!link.isSymbolicLink()) return null; + const resolves = await fs.stat(current).then(() => true, () => false); + return resolves ? null : current; + } + const parent = path.dirname(current); + if (parent === current) return null; + current = parent; + } +} + /** * Read a manifest file, separating "there is no such file" from every other * reason a read can fail. `readFileSafe` collapses the two into `null`, and a @@ -80,12 +106,9 @@ export async function readManifestFile(manifestPath: string, kind: 'projects' | content = await fs.readFile(manifestPath, 'utf-8'); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - // ENOENT is not proof of absence: a committed symlink pointing at a file - // that is not there reads the same way. `lstat` sees the link itself, so - // only a path that resolves to nothing at all counts as "no manifest". - const present = await fs.lstat(manifestPath).then(() => true, () => false); - if (!present) return null; - throw new Error(`Could not read ${kind} manifest ${manifestPath}: it is a symbolic link with no target.`); + const dangling = await danglingLinkOnPath(manifestPath); + if (!dangling) return null; + throw new Error(`Could not read ${kind} manifest ${manifestPath}: ${dangling} is a symbolic link with no target.`); } throw new Error(`Could not read ${kind} manifest ${manifestPath}: ${(error as Error).message}`); } From 0f478c35406e18afaeaf7347f12ee5aa88d267b4 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 17:46:24 +0200 Subject: [PATCH 14/25] chore(ci): re-run review against the rebased head No code change. The Codex review workflow re-reviews on push, and the PR body now carries the real-CLI matrix run on the rebased head. From 45fd1159aa2d2aebb8ca2cd4ea0f7189af98130e Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 18:02:19 +0200 Subject: [PATCH 15/25] fix(manifest): expand '~' when reading a manifest, guard role ids used as fallback namespaces Review findings on #710 after the rebase. readManifestFile replaced readFileSafe/readFileIfExists, which expanded a home-relative repo.localPath. Without the expansion a documented `~/.teamai/...` path is searched under the current directory, read as absent, and roles.yaml absence relaxes the filtering. The path is expanded before both the read and the dangling-link walk. When roles.yaml is absent, skills.ts and push.ts fall back to the role ids as namespaces. A role id is an unrestricted string, so a value such as '../../outside' reached path.join, and SkillsHandler.removeItem could recurse outside the team repo. Both fallbacks now pass the ids through the namespace guard and fail with the rule's message. --- src/__tests__/push-role.test.ts | 18 +++++++++++++++++ src/__tests__/roles.test.ts | 22 ++++++++++++++++++++ src/__tests__/skills.test.ts | 9 +++++++++ src/manifest-schema.ts | 36 +++++++++++++++++++++++++-------- src/push.ts | 3 ++- src/resources/skills.ts | 6 +++++- 6 files changed, 84 insertions(+), 10 deletions(-) diff --git a/src/__tests__/push-role.test.ts b/src/__tests__/push-role.test.ts index 87fe1562..b2b6277a 100644 --- a/src/__tests__/push-role.test.ts +++ b/src/__tests__/push-role.test.ts @@ -1,5 +1,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest'; import { push } from '../push.js'; +import { RolesManifestMissingError } from '../roles.js'; +import { log } from '../utils/logger.js'; const mockAutoDetectInit = vi.fn(); const mockPullRepo = vi.fn(); @@ -350,6 +352,22 @@ describe('push namespace routing', () => { } }); + it('rejects a role id that cannot be a namespace when roles.yaml is absent', async () => { + mockLoadRolesManifest.mockRejectedValue(new RolesManifestMissingError('/repo/manifest/roles.yaml')); + mockAutoDetectInit.mockResolvedValue({ + localConfig: makeLocalConfig({ primaryRole: '../../outside', additionalRoles: [] }), + teamConfig: makeTeamConfig(), + }); + mockSkillHandler(); + + await push({ all: true }); + + expect(vi.mocked(log.error)).toHaveBeenCalledWith( + expect.stringMatching(/Invalid role id used as a skills namespace "\.\.\/\.\.\/outside"/), + ); + expect(mockPushRepoBranch).not.toHaveBeenCalled(); + }); + it('rejects an unsafe scanned skill name before building the role path', async () => { mockAutoDetectInit.mockResolvedValue({ localConfig: makeLocalConfig(), diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index e6e9c6d5..8603a7ac 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -201,6 +201,28 @@ roles: }); }); +describe('loadRolesManifest with a home-relative repo path', () => { + it("expands '~' the way the helpers it replaced did, instead of reading under cwd", async () => { + const home = mkdtempSync(path.join(os.tmpdir(), 'teamai-home-')); + const previousHome = process.env.HOME; + process.env.HOME = home; + try { + const manifestDir = path.join(home, '.teamai', 'team-repo', 'manifest'); + mkdirSync(manifestDir, { recursive: true }); + writeFileSync(path.join(manifestDir, 'roles.yaml'), 'version: 1\nroles:\n - id: hai\n resources: { knowledge: [], skills: [common] }\n', 'utf-8'); + + const manifest = await loadRolesManifest('~/.teamai/team-repo'); + expect(manifest.roles[0]?.resources.skills).toEqual(['common']); + + // A missing file under `~` is still reported as absent, not as an error. + await expect(loadRolesManifest('~/.teamai/other-repo')).rejects.toBeInstanceOf(RolesManifestMissingError); + } finally { + if (previousHome === undefined) delete process.env.HOME; else process.env.HOME = previousHome; + rmSync(home, { recursive: true, force: true }); + } + }); +}); + describe('loadRolesManifestIfPresent', () => { it('returns null when the manifest is absent', async () => { const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-noroles-')); diff --git a/src/__tests__/skills.test.ts b/src/__tests__/skills.test.ts index b46fe087..9067d932 100644 --- a/src/__tests__/skills.test.ts +++ b/src/__tests__/skills.test.ts @@ -334,6 +334,15 @@ scope: 'user', expect(items.find((item) => item.name === 'role-skill')?.status).toBe('modified'); }); + it('refuses a role id that cannot be a namespace when roles.yaml is absent, instead of joining it onto the repo', async () => { + localConfig.primaryRole = '../../outside'; + localConfig.additionalRoles = []; + + await expect(handler.scanLocalForPush(teamConfig, localConfig)).rejects.toThrow( + /Invalid role id used as a skills namespace "\.\.\/\.\.\/outside"/, + ); + }); + it('blocks skills that exist in non-allowed namespaces', async () => { localConfig.primaryRole = 'hai'; localConfig.additionalRoles = []; diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index 46525a90..490af5d0 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -1,5 +1,6 @@ import fs from 'node:fs/promises'; import path from 'node:path'; +import { expandHome } from './utils/fs.js'; import { z } from 'zod'; /** @@ -48,10 +49,25 @@ export function isSafeNamespaceSegment(seg: string): boolean { && !WINDOWS_DEVICE_NAME.test(seg); } +const NAMESPACE_RULE = "resource namespace must be a single path segment (no '/', '\\', ':' or control characters, no trailing '.' or space, which also rules out '.' and '..', and not a Windows device name such as 'CON' or 'COM1')"; + /** A resource namespace: one path segment that cannot escape its parent. */ -export const NamespaceSegmentSchema = z.string().min(1).refine(isSafeNamespaceSegment, { - message: "resource namespace must be a single path segment (no '/', '\\', ':' or control characters, no trailing '.' or space, which also rules out '.' and '..', and not a Windows device name such as 'CON' or 'COM1')", -}); +export const NamespaceSegmentSchema = z.string().min(1).refine(isSafeNamespaceSegment, { message: NAMESPACE_RULE }); + +/** + * A role id that stands in for a namespace when `roles.yaml` is absent. The + * manifest never validated it, so it gets the same check here before it can + * become a path component; an unsafe one fails the command rather than being + * joined onto the team repo. + */ +export function assertSafeFallbackNamespaces(ids: string[], source: string): string[] { + const unsafe = ids.find((id) => !isSafeNamespaceSegment(id)); + if (unsafe !== undefined) { + throw new Error(`Invalid ${source} "${unsafe}": ${NAMESPACE_RULE}`); + } + return ids; +} + /** * Parse a manifest, reporting a failure the way the hand-written checks around @@ -101,19 +117,23 @@ async function danglingLinkOnPath(target: string): Promise { * direction. Absence returns `null` here; anything else throws. */ export async function readManifestFile(manifestPath: string, kind: 'projects' | 'roles'): Promise { + // `repo.localPath` is documented as `~/.teamai/...`; the helpers this replaced + // expanded it, and a path left unexpanded would be searched under the current + // directory, read as absent, and relax the filtering. + const resolvedPath = expandHome(manifestPath); let content: string; try { - content = await fs.readFile(manifestPath, 'utf-8'); + content = await fs.readFile(resolvedPath, 'utf-8'); } catch (error) { if ((error as NodeJS.ErrnoException).code === 'ENOENT') { - const dangling = await danglingLinkOnPath(manifestPath); + const dangling = await danglingLinkOnPath(resolvedPath); if (!dangling) return null; - throw new Error(`Could not read ${kind} manifest ${manifestPath}: ${dangling} is a symbolic link with no target.`); + throw new Error(`Could not read ${kind} manifest ${resolvedPath}: ${dangling} is a symbolic link with no target.`); } - throw new Error(`Could not read ${kind} manifest ${manifestPath}: ${(error as Error).message}`); + throw new Error(`Could not read ${kind} manifest ${resolvedPath}: ${(error as Error).message}`); } if (content.trim() === '') { - throw new Error(`Invalid ${kind} manifest: ${manifestPath} is empty. Delete it, or give it a version and a ${kind} list.`); + throw new Error(`Invalid ${kind} manifest: ${resolvedPath} is empty. Delete it, or give it a version and a ${kind} list.`); } return content; } diff --git a/src/push.ts b/src/push.ts index 57791ab8..f804e464 100644 --- a/src/push.ts +++ b/src/push.ts @@ -22,6 +22,7 @@ import { getDataHome, SYNC_LOCK_FILENAME } from './types.js'; import { acquireLock, releaseLock } from './update.js'; import { assertSafePath, assertSafeResourceName, defaultAllowedRoots } from './utils/path-safety.js'; import { loadRolesManifest, resolveRoleResourceNamespaces, RolesManifestMissingError } from './roles.js'; +import { assertSafeFallbackNamespaces } from './manifest-schema.js'; import { askQuestion, askSelection } from './utils/prompt.js'; import { pathExists, pruneEmptyDirs, readFileSafe, writeFile } from './utils/fs.js'; @@ -81,7 +82,7 @@ async function resolveSkillNamespaces( // Only an absent manifest falls back to the role id as the namespace; a // broken one would send the push into a namespace nothing validated. if (!(error instanceof RolesManifestMissingError)) throw error; - return [primaryRole]; + return assertSafeFallbackNamespaces([primaryRole], 'role id used as a skills namespace'); } } diff --git a/src/resources/skills.ts b/src/resources/skills.ts index c4a8ca41..4a0edb76 100644 --- a/src/resources/skills.ts +++ b/src/resources/skills.ts @@ -9,6 +9,7 @@ import { BUILTIN_SKILL_NAMES } from '../builtin-skills.js'; import { resolveOpenclawWorkspaceDir } from '../openclaw-hooks.js'; import { getHermesHome } from '../hermes-home.js'; import { loadRolesManifest, resolveRoleResourceNamespaces, RolesManifestMissingError } from '../roles.js'; +import { assertSafeFallbackNamespaces } from '../manifest-schema.js'; import { assertWithinRoot } from '../utils/path-safety.js'; import { splitFrontmatter, stringifyFrontmatter } from '../utils/frontmatter.js'; @@ -276,7 +277,10 @@ async function resolveSkillNamespaces(localConfig: LocalConfig): Promise Date: Tue, 22 Sep 2026 18:10:15 +0200 Subject: [PATCH 16/25] fix(config): expand '~' in repo.localPath at the config boundary A home-relative repo.localPath reached simple-git, the manifest readers and every resource path unexpanded, so `teamai pull` failed with 'Cannot use simple-git on a directory that does not exist'. The schema now expands it once, at parse time, so no consumer has to. expandHome moves to utils/home.ts (fs.ts re-exports it) so types.ts can import it without pulling in the fs helpers. --- src/__tests__/hooks-shell-check.test.ts | 3 ++- src/__tests__/types.test.ts | 24 ++++++++++++++++++++++++ src/types.ts | 6 ++++-- src/utils/fs.ts | 12 ++---------- src/utils/home.ts | 11 +++++++++++ 5 files changed, 43 insertions(+), 13 deletions(-) diff --git a/src/__tests__/hooks-shell-check.test.ts b/src/__tests__/hooks-shell-check.test.ts index e1bb1840..f669c120 100644 --- a/src/__tests__/hooks-shell-check.test.ts +++ b/src/__tests__/hooks-shell-check.test.ts @@ -32,7 +32,8 @@ import { log } from '../utils/logger.js'; // Isolate getUserHome() so ensureTeamaiWrapper / bundled-shell detection read // a per-test home directory instead of the real one. const homeState = vi.hoisted(() => ({ home: '' })); -vi.mock('../utils/home.js', () => ({ +vi.mock('../utils/home.js', async (importOriginal) => ({ + ...(await importOriginal()), getUserHome: () => homeState.home, })); diff --git a/src/__tests__/types.test.ts b/src/__tests__/types.test.ts index 9f73f8f0..093f9aa2 100644 --- a/src/__tests__/types.test.ts +++ b/src/__tests__/types.test.ts @@ -56,6 +56,30 @@ describe('MemberConfigSchema', () => { }); }); +describe('LocalConfigSchema', () => { + it("expands a home-relative repo.localPath so git and the manifest readers see an absolute path", () => { + const previousHome = process.env.HOME; + process.env.HOME = '/home/e2e'; + try { + const parsed = LocalConfigSchema.parse({ + repo: { localPath: '~/.teamai/team-repo', remote: 'https://github.com/acme/team.git' }, + username: 'e2e', + }); + expect(parsed.repo.localPath).toBe('/home/e2e/.teamai/team-repo'); + } finally { + if (previousHome === undefined) delete process.env.HOME; else process.env.HOME = previousHome; + } + }); + + it('leaves an absolute repo.localPath untouched', () => { + const parsed = LocalConfigSchema.parse({ + repo: { localPath: '/srv/team-repo', remote: 'https://github.com/acme/team.git' }, + username: 'e2e', + }); + expect(parsed.repo.localPath).toBe('/srv/team-repo'); + }); +}); + describe('TeamaiConfigSchema', () => { it.each(['github', 'tgit', 'cnb', 'git'] as const)( 'accepts the %s provider', diff --git a/src/types.ts b/src/types.ts index 7e73adfa..72275b4c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1,7 +1,7 @@ import { z } from 'zod'; import path from 'node:path'; import { createHash } from 'node:crypto'; -import { getUserHome } from './utils/home.js'; +import { getUserHome, expandHome } from './utils/home.js'; const DEFAULT_COPILOT_HOME = '.copilot'; const COPILOT_USER_MCP_CONFIG = 'mcp-config.json'; @@ -491,7 +491,9 @@ export type MemberConfig = z.infer; export const LocalConfigSchema = z.object({ repo: z.object({ - localPath: z.string(), + // Expanded at the boundary: the path feeds simple-git, the manifest readers + // and every resource path, none of which understand `~`. + localPath: z.string().transform(expandHome), remote: z.string(), /** * Team repo backend. Defaults to 'git' for backward compatibility. diff --git a/src/utils/fs.ts b/src/utils/fs.ts index 1f6ee565..77877dc4 100644 --- a/src/utils/fs.ts +++ b/src/utils/fs.ts @@ -2,7 +2,7 @@ import fse from 'fs-extra'; import crypto from 'node:crypto'; import path from 'node:path'; import { log } from './logger.js'; -import { getUserHome } from './home.js'; +import { expandHome } from './home.js'; const IGNORED_NAMES = new Set([ '__pycache__', @@ -16,15 +16,7 @@ function isIgnored(name: string): boolean { return IGNORED_NAMES.has(name) || name.endsWith('.pyc'); } -/** - * Expand ~ to the platform user home directory in paths. - */ -export function expandHome(p: string): string { - if (p.startsWith('~/') || p === '~') { - return path.join(getUserHome(), p.slice(1)); - } - return p; -} +export { expandHome } from './home.js'; /** * Ensure a directory exists diff --git a/src/utils/home.ts b/src/utils/home.ts index cf4a0340..fc5aed6a 100644 --- a/src/utils/home.ts +++ b/src/utils/home.ts @@ -1,4 +1,5 @@ import os from 'node:os'; +import path from 'node:path'; /** * Resolve the current user's home directory across supported platforms. @@ -27,3 +28,13 @@ export function getUserHome(): string { } return home; } + +/** + * Expand ~ to the platform user home directory in paths. + */ +export function expandHome(p: string): string { + if (p.startsWith('~/') || p === '~') { + return path.join(getUserHome(), p.slice(1)); + } + return p; +} From 9bee58c36561e11bbe79a0dccc7b36954aa81bbf Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 18:15:29 +0200 Subject: [PATCH 17/25] fix(manifest): reject the CONIN$ and CONOUT$ console devices too Review finding on #710. Windows opens the console for these names in any directory, extension or not, the way it does for CON, so a namespace spelled that way cannot be the directory the manifest means. --- docs/usage-guide.md | 4 ++-- docs/usage-guide.zh-CN.md | 2 +- src/__tests__/projects.test.ts | 2 ++ src/__tests__/roles.test.ts | 2 +- src/manifest-schema.ts | 7 ++++--- 5 files changed, 10 insertions(+), 7 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 5c069262..c87bc067 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -249,8 +249,8 @@ neither may escape the directory it names. A **namespace** must be a single path segment: no `/`, `\`, `:` or control character, no trailing `.` or space, and not a Windows device name (`CON`, `NUL`, -`AUX`, `PRN`, `COM1`–`COM9`, `LPT1`–`LPT9`, including the superscript forms -Windows also reads as device numbers, with or without an extension). +`AUX`, `PRN`, `CONIN$`, `CONOUT$`, `COM1`–`COM9`, `LPT1`–`LPT9`, including the +superscript forms Windows also reads as device numbers, with or without an extension). Windows strips a trailing period or space from every path component, so `.. ` would arrive as `..` and escape the parent while `frontend.` would arrive as `frontend` and land in another namespace's directory; the same rule rules out `.` diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 39b16710..1d9fc9a2 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -233,7 +233,7 @@ projects: 都不能越出自己命名的目录。 **namespace** 必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，结尾不能是 `.` -或空格，也不能是 Windows 设备名（`CON`、`NUL`、`AUX`、`PRN`、`COM1`–`COM9`、 +或空格，也不能是 Windows 设备名（`CON`、`NUL`、`AUX`、`PRN`、`CONIN$`、`CONOUT$`、`COM1`–`COM9`、 `LPT1`–`LPT9`，含 Windows 同样识别为设备编号的上标形式，带不带扩展名都算）。Windows 会从每个路径片段删除结尾的句点与空格， 因此 `.. ` 最终变成 `..` 越出上级目录，`frontend.` 最终变成 `frontend` 落进另一个 namespace 的目录；该规则同时排除了 `.` 与 `..`。除此之外不受限制 —— 非 ASCII 名称、 diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index 7cd84f29..3a9d7b8d 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -161,6 +161,8 @@ projects: // Windows opens a device for these in any directory, with or without an // extension, so they cannot name the directory the manifest means. 'CON', 'con', 'NUL', 'aux', 'COM1', 'lpt9', 'CON.txt', + // The console handles are devices too, extension or not. + 'CONIN$', 'conout$', 'CONOUT$.txt', // Windows reads the superscript forms as device numbers too. 'COM\u00b9', 'LPT\u00b3', ]) { diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index 8603a7ac..49559a25 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -132,7 +132,7 @@ roles: 'a\u0009b', 'a\u007fb', 'a\u0085b', '.. ', '.. .', '...', '. ', ' ', 'frontend.', 'frontend ', 'frontend..', - 'CON', 'nul', 'COM1', 'CON.txt', + 'CON', 'nul', 'COM1', 'CON.txt', 'CONIN$', 'CONOUT$.txt', ]) { const repoDir = writeManifest(` version: 1 diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index 490af5d0..34972d2c 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -31,15 +31,16 @@ const TRAILING_DOT_OR_SPACE = /[ .]$/; // Windows reserves these names for devices in every directory, extension or not: // `CON`, `NUL`, `COM1`, `CON.txt` all open a device rather than a file, so a // namespace spelled that way cannot be the directory the manifest means. The set -// is `CON`, `PRN`, `AUX`, `NUL`, `COM1`-`COM9` and `LPT1`-`LPT9`; `COM0` and -// `LPT0` are ordinary names and keep parsing. Windows also reads the superscript +// is `CON`, `PRN`, `AUX`, `NUL`, `COM1`-`COM9` and `LPT1`-`LPT9`, plus the console +// handles `CONIN$` and `CONOUT$`; `COM0` and `LPT0` are ordinary names and keep +// parsing. Windows also reads the superscript // forms of 1, 2 and 3 (U+00B9, U+00B2, U+00B3) as device numbers, so those go in // with the ASCII digits. // // The project id is deliberately left out of this, the way it is left out of the // rules above: it is a working POSIX directory name that the id rule has always // accepted, and narrowing it would break manifests that parse today. -const WINDOWS_DEVICE_NAME = /^(con|prn|aux|nul|(com|lpt)[1-9\u00b9\u00b2\u00b3])(\.|$)/i; +const WINDOWS_DEVICE_NAME = /^(con|conin\$|conout\$|prn|aux|nul|(com|lpt)[1-9\u00b9\u00b2\u00b3])(\.|$)/i; /** True if `seg` is safe to use as a single path segment (no separators, no `..`). */ export function isSafeNamespaceSegment(seg: string): boolean { From ff05b3ec933df16a1330e41350ba71230f461c2d Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 18:30:20 +0200 Subject: [PATCH 18/25] fix(push): guard the role id silent mode uses as a namespace Review finding on #710. With a valid manifest that maps the role to several skill namespaces, silent push assigned primaryRole as the namespace without the check the fallback path already has. It now goes through the same guard, so 'frontend.' or 'CON' fail the push instead of becoming a path. --- src/__tests__/push-role.test.ts | 21 +++++++++++++++++++++ src/push.ts | 7 ++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/__tests__/push-role.test.ts b/src/__tests__/push-role.test.ts index b2b6277a..0412aca2 100644 --- a/src/__tests__/push-role.test.ts +++ b/src/__tests__/push-role.test.ts @@ -287,6 +287,27 @@ describe('push namespace routing', () => { expect(pushedItems[0].relativePath).toBe('skills/hai/skill-a'); }); + it('refuses a role id that cannot be a namespace in silent mode, even with a valid manifest', async () => { + mockLoadRolesManifest.mockResolvedValue({ + version: 1, + roles: [ + { id: 'CON', description: 'device-named role', resources: { knowledge: ['common'], skills: ['common', 'hai'], agents: [] } }, + ], + }); + mockAutoDetectInit.mockResolvedValue({ + localConfig: makeLocalConfig({ primaryRole: 'CON', additionalRoles: [] }), + teamConfig: makeTeamConfig(), + }); + mockSkillHandler(); + + await push({ all: true, silent: true }); + + expect(vi.mocked(log.error)).toHaveBeenCalledWith( + expect.stringMatching(/Invalid role id used as a skills namespace "CON"/), + ); + expect(mockPushRepoBranch).not.toHaveBeenCalled(); + }); + it('explicit --role flag bypasses namespace resolution', async () => { const pushedItems: Array> = []; mockAutoDetectInit.mockResolvedValue({ diff --git a/src/push.ts b/src/push.ts index f804e464..7ae56b92 100644 --- a/src/push.ts +++ b/src/push.ts @@ -871,7 +871,12 @@ async function pushCore( } else if (skillNamespaces.length === 1) { resolvedNamespaceForNew = skillNamespaces[0]; } else if (options.silent) { - resolvedNamespaceForNew = localConfig.primaryRole; + // The role id stands in for a namespace here too, and the manifest + // never validated it as one: guard it before it becomes a path. + [resolvedNamespaceForNew] = assertSafeFallbackNamespaces( + [localConfig.primaryRole], + 'role id used as a skills namespace', + ); } else { console.log(''); console.log('Which namespace should new skills be pushed to?'); From 27112c89f1a3a23bae876abbd7d7f9c484e3537b Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 18:51:19 +0200 Subject: [PATCH 19/25] fix(manifest): reject namespaces that differ only by case, classify the guard as breaking Review findings on #710. Two namespaces of one resource type that differ only by case (or Unicode normalization) name a single directory on the default Windows and macOS filesystems, so a role scoped to 'frontend' would read 'Frontend' too. Each manifest is checked when it loads; the pull path checks roles.yaml against projects.yaml as well, since both share skills/, knowledge/ and agents/. The namespace guard makes a manifest that parsed before fail every pull, so the changelog entry moves under Breaking Changes. --- CHANGELOG.md | 5 +- docs/designs/multi-project-management.md | 3 ++ docs/usage-guide.md | 10 +++- docs/usage-guide.zh-CN.md | 7 ++- src/__tests__/projects.test.ts | 22 +++++++++ .../resource-namespaces-case-alias.test.ts | 48 +++++++++++++++++++ src/__tests__/roles.test.ts | 44 +++++++++++++++++ src/manifest-schema.ts | 30 ++++++++++++ src/projects.ts | 12 ++++- src/resource-namespaces.ts | 13 ++++- src/roles.ts | 12 ++++- 11 files changed, 199 insertions(+), 7 deletions(-) create mode 100644 src/__tests__/resource-namespaces-case-alias.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 90e3a0c2..325c37df 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,10 @@ All notable changes to this project will be documented in this file. See [standa ## [Unreleased] +### 💥 Breaking Changes + +- `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil`, `a/b`, `C:evil`, a bare `..`, any name with a trailing `.` or space — which Win32 strips, making `.. ` arrive as `..` and `frontend.` as `frontend` — and a Windows device name such as `CON` or `COM1` under `resources:` no longer parse; the error names the offending entry. Only traversal is rejected: a namespace that is a plain directory name still parses, non-ASCII names and names with a space included. A manifest that fails to parse now reports the offending entry on one line (`Invalid projects manifest: projects.0.resources.skills.1: ...`) instead of dumping a raw validation object. Two namespaces of one resource type that differ only by case (`frontend`, `Frontend`) are rejected too, within a manifest and between the two, since they name one directory on Windows and macOS. A manifest that ships any of these — a device name, a trailing `.`, a case-only pair — parsed before and fails every pull now; rename the directory and the entry together. + ### ✨ 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`, 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)). @@ -22,7 +26,6 @@ All notable changes to this project will be documented in this file. See [standa ### 🐛 Bug Fixes - `teamai init` no longer hangs without a terminal. When the provider had no session it spawned `gh auth login --web` (or `gf auth login`, `cnb login`) with inherited stdio and waited for a browser device flow that nobody could complete, about five minutes for GitHub, then exited with the provider's error and no hint of the missing credential. Each login now refuses up front when the run is not interactive and names the credential to prepare (`GITHUB_TOKEN` / `GH_TOKEN`, `CNB_TOKEN`, or for TGit a prior `gf auth login`, since a `TGIT_TOKEN` PAT is REST-API-only and cannot clone). A run is non-interactive when stdin is not a TTY or when `CI` or `TEAMAI_NONINTERACTIVE` is set, so an agent sandbox with a pseudo-terminal can declare itself unattended, and every prompt in the CLI follows the same rule. `git` also runs with its prompts closed in that case — `GIT_TERMINAL_PROMPT=0`, `GIT_ASKPASS=echo` and `GCM_INTERACTIVE=never`, each only where the caller set nothing — so a missing clone credential fails at once instead of waiting on a terminal prompt or an askpass or credential-manager dialog. `ssh` keeps its own settings: its batch flag is only reachable through `GIT_SSH_COMMAND`, which would override each repository's `core.sshCommand` (for [#711](https://github.com/Tencent/teamai-cli/issues/711)). -- `manifest/projects.yaml` and `manifest/roles.yaml` now reject a resource namespace that is not a single path segment, the guard a project id already had. A namespace becomes a directory component (`skills//`, `agents//`, `learnings//`), so `../evil`, `a/b`, `C:evil`, a bare `..`, any name with a trailing `.` or space — which Win32 strips, making `.. ` arrive as `..` and `frontend.` as `frontend` — and a Windows device name such as `CON` or `COM1` under `resources:` no longer parse; the error names the offending entry. Only traversal is rejected: a namespace that is a plain directory name still parses, non-ASCII names and names with a space included. A manifest that fails to parse now reports the offending entry on one line (`Invalid projects manifest: projects.0.resources.skills.1: ...`) instead of dumping a raw validation object. - A `manifest/roles.yaml` that exists but does not parse now fails the pull for that scope instead of warning and syncing with no role filter at all. The same applies to `init`, `push` and the legacy role migration, which each fell back to a guess at the namespaces when any error came out of the loader. For a member with no active project that fallback meant an unfiltered sync, so a broken manifest delivered every namespace it was written to gate. Only an absent manifest still means "this team does not use roles"; an unreadable or empty file is an error, as it now is for `manifest/projects.yaml` too. - Cache GC now rejects partial integers such as `12abc`, decimals, zero and unsafe integers for `--max-bytes` and `--stale-days` before deleting anything. An invalid `TEAMAI_CACHE_MAX_BYTES` value falls back to the default 5 GB limit instead of using a numeric prefix. - Usage reporting scopes sessions by path on Windows too. The project/user scope filter compared an event's `cwd` against `projectRoot` with a hard-coded `/` separator, so on Windows only a session started in the project root itself matched: every session started in a subdirectory was dropped from the project team's report and counted in the user scope's instead, which is the isolation the usage guide promises. Windows paths are also compared case-insensitively, so a drive letter or a directory name spelled with different case in the two sources no longer leaks a project session into the user scope. POSIX paths keep their own rules: case-sensitive, and a `\` in a filename stays part of the name. diff --git a/docs/designs/multi-project-management.md b/docs/designs/multi-project-management.md index e1a9627b..fa35f558 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -85,6 +85,9 @@ The id and every namespace are refused at the manifest boundary unless they can name a directory without escaping it, since each becomes a directory component. A namespace must be a single path segment: no `/`, `\`, `:` or control character, no trailing `.` or space, and not a Windows device name (`CON`, `NUL`, `COM1`, …). +Two namespaces of one resource type may not differ only by case, within a manifest +or between `roles.yaml` and `projects.yaml`, since case-insensitive filesystems +would give both the same directory. Win32 strips a trailing period or space from every component, so `.. ` would arrive as `..` and `frontend.` as `frontend`, escaping the parent in the first case and another namespace's directory in the second; `.` and `..` fall out of the diff --git a/docs/usage-guide.md b/docs/usage-guide.md index c87bc067..68b95607 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -256,6 +256,11 @@ would arrive as `..` and escape the parent while `frontend.` would arrive as `frontend` and land in another namespace's directory; the same rule rules out `.` and `..`. Anything else a filesystem accepts stays valid — a non-ASCII name, one holding a space inside it, or one that merely starts like a device (`console`). +Two namespaces of the same resource type may not differ only by case (`frontend` +and `Frontend`): on the default Windows and macOS filesystems they are one +directory, so a role or project scoped to one would read the other's resources. +The check spans both manifests, since `roles.yaml` and `projects.yaml` share the +same `skills/`, `knowledge/` and `agents/` directories. A **project id** keeps its own older and narrower rule, because it is also typed on the command line and split on commas: letters, digits, `.`, `_` and `-`, and @@ -1502,8 +1507,9 @@ roles: Every namespace that takes effect — `knowledge`, `skills` and `agents` — becomes a directory name, so it must be a single path segment: no `/`, `\`, `:` or control -character, no trailing `.` or space, and not a Windows device name, in -`manifest/roles.yaml` exactly as in `manifest/projects.yaml`. A role's +character, no trailing `.` or space, and not a Windows device name, and no two +namespaces of one resource type may differ only by case — in `manifest/roles.yaml` +exactly as in `manifest/projects.yaml`, and across the two. A role's `learnings:` is accepted for backward compatibility and ignored at runtime (learnings are namespaced by project, not by role), so it names no directory and is not checked. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index 1d9fc9a2..e241984c 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -238,6 +238,10 @@ projects: 因此 `.. ` 最终变成 `..` 越出上级目录，`frontend.` 最终变成 `frontend` 落进另一个 namespace 的目录；该规则同时排除了 `.` 与 `..`。除此之外不受限制 —— 非 ASCII 名称、 名称中间含空格的目录、以及只是以设备名开头的名称（如 `console`）仍然合法。 +同一资源类型下的两个 namespace 不能仅有大小写差异（如 `frontend` 与 `Frontend`）：在 +Windows 与 macOS 的默认文件系统上它们是同一个目录，限定到其中一个的 role 或 project +会读到另一个的资源。该校验跨越两个 manifest，因为 `roles.yaml` 与 `projects.yaml` 共用 +同一套 `skills/`、`knowledge/`、`agents/` 目录。 **项目 id** 沿用它原有的、更严格的规则，因为它还会在命令行中输入并按逗号切分： 只允许字母、数字、`.`、`_` 和 `-`，且不能是 `.` 或 `..`。 @@ -1457,7 +1461,8 @@ roles: 真正生效的 namespace（`knowledge`、`skills`、`agents`）都会成为目录名，因此必须是 单个路径片段：不含 `/`、`\`、`:` 和控制字符，结尾不能是 `.` 或空格，也不能是 -Windows 设备名，`manifest/roles.yaml` 与 `manifest/projects.yaml` 规则一致。role 的 +Windows 设备名，且同一资源类型下的两个 namespace 不能仅有大小写差异；`manifest/roles.yaml` +与 `manifest/projects.yaml` 规则一致，且两者之间也做该校验。role 的 `learnings:` 仅为向后兼容而保留、运行时忽略（learnings 按 project 而非 role 划分 namespace），不会成为目录名，因此不做校验。 diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index 3a9d7b8d..2acf6120 100644 --- a/src/__tests__/projects.test.ts +++ b/src/__tests__/projects.test.ts @@ -22,6 +22,28 @@ function writeManifest(content: string): string { return repoDir; } +describe('loadProjectsManifest rejects namespaces that alias each other by case', () => { + it('across projects, for the same resource type', async () => { + const repoDir = writeManifest(` +version: 1 +projects: + - id: a + name: A + resources: { skills: [hai-inference] } + - id: b + name: B + resources: { skills: [HAI-Inference] } +`); + try { + await expect(loadProjectsManifest(repoDir)).rejects.toThrow( + /Invalid projects manifest: skills namespaces "hai-inference" \(project a\) and "HAI-Inference" \(project b\) differ only by case/, + ); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); +}); + describe('loadProjectsManifest', () => { it('returns null when the manifest is absent (projects are optional)', async () => { const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-noproj-')); diff --git a/src/__tests__/resource-namespaces-case-alias.test.ts b/src/__tests__/resource-namespaces-case-alias.test.ts new file mode 100644 index 00000000..eae79c99 --- /dev/null +++ b/src/__tests__/resource-namespaces-case-alias.test.ts @@ -0,0 +1,48 @@ +import { describe, expect, it } from 'vitest'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { resolveResourceNamespaces } from '../resource-namespaces.js'; +import type { LocalConfig } from '../types.js'; + +function repoWith(roles: string, projects: string): string { + const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-ns-case-')); + mkdirSync(path.join(repoDir, 'manifest'), { recursive: true }); + writeFileSync(path.join(repoDir, 'manifest', 'roles.yaml'), roles, 'utf-8'); + writeFileSync(path.join(repoDir, 'manifest', 'projects.yaml'), projects, 'utf-8'); + return repoDir; +} + +function localConfig(repoDir: string): LocalConfig { + return { + repo: { localPath: repoDir, remote: 'https://github.com/acme/team.git' }, + username: 'e2e', + primaryRole: 'fe', + projects: ['p'], + } as LocalConfig; +} + +describe('resolveResourceNamespaces: roles.yaml and projects.yaml share one directory per resource type', () => { + const ROLES = 'version: 1\nroles:\n - id: fe\n resources: { knowledge: [], skills: [frontend] }\n'; + + it('rejects a project namespace that aliases a role namespace by case', async () => { + const repoDir = repoWith(ROLES, 'version: 1\nprojects:\n - id: p\n name: P\n resources: { skills: [Frontend] }\n'); + try { + await expect(resolveResourceNamespaces(localConfig(repoDir))).rejects.toThrow( + /skills namespaces "frontend" \(role fe\) and "Frontend" \(project p\) differ only by case/, + ); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + + it('accepts the same spelling shared by a role and a project', async () => { + const repoDir = repoWith(ROLES, 'version: 1\nprojects:\n - id: p\n name: P\n resources: { skills: [frontend, p-only] }\n'); + try { + const resolved = await resolveResourceNamespaces(localConfig(repoDir)); + expect(resolved?.activeNamespaces.skills).toEqual(expect.arrayContaining(['frontend', 'p-only'])); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); +}); diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index 49559a25..ecf9f6e1 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -201,6 +201,50 @@ roles: }); }); +describe('loadRolesManifest rejects namespaces that alias each other by case', () => { + function writeManifest(content: string): string { + const repoDir = mkdtempSync(path.join(os.tmpdir(), 'teamai-roles-case-')); + mkdirSync(path.join(repoDir, 'manifest'), { recursive: true }); + writeFileSync(path.join(repoDir, 'manifest', 'roles.yaml'), content, 'utf-8'); + return repoDir; + } + + it('across roles, for the same resource type', async () => { + const repoDir = writeManifest(` +version: 1 +roles: + - id: fe + resources: { knowledge: [], skills: [frontend] } + - id: fe2 + resources: { knowledge: [], skills: [Frontend] } +`); + try { + await expect(loadRolesManifest(repoDir)).rejects.toThrow( + /Invalid roles manifest: skills namespaces "frontend" \(role fe\) and "Frontend" \(role fe2\) differ only by case/, + ); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + + it('but not the same spelling used twice, nor the same name under two resource types', async () => { + const repoDir = writeManifest(` +version: 1 +roles: + - id: fe + resources: { knowledge: [frontend], skills: [frontend], agents: [Frontend] } + - id: fe2 + resources: { knowledge: [frontend], skills: [frontend] } +`); + try { + const manifest = await loadRolesManifest(repoDir); + expect(manifest.roles).toHaveLength(2); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); +}); + describe('loadRolesManifest with a home-relative repo path', () => { it("expands '~' the way the helpers it replaced did, instead of reading under cwd", async () => { const home = mkdtempSync(path.join(os.tmpdir(), 'teamai-home-')); diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index 34972d2c..4bf36028 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -138,3 +138,33 @@ export async function readManifestFile(manifestPath: string, kind: 'projects' | } return content; } + +/** One namespace as a manifest declares it, with the entry that declares it. */ +export interface NamespaceEntry { + type: string; + namespace: string; + owner: string; +} + +/** + * Two namespaces of the same resource type that differ only by case (or by + * Unicode normalization) name one directory on the default Windows and macOS + * filesystems, so a role or project scoped to `frontend` would read `Frontend`'s + * resources too — the isolation the namespace exists to provide. `kind` names + * what was being checked, e.g. `roles manifest`. + */ +export function assertNoCaseAliasedNamespaces(entries: Iterable, kind: string): void { + const seen = new Map(); + for (const entry of entries) { + const key = `${entry.type}/${entry.namespace.normalize('NFC').toLowerCase()}`; + const prior = seen.get(key); + if (!prior) { + seen.set(key, entry); + } else if (prior.namespace !== entry.namespace) { + throw new Error( + `Invalid ${kind}: ${entry.type} namespaces "${prior.namespace}" (${prior.owner}) and "${entry.namespace}" (${entry.owner}) ` + + 'differ only by case or Unicode normalization and would name the same directory on a case-insensitive filesystem', + ); + } + } +} diff --git a/src/projects.ts b/src/projects.ts index 682124e9..134cd433 100644 --- a/src/projects.ts +++ b/src/projects.ts @@ -2,7 +2,7 @@ import path from 'node:path'; import YAML from 'yaml'; import { z } from 'zod'; import { ensureDir, writeFile } from './utils/fs.js'; -import { NamespaceSegmentSchema, parseManifest, readManifestFile } from './manifest-schema.js'; +import { NamespaceSegmentSchema, parseManifest, readManifestFile, assertNoCaseAliasedNamespaces, type NamespaceEntry } from './manifest-schema.js'; import type { ResourceNamespaces } from './roles.js'; /** @@ -100,10 +100,20 @@ function validateManifestShape(raw: unknown): ProjectsManifest { } ids.add(project.id); } + assertNoCaseAliasedNamespaces(projectNamespaceEntries(manifest), 'projects manifest'); return manifest; } +/** Every namespace a projects manifest puts to use, with the project that declares it. */ +export function projectNamespaceEntries(manifest: ProjectsManifest): NamespaceEntry[] { + return manifest.projects.flatMap((project) => + PROJECT_RESOURCE_TYPES.flatMap((type) => + project.resources[type].map((namespace) => ({ type, namespace, owner: `project ${project.id}` })), + ), + ); +} + /** * Load the projects manifest. Returns `null` when the file is absent — projects * are optional (a team without partitioning has no projects.yaml), so every diff --git a/src/resource-namespaces.ts b/src/resource-namespaces.ts index 3118767a..b7685bf8 100644 --- a/src/resource-namespaces.ts +++ b/src/resource-namespaces.ts @@ -2,10 +2,12 @@ import type { LocalConfig } from './types.js'; import { loadRolesManifest, resolveRoleResourceNamespaces, + roleNamespaceEntries, RolesManifestMissingError, type ResourceNamespaces, } from './roles.js'; -import { loadProjectsManifest, resolveProjectResourceNamespaces, mergeNamespaces } from './projects.js'; +import { loadProjectsManifest, resolveProjectResourceNamespaces, mergeNamespaces, projectNamespaceEntries } from './projects.js'; +import { assertNoCaseAliasedNamespaces } from './manifest-schema.js'; import { log } from './utils/logger.js'; /** Resolve the same role/project activation policy for resource pull and push. */ @@ -56,6 +58,15 @@ export async function resolveResourceNamespaces(localConfig: LocalConfig) { log.warn('Roles manifest not found. Skipping role-based filtering.'); rolesManifest = null; } + if (rolesManifest && projectsManifest) { + // Each manifest is checked on its own when it loads; the two together share + // the same skills/, knowledge/ and agents/ directories, so a role's + // `frontend` and a project's `Frontend` collide just as two roles' would. + assertNoCaseAliasedNamespaces( + [...roleNamespaceEntries(rolesManifest), ...projectNamespaceEntries(projectsManifest)], + 'manifests (roles.yaml with projects.yaml)', + ); + } if (rolesManifest) { try { roleNamespaces = resolveRoleResourceNamespaces({ diff --git a/src/roles.ts b/src/roles.ts index cafa6e32..6813e811 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -3,7 +3,7 @@ import YAML from 'yaml'; import { z } from 'zod'; import { ensureDir, writeFile } from './utils/fs.js'; import { log } from './utils/logger.js'; -import { NamespaceSegmentSchema, parseManifest, readManifestFile } from './manifest-schema.js'; +import { NamespaceSegmentSchema, parseManifest, readManifestFile, assertNoCaseAliasedNamespaces, type NamespaceEntry } from './manifest-schema.js'; const ROLE_RESOURCE_TYPES = ['knowledge', 'skills', 'agents'] as const; @@ -91,10 +91,20 @@ function validateManifestShape(raw: unknown): RolesManifest { } ids.add(role.id); } + assertNoCaseAliasedNamespaces(roleNamespaceEntries(manifest), 'roles manifest'); return manifest; } +/** Every namespace a roles manifest puts to use, with the role that declares it. */ +export function roleNamespaceEntries(manifest: RolesManifest): NamespaceEntry[] { + return manifest.roles.flatMap((role) => + ROLE_RESOURCE_TYPES.flatMap((type) => + role.resources[type].map((namespace) => ({ type, namespace, owner: `role ${role.id}` })), + ), + ); +} + /** * The manifest file is absent: a team that does not use roles, not a broken one. * Callers that fall back to unfiltered delivery must react to this case ONLY — From d606798c910fec5ca9c180551f8aa233daf2208a Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Tue, 22 Sep 2026 19:13:33 +0200 Subject: [PATCH 20/25] fix(pull): check roles.yaml against projects.yaml for role-less members too Review finding on #710. The cross-manifest case-alias check ran only when the member had a role, so a project-only member pulling skills/Common with a role's skills/common in the same repo was not stopped. roles.yaml is now read whenever a projects manifest is in play; an absent one stays absent, a broken one fails the pull as it does for a member with a role. --- .../resource-namespaces-case-alias.test.ts | 25 ++++++++++++++- src/resource-namespaces.ts | 32 +++++++++++-------- 2 files changed, 43 insertions(+), 14 deletions(-) diff --git a/src/__tests__/resource-namespaces-case-alias.test.ts b/src/__tests__/resource-namespaces-case-alias.test.ts index eae79c99..380da295 100644 --- a/src/__tests__/resource-namespaces-case-alias.test.ts +++ b/src/__tests__/resource-namespaces-case-alias.test.ts @@ -13,12 +13,13 @@ function repoWith(roles: string, projects: string): string { return repoDir; } -function localConfig(repoDir: string): LocalConfig { +function localConfig(repoDir: string, overrides: Partial = {}): LocalConfig { return { repo: { localPath: repoDir, remote: 'https://github.com/acme/team.git' }, username: 'e2e', primaryRole: 'fe', projects: ['p'], + ...overrides, } as LocalConfig; } @@ -36,6 +37,28 @@ describe('resolveResourceNamespaces: roles.yaml and projects.yaml share one dire } }); + it('rejects it for a member with no role too: the collision is in the repo, not in who pulls', async () => { + const repoDir = repoWith(ROLES, 'version: 1\nprojects:\n - id: p\n name: P\n resources: { skills: [Frontend] }\n'); + try { + await expect(resolveResourceNamespaces(localConfig(repoDir, { primaryRole: undefined }))).rejects.toThrow( + /skills namespaces "frontend" \(role fe\) and "Frontend" \(project p\) differ only by case/, + ); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + + it('a member with no role and no roles.yaml still resolves project namespaces', async () => { + const repoDir = repoWith('', 'version: 1\nprojects:\n - id: p\n name: P\n resources: { skills: [p-only] }\n'); + rmSync(path.join(repoDir, 'manifest', 'roles.yaml')); + try { + const resolved = await resolveResourceNamespaces(localConfig(repoDir, { primaryRole: undefined })); + expect(resolved?.activeNamespaces.skills).toEqual(['p-only']); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('accepts the same spelling shared by a role and a project', async () => { const repoDir = repoWith(ROLES, 'version: 1\nprojects:\n - id: p\n name: P\n resources: { skills: [frontend, p-only] }\n'); try { diff --git a/src/resource-namespaces.ts b/src/resource-namespaces.ts index b7685bf8..1f917433 100644 --- a/src/resource-namespaces.ts +++ b/src/resource-namespaces.ts @@ -5,6 +5,7 @@ import { roleNamespaceEntries, RolesManifestMissingError, type ResourceNamespaces, + type RolesManifest, } from './roles.js'; import { loadProjectsManifest, resolveProjectResourceNamespaces, mergeNamespaces, projectNamespaceEntries } from './projects.js'; import { assertNoCaseAliasedNamespaces } from './manifest-schema.js'; @@ -44,8 +45,12 @@ export async function resolveResourceNamespaces(localConfig: LocalConfig) { // ── Role namespaces (optional) ── let roleNamespaces: ResourceNamespaces = { knowledge: [], skills: [], learnings: [], agents: [] }; let allRoleSkillNamespaces = new Set(); - if (primaryRole) { - let rolesManifest; + // roles.yaml is read for a member with a role, and also for a role-less member + // whenever a projects manifest is in play: the two manifests share skills/, + // knowledge/ and agents/, so a project's `Common` collides with a role's + // `common` whether or not this member holds that role. + let rolesManifest: RolesManifest | null = null; + if (primaryRole || projectsManifest) { try { rolesManifest = await loadRolesManifest(localConfig.repo.localPath); } catch (error) { @@ -55,18 +60,19 @@ export async function resolveResourceNamespaces(localConfig: LocalConfig) { // to gate. Let it fail the scope's pull, as an invalid projects manifest // already does. if (!(error instanceof RolesManifestMissingError)) throw error; - log.warn('Roles manifest not found. Skipping role-based filtering.'); - rolesManifest = null; - } - if (rolesManifest && projectsManifest) { - // Each manifest is checked on its own when it loads; the two together share - // the same skills/, knowledge/ and agents/ directories, so a role's - // `frontend` and a project's `Frontend` collide just as two roles' would. - assertNoCaseAliasedNamespaces( - [...roleNamespaceEntries(rolesManifest), ...projectNamespaceEntries(projectsManifest)], - 'manifests (roles.yaml with projects.yaml)', - ); + if (primaryRole) log.warn('Roles manifest not found. Skipping role-based filtering.'); } + } + if (rolesManifest && projectsManifest) { + // Each manifest is checked on its own when it loads; the two together share + // the same skills/, knowledge/ and agents/ directories, so a role's + // `frontend` and a project's `Frontend` collide just as two roles' would. + assertNoCaseAliasedNamespaces( + [...roleNamespaceEntries(rolesManifest), ...projectNamespaceEntries(projectsManifest)], + 'manifests (roles.yaml with projects.yaml)', + ); + } + if (primaryRole) { if (rolesManifest) { try { roleNamespaces = resolveRoleResourceNamespaces({ From 1ce9c4c35cb6915c83e3c44b27f1627901ff6fc2 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 07:33:27 +0200 Subject: [PATCH 21/25] fix(manifest): fold case the way filesystems do when comparing namespaces MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding on #710. The alias key was normalize('NFC').toLowerCase(), which is not case folding: 'σ'/'ς' and 's'/'ſ' stayed distinct although case-insensitive filesystems give each pair one directory. The key now upper- then lowercases each code point on its own, which folds both pairs and sidesteps the context-sensitive final-sigma rule. It errs toward joining ('ß'/'ss', 'ı'/'i'), which can only reject a pair. --- docs/usage-guide.md | 2 +- docs/usage-guide.zh-CN.md | 2 +- .../resource-namespaces-case-alias.test.ts | 14 ++++++++++++ src/__tests__/roles.test.ts | 22 +++++++++++++++++++ src/manifest-schema.ts | 15 ++++++++++++- 5 files changed, 52 insertions(+), 3 deletions(-) diff --git a/docs/usage-guide.md b/docs/usage-guide.md index 68b95607..819d7e1f 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -257,7 +257,7 @@ would arrive as `..` and escape the parent while `frontend.` would arrive as and `..`. Anything else a filesystem accepts stays valid — a non-ASCII name, one holding a space inside it, or one that merely starts like a device (`console`). Two namespaces of the same resource type may not differ only by case (`frontend` -and `Frontend`): on the default Windows and macOS filesystems they are one +and `Frontend`, or under Unicode case folding `σ` and `ς`): on the default Windows and macOS filesystems they are one directory, so a role or project scoped to one would read the other's resources. The check spans both manifests, since `roles.yaml` and `projects.yaml` share the same `skills/`, `knowledge/` and `agents/` directories. diff --git a/docs/usage-guide.zh-CN.md b/docs/usage-guide.zh-CN.md index e241984c..13a566fe 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -238,7 +238,7 @@ projects: 因此 `.. ` 最终变成 `..` 越出上级目录，`frontend.` 最终变成 `frontend` 落进另一个 namespace 的目录；该规则同时排除了 `.` 与 `..`。除此之外不受限制 —— 非 ASCII 名称、 名称中间含空格的目录、以及只是以设备名开头的名称（如 `console`）仍然合法。 -同一资源类型下的两个 namespace 不能仅有大小写差异（如 `frontend` 与 `Frontend`）：在 +同一资源类型下的两个 namespace 不能仅有大小写差异（如 `frontend` 与 `Frontend`，按 Unicode 大小写折叠 `σ` 与 `ς` 也算）：在 Windows 与 macOS 的默认文件系统上它们是同一个目录，限定到其中一个的 role 或 project 会读到另一个的资源。该校验跨越两个 manifest，因为 `roles.yaml` 与 `projects.yaml` 共用 同一套 `skills/`、`knowledge/`、`agents/` 目录。 diff --git a/src/__tests__/resource-namespaces-case-alias.test.ts b/src/__tests__/resource-namespaces-case-alias.test.ts index 380da295..9d5d9719 100644 --- a/src/__tests__/resource-namespaces-case-alias.test.ts +++ b/src/__tests__/resource-namespaces-case-alias.test.ts @@ -48,6 +48,20 @@ describe('resolveResourceNamespaces: roles.yaml and projects.yaml share one dire } }); + it('rejects a project namespace that aliases a role namespace only under Unicode case folding', async () => { + const repoDir = repoWith( + 'version: 1\nroles:\n - id: fe\n resources: { knowledge: [], skills: [ΟΔΟΣ] }\n', + 'version: 1\nprojects:\n - id: p\n name: P\n resources: { skills: [οδοσ] }\n', + ); + try { + await expect(resolveResourceNamespaces(localConfig(repoDir))).rejects.toThrow( + 'skills namespaces "ΟΔΟΣ" (role fe) and "οδοσ" (project p) differ only by case', + ); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('a member with no role and no roles.yaml still resolves project namespaces', async () => { const repoDir = repoWith('', 'version: 1\nprojects:\n - id: p\n name: P\n resources: { skills: [p-only] }\n'); rmSync(path.join(repoDir, 'manifest', 'roles.yaml')); diff --git a/src/__tests__/roles.test.ts b/src/__tests__/roles.test.ts index ecf9f6e1..42cb832f 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -227,6 +227,28 @@ roles: } }); + // Lowercasing alone keeps these apart; the filesystems' case folding does not. + it.each([ + ['final sigma', 'ας', 'ασ'], + ['long s', 'ſkills', 'skills'], + ])('across roles, when only Unicode case folding joins them (%s)', async (_label, first, second) => { + const repoDir = writeManifest(` +version: 1 +roles: + - id: fe + resources: { knowledge: [], skills: [${first}] } + - id: fe2 + resources: { knowledge: [], skills: [${second}] } +`); + try { + await expect(loadRolesManifest(repoDir)).rejects.toThrow( + `Invalid roles manifest: skills namespaces "${first}" (role fe) and "${second}" (role fe2) differ only by case`, + ); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('but not the same spelling used twice, nor the same name under two resource types', async () => { const repoDir = writeManifest(` version: 1 diff --git a/src/manifest-schema.ts b/src/manifest-schema.ts index 4bf36028..bf3ca0cb 100644 --- a/src/manifest-schema.ts +++ b/src/manifest-schema.ts @@ -146,6 +146,19 @@ export interface NamespaceEntry { owner: string; } +/** + * Approximates Unicode case folding, which JavaScript does not expose. + * `toLowerCase()` alone is not folding: it keeps `σ`/`ς` and `s`/`ſ` apart, + * which case-insensitive filesystems treat as one name. Upper- then lowercasing + * each code point on its own folds those, and stays clear of the final-sigma + * rule, which only applies when a cased letter precedes the sigma. It errs + * toward joining (`ß`/`ss` and `ı`/`i` count as one name), which can only + * reject a pair, never let an alias through. + */ +function caseFoldKey(name: string): string { + return Array.from(name.normalize('NFC'), (ch) => ch.toUpperCase().toLowerCase()).join('').normalize('NFC'); +} + /** * Two namespaces of the same resource type that differ only by case (or by * Unicode normalization) name one directory on the default Windows and macOS @@ -156,7 +169,7 @@ export interface NamespaceEntry { export function assertNoCaseAliasedNamespaces(entries: Iterable, kind: string): void { const seen = new Map(); for (const entry of entries) { - const key = `${entry.type}/${entry.namespace.normalize('NFC').toLowerCase()}`; + const key = `${entry.type}/${caseFoldKey(entry.namespace)}`; const prior = seen.get(key); if (!prior) { seen.set(key, entry); From e6ef55845a2e9d23fbc456cead7de3967f0a89a0 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 07:35:17 +0200 Subject: [PATCH 22/25] fix(config): keep the config loadable when the roles manifest is broken MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review finding on #710. migrateLegacyRoleConfig rethrew a manifest parse error, which loadLocalConfig caught and turned into null, so every command reported "teamai is not initialized" — pull included, leaving the member no way to fetch the fixed manifest. The migration now skips with a warning and returns the config unmigrated. That alone would widen delivery: a role-less member who would have been migrated to 'hai' reached resolveResourceNamespaces' unfiltered early return without roles.yaml being read. roles.yaml is now read for every member before that return, so a broken one fails the pull (absent still means unfiltered). --- CHANGELOG.md | 2 +- .../config-legacy-role-migration.test.ts | 54 +++++++++++++++++++ .../resource-namespaces-case-alias.test.ts | 28 ++++++++++ src/config.ts | 11 ++-- src/resource-namespaces.ts | 44 +++++++-------- 5 files changed, 110 insertions(+), 29 deletions(-) create mode 100644 src/__tests__/config-legacy-role-migration.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 325c37df..f43d1a85 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,7 @@ All notable changes to this project will be documented in this file. See [standa ### 🐛 Bug Fixes - `teamai init` no longer hangs without a terminal. When the provider had no session it spawned `gh auth login --web` (or `gf auth login`, `cnb login`) with inherited stdio and waited for a browser device flow that nobody could complete, about five minutes for GitHub, then exited with the provider's error and no hint of the missing credential. Each login now refuses up front when the run is not interactive and names the credential to prepare (`GITHUB_TOKEN` / `GH_TOKEN`, `CNB_TOKEN`, or for TGit a prior `gf auth login`, since a `TGIT_TOKEN` PAT is REST-API-only and cannot clone). A run is non-interactive when stdin is not a TTY or when `CI` or `TEAMAI_NONINTERACTIVE` is set, so an agent sandbox with a pseudo-terminal can declare itself unattended, and every prompt in the CLI follows the same rule. `git` also runs with its prompts closed in that case — `GIT_TERMINAL_PROMPT=0`, `GIT_ASKPASS=echo` and `GCM_INTERACTIVE=never`, each only where the caller set nothing — so a missing clone credential fails at once instead of waiting on a terminal prompt or an askpass or credential-manager dialog. `ssh` keeps its own settings: its batch flag is only reachable through `GIT_SSH_COMMAND`, which would override each repository's `core.sshCommand` (for [#711](https://github.com/Tencent/teamai-cli/issues/711)). -- A `manifest/roles.yaml` that exists but does not parse now fails the pull for that scope instead of warning and syncing with no role filter at all. The same applies to `init`, `push` and the legacy role migration, which each fell back to a guess at the namespaces when any error came out of the loader. For a member with no active project that fallback meant an unfiltered sync, so a broken manifest delivered every namespace it was written to gate. Only an absent manifest still means "this team does not use roles"; an unreadable or empty file is an error, as it now is for `manifest/projects.yaml` too. +- A `manifest/roles.yaml` that exists but does not parse now fails the pull for that scope instead of warning and syncing with no role filter at all, for a member with no role as much as for one with a role. The same applies to `init` and `push`, which each fell back to a guess at the namespaces when any error came out of the loader. The legacy role migration skips with a warning instead of failing, so every command, `pull` included, still loads the config and can fetch the fixed manifest. For a member with no active project that fallback meant an unfiltered sync, so a broken manifest delivered every namespace it was written to gate. Only an absent manifest still means "this team does not use roles"; an unreadable or empty file is an error, as it now is for `manifest/projects.yaml` too. - Cache GC now rejects partial integers such as `12abc`, decimals, zero and unsafe integers for `--max-bytes` and `--stale-days` before deleting anything. An invalid `TEAMAI_CACHE_MAX_BYTES` value falls back to the default 5 GB limit instead of using a numeric prefix. - Usage reporting scopes sessions by path on Windows too. The project/user scope filter compared an event's `cwd` against `projectRoot` with a hard-coded `/` separator, so on Windows only a session started in the project root itself matched: every session started in a subdirectory was dropped from the project team's report and counted in the user scope's instead, which is the isolation the usage guide promises. Windows paths are also compared case-insensitively, so a drive letter or a directory name spelled with different case in the two sources no longer leaks a project session into the user scope. POSIX paths keep their own rules: case-sensitive, and a `\` in a filename stays part of the name. - `teamai tags subscribe` and `teamai tags unsubscribe` now invalidate the pull revision cache, as `teamai skill exclude` already does, so the next `teamai pull` applies the new subscriptions instead of reporting "Already synced" when the team repo has not changed. diff --git a/src/__tests__/config-legacy-role-migration.test.ts b/src/__tests__/config-legacy-role-migration.test.ts new file mode 100644 index 00000000..72aa8b25 --- /dev/null +++ b/src/__tests__/config-legacy-role-migration.test.ts @@ -0,0 +1,54 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { loadLocalConfig } from '../config.js'; +import { log } from '../utils/logger.js'; + +describe('loadLocalConfig: legacy role migration against the team repo roles manifest', () => { + const originalHome = process.env.HOME; + let home: string; + let repoDir: string; + + beforeEach(() => { + home = mkdtempSync(path.join(os.tmpdir(), 'teamai-legacy-role-')); + process.env.HOME = home; + repoDir = path.join(home, 'team-repo'); + mkdirSync(path.join(repoDir, 'manifest'), { recursive: true }); + mkdirSync(path.join(home, '.teamai'), { recursive: true }); + writeFileSync( + path.join(home, '.teamai', 'config.yaml'), + `repo:\n localPath: ${repoDir}\n remote: https://github.com/acme/team.git\nusername: dev\n`, + 'utf-8', + ); + }); + + afterEach(() => { + vi.restoreAllMocks(); + if (originalHome === undefined) delete process.env.HOME; + else process.env.HOME = originalHome; + rmSync(home, { recursive: true, force: true }); + }); + + function writeRoles(content: string): void { + writeFileSync(path.join(repoDir, 'manifest', 'roles.yaml'), content, 'utf-8'); + } + + it('migrates a role-less config to the hai role when the manifest declares it', async () => { + writeRoles('version: 1\nroles:\n - id: hai\n resources: { knowledge: [], skills: [hai] }\n'); + const config = await loadLocalConfig(); + expect(config?.primaryRole).toBe('hai'); + }); + + // Every command loads the config, `pull` included, so a broken manifest that + // failed the load would leave the member unable to pull the fix. The pull + // itself refuses a broken manifest, which is what keeps delivery from widening. + it('keeps the config loadable when the manifest does not parse, and says why it was not migrated', async () => { + writeRoles("version: 1\nroles:\n - id: hai\n resources: { knowledge: [], skills: ['../../evil'] }\n"); + const warn = vi.spyOn(log, 'warn').mockImplementation(() => {}); + const config = await loadLocalConfig(); + expect(config).not.toBeNull(); + expect(config?.primaryRole).toBeUndefined(); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('Invalid roles manifest')); + }); +}); diff --git a/src/__tests__/resource-namespaces-case-alias.test.ts b/src/__tests__/resource-namespaces-case-alias.test.ts index 9d5d9719..65867eed 100644 --- a/src/__tests__/resource-namespaces-case-alias.test.ts +++ b/src/__tests__/resource-namespaces-case-alias.test.ts @@ -62,6 +62,34 @@ describe('resolveResourceNamespaces: roles.yaml and projects.yaml share one dire } }); + // A role-less config is migrated to a manifest-declared `hai` role when the + // manifest parses, so a broken one does gate this member: it must fail the + // pull rather than let it fall through to an unfiltered sync. + it('fails the pull of a member with no role, no project and no projects.yaml when roles.yaml does not parse', async () => { + const repoDir = repoWith("version: 1\nroles:\n - id: hai\n resources: { knowledge: [], skills: ['../../evil'] }\n", ''); + rmSync(path.join(repoDir, 'manifest', 'projects.yaml')); + try { + await expect( + resolveResourceNamespaces(localConfig(repoDir, { primaryRole: undefined, projects: [] })), + ).rejects.toThrow(/Invalid roles manifest/); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + + it('keeps the unfiltered sync for that member when roles.yaml is absent', async () => { + const repoDir = repoWith('', ''); + rmSync(path.join(repoDir, 'manifest', 'roles.yaml')); + rmSync(path.join(repoDir, 'manifest', 'projects.yaml')); + try { + await expect( + resolveResourceNamespaces(localConfig(repoDir, { primaryRole: undefined, projects: [] })), + ).resolves.toBeNull(); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + it('a member with no role and no roles.yaml still resolves project namespaces', async () => { const repoDir = repoWith('', 'version: 1\nprojects:\n - id: p\n name: P\n resources: { skills: [p-only] }\n'); rmSync(path.join(repoDir, 'manifest', 'roles.yaml')); diff --git a/src/config.ts b/src/config.ts index 8c4b0eef..6352fbd8 100644 --- a/src/config.ts +++ b/src/config.ts @@ -30,9 +30,14 @@ async function migrateLegacyRoleConfig(config: LocalConfig, configPath: string): try { manifest = await loadRolesManifest(config.repo.localPath); } catch (error) { - // A repo with no manifest has nothing to migrate. A broken one leaves the - // config role-less, which downstream reads as "no filter", so it surfaces. - if (!(error instanceof RolesManifestMissingError)) throw error; + // A repo with no manifest has nothing to migrate. A broken one must not + // fail the load: every command loads the config, `pull` included, so the + // member could never pull the fix. The config stays role-less for this run, + // and the pull refuses the broken manifest itself (resolveResourceNamespaces), + // so delivery does not widen. + if (!(error instanceof RolesManifestMissingError)) { + log.warn(`Legacy role migration skipped: ${(error as Error).message}`); + } return config; } diff --git a/src/resource-namespaces.ts b/src/resource-namespaces.ts index 1f917433..1bb4d966 100644 --- a/src/resource-namespaces.ts +++ b/src/resource-namespaces.ts @@ -23,6 +23,25 @@ export async function resolveResourceNamespaces(localConfig: LocalConfig) { const projectsManifest = await loadProjectsManifest(localConfig.repo.localPath); const teamHasProjects = !!projectsManifest && projectsManifest.projects.length > 0; + // roles.yaml is read for every member, before any early return. A member with + // a role is filtered by it; a role-less one is still gated by it twice over: + // the legacy migration assigns a manifest-declared `hai` role (and skips, with + // a warning, when the manifest does not parse), and the manifest shares + // skills/, knowledge/ and agents/ with projects.yaml, so a project's `Common` + // collides with a role's `common` whether or not this member holds that role. + let rolesManifest: RolesManifest | null = null; + try { + rolesManifest = await loadRolesManifest(localConfig.repo.localPath); + } catch (error) { + // Only an ABSENT manifest degrades to unfiltered delivery. One that exists + // and does not parse must not: every path below this point would treat the + // roles as "no filter" and deliver the namespaces the manifest was written + // to gate. Let it fail the scope's pull, as an invalid projects manifest + // already does. + if (!(error instanceof RolesManifestMissingError)) throw error; + if (primaryRole) log.warn('Roles manifest not found. Skipping role-based filtering.'); + } + // When there is nothing to filter by AND the team does not use project // partitioning, keep the legacy unfiltered behavior (null = sync everything). // @@ -33,36 +52,11 @@ export async function resolveResourceNamespaces(localConfig: LocalConfig) { // through to an unfiltered sync that reinstalls every project's skills/rules. // So we return a real (possibly empty-active) context and let the cleanup path // below prune the now-inactive project namespaces. - // - // This returns before roles.yaml is read, and deliberately so: the branch is - // reached only when the member HAS NO ROLE, and a role-less member resolves to - // the same unfiltered sync when roles.yaml is perfectly valid — every role - // namespace below is gated on `primaryRole`. The manifest gates nothing for - // them, so reading it here could only add a new way for their pull to fail, - // never close a gap. A member WITH a role never reaches this line. if (!hasRole && !hasProjects && !teamHasProjects) return null; // ── Role namespaces (optional) ── let roleNamespaces: ResourceNamespaces = { knowledge: [], skills: [], learnings: [], agents: [] }; let allRoleSkillNamespaces = new Set(); - // roles.yaml is read for a member with a role, and also for a role-less member - // whenever a projects manifest is in play: the two manifests share skills/, - // knowledge/ and agents/, so a project's `Common` collides with a role's - // `common` whether or not this member holds that role. - let rolesManifest: RolesManifest | null = null; - if (primaryRole || projectsManifest) { - try { - rolesManifest = await loadRolesManifest(localConfig.repo.localPath); - } catch (error) { - // Only an ABSENT manifest degrades to unfiltered delivery. One that exists - // and does not parse must not: every path below this point would treat the - // roles as "no filter" and deliver the namespaces the manifest was written - // to gate. Let it fail the scope's pull, as an invalid projects manifest - // already does. - if (!(error instanceof RolesManifestMissingError)) throw error; - if (primaryRole) log.warn('Roles manifest not found. Skipping role-based filtering.'); - } - } if (rolesManifest && projectsManifest) { // Each manifest is checked on its own when it loads; the two together share // the same skills/, knowledge/ and agents/ directories, so a role's From 2d0bb359f8a37a4614d374b459af84dabe96c2f9 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 07:43:39 +0200 Subject: [PATCH 23/25] fix(status): report a resource type it cannot scan instead of crashing Found running the real CLI on #710. scanLocalForPush resolves namespaces through the roles manifest (agents via resolveResourceNamespaces, skills when it falls back to role ids), and a manifest that does not parse now throws there instead of being read as "no filter". status let that escape as a stack trace after printing half its report. Status is where a member looks to find out why pull failed, so it now warns with the error for that type and lists the rest, as it already does for git status. --- src/__tests__/status-broken-manifest.test.ts | 56 ++++++++++++++++++++ src/status.ts | 10 +++- 2 files changed, 65 insertions(+), 1 deletion(-) create mode 100644 src/__tests__/status-broken-manifest.test.ts diff --git a/src/__tests__/status-broken-manifest.test.ts b/src/__tests__/status-broken-manifest.test.ts new file mode 100644 index 00000000..a95502e8 --- /dev/null +++ b/src/__tests__/status-broken-manifest.test.ts @@ -0,0 +1,56 @@ +import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { rmSync } from 'node:fs'; + +const { repoDir } = vi.hoisted(() => { + const { mkdtempSync } = require('node:fs') as typeof import('node:fs'); + const os = require('node:os') as typeof import('node:os'); + const path = require('node:path') as typeof import('node:path'); + return { repoDir: mkdtempSync(path.join(os.tmpdir(), 'teamai-status-broken-')) }; +}); + +vi.mock('../config.js', async (importOriginal) => ({ + ...(await importOriginal()), + autoDetectInit: vi.fn().mockResolvedValue({ + localConfig: { repo: { localPath: repoDir, remote: 'https://github.com/acme/team.git' }, username: 'dev', scope: 'user' }, + teamConfig: {}, + }), + loadStateForScope: vi.fn().mockResolvedValue({}), +})); +vi.mock('../utils/git.js', () => ({ getRepoStatus: vi.fn().mockResolvedValue({ ahead: 0, behind: 0, modified: [] }) })); +vi.mock('../resources/index.js', () => ({ + getAllHandlers: vi.fn().mockReturnValue([ + { + type: 'skills', + scanLocalForPush: vi.fn().mockRejectedValue(new Error('Invalid roles manifest: roles.0.resources.skills.0: bad')), + }, + { type: 'rules', scanLocalForPush: vi.fn().mockResolvedValue([{ name: 'r' }]) }, + ]), +})); + +import { status } from '../status.js'; +import { log } from '../utils/logger.js'; + +describe('status with a roles manifest that does not parse', () => { + let out: string[]; + + beforeEach(() => { + out = []; + vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { out.push(args.join(' ')); }); + vi.spyOn(log, 'info').mockImplementation(() => {}); + }); + + afterEach(() => { + vi.restoreAllMocks(); + }); + + afterAll(() => { + rmSync(repoDir, { recursive: true, force: true }); + }); + + it('reports the type it could not scan and still lists the rest', async () => { + const warn = vi.spyOn(log, 'warn').mockImplementation(() => {}); + await expect(status({})).resolves.toBeUndefined(); + expect(warn).toHaveBeenCalledWith(expect.stringContaining('[skills] could not scan: Invalid roles manifest')); + expect(out).toContain(' [rules] 1 new'); + }); +}); diff --git a/src/status.ts b/src/status.ts index 1ddaa405..3e1d00ae 100644 --- a/src/status.ts +++ b/src/status.ts @@ -126,7 +126,15 @@ export async function status(options: GlobalOptions): Promise { log.info('Local resources not yet pushed:'); let anyNew = false; for (const handler of getAllHandlers()) { - const items = await handler.scanLocalForPush(teamConfig, localConfig); + let items; + try { + items = await handler.scanLocalForPush(teamConfig, localConfig); + } catch (e) { + // A manifest that does not parse fails the pull and the push; status is + // where the member looks to find out why, so it reports and goes on. + log.warn(` [${handler.type}] could not scan: ${(e as Error).message}`); + continue; + } if (items.length > 0) { anyNew = true; console.log(` [${handler.type}] ${items.length} new`); From 0f80dccb4c25887e0e67444a15e4147bbded5dd3 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 08:15:58 +0200 Subject: [PATCH 24/25] fix(status): do not report "(none)" when a resource type could not be scanned Review finding on #710. With every successful scan empty and one type failing, status printed the warning and then "(none)", which reads as a complete clean result. It now says "(none in the types that could be scanned)" in that case. --- src/__tests__/status-broken-manifest.test.ts | 30 ++++++++++++++------ src/status.ts | 5 +++- 2 files changed, 25 insertions(+), 10 deletions(-) diff --git a/src/__tests__/status-broken-manifest.test.ts b/src/__tests__/status-broken-manifest.test.ts index a95502e8..de0b8da8 100644 --- a/src/__tests__/status-broken-manifest.test.ts +++ b/src/__tests__/status-broken-manifest.test.ts @@ -1,29 +1,32 @@ import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { rmSync } from 'node:fs'; -const { repoDir } = vi.hoisted(() => { +const { repoDir, rules } = vi.hoisted(() => { const { mkdtempSync } = require('node:fs') as typeof import('node:fs'); const os = require('node:os') as typeof import('node:os'); const path = require('node:path') as typeof import('node:path'); - return { repoDir: mkdtempSync(path.join(os.tmpdir(), 'teamai-status-broken-')) }; + return { + repoDir: mkdtempSync(path.join(os.tmpdir(), 'teamai-status-broken-')), + rules: { pending: [{ name: 'r' }] as Array<{ name: string }> }, + }; }); vi.mock('../config.js', async (importOriginal) => ({ ...(await importOriginal()), - autoDetectInit: vi.fn().mockResolvedValue({ + autoDetectInit: vi.fn(async () => ({ localConfig: { repo: { localPath: repoDir, remote: 'https://github.com/acme/team.git' }, username: 'dev', scope: 'user' }, teamConfig: {}, - }), - loadStateForScope: vi.fn().mockResolvedValue({}), + })), + loadStateForScope: vi.fn(async () => ({})), })); -vi.mock('../utils/git.js', () => ({ getRepoStatus: vi.fn().mockResolvedValue({ ahead: 0, behind: 0, modified: [] }) })); +vi.mock('../utils/git.js', () => ({ getRepoStatus: vi.fn(async () => ({ ahead: 0, behind: 0, modified: [] })) })); vi.mock('../resources/index.js', () => ({ - getAllHandlers: vi.fn().mockReturnValue([ + getAllHandlers: vi.fn(() => [ { type: 'skills', - scanLocalForPush: vi.fn().mockRejectedValue(new Error('Invalid roles manifest: roles.0.resources.skills.0: bad')), + scanLocalForPush: vi.fn(async () => { throw new Error('Invalid roles manifest: roles.0.resources.skills.0: bad'); }), }, - { type: 'rules', scanLocalForPush: vi.fn().mockResolvedValue([{ name: 'r' }]) }, + { type: 'rules', scanLocalForPush: vi.fn(async () => rules.pending) }, ]), })); @@ -35,6 +38,7 @@ describe('status with a roles manifest that does not parse', () => { beforeEach(() => { out = []; + rules.pending = [{ name: 'r' }]; vi.spyOn(console, 'log').mockImplementation((...args: unknown[]) => { out.push(args.join(' ')); }); vi.spyOn(log, 'info').mockImplementation(() => {}); }); @@ -53,4 +57,12 @@ describe('status with a roles manifest that does not parse', () => { expect(warn).toHaveBeenCalledWith(expect.stringContaining('[skills] could not scan: Invalid roles manifest')); expect(out).toContain(' [rules] 1 new'); }); + + it('does not report "(none)" when a type could not be scanned', async () => { + rules.pending = []; + vi.spyOn(log, 'warn').mockImplementation(() => {}); + await status({}); + expect(out).not.toContain(' (none)'); + expect(out).toContain(' (none in the types that could be scanned)'); + }); }); diff --git a/src/status.ts b/src/status.ts index 3e1d00ae..a030cf7e 100644 --- a/src/status.ts +++ b/src/status.ts @@ -125,6 +125,7 @@ export async function status(options: GlobalOptions): Promise { console.log(''); log.info('Local resources not yet pushed:'); let anyNew = false; + let anyUnscanned = false; for (const handler of getAllHandlers()) { let items; try { @@ -133,6 +134,7 @@ export async function status(options: GlobalOptions): Promise { // A manifest that does not parse fails the pull and the push; status is // where the member looks to find out why, so it reports and goes on. log.warn(` [${handler.type}] could not scan: ${(e as Error).message}`); + anyUnscanned = true; continue; } if (items.length > 0) { @@ -146,7 +148,8 @@ export async function status(options: GlobalOptions): Promise { } } if (!anyNew) { - console.log(' (none)'); + // A bare "(none)" would read as a clean result for the types it never saw. + console.log(anyUnscanned ? ' (none in the types that could be scanned)' : ' (none)'); } console.log(''); From 2965e4d50a2731286aa617a7664e8113bb880070 Mon Sep 17 00:00:00 2001 From: Saul Moro Date: Wed, 23 Sep 2026 08:15:58 +0200 Subject: [PATCH 25/25] fix(config): a role the manifest could not resolve matches no role-scoped entry Review finding on #710. When the legacy role migration cannot read the roles manifest, the config stayed plainly role-less, and resolveMembership reads role-less as "every role": hooks, MCP servers and env variables scoped to roles reached a member the manifest would have made 'hai'. The pull refused the manifest for skills, but those reconcilers still ran. The migration now marks the in-memory config roleUnresolved, a runtime-only field like dataHome that serializeLocalConfig drops and the schema strips on load. activeRoleIds returns [] for it, so role-scoped entries reach nobody, unscoped ones apply as before, and the reconcilers remove role-scoped entries already installed. The next load decides the role again. --- CHANGELOG.md | 2 +- .../config-legacy-role-migration.test.ts | 34 +++++++++++++++++-- src/config.ts | 17 +++++----- src/membership.ts | 2 +- src/roles.ts | 7 +++- src/types.ts | 7 +++- 6 files changed, 55 insertions(+), 14 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f43d1a85..be98f8f3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -26,7 +26,7 @@ All notable changes to this project will be documented in this file. See [standa ### 🐛 Bug Fixes - `teamai init` no longer hangs without a terminal. When the provider had no session it spawned `gh auth login --web` (or `gf auth login`, `cnb login`) with inherited stdio and waited for a browser device flow that nobody could complete, about five minutes for GitHub, then exited with the provider's error and no hint of the missing credential. Each login now refuses up front when the run is not interactive and names the credential to prepare (`GITHUB_TOKEN` / `GH_TOKEN`, `CNB_TOKEN`, or for TGit a prior `gf auth login`, since a `TGIT_TOKEN` PAT is REST-API-only and cannot clone). A run is non-interactive when stdin is not a TTY or when `CI` or `TEAMAI_NONINTERACTIVE` is set, so an agent sandbox with a pseudo-terminal can declare itself unattended, and every prompt in the CLI follows the same rule. `git` also runs with its prompts closed in that case — `GIT_TERMINAL_PROMPT=0`, `GIT_ASKPASS=echo` and `GCM_INTERACTIVE=never`, each only where the caller set nothing — so a missing clone credential fails at once instead of waiting on a terminal prompt or an askpass or credential-manager dialog. `ssh` keeps its own settings: its batch flag is only reachable through `GIT_SSH_COMMAND`, which would override each repository's `core.sshCommand` (for [#711](https://github.com/Tencent/teamai-cli/issues/711)). -- A `manifest/roles.yaml` that exists but does not parse now fails the pull for that scope instead of warning and syncing with no role filter at all, for a member with no role as much as for one with a role. The same applies to `init` and `push`, which each fell back to a guess at the namespaces when any error came out of the loader. The legacy role migration skips with a warning instead of failing, so every command, `pull` included, still loads the config and can fetch the fixed manifest. For a member with no active project that fallback meant an unfiltered sync, so a broken manifest delivered every namespace it was written to gate. Only an absent manifest still means "this team does not use roles"; an unreadable or empty file is an error, as it now is for `manifest/projects.yaml` too. +- A `manifest/roles.yaml` that exists but does not parse now fails the pull for that scope instead of warning and syncing with no role filter at all, for a member with no role as much as for one with a role. The same applies to `init` and `push`, which each fell back to a guess at the namespaces when any error came out of the loader. The legacy role migration skips with a warning instead of failing, so every command, `pull` included, still loads the config and can fetch the fixed manifest; until it can run, the member holds no role rather than every role, so hooks, MCP servers and env variables scoped by `roles:` reach them no more than skills do. For a member with no active project that fallback meant an unfiltered sync, so a broken manifest delivered every namespace it was written to gate. Only an absent manifest still means "this team does not use roles"; an unreadable or empty file is an error, as it now is for `manifest/projects.yaml` too. - Cache GC now rejects partial integers such as `12abc`, decimals, zero and unsafe integers for `--max-bytes` and `--stale-days` before deleting anything. An invalid `TEAMAI_CACHE_MAX_BYTES` value falls back to the default 5 GB limit instead of using a numeric prefix. - Usage reporting scopes sessions by path on Windows too. The project/user scope filter compared an event's `cwd` against `projectRoot` with a hard-coded `/` separator, so on Windows only a session started in the project root itself matched: every session started in a subdirectory was dropped from the project team's report and counted in the user scope's instead, which is the isolation the usage guide promises. Windows paths are also compared case-insensitively, so a drive letter or a directory name spelled with different case in the two sources no longer leaks a project session into the user scope. POSIX paths keep their own rules: case-sensitive, and a `\` in a filename stays part of the name. - `teamai tags subscribe` and `teamai tags unsubscribe` now invalidate the pull revision cache, as `teamai skill exclude` already does, so the next `teamai pull` applies the new subscriptions instead of reporting "Already synced" when the team repo has not changed. diff --git a/src/__tests__/config-legacy-role-migration.test.ts b/src/__tests__/config-legacy-role-migration.test.ts index 72aa8b25..d253abd1 100644 --- a/src/__tests__/config-legacy-role-migration.test.ts +++ b/src/__tests__/config-legacy-role-migration.test.ts @@ -1,8 +1,9 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { mkdtempSync, mkdirSync, writeFileSync, rmSync } from 'node:fs'; +import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, rmSync } from 'node:fs'; import os from 'node:os'; import path from 'node:path'; -import { loadLocalConfig } from '../config.js'; +import { loadLocalConfig, saveLocalConfig } from '../config.js'; +import { matchesMembership, resolveMembership } from '../membership.js'; import { log } from '../utils/logger.js'; describe('loadLocalConfig: legacy role migration against the team repo roles manifest', () => { @@ -51,4 +52,33 @@ describe('loadLocalConfig: legacy role migration against the team repo roles man expect(config?.primaryRole).toBeUndefined(); expect(warn).toHaveBeenCalledWith(expect.stringContaining('Invalid roles manifest')); }); + + // Role-less normally means "no role filter". Here the manifest decides the + // role and cannot be read, so the member must not receive role-scoped hooks, + // MCP servers or env variables — only what is scoped to nobody in particular. + it('leaves a member whose role could not be resolved outside every role-scoped entry', async () => { + writeRoles("version: 1\nroles:\n - id: hai\n resources: { knowledge: [], skills: ['../../evil'] }\n"); + vi.spyOn(log, 'warn').mockImplementation(() => {}); + const config = await loadLocalConfig(); + if (!config) throw new Error('expected a config'); + const membership = resolveMembership(config); + expect(matchesMembership({ roles: ['hai'] }, membership)).toBe(false); + expect(matchesMembership({}, membership)).toBe(true); + }); + + it('does not persist that unresolved state', async () => { + writeRoles("version: 1\nroles:\n - id: hai\n resources: { knowledge: [], skills: ['../../evil'] }\n"); + vi.spyOn(log, 'warn').mockImplementation(() => {}); + const config = await loadLocalConfig(); + if (!config) throw new Error('expected a config'); + await saveLocalConfig(config); + expect(readFileSync(path.join(home, '.teamai', 'config.yaml'), 'utf-8')).not.toMatch(/roleUnresolved/); + }); + + it('keeps the unfiltered match for a role-less member when the manifest parses', async () => { + writeRoles('version: 1\nroles:\n - id: frontend\n resources: { knowledge: [], skills: [frontend] }\n'); + const config = await loadLocalConfig(); + if (!config) throw new Error('expected a config'); + expect(matchesMembership({ roles: ['frontend'] }, resolveMembership(config))).toBe(true); + }); }); diff --git a/src/config.ts b/src/config.ts index 6352fbd8..5680dcce 100644 --- a/src/config.ts +++ b/src/config.ts @@ -32,13 +32,13 @@ async function migrateLegacyRoleConfig(config: LocalConfig, configPath: string): } catch (error) { // A repo with no manifest has nothing to migrate. A broken one must not // fail the load: every command loads the config, `pull` included, so the - // member could never pull the fix. The config stays role-less for this run, - // and the pull refuses the broken manifest itself (resolveResourceNamespaces), - // so delivery does not widen. - if (!(error instanceof RolesManifestMissingError)) { - log.warn(`Legacy role migration skipped: ${(error as Error).message}`); - } - return config; + // member could never pull the fix. Nor may it leave the config plainly + // role-less, which matches every role-scoped hook, MCP server and env + // variable. The role is unknown for this run: skills fail closed in + // resolveResourceNamespaces, and role-scoped entries reach nobody. + if (error instanceof RolesManifestMissingError) return config; + log.warn(`Legacy role migration skipped: ${(error as Error).message}`); + return { ...config, roleUnresolved: true }; } const haiRole = manifest.roles.find((role) => role.id === 'hai'); @@ -98,9 +98,10 @@ export async function loadLocalConfig(): Promise { * `dataHome` is derived from the projectAnchor at runtime and the config file * lives inside that directory, so it must never be persisted (a stale absolute * path would defeat the anchor-derived design and break on another machine). + * `roleUnresolved` describes one load of the roles manifest, not the member. */ function serializeLocalConfig(config: LocalConfig): string { - const { dataHome: _dataHome, ...persisted } = config; + const { dataHome: _dataHome, roleUnresolved: _roleUnresolved, ...persisted } = config; return YAML.stringify(persisted); } diff --git a/src/membership.ts b/src/membership.ts index ab79037a..f3697528 100644 --- a/src/membership.ts +++ b/src/membership.ts @@ -31,7 +31,7 @@ export type EntryScope = Partial>; * 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[] }, + localConfig: { primaryRole?: string; additionalRoles?: string[]; roleUnresolved?: true; projects?: string[] }, ): Membership { return { roles: activeRoleIds(localConfig), diff --git a/src/roles.ts b/src/roles.ts index 6813e811..834b9099 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -229,8 +229,13 @@ export function resolveRoleResourceNamespaces(input: { * Role ids this member holds, primary first, or null when no primary role is * configured. Null means "no role filter": a member without a role keeps * receiving every resource, the same fallback pull applies to skills and rules. + * A config whose role could not be resolved (`roleUnresolved`) holds none: + * `[]` matches no role-scoped entry. */ -export function activeRoleIds(localConfig: { primaryRole?: string; additionalRoles?: string[] }): string[] | null { +export function activeRoleIds( + localConfig: { primaryRole?: string; additionalRoles?: string[]; roleUnresolved?: true }, +): string[] | null { + if (localConfig.roleUnresolved) return []; if (!localConfig.primaryRole) return null; return [...new Set([localConfig.primaryRole, ...(localConfig.additionalRoles ?? [])])]; } diff --git a/src/types.ts b/src/types.ts index 72275b4c..5fb7468c 100644 --- a/src/types.ts +++ b/src/types.ts @@ -578,8 +578,13 @@ export const LocalConfigSchema = z.object({ * - on SAVE, `serializeLocalConfig` also drops it (belt-and-braces) — the * value is anchor-derived at runtime and the config file lives INSIDE it, so * persisting an absolute path would be both redundant and machine-specific. + * + * `roleUnresolved` is runtime-only the same way. It is set when the legacy role + * migration could not read the roles manifest, so whether this role-less config + * holds a role is unknown for this run; `activeRoleIds` then matches no + * role-scoped entry instead of every one. The next load re-decides it. */ -export type LocalConfig = z.infer & { dataHome?: string }; +export type LocalConfig = z.infer & { dataHome?: string; roleUnresolved?: true }; export type LocalConfigInput = z.input; // ─── Local state (~/.teamai/state.json) ────────────────────