test(deploy): pin the two widest citation ranges' interiors, not just their first lines (BLO-32210) - #1686
test(deploy): pin the two widest citation ranges' interiors, not just their first lines (BLO-32210)#1686allyblockcast[bot] wants to merge 1 commit into
Conversation
… their first lines The citation pinning added in #1674 anchors each `:NNN` range to its FIRST LINE, and three of the six entries additionally carry a `contains` guard holding the rest of the range to the content the citing comment claims is there. The three that got one are the three NARROWEST ranges (3, 3 and 2 lines); the two that went without are the two widest (10 and 5) -- the inverse of where interior drift matters most. Measured, not supposed. Against the previous head: MC6b replace the interior of `:751-760` (script lines 752-754, leaving the 751 anchor intact) -> 81/81 GREEN MC7b replace the last line of `:759-763` (script line 763, anchor intact) -> 81/81 GREEN Both now fail the citation test by name. Guards are in the existing idiom and span the range rather than re-checking the anchor: :751-760 only remaining asymmetry -> unfinished parity fix -> no such deadline (script 752 -> 754 -> 757) :759-763 not a parity gap -> `return` vs `exit` is structurally required (script 759 -> 763) Also makes the invariant mechanical instead of adding a fourth unchecked promise to a file whose thesis is that its comments are checkable: the completeness test now asserts every multi-line entry HAS a `contains`, so the next wide citation cannot be added unguarded and reopen this quietly. Two comment corrections from the same review: - the wrap comment claimed a rewrap "stays green". True of that assertion, false of the suite -- a rewrap deletes a line, so every citation below shifts by -1 and the citation test correctly goes red. It now says so. - the header said "all seven citations" above a six-row table. Both numbers are right (seven occurrences, six distinct ranges, one cited twice); it now says which is which. Test-only. `scripts/approve-paperclip-api-digest.sh` is byte-identical. Refs BLO-32210
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
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: ffc4357
Looks good. Test-only, no shipping-script path touched, and every claim in the description reproduced independently rather than taken on trust. I re-ran the suite and the mutation controls against the files fetched at this exact head:
| control | result |
|---|---|
baseline node --test |
81 pass / 0 fail |
MC6b — replace interior lines 752-754 of :751-760, line-count preserving |
fails every line citation … |
MC7b — replace last line 763 of :759-763, line-count preserving |
fails every line citation … |
MC8 — strip contains off :751-760 |
fails no line citation can be added … |
rewrap :346-348 into one line |
fails, naming all four shifted citations and where each moved to |
Both new guards discriminate, and the structural assertion is load-bearing rather than decorative. I also checked the two corrected comments against reality: `:NNN` occurrences are 7 across 6 distinct ranges (matching the six-row table), and the rewrap claim is exactly right — the last control above shows :751-760, :755-757, :759-763, :761-762 all reported at −1 with their new positions.
Critical Issues (0)
Important Issues (0)
Suggestions (2)
-
[native-codex]
scripts/approve-paperclip-api-digest.test.js:2178— the new assertion checks that acontainsis present, not that it reaches.!c.containsis satisfied by any regex, including one that pins nothing. Measured: replacing the:751-760guard withcontains: /#/leaves the suite at 81 pass / 0 fail, and MC6b then passes green again — i.e. the exact gap this PR closes reopens, while the new assertion reports it as guarded. This is the same distinction the comment two lines above it draws ("Pinned AT ALL is not the same as pinned THROUGHOUT"), applied one level up.- Not asking for it here — the two entries this PR adds are genuine guards, and presence-checking is a strict improvement over nothing. But if a cheap reach check is wanted later, asserting that each
containsmatch extends to within N lines of the range end would close it; the harness already computescitedRange(cite), so the match offset is in hand.
- Not asking for it here — the two entries this PR adds are genuine guards, and presence-checking is a strict improvement over nothing. But if a cheap reach check is wanted later, asserting that each
-
[gstack/review]
scripts/approve-paperclip-api-digest.test.js:2063— "containsadditionally holds the rest of the range to the content the citing comment claims is there" slightly overstates what:751-760achieves. Its match ends at script line 757, so lines 758-760 sit inside the cited range with no content pin from that entry. Line 760 (# to close: this loop's caller (cleanup_on_exit) prints the operator) is inside both:751-760and:759-763and is wildcarded by[\s\S]*in both — a line-count-preserving rewrite of it leaves the suite at 81 pass / 0 fail. (Line 758 is a bare#, so its gap is immaterial; 759 and 761-762 are covered by sibling entries.)- Worth a word only because comment-vs-reality accuracy is this PR's own subject: "every multi-line entry carries one" is exactly true and is the durable claim; "holds the rest of the range" is the part that reaches further than the regexes do. Tightening the
:751-760guard to anchor on its final line — as:759-763already does — would make the sentence true as written.
- Worth a word only because comment-vs-reality accuracy is this PR's own subject: "every multi-line entry carries one" is exactly true and is the durable claim; "holds the rest of the range" is the part that reaches further than the regexes do. Tightening the
Strengths
- The structural assertion is the right shape of fix. Patching the two known instances would have left the default for the next wide citation exactly as unguarded as before; asserting the invariant means the gap fails CI instead of shipping quietly, and MC8 confirms it discriminates.
- Before/after mutation controls with line-count-preserving edits. That is the detail that makes MC6b/MC7b meaningful — a line-shifting mutation would have tripped the first-line anchor and proved nothing about
contains. - The rewrap comment correction is the more valuable of the two comment fixes: the original was true of the assertion and false of the suite, which is precisely the kind of locally-true/globally-wrong comment that sends a future reader in the wrong direction. The corrected version now states both halves, and the control above confirms both.
- Correctly scoped:
scripts/approve-paperclip-api-digest.shis untouched, so nothing here can affect the production deploy chain it tests.
Recommended Action
- No Critical or Important issues — nothing blocking from this review.
- Both Suggestions are optional and neither needs to land here; the second is a one-line regex tightening if you want the comment at
:2063to be literally true.
Thinking Path
Linked Issues or Issue Description
policy-suite pinning this follows on from, merged in test(deploy): pin the two retirement loops' messaging asymmetry (BLO-32122) #1674), BLO-32109 (test(deploy): assert the two retirement loops' retry pacing (BLO-32109) #1671)a96e258b("Not asking for it here"), deferred there on that recommendation.What Changed
Test-only.
scripts/approve-paperclip-api-digest.shis not modified (git diff --quiet origin/master -- scripts/approve-paperclip-api-digest.shis clean).LINE_CITATIONSentry:751-760gainscontains: /only remaining asymmetry[\s\S]*unfinished parity fix[\s\S]*no such deadline/, pinning the interior of the ten lines defending the release loop's flat backoff.LINE_CITATIONSentry:759-763gainscontains: /not a parity gap[\s\S]*returnvsexitis structurally required/, pinning through the last line of the messaging-asymmetry defence.no line citation can be added to this file without being pinned: any multi-linecitelackingcontainsfails the suite. This is what stops the gap reopening — patching the two known instances alone would leave the default for the next wide citation exactly as unguarded as before.Verification
Rebased onto current
master(457f7da26) — the branch had been stacked on #1674's pre-merge commits, which never landed;mastercarries their merge-queue-rebased equivalents. Realgit rebase --onto origin/master, not a splice. Single commit, one file, +34/−7.Mutation-verified with a before/after control, because "the mutation fails now" only proves a gap closed if it passed before. Each mutation confirmed a real edit (
git diff --numstatnon-empty) andbash -n/node --checkclean before its result was recorded; tree restored to 0 modified files after each.master(no fix):751-760target+3/−3every line citation …:759-763target+1/−1every line citation …containsoff:751-760(the gap reopening)+0/−1no line citation can be added …Both MC6b and MC7b are deliberately line-count-preserving, so they cannot trip the first-line anchor via line-shift — they isolate the
containsguard itself. MC8 confirms the structural assertion discriminates rather than being decorative.Risks
Low. Test-only; no shipping script or runtime path is touched.
The one real consequence is intentional and worth naming: a multi-line citation is now required to carry a
contains, so adding one without a guard fails CI. That is the point — but it means a future contributor adding a wide citation must also write its guard. The failure message says exactly that.Inherited from #1674 and unchanged here: any script edit that changes line count above
:763shifts the citations below it and turns this suite red. The failure is true rather than noise (it names where each citation moved to), but it is a real maintenance cost on this one file.Model Used
claude-opus-4-5), 1M context, extended thinking, with tool use and code execution (Claude Code CLI / Claude Agent SDK).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template