Skip to content

fix(cli): publish capture metadata without overwriting files - #3720

Merged
jrusso1020 merged 2 commits into
mainfrom
fix/security-capture-metadata-create
Sep 5, 2026
Merged

fix(cli): publish capture metadata without overwriting files#3720
jrusso1020 merged 2 commits into
mainfrom
fix/security-capture-metadata-create

Conversation

@jrusso1020

@jrusso1020 jrusso1020 commented Sep 5, 2026

Copy link
Copy Markdown
Collaborator

Capture scaffolding checks for meta.json before writing it, so another process can create the file in between and have its metadata truncated. A dangling metadata symlink can also cause its missing target to be created.

Write complete metadata in a private staging directory inside the output directory, then publish it with linkSync. An existing destination entry cannot be replaced or followed; only a publication EEXIST is treated as a harmless collision. Remove staging on success and failure. The initial existence check preserves skipping URL parsing for existing metadata. Metadata bytes/title fallback, agent instruction generation, and the absence of a scaffold index.html stay unchanged.

Addresses CodeQL #266. Staging and destination share a filesystem. Publication requires hard-link support and propagates unsupported-filesystem or permission errors.

Validation: all 209 capture tests pass, including eight new tests covering concurrent creation, dangling symlinks, failure cleanup, existing metadata and scaffold output. The original concurrent-file and dangling-symlink witnesses fail on main. CLI typecheck, parser/core/lint dependency builds, changed-file lint/format, complexity audit and signed hooks pass. The dangling-symlink witness caught a Windows failure in the prior wx-only implementation and remains enabled; revised Windows CI, CodeQL and Magi approval are required before merge.

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Reviewed at exact head f3ed845923a66df29771f3bb07e05afe6a0c1a1d. No blockers.

The write at packages/cli/src/capture/scaffolding.ts:71-83 has the correct ownership matrix:

  • metadata present at the initial check → skip URL parsing and preserve it;
  • metadata absent through open → wx creates exactly once;
  • a file or dangling symlink wins the race → native open returns EEXIST, which is the only swallowed error, and the winner remains untouched;
  • ENOENT/EACCES/other failures propagate before agent generation.

Keeping existsSync is therefore compatibility behavior rather than the safety mechanism; wx is the atomic boundary. On POSIX, O_EXCL|O_CREAT refuses the directory entry itself rather than following a dangling symlink; Node maps the same wx contract to exclusive creation on Windows.

The tests at scaffolding.test.ts:57-103 pin metadata bytes/title fallback, existing-file invalid-URL short-circuit, concurrent winner preservation plus continued agent generation, dangling-symlink target non-creation, non-EEXIST propagation, identical AGENTS/CLAUDE output, and no scaffold index.html.

Independent exact-head verification:

  • 207/207 capture tests passed
  • changed-file formatting passed
  • the repository’s Windows studio-engine-cli lane runs the complete @hyperframes/cli test suite, so these six cases are scheduled there rather than only in Linux CI

Fresh CI has no completed failure; JavaScript CodeQL, Windows CLI/render lanes, build/typecheck, and remaining jobs are still running and remain landing gates.

Verdict: APPROVE
Reasoning: Exclusive creation now owns the race and symlink boundary, while EEXIST-only handling preserves concurrent work and every pre-existing scaffold behavior is directly pinned.

— Magi

Comment thread packages/cli/src/capture/scaffolding.ts Fixed
@jrusso1020 jrusso1020 changed the title fix(cli): create capture metadata exclusively fix(cli): publish capture metadata without overwriting files Sep 5, 2026

@miguel-heygen miguel-heygen left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Re-review at exact head baa9fbc8b391c87ff9091416200814c1d81dfba7. The prior Windows/symlink and checked-path blockers are structurally closed; no new blockers.

The publication protocol at packages/cli/src/capture/scaffolding.ts:71-91 now has clear ownership:

  • mkdtempSync creates a private randomized directory inside outputDir, so staging and destination are necessarily on the same filesystem;
  • the complete metadata is written only to that private path;
  • linkSync(stagedPath, metaPath) atomically creates a second directory entry for the complete inode and neither replaces nor follows an existing destination file/symlink;
  • only link-time EEXIST is treated as another process owning publication; unsupported hard links, permissions, write failures, and all other errors propagate;
  • recursive removal in finally drops the staging link on every post-create exit, while a successful destination hard link remains.

The initial existsSync now controls only the legacy invalid-URL short-circuit; no write targets the checked path, closing #884’s TOCTOU shape. The unchanged race and dangling-symlink witnesses plus the new write/link failure tests at scaffolding.test.ts:79-125 pin destination preservation, target non-creation, continued scaffolding on a winning publisher, error propagation, and zero .hf-meta-* residue.

Independent exact-head verification: 209/209 capture tests and changed-file formatting passed. The fresh Windows studio-engine-cli lane is confirmed to run the whole CLI suite and was still executing; it remains the decisive landing gate after the prior platform-specific failure. Exact-head check runs had no completed failure when reviewed.

Non-blocking note: rmSync in finally can supersede an earlier write/link error—or make a successful publication report failure—if cleanup itself throws. That is fail-loud, retry-safe, and cannot corrupt/replace the destination, so it does not block this security fix; preserving the primary error would only improve diagnosis.

Verdict: APPROVE
Reasoning: Private same-volume staging plus atomic hard-link publication removes both the checked-path write and destination-follow/replace behavior, with cleanup and compatibility paths directly exercised.

— Magi

@jrusso1020
jrusso1020 merged commit e6d2816 into main Sep 5, 2026
64 of 84 checks passed
@jrusso1020
jrusso1020 deleted the fix/security-capture-metadata-create branch September 5, 2026 22:47
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.

3 participants