diff --git a/CHANGELOG.md b/CHANGELOG.md index 7594c6ca1..7505b06d7 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, as a project id already had to be (the id keeps its own narrower ASCII rule). 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. Nothing else 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 - Built-in skill content ships inside the npm package and is printed by the installed CLI: `teamai skill get [--full] [--all]`, `teamai skill path ` for the directory holding a skill's scripts, and `teamai skill list --json` for the catalog. Agents receive one file, `skills/teamai/SKILL.md`, a discovery stub that points at those commands, so what an agent reads always matches the CLI version it is running. `teamai pull` removes the `team-wiki-codebase`, `teamai-share-learnings` and `teamai/references/*.md` trees earlier releases copied into every agent directory, removing only files whose content a release shipped (an edited file, or a member's own skill under an old name, stays), archiving each removed file under `~/.teamai/removed-skills//…` first, and keeping any directory that holds a member's own file; `teamai uninstall` removes only the packaged files from CLI-owned skill directories by the same rule. `share` is served only while recall is on and the team source is writable (not a read-only HTTP one), and the end-of-session share reminder is withheld until then too. The served workflows are English; learning and knowledge-base documents are still written in Simplified Chinese, and an existing knowledge base keeps its file names and headings. The legacy names still resolve as aliases (for [#678](https://github.com/Tencent/teamai-cli/issues/678), [#730](https://github.com/Tencent/teamai-cli/issues/730)). @@ -24,6 +28,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; 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. `push` stops at its scan for such a manifest (exit 2) even with `--role `, since the scan needs it to tell which namespaces are the member's. - `teamai members list` and `teamai projects members` read the roster registered before the reports switch, so a team upgrading past the orphan-branch split no longer sees "No team members registered" while its `members/` still lives on the default branch. The default-branch copy becomes a read-only inherited root, the way learnings' already was: listed in union with the `teamai-reports` copy, with the branch copy winning when the same file exists on both; nothing is copied or deleted, and a cold `members list` still does not publish the reports branch. Member registration merges against the inherited copy too, so a re-init keeps the original `registeredAt` and projects. Fixes [#735](https://github.com/Tencent/teamai-cli/issues/735). - 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 c893bd46e..25ec648ce 100644 --- a/docs/designs/multi-project-management.md +++ b/docs/designs/multi-project-management.md @@ -81,6 +81,27 @@ projects: agents: [hai-inference] # optional; agents// scoped to this project ``` +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 +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. 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 +(`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. Placement follows it: a new agent pushed with `--role`/`--project` lands under `agents//` (the project's `agents` axis), the same way a new rule resolves from `knowledge` and a new skill from `skills` (issue #649). 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 1a1762d47..7037d3e78 100644 --- a/docs/usage-guide.md +++ b/docs/usage-guide.md @@ -243,6 +243,32 @@ projects: agents: [hai-inference] # optional ``` +The project id and every namespace under `resources:` become a directory name +(`skills//`, `learnings//`, `agents//`), so +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`, `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 `.` +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`, 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. + +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 …`): ```bash @@ -622,7 +648,7 @@ Choose namespace [1-3] (default: 1 = common): - A single namespace is auto-selected; use `--role ` to choose one explicitly - Modifying an existing resource automatically keeps its original namespace - The chosen destination is printed for each resource, e.g. `[rules] my-rule → rules/pm/my-rule.md` -- A roles manifest that exists but cannot answer — unparseable, or missing the configured role — stops the push instead of falling back to the shared root: fix `manifest/roles.yaml`, run `teamai roles set `, or pass `--role `. A team with no `manifest/roles.yaml` at all keeps the pre-manifest behavior +- A roles manifest that exists but cannot answer stops the push instead of falling back to the shared root. One that is missing the configured role: fix `manifest/roles.yaml`, run `teamai roles set `, or pass `--role `. One that cannot be read or parsed, or is empty, stops the push at its scan (exit 2), before `--role` is consulted, because the scan needs the manifest to tell which namespaces are yours: fix `manifest/roles.yaml` first. A team with no `manifest/roles.yaml` at all keeps the pre-manifest behavior - `teamai push --dry-run` resolves the same destinations and stops on the same unresolvable namespace, so it never reports a push as viable that the real command refuses - When several namespaces could take a new resource and there is no terminal to ask on (CI, a hook, `TEAMAI_NONINTERACTIVE`), push stops with exit 2, lists them, and asks for `--role ` - `--role`/`--project` places new resources only. An edit of a shared-root rule or agent stays at the shared root, and push says so @@ -854,7 +880,7 @@ servers: `projects` lists project ids from `manifest/projects.yaml` and follows the same rule on the other axis: a server ships to a directory when one of the projects it is bound to (`teamai projects set`) is listed; `projects: []` ships to nobody; a directory bound to no project receives every server. `teamai projects set` to another project removes the ones that no longer match on the next pull. An id that is not in `projects.yaml` produces one warning per pull, and so does a `projects:` key in a team that has no `projects.yaml` at all, where no id can be checked. -One caveat on the empty list, which applies to `roles: []` just as it always has. "Ships to nobody" holds among members who use that axis. A member who has not configured it at all is unfiltered and still receives the entry, because an unconfigured axis filters nothing. Use `tools: []` or remove the entry if you need it to reach no one at all. +One caveat on the empty list, which applies to `roles: []` just as it always has. "Ships to nobody" holds among members who use that axis. A member who has not configured it at all is unfiltered and still receives the entry, because an unconfigured axis filters nothing. A legacy role that could not be resolved because `manifest/roles.yaml` does not load is not "unconfigured": that member receives no role-scoped entry until the manifest is fixed. Use `tools: []` or remove the entry if you need it to reach no one at all. A missing `projects.yaml` does not switch the key off. A directory's active projects come from its own `config.yaml`, so a directory bound to `billing` still filters out a `projects: [checkout]` server whether or not the manifest is there. What the manifest gives you is the ability to check the ids. @@ -1526,6 +1552,15 @@ roles: agents: [common, frontend] # optional; omitted = root-level agents only ``` +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, 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. + `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 is placed the way a new skill is: `--role ` or `--project ` (that project's `agents` namespace) names the directory, and with neither flag it resolves from the primary role's `agents` namespaces. It only stays at the shared root — where every member receives it — when no namespace resolves, and push warns when that happens (see [Push local resources](#push-local-resources)). 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. A namespaced agent can be named as `/`; a bare name that only one namespace has resolves to it, and a bare name found in several places is refused, with the qualified names listed, rather than removed from all of them. 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. Removing a namespaced agent tombstones `/` only, so the same name in another namespace is untouched; a member's flattened `` copy is cleaned, and not pushed again, when it can be that agent's copy (the namespace is active for them, or their machine placed the agent) and their directory does not still receive an agent of that name from another active namespace. A member who never had that namespace keeps their own agent of the same name. 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 013e756dc..437d03f47 100644 --- a/docs/usage-guide.zh-CN.md +++ b/docs/usage-guide.zh-CN.md @@ -228,6 +228,26 @@ projects: agents: [hai-inference] # 可选 ``` +项目 id 与 `resources:` 下的每个 namespace 都会成为目录名 +（`skills//`、`learnings//`、`agents//`），因此 +都不能越出自己命名的目录。 + +**namespace** 必须是单个路径片段：不含 `/`、`\`、`:` 和控制字符，结尾不能是 `.` +或空格，也不能是 Windows 设备名（`CON`、`NUL`、`AUX`、`PRN`、`CONIN$`、`CONOUT$`、`COM1`–`COM9`、 +`LPT1`–`LPT9`，含 Windows 同样识别为设备编号的上标形式，带不带扩展名都算）。Windows 会从每个路径片段删除结尾的句点与空格， +因此 `.. ` 最终变成 `..` 越出上级目录，`frontend.` 最终变成 `frontend` 落进另一个 +namespace 的目录；该规则同时排除了 `.` 与 `..`。除此之外不受限制 —— 非 ASCII 名称、 +名称中间含空格的目录、以及只是以设备名开头的名称（如 `console`）仍然合法。 +同一资源类型下的两个 namespace 不能仅有大小写差异（如 `frontend` 与 `Frontend`，按 Unicode 大小写折叠 `σ` 与 `ς` 也算）：在 +Windows 与 macOS 的默认文件系统上它们是同一个目录，限定到其中一个的 role 或 project +会读到另一个的资源。该校验跨越两个 manifest，因为 `roles.yaml` 与 `projects.yaml` 共用 +同一套 `skills/`、`knowledge/`、`agents/` 目录。 + +**项目 id** 沿用它原有的、更严格的规则，因为它还会在命令行中输入并按逗号切分： +只允许字母、数字、`.`、`_` 和 `-`，且不能是 `.` 或 `..`。 + +违反任一规则的 manifest 会解析失败，错误信息会指出具体条目。 + **命令**（低频的事后修正与查询，对标 `teamai roles …`）： ```bash @@ -594,7 +614,7 @@ Choose namespace [1-3] (default: 1 = common): - 单一命名空间时自动选中；也可用 `--role ` 显式指定 - 修改已有资源时自动保持原 namespace - 每个资源的落点都会打印出来，例如 `[rules] my-rule → rules/pm/my-rule.md` -- 若 roles manifest 存在却无法解析（格式错误，或未包含当前配置的角色），命令会报错停止，而不会退回共享根目录：请修复 `manifest/roles.yaml`、执行 `teamai roles set `，或用 `--role ` 显式指定。团队仓库根本没有 `manifest/roles.yaml` 时，保持原有行为 +- 若 roles manifest 存在却无法给出答案，命令会报错停止，而不会退回共享根目录。未包含当前配置的角色时：请修复 `manifest/roles.yaml`、执行 `teamai roles set `，或用 `--role ` 显式指定。无法读取、无法解析或为空时，push 在扫描阶段即停止（exit 2），早于 `--role` 生效，因为扫描需要 manifest 才能判断哪些 namespace 属于你：请先修复 `manifest/roles.yaml`。团队仓库根本没有 `manifest/roles.yaml` 时，保持原有行为 - `teamai push --dry-run` 会做同样的落点解析，并在同样的无法解析情况下报错，不会把真实命令会拒绝的推送报为可行 - 当有多个 namespace 可接收新资源、且没有可供询问的终端（CI、hook、`TEAMAI_NONINTERACTIVE`）时，push 会以退出码 2 停止，列出这些 namespace，并要求使用 `--role ` - `--role`/`--project` 只放置新资源。对共享根目录 rule 或 agent 的修改仍留在共享根目录，push 会给出提示 @@ -821,7 +841,7 @@ servers: `projects` 填写 `manifest/projects.yaml` 中的项目 id，在另一个维度上遵循同一条规则：目录通过 `teamai projects set` 绑定的任一项目被列出时才会安装该 server；`projects: []` 对任何人都不安装；未绑定任何项目的目录会收到全部 server。`teamai projects set` 切换到其他项目后，不再匹配的 server 会在下一次 pull 时移除。`projects.yaml` 中不存在的 id 每次 pull 只提示一次；团队根本没有 `projects.yaml` 时同样会提示，因为此时无法校验任何 id。 -空列表有一个需要注意的点，它对 `roles: []` 一直同样适用：“对任何人都不安装”指的是使用了该维度的成员。完全未配置该维度的成员不受过滤，仍会收到该条目。如果需要它对所有人都不生效，请用 `tools: []` 或直接删掉该条目。 +空列表有一个需要注意的点，它对 `roles: []` 一直同样适用：“对任何人都不安装”指的是使用了该维度的成员。完全未配置该维度的成员不受过滤，仍会收到该条目。旧版角色因 `manifest/roles.yaml` 无法加载而未能解析时，不算“未配置”：在 manifest 修复之前，该成员收不到任何按角色限定的条目。如果需要它对所有人都不生效，请用 `tools: []` 或直接删掉该条目。 缺少 `projects.yaml` 并不会关掉这个 key。目录的活动项目来自它自己的 `config.yaml`，所以无论清单是否存在，绑定到 `billing` 的目录依然会过滤掉 `projects: [checkout]` 的 server。清单提供的是校验 id 的能力。 @@ -1480,6 +1500,13 @@ roles: agents: [common, frontend] # 可选；省略 = 只同步根目录 agents ``` +真正生效的 namespace（`knowledge`、`skills`、`agents`）都会成为目录名，因此必须是 +单个路径片段：不含 `/`、`\`、`:` 和控制字符，结尾不能是 `.` 或空格，也不能是 +Windows 设备名，且同一资源类型下的两个 namespace 不能仅有大小写差异；`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 与新 skill 一样需要确定落点：`--role ` 或 `--project `（该项目的 `agents` namespace）指定目录；两者都不给时，从主角色的 `agents` namespace 解析。只有在解析不出任何 namespace 时才留在共享根目录（此时全员都会收到），并且 push 会给出警告（见[推送本地资源](#推送本地资源)）。清理会逐个工具检查 YAML 的 `targets` 和旧格式支持；只有活跃的同名 agent 会写入该工具的同一输出文件时，才保留该文件。`teamai remove agents ` 会记录 tombstone。带 namespace 的 agent 可写作 `/`；只有一个 namespace 拥有的简名会解析到该 agent；若简名出现在多个位置，命令会列出完整名称并拒绝执行，而不是从所有位置删除。其他机器下一次 pull 时，会从每个同步中的工具的 agents 目录删除 `.agent.md`、`.md`、`.toml` 和 `.json`。即使该次 pull 发现团队仓库没有变化，也会执行清理。删除带 namespace 的 agent 只记录 `/` 的 tombstone，其他 namespace 中的同名 agent 不受影响；当该副本可能属于这个 agent（该 namespace 对成员活跃，或由其本机放置）且成员的目录没有从另一个活跃 namespace 收到同名 agent 时，其拍平后的 `` 副本会被清理，也不会再被推送。从未启用该 namespace 的成员会保留自己的同名 agent。CLI 内置的 `teamai-recall` 配置与团队 agents 并列部署，但不会被 `teamai push` 上传。 ### GitHub Copilot CLI diff --git a/skill-data/core/references/troubleshooting.md b/skill-data/core/references/troubleshooting.md index af2ec6ad4..2d413a84a 100644 --- a/skill-data/core/references/troubleshooting.md +++ b/skill-data/core/references/troubleshooting.md @@ -34,6 +34,14 @@ This is the #1 onboarding issue. In order: with `--scope user`. 5. **Tool has no hook surface** (e.g. Gemini CLI, JoyCode): there is no auto-sync; run `teamai pull` manually each time. +6. **A command reports a broken manifest** (`Invalid roles manifest…`, + `Invalid projects manifest…`, `Invalid manifests…`, or `…manifest … could not + be read`). `pull` skips that scope on purpose, since syncing without the + manifest would deliver every namespace it gates; `push` stops before pushing + anything, even with `--role`; `status` lists the other resource types. The fix + belongs in the team repo's `manifest/roles.yaml` or `manifest/projects.yaml`, + which the error names by entry — tell the user to ask a team admin. Do not + delete the manifest or edit the local clone to get past it. ## Permission / access denied diff --git a/skill-data/setup/references/manage-admin.md b/skill-data/setup/references/manage-admin.md index c8fe0ae34..03e933824 100644 --- a/skill-data/setup/references/manage-admin.md +++ b/skill-data/setup/references/manage-admin.md @@ -81,6 +81,15 @@ teamai projects members # who is registered on a project A member gets the union of their role resources and their active project's resources. Admins declare projects in `manifest/projects.yaml`, then `teamai push`. +Every namespace that names a directory — `knowledge`, `skills` and `agents` in +either manifest, and `learnings` in `projects.yaml` (a role's `learnings:` is +ignored and unchecked) — 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, across both +manifests. A manifest that breaks this, does not parse, or is empty stops +members' pull for that scope until it is fixed; the error names the entry. Fix +it rather than deleting it — with no `roles.yaml`, delivery is unfiltered. + ## Team dashboard (web UI) ```bash 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 000000000..d253abd11 --- /dev/null +++ b/src/__tests__/config-legacy-role-migration.test.ts @@ -0,0 +1,84 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { mkdtempSync, mkdirSync, readFileSync, writeFileSync, rmSync } from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { 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', () => { + 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')); + }); + + // 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/__tests__/hooks-shell-check.test.ts b/src/__tests__/hooks-shell-check.test.ts index e1bb18409..f669c120f 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__/init.test.ts b/src/__tests__/init.test.ts index 77719dc7b..fc82d453e 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. + RolesManifestNotFoundError: class RolesManifestNotFoundError extends Error {}, })); // Track pathExists calls to simulate directory states @@ -760,5 +763,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, RolesManifestNotFoundError } = await import('../roles.js'); + vi.mocked(loadRolesManifest).mockRejectedValueOnce( + new RolesManifestNotFoundError('/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__/learnings-namespace.test.ts b/src/__tests__/learnings-namespace.test.ts index 916b221f9..6ae81adc5 100644 --- a/src/__tests__/learnings-namespace.test.ts +++ b/src/__tests__/learnings-namespace.test.ts @@ -52,6 +52,14 @@ describe('buildIndex — learnings namespace isolation', () => { expect(await titles()).toEqual(['billing invoice note', 'hai deploy note', 'shared root learning']); }); + it('indexes a non-ASCII namespace the manifest accepts, and still skips a traversal', async () => { + await fse.ensureDir(path.join(learningsDir, '研发')); + await fse.writeFile(path.join(learningsDir, '研发', 'note.md'), '---\ntitle: rd note\n---\nrd body'); + + await buildIndex({ learningsDir, learningsNamespaces: ['研发', '..'], indexPath }); + expect(await titles()).toEqual(['rd note', 'shared root learning']); + }); + it('root-only (undefined namespaces) matches legacy flat behavior', async () => { await buildIndex({ learningsDir, indexPath }); expect(await titles()).toEqual(['shared root learning']); diff --git a/src/__tests__/projects.test.ts b/src/__tests__/projects.test.ts index 474b40f6f..2acf61209 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 { @@ -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-')); @@ -143,6 +165,189 @@ 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']) { + // '\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', + '.. ', '.. .', '...', '. ', ' ', + // 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', + // 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', + ]) { + 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('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('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, 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', 'COM0', 'LPT0', + ]); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + + 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. + 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" — + // 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 +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__/pull-tombstone.test.ts b/src/__tests__/pull-tombstone.test.ts index cba04722a..1ad9391eb 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. + RolesManifestNotFoundError: class RolesManifestNotFoundError 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, RolesManifestNotFoundError } = await import('../roles.js'); + vi.mocked(loadRolesManifest).mockRejectedValueOnce( + new RolesManifestNotFoundError('/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/__tests__/push-namespace-e2e.test.ts b/src/__tests__/push-namespace-e2e.test.ts index 0811b50d5..e94ab20eb 100644 --- a/src/__tests__/push-namespace-e2e.test.ts +++ b/src/__tests__/push-namespace-e2e.test.ts @@ -842,9 +842,11 @@ describe('push places new rules and agents in a namespace (issue #649)', () => { const result = await runCLI(['push', '--all'], fixture.projectRoot, fixture.home); - // Falling back here would publish the rule to the whole team. + // Falling back here would publish the rule to the whole team. The skills + // scan reads the manifest first, so that is where the push stops. expect(result.code, result.output).toBe(2); - expect(result.output).toContain('Cannot resolve where new rules should go'); + expect(result.output).toContain('Invalid roles manifest YAML'); + expect(result.output).not.toMatch(/^\s+at /m); expect(branchFiles(fixture).branch).toBe(''); }, 60_000); diff --git a/src/__tests__/push-role.test.ts b/src/__tests__/push-role.test.ts index 2b004f883..11b1aa008 100644 --- a/src/__tests__/push-role.test.ts +++ b/src/__tests__/push-role.test.ts @@ -4,6 +4,7 @@ import os from 'node:os'; import path from 'node:path'; import { push } from '../push.js'; import { RolesManifestNotFoundError } from '../roles.js'; +import { log } from '../utils/logger.js'; const mockAutoDetectInit = vi.fn(); const mockPullRepo = vi.fn(); @@ -327,6 +328,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({ @@ -392,6 +414,22 @@ describe('push namespace routing', () => { } }); + it('rejects a role id that cannot be a namespace when roles.yaml is absent', async () => { + mockLoadRolesManifest.mockRejectedValue(new RolesManifestNotFoundError('/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(), @@ -930,6 +968,39 @@ describe('push namespace routing for rules and agents', () => { expect(at('agents')).toBe('agents/fe-agents/vr.yaml'); }); + it('refuses to send a new rule to the shared root when the legacy role could not be resolved', async () => { + const pushedItems: Array> = []; + mockAutoDetectInit.mockResolvedValue({ + localConfig: makeLocalConfig({ primaryRole: undefined, roleUnresolved: true }), + teamConfig: makeTeamConfig(), + }); + mockHandlers({ rules: [{ ...newRule }] }, pushedItems); + + await push({ all: true }); + + expect(process.exitCode).toBe(2); + expect(pushedItems).toHaveLength(0); + const { log } = await import('../utils/logger.js'); + expect(vi.mocked(log.error).mock.calls.flat().join(' ')).toContain('your role could not be resolved'); + }); + + it('reports a projects manifest that cannot be loaded for --project instead of throwing', async () => { + const pushedItems: Array> = []; + mockAutoDetectInit.mockResolvedValue({ + localConfig: makeLocalConfig(), + teamConfig: makeTeamConfig(), + }); + mockLoadProjectsManifest.mockRejectedValueOnce(new Error('Invalid projects manifest: /tmp/team-repo/manifest/projects.yaml is empty.')); + mockHandlers({ rules: [{ ...newRule }] }, pushedItems); + + await expect(push({ all: true, project: 'front-app' })).resolves.toBeUndefined(); + + expect(process.exitCode).toBe(2); + expect(pushedItems).toHaveLength(0); + const { log } = await import('../utils/logger.js'); + expect(vi.mocked(log.error).mock.calls.flat().join(' ')).toContain('Cannot resolve --project destinations'); + }); + it('refuses to push to the shared root when the project declares no namespace for the type', async () => { const pushedItems: Array> = []; mockAutoDetectInit.mockResolvedValue({ 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 000000000..65867eed4 --- /dev/null +++ b/src/__tests__/resource-namespaces-case-alias.test.ts @@ -0,0 +1,113 @@ +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, overrides: Partial = {}): LocalConfig { + return { + repo: { localPath: repoDir, remote: 'https://github.com/acme/team.git' }, + username: 'e2e', + primaryRole: 'fe', + projects: ['p'], + ...overrides, + } 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('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('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 }); + } + }); + + // 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')); + 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 { + 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 afac3a57f..ee59eec50 100644 --- a/src/__tests__/roles.test.ts +++ b/src/__tests__/roles.test.ts @@ -155,6 +155,80 @@ 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', 'C:evil', + 'a\u0009b', 'a\u007fb', 'a\u0085b', + '.. ', '.. .', '...', '. ', ' ', + 'frontend.', 'frontend ', 'frontend..', + 'CON', 'nul', 'COM1', 'CON.txt', 'CONIN$', 'CONOUT$.txt', + ]) { + 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('quotes the offending namespace, so the admin can find the text to fix', async () => { + const repoDir = writeManifest(`version: 1 +roles: + - id: hai + resources: + knowledge: [] + skills: ['../evil'] +`); + + try { + await expect(loadRolesManifest(repoDir)).rejects.toThrow(/; got "\.\.\/evil"/); + } finally { + rmSync(repoDir, { recursive: true, force: true }); + } + }); + + it('reports an empty manifest as broken rather than missing', async () => { + // Only ENOENT may become RolesManifestNotFoundError: 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(RolesManifestNotFoundError); + 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(RolesManifestNotFoundError); + rmSync(repoDir, { recursive: true, force: true }); + }); + + 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 @@ -174,6 +248,94 @@ 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 }); + } + }); + + // 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 +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-')); + 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(RolesManifestNotFoundError); + } 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 75b5ec9ad..007dff5c2 100644 --- a/src/__tests__/skills.test.ts +++ b/src/__tests__/skills.test.ts @@ -353,6 +353,37 @@ scope: 'user', expect(items.find((item) => item.name === 'role-skill')?.status).toBe('modified'); }); + it('keeps the role-id fallback when a valid roles.yaml no longer lists the role', async () => { + localConfig.primaryRole = 'hai'; + localConfig.additionalRoles = []; + await fse.outputFile( + path.join(localConfig.repo.localPath, 'manifest', 'roles.yaml'), + 'version: 1\nroles:\n - id: pm\n resources:\n knowledge: []\n skills: [pm]\n', + ); + await fse.outputFile(path.join(localConfig.repo.localPath, 'skills', 'hai', 'role-skill', 'SKILL.md'), '# v1'); + await fse.outputFile(path.join(homeDir, '.claude/skills', 'role-skill', 'SKILL.md'), '# v2'); + + const items = await handler.scanLocalForPush(teamConfig, localConfig); + expect(items.find((item) => item.name === 'role-skill')?.status).toBe('modified'); + }); + + it('stops the scan when roles.yaml exists but does not parse, instead of guessing', async () => { + localConfig.primaryRole = 'hai'; + localConfig.additionalRoles = []; + await fse.outputFile(path.join(localConfig.repo.localPath, 'manifest', 'roles.yaml'), 'version: 1\nroles: [\n'); + + await expect(handler.scanLocalForPush(teamConfig, localConfig)).rejects.toThrow(/Invalid roles manifest YAML/); + }); + + 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/__tests__/status-broken-manifest.test.ts b/src/__tests__/status-broken-manifest.test.ts new file mode 100644 index 000000000..de0b8da88 --- /dev/null +++ b/src/__tests__/status-broken-manifest.test.ts @@ -0,0 +1,68 @@ +import { afterAll, afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { rmSync } from 'node:fs'; + +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-')), + rules: { pending: [{ name: 'r' }] as Array<{ name: string }> }, + }; +}); + +vi.mock('../config.js', async (importOriginal) => ({ + ...(await importOriginal()), + autoDetectInit: vi.fn(async () => ({ + localConfig: { repo: { localPath: repoDir, remote: 'https://github.com/acme/team.git' }, username: 'dev', scope: 'user' }, + teamConfig: {}, + })), + loadStateForScope: vi.fn(async () => ({})), +})); +vi.mock('../utils/git.js', () => ({ getRepoStatus: vi.fn(async () => ({ ahead: 0, behind: 0, modified: [] })) })); +vi.mock('../resources/index.js', () => ({ + getAllHandlers: vi.fn(() => [ + { + type: 'skills', + scanLocalForPush: vi.fn(async () => { throw new Error('Invalid roles manifest: roles.0.resources.skills.0: bad'); }), + }, + { type: 'rules', scanLocalForPush: vi.fn(async () => rules.pending) }, + ]), +})); + +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 = []; + rules.pending = [{ name: 'r' }]; + 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'); + }); + + 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/__tests__/types.test.ts b/src/__tests__/types.test.ts index 9f73f8f09..093f9aa2f 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/bootstrap.ts b/src/bootstrap.ts index 90c6b7b5c..24e2ba065 100644 --- a/src/bootstrap.ts +++ b/src/bootstrap.ts @@ -173,8 +173,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 { RolesManifestNotFoundError } = await import('./roles.js'); + if (!(error instanceof RolesManifestNotFoundError)) throw error; } await ensureDir(localPath); diff --git a/src/config.ts b/src/config.ts index ad03deef3..de838a811 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, RolesManifestNotFoundError } from './roles.js'; async function migrateLegacyRoleConfig(config: LocalConfig, configPath: string): Promise { if (config.primaryRole) { @@ -29,8 +29,16 @@ async function migrateLegacyRoleConfig(config: LocalConfig, configPath: string): let manifest; try { manifest = await loadRolesManifest(config.repo.localPath); - } catch { - return config; + } 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. 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 RolesManifestNotFoundError) 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'); @@ -90,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/contribute.ts b/src/contribute.ts index c3ea10dea..53a458180 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/init.ts b/src/init.ts index e2e0efe1a..e11d01297 100644 --- a/src/init.ts +++ b/src/init.ts @@ -19,7 +19,7 @@ import { getConfigPath, } from './types.js'; import { getUserHome } from './utils/home.js'; -import { describeRoles, listRoleIds, loadRolesManifest } from './roles.js'; +import { describeRoles, listRoleIds, loadRolesManifest, RolesManifestNotFoundError } from './roles.js'; import { loadProjectsManifest, listProjectIds } from './projects.js'; import { memberReadRoots, readMemberConfig, mergeMemberConfig } from './members.js'; import { askQuestion, askConfirmation, askSelection, closePrompt, isInteractive } from './utils/prompt.js'; @@ -61,6 +61,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, @@ -107,7 +114,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]; @@ -432,10 +439,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 RolesManifestNotFoundError || error instanceof NoRoleSelectedError; + if (!lenient) throw error; } Object.assign(localConfig, await resolveActiveProjects(localPath, options.project)); @@ -875,10 +885,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 RolesManifestNotFoundError || 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 new file mode 100644 index 000000000..b97133241 --- /dev/null +++ b/src/manifest-schema.ts @@ -0,0 +1,199 @@ +import fs from 'node:fs/promises'; +import path from 'node:path'; +import { expandHome } from './utils/fs.js'; +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`. +// 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]/; + +// 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 = /[ .]$/; + +// 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`, 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|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 { + return seg.length > 0 + && !UNSAFE_SEGMENT.test(seg) + && !TRAILING_DOT_OR_SPACE.test(seg) + && !WINDOWS_DEVICE_NAME.test(seg); +} + +export 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. The + * message quotes the value (JSON-escaped, so a control character shows): the + * issue path names the entry, but an admin fixing it looks for the text. + */ +export const NamespaceSegmentSchema = z.string().min(1).refine(isSafeNamespaceSegment, (value) => ({ + message: `${NAMESPACE_RULE}; got ${JSON.stringify(value)}`, +})); + +/** + * 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 error = fallbackNamespaceError(ids, source); + if (error !== null) throw new Error(error); + return ids; +} + +/** + * The error `assertSafeFallbackNamespaces` would throw, or null when every id is + * safe, for a caller that reports failures as values. + */ +export function fallbackNamespaceError(ids: string[], source: string): string | null { + const unsafe = ids.find((id) => !isSafeNamespaceSegment(id)); + return unsafe === undefined + ? null + : `Invalid ${source} "${unsafe}": ${NAMESPACE_RULE}. Switch to a role whose id is a valid namespace with \`teamai roles set \`, or add manifest/roles.yaml to map the role to its namespaces.`; +} + + +/** + * 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}`); +} + +/** + * 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 + * 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 { + // `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(resolvedPath, 'utf-8'); + } catch (error) { + if ((error as NodeJS.ErrnoException).code === 'ENOENT') { + const dangling = await danglingLinkOnPath(resolvedPath); + if (!dangling) return null; + throw new Error(`The ${kind} manifest ${resolvedPath} could not be read: ${dangling} is a symbolic link with no target. Point the link at the manifest file, or replace the link with the file itself.`); + } + throw new Error(`The ${kind} manifest ${resolvedPath} could not be read: ${(error as Error).message}. Make it a readable file, then retry.`); + } + if (content.trim() === '') { + throw new Error(`Invalid ${kind} manifest: ${resolvedPath} is empty. Give it a version and a ${kind} list; deleting it instead turns ${kind} filtering off for the whole team.`); + } + return content; +} + +/** One namespace as a manifest declares it, with the entry that declares it. */ +export interface NamespaceEntry { + type: string; + namespace: string; + 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 + * 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}/${caseFoldKey(entry.namespace)}`; + 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. ' + + 'Rename one of them, together with its directory in the team repo', + ); + } + } +} diff --git a/src/membership.ts b/src/membership.ts index ab79037ae..f3697528f 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/projects.ts b/src/projects.ts index 6506b6070..56995efdf 100644 --- a/src/projects.ts +++ b/src/projects.ts @@ -1,7 +1,8 @@ import path from 'node:path'; import YAML from 'yaml'; import { z } from 'zod'; -import { readFileIfExists, ensureDir, writeFile } from './utils/fs.js'; +import { ensureDir, writeFile } from './utils/fs.js'; +import { NamespaceSegmentSchema, parseManifest, readManifestFile, assertNoCaseAliasedNamespaces, type NamespaceEntry } from './manifest-schema.js'; import type { ResourceNamespaces } from './roles.js'; /** @@ -13,28 +14,36 @@ 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 becomes a path component (`skills//`, `learnings//`) just + * 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` + * 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 !== '..'; } +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), { + id: z.string().min(1).refine(isSafeProjectId, { message: "project id must be a single path segment (letters, digits, '.', '_', '-'; no '/', '\\\\', or '..')", }), name: z.string().default(''), @@ -83,7 +92,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)) { @@ -91,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 @@ -104,7 +123,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/push-namespaces.ts b/src/push-namespaces.ts index 660a25dcd..470a54a80 100644 --- a/src/push-namespaces.ts +++ b/src/push-namespaces.ts @@ -12,8 +12,9 @@ * reached the whole team (issue #649). It lives here so every type answers the * same question the same way, and so the answer can be tested without a repo. */ +import { isSafeNamespaceSegment, NAMESPACE_RULE } from './manifest-schema.js'; import { - isSafeNamespaceSegment, findProject, unknownProjectMessage, + findProject, unknownProjectMessage, type ProjectResourceType, type ProjectsManifest, } from './projects.js'; import type { ResourceItem, ResourceType } from './types.js'; @@ -116,8 +117,7 @@ export function resolveProjectNamespace( if (!isSafeNamespaceSegment(namespace)) { return { ok: false, - message: `Project "${projectId}" declares an unusable ${axis} namespace "${namespace}": ` - + "it must be a single path segment (letters, digits, '.', '_', '-'; no '/', '\\', or '..').", + message: `Project "${projectId}" declares an unusable ${axis} namespace "${namespace}": ${NAMESPACE_RULE}.`, }; } diff --git a/src/push.ts b/src/push.ts index e7e621485..88628e712 100644 --- a/src/push.ts +++ b/src/push.ts @@ -23,7 +23,7 @@ import { acquireLock, releaseLock } from './update.js'; import { assertSafePath, assertSafeResourceName, defaultAllowedRoots } from './utils/path-safety.js'; import { loadRolesManifest, resolveRoleResourceNamespaces, RolesManifestNotFoundError } from './roles.js'; import type { ProjectsManifest } from './projects.js'; -import { isSafeNamespaceSegment } from './projects.js'; +import { isSafeNamespaceSegment, NAMESPACE_RULE, fallbackNamespaceError } from './manifest-schema.js'; import { isAtSharedRoot, isPlaceableType, NAMESPACE_AXIS, PLACEABLE_TYPES, resolveProjectNamespace, skillNamespacePath, withNamespace, type PlaceableType, @@ -80,6 +80,16 @@ async function namespaceCandidates( type: PlaceableType, localConfig: LocalConfig, ): Promise { + // A legacy role the manifest could not resolve is not "no role": treating it + // as one would send a new rule or agent to the shared root. + if (localConfig.roleUnresolved) { + return { + ok: false, + message: `Cannot resolve where new ${type} should go: your role could not be resolved because ` + + 'manifest/roles.yaml could not be loaded. Fix manifest/roles.yaml, or pass --role ' + + 'to name the namespace for this push.', + }; + } if (localConfig.primaryRole) { try { const manifest = await loadRolesManifest(localConfig.repo.localPath); @@ -98,8 +108,7 @@ async function namespaceCandidates( if (unsafe !== undefined) { return { ok: false, - message: `The roles manifest declares an unusable ${axis} namespace "${unsafe}": ` - + "it must be a single path segment (letters, digits, '.', '_', '-'; no '/', '\\', or '..'). " + message: `The roles manifest declares an unusable ${axis} namespace "${unsafe}": ${NAMESPACE_RULE}. ` + 'Fix manifest/roles.yaml, or pass --role to name the namespace for this push.', }; } @@ -114,8 +123,11 @@ async function namespaceCandidates( }; } // Legacy fallback: with no manifest at all a role id doubles as its - // skills namespace. That convention only ever existed for skills. - return { ok: true, candidates: type === 'skills' ? [localConfig.primaryRole] : [] }; + // skills namespace. That convention only ever existed for skills. No + // manifest validated the id as a namespace, so it is checked here. + if (type !== 'skills') return { ok: true, candidates: [] }; + const unsafe = fallbackNamespaceError([localConfig.primaryRole], 'role id used as a skills namespace'); + return unsafe === null ? { ok: true, candidates: [localConfig.primaryRole] } : { ok: false, message: unsafe }; } } @@ -179,6 +191,9 @@ async function resolveNamespaceForNew( // Skills keep their historical silent default (the primary role id); no // other axis ever had that convention, so they take the first candidate. const skillsDefault = type === 'skills' ? localConfig.primaryRole : undefined; + // The role id is not one of the candidates the manifest validated. + const unsafe = skillsDefault === undefined ? null : fallbackNamespaceError([skillsDefault], 'role id used as a skills namespace'); + if (unsafe !== null) return { kind: 'unresolvable', message: unsafe }; return { kind: 'namespace', namespace: skillsDefault ?? candidates[0] }; } // No terminal to ask on (CI, a hook, TEAMAI_NONINTERACTIVE): say what the @@ -834,7 +849,13 @@ async function pushCore( return; } const { loadProjectsManifest, findProject, unknownProjectMessage } = await import('./projects.js'); - projectsManifest = await loadProjectsManifest(localConfig.repo.localPath); + try { + projectsManifest = await loadProjectsManifest(localConfig.repo.localPath); + } catch (e) { + log.error(`Cannot resolve --project destinations: ${(e as Error).message}`); + process.exitCode = 2; + return; + } if (!projectsManifest) { log.error('This team repo defines no projects (no manifest/projects.yaml).'); process.exitCode = 2; @@ -938,12 +959,29 @@ async function pushCore( for (const type of pushableTypes) { const handler = getHandler(type); - const items = await handler.scanLocalForPush( - scanTeamConfig, - localConfig, - type === 'agents' ? { namespace: requestedAgentsNamespace } : undefined, - ); - fullScan.push(...items); + try { + const items = await handler.scanLocalForPush( + scanTeamConfig, + localConfig, + type === 'agents' ? { namespace: requestedAgentsNamespace } : undefined, + ); + fullScan.push(...items); + } catch (e) { + // The skills and agents scans read the roles manifest to learn this + // member's namespaces, and one that cannot be read or parsed fails them + // rather than guessing — `--role` cannot stand in, since the scan needs + // the manifest to tell which namespaces are the member's. Report that + // before anything is pushed instead of an uncaught stack trace. + spin.stop(); + const error = e as Error; + log.debug(error.stack ?? error.message); + log.error( + `Could not scan local ${type}: ${error.message.replace(/\.$/, '')}. Nothing was pushed. ` + + 'Fix the file the error names, then retry.', + ); + process.exitCode = 2; + return; + } } // A project that cannot answer for agents is reported below for an agent the diff --git a/src/resource-namespaces.ts b/src/resource-namespaces.ts index a4dccd8c8..45a513d17 100644 --- a/src/resource-namespaces.ts +++ b/src/resource-namespaces.ts @@ -1,6 +1,14 @@ import type { LocalConfig } from './types.js'; -import { loadRolesManifest, resolveRoleResourceNamespaces, type ResourceNamespaces } from './roles.js'; -import { loadProjectsManifest, resolveProjectResourceNamespaces, mergeNamespaces } from './projects.js'; +import { + loadRolesManifest, + resolveRoleResourceNamespaces, + roleNamespaceEntries, + RolesManifestNotFoundError, + type ResourceNamespaces, + type RolesManifest, +} from './roles.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. */ @@ -15,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 RolesManifestNotFoundError)) 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). // @@ -30,14 +57,16 @@ export async function resolveResourceNamespaces(localConfig: LocalConfig) { // ── Role namespaces (optional) ── let roleNamespaces: ResourceNamespaces = { knowledge: [], skills: [], learnings: [], agents: [] }; let allRoleSkillNamespaces = new Set(); + 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) { - let rolesManifest; - try { - rolesManifest = await loadRolesManifest(localConfig.repo.localPath); - } catch { - log.warn('Could not load roles manifest. Skipping role-based filtering.'); - rolesManifest = null; - } if (rolesManifest) { try { roleNamespaces = resolveRoleResourceNamespaces({ diff --git a/src/resources/agents.ts b/src/resources/agents.ts index c551e0832..346a4bdf8 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 { loadStateForScope } from '../config.js'; import { placedResourcePath } from '../push-namespaces.js'; diff --git a/src/resources/skills.ts b/src/resources/skills.ts index 8fcf85935..16f6469c6 100644 --- a/src/resources/skills.ts +++ b/src/resources/skills.ts @@ -8,7 +8,10 @@ import { log } from '../utils/logger.js'; import { isCliOwnedSkillName } 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, RolesManifestNotFoundError, type RolesManifest, +} from '../roles.js'; +import { assertSafeFallbackNamespaces } from '../manifest-schema.js'; import { assertWithinRoot } from '../utils/path-safety.js'; import { splitFrontmatter, stringifyFrontmatter } from '../utils/frontmatter.js'; @@ -257,23 +260,35 @@ async function readPushIgnoredSkills(): Promise> { /** * Resolve skill namespaces from the manifest using the user's configured roles. - * Falls back to [primaryRole, ...additionalRoles] if manifest is unavailable, - * and returns [] if no roles are configured. + * Falls back to [primaryRole, ...additionalRoles] when the manifest is absent or + * does not list the role, returns [] if no roles are configured, and throws when + * the manifest exists but cannot be read or parsed. */ async function resolveSkillNamespaces(localConfig: LocalConfig): Promise { if (!localConfig.primaryRole) return []; + const roleIds = [localConfig.primaryRole, ...(localConfig.additionalRoles ?? [])]; + let manifest: RolesManifest; try { - const manifest = await loadRolesManifest(localConfig.repo.localPath); - const namespaces = resolveRoleResourceNamespaces({ + manifest = await loadRolesManifest(localConfig.repo.localPath); + } catch (error) { + // Fallback: use role ids as namespace names (legacy behavior). Reserved for a + // manifest that is not there — one that exists and does not parse must not be + // silently replaced by a guess at its contents. + if (!(error instanceof RolesManifestNotFoundError)) throw error; + return assertSafeFallbackNamespaces(roleIds, 'role id used as a skills namespace'); + } + + try { + return resolveRoleResourceNamespaces({ manifest, primaryRole: localConfig.primaryRole, additionalRoles: localConfig.additionalRoles ?? [], - }); - return namespaces.skills; + }).skills; } catch { - // Fallback: use role ids as namespace names (legacy behavior) - return [localConfig.primaryRole, ...(localConfig.additionalRoles ?? [])]; + // A valid manifest that no longer lists the role (renamed or removed) keeps + // the legacy guess it always had; push placement still refuses to guess. + return assertSafeFallbackNamespaces(roleIds, 'role id used as a skills namespace'); } } diff --git a/src/roles.ts b/src/roles.ts index d7406e7e0..50ceff98a 100644 --- a/src/roles.ts +++ b/src/roles.ts @@ -1,20 +1,23 @@ import path from 'node:path'; import YAML from 'yaml'; import { z } from 'zod'; -import { readFileSafe, readFileIfExists, ensureDir, pathExists, writeFile } from './utils/fs.js'; +import { ensureDir, writeFile } from './utils/fs.js'; +import { NamespaceSegmentSchema, parseManifest, readManifestFile, assertNoCaseAliasedNamespaces, type NamespaceEntry } from './manifest-schema.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). + // 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(), }); @@ -79,7 +82,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)) { @@ -87,16 +90,26 @@ 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 team repo has no `manifest/roles.yaml` at all — a distinct case from one - * that exists but cannot be parsed. `push` treats them differently: an absent - * manifest is the pre-manifest layout, where a role id doubles as its skills - * namespace, while an unreadable one is a failure that must stop the push - * rather than let a new rule or agent fall back to the shared root (#649). + * The team repo has no `manifest/roles.yaml` at all — a team that does not use + * roles, not a broken one. Callers that relax filtering must react to this case + * ONLY: doing the same for a manifest that exists but cannot be read or parsed + * would hand out every namespace the manifest was written to gate on pull, and + * send a new rule or agent to the shared root on push (#649). */ export class RolesManifestNotFoundError extends Error { constructor(manifestPath: string) { @@ -107,17 +120,11 @@ export class RolesManifestNotFoundError extends Error { export async function loadRolesManifest(repoPath: string): Promise { const manifestPath = path.join(repoPath, 'manifest', 'roles.yaml'); - const content = await readFileSafe(manifestPath); + // Absence is the only case that may relax filtering downstream, so it is the + // only one that becomes RolesManifestNotFoundError: an unreadable or empty file + // throws a plain error and fails the pull or push. + const content = await readManifestFile(manifestPath, 'roles'); if (content === null) { - // `readFileSafe` answers null for EVERY failure, so "no such file" and - // "cannot read it" arrive identically. Only the first is the pre-manifest - // layout; treating a permission error as that one sends new rules and - // agents to the shared root, which is the whole team (#649). - if (await pathExists(manifestPath)) { - throw new Error( - `Roles manifest exists but could not be read: ${manifestPath}. Check its permissions.`, - ); - } throw new RolesManifestNotFoundError(manifestPath); } @@ -140,11 +147,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 RolesManifestNotFoundError) return null; + throw error; + } } export async function saveRolesManifest(repoPath: string, manifest: RolesManifest): Promise { @@ -219,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/status.ts b/src/status.ts index 1ddaa4057..a030cf7e9 100644 --- a/src/status.ts +++ b/src/status.ts @@ -125,8 +125,18 @@ 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()) { - 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}`); + anyUnscanned = true; + continue; + } if (items.length > 0) { anyNew = true; console.log(` [${handler.type}] ${items.length} new`); @@ -138,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(''); diff --git a/src/types.ts b/src/types.ts index 126f340c6..cce8db937 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'; @@ -508,7 +508,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. @@ -593,8 +595,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) ──────────────────── diff --git a/src/utils/fs.ts b/src/utils/fs.ts index 1f6ee5652..77877dc4f 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 cf4a0340a..fc5aed6a1 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; +} diff --git a/src/utils/search-index.ts b/src/utils/search-index.ts index 46a6af272..fdc273e57 100644 --- a/src/utils/search-index.ts +++ b/src/utils/search-index.ts @@ -14,6 +14,7 @@ import { type KnowledgeType, } from '../types.js'; import { getUserHome } from './home.js'; +import { isSafeNamespaceSegment } from '../manifest-schema.js'; /** Resolve search index path dynamically (respects HOME changes in tests). */ function getSearchIndexPath(): string { @@ -493,9 +494,10 @@ async function collectLearningsEntriesFromDir( for (const ns of namespaces ?? []) { // Defense-in-depth: a namespace is a path segment (learnings//). Skip // anything that isn't a safe single segment so a hand-edited config can't - // make the index scan outside the learnings directory. (Inlined rather than - // importing from ../projects.js to keep this low-level util dependency-free.) - if (!/^[A-Za-z0-9._-]+$/.test(ns) || ns === '.' || ns === '..') continue; + // make the index scan outside the learnings directory. The rule must be the + // one `contribute` writes with, or a learning filed under a valid non-ASCII + // namespace would never be indexed. + if (!isSafeNamespaceSegment(ns)) continue; const nsDir = path.join(dir, ns); if (!await pathExists(nsDir)) continue; const files = await listFilesRecursive(nsDir);