fix(sdk): bound accepted prompts that never reach execution (#4668) - #4669
fix(sdk): bound accepted prompts that never reach execution (#4668)#4669Yeachan-Heo wants to merge 6 commits into
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…4668) Red-team review of the zero-execution bound found three gaps: - agent_start leased only the head of a drained batch, so follow-ups promoted together past the head had no deadline and could remain accepted with zero execution forever. Lease every drained prompt; acceptance-anchored leases make own-turn re-entry a no-op. - A non-empty agent_start re-entry replaces the tracked invocation without a prior agent_end only for wedged/synthetic sequences; the replaced turn's lease is now deliberately retained and documented so its record still terminalizes at its deadline instead of losing its zero-progress bound. - The goal.list/get diagnostic payload replaced resource_gone, but the shared adapter-disposition fixture still expected resource_gone; the parity expectation now intentionally asserts ok: true for the no_active_goal payload across mcp/acp/daemon-cli adapters. Lore-id: 4668b2d1 Constraint: queued submissions stay promotion-leased so a prompt behind a legitimately long turn never false-fires Constraint: no release/tag/publish; PR #4669 exact-head only Rejected: clearing the replaced turn's lease at re-entry | leaves its record accepted with no bound Rejected: terminating the replaced record at re-entry | fabricates an outcome for a turn whose real terminal event may still arrive Confidence: high Scope-risk: narrow Reversibility: trivial Directive: every drained prompt correlation must hold a deadline lease; adapter parity expectations track the goal diagnostic payload Tested: bun test session-runtime.test.ts -t zero-execution (5 pass); sdk-adapter-dispositions{,-mcp,-daemon-cli,-acp}.test.ts (291+97+97+97 pass); biome+tsc clean Not-tested: full CI pending on this head
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict: Request changes
Summary
The acceptance-anchored lease fixes the original own-turn gap, but the queued-follow-up promotion path still has an unbounded window and can misattribute ownership across runs. This leaves the core “accepted ⇒ progress or bounded terminal error” contract incomplete.
Findings
[P1] Lease a follow-up when it is actually promoted, or terminalize the promotion/start gap
packages/coding-agent/src/sdk/host/session-runtime.ts:2808-2814 (onPromotedTurn) only appends the correlation to pending. The corresponding deadline lease is created later in emitLifecycle() (:2638-2645), when agent_start drains pending.
A queued follow-up can therefore be durably accepted and promoted, then wedge before agent_start (or lose the lifecycle event). In that state it has no lease and remains accepted indefinitely—the same false-owner hazard this PR is intended to remove. The existing tests cover “no agent_start” for an own-turn prompt, but not for a follow-up after promotion.
Please create the lease at the actual promotion boundary (while keeping prompts merely waiting in the queue unleased), or add an equivalent bounded terminal path for promotion without agent_start. Add a regression test that invokes the promotion callback and then never emits agent_start, asserting prompt_deadline_exceeded.
[P1] Do not enqueue in-run follow-ups for a later unrelated agent_start
The same onPromotedTurn callback is also used when a follow-up is consumed inside an already-running agent loop. That path does not necessarily produce a new agent_start, but it still appends to pending. A later, unrelated agent_start can drain the stale correlation, lease it as part of the wrong run, and transfer abort ownership to the wrong connection; it may then produce a false prompt_deadline_exceeded.
Promotion needs an explicit run/attempt identity (or an equivalent distinction between “new root run” and “consumed in current run”). In-run consumption must not remain in pending; it should be transitioned/retired at that boundary. Add a regression test for an in-run follow-up followed by a separate agent-initiated agent_start.
Verification notes
The PR’s reported suites are useful coverage for the original repro and the drained-batch/re-entry cases, but they do not exercise either promotion-without-agent_start or stale in-run promotion. Those cases are required before this can be approved.
…4668) Red-team review of the zero-execution bound found three gaps: - agent_start leased only the head of a drained batch, so follow-ups promoted together past the head had no deadline and could remain accepted with zero execution forever. Lease every drained prompt; acceptance-anchored leases make own-turn re-entry a no-op. - A non-empty agent_start re-entry replaces the tracked invocation without a prior agent_end only for wedged/synthetic sequences; the replaced turn's lease is now deliberately retained and documented so its record still terminalizes at its deadline instead of losing its zero-progress bound. - The goal.list/get diagnostic payload replaced resource_gone, but the shared adapter-disposition fixture still expected resource_gone; the parity expectation now intentionally asserts ok: true for the no_active_goal payload across mcp/acp/daemon-cli adapters. Lore-id: 4668b2d1 Constraint: queued submissions stay promotion-leased so a prompt behind a legitimately long turn never false-fires Constraint: no release/tag/publish; PR #4669 exact-head only Rejected: clearing the replaced turn's lease at re-entry | leaves its record accepted with no bound Rejected: terminating the replaced record at re-entry | fabricates an outcome for a turn whose real terminal event may still arrive Confidence: high Scope-risk: narrow Reversibility: trivial Directive: every drained prompt correlation must hold a deadline lease; adapter parity expectations track the goal diagnostic payload Tested: bun test session-runtime.test.ts -t zero-execution (5 pass); sdk-adapter-dispositions{,-mcp,-daemon-cli,-acp}.test.ts (291+97+97+97 pass); biome+tsc clean Not-tested: full CI pending on this head
531eb7b to
6a28647
Compare
Verdict: Request changesThe acceptance-anchored lease addresses the original no- A1 / A2 — [P1] Expiry does not fence the pending ownership pathLocation:
Please make deadline expiry atomically retire/invalidate the pending correlation and reject late starts (or have A2 — [P1] Per-record leases are not refreshed for the drained batchLocation: The change creates a lease for every prompt in A3 — Security / Privacy / TrustNo concrete findings in the changed paths. A4 — Verification / Tests / CIThe added zero-execution and adapter coverage is useful, but there is no deterministic test for the two expiry boundaries above. Please add the late-start fence regression and a multi-follow-up batch with tool progress that exceeds the per-prompt lease without false failure. Also add coverage proving that a queued follow-up/busy-steer remains accepted while waiting beyond the deadline, since the implementation intentionally leases only on promotion/start. The current GitHub check set is not green yet: the PR contract bootstrap/path-validation run reports failure/cancellation while several affected-path checks remain pending. A5 — Context / Compatibility / PlatformNo concrete compatibility, persistence, generated-surface, package/install, or platform-specific findings. |
|
Both P1 findings from the review are addressed on exact head
Validation on this head: full session-runtime suite 58/58, zero-execution 8/8, sdk-adapter-dispositions + deadline-lease 297/297, biome + tsc clean. The verdict line and diff digest in the body are rebound to — |
|
Generation-5 exact head Resolves the QA generation-4 blockers on production dispatch:
Evidence on this head: production-path regressions Worktree hygiene note: untracked — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The latest head (7e0b593e) fixes the previously unreachable mid-run steering callback, correctly distinguishes ordinary in-run follow-ups from successor runs, and renews deadlines for every correlation attached to a shared run. However, the remaining ownership/terminalization paths can still associate a timed-out or misclassified request with a later, unrelated run. That is a false-owner regression in the exact area this PR is intended to close.
Findings / Required Changes
-
P1 — Retire expired pending ownership before another run can consume it.
packages/coding-agent/src/sdk/host/session-runtime.ts:2826-2836adds an own-run promotion topending, butpackages/coding-agent/src/sdk/prompt-deadline-manager.ts:86-97only finalizes the reconciliation record and clears the timer. It does not remove that pending correlation. If noagent_startoccurs before expiry, a later unrelatedagent_startcan drain the failed request and make the old client an owner of the new run. Retire the exact pending/owner correlation atomically when the deadline expires, and cover expiry → lateragent_start→ foreign abort. -
P1 — Do not treat the idle snapshot as proof that a prompt ran in its own turn.
packages/coding-agent/src/sdk/host/session-runtime.ts:1390-1439still terminalizes fromqueuedAtDispatch === false, whilepackages/coding-agent/src/session/agent-session.ts:11500-11515can yield during preflight and subsequently enqueue the prompt as steering after another run begins. The record becomes terminal before the actual dequeue callback, so the real consumption boundary cannot repair it. Return the actual delivery disposition fromsendUserMessage, or defer terminalization until the correlated queue/run boundary. -
P1 — Keep agent-initiated in-run prompts out of
pending.
Inpackages/coding-agent/src/sdk/host/session-runtime.ts:2838-2857, the new in-run callback falls back topending.push(...)when the current run has neitheractiveInvocationnordrainedInvocations(for example, a monitor/cron initiated run). There is no subsequentagent_startfor that prompt, so it remains unresolved and can later be drained into an unrelated run. Attach these correlations to an explicit active-run transition list, and only establish requester ownership when the active run is SDK-owned. -
P2 — Make batch deadline cleanup failure-safe.
packages/coding-agent/src/sdk/host/session-runtime.ts:2659-2687clears attached leases only after eachnoteTransitionsucceeds. A rejected transition can leave tail timers alive;agent_endthen prefers the headactiveInvocation, andagent_failedhas no equivalent fallback cleanup. Clear every drained correlation in terminal/finally cleanup for both terminal lifecycle events. -
P2 — Close the observable contract coverage gaps.
packages/coding-agent/test/helpers/sdk-adapter-dispositions-shared.ts:114-142now acceptsgoal.list/getsuccess without asserting the requiredenabled: false,goal: null,reason: "no_active_goal", and diagnostic message payload.packages/coding-agent/src/sdk/host/session-runtime.test.ts:2725-2752also checks onlyturn.prompt_status, not the promisedturn.resultsurface for deadline failures. Add operation-specific adapter assertions andturn.resultassertions, including the replacement-prompt case. -
P2 — Avoid a public details-payload discriminator collision.
packages/agent/src/types.ts:813-819classifies any exact{ failureKind: "argument_validation" | "execution" }object as loop-owned, althoughAgentToolResult.detailsremains arbitrary tool-owned data. A custom tool returning that otherwise valid one-field payload loses its renderer. Use a reserved, collision-resistant loop marker or out-of-band provenance. -
P3 — Update the public hook/release documentation.
The newonSteeringConsumedhook andstartsOwnRundiscriminator are not described in the affected Unreleased sections (packages/agent/CHANGELOG.md:28,packages/coding-agent/CHANGELOG.md:3). Document the hook semantics and shared-run correlation renewal behavior.
CI / Verification
- This review used the exact PR head
7e0b593e12fab13f5444c72f4a7c022454afcf5b. - The subagent verification axis did not execute PR code by design. It inspected the changed tests and available CI metadata.
- Current visible metadata is not a complete green signal: the exact-head PR contract is marked failed/needs-human, and Dev CI is still in progress. The commit's reported targeted-test results were not independently executed here.
Axis Coverage
- A1 — Intent / Policy / Contract: covered; promotion identity improvements are aligned, but the tool-details discriminator remains a compatibility concern.
- A2 — Architecture / Correctness / Failure: covered; the three P1 lifecycle/ownership paths above remain blocking.
- A3 — Security / Privacy / Trust: covered; the stale-owner paths are authorization/trust-boundary defects. No separate secret, injection, path, or privacy issue was identified.
- A4 — Verification / Tests / CI: covered from changed tests and provided CI artifacts; P2 contract-coverage gaps remain.
- A5 — Context / Compatibility / Platform: covered; no platform/generated-surface blocker found; documentation/release-note drift is P3.
|
Generation 6 exact head Fix-forward for QA findings 1-3:
Local evidence: Requesting fresh exact-head independent review at — |
|
Body digest corrected to canonical — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The normal lease/promotion path is directionally aligned with the issue, but several lifecycle paths can retain an accepted SDK correlation past its real run and later attach it to an unrelated turn. That breaks requester ownership and can make a later cross-client abort act on the wrong lifecycle.
Findings / Required Changes
-
[P1] Attach in-run promotions to agent-owned runs instead of deferring them to
pending.
packages/coding-agent/src/sdk/host/session-runtime.ts:2845-2859falls back topendingwhenstartsOwnRun: falseis reported during a monitor/cron-style run that has no SDK root (activeInvocation/drainedInvocations). The next unrelatedagent_startcan then adopt the old requester and lifecycle state, while the real turn is not represented. Track the active run independently of SDK ownership and attach the correlation to that run; only usependingwhen no lifecycle is active. -
[P1] Preserve the actual run identity for maintenance continuations.
packages/coding-agent/src/session/agent-session.ts:5909-5913reports queued messages asstartsOwnRun: trueeven for a maintenance continuation, which continues the logical agent run without a newagent_start(packages/agent/src/agent.ts:1550-1602). This leaves the correlation inpending, where it can false-expire or be adopted by a later run. Propagate the continuation's real lifecycle identity and attach it to the existing transition batch. -
[P1] Do not terminalize from an idle preflight snapshot or retain expired pending ownership.
packages/coding-agent/src/sdk/host/session-runtime.ts:1383-1435usesqueuedAtDispatchas a stale idle snapshot; a delayed preflight that is diverted to steering can therefore be terminalized asagent_endbefore its actual consumption. Separately, expiry clears the lease but leaves itspendingownership available to a later drain (:2829-2835). Base terminalization on the actual delivery/promotion disposition, and remove or skip terminal correlations atomically before a drain. -
[P2] Cover the lifecycle fences and failure cleanup.
Add deterministic regressions for expiry followed by a later foreign run/abort, idle-to-steering preflight diversion, agent-initiated in-run consumption, maintenance continuation consumption, and a transition failure that proves every drained lease is cleared. Assert the durableturn.resultfailure surface as well asprompt_status.
CI / Verification
- Reviewed changed tests and GitHub CI evidence only; no PR code, tests, builds, linters, or formatters were run for this review axis.
- Dev CI run
32168800490was still in progress:gjc-state-gates / static,runtime, andintegritysucceeded;readwas skipped; other affected jobs were pending/skipped andPR contract bootstrapfailed. - Dev CI run
32168067832was cancelled. Validate exact-head PR contractrun32168801111reportedneeds-human; per review policy, that failure is not counted.
Axis Coverage
- A1 — Intent / Policy / Contract: The accepted-prompt ownership contract is still violated on non-SDK-root lifecycle paths.
- A2 — Architecture / Correctness / Failure: Found stale ownership, continuation identity, dispatch-race, and expiry-cleanup failures.
- A3 — Security / Privacy / Trust: Applicable: stale ownership can cross SDK-client requester isolation and authorize a foreign terminal abort.
- A4 — Verification / Tests / CI: Core happy paths are covered, but lifecycle fences/error paths and completed affected-suite CI evidence are insufficient.
- A5 — Context / Compatibility / Platform: Monitor/cron and maintenance-continuation callers are incompatible with the current pending-drain model.
|
Generation 7 exact head P1 fixes for exact a1f7077 review:
Local evidence: Requesting fresh — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The deadline and promotion work addresses the accepted-with-zero-execution failure mode, but the new in-run promotion path grants a newly attached SDK connection authority over an already-running turn. A lifecycle-race fallback can also transfer that authority to a later unrelated turn. The expiry and promotion behavior needs focused regression coverage before this is safe to merge.
Findings / Required Changes
-
[P1] Preserve root-turn abort ownership for in-run promotions.
packages/coding-agent/src/sdk/host/session-runtime.ts:2868-2901adds an in-run submitter'sconnectionIdtoactivePromptOwnerHolder.connectionIdswhenstartsOwnRun === false;packages/agent/src/agent.ts:1798-1812supplies that flag for queued steering/follow-ups. A second SDK connection sharing a session can submit during another connection's streaming turn, become a co-owner, and useturn.abortto terminate the original connection's active turn. Attach the correlation for deadline tracking, but keep abort authorization scoped to the root invocation (or validate ownership per correlation / require an explicit capability). Add a regression proving an attached connection cannot abort a turn it did not start. -
[P2] Do not enqueue an in-run promotion after its run has ended.
packages/coding-agent/src/sdk/host/session-runtime.ts:2902-2904falls back topending.push(...)whenstartsOwnRun === falseandlifecycleActiveis false. A promotion racingagent_endcan therefore be drained by the next unrelatedagent_start, assigning the previous submitter ownership of a turn it did not submit. Terminalize/drop this correlation (while preserving a bounded lease as appropriate) rather than placing it inpending; add the after-agent_endregression case. -
[P1] Cover the expiry-to-next-run ownership fence.
packages/coding-agent/src/sdk/host/session-runtime.ts:2790-2805removes expired items frompending, butpackages/coding-agent/src/sdk/host/session-runtime.test.ts:2715-3250only asserts terminalization. Add a deterministic test that expires an accepted prompt, then emits a lateragent_start, and proves the expired connection has no active-turn authority while a newly submitted connection owns its turn. This protects the new stale-pending cleanup invariant from ordering regressions. -
[P2] Complete the new promotion/deadline test matrix.
packages/coding-agent/test/agent-session-promotion-identity.test.tslacks the newmaintenanceContinuation→startsOwnRun:falsepath (packages/coding-agent/src/session/agent-session.ts:5896-5910). The deadline-renewal test insession-runtime.test.ts:2895-2970also checks only an attached tail correlation andtool_execution_start; assert the head correlation andtool_execution_endpath too. Strengthentest/helpers/sdk-adapter-dispositions-shared.ts:138-152so an empty/missinggoal.list/getpage cannot vacuously pass instead of exposing the requiredno_active_goaldiagnostic. -
[P3] Keep hook evolution rolling-upgrade safe.
The new requiredpromotionparameters inpackages/agent/src/types.ts:340-346and extensiononQueuedPromoteddeclarations can cause a new handler to dereferenceundefinedwhen hosted by an older runtime or simple mock. Prefer optional parameters with guarded reads unless the public extension contract explicitly requires lockstep deployment.
CI / Verification
- Multi-axis, read-only review completed against exact head
87a8c50fde88395ac8ab0d52a555f6ed603f8899(base356633ac98). No PR code was executed locally. - Completed CI evidence includes the targeted
agent-session-promotion-identity.test.tsandagent.test.tsjobs plus state-gate/native-build jobs. - The current PR-contract/bootstrap failures are the repository's intentional
needs-humanreview gate and are excluded from this verdict. Cancelled runs are non-actionable. Several Dev CI shards remain queued/in progress; re-check them before merge.
Axis Coverage
| Axis | Result |
|---|---|
| A1 — Intent / Policy / Contract | Required API change is scoped; P3 rolling-upgrade compatibility advisory noted. |
| A2 — Architecture / Correctness / Failure | No independent blocking defect beyond the ownership/lifecycle findings above. |
| A3 — Security / Privacy / Trust | P1 intra-session abort-ownership escalation; P2 lifecycle-race ownership transfer. |
| A4 — Verification / Tests / CI | P1 expiry-to-next-run fence missing; P2 promotion/renewal/adapter diagnostic gaps. |
| A5 — Context / Compatibility / Platform | No blocking package, generated artifact, migration, or platform regression found. |
|
Exact head Fixes for snowykr @
Local: Requesting — |
|
Coordination handoff from the issue-#4668 lane (worktree diffed against current PR head P2 — tool-details discriminator collision ( +/** Reserved collision-resistant marker for the loop-owned failure envelope (#4668). */
+const LOOP_FAILURE_ENVELOPE_MARKER = "__loopFailureEnvelope";
+
export function toolFailureEnvelope(kind: ToolFailureKind): ToolFailureEnvelope {
- return { failureKind: kind };
+ return { failureKind: kind, [LOOP_FAILURE_ENVELOPE_MARKER]: true };
}
export function isToolFailureEnvelope(value: unknown): value is ToolFailureEnvelope {
if (!value || typeof value !== "object") return false;
+ if ((value as Record<string, unknown>)[LOOP_FAILURE_ENVELOPE_MARKER] !== true) return false;
const keys = Object.keys(value);
- if (keys.length !== 1 || keys[0] !== "failureKind") return false;
+ if (!keys.includes("failureKind")) return false;
const kind = (value as ToolFailureEnvelope).failureKind;
return kind === "argument_validation" || kind === "execution";
}( P3 — Unreleased changelog entries for the new hook semantics: Both are required by the open snowykr review; adding them closes the remaining P2/P3 set. — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The PR addresses the accepted-prompt deadline gap and the updated head adds useful coverage, but several lifecycle paths still detach an accepted correlation from the run that consumes it or remove its recovery state before a durable terminal transition. The agent_failed addition also leaves terminal ownership state live. These break the prompt ownership and bounded-lifetime guarantees this change is intended to establish.
Findings / Required Changes
-
[P1] Do not terminalize an idle-to-busy dispatch before its promotion identity is known
packages/coding-agent/src/sdk/host/session-runtime.ts:1402-1455
isIdle()is sampled before dispatch, while a stream can begin beforesendUserMessage()resolves. In that race the request is queued as steering, butpromotionStartsOwnRunis stillundefined, so the settlement path treats it as an own-run completion and recordsagent_endbeforeonSteeringConsumed(false)arrives. The accepted request is then terminalized and its deadline/ownership retired before it is consumed. Defer settlement until promotion disposition is known, or return the actual queue disposition from dispatch synchronously; synthesize own-run completion only for a confirmed own-run outcome. -
[P1] Treat
agent_failedas a terminal lifecycle boundary and preserve its failure cause
packages/coding-agent/src/sdk/host/session-runtime.ts:2628-2728
The new listener discards the failure event and clears leases, but cleanup oflifecycleActive, invocation batches, owner connection IDs, and publication waiters only runs foragent_end. A failed run without a subsequent end leaves stale ownership that later lifecycle activity can inherit, including abort authority; reconciliation also loses the actual failure reason. Thread the sanitized error into the failed transition and perform terminal teardown foragent_failed(or enforce and test the invariant that anagent_endalways follows it). -
[P1] Do not drop a deadline and pending ownership when expiry reconciliation fails
packages/coding-agent/src/sdk/prompt-deadline-manager.ts:69-104
onExpiredruns fromfinallyand clears the lease even if durable claim/finalize work throws. The runtime callback removes the pending ownership entry, leaving an accepted/in-flight invocation without an owner, retry, or deadline recovery path. Retire pending ownership only after durable terminal confirmation, or retain/retry the lease with an explicit bounded uncertain-outcome path. -
[P1] Report maintenance continuations as in-run consumption
packages/agent/src/agent.ts:1473-1483, 1524-1528
Direct follow-up consumption reportsstartsOwnRun: trueeven whenmaintenanceContinuationcontinues an existing logical run. That can make the SDK wait for anagent_startwhich will never be emitted, mis-correlating or falsely expiring the accepted follow-up. DerivestartsOwnRunfrom the continuation mode and reportfalsewhen it stays in the current run. -
[P2] Keep the adapter parity assertion non-vacuous for the new goal diagnostic
packages/coding-agent/test/helpers/sdk-adapter-dispositions-shared.ts:146-150
The ACP branch returns successfully whengoal.list/gethas no page, so{ ok: true }withoutenabled:false,goal:null,reason:"no_active_goal", or its diagnostic message passes CI. Define and assert ACP's translated diagnostic contract rather than accepting a missing page.
CI / Verification
- Reviewed the exact head
3a589426cc875205697adb915442880a9e7e5b9cagainst356633ac98b4a27d19b7f6d573c286791c913ff4through independent A1–A5 review lanes. No PR code was executed locally. - Exact-head Dev CI run 32191100558 passed the affected session-runtime test, promotion-identity test, and coding-agent shard 1/8.
- Virtual integration validation was still queued at review time. The PR-contract bootstrap is a designated needs-human gate and is excluded from this assessment.
Axis Coverage
| Axis | Result |
|---|---|
| A1. Intent / Policy / Contract | Required changes: promotion and continuation semantics do not consistently match their lifecycle contract. |
| A2. Architecture / Correctness / Failure | Required changes: dispatch race, failure teardown, and durable-state/lease ordering defects. |
| A3. Security / Privacy / Trust | Required changes: stale failed-run ownership can authorize a previous client against a later turn. |
| A4. Verification / Tests / CI | Affected checks pass; the ACP diagnostic assertion is vacuous and virtual integration remains queued. |
| A5. Context / Compatibility / Platform | Required changes: failed-transition history and adapter diagnostic compatibility remain incomplete. |
|
Coordination nudge from the issue-#4668 lane. The exact-head review on
Issue #4668 stays open and closes only after this PR merges to dev with merged-commit evidence. — |
|
Independent exact-head verification from the issue-#4668 lane (throwaway detached worktree at Open blockers remain the 4 P1 + 1 P2 from the exact-head review plus the handed-off envelope/changelog items. — |
…4668) Red-team review of the zero-execution bound found three gaps: - agent_start leased only the head of a drained batch, so follow-ups promoted together past the head had no deadline and could remain accepted with zero execution forever. Lease every drained prompt; acceptance-anchored leases make own-turn re-entry a no-op. - A non-empty agent_start re-entry replaces the tracked invocation without a prior agent_end only for wedged/synthetic sequences; the replaced turn's lease is now deliberately retained and documented so its record still terminalizes at its deadline instead of losing its zero-progress bound. - The goal.list/get diagnostic payload replaced resource_gone, but the shared adapter-disposition fixture still expected resource_gone; the parity expectation now intentionally asserts ok: true for the no_active_goal payload across mcp/acp/daemon-cli adapters. Lore-id: 4668b2d1 Constraint: queued submissions stay promotion-leased so a prompt behind a legitimately long turn never false-fires Constraint: no release/tag/publish; PR #4669 exact-head only Rejected: clearing the replaced turn's lease at re-entry | leaves its record accepted with no bound Rejected: terminating the replaced record at re-entry | fabricates an outcome for a turn whose real terminal event may still arrive Confidence: high Scope-risk: narrow Reversibility: trivial Directive: every drained prompt correlation must hold a deadline lease; adapter parity expectations track the goal diagnostic payload Tested: bun test session-runtime.test.ts -t zero-execution (5 pass); sdk-adapter-dispositions{,-mcp,-daemon-cli,-acp}.test.ts (291+97+97+97 pass); biome+tsc clean Not-tested: full CI pending on this head
3a58942 to
8a61c74
Compare
|
Exact-head update: head All five findings from the 2026-08-18T22:35 exact-head review are addressed at this head:
Verification at this exact head: — |
…4668) Red-team review of the zero-execution bound found three gaps: - agent_start leased only the head of a drained batch, so follow-ups promoted together past the head had no deadline and could remain accepted with zero execution forever. Lease every drained prompt; acceptance-anchored leases make own-turn re-entry a no-op. - A non-empty agent_start re-entry replaces the tracked invocation without a prior agent_end only for wedged/synthetic sequences; the replaced turn's lease is now deliberately retained and documented so its record still terminalizes at its deadline instead of losing its zero-progress bound. - The goal.list/get diagnostic payload replaced resource_gone, but the shared adapter-disposition fixture still expected resource_gone; the parity expectation now intentionally asserts ok: true for the no_active_goal payload across mcp/acp/daemon-cli adapters. Lore-id: 4668b2d1 Constraint: queued submissions stay promotion-leased so a prompt behind a legitimately long turn never false-fires Constraint: no release/tag/publish; PR #4669 exact-head only Rejected: clearing the replaced turn's lease at re-entry | leaves its record accepted with no bound Rejected: terminating the replaced record at re-entry | fabricates an outcome for a turn whose real terminal event may still arrive Confidence: high Scope-risk: narrow Reversibility: trivial Directive: every drained prompt correlation must hold a deadline lease; adapter parity expectations track the goal diagnostic payload Tested: bun test session-runtime.test.ts -t zero-execution (5 pass); sdk-adapter-dispositions{,-mcp,-daemon-cli,-acp}.test.ts (291+97+97+97 pass); biome+tsc clean Not-tested: full CI pending on this head
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 2 actionable issues, led by Renewed leases can be cleared during terminal-transition replay and Required CI gates are failing. These findings require changes before approval.
Findings / Required Changes
- [P1] Renewed leases can be cleared during terminal-transition replay.
Reference:packages/coding-agent/src/sdk/prompt-deadline-manager.ts:88-111
When a real terminal transition is pending, the replay branch awaits reconciliation and then unconditionally calls clear(); progress during that await is not generation-checked, so a renewed live prompt can lose its lease and ownership. Apply the same supersession check used by the normal finalize path before clearing. - [P1] Required CI gates are failing.
Reference:scripts/telegram-daemon-generation-manifest.json:1
The brokered CI summary reports failures for Affected path validation, its evidence producer, root-check, and exact-head PR contract. Resolve the failed gates and rerun CI before treating this change as verified; skipped virtual integration remains unverified.
CI / Verification
- Reviewed the exact remote head:
567012cd298091b9e8fb1401891780e9dc6cb42e. - CI summary: 51 passing, 6 failing, 20 pending/cancelled/skipped.
- Failing checks:
Affected path validation,Affected path validation / evidence producer,Affected path validation / root-check. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | The additive agent_failed lifecycle and promotion-disposition boundaries appear internally consistent; no concrete API or compatibility break was established. |
| A2. Architecture / Correctness / Failure | APPROVED | Concurrency and correctness changes appear internally consistent, with bounded recovery and lifecycle ownership fencing; no actionable defect was established. |
| A3. Security / Privacy / Trust | APPROVED | A3 security and privacy controls appear sound: provider error details are not exposed through the reviewed failure lifecycle or persisted terminal state. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | A4 verification is incomplete: targeted tests pass, but failing CI gates and an untested terminal-replay lease race leave observable lifecycle risk. |
| A5. Context / Compatibility / Platform | APPROVED | Integration contracts and documentation are consistent; platform-specific confidence is limited because Windows/Darwin validation was skipped. |
Limitations
- CI summary provides failing job names but no failure logs, so the root causes of those failures cannot be localized from brokered evidence.
- Virtual integration validation and several Windows/Darwin platform checks were skipped; platform behavior is therefore limited to inspected code and available passing checks.
|
Finding 1 of the
@probepark @snowykr — fresh exact-head review requested at — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 2 actionable issues, led by Gating CI failures require resolution and Make agent_failed.error required. These findings require changes before approval.
Findings / Required Changes
- [P1] Gating CI failures require resolution.
Reference:scripts/ci-dev-affected.ts:78-95
Brokered CI reports failures in affected-path validation, its evidence producer, root-check, and exact-head PR contract; inspect the corresponding job logs and fix or explicitly reconcile the failures before merging. - [P2] Make agent_failed.error required.
Reference:packages/coding-agent/src/extensibility/extensions/types.ts:651-653
AgentFailedEvent documents a guaranteed sanitized {code,message} diagnostic, but declares error optional; require the field or document and handle missing diagnostics consistently across extension and ACP consumers.
CI / Verification
- Reviewed the exact remote head:
0e12a0fda071656facafdcd1d25c5e8553c5ae52. - CI summary: 51 passing, 6 failing, 20 pending/cancelled/skipped.
- Failing checks:
Affected path validation,Affected path validation / evidence producer,Affected path validation / root-check. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | The additive agent_failed lifecycle boundary is wired across core, extensions, wire mapping, and ACP, but its public extension type weakens the promised diagnostic contract. |
| A2. Architecture / Correctness / Failure | APPROVED | Concurrency and correctness protections for lifecycle reconciliation, queue ownership, deadline expiry, and terminal races appear coherently implemented; no actionable defect was established. |
| A3. Security / Privacy / Trust | APPROVED | A3 conclusion: reviewed changes materially reduce provider-error leakage and preserve bounded diagnostic exposure; no actionable security or privacy defect was established. |
| A4. Verification / Tests / CI | CHANGES_REQUESTED | A4 verification is broadly covered by passing targeted tests, but failing gating checks and skipped integration jobs leave merge readiness and platform compatibility unestablished. |
| A5. Context / Compatibility / Platform | APPROVED | Integration and documentation behavior is coherent; platform compatibility remains only partially established because Windows/Darwin validation was skipped. |
Limitations
- CI summary does not provide failure logs or root causes for the reported failures.
- Windows and virtual integration jobs were skipped, so those platform paths remain unverified.
- Windows and Darwin platform jobs were skipped, so platform behavior was not independently validated there; affected-path and exact-head contract checks also failed.
|
Both findings from the
Exact-head evidence: acp-mapper 35/35; agent 20/20; session-runtime 79/79; deadline-manager 13/13; package + root biome/tsc clean. @probepark @snowykr — fresh exact-head review requested at — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 1 actionable issue, led by Protect agent_failed publication from listener exceptions. These findings require changes before approval.
Findings / Required Changes
- [P1] Protect agent_failed publication from listener exceptions.
Reference:packages/agent/src/agent.ts:2073-2088
The catch path emits agent_failed before requestRunTerminal, but event listeners are invoked without isolation; a throwing subscriber can abort the catch path and suppress the required agent_end terminal boundary. Wrap diagnostic publication so listener failures cannot prevent terminalization.
CI / Verification
- Reviewed the exact remote head:
c239060638208f3c6e983b8819b3b8bad589f9fe. - CI summary: 64 passing, 9 failing, 19 pending/cancelled/skipped.
- Failing checks:
Affected path validation,Affected path validation / evidence producer,Affected path validation / test:@gajae-code/coding-agent:shard-7-of-8. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | The API boundary is additive and sanitized, but an untrusted event consumer can currently prevent the documented agent_failed-to-agent_end lifecycle from completing. |
| A2. Architecture / Correctness / Failure | APPROVED | Concurrency and lifecycle correctness appear coherently addressed; no further actionable defect was established, though failed CI shards limit confidence in unverified paths. |
| A3. Security / Privacy / Trust | APPROVED | Security and privacy protections appear sound: provider error details are sanitized before persistence and external lifecycle publication. |
| A4. Verification / Tests / CI | APPROVED | A4 verification is mixed: targeted changed-area checks pass, while broad coding-agent and agent-core CI failures remain unexplained; observable regressions cannot be established without job logs. |
| A5. Context / Compatibility / Platform | APPROVED | Integration contracts and documentation are internally aligned; platform behavior remains incompletely validated because relevant CI checks were skipped or failed. |
Limitations
- CI summary reports failures for coding-agent shards 2, 5, and 7 plus agent-core, but provides no failure output to identify a source-level regression.
- Virtual integration and Windows platform jobs are skipped, so those paths are not verified.
- Affected path validation and exact-head PR contract checks failed, while virtual integration and Windows platform checks were skipped; full integration/platform validation cannot be claimed.
|
The [P1] listener isolation — Exact-head evidence: agent 21/21 (incl. hostile-listener regression); session-runtime 79/79; notifications 60/60; both package checks clean. @probepark @snowykr — fresh exact-head review requested at — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 5 actionable issues, led by Guard untrusted errorStatus access and Listener exceptions can leak raw sensitive error objects to logs. These findings require changes before approval.
Findings / Required Changes
- [P1] Guard untrusted errorStatus access.
Reference:packages/agent/src/agent.ts:2072
The catch path directly reads err?.errorStatus before emitting sanitized agent_failed and agent_end. A provider error with a throwing errorStatus getter can throw from the error handler, preventing the documented terminal lifecycle. Safely extract status inside a guarded helper and always complete failure terminalization. - [P1] Listener exceptions can leak raw sensitive error objects to logs.
Reference:packages/agent/src/agent.ts:2184-2185
console.warn logs the uncaught listener error object directly, despite the surrounding failure path promising sanitized diagnostics; log only a fixed message or sanitized error metadata. - [P1] Preserve failure-attempt correlation.
Reference:packages/coding-agent/src/extensibility/extensions/types.ts:648-653
AgentFailedEvent exposes only error, while the documented diagnostic is correlated to an attempt and AgentEvent carries scope. Add the attempt scope/correlation to this public event and propagate it through AgentSession and downstream adapters. - [P1] Provider errors can forge lifecycle failure classifiers.
Reference:packages/agent/src/agent.ts:80-94
sanitizeAgentFailure forwards provider-controlled codes whenever they appear in the runtime allowlist, includingabortedandprompt_deadline_exceeded. This can misclassify an ordinary provider failure as cancellation or deadline expiry; only runtime-authenticated classifications should be accepted, with all provider codes mapped toagent_failed. - [P1] Provider-controlled failure codes cross the runtime trust boundary.
Reference:packages/agent/src/agent.ts:71-85
sanitizeAgentFailure accepts provider-supplied codes such as aborted, local_buffer_overflow, and prompt_deadline_exceeded; provenance-gate runtime-owned classifications and map untrusted provider codes to a generic provider failure.
CI / Verification
- Reviewed the exact remote head:
1dad5f9bc07aecfe86189b380cfb54bd2892458d. - CI summary: 64 passing, 9 failing, 19 pending/cancelled/skipped.
- Failing checks:
Affected path validation,Affected path validation / evidence producer,Affected path validation / test:@gajae-code/coding-agent:shard-2-of-8. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | The additive failure event is sanitized, but its extension-facing contract omits attempt correlation needed for reliable concurrent-run compatibility. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Concurrency lifecycle ordering is mostly preserved, but provider-controlled failure codes can corrupt cancellation and deadline semantics. |
| A3. Security / Privacy / Trust | CHANGES_REQUESTED | The change improves payload sanitization but still permits provider-controlled classifiers and raw subscriber errors to cross logging or diagnostic trust boundaries. |
| A4. Verification / Tests / CI | APPROVED | A4 verification is incomplete: targeted coverage is broad, but failed checks and skipped platform/integration jobs prevent a clean regression conclusion. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Integration and documentation changes establish additive sanitized failure propagation and daemon generation compatibility; Windows platform behavior remains unverified because its CI jobs were skipped. |
Limitations
- CI summary omits failure logs, so the failing checks cannot be mapped to a specific changed path or regression.
- Windows and virtual-integration checks were skipped, leaving those platform paths unverified.
- Windows native build toolchain and Windows dev/session-path jobs were skipped, so Windows-specific behavior is not established.
|
All five findings from the
Exact-head evidence: agent 22/22 (incl. forged-classifier regression); acp-mapper 35/35; force-abort 8/8; session-runtime 79/79; both package checks and @probepark @snowykr — fresh exact-head review requested at — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 2 actionable issues, led by Failure persistence can be followed by a false successful terminal and Preserve authenticated local failure classifiers. These findings require changes before approval.
Findings / Required Changes
- [P1] Failure persistence can be followed by a false successful terminal.
Reference:packages/coding-agent/src/sdk/host/session-runtime.ts:3099-3168
When an agent_failed reconciliation write fails, the invocation is added to failedTransitions, but the subsequent agent_end transition can still succeed without the failure diagnostic and classify the record as terminal_ok. Preserve and replay the failure reason before agent_end, or prevent terminal success until the failed transition is durably recovered; apply equivalent bounded recovery for skill invocations, which have no deadline lease. - [P1] Preserve authenticated local failure classifiers.
Reference:packages/agent/src/agent.ts:2019-2024
The catch path calls sanitizeAgentFailure(err) without passing the identity-checked managedLocalErrorDiagnostic result, so local_buffer_overflow and other runtime-owned local failures are emitted as generic agent_failed instead of their documented stable code. Extract the authenticated local classifier and pass it to sanitization while continuing to reject foreign provider labels.
CI / Verification
- Reviewed the exact remote head:
751764b6dd682012916ade9383198eb55290ce43. - CI summary: 63 passing, 10 failing, 19 pending/cancelled/skipped.
- Failing checks:
Affected path validation,Affected path validation / evidence producer,Affected path validation / test:@gajae-code/coding-agent:shard-7-of-8. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | CHANGES_REQUESTED | The additive failure-event contract is broadly wired, but local runtime classifier fidelity remains a compatibility risk at the Agent API boundary. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | A concurrency flaw remains in failure-to-terminal reconciliation: persistence failure can lose the diagnostic and produce terminal_ok or an unreconciled invocation. |
| A3. Security / Privacy / Trust | APPROVED | The change adds sanitized failure diagnostics and preserves provider-error confidentiality; no concrete security or privacy vulnerability was established. |
| A4. Verification / Tests / CI | APPROVED | A4/A5 risk remains unresolved: extensive targeted coverage exists, but multiple affected CI suites fail and integration validation is skipped. |
| A5. Context / Compatibility / Platform | APPROVED | Integration and documentation contracts are consistently wired; daemon compatibility is fail-closed, but failed and skipped CI leave broad platform validation incomplete. |
Limitations
- Affected path validation reports failures in multiple coding-agent and agent-core test shards, so this lane cannot claim the full compatibility suite passes; the broker supplied no failure logs to identify whether those failures are related to this boundary.
- CI summary reports failures and skips but provides no failure logs, so passing status for all affected integration suites cannot be established.
- Windows and Darwin platform checks were skipped, so those platform paths remain unverified.
|
Both findings from the
Exact-head evidence: session-runtime 80/80 (incl. the end-reason replay regression); agent 22/22; deadline-manager 13/13; notifications 60/60; both package checks and @probepark @snowykr — fresh exact-head review requested at — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 2 actionable issues, led by Persistent skill reconciliation failure can strand accepted submissions and Unsanitized provider error persisted during recovery. These findings require changes before approval.
Findings / Required Changes
- [P1] Persistent skill reconciliation failure can strand accepted submissions.
Reference:packages/coding-agent/src/sdk/host/session-runtime.ts:1740-1795
Skill rejection retries agent_failed/agent_end only three times, then logs and returns without a deadline lease or recovery owner, leaving the durable record accepted indefinitely. Add durable uncertainty/retry ownership for skills and ensure eventual terminalization. - [P1] Unsanitized provider error persisted during recovery.
Reference:packages/coding-agent/src/sdk/host/session-runtime.ts:1451
rejectionRecoveryIntent copies error.message verbatim; this value is passed into deadline recovery and can be persisted or exposed through prompt status. Use sanitizePromptFailure(error) before constructing the recovery intent.
CI / Verification
- Reviewed the exact remote head:
83242618d3538486a4b7bd5c5e3810f22a40f91f. - CI summary: 64 passing, 10 failing, 19 pending/cancelled/skipped.
- Failing checks:
Affected path validation,Affected path validation / evidence producer,Affected path validation / test:@gajae-code/agent-core. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | The additive agent_failed lifecycle event, queued-promotion dispositions, reconciliation compatibility, ACP mapping, and daemon generation boundary are coherently represented. |
| A2. Architecture / Correctness / Failure | CHANGES_REQUESTED | Concurrency handling covers run identity, lifecycle fencing, deadline races, and queue removal, but persistent skill-write failures can still strand accepted work. |
| A3. Security / Privacy / Trust | CHANGES_REQUESTED | Provider failures are generally sanitized, but durable rejection recovery still exposes raw provider error messages. |
| A4. Verification / Tests / CI | APPROVED | A4 review confirms broad targeted test additions, but failed unit shards and skipped virtual integration leave regressions unresolved. |
| A5. Context / Compatibility / Platform | APPROVED | The integration and platform contracts appear coherently propagated and documented, but failed or skipped CI limits confidence in runtime compatibility. |
Limitations
- Affected path validation and exact-head contract CI jobs failed; virtual integration and several platform jobs were skipped, limiting runtime platform validation.
- Virtual integration validation was skipped, limiting end-to-end regression confidence.
|
Both findings from the
Exact-head evidence: session-runtime 80/80; deadline-manager 13/13; agent 22/22; both package checks and @probepark @snowykr — fresh exact-head review requested at — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 1 actionable issue, led by Use the sanitized failure shape in the wire fixture. These findings require changes before approval.
Findings / Required Changes
- [P2] Use the sanitized failure shape in the wire fixture.
Reference:packages/coding-agent/test/agent-wire/fixtures.ts:24
The agent_failed fixture supplies a raw Error despite the documented{ code, message }contract, and the cast hides this mismatch; replace it with a sanitized diagnostic so wire tests validate the public payload shape.
CI / Verification
- Reviewed the exact remote head:
ced48f214d533e38c48a96fd855c77b797712942. - CI summary: 63 passing, 11 failing, 19 pending/cancelled/skipped.
- Failing checks:
Affected path validation,Affected path validation / evidence producer,Affected path validation / test:@gajae-code/coding-agent:shard-8-of-8. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | API and compatibility boundaries appear additive and explicitly versioned; no concrete breaking boundary defect was established. |
| A2. Architecture / Correctness / Failure | APPROVED | Concurrency lifecycle handling adds bounded acceptance deadlines, promotion identity, removal terminalization, and generation-fenced reconciliation; no concrete defect was established. |
| A3. Security / Privacy / Trust | APPROVED | Security/privacy controls reviewed; no exposed provider-error details or actionable trust-boundary weakness established. |
| A4. Verification / Tests / CI | APPROVED | A4 conclusion: coverage is broad, but observable CI failures and skipped integration validation prevent establishing a clean verification result. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | Integration and documentation contracts are mostly consistent; the remaining risk is a raw Error-shaped agent_failed test fixture masking the sanitized wire contract, with full CI/platform validation incomplete. |
Limitations
- Affected path validation and several coding-agent/agent-core CI jobs failed, while virtual integration and Windows platform jobs were skipped; therefore full cross-platform integration success cannot be established.
|
The [P2] sanitized wire fixture — the Noting the review's A1–A4 axes are APPROVED with only this fixture finding plus CI-gate caveats outstanding; the CI failures cited remain the needs-human contract gate plus shard/evidence cascades documented earlier. @probepark @snowykr — fresh exact-head review requested at — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
CHANGES_REQUESTED
Summary
The five-axis review completed against the exact head and identified 2 actionable issues, led by Clarify sanitized failure payload wording and Raw persistence errors are logged. These findings require changes before approval.
Findings / Required Changes
- [P2] Clarify sanitized failure payload wording.
Reference:docs/sdk-embedding.md:216-218
The text saysagent_failedcarries the provider/local error, while the public README contract specifies only sanitized{ code, message }; revise this wording to state that only the sanitized diagnostic is exposed. - [P2] Raw persistence errors are logged.
Reference:packages/coding-agent/src/sdk/host/session-runtime.ts:3480-3505
The abandoned-submission recovery logsreasonErrorandtransitionErrorobjects without sanitization; filesystem or transport errors may contain sensitive paths or operational details. Log only a bounded sanitized classifier/message, consistent with the surrounding provider-error handling.
CI / Verification
- Reviewed the exact remote head:
fa75b5daadbee83aacc9856062cd18787770c178. - CI summary: 64 passing, 9 failing, 20 pending/cancelled/skipped.
- Failing checks:
Affected path validation,Affected path validation / evidence producer,Affected path validation / test:@gajae-code/agent-core. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | API and compatibility boundaries appear coherently extended; no concrete contract break was established from the reviewed evidence. |
| A2. Architecture / Correctness / Failure | APPROVED | A1-A5 review found no actionable correctness or concurrency issue; lifecycle reconciliation is substantially covered, with remaining confidence limited by failed CI shards. |
| A3. Security / Privacy / Trust | CHANGES_REQUESTED | Failure payloads are sanitized before public emission, but recovery logging still risks leaking raw persistence-error details. |
| A4. Verification / Tests / CI | APPROVED | Verification coverage is broad and targeted tests pass, but unresolved CI failures leave potential regressions unlocalized. |
| A5. Context / Compatibility / Platform | CHANGES_REQUESTED | A1/A2/A4/A5: lifecycle integration is consistently wired, but documentation needs clarification and CI/platform evidence does not establish full compatibility. |
Limitations
- CI summary identifies failing jobs but provides no assertion or log details, so the failures cannot be attributed to a specific changed path.
- Affected path validation failed, including agent-core and coding-agent shards; virtual integration, Windows, and Darwin-arm64 smoke checks were skipped, so those platform/integration paths are not established as passing.
|
Both findings from the
Exact-head evidence: session-runtime 80/80; docs-index gate green; package check clean. @probepark @snowykr — fresh exact-head review requested at — |
snowykr
left a comment
snowykr
left a comment
There was a problem hiding this comment.
Verdict
APPROVED
Summary
The five-axis review completed against the exact head without actionable P0-P2 findings. The reviewed API, correctness, security, verification, and compatibility boundaries are approved.
Findings / Required Changes
None.
CI / Verification
- Reviewed the exact remote head:
dd59add32581b5dc9f5f38ad124fa92577beba05. - CI summary: 64 passing, 9 failing, 19 pending/cancelled/skipped.
- Failing checks:
Affected path validation,Affected path validation / evidence producer,Affected path validation / test:@gajae-code/coding-agent:shard-7-of-8. - Repository policy permits review before all gating checks pass; the current non-passing checks are recorded above and do not establish that checks passed.
Axis Coverage
| Axis | Verdict | Coverage |
|---|---|---|
| A1. Intent / Policy / Contract | APPROVED | Compatibility boundaries appear additive and explicitly versioned; no concrete API break or cross-platform boundary defect was established. |
| A2. Architecture / Correctness / Failure | APPROVED | Concurrency lifecycle changes are covered by targeted tests and the reviewed code preserves bounded ownership, terminalization, and stale-event fencing. |
| A3. Security / Privacy / Trust | APPROVED | Security and privacy controls reviewed; provider error text is sanitized before state, logs, and exposed lifecycle diagnostics. |
| A4. Verification / Tests / CI | APPROVED | A4 conclusion: verification coverage is broad, but failed CI jobs leave regression status unresolved without failure details. |
| A5. Context / Compatibility / Platform | APPROVED | Integration, documentation, and daemon/platform compatibility behavior are coherently updated; no concrete compatibility defect was established. |
Limitations
- CI summary reports failed affected-path and PR-contract jobs without failure details, so their specific causes cannot be assessed; Windows and Darwin platform jobs were skipped.
|
Recovery update: exact remote head is The current-dev integration uncovered and corrected two stale expectations: managed-attempt failure surfaces now assert the PR’s sanitized public diagnostic while retaining structured local-failure authority checks, and the streaming-edit abort test now verifies the #4840-injected settings argument. Focused lifecycle and contract suites, type checks, native build, production SDK host isolation, state-writer checks, and virtual-integration canaries pass. Fresh exact-head review is requested; no release mutation occurred. — |
|
Exact-head repair: — |
|
Architect review blockers repaired at the new exact head: ACP and CLI tailing now keep sanitized — |
Promoted prompts could remain accepted forever when execution never began. Lease and reconcile prompts across promotion, active-run attachment, and terminal recovery so every accepted record reaches a bounded terminal state. Lore-id: pr4669-recovery-v5 Constraint: preserve current dev #4840/#4841 behavior Confidence: high Scope-risk: high Reversibility: revert-commit Tested: focused SDK lifecycle and contract suites; coding-agent and agent checks; native build; state writer and virtual integration canaries
The reconciled SDK lifecycle fix sanitizes agent failure surfaces. Update current-dev managed-attempt coverage to assert the stable public message and retain authority checks through structured diagnostics. Lore-id: pr4669-recovery-v5-integration Constraint: preserve #4840/#4841 structured local-failure behavior Confidence: high Scope-risk: low Reversibility: revert-commit Tested: managed attempt transaction, agent, and force-abort suites
Current dev injects session settings into auto-generated-file checks. Bind the abort regression to that required third argument while retaining the generated-file and cancellation assertions. Lore-id: pr4669-recovery-v5-current-dev Constraint: preserve #4840 session settings propagation Confidence: high Scope-risk: low Reversibility: revert-commit Tested: streaming edit abort suite
Persisted uncertainty recovery records lost their in-memory lease on SDK runtime restart. Reconstruct the lease from the original acceptance timestamp so the hard maximum remains bounded and expiration resumes. Lore-id: pr4669-restart-recovery Constraint: never reset an accepted prompt maximum runtime during recovery Confidence: high Scope-risk: high Reversibility: revert-commit Tested: sdk prompt deadline manager regression; coding-agent check
Diagnostic agent_failed events must not settle ownership before agent_end. Keep ACP and CLI tailing nonterminal, retire completed lifecycle state, terminalize cancelled queued follow-ups, and register deadline-manager coverage. Lore-id: pr4669-lifecycle-consumers Constraint: agent_end is the sole terminal boundary Confidence: high Scope-risk: high Reversibility: revert-commit Tested: focused ACP/CLI-adjacent, lifecycle, deadline, and affected-selector suites; coding-agent check
Diagnostic agent_failed frames no longer settle ACP prompts. Model failure fixtures as agent_failed followed by the authoritative agent_end boundary. Lore-id: pr4669-acp-lifecycle Confidence: high Scope-risk: low Reversibility: revert-commit Tested: sdk ACP prompt terminal suite
|
Reconciled PR #4669 onto current — |
|
Reproduced the current affected-path failure on both exact rebased PR head — |
|
Exact-head contract revalidation requested after the current-dev rebase. The PR body carries the rebased head/base and regenerated canonical digest; this comment is solely the trusted validator trigger. — |
|
Updated PR-contract evidence to the validator-derived exact three-dot digest for current head/base. Revalidation is requested; the prior digest failure is resolved by this replacement evidence. — |
3 participants
Fixes #4668.
Bound accepted SDK prompts that never reach execution: lease every promoted prompt at the promotion boundary and attach in-run consumption to the active run's renewal, so zero-activity accepted records terminalize with
prompt_deadline_exceededand long shared runs cannot false-fire.Current exact head:
7f0d7d3176e6c7c3439f56cfc3dbae0728d1c366, base8a41a1db9d7d81b4a5c30db4d6ac71fc66572b65(currentdev), canonical three-dot digest08d648e6e7aa70255789b46632d6cab2b22c8467faa5d2c5d2f6ee760e5d2ab8. Additionally fixes the eleventh snowykr five-axis review atfa75b5daa(sanitized-diagnostic doc wording; sanitized recovery logging). Additionally fixes the tenth snowykr five-axis review atced48f214(sanitized failure shape in the agent-wire fixture). Additionally fixes the ninth snowykr five-axis review at83242618d(never-park skill recovery; sanitized rejection recovery intent). Additionally fixes the eighth snowykr five-axis review at751764b6d(compound reason replay at agent_end after a failed agent_failed write; authenticated local failure classifiers). Additionally fixes the seventh snowykr five-axis review at1dad5f9bc(guarded errorStatus access, sanitized listener logging, attempt-scope correlation on AgentFailedEvent, provenance-gated lifecycle failure classifiers). Additionally fixes the sixth snowykr five-axis review atc23906063(listener isolation so a throwing subscriber can never suppress the agent_failed → agent_end terminal boundary). Additionally addresses the fifth snowykr five-axis review at0e12a0fda(required AgentFailedEvent diagnostic; root-check unblocked by excluding the dev-emitted ai declaration from biome). Additionally fixes the fourth snowykr five-axis review at567012cd2finding 1 (supersession check on the terminal-transition replay before clearing). Additionally fixes the third snowykr five-axis review atec56b1a7f(attached-correlation agent_failed diagnostics, closed runtime failure-code allowlist, ACP diagnostic metadata fields, continuation-path diagnostic emission, sanitized recovery logging). Additionally fixes the second snowykr five-axis review atda99f12a4(five-argument finalizeOutcome normalization, sanitized terminal state/history, live recovery timer after uncertainty-write exhaustion, typed AgentFailureDiagnostic contract). Additionally fixes the snowykr five-axis review at734461dc8(finalizeOutcome positional recordError/isCurrent compatibility, sanitized exhaustion logging, README sanitized-contract documentation, changelog daemon-generation alignment). Additionally fixes theb1fcde2creview P1: kind-aware rejection recovery so an acceptedskill.invokethat rejects after a transient persistence failure gets a bounded durable retry owner (skills have no deadline lease) instead of stranding accepted forever. Reconciled over dev's advances (fd700948 → f991c501 → 235aee515 → 3cda2e4c): every daemon-generation conflict resolved through the canonical guard repair (now 174 at serving epoch 88, strictly higher than every parent) and the embedded docs index regenerated. All review blockers across generations are fixed at this head with regressions: the five from44048a349(abort-purge hook mismatch, display-index misalignment, lease cleared before durable terminalization, stale activeInvocation renewal, in-run race drop) and the three HIGH from5b2a2f313(clearQueue deferred-SDK-follow-up dispositions, identity-based removal across live+deferred stores, compound failure+terminal recovery that never terminalizes an abandoned prompt as terminal_ok).Reconciled onto current dev and hardened by an independent adversarial review of the reconciled head. The review found and this head fixes every remaining accepted-forever/false-fire strand:
agent_failedframe; they now chainagent_endso the record terminalizes asfailed(regression: accepted-then-failed now reports a terminal failed status).queue.message.remove, queue editing,clearQueue, abort purge) never fired their promotion hook; the hook now fires with a removal disposition and the row terminalizes as a boundedcancelledfailure (regression at both the session and runtime boundary).prompt_deadline_exceeded; the divert disposition retires the acceptance lease and pending entry, and the real consumption boundary re-leases (regression: no false fire past the lease while queued).deadlineRecoveryPendingrow unbounded; recovery now re-arms a bounded lease preserving themaxMscap (deadline-manager regressions updated).agent_endupgrades dropped the supersededprompt_deadline_exceedederror soterminal_okno longer surfaces a contradictory payload.Local exact-head evidence: session-runtime 74/74; deadline-manager 13/13; promotion-identity 5/5; agent-session-concurrent + deferred-shell + managed-publication 42/42; dispositions 582/582; agent 27/27 (focused) and 813/814 package-wide (single failure pre-reproduced on pristine dev fd70094 under ambient shell
OPENAI_BASE_URL); reconciliation/terminal/ACP/notifications 124/124; ci-dev-affected 102/102; telegram guard 75/75; state-writers exit 0; docs-index byte-identical gate 5/5;bun run buildgreen; coding-agent + agent biome/tsc clean. Two non-PR failures classified against pristine dev fd70094:sdk-downgrade-rollbackexpectsbun@1.3.14(runtime pin moved to 1.4.0 in #4795 without updating the fixture), andverify-pr-verdict.test.tspins the same stale1.3.14; the full-packagebun --cwd=packages/coding-agent testcrashes in Bun 1.4.0 itself on both branch and pristine dev, so the affected CI matrix remains the authoritative full-suite surface.Risk classification
low-risk— ordinary fix/maintenance.regression-risk— material regression risk requiring independent review.high-risk— destructive lifecycle/architecture or security/trust-boundary change requiring independent review.gajae.pr-review-verdict.v1 needs-human sha256:08d648e6e7aa70255789b46632d6cab2b22c8467faa5d2c5d2f6ee760e5d2ab8 reviewer:human reviewer-id:probepark evidence:fresh-exact-head-review-required-after-rebase-and-adversarial-review-fixes
Recovery reconciliation at
7f0d7d3176e6c7c3439f56cfc3dbae0728d1c366: semantically transplanted the canonical PR delta onto current dev8a41a1db9d7d81b4a5c30db4d6ac71fc66572b65, retained current-dev #4840/#4841 settings and structured-diagnostic contracts, and replaced stale local validation evidence. Focused lifecycle 140/140, SDK/agent contract 79 pass + 3 skip, managed-attempt transaction 90/90, streaming edit abort 18/18, package checks, native build, production SDK host isolation, state-writer gates, virtual-integration canaries, and affected-path coverage were rerun. The fresh affected-path run is blocked only by the active Ultragoal runtime intercepting headless ask-tool fixtures; the same fixture runs under clean CI session state.Restart-recovery blocker fixed at
7f0d7d3176e6c7c3439f56cfc3dbae0728d1c366: persisteddeadlineRecoveryPendingrecords re-arm an acceptance-anchored lease during runtime hydration, preserving the original hard cap. Added regression coverage for an already-expired recovered lease.