refactor(messaging): clean up provider credential hashes from SandboxEntry (registry) - #5410
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 (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRemoves ChangesRemove providerCredentialHashes from sandbox registry
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@ci/test-file-size-budget.json`:
- Line 11: The file size budget increase for test/nemoclaw-start.test.ts in
ci/test-file-size-budget.json (from 5230 to 5231) is a legitimate change caused
by commit e865b500 but lacks documentation. Document this secondary file size
adjustment in the PR description or commit message to explain that the increase
results from the intentional change to remove provider credential hashes from
sandbox entries, clarifying to reviewers that this is an expected secondary
modification alongside the primary objective of adjusting
test/onboard-messaging.test.ts.
🪄 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: fd1e96e8-7f21-473e-b9aa-c271b0ca8d79
📒 Files selected for processing (9)
ci/test-file-size-budget.jsonsrc/lib/actions/sandbox/rebuild.tssrc/lib/onboard.tssrc/lib/onboard/sandbox-registration.test.tssrc/lib/onboard/sandbox-registration.tssrc/lib/state/registry.tstest/e2e/test-channels-stop-start.shtest/e2e/test-rebuild-hermes.shtest/onboard-messaging.test.ts
💤 Files with no reviewable changes (7)
- src/lib/actions/sandbox/rebuild.ts
- test/e2e/test-channels-stop-start.sh
- test/onboard-messaging.test.ts
- src/lib/state/registry.ts
- src/lib/onboard/sandbox-registration.ts
- src/lib/onboard.ts
- src/lib/onboard/sandbox-registration.test.ts
…redential-hashes # Conflicts: # ci/test-file-size-budget.json
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in the Show a code coverage summary of the most covered files.
TypeScript / code-coverage/cliThe overall coverage in the Show a code coverage summary of the most covered files.
Updated |
PR Review AdvisorFindings: 0 needs attention, 4 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
✅ Action performedReview finished.
|
Selective E2E Results — ✅ All requested jobs passedRun: 27488768739
|
…redential-hashes # Conflicts: # ci/test-file-size-budget.json
## Summary Refreshes release-prep documentation for NemoClaw v0.0.65. Adds the v0.0.65 release-notes section and refreshes generated `nemoclaw-user-*` skills from the Fern MDX source docs. ## Changes - Added the v0.0.65 release notes to `docs/about/release-notes.mdx` with links to the deeper docs pages for lifecycle, troubleshooting, inference, CLI commands, messaging, credentials, network policy, Hermes, and sub-agents. - Regenerated the `nemoclaw-user-*` skills with `scripts/docs-to-skills.py` so release-prep skill output matches the merged source docs. - Used the v0.0.65 announcement discussion as release context: #5472. ## Source Summary - #2492 -> `docs/about/release-notes.mdx`: Documents deadline-based gateway wait reliability in the v0.0.65 recovery summary. - #4958 -> `docs/about/release-notes.mdx`: Documents re-execed OpenClaw gateway health check recovery in the sandbox recovery summary. - #5163 -> `docs/about/release-notes.mdx`: Documents safer uninstall TTY confirmation behavior in the day-two CLI summary. - #5178 -> `docs/about/release-notes.mdx`: Documents fail-closed config restore merge behavior in the rebuild and restore summary. - #5179 -> `docs/about/release-notes.mdx`: Documents WeChat QR token redaction in the messaging summary. - #5182 -> `docs/about/release-notes.mdx`: Documents sustained gateway serving checks in the recovery summary. - #5194 -> `docs/about/release-notes.mdx`: Documents model-router teardown during uninstall in the day-two CLI summary. - #5195 -> `docs/about/release-notes.mdx`: Documents Shields auto-restore lock reconfirmation in the rebuild and restore summary. - #5198 -> `docs/about/release-notes.mdx`: Documents Docker Desktop WSL CDI injection failure handling in the onboarding diagnostics summary. - #5201 -> `docs/about/release-notes.mdx`: Documents sandbox download/upload wrappers and sessions export in the day-two CLI summary. - #5205 -> `docs/about/release-notes.mdx`: Documents reporter-owned model metadata preservation in the rebuild and restore summary. - #5214 -> `docs/about/release-notes.mdx`: Documents managed vLLM model preflight before side effects in the inference setup summary. - #5215 -> `docs/about/release-notes.mdx`: Documents managed vLLM extra serve arguments in the inference setup summary. - #5216 -> `docs/about/release-notes.mdx`: Documents silent OpenClaw runtime fallback surfacing in the onboarding diagnostics summary. - #5225 -> `docs/about/release-notes.mdx`: Documents persisted sandbox gateway lookup in the gateway recovery summary. - #5238 -> `docs/about/release-notes.mdx`: Documents sub-agent gateway dial-back through the sandbox interface in the Hermes and sub-agent summary. - #5248 -> `docs/about/release-notes.mdx`: Documents Discord per-account proxy resolution in the messaging summary. - #5264 -> `docs/about/release-notes.mdx`: Documents reserved Hermes port `8642` handling in the Hermes compatibility summary. - #5267 -> `docs/about/release-notes.mdx`: Documents the narrower Hermes baseline policy in the Hermes compatibility summary. - #5321 -> `docs/about/release-notes.mdx`: Documents restored gateway guard chains in the gateway recovery summary. - #5328 -> `docs/about/release-notes.mdx`: Documents compact persisted messaging plans in the messaging summary. - #5338 -> `docs/about/release-notes.mdx`: Documents manifest channel migration in the messaging summary. - #5352 -> `docs/about/release-notes.mdx`: Documents persisted agent preservation through registry recovery in the rebuild and restore summary. - #5371 -> `.agents/skills/nemoclaw-user-reference/references/commands.md`: Refreshes generated skill output for custom build cache and layer-ordering source docs. - #5379 -> `docs/about/release-notes.mdx`: Documents dashboard port allocation across multiple NemoClaw gateways in the recovery summary. - #5382 -> `docs/about/release-notes.mdx`: Documents recovery when an active gateway has no sandbox spec in the recovery summary. - #5389 -> `.agents/skills/nemoclaw-user-reference/references/troubleshooting.md`: Refreshes generated skill output for declared agent `forward_ports` recovery source docs. - #5400 -> `docs/about/release-notes.mdx`: Documents bounded compatible endpoint probes in the inference setup summary. - #5410 -> `docs/about/release-notes.mdx`: Documents provider credential hash removal from sandbox registry entries in the messaging summary. - #5418 -> `docs/about/release-notes.mdx`: Documents summarized inference validation failures in the onboarding diagnostics summary. - #5457 -> `docs/about/release-notes.mdx`: Documents context-window recomputation after runtime model switches in the inference setup summary. - #5463 -> `docs/about/release-notes.mdx`: Documents cleanup of hard-coded messaging channel stragglers in the messaging summary. ## Skipped - #5366 matched `docs/.docs-skip` entries through skipped experimental paths, so this PR does not add new release-note text for that commit. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [x] Doc only (includes code sample changes) ## Verification - [x] Git hooks passed during commit and push, or `npx prek run --from-ref main --to-ref HEAD` passes - [ ] Targeted tests pass for changed behavior - [ ] Full `npm test` passes (broad runtime changes only) - [ ] Tests added or updated for new or changed behavior - [x] No secrets, API keys, or credentials committed - [x] Docs updated for user-facing behavior changes - [ ] `npm run docs` builds without warnings (doc changes only) - [x] 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) Verification notes: - `npm run docs` passed after rerunning outside the sandbox. Fern reported 0 errors and 1 hidden warning. - The first sandboxed `npm run docs` attempt failed before validation because `tsx` could not create its local IPC pipe under sandbox restrictions. - `npm run build:cli` passed before push to refresh the local `dist/` artifacts used by the CLI typecheck hook. - `npm test` was not run because this is a docs-only release refresh. --- Signed-off-by: Miyoung Choi <miyoungc@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Released NemoClaw v0.0.65 with improved gateway/sandbox recovery, safer day-two workflows, and enhanced Hermes compatibility. * Added managed vLLM extra-arguments configuration via `NEMOCLAW_VLLM_EXTRA_ARGS_JSON`. * Added Hermes troubleshooting guidance for port forwarding and health checks. * **Documentation** * Updated NVIDIA Endpoints/NIM setup and examples to use `NVIDIA_INFERENCE_API_KEY`. * Refined NVIDIA network policy and Model Router API base configuration. * Expanded CLI/environment variable documentation (including sub-agent gateway connectivity) and plugin build performance tips. * **Tests** * Expanded Vitest-backed E2E release validation coverage. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Restore issue #5800 parity package `P0-B` for merged onboard/rebuild/lifecycle bash-suite deltas only. ## Related Issues Refs #5800 Refs #5098 Refs #5225 Refs #5487 Refs #5410 Refs #5760 ## Scope gate - Package: `P0-B — Onboard/rebuild/lifecycle parity` - Included PRs all merged and touched `test/e2e`: yes — #5225, #5487, #5410, #5760 - Out of scope: unmerged/non-bash PRs; shell lane retirement / PR #5756 cleanup ## Parity map | ID | Source PR | Contract | Inference classification | Vitest assertion / waiver | Status | | --- | --- | --- | --- | --- | --- | | B1 | #5225 | Persisted sandbox-entry gateway resolution is used by lifecycle commands. | `none` | Existing `src/lib/actions/sandbox/sandbox-gateway-routing.test.ts`, `src/lib/onboard/gateway-binding.test.ts`, `src/lib/onboard/sandbox-registration.test.ts` | covered | | B2 | #5225 | Onboard repair/double-onboard failures include captured onboard output diagnostics. | `none` | Existing live `test/e2e-scenario/live/onboard-repair.test.ts`, `test/e2e-scenario/live/double-onboard.test.ts`; diagnostics are bash-runner-only verbosity and not a durable Vitest assertion. | waived | | B3 | #5487 | Plain `nemoclaw onboard` auto-detects an `in_progress` session and resumes without `--resume`; `--fresh` suppresses auto-resume. | `hosted-compatible capable` | `test/e2e-scenario/live/onboard-resume.test.ts` Phase 3.5 mutates the completed session to `in_progress`, asserts `(resume mode)` + cached skips, then asserts `--fresh` fails at injected preflight without resume banner. | covered | | B4 | #5410 | Rebuild resumes messaging from `messaging.plan` rather than legacy `providerCredentialHashes`; stale top-level provider hash state is not used. | `none` | `test/e2e-scenario/live/rebuild-hermes.test.ts` curated registry omits `providerCredentialHashes`; `src/lib/onboard/machine/handlers/sandbox.test.ts` refreshes registry-plan credential hashes from env on rebuild resume. | covered | | B5 | #5410 | Empty/staged rebuild messaging plan is preserved and token-backed channels are not rediscovered. | `none` | Existing `src/lib/actions/sandbox/rebuild-messaging-stage.test.ts` and `src/lib/onboard/machine/handlers/sandbox.test.ts`. | covered | | B6 | #5760 | Hosted-inference/messaging rebuild and live answer assertions tolerate model whitespace around integer `42`. | `hosted-compatible capable` | Existing `test/helpers/e2e-answer-assertions.test.ts`, consumed by `agent-turn-latency`, `full-e2e`, `launchable-smoke`, and `sandbox-operations` live Vitests. | covered | | B7 | #5760 | Stabilized hosted inference and messaging rebuild remain validated by live full/sandbox/launchable/rebuild targets. | `hosted-compatible capable` | Existing live targets remain unchanged; local live execution blocked by Docker daemon unavailable. Selective workflow required after PR opens. | follow-up | ## Inference mode support - Default mode for touched live targets: `hosted-compatible capable` for onboard-resume/rebuild-hermes/full/sandbox/launchable answer paths; `none` for unit/process registry and gateway routing tests. - Real inference support preserved: yes for existing hosted-compatible live targets; no new inference adapter seam added here. - Modes validated in this PR: local unit/process Vitests only; live hosted-compatible validation needs GitHub runner/secrets because local Docker daemon is unavailable. - If not validated with real inference: local `NEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/onboard-resume.test.ts test/e2e-scenario/live/rebuild-hermes.test.ts` failed at prereq Docker daemon check before scenario assertions. ## Validation - [x] `git diff --check` - [x] `npm test -- src/lib/onboard/machine/handlers/sandbox.test.ts test/helpers/e2e-answer-assertions.test.ts src/lib/actions/sandbox/sandbox-gateway-routing.test.ts src/lib/onboard/entry-options.test.ts src/lib/onboard/sandbox-registration.test.ts src/lib/actions/sandbox/rebuild-messaging-stage.test.ts` - [x] `npm run typecheck:cli` - [x] `npm run build:cli` - [x] `npm run test-size:check` - [ ] hosted-compatible selective E2E workflow: pending PR / runner dispatch ## Follow-ups / waivers - Waiver B2: bash-only diagnostic verbosity from #5225 is not a durable Vitest contract; existing live tests already preserve the functional repair/double-onboard behavior. - Follow-up B7: dispatch selective live Vitest scenarios on GitHub runner with Docker + hosted inference secret after PR opens. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added coverage for sandbox resume flows to verify updated Telegram credentials are picked up when resuming a rebuild. * Expanded end-to-end onboarding resume scenarios to confirm implicit resume behavior, including skipped cached steps and fresh runs starting from the expected point. * Strengthened rebuild scenario checks to ensure curated registry entries no longer include legacy credential-hash data. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
## Summary Restore issue NVIDIA#5800 parity package `P0-B` for merged onboard/rebuild/lifecycle bash-suite deltas only. ## Related Issues Refs NVIDIA#5800 Refs NVIDIA#5098 Refs NVIDIA#5225 Refs NVIDIA#5487 Refs NVIDIA#5410 Refs NVIDIA#5760 ## Scope gate - Package: `P0-B — Onboard/rebuild/lifecycle parity` - Included PRs all merged and touched `test/e2e`: yes — NVIDIA#5225, NVIDIA#5487, NVIDIA#5410, NVIDIA#5760 - Out of scope: unmerged/non-bash PRs; shell lane retirement / PR NVIDIA#5756 cleanup ## Parity map | ID | Source PR | Contract | Inference classification | Vitest assertion / waiver | Status | | --- | --- | --- | --- | --- | --- | | B1 | NVIDIA#5225 | Persisted sandbox-entry gateway resolution is used by lifecycle commands. | `none` | Existing `src/lib/actions/sandbox/sandbox-gateway-routing.test.ts`, `src/lib/onboard/gateway-binding.test.ts`, `src/lib/onboard/sandbox-registration.test.ts` | covered | | B2 | NVIDIA#5225 | Onboard repair/double-onboard failures include captured onboard output diagnostics. | `none` | Existing live `test/e2e-scenario/live/onboard-repair.test.ts`, `test/e2e-scenario/live/double-onboard.test.ts`; diagnostics are bash-runner-only verbosity and not a durable Vitest assertion. | waived | | B3 | NVIDIA#5487 | Plain `nemoclaw onboard` auto-detects an `in_progress` session and resumes without `--resume`; `--fresh` suppresses auto-resume. | `hosted-compatible capable` | `test/e2e-scenario/live/onboard-resume.test.ts` Phase 3.5 mutates the completed session to `in_progress`, asserts `(resume mode)` + cached skips, then asserts `--fresh` fails at injected preflight without resume banner. | covered | | B4 | NVIDIA#5410 | Rebuild resumes messaging from `messaging.plan` rather than legacy `providerCredentialHashes`; stale top-level provider hash state is not used. | `none` | `test/e2e-scenario/live/rebuild-hermes.test.ts` curated registry omits `providerCredentialHashes`; `src/lib/onboard/machine/handlers/sandbox.test.ts` refreshes registry-plan credential hashes from env on rebuild resume. | covered | | B5 | NVIDIA#5410 | Empty/staged rebuild messaging plan is preserved and token-backed channels are not rediscovered. | `none` | Existing `src/lib/actions/sandbox/rebuild-messaging-stage.test.ts` and `src/lib/onboard/machine/handlers/sandbox.test.ts`. | covered | | B6 | NVIDIA#5760 | Hosted-inference/messaging rebuild and live answer assertions tolerate model whitespace around integer `42`. | `hosted-compatible capable` | Existing `test/helpers/e2e-answer-assertions.test.ts`, consumed by `agent-turn-latency`, `full-e2e`, `launchable-smoke`, and `sandbox-operations` live Vitests. | covered | | B7 | NVIDIA#5760 | Stabilized hosted inference and messaging rebuild remain validated by live full/sandbox/launchable/rebuild targets. | `hosted-compatible capable` | Existing live targets remain unchanged; local live execution blocked by Docker daemon unavailable. Selective workflow required after PR opens. | follow-up | ## Inference mode support - Default mode for touched live targets: `hosted-compatible capable` for onboard-resume/rebuild-hermes/full/sandbox/launchable answer paths; `none` for unit/process registry and gateway routing tests. - Real inference support preserved: yes for existing hosted-compatible live targets; no new inference adapter seam added here. - Modes validated in this PR: local unit/process Vitests only; live hosted-compatible validation needs GitHub runner/secrets because local Docker daemon is unavailable. - If not validated with real inference: local `NEMOCLAW_RUN_E2E_SCENARIOS=1 npx vitest run --project e2e-scenarios-live test/e2e-scenario/live/onboard-resume.test.ts test/e2e-scenario/live/rebuild-hermes.test.ts` failed at prereq Docker daemon check before scenario assertions. ## Validation - [x] `git diff --check` - [x] `npm test -- src/lib/onboard/machine/handlers/sandbox.test.ts test/helpers/e2e-answer-assertions.test.ts src/lib/actions/sandbox/sandbox-gateway-routing.test.ts src/lib/onboard/entry-options.test.ts src/lib/onboard/sandbox-registration.test.ts src/lib/actions/sandbox/rebuild-messaging-stage.test.ts` - [x] `npm run typecheck:cli` - [x] `npm run build:cli` - [x] `npm run test-size:check` - [ ] hosted-compatible selective E2E workflow: pending PR / runner dispatch ## Follow-ups / waivers - Waiver B2: bash-only diagnostic verbosity from NVIDIA#5225 is not a durable Vitest contract; existing live tests already preserve the functional repair/double-onboard behavior. - Follow-up B7: dispatch selective live Vitest scenarios on GitHub runner with Docker + hosted inference secret after PR opens. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Tests** * Added coverage for sandbox resume flows to verify updated Telegram credentials are picked up when resuming a rebuild. * Expanded end-to-end onboarding resume scenarios to confirm implicit resume behavior, including skipped cached steps and fresh runs starting from the expected point. * Strengthened rebuild scenario checks to ensure curated registry entries no longer include legacy credential-hash data. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Summary
Removes the stale top-level SandboxEntry credential hash map that was reintroduced after the messaging credential binding migration. Sandbox registration and rebuild now rely on
messaging.plan.credentialBindings[].credentialHashinstead of duplicating that state.Related Issue
Related: #4908
Changes
SandboxEntryand registry registration.test/onboard-messaging.test.tsfile-size budget after shrinking the test file.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Focused checks run after merging current
origin/main:npm run build:clipassesnpm run typecheck:clipassesnpm run test-size:checkpassesgit diff --checkpassesrg -n "providerCredentialHashes" --hidden --glob '!node_modules' .returns no matchesnpx vitest run test/registry.test.ts src/lib/onboard/sandbox-registration.test.ts test/onboard-messaging.test.ts test/credential-rotation.test.tspassesLocal full hook notes:
npx prek run --all-files, the commit hook, and the pre-push hook were attempted before the merge update but were blocked by broad CLI-suite environment/timeouts unrelated to this change.Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit