fix(ci): isolate credential-backed E2E smokes - #6687
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe PR extracts shared live inference-routing helpers, adds credential-backed OpenAI, Anthropic, and isolation smoke tests, separates provider smoke execution from the required workflow lane, and strengthens PR gate evidence and failure-message assertions. ChangesLive inference routing
PR gate validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant providerSmokeTest
participant nemoclaw
participant inferenceLocal
participant providerAPI
providerSmokeTest->>nemoclaw: onboard sandbox with provider credentials
nemoclaw->>inferenceLocal: expose local inference endpoint
providerSmokeTest->>inferenceLocal: send chat or messages request
inferenceLocal->>providerAPI: route request to configured provider
providerAPI-->>inferenceLocal: return provider response
inferenceLocal-->>providerSmokeTest: return response for PONG assertion
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage remains at 96%, unchanged from the TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
PR Review Advisor — No blocking findingsMerge posture: No blocking advisor findings This is an automated review. Required findings need action before merge. Warnings and optional suggestions do not require a response or follow-up. A human maintainer makes the final merge decision. |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
changed the title
Collaborator
Author
|
Addressed the required review finding in
Independent final review found the revised trust boundary clean, and every PR commit is GitHub Verified. |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/e2e/live/inference-routing-provider-smoke.test.ts (1)
97-110: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winTrim the filesystem scan to stay within the 90s budget. Hashing every character offset across up to 200 files under
/sandbox,/home, and/tmpis expensive, and this helper runs twice in the test. Capping the scanned bytes per file or narrowing thefindscope would keep the leak check while reducing timeout risk.🤖 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/live/inference-routing-provider-smoke.test.ts` around lines 97 - 110, Reduce the workload in the scanScript filesystem search by narrowing the find scope or limiting the number of bytes processed per file, while preserving detection of the target hash. Update the file iteration around execFileSync and the content scanning loop so the helper remains within the 90-second budget when invoked twice.
🤖 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/live/inference-routing-provider-smoke.test.ts`:
- Around line 107-141: Update the scanScript file enumeration to scan `/tmp`
before applying the `head -200` cap, ensuring `/tmp` entries are included even
when `/sandbox` or `/home` contain many files. Preserve the existing hashing and
canary detection logic.
---
Nitpick comments:
In `@test/e2e/live/inference-routing-provider-smoke.test.ts`:
- Around line 97-110: Reduce the workload in the scanScript filesystem search by
narrowing the find scope or limiting the number of bytes processed per file,
while preserving detection of the target hash. Update the file iteration around
execFileSync and the content scanning loop so the helper remains within the
90-second budget when invoked twice.
🪄 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: Enterprise
Run ID: fb547e1f-7a82-4f1d-aa3f-6d9956ccf855
📒 Files selected for processing (8)
.github/workflows/e2e.yamltest/e2e/live/inference-routing-helpers.tstest/e2e/live/inference-routing-provider-smoke.test.tstest/e2e/live/inference-routing.test.tstest/e2e/mock-parity.jsontest/e2e/support/e2e-workflow.test.tstest/pr-e2e-gate.test.tstools/e2e/pr-e2e-gate.mts
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/e2e.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/e2e/pr-e2e-gate.mts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Reviewed CodeRabbit's non-inline performance nitpick about the character-by-character filesystem hash scan. I am not changing that algorithm in this PR: it is pre-existing behavior moved unchanged into the optional credential-smoke file, it is not selected by the PR-safe lane, and improving its worst-case runtime would require a separate sampling/hash design change rather than an evidence-boundary fix. The actionable |
Collaborator
Author
|
Post-merge verification passed on current
No |
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary The PR E2E gate currently rejects `inference-routing` after its four required checks pass because three credential-backed smokes are collected and deliberately skip. This keeps the gate strict, moves those smokes behind a trusted file boundary that the PR workflow does not select, and makes failed coordinator annotations include the concrete evidence summary. ## Changes - Keep the PR-required `inference-routing.test.ts` lane fully discoverable with four ordinary, always-on tests and no skipped evidence. - Move the NVIDIA credential-isolation, OpenAI, and Anthropic smokes into `inference-routing-provider-smoke.test.ts`; all three remain ordinary registered Vitest tests, but no PR workflow supplies provider secrets or selects that file. - Extract shared onboarding, cleanup, command, and protocol helpers so the file split does not duplicate the substantial live-test machinery. - Pin the trusted workflow boundary with a support test, and record the new credential-backed file as live-only under the existing mock-parity policy. Any future secret-bearing lane must run it from trusted `main`. - Keep every reported skip fail-closed, including evidence carrying a spoofed `optionalSkipped` property. - Include the verdict summary in failed coordinator annotations so missing or unfinished shards are visible directly in the job log. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates <!-- Check exactly one tests line and one docs line. Check other lines when applicable. Add every requested justification or approval reference. --> - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This changes an internal CI/test boundary; audits confirmed the existing E2E contributor docs already describe strict evidence, opt-in live tests, and live-only mock-parity decisions. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent final review passed all nine categories: provider smokes remain registered, the trusted PR workflow selects only the credential-free file, no PR job receives provider secrets, and every reported skip remains fail-closed. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification <!-- Check each applicable item only when supported by the requested evidence. Run targeted tests once per relevant change set and rerun after later edits or hook autofixes that can affect the tested behavior. Do not rerun hook-covered checks. --> - [x] PR description includes the DCO sign-off declaration and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run check:diff` passed when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — E2E-support 115 files / 927 tests; gate and mock-parity integration 26 tests; Vitest collection proves a 4-test required file and 3-test provider-smoke file; provider-smoke without opt-ins reports 3 skips; CLI typecheck, project-overlap, test-size, conditional scan, Biome, and mock-parity checks pass. - [x] Applicable broad gate passed — final-head GitHub CI passed the aggregate checks, all CLI shards, E2E-support, both sandbox image builds and downstream smokes, CodeQL, and the trusted `cloud-onboard` PR E2E gate with no skipped or pending evidence. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- <!-- DCO sign-off is required in this PR description, and every commit must appear as Verified in GitHub. Run: git config user.name && git config user.email --> Signed-off-by: Carlos Villela <cvillela@nvidia.com> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
wscurran
added
area: ci
23 tasks
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
2 participants
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.
Summary
The PR E2E gate currently rejects
inference-routingafter its four required checks pass because three credential-backed smokes are collected and deliberately skip. This keeps the gate strict, moves those smokes behind a trusted file boundary that the PR workflow does not select, and makes failed coordinator annotations include the concrete evidence summary.Changes
inference-routing.test.tslane fully discoverable with four ordinary, always-on tests and no skipped evidence.inference-routing-provider-smoke.test.ts; all three remain ordinary registered Vitest tests, but no PR workflow supplies provider secrets or selects that file.main.optionalSkippedproperty.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablecloud-onboardPR E2E gate with no skipped or pending evidence.npm run docsbuilds without warnings (doc changes only)Signed-off-by: Carlos Villela cvillela@nvidia.com