Skip to content

fix(atomic): recheck move authorization before rename - #194

Merged
steipete merged 1 commit into
mainfrom
codex/move-publication-authority
Sep 1, 2026
Merged

fix(atomic): recheck move authorization before rename#194
steipete merged 1 commit into
mainfrom
codex/move-publication-authority

Conversation

@steipete

@steipete steipete commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What Problem This Solves

Fixes an issue where consumers moving files or directories under a revocable authorization could publish after that authorization expired during asynchronous preparation. Checking immediately before calling movePathWithCopyFallback did not protect the eventual rename, including the staged-copy path.

Why This Change Was Made

Add the optional assertBeforeRename callback, captured when the move starts and called synchronously after the directory guards, immediately before each rename dispatch. A refusal preserves the original error and cannot trigger copy fallback, even for EXDEV or EPERM. Promise, thenable, and other non-undefined returns refuse publication; asynchronous rejections are consumed.

Both direct moves and staged-copy publication use the same guarded rename boundary. Existing source-identity checks and cleanup remain in place. This does not cancel an already-dispatched syscall or make an external authorization store atomic with the filesystem. Production source delta is +22 lines; the new public contract and refusal handling require those additions.

User Impact

Consumers can now revalidate the original operation owner at the last synchronous point before publication. Omitting the callback preserves existing behavior. The options type and atomic-operation documentation describe the contract. This PR also prepares the root package, seven native packages, private build workspace, crate, and generated locks for release 0.7.2.

Evidence

  • All 20 new regression cases fail against the unchanged 0.7.1 source and pass with this fix. They cover direct and staged moves, genuine cross-device fallback, callback capture, refusal identity, same-turn dispatch, and asynchronous return rejection. The focused move/atomic cohort passes 79 tests, with one existing platform skip.

  • Live real-directory reproduction through the public package export: registry 0.7.1 publishes replacement bytes after cancellation and permits stale rollback to replace a successor directory. Built 0.7.2 refuses both before rename, preserves source/backup bytes and the successor inode, and still permits restoration by the active original owner. Package hashes remain unchanged during the run.

  • pnpm check: 4,082 tests pass; 2,333 platform/native-specific cases skip in the JavaScript-fallback run. Includes build, boundary lint, documentation examples, package imports, and public API checks.

  • pnpm test:security: 84 tests pass. cargo test --workspace --locked: 65 pass. cargo clippy --workspace --locked -- -D warnings, pnpm docs:site, and git diff --check pass.

  • pnpm native:build and pnpm package:smoke pass on macOS arm64: real root-only npm 12.0.2 and pnpm 11.24.0 installs exercise require/auto/off modes, omitted optionals, and missing-binary refusal. Foreign filtering fixtures prove selection only; platform execution is covered separately by CI.

  • Independent Codex review reports no actionable P0–P2 findings. The new test file adds 197 lines.

  • Tests added or updated when behavior changed

  • Security and compatibility impact considered

  • CHANGELOG.md updated when release-relevant

  • No credentials, private paths, private hosts, or sensitive contents included

@steipete
steipete requested a review from a team as a code owner September 1, 2026 19:29
@clawsweeper

clawsweeper Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

🦞👀
ClawSweeper picked this up.

Pull request received. I will update this pull request when review starts.

@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Sep 1, 2026
@clawsweeper

clawsweeper Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs real behavior proof before merge. Reviewed September 1, 2026, 3:35 PM ET / 19:35 UTC.

ClawSweeper review

What this changes

This PR adds an optional synchronous authorization recheck before direct and staged move renames, with documentation, regression tests, and synchronized 0.7.2 package metadata.

Merge readiness

Blocked until real behavior proof is added - 3 items remain

The implementation appears correct and keeps authorization rejection ahead of every rename publication route, but this external PR lacks an inspectable real-runtime artifact for that security boundary.

Priority: P2
Reviewed head: bf96e5d696342977ed2f55a3e6608ec64feb7f12

Review scores

Measure Result What it means
Overall readiness 🦪 silver shellfish (2/6) The patch is focused and well-covered by regression tests, but it cannot clear the external-PR merge gate without inspectable real behavior proof.
Proof confidence 🦪 silver shellfish (2/6) Needs real behavior proof before merge: Authority-chain proof required: the changed production owner is the guarded filesystem rename, and its direct and staged entrypoints are covered by mocked timing tests, but the supplied body provides only an unlinked claim rather than an inspectable after-fix real-directory trace showing revocation rejects before rename and leaves source/destination unchanged. Post redacted terminal output, logs, or a recording; updating the PR body should trigger a fresh review, or a maintainer can request one with @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Needs proof Needs real behavior proof before merge: Authority-chain proof required: the changed production owner is the guarded filesystem rename, and its direct and staged entrypoints are covered by mocked timing tests, but the supplied body provides only an unlinked claim rather than an inspectable after-fix real-directory trace showing revocation rejects before rename and leaves source/destination unchanged. Post redacted terminal output, logs, or a recording; updating the PR body should trigger a fresh review, or a maintainer can request one with @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed 6 items Final authorization boundary: The changed guarded rename calls the supplied assertion synchronously immediately before dispatching the real filesystem rename, after awaited directory guards complete.
Both publication routes use the boundary: The direct route and the staged-copy publication route both pass the captured callback to guarded rename; assertion failures are kept out of EXDEV/Windows fallback classification.
Focused route coverage: The added tests cover revocation during preparation, direct and forced-EXDEV paths, callback capture, synchronous dispatch ordering, and invalid asynchronous callback returns; they use real temporary directories but mock timing and rename outcomes.
Findings None None.
Security None None.

How this fits together

The atomic move helper prepares or copies a source path, then publishes it through a guarded filesystem rename. The new callback lets a caller revalidate a revocable owner at that final publication boundary.

flowchart LR
  A[Caller authorization] --> B[Atomic move helper]
  C[Source and destination paths] --> B
  B --> D[Directory identity guards]
  D --> E[Final authorization check]
  E --> F[Direct or staged rename]
  F --> G[Published move or refusal]
Loading

Before merge

  • Add real behavior proof - Needs real behavior proof before merge: Authority-chain proof required: the changed production owner is the guarded filesystem rename, and its direct and staged entrypoints are covered by mocked timing tests, but the supplied body provides only an unlinked claim rather than an inspectable after-fix real-directory trace showing revocation rejects before rename and leaves source/destination unchanged. Post redacted terminal output, logs, or a recording; updating the PR body should trigger a fresh review, or a maintainer can request one with @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
  • Resolve merge risk (P1) - The new callback protects a filesystem publication authority boundary, but no inspectable real-runtime artifact demonstrates a revoked caller being refused before direct and forced-EXDEV staged publication.
  • Complete next step (P2) - The remaining blocker is contributor-supplied real behavior proof rather than a mechanical repair task.
Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Implementation and regression delta production +25/-3, tests +197 The small production change is accompanied by focused route and callback-contract coverage.
Release surface 17 files affected; 9 package manifests versioned The public API change is coupled to synchronized package-release metadata.

Merge-risk options

Maintainer options:

  1. Add final-effect authorization proof (recommended)
    Post a redacted real-directory trace showing a revoked owner is rejected before direct and forced-EXDEV staged rename publication.
  2. Accept the proof gap
    Merge based on the focused mocked timing coverage while explicitly accepting that the final filesystem effect was not independently demonstrated.

Technical review

Best possible solution:

Retain the narrow callback design and add redacted final-effect evidence showing revocation prevents both direct and forced-EXDEV staged renames while preserving the source and prior destination.

Do we have a high-confidence way to reproduce the issue?

Yes, source provides a high-confidence path: revoke the callback while directory preparation is paused, then exercise direct and forced-EXDEV staged moves on real source and destination paths. The supplied test implements that scenario with mocked timing, but no inspectable live trace accompanies the PR.

Is this the best way to solve the issue?

Yes, centralizing the callback inside guarded rename covers both direct and staged publication without changing callers that omit it; real final-effect proof is still needed before merge.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 754f920d99af.

