Repository navigation
Conversation
|
🔍 OpenCodeReview found 5 issue(s) in this PR.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| inputs: | ||
| mode: | ||
| description: "all runs and posts a review; review saves it; post publishes a saved review in the same job." |
There was a problem hiding this comment.
The description states that post "publishes a saved review in the same job", but the split-mode feature is specifically designed to allow review and post to run in different jobs (or even different workflows) via the shared state file. The phrase "in the same job" is misleading and may cause users to think cross-job usage is unsupported.
Consider rephrasing to clarify that review and post can run in separate jobs or steps.
Suggestion:
| description: "all runs and posts a review; review saves it; post publishes a saved review in the same job." | |
| description: "all runs and posts a review; review saves it; post publishes a saved review. review and post can run in separate jobs or steps." |
| function identity(context) { | ||
| return { | ||
| repository: `${context.repo.owner}/${context.repo.repo}`, | ||
| run: String(context.runId), |
There was a problem hiding this comment.
Bug: context.runId is not null-checked before being converted to string. If context.runId is undefined or null, String(undefined) produces the literal string "undefined", which would silently match across different runs that also lack a runId, defeating the purpose of identity verification.
Note that post-review-comments.js (line 218) guards against this with context.runId != null ? context.runId : "". Apply the same defensive pattern here.
Suggestion:
| run: String(context.runId), | |
| run: String(context.runId != null ? context.runId : ""), |
|
|
||
| function saveReviewState({ statePath, context, headSha, resultPath, stderrPath, options, outputs = {} }) { | ||
| const result = fs.readFileSync(resultPath, "utf8"); | ||
| JSON.parse(result); |
There was a problem hiding this comment.
Bug: This JSON.parse(result) is used solely for validation (the parsed value is discarded), but it throws an unhandled exception with a raw error message if the result file contains malformed JSON. This crashes the action abruptly.
Meanwhile, runPostReviewComments in post-review-comments.js (lines 262-283) already parses the same result file with a proper try-catch that posts a user-friendly error summary to the PR when parsing fails. This eager validation is therefore both redundant and counterproductive — it prevents the graceful error handling downstream from ever executing.
Remove this line entirely, or wrap it in a try-catch that provides a meaningful error message.
|
|
||
| async function postReviewState({ statePath, github, context, core }) { | ||
| const state = JSON.parse(fs.readFileSync(statePath, "utf8")); | ||
| if (state.version !== 1 || JSON.stringify(state.identity) !== JSON.stringify(identity(context))) { |
There was a problem hiding this comment.
Maintainability: Comparing objects via JSON.stringify relies on key insertion order being identical between the saved identity and the freshly constructed one. While this works today because both objects are created by the same identity() function with hardcoded key order, it is fragile — any future refactoring (e.g., spreading additional properties, reordering keys) could cause valid identities to be rejected.
Consider a field-by-field comparison instead:
const saved = state.identity;
const current = identity(context);
if (saved.repository !== current.repository || saved.run !== current.run ||
saved.attempt !== current.attempt || saved.job !== current.job) {
throw new Error(...);
}| fs.writeFileSync(resultPath, state.result, { mode: 0o600 }); | ||
| fs.writeFileSync(stderrPath, state.stderr, { mode: 0o600 }); | ||
| await runPostReviewComments({ ...state.options, github, context, core, fs, resultPath, stderrPath }); | ||
| for (const [key, value] of Object.entries(state.outputs || {})) core.setOutput(key, value); |
There was a problem hiding this comment.
Unlike runPostReviewComments which defensively guards core.setOutput with if (core && typeof core.setOutput === "function"), this direct call has no such guard. If core is missing or lacks setOutput, this will throw after the review comments have already been successfully posted, producing a misleading failure. Consider adding the same defensive check for consistency and robustness.
Suggestion:
| for (const [key, value] of Object.entries(state.outputs || {})) core.setOutput(key, value); | |
| for (const [key, value] of Object.entries(state.outputs || {})) { | |
| if (core && typeof core.setOutput === "function") core.setOutput(key, value); | |
| } |
| mode: | ||
| description: "all runs and posts a review; review saves it; post publishes a saved review. Transfer the state file when using separate jobs or workflows." | ||
| required: false | ||
| default: all | ||
| state_path: | ||
| description: "Saved review file shared by review and post calls. Use a separate path for each review in a job." | ||
| required: false | ||
| default: ${{ runner.temp }}/ocr-review-state.json |
There was a problem hiding this comment.
Nit: those should be of type string.
Github Actions treat inputs and default inputs as string anyway, but the rest of the action.yml quotes default vals all in strings'
| mode: | |
| description: "all runs and posts a review; review saves it; post publishes a saved review. Transfer the state file when using separate jobs or workflows." | |
| required: false | |
| default: all | |
| state_path: | |
| description: "Saved review file shared by review and post calls. Use a separate path for each review in a job." | |
| required: false | |
| default: ${{ runner.temp }}/ocr-review-state.json | |
| mode: | |
| description: "all runs and posts a review; review saves it; post publishes a saved review. Transfer the state file when using separate jobs or workflows." | |
| required: false | |
| default: 'all' | |
| state_path: | |
| description: "Saved review file shared by review and post calls. Use a separate path for each review in a job." | |
| required: false | |
| default: '${{ runner.temp }}/ocr-review-state.json' |
This branch has not been deployed
2 participants
Description
Adds
mode: all|review|postto the composite action so a caller can generate a fresh GitHub App installation token after a long review and before publishing its comments.allremains the default single-call workflow.reviewsaves the result and publication/checkpoint settings without configuration credentials;postpublishes that state without requiring LLM configuration, checkout, or OCR installation.Saved state is written atomically with restricted permissions, scoped to the repository, workflow run, attempt and job, and rejected when the PR head changes. The action retains its existing publication helper, checkpoint behavior and outputs. The example documents same-job usage and separate state paths for multiple reviews.
Type of Change
How Has This Been Tested?
On commit
d5fac09ba602bb25d6ae989bc36cdb31f3da6f56with Go 1.26.8 and Node 22.16.0:npm run test:github-actionspassed, including 50 action contracts and saved-state round-trip, identity and stale-head checks.npm run test:launcher,make check,make buildandgit diff --checkpassed.make coveragepassed at 91.1% (90% required).make testpassed, including the race-enabled unit suite.ocr review --audience agent --background ...was attempted and failed because no LLM endpoint is configured.Checklist
go fmt,go vet)AI-assisted implementation and technical review used Codex (GPT-6.1-sol); repository context gathering used GPT-6-luna.
The CLA check reports: "Contributor License Agreement is signed."
Related Issues
Closes #1636.