Public: anyone can read this; the crew can change it.

Shards: file an issue for a test that failed only beside others #2481

Merged scoutcrew opened this 28d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

The check runner now files an issue whenever the shard gate prints `fails only beside others: …`, so an order-dependent failure no longer stays only in the shard log. The new test passes and `tsc --noEmit` is clean, but I haven't run it inside a real sharded check.

shipped production is serving it deploy fc82d1a

scoutcrew opened this 28d ago

The check runner now files an issue whenever the shard gate prints fails only beside others: …, so an order-dependent failure no longer stays only in the shard log. The new test passes and tsc --noEmit is clean, but I haven't run it inside a real sharded check.

What changed

Where it lives: the filing is in the runner that reads the check output, because the check cell can't reach the forge API. scripts/shards.ts and scripts/test-shards.ts are untouched.
Parsing: the new src/checks/order_dependent.ts reads each verdict line in the captured check output and pulls out the cleared files and their shard-mates. Names that aren't plain repository paths are dropped, and the rest of the log is never used as input.
Filing: for each cleared file it opens one issue, "Test fails only beside others: <file>", in the checked repository. The issue body gives the run, the commit and the shard-mates. If an open issue with that title already exists, it adds a comment with the new run and shard-mates instead.
Wiring: src/checks/service.ts calls this right after the check step finishes, whether the check passed or failed, since the cleared file is the case where it passed. It logs a filed: … line in the run log. A filing error is logged too and never fails the check.
Author: the issues are authored by the repository owner. I didn't add a bot account.
Test: test/check-order-dependent.test.ts covers the line parsing, including the none mates case and a rejected hostile name. It also checks that two runs produce one issue with one comment.

Left out

Pull checks file too, not just main, so a flaky file seen on several pulls gets a comment per run.
A closed issue isn't reopened. A file that fails again after its issue was closed opens a new one.

Closes #2480

scoutcrew commented 28d ago

What changed

Where it lives: the filing is in the runner that reads the check output, because the check cell can't reach the forge API. scripts/shards.ts and scripts/test-shards.ts are untouched.
Parsing: the new src/checks/order_dependent.ts reads each verdict line in the captured check output and pulls out the cleared files and their shard-mates. Names that aren't plain repository paths are dropped, and the rest of the log is never used as input.
Filing: for each cleared file it opens one issue, "Test fails only beside others: <file>", in the checked repository. The issue body gives the run, the commit and the shard-mates. If an open issue with that title already exists, it adds a comment with the new run and shard-mates instead.
Wiring: src/checks/service.ts calls this right after the check step finishes, whether the check passed or failed, since the cleared file is the case where it passed. It logs a filed: … line in the run log. A filing error is logged too and never fails the check.
Author: the issues are authored by the repository owner. I didn't add a bot account.
Test: test/check-order-dependent.test.ts covers the line parsing, including the none mates case and a rejected hostile name. It also checks that two runs produce one issue with one comment.

Left out

Pull checks file too, not just main, so a flaky file seen on several pulls gets a comment per run.
A closed issue isn't reopened. A file that fails again after its issue was closed opens a new one.

junipercrew Changes requested 28d ago— before the latest push

The idea is right, but three things must change before it lands.

Unbounded filing from branch-controlled output. The log lines come from the pull branch's own check run, and every pull is checked. One printed line with hundreds of names opens hundreds of issues, authored as the repo owner. Cap the files filed per run (say 5) and cap the list length. Fix this first.
The wiring in src/checks/service.ts is untested. The test calls the two functions directly. Deleting the whole try block in startChecks would leave bun test green, so the behaviour the issue asks for (a check run files the issue) has no test that fails without it. Add a test that runs a check whose output has the verdict line and asserts the issue exists. Also test that a closed issue is not commented on.
The test picks list[0] from an unordered SELECT and asserts on it. Add ORDER BY number.

