Skip to content

Commit 31f10c9

Browse files
committed
fix(toolkit): address dojo-code-reviewer feedback on PR #44 (DOJ-4872)
Bot review at 3/5 confidence flagged 2 P2 + 2 P3. All four addressed: P2 — commands/verify.md ref normalization - The behavior section assumed every <ref> arg was a bare branch name and unconditionally prefixed `origin/`, breaking `origin/develop` (→ `origin/origin/develop`), tags (`v1.31.0` → `origin/v1.31.0`), and SHAs. Added a normalization step at the top of the pipeline: * `refs/tags/<tag>` for tag inputs (fetched via `git fetch origin tag <tag> --quiet`) * bare SHA for already-local commits (skips fetch entirely; commits are immutable) * `origin/<branch>` left as-is when user passes `origin/...` * `origin/<branch>` only synthesized for bare branch input - Dropped the `-C "$REPO"` indirection — the slash command's cwd is the repo root, no need for the hardcoded variable that would fail when unset. - Updated the verdict-line shape to `<resolved-ref> @ <sha7>` so the citation reflects whatever normalization produced (tag refs, SHAs, branch refs all render correctly). Added tag + SHA examples. - Expanded the Edge cases section: `origin/<branch>` input, tag input, commit SHA input, ref not found. P2 — hooks/pre-tool-use-claim-verification.sh line 116 - The bare-FS detector regex `(ls|find|cat|head)[[:space:]]` failed to match the command-as-last-token cases like a bare `ls` invocation or `cd src && ls` (no trailing whitespace). Switched the trailing class to `([[:space:]]|$)` so end-of-string also matches. P3 — hooks/pre-tool-use-claim-verification.sh line 162 - Same fix in the suggester branch for `ls`: `ls[[:space:]]` → `ls([[:space:]]|$)` so the ls-specific replacement text is emitted even when the command ends in `ls`. P3 — hooks/pre-tool-use-claim-verification.sh line 181 - Same fix in the suggester branch for `find`: `find[[:space:]]` → `find([[:space:]]|$)`. Verification: - shellcheck hooks/pre-tool-use-claim-verification.sh → 0 issues - bash hooks/pre-tool-use-claim-verification.sh "ls" → warn (new) - bash hooks/pre-tool-use-claim-verification.sh "cd src && ls" → warn (new) - bash hooks/pre-tool-use-claim-verification.sh "ls src/foo" → warn (regression) - bash hooks/pre-tool-use-claim-verification.sh "echo hello" → silent (regression) - bash hooks/pre-tool-use-claim-verification.sh "find" → warn (new) Created by Claude Code on behalf of @lapc506.
1 parent 0e2af72 commit 31f10c9

2 files changed

Lines changed: 73 additions & 24 deletions

File tree

commands/verify.md

Lines changed: 70 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -35,78 +35,127 @@ branch name is passed.
3535

3636
## Behavior
3737

38-
1. **Anchor.** Print the current local checkout state so the reader knows
38+
The command executes in the current workspace (the directory the slash
39+
command was invoked from). No `-C "$REPO"` indirection — the cwd is the
40+
repo. The `<ref>` argument is normalized once at the top so the rest of the
41+
pipeline handles bare branch names, `origin/<branch>`, tags, and SHAs
42+
uniformly.
43+
44+
1. **Normalize the ref.** Decide which fully-qualified ref to query, and
45+
whether a remote fetch makes sense for that ref:
46+
47+
```bash
48+
RAW_REF="$1"
49+
FETCH_TARGET="" # what to pass to `git fetch origin <…> --quiet`; empty = skip
50+
RESOLVED_REF="" # what to pass to git show / ls-tree / grep
51+
52+
if git rev-parse --verify --quiet "refs/tags/$RAW_REF" >/dev/null; then
53+
# Annotated/lightweight tag, e.g. v1.31.0
54+
RESOLVED_REF="refs/tags/$RAW_REF"
55+
FETCH_TARGET="tag $RAW_REF"
56+
elif [[ "$RAW_REF" =~ ^[0-9a-f]{7,40}$ ]] \
57+
&& git cat-file -e "$RAW_REF" 2>/dev/null; then
58+
# Commit SHA (short or long) that already exists locally — no fetch.
59+
RESOLVED_REF="$RAW_REF"
60+
elif [[ "$RAW_REF" == origin/* ]]; then
61+
# User explicitly wrote origin/<branch> — fetch the branch part,
62+
# query the full origin/<branch> name.
63+
RESOLVED_REF="$RAW_REF"
64+
FETCH_TARGET="${RAW_REF#origin/}"
65+
else
66+
# Bare branch name (the common case) — fetch + query origin/<branch>.
67+
RESOLVED_REF="origin/$RAW_REF"
68+
FETCH_TARGET="$RAW_REF"
69+
fi
70+
```
71+
72+
2. **Anchor.** Print the current local checkout state so the reader knows
3973
where the agent was when the verdict was rendered:
4074

4175
```bash
42-
git -C "$REPO" branch --show-current
43-
git -C "$REPO" rev-parse HEAD
76+
git branch --show-current
77+
git rev-parse HEAD
4478
```
4579

46-
2. **Refresh the ref.** Always:
80+
3. **Refresh the ref** (skipped for already-local commit SHAs):
4781

4882
```bash
49-
git -C "$REPO" fetch origin "$REF" --quiet
83+
[ -n "$FETCH_TARGET" ] && git fetch origin $FETCH_TARGET --quiet
5084
```
5185

52-
3. **Resolve the ref's SHA** so the verdict can be re-checked later:
86+
4. **Resolve the ref's SHA** so the verdict can be re-checked later:
5387

5488
```bash
55-
REF_SHA="$(git -C "$REPO" rev-parse --short "origin/$REF")"
89+
REF_SHA="$(git rev-parse --short "$RESOLVED_REF")"
5690
```
5791

58-
4. **Route by argument shape.**
92+
5. **Route by argument shape.**
5993

6094
- If `<path-or-pattern>` contains a `/` and has no shell-glob characters
6195
(no `*`, `?`, `[`, no leading quote), treat it as a path:
6296

6397
```bash
64-
git -C "$REPO" ls-tree "origin/$REF" -- "$ARG"
98+
git ls-tree "$RESOLVED_REF" -- "$ARG"
6599
# If the user wants file content:
66-
git -C "$REPO" show "origin/$REF:$ARG" | head -50
100+
git show "$RESOLVED_REF:$ARG" | head -50
67101
```
68102

69103
- Otherwise, treat it as a regex/pattern to grep across `src/`:
70104

71105
```bash
72-
git -C "$REPO" grep -nF -- "$ARG" "origin/$REF" -- src/
106+
git grep -nF -- "$ARG" "$RESOLVED_REF" -- src/
73107
# If the user passes a regex (contains regex metachars), drop -F:
74-
git -C "$REPO" grep -nE -- "$ARG" "origin/$REF" -- src/
108+
git grep -nE -- "$ARG" "$RESOLVED_REF" -- src/
75109
```
76110

77-
5. **Emit a single verdict line** in this exact shape so it can be pasted
111+
6. **Emit a single verdict line** in this exact shape so it can be pasted
78112
verbatim into Linear, GitHub, or Slack:
79113

80114
```text
81-
origin/<ref> @ <sha7><path-or-pattern> <PRESENT|NOT PRESENT|N MATCHES>
115+
<resolved-ref> @ <sha7><path-or-pattern> <PRESENT|NOT PRESENT|N MATCHES>
82116
```
83117

84-
Examples:
118+
`<resolved-ref>` is whatever the normalization step produced —
119+
`origin/<branch>` for bare/branch input, `origin/origin-typo-guarded` for
120+
`origin/…` input (never doubled), `refs/tags/v1.31.0` for tag input, the
121+
bare SHA for SHA input. Examples:
85122

86123
```text
87124
origin/develop @ c513198 — src/pages/PathDetailPage.tsx PRESENT
88125
origin/develop @ c513198 — src/hooks/path/usePathwayViewModel.ts NOT PRESENT
89126
origin/main @ a3f9c21 — buildPathwayUrl 7 MATCHES (src/utils/url.ts, src/hooks/...)
127+
refs/tags/v1.31.0 @ 0e2af72 — hooks/pre-tool-use-claim-verification.sh PRESENT
128+
c513198 @ c513198 — README.md PRESENT
90129
```
91130

92-
6. **Never read the working tree** to answer this question. No `ls`, no
131+
7. **Never read the working tree** to answer this question. No `ls`, no
93132
`grep -rn src/`, no `find`. If `git cat-file -e` and a stray `ls`
94133
disagree, **the ref wins**.
95134

