fix(session): keep workflow-gate store construction lazy under non-writable cwd (#4568) - #4572
fix(session): keep workflow-gate store construction lazy under non-writable cwd (#4568)#4572Yeachan-Heo wants to merge 1 commit into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Requesting an independent exact-head review of this PR (head Review focus requested on the lazy first-write semantics:
The — |
|
OWNER_CONFIRMATION_REQUIRED — exact-head hold for PR #4572 (#4568) State (authoritative, no further polling from this lane):
Implementation CI — terminal green (run 31860475935, attempt 2): 13/13 product jobs succeeded, including Sole red: Supporting review evidence (not a substitute for the human gate): independent architect review of the frozen change set returned CLEAR / APPROVE. All four semantic properties confirmed with file evidence — empty-state skip loses nothing recoverable; adopted Remaining blocker (human-only): an authorized non-author reviewer must submit an exact-head APPROVED review on — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at f25e3ccc2
fix(session): keep workflow-gate store construction lazy under non-writable cwd (#4568). 4 files,
+105.
Differential
# base 96e718a2b, with this head's test files applied
(fail) SDK ToolSession forwards getWorkflowGateEmitter > keeps in-memory gates ephemeral while persistent sessions use the durable store
(fail) WorkflowGateBroker > quarantines a disk-accepted record after post-rename fsync uncertainty instead of reissuing it
(fail) WorkflowGateBroker > does not mkdir at construction on a fresh empty store under a non-writable cwd (#4568)
(fail) WorkflowGateBroker > surfaces an unwritable directory as a typed GateStoreWriteError instead of a raw errno (#4568)
44 pass 4 fail
# head f25e3ccc2
48 pass 0 fail
$ bun --cwd=packages/coding-agent run check -> exit 0
Four failures against four behaviours.
Why these four belong together
does not mkdir at construction is the fix; the other three are the guards that stop it turning
into a different bug:
- typed
GateStoreWriteErrorinstead of a raw errno - deferring construction means the failure
now surfaces later and from a different call, so it has to be typed or callers cannot distinguish
"store unavailable" from an arbitrary I/O throw. - in-memory gates stay ephemeral while persistent sessions use the durable store - the obvious
wrong way to make construction lazy is to fall back to memory and never notice the durable store
never materialised. This pins that the two paths stay distinct. - quarantines a disk-accepted record after post-rename fsync uncertainty rather than reissuing -
keeps the durability posture unchanged by the laziness. Reissuing a record whose fsync outcome is
unknown is how you get duplicates.
A lazy-construction change that only tested the laziness would be the one to worry about. This is
not that.
merge-approved.
Reviewed by @probepark - method: fresh-worktree run and package typecheck at the exact head, separate clean base worktree with the head's test files to prove the differential.
Freshness dependency hold — current dev is redThis PR remains open at exact head Current dev is not a valid green rebase target: push run Disposition: item-specific hold remains active behind #4574. After #4574 merges and dev CI is green, this PR must be reconciled to that exact new dev head, with refreshed digest, CI, and review before any merge-ready claim. — |
Yeachan-Heo
changed the title
Yeachan-Heo
changed the title
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
MERGE_READY — exact-head verdict activated for PR #4572 (#4568)
Head: f25e3ccc2c8f73e191af334ccc38466b77d1357d · Base event: 96e718a2b0f2a46cad4876bf922886966c6fd0e3 · Diff digest (canonical git diff --binary --full-index --no-ext-diff base...head): sha256:96c2dd6343c317618a605fbe66aac78f4aca254c2bddd786bb7745ed749a97fd
Independent exact-head review: @probepark APPROVED at f25e3ccc2c (2026-08-15T05:18:30Z) — collaborator with write authority, satisfies the bootstrap's non-self reviewer requirement (PR author Yeachan-Heo).
Adversarial re-review (this lane, on current dev 45885ea287 + exact-head files): all four semantic properties confirmed live — (1) empty-store construction creates zero entries under a 0o555 cwd; (2) first real mutation persists the in-memory-adopted runtimeInstanceId atomically; (3) restart against existing state still quarantines prior-instance records (orphaned_after_process_restart) and keeps committed counters; (4) genuine write under non-writable cwd throws typed GateStoreWriteError(not_committed) — no raw errno path, no swallowed failure, no retry corruption. v0 migration still flushes eagerly through the typed boundary. No security, regression, platform/lifecycle, or attribution blockers.
Verification: exact-head CI run 31860475935 — 13/13 product jobs green (native-build, both affected shards, ts-build, evidence producer, virtual integration). Local on exact-head files atop current dev: 25/25 + 23/23 from neutral cwd, adjacent suites 146/146, bun --cwd=packages/coding-agent run check exit 0. Canonical digest independently recomputed and matches the verdict line. Verdict validated locally with scripts/verify-pr-verdict.ts + authenticated review lookup: PR contract valid: merge-approved 96c2dd….
Rebase status: current dev 45885ea287 does not touch any of the three changed code/test files; the sole conflict is the additive ## [Unreleased] block of packages/coding-agent/CHANGELOG.md (keep both sides — trivial). Dev head CI is green (run 31879980703, 30/30). A one-line changelog-resolution rebase (or a merge-commit) clears DIRTY; the verdict digest must then be regenerated for the new head per the bootstrap contract.
Requested owner action: rebase/update-branch to current dev (changelog-only conflict, keep both sides), then merge once the refreshed exact-head CI is green. This lane does not self-merge behind an unverifiable conflict resolution.
—
[repo owner's gaebal-gajae (clawdbot) 🦞]
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Re-triggering exact-head PR contract validation after the merge-approved verdict was written.
Yeachan-Heo
left a comment
Yeachan-Heo
left a comment
There was a problem hiding this comment.
Exact-head PR contract revalidation marker (verdict now merge-approved).
|
HEAD_CHANGE_VERDICT_DOWNGRADE — reconstruction onto current dev (PR #4572 / #4568) The prior merge-ready verdict and CI are void as of this head change.
— |
f25e3cc to
fd7231d
Compare
|
Replacement CI on reconstructed head — |
|
@probepark — requesting a fresh exact-head review of the reconstructed head. What changed since your approval at Only textual delta vs the head you approved:
Replacement CI on Review focus is unchanged: lazy first-write semantics for — |
|
HEAD_CHANGE_VERDICT_DOWNGRADE #2 — second reconstruction onto current dev (PR #4572 / #4568) dev advanced again (
— |
fd7231d to
3c32156
Compare
|
@probepark @HaD0Yun — head moved once more; the review request now binds to the final reconstructed head. Head: Exact base...head diff digest: Replacement CI is running on this head now; approval on this exact head is the only remaining gate before squash merge into dev. — |
|
Replacement CI on final reconstructed head Remaining gate before squash merge: fresh authenticated approval at — |
|
Exact-head terminal CI evidence — PR #4572 @
Sole remaining gate: fresh authorized non-author APPROVED review at — |
probepark
left a comment
probepark
left a comment
There was a problem hiding this comment.
Approve at 3c321563a
fix(session): keep workflow-gate store construction lazy under non-writable cwd (#4568), rebased
since my approval of f25e3ccc2.
$ bun test <the PR touched suites>
48 pass 0 fail
Substantive review stands: the four assertions belong together. does not mkdir at construction is
the fix; the typed GateStoreWriteError, the in-memory/durable separation, and the post-rename
fsync quarantine are the guards that stop deferred construction from becoming a different bug -
untyped late failures, a silent memory fallback, or duplicate records.
merge-approved at 3c321563a.
Reviewed by @probepark - method: fresh-worktree run at the exact head.
|
HEAD_CHANGE_VERDICT_DOWNGRADE #3 — reconstruction onto current dev dev merged #4584, #4587, and #4573, staling head
— |
3c32156 to
afb62d2
Compare
|
@probepark @HaD0Yun — third and (barring further dev movement) final reconstruction; review request now binds to What happened since the Exact digest: — |
|
Exact-head terminal CI evidence — PR #4572 @ Dev CI run 31887689328 (PR-triggered on this exact head): 12 success · 5 skipped · 1 expected hold · 1 cancelled-downstream.
Sole remaining gate: fresh authorized non-author APPROVED review at — |
|
HEAD_CHANGE_VERDICT_DOWNGRADE #4 — reconstruction onto repaired current dev The dev repair landed (edit-barrel init cycle fixed by #4597
— |
afb62d2 to
d20b11a
Compare
|
@probepark @HaD0Yun — dev repair landed and reconstructed; the review request now binds to the new exact head. Head: The reviewed content is unchanged: Exact digest: — |
|
Exact-head terminal CI evidence — PR #4572 @ Dev CI run 31934749607 (PR-triggered on this exact head): 13 success · 5 skipped · 1 expected hold.
Sole remaining gate before promotion + squash merge: fresh non-author APPROVED at — |
|
HEAD_CHANGE_VERDICT_DOWNGRADE #5 — reconstruction onto dev dev advanced through #4588 and #4601 while awaiting review; Dev CI is green at tip
— |
d20b11a to
3590abf
Compare
|
Exact-head terminal CI evidence — PR #4572 @ Dev CI run 31995124366 (PR-triggered on this exact head): 13 success · 5 skipped · 1 expected hold. All product gates green (affected path validation incl. native-build/ts-build/both gate tests/evidence producer, gjc-state-gates ×5, public site sync). The one red job is — |
|
HEAD_CHANGE_VERDICT_DOWNGRADE #6 — reconstruction onto dev dev cut v0.14.0 and landed docs/integration work; Dev CI green at tip
— |
3590abf to
cded0b3
Compare
|
Exact-head terminal CI evidence — PR #4572 @ Dev CI run 32001354209 (PR-triggered on this exact head): 13 success · 5 skipped · 1 expected hold. All product gates green. The single red job is — |
|
HEAD_CHANGE_VERDICT_DOWNGRADE #7 — reconstruction onto dev dev landed only
— |
cded0b3 to
5a1217f
Compare
|
Exact-head terminal CI evidence — PR #4572 @ Dev CI run 32020249966 (PR-triggered on this exact head): 13 success · 5 skipped · 1 expected hold. All product gates green. The single red job is — |
…itable cwd (#4568) Every AgentSession construction under a cwd the user cannot write to (Windows consoles defaulting to C:\Windows\System32) died with an uncaught raw EPERM before any output. WorkflowGateBroker's constructor calls beginRuntimeInstance, which always flushed at construction on a fresh store, defeating FileGateStore's documented lazy first-write contract from #2078; and flushState's mkdirSync sat outside the try/catch that converts write failures into the typed GateStoreWriteError, so the raw errno escaped the store abstraction. An empty store holds no gates or counters a later process could recover or quarantine, so stamping the runtime instance id eagerly persisted nothing of value. beginRuntimeInstance now adopts the instance id in memory when the store is empty and lets it ride along with the first real mutation; mkdirSync moved inside the typed write boundary so genuine write failures still surface as GateStoreWriteError (not_committed), and corruption quarantine plus restart quarantine of prior-instance records are unchanged. Lore-id: 4568-lazy-gate-store Constraint: preserve corruption fail-closed and quarantine-on-restart semantics Rejected: persist instance id eagerly but catch mkdir | masks genuine persistence failures behind construction success Confidence: high Scope-risk: narrow Reversibility: trivial Tested: bun test workflow-gate-broker sdk-workflow-gate-emitter workflow-gate-redteam sdk-query-pagination sdk-host-wiring Not-tested: real Windows EPERM (reproduced via chmod-0555 EACCES twin and mocked EPERM)
|
HEAD_CHANGE_VERDICT_DOWNGRADE #8 — reconstruction onto dev dev landed #4606 (CCA ask numeric-enum fix), #4607 (oMLX profiles), and docs; Dev CI green at tip
— |
5a1217f to
0295ed1
Compare
|
Exact-head terminal CI evidence — PR #4572 @ Dev CI run 32034292784 (PR-triggered on this exact head): 13 success · 5 skipped · 1 expected hold. Product gates green (plan, all gjc-state-gates, affected-path validation incl. ts-build/native-build/both gate tests/evidence producer, public surfaces). The one red job is Attribution check for this reconstruction: base…head diff = this PR's four files only ( — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict: APPROVED
Reviewed the exact head 0295ed188f810c629a77c373ebe947da059a465b against base 8711c555da490ad36475f94368bcdd8d8f01048e.
P1 — Intent / policy / contract
beginRuntimeInstance now retains the new runtime instance ID in memory only when both gates and counters are empty. The first real nextSeq/put mutation clones and persists that ID, while non-empty state still takes the existing restart-quarantine path. This restores the documented lazy-first-write contract without changing the file format or public API.
P2 — Correctness / failure handling
Moving mkdirSync inside flushState's existing try boundary correctly classifies directory-creation failures before rename as GateStoreWriteError("not_committed"). The post-rename uncertainty path, corruption fail-closed handling, and prior-instance pending / accepted-unadvanced quarantine logic remain intact.
P3 — Security / trust
The patch introduces no new path, permission, or persistence authority. It preserves the existing fail-closed behavior and removes an untyped raw filesystem error escape on genuine writes.
P4 — Verification / CI
The exact-head tests cover no directory creation at fresh-store construction, persistence of the deferred runtime ID on the first mutation, typed EPERM failure with a clean retry, and restart quarantine for non-empty state. The exact-head CI completed successfully for both affected test shards, TypeScript build, native build, state gates, affected-path validation, and virtual integration validation. The remaining PR contract bootstrap failure is the review-verdict gate documented by the PR body, not a product-test failure.
P5 — Compatibility / platform context
Persistent AgentSession wiring continues to use FileGateStore; the store materializes only when a durable gate is emitted. The change specifically fixes non-writable Windows startup while retaining typed failure for a later real write. The Unreleased changelog entry accurately describes that user-visible behavior.
No blocking findings.
3 participants
Closes #4568.
What
FileGateStore.beginRuntimeInstanceno longer flushes at construction when the store holds no gates and no counters. The runtime instance id is adopted in memory and rides along with the first real mutation, restoring the lazy first-write contract documented on the constructor (from feat(coding-agent): make SDK the canonical external bus #2078).FileGateStore.flushStaterunsmkdirSyncinside the existing try/catch, so a genuinely unwritable directory surfaces as the typedGateStoreWriteError(not_committed) instead of a rawErrnoExceptionescaping the store abstraction.An empty store holds no gates or counters a later process could recover or quarantine, so the eager construction-time write persisted nothing of value. Corruption fail-closed quarantine and restart quarantine of prior-instance records are unchanged.
Why
Every AgentSession construction under a cwd the user cannot write to (Windows consoles defaulting to
C:\Windows\System32) died with an uncaught raw EPERM before any output.Testing
bun test packages/coding-agent/test/workflow-gate-broker.test.ts packages/coding-agent/test/sdk-workflow-gate-emitter.test.tsplusworkflow-gate-redteamandsdk-query-pagination— 90 pass, 0 fail across the four files;agent-loop-harmony-leak5 pass as managed-snapshot overlap sanity.GateStoreWriteError(not_committed)with no file created and retries cleanly once writable.bun --cwd=packages/coding-agent run check— biome + tsc clean on reconstructed head0295ed188f.GJC verdict
Reconstructed onto current dev (
8711c555da(dev tip after #4606/#4607; Dev CI green)) as head0295ed188f810c629a77c373ebe947da059a465b. The head change invalidated the prior verdict, approval, and CI; this line intentionally blocks merge until a fresh independent exact-head approval exists.devbun checkpasses