From 73e1a71481f58ff5a8ddb7a9954f6d781608a354 Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Sun, 9 Aug 2026 18:23:30 -0700 Subject: [PATCH 1/3] learn: gh --json merged is invalid, and zsh word-split silently breaks id loops Two cross-repo facts measured live on wai#57, 2026-08-09. memories/github.md: `gh pr view --json merged` errors (Unknown JSON field); the existing entry already covered state/mergedAt as the gh --json fix, so this adds that the merged boolean itself is real via REST and the MCP pull_request_read get method, cross-referencing github-mcp-tools.md. memories/tools.md: extends the existing zsh-does-not-word-split entry with a second variant where the unsplit loop's consumer (GraphQL) rejects the concatenated string outright, and a resolve-and-echo pattern makes the single failed call read as several partial successes -- verified by ground-truth recount, not by the loop's own printed lines. --- memories/github.md | 3 +++ memories/tools.md | 57 ++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/memories/github.md b/memories/github.md index 11bd6506..835af0e6 100644 --- a/memories/github.md +++ b/memories/github.md @@ -98,6 +98,9 @@ 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 spelling, not to the underlying data: REST's `GET /repos/{owner}/{repo}/pulls/{number}` and the GraphQL field it wraps both carry a genuine `merged` boolean (`gh api repos///pulls/ --jq .merged` returns `true`/`false` directly), and so does the GitHub MCP tool's `pull_request_read` `get` method --- 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 From 82f643063bd73f01dce80e7c44818efcc9fb6b74 Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Sun, 9 Aug 2026 18:24:40 -0700 Subject: [PATCH 2/3] fixup: split compound sentence flagged by check-new-line-breaks --- memories/github.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/memories/github.md b/memories/github.md index 835af0e6..56cee053 100644 --- a/memories/github.md +++ b/memories/github.md @@ -100,7 +100,8 @@ The GitHub MCP tool surface used in remote/web sessions lives in - **`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 spelling, not to the underlying data: REST's `GET /repos/{owner}/{repo}/pulls/{number}` and the GraphQL field it wraps both carry a genuine `merged` boolean (`gh api repos///pulls/ --jq .merged` returns `true`/`false` directly), and so does the GitHub MCP tool's `pull_request_read` `get` method --- 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. + 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. From 6838e31ea4f31525aa0fc12c9119dc7dd04533c5 Mon Sep 17 00:00:00 2001 From: Douglas Ezra Morrison Date: Sun, 9 Aug 2026 18:44:45 -0700 Subject: [PATCH 3/3] fix: name the referent instead of 'the GraphQL field it wraps' Review finding on #1371: 'it' had no unambiguous antecedent. Its nearest grammatical antecedent was the REST endpoint, which does not wrap a GraphQL field -- the two are sibling APIs. Per shared/writing/ambiguous-reference.md, the remedy is to name the referent rather than reword around the pronoun. Names GraphQL's PullRequest.merged directly, and drops the 'wraps' claim about gh's internals rather than restating it, since it was not load-bearing and I had not verified it. Added the GraphQL query as a second deriving command beside the REST one; both verified against wai#57, which returns true on each. --- memories/github.md | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/memories/github.md b/memories/github.md index 56cee053..1a88a95d 100644 --- a/memories/github.md +++ b/memories/github.md @@ -99,7 +99,9 @@ The GitHub MCP tool surface used in remote/web sessions lives in 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 spelling, not to the underlying data: REST's `GET /repos/{owner}/{repo}/pulls/{number}` and the GraphQL field it wraps both carry a genuine `merged` boolean (`gh api repos///pulls/ --jq .merged` returns `true`/`false` directly), and so does the GitHub MCP tool's `pull_request_read` `get` method --- 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. + 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.**