v0.30.6 bundle: review-boundary supervisor notifications; #620 #624 #625 #626 #628 #629 #630 #631 #610 - #632
Merged
Conversation
… ctx External report from @daemons2000 (vetted valid, P1). The async engine- worker IPC handlers and related callbacks used a captured ExtensionContext/ ExtensionAPI after Pi invalidated it (session replacement/reload, or a finalized headless -p run while the forked engine worker still emits IPC). Every ctx accessor (ctx.ui, ctx.isIdle()) and pi.send* call Pi's assertActive(), which throws 'This extension ctx is stale ...'; that throw, uncaught inside a child_process callback, is a process-fatal uncaughtException that exits the supervising Pi process mid-batch. Same class as #597 (stale captured handle in a background callback) but a distinct path: timers/pi.sendMessage there, child-process IPC/ctx.ui.* here. #597's safeSendMessageFromTimer only wraps pi.sendMessage, so these were unguarded. Fix: generalize the #597 pattern to any thunk (safeCtxCallFromCallback in supervisor.ts) and route all stale-sensitive async-callback vectors through it: - ctx.ui.notify — notify/error IPC cases, child.on('error'|'exit'), the main-thread fallback notify callbacks, and startBatchAsync's .catch (the last caught by Sage code review). - ctx.ui.setWidget — guarded inside updateOrchWidget (single choke point). - ctx.isIdle() — dispatchBatchEndEpilogue now skips dispatch + stopBatchMonitoring + noticeGate.invalidate() when the ctx is stale. - pi.sendUserMessage — both supervisor-alert callbacks. The guard no-ops ONLY on Pi's exact stale-ctx error (isStaleExtensionCtx), logs any other error so real failures still surface, and never rethrows (a throw from an IPC callback is the very crash this fixes). The 'error' IPC branch now persists failed batch state BEFORE touching the UI, so a dead UI sink can never block the dashboard/resume from seeing the failure. Engine state, review, verification, retries, and failure propagation unchanged. 8 new regression tests (supervisor.test.ts 8.23-8.30): wrapper behavior (success/stale/non-stale) + wiring assertions for all vectors incl. startBatchAsync. Strategy + code reviewed with Sage; blocking finding (startBatchAsync.catch) remediated. Full suite 3755 pass / 0 fail / 1 skip. Refs #620.
Sage code-review follow-up. Drives the REAL startBatchInWorker with a
mocked child_process.fork (controllable fake child) and a ctx whose ui
getter throws Pi's exact stale-context error, then emits IPC messages:
B1 ('error' IPC): asserts no uncaught throw escapes, the failed batch
state is persisted BEFORE the supervisor alert (the #620 reorder),
batch phase becomes 'failed', and the supervisor alert still fires.
B2 ('notify' IPC): asserts the stale ctx.ui no-ops without crashing.
Complements the wrapper unit tests (supervisor.test.ts 8.23-8.25) and the
source-assertion wiring tests (8.26-8.30) with an end-to-end behavioral
guarantee against the actual crash class. Isolated in its own file so the
mock.module setup (child_process.fork + persistence.saveBatchState spy)
doesn't leak into other suites. Uses saveBatchState mock to record call
order for the persist-before-alert assertion.
Full suite 3757 pass / 0 fail / 1 skip.
…ost-exit Stage 1 of the review-boundary supervisor-notification work. Bug: worker reply/escalate mail (notify_supervisor / escalate_to_supervisor -> *.msg.json) only reached the supervisor AFTER the worker subprocess exited. lane-runner read the outbox once, in the post-exit block. A worker mailing mid-run (e.g. asking for help to break a review spiral) sat unread until it exited, so the supervisor 'woke up' too late. (Segment-expansion requests are scanned live by the engine, but reply/escalate .msg.json had no live scanner — that asymmetry was the bug.) Fix: extract the outbox surfacing into a reusable drainAndSurfaceOutbox() helper and run it on a live timer (OUTBOX_LIVE_POLL_INTERVAL_MS = 3s) during the worker await — mirroring the existing reviewerRefresh interval — plus once more after exit as a final drain. Each surfaced message is acked (ackOutboxMessage moves it to processed/), so the live timer and the final drain never double-surface the same message. The helper is re-entrancy guarded (outboxDraining flag) so a slow cycle can't overlap the next tick. Not a delivery/wake problem: pi.sendUserMessage always triggers a turn. This was purely a surfacing-timing gap. Tests: lane-runner-outbox-live.test.ts (6 source-assertion tests for the helper + live-timer wiring + final drain). The ack->no-reread dedup invariant that makes double-surfacing impossible is already covered behaviorally by mailbox-v2.test.ts 1.6. Full suite 3763 pass / 0 fail / 1 skip.
…age 2)
Emits the (previously dormant) review lifecycle events and surfaces them on
the supervisor's live events.jsonl tailer so the supervisor is informed at
EVERY review start/end and can adjudicate revisions case-by-case — instead of
relying on the operator to notice reviews happening.
Flow (worker subprocess -> supervisor):
- agent-host.ts observes the worker's review_step tool. At tool_execution_start
it emits RuntimeAgentEvent review_requested {step,reviewType}; at
tool_execution_end it normalizes the reviewer verdict
(normalizeReviewDisposition) and emits review_completed
(APPROVE/REVISE/RETHINK/REFUSED) or review_failed (UNAVAILABLE/UNKNOWN — a
separate broken-reviewer signal, not a spiral).
- lane-runner.ts passes a bridgeReviewEvent onEvent callback to spawnAgent that
maps these to engine events (review_started/review_completed/review_failed)
and writes them to .pi/supervisor/events.jsonl via emitEngineEvent.
- supervisor.ts registers the three types as SIGNIFICANT and adds
formatEventNotification cases (reviewLocation = task/step/lane). They ride
the existing tailer notify (pi.sendMessage triggerTurn:true) = the followUp
tier; the steer escalation comes in Stage 3.
Import-cycle fix: emitEngineEvent is lazily imported in lane-runner (cached),
NOT statically — persistence.ts -> execution.ts -> lane-runner.ts would form a
cycle whose eager load pre-bound execution's executeTaskV2 before
mock.module("lane-runner") could register, breaking spawn-failure-visibility
tests. Lazy load keeps lane-runner's static graph cycle-free; ordering is
preserved (start fires before end; import() cache + queued callbacks).
Sage code-review remediations (all applied):
1. End events carry step+reviewType via a pendingReview slot (tool_execution_end
omits args) — required for per-step adjudication + Stage-3 spiral keying.
2. Dangling review_started closed with review_failed(aborted) on worker
crash/kill/timeout so the supervisor never sees an orphaned 'review starting'.
3. Autonomy gate fixed: review_* always notify (incl. autonomous mode) — the
feature would otherwise be silently disabled exactly where the supervisor
adjudicates autonomously.
4. normalizeReviewDisposition hardened: word-boundary fallback + reject negated
approvals (not approved / disapprove / do not approve).
(Deferred per explicit 'every boundary' requirement: notification coalescing.)
Tests: review-boundary-notifications.test.ts (15) — disposition normalization
incl. negation guards, formatEventNotification for all three, end-event identity,
crash-abort closure, SIGNIFICANT registration, bridge + lazy-import wiring.
Full suite 3778 pass / 0 fail / 1 skip; typecheck/lint(286-671)/format green.
Types: ReviewDisposition; EngineEventType += review_started/completed/failed;
EngineEvent += agentId/reviewStep/reviewType/disposition.
…ignals (Stage 3)
Completes the review-boundary supervisor-notification feature. Builds on the
Stage 2 per-boundary notifications with spiral detection, an actionable
escalation, and the finding-count/trend/round signals the CarbonMantis
supervisor agent identified as its core spiral-vs-converging inputs.
New pure module review-analysis.ts (dependency-free, unit-tested):
- parseFindingCounts: severity-bucketed counts from a review's Issues Found
(configurable vocabulary; unknown severities -> 'other', never dropped).
- computeFindingTrend: lexicographic-by-severity dropping|flat|rising + mixed.
- advanceReviewStreak: the SHARED per-step counter transition (used by both
the live path and resume reconstruction, so they can't drift).
- reconstructReviewStreaks: replays events.jsonl history on resume.
- shouldFireSpiral / shouldFireOrderViolation: pure escalation gates
(first-crossing, trend-gated + cooldown-spaced re-fire).
- sanitizeSpiralConfig: clamps threshold/cooldown >= 1, coerces booleans.
lane-runner: per-(taskId,stepNum) streak state (seeded from history on resume);
on each review END boundary reads the exact review file (via the reviewPath
agent-host parses from the tool return), computes counts+trend, advances the
shared streak, enriches the engine event, and fires a steer-delivered
review-intervention-needed alert on spiral (3 consecutive non-approve) or
order-violation (REFUSED, kept out of the streak). UNAVAILABLE/UNKNOWN never
count. extension.ts delivers the new category as deliverAs:steer (routine
boundaries stay followUp). supervisor formatEventNotification appends the
round/counts/trend signals.
Config: taskRunner.reviewer.severityLabels (default critical/important/minor,
generic) + spiral{enabled,threshold:3,cooldownReviews:2,
treatUnavailableAsNonApprove:false}, threaded engine->executeWave->
buildReviewerEnv (one JSON env var)->laneRunnerConfig->lane-runner with
defensive defaults.
Sage plan-review + code-review; all findings applied (config sanitizer,
documented no-count trend-baseline + enabled-gates-both semantics, plus tests
for the sanitizer / enabled=false / resume replay). Same-finding-class-recurred
flag intentionally deferred as a fast-follow.
Types: ReviewInterventionKind; SupervisorAlertCategory += review-intervention-needed;
SupervisorAlertContext + EngineEvent + ParsedEvent enriched with review fields.
Tests: review-analysis.test.ts (38), review-boundary-notifications.test.ts (+),
plus updated windowing/literal source-assertions. Full suite 3821 pass / 0
fail / 1 skip; typecheck/lint(286-671)/format/CLI-smoke green.
…ncies.json A git stash/merge-conflict artifact was accidentally swept into this generated task-cache by 'git add -A' during the Stage 2 commit (775188c), leaving conflict markers (<<<<<<< / ======= / >>>>>>>) that made the file invalid JSON. This file is a generated dependency cache unrelated to the review-boundary feature; restored to match main.
The notification pipeline was built, but the supervisor (an LLM) had no
guidance on how to interpret or act on review-boundary notifications — it would
likely just relay them to the operator, defeating the autonomy goal. Adds that
guidance to the shipped operational runbook + the always-in-context standing
orders.
supervisor-primer.md:
- Alert Categories table: add review-intervention-needed.
- New '### Review-Boundary Notifications (Active Adjudication)' subsection in
§13a explaining the per-boundary stream + the three adjudication signals
(disposition, round, findings+trend) and the converging-vs-circling read.
- New 'Playbook D: Review Spiral / Adjudication' decision tree in §13b:
order-violation → steer revert+re-review; revision-spiral → trend-gated
judgment (dropping = let it run; flat/rising = read the review file + steer
to a concrete resolution, proceed, or stop+log a blocker).
supervisor.ts system prompt: new standing order '2a. Adjudicate reviews' so the
supervisor actively acts on review notifications (uses the trend to tell
converging from circling; steers via send_agent_message; follows Playbook D)
rather than relaying to the operator. The primer is read on demand; the standing
order keeps this top-of-mind.
Generic/project-agnostic (severity vocab referenced as 'configured
severityLabels, e.g. critical/important/minor or P0/P1/P2'); project-specific
spiral policy belongs in .pi/agents/supervisor.md overrides.
Tests: supervisor-alerts.test.ts 4.16a (primer Playbook D) + 4.16b (standing
order). Full suite 3823 pass / 0 fail / 1 skip; typecheck/lint(286)/format green.
…eview
The review-boundary path fired a spurious review_failed -> 'Reviewer unavailable'
supervisor alert on EVERY review (8 on one task's Step 1 in the reported run),
even though the worker received the correct verdict and the review file was
well-formed. Two compounding defects, both fixed:
1. Narrow tool-result extraction (agent-host). tool_execution_end took
fullResult only from a STRING event.result/event.output, but Pi delivers
tool results as structured content arrays. For review_step that yielded "",
so normalizeReviewDisposition returned UNKNOWN. Fixed with a new
extractToolResultText() that mirrors the existing extractAssistantText()
(string | content-block array | {content} object).
2. UNKNOWN conflated with UNAVAILABLE (agent-host). A supervisor-side parse
miss (UNKNOWN) was bucketed with UNAVAILABLE into review_failed, reporting a
'broken reviewer'. Now ONLY a genuine UNAVAILABLE emits review_failed;
UNKNOWN emits review_completed.
Plus the robust primary fix (lane-runner): the review FILE's '## Verdict:' is
now the authoritative disposition (parseReviewVerdict, reusing the executor's
parser). lane-runner already read the review file for finding counts; it now
reads it once for BOTH counts and verdict, resolves disposition =
fileVerdict ?? payloadDisposition, and classifies review_completed vs
review_failed by the RESOLVED disposition. So review_failed ('Reviewer
unavailable') now fires ONLY when there is genuinely no verdict in the tool
return AND none on disk (reviewer produced no output) -- exactly the
acceptance criterion.
Tests: parseReviewVerdict (## / ### headings, missing), extractToolResultText
(structured array / {content} / string / fallback), + #624 wiring assertions
(agent-host UNAVAILABLE-only bucketing; lane-runner file-authority + resolved
classification). Full suite 3830 pass / 0 fail / 1 skip; typecheck/lint(286)/
format/CLI-smoke green.
…acing) Severity-upgrade follow-up: the live run showed the defect is NOT just supervisor noise — the review_step tool's OWN verdict extraction (which feeds the WORKER and the STATUS auto-log) mis-reported REVISE reviews as APPROVE. Workers marked steps '✅ Complete', advanced past unaddressed P1 findings, and one task nearly finalized unreviewed (TP-2022: R001-R004 all 'REVISE' in the file, all logged 'APPROVE' in STATUS). Distinct from the supervisor-path extraction fixed in 25cec37 — this is the tool-side parse in agent-bridge-extension.ts. Two defects: 1. Brittle primary regex ('###?\s*Verdict[:\s]*…') — missed common reviewer format variants (bold '**Verdict:** REVISE', plain 'Verdict:', dash/em-dash separators, bracketed verdicts, verdict on the following line). 2. APPROVE-biased fallback — on a regex miss, includes('approve') was checked FIRST, so any REVISE review whose body contained the substring 'approve' ('cannot approve', 'approval is blocked', …) flipped to APPROVE. A review gate that fails OPEN. Fix: - parseReviewVerdict (review-analysis.ts) upgraded to a robust line-oriented parser: heading/bold/plain markers, ':'/dash separators, bracket/bold token decoration, verdict-on-next-line, skips the '[APPROVE | REVISE | RETHINK]' template placeholder (2+ distinct tokens = not a verdict), rejects criteria prose ('Verdict criteria: APPROVE means…'). Shared by the tool, lane-runner (file-authority path), and any future consumer — one parser, no drift. - review_step now uses parseReviewVerdict; body fallback may only produce fail-CLOSED guesses (REVISE via \brevise\b/'changes requested', RETHINK). APPROVE is NEVER inferred from prose — approval must be an explicit Verdict line. STATUS logs UNKNOWN rather than a fabricated APPROVE. - Unclear-verdict tool return now instructs the worker to read the review file's Verdict line and NOT mark the step complete without an explicit APPROVE. Tests: parseReviewVerdict format variants + placeholder/prose rejection + TP-2022 regression class (variant-format REVISE with 'approve' in the body → REVISE, never APPROVE); agent-bridge wiring assertions (robust parser used, approve-biased fallback gone, fail-closed messaging). Full suite 3836 pass / 0 fail / 1 skip; typecheck/lint(286)/format green. Reporter's completion-gate request (task cannot finalize while steps lack an APPROVE in their review file) tracked separately — see issue comment.
, #626-minimal, #628-minimal) Driven by three batches of live incidents (see #626/#627/#628). Each is the minimal, fail-safe cut; the designed follow-ups (#626 full coverage gate, #627 held state, #628 takeover state-machine fix) come next. 1. #626-minimal — finalize gate: no .DONE over an outstanding REVISE/RETHINK. Two incidents merged unreviewed code: TP-2037 (worker self-released past a REVISE cap, wrote .DONE) and TP-2039 (lane-runner's checkbox heuristic wrote .DONE for a correctly-holding worker). Before creating/accepting .DONE the lane-runner now checks each gate's LATEST review file (latestReviewFilesPerGate, pure + tested) via the shared parseReviewVerdict; REVISE/RETHINK blocks finalization: worker-written .DONE deleted, STATUS 'Finalize refused', steer-delivered review-intervention-needed alert (new kind 'unresolved-verdict' with remediation guidance: re-review + retry, or record an operator ratification as the next R-numbered APPROVE review file), task returns failed with a clear exitReason. Steps with no reviews are not blocked (full coverage gate = #626 proper). 2. #628-minimal — never destroy uncommitted work in a lane worktree. supervisor_takeover cleanup force-wiped a held lane's worktree; uncommitted files were lost. removeWorktree + forceCleanupWorktree now refuse when the worktree has uncommitted changes (git status --porcelain), unless the caller passes allowDirty after preserving progress. A toplevel-identity check (rev-parse --show-toplevel, realpathSync.native to expand Windows 8.3 short names) makes corrupted/orphaned worktrees — whose git context resolves to the PARENT repo — fall through to removal, so recovery flows keep working. Sage-review blocker fixed: engine.ts + resume.ts reset-failure paths check the result; a dirty refusal is logged loudly, tracked in failedRemovalWorktrees for the cleanup gate, and never force-cleaned. 3. #625 — audit trail integrity: log_recovery_action tool. The supervisor system prompt literally instructed bash echo-append of actions.jsonl, so the LLM invented timestamps (5+ hrs off, non-monotonic, 72/130 on round minutes) and 30+ ad-hoc field names. logRecoveryAction() existed but nothing exposed it. New log_recovery_action tool code-stamps ts + batchId and enforces the AuditTrailEntry schema; the prompt's Audit Trail section is rewritten (NEVER hand-write) and the primer updated in three places. Tests: latestReviewFilesPerGate (incl. TP-2037 scenario), 5.4d dirty-guard behavioral (refusal preserves file+branch; forceCleanup honors the guard; allowDirty removes; corrupted/orphaned still removable), finalize-gate + audit-tool + refusal-surfacing wiring assertions. Full suite 3842 pass / 0 fail / 1 skip; typecheck/format clean; lint 285 (one below prior baseline — incidental autofix).
…pawn; refusal diagnostics (#629) On the v2 runtime segments[] is authoritative — resume's reconstructSegmentFrontier() re-derives task status FROM segment records. orch_retry_task reset only the task record, so the segment stayed failed, computeResumePoint counted the wave done, resumeWaveIndex ran past the end and the batch no-op'd (penster 20260905T165645, TP-2048). The documented #626 finalize-gate remedy (ratify → retry → resume) was unreachable. Core fix — fix the WRITERS, keep segment authority: - new segment-recovery.ts (pure): resetTaskSegmentsForRetry (failed|stalled| running → pending; exit data cleared; retries+1; worktree identity KEPT so resume re-executes in place; succeeded segments preserved), markTaskSegmentsSkipped, applyReExecutionOutcomeToSegments (scoped to the EXECUTED segment — re-execution runs the activeSegmentId unit, not the whole task), taskSegmentsAllSucceeded. - orch_retry_task / orch_skip_task call the writers. - resume re-execute path: transitions the executed segment on success/ failure/error; a non-final segment success leaves the task pending (task completion derives from the frontier); a PAUSE ('skipped' lane result) stays pending instead of becoming failed/unretryable-skipped; the REAL lane outcome (telemetry, diagnostic, timestamps) replaces the synthesized one, with persisted partial-progress metadata preserved. Remediation spawn (Sage-found trap): with every checkbox checked the loop broke before spawning, so retry+resume never launched a worker and the gate refused again. Now, when a finalizing iteration has no remaining steps but a gate's latest verdict is REVISE/RETHINK, the lane spawns a bounded (2) remediation iteration: focus step = the gate's step, 'REVIEW GATE OUTSTANDING' prompt (address findings, re-run review_step, never .DONE or re-check boxes), step status untouched, not counted toward no-progress; the #508 pre-spawn guard and the post-iteration allComplete exit defer to it. review_step's TP-186 complete-step guard is exempted when the latest review for that gate is non-APPROVE (that state IS the anomaly being repaired). Side effects from the same incident: 1. Finalize refusal is a governance outcome, not a crash: new ExitClassification 'review_gate_refusal' (exitCode 0) attached by the gate; tier-0 auto-retry explicitly never retries it. 2. Diagnostic reports no longer clobbered by a no-op resume (5/1h42m run reported as /usr/bin/bash/0s): field-wise evidence merge with the prior JSONL (current state from the new pass; cost/duration/classification/timestamps preserved when the new pass has none — no summing), per-task cost from outcome telemetry (taskExits was never populated), header cost falls back to the per-task sum. 3. skip-dependents blocked an out-of-batch task (dependency graph is repo-wide): computeTransitiveDependents gains a batch scope (visited set distinct from the reported set; traverses through out-of-scope nodes); applied at every recompute/merge site in engine, resume and extension. Tests: issue-629-retry-segment-reset (incident reproduction + fixed round-trip through the production re-execute action + serialization; non- final segment scoping; pause; skip; scope traversal; evidence merge incl. the reconciliation-placeholder case; guard exemption) and review-remediation-spawn (behavioural, mocked spawnAgent + real lane-runner: gate clears → 1 spawn → succeeded; never clears → 2 spawns → review_gate_refusal, no .DONE; no reviews → 0 spawns; two-step focus step; stall exemption). Suite 3881 pass / 0 fail.
…, single gate, confirm path (#631) A replacement supervisor could not resume the batch it inherited: lock takeover copied `phase: executing` into memory and every recovery tool refused on that cached phase, although persisted `executing` means "orchestrator disconnected" and is resumable. The only way out was a hand edit of batch-state.json plus a third session (penster 20260905T210935). The engine is a forked child that outlives its supervisor, so "the previous supervisor pid is dead" proves nothing about the engine. This fix verifies shutdown instead of inferring it, and applies ONE ownership rule everywhere. Engine identity (engine-identity.ts) - `.pi/runtime/<batchId>/engine.json` {pid, supervisorPid, startedAt, exitedAt?, exitReason?} is published BEFORE the engine is told to start: the batch id is preallocated by the supervisor for fresh batches (the engine adopts it) and is the gated target for resume; publication is tmp+rename with read-back and REQUIRED (unpublishable → the child is killed and the start fails closed; the main-thread fallback publishes its own pid or refuses to run). Exit marking is pid-scoped. isProcessAlive treats only ESRCH as dead (EPERM/unknown fail closed). - The engine refuses to resume a batch other than the one it was authorized for (checked BEFORE persisting any reconstruction). - An orphaned engine (parent gone) winds itself down as `paused`; IPC after the channel closes no longer crashes it into a fatal exit. One gate: decideRecoveryOwnership (pure) → recoveryOwnershipGate - engine attached to THIS process (running OR still exiting) → refuse; target's recorded engine alive elsewhere → refuse; no identity → refuse (unknown ownership is not confirmed shutdown); verified dead/exited → proceed. `force` never bypasses it. Always evaluated against the ACTUAL target (persisted, or reconstructed on force-resume), never a cached phase; workspace and repo roots must agree (conflicting persisted batches refuse). - Applied to orch_resume, orch_retry_task, orch_skip_task, orch_force_merge, orch_pause (inherited → administrative pause that persists `paused` — the non-destructive stop), orch_abort (verified local exit: grace → SIGTERM → SIGKILL, else refuse cleanup; fallback must settle), fresh /orch (before stale-state deletion/launch) and orch_integrate (ownership looked up BY THE SELECTED BRANCH over .pi/runtime/*/batch-meta.json — not by whichever batch reconstructs). - Cleanup is bound to the integrated batch: persisted state is deleted only when it belongs to that batch/branch (manual, auto and post-PR CI paths); the TP-051 lane-branch sweep keeps refs of any other batch whose engine is alive or whose ownership is unknown; an explicit branch argument never inherits an unrelated persisted batch id. - Agents still alive before a lane re-executes are terminated with verification (SIGTERM → wait → SIGKILL → wait → throw) instead of fire-and-forget. Operator path - New `orch_confirm_engine_shutdown(note, batchId?)` tool and `/orch-confirm-engine-shutdown [--batch <id>] <note>` command: the explicit, audited legacy path for batches with NO identity (pre-0.30.6; meta-only runtime dirs need `--batch`). Refuses when a real identity exists. Migration note in CHANGELOG (one-time per legacy batch). - Takeover summary reports engine liveness and dead-but-"running" registry agents (no registry hand-edit; resume reconciles). Primer Pattern 9, commands.md (/orch, /orch-resume, /orch-pause, /orch-abort, /orch-integrate, new command, tools table), AGENTS.md tool table. Tests: issue-631-inherited-engine (40 — identity round-trip/corrupt/pid- scoped race repro/confirm semantics; the six policy branches; the gate's bypass scenarios (idle cache + executing persisted + none; local engine exiting while cached paused; fallback attached; foreign alive for every cached phase; verified dead → proceed; operator-confirmed → proceed); legacy reconstructable + confirm round-trip; branch-bound lookup with live A behind orch/A vs newer exited B and non-reconstructable A; owned-only state deletion; CI cleanup preserving a newer batch; waitForChildExit behavioural; wiring for publish-before-init, authorized id, disconnect/IPC ordering, verified termination), stale-branch-cleanup (#631 ownership- aware sweep). Legacy guard tests updated to the single gate. Suite 3922 pass / 0 fail. Accepted Tier-2 residuals (lease/generation follow-up): two SIMULTANEOUS replacement sessions can both authorize against one verified-dead target; markEngineExited is read-check-write, not CAS (pid matching protects the sequential case only).
…d messaging error (#630) escalate_to_supervisor is fire-and-forget: a worker told by project rules to HOLD has nothing to do, ends its turn, and its process exits. With the engine alive the lane-runner relaunched it with the "CRITICAL: you exited prematurely — work continuously" nag (the prompt that pushes a holding worker toward self-release, the TP-2037 class); with the engine gone nothing relaunched it and the registry stayed `running` (#631's domain). Tier-1 cut (the in-tool waitForReply / first-class `held` state is the #627 design follow-up): - A clean exit after an unanswered escalation is a HOLD exit. Evaluated FIRST when no checkboxes progressed — before the cumulative git-diff soft-progress check, which otherwise masked every hold exit behind pre-existing uncommitted work. Not counted toward the no-progress limit; bounded to MAX_HOLD_RELAUNCHES (3); each relaunch gets a hold-resume prompt (act on a delivered ruling; do NOT proceed past the hold / self-approve / write .DONE / re-send). Exhausted → STATUS 'Held — ruling outstanding', task-failure alert 'Hold unresolved' carrying the escalation id and the retry recipe, task fails with that reason. - The hold is released by a reply: a steer delivered to the worker (.steering-pending) OR an instructional reply consumed by the exit-intercept. A reply WATERMARK (lastSupervisorReplyTs, message-creation timestamps on both sides) means an escalation drained later but created before the reply never (re)creates a hold, while a genuinely newer escalation still holds. - send_agent_message to a registry agent whose process is dead returns a distinct, actionable error (pid, last registry update, task; do not hand-edit registry.json; orch_resume reconciles) instead of "unknown session". Tests (issue-630-hold-exit, behavioural: mocked spawnAgent with an async beforeExit hook driving the REAL exit-intercept; real outbox/inbox messages): unanswered escalation with noProgressLimit=1 → exactly 1+3 spawns, hold-resume prompts, never the nag, 'Hold unresolved' + alert; steer after escalation releases the hold and normal stall accounting resumes; intercept-consumed ruling releases the hold (relaunch and first-spawn variants); reply created before the drain still counts; real dirty git worktree does not mask holds; older reply cannot suppress a newer escalation. Suite 3931 pass / 0 fail.
…ue (#610) Every promptly-integrated batch produced stale post-integration output: the "Ready for integration / run orch_integrate()" banner and, in supervised mode, the "Integration Plan … branches have diverged — merge commit … Shall I proceed?" prompt — for an orch branch that no longer existed. Mechanism: the engine finishes while the supervisor is mid-turn, so the batch-end epilogue is DEFERRED to agent_settled (#621's fix for the mid-tool-call splice crash). The supervisor integrates within that same turn (fast-forward, orch branch + batch-state deleted). The turn settles and the deferred epilogue fires unconditionally with content computed from the still-"completed" in-memory state. supersedeDeferredEpilogue() existed but only /orch and /orch-resume called it. Fix: - Manual orch_integrate success → supersedeDeferredEpilogue() and orchBatchState.integratedAt = now (before history/state cleanup). - Auto-integration executor (triggerSupervisorIntegration) records integratedAt on success for the current batch. - runSupervisorBatchEndEpilogue re-resolves at DISPATCH time: skips when integratedAt is set, or when the completed batch's orch branch no longer exists (logged). New optional OrchBatchRuntimeState.integratedAt (in-memory only; the persisted checkpoint is deleted on integration). Tests: gate-model sequence (deferred → invalidate → settle fires nothing) + wiring. Suite 3933 pass / 0 fail. Observed on the TP-114 smoke run (20260906T164113) after this bundle's Tier-1 changes.
This was referenced Sep 6, 2026
… releases); build marker in engine.json Penster feedback on the first PR #632 batch (20260906T194514) asked what happens when the outstanding item is an operator ruling that takes hours. As implemented, ANY steer after the escalation counted as the ruling — so a supervisor acknowledgement ("received, pending") released the hold, turning the worker's next idle exit into an ordinary stall and reverting its relaunch prompt to the "work continuously" nag; with no acknowledgement the three relaunches burned in minutes. Neither fits an hours-long ruling. Contract (explicit, by message type — no content heuristics): - send_agent_message(type="info") = ACKNOWLEDGEMENT: the worker stays on hold, its relaunch budget resets (supervisor engaged). STATUS logs 'Hold acknowledged'. - type="steer" (default) = RULING/INSTRUCTION: releases the hold. Applied on both reply paths (delivered steer via .steering-pending — which now carries `type` — and the exit-intercept's consumed reply). Hold-resume prompt tells the worker to keep working on anything not dependent on the ruling. Primer + tool guidance updated. Also from the feedback: - Build marker: engine.json records taskplaneVersion + taskplaneBuild (sha256 prefix of the loaded extension sources); takeover summary shows it. A local pre-release deploy is identifiable without grepping for tools. - Primer: hand-remediation under an operator ruling (hot-fix commits, ratification files, manual repairs) is a recovery action — log it via log_recovery_action. Tests: ack keeps holding past the 3-relaunch budget and a later steer releases (behavioural); build marker round-trips through engine.json. Suite 3935 pass / 0 fail.
…e notification, one streak advance Penster feedback on PR #632 (batch 20260906T194514, TP-2049): a duplicate review_completed for R002 reached the supervisor; it recognised it as redundant. It is not cosmetic: the same bridge feeds spiral detection, so a repeated end boundary advances the consecutive-non-APPROVE streak twice and fires the spiral a round early (which matches "fired at exactly two consecutive non-APPROVEs" under the default threshold of 3). Emission is 1:1 per tool_execution_end in the plain case (verified on the TP-114 run), so the duplicate comes from a legitimately repeated boundary — a retried worker turn, a re-invoked review_step for the same round, or a doubled RPC event. Rather than chase each, bridgeReviewEvent is now idempotent for END boundaries keyed by (step, reviewType, reviewPath): the second occurrence is logged ("Duplicate review boundary") and neither notifies nor advances the streak. Falls back to (step, type, ts, disposition) when no review path is known. Behavioural test (mocked spawnAgent, real lane-runner, bridge driven with the same end boundary twice): exactly one review_completed engine event with reviewRound 1. Suite 3936 pass / 0 fail.
…ges succeeded-but-unmerged work; retry accepts skipped; truthful completion wording penster batch 20260906T194514: the operator was away ~1h during a held escalation. orch_pause on the OWNED single-wave batch marked the holding task `skipped`, completed the batch 0/1, removed the worktree (work saved only by the #628 guard's saved ref). Then "Ready for integration. Merged to orch branch" fired on zero successes and an empty branch, and retry had no path from `skipped`. Owned-batch pause (B) - Two writers turned a paused task into `skipped` (lane-runner loop top; execution.ts remaining-lane-tasks). A pause now leaves the task `pending` (snapshot status idle, not failed); the wave tally exposes pausedTaskIds and an all-pending wave is never "succeeded". - Pause was only honoured BEFORE the next wave, so a single-wave batch completed. Engine + resume now finalize a wave with paused tasks as `paused` (persist, preserve worktrees, batch_paused, no merge). Every pause exit preserves worktrees. Abort (stop-all) keeps its own path. - PauseSignal gains `cause` (operator | abort | stop-wave | merge-failure); Tier-0 recovery clears ONLY a policy cause — a successful retry can no longer erase an operator's pause. In-process writers (orch_pause, administrative pause, takeover, abort) and the IPC handler stamp it. Resume correctness the pause fix depends on (Sage findings) - Catch-up merge (new step 8d, pure `selectCatchUpLanes`): lanes whose tasks all succeeded, weren't re-executed, and whose LAST wave's LATEST merge record is not succeeded are merged before the wave loop — a mixed wave (A succeeded, B paused) previously merged B alone on resume and dropped A silently. Failure → paused (cause merge-failure), worktrees preserved, alert; 8d re-runs on the next resume. 8c (re-executed branch merge) fails closed the same way and 8d is skipped after an 8c failure so a subset success can never certify the wave. - 8c merge outcomes are attributed to each task's real (last) wave, not the -1 sentinel (which persistence clamped to wave 0). - Resume restores persisted merge history into the fresh runtime state (earlier waves' records were dropped at the next checkpoint). - A re-executed non-final segment advances the frontier (`advanceActiveSegment`) on the persisted AND parsed task. Recovery from the incident state (C) - orch_retry_task accepts `skipped` (skippedTasks--; skipped segments reset) and reopens a `completed` batch as `stopped` (refused if already integrated). partialProgressBranch/Commits are reported as provenance, not cleared. Interim for long holds (until #627): hold exits no longer consume the productive-iteration budget (separate counter; still bounded by relaunches + ack resets + wall clock). Truthful completion (D, #610 recurrence): describeOrchBranchState / …AcrossRepos (workspace-aware) — the batch-complete alert reports outcomes and the orch branch state separately; "Ready for integration" only when a task succeeded AND the branch is verifiably ahead; empty → "Nothing to integrate"; unknown → "Could not verify". Tests: owned-batch-pause (19): behavioural pause → pending / no spawn; wave/engine/resume/Tier-0 wiring; retry-skipped + completed reopen + provenance; two-resume, success→failure, second-wave and merge-history round-trip regressions on real computeResumePoint / selectCatchUpLanes / serializeBatchState; frontier advance; branch-state helper (real git, incl. workspace aggregate). Suite 3955 pass / 0 fail. Docs: commands.md, CHANGELOG.
…able exit-intercept window; resume forwards worker env penster feedback #3 (batches 20260906T194514 / 20260906T221045): Item 4 — an UNCOMMITTED STATUS.md edit flipping a REVISE'd step to ✅ Complete reappeared on every relaunch and was never authored by the worker. Cause: the lane-runner's post-iteration heuristic marks any step whose checkboxes are all checked as Complete regardless of its review verdict. The worker correctly reverted it per the recovery recipe; the runtime flipped it back; the flip also trips review_step's complete-step guard. Step completion is now review-gated with the same scanner as the finalize gate: a step whose latest review is REVISE/RETHINK is not marked Complete ('Step completion withheld' in STATUS). Remediation eligibility is checkbox-based and unaffected; a later APPROVE lets the heuristic run. Item 5 — the 60s exit-intercept reply window is too short for a supervisor inside a long tool call (a blocking --wait). New taskRunner.worker.exit_intercept_timeout_sec (default 60, clamped 15..1800) threaded config-loader → buildWorkerEnv → lane-runner; agent-host's per-intercept safety race is window + 60s (exitInterceptSafetyMs). Sage-found gap while wiring it: resume's reconnect and re-execute paths never forwarded buildWorkerEnv(runnerConfig.worker) — worker model/thinking/ tools (and now this window) were dropped on retry+resume. Forwarded at both sites (buildWorkerEnv was an unused import in resume.ts). Tests: behavioural (all boxes checked, step In Progress, latest review REVISE → step block stays In Progress; 'Step completion withheld'), buildWorkerEnv clamping, wiring incl. resume forwarding. Docs: task-runner.yaml.md, CHANGELOG. Suite 3957 pass / 0 fail.
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.
Summary
Everything since v0.30.5, staged for v0.30.6: the review-boundary supervisor feature (spiral detection + adjudication), the #624 review-gate fixes, three evidence-driven safety cuts, Tier 1 of the Penster incident fixes (#629/#630/#631), and the #610 stale-epilogue regression fix. No release is triggered by merging — the
v0.30.6tag is a separate, deliberate step.Every risky seam was Sage-reviewed (plan → per-stage code review → full-diff review; #631 went through nine rounds of behavioural bypass hunting). Full suite 3933 pass / 0 fail / 1 skip; typecheck, format, lint (285 warnings — baseline) clean; CLI smoke OK. Smoke-tested end-to-end on
TP-114(batch20260906T164113): engine identity published before start, pid-scoped exit, segment authority intact, diagnostics cost populated, clean fast-forward integration, zero leftovers.Feature: review-boundary supervisor notifications
e192c64a,775188cb,b9b4c215).review-intervention-needed; REFUSED = order-violation signal; streaks reconstructed on resume. Config:taskRunner.reviewer.severityLabels,spiral{…}.86e93ec0).Fixes
safeCtxCallFromCallback; persist-before-UI)review_stepverdict gate fails closed — APPROVE only from an explicit Verdict line (REVISE was being flipped to APPROVE)log_recovery_actiontool: audit-trailts/batchIdare code-stamped; the prompt no longer instructs bash-appending.DONEover an outstanding REVISE/RETHINK (worker-written or runtime-heuristic)allowDirty); corrupted/orphaned worktrees still recoverableorch_retry_taskresets v2 segment records (the wave no longer no-ops); resume re-execute transitions the executed segment; pause stays pending; remediation spawn when all boxes are checked but a gate is outstanding;review_gate_refusalclassification; diagnostics no longer clobbered by no-op passes; batch-scopedskip-dependentsforcenever bypasses); administrative pause; branch-bound integration ownership; cleanup bound to the owned batch; verified engine + agent termination; orphan engine winds down as paused;orch_confirm_engine_shutdownlegacy pathHold unresolvedalert); reply watermark releases the hold via steer or exit-intercept; dead-pidsend_agent_messageerrorMigration note (#631)
Batches created before this build have no
engine.json. The first recovery tool or fresh/orchagainst one refuses (fail-closed: unknown ownership ≠ confirmed shutdown) until the operator runs/orch-confirm-engine-shutdown [--batch <id>] <what you verified>once. Documented in CHANGELOG andcommands.md.Accepted Tier-2 residuals (tracked on #631, next branch)
markEngineExitedis read-check-write, not CAS (pid matching covers the sequential case).heldstate + in-toolwaitForReply(Cap-ruling hold: runtime must block .DONE and merge while an operator ruling is pending (.PENDING-RULING) #627/Worker exits when an expectsReply escalation times out; lane-runner does not relaunch it and registry/lane state stay 'running' #630), supervisor_takeover destroys the paused lane: task marked skipped, batch completed 0/1, worktree+branch removed, uncommitted work lost #628 takeover state machine, feat(gate-integrity): task must not finalize succeeded while steps lack an explicit APPROVE in their review files #626 full coverage gate.Test plan