ci(e2e): discover credential-free tests from source - #6670
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
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 E2E workflow discovers credential-free tests from module tags, plans live and shared matrices with validated selectors, executes shared rows, validates workflow contracts, and reports per-test conclusions. Advisor normalization and mock-parity filtering also recognize credential-free test metadata. ChangesCredential-free E2E workflow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant WorkflowDispatch
participant GenerateMatrix
participant SharedE2E
participant WorkflowJobsAPI
participant ReportToPR
WorkflowDispatch->>GenerateMatrix: pass jobs or targets selectors
GenerateMatrix->>SharedE2E: provide test_matrix rows
SharedE2E->>WorkflowJobsAPI: publish per-test job conclusions
ReportToPR->>WorkflowJobsAPI: query Shared E2E (id) jobs
ReportToPR->>ReportToPR: merge conclusions and render PR status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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
E2E Target RecommendationRequired E2E targets: Dispatch required E2E targets:
Full E2E target advisor summaryE2E Target AdvisorBase: Required E2E targets
Optional E2E targets
Relevant changed files
|
Contributor
PR Review Advisor (Nemotron Ultra) — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
🚨 Required before mergeAddress these before merging unless a maintainer explicitly overrides the advisor with rationale.
|
Contributor
PR Review Advisor — Changes requestedMerge posture: Do not merge yet Action checklist
Findings index
Review findings by urgency: 0 required fixes, 1 item to resolve/justify, 0 in-scope improvements
|
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: 3
🧹 Nitpick comments (3)
tools/e2e-advisor/targets.mts (1)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated path-classification rules risk drifting from
execution-profile.mts.
E2E_LIVE_PROFILE_TEST_PATTERN/INTEGRATION_PROFILE_TEST_PATTERNre-implement the same "e2e-live undertest/e2e/live/, integration not undertest/e2e/" shape rules enforced byvalidateTestFileintools/e2e/execution-profile.mts. Since that function isn't exported, the advisor can't reuse it directly, but any future change to the canonical path rules won't automatically propagate here, silently misclassifying changed files.As per path instructions for
tools/{pr-review-advisor,e2e-advisor}/**: "Derive inventories and limits from a canonical source where possible; flag duplicated lists that can silently drift."♻️ Suggested direction
-export function executionProfileRowFromModule( +function validateTestFilePublic(file: string, project: ExecutionProfileProject): void { + validateTestFile(file, project); +}Export a thin validation helper (or the pattern constants themselves) from
execution-profile.mtsand import it here instead of re-declaring the regexes.🤖 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 `@tools/e2e-advisor/targets.mts` around lines 61 - 64, Eliminate duplicated test-path rules between targets.mts and execution-profile.mts by exporting the canonical validation helper or pattern constants from execution-profile.mts, then importing and using them in E2E_LIVE_PROFILE_TEST_PATTERN and INTEGRATION_PROFILE_TEST_PATTERN logic. Preserve the existing classification behavior while ensuring future rule changes in validateTestFile remain synchronized.test/e2e-advisor-targets.test.ts (1)
437-497: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a test for the
changedFileSources-omitted fallback path.Both new cases always supply
changedFileSourcesfor the changed file, so thefs.readFileSync(path.join(root, file), ...)fallback branch inbuildE2eTargetNormalizationContext(targets.mts lines ~551-560) is never exercised. Given the suspected undefined-rootissue flagged intargets.mts, a test that omitschangedFileSourcesfor a profile-pattern-matching changed file would catch this at the unit level rather than only in production.As per path instructions for
**/*.test.{ts,js,mts,mjs,cts,cjs}: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."🤖 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-advisor-targets.test.ts` around lines 437 - 497, Add a unit test covering the changedFileSources-omitted fallback in buildE2eTargetNormalizationContext: use a changed file whose repository source contains the hermetic profile tag, omit changedFileSources, and provide the appropriate root/context so the implementation reads the file via fs.readFileSync. Assert the profile target is selected and e2e-all is removed, ensuring the test exercises the real fallback path and catches an undefined-root failure.test/e2e/support/dockerhub-auth-workflow-boundary.test.ts (1)
16-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer the shared executor-job constant here. This duplicates
HERMETIC_EXECUTION_PROFILE.executorJob; importing it keeps this boundary test aligned withtools/e2e/workflow-boundary.mtsand avoids drift if the job id changes.🤖 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/support/dockerhub-auth-workflow-boundary.test.ts` at line 16, The NO_IMAGE_E2E_JOBS declaration duplicates the hermetic executor job identifier. Import HERMETIC_EXECUTION_PROFILE from the shared workflow-boundary module and reference its executorJob property when defining NO_IMAGE_E2E_JOBS, preserving the existing gateway-health-honest entry.
🤖 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/support/execution-profile.test.ts`:
- Around line 156-167: Replace the conditional guard in the “prints one compact
JSON matrix line from the CLI” test with an expect assertion verifying that
selected is defined, then use a non-null assertion when passing selected.id and
serializing selected; preserve the existing test behavior without adding a
branch.
In `@test/e2e/support/workflow-plan.test.ts`:
- Around line 18-22: Update the firstId function to remove the added if
statement while preserving its missing-row error behavior, using a
nullish-coalescing expression that throws when rows[0]?.id is absent.
In `@tools/e2e/execution-profile.mts`:
- Around line 146-203: Relax the validation in listVitestExecutionProfileModules
so entries only require a string file field; treat projectName as optional and
let normalizeVitestFile derive or handle the project when absent. Update the
VitestFile typing or downstream logic as needed so valid entries without
projectName are processed.
---
Nitpick comments:
In `@test/e2e-advisor-targets.test.ts`:
- Around line 437-497: Add a unit test covering the changedFileSources-omitted
fallback in buildE2eTargetNormalizationContext: use a changed file whose
repository source contains the hermetic profile tag, omit changedFileSources,
and provide the appropriate root/context so the implementation reads the file
via fs.readFileSync. Assert the profile target is selected and e2e-all is
removed, ensuring the test exercises the real fallback path and catches an
undefined-root failure.
In `@test/e2e/support/dockerhub-auth-workflow-boundary.test.ts`:
- Line 16: The NO_IMAGE_E2E_JOBS declaration duplicates the hermetic executor
job identifier. Import HERMETIC_EXECUTION_PROFILE from the shared
workflow-boundary module and reference its executorJob property when defining
NO_IMAGE_E2E_JOBS, preserving the existing gateway-health-honest entry.
In `@tools/e2e-advisor/targets.mts`:
- Around line 61-64: Eliminate duplicated test-path rules between targets.mts
and execution-profile.mts by exporting the canonical validation helper or
pattern constants from execution-profile.mts, then importing and using them in
E2E_LIVE_PROFILE_TEST_PATTERN and INTEGRATION_PROFILE_TEST_PATTERN logic.
Preserve the existing classification behavior while ensuring future rule changes
in validateTestFile remain synchronized.
🪄 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: ca66e0fe-cd20-48f7-b4bd-9811523daaa6
📒 Files selected for processing (31)
.github/workflows/e2e.yamltest/e2e-advisor-targets.test.tstest/e2e/README.mdtest/e2e/live/docs-validation.test.tstest/e2e/live/onboard-negative-paths.test.tstest/e2e/live/openshell-version-pin.test.tstest/e2e/live/ubuntu-repo-cli-smoke.test.tstest/e2e/support/dockerhub-auth-workflow-boundary.test.tstest/e2e/support/docs-validation-workflow-boundary.test.tstest/e2e/support/e2e-operations-workflow-boundary.test.tstest/e2e/support/e2e-report-to-pr-workflow-boundary.test.tstest/e2e/support/e2e-workflow.test.tstest/e2e/support/execution-profile.test.tstest/e2e/support/hermetic-workflow-boundary.test.tstest/e2e/support/prepare-e2e-workflow-boundary.test.tstest/e2e/support/upload-e2e-artifacts-workflow-boundary.test.tstest/e2e/support/workflow-plan.test.tstest/gateway-drift-preflight.test.tstest/pr-e2e-gate.test.tstest/pr-risk-plan.test.tstools/e2e-advisor/targets.mtstools/e2e/docs-validation-workflow-boundary.mtstools/e2e/execution-profile.mtstools/e2e/operations-workflow-boundary.mtstools/e2e/pr-e2e-gate.mtstools/e2e/prepare-e2e-workflow-boundary.mtstools/e2e/upload-e2e-artifacts-workflow-boundary.mtstools/e2e/workflow-boundary.mtstools/e2e/workflow-inventory.mtstools/e2e/workflow-plan.mtsvitest.config.ts
💤 Files with no reviewable changes (2)
- test/e2e/support/docs-validation-workflow-boundary.test.ts
- tools/e2e/docs-validation-workflow-boundary.mts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e/support/e2e-report-to-pr-workflow-boundary.test.ts (2)
93-96: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace only the workflow expression, not the whole source line.
This helper is unnecessarily coupled to the exact formatting of the extracted workflow. Injecting the serialized
needsvalue by replacing${{ toJSON(needs) }}preserves the boundary test while tolerating harmless line or spacing changes.Suggested adjustment
const script = reportScript().replace( - "const needs = ${{ toJSON(needs) }};", - `const needs = ${JSON.stringify(needs)};`, + "${{ toJSON(needs) }}", + JSON.stringify(needs), );🤖 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/support/e2e-report-to-pr-workflow-boundary.test.ts` around lines 93 - 96, Update the script transformation in reportScript() usage to replace only the `${{ toJSON(needs) }}` workflow expression with JSON.stringify(needs), preserving the surrounding source line and avoiding dependence on exact formatting.Source: Path instructions
198-285: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for terminal failure, cancellation, and skip results.
These tests cover success and unknown paths, but not the other terminal conclusions explicitly accepted by the report logic. Add table-driven cases for
failure,cancelled, andskipped, asserting their rendered child rows and aggregate status through the PR comment boundary.🤖 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/support/e2e-report-to-pr-workflow-boundary.test.ts` around lines 198 - 285, Add table-driven coverage alongside the existing executeReport tests for terminal API conclusions failure, cancelled, and skipped. For each case, provide a completed Hermetic E2E child job with the matching conclusion, then assert the rendered child row uses the corresponding status and the PR body reports the expected aggregate status, exercising the report boundary through executeReport.Source: Path instructions
🤖 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 `@test/e2e/support/e2e-report-to-pr-workflow-boundary.test.ts`:
- Around line 93-96: Update the script transformation in reportScript() usage to
replace only the `${{ toJSON(needs) }}` workflow expression with
JSON.stringify(needs), preserving the surrounding source line and avoiding
dependence on exact formatting.
- Around line 198-285: Add table-driven coverage alongside the existing
executeReport tests for terminal API conclusions failure, cancelled, and
skipped. For each case, provide a completed Hermetic E2E child job with the
matching conclusion, then assert the rendered child row uses the corresponding
status and the PR body reports the expected aggregate status, exercising the
report boundary through executeReport.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7a96d054-ae1e-4a5c-9daa-9d843f873354
📒 Files selected for processing (7)
.github/workflows/e2e.yamlscripts/checks/e2e-mock-parity.tstest/e2e-mock-parity.test.tstest/e2e/support/e2e-report-to-pr-workflow-boundary.test.tstest/e2e/support/execution-profile.test.tstest/e2e/support/workflow-plan.test.tstools/e2e/execution-profile.mts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/e2e/support/workflow-plan.test.ts
- tools/e2e/execution-profile.mts
- .github/workflows/e2e.yaml
- test/e2e/support/execution-profile.test.ts
Contributor
E2E Target Results —
|
| Job | Result |
|---|---|
| cloud-onboard |
Contributor
E2E Target Results —
|
| Job | Result |
|---|---|
| agent-turn-latency | |
| bedrock-runtime-compatible-anthropic | |
| brave-search | ✅ success |
| channels-add-remove | |
| channels-stop-start | |
| cloud-inference | |
| cloud-onboard | |
| common-egress-agent | |
| concurrent-gateway-ports | |
| credential-migration | |
| credential-sanitization | |
| cron-preflight-inference-local | |
| device-auth-health | |
| diagnostics | |
| docs-validation | |
| double-onboard | |
| full-e2e | |
| gateway-drift-preflight | ✅ success |
| gateway-guard-recovery | |
| gateway-health-honest | ✅ success |
| generate-matrix | ✅ success |
| gpu-double-onboard | |
| gpu-e2e | |
| hermes-dashboard | |
| hermes-discord | |
| hermes-e2e | |
| hermes-gpu-startup | ⏭️ skipped |
| hermes-inference-switch | |
| hermes-shields-config | |
| hermes-slack | |
| inference-routing | |
| issue-2478-crash-loop-recovery | |
| issue-4434-tui-unreachable-inference | ✅ success |
| issue-4462-scope-upgrade-approval | |
| jetson-nvmap-gpu | ⏭️ skipped |
| kimi-inference-compat | |
| launchable-smoke | |
| live | |
| mcp-bridge | |
| mcp-bridge-dev | ⏭️ skipped |
| messaging-compatible-endpoint | |
| messaging-providers | |
| model-router-provider-routed-inference | |
| network-policy | |
| ollama-auth-proxy | |
| onboard-negative-paths | ✅ success |
| onboard-repair | |
| onboard-resume | |
| openclaw-discord-pairing | |
| openclaw-inference-switch | |
| openclaw-plugin-runtime-exdev | |
| openclaw-skill-cli | |
| openclaw-slack-pairing | |
| openclaw-tui-chat-correlation | |
| openshell-gateway-auth-contract | ⏭️ skipped |
| openshell-gateway-upgrade | |
| openshell-version-pin | ✅ success |
| overlayfs-autofix | ✅ success |
| rebuild-hermes | |
| rebuild-hermes-stale-base | |
| rebuild-openclaw | |
| sandbox-operations | |
| sandbox-rebuild | |
| sandbox-rlimits-connect | ⏭️ skipped |
| sandbox-survival | |
| security-posture | |
| sessions-agents-cli | |
| shields-config | |
| skill-agent | |
| snapshot-commands | |
| spark-install | |
| state-backup-restore | |
| telegram-injection | |
| token-rotation | |
| tunnel-lifecycle | |
| ubuntu-repo-cli-smoke | ✅ success |
| upgrade-stale-sandbox |
Explicit-only jobs skipped:
openshell-gateway-auth-contract(default dispatch excludes the resource-heavy OpenShell auth-contract probe unless selected; validate withjobs=openshell-gateway-auth-contractortargets=openshell-gateway-auth-contract),mcp-bridge-dev(default dispatch excludes moving OpenShell dev artifacts unless explicitly selected; validate withjobs=mcp-bridge-devortargets=mcp-bridge-dev),hermes-gpu-startup(default dispatch excludes this explicit-only job unless selected; validate withjobs=hermes-gpu-startuportargets=hermes-gpu-startup),sandbox-rlimits-connect(default dispatch excludes the destructive rlimit fork/connect probe unless selected; validate withjobs=sandbox-rlimits-connectortargets=sandbox-rlimits-connect),jetson-nvmap-gpu(default dispatch excludes Jetson; explicit dispatch requires allow_jetson_runner_queue=true after confirming an online Jetson runner because queued jobs do not honor timeout-minutes before assignment; validate withjobs=jetson-nvmap-gpuortargets=jetson-nvmap-gpu).
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
E2E Target Results — ✅ All requested jobs passedRun: 29139271972
|
Collaborator
Author
|
PRA-1 architecture justification: A compact static ID-to-file helper would be a second test catalog, which is the drift source this PR intentionally removes. The authoritative inputs are the test file and its real Vitest project; the source tag declares membership only. Discovery emits only Those capabilities remain fixed in the single trusted After this warning, the duplicate selector API was removed from discovery: |
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/e2e/support/hermetic-workflow-boundary.test.ts (1)
52-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftAvoid using the production normalizer to define the expected result.
Because
stripExecutionProfileDeclarationsdetermines which files are expected to be discovered, a regression in declaration parsing can cause both the expected set and discovery result to omit the same file. Use independent fixtures or a separately owned predicate so this test verifies behavior rather than reusing implementation logic.As per path instructions, tests should favor behavioral confidence over implementation lock-in.
🤖 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/support/hermetic-workflow-boundary.test.ts` around lines 52 - 55, The test currently derives its expected file set using the production normalizer stripExecutionProfileDeclarations, allowing parsing regressions to make both sides agree incorrectly. Replace that filter with independently defined fixtures or a separately owned predicate that identifies expected execution-profile declaration files, while continuing to compare against the discovery result.Source: Path instructions
🤖 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/support/e2e-report-to-pr-workflow-boundary.test.ts`:
- Line 74: Replace path.delimiter with the literal POSIX ":" separator when
constructing PATH in the test environment, preserving the existing binDirectory
and process.env.PATH values.
In `@test/e2e/support/hermetic-workflow-boundary.test.ts`:
- Around line 46-55: Update the test’s declared-file collection in “keeps every
source-declared execution profile visible to Vitest discovery” to use the same
e2e-live and integration project-scoped module set as Vitest discovery, rather
than globbing all repository test files; derive both expected and discovered
files from that shared scope without adding a separate catalog.
---
Nitpick comments:
In `@test/e2e/support/hermetic-workflow-boundary.test.ts`:
- Around line 52-55: The test currently derives its expected file set using the
production normalizer stripExecutionProfileDeclarations, allowing parsing
regressions to make both sides agree incorrectly. Replace that filter with
independently defined fixtures or a separately owned predicate that identifies
expected execution-profile declaration files, while continuing to compare
against the discovery result.
🪄 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: 67f8e576-d204-4cb7-9034-466836b6a17d
📒 Files selected for processing (2)
test/e2e/support/e2e-report-to-pr-workflow-boundary.test.tstest/e2e/support/hermetic-workflow-boundary.test.ts
Collaborator
Author
|
Nemotron current-head triage:
Final-head addendum for the newly surfaced Nemotron items:
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
E2E Target Results — ❌ Some jobs failedRun: 29139274144
|
Collaborator
Author
|
Following the terminology cleanup in #6662, I think this PR still has too many names for one behavior. The concrete contract in the README and workflow seems straightforward: a tagged test that needs no credentials or custom runner, setup, or timeout runs in one shared Ubuntu E2E job. We currently describe that as a “hermetic execution profile,” “trusted hermetic E2E executor,” “source-declared trusted execution profile,” “logical test ID,” and “free-standing live E2E test ID.” Could we name the concrete things directly throughout?
One adjacent cleanup: the shell formatter explicitly preserves an older |
Contributor
E2E Target Results — ✅ All default jobs passedRun: 29139274144
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
cv
changed the title
Collaborator
Author
|
Addressed the terminology and rollout feedback in
Verification: full E2E-support project (113 files / 910 tests), PR gate/advisor/mock-parity integration suites (52 tests), docs, project-overlap check, CLI typecheck, real base-to-head mock-parity, and current-base compatibility simulation all pass. Two independent follow-up reviews found no unresolved findings. |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
Author
|
PR Review Advisor current-head triage:
No advisor-required defect remains; PRA-1 is the documented no-catalog design decision requested by the maintainer, and all runtime follow-ups passed. |
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tools/e2e-advisor/targets.mts (1)
61-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the credential-free test path classifier
credentialFreeTestProjectForChangedFileduplicates the e2e-live/integration path rules already enforced intools/e2e/credential-free-tests.mts; exporting a shared helper there would keep changed-file classification aligned if those Vitest project boundaries move.🤖 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 `@tools/e2e-advisor/targets.mts` around lines 61 - 64, Share the credential-free test path classification instead of duplicating its regex rules in credentialFreeTestProjectForChangedFile. Export a reusable helper from credential-free-tests.mts and use it in targets.mts for e2e-live and integration detection, preserving the existing project mapping while keeping both classifiers aligned.Source: Path instructions
test/e2e/support/jetson-workflow-boundary.test.ts (1)
51-57: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid coupling this Jetson-scoped test to the full global explicit-only-jobs list.
Asserting an exact, ordered array pulls in unrelated jobs (
openshell-gateway-auth-contract,mcp-bridge-dev,hermes-gpu-startup,sandbox-rlimits-connect) into a test whose stated purpose is Jetson selectability. Any future unrelated explicit-only job addition/reorder will break this test.♻️ Narrow the assertion to what this test claims
- expect(inventory.explicitOnlyJobs).toEqual([ - "openshell-gateway-auth-contract", - "mcp-bridge-dev", - "hermes-gpu-startup", - "sandbox-rlimits-connect", - "jetson-nvmap-gpu", - ]); + expect(inventory.explicitOnlyJobs).toContain("jetson-nvmap-gpu");🤖 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/support/jetson-workflow-boundary.test.ts` around lines 51 - 57, Update the assertion in the Jetson workflow boundary test to avoid comparing the full ordered explicit-only-jobs list. Assert only that the Jetson-specific job “jetson-nvmap-gpu” is present/selectable, using a containment or focused-filter assertion rather than exact array equality.
🤖 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 `@test/e2e/support/jetson-workflow-boundary.test.ts`:
- Around line 51-57: Update the assertion in the Jetson workflow boundary test
to avoid comparing the full ordered explicit-only-jobs list. Assert only that
the Jetson-specific job “jetson-nvmap-gpu” is present/selectable, using a
containment or focused-filter assertion rather than exact array equality.
In `@tools/e2e-advisor/targets.mts`:
- Around line 61-64: Share the credential-free test path classification instead
of duplicating its regex rules in credentialFreeTestProjectForChangedFile.
Export a reusable helper from credential-free-tests.mts and use it in
targets.mts for e2e-live and integration detection, preserving the existing
project mapping while keeping both classifiers aligned.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 645ed18b-0107-4fd3-94d0-9bed2482cfdd
📒 Files selected for processing (28)
.github/workflows/e2e.yamlscripts/checks/e2e-mock-parity.tstest/e2e-advisor-targets.test.tstest/e2e-mock-parity.test.tstest/e2e/README.mdtest/e2e/live/docs-validation.test.tstest/e2e/live/onboard-negative-paths.test.tstest/e2e/live/openshell-version-pin.test.tstest/e2e/live/ubuntu-repo-cli-smoke.test.tstest/e2e/support/credential-free-tests.test.tstest/e2e/support/dockerhub-auth-workflow-boundary.test.tstest/e2e/support/e2e-report-to-pr-workflow-boundary.test.tstest/e2e/support/e2e-workflow.test.tstest/e2e/support/jetson-workflow-boundary.test.tstest/e2e/support/prepare-e2e-workflow-boundary.test.tstest/e2e/support/shared-e2e-workflow-boundary.test.tstest/e2e/support/upload-e2e-artifacts-workflow-boundary.test.tstest/e2e/support/workflow-plan.test.tstest/gateway-drift-preflight.test.tstools/e2e-advisor/targets.mtstools/e2e/credential-free-tests.mtstools/e2e/pr-e2e-gate.mtstools/e2e/prepare-e2e-workflow-boundary.mtstools/e2e/upload-e2e-artifacts-workflow-boundary.mtstools/e2e/workflow-boundary.mtstools/e2e/workflow-inventory.mtstools/e2e/workflow-plan.mtsvitest.config.ts
✅ Files skipped from review due to trivial changes (6)
- test/e2e/live/onboard-negative-paths.test.ts
- test/e2e/live/ubuntu-repo-cli-smoke.test.ts
- test/e2e/live/docs-validation.test.ts
- test/gateway-drift-preflight.test.ts
- test/e2e/live/openshell-version-pin.test.ts
- test/e2e/README.md
🚧 Files skipped from review as they are similar to previous changes (5)
- vitest.config.ts
- test/e2e-advisor-targets.test.ts
- test/e2e-mock-parity.test.ts
- tools/e2e/workflow-plan.mts
- test/e2e/support/e2e-report-to-pr-workflow-boundary.test.ts
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Collaborator
Author
|
Addressed both current-head CodeRabbit nitpicks in
Focused verification passed: credential-free/shared/Jetson E2E-support tests (32), advisor/mock-parity integration tests (34), Biome, repository hooks, pre-push typecheck, and the real base-to-head mock-parity check. |
Collaborator
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Collaborator
Author
|
Reviewed the current-head Nemotron findings against the checked-in contracts:
The Nemotron job itself ended with |
21 tasks
cv
added a commit
that referenced
this pull request
Jul 11, 2026
<!-- markdownlint-disable MD041 --> ## Summary Prevents `cancel-superseded` from failing once completed E2E workflow history exceeds GitHub's 1,000-result search cap. The controller now lists only non-terminal runs, retains fail-closed pagination, and cancels each matching run at most once. It also removes the temporary base-workflow inventory bridge after #6670 landed on `main`. ## Changes - Query the five documented non-terminal workflow-run statuses instead of traversing all historical `workflow_dispatch` runs. - Paginate and validate every status-specific result set before issuing cancellations, retaining the 1,000-result completeness guard per status. - Deduplicate run IDs observed across status transitions and keep completed runs excluded. - Reproduce the escaped completed-history failure and cover multi-page active results, forward status transitions, and no partial cancellation when an active-status search reaches its limit. - Delete the obsolete `workflow-inventory.mts --shell` compatibility CLI and exercise malformed workflow metadata directly through the production inventory reader. ## 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: the documented PR cancellation contract is unchanged, and the deleted temporary CLI had no documented or tracked consumer; final documentation-writer audits confirmed no user-facing update is needed. - [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 controller and cleanup reviews confirmed fixed status constants, PR-title and run-ID validation, benign completion races, validation before cancellation, and no remaining consumer of the deleted bridge. - [ ] 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 — controller integration (1 file, 20 tests); focused E2E workflow boundary (1 file, 17 tests); full `e2e-support` project (115 files, 926 tests) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved pull request gate cancellation to target only eligible “active” workflow runs across multiple run states, avoiding queued or unrelated runs. - Refined pagination handling for active-run discovery and ensured cancellation happens only when the correct run becomes eligible. - Deduplicates discovered runs before issuing any cancellations. - **Tests** - Expanded end-to-end coverage to verify the exact sequence of active-status pagination queries and the cancellation/rejection behavior at page limits. - **Chores** - Removed the `workflow-inventory` CLI used by end-to-end tests, switching validation to direct assertions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Follow-up to NVIDIA#6662, this replaces repeated credential-free E2E jobs with source discovery and one shared Ubuntu E2E job. Tests opt in beside their code with `e2e/credential-free`; the E2E workflow owns the runner, setup, permissions, secrets, timeout, and artifact policy. ## Changes - Discover tagged tests from their real Vitest projects and derive test IDs from unique filenames, with no separate test catalog or manually maintained matrix. - Run five credential-free tests in the `shared-e2e` matrix job, including the previously unwired Ubuntu repository CLI smoke test. - Move selector and matrix planning from inline workflow Bash into tested TypeScript while preserving `jobs`/`targets` selectors, PR reporting, scorecard inputs, and controller evidence. - Keep the temporary four-key shell output required by the current base E2E workflow. It discovers that workflow's actual job IDs, excludes selectors the base cannot run, and can be deleted once this workflow is on `main`. - Reduce `.github/workflows/e2e.yaml` from 5,430 to 5,334 lines and document the contributor contract in `test/e2e/README.md`. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [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 diff and workflow-contract reviews found no credential exposure, path injection, untrusted runner selection, or unresolved Actions contract mismatch; boundary tests pin the credential-free shared job's no-secret setup and artifact contracts. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Verification - [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 — `npx vitest run --project e2e-support` (113 files, 910 tests); PR gate/advisor/mock-parity integration suites (52 tests); `npm run test:projects:check`; `npm run typecheck:cli`; real `origin/main...HEAD` mock-parity check; exact current-base shell/matrix compatibility simulation - [x] Applicable broad gate passed — earlier PR head: `env -u SSH_CONNECTION -u SSH_CLIENT -u SSH_TTY npm test` (1,388 files, 15,964 tests passed; 39 skipped; 1 todo); current terminology change set reran the complete E2E-support project - [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) — passed with 0 errors and 2 existing Fern warnings - [ ] Doc pages follow the [style guide](https://github.com/NVIDIA/NemoClaw/blob/main/docs/CONTRIBUTING.md) (doc changes only) — no `docs/` pages changed - [ ] New doc pages include SPDX header and frontmatter (new pages only) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added discovery and execution of “credential-free” E2E tests via a shared workflow job. * Added a dedicated workflow planner that builds a live test matrix plus a credential-free test matrix from typed selectors. * Improved PR reporting to include per-test results (including handling for unavailable/unknown results). * **Bug Fixes** * Improved selector parsing/selection behavior for shared E2E routing and dispatch targeting. * Prevented metadata-only changes from being treated as relevant for mock-parity checks. * **Documentation** * Added documentation for credential-free tests, tagging, discovery rules, and local inspection commands. * **Tests** * Extended E2E coverage for selector normalization, credential-free discovery, workflow boundaries, and PR reporting scenarios (including unknown/missing results). <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Hadar301
pushed a commit
to Hadar301/NemoClaw-OpenShift
that referenced
this pull request
Jul 12, 2026
<!-- markdownlint-disable MD041 --> ## Summary Prevents `cancel-superseded` from failing once completed E2E workflow history exceeds GitHub's 1,000-result search cap. The controller now lists only non-terminal runs, retains fail-closed pagination, and cancels each matching run at most once. It also removes the temporary base-workflow inventory bridge after NVIDIA#6670 landed on `main`. ## Changes - Query the five documented non-terminal workflow-run statuses instead of traversing all historical `workflow_dispatch` runs. - Paginate and validate every status-specific result set before issuing cancellations, retaining the 1,000-result completeness guard per status. - Deduplicate run IDs observed across status transitions and keep completed runs excluded. - Reproduce the escaped completed-history failure and cover multi-page active results, forward status transitions, and no partial cancellation when an active-status search reaches its limit. - Delete the obsolete `workflow-inventory.mts --shell` compatibility CLI and exercise malformed workflow metadata directly through the production inventory reader. ## 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: the documented PR cancellation contract is unchanged, and the deleted temporary CLI had no documented or tracked consumer; final documentation-writer audits confirmed no user-facing update is needed. - [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 controller and cleanup reviews confirmed fixed status constants, PR-title and run-ID validation, benign completion races, validation before cancellation, and no remaining consumer of the deleted bridge. - [ ] 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 — controller integration (1 file, 20 tests); focused E2E workflow boundary (1 file, 17 tests); full `e2e-support` project (115 files, 926 tests) - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: - [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> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved pull request gate cancellation to target only eligible “active” workflow runs across multiple run states, avoiding queued or unrelated runs. - Refined pagination handling for active-run discovery and ensured cancellation happens only when the correct run becomes eligible. - Deduplicates discovered runs before issuing any cancellations. - **Tests** - Expanded end-to-end coverage to verify the exact sequence of active-status pagination queries and the cancellation/rejection behavior at page limits. - **Chores** - Removed the `workflow-inventory` CLI used by end-to-end tests, switching validation to direct assertions. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Carlos Villela <cvillela@nvidia.com>
This was referenced Jul 26, 2026
wscurran
added
area: ci
This was referenced Aug 6, 2026
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
Follow-up to #6662, this replaces repeated credential-free E2E jobs with source discovery and one shared Ubuntu E2E job. Tests opt in beside their code with
e2e/credential-free; the E2E workflow owns the runner, setup, permissions, secrets, timeout, and artifact policy.Changes
shared-e2ematrix job, including the previously unwired Ubuntu repository CLI smoke test.jobs/targetsselectors, PR reporting, scorecard inputs, and controller evidence.main..github/workflows/e2e.yamlfrom 5,430 to 5,334 lines and document the contributor contract intest/e2e/README.md.Type of Change
Quality Gates
Verification
Verifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpx vitest run --project e2e-support(113 files, 910 tests); PR gate/advisor/mock-parity integration suites (52 tests);npm run test:projects:check;npm run typecheck:cli; realorigin/main...HEADmock-parity check; exact current-base shell/matrix compatibility simulationenv -u SSH_CONNECTION -u SSH_CLIENT -u SSH_TTY npm test(1,388 files, 15,964 tests passed; 39 skipped; 1 todo); current terminology change set reran the complete E2E-support projectnpm run docsbuilds without warnings (doc changes only) — passed with 0 errors and 2 existing Fern warningsdocs/pages changedSigned-off-by: Carlos Villela cvillela@nvidia.com
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests