test(onboard): serialize dashboard port fixture - #11598
Conversation
|
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. |
|
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 (1)
Included review availability: Your plan provides up to 12 included reviews per hour; 10 remain after this review. 📝 WalkthroughWalkthroughThe sandbox-reuse messaging-provider test now runs sequentially to prevent concurrent execution. ChangesOnboarding messaging test
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~2 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The test scheduling change is ready to merge. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit cb41cdc in the TypeScript / code-coverage/cliThe overall line coverage in commit cb41cdc in the Show a line coverage summary of the most impacted files.
Updated |
rsliter
left a comment
There was a problem hiding this comment.
Reviewed exact commit cb41cdc against the two observed shard-9 dashboard-port collisions. The one-line it.sequential change isolates only the fixture that exercises the real shared dashboard-port allocator while preserving concurrency for the remaining cases. Exact-head required CI passes, including shard 9; CodeRabbit found no actionable issue; DCO and commit verification pass. Approval applies to the implementation. Merge remains separately gated on the repository review lifecycle.
## Outcome Configuration export can read a NemoClaw-managed gateway from the canonical owner-private default state directory even when that directory predates explicit ownership markers. The selected inference-routing E2E also accepts OpenShell's current scope-bound credential placeholder and continues to verify delegated bearer resolution. ## Reason Exact-candidate run 34649953836 and exact-base replay 34651044220 reproduced the same two failures: default-gateway SDK reads stopped before the sandbox inventory RPC, and TC-INF-12 rejected the valid OpenShell 0.0.116 scope-bound placeholder. These inherited failures block deterministic qualification of #11587. ### Related issues Refs #11421 ## Changes - Preserve strict ownership-marker requirements for explicit gateway state overrides while recognizing the canonical owner-private default state used by installations created before markers. - Cover both the legacy-default connection path and rejection of an unmarked explicit override. - Share the bounded OpenShell placeholder grammar for unscoped, generation-scoped, and scope-bound credentials between TC-INF-12 and its mapped fast test. - Integrate current main so trusted E2E compares the candidate against the exact PR base instead of treating intervening main changes as candidate-managed-image inputs. ## Verification - `npx vitest run --project cli src/lib/adapters/openshell/sandbox-command-sdk.test.ts src/lib/adapters/config/live-export-source.test.ts` — 93 tests passed. - `npx vitest run --project e2e-support test/e2e/support/hosted-inference.test.ts` — 19 tests passed. - `npx tsx scripts/checks/e2e-mock-parity.mts --base origin/main --head HEAD` — passed. - `NODE_OPTIONS=--max-old-space-size=8192 npm run typecheck:cli` — passed. - `npm run test:changed` — the growth-guardrail preflight passed; no additional source tests were selected after base integration. - `NODE_OPTIONS=--max-old-space-size=8192 npm run validate:pr` — passed at `1f5269c49138fea461d3351f5d5c39c73f5d1ba1` against canonical main `37261efb99c0dcbfcef6551efa0c59357e96fcbe`. - The pre-repair exact-head hosted CI run [34666334600](https://github.com/NVIDIA/NemoClaw/actions/runs/34666334600) passed every selected job, including all 12 CLI shards. Current-head run [34668234877](https://github.com/NVIDIA/NemoClaw/actions/runs/34668234877) is qualifying the one-line E2E repair. - Trusted manual PR E2E run [34667270786](https://github.com/NVIDIA/NemoClaw/actions/runs/34667270786) ran `inference-routing,network-policy` in mock mode against head `092fdefc472882cd135a4f839d2285da4969b467`, with correlation `57e03ed4-c1ad-4a14-9f45-112ad6c8d10a`. Network policy passed. TC-INF-12 failed after credential rotation because its second assertion still required only the generation-scoped placeholder. Cleanup and artifact upload passed. Commit `1f5269c49138fea461d3351f5d5c39c73f5d1ba1` reuses the bounded credential-reference matcher for the rotated placeholder. - The diff contains no secrets, API keys, or credentials. ## Review notes Alternative review completed for `1f5269c49138fea461d3351f5d5c39c73f5d1ba1`. Local Advisor failed before specialist analysis because its OpenShell sandbox never passed supervisor-relay readiness (`sandbox is not ready`, SSH status 255); cleanup succeeded, but no specialist artifacts were produced. Manual review of the full diff and the one-line E2E repair found no remaining actionable issue. The repair reuses the bounded matcher already exercised by the fast support test. Focused tests and canonical validation passed. The residual limitation is the absence of Advisor specialist artifacts; current-head CodeRabbit and trusted E2E evidence remain pending. ## Dependency - [#11598](#11598) merged as `37261efb99c0dcbfcef6551efa0c59357e96fcbe`; that fix is integrated into the current head, so qualification has resumed. --- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> --------- Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
Outcome
Keeps the dashboard-port-owning onboard-messaging fixture out of sibling overlap, preventing the repeated port collision seen in sharded CI. The other 13 fixtures remain concurrent, preserving nearly all of the suite speedup from #11581.
Reason
The merged parallelization made all 14 child-process fixtures concurrent, but this one case still exercises the real shared dashboard-port allocator. It failed twice on clean GitHub runners because a sibling fixture held the same default port.
Changes
reuses sandbox without refreshing unselected ambient messaging providersas sequential inside the concurrent suite.Verification
npx vitest run --project integration test/onboarding/onboard-messaging.test.ts --sequence.shuffle --sequence.seed=11598— 14/14 passed in 28.52s test time.npx vitest run --project integration test/automation/pull-requests/growth-guardrails.test.ts— 7/7 passed.npm run build:cli— passed.npm --prefix nemoclaw run build— passed.NODE_OPTIONS=--max-old-space-size=8192 npm run check:diff— passed, including formatting, lint, secret scan, source-shape budget, repository checks, and CLI type-check.cb41cdcb40ab77ddd084bf371366012cc175adf1— all 12 CLI shards and all aggregate checks passed; shard 9 passed in 8m46s.cb41cdcb40ab77ddd084bf371366012cc175adf1is Verified.Review notes
Primary evidence: the same untouched fixture failed on the original shard and its clean retry. Both failures were default dashboard-port collisions after #11581 made the suite concurrent.
Signed-off-by: Charan Jagwani cjagwani@nvidia.com
Summary by CodeRabbit