fix: wait for agent prompt readiness - #2537
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:
📝 WalkthroughWalkthroughAgent detection now initializes new agents as unknown and emits a dedicated process-detection event. Codex trust prompts enter a blocked state. Managed launches remain pending until idle detection. CLI startup returns ChangesManaged-agent blocked startup
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Detector
participant AppEvent
participant TerminalState
participant PaneState
participant AgentCLI
Detector->>AppEvent: publish AgentProcessDetected
AppEvent->>TerminalState: begin process acquisition
Detector->>TerminalState: report blocked state
TerminalState->>PaneState: retain pending launch and suppress initial completion
AgentCLI->>TerminalState: poll startup state
TerminalState-->>AgentCLI: return agent_not_ready
Detector->>TerminalState: report matching idle state
TerminalState-->>AgentCLI: report interactive readiness
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
kangal-bot
added
the
ai-review
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/terminal/state.rs (1)
1816-1945: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winActivation gating correctly separates fallback agents from positive-idle-rule agents.
begin_managed_agentseedsprompt_ready/allow_blockedfromrequire_visible_idle, andreconcile_managed_agent_atrequiresprompt_ready(or live full-lifecycle hook authority) for Idle activation, andallow_blockedfor Blocked activation. This matches the objective that generic initialIdle/Blockedstates no longer indicate readiness for agents with a positive idle rule, while agents without one keep existing fallback behavior.Consider adding a short doc comment above
ManagedAgentPhase::Pendingexplaining whatprompt_readyandallow_blockedrepresent and howrequire_visible_idleseeds them, since the interaction between these two flags and the manifest rule is not obvious from the field names alone.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 42cbfeaa-aa3d-4d81-9c2c-1581a4c35972
📒 Files selected for processing (12)
src/app/actions.rssrc/app/agents.rssrc/app/api.rssrc/app/api/agents.rssrc/app/input/mouse.rssrc/app/mod.rssrc/detect/manifest.rssrc/events.rssrc/pane.rssrc/persist/snapshot.rssrc/server/headless.rssrc/terminal/state.rs
1996ced to
f09eeac
Compare
Sure, why not, added the comment in f09eeac |
Greptile SummaryThis PR makes managed agent startup wait for positive prompt-ready evidence when supported, while preserving fallback behavior for agents without such evidence.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/terminal/state.rs | Introduces blocked managed-launch state, positive-evidence readiness reconciliation, and acquisition-completion suppression. |
| src/pane.rs | Publishes process acquisition separately from screen-confirmed state and initializes new detections as unknown. |
| src/app/actions.rs | Reconciles managed readiness on state updates and suppresses startup idle transitions from completion effects. |
| src/server/headless.rs | Propagates completion suppression through headless notification forwarding. |
| src/cli/agent.rs | Waits through unknown or working startup states and returns agent_not_ready for blocked launches. |
| src/detect/manifests/codex.toml | Adds top-region detection for Codex's trust-directory prompt and advances the manifest engine requirement. |
| tests/cli/agent_transport.rs | Updates transport coverage for unknown, blocked, and ready startup responses. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Agent process detected] --> B[Record Unknown acquisition state]
B --> C{Positive prompt evidence available?}
C -->|Yes| D[Wait for visible idle evidence]
C -->|No| E[Use existing fallback readiness]
D --> F{Detected state}
F -->|Blocked| G[Return agent_not_ready]
F -->|Idle after settle| H[Mark interactive ready]
F -->|Working or Unknown| I[Remain launch pending]
E --> H
H --> J[Publish readiness update]
G --> K[Retain agent name for read and send-keys]
Reviews (11): Last reviewed commit: "Merge branch 'master' into issue/2410-ag..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
There was a problem hiding this comment.
i feel like a better approach would be to keep startup readiness based on the detection engine’s public state instead of making agent start inspect manifest details.
if detection reports blocked, startup should never set interactive_ready: true. ideally it should return immediately with a blocked/not-ready result rather than waiting for the 30-second timeout (we can also lower it to 10 imo 30 feels long). idle succeeds, while unknown keeps waiting.
if the codex trust screen is not reliably classified as blocked—or falls back to idle—that seems like a detection problem we should fix there. this would be smaller and keep visible_idle as detector evidence rather than turning it into startup policy.
91f50ef to
b42fd4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/server/headless.rs (1)
11064-11068: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider covering
handle_internal_event_with_pane_updatesin the bypass guard test.The guard test only matches
self.app.handle_internal_event(andself.app.handle_internal_event_with_prefix_sync(. The forwarding method now also usesself.app.handle_internal_event_with_pane_updates(. A future direct call to that method outsidehandle_internal_event_with_forwardingwould bypass forwarding without failing this test.♻️ Proposed guard extension
} else if (line.contains("self.app.handle_internal_event(") - || line.contains("self.app.handle_internal_event_with_prefix_sync(")) + || line.contains("self.app.handle_internal_event_with_prefix_sync(") + || line.contains("self.app.handle_internal_event_with_pane_updates(")) && !line.trim().starts_with("///") && !line.contains("contains(")tests/live_handoff.rs (1)
1477-1497: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winBound the marker-file wait loops in the helper scripts.
Both scripts spin on
while [ ! -f ... ]; do sleep 0.01; donewith no upper bound. If the counterpart process never creates its marker, for example whenserver.live_handofffails before spawning the importer, the script spins until the test harness terminates it. A bounded loop turns that case into a fast, diagnosable failure instead of a busy spin.♻️ Proposed bounded waits
fs::write( &fake_codex, "#!/bin/sh\n\ exec env HERDR_AGENT=codex /bin/sh -c '\n\ printf \"\\033[2J\\033[H> You are in /tmp\\n\\n Do you trust the contents of this directory?\\n\"\n\ touch \"$HERDR_TEST_AGENT_STARTED\"\n\ - while [ ! -f \"$HERDR_TEST_IMPORTER_STARTED\" ]; do sleep 0.01; done\n\ + i=0\n\ + while [ ! -f \"$HERDR_TEST_IMPORTER_STARTED\" ] && [ \"$i\" -lt 3000 ]; do sleep 0.01; i=$((i+1)); done\n\ touch \"$HERDR_TEST_AGENT_EXITED\"\n\ '\n", ) .unwrap(); fs::write( &import_wrapper, "#!/bin/sh\n\ touch \"$HERDR_TEST_IMPORTER_STARTED\"\n\ - while [ ! -f \"$HERDR_TEST_AGENT_EXITED\" ]; do sleep 0.01; done\n\ + i=0\n\ + while [ ! -f \"$HERDR_TEST_AGENT_EXITED\" ] && [ \"$i\" -lt 3000 ]; do sleep 0.01; i=$((i+1)); done\n\ exec \"$HERDR_TEST_HERDR_EXE\" \"$@\"\n", ) .unwrap();
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ed86ff94-d654-4ecc-af8b-ed300f76d16c
📒 Files selected for processing (23)
docs/next/website/src/content/docs/agent-automation.mdxdocs/next/website/src/content/docs/cli-reference.mdxdocs/next/website/src/content/docs/ja/agent-automation.mdxdocs/next/website/src/content/docs/ja/cli-reference.mdxdocs/next/website/src/content/docs/zh-cn/agent-automation.mdxdocs/next/website/src/content/docs/zh-cn/cli-reference.mdxsrc/app/actions.rssrc/app/agent_resume.rssrc/app/agents.rssrc/app/api.rssrc/cli/agent.rssrc/detect/manifest/tests.rssrc/detect/manifests/codex.tomlsrc/events.rssrc/pane.rssrc/persist/restore.rssrc/persist/snapshot.rssrc/server/headless.rssrc/terminal/runtime.rssrc/terminal/state.rstests/cli/agent_transport.rstests/live_handoff.rswebsite/agent-detection/codex.toml
e471cab to
4bfd61c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: df92ee09-234d-46b0-a32c-2fb31078f049
📒 Files selected for processing (4)
src/app/actions.rssrc/events.rssrc/pane.rssrc/terminal/state.rs
4bfd61c to
28c4a9b
Compare
|
Thanks, yep agreed. I reworked this around the detector's public state. The race was earlier than startup policy. Process detection published a temporary Idle, then skipped screen scans for the same three seconds used as the startup settle window. Startup could therefore complete before detection had classified the actual screen. When Herdr sees the process now, it publishes the agent identity as Unknown during that grace period. Idle succeeds, Blocked returns The first live Idle after process acquisition means ready, not completed work. A process exit still counts as completion, and later Unknown to Idle transitions for the same known agent keep their existing completion behavior. The same suppression now reaches the headless notification path, so sound and toast forwarding follow the same rule. I also reproduced the Codex trust screen, including the compact seven-row layout. The detector now anchors on the live header and question, handles wrapping, and does not match trust text copied into a conversation. This removes the manifest introspection and I also looked at 30s vs 10s. We do not currently record enough startup timing data to have super strong data. The detector already spends a fixed three seconds in acquisition grace, so 10 seconds leaves roughly seven seconds for the agent itself to start and reach a known state. So to me 10s sounds a bit too tight. Now that Blocked returns immediately, the 30-second wait only applies while detection is still Working or Unknown. Callers that want faster failure can already use |
|
ty! |
kangal-bot
removed
the
ai-review
3 participants
What changed
herdr agent startnow follows the detector's public state instead of treating process recognition as readiness. A newly recognized agent starts asUnknownduring detection acquisition.Idlecompletes startup,Blockedreturnsagent_not_readyimmediately, andWorkingorUnknownkeep waiting until the caller's timeout.A blocked managed agent keeps its name, so callers can inspect it with
agent readand answer setup prompts withagent send-keys. If detection later reportsIdle, the existing named agent becomes ready foragent prompt.Codex detection now classifies the live
Trust this directoryscreen asBlocked, including the compact wrapped layout, without matching copied trust text in conversation history. The firstIdleafter process acquisition is treated as startup readiness rather than completed work, including in headless notification forwarding.Why
agent startpreviously treated a detectedIdleorBlockedstate as equivalent to proof that the agent's normal input prompt was visible after the settle window. Process detection also published a temporaryIdleand skipped screen scans for the same three-second acquisition window, so startup could complete before Herdr classified the actual screen.The concrete #2410 reproduction is Codex showing its first-run
Trust this directoryprompt. Herdr returnedinteractive_ready: truebefore the normal Codex prompt was visible, so an immediateagent promptcould be sent before Codex was ready.With this change, the trust screen returns
agent_not_readyinstead. Once trust is accepted and Codex reaches its normalIdleprompt, the named agent becomes ready for prompts.Agent coverage
The startup policy is agent-independent:
Idleis ready,Blockedis not ready, andWorkingorUnknowncontinue waiting. Accurate readiness still depends on each agent's detector classifying its live screen correctly. This PR adds the missing Codex trust-screen rule required for #2410.Validation
Unknown,Blocked, and laterIdletransitionsagent_not_readyjust checkwith Rust tests serializedThe Windows media-player test is quite flaky in the parallel batch. It passed alone and in the serialized full check. Might be worth a follow-up fix eventually.
refs #2410