feat(action): opt-in resolution of the bot's own outdated review threads (#567)

[chethanuk]

Description

Closes #567. When OCR comments inline and the author pushes a fix, the conversations have to be resolved by hand. This adds a resolve_outdated input, default off, that resolves review threads GitHub has already marked outdated.

Nothing here asks a model. @lizhengfeng101's objection on the issue — that inferring "was this addressed?" from a later diff is hallucination-prone, and that tolerance for false positives at the last gate before merge is near zero — rules that out. The signal is GitHub's own server-computed isOutdated, which it recomputes against the current head and which is force-push and rebase aware.

@stay-foolish-forever asked on the issue how conservative this should be. isOutdated is necessary but not sufficient, so four vetoes sit on top of it. Each can only refuse to resolve a thread, never cause one to be resolved:

thread marked isOutdated by GitHub
  │
  ├─ carries OCR's comment marker? ─────────── no ──► skip: not_ours
  ├─ every comment from the root's author? ─── no ──► skip: human_reply
  ├─ original line resolvable? ─────────────── no ──► skip: unverified
  ├─ any current finding touches those lines? ─ yes ─► skip: overlap
  └─ this run parsed ≥1 finding? ───────────── no ──► skip: no_findings
      │
      └──► resolveReviewThread   (never minimize, never delete)

Three modes: 'false' (default, no GraphQL calls at all), 'report' (runs the whole gate, logs each thread's verdict, mutates nothing), 'true'.

Two places this departs from the obvious implementation

Identity is read off the node the marker proved, not off users.getAuthenticated(). That endpoint is user-token-only and 403s under every Actions token, including the default GITHUB_TOKEN — so an implementation that trusts it gets botLogin: null on every real run and falls back to a hard-coded github-actions[bot] match. Under any custom GitHub App token that classifies OCR's own comments as human replies: the feature resolves nothing and logs skipped=human_reply:N, which an operator reads as "humans replied to all your threads". The gate now takes the root comment's author as the identity once the marker has proved the thread is ours, and requires every reply to match it. That also closes the PAT case, where the login is a human's and their own replies would otherwise have counted as the bot's.

The overlap veto uses plain span intersection, not sameCommentSpan. Reusing the incremental-dedupe predicate looked right and is backwards here: its first rule is cur.multiline !== other.multiline → false, which is correct when a finer single-line note should not be suppressed by an old block comment, and wrong when the question is "does anything current still touch these lines?". It let three real overlaps through — a single-line thread against a covering multi-line finding, a multi-line thread against a single-line finding inside it, and a thread strictly contained by a current finding. All three now veto. incremental_overlap_threshold is deliberately not consulted: an IoU threshold is a similarity test, and this gate needs "any shared line at all".

Limitations

• contents: write is required for resolveReviewThread, measured rather than assumed. On a pull_request_target workflow that is a real trade, and the README says so rather than burying it.
• 'report' mode does not exercise that permission, so a healthy preview can still be followed by FORBIDDEN on flipping to 'true'. Documented at the input.
• 50 threads per run, ~100s at the existing OCR_SUCCESS_DELAY pacing. No new env knob — the one this originally added duplicated OCR_SUCCESS_DELAY's job and is gone.
• Resolving an outdated thread is not the same claim as "this was fixed". A fix applied elsewhere leaves the thread open; a refactor that moves untouched buggy code closes it. isOutdated means the flagged lines left the diff, and the vetoes exist so that stays the only claim being made.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

• make test passes locally
• Manual testing (described below)

node scripts/github-actions/post-review-comments.test.js — all green, including the pre-existing suites. node scripts/github-actions/check-translation-sync.test.js green. action.yml parses.

The core is a 22-case table over the resolve predicate covering each veto and each ordering between them. On top of that, four cases pin the two behaviors above, each written to fail against the obvious wrong implementation:

• identity derived from the root author, and read off the node the marker proved — both fail if getAuthenticated()'s result is trusted;
• the veto flipped to plain intersection — fails against sameCommentSpan, and covers single-vs-multi and containment, which the old expectations had codified the wrong way round;
• action.yml does not promise incremental_overlap_threshold to resolve_outdated, asserted against the file so the docs cannot drift back.

The identity mock now rejects with 403, which is what production actually returns. Every prior test passed a login that no Actions token can obtain, which is precisely what hid the bug.

With the feature off, all five pre-existing comments_* outputs are asserted byte-identical and no GraphQL call is made.

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable)
• I have signed the CLA

Related Issues

Closes #567

[github-actions]

🔍 OpenCodeReview found 3 issue(s) in this PR.

• ✅ Successfully posted inline: 3 comment(s)

[github-actions]

This is a nested ternary expression (resolveOutdated === "true" ? "resolve" : resolveOutdated === "report" ? "report" : "off"), which is disallowed per the coding standards. Consider using a simple lookup object or an if/else chain for clarity and future extensibility (e.g., if more modes are added later).

Suggestion:

Suggested change

	const resolveMode =
	resolveOutdated === "true" ? "resolve" : resolveOutdated === "report" ? "report" : "off";
	const RESOLVE_MODES = { true: "resolve", report: "report" };
	const resolveMode = RESOLVE_MODES[resolveOutdated] || "off";

[github-actions]

The || 0 guards here are redundant: stats.resolved and stats.resolvedPreview are always initialized to 0 in the stats object (lines 120-121) before any code path reaches setStatsOutputs. The || 0 makes these look like they could be undefined, which they cannot be. This is harmless but slightly misleading for future maintainers.

Suggestion:

Suggested change

	out("comments_resolved", String(stats.resolved || 0));
	out("comments_resolved_preview", String(stats.resolvedPreview || 0));
	out("comments_resolved", String(stats.resolved));
	out("comments_resolved_preview", String(stats.resolvedPreview));

[github-actions]

A new RegExp object is compiled from OCR_COMMENT_ID_SOURCE on every invocation of threadIsOurs. Since this function is called once per thread in the listing (potentially hundreds of times), and the regex is stateless (no /g flag), it can be compiled once at module scope and reused. The code already correctly notes in getPostedCommentIds that /g carries lastIndex and must not be shared — but a non-/g regex is safe to share across calls.

Suggestion:

Suggested change

	function threadIsOurs(thread) {
	const root = rootComment(thread);
	return new RegExp(OCR_COMMENT_ID_SOURCE).test((root && root.body) || "");
	}
	const OCR_ID_RE = new RegExp(OCR_COMMENT_ID_SOURCE);
	function threadIsOurs(thread) {
	const root = rootComment(thread);
	return OCR_ID_RE.test((root && root.body) || "");
	}