fix(action): only rewrite the bot's own summary comment

[ydflow]

What this fixes

Fixes #1557.

The sticky summary comment was located by substring alone: findExistingSummaryComment scanned every issue comment newest-first and returned the first whose body contained <!-- ocr-summary -->, with no check that the bot wrote it. postSummary then rewrote that comment in place, so a human comment that merely quoted the marker had its body silently replaced.

SUMMARY_MARKER is plain Markdown, so quoting a prior review's marker verbatim is a normal thing for a human (or another tool) to do when discussing one — and the comment most worth quoting is exactly the durable evidence a downstream gate reads. The failure direction is the dangerous one: the evidence vanishes with no error, no warning, and no way to distinguish "this was overwritten" from "this was never posted".

Root cause and scope

There are three destructive updateComment sites, reached through two finders:

write site	finder
postSummary (L1067)	findExistingSummaryComment
finalizeSummary anchor (L1182)	findSummaryIssueComment
finalizeSummary upsert (L1198)	findSummaryIssueComment

Both finders are fixed, so every rewrite path is covered rather than just the one the report happened to hit.

The fix

Restrict the anchor to a writer GitHub attributes to a bot, by reusing isCheckpointAuthorOurs — the same gate readCheckpointComment already applies to this very comment. No new identity mechanism is introduced.

I deliberately did not use the botLogin check the issue suggested. getAuthenticatedLogin() calls GET /user, which returns 403 "Resource not accessible by integration" for both the default GITHUB_TOKEN and a GitHub App installation token, so botLogin is null on essentially every real run — gating on it would leave the reporter's exact scenario unprotected. The existing isCheckpointAuthorOurs derives the writer from the comment itself (user.type plus performed_via_github_app), which is attested by GitHub and not settable by a commenter. This reasoning is already documented in the source at checkpointAuthorSlug.

findSummaryIssueComment gains a requireBotAuthor parameter that defaults to true, so any future caller is safe without opting in. readCheckpointComment passes false on purpose: it must distinguish "someone quoted our marker" (author_unverified) from "there is no summary yet" (no_summary_comment), and the tightened default would collapse both into the latter. Both reasons review the full range, so the fail-closed behavior is unchanged and all twelve checkpoint reasons stay reachable.

Tests

Two regression tests in scripts/github-actions/post-review-comments.test.js:

