Add versioned build-manifest, lockfile, and archive-plan schemas with exact facetVersion/lockfileVersion dispatch and supplementary files support - #435
Conversation
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThe protocol adds deterministic archive planning for primary, companion, and supplementary files, with portable-path and collision validation. Facet manifests now distinguish current and legacy naming rules and support supplementary files. Duplicate JSON members are rejected before schema validation. Build manifests and lockfiles gain version-specific schemas, exact-version parsers, structured failures, and expanded public exports. Embedded archive validation uses the legacy facet schema, while engine tests cover updated cross-namespace naming behavior. Sequence Diagram(s)sequenceDiagram
participant BuildPipeline
participant FacetManifestSchema
participant planArchiveEntries
participant ArchiveWriter
BuildPipeline->>FacetManifestSchema: validate facet manifest
FacetManifestSchema->>planArchiveEntries: validate and derive archive entries
planArchiveEntries-->>FacetManifestSchema: ordered entries or errors
FacetManifestSchema-->>BuildPipeline: validation result
BuildPipeline->>ArchiveWriter: write planned archive entries
✨ 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: 2dbd6c3f2b
ℹ️ 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 (!record) continue | ||
| for (const key of Object.keys(record)) { | ||
| const check = validateAssetName(key) | ||
| const check = validateAssetNameSegment(key) |
There was a problem hiding this comment.
runBuildPipeline still emits facetVersion: 0.1 and loads manifests through validateFacetManifest (packages/engine/src/loaders/facet.ts:34), so this current-only single-segment check rejects existing legacy-valid names such as viper-plans/planning before they can be built. The new legacy schema is only used by archive verification, not by the producer; route the 0.1 producer through the legacy validator or defer this restriction until it emits 0.2 artifacts.
Useful? React with 👍 / 👎.
| const SkillDescriptor = type({ | ||
| description: 'string', | ||
| 'adapters?': type.Record('string', 'unknown'), | ||
| 'files?': 'string[]', | ||
| }) |
There was a problem hiding this comment.
A manifest with skills.<name>.files now validates successfully, but the active build path calls collectArchiveEntries, which only adds facet.json and conventional primary prompts (packages/protocol/src/build/content-hash.ts:39-58), then writes a 0.1 manifest. Consequently declared companion files are silently omitted from the emitted archive while facet.json advertises them; either include these files via the archive plan or reject the declarations until the 0.2 producer is enabled.
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}
| ) | ||
| continue | ||
| } | ||
| if (declared.split('/')[0] === 'skills') { |
There was a problem hiding this comment.
site-top-level-under-skills check is case-sensitive (=== 'skills'), but the rest of the collision system uses the case-insensitive collisionKey(). A top-level supplementary path like 'Skills/review/doc.md' or 'SKILLS/review/doc.md' slips past this declaration-site check entirely. On a case-insensitive filesystem (macOS, Windows) those paths land in the same directory as the skill files, defeating the "skill companions belong in the owning skill's files array" invariant without any error. collisionKey is already in scope and costs nothing here.
| if (declared.split('/')[0] === 'skills') { | |
| if (collisionKey(declared.split('/')[0]) === 'skills') { |
2eada07 to
69b1df3
Compare
2dbd6c3 to
fef9bb7
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fef9bb7b03
ℹ️ 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 (data.files.length === 0) { | ||
| return ctx.mustBe('an asset entry with at least one materialized file record') | ||
| } | ||
| for (let i = 0; i < data.files.length; i++) { | ||
| const record = data.files[i] as (typeof data.files)[number] | ||
| const pathCheck = validateAssetName(record.path) | ||
| if (!pathCheck.ok) { | ||
| return ctx.mustBe(`file path "${record.path}" ${pathCheck.reason}`) | ||
| } |
There was a problem hiding this comment.
A 0.2 lockfile currently accepts any nonempty, path-safe files list for every asset. For example, an agent named reviewer can list README.md or skills/other/SKILL.md, and it can omit agents/reviewer.md; a skill can likewise claim another skill's companion. parseLockfileDocument will therefore treat a lockfile that cannot reproduce the declared asset state as valid, allowing archive-only or another asset's files to become owned by the wrong asset once consumers use these records. Require agents/commands to contain exactly their conventional primary path and skills to contain their own primary path plus only descendants of their own skill directory.
Useful? React with 👍 / 👎.
| ) | ||
| continue | ||
| } | ||
| if (declared.split('/')[0] === 'skills') { |
There was a problem hiding this comment.
On a case-insensitive supported filesystem, a declaration such as files: ["Skills/review/notes.md"] resolves beneath the skills/review directory but passes this exact-case check (and does not collide with skills/review/SKILL.md). It is consequently classified as archive-only instead of requiring the owning skill's files declaration, bypassing the ownership/declaration-site rule. Compare the first component with skills using the same portable case-folding policy used for archive collisions.
Useful? React with 👍 / 👎.
fef9bb7 to
6a969ca
Compare
69b1df3 to
673386f
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a969ca927
ℹ️ 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".
| * There is NO fallback between versions: a malformed `0.2` manifest fails | ||
| * as a `0.2` schema violation and is never reinterpreted as `0.1`. | ||
| */ | ||
| export function parseBuildManifestDocument(bytes: Uint8Array | string): ParseBuildManifestResult { |
There was a problem hiding this comment.
parseBuildManifestDocument is the only build-manifest path that invokes findDuplicateJsonMembers; parseFacetArchive, which feeds validateFacetArchive, still calls JSON.parse and the permissive BuildManifestSchema directly. Consequently, a submitted .facet with duplicate security-relevant members such as assets or integrity is accepted with JavaScript's last-member-wins interpretation, despite the new duplicate-member protection. Reuse this parser (or run the same scan) in parseFacetArchive before schema validation so archive verification enforces the promised rule.
Useful? React with 👍 / 👎.
673386f to
f5e650e
Compare
6a969ca to
a3ab253
Compare
f5e650e to
43cb4de
Compare
1ba7c8d to
b6ee73c
Compare
43cb4de to
a17d95c
Compare
Merge activity
|
… exact `facetVersion`/`lockfileVersion` dispatch and supplementary `files` support
b6ee73c to
90a0704
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 90a0704. Configure here.
| `file records sorted by path: "${record.path}" must sort after "${previous}" with no duplicates`, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Lockfile allows case-fold duplicates
Medium Severity
CurrentLockfileSchema only enforces strict lexicographic ordering and exact string inequality between adjacent files paths. Two records whose paths differ only by portable case (for example Skills/review/SKILL.md and skills/review/SKILL.md) can both pass validation.
Reviewed by Cursor Bugbot for commit 90a0704. Configure here.
1 participant
Why
Skills and commands share one logical namespace (design D9), meaning a facet cannot declare a skill and a command with the same name. Previously, the build pipeline permitted this cross-type name sharing. This change enforces the shared namespace at both the schema and build-pipeline levels, while keeping agents as a separate namespace that may freely share names with skills or commands.
This is part of the broader
support-non-asset-fileswork (tasks 1.1–3.3 are now complete), which introduces supplementary file declarations (filesarrays at the top level and per skill), a pure archive-plan derivation, versioned build-manifest and lockfile schemas with exact version dispatch, and duplicate JSON member rejection across all security-relevant documents.Details
Archive plan (
planArchiveEntries): A new pure operation derives the complete, deterministically sorted set of tagged archive entries from manifest data. It classifies entries asmanifest,primary-asset,skill-companion, orarchive-only, and enforces the full portable path grammar (traversal, absolute paths, backslashes, control bytes, empty segments, forbidden characters, Windows reserved device names, trailing dot/space) plus whole-set collision detection (exact duplicates, Unicode normalization aliases, case-fold aliases, file/directory prefix conflicts, and supplementary-vs-primary collisions). Build collection, hashing, verification, and installation all consume this one operation so membership sets cannot drift apart.Versioned schemas with exact dispatch:
LegacyBuildManifestSchema(pinned tofacetVersion: 0.1,assetsmap) andCurrentBuildManifestSchema(pinned tofacetVersion: 0.2,archive: "archive.tar.gz"literal,filesmap) replace the previous permissiveBuildManifestSchema.parseBuildManifestDocumentdispatches by exact equality — a malformed0.2document fails as0.2and is never reinterpreted as0.1. The same pattern applies to lockfiles:LegacyLockfileSchema(pinned tolockfileVersion: 1, identity-only asset entries) andCurrentLockfileSchema(pinned tolockfileVersion: 0.2, per-materialized-file integrity records with strict lexicographic ordering). Note that0.2 < 1numerically, so dispatch must use exact equality, not ordering.Legacy facet-manifest schema:
LegacyFacetManifestSchemafreezes the pre-0.2rules — multi-segment asset names, no shared skill/command namespace, no supplementaryfilesdeclarations — for use only when verifying legacy0.1archives. The currentFacetManifestSchemanow enforces single-segment asset names, the shared skill/command namespace, and delegates supplementary path validation toplanArchiveEntriesso a manifest that validates here always yields a valid archive plan.Duplicate JSON member rejection:
findDuplicateJsonMembersscans already-parsed JSON text for duplicate object member names (decoding escape sequences before comparison) and is applied before schema validation in all three document validators.JSON.parse's last-member-wins collapse would otherwise let two parsers see different declarations in one document.The outer-tar layout constants (
INNER_ARCHIVE_NAME,BUILD_MANIFEST_NAME) are moved tobuild-manifest.tsas the single source of truth and re-exported fromcontent-hash.tsto avoid an import cycle.The existing permissive
BuildManifestSchema,LockfileSchema, andLOCKFILE_VERSIONexports are retained but marked deprecated for removal after downstream consumers migrate to the versioned parsers.Verification
New focused test suites cover all legal and illegal states:
archive-plan.test.ts(path grammar, declaration-site rules, collision classes),build-manifest-versions.test.ts(schema acceptance/rejection, exact dispatch, duplicate member and invalid JSON failures),lockfile-versions.test.ts(legacy and current schemas, dispatch edge cases including the0.2 < 1numeric ordering trap), andduplicate-json-members.test.ts(scanner correctness and integration with facet-manifest validators). The build-pipeline test is corrected to assert that a skill/command name collision fails and adds a separate passing case for an agent sharing a name with a skill.Note
Medium Risk
Touches integrity and manifest validation contracts (shared skill/command namespace, legacy vs current dispatch, duplicate-JSON rejection); behavior changes are largely gated by tests but archive verification still uses legacy manifest rules until the consumer bridge lands.
Overview
Introduces the protocol layer for non-asset archive members and strict 0.1 / 0.2 wire formats, ahead of consumer-bridge and producer work still open in the change plan.
Manifest & membership: Current
FacetManifestSchemanow accepts top-level and per-skillfiles, validates asset names as single segments, and rejects skill/command name collisions (agents stay a separate namespace). A frozenLegacyFacetManifestSchemakeeps multi-segment names and permissive skill/command sharing for legacy0.1verification only.planArchiveEntriesis the single pure derivation of sorted, tagged inner-archive paths (manifest, primary assets, skill companions, archive-only) with portable path grammar and collision rules; the current manifest narrow delegates to it so validation and future build/verify paths stay aligned.Versioned artifacts: Split legacy vs current build manifests (
assets@0.1vsfiles@0.2with pinnedarchive: "archive.tar.gz") and lockfiles (alpha1vs0.2with sorted per-file integrity records).parseBuildManifestDocumentandparseLockfileDocumentdispatch on exact version equality (no cross-version reinterpretation).findDuplicateJsonMembersruns before schema validation on facet manifests, build manifests, and lockfiles.Exports & verification path: Public API expands with archive-plan types, version constants, parsers, and legacy validators.
validateFacetArchivenow validates embeddedfacet.jsonviavalidateLegacyFacetManifest(explicit legacy-only behavior for the existing verifier). Engine build-pipeline test updated: skill+command same name fails; agent+skill same name still passes.Reviewed by Cursor Bugbot for commit 90a0704. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit