fix(openshell): restore default gateway qualification - #11601
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 changes restrict legacy managed gateway-state validation to the canonical default directory and add coverage for explicit overrides. They also add shared hosted inference credential-reference validation and apply it to runtime and support tests. ChangesManaged gateway state validation
Hosted inference credential references
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The PR is mergeable with no remaining actionable risk identified in the changed gateway validation or hosted-inference reference checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall line coverage in commit 1f5269c in the TypeScript / code-coverage/cliThe overall line coverage in commit 1f5269c in the Show a line coverage summary of the most impacted files.
Updated |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
rsliter
left a comment
There was a problem hiding this comment.
Reviewed the complete six-file diff at 092fdef against 37261ef. The canonical default gateway exception remains bounded by the fixed path, trusted ancestors, owner-only state directory, local loopback endpoint, and no-follow bounded TLS reads; explicit overrides still require the ownership marker. The credential reference grammar is bounded and matches the current vNN and stable-hash forms. Security review passes injection, auth, secrets, input validation, configuration, dependencies, serialization, network, error handling, and race checks. All five commits are GitHub-verified. Local build and focused tests pass (93 CLI, 19 E2E-support), CodeRabbit has no actionable finding, and exact-head self-hosted qualification 34666334897 passed. Ordinary exact-head CI 34666334600 is still running and remains a merge gate.
|
PR Review Advisor finished for commit |
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
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
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 at1f5269c49138fea461d3351f5d5c39c73f5d1ba1against canonical main37261efb99c0dcbfcef6551efa0c59357e96fcbe.inference-routing,network-policyin mock mode against head092fdefc472882cd135a4f839d2285da4969b467, with correlation57e03ed4-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. Commit1f5269c49138fea461d3351f5d5c39c73f5d1ba1reuses the bounded credential-reference matcher for the rotated placeholder.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
37261efb99c0dcbfcef6551efa0c59357e96fcbe; that fix is integrated into the current head, so qualification has resumed.Signed-off-by: Julie Yaunches jyaunches@nvidia.com