Conversation
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (22)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30f2eb4996
ℹ️ 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 = opts.companionBytes?.get(assetKey(asset)) ?? {} | ||
| const ownedCompanionPaths = oldOwnedByKey.get(assetKey(asset)) ?? [] |
There was a problem hiding this comment.
For every frozen reproduction path, companionBytes is left undefined; this fallback therefore sends an empty companion bundle while still passing the lockfile's existing owned paths. The adapter treats that as a replacement and deletes every owned companion absent from {}. Consequently, facet install --frozen-lockfile for an already-installed 0.2 skill with companions can silently strip all of its companion files even though the verified artifact and lockfile have not changed.
Useful? React with 👍 / 👎.
| // converges the bundle back to its prior state without touching unowned | ||
| // files. Companion-less skills and single-file assets restore exactly | ||
| // as before. | ||
| const restoreOwned = previous ? Object.keys(previous.companions) : ownedCompanionPaths |
There was a problem hiding this comment.
When an existing skill update adds a companion and a later operation fails, restoreOwned contains only the companions that existed before the update. The rollback install restores the old companion map but does not list the newly added path as owned, so the adapter preserves that new file rather than removing it. This leaves a partial new bundle after a reported rollback; the ownership set must include paths from both the prior and newly installed bundles.
Useful? React with 👍 / 👎.
| adapters: [...adapters], | ||
| oldAssets, | ||
| newAssets: entry.assets, | ||
| companionBytes, |
There was a problem hiding this comment.
This replacement path derives oldAssets solely from the checked-out lockfile, despite the receipt being the record of what this machine actually materialized. For example, after a pull changes a skill from v1 (with references/old.md) to v2 (without it), the current lockfile no longer supplies old.md as an owned path, so the v2 install preserves that v1 companion and then rewrites the receipt to v2, losing the only cleanup record. Feed the matching receipt asset ownership into replacement materialization so obsolete companions are removed.
Useful? React with 👍 / 👎.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| * - `per-file-integrity` — a locked per-file hash does not equal the | ||
| * recomputed archive-entry hash for that exact path. | ||
| */ | ||
| | { code: 'RECONCILE_FACET_INTEGRITY'; facet: string; expected: string; actual: string } |
There was a problem hiding this comment.
Unused reconcile facet failure code
Low Severity
RECONCILE_FACET_INTEGRITY is part of RunInstallFailure and fully rendered in the CLI, but reconcileLockedAgainstPlan never constructs it. Facet integrity only gates whether reconciliation runs; mismatches surface elsewhere or are skipped. The dead variant adds maintenance cost and implies a diagnostic path that cannot occur.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 30f2eb4. Configure here.
30f2eb4 to
f43bc42
Compare
fde47ed to
92429da
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f43bc42e40
ℹ️ 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".
| // Every skill companion path is derived by `planArchiveEntries` to live | ||
| // below the skill root, so this prefix strip is total. | ||
| const relative = file.path.startsWith(skillRoot) ? file.path.slice(skillRoot.length) : file.path | ||
| companions[relative] = new Uint8Array(readFileSync(join(verifiedDir, file.path))) |
There was a problem hiding this comment.
If a companion becomes unreadable after buildVerifiedAssetPlan hashes it—for example, a local source is edited concurrently or a cache file is removed—this synchronous read throws. Every resolver invokes this helper without catching that exception, so runInstall rejects instead of returning its documented RunInstallResult failure and cannot report or roll back cleanly. Convert this read failure into a discriminated result that the resolver maps to an install failure.
AGENTS.md reference: AGENTS.md:L172-L178
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit f43bc42. Configure here.
Merge activity
|
…ciliation through install, receipt, and CLI rendering
f43bc42 to
82a46cf
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82a46cf149
ℹ️ 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".
| if (asset.type !== 'skill') continue | ||
| const skillRoot = `skills/${asset.name}/` | ||
| const primary = `skills/${asset.name}/SKILL.md` | ||
| const companions: SkillCompanionBytes = {} |
There was a problem hiding this comment.
A valid companion path can be exactly __proto__: the archive-plan path grammar permits it, but assigning it to this ordinary {} object invokes the inherited __proto__ setter instead of creating an own property. Object.entries(companions) in the adapter then omits that companion, so installing a skill that declares files: ['__proto__'] records it in the lockfile/receipt but never writes it to the adapter. Initialize this map with Object.create(null) (as the adapter's read path does) so every validated path is represented.
Useful? React with 👍 / 👎.
1 participant
Why
Completes the non-asset file materialization work (tasks 9.3–9.11): skill companion files (e.g.
references/api.md,assets/logo.bin) are now fully materialized, tracked, repaired, and removed alongside their primarySKILL.md, with all state changes committed transactionally and all failure modes surfaced with exact diagnostic output.Details
Pre-materialization reconciliation (9.3). Before any adapter write, when the same artifact is being reproduced (identical facet-level integrity), the previously-locked
0.2entry is checked against the freshly-derived verified asset plan. Mismatches produce four structured failure codes —RECONCILE_FACET_INTEGRITY,RECONCILE_ASSET_IDENTITY,RECONCILE_OWNED_PATH_SET, andRECONCILE_PER_FILE_INTEGRITY— each carrying the exact path or identity that diverged. Because reconciliation runs before materialization, a mismatch leaves all project, lockfile, receipt, and adapter state untouched. Legacy (1) entries skip per-file reconciliation (migration is intentionally permissive at the file level).Receipt
0.2ownership (9.4). The receipt now carries afiles: string[]array per asset recording the exact inner-archive paths materialized for it. Legacy (1) receipts are refined on load to primary-only ownership (one conventional path per asset). Owned file paths are validated for path traversal on load; a bad path drops the whole asset record rather than partially trusting it.Transactional commit and frozen consistency gate (9.5). The frozen consistency gate (lockfile drift detection) now explicitly completes before receipt-driven cleanup begins. A frozen install that would reject an orphaned lockfile entry fails before any materialized asset is deleted, leaving adapter state untouched.
Companion materialization with skip/repair and rollback (9.6).
materializenow receives companion bytes (keyedskill:<name>) from the resolver's verified asset plan and passes them through to adapter install/read/delete requests with the engine-verified owned-path set. Skip-if-identical compares the full bundle (primary + every companion); a single drifted companion forces an atomic bundle replacement. Rollback preimages capture the complete prior bundle so an undo restores the exact previous state.Path-specific drift and offline removal (9.7). Drift reporting names the exact companion paths that changed (drifted, added, or removed). Removal passes the receipt's owned-path set to the adapter delete request so every owned companion is deleted offline without cache or network access, while unowned files in the skill directory are never touched.
Archive compatibility table (9.8). A single
archiveCompatibilityGuidanceutility maps a known archive format to the minimumagent-facetsrelease that supports it, or advises updating to latest for an unknown future format without inventing a minimum version. TheUNSUPPORTED_ARCHIVEregistry error path now uses this table. The CLI renders all four reconciliation failure codes with exact paths and both hash values.readSkillCompanionBytes. A new helper reads companion bytes verbatim from the verified directory, converting full inner-archive paths to skill-root-relative form for the adapter contract. It is called by all three resolvers (local, git, registry) immediately afterbuildVerifiedAssetPlanand threaded throughResolvedFacetalongside the plan.Verification
New and updated tests cover: per-file reconciliation abort before any write; full companion install with binary file round-trip; skip-identical bundle; single-companion drift detection and repair with exact log output; unowned file preservation across update and removal; offline receipt-driven multi-file removal; archive-only file withholding (verified but never materialized or locked); interrupted-install convergence; frozen orphan rejection before cleanup; legacy receipt refinement; owned-path escape containment; and the archive compatibility table for known, unknown, and missing format versions. All focused install, materialization, receipt, lockfile, cache, registry, and CLI install tests pass.
Note
Medium Risk
Changes core install commit, materialization, and receipt semantics (multi-file deletes and frozen ordering); mistakes could corrupt adapter trees or skip cleanup, but failures are fail-closed with extensive test coverage.
Overview
Finishes tasks 9.3–9.11 for non-asset / multi-file skill support: companions are installed, reconciled, repaired, removed offline, and surfaced in the CLI with structured errors.
Engine — before any adapter write,
reconcileLockedAgainstPlancompares a reproduced0.2lockfile entry to the freshly derived verified asset plan (asset identities, owned path sets, per-file hashes). Mismatches returnRECONCILE_*failures and leave lockfile, receipt, and adapter state unchanged. Resolvers threadplanandreadSkillCompanionBytesthroughResolvedFacet.Materialization passes companion bytes and engine-owned path sets into tagged adapter skill requests; skip-if-identical covers the full bundle; rollback journals restore primary + companions; drift logs name specific companion paths; deletes use receipt/lockfile owned paths and leave unowned files alone.
Receipt
0.2stores per-assetfiles[], validates untrusted paths on load, refines legacy1to primary-only ownership, and mirrors lockfile paths in tri-write bootstrap.Frozen installs run lockfile drift (including orphans) before receipt-driven cleanup so assets are not deleted when frozen would reject the state.
CLI adds
archiveCompatibilityGuidanceforUNSUPPORTED_ARCHIVE, plus install UI for all four reconcile failure variants (path + hashes where applicable).Tests cover reconciliation abort, multi-file skill lifecycle, offline removal, archive-only withholding, partial-install convergence, frozen orphan ordering, receipt legacy/escape handling, and compatibility guidance.
Reviewed by Cursor Bugbot for commit 82a46cf. Bugbot is set up for automated code reviews on this repo. Configure here.