mirror of
https://github.com/alibaba/open-code-review.git
synced 2026-08-04 22:11:05 +00:00
12406a35f3
GitHub rejects an entire createReview batch with HTTP 422 when any one inline comment points at a line outside the PR diff. The previous fallback degraded to N separate per-comment reviews, producing N timeline entries for what is logically one review — the churn described in #624. The batch path now filters out the comments it can PROVE are unresolvable against the PR diff and re-sends the survivors as a single review. Dropped comments are reported in the summary with their original span. Correctness constraints, each of which can make things worse if relaxed: - Classification is tri-state (valid/invalid/unknown), not boolean. "invalid" is a claim that must be proven, because it permanently routes a finding to the summary without ever attempting to post it. Missing diff metadata — a patchless binary or oversized file, a truncated file walk — is "unknown", not "invalid". Treating absent metadata as proof of out-of-diff would silently void an entire batch with zero posting attempts, which is strictly worse than the pre-existing behavior. Only "valid" comments are re-batched; "unknown" keeps the per-comment path. - The 422 gate is conservative. GitHub documents 422 on this endpoint as "Validation failed, OR the endpoint has been spammed", so the status code alone is not evidence of a line-resolution problem. The fallback activates only on a confirmed line/diff validation error; anything else, including spam/abuse detection, falls through to the per-comment loop. - Both the primary and the secondary batch run the same cooldown and idempotency reconciliation (cooldownAndReconcile). A secondary batch that fails with a 5xx or network error may still have LANDED with its response lost; retrying it blindly would repost every comment and recreate the exact duplication this change removes. - Diff hunks are parsed as {start,end} ranges, one per hunk, so a multi-line span must satisfy start_line <= line within a SINGLE hunk. A flat per-file line set would accept cross-hunk and reversed spans, which GitHub then rejects with another 422. The diff inventory is fetched once per run and paginated through the existing readWithPacing helper, so it shares the repo's read retry and pacing discipline rather than bypassing it. Verified against live GitHub, not only against mocks. Driving the real runPostReviewComments at a real PR with two in-diff and two out-of-diff comments produced: one 422 on the initial batch, one listFiles read, one successful secondary batch, and EXACTLY ONE review timeline entry holding the two surviving comments, with the two unresolvable ones reported in the summary. The observed 422 body is { "message": "Unprocessable Entity", "errors": ["Line could not be resolved and Line could not be resolved"], "status": "422" } which pins two things the implementation silently depended on. First, this endpoint returns errors[] as plain STRINGS, so the structured entry.field branch is unreachable here (kept as defensive cover for other endpoints, now annotated as such). Second, the decisive wording reaches us only through Octokit's composed error.message; response.data.message alone is "Unprocessable Entity" and matches no pattern, so narrowing the gate to the structured field would disable the fallback while leaving every test green. That footgun is now documented at the call site and pinned by a regression test built from the verbatim live payload. Known limitations: - The 422 gate matches known line-resolution wordings. Live probing found "Line could not be resolved" (line outside a hunk, past EOF, negative, LEFT side, or a span straddling hunks), "Start position could not be resolved" (inverted span) and "Path could not be resolved" (path not in the PR), all covered by /could not be resolved/i. A future wording outside these patterns would fall through to the per-comment loop: never worse than the pre-existing behavior, but it would quietly disable the grouping. - Only RIGHT-side comments are modeled. A LEFT-side comment classifies as "unknown" and takes the per-comment path. The producer emits RIGHT-side comments today. npm run test:github-actions passes, covering secondary landed-then-5xx reconciliation (fully and partially landed), secondary 429 cooldown followed by the per-comment retry, non-line 422s, patchless files, cross-hunk and reversed spans, listFiles truncation, and inventory caching. Refs: #624 |
||
|---|---|---|
| .. | ||
| github-actions | ||
| publish | ||
| install.js | ||
| platform.js | ||
| update.js | ||