feat(eko): add quick replies for exec approvals - #53
Conversation
🔎 Lint report:
|
PR #53 Review: `feat(eko): add quick replies for exec approvals`Files changed: 6 | +388 / -1 | Branch: `feat/eko-quick-replies` → `main` SummaryThe PR adds Eko quick-reply support for three interactive prompts:
All three degrade gracefully when no reply token is available (returns `success=False`, gateway falls back to text). Quick-reply taps arrive as ordinary text messages; the gateway's existing text-intercept chain maps them to the right action. What's Good
Issues and Suggestions1. Minor: Clarify + approval race (theoretical, low severity) If a clarify with `awaiting_text=True` and a tool approval are both pending simultaneously, the clarify intercept at line 7104 runs first and would capture "Approve Once" / "Deny" as a clarify response instead of an approval. This is unlikely in practice (the agent thread is blocked on clarify, so it can't call tools that trigger approval), but could surface with concurrent subagents sharing a session key. Consider: Add a guard in the clarify intercept that skips known approval labels when `_tool_approval_live` is true, e.g.: if _raw_clarify_reply and not _raw_clarify_reply.startswith("/"):
# Skip if this looks like a quick-reply approval tap
if _tool_approval_live and _raw_clarify_reply.lower() in {
"approve once", "approve session", "approve always", "deny"
}:
pass # fall through to approval intercept
else:
_resolved = _clarify_mod.resolve_gateway_clarify(...)This is a nice-to-have, not a blocker — the current code is safe for single-agent sessions. 2. Nit: Inconsistent button label casing in `send_slash_confirm` The approval quick-reply uses title case ("Approve Once", "Approve Always"), but slash-confirm uses "Always Approve" (not "Approve Always"). This is consistent with the existing slash-confirm text-intercept recognition set (which checks `"always approve"`), but it's visually inconsistent for a user seeing both types of prompts. No action needed — this follows the existing gateway convention. 3. Nit: `send_exec_approval` returns `message_id=token` return SendResult(success=True, message_id=token)Using the consumed reply token as `message_id` is a reasonable identifier, but it means the "message ID" is a one-time token that's already been consumed. If anything downstream tries to use it for reply-to or editing, it would fail. This matches the pattern used by `send_slash_confirm` and `send_clarify` in this PR, so it's internally consistent. 4. Suggestion: Consider adding a test for the clarify+approval coexistence scenario A test that has both a pending clarify (awaiting_text) and a pending approval, then sends "Approve Once" and verifies it resolves the approval (not the clarify), would lock down the precedence behavior for future changes. VerdictLGTM with one optional suggestion. The implementation is clean, correctly follows existing patterns, degrades gracefully, and has solid test coverage. The clarify+approval race is theoretical and not a blocker. |
🐛 Bug: Quick-reply approval taps are silently queued, never dispatchedReproduced live — after the quick-reply buttons render and the user taps "Approve Once", nothing happens. The agent stays blocked. Root causeThe approval-label mapping intercept was added to Trace
Evidence from logsThe webhook at ✅ Better fix: set
|
| Button display | Current value |
Fixed value |
|---|---|---|
| Approve Once | "Approve Once" |
"/approve" |
| Approve Session | "Approve Session" |
"/approve session" |
| Approve Always | "Approve Always" |
"/approve always" |
| Deny | "Deny" |
"/deny" |
When the tap arrives as /approve, event.get_command() returns "approve" → should_bypass_active_session("approve") → True → existing slash-command bypass in base.py dispatches it directly. No new bypass needed.
Why this is better than adding a base.py bypass:
- Removes code instead of adding it —
_approval_command_for_text_reply()and the intercept block inrun.pybecome unnecessary - No new path in
base.py— already 3600+ lines with 3 bypass paths; a 4th would be fragile - Uses the existing, well-tested command bypass — same path
/approvetyped manually would take - Same approach works for
send_slash_confirm— "Approve Once" →/approve, "Always Approve" →/always, "Cancel" →/cancel(the slash-confirm intercept already matches_cmd_reply == "approve"etc.) - Clarify is unaffected — clarify choices are dynamic (agent-specified), so they stay as plain text, and the clarify bypass in
base.pyalready handles those correctly
One thing to verify first: does Eko deliver value or data.text on tap? The current payload sets both to the same string. Need to confirm which field the platform sends back, then set value to the slash command and keep data.text as the display label.
…t-active queue The previous approach mapped approval labels in gateway/run.py, but plain-text quick-reply taps were silently queued by base.py before reaching the runner. Setting value=/approve etc. makes the tap arrive as a real slash command that bypasses the queue via the existing command-dispatch path. Removes _approval_command_for_text_reply() and its intercept block from gateway/run.py — fewer lines, no new bypass path.
✅ Re-review after fixDiff looks good. The fix is exactly right:
Tests: 208 passed, 0 failed. This is ready to merge. 🚀 |
Summary
Closes #52.
send_exec_approval()using/bot/v1/message/quickreplywhen a fresh reply token is available/approve,/approve session,/approve always, and/denywhile a dangerous-command approval is pendingTests
scripts/run_tests.sh tests/gateway/test_eko_plugin.py tests/gateway/test_approve_deny_commands.py