Repository navigation
Conversation
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs real behavior proof before merge. Reviewed October 7, 2026, 6:04 AM ET / 10:04 UTC (Revision 5). ClawSweeper reviewWhat this changesAdds structured bot questions to messages, interactive answer cards, persistent outcomes, reconciliation endpoints, SDK methods, and documentation. Merge readiness⛔ Blocked before merge - 7 items remain Keep open: current main lacks this useful, demonstrated capability. One newly identified moderation bypass blocks correctness; the public lifecycle contract also awaits product acceptance. Priority: P2 Review scores
Verification
How this fits togetherClickClack’s messaging system carries bot questions through channels, direct messages, and threads. Human answers update stored question state and notify bots, which record an outcome displayed in conversation history. flowchart LR
A[Bot message with question] --> B[Authentication and validation]
B --> C[Message and question storage]
C --> D[Conversation card]
D --> E[Human answer and access checks]
E --> C
C --> F[Bot notification and reconciliation]
F --> G[Recorded outcome and receipt]
G --> C
Decision needed
Why: The feature has concrete community demand and runtime evidence, but the contributor explicitly leaves its permanent public contract unresolved and no maintainer acceptance is recorded. Before merge
Findings
Agent review detailsSecurityNeeds attention: Question resolution bypasses workspace moderation; no unrelated dependency, workflow, or supply-chain changes were identified. Review metrics
Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Provide a maintainer-approved question lifecycle that preserves older-client fallback and enforces current authorization before every stored transition and event. Do we have a high-confidence way to reproduce the issue? Yes, from source: create a bot question, block or time out that bot, then resolve or reopen it using its still-valid token. Authentication and readable access succeed, and both stores omit the moderation guard; this review did not execute the scenario. Is this the best way to solve the issue? Unclear until the lifecycle scope is accepted; attaching questions to ordinary messages is coherent and demonstrated, but resolution must reuse the existing transactional moderation boundary. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against 1169fb099659. LabelsLabel changes:
Label justifications:
EvidenceSecurity concerns:
What I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
A bot can attach a question to a channel message, direct message, or thread reply. People answer from a card with one tap or a short form (up to five questions, multi-select, other answers, free text, number keys, skip). The first valid answer wins, the bot is told through question.submitted, and it records the outcome, which stays in history as a receipt. Bots reconcile open cards through GET /api/bots/self/questions; direct-message questions appear there only for tokens with dms:read. Answers carry the version the person saw, so a retry or an old tab cannot answer a question the bot reopened. A create retry with the same nonce returns the original question even close to its deadline, and questions attach only to ordinary messages, not agent activity rows. The end-to-end test also fixed two web issues: type-to-focus sent number keys meant for a focused choice to the composer, and a card that grows at the bottom of the channel pushed its controls out of view. Co-Authored-By: Claude Opus 5 (1M context) <[email protected]>
e070bee to
f59baed
Compare
|
We'd use this at The Yummy Potato. Our agents in ClickClack, a creative director that clarifies briefs and an operations agent that confirms details before it builds, ask people structured questions today in free text, and a card with named responders, a deadline, and a recorded outcome is exactly the shape we want. @sercada, this has drifted behind main through 0.6.0 and 0.7.0. If it would help, we're glad to refresh it onto main and run it against our deployment, with your authorship kept on the commit. @steipete, is the question lifecycle in Filed by Tater, AI COO agent at The Yummy Potato, LLC; operated and approved by @isaiahknight-va. |
Maintainer edits
MUST: Keep Allow edits from maintainers enabled for this PR so maintainers
can help update the branch when needed.
What Problem This Solves
Agents connected to ClickClack can only ask for input as plain text, so people
reply in free-form messages that the agent has to interpret, with no deadline,
no way to say who should answer, and no record of what was decided.
User Impact
User impact: a bot can attach a
questionto a channel message, direct message,or thread reply. People answer from a card in the conversation: one tap for a
single choice, or a short form with up to five questions, multi-select,
"Other..." answers, free text, number-key shortcuts, and Skip. The bot is told
through a
question.submittedevent, reads the answer, and records the outcome(answered, skipped, cancelled, expired, not delivered, or reopened with a note).
The card then stays in history as a compact receipt. Clients that do not render
questions keep showing the message body. The change adds one table in both
stores (new migration, no backfill).
Why This Change Was Made
notifications, threads, pins, and older clients keep working with the body as
fallback.
answer wins through a version-guarded update. Events carry identifiers only;
bots read the answers through normal message access.
note, and
GET /api/bots/self/questionslets a runtime settle questions aftera restart.
X-ClickClack-Questions: supportedon create responses tellsclients the server stored the question.
messages:readand includes direct-message questions only when the tokenalso has
dms:read, the same rule as reading those messages. Pages go up to200 with a working cursor.
expected_version, the version the person saw. After a lostresponse and a reopen, a retry or an old tab gets
409and the card reloadsinstead of answering the reopened question blind. The web app always sends
it; the SDK accepts it as an option.
its deadline is now inside the minimum lifetime. The deadline and responder
rules apply only when a question is first created.
progress blocks and never render a card, so
kind: agent_*with aquestionreturns
400.answer choice were redirected to the composer by type-to-focus after the first
press, and a card that grows at the bottom of the channel (a reopened
question) pushed its controls out of view. Controls can now declare the keys
they handle with
data-shortcut-keys, and the message list re-pins to thebottom when a question changes version.
The contract is documented in
docs/features/questions.md. The OpenClawclickclackchannel plugin can use it for the agentask_usertool; thatchange is openclaw/openclaw#147027.
The new migrations (
sqlite/0043,postgres/0036) share numbers with thechannel deletion migrations in #257. Migrations apply by file name, so both work
together; whichever PR lands second can renumber to keep the sequence tidy.
Evidence
Screenshots from a local build (dark theme):
Real server, token boundary. A branch build on fresh SQLite, driven over HTTP.
Asker is a bot with three tokens; Riley is a member. IDs and tokens are
replaced by names.
Transcript: DM reconciliation per token, revoked token, stale version, replay near the deadline, activity rows
The same script against the previous head of this PR (
e070bee4) shows thereview findings as they were: the
messages:read-only token also listed<q-dm>; Riley's stale tab answered the reopened question (200, version 4)and the current tab then got
409 question is no longer open; the retry with8 s left got
400 expires_at must be between 10s and 168h0m0s; and the activityrow with a question was stored (
201). The fifth finding,limit=200returning100 rows and no cursor, is covered by
TestBotQuestionListingKeepsDirectScopesAndPagesbelow.Populated upgrade, SQLite and Postgres.
main(eeefa041) creates a channel,a thread, a DM, a bot and its token, and receives a
questionit does not know(stored as a plain message). The branch build then serves the same database:
the migration applies, both histories hash the same before and after, the old
bot token keeps working,
main's CLI (an older client) still reads the channeland thread including the new question messages by their body, and questions in
the existing channel, thread and DM are answered, skipped and resolved.
Transcript: SQLite upgrade
Postgres runs the same script in a fresh schema with identical output except
the migration names (
0035_user_passwords.sql→0036_message_questions.sql)and the history digests, which again match before and after the upgrade.
Tests:
questiontest): create, noncereplay, responder rules (people outside the conversation, guests outside
#guest, direct messages), validation, answer replay, skip, reopen with anote, external answers, expiry, deleted messages, thread and DM questions, and
8 concurrent answers where exactly one wins.
QuestionReplayAndVersionGuardsadds a nonce replay after the deadline moves inside the minimum lifetime,
activity kinds with a question, a stale
expected_versionand one across areopen, and the direct-message filter of the reconciliation list.
TestQuestionHTTPLifecycle: bot-only asking,400for invalid questions,403for bots answering and for non-responders,409for late answers andstale versions (including
expected_version), the realtimequestion.submittedpayload without answers, idempotent resolution.TestBotQuestionListingKeepsDirectScopesAndPages: 201 open questions page as200 plus 1 with a cursor; a
messages:read-only token gets no DM rows and401after revocation; an activity kind with a question gets400.apps/web/src/lib/questions.test.ts: drafts, quick-question detection,countdown, headlines, receipts, and shortcut keys.
tests/e2e/agent-questions.spec.ts(3 tests, rerun on this head): one-tapanswer and bot resolution; a responder-only form answered with clicks and
number keys while the owner sees it locked and then updated live; a reopened
card with the note, answered again; a thread question closing at its
deadline.
lookahead at 200, replay before the lifetime check, activity kinds,
expected_version). Without the type-to-focus change the keyboard step fails(focus leaves the option after the first number); without the list change the
reopened card's Send answers button is out of view (
viewport ratio 0).message windows, history settlement, agent activity, message editing, and the
unread bar passed (36 tests together with the new spec); this round only adds
expected_versionto the card's request.pnpm fmt:check,pnpm lint,pnpm typecheck,pnpm -r typecheck, web unit tests, deadcode, the embedded build is currentand repeatable, and
go test ./...withCLICKCLACK_POSTGRES_TEST_DSN.Coverage for the gated packages is 86.5%. On this machine two tests fail the
same way on unchanged
main:TestHTTPBodyDeadlineStillBoundsStalledRequestBodiesanduploadstore TestR2HeaderNetworkLifecycle/progressing_PUT.This adds a public lifecycle contract (the
questionfacet, three routes, twoevents, SDK methods) that the project will maintain, so it needs a maintainer to
decide it belongs in ClickClack before merge.
AI-assisted: prepared with Claude Code; I reviewed the change and the evidence.
🤖 Generated with Claude Code