review: post a PR comment when the dispatcher dies without a result - #370
Conversation
…en the dispatcher dies without a result Run 32418662895 (#362) was killed at the 20-minute Bash ceiling during claim validation: no dispatch-result.json, no submission plan, no review, and a green run. The incomplete report's issue fallback is unavailable on a repo with issues disabled, so nothing surfaced on the PR. Now the orchestrator posts one add-comment naming the death and linking the run, then continues at Step 9 so prior fingerprints stand.
🦋 Changeset detectedLatest commit: 65320eb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested — see inline comments.
Note: correctness-reviewer not assessed this run (correctness-reviewer output unavailable).
review details
review-v1.17.1 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| the automated review died mid-dispatch and posted no review, linking | ||
| this run (`$GITHUB_SERVER_URL/$GITHUB_REPOSITORY/actions/runs/$GITHUB_RUN_ID`), | ||
| and noting that the next push triggers a fresh full review; then report | ||
| the run incomplete, skip Steps 4-8, and continue at Step 9 (the cache |
There was a problem hiding this comment.
question (non-blocking): Is the orchestrator prompt the right layer, when the root defect is that report_incomplete's only surface is an issue on an issues-disabled repo? This branch only fires when the agent survives the Bash kill and reliably follows a nested prompt instruction; the compiled workflow's report_incomplete / create_report_incomplete_issue handlers (review.lock.yml:564, 1357) are the generic path and are the thing that silently no-ops here. A deterministic if: always() step that posts a PR comment when the run queued no submission — or a PR-comment fallback for report_incomplete — would cover every silent-death shape including the job-level timeout that #369 is also raising.
review details
found by first-principles
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus 0441b7c6a581 (10 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
…y when the death comment posts; pin the gate shape
The death comment collapses the standing guidance comment (hide-older-comments
matches on the hidden marker regardless of body), but cache-record.ts returned
at the plan === undefined skip before the hold branch's compensation could run,
so the next approving run would read the unchanged risksPatternsKey as guidance
already posted and never restore it. The no-plan branch now recognizes a queued
add_comment and drops the key exactly like the hold branch (shared helper);
an unreadable queue stays a benign skip, since with no plan asserting a comment
it is indistinguishable from an ordinary early death.
Also: a gate backstop test pinning the death shape green (lone add_comment,
no submission plan, empty out/; add_comment is not in KEEP_ITEM_TYPES, so a
violation would strip the notice), the run link interpolated with
${{ github.* }} instead of shell $VAR the model would have to expand, and
the changeset bumped to minor since this changes posting behavior.
|
On the layering question from the review (why the orchestrator prompt rather than a deterministic fallback): the prompt branch only covers deaths the agent survives, which is exactly the observed shape (the dispatcher's Bash call killed at the engine ceiling, orchestrator still running). A job-level timeout kills everything and posts nothing, and this PR doesn't cover that. The deterministic fix (an The blocking finding and the other suggestions are addressed in 7f7eae6: the no-plan death path in |
This comment has been minimized.
This comment has been minimized.
Guidance for reviewersTriage notes for reviewers: risky files by owning team, repeated changes, and files excluded from review. github-actions (2 files)
review detailsreview-v1.18.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation |
There was a problem hiding this comment.
4 of 5 prior review threads resolved; 1 still unaddressed as of 01b73c9:
1 non-blocking thread still open
- question (non-blocking)
workflows/review/review.md:866: Is the orchestrator prompt the right layer, when the root defect is thatreport_incomplete's only surface is an issue...
review details
review-v1.18.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation| the dispatcher; if its call failed, treat the run as over budget and land | ||
| the review from whatever `out/` evidence exists (the gate decides whether | ||
| a verdict may post). Step 9's cache-memory record is also code-owned | ||
| the dispatcher; if its call failed but `dispatch-result.json` exists, |
There was a problem hiding this comment.
note (non-blocking): Only the dispatcher-call death is made visible; a plan-CLI death is still silent. The new clause is conditioned on dispatch-result.json being MISSING, and the retained sibling clause ("if its call failed but dispatch-result.json exists ... land the review from whatever out/ evidence exists") speaks to the dispatcher's call, not to the plan CLI at Step 3 item 3 — I grepped review.md for every other add-comment producer (lines 845, 862, 937) and none covers the plan-missing-with-result shape. Was leaving that adjacent no-plan shape uncovered a deliberate scope call for this PR?
review details
found by completeness| `dispatch-result.json` is MISSING after the dispatcher call (the Bash | ||
| call was killed at the engine ceiling or crashed before writing it), | ||
| there is no plan and you compose nothing by hand: post exactly one | ||
| standalone PR comment with the `add-comment` safe output stating that |
There was a problem hiding this comment.
thought (non-blocking): The death notice is a prompt rule executed by the process that may be dead. This makes visibility of a dead run depend on the orchestrator surviving its own death and following a multi-clause prose instruction, while the repo already has a deterministic chokepoint for exactly this — the post-steps dispatch-conformance gate runs if: always() in the agent job and already rewrites the finalized safe-output queue, and this PR's own new dispatch-gate test shows the gate sees the death shape. review.md's rationale for that gate (lines ~302-305) states the principle directly: "a prompt rule cannot gate an orchestrator that is already ignoring the prompt" — does the same reasoning apply to the notice that a run died?
review details
found by first-principles| the automated review died mid-dispatch and posted no review, linking | ||
| this run (`${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }}`), | ||
| and noting that the next push triggers a fresh full review; then report | ||
| the run incomplete, skip Steps 4-8, and continue at Step 9 (the cache |
There was a problem hiding this comment.
question (non-blocking): The diagnosis names the green run as a cause; the remedy leaves it green. The description lists "the run stayed green" alongside the missing comment as why the death was invisible, but the change only adds the comment and still reports success. Was exiting non-zero on the no-plan path considered and rejected — noting that the documented reason the gate avoids red-flagging infra failures (a spurious failure issue while the queue posts anyway) does not hold here, since nothing posts and issues are disabled on this repo?
review details
found by first-principlesThe death branch keyed off any queued add_comment while the doc, changeset, and fixture all define the shape as three facts; a planless run that still wrote dispatch-result.json or queued a review submission could be misread as a death and lose its risksPatternsKey. The branch now requires all three, with negative tests for each negation plus the readable-queue- without-comment case that had no coverage. Also from the round: the helper's discriminator parameter is a cause literal union instead of shadowing the lib-wide 'label' domain term, and the hold branch's comment no longer duplicates the helper docstring.
There was a problem hiding this comment.
4 of 9 prior review threads resolved; 5 still unaddressed as of 127bda6:
5 non-blocking threads still open (1 previously reported)
- nitpick (non-blocking)
workflows/review/lib/cache-record.test.ts:522:priorRecordis duplicated verbatim instead of hoisted like the other shared fixtures. The new block's `const priorReco... - note (non-blocking)
workflows/review/review.md:856: Only the dispatcher-call death is made visible; a plan-CLI death is still silent. The new clause is conditioned on `disp... - thought (non-blocking)
workflows/review/review.md:862: The death notice is a prompt rule executed by the process that may be dead. This makes visibility of a dead run depend o... - question (non-blocking)
workflows/review/review.md:866: The diagnosis names the green run as a cause; the remedy leaves it green. The description lists "the run stayed green" a... - question (non-blocking)
workflows/review/review.md:866
review details
review-v1.18.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation…ion in the header Three from the latest review round: the module header still promised the no-plan path never writes (false since the death branch), the death guard tested dispatch-result.json by existence while submission.ts defines 'no result' by parseability (readJson treats unparseable like missing, so all three seams now agree, with a test pinning the unparseable case), and the duplicated priorRecord fixture is hoisted to file scope.
There was a problem hiding this comment.
Changes requested — see inline comments.
3 of 7 prior review threads resolved; 4 still unaddressed as of 719b679:
4 non-blocking threads still open (4 previously reported)
- note (non-blocking)
workflows/review/review.md:856 - thought (non-blocking)
workflows/review/review.md:862 - question (non-blocking)
workflows/review/review.md:866 - question (non-blocking)
workflows/review/review.md:866
review details
review-v1.18.0 | schema 2 | depth full | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation…ike every other seam The blocking catch: review.md said 'exists' where cache-record.ts and submission.ts both mean 'parses' (a kill mid-writeFileSync leaves a truncated file that the over-budget branch then throws on, posting nothing), so the prose now says MISSING OR UNPARSEABLE and the over-budget branch requires a parse. Also from the round: the module header stops claiming task mode is untouched (scripted has been the only dispatch mode since 237d540; the header now says why that makes a planless run a death), and the gate-blocked guard gets the one missing justification comment.
There was a problem hiding this comment.
Approved — no blocking issues found.
3 of 7 prior review threads resolved; 4 still unaddressed as of 65320eb:
4 non-blocking threads still open (4 previously reported)
- note (non-blocking)
workflows/review/review.md - thought (non-blocking)
workflows/review/review.md:863 - question (non-blocking)
workflows/review/review.md:867 - question (non-blocking)
workflows/review/review.md:867
Non-blocking observations (5)
workflows/review/review.md:869note (non-blocking): "the next run reposts the guidance" holds only at full depth. (completeness)workflows/review/review.md:862suggestion (non-blocking): The death notice is emitted by the agent whose death it reports, so it only covers the sub-case where the model survives. (holistic)workflows/review/review.md:862suggestion (non-blocking): A ceiling death discards a completed fan-out that the over-budget path would have landed. (first-principles)workflows/review/review.md:864question (non-blocking): Was making the run red considered instead of posting a comment? (first-principles)workflows/review/review.md:867suggestion (non-blocking): "report the run incomplete" is sequenced before "continue at Step 9", so the new cache compensation may never run. (correctness-reviewer)
review details
review-v1.18.0 | schema 2 | depth scoped | re-review scoped blocking-only | enable holistic,completeness,test-adequacy,first-principles,conventions,documentation
The consequential one is sequencing: the dispatcher-death prose said 'report the run incomplete, then continue at Step 9', and reporting incomplete can end the turn, so the cache compensation the death path exists to trigger could be skipped with it. Step 9 now runs first, and the guidance-repost claim is scoped to the next full-depth run. The ban test folds whitespace before matching (a wrapped 'gh aw update' counted as no mention), gains a positive control so a broadened exclusion fails rather than passing vacuously, and checks existence before reading so the stale-allowlist message renders; git grep -l was considered and declined in a comment, since line-based matching is exactly what misses the wrapped form. foldToken's three length thresholds are documented and the empty-subject-tokens early return is pinned.
2 participants
PR #362's last push looked reviewed but wasn't: run 32418662895 blew the 20-minute Bash ceiling during claim validation, so no dispatch-result.json was written, no submission plan could be composed, and no review posted. The run stayed green (the agent job itself didn't fail), and the incomplete report's fallback (filing an issue) is unavailable here because issues are disabled on Khan/actions. The death was invisible on the PR.
So let's just make the orchestrator say so: when
dispatch-result.jsonis missing after the dispatcher call, it posts one standaloneadd-commentstating the review died mid-dispatch and posted no review, links the run, and notes the next push retries; then it reports incomplete, skips Steps 4-8, and continues at Step 9.The death comment collapses the standing risks/patterns guidance comment (
hide-older-commentsmatches on the hidden marker regardless of body), the same thing a hold comment does, and the hold path compensates by droppingrisksPatternsKeyso the next run reposts the guidance. The no-plan death path used to return at theplan === undefinedskip before that compensation could run, socache-record.tsnow recognizes a queuedadd_commentwith no plan staged and drops the key through the same helper the hold branch uses. Fingerprints stay untouched, so the next run reviews in full. One divergence from the hold branch: an unreadable queue stays a benign skip rather than a loud refusal, because with no plan asserting a comment should have queued, it's indistinguishable from an ordinary early death.Gate-wise the shape passes without gate changes: the conformance rules only fire on submit/comment/resolve items, and
add-commentwith nothing else queued passes (same path the HOLD_FOR_HUMAN comment uses;add-commentmax is 1, and Steps 7/8 are skipped so it's the only one). A backstop test now pins that shape green, sinceadd_commentisn't inKEEP_ITEM_TYPESand a violation would strip the death notice.The ceiling itself is raised separately in #369 (1200000 to 1800000 plus timeout-minutes 40 to 50); this PR is only about not failing silently when a call still dies. Changeset is minor: new posting behavior on a previously silent path.