A failing test suite passes agent.test.unit: its completion only checks that the report exists

cmoriarty commented

2026-10-03 09:45:59 -04:00

Owner

agent.test.unit completes on paths-exist:.osf/test/unit.json (src/osf/pipeline/nodes.py). After the agent's turn, Braid runs the suite itself (run_declared_lane). It writes a correct verdict to unit.json, with exit_code, green and reasons, but nothing reads that verdict afterwards: the step logs "lane red" and succeeds.

osf.pipeline.apply says this step's completion is suites-green:unit ("exit 0, a clean ban lint, and the mutation gate on the diff"). That predicate exists, and script.test.unit uses it. The agent step has been on paths-exist since the first 22-node pipeline (23349d1).

The session working on #125 found this in run 41's report: exit_code: 0, green: false, 20 ban violations. Every production run that reached the step passed it:

Run	Suite exit	Ban violations	Scenarios with no test	Outcome
run 22	1	8	29	merged
run 23	0	1	14	merged
run 25	1	11	30	merged
run 26	1	0	17	merged
run 36	0	19	55	merged
run 37	0	20	23	failed later, at its code review
run 38	0	9	0	failed later, at the merge
run 39	0	18	0	abandoned
run 41	0	20	0	merged

Three runs merged with a red suite. The only green report (run 33) is from a quick fix whose agent unit step was skipped. Runs 39 and 41 have no untested scenarios because #121 added the spec tags.

The ban lint is not ready to block

In run 41, all 20 findings are on good tests:

B2 ×9, "existence-only assertion as the sole assertion":
- Six are "shows no X" tests, for example expect(screen.queryByRole("button", { name: "Start the next hand" })).toBeNull(). They fail if the control appears.
- Three check a parser's verdict, for example expect(parseState(without)).toBeNull() in "rejects a payload missing the hand_result field".
B1 ×1, "only subject is a mock or spy": the Testing Library pattern. getByRole(...) throws when the button is missing, fireEvent.click presses it, then expect(onStartNextHand).toHaveBeenCalledTimes(1). It does not pass with the module deleted.
B6 ×10, "every describe carries a spec tag": grouping blocks such as describe("parseState", ...). The tests inside are tagged, and coverage is 31 of 31 scenarios.
B7 misfires on a multi-line TypeScript import (#121, comment 2497).

Blocking on the lint as it is would stop runs over good tests, and push agents to make them worse to get past it.

Proposal

Block on the suite now. agent.test.unit holds only when the report's exit code is 0 and the report is fresh (its tree is the worktree's). This could be an argument to suites-green that leaves the lint and coverage advisory. predicate_failed is already mediated, so a red suite gets two more attempts briefed with the failing output, then goes to a person instead of merging.
Fix the lint's precision.
- B2 accepts an absence assertion (toBeNull() or not.toBeInTheDocument() on a query) and a function's return value as behaviour.
- B1 counts Testing Library getBy* and findBy* queries as assertions.
- B6 accepts an untagged describe whose tests are tagged.
- B7 handles multi-line imports.
Measure the result against the tests of runs 22 to 41.
Then make the lint and scenario coverage block too (suites-green:unit in full). State the rules in the apply brief as well: most tests are written during apply, and today the rules are only in the test step's brief.

Related: #125 (the verification steps redo each other's work).

