Skip to content

Replace positional adapter asset contract with tagged request/result unions and atomic skill-bundle helpers - #438

Merged
eXamadeus merged 2 commits into
mainfrom
julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers
Jul 24, 2026
Merged

Replace positional adapter asset contract with tagged request/result unions and atomic skill-bundle helpers#438
eXamadeus merged 2 commits into
mainfrom
julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers

Conversation

@eXamadeus

@eXamadeus eXamadeus commented Jul 21, 2026

Copy link
Copy Markdown
Member

Why

The previous adapter asset contract used positional parameters (scope, assetType, name, content, metadata) and communicated failures by throwing errors. This made it impossible to represent multi-file skill bundles, forced callers to catch and classify thrown errors, and left no structural distinction between skill, agent, and command requests.

Details

installAsset, readAsset, and deleteAsset now each accept a single request object tagged by assetType and return a discriminated result union. Expected failures (not-found, invalid-companion-path, unsupported-scope, not-implemented, io-failed) are structured values on the result — nothing is thrown. Skill requests carry a companion byte map (CompanionMap) and a caller-supplied owned companion path set; agent and command request types structurally cannot carry companions, so the distinction is enforced at the type level rather than by convention.

defineAdapter stubs for omitted methods now return { ok: false, failure: { code: 'not-implemented', method } } instead of throwing.

Three new SDK helpers cover the skill bundle lifecycle:

  • installSkillBundle / readSkillBundle / deleteSkillBundle — staged all-or-nothing bundle replacement with snapshot/rollback, ownership-set-based deletion, and empty-directory pruning. Ownership is caller-supplied per operation; adapters never enumerate the skill directory or infer ownership from disk.
  • installSingleFileAsset / readSingleFileAsset / deleteSingleFileAsset — result-shaped wrappers for single-file agent and command assets.
  • validateContainedRelativePath — purely textual containment validation applied to every supplied companion path before any filesystem access. One failing path rejects the whole request without touching the filesystem.

The engine's materialize.ts is updated to drive the new contract: readPrevious narrows on the structured not-found result instead of catching ENOENT, and install/delete calls branch on result.ok before recording journal entries. Skill install requests from the engine currently pass empty companion maps and owned-path sets; real companion bytes and lockfile-derived ownership are deferred to the per-file integrity work.

The claude-code, opencode, and codex adapters are fully migrated. Codex delete operations now prune emptied directories consistently with the other adapters, which they previously did not.

Verification

CI — adapter SDK unit tests cover companion-less and multi-file skills, escaping paths in bundles and ownership sets, malformed ownership rejection before filesystem access, idempotence, canonical reads limited to requested owned paths, unowned content preservation, and injected failures at every write/delete/commit boundary. All three first-party adapter test suites are updated and pass.


Note

High Risk
Breaking wire contract and filesystem materialization across SDK, engine, and all adapters; skill-bundle path validation and rollback are security- and data-integrity sensitive, though heavily tested.

Overview
Breaking adapter contract (0.00.1): installAsset, readAsset, and deleteAsset now take a single request tagged by assetType and return { ok: true, … } | { ok: false, failure }. Expected outcomes use failure codes (not-found, invalid-companion-path, unsupported-scope, not-implemented, io-failed) instead of throws; omitted methods in defineAdapter return not-implemented results.

The SDK adds atomic skill-bundle helpers (installSkillBundle / readSkillBundle / deleteSkillBundle) with snapshot rollback, caller-supplied owned companion paths, and pruning, plus result-shaped single-file helpers and validateContainedRelativePath for pre-IO path checks. Skill installs can carry companion byte maps; agent/command types cannot.

Engine and CLI call sites, mocks, and compatibility checks are updated for ADAPTER_API_VERSION 0.1; 0.0 adapters fail closed as unsupported. Materialize/journal paths branch on structured results (including rollback when inverse ops fail).

First-party claude-code, opencode, and codex adapters switch to the new API (skills as bundles; unsupported system scope returns unsupported-scope). Codex agent deletes now prune empty dirs like the others. Docs and changeset describe migration and release ordering (SDK/adapters before a 0.1-only CLI).

Reviewed by Cursor Bugbot for commit 3841183. Bugbot is set up for automated code reviews on this repo. Configure here.

Summary by CodeRabbit

  • New Features
    • Introduced adapter API version 0.1 with request-based asset operations and structured success/failure results.
    • Added support for atomic skill bundles with companion files, preserving file contents and enabling safe updates.
    • Added standardized helpers for single-file assets and skill bundle installation, reading, and deletion.
  • Bug Fixes
    • Improved path-safety checks, rollback behavior, and directory cleanup.
    • Unsupported scopes and missing assets now report clear failures instead of unexpected errors.
  • Documentation
    • Updated adapter guides, CLI references, compatibility details, and troubleshooting for API 0.1.

@changeset-bot

changeset-bot Bot commented Jul 21, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 3841183

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 4 packages
Name Type
@agent-facets/adapter Minor
@agent-facets/adapter-claude-code Minor
@agent-facets/adapter-opencode Minor
@agent-facets/adapter-codex Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai

coderabbitai Bot commented Jul 21, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The adapter API advances from 0.0 to 0.1, replacing positional asset methods with tagged request objects and structured success/failure results. The SDK adds single-file and atomic skill-bundle helpers with containment validation. Claude Code, Codex, and OpenCode migrate to the new contract. The engine consumes structured results, rejects superseded adapters during preflight, and updates rollback handling. CLI fixtures and documentation now describe the 0.1 contract.

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant materialize
  participant Adapter
  participant SDK
  participant Filesystem

  CLI->>materialize: install asset
  materialize->>Adapter: readAsset(read request)
  Adapter->>SDK: read skill bundle or single file
  SDK->>Filesystem: read asset files
  Filesystem-->>SDK: asset bytes or structured failure
  SDK-->>Adapter: tagged read result
  Adapter-->>materialize: asset or failure
  materialize->>Adapter: installAsset(install request)
  Adapter->>SDK: install bundle or single file
  SDK->>Filesystem: write and reconcile asset files
  Filesystem-->>SDK: write result
  SDK-->>Adapter: tagged install result
  Adapter-->>materialize: primary path or failure
Loading
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 61.70% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Title check ✅ Passed The title clearly summarizes the main breaking adapter contract change and the new skill-bundle helpers.
Description check ✅ Passed The description includes Why, Details, and Verification, and provides enough migration and testing context.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

eXamadeus commented Jul 21, 2026

Copy link
Copy Markdown
Member Author

This stack of pull requests is managed by Graphite. Learn more about stacking.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: c6bf5b4501

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread packages/adapter/src/skill-bundle.ts Outdated
// Defense-in-depth: the textual validation above already guarantees
// containment; this re-checks the resolved form so a validator bug
// can't silently become a traversal.
if (abs !== root && !abs.startsWith(`${root}/`)) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Use platform-safe containment for companion paths

On Windows, node:path produces backslash-separated root and abs values, so a valid companion such as references/api.md resolves to C:\...\review\references\api.md but can never start with ${root}/. As a result, every non-empty companion set is rejected as invalid-companion-path by install, read, and delete operations on Windows. Use relative/isAbsolute (as the pruning helper does) or the platform separator for this defense-in-depth containment check.

Useful? React with 👍 / 👎.

Comment on lines +159 to +160
const newSet = new Set(newPaths)
const staleOwned = options.ownedCompanionPaths.filter((p) => !newSet.has(p))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject SKILL.md from companion ownership

A direct SDK caller can supply SKILL.md as either a companion key or an owned companion path because the containment validation accepts it. In the former case the companion-write loop overwrites the assembled primary with opaque bytes; in the latter it is treated as stale and deleted after being written. This violates the bundle contract and can leave a successful install with no valid primary, so reserve the primary filename when validating both supplied companion sets.

Useful? React with 👍 / 👎.

Comment thread packages/engine/src/install/materialize.ts Outdated
Comment thread packages/adapter/src/skill-bundle.ts
@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown

Greptile Summary

This PR replaces the positional adapter asset contract (scope, assetType, name, content, metadata args + thrown errors) with tagged request objects and discriminated result unions, and adds atomic skill-bundle helpers (installSkillBundle / readSkillBundle / deleteSkillBundle) with snapshot-rollback, caller-supplied ownership sets, and empty-directory pruning. The adapter API version is bumped to 0.1; all three first-party adapters (claude-code, opencode, codex) are fully migrated.

  • Breaking API change: installAsset, readAsset, deleteAsset on every adapter now accept a single tagged request object and must return a { ok } result — the 0.0 positional calling convention is rejected before any I/O runs. defineAdapter omitted-method stubs now return not-implemented results instead of throwing.
  • Atomic skill-bundle helpers in packages/adapter/src/skill-bundle.ts: each operation snapshots all files it will touch, performs mutations, and on any handled failure restores the prior bundle from the snapshot — so no partial bundle is left behind. Ownership is caller-supplied per request; adapters never enumerate the skill directory.
  • Engine integration: materialize.ts now branches on structured read/install/delete results; runUndoInstall/runUndoDelete throw on { ok: false } so the journal's rollback counter correctly reflects failed undo operations. Skill requests from the engine currently pass empty companion maps and owned-path sets (deferred to lockfile 0.2); the skip-if-identical check is intentionally limited to primary content and metadata until companion ownership is plumbed.

Confidence Score: 2/5

The skill-bundle filesystem code has two confirmed, exploitable symlink escape paths that can redirect companion writes outside the skill root without any race requirement for the root-level variant — these must be closed before the companion write path is exposed to untrusted input.

The new installSkillBundle helper in skill-bundle.ts contains a confirmed symlink bypass: for root-level companion keys, rejectSymlinkedParents never runs (the while-loop is immediately false) and the final path component itself is never lstat-checked. A pre-placed symlink at the skill root redirects writeFile to an arbitrary external path. A second confirmed vulnerability is the TOCTOU race between validation and mutation — a parent directory replaced by a symlink after the lstat check redirects the companion write. Both were reproduced end-to-end by the security-review subagent.

