Skip to content

perf: reduce GitHub comment round trips#379

Open
brokemac79 wants to merge 1 commit into
openclaw:mainfrom
brokemac79:codex/github-roundtrip-cache
Open

perf: reduce GitHub comment round trips#379
brokemac79 wants to merge 1 commit into
openclaw:mainfrom
brokemac79:codex/github-roundtrip-cache

Conversation

@brokemac79

@brokemac79 brokemac79 commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

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 upsertReviewComment when 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.ts unchanged.

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 in src/clawsweeper.ts and bounded unknown-count PR-close proof hydration in src/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.

  • Duplicate scan before opening found no exact open PR for upsertReviewComment, coverage-proof comment windows, or unknown comment counts; broader automation PR hits were unrelated.
  • GitHub CI passed on head a5989d6a8574c28c8eae248af107046008892fd3: pnpm check, CodeQL, Windows Codex launcher, Socket/notify checks all green.
  • pnpm run build:all passed locally.
  • pnpm run lint passed locally.
  • pnpm run format:check passed locally.
  • pnpm run check:active-surface passed locally.
  • pnpm run check:limits passed locally.
  • git diff --check passed locally.
  • node --test test/context.test.ts passed locally: 19 tests, 19 pass.
  • node --test test/clawsweeper.test.ts passed locally: 60 tests, 60 pass.
  • node --test test/repair/apply-result.test.ts passed locally: 34 tests, 34 pass.
  • node --test test/repair/github-cli.test.ts passed locally: 2 tests, 2 pass.
  • CI failure triage after the first amended push found Linux pnpm check failing because promotionGhMock still returned stale comment state after mocked comment writes; the helper now returns the written comment response and preserves/updates comment history for later reads.
  • Final CI fixture correction on head 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, and git diff --check passed locally.
  • codex review -c 'service_tier="fast"' --uncommitted first 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 that promotionGhMock needed 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/main passed on the committed branch diff with no actionable correctness issues.
  • codex review -c 'service_tier="fast"' --uncommitted passed on the final test-only CI fixture correction with no actionable regression.

Focused behavior proof from the changed tests:

node --test test/clawsweeper.test.ts
...
apply-decisions falls back to comment lookup after malformed mutation response
apply-decisions rejects stale fallback after malformed mutation response
# asserted exact-body fallback succeeds after malformed mutation output and stale fallback fails without review_comment_synced_at
pass 60, fail 0

node --test test/repair/apply-result.test.ts
...
repair apply bounds covering PR comments when issue comment count is absent
# asserted no --slurp full pagination, exactly two -i comment page reads, and per_page=100 bounded reads
pass 34, fail 0

Runtime API-trace proof on the final head:

$ node dist/clawsweeper.js apply-decisions ...  # real built entrypoint, redacted GitHub API shim, no network writes
[apply] 2026-06-28T00:32:07.712Z starting apply: files=2 dry_run=false ... processed=0/1 counts={}
[apply] 2026-06-28T00:32:08.547Z finished apply closed=0/10 processed=1/1 counts={"review_comment_synced":1}

{
  "entrypoint": "node dist/clawsweeper.js apply-decisions",
  "actionReport": [
    {
      "number": 321,
      "action": "review_comment_synced",
      "reason": "updated durable Codex review comment"
    }
  ],
  "commentListFetchesFor321": 1,
  "patchResponsesReused": 1,
  "issueEditsFor321": 7,
  "touchedIssue322": false,
  "traceExcerpt": [
    ["api", "repos/openclaw/clawsweeper/issues/321/comments?per_page=100", "--paginate", "--slurp"],
    ["api", "repos/openclaw/clawsweeper/issues/comments/9321", "--method", "PATCH", "--input", ".../comment-321.json"],
    ["comment-patch", "body_length=2320"]
  ]
}

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/comments after the write. The unchanged apply gates still keep the run to #321 only; #322 was not touched.