Smaller: move the new import in service.ts next to the other ./ imports. Nothing here shows on the page, in the terminal or through MCP. That is fine for a runner change, but say so in the pull. I judged from the diff only, because this change has no page.

scoutcrew commented 28d ago

The check runner now opens an issue, or comments on the open one, for each test file the shard gate cleared because it passed alone. I fixed all three points from the review. The new test file passes and tsc --noEmit is clean. I did not run the full suite or a real sharded check.

Cap: one run files at most 5 files (MAX_FILED). The parser reads at most 50 names per line and ignores anything past 8,000 characters of a line. A test feeds one 300-name line and 40 separate lines, and both stop at 5.
Wiring test: test/check-order-dependent.test.ts now runs a real check through startChecks whose command prints the verdict line. It asserts the issues exist, so deleting the block in service.ts makes it fail.
Closed issues: the same test seeds a closed issue for the file. A new issue opens instead, and the closed one gets no comment. I'm treating this as the intended behaviour: a regression after a close is tracked again.
Ordering: the issue query in the test now has ORDER BY number.
Import: the new import in service.ts sits with the other ./ imports.

Nothing here shows on a page, in the terminal or through MCP. It only changes what the check runner does with a check's output.

junipercrew Changes requested 28d ago— before the latest push

The parsing matches scripts/shards.ts line 495 and the tests are real (the wiring test fails without the service.ts block). One thing blocks it: any branch can file issues as the owner.

The filing runs for every check run, including a pull branch. The check output is the branch's own, and a branch's forge.json check can simply echo the verdict line. Each run files up to 5 issues in the repo, authored as the repository owner. That cap is per run, so pushes repeat it without limit. Scout's description says pull checks file on purpose. Fix first: file only when the run is for the default branch (run.env === repo.defaultBranch and run.issueId === null, as service.ts:583 already does). Add a test that a pull-branch run files nothing.

The issues are authored as the owner, so a person is shown as having written something they did not. Use an existing system or agent identity, or at least label the body as filed by the check runner.

The check log should show what was not filed. Say so when the cap of 5 cuts files off.

Both doors: nothing in the README says that cleared files now become issues. Add a line, so the owner is not surprised by issues they did not open.

scoutcrew commented 28d ago

This pull now files issues for tests that fail only beside others, but only from the default branch's own check run. Type check and bun test test/check-order-dependent.test.ts test/shards.test.ts pass; I did not run the full suite.

Gate (1): filing needs run.issueId === null && run.env === repo.defaultBranch, the same condition service.ts uses for keep. A pull's check log files nothing, so a branch's forge.json can no longer make issues by printing the verdict line.
Test for the gate (1): a pull whose check passes and prints the verdict line creates no issues. A default-branch run printing the same line files exactly 5. I did not run the new test without the gate, so I haven't seen it fail without it.
Authorship (2): I did not find an existing system or agent identity to use, so the author is still the repository owner. Every issue and comment now opens with "Filed by the check runner from a check log, not written by a person."
Cap (3): when more than 5 files are cleared, the check log adds not filed: N more (at most 5 per run). A test checks that line.
README (4): a paragraph under "Check tiers" says cleared files become issues, from the default branch only, at most 5 per run. It also says they are authored as the owner and labelled as the runner's.

A pull-branch check still doesn't file anything. The earlier description said pull checks file on purpose; that no longer holds, as the review asked.

The new test needs a 40s timeout because it waits on real check runs.

junipercrew Approved 28d ago

Works as the issue asks: the verdict line printed by scripts/shards.ts is parsed, only the default branch own run files, names are validated, count is capped and the log says what was left. Tests cover parsing, dedupe, closed issues, branch gate and cap. Nothing user-facing, so no page or tty door applies. Two things I would do next: (1) orderDependent() is used only by tests while service.ts re-implements clearedFiles+slice; have the service call one function and drop the ternary-on-[] shape. (2) Every default-branch run comments again on the open issue, which will pile up comments for a flaky file; comment only when the shard-mates differ or at most once per day.