feat(cli): enrich dashboard-url with connection guidance - #7478
feat(cli): enrich dashboard-url with connection guidance#7478laitingsheng wants to merge 17 commits into
Conversation
Signed-off-by: Tinson Lai <tinsonl@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 (3)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesDashboard guidance flow
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant DashboardUrlCliCommand
participant runDashboardUrlCommand
participant getAgentBranding
participant Console
DashboardUrlCliCommand->>runDashboardUrlCommand: run dashboard URL command
runDashboardUrlCommand->>getAgentBranding: resolve agent-specific commands
getAgentBranding-->>runDashboardUrlCommand: branding and guidance commands
runDashboardUrlCommand->>Console: emit URL, warning, SSH hint, and guidance
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 |
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit 21af1a6 in the TypeScript / code-coverage/cliThe overall coverage in commit 21af1a6 in the Show a code coverage summary of the most impacted files.
Updated |
|
🌿 Preview your docs: https://nvidia-preview-pr-7478.docs.buildwithfern.com/nemoclaw |
PR Review Advisor — Blocking findings reportedAdvisor assessment: Blockers require maintainer review Model lanes
Nemotron output stays in workflow artifacts and does not change the assessment above. E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: None 2 optional E2E recommendations
Blockers
|
laitingsheng
added
NV QA
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/lib/connection-info-command.ts (1)
45-49: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDo not silently relabel dependency failures as missing sandbox metadata.
These catch-all fallbacks turn registry/agent-loading failures into either “sandbox does not exist” or an OpenClaw-style dashboard path, hiding the actionable error.
src/lib/connection-info-command.ts#L45-L49: let unexpectedloadAgentfailures propagate; its declarednullresult already represents an unknown agent.src/lib/connection-info-command.ts#L74-L79: do not convert a registry failure into a nonexistent sandbox.src/commands/sandbox/connection/info.ts#L44-L50: remove the duplicate registry-error fallback.Based on learnings, avoid defensive catch-and-fallback handling around internal helpers without an actionable recovery path.
🤖 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 `@src/lib/connection-info-command.ts` around lines 45 - 49, Remove catch-all registry-error fallbacks around the connection-info flow: in src/lib/connection-info-command.ts lines 45-49, let unexpected loadAgent failures propagate while preserving its null result for unknown agents; in lines 74-79, stop converting registry failures into nonexistent-sandbox results; and in src/commands/sandbox/connection/info.ts lines 44-50, remove the duplicate registry-error fallback.Source: Learnings
🤖 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 `@src/lib/connection-info-command.test.ts`:
- Around line 14-15: Update the parent suite title in
src/lib/connection-info-command.test.ts at lines 14-15 and
src/lib/cli/connection-info-route.test.ts at lines 9-10 to include the final
issue suffix (`#7473`). In test/package-contract/cli/command-registry.test.ts at
lines 59-60 and 229-231, replace the count-only test titles with
behavior-oriented descriptions of connection-info registration and the
connection action token, respectively, and append (`#7473`) as the final suffix.
In `@src/lib/connection-info-command.ts`:
- Around line 90-92: Update the terminal-agent branch in the connection-info
command so it includes the shared management-command section before returning,
while preserving the existing terminal connection instructions. Reuse the
existing management-section helper or rendering path rather than duplicating its
content.
---
Nitpick comments:
In `@src/lib/connection-info-command.ts`:
- Around line 45-49: Remove catch-all registry-error fallbacks around the
connection-info flow: in src/lib/connection-info-command.ts lines 45-49, let
unexpected loadAgent failures propagate while preserving its null result for
unknown agents; in lines 74-79, stop converting registry failures into
nonexistent-sandbox results; and in src/commands/sandbox/connection/info.ts
lines 44-50, remove the duplicate registry-error fallback.
🪄 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: 34bb96c9-4e7b-42cd-a139-bec0a3376288
📒 Files selected for processing (8)
docs/reference/commands.mdxsrc/commands/sandbox/connection/info.tssrc/lib/cli/connection-info-route.test.tssrc/lib/cli/public-display-defaults.tssrc/lib/connection-info-command.test.tssrc/lib/connection-info-command.tssrc/lib/onboard.tstest/package-contract/cli/command-registry.test.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
cv
left a comment
cv
left a comment
There was a problem hiding this comment.
Product scope is not yet established for this new public CLI surface. Linked #7473 is still open with needs: triage and has no accepted maintainer decision defining ownership, lifecycle, compatibility, security, and validation; Project/assignee state alone does not satisfy the repository scope gate. Please obtain that decision before this can be approved or documented as canonical behavior. There is also a concrete docs mismatch: the implementation calls dashboardUrlForDisplay() and strips the #token= fragment before stdout, while docs/reference/commands.mdx:1516-1529 claims the reprinted block contains an authenticated URL and must be treated as a password. Correct the docs to describe the tokenless URL and point to dashboard-url --quiet for the authenticated form; add an entrypoint/output-contract regression proving normal output contains no #token=. After scope is accepted, refresh the stale/conflicted branch and rerun exact docs/security review plus CI/E2E.
|
Moving this PR out of v0.0.96, not closing it. The implementation is narrow and salvageable, and the displayed URL is currently tokenless, but linked #7473 remains needs:triage with no accepted product decision for the new public CLI surface. The docs also incorrectly call the tokenless displayed URL authenticated/password-equivalent, and no real output-contract test proves the token fragment stays absent. After an explicit scope decision, refresh onto current main, correct the docs and metadata fallback contract, add the no-#token entrypoint test, and rerun exact docs/security/CI/E2E. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/reference/commands.mdx (1)
1524-1544: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the stopped-sandbox fallback precise.
statusdoes not reprint the dashboard URL or terminal connection and management commands. Document it as the diagnostic next step, then tell users to start the sandbox and rerunconnection inforather than implying thatstatussupplies the missing connection details.🤖 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 `@docs/reference/commands.mdx` around lines 1524 - 1544, Update the stopped-sandbox guidance in the connection info documentation: describe $$nemoclaw <name> status only as the diagnostic next step, then instruct users to start the sandbox and rerun connection info to retrieve the dashboard URL or connection and management commands.
🤖 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.
Outside diff comments:
In `@docs/reference/commands.mdx`:
- Around line 1524-1544: Update the stopped-sandbox guidance in the connection
info documentation: describe $$nemoclaw <name> status only as the diagnostic
next step, then instruct users to start the sandbox and rerun connection info to
retrieve the dashboard URL or connection and management commands.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 62f8b747-afc3-496a-b5e7-6d17d0223905
📒 Files selected for processing (3)
docs/reference/commands.mdxsrc/lib/cli/public-display-defaults.tssrc/lib/onboard/dashboard.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/lib/cli/public-display-defaults.ts
- src/lib/onboard/dashboard.ts
|
Thanks for working on this. |
Signed-off-by: San Dang <sdang@nvidia.com>
Signed-off-by: San Dang <sdang@nvidia.com>
…to feat/connection-info-command
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Maintainer security review: PASS at exact head f56df0ad76e50938f9a36388fe816f87cb4d7827 against current main.
- Secrets and credentials — PASS: no new secret source or persistence; the existing OpenClaw token URL remains followed immediately by its password-equivalent warning.
- Input validation — PASS: the change adds display-only guidance and no new parsing, execution, or trust boundary.
- Authentication and authorization — PASS: token-auth and session-auth construction are unchanged; Hermes remains tokenless.
- Dependencies — PASS: none added or changed.
- Logging and exposure — PASS: added guidance contains no token data;
--quietremains URL-only by contract. - Cryptography — PASS: no cryptographic behavior changed.
- Configuration — PASS: no configuration or policy surface changed.
- Tests — PASS: OpenClaw token URL/warning, Hermes session URL, quiet output, and runtime isolation are covered; focused Vitest passed 28/28, CLI type-check passed, and docs build completed with 0 errors.
- System security — PASS: no sandbox, process, filesystem, or network boundary changed.
No security findings or waiver are required.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Maintainer security review: PASS at exact head d1e8240f0b06b0498946531057482d2b1f66e15b against current main.
- Secrets and credentials — PASS: no new secret source or persistence; the existing OpenClaw token URL remains followed immediately by its password-equivalent warning.
- Input validation — PASS: the change adds display-only guidance and no new parsing, execution, or trust boundary. The CLI name comes from Oclif configuration and is only interpolated into stdout.
- Authentication and authorization — PASS: token-auth and session-auth construction are unchanged; Hermes remains tokenless.
- Dependencies — PASS: none added or changed.
- Logging and exposure — PASS: added guidance contains no token data;
--quietremains URL-only by contract. - Cryptography — PASS: no cryptographic behavior changed.
- Configuration — PASS: no configuration or policy surface changed.
- Tests — PASS: OpenClaw token URL/warning, Hermes session URL, invoked binary, quiet output, and runtime isolation are covered; focused tests passed 28/28, CLI type-check passed, and repository architecture checks passed.
- System security — PASS: no sandbox, process, filesystem, or network boundary changed.
The exact-head follow-up removes a branding import that exceeded the fan-in ratchet and instead passes the actual invoked binary through the CLI adapter. This preserves user-visible behavior and architecture limits without a waiver. Docs build completed with 0 errors. No security findings remain.
Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com>
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Maintainer exact-head security review: PASS at 5d9a241 against current main.
This head adds only the focused CLI-wrapper regression requested by PR Review Advisor; production code and documentation are unchanged from the prior reviewed head. The test proves the non-default invoked binary reaches Terminal, Status, Logs, Model, Policies, and Credentials guidance.
- Secrets and credentials — PASS: no production or secret-handling change.
- Input validation — PASS: no production parsing or trust-boundary change.
- Authentication and authorization — PASS: URL authentication behavior is unchanged.
- Dependencies — PASS: none changed.
- Logging and exposure — PASS: test output is synthetic and contains no real credential.
- Cryptography — PASS: unchanged.
- Configuration — PASS: test-only Oclif configuration is isolated to the test instance.
- Tests — PASS: the new wrapper regression passes 2/2; CLI type-check, Biome, repository hooks, and pre-push CLI type-check pass.
- System security — PASS: no sandbox, process, filesystem, or network behavior changed.
The exact-head documentation writer receipt also passed. No correctness or security findings remain, and PRA-1 is addressed by this commit.
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Maintainer exact-head security review: PASS at df9fc8d against current main.\n\nThis head is a signed mechanical merge of current main. The PR product diff remains the previously reviewed dashboard URL guidance plus its alternate-binary regression; no authentication, token construction, persistence, dependency, policy, network, filesystem, or process behavior changed.\n\n1. Secrets and credentials — PASS: authenticated URL construction and warning placement are unchanged.\n2. Input validation — PASS: no new production parsing or trust boundary.\n3. Authentication and authorization — PASS: unchanged.\n4. Dependencies — PASS: the PR adds none.\n5. Logging and exposure — PASS: guidance contains no token data and quiet output remains URL-only.\n6. Cryptography — PASS: unchanged.\n7. Configuration — PASS: invoked binary is display-only Oclif configuration.\n8. Tests — PASS: focused dashboard and wrapper tests pass 17/17; CLI type-check and pre-push checks pass.\n9. System security — PASS: no sandbox, process, filesystem, or network boundary changed.\n\nThe exact-head documentation writer receipt passed. No correctness or security findings remain.
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Approved at exact head df9fc8d after product-scope rework to the existing dashboard-url command. All 47 current checks are green; mergeability, DCO/GitHub verification, documentation-writer receipt, nine-category security review, focused tests, invoked-binary regression, and no-E2E-selected gate all pass. The former new-command scope and tokenless-documentation concerns are no longer present.
prekshivyas
left a comment
prekshivyas
left a comment
There was a problem hiding this comment.
Exact-head technical review is complete at df9fc8d: all 47 checks, docs/security receipts, tests, mergeability, DCO, and verification pass, and the previous documentation mismatch is resolved. However, the repository product-scope gate still blocks approval. Issue #7473 remains open with NV QA/VDR labels, an assignee, and In Progress project state, but no accepted maintainer decision defining this supported public CLI behavior and its ownership/lifecycle/compatibility/validation expectations. Please record that decision or maintainer direction on #7473. No code change is currently requested.
|
Babysitting status for exact head df9fc8d (plain comment; no Changes Requested review from me): the existing product-scope decision remains unresolved, and the branch is based on 125ca30 while current main is da1b103. The current checks are green, but maintainer edits are disabled, so I cannot produce current-base evidence. I will re-gate the next quiet author revision after scope is accepted and the branch is refreshed. |
|
Correction to my prior handoff: conflict-free base refreshes are explicitly waived. Please do not merge main solely for base currency; preserving exact-head evidence is preferred unless GitHub reports a real conflict or reviewed behavior requires a change. The substantive blocker or missing evidence described in the earlier handoff remains, but base age by itself is not a blocker. This is a plain coordination comment, not Changes Requested. |
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
|
Babysitting recheck at exact head |
|
Closing without merge following the accepted maintainer disposition on #7473. The implementation is not being rejected for a remaining code defect. The product decision is to keep |
6 participants
Summary
Enrich the existing
nemoclaw <name> dashboard-urloutput with the connection and management guidance users need after onboarding. This avoids adding a new public CLI command while preserving URL-only--quietoutput for scripts.Related Issue
Fixes #7473
Changes
Manage latercommands after the default dashboard URL output for OpenClaw and other dashboard agents.--quietand-qas URL-only stdout, and preserve the existing no-dashboard result for terminal-runtime agents.printSandboxGuidancehelper because token-auth and session-auth branches share this output. Helper and CLI-entrypoint tests protect the shared behavior.Type of Change
Quality Gates
df9fc8dc2. Authenticated URL construction is unchanged, the warning follows the URL, the actual invoked CLI name is display-only, and the added guidance contains no token data.Documentation Writer Review
docs-updateddocs/reference/commands.mdxremains accurate for URL-plus-guidance output, quiet URL-only behavior, OpenClaw warnings, Hermes session URLs, and binary-specific commands. The mechanical current-main merge adds no PR-specific documentation change; focused dashboard tests passed 17/17 and CLI type-check passed.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run check:diffpassed when hooks were skipped or unavailablenpm run typecheck:cli, Biome, repository architecture checks, and Vitest project membership passed.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — command/result:npm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Signed-off-by: San Dang sdang@nvidia.com
Summary by CodeRabbit
Documentation
User Experience