[Bug]: Approval event can precede broker registration

[dennisonbertram]

Work type

Bug / regression

Observed behavior

TestE2E_ToolCallApprovalRoundTrip intermittently receives the live SSE tool.approval_required event and immediately POSTs /v1/runs/{id}/approve, but the API returns 404 (ErrNoPendingApproval). This occurred under the authoritative full race regression while promoting PR #1059. Repeated isolated runs often pass, which is consistent with a scheduling race rather than a deterministic endpoint failure.

Expected behavior

Publication of tool.approval_required must be a readiness guarantee: once any API, TUI, or native GUI client can observe the event, the approval is already durably/in-memory registered and an immediate approve or deny request cannot return ErrNoPendingApproval. Timeout, cancellation, duplicate resolution, and checkpoint-backed behavior must remain correct.

Reproduction

Preconditions: run a tool call under approval: all with a live SSE consumer and an in-memory approval broker.

Observed command: DEVELOPER_DIR=/Library/Developer/CommandLineTools TMPDIR=/private/tmp GOCACHE=<isolated> ./scripts/test-regression.sh; race stage failed at TestE2E_ToolCallApprovalRoundTrip with POST approve: expected 200, got 404.

Small deterministic red: add a broker seam that blocks before pending registration, deliver the emitted event to a client, and prove the immediate approve currently returns ErrNoPendingApproval/404. Also cover deny and the checkpoint-backed broker.

User and operational impact

The harness can show an actionable approval prompt in API/TUI/native GUI and then reject the user's immediate action. This can strand or time out runs and makes conversation-forward UI approval unreliable. No data corruption is known; retry is timing-dependent and is not an acceptable workaround.

Suspected seam and search evidence

Owning seam: internal/harness/runner_step_engine.go emits EventToolApprovalRequired before calling ApprovalBroker.Ask. Both InMemoryApprovalBroker.Ask in internal/harness/approval_broker.go and checkpointApprovalBroker.Ask in internal/harness/checkpoint_brokers.go register pending state only after Ask begins. HTTP handlers in internal/server/http_runs.go map ErrNoPendingApproval to 404. Searches covered ApprovalBroker, all ApprovalRequest constructors, approval event consumers, checkpoint brokers, HTTP approve/deny handlers, runner approval tests, and the E2E SSE path.

Current hypothesis: event publication races broker registration. The API handler and test are behaving consistently with the current contract; retrying or weakening the assertion would hide the product bug.

Blast-radius impact map

• Callers and data flow: runner tool-call approval and plan-exit approval paths; both broker implementations.
• Config/env/defaults: none; no configuration change expected.
• API/CLI/wire formats/tools: approve/deny endpoints and SSE event readiness semantics; event schema should remain compatible.
• Persistence/schema/cache: checkpoint records are affected behaviorally; no schema change expected.
• Concurrency/lifecycle/cancellation/retries: primary risk surface; registration, event publication, waiting, timeout, cancellation, and duplicate resolution ordering.
• Security/auth/permissions/privacy: approval gates must not execute without approval; authentication behavior is unchanged.
• TUI/web/macOS/other clients: all clients that react immediately to tool.approval_required.
• Provider/model/tool catalog: ordinary tool approval and plan-exit approval; no provider schema change.
• Deployment/observability/runbooks: add regression evidence and engineering-log symptom/cause/fix.
• Compatibility: preserve ApprovalBroker behavior for existing brokers or migrate every implementation/caller atomically.
• Existing tests/fixtures: approval broker, checkpoint broker, runner approval, plan mode, server handlers, E2E.
• Documentation: active plan/impact map/log indexes as required by repository process.

Regression test first

Add deterministic tests proving pending state exists before the approval-required event is externally observable for in-memory and checkpoint-backed brokers, plus an HTTP/SSE regression where immediate approve and immediate deny return 200. The red must fail because the event arrives before the broker registration barrier is released, not because of sleeps or load. Preserve the existing E2E round trip and run it under race.

Fix boundaries

In scope: repair the runner/broker contract so registration happens-before event publication, preserve blocking wait/timeout/cancellation semantics, update every implementation/caller, and add permanent regressions.

Out of scope: UI redesign, retrying 404 in clients, weakening the E2E assertion, unrelated TUI PR #1059 behavior, or changing approval policy semantics.

Repair the existing approval abstraction; do not add a parallel approval mechanism.

Diagnostic and observability evidence

Record the exact red test, event/API ordering, status code, targeted normal/race results, full regression, and a live SSE approve/deny canary. No sensitive data is involved.

Verification plan

1. Deterministic red for event-before-registration.
2. Targeted approval broker, checkpoint broker, runner approval, plan mode, server, and E2E tests.
3. Targeted tests with -race and repetition.
4. ./scripts/test-regression.sh with isolated Go cache and CommandLineTools.
5. Hosted fast and race workflows on the exact PR head.
6. Live harnessd SSE flow: observe prompt, approve immediately, conversation completes; repeat for deny.
7. Rebase dependent PR fix(tui): render terminal assistant messages #1059 on the repaired main and repeat its gates.

Rollout and rollback

Merge as an isolated baseline bug PR before dependent UI/TUI promotions. Monitor hosted regression and live approval responses. Roll back the single repair commit if approval registration, cancellation, or timeout regresses; no data migration or repair is expected.

Documentation and handoff

Update the active plan and impact map, docs/logs/engineering-log.md with symptom/cause/fix, relevant observational/system/long-term logs, and folder indexes required by documentation maintenance.

Definition of done

• Deterministic reproduction recorded and red before the fix
• Pending approval is registered before the event is observable
• Immediate approve and deny work for in-memory and checkpoint-backed paths
• Timeout, cancellation, duplicate resolution, plan-exit, and security semantics remain green
• Targeted normal/race/repetition and full regression pass
• Hosted exact-head fast/race checks pass
• Live harness/API SSE conversation proof passes
• Engineering log, plan, impact map, indexes, rollout, and rollback are current

Process acknowledgement

I will preserve the failing regression test, update the issue if the root cause changes, and record any newly discovered bug as its own issue.