Rewrite grok delegation mechanics around verified behavior - #8
Open
kelchm wants to merge 2 commits into
Open
Conversation
The previous guidance (draft PR #6) was built on two claims that don't hold up under test, both traced to experiments run inside /tmp: - "--sandbox does not prevent edits to the working directory" — false. Grok's read-only profile leaves /tmp, /var/tmp, /var/folders and ~/.grok writable by design, so a repo under any of those is unprotected while the sandbox still reports "enforced":true. Outside those paths it blocks the edit tool, shell writes, and new-file creation at the OS level. - "-w gives effective read-only protection for the real checkout" — false, and unsafe. In headless mode (-p / --prompt-file) the flag is silently ignored: no worktree is created, edits land in the real checkout, and --always-approve auto-approves everything. Verified twice, with no stderr warning. Also corrected: there is no ~100 KB input ceiling (151 KB and 204 KB prompts were read in full), and grok does ship a bundled /review skill that works headless — the old skill claimed it had none and hand-rolled a replacement. Changes: - CLAUDE.md: replace the grok Mechanics block with review (/review), custom stance, and implement recipes, plus the verified traps. Notably that grok's --sandbox read-only is NOT codex's -s read-only, and that --tools / --disallowed-tools fail OPEN on an unrecognized name while --deny fails closed. - grok-review: delegate to the bundled /review skill instead of reinventing it; drop the false "physically can't hit the network" claim (untrue on macOS, platform-dependent generally). - grok-implementation: new, mirroring codex-implementation — isolate with plain `git worktree add` + --cwd rather than grok's -w. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR separates Grok review and implementation workflows. It adds isolated worktree procedures, direct review commands, custom stance prompts, structured output guidance, and permission-control documentation. ChangesGrok workflows
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@dot_claude/CLAUDE.md`:
- Around line 55-57: Remove the manual Markdown line breaks so the Grok
mechanics list item in dot_claude/CLAUDE.md lines 55-57 remains on one physical
line, and the review description in dot_claude/skills/grok-review/SKILL.md lines
5-7 also remains on one physical line; preserve their existing wording and
formatting otherwise.
- Around line 59-60: Update the bundled /review command examples to enforce
repository-scoped read-only access by adding Edit($PWD/**) and Write($PWD/**)
denials. Apply this in dot_claude/CLAUDE.md lines 59-60,
dot_claude/skills/grok-review/SKILL.md lines 24-25 for local review mode, and
lines 28-33 for branch and PR review modes; keep $TMPDIR writable for review
notes.
- Around line 71-73: Update the worktree setup and cleanup instructions around
the grok invocation to require a unique, task-specific path instead of the
literal ../grok-task; pass that path consistently to git worktree add and the
grok --cwd option, and remove the worktree after completion.
In `@dot_claude/skills/grok-implementation/SKILL.md`:
- Around line 3-9: The front-matter description in SKILL.md is manually wrapped
across multiple lines; rewrite the description value as one uninterrupted line
while preserving its exact wording and meaning.
- Around line 25-37: Update the Grok prompt and cleanup flow around WORKTREE and
PROMPT so the final handoff occurs before removing the detached worktree:
require Grok to commit all changes on a task branch and report the commit SHA,
or export/apply a patch, then perform git worktree remove only after that
handoff succeeds.
- Around line 25-26: Update the worktree setup and cleanup instructions around
the WORKTREE variable to generate a unique task-specific path for each
invocation instead of using the shared ../grok-task path. Pass the same
$WORKTREE value to both git worktree add --detach and git worktree remove,
including the corresponding cleanup flow at lines 59-61.
In `@dot_claude/skills/grok-review/SKILL.md`:
- Around line 47-48: Update the prompt-building command in the Grok review skill
to include untracked files alongside the existing tracked diff. Append distinct
sections containing the tracked changed-file names from git diff --name-only
HEAD and untracked non-ignored files from git ls-files --others
--exclude-standard, including each untracked file’s materialized content in
PROMPT.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 1278553f-eff5-42b2-9553-94631b9fa4af
📒 Files selected for processing (3)
dot_claude/CLAUDE.mddot_claude/skills/grok-implementation/SKILL.mddot_claude/skills/grok-review/SKILL.md
Comment on lines
+47
to
+48
| # Write the stance below into $PROMPT, then append the target diff: | ||
| { echo; echo '--- CHANGES ---'; git --no-pager diff HEAD; } >> "$PROMPT" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
git --no-pager diff --name-only HEAD
git ls-files --others --exclude-standardRepository: kelchm/dotfiles
Length of output: 153
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf 'Tracked modified files against HEAD:\n'
git --no-pager diff --name-only HEAD || true
printf '\nUntracked files (tracked, name-only if untracked):\n'
git ls-files --others --exclude-standard || true
printf '\nGrok review skill relevant section:\n'
cat -n dot_claude/skills/grok-review/SKILL.md | sed -n '35,60p'
printf '\nSearch for local review /diff behavior hints:\n'
rg -n --no-heading 'review --local|git diff|PROMPT|UNTRACKED|others' dot_claude -SRepository: kelchm/dotfiles
Length of output: 3602
Include untracked files in custom-stance reviews.
git diff HEAD only includes tracked changes. If a new untracked file is being reviewed, this prompt omits the file entirely. Append a separate untracked-file materialization after the tracked diff; for example append the outputs of git --no-pager diff --name-only HEAD and git ls-files --others --exclude-standard into $PROMPT, formatted as distinct sections.
🧰 Tools
🪛 SkillSpector (2.5.1)
[warning] 47: [RA2] Session Persistence: Skill establishes unauthorized persistence across sessions via cron jobs, startup scripts, or state files. Session persistence allows an attacker to maintain access beyond the current interaction.
Remediation: Remove any persistence mechanisms (cron jobs, startup scripts, state files). Skills should not maintain state across sessions without explicit user consent.
(Rogue Agent (RA2))
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@dot_claude/skills/grok-review/SKILL.md` around lines 47 - 48, Update the
prompt-building command in the Grok review skill to include untracked files
alongside the existing tracked diff. Append distinct sections containing the
tracked changed-file names from git diff --name-only HEAD and untracked
non-ignored files from git ls-files --others --exclude-standard, including each
untracked file’s materialized content in PROMPT.
Drops the hard-containment review path. It defended against an adversary we don't have: grok is cooperative, and the realistic failure is it helpfully editing a file mid-review — which is visible in `git status` and revertible. Buying a hard boundary meant denying Bash, which meant losing the bundled /review skill (it needs the shell to collect the diff) and hand-rolling diff materialization, including a bash/zsh-only loop for untracked files. Not worth it, and it discarded /review's native --branch and --pr modes. Review guard is now: --sandbox read-only plus repo-scoped Edit/Write denies, shell left available. Custom-stance reviews name the target and let grok collect the diff itself rather than pre-materializing it. Implementation keeps the stronger worktree handoff: a generated task branch, a required commit SHA in the report, and integration before cleanup. The previous version ran `git worktree remove` at the end, which discards grok's work if it left anything uncommitted. Re-verified on grok 1.0.3 (macOS): `-w` is still a silent no-op headless (edit landed in the real checkout, no worktree created); `--tools` still fails open with one bad name; `/review --local` works under the guard and left the repo untouched. Version references reconciled — the traps were established on 0.2.118, with these three re-checked on 1.0.3. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Replaces #6, whose guidance was built on two claims that don't survive testing. (Supersedes #7, closed automatically when this branch was renamed — same lineage, same content.)
Why #6 was wrong
Both of its central claims trace back to experiments run inside
/tmp:--sandboxdoesn't prevent edits to the working dir-wworktree gives "effective read-only protection"read-onlyprofile leaves/tmp,/var/tmp,/var/foldersand~/.grokwritable by design, so a repo under any of those is unprotected whilesandbox-events.jsonlstill logs"enforced":true. The event log shows the original experiments ran under/private/tmp/.../scratchpad/. Re-run outside/tmp, grok tried the edit tool,sed -i,echo >>, and creating a new file — all four returnedOperation not permitted.-wclaim. In headless mode (-pand--prompt-file) the flag is silently ignored: no worktree is created,pwdis the real checkout, edits land there, and nothing is printed to stderr. Combined with--always-approve, Update Grok review mechanics #6's recipe was strictly more dangerous than the one it replaced.What else this corrects
/reviewskill that works headless, with--local/--branch/--prmodes — the closest analogue tocodex review. The old skill asserted grok had none and hand-rolled a replacement.--tools/--disallowed-toolsfail OPEN. One unrecognized name anywhere in the list silently restores the entire toolset (exit 0, no warning). The vendor README's tool tables are wrong: it documentsbashandrun_terminal_cmd; the real tool isrun_terminal_command.--denyfails closed with a hard error, so the guard is built on deny rules.grok --sandbox read-onlyis notcodex -s read-only. Verified: codex blocks writes in both$HOMEand/tmp; grok blocks only outside its writable exceptions.grok-reviewwas false —curlreturned 200 under the read-only sandbox on macOS (seccomp is Linux-only).Design: seatbelt, not hard containment
The review guard is
--sandbox read-onlyplus repo-scopedEdit/Writedenies, with the shell left available.An earlier revision of this branch went further — broad
Edit(**)/Write(**)/Bash(*)/MCPTool(**)denies with the diff pre-materialized — to get a hard boundary that holds without any OS sandbox. That was dropped deliberately. Grok is cooperative, not adversarial; the realistic failure is it helpfully editing a file mid-review, which is visible ingit statusand revertible. Buying hard containment meant denyingBash, which meant losing/review(it needs the shell to collect the diff), hand-rolling diff materialization including a bash/zsh-only loop for untracked files, and giving up the native--branch/--prmodes. The cost outweighed the threat.Changes
CLAUDE.md— new grok Mechanics block:/review, custom-stance, and implement recipes, plus the verified traps.grok-review— delegates to the bundled/review; repo-scoped denies (a blanketWrite(**)breaks/review's notes file); custom-stance path names the target and lets grok collect the diff.grok-implementation— new, mirroringcodex-implementation. Isolates with plaingit worktree add+--cwd, generates a task branch, requires a commit SHA in the report, and integrates before cleanup —git worktree removeat the end would otherwise discard anything grok left uncommitted. Closes a coverage gap: grok-4.5 is the designated default implementer but had no skill, while the fallback (codex) did.Verification
Guard profiles were run against an adversarial canary prompt (modify a tracked file, create a new file, append via shell, write outside the repo, delegate to a subagent), with controls to prove a breach is detectable:
/tmpenforced=true)--toolswith one bad name--always-approveonlyFindings were independently reviewed by gpt-5.6-sol, which corrected two overreaches now reflected above (plan mode does not cancel all tool calls; the prompt-offload mechanism is inferred, not established).
Caveats
-wis still a silent no-op headless,--toolsstill fails open, and/review --localworks under the guard leaving the repo untouched. The rest still carry the 0.2.118 provenance.🤖 Generated with Claude Code