Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe OCR review workflow now records phase, policy, and infrastructure diagnostics instead of hard-failing on OCR setup, preview, parse, or posting issues. Sticky summary posting and the infrastructure notification job were rewritten to sanitize output, deduplicate comments, and use the new artifacts. New test helpers and Vitest coverage validate the workflow and sanitization behavior. ChangesOCR workflow refactor and diagnostics
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
LLxprt PR Review – PR #2438Issue AlignmentThe PR implements the requested behavior from #2431: OCR infrastructure failures are now non-blocking, while changed-test scope failures remain policy failures that fail the check. It preserves inline review comments and the sticky summary. Evidence: job outputs classify infrastructure vs policy failures; changed-test preview failures write Side Effects
Code Quality
Tests and Coverage
VerdictReady — PR resolves #2431, includes meaningful automated tests, and preserves required policy enforcement. Minor follow-ups: consider extracting large workflow scripts to reusable actions for maintainability, and validate redaction coverage against actual provider stderr samples. |
![github-actions[bot]](https://avatars.github.com/in/15368?s=60&v=4){kind=small}
| needs: code-review | ||
| # Skipped/cancelled superseded OCR runs should not create infrastructure issues; | ||
| # completed green runs still check artifacts because OCR runtime failures are non-blocking. | ||
| if: ${{ !cancelled() && needs.code-review.result == 'success' }} |
There was a problem hiding this comment.
The
notify-ocr-infrastructure-failurejob condition only triggers whenneeds.code-review.result == 'success'. Since all infrastructure failures in thecode-reviewjob now useexit 0(soft failure), the job reportssuccessand the notify job fires correctly for OCR-level infrastructure failures. However, if thecode-reviewjob fails for an unexpected reason (e.g., checkout failure, pr-context script error, uncaught exception in the Post step that isn't wrapped in try-catch), the job result will befailureand the notify job will be skipped entirely — even if an infrastructure failure was recorded in artifacts before the unexpected failure occurred. Consider also triggering onfailure(e.g.,if: ${{ !cancelled() && (needs.code-review.result == 'success' || needs.code-review.result == 'failure') }}), or at minimum documenting this gap so maintainers know unexpected workflow failures won't produce infrastructure issue notifications.
There was a problem hiding this comment.
Addressed in commit 29fcf74: the notify job now also runs when code-review reports failure, so unexpected post-artifact failures can still create the deduplicated infrastructure diagnostic issue.
| ocr-phase.txt | ||
| ocr-infrastructure-failure.txt | ||
| ocr-policy-failure.txt | ||
| if-no-files-found: warn |
There was a problem hiding this comment.
Changing
if-no-files-foundfromerrortowarnmeans that if all diagnostic files are missing (e.g., because theInitialize OCR artifact filesstep didn't run due to an earlier catastrophic failure), the upload step will only emit a warning and the workflow will continue. Thenotify-ocr-infrastructure-failurejob then downloads an empty or non-existent artifact directory and silently skips notification (if [ -d ocr-review-output ]fails → returns 0). This could mask real infrastructure failures where artifacts were never created. Consider keepingif-no-files-found: errorfor the critical diagnostic files (ocr-exit-code.txt, ocr-phase.txt, ocr-infrastructure-failure.txt) or adding a post-download validation step in the notify job that fails loudly when expected artifacts are missing.
There was a problem hiding this comment.
Addressed in commit 29fcf74: the notify job now treats missing artifacts after a failed code-review job, or missing critical diagnostic files after a failed code-review job, as infrastructure diagnostics instead of silently skipping notification.
| it('notifies a deduplicated ci/cd issue for OCR infrastructure errors', () => { | ||
| const notifyJob = workflow.jobs?.['notify-ocr-infrastructure-failure']; | ||
| expect(notifyJob?.needs).toBe('code-review'); | ||
| expect(notifyJob?.['timeout-minutes']).toBe(5); |
There was a problem hiding this comment.
The
code-reviewjob does not specifytimeout-minutes. GitHub Actions defaults to 360 minutes (6 hours) when this is omitted. For a code review job that should complete in a few minutes, a 6-hour ceiling is excessive — if the OCR CLI or a workflow step hangs (e.g., network stall waiting on the LLM endpoint, a stucknpm install, or a deadlock in the github-script step), the job will consume runner minutes for hours before timing out. The siblingnotify-ocr-infrastructure-failurejob correctly setstimeout-minutes: 5. Consider adding a reasonabletimeout-minutes(e.g., 15–30) to thecode-reviewjob as well.
There was a problem hiding this comment.
Addressed in commit 29fcf74: code-review now has a bounded timeout-minutes value, and the workflow contract test asserts it.
OpenCodeReview — PR #2438
|
| 'script should define REDACTION constant', | ||
| ).toBeTruthy(); | ||
| const source = [ | ||
| `const REDACTION = '${redactionMatch?.[1] ?? ''}';`, |
There was a problem hiding this comment.
The optional chaining
?.is redundant here. The precedingexpect(redactionMatch).toBeTruthy()assertion guaranteesredactionMatchis truthy, soredactionMatch[1]can be accessed directly. Consider usingredactionMatch[1] ?? ''for clarity.
There was a problem hiding this comment.
Addressed in commit 29fcf74: removed the redundant optional chaining after the regex match assertion.
| echo "::warning::Failed to create OCR infrastructure issue body file." | ||
| return 1 | ||
| fi | ||
| trap 'rm -f "$body_file"' EXIT RETURN |
There was a problem hiding this comment.
The
trap 'rm -f "$body_file"' EXIT RETURNwill fire the RETURN trap every time ANY shell function returns (not justnotify_ocr_infrastructure_failure). Whenretry_ghreturns at line 1070 (recheck search) or insidecreate_infrastructure_issueat line 1040, the RETURN trap fires and deletes$body_filebefore it can be used at line 1081 (gh issue comment --body-file "$body_file") or line 1084 (create_infrastructure_issue "$body_file"). This means the body file will be missing when it's needed most — during the fallback paths after the initial search.Suggestion: Remove
RETURNfrom the trap and keep onlyEXIT, or move the trap to only cover the end of the function scope by setting it after all uses of$body_fileare complete. Alternatively, set the trap without RETURN and add an explicitrm -f "$body_file"before eachreturn.
There was a problem hiding this comment.
Addressed in commit 29fcf74: removed RETURN from the trap so nested helper returns cannot delete the issue body file before gh issue comment or create uses it.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-24.x-ubuntu-latest' artifact from the main CI run. |
| const source = [ | ||
| `const REDACTION = '${redactionMatch[1] ?? ''}';`, |
There was a problem hiding this comment.
The fallback
redactionMatch[1] ?? ''only guards againstnull/undefined. If the REDACTION constant is accidentally emptied (e.g.,const REDACTION = '';), the captured group would be''(not null), so the??won't trigger. This would inject an empty REDACTION constant into the VM, causingredactSecretDiagnosticsto silently produce no redactions — masking a security regression. Consider validating that the captured value is non-empty, e.g.,expect(redactionMatch[1], 'REDACTION constant should be non-empty').toBeTruthy();before using it.
There was a problem hiding this comment.
Addressed in commit 10d8b31: the helper now asserts the REDACTION constant is non-empty before constructing the VM source.
| export function runNotifySanitizer(notifyRun, input, token) { | ||
| try { | ||
| return executeNotifySanitizer(notifyRun, input, token).replace(/\n$/, ''); |
There was a problem hiding this comment.
runNotifySanitizerdoes not forwardextraEnvtoexecuteNotifySanitizer, so callers cannot test URL redaction (viaOCR_LLM_URL) through this wrapper. This is inconsistent withmakePostSanitizer, which accepts aurlparameter. Consider adding anextraEnvparameter (or aurlparameter) torunNotifySanitizerand passing it through, so the notify-path redaction tests can verify URL redaction just like the post-path tests do.
There was a problem hiding this comment.
Addressed in commit 10d8b31: runNotifySanitizer now forwards extra environment values, and the notify sanitizer has URL redaction coverage.
| ...stderrSection('OCR stderr excerpt', 'ocr-stderr.log'), | ||
| '', | ||
| ...stderrSection('OCR preview stderr excerpt', 'ocr-preview-stderr.log'), |
There was a problem hiding this comment.
Security Risk: The
stderrSectionfunction posts up to 40 lines / 4000 chars of raw stderr content into PR comments after applyingredactSecretDiagnostics. The redaction relies on regex patterns for known secret formats (Authorization headers, api_key=, token=, etc.) plus exact-string matching forOCR_LLM_TOKENandOCR_LLM_URL. However, if the OCR CLI or npm logs the LLM token or URL in a format not covered by these regexes (e.g., as a URL query parameter like?key=..., inside a JSON payload, within a stack trace, or as a bare value on a line), the secret would be publicly exposed in the PR comment visible to all collaborators.Suggestion: Consider one or more of the following mitigations:
- Only include stderr excerpts when an infrastructure failure is actually detected (i.e., gate
stderrSectioncalls behindif (infrastructureFailure)orif (!ran)).- Add additional regex patterns for common LLM API URL formats (e.g.,
\b(key|api[_-]?key|token)\s*[=&]\s*[^\s&]+for query parameters).- Apply a stricter approach: only post stderr lines that match known diagnostic patterns rather than posting a broad excerpt of all stderr content.
There was a problem hiding this comment.
Addressed in commit 10d8b31: stderr excerpts are now only added to the PR summary when OCR did not run cleanly or an infrastructure diagnostic exists, and query-parameter key/token patterns are redacted.
| mark_policy_failure() { | ||
| echo "$1" > ocr-policy-failure.txt | ||
| : > ocr-infrastructure-failure.txt | ||
| } |
There was a problem hiding this comment.
Logic Issue:
mark_policy_failuretruncatesocr-infrastructure-failure.txtwith: > ocr-infrastructure-failure.txt, which silently erases any infrastructure failure diagnostic recorded earlier in the run (e.g., during the install step). If both an infrastructure failure and a policy failure occur in the same run, the infrastructure diagnostic is lost, making root-cause debugging harder and potentially causing the notify job to misclassify the failure as policy-only.Suggestion: Preserve the infrastructure failure diagnostic instead of truncating it, or append the policy failure context to a combined diagnostic.
There was a problem hiding this comment.
Addressed in commit 10d8b31: policy failure marking no longer truncates the infrastructure diagnostic file.
| ocr-phase.txt | ||
| ocr-infrastructure-failure.txt | ||
| ocr-policy-failure.txt | ||
| if-no-files-found: warn |
There was a problem hiding this comment.
Reliability Issue:
if-no-files-foundwas changed fromerrortowarn. While this prevents job failure when artifacts are missing (expected when early setup fails), it also masks cases where expected files likeocr-result.jsonshould exist but don't after a successful OCR run. This reduces visibility into artifact collection failures.Suggestion: If the intent is to allow missing artifacts during degraded runs, consider keeping
warnbut add a follow-up check or log message that flags whenocr-result.jsonis missing andexitCode === 0(i.e., a successful run that should have produced output).
There was a problem hiding this comment.
Addressed in commit 10d8b31: the notify job now treats a missing OCR result artifact after a zero-exit review as an infrastructure diagnostic.
| create_infrastructure_issue() { | ||
| local issue_body_file | ||
| issue_body_file="$1" | ||
| shift | ||
| if retry_gh gh issue create "$@" --body-file "$issue_body_file" --label "ci/cd"; then | ||
| return 0 | ||
| fi | ||
| echo "::warning::Failed to create OCR infrastructure issue with ci/cd label; retrying without labels." | ||
| retry_gh gh issue create "$@" --body-file "$issue_body_file" || echo "::warning::Failed to create OCR infrastructure issue." | ||
| } |
There was a problem hiding this comment.
The
notify-ocr-infrastructure-failurejob doescd ocr-review-output(line 978) before callinggh issue createandgh issue commentwithout the--repoflag. TheghCLI infers the target repository from the git remote of the current working directory, butocr-review-outputis a downloaded-artifact subdirectory with no.gitrepo. This meansghwill either fail to determine the repository or walk up to a parent.gitthat may not correspond to${{ github.repository }}. Pass--repo "${{ github.repository }}"explicitly (or setGH_REPOenv var) to allgh issue createandgh issue commentcalls to ensure issues are always created in the correct repository.
There was a problem hiding this comment.
Addressed in commit af0f119: the notify step now sets GH_REPO from github.repository so gh issue commands resolve the intended repository even after changing directories.
| retry_gh gh issue comment "${EXISTING_ISSUE}" --body-file "$body_file" || echo "::warning::Failed to comment on OCR infrastructure issue." | ||
| return 0 |
There was a problem hiding this comment.
Similarly,
gh issue commentcalls lack--repo. Since the working directory was changed toocr-review-output, theghCLI may not correctly resolve the target repository. Add--repoto bothgh issue commentcalls.
There was a problem hiding this comment.
Addressed in commit af0f119: GH_REPO is now set for the notify step, which covers the gh issue comment calls as well.
| if (length($secret) > 0) { | ||
| s/\Q$secret\E/$redaction/g; | ||
| } | ||
| if (length($url) > 0) { | ||
| s/\Q$url\E/$redaction/g; | ||
| } |
There was a problem hiding this comment.
The Perl redaction uses
\Q$secret\Efor literal escaping. If the secret value itself contains the sequence\E, Perl will terminate the literal quoting early, causing the remainder of the secret to be interpreted as regex metacharacters. This could result in regex errors (causing sanitization to fail) or incomplete redaction, potentially leaking the secret into a GitHub issue body that is publicly visible. Consider handling\Ewithin the secret by splitting and re-quoting, or use a non-regex string replacement approach (e.g., iterating character by character or usingindex/substr).
There was a problem hiding this comment.
Addressed in commit af0f119: the Perl sanitizer now uses quotemeta before exact-value substitution, and the notify redaction test covers a secret containing the E-escape sequence.
| OCR_LLM_TOKEN: ${{ secrets.OCR_LLM_AUTH_TOKEN }} | ||
| OCR_LLM_URL: ${{ vars.OCR_LLM_URL }} | ||
| with: | ||
| script: | |
There was a problem hiding this comment.
The
OCR_LLM_TOKENsecret is now passed to thePost OCR resultsgithub-script step's environment, which previously did not receive it. While it is used for redaction purposes, this increases the secret's exposure surface — if the script encounters an unhandled exception and dumpsprocess.env, or if a future modification inadvertently logs environment variables, the secret would be exposed in the workflow logs. Consider whether the redaction can be performed without having the raw secret in this step's environment, or add a comment documenting why this is necessary and thatprocess.envmust never be logged from this step.
There was a problem hiding this comment.
Addressed in commit af0f119: added an explicit workflow note that the OCR endpoint and credential environment values are provided only for redaction and process.env must not be logged from that step.
| elif [ "${CODE_REVIEW_RESULT:-}" = "failure" ]; then | ||
| phase="artifact" | ||
| infra_failure="phase=artifact; reason=OCR artifacts were unavailable after code-review failed" | ||
| echo "::warning::OCR artifacts were unavailable after code-review failed; creating infrastructure notification." |
There was a problem hiding this comment.
The
notify-ocr-infrastructure-failurejob'selifbranch creates a false-positive infrastructure issue whenCODE_REVIEW_RESULT == "failure"and artifacts are unavailable. If the code-review job failed due to a policy failure (e.g.,exit 1in the preview step at line 393 or 401) but the artifact upload itself failed or was skipped, thepolicy_failurevariable will be empty (since artifacts are unavailable), theinfra_failurewill be set to 'OCR artifacts were unavailable after code-review failed', and the early-return policy check at line 1022 won't trigger. This results in a misleading infrastructure failure issue being created for what was actually a policy violation.Suggestion: Before entering the
elifbranch, consider whether the code-review job could have failed for policy reasons. One approach: pass an additional output from thecode-reviewjob (viaoutputs:in the job definition) indicating whether it was a policy failure, and check that output here before creating an infrastructure issue.
There was a problem hiding this comment.
Addressed in commit 18cea0a: code-review now exposes a policy_failure job output and the notify job checks it before creating missing-artifact infrastructure notifications.
| mark_infrastructure_failure() { | ||
| echo "phase=$1; reason=$2" > ocr-infrastructure-failure.txt | ||
| } | ||
| OCR_PREFIX="${RUNNER_TEMP}/ocr-${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT}" |
There was a problem hiding this comment.
The
mark_infrastructure_failurebash function is defined identically in four separate steps (install, validate, preview, review). This violates DRY — if the function signature or behavior needs to change, it must be updated in four places. Consider extracting it into a shared script file or sourcing a common helper to reduce maintenance burden and the risk of drift between implementations.
There was a problem hiding this comment.
Addressed in commit 18cea0a: the repeated mark_infrastructure_failure shell function is now generated once in the initialization step and sourced by the OCR install, validate, preview, and review steps.
| ocr-phase.txt | ||
| ocr-infrastructure-failure.txt | ||
| ocr-policy-failure.txt | ||
| if-no-files-found: warn |
There was a problem hiding this comment.
The
if-no-files-foundsetting was changed fromerrortowarn. This means if the job fails so early that no artifact files are produced, the workflow will not fail on the upload step, and the downstreamnotify-ocr-infrastructure-failurejob will receive no artifacts. While the notify job handles missing artifacts, reducing the upload failure fromerrortowarndecreases visibility into cases where the workflow is fundamentally broken and produces no output at all. Consider keepingif-no-files-found: error(or at minimumwarnwith an additional explicit check) to surface early-stage failures more prominently.
There was a problem hiding this comment.
Addressed in commit 18cea0a: retained warning-only artifact upload per issue scope, with explicit notify-job checks for missing diagnostics, zero-exit missing OCR result artifacts, and policy-failure classification to avoid misleading infrastructure issues.
| it('redacts exact OCR secrets with regex metacharacters and backslashes in notify diagnostics', () => { | ||
| const notifyRun = commandText( | ||
| stepNamed( | ||
| workflow.jobs?.['notify-ocr-infrastructure-failure'], | ||
| 'Notify OCR infrastructure failure issue', | ||
| ), | ||
| ); |
There was a problem hiding this comment.
The pattern
commandText(stepNamed(workflow.jobs?.['notify-ocr-infrastructure-failure'], 'Notify OCR infrastructure failure issue'))is repeated 6 times across tests (lines 364-369, 383-388, 405-410, 425-430, 440-444, 716-720). Consider extracting the notify job and notify step into shared variables in thebeforeAllblock (e.g.,notifyJobandnotifyStep), similar to howcodeReviewJobis already shared. This reduces duplication and makes the tests more maintainable.
There was a problem hiding this comment.
Addressed in commit 4178e53: notify job, notify step, and notify script text are now initialized once in beforeAll and reused by the workflow tests.
| 'REDACTION constant should be non-empty', | ||
| ).toBeTruthy(); | ||
| const source = [ | ||
| `const REDACTION = '${redactionMatch[1]}';`, |
There was a problem hiding this comment.
The REDACTION value extracted from
redactionMatch[1]is interpolated directly into a JavaScript string literal without escaping. If the value ever contains a backslash (e.g.,\or\n), the string's semantics change when re-interpreted in the VM context — a literal\nbecomes a newline escape,\'could break the string boundary, etc. While the current[REDACTED]constant is safe, this is a latent injection/semantic-corruption bug. Consider usingJSON.stringify(redactionMatch[1])to safely embed the value, e.g.,const REDACTION = ${JSON.stringify(redactionMatch[1])};.
There was a problem hiding this comment.
Addressed in commit 4178e53: the extracted REDACTION constant is now embedded with JSON.stringify before running the sanitizer in the VM context.
| function startsRegexLiteral(source, index) { | ||
| return '({[=,:;!&|?+-*%^~<>)]'.includes( | ||
| previousSignificantChar(source, index), | ||
| ); | ||
| } |
There was a problem hiding this comment.
previousSignificantCharreturns''when no significant character is found (e.g., at the start of source). InstartsRegexLiteral,'({[=,:;!&|?+-*%^~<>)]'.includes('')always returnstruebecauseString.prototype.includes('')matches any string. This accidentally produces the correct result (a/at the start of source should start a regex), but the behavior is non-obvious and fragile. IfpreviousSignificantCharis ever refactored to returnnullorundefined,includes(null)/includes(undefined)would returnfalse, silently breaking regex detection. Consider returningnullfrompreviousSignificantCharand explicitly checking for it:const prev = previousSignificantChar(source, index); return prev === null || '({[=,:;!&|?+-*%^~<>)]'.includes(prev);
There was a problem hiding this comment.
Addressed in commit 4178e53: previousSignificantChar now returns null when no character exists, and startsRegexLiteral handles that case explicitly.
| concurrency: | ||
| group: >- | ||
| ocr-review-${{ github.event.pull_request.number || github.event.issue.number || inputs.pr_number }} | ||
| cancel-in-progress: true |
There was a problem hiding this comment.
If the
code-reviewjob is missing from the workflow YAML,codeReviewJobwill beundefined. Tests that directly accesscodeReviewJob.concurrency(line 49) would throw an unhelpfulTypeError: Cannot read properties of undefinedinstead of a clear assertion failure. Add an existence check here, consistent with the file's other defensive assertions (e.g., theworkflow && typeof workflow === 'object'check above).
There was a problem hiding this comment.
Addressed in commit 4178e53: beforeAll now asserts that the code-review job exists before any tests access its properties.
| const notifyStep = stepNamed( | ||
| notifyJob, | ||
| 'Notify OCR infrastructure failure issue', | ||
| ); | ||
| const notifyRun = normalize(commandText(notifyStep)); |
There was a problem hiding this comment.
Variable shadowing:
notifyStepandnotifyRunare redeclared as localconstinside this test, shadowing the outerletvariables with the same names frombeforeAll. This is confusing — readers may think the test uses the outer-scope variables, and it increases the risk of bugs if the test is modified. The outernotifyRunis not normalized (rawcommandTextoutput), while this local one is normalized vianormalize(commandText(notifyStep)). The subsequent test'uses UTC dates, backoff retries, and label fallback for infrastructure issues'at line 697 uses the outernotifyRun(non-normalized) and appliesnormalize()separately. Consider renaming the local variables (e.g.,localNotifyStep,normalizedNotifyRun) to avoid shadowing and make the scope distinction explicit.
There was a problem hiding this comment.
Addressed in commit HEAD after the latest amend: the test now uses the shared notifyStep and notifyRun variables instead of redeclaring/shadowing them locally.
| if [ "$status" -ne 0 ]; then | ||
| mark_infrastructure_failure "review" "OCR review command failed" | ||
| fi | ||
| exit 0 |
There was a problem hiding this comment.
All infrastructure failures (OCR installation, command-not-found, version check, configuration validation, preview command failure, and review command failure) use
exit 0with::warning::instead of failing the job. Only policy failures (changed test files missing/excluded from review) callcore.setFailed. This means if OCR cannot install or the LLM endpoint is unreachable, the workflow will succeed silently and PRs could merge without any automated code review — defeating the entire purpose of this workflow.Consider making infrastructure failures fail the job (e.g.,
core.setFailed) or at minimum fail the check, while keeping the diagnostic posting and artifact upload viaif: always(). Thenotify-ocr-infrastructure-failurejob is a good mitigation for visibility, but it does not prevent unreviewed PRs from merging.
There was a problem hiding this comment.
Not changed: issue 2431 explicitly requires OCR infrastructure/provider/script failures to be non-blocking PR checks. The workflow still surfaces these via warnings, sticky PR diagnostics, artifacts, and a deduplicated ci/cd issue notification, while changed-test policy failures remain blocking.
| ocr-phase.txt | ||
| ocr-infrastructure-failure.txt | ||
| ocr-policy-failure.txt | ||
| if-no-files-found: warn |
There was a problem hiding this comment.
The
if-no-files-foundwas changed fromerrortowarn. If the workflow's own diagnostic files (ocr-exit-code.txt, ocr-phase.txt, etc.) are missing, the artifact upload silently succeeds with a warning. This reduces visibility when the error-tracking mechanism itself fails, making it harder to debug infrastructure failures through artifacts. Since this step runs withif: always()after diagnostic steps, most files should exist — but if they don't, that itself is a signal worth surfacing as an error.
There was a problem hiding this comment.
Not changed: issue 2431 requires missing/pre-initialization artifact cases to avoid failing the PR check. Visibility is preserved by the notify job, which detects missing or incomplete OCR diagnostics and opens/comments on the deduplicated ci/cd infrastructure issue.
| } catch (error) { | ||
| const stderr = error.stderr ? String(error.stderr) : ''; | ||
| throw new Error(`Notify sanitizer execution failed. stderr:\n${stderr}`, { | ||
| cause: error, | ||
| }); | ||
| } |
There was a problem hiding this comment.
The thrown error message only includes stderr text but omits
error.statusanderror.code, which are valuable for debugging failed sanitizer executions (e.g., distinguishing a non-zero exit from a signal termination). Consider including these in the error message.
There was a problem hiding this comment.
Addressed in the latest amended commit: notify sanitizer failures now include status, code, signal, and stderr in the thrown error, with a regression assertion covering those fields.
| '', | ||
| ); | ||
| } catch (error) { | ||
| const stderr = error && error.stderr ? String(error.stderr) : ''; |
There was a problem hiding this comment.
Inconsistent null-checking pattern: the catch block accesses
error.stderrdirectly viaerror && error.stderr ? String(error.stderr) : '', while the very next lines use the dedicatederrorField()helper forstatus,code, andsignal. The direct access pattern bypasses thetypeof error === 'object'andfieldName in errorguards thaterrorFieldprovides. For consistency and safety, useerrorField(error, 'stderr')here as well.
There was a problem hiding this comment.
Addressed in the latest amended commit: stderr extraction now uses the same guarded errorField helper as status, code, and signal.
| .replace(/\b(token\s*[=:]\s*)([^\s,;&]+)/gi, '$1[REDACTED]') | ||
| .replace(/\b(secret\s*[=:]\s*)([^\s,;&]+)/gi, '$1[REDACTED]'); |
There was a problem hiding this comment.
The generic redaction patterns
\b(token\s*[=:]\s*)and\b(secret\s*[=:]\s*)are overly broad and will false-positive on non-credential contexts in diagnostic output. For example, a stderr log line liketoken=expiredorsecret mode=enabledwould be redacted even though these are not secrets. This could make infrastructure failure diagnostics confusing or unhelpful when troubleshooting. Consider tightening these patterns (e.g., requiring a minimum length for the matched value, or restricting to more specific contexts likeapi_token=,auth_token=,client_secret=) to reduce false positives while still catching common credential leaks.
There was a problem hiding this comment.
Addressed in the latest amended commit: the JS sanitizer now requires generic token and secret values to look credential-like with a minimum length, while preserving exact-secret and specific token-pattern redaction.
| s/\b(token\s*[=:]\s*)([^\s,;&]+)/$1$redaction/gi; | ||
| s/\b(secret\s*[=:]\s*)([^\s,;&]+)/$1$redaction/gi; |
There was a problem hiding this comment.
The same overly broad
token=andsecret=patterns are duplicated in the Perl sanitizer. Consider applying the same tightening here for consistency with the JS redaction function.
There was a problem hiding this comment.
Addressed in the latest amended commit: the Perl sanitizer now applies the same tightened generic token and secret value pattern as the JS sanitizer.
| echo "install" > ocr-phase.txt | ||
| . ./ocr-workflow-helpers.sh | ||
| OCR_PREFIX="${RUNNER_TEMP}/ocr-${GITHUB_RUN_ID}-${GITHUB_RUN_ATTEMPT}" | ||
| if ! npm install --prefix "$OCR_PREFIX" --ignore-scripts @alibaba-group/open-code-review@1.6.1 2>> ocr-stderr.log; then |
There was a problem hiding this comment.
The npm install step does not use any caching mechanism. Each workflow run performs a fresh
npm install --prefixwhich downloads the package and its dependencies. Given the 45-minute timeout and that this runs on every PR review trigger, adding npm caching (e.g.,actions/cachekeyed on the package name and version, ornpm's built-in cache) could improve workflow execution time and reduce flakiness from transient network issues during package download.
There was a problem hiding this comment.
Not changed: deterministic per-run installation under RUNNER_TEMP is part of the issue 2431 requirement, and adding a new cache layer is a broader performance optimization outside this fix. The install remains pinned and non-updating.
| 'Authorization\\s*:\\s*(?:(?:Bearer|Basic|token|ApiKey)\\s+)?', | ||
| 'x-api-key\\s*:\\s*', | ||
| 'api[_-]?key\\s*[=:]\\s*', | ||
| '[?&](?:key|api[_-]?key|token)=', | ||
| 'token\\s*[=:]\\s*', | ||
| 'access[_-]?token\\s*[=:]\\s*', |
There was a problem hiding this comment.
The
expectContainsAllassertion list for the PR diagnostics script is missing therefresh[_-]?token,id[_-]?token, andsecretregex patterns that are present in the workflow YAML (lines 508-511) and tested for redaction viaexpectCommonCredentialsRedactedin the helper file. The same omission exists in the notify diagnostics test at line 437-453. If someone removes these three redaction regex patterns from the workflow YAML, the structuralexpectContainsAlltests won't catch it directly — only the functional redaction tests would catch it indirectly. For consistency and completeness, add these patterns to both assertion lists.
There was a problem hiding this comment.
Addressed in the latest amended commit: the PR diagnostic structural assertions now include refresh token, ID token, secret, and the tightened generic credential-value pattern.
| 'Authorization\\s*:\\s*(?:(?:Bearer|Basic|token|ApiKey)\\s+)?', | ||
| 'x-api-key\\s*:\\s*', | ||
| 'api[_-]?key\\s*[=:]\\s*', | ||
| '[?&](?:key|api[_-]?key|token)=', | ||
| 'token\\s*[=:]\\s*', | ||
| 'access[_-]?token\\s*[=:]\\s*', |
There was a problem hiding this comment.
Same omission as in the PR diagnostics test: the
refresh[_-]?token,id[_-]?token, andsecretredaction regex patterns are present in the workflow YAML's Perl sanitizer but are missing from thisexpectContainsAllassertion list. TheexpectCommonCredentialsRedactedhelper verifies all three are redacted, but if the corresponding regex patterns are removed from the workflow YAML, only the functional test (not this structural test) would detect the regression. Add the missing patterns for consistency.
There was a problem hiding this comment.
Addressed in the latest amended commit: the notify diagnostic structural assertions now include refresh token, ID token, secret, and the tightened generic credential-value pattern.
| if [ "$missing" -ne 0 ]; then | ||
| echo "78" > ocr-exit-code.txt | ||
| mark_infrastructure_failure "validate" "OCR configuration is missing required variables or secrets" | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
Missing required secrets/variables (OCR_LLM_URL, OCR_LLM_TOKEN, OCR_LLM_MODEL) now exits 0 instead of exit 1. If the 'code-review' job is a required status check in branch protection rules, PRs can be merged without any OCR review when secrets are accidentally removed or misconfigured. The only downstream signal is a GitHub issue created by the notify job, which depends on someone actively monitoring those issues. Consider whether infrastructure failures from missing configuration should at least fail the job (or use a separate, non-blocking status check) rather than silently succeeding, so that the absence of OCR review is visible in the PR status checks.
There was a problem hiding this comment.
Not changed: issue 2431 explicitly requires OCR provider/configuration/infrastructure failures to be non-blocking. The workflow records the phase and exit code, posts PR diagnostics, uploads artifacts, and notifies the deduplicated ci/cd issue while keeping changed-test policy failures blocking.
| . ./ocr-workflow-helpers.sh | ||
| mark_policy_failure() { | ||
| echo "$1" > ocr-policy-failure.txt | ||
| } |
There was a problem hiding this comment.
mark_policy_failure()is defined inline in this step'srun:block (line 310-312) rather than in the sourcedocr-workflow-helpers.shhelper file wheremark_infrastructure_failure()lives. This is inconsistent and creates a maintainability hazard: if another step ever needs to callmark_policy_failure, it won't be available unless redefined. Consider movingmark_policy_failureintoocr-workflow-helpers.shalongsidemark_infrastructure_failureso all shared diagnostic helpers are in one place.
There was a problem hiding this comment.
Addressed in the latest amended commit: mark_policy_failure now lives in ocr-workflow-helpers.sh next to mark_infrastructure_failure and is sourced by the preview step.
| create_infrastructure_issue "$body_file" \ | ||
| --title "${ISSUE_TITLE}" | ||
| return 0 |
There was a problem hiding this comment.
The notify job uses
concurrency: cancel-in-progress: false, meaning concurrent infrastructure failures from different PRs all run to completion. There is a TOCTOU race between the firstgh issue listsearch (line 1095) andgh issue create(line 1134): two concurrent jobs could both fail to find an existing issue and both create new duplicate issues. The recheck before create (line 1120) mitigates but does not fully eliminate the race. Consider usinggh issue create --searchto atomically search-or-create, or add a final post-create deduplication step that closes duplicate issues with the same title.
There was a problem hiding this comment.
Not changed: the notify job uses a single repository-wide concurrency group with cancel-in-progress false, so GitHub serializes these notifications rather than running them concurrently. The existing recheck before create remains as an additional duplicate guard.
| return ( | ||
| previousChar === null || '({[=,:;!&|?+-*%^~<>)]'.includes(previousChar) | ||
| ); |
There was a problem hiding this comment.
)is included in the set of characters that can precede a regex literal, but in JavaScript expression context,)/is typically division (e.g.,(a + b) / c), not a regex start. A regex can only follow)after control-flow statement parentheses (if, while, for, etc.). Including)here could cause the parser to incorrectly treat division after a closing parenthesis as a regex literal, potentially skipping characters and producing incorrect function body extraction. This is a latent issue that doesn't affect the current workflow code but could cause subtle bugs if the parsed code changes.
There was a problem hiding this comment.
Addressed in the latest amended commit: regex-literal detection no longer treats a closing parenthesis as a regex-start context, and the extractor test now covers division after a parenthesized expression.
| stderr_excerpt="" | ||
| if [ -f ocr-stderr.log ]; then | ||
| raw_stderr_excerpt="$(head -c 2000 ocr-stderr.log)" | ||
| if ! stderr_excerpt="$(sanitize_diagnostics "$raw_stderr_excerpt")"; then | ||
| echo "::warning::Failed to sanitize OCR stderr diagnostic; skipping notification." | ||
| return 1 | ||
| fi | ||
| fi | ||
| preview_stderr_excerpt="" | ||
| if [ -f ocr-preview-stderr.log ]; then | ||
| raw_preview_stderr_excerpt="$(head -c 2000 ocr-preview-stderr.log)" | ||
| if ! preview_stderr_excerpt="$(sanitize_diagnostics "$raw_preview_stderr_excerpt")"; then | ||
| echo "::warning::Failed to sanitize OCR preview stderr diagnostic; skipping notification." | ||
| return 1 | ||
| fi | ||
| fi |
There was a problem hiding this comment.
Security risk: The
notify-ocr-infrastructure-failurejob postsocr-stderr.logandocr-preview-stderr.logcontent (first 2000 bytes each) to a publicly visible GitHub issue. Whilesanitize_diagnosticsredacts exact secret matches and common credential patterns (Authorization headers, api_key=, token=, etc.), the redaction is pattern-based and could miss secrets appearing in unconventional formats — e.g., embedded in JSON error responses from the LLM API ("error":"invalid api_key: sk-xxxx"), in URL path segments, or in stack traces. Theocr reviewcommand (line 437) writes its stderr toocr-stderr.log, and LLM API error responses are a common source of credential leakage in stderr. Consider either: (1) not including raw stderr excerpts in the GitHub issue body (rely on the run URL for debugging instead), or (2) applying a more aggressive redaction that masks any long alphanumeric string resembling a secret token, or (3) posting the issue as a private/collaborator-only notification rather than a public issue.
There was a problem hiding this comment.
Addressed in the latest amended commit: the infrastructure tracking issue no longer includes stderr or preview stderr excerpts. It records phase, exit code, sanitized infrastructure diagnostic, run URL, and artifact reference; raw stderr remains only in the workflow artifact.
| concurrency: | ||
| group: ocr-review-infrastructure-issue | ||
| cancel-in-progress: false |
There was a problem hiding this comment.
TOCTOU race condition: The
concurrencygroupocr-review-infrastructure-issueusescancel-in-progress: false, allowing multiple notification jobs from different PR failures to run concurrently. While the code performs a double-check (search → comment/create → recheck), there is still a race window between the firstgh issue listsearch and thegh issue createcall where two concurrent jobs could both find no existing issue and both create new ones. The recheck before create (line 1120) mitigates but does not eliminate this — both jobs could pass the recheck simultaneously if their searches complete before either creates. Consider using a GitHub Actions environment lock or a unique issue title suffix per failure to avoid duplicate issue creation.
There was a problem hiding this comment.
Not changed: GitHub Actions concurrency permits only one running job per group, and this notify job uses a single repository-wide group with cancel-in-progress false, so notifications are serialized. The second issue-list recheck remains as an additional duplicate guard.
| # Exact OCR endpoint/credential values are provided only for redaction; | ||
| # do not log process.env from this step. | ||
| OCR_LLM_TOKEN: ${{ secrets.OCR_LLM_AUTH_TOKEN }} | ||
| OCR_LLM_URL: ${{ vars.OCR_LLM_URL }} |
There was a problem hiding this comment.
New secret exposure surface:
OCR_LLM_TOKENandOCR_LLM_URLare newly added to thePost OCR resultsstep'senv:block. Previously these secrets were not available to this step at all. While the code uses them only for redaction purposes and the comment says 'do not log process.env from this step,' any unhandled exception or future code change that referencesprocess.envin an error path could inadvertently expose secrets in workflow logs. Consider extracting only the secret lengths or pre-hashing them for comparison, rather than passing the raw secret values to this step.
There was a problem hiding this comment.
Not changed: raw values are required to perform exact-match redaction before posting PR diagnostics. The step does not log process.env, GitHub masks configured secrets in logs, and OCR diagnostics are sanitized before being included in comments.
| const redactionMatch = postScript.match(/const\s+REDACTION\s*=\s*'([^']*)'/); | ||
| expect( | ||
| redactionMatch, | ||
| 'script should define REDACTION constant', | ||
| ).toBeTruthy(); |
There was a problem hiding this comment.
The regex
/const\s+REDACTION\s*=\s*'([^']*)'/only matches single-quoted string literals. If the workflow script ever definesREDACTIONwith double quotes (e.g.,const REDACTION = "[REDACTED]") or a template literal,redactionMatchwill benulland the subsequent.toBeTruthy()assertion will fail with a generic "script should define REDACTION constant" message that doesn't hint at the quote-style mismatch. Consider also matching double-quoted strings (e.g.,/const\s+REDACTION\s*=\s*(['"])([^'"\n]*)\1/) or providing a more descriptive assertion message when the match fails.
There was a problem hiding this comment.
Addressed in the latest amended commit: the REDACTION extractor now accepts both single- and double-quoted string constants and reports a more specific assertion message.
| # Exact OCR endpoint/credential values are provided only for redaction; | ||
| # do not log process.env from this step. | ||
| OCR_LLM_TOKEN: ${{ secrets.OCR_LLM_AUTH_TOKEN }} | ||
| OCR_LLM_URL: ${{ vars.OCR_LLM_URL }} |
There was a problem hiding this comment.
The OCR_LLM_TOKEN secret is now passed to the 'Post OCR results' github-script step purely for redaction purposes. While the code comment says 'do not log process.env', there is a risk of secret exposure: if an unhandled JavaScript exception occurs (e.g., a TypeError or ReferenceError in the inline comment posting logic), the github-script action may output error stack traces or environment dumps that include process.env contents. The
redactSecretDiagnosticsfunction only redacts strings explicitly passed through it — it does not protect against raw environment variable exposure in uncaught error paths. Consider removing the secret from this step's env entirely and performing redaction in a later step that doesn't have access to the raw secret, or wrap the entire script body in a try/catch that sanitizes any error message before calling core.warning/core.setFailed.
There was a problem hiding this comment.
Not changed: exact redaction requires raw values in this post step. The script does not log process.env, comments are posted best-effort through explicit sanitized values, and GitHub masks configured secrets in logs.
| if (!ran) { | ||
| core.setFailed(`OpenCodeReview failed or produced unparsable output (exit code ${exitCode}).`); | ||
| if (policyFailure) { | ||
| core.setFailed(`OCR policy failure: ${policyFailure}`); | ||
| } else { | ||
| core.warning(`OpenCodeReview failed or produced unparsable output (exit code ${exitCode}).`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Infrastructure failures (install failure, config missing, preview failure, OCR command failure, parse failure) now use
exit 0instead ofexit 1, and the 'Post OCR results' step callscore.warninginstead ofcore.setFailedfor these cases. This means thecode-reviewjob will report 'success' even when OCR completely fails due to infrastructure issues. If branch protection rules require this check to pass, OCR infrastructure failures will go unnoticed — only policy failures (missing/excluded test files) will causecore.setFailed. This is a significant behavioral change that could silently allow infrastructure failures to pass CI gating. Consider whether infrastructure failures should also fail the job (perhaps via theocr-classificationstep output), or document this as an intentional design decision.
There was a problem hiding this comment.
Not changed: this is the explicit issue 2431 requirement. OCR infrastructure/provider/runtime/parse failures must be non-blocking and surfaced via warnings, sticky PR diagnostics, artifacts, and the ci/cd tracking issue; changed-test policy failures still fail the job.
| it('redacts exact OCR secrets with regex metacharacters and backslashes in PR diagnostics', () => { | ||
| const postScript = commandText( | ||
| stepNamed(codeReviewJob, 'Post OCR results'), | ||
| ); | ||
| const secret = String.raw`tok$^.*+?()[]{}|\slash\end`; | ||
| const sanitize = makePostSanitizer(postScript, secret); |
There was a problem hiding this comment.
The pattern
commandText(stepNamed(codeReviewJob, 'Post OCR results'))is repeated across 11 test cases. SincecodeReviewJobis already available frombeforeAll, consider extracting a shared variable (e.g.,let postScript;) set in thebeforeAllhook, or a local helper function, to reduce duplication and make the tests more maintainable. If the step name ever changes, you'd only need to update one location instead of 11.
There was a problem hiding this comment.
Addressed in the latest amended commit: the workflow tests now initialize the Post OCR results step and script once in beforeAll and reuse the shared values.
| ocr-phase.txt | ||
| ocr-infrastructure-failure.txt | ||
| ocr-policy-failure.txt | ||
| if-no-files-found: warn |
There was a problem hiding this comment.
Changing
if-no-files-foundfromerrortowarnmeans the workflow will not fail when expected artifact files are missing (e.g., if the 'Initialize OCR artifact files' step fails early or files are lost). Thenotify-ocr-infrastructure-failurejob depends on downloading these artifacts to determine whether an infrastructure failure occurred. Withwarn, a missing artifact upload is silently accepted, and the downstream notify job'sdownload-artifactstep (which hascontinue-on-error: true) will also silently skip. This creates a gap where infrastructure failures could go entirely undetected if artifacts are lost. Consider keepingif-no-files-found: errorfor the core diagnostic files (ocr-exit-code.txt, ocr-phase.txt) or at minimum adding a health check in the notify job that surfaces missing diagnostic files as a warning.
There was a problem hiding this comment.
Not changed: warning-only artifact upload is part of the issue requirement to avoid misleading upload failures before initialization. The notify job separately handles missing or incomplete artifacts and creates diagnostics for failed code-review runs unless a policy failure output is present.
| cat > ocr-workflow-helpers.sh <<'EOF' | ||
| mark_infrastructure_failure() { | ||
| echo "phase=$1; reason=$2" > ocr-infrastructure-failure.txt | ||
| } | ||
| mark_policy_failure() { | ||
| echo "$1" > ocr-policy-failure.txt | ||
| } | ||
| EOF |
There was a problem hiding this comment.
The heredoc content is indented to match the YAML step indentation level. Because the delimiter is
<<'EOF'(not<<-'EOF'), bash does NOT strip leading whitespace from the content lines. This means the generatedocr-workflow-helpers.shfile will contain lines likemark_infrastructure_failure() {with 10 leading spaces, and the closingEOFline will also have leading spaces. Since the closing delimiter must matchEOFexactly (without leading whitespace), thisEOFwith leading spaces will NOT terminate the heredoc — the heredoc will capture everything until it finds a line containing onlyEOF(with no leading spaces). This will likely cause the heredoc to consume subsequent YAML content or fail. Use<<-'EOF'with tab-indented content, or de-indent the heredoc body and closing delimiter to column 0.
There was a problem hiding this comment.
Not changed: this is a YAML block scalar inside a run step; YAML strips the common indentation before bash receives the script. The generated heredoc delimiter is EOF at column 1 in the shell script, and actionlint plus the workflow tests pass.
2 participants
TLDR
Makes the OCR PR review workflow observational for OCR installation/config/provider/runtime/parse failures while preserving mandatory changed-test scope enforcement and inline OCR review comments.
Dive Deeper
This PR hardens .github/workflows/ocr-review.yml so OCR infrastructure failures no longer turn otherwise-valid PRs red. It now:
The new scripts/tests/ocr-review-workflow.test.js suite parses the workflow and exercises the workflow contract, sanitizer behavior, and failure classification rules.
Reviewer Test Plan
Reviewers can validate by checking that the workflow:
Local commands run:
Open Code Review was run detached repeatedly with timeout 20 and the final actionable High/Medium findings on changed files were addressed. The final OCR run had only disputed/out-of-scope/low findings; OCR infrastructure failures intentionally remain non-blocking per issue scope.
Known local verification notes:
Testing Matrix
Linked issues / bugs
Fixes #2431
Summary by CodeRabbit
New Features
Bug Fixes
Tests