From ec411f44bc1a7f799305a26c17a0c4294ed9e555 Mon Sep 17 00:00:00 2001 From: Mike Swanson Date: Fri, 5 Jun 2026 16:55:56 -0700 Subject: [PATCH] docs(skills): document review path-resolution gotcha in agy + grok MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- .claude/skills/agy/SKILL.md | 16 ++++++++++++++-- .claude/skills/agy/scripts/ask-gemini.sh | 6 ++++++ .claude/skills/grok/SKILL.md | 16 ++++++++++++++-- .claude/skills/grok/scripts/ask-grok.sh | 14 ++++++++++---- 4 files changed, 44 insertions(+), 8 deletions(-) diff --git a/.claude/skills/agy/SKILL.md b/.claude/skills/agy/SKILL.md index caaf31c..ae4eccf 100644 --- a/.claude/skills/agy/SKILL.md +++ b/.claude/skills/agy/SKILL.md @@ -35,8 +35,8 @@ bash "$CLAUDETOOLS_ROOT/.claude/skills/agy/scripts/ask-gemini.sh" ... |------|-------|--------------| | `text` | `ask-gemini.sh text ""` or `text --prompt-file ` | One-shot text answer from an independent model. `--prompt-file` for long content (review/summarize a doc). Default model routing. | | `verify` | `ask-gemini.sh verify ""` or `verify --prompt-file ` | Adversarial second opinion — Gemini tries to REFUTE / find gaps, returns a verdict + reasons. Pinned to the strong model. | -| `review` | `ask-gemini.sh review [""]` | Gemini reads the file itself (its `read_file` tool, read-only `plan` mode) and reviews it. Accepts absolute or repo-relative paths, and paths with spaces. Works even on gitignored files. | -| `review-files` | `ask-gemini.sh review-files [-i ""] [f2 …]` | Review a **set** of files together (cross-file consistency, multi-file change). Paths absolute or repo-relative; spaces OK. No code passed as a shell arg. | +| `review` | `ask-gemini.sh review [""]` | Gemini reads the file itself (its `read_file` tool, read-only `plan` mode) and reviews it. Path resolution: absolute, CWD-relative, or relative to `$CLAUDETOOLS_ROOT` — **see the path gotcha below**. Spaces OK. Works even on gitignored files. | +| `review-files` | `ask-gemini.sh review-files [-i ""] [f2 …]` | Review a **set** of files together (cross-file consistency, multi-file change). Same path resolution as `review` (**see gotcha below**); spaces OK. No code passed as a shell arg. | | `review-diff` | `ask-gemini.sh review-diff [-C ] [-i ""] [-- ]` | Review a **git diff** (`git diff ` from ``; default repo root, use `-C` for a submodule e.g. `-C projects/msp-tools/guru-rmm`). Diff goes via the prompt file; Gemini can `read_file` changed files for full context. | | `image-analyze` | `ask-gemini.sh image-analyze [""]` | **Vision** — Gemini `read_file`s the image and describes/answers about it. Pins the **pro vision model** (the default flash-lite router hallucinates image content). Path absolute or repo-relative; spaces OK. KEYLESS (works on OAuth). | | `search` | `ask-gemini.sh search ""` (or `search --prompt-file `) | **Live Google web search** (sibling of `grok xsearch`) — Gemini uses its `google_web_search` tool and returns the answer **with source URLs**. KEYLESS (works on OAuth). | @@ -47,6 +47,18 @@ The script runs Gemini headless with `-o json`, extracts the answer from ignored), and keeps stderr separate from the JSON so 429-backoff / warning noise never corrupts the parse. +> [!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 `.) + ### Model - `text` uses Gemini's **default routing** (currently a flash-tier model) — fast, cheap. diff --git a/.claude/skills/agy/scripts/ask-gemini.sh b/.claude/skills/agy/scripts/ask-gemini.sh index 5c1f039..079cdee 100644 --- a/.claude/skills/agy/scripts/ask-gemini.sh +++ b/.claude/skills/agy/scripts/ask-gemini.sh @@ -239,6 +239,9 @@ case "$MODE" in [ -z "${1:-}" ] && { echo "usage: $SELF review [instructions]" >&2; exit 2; } target="$1" instr="${2:-Give an independent, critical review of this file: accuracy, gaps/omissions, bugs, and concrete improvements. Be specific.}" + # 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 ("file not found") + # 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 @@ -265,6 +268,9 @@ case "$MODE" in prep_includes list="" declare -A seen=() + # GOTCHA: each relative path resolves against ONLY CWD or $REPO_ROOT ($CLAUDETOOLS_ROOT) -- + # NOT a submodule/subdir. Paths relative to a submodule fail unless CWD is that submodule. + # Pass ABSOLUTE paths for submodule/subtree files (e.g. build the list with `find "$(pwd)/..."`). for f in "${files[@]}"; do if [ -f "$f" ]; then r="$f" elif [ -f "$REPO_ROOT/$f" ]; then r="$REPO_ROOT/$f" diff --git a/.claude/skills/grok/SKILL.md b/.claude/skills/grok/SKILL.md index 35138bf..cc042c8 100644 --- a/.claude/skills/grok/SKILL.md +++ b/.claude/skills/grok/SKILL.md @@ -32,8 +32,8 @@ bash "$CLAUDETOOLS_ROOT/.claude/skills/grok/scripts/ask-grok.sh" ... |------|-------|--------------| | `text` | `ask-grok.sh text ""` or `text --prompt-file ` | One-shot text answer (independent model). `--prompt-file` for long content (review/summarize a doc). | | `verify` | `ask-grok.sh verify ""` or `verify --prompt-file ` | Adversarial second opinion — Grok tries to REFUTE/find gaps, returns a verdict + reasons. | -| `review` | `ask-grok.sh review [""]` | Grok reads the file at `` 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 ""] [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 [""]` | Grok reads the file at `` 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 ""] [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 ] [-i ""] [-- ]` | Review a **git diff** (`git diff ` from ``; 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 "" [out.png]` | `image_gen` (Imagine) → copies the artifact to `out` (default `grok-image.png`). | | `video` | `ask-grok.sh video "" [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 `.) + ## 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): diff --git a/.claude/skills/grok/scripts/ask-grok.sh b/.claude/skills/grok/scripts/ask-grok.sh index 239caf5..cd68149 100644 --- a/.claude/skills/grok/scripts/ask-grok.sh +++ b/.claude/skills/grok/scripts/ask-grok.sh @@ -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 ...] - # 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