Skip to content

fix(selftests): extract-advanced-prs negative checks read the result line, not the random fixture sha - #468

Merged
LukasWodka merged 1 commit into
developfrom
fix/3662-extract-advanced-prs-selftest-sha-collision
Sep 11, 2026
Merged

fix(selftests): extract-advanced-prs negative checks read the result line, not the random fixture sha#468
LukasWodka merged 1 commit into
developfrom
fix/3662-extract-advanced-prs-selftest-sha-collision

Conversation

@LukasWodka

@LukasWodka LukasWodka commented Sep 11, 2026

Copy link
Copy Markdown
Contributor

Summary

selftests went red on main at eb89c1e5 (run 34507764093) with the extractor itself correct: extract-advanced-prs-selftest.sh's "promotion: the release-train PR is not attributed" check grepped the whole extractor output for 901, and that output carries ::warning::commit <40-hex sha>: ... attributed from its SUBJECT for the subject-fallback path. The fixture commits are minted at run time ($RANDOM content + the wall clock), so their shas are random hex — that run's was e57d5bafc191901afac8fc19a0c1e89c21749556, which contains 901. Same latent coin toss on the 979 and 3013 negative checks. Roughly 1% of runs per three-digit needle, on every push to every branch.

The file is byte-identical on develop, so develop's green runs were luck, not a fix.

Change

  • result_line() helper: the script's Found PRs: line, i.e. the attribution the checks are about.
  • The three "is NOT attributed / NOT used" negative assertions grep that line instead of the full output. The positive Found PRs: <n> assertion beside each one still carries the "line is present" half, so an absent result line cannot make the negative pass vacuously.

No change to scripts/extract-advanced-prs.sh.

Evidence

Deterministic reproduction (script in the lane scratchpad; loops the committer date until the fixture sha contains 901, then runs the real extractor with the promotion stub):

tries=9 fixture sha=3dff8cf0dd2249010965663da891ea964b3c8303
::warning::commit 3dff8cf0dd2249010965663da891ea964b3c8303: the API names no merged PR for it; attributed from its SUBJECT (900 ). Verify its card.
Found PRs: 900
OLD assertion (whole output): FAIL - '901' found (in the sha)
NEW assertion (result line 'Found PRs: 900'): pass

Mutation proof — drop the promotion exclusion from the extractor (API_JQselect(.merged_at != null)), run the fixed selftest:

  FAIL  promotion: the release-train PR is not attributed
  FAIL  promotion: it falls through to the subject
extract-advanced-prs selftest: 15 passed, 2 failed

Anchor verified applied (git diff --stat = 1 file changed), restored → 17 passed, 0 failed.

Test plan

  • bash scripts/tests/extract-advanced-prs-selftest.sh17 passed, 0 failed
  • make selftests (full suite) → all green except selftest-pipefail-early-close's gawk is not installed, so the CI awk cannot be checked here — a macOS-host drift (BSD awk), unrelated to this change; CI runs gawk.
  • make lint not runnable locally (ruff guard); this PR touches one bash selftest, no Python.

Tracked internally.


Note

Low Risk
Test-only change to bash selftest assertions; no production or extractor behavior changes.

Overview
Fixes flaky extract-advanced-prs-selftest.sh failures where negative checks searched the entire extractor output for ticket/PR numbers (979, 3013, 901). Warnings include the fixture commit’s runtime-random 40-char SHA, so ~1% of runs falsely match those needles even when attribution is correct.

Adds a result_line() helper that isolates the ^Found PRs: line and routes the three “must NOT attribute” assertions through it; paired positive Found PRs: <n> checks are unchanged. scripts/extract-advanced-prs.sh is untouched.

Reviewed by Cursor Bugbot for commit a5bb86c. Bugbot is set up for automated code reviews on this repo. Configure here.

…line, not the random fixture sha

The "is NOT attributed" assertions grepped the whole extractor output for
a three-digit PR number. That output also carries `::warning::commit
<40-hex sha>` for the subject-fallback path, and the fixture commits are
minted at run time, so their shas are random hex: whenever one contained
the needle the assertion failed with the extractor itself correct. That
is what reddened `selftests` on main at eb89c1e (fixture sha
e57d5bafc191901afac..., needle "901"), roughly 1% of runs per needle.

Grep only the `Found PRs:` line -- the attribution the checks are about.
Mutation-proved: dropping the release-train exclusion from the extractor
reddens the narrowed "promotion" check (Found PRs: 900 901).

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@LukasWodka LukasWodka self-assigned this Sep 11, 2026
@LukasWodka

Copy link
Copy Markdown
Contributor Author

bugbot run

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ Bugbot reviewed your changes and found no new issues!

Comment @cursor review or bugbot run to trigger another review on this PR

Reviewed by Cursor Bugbot for commit a5bb86c. Configure here.

@saqlainsyed007 saqlainsyed007 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Approving. A correct fix for a real flake, scoped the right way. CI green, mergeable, no threads.

The three assert_not_out negative checks grepped the whole extractor output for a three-digit needle (979, 3013, 901), but the script also emits ::warning::commit <40-hex sha> lines and the fixture commits are minted at run time, so their shas are random hex — a negative grep for 901 reddens ~1% of runs whenever the sha contains those digits, which is exactly what broke selftests on main at eb89c1e (…901afac) with the extractor itself correct. Narrowing each negative to result_line "$out" (only the ^Found PRs: attribution line) targets the semantically-correct surface — attribution appears only there, and the incidental warning shas are noise for these assertions.

Checked the vacuity angle the comment calls out: an absent result line returns empty, so a negative assertion on it would pass trivially — but each of the three is paired with a positive assert_out "Found PRs: NNN" (985/914/900) that forces the line non-empty and correct, so the pairing carries the check. Reasoning and the failure citation are accurate. Good hygiene fix.

@LukasWodka
LukasWodka merged commit eb9970a into develop Sep 11, 2026
46 of 48 checks passed
@LukasWodka
LukasWodka deleted the fix/3662-extract-advanced-prs-selftest-sha-collision branch September 11, 2026 07:53
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.

2 participants