feat(hold): first-class held state, typed rulings, hold-first recovery (#627 Stage 1) - #637
Merged
Merged
Conversation
…e 1a)
Pure hold-state module: HoldRecord {escalationId, unit identity, executionId,
agentId, openedAt, deadline, phase open|released|cancelled, escalation text,
ruling {id, replyTo, actor{role,id}, instructions}, deliveryState
none|pending|in-flight|acknowledged}. Transitions are pure and return new
records. evaluateCompletionAuthority() is the single predicate every
completion path will consult: open (incl. expired) or released-but-
unacknowledged holds block; cancelled does not. validateRuling() accepts
only a 'ruling' message correlated by replyTo to an OPEN hold of exactly
this unit (and execution, when known) carrying a trusted actor with role
supervisor|operator — steer never releases, acks never extend the deadline.
Schema: BATCH_STATE_SCHEMA_VERSION 4→5 adds the authoritative top-level
holds table (v4 upconverts to an empty table; a v5 file missing the table is
refused — no silent backfill of an authority table). Statuses gain 'held'
(task, segment, monitor snapshot); PauseSignal gains cause 'hold-timeout';
MailboxMessageType gains 'ruling' with an optional trusted actor stamp,
written only for ruling messages. Config: taskRunner.worker.holdTimeoutMinutes
(default 240, 5..10080) → TASKPLANE_HOLD_TIMEOUT_MIN.
Tests: hold-state.test.ts (29); migration fixtures moved to v5.
…first monitor (#627 stage 1b) Lane-runner: the volatile Tier-1 bridge (pendingEscalation, MAX_HOLD_RELAUNCHES, lastSupervisorReplyTs) is replaced by a durable, runner-owned hold loop. - An escalation opens a HoldRecord that is persisted (strict) BEFORE the outbox message is acked; a persist failure leaves the escalation queued, alerts, and blocks every spawn / no-progress kill / drain until it lands. - Any worker exit with an open hold hands over to awaitHoldResolution(): no worker process, no relaunch, no iteration or stall consumption. The loop publishes 'held' (STATUS, lane snapshot with worker:null, one alert) and polls the unit's inbox as the SINGLE consumer of hold-control mail: ruling → validateRuling (typed, replyTo-correlated, trusted actor role) → release + persist; info → acknowledgement only (deadline unchanged); query → answered from stored state, no spawn; abort → cancel, never approve, task fails; steer → ordinary mail, never releases. - Pause unwinds with a 'held' outcome (hold preserved). Deadline expiry records expiredAt, alerts once, and parks the batch with pause cause 'hold-timeout' — the hold stays open. - The ruling is placed at the TOP of the relaunched worker's initial input with delivery marked in-flight (strict) before spawn; a worker reply with replyTo=<rulingId> acknowledges; unacknowledged rulings are replayed (at-least-once) and completion stays withheld until acknowledged. - evaluateCompletionAuthority() gates step check-off, the all-complete break, and the post-loop finalize (returns 'held', never failed/succeeded, when the budget runs out under a hold). A worker-written .DONE while hold-blocked is quarantined (.DONE.unauthorized-<ts>). - Exit-intercept skips units with an open hold and never consumes rulings. Engine/execution/resume: createHoldStore() over the runtime batch state persists via persistRuntimeStateStrict (throws; best-effort path untouched for telemetry). executeWave/executeLaneV2/monitorLanes thread the store. resolveTaskMonitorState evaluates hold authority BEFORE .DONE, dead-pid and stall (status 'held', sessionAlive true, stall clock reset). Wave tally adds heldTaskIds; a held lane leaves its remaining tasks pending; the pause finalizer treats held as interrupted (preserve worktrees, no merge). Resume restores holds verbatim. agent-host never steers a 'ruling' into a session. Tests: held-state-runner.test.ts (9 behavioural: open-before-ack, no relaunch, initial-input delivery + ack, rejection matrix, query, abort, pause/resume without spawn, expiry → hold-timeout, .DONE quarantine + monitor hold-first, fail-closed persist failure, at-least-once replay). issue-630-hold-exit.test.ts reduced to its surviving wiring tests.
…ery guards, dashboard (#627 stage 1c) send_agent_message: type='ruling' with replyTo (validated against the durable hold table at the sender — correlation, unit, phase, actor role), stamped actor role 'supervisor' by the tool itself. Held units are addressable with NO live worker pid (the runner's hold loop, or the next resume, is the consumer); results say delivered / queued for runner / queued while parked, and state the effect (steer never releases). /orch-rule <escalation id> <text> is the only path that stamps role 'operator'. Resume: hold-first reconciliation (durable table + escalations still unrecorded in a worker outbox → 'held', re-executed into the hold loop, a stale .DONE never marks complete); re-execution is lane-parallel (serial within a lane) so a restored hold does not park unrelated lanes; a 'held' re-execution outcome is preserved as held; the hold store exists from the re-execution phase onward with a mutable persistence context. Guards: orch_retry_task refuses held tasks (retry is not release) and names the ruling call; orch_force_merge refuses a wave with unresolved holds; drainAgentOutbox preserves 'escalate' messages by default (takeover and hard-fail paths can no longer destroy an unpersisted hold); the takeover summary lists held units; engine pre-cleanup preserves worktrees/branches while any hold is unresolved, even when git-clean. Dashboard: held status (⚖) in task badges, segment pills and summary chips. Docs: primer 'Held lane' section + recipe, supervisor template, commands (/orch-rule), task-runner config (hold_timeout_minutes), spec status, CHANGELOG. Tests: held-state-recovery.test.ts (12); windows widened in source-slice tests; drain test updated for escalation preservation.
1. Hold loop reads the inbox BEFORE the deadline check, so a ruling queued
while the batch was parked on hold-timeout is consumed on resume instead
of the lane re-parking with the ruling unread.
2. Every 'held' return parks the batch: the post-loop budget-exhausted path
now sets pause cause hold-timeout (+ alert). A returned held controller
with the batch still running left the wave monitor reporting held forever.
3. Every production executeLaneV2 call carries the strict hold store:
attemptWorkerCrashRetry / attemptModelFallbackRetry (their detached
{ paused:false } signals are now a proxy onto the batch signal — reads
ignore only the R002-4 stop-wave cause, writes go through — and a held
retry outcome is moved to heldTaskIds via recordHeldRetryOutcome) and the
resume reconnect path. Resume's wave pause-finalizer honours heldTaskIds.
4. Resume hold-store persistence context is thunks: a strict checkpoint
during re-execution serializes the persisted task table (synthesized
pre-wave outcomes) instead of an empty outcome list, discovery is exposed
as soon as it exists, and wave-phase checkpoints follow the live
latestAllocatedLanes / allTaskOutcomes variables.
5. Segment scope: holdBindsUnit() — a whole-task hold binds every segment, a
segment hold binds its segment AND the whole-task unit — used by the
completion predicate, mail classification, ack matching and ruling
validation; resume pins a held segment as the task's active segment
(pinHeldSegments) so re-execution runs the held unit.
6. Escalation scope: the agent bridge stamps scope {taskId, segmentId} on
every outbox message. The live drain opens holds only for escalations
that match the unit (stamped scope, or unscoped-but-written-this-run);
foreign ones are left in the outbox. Resume performs VALIDATED replay
before frontier repair and reconciliation (replayUnrecordedEscalations:
scoped → that task; unscoped → only a single-task lane's durable record;
otherwise the resume is refused) and persists the opened holds strictly.
7. Runtime reconstruction rebuilds the hold table from mailbox evidence
(reconstructHoldsFromMailbox: scoped escalations in outbox+processed,
earliest FULLY valid ruling in inbox+ack releases, reply replyTo=ruling
acknowledges) and REFUSES the candidate when authority cannot be
recovered: unscoped escalation, unknown task, segment-scoped evidence
without topology, unreadable/malformed mailbox files.
Tests: +19 (runner: expired-hold ruling consumed on resume, budget-exhausted
held parks, foreign-scope escalation left alone; recovery: replay
attribution/refusal, segment pinning, A→B mailbox reuse refusal, valid-ruling
reconstruction, malformed evidence refusal; wiring). 4009 pass.
A. Singleton units ("TP-1::default") escalated with a blank segment stamp
(FULL_TASK mode clears the prompt-visibility segment vars) and the exact
scope filter rejected the worker's own escalation → task succeeded, .DONE
accepted, hold never opened. The lane now exports the REAL unit identity
(TASKPLANE_UNIT_SEGMENT_ID) for stamping, and escalationMatchesUnit accepts
the current run's own mail for the same task whatever its segment stamp.
B. Resume could complete, clean up and delete state with an open hold: a
.DONE at the canonical task path hid the task from discovery so no
controller was restored. Resume now quarantines unauthorized .DONE markers
of hold-bound tasks (canonical + worktree paths) BEFORE discovery, and a
terminal gate mirrors the engine: unresolved holds → paused, preserve,
never completed, no cleanup.
C. A released ruling for a segment whose checkboxes were complete could
never be delivered: the completed-segment spawn shortcut cancelled the
delivery iteration. The shortcut is bypassed while a ruling is undelivered.
D. stop-wave retries no longer ran: the producer set paused=true with no
cause, so the linked retry signal saw an active pause. The producer stamps
cause 'stop-wave' (never overwriting an existing cause).
E. Pre-wave strict checkpoints dropped partialProgress*/exitDiagnostic and
wrote taskFolder ''. The pre-wave outcome set carries the full persisted
metadata and a synthetic discovery supplies folder/repo/segment fields.
F. Recovery readers were fail-open on malformed evidence: resume replay used
the permissive readOutbox (swallows errors); reconstruction validated only
id/type. New readOutboxStrict (throws) for replay; reconstruction uses
isValidMailboxMessage before batch filtering.
Tests: +7 behavioural/wiring (singleton-unit stamp, delivery vs completed
segment, quarantine, terminal-gate order, linked signal both directions,
metadata carry, strict readers). 4016 pass.
…llback for every resume checkpoint (#627 stage 1) The synthetic pre-wave discovery was replaced by fresh discovery as soon as it existed, and fresh discovery lists only tasks still to run — so a hold checkpoint after that point wrote taskFolder '' (and dropped partial-progress / diagnostic fields) for every completed task. Fresh discovery is now merged OVER the persisted fallback for both checkpoint contexts; fresh entries win, persisted entries fill the gaps. Test: real hold checkpoint through persistRuntimeStateStrict with one held task and one completed task absent from fresh discovery; reload asserts folders, partialProgress* and exitDiagnostic survive. 4017 pass.
…d, finalize binding, ruling trailer
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
First-class
heldstate for escalations — #627 Stage 1Closes the four-incident class (TP-2037/2039/2042/2047): a worker escalation now produces a runtime-owned hold instead of prompt discipline + supervisor acks.
What changes for operators
escalate_to_supervisorholds: the worker exits and is not relaunched, the lane waits at zero cost (no iterations / stall clock / relaunch budget), the task showsheld(⚖), worktree and branch are preserved and never merged.⏸️ Lane heldalert names the escalation id.send_agent_message(type="ruling", replyTo=<escalation id>)(supervisor) or/orch-rule <escalation id> <text>(operator — the only path that stamps roleoperator).infoacknowledges without releasing;queryanswers from stored state (no spawn);abortcancels and fails (never approves);steerno longer releases a hold.taskRunner.worker.holdTimeoutMinutes(default 240): expiry parks the batch with pause causehold-timeout, hold stays open; rule thenorch_resume(force=true).holds[]table; v4 upconverts).Authority is one predicate
evaluateCompletionAuthority()gates step check-off, finalize, the task monitor (hold-first, before.DONE/stall/dead-pid), resume reconciliation (hold-first, with validated replay of unrecorded escalations and hold-aware segment pinning), force-merge, retry, cleanup, takeover and runtime reconstruction (rebuilt from mailbox evidence or refused). Worker-written.DONEunder a hold is quarantined. Hold persistence is strict (fail-closed): a persist failure blocks the lane, never releases.Review
Sage design pass → 3 implementation commits → 4 blocker-only review rounds (7 + 6 + 1 blockers, most reproduced by probes: expired-hold ruling ordering, controller-exit deadlock, retry paths without the store, singleton-unit scope stamping, resume completing over an open hold, stop-wave retries, metadata loss on strict checkpoints, permissive recovery readers) → explicit sign-off at
05ab5ae1.Validation
Suite 4017 pass / 0 fail (+~90 new behavioural tests in
hold-state,held-state-runner,held-state-recovery); typecheck, format, lint (283 < 284 baseline), CLI help/doctor clean.Follow-ups (not in this PR)
Stage 2 ratification record + finalize binding (#627 remainder, feeds #626) — to run as a Taskplane batch on this build; #631 lease/generation; #628 takeover state machine; #626 coverage gate. Closes #630 (subsumed).