perf: reduce GitHub comment round trips#379
Conversation
|
Thanks for the context here. I swept through the related work, and this is now duplicate or superseded. Keep open for maintainer review: the patch is narrow and well-proven, but it changes apply-lane automation paths where merge acceptance should remain human. Canonical path: Close this PR as superseded by #376. So I’m closing this here and keeping the remaining discussion on #376. Review detailsBest possible solution: Close this PR as superseded by #376. Do we have a high-confidence way to reproduce the issue? Not applicable as a performance cleanup rather than a bug report. The contributor supplied focused tests and live-output traces for the changed automation paths. Is this the best way to solve the issue? Yes, with maintainer review: the implementation is narrow for the stated round-trip problem and preserves fallback and freshness behavior, while #376 is only partially overlapping test-helper work. Security review: Security review cleared: No dependency, workflow permission, secret handling, or third-party execution changes were introduced by this diff. AGENTS.md: found and applied where relevant. What I checked:
Likely related people:
Codex review notes: model internal, reasoning high; reviewed against ae63b16d6c74. |
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
d0c43dc to
91ab5a8
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
91ab5a8 to
3650b48
Compare
3650b48 to
0dfd420
Compare
0dfd420 to
a5989d6
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. |
What Problem This Solves
ClawSweeper has a few GitHub comment/proof paths that do avoidable repeat reads. Durable review comment sync writes a comment and can then re-fetch the issue comments list to rediscover the same comment. PR close coverage proof can also fall back to full comment pagination when GitHub does not provide a comment count.
Why This Change Was Made
This reuses GitHub's POST/PATCH comment response from
upsertReviewCommentwhen the response already identifies the written comment. Malformed or empty response fallback is still supported, but it now only accepts a follow-up lookup when that lookup exposes the exact body just written; otherwise it fails closed for retry instead of stamping stale sync metadata.Coverage-proof comment hydration now stays bounded when issue comment counts are absent: the main ClawSweeper path uses the existing link-header context window helper, and repair apply uses a local first/tail link-header window before falling back to full pagination only when GitHub does not expose usable page links. The coverage proof gate also memoizes candidate PR hydration inside one proof operation, while the post-proof freshness recheck remains a fresh GitHub read.
The final diff deliberately leaves
src/repair/github-cli.tsunchanged.Relationship To Existing PRs
PR #376 is related but not a canonical replacement for this PR. #376 focuses on Windows repair test and command portability, including
src/repair/github-cli.ts,src/repair/command-runner.ts,src/repair/git-publish.ts, and broad test fixture updates. This PR's production changes are the durable-comment response reuse insrc/clawsweeper.tsand bounded unknown-count PR-close proof hydration insrc/repair/apply-result.ts. Closing this PR in favor of #376 would drop those production round-trip reductions.User Impact
Apply/comment/proof runs make fewer GitHub API calls and avoid unnecessary full comment pagination on large discussions, without changing close gates, freshness rechecks, durable comment markers, or public review output.
Evidence
Head proofed:
a5989d6a8574c28c8eae248af107046008892fd3.upsertReviewComment, coverage-proof comment windows, or unknown comment counts; broader automation PR hits were unrelated.a5989d6a8574c28c8eae248af107046008892fd3:pnpm check, CodeQL, Windows Codex launcher, Socket/notify checks all green.pnpm run build:allpassed locally.pnpm run lintpassed locally.pnpm run format:checkpassed locally.pnpm run check:active-surfacepassed locally.pnpm run check:limitspassed locally.git diff --checkpassed locally.node --test test/context.test.tspassed locally: 19 tests, 19 pass.node --test test/clawsweeper.test.tspassed locally: 60 tests, 60 pass.node --test test/repair/apply-result.test.tspassed locally: 34 tests, 34 pass.node --test test/repair/github-cli.test.tspassed locally: 2 tests, 2 pass.a5989d6a8574c28c8eae248af107046008892fd3:pnpm run format:check,node --test test/apply-label-sync.test.ts,pnpm run build:all,pnpm run lint,pnpm run check:active-surface,pnpm run check:limits, andgit diff --checkpassed locally.codex review -c 'service_tier="fast"' --uncommittedfirst accepted the stale malformed-response fallback risk; fixed by exact-body fallback verification and a stale-read regression, then rerun clean. A later CI-fixture review accepted thatpromotionGhMockneeded to expose successful comment writes while preserving existing comments; fixed by returning/persisting written comment JSON by id, then rerun clean.codex review -c 'service_tier="fast"' --base origin/mainpassed on the committed branch diff with no actionable correctness issues.codex review -c 'service_tier="fast"' --uncommittedpassed on the final test-only CI fixture correction with no actionable regression.Focused behavior proof from the changed tests:
Runtime API-trace proof on the final head:
That apply trace exercises the durable-comment write path through the real built CLI. It reads the existing comment list once, patches the durable comment, reuses the PATCH response for metadata, and does not re-fetch
/issues/321/commentsafter the write. The unchanged apply gates still keep the run to #321 only; #322 was not touched.That repair trace exercises the unknown-comment-count coverage proof path through the real built repair entrypoint. The covering PR had 160 comments and no issue
commentscount. The runtime used two boundedgh api -ipage reads withper_page=100, did not use--slurpfull pagination, included the recent tail comment in the proof prompt, still performed post-proof freshness/safety reads, and then executed the duplicate close.