perf: bound repair plan PR hydration#49
Conversation
ac5a8e5 to
9d1b504
Compare
ds4psb-ai
left a comment
There was a problem hiding this comment.
Independent code review
Posted on behalf of @ds4psb-ai (read-only contributor). Maintainer judgment owns merge.
Summary verdict
LGTM with two P2 notes — the bounding is mechanical (only files & commits go through ghPagedLimit; comments, reviews, review-comments, and checks stay on full pagination), the helper math holds at every boundary I checked, and the new counts struct extension is backward-compatible.
Findings
P0 / blocker
- (none)
P1 / should fix before merge
- (none)
P2 / nice to have / followup
src/repair/plan-cluster.ts:21-22—Number(process.env.CLAWSWEEPER_MAX_FILES_PER_PR ?? 80)silently coerces a typo'd env var (e.g.CLAWSWEEPER_MAX_FILES_PER_PR=eighty) toNaN, whichghPagedLimitthen turns intomax=0→ empty files array → silent loss of all PR file hydration for the run. Default 80 protects production, but operators tweaking these knobs can disable hydration without warning. Suggest either: (a) guard at module load and warn-and-fallback if!Number.isFinite(x) || x <= 0; or (b) document the parsing rule next to the constant. The same applies toMAX_COMMITS_PER_PR, and the existingMAX_LINKED_REFS/MAX_COMMENTS_PER_ITEM/MAX_REVIEW_COMMENTS_PER_PRtriangulate the same risk.test/repair/plan-cluster.test.ts:225-283— the bounded-vs-full divide test only exercises the 'PR has exactly enough on page 1' codepath. The fake gh script returnslimitentries on the first per_page request, and becauseghPagedLimitthen seesout.length === maxafter page 1, the loop exits before page 2. The multi-page hydration path (e.g.MAX_FILES_PER_PR=150with the helper splitting into two pages of 100 + 50) is not covered. A small extension to the test that driveslimit > 100and asserts the helper actually issues two paginated calls would lock that in.
Test coverage
test/repair/github-cli.test.ts:21-37 covers githubLimitedPagePath:
- happy path (limit=80, page=1)
- existing query param preservation + override of
per_page=100(good — proves theoverride: truepath) - zero/zero edge case (limit=0, page=0 → both clamped to 1)
test/repair/plan-cluster.test.ts:225-283 exercises the end-to-end happy path: 120-file/120-commit PR returns hydrated=80, truncated=40 for both. Good shape.
Gaps (P2):
- Multi-page
ghPagedLimitpath (per the second P2 above). changed_filesreturned by GitHub API asnull/missing —countValuefalls back tofiles.length, which is good, but no test pins this fallback.commits_truncatedandfiles_truncatedsurvive the round-trip throughcompactPlanItem(src/repair/lib.ts:380-387). The end-to-end test asserts oncluster-plan.jsonso this is actually exercised, but it'd be worth a one-line direct unit test forcompactPlanItemto lock the new keys.
Risks I considered and dismissed
- Mechanical preservation claim:
grep -rn 'ghPagedLimit' src/ test/confirms the only consumers are the two lines for files & commits. Issue comments, PR reviews, review comments, and checks all still go throughghPaged/ghPrChecks(src/repair/plan-cluster.ts:189, 197, 198, 201). Future contributors would have to actively replaceghPagedto bound those — not opt-in by accident. ghPagedLimitboundary atmax == perPage: outer loopout.length < maxexits before issuing page 2 — no over-fetch.ghPagedLimitslice safety atmax=120, perPage=100: page 1 returns 100,out.length=100<120, page 2 fetches up to 100, slice trims to 120. Bounded both above and below.githubLimitedPagePathoverridingper_pagefrom caller-passed query: this is the intended behavior because the helper is the SSoT for the page size, and the existing?per_page=50test case on line 31 oftest/repair/github-cli.test.tsproves the override even when the caller passed a different value. Good.- CHANGELOG entry meaningfulness: the +2 lines are observable (anyone reviewing a generated cluster plan will now see
files_truncated/commits_truncatednon-zero on large PRs and correlate to this bound). Warranted.
Clean perf bound. The 'preserve full hydration for safety evidence' framing in the PR description is accurate at the codepath level.
|
Thanks, addressed in What changed:
Validation:
I also tried |
steipete
left a comment
There was a problem hiding this comment.
Reviewed bounded repair plan hydration fix; CI green and targeted local repair tests passed.
Summary
Validation