test(e2e): add OpenClaw TUI chat-correlation coverage slice - #5150
Merged
Conversation
jyaunches
added
the
area: e2e
|
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. |
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:
📝 WalkthroughWalkthroughAdds a live Vitest regression test that provisions a cloud sandbox, executes an in-sandbox websocket repro for issues ChangesOpenClaw TUI Chat Correlation Live E2E Regression Test
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The PR introduces substantial live E2E regression logic with trace analysis, websocket driver generation, sandbox execution orchestration, nondeterministic retry handling, and contract violation detection across 603 lines of high-complexity code in the test file, plus 71 lines of medium-complexity CI workflow setup. The test file spans multiple independent functional checkpoints (trace types, analysis, driver generation, execution, orchestration) that interact intricately; the analyzer logic is dense with edge-case detection for reply correlation, missing finals, and user-turn ordering. The CI job integrates Docker auth retry logic and artifact configuration. Mixed file complexity and interdependent control flow across checkpoints require careful review. Possibly related issues
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 |
Contributor
E2E Advisor RecommendationRequired E2E: None Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
|
Contributor
Vitest E2E Scenario RecommendationRequired Vitest E2E scenarios: Dispatch required Vitest E2E scenarios:
Full Vitest E2E advisor summaryVitest E2E Scenario AdvisorBase: Required Vitest E2E scenarios
Optional Vitest E2E scenarios
Relevant changed files
|
Contributor
PR Review AdvisorFindings: 0 needs attention, 6 worth checking, 0 nice ideas Review findings🛠️ Needs attention
🔎 Worth checking
🌱 Nice ideas
Consider writing more tests for
Since last review detailsCurrent findings:
This is an automated advisory review. A human maintainer must make the final merge decision. |
Collaborator
|
Thanks for writing this up, but this is going in the opposite direction from the post-#5106 / #4941 decision. This PR only edits Concrete asks:
A planning-only inventory flip is not useful in the new direction. A focused migration PR for this coverage would be welcome if it follows the same shape as #5131/#5132/#5133/#5137/#5138/#5139/#5141/#5142/#5143/#5144/#5145/#5146/#5149. |
The openclaw-tui-chat-correlation migration (#5098 epic, #4347 owner) needs to upload a websocket repro driver into a cloud OpenClaw sandbox and run a multi-line shell pipeline that reads the gateway auth token and dispatches the driver. The legacy bash test does this via raw `openshell sandbox upload` and `openshell sandbox exec --name X -- sh -lc "..."` calls. Adds two typed wrappers on SandboxClient: - upload(name, localPath, remotePath) — wraps `openshell sandbox upload` - execShell(name, script, options) — wraps `openshell sandbox exec X -- sh -lc <script>` Both share the existing validateSandboxName guard that rejects flag-shaped names before command construction. Six new unit cases in e2e-clients.test.ts cover argv shape, options propagation, and sandbox-name validation for both helpers. Refs: #4347 #5098
…+ workflow job Ports the live block of test/openclaw-tui-chat-correlation.test.ts into the typed scenario framework as a free-standing live test (#5049 pattern), plus a discrete openclaw-tui-chat-correlation-vitest job in e2e-vitest-scenarios.yaml modeled on #5107's openshell-version-pin-vitest. Coverage: - Onboards a fresh cloud OpenClaw sandbox via OnboardingPhaseFixture.from with the cloud-openclaw profile (already in SUPPORTED_ONBOARDING). - Asserts the pinned 2026.5.27 OpenClaw version (host-side probe via openshell sandbox exec ... openclaw --version). Override via E2E_OPENCLAW_TUI_CORRELATION_PINNED_VERSION when bumping. - Idempotent in-sandbox gateway start on port 18789 via SandboxClient.execShell. - Uploads the websocket repro driver via SandboxClient.upload, runs it via execShell with the gateway auth token read from /sandbox/.openclaw/openclaw.json. - Asserts the #2603 + #3145 contract: no empty finals for submitted runs, no uncorrelated replies, no missing/duplicate replies, no missing/duplicate user turns. Preserves the looksLikeEventCaptureFailure retry-once guard (OpenClaw-side observability race; remove when the runtime exposes a deterministic chat readiness ack). Inventory: bridge-probe entry for test/e2e/test-openclaw-tui-chat-correlation.sh now references this file as its bridge probe (satisfies the inventory invariant that bridge-probe entries have non-empty bridgeProbes pointing at real paths). Bash workflow (nightly-e2e.yaml openclaw-tui-chat-correlation-e2e job) remains untouched per the migration freeze policy; deletion is a follow-up PR with #4357 approval after typed scenario soaks. Refs: #4347 #2603 #3145 #5098 #5049 #5107
First-pass dispatch (run 27284249981) failed at the version-pin assertion
with `spawn openshell ENOENT`. Cloud onboarding (`OnboardingPhaseFixture.from`)
succeeded, but `sandbox.exec(instance.sandboxName, ["openclaw", "--version"])`
spawned with an empty env because no env was passed: `ShellProbe.run`
defaults to `env: { ...(options.env ?? {}) }` (no inherit). Without PATH
the runner can't resolve openshell at ~/.local/bin.
Pass `env: buildAvailabilityProbeEnv()` to every sandbox.* call (exec,
execShell, upload). Mirrors the convention used by every phase fixture
(state-validation.ts, runtime.ts, lifecycle.ts). Documents the convention
inline so the next free-standing live test author follows it.
Refs: #4347 #2603 #3145
…tional
Pass 2 dispatch (run 27285642504) failed at the openclaw-version-pinned
assertion with exit 127 from `openshell sandbox exec <name> -- openclaw --version`.
The matrix `live-scenarios (ubuntu-repo-cloud-openclaw)` job ran 1 of 24
tests (mostly skipped) and the one that ran did not exercise sandbox.exec,
so this latent bug was not visible until a free-standing live test invoked
sandbox.exec against a real openshell binary.
Production code uniformly uses `-n <name>`:
- src/lib/agent/onboard.ts:276,335
- src/lib/onboard/initial-policy.ts:112,118,124
- src/lib/onboard/web-search-verify.ts:77,104
- src/lib/onboard/sandbox-verification-exec.ts:24
- src/lib/onboard/compatible-endpoint-smoke.ts:144
- src/lib/onboard/docker-gpu-supervisor-reconnect.ts:126
- src/lib/status-command-deps.ts:50
Legacy bash tests (test-rebuild-openclaw.sh, test-openclaw-tui-chat-correlation.sh)
and the existing test/openclaw-tui-chat-correlation.test.ts all use the
equivalent `--name <name>` long form. Positional name is rejected by the
real openshell binary as a mis-parsed argument and surfaces as exit 127
("command not found" semantic).
Switch SandboxClient.exec and execShell to `-n` form. SandboxClient.upload
stays positional — production code and the existing live test agree on
positional for `sandbox upload`. Updates the unit-test argv assertions
in e2e-clients, e2e-phase-runtime, and e2e-phase-state-validation; shifts
the chat-completion data-raw payload index from args[11] to args[12].
Refs: #4347 #2603 #3145 #5098
Two consecutive PASS dispatches on the openclaw-tui-chat-correlation-vitest job confirm convergence: - Pass 3: run 27286491062 \xe2\x86\x92 SUCCESS (369.41s) - Pass 4: run 27287442227 \xe2\x86\x92 SUCCESS (358.64s, verification dispatch) Both runs exercised the full 11-step assertion chain end-to-end against a real cloud OpenClaw sandbox: cloud onboarding \xe2\x86\x92 openclaw-version-pinned \xe2\x86\x92 sandbox-gateway-reachable \xe2\x86\x92 live-repro-emitted-result \xe2\x86\x92 6 chat- correlation assertions (#2603 + #3145 contract). Inventory transitions: - status: bridge-probe \xe2\x86\x92 covered - targetVitestScenarios: [test/e2e-scenario/live/openclaw-tui-chat-correlation.test.ts] - bridgeProbes: cleared - deletionReady: false (legacy bash wrapper retained until soak; deletion is a follow-up PR with #4357 approval per the migration freeze policy) Refs: #4347 #2603 #3145 #5098
jyaunches
marked this pull request as ready for review
June 10, 2026 15:49
Collaborator
|
#5126 is now green, so this draft should rebase/reshape before it moves toward review. It still targets |
![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
🤖 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 @.github/workflows/e2e-vitest-scenarios.yaml:
- Around line 215-286: The job openclaw-tui-chat-correlation-vitest currently
always runs even when a manual dispatch filtered by inputs.scenarios should skip
it; add a job-level conditional to respect the scenarios selector by adding an
if that allows the job when not a manual dispatch OR when
github.event.inputs.scenarios contains "openclaw-tui-chat-correlation" (e.g. if:
${{ github.event_name != 'workflow_dispatch' ||
contains(github.event.inputs.scenarios, 'openclaw-tui-chat-correlation') }}), so
the job only executes for the intended manual scenario or non-dispatch events.
In `@test/e2e-scenario/live/openclaw-tui-chat-correlation.test.ts`:
- Line 256: The test hardcodes port 18789 in the WebSocket URL when creating the
client (new WebSocket(...)), which will break if SANDBOX_GATEWAY_PORT changes;
update the WebSocket construction in openclaw-tui-chat-correlation.test.ts to
use the SANDBOX_GATEWAY_PORT constant instead of 18789 (e.g., interpolate
SANDBOX_GATEWAY_PORT into the "ws://127.0.0.1:<port>/ws" URL or pass it into the
script builder so new WebSocket uses the constant), ensuring the WebSocket
instantiation references SANDBOX_GATEWAY_PORT.
In `@test/e2e-scenario/migration/legacy-inventory.json`:
- Around line 581-588: The row was prematurely marked as covered—revert the
"status" field from "covered" back to its prior state (e.g., "migrating" or the
original value) and remove or clear the "targetVitestScenarios" entry until the
focused migration PR lands; also restore or update the
"notes"/"retiredReason"/"deletionReady" fields to reflect that the bash-backed
nightly lane is still retained and that migration evidence remains in PR `#5150`,
making sure to change the JSON keys "status", "targetVitestScenarios", and
"notes" in this object rather than leaving it marked covered.
🪄 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: 8fbf1250-a326-4cdd-b5b8-89c0d09e5973
📒 Files selected for processing (7)
.github/workflows/e2e-vitest-scenarios.yamltest/e2e-scenario/framework-tests/e2e-clients.test.tstest/e2e-scenario/framework-tests/e2e-phase-runtime.test.tstest/e2e-scenario/framework-tests/e2e-phase-state-validation.test.tstest/e2e-scenario/framework/clients/sandbox.tstest/e2e-scenario/live/openclaw-tui-chat-correlation.test.tstest/e2e-scenario/migration/legacy-inventory.json
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-scenario/live/openclaw-tui-chat-correlation.test.ts (1)
525-530: ⚡ Quick winPersist the full repro payload in
issue2603-trace.json.This artifact currently drops the actual events/history/order data that explains a failure, so post-mortems still depend on console output or a rerun. For a 75-minute live probe, saving
reproand the derivedanalysisis a much better debugging default.♻️ Proposed change
- await artifacts.writeJson("issue2603-trace.json", { - sentRuns: repro.sentRuns, - eventCount: repro.events?.length ?? 0, - attempts: attempts.length, - error: repro.error, - }); + await artifacts.writeJson("issue2603-trace.json", { + repro, + attempts, + analysis: repro.error ? undefined : analyzeIssue2603Trace(repro), + });🤖 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-scenario/live/openclaw-tui-chat-correlation.test.ts` around lines 525 - 530, The artifact write currently only writes selected fields (sentRuns, eventCount, attempts, error); change the call to persist the full repro payload and derived analysis by writing the entire repro object and the analysis object to "issue2603-trace.json" (e.g., include repro and analysis alongside attempts and any metadata) so the file contains full events/history/order data; update the artifacts.writeJson invocation that creates "issue2603-trace.json" to serialize repro and analysis (and keep attempts/errors if desired).
🤖 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 `@test/e2e-scenario/live/openclaw-tui-chat-correlation.test.ts`:
- Around line 525-530: The artifact write currently only writes selected fields
(sentRuns, eventCount, attempts, error); change the call to persist the full
repro payload and derived analysis by writing the entire repro object and the
analysis object to "issue2603-trace.json" (e.g., include repro and analysis
alongside attempts and any metadata) so the file contains full
events/history/order data; update the artifacts.writeJson invocation that
creates "issue2603-trace.json" to serialize repro and analysis (and keep
attempts/errors if desired).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 88343285-11b4-4374-a113-ef05f31db27d
📒 Files selected for processing (1)
test/e2e-scenario/live/openclaw-tui-chat-correlation.test.ts
Collaborator
|
Update after #5126 moved the foundation to the fixtures namespace and picked up the sandbox helper prerequisite: this remains a useful salvage candidate, but the reshaped PR should target #5126, drop |
This was referenced Jun 10, 2026
Contributor
Author
|
Migration-principle review for #5098: this PR has useful coverage, but it is drifting larger than “simple Vitest test, not new framework.” Reference from Concerns in this PR:
Recommended path:
Other acceptable options:
Suggested immediate changes:
|
…claw-tui-chat-correlation
prekshivyas
approved these changes
Jun 10, 2026
prekshivyas
left a comment
prekshivyas
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed (code + 9-cat security). Migrates the openclaw TUI chat-correlation E2E (#2603/#3145) to a free-standing live Vitest test, adds a manual CI job + execShell/upload helpers, and fixes exec to pass the -n name flag.
✅ Approve — coverage is a strict superset (all 6 legacy correlation checks + 2 new ordering checks), and the load-bearing -n fix is verified correct: production uniformly uses -n/--name (agent/onboard.ts, initial-policy.ts, web-search-verify.ts), so the old positional name was a latent bug; the 4 unit-test argv updates + the args[11]→args[12] index shift are consistent.
Security: all 9 pass — secrets scoped per-step and machine-enforced by the new workflow-boundary.mts validator (no job-level secret env), actions SHA-pinned, persist-credentials:false, loopback-only websocket, sandbox names validated (rejects flag-shaped names), argv arrays (no shell injection), --ignore-scripts on npm ci. Nits: analyzer types are duplicated rather than imported (documented lockstep cost); the event-capture retry is tightly gated so it can't mask a real correlation/ordering failure.
Contributor
Selective E2E Results — ✅ All requested jobs passedRun: 27306425374
|
This was referenced Jun 10, 2026
This was referenced Jun 18, 2026
21 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
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.
OpenClaw TUI chat-correlation coverage slice
Refs: #4347, #2603, #3145, #5098, #5126
This PR adds a focused free-standing Vitest live coverage slice for the OpenClaw TUI chat-correlation websocket regression guard.
Current scope
This is not a full legacy-migration closeout. It keeps
test/e2e/test-openclaw-tui-chat-correlation.shand the retained nightly lane in place.Adds:
test/e2e-scenario/live/openclaw-tui-chat-correlation.test.tsopenclaw-tui-chat-correlation-vitestjob in.github/workflows/e2e-vitest-scenarios.yamlThe live test exercises a real cloud OpenClaw sandbox via Vitest fixture support and asserts the protocol/history subset of #2603/#3145:
Out of scope:
Simplicity check
Validation
Local:
npx tsc --noEmit --allowImportingTsExtensions --moduleResolution bundler --module esnext --target es2022 --types node,vitest/globals --skipLibCheck test/e2e-scenario/live/openclaw-tui-chat-correlation.test.tsnpx prettier --check .github/workflows/e2e-vitest-scenarios.yaml test/e2e-scenario/live/openclaw-tui-chat-correlation.test.tsnpx vitest run --project e2e-vitest-support test/e2e-scenario/support-tests/e2e-scenarios-workflow.test.ts --reporter=dotLive local run was not completed because Docker is not running locally; CI/manual dispatch should provide live sandbox evidence.
Summary by CodeRabbit