• testSummaryAnchorSkipsHumanCommentQuotingTheMarker — bot summary posted first, then a human quotes the marker (the timeline order the bug needs: the human's is the newest matching comment). Asserts the bot's id is rewritten and the human's text survives. Verified RED before the fix: got [77], expected [42].
• testSummaryAnchorStillFindsTheBotsOwnSummary — the other half of the contract: tightening must not stop the bot finding its own summary, or every run would post a duplicate and the checkpoint marker would stop advancing.

Both failed on the unfixed code and pass now; the full post-review-comments.test.js suite passes (All post-review-comments tests passed., exit 0), as do verify-license.sh (exit 0) and verify-english-only.go (exit 0).

Not run to green locally, for environment reasons unrelated to this change (both reproduce on a clean upstream/main):

• action-contract.test.js — 36 failures, status=null from a spawned subprocess on this Windows host.
• check-plugin-contract.test.js — EPERM: operation not permitted, symlink; creating symlinks on Windows needs elevation.

AI disclosure

Per CONTRIBUTING: this PR was produced with AI assistance (Claude, in the Rivet/Tianshu agent environment). I reviewed and understand every line — the RED reproduction, the choice of isCheckpointAuthorOurs over the suggested botLogin check, and the requireBotAuthor default are my own reasoning, verified against the source rather than taken from the issue text. No commits are attributed to AI.

[github-actions]

✅ OpenCodeReview: Review complete: 0 finding(s) across 1 selected item(s).

[Qiyuanqiii]

Reviewed 6ff05be.

The human-comment overwrite case is covered now, but the ownership check is still broader than the PR title and safety requirement imply.

The destructive summary finders call:

isCheckpointAuthorOurs(comment)

without an expected appSlug. With an empty slug, that helper accepts any comment GitHub attributes to a bot. A different bot or GitHub App that quotes the OCR summary marker can therefore still be selected and rewritten in place.

Since these paths call updateComment, the ownership check should identify the bot/app that owns the OCR summary, not only distinguish bot accounts from human accounts.

Please carry the known app identity into the summary lookup where it is available, or use another identity check that prevents OCR from modifying another bot's comment.

Requesting changes because the remaining failure mode still causes destructive modification of a third-party comment.

[ydflow]

You are right, and thank you for the precise framing — the title promised more than the implementation delivered.

The gap: both finders called isCheckpointAuthorOurs(comment) with no appSlug, so an empty slug meant "any writer GitHub attributes to a bot." Another bot or GitHub App quoting the marker was still selected and rewritten in place. Same silent data loss as the human case, from a machine instead of a person.

Fixed in e63a893 by carrying the known app identity into the summary lookup, as you suggested:

• postSummary, ensureSummaryAnchor and finalizeSummary now take appSlug and pass it to both findExistingSummaryComment and findSummaryIssueComment, so every destructive rewrite path is pinned rather than just the one the report happened to hit.
• The identity itself is not new. OCR_CHECKPOINT_APP_SLUG already existed for the checkpoint reader — set to github-actions whenever github_token is the default token, empty otherwise — so this reuses the mechanism the checkpoint reader already applies to this very comment. No second identity check was introduced. The posting step's env was missing it; added.

One deliberate limit on the pinning, which I want to flag rather than bury: it is not unconditional. An installation token cannot ask GitHub which app it is (GET /app needs a JWT), so when the caller supplies its own token the slug is genuinely unknown and stays empty, keeping the wider "any bot" check. Pinning that case to a guessed slug would reject our own summary on every such run — posting a duplicate and stalling the checkpoint — which trades a narrow destructive case for a guaranteed one on every custom-token run. I judged that the wrong trade; say the word if you read it differently.

Tests, both RED before the fix (got [77], expected [42]):

• testSummaryAnchorSkipsAnotherAppsComment — a different app's bot quotes the marker; asserts only this action's own summary is rewritten and the other app's text survives.
• testSummaryAnchorStillFindsOurSummaryWhenAppIsUnknown — the other half: an empty slug must keep the wider check, or a custom-token run would post a duplicate summary.

post-review-comments.test.js passes (exit 0), as do verify-license.sh (exit 0) and verify-english-only.go (exit 0). action-contract.test.js reports 36 failures on this Windows host from a spawned subprocess returning status=null; the identical 36 fail on a clean upstream/main, so they are pre-existing and unrelated to this change.

[lizhengfeng101]

Thanks, the app-slug pinning in e63a893 covers the other-app case well. Three things before merge:

1. PAT runs post a duplicate summary every run. isCheckpointAuthorOurs rejects any type: "User" author, which is what a PAT-posted summary is, so both finders now return null where main returns the existing comment. Suggestion: when appSlug is empty, also accept a User-typed comment whose login equals getAuthenticatedLogin() (succeeds for a PAT, already memoized) and whose body starts with SUMMARY_MARKER. Please add tests: the PAT summary is updated in place, and the same user's comment quoting the marker mid-body is left alone.

2. Read path still anchors on any author. readCheckpointComment takes the newest marker-bearing comment regardless of author, so after one quote the checkpoint is author_unverified forever and incremental review stops, even though our valid summary sits below it. Please locate our comment with the same author gate as the write path, and report author_unverified only when marker-bearing comments exist but none is ours (reuse the fetched list, no extra call).

3. Comments. Per AGENTS.md, drop the #1557 / #1560 references in the tests and keep the rationale in one place instead of three; the PR description already carries the history.

[lizhengfeng101]

Under a PAT this always continues, so the sticky summary is never found (point 1).

[lizhengfeng101]

With the author check deferred, a newer quoting comment shadows our summary permanently (point 2).