Conversation
Emit strict, redacted telemetry for every OCR artifact path and expose compact run summaries without weakening the trusted-base or reviewed-range guarantees. Add a fail-closed longitudinal aggregator plus behavioral coverage for lifecycle failures, schema reconciliation, artifact hashing, workflow wiring, and CLI operation.
Contributor
📝 WalkthroughWalkthroughAdds a strict OCR telemetry schema and producer, a longitudinal aggregation CLI, atomic redaction and artifact handling, richer workflow evidence and final classification outputs, plus extensive unit, integration, CLI, and workflow tests. ChangesOCR telemetry pipeline
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
WalkthroughBefore this PR, the OCR workflow produced review comments and diagnostics, but it left no structured, queryable record of how each run performed over time. There was no standardized schema for capturing findings, timing, token usage, publication routing, file-read failures, or infrastructure state, and artifact uploads were not gated on validation. After this PR, every OCR workflow run emits a versioned, redacted ocr-telemetry.json record from trusted repository scripts; the record is validated against a strict schema, included in the reviewed-range hash set, and uploaded only after successful validation and hashing. A new longitudinal aggregator can then consume these records across runs to produce averages, trends, and distributions, while the workflow preserves trusted-base execution and atomic write semantics. Release NotesNew Features
Bug Fixes
Tests
Documentation
Refactor
Chore
Changes
Magnitude🎯 4 (XL) Related
Pre-merge Checks
Walkthrough generated by LLxprt PR Review. Planner issue: #2256 |
![github-actions[bot]](https://avatars.github.com/in/15368?s=60&v=4){kind=small}
Contributor
OpenCodeReview — PR #2733
Findings without a resolvable position
Shadow-mode routing preview (issue #2672)
|
Contributor
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. |
Collaborator
Author
|
@coderabbitai review Please review the latest remediation commit. The earlier automated check was rate limited, so a substantive source review is requested. |
Contributor
|
✅ Action performedReview finished.
|
Contributor
OpenCodeReview — PR #2733
OCR stderr excerptOCR preflight excerptOCR preview stderr excerpt |
Contributor
OpenCodeReview — PR #2733
Findings without a resolvable position
Shadow-mode routing preview (issue #2672)
|
Contributor
OpenCodeReview — PR #2733
Shadow-mode routing preview (issue #2672)
|
Contributor
OpenCodeReview — PR #2733
Shadow-mode routing preview (issue #2672)
|
Comment on lines
+598
to
+599
| expect(telemetryUploadStep.with?.path).toBe('ocr-telemetry.json'); | ||
| expect(telemetryUploadStep.with?.['if-no-files-found']).toBe('error'); |
Contributor
There was a problem hiding this comment.
[test/medium] The test does not assert the
if-no-files-foundbehavior for the mainUpload OCR artifactsstep. The workflow changed this value fromwarntoerror, but the refactored test only checks the telemetry upload step'sif-no-files-found. Add an assertion foruploadStep.with?.['if-no-files-found']to preserve coverage of this behavioral change.
Comment on lines
+602
to
608
| it('creates non-telemetry placeholders before producing telemetry', () => { | ||
| const placeholderRun = commandText( | ||
| stepNamed(codeReviewJob, 'Ensure OCR artifact placeholders exist'), | ||
| ); | ||
| for (const artifact of uploadStep.with?.path.trim().split(/\s+/) ?? []) { | ||
| expect(placeholderRun).toContain(artifact); | ||
| } | ||
| expect(placeholderRun).toContain('ocr-routing-decisions.json'); | ||
| expect(placeholderRun).not.toContain('ocr-telemetry.json'); | ||
| }); |
Contributor
There was a problem hiding this comment.
[test/low] The refactored placeholder test no longer verifies that every non-telemetry artifact in the upload list has a corresponding placeholder. The old test iterated over all artifacts and asserted each one was present in the placeholder script. The new test only checks two artifacts (
ocr-routing-decisions.jsonincluded,ocr-telemetry.jsonexcluded). Consider restoring a loop-based assertion over the full upload artifact list to ensure the placeholder step stays synchronized with the upload step.
Comment on lines
+59
to
+63
| afterEach(() => { | ||
| while (temporaryDirectories.length > 0) { | ||
| fs.rmSync(temporaryDirectories.pop(), { recursive: true, force: true }); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
[style/low] Use a
for...ofloop instead ofwhile (temporaryDirectories.length > 0)withpop()for the temporary directory cleanup. Thefor...ofpattern is more idiomatic and readable for iterating over an array to perform cleanup actions.
Comment on lines
+24
to
+32
| let _hashRun; | ||
| let uploadStep; | ||
| let telemetryUploadStep; | ||
| let wallClockStep; | ||
| let _wallClockRun; | ||
| let filesReviewedStep; | ||
| let _filesReviewedRun; | ||
| let finalClassificationStep; | ||
| let _finalClassificationRun; |
Contributor
There was a problem hiding this comment.
[style/low] Variables
_hashRun,_wallClockRun,_filesReviewedRun, and_finalClassificationRunuse underscore prefixes which conventionally indicate intentionally unused variables, yet they are actively referenced in assertions. This naming pattern may trigger lint warnings and misleads readers about variable usage intent. Rename them to remove the underscore prefix (e.g.,hashRun,wallClockRun) since they are read in test bodies.
Comment on lines
+38
to
+41
| telemetryStep = stepNamed( | ||
| codeReviewJob, | ||
| 'Emit OCR telemetry (issue #2676)', | ||
| ); |
Contributor
There was a problem hiding this comment.
[maintainability/low] The step name
'Emit OCR telemetry (issue #2676)'hardcodes a business issue number. While this aids traceability in the GitHub Actions UI, it creates maintenance overhead if issue tracking references change and conflicts with the guideline against hardcoding business numbers. Consider extracting the issue reference to a constant or making it optional, or reference the issue in a comment rather than the step display name.
Contributor
OpenCodeReview — PR #2733
Findings without a resolvable position
Shadow-mode routing preview (issue #2672)
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (5)
scripts/tests/aggregate-ocr-telemetry.test.js (1)
492-546: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePure aggregation tests nested inside the CLI contract suite. These three cases don't touch the CLI and inherit the CLI suite's temp-directory
beforeAll/afterAll. Moving thisdescribeto the top level keeps the suite boundaries meaningful.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/aggregate-ocr-telemetry.test.js` around lines 492 - 546, Move the “aggregateTelemetry — mixed evidence and epoch ordering” describe block out of the CLI contract suite and place it at the top level of the test file. Keep all three aggregation tests unchanged while ensuring they no longer inherit the CLI suite’s temporary-directory beforeAll/afterAll hooks.scripts/tests/ocr-telemetry-lifecycle.test.js (1)
295-318: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winOnly rejection paths are covered for the wall-clock step. All three cases expect
null; a valid epoch-millisecond marker producing a finite nonnegativewall_clock_secondsis never exercised, so a regression that always emits an empty value would pass.💚 Proposed addition
+ it('computes a nonnegative duration from a valid start marker', () => { + const directory = temporaryDirectory('ocr-wall-clock-valid-'); + fs.writeFileSync( + path.join(directory, 'ocr_wall_clock_start.txt'), + String(Date.now() - 2000), + ); + const output = path.join(directory, 'github-output.txt'); + const result = runShell( + commandText(stepNamed(codeReviewJob, 'Capture OCR wall-clock')), + directory, + { GITHUB_OUTPUT: output }, + ); + expect(result.status, result.stderr).toBe(0); + const value = fs + .readFileSync(output, 'utf8') + .match(/^wall_clock_seconds=(.*)$/m)?.[1]; + expect(Number(value)).toBeGreaterThanOrEqual(2); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/ocr-telemetry-lifecycle.test.js` around lines 295 - 318, Add a successful-case assertion alongside the rejection cases in the wall-clock lifecycle test, using a valid epoch-millisecond marker and verifying that the Capture OCR wall-clock step emits a finite, nonnegative wall_clock_seconds value. Keep the existing corrupt-marker coverage unchanged.scripts/tests/ocr-review-workflow-behaviors.test.js (1)
126-140: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winLost coverage for the pre-write placeholder. The workflow still overwrites the source file with
'[REDACTION-PENDING]\n'before staging the redacted temp (.github/workflows/ocr-review.ymlLine 3154) — that write is what prevents an unredacted file from surviving a mid-step crash. With that assertion removed, the guard can be dropped without any test failing.💚 Proposed addition
expectContainsAll(redactRun, [ + "fs.writeFileSync(fileName, '[REDACTION-PENDING]\\n')", 'const temporary = `${fileName}.redacting`', 'fs.writeFileSync(temporary, redact(original))', 'fs.renameSync(temporary, fileName)', ]);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/ocr-review-workflow-behaviors.test.js` around lines 126 - 140, Update the Behavior 4 test in the test case around redactRun to assert that the workflow writes '[REDACTION-PENDING]\n' to the source file before creating or writing the temporary redaction file. Verify this pre-write occurs before fs.writeFileSync(temporary, redact(original)), while preserving the existing atomic rename assertions.scripts/tests/ocr-telemetry-workflow.test.js (1)
24-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnderscore-prefixed names are actually used.
_hashRun,_wallClockRun,_filesReviewedRun, and_finalClassificationRunare all read later (Lines 243, 258, 282, 304); the_prefix conventionally signals "intentionally unused" and now misleads readers (and any unused-var tooling).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/tests/ocr-telemetry-workflow.test.js` around lines 24 - 32, Rename the underscore-prefixed variables _hashRun, _wallClockRun, _filesReviewedRun, and _finalClassificationRun to non-underscored names, and update every later reference to each variable accordingly while leaving the other step variables unchanged..github/workflows/ocr-review.yml (1)
3240-3285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated schema shape risks silent drift. This inline fallback record hand-mirrors the full
RECORD_KEYSset fromscripts/ocr-telemetry-schema.js; any future field added/renamed there will not be reflected here, and the divergence only surfaces at the "Validate redacted OCR telemetry" step (which, in this branch, cannot fall back to the producer either). The behavior stays fail-closed, so this is a maintainability concern rather than a live defect — a schema-key comment or a test asserting this literal againstRECORD_KEYSwould prevent the drift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/ocr-review.yml around lines 3240 - 3285, Add a maintainability guard for the inline fallback record in the Node heredoc: document that its keys must mirror RECORD_KEYS from scripts/ocr-telemetry-schema.js, and add a test or validation that compares this literal’s key set with RECORD_KEYS so added or renamed schema fields fail explicitly. Keep the existing fail-closed fallback behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ocr-review.yml:
- Around line 1211-1215: Update the comment above the node command in the OCR
invocation setup to state that Date.now() writes epoch milliseconds, while
preserving the existing command and the consumer’s division by 1000.
- Around line 2897-2908: Update the post_state derivation near
core.setOutput('post_state', postState) to account for sticky-summary
publication failure: when the summary post was not created and publicationState
remains 'ambiguous' with summaryComment null, report a failed or equivalent
non-success state instead of 'posted'. Preserve the existing RANGE_MODE, ran,
policyFailure, and failedInline distinctions for other outcomes, and reuse the
existing summary publication state symbols.
- Around line 3436-3448: Update the upload-state validation block to include the
outcome of the telemetry artifact upload step,
`steps.upload-ocr-telemetry.outcome`, alongside
`steps.upload-ocr-artifacts.outcome`. Set `upload_state="failed"` and
`infrastructure_failure="true"` when either upload fails or is skipped; retain
`uploaded` only when both outcomes are successful.
In `@scripts/aggregate-ocr-telemetry.js`:
- Around line 147-158: Update detectDuplicates so records with run_id set to
null bypass duplicate detection and do not share the null identity key; retain
the existing duplicate error behavior for records with a non-null run_id and
run_attempt identity.
In `@scripts/ocr-telemetry.js`:
- Around line 294-306: In scripts/ocr-telemetry.js lines 294-306, update
identityFields to lowercase and hex-validate context.sha, returning null for
invalid values using the existing buildGuaranteedTelemetry guard. Also update
the artifact mapping at scripts/ocr-telemetry.js lines 371-378 to restrict
completeness and publication_state to their schema enum sets, return null for
unknown values, and remove the always-null primary argument from terminalValue.
- Around line 489-498: Update crossText to return the same “none” fallback used
by distributionText when by_category_severity is empty, while preserving the
existing formatted output for non-empty distributions.
In `@scripts/tests/ocr-telemetry-schema.test.js`:
- Around line 125-129: Update the __proto__ case in the isOwnNumericDistribution
test to construct a real own "__proto__" property using JSON.parse, rather than
an object literal that changes the prototype; keep the constructor and prototype
rejection assertions unchanged.
---
Nitpick comments:
In @.github/workflows/ocr-review.yml:
- Around line 3240-3285: Add a maintainability guard for the inline fallback
record in the Node heredoc: document that its keys must mirror RECORD_KEYS from
scripts/ocr-telemetry-schema.js, and add a test or validation that compares this
literal’s key set with RECORD_KEYS so added or renamed schema fields fail
explicitly. Keep the existing fail-closed fallback behavior unchanged.
In `@scripts/tests/aggregate-ocr-telemetry.test.js`:
- Around line 492-546: Move the “aggregateTelemetry — mixed evidence and epoch
ordering” describe block out of the CLI contract suite and place it at the top
level of the test file. Keep all three aggregation tests unchanged while
ensuring they no longer inherit the CLI suite’s temporary-directory
beforeAll/afterAll hooks.
In `@scripts/tests/ocr-review-workflow-behaviors.test.js`:
- Around line 126-140: Update the Behavior 4 test in the test case around
redactRun to assert that the workflow writes '[REDACTION-PENDING]\n' to the
source file before creating or writing the temporary redaction file. Verify this
pre-write occurs before fs.writeFileSync(temporary, redact(original)), while
preserving the existing atomic rename assertions.
In `@scripts/tests/ocr-telemetry-lifecycle.test.js`:
- Around line 295-318: Add a successful-case assertion alongside the rejection
cases in the wall-clock lifecycle test, using a valid epoch-millisecond marker
and verifying that the Capture OCR wall-clock step emits a finite, nonnegative
wall_clock_seconds value. Keep the existing corrupt-marker coverage unchanged.
In `@scripts/tests/ocr-telemetry-workflow.test.js`:
- Around line 24-32: Rename the underscore-prefixed variables _hashRun,
_wallClockRun, _filesReviewedRun, and _finalClassificationRun to non-underscored
names, and update every later reference to each variable accordingly while
leaving the other step variables unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2641e8cf-c506-48c5-87fe-4e9cd5ad12a4
📒 Files selected for processing (17)
.github/workflows/ocr-review.ymlpackage.jsonscripts/aggregate-ocr-telemetry.jsscripts/ocr-telemetry-io.jsscripts/ocr-telemetry-schema.jsscripts/ocr-telemetry.jsscripts/tests/aggregate-ocr-telemetry.test.jsscripts/tests/ocr-review-incremental-checkpoint.test.jsscripts/tests/ocr-review-workflow-behaviors.test.jsscripts/tests/ocr-review-workflow-features.test.jsscripts/tests/ocr-review-workflow.test.jsscripts/tests/ocr-reviewed-range-manifest-wiring.test.jsscripts/tests/ocr-telemetry-cli.test.jsscripts/tests/ocr-telemetry-lifecycle.test.jsscripts/tests/ocr-telemetry-schema.test.jsscripts/tests/ocr-telemetry-workflow.test.jsscripts/tests/ocr-telemetry.test.js
Comment on lines
+1211
to
+1215
| # Issue #2676: capture the authoritative wall-clock start for the OCR | ||
| # invocation under both success and failure paths. Written as epoch | ||
| # seconds so the Capture OCR wall-clock step can compute a finite, | ||
| # nonnegative duration. | ||
| node -e 'process.stdout.write(String(Date.now()))' > ocr_wall_clock_start.txt |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Comment contradicts the value actually written. Date.now() writes epoch milliseconds, but the comment says "epoch seconds". The consumer at Line 1333 divides by 1000, so behavior is correct — only the comment is wrong, and it invites a "fix" that would inflate wall_clock_seconds 1000×.
📝 Proposed comment fix
- # invocation under both success and failure paths. Written as epoch
- # seconds so the Capture OCR wall-clock step can compute a finite,
- # nonnegative duration.
+ # invocation under both success and failure paths. Written as epoch
+ # milliseconds (Date.now()); the Capture OCR wall-clock step divides
+ # by 1000 to produce a finite, nonnegative duration in seconds.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # Issue #2676: capture the authoritative wall-clock start for the OCR | |
| # invocation under both success and failure paths. Written as epoch | |
| # seconds so the Capture OCR wall-clock step can compute a finite, | |
| # nonnegative duration. | |
| node -e 'process.stdout.write(String(Date.now()))' > ocr_wall_clock_start.txt | |
| # Issue `#2676`: capture the authoritative wall-clock start for the OCR | |
| # invocation under both success and failure paths. Written as epoch | |
| # milliseconds (Date.now()); the Capture OCR wall-clock step divides | |
| # by 1000 to produce a finite, nonnegative duration in seconds. | |
| node -e 'process.stdout.write(String(Date.now()))' > ocr_wall_clock_start.txt |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ocr-review.yml around lines 1211 - 1215, Update the
comment above the node command in the OCR invocation setup to state that
Date.now() writes epoch milliseconds, while preserving the existing command and
the consumer’s division by 1000.
Comment on lines
+2897
to
+2908
| // Issue #2676: expose the Post OCR results lifecycle state so the | ||
| // telemetry record can truthfully distinguish posted/noop/failed | ||
| // post outcomes. Derived from ran + range mode, not invented. | ||
| let postState = 'posted'; | ||
| if (process.env.RANGE_MODE === 'noop') { | ||
| postState = 'noop'; | ||
| } else if (!ran) { | ||
| postState = policyFailure ? 'policy-failed' : 'failed'; | ||
| } else if (failedInline > 0) { | ||
| postState = 'partial'; | ||
| } | ||
| core.setOutput('post_state', postState); |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
post_state reports posted when the sticky-summary post failed. The summary failure at Lines 2821-2823 is swallowed and leaves publicationState = 'ambiguous' with summaryComment === null, yet this derivation only inspects RANGE_MODE, ran, and failedInline. A run whose summary comment never landed is therefore recorded as a clean posted in telemetry — exactly the kind of publication-routing signal issue #2676 is meant to capture.
🐛 Proposed fix
let postState = 'posted';
if (process.env.RANGE_MODE === 'noop') {
postState = 'noop';
} else if (!ran) {
postState = policyFailure ? 'policy-failed' : 'failed';
- } else if (failedInline > 0) {
+ } else if (failedInline > 0 || publicationState !== 'complete') {
postState = 'partial';
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Issue #2676: expose the Post OCR results lifecycle state so the | |
| // telemetry record can truthfully distinguish posted/noop/failed | |
| // post outcomes. Derived from ran + range mode, not invented. | |
| let postState = 'posted'; | |
| if (process.env.RANGE_MODE === 'noop') { | |
| postState = 'noop'; | |
| } else if (!ran) { | |
| postState = policyFailure ? 'policy-failed' : 'failed'; | |
| } else if (failedInline > 0) { | |
| postState = 'partial'; | |
| } | |
| core.setOutput('post_state', postState); | |
| // Issue `#2676`: expose the Post OCR results lifecycle state so the | |
| // telemetry record can truthfully distinguish posted/noop/failed | |
| // post outcomes. Derived from ran + range mode, not invented. | |
| let postState = 'posted'; | |
| if (process.env.RANGE_MODE === 'noop') { | |
| postState = 'noop'; | |
| } else if (!ran) { | |
| postState = policyFailure ? 'policy-failed' : 'failed'; | |
| } else if (failedInline > 0 || publicationState !== 'complete') { | |
| postState = 'partial'; | |
| } | |
| core.setOutput('post_state', postState); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ocr-review.yml around lines 2897 - 2908, Update the
post_state derivation near core.setOutput('post_state', postState) to account
for sticky-summary publication failure: when the summary post was not created
and publicationState remains 'ambiguous' with summaryComment null, report a
failed or equivalent non-success state instead of 'posted'. Preserve the
existing RANGE_MODE, ran, policyFailure, and failedInline distinctions for other
outcomes, and reuse the existing summary publication state symbols.
Comment on lines
+3436
to
+3448
| upload_outcome="${{ steps.upload-ocr-artifacts.outcome }}" | ||
| telemetry_state="complete" | ||
| hash_state="prepared" | ||
| upload_state="uploaded" | ||
| if [ "$post_outcome" != "success" ] || [ -z "$post_state" ] || [ "$post_state" = "failed" ]; then | ||
| post_state="failed" | ||
| infrastructure_failure="true" | ||
| fi | ||
| if [ "$post_state" = "policy-failed" ]; then policy_failure="true"; infrastructure_failure="true"; fi | ||
| if [ "$source_redaction_outcome" != "success" ]; then infrastructure_failure="true"; fi | ||
| if [ "$telemetry_valid" != "true" ]; then telemetry_state="failed"; infrastructure_failure="true"; fi | ||
| if [ "$hash_valid" != "true" ]; then hash_state="failed"; infrastructure_failure="true"; fi | ||
| if [ "$upload_outcome" != "success" ]; then upload_state="failed"; infrastructure_failure="true"; fi |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
upload_state ignores the telemetry artifact upload. Only steps.upload-ocr-artifacts.outcome is consulted; a failed or skipped upload-ocr-telemetry (the primary #2676 deliverable) still yields upload_state=uploaded and no infrastructure failure.
🐛 Proposed fix
upload_outcome="${{ steps.upload-ocr-artifacts.outcome }}"
+ telemetry_upload_outcome="${{ steps.upload-ocr-telemetry.outcome }}"
@@
- if [ "$upload_outcome" != "success" ]; then upload_state="failed"; infrastructure_failure="true"; fi
+ if [ "$upload_outcome" != "success" ] || [ "$telemetry_upload_outcome" != "success" ]; then
+ upload_state="failed"
+ infrastructure_failure="true"
+ fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| upload_outcome="${{ steps.upload-ocr-artifacts.outcome }}" | |
| telemetry_state="complete" | |
| hash_state="prepared" | |
| upload_state="uploaded" | |
| if [ "$post_outcome" != "success" ] || [ -z "$post_state" ] || [ "$post_state" = "failed" ]; then | |
| post_state="failed" | |
| infrastructure_failure="true" | |
| fi | |
| if [ "$post_state" = "policy-failed" ]; then policy_failure="true"; infrastructure_failure="true"; fi | |
| if [ "$source_redaction_outcome" != "success" ]; then infrastructure_failure="true"; fi | |
| if [ "$telemetry_valid" != "true" ]; then telemetry_state="failed"; infrastructure_failure="true"; fi | |
| if [ "$hash_valid" != "true" ]; then hash_state="failed"; infrastructure_failure="true"; fi | |
| if [ "$upload_outcome" != "success" ]; then upload_state="failed"; infrastructure_failure="true"; fi | |
| upload_outcome="${{ steps.upload-ocr-artifacts.outcome }}" | |
| telemetry_upload_outcome="${{ steps.upload-ocr-telemetry.outcome }}" | |
| telemetry_state="complete" | |
| hash_state="prepared" | |
| upload_state="uploaded" | |
| if [ "$post_outcome" != "success" ] || [ -z "$post_state" ] || [ "$post_state" = "failed" ]; then | |
| post_state="failed" | |
| infrastructure_failure="true" | |
| fi | |
| if [ "$post_state" = "policy-failed" ]; then policy_failure="true"; infrastructure_failure="true"; fi | |
| if [ "$source_redaction_outcome" != "success" ]; then infrastructure_failure="true"; fi | |
| if [ "$telemetry_valid" != "true" ]; then telemetry_state="failed"; infrastructure_failure="true"; fi | |
| if [ "$hash_valid" != "true" ]; then hash_state="failed"; infrastructure_failure="true"; fi | |
| if [ "$upload_outcome" != "success" ] || [ "$telemetry_upload_outcome" != "success" ]; then | |
| upload_state="failed" | |
| infrastructure_failure="true" | |
| fi |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ocr-review.yml around lines 3436 - 3448, Update the
upload-state validation block to include the outcome of the telemetry artifact
upload step, `steps.upload-ocr-telemetry.outcome`, alongside
`steps.upload-ocr-artifacts.outcome`. Set `upload_state="failed"` and
`infrastructure_failure="true"` when either upload fails or is skipped; retain
`uploaded` only when both outcomes are successful.
Comment on lines
+147
to
+158
| function detectDuplicates(sorted) { | ||
| const seen = new Set(); | ||
| for (const record of sorted) { | ||
| const key = `${record.run_id}\u0000${record.run_attempt}`; | ||
| if (seen.has(key)) { | ||
| throw new Error( | ||
| `duplicate (run_id=${record.run_id}, run_attempt=${record.run_attempt}) records are not allowed without an explicit retry policy`, | ||
| ); | ||
| } | ||
| seen.add(key); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Null identity records collide as duplicates.
The schema permits run_id: null / run_attempt: null, and buildGuaranteedTelemetry returns the candidate unchanged when it validates — so two genuinely distinct runs with unavailable identity both key to null\u0000null and abort the entire aggregation. Consider skipping duplicate detection when run_id is null, or extending the key with generated_at.
🩹 Proposed fix
function detectDuplicates(sorted) {
const seen = new Set();
for (const record of sorted) {
+ if (record.run_id === null) continue;
const key = `${record.run_id}\u0000${record.run_attempt}`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function detectDuplicates(sorted) { | |
| const seen = new Set(); | |
| for (const record of sorted) { | |
| const key = `${record.run_id}\u0000${record.run_attempt}`; | |
| if (seen.has(key)) { | |
| throw new Error( | |
| `duplicate (run_id=${record.run_id}, run_attempt=${record.run_attempt}) records are not allowed without an explicit retry policy`, | |
| ); | |
| } | |
| seen.add(key); | |
| } | |
| } | |
| function detectDuplicates(sorted) { | |
| const seen = new Set(); | |
| for (const record of sorted) { | |
| if (record.run_id === null) continue; | |
| const key = `${record.run_id}\u0000${record.run_attempt}`; | |
| if (seen.has(key)) { | |
| throw new Error( | |
| `duplicate (run_id=${record.run_id}, run_attempt=${record.run_attempt}) records are not allowed without an explicit retry policy`, | |
| ); | |
| } | |
| seen.add(key); | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/aggregate-ocr-telemetry.js` around lines 147 - 158, Update
detectDuplicates so records with run_id set to null bypass duplicate detection
and do not share the null identity key; retain the existing duplicate error
behavior for records with a non-null run_id and run_attempt identity.
Comment on lines
+294
to
+306
| function identityFields(context) { | ||
| const normalizedPrNumber = nullableCount(context.prNumber); | ||
| return { | ||
| run_id: nullableString(context.runId), | ||
| run_attempt: normalizeRunAttempt(context.runAttempt), | ||
| pr_number: | ||
| normalizedPrNumber !== null && normalizedPrNumber > 0 | ||
| ? normalizedPrNumber | ||
| : null, | ||
| sha: nullableString(context.sha), | ||
| generated_at: normalizeTimestamp(context.generatedAt), | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Unnormalized field passthrough in buildRecord discards entire records. Several fields are copied straight from context/artifacts into schema-constrained slots; one bad value invalidates the candidate and buildGuaranteedTelemetry replaces every real metric with the empty failed fallback. Normalize per field so a single malformed input degrades only that field.
scripts/ocr-telemetry.js#L294-L306: lowercase and hex-validateshainidentityFields(the same guardbuildGuaranteedTelemetryalready applies), emittingnullwhen it doesn't match.scripts/ocr-telemetry.js#L371-L378: constraincompletenessandpublication_stateto the schema enum sets, emittingnullfor unrecognized artifact values; also drop the always-nullprimary argument in theterminalValuecall.
📍 Affects 1 file
scripts/ocr-telemetry.js#L294-L306(this comment)scripts/ocr-telemetry.js#L371-L378
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/ocr-telemetry.js` around lines 294 - 306, In scripts/ocr-telemetry.js
lines 294-306, update identityFields to lowercase and hex-validate context.sha,
returning null for invalid values using the existing buildGuaranteedTelemetry
guard. Also update the artifact mapping at scripts/ocr-telemetry.js lines
371-378 to restrict completeness and publication_state to their schema enum
sets, return null for unknown values, and remove the always-null primary
argument from terminalValue.
Comment on lines
+489
to
+498
| function crossText(findings) { | ||
| if (!findings) return 'n/a'; | ||
| return Object.entries(findings.by_category_severity) | ||
| .flatMap(([category, severities]) => | ||
| Object.entries(severities).map( | ||
| ([severity, count]) => `${category}×${severity}: ${count}`, | ||
| ), | ||
| ) | ||
| .join(', '); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
crossText renders an empty string for an empty distribution.
distributionText falls back to none, but crossText returns '' when by_category_severity is empty (zero findings), producing a dangling category×severity — line in the step summary.
🩹 Proposed fix
.flatMap(([category, severities]) =>
Object.entries(severities).map(
([severity, count]) => `${category}×${severity}: ${count}`,
),
)
- .join(', ');
+ .join(', ') || 'none';📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function crossText(findings) { | |
| if (!findings) return 'n/a'; | |
| return Object.entries(findings.by_category_severity) | |
| .flatMap(([category, severities]) => | |
| Object.entries(severities).map( | |
| ([severity, count]) => `${category}×${severity}: ${count}`, | |
| ), | |
| ) | |
| .join(', '); | |
| } | |
| function crossText(findings) { | |
| if (!findings) return 'n/a'; | |
| return Object.entries(findings.by_category_severity) | |
| .flatMap(([category, severities]) => | |
| Object.entries(severities).map( | |
| ([severity, count]) => `${category}×${severity}: ${count}`, | |
| ), | |
| ) | |
| .join(', ') || 'none'; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/ocr-telemetry.js` around lines 489 - 498, Update crossText to return
the same “none” fallback used by distributionText when by_category_severity is
empty, while preserving the existing formatted output for non-empty
distributions.
Comment on lines
+125
to
+129
| it('rejects __proto__/constructor/prototype keys', () => { | ||
| expect(isOwnNumericDistribution({ __proto__: { x: 1 } })).toBe(false); | ||
| expect(isOwnNumericDistribution({ constructor: 1 })).toBe(false); | ||
| expect(isOwnNumericDistribution({ prototype: 1 })).toBe(false); | ||
| }); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm how isOwnNumericDistribution treats an empty own-key object and
# a real own __proto__ key, to validate the assertion above.
fd -t f 'ocr-telemetry-schema.js' scripts --exec ast-grep outline {} --match 'isOwnNumericDistribution|UNSAFE_KEYS' --view expandedRepository: vybestack/llxprt-code
Length of output: 316
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== scripts/ocr-telemetry-schema.js (relevant slice) ==\n'
cat -n scripts/ocr-telemetry-schema.js | sed -n '100,180p'
printf '\n== scripts/tests/ocr-telemetry-schema.test.js (relevant slice) ==\n'
cat -n scripts/tests/ocr-telemetry-schema.test.js | sed -n '115,135p'
printf '\n== JS semantics probe for object-literal __proto__ ==\n'
node - <<'JS'
const a = { __proto__: { x: 1 } };
console.log('own keys:', Object.keys(a));
console.log('prototype has x:', Object.getPrototypeOf(a).x);
console.log('hasOwnProperty("__proto__"):', Object.prototype.hasOwnProperty.call(a, '__proto__'));
JSRepository: vybestack/llxprt-code
Length of output: 4801
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== scripts/ocr-telemetry-schema.js (top slice for isPlainObject) ==\n'
cat -n scripts/ocr-telemetry-schema.js | sed -n '1,110p'Repository: vybestack/llxprt-code
Length of output: 3721
Use a real own __proto__ key here { __proto__: { x: 1 } } changes the prototype, so this assertion only covers the plain-object check. If the goal is to exercise the unsafe-key path, switch it to JSON.parse('{"__proto__":1}').
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/tests/ocr-telemetry-schema.test.js` around lines 125 - 129, Update
the __proto__ case in the isOwnNumericDistribution test to construct a real own
"__proto__" property using JSON.parse, rather than an object literal that
changes the prototype; keep the constructor and prototype rejection assertions
unchanged.
Collaborator
Author
|
Evaluated the non-positioned workflow-test finding against the source. The step names and ordering assertions are intentional behavioral contracts for trusted-base execution, redaction-before-validation/hashing, source artifact gating, and lifecycle classification boundaries. Replacing those contracts with generic IDs or broadly removing ordering assertions would allow security- and integrity-relevant workflow reordering regressions. No source change is warranted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TLDR
Adds strict, fail-closed OCR telemetry and longitudinal aggregation so each OCR workflow run records trustworthy finding, timing, file, publication, failure, and token metrics. Reviewers should focus on fallback validity, redaction/hash ordering, and preservation of trusted-base workflow execution.
Dive Deeper
Reviewer Test Plan
Run the focused telemetry/workflow tests:
Validate workflow syntax and lint policy:
Inspect the OCR workflow and verify telemetry is produced from trusted repository scripts, redacted before validation/hashing, and uploaded only after successful validation and hashes.
Exercise the telemetry CLI with malformed metadata, missing context, invalid identity/lifecycle values, and an unwritable destination; confirm it emits a schema-valid fallback when possible and leaves no temporary write artifact.
Run the aggregator over multiple telemetry records and verify deterministic trends and contextual errors for malformed or unreadable files.
Testing Matrix
Local focused verification passed 275 tests. Actionlint and the ESLint policy guard passed. Full npm verification, build, smoke, and CI results are tracked in the PR checks.
Linked issues / bugs
Fixes #2676
Related to #2658, #2672, and #2673.
Summary by CodeRabbit