fix(onboard): continue onboarding when Homebrew refuses the pinned OpenShell tap - #7739
fix(onboard): continue onboarding when Homebrew refuses the pinned OpenShell tap#7739harjothkhara wants to merge 31 commits into
Conversation
…enShell formula Homebrew 6.x refuses to load formulae from taps it has not marked trusted, so brew info fails for the pinned nvidia/openshell tap and the formula identity check aborted onboarding before preflight. Treat an unconfirmed identity as no managed Homebrew service: warn once with brew's own reason and continue on the standalone gateway. Positive evidence of a missing or wrong-tap formula still fails closed. Refs: NVIDIA#7707 Signed-off-by: harjoth <harjoth.khara@gmail.com>
…ebrew formula identity Refs: NVIDIA#7707 Signed-off-by: harjoth <harjoth.khara@gmail.com>
Cross-review found the fallback too broad: any brew info failure skipped the identity gate, and the standalone path can still execute a brew-installed binary. Only the refusal that names the pinned nvidia/openshell formula and tap now degrades to the standalone fallback; every other failure keeps the fail-closed abort. Refs: NVIDIA#7707 Signed-off-by: harjoth <harjoth.khara@gmail.com>
…arning and docs Refs: NVIDIA#7707 Signed-off-by: harjoth <harjoth.khara@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughHomebrew refusal handling now checks the pinned OpenShell formula refusal and the ChangesHomebrew fallback handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Onboarding
participant Homebrew
participant Launchd
Onboarding->>Homebrew: Check pinned OpenShell formula
Homebrew-->>Onboarding: Return untrusted-tap refusal
Onboarding->>Launchd: Probe homebrew.mxcl.openshell
Launchd-->>Onboarding: Return service state
Onboarding-->>Onboarding: Continue with standalone fallback or stop
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
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.
2 additional E2E selections from the second opinionAdvisory only. The primary lane did not select these E2E jobs or targets.
Second-opinion terminology and E2E selections are advisory. They do not change the primary assessment or E2E / PR Gate. 3 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. E2E / PR Gate selects and runs jobs independently. Recommended E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
ci/platform-matrix.json (1)
42-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep generated platform notes to one sentence per source line.
The updated matrix note renders as multi-sentence Markdown table rows, which violates the documentation source rule.
ci/platform-matrix.json#L42-L42: rewrite the macOS note as one sentence while retaining the fallback details.docs/get-started/prerequisites.mdx#L99-L99: regenerate after correcting the matrix note.docs/reference/platform-support.mdx#L84-L84: regenerate after correcting the matrix note.As per coding guidelines, “Keep one sentence per line in Markdown and MDX source files.”
🤖 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 `@ci/platform-matrix.json` at line 42, Rewrite the macOS note in ci/platform-matrix.json at line 42 as a single sentence while preserving all existing runtime, Homebrew, fallback, Colima, Docker CLI, and Xcode details; then regenerate docs/get-started/prerequisites.mdx at line 99 and docs/reference/platform-support.mdx at line 84 so their generated notes also remain one sentence per source line.Source: Coding guidelines
🤖 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/onboard/docker-driver-gateway-service.test.ts`:
- Around line 197-218: Update the test for the unconfirmed formula identity in
startPackageManagedDockerDriverGateway to inject a throwing
startOpenShellGatewayUserService mock, then assert it was not called while
retaining the existing started === false assertion.
---
Nitpick comments:
In `@ci/platform-matrix.json`:
- Line 42: Rewrite the macOS note in ci/platform-matrix.json at line 42 as a
single sentence while preserving all existing runtime, Homebrew, fallback,
Colima, Docker CLI, and Xcode details; then regenerate
docs/get-started/prerequisites.mdx at line 99 and
docs/reference/platform-support.mdx at line 84 so their generated notes also
remain one sentence per source line.
🪄 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: dd6e495c-1f77-4376-be7a-855cc538c54a
📒 Files selected for processing (8)
ci/platform-matrix.jsondocs/get-started/prerequisites.mdxdocs/reference/architecture.mdxdocs/reference/platform-support.mdxdocs/reference/troubleshooting.mdxsrc/lib/onboard/docker-driver-gateway-service.test.tssrc/lib/onboard/docker-driver-gateway-service.tssrc/lib/onboard/gateway-host-runtime-homebrew-untrusted-tap.test.ts
…-refusal fallback Review found the fallback made the standalone cutover path reachable while launchd still owned a loaded openshell service: cutover could adopt that process or kill one launchd would restart. Probe launchctl (which does not load the formula) before degrading and abort with stop-the-service guidance while the unit is loaded. Also bound the refusal match so taps that only start with the pinned name stay fatal, and assert the managed start is never invoked in the fallback test. Refs: NVIDIA#7707 Signed-off-by: harjoth <harjoth.khara@gmail.com>
|
@coderabbitai Declining the one-sentence-per-line nitpick on ci/platform-matrix.json:42. That guideline covers Markdown and MDX sources; this note is a JSON string field. The cell was already five sentences before this PR (container runtime, Homebrew, Colima, Docker CLI, Xcode notes) — this change only extends the existing Homebrew sentence. Rewriting the shared row as one sentence is out of scope here. |
|
Tip For best results, initiate chat on the files or code changes.
🐇 |
Review found two fail-open edges in the tap-refusal fallback. The launchd probe treated a launchctl that could not run as proof the service was stopped; only a completed run that reports the unit missing now permits the fallback. The refusal matcher compared a prefix, so a neighbouring tap such as nvidia/openshell.fork still matched; it now compares the named formula and tap exactly. Refs: NVIDIA#7707 Signed-off-by: harjoth <harjoth.khara@gmail.com>
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 `@src/lib/onboard/docker-driver-gateway-service.ts`:
- Around line 286-287: Update the launchctl result classification near the
status handling so `not-loaded` is returned only for the recognized missing-unit
status/stderr combination; classify other nonzero exits, including unrecognized
stderr, as `unknown` while preserving successful status handling. Add a
regression test covering an indeterminate nonzero launchctl failure and verify
the fallback path remains blocked.
🪄 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: d2a924bc-f7bf-4873-b04c-3d60dfa65614
📒 Files selected for processing (7)
ci/platform-matrix.jsondocs/get-started/prerequisites.mdxdocs/reference/architecture.mdxdocs/reference/platform-support.mdxdocs/reference/troubleshooting.mdxsrc/lib/onboard/docker-driver-gateway-service.test.tssrc/lib/onboard/docker-driver-gateway-service.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- docs/reference/troubleshooting.mdx
- docs/reference/architecture.mdx
- ci/platform-matrix.json
- src/lib/onboard/docker-driver-gateway-service.test.ts
…nit state Review found the probe still read every completed nonzero exit as proof the unit was absent. launchctl reports a missing service as exit 113 with "Could not find service"; only those signals now mean not-loaded, and any other failure stays unknown and keeps the abort. Refs: NVIDIA#7707 Signed-off-by: harjoth <harjoth.khara@gmail.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
wscurran
added
area: onboarding
|
✨ Thanks for the PR. This fixes onboarding to continue with a standalone gateway fallback when Homebrew 6.x refuses the pinned nvidia/openshell tap. Maintainers will review the fallback logic, warning behavior, and that other identity-check failures still fail closed. Related open issues: Related open issues: |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
Signed-off-by: Carlos Villela <cvillela@nvidia.com>
|
@coderabbitai review |
✅ Action performedReview finished.
|
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 `@docs/reference/architecture.mdx`:
- Around line 96-100: The Homebrew formula detection flow must fail closed for
unrecognized brew list failures. In hasOfficialHomebrewFormula, return false
only when allowStandaloneForPinnedTapLoadRefusal accepts the failed probe;
otherwise throw the formula identity error, and add a test covering an altered
brew list diagnostic. Apply this guidance to docs/reference/architecture.mdx
lines 96-100 and docs/reference/troubleshooting.mdx lines 936-940; both sites
require the same behavior update.
🪄 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: 7337ea00-a619-40fa-9023-29ff39e3d769
📒 Files selected for processing (5)
ci/platform-matrix.jsondocs/get-started/prerequisites.mdxdocs/reference/architecture.mdxdocs/reference/platform-support.mdxdocs/reference/troubleshooting.mdx
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/get-started/prerequisites.mdx
- docs/reference/platform-support.mdx
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
apurvvkumaria
left a comment
apurvvkumaria
left a comment
There was a problem hiding this comment.
Security review PASS for revision 5307aa8. Categories reviewed: secrets, input validation, authorization and lifecycle authority, dependencies, logging, cryptography, configuration, tests, and failure modes. The official Apple Silicon missing-keg diagnostic and complete pinned-tap refusal retain their bounded fallback paths; every other list failure stops with a redacted error before launchd inspection or service mutation. Focused suites pass 89/89, and the CLI build, type check, repository checks, and hooks pass. No findings remain.
|
Merge-train status: this branch revision merges cleanly with current The ordinary protected fork workflow was approved, but the branch's PR workflow predates the controller's current run-identity contract. The base-branch coordinator therefore skipped the completed run and cannot produce the required E2E risk plan. A contributor-authorized branch synchronization is required before normal CI and protected E2E can complete. No gate exception or bypass is requested. |
|
Synced the branch onto current The focused onboarding suites still pass 89/89 after the merge, and I refreshed the documentation writer review metadata for the new head. One thing that still needs a maintainer: the new head has not been vetted, so the |
Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
<!-- markdownlint-disable MD041 --> ## Summary Recent main changes left managed-image validation and platform-watch fixtures out of sync with their production contracts. This change restores those gates without weakening package identity, provenance, or forward ownership checks. ## Changes - Align the Hermes managed-image capability checks with the reviewed `aiohttp==3.14.3` update from #8203. - Validate neutral OpenClaw plugin packages from the installed project directories because OpenClaw 2026.7.1 does not persist `plugins.installs` metadata. - Validate Google Chat with the rest of the installed OpenClaw capability union. - Match the BuildKit SLSA base-dependency URI emitted for digest-pinned images while retaining the separate digest check. - Mark the Ubuntu container checkout as a Git safe directory and verify `HEAD` before generating build identity. - Make the VM-driver snapshot fixture report the all-interface forward binding required on WSL. - Leave the macOS Homebrew failure to existing PR #7739, which has the focused product fix. ## Type of Change - [x] Code change (feature, bug fix, or refactor) - [ ] Code change with doc updates - [ ] Doc only (prose changes, no code sample modifications) - [ ] Doc only (includes code sample changes) ## Quality Gates - [x] Tests added or updated for changed behavior - [ ] Existing tests cover changed behavior — justification: - [ ] Tests not applicable — justification: - [ ] Docs updated for user-facing behavior changes - [x] Docs not applicable — justification: This repair changes CI validation and platform test fixtures. It does not change a supported user command, configuration, workflow, or default. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [ ] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Pending review of managed-image provenance and package-identity validation. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `blocked` - Evidence: No documentation paths changed. This host has no independent documentation-writer subagent, so the required final review is pending. - Agent: Pi coding agent <!-- docs-review-head-sha: 6cd1da3 --> <!-- docs-review-agents-blob-sha: 3dd7c24 --> ## DGX Station Hardware Evidence - [ ] Tested on DGX Station - Tested commit: - Station profile/scenario: - Result: - Supporting evidence: ## Verification - [x] PR description includes a `Signed-off-by:` line and every commit appears as `Verified` in GitHub - [x] Normal `pre-commit`, `commit-msg`, and `pre-push` hooks passed, or `npm run validate:pr` passed after refreshing `origin/main` when hooks were skipped or unavailable - [x] Targeted behavior tests pass for the current change set, or tests are marked not applicable above — 6 integration files / 60 tests passed; WSL-bound snapshot fixture passed 5/5; review follow-ups passed 33 and 15 focused tests; repository checks, ShellCheck, and normal hooks passed; the repaired verifier accepted the failed main Deep Agents attestation. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: GitHub CI will run the broad gate; the redundant local broad run was stopped after focused validation passed. - [x] Quality Gates section completed with required justifications or waivers - [x] No secrets, API keys, or credentials committed - [ ] `npm run docs` builds without warnings (doc changes only) - [ ] 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) --- Signed-off-by: Carlos Villela <cvillela@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved managed image validation for Google Chat and plugin installation consistency. - Updated Hermes image checks to use the latest approved `aiohttp` version. - Improved dashboard traffic forwarding behavior in WSL environments. - Corrected Docker dependency evidence generation for platform-specific image references. - **Reliability** - Added safeguards to ensure builds use the intended source revision. - Expanded automated checks for image publication, plugin configuration, and workflow integrity. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
jyaunches
left a comment
jyaunches
left a comment
There was a problem hiding this comment.
Requesting changes because this PR changes lifecycle authority instead of completing NemoClaw's existing Homebrew trust contract.
PR #7555 deliberately trusts only the checksum-verified nvidia/openshell/openshell formula and removes that trust after installation. At head 181e1bffd, src/lib/onboard/docker-driver-gateway-service.ts:516 treats the expected post-install Homebrew 6 refusal as an unavailable package-managed service. It then selects the detached standalone gateway when the launchd unit is absent. The installer can therefore stage a Homebrew-managed service that onboarding immediately abandons.
Please preserve the supported Homebrew path:
- Extract pinned-checksum verification and formula-scoped trust/untrust into one reusable Homebrew operation boundary.
- Use that boundary during installation and subsequent formula inspection plus service start and stop operations.
- For a legacy installation that cannot satisfy pinned verification, stop with an actionable repair instruction or rerun the pinned installer.
- Use the standalone fallback only when Homebrew or the OpenShell formula is genuinely absent.
- Do not use one exact Homebrew stderr sentence as the durable compatibility contract.
- Do not persist formula trust, trust the complete tap, or set
HOMEBREW_NO_REQUIRE_TAP_TRUST. - Fail closed when formula trust cannot be granted or removed.
This design keeps trust scoped to the verified formula and the named operation. It also keeps Homebrew as lifecycle authority for a Homebrew installation.
Please add behavior tests for inspection, start, stop, repair guidance, missing Homebrew, missing formula, trust refusal, and untrust cleanup failure. Update the changed documentation to describe the resulting repair and fallback boundaries.
|
Agreed. If the installer removes formula trust after install, that refusal is the expected state, not a missing service — keying the fallback off it reads the condition backwards. Two questions before I start:
Happy to do the work once the direction is set. |
|
Refreshed onto current The previous red CI was stale base only. Validation on the new head: focused onboarding suites 89/89, plugin credential-filter 8/8, and @jyaunches the design question in your review is untouched by this refresh and still open. Before I rebuild, I want to make sure I build the right thing: should the reusable Homebrew operation boundary wrap each pinned-verification-plus-trust-plus-untrust cycle per operation, or hold trust across a single onboarding run and untrust once at the end? The first is stricter and what I read you as asking for, but it re-verifies on every inspection, start, and stop. |
9 participants
Summary
On macOS with Homebrew 6.x,
nemoclaw onboardaborted before[1/8] Preflightbecause the OpenShell formula identity check threw whenbrew listorbrew infofailed. Homebrew 6.x refuses to load formulae from taps it has not marked trusted, including the pinnednvidia/openshelltap. Onboarding now treats that specific refusal as an unconfirmed identity: it warns once with Homebrew's own reason and continues on the standalone gateway fallback. Every other identity-check failure still fails closed.Related Issue
Fixes #7707
Changes
src/lib/onboard/docker-driver-gateway-service.ts: whenbrew list --formula openshellorbrew info --json=v2 openshellreturns the exact refusal naming the pinnednvidia/openshell/openshellformula and tap, probelaunchctl print gui/<uid>/homebrew.mxcl.openshellwithout loading the formula. Only the exact missing-unit result permits the standalone fallback and one deduplicated warning. A loaded unit or an unknown probe result throwsOpenShellGatewayServiceTrustError, so the operational gateway fallback on currentmaincannot bypass lifecycle authority. Unrecognized identity failures remain fail-closed, while the separate missing-formula fallback on currentmainremains intact. The adjacent source comment records that this compatibility branch should be removed when the minimum supported Homebrew can inspect the pinned formula without trusting or loading the tap.brew listandbrew inforefusals; generic, foreign-tap, and pinned-name-prefix failures; loaded, denied, status-less, unrunnable, and unrecognized launchd probes; exact missing-unit fallback; warn-once behavior; and proof that the package-managed starter is not invoked.docs/reference/troubleshooting.mdx,docs/reference/architecture.mdx, andci/platform-matrix.json(regeneratingdocs/reference/platform-support.mdxanddocs/get-started/prerequisites.mdx) no longer claim onboarding never uses the standalone fallback while Homebrew is present.Known limits, stated plainly:
packaged-serviceowner fails with the existing explicit "authority changed, requires a fresh onboarding run" error rather than migrating. That error is the existing fail-closed contract, not new behavior.brewboundary; not verified against a live Homebrew 6.x host. No onboarding-process-level test asserts[1/8] Preflightis reached; the real-path test stops at gateway-owner resolution.Type of Change
Quality Gates
brew listfailures now stop with a redacted identity error before launchd inspection or service mutation. No findings remain.Documentation Writer Review
docs-updateddocs/get-started/prerequisites.mdx,docs/reference/architecture.mdx,docs/reference/platform-support.mdx, anddocs/reference/troubleshooting.mdxaccurately define the recognized Homebrew refusal, the required missing launchd-unit proof, the fail-closed handling of every other identity query failure, and launchd authority for a loaded service. The follow-up aligns implementation with that documented trust boundary, so no further documentation changes are needed. Focused suites pass 89/89; the CLI build, type check, repository checks, changed-file hooks, and commit hooks pass. Re-confirmed at merge commit76cb83c5e, which merges currentmain(17 commits) with no conflicts and changes no source of this PR.docs/AGENTS.mdis unchanged across the merge (ac44d5a5fat both the previously reviewed head and this head), so the authoring rules the earlier review applied still hold.docs/reference/troubleshooting.mdxwas also changed onmain; the auto-merged result was inspected and this PR's added lines remain correctly placed and one sentence per line. Focused onboarding suites re-run after the merge and pass 89/89. Re-confirmed again at merge commit47218c5e6, which merges currentmain(86 commits) with no conflicts. The effective PR diff is the same nine files.mainreorganized the documentation contract: rootAGENTS.mdmoved DORI routing intodocs/AGENTS.mdand moved the receipt procedure intoCONTRIBUTING.md#documentation-writer-review-receipt, so the hidden blob SHA changes from3dd7c2425toc69aad4d5without changing any rule this PR depends on.docs/reference/troubleshooting.mdxchanged onmainagain; the auto-merged result was inspected and this PR's five added lines remain correctly placed after the standalone-fallback sentence, one sentence per line.76cb83c5e)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 unavailable47218c5e6,npx vitest run --project cli src/lib/onboard/docker-driver-gateway-service.test.ts src/lib/onboard/gateway-host-runtime-homebrew-untrusted-tap.test.tspassed 2 files and 89 tests, andnpx vitest run --project plugin nemoclaw/src/security/credential-filter.test.tspassed 1 file and 8 tests.npm run validate:prpassed in full, includingTypeScript (CLI),TypeScript (plugin), repository checks, and the source-shape and test-file budgets. The priorbuild-typecheckfailure was stale-base only:test/package-contract/cli/policy-prompt-eof.test.tsfailed on the old base withTypeError: (0 , channels_2.listChannels) is not a function, touches nothing this PR changes, and passes 2/2 on the refreshed head.npm run docsbuilds with no new warnings (doc changes only) — 0 errors; the only 2 warnings are pre-existing and identical on pristinemainSigned-off-by: harjoth harjoth.khara@gmail.com
Summary by CodeRabbit