From 04d4c5caff69118a90eaeeeb4a716d883eb7651c Mon Sep 17 00:00:00 2001 From: jesse-merhi <79823012+jesse-merhi@users.noreply.github.com> Date: Wed, 6 May 2026 14:38:39 +1000 Subject: [PATCH] fix: keep hook block reasons internal --- .../diagnostics-otel/src/service.test.ts | 28 +++++++++++ extensions/diagnostics-otel/src/service.ts | 6 +++ .../src/service.test.ts | 31 ++++++++++++ .../diagnostics-prometheus/src/service.ts | 2 + src/agents/cli-runner.reliability.test.ts | 5 +- src/agents/cli-runner.ts | 12 ++--- .../pi-embedded-runner/run/attempt.test.ts | 3 +- src/agents/pi-embedded-runner/run/attempt.ts | 49 +++++++++++++------ src/gateway/session-message-events.test.ts | 5 +- src/gateway/session-utils.fs.test.ts | 7 +-- src/infra/diagnostic-events.ts | 3 +- src/plugins/hook-decision-types.test.ts | 3 ++ src/plugins/hook-decision-types.ts | 13 +++-- 13 files changed, 130 insertions(+), 37 deletions(-) diff --git a/extensions/diagnostics-otel/src/service.test.ts b/extensions/diagnostics-otel/src/service.test.ts index d77783a899c..46cd0d628cc 100644 --- a/extensions/diagnostics-otel/src/service.test.ts +++ b/extensions/diagnostics-otel/src/service.test.ts @@ -646,6 +646,34 @@ describe("diagnostics-otel service", () => { await service.stop?.(ctx); }); + test("records hook-blocked run metrics with safe blocker originator", async () => { + const service = createDiagnosticsOtelService(); + const ctx = createOtelContext(OTEL_TEST_ENDPOINT, { traces: true, metrics: true }); + await service.start(ctx); + + emitDiagnosticEvent({ + type: "run.completed", + runId: "run-1", + provider: "openai", + model: "gpt-5.4", + outcome: "blocked", + blockedBy: "policy-plugin", + durationMs: 100, + }); + await flushDiagnosticEvents(); + + expect(telemetryState.histograms.get("openclaw.run.duration_ms")?.record).toHaveBeenCalledWith( + 100, + expect.objectContaining({ + "openclaw.outcome": "blocked", + "openclaw.blocked_by": "policy-plugin", + }), + ); + expect(JSON.stringify(telemetryState)).not.toContain("matched secret prompt"); + + await service.stop?.(ctx); + }); + test("honors disabled traces when an OpenTelemetry SDK is preloaded", async () => { process.env.OPENCLAW_OTEL_PRELOADED = "1"; const service = createDiagnosticsOtelService(); diff --git a/extensions/diagnostics-otel/src/service.ts b/extensions/diagnostics-otel/src/service.ts index 7a6bc84f062..c22bc53a5f5 100644 --- a/extensions/diagnostics-otel/src/service.ts +++ b/extensions/diagnostics-otel/src/service.ts @@ -1665,6 +1665,9 @@ export function createDiagnosticsOtelService(): OpenClawPluginService { if (evt.channel) { attrs["openclaw.channel"] = evt.channel; } + if (evt.blockedBy) { + attrs["openclaw.blocked_by"] = lowCardinalityAttr(evt.blockedBy, "unknown"); + } durationHistogram.record(evt.durationMs, attrs); if (!tracesEnabled) { return; @@ -1673,6 +1676,9 @@ export function createDiagnosticsOtelService(): OpenClawPluginService { "openclaw.outcome": evt.outcome, }; addRunAttrs(spanAttrs, evt); + if (evt.blockedBy) { + spanAttrs["openclaw.blocked_by"] = lowCardinalityAttr(evt.blockedBy, "unknown"); + } if (evt.errorCategory) { spanAttrs["openclaw.errorCategory"] = lowCardinalityAttr(evt.errorCategory, "other"); } diff --git a/extensions/diagnostics-prometheus/src/service.test.ts b/extensions/diagnostics-prometheus/src/service.test.ts index 1bd10ddee1c..85a67c1a81f 100644 --- a/extensions/diagnostics-prometheus/src/service.test.ts +++ b/extensions/diagnostics-prometheus/src/service.test.ts @@ -43,6 +43,37 @@ describe("diagnostics-prometheus service", () => { expect(rendered).not.toContain("session-should-not-export"); }); + it("records hook-blocked run metrics with safe blocker originator only", () => { + const store = __test__.createPrometheusMetricStore(); + + __test__.recordDiagnosticEvent( + store, + { + ...baseEvent(), + type: "run.completed", + runId: "run-should-not-export", + sessionKey: "session-should-not-export", + provider: "openai", + model: "gpt-5.4", + channel: "slack", + trigger: "message", + durationMs: 250, + outcome: "blocked", + blockedBy: "policy-plugin", + }, + trusted, + ); + + const rendered = __test__.renderPrometheusMetrics(store); + + expect(rendered).toContain( + 'openclaw_run_completed_total{blocked_by="policy-plugin",channel="slack",model="gpt-5.4",outcome="blocked",provider="openai",trigger="message"} 1', + ); + expect(rendered).not.toContain("run-should-not-export"); + expect(rendered).not.toContain("session-should-not-export"); + expect(rendered).not.toContain("matched secret prompt"); + }); + it("drops untrusted plugin-emitted diagnostic events", () => { const store = __test__.createPrometheusMetricStore(); diff --git a/extensions/diagnostics-prometheus/src/service.ts b/extensions/diagnostics-prometheus/src/service.ts index 1fb75e171d7..38d341500e5 100644 --- a/extensions/diagnostics-prometheus/src/service.ts +++ b/extensions/diagnostics-prometheus/src/service.ts @@ -276,6 +276,7 @@ function renderPrometheusMetrics(store: PrometheusMetricStore): string { } function runLabels(evt: { + blockedBy?: string; channel?: string; model?: string; outcome?: string; @@ -283,6 +284,7 @@ function runLabels(evt: { trigger?: string; }): LabelSet { return { + ...(evt.blockedBy ? { blocked_by: lowCardinalityLabel(evt.blockedBy) } : {}), channel: lowCardinalityLabel(evt.channel), model: lowCardinalityLabel(evt.model), outcome: lowCardinalityLabel(evt.outcome, "unknown"), diff --git a/src/agents/cli-runner.reliability.test.ts b/src/agents/cli-runner.reliability.test.ts index 419d916115e..9cf2c3280e9 100644 --- a/src/agents/cli-runner.reliability.test.ts +++ b/src/agents/cli-runner.reliability.test.ts @@ -642,7 +642,7 @@ describe("runCliAgent reliability", () => { pluginId: "policy-plugin", decision: { outcome: "block" as const, - reason: "contains protected content", + reason: "matched secret prompt: secret prompt", message: "The agent cannot read this message.", }, })), @@ -708,10 +708,11 @@ describe("runCliAgent reliability", () => { const blockedLine = JSON.parse(lines[lines.length - 1]); expect(blockedLine.message.content[0].text).toBe("The agent cannot read this message."); expect(JSON.stringify(blockedLine)).not.toContain("secret prompt"); + expect(JSON.stringify(blockedLine)).not.toContain("matched secret prompt"); expect(blockedLine.message.__openclaw.beforeAgentRunBlocked).toMatchObject({ blockedBy: "policy-plugin", - reason: "contains protected content", }); + expect(blockedLine.message.__openclaw.beforeAgentRunBlocked).not.toHaveProperty("reason"); expect(Object.hasOwn(blockedLine.message.__openclaw, "beforeAgentRunBlocked")).toBe(true); } finally { fs.rmSync(dir, { recursive: true, force: true }); diff --git a/src/agents/cli-runner.ts b/src/agents/cli-runner.ts index 1fb2cc20d08..88ef3cbcc77 100644 --- a/src/agents/cli-runner.ts +++ b/src/agents/cli-runner.ts @@ -4,7 +4,7 @@ import { SILENT_REPLY_TOKEN } from "../auto-reply/tokens.js"; import { formatErrorMessage } from "../infra/errors.js"; import { createSubsystemLogger } from "../logging/subsystem.js"; import { buildAgentHookContextChannelFields } from "../plugins/hook-agent-context.js"; -import { resolveBlockMessage } from "../plugins/hook-decision-types.js"; +import { DEFAULT_BLOCK_MESSAGE, resolveBlockMessage } from "../plugins/hook-decision-types.js"; import { getGlobalHookRunner } from "../plugins/hook-runner-global.js"; import { loadCliSessionHistoryMessages } from "./cli-runner/session-history.js"; import type { PreparedCliRunContext, RunCliAgentParams } from "./cli-runner/types.js"; @@ -242,7 +242,6 @@ export async function runPreparedCliAgent( const persistBlockedBeforeAgentRun = async (block: { message: string; pluginId: string; - reason: string; }): Promise => { try { const nowMs = Date.now(); @@ -255,7 +254,6 @@ export async function runPreparedCliAgent( __openclaw: { beforeAgentRunBlocked: { blockedBy: block.pluginId, - reason: block.reason, blockedAt: nowMs, }, }, @@ -419,11 +417,10 @@ export async function runPreparedCliAgent( buildAgentHookContext(hookContext), ); } catch (err) { - const blockMessage = "Request blocked by before_agent_run policy."; + const blockMessage = `${DEFAULT_BLOCK_MESSAGE} by before_agent_run`; await persistBlockedBeforeAgentRun({ message: blockMessage, pluginId: "before_agent_run", - reason: `before_agent_run hook failed closed: ${formatErrorMessage(err)}`, }); runAgentHarnessAgentEndHook({ event: buildBlockedAgentEndEvent(blockMessage), @@ -435,11 +432,12 @@ export async function runPreparedCliAgent( const beforeRunDecision = beforeRunResult?.decision; if (beforeRunDecision?.outcome === "block") { - const blockMessage = resolveBlockMessage(beforeRunDecision); + const blockMessage = resolveBlockMessage(beforeRunDecision, { + blockedBy: beforeRunResult?.pluginId ?? "unknown", + }); await persistBlockedBeforeAgentRun({ message: blockMessage, pluginId: beforeRunResult?.pluginId ?? "unknown", - reason: beforeRunDecision.reason, }); runAgentHarnessAgentEndHook({ event: buildBlockedAgentEndEvent(blockMessage), diff --git a/src/agents/pi-embedded-runner/run/attempt.test.ts b/src/agents/pi-embedded-runner/run/attempt.test.ts index 1ed11223731..1d100182e5a 100644 --- a/src/agents/pi-embedded-runner/run/attempt.test.ts +++ b/src/agents/pi-embedded-runner/run/attempt.test.ts @@ -160,7 +160,6 @@ describe("normalizeMessagesForLlmBoundary", () => { __openclaw: { beforeAgentRunBlocked: { blockedBy: "policy-plugin", - reason: "contains protected content", blockedAt: 1, }, }, @@ -175,7 +174,9 @@ describe("normalizeMessagesForLlmBoundary", () => { { type: "text", text: "The agent cannot read this message." }, ]); expect(output[0]).toHaveProperty("__openclaw.beforeAgentRunBlocked"); + expect(output[0]).not.toHaveProperty("__openclaw.beforeAgentRunBlocked.reason"); expect(JSON.stringify(output)).not.toContain("secret prompt"); + expect(JSON.stringify(output)).not.toContain("matched secret prompt"); expect(input[0]).toHaveProperty("__openclaw"); }); }); diff --git a/src/agents/pi-embedded-runner/run/attempt.ts b/src/agents/pi-embedded-runner/run/attempt.ts index 9b48dc8c018..161590a99b8 100644 --- a/src/agents/pi-embedded-runner/run/attempt.ts +++ b/src/agents/pi-embedded-runner/run/attempt.ts @@ -25,7 +25,10 @@ import { MAX_IMAGE_BYTES } from "../../../media/constants.js"; import { listRegisteredPluginAgentPromptGuidance } from "../../../plugins/command-registry-state.js"; import { getCurrentPluginMetadataSnapshot } from "../../../plugins/current-plugin-metadata-snapshot.js"; import { buildAgentHookContextChannelFields } from "../../../plugins/hook-agent-context.js"; -import { resolveBlockMessage } from "../../../plugins/hook-decision-types.js"; +import { + DEFAULT_BLOCK_MESSAGE, + resolveBlockMessage, +} from "../../../plugins/hook-decision-types.js"; import { getGlobalHookRunner } from "../../../plugins/hook-runner-global.js"; import { extractModelCompat, @@ -736,8 +739,14 @@ export async function runEmbeddedAttempt( let timedOutDuringToolExecution = false; let promptError: unknown = null; let emitDiagnosticRunCompleted: - | ((outcome: "completed" | "aborted" | "error", err?: unknown) => void) + | (( + outcome: "completed" | "aborted" | "blocked" | "error", + err?: unknown, + extra?: { blockedBy?: string }, + ) => void) | undefined; + let beforeAgentRunBlocked = false; + let beforeAgentRunBlockedBy: string | undefined; try { const skillsSnapshotForRun = sandbox?.enabled && sandbox.workspaceAccess !== "rw" ? undefined : params.skillsSnapshot; @@ -800,7 +809,7 @@ export async function runEmbeddedAttempt( }); const diagnosticRunStartedAt = Date.now(); let diagnosticRunCompleted = false; - emitDiagnosticRunCompleted = (outcome, err) => { + emitDiagnosticRunCompleted = (outcome, err, extra) => { if (diagnosticRunCompleted) { return; } @@ -810,7 +819,8 @@ export async function runEmbeddedAttempt( ...diagnosticRunBase, durationMs: Date.now() - diagnosticRunStartedAt, outcome, - ...(err ? { errorCategory: diagnosticErrorCategory(err) } : {}), + ...(extra?.blockedBy ? { blockedBy: extra.blockedBy } : {}), + ...(err && outcome !== "blocked" ? { errorCategory: diagnosticErrorCategory(err) } : {}), }); }; const corePluginToolStages = createEmbeddedRunStageTracker(); @@ -2791,7 +2801,6 @@ export async function runEmbeddedAttempt( const persistBlockedBeforeAgentRun = async (block: { message: string; pluginId: string; - reason: string; }): Promise => { const idempotencyKey = `hook-block:before_agent_run:user:${params.runId}`; if (sessionMessagesContainIdempotencyKey(activeSession.messages, idempotencyKey)) { @@ -2806,7 +2815,6 @@ export async function runEmbeddedAttempt( __openclaw: { beforeAgentRunBlocked: { blockedBy: block.pluginId, - reason: block.reason, blockedAt: nowMs, }, }, @@ -2851,26 +2859,30 @@ export async function runEmbeddedAttempt( ); } catch (err) { log.warn(`before_agent_run hook failed: ${formatErrorMessage(err)}`); + beforeAgentRunBlocked = true; + beforeAgentRunBlockedBy = "before_agent_run"; await persistBlockedBeforeAgentRun({ - message: "Request blocked by before_agent_run policy.", + message: `${DEFAULT_BLOCK_MESSAGE} by before_agent_run`, pluginId: "before_agent_run", - reason: "before_agent_run hook failed closed", }); - promptError = new Error("Request blocked by before_agent_run policy."); + promptError = new Error(`${DEFAULT_BLOCK_MESSAGE} by before_agent_run`); promptErrorSource = "hook:before_agent_run"; skipPromptSubmission = true; } const beforeRunDecision = beforeRunResult?.decision; const beforeRunPluginId = beforeRunResult?.pluginId ?? "unknown"; if (beforeRunDecision?.outcome === "block") { - const blockReplacementMsg = resolveBlockMessage(beforeRunDecision); + beforeAgentRunBlocked = true; + beforeAgentRunBlockedBy = beforeRunPluginId; + const blockReplacementMsg = resolveBlockMessage(beforeRunDecision, { + blockedBy: beforeRunPluginId, + }); log.warn( `before_agent_run hook blocked by ${beforeRunPluginId}: ${beforeRunDecision.reason}`, ); await persistBlockedBeforeAgentRun({ message: blockReplacementMsg, pluginId: beforeRunPluginId, - reason: beforeRunDecision.reason, }); promptError = new Error(blockReplacementMsg); promptErrorSource = "hook:before_agent_run"; @@ -3783,12 +3795,19 @@ export async function runEmbeddedAttempt( cleanupError = err; } emitDiagnosticRunCompleted?.( - cleanupError || promptError + cleanupError ? "error" - : aborted || timedOut || idleTimedOut || timedOutDuringCompaction - ? "aborted" - : "completed", + : beforeAgentRunBlocked + ? "blocked" + : promptError + ? "error" + : aborted || timedOut || idleTimedOut || timedOutDuringCompaction + ? "aborted" + : "completed", cleanupError ?? promptError, + beforeAgentRunBlocked + ? { blockedBy: beforeAgentRunBlockedBy ?? "before_agent_run" } + : undefined, ); if (cleanupError) { await Promise.reject(cleanupError); diff --git a/src/gateway/session-message-events.test.ts b/src/gateway/session-message-events.test.ts index ec137709e69..f3bce878c28 100644 --- a/src/gateway/session-message-events.test.ts +++ b/src/gateway/session-message-events.test.ts @@ -314,7 +314,7 @@ describe("session.message websocket events", () => { role: "user", content: [{ type: "text", text: "The agent cannot read this message." }], __openclaw: { - beforeAgentRunBlocked: { blockedBy: "policy-plugin", reason: "blocked", blockedAt: 1 }, + beforeAgentRunBlocked: { blockedBy: "policy-plugin", blockedAt: 1 }, }, }, }); @@ -326,6 +326,7 @@ describe("session.message websocket events", () => { { type: "text", text: "The agent cannot read this message." }, ]); expect(JSON.stringify(payload.message)).not.toContain("secret blocked prompt"); + expect(JSON.stringify(payload.message)).not.toContain("contains protected content"); }); }); @@ -353,7 +354,6 @@ describe("session.message websocket events", () => { __openclaw: { beforeAgentRunBlocked: { blockedBy: "policy-plugin", - reason: "contains protected content", blockedAt: Date.now(), }, }, @@ -373,6 +373,7 @@ describe("session.message websocket events", () => { { type: "text", text: "The agent cannot read this message." }, ]); expect(JSON.stringify(payload.message)).not.toContain("secret blocked prompt"); + expect(JSON.stringify(payload.message)).not.toContain("contains protected content"); }); }); diff --git a/src/gateway/session-utils.fs.test.ts b/src/gateway/session-utils.fs.test.ts index 378994efaf1..12faf732f78 100644 --- a/src/gateway/session-utils.fs.test.ts +++ b/src/gateway/session-utils.fs.test.ts @@ -82,7 +82,6 @@ function appendBlockedUserMessageWithSessionManager(params: { originalText?: string; redactedText: string; pluginId: string; - reason: string; idempotencyKey?: string; }): string { const sessionManager = SessionManager.open(params.sessionFile, path.dirname(params.sessionFile)); @@ -94,7 +93,6 @@ function appendBlockedUserMessageWithSessionManager(params: { __openclaw: { beforeAgentRunBlocked: { blockedBy: params.pluginId, - reason: params.reason, blockedAt: Date.now(), }, }, @@ -1300,7 +1298,6 @@ describe("readSessionMessages", () => { originalText: "[hitl:block] hello", redactedText: "Blocked by HITL test hook.", pluginId: "hitl-test-hooks", - reason: "blocked by test policy", }); expect(messageId).toBeTruthy(); @@ -1316,6 +1313,7 @@ describe("readSessionMessages", () => { { role: "user", text: [{ type: "text", text: "Blocked by HITL test hook." }] }, ]); expect(JSON.stringify(out)).not.toContain("[hitl:block] hello"); + expect(JSON.stringify(out)).not.toContain("matched original"); }); test("keeps repeated blocked hook messages together in a new session", async () => { @@ -1343,14 +1341,12 @@ describe("readSessionMessages", () => { originalText: "[hitl:block] first", redactedText: "Blocked by HITL test hook.", pluginId: "hitl-test-hooks", - reason: "blocked by test policy", }); appendBlockedUserMessageWithSessionManager({ sessionFile, originalText: "[hitl:block] second", redactedText: "Blocked again by HITL test hook.", pluginId: "hitl-test-hooks", - reason: "blocked by test policy", }); const out = readSessionMessages(sessionId, storePath, sessionFile); @@ -1365,6 +1361,7 @@ describe("readSessionMessages", () => { ]); expect(JSON.stringify(out)).not.toContain("[hitl:block] first"); expect(JSON.stringify(out)).not.toContain("[hitl:block] second"); + expect(JSON.stringify(out)).not.toContain("matched original"); }); }); diff --git a/src/infra/diagnostic-events.ts b/src/infra/diagnostic-events.ts index 5653525244f..b809b1e6bbd 100644 --- a/src/infra/diagnostic-events.ts +++ b/src/infra/diagnostic-events.ts @@ -388,8 +388,9 @@ export type DiagnosticRunStartedEvent = DiagnosticRunBaseEvent & { export type DiagnosticRunCompletedEvent = DiagnosticRunBaseEvent & { type: "run.completed"; durationMs: number; - outcome: "completed" | "aborted" | "error"; + outcome: "completed" | "aborted" | "blocked" | "error"; errorCategory?: string; + blockedBy?: string; }; export type DiagnosticHarnessRunPhase = "prepare" | "start" | "send" | "resolve" | "cleanup"; diff --git a/src/plugins/hook-decision-types.test.ts b/src/plugins/hook-decision-types.test.ts index ed7a10f79bc..284d6e104a3 100644 --- a/src/plugins/hook-decision-types.test.ts +++ b/src/plugins/hook-decision-types.test.ts @@ -65,6 +65,9 @@ describe("HookDecision helpers", () => { expect(resolveBlockMessage(explicit)).toBe("Please rephrase your request."); expect(resolveBlockMessage(fallback)).toBe(DEFAULT_BLOCK_MESSAGE); + expect(resolveBlockMessage(fallback, { blockedBy: "policy-plugin" })).toBe( + `${DEFAULT_BLOCK_MESSAGE} by policy-plugin`, + ); expect(resolveBlockMessage({ ...explicit, message: " " })).toBe(DEFAULT_BLOCK_MESSAGE); }); }); diff --git a/src/plugins/hook-decision-types.ts b/src/plugins/hook-decision-types.ts index cedf2371a64..4f908a3ee07 100644 --- a/src/plugins/hook-decision-types.ts +++ b/src/plugins/hook-decision-types.ts @@ -28,10 +28,15 @@ export type HookDecisionBlock = { metadata?: Record; }; -export function resolveBlockMessage(decision: HookDecisionBlock): string { - return typeof decision.message === "string" && decision.message.trim() - ? decision.message - : DEFAULT_BLOCK_MESSAGE; +export function resolveBlockMessage( + decision: HookDecisionBlock, + params: { blockedBy?: string } = {}, +): string { + if (typeof decision.message === "string" && decision.message.trim()) { + return decision.message; + } + const blockedBy = params.blockedBy?.trim(); + return blockedBy ? `${DEFAULT_BLOCK_MESSAGE} by ${blockedBy}` : DEFAULT_BLOCK_MESSAGE; } /** Outcome severity for most-restrictive-wins merging. Higher = more restrictive. */