feat(chat): interactive button contract + workflow trigger - #76
Conversation
Expose the public API button v3 fields on chat send/update (JSON passthrough, approval/workflow ergonomic flags, light pre-validation) and add dailybot workflow trigger for api_trigger automations. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
## Summary Resolve the five live review comments on #76 before re-running the gate. ## Change Log - Annotate auth_hint / rate_msg locals (Rule 2) - Code-less 401/403 now exit EXIT_NOT_AUTHENTICATED / EXIT_PERMISSION_DENIED - Prefer env-var wording on --callback-bearer help - Wire API_TRIGGER_TYPE into --filter help + empty-filter tip - Measure trigger payload size with httpx-compatible JSON serialization Co-authored-by: Cursor <cursoragent@cursor.com>
xergioalex
added
Ready
AI review for
|
There was a problem hiding this comment.
Verdict
Solid, well-tested feature — no blocking issues. A few low-severity consistency/UX notes; nothing that should hold up merge.
The interactive-button contract v3 and workflow trigger land cleanly: callbacks stay thin (parse → resolve → client → display), every client.* call is wrapped in try/except APIError, error handling dispatches on the machine-readable code field (not detail), new error codes are mapped in ERROR_CODE_MESSAGES with an alignment test, all HTTP is mocked in tests, magic numbers are extracted to module-level constants, and user-facing strings consistently use "Dailybot". Credential hygiene is good — the --callback-bearer token is never echoed (success --json emits the server response, not the payload) and the help text steers users toward $TOKEN env indirection.
Findings table
| # | Severity | File | Summary |
|---|---|---|---|
| 1 | ℹ️ info | dailybot_cli/commands/chat.py:354 |
Code-less 429 exits 1 while a coded 429 routes to exit_for_api_error (exit 6) — inconsistent with the 401/403 alignment done just above |
| 2 | ℹ️ info | dailybot_cli/commands/workflow.py:206 |
Size-guard comment claims it matches httpx's json= serialization, but httpx uses compact separators=(",", ":") while this uses default (spaced) separators |
| 3 | ℹ️ info | dailybot_cli/commands/workflow.py:131 |
--filter runs client-side on only the fetched page(s); the pagination footer still reports the unfiltered server count/next, which can mislead when --all isn't passed |
| 4 | ℹ️ info | dailybot_cli/commands/chat.py:450 |
Praise — thoughtful credential-hygiene guidance on --callback-bearer |
Cross-cutting notes
- Exit-code change (intentional, tests updated): code-less 401/403 chat errors now exit 3/4 (via
EXIT_NOT_AUTHENTICATED/EXIT_PERMISSION_DENIED) instead of the previous1. This is a headless-contract change; it's consistent withexit_for_api_errorand the updated tests assert it, so it looks deliberate — just calling it out for release notes. - Auth resolution order (AGENTS.md Rule 14) is untouched;
trigger_workflowreuses_request/_handle_responseexactly likeget_workflow. No credential-file writes, no env.json changes, all HTTP mocked athttpx.*. Good.
Recommendation: approve
| EXIT_NOT_AUTHENTICATED if e.status_code == 401 else EXIT_PERMISSION_DENIED | ||
| ) | ||
| raise SystemExit(exit_code) | ||
| if e.status_code == 429 and e.code is None: |
There was a problem hiding this comment.
The comment above the 401/403 block says it matches exit_for_api_error's status→exit-code contract "whether or not the server sent code". This 429 branch breaks that symmetry: a code-less 429 exits 1, but a 429 that carries any code falls through to exit_for_api_error(...), which maps 429 → EXIT_RATE_LIMITED (6). So headless consumers get a different exit code for the same HTTP status depending on whether the backend attached a code.
Low impact (429s rarely carry a code), but if the goal is a stable code, consider using EXIT_RATE_LIMITED here too (and dropping the e.code is None guard) so plain and coded 429s agree.
| # Match httpx's json= serialization (default separators + ensure_ascii=False) | ||
| # so the local size guard measures the same bytes that hit the wire. | ||
| encoded: bytes = json.dumps(parsed, ensure_ascii=False).encode("utf-8") |
There was a problem hiding this comment.
The comment claims this measures "the same bytes that hit the wire" as httpx's json= serialization, but they don't quite match: httpx (0.28) serializes with compact separators=(",", ":"), whereas json.dumps(parsed, ensure_ascii=False) here uses the default spaced separators (", " / ": "). For a payload with many keys this local count is larger than the wire bytes, so a request just under 8 KiB on the wire could be falsely rejected near the boundary.
The server is authoritative (workflow_trigger_payload_invalid), so this is a heuristic nit, not a correctness bug — but to actually match the wire:
| # Match httpx's json= serialization (default separators + ensure_ascii=False) | |
| # so the local size guard measures the same bytes that hit the wire. | |
| encoded: bytes = json.dumps(parsed, ensure_ascii=False).encode("utf-8") | |
| encoded: bytes = json.dumps(parsed, ensure_ascii=False, separators=(",", ":")).encode("utf-8") |
| if trigger_filter: | ||
| needle: str = trigger_filter.strip().lower() | ||
| # Compare against the same canonical value used by chat buttons / | ||
| # ``workflow trigger`` (API_TRIGGER_TYPE) and any other server type. | ||
| workflows = [wf for wf in workflows if str(wf.get("trigger_type", "")).lower() == needle] |
There was a problem hiding this comment.
--filter is applied client-side to only the workflows already fetched. Without --all (or a page containing them) a matching api_trigger workflow on a later page is invisible, and the footer at print_pagination_footer(len(workflows), meta.get("count"), ...) still reports the unfiltered server count/next — so the output can read like "Showing 1 of 50, more available" after filtering, which is confusing.
The empty-result hint is a nice touch; consider also nudging toward --all in it (it only fires for api_trigger + empty today). Docs already recommend --all, so this is minor.
| default=None, | ||
| help=( | ||
| "Optional bearer token for callback_auth on approval buttons. Prefer " | ||
| 'passing via an env var (e.g. --callback-bearer "$TOKEN") — a raw ' |
There was a problem hiding this comment.
Nice — proactively warning that a raw --callback-bearer token lands in shell history and process lists, and steering toward $TOKEN env indirection, is exactly the right credential-hygiene posture for a public CLI (AGENTS.md Rule 11). The token is also correctly kept out of all output paths (success --json emits the server response, not the payload).
1 participant
Summary
dailybot chat send/update:--buttonsJSON passthrough (no key stripping), approval-flow flags (--approve-button/--reject-button/--callback-url/--callback-bearer), and--workflow-button, with light client-side pre-validation (≤25 buttons, required label, mutual exclusivity of the five callbacks).dailybot workflow trigger <uuid> [--payload](POST /v1/workflows/{uuid}/trigger/) forapi_triggerworkflows, plusworkflow list --filter api_trigger.button_*/workflow_*error codes; chat--jsonnow emits structured{error, status, code, detail}on failure. README + API_REFERENCE examples updated.Companion skill-pack PR: DailybotHQ/agent-skill#43
Test plan
pytest tests/chat_commands_test.py tests/workflow_commands_test.py tests/api_client_test.py::TestDailyBotClientWorkflowTrigger tests/api_error_codes_alignment_test.pydailybot chat send --helpshows approval / workflow-button /--buttonsexamplesdailybot workflow trigger --helpdocumentsapi_trigger+ async 202--callback-url,--workflow-button,workflow trigger --payload '{"env":"prod"}' --json--payload-json/ unknown button keys still forward untouched