fix(review-tutor): let the model open the tutor without pre-approving its shell call - #92
Merged
Merged
Conversation
The Claude plugin skill carried disable-model-invocation, so the tutor could only be started by typing /review-tutor. An agent asked to walk a PR could not reach it, which is the main way the tutor gets used. Codex and Pi never had an equivalent gate, so this also makes the three harnesses behave the same. Widen the description with the trigger phrasing the model matches on. The allowed-tools bound, the argv validation, and the rule that nothing from the conversation reaches the tutor are unchanged.
Removing disable-model-invocation made the skill reachable from model context, which turned the pre-approved allowed-tools rule into a shell injection path: permission matching splits on compound operators but not on $(...), and the tutor's whole job is reading untrusted diffs. The skill body's argv rules are model instructions, not enforcement. Drop the pre-approval instead of the invocability. The model can still decide to open the tutor; the Bash call now goes through normal confirmation, so the user sees the exact command. Record the general rule in AGENTS.md.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What changed
The Review Tutor Claude Code skill carried
disable-model-invocation: true, so the tutor could only be started by a human typing/review-tutor. An agent asked to walk a PR could not reach it, which is how the tutor mostly gets used. Codex and Pi never had an equivalent gate.Two commits, because the first one was wrong on its own:
disable-model-invocationand widen the description with the trigger phrasing.allowed-tools: Bash(npx -y @pickforge/review-tutor *)pre-approval.Step 2 is the important one. Once the model can invoke the skill, a pre-approved Bash rule stops being a boundary: Claude Code's permission matching splits on compound operators but not on
$(...), so'worktree'$(...)still matches the approved prefix and the shell evaluates it before the CLI ever validates its argv. The tutor's entire job is reading untrusted diffs, so that path is reachable by prompt injection from a hostile PR. The skill body's quoting and charset rules are model instructions, not enforcement.Dropping the pre-approval keeps what was wanted (the model can decide to open the tutor) and gives back what mattered (the user sees the exact command before it runs).
Tested
npx vitest run packages/review-tutor— 395 passed, 12 filesnameanddescriptionpresent, nodisable-model-invocation, noallowed-tools.claude/settings.jsonnor.claude/settings.local.jsoncarries a matching Bash ruleNot tested
git-subdirfrommain, so this takes effect after merge plus a plugin update.Known risks
A user-level broad Bash permission or bypass-permissions mode can still suppress the confirmation. That is outside this skill's control, and it is why the rule now lives in
AGENTS.mdrather than only in this file.Review
Risk class: standard. One reviewer, security and trust boundaries profile (Sol, medium), which raised the P1 above; one targeted fix-verification round, clean. KISS verdict:
keep.Durable lesson promoted to
AGENTS.md: never pair a pre-approvedallowed-tools: Bash(...)rule with a model-invocable skill whose argument the model supplies.No tracking issue — the change is six lines across two files and does not belong to an existing epic.