coop: make the agent reliably wait for developer confirmation - #1862
coop: make the agent reliably wait for developer confirmation#1862tomelm wants to merge 11 commits into
Conversation
| return outputStopHookDecision(out, stopHookDecision{}) | ||
| } | ||
|
|
||
| resume, err := resumer.Resume(session.ID) |
There was a problem hiding this comment.
Could we store if the developer selects Finish? Right now the agent runs stripe coop stop, but this hook then checks the completed session, receives another next-action command. and prevents the agent from existing.
There was a problem hiding this comment.
Good catch — reproduced exactly as you describe, and fixed in bfa8070e.
The gap was that nothing recorded the Finish choice. Every other next action reports back through --completed=<id> when its follow-up finishes; Finish has no follow-up, so consuming the selection is the only chance to record it. Without that the session sat as "completed with a next action still owed" forever, Resume kept answering with the next-action command, and the hook kept blocking — so the agent could never exit and the suggestions would reappear for the developer.
Two changes:
consumeSelectionrecordsdone(coop.FinishActionID) intoNextSteps.Completedwhen that is what the developer picked.Resumereturns nonextfor a finished session, which is already the hook's "nothing to do" signal, so it allows the stop.
Verified end to end against a real session:
1. next-action -> next = stripe coop stop --session=coop_06c23062
2. coop stop -> state = completed
3. stop-hook -> {} # allow, agent exits
Step 3 previously returned decision: "block" with another next-action command.
The case that must not regress — a completed session where the developer has not chosen Finish — still blocks with next-action. Both directions are covered by TestResumeReportsNothingToRunAfterDeveloperFinishes, TestResumeStillOffersNextActionBeforeFinish, and TestRunRecordsTheFinishSelection.
Note the branch has been rebased onto the rewritten coop (it had been rewritten onto newer master, which was making this PR show a 102-file diff). The fix above was re-verified after that rebase, since it landed on a rewritten resume.go/nextaction.go.
Selecting Finish hands the agent "stripe coop stop", which completes the session. Resume answered any completed session with the next-action command, so the stop hook then held the turn open and handed back next-action again — the agent could never exit, and the developer would see the post-completion suggestions reappear. Nothing recorded the Finish choice. Other next actions report back through --completed when their follow-up finishes; Finish has no follow-up, so consuming the selection is the only chance to record it. It is now written to NextSteps.Completed there, and Resume treats a finished session as having nothing left to run. Reported by @kwu-stripe on stripe#1862. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The tmux resume path sent literal keystrokes into the agent's TUI after a review decision — a prompt plus Enter, submitted as a turn on the agent's behalf. That only works when Co-op launched the agent under tmux, and driving another program's input is not a mechanism we want to depend on. Removed: tmuxAgentResumer, the @stripe_coop_agent pane tagging that existed only so the resumer could find the pane, and the TUI ReviewDecisionNotifier wiring. Panes are split plainly again. "stripe coop agent resume" is kept. It is read-only, types nothing, and remains useful as a manual recovery command when an agent loses track of the lifecycle. Review decisions now reach the agent solely through its own await-review call, which the following commits make survive an agent harness's command timeout. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two paths could silently lose a developer decision that landed while the agent was between commands. Both are reachable today and become routine once await-review returns on an interval. await-review: requesting changes moves the node out of review and back to active with a rejection note. AwaitReview only special-cased the in-review state, so the next call fell through to alreadyMovedResponse, whose next points at the following *pending* task. The agent skipped the rejected work and never saw the feedback. It now reports the rejection, reusing rejectedStepResponse. The check requires a rejection note so a step with one task in review and another legitimately active is not misread as a rejection. next-action: ShowSuggestions unconditionally clears NextSteps.Selected, so a choice the developer made while the previous invocation was exiting was erased by the next one and the agent waited on a selection that no longer existed. Run now consumes a pending selection before republishing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
await-review blocked for five minutes and asked agents to allow a six-minute shell timeout. That only holds if the model actually sets one. Claude Code's Bash tool defaults to 120s and its override env vars have open bugs, so a model that does not set a timeout has the command killed at two minutes — before the internal deadline — and receives a raw harness string with no ok and no next instead of structured JSON. Cheap models respond by assuming the step passed or by stopping to ask a question in a pane the developer is not watching. Each call now waits at most AwaitTimeout (45s, under the harness default) and returns state "waiting" with advance_allowed false and next repeating the exact command just run. The developer is not rushed: review state lives in the session file, so the total wait stays unbounded across calls. AwaitHarnessTimeout drops to 90s and is still advertised as wait_timeout_seconds. advance_allowed becomes the single field an agent branches on, and the waiting message deliberately avoids failure vocabulary — a response reading as an error is what makes models abandon the loop. The heartbeat is now kept while waiting and cleared on every other outcome. Clearing it each interval would make the TUI report the agent idle during the gap between calls on any review longer than two minutes. A keep-alive line goes to stderr every ten seconds so the wait is never a silent process; stdout stays a single JSON document. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
next-action blocked for ten minutes and then returned ErrSelectionTimeout, which reached outputCoopError and left the agent with JSON on stderr and a non-zero exit. An agent that sees a non-zero exit treats the session as broken and gives up — exactly wrong when the developer is merely still deciding. This was the original bug in its worst form, and the review-wait work left it untouched. waitForSelection now waits one NextActionInterval (45s, matching await-review's reasoning about harness defaults) and reports an unmade choice as an empty selection rather than an error. Run turns that into a waiting response carrying state "waiting", advance_allowed false, and a next that repeats the invoked command, so the command exits zero and the agent knows to run it again. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Returning await-review on an interval removes the hard failure, but a model can still decide to stop mid-session and the CLI has no way to force it back. A Stop hook does: it fires exactly when the agent tries to end its turn, which is precisely when it has drifted. The hook asks "stripe coop agent resume" what the session needs rather than recomputing the lifecycle. Resume already handles aborted and completed sessions, rejected work, steps ready for review, and the next pending task, and its empty next is a ready-made "nothing to do" signal. When it yields an actionable command the hook blocks and hands that exact command back, so the agent gets the real next step instead of an instruction to go look one up. This also gives resume a purpose again now that nothing types into the agent pane: it is the shared lifecycle query behind both the hook and manual recovery. Injection is via launch flags only — claude --settings and codex -c — so nothing is written into the user's repo and there is nothing to clean up. Agents Co-op did not launch get no hook and rely on the interval alone. Codex gates hooks behind an interactive trust prompt, so its hook stays inert until the developer accepts it; that is noted in the code and docs. Blocking forever would burn tokens whenever the developer walks away, so the hook stops blocking after three consecutive blocks with no session progress and lets the turn end, leaving the TUI's idle state to prompt a rejoin. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adversarial review of the previous commit turned up several ways the hook could burn tokens indefinitely or act on the wrong session. The block budget was keyed on the session version, but the very command the hook orders can bump it: next-action republished its suggestions on every call, so the counter reset each round and the escape hatch could never fire. Verified before the fix — eight consecutive Stop events all reported block/1. The budget now keys on the pending command, and ShowSuggestions skips a no-op republish so the version stops churning at all. Releasing also cleared the counter, so the next Stop event started from zero and blocked three more times; the agent could never actually stop. The exhausted budget is now persisted until the lifecycle moves on. A failed counter write was swallowed, which left the loop unbounded with no release — the opposite of the fail-open the comments claimed. It now allows the stop. The discovery-mode fallback resolved "latest active session" from a machine-global directory. With two worktrees running Co-op, or one abandoned session from an earlier run, the hook could hand agent A a command targeting session B and mutate it. Sessions do not record which directory they belong to, so the fallback now requires exactly one active session, recently updated, and otherwise fails open. Codex auto-approve replaced the injected hook flag instead of appending to it, so the mode most sessions use launched with no Stop hook at all. Both launcher paths now have auto-approve coverage. Also: drain the harness event payload before any early return, register the command's error handling like its siblings, address the tmux pane by id rather than a hardcoded index that breaks under pane-base-index, and clean up sidecar files on session delete. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three defects in the lifecycle responses, all reachable now that await returns every 45s instead of every five minutes. The rejection check keyed on RejectionNote alone, but the note survives until a task reaches Done — it is not cleared when the task is re-activated for a redo. So once the agent obeyed a rejection and ran start-work, any further await call reported the same rejection again and sent it back to start-work on work already in flight. Resume had this right, requiring an empty Activity; both sites now share isUnstartedRejection so they cannot drift apart again. Confirming the final task completes the session, and the next await then failed requireActiveSession: non-zero exit, empty stdout, JSON on stderr, and a recovery hint pointing at status rather than next-action. That is the end of every session, and the agent is under instructions to keep re-running await, so it would routinely read the finish line as a broken session. It now routes through Resume, which hands back the next-action command; a genuinely aborted session still errors. Resume's "task is already active" branch carries no next, so advertising advance_allowed true promised a command that was not there. It now leaves the field unset. Rejecting two tasks in one step left the agent with "stripe coop status" and nothing to run, because nextInStepOrStatus only looked for pending siblings. It now also offers a sibling still carrying unstarted feedback. Finally, the heartbeat is removed on every await return again. Retaining it between calls did not prevent the TUI reporting the agent idle — that check falls through to the last session change, which is frozen for the whole review — and it left a killed agent indistinguishable from a patient one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The prompt claimed "every response carries advance_allowed". It does not — start-work, report-check, the session bootstrap, stop, and every next-action outcome omit it, and error responses carry their command under recovery rather than a top-level next, which the field's contract never mentioned. A model told the field is always present has no rule for its absence. The prompt now states the false case and says any other response is followed normally, and points at recovery for failures. It also told the agent to run "stripe coop agent resume" with no session, which exits non-zero — a regression from when the TUI supplied the full command. It now shows the flag with the session_id every response carries. README fixes: next-action and await-review are no longer described as blocking, the five-minute waiter is gone, and wait_timeout_seconds is documented as carrying the interval rather than the harness timeout, which is prompt-only and never on the wire. Test fixes, all cases that could not fail: the discovery-mode assertion matched a substring that never occurs even with a session; the cumulative-wait test passed equally for per-process timing, so it now backdates the review entry and asserts a span no in-process timer could produce, plus the review prompt it was named for; and the Codex TOML test claimed verification while only checking a prefix, so it now actually parses the value. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The block budget counted consecutive drift attempts, which is a poor proxy for what it is actually detecting: a developer who has walked away. At three attempts an agent drifting on every await call exhausted it in about three minutes, well inside a genuine review — someone clicking through a checkout in a browser easily takes longer. The budget is now primarily wall-clock: the hook keeps holding turns until stopBlockWindow (30 minutes) passes with the lifecycle unchanged. maxConsecutiveStopBlocks stays as a runaway guard for an agent that re-stops immediately without waiting, where turns would otherwise burn far faster than the window anticipates. Both restart when the pending command changes, so a healthy long session is never cut off. That matters more than it used to because releasing is now terminal. With the tmux typing gone, a stopped agent never sees a later confirmation: no turn is running, so no Stop event fires and the hook cannot help. The TUI now closes that loop. A review decision made while no agent is waiting reports the exact "stripe coop agent resume --session=<id>" to paste into the agent pane, and that status persists rather than expiring like an acknowledgement, because it is an action the developer has to take. The status line wraps now: it can carry a command, which is longer than the short acknowledgements it used to hold and would have overflowed the narrow layout. Verified by rendering frames at 69, 100, and 120 columns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Selecting Finish hands the agent "stripe coop stop", which completes the session. Resume answered any completed session with the next-action command, so the stop hook then held the turn open and handed back next-action again — the agent could never exit, and the developer would see the post-completion suggestions reappear. Nothing recorded the Finish choice. Other next actions report back through --completed when their follow-up finishes; Finish has no follow-up, so consuming the selection is the only chance to record it. It is now written to NextSteps.Completed there, and Resume treats a finished session as having nothing left to run. Reported by @kwu-stripe on stripe#1862. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The old maxConsecutiveStopBlocks doc was left stranded above stopBlockWindow when the budget became wall-clock bound, describing a limit that no longer exists and a TUI idle path that no longer applies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
b1acaa3 to
9129360
Compare
Summary
The agent frequently failed to wait for, or wake up on, the developer's confirmation — worst with cheaper models. The cause is a timeout race, not prompting.
await-reviewblocked for five minutes and asked agents to allow a six-minute shell timeout. That only holds if the model actually sets one. Claude Code's Bash tool defaults to 120s and its override env vars have open bugs, so a model that does not set a timeout has the command killed at two minutes — before the internal deadline — and receives a raw harness string with nookand nonextinstead of structured JSON. Cheap models respond by assuming the step passed, or by stopping to ask a question in a pane the developer is not watching.Each call now waits at most
AwaitTimeout(45s, under the harness default) and returnsstate: "waiting"withadvance_allowed: falseand anextrepeating the exact command just run. The developer is not rushed: review state lives in the session file, so the total wait stays unbounded across calls.next-actionwas untouched by the earlier review-wait work and still had the original bug in its worst form — a ten-minute block that returnedErrSelectionTimeout, surfacing as JSON on stderr with a non-zero exit. An agent reads that as a broken session and gives up.Relationship to #1826
#1826 fixed the same class of problem by tagging the tmux agent pane and typing a resume prompt into it. That mechanism is removed here: it only works when Co-op launched the agent under tmux, and driving another program's input is not something to depend on.
stripe coop agent resumeis kept — it is read-only, types nothing, and is now the shared lifecycle query behind both the Stop hook and manual recovery.AwaitTimeout/AwaitHarnessTimeoutand the harness-headroom invariant from #1826 are kept, retuned to 45s / 90s so the interval sits under the harness default rather than only under an advertised timeout.Also fixed
Races that are reachable today and become routine once calls are frequent and short:
active, soAwaitReviewfell through toalreadyMovedResponse, whosenextpointed at the following pending task. The agent skipped the rejected work and never saw the feedback.ShowSuggestionsclearedNextSteps.Selectedunconditionally, erasing a choice the developer made while the previous call was exiting.await-reviewthen failedrequireActiveSession— non-zero exit, empty stdout, recovery pointing atstatusrather thannext-action. That is the end of every session.Stop hook
Returning on an interval removes the hard failure, but a model can still stop mid-session and the CLI cannot force it back. A
Stophook can: it fires exactly when the agent tries to end its turn. It asksresumewhat the session needs rather than recomputing the lifecycle, and blocks with that exact command.Injection is via launch flags only —
claude --settingsandcodex -c— so nothing is written into the user's repo. Bounded by a 30-minute wall-clock window (what it detects is an absent developer; attempt count is a poor proxy), with a count ceiling as a runaway guard. When it does release the agent stops for good, so the TUI now reports the exactstripe coop agent resumecommand to paste, and keeps it on screen.Test plan
make test(52 packages),go test -race ./pkg/coop/... ./pkg/cmd/coop,make lint— all cleanawait-reviewcalls, then confirming from the TUI and watching the agent advance within secondsThree adversarial reviews found nine defects, fixed in the last four commits — including an escape hatch that could never fire because the command the hook ordered bumped the version the budget keyed on, and a Codex auto-approve path that silently dropped the hook.
Notes for review
stripeearlier onPATHsilently reintroduces the old behavior, since agent instructions invoke barestripe. Worth a version check incoop runas a follow-up; not included here.🤖 Generated with Claude Code