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:
@@ -35,8 +35,8 @@ bash "$CLAUDETOOLS_ROOT/.claude/skills/agy/scripts/ask-gemini.sh" <mode> ...
|
||||
|------|-------|--------------|
|
||||
| `text` | `ask-gemini.sh text "<prompt>"` or `text --prompt-file <path>` | 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 "<claim/finding>"` or `verify --prompt-file <path>` | Adversarial second opinion — Gemini tries to REFUTE / find gaps, returns a verdict + reasons. Pinned to the strong model. |
|
||||
| `review` | `ask-gemini.sh review <file-path> ["<instructions>"]` | 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 "<instr>"] <f1> [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 <file-path> ["<instructions>"]` | 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 "<instr>"] <f1> [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 <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`). Diff goes via the prompt file; Gemini can `read_file` changed files for full context. |
|
||||
| `image-analyze` | `ask-gemini.sh image-analyze <image-path> ["<question>"]` | **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 "<query>"` (or `search --prompt-file <path>`) | **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 <submodule-dir>`.)
|
||||
|
||||
### Model
|
||||
|
||||
- `text` uses Gemini's **default routing** (currently a flash-tier model) — fast, cheap.
|
||||
|
||||
@@ -239,6 +239,9 @@ case "$MODE" in
|
||||
[ -z "${1:-}" ] && { echo "usage: $SELF review <file-path> [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"
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user