You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The OCR workflow safely reviews pull request heads, but every pull_request_target synchronize event is treated as a fresh full-PR review. A small follow-up push therefore re-reviews the cumulative merge-base-to-head diff, consumes millions of tokens, and can publish dozens of repeated or low-priority comments.
This issue should make subsequent OCR runs incremental without creating review-coverage gaps, improve runtime and coverage observability, reduce pedantic visible comments without silently losing findings, and establish a controlled way to tune OCR version, rules, model, and concurrency.
OCR therefore reviewed 9 times as many files and 48.6 times as many changed lines as the newest push. Fifty-two of the 75 findings were on paths not touched by that push.
Verified diagnosis
1. Synchronize events always review the cumulative PR range
The workflow resolves the current PR base/head, computes the merge base, and always invokes OCR from that merge base to the current head:
Range resolution: .github/workflows/ocr-review.yml lines 89-182
It does not retain the last successfully reviewed head. Existing exact deduplication occurs after model generation and therefore cannot reduce OCR runtime or token use.
2. File-level concurrency was reduced from 8 to 2
Commit 83bd764 upgraded OCR from v1.6.1 to v1.7.9 and explicitly set concurrency 2. Both versions default to concurrency 8 upstream:
The 3.64-fold increase is consistent with the fourfold reduction in parallel slots. The version and concurrency changed together, so their effects cannot be completely isolated; concurrency is the strongest evidenced contributor, not a proven sole cause. Later concurrency-2 runs on the same PR took 39:04, 43:42, and 46:31 in the OCR step.
3. Cancellation prevents stale parallel runs but does not provide incremental state
This also makes a naive event.before-to-head implementation unsafe: if an intermediate run is canceled or fails, the next event.before boundary could skip changes that never completed review. The authoritative boundary must be the last successfully complete reviewed head.
4. Current rules and publication discard useful priority information
OCR v1.7.9 emits structured category and severity intended for CI grouping and filtering:
Our posting path renders content/location but does not display or apply category/severity policy. The target run contained:
4 high, 37 medium, 33 low, and 1 undocumented info severity;
13 bug, 15 undocumented correctness, 31 maintainability, 12 test, 3 security, and 1 other category.
The undocumented values prove downstream validation must fail open rather than silently suppress unknown schema values.
The trusted OCR rules currently focus on path inclusion/exclusion and retain broad built-in checks. Those built-ins request DRY extraction, comments, defensive checks, typo/style cleanup, and missing tests. OCR's final fact-check filter removes findings directly disproven by the diff; it intentionally does not act as a value or pedantry filter.
After exact deduplication, conservatively moving only low maintainability/test/style/documentation and info-like findings from inline comments to the sticky summary would have reduced inline candidates from 74 to 46 while retaining every finding in durable output. This is a presentation projection, not a claim that every moved finding is invalid.
5. Comment volume creates a separate reliability and usability problem
PR #2610 accumulated 637 OCR-marked inline comments across reruns. In the target run, a 71-comment batch returned a GitHub Server Error after partial success. Fallback ultimately found the comments present, but the sticky summary reported only 50 inline comments.
This posting/accounting defect is not the 46-minute runtime cause, but it reinforces the need for deterministic bounded publication, stable finding identity, and exact reconciliation after ambiguous writes.
Desired architecture
Manifest-backed incremental range selection
Coordinate with #2575 and upstream manifest work rather than inventing a conflicting completeness format:
Use a bot-owned, versioned, machine-readable checkpoint containing at least:
last completely reviewed head;
base ref/base SHA and merge base;
PR number and source run URL;
OCR version and non-secret model identity;
rules, policy, and workflow-schema hashes;
completion and publication state.
Range policy:
Opened, reopened, and ready-for-review: full merge-base-to-head baseline.
Synchronize with a compatible complete checkpoint that is an ancestor of current head: checkpoint-to-current-head.
Same head: observable no-op unless full review is explicitly requested.
Missing/corrupt/incomplete checkpoint, canceled or failed prior run, force push/non-ancestor lineage, base change, or material OCR/model/rule policy change: full merge-base-to-head fallback.
Manual workflow dispatch and an authorized /ocr full command: force a full review.
Keep cancel-in-progress. Never advance the checkpoint from a canceled, partial, failed, malformed, or ambiguously published run. Treat pull request event before/after SHAs as diagnostics rather than authoritative completion state.
OCR already supports arbitrary ranges through from/to. Its resume feature is useful for compatible same-range retries, not as a replacement for durable last-successful-head state. The reusable action's incremental option suppresses overlapping publication only; it does not reduce model work.
High-signal finding policy
Validate category and severity against the deployed schema.
Treat unknown/missing values as visible and record schema drift.
Display category/severity in published findings.
Initially keep high/critical, security, medium, bug/correctness, and unknown findings inline.
Move low maintainability/test/style/documentation findings into a collapsed sticky-summary section by default.
Preserve every raw finding and its tier/suppression reason in the redacted artifact.
Sort and proactively cap inline comments; put overflow in the summary/artifact rather than waiting for a large batch to fail.
Run the publication policy in shadow mode against a human-adjudicated historical corpus before suppressing visible comments. Severity is model-generated metadata, not calibrated confidence.
Project-specific generation guidance
A/B test trusted repository rules/background that require:
a causal connection to changed behavior;
a concrete behavioral, security, data-loss, portability, performance, or test-validity impact;
a reproducible failure path or violated repository contract;
caller/contract evidence before asking for extra defensive handling;
one canonical comment per root cause;
no naming, style, DRY, hypothetical-future, duplicated test-helper, extra-comment, or nicer-diagnostic suggestions unless they demonstrate a probable defect or explicit rule violation.
Be deliberate about OCR rule composition. A matching user rule replaces the built-in rule unless merge_system_rule is enabled:
Merging every built-in check may reintroduce the pedantry this work is intended to reduce; replacing broad rules may reduce recall. Evaluate both against the same labeled corpus.
Observability and posting correctness
Emit a machine-readable metadata artifact plus job/sticky summary containing:
event/action and event before/after;
API-resolved base/head;
checkpoint before/after;
selected range mode and fallback reason;
cumulative and selected base/head/files/lines;
eligible, excluded, selected, completed, reused, and failed files when available;
OCR version/model/rule/policy hashes and concurrency;
input/output/cache/total tokens and exact OCR/job duration;
warnings, terminal completeness state, publication state, and source run URL.
Reconcile ambiguous batch writes using deterministic per-run finding markers before retrying every candidate. Report mutually exclusive counters that match GitHub's observed state.
Treat that upgrade as a correctness prerequisite for experiments, not a promised latency fix.
Replay immutable small-delta and large-full ranges with fixed OCR version, model, and rules at concurrency 2, 4, and 8.
Record duration, per-file latency where available, tokens/cache use, 429/529 responses, timeouts, partial/all-file failures, and adjudicated precision/recall.
Adopt the highest concurrency supported by observed provider/account limits and complete-coverage reliability. Do not restore 8 merely because it is OCR's default.
Evaluate max-tools, refined path exclusions, provider/model alternatives, and persisted same-range resume only after incremental range correctness is established.
Do not combine range, rule, version, model, and concurrency changes in one experiment.
Normal synchronize runs use the newest compatible successfully complete checkpoint as their base.
Rapid pushes with a canceled or failed intermediate run lose no commits.
Opened/reopened/ready/manual-full events retain a verified full-review path.
Force push, non-ancestor state, base change, missing/corrupt state, and material policy changes deterministically fall back full and record the reason.
Same-head reruns are observable no-ops unless full mode is requested.
Canceled, partial, failed, malformed, incomplete, or ambiguously published runs never advance the checkpoint.
Changed tests in the selected range remain reviewable.
Existing trusted-workflow, fork-safety, least-privilege, head-race, and cancellation guarantees remain intact.
Measurement and completeness
Artifacts and summaries record exact selected/cumulative range, files/lines, OCR version/model/concurrency, tokens, duration, findings, warnings, terminal state, and checkpoint transition.
A run cannot claim complete or no findings unless all selected files completed, were safely reused, or were explicitly waived under the completeness contract.
Selected/completed/reported file-count discrepancies are detected and explained.
Controlled experiments hold refs, model, rules, and OCR version constant when varying concurrency.
Signal and delivery
Valid category/severity appear in comments; unknown values fail open and are counted.
Every raw finding and tier/suppression reason remains in a redacted artifact.
Problem
The OCR workflow safely reviews pull request heads, but every pull_request_target synchronize event is treated as a fresh full-PR review. A small follow-up push therefore re-reviews the cumulative merge-base-to-head diff, consumes millions of tokens, and can publish dozens of repeated or low-priority comments.
This issue should make subsequent OCR runs incremental without creating review-coverage gaps, improve runtime and coverage observability, reduce pedantic visible comments without silently losing findings, and establish a controlled way to tune OCR version, rules, model, and concurrency.
Primary evidence
Run: https://github.com/vybestack/llxprt-code/actions/runs/29946076923
Job: https://github.com/vybestack/llxprt-code/actions/runs/29946076923/job/89011650080
PR: #2610
Exact GitHub timestamps show:
OCR execution occupied 2,791 of 2,841 job seconds, or 98.2%. Checkout, installation, and result posting are not the main latency source.
The run reported:
The push that triggered the run changed only 7 files and +282/-41 lines:
1436f7b...cdd6a6c
OCR therefore reviewed 9 times as many files and 48.6 times as many changed lines as the newest push. Fifty-two of the 75 findings were on paths not touched by that push.
Verified diagnosis
1. Synchronize events always review the cumulative PR range
The workflow resolves the current PR base/head, computes the merge base, and always invokes OCR from that merge base to the current head:
It does not retain the last successfully reviewed head. Existing exact deduplication occurs after model generation and therefore cannot reduce OCR runtime or token use.
2. File-level concurrency was reduced from 8 to 2
Commit 83bd764 upgraded OCR from v1.6.1 to v1.7.9 and explicitly set concurrency 2. Both versions default to concurrency 8 upstream:
https://github.com/alibaba/open-code-review/blob/v1.7.9/README.md#L472-L486
A close same-PR comparison around that deployment:
The 3.64-fold increase is consistent with the fourfold reduction in parallel slots. The version and concurrency changed together, so their effects cannot be completely isolated; concurrency is the strongest evidenced contributor, not a proven sole cause. Later concurrency-2 runs on the same PR took 39:04, 43:42, and 46:31 in the OCR step.
3. Cancellation prevents stale parallel runs but does not provide incremental state
Per-PR cancel-in-progress correctly cancels obsolete work. Run https://github.com/vybestack/llxprt-code/actions/runs/29937651406 was canceled after spending 38:42 in OCR.
This also makes a naive event.before-to-head implementation unsafe: if an intermediate run is canceled or fails, the next event.before boundary could skip changes that never completed review. The authoritative boundary must be the last successfully complete reviewed head.
4. Current rules and publication discard useful priority information
OCR v1.7.9 emits structured category and severity intended for CI grouping and filtering:
Our posting path renders content/location but does not display or apply category/severity policy. The target run contained:
The undocumented values prove downstream validation must fail open rather than silently suppress unknown schema values.
The trusted OCR rules currently focus on path inclusion/exclusion and retain broad built-in checks. Those built-ins request DRY extraction, comments, defensive checks, typo/style cleanup, and missing tests. OCR's final fact-check filter removes findings directly disproven by the diff; it intentionally does not act as a value or pedantry filter.
After exact deduplication, conservatively moving only low maintainability/test/style/documentation and info-like findings from inline comments to the sticky summary would have reduced inline candidates from 74 to 46 while retaining every finding in durable output. This is a presentation projection, not a claim that every moved finding is invalid.
5. Comment volume creates a separate reliability and usability problem
PR #2610 accumulated 637 OCR-marked inline comments across reruns. In the target run, a 71-comment batch returned a GitHub Server Error after partial success. Fallback ultimately found the comments present, but the sticky summary reported only 50 inline comments.
This posting/accounting defect is not the 46-minute runtime cause, but it reinforces the need for deterministic bounded publication, stable finding identity, and exact reconciliation after ambiguous writes.
Desired architecture
Manifest-backed incremental range selection
Coordinate with #2575 and upstream manifest work rather than inventing a conflicting completeness format:
Use a bot-owned, versioned, machine-readable checkpoint containing at least:
Range policy:
Keep cancel-in-progress. Never advance the checkpoint from a canceled, partial, failed, malformed, or ambiguously published run. Treat pull request event before/after SHAs as diagnostics rather than authoritative completion state.
OCR already supports arbitrary ranges through from/to. Its resume feature is useful for compatible same-range retries, not as a replacement for durable last-successful-head state. The reusable action's incremental option suppresses overlapping publication only; it does not reduce model work.
High-signal finding policy
Add stable finding fingerprints and semantic clustering alibaba/open-code-review#369
Run the publication policy in shadow mode against a human-adjudicated historical corpus before suppressing visible comments. Severity is model-generated metadata, not calibrated confidence.
Project-specific generation guidance
A/B test trusted repository rules/background that require:
Be deliberate about OCR rule composition. A matching user rule replaces the built-in rule unless merge_system_rule is enabled:
https://github.com/alibaba/open-code-review/blob/v1.7.9/README.md#L635-L669
Merging every built-in check may reintroduce the pedantry this work is intended to reduce; replacing broad rules may reduce recall. Evaluate both against the same labeled corpus.
Observability and posting correctness
Emit a machine-readable metadata artifact plus job/sticky summary containing:
Reconcile ambiguous batch writes using deterministic per-run finding markers before retrying every candidate. Report mutually exclusive counters that match GitHub's observed state.
Controlled version and concurrency experiments
https://github.com/alibaba/open-code-review/releases/tag/v1.7.15
Implementation phases
Phase 0 — Instrumentation and benchmark corpus
Phase 1 — Complete-checkpoint incremental reviews
Phase 2 — Structured presentation and publication reconciliation
Phase 3 — Generation-quality and performance experiments
Phase 4 — Consume upstream state and identity support
Acceptance criteria
Range and coverage
Measurement and completeness
Signal and delivery
Tests and verification
Constraints / non-goals
Related work
llxprt-code
OpenCodeReview upstream