From 95bf450dc9732597c0abb62fb74138318e13526c Mon Sep 17 00:00:00 2001 From: Vignesh Natarajan Date: Wed, 29 Apr 2026 00:42:21 -0700 Subject: [PATCH] Agents: address inferred commitments review (#74189) --- src/commands/commitments.test.ts | 83 ++++++++++++++++++++++++++++++ src/commands/commitments.ts | 19 +++++-- src/commitments/extraction.test.ts | 25 +++++++++ src/commitments/extraction.ts | 8 --- src/commitments/runtime.test.ts | 7 ++- src/commitments/runtime.ts | 13 +---- src/commitments/store.ts | 6 +-- 7 files changed, 131 insertions(+), 30 deletions(-) create mode 100644 src/commands/commitments.test.ts diff --git a/src/commands/commitments.test.ts b/src/commands/commitments.test.ts new file mode 100644 index 00000000000..0c20d7b6d93 --- /dev/null +++ b/src/commands/commitments.test.ts @@ -0,0 +1,83 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; +import type { CommitmentRecord } from "../commitments/types.js"; +import type { RuntimeEnv } from "../runtime.js"; +import { commitmentsListCommand } from "./commitments.js"; + +const mocks = vi.hoisted(() => ({ + listCommitments: vi.fn(), + markCommitmentsStatus: vi.fn(), + resolveCommitmentStorePath: vi.fn(() => "/tmp/openclaw-commitments.json"), + loadConfig: vi.fn(() => ({ + commitments: { + store: "/tmp/openclaw-commitments.json", + }, + })), +})); + +vi.mock("../commitments/store.js", () => ({ + listCommitments: mocks.listCommitments, + markCommitmentsStatus: mocks.markCommitmentsStatus, + resolveCommitmentStorePath: mocks.resolveCommitmentStorePath, +})); + +vi.mock("../config/config.js", () => ({ + loadConfig: mocks.loadConfig, +})); + +function createRuntime(): { runtime: RuntimeEnv; logs: string[] } { + const logs: string[] = []; + return { + logs, + runtime: { + log: (message: unknown) => logs.push(String(message)), + error: vi.fn(), + exit: vi.fn(), + }, + }; +} + +function commitment(overrides?: Partial): CommitmentRecord { + return { + id: "cm_escape", + agentId: "main\u001b[31m", + sessionKey: "agent:main:session\u001b]8;;https://example.test\u0007", + channel: "telegram", + to: "+15551234567\u001b[0m", + kind: "event_check_in", + sensitivity: "routine", + source: "inferred_user_context", + status: "pending", + reason: "The user mentioned an interview.", + suggestedText: "How did it go?\u001b]52;c;YWJj\u0007\nspoofed", + dedupeKey: "interview:2026-04-30", + confidence: 0.91, + dueWindow: { + earliestMs: Date.parse("2026-04-30T17:00:00.000Z"), + latestMs: Date.parse("2026-04-30T23:00:00.000Z"), + timezone: "America/Los_Angeles", + }, + sourceUserText: "I have an interview tomorrow.", + createdAtMs: Date.parse("2026-04-29T16:00:00.000Z"), + updatedAtMs: Date.parse("2026-04-29T16:00:00.000Z"), + attempts: 0, + ...overrides, + }; +} + +describe("commitments command", () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.listCommitments.mockResolvedValue([commitment()]); + }); + + it("sanitizes untrusted commitment fields in table output", async () => { + const { runtime, logs } = createRuntime(); + + await commitmentsListCommand({}, runtime); + + const output = logs.join("\n"); + expect(output).not.toContain("\u001b"); + expect(output).not.toContain("\u0007"); + expect(output).toContain("\\nspoofed"); + }); +}); diff --git a/src/commands/commitments.ts b/src/commands/commitments.ts index 3b0e1b5a08d..871ccf4b72f 100644 --- a/src/commands/commitments.ts +++ b/src/commands/commitments.ts @@ -8,6 +8,7 @@ import { loadConfig } from "../config/config.js"; import { info } from "../globals.js"; import type { RuntimeEnv } from "../runtime.js"; import { normalizeOptionalString } from "../shared/string-coerce.js"; +import { sanitizeTerminalText } from "../terminal/safe-text.js"; import { isRich, theme } from "../terminal/theme.js"; const STATUS_VALUES = new Set([ @@ -22,6 +23,10 @@ function truncate(value: string, maxChars: number): string { return value.length <= maxChars ? value : `${value.slice(0, maxChars - 1)}...`; } +function safe(value: string): string { + return sanitizeTerminalText(value); +} + function parseStatus(raw: string | undefined, runtime: RuntimeEnv): CommitmentStatus | undefined { const status = normalizeOptionalString(raw); if (!status) { @@ -55,19 +60,23 @@ function formatRows(commitments: CommitmentRecord[], rich: boolean): string[] { const lines = [rich ? theme.heading(header) : header]; for (const commitment of commitments) { const scope = truncate( - [commitment.agentId, commitment.channel, commitment.to ?? commitment.sessionKey] + [ + safe(commitment.agentId), + safe(commitment.channel), + safe(commitment.to ?? commitment.sessionKey), + ] .filter(Boolean) .join("/"), 28, ); lines.push( [ - truncate(commitment.id, 16).padEnd(16), - commitment.status.padEnd(10), - commitment.kind.padEnd(16), + truncate(safe(commitment.id), 16).padEnd(16), + safe(commitment.status).padEnd(10), + safe(commitment.kind).padEnd(16), formatDue(commitment.dueWindow.earliestMs).padEnd(24), scope.padEnd(28), - truncate(commitment.suggestedText, 90), + truncate(safe(commitment.suggestedText), 90), ].join(" "), ); } diff --git a/src/commitments/extraction.test.ts b/src/commitments/extraction.test.ts index b18786afc3f..5505ff016f3 100644 --- a/src/commitments/extraction.test.ts +++ b/src/commitments/extraction.test.ts @@ -4,6 +4,7 @@ import path from "node:path"; import { afterEach, describe, expect, it } from "vitest"; import type { OpenClawConfig } from "../config/config.js"; import { + buildCommitmentExtractionPrompt, parseCommitmentExtractionOutput, persistCommitmentExtractionResult, validateCommitmentCandidates, @@ -77,6 +78,30 @@ describe("commitment extraction", () => { }); }); + it("omits routing scope identifiers from extractor prompts", () => { + const prompt = buildCommitmentExtractionPrompt({ + items: [ + item({ + itemId: "public-item-1", + agentId: "agent-secret", + sessionKey: "session-secret", + channel: "channel-secret", + accountId: "account-secret", + to: "+15551234567", + threadId: "thread-secret", + }), + ], + }); + + expect(prompt).toContain("public-item-1"); + expect(prompt).not.toContain("agent-secret"); + expect(prompt).not.toContain("session-secret"); + expect(prompt).not.toContain("channel-secret"); + expect(prompt).not.toContain("account-secret"); + expect(prompt).not.toContain("+15551234567"); + expect(prompt).not.toContain("thread-secret"); + }); + it("rejects disabled, low-confidence, and non-future candidates", () => { const cfg: OpenClawConfig = { commitments: { diff --git a/src/commitments/extraction.ts b/src/commitments/extraction.ts index ab38548eb0c..44d2c4d99b4 100644 --- a/src/commitments/extraction.ts +++ b/src/commitments/extraction.ts @@ -204,14 +204,6 @@ export function buildCommitmentExtractionPrompt(params: { itemId: item.itemId, now: new Date(item.nowMs).toISOString(), timezone: item.timezone, - scope: { - agentId: item.agentId, - sessionKey: item.sessionKey, - channel: item.channel, - accountId: item.accountId, - to: item.to, - threadId: item.threadId, - }, latestUserMessage: item.userText, assistantResponse: item.assistantText ?? "", existingPendingCommitments: formatExistingPending(item), diff --git a/src/commitments/runtime.test.ts b/src/commitments/runtime.test.ts index 6e330512b06..69fde2608c8 100644 --- a/src/commitments/runtime.test.ts +++ b/src/commitments/runtime.test.ts @@ -109,7 +109,12 @@ describe("commitment extraction runtime", () => { const store = await loadCommitmentStore(cfg.commitments?.store); expect(extractBatch).toHaveBeenCalledTimes(1); - expect(extractBatch.mock.calls[0]?.[0].items).toHaveLength(2); + const batchItems = extractBatch.mock.calls[0]?.[0].items; + expect(batchItems).toHaveLength(2); + expect(batchItems?.[0]?.itemId).not.toContain("main"); + expect(batchItems?.[0]?.itemId).not.toContain("telegram"); + expect(batchItems?.[0]?.itemId).not.toContain("15551234567"); + expect(batchItems?.[0]?.itemId).not.toContain("m1"); expect(store.commitments.map((commitment) => commitment.dedupeKey)).toEqual([ "event:1", "event:2", diff --git a/src/commitments/runtime.ts b/src/commitments/runtime.ts index c3ceec6a064..d7b8e5e7797 100644 --- a/src/commitments/runtime.ts +++ b/src/commitments/runtime.ts @@ -88,17 +88,8 @@ export function resetCommitmentExtractionRuntimeForTests(): void { } function buildItemId(params: CommitmentExtractionEnqueueInput, nowMs: number): string { - const source = normalizeOptionalString(params.sourceMessageId) ?? randomUUID(); - return [ - params.agentId, - params.sessionKey, - params.channel, - params.accountId ?? "", - params.to ?? "", - params.threadId ?? "", - source, - nowMs, - ].join(":"); + const source = normalizeOptionalString(params.sourceMessageId) ? "message" : "turn"; + return `${source}:${nowMs.toString(36)}:${randomUUID()}`; } function isUsefulText(value: string | undefined): boolean { diff --git a/src/commitments/store.ts b/src/commitments/store.ts index af0b7aeec3c..292df30c09b 100644 --- a/src/commitments/store.ts +++ b/src/commitments/store.ts @@ -265,11 +265,7 @@ export async function upsertInferredCommitments(params: { store.commitments.push(record); created.push(record); } - if (created.length > 0) { - await saveCommitmentStore(resolved.store, store); - } else { - await saveCommitmentStore(resolved.store, store); - } + await saveCommitmentStore(resolved.store, store); return created; }