fix(testing): harden gateway bind verification#1847
fix(testing): harden gateway bind verification#1847cerebrum330 wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
📝 WalkthroughWalkthroughThis change adds loopback-only binding enforcement for the OpenShell gateway, ensuring the gateway container publishes to Changes
Sequence Diagram(s)sequenceDiagram
participant Gateway as Gateway Lifecycle
participant Inspector as Container Inspector
participant Parser as Output Parser
participant Docker as Docker Daemon
participant Health as Health Checker
Gateway->>Inspector: enforceLoopbackGatewayBinding()
Inspector->>Docker: docker inspect openshell-cluster-nemoclaw
Docker-->>Inspector: container config (port bindings)
Inspector->>Parser: parseGatewayEndpointHost(info)
Parser-->>Inspector: 127.0.0.1 or other host
Inspector->>Inspector: Check if binding<br/>already loopback-only
alt Binding not loopback
Inspector->>Docker: docker stop container
Docker-->>Inspector: stopped
Inspector->>Docker: docker rm container
Docker-->>Inspector: removed
Inspector->>Docker: docker run with -p 127.0.0.1:8080:30051/tcp
Docker-->>Inspector: container created
Inspector->>Health: Check container health
Health-->>Inspector: healthy
Inspector-->>Gateway: repaired: true
else Already loopback
Inspector-->>Gateway: repaired: false
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
bin/lib/onboard.js (1)
769-776: Multi-element entrypoint silently dropped.Lines 769-771 only handle single-element entrypoints. If the container has a multi-element entrypoint like
["/bin/sh", "-c"], the entrypoint is silently omitted from the recreated container, causing it to fall back to the image default.For the OpenShell gateway cluster image this is likely fine since the entrypoint is typically a single script, but consider logging a warning or handling multi-element entrypoints for robustness.
💡 Optional: Handle multi-element entrypoints
- if (Array.isArray(config.Entrypoint) && config.Entrypoint.length === 1) { - cmdParts.push("--entrypoint", config.Entrypoint[0]); + if (Array.isArray(config.Entrypoint) && config.Entrypoint.length > 0) { + // Docker --entrypoint takes the executable; extra args go after the image + cmdParts.push("--entrypoint", config.Entrypoint[0]); + // Note: Multi-element entrypoints need args passed after the image name, + // which is already handled by config.Cmd in most cases. }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/onboard.js` around lines 769 - 776, The code currently only handles single-element entrypoints (config.Entrypoint) and silently drops multi-element arrays; update the logic around config.Entrypoint and the code that pushes "--entrypoint" into cmdParts so multi-element entrypoints are preserved (e.g., join the array into a single string or iterate and pass each part properly) or, at minimum, emit a clear warning via the existing logger when config.Entrypoint is an array with length > 1; adjust the branch that currently does if (Array.isArray(config.Entrypoint) && config.Entrypoint.length === 1) to handle multi-element arrays or log the warning while still ensuring cmdParts contains the intended entrypoint form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@bin/lib/onboard.js`:
- Around line 769-776: The code currently only handles single-element
entrypoints (config.Entrypoint) and silently drops multi-element arrays; update
the logic around config.Entrypoint and the code that pushes "--entrypoint" into
cmdParts so multi-element entrypoints are preserved (e.g., join the array into a
single string or iterate and pass each part properly) or, at minimum, emit a
clear warning via the existing logger when config.Entrypoint is an array with
length > 1; adjust the branch that currently does if
(Array.isArray(config.Entrypoint) && config.Entrypoint.length === 1) to handle
multi-element arrays or log the warning while still ensuring cmdParts contains
the intended entrypoint form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a1c36552-7b97-4b17-9db4-b9255dfe83f8
📒 Files selected for processing (7)
bin/lib/onboard.jsdocs/reference/troubleshooting.mdeslint.config.mjsscripts/install.shtest/e2e/test-double-onboard.shtest/install-preflight.test.jstest/onboard.gateway-bind.test.js
wscurran
added
CI/CD
|
✨ Thanks for submitting this PR, which proposes a way to improve testing and fix issues with gateway bind verification. |
|
Hey @cerebrum330 — thanks for working on this! The idea of hardening gateway bind verification and supporting ANSI-colored openshell output was a good catch. This one has gone a bit stale (no review activity in ~10 days) and touches a broad set of files ( Going to close this for now to keep the PR backlog tidy, but if you'd like to revisit the gateway bind hardening in a fresh PR with a narrower scope, that'd be very welcome. Thanks for the contribution! 🙏 |
4 participants
Summary
Testing
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests