Fix nightly eval response extraction (Fixes #2651)#2652
Merged
Conversation
github-actions
Bot
added
the
maintainer:e2e:ok
Contributor
|
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 PR adds structured process capture and timeout handling, prevents overlapping ChangesEval diagnostics pipeline
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
LLxprt PR Review – PR #2652Issue AlignmentStrong. The PR directly resolves #2651 by forcing eval prompts into Side Effects
Code QualityGood. Tests and CoverageCoverage impact: increase. New tests cover: process-run capture/error/timeout paths ( VerdictReady. The implementation is well-tested, addresses the root cause described in #2651, and improves observability without regressing plain-text stderr behavior. The behavioral changes to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
packages/test-utils/src/process-run.test.ts (1)
131-161: 📐 Maintainability & Code Quality | 🔵 TrivialConsider adding coverage for the graceful-shutdown-after-SIGTERM path.
This test only exercises the force-kill branch (child ignores
SIGTERM, requiresSIGKILL). Adding a case where the child exits cleanly during theTERMINATION_GRACE_MSwindow (after receivingSIGTERM) would confirmdidTimeout/exitCodeare still reported correctly whenSIGKILLisn't needed.🤖 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 `@packages/test-utils/src/process-run.test.ts` around lines 131 - 161, The process-run tests lack coverage for a child that exits gracefully after SIGTERM. Add a case alongside “captures shutdown output and force-kills a run that ignores SIGTERM” where the child handles SIGTERM, emits shutdown output, and exits during the TERMINATION_GRACE_MS window; assert the timeout result reports didTimeout correctly with the child’s actual exitCode and without requiring SIGKILL.packages/test-utils/src/process-run.ts (1)
128-268: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicated close/error-handling scaffolding between
spawnRunandspawnRunWithTimeout.Both functions repeat the same
settledguard,captureRuninvocation, and close/error branching (only the timeout-specific timer logic differs). Extracting the shared settle/capture pattern into a helper would reduce duplication and the risk of the two implementations drifting (as seen in theclearTimersasymmetry noted above).🤖 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 `@packages/test-utils/src/process-run.ts` around lines 128 - 268, Extract the shared process settlement, captureRun invocation, and close/error branching from spawnRun and spawnRunWithTimeout into a reusable helper. Keep timeout-specific state, timer cleanup, timeoutError handling, and signal escalation in spawnRunWithTimeout, while preserving successful transformation and nonzero-exit error behavior in both functions. Ensure the helper consistently applies the settled guard and cleanup behavior so the two implementations cannot drift.packages/test-utils/src/stdout-filter.test.ts (1)
108-138: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the "non-standalone" telemetry-line branch.
All removal tests place the telemetry object alone on its own line(s), exercising only the standalone branch of
expandStandaloneLine(stdout-filter.ts lines 216-218 vs 220-223). Consider adding a case where a telemetry object shares a line with other text (e.g.prefix {telemetry-object} suffix) to confirm the non-standalone{ start: objectStart, end: objectEnd + 1 }path behaves as intended.🤖 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 `@packages/test-utils/src/stdout-filter.test.ts` around lines 108 - 138, Add test coverage in the stdout-filter tests for a telemetry object embedded within surrounding text on the same line, such as a prefix and suffix around the object. Assert that stripTelemetryFromStdout removes only the telemetry object while preserving the surrounding text, exercising the non-standalone expandStandaloneLine branch.packages/test-utils/src/stdout-filter.ts (1)
128-197: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting the two telemetry shapes being detected.
isJsonTelemetryObjectandisInspectedTelemetryObjectencode two distinct detection strategies (OTel JSON LogRecord shape vs. Nodeutil.inspectconsole-exporter text) via several magic constants/regexes with no comment tying them back to their source format. Given the complexity here, a short comment per function explaining which real-world output shape it targets would help future maintainers avoid regressing the heuristics.🤖 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 `@packages/test-utils/src/stdout-filter.ts` around lines 128 - 197, Document the two telemetry detection strategies by adding a concise comment above isJsonTelemetryObject describing the OpenTelemetry JSON LogRecord shape it recognizes, and above isInspectedTelemetryObject describing the Node util.inspect console-exporter text shape it recognizes. Keep the existing detection logic and heuristics 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.
Nitpick comments:
In `@packages/test-utils/src/process-run.test.ts`:
- Around line 131-161: The process-run tests lack coverage for a child that
exits gracefully after SIGTERM. Add a case alongside “captures shutdown output
and force-kills a run that ignores SIGTERM” where the child handles SIGTERM,
emits shutdown output, and exits during the TERMINATION_GRACE_MS window; assert
the timeout result reports didTimeout correctly with the child’s actual exitCode
and without requiring SIGKILL.
In `@packages/test-utils/src/process-run.ts`:
- Around line 128-268: Extract the shared process settlement, captureRun
invocation, and close/error branching from spawnRun and spawnRunWithTimeout into
a reusable helper. Keep timeout-specific state, timer cleanup, timeoutError
handling, and signal escalation in spawnRunWithTimeout, while preserving
successful transformation and nonzero-exit error behavior in both functions.
Ensure the helper consistently applies the settled guard and cleanup behavior so
the two implementations cannot drift.
In `@packages/test-utils/src/stdout-filter.test.ts`:
- Around line 108-138: Add test coverage in the stdout-filter tests for a
telemetry object embedded within surrounding text on the same line, such as a
prefix and suffix around the object. Assert that stripTelemetryFromStdout
removes only the telemetry object while preserving the surrounding text,
exercising the non-standalone expandStandaloneLine branch.
In `@packages/test-utils/src/stdout-filter.ts`:
- Around line 128-197: Document the two telemetry detection strategies by adding
a concise comment above isJsonTelemetryObject describing the OpenTelemetry JSON
LogRecord shape it recognizes, and above isInspectedTelemetryObject describing
the Node util.inspect console-exporter text shape it recognizes. Keep the
existing detection logic and heuristics unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: dd8bde1f-74d7-442c-bd6f-8ab811c8793f
📒 Files selected for processing (10)
evals/README.mdevals/test-helper.tsintegration-tests/json-output.test.tspackages/test-utils/src/process-run.test.tspackages/test-utils/src/process-run.tspackages/test-utils/src/stdout-filter.test.tspackages/test-utils/src/stdout-filter.tspackages/test-utils/src/test-rig.test.tspackages/test-utils/src/test-rig.tsscripts/tests/evals-save-memory-assertion.test.js
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
OpenCodeReview — PR #2652
|
Collaborator
Author
|
CodeRabbit review dispositions:
Verification after remediation: test-utils 49 tests passed, package typecheck passed, focused ESLint and formatting passed, and git diff checks passed. |
Collaborator
Author
|
PR OCR thread dispositions for commit 5b0533f:
Focused verification after these changes: test-utils 49 tests passed, eval contract 42 tests passed, test-utils and eval TypeScript checks passed, focused lint/format checks passed, and git diff checks passed. |
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
|
Latest OCR findings disposition for commit 27076b0: Addressed:
Not changed:
Focused verification passed: test-utils 49 tests, eval contract 42 tests, test-utils typecheck, eval TypeScript compile, lint, format, and diff checks. |
Collaborator
Author
|
CodeRabbit docstring coverage warning disposition: no blanket comments were added. The changed public contracts and the non-obvious process, artifact, and telemetry-format boundaries are documented. Adding repetitive comments to self-describing private helpers would conflict with this repository's convention of commenting why rather than restating what the code does. |
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
Fixes the recurring nightly save_memory eval failure by running eval cases through the CLI JSON output contract and validating only the schema-checked assistant response. Process diagnostics remain available separately instead of contaminating exact response assertions.
Dive Deeper
The previous eval hardening correctly required the exact answer$blue$ , but TestRig plain-text runs append successful-process stderr diagnostics to the returned stdout. The eval consequently compared $blue$ against a much larger human-readable output and failed even when save_memory succeeded.
This change:
The local Open Code Review completed with complete_best_effort coverage. Its source-grounded findings were remediated for timeout cleanup, test resource cleanup, telemetry false positives, indentation handling, and schema-string centralization. One reported TestRig raw/transformed stdout concern was dismissed because the raw stdout assignment is required for failed and timed-out run telemetry parsing, while structured raw capture is now explicit.
Reviewer Test Plan
Run the deterministic JSON/save_memory integration test:
Run the eval contract tests:
Run the TestRig process and Podman filtering tests:
Confirm the new integration case observes:
After merge, dispatch Evals Nightly and confirm all matrix attempts validate the assistant response rather than combined diagnostics.
Testing Matrix
Local verification completed on macOS:
The monolithic npm run test command reached the agents Vitest workspace after prior workspace suites passed, then stopped producing output with Vitest sleeping and no durable status marker. The process was terminated rather than left orphaned. All changed areas and the previously established sequential workspace/CLI coverage passed.
Linked issues / bugs
Fixes #2651
Related to #2605
Summary by CodeRabbit