packages/adapter/src/skill-bundle.ts — specifically the rejectSymlinkedParents function (missing abs-level lstat) and the split between validation and mutation phases (TOCTOU window).

Security Review

Two symlink-escape vulnerabilities were confirmed REPRODUCED by the security-review subagent in the new skill-bundle.ts code.

Root-level companion bypass (S1): rejectSymlinkedParents(abs, root) starts its walk at dirname(abs). For a companion whose key is a simple filename (e.g. api.md), dirname(abs) === root, so the while-loop guard is immediately false and no symlink checks run. Additionally, abs itself is never lstat-checked — only its parent chain is walked. A pre-placed symlink at <skill-root>/api.md → /external/target therefore passes all validation, and the subsequent writeFile(abs, bytes) writes companion bytes to the external location. Confirmed with a concrete PoC by the security-review subagent.

TOCTOU race on parent replacement (S2): resolveCompanionPaths lstat-checks parent directories during validation, but mkdir/writeFile execute in a separate block after validation completes — the snapshot phase further extends the window. An actor with write access to the skill directory can atomically replace a validated real directory with a symlink between these two phases. The mutation-phase mkdir(dirname(abs), { recursive: true }) follows the link and the companion write lands outside the root. Confirmed with a concrete PoC by the security-review subagent.

Suggested fixes: (1) lstat(abs) after the parent walk and reject if symbolic link; (2) either re-lstat immediately before each write (narrows but does not eliminate the race) or use atomic write-then-rename within a root-anchored file descriptor.

Reviews (10): Last reviewed commit: "Update custom-adapters.mdx" | Re-trigger Greptile

Comment thread packages/adapter/src/asset-fs.ts
Comment thread packages/adapter/src/skill-bundle.ts Outdated
@eXamadeus
eXamadeus force-pushed the julian/07-20-add_strict_raw_tar-header_validation_0.2_archive_verification_and_structured_archiveverificationfailure_to_the_consumer_bridge branch from 7e815ef to 2b854bf Compare July 22, 2026 21:29
@eXamadeus
eXamadeus force-pushed the julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers branch from c6bf5b4 to 332e90d Compare July 22, 2026 21:29
Comment thread packages/adapter/src/skill-bundle.ts

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 332e90d12d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

