Skip to content

fix(test): wait for the startup composer before pointer assertions - #5979

Open
Hmbown wants to merge 1 commit into
mainfrom
fix/pty-startup-readiness-20260907
Open

fix(test): wait for the startup composer before pointer assertions#5979
Hmbown wants to merge 1 commit into
mainfrom
fix/pty-startup-readiness-20260907

Conversation

@Hmbown

@Hmbown Hmbown commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Test-only fix for the Buildkite/Linux PTY flake (#1861/#1867 died asserting mid-redraw: the launch header arrives before the composer, with onboarding rows still on screen).

Waits for the launch header + composer + cleared onboarding before asserting the existing startup contract. All startup and pointer-queue assertions preserved — it still catches the failure mode, after the redraw settles.

Audit: verified honest (see codewhale-ops/AUDIT-REPORT-V0913-20260907.md).

Validation: focused all-features cucumber test 1 passed / 0 failed across five PTY sizes; cargo fmt clean. Hosted CI runs on this PR.


Devin Review

Note

Low Risk
Test-only synchronization change in a long-running PTY regression; no production code paths affected.

Overview
Stabilizes the real-PTY active composer pointer cucumber test after onboarding by replacing a single-text wait on "New session" with a frame predicate that also requires the composer prompt () and confirms onboarding copy ("You're ready.") is gone.

This targets Buildkite/Linux flakes where PTY reads split a redraw so the launch header appears before the input surface is painted. assert_startup_contract and the rest of the pointer-submit checks are unchanged—the test just waits for a fully settled startup UI before those assertions run.

Reviewed by Cursor Bugbot for commit 2416013. Bugbot is set up for automated code reviews on this repo. Configure here.

Buildkite 1861 and 1867 failed on Linux with partial launch frames; all cited macOS jobs passed. Wait for the composer and cleared onboarding as well as the launch header, preserving all startup and pointer queue assertions.

Validation: focused all-features cucumber test 1 passed, 0 failed across five PTY sizes (40x12 through 140x40); cargo fmt --all -- --check passed; git diff --check passed. npm test and npm run check:web both unavailable: root package.json defines neither script. Hosted CI not run for this commit.
Signed-off-by: CodeWhale Bot <bot@codewhale.net>
Copilot AI lite review requested due to automatic review settings September 7, 2026 04:48
@cursor

cursor Bot commented Sep 7, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_e1f98906-bf40-4621-a8e6-8f8b5c3217b3)

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 7, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-07T04:51:03.607480Z 2416013 PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@claude

claude Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Claude finished @Hmbown's task in 1m 58s —— View job


Review: PTY startup-readiness wait fix

  • Gather context (PR diff, changed file)
  • Understand the request (code review of test-only flake fix)
  • Review diff for correctness, provider/route safety, reuse, tests, security
  • Post findings

Summary: This is a well-targeted, minimal test-only fix. Verdict: looks good to merge.

crates/tui/tests/cucumber/active_composer_pointer_pty.rs:104-111 replaces a single wait_or_panic(&mut tui, "New session", ...) with a tui.wait_for(...) predicate requiring all three of: "New session" present, '❯' present, and "You're ready." gone, before calling assert_startup_contract.

I checked the '❯' needle isn't ambiguous: it's set exactly once in the renderer, as the composer's own cursor/prompt glyph (crates/tui/src/tui/widgets/mod.rs:1939, buf[(prompt_x, cursor_y)].set_symbol("❯")), not a generic decoration reused elsewhere on the launch screen. So the predicate genuinely detects "composer is painted," matching the stated flake mechanism (header renders before composer, stale onboarding rows still present) rather than papering over it with an arbitrary sleep/retry.

Also confirmed:

  • Harness::wait_for (crates/tui/tests/support/qa_harness/harness.rs:209) takes FnMut(&Frame) -> bool and returns anyhow::Result<()>, so the closure and .unwrap_or_else(|error| panic!("...{error}")) type-check; the anyhow::Error Display includes the frame's debug_dump(), so diagnostic output on failure is preserved (though it's slightly less specific than before about which of the three conditions failed — minor, not a blocker).
  • assert_startup_contract (line 303) still separately asserts "codewhale" and "❯", so the new pre-wait and the existing contract assertion aren't duplicating logic, just sequencing it correctly.
  • No changes to the pointer/queue assertions later in the file — the stated "all startup and pointer-queue assertions preserved" claim holds; this is a pure wait-condition tightening.
  • wait_or_panic remains used elsewhere in the file (lines 86, 94, 155), so it isn't dead code.

No correctness, provider/route-safety, reuse, or security issues found. No new test coverage is needed since this fixes flakiness in an existing regression test rather than adding behavior.

Note on verification: I could not run cargo check/cargo test in this sandbox (cargo invocations required approval that wasn't available), so this review is based on static reading of the diff and the harness/widget source it depends on, plus the PR's own reported validation (cucumber test passing across 5 PTY sizes). The relevant CI checks (CI, Codewhale PR Review) were still queued/in-progress at review time.
· Branch

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

Devin Review

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

The change is a small, test-only synchronization improvement that uses existing harness primitives and does not alter the asserted behavior beyond waiting for the UI to settle.

Pull request overview

Stabilizes the real-PTY cucumber regression active_composer_pointer_submit_queues_without_provider by waiting for a fully-settled startup screen (launch header + composer prompt + onboarding cleared) before running the existing startup and pointer-submit assertions, addressing a redraw-splitting flake observed on Buildkite/Linux PTY.

Changes:

  • Replaces a single-string wait_or_panic(..., "New session", ...) with a Harness::wait_for frame predicate that also requires the composer prompt () and absence of onboarding copy ("You're ready.").
  • Preserves existing assert_startup_contract and downstream pointer/queue assertions, changing only synchronization timing.
File summaries
File Description
crates/tui/tests/cucumber/active_composer_pointer_pty.rs Strengthens startup wait condition to ensure the composer is painted and onboarding text is cleared before asserting startup/pointer behavior.
Review details
  • Files reviewed: 1/1 changed files
  • Comments generated: 0
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@codewhale-agent codewhale-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Codewhale review

Test-only synchronization change in the active composer pointer PTY cucumber test replaces the single launch-header wait with a frame predicate that also waits for the composer prompt and for onboarding copy to clear before existing startup and pointer assertions run.

Assessment

The change is low risk and directly addresses the described PTY redraw race. Existing startup and pointer-queue assertions are preserved, and the strengthened wait condition should prevent the flaky mid-redraw assertion without hiding the original failure mode.


Advisory review by Codewhale (codewhale review --pr 5979 --post, head 2416013a5e3fae5e80f917d549b361d388160672). Line-specific findings are also posted as inline review comments; mechanical fixes arrive as committable suggestions you can apply from the Files tab. CODEOWNERS approval still governs merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants