diff --git a/plugins/disk-hygiene/.claude-plugin/plugin.json b/plugins/disk-hygiene/.claude-plugin/plugin.json index 13fd6058b..13dee8761 100644 --- a/plugins/disk-hygiene/.claude-plugin/plugin.json +++ b/plugins/disk-hygiene/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "disk-hygiene", - "version": "0.6.2", + "version": "0.6.3", "description": "Context-aware disk hygiene for arbitrary directory trees: inventories orphaned and temporary artifacts, classifies evidence into review tiers, and offers exact-path cleanup only after a fresh safety preview and explicit per-tier approval. The target is read-only by default; OS-managed paths, links and mount points, VCS-tracked content, changed entries, and live-handle uncertainty fail closed.", "author": { "name": "Melodic Software", diff --git a/plugins/disk-hygiene/CHANGELOG.md b/plugins/disk-hygiene/CHANGELOG.md index a3e0ecb12..f991f1493 100644 --- a/plugins/disk-hygiene/CHANGELOG.md +++ b/plugins/disk-hygiene/CHANGELOG.md @@ -3,6 +3,38 @@ All notable changes to the `disk-hygiene` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.6.3] + +### Fixed + +- **The destructive-action guard was failing open on the bundled `clean` skill.** The + skill-frontmatter PreToolUse hook passed `--authorized-data-root ${CLAUDE_PLUGIN_DATA}` in its + args, but Claude Code refuses to launch a skill-scoped hook that references `${CLAUDE_PLUGIN_DATA}` + (it is plugin-only; only `${CLAUDE_PLUGIN_ROOT}` is available to skill hooks) and treats the failed + launch as a non-blocking error — so the guard silently never ran and `rm -rf`, engine `apply`, and + the PowerShell deletion belt were all ungated. This recurs the fail-open shape earlier fixes + addressed through a new vector (hook launch failure via an unsupported substitution token); the + 0.4.4 premise that "inline placeholder substitution resolves in exec-form hook args" does not hold + for `${CLAUDE_PLUGIN_DATA}` in a skill-scoped hook. + - The hook now passes only `--plugin-root ${CLAUDE_PLUGIN_ROOT}` — the sole substitution a skill + hook receives — so it always launches. `destructive_guard.py` derives the authorized data root + from the plugin root using Claude Code's documented persistent-data-directory layout + (`/data/`, `` = the sanitized `@`). Every failure mode is + fail-closed: an unrecognized layout yields no authority, so `--data-root` engine calls are denied + while the destructive-action guard stays fully active. A direct `--authorized-data-root` and the + `CLAUDE_PLUGIN_DATA` environment variable remain accepted as additional/fallback channels for + hosts that can supply them. + - **Known limitation (platform gap):** the `disk_hygiene_enabled` kill switch can no longer reach + the guard on a skill-frontmatter hook. Its only channels are the `--disk-hygiene-enabled` argv + flag (which needs the `${user_config.*}` substitution skill hooks do not receive) and the + `CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED` environment variable (which the runtime does not + inject into skill hooks). The guard therefore defaults to enabled and cannot honor a configured + `false` by denying outright; it still forces a human prompt before every mutation, and the skill + body's substituted value lets the model self-enforce audit-only. This never functioned on 0.4.6 + either (the hook did not launch at all), so it is a documented gap rather than a regression. + Delivering the kill switch to a skill-scoped guard needs a channel skill hooks do not yet have. + (#983) + ## [0.6.2] ### Fixed diff --git a/plugins/disk-hygiene/README.md b/plugins/disk-hygiene/README.md index 6e28e2de7..0112cc21e 100644 --- a/plugins/disk-hygiene/README.md +++ b/plugins/disk-hygiene/README.md @@ -137,10 +137,14 @@ hand-cleaning the zone. - **MCP / external trust:** no MCP server, agent, dependency, or third-party service is shipped. - **Configuration:** one non-sensitive `userConfig` boolean (`disk_hygiene_enabled`, default `true`) gating the execution tiers — setting it `false` puts `/disk-hygiene:clean` in audit-only - mode (enforced by the skill-scoped guard, which denies every deletion lane there; a direct - `hygiene.py` invocation outside that skill does not read the toggle and answers only to the - engine's own preview/approval-token gate). The toggle can only narrow the destructive surface, - never widen it. No credentials. Policy comes from an explicit invocation + mode. That mode is enforced by the skill, which resolves the toggle through the bundled kill-switch + probe and self-enforces; the skill-scoped guard cannot independently enforce it, because a + skill-frontmatter hook reaches the guard with neither the `${user_config.*}` substitution nor the + `CLAUDE_PLUGIN_OPTION_*` environment variable, though the guard still forces a human prompt before + every mutation. A direct `hygiene.py` invocation outside that skill does not read the toggle and + answers only to the engine's own preview/approval-token gate. The toggle can only narrow the + destructive surface, never widen it (see [the safety model](skills/clean/reference/safety-model.md) + for the degraded-mode detail). No credentials. Policy comes from an explicit invocation argument or standing `disk-hygiene.json` files under `~/.claude/` and the consumer project's `.claude/`. All policy input is pattern-only and additive: it can add protections and discovery hints or disable hints, and cannot weaken hard guards or authorize removal, so ambient config diff --git a/plugins/disk-hygiene/skills/clean/SKILL.md b/plugins/disk-hygiene/skills/clean/SKILL.md index d39e21ee1..be798643a 100644 --- a/plugins/disk-hygiene/skills/clean/SKILL.md +++ b/plugins/disk-hygiene/skills/clean/SKILL.md @@ -10,7 +10,7 @@ hooks: hooks: - type: command command: "python3" - args: ["${CLAUDE_PLUGIN_ROOT}/skills/clean/scripts/destructive_guard.py", "--authorized-data-root", "${CLAUDE_PLUGIN_DATA}", "--disk-hygiene-enabled", "${user_config.disk_hygiene_enabled}"] + args: ["${CLAUDE_PLUGIN_ROOT}/skills/clean/scripts/destructive_guard.py", "--plugin-root", "${CLAUDE_PLUGIN_ROOT}"] --- # Disk hygiene @@ -42,14 +42,14 @@ directory, symlink, or Windows reparse point. running the bundled probe (the guard allows exactly this argument-free shape): `"" "${CLAUDE_PLUGIN_ROOT}/skills/setup/scripts/kill_switch_probe.py"` and honor the `effective` value it reports; on `degraded: true` proceed as enabled but say the configured - value could not be read. In this audit-only mode the guard denies every - deletion lane, including the flagged PowerShell mutation spellings, not only the Bash engine - apply. The kill-switch value reaches the guard as a runtime-substituted hook argument - (`--disk-hygiene-enabled ${user_config.disk_hygiene_enabled}`), so a configured `false` is - honored even where the runtime does not inject the `CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED` - environment variable; the environment variable is only a fallback. The hook - runs in shell-free exec form and reports its absolute Python interpreter and the authorized - `--data-root` value in denial guidance. Use that exact interpreter path as `` for + value could not be read. Honoring that value is your responsibility: a skill-frontmatter hook + receives neither the `${user_config.*}` substitution nor the + `CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED` environment variable, so the guard cannot independently + enforce audit-only mode — it stays active and still forces a human prompt before every mutation, + but a configured `false` reaches only you, not the guard. Do not treat the guard as the kill + switch's backstop here. The hook runs in shell-free exec form and reports its absolute Python + interpreter and the authorized `--data-root` value in denial guidance. Use that exact interpreter + path as `` for every engine call; bare `python`/`python3` is rejected because Bash aliases and functions can replace them. If either value is not known yet, submit the otherwise exact scan shape once with bare `python`: the guard must deny it and report both, after which retry the scan with the @@ -71,9 +71,12 @@ stay there, never in the target or `${CLAUDE_PLUGIN_ROOT}`. Run: [--max-depth ] [--confirmed-large-scan] ``` -The guard validates `--data-root` against the authorized data root it receives as a -runtime-substituted hook argument (`${CLAUDE_PLUGIN_DATA}`), so generated state provably lands in the -plugin data directory even when the shell environment lacks `CLAUDE_PLUGIN_DATA`. +The guard validates `--data-root` against the plugin data directory it derives from +`${CLAUDE_PLUGIN_ROOT}` (passed to the guard as `--plugin-root`, the only substitution a +skill-frontmatter hook receives), confining generated state to the plugin data directory even when +the guard's own environment lacks `CLAUDE_PLUGIN_DATA`. If the guard cannot recognize the install +layout it derives no authority and denies `--data-root` engine calls rather than trusting a guessed +path, so re-run reporting a denial is a coverage gap, not a clean result. For a large root (a home directory, anything whose recursive walk could exceed the engine's entry cap), start with a bounded pass: add `--max-depth 1` to inventory the target's loose files and diff --git a/plugins/disk-hygiene/skills/clean/reference/safety-model.md b/plugins/disk-hygiene/skills/clean/reference/safety-model.md index 1a79a657a..79f6c36ce 100644 --- a/plugins/disk-hygiene/skills/clean/reference/safety-model.md +++ b/plugins/disk-hygiene/skills/clean/reference/safety-model.md @@ -54,24 +54,58 @@ The skill-scoped Bash guard accepts only complete literal words in the three dec shapes. It rejects every Bash expansion family, glob/word-splitting input, redirection, operator, escape, and compound-command form before validating arguments. Canonical script-path comparison uses the host platform's path case rules; POSIX path identity is never case-folded. A `--data-root` value -is accepted only when it matches the authorized data root the guard receives as a runtime-substituted -hook argument (`${CLAUDE_PLUGIN_DATA}`) — the shell environment is never trusted for it (an env var is -honored only as a fallback), and absent that authority the flag fails closed. `--max-depth` accepts -only a bare positive-integer literal. `--confirmed-large-scan` is the one valueless scan flag; the -guard permits at most one and rejects any trailing value, so the scan grammar stays exact. +is accepted only when it matches the plugin data directory the guard derives from +`${CLAUDE_PLUGIN_ROOT}` — the only substitution a skill-frontmatter hook receives, passed to the +guard as `--plugin-root` and mapped to `/data/` per the documented +[persistent-data-directory](https://code.claude.com/docs/en/plugins-reference#persistent-data-directory) +layout. A host that can substitute `${CLAUDE_PLUGIN_DATA}` itself may instead pass it directly as +`--authorized-data-root`, and the `CLAUDE_PLUGIN_DATA` environment variable is honored last; absent +every channel the flag fails closed. `--max-depth` accepts only a bare positive-integer literal. +`--confirmed-large-scan` is the one valueless scan flag; the guard permits at most one and rejects +any trailing value, so the scan grammar stays exact. + +Deriving the data root from `${CLAUDE_PLUGIN_ROOT}` couples to the one undocumented part of that +layout — the `cache///` shape of the installation root (the install root +is the version leaf; a directly-linked local install omits it). The guard anchors on the +`/cache` marker rather than a fixed depth, taking the marketplace and name from the two +segments after `cache` and reading `data` as `cache`'s sibling, so a version leaf does not shift the +result. That coupling is acceptable only because its sole failure mode is fail-closed: an +unrecognized layout yields no authority, so `--data-root` engine calls are denied while the +destructive-action guard stays fully active. The plugins reference documents all three path +variables (`CLAUDE_PLUGIN_ROOT`/`CLAUDE_PLUGIN_DATA`/`CLAUDE_PROJECT_DIR`) as exported to hook +processes as environment variables, so the guard's `CLAUDE_PLUGIN_DATA` env fallback should carry the +authority wherever the runtime honors that for skill hooks — the derivation is then a redundant belt. +An earlier Claude Code build was observed not to export it to a skill hook, which is why both +channels exist. + +A `claude --plugin-dir ` development session is the one shape with no derivable authority: a +bare checkout has no `/cache/` structure and no stable marketplace-keyed data +``, so the marker walk finds nothing. That dev workflow relies solely on the `CLAUDE_PLUGIN_DATA` +environment variable; where a Claude Code build does not export it to a skill hook, the engine lane is +fail-closed there (every `--data-root` invocation denied) while the destructive-action guard itself +stays fully active. This is a deliberate safe-over-convenient tradeoff for a development-only mode, +not a security gap — a local developer sets `CLAUDE_PLUGIN_DATA` or exercises the engine lane through +a real marketplace install. The same guard also covers the PowerShell tool with the inverse tradeoff: PowerShell stays open for read-only support work, while engine invocations are hard-denied (Bash is the only engine lane) and known deletion spellings and .NET Delete calls resolve against the `disk_hygiene_enabled` kill -switch — the same bar as the engine apply lane. When execution is enabled they are downgraded to a -final human permission prompt; in audit-only mode (`disk_hygiene_enabled` is `false`) they are -denied outright, so the kill switch blocks deletions on the PowerShell lane too and not only the -Bash engine apply. The kill-switch value reaches the guard as a runtime-substituted hook argument -(`--disk-hygiene-enabled ${user_config.disk_hygiene_enabled}`), so a configured `false` holds even -where the runtime does not inject `CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED` into the hook -environment (the environment variable is only a fallback). That lane is a raised bar, not -fail-closed: an unknown mutation spelling passes it, so the engine's own containment, revalidation, -and platform gates remain the deletion authority. +switch. When the guard sees execution enabled they are downgraded to a final human permission prompt; +when it sees a configured `false` (audit-only mode) they are denied outright, so the kill switch would +block deletions on the PowerShell lane too and not only the Bash engine apply. + +That kill-switch enforcement is, however, only as reachable as the value is. The guard reads it from a +`--disk-hygiene-enabled` argv flag or the `CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED` environment +variable, but a skill-frontmatter hook receives neither — Claude Code substitutes only +`${CLAUDE_PLUGIN_ROOT}` into a skill hook's args and does not inject `CLAUDE_PLUGIN_OPTION_*` into its +environment. So in the bundled skill deployment the guard defaults to enabled and cannot honor a +configured `false` by denying; it still forces a human prompt before every mutation, and the model +itself reads the substituted `disk_hygiene_enabled` value from the skill content and self-enforces +audit-only. Enforcing the kill switch in the guard needs a delivery channel skill hooks do not yet +have (a plugin-scoped hook or MCP server that can carry the value, or Claude Code adding +`${user_config.*}` substitution for skill hooks). Even when the switch is reachable, the PowerShell +lane is a raised bar, not fail-closed: an unknown mutation spelling passes it, so the engine's own +containment, revalidation, and platform gates remain the deletion authority. A depth-limited scan records every directory it declined to enter in `truncated_paths`. Truncated directories have no captured descendant set, so the preview blocks them (and anything beneath them) diff --git a/plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py b/plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py index 1a9a47120..018388571 100755 --- a/plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py +++ b/plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py @@ -112,6 +112,52 @@ def _data_root_key(value: str) -> str: _CLAUDE_PLUGIN_DATA_ENV = "CLAUDE_PLUGIN_DATA" _AUTHORIZED_DATA_ROOT_PLACEHOLDER = f"${{{_CLAUDE_PLUGIN_DATA_ENV}}}" +_PLUGIN_ROOT_FLAG = "--plugin-root" +_PLUGIN_ROOT_PLACEHOLDER = "${CLAUDE_PLUGIN_ROOT}" +_PLUGIN_CACHE_DIRNAME = "cache" +_PLUGINS_DIRNAME = "plugins" +_PLUGIN_DATA_DIRNAME = "data" +_PLUGIN_ID_DISALLOWED = re.compile(r"[^A-Za-z0-9_-]") + + +def _plugin_data_root_from_root(plugin_root: str) -> str | None: + """Derive the persistent data root from the plugin's installation root. + + A skill-frontmatter hook may substitute only ``${CLAUDE_PLUGIN_ROOT}`` into + its args — ``${CLAUDE_PLUGIN_DATA}`` is plugin-only and makes Claude Code + refuse to launch a skill hook — so the guard reconstructs the data root from + the installation root. Claude Code lays a marketplace plugin out at + ``/cache///`` — the install root is the + version leaf — and persists its data at ``/data/``, where ```` + is ``@`` with every character outside ``[A-Za-z0-9_-]`` + replaced by ``-`` (plugins reference, "Persistent data directory"). + + The install root is version-specific, so this anchors on the + ``/cache`` marker rather than a fixed depth: the segment after + ``cache`` is the marketplace, the next is the name, any further segments (a + ```` leaf, absent for a directly-linked local install) are ignored, + and ``data`` is ``cache``'s sibling. A root without that marker, or with no + name segment after the marketplace, yields ``None`` so the caller fails closed + instead of trusting a guessed path. + """ + parts = Path(plugin_root).parts + for index in range(1, len(parts)): + if not ( + parts[index].casefold() == _PLUGIN_CACHE_DIRNAME + and parts[index - 1].casefold() == _PLUGINS_DIRNAME + ): + continue + if index + 2 >= len(parts): + return None + marketplace, name = parts[index + 1], parts[index + 2] + if not marketplace or not name: + return None + plugin_id = _PLUGIN_ID_DISALLOWED.sub("-", f"{name}@{marketplace}") + plugins_dir = Path(*parts[:index]) + return os.fspath(plugins_dir / _PLUGIN_DATA_DIRNAME / plugin_id) + return None + + _DISK_HYGIENE_ENABLED_FLAG = "--disk-hygiene-enabled" _DISK_HYGIENE_ENABLED_ENV = "CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED" _DISK_HYGIENE_ENABLED_PLACEHOLDER = "${user_config.disk_hygiene_enabled}" @@ -120,10 +166,10 @@ def _data_root_key(value: str) -> str: def _argv_flag_value(argv: list[str], flag: str) -> str | None: """Read the runtime-substituted value the frontmatter hook passed for ``flag``. - Inline placeholder substitution resolves in hook ``args`` even where - environment injection does not, so the guard's own argv is the authoritative, - model-unforgeable channel for a value the harness controls. Accepts both the - space-separated (``--flag value``) and ``--flag=value`` spellings. + The guard's own argv is a model-unforgeable channel for a value the harness + substitutes at launch (e.g. ``${CLAUDE_PLUGIN_ROOT}``), reaching the guard even + where environment injection does not. Accepts both the space-separated + (``--flag value``) and ``--flag=value`` spellings. """ for index, token in enumerate(argv): if token == flag and index + 1 < len(argv): @@ -140,14 +186,30 @@ def _argv_authorized_data_root(argv: list[str]) -> str | None: def resolve_authorized_data_root() -> str | None: - """Resolve the authoritative data root from the hook argv, then the environment. + """Resolve the authoritative data root a skill-frontmatter hook can supply. + + Precedence, highest first: + + 1. ``--authorized-data-root`` — a direct path, for a host that can substitute + ``${CLAUDE_PLUGIN_DATA}`` itself (a plugin ``hooks.json`` hook can; a skill + hook cannot). + 2. ``--plugin-root ${CLAUDE_PLUGIN_ROOT}`` — the only substitution a skill hook + receives; the data root is derived from it. This is the channel the bundled + ``clean`` skill uses. + 3. The ``CLAUDE_PLUGIN_DATA`` environment variable, if present. - A literal, unsubstituted placeholder is treated as absent so the environment - fallback still applies. + A literal, unsubstituted placeholder is treated as absent at each step. Absent + every channel the guard has no authority and every ``--data-root`` engine call + fails closed. """ - from_argv = _argv_authorized_data_root(sys.argv[1:]) - if from_argv and from_argv != _AUTHORIZED_DATA_ROOT_PLACEHOLDER: - return from_argv + direct = _argv_flag_value(sys.argv[1:], _AUTHORIZED_DATA_ROOT_FLAG) + if direct and direct != _AUTHORIZED_DATA_ROOT_PLACEHOLDER: + return direct + plugin_root = _argv_flag_value(sys.argv[1:], _PLUGIN_ROOT_FLAG) + if plugin_root and plugin_root != _PLUGIN_ROOT_PLACEHOLDER: + derived = _plugin_data_root_from_root(plugin_root) + if derived: + return derived return os.environ.get(_CLAUDE_PLUGIN_DATA_ENV) @@ -155,14 +217,20 @@ def resolve_disk_hygiene_enabled() -> bool: """Resolve the execution kill switch from the hook argv, then the environment. The kill switch is a safety control: ``false`` is audit-only mode and must - prevent every deletion lane. The runtime does not inject - ``CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED`` into a skill-frontmatter hook's - process environment, so the configured value reaches the guard through the - hook argv (``--disk-hygiene-enabled ${user_config.disk_hygiene_enabled}``); - the environment variable is honored only as a fallback. A literal, - unsubstituted placeholder or an empty value is treated as absent. When no - channel supplies a value the guard fails safe to enabled — the guard stays - active and still gates every mutation behind the final human prompt. + prevent every deletion lane. A host supplies the value either as a + ``--disk-hygiene-enabled`` argv flag or as the + ``CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED`` environment variable; argv wins. + A literal, unsubstituted placeholder or an empty value is treated as absent. + + A skill-frontmatter hook can supply neither reliably: Claude Code substitutes + only ``${CLAUDE_PLUGIN_ROOT}`` into a skill hook's args (so + ``${user_config.disk_hygiene_enabled}`` cannot be passed on argv), and it does + not inject ``CLAUDE_PLUGIN_OPTION_*`` into a skill hook's environment. In that + deployment no channel supplies a value and the guard fails safe to enabled — + it stays active and still gates every mutation behind the final human prompt, + but cannot honor a configured ``false`` by denying outright. Delivering the + kill switch to a skill-scoped guard needs a channel skill hooks do not yet + have; see the plugin's safety model. """ from_argv = _argv_flag_value(sys.argv[1:], _DISK_HYGIENE_ENABLED_FLAG) if from_argv and from_argv != _DISK_HYGIENE_ENABLED_PLACEHOLDER: @@ -369,9 +437,10 @@ def _bash_denial_guidance(authority: str | None) -> str: f' Pass --data-root "{data_root}" so generated state lands in the plugin data directory.' if data_root else ( - " The guard did not receive an authorized data root (neither the" - f" {_AUTHORIZED_DATA_ROOT_FLAG} hook argument nor CLAUDE_PLUGIN_DATA)," - " so --data-root cannot be validated and engine calls fail closed." + " The guard did not receive an authorized data root (none of the" + f" {_PLUGIN_ROOT_FLAG} or {_AUTHORIZED_DATA_ROOT_FLAG} hook arguments" + " nor CLAUDE_PLUGIN_DATA resolved one), so --data-root cannot be" + " validated and engine calls fail closed." ) ) return ( diff --git a/plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py b/plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py index ac3dc86e6..9ad1af709 100755 --- a/plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py +++ b/plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py @@ -1270,11 +1270,13 @@ def run_guard_enabled_argv( ) -> dict[str, object] | None: """Drive the guard with the kill switch supplied via hook argv, not the env. - Mirrors the skill-frontmatter hook, which substitutes - ``--disk-hygiene-enabled ${user_config.disk_hygiene_enabled}`` into the - guard's own argv while CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED is absent - from the hook process environment — the exact env-injection failure this - fix addresses. + Exercises the ``--disk-hygiene-enabled`` argv channel with + CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED absent — the delivery a host that + can substitute ``${user_config.disk_hygiene_enabled}`` (a plugin + ``hooks.json`` hook) would use. The bundled skill-frontmatter hook cannot + supply this value (Claude Code substitutes only ``${CLAUDE_PLUGIN_ROOT}`` + into skill-hook args), so this proves the guard's argv path itself, not the + shipped skill deployment. """ argv = [ str(SCRIPT_DIR / "destructive_guard.py"), @@ -1592,11 +1594,13 @@ def test_guard_denies_data_root_without_hook_authority(self) -> None: def run_guard_hook_argv( self, command: str, authorized: str | None ) -> dict[str, object]: - """Drive the guard with the data root supplied via hook argv, not the env. + """Drive the guard with the data root supplied directly via hook argv. - Mirrors the skill-frontmatter hook, which substitutes - ``--authorized-data-root ${CLAUDE_PLUGIN_DATA}`` into the guard's own argv - while CLAUDE_PLUGIN_DATA is absent from the hook process environment. + Exercises the ``--authorized-data-root`` channel — the direct path a host + that can substitute ``${CLAUDE_PLUGIN_DATA}`` itself (a plugin ``hooks.json`` + hook) may pass — with CLAUDE_PLUGIN_DATA absent from the process + environment. The bundled ``clean`` skill instead uses ``--plugin-root`` + (see ``run_guard_plugin_root``), the only substitution a skill hook gets. """ argv = [str(SCRIPT_DIR / "destructive_guard.py")] if authorized is not None: @@ -1700,6 +1704,57 @@ def test_guard_denies_data_root_when_argv_and_env_both_absent(self) -> None: result["hookSpecificOutput"]["permissionDecisionReason"], ) + def run_guard_plugin_root( + self, command: str, plugin_root: str + ) -> dict[str, object]: + """Drive the guard exactly as the shipped skill hook does. + + The skill-frontmatter hook passes ``--plugin-root ${CLAUDE_PLUGIN_ROOT}`` + (the only substitution a skill hook receives) and nothing else; the guard + derives the authorized data root from it. CLAUDE_PLUGIN_DATA is absent from + the process environment, the exact skill-hook condition. + """ + argv = [str(SCRIPT_DIR / "destructive_guard.py"), "--plugin-root", plugin_root] + environment = { + key: value + for key, value in os.environ.items() + if key != "CLAUDE_PLUGIN_DATA" + } + environment["CLAUDE_PLUGIN_OPTION_DISK_HYGIENE_ENABLED"] = "true" + stdin = io.StringIO(json.dumps({"tool_input": {"command": command}})) + stdout = io.StringIO() + with ( + mock.patch("sys.stdin", stdin), + redirect_stdout(stdout), + mock.patch.object(guard.sys, "argv", argv), + mock.patch.dict("os.environ", environment, clear=True), + ): + self.assertEqual(0, guard.main()) + return json.loads(stdout.getvalue()) + + def test_guard_derives_data_root_from_plugin_root_argv(self) -> None: + script = SCRIPT_DIR / "hygiene.py" + with tempfile.TemporaryDirectory() as temporary: + plugins = Path(temporary).resolve() / "plugins" + # The install root is the version leaf under cache///. + plugin_root = plugins / "cache" / "acme" / "disk-hygiene" / "0.4.8" + plugin_root.mkdir(parents=True) + authorized = str(plugins / "data" / "disk-hygiene-acme") + other = str(plugins / "data" / "elsewhere") + base = f'"{self.python_command()}" "{script}" scan --target t --output s' + self.assertEqual( + "allow", + self.run_guard_plugin_root( + f'{base} --data-root "{authorized}"', str(plugin_root) + )["hookSpecificOutput"]["permissionDecision"], + ) + self.assertEqual( + "deny", + self.run_guard_plugin_root( + f'{base} --data-root "{other}"', str(plugin_root) + )["hookSpecificOutput"]["permissionDecision"], + ) + def test_argv_authorized_data_root_parses_both_arg_spellings(self) -> None: self.assertEqual( "/data", guard._argv_authorized_data_root(["--authorized-data-root", "/data"]) @@ -1743,6 +1798,82 @@ def test_resolve_authorized_data_root_precedence(self) -> None: "contains ${ must be preserved as the argv authority", ) + def test_resolve_authorized_data_root_derives_from_plugin_root(self) -> None: + script = str(SCRIPT_DIR / "destructive_guard.py") + plugin_root = os.fspath( + Path("/x/plugins/cache/acme/disk-hygiene/0.4.8") + ) + derived = os.fspath(Path("/x/plugins/data/disk-hygiene-acme")) + environment = { + key: value + for key, value in os.environ.items() + if key != "CLAUDE_PLUGIN_DATA" + } + with mock.patch.dict("os.environ", environment, clear=True): + with mock.patch.object( + guard.sys, "argv", [script, "--plugin-root", plugin_root] + ): + self.assertEqual(derived, guard.resolve_authorized_data_root()) + # A direct --authorized-data-root outranks plugin-root derivation. + with mock.patch.object( + guard.sys, + "argv", + [script, "--authorized-data-root", "/direct", "--plugin-root", plugin_root], + ): + self.assertEqual("/direct", guard.resolve_authorized_data_root()) + # An unresolvable plugin-root layout yields no authority, then env. + with mock.patch.object( + guard.sys, "argv", [script, "--plugin-root", "/not/a/plugin/layout"] + ): + self.assertIsNone(guard.resolve_authorized_data_root()) + with mock.patch.dict( + "os.environ", {"CLAUDE_PLUGIN_DATA": "/from-env"}, clear=False + ): + with mock.patch.object( + guard.sys, "argv", [script, "--plugin-root", "/not/a/plugin/layout"] + ): + self.assertEqual("/from-env", guard.resolve_authorized_data_root()) + + def test_plugin_data_root_from_root_follows_documented_layout(self) -> None: + # Real marketplace install: the install root is the VERSION leaf under + # /cache///; data is /data/ + # with the sanitized "@". The version is ignored. + self.assertEqual( + os.fspath(Path("/x/plugins/data/disk-hygiene-melodic-software")), + guard._plugin_data_root_from_root( + os.fspath( + Path("/x/plugins/cache/melodic-software/disk-hygiene/0.4.8") + ) + ), + ) + # A directly-linked local install omits the leaf; the name is + # then the root itself. + self.assertEqual( + os.fspath(Path("/x/plugins/data/disk-hygiene-melodic-software")), + guard._plugin_data_root_from_root( + os.fspath(Path("/x/plugins/cache/melodic-software/disk-hygiene")) + ), + ) + # The "@" separator and every other disallowed character (".") collapse + # to "-", per the documented id-sanitization rule. + self.assertEqual( + os.fspath(Path("/x/plugins/data/my-plugin-my-market")), + guard._plugin_data_root_from_root( + os.fspath(Path("/x/plugins/cache/my.market/my.plugin/1.2.3")) + ), + ) + # No /cache marker, "cache" not under a "plugins" parent, or no + # name segment after the marketplace: fail closed. + for stray in ( + "/somewhere/else/plugin", + "/a/b", + "/x/store/cache/m/n", + "/x/plugins/cache/only-marketplace", + ): + self.assertIsNone( + guard._plugin_data_root_from_root(os.fspath(Path(stray))), stray + ) + def test_argv_flag_value_parses_both_arg_spellings(self) -> None: self.assertEqual( "false", @@ -1807,65 +1938,65 @@ def drive(argv_tail: list[str], env: dict[str, str]) -> bool: # No channel supplies a value: fail safe to enabled (guard active). self.assertTrue(drive([], {})) - def test_skill_hook_passes_disk_hygiene_enabled_flag_matching_constant( - self, - ) -> None: - """Lock the config<->code seam the kill switch depends on. - - The frontmatter hook must pass the exact flag literal the guard parses, - immediately followed by the ${user_config.disk_hygiene_enabled} token. If - either side is renamed without the other, the guard silently loses the - configured value and falls back to the (absent) env var — defaulting to - enabled and defeating the kill switch, the regression this test guards. - """ - skill = SCRIPT_DIR.parent / "SKILL.md" - text = skill.read_text(encoding="utf-8") + @staticmethod + def _skill_hook_command_and_args() -> tuple[str, list[str]]: + """Return the frontmatter guard hook's `command` and parsed `args`.""" + text = (SCRIPT_DIR.parent / "SKILL.md").read_text(encoding="utf-8") args_line = next( - ( - line - for line in text.splitlines() - if "destructive_guard.py" in line and "args:" in line - ), - None, + line + for line in text.splitlines() + if "destructive_guard.py" in line and "args:" in line ) - self.assertIsNotNone(args_line, "frontmatter hook args line not found") - assert args_line is not None - array_text = args_line[args_line.index("[") : args_line.rindex("]") + 1] - args = json.loads(array_text) - self.assertIn(guard._DISK_HYGIENE_ENABLED_FLAG, args) - flag_index = args.index(guard._DISK_HYGIENE_ENABLED_FLAG) - self.assertEqual( - guard._DISK_HYGIENE_ENABLED_PLACEHOLDER, args[flag_index + 1] + args = json.loads(args_line[args_line.index("[") : args_line.rindex("]") + 1]) + command_line = next( + line + for line in text.splitlines() + if line.strip().startswith("command:") ) + command = command_line.split(":", 1)[1].strip().strip('"') + return command, args - def test_skill_hook_passes_authorized_data_root_flag_matching_constant( + def test_skill_hook_args_launch_with_only_skill_available_substitutions( self, ) -> None: - """Lock the config<->code seam the fix depends on. + """Guard the guard's launch: the hook must reference only skill-available tokens. + + Claude Code refuses to launch a skill-frontmatter hook whose command or + args reference any substitution token it does not provide to skill hooks, + and treats that refusal as a non-blocking error — so the destructive-action + guard silently never runs. Empirically the only token available to a skill + hook is `${CLAUDE_PLUGIN_ROOT}`: `${CLAUDE_PLUGIN_DATA}` is plugin-only and + `${user_config.*}` is not substituted for skill hooks, and either one + reintroduces the fail-open. This asserts the declared tokens are within that + allowlist; that the hook then launches is only fully verifiable in a live + Claude Code session with the plugin installed. + """ + command, args = self._skill_hook_command_and_args() + tokens = set(re.findall(r"\$\{[^}]+\}", command)) + for value in args: + tokens.update(re.findall(r"\$\{[^}]+\}", value)) + allowed = {"${CLAUDE_PLUGIN_ROOT}"} + self.assertLessEqual( + tokens, + allowed, + "skill-hook command/args reference tokens Claude Code does not " + f"substitute for skill hooks: {sorted(tokens - allowed)}", + ) + + def test_skill_hook_passes_plugin_root_flag_matching_constant(self) -> None: + """Lock the config<->code seam the data-root derivation depends on. The frontmatter hook must pass the exact flag literal the guard parses, - immediately followed by the ${CLAUDE_PLUGIN_DATA} placeholder. If either - side is renamed without the other, the guard silently loses its authority - and the engine lane fails closed — the regression this test guards. + immediately followed by the ${CLAUDE_PLUGIN_ROOT} token from which the + guard derives the authorized data root. If either side is renamed without + the other, the guard loses its authority and the engine lane fails closed — + the regression this test guards. """ - skill = SCRIPT_DIR.parent / "SKILL.md" - text = skill.read_text(encoding="utf-8") - args_line = next( - ( - line - for line in text.splitlines() - if "destructive_guard.py" in line and "args:" in line - ), - None, - ) - self.assertIsNotNone(args_line, "frontmatter hook args line not found") - assert args_line is not None - array_text = args_line[args_line.index("[") : args_line.rindex("]") + 1] - args = json.loads(array_text) + _command, args = self._skill_hook_command_and_args() self.assertTrue(args[0].endswith("destructive_guard.py"), args) - self.assertIn(guard._AUTHORIZED_DATA_ROOT_FLAG, args) - flag_index = args.index(guard._AUTHORIZED_DATA_ROOT_FLAG) - self.assertEqual("${CLAUDE_PLUGIN_DATA}", args[flag_index + 1]) + self.assertIn(guard._PLUGIN_ROOT_FLAG, args) + flag_index = args.index(guard._PLUGIN_ROOT_FLAG) + self.assertEqual(guard._PLUGIN_ROOT_PLACEHOLDER, args[flag_index + 1]) def test_skill_hook_interpreter_is_python3_and_resolves(self) -> None: """Lock the guard's launch interpreter and prove it resolves. @@ -2043,9 +2174,10 @@ def test_powershell_deletion_spellings_denied_in_audit_only_mode(self) -> None: ) def test_kill_switch_blocks_every_lane_via_argv_without_env(self) -> None: - """Acceptance (B2+D3 together): a configured ``false`` reaching the guard - only through the hook argv — the env var UNSET, the exact env-injection - failure — must block deletions on both the PowerShell and Bash lanes.""" + """A configured ``false`` reaching the guard only through the + ``--disk-hygiene-enabled`` argv channel — the env var UNSET — must block + deletions on both the PowerShell and Bash lanes. Proves the guard's argv + kill-switch logic for a host that can deliver the value there.""" script = SCRIPT_DIR / "hygiene.py" powershell = self.run_guard_enabled_argv( "Remove-Item -Recurse -Force C:/tmp/example", "PowerShell", "false" diff --git a/plugins/disk-hygiene/skills/setup/scripts/kill_switch_probe.py b/plugins/disk-hygiene/skills/setup/scripts/kill_switch_probe.py index 41ad4f20b..8c808694c 100755 --- a/plugins/disk-hygiene/skills/setup/scripts/kill_switch_probe.py +++ b/plugins/disk-hygiene/skills/setup/scripts/kill_switch_probe.py @@ -10,8 +10,12 @@ definitive read is impossible. Report-only: exit code is always 0 and the single-line JSON on stdout is the -whole contract. Enforcement stays with ``destructive_guard.py``, which receives -the runtime-substituted ``--disk-hygiene-enabled`` hook argument. +whole contract. This report is how the ``clean`` skill self-enforces audit-only +mode: a skill-frontmatter hook reaches ``destructive_guard.py`` with neither the +``${user_config.*}`` substitution nor the ``CLAUDE_PLUGIN_OPTION_*`` environment +variable, so the guard cannot read the toggle or deny on it — it still gates +every mutation behind a human prompt, but honoring a configured ``false`` is the +model's responsibility, driven by this probe. Scope: managed settings and a ``--settings`` flag can also carry ``pluginConfigs`` and are not visible here; the ``detail`` sentence states the