diff --git a/memories/github.md b/memories/github.md index 11bd6506..1a88a95d 100644 --- a/memories/github.md +++ b/memories/github.md @@ -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 --json state,mergedAt`. + Verified 2026-08-09: `gh pr view --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///pulls/ --jq .merged` returns `true`, and `gh api graphql -f query='{repository(owner:"",name:""){pullRequest(number:){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. diff --git a/memories/tools.md b/memories/tools.md index 8b6e9c84..b39ec000 100644 --- a/memories/tools.md +++ b/memories/tools.md @@ -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