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 (14)
📝 WalkthroughWalkthroughThe build pipeline now supports supplementary archive files, validates their filesystem identity, preserves their bytes, and includes them in facet 0.2 archives and Sequence Diagram(s)sequenceDiagram
participant CLI
participant runBuildPipeline
participant loadSupplementarySources
participant FacetArchive
CLI->>runBuildPipeline: build facet source
runBuildPipeline->>loadSupplementarySources: load and validate supplementary files
loadSupplementarySources-->>runBuildPipeline: validated file bytes
runBuildPipeline->>FacetArchive: create facet 0.2 archive
FacetArchive-->>runBuildPipeline: archive and fileHashes
runBuildPipeline-->>CLI: build result
🚥 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: e62f6c56e9
ℹ️ 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".
| // parent symlink that stays inside the root; walking with `lstat` rejects | ||
| // it. Only existing components are checked — a missing parent surfaces as | ||
| // a `missing` target below. | ||
| const parentSymlink = await firstSymlinkedParent(rootReal, dirname(sourcePath)) |
There was a problem hiding this comment.
When rootDir has a different lexical spelling from its real path—such as an explicit symlink passed to facet build, or macOS /var resolving under /private/var—dirname(sourcePath) does not start with rootReal, so firstSymlinkedParent immediately returns null. A declared path such as linkdir/note.md, where linkdir is a symlink to another directory inside the facet root, is then accepted because lstat follows the parent link and the later containment check still passes. This bypasses the new no-symlinked-parent validation; walk from the lexical root or construct the checked path from rootReal before calling the helper.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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: 2
🧹 Nitpick comments (1)
packages/cli/src/__tests__/candidate-archive.e2e.test.ts (1)
144-146: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse descriptive archive-byte names.
Rename
aandbto identify the two deterministic-build artifacts. As per coding guidelines, “Use meaningful variable names.”Proposed refactor
-const a = await build('det-a') -const b = await build('det-b') -expect(Array.from(a)).toEqual(Array.from(b)) +const firstArchiveBytes = await build('det-a') +const secondArchiveBytes = await build('det-b') +expect(Array.from(firstArchiveBytes)).toEqual(Array.from(secondArchiveBytes))Source: Coding guidelines
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2afe0a86-7359-49da-8be0-29e6d36ba480
📒 Files selected for processing (14)
openspec/changes/support-non-asset-files/tasks.mdpackages/cli/src/__tests__/candidate-archive.e2e.test.tspackages/cli/src/__tests__/create-build.e2e.test.tspackages/cli/src/commands/build.tspackages/cli/src/tui/views/build/build-view.tsxpackages/engine/src/__tests__/build-pipeline.test.tspackages/engine/src/build/__tests__/load-supplementary-sources.test.tspackages/engine/src/build/load-supplementary-sources.tspackages/engine/src/build/pipeline.tspackages/engine/src/install/__tests__/run-add.test.tspackages/engine/src/install/__tests__/run-install.chain.test.tspackages/engine/src/install/__tests__/run-install.receipt.test.tspackages/engine/src/install/__tests__/run-install.test.tspackages/engine/src/install/__tests__/run-remove.test.ts
| // Complete inner-archive entry listing (includes facet.json + primaries). | ||
| expect(Array.isArray(doc.files)).toBe(true) | ||
| expect(doc.files).toContain('facet.json') |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert at least one primary archive entry.
A regressed files: ['facet.json'] payload still passes, despite this fixture containing the helper skill. Assert its primary path too.
Proposed test update
expect(Array.isArray(doc.files)).toBe(true)
-expect(doc.files).toContain('facet.json')
+expect(doc.files).toEqual(
+ expect.arrayContaining(['facet.json', 'skills/helper/SKILL.md']),
+)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Complete inner-archive entry listing (includes facet.json + primaries). | |
| expect(Array.isArray(doc.files)).toBe(true) | |
| expect(doc.files).toContain('facet.json') | |
| // Complete inner-archive entry listing (includes facet.json + primaries). | |
| expect(Array.isArray(doc.files)).toBe(true) | |
| expect(doc.files).toEqual( | |
| expect.arrayContaining(['facet.json', 'skills/helper/SKILL.md']), | |
| ) |
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| try { | ||
| stats = await lstat(sourcePath) | ||
| } catch { | ||
| failures.push({ code: 'missing', archivePath: entry.path, declarationSite, sourcePath }) | ||
| continue |
There was a problem hiding this comment.
When lstat fails with EACCES, ELOOP, or another filesystem error, this catch reports the declaration as missing. A present but inaccessible supplementary file therefore produces the wrong structured failure and tells the author to create a file that already exists; distinguish ENOENT from other errors and use the existing unreadable case for the latter.
Context Used: AGENTS.md (source)
30f2eb4 to
f43bc42
Compare
e62f6c5 to
fc27156
Compare
|
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc27156b1e
ℹ️ 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".
| archive: INNER_ARCHIVE_NAME, | ||
| integrity, | ||
| assets: assetHashes, | ||
| files: fileHashes, |
There was a problem hiding this comment.
A fresh install from a git source now builds a 0.2 manifest here, but resolve-git.ts still parses that JSON as the legacy BuildManifest and passes buildManifest.assets to cachePutVerified. Since this manifest contains only files, that value is undefined; cachePutVerified immediately calls Object.entries(archive.fileHashes), so every uncached git install crashes before caching or materialization. Use buildResult.fileHashes (or versioned manifest parsing) for that cache audit.
Useful? React with 👍 / 👎.
|
|
||
| const outer = parseTar(result.archiveBytes) | ||
| const inner = outer.find((e) => e.name === 'archive.tar.gz') | ||
| if (!inner?.data) throw new Error('inner archive missing') |
There was a problem hiding this comment.
This newly added test narrows an optional archive entry with if (...) throw; repository test guidance requires expect.unreachable() for these guards so the assertion intent is explicit and follows the project-wide convention.
AGENTS.md reference: AGENTS.md:L320-L325
Useful? React with 👍 / 👎.
Merge activity
|
…en archive assembly, and filesystem-identity validation
fc27156 to
64d85a2
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.
Reviewed by Cursor Bugbot for commit 64d85a2. Configure here.
| sourcePath: join(rootDir, entry.path), | ||
| }) | ||
| } | ||
| return { ok: false, failures } |
There was a problem hiding this comment.
Root realpath fails silently
Low Severity
If realpath(rootDir) throws and the archive plan has no supplementary entries, loadSupplementarySources returns { ok: false, failures: [] }. The pipeline maps that to a validation failure with zero errors, so asset-only builds can fail at “Loading files” with no actionable message.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 64d85a2. Configure here.
1 participant
Why
The producer was emitting
0.1build manifests with a partialassetsmap covering only primary assets. Supporting non-asset supplementary files (skill companions and archive-only files declared infiles) requires a complete entry map and a format bump to0.2.Details
Build manifest format (
0.2): Producers now always emitfacetVersion: 0.2with afilesmap covering every inner-archive entry —facet.json, all primary assets, and all supplementary files. Theassetskey is gone from producer output entirely; it remains a legacy consumer-input shape only. There is no runtime flag or dual-format mode.Supplementary source loading (
load-supplementary-sources.ts): A new filesystem-identity validation layer sits between the protocol's pure path-grammar checks and archive assembly. Before any output is touched, every declaredskill-companionandarchive-onlyentry is validated against a strict set of disk-identity rules: the file must exist, be a regular file, have no symlinked parent components, not be a hard link (nlink === 1), resolve within the facet root viarealpath, and not share an inode with another declared entry. All failures are collected in a single pass and surfaced as structuredValidationErrorvalues. If any fail, the pipeline returns early and priordist/output is preserved untouched.Archive plan as single source of truth:
collectArchiveEntriesFromPlanreplacescollectArchiveEntries, driving both archive assembly and thefileshash map from the sharedplanArchiveEntriesresult. This ensures the producer and verifier derive membership and ordering from the same derivation, making disagreement structurally impossible. The plan is pre-sorted lexicographically, so tar output is deterministic across machines.BuildResult.fileHashesreplacesassetHashes: The result type now exposesfileHashes(the complete per-entry map) andfacetVersion. CLI output, TUI build view, and--jsonschema version ("2") are updated accordingly — the--jsonschema bumps from"1"to"2"because theassetsarray is replaced byfiles, and consumers pinned to schema1must migrate.TOCTOU acknowledgment: The
lstat/realpath/readFilesequence has an irreducible race window. The threat model is malformed or accidental inputs over a trusted source tree, not an active attacker. The finalreadFileis performed against the validated resolved path so any swap produces a visible hash mismatch rather than a silently unvalidated entry.Verification
Tasks 10.1–10.4 and 11.1–11.7 are marked complete. A new
candidate-archive.e2e.test.tsbuilds a representative facet (text companion, binary companion, archive-only README) with the compiled CLI and validates the resulting.facetthroughvalidateFacetArchive— the same verifier a registry stage uses — confirming exact0.2membership and byte-identical output across two independent builds. Theload-supplementary-sources.test.tsunit tests cover the full filesystem-identity failure matrix (missing, directory, symlink target, symlinked parent, hard link, out-of-tree escape, resolved-source alias) plus happy-path byte fidelity. Pipeline-level tests inbuild-pipeline.test.tscover the complete supplementary success and failure cases, determinism, scoped output paths, and validation-before-cleanup behavior.Note
Medium Risk
Changes the authoritative build output format and breaks
--jsonschema 1 consumers; validation-before-write reduces risk of corrupt dist output.Overview
The build producer now always emits
facetVersion0.2 build manifests with a full inner-archivefileshash map (manifest, primaries, and declared supplementary paths). The legacyassetsmap is no longer written;BuildResultexposesfileHashesandfacetVersioninstead ofassetHashes.A new Loading files stage loads skill companions and archive-only sources via
loadSupplementarySources, with filesystem checks (regular files only, no symlink/hard-link escapes, distinct inodes) beforedist/is touched. Archive membership and ordering come fromplanArchiveEntriesthroughcollectArchiveEntriesFromPlan, replacingcollectArchiveEntries.CLI/TUI output and
--jsonschema2now reportfacetVersion, completefiles, and entry counts. Install test mocks readmanifest.filesfor registry stubs. Tests add supplementary pipeline coverage, loader failure matrix, andcandidate-archive.e2efor registry-grade0.2interop and deterministic bytes.Reviewed by Cursor Bugbot for commit 64d85a2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests