Skip to content

fix(hermes): read API port from gateway identity - #9055

Closed
cv wants to merge 1 commit into
mainfrom
codex/fix-9044
Closed

fix(hermes): read API port from gateway identity#9055
cv wants to merge 1 commit into
mainfrom
codex/fix-9044

Conversation

@cv

@cv cv commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

Hermes MCP transactions now resolve the sandbox API port from the validated same-identity gateway process. They no longer read the protected service-manager environment, which OpenShell denies across identities.

Related Issue

Fixes #9044

Changes

  • Read NEMOCLAW_HERMES_API_PORT from /proc/<gateway-pid>/environ after the existing gateway PID, owner, launcher, managed-parent, and start-time checks.
  • Recheck the gateway identity after the bounded environment read. Reject an unavailable, oversized, malformed, ambiguous, or identity-changed source without logging environment values.
  • Add a real same-identity process regression test that fails when the helper reads the service-manager PID. Keep focused negative tests for access denial, invalid ports, duplicate values, and identity changes.
  • Update the OpenShell 0.0.101 child-environment manifest with the changed helper's exact SHA-256 integrity value and keep its migration-review expectation aligned.
  • Root cause: the per-sandbox port change selected the service manager as the environment source. Existing tests mocked that cross-identity read and did not exercise the Linux process boundary.

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: The API-port range, default, onboarding input, MCP commands, and lifecycle behavior do not change.
  • 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: The nine-category security review passed with no findings for the behavior change: fix(hermes): read API port from gateway identity #9055 (comment). The later amendment only synchronizes the trusted manifest digest with the reviewed helper bytes.
  • 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: The change corrects an internal Hermes MCP API-port lookup. Existing documentation remains accurate for the environment-variable range, default, onboarding input, MCP commands, and lifecycle behavior. The reviewer found no blocker or writing suggestion.
  • Agent: Codex Desktop

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 — npx vitest run --project integration test/hermes-mcp-api-port.test.ts test/hermes-mcp-probe-api-port.test.ts test/hermes-mcp-config-transaction.test.ts test/hermes-mcp-apply-race.test.ts test/hermes-mcp-rollback-pending.test.ts test/hermes-mcp-integrity-state.test.ts test/hermes-mcp-reload-convergence.test.ts test/hermes-mcp-force-cleanup.test.ts test/hermes-mcp-private-target-validation.test.ts test/openshell-0.0.101-migration-review.test.ts: 10 files and 73 tests passed after the integrity-pin repair.
  • Applicable broad gate passed — npm test for broad runtime/test-harness changes; npm run check for repo-wide validation/coverage changes — command/result: Not run; this change is limited to one Hermes transaction helper and its focused integration tests.
  • 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: Carlos Villela cvillela@nvidia.com

Resolve the per-sandbox API port from the validated same-UID gateway
process instead of the protected service manager environment.

Signed-off-by: Carlos Villela <cvillela@nvidia.com>
@cv cv self-assigned this Aug 13, 2026
@copy-pr-bot

copy-pr-bot Bot commented Aug 13, 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 13, 2026

Copy link
Copy Markdown
Contributor

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Enterprise

Run ID: 7f8447aa-9d56-4535-b2de-002b3db120e9

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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

@cv

cv commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Security review verdict: PASS

No security finding remains in commit f62c0541ce.

Category Verdict Evidence
Secrets and credentials PASS The helper reads a bounded environment from the validated same-identity gateway. It selects only NEMOCLAW_HERMES_API_PORT and does not log or return environment contents.
Input validation and data sanitization PASS The existing parser permits ASCII decimal ports from 8642 through 8652. It rejects duplicate, malformed, non-ASCII, and out-of-range values.
Authentication and authorization PASS The existing gateway authority verifies the expected UID, trusted launcher arguments, managed parent, and process start identity. The change rechecks the complete identity after the environment read.
Dependencies and third-party libraries PASS The PR adds no dependency or downloaded artifact.
Error handling and logging PASS An unavailable or oversized process environment fails closed with a fixed error that contains no environment value.
Cryptography and data protection PASS The PR changes no cryptographic mechanism or stored data.
Configuration and security headers PASS The API-port range and default remain unchanged. The root-separated topology retains its root-owned marker path.
Security testing PASS The regression test reads a real same-identity child process. Negative tests cover denied access, oversized or invalid values, duplicate values, unsafe root markers, and changed identities.
System security PASS The read is scoped to the already validated gateway PID. A second identity check rejects process exit, PID reuse, launcher change, parent change, or start-identity drift during the read.

Files reviewed:

  • agents/hermes/mcp-config-transaction.py
  • test/hermes-mcp-api-port.test.ts

@github-code-quality

Copy link
Copy Markdown
Contributor

Code Coverage Overview

Languages: TypeScript

TypeScript / code-coverage/plugin

The overall coverage in commit f62c054 in the codex/fix-9044 branch remains at 96%, unchanged from commit ddcdd56 in the main branch.

@github-actions

github-actions Bot commented Aug 13, 2026

Copy link
Copy Markdown
Contributor

PR Review Advisor — No blocking findings reported

Advisor assessment: No blocking advisor findings reported
Next action: No advisor follow-up needed.
Findings: 0 blockers · 0 warnings · 0 suggestions

Model lanes

  • GPT-5.6 Terra (primary): Completed · high confidence · 0 blockers · 0 warnings · 0 suggestions
  • Nemotron 3 Ultra (second opinion): Failed after a partial review · low confidence · 0 blockers · 0 warnings · 0 suggestions

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

2 semantic terminology decisions

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

  • justified — identity-bound gateway environment at test/hermes-mcp-api-port.test.ts:51: Keep this modifier where the process-identity trust boundary matters.
  • established — gateway environment at agents/hermes/mcp-config-transaction.py:1048: Use gateway environment consistently for this process environment.

E2E guidance

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

Recommended E2E: None

Manual-only E2E: hermes-e2e, hermes-inference-switch, managed-image-multiarch-startup, security-posture, full-e2e
The manual PR workflow does not run these selectors for the commit under review. Run them from reviewed code on main.

3 optional E2E recommendations
  • mcp-bridge
  • concurrent-gateway-ports
  • hermes-gpu-startup

Workflow run details

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

@cv

cv commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator Author

Superseded by #9059, which includes the corrected reviewed-source integrity digest without rewriting the protected branch.

@cv cv closed this Aug 13, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Hermes E2E cannot read the managed service environment after per-sandbox API ports

1 participant