undo: async () => {
if (previous) {
await adapter.installAsset(asset.scope, asset.type, asset.name, previous.content, previous.metadata ?? {})
await adapter.installAsset(installRequestFor(asset, previous.content, previous.metadata ?? {}))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Propagate failed rollback results

When a later materialization operation fails and the journal rolls this write back, first-party adapters now report an undo failure as { ok: false } rather than throwing. This callback ignores that result, so InstallJournal.rollback() counts the undo as successful and the install can report a clean rollback even though the original asset was not restored (the delete undo at line 304 has the same issue). Check each result and throw or otherwise surface its failure to the journal.

AGENTS.md reference: AGENTS.md:L172-L183

Useful? React with 👍 / 👎.

if (!check.ok) {
return { ok: false, failure: { code: 'invalid-companion-path', path: relPath, reason: check.reason } }
}
const abs = resolve(join(root, relPath))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject symlinked companion parents

If a user-controlled directory inside an existing skill root is a symlink (for example, references -> /tmp/outside), this lexical resolve() check still accepts references/api.md; the subsequent mkdir/writeFile follows the symlink and writes the companion outside the skill root. Validate canonical existing parents with lstat/realpath (and reject symlink components) before mutating bundle paths so the promised containment holds against filesystem links.

Useful? React with 👍 / 👎.

@eXamadeus
eXamadeus force-pushed the julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers branch from 332e90d to b24cc08 Compare July 22, 2026 22:01
@eXamadeus
eXamadeus force-pushed the julian/07-20-add_strict_raw_tar-header_validation_0.2_archive_verification_and_structured_archiveverificationfailure_to_the_consumer_bridge branch from 2b854bf to 3526a29 Compare July 22, 2026 22:01

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b24cc08b5e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

const snapshot: Snapshot = new Map()
try {
// Snapshot everything we will touch before mutating anything.
await snapshotFile(snapshot, paths.primaryFile)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate primary paths before mutating skill bundles

SkillBundlePaths is a public SDK input, but only companion paths are validated here. A direct adapter-SDK caller can provide a legitimate root with primaryFile outside it (for example, /tmp/victim); install will snapshot and overwrite that file, while delete will remove it. Validate that the resolved primary is a strict descendant of root before any snapshot, read, or mutation so the documented containment guarantee holds.

AGENTS.md reference: packages/adapter/AGENTS.md:L10-L20

Useful? React with 👍 / 👎.

@eXamadeus
eXamadeus force-pushed the julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers branch from b24cc08 to 947602e Compare July 23, 2026 00:04
@eXamadeus
eXamadeus force-pushed the julian/07-20-add_strict_raw_tar-header_validation_0.2_archive_verification_and_structured_archiveverificationfailure_to_the_consumer_bridge branch from 3526a29 to 93a9c9b Compare July 23, 2026 00:04
@eXamadeus
eXamadeus force-pushed the julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers branch from 947602e to 842359d Compare July 23, 2026 00:04
@eXamadeus
eXamadeus force-pushed the julian/07-20-add_strict_raw_tar-header_validation_0.2_archive_verification_and_structured_archiveverificationfailure_to_the_consumer_bridge branch from 93a9c9b to 009140b Compare July 23, 2026 00:05

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 842359d53c

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

const companions: Record<string, Uint8Array> = {}
for (const [relPath, abs] of validated.resolved) {
try {
companions[relPath] = new Uint8Array(await readFile(abs))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use a prototype-free map for returned companions

When an owned companion is legitimately named __proto__, this assignment invokes the inherited prototype setter rather than creating a map entry. The file is read successfully but is omitted from asset.companions (and the returned object's prototype is mutated), so a subsequent integrity/reconciliation pass treats that companion as missing. Use Object.create(null) or a Map-backed conversion for companion results.

Useful? React with 👍 / 👎.

Comment on lines +183 to +188
for (const [relPath, bytes] of Object.entries(options.companions)) {
// biome-ignore lint/style/noNonNullAssertion: resolved contains every validated path
const abs = resolved.get(relPath)!
currentPath = abs
await mkdir(dirname(abs), { recursive: true })
await writeFile(abs, bytes)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject companion paths that collide by filesystem case

On case-insensitive filesystems (including standard Windows and many macOS volumes), a bundle containing both References/api.md and references/api.md passes validation but both writes target the same file. The second write silently overwrites the first and the install reports success; later reads return the second bytes under both keys. Detect filesystem-equivalent duplicate companion paths before snapshotting/writing and return a structured failure.

Useful? React with 👍 / 👎.

@eXamadeus
eXamadeus force-pushed the julian/07-20-add_strict_raw_tar-header_validation_0.2_archive_verification_and_structured_archiveverificationfailure_to_the_consumer_bridge branch from 009140b to e2b15b8 Compare July 23, 2026 00:48
@eXamadeus
eXamadeus force-pushed the julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers branch from 842359d to 99729d4 Compare July 23, 2026 00:48
@eXamadeus
eXamadeus force-pushed the julian/07-20-add_strict_raw_tar-header_validation_0.2_archive_verification_and_structured_archiveverificationfailure_to_the_consumer_bridge branch from e2b15b8 to 54323fe Compare July 23, 2026 01:13
@eXamadeus
eXamadeus force-pushed the julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers branch from 99729d4 to 98ab0d2 Compare July 23, 2026 01:13

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 98ab0d260f

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

// stale-owned sweep deletes a primary that was just written. A companion
// named SKILL.md in a sub-directory (references/SKILL.md) is fine — it
// resolves to a different path than the primary.
if (abs === primaryAbs) {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject case-equivalent primary companion paths

On case-insensitive filesystems, a companion named skill.md (or another case-only variant of SKILL.md) resolves to the primary file even though these absolute-path strings differ. It passes this exact comparison and the companion write then overwrites the assembled primary with opaque bytes; if supplied only as an owned path it can likewise be removed as stale. Compare the companion against the primary using the same portable case-fold/NFC key used for companion collisions.

Useful? React with 👍 / 👎.

Comment on lines +320 to +321
await mkdir(dirname(abs), { recursive: true })
await writeFile(abs, bytes)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject symlinked companion targets before writing

When an existing companion file itself is a symlink (for example, references/api.md -> /tmp/victim), rejectSymlinkedParents permits it because only its parent directories are checked. writeFile follows that final symlink, so an otherwise contained bundle request writes outside the skill root despite the helper's containment guarantee. Lstat and reject the target itself (and similarly protect the primary) before snapshotting or mutating.

Useful? React with 👍 / 👎.

Comment thread packages/adapter/src/skill-bundle.ts
@eXamadeus
eXamadeus force-pushed the julian/07-20-add_strict_raw_tar-header_validation_0.2_archive_verification_and_structured_archiveverificationfailure_to_the_consumer_bridge branch from 54323fe to e4de25e Compare July 23, 2026 02:17
@eXamadeus
eXamadeus force-pushed the julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers branch 2 times, most recently from daba02d to f9cf6ea Compare July 23, 2026 02:19
@eXamadeus
eXamadeus force-pushed the julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers branch from f9cf6ea to 3feeead Compare July 23, 2026 02:29
@graphite-app
graphite-app Bot changed the base branch from graphite-base/438 to main July 23, 2026 02:30
@eXamadeus
eXamadeus force-pushed the julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers branch from 3feeead to b499951 Compare July 23, 2026 02:30
@mintlify

mintlify Bot commented Jul 23, 2026

Copy link
Copy Markdown
Contributor

Preview deployment for your docs. Learn more about Mintlify Previews.

Project Status Preview Updated (UTC)
AgentFacets 🟢 Ready View Preview Jul 23, 2026, 2:30 AM

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b499951e0e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +198 to +200
while (current !== root && isStrictlyBelow(current, root)) {
chain.push(current)
current = dirname(current)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Reject a symlinked skill root before materializing

When an existing skill root itself is a symlink (for example, skills/review -> /tmp/outside), this walk deliberately stops before checking root; the later mkdir/writeFile calls therefore follow that link and install the primary and companions outside the adapter-controlled tree. This is distinct from the intermediate-parent check: validate the root directory itself with lstat (while still allowing symlinked ancestors such as /var) before any snapshot or mutation.

Useful? React with 👍 / 👎.

failure: { code: 'io-failed', operation: 'delete', path: path.file, message: errorMessage(err) },
}
}
return { ok: true, existed, deletedPaths: existed ? [path.file] : [] }

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sidecar cleanup skews delete result

Medium Severity

The new result-shaped delete helpers set existed from the primary (and owned companions) only, then still remove legacy ${primary}.meta.json sidecars via deleteAssetFile or an extra rm. When the primary is absent but a sidecar remains, delete succeeds with existed: false and empty deletedPaths even though a file was removed, breaking the tagged contract’s idempotent bookkeeping.

Additional Locations (1)
Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b499951. Configure here.

return { ok: false, failure: { code: 'invalid-companion-path', path: relPath, reason: parentIssue } }
}

resolvedMap.set(relPath, abs)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Companion symlink files bypass containment

Medium Severity

resolveCompanionPaths rejects symlinked parent directories but never inspects the resolved companion path itself. If that leaf already exists as a symbolic link, subsequent writeFile / readFile calls follow the link, so bundle install or read can touch bytes outside the skill root despite the stated pre-mutation containment checks.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit b499951. Configure here.

@eXamadeus
eXamadeus force-pushed the julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers branch from b499951 to 8840e4d Compare July 23, 2026 04:03

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

There are 3 total unresolved issues (including 2 from previous reviews).

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 8840e4d. Configure here.

// the sidecar path.
await snapshotFile(snapshot, sidecarPath)
currentPath = sidecarPath
await rm(sidecarPath, { force: true })

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sidecar cleanup removes unowned files

Medium Severity

deleteSkillBundle always removes ${primaryFile}.meta.json after the owned-target loop, even when that path was never listed in ownedCompanionPaths. A valid companion at the relative path SKILL.md.meta.json resolves to the same absolute path as the legacy sidecar, so a delete with an empty owned set can remove that file while still claiming to preserve unowned skill files.

Fix in Cursor Fix in Web

Reviewed by Cursor Bugbot for commit 8840e4d. Configure here.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
docs/guides/custom-adapters.mdx (1)

267-267: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Stale assetPath reference after example rewrite.

The scaffolded example was rewritten to use baseDir + skillPaths(name) (no assetPath helper exists anymore), but this sentence still tells readers to check files against "your assetPath". This now points to a symbol that doesn't exist in the example.

📝 Proposed fix
-Check that files landed where your `assetPath` puts them. From here, harden `buildAssetMetadata` with a real schema and flesh out the on-disk layout your tool expects.
+Check that files landed where your `baseDir`/`skillPaths()` layout puts them. From here, harden `buildAssetMetadata` with a real schema and flesh out the on-disk layout your tool expects.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 36b52b7f-0fa6-49c5-8802-1de6b68b9eaa

📥 Commits

Reviewing files that changed from the base of the PR and between 72e9088 and 8840e4d.

📒 Files selected for processing (38)
  • .changeset/tagged-adapter-asset-contract.md
  • docs/cli/adapters/install.mdx
  • docs/cli/adapters/list.mdx
  • docs/guides/custom-adapters.mdx
  • docs/guides/troubleshooting.mdx
  • packages/adapter/src/__tests__/index.test.ts
  • packages/adapter/src/__tests__/skill-bundle.test.ts
  • packages/adapter/src/api-version.ts
  • packages/adapter/src/asset-fs.ts
  • packages/adapter/src/define-adapter.ts
  • packages/adapter/src/index.ts
  • packages/adapter/src/skill-bundle.ts
  • packages/adapter/src/types.ts
  • packages/adapters/claude-code/src/__tests__/adapter.test.ts
  • packages/adapters/claude-code/src/index.ts
  • packages/adapters/codex/src/__tests__/adapter.test.ts
  • packages/adapters/codex/src/index.ts
  • packages/adapters/opencode/src/__tests__/adapter.test.ts
  • packages/adapters/opencode/src/index.ts
  • packages/cli/src/__tests__/adapter-install-cli.e2e.test.ts
  • packages/cli/src/commands/add/__tests__/add.test.ts
  • packages/cli/src/commands/install/__tests__/install-cli.test.ts
  • packages/cli/src/util/__tests__/adapter-install-errors.test.ts
  • packages/engine/src/__tests__/build-pipeline.test.ts
  • packages/engine/src/__tests__/materialize.test.ts
  • packages/engine/src/__tests__/run-install.test.ts
  • packages/engine/src/adapters/__tests__/api-compatibility.test.ts
  • packages/engine/src/adapters/__tests__/inspect.test.ts
  • packages/engine/src/adapters/__tests__/placement-managed.test.ts
  • packages/engine/src/adapters/__tests__/verify.test.ts
  • packages/engine/src/adapters/api-compatibility.ts
  • packages/engine/src/adapters/verify.ts
  • packages/engine/src/install/__tests__/run-add.test.ts
  • packages/engine/src/install/__tests__/run-install.chain.test.ts
  • packages/engine/src/install/__tests__/run-install.receipt.test.ts
  • packages/engine/src/install/__tests__/run-install.test.ts
  • packages/engine/src/install/__tests__/run-remove.test.ts
  • packages/engine/src/install/materialize.ts

Comment thread docs/guides/custom-adapters.mdx
Comment on lines +316 to +321
for (const [relPath, bytes] of Object.entries(options.companions)) {
// biome-ignore lint/style/noNonNullAssertion: resolved contains every validated path
const abs = resolved.get(relPath)!
currentPath = abs
await mkdir(dirname(abs), { recursive: true })
await writeFile(abs, bytes)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 security Root-level companion bypasses symlink guard, enabling arbitrary file write

rejectSymlinkedParents(abs, root) starts its walk at dirname(abs). For a root-level companion like api.md, dirname(abs) equals root, so the while condition current !== root is immediately false — zero iterations run and no symlink check occurs. Additionally, abs itself (the final path component) is never lstat-checked, so a pre-placed symlink at <root>/api.md passes all validation and the subsequent writeFile(abs, bytes) follows the link to an external target. An attacker who can create a file at the skill root level (or a path whose final component is a symlink, with real parent dirs) redirects companion bytes outside the root on any install that names that path.

Fix: call lstat(abs) after the parent walk and reject if it is a symbolic link.

How this was verified: The security-review subagent created a symlink at <skill-root>/api.md → /outside/api.md, called installSkillBundle with companion key 'api.md', and confirmed writeFile followed the link and wrote the companion bytes to the external target. The root-level bypass (while loop never executes) was confirmed by tracing installSkillBundle → resolveCompanionPaths → rejectSymlinkedParents; the external file contained the injected content.

Comment on lines +313 to +321
try {
await mkdir(dirname(paths.primaryFile), { recursive: true })
await writeFile(paths.primaryFile, assembleAssetContent(options.content, options.metadata), 'utf8')
for (const [relPath, bytes] of Object.entries(options.companions)) {
// biome-ignore lint/style/noNonNullAssertion: resolved contains every validated path
const abs = resolved.get(relPath)!
currentPath = abs
await mkdir(dirname(abs), { recursive: true })
await writeFile(abs, bytes)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P0 security TOCTOU race: symlink inserted between validation and mutation can redirect companion write

resolveCompanionPaths lstat-checks parent directories during the validation phase, but mkdir/writeFile execute in a separate block after validation completes. An actor who can write to the skill directory during that window can atomically rename a validated real directory to a symlink pointing outside the root. The subsequent mkdir(dirname(abs), { recursive: true }) follows the link, and writeFile(abs, bytes) writes companion bytes to the external location. The race window spans the entire snapshot + mutation phase.

Practical mitigations include: re-lstat the resolved path immediately before each write (with the understanding that races still exist at a narrow level), writing to a temp path inside the root then using rename, or using platform-specific no-follow open semantics.

How this was verified: The security-review subagent validated a companion path with a real parent directory, then replaced the parent with a symlink targeting an external dir before the mutation phase ran. The companion write landed outside the skill root; exit code 0; external file contained the injected content.

eXamadeus commented Jul 24, 2026

Copy link
Copy Markdown
Member Author

Merge activity

  • Jul 24, 4:47 AM UTC: A user started a stack merge that includes this pull request via Graphite.
  • Jul 24, 4:47 AM UTC: @eXamadeus merged this pull request with Graphite.

@eXamadeus
eXamadeus merged commit d20cdae into main Jul 24, 2026
8 of 9 checks passed
@eXamadeus
eXamadeus deleted the julian/07-20-replace_positional_adapter_asset_contract_with_tagged_request_result_unions_and_atomic_skill-bundle_helpers branch July 24, 2026 04:47
eXamadeus pushed a commit that referenced this pull request Jul 24, 2026
This PR was auto-generated by the release workflow. When you're ready to do a release, you can merge this and the packages will be published to npm automatically. If you're not ready to do a release yet, that's fine, whenever you add more changesets to main, this PR will be updated.

# Releases
## @agent-facets/adapter@0.28.0

### Minor Changes

- [#438](#438) [`d20cdae`](d20cdae) Thanks [@eXamadeus](https://github.com/eXamadeus)! - **BREAKING (pre-1.0 minor):** the adapter asset contract is now tagged request/result unions instead of positional parameters, and the adapter API identifier advances from `0.0` to `0.1`.
    `installAsset`, `readAsset`, and `deleteAsset` each take a single request object tagged by `assetType` and return a discriminated result — expected failures (`not-found`, `invalid-companion-path`, `unsupported-scope`, `not-implemented`, `io-failed`) are structured values, never thrown errors. Skill requests carry a companion byte map plus the caller-verified owned companion path set for atomic multi-file skill bundles; agent and command requests structurally cannot carry companions. `defineAdapter` stubs for omitted methods now return `not-implemented` failures instead of throwing.
    The SDK's canonical `ADAPTER_API_VERSION` is now `0.1`, identifying this tagged contract; `defineAdapter()` stamps it and first-party packages publish `"facetAdapterApiVersion": "0.1"`. `0.0` named the earlier positional contract: a CLI that supports only `0.1` classifies a `0.0` adapter as well-formed but unsupported and fails closed (before any contract method or project write) with reinstall guidance. There is no positional/tagged compatibility bridge — an adapter built against `0.0` must be rebuilt against a `0.1` SDK release and reinstalled.
    New SDK helpers: `installSkillBundle` / `readSkillBundle` / `deleteSkillBundle` (staged all-or-nothing bundle replacement with rollback, ownership-set-based deletion, and empty-directory pruning), `installSingleFileAsset` / `readSingleFileAsset` / `deleteSingleFileAsset` (result-shaped single-file operations), and `validateContainedRelativePath` (pre-filesystem containment validation applied to every supplied companion path).
    Every adapter implementing the previous positional contract must migrate. The first-party claude-code, opencode, and codex adapters are migrated in their matching minor releases; codex delete operations now prune emptied directories consistently with the other adapters.
    Release ordering: this SDK release and the three first-party adapter releases publish `0.1` to npm **before** any `agent-facets` CLI release requires `0.1`. Until that CLI ships, existing `0.0` CLIs keep selecting the highest compatible `0.0` adapter release, so this changeset intentionally carries **no** `agent-facets` bump — the CLI change that makes `0.1` the supported set lands in a later release cycle gated on all three first-party adapters having published `facetAdapterApiVersion: 0.1`.

## @agent-facets/adapter-claude-code@0.9.0

### Minor Changes

- [#438](#438) [`d20cdae`](d20cdae) Thanks [@eXamadeus](https://github.com/eXamadeus)! - **BREAKING (pre-1.0 minor):** the adapter asset contract is now tagged request/result unions instead of positional parameters, and the adapter API identifier advances from `0.0` to `0.1`.
    `installAsset`, `readAsset`, and `deleteAsset` each take a single request object tagged by `assetType` and return a discriminated result — expected failures (`not-found`, `invalid-companion-path`, `unsupported-scope`, `not-implemented`, `io-failed`) are structured values, never thrown errors. Skill requests carry a companion byte map plus the caller-verified owned companion path set for atomic multi-file skill bundles; agent and command requests structurally cannot carry companions. `defineAdapter` stubs for omitted methods now return `not-implemented` failures instead of throwing.
    The SDK's canonical `ADAPTER_API_VERSION` is now `0.1`, identifying this tagged contract; `defineAdapter()` stamps it and first-party packages publish `"facetAdapterApiVersion": "0.1"`. `0.0` named the earlier positional contract: a CLI that supports only `0.1` classifies a `0.0` adapter as well-formed but unsupported and fails closed (before any contract method or project write) with reinstall guidance. There is no positional/tagged compatibility bridge — an adapter built against `0.0` must be rebuilt against a `0.1` SDK release and reinstalled.
    New SDK helpers: `installSkillBundle` / `readSkillBundle` / `deleteSkillBundle` (staged all-or-nothing bundle replacement with rollback, ownership-set-based deletion, and empty-directory pruning), `installSingleFileAsset` / `readSingleFileAsset` / `deleteSingleFileAsset` (result-shaped single-file operations), and `validateContainedRelativePath` (pre-filesystem containment validation applied to every supplied companion path).
    Every adapter implementing the previous positional contract must migrate. The first-party claude-code, opencode, and codex adapters are migrated in their matching minor releases; codex delete operations now prune emptied directories consistently with the other adapters.
    Release ordering: this SDK release and the three first-party adapter releases publish `0.1` to npm **before** any `agent-facets` CLI release requires `0.1`. Until that CLI ships, existing `0.0` CLIs keep selecting the highest compatible `0.0` adapter release, so this changeset intentionally carries **no** `agent-facets` bump — the CLI change that makes `0.1` the supported set lands in a later release cycle gated on all three first-party adapters having published `facetAdapterApiVersion: 0.1`.

## @agent-facets/adapter-codex@0.7.0

### Minor Changes

- [#438](#438) [`d20cdae`](d20cdae) Thanks [@eXamadeus](https://github.com/eXamadeus)! - **BREAKING (pre-1.0 minor):** the adapter asset contract is now tagged request/result unions instead of positional parameters, and the adapter API identifier advances from `0.0` to `0.1`.
    `installAsset`, `readAsset`, and `deleteAsset` each take a single request object tagged by `assetType` and return a discriminated result — expected failures (`not-found`, `invalid-companion-path`, `unsupported-scope`, `not-implemented`, `io-failed`) are structured values, never thrown errors. Skill requests carry a companion byte map plus the caller-verified owned companion path set for atomic multi-file skill bundles; agent and command requests structurally cannot carry companions. `defineAdapter` stubs for omitted methods now return `not-implemented` failures instead of throwing.
    The SDK's canonical `ADAPTER_API_VERSION` is now `0.1`, identifying this tagged contract; `defineAdapter()` stamps it and first-party packages publish `"facetAdapterApiVersion": "0.1"`. `0.0` named the earlier positional contract: a CLI that supports only `0.1` classifies a `0.0` adapter as well-formed but unsupported and fails closed (before any contract method or project write) with reinstall guidance. There is no positional/tagged compatibility bridge — an adapter built against `0.0` must be rebuilt against a `0.1` SDK release and reinstalled.
    New SDK helpers: `installSkillBundle` / `readSkillBundle` / `deleteSkillBundle` (staged all-or-nothing bundle replacement with rollback, ownership-set-based deletion, and empty-directory pruning), `installSingleFileAsset` / `readSingleFileAsset` / `deleteSingleFileAsset` (result-shaped single-file operations), and `validateContainedRelativePath` (pre-filesystem containment validation applied to every supplied companion path).
    Every adapter implementing the previous positional contract must migrate. The first-party claude-code, opencode, and codex adapters are migrated in their matching minor releases; codex delete operations now prune emptied directories consistently with the other adapters.
    Release ordering: this SDK release and the three first-party adapter releases publish `0.1` to npm **before** any `agent-facets` CLI release requires `0.1`. Until that CLI ships, existing `0.0` CLIs keep selecting the highest compatible `0.0` adapter release, so this changeset intentionally carries **no** `agent-facets` bump — the CLI change that makes `0.1` the supported set lands in a later release cycle gated on all three first-party adapters having published `facetAdapterApiVersion: 0.1`.

## @agent-facets/adapter-opencode@0.10.0

### Minor Changes

- [#438](#438) [`d20cdae`](d20cdae) Thanks [@eXamadeus](https://github.com/eXamadeus)! - **BREAKING (pre-1.0 minor):** the adapter asset contract is now tagged request/result unions instead of positional parameters, and the adapter API identifier advances from `0.0` to `0.1`.
    `installAsset`, `readAsset`, and `deleteAsset` each take a single request object tagged by `assetType` and return a discriminated result — expected failures (`not-found`, `invalid-companion-path`, `unsupported-scope`, `not-implemented`, `io-failed`) are structured values, never thrown errors. Skill requests carry a companion byte map plus the caller-verified owned companion path set for atomic multi-file skill bundles; agent and command requests structurally cannot carry companions. `defineAdapter` stubs for omitted methods now return `not-implemented` failures instead of throwing.
    The SDK's canonical `ADAPTER_API_VERSION` is now `0.1`, identifying this tagged contract; `defineAdapter()` stamps it and first-party packages publish `"facetAdapterApiVersion": "0.1"`. `0.0` named the earlier positional contract: a CLI that supports only `0.1` classifies a `0.0` adapter as well-formed but unsupported and fails closed (before any contract method or project write) with reinstall guidance. There is no positional/tagged compatibility bridge — an adapter built against `0.0` must be rebuilt against a `0.1` SDK release and reinstalled.
    New SDK helpers: `installSkillBundle` / `readSkillBundle` / `deleteSkillBundle` (staged all-or-nothing bundle replacement with rollback, ownership-set-based deletion, and empty-directory pruning), `installSingleFileAsset` / `readSingleFileAsset` / `deleteSingleFileAsset` (result-shaped single-file operations), and `validateContainedRelativePath` (pre-filesystem containment validation applied to every supplied companion path).
    Every adapter implementing the previous positional contract must migrate. The first-party claude-code, opencode, and codex adapters are migrated in their matching minor releases; codex delete operations now prune emptied directories consistently with the other adapters.
    Release ordering: this SDK release and the three first-party adapter releases publish `0.1` to npm **before** any `agent-facets` CLI release requires `0.1`. Until that CLI ships, existing `0.0` CLIs keep selecting the highest compatible `0.0` adapter release, so this changeset intentionally carries **no** `agent-facets` bump — the CLI change that makes `0.1` the supported set lands in a later release cycle gated on all three first-party adapters having published `facetAdapterApiVersion: 0.1`.

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Medium Risk**
> Publishes a breaking pre-1.0 adapter contract (`0.1`) that forces third-party adapter rebuilds; lockfile schema change may affect tooling that reads facets.lock.
> 
> **Overview**
> Automated **release** PR that bumps **`@agent-facets/adapter`** to **0.28.0** and the claude-code, opencode, and codex adapter packages to matching minors, with changelog entries for the **adapter API `0.0` → `0.1`** breaking change from [#438](#438). The consumed changeset is removed and **`bun.lock`** version pins are updated accordingly; there is **no `agent-facets` CLI bump** in this cycle.
> 
> **`facets.lock`** moves to **`lockfileVersion` 0.2**, adding per-asset **`files`** entries (relative path + `sha256` integrity) for every locked facet asset instead of name-only asset records.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit 95e8cc6. Bugbot is set up for automated code reviews on this repo. Configure [here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant