Skip to content

Latest commit

 

History

History
29 lines (18 loc) · 15.3 KB

controller-loop.md

File metadata and controls

29 lines (18 loc) · 15.3 KB

The controller loop

An orchestrated step wakes, reads its envelope, and reports exactly one NextMove (work-types.ts) via mcp__outpost__submit_step_progress:

move meaning
self-round keep going myself; optional action temporarily rebinds the session to a sub-action (boundAction)
dispatch fan out to child action sessions (Dispatch[]); an identical (action, brief) needs an explicit retryOf
wait park on WaitSpec — watched events (pr-comments, ci, review-state, pr-state, head-moved) and/or all dispatches done. No timer: a controller cannot arm its own resumeAt (see below)
gate ask the user a question and hold (voluntary; distinct from the write-draft gate)
resolve / fail settle the step

What wakes it lands in step.inbox (user-message, dispatch-done, external from the PR watcher, gate-resolved, timer, policy-rejection); lastDelivered persists the batch so a cold resume can still show what woke it. The loop has no round cap — it is bounded only by the finer guards in src/steps/orchestrated-policy.ts: an unproductive-self-round cap, a per-dispatch attempt cap, and a two-strikes rule on policy rejections. A MAX_ROUNDS = 80 wall used to sit above those and was removed: it policed nothing the finer guards didn't (a controller that stays productive never trips them either way), and what it actually did was fail live steps mid-flight — CS-1608's PR was open, approved and one check from merge when its step hit round 82 and died, which is a far worse outcome than a runaway burning extra tokens. roundsSpent survives as the count the envelope reports back to the controller, informational only. Runtime lives in src/work/orchestrated-runner.ts; PR facts the controller reads (step.pr) are maintained by src/integrations/pr-watcher.ts, not by the controller itself.

