feat(review): add heartbeat, timeout, cleanup, and structured verdict metadata #137

Activity

1. BaseInfinity

   added

   enhancementNew feature or requestNew feature or request

   on Aug 15, 2026
2. BaseInfinity

   added this to the 1.0.0 — Bounded autonomous delivery milestone on Aug 15, 2026
3. BaseInfinity commented on Aug 15, 2026

   

   BaseInfinity

   on Aug 15, 2026

   OwnerAuthor

   More actions

   Sibling-repo evidence: we built this twice and deleted it both times

   claude-sdlc-harness hit the identical symptom — a non-interactive review leg silent for minutes, with no way to tell reasoning from stall. Filed as #590 after two legs were waited on for 51 and 28 minutes. It shipped fixed in v1.98.0 (2026-08-15).

   The root cause was not lifecycle observability. It was stdin.

   The reviewer CLI reads stdin to EOF and appends it to the argv prompt — they concatenate, they are not alternatives. Handed a pipe nobody closes, it blocks on that read before contacting the provider, forever. Both of our "silent reviewer" incidents were processes that had never started. Verified against the real 0.147.0 binary with the launcher's own stdin on an unclosed fifo: with < /dev/null on the child, the leg completes normally.

   That matters for this issue's framing: a heartbeat on those runs would have reported healthy the entire time. The child was alive, the output stream was intact, nothing had timed out. Liveness was never the missing signal.

   We built the runner this issue asks for. Twice. Both were falsified with running code.

   Attempt 1 — watch the output file and the process table. Every signal was refuted:

   • -o output carries no completion marker
   • tokens used appears in the CLI's echoed prompt, so a crashed leg reads as complete
   • an fd reported as PIPE may be at EOF and perfectly healthy
   • the hang's byte signature is machine-dependent (39 bytes on one host, 143 on another)
   • enumerating holders of the output file counts readers, so an operator running tail -f flips the verdict

   Attempt 2 — record child pid and exit status in sidecar files. Also refuted:

   • the child exits before the status is published, so a successful leg reads as dead
   • killing only the launcher leaves a status that can never arrive while the pid still looks alive
   • a stale status from a previous run is read as this run's
   • a reused pid is indistinguishable from the original process

   Same mistake in two costumes: a second observer reconstructing what the first process already knows. The launcher's own exit status is that knowledge, delivered by the process that has it.

   What we shipped instead

   scripts/run-review-leg.sh — about 40 lines of actual logic:

   • child gets /dev/null on stdin regardless of what the parent inherited (the whole prevention)
   • the launcher execs the reviewer, so the caller sees its exit status with nothing in between that can die separately
   • no heartbeat, no watcher, no sidecars. The task's exit status is the verdict: 0 completed, non-zero failed, nothing by your deadline means inspect and relaunch.
   • a PreToolUse hook (#610) refuses a review leg typed outside the launcher, so a leg cannot be started without an owner

   Where our experience does back this issue

   Three of your acceptance criteria we independently proved necessary:

   1. Process-tree cleanup is real and non-optional. Our CI preflight probe backgrounds gh. Without set -m the subshell shares the parent's process group, so killing by pid leaves gh alive, reparented to PID 1. Review ran the fixture and observed exactly that. Fix: set -m to make the subshell a group leader, then kill -TERM -- "-$PID". Also disown after, or bash's job-termination notice leaks into the reviewer's own output file.

   2. Do not use kill -0 for liveness. A background job of the shell stays a zombie until waited on, so kill -0 reports it alive after it finished. wait -n solves it but needs bash 4 and macOS ships 3.2. We use a done-marker file plus a bounded poll.

   3. "Bounded wall-clock timeout" cannot lean on the child. We wrote "gh carries its own network timeouts" and it is false — gh 2.92 configures no overall HTTP timeout (cli/cli api/http_client.go, go-gh pkg/api/client_options.go). A stall would block before the reviewer ever launched. The deadline has to be the launcher's own and depend on nothing.

   One more that bit us: set -e in the probe subshell killed it before it could write its marker, so every failing child burned the full deadline and reported "abandoned" — the one case it most needed to report correctly. set +e inside the subshell.

   Suggestion

   Split #137. The cleanup and bounded-timeout half is well-evidenced and we'd copy your version. The heartbeat and liveness-classification half is the part we spent two attempts on and could not make sound — every completion signal was forgeable, and the actual bug produced a perfectly healthy-looking process. Before building it, check whether your silent runs are the same stdin concatenation; if so, < /dev/null on the child is the entire fix and the runner is unnecessary.

   Cross-references: claude-sdlc-harness #590 (incident), #610 (the gate requiring the launcher), #613 (the preflight that nearly reintroduced the hang), #594 (the launcher is still repo-internal and does not ship — already linked here).
4. BaseInfinity commented on Aug 15, 2026

   

   BaseInfinity

   on Aug 15, 2026

   OwnerAuthor

   More actions

   Official host guidance checked (2026-08-15)

   This issue should supervise each host through its structured non-interactive interface rather than infer liveness from terminal text.

   Codex leg

   Current Codex CLI exposes:

   • codex exec review ... --json for JSONL lifecycle events;
   • --output-schema for a constrained final verdict;
   • --output-last-message for a durable final-response fallback;
   • process exit status in addition to the event stream.

   Official references:

   • https://learn.chatgpt.com/docs/non-interactive-mode
   • https://learn.chatgpt.com/docs/code-review

   Implementation implication: heartbeat metadata should report elapsed time plus the last structured event timestamp/type. It must not claim the model is "thinking" merely because the process is alive.

   Claude/Fable leg

   Current Claude Code documents and exposes:

   • claude -p --output-format stream-json for real-time newline-delimited events;
   • --json-schema for structured final output;
   • nonzero process exit on failure, while in-run failures such as missing authentication may also be reported in the stdout result;
   • background Bash cleanup after the final result;
   • since v2.1.182, a default ten-minute ceiling for background subagents/workflows in print mode;
   • SIGTERM aborts the turn, terminates running Bash process trees, runs SessionEnd hooks, and exits 143.

   Official reference:

   • https://code.claude.com/docs/en/headless

   Do not blindly use --bare for our subscription-backed Fable lane: the official docs say bare mode does not use OAuth/keychain credentials and requires an API key for the Anthropic API. Prefer the narrowest safe non-interactive configuration that preserves the intended subscription authentication.

   Runner design confirmed

   1. Spawn each reviewer in its own process group/job.
   2. Close stdin and do not allocate an interactive TTY.
   3. Consume JSONL/stream-json continuously and write the raw sanitized attempt log locally.
   4. Emit concise heartbeats based on elapsed time and last observable event/output activity, never chain-of-thought.
   5. Enforce both:
      • inactivity/stall classification; and
      • an independent hard wall-clock deadline even if noisy output continues.
   6. On cancel/timeout: TERM, bounded grace, KILL escalation, then verify the full process tree is gone on POSIX and Windows.
   7. Classify terminal state as CLEAN, FINDINGS, AUTH_OR_INPUT_BLOCKED, PROVIDER_OR_TRANSPORT_FAILURE, TIMED_OUT, CANCELLED, INVALID_VERDICT, or INTERNAL_RUNNER_FAILURE.
   8. Retry at most once and only before a valid verdict for a classified infrastructure failure. Never retry findings or a valid clean verdict.
   9. Bind the terminal receipt to exact candidate SHA/tree, host executable/version, attempt history, timestamps, exit/signal, retry reason, verdict, and sanitized log path.
   10. Test healthy silence, noisy infinite output, auth failure on stdout, malformed/truncated JSONL, descendant-held stdio, ignored TERM, cancellation, and Windows process-tree cleanup.

   This validates the existing acceptance criteria and adds two important details: parse Claude failures from both stdout and exit status, and do not let continuing output defeat the hard wall-clock timeout.
5. BaseInfinity commented on Aug 15, 2026

   

   BaseInfinity

   on Aug 15, 2026

   OwnerAuthor

   More actions

   Conceded — the structured-stream citation defeats my objection, and I want to be precise about why

   My previous comment argued the heartbeat half was unsound. That argument does not survive the docs you cited, and the distinction is one I drew myself without following it through.

   What I actually falsified was inference over unstructured output: watching an -o text file and the process table, and trying to reconstruct the child's state from byte counts, fd states, and file holders. Every one of those signals was forgeable, and I stand by each individual falsification. The conclusion I drew from them — "a runner cannot know" — was too wide.

   The line my own comment drew was between a second observer reconstructing what the first process knows, and the first process reporting it. codex exec review --json and --output-schema, and Claude's stream-json, are the host reporting. That is first-party knowledge on a documented contract, not reconstruction. It falls on the sound side of the line I was arguing for, and I put it on the wrong side.

   Two specifics where the citation is strictly better than what we shipped:

   • Our hang would have been caught, and caught early. The stdin-concatenation block happens before the provider is contacted, so a JSONL stream emits zero events — unambiguous, and distinguishable from slow reasoning, which emits events. That is exactly the discrimination I claimed was unavailable. It was unavailable to a byte-counting observer, which is a statement about our implementation, not about the problem.
   • tokens used in the echoed prompt — my sharpest falsification — is an artifact of scraping human-readable output. A typed event stream has no such collision.

   Your item 7, "bind the verdict to the exact reviewed tree," is the same primitive we are landing right now on our side for claude-sdlc-harness#540: git write-tree as candidate_tree, verified at the commit boundary and again at the merge boundary. We read #111's dual-review.cjs (requireFrozenIndex then candidateTree) rather than deriving it, as your ruling said to. Two notes from doing it that may save you a round:

   • At the commit boundary, base_tree is not observable without knowing whether the commit is an amend (base is HEAD vs HEAD^), and determining that means parsing git's option grammar. We deferred base_tree to the merge boundary, where the base ref is well-defined, and rely on candidate_tree alone at commit — a rebase changes the index tree anyway, so nothing is lost.
   • We refuse when tracked worktree ≠ index, which is your requireFrozenIndex. That single precondition is what let us avoid flag parsing entirely: with a clean tracked tree, git commit and git commit -a commit the same tree.

   Agreed on your sequencing: #111, then #137, before 1.0.

   Retracting my "split #137" suggestion. It rested on the heartbeat half being unsound and it is not.

   For our part, adopting the structured-stream contract in scripts/run-review-leg.sh is now a real gap on our side rather than a design we rejected, and I am filing it against ourselves. Our launcher's exit-status-only design is the correct floor — it needs no cooperation from the child — but it is a floor, not a ceiling, and "nothing by your deadline" is a worse answer than "zero events in 90 seconds" when the host will tell us which.
6. BaseInfinity commented on Aug 16, 2026

   

   BaseInfinity

   on Aug 16, 2026

   OwnerAuthor

   More actions

   Actual Fable-high field evidence

   Feedback type: pattern confirmation / provenance requirement

   In a five-milestone overnight Codex Desktop run, every risk tier that required Fable used the real subscription-backed command class claude -p --model fable --effort high. A native Codex subagent was not treated as a substitute. The reviews completed and supplied valid clean verdicts against frozen candidates.

   What worked

   • The external process boundary provided genuinely cross-model review.
   • Exact candidate/proof prompts plus a frozen tree made the verdict actionable.
   • Subscription-backed invocation worked non-interactively on Windows without API-key substitution.

   What remains weak

   • “Fable reviewed” is only a label unless the receipt proves the executable, model, effort, candidate tree, and terminal result.
   • Silent intervals remain operationally ambiguous in the Desktop conversation.
   • A fallback to a native Codex worker could accidentally preserve the label while removing the cross-model property.

   Suggested receipt fields

   Record executable identity/version, sanitized exact argv class, model=fable, effort=high, authentication class (subscription-backed, never credential material), candidate SHA/tree, start/end timestamps, structured-output mode, exit status, retry reason, and verdict. Fail closed as AUTH_OR_INPUT_BLOCKED or PROVIDER_OR_TRANSPORT_FAILURE when the real Fable leg cannot launch; never silently substitute another model. This fits #137’s structured metadata and process supervision contract.
7. BaseInfinity commented on Aug 16, 2026

   

   BaseInfinity

   on Aug 16, 2026

   OwnerAuthor

   More actions

   Dogfood reproduction from a bundled 0.7.35 consumer on 2026-08-16:

   • dual-review.cjs hard-coded a 10-minute production timeout because configuredDuration() ignored CODEX_SDLC_REVIEW_TIMEOUT_MS unless CODEX_SDLC_TEST_MODE=1. A real Opus 4.8 xhigh review timed out without a receipt.
   • The retry exited nonzero with empty stderr; the wrapper discarded Claude JSON written to stdout and emitted only Fable review failed.
   • The finally block removed the temporary Sol/Claude outputs, leaving no phase/candidate diagnostic.
   • A direct structured claude -p --model claude-opus-4-8 --effort xhigh probe succeeded under first-party subscription auth, confirming this was adapter behavior rather than missing subscription/model access.
   • The consumer shim also mislabeled the actual Opus 4.8 xhigh leg as Fable High in the receipt, overlapping fix(review): fail closed on unavailable Fable and bind the actual reviewer identity #143.

   Local recovery for the blocked consumer release: 30-minute default with a one-hour bounded override, parse nonzero Claude stdout diagnostics, write a local error receipt with phase/base/tree, pin claude-opus-4-8 + xhigh directly, and record the actual reviewer identity. Please retain production-timeout, stdout-error-envelope, error-receipt, and truthful-identity regressions in the new review-gate lifecycle suite.
8. BaseInfinity commented on Aug 16, 2026

   

   BaseInfinity

   on Aug 16, 2026

   OwnerAuthor

   More actions

   #143 receipt-schema follow-up

   PR #152 shipped truthful requested-versus-actual cross-model reviewer identity and receipt schema v2. The bounded final review found no blocking consumer defect, but #137 should explicitly audit every receipt reader before extending the lifecycle runner:

   • delivery/merge gates;
   • diagnostics and status output;
   • setup/update migration;
   • Windows fixtures;
   • any downstream tooling reading dual-review.json.

   Keep the audit narrow: prove each consumer either understands schema v2 or fails closed. Do not rerun #143 reviewer-lane design.
9. BaseInfinity

   closed this as completedin #156on Aug 17, 2026
10. BaseInfinity commented on Aug 17, 2026

    

    BaseInfinity

    on Aug 17, 2026

    OwnerAuthor

    More actions

    Post-merge cadence evidence for the #109 pilot (captured from the exact #137 review receipt and PR #156):

    • Local delivery date: 2026-08-16 America/Los_Angeles
    • Stable base: e1a635d78d7e97747324dbc7e312e023cb01b91f
    • Candidate tree: 26d7faebc711590efa5c30f1646c53e8fc75bb49
    • Patch SHA-256: sha256:94c5b0e3931343605033ef1239b5b708d7e8a7021d33f5956b0d1c5f5d52d55f
    • Changed files: 5
    • Broad proof runs: 1; duplicate unchanged-tree broad proofs: 0
    • Sol: gpt-5.6-sol high, 199453 ms, confidence 94, CERTIFIED
    • Cross-model: requested Fable; actual quota fallback claude-opus-4-8 xhigh, 458091 ms, confidence 80, CERTIFIED
    • Reconciliation: skipped on initial agreement; 0 rounds
    • PR feat(review): supervise reviewer lifecycle #156 merged at 2026-08-17T01:24:24Z; issue closed one second later
    • Windows windows-npm check succeeded

    This observation is deliberately ineligible for the ten-delivery decision because subscription quota/token costs and total delivery minutes were not captured. Unknown measurements must not be treated as zero.