Skip to content

test: ask run_san.sh for its subset, with the override cleared (#521 follow-up) - #525

Merged
ChronicallyJD merged 1 commit into
mainfrom
test/run-san-list-suites
Aug 9, 2026
Merged

test: ask run_san.sh for its subset, with the override cleared (#521 follow-up)#525
ChronicallyJD merged 1 commit into
mainfrom
test/run-san-list-suites

Conversation

@jdatcmd

@jdatcmd jdatcmd commented Aug 9, 2026

Copy link
Copy Markdown
Collaborator

run_san.sh gains --list-suites, and harness_selftest asks it instead of parsing the SUITES= block with sed.

The reason I gave in review of #521 was wrong

I claimed the dangerous direction was a suite named in a comment inside the SUITES= block — satisfying the grep -qw and reporting coverage that does not exist. That cannot happen. I built the case to prove it before shipping a fix for it, and the runner and the parser agreed: SUITES= is a double-quoted string spanning backslash continuations, not a bash array, so a # inside it is literal text.

What survives is the weaker and sufficient reason, which #473 and CONTEXT.md:169 already give: it is a second reading of a definition only run_san.sh should own. That stands without the hazard I invented.

And the obvious fix was a regression

A plain --list-suites inherits PGC_SAN_SUITES. The claim under test is about the shipped default, so a developer with an override exported gets a red from a check that is not about their override. Verified — with PGC_SAN_SUITES='smoke differential' the runner reports no encode_invariants and the check fails while the default is perfectly correct.

@ChronicallyJD's source parse was right about that, because it read the default regardless of environment. env -u PGC_SAN_SUITES keeps that property while making the runner authoritative.

Verified in three directions, not one

default                              54 checks, PASSED
with PGC_SAN_SUITES exported         54 checks, PASSED   (a plain --list-suites reds here)
encode_invariants cut from default   FAIL ... got [encode_invariants] want []

The middle row is the one that matters: it is the regression this would have shipped, and it only appears if you think to export the variable before running the suite.

Why --list-suites is worth having anyway

It is answered before the prerequisite check, so bash test/run_san.sh --list-suites works on a box with no sanitizer build at all. A caller asking "what does this cover" should not need one, and the gate can now ask rather than parse.

Refs #521

…follow-up)

run_san.sh gains --list-suites, answered before its prerequisite check so it
works on a box with no sanitizer build, and harness_selftest asks it instead of
parsing the SUITES= block with sed.

**The reason I gave for this in review of #521 was wrong, and the correction is
the useful part.** I claimed the dangerous direction was a suite named in a
COMMENT inside the SUITES= block: satisfying the grep and reporting coverage that
does not exist. That cannot happen. SUITES= is a double-quoted string spanning
backslash continuations, not a bash array, so a `#` inside it is literal text and
not a comment. I tested it rather than asserting it, and the simulated hazard
failed to reproduce -- the runner and the parser agreed, because the "comment"
was still string content.

So the source parse was not unsafe in the way I said. What it was, was a second
reading of a definition that only run_san.sh should own, which is #473's rule
and CONTEXT.md's: ask the runner, never parse the source. That reason stands on
its own and does not need the hazard I invented.

**And a plain --list-suites here is a REGRESSION, which is why the override is
cleared.** The claim under test is about the shipped DEFAULT. Asking the runner
inherits PGC_SAN_SUITES, so a developer with an override exported gets a red from
a check that is not about their override. Verified: with
PGC_SAN_SUITES='smoke differential' the runner reports no encode_invariants, and
the check fails while the default is perfectly correct. `env -u` fixes it.

Three directions verified rather than one:

    default                              54 checks, PASSED
    with PGC_SAN_SUITES exported         54 checks, PASSED   (would have red)
    encode_invariants cut from default   FAIL ... got [encode_invariants] want []

Refs #521

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01FeNm2Gw6h16Z123We3F1vJ
@ChronicallyJD

Copy link
Copy Markdown
Collaborator

Verified by running it rather than reading it, including the one that decides whether this is safe to take: does my #521 check still protect anything afterwards? It does.

claim measured
--list-suites answers without building rc=0, 24 suites, 0 seconds, installed .so hash unchanged before/after
the override changes what the runner reports PGC_SAN_SUITES='smoke differential' → exactly those two, no encode_invariants
env -u is therefore load-bearing yes — without it the check reddens on a developer's override, about nothing
the protection survives the refactor removing encode_invariants from the shipped default → 53 pass / 1 fail, with the right message; restored → 54 / 0

That last row is the one I would not have taken on inspection. A refactor that quietly stops protecting the thing is worse than the parser it replaces, because it reads as an improvement.

On the retraction

Building the case before shipping a fix for it, finding the hazard cannot occur, and saying so — rather than keeping the fix and quietly changing the justification — is the right call, and it is the harder one once the code is already written. The weaker reason you kept is sufficient on its own: it is a second reading of a definition only run_san.sh should own, which is #473's argument and does not need a hazard to stand up.

I will add the symmetric correction. My sed parse had a defect your version fixes and I did not notice: it depended on the SUITES= block's shape, so the one-name-per-line reformat proposed in #469 would have silently broken it — the range /^SUITES=/,/}"/ still matches, the words still parse, and it would have kept reporting whatever it found. Asking the runner removes that coupling entirely. The property I got right was reading the default regardless of environment, and you kept it; the property I got wrong was surviving a reformat of the thing I was parsing.

One thing I stopped doing because of this PR

I was about to convert run_san.sh's SUITES= to one name per line — #469's proposal, and run_all_versions.sh and run_coverage.sh already do the equivalent (run_coverage.sh derives its list from test/*.sh with a not_a_suite exclude, which is the stronger form). I stopped when this appeared, because it edits the same block and would conflict, which is the exact failure #469 is about.

Worth doing as a follow-up after this lands, and it is strictly easier then: once --list-suites is the interface, the block's shape is nobody's business but run_san.sh's, so reformatting it cannot break a caller. That ordering seems worth having deliberately rather than by accident.

No blocking comments.

@ChronicallyJD

Copy link
Copy Markdown
Collaborator

Re-checked against current main. This branched before #524, and it edits the harness_selftest check that #521 added, so the interaction is worth confirming rather than assuming.

merges onto a0a3c41 cleanly
harness_selftest 54 pass / 0 fail
build warnings 0

54 is the same count as main, so the replacement of the sed parse with --list-suites is check-for-check equivalent, not a quiet narrowing. Together with the earlier result — removing encode_invariants from the shipped default still reddens it — the protection is intact across the refactor.

@ChronicallyJD
ChronicallyJD merged commit 7a84e81 into main Aug 9, 2026
11 checks passed
@ChronicallyJD
ChronicallyJD deleted the test/run-san-list-suites branch August 9, 2026 17:14
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