Skip to content

fix(commands): scope microbit-enforcer SessionStart marker-clear to startup|clear (dogfood F1) - #88

Merged
tigers1997 merged 3 commits into
mainfrom
dogfood/f1-sessionstart-matcher
Jun 26, 2026
Merged

fix(commands): scope microbit-enforcer SessionStart marker-clear to startup|clear (dogfood F1)#88
tigers1997 merged 3 commits into
mainfrom
dogfood/f1-sessionstart-matcher

Conversation

@tigers1997

Copy link
Copy Markdown
Owner

What

F1 from the container dogfood. The microbit-enforcer SessionStart marker-clear shipped with no matcher, so it fired on every SessionStart source (startup, resume, clear, compact). A /freeze//guard//careful marker was therefore silently wiped on --resume and after compaction — mid-session, exactly when a long session most needs the freeze to hold.

Fix

templates/commands/microbit-enforcer/settings-patch.json SessionStart entry now carries "matcher": "startup|clear" (source set confirmed against code.claude.com/docs/en/hooks-guide). Markers clear only on a brand-new session or /clear; they persist across --resume/--continue and compaction.

Lifecycle docs corrected to match — across both the templates and the python-uv-fastapi example mirror: freeze/guard/careful SKILL.md, the microbit-enforcer.sh header, and the templates/core/CLAUDE.md gitignore note.

Tests

New test/microbit-enforcer/test-sessionstart-matcher.sh (asserts the merged settings carry the matcher; red against the unmatched form). configure.py --check clean; full microbit-enforcer suite green.

Scope

Split out of #87 per CONTRIBUTING.md ("one logical change per PR") — this is the fix/patch half. The feat half (F2, the check_stack_reality [ STACK WARNINGS ] preflight) ships as a separate PR. No tested_up_to / CC_VERSION bump.

🤖 Generated with Claude Code

…tartup|clear (dogfood F1)

The SessionStart hook that clears the freeze/guard/careful markers shipped
with no matcher, firing on every source — including resume and compact — so
a /freeze was silently lost mid-session after compaction or --resume. Now
"matcher": "startup|clear": markers clear only on a fresh session or /clear
and survive --resume and compaction.

Sources verified against code.claude.com/docs/en/hooks-guide (startup,
resume, clear, compact; matcher filters on source). The freeze/guard/careful
SKILL.md notes, the microbit-enforcer.sh header, and the
templates/core/CLAUDE.md gitignore note are corrected to match, across both
the templates and the python-uv-fastapi example mirror. New
test/microbit-enforcer/test-sessionstart-matcher.sh.

Split from the originally-bundled PR #87 per CONTRIBUTING.md (one logical
change per PR); F2 (stack-reality preflight) ships as a separate PR.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

VERDICT: BLOCK

Summary

PR #88 correctly scopes the microbit-enforcer SessionStart marker-clear hook to startup|clear sources by adding "matcher": "startup|clear" to settings-patch.json — the fix is right, the matcher format is valid (pipe-separated regex alternation is the established codebase pattern, confirmed by "startup|clear|compact" already in config_schema.py:260), and documentation updates are consistent across templates and the example mirror. One blocking gap: the fix does not work for existing users running cc-configure --retrofit.

Blocking

configure.py — retrofit leaves ghost matcherless SessionStart group that re-negates the fix

configure.py:_merge_hook_groups (around line 1248) deduplicates SessionStart groups by keying on the matcher value:

matcher = ng.get("matcher")                            # new template → "startup|clear"
targets = [g for g in out if g.get("matcher") == matcher]  # finds existing groups with SAME matcher

Before this PR, any user who already ran cc-configure has a SessionStart entry with no matcher key (g.get("matcher") == None). On retrofit, None != "startup|clear" so targets is empty and the new group is appended as a second entry alongside the old one. The old matcherless entry fires on every SessionStart source (startup, resume, clear, compact) while the new startup|clear entry is redundant — the old one wins and markers are still wiped on --resume and compact. The retrofit path reproduces the original bug for every existing installation.

A migration guard is needed: before appending the new startup|clear group, remove any existing SessionStart group whose hook commands overlap (e.g. contain .frozen) but whose matcher is None. The self-heal test (test/retrofit-hooks/test-self-heal-prior-buildup.sh, expected_sessstart=4) does not cover this case because it only tests exact-duplicate collapsing, not matcher-value-changed migration.

Advisory

test/microbit-enforcer/test-sessionstart-matcher.sh:11 — proj_root=$(pwd) breaks if not run from repo root

The established test pattern across the suite uses sys.path.insert(0, '.') (callers are expected to cd to repo root). This script instead captures $(pwd) and shell-interpolates it into the Python -c string. If a developer runs cd test/microbit-enforcer && bash test-sessionstart-matcher.sh, configure is not importable and the test fails with ModuleNotFoundError rather than a meaningful assertion failure. Replace with the repo-root-relative sys.path.insert(0, '.') pattern used by test-duplicate-hook-commands.sh, test-preflight-detects-violations.sh, etc., and use a python3 - <<'EOF' heredoc instead of -c to stay consistent with the codebase style.

…p on retrofit (PR #88 review)

Existing installs carry a matcherless SessionStart marker-clear group;
_merge_hook_groups keys by matcher, so cc-configure --retrofit would append
the new startup|clear group beside the old one and the stale matcherless group
kept wiping markers on resume/compact — re-negating the freeze-loss fix on
every upgrade (the same class as PR #73's standalone->bundled bug, for a
matcher change).

_merge_hook_groups now does a pre-pass: for each configurator command it
collects the matcher(s) the new template places it under, then strips that
command from any existing group whose matcher is not one of them (a stale
placement). This preserves the deliberate same-command-under-multiple-matchers
pattern (the mcp drift-check ships under both startup and resume — the
no-duplication / self-heal tests still expect 4 SessionStart groups) and never
touches a user's own command.

New test/retrofit-hooks/test-sessionstart-matcher-migration.sh. Also aligns
test-sessionstart-matcher.sh to the heredoc / sys.path '.' style used by the
merge tests (review advisory). --check clean, 104/104 fixtures.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
tigers1997 added a commit that referenced this pull request Jun 26, 2026
…t the repo root (dogfood F2) (#89)

New check_stack_reality() preflight warns at configure time when a configured
check command's (typecheck/lint/test) stack manifest is missing at the repo
root — the exact condition that makes stop-run-checks.sh silently skip at
runtime (skip rule 3). Maps each command's first binary via the shlex-guarded
extract_first_binaries() to a stack manifest (new BINARY_TO_MANIFEST, mirroring
manifest_for() in stop-run-checks.sh); names the subdir for monorepos
(frontend/package.json). When a warned binary is also absent from the host PATH
and a compose/Dockerfile is present, it notes the host hooks will no-op because
the toolchain lives in the container — host-PATH-gated, so a monorepo whose
compose file only backs services doesn't get a false warning.

Also hardens the pre-existing extract_first_binaries: shlex.split is now
guarded against ValueError (an unbalanced quote in a cmd_* field used to crash
the configurator).

Tests: test/cc-manifest/test-stack-reality-preflight.sh (8 cases incl. the
host-PATH gate via an injected which) + a malformed-quote case in
test-check-commands-extracted.sh. --check clean, 103/103 fixtures, 5 persona
snapshots, module smoke.

Split from the originally-bundled PR #87 per CONTRIBUTING.md (one logical
change per PR); F1 (the SessionStart matcher fix) ships as #88. Incorporates
review advisories A1 (host-PATH-gated container note) + A2 (reuse
extract_first_binaries).

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

VERDICT: COMMENT-ONLY

PR fixes the matcherless SessionStart bug correctly (the template, docs, and example mirror are all updated consistently) and adds sensible retrofit migration logic to avoid the stale-group double-fire on --retrofit. The two test files cover the primary scenario and user-hook preservation. Two advisory findings below; nothing blocking.

Advisory

configure.py:1268 — new falsy filter silently drops groups with a non-list hooks value

out = [g for g in out if g.get("hooks")]

This line is new in this PR. Before it, every group cloned from existing_groups survived in out unconditionally. After it, any group where g.get("hooks") is falsy — including groups with hooks: null or a missing hooks key entirely — is silently deleted, even though the migration loop above skipped them (isinstance(hooks, list) guard). In practice Claude Code always generates list-typed hooks, so real installs are unlikely to be affected, but the filter is doing double duty (intended: drop groups emptied by stripping; unintended: also drop schema-invalid groups that were never touched). A tighter guard — e.g. out = [g for g in out if g.get("hooks") != []] — would restrict deletion to groups the stripping loop actually emptied.

test/retrofit-hooks/test-sessionstart-matcher-migration.sh:59 — multi-matcher test exercises steady-state only, not migration

merged3, _ = deep_merge_settings(two, two)   # existing == new

two has DRIFT under both "startup" and "resume". Passing it as both existing and new means new_matchers[DRIFT] = {"startup", "resume"} — every existing group's matcher is already in the set, so the stripping branch never fires. The test asserts both matchers survive, which they would even if the migration block were entirely absent. The scenario it was meant to guard (a stale third matcher — e.g. None or "other" — being stripped while the two valid matchers are preserved) is untested. A failing regression in that direction would pass this test silently.

@claude

claude Bot commented Jun 26, 2026

Copy link
Copy Markdown

VERDICT: COMMENT-ONLY

PR #88 fixes the microbit-enforcer SessionStart marker-clear bug (matcherless hook fired on every source including resume/compact, silently wiping freeze markers mid-session) by adding "matcher": "startup|clear" to the settings-patch template. It also adds a retrofit migration path in _merge_hook_groups to collapse the stale matcherless group into the new scoped one on --retrofit, with accompanying docs and two new test scripts.

Blocking

None.

Advisory

test/retrofit-hooks/test-sessionstart-matcher-migration.sh:48 — Test case 2 doesn't assert CLEAR is absent from the matcherless group

Test case 2 (the mixed-group scenario: existing matcherless group with both CLEAR and a user's echo my-own-hook) only asserts:

assert any("my-own-hook" in h.get("command", "") for g in ss2 for h in g.get("hooks", []))

This passes as long as my-own-hook exists anywhere in ss2. If the migration code had a bug where it failed to strip CLEAR from the old matcherless group while still appending the new startup|clear group, CLEAR would exist in both — once matcherless (firing on every session source) and once under startup|clear. That is exactly the double-fire regression the PR is fixing, and the test would not catch it.

Test case 1 correctly asserts n_clear == 1 at line 35–36; the same check is worth adding to test case 2:

n_clear2 = sum(1 for g in ss2 for h in g.get("hooks", []) if ".frozen" in h.get("command", ""))
assert n_clear2 == 1, f"CLEAR must appear exactly once after migration; got {n_clear2}: {ss2}"

The test comment on line 38–39 is also slightly misleading: it says the mixed group "must be left untouched" but the code correctly strips CLEAR from it (leaving only the user hook); clearer wording would be something like "the user's own hook survives; the configurator command is migrated to its new matcher."

No blocking issues — the core logic, migration path, and primary test coverage are correct.

@tigers1997
tigers1997 merged commit 4d37367 into main Jun 26, 2026
4 checks passed
@tigers1997
tigers1997 deleted the dogfood/f1-sessionstart-matcher branch June 26, 2026 22:49
tigers1997 added a commit that referenced this pull request Jun 26, 2026
…dant import; tighten migration test (review follow-ups) (#90)

Three non-blocking advisories from the split dogfood PRs #88/#89:

(1) check_stack_reality's container-note gate used shutil.which(b), which
always returns None for a path-like binary like ./gradlew (a project-local
wrapper, never on PATH) — so a Gradle-wrapper project with a Dockerfile and no
root build.gradle got a spurious "toolchain in container" note. The gate now
file-checks path-like binaries ((target_dir / b).exists()) and only consults
shutil.which for bare names.

(2) the inline `import shutil` inside check_stack_reality was redundant (shutil
is imported at module top) and is removed.

(3) test-sessionstart-matcher-migration.sh case 2 now asserts the marker-clear
migrates OUT of a user's mixed matcherless group (exactly once, under
startup|clear), not merely that the user hook survives — and the F1 CHANGELOG
bullet's stale "subset" description is corrected to the shipped placement-set
pre-pass. New ./gradlew case in test-stack-reality-preflight.sh.

No behavior change for non-path stacks. --check clean, 105/105 fixtures.

Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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