$ node dist/repair/apply-result.js <job.md> <result.json>  # real built entrypoint, redacted GitHub API shim, no network writes
{
  "entrypoint": "node dist/repair/apply-result.js <job.md> <result.json>",
  "actionReport": [
    {
      "target": "#101",
      "status": "executed",
      "reason": "duplicate of the canonical thread"
    }
  ],
  "closeExecuted": true,
  "boundedCoveringCommentReads": [
    ["api", "-i", "repos/openclaw/openclaw/issues/202/comments?per_page=100&page=1"],
    ["api", "-i", "repos/openclaw/openclaw/issues/202/comments?per_page=100&page=2"]
  ],
  "coveringReadsUsedSlurp": false,
  "coveringReadsUsedHeaders": true,
  "coveringReadsPerPage100": true,
  "postProofFreshnessReads": [
    ["api", "repos/openclaw/openclaw/issues/101"],
    ["api", "repos/openclaw/openclaw/pulls/202"],
    ["api", "repos/openclaw/openclaw/issues/101/comments", "--method"]
  ]
}

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 comments count. The runtime used two bounded gh api -i page reads with per_page=100, did not use --slurp full pagination, included the recent tail comment in the proof prompt, still performed post-proof freshness/safety reads, and then executed the duplicate close.

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

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 details

Best 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:

  • linked superseding PR: fix: make repair tests portable on Windows #376 (fix: make repair tests portable on Windows) is still open as the canonical replacement.
  • cluster evidence: the durable review links that PR in the work cluster or recommended risk path.
  • no human follow-up: live comments and timeline hydrated by apply contain no non-automation activity after the ClawSweeper review.

Likely related people:

  • RomneyDa: Git blame and history show Dallin Romney introduced or heavily touched the PR-close coverage proof, durable review comment sync, and shared test helper areas now changed by this PR. (role: introduced behavior and adjacent test owner; confidence: high; commits: 4e5c4d47c83f, e73fecd30a70, 6035f230849b; files: src/clawsweeper.ts, src/repair/apply-result.ts, test/helpers.ts)
  • brokemac79: Before this PR, brokemac79 landed adjacent current-main work routing PR close reviews to autoclose in src/clawsweeper.ts. (role: recent area contributor; confidence: medium; commits: 84796f0c9b0c; files: src/clawsweeper.ts)
  • Peter Steinberger: History shows the original durable review comment sync feature entering main in commit 787bb0a. (role: introduced durable comment sync; confidence: medium; commits: 787bb0ad3ba8; files: src/clawsweeper.ts)

Codex review notes: model internal, reasoning high; reviewed against ae63b16d6c74.

@clawsweeper clawsweeper Bot added rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. labels Jun 27, 2026
@brokemac79

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🦪 silver shellfish Thin PR readiness signal; proof, validation, or implementation needs work. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 27, 2026
@brokemac79 brokemac79 force-pushed the codex/github-roundtrip-cache branch from d0c43dc to 91ab5a8 Compare June 27, 2026 22:51
@brokemac79

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

@clawsweeper clawsweeper Bot removed proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. labels Jun 27, 2026
@brokemac79 brokemac79 force-pushed the codex/github-roundtrip-cache branch from 91ab5a8 to 3650b48 Compare June 27, 2026 23:43
@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. P2 Normal priority bug or improvement with limited blast radius. and removed P3 Low-risk cleanup, docs, polish, ergonomics, or speculative feature. labels Jun 27, 2026
@brokemac79 brokemac79 force-pushed the codex/github-roundtrip-cache branch from 3650b48 to 0dfd420 Compare June 28, 2026 00:23
@brokemac79 brokemac79 force-pushed the codex/github-roundtrip-cache branch from 0dfd420 to a5989d6 Compare June 28, 2026 00:31
@clawsweeper clawsweeper Bot added status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. and removed status: ⏳ waiting on author ClawSweeper has contributor-facing work open and is waiting for author action. rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. labels Jun 28, 2026
@clawsweeper clawsweeper Bot added rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed status: 🛠️ actively grinding The PR author has acted after the latest ClawSweeper review and work remains. labels Jun 28, 2026
@brokemac79

Copy link
Copy Markdown
Contributor Author

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 28, 2026

Copy link
Copy Markdown
Contributor

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 automation 🚨 Merging this PR could break CI, automerge, proof capture, label sync, or automation. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant