fix(e2e): retry transient OpenClaw TUI lookup - #9214
Conversation
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com>
|
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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughChangesTUI stdin retry
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This localized E2E verifier change adds bounded retries for transient TUI disappearance while preserving fatal handling for stable errors; no actionable merge-blocking risk remains after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant LaunchAgentTurn
participant FakeOpenShell
participant TransientTUI
participant ReplacementTUI
LaunchAgentTurn->>TransientTUI: Resolve stdin path
TransientTUI-->>LaunchAgentTurn: Exit 1 after stdin closure
LaunchAgentTurn->>FakeOpenShell: Report first command exit
FakeOpenShell->>ReplacementTUI: Create retry marker and start replacement
ReplacementTUI-->>FakeOpenShell: Accept retry command
FakeOpenShell-->>LaunchAgentTurn: Return successful retry
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
PR Review Advisor — No blocking findings reportedAdvisor assessment: No blocking advisor findings reported Model lanes
Second-opinion terminology and E2E selections are advisory. Live E2E does not run automatically for pull requests. 1 semantic terminology decisionTerminology 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 for the commit under review. Recommended E2E: None Manual-only E2E: 1 optional E2E recommendation
This automated review informs maintainers. Warnings and suggestions do not require a response. A maintainer decides whether to merge. |
<!-- markdownlint-disable MD041 --> ## Summary Record the exact PTY used by the OpenClaw TUI from inside the same sandbox execution process, so the live launch fixture submits input only after that PTY enters noncanonical mode. Structured session JSONL remains the sole success condition; the PTY record authorizes test input timing only. This PR changes internal E2E test and evidence behavior, not a production path or supported product behavior. ## Related Issue Relates to #9200. Preserves the launch-turn acceptance contract established by #9160, #9213, and #9214. ## Ownership PR #9250 owns the OpenClaw PTY slice because the launch process records its own fd 0 PTY identity and the verifier authenticates that direct `/dev/pts/<n>` path, avoiding the `PR_SET_DUMPABLE=0` sibling-process `/proc` boundary. PR #9251 removed its alternate PTY implementation before merge, so the two merged scopes do not overlap. ## Changes - Route only the exact target OpenShell `sandbox exec --tty --timeout 0` invocation for `bash -lc "openclaw tui"` through a private host shim; pass unrelated calls through to the pinned OpenShell binary unchanged and reject malformed or duplicate launch interception. - Publish the launch process's own fd 0 PTY identity atomically before `execve`, using a task-owned mode-0700 directory and a mode-0600 record bound to the run ID and device identity. - Qualify noncanonical input mode through the recorded `/dev/pts/<n>` device without scanning TUI processes or dereferencing another process's `/proc/<pid>/fd/0`. - Keep missing-record and canonical-mode states pending, while malformed metadata, permission errors, device drift, non-PTY input, and termios failures remain fatal. - Clean up only authenticated exact entries, retain structured session JSONL as the sole success condition, and keep both input submissions one-time and ordered. - Resolve empty and relative `TMPDIR` values to an absolute host test-temporary root before the shim invokes host `mktemp`, preserving the absolute-path authority check. - Add deterministic support coverage for exact dispatch, pass-through, absolute OpenShell command authority, host temporary-root normalization, record metadata, pending and fatal classifications, cleanup and residue, terminal-copy independence, one-time input, and structured-session rejection cases. Source evidence: automatic main E2E run `31935105333`, job `95137250015`, artifact `9260707493`, digest `sha256:4148d837372981f08a4a6b53aaa47cded5ef5b33f6b7e2d209dc18979d2c3803`. The status-1 failure does not distinguish a missing record, PTY churn, or persistent canonical mode; this change preserves those distinct structured diagnostics. ## 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: the diff changes only the internal live-E2E driver and deterministic support fixtures; no public command, configuration, default, supported workflow, or product behavior changes. - [x] Sensitive paths changed (security, policy, credentials, preflight, onboarding, inference, runner, sandbox, or messaging) - [x] Sensitive-path review completed or maintainer-approved waiver recorded — reviewer/approval link/justification: review of the commit under review, `0debf4b3e`, found no path-traversal, command-authority, symlink, ownership, permission, race, cleanup, secret-handling, or denial-of-service regression. The new normalization is applied before the existing absolute-path check and private-directory creation contract. - [ ] 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: `test/e2e/live/launch-agent-turn.ts` and `test/e2e/support/launch-agent-turn.test.ts` change internal OpenClaw E2E PTY evidence and deterministic support coverage only. The reviewed patch normalizes empty or relative `TMPDIR` input to an absolute host test-temporary root. It changes no public CLI, configuration, default, supported workflow, or user-facing product behavior. - Agent: `Codex Desktop` <!-- docs-review-head-sha: 0debf4b --> <!-- docs-review-agents-blob-sha: e30afb2 --> ## 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 published 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 — commit `0debf4b3e` passed the focused macOS e2e-support suite with 8 tests and 20 intentional Linux-only skips. - [x] Applicable broad gate passed — GitHub CI for latest PR commit `0debf4b3e` passed all 12 CLI shards, aggregate `cli-tests`, final `checks`, Linux launch support coverage, CodeQL, and Security Code Scanning. - [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) Local commit-bound checks passed for commit under review `0debf4b3e`: focused macOS e2e-support, Oxfmt, Oxlint, CLI build and typecheck, repository checks, source-shape, test-size, conditional scan, `git diff --check`, and `npm run validate:pr`. The complete base-to-head diff is two files. --- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> ## Test evidence summary - Added internal E2E coverage for terminal sessions, exact launch dispatch, malformed or duplicate interception, delayed PTY setup, authenticated cleanup, bounded timeouts, relative OpenShell rejection, host temporary-root normalization, and structured-session rejection. - No production feature, public command, configuration, default, or release behavior changes. --------- Signed-off-by: Senthil Ravichandran <senthilr@nvidia.com> Signed-off-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <prekshiv@nvidia.com> Co-authored-by: Prekshi Vyas <34834085+prekshivyas@users.noreply.github.com>
Summary
The live OpenClaw E2E verifier now retries when its single matching TUI process disappears while descriptor 0 is being resolved. Stable input errors remain fatal, and the regression test proves that input reaches only the replacement TUI.
Related Issue
Related to #9160. Ownership for main run
31892326555, job95031519568, is recorded in the claim comment.Changes
ENOENTandESRCHfrom the single matching TUI descriptor-0 lookup as pending; keep multiple processes, stable non-PTY input, permission or unknown lookup errors, terminal-mode errors, and fatal structured evidence fatal.Atomics.waitwhile preserving its one-second liveness deadline.Type of Change
Quality Gates
9291e388dwith no findings; retryable errors remain bounded and cannot qualify without structured session evidence.Documentation Writer Review
no-docs-needed9291e388d, the complete diff changes onlytest/e2e/live/launch-agent-turn.tsandtest/e2e/support/launch-agent-turn.test.ts; the follow-up replaces a test-only cleanup-poll child process with a bounded in-process wait and does not change supported product behavior or a user-visible surface./root/openclaw_docs_review)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 unavailablee2e-supportfocused file: 19/19 passed; macOS: 6 passed and 13 Linux-only skipped. Archive SHA-256:93a22b850fddca0eeea5ecaaf044893f002283c73a597ce23384dfdbf952a20b; Linux JSON SHA-256:e2ac8d5a739340fe39f3685fc5f6b58631eb5cfcc8d9b889f20f87b9d12a7658.npm testfor broad runtime/test-harness changes;npm run checkfor repo-wide validation/coverage changes — not applicable to this two-file E2E-support correction; CLI typecheck, Oxfmt, Oxlint, repository checks,git diff --check, andnpm run validate:prpassed.npm run docsbuilds without warnings (doc changes only)Independent code review passed with no findings for
9291e388d. Independent security review passed all nine categories with no findings.Signed-off-by: Senthil Ravichandran senthilr@nvidia.com
Summary by CodeRabbit
Bug Fixes
Tests