From b429c94724a972faf34da4fbb66d29ac299a9a98 Mon Sep 17 00:00:00 2001 From: TerrifiedBug <35064668+TerrifiedBug@users.noreply.github.com> Date: Sun, 23 Aug 2026 09:32:24 +0000 Subject: [PATCH] fix: a failed target resolution names the rung that declined (#72) release 0.12.4 The refusal callers got was "no active telegram chat - pass chat_id": true, and unactionable. It could not separate "topics are off" from "a live claim exists but its session identity is not mine", and those want opposite responses. Measured on conductor#882, that ambiguity cost two 300-second arming attempts and a five-hour delivery hold while a matching claim sat in the registry the whole time. The ladder logged only its successes, at debug; the one case a human is debugging - `resolved === undefined` - logged nothing at all. So the ladder now reports why each rung declined, and the refusal carries it: no active telegram chat - pass chat_id. [telegram] telegram_send found no target: nothing inbound this turn; this session owns no topic; topic registry has 2 claim(s), none matching this session's identity; no DM owner pinned One line per failed call, never one per rung. Counts, never ids - the precedent is `resolveProjectTopicId`'s rule that a log line is a place ids leak from, and a test asserts no thread id, pid or session file appears. Four cases are now distinguishable that previously read identically: an empty registry, a registry for another chat, claims that exist but do not match this session, and a DM owner pinned to somebody else. Two existing assertions changed on purpose: they pinned the exact old string. Both now assert the actionable prefix plus the rung, and one of them pins the distinction it was blind to - "DM owner is another session" is not "no DM owner pinned". Verified against the fake #72 names: replacing the reasons with bare rung names ("topic registry") turns two of the three new tests red. bun run check: 317 pass, 0 fail. --- docs/guide.md | 2 +- package.json | 2 +- src/index.ts | 104 ++++++++++++++++++++++++++++++++------- src/index.wiring.test.ts | 72 +++++++++++++++++++++++++-- 4 files changed, 157 insertions(+), 23 deletions(-) diff --git a/docs/guide.md b/docs/guide.md index 4277ea1..5d08613 100644 --- a/docs/guide.md +++ b/docs/guide.md @@ -42,7 +42,7 @@ Install the published extension: ```bash omp plugin install omp-telegram -omp plugin list # → omp-telegram@0.12.3 +omp plugin list # → omp-telegram@0.12.4 ``` There is no build step and no runtime dependency install. The extension uses diff --git a/package.json b/package.json index 8ecfb3f..2352501 100644 --- a/package.json +++ b/package.json @@ -1,6 +1,6 @@ { "name": "omp-telegram", - "version": "0.12.3", + "version": "0.12.4", "description": "Standalone Telegram bridge, session topics, and herdr control plane for omp", "type": "module", "license": "MIT", diff --git a/src/index.ts b/src/index.ts index 4ae19a1..662a25e 100644 --- a/src/index.ts +++ b/src/index.ts @@ -922,15 +922,41 @@ export default function telegramExtension(pi: ExtensionAPI): void { } } - function resolveToolTarget( + /** + * The target, or why every rung declined (#72). + * + * `declines` exists because the refusal callers used to get — + * "no active telegram chat" — is true and unactionable: it cannot separate + * "topics are off" from "a live claim exists but its session identity is not + * mine", and those want opposite responses. Measured on conductor#882: that + * ambiguity cost two 300-second arming attempts and a five-hour delivery hold + * while a matching claim sat in the registry the whole time. + * + * Counts, never ids. `resolveProjectTopicId`'s rule holds here too: a log line + * is a place ids leak from. + */ + interface TargetResolution { + target?: ResolvedToolTarget; + declines: readonly string[]; + } + + function resolveTarget( ctx: ExtensionContext | undefined, currentAccess: Access = loadAccess(warn), - ): ResolvedToolTarget | undefined { + ): TargetResolution { + const declines: string[] = []; const active = outbound.lastTarget(); - if (active) return { ...active, source: "active inbound" }; + if (active) return { target: { ...active, source: "active inbound" }, declines: [] }; + declines.push("nothing inbound this turn"); + if (ownTopic && currentAccess.topicsChat) { - return { chatId: currentAccess.topicsChat, threadId: ownTopic.threadId, source: "session topic" }; + return { + target: { chatId: currentAccess.topicsChat, threadId: ownTopic.threadId, source: "session topic" }, + declines: [], + }; } + declines.push(currentAccess.topicsChat ? "this session owns no topic" : "topics are off"); + const current = ctx ?? lastCtx; const identity = { sessionId: current?.sessionManager?.getSessionId(), @@ -940,18 +966,37 @@ export default function telegramExtension(pi: ExtensionAPI): void { if (currentAccess.topicsChat) { const registry = loadRegistry(warn); if (registry.chatId === currentAccess.topicsChat) { - for (const [threadId, entry] of Object.entries(registry.threads)) { + const entries = Object.entries(registry.threads); + for (const [threadId, entry] of entries) { const belongs = hasIdentity ? sameSession(entry, identity) : entry.pid === process.pid; if (belongs) { - return { chatId: currentAccess.topicsChat, threadId: Number(threadId), source: "topic registry" }; + return { + target: { chatId: currentAccess.topicsChat, threadId: Number(threadId), source: "topic registry" }, + declines: [], + }; } } + // The case #882 could not diagnose, and the reason this whole list + // exists: claims are present, so the registry is fine and the bridge has + // run — the mismatch is identity, which points at a resumed session or a + // plugin too old to compare session files. + declines.push( + entries.length === 0 + ? "topic registry carries no claims" + : `topic registry has ${entries.length} claim(s), none matching this session's ` + + `${hasIdentity ? "identity" : "pid (no session identity available)"}`, + ); + } else { + declines.push("topic registry names a different chat"); } } + const dmOwner = loadDmOwner(warn); const dmChat = pairedOwnerId(currentAccess); const ownsDm = dmOwner && (hasIdentity ? sameSession(dmOwner, identity) : dmOwner.pid === process.pid); - return dmOwner && dmChat && ownsDm ? { chatId: dmChat, source: "DM owner" } : undefined; + if (dmOwner && dmChat && ownsDm) return { target: { chatId: dmChat, source: "DM owner" }, declines: [] }; + declines.push(dmOwner === undefined ? "no DM owner pinned" : "DM owner is another session"); + return { declines }; } function logTargetFallback(tool: string, resolved: ResolvedToolTarget | undefined): void { @@ -960,6 +1005,11 @@ export default function telegramExtension(pi: ExtensionAPI): void { } } + /** One line per failed call, naming every rung and why it declined (#72). */ + function targetFailureDetail(tool: string, declines: readonly string[]): string { + return `[telegram] ${tool} found no target: ${declines.join("; ")}`; + } + /** * Claim this session's forum topic once. Exact saved-session identity wins. * A missing remote topic is forgotten and replaced; otherwise create a topic. @@ -2004,12 +2054,16 @@ export default function telegramExtension(pi: ExtensionAPI): void { chatId = p.chat_id; threadId = p.thread_id != null && p.thread_id !== "" ? Number(p.thread_id) : undefined; } else { - const resolved = resolveToolTarget(ctx, currentAccess); - logTargetFallback("telegram_send", resolved); - chatId = resolved?.chatId; - threadId = resolved?.threadId; + const resolution = resolveTarget(ctx, currentAccess); + logTargetFallback("telegram_send", resolution.target); + chatId = resolution.target?.chatId; + threadId = resolution.target?.threadId; + if (chatId === undefined) { + const detail = targetFailureDetail("telegram_send", resolution.declines); + log.warn(detail); + return errorResult(`no active telegram chat — pass chat_id. ${detail}`); + } } - if (!chatId) return errorResult("no active telegram chat — pass chat_id"); assertAllowedChat(chatId, currentAccess); const replyTo = p.reply_to != null && p.reply_to !== "" ? Number(p.reply_to) : undefined; const ids: number[] = []; @@ -2058,15 +2112,23 @@ export default function telegramExtension(pi: ExtensionAPI): void { const questions: PromptQuestion[] = p.questions.map((q) => ({ ...q, options: q.options ?? [] })); const canTerminal = ctx?.hasUI === true && typeof ctx.ui?.askDialog === "function"; let resolved = activePromptTarget ? { ...activePromptTarget } : undefined; + let declines: readonly string[] = []; if (!resolved && token.length > 0) { const currentAccess = loadAccess(warn); - const fallback = resolveToolTarget(ctx, currentAccess); - logTargetFallback("telegram_ask", fallback); - resolved = buildPromptTarget(fallback, currentAccess); + const resolution = resolveTarget(ctx, currentAccess); + logTargetFallback("telegram_ask", resolution.target); + declines = resolution.declines; + resolved = buildPromptTarget(resolution.target, currentAccess); } const target = resolved; if (!target && !canTerminal) { - return errorResult("telegram_ask has no surface available — no Telegram target and no interactive terminal."); + // Name the rungs here too: "no surface available" reads as a config + // problem, and the common cause is a target that nearly resolved (#72). + const detail = declines.length === 0 ? "" : ` ${targetFailureDetail("telegram_ask", declines)}`; + if (detail.length > 0) log.warn(detail.trim()); + return errorResult( + `telegram_ask has no surface available — no Telegram target and no interactive terminal.${detail}`, + ); } const posted: AskSurfaceState = { terminal: false, telegram: false }; const surfaceErrors: AskSurfaceErrors = {}; @@ -2211,10 +2273,16 @@ export default function telegramExtension(pi: ExtensionAPI): void { const p = params as ReactParams; try { const currentAccess = loadAccess(warn); - const resolved = p.chat_id ? undefined : resolveToolTarget(ctx, currentAccess); + const resolution = p.chat_id ? undefined : resolveTarget(ctx, currentAccess); + const resolved = resolution?.target; logTargetFallback("telegram_react", resolved); const chatId = p.chat_id ?? resolved?.chatId; - if (!chatId) return errorResult("no active telegram chat — pass chat_id"); + if (!chatId) { + const detail = + resolution === undefined ? "" : ` ${targetFailureDetail("telegram_react", resolution.declines)}`; + if (detail.length > 0) log.warn(detail.trim()); + return errorResult(`no active telegram chat — pass chat_id.${detail}`); + } assertAllowedChat(chatId, currentAccess); await outbound.react(chatId, Number(p.message_id), p.emoji); return { content: [{ type: "text", text: "reacted" }] }; diff --git a/src/index.wiring.test.ts b/src/index.wiring.test.ts index 6611d62..77eb8e9 100644 --- a/src/index.wiring.test.ts +++ b/src/index.wiring.test.ts @@ -5,7 +5,7 @@ import { tmpdir } from "node:os"; import { basename, join } from "node:path"; import { type Access, defaultAccess, loadAccess } from "./access"; import telegramExtension from "./index"; -import { claimDmOwner, loadDmOwner, loadRegistry } from "./topics"; +import { claimDmOwner, loadDmOwner, loadRegistry, saveRegistry } from "./topics"; type EventHandler = (event: unknown, ctx: unknown) => unknown; type CommandHandler = (args: string, ctx: unknown) => unknown; @@ -402,7 +402,13 @@ describe("extension wiring", () => { { sessionManager: { getSessionId: () => "session-1", getSessionFile: () => "/tmp/session-1.jsonl" } }, ); expect(result.isError).toBe(true); - expect(result.content[0].text).toBe("no active telegram chat — pass chat_id"); + // The refusal still leads with the actionable instruction, and now names the + // rungs (#72). This case is "a DM owner exists but is somebody else" — + // distinct wording from "none pinned", which is the whole point: the two + // want opposite responses and used to read identically. + expect(result.content[0].text).toContain("no active telegram chat — pass chat_id"); + expect(result.content[0].text).toContain("DM owner is another session"); + expect(result.content[0].text).not.toContain("no DM owner pinned"); }); test("telegram_send still refuses when no bridge session has claimed a target", async () => { @@ -410,7 +416,67 @@ describe("extension wiring", () => { const h = harness(["ask"]); const result = await h.tools.get("telegram_send")!.execute("t", { text: "done" }, undefined, undefined, {}); expect(result.isError).toBe(true); - expect(result.content[0].text).toBe("no active telegram chat — pass chat_id"); + expect(result.content[0].text).toContain("no active telegram chat — pass chat_id"); + expect(result.content[0].text).toContain("no DM owner pinned"); + }); + + test("a live claim that is not this session's names the identity mismatch, and leaks no ids (#72)", async () => { + // The case conductor#882 could not diagnose. Claims are present, so the + // registry is fine and the bridge has run — the mismatch is identity, which + // points at a resumed session or a plugin too old to compare session files. + // Before this, it read exactly like "no claims at all". + writeAccess({ enabled: true, allowFrom: ["42"], topicsChat: "42" }); + saveRegistry({ + version: 1, + chatId: "42", + threads: { + "8801": { pid: 999_999, cwd: "/foreign", name: "foreign", claimedAt: 1, sessionId: "foreign-a", sessionFile: "/tmp/foreign-a.jsonl" }, + "8802": { pid: 999_998, cwd: "/other", name: "other", claimedAt: 2, sessionId: "foreign-b", sessionFile: "/tmp/foreign-b.jsonl" }, + }, + }); + const h = harness(["ask"]); + const result = await h.tools.get("telegram_send")!.execute("t", { text: "done" }, undefined, undefined, { + sessionManager: { getSessionId: () => "mine", getSessionFile: () => "/tmp/mine.jsonl" }, + }); + expect(result.isError).toBe(true); + const text = result.content[0].text as string; + expect(text).toContain("no active telegram chat — pass chat_id"); + // Distinguishable from an empty registry, and it says how many exist. + expect(text).toContain("topic registry has 2 claim(s), none matching this session's identity"); + expect(text).not.toContain("carries no claims"); + // Every rung, one line. + expect(text).toContain("nothing inbound this turn"); + expect(text).toContain("this session owns no topic"); + expect(text.split("\n")).toHaveLength(1); + // Counts, never ids: the precedent is resolveProjectTopicId's rule that a + // log line is a place ids leak from. + for (const id of ["8801", "8802", "999999", "999998", "/tmp/foreign-a.jsonl", "/tmp/mine.jsonl"]) { + expect(text).not.toContain(id); + } + }); + + test("an empty registry says so, rather than blaming identity (#72)", async () => { + writeAccess({ enabled: true, allowFrom: ["42"], topicsChat: "42" }); + saveRegistry({ version: 1, chatId: "42", threads: {} }); + const h = harness(["ask"]); + const result = await h.tools.get("telegram_send")!.execute("t", { text: "done" }, undefined, undefined, { + sessionManager: { getSessionId: () => "mine", getSessionFile: () => "/tmp/mine.jsonl" }, + }); + expect(result.content[0].text).toContain("topic registry carries no claims"); + expect(result.content[0].text).not.toContain("none matching"); + }); + + test("a registry for another chat is named as such, not as an identity problem (#72)", async () => { + writeAccess({ enabled: true, allowFrom: ["42"], topicsChat: "42" }); + saveRegistry({ + version: 1, + chatId: "999", + threads: { "8801": { pid: process.pid, cwd: "/here", name: "here", claimedAt: 1 } }, + }); + const h = harness(["ask"]); + const result = await h.tools.get("telegram_send")!.execute("t", { text: "done" }, undefined, undefined, {}); + expect(result.content[0].text).toContain("topic registry names a different chat"); + expect(result.content[0].text).not.toContain("999"); }); test("before_agent_start leaves ask untouched for a plain terminal turn with notify off", async () => {