fix: deliver ClickClack media and use runtime model budgets [AI-assisted]#105775
Conversation
|
Codex review: needs maintainer review before merge. Reviewed July 12, 2026, 11:51 PM ET / July 13, 2026, 03:51 UTC. Summary PR surface: Source +275, Tests +392, Docs +13, Generated 0. Total +680 across 13 files. Reproducibility: yes. from source and supplied live evidence: current ClickClack lacks a media sender and the prior model path used a fixed 96-token completion budget, but this review did not independently execute a current-main reproduction. Review metrics: 1 noteworthy metric.
Stored data model Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review detailsBest possible solution: Refresh the branch onto current main, retain the plugin-owned media delivery and runtime-default fallback, then either remove the account-level token option or obtain explicit owner approval for its distinct per-account use case; also document and test the minimum ClickClack nonce contract needed for retry-safe channel, thread, and DM delivery. Do we have a high-confidence way to reproduce the issue? Yes from source and supplied live evidence: current ClickClack lacks a media sender and the prior model path used a fixed 96-token completion budget, but this review did not independently execute a current-main reproduction. Is this the best way to solve the issue? Partly: implementing media inside the ClickClack plugin and deferring unset budgets to the runtime are the narrowest fixes, while the additional account-level token setting is not yet shown to be better than the existing provider/model configuration. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 6dfa4c27a3ad. Label changesLabel justifications:
Evidence reviewedPR surface: Source +275, Tests +392, Docs +13, Generated 0. Total +680 across 13 files. View PR surface stats
Acceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (5 earlier review cycles)
|
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
f2c6d2a to
80b44b0
Compare
16968ff to
0db17a0
Compare
0db17a0 to
f65dc98
Compare
f65dc98 to
b65a255
Compare
b65a255 to
51a19f2
Compare
|
Merged using rebase. What changed after review:
Verification:
Landed on
Thanks @jjjhenriksen! |
This PR is AI-assisted. I have personally verified all code and AI was used for syntax.
Summary
Fix two ClickClack outbound failure modes:
The patch also makes durable media retries idempotent and logs model-mode completions that produce no sendable text.
What Problem This Solves
The ClickClack CLI accepted
--mediaand reported delivery success, but the installed outbound adapter warned thatsendMediawas missing, dropped the media URL, and created only a text message.Separately,
replyMode: "model"always calledllm.completewithmaxTokens: 96. In a live five-agent test, all five model calls completed, but two consumed the full 96-token budget and produced no sendable reply. Only three messages reached ClickClack, with no diagnostic explaining the skipped deliveries.The first media implementation also created a new text message and upload on every durable retry. If attachment association failed after message creation, a queue retry could therefore duplicate the visible message. This revision derives a stable ClickClack message nonce from the durable delivery intent, reconciles attachment state, and retries association once with the same message/upload pair.
Change Type (select all)
Scope (select all touched areas)
Linked Issue/PR
Evidence
127.0.0.1:18082, five named ClickClack bot accounts.Helloevent reached all five bot connections and all fiveruntime.llmcalls completed. ClickClack persisted replies from Mustang, Clipper, and Nighthawk only. Blackbird and Dragon Lady both reached the hard-coded 96-token ceiling and emitted no outbound message request.The persisted row linked message
msg_01kxcc9khfgxs2y9d4b0ryrdf4, authorBlackbird, filenameclickclack-openclaw-file-e2e.txt, content typetext/plain, and byte size51.Live ClickClack channel view showing Blackbird's persisted message, attachment card, filename, size, and download control:
Live ClickClack artifact viewer showing the same 51-byte attachment opened successfully with its exact contents:
The focused tests assert that a lost attachment response is reconciled, a transient failure retries the same
msg_out/upl_1pair, a durable retry reuses the nonce-keyed message, and two failed association attempts still reject rather than reporting false success.git range-diffconfirmed all 10 prepared commits were patch-identical after the server-side rebase.tbx_01kxecpe28z66hyfwn4ctm8tc2passed 64 ClickClack tests, 144 outbound delivery tests, and 4 message-planning tests.main.Root Cause
mediaUrland used its text fallback.dispatchModelReplyhard-codedmaxTokens: 96and returned silently when the resulting text was empty.User-visible / Behavior Changes
replyMode: "model"omits a ClickClack-specificmaxTokenscap, allowing the selected runtime and model output budget to apply.Compatibility / Migration
Yes/No): YesYes/No): NoYes/No): No