From 339cc4f12aab2a0afd27aba4ea5a79e7ef6e9972 Mon Sep 17 00:00:00 2001 From: ragesaq <11304287+ragesaq@users.noreply.github.com> Date: Sat, 4 Jul 2026 19:16:29 +0000 Subject: [PATCH] =?UTF-8?q?fix(clickclack):=20address=20review=20=E2=80=94?= =?UTF-8?q?=20docs,=20lint=20lanes,=20activity=20coalescing?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - docs: document agentActivity option, the non-inherited agent_activity:write scope, default-off and best-effort degradation behavior - activity: type the enqueue catch parameter as unknown (lint lane) - activity: treat toolCallId as opaque; normalize only itemId lane prefixes - activity: skip identical/stale-shorter commentary snapshots so no redundant PATCHes queue - http-client: fail fast when createActivityMessage has no channel or conversation target; narrow mocked request bodies before JSON.parse --- docs/channels/clickclack.md | 27 +++++++++++++++- extensions/clickclack/src/activity.test.ts | 32 +++++++++++++++++-- extensions/clickclack/src/activity.ts | 26 +++++++++------ extensions/clickclack/src/http-client.test.ts | 26 +++++++++++++-- extensions/clickclack/src/http-client.ts | 3 ++ 5 files changed, 100 insertions(+), 14 deletions(-) diff --git a/docs/channels/clickclack.md b/docs/channels/clickclack.md index 00191759cd50..878db797f02b 100644 --- a/docs/channels/clickclack.md +++ b/docs/channels/clickclack.md @@ -130,6 +130,30 @@ bit: Keep the trust bit off if you only use the default `agent` reply mode; it is not needed there. +## Agent activity rows + +By default a ClickClack channel shows nothing while an agent turn runs; only the final reply lands. Set `agentActivity: true` on an account to publish durable `agent_commentary` and `agent_tool` message rows while the turn is in progress: + +```json5 +{ + channels: { + clickclack: { + enabled: true, + token: { source: "env", provider: "default", id: "CLICKCLACK_BOT_TOKEN" }, + workspace: "default", + agentActivity: true, + }, + }, +} +``` + +Requirements and behavior: + +- **Off by default.** Stock setups and older ClickClack servers are untouched. +- **Requires the `agent_activity:write` token scope.** This scope is separate from `bot:write` and is not inherited by it; create the bot token with `--scopes bot:write,agent_activity:write` (or grant the scope to an existing token) before enabling the option. +- **Best-effort degradation.** If the token lacks `agent_activity:write` or the server rejects activity writes, failures are logged and the final reply still delivers normally; no activity rows appear. +- Rows are grouped per turn (`turn_id`), coalesced so one logical step is one row, and tool rows use the same progress formatting as Discord/Slack/Telegram (tool name plus command detail). + ## Targets - `channel:` sends to a workspace channel. Bare targets default to `channel:`. @@ -153,8 +177,9 @@ ClickClack token scopes are enforced by the ClickClack API. - `bot:read`: read workspace/channel/message/thread/DM/realtime/profile data. - `bot:write`: `bot:read` plus channel messages, thread replies, DMs, and uploads. - `bot:admin`: `bot:write` plus channel creation. +- `agent_activity:write`: durable agent activity rows (`agent_commentary` / `agent_tool`). Not inherited by `bot:write` or `bot:admin`; required only when `agentActivity: true` is set. -OpenClaw only needs `bot:write` for normal agent chat. +OpenClaw only needs `bot:write` for normal agent chat. Add `agent_activity:write` when enabling [agent activity rows](#agent-activity-rows). ## Troubleshooting diff --git a/extensions/clickclack/src/activity.test.ts b/extensions/clickclack/src/activity.test.ts index f0caf0b1cdce..d5cb2345ae1c 100644 --- a/extensions/clickclack/src/activity.test.ts +++ b/extensions/clickclack/src/activity.test.ts @@ -71,6 +71,26 @@ describe("createClickClackActivityPublisher", () => { expect(updateMessageBody).toHaveBeenCalledWith("msg_1", "First and second"); }); + it("skips redundant PATCHes for identical or stale-shorter commentary snapshots", async () => { + const { client, createActivityMessage, updateMessageBody } = createClientMock(); + const publisher = createClickClackActivityPublisher({ + client, + target: { channelId: "chn_1" }, + turnId: "msg_turn", + flushMs: 10, + }); + + publisher.onItemEvent({ itemId: "c1", kind: "preamble", progressText: "First and second" }); + await vi.advanceTimersByTimeAsync(20); + // Identical snapshot and a stale shorter frame must not queue new flushes. + publisher.onItemEvent({ itemId: "c1", kind: "preamble", progressText: "First and second" }); + publisher.onItemEvent({ itemId: "c1", kind: "preamble", progressText: "First" }); + await publisher.finalize(); + + expect(createActivityMessage).toHaveBeenCalledTimes(1); + expect(updateMessageBody).not.toHaveBeenCalled(); + }); + it("opens a new durable row for each commentary segment (item id)", async () => { const { client, createActivityMessage } = createClientMock(); const publisher = createClickClackActivityPublisher({ @@ -98,10 +118,18 @@ describe("createClickClackActivityPublisher", () => { turnId: "msg_turn", }); - publisher.onItemEvent({ toolCallId: "tool:toolu_1", kind: "tool", name: "exec" }); + // The runtime emits one opaque toolCallId across all frames of a call; + // the lane prefix (tool:/command:) lives on itemId only. + publisher.onItemEvent({ + itemId: "tool:toolu_1", + toolCallId: "toolu_1", + kind: "tool", + name: "exec", + }); await publisher.finalize(); publisher.onItemEvent({ - toolCallId: "command:toolu_1", + itemId: "command:toolu_1", + toolCallId: "toolu_1", kind: "command", name: "exec", progressText: "ls -la", diff --git a/extensions/clickclack/src/activity.ts b/extensions/clickclack/src/activity.ts index d5d44ce7901b..09b36d4dd1c4 100644 --- a/extensions/clickclack/src/activity.ts +++ b/extensions/clickclack/src/activity.ts @@ -132,7 +132,7 @@ export function createClickClackActivityPublisher(params: { let chain: Promise = Promise.resolve(); const enqueue = (work: () => Promise): Promise => { - chain = chain.then(work).catch((error) => { + chain = chain.then(work).catch((error: unknown) => { params.onError?.(error); }); return chain; @@ -188,10 +188,13 @@ export function createClickClackActivityPublisher(params: { commentaryByItem.set(key, segment); } // Snapshots are cumulative per item; never shrink the row body on a - // shorter (stale or whitespace-normalized) frame. - if (text.length >= segment.body.length) { - segment.body = text; + // shorter (stale or whitespace-normalized) frame, and skip identical + // snapshots entirely so out-of-order frames cannot queue redundant + // PATCHes. + if (text.length < segment.body.length || text === segment.body) { + return; } + segment.body = text; segment.dirty = true; if (!segment.timer) { segment.timer = setTimeout(() => { @@ -202,11 +205,16 @@ export function createClickClackActivityPublisher(params: { }; const toolRowKey = (payload: ClickClackItemEventPayload): string => { - // The same call can surface with lane-prefixed ids (`tool:X`, - // `command:X`) on some frames and the bare id on others; strip the prefix - // from whichever identifier we use so all frames share one key. - const base = payload.toolCallId?.trim() || payload.itemId?.trim() || ""; - return base.replace(/^(tool|command):/, ""); + // toolCallId is an opaque identifier: use it untouched when present. + // itemId carries a lane/lifecycle prefix (`tool:X`, `command:X`) on some + // frames and appears bare on others, so normalize only itemId to give + // all frames of one call a shared key. + const toolCallId = payload.toolCallId?.trim(); + if (toolCallId) { + return toolCallId; + } + const itemId = payload.itemId?.trim() ?? ""; + return itemId.replace(/^(tool|command):/, ""); }; const handleDiscreteItem = (payload: ClickClackItemEventPayload): void => { diff --git a/extensions/clickclack/src/http-client.test.ts b/extensions/clickclack/src/http-client.test.ts index 45756ab66748..ecee8442cc23 100644 --- a/extensions/clickclack/src/http-client.test.ts +++ b/extensions/clickclack/src/http-client.test.ts @@ -4,6 +4,14 @@ import { createClickClackClient } from "./http-client.js"; const LOOPBACK_RESPONSE_BYTES = 18 * 1024 * 1024; +function requestBodyJson(init: RequestInit | undefined): unknown { + const body = init?.body; + if (typeof body !== "string") { + throw new Error("expected string request body"); + } + return JSON.parse(body); +} + async function listenLoopbackServer(server: Server): Promise { return await new Promise((resolve, reject) => { server.once("error", reject); @@ -176,13 +184,27 @@ describe("ClickClack HTTP client", () => { expect.objectContaining({ method: "POST" }), ); const init = fetchMock.mock.calls[0]?.[1]; - expect(JSON.parse(String(init?.body))).toEqual({ + expect(requestBodyJson(init)).toEqual({ body: "ran bash", kind: "agent_tool", turn_id: "t1", }); }); + it("rejects activity rows without a channel or conversation target", async () => { + const fetchMock = vi.fn(); + const client = createClickClackClient({ + baseUrl: "https://clickclack.example", + token: "test-token", + fetch: fetchMock as unknown as typeof fetch, + }); + + await expect( + client.createActivityMessage({ body: "orphan row", kind: "agent_commentary" }), + ).rejects.toThrow("createActivityMessage requires a channelId or conversationId"); + expect(fetchMock).not.toHaveBeenCalled(); + }); + it("routes DM activity rows through the conversation create path", async () => { const fetchMock = vi.fn( async () => @@ -231,6 +253,6 @@ describe("ClickClack HTTP client", () => { expect.objectContaining({ method: "PATCH" }), ); const init = fetchMock.mock.calls[0]?.[1]; - expect(JSON.parse(String(init?.body))).toEqual({ body: "longer" }); + expect(requestBodyJson(init)).toEqual({ body: "longer" }); }); }); diff --git a/extensions/clickclack/src/http-client.ts b/extensions/clickclack/src/http-client.ts index 34279b25761b..09fedeb26bd9 100644 --- a/extensions/clickclack/src/http-client.ts +++ b/extensions/clickclack/src/http-client.ts @@ -127,6 +127,9 @@ export function createClickClackClient(options: ClientOptions) { kind: "agent_commentary" | "agent_tool"; turnId?: string; }): Promise => { + if (!params.channelId && !params.conversationId) { + throw new Error("createActivityMessage requires a channelId or conversationId"); + } const path = params.channelId ? `/api/channels/${encodeURIComponent(params.channelId)}/messages` : `/api/dms/${encodeURIComponent(params.conversationId ?? "")}/messages`;