fix(messaging): use native WebSocket credential rewrite#3323
Open
ericksoa wants to merge 47 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR removes Hermes local Discord facade, decode-proxy, and Slack token rewriter components, replacing them with OpenShell native WebSocket (L7) credential-rewrite support. Policy and schema are extended for websocket protocol. OpenShell is pinned to 0.0.38 with messaging capability validation. Hermetic fake Discord Gateway and Slack API E2E helpers are added. Hermes startup, runtime, recovery, and onboarding are simplified. Tests and CI are updated throughout. ChangesMessaging Bridge Simplification & Native WebSocket Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
test/gateway-state.test.ts (1)
150-153: ⚡ Quick winAdd one ANSI-wrapped error fixture to lock in the new strip-and-reject path.
The new behavior depends on ANSI stripping before matching error text; a dedicated ANSI refusal fixture would prevent subtle parser regressions.
Proposed test addition
+const STATUS_SERVER_STATUS_REFUSED_ANSI = ` +Server Status + +Gateway: nemoclaw +Server: https://127.0.0.1:8080/ +\x1b[31mError:\x1b[0m Connection refused (os error 61) +`; + describe("isGatewayConnected", () => { @@ it("does not treat Server Status with connection errors as connected", () => { expect(isGatewayConnected(STATUS_SERVER_STATUS_REFUSED)).toBe(false); }); + + it("does not treat ANSI-wrapped connection errors as connected", () => { + expect(isGatewayConnected(STATUS_SERVER_STATUS_REFUSED_ANSI)).toBe(false); + });🤖 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 `@test/gateway-state.test.ts` around lines 150 - 153, Add a new ANSI-wrapped refusal fixture and use it in the existing test that calls isGatewayConnected so the ANSI-stripping path is exercised; create a constant (e.g., STATUS_SERVER_STATUS_REFUSED_ANSI) alongside STATUS_SERVER_STATUS_REFUSED containing the same refusal message wrapped with ANSI escape sequences, then add a test case in test/gateway-state.test.ts that calls isGatewayConnected(STATUS_SERVER_STATUS_REFUSED_ANSI) and expects false to lock in the strip-and-reject behavior.test/policies.test.ts (1)
806-848: ⚡ Quick winAdd Hermes Slack websocket assertions next to the Discord gateway check.
This PR changes
wss-primary.slack.comandwss-backup.slack.comin both Hermes policy files, but the new Hermes-specific structured test only exercisesgateway.discord.gg. A Slack regression in either YAML would currently pass.Suggested test shape
- it("Hermes Discord gateway policy enables native WebSocket credential rewrite", () => { + it("Hermes messaging gateways use native inspected WebSocket policies", () => { const policyFiles = [ path.join(REPO_ROOT, "agents/hermes/policy-additions.yaml"), path.join(REPO_ROOT, "agents/hermes/policy-permissive.yaml"), ]; + const cases = [ + { + host: "gateway.discord.gg", + expected: { websocket_credential_rewrite: true }, + }, + { + host: "wss-primary.slack.com", + expected: {}, + }, + { + host: "wss-backup.slack.com", + expected: {}, + }, + ]; for (const file of policyFiles) { const content = fs.readFileSync(file, "utf8"); const parsed = YAML.parse(content) as { network_policies?: Record< @@ ); - const endpoint = endpoints.find((candidate) => candidate.host === "gateway.discord.gg"); - expect(endpoint).toBeTruthy(); - expect(endpoint).toMatchObject({ - protocol: "websocket", - enforcement: "enforce", - websocket_credential_rewrite: true, - }); - expect(endpoint).not.toHaveProperty("access"); - expect(endpoint).not.toHaveProperty("tls"); - expect(endpoint?.rules).toEqual( - expect.arrayContaining([ - { allow: { method: "GET", path: "/**" } }, - { allow: { method: "WEBSOCKET_TEXT", path: "/**" } }, - ]), - ); + for (const { host, expected } of cases) { + const endpoint = endpoints.find((candidate) => candidate.host === host); + expect(endpoint).toBeTruthy(); + expect(endpoint).toMatchObject({ + protocol: "websocket", + enforcement: "enforce", + ...expected, + }); + expect(endpoint).not.toHaveProperty("access"); + expect(endpoint).not.toHaveProperty("tls"); + expect(endpoint?.rules).toEqual( + expect.arrayContaining([ + { allow: { method: "GET", path: "/**" } }, + { allow: { method: "WEBSOCKET_TEXT", path: "/**" } }, + ]), + ); + } } } });🤖 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 `@test/policies.test.ts` around lines 806 - 848, The test currently only asserts on the Discord gateway endpoint (candidate.host === "gateway.discord.gg"), so add analogous assertions for Slack WebSocket hosts "wss-primary.slack.com" and "wss-backup.slack.com": after building endpoints from policyFiles, locate endpoints with host === "wss-primary.slack.com" and host === "wss-backup.slack.com" and assert they exist, have protocol "websocket", enforcement "enforce", websocket_credential_rewrite true, no "access" or "tls" properties, and rules include the GET and WEBSOCKET_TEXT allow entries (same expectations used for the Discord endpoint).test/e2e/test-messaging-providers.sh (1)
945-951: 💤 Low valueMinor off-by-one in line count calculation.
Line 947's
tail -n "$((capture_after_negative - capture_before_negative + 1))"adds an extra line. Ifbefore=10andafter=12, this calculates 3 lines when you likely want 2 new lines. However, since the check is for presence ofDEFINITELY_NOT_REGISTEREDin the capture, the extra line is harmless (makes false negatives less likely, not more).This is low impact and the test logic is sound.
🤖 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 `@test/e2e/test-messaging-providers.sh` around lines 945 - 951, The tail line uses tail -n "$((capture_after_negative - capture_before_negative + 1))" which introduces an off-by-one; update the calculation to use tail -n "$((capture_after_negative - capture_before_negative))" (or otherwise ensure the computed count reflects only new lines) so the slice from FAKE_DISCORD_GATEWAY_CAPTURE_FILE between capture_before_negative and capture_after_negative is the correct length; modify the expression near the condition that references fake_gateway_ready, dc_ws_negative, capture_after_negative and capture_before_negative accordingly.
🤖 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.
Inline comments:
In `@scripts/install-openshell.sh`:
- Around line 36-40: The installer is pinned to a non-existent OpenShell tag via
MIN_VERSION and MAX_VERSION set to "0.0.38"; change those to an existing release
(e.g., "0.0.37") or make the version configurable, and ensure the release tag
exists before shipping; additionally, instead of relying only on numeric gating,
add a runtime capability check before the early-success exit path (the
download/validation block that currently short-circuits success) to verify the
specific feature/commit (PR `#1286` / commit
eab184f20bb27c1db8b62deb33717590b018a24a) is present—for example, after cloning
or extracting the OpenShell artifact inspect for the expected file/flag or check
the commit hash in the repo metadata and abort with a clear error if the
capability is missing.
In `@src/lib/onboard.ts`:
- Around line 4250-4259: The recoverGatewayRuntime() function currently calls
the CLI to start the gateway unconditionally; wrap its startup logic with the
same gatewayCliSupportsLifecycleCommands() guard used earlier (check
gatewayCliSupportsLifecycleCommands() at the start of recoverGatewayRuntime()),
and if lifecycle commands are unsupported, skip attempting "openshell gateway
start" and either return gracefully or throw the same descriptive error (or
respect an exitOnFailure-like behavior if recoverGatewayRuntime accepts such a
flag). Update recoverGatewayRuntime() to log the same guidance (mentioning
GATEWAY_NAME/GATEWAY_PORT or pointing to "openshell gateway add/select") when
skipping execution so callers on package-managed OpenShell do not invoke
unsupported CLI commands.
- Around line 3254-3264: The registry.clearAll() call should only run when the
gateway was actually destroyed via lifecycle commands; update the conditional so
registry.clearAll() executes only when hasLifecycleCommands is true and
destroyResult.status === 0 (i.e., after the run initiated by
gatewayCliSupportsLifecycleCommands() succeeded). Locate the branch using
hasLifecycleCommands, runOpenshell([... "gateway", "destroy" ...]) /
runOpenshell([... "gateway", "remove" ...]) and the registry.clearAll() call and
change the guard so removal-via-"gateway remove" does not clear the sandbox
registry.
---
Nitpick comments:
In `@test/e2e/test-messaging-providers.sh`:
- Around line 945-951: The tail line uses tail -n "$((capture_after_negative -
capture_before_negative + 1))" which introduces an off-by-one; update the
calculation to use tail -n "$((capture_after_negative -
capture_before_negative))" (or otherwise ensure the computed count reflects only
new lines) so the slice from FAKE_DISCORD_GATEWAY_CAPTURE_FILE between
capture_before_negative and capture_after_negative is the correct length; modify
the expression near the condition that references fake_gateway_ready,
dc_ws_negative, capture_after_negative and capture_before_negative accordingly.
In `@test/gateway-state.test.ts`:
- Around line 150-153: Add a new ANSI-wrapped refusal fixture and use it in the
existing test that calls isGatewayConnected so the ANSI-stripping path is
exercised; create a constant (e.g., STATUS_SERVER_STATUS_REFUSED_ANSI) alongside
STATUS_SERVER_STATUS_REFUSED containing the same refusal message wrapped with
ANSI escape sequences, then add a test case in test/gateway-state.test.ts that
calls isGatewayConnected(STATUS_SERVER_STATUS_REFUSED_ANSI) and expects false to
lock in the strip-and-reject behavior.
In `@test/policies.test.ts`:
- Around line 806-848: The test currently only asserts on the Discord gateway
endpoint (candidate.host === "gateway.discord.gg"), so add analogous assertions
for Slack WebSocket hosts "wss-primary.slack.com" and "wss-backup.slack.com":
after building endpoints from policyFiles, locate endpoints with host ===
"wss-primary.slack.com" and host === "wss-backup.slack.com" and assert they
exist, have protocol "websocket", enforcement "enforce",
websocket_credential_rewrite true, no "access" or "tls" properties, and rules
include the GET and WEBSOCKET_TEXT allow entries (same expectations used for the
Discord endpoint).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 788c24f2-f82e-42d1-9f32-8853a8dc3103
⛔ Files ignored due to path filters (2)
nemoclaw/package-lock.jsonis excluded by!**/package-lock.jsonpackage-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (44)
Dockerfileagents/hermes/Dockerfileagents/hermes/config/messaging-config.tsagents/hermes/discord-facade.pyagents/hermes/discord-preload/sitecustomize.pyagents/hermes/policy-additions.yamlagents/hermes/policy-permissive.yamlagents/hermes/start.shnemoclaw-blueprint/blueprint.yamlnemoclaw-blueprint/policies/openclaw-sandbox-permissive.yamlnemoclaw-blueprint/policies/presets/discord.yamlnemoclaw-blueprint/policies/presets/slack.yamlnemoclaw-blueprint/scripts/ws-proxy-fix.jsnemoclaw-blueprint/scripts/ws-proxy-fix.tspackage.jsonschemas/policy-preset.schema.jsonschemas/sandbox-policy.schema.jsonscripts/generate-openclaw-config.pyscripts/install-openshell.shscripts/nemoclaw-start.shsrc/lib/agent/runtime.test.tssrc/lib/agent/runtime.tssrc/lib/onboard.tssrc/lib/onboard/usage-notice.tssrc/lib/state/gateway.tstest/e2e/lib/discord-gateway-proof.shtest/e2e/lib/fake-discord-gateway.cjstest/e2e/lib/fake-slack-api.cjstest/e2e/lib/slack-api-proof.shtest/e2e/test-hermes-discord-e2e.shtest/e2e/test-messaging-providers.shtest/gateway-state.test.tstest/generate-hermes-config.test.tstest/generate-openclaw-config.test.tstest/hermes-discord-facade.test.tstest/nemoclaw-start.test.tstest/policies.test.tstest/sandbox-build-context.test.tstest/sandbox-init.test.tstest/sandbox-provisioning.test.tstest/seccomp-guard.test.tstest/service-env.test.tstest/validate-blueprint.test.tstest/validate-config-schemas.test.ts
💤 Files with no reviewable changes (9)
- test/hermes-discord-facade.test.ts
- agents/hermes/config/messaging-config.ts
- nemoclaw-blueprint/scripts/ws-proxy-fix.ts
- test/nemoclaw-start.test.ts
- agents/hermes/discord-preload/sitecustomize.py
- agents/hermes/discord-facade.py
- nemoclaw-blueprint/scripts/ws-proxy-fix.js
- test/seccomp-guard.test.ts
- Dockerfile
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
test/e2e/test-messaging-providers.sh (1)
243-280:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the pre-merged Slack bootstrap policy in sync with the rewrite-aware preset.
This hardcoded block still omits the new rewrite flags. During install, Hermes/OpenClaw boots against this pre-merged policy before the real Slack preset is applied, so the bootstrap path is now behaviorally different from
presets/slack.yaml: REST calls won't getrequest_body_credential_rewrite, and Socket Mode hosts won't getwebsocket_credential_rewrite. That can regress the exact placeholder/alias flow this PR is trying to validate.Suggested diff
- host: slack.com port: 443 protocol: rest enforcement: enforce + request_body_credential_rewrite: true rules: - allow: { method: GET, path: "/**" } - allow: { method: POST, path: "/**" } - host: api.slack.com port: 443 protocol: rest enforcement: enforce + request_body_credential_rewrite: true rules: - allow: { method: GET, path: "/**" } - allow: { method: POST, path: "/**" } - host: hooks.slack.com port: 443 protocol: rest enforcement: enforce + request_body_credential_rewrite: true rules: - allow: { method: GET, path: "/**" } - allow: { method: POST, path: "/**" } - host: wss-primary.slack.com port: 443 protocol: websocket enforcement: enforce + websocket_credential_rewrite: true rules: - allow: { method: GET, path: "/**" } - allow: { method: WEBSOCKET_TEXT, path: "/**" } - host: wss-backup.slack.com port: 443 protocol: websocket enforcement: enforce + websocket_credential_rewrite: true rules: - allow: { method: GET, path: "/**" } - allow: { method: WEBSOCKET_TEXT, path: "/**" }🤖 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 `@test/e2e/test-messaging-providers.sh` around lines 243 - 280, The pre-merged Slack bootstrap policy block (the slack endpoints with hosts like slack.com, api.slack.com, hooks.slack.com, wss-primary.slack.com, wss-backup.slack.com) is missing the new rewrite flags so bootstrap behavior differs from presets/slack.yaml; update each REST endpoint entry to include request_body_credential_rewrite (or the exact key used in presets) and add websocket_credential_rewrite to websocket entries (wss-primary.slack.com, wss-backup.slack.com) so the temporary pre-merged policy matches the rewrite-aware preset applied later.agents/hermes/policy-permissive.yaml (1)
4-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the “no L7 filtering” header text to match current websocket rules.
Line 5 says this policy has no L7 filtering, but the websocket endpoints now explicitly filter by method/path (
GET+WEBSOCKET_TEXT). Please align the header comment to avoid operator confusion.Suggested doc-only patch
-# All known Hermes-relevant endpoints opened with access: full (no L7 filtering). +# All known Hermes-relevant endpoints are broadly opened. +# Note: websocket endpoints still use explicit method/path allow rules.Also applies to: 191-196, 233-246
🤖 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 `@agents/hermes/policy-permissive.yaml` around lines 4 - 6, Update the header comment that currently claims "no L7 filtering" to accurately state that websocket endpoints are explicitly filtered by method/path (specifically GET + WEBSOCKET_TEXT) in this permissive policy; edit the top-of-file comment in policy-permissive.yaml and the analogous header comments covering the other occurrences (around the blocks noted at lines 191-196 and 233-246) so they no longer say "no L7 filtering" but instead mention that websocket routes are constrained by method/path (GET + WEBSOCKET_TEXT) while other endpoints remain broadly open.
🧹 Nitpick comments (1)
test/e2e/test-hermes-slack-e2e.sh (1)
454-530: 🏗️ Heavy liftSwitch this Phase 6 proof to the fake Slack API helper instead of live
slack.com.The probe still POSTs to real Slack, so this test remains internet-dependent and never observes the request-body rewrite at a capture boundary. That means the new
request_body_credential_rewritepath can regress while this phase still passes or flakes based on external Slack behavior. Reusing the fake Slack API flow fromtest/e2e/test-messaging-providers.shwould make this proof hermetic and actually validate the new rewrite contract.🤖 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 `@test/e2e/test-hermes-slack-e2e.sh` around lines 454 - 530, Replace the live slack.com calls in the Python probe (the call(...) function and slack_probe usage) with the project’s fake Slack API helper used by the other messaging-provider tests: change the Request target from "https://slack.com/api/{path}" to the fake Slack helper endpoint (the same helper invoked by the messaging-provider tests), ensure the probe uses the same header/token shaping the helper expects (keep the existing token-prefix logic) and reuse the same invocation pattern (sandbox_exec_stdin -> Python) so the test becomes hermetic and will exercise the request_body_credential_rewrite path rather than hitting the real Slack API.
🤖 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.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 3254-3272: The destroy flow must not clear local state or mark the
gateway as missing when the underlying destroy command failed; update the logic
in destroyGateway()/the block using hasLifecycleCommands, destroyResult,
runOpenshell, registry.clearAll(), and dockerRemoveVolumesByPrefix so that
registry.clearAll() and any code that sets gatewayReuseState = "missing" only
run when destroyResult.status === 0, and ensure the function propagates the
failure (throw or return a non-zero/failure result) when
runOpenshell(["gateway","destroy",...]) fails so callers cannot continue
assuming the gateway was removed; apply the same guard/failure propagation to
the other similar blocks referenced (the other occurrences using
runOpenshell/destroyResult at the listed spots).
- Around line 1424-1438: The current probe in
gatewayCliSupportsLifecycleCommands() treats an empty or failed
`runCaptureOpenshell(["gateway","--help"])` (normalized.trim() === "") as
indicating lifecycle support; change the boolean assignment so a missing/empty
help output does NOT imply support. Specifically, in
gatewayCliSupportsLifecycleCommands(), use a guard that requires non-empty help
before checking for /\bstart\b/ and /\bdestroy\b/ (e.g.,
Boolean(normalized.trim()) && /\bstart\b/.test(normalized) &&
/\bdestroy\b/.test(normalized)) instead of the current !normalized.trim() ||
(...) logic; keep the runCaptureOpenshell/ANSI_RE usage but ensure
gatewayLifecycleCommandsSupported and its evaluation only become true when the
help output proves both commands exist.
- Around line 4252-4256: The error text that tells users to run "openshell
gateway add http://127.0.0.1:${GATEWAY_PORT} ..." is out of sync with the
runtime which uses getGatewayLocalEndpoint(); update the two places that print
those recovery hints (the block guarded by gatewayCliSupportsLifecycleCommands()
and the later similar block) to call getGatewayLocalEndpoint() instead of
hardcoding "http://127.0.0.1:${GATEWAY_PORT}", leaving the rest of the message
(flags and ${GATEWAY_NAME}) unchanged so the printed command matches the actual
local endpoint used by the implementation.
In `@test/e2e/lib/fake-slack-api.cjs`:
- Around line 43-56: The test helper is currently persisting raw secrets
(Authorization header and request body) into the capture log via record;
instead, change the record payload so it does not include raw Authorization or
full body—replace them with derived fields only: keep tokenMatchesExpected and
tokenLooksPlaceholder (booleans), add a hashed or redacted tokenSuffix (e.g.,
last 4 chars) and a bodyRedacted boolean or hash, and remove direct
authorization/body values before writing to captureFile; update any test
consumers to assert against tokenMatchesExpected, tokenLooksPlaceholder,
tokenSuffix or bodyRedacted/hash rather than the raw Authorization/body.
In `@test/onboard.test.ts`:
- Around line 5054-5061: Add symmetric placeholder-leak checks for Discord and
Telegram to the existing assertions on createCommand.command: ensure you call
assert.doesNotMatch(createCommand.command, /DISCORD_BOT_TOKEN=/) and
assert.doesNotMatch(createCommand.command, /TELEGRAM_BOT_TOKEN=/) alongside the
existing Slack checks so the sandbox create command never contains those
placeholder environment variable keys; update the test in onboard.test.ts near
the other doesNotMatch assertions referencing createCommand.command.
---
Outside diff comments:
In `@agents/hermes/policy-permissive.yaml`:
- Around line 4-6: Update the header comment that currently claims "no L7
filtering" to accurately state that websocket endpoints are explicitly filtered
by method/path (specifically GET + WEBSOCKET_TEXT) in this permissive policy;
edit the top-of-file comment in policy-permissive.yaml and the analogous header
comments covering the other occurrences (around the blocks noted at lines
191-196 and 233-246) so they no longer say "no L7 filtering" but instead mention
that websocket routes are constrained by method/path (GET + WEBSOCKET_TEXT)
while other endpoints remain broadly open.
In `@test/e2e/test-messaging-providers.sh`:
- Around line 243-280: The pre-merged Slack bootstrap policy block (the slack
endpoints with hosts like slack.com, api.slack.com, hooks.slack.com,
wss-primary.slack.com, wss-backup.slack.com) is missing the new rewrite flags so
bootstrap behavior differs from presets/slack.yaml; update each REST endpoint
entry to include request_body_credential_rewrite (or the exact key used in
presets) and add websocket_credential_rewrite to websocket entries
(wss-primary.slack.com, wss-backup.slack.com) so the temporary pre-merged policy
matches the rewrite-aware preset applied later.
---
Nitpick comments:
In `@test/e2e/test-hermes-slack-e2e.sh`:
- Around line 454-530: Replace the live slack.com calls in the Python probe (the
call(...) function and slack_probe usage) with the project’s fake Slack API
helper used by the other messaging-provider tests: change the Request target
from "https://slack.com/api/{path}" to the fake Slack helper endpoint (the same
helper invoked by the messaging-provider tests), ensure the probe uses the same
header/token shaping the helper expects (keep the existing token-prefix logic)
and reuse the same invocation pattern (sandbox_exec_stdin -> Python) so the test
becomes hermetic and will exercise the request_body_credential_rewrite path
rather than hitting the real Slack API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3374bf15-e017-4b69-a961-1d8c2a9828c2
📒 Files selected for processing (44)
.github/workflows/nightly-e2e.yaml.github/workflows/sandbox-images-and-e2e.yamlagents/hermes/Dockerfileagents/hermes/config/messaging-config.tsagents/hermes/decode-proxy.pyagents/hermes/discord-preload/sitecustomize.pyagents/hermes/policy-additions.yamlagents/hermes/policy-permissive.yamlagents/hermes/start.shagents/openclaw/policy-permissive.yamlnemoclaw-blueprint/policies/openclaw-sandbox-permissive.yamlnemoclaw-blueprint/policies/presets/slack.yamlnemoclaw-blueprint/scripts/slack-token-rewriter.jsschemas/policy-preset.schema.jsonschemas/sandbox-policy.schema.jsonscripts/generate-openclaw-config.pyscripts/install-openshell.shscripts/nemoclaw-start.shsrc/lib/agent/runtime.test.tssrc/lib/agent/runtime.tssrc/lib/onboard.tstest/cli.test.tstest/e2e/lib/fake-slack-api.cjstest/e2e/lib/slack-api-proof.shtest/e2e/test-hermes-discord-e2e.shtest/e2e/test-hermes-e2e.shtest/e2e/test-hermes-slack-e2e.shtest/e2e/test-messaging-providers.shtest/e2e/test-rebuild-hermes.shtest/gateway-state.test.tstest/generate-hermes-config.test.tstest/generate-openclaw-config.test.tstest/hermes-decode-proxy.test.tstest/install-openshell-version-check.test.tstest/nemoclaw-start.test.tstest/onboard.test.tstest/policies.test.tstest/runner.test.tstest/sandbox-init.test.tstest/sandbox-provisioning.test.tstest/slack-token-rewriter-sync.test.tstest/slack-token-rewriter.test.tstest/validate-blueprint.test.tstest/validate-config-schemas.test.ts
💤 Files with no reviewable changes (7)
- nemoclaw-blueprint/scripts/slack-token-rewriter.js
- test/slack-token-rewriter.test.ts
- test/hermes-decode-proxy.test.ts
- test/slack-token-rewriter-sync.test.ts
- agents/hermes/config/messaging-config.ts
- agents/hermes/discord-preload/sitecustomize.py
- agents/hermes/decode-proxy.py
✅ Files skipped from review due to trivial changes (3)
- .github/workflows/nightly-e2e.yaml
- test/e2e/test-hermes-e2e.sh
- test/generate-openclaw-config.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- test/gateway-state.test.ts
- test/generate-hermes-config.test.ts
- schemas/policy-preset.schema.json
- nemoclaw-blueprint/policies/openclaw-sandbox-permissive.yaml
- schemas/sandbox-policy.schema.json
- scripts/generate-openclaw-config.py
- test/e2e/lib/slack-api-proof.sh
- nemoclaw-blueprint/policies/presets/slack.yaml
- test/policies.test.ts
- test/nemoclaw-start.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 4007-4017: When verifying gateway health (in the block using
verifyGatewayContainerRunning() and in the image-drift branch using
getGatewayClusterImageDrift()), if the check indicates the gateway is stale or
drifted and destroyGateway() fails, downgrade gatewayReuseState from "healthy"
to a non-reuse value (e.g., "missing") so later canReuseHealthyGateway logic
cannot reuse it; update the branches around verifyGatewayContainerRunning() and
getGatewayClusterImageDrift() to set gatewayReuseState = "missing" whenever
cleanup/destroyGateway() returns false (and keep the existing logs/warnings).
In `@test/e2e/lib/fake-slack-api.cjs`:
- Around line 64-73: The auth response currently uses only tokenMatchesExpected
to decide success, so a correct header but wrong request-body token can be
accepted; update the auth decision to require both the header match
(tokenMatchesExpected) and that the parsed body token (e.g., the variable that
holds form field "token") also equals the expected token before returning 200.
Locate the tokenMatchesExpected check in fake-slack-api.cjs and combine it with
the body token validation (logical AND) when setting the status and error string
so incorrect form tokens produce the 401/"invalid_auth" outcome.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 21ea2b06-a0f5-4816-912d-c80014218a01
📒 Files selected for processing (4)
src/lib/onboard.tstest/e2e/lib/fake-slack-api.cjstest/e2e/test-messaging-providers.shtest/onboard.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e/test-messaging-providers.sh
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@test/e2e/lib/fake-slack-api.cjs`:
- Line 11: Validate the FAKE_SLACK_API_PORT value before using it to start the
server: replace the blind coercion in the port variable with an explicit parse
and check (e.g., parseInt/Number and Number.isInteger) and ensure it falls in
the allowed range (0 or 1–65535); if validation fails, log a clear error and
exit (or throw) before calling server.listen(); update the code paths around the
port constant and the server.listen(...) call so invalid env values fail fast
with a descriptive message.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1e61c8f3-0db3-4fd0-8cfb-17991481fa25
📒 Files selected for processing (3)
src/lib/onboard.tstest/e2e/lib/fake-slack-api.cjstest/gateway-liveness-probe.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/lib/onboard.ts
wscurran
added
CI/CD
…websocket # Conflicts: # .github/workflows/nightly-e2e.yaml # nemoclaw-blueprint/blueprint.yaml # nemoclaw-blueprint/scripts/slack-token-rewriter.js # scripts/install-openshell.sh # scripts/nemoclaw-start.sh # src/lib/onboard.ts # test/e2e/test-hermes-discord-e2e.sh # test/e2e/test-messaging-providers.sh # test/install-openshell-version-check.test.ts # test/nemoclaw-start.test.ts # test/sandbox-init.test.ts # test/slack-token-rewriter.test.ts
…websocket # Conflicts: # agents/hermes/start.sh # test/sandbox-init.test.ts
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/lib/onboard.ts (1)
3865-3879:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the registry intact on registration-only
gateway removepaths.When
gatewayCliSupportsLifecycleCommands()is false, this falls back toopenshell gateway remove, which only unregisters a package-managed gateway. Clearingregistryin that case forgets sandboxes that still exist behind the host-managed gateway, so reconnect/resume breaks after the gateway is re-added. Please gateregistry.clearAll()behind the destructive paths only (dockerDriverorhasLifecycleCommands).Suggested fix
- if (gatewayRemoved) { + if (gatewayRemoved && (dockerDriver || hasLifecycleCommands)) { registry.clearAll(); }🤖 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/onboard.ts` around lines 3865 - 3879, The code currently clears registry regardless of whether the gateway removal was destructive; change the logic so registry.clearAll() runs only for destructive removals (when dockerDriver is true or gatewayCliSupportsLifecycleCommands() is true) by gating the registry.clearAll() call behind those conditions (use the existing dockerDriver and hasLifecycleCommands variables or the results of removeDockerDriverGatewayRegistration()/runOpenshell(...) that indicate destructive removal); update the block around gatewayRemoved and registry.clearAll to only call registry.clearAll() for the dockerDriver path or when hasLifecycleCommands is true (leave the non-destructive runOpenshell(["gateway","remove",...]) path from forgetting local sandboxes).
🧹 Nitpick comments (2)
test/nemoclaw-start.test.ts (1)
1403-1443: ⚡ Quick winAdd a revision-scoped placeholder case to this tripwire test.
This suite only locks in the legacy uppercase placeholder and the canonical
openshell:resolve:env:SLACK_BOT_TOKENform. The new runtime flow also uses revision-scoped placeholders likeopenshell:resolve:env:v11_SLACK_BOT_TOKEN, so a regression on that shape would currently slip through.🧪 Suggested assertion
expect(run('{"botToken":"xoxb-OPENSHELL-RESOLVE-ENV-SLACK_BOT_TOKEN"}\n').status).toBe(0); expect(run('{"token":"openshell:resolve:env:SLACK_BOT_TOKEN"}\n').status).toBe(0); + expect(run('{"token":"openshell:resolve:env:v11_SLACK_BOT_TOKEN"}\n').status).toBe(0);🤖 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 `@test/nemoclaw-start.test.ts` around lines 1403 - 1443, Add a new assertion to the "Slack secrets-on-disk tripwire" test that verifies revision-scoped placeholders pass the tripwire: when calling run(...) (the helper that writes openclaw.json and executes verify_no_slack_secrets_on_disk), assert that a config containing a revision-scoped placeholder like "openshell:resolve:env:v11_SLACK_BOT_TOKEN" (or similar "v<revision>_SLACK_BOT_TOKEN") returns status 0; update the test in test/nemoclaw-start.test.ts near the existing run(...) expectations so verify_no_slack_secrets_on_disk accepts the new placeholder shape in addition to the uppercase legacy and canonical forms.scripts/nemoclaw-start.sh (1)
1800-1800: ⚡ Quick winFactor the tmp trust-boundary allowlist into one helper.
This
validate_tmp_permissionsargument list is duplicated in both startup paths. For a security-sensitive allowlist like this, keeping two copies makes it easy to drift the root and non-root validation sets the next time a preload is added or removed.♻️ Suggested shape
+_TMP_TRUST_BOUNDARY_FILES=( + "$_SANDBOX_SAFETY_NET" + "$_PROXY_FIX_SCRIPT" + "$_NEMOTRON_FIX_SCRIPT" + "$_WS_FIX_SCRIPT" + "$_SECCOMP_GUARD_SCRIPT" + "$_CIAO_GUARD_SCRIPT" + "$_TELEGRAM_DIAGNOSTICS_SCRIPT" + "$_SLACK_GUARD_SCRIPT" +) + +validate_runtime_preloads() { + validate_tmp_permissions "${_TMP_TRUST_BOUNDARY_FILES[@]}" +} ... - validate_tmp_permissions "$_SANDBOX_SAFETY_NET" "$_PROXY_FIX_SCRIPT" "$_NEMOTRON_FIX_SCRIPT" "$_WS_FIX_SCRIPT" "$_SECCOMP_GUARD_SCRIPT" "$_CIAO_GUARD_SCRIPT" "$_TELEGRAM_DIAGNOSTICS_SCRIPT" "$_SLACK_GUARD_SCRIPT" + validate_runtime_preloads ... -validate_tmp_permissions "$_SANDBOX_SAFETY_NET" "$_PROXY_FIX_SCRIPT" "$_NEMOTRON_FIX_SCRIPT" "$_WS_FIX_SCRIPT" "$_SECCOMP_GUARD_SCRIPT" "$_CIAO_GUARD_SCRIPT" "$_TELEGRAM_DIAGNOSTICS_SCRIPT" "$_SLACK_GUARD_SCRIPT" +validate_runtime_preloadsAs per coding guidelines,
scripts/nemoclaw-start.shchanges affect every sandbox boot and are invisible to unit tests.Also applies to: 1965-1965
🤖 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 `@scripts/nemoclaw-start.sh` at line 1800, The duplicated tmp trust-boundary argument list passed to validate_tmp_permissions should be factored into a single helper so both startup paths use the same allowlist; create a canonical helper (for example a shell array or function named TMP_TRUST_ALLOWLIST or get_tmp_allowlist) that contains the entries "_SANDBOX_SAFETY_NET", "_PROXY_FIX_SCRIPT", "_NEMOTRON_FIX_SCRIPT", "_WS_FIX_SCRIPT", "_SECCOMP_GUARD_SCRIPT", "_CIAO_GUARD_SCRIPT", "_TELEGRAM_DIAGNOSTICS_SCRIPT", and "_SLACK_GUARD_SCRIPT", then replace both direct calls to validate_tmp_permissions(...) with a call that expands or passes that helper (e.g., validate_tmp_permissions "${TMP_TRUST_ALLOWLIST[@]}" or validate_tmp_permissions $(get_tmp_allowlist)), ensuring you preserve the exact identifiers and ordering and update both startup paths that previously duplicated the list.
🤖 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.
Inline comments:
In `@test/e2e/lib/fake-slack-api.cjs`:
- Around line 39-42: The request body is currently buffered unbounded via the
chunks array in the req.on("data", ...) handler; add a max size guard (e.g.,
MAX_BODY_BYTES) and in the req.on("data", chunk) handler track accumulated
bytes, and if the limit is exceeded, stop further processing by destroying or
pausing the socket and respond with an appropriate error (413 Payload Too Large)
instead of continuing to Buffer.concat; update the code around the
chunks/req.on("data") and req.on("end") logic to enforce this cap and clean up
resources when triggered.
---
Duplicate comments:
In `@src/lib/onboard.ts`:
- Around line 3865-3879: The code currently clears registry regardless of
whether the gateway removal was destructive; change the logic so
registry.clearAll() runs only for destructive removals (when dockerDriver is
true or gatewayCliSupportsLifecycleCommands() is true) by gating the
registry.clearAll() call behind those conditions (use the existing dockerDriver
and hasLifecycleCommands variables or the results of
removeDockerDriverGatewayRegistration()/runOpenshell(...) that indicate
destructive removal); update the block around gatewayRemoved and
registry.clearAll to only call registry.clearAll() for the dockerDriver path or
when hasLifecycleCommands is true (leave the non-destructive
runOpenshell(["gateway","remove",...]) path from forgetting local sandboxes).
---
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Line 1800: The duplicated tmp trust-boundary argument list passed to
validate_tmp_permissions should be factored into a single helper so both startup
paths use the same allowlist; create a canonical helper (for example a shell
array or function named TMP_TRUST_ALLOWLIST or get_tmp_allowlist) that contains
the entries "_SANDBOX_SAFETY_NET", "_PROXY_FIX_SCRIPT", "_NEMOTRON_FIX_SCRIPT",
"_WS_FIX_SCRIPT", "_SECCOMP_GUARD_SCRIPT", "_CIAO_GUARD_SCRIPT",
"_TELEGRAM_DIAGNOSTICS_SCRIPT", and "_SLACK_GUARD_SCRIPT", then replace both
direct calls to validate_tmp_permissions(...) with a call that expands or passes
that helper (e.g., validate_tmp_permissions "${TMP_TRUST_ALLOWLIST[@]}" or
validate_tmp_permissions $(get_tmp_allowlist)), ensuring you preserve the exact
identifiers and ordering and update both startup paths that previously
duplicated the list.
In `@test/nemoclaw-start.test.ts`:
- Around line 1403-1443: Add a new assertion to the "Slack secrets-on-disk
tripwire" test that verifies revision-scoped placeholders pass the tripwire:
when calling run(...) (the helper that writes openclaw.json and executes
verify_no_slack_secrets_on_disk), assert that a config containing a
revision-scoped placeholder like "openshell:resolve:env:v11_SLACK_BOT_TOKEN" (or
similar "v<revision>_SLACK_BOT_TOKEN") returns status 0; update the test in
test/nemoclaw-start.test.ts near the existing run(...) expectations so
verify_no_slack_secrets_on_disk accepts the new placeholder shape in addition to
the uppercase legacy and canonical forms.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 1ffdca16-4307-49a1-be5b-2f6cdad92b16
📒 Files selected for processing (22)
.github/workflows/nightly-e2e.yamlDockerfileagents/hermes/config/messaging-config.tsagents/hermes/policy-additions.yamlagents/hermes/start.shpackage.jsonscripts/install-openshell.shscripts/nemoclaw-start.shsrc/lib/agent/runtime.test.tssrc/lib/onboard.tssrc/lib/state/gateway.tstest/cli.test.tstest/e2e/lib/fake-slack-api.cjstest/e2e/test-hermes-slack-e2e.shtest/e2e/test-messaging-providers.shtest/generate-hermes-config.test.tstest/install-openshell-version-check.test.tstest/nemoclaw-start.test.tstest/onboard.test.tstest/runner.test.tstest/sandbox-init.test.tstest/sandbox-provisioning.test.ts
💤 Files with no reviewable changes (2)
- agents/hermes/config/messaging-config.ts
- Dockerfile
✅ Files skipped from review due to trivial changes (1)
- test/generate-hermes-config.test.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- test/runner.test.ts
- test/sandbox-provisioning.test.ts
- scripts/install-openshell.sh
- src/lib/state/gateway.ts
- agents/hermes/policy-additions.yaml
- test/onboard.test.ts
- test/e2e/test-messaging-providers.sh
- test/e2e/test-hermes-slack-e2e.sh
- agents/hermes/start.sh
- src/lib/agent/runtime.test.ts
- test/cli.test.ts
…websocket # Conflicts: # test/gateway-liveness-probe.test.ts
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryPi Semantic E2E AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@src/lib/onboard.ts`:
- Around line 3661-3665: The volume-prune call is executed unconditionally and
can corrupt a live gateway when destroyGateway() returns false; change the logic
so dockerRemoveVolumesByPrefix(`openshell-cluster-${GATEWAY_NAME}`, {
ignoreError: true }) is only executed when the gateway removal actually
succeeded (i.e., destroyGateway() returned true or no-failure result).
Concretely, update the block that currently checks dockerDriver ||
hasLifecycleCommands to also verify the successful removal result from
destroyGateway (or an explicit success flag returned by the caller) before
calling dockerRemoveVolumesByPrefix; reference the destroyGateway return value
and the existing symbols dockerDriver, hasLifecycleCommands,
dockerRemoveVolumesByPrefix, and GATEWAY_NAME when making the guard change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 97c054e5-f419-4e0c-b634-50dcf93944e4
📒 Files selected for processing (8)
.github/workflows/nightly-e2e.yamlscripts/nemoclaw-start.shsrc/lib/onboard.tstest/cli.test.tstest/gateway-liveness-probe.test.tstest/onboard.test.tstest/policies.test.tstest/sandbox-build-context.test.ts
✅ Files skipped from review due to trivial changes (1)
- test/sandbox-build-context.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- test/cli.test.ts
- test/gateway-liveness-probe.test.ts
- test/policies.test.ts
- test/onboard.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@scripts/install-openshell.sh`:
- Line 93: The failure path in openshell_has_required_messaging_features()
conflates a missing strings binary with true lack of messaging-rewrite support;
update the function to detect if command -v strings fails and return a distinct
non-zero status while exporting or returning an error reason, and then change
the callers that currently log "OpenShell missing messaging rewrite support." to
check that reason and produce a specific error like "OpenShell missing 'strings'
(install binutils)" when strings is absent; ensure both call sites that handle
openshell_has_required_messaging_features() use this new distinction so minimal
container images without binutils get the correct diagnostic.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 565beee2-aa6d-454a-8887-ba1fcaa03fcb
📒 Files selected for processing (2)
.github/workflows/nightly-e2e.yamlscripts/install-openshell.sh
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Signed-off-by: Aaron Erickson <aerickson@nvidia.com>
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25711376978
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25711440456
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25713038306
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25713229387
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25716272137
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25717826183
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25740269924
|
…websocket # Conflicts: # test/install-openshell-version-check.test.ts
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25742530325
|
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25744597156
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25744597156
|
Contributor
Selective E2E Results —
|
| Job | Result |
|---|---|
| brave-search-e2e | |
| cloud-e2e | |
| cloud-inference-e2e | |
| cloud-onboard-e2e | |
| credential-migration-e2e | |
| credential-sanitization-e2e | |
| deployment-services-e2e | |
| device-auth-health-e2e | |
| diagnostics-e2e | |
| docs-validation-e2e | |
| double-onboard-e2e | |
| gateway-health-honest-e2e | |
| gpu-double-onboard-e2e | ⏭️ skipped |
| gpu-e2e | ⏭️ skipped |
| hermes-discord-e2e | |
| hermes-e2e | |
| hermes-inference-switch-e2e | |
| hermes-slack-e2e | |
| inference-routing-e2e | |
| issue-2478-crash-loop-recovery-e2e | |
| kimi-inference-compat-e2e | |
| launchable-smoke-e2e | |
| messaging-compatible-endpoint-e2e | |
| messaging-providers-e2e | |
| network-policy-e2e | |
| onboard-repair-e2e | |
| onboard-resume-e2e | |
| openclaw-inference-switch-e2e | |
| openshell-gateway-upgrade-e2e | |
| overlayfs-autofix-e2e | |
| rebuild-hermes-e2e | |
| rebuild-hermes-stale-base-e2e | |
| rebuild-openclaw-e2e | |
| runtime-overrides-e2e | |
| sandbox-operations-e2e | |
| sandbox-survival-e2e | |
| shields-config-e2e | |
| skill-agent-e2e | |
| snapshot-commands-e2e | |
| telegram-injection-e2e | |
| token-rotation-e2e | |
| upgrade-stale-sandbox-e2e |
Contributor
Selective E2E Results — ❌ Some jobs failedRun: 25765294451
|
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 25765443073
|
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
4 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
allowed_ipsallowed_ipsand tighten gateway-state handling when OpenShell status output includes transport errorsDependency
This PR depends on OpenShell PR #1286: NVIDIA/OpenShell#1286
Required OpenShell head tested locally:
eab184f20bb27c1db8b62deb33717590b018a24a.The dependency is required because NemoClaw now uses
openshell policy update --add-endpoint ... allowed-ip=...for hermetic fake provider endpoints, and native WebSocket credential rewrite must work for Discord Gateway traffic. Without OpenShell #1286, the fake providers remain blocked by private-IP policy and WebSocket IDENTIFY token rewrite is not proven.Validation
npm run build:clinpx vitest run test/validate-config-schemas.test.ts test/policies.test.ts test/generate-openclaw-config.test.ts test/generate-hermes-config.test.ts test/hermes-decode-proxy.test.ts test/gateway-state.test.tsbash -n test/e2e/test-messaging-providers.sh test/e2e/test-hermes-discord-e2e.sh test/e2e/lib/*.shnode --check test/e2e/lib/fake-discord-gateway.cjs test/e2e/lib/fake-slack-api.cjsNEMOCLAW_OPENSHELL_BIN=/Users/aerickson/Documents/NemoDev/0510-fix-messaging/OpenShell/target/debug/openshell bash test/e2e/test-hermes-discord-e2e.sh- 24 passed, 0 failedNEMOCLAW_OPENSHELL_BIN=/Users/aerickson/Documents/NemoDev/0510-fix-messaging/OpenShell/target/debug/openshell bash test/e2e/test-messaging-providers.sh- 60 passed, 0 failed, 5 skippedfix/native-messaging-websocketNotes
This branch should not be merged before OpenShell #1286 lands or NemoClaw pins/installs an OpenShell build containing that PR.
Summary by CodeRabbit