From 8df89d8e3334b75a47b8c358990a69a3cfa13a49 Mon Sep 17 00:00:00 2001 From: Tianyi Cui <53024+tianyicui@users.noreply.github.com> Date: Tue, 30 Jun 2026 12:19:18 +0800 Subject: [PATCH] =?UTF-8?q?fix(events):=20address=20Codex=20review=20?= =?UTF-8?q?=E2=80=94=20balance=20step=20on=20step/start-listener=20throw,?= =?UTF-8?q?=20restore=20/goal=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex review of PR-A found three blockers: - A throwing step/start session-event listener left an unbalanced log (turn/start → step/start → turn/end with no step/end), which the invariants oracle rejects — masked because that rejection was itself contained as a throwing turn/end listener. Fix the root cause in the loop: mark the step open BEFORE appending step/start (Session.append pushes before notifying), so the outer catch's closeStep() appends the balancing step/end. The test now asserts the balanced outcome (stepEnd:1, step/end before turn/end); proven load-bearing (revert the reorder → the test goes red with stepEnd:0). - Reintroduce the /goal-pattern guard deleted in the prior commit, migrated to a step/end session-event listener (the surviving step-boundary hook point), with a no-tools first step so it exercises the hasSteering continuation override. - Update packages/core/agent/README.md: step boundaries are no longer agent/* emits. --- packages/core/agent-loop/src/loop.ts | 7 ++- .../agent-loop/tests/review-fixes.spec.ts | 57 +++++++++++++++---- packages/core/agent/README.md | 5 +- 3 files changed, 54 insertions(+), 15 deletions(-) diff --git a/packages/core/agent-loop/src/loop.ts b/packages/core/agent-loop/src/loop.ts index 28af01a027..dad1ed9320 100644 --- a/packages/core/agent-loop/src/loop.ts +++ b/packages/core/agent-loop/src/loop.ts @@ -382,8 +382,13 @@ async function runTurn(ctx: Context, agent: ReactLoopAgent, handle: LoopHandle, // turn-start listeners on the first step) joins before the request. drainSteering(ctx, agent, turn) - session.append('step/start', { turn, step }) + // Mark the step open BEFORE the append: Session.append pushes the event + // to the log before notifying session/event listeners, so a THROWING + // step/start listener leaves step/start in the log. Setting stepOpen first + // means the outer catch's closeStep() then appends the balancing step/end + // (turn stays enclosed) instead of stranding an open step under turn/end. stepOpen = true + session.append('step/start', { turn, step }) const abort = new AbortController() handle.setAbort(abort) diff --git a/packages/core/agent-loop/tests/review-fixes.spec.ts b/packages/core/agent-loop/tests/review-fixes.spec.ts index 7ee4923ce8..28d5134cf8 100644 --- a/packages/core/agent-loop/tests/review-fixes.spec.ts +++ b/packages/core/agent-loop/tests/review-fixes.spec.ts @@ -169,6 +169,35 @@ describe('HIGH: steering from late extension points is never stranded', () => { expect(JSON.stringify(adapter.requests[1]!.messages)).toContain('one more thing') }) + it('steer() from a step/end session-event listener reaches the next request (/goal pattern)', async () => { + // The /goal pattern steers from a step boundary so the model addresses a + // standing goal before stopping. Step boundaries have no agent/* mirror, so + // the surviving hook point is the durable step/end session event. With a + // no-tools first step the default continuation is stop; the steering queued + // here must force the hasSteering override and reach the next request. + const adapter = new MockAdapter([ + textResponse('no tools, would stop'), + textResponse('after goal reminder'), + ]) + const ctx = await harness(adapter) + const agent = ctx.agentLoop.create(AgentId('a1'), { model: 'mock' }) + + let steeredOnce = false + ctx.on('session/event', (subject, event) => { + if (subject !== agent.session || event.type !== 'step/end' || steeredOnce) return + steeredOnce = true + agent.steer([{ type: 'text', text: 'goal reminder from step/end' }]) + }) + + send(agent, 'go') + await waitForIdle(ctx, agent) + + // steering from the step/end listener forced a second step (hasSteering + // override) and reached the next model request. + expect(adapter.requests).toHaveLength(2) + expect(JSON.stringify(adapter.requests[1]!.messages)).toContain('goal reminder from step/end') + }) + it('steer() from an agent/turn-end listener becomes a queued message for the next turn', async () => { const adapter = new MockAdapter([textResponse('turn 1'), textResponse('turn 2')]) const ctx = await harness(adapter) @@ -593,18 +622,19 @@ describe('P1-5: a started turn (and any open step) is always closed on a boundar expect(adapter.requests).toHaveLength(0) }) - it('a throwing step/start session-event listener fails the turn balanced (no step stranded open)', async () => { + it('a throwing step/start session-event listener closes the open step then the turn (step/end before turn/end)', async () => { const adapter = new MockAdapter([textResponse('never reached')]) const ctx = await balancedHarness(adapter) const agent = ctx.agentLoop.create(AgentId('a-stepstart'), { model: 'mock' }) // Step boundaries have no agent/* mirror; a throwing step/start session-event - // listener is the surviving step-boundary-listener failure. The throw fires - // INSIDE session.append('step/start') — before the loop marks the step open — - // so the loop never had an open step to close (no step/end is owed). The - // throw drives the outer catch, which fails the turn balanced. The invariants - // oracle (balancedHarness) rejects any imbalance, so a green run proves the - // turn/start..turn/end nesting holds with a lone step/start and no step/end. + // listener is the surviving step-boundary-listener failure. The loop marks + // the step open BEFORE appending step/start (Session.append pushes before + // notifying, so a post-push listener throw still leaves stepOpen=true), so + // the outer catch's closeStep() appends the balancing step/end — the turn + // stays enclosed. The invariants oracle (balancedHarness) rejects any + // imbalance, so a green run proves turn/start → step/start → step/end → + // turn/end nesting holds. let threw = false ctx.on('session/event', (_s, event) => { if (event.type === 'step/start' && !threw) { threw = true; throw new Error('boom step-start') } @@ -615,13 +645,16 @@ describe('P1-5: a started turn (and any open step) is always closed on a boundar send(agent, 'go') await waitForIdle(ctx, agent) + const e = [...agent.session.events] const c = boundaryCounts(agent) - // step/start was appended (Session.append pushes before notifying), but the - // listener throw pre-empted the loop marking the step open, so no step/end is - // owed; the turn still closes exactly once with an error, balanced. - expect(c).toMatchObject({ turnStart: 1, turnEnd: 1, stepStart: 1, stepEnd: 0, errors: 1 }) + expect(c).toMatchObject({ turnStart: 1, turnEnd: 1, stepStart: 1, stepEnd: 1, errors: 1 }) expect(errors.map(x => x.message)).toEqual(['boom step-start']) - expect(c.lastTurnEnd?.type === 'turn/end' && c.lastTurnEnd.data.reason.kind).toBe('error') + // step/end precedes turn/end (the invariants oracle would reject + // turn/end-while-step-open, but assert the order explicitly too). + const stepEndIdx = e.findIndex(x => x.type === 'step/end') + const turnEndIdx = e.findIndex(x => x.type === 'turn/end') + expect(stepEndIdx).toBeGreaterThanOrEqual(0) + expect(stepEndIdx).toBeLessThan(turnEndIdx) }) it('a throwing agent/error listener during a step-error path still balances the turn, loop survives', async () => { diff --git a/packages/core/agent/README.md b/packages/core/agent/README.md index d0ec0ee614..1d1b839da3 100644 --- a/packages/core/agent/README.md +++ b/packages/core/agent/README.md @@ -32,10 +32,11 @@ The full `agent/*` event taxonomy is declared via declaration merging in `dsh-ag - `agent/status` — idle / running / disposed transition - `agent/queued` — message entered inbox (source-resolved, steering flag) -#### Turn/step boundaries (emit) +#### Turn boundaries (emit) - `agent/turn-start`, `agent/turn-end` (carries `TurnEndReason`) -- `agent/step-start`, `agent/step-end` + +Step boundaries are NOT mirrored as `agent/*` emits: a consumer that needs per-step boundaries reads the durable `step/start`/`step/end` session events (the session log is the live boundary feed). The turn boundaries stay as `agent/*` emits because the stdio UI needs the `Agent` handle (`agent.id`) at the boundary, which the session event does not carry. See [the event-domain-semantics RFC](../../../docs/rfc/implemented/architecture/2026-06-30-event-domain-semantics.md). #### Interception seams (waterfall)