fix: every listing read in the tracker walks the whole listing (#308) - #312
Conversation
There was a problem hiding this comment.
Approved.
I found no correctness issues. The diff paginates Comments, SubIssues, listIssues/MilestoneIssues, and OpenPRsForBranch, while leaving RecentIssues and TaskBranches as deliberate one-page reads with code comments matching their contracts.
I verified gh api --paginate against a forced multi-page REST array endpoint, including the -X GET -f per_page=1 form used by OpenPRsForBranch; it produced a single JSON array, so gh.JSON reads all pages rather than silently decoding only a first array. The new tests drive listings past one page and assert the last item arrives, with Comments also preserving oldest-first order and latest-wins verdict behavior.
Record checks passed: the task has a plan and decision comments, closingIssuesReferences lists #308 only, the changelog entry is under Unreleased and ends (#308), and both commit subjects are conventional, lowercase after the type, under 100 characters, and reference (#308). origin/main is an ancestor of this branch; the fetched #309 task branch has no diff from origin/main in this clone.
Verification run:
go test ./...go vet ./...gofmt -l .go build -o ./gh-codecrew ./cmd/codecrew./gh-codecrew status./gh-codecrew milestone close M15 --dry-run(stopped at the expectedOPEN_TASKSrefusal and wrote nothing)
Comments, SubIssues and the issue listing behind MilestoneIssues stopped at GitHub's first hundred rows, so a milestone issue past a hundred comments lost its newest ones — the comments latest-wins reads — and milestone close refused VERDICT_MISSING for a requirement whose satisfied verdict sat on a later page (#264). Those three and OpenPRsForBranch now carry --paginate, which gh joins into one JSON array in request order, so oldest-first order survives the page boundary. Two reads stay on one page deliberately, each said so in a comment: RecentIssues wants the newest page and only that (the created-issue floor, #195), and TaskBranches reports hasNextPage instead, which the sweep's "swept in part" is built on (SPEC §6). The gh fake in the tracker tests now pages the way the real gh does — the whole listing only when the call carried --paginate — so a reader that drops the flag fails the suite instead of passing on a fixture that fits in one page. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr
9b313b6
e4a31d0 to
9b313b6
Compare
There was a problem hiding this comment.
Approved rebase check.
Round one approved e4a31d0; the rebased head is 9b313b6 on base 8f9d9e9. I compared the PR patch before and after the rebase excluding CHANGELOG.md; the stable patch IDs match, and a direct old-head to new-head content comparison outside CHANGELOG.md differs only in the #307 mainline files (SPEC.md, docs/identities.md, internal/cli/status.go, internal/cli/status_test.go, internal/cli/task.go, internal/cli/task_test.go). The PR's tracker diff is unchanged.
CHANGELOG.md keeps both the #308 and #307 Unreleased entries. closingIssuesReferences is still #308 only. The two rebased commit messages are unchanged from the approved head, conventional, lowercase after the type, under 100 characters, and reference (#308).
Verified on 9b313b6: go test ./..., go vet ./..., gofmt -l ., go build -o ./gh-codecrew ./cmd/codecrew, ./gh-codecrew status, and ./gh-codecrew milestone close M15 --dry-run (expected OPEN_TASKS refusal with dry run: nothing written).
Closes #308. Requirement: M15-R3. Adopts the capture under
## Adoptson the task.What was wrong
Four listing reads in
internal/tracker/github.goasked forper_page=100and read whatever came back in one response — GitHub's first page. The sharpest consequence is the one the capture named:Commentsreturns the oldest-first comment listing, so an issue past a hundred comments loses its newest comments, which are exactly the ones latest-wins reads. On a long milestone issuemilestone closewould refuseVERDICT_MISSINGfor a requirement whose satisfied QA verdict sat on page two, andstatuswould show a superseded verdict as current.What changed
Comments,SubIssues, the issue listing behindMilestoneIssues/listMilestones, andOpenPRsForBranchnow carry--paginate. gh follows theLinkheader and joins a REST array endpoint's pages into one JSON array, in request order — sogh.JSONunmarshals the whole listing unchanged (the mechanismLabelshas used since it was paginated), andCommentsstays oldest-first across the page boundary. I checked the join against this repo's ownlabelsandpullsendpoints on gh 2.100 before relying on it, including the-X GET -f …formOpenPRsForBranchbuilds.listIssuessplit in two, because one caller does not want the walk:listIssuespaginates, andissueListing(repo, query, paginate bool)is the shared body.The two deliberate exceptions, each stated in a code comment
RecentIssuesreads the newest page and only that. It is the created-issue floor of #195 — a second source for a number, not a set that must be complete — and walking a repo's entire issue history to raise it would cost a request per hundred issues for nothing. The test asserts it does not paginate, so the exception is guarded rather than merely commented.TaskBranchesreportshasNextPageand its callers already say "swept in part" (SPEC §6), and #310 builds on that flag this milestone. Itsfirst: 100now carries the comment saying so.Tests
The tracker's fake gh grew
pagingGH, which pages the way the real gh does: the whole listing only when the call carried--paginate, truncated to the first hundred otherwise. That is what makes these tests worth having — I verified by removing the flag from all four readers, which fails all four tests with "returned 100 of …".Comments: the 101-comment issue the capture asked for, with a supersedednot satisfiedverdict on page one and the livesatisfiedone last. Asserts all 101 arrive, in order, fully mapped, and thatParseVerdictslatest-wins lands onsatisfied.SubIssues: 150 rows, the last one in a spoke repo, so both the page-two row and its cross-repo mapping survive.MilestoneIssues: 150 rows with a pull request at row 120 — every page walked, and PRs still dropped wherever they fall — plus theRecentIssuesassertion above in the same test.OpenPRsForBranch: 150 rows, with the<owner>:<ref>head grammar still asserted.go test ./...,go vet ./...,gofmt -l .clean. Beyond the suite I built the binary and ranstatusandmilestone close M15 --dry-runagainst this hub, which exercise the paginatedMilestoneIssues,SubIssuesandCommentsreads live; both behave as before.SPEC
No change.
git grep -n "hundred\|per_page\|first page\|one page"finds no sentence implying a hundred-comment ceiling; the only partial-listing language in §6 isTaskBranches's "swept in part", which stays true. Pagination is an implementation correction, not a protocol change.For the reviewer
Three Decisions on the task:
--paginateover a GraphQL cursor; the GraphQLfirst: Nconnections (ClosingPRs,LinkedBranches,Task) left alone as bounded per-issue relations rather than listings; and the new test seam, with the one-line change tofakeGH's/pullsmatcher it needed (--paginatenow sits ahead of the path). The second is the judgement call most worth a second opinion.No deviations from the plan.
🤖 Generated with Claude Code
https://claude.ai/code/session_017Zu94NTC5DJskcpMTSofzr