From 4da678f300eceec1fe08bdb74dce72e865ed3803 Mon Sep 17 00:00:00 2001 From: bo Date: Sat, 8 Aug 2026 23:57:25 +0800 Subject: [PATCH 1/2] feat(skills): adopt multi-file skill packages Replace the legacy single-string Skill model with strict package discovery, bounded resources, progressive disclosure, compiled builtin manifests, and richer workflow guidance. BREAKING CHANGE: project and user Skills must use /SKILL.md packages with the standard five-field frontmatter; when_to_use, allowed_tools, and legacy parsing are removed. --- AGENTS.md | 5 + CHANGELOG.md | 18 + README.md | 4 +- bun.lock | 3 + docs/agents/multi-agent-design.md | 15 + docs/concepts.md | 14 +- .../goals/skill-package-hard-cut-plan-goal.md | 222 ++++++++++ docs/goals/skill-package-hard-cut-progress.md | 139 ++++++ packages/agent-core/package.json | 1 + .../src/agents/configured-agent.test.ts | 57 +-- .../agent-core/src/agents/configured-agent.ts | 2 +- .../agents/definitions/definitions.test.ts | 4 +- .../agent-core/src/agents/factory.test.ts | 4 +- packages/agent-core/src/agents/factory.ts | 2 +- .../src/agents/session-agent-manager.test.ts | 11 +- .../agent-core/src/commands/skill.test.ts | 5 +- packages/agent-core/src/commands/skill.ts | 2 +- .../agent-core/src/prompt/compiler.test.ts | 45 ++ packages/agent-core/src/prompt/compiler.ts | 28 +- .../builtin-standalone.integration.test.ts | 168 +++++++ .../src/skills/builtin/analyze-work/SKILL.md | 93 +++- .../references/diagnosis-method.md | 35 ++ .../skills/builtin/automation-create/SKILL.md | 52 ++- .../references/schedule-examples.md | 27 ++ .../src/skills/builtin/codemap/SKILL.md | 54 ++- .../references/evidence-map-example.md | 40 ++ .../src/skills/builtin/execute-plan/SKILL.md | 45 +- .../references/execution-checkpoints.md | 24 + .../src/skills/builtin/git-master/SKILL.md | 85 +++- .../git-master/references/operation-safety.md | 22 + .../src/skills/builtin/goal-review/SKILL.md | 123 +++++- .../references/evidence-matrix-example.md | 22 + .../src/skills/builtin/manifest.test.ts | 81 ++++ .../agent-core/src/skills/builtin/manifest.ts | 99 +++-- .../skills/builtin/orchestrate-work/SKILL.md | 52 ++- .../references/delegation-packet.md | 29 ++ .../src/skills/builtin/plan-work/SKILL.md | 67 ++- .../builtin/plan-work/assets/plan-template.md | 55 +++ .../src/skills/builtin/research-docs/SKILL.md | 55 ++- .../references/source-evaluation.md | 34 ++ .../src/skills/builtin/review-change/SKILL.md | 105 ++++- .../review-change/references/review-lenses.md | 36 ++ .../src/skills/builtin/review-work/SKILL.md | 96 +++- .../review-work/references/review-packet.md | 38 ++ .../src/skills/builtin/run-goal/SKILL.md | 52 ++- .../src/skills/builtin/safe-refactor/SKILL.md | 70 ++- .../references/boundary-verification.md | 27 ++ .../src/skills/builtin/shape-todo/SKILL.md | 89 +++- .../references/todo-shaping-template.md | 37 ++ .../src/skills/package-reader.test.ts | 393 ++++++++++++++++ .../agent-core/src/skills/package-reader.ts | 358 +++++++++++++++ packages/agent-core/src/skills/schema.test.ts | 229 ++++++++-- packages/agent-core/src/skills/schema.ts | 159 +++++-- .../agent-core/src/skills/service.test.ts | 418 ++++++++++++++---- packages/agent-core/src/skills/service.ts | 287 +++++++----- packages/agent-core/src/skills/types.ts | 48 +- .../builtins/model-visible-contract.test.ts | 5 +- .../src/tools/builtins/skill-list.test.ts | 2 +- .../src/tools/builtins/skill-list.ts | 2 +- .../src/tools/builtins/skill-read.test.ts | 171 +++++-- .../src/tools/builtins/skill-read.ts | 112 ++++- 61 files changed, 3992 insertions(+), 585 deletions(-) create mode 100644 docs/goals/skill-package-hard-cut-plan-goal.md create mode 100644 docs/goals/skill-package-hard-cut-progress.md create mode 100644 packages/agent-core/src/skills/builtin-standalone.integration.test.ts create mode 100644 packages/agent-core/src/skills/builtin/analyze-work/references/diagnosis-method.md create mode 100644 packages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.md create mode 100644 packages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.md create mode 100644 packages/agent-core/src/skills/builtin/execute-plan/references/execution-checkpoints.md create mode 100644 packages/agent-core/src/skills/builtin/git-master/references/operation-safety.md create mode 100644 packages/agent-core/src/skills/builtin/goal-review/references/evidence-matrix-example.md create mode 100644 packages/agent-core/src/skills/builtin/manifest.test.ts create mode 100644 packages/agent-core/src/skills/builtin/orchestrate-work/references/delegation-packet.md create mode 100644 packages/agent-core/src/skills/builtin/plan-work/assets/plan-template.md create mode 100644 packages/agent-core/src/skills/builtin/research-docs/references/source-evaluation.md create mode 100644 packages/agent-core/src/skills/builtin/review-change/references/review-lenses.md create mode 100644 packages/agent-core/src/skills/builtin/review-work/references/review-packet.md create mode 100644 packages/agent-core/src/skills/builtin/safe-refactor/references/boundary-verification.md create mode 100644 packages/agent-core/src/skills/builtin/shape-todo/references/todo-shaping-template.md create mode 100644 packages/agent-core/src/skills/package-reader.test.ts create mode 100644 packages/agent-core/src/skills/package-reader.ts diff --git a/AGENTS.md b/AGENTS.md index 813e56a0..9940bf17 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -198,6 +198,7 @@ packages/agent-core/src/ ├── automations/ # Canonical Automation schemas, schedule, durable Invocation, Session dispatch ├── todos/ # ProjectTodo schema, serialized state, and narrow Session-entry coordination ├── lsp/ # LspClientPool (acquire/release, idle timeout, crash detection), StdioLspTransport, auto-installer, 18 language servers, 50+ ext mappings +├── skills/ # Standard local Skill packages: schema, package reader, source resolver, embedded builtin manifest ├── llm/ # Managed LLM runtime: runLlmStream/runLlmText/runLlmObject, retry/recovery, adapter test seam ├── projects/ # ProjectRegistry + per-workspace HITL/memory/approval context resolver ├── prompt/ # PromptContractCompiler V2: typed kernel/runtime/role/collaboration/context/overlay layers + trace/eval @@ -396,6 +397,10 @@ All six implement `Agent`: `store: StoreApi`, `run(options) - Ordinary root Lead activates `orchestrate-work`; active Goal activates `run-goal`; root Discussion activates `shape-todo`, derived from authoritative runtime facts on every Execution. - `plan-work` writes one ordinary Markdown Plan per Todo under `.archcode/plans/`. Plan has no service, state, ID, API, dedicated page, or Goal link. `execute-plan` is activated only by the Todo-to-work handoff when that file exists. - `review-work` guides Lead review orchestration. Analyst analysis/review Skills include `analyze-work`, `review-change`, and the reserved `goal-review` final gate. +- A Skill is one package: required `SKILL.md`; optional `scripts/`, `references/`, `assets/`, and other contained resources. Its strict YAML frontmatter accepts `name`, `description`, optional `license`, `compatibility`, and `metadata`; `description` states both method and activation timing. +- Discovery (`skill_list` and available Prompt metadata) returns exactly name, description, and source. Entry activation (`skill_read({ name })`) returns the entry plus sorted resource descriptors; `skill_read({ name, resource })` reads exactly one listed text resource on demand. Binary assets are valid package resources but are not returned by the text-only tool. +- Project `.archcode/skills//` > user `~/.archcode/skills//` > embedded builtin is whole-package precedence: bodies and resources never merge or fall through. Reserved lifecycle builtins remain unshadowable and Agent-gated. +- Skills remain guidance only: their package metadata and resources cannot grant tools or permissions, execute scripts automatically, change Agent/Profile/MCP/workspace scope/delegation, or grant completion authority. Scripts use only existing Bash permissions. **MCP visibility by agent:** diff --git a/CHANGELOG.md b/CHANGELOG.md index 85d726b9..b491c753 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,24 @@ and this project uses [Semantic Versioning](https://semver.org/spec/v2.0.0.html) ## [Unreleased] +### Changed + +- Replace single-file workflow Skills with standard local Skill packages: + `SKILL.md` plus optional `scripts/`, `references/`, `assets/`, and other + contained resources. Skill discovery is metadata-only; entry and listed + resources are disclosed progressively. + +### Breaking Changes + +- Project and user Skills must use one directory per Skill: + `//SKILL.md`. Manually update existing entries to use only `name` and `description` + (required), with optional `license`, `compatibility`, and string-map + `metadata`. Move former activation guidance into `description`; move + supporting files beneath the package and reference them with package-relative + paths. `when_to_use`, `allowed_tools`, and all other + top-level frontmatter fields are rejected. There is no migration, fallback, + compatibility reader, or resource merge with lower-precedence packages. + ## [0.0.8] - 2026-08-04 ### Added diff --git a/README.md b/README.md index e4bdd4aa..e1c2b227 100644 --- a/README.md +++ b/README.md @@ -142,8 +142,8 @@ workbench. ## Built in - Structured file, shell, Git, search, LSP, Web, memory, and MCP tools -- Built-in and project workflow Skills for Todo shaping, planning, review, and - repeatable working methods +- Built-in and project workflow Skill packages for Todo shaping, planning, + review, and repeatable working methods - Project memory and context compaction - Optional Git worktree execution - GitHub and custom MCP integrations diff --git a/bun.lock b/bun.lock index d730c57f..25c9b3e2 100644 --- a/bun.lock +++ b/bun.lock @@ -125,6 +125,7 @@ "vscode-jsonrpc": "^8.2.1", "vscode-languageserver-protocol": "^3.17.5", "vscode-languageserver-types": "^3.17.5", + "yaml": "^2.8.1", "zod": "^4.4.2", "zustand": "^5.0.13", }, @@ -1473,6 +1474,8 @@ "yallist": ["yallist@3.1.1", "", {}, "sha512-a4UGQaWPH59mOXUYnAG2ewncQS4i4F43Tv3JoAM+s2VDAmS9NsK8GpDMLrCHPksFT7h3K6TOoUNn2pb7RoXx4g=="], + "yaml": ["yaml@2.9.0", "", { "bin": { "yaml": "bin.mjs" } }, "sha512-2AvhNX3mb8zd6Zy7INTtSpl1F15HW6Wnqj0srWlkKLcpYl/gMIMJiyuGq2KeI2YFxUPjdlB+3Lc10seMLtL4cA=="], + "zod": ["zod@4.4.2", "", {}, "sha512-IynmDyxsEsb9RKzO3J9+4SxXnl2FTFSzNBaKKaMV6tsSk0rw9gYw9gs+JFCq/qk2LCZ78KDwyj+Z289TijSkUw=="], "zod-to-json-schema": ["zod-to-json-schema@3.25.2", "", { "peerDependencies": { "zod": "^3.25.28 || ^4" } }, "sha512-O/PgfnpT1xKSDeQYSCfRI5Gy3hPf91mKVDuYLUHZJMiDFptvP41MSnWofm8dnCm0256ZNfZIM7DSzuSMAFnjHA=="], diff --git a/docs/agents/multi-agent-design.md b/docs/agents/multi-agent-design.md index 4c113113..9cc56670 100644 --- a/docs/agents/multi-agent-design.md +++ b/docs/agents/multi-agent-design.md @@ -43,6 +43,21 @@ Discussion ─┬─ Explore Stable Agent prompts describe identity and authority. Workflow methods live in Skills, including `orchestrate-work`, `plan-work`, `execute-plan`, `run-goal`, `shape-todo`, `review-work`, and `goal-review`. Analyst can combine analysis and review Skills without creating a new Agent identity for every professional role. +A Skill is a standard local package: required `SKILL.md`; optional +`scripts/`, `references/`, `assets/`, and other contained resources. Its +frontmatter accepts only `name`, `description`, `license`, `compatibility`, and +`metadata`; `description` contains both the method and activation timing. +`skill_list` and Prompt discovery expose metadata only. `skill_read` then loads +the entry and its resource descriptors, and can load exactly one listed text +resource on demand. Project > user > builtin is whole-package precedence; no +entry or resource is merged from a lower source, and reserved lifecycle +builtins remain unshadowable. + +The package mechanism changes disclosure and storage only. A Skill cannot add +tools, execute a script automatically, change Profiles or MCP access, widen +workspace scope, change delegation, or grant completion authority. A script, +when applicable, is run only through the Agent's existing Bash permission. + A Plan is an ordinary Markdown file under `.archcode/plans/`, not a service, state machine, Session identity, or Goal dependency. ## Sessions, Todos, and Goals diff --git a/docs/concepts.md b/docs/concepts.md index d3e84f32..a2df9535 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -109,8 +109,18 @@ Root Lead and Discussion default to `principal`; Analyst uses `deep`; Explore and Librarian use `fast`; Build can use `deep` or `fast`. Profiles do not change Agent tools or authority. -Skills provide task-specific working methods. They guide behavior without -granting additional tools or permissions. +Skills are task-specific working methods packaged as a directory: a required +`SKILL.md` entry plus optional `scripts/`, `references/`, `assets/`, and other +resources. The entry has standard YAML frontmatter (`name`, `description`, and +optional `license`, `compatibility`, `metadata`); its description says both +what the Skill does and when to use it. + +Discovery exposes only a Skill's name, description, and source. Activating it +loads its entry and a resource list; an Agent reads one listed resource only +when needed. Project packages override user packages, which override builtin +packages, as whole packages. Reserved lifecycle builtins cannot be shadowed. +Skills guide behavior without granting tools, permissions, delegation, +Profiles, MCP access, workspace scope, or completion authority. ## Approvals and questions diff --git a/docs/goals/skill-package-hard-cut-plan-goal.md b/docs/goals/skill-package-hard-cut-plan-goal.md new file mode 100644 index 00000000..a0790c59 --- /dev/null +++ b/docs/goals/skill-package-hard-cut-plan-goal.md @@ -0,0 +1,222 @@ +# Skill Package Hard-Cut Plan Goal + +> 本文是 ArchCode Skill Package 重构与内置 Skill 优化的实施、验收契约。实施进度与证据另记于 `skill-package-hard-cut-progress.md`，不得回写并稀释本契约。 + +## Objective + +把 ArchCode 的 Skill 从“一个 Markdown 字符串”彻底重构为符合 Agent Skills 公开规范的本地多文件包，并在同一 hard cut 中迁移、精炼现有 14 个 builtin Skills。这里的“标准”指包结构、稳定 frontmatter、相对路径和渐进披露；唯一明确排除的是规范中可选且标记为 experimental 的 `allowed-tools`。完成后： + +- 模型发现 Skill 时只看到标准元数据；激活时读取 `SKILL.md` 与资源清单；需要细节时再读取单个资源。 +- project、user、builtin 三种来源使用同一包语义、同一校验和同一权限边界。 +- 内置 Skill 可携带 `references/`、`assets/` 等标准资源，并完整进入 Bun 编译产物；没有实际用途就不创建目录。 +- Skill 仍然只是方法指导，绝不授予工具、Agent、Profile、MCP、workspace 或完成权限。 +- 旧单字符串模型、旧 frontmatter、兼容读取、fallback、迁移和墓碑测试全部删除。 + +## Evidence Baseline + +实施以以下固定源码快照为参考，不追随浮动分支： + +| Evidence | 固定快照 | 本 Goal 借鉴内容 | +| --- | --- | --- | +| Agent Skills | [`217be54`](https://github.com/agentskills/agentskills/tree/217be548739f21d6008915c29aefe320ea1a90af) | 目录结构、标准 frontmatter、渐进披露、相对资源路径 | +| OpenCode | [`fe82a1b`](https://github.com/anomalyco/opencode/tree/fe82a1b6ca4f535beb973b0867017e3f639f85ed) | 元数据发现、激活后暴露资源清单、包级优先级 | +| Superpowers | [`44c9b2d`](https://github.com/obra/superpowers/tree/44c9b2d6e889982ac18c27d05a19fefe335194e1) | MIT 工作流内容、references/scripts/examples 拆分和行为场景设计 | +| OMO | [`76b9aa8`](https://github.com/code-yeongyu/oh-my-openagent/tree/76b9aa8d3adb7260ca2a260241e3bafb3e78db0c) | 多来源 loader 与 shared-skills 的设计证据；Sustainable Use License 内容只研究，不直接复制 | +| OMO Slim | [`ecb4f55`](https://github.com/alvinunreal/oh-my-opencode-slim/tree/ecb4f55e87c7cea9f18759eaca0eff8fb7edf1d0) | MIT 的 codemap、verification、worktree 等精简方法与来源标注 | + +参考不等于照搬宿主能力。ArchCode 不引入竞品的远程目录、Skill 内 MCP、model/agent override、自动更新或 Skill 授权语义。 + +## Locked Architecture + +### Package contract + +```text +/ +├── SKILL.md # required: metadata + concise method entry +├── scripts/ # optional: executable code +├── references/ # optional: documentation loaded on demand +├── assets/ # optional: templates and static resources +└── ... # optional: any additional files/directories +``` + +This is the public Agent Skills directory shape rather than an ArchCode-specific taxonomy。Builtin examples belong in focused `references/` files；reusable templates belong in `assets/`。Additional directories are accepted by the package reader because the specification permits them, but curated builtins use the three standard directories unless a concrete need justifies another one。 + +Curated `SKILL.md` entries stay below the specification's recommended 500 lines and reference supporting files directly with Skill-root-relative paths such as `references/review-packet.md` or `assets/plan-template.md`。Do not require the model to discover a resource through a chain of nested reference files。 + +- project source remains `/.archcode/skills//`；user source remains `~/.archcode/skills//`；builtin source remains compiled with Agent Core。 +- Skill discovery remains one directory below each source root；only supporting resources recurse inside a package。Do not add `.claude`、`.agents`、OpenCode directories or arbitrary source configuration。 +- Ordinary Skill precedence remains project > user > builtin。A winning package replaces the lower package atomically；entry and resources never merge or fall through。 +- Reserved lifecycle builtin Skills remain unshadowable and still require the current Agent eligibility。An invalid or unreadable winning package fails closed。 + +### Standard metadata hard cut + +`SKILL.md` uses strict YAML frontmatter： + +- required: `name`, `description`；`description` must describe both function and activation timing。 +- supported optional fields: `license`, `compatibility`, `metadata`。 +- `name` must match `^[a-z0-9]+(?:-[a-z0-9]+)*$`, be 1–64 characters, and equal the package directory name。 +- `description` is 1–1024 characters and contains both what the Skill does and when to use it；`compatibility`, when present, is 1–500 characters；`metadata` is a string-to-string map。 +- Delete ArchCode-only `when_to_use` and `allowed_tools` from types, parser, prompts, tools, tests and all builtin files。No dual schema or migration。 +- Add the maintained `yaml` runtime dependency for Skill frontmatter only。Do not expand or replace the repository-wide simple frontmatter utility used by unrelated subsystems。 +- Reject every top-level frontmatter field outside the five adopted fields through one generic unknown-field rule；do not add field-specific ignore, display, compatibility or rejection branches。 + +### Runtime ownership and read flow + +Keep one public `SkillService`; do not build a generic plugin/catalog framework。Its internals have four cohesive responsibilities： + +```text +schema.ts -> parse and validate Skill metadata/body +package-reader.ts -> validate and read one filesystem package +builtin/manifest.ts -> provide embedded builtin package contents +service.ts -> resolve source precedence and Agent eligibility +``` + +The model-facing flow is： + +1. `skill_list({})` and the System Prompt list exactly `name`, `description`, `source`。They do not load body or resource contents。 +2. Active lifecycle Skills and `skill_read({ name })` load the winning `SKILL.md` body plus a sorted list of relative resource paths；resource contents remain absent。 +3. `skill_read({ name, resource })` re-resolves precedence and attempts a text read of exactly one listed resource from the current winning package。Valid UTF-8 is returned exactly；binary content gets a deterministic unsupported-binary result rather than invalidating the package。`resource` is a package-relative path, never an arbitrary filesystem path；if the winner changed after activation, no snapshot guarantee is made and no lower source may supply the resource。 +4. Do not add `skill_resource_list`, `skill_resource_read`, cache/invalidation, VFS, registry interface or another service。The existing `skill_read` owns both entry and contained-resource reads。 + +`ResolvedSkill` becomes a package-oriented result containing metadata, entry body, source, source label, optional filesystem package root and immutable `{ path, bytes }` resource descriptors。Prompt trace may keep its existing `{name, source}` protocol shape；it must use the resolved package source and must not invent a new persisted Skill state machine。 + +`skill_read` output is deterministic： + +- Entry read emits the current metadata header in fixed field order (`name`, `description`, `source`, optional filesystem `root`, then present optional fields), a `Resources` section sorted by `path` as `- ( bytes)`, then the exact Markdown body。An empty package emits `Resources: none`。The root lets an Agent resolve standard relative script/asset references for project/user packages；embedded builtins have no pretend filesystem path。 +- Text resource read emits a fixed header in the order `skill`, `source`, `resource`, `bytes`, then the exact decoded resource text。Unsupported binary reads use the same identity header plus a fixed error code/hint and never repeat the package body or other resource contents。 +- `skill_list` and available Prompt metadata remain exactly `name`, `description`, `source`；license, compatibility and provenance appear only after entry activation。 + +### Resource and packaging boundaries + +- `SKILL.md` must be a regular, valid UTF-8 file。Package resources may contain arbitrary bytes, as required for standard `assets/`；the text-only `skill_read` path decodes a requested resource with fatal UTF-8 validation and returns a clear unsupported-binary error when it is not text。Symlinked package roots, entries, directories or resources are rejected even when the target stays inside the root。 +- Normalize resource paths to sorted POSIX-relative paths。Reject absolute paths, empty segments, `.`/`..`, backslashes and paths outside the winning package。 +- Fixed v1 limits, with no config surface: frontmatter 16 KiB；`SKILL.md` 128 KiB；one resource 1 MiB；at most 128 resource files；at most 256 total directory entries；resource depth at most 8 segments below the package root；aggregate bytes of `SKILL.md` plus all resource files 8 MiB。 +- Discovery reads only enough bytes to close and validate the bounded frontmatter。Body, bounded resource traversal, file sizes and aggregate limits are validated at activation；resource bytes and UTF-8 validity are checked only when that one resource is requested。 +- `scripts/` follows the standard meaning: executable code referenced relative to the Skill root。An Agent may execute an accessible project/user script only through its existing Bash permission and normal path controls；this does not create a Skill-owned permission path。The 14 curated builtins do not add executable scripts in this Goal, so compiled-builtin materialization is unnecessary。Do not add `skill_run`, temp materialization or automatic execution。 +- Replace `BUILTIN_SKILL_BODIES` with `BUILTIN_SKILL_PACKAGES`。Use explicit static imports for every curated builtin entry/resource；text entries may use Bun's text loader and arbitrary assets use the file loader plus `Bun.file(...).bytes()` so standalone binaries preserve exact bytes。Do not add code generation or runtime source-tree scanning。 +- A manifest completeness test compares the source builtin directories with the declared package map during tests。A serial integration smoke compiles and executes a temporary standalone binary that imports the real `SkillService` and builtin manifest: it asserts one real embedded builtin text resource, then constructs one test-only builtin package through the same package contract from a statically imported non-UTF-8 fixture and asserts byte-for-byte equality before the text reader returns the standard unsupported-binary result。It uses an isolated temp directory and leaves no product hook or artifact。The normal `bun run build` must also succeed。 + +## Builtin Skill Content Migration + +Analyze and migrate one Skill at a time。Do not create empty directories, ceremonial references or duplicate a method merely to make every Skill multi-file。 + +| Skill(s) | Required improvement and primary reference | +| --- | --- | +| `analyze-work` | Keep mode selection in `SKILL.md`; move falsifiable diagnosis, root-cause tracing and boundary probes into focused references，primarily Superpowers systematic-debugging | +| `safe-refactor` | Keep behavior-preserving loop in entry；add dependency-boundary and verification decision material from MIT refactoring/TDD sources | +| `review-change` | Separate plan/code/security lenses, finding quality, severity/confidence and unable-to-conclude examples，using Superpowers review/verification methods | +| `review-work` | Keep Lead orchestration in entry；move review-packet and remediation-loop detail into a reference，without duplicating Analyst review logic | +| `goal-review` | Keep criterion-by-criterion independent final gate；provide a compact evidence-matrix example，without machine verdicts or Goal status authority | +| `shape-todo` | Adapt Superpowers brainstorming around evidence-first shaping；provide a concise scope/decision/acceptance template tied to the bound Todo | +| `plan-work` | Adapt writing-plans structure into an ArchCode Plan template；retain Analyst read-only draft and Lead/Discussion write ownership | +| `execute-plan` | Adapt executing-plans checkpoints while preserving Todo-bound Plan authority and current handoff semantics | +| `orchestrate-work` | Keep direct-vs-delegate decisions concise；use references only for delegation packets and integration gates | +| `run-goal` | Keep current Goal lifecycle, stop conditions and fresh final Analyst review；do not import a generic workflow engine | +| `codemap` | Adapt OMO Slim codemap ideas into a slim evidence map schema and one example；do not create persistent codemap runtime services | +| `research-docs` | Add source ranking, version/conflict handling, direct-link and stopping criteria as a reusable reference；keep official-source-first behavior | +| `git-master` | Use MIT Git/worktree/finish-branch material for operation selection and safety；do not copy Sustainable Use License text from OMO main | +| `automation-create` | Remain runtime-schema-led and compact；add examples only when they clarify once/interval/cron and timezone ambiguity | + +Each migrated Skill must record `license` and `metadata` provenance (`archcode/source`, pinned commit, and adaptation type)。Substantial MIT text/code adaptation bundles the required notice；idea-only rewrites remain ArchCode MIT and still cite the evidence in the progress matrix。OMO main is never a direct-copy source。 + +## Implementation Plan + +1. **Hard-cut domain/schema**：replace string/body types with package types；add Skill-specific YAML parsing with only the five adopted Agent Skills metadata fields/limits and one generic unknown-field rule；remove old fields and simple-YAML dependence from Skills。 +2. **Filesystem package reader**：implement bounded header discovery, activation-time recursive manifest validation, entry UTF-8/size/symlink/path checks, arbitrary-byte resource inventory and single text-resource reads。 +3. **Resolver**：refactor `SkillService` to resolve complete project/user/builtin packages atomically while preserving ordinary precedence, reserved builtins, Agent eligibility and fail-closed behavior。 +4. **Builtin embedding**：replace the body map with the explicit package map and completeness tests；prove all declared resources work in tests and `bun run build`。 +5. **Model contract**：extend `skill_read` with optional `resource`；update `skill_list`, Prompt rendering, traces, tool descriptions and model-visible contract tests；do not add another tool。 +6. **Migrate content one by one**：for each of the 14 Skills, first record functional overlap/source/license, then rewrite/split only justified content, run its structural and role-boundary checks, and continue to the next Skill。 +7. **Documentation and hard-cut cleanup**：update `AGENTS.md` and active architecture docs；add the data-breaking release note for existing project/user Skills；delete obsolete exports, fixtures and tests without adding legacy rejection/tombstone coverage。 +8. **Verification and review**：run focused tests, all Agent Core lanes, root typecheck/test/build and diff checks；run the pinned official `skills-ref validate` against every builtin package as one-off conformance evidence without adding it as an ArchCode runtime dependency；then use a fresh independent deep Reviewer to inspect the full implementation against every AC and repeat fix -> review for blocking/high findings。 + +## Non-goals + +- No remote HTTP/Git catalog, marketplace, installation, update, lockfile or source configuration。 +- No `.claude`/`.agents` compatibility scan, recursive nested Skill discovery or cross-package resource references。 +- No Skill-provided MCP, Agent/Profile/model override, tool permission grant, hooks or executable runtime。 +- No new image/PDF renderer, generic MIME system, binary-to-model transport, VFS, cache, version resolver or workflow engine。Binary files remain valid package assets；this Goal does not invent a second media toolchain to render them。 +- No migration of persisted runtime state；Skills remain filesystem/builtin guidance and are not new product work items。 + +## Risks And Controls + +| Risk | Control / accepted tradeoff | +| --- | --- | +| Existing user/project Skills stop parsing | Intentional hard cut；publish exact new schema and manual conversion instructions，with no fallback or migration | +| Package traversal or symlink escape | Reject all symlinks；canonical contained paths, POSIX-relative validation and fixed depth/entry/file/aggregate limits | +| Builtin resources work in source but disappear from binary | Explicit static imports, manifest completeness test and full standalone build | +| Skill metadata accidentally expands authority | Only the five adopted metadata fields exist；runtime Agent definitions remain the sole tool-permission authority | +| Content becomes long or duplicated | Entry stays a concise router；resources are focused and loaded on demand；no forced resource count or shared generic framework | +| Upstream copying violates license | Pin commit/license per Skill；copy only MIT-compatible material with required notice；OMO main is research-only | +| Scripts create a hidden execution path | No executor/materializer；readable contents only，execution remains ordinary Bash permission where a real filesystem path exists | + +## Acceptance Criteria + +AC-01 through AC-09 must all have source, test, search or build evidence。Any missing item is `NOT_DONE`。 + +### AC-01: one package model and exact source semantics + +- Production types and service represent a Skill as one package with metadata, entry, source and resources；`Record` builtin bodies and entry-only candidates no longer exist。 +- Only project、user、builtin sources exist；ordinary precedence is project > user > builtin；reserved builtins remain unshadowable and Agent-gated。 +- A winning package is atomic：a missing resource never falls through to a lower source；invalid/unreadable winning packages fail closed。 + +### AC-02: standard metadata hard cut is complete + +- Required/optional fields, limits, name regex and directory-name equality match “Standard metadata hard cut”。Nested `metadata` parses correctly through the dedicated YAML parser。 +- Every builtin uses the new schema；available-skill Prompt/tool output no longer depends on separate `when_to_use`。 +- Production code and current fixtures have zero `when_to_use`, `allowed_tools`, `BUILTIN_SKILL_BODIES` or Skills using the generic simple-YAML parser。 +- A generic unknown-field fixture fails validation；production code has no field-specific branch for unsupported frontmatter names。 +- No alias, optional legacy field, dual parse, migration, feature flag, fallback or tombstone test remains。 + +### AC-03: progressive disclosure is real + +- `skill_list` and available Prompt metadata succeed without reading body or walking resource contents；a focused test proves metadata discovery does not invoke full-entry/resource reads。 +- `skill_read({name})` returns the exact winning entry body and sorted relative resource descriptors, but none of their contents。 +- `skill_read({name, resource})` re-resolves and returns exactly one declared resource from the current winning package；unknown or unlisted resources fail and never fall through to a lower source。Changing files or precedence between calls may change the winner and is intentionally not snapshot-consistent。 +- Auto-active lifecycle Skills use the same resolved package result and expose the same resource list as explicit activation。 +- Entry and resource reads use the exact deterministic envelopes defined in “Runtime ownership and read flow”；tests assert field order, sorted descriptors, exact body/text and absence of unrequested resource contents。 + +### AC-04: resource safety and limits are enforced + +- Tests cover every fixed byte/depth/entry/count limit at below/equal/above boundaries, valid entry UTF-8, arbitrary-byte resource inventory, text-read invalid UTF-8/unsupported-binary failure, absolute/traversal/backslash paths and symlinked root/entry/directory/resource。 +- Package resource descriptors are stable, unique, sorted POSIX-relative paths；`SKILL.md` is never duplicated as a resource。 +- Project/user reads stay inside the winning package；builtin reads use only the embedded map。No model input can supply a source or absolute base path。 + +### AC-05: builtin packages survive standalone compilation + +- `BUILTIN_SKILL_PACKAGES` contains all 14 entries and every non-`SKILL.md` file under each builtin directory；the completeness test rejects omissions and extras。 +- At least one multi-file builtin is read through `skill_read` in ordinary tests。A serial integration smoke compiles and runs a temporary standalone binary against the real `SkillService`/builtin manifest, asserts one real embedded text resource, and passes a statically imported non-UTF-8 fixture through the same builtin package contract to prove exact byte round-trip plus deterministic unsupported-binary reading；`bun run build` also exits 0。 +- There is no runtime scan of the repository source tree, generated manifest, dev-only filesystem fallback or post-build copy step。 + +### AC-06: Agent authority is unchanged + +- Skill availability remains definition-based plus custom project/user behavior；resource reads require the same allowed Skill name and current Agent eligibility as entry reads。 +- The only accepted metadata fields are `name`, `description`, `license`, `compatibility`, and `metadata`；none can add, remove or pre-approve a tool for any Agent。 +- No Skill can alter delegation targets, Profile/model, MCP, Goal completion authority, workspace scope or permissions；Prompt states the same guidance-only contract。 +- No `skill_run`, automatic script execution, temp materialization or second execution path exists。 + +### AC-07: all builtin content is individually justified + +- The progress document contains one row per builtin Skill with pinned source, license, functional overlap, retained ArchCode constraints, files added/moved and reason for either using or not using resources。 +- Every `SKILL.md` is a concise entry method；detailed reusable material lives in focused resources and every referenced relative path exists。No empty/ceremonial directory or duplicate cross-Skill manual is accepted。 +- Every builtin entry is below 500 lines, directly names each supporting resource with a Skill-root-relative path, and does not require multi-hop reference discovery。 +- The role boundaries in the migration table are preserved，especially Discussion no implementation、Analyst source-read-only、Lead completion ownership and runtime-schema-led Automation。 +- Substantial third-party adaptations include required license/notice；OMO main contributes no copied text/code。 + +### AC-08: hard cut and low-coupling audit passes + +- Deleted legacy types/functions/exports/tests/docs have no consumer；no compatibility adapter, migration, fallback, deprecated field, dual read/write or graveyard test exists。 +- `SkillService + package-reader + schema + builtin manifest` is the complete implementation；there is no generic source registry、catalog interface、VFS、cache、resource service or workflow abstraction。 +- `skill_read` is the only full Skill/resource read tool；all six Agent definitions continue to receive Skill access through the existing shared capability package。 + +### AC-09: verification and independent acceptance are complete + +- Focused schema/service/tool/prompt/manifest/model-visible tests pass，including package precedence, atomic no-merge, progressive disclosure and resource security。 +- The official `skills-ref validate` command from the pinned Agent Skills evidence snapshot accepts all 14 builtin packages；the command/version and output are recorded in the progress evidence, with no production dependency or network call added to ArchCode。 +- Agent Core unit、integration and architecture lanes pass；`bun run typecheck`, `bun run test`, `bun run build` and `git diff --check` all exit 0。 +- Exact searches and manual classification prove AC-02、AC-06 and AC-08 cleanup without deleting legitimate historical records。 +- A fresh independent `gpt-5.6-sol` deep Reviewer checks the final diff criterion-by-criterion。Any blocker/high finding is fixed and the full affected AC is re-reviewed before completion。 + +## Hard-Cut Audit + +Before marking implementation complete, search and classify at minimum：`when_to_use`, `allowed_tools`, `BUILTIN_SKILL_BODIES`, `Record` Skill inputs, entry-only `SkillCandidate.content`, Skills calling the generic simple frontmatter parser, resource fallback/merge, Skill permission grants, `skill_run`, remote catalog/config and compatibility source directories。Historical docs may remain only when clearly historical and not imported or presented as the current contract。 diff --git a/docs/goals/skill-package-hard-cut-progress.md b/docs/goals/skill-package-hard-cut-progress.md new file mode 100644 index 00000000..ea28f42f --- /dev/null +++ b/docs/goals/skill-package-hard-cut-progress.md @@ -0,0 +1,139 @@ +# Skill Package Hard-Cut Progress + +> 本文仅记录 [`skill-package-hard-cut-plan-goal.md`](./skill-package-hard-cut-plan-goal.md) 的执行进度、风险和验收证据。Goal 契约保持只读，不在这里重定义范围。 + +## Status + +- Goal: complete +- Branch: `codex/skill-optimization` +- Started: 2026-08-08 +- Current phase: accepted and complete + +## Baseline + +- Current production model loads each builtin as one imported `SKILL.md` string. +- Current filesystem discovery parses complete `SKILL.md` content rather than bounded metadata only. +- Current frontmatter uses ArchCode-only `when_to_use` and one builtin uses `allowed_tools`. +- Current `skill_read` accepts only `name`; package resources are not represented or readable. +- Existing 14 builtin `SKILL.md` edits predate this execution and are preserved as migration input. +- Unrelated untracked `packages/agent-core/src/.DS_Store` is outside scope and must remain untouched. + +## First-Principles Decisions + +| Decision | Reason | +| --- | --- | +| Keep one `SkillService` with schema, package reader, resolver, and builtin manifest modules | These are the four cohesive responsibilities required by the Goal; a registry/VFS/cache would add coupling without a current need. | +| Treat source precedence as whole-package replacement | Mixing an entry from one source with resources from another makes relative references unsafe and non-reproducible. | +| Adopt only stable Agent Skills metadata fields | Skill guidance must not become an alternate permission system; all unknown top-level fields use one validation rule. | +| Accept arbitrary resource bytes but keep `skill_read` text-only | Standard assets may be binary, while ArchCode's model-facing tool result is text; binary transport/rendering is a separate capability and remains out of scope. | +| Preserve runtime Agent eligibility and reserved builtin rules | Package loading changes storage and disclosure, not authority. | + +## Workstream Progress + +| Workstream | Owner | Status | Evidence | +| --- | --- | --- | --- | +| Core package schema/reader/resolver | root | complete | One package model, bounded discovery, fail-closed whole-package winner, resource safety, and typed single-resource reads; final focused review set 78/78 | +| Builtin content/package migration | delegated worker | complete | 14 standard entries; 13 justified resources; builtin diff-check pass | +| Model-facing tools and Prompt | delegated worker | complete | 57 focused tests pass; assigned diff-check pass | +| Architecture/test change map | delegated explorer | complete | Winner/discovery/limits/build/doc risks audited; official validator verified | +| Active docs and breaking release note | delegated worker | complete | Five active docs updated; assigned diff-check pass | +| Full verification | root | complete | Agent Core unit/integration/architecture, root typecheck/test/build, validator, standalone binary, and diff checks pass | +| Independent final review | fresh `gpt-5.6-sol` (`xhigh`) reviewer | complete | No open Blocker, Major, Minor, or required Advisory after fix-review closure | + +## Builtin Migration Matrix + +All third-party entries below are idea-only rewrites under ArchCode's MIT license; no OMO main text/code was copied and no substantial third-party text requiring a bundled notice was introduced. + +| Skill | Pinned source / license | Functional overlap and retained ArchCode constraint | Package change / justification | +| --- | --- | --- | --- | +| `analyze-work` | Superpowers `44c9b2d`, MIT | Falsifiable root-cause method; retain Analyst read-only architecture/gap modes | `references/diagnosis-method.md` isolates hypothesis, boundary tracing, and stop rules | +| `automation-create` | ArchCode `f00efe7`, MIT | Runtime-schema-led creation and user confirmation | `references/schedule-examples.md` isolates once/interval/cron/timezone disambiguation | +| `codemap` | OMO Slim `ecb4f55`, MIT ideas | Evidence map for current task, no persistent codemap service | `references/evidence-map-example.md` supplies the focused output pattern | +| `execute-plan` | Superpowers `44c9b2d`, MIT ideas | Checkpointed execution; retain Todo-bound Plan authority | `references/execution-checkpoints.md` isolates checkpoint and acceptance boundaries | +| `git-master` | Superpowers `44c9b2d`, MIT ideas | Git/worktree operation safety; preserve current runtime tools | `references/operation-safety.md` isolates operation/recovery decision cards | +| `goal-review` | ArchCode `f00efe7`, MIT | Independent evidence review without machine verdict or Goal authority | `references/evidence-matrix-example.md` supplies a human-readable criterion matrix | +| `orchestrate-work` | ArchCode `f00efe7`, MIT | Lead delegation/integration while retaining ownership | `references/delegation-packet.md` isolates bounded handoff evidence | +| `plan-work` | Superpowers `44c9b2d`, MIT ideas | Executable Plan structure; preserve Analyst draft vs Lead/Discussion write boundary | `assets/plan-template.md` is a reusable Plan template | +| `research-docs` | ArchCode `f00efe7`, MIT | Official-source-first research with fact/inference separation | `references/source-evaluation.md` isolates ranking/conflict/stopping rules | +| `review-change` | Superpowers `44c9b2d`, MIT ideas | Plan/code/security review through an Analyst read-only lens | `references/review-lenses.md` isolates lens-specific evidence checks | +| `review-work` | Superpowers `44c9b2d`, MIT ideas | Lead review orchestration and remediation closure | `references/review-packet.md` isolates packet and fix-review handoff | +| `run-goal` | ArchCode `f00efe7`, MIT | Goal execution/recovery/final review lifecycle | Kept single-file because no non-duplicative reusable detail justified a resource | +| `safe-refactor` | Superpowers `44c9b2d`, MIT ideas | Behavior-preserving refactor loop | `references/boundary-verification.md` isolates dependency-boundary verification | +| `shape-todo` | Superpowers `44c9b2d`, MIT ideas | Evidence-first shaping; Discussion never implements | `references/todo-shaping-template.md` supplies scoped decision/acceptance structure | + +## Verification Evidence + +| Check | Result | Notes | +| --- | --- | --- | +| `git diff --check` for Goal plan | pass | Passed before implementation began. | +| Builtin frontmatter/name/direct-resource self-check | pass | 14/14 entries use only adopted fields, match directory names, stay below 500 lines, and directly reference every resource. | +| Pinned official `skills-ref 0.1.0` | pass | Agent Skills commit `217be548...`; 14/14 builtin directories returned `Valid skill`. This validates frontmatter/name only, not runtime resources. | +| Model-facing Skill/Prompt focused tests | pass | 57 pass, 0 fail. | +| Active documentation diff-check | pass | `AGENTS.md`, README, CHANGELOG, concepts, and multi-agent design. | +| Final focused schema/package/service/manifest/tool/prompt set | pass | 78 pass, 0 fail; includes source precedence, discovery, every fixed boundary, ancestry symlinks, arbitrary bytes, manifest completeness, envelopes, and Prompt disclosure. | +| Standalone builtin binary smoke | pass | 1 pass, 0 fail; compiles the real service/manifest, reads a real text reference, proves exact binary SHA-256/length round-trip, and checks the unsupported-binary envelope. | +| Agent Core unit lane | pass | 2,906 pass, 0 fail across 213 files. | +| Agent Core integration lane | pass | 141 pass, 0 fail across 24 files. | +| Agent Core architecture lane | pass | 81 pass, 0 fail across 17 files. | +| `bun run typecheck` | pass | 5/5 workspace tasks successful. | +| `bun run test` | pass | 8/8 Turborepo tasks successful. | +| `bun run build` | pass | Full typecheck, Vite production build, temporary embedded-asset entry, and compiled binary pipeline exited 0. | +| `git diff --check` | pass | Full worktree diff has no whitespace errors. | + +The pinned validator command was: + +```sh +UV_CACHE_DIR=/tmp/archcode-skill-uv-cache \ +UV_TOOL_DIR=/tmp/archcode-skill-uv-tools \ +UV_PYTHON_INSTALL_DIR=/tmp/archcode-skill-uv-python \ +uvx --from 'git+https://github.com/agentskills/agentskills.git@217be548739f21d6008915c29aefe320ea1a90af#subdirectory=skills-ref' \ + skills-ref validate +``` + +## Hard-Cut Search Classification + +- Production Skill types, parser, tools, Prompt, manifest, and builtin entries contain zero `when_to_use`, `allowed_tools`, or `BUILTIN_SKILL_BODIES`. The only current active-doc occurrence is the CHANGELOG breaking-removal instruction. +- `packages/agent-core/src/utils/frontmatter.ts` and its memory consumers remain because they belong to the unrelated Memory subsystem; the Skill schema imports `yaml` directly and has no dependency on that generic parser. +- `Record` remains only in unrelated generic/config/test helper types and test-only resource fixture builders; builtin production input is `BuiltinSkillPackage`, whose resources accept `string | Uint8Array`. +- `SkillCandidate` remains a private resolver union containing only source/root or embedded package identity; it has no entry/body/content field and performs no cross-source merge. +- Production has no `skill_run`, resource-list/resource-read sibling tool, remote catalog/source registry, compatibility source scan, cache, VFS, migration, or fallback resource path. Generic uses of words such as compatibility, merge, and fallback outside the Skill package subsystem are unrelated and retained. + +## Risks / Corrections + +- Binary builtin assets require a real standalone byte round-trip, not only a text-resource build check. +- Generic unknown-field validation must not become a field-specific legacy/tombstone test. +- Discovery tests must prove that neither the Markdown body nor package traversal is performed, rather than merely checking a metadata-shaped return value. +- Discovery originally checked total entry size; corrected because body size belongs to activation. Prefix reads now loop to EOF/limit rather than assuming one file read fills the buffer. +- Builtin package entry counts originally omitted implicit resource directories; corrected to match filesystem package accounting. +- Resource reads now open without following the final symlink and re-check file type/size on the open handle before and after reading. + +## Independent Final Review And Fix Closure + +The required fresh independent review used `gpt-5.6-sol` with `xhigh` reasoning. It found one Major and five Minor defects; each was fixed and re-reviewed by the same reviewer: + +| Severity | Finding | Closure evidence | +| --- | --- | --- | +| Major | A symlink in project Skill ancestry could make an outside package appear under the lexical project path | Reader APIs now require a trusted boundary and check every directory before and after discovery/activation/resource reads; project/user source boundaries are explicit; package/service regressions pass. | +| Minor | `listForAgent` could `readdir` a symlinked source root before package validation | Source ancestry is checked before and after name enumeration, including an empty external-root regression. | +| Minor | Direct candidate existence could probe through a source symlink and then fall back to builtin | Candidate existence now uses the same boundary-aware traversal before any guessed package stat or fallback. | +| Minor | Builtin resources could use the impossible path `SKILL.md/hidden.txt` | Every resource whose first segment is `SKILL.md` is rejected; builtin and generic path regressions pass. | +| Minor | Description/compatibility limits were counted after trim | Raw YAML strings are code-point counted before trim/min validation, matching the pinned validator boundary. | +| Minor | Inherited object property `constructor` could be mistaken for a builtin Skill | Builtin lookup requires `Object.hasOwn`; direct discovery and activation return no Skill for inherited names. | + +Final reviewer conclusion: AC-01 through AC-09 pass, with no open Blocker, Major, Minor, or required Advisory. It separately confirmed that active Prompt metadata and UTF-8 BOM behavior do not violate the locked contract: auto-active Skills expose the required body/resource inventory, while optional metadata remains available through explicit entry activation; decoded text semantics do not require preserving a BOM as body content. + +## Final Acceptance Audit + +| Criterion | Result | Primary evidence | +| --- | --- | --- | +| AC-01 | pass | Package-oriented types/service, own-property builtin lookup, project > user > builtin whole-package tests, reserved gates, no resource fallthrough. | +| AC-02 | pass | Strict five-field YAML schema and raw character limits; 14/14 validator pass; zero old fields/body map/Skill generic-parser use. | +| AC-03 | pass | Metadata-only discovery tests; deterministic entry/resource envelopes; active Prompt resource descriptors; winner re-resolution tests. | +| AC-04 | pass | Below/equal/above limits, invalid paths, entry/resource UTF-8, arbitrary bytes, full ancestry and root/entry/directory/resource symlink regressions. | +| AC-05 | pass | Complete explicit 14-package manifest; real text resource plus exact binary standalone round-trip; production build passes. | +| AC-06 | pass | Existing Agent-definition eligibility remains authoritative; guidance-only Prompt; no executor/materializer/permission metadata. | +| AC-07 | pass | Fourteen-row migration matrix, direct resource references, all entries below 500 lines, role-boundary checks, no copied OMO main content. | +| AC-08 | pass | Only schema/package-reader/service/manifest responsibilities; hard-cut search classification; no registry/VFS/cache/second resource tool/fallback. | +| AC-09 | pass | Focused and all Agent Core lanes, root typecheck/test/build, official validator, diff-check, and fresh independent fix-review all pass. | + +Accepted residual risk: a separate malicious local process could attempt a nanosecond-scale directory swap between pre/post checks. Static and persistent symlink escape is rejected; fully atomic `openat`-style traversal is unavailable through the current JavaScript file API and is outside this local same-trust-boundary Goal. diff --git a/packages/agent-core/package.json b/packages/agent-core/package.json index 5f61724a..ea00378f 100644 --- a/packages/agent-core/package.json +++ b/packages/agent-core/package.json @@ -57,6 +57,7 @@ "vscode-jsonrpc": "^8.2.1", "vscode-languageserver-protocol": "^3.17.5", "vscode-languageserver-types": "^3.17.5", + "yaml": "^2.8.1", "zod": "^4.4.2", "zustand": "^5.0.13" }, diff --git a/packages/agent-core/src/agents/configured-agent.test.ts b/packages/agent-core/src/agents/configured-agent.test.ts index f4acdbf1..8a4d0932 100644 --- a/packages/agent-core/src/agents/configured-agent.test.ts +++ b/packages/agent-core/src/agents/configured-agent.test.ts @@ -1,6 +1,6 @@ import { afterAll, afterEach, beforeAll, describe, expect, mock, test } from "bun:test"; -import { mkdir, realpath, rm, writeFile } from "node:fs/promises"; -import { join } from "node:path"; +import { mkdir, rm, writeFile } from "node:fs/promises"; +import { join, resolve } from "node:path"; import { tmpdir } from "node:os"; import { z } from "zod"; import { ModelInfo } from "../provider/model"; @@ -64,22 +64,6 @@ function createTestSkillService(): SkillService { return new SkillService(); } -function createSkillServiceWithToolGrant(): SkillService { - return new SkillService({ - builtinSkills: { - "github-skill": [ - "---", - "name: github-skill", - "description: GitHub skill", - "when_to_use: Use for GitHub watching.", - "allowed_tools: [github_get_pull_request, github_merge_pull_request]", - "---", - "This skill can describe GitHub workflows but cannot grant tools.", - ].join("\n"), - }, - }); -} - class RecordingBackgroundTaskManager { readonly dispatched: string[] = []; drainCalls = 0; @@ -432,14 +416,13 @@ describe("ConfiguredAgent", () => { test("executes commands before admission and returns continuation as ordinary text", async () => { const skillService = new SkillService({ builtinSkills: { - "git-master": [ + "git-master": { entry: [ "---", "name: git-master", - "description: Git expertise", - "when_to_use: Use for git work.", + "description: Git expertise. Use for git work.", "---", "Full body", - ].join("\n"), + ].join("\n"), resources: {} }, }, }); const agent = createAgent({ definition: leadAgentDefinition, skillService }); @@ -463,7 +446,7 @@ describe("ConfiguredAgent", () => { test("does not append a command notice or continuation after Stop wins during the handler", async () => { const abortController = new AbortController(); const skillService = { - readForAgent: mock(async () => { + discoverForAgent: mock(async () => { abortController.abort(new Error("Session family cancelled")); return { name: "git-master" }; }), @@ -1334,29 +1317,6 @@ describe("ConfiguredAgent", () => { expect(streamFn).not.toHaveBeenCalled(); }); - test("skill metadata allowed_tools is prompt metadata only and cannot grant missing tools", async () => { - const streamFn = setupMockStreamText("skill metadata ok"); - const skillService = createSkillServiceWithToolGrant(); - const store = createStore(crypto.randomUUID(), tmpRoot, { - agentName: "lead", - activeSkillNames: ["github-skill"], - }); - const agent = createAgent({ - definition: definitionWith({ tools: { tools: ["file_read"] }, skills: ["github-skill"] }), - skillService, - store, - }); - - await runAgent(agent, "skill metadata run"); - - const callArgs = streamFn.mock.calls[0]![0] as { system: string }; - expect(callArgs.system).toContain("[allowed_tools: github_get_pull_request, github_merge_pull_request]"); - expect(callArgs.system).toContain("This skill can describe GitHub workflows but cannot grant tools."); - expect(callArgs.system).toContain("- file_read"); - expect(callArgs.system).not.toContain("- github_get_pull_request"); - expect(callArgs.system).not.toContain("- github_merge_pull_request"); - }); - test("fails closed when a persisted active Skill is deleted between runs", async () => { const streamFn = setupMockStreamText("active skill loaded"); const skillName = "ephemeral-skill"; @@ -1365,8 +1325,7 @@ describe("ConfiguredAgent", () => { await writeFile(join(skillDir, "SKILL.md"), [ "---", `name: ${skillName}`, - "description: Temporary skill", - "when_to_use: Use for this test.", + "description: Temporary skill. Use for this test.", "---", "Temporary instructions.", ].join("\n")); @@ -1384,7 +1343,7 @@ describe("ConfiguredAgent", () => { try { await runAgent(agent, "first run"); expect(store.getState().promptTraces?.at(-1)?.skills.active).toEqual([ - { name: skillName, source: join(await realpath(skillDir), "SKILL.md") }, + { name: skillName, source: resolve(skillDir) }, ]); await rm(skillDir, { recursive: true, force: true }); const eventStart = store.getState().events.length; diff --git a/packages/agent-core/src/agents/configured-agent.ts b/packages/agent-core/src/agents/configured-agent.ts index 1bbe9b5c..4ad2bbb4 100644 --- a/packages/agent-core/src/agents/configured-agent.ts +++ b/packages/agent-core/src/agents/configured-agent.ts @@ -339,7 +339,7 @@ export class ConfiguredAgent implements Agent { }); const trace = durablePromptTrace(createFailedPromptTrace(contract, error, { status: "error", - active: activeSkills.map((skill) => ({ name: skill.metadata.name, source: skill.path ?? skill.source })), + active: activeSkills.map((skill) => ({ name: skill.metadata.name, source: skill.sourceLabel })), })); this.store.getState().append({ type: "prompt-trace", trace }); await this.storeManager.flushSession(this.store.getState().sessionId, this.projectRoot); diff --git a/packages/agent-core/src/agents/definitions/definitions.test.ts b/packages/agent-core/src/agents/definitions/definitions.test.ts index 4ac58848..f9935706 100644 --- a/packages/agent-core/src/agents/definitions/definitions.test.ts +++ b/packages/agent-core/src/agents/definitions/definitions.test.ts @@ -16,7 +16,7 @@ import { import { TOOL_COMPRESS, } from "../../tools/names"; -import { BUILTIN_SKILL_BODIES } from "../../skills"; +import { BUILTIN_SKILL_PACKAGES } from "../../skills"; const EXPECTED_TOOL_MATRIX = { lead: [ @@ -240,7 +240,7 @@ describe("Agent catalog", () => { } for (const name of ["orchestrate-work", "plan-work", "execute-plan", "run-goal", "shape-todo", "review-work", "goal-review"] as const) { - expect(BUILTIN_SKILL_BODIES[name]).toBeString(); + expect(BUILTIN_SKILL_PACKAGES[name].entry).toBeString(); } expect(leadAgentDefinition.skills).toEqual(expect.arrayContaining([ "orchestrate-work", "plan-work", "execute-plan", "run-goal", "review-work", diff --git a/packages/agent-core/src/agents/factory.test.ts b/packages/agent-core/src/agents/factory.test.ts index bc8c5361..12c0414f 100644 --- a/packages/agent-core/src/agents/factory.test.ts +++ b/packages/agent-core/src/agents/factory.test.ts @@ -62,8 +62,8 @@ function createTestSkillService(): SkillService { function createSkillServiceWithBuiltins(): SkillService { return new SkillService({ builtinSkills: { - "git-master": "---\nname: git-master\ndescription: Git helper\nwhen_to_use: Use for git operations.\n---\nUse git carefully.", - codemap: "---\nname: codemap\ndescription: Code map helper\nwhen_to_use: Use before implementation.\n---\nMap code first.", + "git-master": { entry: "---\nname: git-master\ndescription: Git helper. Use for git operations.\n---\nUse git carefully.", resources: {} }, + codemap: { entry: "---\nname: codemap\ndescription: Code map helper. Use before implementation.\n---\nMap code first.", resources: {} }, }, }); } diff --git a/packages/agent-core/src/agents/factory.ts b/packages/agent-core/src/agents/factory.ts index 3c6e4b64..ec337240 100644 --- a/packages/agent-core/src/agents/factory.ts +++ b/packages/agent-core/src/agents/factory.ts @@ -168,7 +168,7 @@ async function resolveDelegatedSkillNames( } for (const skillName of dedupedNames) { - const skill = await skillService.readForAgent(workspaceRoot, skillName, targetDefinition.skills); + const skill = await skillService.discoverForAgent(workspaceRoot, skillName, targetDefinition.skills); if (skill === null) { throw new SkillNotFoundError(skillName); } diff --git a/packages/agent-core/src/agents/session-agent-manager.test.ts b/packages/agent-core/src/agents/session-agent-manager.test.ts index f4536f0b..718ccbf5 100644 --- a/packages/agent-core/src/agents/session-agent-manager.test.ts +++ b/packages/agent-core/src/agents/session-agent-manager.test.ts @@ -3,7 +3,7 @@ import { z } from "zod"; import { ModelInfo } from "../provider/model"; import type { ExecutionModelBinding } from "../models"; import { SkillService } from "../skills"; -import { BUILTIN_SKILL_BODIES } from "../skills/builtin/manifest"; +import { BUILTIN_SKILL_PACKAGES } from "../skills/builtin/manifest"; import { SessionStoreManager } from "../store/session-store-manager"; import type { ToolRegistry } from "../tools/registry"; import type { AnyToolDescriptor } from "../tools/types"; @@ -172,15 +172,14 @@ function createIdentityManager( ]); const skillService = new SkillService({ builtinSkills: { - ...BUILTIN_SKILL_BODIES, - [IDENTITY_SKILL_NAME]: [ + ...BUILTIN_SKILL_PACKAGES, + [IDENTITY_SKILL_NAME]: { entry: [ "---", `name: ${IDENTITY_SKILL_NAME}`, - "description: Identity fixture", - "when_to_use: Verify persisted child identity.", + "description: Identity fixture. Use to verify persisted child identity.", "---", IDENTITY_SKILL_BODY, - ].join("\n"), + ].join("\n"), resources: {} }, }, }); diff --git a/packages/agent-core/src/commands/skill.test.ts b/packages/agent-core/src/commands/skill.test.ts index 74dfa20c..547822ec 100644 --- a/packages/agent-core/src/commands/skill.test.ts +++ b/packages/agent-core/src/commands/skill.test.ts @@ -15,7 +15,10 @@ afterAll(async () => { const gitMasterBody = "FULL GIT MASTER BODY MUST NOT LEAK"; const builtinSkills = { - "git-master": `---\nname: git-master\ndescription: Git guidance.\nwhen_to_use: Use for git operations.\n---\n\n${gitMasterBody}`, + "git-master": { + entry: `---\nname: git-master\ndescription: Git guidance. Use for git operations.\n---\n\n${gitMasterBody}`, + resources: {}, + }, }; function createCommand(agentSkills: readonly string[] = ["git-master"]) { diff --git a/packages/agent-core/src/commands/skill.ts b/packages/agent-core/src/commands/skill.ts index 25dcb186..aca29305 100644 --- a/packages/agent-core/src/commands/skill.ts +++ b/packages/agent-core/src/commands/skill.ts @@ -32,7 +32,7 @@ export function createSkillCommand(): CommandDescriptor { } try { - const skill = await ctx.skillService.readForAgent(ctx.cwd, name, ctx.agentSkills); + const skill = await ctx.skillService.discoverForAgent(ctx.cwd, name, ctx.agentSkills); if (skill === null) { return unavailable(name, ctx.agentName); } diff --git a/packages/agent-core/src/prompt/compiler.test.ts b/packages/agent-core/src/prompt/compiler.test.ts index 50a580bd..8586a5ad 100644 --- a/packages/agent-core/src/prompt/compiler.test.ts +++ b/packages/agent-core/src/prompt/compiler.test.ts @@ -77,6 +77,51 @@ function contract(overrides: Partial = {}): PromptContractV2 { } describe("PromptContractCompiler", () => { + test("renders available Skill discovery as name, description, and source only", async () => { + const result = await new PromptContractCompiler().compile(contract({ + availableSkills: [{ + name: "codemap", + description: "Map an unfamiliar codebase when locating architecture or entry points.", + source: "builtin", + }], + })); + + expect(result.prompt).toContain( + "- codemap: Map an unfamiliar codebase when locating architecture or entry points. (source=builtin)", + ); + }); + + test("renders active Skill body and sorted resource descriptors without resource contents", async () => { + const result = await new PromptContractCompiler().compile(contract({ + activeSkills: [{ + metadata: { + name: "codemap", + description: "Map an unfamiliar codebase when locating architecture or entry points.", + }, + source: "project", + sourceLabel: "/workspace/.archcode/skills/codemap", + root: "/workspace/.archcode/skills/codemap", + resources: [ + { path: "references/z.md", bytes: 20 }, + { path: "references/a.md", bytes: 10 }, + ], + body: "ENTRY_BODY", + }], + })); + + expect(result.prompt).toContain( + "### codemap (source=/workspace/.archcode/skills/codemap; root=/workspace/.archcode/skills/codemap)", + ); + expect(result.prompt.indexOf("references/a.md")).toBeLessThan( + result.prompt.indexOf("references/z.md"), + ); + expect(result.prompt).toContain("ENTRY_BODY"); + expect(result.trace.skills.active).toEqual([{ + name: "codemap", + source: "/workspace/.archcode/skills/codemap", + }]); + }); + test("keeps Runtime and Current Context free of Session Goal state", async () => { const result = await new PromptContractCompiler().compile(contract()); diff --git a/packages/agent-core/src/prompt/compiler.ts b/packages/agent-core/src/prompt/compiler.ts index 2a4a10df..5b3a3278 100644 --- a/packages/agent-core/src/prompt/compiler.ts +++ b/packages/agent-core/src/prompt/compiler.ts @@ -49,7 +49,7 @@ export class PromptContractCompiler { sections: rendered.map(({ trace }) => trace), skills: { status: contract.availableSkills.length === 0 && contract.activeSkills.length === 0 ? "absent" : "present", - active: contract.activeSkills.map((skill) => ({ name: skill.metadata.name, source: skill.path ?? skill.source })), + active: contract.activeSkills.map((skill) => ({ name: skill.metadata.name, source: skill.sourceLabel })), }, visibleTools: [...contract.allowedTools], agentsMd: contract.agentsMd.status, @@ -66,7 +66,7 @@ export function createFailedPromptTrace( error: unknown, skills: PromptTrace["skills"] = { status: contract.availableSkills.length === 0 && contract.activeSkills.length === 0 ? "absent" : "present", - active: contract.activeSkills.map((skill) => ({ name: skill.metadata.name, source: skill.path ?? skill.source })), + active: contract.activeSkills.map((skill) => ({ name: skill.metadata.name, source: skill.sourceLabel })), }, ): PromptTrace { const message = error instanceof Error ? error.message : String(error); @@ -171,13 +171,21 @@ function renderCollaboration(contract: PromptContractV2): string { } function renderSkills(contract: PromptContractV2): string { - const available = contract.availableSkills.map((skill) => { - const allowedTools = skill.allowed_tools === undefined || skill.allowed_tools.length === 0 - ? "" - : ` [allowed_tools: ${skill.allowed_tools.join(", ")}]`; - return `- ${skill.name}: ${skill.description}${allowedTools} (source=${skill.source}; when=${skill.when_to_use})`; + const available = contract.availableSkills.map( + (skill) => `- ${skill.name}: ${skill.description} (source=${skill.source})`, + ); + const active = contract.activeSkills.map((skill) => { + const source = skill.root === undefined + ? `source=${skill.sourceLabel}` + : `source=${skill.sourceLabel}; root=${skill.root}`; + const resources = [...skill.resources] + .sort((a, b) => lexicalCompare(a.path, b.path)) + .map((resource) => `- ${resource.path} (${resource.bytes} bytes)`); + const resourceSection = resources.length === 0 + ? "Resources: none" + : `Resources:\n${resources.join("\n")}`; + return `### ${skill.metadata.name} (${source})\n\n${resourceSection}\n\n${skill.body}`; }); - const active = contract.activeSkills.map((skill) => `### ${skill.metadata.name} (source=${skill.path ?? skill.source})\n\n${skill.body}`); return `## Skills Skills provide optional workflow guidance. They never expand tools, runtime permissions, delegation targets, transitions, or completion authority. @@ -190,6 +198,10 @@ Active: ${active.length === 0 ? "- none" : active.join("\n\n---\n\n")}`; } +function lexicalCompare(a: string, b: string): number { + return a < b ? -1 : a > b ? 1 : 0; +} + function renderTools(contract: PromptContractV2): string { const mcp = Object.entries(contract.runtime.mcp).map(([name, status]) => `- ${name}: ${status}`); return `## Tool Visibility diff --git a/packages/agent-core/src/skills/builtin-standalone.integration.test.ts b/packages/agent-core/src/skills/builtin-standalone.integration.test.ts new file mode 100644 index 00000000..27684a16 --- /dev/null +++ b/packages/agent-core/src/skills/builtin-standalone.integration.test.ts @@ -0,0 +1,168 @@ +import { expect, setDefaultTimeout, test } from "bun:test"; +import { mkdtemp, rm } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +setDefaultTimeout(120_000); + +const repositoryRoot = join(import.meta.dir, "../../../.."); +const fixturePath = join(repositoryRoot, "apps/web/public/favicon.ico"); +const skillsEntrypoint = join(import.meta.dir, "index.ts"); +const skillReadModule = join(import.meta.dir, "../tools/builtins/skill-read.ts"); + +test("standalone binary preserves real builtin resources and arbitrary embedded bytes", async () => { + const tempRoot = await mkdtemp(join(tmpdir(), "archcode-skill-standalone-")); + try { + const sourceFixtureBytes = await Bun.file(fixturePath).bytes(); + const sourceFixtureDigest = sha256(sourceFixtureBytes); + expect(() => new TextDecoder("utf-8", { fatal: true }).decode(sourceFixtureBytes)).toThrow(); + + const entrypoint = join(tempRoot, "main.ts"); + const executable = join(tempRoot, "skill-smoke"); + await Bun.write(entrypoint, standaloneSource({ + fixturePath, + skillsEntrypoint, + skillReadModule, + })); + + const compiler = Bun.spawn([ + "bun", + "build", + entrypoint, + "--target=bun", + "--minify", + "--compile", + `--outfile=${executable}`, + ], { + cwd: repositoryRoot, + stdout: "pipe", + stderr: "pipe", + }); + const [compileExitCode, compileStdout, compileStderr] = await Promise.all([ + compiler.exited, + new Response(compiler.stdout).text(), + new Response(compiler.stderr).text(), + ]); + if (compileExitCode !== 0) { + throw new Error([ + `Standalone Skill smoke compilation exited ${compileExitCode}`, + compileStdout, + compileStderr, + ].filter(Boolean).join("\n")); + } + + const process = Bun.spawn([executable], { + cwd: tempRoot, + stdout: "pipe", + stderr: "pipe", + env: {}, + }); + const [exitCode, stdout, stderr] = await Promise.all([ + process.exited, + new Response(process.stdout).text(), + new Response(process.stderr).text(), + ]); + expect(exitCode).toBe(0); + expect(stderr).toBe(""); + + const result = JSON.parse(stdout) as StandaloneResult; + expect(result).toMatchObject({ + builtinSource: "builtin", + builtinResource: "references/evidence-map-example.md", + builtinTextFound: true, + fixtureBytes: sourceFixtureBytes.byteLength, + fixtureDigest: sourceFixtureDigest, + serviceDigest: sourceFixtureDigest, + unsupportedError: true, + unsupportedCode: "TOOL_SKILL_RESOURCE_BINARY_UNSUPPORTED", + }); + expect(result.unsupportedText).toBe([ + "---", + "skill: binary-fixture", + "source: builtin", + "resource: assets/favicon.ico", + `bytes: ${sourceFixtureBytes.byteLength}`, + "---", + "", + "error: TOOL_SKILL_RESOURCE_BINARY_UNSUPPORTED", + "hint: Binary Skill resources are valid package assets but cannot be returned by the text-only skill_read tool.", + ].join("\n")); + } finally { + await rm(tempRoot, { recursive: true, force: true }); + } +}); + +interface StandaloneResult { + readonly builtinSource: string; + readonly builtinResource: string; + readonly builtinTextFound: boolean; + readonly fixtureBytes: number; + readonly fixtureDigest: string; + readonly serviceDigest: string; + readonly unsupportedError: boolean; + readonly unsupportedCode?: string; + readonly unsupportedText: string; +} + +function standaloneSource(paths: { + readonly fixturePath: string; + readonly skillsEntrypoint: string; + readonly skillReadModule: string; +}): string { + return [ + `import embeddedFixturePath from ${JSON.stringify(paths.fixturePath)} with { type: "file" };`, + `import { BUILTIN_SKILL_PACKAGES, SkillService } from ${JSON.stringify(paths.skillsEntrypoint)};`, + `import { formatResolvedSkillResource } from ${JSON.stringify(paths.skillReadModule)};`, + "", + "const fixtureBytes = await Bun.file(embeddedFixturePath).bytes();", + "const fixtureDigest = new Bun.CryptoHasher(\"sha256\").update(fixtureBytes).digest(\"hex\");", + "const builtinSkills = {", + " ...BUILTIN_SKILL_PACKAGES,", + " \"binary-fixture\": {", + " entry: [", + " \"---\",", + " \"name: binary-fixture\",", + " \"description: Verifies arbitrary embedded bytes when standalone Skill packages are compiled.\",", + " \"---\",", + " \"\",", + " \"Read the binary fixture.\",", + " ].join(\"\\n\"),", + " resources: { \"assets/favicon.ico\": fixtureBytes },", + " },", + "};", + "const service = new SkillService({ userSkillsRoot: \"/definitely-missing-user-skills\", builtinSkills });", + "const realResource = await service.readResourceForAgent(", + " \"/definitely-missing-project\",", + " \"codemap\",", + " \"references/evidence-map-example.md\",", + " [\"codemap\"],", + ");", + "if (realResource === null) throw new Error(\"real builtin resource was not resolved\");", + "const binaryResource = await service.readResourceForAgent(", + " \"/definitely-missing-project\",", + " \"binary-fixture\",", + " \"assets/favicon.ico\",", + " [\"binary-fixture\"],", + ");", + "if (binaryResource === null) throw new Error(\"binary builtin resource was not resolved\");", + "const serviceDigest = new Bun.CryptoHasher(\"sha256\").update(binaryResource.content).digest(\"hex\");", + "const unsupported = formatResolvedSkillResource(binaryResource);", + "const unsupportedText = unsupported.draft.kind === \"text\" ? unsupported.draft.text : \"\";", + "console.log(JSON.stringify({", + " builtinSource: realResource.source,", + " builtinResource: realResource.resource.path,", + " builtinTextFound: new TextDecoder().decode(realResource.content).includes(\"Evidence map shape\"),", + " fixtureBytes: fixtureBytes.byteLength,", + " fixtureDigest,", + " serviceDigest,", + " unsupportedError: unsupported.isError,", + " unsupportedCode: unsupported.details?.error?.code,", + " unsupportedText,", + "}));", + "", + ].join("\n"); +} + +function sha256(bytes: Uint8Array): string { + return new Bun.CryptoHasher("sha256").update(bytes).digest("hex"); +} diff --git a/packages/agent-core/src/skills/builtin/analyze-work/SKILL.md b/packages/agent-core/src/skills/builtin/analyze-work/SKILL.md index 6f112f22..d085b383 100644 --- a/packages/agent-core/src/skills/builtin/analyze-work/SKILL.md +++ b/packages/agent-core/src/skills/builtin/analyze-work/SKILL.md @@ -1,12 +1,89 @@ --- name: analyze-work -description: Synthesize architecture, complex-debugging, and gap analysis into one evidence-backed recommendation. -when_to_use: Use for open design, difficult root-cause analysis, hidden requirements, tradeoffs, concurrency, migrations, or overdesign risk. +description: Synthesize architecture, difficult root-cause, or gap analysis into an evidence-backed recommendation when implementation direction is uncertain. +license: MIT +metadata: + archcode/source: "Superpowers systematic-debugging concepts" + archcode/source-commit: "44c9b2d6e889982ac18c27d05a19fefe335194e1" + archcode/adaptation: "idea-only rewrite" --- -- Establish the actual mechanism, ownership boundaries, invariants, and failure path before proposing a solution. -- Separate observed facts, falsified hypotheses, remaining unknowns, and inferences. -- Compare viable approaches by correctness, operational risk, complexity, reversibility, and product fit. -- Search for hidden intent, missing acceptance criteria, incompatible assumptions, and unnecessary machinery. -- Use Explore or Librarian only to fill specific evidence gaps, then synthesize their results yourself. -- Recommend one direction and state tradeoffs, unresolved risks, counterevidence, and the smallest decisive validation. +# Analyze Work + +Produce a decision-ready analysis before implementation. Choose one primary mode below; combine modes only when the question genuinely spans them. + +## Inputs + +Establish the smallest complete brief: + +- the question or failure to explain; +- the intended behavior, constraints, and observable success criteria; +- the relevant workspace, change range, runtime environment, or external contract; +- known evidence, attempted fixes, and explicit non-goals; +- the decision the Lead or user must make. + +If a missing product choice would change the analysis materially, identify it instead of silently choosing. Use an Explore or Librarian child only for a separable evidence gap, and verify and synthesize the returned evidence yourself. + +## Evidence Discipline + +1. Reconstruct the current mechanism from primary evidence: code, tests, configuration, logs, history, and authoritative documentation. +2. Trace ownership and data flow across relevant boundaries. Record where validation, state mutation, persistence, concurrency, permissions, retries, and user-visible behavior actually live. +3. Keep four categories distinct: + - **Observed:** directly supported by inspected evidence. + - **Inferred:** the best explanation derived from observations. + - **Falsified:** a plausible explanation contradicted by evidence. + - **Unknown:** information not available or not safely testable. +4. Cite evidence precisely enough to recheck: file and symbol or line, command and material output, event sequence, or authoritative source and version. +5. Prefer the smallest decisive observation over broad inventory. Do not turn directory listings, child reports, or passing unrelated checks into proof. + +For falsifiable diagnosis, boundary tracing, and deciding when to stop, read [references/diagnosis-method.md](references/diagnosis-method.md). + +## Mode A: Architecture or Design + +Use when choosing a mechanism, ownership boundary, migration, or concurrency model. + +1. State the problem as an invariant or observable outcome, not as a preferred implementation. +2. Map the current design: entry points, owners, state transitions, trust boundaries, failure handling, and existing extension points. +3. Extract hard constraints from current behavior, architecture tests, persisted formats, compatibility promises, and user decisions. Separate those from habits that may be changed. +4. Define at least one viable minimal direction. Add alternatives only when they expose a real tradeoff; do not manufacture options. +5. Compare directions on correctness, complexity, coupling, operational risk, migration cost, reversibility, testability, and consistency with the product model. +6. Stress the preferred direction with failure cases: partial success, restart or retry, stale state, concurrency, cancellation, authorization, and rollback as relevant. +7. Recommend one direction, identify what it deliberately does not build, and name the smallest experiment or test that would invalidate the recommendation. + +Reject designs that introduce a new service, state machine, identity, persistence layer, or abstraction without a demonstrated requirement that existing ownership cannot satisfy. + +## Mode B: Root-Cause Diagnosis + +Use for bugs, failures, regressions, and integration surprises. Define expected versus actual behavior, reproduce or inspect fresh evidence, trace the failing value or event across owners, then test one falsifiable hypothesis at a time. Classify the result as confirmed, probable, or unresolved; recommend a fix only after the causal mechanism is established. + +## Mode C: Gap Analysis + +Use when comparing a requirement, Plan, protocol, migration target, or acceptance contract with the current state. + +1. Normalize the target into individually decidable obligations. Preserve explicit non-goals and distinguish requirements from examples. +2. For each obligation, locate current implementation and verification evidence. +3. Classify each obligation as satisfied, partially satisfied, missing, contradicted, or unverifiable. Never count file presence, intent, or a child claim as behavioral completion. +4. Trace dependencies between gaps so root omissions are not reported repeatedly as downstream symptoms. +5. Distinguish a defect from an intentional scope boundary, stale documentation, or optional improvement. +6. Order the minimum closure sequence and pair every proposed step with observable acceptance evidence. + +## Severity and Confidence + +Use severity for consequence, not emphasis: Blocker prevents a safe outcome; Major is a material correctness, data, security, compatibility, or acceptance failure; Minor is bounded; Advisory is optional. State confidence separately. High impact with uncertain evidence is a risk to investigate, not a confirmed finding. + +## Stop Conditions + +Stop and report the limitation when the governing objective is missing, the relevant artifact cannot be identified, evidence access is unavailable, a destructive or state-changing diagnostic would require new authority, or a required product decision cannot be inferred. Do not fill the gap with speculation. + +## Output + +Return a concise natural-language report containing: + +1. mode, question, scope, and bottom-line recommendation or diagnosis; +2. current mechanism or target-versus-actual map; +3. observed facts, inferences, falsified hypotheses, and unknowns; +4. severity-ordered risks or gaps with exact evidence; +5. recommended direction, alternatives considered, and explicit non-goals; +6. smallest decisive validation and remaining decisions. + +Do not modify source. The analysis informs the Lead; it does not transfer implementation or completion authority. diff --git a/packages/agent-core/src/skills/builtin/analyze-work/references/diagnosis-method.md b/packages/agent-core/src/skills/builtin/analyze-work/references/diagnosis-method.md new file mode 100644 index 00000000..20d7089b --- /dev/null +++ b/packages/agent-core/src/skills/builtin/analyze-work/references/diagnosis-method.md @@ -0,0 +1,35 @@ +# Falsifiable diagnosis method + +Use this only when the task is diagnosis rather than broad design. + +## Investigation record + +Keep one compact record so evidence does not get replaced by the latest theory: + +| Field | Record | +| --- | --- | +| Expected / actual | One observable difference, not “it fails” | +| Reproduction | Exact input, environment, first bad result, and complete error | +| Working comparison | Nearest path that succeeds under comparable conditions | +| First divergent boundary | Owner, input, output, state/configuration, and timestamp/order | +| Current hypothesis | `X causes Y because Z` | +| Discriminating observation | Result that would confirm it and result that would falsify it | + +## Probe loop + +1. Reproduce or obtain fresh evidence before explaining the cause. If the failure is intermittent, record frequency and the smallest known precondition. +2. Compare the failing path with a nearby working path. Trace the wrong value, state, or event backwards from the first visible symptom; do not start at the component you already suspect. +3. At every crossed boundary, identify the owner and capture the value before and after validation, transformation, persistence, queueing, retry, or permission checks. +4. State one hypothesis in falsifiable form. Prefer a probe that observes existing state; if a state-changing probe is necessary, obtain authority and isolate its effect. +5. Change one variable or inspect one boundary. Record the result as confirmed, falsified, or inconclusive before forming the next hypothesis. +6. Reproduce the repaired path and the nearest working comparison. Then run the narrow regression check that would fail if the causal mechanism returned. + +## Claim quality + +- **Confirmed cause:** the mechanism explains the symptom, the decisive probe isolates it, and the proposed fix acts at that mechanism. +- **Probable cause:** evidence points to one mechanism but a decisive observation is unavailable; state confidence and the missing probe. +- **Unresolved:** multiple mechanisms remain viable or evidence conflicts; list the next observation that would distinguish them. + +“The error disappeared after several edits” is not causal proof. Neither is a stack trace that only shows where the failure surfaced, a child report without inspected evidence, or a configuration difference without a demonstrated path to the symptom. + +After three inconsistent hypotheses, stop patching. Recheck the architecture and assumptions and surface the impasse. diff --git a/packages/agent-core/src/skills/builtin/automation-create/SKILL.md b/packages/agent-core/src/skills/builtin/automation-create/SKILL.md index f211b4d4..e3cc1527 100644 --- a/packages/agent-core/src/skills/builtin/automation-create/SKILL.md +++ b/packages/agent-core/src/skills/builtin/automation-create/SKILL.md @@ -1,15 +1,47 @@ --- name: automation-create -description: Clarify a time-triggered Automation and obtain the user's response before creating it. -when_to_use: Use immediately when the user asks to create an Automation, or after the user accepts a suggestion for an explicit one-time or recurring time trigger. -allowed_tools: [automation_create, ask_user] +description: Clarify a requested time-triggered Automation and obtain explicit confirmation before creation, including when a user accepts a one-time or recurring scheduling suggestion. +license: MIT +metadata: + archcode/source: "ArchCode runtime schema" + archcode/source-commit: "f00efe7ab3cd87f951797d9b4bf14415f10abd7a" + archcode/adaptation: "original rewrite" --- -Turn the user's scheduling intent into one concise, committed Automation proposal. +Turn the user's scheduling intent into one typed, complete Automation proposal, then obtain explicit human confirmation before committing it. For trigger examples and timezone ambiguities, read [references/schedule-examples.md](references/schedule-examples.md). -- Ask only for information that is genuinely missing. Do not run a fixed questionnaire. -- Once the proposal is complete, use `ask_user` to present a complete summary: name, trigger, and action. For `start_session`, include the message and location (`project` or `worktree`); for `send_message`, include the target Session and message. Choose wording, options, and free-text availability that fit the conversation. -- After receiving and understanding the user's response, use your judgment: create with `automation_create` if the user accepts the proposed values; if the user requests changes, revise the proposal and obtain a response to the revised complete summary; if the user declines, do not create it. -- If the proposed values change materially before creation, present the revised complete summary and obtain a response before creating it. -- If any required field is missing, keep clarifying in this ordinary Session; do not create a partial Automation. -- If the user declines, continue helping in the ordinary Session. Do not create the Automation and do not repeat the suggestion for the same intent. +1. Ask only for information that is genuinely missing or ambiguous; do not run a + fixed questionnaire. Clarify the display name and exactly one schema-supported + trigger: + - `once`: an exact ISO 8601 date-time with an explicit UTC offset; + - `interval`: a recurring interval in milliseconds that meets the runtime minimum; + - `cron`: an exact five-field cron expression plus an IANA timezone. + Translate phrases such as “tomorrow morning”, “every weekday”, or “local time” + into concrete values. Do not silently assume a date, timezone, daylight-saving + behavior, or cron interpretation; surface the ambiguity and ask the smallest + question that resolves it. +2. Clarify exactly one action and its complete payload. For `start_session`, capture + the initial message and location (`project` or `worktree`). For `send_message`, + capture the target existing Session ID and message. Do not invent other action + kinds, fields, status values, origins, or scheduler behavior; the ArchCode schema + and runtime derive the remaining Automation state. +3. Check the normalized trigger and action before asking for approval: verify the + time and timezone are understandable, the cron has five fields and a valid IANA + timezone, the interval is not below the supported minimum, and the action has all + required fields. If a value changed during clarification, treat it as a new + proposal. +4. Once complete, use `ask_user` to present a final summary for inspection. Include + the name, the exact trigger (date/time and offset, interval, or cron plus timezone), + and the full action. Include the `start_session` message/location or the + `send_message` target Session/message. Choose wording, options, and free-text + availability that fit the conversation; do not create before this confirmation. +5. Interpret the response semantically. If it accepts the displayed values, call + `automation_create` with only the schema fields `name`, `trigger`, and `action`. + If it requests a change, revise the proposal and present the revised complete + summary through `ask_user` again. If it declines, do not create the Automation or + repeat the same suggestion. A materially changed value always requires a fresh + summary and response. +6. If any required field remains missing, keep clarifying in this ordinary Session; + never create a partial Automation. If schema validation or creation reports an + error, explain the concrete invalid value and revise it deliberately rather than + silently retrying a different schedule. diff --git a/packages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.md b/packages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.md new file mode 100644 index 00000000..28da6de2 --- /dev/null +++ b/packages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.md @@ -0,0 +1,27 @@ +# Scheduling clarification examples + +Use these as normalization examples, not values to copy without confirmation. + +| User wording | Missing decision | Valid normalized trigger | +| --- | --- | --- | +| “Tomorrow morning” | Exact date, local clock time, and UTC offset | `{ "kind": "once", "at": "2026-08-09T09:00:00+08:00" }` | +| “Every five minutes” | Whether a fixed elapsed interval is intended | `{ "kind": "interval", "everyMs": 300000 }` | +| “Weekdays at nine” | IANA timezone | `{ "kind": "cron", "expression": "0 9 * * 1-5", "timezone": "Asia/Shanghai" }` | + +An interval is an integer number of milliseconds and must satisfy the runtime minimum of 30,000 ms. Keep it as `interval`; translating it to cron changes elapsed-time semantics into calendar-time semantics. + +A cron expression has exactly five fields: minute, hour, day of month, month, and day of week. It always needs an IANA timezone even when the expression appears obvious. Do not substitute a UTC offset for a timezone because daylight-saving rules differ. + +## Action examples + +- New work in the existing project: `{ "kind": "start_session", "message": "Review the open incidents and summarize actionable regressions.", "location": "project" }` +- Isolated implementation work: `{ "kind": "start_session", "message": "Run the approved dependency upgrade Plan and verify it.", "location": "worktree" }` +- Continue one known Session: `{ "kind": "send_message", "sessionId": "", "message": "Recheck the failed CI run and continue only if the cause is confirmed." }` + +Do not use `send_message` without an exact existing Session UUID. Do not use `start_session` when the user intends to continue accumulated Session context. + +## Confirmation card + +Before creation, show the user the display name, normalized trigger, timezone or offset, action kind, complete message, and location or target Session. If any displayed value changes, show the complete card again; confirmation of an earlier card does not authorize a revised schedule. + +Daylight-saving behavior belongs to the chosen timezone. If a user names “local time” but the location is not known, ask; do not guess. diff --git a/packages/agent-core/src/skills/builtin/codemap/SKILL.md b/packages/agent-core/src/skills/builtin/codemap/SKILL.md index 16f5c0c5..751e9d3b 100644 --- a/packages/agent-core/src/skills/builtin/codemap/SKILL.md +++ b/packages/agent-core/src/skills/builtin/codemap/SKILL.md @@ -1,14 +1,48 @@ --- name: codemap -description: Build a concise map of code ownership, flows, and extension points. -when_to_use: Use before implementation when orientation matters - entering an unfamiliar module, understanding data flow, finding extension points, or planning where to make changes. +description: Build a concise evidence-backed map of code ownership, flows, and extension points when orienting in an unfamiliar module or planning a change. +license: MIT +metadata: + archcode/source: "OMO Slim codemap concepts" + archcode/source-commit: "ecb4f55e87c7cea9f18759eaca0eff8fb7edf1d0" + archcode/adaptation: "idea-only rewrite" --- -- Trace entry points: main exports, route handlers, CLI commands, or public API surfaces. -- Follow the primary data flow from input to output, noting key transformations and boundary crossings. -- Record which files own core behavior versus adapters, presentation, or configuration. -- Identify extension points: plugin hooks, strategy patterns, middleware chains, and callback registrations. -- Note circular dependencies or tight couplings that limit safe modification. -- Call out invariants, assumptions, and constraints a new contributor might miss. -- Prefer short, actionable maps tied to the specific task over broad directory listings. -- Update the map if exploration reveals structure diverges from initial assumptions. \ No newline at end of file +Build a slim, evidence-backed map for the task at hand. Map behavior and ownership, not every directory or an imagined architecture. Use [references/evidence-map-example.md](references/evidence-map-example.md) for the target output shape. + +1. **Set the boundary.** Name the module, feature, or question being mapped and the + caller who will use the result. Start with the smallest relevant search surface; + widen it only when the call path or ownership is still unclear. +2. **Trace entry points.** Find main exports, route handlers, CLI commands, event + consumers, scheduled jobs, or public API surfaces. Record the concrete file and + symbol (and line or command evidence when useful). If no true entry point exists, + say so rather than inventing one. +3. **Assign responsibilities.** Identify which files or modules own domain rules, + state and persistence, orchestration, adapters/integrations, presentation, and + configuration. Distinguish the source of truth from wrappers and re-exports. +4. **Follow the call chain.** Walk the primary path from entry point to observable + output, including important transforms, validation, error paths, event emission, + and persistence boundaries. Keep the order explicit and name the symbols that + make each hop. +5. **Describe the data flow.** State the input shape, normalization or validation, + state changes, messages/events, external calls, and final output. Note where data + crosses package, process, network, or storage boundaries. +6. **List integration points.** Include registries, middleware and hook chains, + plugin/strategy/callback extension points, providers, queues, files, databases, + APIs, and test seams. Note circular dependencies or tight couplings that make a + change risky. +7. **Record invariants and assumptions.** Call out schema and type contracts, + authorization and workspace boundaries, lifecycle/concurrency rules, required + ordering, compatibility constraints, and assumptions a new contributor could + otherwise break. Label an inference as an inference. +8. **Assess the impact surface.** Identify direct callers and consumers, transitive + effects, tests, configuration, migrations, API/UI contracts, and documentation + likely to change. Rank the surface by relevance to the requested change instead + of listing unrelated files. +9. **Update after probing.** If exploration disproves the initial map, revise it and + explain the correction. Stop when the relevant path, ownership, integrations, + invariants, and impact are sufficient for the task; leave concrete unknowns and + the next narrow probe when they are not. + +Tie each material claim to a file, symbol, test, or command. Keep the map short and +actionable; avoid broad directory dumps and unsupported speculation. diff --git a/packages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.md b/packages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.md new file mode 100644 index 00000000..b3a7bcbe --- /dev/null +++ b/packages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.md @@ -0,0 +1,40 @@ +# Evidence map shape + +Use the smallest map that makes the requested change or explanation safe. A useful map connects claims rather than listing directories. + +```markdown +## Scope +Question: Where is an incoming request admitted, persisted, and exposed to clients? +Excluded: unrelated page rendering and provider implementation. + +## Entry and ownership +- `routes/example.ts#handler` — transport validation and status mapping [source] +- `domain/service.ts#execute` — domain invariant and mutation owner [source, test] +- `store/repository.ts#save` — persistence boundary [source] + +## Primary flow +HTTP input + -> route schema + -> domain command + -> repository commit + -> event publication + -> client projection + +At each arrow record the concrete type/value, sync or async ordering, error path, +and whether the boundary mutates state. + +## Invariants and impact +- Invariant: only the domain service may create the durable record [architecture test]. +- Direct consumers: symbols and tests that call the changed owner [references]. +- Transitive impact: API/event types or persisted data read by other packages [evidence]. + +## Unknowns +- Unknown: whether retry can publish a duplicate event. +- Next probe: inspect idempotency key ownership and the retry integration test. +``` + +Evidence tags should resolve to a file plus symbol or tight line, a test whose assertion proves the claim, a command observation, or an authoritative external contract. “This folder seems responsible” and “tests pass” are not evidence locators. + +Prefer one primary happy path plus the material alternate paths: validation failure, authorization denial, partial persistence, retry/restart, and cancellation only when they affect the question. End the map when a reader can identify the owner to change, its callers, the invariants to preserve, and the next unresolved probe. + +For each material statement, attach a file and symbol, relevant test, or command observation. The map is complete when the requested change can be placed safely; it is not an inventory of the repository. diff --git a/packages/agent-core/src/skills/builtin/execute-plan/SKILL.md b/packages/agent-core/src/skills/builtin/execute-plan/SKILL.md index 9898604f..a24a92bd 100644 --- a/packages/agent-core/src/skills/builtin/execute-plan/SKILL.md +++ b/packages/agent-core/src/skills/builtin/execute-plan/SKILL.md @@ -1,14 +1,39 @@ --- name: execute-plan -description: Hand an existing Project Todo Plan into an ordinary Lead Session and let the user decide whether to create a Goal before execution. -when_to_use: Use for the first message of Todo Start Work when `.archcode/plans/.md` already exists. +description: Execute the existing Plan for Todo Start Work in an ordinary Lead Session, then let the user decide whether to create a Goal before implementation. +license: MIT +metadata: + archcode/source: "Superpowers executing-plans concepts" + archcode/source-commit: "44c9b2d6e889982ac18c27d05a19fefe335194e1" + archcode/adaptation: "idea-only rewrite" --- -1. Read the Plan at the exact path supplied by the Start Work request before taking any implementation action. -2. Check that the Plan is executable: its goal, scope, ordered steps, dependencies, acceptance criteria, validation, risks, and unresolved decisions must be concrete. Ask the user to resolve any critical gap before proceeding. -3. Draft a Goal objective from the Plan's goal and observable acceptance criteria. Do not add the Plan path, content, summary, hash, version, or other Plan linkage to Goal state. -4. Use `ask_user` to explicitly ask whether to create that Goal. Do not infer consent and do not create it silently. -5. If the user agrees, call the existing `create_goal` with only the agreed objective. Continue under the existing `run-goal` protocol, including its verification and independent final Review. -6. If the user declines, continue implementing the Plan as an ordinary Lead Session without creating a Goal. -7. Treat the Plan as ordinary Markdown, not execution state. Do not add a Plan service, status, version, lock, snapshot, watcher, Goal link, or second Review flow. -8. During an active Goal, mention Plan changes only when the user explicitly says in this current Lead Session that the Plan changed or asks to change it. Then explain that the current Goal continues under its established objective and acceptance criteria unless the user separately changes the Goal. Never claim automatic Plan detection, synchronization, or restart. +## Load and authorize + +1. Read the Plan at the exact path supplied by the Start Work request before taking any implementation action. Do not search for or substitute another Plan. +2. Compare it with the current repository state. Confirm that its goal, scope, ordered steps, dependencies, interfaces, acceptance criteria, validation, risks, and unresolved decisions are still executable. +3. Stop before implementation when a critical instruction is ambiguous, an assumption is stale, a required dependency is missing, the Plan conflicts with current code or user intent, or the evidence cannot decide a material product choice. Explain the concrete gap and ask one focused question; do not guess through it. +4. Draft a Goal objective from the Plan's goal and observable acceptance criteria. Do not put the Plan path, content, summary, hash, version, or other Plan linkage into Goal state. +5. Use `ask_user` to explicitly ask whether to create that Goal. Create it with only the agreed objective when the user clearly agrees; if the user declines, continue as an ordinary Lead Session. Do not infer consent or delay this choice until after implementation starts. + +## Execute in reviewable increments + +For a compact checkpoint card at each Plan boundary, read [references/execution-checkpoints.md](references/execution-checkpoints.md). + +1. Convert the ordered Plan into the current execution sequence. Respect prerequisites and use `orchestrate-work` for any bounded delegation; parallelize only Plan steps that remain independent in the current repository. +2. For each deliverable, inspect the baseline, make the smallest coherent change, and run the specified narrow verification. Read the full result before advancing. +3. At each dependency boundary, reconcile child work and recheck interfaces, callers, shared state, and the current diff. A child report or an isolated passing test does not establish integrated correctness. +4. If implementation reveals a minor local detail that preserves the Plan's objective and scope, record the deviation in the final report and continue. If it changes architecture, scope, acceptance, dependency order, safety, or user-visible behavior, stop and obtain a decision before proceeding. +5. If verification fails, diagnose the failure instead of marking the step complete. Stop and ask when repeated attempts do not produce new evidence, an external prerequisite is required, or the Plan must be reconsidered. + +## Completion gate + +After implementation: + +1. Re-read the Plan and check every acceptance criterion and non-goal against the final attributable changes. +2. Run fresh, proportionate verification for the integrated result, including repository-required broader checks. Do not infer build success from lint, behavioral correctness from typecheck, or overall completion from one passing subsystem. +3. Inspect complete output and exit status. Claim only what that evidence proves; identify skipped or unavailable checks and their impact. +4. If a Goal was created, finish only through `run-goal`, including terminal children, independent fresh `goal-review`, remediation, and `update_goal`. Do not create a second Review flow for the Plan. +5. Otherwise, report the delivered outcome, material Plan deviations, files or behavior changed, exact verification results, and residual risk as an ordinary Lead Session. + +Treat the Plan as ordinary Markdown, not execution state. Do not add a Plan service, status, version, lock, snapshot, watcher, Goal link, or completion mirror. During an active Goal, discuss Plan changes only when the user explicitly reports or requests them in this Lead Session; the established Goal objective and acceptance criteria continue unless the user separately changes the Goal. Never claim automatic Plan detection, synchronization, or restart. diff --git a/packages/agent-core/src/skills/builtin/execute-plan/references/execution-checkpoints.md b/packages/agent-core/src/skills/builtin/execute-plan/references/execution-checkpoints.md new file mode 100644 index 00000000..26399d55 --- /dev/null +++ b/packages/agent-core/src/skills/builtin/execute-plan/references/execution-checkpoints.md @@ -0,0 +1,24 @@ +# Execution checkpoint card + +Use one card per dependency boundary, not one per trivial edit. + +```markdown +### Checkpoint: +- Plan obligation: +- Preconditions verified: +- Files/symbols and owner: +- Observable change: +- Invariants/non-goals preserved: +- Narrow verification and expected signal: +- Result: supported / partial / failed +- Diff or interface impact on later steps: +- Decision: continue / remediate / revise Plan / ask user +``` + +Before beginning, verify that the prerequisite and referenced code still exist, the intended owner remains correct, and the step does not depend on an unresolved product choice. Name the failure that the narrow verification must detect; “run tests” is not a decisive expectation. + +After implementation, inspect the attributable diff and complete verification output. Re-read callers, types, persisted shapes, or event order at the changed seam. If the produced interface differs from the Plan, update the remaining execution sequence only when the difference is local and objective-preserving; otherwise stop for a Plan or user decision. + +At integration boundaries, read the current diff and recheck callers, shared state, and produced interfaces. A child report and an isolated passing test are leads, not integrated proof. + +Do not advance when a test passed for the wrong path, an expected assertion was never exercised, generated output is missing, or a later step now relies on an interface that was not produced. Record the smallest remediation and repeat the same checkpoint with fresh evidence. diff --git a/packages/agent-core/src/skills/builtin/git-master/SKILL.md b/packages/agent-core/src/skills/builtin/git-master/SKILL.md index 47a52fce..75c51446 100644 --- a/packages/agent-core/src/skills/builtin/git-master/SKILL.md +++ b/packages/agent-core/src/skills/builtin/git-master/SKILL.md @@ -1,16 +1,77 @@ --- name: git-master -description: Plan and execute safe git operations with reviewable history. -when_to_use: Use for commits, rebases, branch management, blame, bisect, cherry-pick, PR preparation, and any git history operation. +description: Plan and execute safe Git operations with reviewable history when inspecting, committing, rebasing, managing branches, or preparing a pull request. +license: MIT +metadata: + archcode/source: "Superpowers Git and worktree concepts" + archcode/source-commit: "44c9b2d6e889982ac18c27d05a19fefe335194e1" + archcode/adaptation: "idea-only rewrite" --- -- Inspect `git status`, `git diff`, and recent `git log` before changing history or committing. -- Keep commits atomic and focused: one logical change per commit, with a clear message matching repo style. -- Stage only intended files; never include secrets, generated artifacts, or unrelated changes. -- Prefer `git rebase` for local cleanup before push, but avoid rebasing shared branches. -- Use `git log --oneline`, `git blame`, and `git bisect` for history investigation over manual guessing. -- For PRs, verify base branch, squash or rebase strategy, and diff from base before pushing. -- When undoing changes, prefer `git stash` or `git revert` over `git reset --hard` unless you explicitly intend to discard. -- Prompt the user before force-push, branch deletion, or any operation that rewrites shared history. -- Run relevant tests or checks after branch operations to catch regressions early. -- Document risky operations briefly: what was done, which refs moved, and how to undo. \ No newline at end of file +# Git Master + +Use Git as an evidence source first and a mutation tool only when the user has authorized the requested effect. Preserve unrelated work, repository conventions, and recoverability. + +For operation-selection and recovery reminders, read [references/operation-safety.md](references/operation-safety.md). + +## Choose the operation + +Classify the request before running commands. Do not mix modes unless the task requires it. + +- **Inspect:** explain working-tree, branch, upstream, or divergence state. +- **Commit:** create one or more reviewable commits from the intended changes. +- **History:** locate when, where, or why a change was introduced with log, blame, or bisect. +- **Rebase or cherry-pick:** deliberately move commits while preserving the intended patch set. +- **Branch or PR preparation:** verify the base, final diff, checks, and publication readiness. + +If the requested end state is unclear, inspect first and ask before choosing a history-changing operation. + +## Establish ground truth + +Before any Git write: + +1. Read `git_status`, the relevant unstaged and staged `git_diff`, and recent history. +2. Identify the current branch, upstream, intended base, and whether the checkout is a linked or externally managed worktree when that affects the operation. +3. Separate task changes from pre-existing or unrelated changes. Never discard, overwrite, stage, or hide someone else's work to make the tree look clean. +4. Check repository instructions and recent commit messages before choosing message style or integration strategy. + +## Create commits + +1. Group changes by logical responsibility, not by file type or convenience. A commit should be understandable and reversible on its own. +2. Keep inseparable code, tests, schema changes, and documentation together; split independent changes. +3. Stage explicit intended paths. Do not use broad staging when unrelated or unreviewed files are present. +4. Re-read the staged diff. Check for secrets, generated artifacts, debug output, accidental formatting churn, and missing tests. +5. Run verification proportionate to the staged change. Do not claim a commit is verified from an older run against a different tree. +6. Write a message that follows the repository's observed convention and states the change rather than the activity. +7. After committing, inspect status and report the commit identifier, subject, verification, and any intentionally uncommitted files. + +Never amend an existing commit unless the user requested an amend or the current workflow explicitly authorizes it. + +## Investigate history + +- Use `git log` with path, symbol, author, date, or content filters to narrow the search before reading large history ranges. +- Use `git blame` to identify the introducing commit, then inspect that commit and its surrounding history; do not treat the author line alone as an explanation. +- Use `git bisect` only when there is a reproducible good/bad predicate and the range endpoints are known. Record the result and return the repository to its original state afterward. +- Distinguish evidence from inference: cite the relevant commit, patch, or line history and explain what it does and does not prove. + +## Rewrite or move history + +1. Confirm the exact commits, destination, ordering, and expected final graph before rebase, cherry-pick, or reset-like work. +2. Do not rebase, amend, force-push, delete a branch, or rewrite a shared or published ref without explicit user authorization. +3. Before resolving a conflict, understand both sides and preserve the combined intended behavior. Never choose ours/theirs mechanically. +4. After the operation, compare the new range with the original intent, run relevant checks, and inspect status and log. +5. If the remote moved or a push is rejected, stop and investigate. Do not use force as a retry strategy. + +## Prepare a branch or PR + +- Confirm the real base branch; do not assume it is `main`. +- Review the complete committed base-to-HEAD diff. Separately inspect `git_status`, both staged and unstaged `git_diff` views, and the contents of relevant untracked files; none of that uncommitted work appears in the base-to-HEAD commit range. +- Run the required checks on the exact tree being proposed. +- Push, create a PR, merge, delete branches, or clean worktrees only when the user requested that external or destructive effect. +- Preserve a worktree needed for review follow-up unless its owner explicitly authorizes cleanup. + +## Stop conditions + +Stop and report before acting when authorization is missing, the base or target ref is uncertain, unrelated changes overlap the operation, a conflict's intended resolution is unclear, verification fails, or the operation would make recovery materially harder than the user requested. + +Finish with the resulting branch/ref state, commits created or moved, verification evidence, remaining local changes, and any action still awaiting authorization. diff --git a/packages/agent-core/src/skills/builtin/git-master/references/operation-safety.md b/packages/agent-core/src/skills/builtin/git-master/references/operation-safety.md new file mode 100644 index 00000000..18ef1b29 --- /dev/null +++ b/packages/agent-core/src/skills/builtin/git-master/references/operation-safety.md @@ -0,0 +1,22 @@ +# Git operation safety card + +Inspect before mutation: status, staged and unstaged diff, relevant history, current branch, upstream, intended base, and worktree context. Classify the operation as inspect, commit, history, rebase/cherry-pick, or branch/PR preparation before choosing commands. + +| Operation | Required ground truth | Stop when | +| --- | --- | --- | +| Commit | intended files, staged/unstaged split, repository message rules | unrelated changes overlap or commit grouping is ambiguous | +| History investigation | exact symbol/path/question and relevant date or branch range | rename/move makes the initial path incomplete; expand deliberately | +| Rebase/cherry-pick | source commits, target/base, upstream state, dirty worktree | target history or conflict policy is not authorized | +| Branch/PR preparation | base-to-HEAD committed diff, worktree state, checks, upstream | publication, push, base, or destructive cleanup was not requested | + +For a commit, stage explicit intended paths, inspect the staged diff, and verify that no required new file is omitted. The staged patch—not the working-tree summary—is the proposed commit. Split only when changes have independent intent and remain buildable/reviewable; do not split coupled production and regression-test changes for appearance. + +For history rewriting, first identify a recovery reference and confirm whether commits may already be shared. Preserve conflicts for inspection, resolve them from the intended combined behavior, then inspect the rewritten patch set rather than trusting command success. + +For branch or PR readiness, review the base-to-HEAD committed diff. Separately inspect `git_status`, staged and unstaged diffs, and relevant untracked files; none of those are included in base-to-HEAD history. + +## Mutation report + +After a write, report the exact effect: commit IDs or branch changed, files included, verification run, remaining dirty state, and whether anything was pushed or published. A local commit is not a push, and a pushed branch is not a merged change. + +Stop for a decision when the desired history, target branch, conflict behavior, commit grouping, or external publication is ambiguous. Never use cleanup or destructive history rewriting to hide another person's changes. diff --git a/packages/agent-core/src/skills/builtin/goal-review/SKILL.md b/packages/agent-core/src/skills/builtin/goal-review/SKILL.md index c17dbe65..c82c76eb 100644 --- a/packages/agent-core/src/skills/builtin/goal-review/SKILL.md +++ b/packages/agent-core/src/skills/builtin/goal-review/SKILL.md @@ -1,12 +1,119 @@ --- name: goal-review -description: Perform an independent evidence-based final review before a Goal completes. -when_to_use: Use in a fresh direct deep Analyst Session reviewing the current Goal. +description: Perform an independent evidence-based final review in a fresh direct deep Analyst Session before a Goal completes. +license: MIT +metadata: + archcode/source: "ArchCode Goal review protocol" + archcode/source-commit: "f00efe7ab3cd87f951797d9b4bf14415f10abd7a" + archcode/adaptation: "original rewrite" --- -- Independently compare the complete Goal objective and acceptance criteria with the final attributable changes and verification evidence. -- Inspect the actual repository and runtime evidence. Use read-only Explore or Librarian children only for separable evidence questions. -- Clearly identify material correctness, safety, scope, verification, or acceptance gaps. Do not describe the Goal as complete when required work remains. -- Do not modify source, control Goal state, or claim that this Skill itself grants completion authority. -- Give the Lead a normal natural-language report with a clear conclusion, severity-ordered findings, evidence checked, verification gaps, and residual risk. -- Use the language and structure that best communicates the result. The Lead interprets the full report and decides whether remediation or Goal completion is appropriate. +# Goal Review + +Perform a fresh, independent, read-only review of the final Goal result. Produce an ordinary natural-language evidence report for the Lead. This Skill does not own Goal state and does not define a Runtime-parsed approval token or verdict protocol. + +## Required Inputs + +Establish from the current review brief and repository evidence: + +- the complete Goal objective and all observable acceptance criteria; +- explicit constraints, non-goals, and user decisions; +- the final attributable changes and baseline; +- the implementation and remediation history only where it explains current risk; +- fresh verification evidence for the final state; +- known limitations, deferred work, and operational or migration requirements. + +Do not reconstruct a missing Goal objective from a Plan, diff, or earlier summary. If the authoritative objective or final change surface is unavailable, stop and explain why an independent completion assessment is not possible. + +## Independence Rules + +- Inspect primary evidence yourself: current code, diff, tests, configuration, persisted formats, logs, and authoritative documentation as relevant. +- Treat Lead, Build, and prior reviewer summaries as pointers, not proof. +- Use read-only Explore or Librarian children only for separable evidence questions, then check and synthesize their evidence yourself. +- Do not modify source, propose opportunistic refactors, resume implementation, or soften a finding to help the Goal finish. +- Review the current final state. A report issued before later remediation is stale and cannot cover that remediation. + +## Review Method + +### 1. Normalize the Goal contract + +Translate the objective into a checklist of individually observable obligations. Preserve scope boundaries. Identify acceptance criteria that are ambiguous or not decidable and state the missing decision rather than inventing one. + +### 2. Build an evidence matrix + +Use [references/evidence-matrix-example.md](references/evidence-matrix-example.md) as a compact readable model; it is not a machine verdict. + +For every obligation, record: + +- the implementation or artifact that satisfies it; +- the exact verification evidence and freshness; +- relevant negative or edge cases; +- status as supported, partially supported, unsupported, contradicted, or unverifiable. + +File existence, code plausibility, an old test run, or an agent's completion claim is not sufficient evidence of behavior. + +### 3. Inspect the final change end to end + +Trace affected behavior across relevant callers and boundaries. Check: + +- correctness and preservation of existing behavior; +- persistence, migrations, restart, retry, and rollback or recovery; +- concurrency, cancellation, partial failure, cleanup, and idempotency; +- permissions, trust boundaries, secret handling, validation, and destructive actions; +- public types, compatibility, configuration, error mapping, and user-visible states; +- scope discipline: required work is present and unrelated machinery was not introduced. + +### 4. Challenge the verification + +Map each completion claim to the command, observation, or artifact that proves it. Confirm that evidence was produced against the final attributable state, inspect full material output and exit status, and identify what the check cannot prove. Prefer fresh targeted regression evidence plus proportionate broader checks. + +When a test was added for a bug, look for evidence that it detects the original failure rather than merely passing with the current code. When a check could not be run, report that as a verification gap; do not infer success. + +### 5. Verify candidate findings + +Before reporting a defect, identify the violated Goal obligation or invariant, cite exact evidence, trace a concrete failure path and impact, and check whether another layer prevents it. Separate confirmed defects from questions and optional improvements. + +## Severity + +- **Blocker:** evidence shows the Goal's core outcome is not achieved or proceeding would create severe security, data, or operational harm. +- **Major:** a material correctness, acceptance, security, compatibility, persistence, or migration gap remains. +- **Minor:** a bounded defect or meaningful verification weakness that does not defeat the core outcome. +- **Advisory:** optional cleanup or future hardening outside the completion decision. + +Severity is about consequence and likelihood, not implementation effort. State uncertainty separately. An unverified high-impact possibility is a verification gap, not automatically a confirmed Blocker. + +## Completion Assessment + +Use ordinary prose to explain whether the inspected evidence supports the Goal objective or whether material gaps remain. The assessment must follow the evidence matrix: + +- any confirmed Blocker or Major gap means required remediation remains; +- an unverifiable required criterion means the report cannot support a confident completion conclusion; +- Minor or Advisory items may remain only when they do not contradict the objective or agreed acceptance criteria, and they must be disclosed; +- no findings does not mean perfect software; describe the reviewed scope and residual risk. + +Do not emit `PASS`, `FAIL`, a magic prefix, JSON verdict, score, or any other machine-oriented completion signal. The Lead reads the entire report, resolves material gaps, performs its own verification, and decides whether Goal completion is appropriate. + +## Stop Conditions + +Return an unable-to-conclude report instead of guessing when: + +- the Goal objective, acceptance criteria, baseline, or final changes are missing or inconsistent; +- primary evidence cannot be accessed; +- required verification needs mutation, credentials, environment access, or authority not available to the Analyst; +- a product decision is required to decide whether an obligation is satisfied; +- the final state changes during review. + +Name the missing evidence and the smallest step needed for a fresh review. + +## Output + +Write a natural-language report with: + +1. a short completion assessment and the exact scope reviewed; +2. the criterion-by-criterion evidence matrix or equivalent readable mapping; +3. findings in severity order, each with requirement, evidence, failure path, impact, and smallest remediation or proof needed; +4. verification commands or observations checked, material results, freshness, and limitations; +5. missing evidence, unresolved questions, and residual risk; +6. a clear statement of what the evidence supports and what remains for the Lead to decide. + +Do not call or recommend a Goal state mutation as if it were the reviewer's action. This report is independent evidence, not completion authority. diff --git a/packages/agent-core/src/skills/builtin/goal-review/references/evidence-matrix-example.md b/packages/agent-core/src/skills/builtin/goal-review/references/evidence-matrix-example.md new file mode 100644 index 00000000..4684f190 --- /dev/null +++ b/packages/agent-core/src/skills/builtin/goal-review/references/evidence-matrix-example.md @@ -0,0 +1,22 @@ +# Evidence matrix example + +| Obligation | Current artifact | Fresh evidence | Edge case | Status | +| --- | --- | --- | --- | --- | +| Resource is bounded | package reader limit | focused test exits 0 and asserts equal/above | exact limit and one byte above | supported | +| Winning source is atomic | resolver implementation | precedence and missing-resource tests | invalid high-priority package | supported | +| Compiled artifact contains resources | static manifest | standalone binary byte comparison | non-UTF-8 asset | supported | +| Existing user packages still load | no migration exists | none | old single-file shape | unsupported, intentional breaking change | + +For every row, identify the governing acceptance text before looking for evidence. “Current artifact” locates the implementation; it is not proof. “Fresh evidence” names the exact command, inspection, or runtime observation and material result. The edge case should be capable of falsifying the claim. + +Use these evidence categories consistently: + +- **supported:** fresh evidence covers the complete obligation and material edge path; +- **partially supported:** only part of the obligation or one layer is proven; +- **unsupported:** the obligation is not implemented or evidence demonstrates failure; +- **contradicted:** implementation or behavior conflicts with the obligation; +- **unverifiable:** required evidence is inaccessible or would require authority the Reviewer lacks. + +After building the matrix, trace cross-row risks that a per-file review misses: source selection into activation, activation into Prompt/tool output, persistence into restart, or implementation into compiled delivery. Verification performed before the final material change is stale. + +Use `supported`, `partially supported`, `unsupported`, `contradicted`, or `unverifiable` as prose evidence categories. The Lead, not this table, decides Goal status. diff --git a/packages/agent-core/src/skills/builtin/manifest.test.ts b/packages/agent-core/src/skills/builtin/manifest.test.ts new file mode 100644 index 00000000..38e241e7 --- /dev/null +++ b/packages/agent-core/src/skills/builtin/manifest.test.ts @@ -0,0 +1,81 @@ +import { describe, expect, test } from "bun:test"; +import { readdir } from "node:fs/promises"; +import { join } from "node:path"; +import { activateBuiltinSkill } from "../package-reader"; +import { SkillService } from "../service"; +import { BUILTIN_SKILL_PACKAGES } from "./manifest"; + +const builtinRoot = import.meta.dir; + +describe("builtin Skill package manifest", () => { + test("declares exactly every builtin package directory", async () => { + const sourceNames = (await readdir(builtinRoot, { withFileTypes: true })) + .filter((entry) => entry.isDirectory()) + .map((entry) => entry.name) + .sort(); + const manifestNames = Object.keys(BUILTIN_SKILL_PACKAGES).sort(); + + expect(sourceNames).toHaveLength(14); + expect(manifestNames).toEqual(sourceNames); + }); + + test("declares every non-entry source file once with no embedded extras", async () => { + for (const [name, skillPackage] of Object.entries(BUILTIN_SKILL_PACKAGES)) { + const sourceFiles = await listRelativeFiles(join(builtinRoot, name)); + expect(Object.keys(skillPackage.resources).sort()).toEqual( + sourceFiles.filter((path) => path !== "SKILL.md"), + ); + + const activated = activateBuiltinSkill(skillPackage, name); + expect(activated.metadata.name).toBe(name); + expect(activated.resources.map((resource) => resource.path)).toEqual( + Object.keys(skillPackage.resources).sort(), + ); + } + }); + + test("reads a real multi-file builtin through the ordinary SkillService path", async () => { + const service = new SkillService({ + userSkillsRoot: join(builtinRoot, "__definitely_missing_user_skills__"), + }); + + const entry = await service.readForAgent(builtinRoot, "codemap", ["codemap"]); + expect(entry?.source).toBe("builtin"); + expect(entry?.resources).toContainEqual({ + path: "references/evidence-map-example.md", + bytes: expect.any(Number), + }); + expect(entry?.body).not.toContain("## Evidence Map Example"); + + const resource = await service.readResourceForAgent( + builtinRoot, + "codemap", + "references/evidence-map-example.md", + ["codemap"], + ); + expect(resource?.source).toBe("builtin"); + expect(new TextDecoder().decode(resource?.content)).toContain("Evidence map shape"); + }); +}); + +async function listRelativeFiles(root: string): Promise { + const files: string[] = []; + + async function walk(directory: string, prefix: readonly string[]): Promise { + const entries = await readdir(directory, { withFileTypes: true }); + for (const entry of entries) { + const path = [...prefix, entry.name]; + if (entry.isSymbolicLink()) throw new Error(`Builtin source contains a symlink: ${path.join("/")}`); + if (entry.isDirectory()) { + await walk(join(directory, entry.name), path); + } else if (entry.isFile()) { + files.push(path.join("/")); + } else { + throw new Error(`Builtin source contains a non-file entry: ${path.join("/")}`); + } + } + } + + await walk(root, []); + return files.sort(); +} diff --git a/packages/agent-core/src/skills/builtin/manifest.ts b/packages/agent-core/src/skills/builtin/manifest.ts index e02e270b..4a72f488 100644 --- a/packages/agent-core/src/skills/builtin/manifest.ts +++ b/packages/agent-core/src/skills/builtin/manifest.ts @@ -1,33 +1,80 @@ -import gitMaster from "./git-master/SKILL.md" with { type: "text" }; -import safeRefactor from "./safe-refactor/SKILL.md" with { type: "text" }; -import codemap from "./codemap/SKILL.md" with { type: "text" }; -import reviewWork from "./review-work/SKILL.md" with { type: "text" }; -import researchDocs from "./research-docs/SKILL.md" with { type: "text" }; +import analyzeWork from "./analyze-work/SKILL.md" with { type: "text" }; +import analyzeWorkDiagnosis from "./analyze-work/references/diagnosis-method.md" with { type: "text" }; import automationCreate from "./automation-create/SKILL.md" with { type: "text" }; +import automationScheduleExamples from "./automation-create/references/schedule-examples.md" with { type: "text" }; +import codemap from "./codemap/SKILL.md" with { type: "text" }; +import codemapEvidenceMap from "./codemap/references/evidence-map-example.md" with { type: "text" }; +import executePlan from "./execute-plan/SKILL.md" with { type: "text" }; +import executePlanCheckpoints from "./execute-plan/references/execution-checkpoints.md" with { type: "text" }; +import gitMaster from "./git-master/SKILL.md" with { type: "text" }; +import gitMasterSafety from "./git-master/references/operation-safety.md" with { type: "text" }; +import goalReview from "./goal-review/SKILL.md" with { type: "text" }; +import goalReviewMatrix from "./goal-review/references/evidence-matrix-example.md" with { type: "text" }; import orchestrateWork from "./orchestrate-work/SKILL.md" with { type: "text" }; +import orchestrateDelegationPacket from "./orchestrate-work/references/delegation-packet.md" with { type: "text" }; import planWork from "./plan-work/SKILL.md" with { type: "text" }; -import executePlan from "./execute-plan/SKILL.md" with { type: "text" }; +import planWorkTemplate from "./plan-work/assets/plan-template.md" with { type: "text" }; +import researchDocs from "./research-docs/SKILL.md" with { type: "text" }; +import researchSourceEvaluation from "./research-docs/references/source-evaluation.md" with { type: "text" }; +import reviewChange from "./review-change/SKILL.md" with { type: "text" }; +import reviewChangeLenses from "./review-change/references/review-lenses.md" with { type: "text" }; +import reviewWork from "./review-work/SKILL.md" with { type: "text" }; +import reviewWorkPacket from "./review-work/references/review-packet.md" with { type: "text" }; import runGoal from "./run-goal/SKILL.md" with { type: "text" }; +import safeRefactor from "./safe-refactor/SKILL.md" with { type: "text" }; +import safeRefactorVerification from "./safe-refactor/references/boundary-verification.md" with { type: "text" }; import shapeTodo from "./shape-todo/SKILL.md" with { type: "text" }; -import goalReview from "./goal-review/SKILL.md" with { type: "text" }; -import analyzeWork from "./analyze-work/SKILL.md" with { type: "text" }; -import reviewChange from "./review-change/SKILL.md" with { type: "text" }; +import shapeTodoTemplate from "./shape-todo/references/todo-shaping-template.md" with { type: "text" }; +import type { BuiltinSkillPackage } from "../types"; + +export const BUILTIN_SKILL_PACKAGES = { + "analyze-work": packageOf(analyzeWork, { + "references/diagnosis-method.md": analyzeWorkDiagnosis, + }), + "automation-create": packageOf(automationCreate, { + "references/schedule-examples.md": automationScheduleExamples, + }), + codemap: packageOf(codemap, { + "references/evidence-map-example.md": codemapEvidenceMap, + }), + "execute-plan": packageOf(executePlan, { + "references/execution-checkpoints.md": executePlanCheckpoints, + }), + "git-master": packageOf(gitMaster, { + "references/operation-safety.md": gitMasterSafety, + }), + "goal-review": packageOf(goalReview, { + "references/evidence-matrix-example.md": goalReviewMatrix, + }), + "orchestrate-work": packageOf(orchestrateWork, { + "references/delegation-packet.md": orchestrateDelegationPacket, + }), + "plan-work": packageOf(planWork, { + "assets/plan-template.md": planWorkTemplate, + }), + "research-docs": packageOf(researchDocs, { + "references/source-evaluation.md": researchSourceEvaluation, + }), + "review-change": packageOf(reviewChange, { + "references/review-lenses.md": reviewChangeLenses, + }), + "review-work": packageOf(reviewWork, { + "references/review-packet.md": reviewWorkPacket, + }), + "run-goal": packageOf(runGoal), + "safe-refactor": packageOf(safeRefactor, { + "references/boundary-verification.md": safeRefactorVerification, + }), + "shape-todo": packageOf(shapeTodo, { + "references/todo-shaping-template.md": shapeTodoTemplate, + }), +} as const satisfies Readonly>; -export const BUILTIN_SKILL_BODIES = { - "git-master": gitMaster, - "safe-refactor": safeRefactor, - codemap, - "review-work": reviewWork, - "research-docs": researchDocs, - "automation-create": automationCreate, - "orchestrate-work": orchestrateWork, - "plan-work": planWork, - "execute-plan": executePlan, - "run-goal": runGoal, - "shape-todo": shapeTodo, - "goal-review": goalReview, - "analyze-work": analyzeWork, - "review-change": reviewChange, -} as const; +export type BuiltinSkillName = keyof typeof BUILTIN_SKILL_PACKAGES; -export type BuiltinSkillName = keyof typeof BUILTIN_SKILL_BODIES; +function packageOf( + entry: string, + resources: Readonly> = {}, +): BuiltinSkillPackage { + return Object.freeze({ entry, resources: Object.freeze(resources) }); +} diff --git a/packages/agent-core/src/skills/builtin/orchestrate-work/SKILL.md b/packages/agent-core/src/skills/builtin/orchestrate-work/SKILL.md index 4bec0e55..2342a2e8 100644 --- a/packages/agent-core/src/skills/builtin/orchestrate-work/SKILL.md +++ b/packages/agent-core/src/skills/builtin/orchestrate-work/SKILL.md @@ -1,15 +1,45 @@ --- name: orchestrate-work -description: Route ordinary Lead work between direct execution and bounded child collaboration while retaining technical ownership. -when_to_use: Runtime activates this for an ordinary root Lead Session. +description: Route ordinary root Lead work between direct execution and bounded child collaboration while retaining technical ownership. +license: MIT +metadata: + archcode/source: "ArchCode delegation protocol" + archcode/source-commit: "f00efe7ab3cd87f951797d9b4bf14415f10abd7a" + archcode/adaptation: "original rewrite" --- -- Start from the user's outcome and acceptance evidence, not from a desire to create children. -- Work directly when the change is simple, tightly coupled, on the critical path, or already fully understood. -- Delegate only a separable result: Analyst for deep reasoning or independent criticism, Build for a bounded implementation, Explore for local evidence, Librarian for external evidence. -- Choose `fast` for low-risk known-pattern work and `deep` for ambiguous, cross-domain, security-sensitive, concurrent, migration, or otherwise difficult work. Analyst is always `deep`; Explore and Librarian are always `fast`. -- Parallelize only tasks known to be independent. Keep overlapping files, shared state, and dependency chains serial. -- Give each child a complete objective and the minimum useful Skills. Do not invent path ownership or shift final responsibility into the delegation. -- Integrate child evidence, resolve conflicts, verify the final state, and deliver the result yourself. -- Before creating a Goal, use the ordinary `ask_user` tool to ask whether to create it. Choose the wording and language that best fit the conversation. -- Interpret the user's answer semantically. Call `create_goal` when the answer clearly means yes; otherwise continue ordinarily or clarify as needed. +## Decide the execution shape + +1. Restate the requested outcome, constraints, and evidence that would prove it. Inspect the current state enough to separate known facts from assumptions before choosing an execution shape. +2. Work directly when the change is small, tightly coupled, on the critical path, or already understood. Delegation has coordination cost; do not create children merely because they are available. +3. Delegate only a separable result: + - Analyst for deep reasoning, gap analysis, or independent criticism; + - Build for one bounded implementation outcome; + - Explore for one local-code evidence question; + - Librarian for one current external-evidence question. +4. Choose `fast` for low-risk, known-pattern Build work and `deep` for ambiguous, cross-domain, security-sensitive, concurrent, migration, or otherwise difficult Build work. Analyst is always `deep`; Explore and Librarian are always `fast`. + +## Delegate safely + +Use [references/delegation-packet.md](references/delegation-packet.md) when preparing a child brief or an integration gate. + +- Partition by independently understandable problem domain or deliverable, not by arbitrary file count. Run children in parallel only when they have no shared mutable state, overlapping changes, or dependency order. Keep related failures, common root-cause investigations, and integration-sensitive work together or serial. +- Give every child a self-contained objective containing the relevant evidence, exact scope, constraints, acceptance conditions, downstream decision, and expected report. Supply only the Skills needed for that result. +- Do not invent path ownership, leases, or new collaboration state. A bounded Build scope reduces interference but does not create Runtime ownership. The Lead remains responsible for all integration and completion claims. +- If new evidence shows that supposedly independent tasks overlap, stop parallel mutation, reconcile the shared design, and continue serially. + +## Integrate and verify + +1. Read each child report critically; a child success claim is not proof. Inspect the attributable diff or evidence and resolve contradictions before accepting it. +2. Integrate in dependency order. Recheck callers, interfaces, shared state, and user-visible behavior after combining results; individually valid changes can still conflict. +3. Run the narrowest decisive checks for each changed behavior, then the broader project checks required by the repository and the combined risk. Read complete output and exit status. +4. Before claiming success, compare the final state against every requested outcome and acceptance condition. Report only what fresh evidence establishes, plus unverified areas and residual risk. + +## Stop conditions + +- Stop and ask for a focused decision when scope, product intent, destructive authority, or an external prerequisite cannot be inferred safely. +- Stop delegating and investigate directly when failures appear related or the full system state is required. +- Do not call work complete while material verification fails, a required child is still non-terminal, or an unresolved finding can change correctness. +- Report a concrete blocker instead of guessing or repeatedly applying the same failed approach. + +Before creating a Goal, use ordinary `ask_user` to ask whether to create it. Interpret the answer semantically: call `create_goal` only when the user clearly agrees; otherwise continue ordinarily or clarify. Goal creation does not change any of the orchestration and verification duties above. diff --git a/packages/agent-core/src/skills/builtin/orchestrate-work/references/delegation-packet.md b/packages/agent-core/src/skills/builtin/orchestrate-work/references/delegation-packet.md new file mode 100644 index 00000000..f8f6ae3f --- /dev/null +++ b/packages/agent-core/src/skills/builtin/orchestrate-work/references/delegation-packet.md @@ -0,0 +1,29 @@ +# Delegation packet and integration gate + +Use a packet when a child can produce a bounded result that the Lead can independently inspect. + +```markdown +Title: +Outcome: +Evidence already known: +Scope: ; explicitly excluded: +Constraints: +Acceptance: +Downstream use: +Return: +``` + +Give an Explore or Librarian a question and evidence target, not the conclusion to confirm. Give a Build exact change ownership and integration constraints, while reminding it that other work may coexist. Give an Analyst the governing contract and attributable surface without feeding it the desired verdict. + +Do not delegate a product decision, Goal completion, final integration, or a tiny task whose explanation costs more than direct work. Do not split by arbitrary file count when files share one invariant or interface. + +On return, inspect the attributable diff or evidence, resolve contradictions, integrate in dependency order, recheck callers and shared state, and run fresh checks against the combined result. + +## Integration gate + +- Confirm the child stayed inside scope and did not silently change the contract. +- Read changed code or primary evidence; do not integrate from the summary alone. +- Resolve overlaps against the current worktree rather than reverting another contributor. +- Recheck the produced interface at every downstream consumer. +- Run narrow checks for the contribution, then combined checks for shared boundaries. +- Keep ownership with Lead until the final report and any Goal transition are complete. diff --git a/packages/agent-core/src/skills/builtin/plan-work/SKILL.md b/packages/agent-core/src/skills/builtin/plan-work/SKILL.md index d4e05b58..9c997a0b 100644 --- a/packages/agent-core/src/skills/builtin/plan-work/SKILL.md +++ b/packages/agent-core/src/skills/builtin/plan-work/SKILL.md @@ -1,21 +1,54 @@ --- name: plan-work -description: Research, create, or improve the one evidence-backed Markdown implementation Plan for a task or bound Project Todo. -when_to_use: Use when uncertainty, sequencing, or risk makes a durable Plan useful, or when the user asks for a plan. +description: Research, create, or improve the evidence-backed Markdown implementation Plan for a task or bound Project Todo when uncertainty, sequencing, risk, or a user request warrants one. +license: MIT +metadata: + archcode/source: "Superpowers writing-plans concepts" + archcode/source-commit: "44c9b2d6e889982ac18c27d05a19fefe335194e1" + archcode/adaptation: "idea-only rewrite" --- -1. First decide from the bound Todo and current request whether the intended outcome and scope are concrete enough to choose relevant evidence. If not, ask one focused clarification without inspecting the workspace, probing the Plan, or calling another tool. Once sufficient, inspect enough code, evidence, and constraints to make the Plan executable. Before finalizing, ask the user about every unresolved choice that affects the objective, scope, implementation, dependencies, acceptance criteria, or validation. -2. In a Todo Discussion, derive the Todo ID only from the current Session binding and use exactly the runtime-provided `todoPlanPath`. Use `todoPlanState` to decide whether to read the existing file or create it; never accept a different path or Todo ID from the request, and never scan the workspace to discover Plan existence. -3. Outside a Todo Discussion, use the safe direct child path under `.archcode/plans/` named by the current task unless the user explicitly requested another deliverable path. -4. In a Todo Discussion, when `todoPlanState=present`, read the exact target file before changing it; when `todoPlanState=absent`, create that exact file without probing its parent directory. Outside a Todo Discussion, retain read-before-edit for an existing target. Preserve confirmed user content and improve it; never create a second Plan. -5. Write ordinary Markdown containing all seven content classes: - - goal and background; - - scope and non-goals; - - ordered implementation steps; - - dependencies and required sequence; - - acceptance criteria; - - validation methods; - - risks and items requiring confirmation. -6. Make every acceptance criterion observable and decidable, and state how it will be judged. Resolve unknowns before finalizing; never use vague criteria such as "mostly complete", "handle appropriately", or "as needed". -7. Do not create Plan IDs, sidecar metadata, status, versions, approvals, locks, snapshots, services, APIs, Goal links, file watchers, or progress mirrors. -8. In a Discussion, stop after shaping the Todo and Plan; do not begin product code implementation. In an ordinary Lead Session, a Plan remains guidance rather than a new workflow state machine. +## Establish the planning target + +1. In a Todo Discussion, derive identity only from the current Session binding. Use exactly the runtime-provided `todoPlanPath` and `todoPlanState`; never accept another Todo ID or path and never scan the workspace to discover Plan existence. +2. Start from the bound Todo or current request, then inspect the smallest relevant implementation, tests, conventions, constraints, and existing Plan identified by authoritative context. Use Explore or Librarian only for separable evidence questions. Do not ask the user for facts available from the request, repository, Plan, or tool output. +3. After investigation, ask one focused question only when an unresolved product or scope choice materially affects the objective, dependencies, acceptance, or validation and evidence cannot decide it. Do not infer the user's preference. +4. Outside a Todo Discussion, use a safe direct child of `.archcode/plans/` named for the current task unless the user explicitly requests another deliverable path. +5. When the target exists, read it before editing or producing an improved draft and preserve confirmed user content. When a Discussion reports `todoPlanState=absent`, create only the exact supplied path when the current Agent has write authority. Maintain one Plan, never a replacement or parallel variant. + +Lead and Discussion may write the Plan through their available file tools. Analyst is source-read-only: when this Skill is delegated to an Analyst, produce a complete evidence-backed Plan draft for the parent Lead, do not create or edit the Plan file, and do not claim it was saved. + +## Design executable work + +- Start with the claim the implementation must establish. Identify likely failure modes and choose the smallest credible evidence for each acceptance condition before writing implementation steps. +- Map the relevant files, symbols, interfaces, and existing tests. State what each affected unit is responsible for and follow established architecture rather than planning an unrelated cleanup. +- Divide work into ordered, independently checkable deliverables. Each step must state the concrete change, relevant location, prerequisites or produced interface, and verification. Split only at real dependency or review boundaries; keep tightly coupled changes together. +- Mark which steps may run independently and why. Never prescribe parallel mutation where files, contracts, state, fixtures, or validation resources overlap. +- Prefer the smallest root-cause change. Include migrations, compatibility, cleanup, rollback, documentation, or observability only when the requested outcome actually needs them. + +## Required Plan content + +Use [assets/plan-template.md](assets/plan-template.md) as the complete structure, adapting it to the task rather than retaining placeholders. + +Produce ordinary Markdown with these seven content classes: + +1. goal and evidence-backed background; +2. scope and explicit non-goals; +3. ordered implementation steps with concrete files or symbols where known; +4. dependencies, produced/consumed interfaces, and safe parallel boundaries; +5. observable acceptance criteria; +6. validation methods, expected signals, and relevant failure cases; +7. risks, assumptions, and decisions still requiring confirmation. + +Use enough implementation detail that another Lead or Build can act without rediscovering the design, but do not paste large speculative code blocks. Never leave placeholders such as “TBD,” “handle appropriately,” “add tests,” or “as needed.” For each acceptance criterion, name how it will be judged and distinguish decisive evidence from weaker supporting checks. + +## Self-review and handoff + +Before finalizing: + +1. Trace every requirement and non-goal to at least one step or explicit exclusion. +2. Check that step order, paths, symbols, and produced/consumed interfaces agree across the Plan. +3. Check that each material behavior and failure mode has proportionate verification and that the stated commands or inspections actually exist in this repository. +4. Remove unjustified machinery and resolve all execution-blocking unknowns. If a material choice remains open, ask the user and revise the Plan before calling it executable. + +Do not create Plan IDs, sidecar metadata, status, versions, approvals, locks, snapshots, services, APIs, Goal links, file watchers, or progress mirrors. In a Discussion, stop after shaping the Todo and Plan and report the saved path, key decisions, remaining risks, and readiness for a new ordinary Lead Session. In an Analyst Session, return the proposed Plan and evidence to the parent without claiming a file mutation. Do not begin product implementation. In an ordinary Lead Session, the Plan remains guidance rather than workflow state. diff --git a/packages/agent-core/src/skills/builtin/plan-work/assets/plan-template.md b/packages/agent-core/src/skills/builtin/plan-work/assets/plan-template.md new file mode 100644 index 00000000..f2248de9 --- /dev/null +++ b/packages/agent-core/src/skills/builtin/plan-work/assets/plan-template.md @@ -0,0 +1,55 @@ +# + +## Goal and background + +State the intended observable result, why it matters, and the evidence-backed current mechanism. Link the governing Todo or request when one exists; do not invent a Plan ID or runtime state. + +## Scope and non-goals + +State the ownership boundary, user-visible behavior, data or interfaces that change, and explicit exclusions. Distinguish a deliberate non-goal from deferred required work. + +## Current mechanism and constraints + +- Entry points and current owner: +- Data/state/event flow: +- Invariants and architecture boundaries: +- Repository conventions and required checks: +- External/version constraints: +- Confirmed user decisions: + +Separate verified facts from assumptions. Mark any assumption that would change the design if false. + +## Chosen direction + +Explain the smallest coherent design, where each responsibility will live, and why it fits existing ownership. Record rejected alternatives only when they expose a real tradeoff. State what this direction deliberately does not build. + +## Ordered implementation + +For each step include: + +1. **Deliverable:** observable result of this step. +2. **Location and owner:** relevant files/symbols and the responsibility being changed. +3. **Concrete change:** types, control flow, state, errors, tests, docs, or cleanup. +4. **Prerequisite / produced interface:** what must exist before and what later steps consume. +5. **Failure and edge paths:** only those material to this boundary. +6. **Decisive verification:** command or inspection, expected signal, and what it does not prove. + +## Dependencies and parallel boundaries + +State ordering constraints and only genuinely independent work. Shared mutable state, the same interface, overlapping files, or one step consuming another step's output makes work sequential unless an explicit seam removes that dependency. + +## Acceptance and validation + +| Acceptance condition | Evidence / command | Expected result | Failure or edge case | +| --- | --- | --- | --- | +| | | | | + +Include repository-required typecheck, unit, integration, architecture, and build lanes only when relevant; do not claim behavioral proof from typecheck alone. + +## Risks and decisions + +Record likelihood/impact, control, rollback or recovery boundary, and the user decision required. If no decision is required, say why evidence already determines the direction. + +## Completion report requirements + +Name the artifacts, behavior changes, Plan deviations, verification results, skipped checks, and residual risks the executor must report. Completion means every acceptance row is supported or an explicit unresolved item is returned to the user; file presence or implementation intent is not enough. diff --git a/packages/agent-core/src/skills/builtin/research-docs/SKILL.md b/packages/agent-core/src/skills/builtin/research-docs/SKILL.md index 7b3f6545..972e72d4 100644 --- a/packages/agent-core/src/skills/builtin/research-docs/SKILL.md +++ b/packages/agent-core/src/skills/builtin/research-docs/SKILL.md @@ -1,14 +1,49 @@ --- name: research-docs -description: Research external documentation and turn it into implementation guidance. -when_to_use: Use when uncertain about library, API, or platform behavior - when integrating unfamiliar packages, or when designing against an external specification. +description: Research external documentation into implementation guidance when library, API, or platform behavior is uncertain during integration or external-spec design. +license: MIT +metadata: + archcode/source: "ArchCode documentation research method" + archcode/source-commit: "f00efe7ab3cd87f951797d9b4bf14415f10abd7a" + archcode/adaptation: "original rewrite" --- -- Start with official documentation; fall back to reputable OSS examples only when docs are incomplete. -- Extract exact APIs, parameter types, default values, and version-sensitive behavior. -- Note which version of the library or platform the documentation describes; flag version mismatches. -- Separate confirmed facts from assumptions; label inferences explicitly for the consumer. -- When multiple sources conflict, prefer the official source and note the discrepancy. -- Convert findings into concise, ordered steps the implementing agent can follow immediately. -- Include copy-ready code snippets for non-obvious API usage; avoid paraphrasing working examples. -- Record open questions or areas where documentation was insufficient; do not silently guess. \ No newline at end of file +Use a bounded, source-first investigation to answer one implementation question. The +deliverable is concise guidance with traceable links, not a pasted documentation dump. + +For source ranking, version conflicts, direct-link requirements, and stopping criteria, read [references/source-evaluation.md](references/source-evaluation.md). + +1. Define the question, target package/platform, project language, and the behavior + the implementation must support. Read the repository manifest and lockfile when + available to establish the exact dependency/runtime version; if it is unavailable, + state that limitation. +2. Start with the **official** API reference, specification, migration/release notes, + and repository examples for that version. Prefer a direct page or stable source + URL over a search-result page. Use reputable OSS examples only to fill a gap after + official material has been checked, and label them as secondary evidence. +3. Extract only the facts needed to implement: API names and signatures, parameter + types, defaults, lifecycle/order rules, errors, limits, compatibility, and + version-sensitive behavior. Record the source title, direct link, and documented + version for every material claim. +4. Keep confirmed facts, local observations, and inferences separate. Mark an + inference explicitly and explain what evidence would verify it. Do not silently + turn an example or a remembered default into a project fact. +5. When sources conflict, first check whether they describe different versions or + platforms. Prefer the official documentation for the project's exact version; + if official sources still disagree, report the discrepancy, use a minimal local + compile/test/probe when safe, and leave the result as unresolved rather than + guessing. +6. Convert the result into short, ordered implementation steps with the relevant + constraints and validation checks. Include one minimal, copy-ready example only + when it clarifies a non-obvious API shape; adapt it to the project's language and + do not require large copied passages or full source listings. +7. End with open questions, version mismatches, inaccessible sources, or risks that + could change the recommendation. Link the official pages inline so the consumer + can verify them without repeating the search. + +Stop when the target version, required API shape, defaults/constraints, and a safe +validation path are established and no blocking unknown remains. If sources are +unavailable, contradictory, or require unbounded investigation, stop after a small +number of reasonable attempts, report the blocker and evidence gathered, and let the +implementing agent decide whether a local experiment is authorized. Never hide an +unsupported guess behind confident prose. diff --git a/packages/agent-core/src/skills/builtin/research-docs/references/source-evaluation.md b/packages/agent-core/src/skills/builtin/research-docs/references/source-evaluation.md new file mode 100644 index 00000000..ad1f4d72 --- /dev/null +++ b/packages/agent-core/src/skills/builtin/research-docs/references/source-evaluation.md @@ -0,0 +1,34 @@ +# Source evaluation and stopping rules + +Rank evidence: exact-version official reference or specification first; official release notes and repository examples next; reputable open-source usage only to fill an official gap. Link directly to the source page, not a result page, for every material claim. + +## Evidence record + +For each material implementation claim, capture: + +| Field | Content | +| --- | --- | +| Question | One behavior, default, constraint, or API shape | +| Project version/platform | Lockfile or manifest evidence, or explicit unknown | +| Source | Direct official page/repository link and documented version | +| Fact | Narrow paraphrase supported by that source | +| Local implication | File/interface/validation affected in this repository | +| Confidence / gap | Confirmed, version-matched, inferred, or unresolved | +| Verification | Minimal compile, test, request, or runtime observation | + +Do not cite a landing page for a claim found only on a nested API page. Search results, snippets, generated summaries, and third-party examples are discovery aids, not primary evidence. + +When sources conflict, first identify version or platform differences. Prefer the official source matching the project's installed version. If that remains contradictory, state it and propose one bounded local probe when safe; do not guess. + +Example conflict handling: + +1. Docs for the latest release show a new option, but the lockfile pins an older release. +2. Check the pinned release reference and release notes for the introduction version. +3. If the pinned source lacks the option, report it as unavailable rather than copying the latest example. +4. When types and prose disagree at the same version, inspect the official implementation or run a minimal compile/probe and label the result as local observation. + +## Minimal example quality + +A copy-ready example contains only the imports, inputs, call, result/error handling, and lifecycle ordering needed to demonstrate the non-obvious contract. Remove unrelated setup. Mark placeholders and never include secrets. State the version and what the example does not establish. + +Stop when version, required API shape, constraints/defaults, and a validation path are known. Stop earlier with an explicit limitation after a small reasonable investigation if the source is unavailable or the question is unbounded. diff --git a/packages/agent-core/src/skills/builtin/review-change/SKILL.md b/packages/agent-core/src/skills/builtin/review-change/SKILL.md index ffac5a79..43719d34 100644 --- a/packages/agent-core/src/skills/builtin/review-change/SKILL.md +++ b/packages/agent-core/src/skills/builtin/review-change/SKILL.md @@ -1,13 +1,100 @@ --- name: review-change -description: Review a Plan or implementation for correctness, completeness, safety, and verifiability. -when_to_use: Use in an Analyst Session for plan review, code review, security review, or a combined independent review. +description: Independently review a Plan or implementation for correctness, completeness, safety, and verifiability in an Analyst Session. +license: MIT +metadata: + archcode/source: "Superpowers review and verification concepts" + archcode/source-commit: "44c9b2d6e889982ac18c27d05a19fefe335194e1" + archcode/adaptation: "idea-only rewrite" --- -- Reconstruct the governing objective and constraints before examining the proposed or completed work. -- Trace changed behavior through callers, boundaries, persistence, concurrency, permissions, error paths, and user-visible consequences as relevant. -- For a Plan, test whether each step is executable, ordered, evidence-backed, and paired with acceptance verification. -- For code, inspect the attributable diff and run or assess proportionate tests and diagnostics without modifying source. -- For security-sensitive work, examine trust boundaries, validation, secret handling, authorization, injection, destructive behavior, and auditability. -- Report actionable findings in severity order with exact evidence. Distinguish blocking defects from residual risk and optional improvements. -- When `goal-review` is active, provide the independent Goal report described by that Skill. +# Review Change + +Perform an independent, read-only review of a defined Plan or implementation. The purpose is to find material problems and explain them with reproducible evidence, not to approve by tone or reward complexity. + +For lens prompts, finding quality, severity/confidence, and unable-to-conclude examples, read [references/review-lenses.md](references/review-lenses.md). + +## Required Inputs + +Before reviewing, identify: + +- the governing objective, acceptance criteria, constraints, and non-goals; +- the exact Plan, diff, commits, files, or other attributable change surface; +- the baseline against which the change is judged; +- verification already run, including commands, environment, and results; +- known risks or questions the requester wants challenged. + +If the change surface or governing requirement is ambiguous, stop and request a precise boundary. Do not review the entire repository as a substitute. + +## Review Procedure + +### 1. Reconstruct the contract + +Restate what must remain true before reading the proposed solution in detail. Locate primary evidence for the requirement and current behavior. Separate explicit acceptance criteria from assumptions and optional improvements. + +### 2. Establish the attributable change + +Inspect the actual diff or Plan, not only a summary. Check for unstated generated files, migrations, configuration, schema changes, or call sites needed to make the change work. Preserve unrelated workspace changes outside the review conclusion. + +### 3. Trace affected behavior + +Follow each material path from entry to observable outcome. Inspect, as relevant: + +- callers, consumers, public types, and compatibility boundaries; +- validation, permissions, trust boundaries, secret handling, and destructive actions; +- persistence, migrations, restart and retry behavior; +- concurrency, cancellation, partial failure, cleanup, and idempotency; +- error mapping, logging, auditability, and user-visible states; +- tests that prove the changed behavior and protect the failure mode. + +Use nearby working implementations and architecture constraints as comparison evidence. A stylistic preference is not a defect. + +### 4. Apply the appropriate lens + +Choose only relevant Plan, code, and security lenses. Trace every claimed problem from requirement through concrete failure path and check whether another layer already prevents it. + +### 5. Verify each candidate finding + +Before reporting a finding: + +1. state the violated requirement or invariant; +2. cite the exact code, Plan text, runtime evidence, or missing verification; +3. trace a concrete input or state to the harmful outcome; +4. check whether another layer already prevents it; +5. state the smallest correction or acceptance test, without designing the implementation unnecessarily. + +If the issue cannot survive this check, omit it or label it as an open question. Validate external or child feedback against the repository; do not forward it blindly. + +### 6. Assess verification + +Match evidence to claims. A typecheck does not prove runtime behavior; a unit test does not prove an integration boundary; a passing test added after the fix does not by itself prove it detects the regression. Run safe, proportionate read-only checks when useful. Otherwise state exactly what was not run and why. + +## Severity + +Use Blocker for unsafe or objective-defeating work, Major for a material reproducible defect, Minor for a bounded defect or meaningful verification weakness, and Advisory for optional future work. Severity reflects consequence and likelihood, not fix size; state uncertainty separately. + +## Stop Conditions + +Stop and return an “unable to conclude” report when the objective, baseline, or attributable change cannot be established; required evidence is inaccessible; or verification would require mutation or authority the Analyst does not have. Name the missing evidence and the smallest next step. + +Do not modify source, resolve findings, or claim delivery completion. When `goal-review` is active, follow that Skill's final-Goal evidence method and output requirements rather than turning this into a second verdict format. + +## Output + +Lead with the findings, ordered by severity. Each actionable finding must include: + +- severity and short title; +- violated requirement or invariant; +- exact evidence location; +- concrete failure scenario and impact; +- smallest acceptable correction or verification. + +Then include: + +- review scope and baseline; +- checks run and material results; +- open questions or verification gaps; +- residual risks and advisory observations; +- a plain-language overall assessment. + +If there are no actionable findings, say so directly but still state scope, evidence checked, checks not run, and residual risk. “No findings” means none were supported by the reviewed evidence, not that the change is proven perfect. diff --git a/packages/agent-core/src/skills/builtin/review-change/references/review-lenses.md b/packages/agent-core/src/skills/builtin/review-change/references/review-lenses.md new file mode 100644 index 00000000..6ee5ba0d --- /dev/null +++ b/packages/agent-core/src/skills/builtin/review-change/references/review-lenses.md @@ -0,0 +1,36 @@ +# Review lenses and finding quality + +Choose only the lenses the change actually crosses. + +## Plan lens + +- Does every step name an owner/location, concrete change, prerequisite, produced interface, and decisive check? +- Are acceptance conditions observable and mapped to evidence, including a falsifying edge case? +- Are migration, cleanup, rollback, documentation, and operational steps included only when required? +- Does the ordering match dependencies, and are claimed parallel boundaries genuinely independent? +- Does the Plan introduce a service, state machine, compatibility path, or abstraction without a demonstrated need? + +## Code lens + +- Trace changed behavior from entry through validation, owner, side effect/persistence, error mapping, and consumer. +- Check direct and transitive callers, public types, restart/retry/cancellation, partial failure, and cleanup as relevant. +- Confirm the fix acts on the causal mechanism and removes obsolete paths when a hard cut was required. +- Read the regression test: would it fail on the old defect, and does it exercise the production path rather than a duplicate implementation? + +## Security lens + +Name the protected asset, attacker-controlled input, trust boundary, enforcement owner, and concrete exploit path. Check canonicalization order, symlinks/path traversal, authorization timing, secret exposure, command construction, external effects, and fail-open behavior only when present in scope. A generic “could be insecure” concern is not a finding. + +An actionable finding states the violated invariant, exact evidence, concrete failure scenario, impact, and smallest adequate correction or missing proof. If the objective, baseline, attributable surface, or necessary read-only evidence is missing, report `unable to conclude` with the smallest next step instead of guessing. + +```text +Major — Source precedence can cross the workspace boundary +Invariant: project resources must remain below the project Skill root. +Evidence: resolver accepts the lexical path before checking an ancestor symlink. +Scenario: .archcode/skills points outside; reading demo loads external content as project. +Correction: validate the trusted-root ancestry before discovery and add a regression. +``` + +By contrast, “rename this helper”, “add comments”, or “use my preferred abstraction” is Advisory at most unless it produces a concrete correctness, maintenance, or acceptance failure. If another layer blocks the alleged path, omit the finding and cite that prevention in the review notes. + +`Unable to conclude` should name the missing contract or evidence, why it is necessary, and the smallest safe action that would make the review decidable. It is not a euphemism for approval. diff --git a/packages/agent-core/src/skills/builtin/review-work/SKILL.md b/packages/agent-core/src/skills/builtin/review-work/SKILL.md index 9b06f7f7..891e7da7 100644 --- a/packages/agent-core/src/skills/builtin/review-work/SKILL.md +++ b/packages/agent-core/src/skills/builtin/review-work/SKILL.md @@ -1,15 +1,91 @@ --- name: review-work -description: Let Lead assemble evidence, choose proportionate independent review, and drive fix-review closure. -when_to_use: Use when completed work needs independent criticism or a review and remediation loop before delivery. +description: Let Lead assemble evidence, choose proportionate independent review, and drive fix-review closure when completed work needs criticism before delivery. +license: MIT +metadata: + archcode/source: "Superpowers review and verification concepts" + archcode/source-commit: "44c9b2d6e889982ac18c27d05a19fefe335194e1" + archcode/adaptation: "idea-only rewrite" --- -1. Reconstruct the exact objective, constraints, acceptance criteria, attributable changes, and verification already run. -2. Decide whether direct Lead verification is sufficient or an independent Analyst is justified by risk, ambiguity, or separation of responsibility. -3. Default to one Analyst. Activate all review methods needed for this review in that one child; add another Analyst only for a genuinely independent viewpoint. -4. Give the Analyst evidence and questions, not a desired verdict. Never treat a child claim as proof without checking the referenced evidence. -5. Resolve findings yourself. Fix material gaps directly or through a bounded Build, then run proportionate verification again. -6. For an ordinary review, resume or replace the Analyst according to context contamination and independence needs. Do not create Review state. -7. For Goal completion, follow `run-goal` and use a fresh Analyst with `deep` plus `goal-review`; the Lead interprets that independent report before deciding whether to complete. +# Review Work -Report what was reviewed, material findings and fixes, verification evidence, and residual risk. +Lead owns the review outcome: define what is being reviewed, obtain proportionate independent criticism, verify every material finding, drive remediation, and report the final evidence. A review is a workflow step, not new persisted Review state. + +For a review-packet checklist and remediation-loop detail, read [references/review-packet.md](references/review-packet.md). Do not duplicate the Analyst's `review-change` method. + +## Inputs + +Resolve the objective and exact attributable change boundary before choosing a reviewer. Review cannot compensate for an undefined contract. + +## Choose the Review Depth + +Direct Lead verification may be sufficient for a small, obvious, low-risk change with narrow impact and decisive checks. Use an independent Analyst when any of these apply: + +- behavior spans components, persistence, concurrency, permissions, security, migrations, or compatibility; +- the change is large, ambiguous, difficult to reverse, or based on uncertain assumptions; +- diagnosis or implementation required several iterations; +- acceptance depends on more than a single mechanical check; +- independent criticism is explicitly requested or required before Goal completion. + +Default to one Analyst and give it all relevant review lenses. Add another only for a genuinely independent specialty or disputed high-impact finding; do not duplicate the same review for ceremony. + +## Prepare an Independent Review + +Give the Analyst a precise brief, not the whole Session narrative and not a desired verdict: + +- what must be true; +- what changed and the exact baseline or paths; +- what evidence already exists; +- which risks or assumptions deserve adversarial attention; +- which artifacts are unrelated and out of scope. + +Activate `review-change` for ordinary Plan or implementation review. The Analyst remains read-only. Use Explore or Librarian only through the established delegation boundaries for narrow evidence questions; do not invent new responsibilities or permissions. + +## Evaluate Feedback Technically + +For every proposed finding: + +1. Understand the exact claimed failure and affected requirement. +2. Verify it against current code, tests, product behavior, and architecture constraints. +3. Classify it as confirmed, false positive, already prevented elsewhere, unresolved question, or optional improvement. +4. Assign consequence-based severity: + - **Blocker:** unsafe to proceed or stated outcome defeated. + - **Major:** material correctness, security, data, compatibility, or acceptance defect. + - **Minor:** bounded defect or meaningful verification weakness. + - **Advisory:** optional simplification or future hardening. +5. Push back on technically incorrect, out-of-scope, or overdesigned advice with evidence. Never implement feedback merely because a reviewer stated it confidently. + +## Remediation Loop + +Resolve Blocker and Major findings before delivery, make the smallest root-cause correction, run fresh narrow then proportionate broad checks, and re-review whenever remediation changes the reviewed risk boundary. Check disputed evidence yourself before multiplying reviewers; return to diagnosis when repeated fixes do not add evidence. + +## Goal Boundary + +For Goal completion, follow `run-goal`: after implementation, fresh verification, and terminal children, create a fresh direct `deep` Analyst with `goal-review`. If remediation changes the final state, obtain another fresh Goal review. The Lead interprets the complete natural-language report and alone decides whether evidence supports calling `update_goal`; the reviewer and Skill do not control Goal state. + +## Stop Conditions + +Stop the review loop and report the exact limitation when: + +- the objective, baseline, or final change surface cannot be established; +- a finding depends on a missing product decision; +- required verification is unavailable, unsafe, or needs new authority; +- remediation would expand scope materially beyond the user's authorization; +- evidence remains contradictory after the smallest discriminating checks. + +Do not convert missing evidence into approval or failure. State what can and cannot be concluded and the next decisive action. + +## Output + +Report: + +1. objective, scope, baseline, and review depth chosen; +2. who or what reviewed which surface; +3. confirmed findings in severity order, plus false positives or disputed items that materially affected the decision; +4. fixes made and the exact evidence that rechecked them; +5. fresh verification commands and outcomes; +6. unresolved Minor findings, residual risk, and anything not verified; +7. the current delivery status in plain language. + +Never imply that review alone proves completion. The final status must match the newest attributable code and fresh verification evidence. diff --git a/packages/agent-core/src/skills/builtin/review-work/references/review-packet.md b/packages/agent-core/src/skills/builtin/review-work/references/review-packet.md new file mode 100644 index 00000000..1c2af170 --- /dev/null +++ b/packages/agent-core/src/skills/builtin/review-work/references/review-packet.md @@ -0,0 +1,38 @@ +# Review packet and remediation loop + +Use this packet to let an independent reviewer reconstruct the work without steering it toward approval. + +```markdown +## Contract +- Objective and acceptance criteria: +- Constraints / non-goals / user decisions: + +## Attributable surface +- Baseline commit or artifact: +- Final diff, files, schemas, migrations, generated artifacts: +- Unrelated work explicitly excluded: + +## Implementation map +- Changed owners and interfaces: +- State/data/event flow affected: +- Failure, retry, permission, persistence, or compatibility boundaries: + +## Verification +| Claim | Fresh command/inspection | Environment | Exit/material result | Limitation | +| --- | --- | --- | --- | --- | + +## Challenge targets +- Known risks and assumptions: +- Decisions that must not be reopened: +- Evidence gaps or checks not run: +``` + +Do not include a desired verdict or tell the reviewer that another reviewer already approved. Include failed checks and known limitations; omitting them makes the packet less independent, not more persuasive. + +For each confirmed finding, record classification, severity, correction, and fresh proof. Re-review after a material correction changes the reviewed behavior or invalidates prior evidence. Stop for a decision or external limit; do not treat missing evidence as approval. + +| Finding | Verified? | Correction | Affected acceptance | Fresh proof | Re-review | +| --- | --- | --- | --- | --- | --- | +| | yes/no | | | | open/closed | + +Lead should independently reproduce or inspect material findings before changing code. Reject false positives with evidence. After a fix, rerun the narrow regression and every broader check invalidated by the change, then ask the reviewer to recheck the full affected acceptance condition—not only the edited line. diff --git a/packages/agent-core/src/skills/builtin/run-goal/SKILL.md b/packages/agent-core/src/skills/builtin/run-goal/SKILL.md index 2e2ea7ac..734aad42 100644 --- a/packages/agent-core/src/skills/builtin/run-goal/SKILL.md +++ b/packages/agent-core/src/skills/builtin/run-goal/SKILL.md @@ -1,14 +1,46 @@ --- name: run-goal -description: Drive an authorized Goal through execution, recovery, review, remediation, and truthful completion. -when_to_use: Runtime activates this for a root Lead Session with an active Goal. +description: Drive an authorized Goal through execution, recovery, review, remediation, and truthful completion in a root Lead Session with an active Goal. +license: MIT +metadata: + archcode/source: "ArchCode Goal lifecycle" + archcode/source-commit: "f00efe7ab3cd87f951797d9b4bf14415f10abd7a" + archcode/adaptation: "original rewrite" --- -- Read the exact Goal objective, status, and blocked reason from the latest `goal-notice` in model-visible Session history. That notice is the authoritative work instruction across continuations. -- Use `get_goal` only when current usage, execution, or budget accounting is needed. Never use it to recover a missing objective, status, or blocked reason; a missing current `goal-notice` is an invalid Goal context and must stop execution. -- Continue direct work and bounded delegation until the objective is verifiably complete, a real HITL decision is needed, or progress is genuinely blocked. -- Do not broaden authority, create a parallel workflow engine, or treat a Plan as required Goal state. -- After implementation and verification finish and all children are terminal, create a fresh direct deep Analyst with `goal-review`. -- Interpret the Analyst's complete evidence report in context. If it identifies material gaps, fix and verify them, then create another fresh review Analyst; a report from before the changes is not a fresh review of the final result. -- Call `update_goal` with `status=complete` only when the latest fresh review and the Lead's own evidence support completion. The Lead owns this semantic judgment; the Runtime only enforces typed Goal state, terminal children, and the current Goal instance/generation. -- Report exact blockers rather than marking difficult, incomplete, or budget-limited work blocked. +## Reconstruct the authoritative target + +1. Read the exact objective, status, and blocked reason from the latest `goal-notice` in model-visible Session history. The notice is authoritative across continuations. +2. Stop if the current notice is missing or internally unusable; `get_goal` is accounting-only and must not be used to reconstruct semantic Goal state. Use it only when current usage, execution count, elapsed execution time, or token budget is needed. +3. Translate the objective into an internal acceptance map: required outcomes, constraints, observable evidence, likely failure modes, and explicit non-goals. Do not add new Goal state or silently broaden the user's authority. + +Keep a compact working ledger in the current reasoning or ordinary work artifacts: one row per obligation with current evidence, `supported` / `partial` / `failed` / `unknown`, and the next decisive action. This is execution bookkeeping, not persisted Goal state, and it must be rebuilt when later evidence invalidates a row. + +## Execute and control risk + +- Choose direct work or bounded delegation through the ordinary Lead topology. Use Analyst for difficult reasoning or review, Build for bounded implementation, Explore for local evidence, and Librarian for external evidence; no child owns Goal completion. +- Order work by dependency and observable delivery boundary. Parallelize only independent tasks with no shared mutable state, overlapping edits, or sequential contract. Integrate and verify each boundary before advancing. +- At each checkpoint, compare actual state with the acceptance map. Run the narrowest decisive check for the changed behavior, then broader repository checks in proportion to integration risk. Treat stale output, partial output, and child claims as leads rather than proof. +- Record material Plan deviations and failed verification when they occur. Do not reconstruct a clean success narrative at the end or let a later broad pass erase an unresolved narrow failure. +- When evidence invalidates the approach, revise the implementation path without changing the Goal objective. Ask the user when the objective, scope, external authority, destructive action, or an unavailable prerequisite requires a decision. +- Do not create a parallel workflow engine or require a Plan as Goal state. A Plan may guide execution, but it has no Goal linkage or automatic synchronization. + +## Stop and blocked decisions + +Continue until the objective is verifiably satisfied, a real HITL decision is required, or progress is genuinely blocked. + +- Do not mark blocked because work is hard, incomplete, slow, budget-limited, or needs another evidence-producing attempt. +- Mark blocked only when a concrete external condition prevents meaningful progress and safe in-scope alternatives are exhausted. State the exact condition, evidence, impact, and what would unblock it. +- If a child is suspended or non-terminal, resolve, resume, cancel as authorized, or wait as appropriate; do not proceed to Goal completion. + +## Verification and independent final review + +1. Re-read the Goal objective and acceptance map after implementation. For every completion claim, identify a decisive command, inspection, or artifact; run it fresh and read the complete result and exit status. +2. If verification fails or leaves a material requirement unproven, remediate and rerun the relevant checks. Report the actual incomplete state instead of implying success. +3. After implementation and Lead verification finish and every child in the Session family is terminal, create a fresh direct deep Analyst with `goal-review`. Give it the full objective, attributable final changes, evidence, known limitations, and questions without suggesting the desired verdict. +4. Interpret the Analyst's complete report rather than a label. Fix every material gap, rerun proportionate verification, and create another fresh review Analyst after changes; a report produced before remediation is not a review of the final result. +5. Call `update_goal` with `status=complete` only when the latest fresh review and the Lead's own fresh evidence support every material acceptance condition. The Lead owns this semantic judgment; the Runtime enforces typed Goal state, terminal children, and the current Goal instance and generation. + +## Final report + +Lead with the achieved outcome or exact blocker. Report the objective checked, material changes, fresh verification commands or inspections and results, independent review conclusion and remediation, skipped or unavailable checks, and residual risk. Never claim completion from confidence, prior runs, a clean diff alone, or a child report. diff --git a/packages/agent-core/src/skills/builtin/safe-refactor/SKILL.md b/packages/agent-core/src/skills/builtin/safe-refactor/SKILL.md index b7675eba..3cd98219 100644 --- a/packages/agent-core/src/skills/builtin/safe-refactor/SKILL.md +++ b/packages/agent-core/src/skills/builtin/safe-refactor/SKILL.md @@ -1,14 +1,64 @@ --- name: safe-refactor -description: Refactor code while preserving behavior through scoped changes and verification. -when_to_use: Use when restructuring or renaming code without changing behavior - variable, function, or type renames, extracting modules, moving files, simplifying logic, or consolidating duplicates. +description: Refactor code while preserving behavior through scoped changes and verification when restructuring, renaming, extracting, moving, simplifying, or consolidating code. +license: MIT +metadata: + archcode/source: "MIT refactoring and TDD concepts" + archcode/source-commit: "44c9b2d6e889982ac18c27d05a19fefe335194e1" + archcode/adaptation: "idea-only rewrite" --- -- Identify public contracts first: exported symbols, API signatures, and test surface. -- Trace call sites and dependents before renaming or moving; update all references in one coherent batch. -- Make the smallest transformation that achieves the goal; avoid mixing refactoring with behavior changes. -- Preserve existing error-handling paths and data shapes unless the refactoring goal explicitly requires otherwise. -- Run `lsp_find_references` and `lsp_diagnostics` after each transformation step to catch missed updates. -- When extracting a module, ensure imports in both old and new locations resolve correctly before deleting the original. -- Verify with targeted diagnostics and the most relevant test suite before widening scope. -- If a refactoring step introduces type errors, fix them immediately rather than proceeding with broken intermediate states. \ No newline at end of file +# Safe Refactor + +A refactor changes structure without changing observable behavior. Optimize for clearer ownership and lower complexity, not fewer lines or a more fashionable pattern. + +For dependency-boundary and verification decisions, read [references/boundary-verification.md](references/boundary-verification.md). + +## Confirm the boundary + +Before editing, state: + +- the structural problem being removed; +- the behavior that must remain identical, including outputs, errors, side effects, ordering, persistence, and public types; +- the files or subsystem in scope and what is explicitly out of scope; +- the evidence that will detect a behavior change. + +If the requested result changes behavior or public semantics, treat that part as a behavior change rather than hiding it inside the refactor. + +## Understand before changing + +1. Read the implementation, callers, tests, neighboring conventions, and relevant project instructions. +2. Trace exported symbols, API boundaries, data shapes, error paths, lifecycle hooks, and side effects. Use LSP references when available and appropriate; otherwise combine text, AST, and call-site searches. +3. Establish a clean behavioral baseline with existing tests or a focused characterization test. A characterization test should capture intended behavior, not freeze an accidental implementation detail. +4. Explain why the current structure exists. Preserve constraints that are still real; remove indirection only when it no longer provides ownership, substitution, testability, or isolation value. + +Do not proceed while you cannot distinguish preserved behavior from the proposed structural change. + +## Design the transformation + +- Choose the smallest coherent seam that achieves the requested structural result completely. +- Keep responsibilities together and dependencies directed toward the component that owns the contract. +- Avoid drive-by cleanup, speculative abstractions, compatibility wrappers, and unrelated renames. +- Plan symbol moves and deletions together with all callers, imports, exports, tests, configuration, and documentation that depend on them. +- Prefer explicit, project-native code over compressed or clever replacements. + +## Execute incrementally + +1. Change one coherent structural step at a time while keeping the tree understandable. +2. Update all dependents for that step; do not leave dual paths, temporary fallbacks, or dead compatibility layers in the final result. +3. Run the cheapest relevant feedback after each risky step: references or diagnostics when supported, otherwise targeted typecheck, build, or tests. +4. If a step breaks behavior or makes the design harder to explain, revert or repair that step before continuing. Do not stack further changes on a broken intermediate state. +5. Delete obsolete code only after callers and behavior have moved and the replacement is verified. + +Tests may be added to expose an uncovered contract. Do not weaken assertions or rewrite expected behavior merely to make the refactor pass. + +## Verify the result + +1. Inspect the final diff for accidental behavior changes, duplicated paths, stale exports, unresolved imports, weakened errors, or unrelated churn. +2. Re-run the focused checks that proved the baseline, then widen to the affected package or repository checks in proportion to risk. +3. Confirm every caller now uses the intended structure and the removed structure is no longer referenced. +4. Compare the result with project conventions and the original goal: the new design should be easier to understand, change, or test for a concrete reason. + +Stop and report when the preserved behavior is ambiguous, required coverage cannot be established, a public contract must change, or verification exposes a failure outside the authorized scope. + +Finish with the structural change, contracts preserved, obsolete paths removed, verification run, and any residual risk. diff --git a/packages/agent-core/src/skills/builtin/safe-refactor/references/boundary-verification.md b/packages/agent-core/src/skills/builtin/safe-refactor/references/boundary-verification.md new file mode 100644 index 00000000..5e171f12 --- /dev/null +++ b/packages/agent-core/src/skills/builtin/safe-refactor/references/boundary-verification.md @@ -0,0 +1,27 @@ +# Dependency boundaries and verification + +Before moving a seam, identify exported symbols, callers, data shapes, side effects, error paths, lifecycle hooks, configuration, and tests. Keep the contract owner on the dependency direction that already owns the policy; do not create wrappers merely to preserve an obsolete shape. + +## Boundary inventory + +| Concern | Baseline evidence | Must remain true | Verification | +| --- | --- | --- | --- | +| Public symbol/type | definitions and callers | same supported contract or explicit hard cut | typecheck + caller inspection | +| State/side effect | owner and event/order path | same observable mutation and ordering | focused behavior test | +| Error/permission | mapping and guard owner | same denial/failure semantics | negative-path test | +| Lifecycle | startup, retry, cancellation, cleanup | no leak, duplicate, or stale owner | integration inspection/test | +| Persistence/config | schema and readers/writers | compatible shape or complete migration | round-trip/restart test | + +Establish the baseline before editing. A characterization test is useful only when it captures intentional behavior at the real boundary; do not freeze an accidental implementation detail merely because it exists. + +Apply one coherent structural move: introduce the new owner or seam, move all callers that belong to that step, verify, then remove the obsolete path. When the requirement is a hard cut, do not leave wrappers, aliases, dual writes, fallback reads, or tests whose only purpose is preserving the deleted shape. + +## Verification ladder + +1. Reference/symbol search proves which callers were considered, not runtime correctness. +2. Diagnostics and typecheck prove static consistency, not behavior or build delivery. +3. Focused tests prove the changed invariant and material negative path. +4. Integration/architecture tests prove crossed process, persistence, package, or dependency boundaries. +5. Build or compiled smoke proves delivery when bundling/static assets are part of the seam. + +Inspect the final diff for duplicated policy, reversed dependencies, temporary adapters, and unrelated cleanup. Compare the same behavioral inventory before and after; fewer files or passing types alone do not establish a safe refactor. diff --git a/packages/agent-core/src/skills/builtin/shape-todo/SKILL.md b/packages/agent-core/src/skills/builtin/shape-todo/SKILL.md index f9ca57d3..7e23e00a 100644 --- a/packages/agent-core/src/skills/builtin/shape-todo/SKILL.md +++ b/packages/agent-core/src/skills/builtin/shape-todo/SKILL.md @@ -1,16 +1,81 @@ --- name: shape-todo -description: Clarify and update one Todo bound to a Discussion Session, including creating or improving its unique Plan when requested. -when_to_use: Runtime activates this for a Todo Discussion Session. +description: Clarify and update one Todo bound to a Discussion Session, including its unique Plan when requested, before implementation begins. +license: MIT +metadata: + archcode/source: "Superpowers brainstorming concepts" + archcode/source-commit: "44c9b2d6e889982ac18c27d05a19fefe335194e1" + archcode/adaptation: "idea-only rewrite" --- -- Treat the runtime-bound Todo as the only Todo you may update; never accept a Todo identity from model input. -- First decide whether the bound Todo and current request state a concrete enough outcome and scope to choose relevant evidence. If not, ask one focused clarification before inspecting the workspace, probing the Plan, or calling another tool. Otherwise investigate existing code and evidence before asking only for decisions that evidence cannot answer. -- Capture objective, scope, constraints, acceptance criteria, risks, and explicit product choices in the bound Todo. -- Move the Todo to Ready only after user confirmation and sufficient execution clarity. -- When the user asks to create or improve a Plan, use `plan-work` and the exact runtime-provided `todoPlanPath`. Use `todoPlanState` to distinguish an existing Plan from an absent one; never discover Plan existence by scanning the workspace. -- Read an existing Plan before editing it. Never create another Plan file or Plan metadata for the bound Todo. -- Use Explore or Librarian only for separable research questions. Do not delegate Analyst or Build. -- Do not begin product code implementation, modify product source, create a Goal or Automation, or create execution resources from the Discussion Session. -- Bash and file tools support research, Todo shaping, and Plan authoring. This is a behavioral boundary, not a security sandbox; global permissions and protected-path rules remain authoritative. -- Ready execution starts in a new ordinary Lead Session; the Discussion does not become the executor. +Use this as a bounded, evidence-led brainstorming loop for the one Todo bound to the +current Discussion Session. The goal is a decision-ready Todo and, when requested, +one executable Markdown Plan—not an implementation. + +Use [references/todo-shaping-template.md](references/todo-shaping-template.md) to capture scope, decisions, and observable acceptance without turning Discussion into implementation. + +### Keep the binding authoritative + +- Treat the runtime-bound Todo as the only Todo that may be read or updated. Derive + its identity from the Session binding; never accept a Todo ID, title, or path from + model input. +- Begin with the bound Todo and the current request, then inspect the smallest + relevant repository evidence needed to establish current behavior and constraints. + Use `todoPlanState` rather than probing for Plan existence. Do not ask the user for + facts available from the request, repository, Plan, or tool output; ask one focused + question only when a material product choice remains after investigation. +- Keep the Discussion attached to that Todo throughout the conversation. Do not + create a second Todo, a shadow status, or a parallel execution record. + +### Clarify in stages + +Run a short converge-and-confirm loop rather than a one-shot guess: + +1. Restate the problem, intended user outcome, and the decision this Todo needs to + support without filling gaps with an assumed preference. +2. Inspect the smallest relevant set of repository files, tests, configuration, and + existing Plan evidence selected by authoritative runtime context. Expand the + search only when new evidence changes the map. +3. Separate **repository facts** from **user product choices**. Facts are observed + behavior or source evidence and should include a path, symbol, test, or command + when useful. Choices include desired behavior, UX, compatibility, priorities, + non-goals, and trade-offs; never present an inferred preference as a fact. +4. State remaining assumptions and risks. Ask only the next focused question that + evidence cannot answer; when more than one solution is plausible, offer bounded + options, explain the meaningful trade-off, and let the user choose. Do not treat a + speculative implementation as approved. +5. Confirm the selected solution shape, scope and non-goals, dependencies, and + observable acceptance criteria. Revisit any item that is still ambiguous before + marking the Todo ready. + +### Capture and ready gate + +- Update the bound Todo with the agreed objective, background, scope, non-goals, + constraints, dependencies, risks, validation approach, acceptance criteria, and + explicit product choices. Make acceptance criteria observable and decidable. +- Move the Todo to `Ready` only after the user confirms the direction and the next + Lead Session can execute it without guessing about scope or success. Evidence + alone is not user approval. + +### Plan handling + +- When the user asks to create or improve a Plan, use `plan-work` and the exact + runtime-provided `todoPlanPath`. Use `todoPlanState` to distinguish a present Plan + from an absent one; never scan the workspace to discover Plan existence. +- If `todoPlanState=present`, read that exact file before editing it. If it is absent, + create only that exact path when a Plan is requested. Never create another Plan + file, Plan ID, sidecar metadata, or Plan service for this Todo. +- Keep the Plan as ordinary Markdown guidance. Do not claim that Plan changes are + execution state or automatically synchronized with a Goal. + +### Discussion boundaries + +- Use Explore or Librarian only for separable evidence questions. Do not delegate + Analyst or Build, and do not shift the final product decision to a child. +- Do not begin product-code implementation, modify product source, create a Goal or + Automation, or create execution resources from this Discussion Session. +- Bash and file tools may support research, Todo shaping, and Plan authoring only. + This is a behavioral boundary, not a security sandbox; global permissions and + protected-path rules remain authoritative. +- Ready work starts in a new ordinary Lead Session. This Discussion never becomes + the executor. diff --git a/packages/agent-core/src/skills/builtin/shape-todo/references/todo-shaping-template.md b/packages/agent-core/src/skills/builtin/shape-todo/references/todo-shaping-template.md new file mode 100644 index 00000000..016230e0 --- /dev/null +++ b/packages/agent-core/src/skills/builtin/shape-todo/references/todo-shaping-template.md @@ -0,0 +1,37 @@ +# Todo shaping template + +```markdown +## Outcome +Problem observed: +Intended user-visible result: + +## Evidence +- Repository/runtime fact: +- Existing behavior or constraint: +- Assumption still needing evidence: + +## Scope and non-goals +- Included owner/flow/interface: +- Explicitly excluded: + +## Decisions +- Confirmed direction and rationale: +- Remaining product choice for the user: + +## Dependencies and risks +- Prerequisite, external authority, migration, or sequencing risk: +- Control or decision required: + +## Acceptance +- Given , when , then . +- Failure/edge case: . +- Verification or inspection: +``` + +Keep repository facts separate from product choices. “The current API has no batch endpoint” can be established from code; “users should see partial success” is a product decision unless already specified. + +Weak acceptance: “Improve error handling” or “works correctly.” Strong acceptance: “When the provider times out before any side effect, the Session remains retryable and the UI shows the mapped timeout state; the focused integration test observes both.” Do not require implementation details such as a particular class name unless that ownership is itself an accepted constraint. + +Before Ready, check that the outcome is singular, scope is bounded, exclusions do not omit required work, every remaining choice is either answered or explicitly blocks execution, and the next Lead can identify decisive completion evidence. + +Move to Ready only after the user confirms the direction and the next Lead can execute without guessing. This template never authorizes implementation. diff --git a/packages/agent-core/src/skills/package-reader.test.ts b/packages/agent-core/src/skills/package-reader.test.ts new file mode 100644 index 00000000..b07c10b2 --- /dev/null +++ b/packages/agent-core/src/skills/package-reader.test.ts @@ -0,0 +1,393 @@ +import { afterAll, beforeEach, describe, expect, test } from "bun:test"; +import { mkdir, rm, symlink } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import { + activateBuiltinSkill, + activateFilesystemSkill as activateFilesystemSkillAt, + discoverFilesystemSkill as discoverFilesystemSkillAt, + readBuiltinSkillResource, + readFilesystemSkillResource as readFilesystemSkillResourceAt, + SKILL_PACKAGE_MAX_BYTES, + SKILL_PACKAGE_MAX_ENTRIES, + SKILL_RESOURCE_MAX_BYTES, + SKILL_RESOURCE_MAX_DEPTH, + SKILL_RESOURCE_MAX_FILES, + validateResourcePath, +} from "./package-reader"; +import type { BuiltinSkillPackage } from "./types"; + +const tmpRoot = join(tmpdir(), "archcode-skill-package-reader", crypto.randomUUID()); +const encoder = new TextEncoder(); + +function entry(name = "test-skill", body = "Entry body.\n"): string { + return `--- +name: ${name} +description: Provides package-reader guidance when Skill packages are being validated. +--- + +${body}`; +} + +function builtin( + resources: Readonly>, + name = "test-skill", +): BuiltinSkillPackage { + return { entry: entry(name), resources }; +} + +async function writePackage( + packageRoot: string, + resources: Readonly> = {}, + markdown = entry(), +): Promise { + await mkdir(packageRoot, { recursive: true }); + await Bun.write(join(packageRoot, "SKILL.md"), markdown); + for (const [path, value] of Object.entries(resources)) { + const destination = join(packageRoot, ...path.split("/")); + await mkdir(dirname(destination), { recursive: true }); + await Bun.write(destination, value); + } +} + +function filesystemLocation(packageRoot: string) { + return { boundaryRoot: dirname(packageRoot), root: packageRoot }; +} + +function discoverFilesystemSkill(packageRoot: string, expectedName: string) { + return discoverFilesystemSkillAt(filesystemLocation(packageRoot), expectedName); +} + +function activateFilesystemSkill(packageRoot: string, expectedName: string) { + return activateFilesystemSkillAt(filesystemLocation(packageRoot), expectedName); +} + +function readFilesystemSkillResource(packageRoot: string, expectedName: string, resource: string) { + return readFilesystemSkillResourceAt(filesystemLocation(packageRoot), expectedName, resource); +} + +describe("Skill package reader", () => { + beforeEach(async () => { + await rm(tmpRoot, { recursive: true, force: true }); + await mkdir(tmpRoot, { recursive: true }); + }); + + afterAll(async () => { + await rm(tmpRoot, { recursive: true, force: true }); + }); + + test("discovery reads bounded metadata without validating the body or traversing resources", async () => { + const packageRoot = join(tmpRoot, "discovery", "test-skill"); + await writePackage(packageRoot); + await Bun.write(join(packageRoot, "SKILL.md"), new Blob([entry(), Uint8Array.from([0xff])])); + await symlink(join(tmpRoot, "outside"), join(packageRoot, "linked-directory")); + + expect(await discoverFilesystemSkill(packageRoot, "test-skill")).toEqual({ + name: "test-skill", + description: "Provides package-reader guidance when Skill packages are being validated.", + }); + await expect(activateFilesystemSkill(packageRoot, "test-skill")).rejects.toThrow("valid UTF-8"); + }); + + test("activation returns stable sorted POSIX descriptors without SKILL.md contents", async () => { + const packageRoot = join(tmpRoot, "sorted", "test-skill"); + const arbitraryBytes = Uint8Array.from([0x00, 0xff, 0x80, 0x41]); + await writePackage(packageRoot, { + "z-last.bin": arbitraryBytes, + "references/b.md": "bbb", + "assets/a.txt": "a", + }); + + const activated = await activateFilesystemSkill(packageRoot, "test-skill"); + expect(activated.resources).toEqual([ + { path: "assets/a.txt", bytes: 1 }, + { path: "references/b.md", bytes: 3 }, + { path: "z-last.bin", bytes: 4 }, + ]); + expect(activated.resources.map((resource) => resource.path)).not.toContain("SKILL.md"); + const read = await readFilesystemSkillResource(packageRoot, "test-skill", "z-last.bin"); + expect([...read.content]).toEqual([...arbitraryBytes]); + }); + + test("builtin inventory and reads preserve arbitrary bytes", () => { + const bytes = Uint8Array.from([0xff, 0xfe, 0x00, 0x61]); + const skillPackage = builtin({ "assets/arbitrary.bin": bytes }); + + expect(activateBuiltinSkill(skillPackage, "test-skill").resources).toEqual([ + { path: "assets/arbitrary.bin", bytes: 4 }, + ]); + const read = readBuiltinSkillResource(skillPackage, "test-skill", "assets/arbitrary.bin"); + expect([...read.content]).toEqual([...bytes]); + expect(read.content).not.toBe(bytes); + }); + + test("builtin packages cannot place resources below the SKILL.md entry path", () => { + expect(() => activateBuiltinSkill( + builtin({ "SKILL.md/hidden.txt": "impossible filesystem shape" }), + "test-skill", + )).toThrow("cannot be a resource directory"); + }); + + test("rejects absolute, traversal, empty-segment, dot-segment, backslash, NUL, excessive-depth, and entry paths", () => { + const invalid = [ + "", + "/absolute.md", + "../escape.md", + "references/../escape.md", + "references//file.md", + "./file.md", + "references/./file.md", + "references\\file.md", + "references/file\0.md", + "SKILL.md", + "SKILL.md/hidden.txt", + [...Array(SKILL_RESOURCE_MAX_DEPTH).fill("d"), "file.md"].join("/"), + ]; + for (const path of invalid) expect(() => validateResourcePath(path)).toThrow(); + + expect(() => validateResourcePath("references/file.md")).not.toThrow(); + }); + + test("enforces resource depth below, equal, and above the fixed limit", () => { + for (const depth of [SKILL_RESOURCE_MAX_DEPTH - 1, SKILL_RESOURCE_MAX_DEPTH]) { + const path = pathAtDepth(depth); + expect(activateBuiltinSkill(builtin({ [path]: "ok" }), "test-skill").resources[0]?.path).toBe(path); + } + expect(() => activateBuiltinSkill( + builtin({ [pathAtDepth(SKILL_RESOURCE_MAX_DEPTH + 1)]: "too deep" }), + "test-skill", + )).toThrow(`depth exceeds ${SKILL_RESOURCE_MAX_DEPTH}`); + }); + + test("enforces one-resource bytes below, equal, and above the fixed limit", () => { + for (const size of [SKILL_RESOURCE_MAX_BYTES - 1, SKILL_RESOURCE_MAX_BYTES]) { + const activated = activateBuiltinSkill( + builtin({ "assets/payload.bin": new Uint8Array(size) }), + "test-skill", + ); + expect(activated.resources).toEqual([{ path: "assets/payload.bin", bytes: size }]); + } + expect(() => activateBuiltinSkill( + builtin({ "assets/payload.bin": new Uint8Array(SKILL_RESOURCE_MAX_BYTES + 1) }), + "test-skill", + )).toThrow(`exceeds ${SKILL_RESOURCE_MAX_BYTES} bytes`); + }); + + test("enforces filesystem resource bytes below, equal, and above the fixed limit", async () => { + for (const size of [SKILL_RESOURCE_MAX_BYTES - 1, SKILL_RESOURCE_MAX_BYTES]) { + const packageRoot = join(tmpRoot, `filesystem-resource-${size}`, "test-skill"); + await writePackage(packageRoot, { "assets/payload.bin": new Uint8Array(size) }); + expect((await activateFilesystemSkill(packageRoot, "test-skill")).resources).toEqual([ + { path: "assets/payload.bin", bytes: size }, + ]); + } + const aboveRoot = join(tmpRoot, "filesystem-resource-above", "test-skill"); + await writePackage(aboveRoot, { + "assets/payload.bin": new Uint8Array(SKILL_RESOURCE_MAX_BYTES + 1), + }); + await expect(activateFilesystemSkill(aboveRoot, "test-skill")) + .rejects.toThrow(`exceeds ${SKILL_RESOURCE_MAX_BYTES} bytes`); + }); + + test("enforces resource-file count below, equal, and above the fixed limit", () => { + for (const count of [SKILL_RESOURCE_MAX_FILES - 1, SKILL_RESOURCE_MAX_FILES]) { + expect(activateBuiltinSkill(builtin(flatResources(count)), "test-skill").resources).toHaveLength(count); + } + expect(() => activateBuiltinSkill( + builtin(flatResources(SKILL_RESOURCE_MAX_FILES + 1)), + "test-skill", + )).toThrow(`more than ${SKILL_RESOURCE_MAX_FILES} resource files`); + }); + + test("enforces filesystem resource-file count below, equal, and above the fixed limit", async () => { + for (const count of [SKILL_RESOURCE_MAX_FILES - 1, SKILL_RESOURCE_MAX_FILES]) { + const packageRoot = join(tmpRoot, `filesystem-count-${count}`, "test-skill"); + await writePackage(packageRoot, flatResources(count)); + expect((await activateFilesystemSkill(packageRoot, "test-skill")).resources).toHaveLength(count); + } + const aboveRoot = join(tmpRoot, "filesystem-count-above", "test-skill"); + await writePackage(aboveRoot, flatResources(SKILL_RESOURCE_MAX_FILES + 1)); + await expect(activateFilesystemSkill(aboveRoot, "test-skill")) + .rejects.toThrow(`more than ${SKILL_RESOURCE_MAX_FILES} resource files`); + }); + + test("counts builtin implicit directories at directory-entry boundaries", () => { + const below = resourcesWithImplicitDirectories(127, false); + const equal = resourcesWithImplicitDirectories(127, true); + const above = resourcesWithImplicitDirectories(128, false); + + expect(countImplicitEntries(below)).toBe(SKILL_PACKAGE_MAX_ENTRIES - 1); + expect(activateBuiltinSkill(builtin(below), "test-skill").resources).toHaveLength(127); + expect(countImplicitEntries(equal)).toBe(SKILL_PACKAGE_MAX_ENTRIES); + expect(activateBuiltinSkill(builtin(equal), "test-skill").resources).toHaveLength(128); + expect(countImplicitEntries(above)).toBe(SKILL_PACKAGE_MAX_ENTRIES + 1); + expect(() => activateBuiltinSkill(builtin(above), "test-skill")) + .toThrow(`more than ${SKILL_PACKAGE_MAX_ENTRIES} directory entries`); + }); + + test("enforces filesystem directory entries below, equal, and above the fixed limit", async () => { + const cases = [ + { label: "below", resources: resourcesWithImplicitDirectories(127, false), expected: 127 }, + { label: "equal", resources: resourcesWithImplicitDirectories(127, true), expected: 128 }, + ] as const; + for (const item of cases) { + const packageRoot = join(tmpRoot, `filesystem-entries-${item.label}`, "test-skill"); + await writePackage(packageRoot, item.resources); + expect((await activateFilesystemSkill(packageRoot, "test-skill")).resources).toHaveLength(item.expected); + } + + const aboveRoot = join(tmpRoot, "filesystem-entries-above", "test-skill"); + await writePackage(aboveRoot, resourcesWithImplicitDirectories(128, false)); + await expect(activateFilesystemSkill(aboveRoot, "test-skill")) + .rejects.toThrow(`more than ${SKILL_PACKAGE_MAX_ENTRIES} directory entries`); + }); + + test("enforces aggregate package bytes below, equal, and above the fixed limit", () => { + const entryBytes = encoder.encode(entry()).byteLength; + for (const total of [SKILL_PACKAGE_MAX_BYTES - 1, SKILL_PACKAGE_MAX_BYTES]) { + const skillPackage = builtin(resourcesWithTotalBytes(total - entryBytes)); + const activated = activateBuiltinSkill(skillPackage, "test-skill"); + const actualTotal = entryBytes + activated.resources.reduce((sum, resource) => sum + resource.bytes, 0); + expect(actualTotal).toBe(total); + } + expect(() => activateBuiltinSkill( + builtin(resourcesWithTotalBytes(SKILL_PACKAGE_MAX_BYTES + 1 - entryBytes)), + "test-skill", + )).toThrow(`exceeds ${SKILL_PACKAGE_MAX_BYTES} aggregate bytes`); + }); + + test("enforces filesystem aggregate bytes below, equal, and above the fixed limit", async () => { + const entryBytes = encoder.encode(entry()).byteLength; + for (const total of [SKILL_PACKAGE_MAX_BYTES - 1, SKILL_PACKAGE_MAX_BYTES]) { + const packageRoot = join(tmpRoot, `filesystem-aggregate-${total}`, "test-skill"); + await writePackage(packageRoot, resourcesWithTotalBytes(total - entryBytes)); + const activated = await activateFilesystemSkill(packageRoot, "test-skill"); + const actualTotal = entryBytes + activated.resources.reduce((sum, resource) => sum + resource.bytes, 0); + expect(actualTotal).toBe(total); + } + + const aboveRoot = join(tmpRoot, "filesystem-aggregate-above", "test-skill"); + await writePackage( + aboveRoot, + resourcesWithTotalBytes(SKILL_PACKAGE_MAX_BYTES + 1 - entryBytes), + ); + await expect(activateFilesystemSkill(aboveRoot, "test-skill")) + .rejects.toThrow(`exceeds ${SKILL_PACKAGE_MAX_BYTES} aggregate bytes`); + }); + + test("rejects symlinked package roots, entries, directories, and resources", async () => { + const targetRoot = join(tmpRoot, "symlinks", "target-root"); + await writePackage(targetRoot); + const linkedRoot = join(tmpRoot, "symlinks", "linked-root"); + await symlink(targetRoot, linkedRoot, "dir"); + await expect(activateFilesystemSkill(linkedRoot, "test-skill")).rejects.toThrow("must not be a symlink"); + + const entryRoot = join(tmpRoot, "symlinks", "entry-root"); + await mkdir(entryRoot, { recursive: true }); + const externalEntry = join(tmpRoot, "symlinks", "external-entry.md"); + await Bun.write(externalEntry, entry()); + await symlink(externalEntry, join(entryRoot, "SKILL.md"), "file"); + await expect(activateFilesystemSkill(entryRoot, "test-skill")).rejects.toThrow("must not be a symlink"); + + const directoryRoot = join(tmpRoot, "symlinks", "directory-root"); + await writePackage(directoryRoot); + const externalDirectory = join(tmpRoot, "symlinks", "external-directory"); + await mkdir(externalDirectory, { recursive: true }); + await symlink(externalDirectory, join(directoryRoot, "references"), "dir"); + await expect(activateFilesystemSkill(directoryRoot, "test-skill")).rejects.toThrow("symlinks are not allowed"); + + const resourceRoot = join(tmpRoot, "symlinks", "resource-root"); + await writePackage(resourceRoot); + const externalResource = join(tmpRoot, "symlinks", "external-resource.md"); + await Bun.write(externalResource, "outside"); + await symlink(externalResource, join(resourceRoot, "resource.md"), "file"); + await expect(activateFilesystemSkill(resourceRoot, "test-skill")).rejects.toThrow("symlinks are not allowed"); + }); + + test("rejects a symlink in package ancestry below the trusted source boundary", async () => { + const boundaryRoot = join(tmpRoot, "ancestry", "project"); + const externalSkillsRoot = join(tmpRoot, "ancestry", "external-skills"); + const externalPackageRoot = join(externalSkillsRoot, "test-skill"); + await writePackage(externalPackageRoot); + await mkdir(join(boundaryRoot, ".archcode"), { recursive: true }); + await symlink(externalSkillsRoot, join(boundaryRoot, ".archcode", "skills"), "dir"); + const lexicalPackageRoot = join(boundaryRoot, ".archcode", "skills", "test-skill"); + const location = { boundaryRoot, root: lexicalPackageRoot }; + + await expect(discoverFilesystemSkillAt(location, "test-skill")) + .rejects.toThrow("must not be a symlink"); + await expect(activateFilesystemSkillAt(location, "test-skill")) + .rejects.toThrow("must not be a symlink"); + await expect(readFilesystemSkillResourceAt(location, "test-skill", "missing.md")) + .rejects.toThrow("must not be a symlink"); + }); + + test("rejects a non-UTF-8 SKILL.md without rejecting arbitrary-byte resources", async () => { + const packageRoot = join(tmpRoot, "utf8", "test-skill"); + await writePackage(packageRoot, { "assets/valid.bin": Uint8Array.from([0xff, 0xfe]) }); + expect((await activateFilesystemSkill(packageRoot, "test-skill")).resources).toEqual([ + { path: "assets/valid.bin", bytes: 2 }, + ]); + + await Bun.write(join(packageRoot, "SKILL.md"), new Blob([entry(), Uint8Array.from([0xff])])); + await expect(activateFilesystemSkill(packageRoot, "test-skill")).rejects.toThrow("valid UTF-8"); + }); + + test("validates resource input again before filesystem access", async () => { + const packageRoot = join(tmpRoot, "input", "test-skill"); + await writePackage(packageRoot, { "references/guide.md": "guide" }); + + for (const resource of ["/tmp/outside", "../outside", "references\\guide.md", "references//guide.md"]) { + await expect(readFilesystemSkillResource(packageRoot, "test-skill", resource)).rejects.toThrow(); + } + }); +}); + +function pathAtDepth(depth: number): string { + if (depth < 1) throw new Error("Depth must be positive"); + return [...Array(Math.max(0, depth - 1)).fill("d"), "file.bin"].join("/"); +} + +function flatResources(count: number): Record { + return Object.fromEntries( + Array.from({ length: count }, (_, index) => [`resource-${String(index).padStart(3, "0")}.txt`, "x"]), + ); +} + +function resourcesWithImplicitDirectories( + directoryCount: number, + addExtraResource: boolean, +): Record { + const resources = Object.fromEntries( + Array.from({ length: directoryCount }, (_, index) => [ + `d-${String(index).padStart(3, "0")}/resource.txt`, + "x", + ]), + ); + if (addExtraResource) resources["d-000/extra.txt"] = "x"; + return resources; +} + +function countImplicitEntries(resources: Readonly>): number { + const directories = new Set(); + for (const path of Object.keys(resources)) { + const segments = path.split("/"); + for (let index = 1; index < segments.length; index += 1) { + directories.add(segments.slice(0, index).join("/")); + } + } + return 1 + Object.keys(resources).length + directories.size; +} + +function resourcesWithTotalBytes(totalBytes: number): Record { + const resources: Record = {}; + let remaining = totalBytes; + let index = 0; + while (remaining > 0) { + const size = Math.min(SKILL_RESOURCE_MAX_BYTES, remaining); + resources[`assets/chunk-${index}.bin`] = new Uint8Array(size); + remaining -= size; + index += 1; + } + return resources; +} diff --git a/packages/agent-core/src/skills/package-reader.ts b/packages/agent-core/src/skills/package-reader.ts new file mode 100644 index 00000000..7b0e7d15 --- /dev/null +++ b/packages/agent-core/src/skills/package-reader.ts @@ -0,0 +1,358 @@ +import { constants } from "node:fs"; +import { lstat, open, readdir } from "node:fs/promises"; +import { isAbsolute, join, posix, relative, resolve, sep } from "node:path"; +import { + parseSkillHeaderBytes, + parseSkillMarkdown, + SKILL_ENTRY_MAX_BYTES, + SKILL_FRONTMATTER_MAX_BYTES, +} from "./schema"; +import type { + BuiltinSkillPackage, + SkillMetadata, + SkillResourceDescriptor, +} from "./types"; + +export const SKILL_ENTRY_FILE = "SKILL.md"; +export const SKILL_RESOURCE_MAX_BYTES = 1024 * 1024; +export const SKILL_RESOURCE_MAX_FILES = 128; +export const SKILL_PACKAGE_MAX_ENTRIES = 256; +export const SKILL_RESOURCE_MAX_DEPTH = 8; +export const SKILL_PACKAGE_MAX_BYTES = 8 * 1024 * 1024; + +const DISCOVERY_READ_MAX_BYTES = SKILL_FRONTMATTER_MAX_BYTES + 16; + +export interface ActivatedSkillPackage { + readonly metadata: SkillMetadata; + readonly body: string; + readonly resources: readonly SkillResourceDescriptor[]; +} + +export interface FilesystemSkillPackageLocation { + /** Trusted workspace or user boundary. Every descendant through root is checked without following symlinks. */ + readonly boundaryRoot: string; + readonly root: string; +} + +export class SkillPackageResourceNotFoundError extends Error { + constructor(public readonly resource: string) { + super(`Skill resource is not listed: ${resource}`); + this.name = "SkillPackageResourceNotFoundError"; + } +} + +export async function filesystemSkillDirectoryExists( + location: FilesystemSkillPackageLocation, +): Promise { + try { + await assertFilesystemSkillAncestry(location); + return true; + } catch (error) { + if (isNoEntryError(error)) return false; + throw error; + } +} + +export async function discoverFilesystemSkill( + location: FilesystemSkillPackageLocation, + expectedName: string, +): Promise { + const { root } = location; + await assertFilesystemSkillAncestry(location); + await assertRegularDirectory(root, "Skill package root"); + const entryPath = join(root, SKILL_ENTRY_FILE); + await assertRegularFile(entryPath, "SKILL.md"); + const headerBytes = await readPrefix(entryPath, DISCOVERY_READ_MAX_BYTES); + const { metadata } = parseSkillHeaderBytes(headerBytes); + assertExpectedName(metadata, expectedName); + await assertFilesystemSkillAncestry(location); + return metadata; +} + +export async function activateFilesystemSkill( + location: FilesystemSkillPackageLocation, + expectedName: string, +): Promise { + const { root } = location; + await assertFilesystemSkillAncestry(location); + await assertRegularDirectory(root, "Skill package root"); + const entryPath = join(root, SKILL_ENTRY_FILE); + const entryBytes = await readRegularFileBounded(entryPath, SKILL_ENTRY_MAX_BYTES, "SKILL.md"); + let entryText: string; + try { + entryText = new TextDecoder("utf-8", { fatal: true }).decode(entryBytes); + } catch { + throw new Error("SKILL.md must be valid UTF-8"); + } + const { metadata, body } = parseSkillMarkdown(entryText); + assertExpectedName(metadata, expectedName); + const resources = await walkFilesystemResources(root, entryBytes.byteLength); + await assertFilesystemSkillAncestry(location); + return { metadata, body, resources }; +} + +export async function readFilesystemSkillResource( + location: FilesystemSkillPackageLocation, + expectedName: string, + resource: string, +): Promise<{ readonly descriptor: SkillResourceDescriptor; readonly content: Uint8Array }> { + const { root } = location; + validateResourcePath(resource); + const activated = await activateFilesystemSkill(location, expectedName); + const descriptor = activated.resources.find((candidate) => candidate.path === resource); + if (descriptor === undefined) throw new SkillPackageResourceNotFoundError(resource); + const content = await readRegularFileBounded( + join(root, ...resource.split("/")), + SKILL_RESOURCE_MAX_BYTES, + `Skill resource "${resource}"`, + ); + if (content.byteLength !== descriptor.bytes) { + throw new Error(`Skill resource changed while reading: ${resource}`); + } + await assertFilesystemSkillAncestry(location); + return { descriptor, content }; +} + +export function discoverBuiltinSkill( + skillPackage: BuiltinSkillPackage, + expectedName: string, +): SkillMetadata { + const entryBytes = new TextEncoder().encode(skillPackage.entry.slice(0, DISCOVERY_READ_MAX_BYTES)); + const { metadata } = parseSkillHeaderBytes(entryBytes.subarray(0, DISCOVERY_READ_MAX_BYTES)); + assertExpectedName(metadata, expectedName); + return metadata; +} + +export function activateBuiltinSkill( + skillPackage: BuiltinSkillPackage, + expectedName: string, +): ActivatedSkillPackage { + const entryBytes = new TextEncoder().encode(skillPackage.entry); + if (entryBytes.byteLength > SKILL_ENTRY_MAX_BYTES) { + throw new Error(`SKILL.md exceeds ${SKILL_ENTRY_MAX_BYTES} bytes`); + } + const { metadata, body } = parseSkillMarkdown(skillPackage.entry); + assertExpectedName(metadata, expectedName); + const resources = builtinResourceDescriptors(skillPackage, entryBytes.byteLength); + return { metadata, body, resources }; +} + +export function readBuiltinSkillResource( + skillPackage: BuiltinSkillPackage, + expectedName: string, + resource: string, +): { readonly descriptor: SkillResourceDescriptor; readonly content: Uint8Array } { + validateResourcePath(resource); + const activated = activateBuiltinSkill(skillPackage, expectedName); + const descriptor = activated.resources.find((candidate) => candidate.path === resource); + if (descriptor === undefined) throw new SkillPackageResourceNotFoundError(resource); + const value = skillPackage.resources[resource]; + if (value === undefined) throw new Error(`Skill resource is not embedded: ${resource}`); + const content = typeof value === "string" ? new TextEncoder().encode(value) : value.slice(); + return { descriptor, content }; +} + +export function validateResourcePath(resource: string): void { + if (resource.length === 0) throw new Error("Skill resource path must not be empty"); + if (resource.includes("\0")) throw new Error("Skill resource path must not contain NUL bytes"); + if (resource.includes("\\")) throw new Error("Skill resource path must use POSIX separators"); + if (posix.isAbsolute(resource)) throw new Error("Skill resource path must be relative"); + const segments = resource.split("/"); + if (segments.some((segment) => segment === "" || segment === "." || segment === "..")) { + throw new Error("Skill resource path contains an invalid segment"); + } + if (segments.length > SKILL_RESOURCE_MAX_DEPTH) { + throw new Error(`Skill resource depth exceeds ${SKILL_RESOURCE_MAX_DEPTH}`); + } + if (segments[0] === SKILL_ENTRY_FILE) { + throw new Error("SKILL.md is the package entry and cannot be a resource directory"); + } +} + +async function walkFilesystemResources( + root: string, + entryBytes: number, +): Promise { + const resources: SkillResourceDescriptor[] = []; + let totalEntries = 0; + let totalBytes = entryBytes; + + async function walk(directory: string, prefix: readonly string[]): Promise { + const entries = await readdir(directory, { withFileTypes: true }); + entries.sort((a, b) => lexicalCompare(a.name, b.name)); + for (const entry of entries) { + totalEntries += 1; + if (totalEntries > SKILL_PACKAGE_MAX_ENTRIES) { + throw new Error(`Skill package contains more than ${SKILL_PACKAGE_MAX_ENTRIES} directory entries`); + } + const segments = [...prefix, entry.name]; + const relativePath = segments.join("/"); + const absolutePath = join(directory, entry.name); + if (entry.isSymbolicLink()) throw new Error(`Skill package symlinks are not allowed: ${relativePath}`); + if (entry.isDirectory()) { + if (segments.length > SKILL_RESOURCE_MAX_DEPTH) { + throw new Error(`Skill resource depth exceeds ${SKILL_RESOURCE_MAX_DEPTH}: ${relativePath}`); + } + await assertRegularDirectory(absolutePath, `Skill resource directory "${relativePath}"`); + await walk(absolutePath, segments); + continue; + } + if (!entry.isFile()) throw new Error(`Skill package entry must be a regular file: ${relativePath}`); + if (prefix.length === 0 && entry.name === SKILL_ENTRY_FILE) continue; + validateResourcePath(relativePath); + const info = await assertRegularFile(absolutePath, `Skill resource "${relativePath}"`); + if (info.size > SKILL_RESOURCE_MAX_BYTES) { + throw new Error(`Skill resource exceeds ${SKILL_RESOURCE_MAX_BYTES} bytes: ${relativePath}`); + } + resources.push(Object.freeze({ path: relativePath, bytes: info.size })); + if (resources.length > SKILL_RESOURCE_MAX_FILES) { + throw new Error(`Skill package contains more than ${SKILL_RESOURCE_MAX_FILES} resource files`); + } + totalBytes += info.size; + if (totalBytes > SKILL_PACKAGE_MAX_BYTES) { + throw new Error(`Skill package exceeds ${SKILL_PACKAGE_MAX_BYTES} aggregate bytes`); + } + } + } + + await walk(root, []); + return Object.freeze(resources.sort((a, b) => lexicalCompare(a.path, b.path))); +} + +function builtinResourceDescriptors( + skillPackage: BuiltinSkillPackage, + entryBytes: number, +): readonly SkillResourceDescriptor[] { + const paths = Object.keys(skillPackage.resources).sort(lexicalCompare); + if (paths.length > SKILL_RESOURCE_MAX_FILES) { + throw new Error(`Skill package contains more than ${SKILL_RESOURCE_MAX_FILES} resource files`); + } + const directories = new Set(); + const pathSet = new Set(paths); + for (const path of paths) { + const segments = path.split("/"); + for (let index = 1; index < segments.length; index += 1) { + const directory = segments.slice(0, index).join("/"); + if (pathSet.has(directory)) { + throw new Error(`Builtin Skill resource path is both a file and directory: ${directory}`); + } + directories.add(directory); + } + } + if (paths.length + directories.size + 1 > SKILL_PACKAGE_MAX_ENTRIES) { + throw new Error(`Skill package contains more than ${SKILL_PACKAGE_MAX_ENTRIES} directory entries`); + } + let totalBytes = entryBytes; + const resources = paths.map((path) => { + validateResourcePath(path); + const value = skillPackage.resources[path]; + if (value === undefined) throw new Error(`Skill resource is missing: ${path}`); + const bytes = typeof value === "string" ? Buffer.byteLength(value, "utf8") : value.byteLength; + if (bytes > SKILL_RESOURCE_MAX_BYTES) { + throw new Error(`Skill resource exceeds ${SKILL_RESOURCE_MAX_BYTES} bytes: ${path}`); + } + totalBytes += bytes; + if (totalBytes > SKILL_PACKAGE_MAX_BYTES) { + throw new Error(`Skill package exceeds ${SKILL_PACKAGE_MAX_BYTES} aggregate bytes`); + } + return Object.freeze({ path, bytes }); + }); + return Object.freeze(resources); +} + +async function readPrefix(path: string, maxBytes: number): Promise { + const handle = await open(path, constants.O_RDONLY | constants.O_NOFOLLOW); + const buffer = new Uint8Array(maxBytes); + try { + const info = await handle.stat(); + if (!info.isFile()) throw new Error("SKILL.md must be a regular file"); + let offset = 0; + while (offset < buffer.byteLength) { + const { bytesRead } = await handle.read(buffer, offset, buffer.byteLength - offset, offset); + if (bytesRead === 0) break; + offset += bytesRead; + } + return buffer.subarray(0, offset); + } finally { + await handle.close(); + } +} + +async function readRegularFileBounded( + path: string, + maxBytes: number, + label: string, +): Promise { + await assertRegularFile(path, label); + const handle = await open(path, constants.O_RDONLY | constants.O_NOFOLLOW); + try { + const info = await handle.stat(); + if (!info.isFile()) throw new Error(`${label} must be a regular file`); + if (info.size > maxBytes) throw new Error(`${label} exceeds ${maxBytes} bytes`); + const buffer = new Uint8Array(info.size); + let offset = 0; + while (offset < buffer.byteLength) { + const { bytesRead } = await handle.read(buffer, offset, buffer.byteLength - offset, offset); + if (bytesRead === 0) break; + offset += bytesRead; + } + const after = await handle.stat(); + if (offset !== info.size || after.size !== info.size) throw new Error(`${label} changed while reading`); + return buffer; + } finally { + await handle.close(); + } +} + +async function assertRegularDirectory(path: string, label: string) { + const info = await lstat(path); + if (info.isSymbolicLink()) throw new Error(`${label} must not be a symlink`); + if (!info.isDirectory()) throw new Error(`${label} must be a directory`); + return info; +} + +export async function assertFilesystemSkillAncestry( + location: FilesystemSkillPackageLocation, +): Promise { + const boundaryRoot = resolve(location.boundaryRoot); + const packageRoot = resolve(location.root); + const descendant = relative(boundaryRoot, packageRoot); + if ( + descendant === "" + || descendant === ".." + || descendant.startsWith(`..${sep}`) + || isAbsolute(descendant) + ) { + throw new Error(`Skill filesystem root must be contained by its source boundary: ${packageRoot}`); + } + + await assertRegularDirectory(boundaryRoot, "Skill source boundary"); + let current = boundaryRoot; + for (const segment of descendant.split(sep)) { + current = join(current, segment); + await assertRegularDirectory(current, `Skill package ancestry "${current}"`); + } +} + +async function assertRegularFile(path: string, label: string) { + const info = await lstat(path); + if (info.isSymbolicLink()) throw new Error(`${label} must not be a symlink`); + if (!info.isFile()) throw new Error(`${label} must be a regular file`); + return info; +} + +function assertExpectedName(metadata: SkillMetadata, expectedName: string): void { + if (metadata.name !== expectedName) { + throw new Error( + `frontmatter.name must match package directory "${expectedName}" (received "${metadata.name}")`, + ); + } +} + +function isNoEntryError(error: unknown): boolean { + return error !== null && typeof error === "object" && "code" in error && error.code === "ENOENT"; +} + +function lexicalCompare(a: string, b: string): number { + return a < b ? -1 : a > b ? 1 : 0; +} diff --git a/packages/agent-core/src/skills/schema.test.ts b/packages/agent-core/src/skills/schema.test.ts index 908ed040..41ea9b77 100644 --- a/packages/agent-core/src/skills/schema.test.ts +++ b/packages/agent-core/src/skills/schema.test.ts @@ -1,80 +1,217 @@ import { describe, expect, test } from "bun:test"; -import { parseSkillMarkdown, SkillMetadataSchema } from "./schema"; -import { BUILTIN_SKILL_BODIES } from "./builtin/manifest"; -import type { BuiltinSkillName } from "./builtin/manifest"; +import { + parseSkillFrontmatter, + parseSkillHeaderBytes, + parseSkillMarkdown, + SKILL_COMPATIBILITY_MAX_LENGTH, + SKILL_DESCRIPTION_MAX_LENGTH, + SKILL_ENTRY_MAX_BYTES, + SKILL_FRONTMATTER_MAX_BYTES, + SKILL_NAME_MAX_LENGTH, + SkillMetadataSchema, +} from "./schema"; -describe("SkillMetadataSchema", () => { - test("parses required skill frontmatter", () => { +const encoder = new TextEncoder(); + +describe("Skill schema", () => { + test("parses the adopted Agent Skills metadata and preserves the exact body", () => { const parsed = parseSkillMarkdown(`--- name: safe-refactor -description: Refactor safely -when_to_use: Use when restructuring code without behavior changes. -allowed_tools: grep, file_read, lsp_diagnostics +description: Refactors code without behavior changes when a repository needs structural cleanup. +license: MIT +compatibility: Requires repository source access. +metadata: + archcode/source: superpowers + archcode/adaptation: idea-only --- Follow the plan. `); - expect(parsed.metadata).toEqual({ - name: "safe-refactor", - description: "Refactor safely", - when_to_use: "Use when restructuring code without behavior changes.", - allowed_tools: ["grep", "file_read", "lsp_diagnostics"], + expect(parsed).toEqual({ + metadata: { + name: "safe-refactor", + description: "Refactors code without behavior changes when a repository needs structural cleanup.", + license: "MIT", + compatibility: "Requires repository source access.", + metadata: { + "archcode/source": "superpowers", + "archcode/adaptation": "idea-only", + }, + }, + body: "\nFollow the plan.\n", }); - expect(parsed.body).toBe("Follow the plan.\n"); }); - test("rejects missing when_to_use", () => { + test("requires name and description", () => { + expect(() => SkillMetadataSchema.parse({ description: "Use this when needed." })).toThrow(); + expect(() => SkillMetadataSchema.parse({ name: "git-master" })).toThrow(); + }); + + test("rejects a generic unknown top-level field", () => { expect(() => SkillMetadataSchema.parse({ name: "git-master", - description: "Git guidance", + description: "Guides Git operations when repository history must change safely.", + custom: "unsupported", })).toThrow(); }); - test("rejects empty when_to_use after trim", () => { - expect(() => SkillMetadataSchema.parse({ + test("requires metadata to be a string-to-string map", () => { + expect(SkillMetadataSchema.parse({ name: "git-master", - description: "Git guidance", - when_to_use: " ", - })).toThrow(); + description: "Guides Git operations when repository history must change safely.", + metadata: { source: "archcode", revision: "abc123" }, + }).metadata).toEqual({ source: "archcode", revision: "abc123" }); + + for (const metadata of [{ nested: { value: "no" } }, { count: 1 }, ["no"]]) { + expect(() => SkillMetadataSchema.parse({ + name: "git-master", + description: "Guides Git operations when repository history must change safely.", + metadata, + })).toThrow(); + } }); - test("trims whitespace from when_to_use", () => { - const parsed = SkillMetadataSchema.parse({ - name: "codemap", - description: "Map code", - when_to_use: " Use before implementation. ", - }); - expect(parsed.when_to_use).toBe("Use before implementation."); + test("enforces the exact lowercase kebab name grammar and length", () => { + for (const name of ["a", "git-master", "skill-1", "a".repeat(SKILL_NAME_MAX_LENGTH)]) { + expect(SkillMetadataSchema.parse({ name, description: "Use this Skill when needed." }).name).toBe(name); + } + + for (const name of [ + "Git", + "-bad", + "bad-", + "bad_name", + "double--hyphen", + "a".repeat(SKILL_NAME_MAX_LENGTH + 1), + "", + ]) { + expect(() => SkillMetadataSchema.parse({ name, description: "Use this Skill when needed." })).toThrow(); + } }); - test("rejects unknown metadata fields", () => { + test("enforces description below, equal, and above its byte-independent character limit", () => { + for (const length of [SKILL_DESCRIPTION_MAX_LENGTH - 1, SKILL_DESCRIPTION_MAX_LENGTH]) { + expect(SkillMetadataSchema.parse({ name: "a", description: "d".repeat(length) }).description).toHaveLength(length); + } expect(() => SkillMetadataSchema.parse({ - name: "git-master", - description: "Git guidance", - when_to_use: "Use for Git work.", - unexpectedField: true, + name: "a", + description: "d".repeat(SKILL_DESCRIPTION_MAX_LENGTH + 1), })).toThrow(); }); - test("validates skill names with the exact lowercase kebab pattern", () => { - for (const name of ["a", "git-master", "skill-1"] as const) { - expect(SkillMetadataSchema.parse({ name, description: "ok", when_to_use: "Use when needed." }).name).toBe(name); + test("counts astral Unicode description characters as code points", () => { + const equal = "😀".repeat(SKILL_DESCRIPTION_MAX_LENGTH); + expect([...equal]).toHaveLength(SKILL_DESCRIPTION_MAX_LENGTH); + expect(SkillMetadataSchema.parse({ name: "a", description: equal }).description).toBe(equal); + + const above = "😀".repeat(SKILL_DESCRIPTION_MAX_LENGTH + 1); + expect(() => SkillMetadataSchema.parse({ name: "a", description: above })).toThrow(); + }); + + test("enforces compatibility below, equal, and above its character limit", () => { + for (const length of [SKILL_COMPATIBILITY_MAX_LENGTH - 1, SKILL_COMPATIBILITY_MAX_LENGTH]) { + expect(SkillMetadataSchema.parse({ + name: "a", + description: "Use this Skill when needed.", + compatibility: "c".repeat(length), + }).compatibility).toHaveLength(length); } + expect(() => SkillMetadataSchema.parse({ + name: "a", + description: "Use this Skill when needed.", + compatibility: "c".repeat(SKILL_COMPATIBILITY_MAX_LENGTH + 1), + })).toThrow(); + }); + + test("counts astral Unicode compatibility characters as code points", () => { + const equal = "😀".repeat(SKILL_COMPATIBILITY_MAX_LENGTH); + expect([...equal]).toHaveLength(SKILL_COMPATIBILITY_MAX_LENGTH); + expect(SkillMetadataSchema.parse({ + name: "a", + description: "Use this Skill when needed.", + compatibility: equal, + }).compatibility).toBe(equal); + + const above = "😀".repeat(SKILL_COMPATIBILITY_MAX_LENGTH + 1); + expect(() => SkillMetadataSchema.parse({ + name: "a", + description: "Use this Skill when needed.", + compatibility: above, + })).toThrow(); + }); + + test("counts outer whitespace before trimming bounded metadata strings", () => { + expect(SkillMetadataSchema.parse({ + name: "a", + description: ` ${"d".repeat(SKILL_DESCRIPTION_MAX_LENGTH - 2)} `, + compatibility: ` ${"c".repeat(SKILL_COMPATIBILITY_MAX_LENGTH - 2)} `, + })).toMatchObject({ + description: "d".repeat(SKILL_DESCRIPTION_MAX_LENGTH - 2), + compatibility: "c".repeat(SKILL_COMPATIBILITY_MAX_LENGTH - 2), + }); + + expect(() => SkillMetadataSchema.parse({ + name: "a", + description: ` ${"d".repeat(SKILL_DESCRIPTION_MAX_LENGTH)} `, + })).toThrow(`at most ${SKILL_DESCRIPTION_MAX_LENGTH} characters`); + expect(() => SkillMetadataSchema.parse({ + name: "a", + description: "Use this Skill when needed.", + compatibility: ` ${"c".repeat(SKILL_COMPATIBILITY_MAX_LENGTH)} `, + })).toThrow(`at most ${SKILL_COMPATIBILITY_MAX_LENGTH} characters`); + }); - for (const name of ["Git", "-bad", "bad_name", ""] as const) { - expect(() => SkillMetadataSchema.parse({ name, description: "bad" })).toThrow(); + test("enforces frontmatter bytes below, equal, and above 16 KiB", () => { + for (const size of [SKILL_FRONTMATTER_MAX_BYTES - 1, SKILL_FRONTMATTER_MAX_BYTES]) { + const frontmatter = frontmatterWithExactBytes(size); + expect(encoder.encode(frontmatter)).toHaveLength(size); + expect(parseSkillFrontmatter(frontmatter).name).toBe("a"); } + const above = frontmatterWithExactBytes(SKILL_FRONTMATTER_MAX_BYTES + 1); + expect(() => parseSkillFrontmatter(above)).toThrow(`exceeds ${SKILL_FRONTMATTER_MAX_BYTES} bytes`); }); - test("builtin skill when_to_use values contain no quotes", () => { - for (const [skillName, body] of Object.entries(BUILTIN_SKILL_BODIES)) { - const parsed = parseSkillMarkdown(body); - expect(parsed.metadata.when_to_use).not.toStartWith('"'); - expect(parsed.metadata.when_to_use).not.toEndWith('"'); - expect(parsed.metadata.when_to_use).not.toStartWith("'"); - expect(parsed.metadata.when_to_use).not.toEndWith("'"); - expect(parsed.metadata.name).toBe(skillName as BuiltinSkillName); + test("enforces SKILL.md bytes below, equal, and above 128 KiB", () => { + for (const size of [SKILL_ENTRY_MAX_BYTES - 1, SKILL_ENTRY_MAX_BYTES]) { + const markdown = markdownWithExactBytes(size); + expect(encoder.encode(markdown)).toHaveLength(size); + expect(parseSkillMarkdown(markdown).metadata.name).toBe("a"); } + expect(() => parseSkillMarkdown(markdownWithExactBytes(SKILL_ENTRY_MAX_BYTES + 1))) + .toThrow(`exceeds ${SKILL_ENTRY_MAX_BYTES} bytes`); + }); + + test("parses CRLF delimiters and returns the correct byte body offset", () => { + const content = encoder.encode("---\r\nname: a\r\ndescription: Use this Skill when needed.\r\n---\r\nbody\r\n"); + const parsed = parseSkillHeaderBytes(content); + expect(parsed.metadata.name).toBe("a"); + expect(new TextDecoder().decode(content.subarray(parsed.bodyOffset))).toBe("body\r\n"); + }); + + test("rejects invalid UTF-8 in frontmatter and a missing closing delimiter", () => { + const prefix = encoder.encode("---\nname: a\ndescription: "); + const suffix = encoder.encode("\n---\nbody"); + const invalid = new Uint8Array(prefix.byteLength + 1 + suffix.byteLength); + invalid.set(prefix); + invalid[prefix.byteLength] = 0xff; + invalid.set(suffix, prefix.byteLength + 1); + expect(() => parseSkillHeaderBytes(invalid)).toThrow("valid UTF-8"); + expect(() => parseSkillHeaderBytes(encoder.encode("---\nname: a\n"))).toThrow("closing delimiter"); }); }); + +function frontmatterWithExactBytes(target: number): string { + const prefix = "name: a\ndescription: Use this Skill when needed.\nmetadata:\n pad: "; + const suffix = "\n"; + const padding = target - encoder.encode(prefix + suffix).byteLength; + if (padding < 1) throw new Error(`Target ${target} is too small for valid frontmatter`); + return `${prefix}${"x".repeat(padding)}${suffix}`; +} + +function markdownWithExactBytes(target: number): string { + const prefix = "---\nname: a\ndescription: Use this Skill when needed.\n---\n"; + const padding = target - encoder.encode(prefix).byteLength; + if (padding < 0) throw new Error(`Target ${target} is too small for valid Skill Markdown`); + return `${prefix}${"b".repeat(padding)}`; +} diff --git a/packages/agent-core/src/skills/schema.ts b/packages/agent-core/src/skills/schema.ts index 2589c31f..67649b9e 100644 --- a/packages/agent-core/src/skills/schema.ts +++ b/packages/agent-core/src/skills/schema.ts @@ -1,42 +1,143 @@ +import { parse as parseYaml } from "yaml"; import { z } from "zod/v4"; -import { parseFrontmatter as parseGenericFrontmatter } from "../utils/frontmatter"; import type { SkillMetadata } from "./types"; -export const SKILL_NAME_REGEX = /^[a-z0-9][a-z0-9-]*$/; - -const allowedToolsSchema = z.preprocess((value) => { - if (typeof value !== "string") return value; - const trimmed = value.trim(); - if (trimmed === "") return []; - const unwrapped = trimmed.startsWith("[") && trimmed.endsWith("]") - ? trimmed.slice(1, -1) - : trimmed; - return unwrapped - .split(",") - .map((item) => item.trim().replace(/^['\"]|['\"]$/g, "")) - .filter((item) => item.length > 0); -}, z.array(z.string().min(1))); +export const SKILL_NAME_REGEX = /^(?!.*--)[a-z0-9]+(?:-[a-z0-9]+)*$/; +export const SKILL_NAME_MAX_LENGTH = 64; +export const SKILL_DESCRIPTION_MAX_LENGTH = 1_024; +export const SKILL_COMPATIBILITY_MAX_LENGTH = 500; +export const SKILL_FRONTMATTER_MAX_BYTES = 16 * 1024; +export const SKILL_ENTRY_MAX_BYTES = 128 * 1024; + +const metadataMapSchema = z.record(z.string(), z.string()); + +function boundedTrimmedString(label: string, maxLength: number) { + return z.string() + .refine( + (value) => Array.from(value).length <= maxLength, + `Skill ${label} must be at most ${maxLength} characters`, + ) + .transform((value) => value.trim()) + .pipe(z.string().min(1, `Skill ${label} must not be empty`)); +} export const SkillMetadataSchema = z.strictObject({ - name: z.string().regex(SKILL_NAME_REGEX, "Skill name must match ^[a-z0-9][a-z0-9-]*$"), - description: z.string().min(1), - when_to_use: z.string().trim().min(1), - allowed_tools: allowedToolsSchema.optional(), + name: z.string().min(1).max(SKILL_NAME_MAX_LENGTH).regex( + SKILL_NAME_REGEX, + "Skill name must use lowercase letters, digits, and single hyphens", + ), + description: boundedTrimmedString("description", SKILL_DESCRIPTION_MAX_LENGTH), + license: z.string().trim().min(1).optional(), + compatibility: boundedTrimmedString("compatibility", SKILL_COMPATIBILITY_MAX_LENGTH).optional(), + metadata: metadataMapSchema.optional(), }); -export type ParsedSkillMetadata = z.infer; +export interface ParsedSkillMarkdown { + readonly metadata: SkillMetadata; + readonly body: string; +} + +export interface ParsedSkillHeader { + readonly metadata: SkillMetadata; + readonly bodyOffset: number; +} + +export function parseSkillFrontmatter(frontmatter: string): SkillMetadata { + const bytes = Buffer.byteLength(frontmatter, "utf8"); + if (bytes > SKILL_FRONTMATTER_MAX_BYTES) { + throw new Error(`Skill frontmatter exceeds ${SKILL_FRONTMATTER_MAX_BYTES} bytes`); + } + + let parsed: unknown; + try { + parsed = parseYaml(frontmatter, { maxAliasCount: 10, uniqueKeys: true }); + } catch (error) { + throw new Error(`Invalid Skill YAML frontmatter: ${error instanceof Error ? error.message : String(error)}`); + } -export function parseSkillMarkdown(content: string): { - metadata: SkillMetadata; - body: string; -} { - const { frontmatter, body } = parseGenericFrontmatter(content); - const metadata = SkillMetadataSchema.parse(frontmatter); - return { metadata, body }; + if (parsed === null || typeof parsed !== "object" || Array.isArray(parsed)) { + throw new Error("Skill frontmatter must be a YAML mapping"); + } + return SkillMetadataSchema.parse(parsed); +} + +export function parseSkillHeaderBytes(content: Uint8Array): ParsedSkillHeader { + const openingLength = openingDelimiterLength(content); + const closing = findClosingDelimiter(content, openingLength); + if (closing === null) { + throw new Error(`Skill frontmatter closing delimiter was not found within ${SKILL_FRONTMATTER_MAX_BYTES} bytes`); + } + if (closing.frontmatterEnd - openingLength > SKILL_FRONTMATTER_MAX_BYTES) { + throw new Error(`Skill frontmatter exceeds ${SKILL_FRONTMATTER_MAX_BYTES} bytes`); + } + + let frontmatter: string; + try { + frontmatter = new TextDecoder("utf-8", { fatal: true }).decode( + content.subarray(openingLength, closing.frontmatterEnd), + ); + } catch { + throw new Error("Skill frontmatter must be valid UTF-8"); + } + return { + metadata: parseSkillFrontmatter(frontmatter), + bodyOffset: closing.bodyOffset, + }; +} + +export function parseSkillMarkdown(content: string): ParsedSkillMarkdown { + const bytes = new TextEncoder().encode(content); + if (bytes.byteLength > SKILL_ENTRY_MAX_BYTES) { + throw new Error(`SKILL.md exceeds ${SKILL_ENTRY_MAX_BYTES} bytes`); + } + const { metadata, bodyOffset } = parseSkillHeaderBytes(bytes); + return { + metadata, + body: new TextDecoder().decode(bytes.subarray(bodyOffset)), + }; } export function assertSkillName(name: string): void { - if (!SKILL_NAME_REGEX.test(name)) { - throw new Error(`Skill name must match ^[a-z0-9][a-z0-9-]*$: ${name}`); + if (name.length > SKILL_NAME_MAX_LENGTH || !SKILL_NAME_REGEX.test(name)) { + throw new Error( + `Skill name must be 1-${SKILL_NAME_MAX_LENGTH} lowercase letters, digits, or single hyphen-separated segments: ${name}`, + ); } } + +function openingDelimiterLength(content: Uint8Array): number { + if (startsWithBytes(content, [45, 45, 45, 10])) return 4; + if (startsWithBytes(content, [45, 45, 45, 13, 10])) return 5; + throw new Error("Skill Markdown must start with a YAML frontmatter delimiter"); +} + +function findClosingDelimiter( + content: Uint8Array, + from: number, +): { readonly frontmatterEnd: number; readonly bodyOffset: number } | null { + let lineStart = from; + while (lineStart <= content.byteLength) { + let lineEnd = lineStart; + while (lineEnd < content.byteLength && content[lineEnd] !== 10 && content[lineEnd] !== 13) { + lineEnd += 1; + } + if ( + lineEnd - lineStart === 3 + && content[lineStart] === 45 + && content[lineStart + 1] === 45 + && content[lineStart + 2] === 45 + ) { + let bodyOffset = lineEnd; + if (content[bodyOffset] === 13) bodyOffset += 1; + if (content[bodyOffset] === 10) bodyOffset += 1; + return { frontmatterEnd: lineStart, bodyOffset }; + } + if (lineEnd >= content.byteLength) return null; + lineStart = lineEnd + (content[lineEnd] === 13 && content[lineEnd + 1] === 10 ? 2 : 1); + } + return null; +} + +function startsWithBytes(content: Uint8Array, expected: readonly number[]): boolean { + return expected.every((byte, index) => content[index] === byte); +} diff --git a/packages/agent-core/src/skills/service.test.ts b/packages/agent-core/src/skills/service.test.ts index 2339e556..253bc495 100644 --- a/packages/agent-core/src/skills/service.test.ts +++ b/packages/agent-core/src/skills/service.test.ts @@ -1,25 +1,51 @@ import { afterAll, beforeEach, describe, expect, test } from "bun:test"; -import { mkdir, rm } from "node:fs/promises"; -import { join } from "node:path"; -import { SkillService, SkillValidationError } from "./service"; +import { mkdir, rm, symlink } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { dirname, join } from "node:path"; +import type { BuiltinSkillPackage } from "./types"; +import { + RESERVED_BUILTIN_SKILL_NAMES, + SkillPathError, + SkillResourceNotFoundError, + SkillService, + SkillValidationError, +} from "./service"; -const tmpRoot = join(import.meta.dir, "__test_tmp__", "skill-service", crypto.randomUUID()); +const tmpRoot = join(tmpdir(), "archcode-skill-service", crypto.randomUUID()); -function skillMarkdown(name: string, description = `${name} description`, body = `${name} body`, whenToUse = `Use when ${name} is needed.`): string { +function skillMarkdown( + name: string, + description = `${name} guides work when ${name} is needed.`, + body = `${name} body`, +): string { return `--- name: ${name} description: ${description} -when_to_use: ${whenToUse} --- ${body} `; } -async function writeSkill(root: string, name: string, content: string): Promise { - const filePath = join(root, name, "SKILL.md"); - await mkdir(join(root, name), { recursive: true }); - await Bun.write(filePath, content); +function builtinSkill(name: string, body: string, resources: BuiltinSkillPackage["resources"] = {}): BuiltinSkillPackage { + return { entry: skillMarkdown(name, `${name} builtin guidance when builtin behavior is needed.`, body), resources }; +} + +async function writeSkill( + root: string, + name: string, + content: string, + resources: Readonly> = {}, +): Promise { + const packageRoot = join(root, name); + await mkdir(packageRoot, { recursive: true }); + await Bun.write(join(packageRoot, "SKILL.md"), content); + for (const [path, value] of Object.entries(resources)) { + const destination = join(packageRoot, ...path.split("/")); + await mkdir(dirname(destination), { recursive: true }); + await Bun.write(destination, value); + } + return packageRoot; } describe("SkillService", () => { @@ -37,117 +63,335 @@ describe("SkillService", () => { await rm(tmpRoot, { recursive: true, force: true }); }); - test("resolves project skills before user and builtin without merging", async () => { - await writeSkill(projectSkillsRoot, "git-master", skillMarkdown("git-master", "project git")); - await writeSkill(userSkillsRoot, "git-master", skillMarkdown("git-master", "user git")); + test("resolves one atomic package with project > user > builtin precedence", async () => { + const projectPackageRoot = await writeSkill( + projectSkillsRoot, + "codemap", + skillMarkdown("codemap", "Project mapping when this checkout is under investigation.", "PROJECT_ENTRY"), + { "references/project.md": "PROJECT_RESOURCE" }, + ); + await writeSkill( + userSkillsRoot, + "codemap", + skillMarkdown("codemap", "User mapping when any checkout is under investigation.", "USER_ENTRY"), + { "references/user.md": "USER_RESOURCE" }, + ); + const service = new SkillService({ + userSkillsRoot, + builtinSkills: { codemap: builtinSkill("codemap", "BUILTIN_ENTRY", { "references/builtin.md": "BUILTIN_RESOURCE" }) }, + }); - const service = new SkillService({ userSkillsRoot }); - const skill = await service.readForAgent(projectRoot, "git-master"); + const skill = await service.readForAgent(projectRoot, "codemap", ["codemap"]); - expect(skill?.source).toBe("project"); - expect(skill?.metadata.description).toBe("project git"); - expect(skill?.path).toBe(join(projectSkillsRoot, "git-master", "SKILL.md")); + expect(skill).toEqual({ + metadata: { + name: "codemap", + description: "Project mapping when this checkout is under investigation.", + }, + body: "\nPROJECT_ENTRY\n", + source: "project", + sourceLabel: projectPackageRoot, + root: projectPackageRoot, + resources: [{ path: "references/project.md", bytes: 16 }], + }); }); - test("resolves ordinary user skills before builtin", async () => { - await writeSkill(userSkillsRoot, "codemap", skillMarkdown("codemap", "user codemap")); + test("resolves user before builtin when no project package exists", async () => { + await writeSkill(userSkillsRoot, "codemap", skillMarkdown( + "codemap", + "User mapping when a checkout is under investigation.", + "USER_ENTRY", + )); + const service = new SkillService({ + userSkillsRoot, + builtinSkills: { codemap: builtinSkill("codemap", "BUILTIN_ENTRY") }, + }); - const service = new SkillService({ userSkillsRoot }); - const skill = await service.readForAgent(projectRoot, "codemap"); + expect((await service.readForAgent(projectRoot, "codemap", ["codemap"]))?.source).toBe("user"); + }); - expect(skill?.source).toBe("user"); - expect(skill?.metadata.description).toBe("user codemap"); + test("never falls through to a lower package for an unlisted resource", async () => { + await writeSkill(projectSkillsRoot, "codemap", skillMarkdown( + "codemap", + "Project mapping when this checkout is under investigation.", + )); + await writeSkill(userSkillsRoot, "codemap", skillMarkdown( + "codemap", + "User mapping when a checkout is under investigation.", + ), { "references/lower.md": "LOWER_RESOURCE" }); + const service = new SkillService({ userSkillsRoot, builtinSkills: {} }); + + await expect(service.readResourceForAgent( + projectRoot, + "codemap", + "references/lower.md", + ["codemap"], + )).rejects.toBeInstanceOf(SkillResourceNotFoundError); }); - test("the reserved Automation creation skill cannot be shadowed", async () => { - const name = "automation-create"; - await writeSkill(projectSkillsRoot, name, skillMarkdown(name, `project ${name}`)); - await writeSkill(userSkillsRoot, name, skillMarkdown(name, `user ${name}`)); + test("fails closed when the winning package is invalid", async () => { + await writeSkill(projectSkillsRoot, "codemap", skillMarkdown( + "wrong-name", + "Broken project override when validating precedence.", + )); + await writeSkill(userSkillsRoot, "codemap", skillMarkdown( + "codemap", + "Valid user package when validating precedence.", + )); + const service = new SkillService({ userSkillsRoot }); + try { + await service.readForAgent(projectRoot, "codemap", ["codemap"]); + throw new Error("Expected invalid winning package to fail"); + } catch (error) { + expect(error).toMatchObject({ + name: "SkillValidationError", + source: "project", + skillName: "codemap", + } satisfies Partial); + } + }); + + test("fails closed when the winning SKILL.md is unreadable", async () => { + await mkdir(join(projectSkillsRoot, "codemap", "SKILL.md"), { recursive: true }); + await writeSkill(userSkillsRoot, "codemap", skillMarkdown( + "codemap", + "Valid user package when validating unreadable precedence.", + )); const service = new SkillService({ userSkillsRoot }); - expect((await service.readForAgent(projectRoot, "automation-create"))?.source).toBe("builtin"); + await expect(service.readForAgent(projectRoot, "codemap", ["codemap"])) + .rejects.toMatchObject({ source: "project", skillName: "codemap" }); }); - test("reserved lifecycle Skills cannot be shadowed or loaded by an ineligible Agent", async () => { - for (const name of ["orchestrate-work", "plan-work", "execute-plan", "run-goal", "shape-todo", "review-work", "goal-review"]) { - await writeSkill(projectSkillsRoot, name, skillMarkdown(name, `project ${name}`)); - await writeSkill(userSkillsRoot, name, skillMarkdown(name, `user ${name}`)); - } + test("fails closed when the winning package directory has no SKILL.md", async () => { + await mkdir(join(projectSkillsRoot, "codemap"), { recursive: true }); + await writeSkill(userSkillsRoot, "codemap", skillMarkdown( + "codemap", + "Valid user package when a missing project entry is checked.", + )); const service = new SkillService({ userSkillsRoot }); - expect((await service.readForAgent(projectRoot, "goal-review", ["goal-review"]))?.source).toBe("builtin"); - expect((await service.readForAgent(projectRoot, "execute-plan", ["execute-plan"]))?.source).toBe("builtin"); - expect(await service.readForAgent(projectRoot, "goal-review", ["codemap"])).toBeNull(); - expect(await service.readForAgent(projectRoot, "execute-plan", ["codemap"])).toBeNull(); - const listed = await service.listForAgent(projectRoot, ["codemap"]); - expect(listed.map((entry) => entry.name)).not.toContain("goal-review"); + await expect(service.readForAgent(projectRoot, "codemap", ["codemap"])) + .rejects.toMatchObject({ source: "project", skillName: "codemap" }); }); - test("falls back to statically bundled builtin manifest", async () => { + test("does not touch a damaged lower user package after resolving a valid project winner", async () => { + await writeSkill(projectSkillsRoot, "codemap", skillMarkdown( + "codemap", + "Valid project mapping when lower sources are damaged.", + "PROJECT_ENTRY", + ), { "references/project.md": "PROJECT_RESOURCE" }); + const damagedUserRoot = join(userSkillsRoot, "codemap"); + await mkdir(damagedUserRoot, { recursive: true }); + await Bun.write(join(damagedUserRoot, "SKILL.md"), "not valid Skill Markdown"); + await symlink(join(tmpRoot, "outside"), join(damagedUserRoot, "linked-resource")); const service = new SkillService({ userSkillsRoot }); - const skill = await service.readForAgent(projectRoot, "codemap"); - expect(skill?.source).toBe("builtin"); - expect(skill?.metadata.name).toBe("codemap"); - expect(skill?.body).toContain("Trace entry points"); + const skill = await service.readForAgent(projectRoot, "codemap", ["codemap"]); + expect(skill?.source).toBe("project"); + expect(skill?.body).toContain("PROJECT_ENTRY"); + expect(skill?.resources).toEqual([{ path: "references/project.md", bytes: 16 }]); }); - test("does not fall back when a higher priority skill is invalid", async () => { - await writeSkill(projectSkillsRoot, "codemap", `--- -name: wrong-name -description: bad override -when_to_use: Use when broken. ---- + test("re-resolves the current winner independently on every entry and resource read", async () => { + await writeSkill(userSkillsRoot, "codemap", skillMarkdown( + "codemap", + "User mapping when a checkout is under investigation.", + "USER_ENTRY", + ), { "references/user.md": "USER_RESOURCE" }); + const service = new SkillService({ userSkillsRoot, builtinSkills: {} }); -Broken. - `); + expect((await service.readForAgent(projectRoot, "codemap", ["codemap"]))?.source).toBe("user"); - const service = new SkillService({ userSkillsRoot }); - let thrown: unknown; - try { - await service.readForAgent(projectRoot, "codemap"); - } catch (error) { - thrown = error; + await writeSkill(projectSkillsRoot, "codemap", skillMarkdown( + "codemap", + "Project mapping when this checkout is under investigation.", + "PROJECT_ENTRY", + ), { "references/project.md": "PROJECT_RESOURCE" }); + + const projectResource = await service.readResourceForAgent( + projectRoot, + "codemap", + "references/project.md", + ["codemap"], + ); + expect(projectResource?.source).toBe("project"); + expect(new TextDecoder().decode(projectResource?.content)).toBe("PROJECT_RESOURCE"); + await expect(service.readResourceForAgent(projectRoot, "codemap", "references/user.md", ["codemap"])) + .rejects.toBeInstanceOf(SkillResourceNotFoundError); + }); + + test("reserved lifecycle builtins are unshadowable and Agent-gated", async () => { + const builtinSkills: Record = {}; + for (const name of RESERVED_BUILTIN_SKILL_NAMES) { + builtinSkills[name] = builtinSkill(name, `BUILTIN_${name}`); + await writeSkill(projectSkillsRoot, name, skillMarkdown( + name, + `Project override when ${name} is activated.`, + `PROJECT_${name}`, + )); + await writeSkill(userSkillsRoot, name, skillMarkdown( + name, + `User override when ${name} is activated.`, + `USER_${name}`, + )); + } + const service = new SkillService({ userSkillsRoot, builtinSkills }); + + for (const name of RESERVED_BUILTIN_SKILL_NAMES) { + const skill = await service.readForAgent(projectRoot, name, [name]); + expect(skill?.source).toBe("builtin"); + expect(skill?.body).toContain(`BUILTIN_${name}`); + expect(await service.readForAgent(projectRoot, name, ["codemap"])).toBeNull(); } - expect(thrown).toMatchObject({ - name: "SkillValidationError", + expect((await service.listForAgent(projectRoot, ["codemap"])).map((entry) => entry.name)) + .not.toEqual(expect.arrayContaining([...RESERVED_BUILTIN_SKILL_NAMES])); + }); + + test("lists custom packages regardless of builtin allow-list and only eligible builtins", async () => { + await writeSkill(projectSkillsRoot, "team-conventions", skillMarkdown( + "team-conventions", + "Applies team conventions when changing this project.", + )); + const service = new SkillService({ + userSkillsRoot, + builtinSkills: { + codemap: builtinSkill("codemap", "CODEMAP"), + "git-master": builtinSkill("git-master", "GIT"), + }, + }); + + expect(await service.listForAgent(projectRoot, ["codemap"])).toEqual([ + { + name: "codemap", + description: "codemap builtin guidance when builtin behavior is needed.", + source: "builtin", + }, + { + name: "team-conventions", + description: "Applies team conventions when changing this project.", + source: "project", + }, + ]); + expect(await service.readForAgent(projectRoot, "git-master", ["codemap"])).toBeNull(); + }); + + test("metadata discovery does not read the body or walk package resources", async () => { + const packageRoot = await writeSkill( + projectSkillsRoot, + "codemap", + skillMarkdown( + "codemap", + "Maps code when repository discovery is needed.", + "VALID_PREFIX", + ), + ); + await Bun.write(join(packageRoot, "SKILL.md"), new Blob([ + skillMarkdown( + "codemap", + "Maps code when repository discovery is needed.", + "VALID_PREFIX", + ), + Uint8Array.from([0xff]), + ])); + await symlink(join(tmpRoot, "outside"), join(packageRoot, "linked-resources")); + const service = new SkillService({ userSkillsRoot, builtinSkills: {} }); + + expect(await service.listForAgent(projectRoot, [])).toEqual([{ + name: "codemap", + description: "Maps code when repository discovery is needed.", source: "project", - skillName: "codemap", - } satisfies Partial); + }]); + await expect(service.readForAgent(projectRoot, "codemap", [])) + .rejects.toMatchObject({ source: "project", skillName: "codemap" }); }); - test("does not fall back when a higher priority skill file cannot be read", async () => { - await mkdir(join(projectSkillsRoot, "codemap", "SKILL.md"), { recursive: true }); + test("ignores non-directory discovery entries and validates directory-name equality", async () => { + await mkdir(projectSkillsRoot, { recursive: true }); + await Bun.write(join(projectSkillsRoot, "plain-file"), skillMarkdown( + "plain-file", + "A plain file that must not be discovered as a package.", + )); + const service = new SkillService({ userSkillsRoot, builtinSkills: {} }); + expect(await service.listForAgent(projectRoot, [])).toEqual([]); - const service = new SkillService({ userSkillsRoot }); - let thrown: unknown; - try { - await service.readForAgent(projectRoot, "codemap"); - } catch (error) { - thrown = error; - } - expect(thrown).toBeDefined(); + await rm(join(projectSkillsRoot, "plain-file")); + await writeSkill(projectSkillsRoot, "directory-name", skillMarkdown( + "different-name", + "A mismatched package when directory identity is checked.", + )); + + await expect(service.listForAgent(projectRoot, [])).rejects.toMatchObject({ + source: "project", + skillName: "directory-name", + }); }); - test("lists all custom Skills plus only eligible builtins", async () => { - await writeSkill(projectSkillsRoot, "safe-refactor", skillMarkdown("safe-refactor", "project safe")); - await writeSkill(userSkillsRoot, "git-master", skillMarkdown("git-master", "user git")); + test("rejects a symlinked project package root through list and direct read", async () => { + const externalPackage = join(tmpRoot, "external", "symlink-skill"); + await writeSkill( + join(tmpRoot, "external"), + "symlink-skill", + skillMarkdown( + "symlink-skill", + "External guidance when symlink boundaries are being checked.", + ), + ); + await mkdir(projectSkillsRoot, { recursive: true }); + await symlink(externalPackage, join(projectSkillsRoot, "symlink-skill"), "dir"); + const service = new SkillService({ userSkillsRoot, builtinSkills: {} }); - const service = new SkillService({ userSkillsRoot }); - const entries = await service.listForAgent(projectRoot, ["git-master", "safe-refactor"]); + await expect(service.listForAgent(projectRoot, [])) + .rejects.toBeInstanceOf(SkillPathError); + await expect(service.readForAgent(projectRoot, "symlink-skill", [])) + .rejects.toBeInstanceOf(SkillPathError); + }); - expect(entries).toEqual([ - { name: "git-master", description: "user git", when_to_use: "Use when git-master is needed.", source: "user", allowed_tools: undefined }, - { name: "safe-refactor", description: "project safe", when_to_use: "Use when safe-refactor is needed.", source: "project", allowed_tools: undefined }, - ]); + test("rejects a project skills-root symlink that points outside the workspace", async () => { + const externalSkillsRoot = join(tmpRoot, "external-skills-root"); + await writeSkill( + externalSkillsRoot, + "escaped-skill", + skillMarkdown( + "escaped-skill", + "External guidance when source-root ancestry is being checked.", + ), + ); + await mkdir(join(projectRoot, ".archcode"), { recursive: true }); + await rm(projectSkillsRoot, { recursive: true, force: true }); + await symlink(externalSkillsRoot, projectSkillsRoot, "dir"); + const service = new SkillService({ userSkillsRoot, builtinSkills: {} }); + + await expect(service.listForAgent(projectRoot, [])) + .rejects.toBeInstanceOf(SkillPathError); + await expect(service.readForAgent(projectRoot, "escaped-skill", [])) + .rejects.toBeInstanceOf(SkillPathError); }); - test("allows a valid custom Skill outside the builtin eligibility list", async () => { - await writeSkill(userSkillsRoot, "team-conventions", skillMarkdown("team-conventions")); - const service = new SkillService({ userSkillsRoot }); + test("rejects a source-root symlink before list discovery reads external names", async () => { + const emptyExternalSkillsRoot = join(tmpRoot, "empty-external-skills-root"); + await mkdir(emptyExternalSkillsRoot, { recursive: true }); + await mkdir(join(projectRoot, ".archcode"), { recursive: true }); + await rm(projectSkillsRoot, { recursive: true, force: true }); + await symlink(emptyExternalSkillsRoot, projectSkillsRoot, "dir"); + const service = new SkillService({ + userSkillsRoot, + builtinSkills: { codemap: builtinSkill("codemap", "BUILTIN") }, + }); - expect((await service.readForAgent(projectRoot, "team-conventions", ["codemap"]))?.source).toBe("user"); - expect(await service.readForAgent(projectRoot, "git-master", ["codemap"])).toBeNull(); + await expect(service.listForAgent(projectRoot, [])) + .rejects.toBeInstanceOf(SkillPathError); + await expect(service.readForAgent(projectRoot, "codemap", ["codemap"])) + .rejects.toBeInstanceOf(SkillPathError); + }); + + test("does not resolve inherited object properties as builtin Skill names", async () => { + const service = new SkillService({ userSkillsRoot, builtinSkills: {} }); + + expect(await service.discoverForAgent(projectRoot, "constructor", ["constructor"])) + .toBeNull(); + expect(await service.readForAgent(projectRoot, "constructor", ["constructor"])) + .toBeNull(); }); }); diff --git a/packages/agent-core/src/skills/service.ts b/packages/agent-core/src/skills/service.ts index fb228f54..edaea1b2 100644 --- a/packages/agent-core/src/skills/service.ts +++ b/packages/agent-core/src/skills/service.ts @@ -1,20 +1,31 @@ import { readdir } from "node:fs/promises"; import { homedir } from "node:os"; -import { join, resolve } from "node:path"; +import { dirname, join, resolve } from "node:path"; import { PROJECT_STATE_DIR_NAME, USER_DATA_DIR_NAME } from "@archcode/protocol"; -import { BUILTIN_SKILL_BODIES } from "./builtin/manifest"; -import { assertSkillName, parseSkillMarkdown } from "./schema"; -import type { ResolvedSkill, SkillIndexEntry, SkillSource } from "./types"; +import { BUILTIN_SKILL_PACKAGES } from "./builtin/manifest"; import { - ONE_SHOT_FILE_READ_MAX_BYTES, - assertUtf8TextWithinLimit, - readUtf8FileBounded, - resolveContainedPath, - SafePathError, -} from "../utils/safe-file"; + activateBuiltinSkill, + activateFilesystemSkill, + assertFilesystemSkillAncestry, + discoverBuiltinSkill, + discoverFilesystemSkill, + filesystemSkillDirectoryExists, + readBuiltinSkillResource, + readFilesystemSkillResource, + SkillPackageResourceNotFoundError, +} from "./package-reader"; +import { assertSkillName } from "./schema"; +import type { + BuiltinSkillPackage, + ResolvedSkill, + ResolvedSkillResource, + SkillIndexEntry, + SkillMetadata, + SkillSource, +} from "./types"; const PROJECT_SKILLS_DIR = join(PROJECT_STATE_DIR_NAME, "skills"); -const SKILL_FILE = "SKILL.md"; + export const RESERVED_BUILTIN_SKILL_NAMES = new Set([ "automation-create", "orchestrate-work", @@ -48,6 +59,16 @@ export class SkillNotFoundError extends Error { } } +export class SkillResourceNotFoundError extends Error { + constructor( + public readonly skillName: string, + public readonly resource: string, + ) { + super(`Skill resource not found: ${skillName}/${resource}`); + this.name = "SkillResourceNotFoundError"; + } +} + export class SkillValidationError extends Error { public readonly skillName: string; public readonly source: SkillSource; @@ -71,23 +92,31 @@ export class SkillValidationError extends Error { } export interface SkillServiceOptions { - userSkillsRoot?: string; - builtinSkills?: Record; + readonly userSkillsRoot?: string; + readonly builtinSkills?: Readonly>; } -interface SkillCandidate { - source: SkillSource; - path?: string; - content?: string; -} +type SkillCandidate = + | { + readonly source: "project" | "user"; + readonly boundaryRoot: string; + readonly root: string; + } + | { + readonly source: "builtin"; + readonly skillPackage: BuiltinSkillPackage; + }; export class SkillService { public readonly userSkillsRoot: string; - readonly #builtinSkills: Record; + readonly #userSkillsBoundaryRoot: string; + readonly #builtinSkills: Readonly>; constructor(options: SkillServiceOptions = {}) { + const defaultUserRoot = options.userSkillsRoot === undefined; this.userSkillsRoot = resolve(options.userSkillsRoot ?? join(homedir(), USER_DATA_DIR_NAME, "skills")); - this.#builtinSkills = options.builtinSkills ?? BUILTIN_SKILL_BODIES; + this.#userSkillsBoundaryRoot = defaultUserRoot ? resolve(homedir()) : dirname(this.userSkillsRoot); + this.#builtinSkills = options.builtinSkills ?? BUILTIN_SKILL_PACKAGES; } async listForAgent( @@ -98,18 +127,22 @@ export class SkillService { const entries: SkillIndexEntry[] = []; for (const name of names) { - const skill = await this.readForAgent(projectRoot, name, allowedNames); - if (skill === null) continue; - entries.push({ - name: skill.metadata.name, - description: skill.metadata.description, - when_to_use: skill.metadata.when_to_use, - source: skill.source, - allowed_tools: skill.metadata.allowed_tools, - }); + const entry = await this.discoverForAgent(projectRoot, name, allowedNames); + if (entry !== null) entries.push(entry); } + return entries.sort((a, b) => lexicalCompare(a.name, b.name)); + } - return entries.sort((a, b) => a.name.localeCompare(b.name)); + async discoverForAgent( + projectRoot: string, + name: string, + allowedNames?: readonly string[], + ): Promise { + assertSkillName(name); + const candidate = await this.#resolveWinningCandidate(projectRoot, name, allowedNames); + if (candidate === null) return null; + const metadata = await this.#discoverCandidate(name, candidate); + return { name: metadata.name, description: metadata.description, source: candidate.source }; } async readForAgent( @@ -118,14 +151,52 @@ export class SkillService { allowedNames?: readonly string[], ): Promise { assertSkillName(name); - - const candidates = await this.#candidates(projectRoot, name, allowedNames); - for (const candidate of candidates) { - if (candidate.content === undefined) continue; - return this.#parseCandidate(name, candidate); + const candidate = await this.#resolveWinningCandidate(projectRoot, name, allowedNames); + if (candidate === null) return null; + try { + const activated = candidate.source === "builtin" + ? activateBuiltinSkill(candidate.skillPackage, name) + : await activateFilesystemSkill(candidate, name); + return { + metadata: activated.metadata, + body: activated.body, + source: candidate.source, + sourceLabel: candidate.source === "builtin" ? "builtin" : candidate.root, + ...(candidate.source === "builtin" ? {} : { root: candidate.root }), + resources: activated.resources, + }; + } catch (error) { + throw this.#validationError(name, candidate, error); } + } - return null; + async readResourceForAgent( + projectRoot: string, + name: string, + resource: string, + allowedNames?: readonly string[], + ): Promise { + assertSkillName(name); + const candidate = await this.#resolveWinningCandidate(projectRoot, name, allowedNames); + if (candidate === null) return null; + try { + const read = candidate.source === "builtin" + ? readBuiltinSkillResource(candidate.skillPackage, name, resource) + : await readFilesystemSkillResource(candidate, name, resource); + return { + skillName: name, + source: candidate.source, + sourceLabel: candidate.source === "builtin" ? "builtin" : candidate.root, + ...(candidate.source === "builtin" ? {} : { root: candidate.root }), + resource: read.descriptor, + content: read.content, + }; + } catch (error) { + if (error instanceof SkillPackageResourceNotFoundError) { + throw new SkillResourceNotFoundError(name, resource); + } + throw this.#validationError(name, candidate, error); + } } async #discoverNames( @@ -134,93 +205,99 @@ export class SkillService { ): Promise { const allowed = allowedNames === undefined ? null : new Set(allowedNames); const names = new Set(); - - for (const root of [this.#projectSkillsRoot(projectRoot), this.userSkillsRoot]) { - for (const name of await this.#listSkillDirs(root)) { - if (RESERVED_BUILTIN_SKILL_NAMES.has(name)) continue; - names.add(name); + const filesystemSources = [ + { boundaryRoot: resolve(projectRoot), root: this.#projectSkillsRoot(projectRoot) }, + { boundaryRoot: this.#userSkillsBoundaryRoot, root: this.userSkillsRoot }, + ] as const; + for (const source of filesystemSources) { + for (const name of await this.#listSkillDirs(source.boundaryRoot, source.root)) { + if (!RESERVED_BUILTIN_SKILL_NAMES.has(name)) names.add(name); } } - for (const name of Object.keys(this.#builtinSkills)) { - if (allowed !== null && !allowed.has(name)) continue; - names.add(name); + if (allowed === null || allowed.has(name)) names.add(name); } - - return [...names].sort(); + return [...names].sort(lexicalCompare); } - async #candidates( + async #resolveWinningCandidate( projectRoot: string, name: string, allowedNames?: readonly string[], - ): Promise { - const builtin = this.#builtinSkills[name]; + ): Promise { + const allowed = allowedNames === undefined || allowedNames.includes(name); + const builtin = Object.hasOwn(this.#builtinSkills, name) + ? this.#builtinSkills[name] + : undefined; if (RESERVED_BUILTIN_SKILL_NAMES.has(name)) { - return allowedNames === undefined || allowedNames.includes(name) - ? [{ source: "builtin", content: builtin }] - : []; + return allowed && builtin !== undefined + ? { source: "builtin", skillPackage: builtin } + : null; } - const projectPath = await this.#resolveSkillPath(this.#projectSkillsRoot(projectRoot), name); - const userPath = await this.#resolveSkillPath(this.userSkillsRoot, name); - return [ - { source: "project", path: projectPath, content: await this.#readFileOrUndefined(projectPath) }, - { source: "user", path: userPath, content: await this.#readFileOrUndefined(userPath) }, - ...(allowedNames === undefined || allowedNames.includes(name) - ? [{ source: "builtin" as const, content: builtin }] - : []), - ]; + const projectPackageRoot = resolve(this.#projectSkillsRoot(projectRoot), name); + const projectCandidate = { + source: "project" as const, + boundaryRoot: resolve(projectRoot), + root: projectPackageRoot, + }; + if (await this.#filesystemCandidateExists(projectCandidate)) { + return projectCandidate; + } + const userPackageRoot = resolve(this.userSkillsRoot, name); + const userCandidate = { + source: "user" as const, + boundaryRoot: this.#userSkillsBoundaryRoot, + root: userPackageRoot, + }; + if (await this.#filesystemCandidateExists(userCandidate)) { + return userCandidate; + } + return allowed && builtin !== undefined + ? { source: "builtin", skillPackage: builtin } + : null; } - #parseCandidate(requestedName: string, candidate: SkillCandidate): ResolvedSkill { + async #discoverCandidate(name: string, candidate: SkillCandidate): Promise { try { - const content = candidate.content; - if (content === undefined) throw new Error("Skill content is missing"); - assertUtf8TextWithinLimit(content, ONE_SHOT_FILE_READ_MAX_BYTES); - const { metadata, body } = parseSkillMarkdown(content); - if (metadata.name !== requestedName) { - throw new Error( - `frontmatter.name must match requested skill "${requestedName}" (received "${metadata.name}")`, - ); - } - return { - metadata, - body, - source: candidate.source, - path: candidate.path, - }; + return candidate.source === "builtin" + ? discoverBuiltinSkill(candidate.skillPackage, name) + : await discoverFilesystemSkill(candidate, name); } catch (error) { - throw new SkillValidationError({ - skillName: requestedName, - source: candidate.source, - path: candidate.path, - message: error instanceof Error ? error.message : String(error), - cause: error, - }); + throw this.#validationError(name, candidate, error); } } - #projectSkillsRoot(projectRoot: string): string { - return resolve(projectRoot, PROJECT_SKILLS_DIR); + #validationError(name: string, candidate: SkillCandidate, error: unknown): SkillValidationError { + return new SkillValidationError({ + skillName: name, + source: candidate.source, + ...(candidate.source === "builtin" ? {} : { path: candidate.root }), + message: error instanceof Error ? error.message : String(error), + cause: error, + }); } - async #resolveSkillPath(root: string, name: string): Promise { + async #filesystemCandidateExists( + candidate: Extract, + ): Promise { try { - return await resolveContainedPath(join(name, SKILL_FILE), root); + return await filesystemSkillDirectoryExists(candidate); } catch (error) { - if (error instanceof SafePathError) { - throw new SkillPathError(error.path, error.reason); - } - throw error; + throw new SkillPathError(candidate.root, error instanceof Error ? error.message : String(error)); } } - async #listSkillDirs(root: string): Promise { + #projectSkillsRoot(projectRoot: string): string { + return resolve(projectRoot, PROJECT_SKILLS_DIR); + } + + async #listSkillDirs(boundaryRoot: string, root: string): Promise { try { + await assertFilesystemSkillAncestry({ boundaryRoot, root }); const entries = await readdir(root, { withFileTypes: true }); - return entries - .filter((entry) => entry.isDirectory()) + const names = entries + .filter((entry) => entry.isDirectory() || entry.isSymbolicLink()) .map((entry) => entry.name) .filter((name) => { try { @@ -230,18 +307,12 @@ export class SkillService { return false; } }) - .sort(); - } catch { - return []; - } - } - - async #readFileOrUndefined(filePath: string): Promise { - try { - return await readUtf8FileBounded(filePath, ONE_SHOT_FILE_READ_MAX_BYTES); + .sort(lexicalCompare); + await assertFilesystemSkillAncestry({ boundaryRoot, root }); + return names; } catch (error) { - if (isNoEntryError(error)) return undefined; - throw error; + if (isNoEntryError(error)) return []; + throw new SkillPathError(root, error instanceof Error ? error.message : String(error)); } } } @@ -249,3 +320,7 @@ export class SkillService { function isNoEntryError(error: unknown): boolean { return error !== null && typeof error === "object" && "code" in error && error.code === "ENOENT"; } + +function lexicalCompare(a: string, b: string): number { + return a < b ? -1 : a > b ? 1 : 0; +} diff --git a/packages/agent-core/src/skills/types.ts b/packages/agent-core/src/skills/types.ts index 038c4feb..2d05b406 100644 --- a/packages/agent-core/src/skills/types.ts +++ b/packages/agent-core/src/skills/types.ts @@ -1,23 +1,45 @@ export type SkillSource = "project" | "user" | "builtin"; export interface SkillMetadata { - name: string; - description: string; - when_to_use: string; - allowed_tools?: string[]; + readonly name: string; + readonly description: string; + readonly license?: string; + readonly compatibility?: string; + readonly metadata?: Readonly>; +} + +export interface SkillResourceDescriptor { + readonly path: string; + readonly bytes: number; } export interface ResolvedSkill { - metadata: SkillMetadata; - body: string; - source: SkillSource; - path?: string; + readonly metadata: SkillMetadata; + readonly body: string; + readonly source: SkillSource; + readonly sourceLabel: string; + /** Absolute package root for project/user Skills. Embedded builtins intentionally have no pretend path. */ + readonly root?: string; + readonly resources: readonly SkillResourceDescriptor[]; +} + +export interface ResolvedSkillResource { + readonly skillName: string; + readonly source: SkillSource; + readonly sourceLabel: string; + readonly root?: string; + readonly resource: SkillResourceDescriptor; + readonly content: Uint8Array; } export interface SkillIndexEntry { - name: string; - description: string; - when_to_use: string; - source: SkillSource; - allowed_tools?: string[]; + readonly name: string; + readonly description: string; + readonly source: SkillSource; +} + +/** Complete embedded package. Resource values preserve bytes; no filesystem fallback exists. */ +export interface BuiltinSkillPackage { + readonly entry: string; + readonly resources: Readonly>; } diff --git a/packages/agent-core/src/tools/builtins/model-visible-contract.test.ts b/packages/agent-core/src/tools/builtins/model-visible-contract.test.ts index 48c2ef42..94681c1f 100644 --- a/packages/agent-core/src/tools/builtins/model-visible-contract.test.ts +++ b/packages/agent-core/src/tools/builtins/model-visible-contract.test.ts @@ -326,15 +326,16 @@ const CONTRACTS: readonly ModelVisibleContract[] = [ tool: "skill_list", competitorEvidenceIds: ["CC-160-A:Skill", "OC:skill"], runtimeSourceIds: ["tools/builtins/skill-list.ts:6-35"], - descriptionPatterns: [/currently allowed for this Agent/i, /System Prompt normally already lists the same allowed metadata/i, /fresh machine-readable copy/i, /call skill_read directly/i, /skill_list\(\{\}\)/, /exact returned name/i, /Never guess or invent/i], + descriptionPatterns: [/currently allowed for this Agent/i, /System Prompt normally already lists the same allowed metadata/i, /fresh machine-readable copy/i, /call skill_read directly/i, /skill_list\(\{\}\)/, /exact returned name/i, /Never guess or invent/i, /exactly name, description, and source/i, /resource contents are omitted/i], }, { tool: "skill_read", competitorEvidenceIds: ["CC-160-A:Skill", "OC:skill"], runtimeSourceIds: ["tools/builtins/skill-read.ts:10-14,83-105"], - descriptionPatterns: [/allowed.*Agent/i, /available names are already listed in the System Prompt/i, /skill_read\(/, /Read the Skill before the work it governs/i, /Do not load unrelated Skills/i, /cannot expand/i, /permissions/, /workspace/], + descriptionPatterns: [/allowed.*Agent/i, /available names are already listed in the System Prompt/i, /skill_read\(/, /metadata, filesystem root when available, sorted resource descriptors, and entry body/i, /exactly one listed UTF-8 text resource/i, /unsupported-binary error/i, /Read the Skill before the work it governs/i, /supporting resources only when needed/i, /Do not load unrelated Skills/i, /cannot expand/i, /permissions/, /workspace/], schema: [ { path: ["properties", "name"], descriptionPatterns: [/System Prompt's available-skill list or skill_list/i, /exact/i] }, + { path: ["properties", "resource"], descriptionPatterns: [/Skill-root-relative/i, /Resources list/i, /cannot.*arbitrary filesystem path/i] }, ], }, { diff --git a/packages/agent-core/src/tools/builtins/skill-list.test.ts b/packages/agent-core/src/tools/builtins/skill-list.test.ts index 9224b9d3..ee4aeca2 100644 --- a/packages/agent-core/src/tools/builtins/skill-list.test.ts +++ b/packages/agent-core/src/tools/builtins/skill-list.test.ts @@ -61,7 +61,7 @@ describe("skill_list tool", () => { for (const entry of entries) { expect(entry.description.length).toBeGreaterThan(0); expect(entry.source).toBe("builtin"); - expect("body" in entry).toBe(false); + expect(Object.keys(entry).sort()).toEqual(["description", "name", "source"]); } }); diff --git a/packages/agent-core/src/tools/builtins/skill-list.ts b/packages/agent-core/src/tools/builtins/skill-list.ts index 6a6ba08c..224970cb 100644 --- a/packages/agent-core/src/tools/builtins/skill-list.ts +++ b/packages/agent-core/src/tools/builtins/skill-list.ts @@ -14,7 +14,7 @@ export function createSkillListTool() { description: [ "Discover the Skills currently allowed for this Agent. The System Prompt normally already lists the same allowed metadata; call skill_list only when you need a fresh machine-readable copy, and call skill_read directly when an exact matching Skill is already visible.", "", - "Call `skill_list({})`, inspect each returned name, description, and usage guidance, choose only an exact returned name, then call `skill_read({\"name\":\"\"})` before doing the governed work. Never guess or invent a Skill name. The result is metadata-only JSON with source and optional allowed-tools declarations; Skill bodies are omitted. An empty list means no Skill is available to this Agent.", + "Call `skill_list({})`, inspect each returned name, description, and source, choose only an exact returned name, then call `skill_read({\"name\":\"\"})` before doing the governed work. Never guess or invent a Skill name. The result is metadata-only JSON containing exactly name, description, and source; Skill bodies and resource contents are omitted. An empty list means no Skill is available to this Agent.", ].join("\n"), inputSchema: SkillListInputSchema, traits: { readOnly: true, destructive: false, concurrencySafe: true }, diff --git a/packages/agent-core/src/tools/builtins/skill-read.test.ts b/packages/agent-core/src/tools/builtins/skill-read.test.ts index 213c8e08..ded40bd1 100644 --- a/packages/agent-core/src/tools/builtins/skill-read.test.ts +++ b/packages/agent-core/src/tools/builtins/skill-read.test.ts @@ -1,7 +1,7 @@ import { afterAll, beforeEach, describe, expect, test } from "bun:test"; import { mkdir, rm } from "node:fs/promises"; import { tmpdir } from "node:os"; -import { join } from "node:path"; +import { dirname, join } from "node:path"; import { SkillService } from "../../skills"; import { storeManager } from "../../store/store"; import { createMockStore } from "../../store/test-helpers"; @@ -9,8 +9,7 @@ import { createTestProjectContext } from "../test-project-context"; import { expectTextDraft } from "../test-results"; import { createToolExecutionContext, type ToolExecutionContext } from "../types"; import { createBuiltinToolDescriptors } from "./index"; -import { SkillReadInputSchema, skillReadTool } from "./skill-read"; -import { ONE_SHOT_FILE_READ_MAX_BYTES } from "../../utils/safe-file"; +import { formatResolvedSkillResource, SkillReadInputSchema, skillReadTool } from "./skill-read"; const tmpRoot = join(tmpdir(), "archcode-skill-read-tool", crypto.randomUUID()); const projectRoot = join(tmpRoot, "project"); @@ -36,10 +35,19 @@ function makeContext(agentSkills: readonly string[], cwd = projectRoot): ToolExe cwd, }); } -async function writeProjectSkill(name: string, content: string): Promise { +async function writeProjectSkill( + name: string, + content: string, + resources: Readonly> = {}, +): Promise { const skillDir = join(projectSkillsRoot, name); await mkdir(skillDir, { recursive: true }); await Bun.write(join(skillDir, "SKILL.md"), content); + for (const [path, resource] of Object.entries(resources)) { + const destination = join(skillDir, path); + await mkdir(dirname(destination), { recursive: true }); + await Bun.write(destination, resource); + } } async function writeExecutionSkill(name: string, content: string): Promise { @@ -59,30 +67,148 @@ describe("skill_read tool", () => { await rm(tmpRoot, { recursive: true, force: true }); }); - test("allowed skill returns full metadata and body content", async () => { + test("entry read returns fixed metadata, root, sorted descriptors, and body without resource contents", async () => { + await writeProjectSkill("codemap", `--- +name: codemap +description: Maps code architecture and entry points when investigating an unfamiliar repository. +license: MIT +compatibility: Requires repository source access. +metadata: + zeta: last + alpha: first +--- + +ENTRY_BODY +`, { + "references/z-last.md": "RESOURCE_Z", + "references/a-first.md": "RESOURCE_A", + }); + const result = await skillReadTool.execute({ name: "codemap" }, makeContext(["codemap"])); const output = expectTextDraft(result); expect(output).toContain("---\nname: codemap"); - expect(output).toContain("description:"); - expect(output).toContain("when_to_use:"); - expect(output).toContain("source: builtin"); - expect(output).toContain("Trace entry points"); + expect(output).toContain(`source: ${join(projectSkillsRoot, "codemap")}`); + expect(output).toContain(`root: ${join(projectSkillsRoot, "codemap")}`); + expect(output).toContain("license: MIT"); + expect(output).toContain("compatibility: Requires repository source access."); + expect(output).toContain('metadata: {"alpha":"first","zeta":"last"}'); + const headerKeys = output.split("---\n", 2)[1]! + .trimEnd() + .split("\n") + .map((line) => line.slice(0, line.indexOf(":"))); + expect(headerKeys).toEqual([ + "name", + "description", + "source", + "root", + "license", + "compatibility", + "metadata", + ]); + expect(output.indexOf("references/a-first.md")).toBeLessThan(output.indexOf("references/z-last.md")); + expect(output).toContain("ENTRY_BODY"); + expect(output).not.toContain("RESOURCE_A"); + expect(output).not.toContain("RESOURCE_Z"); + }); + + test("entry read emits Resources: none for a package without supporting files", async () => { + await writeProjectSkill("codemap", `--- +name: codemap +description: Maps code architecture when investigating an unfamiliar repository. +--- + +ENTRY_BODY +`); + + const result = await skillReadTool.execute({ name: "codemap" }, makeContext(["codemap"])); + expect(expectTextDraft(result)).toContain("Resources: none\n\n\nENTRY_BODY\n"); + }); + + test("resource read returns exactly one UTF-8 resource with a fixed identity header", async () => { + await writeProjectSkill("codemap", `--- +name: codemap +description: Maps code architecture when investigating an unfamiliar repository. +--- + +ENTRY_BODY +`, { + "references/guide.md": "RESOURCE_TEXT\n", + "references/other.md": "OTHER_TEXT", + }); + + const result = await skillReadTool.execute( + { name: "codemap", resource: "references/guide.md" }, + makeContext(["codemap"]), + ); + + expect(expectTextDraft(result)).toBe([ + "---", + "skill: codemap", + `source: ${join(projectSkillsRoot, "codemap")}`, + "resource: references/guide.md", + "bytes: 14", + "---", + "", + "RESOURCE_TEXT\n", + ].join("\n")); + expect(expectTextDraft(result)).not.toContain("ENTRY_BODY"); + expect(expectTextDraft(result)).not.toContain("OTHER_TEXT"); + }); + + test("binary resource read returns deterministic unsupported-binary identity and error", async () => { + const result = formatResolvedSkillResource({ + skillName: "codemap", + source: "builtin", + sourceLabel: "builtin", + resource: { path: "assets/image.bin", bytes: 3 }, + content: Uint8Array.from([0xff, 0xfe, 0xfd]), + }); + + expect(result.isError).toBe(true); + expect(result.details?.error?.code).toBe("TOOL_SKILL_RESOURCE_BINARY_UNSUPPORTED"); + expect(expectTextDraft(result)).toBe([ + "---", + "skill: codemap", + "source: builtin", + "resource: assets/image.bin", + "bytes: 3", + "---", + "", + "error: TOOL_SKILL_RESOURCE_BINARY_UNSUPPORTED", + "hint: Binary Skill resources are valid package assets but cannot be returned by the text-only skill_read tool.", + ].join("\n")); + }); + + test("unknown resource returns a structured error without lower-source fallback", async () => { + await writeProjectSkill("codemap", `--- +name: codemap +description: Maps code architecture when investigating an unfamiliar repository. +--- + +ENTRY_BODY +`); + + const result = await skillReadTool.execute( + { name: "codemap", resource: "references/missing.md" }, + makeContext(["codemap"]), + ); + + expect(result.isError).toBe(true); + expect(result.details?.error?.code).toBe("TOOL_SKILL_RESOURCE_NOT_FOUND"); }); test("resolves project-local Skills from execution cwd, not canonical project root", async () => { await writeProjectSkill("codemap", `--- name: codemap -description: canonical marker -when_to_use: Test canonical Skill resolution. +description: Maps the canonical checkout when testing Skill resolution. --- CANONICAL_SKILL_BODY `); await writeExecutionSkill("codemap", `--- name: codemap -description: worktree marker -when_to_use: Test worktree Skill resolution. +description: Maps the execution worktree when testing Skill resolution. --- WORKTREE_SKILL_BODY @@ -147,29 +273,18 @@ Broken body. expect(result.details?.error).toBeDefined(); }); - test("rejects a Skill one byte over the one-shot file cap without partial fallback", async () => { - const header = `---\nname: codemap\ndescription: oversized\nwhen_to_use: boundary test\n---\n\n`; - await writeProjectSkill( - "codemap", - header + "x".repeat(ONE_SHOT_FILE_READ_MAX_BYTES - new TextEncoder().encode(header).byteLength + 1), - ); - - const result = await skillReadTool.execute({ name: "codemap" }, makeContext(["codemap"])); - expect(result.isError).toBe(true); - expect(result.details?.error?.code).toBe("TOOL_OUTPUT_POLICY_VIOLATION"); - expect(expectTextDraft(result)).not.toContain("x".repeat(1_024)); - }); - - test("input schema rejects unknown keys including agent, role, source, and path", () => { + test("input schema accepts an optional listed resource and rejects authority overrides", () => { expect(SkillReadInputSchema.safeParse({ name: "codemap" }).success).toBe(true); + expect(SkillReadInputSchema.safeParse({ name: "codemap", resource: "references/guide.md" }).success).toBe(true); expect(SkillReadInputSchema.safeParse({ name: "codemap", agentName: "lead" }).success).toBe(false); expect(SkillReadInputSchema.safeParse({ name: "codemap", role: "builder" }).success).toBe(false); expect(SkillReadInputSchema.safeParse({ name: "codemap", source: "builtin" }).success).toBe(false); expect(SkillReadInputSchema.safeParse({ name: "codemap", path: "/tmp/SKILL.md" }).success).toBe(false); + expect(SkillReadInputSchema.safeParse({ name: "codemap", resource: "" }).success).toBe(false); }); test("input schema rejects invalid skill names", () => { - for (const invalidName of ["../x", "Git-Master", ""]) { + for (const invalidName of ["../x", "Git-Master", "double--hyphen", "trailing-", ""]) { expect(SkillReadInputSchema.safeParse({ name: invalidName }).success).toBe(false); } }); diff --git a/packages/agent-core/src/tools/builtins/skill-read.ts b/packages/agent-core/src/tools/builtins/skill-read.ts index 94b0f48d..bd7f724c 100644 --- a/packages/agent-core/src/tools/builtins/skill-read.ts +++ b/packages/agent-core/src/tools/builtins/skill-read.ts @@ -3,15 +3,22 @@ import { defineTool } from "../define-tool"; import { createToolErrorResult } from "../errors"; import { createTextToolResult } from "../results"; import type { RawToolResult, ToolExecutionContext } from "../types"; -import { SkillNotFoundError, SkillPathError, SkillValidationError, type ResolvedSkill } from "../../skills"; +import { + SkillNotFoundError, + SkillPathError, + SkillResourceNotFoundError, + SkillValidationError, + type ResolvedSkill, + type ResolvedSkillResource, +} from "../../skills"; import { SKILL_NAME_REGEX } from "../../skills/schema"; -import { BoundedFileReadError, ONE_SHOT_FILE_READ_MAX_BYTES } from "../../utils/safe-file"; -const SKILL_NAME_MESSAGE = "Skill name must match pattern ^[a-z0-9][a-z0-9-]*$"; +const SKILL_NAME_MESSAGE = "Skill name must match pattern ^[a-z0-9]+(?:-[a-z0-9]+)*$"; export const SkillReadInputSchema = z .object({ - name: z.string().regex(SKILL_NAME_REGEX, SKILL_NAME_MESSAGE).describe("Exact allowed Skill name matching ^[a-z0-9][a-z0-9-]*$; copy it from the System Prompt's available-skill list or skill_list instead of guessing."), + name: z.string().regex(SKILL_NAME_REGEX, SKILL_NAME_MESSAGE).describe("Exact allowed Skill name matching ^[a-z0-9]+(?:-[a-z0-9]+)*$; copy it from the System Prompt's available-skill list or skill_list instead of guessing."), + resource: z.string().min(1).optional().describe("Optional Skill-root-relative resource path copied exactly from the entry's Resources list, for example references/review-packet.md. It cannot select a source or read an arbitrary filesystem path."), }) .strict(); @@ -22,31 +29,60 @@ export function formatResolvedSkill(skill: ResolvedSkill): string { "---", `name: ${skill.metadata.name}`, `description: ${skill.metadata.description}`, - `when_to_use: ${skill.metadata.when_to_use}`, - `source: ${skill.source}`, + `source: ${skill.sourceLabel}`, ]; - if (skill.metadata.allowed_tools !== undefined) { - headerLines.push(`allowed_tools: ${JSON.stringify(skill.metadata.allowed_tools)}`); + if (skill.root !== undefined) { + headerLines.push(`root: ${skill.root}`); + } + if (skill.metadata.license !== undefined) headerLines.push(`license: ${skill.metadata.license}`); + if (skill.metadata.compatibility !== undefined) { + headerLines.push(`compatibility: ${skill.metadata.compatibility}`); + } + if (skill.metadata.metadata !== undefined) { + const metadata = Object.fromEntries(Object.entries(skill.metadata.metadata).sort(([a], [b]) => lexicalCompare(a, b))); + headerLines.push(`metadata: ${JSON.stringify(metadata)}`); } headerLines.push("---"); - return [headerLines.join("\n"), skill.body].join("\n\n"); + const resources = [...skill.resources] + .sort((a, b) => lexicalCompare(a.path, b.path)) + .map((resource) => `- ${resource.path} (${resource.bytes} bytes)`); + const resourceSection = resources.length === 0 + ? "Resources: none" + : `Resources:\n${resources.join("\n")}`; + return [headerLines.join("\n"), resourceSection, skill.body].join("\n\n"); } -function skillReadError(error: unknown, name: string): RawToolResult { - const boundedReadError = error instanceof BoundedFileReadError - ? error - : error instanceof SkillValidationError && error.cause instanceof BoundedFileReadError - ? error.cause - : undefined; - if (boundedReadError !== undefined) { - return createToolErrorResult({ - kind: "execution", - code: "TOOL_OUTPUT_POLICY_VIOLATION", - message: `Skill exceeds the ${ONE_SHOT_FILE_READ_MAX_BYTES}-byte one-shot read limit`, - name: boundedReadError.name, +export function formatResolvedSkillResource(resource: ResolvedSkillResource): RawToolResult { + const identity = [ + "---", + `skill: ${resource.skillName}`, + `source: ${resource.sourceLabel}`, + `resource: ${resource.resource.path}`, + `bytes: ${resource.resource.bytes}`, + "---", + ].join("\n"); + + try { + const text = new TextDecoder("utf-8", { fatal: true }).decode(resource.content); + return createTextToolResult(`${identity}\n\n${text}`); + } catch { + const code = "TOOL_SKILL_RESOURCE_BINARY_UNSUPPORTED"; + const hint = "Binary Skill resources are valid package assets but cannot be returned by the text-only skill_read tool."; + return createTextToolResult(`${identity}\n\nerror: ${code}\nhint: ${hint}`, { + isError: true, + details: { + error: { + kind: "execution", + code, + name: "SkillResourceBinaryUnsupportedError", + hint, + }, + }, }); } +} +function skillReadError(error: unknown, name: string): RawToolResult { if (error instanceof SkillNotFoundError) { return createToolErrorResult({ kind: "file-not-found", @@ -55,6 +91,14 @@ function skillReadError(error: unknown, name: string): RawToolResult { }); } + if (error instanceof SkillResourceNotFoundError) { + return createToolErrorResult({ + kind: "file-not-found", + code: "TOOL_SKILL_RESOURCE_NOT_FOUND", + message: `Skill resource not found or not allowed for current agent: ${error.skillName}/${error.resource}`, + }); + } + if (error instanceof SkillValidationError) { return createToolErrorResult({ kind: "execution", @@ -73,7 +117,7 @@ function skillReadError(error: unknown, name: string): RawToolResult { }); } - if (error instanceof Error && error.message.includes("Skill name must match")) { + if (error instanceof Error && error.message.includes("Skill name must")) { return createToolErrorResult({ kind: "execution", code: "TOOL_SKILL_INVALID_NAME", @@ -94,9 +138,9 @@ export function createSkillReadTool() { return defineTool({ name: "skill_read", description: [ - "Load the full body of one Skill allowed for the current Agent when its description or when-to-use guidance matches the task. The available names are already listed in the System Prompt when discovery succeeded; otherwise call skill_list. Use an exact visible name, for example `skill_read({\"name\":\"git-master\"})` only when `git-master` appears in that list.", + "Load one Skill allowed for the current Agent. `skill_read({\"name\":\"git-master\"})` returns its metadata, filesystem root when available, sorted resource descriptors, and entry body. `skill_read({\"name\":\"git-master\",\"resource\":\"references/example.md\"})` returns exactly one listed UTF-8 text resource; binary assets return a deterministic unsupported-binary error. The available names are already listed in the System Prompt when discovery succeeded; otherwise call skill_list. Use an exact visible name only.", "", - "Read the Skill before the work it governs, then follow its workflow and referenced resources. Do not load unrelated Skills for ceremony. This tool accepts no agent, role, source, or path override. Skill instructions guide existing capabilities but cannot expand the Agent's tools, permissions, delegation targets, or workspace scope.", + "Read the Skill before the work it governs, then load supporting resources only when needed. Copy resource paths from the entry's Resources list; they are Skill-root-relative and cannot read arbitrary filesystem paths. Do not load unrelated Skills for ceremony. This tool accepts no agent, role, source, or filesystem-root override. Skill instructions guide existing capabilities but cannot expand the Agent's tools, permissions, delegation targets, or workspace scope.", ].join("\n"), inputSchema: SkillReadInputSchema, traits: { readOnly: true, destructive: false, concurrencySafe: true }, @@ -113,6 +157,22 @@ export function createSkillReadTool() { }); } try { + if (input.resource !== undefined) { + const resource = await ctx.skillService.readResourceForAgent( + ctx.cwd, + input.name, + input.resource, + ctx.agentSkills, + ); + if (resource === null) { + return createToolErrorResult({ + kind: "file-not-found", + code: "TOOL_SKILL_RESOURCE_NOT_FOUND", + message: `Skill resource not found or not allowed for current agent: ${input.name}/${input.resource}`, + }); + } + return formatResolvedSkillResource(resource); + } const skill = await ctx.skillService.readForAgent(ctx.cwd, input.name, ctx.agentSkills); if (skill === null) { return createToolErrorResult({ @@ -130,3 +190,7 @@ export function createSkillReadTool() { } export const skillReadTool = createSkillReadTool(); + +function lexicalCompare(a: string, b: string): number { + return a < b ? -1 : a > b ? 1 : 0; +} From 8f926badf1ca40300ec10408ed62e9d157fe4dd9 Mon Sep 17 00:00:00 2001 From: bo Date: Sun, 9 Aug 2026 00:27:32 +0800 Subject: [PATCH 2/2] fix(skills): address review findings Restore historical docs, tighten filesystem package identity checks, and reject case-aliased Skill entries. Correct skill_read error semantics and expand package, schema, manifest, path-safety, and standalone integration coverage. --- docs/agents/multi-agent-design.md | 15 -- docs/concepts.md | 14 +- .../builtin-standalone.integration.test.ts | 175 ++++++++++-------- .../references/schedule-examples.md | 2 +- .../references/evidence-map-example.md | 14 +- .../src/skills/builtin/git-master/SKILL.md | 3 +- .../git-master/references/operation-safety.md | 4 +- .../src/skills/builtin/manifest.test.ts | 6 + .../skills/builtin/orchestrate-work/SKILL.md | 2 +- .../src/skills/package-reader.test.ts | 57 +++++- .../agent-core/src/skills/package-reader.ts | 100 +++++++++- packages/agent-core/src/skills/schema.test.ts | 21 +++ .../agent-core/src/skills/service.test.ts | 10 +- .../src/tools/builtins/skill-read.test.ts | 35 +++- .../src/tools/builtins/skill-read.ts | 9 +- 15 files changed, 327 insertions(+), 140 deletions(-) diff --git a/docs/agents/multi-agent-design.md b/docs/agents/multi-agent-design.md index 9cc56670..4c113113 100644 --- a/docs/agents/multi-agent-design.md +++ b/docs/agents/multi-agent-design.md @@ -43,21 +43,6 @@ Discussion ─┬─ Explore Stable Agent prompts describe identity and authority. Workflow methods live in Skills, including `orchestrate-work`, `plan-work`, `execute-plan`, `run-goal`, `shape-todo`, `review-work`, and `goal-review`. Analyst can combine analysis and review Skills without creating a new Agent identity for every professional role. -A Skill is a standard local package: required `SKILL.md`; optional -`scripts/`, `references/`, `assets/`, and other contained resources. Its -frontmatter accepts only `name`, `description`, `license`, `compatibility`, and -`metadata`; `description` contains both the method and activation timing. -`skill_list` and Prompt discovery expose metadata only. `skill_read` then loads -the entry and its resource descriptors, and can load exactly one listed text -resource on demand. Project > user > builtin is whole-package precedence; no -entry or resource is merged from a lower source, and reserved lifecycle -builtins remain unshadowable. - -The package mechanism changes disclosure and storage only. A Skill cannot add -tools, execute a script automatically, change Profiles or MCP access, widen -workspace scope, change delegation, or grant completion authority. A script, -when applicable, is run only through the Agent's existing Bash permission. - A Plan is an ordinary Markdown file under `.archcode/plans/`, not a service, state machine, Session identity, or Goal dependency. ## Sessions, Todos, and Goals diff --git a/docs/concepts.md b/docs/concepts.md index a2df9535..d3e84f32 100644 --- a/docs/concepts.md +++ b/docs/concepts.md @@ -109,18 +109,8 @@ Root Lead and Discussion default to `principal`; Analyst uses `deep`; Explore and Librarian use `fast`; Build can use `deep` or `fast`. Profiles do not change Agent tools or authority. -Skills are task-specific working methods packaged as a directory: a required -`SKILL.md` entry plus optional `scripts/`, `references/`, `assets/`, and other -resources. The entry has standard YAML frontmatter (`name`, `description`, and -optional `license`, `compatibility`, `metadata`); its description says both -what the Skill does and when to use it. - -Discovery exposes only a Skill's name, description, and source. Activating it -loads its entry and a resource list; an Agent reads one listed resource only -when needed. Project packages override user packages, which override builtin -packages, as whole packages. Reserved lifecycle builtins cannot be shadowed. -Skills guide behavior without granting tools, permissions, delegation, -Profiles, MCP access, workspace scope, or completion authority. +Skills provide task-specific working methods. They guide behavior without +granting additional tools or permissions. ## Approvals and questions diff --git a/packages/agent-core/src/skills/builtin-standalone.integration.test.ts b/packages/agent-core/src/skills/builtin-standalone.integration.test.ts index 27684a16..ec3e6863 100644 --- a/packages/agent-core/src/skills/builtin-standalone.integration.test.ts +++ b/packages/agent-core/src/skills/builtin-standalone.integration.test.ts @@ -1,95 +1,96 @@ -import { expect, setDefaultTimeout, test } from "bun:test"; -import { mkdtemp, rm } from "node:fs/promises"; -import { tmpdir } from "node:os"; +import { afterAll, expect, setDefaultTimeout, test } from "bun:test"; +import { mkdir, rm } from "node:fs/promises"; import { join } from "node:path"; setDefaultTimeout(120_000); const repositoryRoot = join(import.meta.dir, "../../../.."); -const fixturePath = join(repositoryRoot, "apps/web/public/favicon.ico"); +const fixturePath = join(import.meta.dir, "../../package.json"); const skillsEntrypoint = join(import.meta.dir, "index.ts"); const skillReadModule = join(import.meta.dir, "../tools/builtins/skill-read.ts"); +const tempRoot = join(import.meta.dir, "__test_tmp__", "builtin-standalone", crypto.randomUUID()); + +afterAll(async () => { + await rm(tempRoot, { recursive: true, force: true }); +}); test("standalone binary preserves real builtin resources and arbitrary embedded bytes", async () => { - const tempRoot = await mkdtemp(join(tmpdir(), "archcode-skill-standalone-")); - try { - const sourceFixtureBytes = await Bun.file(fixturePath).bytes(); - const sourceFixtureDigest = sha256(sourceFixtureBytes); - expect(() => new TextDecoder("utf-8", { fatal: true }).decode(sourceFixtureBytes)).toThrow(); + await mkdir(tempRoot, { recursive: true }); + const sourceSeedBytes = await Bun.file(fixturePath).bytes(); + const sourceFixtureBytes = appendInvalidUtf8Byte(sourceSeedBytes); + const sourceFixtureDigest = sha256(sourceFixtureBytes); + expect(() => new TextDecoder("utf-8", { fatal: true }).decode(sourceFixtureBytes)).toThrow(); - const entrypoint = join(tempRoot, "main.ts"); - const executable = join(tempRoot, "skill-smoke"); - await Bun.write(entrypoint, standaloneSource({ - fixturePath, - skillsEntrypoint, - skillReadModule, - })); + const entrypoint = join(tempRoot, "main.ts"); + const executable = join(tempRoot, "skill-smoke"); + await Bun.write(entrypoint, standaloneSource({ + fixturePath, + skillsEntrypoint, + skillReadModule, + })); - const compiler = Bun.spawn([ - "bun", - "build", - entrypoint, - "--target=bun", - "--minify", - "--compile", - `--outfile=${executable}`, - ], { - cwd: repositoryRoot, - stdout: "pipe", - stderr: "pipe", - }); - const [compileExitCode, compileStdout, compileStderr] = await Promise.all([ - compiler.exited, - new Response(compiler.stdout).text(), - new Response(compiler.stderr).text(), - ]); - if (compileExitCode !== 0) { - throw new Error([ - `Standalone Skill smoke compilation exited ${compileExitCode}`, - compileStdout, - compileStderr, - ].filter(Boolean).join("\n")); - } + const compiler = Bun.spawn([ + "bun", + "build", + entrypoint, + "--target=bun", + "--minify", + "--compile", + `--outfile=${executable}`, + ], { + cwd: repositoryRoot, + stdout: "pipe", + stderr: "pipe", + }); + const [compileExitCode, compileStdout, compileStderr] = await Promise.all([ + compiler.exited, + new Response(compiler.stdout).text(), + new Response(compiler.stderr).text(), + ]); + if (compileExitCode !== 0) { + throw new Error([ + `Standalone Skill smoke compilation exited ${compileExitCode}`, + compileStdout, + compileStderr, + ].filter(Boolean).join("\n")); + } - const process = Bun.spawn([executable], { - cwd: tempRoot, - stdout: "pipe", - stderr: "pipe", - env: {}, - }); - const [exitCode, stdout, stderr] = await Promise.all([ - process.exited, - new Response(process.stdout).text(), - new Response(process.stderr).text(), - ]); - expect(exitCode).toBe(0); - expect(stderr).toBe(""); + const process = Bun.spawn([executable], { + cwd: tempRoot, + stdout: "pipe", + stderr: "pipe", + env: {}, + }); + const [exitCode, stdout, stderr] = await Promise.all([ + process.exited, + new Response(process.stdout).text(), + new Response(process.stderr).text(), + ]); + expect(exitCode).toBe(0); + expect(stderr).toBe(""); - const result = JSON.parse(stdout) as StandaloneResult; - expect(result).toMatchObject({ - builtinSource: "builtin", - builtinResource: "references/evidence-map-example.md", - builtinTextFound: true, - fixtureBytes: sourceFixtureBytes.byteLength, - fixtureDigest: sourceFixtureDigest, - serviceDigest: sourceFixtureDigest, - unsupportedError: true, - unsupportedCode: "TOOL_SKILL_RESOURCE_BINARY_UNSUPPORTED", - }); - expect(result.unsupportedText).toBe([ - "---", - "skill: binary-fixture", - "source: builtin", - "resource: assets/favicon.ico", - `bytes: ${sourceFixtureBytes.byteLength}`, - "---", - "", - "error: TOOL_SKILL_RESOURCE_BINARY_UNSUPPORTED", - "hint: Binary Skill resources are valid package assets but cannot be returned by the text-only skill_read tool.", - ].join("\n")); - } finally { - await rm(tempRoot, { recursive: true, force: true }); - } + const result = JSON.parse(stdout) as StandaloneResult; + expect(result).toMatchObject({ + builtinSource: "builtin", + builtinResource: "references/evidence-map-example.md", + builtinTextFound: true, + fixtureBytes: sourceFixtureBytes.byteLength, + fixtureDigest: sourceFixtureDigest, + serviceDigest: sourceFixtureDigest, + unsupportedError: true, + unsupportedCode: "TOOL_SKILL_RESOURCE_BINARY_UNSUPPORTED", + }); + expect(result.unsupportedText).toBe([ + "---", + "skill: binary-fixture", + "source: builtin", + "resource: assets/fixture.bin", + `bytes: ${sourceFixtureBytes.byteLength}`, + "---", + "", + "error: TOOL_SKILL_RESOURCE_BINARY_UNSUPPORTED", + "hint: Binary Skill resources are valid package assets but cannot be returned by the text-only skill_read tool.", + ].join("\n")); }); interface StandaloneResult { @@ -114,7 +115,10 @@ function standaloneSource(paths: { `import { BUILTIN_SKILL_PACKAGES, SkillService } from ${JSON.stringify(paths.skillsEntrypoint)};`, `import { formatResolvedSkillResource } from ${JSON.stringify(paths.skillReadModule)};`, "", - "const fixtureBytes = await Bun.file(embeddedFixturePath).bytes();", + "const fixtureSeedBytes = await Bun.file(embeddedFixturePath).bytes();", + "const fixtureBytes = new Uint8Array(fixtureSeedBytes.byteLength + 1);", + "fixtureBytes.set(fixtureSeedBytes);", + "fixtureBytes[fixtureSeedBytes.byteLength] = 0xff;", "const fixtureDigest = new Bun.CryptoHasher(\"sha256\").update(fixtureBytes).digest(\"hex\");", "const builtinSkills = {", " ...BUILTIN_SKILL_PACKAGES,", @@ -127,7 +131,7 @@ function standaloneSource(paths: { " \"\",", " \"Read the binary fixture.\",", " ].join(\"\\n\"),", - " resources: { \"assets/favicon.ico\": fixtureBytes },", + " resources: { \"assets/fixture.bin\": fixtureBytes },", " },", "};", "const service = new SkillService({ userSkillsRoot: \"/definitely-missing-user-skills\", builtinSkills });", @@ -141,7 +145,7 @@ function standaloneSource(paths: { "const binaryResource = await service.readResourceForAgent(", " \"/definitely-missing-project\",", " \"binary-fixture\",", - " \"assets/favicon.ico\",", + " \"assets/fixture.bin\",", " [\"binary-fixture\"],", ");", "if (binaryResource === null) throw new Error(\"binary builtin resource was not resolved\");", @@ -166,3 +170,10 @@ function standaloneSource(paths: { function sha256(bytes: Uint8Array): string { return new Bun.CryptoHasher("sha256").update(bytes).digest("hex"); } + +function appendInvalidUtf8Byte(bytes: Uint8Array): Uint8Array { + const result = new Uint8Array(bytes.byteLength + 1); + result.set(bytes); + result[bytes.byteLength] = 0xff; + return result; +} diff --git a/packages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.md b/packages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.md index 28da6de2..a3bb8a02 100644 --- a/packages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.md +++ b/packages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.md @@ -4,7 +4,7 @@ Use these as normalization examples, not values to copy without confirmation. | User wording | Missing decision | Valid normalized trigger | | --- | --- | --- | -| “Tomorrow morning” | Exact date, local clock time, and UTC offset | `{ "kind": "once", "at": "2026-08-09T09:00:00+08:00" }` | +| “Tomorrow morning” | Exact date, local clock time, and UTC offset | Recalculate the date from the current local date, then confirm `{ "kind": "once", "at": "T09:00:00+08:00" }` | | “Every five minutes” | Whether a fixed elapsed interval is intended | `{ "kind": "interval", "everyMs": 300000 }` | | “Weekdays at nine” | IANA timezone | `{ "kind": "cron", "expression": "0 9 * * 1-5", "timezone": "Asia/Shanghai" }` | diff --git a/packages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.md b/packages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.md index b3a7bcbe..02a6001c 100644 --- a/packages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.md +++ b/packages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.md @@ -8,9 +8,9 @@ Question: Where is an incoming request admitted, persisted, and exposed to clien Excluded: unrelated page rendering and provider implementation. ## Entry and ownership -- `routes/example.ts#handler` — transport validation and status mapping [source] -- `domain/service.ts#execute` — domain invariant and mutation owner [source, test] -- `store/repository.ts#save` — persistence boundary [source] +- `routes/example.ts#handler` — transport validation and status mapping [source: `routes/example.ts#handler`] +- `domain/service.ts#execute` — domain invariant and mutation owner [source: `domain/service.ts#execute`; test: `domain/service.test.ts` "rejects duplicate commands"] +- `store/repository.ts#save` — persistence boundary [source: `store/repository.ts#save`] ## Primary flow HTTP input @@ -24,16 +24,16 @@ At each arrow record the concrete type/value, sync or async ordering, error path and whether the boundary mutates state. ## Invariants and impact -- Invariant: only the domain service may create the durable record [architecture test]. -- Direct consumers: symbols and tests that call the changed owner [references]. -- Transitive impact: API/event types or persisted data read by other packages [evidence]. +- Invariant: only the domain service may create the durable record [test: `architecture/ownership.test.ts` "keeps record creation in the domain service"]. +- Direct consumers: `routes/example.ts#handler` and `domain/service.test.ts` [references: `rg -n "service\\.execute" routes domain`]. +- Transitive impact: `contracts/example-event.ts#ExampleCreated` is consumed by the client projection [evidence: `client/projector.ts#applyExampleCreated`]. ## Unknowns - Unknown: whether retry can publish a duplicate event. - Next probe: inspect idempotency key ownership and the retry integration test. ``` -Evidence tags should resolve to a file plus symbol or tight line, a test whose assertion proves the claim, a command observation, or an authoritative external contract. “This folder seems responsible” and “tests pass” are not evidence locators. +Evidence tags should resolve to a file plus symbol or tight line, a named test whose assertion proves the claim, a reproducible command observation, or an authoritative external contract. The names above are illustrative placeholders for a hypothetical repository; replace every one with a locator from the repository being mapped. “This folder seems responsible” and “tests pass” are not evidence locators. Prefer one primary happy path plus the material alternate paths: validation failure, authorization denial, partial persistence, retry/restart, and cancellation only when they affect the question. End the map when a reader can identify the owner to change, its callers, the invariants to preserve, and the next unresolved probe. diff --git a/packages/agent-core/src/skills/builtin/git-master/SKILL.md b/packages/agent-core/src/skills/builtin/git-master/SKILL.md index 75c51446..e80f7fd6 100644 --- a/packages/agent-core/src/skills/builtin/git-master/SKILL.md +++ b/packages/agent-core/src/skills/builtin/git-master/SKILL.md @@ -65,6 +65,7 @@ Never amend an existing commit unless the user requested an amend or the current ## Prepare a branch or PR - Confirm the real base branch; do not assume it is `main`. +- Never commit to or push directly to a protected base branch. Use a focused feature branch and a pull request for protected-branch work. - Review the complete committed base-to-HEAD diff. Separately inspect `git_status`, both staged and unstaged `git_diff` views, and the contents of relevant untracked files; none of that uncommitted work appears in the base-to-HEAD commit range. - Run the required checks on the exact tree being proposed. - Push, create a PR, merge, delete branches, or clean worktrees only when the user requested that external or destructive effect. @@ -72,6 +73,6 @@ Never amend an existing commit unless the user requested an amend or the current ## Stop conditions -Stop and report before acting when authorization is missing, the base or target ref is uncertain, unrelated changes overlap the operation, a conflict's intended resolution is unclear, verification fails, or the operation would make recovery materially harder than the user requested. +Stop and report before acting when authorization is missing, the base or target ref is uncertain, the current branch is a protected base branch, unrelated changes overlap the operation, a conflict's intended resolution is unclear, verification fails, or the operation would make recovery materially harder than the user requested. Finish with the resulting branch/ref state, commits created or moved, verification evidence, remaining local changes, and any action still awaiting authorization. diff --git a/packages/agent-core/src/skills/builtin/git-master/references/operation-safety.md b/packages/agent-core/src/skills/builtin/git-master/references/operation-safety.md index 18ef1b29..06e905f7 100644 --- a/packages/agent-core/src/skills/builtin/git-master/references/operation-safety.md +++ b/packages/agent-core/src/skills/builtin/git-master/references/operation-safety.md @@ -7,13 +7,13 @@ Inspect before mutation: status, staged and unstaged diff, relevant history, cur | Commit | intended files, staged/unstaged split, repository message rules | unrelated changes overlap or commit grouping is ambiguous | | History investigation | exact symbol/path/question and relevant date or branch range | rename/move makes the initial path incomplete; expand deliberately | | Rebase/cherry-pick | source commits, target/base, upstream state, dirty worktree | target history or conflict policy is not authorized | -| Branch/PR preparation | base-to-HEAD committed diff, worktree state, checks, upstream | publication, push, base, or destructive cleanup was not requested | +| Branch/PR preparation | base-to-HEAD committed diff, worktree state, checks, upstream | current branch is a protected base branch; publication, push, base, or destructive cleanup was not requested | For a commit, stage explicit intended paths, inspect the staged diff, and verify that no required new file is omitted. The staged patch—not the working-tree summary—is the proposed commit. Split only when changes have independent intent and remain buildable/reviewable; do not split coupled production and regression-test changes for appearance. For history rewriting, first identify a recovery reference and confirm whether commits may already be shared. Preserve conflicts for inspection, resolve them from the intended combined behavior, then inspect the rewritten patch set rather than trusting command success. -For branch or PR readiness, review the base-to-HEAD committed diff. Separately inspect `git_status`, staged and unstaged diffs, and relevant untracked files; none of those are included in base-to-HEAD history. +For branch or PR readiness, never commit to or push directly to a protected base branch; use a focused feature branch and pull request. Review the base-to-HEAD committed diff. Separately inspect `git_status`, staged and unstaged diffs, and relevant untracked files; none of those are included in base-to-HEAD history. ## Mutation report diff --git a/packages/agent-core/src/skills/builtin/manifest.test.ts b/packages/agent-core/src/skills/builtin/manifest.test.ts index 38e241e7..7223f54b 100644 --- a/packages/agent-core/src/skills/builtin/manifest.test.ts +++ b/packages/agent-core/src/skills/builtin/manifest.test.ts @@ -31,6 +31,12 @@ describe("builtin Skill package manifest", () => { expect(activated.resources.map((resource) => resource.path)).toEqual( Object.keys(skillPackage.resources).sort(), ); + + for (const [path, value] of Object.entries(skillPackage.resources)) { + const embedded = typeof value === "string" ? new TextEncoder().encode(value) : value; + const source = await Bun.file(join(builtinRoot, name, ...path.split("/"))).bytes(); + expect([...embedded]).toEqual([...source]); + } } }); diff --git a/packages/agent-core/src/skills/builtin/orchestrate-work/SKILL.md b/packages/agent-core/src/skills/builtin/orchestrate-work/SKILL.md index 2342a2e8..9c97ba5e 100644 --- a/packages/agent-core/src/skills/builtin/orchestrate-work/SKILL.md +++ b/packages/agent-core/src/skills/builtin/orchestrate-work/SKILL.md @@ -1,6 +1,6 @@ --- name: orchestrate-work -description: Route ordinary root Lead work between direct execution and bounded child collaboration while retaining technical ownership. +description: When ordinary root Lead work requires choosing between direct execution and bounded child collaboration, route it while retaining technical ownership. license: MIT metadata: archcode/source: "ArchCode delegation protocol" diff --git a/packages/agent-core/src/skills/package-reader.test.ts b/packages/agent-core/src/skills/package-reader.test.ts index b07c10b2..d2b426f3 100644 --- a/packages/agent-core/src/skills/package-reader.test.ts +++ b/packages/agent-core/src/skills/package-reader.test.ts @@ -1,6 +1,5 @@ import { afterAll, beforeEach, describe, expect, test } from "bun:test"; import { mkdir, rm, symlink } from "node:fs/promises"; -import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { activateBuiltinSkill, @@ -8,6 +7,7 @@ import { discoverFilesystemSkill as discoverFilesystemSkillAt, readBuiltinSkillResource, readFilesystemSkillResource as readFilesystemSkillResourceAt, + SkillPackageResourceNotFoundError, SKILL_PACKAGE_MAX_BYTES, SKILL_PACKAGE_MAX_ENTRIES, SKILL_RESOURCE_MAX_BYTES, @@ -17,7 +17,7 @@ import { } from "./package-reader"; import type { BuiltinSkillPackage } from "./types"; -const tmpRoot = join(tmpdir(), "archcode-skill-package-reader", crypto.randomUUID()); +const tmpRoot = join(import.meta.dir, "__test_tmp__", "package-reader", crypto.randomUUID()); const encoder = new TextEncoder(); function entry(name = "test-skill", body = "Entry body.\n"): string { @@ -121,6 +121,22 @@ describe("Skill package reader", () => { expect(read.content).not.toBe(bytes); }); + test("reports unlisted builtin and filesystem resources with the package not-found type", async () => { + const skillPackage = builtin({ "assets/present.bin": "x" }); + expect(() => readBuiltinSkillResource(skillPackage, "test-skill", "assets/absent.bin")) + .toThrow(SkillPackageResourceNotFoundError); + try { + readBuiltinSkillResource(skillPackage, "test-skill", "assets/absent.bin"); + } catch (error) { + expect(error).toMatchObject({ name: "SkillPackageResourceNotFoundError" }); + } + + const packageRoot = join(tmpRoot, "not-found", "test-skill"); + await writePackage(packageRoot, { "references/guide.md": "guide" }); + await expect(readFilesystemSkillResource(packageRoot, "test-skill", "references/absent.md")) + .rejects.toBeInstanceOf(SkillPackageResourceNotFoundError); + }); + test("builtin packages cannot place resources below the SKILL.md entry path", () => { expect(() => activateBuiltinSkill( builtin({ "SKILL.md/hidden.txt": "impossible filesystem shape" }), @@ -148,15 +164,26 @@ describe("Skill package reader", () => { expect(() => validateResourcePath("references/file.md")).not.toThrow(); }); - test("enforces resource depth below, equal, and above the fixed limit", () => { + test("enforces resource depth below, equal, and above the fixed limit", async () => { for (const depth of [SKILL_RESOURCE_MAX_DEPTH - 1, SKILL_RESOURCE_MAX_DEPTH]) { const path = pathAtDepth(depth); expect(activateBuiltinSkill(builtin({ [path]: "ok" }), "test-skill").resources[0]?.path).toBe(path); + + const packageRoot = join(tmpRoot, `filesystem-depth-${depth}`, "test-skill"); + await writePackage(packageRoot, { [path]: "ok" }); + expect((await activateFilesystemSkill(packageRoot, "test-skill")).resources[0]?.path).toBe(path); } expect(() => activateBuiltinSkill( builtin({ [pathAtDepth(SKILL_RESOURCE_MAX_DEPTH + 1)]: "too deep" }), "test-skill", )).toThrow(`depth exceeds ${SKILL_RESOURCE_MAX_DEPTH}`); + + const aboveRoot = join(tmpRoot, "filesystem-depth-above", "test-skill"); + await writePackage(aboveRoot, { + [pathAtDepth(SKILL_RESOURCE_MAX_DEPTH + 1)]: "too deep", + }); + await expect(activateFilesystemSkill(aboveRoot, "test-skill")) + .rejects.toThrow(`depth exceeds ${SKILL_RESOURCE_MAX_DEPTH}`); }); test("enforces one-resource bytes below, equal, and above the fixed limit", () => { @@ -304,6 +331,19 @@ describe("Skill package reader", () => { await expect(activateFilesystemSkill(resourceRoot, "test-skill")).rejects.toThrow("symlinks are not allowed"); }); + test("requires the filesystem entry name to be exactly SKILL.md", async () => { + const packageRoot = join(tmpRoot, "entry-case", "test-skill"); + await mkdir(packageRoot, { recursive: true }); + await Bun.write(join(packageRoot, "skill.md"), entry()); + + await expect(discoverFilesystemSkill(packageRoot, "test-skill")) + .rejects.toThrow("must be named exactly SKILL.md"); + await expect(activateFilesystemSkill(packageRoot, "test-skill")) + .rejects.toThrow("must be named exactly SKILL.md"); + expect(() => validateResourcePath("skill.md")).toThrow("cannot be a resource"); + expect(() => validateResourcePath("skill.md/hidden.txt")).toThrow("cannot be a resource"); + }); + test("rejects a symlink in package ancestry below the trusted source boundary", async () => { const boundaryRoot = join(tmpRoot, "ancestry", "project"); const externalSkillsRoot = join(tmpRoot, "ancestry", "external-skills"); @@ -337,8 +377,15 @@ describe("Skill package reader", () => { const packageRoot = join(tmpRoot, "input", "test-skill"); await writePackage(packageRoot, { "references/guide.md": "guide" }); - for (const resource of ["/tmp/outside", "../outside", "references\\guide.md", "references//guide.md"]) { - await expect(readFilesystemSkillResource(packageRoot, "test-skill", resource)).rejects.toThrow(); + const cases = [ + { resource: "/tmp/outside", message: "must be relative" }, + { resource: "../outside", message: "invalid segment" }, + { resource: "references\\guide.md", message: "POSIX separators" }, + { resource: "references//guide.md", message: "invalid segment" }, + ] as const; + for (const item of cases) { + await expect(readFilesystemSkillResource(packageRoot, "test-skill", item.resource)) + .rejects.toThrow(item.message); } }); }); diff --git a/packages/agent-core/src/skills/package-reader.ts b/packages/agent-core/src/skills/package-reader.ts index 7b0e7d15..c45cf585 100644 --- a/packages/agent-core/src/skills/package-reader.ts +++ b/packages/agent-core/src/skills/package-reader.ts @@ -60,6 +60,7 @@ export async function discoverFilesystemSkill( const { root } = location; await assertFilesystemSkillAncestry(location); await assertRegularDirectory(root, "Skill package root"); + await assertExactSkillEntryName(root); const entryPath = join(root, SKILL_ENTRY_FILE); await assertRegularFile(entryPath, "SKILL.md"); const headerBytes = await readPrefix(entryPath, DISCOVERY_READ_MAX_BYTES); @@ -76,6 +77,7 @@ export async function activateFilesystemSkill( const { root } = location; await assertFilesystemSkillAncestry(location); await assertRegularDirectory(root, "Skill package root"); + await assertExactSkillEntryName(root); const entryPath = join(root, SKILL_ENTRY_FILE); const entryBytes = await readRegularFileBounded(entryPath, SKILL_ENTRY_MAX_BYTES, "SKILL.md"); let entryText: string; @@ -101,6 +103,7 @@ export async function readFilesystemSkillResource( const activated = await activateFilesystemSkill(location, expectedName); const descriptor = activated.resources.find((candidate) => candidate.path === resource); if (descriptor === undefined) throw new SkillPackageResourceNotFoundError(resource); + const ancestry = await captureResourceDirectoryIdentity(location, resource); const content = await readRegularFileBounded( join(root, ...resource.split("/")), SKILL_RESOURCE_MAX_BYTES, @@ -109,6 +112,7 @@ export async function readFilesystemSkillResource( if (content.byteLength !== descriptor.bytes) { throw new Error(`Skill resource changed while reading: ${resource}`); } + await assertDirectoryIdentityUnchanged(ancestry); await assertFilesystemSkillAncestry(location); return { descriptor, content }; } @@ -117,8 +121,8 @@ export function discoverBuiltinSkill( skillPackage: BuiltinSkillPackage, expectedName: string, ): SkillMetadata { - const entryBytes = new TextEncoder().encode(skillPackage.entry.slice(0, DISCOVERY_READ_MAX_BYTES)); - const { metadata } = parseSkillHeaderBytes(entryBytes.subarray(0, DISCOVERY_READ_MAX_BYTES)); + const entryBytes = encodeUtf8Prefix(skillPackage.entry, DISCOVERY_READ_MAX_BYTES); + const { metadata } = parseSkillHeaderBytes(entryBytes); assertExpectedName(metadata, expectedName); return metadata; } @@ -164,7 +168,7 @@ export function validateResourcePath(resource: string): void { if (segments.length > SKILL_RESOURCE_MAX_DEPTH) { throw new Error(`Skill resource depth exceeds ${SKILL_RESOURCE_MAX_DEPTH}`); } - if (segments[0] === SKILL_ENTRY_FILE) { + if (segments[0]?.toLowerCase() === SKILL_ENTRY_FILE.toLowerCase()) { throw new Error("SKILL.md is the package entry and cannot be a resource directory"); } } @@ -178,6 +182,10 @@ async function walkFilesystemResources( let totalBytes = entryBytes; async function walk(directory: string, prefix: readonly string[]): Promise { + const directoryIdentity = await assertRegularDirectory( + directory, + prefix.length === 0 ? "Skill package root" : `Skill resource directory "${prefix.join("/")}"`, + ); const entries = await readdir(directory, { withFileTypes: true }); entries.sort((a, b) => lexicalCompare(a.name, b.name)); for (const entry of entries) { @@ -213,6 +221,11 @@ async function walkFilesystemResources( throw new Error(`Skill package exceeds ${SKILL_PACKAGE_MAX_BYTES} aggregate bytes`); } } + await assertSamePathIdentity( + directory, + directoryIdentity, + prefix.length === 0 ? "Skill package root" : `Skill resource directory "${prefix.join("/")}"`, + ); } await walk(root, []); @@ -283,11 +296,12 @@ async function readRegularFileBounded( maxBytes: number, label: string, ): Promise { - await assertRegularFile(path, label); + const pathInfo = await assertRegularFile(path, label); const handle = await open(path, constants.O_RDONLY | constants.O_NOFOLLOW); try { const info = await handle.stat(); if (!info.isFile()) throw new Error(`${label} must be a regular file`); + assertSameIdentity(pathInfo, info, label); if (info.size > maxBytes) throw new Error(`${label} exceeds ${maxBytes} bytes`); const buffer = new Uint8Array(info.size); let offset = 0; @@ -298,6 +312,7 @@ async function readRegularFileBounded( } const after = await handle.stat(); if (offset !== info.size || after.size !== info.size) throw new Error(`${label} changed while reading`); + assertSameIdentity(info, after, label); return buffer; } finally { await handle.close(); @@ -311,6 +326,70 @@ async function assertRegularDirectory(path: string, label: string) { return info; } +interface FileIdentity { + readonly path: string; + readonly label: string; + readonly dev: number; + readonly ino: number; +} + +async function assertExactSkillEntryName(root: string): Promise { + const names = await readdir(root); + const aliases = names.filter((name) => name.toLowerCase() === SKILL_ENTRY_FILE.toLowerCase()); + if (aliases.length !== 1 || aliases[0] !== SKILL_ENTRY_FILE) { + throw new Error(`Skill package entry must be named exactly ${SKILL_ENTRY_FILE}`); + } +} + +async function captureResourceDirectoryIdentity( + location: FilesystemSkillPackageLocation, + resource: string, +): Promise { + await assertFilesystemSkillAncestry(location); + const identities: FileIdentity[] = []; + let current = location.root; + const parentSegments = resource.split("/").slice(0, -1); + const roots = ["", ...parentSegments]; + for (const segment of roots) { + if (segment !== "") current = join(current, segment); + const label = segment === "" ? "Skill package root" : `Skill resource directory "${current}"`; + const info = await assertRegularDirectory(current, label); + identities.push({ path: current, label, dev: info.dev, ino: info.ino }); + } + return identities; +} + +async function assertDirectoryIdentityUnchanged( + identities: readonly FileIdentity[], +): Promise { + for (const identity of identities) { + const info = await assertRegularDirectory(identity.path, identity.label); + assertSameIdentity(identity, info, identity.label); + } +} + +async function assertSamePathIdentity( + path: string, + expected: { readonly dev: number; readonly ino: number }, + label: string, +): Promise { + const current = await lstat(path); + if (current.isSymbolicLink() || !current.isDirectory()) { + throw new Error(`${label} changed while reading`); + } + assertSameIdentity(expected, current, label); +} + +function assertSameIdentity( + expected: { readonly dev: number; readonly ino: number }, + actual: { readonly dev: number; readonly ino: number }, + label: string, +): void { + if (expected.dev !== actual.dev || expected.ino !== actual.ino) { + throw new Error(`${label} changed while reading`); + } +} + export async function assertFilesystemSkillAncestry( location: FilesystemSkillPackageLocation, ): Promise { @@ -341,6 +420,19 @@ async function assertRegularFile(path: string, label: string) { return info; } +function encodeUtf8Prefix(value: string, maxBytes: number): Uint8Array { + const output = new Uint8Array(maxBytes); + const encoder = new TextEncoder(); + let offset = 0; + for (const character of value) { + const bytes = encoder.encode(character); + if (offset + bytes.byteLength > maxBytes) break; + output.set(bytes, offset); + offset += bytes.byteLength; + } + return output.subarray(0, offset); +} + function assertExpectedName(metadata: SkillMetadata, expectedName: string): void { if (metadata.name !== expectedName) { throw new Error( diff --git a/packages/agent-core/src/skills/schema.test.ts b/packages/agent-core/src/skills/schema.test.ts index 41ea9b77..bc0b0c20 100644 --- a/packages/agent-core/src/skills/schema.test.ts +++ b/packages/agent-core/src/skills/schema.test.ts @@ -199,6 +199,27 @@ Follow the plan. expect(() => parseSkillHeaderBytes(invalid)).toThrow("valid UTF-8"); expect(() => parseSkillHeaderBytes(encoder.encode("---\nname: a\n"))).toThrow("closing delimiter"); }); + + test("rejects duplicate YAML keys and alias expansion beyond the fixed limit", () => { + expect(() => parseSkillFrontmatter([ + "name: a", + "description: Use this Skill when needed.", + "description: Duplicate key.", + ].join("\n"))).toThrow("Invalid Skill YAML frontmatter"); + + const aliasExpansion = [ + "name: a", + "description: Use this Skill when needed.", + "metadata:", + " a: &a [x, x]", + " b: &b [*a, *a]", + " c: &c [*b, *b]", + " d: &d [*c, *c]", + " e: *d", + ].join("\n"); + expect(() => parseSkillFrontmatter(aliasExpansion)) + .toThrow("Excessive alias count"); + }); }); function frontmatterWithExactBytes(target: number): string { diff --git a/packages/agent-core/src/skills/service.test.ts b/packages/agent-core/src/skills/service.test.ts index 253bc495..0b7bea93 100644 --- a/packages/agent-core/src/skills/service.test.ts +++ b/packages/agent-core/src/skills/service.test.ts @@ -1,6 +1,5 @@ import { afterAll, beforeEach, describe, expect, test } from "bun:test"; import { mkdir, rm, symlink } from "node:fs/promises"; -import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import type { BuiltinSkillPackage } from "./types"; import { @@ -11,7 +10,7 @@ import { SkillValidationError, } from "./service"; -const tmpRoot = join(tmpdir(), "archcode-skill-service", crypto.randomUUID()); +const tmpRoot = join(import.meta.dir, "__test_tmp__", "service", crypto.randomUUID()); function skillMarkdown( name: string, @@ -245,8 +244,11 @@ describe("SkillService", () => { expect(skill?.body).toContain(`BUILTIN_${name}`); expect(await service.readForAgent(projectRoot, name, ["codemap"])).toBeNull(); } - expect((await service.listForAgent(projectRoot, ["codemap"])).map((entry) => entry.name)) - .not.toEqual(expect.arrayContaining([...RESERVED_BUILTIN_SKILL_NAMES])); + const listed = (await service.listForAgent(projectRoot, ["codemap"])) + .map((entry) => entry.name); + for (const name of RESERVED_BUILTIN_SKILL_NAMES) { + expect(listed).not.toContain(name); + } }); test("lists custom packages regardless of builtin allow-list and only eligible builtins", async () => { diff --git a/packages/agent-core/src/tools/builtins/skill-read.test.ts b/packages/agent-core/src/tools/builtins/skill-read.test.ts index ded40bd1..bd425108 100644 --- a/packages/agent-core/src/tools/builtins/skill-read.test.ts +++ b/packages/agent-core/src/tools/builtins/skill-read.test.ts @@ -1,6 +1,5 @@ import { afterAll, beforeEach, describe, expect, test } from "bun:test"; import { mkdir, rm } from "node:fs/promises"; -import { tmpdir } from "node:os"; import { dirname, join } from "node:path"; import { SkillService } from "../../skills"; import { storeManager } from "../../store/store"; @@ -11,7 +10,7 @@ import { createToolExecutionContext, type ToolExecutionContext } from "../types" import { createBuiltinToolDescriptors } from "./index"; import { formatResolvedSkillResource, SkillReadInputSchema, skillReadTool } from "./skill-read"; -const tmpRoot = join(tmpdir(), "archcode-skill-read-tool", crypto.randomUUID()); +const tmpRoot = join(import.meta.dir, "__test_tmp__", "skill-read", crypto.randomUUID()); const projectRoot = join(tmpRoot, "project"); const projectSkillsRoot = join(projectRoot, ".archcode", "skills"); const executionCwd = join(tmpRoot, "project.worktrees", "session-skill"); @@ -198,6 +197,38 @@ ENTRY_BODY expect(result.details?.error?.code).toBe("TOOL_SKILL_RESOURCE_NOT_FOUND"); }); + test("unknown resource on an unresolved Skill reports the Skill-level error", async () => { + const result = await skillReadTool.execute( + { name: "missing-skill", resource: "references/missing.md" }, + makeContext(["missing-skill"]), + ); + + expect(result.isError).toBe(true); + expect(result.details?.error?.code).toBe("TOOL_SKILL_NOT_FOUND"); + expect(expectTextDraft(result)).toContain( + "Skill not found or not allowed for current agent: missing-skill", + ); + }); + + test("rejects traversal and absolute resource paths at the tool boundary", async () => { + await writeProjectSkill("codemap", `--- +name: codemap +description: Maps code architecture when investigating an unfamiliar repository. +--- + +ENTRY_BODY +`, { "references/guide.md": "guide" }); + + for (const resource of ["../../etc/passwd", "references/../../escape.md", "/etc/passwd"]) { + const result = await skillReadTool.execute( + { name: "codemap", resource }, + makeContext(["codemap"]), + ); + expect(result.isError).toBe(true); + expect(result.details?.error?.code).toBe("TOOL_SKILL_INVALID"); + } + }); + test("resolves project-local Skills from execution cwd, not canonical project root", async () => { await writeProjectSkill("codemap", `--- name: codemap diff --git a/packages/agent-core/src/tools/builtins/skill-read.ts b/packages/agent-core/src/tools/builtins/skill-read.ts index bd7f724c..cde7e120 100644 --- a/packages/agent-core/src/tools/builtins/skill-read.ts +++ b/packages/agent-core/src/tools/builtins/skill-read.ts @@ -13,11 +13,12 @@ import { } from "../../skills"; import { SKILL_NAME_REGEX } from "../../skills/schema"; -const SKILL_NAME_MESSAGE = "Skill name must match pattern ^[a-z0-9]+(?:-[a-z0-9]+)*$"; +const SKILL_NAME_PATTERN = "^(?!.*--)[a-z0-9]+(?:-[a-z0-9]+)*$"; +const SKILL_NAME_MESSAGE = `Skill name must match pattern ${SKILL_NAME_PATTERN} (no consecutive hyphens)`; export const SkillReadInputSchema = z .object({ - name: z.string().regex(SKILL_NAME_REGEX, SKILL_NAME_MESSAGE).describe("Exact allowed Skill name matching ^[a-z0-9]+(?:-[a-z0-9]+)*$; copy it from the System Prompt's available-skill list or skill_list instead of guessing."), + name: z.string().regex(SKILL_NAME_REGEX, SKILL_NAME_MESSAGE).describe(`Exact allowed Skill name matching ${SKILL_NAME_PATTERN}, with no consecutive hyphens; copy it from the System Prompt's available-skill list or skill_list instead of guessing.`), resource: z.string().min(1).optional().describe("Optional Skill-root-relative resource path copied exactly from the entry's Resources list, for example references/review-packet.md. It cannot select a source or read an arbitrary filesystem path."), }) .strict(); @@ -167,8 +168,8 @@ export function createSkillReadTool() { if (resource === null) { return createToolErrorResult({ kind: "file-not-found", - code: "TOOL_SKILL_RESOURCE_NOT_FOUND", - message: `Skill resource not found or not allowed for current agent: ${input.name}/${input.resource}`, + code: "TOOL_SKILL_NOT_FOUND", + message: `Skill not found or not allowed for current agent: ${input.name}`, }); } return formatResolvedSkillResource(resource);