feat: Oracle-timeout block really keeps the worktree: dispatch() reaps every candidate before the keep-the-tree code runs (#538 follow-up to #545)

[mabry1985]

Summary

Oracle-timeout block really keeps the worktree: dispatch() reaps every candidate before the keep-the-tree code runs (#538 follow-up to #545) (bd-jgyi)

See the diff for details.

Salvaged from feat-bd-jgyi-oracle-timeout-block-really-keeps-the-wo by the operator — no coder was dispatched (#427). It held: 1 commit(s) that no other branch, tag or remote holds; 1 commit(s) ahead of main

Fixes #538

[mabry1985]

Review findings (blocking)

1 finding(s): 1 major.

Major

• coder_seam.py:2603 [correctness] — confirmed: _pick_salvage_candidate ignores each candidate's verify outcome and keeps the LAST candidate with changes, so on an oracle timeout it can keep a candidate whose tests definitively FAILED (a non-timeout failing verdict from a concurrent best-of-k sibling, or a path-scoped oracle that ran and exited non-zero) while reaping the timed-out, possibly-green candidate the PR exists to preserve — board_salvage_feature then publishes the known-bad build (how often depends on whether candidates from earlier rounds are still on disk at salvage time).
  • evidence: for wt, branch in candidates:
    if not worktree.tree_exists(wt):
    continue
    fallback = (wt, branch)
    try:
    if await worktree.unpublished_work(wt, branch=branch, base=base):
    chosen = (wt, branch)
  • verifier: The PR-head code picks by tree_exists + unpublished_work only, last one wins, and never looks at the verdict info the adapter tracks (_completed_failures, _verify_span, _oracle_by_wt). verify() tests best-of-k siblings at the same time and raises OracleTimeout after the 2nd separate timed-out round, so a sibling that failed outright can be the one kept.

The coder is re-driving a fix for these findings.

#547

[protoreview]

QA panel review — PASS

code-review-structural · head 04cdafa0adbc · formal

Low-risk PR. Production changes in coder_seam.py and loop/drive.py reviewed from four angles and came back clean. The single finding is a confirmed nit: a dead _FakeResult dataclass in the new test file, verified at head 04cdafa. No panel disagreement, no coverage gaps, no prior requests to disposition.

Findings

	Severity	Location	Finding	Verified
⚪	nit	tests/test_oracle_timeout_keep_tree.py:77	The new test file defines a _FakeResult dataclass that is never used anywhere in the file — dead code left in from copying the helper set out of test_coder_s…	⚠️ uncertain

findings JSON (machine-readable)

[
  {
    "file": "tests/test_oracle_timeout_keep_tree.py",
    "line": 77,
    "severity": "nit",
    "category": "conventions",
    "claim": "The new test file defines a `_FakeResult` dataclass that is never used anywhere in the file \u2014 dead code left in from copying the helper set out of test_coder_seam.py.",
    "evidence": "@dataclass\nclass _FakeResult:\n    solution: str | None\n    passed: bool | None\n    rung: str\n    gens_spent: int\n    candidates_tried: int\n    verdict: _FakeVerdict | None = None\n    note: str = \"\"\n\n\u2014 no reference to `_FakeResult` appears anywhere else in the file; the tests only use `_FakeVerdict`, `_FakeBudget`, and the `_fake_coder`/`_oracle_timeout`/`_dispatch_timing_out` helpers.",
    "verdict": "uncertain",
    "note": "Read the full file at head 04cdafa: `_FakeResult` is defined at line 77 but never referenced anywhere else in the 200+ line file. All tests use `_FakeVerdict` (as `_verdict_cls`) and `_FakeBudget` (as `_budget_cls`); no instantiation or type annotation of `_FakeResult` exists. \u2014 absence refuted by a repo-wide search at the reviewed head: referenced at `tests/test_coder_seam.py:125` \u2014 downgraded to uncertain, cannot gate a merge (issue #259)",
    "ungrounded": true,
    "absence_refuted": "tests/test_coder_seam.py:125"
  }
]

---

1 absence finding(s) (“no test” / “dead” / “never called” / “does not exist”) downgraded to uncertain: an absence is a claim about the whole repo, so it is checked by a search of the repo at this head, and a reference found there refutes it (issue #259). Each still stands for a human to judge.

• tests/test_oracle_timeout_keep_tree.py (nit) — referenced at tests/test_coder_seam.py:125

[protoreview]

Promoting the PASS verdict for head 04cdafa0adbc: all checks terminal-green, zero unresolved review threads. (approve-on-green)

Open findings carried by this approval — non-blocking, but they did not go away:

• nit tests/test_oracle_timeout_keep_tree.py:77 — The new test file defines a _FakeResult dataclass that is never used anywhere in the file — dead code left in from copying the helper set out of test_coder_seam.py.

Approving a WARN does not resolve its findings (issue #22).

[protoreview]

QA panel review — PASS

code-review-structural · head ad24a71bf13e · formal

No findings were raised, so there was nothing to verify. The panel's five lanes (correctness, removed-behavior, cross-file, conventions, and the protoPatch structural pass) all came back clean on this diff, and the verifier confirmed nothing-to-verify. The one prior nit (unused _FakeResult dataclass in the test file) is below the blocker/major threshold and carries no disposition requirement. No coverage gaps to flag.

No findings — the review came back clean.

findings JSON (machine-readable)

[]

[protoreview]

Promoting the PASS verdict for head ad24a71bf13e: all checks terminal-green, zero unresolved review threads. (approve-on-green)