fix: align OpenClaw pairing state root#3737
Conversation
|
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:
📝 WalkthroughWalkthroughExports OPENCLAW_* pinned to /sandbox/.openclaw, ensures state subdirs and permissions, writes OPENCLAW_* into the connect-session env file, updates tests and provisioning checks, and adds a hermetic fake Slack REST+Socket Mode server, websocket policy helper, CI job, and E2E pairing test. ChangesOpenClaw state path standardization for pairing request resolution
sequenceDiagram
participant Host
participant SandboxGateway
participant FakeSlack
participant OpenClawState
Host->>SandboxGateway: run install and start gateway, start fake Slack via test
SandboxGateway->>FakeSlack: websocket upgrade to /socket-mode with placeholder token
FakeSlack->>SandboxGateway: send socket_mode event (event_callback)
SandboxGateway->>OpenClawState: issuePairingChallenge -> write pending pairing to credentials/slack-pairing.json
Host->>SandboxGateway: connect-shell approve pairing (approve code)
SandboxGateway->>OpenClawState: consume pairing, update slack-default-allowFrom.json
FakeSlack->>Host: capture shows rewritten token/body for websocket and chat.postMessage
🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
E2E Advisor RecommendationRequired E2E: Dispatch hint: Auto-dispatched E2E: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/nemoclaw-start.sh (1)
228-239: Run the entrypoint E2E suites before merge.Given these are
scripts/nemoclaw-start.shentrypoint-path changes, run the recommended E2E jobs to validate real sandbox lifecycle behavior (restart recovery, gateway kill recovery, and full onboard/cloud path).Also applies to: 1595-1601
🤖 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` around lines 228 - 239, Change introduces sandbox-wide OpenClaw state env vars (OPENCLAW_HOME, OPENCLAW_STATE_DIR, OPENCLAW_CONFIG_PATH, OPENCLAW_OAUTH_DIR) set from _SANDBOX_HOME/_OPENCLAW_STATE_DIR; before merging, run the entrypoint E2E suites that exercise sandbox lifecycle (restart recovery, gateway kill recovery, full onboard/cloud path) to validate these env changes; if failures arise, adjust the script to preserve per-user behavior or ensure shared state permissions and paths (check usages of _SANDBOX_HOME, _OPENCLAW_STATE_DIR and any code reading OPENCLAW_HOME/OPENCLAW_OAUTH_DIR) and rerun E2E until they pass.
🤖 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.
Nitpick comments:
In `@scripts/nemoclaw-start.sh`:
- Around line 228-239: Change introduces sandbox-wide OpenClaw state env vars
(OPENCLAW_HOME, OPENCLAW_STATE_DIR, OPENCLAW_CONFIG_PATH, OPENCLAW_OAUTH_DIR)
set from _SANDBOX_HOME/_OPENCLAW_STATE_DIR; before merging, run the entrypoint
E2E suites that exercise sandbox lifecycle (restart recovery, gateway kill
recovery, full onboard/cloud path) to validate these env changes; if failures
arise, adjust the script to preserve per-user behavior or ensure shared state
permissions and paths (check usages of _SANDBOX_HOME, _OPENCLAW_STATE_DIR and
any code reading OPENCLAW_HOME/OPENCLAW_OAUTH_DIR) and rerun E2E until they
pass.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 26f7db05-57e5-42fc-a39d-46c88f850443
📒 Files selected for processing (3)
scripts/nemoclaw-start.shtest/nemoclaw-start.test.tstest/sandbox-provisioning.test.ts
Selective E2E Results — ✅ All requested jobs passedRun: 26044452842
|
ericksoa
changed the title
ericksoa
added
v0.0.46
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/e2e/lib/fake-slack-api.cjs (1)
39-66:⚠️ Potential issue | 🟠 Major | ⚡ Quick winImplement the real Socket Mode bootstrap response for
apps.connections.open.
slackResponseFor()must return a successful response with a WebSocket URL when the/api/apps.connections.openendpoint is called with valid app-token authentication. Currently, it returnsok: falseeven when auth succeeds, breaking any client that follows Slack's actual Socket Mode bootstrap flow. Real clients callapps.connections.openfirst to obtain a dynamic WebSocket URL before attempting the connection—this fake implementation will silently pass the existing bespoke E2E tests but won't catch regressions in the actual Socket Mode connection path.💡 Suggested fix
-function slackResponseFor(pathname, authAccepted) { +function slackResponseFor(pathname, authAccepted, hostHeader = "") { if (!authAccepted) { return { status: 401, body: { ok: false, error: "bad_auth", endpoint: pathname } }; } + if (pathname === "/api/apps.connections.open") { + return { + status: 200, + body: { + ok: true, + url: `ws://${hostHeader}/socket-mode`, + }, + }; + } if (pathname === "/api/chat.postMessage") { return { status: 200, @@ - const response = slackResponseFor(pathname, authAccepted); + const response = slackResponseFor(pathname, authAccepted, req.headers.host || "");🤖 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/lib/fake-slack-api.cjs` around lines 39 - 66, slackResponseFor currently returns an error for /api/apps.connections.open even when authAccepted is true; update slackResponseFor to detect pathname === "/api/apps.connections.open" and when authAccepted is true return a 200 body matching Slack's Socket Mode bootstrap (ok: true and a url field with a websocket URL string) so clients can retrieve the dynamic WebSocket URL; use expectedTokenForPath/tokenLooksPlaceholder logic to ensure auth is validated for apps.connections.open and return the success response only when the app-token is accepted.
🤖 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/test-openclaw-slack-pairing.sh`:
- Around line 372-377: The test currently hard-codes OPENCLAW_STATE_DIR in the
exec gosu gateway env block (the line setting
OPENCLAW_STATE_DIR=/sandbox/.openclaw), which masks regressions in the gateway
startup wiring; remove that hard-coded export or replace it so the test inherits
the real runtime value (e.g., omit the OPENCLAW_STATE_DIR assignment or set
OPENCLAW_STATE_DIR="$OPENCLAW_STATE_DIR") so the gateway helper uses the actual
state-root provided by the startup logic instead of /sandbox/.openclaw.
---
Outside diff comments:
In `@test/e2e/lib/fake-slack-api.cjs`:
- Around line 39-66: slackResponseFor currently returns an error for
/api/apps.connections.open even when authAccepted is true; update
slackResponseFor to detect pathname === "/api/apps.connections.open" and when
authAccepted is true return a 200 body matching Slack's Socket Mode bootstrap
(ok: true and a url field with a websocket URL string) so clients can retrieve
the dynamic WebSocket URL; use expectedTokenForPath/tokenLooksPlaceholder logic
to ensure auth is validated for apps.connections.open and return the success
response only when the app-token is accepted.
🪄 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: af8dbcc7-bb9e-4833-a56a-f7cf4c16f98f
📒 Files selected for processing (5)
.coderabbit.yaml.github/workflows/nightly-e2e.yamltest/e2e/lib/fake-slack-api.cjstest/e2e/lib/slack-api-proof.shtest/e2e/test-openclaw-slack-pairing.sh
Selective E2E Results — ❌ Some jobs failedRun: 26049442236
|
Selective E2E Results — ✅ All requested jobs passedRun: 26049469071
|
Selective E2E Results — ✅ All requested jobs passedRun: 26050814842
|
Selective E2E Results — ❌ Some jobs failedRun: 26050722409
|
Selective E2E Results — ✅ All requested jobs passedRun: 26051141655
|
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/test-openclaw-slack-pairing.sh`:
- Around line 102-120: The sandbox_exec function currently masks the remote
command's exit status via the unconditional "|| true", causing callers to see
output even when the remote command failed; remove "|| true", capture the ssh
exit code after the subshell (e.g., result=$(run_with_timeout ... 2>&1); rc=$?),
then clean up the temp ssh_config, echo "$result", and return the captured rc
(return $rc) so callers can check success; apply the same pattern to the other
sandbox_exec call sites referenced.
🪄 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: 69ac7c44-a7c4-47cf-b2e9-723586787604
📒 Files selected for processing (2)
test/e2e/docs/parity-inventory.generated.jsontest/e2e/test-openclaw-slack-pairing.sh
✅ Files skipped from review due to trivial changes (1)
- test/e2e/docs/parity-inventory.generated.json
Selective E2E Results — ❌ Some jobs failedRun: 26051783730
|
Selective E2E Results — ✅ All requested jobs passedRun: 26051864791
|
Selective E2E Results — ❌ Some jobs failedRun: 26052400445
|
Selective E2E Results — ✅ All requested jobs passedRun: 26052470332
|
Selective E2E Results — ❌ Some jobs failedRun: 26053134608
|
Selective E2E Results — ✅ All requested jobs passedRun: 26053183931
|
Selective E2E Results — ❌ Some jobs failedRun: 26053932989
|
Selective E2E Results — ✅ All requested jobs passedRun: 26054001814
|
Selective E2E Results — ✅ All requested jobs passedRun: 26055052164
|
Selective E2E Results — ✅ All requested jobs passedRun: 26055097512
|
Selective E2E Results — ✅ All requested jobs passedRun: 26056294003
|
Selective E2E Results — ✅ All requested jobs passedRun: 26056358009
|
…te-root-3730 # Conflicts: # test/e2e/docs/parity-inventory.generated.json # test/e2e/lib/fake-slack-api.cjs
Selective E2E Results — ✅ All requested jobs passedRun: 26065036322
|
Selective E2E Results — ✅ All requested jobs passedRun: 26065061367
|
…te-root-3730 # Conflicts: # .coderabbit.yaml
Selective E2E Results — ✅ All requested jobs passedRun: 26076680090
|
…te-root-3730 # Conflicts: # test/e2e/docs/parity-inventory.generated.json # test/e2e/docs/parity-map.yaml
cjagwani
dismissed
their stale review
Softening to non-blocking comment review — the fix is correct; my three concerns are PR-shape suggestions, not gates.
This comment was marked as spam.
This comment was marked as spam.
Sorry, something went wrong.
Selective E2E Results — ✅ All requested jobs passedRun: 26117819739
|
Selective E2E Results — ✅ All requested jobs passedRun: 26118325036
|
cjagwani
left a comment
cjagwani
left a comment
There was a problem hiding this comment.
LGTM — fix looks right and the e2e is thorough. No blockers from my side.
Selective E2E Results — ✅ All requested jobs passedRun: 26118674522
|
Selective E2E Results — ✅ All requested jobs passedRun: 26119168776
|
…te-root-3730 # Conflicts: # .coderabbit.yaml
Selective E2E Results — ✅ All requested jobs passedRun: 26119839024
|
3 participants
Summary
/sandbox/.openclawopenclaw pairing approve slack <code>sees gateway-created pending requests.openclawwritable state directoriesFixes #3730.
Test Plan
npm run build:clibash -n scripts/nemoclaw-start.shgit diff --checknpm run source-shape:check./node_modules/.bin/vitest run test/nemoclaw-start.test.ts test/sandbox-provisioning.test.ts./node_modules/.bin/vitest run test/generate-openclaw-config.test.ts test/onboard-messaging.test.ts test/policies.test.ts test/validate-blueprint.test.ts --testTimeout 15000Notes
--no-verifybecause the local hook repeatedly failed before running tests onrm: dist: Directory not emptycaused by generateddist/.DS_Store. The same full CLI hook passed during commit after clearingdist/and buildingnemoclaw/dist.Summary by CodeRabbit