fix(testenv): -update-skips deleted the audit trail it exists to protect - #699
Merged
Merged
Conversation
Closes #697. The ratchet's own failure message tells an author to regenerate with -update-skips. Following that instruction deleted 142 of the census file's 166 lines -- every justification for every previous raise, on the one file whose entire purpose is to make raises reviewable. Hit while raising the budget for #696, worked around by hand there, filed rather than buried. THE CAUSE. writeSkipCensus unmarshalled into skipCensus, built a FRESH one from the two fields that struct declares, and marshalled that. Every other key in the file was dropped by construction. There are seven of them -- raisedBy, loweredBy, raisedBy2, raisedBy3, loweredBy2, raisedBy4, raisedBy5 -- the numbering being what happens when a hand edit has nowhere obvious to append. THE FIX DOES NOT KNOW THE HISTORY SCHEMA, deliberately. "Preserve every key I do not own" stays correct when the eighth history key appears; a fix that carried `raisedBy` by name would have dropped the other six, and would have passed a test written about raisedBy. So nextCensusFile rewrites `total` and `byPackage`, leaves everything else exactly as it found it, and leaves an existing `note` alone too -- that is prose somebody may have edited. ORDER IS PRESERVED, read back off the token stream because encoding/json does not keep it. This file is read in diffs: reordering it on every regeneration buries the two lines that actually changed under a rewrite of the whole file, which is a large part of how the original bug stayed invisible. Verified against the real file: regeneration is now BYTE-IDENTICAL where it previously produced +1/-142. Four mutations, each turning a test red: dropping unknown keys (the old behaviour), setting a new key without ordering it, overwriting an edited note, and failing to record key order. The test asserts the general property rather than raisedBy specifically, for the reason above. Claude-Session: https://claude.ai/code/session_01A8N3W5ct9SZtHK9sCDD9cL
There was a problem hiding this comment.
🟡 Changes recommended
nextCensusFile’s pretty-print path likely introduces extra leading spaces for object/array values, risking non-byte-identical regeneration and noisy diffs in testdata/skips.json.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR fixes the -update-skips regeneration path so it preserves the skip census file’s hand-maintained audit/history keys (and top-level key order) instead of rewriting testdata/skips.json from only the struct-owned fields and dropping everything else.
Changes:
- Introduces
nextCensusFileto rewrite onlytotalandbyPackagewhile carrying through all other keys in their original order. - Updates
writeSkipCensusto usenextCensusFileso regeneration no longer deletes the audit trail. - Adds regression tests covering preservation of unknown keys, preservation of existing
note, and key order, plus the “absent census” bootstrap case.
File summaries
| File | Description |
|---|---|
| internal/testenv/skips_test.go | Adds ordered-preserving census rewrite helper, updates regeneration path to preserve unknown keys, and adds regression tests for key preservation/order. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+293
to
+297
| var pretty bytes.Buffer | ||
| if err := json.Indent(&pretty, fields[k], " ", " "); err != nil { | ||
| return nil, err | ||
| } | ||
| fmt.Fprintf(&buf, " %s: %s", kb, pretty.String()) |
Comment on lines
+209
to
+212
| // censusKeysOwnedByTheWriter are the only two the regenerator computes. Every | ||
| // other key in the file belongs to whoever hand-edited it, and is carried | ||
| // through untouched -- see nextCensusFile. | ||
| var censusKeysOwnedByTheWriter = map[string]bool{"total": true, "byPackage": true} |
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #697.
The skip ratchet's own failure message tells an author what to do:
Following that instruction deleted 142 of the file's 166 lines — every
justification for every previous raise, on the one file whose entire purpose is
to make raises reviewable. I hit this raising the budget for #696, worked around
it by hand there, and filed it rather than leaving the workaround buried.
Cause
writeSkipCensusunmarshalled intoskipCensus, built a fresh one from thetwo fields that struct declares, and marshalled that. Every other key was
dropped by construction. There are seven of them:
The numbering is what happens when a hand edit has nowhere obvious to append.
The fix does not know the history schema
Deliberately. "Preserve every key I do not own" stays correct when the eighth
history key appears — and a fix that carried
raisedByby name would havedropped the other six and passed a test written about
raisedBy. That iswhy the test asserts the general property.
nextCensusFilerewritestotalandbyPackage, leaves everything else as itfound it, and leaves an existing
notealone too — that is prose somebody mayhave edited.
Order is preserved, read back off the token stream because
encoding/jsondoes not keep it. This file is read in diffs; reordering it on every
regeneration buries the two lines that actually changed under a whole-file
rewrite, which is a large part of how the original bug stayed invisible.
Verification
Regeneration against the real file is now byte-identical, where it
previously produced
+1/−142.Four mutations, each turning a test red: dropping unknown keys (the old
behaviour), setting a new key without ordering it, overwriting an edited note,
and failing to record key order.
https://claude.ai/code/session_01A8N3W5ct9SZtHK9sCDD9cL