Repository navigation
#242: the no-copy guard must not publish what it forbids — and then, delete most of the guard - #252
Merged
Conversation
…ecisions it forced Registered before any code moves, so the reasoning survives the fixes. FIVE NEW FROM /code-review max ON THE MERGED #239 (all reproduced here, none taken on trust): C-89 the no-copy guard prints the value it forbids, in one branch of three and proven on one side of two; C-90 the replacement drift checks assert less than their docstrings say, and one mutation proof is a tautology; C-91 the git plumbing turns ordinary developer states into hard errors with the wrong diagnosis; C-92 this repository lost its only check that the FAO consumer still SELECTS by the delivery label; C-93 a mutation proof written by whoever wrote the guard tests that author's imagination — 13 forms proposed by the author, 13 caught; 29 proposed independently, 15 missed. THREE MORE FROM THE EXPERT REVIEW OF THE STANDING DECISIONS: C-94 nothing anywhere detects an invisible delivery at the time it happens — every mechanism aimed at it is a CI-time proxy — deferred with a trigger and an owner rather than omitted; C-95 the integration-test prohibition is cited to þing-02 D2 in three places when it is þing-01 D2, and the real ruling is CONDITIONAL and GRANTS read-only preflight; C-96 the registry table carrying that permission is classified "not about this package". C-95 and C-96 compound into the finding that matters most for planning: a whole class of mechanism was believed closed for weeks because a citation pointed at the wrong verdict and the table stating the permission was declared none of our business. AMENDED: C-57 (my own amendment overclaimed — it cited a backticked example as proof the mid-sentence class was covered; it passes only because of the backticks, and a fourth failure direction was found: removing the length floor admitted this repo's own package names into a security guard's ban-set); C-86 (its partial-mitigation paragraph overstates what shipped — one of the three replacement checks does not match on the facts this repository declares); C-87 (one partner's residual got worse, not better). Header 93/20/73 -> 96/23/73. Epic #241, tracking #251. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
C-89's first two thirds. The guard that exists to stop a coordinate value being published was publishing one, into a world-readable CI log, on precisely the event it exists to catch. THE VALUE IS NO LONGER A PARAMETER. Both branches build their finding through `_report_a_copy(where, coordinates)`, which is never handed the value — so a future call site cannot print one however it is written. That is the difference between a rule and a design: the markdown branch was taught not to print on 2026-08-11, the AST branch was not, and the two drifted for a day because each read fine on its own. It names ALL coordinates sharing the value, not one. Measured against the live registry: two pairs share a value (the prod-forecasts bucket and collection share both their id and their name), so a `value -> name` dict would keep the last and name the wrong coordinate half the time — in the one message a maintainer uses to find the copy. THE ROTATION PROOF CHECKED ONE SIDE OF TWO. It asserted the pinned value was absent and said nothing about the freshly rotated one, which is the more damaging half. Mutation- proven: leaking the post-rotation side while keeping the pinned side digested fails now and passed before. AND THE RULE NOW HAS A CHECK, which is the part that matters. `test_every_finding_goes_ through_the_one_reporter` asserts by AST that every `copied.append` in the scan calls the one reporter. Nothing compared the two branches, which is why prose saying "never print a value" coexisted with a branch printing one. Read as source rather than run, because the leaking branch only executes when a coordinate has actually been copied — a state this repository must never be in, so a behavioural test of it would have nothing to observe. Three mutations, each reverted: (1) restore the original AST f-string -> the new guard fails, naming the line; (2) leak the post-rotation side -> the rotation proof fails; (3) inject a real value declared by TWO coordinates into a package module -> caught, both coordinates named, no value in the message. Deliberately NOT in this change: the `secret` exemption and the ban-set's package-name collision. Those are the scan's SCOPE, not its reporting, and belong with the matcher rewrite in #243. C-89 stays open rather than being closed on two thirds of its content. Suite 411 passed / 1 skipped / 40 xfailed, ruff clean. The one pre-existing failure (`test_the_consumer_still_filters_on_the_name_until_it_checks_itself[crafd]`) is #248's, red since views-crafdapi refactored a literal into a named constant on 2026-08-12. Closes #242. Epic #241. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…sage instead Five independent reviewers were run against the first attempt. They agreed, and they were right: the guard I wrote was worse than nothing. IT MISSED SIX OF EIGHT MUTATIONS AND FIRED ON A LEGITIMATE REFACTOR. It asserted that every finding was ROUTED through `_report_a_copy` — an AST walk over the scan's own source. But routing is not safety. The value could be smuggled through either of that helper's two parameters, appended with `extend` or `+=`, or reported from a renamed accumulator, and in every case the guard stayed green while a coordinate reached the log. Meanwhile `finding = _report_a_copy(...); copied.append(finding)` — an ordinary refactor that leaks nothing — failed it. A guard that misses the thing and cries wolf on the innocent is C-82's shape and ADR-014 §3's deletion criterion at once, and my docstring's claim that a caller "cannot print one however it is written" was false on the day I wrote it. SO READ THE FINISHED MESSAGE, NOT THE SYNTAX THAT PRODUCED IT. `test_the_scan_reports_ a_copy_without_reprinting_it` plants a value in a fixture tree, runs the real scan through both halves, and asserts the message names the coordinate and never the value. It does not matter how a finding is built or what the final assertion interpolates. All five surviving mutations are now caught — including one that leaked the entire ban-set through the assertion message rather than through any finding. That is also the answer to C-93: not a longer list of mutants I thought of, but a test whose shape makes the class unreachable. AND THE SCAN'S OWN DOCSTRING CARRIED THREE REGISTRY VALUES. pytest prints the failing function's source, so the guard would have published them on precisely the event it exists to catch — message clean, traceback not. Now it names coordinates. The wider measurement is registered as C-97: 33 standalone values in docstrings and comments across nine files, production modules included, excluded from the scan by a deliberate C-57 decision taken before anyone counted them. Three of them cannot be told from ordinary code at all — two are this repo's package names, one is a function name. Also fixed, both found by review and both real: - the markdown half named the coordinate the ASSIGNMENT names, not the one that DECLARES the value. Those differ exactly when a document pastes one coordinate's value beside another's name — the ordinary copy-paste slip — so it asserted a false fact and hid the coordinate actually leaked. - the rotation loop hardcoded the fixture's two literals a dozen lines from where the fixture builds them. Renaming the fixture strings turned the assertion into "two dead strings are absent". Now taken from the fixture's own variables. Register: C-89 amended with what was tried and what failed; C-97 opened; header 96/23 -> 97/24. The earlier "Mitigated" heading corrected to "Partial mitigation", which is one of the two phrasings this register declares — inventing a third passed the integrity guard by evading its string rather than complying, the identical escape ADR-014 §5 records C-15 making. C-89's Location line numbers repointed; the fix had moved all three and the amendment cited the old ones. The file is +112 lines against `development`, which is the wrong direction for this epic. This story is inherently additive — it adds a guarantee's check where there was none. #245 and #248 are deletions and are where the budget comes back. Suite 411 (410 passed / 1 skipped / 40 xfailed), ruff clean. The one failure is #248's, red since views-crafdapi refactored on 2026-08-12. Closes #242. Epic #241. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
An adversarial mutation review ran 14 mutations against iteration 2. Twelve survived. Seven were leak channels; all seven are now closed. Five are refused in writing. A MESSAGE TEST ONLY SEES THE MESSAGE. `print(node.value)` beside the append, or a `logging.warning`, left the assertion text clean while pytest published the value under Captured stdout and Captured log call. The behavioural test now reads capsys and caplog as well. A stray debug print is an ordinary accident, not an adversarial one. SUBSTRING ABSENCE IS NOT ABSENCE. A finding carrying `value[:12]` or `value[::-1]` walked straight past `planted not in message`, and a reader recovers both instantly. Replaced by `_carries`: does any eight-character window, forwards or backwards, appear. AND THE DRIFT REPORTER HAD NO TEST AT ALL. `_describe_changes` is exercised only by the rotation proof, through one shape, with fixture values SHORTER THAN EVERY REAL COORDINATE — so a digest leaking only above a length threshold passed, as did a reversing digest, as did the appeared/removed branches a both-sides fixture never reaches. Three leaks, all green. It now has its own test, driven through all three shapes with an over-long opaque value. A HELPER'S PARAMETER IS A PUBLICATION CHANNEL. `_dotenv_use` took file text; pytest renders a frame's arguments and `ast.parse` raises on any malformed module, so a package file that both failed to parse and carried a value would have printed itself in full. It takes a Path now and reads inside. And the fixture values are opaque now because a readable one bit me: the first planted string contained the word "coordinate", which also appears in the check's own failure prose, so the window test fired on the test's own English. My cry-wolf, caught by my own guard, fixed in the fixture rather than by weakening the check. WHAT I AM NOT CHASING, and why it is written down rather than left to be rediscovered. Five mutations survive. Four narrow the scan's SCOPE — a length floor, a dropped section, a swallowed SyntaxError, a `break` after the first finding — and none is visible to a test that plants its own fixture, because the fixture satisfies whatever narrowing is applied. They are #243's subject and are routed there with reproductions; fixing them here would be doing that story badly and twice. The fifth deletes the no-print assertion itself, which is infinite regress: a guard on a guard on a guard has no fixed point, and the register carries that risk instead. The eight-character window is a judgement, not a law — a six-character leak would pass, and chasing it is the whack-a-mole this epic exists to refuse. Register: C-89 amended with all three passes, including what was tried and failed. Suite 412 (411 passed / 1 skipped / 40 xfailed), ruff clean. The one failure is #248's. Closes #242. Epic #241. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The maintainer's verdict on iterations 1-3: "so complicated and heavy with technical
debt that you are not even able to maintain it. Then no one can." He is right, and the
measurement is worse than the impression. Four commits had grown one file by +195 lines
(1145 -> 1340, +17%). +95 of that was PROSE restating the risk register. All 149 lines
of new function bodies were tests of another test in the same file. Meta tests went from
37% to 45% of the file.
This commit deletes 140 of those lines. Every one of the four outstanding review
findings is resolved by subtraction.
TWO OF MY OWN ADDITIONS WERE DEFECTIVE, AND A DESIGN REVIEW CAUGHT BOTH.
`test_the_drift_report_never_carries_a_value_in_any_recoverable_form` claimed to drive
`_describe_changes`'s appeared/removed branches. It did not. Those branch on
`was is None`; my fixtures passed `_ABSENT`, a sentinel OBJECT. The branch is never
entered — it ran one path three times while asserting three, and the register repeated
the claim. That is an ADR-014 §1 defect inside the fix for an ADR-014 §1 defect. The
branches it named return constant strings and cannot leak at all. DELETED.
`_carries`, my eight-character-window "recoverability" predicate, is STRICTLY WORSE THAN
PLAIN `in`. The window degrades to exact match below eight characters, and this package's
two directory names are five-character declared values. Measured:
`_carries("views_postprocessing/unfao/leaky.py …", "unfao")` is True — it condemns a
bare, safe file path while screaming that the scan published a value. Simultaneously
stricter than `in` on opaque fixtures and identical to it on the values that matter.
DELETED; both sites use plain `not in`, which is the vocabulary this file already uses in
`test_the_environment_refusal_logs_names_and_never_values`.
ALSO DELETED: `_report_a_copy` — 2 lines of code under 14 lines of docstring, 11 of them
explaining what it does NOT guarantee. Its safety claim was falsified in iteration 2 and
it survived anyway. Two inline f-strings now, which is what WET-before-DRY prescribes:
the second incident did not have the same shape as the first, and the helper's signature
forced the markdown branch into a detour it had to justify with a six-line comment. The
behavioural guard is what holds the rule, not the formatter.
The output guard SURVIVES, at 19 lines instead of 78. ADR-014 §1 does not permit a
guarantee with no check, and this regression already happened once, silently. But it no
longer monkeypatches four globals, shells out to `git init`, or guards a `print` nobody
has written.
AND THE ONE REAL OPEN LEAK IS NOW CLOSED, AT BOTH SITES. Iteration 3 claimed a malformed
package module could no longer publish itself because `_dotenv_use` takes a Path.
Measured: that changes nothing — the leak is `ast.parse`'s own frame, which pytest
renders with the source as its argument, and `path.read_text()` still hands it there. The
fix is `raise ... from None`, where `from None` is load-bearing because the chained
SyntaxError carries the same text. Applied to BOTH parse sites; the scan itself parses
every package .py, and fixing one and not the other is C-89's shape verbatim.
Proven: a malformed module carrying a real value now fails with "does not parse (line
92)" and the value appears ZERO times, where it appeared before.
Five mutations re-proven: the original C-89 defect fails; a widened assertion fails; a
raw digest fails; a malformed module fails without publishing itself; a legitimate
refactor passes.
Register: C-89's Location is now FUNCTION NAMES — it has cited stale line numbers twice,
because each fix moved them. The false "it takes a Path now" sentence is corrected. The
narration of both deleted additions is removed, including one claim that was never true.
Two deferrals get triggers: consolidating into tests/test_redaction_guard.py when #243
finishes with this file, and a pre-existing proof that re-types its subject inline.
tests/test_env_declaration.py: 1340 -> 1200 (+55 over the pre-story 1145, not +195).
Suite 410 passed / 1 skipped / 40 xfailed, ruff clean. The one failure is #248's.
Closes #242. Epic #241.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This was referenced Aug 12, 2026
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
1 participant
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 #242. First story of epic #241.
The defect (C-89)
The no-copy scan had two reporting branches. One was taught not to print the coordinate value; the other was not. So on the single event the guard exists to catch, it published a coordinate into a world-readable CI log. Public repo, unredactable logs.
Four iterations, and the fourth deletes most of the first three
tests/test_env_declaration.pyOf the +195, +95 was prose restating the register, and all 149 lines of new function bodies were tests of another test in the same file. Meta tests went 37% → 45%.
Two of my own additions were defective, both caught by design review:
_describe_changes's appeared/removed branches. Those branch onwas is None; the fixtures passed_ABSENT, a sentinel object. Never entered — one path run three times while asserting three. And those branches return constant strings and cannot leak. Deleted._carrieswas strictly worse than plainin. Its 8-char window degrades to exact match below 8 characters, and this package's two directory names are 5-character declared values. Measured:_carries("views_postprocessing/unfao/leaky.py …", "unfao")→True. It condemns a bare, safe file path. Deleted._report_a_copyalso went — 2 lines of code under 14 of docstring, 11 explaining what it does not guarantee. Two inline f-strings now; WET-before-DRY, since the second incident did not have the first's shape.The output guard survives at 19 lines instead of 78. ADR-014 §1 doesn't permit a guarantee with no check, and this regression already happened once silently.
The one real open leak, closed at both sites
Iteration 3 claimed a malformed package module could no longer publish itself because
_dotenv_usetakes aPath. Measured: that changes nothing — the leak isast.parse's own frame, which pytest renders with the source as its argument. The fix israise ... from None;from Noneis load-bearing because the chainedSyntaxErrorcarries the same text. Applied to both parse sites — the scan itself parses every package.py, and fixing one and not the other is C-89's shape verbatim.Proven: a malformed module carrying a real value fails with
does not parse (line 92)and the value appears zero times. It appeared before.Mutations
digest()returns the raw valueDeliberately not chased
Four surviving mutations narrow the scan's scope — routed to #243, which owns scope. A fifth deletes the no-print assertion: infinite regress, carried in the register. Two residuals recorded: the markdown branch isn't behaviourally proven (it holds no value by construction), and a leak shorter than the fixture would pass.
The one failure is #248's — red since views-crafdapi refactored on 2026-08-12.
🤖 Generated with Claude Code