Repository navigation
Conversation
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I reviewed this pull request and requested human review from: Comment Powered by Oz |
There was a problem hiding this comment.
Overview
This PR routes Hermes structured CLI-agent events through the existing default session listener and derives the support gate from handler availability to prevent future table drift. The added regression exercises the Hermes parse path, support gate, prompt_submit forwarding, and stop forwarding.
Concerns
- No blocking concerns found.
- No approved spec context was provided, so there were no spec commitments to compare against.
- Security pass: no new input trust boundary, secret handling, auth, or injection concern was introduced by this listener routing change.
Verdict
Found: 0 critical, 0 important, 0 suggestions
Approve
Comment /warp-agent-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
607e718 to
b9648af
Compare
|
Thanks and sorry for the delay! |
Hermes is already a recognized CLIAgent and structured warp://cli-agent OSC 777 payloads with "agent":"hermes" parse to CLIAgent::Hermes, but create_handler() mapped Hermes to None and is_agent_supported() did not list it, so TerminalView::handle_cli_agent_notification() dropped every Hermes event before registering a listener. Hermes sessions therefore never got tab status badges or notification mailbox entries. Route Hermes to DefaultSessionListener, the same listen-only path used by Droid/Pi/OhMyPi/Auggie, and derive is_agent_supported() from create_handler() so the two tables can no longer drift apart. The regression test parses the exact payloads from warpdotdev#15666, asserts the parsed agent passes the is_agent_supported gate, and checks that the Hermes handler forwards prompt_submit and stop. Fixes warpdotdev#15666
b9648af to
be0cd5d
Compare
|
Thanks for the review, @bnavetta — no worries at all! Rebased onto current master to clear the conflict with #15999 (Muse support), which touched the same two spots in The earlier Windows failure was |
Description
Hermes is already a recognized
CLIAgent(command prefixhermes, display name, brand color, skill providers), and structuredwarp://cli-agentOSC 777 payloads with"agent":"hermes"already parse toCLIAgent::Hermes. Butcreate_handler()inapp/src/terminal/cli_agent_sessions/listener/mod.rsrouted Hermes toNoneandis_agent_supported()didn't list it, soTerminalView::handle_cli_agent_notification()dropped every Hermes event at the support gate — no tab status badge, no notification mailbox entry.This PR:
CLIAgent::HermestoDefaultSessionListener, the same listen-only path Droid / Pi / OhMyPi / Auggie use (no install flow — we just listen, per the approach discussed on Add Hermes CLI-agent notification support #10001).is_agent_supported()fromcreate_handler(agent).is_some()so the two tables can't drift apart again. The previous hand-maintained list matchedcreate_handler'sSomearms exactly, so no other agent's behavior changes.This builds on @DarkGenius's earlier PR #15665 (auto-closed by the stale bot, not rejected), and incorporates the review feedback left there: the support check is now derived from the handler table, and the regression test goes through the
is_agent_supportedgate instead of bypassing it. Thanks @DarkGenius for the original report and fix.Linked Issue
Closes #15666
ready-to-specorready-to-implement.Testing
Added
hermes_structured_events_pass_support_gate_and_are_forwarded(listenermod_tests.rs): parses the exactprompt_submit/stoppayloads from #15666 viaparse_event, asserts the resolved agent isHermesand passesis_agent_supported, then checks the Hermes handler forwards both events. It fails onmasterat theis_agent_supportedassertion and passes with this change.cargo nextest run --no-fail-fast -p warp --lib cli_agent— 240/240 passed./script/format --check— cleancargo clippy --workspace --all-targets --tests -- -D warnings— cleancargo clippy -p warp --all-targets --tests -- -D warnings— cleanI have manually tested my changes locally with
./script/runScreenshots / Videos
Local OSS build of this branch (
./script/run). The repro sends the two payloads from the issue from one long-lived block (the CLI agent session is tied to the running block, so sending them from two separate commands ends the session in between):Before (
master): both events are dropped at the support gate, so the tab keeps its plain shell state and nothing reaches the notification mailbox — the behavior described in #15666.After —
prompt_submit: the tab shows the Hermes badge in the in-progress state.After —
stop: the badge switches to success, the tab gets an unread dot, and the mailbox icon shows a new notification.After — mailbox: the notification panel lists "Hermes completed — Task completed."
Agent Mode