96135
## Edge cases
97136

98-
- **Ref not found / no upstream:** print `origin/<ref> NOT FOUND — did you
99-
mean origin/main?` and exit with the suggestion. Don't fall back to the
100-
working tree.
137+
- **`origin/<branch>` input (already qualified):** the normalization step
138+
detects the `origin/` prefix and queries `origin/<branch>` as-is — no
139+
doubling like `origin/origin/develop`.
140+
- **Tag input (`v1.31.0`, etc.):** normalized to `refs/tags/<tag>`. The
141+
fetch uses `git fetch origin tag <tag> --quiet` to refresh the tag ref
142+
specifically (without pulling other refs).
143+
- **Commit SHA input (7-40 hex chars):** normalized to the bare SHA. Skips
144+
the fetch when the commit already exists locally; if the SHA is unknown
145+
locally, falls through to the bare-branch branch and lets the fetch
146+
surface the error.
147+
- **Ref not found / no upstream:** print `<resolved-ref> NOT FOUND — did
148+
you mean origin/main?` and exit with the suggestion. Don't fall back to
149+
the working tree.
101150
- **Path with shell metacharacters:** quote `$ARG`. Treat as path if it
102151
contains `/`, otherwise as a pattern.
103152
- **Multiple worktrees:** the command operates in the worktree it's invoked
104-
from but always queries `origin/<ref>` — the working tree's branch is
153+
from but always queries the resolved ref — the working tree's branch is
105154
irrelevant. If the user is in a worktree on a different branch, the
106155
result is still authoritative for the named ref.
107156
- **Squash-merged PRs:** `origin/<branch>` is rewritten on squash-merge; a
108157
pre-merge `git fetch` you ran 10 minutes ago is stale. The command always
109-
re-fetches.
158+
re-fetches (except for already-local commit SHAs, which are immutable).
110159
111160
## Why
112161

hooks/pre-tool-use-claim-verification.sh

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -106,7 +106,7 @@ is_bare_fs_op() {
106106
# after a command separator (; | && ||). Use ERE so we can express the
107107
# leading-position constraint without shellcheck flagging redundant case
108108
# globs (SC2221/SC2222).
109-
if printf '%s' "$cmd" | grep -qE '(^|[;&|][[:space:]]*)(ls|find|cat|head)[[:space:]]'; then
109+
if printf '%s' "$cmd" | grep -qE '(^|[;&|][[:space:]]*)(ls|find|cat|head)([[:space:]]|$)'; then
110110
return 0
111111
fi
112112
if printf '%s' "$cmd" | grep -qE 'grep[[:space:]]+-[rRnliN]+'; then
@@ -153,7 +153,7 @@ fi
153153

154154
# Best-effort suggested replacement, branched by detected op.
155155
SUGGESTED=""
156-
if printf '%s' "$COMMAND" | grep -qE '(^|[;&|][[:space:]]*)ls[[:space:]]'; then
156+
if printf '%s' "$COMMAND" | grep -qE '(^|[;&|][[:space:]]*)ls([[:space:]]|$)'; then
157157
# Extract the path argument (best-effort, first non-flag token after ls).
158158
path_arg="$(printf '%s' "$COMMAND" \
159159
| sed -E 's/^.*ls[[:space:]]+(-[A-Za-z]+[[:space:]]+)*//' \
@@ -178,7 +178,7 @@ elif printf '%s' "$COMMAND" | grep -qE 'grep[[:space:]]+-[rRnliN]+'; then
178178
else
179179
SUGGESTED="git fetch origin <branch> --quiet && git grep \"<pattern>\" origin/<branch> -- src/"
180180
fi
181-
elif printf '%s' "$COMMAND" | grep -qE '(^|[;&|][[:space:]]*)find[[:space:]]'; then
181+
elif printf '%s' "$COMMAND" | grep -qE '(^|[;&|][[:space:]]*)find([[:space:]]|$)'; then
182182
SUGGESTED="git fetch origin <branch> --quiet && git ls-tree -r origin/<branch> -- <path> | grep <pattern>"
183183
else
184184
SUGGESTED="git fetch origin <branch> --quiet && git show origin/<branch>:<path>"

0 commit comments

Comments
 (0)