Repository navigation
Conversation
github-actions
Bot
added
documentation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: MUST_FIX: The new timing self-test rejects a successful review when context construction takes more than half a second.
Must fix
1. Make the context-timing test independent of machine speed
tools/review/local.py:855. Kind A: context construction takes one second → the correctly placed timer reports context 1 s → this assertion fails, and self_test() returns 1. Git commands, diagnostics, or a scheduler delay can consume that interval. Nothing bounds it below half a second.
Fix this now to prevent intermittent self-test and CI failures. Replace the timing case at lines 845–856 with a local clock fixture. Its calls represent review start, pre-check start/end, context start/end, and summary. This keeps the timer-boundary check without a real sleep:
from types import SimpleNamespace
from unittest.mock import patch
open(os.path.join(repo, "tools/check-levers.sh"), "w").write("exit 0\n")
sh("git", "add", "-A")
sh("git", "commit", "-qm", "topic: a timing pre-check")
runner("**Verdict: CLEAN**: fine\n\n### Checked\nall")
buf = io.StringIO()
a = argparse.Namespace(base="basebr", full=False, minutes=10,
body=None, no_pre_checks=False)
ticks = iter((0, 0, 10, 10, 11, 11))
fake_time = SimpleNamespace(time=lambda: next(ticks))
with patch.dict(g, time=fake_time), contextlib.redirect_stdout(buf):
rc = review(a, stubbed=True, say=lambda m: None)
ctx_s = re.search(r"context (\d+) s", buf.getvalue())
expect(rc == 0 and ctx_s is not None and int(ctx_s.group(1)) == 1,
f"context time starts after pre-checks: {buf.getvalue()!r}")Non-blocking
⚠ Cover the CLI timeout branch: tools/review/local.py:580.
Both added CLI cases still pass if timeout=15 at line 364 is deleted. Neither exercises TimeoutExpired; that mutation lets a hung CLI hold a slot before the role timer starts.
Add a mocked subprocess case that checks the timeout argument, raises TimeoutExpired, and asserts Stop(75).
Checked
Duplicates: no duplication finding was submitted.
Complexity: no Rust files changed; dryer and crapper did not run.
Silent failure: device generation errors raise Stop(70); CLI timeout and nonzero exit raise Stop(75).
Tests: isolated in-memory checks confirmed CLI success/error/timeout behavior and the timing assertion's false failure. The proposed clock fixture passes the current timer placement and fails when the timer moves before pre-checks.
Gates: pre-checks remain enabled by default; the PR body documents the explicit replay bypass.
Escape catalog: C1–C4 do not match these changes.
Notes
The full self-test was not run because it writes outside OUT. The PR body has no FAIL-first output for the added cases.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
…hecks when asked A binary on PATH that fails claude --version is a broken install: the run ends 75 and names it, before a review. --no-pre-checks skips the text checks and leaves that probe in place. The self-test does not call the installed claude.
…he checks A third pre-review waits on a flock slot. device-dirs.txt is reused for a base sha. The summary's context seconds start once the pre-checks have finished.
The third run polls both flock slots. The device-dir cache is published with replace, so a reader does not see a short file. The local-check table names cargo, which post.py device runs.
cargo metadata in the base tree would otherwise install the pinned nightly. The self-test row is red when that env is missing.
…ecks, so the flag is read directly Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…the toolchain to the repo The timing self-test reads a fake clock. Pre-check time is 10s on that clock and context time is 1s, so the boundary does not depend on a sleep. Moving the context sample to before the pre-checks reports 11s and the test goes red. post.py device inherits the caller's environment. rustup then reads rust-toolchain.toml. install.sh installs only that channel.
A call without a sha skipped the cache and still wrote the device list. The only caller passes the sha. The argument is required, and the test stubs take it the same way.
The SIGTERM child sets HOME to its temp directory, so that run does not take the caller's slots.
…in English A third pre-review prints one line before it polls. claude --version's exit 75 says broken install.
There was a problem hiding this comment.
Verdict: MUST_FIX: The new CLI health check accepts a silent executable.
Must fix
1. Reject an empty successful version response
tools/review/local.py:379 Kind A: a claude stub that only runs exit 0 passes this check without answering. review() then builds the context, the base runner rejects the empty reviewer transcript, and the command exits 70 instead of rejecting the broken installation with 75.
Rejecting it here avoids repeating context generation and diagnostics on each attempt. Check for output as well as status, and add a self-test for empty output with exit 0:
if p.returncode != 0 or not (p.stdout + p.stderr).strip():
raise Stop(75, f"pre-review: claude --version failed (rc {p.returncode}): broken install; nothing was reviewed")Checked
Evidence: followed the CLI response through review(), the base shell runner and the transcript summarizer.
Tests: an in-memory check confirms empty success passes the current guard. The replacement rejects empty or whitespace-only output with 75, accepts version output, and preserves rejection of nonzero status. The existing CLI self-test does not cover empty success.
Inline suggestion: line 379 is added by the diff; the replacement parses as Python.
Escape catalog: no pattern matches this finding. No ledger lessons are in force.
Notes
Dryer and crapper did not run because no Rust files changed. The full self-test was not run because it writes temporary files outside the allowed outputs. The CLI guard and replacement were exercised with mocked subprocess results.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
| stdin=subprocess.DEVNULL, timeout=15) | ||
| except (OSError, subprocess.TimeoutExpired) as e: | ||
| raise Stop(75, f"pre-review: claude --version failed ({type(e).__name__}): broken install; nothing was reviewed") from e | ||
| if p.returncode != 0: |
There was a problem hiding this comment.
An executable that exits 0 without output passes the startup health check; reject silent success as a broken install.
| if p.returncode != 0: | |
| if p.returncode != 0 or not (p.stdout + p.stderr).strip(): |
…oken install A binary that exits 0 with empty or whitespace-only output used to pass the health check. review() then built the context and exited 70 on the empty transcript. It is exit 75, the same as a red --version, before any of that work. Self-test: python3 tools/review/local.py --self-test, 0 failed. The empty and whitespace stubs were red before this change (claude_ok returned).
There was a problem hiding this comment.
Since the last review: finding 1 is fixed at tools/review/local.py:379. Empty and whitespace-only successful responses now raise the named broken-install error with code 75.
Verdict: CLEAN: The previous finding is resolved, and no new defect remains in the incremental diff.
Checked
Duplicates: 0 new functions, types or constants; the added cases share one loop. No dryer pairs were available.
Complexity: no Rust functions changed; no crapper results were available.
Silent failure: 1 changed guard traced through review(); empty output stops the run before context generation.
Tests: the added assertions at tools/review/local.py:623 pass in isolation; both fail when line 379 uses the old guard. Removing strip() fails the whitespace case. Eight mocked output and exception checks pass, including stderr-only version text.
Gates: no thresholds, pins or gate removals changed.
Escape catalog: no patterns matched.
Notes
Dryer and crapper did not run because no Rust files changed. The added test loop ran in memory with subprocess and file operations mocked; the full self-test was not run because it writes outside the permitted output files.
Not approved: this pull request changes tools/review/local.py, which changes the bot, CI or the toolchain and needs a person's approval.
🔨 Shingler, bloomery's review bot: approves a clean pull request and requests changes on a must-fix (how it works)
…aude that does not answer --version refused The squashed #229 on today's main: a third pre-review waits on a flock slot, the device-dir list is cached by the base sha, the context time is measured after the pre-checks, a claude that prints no version ends the run 75, and post.py device runs under the stable toolchain. Main had already moved the ledger read ahead of the pre-checks, so only the device-dir call takes the base sha here. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Landed on main as one commit, e0260b2: this PR's net diff on today's main. Main had moved the ledger read ahead of the pre-checks, so only the device-dir call takes the base sha; one older self-test namespace gained no_pre_checks. local.py --self-test ok (0 failed). Bot-written (Claude Code, on the maintainer's instruction). |
1 participant
What
Two pre-review slots (a third run waits on a flock slot), a device-dir cache by base sha, the context time measured
after the pre-checks, a claude that does not answer --version refused (75), and post.py device under the stable
toolchain.
--no-pre-checkshas a user: the lead adopted it (10-15 05:2x) for replays of old heads, e.g. the reviewer-modelmeasurement that re-reviews six earlier commits. There the pre-checks judge a tree that is no longer the push
(a pre-fix head is red by design) and three recipes self-tests at once took 1271 s. A push never passes it.
Pre-review CLEAN at 67b2cfd.
The bloomery AI assistant wrote this.