Conversation
|
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: fde47edfbd
ℹ️ 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 (key === undefined) { | ||
| throw new Error(`internal: companion "${entry.path}" references undeclared skill "${entry.skill}"`) | ||
| } |
There was a problem hiding this comment.
SKILL.md
A valid skill companion such as README.md or .meta sorts before skills/<name>/SKILL.md in planArchiveEntries, so this branch runs before the corresponding primary-asset has populated skillKeyByName. The resulting throw escapes buildVerifiedAssetPlan (and therefore runInstall, which only releases its lock in finally) instead of returning the advertised BUILD_FAILED result, causing installation of otherwise valid facets to crash. Build the primary-asset lookup independently of iteration order, or return a discriminated failure here.
AGENTS.md reference: AGENTS.md:L172-L178
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: fccd5d08-68f0-4d11-b616-80b7c41a640f
📒 Files selected for processing (13)
openspec/changes/support-non-asset-files/tasks.mdpackages/cli/src/commands/install/__tests__/install-cli.test.tspackages/engine/src/__tests__/run-install.test.tspackages/engine/src/install/__tests__/lockfile-io.test.tspackages/engine/src/install/__tests__/run-remove.test.tspackages/engine/src/install/commit/finalize-facet.tspackages/engine/src/install/commit/resolve-facet.tspackages/engine/src/install/commit/resolve-git.tspackages/engine/src/install/commit/resolve-local.tspackages/engine/src/install/commit/resolve-registry.tspackages/engine/src/install/lockfile-io.tspackages/engine/src/install/run-install.tspackages/engine/src/install/verified-asset-plan.ts
| import { | ||
| CURRENT_LOCKFILE_VERSION, | ||
| type LEGACY_LOCKFILE_VERSION, | ||
| type Lockfile, | ||
| type LockfileParseFailure, | ||
| parseLockfileDocument, | ||
| SUPPORTED_LOCKFILE_VERSIONS, | ||
| } from '@agent-facets/protocol' |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Locate relevant files:"
fd -a 'lockfile-io\.ts$|.*protocol.*' . | sed 's#^\./##' | head -100
echo
echo "File outline:"
ast-grep outline packages/engine/src/install/lockfile-io.ts --view expanded || true
echo
echo "Relevant lockfile-io.ts lines:"
cat -n packages/engine/src/install/lockfile-io.ts | sed -n '1,80p'
echo
echo "Search lockfile version definitions:"
rg -n "LEGACY_LOCKFILE_VERSION|CURRENT_LOCKFILE_VERSION|SUPPORTED_LOCKFILE_VERSIONS|type LoadedLockfileVersion" packages -SRepository: agent-facets/facets
Length of output: 12545
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Protocol lockfile version exports:"
cat -n packages/protocol/src/schemas/lockfile.ts | sed -n '1,45p'
echo
echo "Protocol index exports around constants:"
cat -n packages/protocol/src/index.ts | sed -n '138,152p'
echo
echo "TypeScript availability:"
type tsc || true
if command -v tsc >/dev/null 2>&1; then
tmp="$(mktemp -d)"
cat > "$tmp/import-type-about.ts" <<'TS'
import { LEGACY_LOCKFILE_VERSION, CURRENT_LOCKFILE_VERSION } from './@agent-facets/protocol'
export type LoadedLockfileVersion = typeof LEGACY_LOCKFILE_VERSION | typeof CURRENT_LOCKFILE_VERSION
declare const _: LoadedLockfileVersion
console.log(_)
TS
(cd "$tmp" && tsc --noEmit --strict import-type-about.ts) || true
fi
echo
echo "Standalone TypeScript import-type about typeof semantics:"
if command -v tsc >/dev/null 2>&1; then
tmp="$(mktemp -d)"
cat > "$tmp/lib.ts" <<'TS'
export const LEGACY_LOCKFILE_VERSION = 1
export const CURRENT_LOCKFILE_VERSION = 0.2
TS
cat > "$tmp/main.ts" <<'TS'
import { LEGACY_LOCKFILE_VERSION, CURRENT_LOCKFILE_VERSION } from './lib'
import { type LEGACY_LOCKFILE_VERSION as TypeOnlyLEGACY, CURRENT_LOCKFILE_VERSION as CurrentImport } from './lib'
export type LoadedLockfileVersion = typeof LEGACY_LOCKFILE_VERSION | typeof CURRENT_LOCKFILE_VERSION
export type LoadedLockfileVersionFromTypeImport = typeof TypeOnlyLEGACY | typeof CurrentImport
TS
(cd "$tmp" && tsc --noEmit --strict main.ts && echo "compile ok") || (cd "$tmp" && tsc --noEmit --strict main.ts) >/tmp/tsc_out.txt 2>&1
cat /tmp/tsc_out.txt
fiRepository: agent-facets/facets
Length of output: 3332
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
cat > "$tmp/lib.ts" <<'TS'
export const LEGACY_LOCKFILE_VERSION = 1
export const CURRENT_LOCKFILE_VERSION = 0.2
TS
cat > "$tmp/main.ts" <<'TS'
import { LEGACY_LOCKFILE_VERSION, CURRENT_LOCKFILE_VERSION } from './lib'
import type { LEGACY_LOCKFILE_VERSION as TypeOnlyLegacy, CURRENT_LOCKFILE_VERSION as TypeOnlyCurrent } from './lib'
export type LoadedLockfileVersionFromValueImports = typeof LEGACY_LOCKFILE_VERSION | typeof CURRENT_LOCKFILE_VERSION
export type LoadedLockfileVersionFromTypeImports = typeof TypeOnlyLegacy | typeof TypeOnlyCurrent
export type LoadedLockfileVersionMixed = typeof TypeOnlyLegacy | typeof CURRENT_RUNTIME_IMPORT
import { CURRENT_LOCKFILE_VERSION as CURRENT_RUNTIME_IMPORT } from './lib'
TS
(
cd "$tmp"
echo "compiler:"
tsc --version
echo
tsc --noEmit --strict main.ts
) || trueRepository: agent-facets/facets
Length of output: 182
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
tmp="$(mktemp -d)"
cat > "$tmp/lib.ts" <<'TS'
export const LEGACY_LOCKFILE_VERSION = 1
export const CURRENT_LOCKFILE_VERSION = 0.2
TS
cat > "$tmp/main.ts" <<'TS'
import { LEGACY_LOCKFILE_VERSION, CURRENT_LOCKFILE_VERSION } from './lib'
import type { LEGACY_LOCKFILE_VERSION as TypeOnlyLegacy, CURRENT_LOCKFILE_VERSION as TypeOnlyCurrent } from './lib'
export type LoadedLockfileVersionFromValueImports = typeof LEGACY_CHECK | typeof CURRENT_CHECK
const TYPE_ONLY_LEGACY_CHECK = TypeOnlyLegacy
const TYPE_ONLY_CURRENT_CHECK = TypeOnlyCurrent
export type LoadedLockfileVersionMixed = typeof LEGACY_CHECK | typeof CURRENT_CHECK
import { LEGACY_LOCKFILE_VERSION as LEGACY_CHECK, CURRENT_LOCKFILE_VERSION as CURRENT_CHECK } from './lib'
TS
(
cd "$tmp"
echo "compiler:"
tsc --version
echo
tsc --noEmit --strict main.ts
) || trueRepository: agent-facets/facets
Length of output: 417
Use a value import for LEGACY_LOCKFILE_VERSION.
LEGACY_LOCKFILE_VERSION is declared as export const in protocol, but in lockfile-io.ts it is imported with the per-specifier type modifier while LoadedLockfileVersion uses typeof LEGACY_LOCKFILE_VERSION. Drop the type modifier on that import specifier; the import itself is not used as a runtime value here, and the type import can remain if LEGACY_LOCKFILE_VERSION is imported as a value from protocol for this file.
fde47ed to
92429da
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
There are 2 total unresolved issues (including 1 from previous review).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 92429da. Configure here.
| scope: 'project', | ||
| type: entry.assetType, | ||
| name: entry.name, | ||
| files: [{ path: entry.path, integrity: hashFor(entry.path) }], |
There was a problem hiding this comment.
Migration resets asset scope to project
Medium Severity
The buildVerifiedAssetPlan function hardcodes asset scope to 'project'. This overwrites existing user or system scopes during lockfile migration or re-derivation, which can change materialization targets and drift deletion keys.
Reviewed by Cursor Bugbot for commit 92429da. Configure here.
Merge activity
|
…ion dispatch, and `buildVerifiedAssetPlan`
92429da to
7a385fb
Compare
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a385fb878
ℹ️ 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".
| // verbatim, and it never reaches this build branch (an audited hit is | ||
| // required for frozen reproduction; a frozen rebuild would fail the | ||
| // one-check reproduction guard rather than rewrite). | ||
| const plan = buildVerifiedAssetPlan(content.manifest, sourceDir) |
There was a problem hiding this comment.
For a git facet that declares a skill companion or top-level supplementary file, runBuildPipeline still produces buildManifest.assets from the legacy primary-only collector, so the cache sidecar and its later self-audit do not cover that declared file. This new call then reads that unverified file from the cache and writes its hash into the 0.2 lockfile; a companion modified after caching therefore passes the cached integrity check and is silently re-pinned on a normal install. Include the complete archive plan in the build/cache verification set (or verify this plan against a complete trusted archive) before deriving files[].
Useful? React with 👍 / 👎.
1 participant
Why
Tasks 9.1 and 9.2 of the non-asset-files spec: replace the legacy numeric-ordered lockfile version handling with exact version dispatch, and derive per-file integrity records from verified on-disk bytes rather than copying self-declared values from build manifests.
Details
Exact version dispatch (design D10).
loadLockfilenow delegates toparseLockfileDocumentin the protocol package, which selects the legacy-alpha1schema or the current0.2schema by exact equality. An unknown version produces a structuredunsupported-lockfile-versionfailure with an actionable "Upgrade the CLI" message. A malformed0.2document is never silently reinterpreted as legacy1. The loaded version is surfaced asresult.versionso the orchestrator can branch on it without re-parsing the raw document.Current lockfile version is
0.2. The version constant is renamed fromLOCKFILE_VERSIONtoCURRENT_LOCKFILE_VERSION(value0.2), withLEGACY_LOCKFILE_VERSION(1) introduced for the compatibility window. Version dispatch is by exact equality, so0.2is "current" even though it is numerically less than1.Per-file integrity via
buildVerifiedAssetPlan. A newverified-asset-plan.tsmodule is the single producer offiles[]records for every resolve path (registry, git, local). It classifies inner-archive entries using the shared protocolplanArchiveEntriesderivation (design D3/D12) and recomputes each file's hash from the verified on-disk bytes viacomputeDirIntegrity— never from a build manifest's self-declared value. The plan carries only paths, hashes, and ownership; no bytes.Migration behavior. A normal install always writes the current
0.2schema. When a legacy1lockfile is loaded and the content reproduces the locked integrity, the entry's identity (source/version/integrity) is preserved from the lockfile butassets[].filesis re-derived from the verified slot, migrating the entry to0.2. Frozen mode never rewrites the lockfile, so a legacy entry is inherited verbatim and the writtenlockfileVersionstays1.Asset ordering. Assets are sorted skills → agents → commands, then by name; each asset's
filesarray is sorted lexicographically by path. This keeps lockfile diffs stable across installs.Verification
CI. New and updated tests cover: fresh installs writing
0.2lockfiles with recomputed per-file records; round-trip loading of both0.2and legacy1documents; exact version dispatch rejecting unsupported versions; migration of a legacy1lockfile to0.2on a normal (non-frozen) install; and frozen installs retaining the loaded version verbatim.Note
Medium Risk
Changes core install lockfile I/O and how every facet source builds lockfile asset metadata; behavior diverges between normal migration and frozen no-rewrite paths, with broad test coverage but receipt/materialization follow-ups still pending.
Overview
Install now treats
facets.lock0.2as current: each asset carries afiles[]list of inner-archive paths with recomputedsha256hashes, not identity-only rows.loadLockfileuses protocolparseLockfileDocumentwith exact version dispatch (1vs0.2, not numeric ordering), surfacesresult.version, and bootstraps missing files at0.2.buildVerifiedAssetPlanis the single producer for lockfile asset records on registry, git, and local resolve paths: it classifies entries viaplanArchiveEntriesand hashes verified on-disk bytes viacomputeDirIntegrity, replacingcomputeAssetListat commit time.loadFacetContentalso returns the rawFacetManifestfor that classification.Migration: a normal install always writes
lockfileVersion: 0.2while preserving locked facet identity (source/version/integrity) and re-derivingfiles[]from the verified slot. Frozen installs inherit lockfile entries verbatim (legacy1stays1) and keep the loaded top-level version.Tests and the CLI install happy path assert
0.2output, legacy1load, unsupported-version errors, and git cache-hit migration to per-file records.Reviewed by Cursor Bugbot for commit 7a385fb. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit