Skip to content

fix(scrub): point the drift guard at the REPO source so it can never be tier-invisible - #291

Merged
ZacxDev merged 1 commit into
mainfrom
fix/scrub-drift-guard-repo-source
Aug 2, 2026
Merged

fix(scrub): point the drift guard at the REPO source so it can never be tier-invisible#291
ZacxDev merged 1 commit into
mainfrom
fix/scrub-drift-guard-repo-source

Conversation

@ZacxDev

@ZacxDev ZacxDev commented Aug 2, 2026

Copy link
Copy Markdown
Contributor

Second of the three regressions #276 shipped. #289 fixed the first, #290 the third.

The bug

test_patterns_cover_bash_guard asserted that scrub.py's SECRET_PATTERNS is a superset of the Bash hook's, by ast.parseing the deployed hook:

_BASH_GUARD = Path.home() / ".claude" / "hooks" / "bash-guard.py"
if not _BASH_GUARD.exists():
    pytest.skip(...)

#276 moved SECRET_PATTERNS out of bash-guard.py into guard_core.py. The parser returned [] and the test began failing:

AssertionError: could not parse SECRET_PATTERNS from bash-guard.py

Why it stayed broken — the part that matters

Keyed on $HOME, the test skipped in the nix sandbox (synthetic HOME, no hook) and ran only on a switched dev host. So it failed on every dev host — breaking the pre-push tier — while the hermetic gate, the tier that actually gates merges, stayed green and silent.

That is the property you asked me to make sure I addressed: the sandbox tier could not fail on it. It is the exact complement of #290's defect, which failed only in the sandbox. Both hid behind #289's red.

I also caught it changing under me mid-session: the same tree passed at 16:45 and failed at 17:03, because a concurrent home-manager switch swapped the deployed hook.

The fix, and the options I considered

I did not pick silently. Three ways to fix it:

A — repoint at the deployed ~/.claude/hooks/guard_core.py. Smallest diff. Rejected: keeps the tier-invisibility. It would still skip in the sandbox, so the next time this breaks, the gate again cannot see it.

B — compare repo ↔ repo (chosen). Both session_insight/scrub.py and claude-hooks/guard_core.py are tracked in this repo, so the test runs in every tier and never skips. The deployed copy is generated from the repo file by home-manager, so the repo is the correct referent anyway: pointing at $HOME answered a question about the machine, not about the commit. It also removes a pinned skip rather than adding one.

C — B plus an extra deployed-copy check that skips when absent. Strictly more coverage on dev hosts. Rejected for now: it re-introduces an environment-dependent skip and a pin to maintain, to catch a case (deployed copy diverging from its own generator) that home-manager makes structurally impossible. Happy to add it if you disagree — it is additive.

The empty-parse case is now a loud, named assertion instead of the silence that hid #276.

🔴 A required companion change, not a cosmetic one

The test no longer skips, so the conditional EXPECTED_SKIPS entry pinning that skip must go — otherwise #284's accounting fails the gate with "FEWER than pinned: a pinned skip now RUNS". Removed, with a comment explaining why it must not come back:

If you find yourself re-adding a conditional skip for this suite, the drift guard has been re-pointed at something environment-dependent again. Fix that instead.

Reachability — both assertions broken on purpose

Each fails for its own reason, not a neighbour's:

mutation result
inject a pattern into guard_core.py that scrub.py lacks scrub.py drifted from guard_core.py — these SECRET_PATTERNS are no longer covered: ['\bmutant-XYZ-[0-9]{9}\b']
rename the literal, reproducing #276's move could not parse SECRET_PATTERNS from guard_core.py …

guard_core.py restored byte-identical after each (verified against HEAD, not assumed).

Counts

Merge order

Independent of #290 textually, but #290 should land first — on current main this branch alone leaves the sandbox red for #290's reason. Neither is stacked on the other.

🤖 Generated with Claude Code

…be tier-invisible

test_patterns_cover_bash_guard compared scrub.py's SECRET_PATTERNS against
the DEPLOYED hook (Path.home()/'.claude'/'hooks'/'bash-guard.py') and
skipped when that file was absent. Two problems, and the second is why it
sat broken:

  1. WRONG REFERENT. The deployed copy is generated from the repo file by
     home-manager, so the test answered a question about the machine, not
     about the commit.
  2. IT COULD NOT FAIL IN CI. Keyed on $HOME it SKIPPED in the nix sandbox
     and RAN only on a switched dev host. When #276 moved SECRET_PATTERNS
     out of bash-guard.py into guard_core.py, the ast parser returned []
     and the test failed on every dev host while the hermetic gate — the
     tier that gates merges — stayed green and silent.

Now compares two files both TRACKED IN THIS REPO
(session_insight/scrub.py vs claude-hooks/guard_core.py), so it runs in
every tier and NEVER skips. Renamed to test_patterns_cover_guard_core.

The empty-parse case is now an explicit, loud assertion naming the cause
rather than the silence that hid #276.

🔴 run-tests.sh: the conditional EXPECTED_SKIPS entry that pinned the old
environment-dependent skip is REMOVED — mandatory, not cosmetic. The test
no longer skips, so leaving the pin would fail the gate with 'FEWER than
pinned'. Replaced with a comment saying why it must not come back.

REACHABILITY — both assertions broken on purpose, each failing for its OWN
reason (not a neighbour's):
  * inject a pattern into guard_core.py that scrub.py lacks
    -> 'scrub.py drifted ... no longer covered:
        ['\\bmutant-XYZ-[0-9]{9}\\b']'
  * rename the literal, reproducing #276's move
    -> 'could not parse SECRET_PATTERNS from guard_core.py ...'
guard_core.py restored byte-identical after each.

session_insight suite: 57 passed, 0 skipped (was 56 passed + 1 skipped in
the sandbox / 56 passed + 1 FAILED on a dev host).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@ZacxDev

ZacxDev commented Aug 2, 2026

Copy link
Copy Markdown
Contributor Author

nix flake check green on the MERGED tree

Verified on an integration branch off main with #290 + this PR merged — not on this branch alone, which would have been red for #290's reason and told me nothing.

ort auto-merged scripts/run-tests.sh (both PRs edit it, different regions). Per "a clean git merge is not a clean merge", I checked the merged result was coherent rather than trusting the clean merge: nix-instantiate present in REQUIRED_TOOLS, the conditional skip gone, EXPECTED_SKIPS down to one entry.

PASS  scripts/tests                                  (collected=959 passed=959 skipped=0)
PASS  scripts/session-analysis/session_insight/tests (collected=57  passed=57  skipped=0)
TOTAL collected=4353  passed=4352  skipped=1  failed=0   (floor: 2850)
---- skips (pinned: 1) ----   -> observed 1, reconciled
RESULT: PASS

nix flake check -> all checks passed   (pytests + nodetests)

The load-bearing line for this PR is the skip accounting. Against #290 alone the same tree reported skipped=2 / pinned: 2; with this PR it is skipped=1 / pinned: 1, and session_insight goes 57 passed, 0 skipped. The skip became a pass, and removing the conditional EXPECTED_SKIPS entry is what keeps #284's accounting balanced — leaving it would have failed the gate with "FEWER than pinned".

Counted from the build log, not read off an exit status: the wrapper reported exit 0 for the pipeline because $? saw a trailing echo.

@ZacxDev
ZacxDev merged commit 6859606 into main Aug 2, 2026
@ZacxDev
ZacxDev deleted the fix/scrub-drift-guard-repo-source branch August 2, 2026 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant