Skip to content

fix(e2e): honor managed gateway unit path - #9261

Merged
senthilr-nv merged 2 commits into
mainfrom
codex/fix-portable-gateway-unit-path
Aug 17, 2026
Merged

fix(e2e): honor managed gateway unit path#9261
senthilr-nv merged 2 commits into
mainfrom
codex/fix-portable-gateway-unit-path

Conversation

@senthilr-nv

@senthilr-nv senthilr-nv commented Aug 16, 2026

Copy link
Copy Markdown
Collaborator

Summary

The protected Portable Profile systemctl fixture now reads the OpenShell gateway binary selected by the managed user service instead of assuming the user XDG bin directory. It accepts only the three installer-owned locations and rejects ambiguous or untrusted service identities.

This restores the portable-launch fixture after automatic run 31969858915, job 95220512323, rejected a valid /usr/local/bin/openshell-gateway unit. The immutable artifact is 9269507500 with digest sha256:2bc324b785af9d93abef8dc3a4bc4da38b4c2d30f51e7f88838e8cf0cd6315e0.

Related Issue

Relates #9208.

Changes

  • Read exactly one absolute ExecStart path from the managed OpenShell gateway user service.
  • Accept only the installer-supported XDG user bin, /usr/local/bin, or /usr/bin gateway locations.
  • Keep the exact ExecStartPre, unit marker, environment, file type, permission, and symlink checks fail closed.
  • Cover both system locations and an untrusted path with deterministic fixture tests.

Type of Change

  • 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

  • Tests added or updated for changed behavior
  • Existing tests cover changed behavior — justification:
  • Tests not applicable — justification:
  • Docs updated for user-facing behavior changes
  • Docs not applicable — justification: This changes only the protected Portable Profile E2E fixture and its deterministic support coverage; production and supported user behavior are unchanged.
  • 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: Independent nine-category security review of commit 6b8fc44b8 passed with no findings; the path allowlist matches installer authority and preserves existing unit, environment, process-identity, logging, and cleanup controls.
  • Non-success, skipped, or missing CI check accepted by maintainer — check name, approval link, and follow-up issue:

Documentation Writer Review

  • Documentation writer subagent reviewed the completed changes
  • Result: no-docs-needed
  • Evidence: Independent Pi CLI review covered the complete two-file PR diff at latest PR commit 338dd2fd6. The change is limited to an internal E2E fixture and support tests for installer-selected OpenShell gateway paths. It changes no public command, configuration, workflow, default, or supported behavior. The conflict-free main merge incorporates the terminal-test fix required by CI.
  • Agent: Pi CLI

DGX Station Hardware Evidence

  • Tested on DGX Station
  • Tested commit:
  • Station profile/scenario:
  • Result:
  • Supporting evidence:

Verification

  • PR description includes a Signed-off-by: line and every commit appears as Verified in GitHub
  • 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
  • Targeted behavior tests pass for the current change set, or tests are marked not applicable above — command/result or justification: ./node_modules/.bin/vitest run --project e2e-support test/e2e/support/portable-profile-systemctl-shim.test.ts passed 30/30 on exact current main.
  • 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 to this two-file fixture repair. Current-main checks:repository, CLI typecheck, source-shape, test-size, conditional, test-loop, and scoped normal hooks passed.
  • Quality Gates section completed with required justifications or waivers
  • No secrets, API keys, or credentials committed
  • npm run docs builds without warnings (doc changes only)
  • Doc pages follow the style guide (doc changes only)
  • New doc pages include SPDX header and frontmatter (new pages only)

Signed-off-by: Senthil Ravichandran senthilr@nvidia.com

Summary by CodeRabbit

  • Bug Fixes

    • Improved validation of the managed gateway service configuration.
    • Gateway binaries must now use trusted system locations and be regular, readable executables.
    • Added checks to ensure service start commands consistently reference the configured gateway binary.
    • Improved error reporting when the gateway identity or executable configuration is invalid.
  • Tests

    • Added coverage for valid gateway paths in trusted locations.
    • Added validation for rejecting untrusted paths and missing executable output.

Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
@senthilr-nv senthilr-nv added the v0.0.110 Release target label Aug 16, 2026
@senthilr-nv senthilr-nv self-assigned this Aug 16, 2026
@copy-pr-bot

copy-pr-bot Bot commented Aug 16, 2026

Copy link
Copy Markdown

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.

@coderabbitai

coderabbitai Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 16384e9b-6d31-4363-9e69-f2938b7552c7

📥 Commits

Reviewing files that changed from the base of the PR and between 1feaf2b and 6b8fc44.

📒 Files selected for processing (2)
  • test/e2e/fixtures/portable-profile-systemctl-shim.sh
  • test/e2e/support/portable-profile-systemctl-shim.test.ts

Included review availability: Your plan includes up to 12 reviews per rolling hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

The portable systemctl shim now resolves the gateway binary from the managed systemd unit. It accepts trusted installation paths, validates matching service entries, rejects symlinks, and adds tests for accepted and untrusted paths.

Changes

Gateway binary validation

