Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions memories/github.md
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,12 @@ The GitHub MCP tool surface used in remote/web sessions lives in
That is worse than the inference case above, because the claim is now published prose a later reader inherits, and "a human already verified this" is precisely the sentence that stops the next person checking.
Correct it in the thread when you see it, naming which account is actually a session identity; don't let it stand just because the surrounding verdict was clean. (`ucdavis/bcs#532`, 2026-07-31: a `claude-review` pass reported a fix as human-confirmed when `dem-extra1` was the Claude session that made it, and no human had touched the PR at that point.)
- **`gh pr view --json` does not accept `merged` as a field.** Use `state` (returns `"MERGED"`) and `mergedAt` (ISO timestamp, null if not merged) to check merge status. Example: `gh pr view <N> --json state,mergedAt`.
Verified 2026-08-09: `gh pr view <N> --json merged` fails with `Unknown JSON field: "merged"` and prints the full valid field list (`gh` 2.96.0), which includes `state`, `mergedAt`, `mergedBy`, `mergeCommit`, `closed`, and `closedAt` --- no bare `merged`.
That absence is specific to `gh --json`'s own field-name allowlist, not to the underlying data.
REST's `GET /repos/{owner}/{repo}/pulls/{number}` and the GraphQL `PullRequest.merged` field each carry a genuine `merged` boolean, verified 2026-08-09 against `Morrison-Lab/wai#57`: `gh api repos/<o>/<r>/pulls/<N> --jq .merged` returns `true`, and `gh api graphql -f query='{repository(owner:"<o>",name:"<r>"){pullRequest(number:<N>){merged}}}'` returns `true` as well.
The GitHub MCP tool's `pull_request_read` `get` method carries it too --- see [`github-mcp-tools.md`](github-mcp-tools.md)'s note that `list_pull_requests` reports `merged: false` for every PR while `pull_request_read` `get` reports it correctly.
So the fix differs by surface.
Under `gh --json`, read `state`/`mergedAt`; under REST or the MCP `get` method, the `merged` field itself already works.
**Never compare that `mergedAt` against a git timestamp as strings --- convert both to epochs first.**
Every GitHub API timestamp is UTC (`...Z`), while git's `%cI`/`%cd` render in the *machine's local zone*, so a lexicographic `<` between them compares clock faces from two different zones and silently answers wrong.
It fails in the unsafe direction west of UTC: a commit made *after* the merge still sorts first.
Expand Down
57 changes: 57 additions & 0 deletions memories/tools.md
Original file line number Diff line number Diff line change
Expand Up @@ -616,6 +616,63 @@ The second time, a branch sweep reported all 19 local branches as having no
PR; the immediately preceding run of the same data had correctly shown 16 as
`MERGED`, which is the only reason the contradiction was noticed at all.)

**A second variant: the consumer can reject the unsplit string outright, and
the loop's own progress output can then read as partial success.**
The case above assumes the downstream consumer tolerates the unsplit string
silently --- a `[ "$2" = "MERGED" ]` test just goes false.
A stricter consumer does not.
Collecting several GraphQL node ids into one variable and looping over them
unquoted --- `IDS=$(gh api graphql ... --jq '...|.id')` then
`for id in $IDS; do ...; done` --- runs the loop body exactly **once**, with
every id joined by the embedded newlines from `$IDS`, because zsh never split
it.
GraphQL rejects that single malformed id outright, so the call errors instead
of going quiet:

```
Could not resolve to a node with the global id of 'PRRT_kwDOTYdrl86Xsy0A
PRRT_kwDOTYdrl86Xsy1o
...'
```

The dangerous part is not the error.
It is that a resolve-and-echo pattern inside the loop body can print a
per-item success line for each id the error message happened to name, so the
transcript reads like several calls each partly succeeded, when only one call
ever ran and it failed.
The tell is a ground-truth count that does not move: re-querying the number
of items still needing resolution, rather than trusting the loop's own
printed lines, is what actually caught it.

Pipe multi-line command output into `while read -r`, never into an unquoted
`for` loop, for exactly this idiom:

```zsh
gh api graphql -f query='...' --jq '...|.id' |
while read -r id; do
[ -z "$id" ] && continue
gh api graphql -f t="$id" -f query='...'
done
```

- **Do:** pipe multi-line command output into `while read -r`, not into an
unquoted `for x in $var` loop.
- **Do:** re-query a ground-truth count (unresolved threads, open items)
after a loop like this, rather than trusting its own printed progress
lines.
- **Don't:** read a script's per-item success echoes as evidence a
multi-step loop ran more than once; the unsplit-variable failure can make
one failed call look like several partial successes.

(2026-08-09, resolving PR review threads on
[`Morrison-Lab/wai#57`](https://github.com/Morrison-Lab/wai/pull/57):
the `for id in $IDS` loop above ran once over all five thread ids
concatenated by embedded newlines, and the GraphQL error it produced named
all five in one string.
The loop's per-line echo made the transcript read as four of five threads
resolved; the unresolved-thread count, re-queried independently, was
unchanged at 5.)

## `grep` in a Claude Code session is a shell function, so a script gets a different program

Sibling of the entry above: another case where the harness's shell is not the
Expand Down
Loading