feat(action): opt-in completeness-gated checkpoint ranges (#476) - #945
feat(action): opt-in completeness-gated checkpoint ranges (#476)#945chethanuk wants to merge 1 commit into
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
🔍 OpenCodeReview found 10 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| checkpoint_range: | ||
| description: >- |
There was a problem hiding this comment.
No spelling errors found in YAML keys. All keys are correctly spelled.
| - name: Resolve review range | ||
| if: inputs.checkpoint_range == 'true' | ||
| id: range | ||
| uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 | ||
| env: | ||
| OCR_FULL_REVIEW: ${{ inputs.full_review }} | ||
| OCR_STICKY_SUMMARY: ${{ inputs.sticky_summary }} | ||
| # "reopened" / "ready_for_review" ask for a fresh look at the whole PR. | ||
| OCR_EVENT_ACTION: ${{ github.event.action }} |
There was a problem hiding this comment.
The 'Resolve review range' step references process.env.HEAD_SHA, process.env.BASE_REF, and process.env.MERGE_BASE without declaring them in the step's env: block. These are expected to be set via $GITHUB_ENV by earlier steps (lines 279-280, 308). While the fail-closed design in resolveCheckpointRange should catch most issues, if any earlier step fails or is conditionally skipped, these variables will be empty strings. An empty headSha would result in an empty range_to output, and the review step would then run --from X --to '', potentially causing git errors or unexpected behavior. Consider explicitly declaring these as environment variables in this step's env: block (similar to how the post step does on lines 554-555) to make the dependency explicit and catch failures earlier.
| OCR_FINGERPRINT_INPUTS: >- | ||
| ${{ inputs.llm_url }}|${{ inputs.llm_model }}|${{ inputs.llm_use_anthropic }}|${{ | ||
| inputs.language }}|${{ inputs.llm_extra_body }}|${{ inputs.rule }}|${{ | ||
| inputs.route_severity_below }}|${{ inputs.route_categories }}|${{ inputs.background }} |
There was a problem hiding this comment.
The OCR_FINGERPRINT_INPUTS environment variable uses YAML folded scalar (>-) which collapses newlines into spaces and normalizes whitespace. This means the fingerprint computation depends on YAML's folding behavior, which could produce inconsistent results if the number of interpolation expressions or their formatting changes between workflow runs (e.g., GitHub Actions runner version changes, or if someone reformats this block). An inconsistent fingerprint would silently force a full review every time, defeating the purpose of checkpointing without any visible error. Consider using a plain string scalar or explicitly joining the values in the JavaScript code to make the fingerprint computation more robust and explicit.
| reviewCommentBatchSize: parseInt(process.env.OCR_REVIEW_COMMENT_BATCH_SIZE, 10), | ||
| routeSeverityBelow: process.env.OCR_ROUTE_SEVERITY_BELOW, | ||
| routeCategories: process.env.OCR_ROUTE_CATEGORIES, | ||
| checkpointEnabled: ${{ inputs.checkpoint_range == 'true' }}, |
There was a problem hiding this comment.
The checkpointEnabled parameter uses a GitHub Actions template expression ${{ inputs.checkpoint_range == 'true' }} which is evaluated at workflow parse time and injected directly into the JavaScript source as a boolean literal. While this pattern is also used for stickySummary and incremental (lines 580-581), it makes the script harder to reason about and debug compared to passing boolean values as environment variables and comparing them as strings (like OCR_FULL_REVIEW in the resolve step). This inconsistency is minor but worth noting for maintainability.
| // (#476). Gated on summaryUrl because the marker lives inside the summary | ||
| // comment: a summary that never published carries no checkpoint, so claiming | ||
| // one on the output would lie to the caller. | ||
| out("checkpoint_after", stats.summaryUrl && stats.checkpointAfter ? stats.checkpointAfter : ""); |
There was a problem hiding this comment.
Style: The style guide prohibits == and != in favor of strict equality (=== / !==). This line uses != which violates that rule. Since both stats.summaryUrl (initialized as "") and stats.checkpointAfter (initialized as "") are always strings, you can simplify to a truthy check without loose equality:
Suggestion:
| out("checkpoint_after", stats.summaryUrl && stats.checkpointAfter ? stats.checkpointAfter : ""); | |
| out("checkpoint_after", stats.summaryUrl && stats.checkpointAfter ? stats.checkpointAfter : ""); | |
| // Note: if the != null was intentional to distinguish null/undefined from "", | |
| // consider: out("checkpoint_after", stats.summaryUrl !== "" && stats.checkpointAfter !== "" ? stats.checkpointAfter : ""); |
| // (#476). Gated on summaryUrl because the marker lives inside the summary | ||
| // comment: a summary that never published carries no checkpoint, so claiming | ||
| // one on the output would lie to the caller. | ||
| out("checkpoint_after", stats.summaryUrl && stats.checkpointAfter ? stats.checkpointAfter : ""); |
There was a problem hiding this comment.
Style issue: Use strict equality (!==) instead of loose equality (!=). Since both stats.summaryUrl (initialized as "") and stats.checkpointAfter (initialized as "") are always strings, you can use a truthy check or strict inequality comparison with an empty string:
Suggestion:
| out("checkpoint_after", stats.summaryUrl && stats.checkpointAfter ? stats.checkpointAfter : ""); | |
| out("checkpoint_after", stats.summaryUrl !== "" && stats.checkpointAfter !== "" ? stats.checkpointAfter : ""); |
| function preserveCheckpointMarker(newBody, oldBody) { | ||
| if (typeof oldBody !== "string" || oldBody === "") return newBody; | ||
| if (new RegExp(CHECKPOINT_MARKER_PATTERN).test(newBody || "")) return newBody; | ||
| const found = oldBody.match(new RegExp(CHECKPOINT_MARKER_PATTERN, "g")) || []; | ||
| return found.length === 1 ? `${newBody}\n\n${found[0]}` : newBody; | ||
| } |
There was a problem hiding this comment.
Performance: CHECKPOINT_MARKER_PATTERN is compiled into a new RegExp object at every call site (4 times total: twice here in preserveCheckpointMarker, once in parseCheckpointMarker, once in readCheckpointComment). Since the pattern is a module-level constant, it should be compiled once at the top level and reused. This avoids redundant regex compilation overhead on every invocation, which matters especially for the hot summary-write paths.
Pre-compile at module scope:
const CHECKPOINT_MARKER_RE = new RegExp(CHECKPOINT_MARKER_PATTERN);
const CHECKPOINT_MARKER_RE_GLOBAL = new RegExp(CHECKPOINT_MARKER_PATTERN, "g");Then use these constants in preserveCheckpointMarker, parseCheckpointMarker, and readCheckpointComment. Note that the global variant needs lastIndex reset between uses if used with .exec() in a loop (as parseCheckpointMarker does).
| function preserveCheckpointMarker(newBody, oldBody) { | ||
| if (typeof oldBody !== "string" || oldBody === "") return newBody; | ||
| if (new RegExp(CHECKPOINT_MARKER_PATTERN).test(newBody || "")) return newBody; | ||
| const found = oldBody.match(new RegExp(CHECKPOINT_MARKER_PATTERN, "g")) || []; | ||
| return found.length === 1 ? `${newBody}\n\n${found[0]}` : newBody; | ||
| } |
There was a problem hiding this comment.
Performance issue: CHECKPOINT_MARKER_PATTERN is compiled into new RegExp objects on every invocation (twice in this function alone). Since the pattern is a module-level constant, it should be pre-compiled once and reused. This avoids unnecessary regex compilation overhead, especially important for the hot summary-write paths.
Add at module scope:
const CHECKPOINT_MARKER_RE = new RegExp(CHECKPOINT_MARKER_PATTERN);
const CHECKPOINT_MARKER_RE_GLOBAL = new RegExp(CHECKPOINT_MARKER_PATTERN, "g");Then replace the new RegExp() calls throughout the file. Note: when using the global variant with .exec() in loops (as in parseCheckpointMarker), reset lastIndex = 0 before each use.
| const body = comment.body || ""; | ||
| const payload = parseCheckpointMarker(body); | ||
| if (!payload) return { reason: "corrupt_checkpoint", payload: null, raw: "" }; | ||
| const raw = (new RegExp(CHECKPOINT_MARKER_PATTERN).exec(body) || [""])[0]; |
There was a problem hiding this comment.
Dead code: At this point in the control flow, parseCheckpointMarker(body) has already returned a non-null payload (line 2431), which guarantees the body contains exactly one valid marker. Therefore new RegExp(CHECKPOINT_MARKER_PATTERN).exec(body) will always find a match, and the || [""] fallback is unreachable. The line could be simplified for clarity:
const raw = new RegExp(CHECKPOINT_MARKER_PATTERN).exec(body)[0];This also benefits from using a pre-compiled regex constant as suggested above.
| const body = comment.body || ""; | ||
| const payload = parseCheckpointMarker(body); | ||
| if (!payload) return { reason: "corrupt_checkpoint", payload: null, raw: "" }; | ||
| const raw = (new RegExp(CHECKPOINT_MARKER_PATTERN).exec(body) || [""])[0]; |
There was a problem hiding this comment.
Dead code: At this point in the control flow, parseCheckpointMarker(body) has already returned a non-null payload (line 2431), which guarantees the body contains exactly one valid marker. Therefore new RegExp(CHECKPOINT_MARKER_PATTERN).exec(body) will always find a match, making the || [""] fallback unreachable. The line can be simplified:
Suggestion:
| const raw = (new RegExp(CHECKPOINT_MARKER_PATTERN).exec(body) || [""])[0]; | |
| const raw = new RegExp(CHECKPOINT_MARKER_PATTERN).exec(body)[0]; |
4792ae5 to
fca024b
Compare
On every push the reusable Action re-reviews the whole merge-base range, so later pushes on a large PR cost the same as the first one. This adds an opt-in `checkpoint_range` input that records the last completely reviewed head in the sticky summary and narrows the next run to checkpoint..head. Every gate fails closed to a full review: force-push, base change, config or rule change, incomplete prior run, or any resolver error. The config fingerprint hashes one environment variable per axis instead of a single "|"-joined string. The joined form let a value containing the separator shift the field boundaries, so `route_severity_below=low|a` with an empty `route_categories` hashed identically to `low` with `route_categories=a|` — a checkpoint surviving a config change that should have invalidated it. It also made the digest depend on how the YAML happened to be wrapped.
fca024b to
e7dcf92
Compare
2 participants
Description
Closes #476. The reusable Action's
incrementalinput suppresses duplicate comments but does not narrow the range: every push still invokes OCR over the full merge-base to head diff. On the production run cited in the issue, a push of 7 files and 323 lines was reviewed as 63 files and 15,700 lines, taking 46:31 and 3,981,022 tokens.This adds an opt-in
checkpoint_rangeinput (default'false'). A run that completes records its reviewed head in a base64 marker appended to the sticky summary comment. The next push reads that marker and reviewscheckpoint..headinstead ofmerge-base..head. A second input,full_review, forces the full range on demand.The marker is only trusted when every gate agrees, and any doubt widens back to the full range rather than narrowing. In order: sticky summary off, manual override, reopened / ready_for_review, no summary comment yet, the comment was not written by a bot identity GitHub attests, the marker is absent or malformed or duplicated, the payload is for another PR or another marker version or a run that did not complete, the base ref or merge-base moved, the config fingerprint changed (model, language,
llm_extra_body,background, routing, resolved OCR version, and the contents of bothruleand.opencodereview/rule.json), the checkpoint is not an ancestor of the new head, the commit is not in this clone, a rule file could not be read, or the resolver itself threw. Nine outputs (range_mode,range_reason,range_from,range_to,checkpoint_before,checkpoint_after,ancestry,source_run,range_summary) report which of those happened.Writing the checkpoint is gated separately: it advances only when the manifest reports
terminal_state: complete, nothing failed to post, and the summary comment actually published. A canceled or half-failed run carries the previous checkpoint forward unchanged, so the next run covers the range it never read. A rerun on an unchanged head reportssame_head_noopand leaves the existing summary untouched instead of replacing its findings with "No comments generated".Limitations
pull_request_targetexists to distrust, posts as aUserand is rejected. The README says this plainly rather than burying it.terminal_state: completemeans nothing the run selected failed. Waived items and files excluded before selection (unsupported types, size limits) sit inside that guarantee, so a checkpoint claims "everything this configuration chose to review was reviewed", not "every byte was read".Type of Change
How Has This Been Tested?
make testpasses locallynode scripts/github-actions/post-review-comments.test.jsandnode scripts/github-actions/check-translation-sync.test.jsboth green, andaction.ymlparses as YAML.The new coverage is a table over the resolver covering each gate and each ordering between them, plus round-trip cases that write a marker and read it back, force-push and base-change fallbacks, the same-head no-op, and the marker-preservation path that stops a completing run from blanking a checkpoint it could not re-derive. With
checkpoint_rangeleft at its default, the existing outputs are asserted unchanged and no extra API call is made.Checklist
go fmt,go vet)Related Issues
Closes #476