fix(cli): accept tagged trusted gateway identity - #8757
Merged
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Contributor
📝 WalkthroughWalkthroughThe gateway readiness path now validates stable process generations, trusted executable paths, Docker-driver environment evidence, and target-bound listeners. Tagged gateway identities can pass when owned-target and executable checks succeed. Tests cover accepted identities and fail-closed rejection cases. ChangesGateway process identity validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Code Coverage OverviewLanguages: TypeScript TypeScript / code-coverage/pluginThe overall coverage in commit e066143 in the TypeScript / code-coverage/cliThe overall coverage in commit e066143 in the Show a code coverage summary of the most impacted files.
Updated |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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)
src/lib/readiness/gateway-production.ts (1)
206-232: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winUse executable evidence in the onboarding identity path.
processIdentityMatchesGatewayBinarychecks only the command line. A foreign Linux process can spoof its command line and Docker-driver environment markers. Listener discovery andstopDockerDriverGatewayProcesscan then accept or terminate the wrong PID. ReusegatewayProcessIdentityMatchesTrustedBinarywith/proc/<pid>/exeevidence, or add equivalent executable checks.🤖 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/readiness/gateway-production.ts` around lines 206 - 232, Update processIdentityMatchesGatewayBinary and the related listener-discovery and stopDockerDriverGatewayProcess paths to validate the candidate PID’s actual executable via /proc/<pid>/exe, reusing gatewayProcessIdentityMatchesTrustedBinary where applicable. Do not rely solely on spoofable command-line or Docker-driver environment markers; reject candidates whose executable evidence does not match the trusted gateway binary before accepting or terminating the PID.Source: Path instructions
🤖 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 `@src/lib/readiness/gateway-production.ts`:
- Around line 206-232: Update processIdentityMatchesGatewayBinary and the
related listener-discovery and stopDockerDriverGatewayProcess paths to validate
the candidate PID’s actual executable via /proc/<pid>/exe, reusing
gatewayProcessIdentityMatchesTrustedBinary where applicable. Do not rely solely
on spoofable command-line or Docker-driver environment markers; reject
candidates whose executable evidence does not match the trusted gateway binary
before accepting or terminating the PID.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 60ceb11d-4258-4c29-9dcb-4bddca8065d6
📒 Files selected for processing (2)
src/lib/readiness/gateway-production.test.tssrc/lib/readiness/gateway-production.ts
senthilr-nv
added
bug-fix
Contributor
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
3 terminology differences from the second opinionAdvisory only. These are normalized differences from the primary terminology receipt.
5 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 against this exact revision. Recommended E2E: None 2 optional E2E recommendations
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Contributor
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)
src/lib/readiness/gateway-production.ts (1)
206-232: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftComplete the identity-path cutover before merge.
gatewayProcessCmdlineMatchesremains executable from the runtime path and its tests. Add public-entrypoint tests for onboarding,host:probe, and rebuild preflight. Remove the superseded path, or document a bounded compatibility window with a linked retirement issue and exit criteria.🤖 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/readiness/gateway-production.ts` around lines 206 - 232, Complete the identity-path cutover by removing the superseded gatewayProcessCmdlineMatches runtime path and updating all callers to use gatewayProcessIdentityMatchesTrustedBinary. Add public-entrypoint tests covering onboarding, host:probe, and rebuild preflight, including their trusted/untrusted outcomes. If compatibility must remain, document its bounded window, linked retirement issue, and explicit exit criteria.Source: Path instructions
🧹 Nitpick comments (1)
src/lib/readiness/gateway-production.ts (1)
166-180: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueAdd one-sided null-generation coverage
gatewayProcessSamplesMatchTrustedBinaryalready rejects either missing generation. Add tests fornull/valid and valid/null; the current test covers onlynull/null.🤖 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/readiness/gateway-production.ts` around lines 166 - 180, Add tests for gatewayProcessSamplesMatchTrustedBinary covering both one-sided missing-generation cases: null/valid and valid/null, asserting each returns false. Keep the existing null/null coverage and trusted-binary matching behavior unchanged.
🤖 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 `@src/lib/readiness/gateway-production.ts`:
- Around line 206-232: Complete the identity-path cutover by removing the
superseded gatewayProcessCmdlineMatches runtime path and updating all callers to
use gatewayProcessIdentityMatchesTrustedBinary. Add public-entrypoint tests
covering onboarding, host:probe, and rebuild preflight, including their
trusted/untrusted outcomes. If compatibility must remain, document its bounded
window, linked retirement issue, and explicit exit criteria.
---
Nitpick comments:
In `@src/lib/readiness/gateway-production.ts`:
- Around line 166-180: Add tests for gatewayProcessSamplesMatchTrustedBinary
covering both one-sided missing-generation cases: null/valid and valid/null,
asserting each returns false. Keep the existing null/null coverage and
trusted-binary matching behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c83d0e94-eabb-4806-a09e-ea39dbdf6905
📒 Files selected for processing (2)
src/lib/readiness/gateway-production.test.tssrc/lib/readiness/gateway-production.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/readiness/gateway-production.test.ts
jyaunches
approved these changes
Aug 10, 2026
jyaunches
added a commit
that referenced
this pull request
Aug 10, 2026
Signed-off-by: Julie Yaunches <jyaunches@nvidia.com>
23 tasks
apurvvkumaria
pushed a commit
that referenced
this pull request
Aug 10, 2026
## Summary OpenShell v0.0.101 reports its active gateway endpoint as `Server:`, while NemoClaw readiness accepted only `Gateway endpoint:`. This companion to #8757 accepts both producer labels, requires every recognized HTTP(S) loopback endpoint to agree with the expected port, and restores reuse of a healthy managed gateway. The first trusted PR E2E run also exposed a registered-but-stopped gateway state: OpenShell returned a connection error without repeating the gateway name, and public readiness classified the selected gateway as unknown instead of stale. The revised collector scopes `status` to the exact gateway and carries that authority into the existing stale-state classifier. The escaped regression had two detection gaps: the parser unit fixture modeled only the older label, and the live double-onboard target did not explicitly assert the producer label or the reuse outcome. ## Changes - Parse the exact `Gateway endpoint:` and `Server:` labels, failing closed on empty, malformed, off-host, off-port, non-HTTP(S), trailing, or conflicting evidence. - Scope public readiness status to the selected gateway and preserve registered connection failures as stale lifecycle state. - Add deterministic coverage across repeated production readiness probes sharing the same healthy gateway fixture. - Strengthen the live `double-onboard` target to prove the first invocation starts the gateway and the second reuses the same runtime. ## 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 restores the documented existing-gateway reuse behavior and does not change a command, flag, configuration, default, output, or supported workflow. - [x] Sensitive paths changed - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: an independent nine-category review passed for the production change; endpoint text remains supplementary to authoritative listener and runtime ownership evidence, and the stale-state probe is explicitly scoped to the selected gateway. The later exact-head change only linearizes a test mock for the repository growth guardrail. - [ ] 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: `no-docs-needed` - Evidence: The change restores the documented NemoClaw-managed OpenShell gateway readiness contract for OpenShell v0.0.101. It accepts `Server:` endpoint output, scopes status probes to the resolved gateway, and preserves a registered gateway as `stale` after a connection error. It does not change a NemoClaw command, flag, configuration, default, output, or supported workflow. `docs/reference/system-readiness.mdx` already documents canonical gateway ownership, stale reuse state, and loopback/port endpoint binding. `docs/reference/commands.mdx` already documents existing-gateway reuse and stale reconciliation. `docs/changelog/2026-08-10.mdx` already covers v0.0.106 readiness with OpenShell v0.0.101. - Agent: Codex Desktop <!-- docs-review-head-sha: 04228c9 --> <!-- 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: - `npm exec -- vitest run --project cli src/lib/readiness/gateway-production.test.ts` — 51/51 passed - `npm exec -- vitest run --project integration test/onboard-gateway-port-conflict-fast-fail.test.ts` — 2/2 passed - `npm run typecheck:cli` — passed - `npm run checks:repository` — passed - `npm run test:e2e-phases:check` — passed across 125 tests in 81 files - repository test-title checks — passed in the normal commit hooks - [ ] 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; focused validation covers the changed readiness and E2E contracts. - [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: Julie Yaunches <jyaunches@nvidia.com> <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Improved gateway endpoint detection across supported HTTP(S) formats. * Correctly identifies invalid, malformed, mismatched, or conflicting endpoints. * Prevents incorrect gateway status results when endpoint information is incomplete or unrelated. * **Reliability** * Re-onboarding now validates the active gateway endpoint and configured port. * NemoClaw reuses a healthy gateway only when its verified listener and endpoint own the configured gateway port. <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Julie Yaunches <jyaunches@nvidia.com> Co-authored-by: Senthil Ravichandran <senthilr@nvidia.com>
23 tasks
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
Linux readiness now recognizes the target-bound OpenShell gateway tag when the kernel-backed executable matches the independently trusted gateway binary. The existing Docker-driver environment, target, and stable process-sampling checks still fail closed.
Related Issue
Fixes #8755
Changes
openshell-gateway[nemoclaw=<name>;port=<port>]as process identity only after/proc/<pid>/exematches the trusted gateway binary and the tag matches the configured target.Type of Change
Quality Gates
docs/reference/system-readiness.mdxalready documents the independently resolved, kernel-backed executable match and stable process-sampling contract.Documentation Writer Review
no-docs-neededdocs/reference/system-readiness.mdxalready documents the exact gateway target, independently resolved kernel-backed executable identity, and stable process-sampling contract implemented by this change.DGX Station Hardware Evidence
Verification
Signed-off-by:line and every commit appears asVerifiedin GitHubpre-commit,commit-msg, andpre-pushhooks passed, ornpm run validate:prpassed after refreshingorigin/mainwhen hooks were skipped or unavailablenpm run test:changedpassed 72/72;npm run build:cli,npm run typecheck:cli,npm run test:titles:check, andnpm run checks:repositorypassed.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: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit