test(e2e): migrate messaging compatible endpoint#5362
Merged
Conversation
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
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:
📝 WalkthroughWalkthroughAdds a live Vitest e2e scenario that runs a local OpenAI-compatible mock, orchestrates host/sandbox onboarding, validates inference and agent flows (including hop-by-hop header leakage checks), provides helper utilities and tests, and wires a new free-standing workflow job plus boundary validators and dispatch tests. ChangesMessaging-compatible endpoint scenario and workflow integration
Transient provider and network-policy test infrastructure improvements
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorFailed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-advisor-raw-output.txt |
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: None Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorFailed: Could not parse JSON from advisor output; see /home/runner/work/NemoClaw/NemoClaw/artifacts/e2e-advisor/e2e-scenario-advisor-raw-output.txt |
Contributor
PR Review AdvisorFindings: 0 needs attention, 1 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
This is an automated advisory review. A human maintainer must make the final merge decision. |
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27437963918
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27437963971
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27437777296
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27438223514
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27438465910
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27438466027
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27438465945
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27438779383
|
|
|
||
| function nodeEvalArg(source: string): string { | ||
| const encoded = Buffer.from(source, "utf8").toString("base64"); | ||
| return `eval(Buffer.from(${JSON.stringify(encoded)}, "base64").toString("utf8"))`; |
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27438914870
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27438914973
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27438914913
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27439518876
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27439518922
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27439518985
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27440106575
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27440106503
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27442141768
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27442141735
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27442601602
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27443004907
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27443006263
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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-scenario/live/messaging-compatible-endpoint-helpers.ts`:
- Around line 155-168: parseOpenClawAgentText currently treats record content
values as plain strings (using add(record[key])) and therefore skips structured
payloads like content: [{ type: "output_text", text: "..." }]; update the logic
in the loop that iterates containerKeys/record keys (and the choices handling)
to detect arrays/objects and recurse or traverse them to extract nested text
fields (e.g., content[].text and content[].message/delta) before calling add or
collect, keeping existing helpers add and collect and reusing the choices/record
handling to walk nested objects/arrays rather than only accepting strings.
In `@test/e2e-scenario/live/messaging-compatible-endpoint.test.ts`:
- Around line 153-159: The mock currently returns 200 for GET /v1/models and
/models even when no bearer token is provided; update the conditional handling
in the GET branch so it requires auth (e.g., check that auth is present/truthy)
before returning the model list: if auth is missing, respond with a 401 JSON
error and do not push the request to requests, otherwise proceed to push ({
method: "GET", path: requestPath, auth, hopHeaders: [] }) and return the 200
jsonResponse with the model payload using the existing jsonResponse helper and
the model variable.
In `@test/e2e-scenario/live/network-policy.test.ts`:
- Around line 67-70: The wrapper always returns a non-empty command which masks
empty-input checks in trustedSandboxShellScript; modify shellEvalArg to first
detect an empty or whitespace-only script and return an empty string immediately
(so callers can validate/handle empties), otherwise perform the base64 encoding
and return the printf|base64 -d|sh wrapper as before; ensure callers of
shellEvalArg (e.g., trustedSandboxShellScript) continue to rely on an empty
return value to signal no-op input.
🪄 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: a1de2f9f-18bd-4cf0-9cc4-aae0fc7239c0
📒 Files selected for processing (10)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/live/messaging-compatible-endpoint-helpers.tstest/e2e-scenario/live/messaging-compatible-endpoint.test.tstest/e2e-scenario/live/network-policy-transient-provider.tstest/e2e-scenario/live/network-policy.test.tstest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstest/e2e-scenario/support-tests/messaging-compatible-endpoint-helpers.test.tstest/e2e-scenario/support-tests/network-policy-transient-provider.test.tstools/e2e-scenarios/free-standing-jobs.envtools/e2e-scenarios/workflow-boundary.mts
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27443238420
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27443236917
|
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Contributor
Vitest E2E Scenario Results — ❌ Some jobs failedRun: 27444186648
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27444185425
|
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27444255765
|
Collaborator
Author
|
PR Advisor follow-up: confirmed against the #5098 migration scope for this task that this PR intentionally migrates only the messaging-compatible-endpoint Vitest slice. Legacy shell deletion and durable nightly shell wiring removal remain deferred to the #5098 Phase 11 cleanup so this PR can preserve the existing shell/system boundary while adding the new live Vitest coverage. |
Contributor
Vitest E2E Scenario Results — ✅ All jobs passedRun: 27444600238
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts (2)
677-686: ⚡ Quick winAssert the derived
report-to-prfailure here too.This synthetic
FREE_STANDING_VITEST_JOBtest only checks the per-job violations. If the inventory-to-report-to-prcoupling regresses, this still passes even though PR reporting would stop waiting onad-hoc-derived-vitest. Addreport-to-pr job must wait for ad-hoc-derived-vitestto the expected errors.🤖 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-scenario/support-tests/e2e-scenarios-workflow.test.ts` around lines 677 - 686, The test's expected violations list for validateE2eVitestScenariosWorkflowBoundary is missing the coupling assertion that the report-to-pr job depends on ad-hoc-derived-vitest; update the array passed to expect.arrayContaining in the test (the block where validateE2eVitestScenariosWorkflowBoundary(workflowPath) is asserted) to include the string "report-to-pr job must wait for ad-hoc-derived-vitest" so the synthetic FREE_STANDING_VITEST_JOB scenario also asserts the inventory-to-report-to-pr dependency regression.
701-708: ⚡ Quick winStop mutating the workflow fixture with exact-string replacements.
Both tests depend on literal indentation and newline matches to rename the job and remove its
report-to-prneed. A harmless YAML reformat or reorderedneedslist will leave the fixture unchanged and make these tests fail for the wrong reason. Parse the workflow and editjobs[...]/jobs["report-to-pr"].needsstructurally, like the object-based mutation used above.Also applies to: 730-737
🤖 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-scenario/support-tests/e2e-scenarios-workflow.test.ts` around lines 701 - 708, The tests currently mutate the workflow fixture via fragile string replacements on the workflow variable when writing to renamedWorkflowPath and missingReportNeedPath; instead parse the YAML into an object, locate and rename the job under jobs (e.g., update jobs["runtime-overrides-vitest"] to jobs["runtime-overrides-missing"] or move its entry), and remove the "runtime-overrides-vitest" entry from jobs["report-to-pr"].needs structurally, then serialize the YAML back to disk and write to renamedWorkflowPath/missingReportNeedPath; use the same object-based mutation pattern used elsewhere in the file (jobs[...] and jobs["report-to-pr"].needs) to avoid brittle exact-string edits.
🤖 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-scenario/support-tests/e2e-scenarios-workflow.test.ts`:
- Around line 677-686: The test's expected violations list for
validateE2eVitestScenariosWorkflowBoundary is missing the coupling assertion
that the report-to-pr job depends on ad-hoc-derived-vitest; update the array
passed to expect.arrayContaining in the test (the block where
validateE2eVitestScenariosWorkflowBoundary(workflowPath) is asserted) to include
the string "report-to-pr job must wait for ad-hoc-derived-vitest" so the
synthetic FREE_STANDING_VITEST_JOB scenario also asserts the
inventory-to-report-to-pr dependency regression.
- Around line 701-708: The tests currently mutate the workflow fixture via
fragile string replacements on the workflow variable when writing to
renamedWorkflowPath and missingReportNeedPath; instead parse the YAML into an
object, locate and rename the job under jobs (e.g., update
jobs["runtime-overrides-vitest"] to jobs["runtime-overrides-missing"] or move
its entry), and remove the "runtime-overrides-vitest" entry from
jobs["report-to-pr"].needs structurally, then serialize the YAML back to disk
and write to renamedWorkflowPath/missingReportNeedPath; use the same
object-based mutation pattern used elsewhere in the file (jobs[...] and
jobs["report-to-pr"].needs) to avoid brittle exact-string edits.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 78f15341-2644-459f-9d28-38180ae3b81e
📒 Files selected for processing (3)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/support-tests/e2e-scenarios-workflow.test.tstools/e2e-scenarios/workflow-boundary.mts
🚧 Files skipped from review as they are similar to previous changes (2)
- .github/workflows/e2e-vitest-scenarios.yaml
- tools/e2e-scenarios/workflow-boundary.mts
…compatible-endpoint
…compatible-endpoint
…compatible-endpoint
…compatible-endpoint
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
3 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
Migrates
test/e2e/test-messaging-compatible-endpoint.shinto a focused live Vitest scenario with a local OpenAI-compatible endpoint mock and Telegram-enabled custom-provider onboarding. Adds the free-standing workflow dispatch job and boundary tests needed to run the migrated scenario from CI.Related Issue
Refs #5098
Changes
test/e2e-scenario/live/messaging-compatible-endpoint.test.tscovering the legacy compatible endpoint,inference.local, OpenClaw agent turn, Telegram config, and proxy hop-header stripping contracts.messaging-compatible-endpoint-vitestin the free-standing E2E inventory and workflow dispatch job.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Targeted verification run:
npx biome check --write test/e2e-scenario/live/messaging-compatible-endpoint.test.ts test/e2e-scenario/live/network-policy-transient-provider.ts test/e2e-scenario/support-tests/network-policy-transient-provider.test.tsnpx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts test/e2e-scenario/support-tests/network-policy-transient-provider.test.ts --reporter=defaultNEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/messaging-compatible-endpoint.test.ts -t "messaging-compatible-endpoint live test local classifiers" --reporter=defaultnpm run typecheck:clinpm run build:clinpx tsx scripts/check-test-file-size-budget.ts test/e2e-scenario/live/messaging-compatible-endpoint.test.tsNote:
npx prek run --files ...reached the broadtest-clihook and failed on unrelated 5s timeout tests (test/cli/snapshot-shields.test.tsandsrc/lib/onboard/web-search-flow.test.ts). The migration-specific support tests passed in that run.Signed-off-by: Carlos Villela cvillela@nvidia.com
Scope Note
Confirmed against #5098 migration scope: this PR intentionally migrates only the messaging-compatible endpoint Vitest slice. Deleting
test/e2e/test-messaging-compatible-endpoint.shand removing any durable legacy/nightly shell wiring remain deferred to #5098 Phase 11 cleanup.Summary by CodeRabbit
Tests
Chores