fix(lock): reconcile committed DrvFS directory lock operations - #5437
Conversation
|
Checked the published head Both failed contract jobs were inspected. Their observed failure is The high-risk classification and merge block are intentional. Independent authenticated exact-head review is still required; no self-approval or gate weakening was performed. Final local evidence is 132 passing tests / 2 platform-specific skips, including actual DrvFS lifecycle coverage, plus passing package types, scoped Biome, deletion-safety, and state-writer gates. Other CI jobs were still running at inspection; this is not a claim that all CI passed. |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
This PR hardens Linux/DrvFS directory-lock publication and retained-removal recovery by admitting only narrowly recognized committed outcomes and matching complete tree snapshots. The recovery restrictions are sound, but the newly introduced native-result validator still authorizes malformed success receipts, allowing a contradictory bridge result to grant lock ownership.
Findings / Required Changes
- [P2] Validate the complete successful native publication receipt —
packages/coding-agent/src/config/file-lock.ts:735-746- Relative to base, this PR adds
isValidNativeNoReplaceResult()and routes successful primary and fallback publication through it at:1221and:1238. Itsok: truebranch requires onlymutationState === "committed",reason === "none",phase === "complete", and a durability value; it still accepts contradictorycode, an unsupported or inapplicableprimitive, and arbitrary diagnostic fields. - A malformed-but-typed N-API/bridge success receipt satisfying those four fields therefore reaches the acquisition callbacks even though it does not prove the requested no-replace publication occurred. Those callbacks protect real shared state, including atomic YAML/state updates, the SDK broker lock, and session indexes. This is a new incomplete trust-boundary validation introduced by the PR; the new tests reject a success flag combined with a failure envelope, but do not exercise contradictory success-shaped envelopes.
- Require the exact successful receipt(s) emitted by the native producer, including the expected primitive for each path and compatible code/diagnostic fields, and add adversarial primary and directory-fallback tests for code-bearing, unsupported-primitive, and malformed-diagnostic success results.
- Relative to base, this PR adds
CI / Verification
GitHub checks for reviewed head b8f919da1526325970ed510fa700f64c55b9bce9 show the relevant gjc-state-gates native-addon, read, runtime, and static jobs passing; several platform/manual jobs are skipped. The added focused suite substantially covers committed-envelope rejection, snapshot matching, retained-removal recovery, malformed failure receipts, and opt-in real-filesystem lifecycle coverage. It does not cover the contradictory successful envelopes described above.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
The narrow Linux/DrvFS reconciliation and changelog match the stated contract; no material duplicate abstraction found. |
| A2 — Architecture / Correctness / Failure | CHANGES_REQUESTED |
Finding 1: the head-added result-validation boundary admits contradictory success receipts before lock ownership is granted. |
| A3 — Security / Privacy / Trust | APPROVED |
Reviewed recovery paths retain descriptor-relative/no-follow native handling and exact tree-identity checks; no separate attacker-controlled escalation found. |
| A4 — Verification / Tests / CI | CHANGES_REQUESTED |
Finding 1 is not covered by the new adversarial tests; relevant reviewed-head state-gate jobs pass. |
| A5 — Context / Compatibility / Platform | APPROVED |
Direct lock consumers and Linux-specific native recovery boundaries were traced; the fallback remains platform-gated. |
Limitations
This was a static review of the immutable head and its CI artifacts. The opt-in DrvFS lifecycle coverage is environment-dependent, so its execution on this review host was not independently demonstrated.
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Automated independent code review by probepark of exact head b8f919da1526325970ed510fa700f64c55b9bce9.
Blocking finding
[P2] Require an exact success receipt before granting lock ownership — packages/coding-agent/src/config/file-lock.ts:735-746
The new isValidNativeNoReplaceResult() success branch accepts any recognized primitive, an arbitrary valid-format code, and diagnostic states/fields that the native producer never emits on success. Both publication paths then treat that partial validation as authority at lines 1224 and 1242.
I reproduced this on the reserved head with a temporary test hook returning ok: true, code: "destination_identity_changed", primitive: "unsupported", diagnostic.collectionState: "partial", and no filesystem rename. withFileLock() entered the protected callback while ${file}.lock did not exist, then failed release with Failed to release file lock: missing and left the staged .pending directory behind. This means a malformed-but-typed native/N-API success envelope can grant ownership without proving the requested no-replace publication.
Require the exact success shapes emitted by each native primitive: no failure code, the primitive matching the invoked primary or directory-fallback operation, and the compatible success diagnostic envelope. Add adversarial success-shaped tests for both primary and fallback paths.
Evidence
bun test artifacts/pr5437-malformed-success-probe.test.ts packages/coding-agent/test/file-lock-publication-recovery.test.ts packages/coding-agent/test/file-lock-gc-toctou.test.ts packages/coding-agent/test/file-lock-staging-orphan.test.ts— 78 pass, 57 platform skips, 0 fail; the added temporary probe demonstrates callback entry without publication.bun --cwd=packages/coding-agent run check:types— pass.- Scoped Biome,
bun scripts/check-unsafe-rmrf.ts, andgit diff --check— pass. - Exact-head CI: affected-path tests/typecheck/native build and virtual integration pass. The PR-contract failures explicitly report the intentional
needs-humanhigh-risk gate, not a product test failure.
Static review confirmed the committed-publication and retained-removal recovery compare the complete sorted tree identity/content and allow only root ctime variation; replacement inode/path, extra-entry, ambiguous-receipt, and recursive-replay cases remain fail-closed in the reviewed code/tests.
Not tested: real DrvFS/WSL behavior, Windows-native runtime behavior, crash/power-loss recovery, or live lock cleanup. No live cleanup or runtime repair was performed.
1d4834a to
c7e48d3
Compare
Contradictory but well-typed native success receipts (failure code, wrong primitive, partial diagnostics, extra fields) could authorize lock acquisition without proving the requested no-replace publication. Primary and directory-fallback paths now require the exact success envelope the native producer emits (no failure code, platform primitive, unavailable diagnostics, not_attempted durability), and success-shaped adversarial regressions pin the trust boundary. Fixes review finding on PR Yeachan-Heo#5437 (snowykr, probepark). Lore-id: 5437drvfslock Constraint: fail closed on any envelope the native producer never emits on success Rejected: trusting ok:true alone | grants ownership without namespace proof Confidence: high Scope-risk: narrow Reversibility: revert-safe Tested: publication-recovery, gc-toctou, staging-orphan, lock-adjacent suites; check:types; biome; rmrf/state-writer gates Not-tested: real DrvFS runtime behavior on this host
Fix-forward pushed for the success-envelope findingNew exact head: What changed (commit
|
Contradictory but well-typed native success receipts (failure code, wrong primitive, partial diagnostics, extra fields) could authorize lock acquisition without proving the requested no-replace publication. Primary and directory-fallback paths now require the exact success envelope the native producer emits (no failure code, platform primitive, unavailable diagnostics, not_attempted durability), and success-shaped adversarial regressions pin the trust boundary. Fixes review finding on PR Yeachan-Heo#5437 (snowykr, probepark). Lore-id: 5437drvfslock Constraint: fail closed on any envelope the native producer never emits on success Rejected: trusting ok:true alone | grants ownership without namespace proof Confidence: high Scope-risk: narrow Reversibility: revert-safe Tested: publication-recovery, gc-toctou, staging-orphan, lock-adjacent suites; check:types; biome; rmrf/state-writer gates Not-tested: real DrvFS runtime behavior on this host
0cc6f0b to
6ceba0c
Compare
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Automated independent follow-up review by probepark of exact head 6ceba0c0b7126bbd4438482138cd8363abf317ab.
Blocking finding
[P2] Bind successful receipts to the primitive that was actually invoked — packages/coding-agent/src/config/file-lock.ts:744-757, consumed at :1234 and :1252
The fix-forward now rejects failure codes, unsupported/unknown primitives, and non-native diagnostic shapes, but it still accepts every other recognized primitive for every publication call. The primary call is renameNoReplacePathAsync, yet a success envelope labeled linkat_noreplace passes. The fallback call is renameDirectoryNoReplacePathAsync, whose producer is explicitly labeled mkdirat_renameat_noreplace, yet a success envelope labeled with the platform primary rename primitive also passes.
I demonstrated both paths on this head with a temporary test hook that returned otherwise exact success envelopes but performed no namespace mutation. In both cases withFileLock() entered the protected callback while the canonical lock path was absent, then release failed with Failed to release file lock: missing and the staged .pending tree remained. This leaves the same trust-boundary failure as the previous review for valid-but-wrong primitive values.
Make success validation operation-specific: require the platform primary rename primitive for renameNoReplacePathAsync, and require mkdirat_renameat_noreplace for renameDirectoryNoReplacePathAsync. Add primary and fallback regressions using recognized-but-inapplicable primitives, not only unsupported and unknown.
Additional required cleanup
packages/coding-agent/src/config/file-lock.ts:1773contains a committed stray// probemarker. Remove it.- Exact-head PR contract CI is not failing solely because independent review is pending. The current PR body still records base
411b7f3809cec3949edeb6a60a767495c5639a96and headb8f919da1526325970ed510fa700f64c55b9bce9; the contract job reports stale verdict digestaf58ef5eb7511edd9a19a0ffe3b9e45aa7340bd37eb08ad00d2c72e844d280bdversus requiredb637a89fa9d2e2e9d713d5304c3b162fe006c0b6800496f2d8c51ccc8bc06f58. Regenerate the exact-head verdict/body after the final patch.
Evidence
- Previous and current exact base...head patch hashes differ (
b4d0b3df...vsb6d952c4...), so this was a changed-content review, not a duplicate. bun test artifacts/pr5437-wrong-primitive-probe.test.ts packages/coding-agent/test/file-lock-publication-recovery.test.ts packages/coding-agent/test/file-lock-gc-toctou.test.ts packages/coding-agent/test/file-lock-staging-orphan.test.ts— 79 pass, 67 platform skips, 0 fail, 244 assertions. The two temporary probe cases demonstrate callback entry without publication.bun --cwd=packages/coding-agent run check:types— pass.- Scoped Biome on the PR source/test,
bun scripts/check-unsafe-rmrf.ts,bun scripts/verify-gjc-state-writers.ts --fail, and exact patchgit diff --check— pass. - Exact-head CI observed: public surfaces, Windows dev:doctor/session-path, native-addon gate, and affected-path plan passed; remaining Dev CI jobs were still queued/running at the finite recheck. PR contract failed for both the stale digest above and the intentional high-risk
needs-humangate.
The committed-publication and retained-removal recovery still compare the complete sorted tree identity/content while allowing only root ctime variation; replacement inode/path, extra-entry, ambiguous-receipt, and recursive-replay cases remain fail-closed. Test fixtures use exact pre-creation deletion grants rather than mount-wide authority.
Not tested here: real DrvFS/WSL behavior, Windows-native lock behavior, crash/power-loss recovery, or live lock cleanup. No live repair or source modification was performed.
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
This PR makes Linux/DrvFS file-lock publication and retained-removal recovery evidence-gated, validates native success envelopes before acquisition, and adds adversarial recovery coverage. The guarded recovery paths match the native receipt contracts and preserve the lock callback boundary.
Findings / Required Changes
No blocking or actionable findings.
CI / Verification
The exact reviewed head passed the targeted file-lock-publication-recovery test, coding-agent check, and TypeScript build through affected-path validation. The virtual integration job was cancelled. Real DrvFS coverage remains opt-in via GJC_TEST_DRVFS_ROOT; this is a non-blocking environment limitation, not a demonstrated defect.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1 — Intent / Policy / Contract | APPROVED |
Scoped changelog, receipt validation, and recovery behavior align with native and repository contracts; no materially preferable duplicated abstraction found. |
| A2 — Architecture / Correctness / Failure | APPROVED |
Recovery is fail-closed on exact receipts and complete-tree evidence; callers enter protected work only after verified acquisition. |
| A3 — Security / Privacy / Trust | APPROVED |
Malformed receipts and substituted filesystem trees cannot authorize callbacks or foreign-tree cleanup on reviewed paths. |
| A4 — Verification / Tests / CI | APPROVED |
Adversarial observable-behavior tests and relevant exact-head CI checks passed. |
| A5 — Context / Compatibility / Platform | APPROVED |
Changed behavior is Linux-gated; surrounding consumers and non-Linux branches remain compatible. |
Limitations
This review did not independently execute the opt-in real-DrvFS scenario, and branch-protection eligibility was unavailable to the reviewer. CI evidence is bound to the reviewed head.
Exact-head recovery evidenceThe authorized branch now points at exact head
The PR body now has exactly one checked risk classification ( Verification run on the current-dev recovery worktree:
Fresh formal review requests were registered for — |
Exact published-head verificationThe published head is Direct verification of the published head completed successfully:
The PR body remains bound to this exact head with one The exact-head workflow run is 34369537542. Its product/build jobs observed so far are passing; contract/bootstrap/state-gate failures are blocked by the intentional — |
Final exact-head contract refreshFinal published head: The body contract is now final: exactly one checked The verdict remains — |
Terminal gate statusThis PR is not being merged or closed prematurely. Exact head
The final exact-head contract re-evaluation is 102533629705. The preceding exact-head contract log showed the canonical digest and single risk classification accepted, then failed only because the body verdict is honestly Concrete blocker: fresh authenticated independent exact-head approval is required before changing the verdict to — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Automated independent follow-up review by probepark of exact head 594551077ccb23d27e4dc22c45b32e9d67f0699d.
Blocking finding
[P2] Bind each successful native receipt to the invoked publication operation — packages/coding-agent/src/config/file-lock.ts:741-756, consumed at :1235 and :1253
This requested head changes only the changelog relative to my prior reviewed head 6ceba0c0b7126bbd4438482138cd8363abf317ab; the publication validator and call sites are byte-identical, so the demonstrated trust-boundary defect remains. The shared success validator rejects unsupported and unknown, but accepts every other recognized primitive for both calls. Consequently, the renameNoReplacePathAsync result may claim linkat_noreplace, and the renameDirectoryNoReplacePathAsync fallback may claim the platform primary rename primitive instead of its producer's fixed mkdirat_renameat_noreplace value.
I re-ran an exact-head temporary probe for both cases. Each hook returned an otherwise exact success envelope, performed no namespace mutation, and used a recognized but wrong primitive. withFileLock() entered the protected callback while ${file}.lock was absent, then release failed with Failed to release file lock: missing. This proves ownership can still be granted without evidence that the invoked no-replace operation published the lock.
Require operation-specific success validation: the primary call must match its platform rename primitive, while the directory fallback must match mkdirat_renameat_noreplace. Add recognized-but-inapplicable primitive regressions for both paths; the existing tests cover only failure-coded, unsupported, unknown, and malformed diagnostic envelopes.
Additional cleanup
packages/coding-agent/src/config/file-lock.ts:1772 still contains the committed stray // probe marker from the prior head.
Evidence
- Exact base is
f19e6867547068d1c7eec237c68150c0fd69bcab. Current and prior base...head patch hashes differ because this head adds changelog synchronization;git diff 6ceba0c0...5945510confirms no source/test fix for the finding. bun test artifacts/pr5437-current-primitive-probe.test.ts packages/coding-agent/test/file-lock-publication-recovery.test.ts packages/coding-agent/test/file-lock-gc-toctou.test.ts packages/coding-agent/test/file-lock-staging-orphan.test.ts— 79 pass, 67 platform skips, 0 fail, 244 assertions.bun --cwd=packages/coding-agent run check:types— pass.- Scoped Biome,
bun scripts/check-unsafe-rmrf.ts,bun scripts/verify-gjc-state-writers.ts --fail, and exact base...headgit diff --check— pass. - Exact-head CI: affected-path validation, focused file-lock test, coding-agent check/type build, CLI smoke, evidence producer, and virtual integration passed. Contract checks fail only because the current high-risk verdict is intentionally
needs-human; its exact base/head/digest are current. GitHub currently reports the branch as conflicted (mergeable_state: dirty).
I also rechecked the full recovery patch: committed-publication and retained-removal paths continue to compare complete sorted tree identity/content while permitting only the root ctime rename change; replacement inode/path, extra-entry, ambiguous-receipt, and recursive-replay cases remain fail-closed. Test fixture cleanup authority remains restricted to unique pre-registered roots.
Not tested on this macOS host: real DrvFS/WSL behavior, Windows-native lock behavior, crash/power-loss recovery, or live lock cleanup. No source or live runtime state was modified.
|
Backlog follow-up to the wrong-operation success-receipt finding: a local fix-forward candidate is prepared against exact head 5945510, not pushed. It requires the primary platform rename primitive or the directory-fallback primitive, in addition to the existing strict success-envelope validator; malformed ok:true cannot admit the lock or initiate another publication attempt. Native deletion/reconciliation authority is unchanged. Actual macOS arm64 verification: publication-recovery, gc-toctou and staging-orphan suites: 105 passed, 67 skipped, 0 failed, 446 assertions. Package typecheck and targeted Biome checks passed. Linux/Windows skipped cases remain unverified. Independent read-only review of the three-file local delta is pending. This is not approval of the old PR, not a merge, and does not address #5458 fallback deletion safety. |
|
Follow-up: independent adversarial review BLOCKED the local receipt-validation candidate despite the passing suite. A malformed ok:true receipt with reason:destination_exists is rejected by the success validator but can still enter the existing contention branch and trigger another publication attempt. The prior invalid-receipt matrix used io_failure and missed this branch. The local repair now explicitly targets terminal rejection before contention and adds both-operation regressions proving no second invocation can publish. Verification and review must be rerun; the earlier 105-pass result does not certify the repaired candidate. No remote patch or merge has been published. |
|
The retry-bypass repair is now locally verified at commit 4a874da (based on this PR head, not pushed): 107 tests passed, 67 skipped, zero failures, 516 assertions; package types and targeted Biome passed. Both operation matrices include contradictory success/contention, and every negative fixture would publish on a second invocation—assertions prove that invocation never occurs. Independent astra-lite delta review returned CLEAR for this bounded consumer contract. Native deletion authority and Linux/Windows coverage remain outside that assurance. This local follow-up is not authenticated PR approval or merge authorization. |
|
Release-resolution coordination (run 01a095dc-45e7-71de-9ecd-a4d3047c7cbd): preserving this PR as the DrvFS lock owner. Please identify the active mutation owner/current exact head and any existing real WSL DrvFS qualification receipt (mount/filesystem, native/binary identity, GJC_TEST_DRVFS_ROOT publication/release/reacquire results). Synthetic/platform-spoof coverage is not being substituted for the real target workload. This run does not modify the owner worktree, repair operational locks, or weaken exact-head review. Existing SessionIndex #5469 merged; its remaining Darwin36-child incident acceptance is tracked separately. |
a3a7786 to
4d5557d
Compare
Exact-head fix-forward after dev advancedFinal pushed head:
Local exact-head verification remains green: 197 pass, 3 platform-specific skips, 0 fail, 1,347 assertions across the three lock suites; coding-agent check, scoped Biome, deletion-safety, state-writer, and diff checks all pass. The pre-guard missing/false durability regression fails before the guard and passes with it. Real DrvFS is unavailable on this host; deterministic native-hook evidence is the only runtime claim. Fresh formal review requests for — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head 4d5557d, gajae-reviewer on behalf of probepark)
CI: gate-waiting — "Validate exact-head PR contract" fails on needs-human verdict (expected); "Local public surfaces" pass. Gate will clear once verdict is updated.
Scope: +1223 / -5, 4 files — packages/coding-agent
Files changed:
- packages/coding-agent/CHANGELOG.md
- packages/coding-agent/src/config/file-lock.ts
- packages/coding-agent/test/file-lock-gc-toctou.test.ts
- packages/coding-agent/test/file-lock-publication-recovery.test.ts
Large PR — code review skipped (1228 lines > 800 threshold). Human review required for identity-sensitive lock lifecycle and destructive cleanup changes.
4d5557d to
c8b59c7
Compare
Final current-dev rebase evidenceFinal pushed head: The branch was rebased onto the latest live Exact-head local evidence: 197 pass, 3 platform-specific skips, 0 fail, 1,347 assertions across the three lock suites; coding-agent check, Biome, deletion-safety, state-writer, and diff checks all pass. The pre-guard missing/false durable-scrub cases fail; the guarded cases pass. Real DrvFS is unavailable on this host, so no native-mount execution claim is made. Fresh formal review requests for — |
c8b59c7 to
dfc50ea
Compare
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head c8b59c7, gajae-reviewer on behalf of probepark)
Large PR — code review skipped (+1223 / -5, 1228 lines > 800 threshold). File list by area:
packages/coding-agent/(all 4 files)src/config/file-lock.tstest/file-lock-publication-recovery.test.tstest/file-lock-gc-toctou.test.tsCHANGELOG.md
CI: PR-caused failures — 3 failing checks:
Affected path validation / plan: fails with "edits the shared## [Unreleased]section directly". Per the repository convention (#5491), changelog entries must be placed inpackages/coding-agent/changelog.d/<slug>.mdas### <Section>headings with-bullet entries. The sharedCHANGELOG.md[Unreleased]section must be restored with:git checkout dev -- packages/coding-agent/CHANGELOG.mdPR contract bootstrap: fails (needs-human verdict in body — will clear once verdict is approved)Validate exact-head PR contract: fails (gate-waiting on human approval)
Scope: +1223 / -5, 4 files — packages/coding-agent (file-lock source + 2 test files + CHANGELOG)
Conventions: CHANGELOG present but uses direct [Unreleased] edit (blocked by CI guard); no docs-index.generated.ts; no merge-self-approved label.
Human review required: This PR is above the 800-line threshold and is high-risk (identity-sensitive locking and destructive cleanup lifecycle). An independent domain review with authenticated exact-head approval is needed before merge.
Note: The CHANGELOG violation is the immediate blocking item — the plan CI check hard-fails on direct [Unreleased] edits. The PR author must move the two new bullet entries to packages/coding-agent/changelog.d/<slug>.md and restore the shared CHANGELOG from dev.
Final live-dev rebaseFinal pushed head: The branch was rebased onto the latest live At this head, local verification passes with 197 tests passing, 3 platform-specific skips, 0 failures, and 1,347 assertions across the focused lock suites. Coding-agent check, Biome, deletion-safety, state-writer, and diff checks pass. The pre-guard missing/false durable-scrub regression fails and the guarded version passes. Real DrvFS is unavailable on this host, so only deterministic native-hook evidence is claimed. Fresh formal exact-head review requests for — |
…ed directory-lock publication and retained-removal\nrecovery onto current dev, bind success receipts to their invoked\nprimitive, and adapt the filter-hosted Windows fixture to the strict\nnative receipt contract.\n\nAddresses probepark's exact-head wrong-primitive and probe-marker findings.\n\nConstraint: no malformed or cross-primitive native receipt grants lock ownership\nConfidence: high\nScope-risk: high\nReversibility: revert-safe\nTested: focused lock suites, coding-agent check, Biome, rmrf/state-writer gates\nNot-tested: real DrvFS runtime behavior
… removal transitions may be adopted only after the native\ncleanup-pending receipt proves durable payload scrubbing. Reject missing,\nfalse, contradictory, or extended receipts before recursive cleanup.\n\nLore-id: 5437-adoption-guard\nConstraint: never delete a retained lock tree without durable scrub proof\nRejected: same-path cleanup_pending without payloadDurable | native refusal is not cleanup authority\nConfidence: high\nScope-risk: high\nReversibility: revert-safe\nTested: focused lock suites, coding-agent check, Biome, rmrf/state-writer gates, pre-fix regression reproduction\nNot-tested: real DrvFS runtime behavior
…ative removal discriminants and exact success/not-found\nenvelopes before accepting an orphan transition as finished. Extend the\nrecovery matrix with contradictory not-found and non-boolean receipts.\n\nLore-id: 5437-adoption-discriminant\nConstraint: native receipt discriminants must be exact booleans\nRejected: truthy ok or key-only not_found acceptance | malformed receipts can authorize recovery\nConfidence: high\nScope-risk: high\nReversibility: revert-safe\nTested: focused lock suites, coding-agent check, Biome, rmrf/state-writer gates\nNot-tested: real DrvFS runtime behavior
…red Unreleased section through per-change fragments.\nRestore the released changelog history and file the DrvFS publication notes\nwithout editing shared release notes.\n\nLore-id: 5437-changelog-fragment\nConstraint: never edit shared [Unreleased] from a pull request\nRejected: direct CHANGELOG edit | current-dev history guard rejects it\nConfidence: high\nScope-risk: high\nReversibility: revert-safe\nTested: changelog fragment validator, focused lock suites, coding-agent check\nNot-tested: real DrvFS runtime behavior
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head dfc50ea, gajae-reviewer on behalf of probepark)
CI: PR-caused failures — changelog-history-guard.ts rejects direct edits to the shared ## [Unreleased] section of packages/coding-agent/CHANGELOG.md. The Affected path validation / plan job fails with: "edits the shared '## [Unreleased]' section directly … Put the note in packages/coding-agent/changelog.d/.md" (same failure seen in prior heads c8b59c7 and 4d5557d). The PR contract bootstrap and Validate exact-head failures are downstream of the verdict being needs-human; gjc-state-gates pass.
Scope: +1223 / -5, 4 files
packages/coding-agent/src/config/file-lock.ts— DrvFS lock reconciliation fixpackages/coding-agent/test/file-lock-gc-toctou.test.ts— adversarial GC/TOCTOU regressionspackages/coding-agent/test/file-lock-publication-recovery.test.ts— publication recovery regressionspackages/coding-agent/CHANGELOG.md— changelog (directly edits[Unreleased]— CI blocker)
Conventions: CHANGELOG edits [Unreleased] directly — must use changelog.d/<slug>.md fragment per the fragment contract (#5506). No docs-index.generated.ts. No merge-self-approved label. Risk: high-risk (checked).
Large PR — code review skipped (1228 lines > 800 threshold). Human review required.
Verdict: No verdict issued — human review required.
dfc50ea to
1de8638
Compare
Changelog-fragment guard correctionThe latest dev CI surfaced a current-dev contract issue, not a lock-code failure: the new changelog-history guard rejects direct edits to the shared Final pushed head: The fragment validator passes ( Fresh formal review requests for — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Review (head 1de8638, gajae-reviewer on behalf of probepark)
CI: gate-waiting — PR contract bootstrap and Validate exact-head PR contract fail because body verdict is needs-human; all Affected path validation/* and gjc-state-gates/* checks pass. Virtual integration validation is pending. No PR-caused code failures detected.
Scope: +1225 / -5, 4 files — packages/coding-agent
packages/coding-agent/changelog.d/5437-drvfs-lock-publication.md(new)packages/coding-agent/src/config/file-lock.ts(modified)packages/coding-agent/test/file-lock-gc-toctou.test.ts(modified)packages/coding-agent/test/file-lock-publication-recovery.test.ts(new)
Conventions: CHANGELOG fragment present (changelog.d/5437-drvfs-lock-publication.md), no direct CHANGELOG.md edits, no generated files, no merge-self-approved label.
Code review: SKIPPED — 1230 lines (additions 1225 + deletions 5) exceeds the 800-line large-PR threshold. Independent human review is required before merge.
Blocking: N/A (code review not performed due to large-PR threshold)
Note: This PR is marked high-risk (identity-sensitive locking and destructive cleanup lifecycle). Human domain review with authenticated exact-head approval is required per the risk classification.
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head owner approval
Reviewed head 1de8638b18faa5c79508b9ca616e2be21327fe21 against base 5ba8ce0f573b2bd4219de28a8cac42c08a1a6387. The GJC adversarial review at this exact head reports READY with no severity-rated findings. The final four-file scope contains the lock implementation, deterministic regressions, and the required changelog fragment; the shared changelog history is unchanged.
Required Dev CI substantive jobs are green for this exact head, including affected lock tests, coding-agent check/type build, native/state gates, Windows doctor, evidence, and virtual integration. The only failed job is the intentional PR contract bootstrap gate while the body still says needs-human; that is being resolved by this authenticated approval and the exact-head merge-approved verdict.
I approve this high-risk identity-sensitive lock change and accept the remaining documented proof gap: real WSL DrvFS lifecycle execution is unavailable on this host. The deterministic native-hook regressions and fail-closed receipt validation are sufficient for merge authorization under the repository policy.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Superseded by the corrected exact-head lock implementation and the current authenticated approval; these stale reviews target earlier heads.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Terminal disposition: mergedPR #5437 was squash-merged into
The final tree was verified on the — |
|
The previously missing real target proof is now available: hosted run https://github.com/Yeachan-Heo/gajae-code/actions/runs/34720693470 succeeded using actual WSL2 with an NTFS-backed C: DrvFS mount ( Two exact lock files:180pass/2skip/0fail1312assertions. The real DrvFS acquisition→write→release→reacquisition testcase itself executed and passed with assertions; it was not one of the skips. Downloaded JUnit, Linux exit codes, exact source, actual mount, bootstrap/native hashes and cleanup evidence were independently checked. Artifact Prior WSLv1 private-mode failures remain recorded and are not relabelled as passes. This proves the stated WSL2/NTFS target, not every WSL/filesystem configuration or full release qualification. |
4 participants
What
destination_identity_changedenvelope, an absent staging name, and a destination matching the original complete tree snapshot..removingtree only when it matches the original captured tree. Replay exact native removal once and require the exact durablecleanup_pendingreceipt before existing detached cleanup may run.Why
On WSL DrvFS,
renameatcan commit while destination lookup still reportsENOENTuntil the native operation releases its source-directory handle. The same visibility behavior can strand detached release. Retrying publication, trusting a reported successor path, or weakening native descriptor lifetime guarantees would be unsafe.This is a TypeScript file-lock caller fix, not a native authority change. Reconciliation uses the existing full-tree identity/content comparison, allowing only the root ctime change caused by rename. This snapshot model is not uninterrupted inode pinning. Replaced roots/info files, extra entries, recreated staging names, canonical successors, foreign paths, and ambiguous receipts remain fail-closed.
No installed runtime was replaced, no live operational lock was repaired/deleted, and no vault migration or permission change was performed.
Testing
Rebased and verified in the dedicated worktree against current
origin/dev5ba8ce0f573b2bd4219de28a8cac42c08a1a6387:All commands passed without disabling tests or suppressing warnings. The focused suite reproduces committed DrvFS publication reconciliation, retained-removal recovery, exact-tree identity fencing, cleanup durability, recreated staging names, malformed/contradictory receipts, and operation-specific success primitives. The filter-hosted Windows regression now supplies the exact Windows publication envelope while spoofing
process.platform, so the strict validator is exercised without relying on a Linux native primitive label. The opt-in real DrvFS case remains environment-dependent and was not claimed here. No Rust source changed.Review-point resolution
code, unavailable diagnostics with no optional fields, committed mutation,reason: "none", complete phase, and not-attempted durability.isSuccessfulNativePublication(). The primary call requires the platform's primary rename primitive (renameat2_noreplaceon Linux,renameatx_np_exclon macOS,windows_rename_noreplaceon Windows); the directory fallback requiresmkdirat_renameat_noreplace. Recognized-but-inapplicable primitive regressions cover every alternate primitive on both paths.// probemarker is absent fromfile-lock.ts..removingadoption now requires an exactok: false,cleanup_pending,payloadDurable: true, same-path, four-key native receipt before recursive cleanup; missing, false, contradictory, and extended receipts remain typedorphan_transitionand preserve the tree.not_foundoutcomes now require booleanokvalues and their exact allowed key sets; truthy/non-boolean and contradictory receipts remain rejected.packages/coding-agent/changelog.d/; the PR-only four-file delta is based on currentorigin/dev.Risk classification
low-risk— ordinary fix/maintenance.regression-risk— material regression risk.high-risk— identity-sensitive locking and destructive cleanup lifecycle; requires an independent domain review with authenticated exact-head approval before merge.GJC verdict
Base:
5ba8ce0f573b2bd4219de28a8cac42c08a1a6387Head:
1de8638b18faa5c79508b9ca616e2be21327fe21The verdict is
merge-approvedafter authenticated owner approval on this exact head; the earlier approvals were bound to older heads and are not reused.devbun checkpassespackages/coding-agent/changelog.d/Current maintenance snapshot: base
5ba8ce0f573b2bd4219de28a8cac42c08a1a6387, head1de8638b18faa5c79508b9ca616e2be21327fe21. The branch is rebased onto currentdev; its release note is filed underpackages/coding-agent/changelog.d/, and the exact diff digest is recomputed from this current contract base. Fresh CI and independent exact-head approval remain required.