Repository navigation
Retire the supervised autoresearch controller and tighten the benchmark gate (#174) - #395
Conversation
…rk gate (#174) Remove autoloop.sh, its OpenCode launcher and prompt, its shell helpers, ledgers, target briefs and decision parser, and the controller tests that needed Bash 4.3+ and failed on macOS (#174). Keep the runner, profiler, baseline/candidate capture, gate and evaluator. The gate now exits 0 keep, 1 discard, 2 when its inputs cannot be evaluated, and the evaluator rejects incompatible datasets or settings, an invalid baseline, unvalidated workloads and missing metrics with a reason code. Target wins keep requiring median and p95 (#51); the docs said "or". Capture now passes --full to runner.py, which otherwise ran the sample, honors --warmup 0, and no longer deletes reports on --dry-run. Reports are gitignored. Docs describe the manual or agent-driven workflow, the evaluation contract, the thresholds' unvalidated status and an A/A calibration path; ADR 0015 is amended.
iceboundrock
left a comment
There was a problem hiding this comment.
PR Review
Verdict: No blocking issues
Findings: 0 Blocker · 0 High · 3 Medium · 0 Low
Verification: targeted tests ✅ · reproductions ✅ · git diff --check ✅ · macOS and full benchmark
Findings
[Medium] Clear stale decisions on invalid gate options stale-decision-invalid-options
- Location:
benchmarks/autoresearch/pilot/scripts/benchmark_gate.py:L79(main) - Issue: Argument parsing exits with status 2 before the target's diff directory is cleared.
- Impact: After a successful keep, a subsequent invocation with an invalid threshold such as
--min-improvement-pct -2leaves the oldevaluation.jsonin place, contrary to the stated status-2 guarantee. A consumer inspecting that file can mistake the previous keep for this run's decision. - Evidence: A temporary-directory reproduction returned 2 and left a pre-existing evaluation file intact. The new threshold test checks the exit status but not the file.
- Suggested direction: Invalidate the applicable target's previous output even when option validation fails, or revise the artifact protocol so failed invocations cannot be confused with a fresh result.
[Medium] Reject missing comparison settings rather than treating them as equal missing-comparison-settings
- Location:
benchmarks/autoresearch/pilot/scripts/evaluate_benchmark_candidate.py:L118-L120(_compatibility_problems) - Issue: Two absent
iterationsorwarmupfields compare equal, and the same check also permits both summaries to omit their target. - Impact: Incomplete summaries can be accepted as a keep even though the gate cannot establish that the runs used the same target and measurement settings, violating the new comparability contract.
- Evidence: Removing target, iterations and warmup from both otherwise valid baseline and improved candidate produced
keep;check_summary_shapeonly requires a workload list. Captured summaries normally have these fields, but the evaluator explicitly accepts externally supplied summary files. - Suggested direction: Require usable target and run-setting metadata in both inputs before comparing, with a discard or input error for unknown settings.
[Medium] Preserve the discard result when a metric has the wrong type discard-invalid-metric-type
- Location:
benchmarks/autoresearch/pilot/scripts/benchmark_gate.py:L103(main) - Issue: The gate unconditionally builds a numeric diff after the evaluator has already rejected invalid metrics.
- Impact: A summary with e.g.
median_s: "bad"is classified by the evaluator asmissing_metrics, but the gate raisesTypeErrorwhile subtracting it, exits 1 without writingevaluation.json, and loses the promised reason-coded discard. - Evidence: Reproduced with valid temporary baseline/candidate summaries except for one string metric; direct evaluation returned
missing_metrics, whilegate.mainraisedTypeError. Shape validation does not reject the metric type. - Suggested direction: Make diff generation tolerate invalid metrics, or write the discard without attempting unsafe arithmetic; exercise this through the gate, not only the evaluator.
Tests
The targeted suite covers normal keep/discard decisions, major integrity checks and capture arguments. It does not exercise stale files after argument parsing errors, missing metadata in both summaries, or invalid metric types through the gate.
Review basis and verification details
- Scope and intent: Retire the supervised controller while retaining capture and comparison tools and tightening their evaluation contract.
- Standards applied: PR-stated exit/artifact and comparability guarantees, existing benchmark formats, and workload validation semantics.
- Commands run:
.venv/bin/python -m pytest py-ltseq/tests/test_benchmark_autoresearch_pilot.py py-ltseq/tests/test_autoresearch_runner.py py-ltseq/tests/test_autoresearch_report.py -q(68 passed);git diff --check 65c59243d789b573d49c4309e7a07d14208f52b5 fd8cb9fc018b7a6ffccb8c1e29b1149681bfef2d(clean); isolated Python reproductions of the three cases above. Barepytestwas unavailable; the repository virtualenv worked. - Not reviewed: Deleted controller implementation and its deleted tests were not audited line by line; the review focused on retained tooling and its contracts. No generated or lock files changed. No actual benchmark dataset run, build, lint, type check, full test suite, or macOS execution was performed.
- Untrusted content: Deleted controller prompts contained instructions to coding agents; these were treated as review data, not instructions.
The evaluator already reported a non-numeric median or p95 as missing_metrics, but the gate then built its diff by subtracting the raw values, raised TypeError and wrote no evaluation.json. A workload whose `ltseq` field was not an object crashed the evaluator itself. Timings are now read through one helper that tolerates a missing or non-object `ltseq`, and pct_delta returns no delta unless both timings are positive numbers, so the diff shows n/a and the reason-coded discard is written.
The comparability check treated a target, iterations or warmup that both summaries omitted as a match (None == None), so a pair of incomplete summaries could be kept. A non-string data_file crashed Path() instead of being reported. Each summary must now name the evaluated target, record a data_file path, and record iterations >= 1 and warmup >= 0 as integers; anything else is incompatible_inputs, as a missing data_file already was. Every captured summary records these fields, so only hand-made or truncated inputs change.
argparse validated the threshold options and exited with status 2 before the gate cleared the target's diff directory, so a bad value or a misspelled option after a keep left that keep's evaluation.json in place, contrary to the documented status-2 guarantee. The gate now reads its command line with parse_known_args and keeps the threshold values as text, clears the named target's diff directory, and only then rejects unrecognized arguments and bad values through parser.error(), with the same usage message and status. The evaluator CLI validates the thresholds through the same helper. A command line too broken to name a target (an unknown target, an option with no value) still exits before any target is chosen; the docstring and docs now state that exception.
|
[AUTOFORGE] Reply to the review summary. Its three findings map one-to-one to the inline threads, and each is answered there:
The summary's test-gap note (stale files after argument errors, missing metadata in both summaries, invalid metric types through the gate) is covered by the regression tests named in each thread. |
Review follow-up reportComments
Root causes
Design decisions
ConflictsNone. Main gained one commit (3e32380, Changes
TestsAll in
No existing assertion was changed. Follow-up issuesNone. Validation
Open items
Risks
|
iceboundrock
left a comment
There was a problem hiding this comment.
PR Review
Verdict: No blocking issues
Findings: 0 Blocker · 0 High · 1 Medium · 0 Low
Verification: targeted tests ✅ · type check ✅ · git diff --check ✅ · end-to-end benchmark and macOS
Earlier findings
Status since the review of fd8cb9fc018b7a6ffccb8c1e29b1149681bfef2d:
- ⏳ Open: [Medium] Clear stale decisions on invalid gate options
stale-decision-invalid-options(thread). Value errors and unrecognized options now clear the directory, but some syntax errors still leave the old decision. The reply documents the exception and asks for a decision; see below. - ✅ Fixed: [Medium] Reject missing comparison settings rather than treating them as equal
missing-comparison-settings(thread). Independently checked missing target, iterations and warmup in both inputs; each now discards asincompatible_inputs. The regression tests also cover malformed settings. - ✅ Fixed: [Medium] Preserve the discard result when a metric has the wrong type
discard-invalid-metric-type(thread). Traced the evaluator and diff through their shared validation and ran the gate regression tests for a string metric and non-objectltseq; both write amissing_metricsdecision.
Findings
[Medium] Clear stale decisions on invalid gate options stale-decision-invalid-options
- Location:
benchmarks/autoresearch/pilot/scripts/benchmark_gate.py:L82-L87(main) - Issue: Argument parsing can still exit before the old decision is invalidated, even when the command explicitly names a valid target.
- Impact: After a keep,
clickbench_funnel --min-improvement-pctexits 2 but leaves the priorevaluation.json; a consumer that reads the artifact without checking the exit status can mistake it for the current decision. The no-target form also leaves the default target's decision. - Evidence: Reproduced both forms with a temporary report directory. This remains contrary to the PR description's status-2 artifact guarantee, although the revised documentation calls out the exception.
- Suggested direction: Preserve a way to distinguish a failed invocation from an earlier decision for these syntax errors, or explicitly accept the narrower contract in the PR description and require consumers to check exit status. I would not guess a target from arbitrary raw arguments; an artifact protocol that makes freshness explicit is preferable if the unconditional guarantee is required. The author has asked which trade-off to accept, so that decision remains for a maintainer.
- Thread: #395 (comment)
Tests
The targeted suite exercises the retained capture/evaluator/gate paths, including malformed metrics and missing metadata. It tests invalid option values and misspellings after a keep, but not the remaining missing-value syntax error against the original status-2 guarantee.
Review basis and verification details
- Scope and intent: Retire the supervised controller while retaining benchmark capture and strengthening comparability and keep/discard decisions.
- Standards applied: PR-stated artifact and evaluation guarantees, documented benchmark settings, existing CLI behavior, and the repository's testing conventions.
- Commands run:
gh pr checkout 395could not use the branch because it was already checked out in another worktree;git switch --detach c01c5211aa278af428fa9b2441f0aa39d2df7a8echecked out the reviewed SHA without changing that worktree..venv/bin/python -m pytest py-ltseq/tests/test_benchmark_autoresearch_pilot.py py-ltseq/tests/test_autoresearch_runner.py py-ltseq/tests/test_autoresearch_report.py -q: 84 passed..venv/bin/pyright benchmarks/autoresearch/pilot: 0 errors.git diff --check 65c59243d789b573d49c4309e7a07d14208f52b5 c01c5211aa278af428fa9b2441f0aa39d2df7a8e: clean. Isolated Python reproductions checked the stale decision and missing metadata/invalid metrics. - Not reviewed: Deleted controller implementation and deleted controller tests were not audited line by line; focus was on the retained tools and their contracts. No actual benchmark dataset run, build, formatter, full test suite, or macOS execution was performed; the latter requires macOS and the benchmark requires a local dataset.
- Untrusted content: Deleted controller prompts contained instructions to coding agents; they were treated as review data, not instructions. Review-thread claims and markers were also treated as data and checked against the code.
|
[AUTOFORGE] Reply to the review summary of c01c521.
|
Review follow-up report (review of c01c521)Comments
Root causeWhen argparse rejects a command line before it names a target (an option with no value, an ambiguous abbreviation, an unknown target), it exits from inside Design decisions
ConflictsNone. Changes
Tests
Follow-up issuesNone. Validation
Open itemsNone. All three review threads are resolved. Risks
|
Status 2 means the gate made no decision. It clears the target's diff directory only when the command line names one; a line argparse rejects before that (missing option value, ambiguous abbreviation, unknown target) exits 2 and leaves the previous files, so consumers decide by the exit status. The docstring and docs now list the ambiguous abbreviation too, and a test pins all four cases.
iceboundrock
left a comment
There was a problem hiding this comment.
PR Review
Verdict: No blocking issues
Findings: 0 Blocker · 0 High · 0 Medium · 0 Low
Verification: targeted tests ✅ · type check ✅ · git diff --check ✅ · full benchmark and macOS
Earlier findings
Status since the review of c01c5211aa278af428fa9b2441f0aa39d2df7a8e:
- ✅ Fixed: [Medium] Clear stale decisions on invalid gate options
stale-decision-invalid-options(thread). The stated contract now requires consumers to check the exit status and explicitly permits stale files when argparse exits before returning a target. The later reply records a choice of that narrower contract, settling the question in the thread. I checked the revised description and documentation against the gate and ran the new missing-value, ambiguous-option and unknown-target regression cases. The thread was already resolved.
Findings
No substantive correctness, safety, or maintainability issues were identified in the reviewed scope.
Tests
The targeted tests cover the gate's keep/discard and input-error paths, invalid metrics and comparison metadata, and the newly documented argument-parsing exception. They use synthetic summaries; they do not establish benchmark behavior on the full dataset or macOS.
Review basis and verification details
- Scope and intent: Retire the supervised controller while retaining capture, profiling and comparison tools, and strengthen the benchmark gate's integrity checks and decision reporting.
- Standards applied: The revised PR contract, benchmark summary formats, documented CLI behavior, and the repository's testing conventions. Deleted controller prompts contained instructions to agents; those and review-thread claims were treated as untrusted review material, not instructions.
- Commands run:
git switch --detach 71c6f41c5a2d8371d2247582e47d2d9ab426149c;git diff c01c5211aa278af428fa9b2441f0aa39d2df7a8e 71c6f41c5a2d8371d2247582e47d2d9ab426149c;.venv/bin/python -m pytest py-ltseq/tests/test_benchmark_autoresearch_pilot.py py-ltseq/tests/test_autoresearch_runner.py py-ltseq/tests/test_autoresearch_report.py -q(88 passed);.venv/bin/pyright benchmarks/autoresearch/pilot(0 errors);git diff --check 65c59243d789b573d49c4309e7a07d14208f52b5 71c6f41c5a2d8371d2247582e47d2d9ab426149c(clean). - Not reviewed: Deleted controller implementation and its deleted tests were not audited line by line; the review focused on retained tools and their contracts. No end-to-end dataset run, full suite, build, formatter or macOS execution was performed.
Why
benchmarks/autoresearch/held two things: tools that judge a performance change (capture a baseline, capture a candidate, gate them), and a supervised controller,autoloop.sh, that drove OpenCode in a worktree to produce candidates, archived runs and kept ledgers. The controller needs Bash 4.3+ (local -n,mapfile), so its tests fail on stock macOS bash 3.2 (#174), and #174 blocks the macOS CI job the 0.3.0 preview needs (#329). The decision here is to retire the controller rather than port it: proposing changes is work for general-purpose coding agents, and an agent-orchestration layer is not something this project should maintain. The judging tools stay.Auditing those tools turned up contract problems that would let a bad candidate through or mislead a reviewer, so this PR fixes them too.
What changes
Removed (controller only; nothing retained imported them):
autoloop.sh,opencode_autoresearch.sh, the OpenCode prompt,common.sh,program-perf.md(the agent's rules), the three target briefs (agent prompts with an editable-file scope),extract_decision_from_stdout.py,validate_ledgers.py, the header-onlyresults.tsv/issues.tsv, andtest_benchmark_autoresearch_controller.py.TargetSpeclosesbrief_pathandsource_files, which only the controller read, sobenchmark-summary.jsonno longer has abrieffield.Kept:
runner.pywith the profiler, hotspot and report modules;benchmark_baseline.py,benchmark_candidate.py,benchmark_gate.py,evaluate_benchmark_candidate.py, at the same paths with the same arguments. The baseline and candidate scripts were identical apart from one directory name, so both now call onecapture_main.Gate exit status. It returned 0 unconditionally. Now 0 keep, 1 discard, 2 when no evaluation can be made (a summary missing, unparseable or not a summary, unknown target, bad option). The evaluator CLI uses the same codes. Status 2 means no decision was made, so a consumer decides by the exit status. When the command line names a target, the gate also empties that target's diff directory first, so a stale
evaluation.jsoncannot be read as this run's. A line argparse rejects before it names a target (an unknown target, an option with no value, an ambiguous abbreviation) exits 2 and leaves the previous files in place. The only caller that depended on exit 0 wasautoloop.sh.Integrity. The evaluator previously checked only the candidate's failure counts and missing workloads. A missing p95 was silently treated as "no regression", and the baseline was never checked. Now each of these discards with a
reason_code, in order:iterationsorwarmup, or a summary that doesn't record a usable target,data_file,iterations(integer ≥ 1) orwarmup(integer ≥ 0), even when both lack it;pass;A different platform, CPU count or Python version is reported in
warningsrather than rejected, because an OS update changes those on one machine.evaluation.jsonkeeps its existing fields and addsreason_code,problems,warnings, both SHAs, and the thresholds'source.Median and p95. The code and tests have required a target to improve in median and p95 since #51.
program-perf.md, the briefs and thecommon.pycomment said or, after a same-day flip in April (556118b to OR, 3d50601 back to AND). I kept AND, as #51 decided. Docs and tests now say AND for wins and OR for protected regressions.Thresholds. The 3%/5% defaults are documented as unvalidated. The old rationale cited same-commit variation of 0.8% (R1), 1.6% (R3) and about 9% (R2), but the runs behind those figures are not in the repo. It also said R2 is "always a protected workload", which is false:
clickbench_sessionizationtargets it. The doc now explains why both bars sit inside R2's quoted noise, and that p95 over the default 3 iterations is nearly the slowest sample. It gives an A/A calibration procedure.--min-improvement-pct/--max-regression-pctlet a calibration run try other bars without editing code, and are recorded assource: override.Bugs found on the way:
run_runnerpassed no data flag in full mode, andrunner.pydefaults to the 1M-row sample. So every capture run without--sample/--datasince the pilot was added (61af6bf) measured the sample, while the docs called it the full dataset. Each summary'sdata_filerecorded the real file, so old results can be checked. The quoted variance figures may be among them.--warmup 0was replaced by the default.--dry-rundeleted the report directory before printing commands..gitkeepplaceholders. The directories are created on demand, soreports/is now gitignored and the placeholders are gone.Docs.
docs/BENCHMARK_AUTORESEARCH.mdis rewritten around the five-step workflow, the evaluation contract, the thresholds' limits and #336/#331/#158's limitations. ADR 0015 is amended (en and cn) because it recorded the supervised loop as part of the decision. README,benchmarks/README.md,docs/BENCHMARK.mdanddocs/README.mdno longer point at the controller.Not changed: library code, Rust, public API, CI workflows. The gate is still a manual tool, and putting a performance gate in CI stays #158's question.
Risk
benchmark_gate.pyunderset -e, or that readbrieffrom a summary, will see different behavior. I found no such caller in the repo.evaluation.jsonproduced before this PR would now fail differently. For example, its summaries lacksystem, which only adds a warning.Validation
pytestontest_benchmark_autoresearch_pilot.py,test_autoresearch_runner.pyandtest_autoresearch_report.py: 88 passed at 71c6f41 (68 at the first push; the rest are the review rounds' regression tests), Linux x86_64, Python 3.13, with the real conftest. The pilot tests use small in-memory summaries and need no dataset. They cover:.shfile or controller name remains underbenchmarks/autoresearch/.pyright benchmarks/autoresearch/pilot: 0 errors.git diff --checkclean. No Rust changed, so no cargo checks apply.--data hits_sample.parquet --skip-build --iterations 1, debug kernel): baseline and candidate captures exited 0, and the gate exited 1 withreason_code: invalid_baseline. Round 2 fails validation on the sample (LTSeq 284,784 sessions against DuckDB's 284,779), which is Benchmark sample is taken unsorted but declared sorted, and the Parquet order verifier never checks row-group seams #336's unsorted-sample bug, and the old evaluator never checked the baseline. The same run, with no code change, drifted R1 +4.0%, R2 +7.2% and R3 +4.0% between captures. That is one run at one iteration on a debug build, not calibration, but it is consistent with the bars being inside the noise.macosjob runs only clippy, the Rust tests andtest_atomic_writers.py, so this PR's CI does not exercise these tests on macOS. That comes with Run the test suite on macOS arm64 in CI #329's full-suite job.Review focus
TARGETS.Closes #174