`agent.test.unit` completes on `paths-exist:.osf/test/unit.json` (`src/osf/pipeline/nodes.py`). After the agent's turn, Braid runs the suite itself (`run_declared_lane`). It writes a correct verdict to `unit.json`, with `exit_code`, `green` and `reasons`, but nothing reads that verdict afterwards: the step logs "lane red" and succeeds. `osf.pipeline.apply` says this step's completion is `suites-green:unit` ("exit 0, a clean ban lint, and the mutation gate on the diff"). That predicate exists, and `script.test.unit` uses it. The agent step has been on `paths-exist` since the first 22-node pipeline (23349d1). The session working on #125 found this in run 41's report: `exit_code: 0`, `green: false`, 20 ban violations. Every production run that reached the step passed it: | Run | Suite exit | Ban violations | Scenarios with no test | Outcome | |---|---|---|---|---| | run 22 | **1** | 8 | 29 | merged | | run 23 | 0 | 1 | 14 | merged | | run 25 | **1** | 11 | 30 | merged | | run 26 | **1** | 0 | 17 | merged | | run 36 | 0 | 19 | 55 | merged | | run 37 | 0 | 20 | 23 | failed later, at its code review | | run 38 | 0 | 9 | 0 | failed later, at the merge | | run 39 | 0 | 18 | 0 | abandoned | | run 41 | 0 | 20 | 0 | merged | Three runs merged with a red suite. The only green report (run 33) is from a quick fix whose agent unit step was skipped. Runs 39 and 41 have no untested scenarios because #121 added the spec tags. ## The ban lint is not ready to block In run 41, all 20 findings are on good tests: - **B2 ×9**, "existence-only assertion as the sole assertion": - Six are "shows no X" tests, for example `expect(screen.queryByRole("button", { name: "Start the next hand" })).toBeNull()`. They fail if the control appears. - Three check a parser's verdict, for example `expect(parseState(without)).toBeNull()` in "rejects a payload missing the hand_result field". - **B1 ×1**, "only subject is a mock or spy": the Testing Library pattern. `getByRole(...)` throws when the button is missing, `fireEvent.click` presses it, then `expect(onStartNextHand).toHaveBeenCalledTimes(1)`. It does not pass with the module deleted. - **B6 ×10**, "every describe carries a spec tag": grouping blocks such as `describe("parseState", ...)`. The tests inside are tagged, and coverage is 31 of 31 scenarios. - **B7** misfires on a multi-line TypeScript import (#121, comment 2497). Blocking on the lint as it is would stop runs over good tests, and push agents to make them worse to get past it. ## Proposal 1. **Block on the suite now.** `agent.test.unit` holds only when the report's exit code is 0 and the report is fresh (its tree is the worktree's). This could be an argument to `suites-green` that leaves the lint and coverage advisory. `predicate_failed` is already mediated, so a red suite gets two more attempts briefed with the failing output, then goes to a person instead of merging. 2. **Fix the lint's precision.** - B2 accepts an absence assertion (`toBeNull()` or `not.toBeInTheDocument()` on a query) and a function's return value as behaviour. - B1 counts Testing Library `getBy*` and `findBy*` queries as assertions. - B6 accepts an untagged `describe` whose tests are tagged. - B7 handles multi-line imports. Measure the result against the tests of runs 22 to 41. 3. **Then make the lint and scenario coverage block too** (`suites-green:unit` in full). State the rules in the apply brief as well: most tests are written during apply, and today the rules are only in the test step's brief. Related: #125 (the verification steps redo each other's work).

cmoriarty commented

2026-10-03 09:55:19 -04:00

Author

Owner

Also in scope (folded in at the user's word): the error after a second cut-off still says the model was cut off "the second time after being asked to answer without thinking" (_cut_off in src/osf/engine/executors.py). Since #136, the re-prompt after a cut-off thinks briefly (the brief variant), so the text should say that.

Work starts after the session implementing #125, #126, #127, #114, #113 and #73 has pushed. It also edits nodes.py and prompts.py.

Also in scope (folded in at the user's word): the error after a second cut-off still says the model was cut off "the second time after being asked to answer without thinking" (`_cut_off` in `src/osf/engine/executors.py`). Since #136, the re-prompt after a cut-off thinks briefly (the `brief` variant), so the text should say that. Work starts after the session implementing #125, #126, #127, #114, #113 and #73 has pushed. It also edits `nodes.py` and `prompts.py`.

cmoriarty commented

2026-10-03 11:12:30 -04:00

Author

Owner

Corrections after checking this against the code. They are recorded in the proposal, test-lanes-hold-on-the-verdict, which is waiting for review before any code changes.

B7 is already fixed. Its misfire on multi-line imports was fixed in #121, and runs 39 and 41 have no B7.
19 of run 41's 20 findings are on sound tests, not all 20. The weak one is an older test whose only check of "accepts an empty seat" is expect(parseState(…)).not.toBeNull().
15 of the 20 are on tests written before run 41, in files run 41 touched, because the lint reads whole files. The 5 on run 41's own tests are all sound. The proposal narrows the lint to the tests a run adds or changes.
agent.test.e2e has the same hole. It also completes on paths-exist, so it is included.

Corrections after checking this against the code. They are recorded in the proposal, `test-lanes-hold-on-the-verdict`, which is waiting for review before any code changes. - **B7 is already fixed.** Its misfire on multi-line imports was fixed in #121, and runs 39 and 41 have no B7. - **19 of run 41's 20 findings are on sound tests, not all 20.** The weak one is an older test whose only check of "accepts an empty seat" is `expect(parseState(…)).not.toBeNull()`. - **15 of the 20 are on tests written before run 41**, in files run 41 touched, because the lint reads whole files. The 5 on run 41's own tests are all sound. The proposal narrows the lint to the tests a run adds or changes. - **`agent.test.e2e` has the same hole.** It also completes on `paths-exist`, so it is included.

cmoriarty commented

2026-10-03 11:27:12 -04:00

Author

Owner

Correction to my previous comment: 17 of run 41's 20 findings were on tests written before the run, and 3 on run 41's own tests, all three sound. I wrote 15 and 5. The measurement recorded in the change's design confirms 17 and 3.