fix(installer): classify an unreachable systemd user manager before the upstream gateway gate - #8945
Closed
Dongni-Yang wants to merge 3 commits into
Closed
fix(installer): classify an unreachable systemd user manager before the upstream gateway gate#8945Dongni-Yang wants to merge 3 commits into
Dongni-Yang wants to merge 3 commits into
Conversation
…he upstream gateway gate A packaged static openshell-gateway.service unit file was treated as proof that the upstream user service is inspectable. Without a systemd user session, systemctl --user fails with "Failed to connect to bus: No medium found"; the installer discarded that stderr and exited with the unrelated "Could not locate the gateway binary" error. Classify only known user-manager-unavailable diagnostics (same pattern as userManagerLooksUnavailable on the TypeScript side) as eligible for the NemoClaw-managed fallback, and reject the fallback when an enablement link in any persistent user unit directory could let the packaged unit activate later and compete for gateway port 8080. Every other service-query failure keeps the existing fail-closed error, and the reachable-manager identity/version gate is unchanged. Closes #8926 Signed-off-by: Dongni Yang <dongniy@nvidia.com>
Contributor
|
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)
📝 WalkthroughWalkthroughThe pull request adds installer tests for unavailable systemd user managers. The tests cover safe service staging, enabled upstream activation links, glob characters in configuration paths, and fail-closed handling of unrelated ChangesGateway installation
Estimated code review effort: 2 (Simple) | ~10 minutes Mergeability Score: ⚪ Minimal · up to The installer now distinguishes an unavailable systemd user manager from a reachable manager while preserving existing compatibility checks and preventing fallback when an activation link exists. No actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit f2f0038 in the TypeScript / code-coverage/cliThe overall coverage in commit f2f0038 in the Show a code coverage summary of the most impacted files.
Updated |
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
7 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
3 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. Live E2E does not run automatically for pull requests. 2 semantic terminology decisionsTerminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.
E2E guidanceAdvisory only. A maintainer can dispatch the default E2E suite for the commit under review. Recommended E2E: None Manual-only E2E: This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
23 tasks
prekshivyas
approved these changes
Aug 13, 2026
prekshivyas
left a comment
prekshivyas
left a comment
Collaborator
There was a problem hiding this comment.
Approved based on code review. Required CI checks remain merge gates.
cv
pushed a commit
that referenced
this pull request
Aug 13, 2026
## Summary When `systemctl --user` reports a known unavailable-manager error, installation now keeps an existing standalone gateway. Installation stops if activation or uncertain evidence could create a second gateway on port 8080. ## Related Issue Fixes #8926 Supersedes #8945 by Dongni Yang. This implementation was developed independently and does not reuse or modify that contributor's commits. ## Changes - Add a three-state user-service inspection: reachable, known unavailable, or failed. A failed command cannot prove service identity. - Permit standalone fallback only for known manager failures with no activation path. A port check cannot prevent later systemd activation. - Validate the effective unit path, executable path, and gateway version when the user manager is reachable. - Inspect both gateway service names across configuration, package, runtime, control, transient, and generator activation roots. - Reject custom unit search paths, inaccessible paths, dangling links, ambiguous metadata, unknown errors, and version mismatches. - Apply the same lifecycle rules to TypeScript onboarding. Require explicit fallback permission before standalone startup. - Add installer and CLI regression tests for fallback, refusal, identity, version, and activation behavior. - Document host gateway ownership, the fallback boundary, and the required remediation. - Preserve the original report attribution with `Reported-by: Hadi Askari` in the commit. ## Type of Change - [ ] Code change (feature, bug fix, or refactor) - [x] 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: - [x] Docs updated for user-facing behavior changes - [ ] Docs not applicable — justification: - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: Independent security review completed on the published branch revision. The focused follow-up escaped the environment-controlled systemd override diagnostic and added a log-line-injection regression test; all nine review categories now pass with no open findings. - [ ] Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue: No waiver requested. Current `main` fails the test-size budget because `test/managed-image-publication-workflow.test.ts` has 1,529 lines. This PR does not change that file and must not merge until the gate passes. ## Documentation Writer Review - [x] Documentation writer subagent reviewed the completed changes - Result: `docs-updated` - Evidence: Updated `docs/deployment/gateway-lifecycle-authority.mdx`, `docs/reference/architecture.mdx`, and `docs/reference/troubleshooting.mdx`. Reviewed the OpenClaw, Hermes, and Deep Agents generated variants. - Agent: Codex Desktop <!-- docs-review-head-sha: b62d5a5 --> <!-- docs-review-agents-blob-sha: c4923a3 --> ## 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 — command/result or justification: The focused gateway lifecycle suites passed 98 tests, including all 34 installer lifecycle tests. ShellCheck, shfmt, Biome, repository checks, source-shape checks, the test-conditional guardrail, the full CLI build, and `npm run typecheck:cli` passed. - [ ] Applicable broad gate passed — `npm test` for broad runtime/test-harness changes; `npm run check` for repo-wide validation/coverage changes — command/result: Not applicable. The change affects the installer and gateway lifecycle, which the targeted suites cover. - [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) — The build passed with no errors and two existing Fern warnings. - [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) --- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **Bug Fixes** - Improved gateway installation checks for service identity, executable paths, versions, and systemd metadata. - Prevented unsafe standalone fallback when another gateway service may claim port 8080. - Installation now stops with clearer diagnostics for missing, invalid, untrusted, or incompatible service information. - Preserved standalone installation only when systemd is genuinely unavailable and no conflicting activation is detected. - Revalidated service conditions before making lifecycle changes. - **Documentation** - Added guidance for systemd availability issues, service conflicts, activation paths, inspection commands, and remediation steps. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Apurv Kumaria <akumaria@nvidia.com>
Contributor
Author
|
Closing: #8949 merged and covers this fix's full scope, including the activation-link hardening (glob-metachar-safe probes, ~/.config fallback, XDG_DATA_HOME / XDG_CONFIG_DIRS roots). Nothing left here to land separately. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
On Linux,
scripts/install.shtreated a packaged staticopenshell-gateway.serviceunit file as proof that the upstream user service is inspectable. Without a systemd user session,systemctl --user showfails withFailed to connect to bus: No medium found; the installer discarded that diagnostic (2>/dev/null) and exited before onboarding with the unrelated message:The TypeScript gateway lifecycle already classifies an unavailable user manager (
userManagerLooksUnavailable+ activation-link gate insrc/lib/onboard/docker-driver-gateway-service.ts:561-583,981-986); the installer preflight did not.Change
openshell_user_manager_unavailable_reason: classifies systemctl stderr with the same pattern as the TSuserManagerLooksUnavailable.upstream_openshell_gateway_user_manager_unavailable: probessystemctl --user show openshell-gateway.serviceand classifies only its stderr; any other failure keeps the existing fail-closed path.upstream_openshell_gateway_user_service_activation_link_installed: detects*.wants//*.requires/enablement links for the packaged unit across the persistent directories the user manager honors: the env-derived config home,~/.config(the manager resolves its config home from the login environment, not the installer shell),$XDG_DATA_HOME/~/.local/share,/etc/xdg/systemd/user,/etc/systemd/user, and the three static unit directories. Directory heads are quoted so glob metacharacters inHOME/XDG_CONFIG_HOMEstay literal; dangling links count (fail closed).install_nemoclaw_openshell_gateway_user_service: when the manager is unavailable and no activation link exists, falls through to staging the NemoClaw-managed unit (the packaged static unit can never activate, so no port-8080 competition is possible); when the manager is unavailable but an enabled link exists, fails closed with a topology-specific remediation; when the manager is reachable, the identity/version gate is unchanged.Acceptance criteria mapping
require_compatible_upstream_openshell_gateway_servicepath unchanged.No medium foundfall-through, unavailable manager + enabled activation link, enabled link under a glob-metacharacter config home, and non-classified systemctl failure staying fail-closed.Testing
npx vitest run --project integration test/install-openshell-gateway-service.test.ts— 21/21. The fall-through and enabled-link tests are red→green against the unmodified tree; the fail-closed guard is a characterization test (passes before and after by design).--project integrationsuite green.shfmt -i 2 -ci -bn -d scripts/install.shclean; shellcheck introduces no new findings; prek hooks (source-shape budget, test size budget, repository checks) pass.Closes #8926
Signed-off-by: Dongni Yang dongniy@nvidia.com
Summary by CodeRabbit