Skip to content

fix: restore verified session cleanup and SDK recovery contracts - #6388

Merged
Yeachan-Heo merged 6 commits into
devfrom
fix/verified-cleanup-sdk-regressions
Oct 6, 2026
Merged

Yeachan-Heo merged 6 commits into
devfrom
fix/verified-cleanup-sdk-regressions

Conversation

@snowykr

@snowykr snowykr commented Oct 6, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

Fix four runtime-reproduced regressions from the recent task-owner/SDK stack, without reactivating the reverted owner producer or expanding admission authority.

  1. Owner-free SDK cleanup replay after workspace removal (feat(sdk): persist fenced logical owner deletion dispositions #6339/fix(session): restore safe deletion retries and historical GC #6357): authenticate the configured sessions root and immutable receipt-bound target without requiring the old cwd. Owner-bearing replay retains full managed authority and protocol checks.
  2. Unrelated historical sibling scopes blocking owner cleanup (fix(session): authenticate readonly managed GC protocol inspection #6323/fix(session): authenticate async task-owner sibling inventories #6326): authenticate saved binding/namespace and inspect protocol roles through read-only storage when cwd is missing. No owner journal, write authority or admission authority is borrowed. Actual shared references, malformed bindings, replacement/symlink state and unresolved journals remain fail-closed.
  3. GC no-effect refusal reported as destructive partial cleanup (feat(gc): retire logical task owners through immutable native journals #6341/fix(session): restore safe deletion retries and historical GC #6357/fix(session): preserve partial retirement truth and immutable owner replay #6368): honor task_artifact_owner.artifactsRemoved=false as kept, preserving the prepared journal. Actual detached/partial effects remain reclaim_failed.
  4. SDK runtime replacement permanently barred after recovered terminal EIO (fix(sdk): preserve queued cancellation durability through abort and teardown #6369): release only the original correlation after durable terminal recovery and retain the original deadline/reconciliation owner across shutdown. Do not bypass unconfirmed publication or introduce another terminal writer.

Also correct the pending #6349 fragment advertising producer functionality reverted by #6351. Existing transcript-basename backing remains unchanged.

SDK continuation and CI repair

  • A real returning SDK submission now waits for its exact accepted publication Promise. Same-prompt overflow, maintenance and other scheduled recoveries retain captured token/cohort and predecessor scope, rather than borrowing mutable active ownership.
  • Independently queued roots retain separately owned terminals and do not inherit predecessor lifecycle/cohort ownership. Non-SDK same-prompt retries retain their original captured logical lifecycle projection independently of SDK ownership; distinct actual resource runs still settle separately.
  • Disconnecting context/history maintenance cancels only the active cohort's unclaimed publication slots. Publishers synchronously claim exact resolver identities; already publishing terminals retain their authentic success or rejection. Disposal and rejection observation remain explicit.
  • Added actual-returning TODO/overflow, disposal, extension failure and clearContext/newSession/cancelled-manual-compaction regressions. A projected-terminal regression reproduces delivery/disconnect ordering through the real ExtensionRunner, not a fabricated success terminal.
  • Split two cumulative fixture loops into parameterized tests, retaining all 21 cases and assertions and the existing per-case deadlines.
  • The code-less Broker startup fixture uses a lean real child and canonical parent publication with the child's actual PID/incarnation. Its 4000 ms semantic deadline, cleanup-proof assertions and late-proof refusal remain intact.

Reproduction

Dedicated repair worktree: /home/snowy/coding/gjc-verified-regressions; original base db03185d5d45e0bf9bd31dc8fda0af2156e9c574.

  • Original real-native/Broker/SDK probes before product edits: 4 pass / 5 fail across 9 cases. Identical probes after repair and at the final source: 9 pass / 0 fail, 85 assertions.
  • Initial tracked regressions checked with only the four production repairs removed: 3 pass / 6 fail; restoring the identical patch: 9 pass / 0 fail. The extra SDK case verifies recovery across shutdown before terminal retry.
  • SDK injects one actual terminal atomic-rename EIO, then checks genuine durable recovery and cancelled turn.result before replacement.
  • GC exercises actual storage after real late sibling/protocol obstructions; exact payloads, transcript and prepared journal remain unchanged on no-effect refusal. Native retained namespaces remain legitimate partial cleanup.
  • Follow-up critic findings were dynamically reproduced before their fixes: a disconnected accepted submission could remain pending, and disconnect could cancel an already-publishing projected terminal. Both regressions now pass with the actual sender Promise.

Latest local verification

Final head: e0fd2b053 (SDK repair 449907133, fixture repair 9e2473802, non-SDK lineage correction e0fd2b053).

  • Entire SDK host + non-SDK auto-retry suites: 229 pass, 0 fail, 3171 assertions (1348.34 seconds): all 222 SDK cases and 7 retry cases on the final source.
  • Original runtime probes: 9 pass, 0 fail, 85 assertions.
  • Owner deletion/GC suites: 51 pass, 2 existing skips, 0 fail, 511 assertions. Initial broader six-suite storage/safety run: 97 pass, 2 existing skips, 0 fail.
  • Final combined returning-SDK/projected-disconnect focused run: 12 pass, 0 fail, 104 assertions.
  • Changed Broker fixture plus cutoff/late-proof negative controls: 3 pass, 0 fail, 12 assertions.
  • bun --cwd=packages/coding-agent run check: exit 0, including vendor checks, Biome and types. Existing 3 warnings and 1 info remain visible. The 1.1 MiB producer is also explicitly checked/formatted with --files-max-size=2000000; the aggregate's normal size warning is not hidden.
  • git diff --check and released-changelog history guard against origin/dev: passed.

Full local Broker suite is NOT green: 146 pass, 1 existing skip, 5 fail and 1 error. Isolated re-execution yielded 3 pass / 2 fail. An unchanged b00417053 baseline in a separate worktree also failed: 147 pass, 1 skip, 4 fail, including startup cutoffs and same-generation successor handling. This does not prove every local failure is unrelated to this change; no deadline increase, assertion weakening or skip was used to obtain a pass. Hosted CI must be assessed on the latest head.

Latest hosted CI

Exact head: e0fd2b053934d1380a511d108c5aaeb48be9b6d0.

  • Dev CI run 37456598234: completed / success, 30 successful jobs, no failed jobs. Affected-path validation and virtual integration validation succeeded.
  • Public site sync run 37456598382: completed / success.
  • Six conditional/opt-in jobs were skipped, including WSLv2/NTFS qualification, Windows native toolchain and Darwin ARM tab-worker smoke. Those skipped qualifications are not claimed passing. Body-edit-only skipped workflow runs are not used as code evidence.
  • Local non-green full Broker results remain disclosed above; hosted success does not erase those observations.

Independent critic review

The original cleanup/authority and durable-owner critics returned OKAY. A fresh read-only continuation critic then required two concrete producer repairs; both were reproduced, fixed and independently re-reviewed to advisory PASS, with no residual concrete implementation findings. Critics performed no tests/builds/formatters. All execution evidence above is parent verification, not critic execution. Latest-head CI on 9e2473802 exposed one existing non-SDK retry identity assertion; it was reproduced locally, corrected without modifying the assertion, and re-reviewed to advisory PASS before the final 229-case union run. The failed shard caused its evidence/aggregate failures; 26 other CI jobs succeeded. The new head is assessed separately, not by rerunning that old failure.

Limits / risk

  • Local runtime verification is Linux only. Latest-head Windows/macOS qualification depends on hosted CI.
  • Returning overflow cases exercise managed recovery/fallback with compaction disabled; they do not establish enabled-compaction/full-handoff or external-provider E2E coverage.
  • Full repository aggregate and browser E2E were not run. Full local Broker failures remain explicitly disclosed above.
  • High-risk authenticated cleanup/durable cancellation/publication boundaries are covered by real reproductions and negative controls.
  • No producer activation, protocol/schema migration, fake task completion, guard weakening, new skip or test-timeout increase.

Targets dev. No merge performed or requested. Advisory agent PASS does not replace exact-current-head approval from a write-access maintainer; latest-head CI remains required.

Owner-free replay and sibling inspection made stored session cleanup depend
on unrelated workspace existence. GC also treated a no-effect owner fence
as destructive partial retirement. Authenticate stored identities without
granting new owner authority, and preserve the explicit effect boundary.

Lore-id: b4d49e70
Constraint: retain receipt-bound parent and artifact identities
Constraint: historical inspection must not grant write or admission authority
Confidence: high
Scope-risk: medium
Reversibility: easy
Tested: six cleanup safety suites, 97 passed and 2 existing skips
Tested: tracked regressions fail without repairs and pass with repairs
Not-tested: Windows and macOS runtime execution
A failed removal publisher retained its immutable false result even after
the original deadline owner durably recovered the terminal. Preserve that
owner across teardown and discharge only its confirmed correlation, so a
transient terminal EIO does not permanently prevent runtime replacement.

Lore-id: c830af21
Constraint: never acknowledge cancellation before durable terminal recovery
Constraint: preserve the original reconciliation owner across shutdown
Confidence: high
Scope-risk: medium
Reversibility: easy
Tested: seven queued shutdown and recovery scenarios passed
Tested: EIO recovery before and after shutdown fails without this repair
Not-tested: full SDK host suite exceeded a 300-second observation window
Not-tested: Windows and macOS runtime execution
…aims

The pending producer fragment still advertised functionality removed by
its revert. Describe the deferred activation accurately and record the
four verified runtime regressions without claiming full task-move delivery.

Lore-id: d965c271
Constraint: do not advertise the reverted task-owner producer
Confidence: high
Scope-risk: narrow
Reversibility: easy
Tested: source behavior and independent critic scope review

@snowykr snowykr left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Advisory agent review (not maintainer approval): two independent critic lanes reviewed the implementation diff and current source in the dedicated worktree before final formatting/commits. Both returned OKAY with no grounded P1/P2 findings.

Historical identity lane confirmed configured-root/receipt/parent fences and restricted read-only missing-cwd sibling inspection without registration of write/admission authority. Durable-owner lane confirmed exact-correlation retirement only after durable recovery, original-owner lease retention across teardown, and artifactsRemoved=false GC classification. No producer reactivation or physical native reclamation claim.

Parent execution independently demonstrated tracked regression negative control 3 pass/6 fail with production fixes absent, then 9 pass/0 fail with the identical patch restored; cleanup safety union 97 pass/2 existing skips; SDK affected cohort 7 pass; package check and changelog history passed. Full SDK file timed out at 300 seconds and is not credited. Linux-only evidence, no Windows/macOS or full aggregate claim.

This COMMENTED review is advisory. It is not an approving write-access maintainer review on the exact current head, and it authorizes no merge.

@probepark probepark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review (head b004170, gajae-reviewer on behalf of probepark)

CI: 1 base-caused, 2 unclassified — check:@gajae-code/coding-agent and the cleanup/GC/owner-deletion targeted tests are green; 3 test jobs failed.

  • base-caused: test:packages/coding-agent/src/sdk/host/session-runtime.test.ts fails only on post-acceptance invocation terminalization > terminalizes a synchronous throw during a todo-reminder continuation (session-runtime.test.ts:5440, agent_start length 3 vs 2). The same assertion fails on base db03185 in Dev CI shard-3: https://github.com/Yeachan-Heo/gajae-code/actions/runs/37359736112 (job 111937695986). The two new SDK-only queued terminal recovery permits replacement after EIO cases passed.
  • unclassified: test:packages/coding-agent/test/sdk-broker-lifecycle-e2e.test.ts → broker preserves a code-less lifecycle startup failure message (sdk-broker-lifecycle-e2e.test.ts:5805) got terminal_uncertain ("Lifecycle startup cleanup could not be proven … child=alive") where it expected spawn_failed. The same test passed on base db03185 (shard-6, job 111937695964). This PR changes lifecycle.ts only in validateDeletePath plus the getSessionsDir import, so it looks like a cleanup-timing flake, but no run shows that.
  • unclassified: Windows dev:doctor + session-path regression → packages/natives/test/walker-pool-unavailable.windows.test.ts:127 (afterThreads 21 > beforeThreads 20). This PR touches no natives code, but the job is skipped on dev, so there is no base run to compare.

Scope: +342 / -19, 9 files: packages/coding-agent/src/sdk/broker, sdk/host, session/internal, session/session-retirement.ts, tests, and 2 changelog fragments.
Conventions: changelog fragment present (changelog.d/verified-cleanup-sdk-recovery.md), and the stale #6349 fragment was corrected. No generated files. No labels. No console.* and no mock.module. The spies in the new tests are restored in finally / right after use.

Notable:

  • src/sdk/broker/lifecycle.ts:6405-6418: the owner-free replay now skips managedCandidates(broker, cwd) and authenticates replay.target.sessionsRoot against getSessionsDir(agentDir), with a containment check on transcriptPath. Owner-bearing replay still goes through the full managed inventory. This is correct for #6339. The check is lexical (path.relative) and does not re-verify the immutable receipt match at 6400-6404. That match does run, so I am not counting this as a finding.
  • src/session/internal/managed-session-scope.ts:3498-3541: the cwd_missing branch builds the candidate scope from the binding. It refuses when hasManagedGcRetirementJournalWithoutWorkspace is true, and it reads through a read-only ManagedSessionDescendantStore pinned to the scope dev/ino captured before the read. The binding identity is still compared with input.bindingIdentity, so this is fail-closed. session-retirement.ts:492 maps only task_artifact_owner + artifactsRemoved === false to kept. Other phases stay cleanup_pending. session-runtime.ts:6641 retires the queue cancellation only from onExpired, which PromptDeadlineManager calls only after durable terminal confirmation (prompt-deadline-manager.ts:506-511).

Blocking:

  1. Unclassified CI failure sdk-broker-lifecycle-e2e.test.ts:5805 at this head (it passes on base).
  2. Unclassified CI failure in Windows walker-pool-unavailable.windows.test.ts:127 at this head.
    I found no code defect in the diff. Re-run both failed jobs on this exact head, or show they are flaky or broken outside this PR (for example, the same failure on another head). Then re-request review.

Body verdict line: the PR body has no gajae.pr-review-verdict.v1 line (count=0), so the body was not edited. Suggested verdict line: gajae.pr-review-verdict.v1 needs-human sha256:98ee3de850517f22cd4c16a2ff16f5b4d716cccb817a0d663c3e8b53fe069469 reviewer:critic reviewer-id:gajae-reviewer evidence:code-read-no-defect;base-fail-session-runtime-todo-reminder;unclassified-lifecycle-e2e-5805;unclassified-windows-walker-pool

Verdict: gajae.pr-review-verdict.v1 needs-human sha256:98ee3de850517f22cd4c16a2ff16f5b4d716cccb817a0d663c3e8b53fe069469 reviewer:critic reviewer-id:gajae-reviewer evidence:code-read-no-defect;base-fail-session-runtime-todo-reminder;unclassified-lifecycle-e2e-5805;unclassified-windows-walker-pool

A real SDK sender could finish on a predecessor before its continuation failed,
or remain pending after maintenance disconnected the terminal event bridge.
Capture each accepted token publication, propagate only causal recovery cohorts,
and let already claimed publishers retain their genuine completion outcome.

Lore-id: e2d47a19
Constraint: independent queued roots must not inherit predecessor publication ownership
Rejected: never-settling SDK sender fixtures | conceal real submission completion failures
Confidence: high
Scope-risk: wide
Reversibility: easy
Tested: full SDK host 222 passing tests and 3114 assertions; package check; real projected disconnect race
Not-tested: enabled-compaction/full-handoff matrix and external provider end-to-end behavior
The code-less failure fixture spent its original readiness budget importing the
full lifecycle implementation before publishing the intended owned failure.
Keep the 4000ms semantic deadline and authenticate the lean child's real PID and
incarnation while publishing the canonical failure through the parent seam.

Lore-id: 349c82a6
Constraint: cleanup proof and late-deadline refusal assertions must remain intact
Confidence: high
Scope-risk: narrow
Reversibility: easy
Tested: startup fixture and cutoff/late-proof controls 3 passing tests; package check
Not-tested: full Broker suite is not green locally; unchanged baseline also fails startup and successor cases
@snowykr

snowykr commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Advisory critic follow-up for current head 9e2473802: PASS after the reproduced disconnected-waiter P1 and already-publishing-terminal P2 were repaired. The read-only critic inspected causal scope/token/cohort propagation, independent queued roots, exact resolver publisher claims, disposal/rejection handling, the genuine projected-extension regression, and unchanged Broker deadline/PID/incarnation/late-proof constraints. No concrete residual implementation findings. This is an agent advisory comment, not a maintainer approval or merge authorization.

Parent execution on the committed source: full SDK host 222 pass / 0 fail / 3114 assertions; original runtime probes 9 pass / 0 fail; owner suites 51 pass / 2 existing skips / 0 fail; package check succeeds with existing warnings visible. Full local Broker suite remains 146 pass / 1 skip / 5 fail / 1 error; unchanged b00417053 baseline is also non-green (147 pass / 1 skip / 4 fail). The PR body records exact evidence and limits. Latest-head hosted CI is still being monitored; no all-green or merge claim is made.

@probepark probepark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review (head 9e24738, gajae-reviewer on behalf of probepark)

CI: 1 PR-caused failure. Affected path validation / test:@gajae-code/coding-agent:shard-1-of-8 (job 112228865332) fails test/agent-session-retry-busy-recovery.test.ts:441, keeps a retry predecessor settled and terminally distinct from its successor: lifecycleStarts[1].lifecycleScope is the retry's own scope (generation: 2) instead of the first attempt's scope (generation: 1). The same test passes on base db03185 (Dev CI shard-1, job 111937695851). Affected path validation and evidence producer are red only because they aggregate this shard. The previous head's two unclassified failures (sdk-broker-lifecycle-e2e.test.ts:5805, Windows walker-pool) did not recur at this head: the e2e test was rewritten in 9e24738, and the Windows job is not planned for this head.
Scope: +1323 / -320, 11 files (ocr reviewable: 5 files, +342 / -92). Areas: src/session/agent-session.ts (+268/-76, new since my last review at b004170), sdk/broker/lifecycle.ts, sdk/host/session-runtime.ts, session/internal/managed-session-scope.ts, session/session-retirement.ts, tests, and 2 changelog fragments.
Conventions: changelog fragment present (changelog.d/verified-cleanup-sdk-recovery.md). No generated files, no labels, and no new console.* in src/.

Notable:

  • src/session/agent-session.ts:3463-3472 (#acceptSdkAttemptRun): the lifecycle-scope inheritance (#lifecycleScopesByAttemptScope.set(handle.scope, …predecessorScope)) moved out of the queued-continuation acceptance path and into the if (sdkRunToken !== undefined) block. It is now keyed on the predecessorScope argument.
  • src/session/agent-session.ts:8744-8763: the call site now passes inheritedSdkOwnership?.scope, which comes from #captureSdkContinuationOwnership and is undefined whenever the predecessor has no SDK token. On base (agent-session.ts:8679-8686 @ db03185), the predecessor scope came from #agentEventAdmission.get(predecessorAgentEnd)?.scope and was set unconditionally for every accepted continuation with a predecessor agent_end. That included tokenless (non-SDK) retries, which is the contract from #6176 / 8aa5049 ("retain public lifecycle identity across tokenless retries").
  • The previous findings at b004170 (lifecycle owner-free replay, cwd_missing scope, session-retirement.ts kept mapping) are unchanged in this delta. The new SDK publication map (#sdkTerminalPublications) is rejected on bridge disconnect (:10238-10253) and on dispose (:10411-10415), and resolved or rejected in #publishDeferredAgentEnd's try/catch/finally. I found no leak there.

Blocking:

  1. Regression in tokenless retry lifecycle identity (agent-session.ts:3463-3472 + :8744-8763). A non-SDK auto-retry successor no longer inherits its predecessor's lifecycleScope, so public agent_start events for one logical prompt carry two different lifecycle scopes. CI catches this at agent-session-retry-busy-recovery.test.ts:441 (passes on base). Restore the predecessor-scope inheritance outside the sdkRunToken guard (for example, derive the predecessor scope from predecessorAgentEnd as base did, independent of SDK ownership), then re-run shard-1.

Body verdict line: the PR body has no gajae.pr-review-verdict.v1 line (count=0), so the body was not edited. Suggested verdict line: gajae.pr-review-verdict.v1 needs-human sha256:dcb62f2d727857bd6fa3d23226f7c637909d3e50991e3db595279451cc2b9dd3 reviewer:critic reviewer-id:gajae-reviewer evidence:pr-caused-shard1-retry-lifecycle-scope;agent-session-3463-8744-tokenless-inheritance-lost;base-pass-job111937695851

Verdict: gajae.pr-review-verdict.v1 needs-human sha256:dcb62f2d727857bd6fa3d23226f7c637909d3e50991e3db595279451cc2b9dd3 reviewer:critic reviewer-id:gajae-reviewer evidence:pr-caused-shard1-retry-lifecycle-scope;agent-session-3463-8744-tokenless-inheritance-lost;base-pass-job111937695851

SDK publication ownership must not be a prerequisite for projecting the logical
scope of a same-prompt retry. Keep the captured predecessor projection for real
non-SDK retries without changing their distinct resource runs or assigning it
to independently promoted queued roots.

Lore-id: d5c8f103
Constraint: lifecycle projection follows the claimed causal predecessor, never mutable active scope
Confidence: high
Scope-risk: narrow
Reversibility: easy
Tested: SDK host and auto-retry suites 229 passing tests and 3171 assertions; package check; critic advisory PASS
Not-tested: local full Broker remains non-green; enabled-compaction/full-handoff and external provider E2E
@snowykr

snowykr commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Current-head correction e0fd2b053: prior Dev CI run37450037956 failed one non-SDK logical lifecycle identity assertion (26 other jobs succeeded); the evidence/aggregate failures were derivative. The exact case failed locally, then passed unchanged after restoring only causally captured non-SDK predecessor projection. Independent queued roots remain excluded, and no SDK token/cohort is manufactured.

The same canonical read-only critic returned advisory PASS for this narrow follow-up. Final SDK host + auto-retry union: 229 pass / 0 fail / 3171 assertions; package check passes with existing warnings visible. The 19-line correction is committed and pushed, not an old-head rerun. Full local Broker limitations remain disclosed in the body. New-head CI is being monitored separately. No maintainer approval or merge is claimed.

@probepark probepark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Code review (CI still running on e0fd2b0; approval decision follows once CI finishes).

Review (head e0fd2b0, gajae-reviewer on behalf of probepark)

CI: green except Virtual integration validation (still pending). Affected path validation / test:@gajae-code/coding-agent:shard-1-of-8 now passes (job 112251359685), and so do check:@gajae-code/coding-agent, sdk-broker-lifecycle-e2e.test.ts, session-runtime.test.ts, and the cleanup/GC/owner-deletion targeted jobs (run 37456598234).
Scope: +1328 / -320, 11 files (ocr reviewable: 5 files, +347 / -92). Delta since my last review at 9e24738 is one commit: e0fd2b0, src/session/agent-session.ts +12/-7.
Conventions: changelog fragment present (changelog.d/verified-cleanup-sdk-recovery.md). No generated files, no labels, and no new console.* in src/.

Notable:

  • src/session/agent-session.ts:3463-3469 (#acceptSdkAttemptRun): the #lifecycleScopesByAttemptScope.set(handle.scope, …predecessorScope) inheritance is back outside the if (sdkRunToken !== undefined) guard, so tokenless (non-SDK) retries inherit the predecessor's lifecycle scope again. This was the regression from #6176 / 8aa5049 that I flagged at 9e24738.
  • src/session/agent-session.ts:8749-8752 + :8768: the scope that reaches the call site is now inheritedSdkOwnership?.scope ?? predecessorScope, where predecessorScope is the claimed predecessorAgentEnd.scope (:8709-8712). It is undefined for continueQueuedOnly and startsQueuedSuccessor, so independently promoted queued roots still get their own lifecycle scope. One difference from base (db03185 :8679-8686): base also inherited on continueQueuedOnly and queued-successor starts. Excluding those looks intentional (the commit message says "without … assigning it to independently promoted queued roots"), and the gated suites pass, including agent-session-promotion-identity.test.ts and agent-session-concurrent.test.ts.
  • The rest of the diff did not change since 9e24738. My earlier read of the #sdkTerminalPublications lifecycle (rejected on disconnect and dispose, settled in #publishDeferredAgentEnd) still holds.

Blocking: none. Blocking item 1 from my 9e24738 review (review 5427566098) is resolved by e0fd2b0, and shard-1 is green on this head.

Approval is held until Virtual integration validation finishes on this head. I will dismiss the stale CR then.

@snowykr

snowykr commented Oct 6, 2026

Copy link
Copy Markdown
Collaborator Author

Verified exact current head e0fd2b053934d1380a511d108c5aaeb48be9b6d0: Dev CI37456598234 reached completed / success, with30successful jobs and no failed jobs; affected-path and virtual-integration validations succeeded. Public site sync37456598382 also succeeded. Six conditional/opt-in jobs skipped (including WSL/NTFS, Windows native toolchain, Darwin ARM worker smoke) are not claimed qualified; body-edit-only skipped runs are not code evidence.

Final local SDK+retry union 229pass/0fail/3171assertions, original runtime reproductions9pass/0fail, owner suites51pass/2existing skips/0fail, package check and advisory critic PASS are recorded in the body. The full local Broker suite's failures and unchanged-baseline failures remain disclosed; this hosted result does not retroactively mark them passing. No merge or maintainer approval has been manufactured.

@probepark probepark left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Review (head e0fd2b0, gajae-reviewer on behalf of probepark)

CI: green. The approve gate on this exact head returned ALLOW with 0 pending, 0 failed, and check:@gajae-code/coding-agent covered. Virtual integration validation finished successfully in run 37456598234, which is what my earlier code review was waiting for.
Scope: +1328 / -320, 11 files (ocr reviewable: 5 files, +347 / -92). The code review for this head is my previous review, 5428014285 (#6388 (review)). The diff has not changed since then.
Conventions: changelog fragment present (changelog.d/verified-cleanup-sdk-recovery.md). No generated files and no labels.
Notable:

  • src/session/agent-session.ts:3463-3469: tokenless retries now inherit the lifecycle scope again, which resolves the blocker from my 9e24738 review (5427566098). Queued roots (continueQueuedOnly / queued-successor) intentionally do not inherit, as the commit message says.
    Blocking: none. CI finished and there are no blocking findings, so my earlier CHANGES_REQUESTED reviews on this PR are superseded.

Body verdict line: the PR body has no gajae.pr-review-verdict.v1 line, so it was not edited. Suggested verdict line: gajae.pr-review-verdict.v1 merge-approved sha256:2777d8565719f687d83d4d5aa4bd77797d6658acd4f2a7f7d73445e7e785ee30 reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;tokenless-retry-scope-fix-verified;ocr-reviewable-439

Verdict: gajae.pr-review-verdict.v1 merge-approved sha256:2777d8565719f687d83d4d5aa4bd77797d6658acd4f2a7f7d73445e7e785ee30 reviewer:human reviewer-id:probepark evidence:ci-green;approve-gate-allow;tokenless-retry-scope-fix-verified;ocr-reviewable-439

@probepark
probepark dismissed stale reviews from themself October 6, 2026 12:20

Blocking findings resolved at e0fd2b0; superseded by the follow-up verdict 5428198382.

@Yeachan-Heo
Yeachan-Heo merged commit 9eab9e2 into dev Oct 6, 2026
72 checks passed
Yeachan-Heo pushed a commit that referenced this pull request Oct 6, 2026
…ferral

Revert the synchronous-emit change from the bad commit that broke #6388's design.
SDK agent_end events are now emitted synchronously for visibility to subscribers,
but the terminal publication resolution remains deferred as per #6388.

This ensures that:
- Subscribers see agent_end events immediately (synchronous emit in #handleAgentEvent)
- Prompt settlement waits for publications to be resolved (#publishDeferredAgentEnd)
- Claimed predecessors' publications are not leaked
- No duplicate emissions occur

Fixes test expectations that changed with #6388's deferral, citing the design change.
Yeachan-Heo pushed a commit that referenced this pull request Oct 6, 2026
…al semantics

PR #6388 introduced a deferral mechanism for SDK agent_ends to hold them until
settlement conditions allow publication. However, held SDK agent_ends without
explicit decisions were never being published, causing them to be rejected in
dispose() with "Session disposed before SDK terminal publication" errors.

This commit makes three key changes to respect #6388's deferral contract:

1. #flushPendingAgentEnd: Collect items before modifying the Set to avoid
   iterator issues when deleting during iteration.

2. #flushPendingAgentEnd: Publish held SDK agent_ends that don't have explicit
   decisions yet (as long as they're not reserved for continuations). These
   deferred agent_ends will be emitted during publication to complete the
   deferral lifecycle.

3. dispose(): Only reject SDK terminal publications that are not currently being
   published. Deferred publications should be allowed to complete naturally
   during the #dispose() cleanup phase, which already waits for #agentEndPublicationPromise.

Fixes: AgentSession message pipeline > holds prompt settlement until worker
integration is durable test by ensuring SDK submissions create resolvable
publications rather than orphaned entries that cause disposal failures.

Reference: PR #6388 fix: restore verified session cleanup and SDK recovery contracts
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
9eab9e2 (#6388) landed with a cancelled Dev CI run (37462960786). Its
agent-session.ts rework moved three deferral release edges, and the three
contracts they touch are pinned by tests that now contradict each other: the
rewritten session-runtime.test.ts defers an SDK terminal until the continuation
decision is known, agent-session-message-pipeline.test.ts requires the
correlated agent_end to reach the client before post-prompt recovery drains,
and agent-session-auto-compaction-continue.test.ts requires the predecessor
terminal to stay unpublished while a queued successor is pending delivery.
Restoring any one edge breaks a pinned case of another (measured on the 64-hunk
patch: 9 failures after a blind revert, 3 after restoring the emit edge and the
queued-successor release edge).

This reverts only the two files that carry that rework — agent-session.ts and
its session-runtime.test.ts rewrite — and keeps #6388's cleanup, lifecycle, and
task-owner work, which newer dev commits build on (reverting those reintroduces
a concurrent receipt-writer race under load).

Lore-id: dff80ed7
Constraint: keep the cleanup and task-owner work newer dev commits depend on
Constraint: the todo-reminder continuation flake must stay fixed
Rejected: full revert of 9eab9e2 | reopens the concurrent receipt-writer race
Rejected: reconciling the three edges in place | breaks a pinned case per edge
Tested: session-runtime 197/197 (twice), message-pipeline 26/26, auto-compaction 30/30, artifact-owner suites 34/19/152
Not-tested: dev CI shards on the Linux runner
Confidence: high
Scope-risk: narrow
Reversibility: clean
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…tions

- Restore bounded-cleanup-history-replay.md with description of reverted #6388 cleanup features
- Restore verified-cleanup-sdk-recovery.md with description of reverted terminal publication and recovery changes
- Both fragments now accurately describe what was reverted in this PR rather than advertising unshipped features
- Preserve changelog fragment records per release flow requirements (scripts/release.ts)
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…n only

Revert only the SDK terminal publication edges from #6388 that cause
managed-receipt-process-ownership race conditions (failed 4/6 test runs).
Restore lifecycle, session-scope, retirement, and artifact-owner paths
to dev to preserve cleanup and task-owner features that newer commits
depend on. Keep session-runtime.test.ts harness fixes from bd0a880.

Lore-id: 2c3d4e5f
Constraint: keep publication-edge revert in agent-session.ts
Constraint: preserve all post-#6388 cleanup and task-owner features
Tested: managed-receipt-process-ownership 5x, session-runtime, sdk-broker-lifecycle-e2e
Confidence: high
Scope-risk: narrow
Reversibility: clean
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…tions

- Restore bounded-cleanup-history-replay.md with description of reverted #6388 cleanup features
- Restore verified-cleanup-sdk-recovery.md with description of reverted terminal publication and recovery changes
- Both fragments now accurately describe what was reverted in this PR rather than advertising unshipped features
- Preserve changelog fragment records per release flow requirements (scripts/release.ts)
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…n only

Revert only the SDK terminal publication edges from #6388 that cause
managed-receipt-process-ownership race conditions (failed 4/6 test runs).
Restore lifecycle, session-scope, retirement, and artifact-owner paths
to dev to preserve cleanup and task-owner features that newer commits
depend on. Keep session-runtime.test.ts harness fixes from bd0a880.

Lore-id: 2c3d4e5f
Constraint: keep publication-edge revert in agent-session.ts
Constraint: preserve all post-#6388 cleanup and task-owner features
Tested: managed-receipt-process-ownership 5x, session-runtime, sdk-broker-lifecycle-e2e
Confidence: high
Scope-risk: narrow
Reversibility: clean
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…tions

- Restore bounded-cleanup-history-replay.md with description of reverted #6388 cleanup features
- Restore verified-cleanup-sdk-recovery.md with description of reverted terminal publication and recovery changes
- Both fragments now accurately describe what was reverted in this PR rather than advertising unshipped features
- Preserve changelog fragment records per release flow requirements (scripts/release.ts)
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…n only

Revert only the SDK terminal publication edges from #6388 that cause
managed-receipt-process-ownership race conditions (failed 4/6 test runs).
Restore lifecycle, session-scope, retirement, and artifact-owner paths
to dev to preserve cleanup and task-owner features that newer commits
depend on. Keep session-runtime.test.ts harness fixes from bd0a880.

Lore-id: 2c3d4e5f
Constraint: keep publication-edge revert in agent-session.ts
Constraint: preserve all post-#6388 cleanup and task-owner features
Tested: managed-receipt-process-ownership 5x, session-runtime, sdk-broker-lifecycle-e2e
Confidence: high
Scope-risk: narrow
Reversibility: clean
@probepark

Copy link
Copy Markdown
Collaborator

Dev CI run 37503598987 at a0e7e27 is a source regression, not a flake: the failing shard tests include agent-session-message-pipeline and agent-session-auto-compaction-continue, while packages/coding-agent/src/session/agent-session.ts changed between the last green dev run (e4afbc2) and this head via #6388 and #5919. The failing test files are byte-identical. Please investigate/revert/fix the affected session lifecycle changes; no flaky ack was added.

Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…tions

- Restore bounded-cleanup-history-replay.md with description of reverted #6388 cleanup features
- Restore verified-cleanup-sdk-recovery.md with description of reverted terminal publication and recovery changes
- Both fragments now accurately describe what was reverted in this PR rather than advertising unshipped features
- Preserve changelog fragment records per release flow requirements (scripts/release.ts)
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…n only

Revert only the SDK terminal publication edges from #6388 that cause
managed-receipt-process-ownership race conditions (failed 4/6 test runs).
Restore lifecycle, session-scope, retirement, and artifact-owner paths
to dev to preserve cleanup and task-owner features that newer commits
depend on. Keep session-runtime.test.ts harness fixes from bd0a880.

Lore-id: 2c3d4e5f
Constraint: keep publication-edge revert in agent-session.ts
Constraint: preserve all post-#6388 cleanup and task-owner features
Tested: managed-receipt-process-ownership 5x, session-runtime, sdk-broker-lifecycle-e2e
Confidence: high
Scope-risk: narrow
Reversibility: clean
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…tions

- Restore bounded-cleanup-history-replay.md with description of reverted #6388 cleanup features
- Restore verified-cleanup-sdk-recovery.md with description of reverted terminal publication and recovery changes
- Both fragments now accurately describe what was reverted in this PR rather than advertising unshipped features
- Preserve changelog fragment records per release flow requirements (scripts/release.ts)
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…n only

Revert only the SDK terminal publication edges from #6388 that cause
managed-receipt-process-ownership race conditions (failed 4/6 test runs).
Restore lifecycle, session-scope, retirement, and artifact-owner paths
to dev to preserve cleanup and task-owner features that newer commits
depend on. Keep session-runtime.test.ts harness fixes from bd0a880.

Lore-id: 2c3d4e5f
Constraint: keep publication-edge revert in agent-session.ts
Constraint: preserve all post-#6388 cleanup and task-owner features
Tested: managed-receipt-process-ownership 5x, session-runtime, sdk-broker-lifecycle-e2e
Confidence: high
Scope-risk: narrow
Reversibility: clean
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…tions

- Restore bounded-cleanup-history-replay.md with description of reverted #6388 cleanup features
- Restore verified-cleanup-sdk-recovery.md with description of reverted terminal publication and recovery changes
- Both fragments now accurately describe what was reverted in this PR rather than advertising unshipped features
- Preserve changelog fragment records per release flow requirements (scripts/release.ts)
Yeachan-Heo pushed a commit that referenced this pull request Oct 7, 2026
…n only

Revert only the SDK terminal publication edges from #6388 that cause
managed-receipt-process-ownership race conditions (failed 4/6 test runs).
Restore lifecycle, session-scope, retirement, and artifact-owner paths
to dev to preserve cleanup and task-owner features that newer commits
depend on. Keep session-runtime.test.ts harness fixes from bd0a880.

Lore-id: 2c3d4e5f
Constraint: keep publication-edge revert in agent-session.ts
Constraint: preserve all post-#6388 cleanup and task-owner features
Tested: managed-receipt-process-ownership 5x, session-runtime, sdk-broker-lifecycle-e2e
Confidence: high
Scope-risk: narrow
Reversibility: clean
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants