fix(e2e): scope generic GPU risk signals to PR runs - #8639
Conversation
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
📝 WalkthroughWalkthroughThe PR separates the dispatch checkout SHA from the effective llama.cpp qualification head SHA. It updates workflow bindings, qualification evidence, workflow boundary validation, risk-signal tests, and the CLI workflow contract checksum. ChangesLlama.cpp E2E SHA identity
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit e4049cc in the TypeScript / code-coverage/cliThe overall coverage in commit e4049cc in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e-risk-signal-reporter.test.ts`:
- Around line 91-96: Update the test around configuredEnvironment to retain a
valid correlation ID and all other activation inputs while setting only
NEMOCLAW_E2E_EXPECTED_SHA to an empty string. Assert that configuredEnvironment
returns null, isolating the absent expected-SHA guard.
🪄 Autofix
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: Enterprise
Run ID: 54133fd1-e2ba-4b5c-a37d-09a2ff399c57
📒 Files selected for processing (7)
.github/workflows/e2e.yaml.github/workflows/pr-self-hosted.yamltest/e2e-risk-signal-reporter.test.tstest/e2e/live/llama-cpp-generic-gpu.test.tstest/e2e/support/e2e-operations-workflow-boundary.test.tstools/e2e/cli-artifact-workflow-boundary.mtstools/e2e/workflow-boundary.mts
| expect( | ||
| configuredEnvironment({ | ||
| NEMOCLAW_E2E_CORRELATION_ID: "", | ||
| NEMOCLAW_E2E_EXPECTED_SHA: "", | ||
| }), | ||
| ).toBeNull(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test the absent expected-SHA guard independently.
This test clears both identity values. It passes if reporting incorrectly activates when a correlation ID exists but NEMOCLAW_E2E_EXPECTED_SHA is absent.
Keep a valid correlation ID and the other activation inputs. Set only NEMOCLAW_E2E_EXPECTED_SHA to "". Expect configuredEnvironment to return null.
Proposed test change
expect(
configuredEnvironment({
- NEMOCLAW_E2E_CORRELATION_ID: "",
+ E2E_ARTIFACT_DIR: "/tmp/e2e-risk-signal-test",
+ E2E_TARGET_ID: "llama-cpp-generic-gpu",
+ GITHUB_WORKSPACE: "/workspace",
+ NEMOCLAW_E2E_CORRELATION_ID: CORRELATION_ID,
NEMOCLAW_E2E_EXPECTED_SHA: "",
+ NEMOCLAW_E2E_SHARD: "default",
}),
).toBeNull();As per path instructions, “Review tests for behavioral confidence rather than implementation lock-in.”
📝 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.
| expect( | |
| configuredEnvironment({ | |
| NEMOCLAW_E2E_CORRELATION_ID: "", | |
| NEMOCLAW_E2E_EXPECTED_SHA: "", | |
| }), | |
| ).toBeNull(); | |
| expect( | |
| configuredEnvironment({ | |
| E2E_ARTIFACT_DIR: "/tmp/e2e-risk-signal-test", | |
| E2E_TARGET_ID: "llama-cpp-generic-gpu", | |
| GITHUB_WORKSPACE: "/workspace", | |
| NEMOCLAW_E2E_CORRELATION_ID: CORRELATION_ID, | |
| NEMOCLAW_E2E_EXPECTED_SHA: "", | |
| NEMOCLAW_E2E_SHARD: "default", | |
| }), | |
| ).toBeNull(); |
🤖 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 `@test/e2e-risk-signal-reporter.test.ts` around lines 91 - 96, Update the test
around configuredEnvironment to retain a valid correlation ID and all other
activation inputs while setting only NEMOCLAW_E2E_EXPECTED_SHA to an empty
string. Assert that configuredEnvironment returns null, isolating the absent
expected-SHA guard.
Source: Path instructions
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite against this exact revision. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
wscurran
added
chore
3 participants
Summary
Automatic main-push generic GPU E2E runs currently activate risk-signal reporting with
github.shaeven though no correlation UUID exists, so Vitest exits before the live test starts. Keep reporting inactive whencheckout_shais absent, bind source attestation to a separate candidate SHA, and preserve the exact SHA and lowercase UUIDv4 contract for manual PR qualification.Changes
inputs.checkout_sha, leaving automatic main pushes inactive.Failure Evidence
c3bbad78306030ea650073372c0d9f8f82974a15CLI artifact and OpenShell v0.0.99, then failed at Vitest startup with a nonempty expected SHA, empty correlation ID, andrisk signal requires a lowercase UUIDv4 correlation id.290eb94884eb29bc8aac89685e18525bb1292ed3after inspecting merged PR ci(workflows): consolidate workflow infrastructure #8632. PR ci(workflows): consolidate workflow infrastructure #8632 did not change or supersede this risk-signal contract.Type of Change
Quality Gates
e9b4613efpassed with no findings.Documentation Writer Review
no-docs-neededDGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailable48E2E operations workflow tests,40CLI artifact workflow tests,20risk-signal reporter tests, and all441path-selected tests passed;npm run validate:prpassed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: Not run locally; fresh PR automation will provide the repository-wide gates.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Apurv Kumaria akumaria@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests