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 the active manifest supports it, preventing generic startup states from being treated as interactive readiness.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| src/terminal/state.rs | Adds prompt-evidence timestamps and manifest-aware managed-launch reconciliation so positive readiness must postdate launch. |
| src/app/actions.rs | Processes agent-detection events, reconciles managed launches, and suppresses startup transitions from appearing as completed work. |
| src/server/headless.rs | Forwards readiness-relevant pane updates while avoiding misleading startup completion notifications. |
| src/app/api.rs | Publishes managed-agent readiness-only changes to API subscribers. |
| src/pane.rs | Emits explicit process-detection events separately from screen-derived lifecycle state. |
| src/detect/manifests/codex.toml | Classifies Codex’s trust-directory prompt as blocked rather than prompt-ready. |
| tests/cli/agent_transport.rs | Extends transport coverage for delayed positive readiness and blocked startup behavior. |
Sequence Diagram
sequenceDiagram
participant CLI as agent start
participant API as App/API
participant Detector as Agent detector
participant State as TerminalState
CLI->>API: start managed agent
API->>State: begin pending launch
Detector->>State: process detected
Detector->>State: lifecycle state
alt positive prompt evidence or authoritative idle
State->>State: mark launch active
API-->>CLI: "interactive_ready = true"
else blocked
API-->>CLI: agent_not_ready
else timeout
API-->>CLI: timeout
end
Reviews (7): 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 |
3 participants
What changed
herdr agent startnow waits for positive prompt-ready evidence when the active agent manifest provides it. The initial fallbackIdleorBlockedstate no longer makes those launches ready. Agents without a positive idle rule keep the existing fallback behavior, and readiness-only changes are published to API clients.Why
agent startpreviously treated a generic detectedIdleorBlockedstate as equivalent to proof that the agent's normal input prompt was visible after the settle window. Those are not always the same thing. Herdr can identify the process and publish an initial fallback state while the agent is still in setup.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, a managed launch stays pending when the active detector can provide positive prompt evidence. It becomes ready only after that evidence arrives or a full lifecycle hook reports an authoritative idle state. Otherwise the caller's timeout still applies. Prompt evidence observed before the managed launch is also ignored.
Agent coverage
The policy comes from the active agent manifest. There is no hard-coded Codex path. With the current bundled manifests:
amp,claude,codex,devin,grok,hermes,kiro, andmaki. Their detectors already expose positive idle evidence, so generic startupIdleorBlockedno longer counts as readypi,omp,mastracode,opencode,kilo, andkimi. This PR keeps their existing fallback behavior when hook authority is not activegemini,cursor,agy,cline,copilot,droid, andqodercli. They do not currently expose positive idle evidence or full lifecycle authority, so they retain the existing generic readiness fallbackAdding reliable positive readiness for the last group needs per-agent evidence and is outside #2410.
Validation
interactive_readybefore a Codex prompt was visible on Windowscargo test --bin herdr managed_agent_ -j 2just 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