chore(ci): Claude Code review - keep settled threads out of the subagent

[ovr]

The review-thread subagent was reading everything a PR had accumulated: gh list-review-threads returned resolved threads, whose every rule (resolve a stale thread, skip a duplicate finding) is gated on the thread being unresolved, so they were bodies it read only to discard — on #10604, 71 threads over 2 pages and 117KB, where 15 threads and 13KB are actionable. The alias now filters them out, pages at 100 (the GraphQL maximum, which the filter makes affordable) and inlines up to 25 comments per thread instead of only the opening one, with show-review-thread <id> <endCursor> continuing past 25; across ten sampled PRs no unresolved thread had a reply at all, so the chains this carries are the ones worth reading — a thread still open because a human pushed back. review-threads.md also tells the subagent not to go after the PR's top-level comments: our own tracking comments are there, each restating a whole past review round, and none of them say whether a thread is live.

Verified live against #10604, #11881 and #11859: filtered listing, thread paging, comment-cursor continuation (0 nodes past the last comment, not a re-read), and the jq shape on a synthetic three-comment thread. The workflow file ends up unchanged.

Check List

• Tests have been run in packages where changes have been made if available
• Linter has been run for changed code
• Tests for the changes have been added if not covered yet
• Docs have been added / updated if required

🤖 Generated with Claude Code

[claude]

Claude finished @ovr's task in 1m 54s —— View job

---

Review complete — 0 high, 0 medium, 3 low

Findings and checklist

• Read the PR diff against origin/master and both changed files in full
• Cross-checked the doc against the alias it documents
• Checked the --jq filter semantics and paging contract
• Checked thread hygiene (no review threads exist on this PR — no-op)
• Posted findings

The change is sound and the two files agree with each other: the doc's description of the listing output (id, isOutdated, path, line, originalLine, comment chain, comments.totalCount, comments.pageInfo) matches what the jq filter emits after del(.isResolved), and both isResolved gates were correctly dropped from the rules since the field is no longer there to test. The warning that the filter runs after the page is cut — so a page can come back with two nodes or none and still have a next one — is the non-obvious consequence of post-filtering and it is the right thing to have written down. Aliases pass arguments through gh's -F, not the shell, so the added $2 cursor is not an injection surface.

Low

1. Stale argument name in the script header — .github/actions/setup-claude-code-review.sh:7. Inline comment posted. Line 8 gained [comment-cursor] and the doc renames this one [thread-cursor]; the header is now the only place the two cursors share a name.

2. Wrong PR number fails as a jq error — .github/actions/setup-claude-code-review.sh:40-41. Inline comment posted. pullRequest is null for a nonexistent number, so .nodes[] aborts with Cannot iterate over null (exit 5) where the unfiltered response used to read as "no threads".

3. The worst case moved, and the doc does not bound it. reviewThreads(first: 100) × comments(first: 25) is a 2500-comment-node ceiling per page against the old 50 opening comments. The ten-PR sample ("no unresolved thread had a reply at all") is what makes this affordable today, but it is a sample, not a bound — a PR that accumulates many unresolved threads with real back-and-forth would hand the subagent a page an order of magnitude larger than anything the old alias could produce, and the filter cannot help because those threads are exactly the unresolved ones. Nothing to change in this PR; worth knowing that the bad case is now "many unresolved threads with replies" rather than "many threads".

Note on the PR description

The last paragraph still says mcp__github__get_issue_comments is dropped from allowedTools, but 5df887c put it back (.github/workflows/claude-code-review.yml:111 still lists it) and that file is not among this PR's changed files. The guidance moved into .claude/docs/review-threads.md:24-28 instead — which is the better place for it, since it explains why the tracking comment is useless to the subagent rather than just making it unreachable. Worth updating the description so the merge commit does not claim a change it no longer makes.

· branch `split-tracking-comments-from-review`

[claude]

Claude finished @ovr's task in 4m 2s —— View job

---

Approve — no findings. Verified the three aliases live; one prior finding of mine withdrawn.

Review detail

