ci(launchpad): run the review-agent unit suites via one CONTROLS row (#575)

[serina-mcfall]

Refs #575

Issue type

Task

Provenance

Field	Value
Harness / provider	Claude Code (Anthropic)
Model	Claude Opus 5
Initiating human	@serina-mcfall

What this does

Seven test_*.py suites sit beside run_controls.py and all of them pass, but nothing ran them. This adds one row to CONTROLS and the small control that row points at.

     ("check_fetch_states.py", False),
+    ("check_unit_suites.py", False),
 ]

No workflow file changes — launchpad-review-agent-controls.yml already filters on launchpad/review-agent/**.

Why one row and not a new workflow

Two records rule out a second CI entry point, and this follows them rather than reopening the question:

• task: run launchpad/agents Python test suites in CI #270's Out of scope: "duplicating it here would create a second CI entry point that task: adjudication pass over every reported finding #118 explicitly rules out."
• launchpad/plans/2026-08-13-issue-118-adjudication.md:187: "STEP 10 appends one row; no second workflow is added."

run_controls.py already runs each entry as subprocess.run([sys.executable, HERE / script]) and treats non-zero as FAIL, so a discovery runner needed no change to the harness.

The control is named check_unit_suites.py, not check_adjudication.py — the #118 plan reserves that name for STEP 10's one-control-per-done-criterion work, and this must not squat it.

Why the gap existed

The escalation dead-ended. #566's review raised it as a High finding recorded against #270; #270 handed it to #118 STEP 10; STEP 10's done-when never mentions test_*.py, and check_adjudication.py does not exist on any branch. Five of the suites already record the gap in their own docstrings.

Verification

The harness picks it up and names it in the summary:

$ python3 run_controls.py
PASS  check_contract.py
...
PASS  check_fetch_states.py
PASS  check_unit_suites.py

14 passed, 0 failed, 0 skipped

Both failure modes proven rather than assumed. A control that cannot fail would reproduce the very defect this closes.

Zero-case discovery — the same code with a pattern matching no files:

FAIL  discovery over test_nothing_matches_*.py collects at least one test case (got 0)

1 failure(s)
exit=1

A genuinely broken assertion — one assertEqual flipped to assertNotEqual in test_verdicts.py, then reverted:

PASS  discovery over test_*.py collects at least one test case (got 176)
FAIL  all 176 test cases pass (exit 1)

FAIL: test_well_formed_pair_validates_clean (test_verdicts.WellFormedPairTests...)
    self.assertNotEqual(verdicts.validate(input_doc, output_doc), [])
AssertionError: [] == []

The guard counts cases, not files: renaming every test_* method to check_* would satisfy a file count while discovery collected zero. That trap is documented in launchpad-agents-tests.yml, whose first version had it.

Cost: 176 cases in 0.78s on this branch (271 on #566's head, which adds suites). No third-party dependency — every import is stdlib or a sibling module.

Confirmed on CI

The controls check on this PR went green, and the log shows the new row running rather than merely a green job — run 32693706845:

PASS  check_fetch_states.py
PASS  check_unit_suites.py

14 passed, 0 failed, 0 skipped

Not verified

• I did not see the control go red on a CI runner — both failure modes were proven locally, not by pushing a deliberate break to this branch. task: run the review-agent unit suites in CI — one row in CONTROLS, no second workflow #575's done-when asks for that; it is the one criterion this PR has not evidenced on CI.
• I did not audit the thirteen existing CONTROLS entries for overlap with the test_*.py cases. Some assertions may be duplicated, which would make this additive coverage rather than wholly new coverage.
• No reviewer has run on this branch. It was opened as a draft for that reason; a ready_for_review event marked it ready at 2026-08-24T05:31:06Z, which was not this agent and is not done by any workflow in the repository. Treat it as awaiting a first review regardless of its draft state.
• I did not change what any test asserts, so anything the new coverage exposes later is a separate finding.

[serina-mcfall]

Review at ef914626b — no blocker. One Medium, one Low.

Agent-authored under @serina-mcfall's instruction. Per launchpad/AGENTS.md:165 — "Draft everything. Approve nothing." — this approves nothing.

Independence, stated up front: this PR is authored under the same identity I am acting as (serina-mcfall, 05:26 today, a different session). I did not write it, and everything below was re-derived rather than taken from the docstring — but this is not an independent review in the org sense, and it should not be the only one.

It is a real check, and I proved that rather than reading it

Three mutations, all caught:

$ python3 check_unit_suites.py                        # as-is
PASS  discovery over test_*.py collects at least one test case (got 176)
PASS  all 176 test cases pass (exit 0)

# rename every `def test_` to `def check_` across all seven suites
# — the filename trap the docstring names:
FAIL  discovery over test_*.py collects at least one test case (got 0)

# flip one assertEqual to assertNotEqual:
PASS  discovery ... (got 176)
FAIL  all 176 test cases pass (exit 1)
FAIL: test_well_formed_document_validates_clean (test_findings...)

The two-step split does what the docstring claims. Counting cases, not files, is the part that matters: the filename mutation leaves seven test_*.py files on disk and a file-counting guard would have reported seven suites while discovery collected zero. That trap is real and the comment at :45-49 is right that launchpad-agents-tests.yml's first version had it.

The integration is sound too, which is where a control like this usually fails. run_controls.py:56-61 runs each control as a subprocess and appends to failed on non-zero; :70-71 returns 1. And :72-75 returns 2 when a control was skipped, with "A skipped control has not run. Do not report a clean gate." — so this row cannot be satisfied by absence.

Every factual claim in the docstring reproduces:

Claim	Check
Seven suites in the directory	git ls-tree | grep -c '/test_[^/]*\.py$' → 7 ✓
CONTROLS listed thirteen entries, none a test_*.py runner	13 on launchpad, none a test module ✓
The one workflow using discover is scoped to launchpad/agents	discover("launchpad/agents", pattern="test_*.py") ✓
plans/2026-08-13-issue-118-adjudication.md:187 — "STEP 10 appends one row; no second workflow is added"	verbatim ✓

Medium — nothing in the chain has a timeout, and the job has none either

$ grep -c timeout check_unit_suites.py run_controls.py .github/workflows/launchpad-review-agent-controls.yml
check_unit_suites.py:0
run_controls.py:1        # ← line 40, and it is on the `gh api rate_limit` probe, not on control invocation
launchpad-review-agent-controls.yml:0

So: check_unit_suites.py:68's subprocess.run has no timeout, run_controls.py:56's control invocation has no timeout, and the workflow job has no timeout-minutes. A single hanging test case runs to GitHub's default job limit — six hours — with no signal beyond a stalled job.

The missing timeouts in run_controls.py and the workflow are pre-existing and not this PR's to fix. What is this PR's is that it is the row that makes a hang reachable: the other twelve controls do bounded, self-contained work, and this one shells out to an arbitrary and growing test suite. A future test that opens a socket, waits on a lock, or patches subprocess.run incompletely turns a red build into a six-hour stall.

Cheap and local: timeout= on the subprocess.run at :68, treating TimeoutExpired as a FAIL with the elapsed budget in the label. security_audit_secrets_check.py already has that shape (_TIMEOUT_SECONDS, except subprocess.TimeoutExpired), so the pattern is in the repo. A timeout-minutes on the job is worth raising separately since it protects all thirteen.

Medium rather than Blocker: the suites are unit tests that mock their I/O, so nothing hangs today. This is about what the row makes possible, not what it does now.

Low — the failure tail can cut the first failure

:77-78 prints proc.stdout[-2000:] and proc.stderr[-4000:]. unittest writes the per-failure tracebacks first and the summary last, so tail-truncation reliably keeps the count and the last failures while dropping the earliest — and the earliest is usually the one to debug. With 176 cases and a handful failing, 4,000 characters is enough today; it is the wrong end to keep if a change ever breaks a dozen.

Not worth its own round trip. If the file is open for the timeout fix, printing the head of stderr alongside the tail (or raising the tail budget) costs a line.

What I would keep exactly as it is

The docstring is the strongest part, and unusually so for a CI control. It records why the escalation dead-ended — #566's review pointed at #270, #270's "Out of scope" handed it to #118 STEP 10, and STEP 10 is one-control-per-done-criterion over adjudication behaviour and never runs test_*.py. That is the reason seven passing suites went unrun, and it is the thing a future reader needs in order not to re-create the gap. "A suite nothing runs is a claim, not a check: it cannot fail, so it cannot protect anything" is the right sentence to have written down.

Two design calls I would not change: one CONTROLS row rather than a second workflow, with the plan-doc line cited rather than asserted; and a subprocess rather than an in-process TextTestRunner, for the two stated reasons — the suites patch process-global state, and a failure reproduces verbatim as the command a contributor runs by hand. Both are argued rather than assumed.

Also correct, and easy to get wrong: an ImportError counts as one case because the loader substitutes _FailedTest, so it passes step 1 and fails step 2. The comment at :51-53 names that explicitly, and it is the right split — "nothing to run" and "does not pass" are different failures.

---

Reviewed at head ef914626b against origin/launchpad. The three mutations, the four docstring claims and the run_controls.py exit path were all run by me directly. Not verified: behaviour on a genuinely hanging test — I reasoned that from the absent timeout rather than inducing a stall, and did not run this under the real workflow.

🤖 Review drafted by Claude Code (claude-opus-5) for @serina-mcfall.

[tucktuck101]

approved