feat(work-items): loop-start permission preflight — detect and report once, never self-apply - #730
Conversation
|
Claude finished @kyle-sexton's task in 2m 50s —— View job PR Review —
|
![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: 7270e026b0
ℹ️ 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".
… once, never self-apply (#495) New skills/work/scripts/preflight.sh (+28-case hermetic test) reports up front what an unattended lane would otherwise discover as mid-cycle prompts: cwd-not-a-repo (note), core working verbs uncovered by any Bash()/PowerShell() allow rule, and a worktree root missing from additionalDirectories — Windows and git-bash path spellings folded for comparison, always exit 0, --count for a scripted gate, no live permission probe. Report-only by design: the classifier blocks an agent broadening its own permissions.allow, so remediation points at the standards claude-permissions component composed through the dotfiles seam, never a self-grant. reference/permission-preflight.md is the source of truth; the work skill wires the check as the first loop-start action and notes babysit-prs applicability by pointer. Bump 0.17.0 + CHANGELOG (composed above the concurrently-merged 0.16.0). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7270e02 to
0a8773e
Compare
|
Claude finished @kyle-sexton's task in 2m 55s —— View job PR Review —
|
| Issue | Status |
|---|---|
CHANGELOG merge-conflict marker (||||||| Stash base) |
✅ Fixed — clean in 0a8773e |
../ reach-out in reference/permission-preflight.md |
❌ Still present |
dotfiles#233 vs PR-cited dotfiles#242 in preflight.sh |
❌ Still present |
Remaining: Should Fix
1. ../ relative path in reference/permission-preflight.md still violates the plugin-form-safe rule
Line 13 of reference/permission-preflight.md still links:
the repo's [permission-rule-hygiene convention](../../../docs/conventions/permission-rule-hygiene/README.md)The file does exist at docs/conventions/permission-rule-hygiene/README.md in this repo, so the link works during development. But from plugins/work-items/reference/, the path ../../../ escapes the plugin root entirely. When the plugin is installed to its isolated cache, the path breaks. CLAUDE.md is explicit: no ../ reach-outs. Replace with a prose reference or an absolute GitHub URL:
the repo's [permission-rule-hygiene convention](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/conventions/permission-rule-hygiene/README.md)Remaining: Minor
2. Reference number discrepancy — dotfiles#233 vs. dotfiles#242
Line 205 of preflight.sh names dotfiles#233 as the chezmoi composition seam in the gap-(b) message:
composed into settings via dotfiles#233The PR description cites melodic-software/dotfiles#242 for that seam. These should be reconciled — if #233 was the original seam PR and #242 is the live one operators need to reference, the message should cite the current one. Operators reading the gap message and looking at #233 will not find actionable guidance.
Evaluating Codex Findings
Codex P2 — preflight.sh:136: Narrow specific rules produce false negatives
Confirmed as a real edge case. In verb_covered, the branch "$verb "* matches any rule whose inner body starts with the probe verb followed by a space. This means Bash(git commit --amend) satisfies the git commit probe — the preflight reports "covered" even though that rule grants only the amend path, not bare commits. The test in case 7 guards the hyphenated-subcommand boundary (git commit-tree) but there is no case for the specific-argument false-negative.
In practice this is low impact because the fleet's canonical floor uses Bash(git commit *) (wildcard), and the preflight is report-only — a false negative means a missed warning, not a permission grant. But an operator who has only a narrow --amend rule (e.g., from a prior manual entry) will get a clean preflight and then be surprised by mid-cycle prompts for bare commits.
Codex P2 — preflight.sh:113: settings.local.json in main checkout vs. worker worktrees
Confirmed as a logically valid concern. The collect() function unions in settings.local.json from the git toplevel of the preflight's working directory (typically the main checkout). Workers dispatched by the lane run in separate linked worktrees that are fresh, clean checkouts — settings.local.json (gitignored) is not propagated unless the operator explicitly copies it. An operator who added a missing verb to the main checkout's settings.local.json and got a clean preflight will still see per-operation prompts in each worker worktree.
The canonical remedy (operator-side via user-global ~/.claude/settings.json) sidesteps this entirely, and the reference doc correctly points there. The gap is worth noting in a comment or in the reference doc, so operators who reach for settings.local.json understand the scope. Not a must-fix, but worth documenting.
What Remains Good
Everything from the previous review's "What's Good" section still holds for this commit — the verb_covered boundary logic, normalize_path + dir_covered Windows/POSIX folding, settings union, --count mode, test isolation, and 28/28 assertions are all intact and correct. The SKILL.md wiring is clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a8773e122
ℹ️ 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".
…ee scoping, git add probe, deny-wins, comment hygiene Verb coverage now matches only an open grant (bare verb, space or colon open-glob) so a flag-scoped rule never masquerades as permission; --project-root scopes project-settings reads to the dispatched worker worktree (orchestrator contract documented); git add joins the probe set (staging precedes committing); deny rules are collected across the same scope set and win over allow with a distinct denied message (exact-shape-only, documented — never false-flags the flag-scoped deny floor); issue-number references dropped from code comments for the comment-hygiene gate. 44 hermetic checks pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 27s —— View job PR Review —
|
| Issue | Status |
|---|---|
CHANGELOG merge-conflict marker (||||||| Stash base) |
✅ Fixed — 0a8773e |
Codex P2 — narrow rule false-negative (git commit --amend masking bare git commit) |
✅ Fixed — a70cebc |
Codex P2 — settings.local.json in main checkout masking worker-side gap |
✅ Fixed — a70cebc (--project-root) |
Codex P2 — git add missing from probe set |
✅ Fixed — a70cebc |
| Codex P2 — deny rules not consulted | ✅ Fixed — a70cebc |
../ reach-out in reference/permission-preflight.md |
❌ Still present |
dotfiles#233 vs PR-cited dotfiles#242 in preflight.sh |
❌ Still present |
The a70cebc Fix Batch — Verified Correct
Open-grant matching (verb_in_rules). The new case arms at line 171 match "$verb", "$verb "'*', and "$verb:"'*' — the bare verb and its two open-glob spellings only. Bash(git commit --amend) inner is "git commit --amend", which matches none of the three arms → correctly stays a gap. Case 7b (two flag-scoped rules → two gaps) verifies this.
--project-root worktree scoping. proj_src / proj_toplevel / proj_base correctly re-point the project settings layer to the passed root while user-global settings are read regardless. Case 13 confirms: without --project-root the main checkout's settings.local.json masks the gap; with it pointing at the worker worktree, the gap surfaces.
git add probe. Line 251. Case 5b verifies a settings file with all other verbs but no git add rule yields exactly one gap. The FULL_ALLOW fixture at test line 72 includes Bash(git add *), keeping the clean-run case (case 3) correct.
Deny-wins logic. verb_denied() is checked first; denied verbs emit "is DENIED" and skip the allow check. The exact-shape-only conservatism is correct: a flag-scoped deny rule like Bash(git push --force) does NOT match the git push probe (inner "git push --force" matches none of the three case arms), so the standard destructive-verb deny floor is never false-flagged. Cases 14 and 15 verify the distinct message and gap count.
44 assertions. Verified across 15 cases (2+2+4+3+5+2+2+1+4+3+1+3+1+1+3+4+3 = 44). ✓
No new issues introduced by the fix batch.
Remaining: Should Fix
../ relative path in reference/permission-preflight.md:13 — third round
Line 13 still links:
[permission-rule-hygiene convention](../../../docs/conventions/permission-rule-hygiene/README.md)From plugins/work-items/reference/, ../../../ escapes the plugin root entirely. CLAUDE.md is explicit: "No ../ reach-outs." When installed to the isolated cache, this path does not exist. Replace with an absolute URL:
[permission-rule-hygiene convention](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/conventions/permission-rule-hygiene/README.md)Remaining: Minor
dotfiles#233 vs. dotfiles#242 in preflight.sh:249
Line 249 still names dotfiles#233 in the gap-(b) operator message:
composed into settings via dotfiles#233The PR description cites melodic-software/dotfiles#242 as the chezmoi composition seam. An operator reading the gap message and looking at #233 will not find current actionable guidance. Unify to the live reference.
What's Good
Everything from prior reviews holds. The a70cebc batch is a clean, well-scoped fix: the verb_in_rules rewrite is correct and minimal, --project-root is correctly documented in the reference doc and usage string, the git add probe follows directly from the commit workflow, and the deny-check-first posture with distinct messaging is the right design. The 16 new assertions in cases 5b, 7b, 13, 14, and 15 directly exercise each of the four fixed behaviors. The reference doc (permission-preflight.md) is substantially better for it — the open-grant definition, deny-wins semantics, and --project-root guidance are now all in one place and accurate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a70cebccc0
ℹ️ 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".
Coverage requires an open-glob grant (bare-exact matches only the argumentless literal, which no lane invocation is); deny keeps the bare spelling (erring wide on deny is safe). The autonomous path (--worktree-root) excludes gitignored settings.local.json from coverage reads — a fresh linked worktree carries tracked project settings only — with a report header saying so; the interactive path keeps local in scope; deny always reads local. 50 hermetic checks pass; live smoke on both paths matches expectations. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 57s —— View job PR Review —
|
| Issue | Status |
|---|---|
| CHANGELOG merge-conflict marker | ✅ Fixed — 0a8773e |
Codex P2 — narrow rule false-negative (git commit --amend masking bare git commit) |
✅ Fixed — a70cebc |
Codex P2 — settings.local.json in main checkout masking worker-side gap |
✅ Fixed — a70cebc (--project-root) |
Codex P2 — git add missing from probe set |
✅ Fixed — a70cebc |
| Codex P2 — deny rules not consulted | ✅ Fixed — a70cebc |
Codex P2 — bare-exact allow (Bash(git commit)) treated as coverage |
✅ Fixed — 987e198 |
Codex P2 — --worktree-root path still reads main checkout's settings.local.json |
✅ Fixed — 987e198 |
../ reach-out in reference/permission-preflight.md |
❌ Still present (third round) |
dotfiles#233 vs. PR-cited dotfiles#242 in preflight.sh |
❌ Still present (third round) |
The 987e198 Fix Batch — Verified Correct
Bare-exact allow is no longer coverage. verb_covered() now passes "open-glob-only" to verb_in_rules, and the match_bare check ("open-glob-only" == "bare") never fires. Bash(git commit) → inner git commit → hits neither "$verb "'*' nor "$verb:"'*' → correctly stays a gap. Case 7c verifies: one bare-exact commit rule → one gap.
The case arm "$verb "'*' and "$verb:"'*' are quoted literal matches (not globs), so Bash(git push --force-with-lease *) → inner git push --force-with-lease * is distinct from the literal string git push * and correctly does NOT cover git push. Cases 7b (two flag-scoped rules → two gaps) and 7c (bare-exact → gap) both confirm.
Autonomous path drops settings.local.json from coverage. collect() now takes a local_mode parameter; when --worktree-root is set, cov_local="no-local" and the local file is not read for allow + additionalDirectories. Deny always reads it ("with-local"), erring wide. Case 16 verifies the split: interactive path closes a gap that only lives in settings.local.json; autonomous path surfaces it, and the report header announces the exclusion.
50 assertions verified. Case 7c (2) + Case 16 (4) = 6 new assertions; the existing 44 are intact.
No new issues introduced by this batch.
Remaining: Should Fix
1. ../ relative path in reference/permission-preflight.md:13 — third round
Line 13 still links:
[permission-rule-hygiene convention](../../../docs/conventions/permission-rule-hygiene/README.md)CLAUDE.md: "reference only files inside the plugin via ${CLAUDE_PLUGIN_ROOT}; No ../ reach-outs." From plugins/work-items/reference/, ../../../ resolves to the repo root — outside the plugin. When installed to the isolated cache, this path does not exist. Replace with an absolute GitHub URL:
[permission-rule-hygiene convention](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/conventions/permission-rule-hygiene/README.md)Remaining: Minor
2. dotfiles#233 vs. dotfiles#242 in preflight.sh:271 — third round
Line 271 still names dotfiles#233 in the gap-(b) operator message:
composed into settings via dotfiles#233The PR description cites melodic-software/dotfiles#242 as the live chezmoi composition seam. An operator looking at #233 will not find actionable guidance if #242 is the current PR.
Trivial
FULL_ALLOW fixture carries a dead-weight bare-exact entry.
preflight.test.sh:72 has "Bash(git push)" alongside "Bash(git push *)". Under the new semantics, bare-exact entries provide no coverage; git push is covered by the open-glob form, so the fixture is correct and all clean-run assertions pass. But a future reader of the fixture might believe the bare entry contributes, or might drop the open-glob thinking the bare suffices. Consider removing "Bash(git push)" from FULL_ALLOW to keep the fixture a clean example of the open-glob-only coverage contract.
What's Good
Everything from prior reviews holds. The 987e198 batch is minimal and well-scoped: verb_covered calling verb_in_rules with "open-glob-only" is the exact right change (one word at the call site, two new test cases), and the collect() local_mode parameter is clean — the autonomous/interactive split is handled in one place with deny always wide. The reference doc update (open-glob definition, deny-wider-than-coverage, autonomous path note) is accurate and matches the implementation.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 987e198cf9
ℹ️ 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".
… comparison Windows filesystems are case-insensitive, so both the drive-letter and git-bash spellings fold whole-path lowercase before comparison; POSIX paths stay case-sensitive (a true single-letter root is the accepted, documented collision). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 4m 1s —— View job Security Review —
|
|
Claude finished @kyle-sexton's task in 1m 51s —— View job PR Review —
|
| Issue | Status |
|---|---|
| CHANGELOG merge-conflict marker | ✅ Fixed — 0a8773e |
| Codex P2 — narrow rule false-negative | ✅ Fixed — a70cebc |
Codex P2 — settings.local.json masking worker-side gap |
✅ Fixed — a70cebc (--project-root) |
Codex P2 — git add missing from probe set |
✅ Fixed — a70cebc |
| Codex P2 — deny rules not consulted | ✅ Fixed — a70cebc |
| Codex P2 — bare-exact allow treated as coverage | ✅ Fixed — 987e198 |
Codex P2 — --worktree-root still reads main settings.local.json |
✅ Fixed — 987e198 |
| Codex P2 — Windows path case only lowercases drive letter | ✅ Fixed — 3251fff |
../ reach-out in reference/permission-preflight.md |
❌ Still present (fourth round) |
dotfiles#233 vs. PR-cited dotfiles#242 in preflight.sh |
❌ Still present (fourth round) |
The 3251fff Fix — Verified Correct
Windows path case-folding is now whole-path. normalize_path() at lines 214–228 now folds the entire path lowercase before the drive-colon extraction for the [A-Za-z]:/*) arm, and adds a symmetric /[A-Za-z]/*) arm that folds the git-bash form in full. The transformation chain for D:\Repos\.Worktrees is: backslash→slash (D:/Repos/.Worktrees) → whole-path lowercase (d:/repos/.worktrees) → drive extraction → /d/repos/.worktrees. Both directions now normalize to the same value, so the ancestor check in dir_covered() correctly resolves. POSIX paths with no leading drive letter hit the *) arm and remain untouched — case-sensitive as before.
No test was added for the mixed-case scenario. The Codex reviewer explicitly said "add a mixed-case fixture." The existing WIN_ROOT fixture (test line 78) is D:\repos\.worktrees — already all lowercase, so case 9 never exercises the new code path. This means a regression in normalize_path() for mixed-case Windows paths would not be caught by the test suite.
Consider adding a fixture:
WIN_ROOT_MIXED="D:${BS}Repos${BS}.Worktrees" # mixed-case Windows form
# Case 9b: mixed-case Windows additionalDirectories entry covers POSIX-form child
assert_eq "mixed-case Windows entry covers POSIX child" "0" \
"$(run "$REPO" "$CFG_MIXED" --count --worktree-root "$POSIX_CHILD")"This is a minor quality gap — the fix itself is correct, the production behaviour is correct, and the test suite has 54 assertions covering the rest of the logic. But the untested code path is the specific scenario the Codex P2 was about.
Remaining: Should Fix
1. ../ relative path in reference/permission-preflight.md:13 — fourth round
Line 13 still links:
[permission-rule-hygiene convention](../../../docs/conventions/permission-rule-hygiene/README.md)From plugins/work-items/reference/, ../../../ escapes the plugin root entirely. CLAUDE.md is explicit: "reference only files inside the plugin via ${CLAUDE_PLUGIN_ROOT}; No ../ reach-outs." When installed to the isolated cache, the path does not exist. Replace with an absolute GitHub URL:
[permission-rule-hygiene convention](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/conventions/permission-rule-hygiene/README.md)Remaining: Minor
2. dotfiles#233 vs. dotfiles#242 in preflight.sh:281 — fourth round
Line 281 still names dotfiles#233 in the gap-(b) operator message. The PR description cites melodic-software/dotfiles#242 as the live chezmoi composition seam. An operator looking at #233 will not find actionable guidance.
What's Good
Everything from prior reviews holds. The 3251fff fix is minimal and correct — two targeted case arms, matched prose comments, and no changes to the surrounding logic. The 54 existing assertions remain intact and green. The overall PR is in good shape; the two remaining items are a design-rule violation (the ../ link) and an operator-facing reference number discrepancy — both straightforward one-line fixes.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3251fffa19
ℹ️ 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".
…uances A gapped verb holding only a bare-exact allow now gets a precise message (argumentless invocations like babysit's plain git push are covered; argument-carrying work-lane forms are not — grant the open glob) instead of the generic missing-allow text; a distinct --project-root naming a real worker worktree reads that checkout's own settings.local.json, and the autonomous local-exclusion applies only pre-dispatch. 56 hermetic checks pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 59s —— View job PR Review —
|
| Issue | Status |
|---|---|
| CHANGELOG merge-conflict marker | ✅ Fixed — 0a8773e |
| Codex P2 — narrow rule false-negative | ✅ Fixed — a70cebc |
Codex P2 — settings.local.json masking worker-side gap (--project-root) |
✅ Fixed — a70cebc |
Codex P2 — git add missing from probe set |
✅ Fixed — a70cebc |
| Codex P2 — deny rules not consulted | ✅ Fixed — a70cebc |
| Codex P2 — bare-exact allow treated as coverage | ✅ Fixed — 987e198 |
Codex P2 — --worktree-root still reads main settings.local.json (pre-dispatch) |
✅ Fixed — 987e198 |
| Codex P2 — Windows path case only lowercases drive letter | ✅ Fixed — 3251fff |
| Codex P2 — bare-exact grant gives generic "no allow" gap message | ✅ Fixed — cc77f26 |
Codex P2 — worker with propagated settings.local.json gets false gaps |
✅ Fixed — cc77f26 |
../ reach-out in reference/permission-preflight.md |
❌ Still present (fifth round) |
dotfiles#233 vs. PR-cited dotfiles#242 in preflight.sh |
❌ Still present (fifth round) |
The cc77f26 Fix Batch — Verified Correct
Nuanced bare-exact gap message. The new bare-exact-only mode in verb_in_rules powers the new verb_bare_exact() helper. The three-way mode logic is correct:
open-glob-only(coverage): skips open-glob check whenmode == "bare-exact-only", and only checks bare whenmode == "bare" || "bare-exact-only"→ foropen-glob-only, only the open-glob arm fires.Bash(git push *)→return 0;Bash(git push)→ falls through →return 1. ✓bare-exact-only(sharpened-message probe): skips open-glob, checks bare only →Bash(git push)→return 0;Bash(git push *)→return 1. ✓bare(deny): checks both → any form satisfies. ✓
The gap emission chain at lines 301–310 is: deny-check → open-glob-covered → bare-exact-only → generic missing. Each path is exclusive, and the right case hits the right message. A verb with both a flag-scoped rule AND a bare-exact (but no open-glob) correctly routes to the nuanced bare-exact message. ✓
Distinct --project-root reads the worker's own settings.local.json. Lines 163–178: distinct_project_root is set when --project-root is given AND proj_base != repo_root. The two-flag split (distinct_project_root / local_excluded) is clean:
--worktree-rootonly (pre-dispatch) →local_excluded="yes"→cov_local="no-local"✓--worktree-root+ distinct--project-root(named worker) →distinct_project_root="yes"→local_excluded=""→cov_local="with-local", reads the WORKER's own local ✓- Neither →
cov_local="with-local"(interactive) ✓
Non-git --project-root is safe: proj_base defaults to the raw path, distinct_project_root fires, read_array silently skips missing files. ✓
Case 17 logic verified. Case 17 writes Bash(gh pr create *) only to a separate git init worker's settings.local.json. With --project-root <worker>: proj_base = WORKER2TOP ≠ repo_root → distinct → reads worker's local → 0 gaps. Without: local excluded (pre-dispatch) → gh pr create missing → 1 gap. Both assertions pass as expected. ✓
60 assertions across 17 cases. Case 7c revised (+3: three new assertions + old 2 replaced) + case 17 (+4). All prior cases intact.
Remaining: Should Fix
1. ../ relative path in reference/permission-preflight.md:13 — fifth round
Line 13 still links:
[permission-rule-hygiene convention](../../../docs/conventions/permission-rule-hygiene/README.md)From plugins/work-items/reference/, ../../../ resolves to the repo root — outside the plugin. CLAUDE.md is explicit: "reference only files inside the plugin via ${CLAUDE_PLUGIN_ROOT}; No ../ reach-outs." When installed to the isolated cache, this path does not exist. Replace with an absolute GitHub URL:
[permission-rule-hygiene convention](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/conventions/permission-rule-hygiene/README.md)Remaining: Minor
2. dotfiles#233 vs. dotfiles#242 in preflight.sh:310 — fifth round
Line 310 still names dotfiles#233 in the gap-(b) operator message:
composed into settings via dotfiles#233The PR description cites melodic-software/dotfiles#242 as the live chezmoi composition seam. An operator reading the gap message and following #233 will not find current actionable guidance.
Trivial
FULL_ALLOW fixture still carries a dead-weight Bash(git push) alongside Bash(git push *).
preflight.test.sh:72 has both entries. With the new verb_bare_exact function now in the codebase, a future reader of FULL_ALLOW might wonder if the bare entry is intentionally demonstrating the nuanced-message path — it is not; verb_covered hits the open-glob first and the bare entry is never consulted. Consider removing "Bash(git push)" from FULL_ALLOW for fixture clarity. Noted again but still non-blocking.
What's Good
Everything from prior reviews holds. The cc77f26 batch is well-scoped and correct: the three-way verb_in_rules mode is a minimal, clean generalization; the distinct_project_root / local_excluded two-flag split handles all three invoke paths without combinatorial complexity; and case 17 directly exercises the specific scenario Codex raised (propagated worker-local settings). The reference doc update — the two-bullet settings.local.json scope note and the revised GAP (b) description now enumerating all three sub-cases — is accurate and matches the implementation. 60 assertions across 17 cases.
|
Claude finished @kyle-sexton's task in 3m 34s —— View job Security Review —
|
| Finding | Previous verdict | Current status |
|---|---|---|
Heredoc delimiter collision (<<RULES, <<DIRS) |
SUGGESTION, HIGH confidence | ❌ Still present |
$worktree_root reflected in gap message |
SUGGESTION, HIGH confidence | ❌ Still present |
Finding 1 — SUGGESTION (confidence: HIGH)
Heredoc delimiter collision can produce false results in verb and directory coverage checks
Still present from prior review. preflight.sh:220–223 and preflight.sh:276–279:
done <<RULES
$rules
RULESdone <<DIRS
$ALL_ADDDIRS
DIRS$rules is the newline-joined output of jq -r '.permissions.allow | .[]' (or deny) across up to three settings files. If any permission rule entry equals the bare string RULES on its own line, bash terminates the heredoc at that point and all subsequent entries are silently dropped.
The security-relevant direction is the deny check: if a deny rule that would flag a verb as blocked appears after a RULES-named entry in the joined output, verb_denied() returns false — the verb appears uncovered-but-not-denied — while the verb's allow rule (if present) would still be matched. The result is the preflight reports a covered verb when it is in fact denied in the effective configuration. Since this is a report-only tool, the actual Claude Code permission engine is unaffected; only the preflight report misleads the operator.
Prerequisites: an adversary would need write access to one of the three settings files to plant a literal RULES or DIRS entry — at which point they have far more direct options. Practical risk is very low.
Suggested fix — switch both loops from heredoc to process substitution, eliminating the delimiter problem entirely:
while IFS= read -r rule; do
...
done < <(printf '%s\n' "$rules")
while IFS= read -r entry; do
...
done < <(printf '%s\n' "$ALL_ADDDIRS")Process substitution is available under #!/usr/bin/env bash. The return statements inside each function work correctly with process substitution — they exit the function, not any subshell.
Finding 2 — SUGGESTION (confidence: HIGH)
User-supplied paths reflected verbatim in output messages
Still present from prior review, with one new instance added in cc77f26. Three places reflect user-supplied values in the output:
preflight.sh:322—$worktree_root(from--worktree-root/PREFLIGHT_WORKTREE_ROOT) in gap(c)message (identified previously)preflight.sh:337—$proj_basein the new distinct-project-root header (added incc77f26)
$proj_base is the output of git rev-parse --show-toplevel applied to the caller-supplied --project-root argument, or $project_root directly if the git call returns nothing. Both are local file-system paths that can contain unusual characters.
The output goes to stdout, which the SKILL renders in the agent context. All three output calls use echo / printf '%s\n' — no format-string issue — and the data never reaches a eval or command substitution. Risk is display-side only: a path containing ANSI escapes or markdown characters could produce visually anomalous output. No command injection is possible via this path.
No code change strictly necessary; noting for completeness given the prior review flagged $worktree_root and cc77f26 added a parallel instance.
cc77f26 — New Code Cleared
The changes in the HEAD commit introduce:
-
distinct_project_rootdetection — compares$proj_base(git-resolved) with$repo_root(git-resolved). Both values are outputs ofgit -C <double-quoted-path> rev-parse --show-toplevel. No user-controlled data enters the comparison in an unsafe way. -
verb_bare_exact/bare-exact-onlymode inverb_in_rules— probed with hardcoded verb strings only (git add,git commit,git push,gh pr create,gh issue comment). The[[ "$inner" == "$verb" ]]comparison is a safe string equality test. No injection surface. -
New nuanced gap message — contains
$verb(hardcoded probe string) interpolated into a display-only string. Safe. -
$proj_basein the new header message — the only new reflection of a user-supplied value (covered in Finding 2 above). -
Test case 17 — injects
settings.local.jsonviajq -n '…'into a temp directory undermktemp -d. Cleanup istrap 'rm -rf "$TEST_TMPDIR"' EXIT. The real~/.claudeis never read (CLAUDE_CONFIG_DIR override). No security concerns.
Explicitly Cleared (Full PR)
git -C "$CHECK_DIR"/git -C "$proj_src"— both arguments are double-quoted throughout; no word-splitting or glob expansion. A path beginning with-is passed as an operand to-C, not a flag. Safe.jq -r "$path // [] | .[]"inread_array—$pathis always a hardcoded literal (.permissions.allow,.permissions.deny,.permissions.additionalDirectories) from thecollect()caller. No user-controlled data enters the jq filter expression.case "$want" in "$entry"/*)indir_covered— in a bashcasepattern, the double-quoted"$entry"portion is treated literally (glob characters within the variable value are not re-expanded). The/*suffix is the only glob component. Not exploitable.printf '%s\n' "${findings[@]}"—%sformat specifier; no format-string injection possible.PREFLIGHT_FIXTURE_DIR/CLAUDE_CONFIG_DIRenv injection — used only asgit -Carguments and file-read paths, both double-quoted. An attacker able to set these already has local shell access.- No network access, no credentials read or logged anywhere in the diff.
Summary
No CRITICAL or IMPORTANT security issues. Two SUGGESTION-level findings remain from the prior review: the heredoc delimiter collision (low practical risk given the report-only posture and write-access prerequisite) and user-supplied path reflection in output (display-only). The cc77f26 commit adds no new security vulnerabilities.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc77f26b5e
ℹ️ 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".
… every absolute path The trailing-slash strip left the root entry expanding to //* which matched nothing, so an operator-configured ["/"] false-gapped. Root (and the defensive empty string) now short-circuit as universal ancestors. Test case pins it (with MSYS path-conversion guarded in the fixture). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 3m 23s —— View job PR Review —
|
| Issue | Status |
|---|---|
| CHANGELOG merge-conflict marker | ✅ Fixed — 0a8773e |
| Codex P2 — narrow rule false-negative | ✅ Fixed — a70cebc |
Codex P2 — settings.local.json masking worker-side gap |
✅ Fixed — a70cebc |
Codex P2 — git add missing from probe set |
✅ Fixed — a70cebc |
| Codex P2 — deny rules not consulted | ✅ Fixed — a70cebc |
| Codex P2 — bare-exact allow treated as coverage | ✅ Fixed — 987e198 |
Codex P2 — --worktree-root reads main checkout's settings.local.json pre-dispatch |
✅ Fixed — 987e198 |
| Codex P2 — Windows path case only lowercases drive letter | ✅ Fixed — 3251fff |
| Codex P2 — bare-exact grant gives generic gap message | ✅ Fixed — cc77f26 |
Codex P2 — worker with propagated settings.local.json gets false gaps |
✅ Fixed — cc77f26 |
| Codex P2 — filesystem root entry produces false GAP (c) | ✅ Fixed — 803f04e |
../ reach-out in reference/permission-preflight.md |
❌ Still present (sixth round) |
dotfiles#233 vs. PR-cited dotfiles#242 in preflight.sh |
❌ Still present (sixth round) |
The 803f04e Fix — Verified Correct
Filesystem root short-circuits correctly. The new guard at dir_covered() line 273:
[[ "$entry" == "/" || "$entry" == "" ]] && return 0is inserted after normalize_path "$entry" runs. Tracing "/" through normalize_path: the trailing-slash strip arm ?*/) requires at least two characters, so a bare / is left unchanged and normalize_path "/" returns "/". The guard's "$entry" == "/" arm therefore fires and dir_covered returns 0 — correctly treating the filesystem root as an ancestor of every absolute path. The "$entry" == "" arm is a defensive guard for a degenerate empty-string entry; it does not arise from a / input but protects against any future normalization path that could produce empty.
Case 18 exercises the fix directly. MSYS_NO_PATHCONV=1 correctly prevents git-bash from rewriting the bare "/" argument to the Git install directory before jq sees it. The single assertion confirms --count is 0 when additionalDirectories contains "/" and the worktree root is a normal absolute child. ✓
61 total assertions across 18 cases. Case 18 adds 1; all prior 60 are intact.
No new issues introduced by 803f04e.
Remaining: Should Fix
1. ../ relative path in reference/permission-preflight.md:13 — sixth round
Line 13 still links:
[permission-rule-hygiene convention](../../../docs/conventions/permission-rule-hygiene/README.md)From plugins/work-items/reference/, ../../../ escapes the plugin root entirely. CLAUDE.md is explicit: "reference only files inside the plugin via ${CLAUDE_PLUGIN_ROOT}; No ../ reach-outs." When installed to the isolated cache, this path does not exist. Replace with an absolute GitHub URL:
[permission-rule-hygiene convention](https://github.com/melodic-software/claude-code-plugins/blob/main/docs/conventions/permission-rule-hygiene/README.md)Remaining: Minor
2. dotfiles#233 vs. dotfiles#242 in preflight.sh:313 — sixth round
Line 313 still names dotfiles#233 in the gap-(b) operator message:
composed into settings via dotfiles#233The PR description cites melodic-software/dotfiles#242 as the live chezmoi composition seam. An operator reading the gap message and following #233 will not find current actionable guidance.
Trivial
FULL_ALLOW fixture still carries a dead-weight Bash(git push) alongside Bash(git push *).
preflight.test.sh:72 — with the bare-exact exclusion now in the codebase, a future reader of FULL_ALLOW might believe the bare entry contributes to coverage, or might drop the open glob thinking the bare suffices. Neither changes the correctness of any case (the open glob covers all clean-run assertions), but the fixture is cleaner without it.
What's Good
Everything from prior reviews holds. The 803f04e fix is exactly scoped: two lines added to dir_covered (the guard and its explanatory comment), one assertion in a new case 18, and MSYS_NO_PATHCONV=1 on the fixture write call for cross-platform correctness. No surrounding logic touched. The fix correctly handles the degenerate "" case as a defensive bonus without requiring a dedicated test (the practical value of / coverage is what matters).
The PR is now one trivial one-liner (the ../ link) away from clearing all design-rule requirements, and a second one-liner (the dotfiles#242 reference) away from operator-facing correctness in the gap message. Both are straightforward edits.
|
Claude finished @kyle-sexton's task in 2m 19s —— View job Security Review —
|
| Finding | Previous verdict | Current status |
|---|---|---|
Heredoc delimiter collision (<<RULES, <<DIRS) |
SUGGESTION, HIGH confidence | ❌ Still present |
| User-supplied paths reflected verbatim in output messages | SUGGESTION, HIGH confidence | ❌ Still present |
Finding 1 — SUGGESTION (confidence: HIGH)
Heredoc delimiter collision produces silent false results in verb and directory coverage checks
Still present, fourth round. preflight.sh:220–223 and preflight.sh:279–282:
done <<RULES
$rules
RULESdone <<DIRS
$ALL_ADDDIRS
DIRSIf any permission entry in the expanded variable equals the bare delimiter string (RULES or DIRS) on its own line, bash terminates the heredoc at that point and all subsequent entries are silently dropped. The most security-relevant direction remains the deny check: a deny rule that appears after a RULES-named entry would be silently skipped, causing verb_denied() to return false — the verb appears uncovered-but-not-denied — while an allow rule for the same verb would still be matched. The preflight would report a verb as covered when it is in fact denied. Since this is a report-only tool, the actual Claude Code permission engine is unaffected; only the preflight report misleads the operator. Practical risk remains very low (requires write access to a settings file).
Suggested fix — switch both loops to process substitution, eliminating the delimiter problem entirely:
while IFS= read -r rule; do
...
done < <(printf '%s\n' "$rules")
while IFS= read -r entry; do
...
done < <(printf '%s\n' "$ALL_ADDDIRS")Available under #!/usr/bin/env bash. The return statements inside each function work correctly with process substitution — they exit the function, not any subshell.
Finding 2 — SUGGESTION (confidence: HIGH)
User-supplied paths reflected verbatim in output messages
Still present. Two locations:
preflight.sh:325—$worktree_root(from--worktree-root/PREFLIGHT_WORKTREE_ROOT) in the gap(c)messagepreflight.sh:340—$proj_basein the distinct-project-root header
Both use echo / printf '%s\n' — no format-string injection possible. The output goes to stdout rendered in the agent context as plain text. Risk is display-side only: a path containing ANSI escape sequences or markdown syntax could produce visually anomalous output. No command injection path exists. No code change strictly necessary; noting for awareness as both prior reviews flagged this.
803f04e — New Code Cleared
The change in 803f04e adds three lines to dir_covered() at lines 271–273:
[[ "$entry" == "/" || "$entry" == "" ]] && return 0"$entry" == "/" arm — correct. normalize_path "/" returns "/" unchanged (the trailing-slash strip pattern ?*/ requires at least two characters and / alone has only one); without this check, the case pattern "$entry"/* would expand to //*, which doesn't match any standard absolute path. The short-circuit is the right fix.
"$entry" == "" arm — defensive and correct. [[ -n "$entry" ]] || continue is evaluated before normalize_path is called, so empty raw entries are skipped. The empty check after normalize_path guards against any hypothetical non-empty input that normalizes to empty; analysis of normalize_path's transformation chain (backslash→slash, Windows drive folding, trailing-slash strip) produces no such input in practice. The check is harmless and cannot be leveraged: even if it fired unexpectedly, it would only cause a false-negative in the report (missing a gap warning), which is in the same direction as the existing report-only posture.
MSYS_NO_PATHCONV=1 in Case 18 — correct. Prevents git-bash from rewriting the bare "/" argument to the Git install root before jq sees it. Inline env-var assignment scoped to the write_settings call; no shell state leakage.
No new security vulnerabilities introduced by 803f04e.
Explicitly Cleared (Full PR, confirmed against HEAD)
git -C "$CHECK_DIR"/git -C "$proj_src"— double-quoted throughout; no word-splitting or glob expansion. Safe.jq -r "$path // [] | .[]"inread_array—$pathis always a hardcoded literal from thecollect()caller; no user-controlled data enters the jq filter expression.case "$want" in "$entry"/*)indir_covered—"$entry"in a bashcasepattern is treated literally; glob characters in the variable value are not re-expanded. The/*suffix is the only glob component. Not exploitable.printf '%s\n' "${findings[@]}"—%sformat specifier; no format-string injection possible.PREFLIGHT_FIXTURE_DIR/CLAUDE_CONFIG_DIRenv injection (tests) — used only as quotedgit -Carguments and file-read paths. The real~/.claudeis never read.- No network access, no credentials read or logged anywhere in this diff.
Summary
No CRITICAL or IMPORTANT security issues found in this PR or in the 803f04e commit. Two SUGGESTION-level findings carry over from prior reviews without change: the heredoc delimiter collision (low practical risk, report-only tool, write-access prerequisite) and user-supplied path reflection in output (display-side only, no injection path). The 803f04e fix is minimal, correct, and introduces no new attack surface.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 803f04ecd4
ℹ️ 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".
1 participant
Summary
Ships #495's three fix directions:
reference/permission-preflight.mdcites the standardsclaude-permissionscomponent (the fleet's canonical allow/deny floor, merged today: 60 allow incl. PowerShell read-parity, 244 deny) composed operator-side via the dotfiles chezmoi seam — no restated list to drift.permissions.additionalDirectoriesentry, root-agnostic.skills/work/scripts/preflight.shreports ONCE, up front — cwd-not-a-repo (note), probed core verbs (git commit,git push,gh pr create,gh issue comment) uncovered by any allow rule, worktree root not inadditionalDirectories. Always exits 0 (report-only),--countfor scripted gating, no live permission probe. Never self-applies: the auto-mode classifier blocks an agent editing its ownpermissions.allow(empirically hit during work-items(work) + autonomy: unattended loop prompts the human — core git/gh verbs and out-of-tree write dirs missing from a shippable permission preflight #495's own remediation attempt), so the check detects and points at operator-side remediation. Wired as the work skill's first loop-start action; babysit-prs applicability noted by pointer (no cross-plugin edit).28/28 hermetic test cases; shellcheck/shfmt/markdownlint/typos/validate-plugin-contracts/changelog-parity all green. Live smoke on this machine correctly flagged two real gaps. Version 0.17.0 (composed above the concurrently-merged 0.16.0 container-verbs entry).
Related
Closes #495
🤖 Generated with Claude Code