• Read the diff and both changed files in full
• Verify the alias behaviour live (#10604 listing, thread cursor continuation, error surfacing)
• Check for stale references to the old alias contract elsewhere in .claude / workflow
• Review prior review threads on this PR
• Post findings

Behaviour checks against the real API

Every claim the PR body and .claude/docs/review-threads.md make about the new aliases holds:

Check	Result
gh list-review-threads cube-js cube 10604	one page, hasNextPage: false, 15 nodes, 18 KB (was 71 threads / 2 pages / 117 KB)
isResolved stripped from output	yes — del(.isResolved) leaves only id, isOutdated, path, line, originalLine, comments
author.login for the bot	claude, matching what review-threads.md:18 tells the subagent to match on
gh show-review-thread <id> <endCursor>	continues past the last comment — nodes: [], hasNextPage: false, endCursor: null; not a re-read
gh show-review-thread <id> (no cursor)	re-reads from the first comment, as documented
100 × 25 node cost	well inside the GraphQL node limit; the #10604 call (71 unresolved+resolved threads) returned without a timeout

On #10604 no unresolved thread had comments.totalCount > 1, so the 25-comment inlining costs nothing there and the show-review-thread cursor path only engages on genuinely long chains — consistent with the sampling in the PR body.

Prior thread

.github/actions/setup-claude-code-review.sh:41 — PRRT_kwDOCOH19M6ilF9e, my earlier claim that .nodes[] needs a null guard. @ovr's rebuttal is correct and I'm withdrawing the finding. GraphQL returns a 200 with an errors array, and gh fails on it before --jq is applied:

$ gh list-review-threads cube-js cube 99999999
{"data":{"repository":{"pullRequest":null}},"errors":[{"type":"NOT_FOUND","path":["repository","pullRequest"],
 "message":"Could not resolve to a PullRequest with the number of 99999999."}]}
gh: Could not resolve to a PullRequest with the number of 99999999.
exit 1

Same for a bad thread id through show-review-thread. jq never sees the null, so there is no Cannot iterate over null path to guard. Left the thread unresolved rather than closing it myself — the rules keep me off a thread a human has replied on.

Notes, nothing to change

• Dropping the four-line paging rationale above the alias is the right call — review-threads.md is where the subagent reads it, and the script no longer had to keep a second copy in sync. The one line that stayed (setup-claude-code-review.sh:44) is the one a reader couldn't reconstruct: why a second alias exists at all.
• The filter is client-side, so a #10604-sized PR still transfers every resolved body over the wire; only the subagent's context is saved. That's the cost that mattered, and the doc doesn't overclaim it.
• show-review-thread still returns isResolved while the listing strips it. Deliberate asymmetry and useful — a thread fetched by id is the one case where you didn't get resolution state implicitly.
• No stale references to the old contract anywhere else: .claude/commands/cube-review.md:84 only @-includes the doc, and the workflow's --allowedTools patterns (Bash(gh show-review-thread:*)) already admit the new second argument.

· branch [`split-tracking-comments-from-review`](https://github.com/cube-js/cube/tree/split-tracking-comments-from-review)

[claude]

Claude finished @ovr's task in 5m 7s —— View job

---

Review: 1 medium, 1 nit — the aliases behave as described; one rule the run has no tool for

Full review

• Read the diff and both changed files in full
• Verified the aliases live against chore(ci): Claude Code review - keep settled threads out of the subagent #11888 and feat: AST SQL rewriting #10604
• Checked review-threads.md rules for internal consistency
• Handled prior review threads
• Posted findings

Medium — .claude/docs/review-threads.md:92

Resolve it and say in the reply that you are withdrawing it has no tool behind it. Nothing in claude-code-review.yml:111 writes into an existing review thread: create_inline_comment has no in_reply_to (its schema is path/line/startLine/side/commit_id/body), gh pr comment is top-level, the three aliases are list/show/resolve, and raw gh api graphql is denied. Details and two fix directions in the inline comment.

Nit — .github/actions/setup-claude-code-review.sh:29

comments.totalCount no longer drives a rule. review-threads.md:59 replaced it with comments.pageInfo.hasNextPage ("You are missing part of it only where comments.pageInfo.hasNextPage is true"), yet the field is still selected, still documented at review-threads.md:42, and costs a line per thread in the payload this PR exists to trim. Same for the totalCount added to show-review-thread at line 58. Keep it if you want the agent to see how much it would be paging into; otherwise it is dead weight.

Verified live

Ran the installed aliases from this job:

check	result
gh list-review-threads cube-js cube 10604	one page, hasNextPage: false, 15 nodes; keys exactly comments, id, isOutdated, line, originalLine, path — no isResolved
same, with the returned endCursor as 4th arg	{"nodes":[],"pageInfo":{"endCursor":null,"hasNextPage":false}}
gh show-review-thread <id>	full thread, --jq '.data.node' unwrapping correct
gh show-review-thread <id> <comments.pageInfo.endCursor>	nodes: [], hasNextPage: false — continues past the last comment, not a re-read
base64 cursor with == padding through -F	passed through as a String, not coerced

So the filter, the del(.isResolved), the two cursor arguments and the 100/25 page sizes all do what review-threads.md says they do. The 71-of-which-15 ratio on #10604 is the claimed one.

Rules consistency

No dangling isResolved references survive the removal of the field — grep over the committed file finds only prose uses. The two new caveats are the ones that actually bite and both are correct: the top-level pageInfo is emitted before the jq filter, so a page can come back with zero nodes and still have a next one (review-threads.md:53-55), and the endCursor to disambiguate is the one beside nodes, not a thread's own (:46-48).

Review threads

gh list-review-threads cube-js cube 11888 returns {"nodes":[],"pageInfo":{...,"hasNextPage":false}} — no unresolved threads on this PR, so nothing to resolve and no finding to suppress as a duplicate. I skipped the subagent for that reason rather than spawning one to read an empty list.

I did not re-raise the --jq-on-null finding from the earlier round: @ovr's refutation on setup-claude-code-review.sh:41 is correct — gh exits 1 on the GraphQL errors entry before --jq is applied, so there is no silent empty reading to lose and (.nodes // [])[] would only mask a legible error.

Not run

No tests or linter — the change is a bash alias-installer and a markdown doc, neither covered by a package test suite, and yarn lint does not reach .claude/ or .github/. bash -n on the installer is implicitly clean since the job's setup step ran it and the aliases work.

· [branch](https://github.com/cube-js/cube/tree/split-tracking-comments-from-review)

[claude]

Reviewing

• Read PR diff
• Read changed files in full
• Verify alias behaviour
• Check prior review threads
• Post findings

View job run