feat(skills): adopt multi-file skill packages - #14
Merged
Conversation
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 <name>/SKILL.md packages with the standard five-field frontmatter; when_to_use, allowed_tools, and legacy parsing are removed.
📝 WalkthroughWalkthroughChangesThis change replaces single-file Skills with validated, multi-file Skill packages. It adds YAML metadata parsing, resource discovery and reads, source precedence, builtin package embedding, progressive discovery, and updated prompt, agent, command, and tool integrations. Skill package architecture
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
boh5
marked this pull request as ready for review
August 8, 2026 15:59
Owner
Author
|
@coderabbitai review |
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (9)
packages/agent-core/src/skills/package-reader.test.ts (2)
112-129: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test that asserts
SkillPackageResourceNotFoundErrorby name.No test covers the not-found path for either reader. The class identity is load-bearing:
service.ts#readResourceForAgentuseserror instanceof SkillPackageResourceNotFoundErrorto translate the failure intoSkillResourceNotFoundError. If the reader ever throws a plainErrorfor an unlisted resource, the service silently reclassifies it as a validation error and the test suite stays green.The repository guideline also requires asserting custom error names, not only messages.
🧪 Proposed test
+ test("reports an unlisted resource as SkillPackageResourceNotFoundError", async () => { + const skillPackage = builtin({ "assets/present.bin": "x" }); + expect(() => readBuiltinSkillResource(skillPackage, "test-skill", "assets/absent.bin")) + .toThrow(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); + });Add
SkillPackageResourceNotFoundErrorto the import list on lines 5-17.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/package-reader.test.ts` around lines 112 - 129, Add a not-found test covering both resource readers, using an unlisted resource path and asserting the thrown error is a SkillPackageResourceNotFoundError and has that exact name. Update the imports in package-reader.test.ts to include SkillPackageResourceNotFoundError, and preserve the existing successful read behavior.Source: Coding guidelines
151-160: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd the filesystem counterpart for the depth limit.
Every other limit is tested on both the builtin path and the filesystem path. Depth is builtin-only.
walkFilesystemResourceshas its own separate depth check with a different error message, and that branch is uncovered.🧪 Proposed test
+ test("enforces filesystem resource depth below, equal, and above the fixed limit", async () => { + for (const depth of [SKILL_RESOURCE_MAX_DEPTH - 1, SKILL_RESOURCE_MAX_DEPTH]) { + const packageRoot = join(tmpRoot, `filesystem-depth-${depth}`, "test-skill"); + await writePackage(packageRoot, { [pathAtDepth(depth)]: "ok" }); + expect((await activateFilesystemSkill(packageRoot, "test-skill")).resources[0]?.path) + .toBe(pathAtDepth(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}`); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/package-reader.test.ts` around lines 151 - 160, Add filesystem coverage to the depth-limit test near activateBuiltinSkill, exercising walkFilesystemResources with paths just below, at, and above SKILL_RESOURCE_MAX_DEPTH. Assert valid depths are accepted and the over-limit filesystem path throws its branch-specific depth error, while preserving the existing builtin assertions.packages/agent-core/src/skills/package-reader.ts (2)
120-121: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueEncode once, then bound once.
Line 120 slices the entry by UTF-16 code units and line 121 bounds the result again by bytes. The string slice can split a surrogate pair, and the byte
subarraycan cut mid-UTF-8-sequence. Neither crashes, but a valid builtin package whose frontmatter approaches 16 KiB with multibyte characters would fail with "Skill frontmatter must be valid UTF-8" instead of parsing. Encoding the full entry once and taking a single byte-bounded prefix removes the double truncation and matches the filesystem path's byte-prefix behavior.♻️ Proposed change
- 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 = new TextEncoder().encode(skillPackage.entry); + const { metadata } = parseSkillHeaderBytes(entryBytes.subarray(0, DISCOVERY_READ_MAX_BYTES));🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/package-reader.ts` around lines 120 - 121, Update the entry-byte preparation around parseSkillHeaderBytes to encode the full skillPackage.entry once, then pass only a single prefix bounded by DISCOVERY_READ_MAX_BYTES. Remove the character-based slice and avoid a second truncation that can split multibyte UTF-8 sequences, while preserving the existing metadata parsing flow.
100-108: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftEach resource read re-walks the whole package.
readFilesystemSkillResourcecallsactivateFilesystemSkill, which re-reads and re-parsesSKILL.md(up to 128 KiB) andlstats every entry in the tree (up to 256) only to find one descriptor. Progressive disclosure means agents read resources one at a time, so this cost is paid on everyskill_readresource call.A targeted path is enough: assert ancestry,
lstatthe single joined resource path, and derive the descriptor from thatstat. The safety properties are unchanged, becausevalidateResourcePathalready rejects traversal andreadRegularFileBoundedusesO_NOFOLLOW. The only loss is the implicit "the whole package is still valid" check, which the caller does not depend on for a single read.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/package-reader.ts` around lines 100 - 108, Update readFilesystemSkillResource to avoid calling activateFilesystemSkill for each resource read; after validateResourcePath, assert the resource remains within root, lstat only the joined resource path, and derive the descriptor from that stat before bounded reading. Preserve SkillPackageResourceNotFoundError for missing or invalid targeted resources and retain the existing traversal and no-follow safety checks.packages/agent-core/src/skills/schema.test.ts (1)
192-201: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd tests for the YAML hardening options.
parseSkillFrontmattersetsuniqueKeys: trueandmaxAliasCount: 10. No test covers either. Both are security-relevant: duplicate keys change which value wins, and unbounded aliases allow a billion-laughs expansion. Add two cases so a future options change cannot regress silently.🧪 Proposed tests
+ test("rejects duplicate frontmatter keys and alias expansion bombs", () => { + expect(() => parseSkillFrontmatter([ + "name: a", + "description: Use this Skill when needed.", + "description: Duplicate key.", + ].join("\n"))).toThrow("Invalid Skill YAML frontmatter"); + + const bomb = [ + "name: a", + "description: Use this Skill when needed.", + "metadata:", + " a: &a x", + ...Array.from({ length: 12 }, (_, index) => ` b${index}: *a`), + ].join("\n"); + expect(() => parseSkillFrontmatter(bomb)).toThrow("Invalid Skill YAML frontmatter"); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/schema.test.ts` around lines 192 - 201, Add coverage in the schema parser tests for the YAML hardening configured by parseSkillFrontmatter: verify duplicate mapping keys are rejected with uniqueKeys enabled, and verify inputs exceeding the maxAliasCount of 10 are rejected. Keep the tests focused on parseSkillFrontmatter/parseSkillHeaderBytes behavior and assert both cases fail rather than silently accepting unsafe YAML.packages/agent-core/src/skills/service.test.ts (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTemporary test directory does not use
__test_tmp__/.The coding guidelines require
__test_tmp__/for temporary test directories. This file usestmpdir(). Confirm whether the repository has standardized ontmpdir()for this suite; if so, no change is needed.As per coding guidelines: "Use
__test_tmp__/for temporary test directories and clean them inafterAll."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/service.test.ts` at line 14, Update the temporary directory setup in the service test to use the repository-standard __test_tmp__/ location instead of tmpdir(), unless this suite is explicitly standardized on tmpdir(). Ensure the generated test directory remains unique and is cleaned up in afterAll.Source: Coding guidelines
packages/agent-core/src/tools/builtins/skill-read.test.ts (1)
276-284: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd tool-level rejection tests for traversal and absolute resource paths.
SkillReadInputSchemaconstrainsresourcewith.min(1)only. Rejection of../segments and absolute paths depends entirely onvalidateResourcePathinside the package reader.resourceis model-controlled input, so assert the rejection at this boundary too.🔒️ Proposed cases
test("input schema accepts an optional listed resource and rejects authority overrides", () => {Add a separate test that executes the tool:
test("rejects traversal and absolute resource paths", async () => { await writeProjectSkill("codemap", `--- name: codemap description: Maps code architecture when investigating an unfamiliar repository. --- ENTRY_BODY `); 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); } });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/tools/builtins/skill-read.test.ts` around lines 276 - 284, Add a separate async test near the existing SkillReadInputSchema coverage that creates the project skill, executes skillReadTool with traversal and absolute resources (“../../etc/passwd”, “references/../../escape.md”, and “/etc/passwd”), and asserts each result has isError set to true. Reuse writeProjectSkill and makeContext with the existing codemap fixture.packages/agent-core/src/skills/service.ts (1)
324-326: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
lexicalCompareis now defined identically in three changed files (four includingpackage-reader.ts). The shared root cause is a missing exported comparator utility. All copies produce deterministic ordering that feeds the prompt hash and tool output, so they must not drift.
packages/agent-core/src/skills/service.ts#L324-L326: export this implementation from a shared utils module and import it here.packages/agent-core/src/prompt/compiler.ts#L201-L204: delete the local copy and import the shared comparator.packages/agent-core/src/tools/builtins/skill-read.ts#L193-L196: delete the local copy and import the shared comparator.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/service.ts` around lines 324 - 326, Create and export the shared lexicalCompare utility from packages/agent-core/src/skills/service.ts (lines 324-326), then import and use it there; remove the duplicate local implementations in packages/agent-core/src/prompt/compiler.ts (lines 201-204) and packages/agent-core/src/tools/builtins/skill-read.ts (lines 193-196), updating imports as needed so all callers use one comparator.packages/agent-core/src/skills/builtin/manifest.test.ts (1)
22-35: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd a content equality check for each embedded resource.
This test verifies resource keys only. It does not verify that each embedded value matches the file at that path. A mis-wired import in
manifest.ts— for examplecodemapmappingreferences/evidence-map-example.mdtogoalReviewMatrix— passes every current assertion. The manifest contains 14 hand-written import/key pairs, so this swap is plausible.💚 Proposed content assertion
const activated = activateBuiltinSkill(skillPackage, name); expect(activated.metadata.name).toBe(name); expect(activated.resources.map((resource) => resource.path)).toEqual( Object.keys(skillPackage.resources).sort(), ); + + for (const [path, value] of Object.entries(skillPackage.resources)) { + const onDisk = await Bun.file(join(builtinRoot, name, ...path.split("/"))).text(); + expect(typeof value === "string" ? value : new TextDecoder().decode(value)).toBe(onDisk); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/builtin/manifest.test.ts` around lines 22 - 35, Extend the test in “declares every non-entry source file once with no embedded extras” to compare each skillPackage.resources value with the contents of its corresponding source file under builtinRoot/name, while retaining the existing key and activation assertions. Ensure every embedded resource’s path-to-content mapping is validated, including nested resource paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/agents/multi-agent-design.md`:
- Around line 46-60: Restore the original content of
docs/agents/multi-agent-design.md, removing the changes to the Skill package and
disclosure descriptions. Do not modify or relocate this historical document;
place any required new architecture guidance in an allowed non-docs location
instead.
In `@docs/concepts.md`:
- Around line 112-123: Restore the original historical content and path of
docs/concepts.md, removing the replacement around the Skills description. Move
the current Skills contract to the designated active documentation location
permitted for updates, preserving its content there without modifying other
historical docs.
In `@packages/agent-core/src/skills/builtin-standalone.integration.test.ts`:
- Around line 8-10: 将 builtin standalone 集成测试中的 fixture 依赖从
apps/web/public/favicon.ico 移除；在 packages/agent-core 内新增一个非 UTF-8 二进制
fixture，并更新相关路径与读取逻辑，使测试资源从该本地 fixture 导入，同时保留 skillsEntrypoint 使用现有 index.ts。
- Around line 13-93: Update the standalone binary test to create its temporary
workspace under the repository’s __test_tmp__/ directory instead of using
mkdtemp in the system temp directory. Move cleanup out of the test’s finally
block into an afterAll hook that removes the test directory, preserving the
existing test behavior and cleanup lifecycle.
In
`@packages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.md`:
- Line 7: Update the “Tomorrow morning” example in the schedule documentation to
avoid the hard-coded 2026-08-09 date; use a date-neutral placeholder or
explicitly instruct users/agents to recalculate the date from the current date
while preserving the local time and UTC offset format.
In
`@packages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.md`:
- Around line 11-13: Update the evidence-map example entries and corresponding
guidance to use concrete locators for each evidence tag, including a file and
symbol, test, command, or contract; where concrete values are intentionally
omitted, label them explicitly as placeholders. Apply this consistently to the
examples around the source, test, references, and evidence markers.
In `@packages/agent-core/src/skills/builtin/git-master/SKILL.md`:
- Around line 65-71: Update the branch/PR guidance in
packages/agent-core/src/skills/builtin/git-master/SKILL.md lines 65-71 to forbid
committing to or pushing directly to protected main, requiring a focused feature
branch and pull request instead. Add the same protected-branch rule to the
branch/PR stop condition in
packages/agent-core/src/skills/builtin/git-master/references/operation-safety.md
lines 5-10, keeping both documents consistent.
In `@packages/agent-core/src/skills/builtin/orchestrate-work/SKILL.md`:
- Line 3: Update the description metadata for the orchestrate-work Skill to
explicitly state when it should be activated, while preserving its existing
method and Lead ownership details. Include a clear trigger for ordinary root
Lead work that is routed through direct execution or bounded child
collaboration.
In `@packages/agent-core/src/skills/package-reader.test.ts`:
- Around line 336-343: Update the resource-validation test around
readFilesystemSkillResource to assert the rejection message produced by
validateResourcePath for each invalid resource, rather than accepting any thrown
error. Keep the existing invalid inputs and verify each failure specifically
indicates path validation rejected the resource before filesystem access.
- Line 20: Update the temporary root in the package-reader tests to be created
under the repository’s __test_tmp__/ directory while retaining the unique
subdirectory name and existing afterAll cleanup; then remove the unused node:os
tmpdir import.
In `@packages/agent-core/src/skills/service.test.ts`:
- Around line 248-249: Update the reserved-name assertion in the
service.listForAgent test to verify each name in RESERVED_BUILTIN_SKILL_NAMES is
absent individually, rather than negating a single arrayContaining assertion.
Preserve the existing ineligible-agent authorization scenario and ensure the
test fails if any one reserved builtin appears in the listing.
In `@packages/agent-core/src/tools/builtins/skill-read.ts`:
- Around line 16-21: Update SKILL_NAME_MESSAGE and the name field’s .describe()
text in SkillReadInputSchema to advertise the enforced no-consecutive-hyphen
rule from SKILL_NAME_REGEX, including the (?!.*--) constraint. Keep the
validation regex unchanged and ensure all name-pattern descriptions consistently
reject names containing consecutive hyphens.
- Around line 160-175: Update the null-result handling in the resource branch of
the skill-read tool to report an unresolved or unavailable Skill rather than a
missing resource. Use the existing Skill-level error code/message conventions,
while leaving SkillResourceNotFoundError handling through skillReadError
unchanged.
---
Nitpick comments:
In `@packages/agent-core/src/skills/builtin/manifest.test.ts`:
- Around line 22-35: Extend the test in “declares every non-entry source file
once with no embedded extras” to compare each skillPackage.resources value with
the contents of its corresponding source file under builtinRoot/name, while
retaining the existing key and activation assertions. Ensure every embedded
resource’s path-to-content mapping is validated, including nested resource
paths.
In `@packages/agent-core/src/skills/package-reader.test.ts`:
- Around line 112-129: Add a not-found test covering both resource readers,
using an unlisted resource path and asserting the thrown error is a
SkillPackageResourceNotFoundError and has that exact name. Update the imports in
package-reader.test.ts to include SkillPackageResourceNotFoundError, and
preserve the existing successful read behavior.
- Around line 151-160: Add filesystem coverage to the depth-limit test near
activateBuiltinSkill, exercising walkFilesystemResources with paths just below,
at, and above SKILL_RESOURCE_MAX_DEPTH. Assert valid depths are accepted and the
over-limit filesystem path throws its branch-specific depth error, while
preserving the existing builtin assertions.
In `@packages/agent-core/src/skills/package-reader.ts`:
- Around line 120-121: Update the entry-byte preparation around
parseSkillHeaderBytes to encode the full skillPackage.entry once, then pass only
a single prefix bounded by DISCOVERY_READ_MAX_BYTES. Remove the character-based
slice and avoid a second truncation that can split multibyte UTF-8 sequences,
while preserving the existing metadata parsing flow.
- Around line 100-108: Update readFilesystemSkillResource to avoid calling
activateFilesystemSkill for each resource read; after validateResourcePath,
assert the resource remains within root, lstat only the joined resource path,
and derive the descriptor from that stat before bounded reading. Preserve
SkillPackageResourceNotFoundError for missing or invalid targeted resources and
retain the existing traversal and no-follow safety checks.
In `@packages/agent-core/src/skills/schema.test.ts`:
- Around line 192-201: Add coverage in the schema parser tests for the YAML
hardening configured by parseSkillFrontmatter: verify duplicate mapping keys are
rejected with uniqueKeys enabled, and verify inputs exceeding the maxAliasCount
of 10 are rejected. Keep the tests focused on
parseSkillFrontmatter/parseSkillHeaderBytes behavior and assert both cases fail
rather than silently accepting unsafe YAML.
In `@packages/agent-core/src/skills/service.test.ts`:
- Line 14: Update the temporary directory setup in the service test to use the
repository-standard __test_tmp__/ location instead of tmpdir(), unless this
suite is explicitly standardized on tmpdir(). Ensure the generated test
directory remains unique and is cleaned up in afterAll.
In `@packages/agent-core/src/skills/service.ts`:
- Around line 324-326: Create and export the shared lexicalCompare utility from
packages/agent-core/src/skills/service.ts (lines 324-326), then import and use
it there; remove the duplicate local implementations in
packages/agent-core/src/prompt/compiler.ts (lines 201-204) and
packages/agent-core/src/tools/builtins/skill-read.ts (lines 193-196), updating
imports as needed so all callers use one comparator.
In `@packages/agent-core/src/tools/builtins/skill-read.test.ts`:
- Around line 276-284: Add a separate async test near the existing
SkillReadInputSchema coverage that creates the project skill, executes
skillReadTool with traversal and absolute resources (“../../etc/passwd”,
“references/../../escape.md”, and “/etc/passwd”), and asserts each result has
isError set to true. Reuse writeProjectSkill and makeContext with the existing
codemap fixture.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0cd28cb6-ccbd-4abf-b1fe-4c6450648c97
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock,!bun.lock
📒 Files selected for processing (60)
AGENTS.mdCHANGELOG.mdREADME.mddocs/agents/multi-agent-design.mddocs/concepts.mddocs/goals/skill-package-hard-cut-plan-goal.mddocs/goals/skill-package-hard-cut-progress.mdpackages/agent-core/package.jsonpackages/agent-core/src/agents/configured-agent.test.tspackages/agent-core/src/agents/configured-agent.tspackages/agent-core/src/agents/definitions/definitions.test.tspackages/agent-core/src/agents/factory.test.tspackages/agent-core/src/agents/factory.tspackages/agent-core/src/agents/session-agent-manager.test.tspackages/agent-core/src/commands/skill.test.tspackages/agent-core/src/commands/skill.tspackages/agent-core/src/prompt/compiler.test.tspackages/agent-core/src/prompt/compiler.tspackages/agent-core/src/skills/builtin-standalone.integration.test.tspackages/agent-core/src/skills/builtin/analyze-work/SKILL.mdpackages/agent-core/src/skills/builtin/analyze-work/references/diagnosis-method.mdpackages/agent-core/src/skills/builtin/automation-create/SKILL.mdpackages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.mdpackages/agent-core/src/skills/builtin/codemap/SKILL.mdpackages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.mdpackages/agent-core/src/skills/builtin/execute-plan/SKILL.mdpackages/agent-core/src/skills/builtin/execute-plan/references/execution-checkpoints.mdpackages/agent-core/src/skills/builtin/git-master/SKILL.mdpackages/agent-core/src/skills/builtin/git-master/references/operation-safety.mdpackages/agent-core/src/skills/builtin/goal-review/SKILL.mdpackages/agent-core/src/skills/builtin/goal-review/references/evidence-matrix-example.mdpackages/agent-core/src/skills/builtin/manifest.test.tspackages/agent-core/src/skills/builtin/manifest.tspackages/agent-core/src/skills/builtin/orchestrate-work/SKILL.mdpackages/agent-core/src/skills/builtin/orchestrate-work/references/delegation-packet.mdpackages/agent-core/src/skills/builtin/plan-work/SKILL.mdpackages/agent-core/src/skills/builtin/plan-work/assets/plan-template.mdpackages/agent-core/src/skills/builtin/research-docs/SKILL.mdpackages/agent-core/src/skills/builtin/research-docs/references/source-evaluation.mdpackages/agent-core/src/skills/builtin/review-change/SKILL.mdpackages/agent-core/src/skills/builtin/review-change/references/review-lenses.mdpackages/agent-core/src/skills/builtin/review-work/SKILL.mdpackages/agent-core/src/skills/builtin/review-work/references/review-packet.mdpackages/agent-core/src/skills/builtin/run-goal/SKILL.mdpackages/agent-core/src/skills/builtin/safe-refactor/SKILL.mdpackages/agent-core/src/skills/builtin/safe-refactor/references/boundary-verification.mdpackages/agent-core/src/skills/builtin/shape-todo/SKILL.mdpackages/agent-core/src/skills/builtin/shape-todo/references/todo-shaping-template.mdpackages/agent-core/src/skills/package-reader.test.tspackages/agent-core/src/skills/package-reader.tspackages/agent-core/src/skills/schema.test.tspackages/agent-core/src/skills/schema.tspackages/agent-core/src/skills/service.test.tspackages/agent-core/src/skills/service.tspackages/agent-core/src/skills/types.tspackages/agent-core/src/tools/builtins/model-visible-contract.test.tspackages/agent-core/src/tools/builtins/skill-list.test.tspackages/agent-core/src/tools/builtins/skill-list.tspackages/agent-core/src/tools/builtins/skill-read.test.tspackages/agent-core/src/tools/builtins/skill-read.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 61 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
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.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/agent-core/src/skills/package-reader.ts (1)
106-115: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftPrevent the directory-replacement TOCTOU.
The identity checks do not bind later
readdir()oropen()calls to the checked directories. An attacker can replace the package root or an intermediate resource directory with a symlink during path resolution, then restore it before the final identity check.O_NOFOLLOWprotects only the final path component.
packages/agent-core/src/skills/package-reader.ts#L106-L115: Read resources relative to an already-bound parent directory, not a newly resolved joined path.packages/agent-core/src/skills/package-reader.ts#L61-L69: Bind discovery entry access to a trusted root directory or immutable snapshot.packages/agent-core/src/skills/package-reader.ts#L78-L92: Bind activation entry access and traversal to the same trusted root.packages/agent-core/src/skills/package-reader.ts#L185-L228: Traverse resources through bound directory handles or an immutable snapshot.packages/agent-core/src/skills/package-reader.ts#L299-L315: Do not rely on final-componentO_NOFOLLOWto secure unresolved parent directories.packages/agent-core/src/skills/package-reader.ts#L344-L391: Replace post-read identity detection with a mechanism that prevents pathname re-resolution races.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/agent-core/src/skills/package-reader.ts` around lines 106 - 115, Eliminate pathname re-resolution races throughout package-reader.ts by binding discovery, activation, traversal, and resource reads to a trusted root directory or immutable snapshot. Update the logic at packages/agent-core/src/skills/package-reader.ts lines 61-69, 78-92, 106-115, 185-228, 299-315, and 344-391: use bound directory handles for each operation and replace post-read identity checks with race-resistant access; do not rely on final-component O_NOFOLLOW for unresolved parent directories.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/agent-core/src/skills/package-reader.ts`:
- Around line 106-115: Eliminate pathname re-resolution races throughout
package-reader.ts by binding discovery, activation, traversal, and resource
reads to a trusted root directory or immutable snapshot. Update the logic at
packages/agent-core/src/skills/package-reader.ts lines 61-69, 78-92, 106-115,
185-228, 299-315, and 344-391: use bound directory handles for each operation
and replace post-read identity checks with race-resistant access; do not rely on
final-component O_NOFOLLOW for unresolved parent directories.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b32c6e9e-d962-4e4e-9cba-e3d2bcb4dc45
📒 Files selected for processing (13)
packages/agent-core/src/skills/builtin-standalone.integration.test.tspackages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.mdpackages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.mdpackages/agent-core/src/skills/builtin/git-master/SKILL.mdpackages/agent-core/src/skills/builtin/git-master/references/operation-safety.mdpackages/agent-core/src/skills/builtin/manifest.test.tspackages/agent-core/src/skills/builtin/orchestrate-work/SKILL.mdpackages/agent-core/src/skills/package-reader.test.tspackages/agent-core/src/skills/package-reader.tspackages/agent-core/src/skills/schema.test.tspackages/agent-core/src/skills/service.test.tspackages/agent-core/src/tools/builtins/skill-read.test.tspackages/agent-core/src/tools/builtins/skill-read.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/agent-core/src/skills/builtin/git-master/references/operation-safety.md
- packages/agent-core/src/skills/builtin/automation-create/references/schedule-examples.md
- packages/agent-core/src/skills/builtin/manifest.test.ts
- packages/agent-core/src/skills/builtin-standalone.integration.test.ts
- packages/agent-core/src/tools/builtins/skill-read.ts
- packages/agent-core/src/tools/builtins/skill-read.test.ts
- packages/agent-core/src/skills/builtin/codemap/references/evidence-map-example.md
- packages/agent-core/src/skills/builtin/orchestrate-work/SKILL.md
- packages/agent-core/src/skills/builtin/git-master/SKILL.md
- packages/agent-core/src/skills/package-reader.test.ts
Owner
Author
|
CodeRabbit outside-diff TOCTOU finding reviewed: static symlink/traversal escapes, case-aliased entries, persistent replacement, and final-component symlinks are rejected; 8f926ba adds per-directory identity snapshots and before/after checks. A same-UID process that swaps and restores an ancestor between syscalls is outside ArchCode's local trust boundary because project/user Skills are user-controlled inputs and that actor can directly mutate Skills, config, or the installation. Bun/Node has no portable dirfd-relative openat API, so we are not adding FFI or platform-specific filesystem code for this out-of-scope threat model. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
<name>/SKILL.md.Why
The previous Skill system only loaded one Markdown body and could not expose reusable supporting resources progressively. The built-in workflows were also too shallow for reliable planning, analysis, review, execution, and research.
Breaking change
Project and user Skills must now use
<name>/SKILL.mdwith the standard five-field frontmatter.when_to_use,allowed_tools, and the legacy parser/fallback are removed intentionally; there is no compatibility migration path.Validation
bun run typecheckbun run testbun run buildgpt-5.6-solxhigh review: no open findingsSummary by CodeRabbit