refactor(e2e): remove Jetson dispatch backend - #8871
Conversation
Signed-off-by: San Dang <sdang@nvidia.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe PR replaces Colossus-specific Jetson dispatch with an operator-owned HTTP contract and client. It adds v1 contract fixtures, validation tests, workflow-boundary updates, and operator documentation. It removes the former Colossus deployment, service, worker, lifecycle, and cleanup components. ChangesJetson dispatch migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActions
participant dispatcherRequest
participant JetsonDispatchController
participant JetsonDispatchContract
GitHubActions->>dispatcherRequest: submit dispatch request
dispatcherRequest->>JetsonDispatchController: authenticated HTTP request
JetsonDispatchController-->>dispatcherRequest: queued job status
dispatcherRequest->>JetsonDispatchController: poll job status
JetsonDispatchController-->>dispatcherRequest: completed status and artifact
dispatcherRequest->>JetsonDispatchContract: parse status and artifact
JetsonDispatchContract-->>GitHubActions: validated dispatch result
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
test/e2e/support/jetson-dispatch-client.test.ts (2)
256-276: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCover the streaming byte bound, not only the
Content-Lengthshort circuit.This test sets a
Content-Lengthheader of65537, so it exercises only the header pre-check indispatcherRequest.dispatcherRequesthas a second bound that accumulatesresponseByteswhile reading the body and throws when the total passesmaxBytes.The streaming bound is the branch that matters more.
Content-Lengthcomes from the dispatcher, so a backend can omit it; the streaming accumulator is then the only limit. Add a case that returns a body stream with noContent-Length.💚 Proposed additional case
+ it("rejects an oversized streamed response without Content-Length (`#8142`)", async () => { + const chunk = new TextEncoder().encode("x".repeat(4096)); + const fetchImpl = vi.fn( + async () => + new Response( + new ReadableStream({ + pull(controller) { + controller.enqueue(chunk); + }, + }), + { status: 202 }, + ), + ); + + await expect( + dispatcherRequest({ + baseUrl: new URL("https://dispatch.test/"), + method: "GET", + path: "v1/jobs", + maxBytes: 8 * 1024, + fetchImpl, + tokenProvider: async () => "oidc-token", + }), + ).rejects.toThrow("Jetson dispatcher response is too large"); + });🤖 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-dispatch-client.test.ts` around lines 256 - 276, Extend the dispatcherRequest rejection tests with a case that omits Content-Length and returns a Response body stream whose accumulated chunks exceed maxBytes. Assert that reading the stream rejects with "Jetson dispatcher response is too large", covering the responseBytes enforcement branch rather than the header pre-check.
57-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the rejection reason, not just that an error was thrown.
Lines 58, 103, and 163 use a bare
toThrow(). That assertion passes for any thrown error, including one raised for an unintended reason. Two cases are sensitive to this:
- Line 85 builds
{...completedStatus, device: undefined}to test the "successful result must include device identity" rule.requireFieldslistsdeviceas optional, so the intended error comes from line 257 oftools/e2e/jetson-dispatch-contract.mts. Ifdevicewere ever dropped from the optional list, the test would still pass on the unknown-field error instead.- Line 144 supplies a mismatched
expectedJobId. Any earlier validation failure would satisfy the assertion.The file already uses the stronger form at lines 67 and 133. Add an expected message per case so each test pins the rule it claims to exercise.
As per path instructions: "Flag copied production algorithms, broad mocks that bypass the behavior under test, and conditionals that make a test pass without exercising its claim."♻️ Proposed change for the status cases
it.each([ { name: "an extra status field", value: { ...(vectors.queuedResponse as { job: object }).job, command: "untrusted" }, + expectedError: "queued Jetson dispatch status fields do not match", }, { name: "a successful result without device identity", value: { ...(vectors.completedStatus as Record<string, unknown>), device: undefined }, + expectedError: "successful Jetson dispatch must include device identity", }, - ])("rejects $name (`#8142`)", ({ value }) => { - expect(() => parseJetsonDispatchStatus(value)).toThrow(); + ])("rejects $name (`#8142`)", ({ expectedError, value }) => { + expect(() => parseJetsonDispatchStatus(value)).toThrow(expectedError); });Also applies to: 102-104, 160-164
🤖 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-dispatch-client.test.ts` around lines 57 - 59, Replace the bare toThrow() assertions in the invalidRequests, status, and mismatched expectedJobId cases with message-specific expectations, matching the existing stronger assertion style used in the file. Use the exact contract error message for each scenario so the tests verify the intended validation rule rather than any unrelated thrown error.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/README.md`:
- Line 800: Update the sentence describing the `jetson-nvmap-gpu` skip condition
to name the workflow input explicitly as `allow_jetson_dispatch` instead of
referring to “its dispatch flag,” matching the terminology used elsewhere in the
document.
In `@test/maintainer-e2e-skill.test.ts`:
- Line 225: Update the test description for the full dispatch scenario to use
the repository’s established “operator-owned” terminology instead of
“operator-backend,” matching risk-plan.mts and the PR wording.
In `@tools/e2e/jetson-dispatch-contract.mts`:
- Around line 232-240: Update the completed-result validation around the status
conclusion and cleanup membership checks to require the raw values to be strings
before comparing them against the allowed string literals. Remove the
String(...) coercions while preserving the existing cleanup/conclusion
consistency rule, so values later cast into JetsonDispatchStatus remain valid
declared types.
---
Nitpick comments:
In `@test/e2e/support/jetson-dispatch-client.test.ts`:
- Around line 256-276: Extend the dispatcherRequest rejection tests with a case
that omits Content-Length and returns a Response body stream whose accumulated
chunks exceed maxBytes. Assert that reading the stream rejects with "Jetson
dispatcher response is too large", covering the responseBytes enforcement branch
rather than the header pre-check.
- Around line 57-59: Replace the bare toThrow() assertions in the
invalidRequests, status, and mismatched expectedJobId cases with
message-specific expectations, matching the existing stronger assertion style
used in the file. Use the exact contract error message for each scenario so the
tests verify the intended validation rule rather than any unrelated thrown
error.
🪄 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: 71d176fd-e6bd-4bda-a193-49a302ba7d65
📒 Files selected for processing (27)
.agents/skills/nemoclaw-maintainer-e2e/SKILL.md.github/workflows/e2e.yamltest/e2e/README.mdtest/e2e/docs/README.mdtest/e2e/docs/jetson-colossus-dispatch.mdtest/e2e/docs/jetson-dispatch.mdtest/e2e/support/colossus-jetson-dispatch-deploy.test.tstest/e2e/support/jetson-dispatch-client.test.tstest/e2e/support/jetson-dispatch-contract.test.tstest/e2e/support/jetson-dispatch-worker.test.tstest/e2e/support/jetson-workflow-boundary.test.tstest/helpers/vitest-watch-triggers.tstest/maintainer-e2e-skill.test.tstest/vitest-watch-triggers.test.tstools/advisors/risk-plan.mtstools/e2e/cli-artifact-workflow-boundary.mtstools/e2e/colossus-jetson-dispatch-deploy.shtools/e2e/colossus-jetson-dispatch.environmenttools/e2e/contracts/v1/jetson-dispatch.jsontools/e2e/jetson-dispatch-cleanup.shtools/e2e/jetson-dispatch-client.mtstools/e2e/jetson-dispatch-contract.mtstools/e2e/jetson-dispatch-lifecycle.mtstools/e2e/jetson-dispatch-service.mtstools/e2e/jetson-dispatch-worker.mtstools/e2e/nemoclaw-jetson-dispatch.servicetools/e2e/workflow-boundary.mts
💤 Files with no reviewable changes (11)
- tools/e2e/nemoclaw-jetson-dispatch.service
- test/e2e/docs/jetson-colossus-dispatch.md
- test/e2e/support/jetson-dispatch-worker.test.ts
- test/e2e/support/jetson-dispatch-contract.test.ts
- test/e2e/support/colossus-jetson-dispatch-deploy.test.ts
- tools/e2e/jetson-dispatch-cleanup.sh
- tools/e2e/jetson-dispatch-worker.mts
- tools/e2e/jetson-dispatch-lifecycle.mts
- tools/e2e/colossus-jetson-dispatch.environment
- tools/e2e/jetson-dispatch-service.mts
- tools/e2e/colossus-jetson-dispatch-deploy.sh
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 0c06125 in the TypeScript / code-coverage/cliThe overall coverage in commit 0c06125 in the Show a code coverage summary of the most impacted files.
Updated |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
4 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 3 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 for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
Signed-off-by: Charan Jagwani <cjagwani@nvidia.com>
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
Approved at exact head 0c06125. Reviewed the retained Jetson dispatch contract/client boundary and backend removal, including all nine security categories. The branch is current with main and cleanly mergeable; the repository gate reports all 51 checks green, no unresolved major/critical CodeRabbit findings, DCO present, and all commits Verified. Exact-head PR Review Advisor reports 0 blockers, 0 warnings, and 0 suggestions in the primary lane; the second opinion has no blockers. The two fixture regressions exposed after refreshing from main were fixed and all affected CLI shards now pass.
3 participants
Summary
Remove the Jetson dispatch backend from NemoClaw after the operator-owned service completed recovery and a trusted GitHub proof run. NemoClaw continues to own the GitHub controller, strict versioned HTTP client contract, dispatch receipts, artifacts, and live Jetson target.
The backend is moving to the operator-owned nemoclaw-colossus-jetson-tunnel project. This link records the migration destination for this change; NemoClaw documentation describes the backend as operator-owned infrastructure.
Related Issue
Closes #8142
Changes
e2e.yamlcontroller, GitHub OIDC token use,JETSON_DISPATCH_URL, receipt and artifact handling, andjetson-nvmap-gpulive target.1.0.0and a static compatibility vector in NemoClaw because the controller cannot depend on an operator backend package at build time.jetson-dispatch-client.test.tsprotects every request, status, artifact, OIDC, response-bound, and cancellation behavior at that boundary.Type of Change
Quality Gates
Documentation Writer Review
docs-updated.agents/skills/nemoclaw-maintainer-e2e/SKILL.md,.github/workflows/e2e.yaml,test/e2e/README.md,test/e2e/docs/README.md, deletedtest/e2e/docs/jetson-colossus-dispatch.md, and newtest/e2e/docs/jetson-dispatch.mdCodex CLIDGX 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 unavailablenpm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result: full E2E-support reached 2,358 passing tests; three unrelated OpenShell user-service tests failed because this host rejects the untrustednvidia/openshellHomebrew tap.npm run docsbuilds without warnings (doc changes only)External rollout proof: GitHub Actions run 31571534932 dispatched candidate
5aaf22b948162d0d697d408f91b2d3b8549860c9as jobe8494d0e5b9a9fb49657cfb92426d9b0d8b31c558bafb45065c2e881507a053f. NemoClaw and job-local OpenShell installation passed, cleanup succeeded, and the run reached the separately scoped/dev/nvmapGID 44 versus 998 failure.Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
New Features
Documentation
Tests