Skip to content

Retire the supervised autoresearch controller and tighten the benchmark gate (#174) - #395

Merged
iceboundrock merged 5 commits into
mainfrom
retire-autoloop
Nov 11, 2026
Merged

iceboundrock merged 5 commits into
mainfrom
retire-autoloop

Conversation

@iceboundrock

@iceboundrock iceboundrock commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor

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-only results.tsv/issues.tsv, and test_benchmark_autoresearch_controller.py. TargetSpec loses brief_path and source_files, which only the controller read, so benchmark-summary.json no longer has a brief field.

Kept: runner.py with 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 one capture_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.json cannot 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 was autoloop.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:

  • incompatible inputs: a different target, dataset (name and size), iterations or warmup, or a summary that doesn't record a usable target, data_file, iterations (integer ≥ 1) or warmup (integer ≥ 0), even when both lack it;
  • an invalid baseline;
  • a candidate correctness failure (count or a failed workload validation);
  • an infrastructure failure;
  • a missing workload;
  • a workload that did not validate pass;
  • a missing or non-positive median or p95;
  • a protected regression;
  • no target improvement.

A different platform, CPU count or Python version is reported in warnings rather than rejected, because an OS update changes those on one machine. evaluation.json keeps its existing fields and adds reason_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 the common.py comment 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_sessionization targets 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-pct let a calibration run try other bars without editing code, and are recorded as source: override.

Bugs found on the way:

  • run_runner passed no data flag in full mode, and runner.py defaults to the 1M-row sample. So every capture run without --sample/--data since the pilot was added (61af6bf) measured the sample, while the docs called it the full dataset. Each summary's data_file recorded the real file, so old results can be checked. The quoted variance figures may be among them.
  • --warmup 0 was replaced by the default.
  • --dry-run deleted the report directory before printing commands.
  • Every capture deleted the tracked .gitkeep placeholders. The directories are created on demand, so reports/ is now gitignored and the placeholders are gone.

Docs. docs/BENCHMARK_AUTORESEARCH.md is 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.md and docs/README.md no 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

  • Anyone with a script that ran benchmark_gate.py under set -e, or that read brief from a summary, will see different behavior. I found no such caller in the repo.
  • An evaluation.json produced before this PR would now fail differently. For example, its summaries lack system, which only adds a warning.
  • Removing the controller loses its run-archive and ledger tooling. Nothing used the ledgers: they held only headers.

Validation

  • pytest on test_benchmark_autoresearch_pilot.py, test_autoresearch_runner.py and test_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:
    • each reason code;
    • AND for wins and OR for regressions;
    • gate exit codes 0/1/2 and what each leaves on disk;
    • capture artifacts, and the data flag passed to the runner;
    • the evaluator and capture scripts run as subprocesses from outside the repo;
    • a guard that no .sh file or controller name remains under benchmarks/autoresearch/.
  • Full-suite collection: 23,408 tests, no import errors from the removals.
  • pyright benchmarks/autoresearch/pilot: 0 errors. git diff --check clean. No Rust changed, so no cargo checks apply.
  • An end-to-end run on the local 1M-row sample (--data hits_sample.parquet --skip-build --iterations 1, debug kernel): baseline and candidate captures exited 0, and the gate exited 1 with reason_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.
  • macOS not run locally. No retained test or script calls bash any more, so test: autoresearch controller tests fail on macOS (bash 3.2 lacks 'local -n') #174's three failures cannot occur. CI's macos job runs only clippy, the Rust tests and test_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

  • The order of the integrity checks, and whether a system mismatch should reject rather than warn.
  • The ADR 0015 amendment, which records retiring the controller as a project decision.
  • Whether the target briefs should have stayed as context for coding agents. I removed them because they were controller prompts and duplicate TARGETS.

Closes #174

…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 iceboundrock left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ⚠️ not run (unavailable in this environment)

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 -2 leaves the old evaluation.json in 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 iterations or warmup fields 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_shape only 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 as missing_metrics, but the gate raises TypeError while subtracting it, exits 1 without writing evaluation.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, while gate.main raised TypeError. 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. Bare pytest was 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.
@iceboundrock

Copy link
Copy Markdown
Contributor Author

[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.

@iceboundrock

Copy link
Copy Markdown
Contributor Author

Review follow-up report

Comments

Comment Disposition Summary
Clear stale decisions on invalid gate options fixed-differently (open) Bad option values and unrecognized options now clear the target's diff directory before exiting 2. Command lines that never name a target are a documented exception.
Reject missing comparison settings fixed (resolved) Both summaries must record a usable target, data_file, iterations and warmup. Otherwise the result is incompatible_inputs.
Preserve the discard on a wrong-typed metric fixed (resolved) The diff tolerates malformed timings, so the gate writes the missing_metrics discard instead of raising.
Review summary answered Conversation reply pointing to the three threads.

Root causes

  • Stale decision: argparse validated the option values and exited before the gate cleared the target's diff directory. The status-2 guarantee held only for errors the gate itself raised.
  • Missing settings: the comparability check compared raw .get() values, so a setting absent from both summaries matched as None == None. The target check skipped a missing target outright.
  • Metric type: summaries come from files the gate doesn't control, yet pct_delta and the ltseq lookups assumed numbers and dicts. The evaluator's missing_metrics check ran on one path, while the gate's diff did arithmetic on another. The same assumption also crashed the evaluator itself on a non-object ltseq, a case the review didn't name.

Design decisions

  • Defer option validation instead of guessing the target. The gate parses with parse_known_args, keeps the threshold values as text, clears the named target's directory, and then rejects through parser.error(). That keeps argparse's usage message and status 2. Two alternatives were rejected. Scanning raw argv for a target name can clear the wrong target's results. Declaring the options nargs="?" would make --help show their values as optional. The cost is the remaining exception: an unknown target, an option with no value, or an ambiguous abbreviation exits 2 before any target is chosen and changes nothing. The docstring and docs/BENCHMARK_AUTORESEARCH.md say so. That thread stays open in case you'd rather pay one of the rejected costs.
  • Unusable settings are incompatible_inputs, not an input error (status 2). A missing data_file was already a reason-coded discard, so missing or invalid target, iterations and warmup follow it. "Usable" means an integer, iterations ≥ 1 and warmup ≥ 0, matching what bench_vs.py writes. Every captured summary, pre-PR ones included, records these fields.
  • Fix the diff, not the gate's control flow. Making pct_delta return None unless both timings are positive numbers keeps benchmark-diff.json written for every evaluable pair, with n/a where a timing is malformed. One is_positive_number and one ltseq_metric in common.py now serve both the evaluator and the diff, so the two paths can't disagree again.

Conflicts

None. Main gained one commit (3e32380, test_gil_release.py only), which merges cleanly, so no merge commit was made.

Changes

  • 84b299f: malformed timings no longer crash the diff or the evaluator.
  • 69c5ad4: both summaries must record usable comparison settings.
  • c01c521: clear the previous decision before rejecting options. The evaluator CLI shares the new threshold_overrides helper.

Tests

All in py-ltseq/tests/test_benchmark_autoresearch_pilot.py. Each new test failed before its fix.

  • test_candidate_integrity_failures_discard: two cases added, a string median and a non-object ltseq.
  • test_gate_writes_the_discard_for_a_malformed_metric (new): through gate.main, checks exit 1, the missing_metrics evaluation and n/a in the diff.
  • test_settings_missing_from_both_summaries_discard (new): 8 cases where both summaries lack or corrupt the same field.
  • test_gate_usage_errors_clear_the_previous_decision (new): a negative value, a non-numeric value and a misspelled option, each after a keep. Checks exit 2, the message, and an empty diff directory.
  • test_gate_help_leaves_the_previous_decision (new): --help exits 0 and keeps the files.
  • test_evaluator_cli_exit_statuses: one case appended, --min-improvement-pct 0 exits 2 with the argparse-style message.

No existing assertion was changed.

Follow-up issues

None.

Validation

  • Baseline before changes: pytest test_benchmark_autoresearch_pilot.py test_autoresearch_runner.py test_autoresearch_report.py gave 68 passed. Final: 84 passed (Linux x86_64, Python 3.13.5).
  • pyright benchmarks/autoresearch/pilot: 0 errors. git diff --check fd8cb9f..HEAD: clean.
  • Manual CLI check: benchmark_gate.py clickbench_funnel --min-improvement-pct (no value) exits 2 and leaves reports/ unchanged. --help renders as before.
  • Not run: ruff (no project config, not installed); the full suite, which CI runs; the Rust checks, since no Rust changed.
  • CI on c01c521: macos passed. About 11 minutes after the push, build-and-test (3.13) had passed clippy and the Rust tests and was still installing Python dependencies, so the Python test step that runs these tests hadn't started. build-and-test (3.14) and memory-harness were still pending.

Open items

  • The stale-decision thread: is the documented exception acceptable, or should the gate cover argparse syntax errors at the cost of argv guessing or optional-looking option values?

Risks

  • The gate now accepts option abbreviations through parse_known_args, as before, but an ambiguous abbreviation joins the documented exception.
  • The PR description's integrity list still says "a different target, dataset (name and size), iterations or warmup". It doesn't say that a value missing from both summaries is now also rejected. I left the description unedited; the contract table in the docs is current.
  • No instructions aimed at agents were found in the PR content addressed here. The deleted controller prompts were not acted on.

@iceboundrock iceboundrock left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ⚠️ not run (dataset and macOS unavailable)

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 as incompatible_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-object ltseq; both write a missing_metrics decision.

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-pct exits 2 but leaves the prior evaluation.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 395 could not use the branch because it was already checked out in another worktree; git switch --detach c01c5211aa278af428fa9b2441f0aa39d2df7a8e checked 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.

@iceboundrock

Copy link
Copy Markdown
Contributor Author

[AUTOFORGE] Reply to the review summary of c01c521.

  • stale-decision-invalid-options is needs-decision. The thread reply asks one question. A: accept the narrower contract (consumers check the exit status). B: on a usage error that names no target, clear every target's diff directory. Those files are derived and rebuildable. I recommend A, and I've made no change until a maintainer picks one.
  • The two findings marked fixed (missing-comparison-settings, discard-invalid-metric-type) need nothing further.
  • The test gap you noted, no test of the missing-value syntax error against the status-2 guarantee, depends on that answer. Under A, a test pins the exception: exit 2 and the old directory untouched. Under B, a test checks that the directories are cleared.

@iceboundrock

iceboundrock commented Nov 11, 2026 •

Copy link
Copy Markdown
Contributor Author

Review follow-up report (review of c01c521)

Comments

Comment Disposition Summary
Clear stale decisions on invalid gate options fixed (resolved) The maintainer chose A, the narrower contract. Status 2 means no decision was made, and a consumer reads the exit status. The diff directory is emptied only when the command line names a target. The docs, the docstring and the PR description now say so, and a test pins the exception. Reply.
Review summary answered Conversation reply. The two findings already marked fixed need nothing further. The missing-value test the summary asked for is now in 71c6f41.

Root cause

When 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 parse_known_args. The gate never learns which directory to clear. The PR description had promised an empty directory on every status 2, which the code can't keep for these lines without picking a target itself.

Design decisions

  • The maintainer chose A over B. B would clear all three targets' diff directories whenever a usage error names no target. A keeps the code as it was and narrows what status 2 promises: no decision was made, and the directory is emptied only when the command line names a target. Nothing in the repo reads evaluation.json now that autoloop.sh is gone, and the person running the gate by hand sees argparse's error.
  • Reading the target from a partially parsed namespace was rejected. argparse fills in the default target before it parses, so a line that fails early would clear the default target's directory instead of the one named later.

Conflicts

None. main has one commit the branch lacks (3e32380, test_gil_release.py only). GitHub reports the PR mergeable, so I made no merge commit.

Changes

  • 71c6f41: the gate's docstring and docs/BENCHMARK_AUTORESEARCH.md add the ambiguous abbreviation to the documented exception and say to check the exit status, not the files.
  • PR description, edited in place: the status-2 sentence states the narrower contract. The integrity list now says that a summary missing a usable target, data_file, iterations or warmup is rejected even when both summaries lack it, which was the gap noted last round. The test count is 88.

Tests

  • New: test_gate_usage_errors_before_a_target_leave_the_previous_decision. After a keep, four command lines (clickbench_funnel --min-improvement-pct, a bare --min-improvement-pct, --m 2 clickbench_funnel, clickbench_funnle) must each exit 2, print argparse's message, and leave the diff directory byte-for-byte unchanged. I checked that the message substrings match on Python 3.13, 3.14 and 3.15.
  • No existing assertion was changed.

Follow-up issues

None.

Validation

  • Baseline on c01c521: the three targeted test files gave 84 passed. Final on 71c6f41: 88 passed (Linux x86_64, Python 3.13.5).
  • pyright benchmarks/autoresearch/pilot: 0 errors. git diff --check: clean.
  • The full suite was not run: the change is a docstring, a doc paragraph and one test file. CI runs the full suite.
  • CI on 71c6f41: macos passed. build-and-test (3.13), build-and-test (3.14) and memory-harness are still pending; build-and-test usually takes 42–50 minutes.

Open items

None. All three review threads are resolved.

Risks

  • A consumer that judges the result by whether evaluation.json exists, without checking the exit status, can still read a stale decision after one of the four rejected command lines. This is the contract the maintainer accepted, and the docs say it.
  • No instructions aimed at agents were found in the PR content.

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 iceboundrock left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ⚠️ not run (dataset and macOS unavailable)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

test: autoresearch controller tests fail on macOS (bash 3.2 lacks 'local -n')

1 participant