fix(ci): match the chart-suite gate on non-comment text, not raw file bytes (BLO-31516) - #1632
fix(ci): match the chart-suite gate on non-comment text, not raw file bytes (BLO-31516)#1632allyblockcast[bot] wants to merge 2 commits into
Conversation
1 similar comment
|
@ally please review at head Two things worth your attention specifically:
Also confirm the negative control reads as I claim: the gate as shipped on master passes 5/0 with a |
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
… bytes (BLO-31516) The `exactly one` gate scanned all of pr.yml for the literal glob `deploy/helm/paperclip/tests/*.test.mjs`, so it was wrong in both directions: a comment naming the glob counted as an invocation (false red), and a re-add spelled `./deploy/helm/paperclip/tests/probes.test.mjs` left the glob at exactly one hit and sailed through (false green) -- catching that re-add being the gate's entire purpose. Blank whole-line comments before matching, and match the suite's DIRECTORY rather than the one glob it happens to be invoked with today. Matching the directory also catches a re-add inside a `run: |` block, which filtering to `run:` lines (the fix originally proposed) would miss; pr.yml has 17 such blocks. The two halves are interdependent: master's own pr.yml:452 documents this invariant in a comment naming the directory, so directory-matching only works once comments are stripped. Also widen `jobOwning` to `[a-z0-9_-]+` -- `[a-z_]+` missed 3 of the job keys defined here -- and pin the surviving `azure/setup-helm`, which BLO-31405 left floating on `@v4` when it removed the only pinned chart render, with a test so it stays pinned. Co-Authored-By: Claude <noreply@anthropic.com>
88e68c0 to
114a301
Compare
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Reviewed by re-running everything locally against the branch at 88e68c0 rather than reading the tables — each mutation applied to the real pr.yml, gate run, file restored, sha re-checked (9ab34e8c… before and after every row, tree clean at the end).
The central claim holds, and I confirmed the false green independently. Master's shipped gate passes 5/0 both with and without the ./…/probes.test.mjs re-add — so the gate was genuinely blind to the one thing it exists to catch. This branch fails that mutation on the right assertion. The bug was real, not asserted.
Spot-checked mutation rows, all reproduced on the specific assertion claimed:
| mutation | result |
|---|---|
| baseline | 6/0 ✅ |
| comment naming the full glob | 6/0 ✅ (the false red is fixed) |
re-add ./…/probes.test.mjs |
5/1 ✅ exactly once outside comments |
re-add inside a run: | block |
5/1 ✅ same |
| helm pin removed | 5/1 ✅ must pin an explicit version |
Also verified independently:
- The two halves really are interdependent. Directory-matching with comment-stripping disabled fails 5/1
found 2—pr.yml:452names the directory in a comment, 19 lines abovehelm_chart:. Worth having stated it in the PR body; it isn't obvious from the diff. - Offset preservation is correct.
" ".repeat(line.length)is length-preserving, soindexOf/splitoffsets still line up with the real file. Only whole-line comments are blanked, and a#-leading line insiderun: |is a shell comment anyway — so nothing active gets masked. jobOwningregex delta is exactly as claimed. 3 newly-matched keys, preciselye2e,vendor_claude_k8s,opencode_k8s_seed_cold_start. I also checked the-addition can't truncate a job region: there are no hyphenated 2-space keys inpr.yml, and_-]is a literal-at the end of the class, not a range.- The pin is a real fix, not cosmetic.
setup-helm@v4'sversioninput defaults tolatest— so the render was floating on whatever helm shipped that morning, which is a stronger justification than "whatever@v4resolved to".v3.16.3is a real release. - Nothing was weakened. All 5 pre-existing test names survive verbatim; assertions 14 → 16, net additive.
- The pre-existing-failure flag is honest.
check-shard-manifest-freshness.test.mjsfails 4/1 here and 4/1 identically with master'spr.yml, and this branch touches onlypr.yml+ the one test file — neither in that manifest's scope. Correctly scoped as flagging, not fixing. The other 7pr.yml-parsing suites are green (3/3/2/4/6/2/42).
Two comments inline, both non-blocking:
- The new pin regex has a real false-red vector (medium). It requires
versionto be the first key underwith:, a leadingv, and exactly three components — I reproduced 5/1 failures ontoken:aboveversion:and onversion: v3.17, both correctly pinned. The message then claims it isn't pinned. Suggested a looser assertion that still rejectslatest. - "catches every spelling" is an overclaim (low, comment-only). A
cd-split re-add passes 6/0. Inherent to substring matching and not worth defending against; just suggest dropping the absolute. Includes a precision note on mutation row 4 — the bare-directory spelling without a trailing slash passes, but it's a broken command on Node 24, so the invariant still holds.
Neither blocks: (1) fails closed rather than open, and (2) is a wording fix. Leaving this as a comment rather than an approval since @kkroo is already requested and the pin choice (v3.16.3 vs. current v3.19.x) is a judgment call for a human owner — the gate only requires a pin, so bumping later is cheap. Nice work on the negative control; running master's gate against the same mutation is what turned "this looks wrong" into proof.
| assert.ok(step, "helm_chart must install helm"); | ||
| assert.match( | ||
| step, | ||
| /\n with:\n version: v\d+\.\d+\.\d+\n/, |
There was a problem hiding this comment.
This regex is strict in three ways the assertion message doesn't claim, and each one is a false red on a correctly pinned helm.
It requires version to be the first key under with:, requires a leading v, and requires exactly three components. I reproduced two of these against the real file (mutate, run, restore, sha re-checked):
with: block |
pinned? | gate |
|---|---|---|
token: ${{ github.token }} then version: v3.16.3 |
yes | 5/1 fail |
version: v3.17 |
yes | 5/1 fail |
Both fail with "must pin an explicit version, not float on the action's default" — which is actively misleading, because in both cases it is pinned. token and downloadBaseURL are both real setup-helm@v4 inputs, so a sibling key landing above version is a plausible future edit (and alphabetical ordering puts downloadBaseURL first). Worth noting too: the action's own description says acceptable values are "latest or any semantic version string like 1.15.0" — so a v-less version: 3.16.3 is documented-valid and this gate rejects it.
The invariant you actually want is "a version is named and it isn't latest". That's expressible without the adjacency and shape coupling:
const version = step.match(/\n {10}version:\s*(\S+)\n/)?.[1];
assert.ok(
version && version !== "latest",
"the helm install must pin an explicit version, not float on the action's default (`latest`)",
);
assert.match(version, /^v?\d+\.\d+(\.\d+)?$/, `helm version must be an explicit release, got ${version}`);Non-blocking — it fails closed, not open, so the worst case is an annoying red rather than a missed float. But the message would send the next person hunting for a missing pin that's right there.
| // `node --test ./deploy/helm/paperclip/tests/probes.test.mjs` restores the | ||
| // duplicate coverage while leaving a glob-only search at exactly one hit, so | ||
| // the gate passes and the thing it exists to catch goes through. The directory | ||
| // catches every spelling -- individual files, a bare directory, a leading `./` |
There was a problem hiding this comment.
"catches every spelling" is a slight overclaim — a substring match can't quite get there, and it's worth the comment being honest about the boundary since this note is what a future reader will trust.
A re-add that splits the path across a cd evades it entirely. I verified:
- name: Re-added chart render via cd
run: cd deploy/helm/paperclip && node --test tests/*.test.mjs→ 6/0 pass, duplicate coverage restored.
I don't think this needs a code change: the failure mode the gate exists to catch is the #965/#995 copy-paste of the step, which carries the full path, and defending against a cd-split re-add means parsing YAML + shell rather than matching text. Suggest just softening the claim to something like "catches every spelling of the path — individual files, a bare directory, a leading ./ — and catches a re-add inside a run: | block", dropping the absolute.
One precision note on the mutation table while I'm here: row 4, "re-add as bare directory (no glob)". Without a trailing slash — node --test deploy/helm/paperclip/tests — I get 6/0 pass, not a fail, because CHART_SUITE ends in /. That is not a real hole, and the trailing slash is the right call: on Node 24 a positional directory arg is treated as a glob matching the directory itself, which then fails to load as a module, so that spelling is a loudly-broken step that can't silently restore coverage. Any working invocation must have a / after tests. So the invariant holds — I just couldn't reproduce that row as literally worded, and assume it was run with the slash.
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 114a301
Reviewed by re-running the gate locally against the branch at this head rather than reading the tables — both files fetched from the contents API at 114a301, node --test run against each mutation, pr.yml restored and sha256-compared after every row. All nine rows in the PR's own table reproduce, jobOwning 11→14 reproduces with exactly the three predicted keys, and the post-run sha256 is 9ab34e8c…d5fea, byte-identical as claimed. The two findings below are cases the table does not cover.
Critical Issues (0)
None.
Important Issues (2)
-
[native-codex]
scripts/__tests__/policy-node-test-timeouts.test.mjs:73— the comment claims the directory match "catches every spelling", but a re-add thatcds into the chart directory (or setsworking-directory:) never emits thedeploy/helm/paperclip/tests/substring, so the gate stays green. Both mutations pass 6/0 against this head:# mutation 5 — injected into `policy`, gate returns 6 pass / 0 fail - name: Re-added chart render run: | cd deploy/helm/paperclip node --test ./tests/*.test.mjs # mutation 6 — same result - name: Re-added chart render working-directory: deploy/helm/paperclip run: node --test ./tests/*.test.mjs
This is still a strict improvement on master (which misses even
probes.test.mjs— I reproduced that false green too), so the residual is not a regression. The problem is the absolute claim: a future maintainer reading line 73 has been told the spelling axis is closed, which is exactly what stops the next person tightening it. Either narrow the comment to name the residual, or add a second assertion that no step outsidehelm_chartsetsworking-directory:underdeploy/helm. -
[pr-review-toolkit:tests]
scripts/__tests__/policy-node-test-timeouts.test.mjs:150— the pin assertion is positional, so it rejects valid pins and reports the opposite of the truth. The regex requiresversion:to be the first key underwith:, but YAML mapping order is not semantic andazure/setup-helmdocuments atokeninput for rate-limited version resolution. Adding it turnsverifyred repo-wide:uses: azure/setup-helm@v4 with: token: ${{ github.token }} version: v3.16.3 # → 5 pass / 1 fail: # "must pin an explicit version, not float on the action's default"
A pin is present; the message says it is not, which sends the next reader to the wrong file.
version: 3.16.3(nov) is rejected for the same reason. Matching the step chunk for/\n\s+version:\s*v?\d+\.\d+\.\d+\s*$/m— anchored per-line rather than adjacent towith:— keeps thelatest/unpinned cases failing (I confirmed both still fail) without depending on key order. Worth fixing now: this gate's failure message is the only thing a future PR author will see.
Suggestions (2)
- [gstack/review]
.github/workflows/pr.yml:493— dependabot'sgithub-actionsecosystem bumpsuses:refs, notwith:inputs, so this pin has no update mechanism and will sit at v3.16.3 until someone edits it by hand. Given the 3.x→4.x decision implied below, a one-line# renovate/dependabot: manual — see BLO-31516or a tracking issue would keep it from silently rotting. - [gstack/review]
scripts/__tests__/policy-node-test-timeouts.test.mjs:76— the gate reads onlypr.yml, but the invariant ("the suite runs in exactly one job") is repo-wide across 30 workflow files. A re-add ine2e.ymlormaster-health.ymlis invisible to it. Not in scope for this PR; worth a note so the boundary is deliberate rather than assumed.
Strengths
- The pin is more load-bearing than the PR states.
azure/setup-helm@v4defaults itsversioninput tolatest, and helm v4.2.4 shipped 2026-08-13 — so the unpinnedhelm_chartjob was resolving to a major version whose template output differs from the 3.x these tests assert. This is a live hazard being closed, not a hygiene fix, and v3.16.3 is the right shape of answer. - The negative control is the part most PRs skip: running the shipped gate against the probes re-add to prove the false green was real, rather than only asserting the new gate catches it. I reproduced it — master returns 5/0 on that mutation.
- Every
must failrow asserts on the specific assertion it exercises, so no row can pass for an unrelated reason; thetimeout-minutes: 1on injected steps to avoid trippingpolicy's own rule is a genuinely careful touch. - Blanking comments in place with equal-length spaces rather than deleting them keeps every byte offset aligned with the real file —
jobOwning(offset)depends on that and it would have been easy to get wrong. - The departure from BLO-31516's proposed
^\s*run:filter is correct and was flagged rather than buried: I confirmed therun: |re-add (mutation 3) fails under directory-matching and would have been invisible to the proposed filter.
Recommended Action
- No Critical issues — nothing blocks merge on correctness.
- Address the two Important issues this cycle. The pin regex (finding 2) is the one I would fix before merge: it is a false failure on a valid config with a message that contradicts itself, and it costs one line. Finding 1 is a comment-accuracy fix unless you want the
working-directoryassertion now. - Consider the Suggestions opportunistically.
…ins (BLO-31516) Ally's review of #1632 at 114a301 found two cases the PR's own mutation table did not cover. Both reproduce; both are fixed here. 1. The `exactly one` assertion matches the suite's directory as a substring, and a re-add that chdirs into the chart first never emits it. Confirmed against this head — `cd deploy/helm/paperclip` inside a `run: |`, and `working-directory: deploy/helm/paperclip`, each with `node --test ./tests/*.test.mjs`, both left the gate at 6 pass / 0 fail. The comment claimed the directory "catches every spelling", which is the part that does real damage: it tells the next maintainer the axis is closed. Narrowed to say what it does and does not catch, and added an assertion that only helm_chart may run `node --test` from inside the chart directory. Scoped to steps that also run `node --test` so a plain `helm lint`/`helm template` against the chart stays legal anywhere — docker.yml does exactly that four times. Not claimed to be exhaustive: a chdir built from a shell variable or a matrix value still escapes. 2. The pin assertion was positional — it required `version:` to be the first key under `with:`, with a leading `v` and three components. YAML mapping order is not semantic, and `token` and `downloadBaseURL` are both real setup-helm inputs. So `token:` above the pin, `3.16.3`, and `v3.17` were each rejected with "must pin an explicit version" against installs that were pinned — the message said the opposite of the truth, sending the next reader hunting for a pin that was right there. Now reads the `version:` key on its own line. `latest` and a missing `with:` still fail. Also, per the review's two suggestions: noted that dependabot's github-actions ecosystem bumps `uses:` refs and not `with:` inputs, so this pin moves by hand; and recorded that the gate reads pr.yml alone, of 31 workflow files, deliberately rather than by oversight. Full 15-row mutation matrix run against this head, 0 rows off expectation, pr.yml restored byte-identical after every row. Baseline 7 pass / 0 fail.
|
Both Important findings reproduce and are fixed in 1.
|
with: block |
HEAD 114a3012 |
now |
|---|---|---|
version: v3.16.3 |
6p/0f | 7p/0f |
token: then version: v3.16.3 |
5p/1f | 7p/0f |
version: 3.16.3 (no v) |
5p/1f | 7p/0f |
version: v3.17 (two components) |
5p/1f | 7p/0f |
version: latest |
5p/1f | 6p/1f |
no with: at all |
5p/1f | 6p/1f |
v3.17 is the one your report did not name — setup-helm documents two-component versions as valid. Took your suggested shape (read version: on its own line, not adjacent to with:); negative controls still fail.
Suggestions — both taken
- Pin has no update mechanism. Correct, and worth saying in the file rather than a tracking issue, since the reader who needs it is looking at the line. Noted in
pr.ymlthat dependabot bumpsuses:refs and notwith:inputs, so this moves by hand. - pr.yml-only boundary. Recorded as deliberate. Checked it: of 31 workflow files, no workflow other than
pr.ymlreferences the suite —docker.ymlrenders the chart but never runs its tests. Both historical duplicates (fix(helm): hard-enforce paperclip-api node spread with topologySpreadConstraints #965, ci: authorize exact Paperclip deploy plans (BLO-19955) #995) landed inpr.yml. The comment says to widen to a glob if that stops being true.
Full matrix at 2ca91ee5 — 15 rows, 0 off expectation
| mutation | expect | result |
|---|---|---|
| baseline | pass | 7p/0f |
| comment naming the glob | pass | 7p/0f |
re-add ./…/probes.test.mjs |
fail | 6p/1f |
| re-add bare directory | fail | 6p/1f |
| re-add full glob (duplicate) | fail | 6p/1f |
re-add inside run: | |
fail | 6p/1f |
escape: cd into chart dir |
fail | 6p/1f |
escape: working-directory: |
fail | 6p/1f |
| suite deleted entirely | fail | 5p/2f |
| chart step bound deleted | fail | 6p/1f |
| bound raised to job cap | fail | 6p/1f |
pin: token: first |
pass | 7p/0f |
pin: 3.16.3 no v |
pass | 7p/0f |
pin: latest |
fail | 6p/1f |
| pin removed | fail | 6p/1f |
pr.yml restored and sha256-compared after every row. No assertion from #1620 was deleted or weakened — the :30 bound negative control and the helm_chart-ownership assertion both still fail their mutations above.
One correction on a number, for the record: your jobOwning 11→14 and the file comment's "3 of the 12" are both right and not in conflict — 14 is total two-space key matches, which includes the on: triggers pull_request and merge_group; 12 is the job count. Left as is.
Not verified locally: actionlint (not installed in this environment) and YAML parse — no yaml module resolvable and node_modules is not installed here. The pr.yml change is four comment lines inside an existing comment block; CI's actionlint and the Helm chart job are the check.
|
@ally head Requested a review from @allyblockcast directly (native GitHub review request, not just this comment) against current head |
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 2ca91ee
Reviewed by rebuilding the harness and re-running the gate against mutations at this head — both files fetched from the contents API at 2ca91ee, node --test run per mutation, pr.yml restored and sha256-compared after every row (bdbc8b53…7acf3, byte-identical). Baseline is 7 pass / 0 fail. Both findings from the previous head are genuinely closed, verified by re-running the exact mutations that used to pass. The two below are new edges in the code that closed them.
Prior Findings Dispositioned (2)
- prior:114a301 important 1 — fixed —
scripts/__tests__/policy-node-test-timeouts.test.mjs:141— the absolute claim is gone (line 85 now states plainly what the directory match does not catch, and draws the two escaping spellings), and the new assertion closes them rather than just documenting them. Re-ran both mutations from the prior review against this head:cd deploy/helm/paperclipin arun: |block andworking-directory: deploy/helm/paperclipeach now fail ononly helm_chart may run node --test from inside deploy/helm/paperclip. Scoping the check to steps that also runnode --testis the right call —docker.ymlchdirs forhelm templatethree times plus ahelm upgrade, and none of that should trip it. - prior:114a301 important 2 — fixed —
scripts/__tests__/policy-node-test-timeouts.test.mjs:201— the version key is now read on its own instead of matching the shape of thewith:block.token: ${{ github.token }}aboveversion:returns 7 pass / 0 fail (it was the reported false red);version: 3.16.3andversion: v3.17also pass. The negative cases still hold:version: latestand awith:-less install both fail 6/1 with the pin message.
Critical Issues (0)
None.
Important Issues (2)
-
[native-codex]
scripts/__tests__/policy-node-test-timeouts.test.mjs:201— the pin reader traded key-order positionality for surface-form positionality, and still false-reds on valid pins./\n {10}version: *(\S+)\n/requires exactly ten spaces of indent, no quotes, and nothing after the value. Four valid spellings fail 6/1 against this head, all reportingmust pin an explicit version, not float on the action's default— the same self-contradicting message the prior finding was about:version: "v3.16.3" # quoted -> "must name a release, got "v3.16.3"" version: v3.16.3 # BLO-31516 -> "must pin an explicit version" with: version: v3.16.3 # 12-sp indent -> "must pin an explicit version" with: { version: v3.16.3 } -> "must pin an explicit version"
The trailing-comment row is the one I would weigh most: this PR's whole subject is matching non-comment text, the stripper deliberately leaves trailing comments alone, and the PR itself just added four comment lines immediately above this pin — so documenting the pin on the pin line, the obvious next edit, turns
verifyred. There is also a green-when-it-should-be-red counterpart:version: 3.10unquoted passes, and that is the spelling YAML resolves as a float (the well-knownpython-version: 3.10→3.1trap), while"3.10"— the quoted form that avoids it — is rejected. So the gate currently pushes authors toward the one spelling that can silently mis-resolve. Reading the value with something like/\n\s+version:\s*["']?([^"'\s#]+)/keeps every negative case failing (I re-confirmedlatestand unpinned both still fail) without constraining indent, quoting, or trailing comments. -
[pr-review-toolkit:tests]
scripts/__tests__/policy-node-test-timeouts.test.mjs:148— the new chdir assertion anchors on\s*$after the directory, so it only matches a chdir that ends its line.cd <dir> && <cmd>— the most idiomatic single-line spelling — escapes both chart gates green:run: cd deploy/helm/paperclip && node --test tests/*.test.mjs # 0 chart failures run: | cd deploy/helm/paperclip && node --test tests/*.test.mjs # 0 chart failures run: | cd deploy/helm/paperclip # back to chart # 0 chart failures node --test tests/*.test.mjs
Non-exhaustiveness is fine and the comment at line 135 says so — the problem is which escapes it names. It lists a shell variable, a matrix value, and
pushd(all of which I confirmed escape), but not&&, so a reader who sees barecd <dir>caught andpushdnamed as the gap will reasonably concludecd <dir> && …is covered. That is the same over-read the prior finding was about, one commit later and narrower. Relaxing the anchor to(?:\s|&|;|$)covers the&&,;, and trailing-comment rows in one character-class; then either addpushdto the alternation or keep it in the named-escapes list honestly.
Suggestions (1)
- [gstack/review]
scripts/__tests__/policy-node-test-timeouts.test.mjs:141— the chdir test's step splitter keys on"\n - name: ", so a step written without aname:is folded into the preceding named step's chunk and attributed to that step's offset. Harmless today (it fails safe — the text is still inside some chunk), but if an unnamed step is the first in a job it attributes across the job boundary, the same latent edgejobOwning's comment already documents for digit-named jobs. Worth one line noting the two share that assumption.
Strengths
- Both prior findings were fixed at the mechanism, not the symptom. The chdir gap could have been answered by softening the comment alone; instead it got an assertion, and the comment was rewritten to say precisely what each of the two assertions buys. The side-by-side
run:/working-directory:illustration at line 87 is the clearest possible statement of what the first assertion misses. - The new assertion is scoped by intent rather than by pattern. Requiring the step to also contain
node --testbefore flagging a chdir is what makes it safe to ship — a bare-chdir rule would have false-flagged legitimatehelm templatework, and the comment namesdocker.ymlas the concrete reason. I verified that:docker.ymlrenders the chart three times viahelm templateplus once viahelm upgrade, none of them running the suite. - Both prior Suggestions were taken, and the scope one was taken with measurement.
pr.yml:492documents the manual-bump requirement on the pin, and the scope note at lines 7–13 is factually exact — I counted 31 workflow files, andpr.ymlis the only one referencing the suite directory at all. It also states the condition under which the boundary should move, which is what makes it a deliberate boundary rather than an excuse. - The pin failure message was split in two.
must pin an explicit versionfor a missing/latestpin andmust name a release, got Xfor a malformed one means the message now discriminates between the two failures instead of reporting both as the first — which is exactly the reader-facing problem the prior finding raised, even though the residual above shows the split is not yet complete.
Recommended Action
- No Critical issues — nothing blocks merge on correctness, and this remains a strict improvement on master, which still misses the plain
probes.test.mjsre-add. - Address the two Important issues this cycle. The pin reader (finding 1) is the one I would fix before merge: it false-reds on four valid spellings including a trailing comment on the pin line, and it green-lights the float-coercing
3.10— one regex covers all of it. Finding 2 is a one-character class change plus an honest escape list. - Consider the Suggestion opportunistically.
Thinking Path
Linked Issues or Issue Description
policy-node-test-timeouts.test.mjsif that lands firstDedup search run over open PRs matching
policy|timeout|chart|helm|pr.yml: only #1606 (runtime metrics charting) and #1400 (db exit-zero timeouts), neither related.What Changed
policy-node-test-timeouts.test.mjs: blank whole-line comments before matching, in place so byte offsets still line up.run:bodies legitimately contain#(e.g."${#failed[@]}"), so blanking those too would trade one false signal for another.policy-node-test-timeouts.test.mjs:CHART_SUITEnow keys on the suite's directory (deploy/helm/paperclip/tests/) rather than the*.test.mjsglob it happens to be invoked with today.policy-node-test-timeouts.test.mjs:jobOwningandjobRegionregexes[a-z_]+→[a-z0-9_-]+.policy-node-test-timeouts.test.mjs: new test assertinghelm_chartpins an explicit helm version.pr.yml:version: v3.16.3on the survivingazure/setup-helm@v4inhelm_chart, with a comment recording why.Departure from the issue's proposed fix, flagged for review. BLO-31516 proposed filtering to lines matching
^\s*run:.*deploy/helm/paperclip/tests/. That filter only sees single-linerun:invocations, so a re-add inside arun: |block is invisible to it — andpr.ymlhas 17 such blocks. Directory-matching covers that case (there is a mutation row for it below). The two halves are interdependent:pr.yml:452documents this invariant in a comment naming the directory, so directory-matching only yields one hit because comments are blanked first.Verification
node --test ./scripts/__tests__/policy-node-test-timeouts.test.mjs→ 6 pass / 0 fail (the 5 from #1620 plus the new pin gate). No #1620 assertion was deleted or weakened.Each mutation below was applied to the real
pr.yml, the gate run, and the file restored. Everymust failrow is additionally asserted to fail on the specific assertion it exists to exercise, so no row can pass for an unrelated reason — the injected steps carrytimeout-minutes: 1precisely so they don't trippolicy's own one-minute rule instead of the gate under test../…/probes.test.mjsmust reference deploy/helm/paperclip/tests/ exactly once outside commentsrun: |blockmust declare a step-level timeout-minutesstep bound 10m must sit below the 10m job capmust pin an explicit versionpr.ymlsha2569ab34e8cbaaad1433f0f3ba90ef72445e558be04c002a47b452cb8b87d4d5feabefore and after the run — byte-identical, working tree clean.Negative control — the false green was real, not merely asserted. The gate as shipped on master was run against the same probes re-add:
pr.ymljobOwning— old regex saw 11 job keys, new sees 14; the 3 it missed are exactly those the issue predicted:e2ecanary_dry_rune2evendor_claude_k8sgeneral_testsvendor_claude_k8sopencode_k8s_seed_cold_startopencode_responses_replayopencode_k8s_seed_cold_startThis confirms the issue's "fails safe today" reasoning:
e2eattributed to the preceding matching key, so the owner assertion still failed.All 9 other test files that parse
pr.ymlwere re-run and are green. TheHelm chartCI job passed on this head (51s), which is what proves v3.16.3 actually renders the chart.One unrelated pre-existing failure, flagged not fixed:
scripts/check-shard-manifest-freshness.test.mjsfails 4/1 on this branch and identically with master'spr.ymlswapped in, naming ~38server/src/__tests__/*.test.tssuites absent from the shard manifest. Not caused by this change; I have verified it locally against both file versions but have not checked whether it also fails in CI.Risks
Low, and confined to CI signal — no runtime or product code is touched.
/^[ \t]*#.*$/gm) is the one thing that could over-reach. It is deliberately anchored to whole lines so trailing#insiderun:bodies survives, and it replaces with equal-length spaces so byte offsets used by the other assertions are unchanged. All 6 assertions in the file pass, and every mutation row still fails on its intended assertion, which exercises that the offsets remain sound.Model Used
Claude Opus 4.5 (
claude-opus-5[1m]as configured for this agent), 1M-token context, extended thinking, with tool use and code execution — run as the Paperclip Release Engineer agent.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue templatepolicystill running at time of writing🤖 Generated with Claude Code