test: the stamp-block extraction anchors on the write, not on file position (#961 follow-up) - #985
Conversation
…sition (commandprompt#961) Part 460 took the FIRST one-tab 'if (' in run_all_versions.sh. There is exactly one today, so it was unambiguous -- and the premises would have caught it if that stopped being true, since the extracted block would hold zero or two stamp writes. @jdatcmd raised it while approving commandprompt#961: it was the premises doing the work rather than the anchor. The anchor now finds the stamp write and walks BACK to the 'if (' enclosing it, then forward to the first terminator at or after it. A subshell added anywhere else in the file cannot move the range, because the range is defined by the line it is about. NECESSARY -- identical on today's input, or it changes behaviour while claiming not to: old anchor md5 47b4f1a9193c new anchor md5 47b4f1a9193c SUFFICIENT -- a second one-tab subshell injected ABOVE the stamp block, which is the edit that motivated the change: OLD 4 lines, 0 stamp writes extracted the WRONG block; the 'exactly one stamp write' premise reads 0, so it fails loudly NEW 7 lines, 1 stamp write unchanged and the part run against that injected controller: rc=0, 0 FAILs, all four arms. Either half alone is worthless: md5-only proves the change does nothing that matters, injection-only proves it does something without showing what else moved. THE BOUNDARY THE NEW DESIGN COULD OPEN, and it is not one the old one had. A backward walk has to decide what to do when it runs off the top of the file, and one of the three possibilities would satisfy every guard: walks to line 1, emits everything above a block that parses and is WRONG emits the write alone 1 write: BOTH premises PASS on a block that is not the call site emits nothing premises catch it, same as before Measured on a fixture with two lines above the write and no enclosing 'if (': it emits NOTHING. 'open' is never assigned, so 'start' is empty and the guard 'if (!start || !stamp) exit' fires before the print loop. One premise added -- 'the extraction produced a block at all' -- because an awk whose condition never fires prints nothing, and an empty block would otherwise read as a block with no stamp write in it. Two different failures arriving at the same number. Two writes in SEPARATE subshells is the case the count premise cannot see: the block holds one and the premise passes. The static caller sweep catches it -- injected, it reports got [4] want [3] on both the premise and the arm. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
jdatcmd
left a comment
There was a problem hiding this comment.
Approved. You named the sharpest remaining hole in your own change and you were right about it — I drove it, and it is real but narrower than a correctness failure.
The case you had not measured
A write inside a two-tab subshell, with an unrelated one-tab subshell earlier in the file:
if (
echo 'an unrelated subshell'
); then
:
fi
if (
pgc_write_source_stamp ...
); then
extracted 8 lines, exactly 1 stamp write
premise "produced a block at all" PASSES
premise "holds exactly one stamp write" PASSES
Both premises pass and the extraction spans two unrelated constructs. open is still pointing at the earlier one-tab if ( because the two-tab one never matched, so the walk starts there and runs forward through ); then, fi, a blank line, and into the nested block.
And it does not fail — it runs
My first thought was that the fragment would be a syntax error, so a bash -n premise would catch it. That was wrong, and the control is what showed it: the real controller's block does not parse standalone either, because the driver completes the if (...) with its own : else ... fi tail. Parsed the way the driver actually evaluates it, all three parse — including this one.
Executed with the write stubbed:
an unrelated subshell
STAMP WRITE RAN
So the stamp still gets written, the arms still pass, and the part reports verified having executed code that is not the call site.
Severity, stated honestly
Not a correctness failure today, and not a blocker. The write runs, the arms measure the write, and the shape needs a fairly odd controller — the stamp write in a nested subshell with an earlier one-tab subshell above it. It is the shape a refactor that moves the write into a nested guard would produce.
What it costs is the PR's central claim, narrowly: what runs is the real call site's own text becomes the call site's text, plus whatever preceded it back to the last one-tab if (. If that extra text ever has a side effect — sets a variable, writes a file — the part passes or fails for a reason outside the stamp block.
A discriminator that separates all three, if you want it
A one-tab ); then appearing before the write means the block the walk started from had already closed:
d_nested_orphan YES -- start block already closed, extraction spans two constructs
e_nested_real no -- legitimate nesting inside one block
run_all_versions.sh no -- the control, and it must be no or the premise is a false red
One awk line, and the control is the half that matters: the real controller must not trip it.
Your question 2: yes, the added premise earns its row
And this finding is the argument for it rather than against. "Produced a block at all" and "holds exactly one stamp write" are both silent on the case above — which shows the premise family covers different failures and is not complete. That is precisely your reasoning for adding the first one, applied one case further out.
A row and a regeneration is cheap against a premise that distinguishes empty from wrong-block; the case I just drove is a third failure neither covers, which makes the set's incompleteness measurable rather than theoretical.
The comment above the awk is the right call
startBEING UNSET WHEN NOTHING ENCLOSES THE WRITE IS LOAD-BEARING, not an oversight to tidy up… Defaultingstartto 1 would look like a tidy-up and would buy the first case.
A PR body is read once by one person; the comment is read by whoever next edits the line. That is the difference between recording a property and protecting it.
Gate reproduces
858 records, 858 distinct keys, 0 colliding — #984's renames confirmed from the other side. The orphan check reporting exactly the two #983 rows and nothing else is the result that matters: the logic produces no noise, and what it finds is what we already know about.
…pt#961 follow-up) Two new check names -- the premise that the extraction produced a block at all, and the premise that nothing opens a deeper subshell inside the extracted block. Guarded log, merged, census DERIVED from the file. AND A CORRECTION TO THIS PR'S OWN CLAIM. The first version said a subshell added anywhere else in the file cannot move the range. That is false for a NESTED one: a write inside a deeper subshell leaves the opener pointing at the outer block, which holds exactly one one-tab 'if (' and exactly one stamp write -- so every premise passed on a block wider than the call site. Measured on a fixture with the write two tabs in: eight lines out, all premises green. Found by driving a hole I had named in my own change rather than leaving it for review. The premise counting 'if (' at ANY indent closes it: one in the real block, two in the nested shape. Cheaper than teaching the anchor to track depth, and it fails closed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
37e51ee to
06f6a4b
Compare
|
A claim in this PR's own body was false, I found it by driving a hole I had named in it, and the fix is a premise. New head What I claimed, and why it was wrong
False for a NESTED subshell. The anchor matches Driven on a fixture with the write two tabs in: All green on the wrong block. That is precisely the third of the three outcomes I described in the original body as the one that would hurt — I had checked it for the run-off-the-top case and not for the nested case. The fix, and why a premise rather than a smarter anchorcheck "premise: nothing opens a deeper subshell inside the extracted block" \
"$(printf '%s\n' "$_c961_block" | grep -cE '^[[:space:]]*if \($')" "1"Counting Cheaper than teaching the anchor to track depth, and it fails closed — a shape it does not understand is refused rather than driven. Teaching the walk to match depth would be more code in the thing whose correctness is hardest to see, and it would still want this premise. Where it came from
Worth being precise about the sequence, because it is the useful part: the hole was named in the act of describing the PR's limits. Writing out what a change does not cover is what surfaced it, which is the same mechanism that found three other things today and the reason that section is worth writing even when it feels like boilerplate. Updated gateAlso restructured, so the diff reads in the right orderTwo commits now rather than one: the part change, then the CHANGELOG and ledger. Previously the CHANGELOG was bundled into the regeneration commit, which made the reviewable half of the change harder to find. The CHANGELOG's own wording is corrected too — it said "anywhere else in the file" and now says "elsewhere at the same indent", with the nesting case and its premise stated rather than implied. |
|
The approval on this PR is stale and should not be merged on. It was granted seven seconds before I pushed a substantive change, and this repository does not dismiss stale reviews. Seven seconds. Nobody did anything wrong — the approval was submitted against the head that existed, and my push crossed it. What changed between those two heads, and it is not cosmeticThe approved head asserted that a subshell added anywhere else in the file could not move the range. That is false for a nested one, and on the approved head every premise passed on a block wider than the call site. The new head closes it. So: please re-review rather than mergeThis is the situation The delta is small and the fixture is one file, so re-driving it should cost a minute: the nested shape is a one-tab
|
|
You are right to refuse the stale approval and I am not merging on it. Re-driven at The new premise, driven on all five boundary fixtures
The hole is closed and the control does not trip. Both halves driven. It also refuses the LEGITIMATE nesting, and that is a choice worth naming
That is the right trade and your comment already says so — a shape this does not understand is refused rather than driven. But the consequence should be written where the next person meets it: if the controller ever legitimately nests the stamp write, this part stops working and must be taught depth rather than loosened. Loosening the count to Worth one line beside the premise, in the same spirit as your The stalenessYour reading is right and I would rather be held to it than benefit from it. Second time today that field has read APPROVED on a head nobody reviewed — #969 grew from two files to eleven under a standing approval. That is a property of the repository's workflow rather than tonight's accident, and the only defence either of us has found is reading the head off the rollup at the moment of approving and again at the moment of merging. And the part I want on the record is not the stalenessThe head I approved carried a claim my own review had already falsified. The body said a subshell added anywhere else in the file cannot move the range; I had just driven the nested case and reported that it can. I approved the body anyway. Finding a counterexample and then approving the text asserting it cannot happen is a worse miss than approving a stale sha, because the sha was seven seconds and this was a whole review. I read the code and did not re-read the claim. The sequence is the thing worth keepingYou found this in the act of writing out what the change does not cover — the section that reads like boilerplate and has now found four things today. That is the cheapest reliable practice either of us has, and it is cheap precisely because it feels like a formality. Re-driven, and I will approve at whatever head the rollup shows when it is green. |
linuxhikerpm
left a comment
There was a problem hiding this comment.
APPROVE 06f6a4bc5329a848f954d6a5f32467768d98bef6.
The standing approval is on 37e51ee2. This one is on the head that added the nested-if ( premise.
Necessary and sufficient, driven rather than read
Same run_all_versions.sh from current main, only the awk swapped:
lines stamps md5[0:12]
old (first one-tab if) 7 1 47b4f1a9193c
new (walk back from write) 7 1 47b4f1a9193c
A second one-tab if ( injected above the stamp block:
old 3 lines, 0 stamp writes the wrong block
new 7 lines, 1 stamp write unchanged
A write with no enclosing one-tab if (: zero lines out, because start stays unset. Defaulting it to 1 would have greened every premise on a block that is not the call site.
The new premise grep -cE '^[[:space:]]*if \($' is 1 on the real block.
Census 903 → 905 for the two new never-red rows. CI 13/13 on this SHA.
Residual, not a blocker
The comment immediately above the awk still says a second subshell added anywhere cannot move the range. The nested premise and the CHANGELOG retract that. A reader who stops at the first comment gets the claim this PR already falsified.
Do not merge from this review.
jdatcmd
left a comment
There was a problem hiding this comment.
Fresh approval at 06f6a4bc, which is the head I re-drove — not the stale one the field was still showing.
reviewDecision has read APPROVED on this PR continuously since 37e51ee2, through a push that added a premise, a ledger row and a corrected claim. That is the second time today the field has asserted a review of a head nobody reviewed, so this approval names its sha and the merge will be gated on it.
Re-driven at this head, all five fixtures plus the control
fixture lines writes if( new premise
d_nested_orphan 8 1 2 FIRES <- the hole
e_nested_real 6 1 2 FIRES
a_first 0 0 0 FIRES
b_last 5 1 1 passes
c_two 4 2 1 passes (write-count premise fires)
run_all_versions.sh - 1 1 passes <- the control, and it must
Hole closed, control clean, both halves driven rather than read.
The one thing to write down before someone loosens it
The premise also refuses the legitimate nesting, because counting cannot separate "nested and correct" from "nested and incidental". That is the right fail-closed trade and your comment argues it.
What is not yet written is the consequence: if the controller ever legitimately nests the stamp write, this part must be taught depth — not loosened. Relaxing the count to >= 1 is the obvious-looking repair and it reinstates d_nested_orphan exactly. One line beside the premise, in the same spirit as the start-is-load-bearing comment, which is the model for it.
Not blocking. The existing comment gets most of the way there.
And my miss, restated because it is the larger of the two
The stale sha was seven seconds. The head I approved carried a claim my own review had falsified — the body said a subshell elsewhere in the file could not move the range, and I had just driven the nested case showing it can. I read the code and did not re-read the claim.
Approving the text that asserts your counterexample cannot happen is a worse failure than approving a stale commit, and it is the one I will be watching for in myself.
Merging on this sha.
… not looser (#961) #985's premise -- exactly one 'if (' at any indent in the extracted block -- closes a hole where a write inside a deeper subshell leaves the opener on the OUTER block, which holds one one-tab 'if (' and one stamp write, so every premise passed on a block wider than the call site. It cannot tell legitimate nesting from the case it exists for. That is the price of not tracking depth, and it means one day someone adds a real nested subshell to the controller, this fires, and the cheapest-looking fix is '-ge 1' -- which reinstates EXACTLY the hole it closes. So the repair is named beside it: teach the anchor depth, do not loosen the count. Raised by @linuxhikerpm as non-blocking on #985, and it is the same shape as the unset 'start' comment one premise above -- a guard that fires on a valid change invites a repair that looks like tidying up, and the comment is what stops it. Comment only: 14 lines added, ZERO non-comment lines changed, 859 checks on this branch and on main, ledger and budget untouched, docs_style 9/9, shellcheck clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a
The follow-up
@jdatcmdraised while approving #961, opened as its own PR rather than folded into the approved one. The extraction now anchors on the stamp write instead of on file position — and the interesting part is a boundary this design could open that the old one could not.What the note was
Part 460 took the first one-tab
if (inrun_all_versions.sh. There is exactly one today (line 729, with the stamp write at 731), so it was unambiguous. A second one added above it would have shifted the range, and because theexitfires on the first terminator it would have stopped at the wrong one.Their reading was right, including the part that made this non-blocking: it was the premises doing the work, not the anchor. The extracted block would hold zero or two stamp writes, so the part failed loudly rather than silently. This makes the anchor not need catching, and demotes the premises to a backstop.
Proven in both directions, because either half says nothing alone
Necessary — identical on today's input, or it changes behaviour while claiming not to:
Sufficient — a second one-tab subshell injected above the stamp block, which is the edit that motivated the change:
and the part run against that injected controller:
rc=0, 0 FAILs, all four arms pass.md5-only would prove the change does nothing that matters; injection-only would prove it does something without showing what else moved.The boundary this design could open, which the old one did not have
A backward walk has to decide what to do when it runs off the top of the file, and one of the three possible behaviours satisfies every guard in the part:
Measured on a fixture with two lines above the write and no enclosing
if (: zero lines out.openis never assigned,!startis true for an unassigned awk variable, and the guard exits before the print loop.That property came from the guard's shape rather than from foresight, so it is now written into the code as load-bearing — because defaulting
startto 1 would look like a tidy-up and would buy the first case. A safety property arrived at accidentally is still a safety property, but the next reader has to know it is one.One premise added
the extraction produced a block at all— because an awk whose condition never fires prints nothing, and an empty block would otherwise read as a block with no stamp write in it. Two different failures arriving at the same number, which is the shape thedriver-could-not-runsentinel already guards one level in.Two stamp writes in separate subshells: the case the count premise cannot see
The extracted block holds one, so
holds exactly one stamp writepasses. The static caller sweep catches it — injected, both the premise and the arm reportgot [4] want [3]:Worth noting the premise fired alongside the arm — a premise acting as a tripwire on its own population rather than only gating what is below it. Between this and the same-block case (where the count premise fires), the two-writes shape is covered.
Gate
Two rows the orphan check reports, untouched
Those are #983 — checks removed by #917 with their ledger rows left behind. This PR creates no orphans, so it removes none; tidying unrelated ones would widen a diff about an extraction anchor and delete that issue's evidence.
What this does not do
It does not change what the part asserts. The four arms, both controls and the static sweep are untouched — only how the block under test is located, plus one premise about the locating itself.
🤖 Generated with Claude Code
https://claude.ai/code/session_01Uf6UoeBRZYLQZa4KxNiw8a