broker: make detect_cli_ready grid-aware#872
Closed
willwashburn wants to merge 8 commits into
Closed
Conversation
Port the wait taxonomy from montanaflynn/headless-terminal's `internal/wait/wait.go` (#800) into a new `src/wait.rs` module: - `WaitCondition` enum: Text (substring or regex), Idle, Change, Exit, Cursor (kept as a stub — see issue body — until the VT100 grid lands). - `WaitSet` AND-composes multiple conditions; chaining builders (`text`, `text_regex`, `idle`, `change`, `exit`, `cursor`). - `WaitState` streaming evaluator with `feed`/`feed_raw`/`mark_exited` and a `check`/`is_ready` query. Trigger priority mirrors ht (Exit > Text > Cursor > Idle > Change). - `WaitSet::evaluate(&WaitSnapshot)` for synchronous callers that already hold accumulated output and timing. - Per-CLI pre-built wait sets in `wait::for_cli` (claude / gemini / codex / generic + dispatch) so new readiness call sites have a declarative replacement for the per-CLI substring rules in `helpers::detect_cli_ready`. Rolling `detect_cli_ready` itself onto these builders is a behavior-preserving follow-up. 17 unit tests cover each primitive, AND composition, idle reset on chunk, regex compile errors, ANSI-stripping feed, trigger priority, and the per-CLI builders (including claude's onboarding-menu false positive). Full broker suite: 336 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Treat ht as inspiration rather than a hard reference — no need to peg the module to upstream's naming or evolution. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the per-CLI substring/regex blocks inside helpers::detect_cli_ready with calls into the shared wait::for_cli builders. The per-CLI text rules now live in exactly one place — src/wait.rs — and detect_cli_ready becomes a thin polling-style caller that constructs windowed snapshots, dispatches to the right WaitSet, and keeps the three pieces that don't fit the AND-composed primitive inline: the explicit `->pty:ready` signal, gemini's "waiting for auth" negative check, and the byte-count startup fallback for non-claude / non-gemini CLIs. Side changes: - `wait::for_cli::*()` builders are now text-only (no implicit `.idle(IDLE_SETTLE)`), so they compose cleanly with both polling callers and streaming callers. Streaming callers can chain `.idle(...)` directly or use the new `wait::for_cli::with_settle(...)` convenience. - Codex / generic text rules use substring-style regex alternation (`(> |\$ |...|❯)`) to preserve detect_cli_ready's historical loose substring matching exactly. All 17 detect_cli_ready tests pass unchanged. Full broker suite: 337 passed (was 336 before this PR; +1 wait::for_cli::with_settle test). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace "tracked separately" with the actual issue number now that I have it. No behavior change. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR implements grid-aware CLI readiness detection by introducing a composable wait-condition system ( ChangesGrid-Aware CLI Readiness Detection
Sequence DiagramsequenceDiagram
participant PtyWorker as PTY Worker
participant PtySession as PtySession (screen/cursor)
participant GridSnapshot as GridReadinessSnapshot
participant Detection as Detection Function
participant WaitSnapshot as WaitSnapshot
participant CliPreset as for_cli Preset
participant Result
PtyWorker->>PtySession: capture screen_text()
PtyWorker->>PtySession: capture cursor_position()
PtyWorker->>GridSnapshot: build snapshot(screen, cursor)
PtyWorker->>Detection: detect_cli_ready_with_grid(cli, grid)
Detection->>WaitSnapshot: builder from grid
Detection->>CliPreset: dispatch preset (Claude/Gemini/etc.)
CliPreset->>WaitSnapshot: evaluate conditions (text/cursor)
WaitSnapshot->>Result: return satisfied / not satisfied
Detection->>Result: return readiness bool
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 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/wait.rs`:
- Around line 174-188: The text_only constructor for WaitSnapshot sets idle_for
= Duration::MAX and change_seen = true which silently satisfies settle/burst
conditions (affecting with_settle(...) and burst_then_idle(...)) for no-grid
callers; change text_only (or replace it with a new constructor) so these
timing/change fields default to an unsatisfied state (e.g., idle_for =
Duration::ZERO or a sentinel representing "not idling" and change_seen = false)
or require callers to supply explicit timing/change state, and update references
to WaitSnapshot::text_only and any tests using it so WaitState semantics remain
consistent for offline/test fallback paths.
- Around line 160-172: WaitSnapshot currently only exposes flattened screen text
and cursor pos (screen, cursor) which can't verify a prompt at the live cell;
add a minimal cell-aware view to the snapshot (e.g. a reference field like grid:
&'a Grid or cells: &'a [CellRow]/&'a [Cell] that exposes per-cell content and
attributes) and update WaitSet::evaluate to populate this new field so callers
can inspect the exact cell under the cursor when checking for prompts rather
than relying on regexes over screen; keep existing fields (screen, cursor,
idle_for, change_seen, exited) and ensure the new field is a lightweight
borrowed reference (no cloning) so wait logic remains grid-backed.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 45f53e25-441c-48e9-bcc5-057486e322e4
📒 Files selected for processing (4)
src/helpers.rssrc/main.rssrc/pty_worker.rssrc/wait.rs
Comment on lines
+160
to
+172
| /// Snapshot of the world used by [`WaitSet::evaluate`]. | ||
| pub struct WaitSnapshot<'a> { | ||
| /// ANSI-stripped screen text accumulated so far. | ||
| pub screen: &'a str, | ||
| /// How long since the last output chunk arrived. Used by `Idle`. | ||
| pub idle_for: Duration, | ||
| /// Whether any output chunk has been observed. | ||
| pub change_seen: bool, | ||
| /// Whether the process has exited. | ||
| pub exited: bool, | ||
| /// Current 1-indexed cursor position from the PTY grid, if known. | ||
| pub cursor: Option<(u16, u16)>, | ||
| } |
Contributor
There was a problem hiding this comment.
WaitSnapshot still can't express cell-aware prompt checks.
screen plus (row, col) is not enough to verify that the live cursor is sitting on a bare Claude prompt in the visible grid. As written, the CLI preset still has to fall back to regexes over flattened text, so a standalone ❯ / > line elsewhere on screen can satisfy readiness even when the input cursor is not on that row. Please carry a minimal grid/cell view through the snapshot so the wait layer can keep Claude fully grid-backed instead of pushing that logic back into ad hoc helpers.
🤖 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/wait.rs` around lines 160 - 172, WaitSnapshot currently only exposes
flattened screen text and cursor pos (screen, cursor) which can't verify a
prompt at the live cell; add a minimal cell-aware view to the snapshot (e.g. a
reference field like grid: &'a Grid or cells: &'a [CellRow]/&'a [Cell] that
exposes per-cell content and attributes) and update WaitSet::evaluate to
populate this new field so callers can inspect the exact cell under the cursor
when checking for prompts rather than relying on regexes over screen; keep
existing fields (screen, cursor, idle_for, change_seen, exited) and ensure the
new field is a lightweight borrowed reference (no cloning) so wait logic remains
grid-backed.
Comment on lines
+174
to
+188
| impl<'a> WaitSnapshot<'a> { | ||
| /// Build a text-only snapshot for callers that do not have a VT grid. | ||
| /// | ||
| /// Idle/Change are treated as satisfied so only `Text` conditions | ||
| /// participate. `Cursor` conditions remain unsatisfied because no | ||
| /// cursor position is available. | ||
| pub fn text_only(screen: &'a str) -> Self { | ||
| Self { | ||
| screen, | ||
| idle_for: Duration::MAX, | ||
| change_seen: true, | ||
| exited: false, | ||
| cursor: None, | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
text_only() silently disables settle/burst semantics.
This constructor hardcodes idle_for = Duration::MAX and change_seen = true, so with_settle(...) and burst_then_idle(...) become immediately satisfied for no-grid callers. That makes snapshot evaluation behave differently from WaitState exactly on the offline/test fallback path this PR keeps around. Please default these fields to unsatisfied, or require callers to pass explicit timing/change state when they want snapshot evaluation.
🤖 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/wait.rs` around lines 174 - 188, The text_only constructor for
WaitSnapshot sets idle_for = Duration::MAX and change_seen = true which silently
satisfies settle/burst conditions (affecting with_settle(...) and
burst_then_idle(...)) for no-grid callers; change text_only (or replace it with
a new constructor) so these timing/change fields default to an unsatisfied state
(e.g., idle_for = Duration::ZERO or a sentinel representing "not idling" and
change_seen = false) or require callers to supply explicit timing/change state,
and update references to WaitSnapshot::text_only and any tests using it so
WaitState semantics remain consistent for offline/test fallback paths.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Member
Author
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
2 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
Stacked on #837. Depends on #837 landing first.
Fixes #868.
PtySession::screen_text()andPtySession::cursor_position()Testing
cargo fmtcargo checkcargo test detect_cli_readycargo test startup_gatecargo test