From e5f426930e235abf00e1b09178841509f3d3baed Mon Sep 17 00:00:00 2001 From: bob Date: Fri, 26 Jun 2026 19:48:28 -0400 Subject: [PATCH] fix(preflight): host-PATH gate handles path-like binaries; drop redundant import; tighten migration test (review follow-ups) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- CHANGELOG.md | 4 +++- configure.py | 18 ++++++++++++------ .../test-stack-reality-preflight.sh | 14 +++++++++++++- .../test-sessionstart-matcher-migration.sh | 12 +++++++++--- 4 files changed, 37 insertions(+), 11 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c32b3be..8473e76 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,7 +4,9 @@ All notable changes to this project. Format: [Keep a Changelog](https://keepacha ## Unreleased -- **fix(commands): scope the microbit-enforcer SessionStart marker-clear to fresh-slate sources so a long session keeps its freezes (dogfood F1).** The `freeze`/`guard`/`careful` micro-behaviors drop `.claude/.frozen` / `.guarded` / `.careful` markers that the `microbit-enforcer.sh` PreToolUse hook reads to reject Write/Edit. The companion SessionStart hook that clears those markers shipped with **no `matcher`**, so it fired on *every* SessionStart source — including `resume` and `compact`, not just `startup`. Net effect: in a long session you `/freeze` a file, context compacts (a SessionStart with `source: compact`) or you `--resume`, and the marker is silently deleted mid-task — the enforcer stops protecting the file exactly when you'd most want it. **Fix:** the SessionStart entry in `templates/commands/microbit-enforcer/settings-patch.json` now carries `"matcher": "startup|clear"` (sources verified against `code.claude.com/docs/en/hooks-guide` — SessionStart sources are `startup`, `resume`, `clear`, `compact`, and the matcher filters on source), so markers clear only on a brand-new session or `/clear` and persist across `--resume`/`--continue` and compaction. The freeze/guard/careful `SKILL.md` lifecycle notes, the `microbit-enforcer.sh` header, and the `templates/core/CLAUDE.md` gitignore-block note are corrected to match — across both the templates and the `python-uv-fastapi` example mirror. New `test/microbit-enforcer/test-sessionstart-matcher.sh` asserts the merged settings carry the matcher. **Retrofit migration:** existing installs carry a *matcherless* SessionStart marker-clear group, and `_merge_hook_groups` keys by matcher — so without handling, `cc-configure --retrofit` would append the new `startup|clear` group beside the old one and the stale matcherless group would keep wiping markers on resume/compact, re-negating the fix on every upgrade (the same class as PR #73's standalone→bundled hook bug, for a matcher *change*). The merge now drops a stale different-matcher group whose commands are a subset of the new group's (a pure configurator group, no user additions), so the command migrates once to the new matcher; a group that also holds user hooks isn't a subset and is left untouched. New `test/retrofit-hooks/test-sessionstart-matcher-migration.sh`. Surfaced by dogfooding the configurator into a containerized project. +- **fix(preflight): host-PATH gate handles path-like binaries (`./gradlew`); drop redundant `import shutil`; tighten the retrofit-migration test (review follow-ups to #88/#89).** Three non-blocking advisories from the split dogfood PRs: **(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`/compose and no root `build.gradle` got a spurious "toolchain lives in the 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/retrofit-hooks/test-sessionstart-matcher-migration.sh` case 2 now asserts the marker-clear migrates *out* of a user's mixed matcherless group (appearing exactly once, under `startup|clear`), not merely that the user hook survives. New `./gradlew` case in `test/cc-manifest/test-stack-reality-preflight.sh`. No behavior change for non-path stacks. + +- **fix(commands): scope the microbit-enforcer SessionStart marker-clear to fresh-slate sources so a long session keeps its freezes (dogfood F1).** The `freeze`/`guard`/`careful` micro-behaviors drop `.claude/.frozen` / `.guarded` / `.careful` markers that the `microbit-enforcer.sh` PreToolUse hook reads to reject Write/Edit. The companion SessionStart hook that clears those markers shipped with **no `matcher`**, so it fired on *every* SessionStart source — including `resume` and `compact`, not just `startup`. Net effect: in a long session you `/freeze` a file, context compacts (a SessionStart with `source: compact`) or you `--resume`, and the marker is silently deleted mid-task — the enforcer stops protecting the file exactly when you'd most want it. **Fix:** the SessionStart entry in `templates/commands/microbit-enforcer/settings-patch.json` now carries `"matcher": "startup|clear"` (sources verified against `code.claude.com/docs/en/hooks-guide` — SessionStart sources are `startup`, `resume`, `clear`, `compact`, and the matcher filters on source), so markers clear only on a brand-new session or `/clear` and persist across `--resume`/`--continue` and compaction. The freeze/guard/careful `SKILL.md` lifecycle notes, the `microbit-enforcer.sh` header, and the `templates/core/CLAUDE.md` gitignore-block note are corrected to match — across both the templates and the `python-uv-fastapi` example mirror. New `test/microbit-enforcer/test-sessionstart-matcher.sh` asserts the merged settings carry the matcher. **Retrofit migration:** existing installs carry a *matcherless* SessionStart marker-clear group, and `_merge_hook_groups` keys by matcher — so without handling, `cc-configure --retrofit` would append the new `startup|clear` group beside the old one and the stale matcherless group would keep wiping markers on resume/compact, re-negating the fix on every upgrade (the same class as PR #73's standalone→bundled hook bug, for a matcher *change*). The merge now does a per-command 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 under a *different* matcher (migrating it once). This preserves the deliberate same-command-under-multiple-matchers pattern (the mcp drift-check ships under both `startup` and `resume`) and a user's own commands in a mixed group — only the configurator command migrates out. New `test/retrofit-hooks/test-sessionstart-matcher-migration.sh`. Surfaced by dogfooding the configurator into a containerized project. - **feat(preflight): `[ STACK WARNINGS ]` — flag a configured check stack that doesn't exist at the repo root (dogfood F2).** A user who keeps the default Node/pnpm intake answers (or otherwise configures `pnpm`/`uv`/`cargo`/… check commands) on a project whose manifest lives in a subdir or behind a container got **zero feedback** that the scaffolded Stop hook would never run: `stop-run-checks.sh` skips any check whose stack manifest is absent at the root (skip rule 3), so the whole typecheck/lint/test loop self-disables silently. New `check_stack_reality(target_dir, form_values)` preflight maps each configured check command's (typecheck/lint/test) first-binary — via the shlex-guarded `extract_first_binaries` — to its stack manifest (new `BINARY_TO_MANIFEST`, mirroring `manifest_for()` in `stop-run-checks.sh`) and warns when that manifest is missing from the project root, naming the subdir when it's one level down (`frontend/package.json`). When a warned binary is **also absent from the host PATH** and a `docker-compose.yml`/`Dockerfile` is present, it appends a note that the host-side format/check hooks will no-op because the toolchain lives in the container — host-PATH-gated, so a monorepo whose compose file only backs services (host toolchain present) doesn't get a false "no-op" warning. Warning-only; never blocks scaffolding; stays silent when the stack matches the root or no command maps to a known manifest (bare `tsc`/`pytest`/`ruff`). Rendered as a `[ STACK WARNINGS ]` block alongside the existing preflights. New `test/cc-manifest/test-stack-reality-preflight.sh` (matched-silent, unmappable-silent, monorepo-names-subdir, container-note-when-toolchain-off-host, container-note-suppressed-when-on-host, absent-everywhere, install-only-silent, malformed-quote-safe) + a malformed-quote case in `test-check-commands-extracted.sh`. Split out of the originally-bundled #87 per `CONTRIBUTING.md` (one logical change per PR); F1 (the SessionStart matcher fix) ships separately. Surfaced by dogfooding the configurator into a Python + FastAPI + Vite project that runs entirely via Docker. diff --git a/configure.py b/configure.py index 75deae3..f176724 100755 --- a/configure.py +++ b/configure.py @@ -1013,7 +1013,6 @@ def check_stack_reality(target_dir, form_values: dict, which=None) -> list: command maps to a known manifest (e.g. bare tsc / pytest / ruff).""" fv = form_values or {} if which is None: - import shutil which = shutil.which # Reuse extract_first_binaries (shlex-guarded) for the three commands the @@ -1062,11 +1061,18 @@ def check_stack_reality(target_dir, form_values: dict, which=None) -> list: ".claude-config.json or re-run cc-configure.".format( b=bins_str, m=manifest)) - # Container note only when a warned toolchain binary isn't on the host PATH — - # then a compose/Dockerfile means it likely lives in the container and the - # host-side hooks no-op. If the binary IS on the host (e.g. a monorepo whose - # compose file is only for backing services), the hooks run fine. - if warnings and any(which(b) is None for b in warned_bins): + # Container note only when a warned toolchain binary isn't reachable on the + # host — then a compose/Dockerfile means it likely lives in the container and + # the host-side hooks no-op. If the binary IS on the host (e.g. a monorepo + # whose compose file is only for backing services), the hooks run fine. A + # path-like binary (`./gradlew`, `./mvnw`) is never on PATH, so check the file + # relative to the project root instead of shutil.which (which always returns + # None for it, which would fire the note spuriously). + def _off_host(b): + if "/" in b: + return not (target_dir / b).exists() + return which(b) is None + if warnings and any(_off_host(b) for b in warned_bins): container = next( (n for n in ("docker-compose.yml", "docker-compose.yaml", "compose.yml", "compose.yaml", "Dockerfile") diff --git a/test/cc-manifest/test-stack-reality-preflight.sh b/test/cc-manifest/test-stack-reality-preflight.sh index e1a57f4..0366b2f 100755 --- a/test/cc-manifest/test-stack-reality-preflight.sh +++ b/test/cc-manifest/test-stack-reality-preflight.sh @@ -87,4 +87,16 @@ out=$(run "$d" "{\"cmd_test\":\"pnpm test --grep 'foo\"}" all) echo "$out" | grep -q "no package.json at the project root" \ || { echo "FAIL: malformed-quote cmd should degrade gracefully and still warn; got: $out"; exit 1; } -echo "PASS: check_stack_reality — root-mismatch warns, names subdirs, container note gated on host-PATH absence, stop-hook-key scope, survives malformed quotes, silent when matched" +# 8. Path-like binary (./gradlew) present at root -> NOT off-host (it's a local +# file, not a PATH binary), so the container note is suppressed even with a +# Dockerfile/compose. shutil.which('./gradlew') always returns None; the gate +# file-checks path-like binaries instead of trusting PATH. (which-mode is +# irrelevant here — the path branch never calls which.) +d="$tmp/gradlew"; mkdir -p "$d/app"; touch "$d/app/build.gradle" "$d/gradlew" "$d/docker-compose.yml" +out=$(run "$d" '{"cmd_test":"./gradlew test"}' none) +echo "$out" | grep -q "no build.gradle at the project root" \ + || { echo "FAIL: gradlew monorepo should warn about missing root build.gradle; got: $out"; exit 1; } +echo "$out" | grep -q "docker compose exec" \ + && { echo "FAIL: container note must be suppressed when ./gradlew exists at root; got: $out"; exit 1; } || true + +echo "PASS: check_stack_reality — root-mismatch warns, names subdirs, container note gated on host reachability (incl. path-like ./gradlew), stop-hook-key scope, survives malformed quotes, silent when matched" diff --git a/test/retrofit-hooks/test-sessionstart-matcher-migration.sh b/test/retrofit-hooks/test-sessionstart-matcher-migration.sh index 83347dc..5bb48f6 100755 --- a/test/retrofit-hooks/test-sessionstart-matcher-migration.sh +++ b/test/retrofit-hooks/test-sessionstart-matcher-migration.sh @@ -35,8 +35,9 @@ assert ss[0].get("matcher") == "startup|clear", \ n_clear = sum(1 for g in ss for h in g.get("hooks", []) if ".frozen" in h.get("command", "")) assert n_clear == 1, f"marker-clear command should appear exactly once, got {n_clear}: {ss}" -# A matcherless group that ALSO holds a user command is not a pure configurator -# group (not a subset), so it must be left untouched — user hook preserved. +# A matcherless group that ALSO holds a user command: the migration is +# per-command, so the configurator's marker-clear migrates OUT to startup|clear +# while the user's own hook stays put under the matcherless group. existing2 = {"hooks": {"SessionStart": [ {"hooks": [ {"type": "command", "command": CLEAR, "timeout": 5}, @@ -45,8 +46,13 @@ existing2 = {"hooks": {"SessionStart": [ ]}} merged2, _ = deep_merge_settings(existing2, new) ss2 = merged2["hooks"]["SessionStart"] -assert any("my-own-hook" in h.get("command", "") for g in ss2 for h in g.get("hooks", [])), \ +matcherless2 = [g for g in ss2 if g.get("matcher") is None] +assert any("my-own-hook" in h.get("command", "") for g in matcherless2 for h in g.get("hooks", [])), \ f"a user's own SessionStart hook must be preserved across retrofit: {ss2}" +assert not any(".frozen" in h.get("command", "") for g in matcherless2 for h in g.get("hooks", [])), \ + f"the marker-clear must migrate OUT of the matcherless group (not duplicate): {ss2}" +assert sum(1 for g in ss2 for h in g.get("hooks", []) if ".frozen" in h.get("command", "")) == 1, \ + f"marker-clear should appear exactly once after migration: {ss2}" # Deliberate same-command-under-multiple-matchers (the mcp drift-check ships # under both startup and resume) must survive — both are valid placements in