Labels

Label changes:

  • add P2: This is a bounded public filesystem-mutation hardening change with security-sensitive but non-emergency impact.
  • add merge-risk: 🚨 security-boundary: The new public callback decides whether the final filesystem publication rename may proceed.
  • add rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • add status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Authority-chain proof required: the changed production owner is the guarded filesystem rename, and its direct and staged entrypoints are covered by mocked timing tests, but the supplied body provides only an unlinked claim rather than an inspectable after-fix real-directory trace showing revocation rejects before rename and leaves source/destination unchanged. Post redacted terminal output, logs, or a recording; updating the PR body should trigger a fresh review, or a maintainer can request one with @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Label justifications:

  • P2: This is a bounded public filesystem-mutation hardening change with security-sensitive but non-emergency impact.
  • merge-risk: 🚨 security-boundary: The new public callback decides whether the final filesystem publication rename may proceed.
  • rating: 🦪 silver shellfish: Overall readiness is 🦪 silver shellfish; proof is 🦪 silver shellfish and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Authority-chain proof required: the changed production owner is the guarded filesystem rename, and its direct and staged entrypoints are covered by mocked timing tests, but the supplied body provides only an unlinked claim rather than an inspectable after-fix real-directory trace showing revocation rejects before rename and leaves source/destination unchanged. Post redacted terminal output, logs, or a recording; updating the PR body should trigger a fresh review, or a maintainer can request one with @clawsweeper re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Evidence

What I checked:

  • Final authorization boundary: The changed guarded rename calls the supplied assertion synchronously immediately before dispatching the real filesystem rename, after awaited directory guards complete. (src/guarded-mutation.ts:81, bf96e5d69634)
  • Both publication routes use the boundary: The direct route and the staged-copy publication route both pass the captured callback to guarded rename; assertion failures are kept out of EXDEV/Windows fallback classification. (src/move-path.ts:398, bf96e5d69634)
  • Focused route coverage: The added tests cover revocation during preparation, direct and forced-EXDEV paths, callback capture, synchronous dispatch ordering, and invalid asynchronous callback returns; they use real temporary directories but mock timing and rename outcomes. (test/move-path-authority.test.ts:51, bf96e5d69634)
  • Release metadata pattern: The release workflow validates that the tag, root package, native workspace, crate, seven platform packages, and changelog section all declare the same version; the proposed metadata is internally aligned. (.github/workflows/release.yml:35, bf96e5d69634)
  • Area history: The latest available main-side history record for the move helper is commit f748844 by steipete, supporting routing to the existing area contributor. (src/move-path.ts:370, f74884494505)
  • Submitted proof gap: The PR body describes a live real-directory run, but the reviewed snapshot contains no terminal output, recording, log, or linked artifact that lets reviewers inspect the after-fix denial before the actual rename.

Likely related people:

  • steipete: Suggested for follow-up; no historical authorship or introduction is verified. (role: unverified routing candidate; confidence: low)

Rank-up moves

Optional improvements that raise the rating; they are not merge blockers.

  • Add final-effect proof for the nearest unauthorized principal and prove revocation invalidates it before filesystem I/O.
  • Post a redacted real-directory trace for both direct rename and forced-EXDEV staged publication.

Rating scale

Score Internal tier Crab rank Meaning
6/6 S 🦀 challenger crab Exceptional readiness
5/6 A 🦞 diamond lobster Very strong readiness
4/6 B 🐚 platinum hermit Good normal PR; ordinary maintainer review
3/6 C 🦐 gold shrimp Useful, but confidence is limited
2/6 D 🦪 silver shellfish Proof or implementation needs work
1/6 F 🧂 unranked krab Not merge-ready
N/A NA 🌊 off-meta tidepool Rating does not apply

Overall follows the weaker of proof and patch quality.
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics.

Workflow

  • ClawSweeper keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@steipete
steipete merged commit e9ab355 into main Sep 1, 2026
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. P2 Normal priority bug or improvement with limited blast radius. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant