docs(skills): document review path-resolution gotcha in agy + grok

review/review-files resolve relative paths only against CWD or
$CLAUDETOOLS_ROOT, never a submodule/subdir — so submodule-relative
paths fail with "file not found". Add a [!WARNING] callout to both
SKILL.md files, fix the misleading "absolute or repo-relative" table
wording, and add inline GOTCHA comments at each resolution site in
both scripts. Bitten us repeatedly (latest: GuruConnect review).
This commit is contained in:
2026-06-05 16:55:56 -07:00
parent 8b82e28ed6
commit 2795fab3c7
4 changed files with 44 additions and 8 deletions

View File

@@ -32,8 +32,8 @@ bash "$CLAUDETOOLS_ROOT/.claude/skills/grok/scripts/ask-grok.sh" <mode> ...
|------|-------|--------------|
| `text` | `ask-grok.sh text "<prompt>"` or `text --prompt-file <path>` | One-shot text answer (independent model). `--prompt-file` for long content (review/summarize a doc). |
| `verify` | `ask-grok.sh verify "<claim/finding>"` or `verify --prompt-file <path>` | Adversarial second opinion — Grok tries to REFUTE/find gaps, returns a verdict + reasons. |
| `review` | `ask-grok.sh review <file-path> ["<instructions>"]` | Grok reads the file at `<path>` itself (its `read_file` tool) and reviews it — no embedding, handles large files, can pull in referenced files. Accepts absolute or repo-relative paths, and paths with spaces. |
| `review-files` | `ask-grok.sh review-files [-i "<instr>"] <f1> [f2 …]` | Review a **set** of files together (grok `read_file`s each) — for cross-file consistency or a multi-file change. Paths absolute or repo-relative; spaces OK. No code passed as a shell arg → no quote hell. |
| `review` | `ask-grok.sh review <file-path> ["<instructions>"]` | Grok reads the file at `<path>` itself (its `read_file` tool) and reviews it — no embedding, handles large files, can pull in referenced files. Path resolution: absolute, CWD-relative, or relative to `$CLAUDETOOLS_ROOT`**see the path gotcha below**. Spaces OK. |
| `review-files` | `ask-grok.sh review-files [-i "<instr>"] <f1> [f2 …]` | Review a **set** of files together (grok `read_file`s each) — for cross-file consistency or a multi-file change. Same path resolution as `review` (**see gotcha below**); spaces OK. No code passed as a shell arg → no quote hell. |
| `review-diff` | `ask-grok.sh review-diff [-C <repo-dir>] [-i "<instr>"] <gitref> [-- <pathspec>]` | Review a **git diff** (`git diff <gitref>` from `<repo-dir>`; default repo root, use `-C` for a submodule e.g. `-C projects/msp-tools/guru-rmm`). The diff goes via the prompt file (not a shell arg); grok can `read_file` changed files for full context (cwd = repo dir). |
| `image` | `ask-grok.sh image "<prompt>" [out.png]` | `image_gen` (Imagine) → copies the artifact to `out` (default `grok-image.png`). |
| `video` | `ask-grok.sh video "<motion prompt>" <input-image> [out.mp4]` | `image_to_video` on an input image → copies to `out`. ~60-90s. |
@@ -46,6 +46,18 @@ media **retrieves the artifact by sessionId** from
recovered even when a headless run reports `stopReason: Cancelled` before echoing
the path (a known finalization quirk of the `-p` mode).
> [!WARNING]
> **Path gotcha for `review` / `review-files` (this has bitten us repeatedly).**
> A relative path is resolved against ONLY two roots: your **current directory**,
> and **`$CLAUDETOOLS_ROOT`** (`/d/claudetools`). It is NOT resolved against a
> submodule or any arbitrary subdir. So a path like `server/src/api/auth.rs` that
> is relative to a submodule (e.g. `projects/msp-tools/guru-connect/`) fails with
> `file not found` whenever your CWD isn't that submodule — even though the file
> obviously exists. **When reviewing files in a submodule or any non-root subtree,
> pass ABSOLUTE paths** (e.g. build the list with `find "$(pwd)/server/src" -name '*.rs'`
> from inside the submodule). Absolute paths always work regardless of CWD and
> tolerate spaces. (For `review-diff`, the analogous fix is `-C <submodule-dir>`.)
## Machine availability (fleet)
Grok is **per-machine** — the skill syncs fleet-wide but the binary does not. Availability is gated by `identity.json` (per-machine, gitignored):

View File

@@ -207,8 +207,11 @@ case "$MODE" in
target="$1"
instr="${2:-Give an independent, critical review of this file: accuracy, gaps/omissions, and concrete improvements. Be specific.}"
# Grok reads the file itself (no embedding). Resolve to an absolute path (as given, or
# repo-relative), then hand grok the native-Windows ABSOLUTE path so read_file works
# regardless of cwd, and tolerates absolute paths and spaces.
# relative to $REPO_ROOT), then hand grok the native-Windows ABSOLUTE path so read_file
# works regardless of cwd, and tolerates absolute paths and spaces.
# GOTCHA: a relative path resolves against ONLY CWD or $REPO_ROOT ($CLAUDETOOLS_ROOT) --
# NOT a submodule/subdir. "server/src/x.rs" relative to a submodule fails unless CWD is
# that submodule. Pass ABSOLUTE paths for submodule/subtree files.
if [ -f "$target" ]; then resolved="$target"
elif [ -f "$REPO_ROOT/$target" ]; then resolved="$REPO_ROOT/$target"
else echo "[$SELF] file not found: $target" >&2; exit 2; fi
@@ -234,8 +237,11 @@ case "$MODE" in
;;
review-files)
# review-files [-i "instructions"] <file> [file ...]
# Reviews a SET of files together (grok read_file's each). Paths may be absolute or
# repo-relative; spaces are fine. No code is passed as a shell arg -> no quote hell.
# Reviews a SET of files together (grok read_file's each). Paths may be absolute,
# CWD-relative, or relative to $REPO_ROOT ($CLAUDETOOLS_ROOT); spaces are fine.
# GOTCHA: a relative path is NOT resolved against a submodule/subdir -- "server/src/x.rs"
# relative to a submodule fails ("file not found") unless CWD is that submodule. Pass
# ABSOLUTE paths for submodule/subtree files. No code passed as a shell arg -> no quote hell.
instr='Independently review these files together as a unit: correctness/bugs, gaps, cross-file consistency, and concrete improvements. Be specific and cite file:line.'
files=()
while [ $# -gt 0 ]; do