Skip to content

docs(write): scope create-only publication visibility by backend - #225

Merged
steipete merged 3 commits into
mainfrom
fix/create-only-publication
Sep 4, 2026
Merged

docs(write): scope create-only publication visibility by backend#225
steipete merged 3 commits into
mainfrom
fix/create-only-publication

Conversation

@steipete

@steipete steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Live testing of main confirmed a real contract violation: in the pure-JavaScript fallback, Root.create(), createJson(), and write({ overwrite: false }) claim the final destination pathname with O_EXCL before content is written, so a concurrent observer can see an empty file at the target. The writing docs promised sibling-temp commit with no half-written replacement visible for these operations.

A staging fix was implemented and fully tested locally (private temp file published via hardlink link+unlink). It worked, but the hosted sidecar-contention proof caught a worse regression: between the link and the unlink, the published name transiently has two links, and concurrent readers/creators that enforce single-link identity fail with hardlink/path-alias errors instead of clean contention. POSIX plus Node's fs API offers no atomic operation that is simultaneously no-clobber, content-complete, and single-link: rename clobbers, O_EXCL open exposes the name early, and link+unlink exposes a two-link window. The native binding's renameNoReplace is the correct mechanism and already provides the guarantee.

So this PR resolves the defect at the contract boundary rather than by regressing concurrent behavior:

  • Docs: the sibling-temp no-visibility guarantee for create-only writes is scoped to backends with atomic no-replace publication (the native binding, default in auto/require). The pure-JavaScript fallback's claim-then-write behavior is documented explicitly, matching the package's existing best-effort fallback model.
  • Regression tests: new coverage pins both behaviors — the fallback claims the name before content (so a future change is deliberate), and the native backend keeps the destination absent until publication. Outcome semantics (final bytes, already-exists collision preservation, failure cleanup) are asserted for all four create-only operations.
  • Changelog: records the contract clarification.

Evidence

  • Defect reproduced on installed consumers of main with a separate-process observer: all four create-only operations exposed the destination at size 0 before content, in native-off mode; the same probe in require mode saw the destination absent until publication.
  • The intermediate hardlink implementation passed all local suites but failed hosted macOS Node 24 CI (sidecar-contention-proof root-async: hardlink/path-alias errors, 30/400 acquisitions before the watchdog). That failure is the reason the code change was withdrawn; it is described here rather than hidden.
  • After reverting to the documented-behavior resolution: full CI-bounded check, security suite, and both sidecar-contention modes pass locally; hosted CI must pass before merge.

Boundaries

No runtime code changes ship in this PR. No API changes, no new options. The fallback limitation is a Node/POSIX capability gap, not a new relaxation: native (the default) already satisfies the strong contract.

@steipete
steipete requested a review from a team as a code owner September 4, 2026 09:15
@clawsweeper clawsweeper Bot added P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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 4, 2026
@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Codex review: needs maintainer review before merge. Reviewed September 4, 2026, 6:41 AM ET / 10:41 UTC.

ClawSweeper review

What this changes

The PR scopes create-only write visibility guarantees by backend, documents the JavaScript fallback’s exclusive-create visibility window, and adds regression coverage for all create-only APIs.

Merge readiness

Ready for maintainer review

Keep open: this accurately corrects a current documentation overstatement and pins the distinct native and JavaScript fallback behaviors. The prior auto-mode qualification has been addressed, and no introduced correctness defect remains.

Priority: P2
Reviewed head: 15020913782c92271a79ac130983f3bcf8dc4e81

Review scores

Measure Result What it means
Overall readiness 🐚 platinum hermit (4/6) A focused, source-aligned documentation correction with credible real behavior evidence and targeted regression coverage.
Proof confidence 🦞 diamond lobster (5/6) Sufficient (live_output): The changed surface is documentation and regression coverage for Root create-only publication. The supplied separate-process live output observes the real installed behavior at the first content write for all four APIs: native-off exposes a zero-byte target, while native-require keeps it absent until completed publication; current source matches both paths.
Patch quality 🐚 platinum hermit (4/6) No actionable review findings were identified.

Verification

Check Result Evidence
Real behavior Verified Sufficient (live_output): The changed surface is documentation and regression coverage for Root create-only publication. The supplied separate-process live output observes the real installed behavior at the first content write for all four APIs: native-off exposes a zero-byte target, while native-require keeps it absent until completed publication; current source matches both paths.
Evidence reviewed 5 items Fallback behavior on current main: The guarded JavaScript fallback opens a create-only target with O_CREAT | O_EXCL and only then writes the supplied content, establishing the documented observer-visible empty-file interval.
Native publication behavior on current main: The native staged-file owner uses renameNoReplace for overwrite:false publication after private staging, matching the strong guarantee described by the revised documentation.
Revised public contract and coverage: The introduced documentation qualifies auto mode on successful binding load and directs callers needing the guarantee to require mode; the new tests cover the four create-only API forms in both backend modes.
Findings None None.
Security None None.

How this fits together

Root is fs-safe’s capability-scoped filesystem writer for untrusted paths. Create-only calls select either native private staging with no-replace publication or the guarded JavaScript fallback, which claims the final name before writing bytes.

flowchart LR
  A[Caller uses a Root handle] --> B[Create-only write]
  B --> C{Native binding loaded?}
  C -->|Yes| D[Private staging]
  D --> E[No-replace publication]
  C -->|No| F[Exclusive final-name claim]
  F --> G[Write content in place]
  E --> H[Documented visibility contract]
  G --> H
Loading

Before merge

None.

Agent review details

Security

None.

Review metrics

Metric Value Why it matters
Changed surface 0 production lines, docs +17/-3, tests +137 The PR changes the public explanation and regression coverage without changing the established write implementation.

Technical review

Best possible solution:

Land the backend-specific contract wording and its focused regression coverage so callers can select require mode when they need no early destination visibility.

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

Yes. The supplied separate-process observation covers all four create-only operations, and current-main source independently shows the fallback creates the final name with O_EXCL before writing content.

Is this the best way to solve the issue?

Yes. Clarifying the backend boundary is safer than a link-and-unlink fallback implementation because the existing hardlink-rejection invariant remains intact while require mode provides the strong guarantee.

AGENTS.md: found and applied where relevant.

Codex review notes: model internal, reasoning high; reviewed against 15d313de56d3.

Labels

Label changes:

  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The changed surface is documentation and regression coverage for Root create-only publication. The supplied separate-process live output observes the real installed behavior at the first content write for all four APIs: native-off exposes a zero-byte target, while native-require keeps it absent until completed publication; current source matches both paths.
  • remove rating: 🦐 gold shrimp: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: ⏳ waiting on author: Current PR status label is status: 👀 ready for maintainer look.
  • remove merge-risk: 🚨 security-boundary: Current PR review selected no merge-risk labels.

Label justifications:

  • P2: This is a bounded public-contract clarification for a security-sensitive filesystem write behavior, with no runtime regression introduced by the patch.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🦞 diamond lobster and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (live_output): The changed surface is documentation and regression coverage for Root create-only publication. The supplied separate-process live output observes the real installed behavior at the first content write for all four APIs: native-off exposes a zero-byte target, while native-require keeps it absent until completed publication; current source matches both paths.
  • proof: sufficient: Contributor real behavior proof is sufficient. The changed surface is documentation and regression coverage for Root create-only publication. The supplied separate-process live output observes the real installed behavior at the first content write for all four APIs: native-off exposes a zero-byte target, while native-require keeps it absent until completed publication; current source matches both paths.

Evidence

What I checked:

  • Fallback behavior on current main: The guarded JavaScript fallback opens a create-only target with O_CREAT | O_EXCL and only then writes the supplied content, establishing the documented observer-visible empty-file interval. (src/pinned-write.ts:190, 15d313de56d3)
  • Native publication behavior on current main: The native staged-file owner uses renameNoReplace for overwrite:false publication after private staging, matching the strong guarantee described by the revised documentation. (src/native-staged-file.ts:234, 15d313de56d3)
  • Revised public contract and coverage: The introduced documentation qualifies auto mode on successful binding load and directs callers needing the guarantee to require mode; the new tests cover the four create-only API forms in both backend modes. (docs/writing.md:6, 15020913782c)
  • Real behavior evidence: The supplied PR comment records a separate-process, first-write observation for all four APIs: native-off exposed an empty target and native-require kept it absent until publication. (15020913782c)
  • Feature-history routing: Main-branch history shows sustained work on the writing and native-publication surfaces, including retained-directory staging and recent pinned-write safety fixes. (d73327569edf)

Likely related people:

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

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.

History

Review history (3 earlier review cycles)
  • reviewed 2026-09-04T09:28:57.807Z sha c94e9ad :: needs real behavior proof before merge. :: [P1] Do not delete a target after publication commits | [P2] Document the public create-only publication change
  • reviewed 2026-09-04T10:00:12.350Z sha 84ead44 :: needs real behavior proof before merge. :: [P2] Scope staging to the native backend
  • reviewed 2026-09-04T10:22:35.989Z sha b967e9d :: blocked before merge. :: [P2] Qualify the default auto-mode publication claim

@steipete
steipete force-pushed the fix/create-only-publication branch from c94e9ad to 06c6167 Compare September 4, 2026 09:50
@steipete steipete changed the title fix(write): stage create-only publication through private temp files docs(write): scope create-only publication visibility by backend Sep 4, 2026
The pure-JavaScript fallback has no atomic no-clobber rename: O_EXCL open exposes the final name early, and link+unlink exposes a two-link window that breaks concurrent readers enforcing single-link identity (proven by the sidecar-contention proof). Scope the no-visibility guarantee to backends with atomic no-replace publication, pin both behaviors with regression tests, and document the fallback claim-then-write behavior explicitly.
@steipete
steipete force-pushed the fix/create-only-publication branch from 06c6167 to 84ead44 Compare September 4, 2026 09:53
@clawsweeper clawsweeper Bot added P2 Normal priority bug or improvement with limited blast radius. and removed P1 Urgent regression or broken agent/channel workflow affecting real users now. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. labels Sep 4, 2026
@steipete

steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Addressing the review on head b967e9d (docs precision fix pushed).

1. Fallback staging claim corrected. The docs no longer say create-only writes "share that staging." They now state explicitly that sibling-temp staging with atomic no-replace publication exists only on backends that provide it, and that the pure-JavaScript fallback does not stage: it claims the final name with O_EXCL and writes content in place. The fs.create section carries the same distinction.

2. Real-behavior evidence. Since this PR changes no runtime code, the relevant proof is the documented current behavior, executed on installed packages from actual main (027f643, the runtime-ancestor of this branch; the two later main commits touch CI fixtures and dev dependencies only). A separate Node process observed the real destination while each create-only call was paused immediately before its first content write — no mocked filesystem state:

{
  "probe": "separate-process observer at first content write",
  "nativeOff": [
    {
      "operation": "create",
      "beforeWriteExists": true,
      "beforeWriteBytes": 0,
      "finalContentVerified": true
    },
    {
      "operation": "write-no-replace",
      "beforeWriteExists": true,
      "beforeWriteBytes": 0,
      "finalContentVerified": true
    },
    {
      "operation": "createJson",
      "beforeWriteExists": true,
      "beforeWriteBytes": 0,
      "finalContentVerified": true
    },
    {
      "operation": "writeJson-no-replace",
      "beforeWriteExists": true,
      "beforeWriteBytes": 0,
      "finalContentVerified": true
    }
  ],
  "nativeRequire": [
    {
      "operation": "create",
      "beforeWriteExists": false,
      "finalContentVerified": true
    },
    {
      "operation": "write-no-replace",
      "beforeWriteExists": false,
      "finalContentVerified": true
    },
    {
      "operation": "createJson",
      "beforeWriteExists": false,
      "finalContentVerified": true
    },
    {
      "operation": "writeJson-no-replace",
      "beforeWriteExists": false,
      "finalContentVerified": true
    }
  ]
}

In native-off mode the destination existed at size 0 before content (the documented limitation now stated in the docs). In require mode the destination was absent until publication (the guarantee). The regression tests added in this PR execute the same assertions in the repo: fallback name-claim visibility is pinned at test/root-create-publication.test.ts, and the native no-visibility guarantee is asserted through the real bundled binding on all CI platforms (skipped only where no binding exists).

The withdrawn hardlink implementation's failure is preserved in the PR description: it passed all local suites but regressed hosted sidecar contention through its transient two-link publication window, which is why the code change was abandoned rather than weakened further.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. and removed status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. labels Sep 4, 2026
@steipete

steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Addressing the re-review finding on head 1502091 (pushed).

Default auto mode claim qualified. The docs previously said the no-visibility guarantee comes with "the native binding used by the default auto and require modes." Corrected: auto only uses the native binding when it loads successfully, so the guarantee is now stated as require mode, or auto mode with a successfully loaded binding, and callers who need the guarantee are directed to require mode. The fs.create section uses the same qualification.

No code changes; the observer receipts in the previous comment remain the behavior evidence.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event exact_review_queue).
Result: when the review finishes, ClawSweeper will create the durable review comment if needed or update the existing comment in place.

Re-review progress:

@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦐 gold shrimp Decent PR readiness signal, but merge confidence is limited. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. merge-risk: 🚨 security-boundary 🚨 Merging this PR could weaken sandboxing, authorization, credentials, or sensitive data. labels Sep 4, 2026
@steipete
steipete merged commit e875fdc into main Sep 4, 2026
29 checks passed
@steipete

steipete commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Merged as e875fdc and verified on clean main (all 20 new contract tests pass there).

How this resolved. Live testing with a separate-process observer confirmed the original contract violation on main: in native-off mode, create, createJson, and both overwrite: false forms exposed the final pathname at size 0 before content was written. A staging fix (private temp + hardlink link+unlink publication) was implemented and passed every local suite, but hosted CI caught the catch: the link→unlink sequence leaves a transient two-link window, and the macOS sidecar-contention proof failed with hardlink/path-alias errors from concurrent readers enforcing single-link identity. POSIX + Node's fs API offers no operation that is simultaneously no-clobber, content-complete, and single-link, so the writer change was withdrawn rather than weakening the hardlink-rejection contract.

The landed change resolves the defect at the contract boundary: the no-visibility guarantee is scoped to backends with atomic no-replace publication (require mode, or auto with a successfully loaded binding), the fallback's claim-then-write behavior is documented explicitly, and regression tests pin both behaviors so any future backend change is deliberate. Final review confirmed all three earlier blockers resolved and endorsed the docs-only approach as safer than the withdrawn implementation.

Verification. Full CI and coverage green on the reviewed head 1502091 (including Windows, macOS, Linux, musl, and the sidecar-contention proofs in off and require modes); security suite 84/84 locally; the flaky dispatch-fixture timeout and the cancelled coverage job were rerun to green. One pre-existing Windows stream-limit cleanup assertion flaked in a pre-rebase run of unchanged code and passed on rerun.

No release or publication was performed.

@steipete
steipete deleted the fix/create-only-publication branch September 4, 2026 10:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant