Skip to content

fix(disk-hygiene): name the failing token and flag-order rule in the engine-gate denial - #5582

Merged
kyle-sexton merged 12 commits into
mainfrom
fix/5519-engine-gate-denial-text
Sep 30, 2026
Merged

kyle-sexton merged 12 commits into
mainfrom
fix/5519-engine-gate-denial-text

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Refs #5519

Summary

The Bash engine-gate denial in destructive_guard.py gave no hint which token failed the exact-engine grammar, what the flag-order rule is, or which mention forms are gated. The denial now says all three. No behavior change: the guard matches and denies exactly the same commands.

This PR uses Refs, not Closes; whether it closes #5519 is left to the owner.

Fix

  • _engine_mismatch_reason names what the classifier refuses (parse, word count, interpreter, engine script, subcommand, --data-root, per-subcommand grammar via engine_grammar.explain_mismatch). It runs only on the deny path.
  • An engine operand of a command that is not the hook's Python (grep foo "<engine path>", cat "<engine path>") is named first, as an absolute engine path or as a relative word that resolves to the engine from the current directory. Before, the reason blamed the command word (grep) as "not this hook's Python".
  • A word the gate reads as an engine call (a quoted payload holding an interpreter and the engine filename, such as gh issue list --search "python3 <engine>") is named as the gated word. Before, the reason named gh and asked for the hook's Python. The gate and the reason share one helper, _reads_as_engine_payload, so they cannot disagree on which word gated.
  • An unparsable command names the first operator class present (pipe, redirect, ;, &, substitution, glob, newline, !/#, backslash, quote) instead of listing all of them. A test pins the label table to the characters the literal parser rejects.
  • _engine_flag_order_rule states the flag-order rule from the declared subcommand specs, and _ENGINE_GATE_SCOPE states the gated mention forms and the read-only forms in the owner's words, with the condition the owner chose (option 2, PR comment 2026-09-30T19:04Z): a relative path or bare name that resolves to the installed engine from the current directory is still gated.
  • Relaxing the guard (option a) is deferred by the owner's decision to a separate, security-reviewed change.
  • disk-hygiene 0.41.1 with a CHANGELOG entry above 0.41.0. No doc quotes the old denial text.

Owner decision applied

The Codex P2 thread found that the advertised read-only forms are denied when the word resolves to the installed engine (the literal branch of _engine_gate_relevant gates on file identity). The owner took option 2: keep the forms and add the condition. test_engine_gate_gates_a_relative_word_that_resolves_to_the_engine pins both the gated shapes and that their denial states the condition.

_engine_gate_relevant result per working directory:

Working directory Command Gated
plugin root or repo root grep foo <relative path to the engine> yes
plugin root or repo root git grep foo -- <relative path to the engine> yes
repo root rg foo <bare engine name> no
the engine's directory rg foo <bare engine name> yes
the engine's directory git grep foo -- <bare engine name> yes
any directory tried git show <rev>:<path to the engine> no
an unrelated directory grep foo <relative path to the engine> no

Verification

  • test_hygiene (run as CI does, from the scripts directory): 674 tests OK (1 skipped) on the merged head, including tests for the denial text, agreement between the explainer and the classifier over every declared subcommand (handoff-apply included), the advertised read-only forms and their resolving-word condition, the engine operand reason, the payload-word reason, and the operator reason. The payload-word test also asserts _engine_gate_relevant still gates those commands and still defers a plain mention.
  • The rest of the disk-hygiene Python suites (scripts, lib, setup): OK.
  • scripts/run-ruff.sh check on the changed files: all checks passed.
  • scripts/check-changelog-parity.sh --check, --check-order, and scripts/validate-plugins.sh: pass.

Related

🤖 Generated with Claude Code

kyle-sexton and others added 4 commits September 30, 2026 01:32
… gated shapes

The engine-gate denial now says which word of the denied command broke which
rule, states the required-flag order, and states which mention shapes are
gated and which read-only forms work. Text only: the classifier, the grammar
walk and the allow path are unchanged.

Refs #5519

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…t and advertised read-only forms

Refs #5519

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…engine-gate denial

Bump disk-hygiene to 0.29.3 and record the denial-text change.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…n the gate denial

The engine-gate denial named the wrong token for a mention with an
engine-path operand (it blamed the command word as "not this hook's
Python") and listed every operator class for an unparsable command.

- An engine operand of a command that is not the hook's Python is now
  named first, saying whether it is an absolute path or a relative word
  that resolves to the engine from the current directory.
- The unparsable reason names the first offending character's class, and
  a test pins the label table to the parser's rejected characters.
- The read-only-forms test restores the working directory before the
  temporary directory is removed, so it also runs on Windows, and a new
  test pins that a relative word resolving to the engine is gated while
  a rev:path form is not.

The denial text is unchanged where the owner wrote it; the guard matches
and denies exactly as before.

Refs #5519

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

PR body contract — issue linkage

This PR body conforms to the issue-linkage contract. Nothing to do.

kyle-sexton and others added 3 commits September 30, 2026 12:51
…denial-text

Renumber the denial-text entry to disk-hygiene 0.34.1 above main's 0.34.0.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…denial-text

Renumber the denial-text entry to disk-hygiene 0.34.2 above main's 0.34.1.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…denial-text

# Conflicts:
#	plugins/disk-hygiene/.claude-plugin/plugin.json
#	plugins/disk-hygiene/CHANGELOG.md
@kyle-sexton
kyle-sexton marked this pull request as ready for review September 30, 2026 17:43
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-30T17:50:06.404713Z 5dc6aa5 Draft marked ready
ℹ️ 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" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 4m 43s —— View job


Claude finished the code review

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow its instructions
  • Post inline comments for findings anchored to changed lines
  • Post summary comment with overview + any cross-file findings

Summary

Reviewed the diff (git diff origin/main...HEAD) against the review:code-review lane criteria: correctness and maintainability on the changed files, scoped to plugins/disk-hygiene/lib/engine_grammar.py, plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py, test_hygiene.py, CHANGELOG.md, and plugin.json.

No blocking correctness findings. I traced _engine_mismatch_reason and explain_mismatch stage-by-stage against the functions they must agree with (classify_exact_engine_command / match_invocation) and confirmed:

  • The required-flag, optional-tail, requires, and one_of checks in explain_mismatch (plugins/disk-hygiene/lib/engine_grammar.py) walk the grammar in the same short-circuit order as match_invocation, so it returns None exactly when the classifier accepts.
  • _OPERATOR_LABELS (destructive_guard.py:2115-2128) covers exactly the 21 characters in _SHELL_EXPANSION_OR_OPERATOR_CHARS, matching the new pinning test.
  • The "engine operand named first" priority in _engine_mismatch_reason (destructive_guard.py:2148-2195) uses the same _script_path_key string-identity comparison as classify_exact_engine_command's own tokens[1] check, so it never disagrees with what the classifier actually denies.
  • String concatenation in the rebuilt _bash_denial_guidance body (destructive_guard.py:2251-2269) produces correctly-spaced text in both the command is None and populated-reason cases.
  • plugin.json/CHANGELOG.md version bump (0.34.4 → 0.34.5) is consistent and dated correctly.

One theoretical, currently-unreachable note (not filed as a finding, doesn't meet the high-signal bar since I could not construct a real caller that reaches it): _engine_mismatch_reason computes _is_current_python(tokens[0]) before checking len(tokens) < 3, whereas classify_exact_engine_command short-circuits length before indexing. tokens[0] would raise IndexError if tokens == [], but that only happens when _literal_shell_words parses a whitespace-only command, and _engine_gate_relevant already returns False (deferring, never denying) for that input on every path that reaches _bash_denial_guidance. Worth keeping in mind if _engine_mismatch_reason ever gets a caller that doesn't go through _engine_gate_relevant first.

The PR's own "Open question for the owner" (the gap between the advertised read-only-forms sentence and what _engine_gate_relevant actually gates for a relative/bare-name mention) is accurately described and already pinned by tests (test_engine_gate_defers_the_read_only_forms_the_denial_advertises, test_engine_gate_gates_a_relative_word_that_resolves_to_the_engine); that's a product-wording decision for the owner, not a code defect.

Verification note: this lane's sandbox declined approval for scripts/run-ruff.sh and python3 -m pytest/python3 -c, so I could not independently re-run the test suite or the ruff check myself — the PR body's "601 tests OK" and ruff-clean claims are author-reported and unverified by me. My review is based on static reading of the diff plus the pre-existing match_invocation/classify_exact_engine_command/_engine_gate_relevant logic it must agree with.

Branch: fix/5519-engine-gate-denial-text

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 44s —— View job


Security review — PR #5582

Scope reviewed: all 5 changed files, with focus on the two files with security-relevant surface: destructive_guard.py and engine_grammar.py. plugin.json and CHANGELOG.md are metadata-only; test_hygiene.py is test-only.

No security issues found.

Verified specifically:

  • Decision/message separation holds. In _decide, command_kind = classify_exact_engine_command(command, authority) is computed independently of the new _bash_denial_guidance(authority, command=command) call, which only fires after the deny/ask/allow verdict is already settled. _engine_mismatch_reason and engine_grammar.explain_mismatch are reason-text generators on the already-denied path; neither can change an allow/deny/ask outcome.
  • Fail-closed boundary still covers the new code. The new text-building functions run inside _decide, which main() wraps in a broad except BaseException that denies (exit 2) on any exception (destructive_guard.py lines ~2926-3006). A bug or slow filesystem resolve in _resolves_to_engine/_script_path_key on the deny path can't fail open.
  • No new info disclosure. The denial text echoes back only tokens the caller's own command already contained (via engine_grammar.clip_token, which applies repr() with a length cap — neutralizing control-character/escape-sequence injection into the rendered message) plus the interpreter/engine paths that were already disclosed by the pre-existing _bash_allowlist_disclosure grammar text.
  • explain_mismatch's external-check plumbing is unchanged in risk: it reuses the same _is_authorized_data_root equality check already used by match_invocation/classify_exact_engine_command, with no new filesystem side effects.
  • No instruction-surface deletion. The only removed lines are the old _bash_denial_guidance signature/docstring/text that the diff directly replaces with an expanded version carrying the same grammar disclosure plus additional detail — nothing is narrowed or dropped.

The PR's own "Open question for the owner" (the _ENGINE_GATE_SCOPE wording not holding for a relative/bare engine name run from the engine's own directory) is a documentation-accuracy gap, not a security bypass: in every case in the author's table, the mismatch is the guard being more restrictive than the advertised text says, never less.
· branch fix/5519-engine-gate-denial-text

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5dc6aa5a08

ℹ️ 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".

Comment thread plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py Outdated
@kyle-sexton

Copy link
Copy Markdown
Contributor Author

Resolution of the Codex P2 thread (discussion_r4147675721), within the #5519 decision (change the denial text only, no guard relaxation): take option 2. The denial text must not advertise a form the guard still denies, so _ENGINE_GATE_SCOPE keeps the relative-path and bare-name read-only forms only with the condition that a path resolving to the installed engine is still gated. This is an accuracy correction to the message, not a change to what the guard allows. Options 1 (keep the inaccurate sentence) and 3 (advertise git show <rev>:<path>) are not taken. Next: the repair agent edits the text and its test, replies on the thread, and resolves it.

…denial-text

Renumber disk-hygiene 0.34.5 -> 0.36.1 above main's 0.36.0 and pass the
command into main's single not-exact-engine-command deny.

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

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 44s —— View job


Claude finished the code review

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow its instructions
  • Post inline comments for findings anchored to changed lines
  • Post summary comment with overview + any cross-file findings

Summary

Reviewed the diff between b02fccf (main at the last merge) and HEAD (8fd879c), which matches the PR's stated 459/−6 across the 5 changed files, scoped to correctness/maintainability per this lane (security is already covered by the separate security-review lane on this PR).

The core logic (_engine_mismatch_reason / engine_grammar.explain_mismatch walking the same stage order as classify_exact_engine_command / match_invocation, _OPERATOR_LABELS pinned against _SHELL_EXPANSION_OR_OPERATOR_CHARS, the engine-operand-named-first priority using the same _script_path_key identity check as the classifier) was already reviewed in depth on the prior head (5dc6aa5) by this lane and by Codex, and I re-traced the same stage-by-stage agreement — no new issues there. The version bump (0.36.0 → 0.36.1 after the main merge) and CHANGELOG entry are correctly sequenced.

One finding posted inline (destructive_guard.py:2265): the owner's 2026-09-30T19:04:22Z comment resolved the Codex P2 thread by picking option 2 — _ENGINE_GATE_SCOPE needs the "resolves to the installed engine" condition added, since the current sentence advertises relative-path/bare-name read-only forms as working when the PR's own new test (test_engine_gate_gates_a_relative_word_that_resolves_to_the_engine) proves they're denied from the plugin/repo root or the engine's own directory. Diffing this head against the previously-reviewed commit shows that text is still byte-for-byte unchanged, so the decided fix hasn't landed on this SHA yet.

Everything else — the flag-order rule derivation from declared subcommand specs, clip_token's length cap, the unparsable-command operator-priority logic, and the new test coverage in test_hygiene.py — checked out.

Verification note: I did not independently run hygiene.test.sh, run-python-hook.test.sh, or scripts/run-ruff.sh in this session; the PR body's test/lint results are author-reported and unverified by me here.
· Branch: fix/5519-engine-gate-denial-text

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 0s —— View job


Security review — PR #5582

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:security-review and follow its instructions exactly
  • Post findings

Result: no new security-relevant change since the prior review

I scoped the diff to origin/main...HEAD restricted to this PR's 5 changed files (git diff origin/main HEAD -- plugins/disk-hygiene/.claude-plugin/plugin.json plugins/disk-hygiene/CHANGELOG.md plugins/disk-hygiene/lib/engine_grammar.py plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py), which matches the PR's reported +459/-6 across 5 files.

The security-relevant code in that scoped diff — _engine_mismatch_reason, _unparsable_reason, _resolves_to_engine, _engine_flag_order_rule, _ENGINE_GATE_SCOPE, and the rebuilt _bash_denial_guidance/_decide call in destructive_guard.py, plus clip_token/required_order/explain_mismatch in engine_grammar.py — is byte-identical to what was already reviewed on commit 5dc6aa5a (the two prior claude security/code reviews and the Codex review already on this thread). I diffed 5dc6aa5a...HEAD directly to confirm: the only PR-scoped changes since that commit are the version bump (0.34.5 → 0.36.1, since this branch re-merged main and picked up an unrelated intervening release) and a CHANGELOG.md entry ordering artifact. The apparent line-number shifts in destructive_guard.py/engine_grammar.py come entirely from unrelated code merged in from main (the handoff-apply subcommand, the marker-free identity-probe dedupe, etc.) landing above this PR's own hunks — none of that merged-in code is part of this PR, and each of those commits already went through its own review before landing on main.

Per this lane's skip gate ("this head already has a successful security review that still applies"), I'm not re-deriving findings that are already on record. Confirming the substance of the prior reviews on this PR:

  • Decision/message separation holds — _bash_denial_guidance/_engine_mismatch_reason run only after _decide has already settled deny/ask/allow; they generate reason text, never a verdict.
  • Fail-closed boundary is unchanged — the new code runs inside _decide, still wrapped by main()'s broad except BaseException → deny.
  • No new info disclosure — tokens are echoed back only via engine_grammar.clip_token (repr() + length cap), and only content the caller's own command already contained.
  • No instruction-surface deletion — nothing in this diff removes or narrows a standing guardrail; it only adds explanatory text on an already-denied path.

The one open item — _ENGINE_GATE_SCOPE's advertised read-only forms not holding when a relative/bare name resolves to the installed engine (Codex P2, destructive_guard.py:2269) — is a message-accuracy gap, not a security bypass (the guard is only ever more restrictive than the advertised text, never less), and is already being tracked and resolved on that review thread per the owner's 2026-09-30 19:04 comment (taking option 2). No action needed from this lane.

No security issues found in this PR.
· Branch: fix/5519-engine-gate-denial-text

Comment thread plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py
Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 31s —— View job


Claude finished the code review

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow its instructions
  • Post inline comments for findings anchored to changed lines
  • Post summary comment with overview + any cross-file findings

Summary

Scoped the diff correctly per this lane's criteria: git diff origin/main...HEAD --stat on HEAD 9a3fdb6 reports the same 5 files as the PR metadata (plugin.json, CHANGELOG.md, engine_grammar.py +105/-0, destructive_guard.py, test_hygiene.py +209 lines net of the diff below).

This push (8fd879c → 9a3fdb6) is a merge: sync origin/main into fix/5519, renumber disk-hygiene 0.40.1 plus the unrelated fix(ai-slop) commit. Diffing 8fd879c against 9a3fdb6 directly shows the only content that changed is: the version bump (0.36.1 → 0.40.1), CHANGELOG entries for origin/main's intervening 0.37.0–0.40.0 releases (inventory subcommand, owner registry, /disk-hygiene:check, the missing-node notice), and the inventory subcommand being folded into _READONLY_ENGINE_SUBCOMMANDS — all already present on origin/main and therefore excluded from the correctly-scoped origin/main...HEAD three-dot diff. Re-diffing this PR's own four files against the current origin/main...HEAD confirms the PR-owned content (_engine_mismatch_reason, _unparsable_reason, _resolves_to_engine, _engine_flag_order_rule, _ENGINE_GATE_SCOPE, the rebuilt _bash_denial_guidance/_decide call, and engine_grammar.explain_mismatch/clip_token/required_order) is byte-for-byte unchanged from 5dc6aa5a/8fd879c, which this lane and the security lane already reviewed in depth on this thread.

Skip gate applies (criterion 4: "this head already has a successful review from this lane that still applies"). No new correctness/maintainability findings — the stage-order agreement between _engine_mismatch_reason/explain_mismatch and classify_exact_engine_command/match_invocation, the _OPERATOR_LABELS/_SHELL_EXPANSION_OR_OPERATOR_CHARS pinning, and the engine-operand-priority logic were already traced and confirmed correct in the prior passes on this PR.

Outstanding item carried forward, not re-filed as a new finding: the owner's 2026-09-30T19:04:22Z decision (resolving Codex thread discussion_r4147675721, taking option 2) to add the "resolves to the installed engine" qualifier to _ENGINE_GATE_SCOPE has still not landed on this head. The text at
destructive_guard.py:2265-2269
is still byte-identical to what the existing open inline thread on this PR already flags, so I did not duplicate that comment. Until it's edited, the denial continues to advertise a relative-path/bare-name read-only form as working in the checkout's ordinary working directories (repo root, plugin root, the engine's own directory), which test_engine_gate_gates_a_relative_word_that_resolves_to_the_engine (already in this PR) itself proves is denied there.

Verification note: static diff review only in this pass — I did not re-run test_hygiene.py, run-ruff.sh, or the changelog/plugin validators in this session; the version bump and CHANGELOG entry are consistent with origin/main's current 0.40.0 state by inspection.
· Branch: fix/5519-engine-gate-denial-text

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 7s —— View job


Security review — PR #5582

Result: skip gate applies — no new security-relevant change since the prior review

I scoped the diff with git diff origin/main...HEAD restricted to this PR's 5 changed files. origin/main resolves to 819c7e1, which is exactly git merge-base origin/main HEAD, so the three-dot diff is the PR's own change set, not the unrelated main history (#5590 inventory subcommand, #5584, #4606) this branch picked up via its sync merges. That scoped diff is 459 insertions / 6 deletions across the same 5 files the PR declares, confirming the scoping is correct.

Diffing that scoped content against the last-reviewed commit on this PR (8fd879c, already reviewed twice on this thread — by claude and by the chatgpt-codex-connector) shows the security-relevant code — _engine_mismatch_reason, _unparsable_reason, _resolves_to_engine, _engine_flag_order_rule, _ENGINE_GATE_SCOPE, and the _bash_denial_guidance/_decide wiring in destructive_guard.py, plus clip_token/required_order/explain_mismatch in engine_grammar.py — is byte-identical to what was already reviewed. The only change between 8fd879c and 9a3fdb6 within this PR's scope is the mechanical version renumber (plugin.json 0.36.1 → 0.40.1, sequenced above main's now-current 0.40.0) and the matching CHANGELOG.md entry reordering — both non-security.

Per this lane's skip gate ("this head already has a successful security review that still applies"), I'm not re-deriving findings already on record. Confirming the substance still holds:

  • Decision/message separation holds — the reason-text generators run only after _decide has already settled deny/ask/allow; they can't change a verdict.
  • Fail-closed boundary is unchanged — this code runs inside _decide, still wrapped by main()'s broad except BaseException → deny.
  • No new info disclosure — echoed tokens go through engine_grammar.clip_token (repr() + length cap), and only echo content the caller's own command already contained.
  • No instruction-surface deletion — nothing in this diff removes or narrows a standing guardrail; it only adds explanatory text on an already-denied path.

The one open item — _ENGINE_GATE_SCOPE's advertised read-only forms not holding when a relative/bare name resolves to the installed engine (Codex P2, destructive_guard.py:2269) — remains a message-accuracy gap, not a security bypass: in every row of the author's table the guard is only ever more restrictive than the advertised text, never less. The owner decided (2026-09-30T19:04:22Z) to take option 2 for the wording, but that edit hasn't landed on this SHA yet; that's tracked on the Codex review thread and in the code-review lane, not this one.

No security issues found in this PR.
· Branch: fix/5519-engine-gate-denial-text

kyle-sexton and others added 2 commits September 30, 2026 18:08
… gated payload word

The engine-gate scope sentence now adds that a relative path or bare name
which resolves to the installed engine from the current directory is still
gated (owner decision on PR 5582, option 2). Text only: the guard allows
and denies exactly as before.

A quoted word the gate reads as an engine call, such as a gh --search
query holding an interpreter and the engine filename, is now named as the
gated word. The denial used to name the command head and ask for the
hook's Python. The gate and the reason share one payload test, so they
cannot disagree on which word gated.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
disk-hygiene on origin/main is still 0.40.0, so this branch stays 0.40.1.

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

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 3m 10s —— View job


Claude finished the code review

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow its instructions
  • Post inline comments for findings anchored to changed lines
  • Post summary comment with overview + any cross-file findings

Summary

This push (9a3fdb6 → ba89e97) adds one substantive commit, 45291e1 ("qualify the engine-gate read-only forms and name a gated payload word"), plus a merge-only sync commit. I scoped the review to git diff 9a3fdb6..ba89e97 -- <this PR's 5 files>, which isolates exactly that commit's content (the merge added nothing beyond it), and confirmed git diff origin/main...HEAD --stat still matches the PR's reported 522/−18 across the same 5 files.

This commit is the owner-decided fix from the open Codex P2 thread and the prior claude review comment on this PR (discussion_r4147675721, resolved 2026-09-30T19:04:22Z, "take option 2"). It:

  • Adds the qualifier to _ENGINE_GATE_SCOPE (destructive_guard.py:2289-2296): "A relative path or bare name that resolves to the installed engine from the current directory is still gated." I checked this sentence against every row of the PR body's _engine_gate_relevant-per-directory table — it correctly predicts each row (e.g. repo root + bare name doesn't resolve there, so it stays ungated and outside the caveat; the engine's own directory + bare name does resolve, so the caveat correctly warns it's still gated). New test test_engine_gate_gates_a_relative_word_that_resolves_to_the_engine pins the sentence into the actual denial reason for exactly the plugin-root/engine-directory cases the table flags.
  • Extracts _reads_as_engine_payload (a quoted compound like gh issue list --search "python3 hygiene.py" that reads as an engine call) as a named, shared helper used by both _engine_gate_relevant (unchanged behavior — I traced the refactor and it's a pure extraction, same short-circuit order, same _carries_marker guard) and the new _engine_mismatch_reason payload branch (destructive_guard.py:2217-2225), so the denial now names the actual gated word instead of misreporting gh as "not this hook's Python." Traced all four new test cases (test_engine_gate_denial_names_the_payload_word_it_gated) by hand against _literal_shell_words tokenization and confirmed each produces the expected reason text.

No correctness or maintainability findings. The fix is precisely scoped to the wording gap that was already flagged twice on this thread, doesn't touch the classifier's actual allow/deny logic (confirmed: _reads_as_engine_payload returns False early for any word _carries_marker already handles, so no double-counting), and is covered by tests that exercise the real hook end-to-end via _gated_reason/run_guard_engine_gate, not just the reason-string helper in isolation.

CHANGELOG entry was amended in place under the existing [0.40.1] heading (no version bump needed, still unreleased) and accurately describes both changes; no doc elsewhere quotes the old _ENGINE_GATE_SCOPE sentence.

Verification note: static diff/code reading only in this pass — I did not re-run test_hygiene.py, scripts/run-ruff.sh, or the changelog/plugin validators in this session; the PR body's test/lint results are author-reported and unverified by me here.

Branch: fix/5519-engine-gate-denial-text

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 22s —— View job


Security review — PR #5582

Result: no security issues found

Scope. git diff origin/main...HEAD --stat (origin/main = bd07e43, the PR's own merge base) reports the same 5 files and the same 522/−18 totals the PR declares, confirming the scoping is correct.

What's new since the last review on this thread. The prior two reviews on this PR (on 9a3fdb6) both flagged the same outstanding item: the owner's 2026-09-30T19:04:22Z decision (Codex thread discussion_r4147675721, option 2) to qualify _ENGINE_GATE_SCOPE had not yet landed. Commit 45291e1 — new since that review — lands it:

  • _ENGINE_GATE_SCOPE now adds the sentence "A relative path or bare name that resolves to the installed engine from the current directory is still gated," closing the gap Codex and the prior reviews on this thread flagged (the denial no longer advertises a retry shape the guard still denies).
  • _is_interpreter is hoisted from a nested closure in _engine_gate_relevant to a module-level function, and the inline "quoted compound payload names the engine + an interpreter/itself" check is extracted into _reads_as_engine_payload, reused by both the gate and the new reason text.
  • _engine_mismatch_reason gains a payload branch that names the word the gate actually matched (e.g. a gh --search "python3 hygiene.py" string) instead of falling through to the generic "not this hook's Python" / word-count messages.

Verified, not just read:

  • Refactor is behavior-preserving. I traced the removed inline block in _engine_gate_relevant (the _ENGINE_MARKER in folded and " " in word / _ENGINE_MARKER in folded and "python" in folded pair) against the new _reads_as_engine_payload: same casefold, same first-token/interpreter check, same final "python" in folded fallback, called only where _carries_marker(word) is already False in both old and new code. The gate's actual allow/deny behavior is unchanged — only the code moved.
  • Reason/gate tokenization still agrees. _engine_mismatch_reason's tokens = _literal_shell_words(command) (default allow_backslash=False) matches the Bash-path words = _literal_shell_words(command, allow_backslash=allow_backslash) in _engine_gate_relevant (also False for Bash), so the new payload lookup scans the same token set the gate itself scanned — it can't name a word the gate didn't actually act on.
  • Decision/message separation still holds. _bash_denial_guidance(authority, command=command) is still the last statement of _decide's final deny branch (destructive_guard.py:3005-3012), called via _settle(..., "deny", ...) after the verdict is already fixed — this commit only changes what the message says, not when or whether it's denied.
  • No new disclosure risk. The payload word is echoed via engine_grammar.clip_token (already-reviewed repr() + length cap), and is only ever a substring of the caller's own command.

This closes out the last open item from the prior two reviews on this PR; no new security-relevant surface was introduced by 45291e1.

Verification note: static diff review only — this lane's sandbox did not permit running scripts/run-ruff.sh or the test suite in this session, so the PR body's test/lint claims are author-reported and unverified by me here.

No security issues found in this PR.
· Branch: fix/5519-engine-gate-denial-text

origin/main released disk-hygiene 0.41.0, so this branch's entry moves
from 0.40.1 to 0.41.1, above it.

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

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 2m 33s —— View job


Claude finished the code review

  • Gather context (PR body, diff, CLAUDE.md/AGENTS.md conventions)
  • Invoke /review:code-review and follow its instructions
  • Post inline comments for findings anchored to changed lines
  • Post summary comment with overview + any cross-file findings

Summary

Scoped the diff correctly per this lane's criteria: gh pr diff 5582 (base origin/main, which resolves to the same content as this branch's merge base) reports the same 5 files and the same 522/−18 totals the PR declares.

This push (ba89e97 → 226d42a) is two commits: 4f5208c (feat(disk-hygiene): class-matched policy rules, atime/ctime age basis, reference in-flight input, #5628) and the 226d42a sync-merge. Both are origin/main content that landed on this branch via the merge, not this PR's own work — confirmed by diffing the properly-scoped origin/main...HEAD diff against what's already been reviewed on this thread:

  • plugins/disk-hygiene/lib/engine_grammar.py — the PR-owned hunk (clip_token, required_order, _order_rule, explain_mismatch) is byte-identical to what this lane and the security lane already traced stage-by-stage against match_invocation on commit 5dc6aa5/8fd879c.
  • plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py — the PR-owned hunk (_OPERATOR_LABELS, _unparsable_reason, _resolves_to_engine, _engine_mismatch_reason with the payload/operand priority, _engine_flag_order_rule, _ENGINE_GATE_SCOPE with the owner's option-2 qualifier, _reads_as_engine_payload) is byte-identical to commit 45291e1, which this lane's prior pass (2026-09-30T22:09:50Z) and the security lane's prior pass (2026-09-30T22:09:53Z) already reviewed in full, including verifying the refactor of _is_interpreter/the inline payload check into a function shared by _engine_gate_relevant and the new reason text is behavior-preserving.
  • plugins/disk-hygiene/skills/clean/scripts/test_hygiene.py — the PR-owned hunk (the engine-gate denial tests from line 10464 on: test_engine_gate_denial_names_the_flag_order_violation, test_engine_mismatch_reason_names_each_early_stage, test_engine_gate_gates_a_relative_word_that_resolves_to_the_engine, test_engine_gate_denial_names_the_payload_word_it_gated, etc.) is unchanged. The ~380 new lines that landed in this file via the merge (class-matched policy rule tests, birthtime/atime/ctime age-basis tests) belong to #5628 on main, not this PR, and sit outside the origin/main...HEAD scope.
  • plugins/disk-hygiene/.claude-plugin/plugin.json / CHANGELOG.md — version bump 0.41.0 → 0.41.1 (correctly sequenced above main's current 0.41.0), with a [0.41.1] entry accurately describing both the flag-order/scope-qualifier text and the payload-word naming. No behavior change implied or made.

Skip gate applies (criterion 4: this head already has a successful review from this lane that still applies, and the security lane already reviewed the same content in depth). No new correctness or maintainability findings — this PR's own code is identical to what was reviewed on ba89e97/45291e1, including the owner's option-2 fix for the Codex P2 thread (_ENGINE_GATE_SCOPE qualifier) and the payload-word naming, both already confirmed correct in prior passes.

Verification note: static diff review only in this pass (gh pr diff, git diff between commits already in the local branch history) — I did not re-run test_hygiene.py, scripts/run-ruff.sh, or the changelog/plugin validators in this session; the PR body's test/lint results are author-reported and unverified by me here.

Branch: fix/5519-engine-gate-denial-text

@claude

claude Bot commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

Claude finished @kyle-sexton's task in 1m 31s —— View job


Security review — PR #5582

Result: skip gate applies — no new security-relevant change since the prior review

Scope. This is a shallow checkout, so git diff origin/main...HEAD (three-dot) can't resolve a merge base; git diff origin/main HEAD (two-dot, tree-to-tree) against the 5 files this PR declares reports 522 insertions / 18 deletions, matching the PR metadata exactly, so the scoping is correct.

What changed since the last review on this thread. The prior review (on ba89e97) found no security issues and confirmed the owner's option-2 fix (45291e1) had landed. This push (ba89e97 → 226d42a) is a merge: sync origin/main into fix/5519, renumber disk-hygiene 0.41.1. Diffing ba89e97..226d42a directly on this PR's 5 files shows:

  • plugins/disk-hygiene/skills/clean/scripts/destructive_guard.py — byte-identical, zero diff. This is the file carrying all of this PR's security-relevant surface (_engine_mismatch_reason, _resolves_to_engine, _engine_flag_order_rule, _ENGINE_GATE_SCOPE, _reads_as_engine_payload, and the _decide/_bash_denial_guidance wiring), and none of it moved.
  • plugins/disk-hygiene/lib/engine_grammar.py — gained one declarative Flag("--in-flight-refs", ...) entry on the scan subcommand spec. This is not this PR's content: it's origin/main's already-merged, already-reviewed #5628 (4f5208c), picked up by the sync merge. Confirmed origin/main's own plugin.json is already at 0.41.0 (post-feat(disk-hygiene): class-matched policy rules, atime/ctime age basis, reference in-flight input #5628), so this flag is already present in origin/main and does not appear in the origin/main HEAD scoped diff used above — it only showed up in the ba89e97..226d42a diff because ba89e97 predates that sync. Since explain_mismatch/_engine_mismatch_reason read SUBCOMMANDS declaratively (by design, per this PR's own stated goal of covering handoff-apply's flags the same way), a new optional flag on an existing subcommand doesn't add new logic for this PR to be reviewed on — it's covered by the same mechanism already reviewed.
  • plugins/disk-hygiene/CHANGELOG.md / plugin.json — version renumber (0.40.1 → 0.41.1) and a new [0.41.0] entry for feat(disk-hygiene): class-matched policy rules, atime/ctime age basis, reference in-flight input #5628's own content, both non-security, sequenced above origin/main's current 0.41.0.
  • test_hygiene.py — all new tests are for feat(disk-hygiene): class-matched policy rules, atime/ctime age basis, reference in-flight input #5628 (class-matched policy rules, atime/ctime basis, in-flight refs). No changes to the engine-gate tests this PR owns (test_engine_gate_gates_a_relative_word_that_resolves_to_the_engine, test_engine_gate_denial_names_the_payload_word_it_gated, etc.).

Per this lane's skip gate ("this head already has a successful security review that still applies"), I'm not re-deriving findings already on record from the ba89e97 review. That review's conclusions still hold verbatim: decision/message separation holds (_bash_denial_guidance runs only after _decide has already settled deny/ask/allow), the fail-closed except BaseException boundary in main() still wraps this code unchanged, token echoing still goes through engine_grammar.clip_token (repr() + length cap) on content only the caller's own command already contained, and no instruction-surface guardrail is narrowed or removed — this PR only adds explanatory text on an already-denied path.

No security issues found in this PR.
· Branch: fix/5519-engine-gate-denial-text

@kyle-sexton
kyle-sexton merged commit efb9f4b into main Sep 30, 2026
29 checks passed
@kyle-sexton
kyle-sexton deleted the fix/5519-engine-gate-denial-text branch September 30, 2026 22:33
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.

disk-hygiene: relax the engine guard for read-only mentions and name the failing token in the denial

1 participant