Layer / File(s) Summary
Managed path resolution
test/e2e/fixtures/portable-profile-systemctl-shim.sh
The shim extracts one absolute ExecStart path and accepts only trusted gateway locations.
Unit and binary checks
test/e2e/fixtures/portable-profile-systemctl-shim.sh, test/e2e/support/portable-profile-systemctl-shim.test.ts
The shim validates ExecStart, ExecStartPre, and binary file properties. Tests cover trusted paths and reject an untrusted path.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 6b8fc

This localized fixture change restores support for managed gateway paths while preserving fail-closed validation; no actionable merge-blocking risk remains after normal checks and review.

Possibly related PRs

  • NVIDIA/NemoClaw#9225: Extends the gateway unit validation and binary-path handling in the same shim and tests.

Suggested reviewers: prekshivyas, cv

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the fix to use the managed gateway unit path.
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/fix-portable-gateway-unit-path

Comment @coderabbitai help to get the list of available commands.

@github-code-quality

github-code-quality Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit 338dd2f in the codex/fix-portable-g... branch remains at 96%, unchanged from commit 2d03922 in the main branch.

TypeScript / code-coverage/cli

The overall coverage in commit 338dd2f in the codex/fix-portable-g... branch is 82%. The coverage in commit 2d03922 in the main branch is 83%.

Show a code coverage summary of the most impacted files.
File main 2d03922 codex/fix-portable-g... 338dd2f +/-
src/lib/cua/bounded-file.ts 94% 84% -10%
src/lib/cua/run...ime-manifest.ts 91% 84% -7%
src/lib/cua/contract.ts 87% 80% -7%
src/lib/state/o...d-checkpoint.ts 91% 87% -4%
src/lib/trace.ts 94% 90% -4%
src/lib/state/registry.ts 92% 90% -2%
src/lib/state/o...oard-session.ts 88% 87% -1%
src/lib/onboard...press-resume.ts 79% 82% +3%
src/lib/onboard...der/snapshot.ts 75% 83% +8%
src/lib/securit...ntial-filter.ts 72% 88% +16%

Updated August 17, 2026 01:16 UTC

@github-actions

github-actions Bot commented Aug 16, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: Review the warnings below.
Findings: 0 blockers · 1 warning · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 1 warning · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Model comparison: normalized findings differ; normalized terminology decisions differ; normalized E2E selections match; Nemotron reported the same number of blockers, 1 fewer warning, the same number of suggestions.
1 terminology difference from the second opinion

Advisory only. These are normalized differences from the primary terminology receipt.

  • OpenShell gateway user service identity at test/e2e/fixtures/portable-profile-systemctl-shim.sh:502: selected only by the second-opinion lane as justified.

Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests.

3 semantic terminology decisions

Terminology decisions are advisory. They affect the assessment only when a separate finding identifies concrete semantic impact.

  • established — managed OpenShell gateway user service at test/e2e/support/portable-profile-systemctl-shim.test.ts:470: Keep this term. It identifies the existing managed-service concept.
  • justified — installer-selected OpenShell gateway binary at test/e2e/support/portable-profile-systemctl-shim.test.ts:456: Keep the modifier. It distinguishes the service-recorded executable from an assumed path.
  • justified — untrusted binary path at test/e2e/support/portable-profile-systemctl-shim.test.ts:470: Keep the modifier. It identifies paths that the fixture must reject.

E2E guidance

Advisory only. A maintainer can dispatch the default E2E suite for the commit under review.

Recommended E2E: None

Manual-only E2E: cloud-onboard, security-posture, cloud-inference
The manual PR workflow does not run these selectors for the commit under review. Run them from reviewed code on main.

1 warning · 0 suggestions

Warnings

Warnings do not block.

PRA-1 Warning — Exercise gateway launch with an installer-selected system path

  • Location: test/e2e/support/portable-profile-systemctl-shim.test.ts:455
  • Category: tests
  • Problem: The new parameterized test verifies that read_managed_gateway_binary_path accepts /usr/local/bin and /usr/bin. It does not start the managed gateway service with either selected path.
  • Impact: A later change can preserve path parsing while failing to launch the gateway from an accepted system path. The portable-profile fixture would then reject or fail a valid installer-managed service.
  • Recommendation: Extend the managed gateway lifecycle test to use an accepted system path and verify that the fixture starts the gateway from the unit-selected executable.
  • Verification: Inspect the managed gateway lifecycle test and confirm that it writes the default XDG path rather than an accepted system path.
  • Test coverage: Add a fixture lifecycle test that writes a managed gateway unit with an accepted system binary path, starts the gateway service, and verifies the recorded gateway command used that path.
  • Evidence: test/e2e/support/portable-profile-systemctl-shim.test.ts:455-467 tests accepted system paths only with read_managed_gateway_binary_path. The static test inventory identifies an existing managed gateway lifecycle test but no lifecycle test that selects a system binary path.

Workflow run details

This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge.

@cv cv left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Issue #9208 covers this internal Portable Profile fixture repair. The implementation restricts the managed gateway binary to installer-authorized paths and preserves fail-closed unit, executable, process, and cleanup checks. Current CI passes at 338dd2fd6, GitHub reports the PR as mergeable, all repository gates pass, and the complete security review found no blocking issue.

@senthilr-nv
senthilr-nv merged commit 8158c4b into main Aug 17, 2026
70 checks passed
@senthilr-nv
senthilr-nv deleted the codex/fix-portable-gateway-unit-path branch August 17, 2026 01:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

v0.0.110 Release target

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants