Repository navigation
feat: Solve oracle stops on missing command without spending generation/tier ladder (#542 slice 1) - #557
Conversation
Review findings (blocking)1 finding(s): 1 major. Major
These findings persist after 2 fix attempt(s) — needs human review: #557 |
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 1dbe7bc65f55 · formal
Low-risk PR: moves a missing-command gate into coder_seam, adds an OracleMissingCommand exception with handlers, and ships a focused test. The panel raised two nits; verification refuted the import-style nit (the new file actually uses the correct bare form) and confirmed the changelog filename nit. One nit remains: the changelog filename 542-oracle.fix.md mixes the repo's two naming conventions. Nothing blocks merge.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| ⚪ | nit | changelog.d/542-oracle.fix.md |
Changelog filename mixes the repo's two naming patterns (hyphen-slug and dot-type) with a dot inside the slug, inconsistent with all existing entries. | confirmed |
findings JSON (machine-readable)
[
{
"file": "changelog.d/542-oracle.fix.md",
"line": 0,
"severity": "nit",
"category": "conventions",
"claim": "Changelog filename mixes the repo's two naming patterns (hyphen-slug and dot-type) with a dot inside the slug, inconsistent with all existing entries.",
"evidence": "New file is `changelog.d/542-oracle.fix.md`; existing files follow `3405.fixed.md` (issue+type) or `3832-source-issue-guard-keeps-work.md` (issue+hyphen-slug) \u2014 none have a dot mid-slug.",
"verdict": "confirmed",
"note": "Directory listing at head SHA shows patterns: `<issue>.md`, `<issue>.<type>.md` (3405.fixed.md), `<issue>-<slug>.md` (3832-source-issue-guard-keeps-work.md). `542-oracle.fix.md` uniquely combines hyphen-slug + dot-type; no existing file does."
}
]
There was a problem hiding this comment.
Promoting the PASS verdict for head 1dbe7bc65f55: 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
changelog.d/542-oracle.fix.md:0— Changelog filename mixes the repo's two naming patterns (hyphen-slug and dot-type) with a dot inside the slug, inconsistent with all existing entries.
Approving a WARN does not resolve its findings (issue #22).
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 3b612cdf05a2 · formal
No findings were raised, so there was nothing to verify. The panel (all four LLM angles plus the structural engine) found no defects in this refactor. The one prior-round nit (changelog filename pattern) remains open in its own thread and is not carried here. No coverage gaps noted.
No findings — the review came back clean.
findings JSON (machine-readable)
[]
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 2c7c3e7c3664 · formal
No findings were raised, so there was nothing to verify. The panel (four LLM lanes plus protoPatch) reviewed the PR's changes to move gate-command constants into coder_seam.py, add the OracleMissingCommand exception path, and wire up drive/salvage handling. No correctness, removed-behavior, cross-file, or convention defects surfaced. The single protoPatch finding (a pre-existing AttributeError risk in _resolve_gate_cmd) was dropped because that function is not modified by this diff. No coverage gaps; all changed files were read by multiple lanes.
No findings — the review came back clean.
findings JSON (machine-readable)
[]
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head d2ad20bb5488 · formal
PR #557 adds an OracleMissingCommand exception path to distinguish a missing shell command from a test failure in the oracle. All production-code findings came back clean across multiple angles; the only surviving item is a nit on the changelog filename, confirmed by the verifier. No prior blocker/major items are open. No coverage gaps: every changed file was examined by at least two finders.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| ⚪ | nit | changelog.d/542-oracle.fix.md |
Changelog filename mixes the repo's two non-dominant naming patterns (hyphen-slug and dot-type suffix); the dominant pattern is bare .md. | confirmed |
findings JSON (machine-readable)
[
{
"file": "changelog.d/542-oracle.fix.md",
"line": 0,
"severity": "nit",
"category": "conventions",
"claim": "Changelog filename mixes the repo's two non-dominant naming patterns (hyphen-slug and dot-type suffix); the dominant pattern is bare <number>.md.",
"evidence": "Base-ref changelog.d/ entries: 396.md, 417.md, 419.md, 421.md, 424.md, 426.md, 428.md, 429.md, 430.md, 431.md, 432.md, 433.md, 434.md, 435.md, 436.md, 437.md, 438.md, 439.md, 440.md \u2014 all bare <number>.md. PR adds `542-oracle.fix.md` which has both a hyphen-slug and a dot-type suffix.",
"verdict": "confirmed",
"note": "Diff confirms new file `542-oracle.fix.md`. Base-ref main/ has 21 bare-number files vs. 1 dot-type (3405.fixed.md) and 1 hyphen-slug (3832-source-issue-guard-keeps-work.md); bare <number>.md is clearly dominant. The new name mixes both minority patterns."
}
]
There was a problem hiding this comment.
Promoting the PASS verdict for head d2ad20bb5488: 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
changelog.d/542-oracle.fix.md:0— Changelog filename mixes the repo's two non-dominant naming patterns (hyphen-slug and dot-type suffix); the dominant pattern is bare .md.
Approving a WARN does not resolve its findings (issue #22).
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 694f95799854 · formal
Clean PR. The panel found a single nit (inconsistent test-helper import style, confirmed by the verifier); no correctness, regression, or cross-file issues were raised across the heavily-changed production files. Verification confirmed the one finding with no refutations or drops. No prior blocker/major items are open. No coverage gaps identified by the panel.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| ⚪ | nit | tests/test_oracle_missing_command_542.py:10 |
Inconsistent import style for shared test helpers: some test files use from tests.test_loop import … while others use from test_loop import …, the latter r… |
confirmed |
findings JSON (machine-readable)
[
{
"file": "tests/test_oracle_missing_command_542.py",
"line": 10,
"severity": "nit",
"category": "conventions",
"claim": "Inconsistent import style for shared test helpers: some test files use `from tests.test_loop import \u2026` while others use `from test_loop import \u2026`, the latter relying on pytest's implicit path injection rather than the explicit `pythonpath` setting.",
"evidence": "tests/test_oracle_missing_command_542.py:10 \u2014 flagged by structural analysis (protopatch confidence: high). The new file's import of `_adapter` from `test_preflight_oracle_456_459` follows the bare-module style, inconsistent with `test_merged_verify_infra.py` which uses the `tests.`-prefixed form.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read both files at head: test_oracle_missing_command_542.py:10 has `from test_loop import FEATURE, FakeLoopStore, _drive_with` (bare) while test_merged_verify_infra.py:23 has `from tests.test_loop import _aret, _VerifyStore, _vloop` (prefixed). The inconsistency is real."
}
]Addressed since the last round (1 minor/nit): changelog.d/542-oracle.fix.md:0 — no longer raised, and the PR changed the code around them since they were raised.
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 44fa8b65764f · formal
The PR adds an OracleMissingCommand exception path, refactors shared helpers into coder_seam.py, and introduces a 443-line test file. Verification confirmed the single panel finding (a nit on cross-test-file private helper imports). No blocker or major issues were raised; production code received four independent clean passes. The one thing to fix first is nothing blocking — the nit on import coupling is a maintainability suggestion, not a defect. No panel disagreement. No coverage gaps: the verifier annotated all findings (n=1 of 1).
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| ⚪ | nit | tests/test_oracle_missing_command_542.py:12 |
Cross-test-file coupling via private helper imports creates a fragile import graph; shared fixtures should be extracted into a common test utilities module. | confirmed |
findings JSON (machine-readable)
[
{
"file": "tests/test_oracle_missing_command_542.py",
"line": 12,
"severity": "nit",
"category": "conventions",
"claim": "Cross-test-file coupling via private helper imports creates a fragile import graph; shared fixtures should be extracted into a common test utilities module.",
"evidence": "Four of the twelve owned test files import private helpers (prefixed with underscore) from sibling test files: `_FakeBudget`, `_FakeResult`, `_FakeVerdict`, `_fake_coder`, `_repo` from `test_oracle_timeout_keep_tree`; `_adapter` from `test_preflight_oracle_456_459`; `FEATURE`, `FakeLoopStore`, `_drive_with` from `test_loop`; and `_aret`, `_VerifyStore`, `_vloop` from `tests.test_loop`.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Line 12: `from test_loop import FEATURE, FakeLoopStore, _drive_with`; line 14: `from test_oracle_timeout_keep_tree import _FakeBudget, _FakeResult, _FakeVerdict, _fake_coder, _repo` \u2014 all underscore-prefixed private helpers imported from sibling test files. Pattern corroborated in test_preflight_oracle_456_459.py line 33: `from test_loop import FEATURE, _drive_with, _EscalatingStore, _rung_env`."
}
]Addressed since the last round (1 minor/nit): tests/test_oracle_missing_command_542.py:10 — no longer raised, and the PR changed the code around them since they were raised.
… preflight hold (#542) Review on #557: parking the card in the preflight-hold set let any clean preflight (or restart recovery, which re-adopts 'gate preflight failed' reasons) release it, though the preflight command can differ from the acceptance oracle, so the card re-dispatched into the same missing command and spent generations again. Block under a new non-self-healing oracle-missing class with its own reason prefix; the operator repairs the environment and unblocks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
🛑 Review cap reached — this PR has used its budget of 6 complete automated review round(s). Further pushes will not trigger new reviews. The cap resets when:
|
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head f043f659f493 · formal
No findings were raised, so there was nothing to verify. The panel walked the diff and found no new defects; the one protoPatch nit (cross-test-file helper imports) overlapped an open thread from round 6 and was suppressed per convergence discipline. No coverage gaps.
No findings — the review came back clean.
findings JSON (machine-readable)
[]Addressed since the last round (1 minor/nit): tests/test_oracle_missing_command_542.py:12 — no longer raised, and the PR changed the code around them since they were raised.
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 8ecfe8373f2b · formal
Low-risk PR. The one actionable item is the pytest.fail.Exception reference at line 50 of the new test file — it will raise AttributeError at test execution time; change it to pytest.Failed. The two nits (changelog filename pattern, cross-test-module import coupling) are cosmetic. All three findings were confirmed by the verifier with no severity or claim changes. No panel disagreement. No gaps.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟡 | minor | tests/test_failed_ci_fixture_setup.py:50 |
pytest.fail.Exception is not a valid exception class reference; pytest.fail is a function, not a class, so this will raise AttributeError at test collection ti… | confirmed |
| ⚪ | nit | tests/test_failed_ci_fixture_setup.py:8 |
New test file couples to another test module via direct import and monkeypatching of its module-level references, the same fragile cross-test-file pattern alre… | confirmed |
| ⚪ | nit | changelog.d/542-oracle.fix.md |
Changelog filename mixes the repo's two non-dominant naming patterns (hyphen-slug and dot-type suffix); the dominant pattern is bare .md. | confirmed |
findings JSON (machine-readable)
[
{
"file": "tests/test_failed_ci_fixture_setup.py",
"line": 50,
"severity": "minor",
"category": "correctness",
"claim": "pytest.fail.Exception is not a valid exception class reference; pytest.fail is a function, not a class, so this will raise AttributeError at test collection time.",
"evidence": "Line 50 uses `pytest.fail.Exception` as the expected exception in `pytest.raises`. In all modern pytest versions (5.0+), `pytest.fail` is a function (the `fail()` helper), not a class. The exception class is `pytest.Failed` (capital F). Evaluating `pytest.fail.Exception` raises `AttributeError: function 'fail' has no attribute 'Exception'`.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read the file at head: line 50 is ` with pytest.raises(pytest.fail.Exception, match=\"did not appear\"):`. `pytest.fail` is a function (the `fail()` helper in `_pytest.outcomes`); the exception class is `pytest.Failed`. Accessing `.Exception` on a function object raises `AttributeError` at test execution time (not collection, but the test will fail)."
},
{
"file": "tests/test_failed_ci_fixture_setup.py",
"line": 8,
"severity": "nit",
"category": "conventions",
"claim": "New test file couples to another test module via direct import and monkeypatching of its module-level references, the same fragile cross-test-file pattern already flagged for test_oracle_missing_command_542.py.",
"evidence": "import test_publish_gate_real as tier\n\u2026\nmonkeypatch.setattr(tier.subprocess, \"run\", run)\nmonkeypatch.setattr(tier.asyncio, \"sleep\", sleep)",
"verdict": "confirmed",
"note": "Read the file at head: line 8 is `import test_publish_gate_real as tier`; lines 25\u201326 are `monkeypatch.setattr(tier.subprocess, \"run\", run)` and `monkeypatch.setattr(tier.asyncio, \"sleep\", sleep)`. The cross-test-module import-and-patch pattern is present verbatim."
},
{
"file": "changelog.d/542-oracle.fix.md",
"line": 0,
"severity": "nit",
"category": "conventions",
"claim": "Changelog filename mixes the repo's two non-dominant naming patterns (hyphen-slug and dot-type suffix); the dominant pattern is bare <number>.md.",
"evidence": "Existing entries: 396.md, 417.md, 419.md, 421.md, 424.md, 426.md, 428.md \u2026 440.md (all bare <number>.md). New file: 542-oracle.fix.md",
"verdict": "confirmed",
"note": "Listed changelog.d/ at head: 15 of 17 files are bare `<number>.md` (396, 417, 419, 421, 424, 426, 428, 429, 430, 431, 432, 433, 434, 435, 436); the two exceptions are `3405.fixed.md` and `3832-source-issue-guard-keeps-work.md`. The new `542-oracle.fix.md` mixes both non-dominant patterns."
}
]Converged — the following are notes, not gates. Every finding below is minor/nit and lands on code that changed in response to this panel's own earlier rounds, so the verdict no longer holds on them (issue #23). They are worth doing; they are not worth another review round:
-
tests/test_failed_ci_fixture_setup.py:50(minor) — pytest.fail.Exception is not a valid exception class reference; pytest.fail is a function, not a class, so this will raise AttributeError at test collection time. -
tests/test_failed_ci_fixture_setup.py:8(nit) — New test file couples to another test module via direct import and monkeypatching of its module-level references, the same fragile cross-test-file pattern already flagged for test_oracle_missing_comma -
changelog.d/542-oracle.fix.md:0(nit) — Changelog filename mixes the repo's two non-dominant naming patterns (hyphen-slug and dot-type suffix); the dominant pattern is bare .md.
There was a problem hiding this comment.
Promoting the PASS verdict for head 8ecfe8373f2b: 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:
- minor
tests/test_failed_ci_fixture_setup.py:50— pytest.fail.Exception is not a valid exception class reference; pytest.fail is a function, not a class, so this will raise AttributeError at test collection time. - nit
tests/test_failed_ci_fixture_setup.py:8— New test file couples to another test module via direct import and monkeypatching of its module-level references, the same fragile cross-test-file pattern already flagged for test_oracle_missing_comma - nit
changelog.d/542-oracle.fix.md:0— Changelog filename mixes the repo's two non-dominant naming patterns (hyphen-slug and dot-type suffix); the dominant pattern is bare .md.
Approving a WARN does not resolve its findings (issue #22).
…on/tier ladder (#542 slice 1)
…on/tier ladder (#542 slice 1)
…on/tier ladder (#542 slice 1)
…r output (#542) A path-scoped composed oracle turned a component's exit 127/9009 into 1, and a later component's output could push the shell's not-found diagnostic off the tail, so solve kept spending generations on an oracle that could not run. The composed exit is now sticky 127 when any component could not be found; ordinary failures stay 1. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on/tier ladder (#542 slice 1)
…on/tier ladder (#542 slice 1)
… preflight hold (#542) Review on #557: parking the card in the preflight-hold set let any clean preflight (or restart recovery, which re-adopts 'gate preflight failed' reasons) release it, though the preflight command can differ from the acceptance oracle, so the card re-dispatched into the same missing command and spent generations again. Block under a new non-self-healing oracle-missing class with its own reason prefix; the operator repairs the environment and unblocks. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…on/tier ladder (#542 slice 1)
…on/tier ladder (#542 slice 1)
2349a2b to
0d2931c
Compare
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 0d2931c1425f · formal
Low overall risk: two minor test-file defects, no production-code regressions in the diff. Fix first: the pytest.fail.Exception reference at tests/test_failed_ci_fixture_setup.py:50 will raise AttributeError at test execution and block the suite. The panel raised no disagreements. Verification confirmed all three synthesized findings but noted that the loop/attach.py TypeError is pre-existing code outside this PR's diff, so it is dropped from the final list. No gaps.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟡 | minor | tests/test_failed_ci_fixture_setup.py:50 |
pytest.fail.Exception is not a valid exception class reference; pytest.fail is a function, so evaluating this expression raises AttributeError at test executio… | confirmed |
| 🟡 | minor | tests/test_publish_gate_real.py:383 |
The original _wait_for_completed_fixture (line 383) is dead code: the PR adds a byte-for-byte identical second definition at line 443, which shadows it at modu… | confirmed |
findings JSON (machine-readable)
[
{
"file": "tests/test_failed_ci_fixture_setup.py",
"line": 50,
"severity": "minor",
"category": "correctness",
"claim": "pytest.fail.Exception is not a valid exception class reference; pytest.fail is a function, so evaluating this expression raises AttributeError at test execution time.",
"evidence": "with pytest.raises(pytest.fail.Exception, match=\"did not appear\"):",
"source": "protopatch",
"verdict": "confirmed",
"note": "Code confirmed at line 50: `with pytest.raises(pytest.fail.Exception, match=\"did not appear\"):`. `pytest.fail` is a plain function (defined in `_pytest.outcomes`); it has no `.Exception` attribute. Evaluating the expression raises `AttributeError` at test execution time. The correct reference is the `Failed` exception class (e.g. via `pytest.fail.Exception` does not exist; use the exception raised by `pytest.fail()` directly or import `Failed`)."
},
{
"file": "tests/test_publish_gate_real.py",
"line": 383,
"severity": "minor",
"category": "conventions",
"claim": "The original _wait_for_completed_fixture (line 383) is dead code: the PR adds a byte-for-byte identical second definition at line 443, which shadows it at module load time. Flagged by both conventions and structural review.",
"evidence": "Line 383: `async def _wait_for_completed_fixture(run_id: str, *, timeout: float = 600) -> int:` \u2026 Line 443: `async def _wait_for_completed_fixture(run_id: str, *, timeout: float = 600) -> int:` \u2014 both bodies are identical (`deadline = time.monotonic() + timeout` \u2026 `return attempt`). The first is unreachable.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Read lines 375\u2013460 of test_publish_gate_real.py at PR head: two identical `async def _wait_for_completed_fixture(run_id: str, *, timeout: float = 600) -> int:` definitions at lines 383 and 443 with identical docstrings and bodies. Python module-level redefinition means the second shadows the first; the first is dead code."
}
]Converged — the following are notes, not gates. Every finding below is minor/nit and lands on code that changed in response to this panel's own earlier rounds, so the verdict no longer holds on them (issue #23). They are worth doing; they are not worth another review round:
-
tests/test_failed_ci_fixture_setup.py:50(minor) — pytest.fail.Exception is not a valid exception class reference; pytest.fail is a function, so evaluating this expression raises AttributeError at test execution time. -
tests/test_publish_gate_real.py:383(minor) — The original _wait_for_completed_fixture (line 383) is dead code: the PR adds a byte-for-byte identical second definition at line 443, which shadows it at module load time. Flagged by both conventions
Addressed since the last round (2 minor/nit): tests/test_failed_ci_fixture_setup.py:8, changelog.d/542-oracle.fix.md:0 — no longer raised, and the PR changed the code around them since they were raised.
There was a problem hiding this comment.
Promoting the PASS verdict for head 0d2931c1425f: 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:
- minor
tests/test_failed_ci_fixture_setup.py:50— pytest.fail.Exception is not a valid exception class reference; pytest.fail is a function, so evaluating this expression raises AttributeError at test execution time. - minor
tests/test_publish_gate_real.py:383— The original _wait_for_completed_fixture (line 383) is dead code: the PR adds a byte-for-byte identical second definition at line 443, which shadows it at module load time. Flagged by both conventions
Approving a WARN does not resolve its findings (issue #22).
Summary
Missing-command oracle results now stop solve immediately with infrastructure evidence, without spending another generation or advancing the tier ladder. Drive blocks with diagnostics identifying the project, exact command, missing executable, and
local_gate_cmdrepair action, while preserving the candidate tree for recovery. Ordinary test failures retain existing search and ladder behavior. Deterministic regression tests and a changelog entry cover this change.Refs #542; the follow-up preflight claim gate closes the issue.