A wake costs a round, so the watcher fires on what the ladder can act on — not on what moved. changedSignals (pr-watcher.ts) used to push an external item for any change to any watched signal, and the canonical row-17 wait names all four, so waitSatisfied's per-event filter never filtered anything. The gap between "a signal moved" and "a signal reached a value some ladder row reads" was pure waste: the controller woke, matched no row, and re-armed the same wait. Three suppressions now, each tied to a row rather than to a hunch — CI → pending (no row reads it, and the controller's own fix-ci push causes it), CI → success while review is unapproved (row 16 is the only reader of success and needs the approval too; green with an approval still fires, since suppressing that one would strand the step with nothing left to carry the news), and a comment authored by the account the daemon posts as (row 13 matches only comments that aren't yours; resolved once via gh api user and cached, failing open so an unresolvable identity silences nothing). Facts are still recorded when the wake is suppressed — the PWA badge and the controller's next envelope read pr directly. On top of that, EXTERNAL_QUIET_MS (orchestrated-inbox.ts) holds a batch of watcher events for two minutes measured from the oldest held item, so a reviewer leaving four comments costs one round; oldest-not-newest is what stops sustained churn from deferring the wake forever. Only an all-external batch is ever held — a user message, a settled dispatch, a resolved gate or a due soak is somebody actually waiting. Nothing else releases a held batch (the watcher pushes only on movement, and a quiet PR moves nothing), so deliverInbox arms host.scheduleDelivery → WorkEngine.scheduleDelivery, a one-shot in-memory tick. That timer is not the safety net: it dies with the process and is skipped when the session reads as mid-turn, and a quiet PR never pushes again to re-arm it — which is how CS-2099 sat "approved, CI green" with its wake undelivered. WorkEngine.redeliverParked runs after every 5-minute watcher sweep and re-runs delivery for each parked controller with a non-empty inbox, so a lost timer costs one sweep, not the step. Precedent for why this matters: the mergeable-flapping bug at pr-watcher.ts's UNKNOWN guard cost one step 76 identical "PR open" wakes. The guard yields to one thing — a new head: a conflicting verdict stored against an older headRefOid is replaced by UNKNOWN, which never wakes, so the card stops reading "Merge conflicts" once the resolving push lands, and unknown → mergeable carries the cleared-conflict wake that conflicting → mergeable used to. Separately, pr.hasUnpushedWork is read off the step's worktree (WorkEngine.readUnpushedWork) on every sweep and every controller move; it never wakes anything, it only lets a parked card say "Waiting for push" instead of "Ready to merge" when what's blocking the merge is your uncommitted fix.

A fourth suppression, on the other half of the watcher: head-moved while the daemon is itself opening the PR. The PWA's squash-and-open-PR button pushes the branch and then runs gh pr create, and for the seconds in between origin has the branch with no PR on it — precisely the state row 8 reads as "implemented and pushed, open the PR". A sweep landing in that window woke the controller with head-moved and "no PR yet", which is one of the two ways a gh pr create draft reached the user for a PR that already existed or was seconds from existing (the other is the controller reading its memo instead of pr — see duplicatePrCreate above). PrFacts.isOpeningPr brackets the daemon's own call (markPrOpening/finishPrOpening, engine.ts, from both PR-opening git routes); syncBranchHead records the head but pushes no inbox item while it's set. It is cleared in reconcileInterruptedSteps as well as by finishPrOpening, because a flag that outlived a daemon restart would silence that step's head-moved for the rest of its life. finishPrOpening also stores the URL gh pr create just printed — the one moment the daemon knows a PR's URL without discovery, which is what makes the card show the PR on button-release instead of on the 1m escalation, and what puts prUrl in the controller's envelope before its next wake rather than after. Note the asymmetry this does not fix: a PR opened by the controller's own approved row-8 draft still goes through discoverPr, because that call runs in the session and nothing captures its stdout. Pinned by pr-watcher-opening.test.ts and engine-pr-opening.test.ts.

A controller does not wake itself — the daemon does, and validateNext refuses a wait carrying resumeAt. The move was there for real gaps in what the watcher could see, but a self-armed timer can only ever wake the controller to discover nothing changed, at two rounds a firing (the delivery plus the wait it re-arms), and skill prose telling it not to didn't hold. The one gap that actually justified a poll was row 8 of code.orchestrate-pr — "implemented, waiting for the user to push" — where gh pr view has nothing to read and discoverPr keeps missing, so the SKILL prescribed an hourly resumeAt. Closed at the source instead: while no PR is known, syncBranchHead (pr-watcher.ts) reads origin's head for the step's own branch via git ls-remote and fires head-moved when it appears or moves, so the branch landing IS the event and row 8 parks indefinitely on ["head-moved","pr-state"]. Unlike the PR's headRefOid, a first observation fires here — an unpushed branch reads as absent, not as an unknown sha, so there's nothing to guard against. The refusal names both escapes (events, or gate when nothing on the list can end the wait), since a controller told only "illegal" is as likely to reach for fail. WaitSpec.resumeAt survives as runtime-only plumbing — the MCP schema no longer offers it and the policy refuses it, but steps parked before the cutover still hold one on disk and must not hang. meta.wait's soak is a different field (ActionStep.resumeAt) and is untouched: that one is the user's hold, not the controller's.

resolve means the PR merged — the policy enforces it, it isn't just skill prose. A controller holding a writable workspace owns its PR, so validateNext refuses its resolve while step.pr.prUrl is set and prState isn't merged. resolve and fail used to skip every guard, which let a controller near the (since-removed) round wall pick the exit that reads as done: the egress-v2 step resolved at round 78 of 80 with its PR open, approved, and one check from merge, and the unfinished work dropped out of the cockpit. Three carve-outs, each load-bearing — workspace.kind (a readonly reviewer like code.orchestrate-review settles on a verdict it could never merge), an absent prUrl (the step never opened one), and boundAction === 'code.merge-pr' (that round re-reads GitHub, so it knows about a merge the watcher hasn't swept — and since the watcher only syncs live steps, a just-merged step's recorded prState is frozen at open the moment it resolves; 3 of the 4 such steps on disk are merged PRs whose facts never caught up). Pinned by orchestrated-policy.test.ts.

And a PR is opened once — same reason, opposite end of the ladder. submitDraft refuses a gh pr create draft (duplicatePrCreate, orchestrated-policy.ts) while step.pr.prUrl is set and the PR isn't closed. Row 8 is the only row that opens a PR and pr.prUrl is its falsifier, but the falsifier lived in skill prose and prose did not hold: CS-2068's controller drafted gh pr create on a turn whose envelope carried the PR's URL, prState: "open", a headRefOid equal to its own HEAD, and a delivery item reading "review review_required; PR open" — and wrote "No PR exists yet" in the evidence. What it followed instead was its own memo's ON WAKE script, written back when pr was still null. The daemon's data was never wrong, so nothing on the watcher side fixes this; the user's only recourse was to deny the card, and they'd been doing that every time they used the PWA's squash-and-open-PR button. The branch check row 8 prescribes (git rev-parse --verify origin/<branch>) actively misleads here — a PR opened by hand leaves exactly the pushed branch that check looks for. closed is carved out because that's row 10's fail to make, not this guard's. Pinned by write-draft-pr-exists.test.ts.

Nothing reviewed a PR-shaped step's code before it reached the user. code.orchestrate-pr ran spec → plan → implement in one session, each round reading the last, and then went straight to opening the PR. The only review in that chain was code.implement's own Step 3 — the session that wrote the diff grading its own diff, which cannot catch what it was already wrong about. code.review-diff sat in the controller's roster and no ladder row ever invoked it. The observable cost: an external reviewer (Devin) found one or two real issues on essentially every PR the step opened. Rows 5-7 are the fix — dispatch fresh lens sessions, synthesize, bind code.implement for what's blocking, re-review — capped at three passes, then gate. Two details are load-bearing and easy to undo: the lenses take workspace: {"kind":"none"} plus inputs.worktreePath, because at that point the change is uncommitted and a dispatched child's detached checkout holds the branch tip without it — a lens pointed at its own cwd finds a clean tree and reports no issues, which is worse than not reviewing at all since it wears a review's clothes. And each pass must name itself (pass 2) in the brief, or the identical-(action, brief) dispatch guard rejects the re-review. The loop's state is two artifacts, not the memo: review (first line pass <n> — blocking|clean) and reviewFixes (first line addressed pass <n>), so a cold resume can tell "reviewed and clean" from "reviewed, fixed, not re-reviewed".

code.review-diff is the only lens either controller runs on its own; code.security-review and code.review-ui are on user request only. Both were first written to fire off the diff's shape — security whenever the change touched auth/input/deps, UI whenever it touched src/pwa/**, and in code.orchestrate-review security on every PR on the grounds that somebody else's code doesn't get good faith. Each rule reads as prudent and each one fires on nearly every change, so what the user actually experiences is a standing three-session tax on work they asked one review for — and in orchestrate-pr it is charged again on every re-review pass. A controller that thinks a diff warrants one says so in the review it writes and lets the user call it; the two lenses stay in both rosters so that call is one message away. Do not re-derive a "dispatch it when the diff touches X" rule: that is the thing that was removed, and the diff always touches X.

Pre-approvals let the user skip the clicks they'd rubber-stamp, and only the user can grant one. Preapprovals (work-types.ts) — spec (gate/auto/skip), push, openPr, replies, merge, syncBase, rerunCi, and landing (merged/approved/direct) — lives in Settings as a global default (preapprovalDefaults in preferences), on the job, and on each orchestrated step as an override; effectivePreapprovals (src/work/preapprovals.ts) layers them settings → job → step, live, gated when absent. syncBase covers code.resolve-conflicts' git commit --no-edit + git push origin <branch> only while the daemon itself reads an in-progress merge of origin/<base> (gitMergingFromBase); push covers it too. rerunCi covers code.fix-ci's gh run rerun <id> [--failed] once the daemon re-reads the run (rerun-readiness.ts) and finds it on the step's branch, completed, not green, and on its first attempt — one automatic retry per run, since a check that fails its rerun is no longer plausibly a flake. Every real value is written by a user HTTP route: Settings (PATCH /api/preferences re-validates the key), job creation, a schedule (copied onto each job it spawns), plan approval (stepPreapprovals), or PUT …/steps/:id/preapprovals. A planner writes only proposedPreapprovals; materialize strips a smuggled preapprovals and the reconcile patch can't carry one; under planReview: 'auto' proposals are clamped to the job's own settings (clampToCeiling), so they can only tighten; create_job over MCP refuses both fields. Enforcement is one seam: submitDraft asks classifyDraft (src/work/preapproval-classify.ts) whether every call is a known shape against the step's own branch/base/PR and raises only the dangerous-writes.ts findings that shape expects — a confirm finding never is — then calls acceptDraft itself with approvedBy: 'preapproval'. Pins, digests and allows() are untouched; a draft with one uncovered call goes to the user whole with autoApproveMiss saying why. A merge is additionally re-read fresh (merge-readiness.ts) and must carry --match-head-commit. Two policy changes ride along: under spec: 'gate' the daemon now refuses binding code.plan/code.implement until specApprovedAt is stamped (row 2 used to be prose only), and landing: 'approved' is a fourth resolve carve-out. The run ledger scores these drafts auto-accepted, outside every rate — they say nothing about whether the user liked the payload. Pinned by preapproval-*.test.ts and tests/e2e/preapprovals.spec.ts.