docs(disk-hygiene): disclose the PowerShell lane's uncovered mutation spellings - #1305
Conversation
… spellings The safety model and the clean skill described the PowerShell belt as gating "known deletion spellings" without stating what it does not see. Move, rename, overwriting-writer, and volume-format spellings reach the tool with no guard verdict at all — audit-only mode included — so the documented protection read broader than the enumerated regex delivers. Name the uncovered families where the security model is stated, with the per-path human approval and the consumer permission policy identified as the only backstops there. Guard behavior is unchanged; closing the gap is #387. Closes #386 Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 37s —— View job PR Review
This is a clean, well-scoped docs-only PR. The content is accurate and the disclosure is appropriately placed. Two minor notes: Nit:
|
|
Claude finished @kyle-sexton's task in 49s —— View job Security ReviewTasks
ScopeFour files changed:
All changes are documentation only. No Python guard logic, hook configuration, permission grants, workflow files, or dependency pins are touched. FindingsNo security issues found. The substantive change is the new paragraph in Nothing in the diff:
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78a95d3f76
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ed spellings Codex review on #1305: the fail-closed paragraph promised "a human prompt before every mutation", which the newly disclosed PowerShell coverage hole contradicts — an unflagged Move-Item or Set-Content defers with no verdict whether the toggle is readable or not. Bound the guarantee to what the guard actually sees, and separate the TODO annotation from the security-model prose so it does not read as a normative claim. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 26s —— View job PR Review
The second commit ( Codex P2 — "Reconcile the all-mutations guarantee": addressedThe disputed sentence at
The qualification "it sees" plus the parenthetical breakdown eliminates the contradiction: the guarantee is now scoped to what the guard actually observes, not to all possible mutations. The Codex finding is resolved without weakening the fail-closed claim for the surfaces the guard does cover. Prior Claude review nit —
|
|
Claude finished @kyle-sexton's task in 1m 2s —— View job Security ReviewTasks
ScopeFour files changed:
All changes are documentation only. No guard logic, hook configuration, permission grants, workflow files, or dependency pins are touched. FindingsNo security issues found. The substantive changes are:
Nothing in the diff:
The gap disclosed (unflagged PowerShell spellings) pre-dates this PR. This PR makes the documentation more accurate about that pre-existing gap, which is a net improvement to security posture. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dbfa4e2f2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Same over-promise Codex caught in the fail-closed paragraph, in the audit-only direction: "denies every mutation lane" reads as total coverage, but the belt only ever sees the flagged deletion spellings. Name the two lanes as the scope rather than implying all of them. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 49s —— View job PR Review
What
|
|
Claude finished @kyle-sexton's task in 1m 42s —— View job Security ReviewTasks
ScopeFour files changed across three commits (
All changes are documentation only. No guard Python logic, hook configuration, permission grants, workflow files, dependency pins, or secrets are touched. FindingsSUGGESTION · PLAUSIBLE — Backstop attribution overstates containment for volume operations
The sentence attributes "per-path human approval" as a backstop for every listed spelling uniformly, but This is a documentation accuracy issue, not a code regression; the underlying gap is tracked in #387. But a user reading this disclosure to decide whether to enable the lane could infer stronger backstop coverage for This is the live Codex P2 finding raised against Everything else is clean:
|
…or these spellings Codex review on #1305: the manual handoff approves the paths selected for removal, so it cannot cover what an unflagged mutation collaterally destroys — a Move-Item -Force destination, a truncated Out-File target, a whole volume. Listing it as a backstop overstated the residual protection in the same way the paragraph set out to stop. The consumer's permission policy is the only one left. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 12s —— View job PR Review
Codex P2 (second round) — "Do not treat path approval as containment for these mutations": resolved
The previous wording listed the per-path approval as a co-backstop alongside the permission policy; this commit correctly strips it out and instead explains why it doesn't apply — the handoff's approval is scoped to removal paths, not to collateral targets. The concrete examples ( All findings across this review cycle are now resolved:
No outstanding issues. Ready to merge. |
|
Claude finished @kyle-sexton's task in 59s —— View job Security ReviewTasks
ScopeFour files changed across four commits (
All changes are documentation only. No guard Python logic, hook configuration, permission grants, workflow files, dependency pins, or secrets are touched. FindingsNo security issues found. The live Codex P2 finding that survived the The paragraph at
This is precise: it names the sole backstop (permission policy), explicitly excludes the guard, and explains why per-path approval doesn't apply to these spellings rather than implying it does. The volume-operation case ( All other findings from the review cycle remain closed:
Nothing in the diff:
All prior security findings resolved. No new issues introduced. |
|
Closing out the one item from the security review on
Verified against the guard source rather than the review prose: Both review threads on this PR are addressed on HEAD This was generated by AI on operator direction. 🤖 |
… setup check (#1124) ## Summary Closes a fail-open vector in the `disk-hygiene` destructive-action guard on Windows. The `clean` skill registers its PreToolUse guard as the literal hook command `python3` (`skills/clean/SKILL.md`). On stock Windows, `python3` resolves to `%LOCALAPPDATA%\Microsoft\WindowsApps\python3.exe` — a zero-length App Execution Alias reparse stub that opens the Microsoft Store (or exits) instead of running an interpreter. The guard process never starts, so it emits neither of the two signals that let a PreToolUse hook block a tool call — exit code 2 or a `deny` decision — and Claude Code lets the destructive `Bash`/`PowerShell` command proceed **ungated**. This is the same fail-open shape as the 0.6.3 fix (a failed hook launch treated as non-blocking), reached through a new vector: the guard's launch *name* resolving to the Store stub rather than a real interpreter. This is a `DEFER-FORBIDDEN` fail-open gap in a safety guard — detection, tests, and docs are all part of the fix, not follow-ups. ## Fix - **New inspect-only probe** `skills/setup/scripts/python3_alias_probe.py`: classifies what the name `python3` resolves to (via `shutil.which`, `--path` override for tests) **without executing it** (running the stub pops the Store / hangs). Verdicts: `store-alias-stub` (zero-length file under a `WindowsApps` path component), `ok` (real interpreter — including the Store's *genuine* Python under a versioned `WindowsApps\PythonSoftwareFoundation...\` subdir, which has non-zero size), `not-found`, `indeterminate` (identity unreadable). On Windows it also reports the `IO_REPARSE_TAG_APPEXECLINK` reparse attribute as corroborating evidence, but the cross-platform verdict uses the portable zero-length + `WindowsApps`-component signal. - **`setup check` (SKILL.md step 1)** now runs the probe after locating the working interpreter and **fails closed on every verdict except `ok`**: `store-alias-stub` and `indeterminate` both FAIL with remediation (disable the `python3` App execution alias, or install real Python ahead of WindowsApps on `PATH`); `not-found` folds into the floor's absent-interpreter FAIL. A bare `command -v python3` success is explicitly called out as insufficient — it matches the stub too. - **README** requirements section documents the vector and *why* it fails open. - **Version bump → `0.9.2`** in `plugin.json` + matching `CHANGELOG.md` entry. (Re-versioned three times as `main` moved under the PR — 0.6.5 → 0.6.6 after PR #1118, 0.9.0 → 0.9.1 after the kill-switch release landed as 0.9.0, then 0.9.1 → 0.9.2 after #1305 took 0.9.1; each time by merging `origin/main` in and re-slotting the CHANGELOG entry on top. Merge, not rebase, per branch policy. The merged tree confirms the vector still holds post-0.9.0: both guard surfaces — the plugin-level engine gate in `hooks/hooks.json` and the skill-scoped belt — still launch via the literal command `python3`.) ## Verification - **New unit tests** `test_python3_alias_probe.py` (10 cases, run via `python3_alias_probe.test.sh`) — all pass. They fabricate a zero-length `.../WindowsApps/python3.exe` in a tempdir so detection is exercised **cross-platform** without a real Windows box: stub detected; case-insensitive `WindowsApps` match; real interpreter under a versioned WindowsApps subdir → `ok` (no false positive); zero-length *outside* WindowsApps → `ok`; `not-found`; the `indeterminate` stat-failure path (fail-closed); and an assertion that the probe **never** invokes `subprocess` on the candidate. - Full plugin suites green post-merge: setup probe 10/10, `kill_switch_probe` unaffected, clean engine `test_hygiene.py` 112 passed (4 skipped). - `markdownlint-cli2`, `shellcheck`, and `typos` clean on all changed files; new scripts tracked `100755` (exec-bit lane parity with the existing `kill_switch_probe.*`). - **Empirical signal confirmation (this Windows 11 box):** inspected `%LOCALAPPDATA%\Microsoft\WindowsApps` — all 35 App-Execution-Alias `.exe` stubs are `Length 0` with the ReparsePoint attribute set, confirming the zero-length detection signal against ground truth (not asserted from memory). Post-0.9.0-merge re-verification on the same box: the probe run with `--path` pointed at a live AppExecLink stub returns `store-alias-stub` (with `reparse_point: true` corroboration), the real interpreter resolution returns `ok`, and an unreadable WindowsApps path returns `indeterminate` (fail-closed). - **Hook-failure-semantics claim verified live against current docs** (fetched this session): [code.claude.com/docs/en/hooks](https://code.claude.com/docs/en/hooks) — "**Exit 2** … `PreToolUse` blocks the tool call"; "**Any other exit code** is a non-blocking error … Execution continues"; PreToolUse can alternatively block via exit-0 JSON `permissionDecision: deny`. A guard that never launches emits neither exit 2 nor a `deny`, so the non-blocking path is taken — the doc-grounded basis for the fail-open, stated as that logical consequence rather than as a documented launch-failure rule. ## Related - Source: handoff-inbox item `20260723-021058-disk-hygiene-0-6-4-consumer-audit`, finding **F3**. - **Root cause note (out of scope here):** the terminal root cause is the guard hook using the bare name `python3` (`skills/clean/SKILL.md`), which changing the hook's launch command would address — that is guard-registration territory (operator-gated, #1107), so this PR is the assigned *mitigation* (detect + document + fail closed in `setup check`), not a change to the guard's registration. - Deferred low-risk items surfaced by independent review, none blocking: `python3_alias_probe.test.sh`'s `command -v python3` fallback could itself resolve to a Store stub on a stub-only machine (kept at parity with the sibling `kill_switch_probe.test.sh` wrapper rather than diverging); a symlink whose own path is outside WindowsApps but whose target is inside would evade the path-component check (AppExecLink aliases are reparse points, not symlinks, so not the real-world shape). Closes #1110 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
1 participant
Closes #386
Summary
#386 grouped three disk-hygiene doc corrections (D1, D2, D4 — the body has no D3). Re-verified each
against current
mainbefore implementing: two of the three were already fixed by later work, sothis PR lands only the one that is still live.
D1 — Windows mislabeled "full": already fixed.
skills/setup/SKILL.mdnow splits the lanesexplicitly ("Windows (full audit … engine execution unsupported"), matching
execution_blockersinhygiene.py.README.mdsays the same. No change needed.D2 — false "hook process receives
CLAUDE_PLUGIN_DATA" premise: already fixed. Both cited sitesnow describe the real mechanism —
clean/SKILL.md("validates--data-rootagainst the plugin datadirectory it derives from
${CLAUDE_PLUGIN_ROOT}") andreference/safety-model.md(derivation +--authorized-data-root+ env fallback, with the fail-closed consequence stated). No change needed.The 0.3.0 CHANGELOG entry still carries the original wording and is deliberately left alone: it is
a released historical record, the issue scoped it out ("Beyond the already-known CHANGELOG 0.3.0
line"), and later entries (0.4.x, 0.9.0) already record the correction.
D4 — PowerShell-lane gaps: half fixed, half live. Sub-gap (a), the kill switch not closing the
lane, was fixed by #382 — the guard now denies flagged spellings outright in audit-only mode, and the
docs say so. Sub-gap (b) is still real: the lane enumerates spellings, and
Move-Item/mv,Rename-Item, overwriting writers (Set-Content,Out-File,>,New-Item -Force), andFormat-Volume/Clear-Diskare absent from_POWERSHELL_MUTATION_WORDS, so they reach the tool withno guard verdict at all. The docs described the lane's coverage without naming that hole, which
reads as broader protection than the regex delivers.
This PR discloses that hole once, where the security model is stated
(
reference/safety-model.md), names the consumer permission policy as its only backstop, and carriesTODO(#387).clean/SKILL.md's existing "raised bar, not a fail-closed lane" gotcha gains one clausepointing at that paragraph rather than restating it.
Review-driven follow-ups (Codex, both accepted) tightened two adjacent over-promises the disclosure
exposed: the fail-closed paragraph's "a human prompt before every mutation" is now bounded to what the
guard sees, the audit-only "denies every mutation lane" to the two lanes it gates, and the manual
handoff's per-path approval is no longer offered as a backstop here — it approves the paths selected
for removal, so it cannot cover what an unflagged spelling collaterally destroys (a
Move-Item -Forcedestination, a truncated
Out-Filetarget, a whole volume).Docs only — no guard behavior changes. Extending the flagged set is #387's job, not this issue's.
Test plan
markdownlint-cli2overplugins/disk-hygiene/**/*.md— 0 errors.scripts/check-changed-skills.sh origin/main—cleanPASS, 0 errors (344/500 lines, all 6 base-reftrigger phrases preserved).
scripts/check-skill-portability.sh origin/main— no unexcused coupling tokens.scripts/validate-plugins.sh— all manifests + catalog valid.scripts/check-changelog-parity.sh --check-bump origin/main— version bump 0.9.0 → 0.9.1 has itsmatching CHANGELOG entry.
Fresh-docs mandate discharged for the harness-behavior claims sitting at the edit site:
plugins-reference (environment variables +
persistent data directory) and hooks (skill-frontmatter
hooks). Result: the reference documents all three path variables as exported to hook processes and
substituted in "hook and monitor commands", but neither page carves out skill-frontmatter hooks,
so the existing safety-model text — which already attributes the non-export to an observed Claude Code
build rather than to the docs — is still accurate and was left unchanged.
Related
CLAUDE_PLUGIN_DATAenv premise (closed; D2's root cause).🤖 Generated with Claude Code