From bf0fda9dca05580041ef92e530fe49536b335815 Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Thu, 3 Sep 2026 11:23:31 -0700 Subject: [PATCH] fix(hardening): 503 means unavailable, and an unrouted quote-reply says so (TASK-099 sites 4, 7) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two remaining sites from the silent-failure sweep, both instances of the same discriminator: a failure returning a value the caller cannot tell from success. Site 4 — GET /api/summaries/all-posts. #1501 stopped the summarizer fabricating filler and made the handler fail closed with 503, but unconditionally: every throw out of summarizeAllPosts reported as a transient outage, so a TypeError in the post mapping told the caller to retry an endpoint that is permanently broken. The two causes the route's own comment names — the rate-limit cooldown throw, and a generateText failure — are now tagged at the throw site with code 'summary_unavailable', and only those 503. Everything else is 500. The LLM error's message is interpolated into the wrapper rather than replaced, because the outer catch still scans that string for '429' / 'Resource exhausted' to arm the cooldown. Site 7 — routeReplyContent. The class's non-catch member: no exception, so a catch-keyed scan cannot find it. It answered {content, routedAgent: null} for two conditions that are not the same fact — "this was not a quote-reply" and "this WAS one and the relayMap did not contain it". The map is capped at RELAY_MAP_CAP and Telegram scrollback outlives it, so a human long-pressing an older relayed line expressed routing intent that degraded into an unaddressed broadcast with nothing to show for it. A third field, replyStatus, separates them, and the inbound relay logs the miss with the tgMessageId and map size. ROUTING IS UNCHANGED: an unmatched quote-reply still relays, still unaddressed. Only the reporting is new. Residue, named not built: the operator can now see an unmatched quote-reply; the Telegram user still cannot. Telling them changes what lands in the pod, which is a product call rather than a silent-failure fix. Verified: 57/57 across the seven touched suites under node@22, including the service-tier summaries suite whose end-to-end 503 assertion still passes through the real code path. Both fixes proved by mutation — reverting the route to an unconditional 503 reds the two 500 cases; rethrowing the LLM error untagged reds the service tagging case; collapsing the unmatched branch back onto 'not-a-reply' reds three. Backend typecheck: 50 errors, all pre-existing, none in any changed file. Co-Authored-By: Claude Opus 5 --- .../routes/summaries.allPostsFailure.test.js | 134 ++++++++++++++++++ .../services/llmFallbacks.failClosed.test.ts | 50 +++++++ .../services/telegramBridgeService.test.js | 67 +++++++++ backend/routes/summaries.ts | 27 +++- backend/services/summarizerService.ts | 40 +++++- backend/services/telegramBridgeService.ts | 54 +++++-- 6 files changed, 357 insertions(+), 15 deletions(-) create mode 100644 backend/__tests__/unit/routes/summaries.allPostsFailure.test.js diff --git a/backend/__tests__/unit/routes/summaries.allPostsFailure.test.js b/backend/__tests__/unit/routes/summaries.allPostsFailure.test.js new file mode 100644 index 000000000..80774bd11 --- /dev/null +++ b/backend/__tests__/unit/routes/summaries.allPostsFailure.test.js @@ -0,0 +1,134 @@ +// TASK-099 site 4 — follow-up to #1501. +// +// #1501 stopped `GET /api/summaries/all-posts` fabricating a filler summary +// and made the handler fail closed with 503. It did so UNCONDITIONALLY, so +// every throw out of `summarizeAllPosts` — a Mongo error, a TypeError in the +// post mapping, anything — reported as a transient outage. A 503 is an +// instruction to retry, and retrying a code defect never succeeds. +// +// These two cases differ only in the error the service throws. Before the +// fix both answered 503; the second is the one that must not. + +jest.mock('../../../middleware/auth', () => (req, res, next) => { + req.user = { id: 'user1' }; + req.userId = 'user1'; + next(); +}); + +const mockSummarizeAllPosts = jest.fn(); +jest.mock('../../../services/summarizerService', () => { + // Mirror the real module: it exports the singleton AND the marker constant + // the route matches on. A suite that mocks only the method leaves the + // constant undefined, which is exactly the regression the route's `||` + // fallback exists to stop — covered below. + class SummaryUnavailableError extends Error { + constructor(message) { + super(message); + this.name = 'SummaryUnavailableError'; + this.code = 'summary_unavailable'; + } + } + return { + summarizeAllPosts: (...args) => mockSummarizeAllPosts(...args), + SUMMARY_UNAVAILABLE: 'summary_unavailable', + SummaryUnavailableError, + constructor: { getRecentSummaries: jest.fn(), garbageCollectForDigest: jest.fn() }, + }; +}); + +jest.mock('../../../services/chatSummarizerService', () => ({ + getMultiplePodSummaries: jest.fn(), + summarizePodMessages: jest.fn(), + constructor: { getRecentChatSummariesByPodType: jest.fn(), getLatestPodSummary: jest.fn() }, +})); +jest.mock('../../../services/schedulerService', () => ({ + getStatus: jest.fn(), + constructor: { triggerSummarizer: jest.fn(), summarizeIntegrationBuffers: jest.fn(), dispatchPodSummaryRequests: jest.fn() }, +})); +jest.mock('../../../services/dailyDigestService', () => ({ + generateUserDailyDigest: jest.fn(), + generateAllDailyDigests: jest.fn().mockResolvedValue([]), +})); +jest.mock('../../../services/agentEventService', () => ({ enqueue: jest.fn() })); +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { find: jest.fn().mockReturnValue({ select: jest.fn().mockReturnValue({ lean: jest.fn().mockResolvedValue([]) }) }) }, +})); +jest.mock('../../../services/dmService', () => ({ canViewPod: jest.fn() })); +jest.mock('../../../models/Pod'); +jest.mock('../../../models/User'); +jest.mock('../../../models/Summary'); + +const request = require('supertest'); +const express = require('express'); + +const { SummaryUnavailableError } = require('../../../services/summarizerService'); +const routes = require('../../../routes/summaries'); + +describe('GET /api/summaries/all-posts — 503 means unavailable, not broken', () => { + let app; + let errorSpy; + + beforeEach(() => { + app = express(); + app.use(express.json()); + app.use('/api/summaries', routes); + jest.clearAllMocks(); + errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + }); + + afterEach(() => errorSpy.mockRestore()); + + it('answers 503 when the LLM is unavailable', async () => { + mockSummarizeAllPosts.mockRejectedValue( + new SummaryUnavailableError('All-posts summary generation failed at the LLM: connect ECONNREFUSED'), + ); + + const res = await request(app).get('/api/summaries/all-posts'); + + expect(res.status).toBe(503); + expect(res.body).toEqual({ error: 'summary_unavailable' }); + }); + + it('answers 503 when the rate-limit cooldown is armed', async () => { + mockSummarizeAllPosts.mockRejectedValue( + new SummaryUnavailableError('All-posts summary generation is cooling down after an LLM rate limit'), + ); + + const res = await request(app).get('/api/summaries/all-posts'); + + expect(res.status).toBe(503); + }); + + it('answers 500 — not 503 — for a code defect inside the summarizer', async () => { + // The measured case: injecting `(undefined).boom()` at the top of + // summarizeAllPosts used to return `503 summary_unavailable`. + mockSummarizeAllPosts.mockRejectedValue( + new TypeError("Cannot read properties of undefined (reading 'boom')"), + ); + + const res = await request(app).get('/api/summaries/all-posts'); + + expect(res.status).toBe(500); + expect(res.body).not.toEqual({ error: 'summary_unavailable' }); + }); + + it('answers 500 for a datastore failure, which is not this endpoint being unavailable', async () => { + // A Post.find() failure is a dependency outage, but it is not the one the + // 503 sentence claims, and it is not something the caller fixes by + // retrying this route. Fail closed loudly rather than mislabel it. + mockSummarizeAllPosts.mockRejectedValue(new Error('MongoNetworkError: connection 3 to db timed out')); + + const res = await request(app).get('/api/summaries/all-posts'); + + expect(res.status).toBe(500); + }); + + it('still returns the summary on the success path', async () => { + mockSummarizeAllPosts.mockResolvedValue({ title: 'Community Overview • 3 recent posts', content: 'x', metadata: {} }); + + const res = await request(app).get('/api/summaries/all-posts'); + + expect(res.status).toBe(200); + expect(res.body.title).toBe('Community Overview • 3 recent posts'); + }); +}); diff --git a/backend/__tests__/unit/services/llmFallbacks.failClosed.test.ts b/backend/__tests__/unit/services/llmFallbacks.failClosed.test.ts index 7bd3a4265..6670044dc 100644 --- a/backend/__tests__/unit/services/llmFallbacks.failClosed.test.ts +++ b/backend/__tests__/unit/services/llmFallbacks.failClosed.test.ts @@ -68,3 +68,53 @@ describe('LLM summaries fail closed', () => { .rejects.toThrow('LLM unavailable'); }); }); + +// TASK-099 site 4. Failing closed is the first half; saying WHICH failure is +// the second. `GET /api/summaries/all-posts` turns a throw from here into a +// 503, and 503 is an instruction to retry — correct for an LLM outage, +// actively wrong for a defect in this method. The route reads `.code`, so the +// tag has to be applied at the throw site, and only at the two causes the +// route's own comment names. +describe('summarizeAllPosts tags the causes that are genuinely unavailability', () => { + const postsQuery = (result) => { + const query = { + populate: jest.fn(), + sort: jest.fn(), + limit: jest.fn(), + lean: jest.fn(), + }; + query.populate.mockReturnValue(query); + query.sort.mockReturnValue(query); + query.limit.mockReturnValue(query); + if (result instanceof Error) query.lean.mockRejectedValue(result); + else query.lean.mockResolvedValue(result); + return query; + }; + const onePost = [{ content: 'A real post', tags: [], userId: { username: 'lily' } }]; + + beforeEach(() => { + jest.clearAllMocks(); + }); + + it("tags an LLM failure with code 'summary_unavailable'", async () => { + Post.find.mockReturnValueOnce(postsQuery(onePost)); + generateText.mockRejectedValueOnce(new Error('connect ECONNREFUSED')); + + const err = await summarizerService.summarizeAllPosts().catch((e) => e); + + expect(err.code).toBe('summary_unavailable'); + // The original message survives inside the wrapper: the outer catch still + // scans it for '429' / 'Resource exhausted' to arm the cooldown, so + // replacing the text rather than interpolating it would disarm that. + expect(err.message).toContain('connect ECONNREFUSED'); + }); + + it('leaves a datastore failure untagged, so the route reports it as a server error', async () => { + Post.find.mockReturnValueOnce(postsQuery(new Error('MongoNetworkError: connection 3 to db timed out'))); + + const err = await summarizerService.summarizeAllPosts().catch((e) => e); + + expect(err.message).toContain('MongoNetworkError'); + expect(err.code).toBeUndefined(); + }); +}); diff --git a/backend/__tests__/unit/services/telegramBridgeService.test.js b/backend/__tests__/unit/services/telegramBridgeService.test.js index 227797621..4d17c6f1d 100644 --- a/backend/__tests__/unit/services/telegramBridgeService.test.js +++ b/backend/__tests__/unit/services/telegramBridgeService.test.js @@ -93,4 +93,71 @@ describe('telegramBridgeService — quote-reply routing', () => { expect(out.routedAgent).toBeNull(); expect(out.content).toBe('plain message'); }); + + // TASK-099 site 7. The two cases below returned an identical value, so a + // caller could not tell "nobody quoted anything" from "someone quoted a + // relayed line and we failed to route it". Both still relay; only the + // second is a failure, and `replyStatus` is the only thing that says so. + describe('a quote-reply that does not route is distinguishable from no quote', () => { + it('reports not-a-reply when there was no quote', () => { + const out = routeReplyContent({ content: 'plain message', replyToTgMessageId: null, relayMap }); + expect(out.replyStatus).toBe('not-a-reply'); + expect(out.routedAgent).toBeNull(); + }); + + it('reports unmatched when the quoted message is absent from the relayMap', () => { + const out = routeReplyContent({ + content: 'unrelated reply', + replyToTgMessageId: '999', + relayMap, + }); + expect(out.replyStatus).toBe('unmatched'); + expect(out.routedAgent).toBeNull(); + // The routing behaviour is deliberately unchanged: still relayed, still + // unaddressed. Only the reporting is new. + expect(out.content).toBe('unrelated reply'); + }); + + it('reports unmatched when the entry aged out past RELAY_MAP_CAP', () => { + // The eviction case, which is why this is not a corner: the writer + // $slices the map to the newest 100 entries while Telegram scrollback + // keeps every relayed message long-pressable. + const evicted = Array.from({ length: 100 }, (_, i) => ({ + tgMessageId: String(1000 + i), + agentUsername: 'gene-fix-agent', + })); + const out = routeReplyContent({ + content: 'reply to something old', + replyToTgMessageId: '101', + relayMap: evicted, + }); + expect(out.replyStatus).toBe('unmatched'); + expect(out.routedAgent).toBeNull(); + }); + + it('reports unmatched when the integration has no relayMap at all', () => { + const out = routeReplyContent({ content: 'reply', replyToTgMessageId: '101', relayMap: undefined }); + expect(out.replyStatus).toBe('unmatched'); + }); + + it('reports unmatched when the matched entry carries no agentUsername', () => { + const out = routeReplyContent({ + content: 'reply', + replyToTgMessageId: '101', + relayMap: [{ tgMessageId: '101', agentUsername: null }], + }); + expect(out.replyStatus).toBe('unmatched'); + expect(out.routedAgent).toBeNull(); + }); + + it('reports routed on a hit', () => { + const out = routeReplyContent({ + content: 'looks wrong', + replyToTgMessageId: '101', + relayMap, + }); + expect(out.replyStatus).toBe('routed'); + expect(out.routedAgent).toBe('gene-fix-agent'); + }); + }); }); diff --git a/backend/routes/summaries.ts b/backend/routes/summaries.ts index efe00ece3..92e436764 100644 --- a/backend/routes/summaries.ts +++ b/backend/routes/summaries.ts @@ -2,6 +2,12 @@ const express = require('express'); // eslint-disable-next-line global-require const summarizerService = require('../services/summarizerService'); +// The `|| ...` is not defensive noise: several suites mock +// `services/summarizerService` wholesale, and a destructure of an absent +// export yields `undefined` — which would then match every error that carries +// no `code` and restore the exact unconditional-503 behaviour this replaces. +// eslint-disable-next-line global-require +const SUMMARY_UNAVAILABLE = require('../services/summarizerService').SUMMARY_UNAVAILABLE || 'summary_unavailable'; // eslint-disable-next-line global-require const Summary = require('../models/Summary'); // eslint-disable-next-line global-require @@ -257,10 +263,23 @@ router.get('/all-posts', summariesReadRateLimit, auth, async (_req: AuthReq, res const allPostsSummary = await summarizerService.summarizeAllPosts(); res.json(allPostsSummary); } catch (error) { - // Fail closed: the summarizer no longer fabricates filler when the LLM is - // unavailable, so this is a service-unavailable, not a server bug. - console.error('All posts summary unavailable:', (error as Error).message); - res.status(503).json({ error: 'summary_unavailable' }); + // Fail closed, and say WHICH failure. #1501 stopped the summarizer + // fabricating filler and made this a 503 — but unconditionally, so a + // defect inside `summarizeAllPosts` (a Mongo error, a TypeError in the + // post mapping) reported as a transient outage and told the caller to + // retry an endpoint that is permanently broken. 503 now means only what + // the sentence above claims: the LLM is unavailable, or the rate-limit + // cooldown is armed. Both are tagged at the throw site with + // `code: 'summary_unavailable'`. Anything else is a server bug. TASK-099 + // site 4, follow-up to #1501. + const code = (error as { code?: string })?.code; + if (code === SUMMARY_UNAVAILABLE) { + console.error('All posts summary unavailable:', (error as Error).message); + res.status(503).json({ error: 'summary_unavailable' }); + return; + } + console.error('Error generating all posts summary:', error); + res.status(500).json({ error: 'Failed to generate all posts summary' }); } }); diff --git a/backend/services/summarizerService.ts b/backend/services/summarizerService.ts index 78f56825d..9448f3c34 100644 --- a/backend/services/summarizerService.ts +++ b/backend/services/summarizerService.ts @@ -36,6 +36,32 @@ interface AllPostsSummary { }; } +// TASK-099 site 4. `GET /api/summaries/all-posts` reported EVERY throw out of +// `summarizeAllPosts` as `503 summary_unavailable`, so a code defect in this +// method was indistinguishable from the LLM being down — and 503 tells the +// caller to retry an endpoint that is permanently broken. Only the two causes +// the route's fail-closed comment actually names carry this marker: the +// rate-limit cooldown, and a `generateText` failure. Everything else (a Mongo +// error, a TypeError in the post mapping) stays an ordinary throw and the +// route answers 500. +// +// The route matches on `.code`, not `instanceof`: this module ships through +// the `module.exports = exports.default` CJS-compat shim at the foot of the +// file, and a duplicated module registration would break identity while a +// string property survives it. +export const SUMMARY_UNAVAILABLE = 'summary_unavailable'; + +export class SummaryUnavailableError extends Error { + code: string; + + constructor(message: string, options?: { cause?: unknown }) { + super(message); + this.name = 'SummaryUnavailableError'; + this.code = SUMMARY_UNAVAILABLE; + if (options && 'cause' in options) (this as { cause?: unknown }).cause = options.cause; + } +} + interface AllPostsCache { summary: AllPostsSummary | null; createdAt: number; @@ -394,10 +420,20 @@ Please create an engaging 3-4 sentence community overview that: This is for new visitors to understand what the community is all about. Focus on the content and conversations, not just statistics.`; if (now < cache.cooldownUntil) { - throw new Error('All-posts summary generation is cooling down after an LLM rate limit'); + throw new SummaryUnavailableError('All-posts summary generation is cooling down after an LLM rate limit'); } console.log('Generating all-posts summary with LLM...'); - const summaryText = await generateText(prompt, { temperature: 0.4 }) as string; + let summaryText: string; + try { + summaryText = await generateText(prompt, { temperature: 0.4 }) as string; + } catch (llmError) { + // Message is interpolated, not replaced: the outer catch below still + // scans it for '429'/'Resource exhausted' to arm the cooldown. + throw new SummaryUnavailableError( + `All-posts summary generation failed at the LLM: ${(llmError as Error)?.message}`, + { cause: llmError }, + ); + } console.log( `✓ LLM returned all-posts summary: "${summaryText.substring(0, 100)}..."`, ); diff --git a/backend/services/telegramBridgeService.ts b/backend/services/telegramBridgeService.ts index 2ad26a776..366832592 100644 --- a/backend/services/telegramBridgeService.ts +++ b/backend/services/telegramBridgeService.ts @@ -81,22 +81,46 @@ export const shouldEscalate = (opts: { // Prefix an inbound Telegram quote-reply with the @mention of the agent whose // relayed line was quoted, so the normal mention pipeline routes it. Pure — // unit-tested without any I/O. +// +// TASK-099 site 7. This is the class's non-catch member: no exception is +// thrown, so a catch-keyed sweep cannot find it. It used to answer +// `{ content, routedAgent: null }` for two conditions that are not the same +// fact — "this was not a quote-reply" and "this WAS a quote-reply and the +// relayMap did not contain it" — and the second is the failure. The map is +// capped at RELAY_MAP_CAP entries and Telegram scrollback outlives it, so a +// human long-pressing an older relayed line expresses routing intent that +// silently degrades into an unaddressed broadcast: nothing wakes, and the +// return value is byte-identical to the ordinary no-reply success. +// +// `replyStatus` is the discriminator. It changes no routing: an unmatched +// reply still relays, exactly as before. What changes is that the caller can +// now see it happened and says so in the log. +export type ReplyRouteStatus = 'not-a-reply' | 'routed' | 'unmatched'; + export const routeReplyContent = (opts: { content: string; replyToTgMessageId?: string | null; relayMap?: RelayMapEntry[]; -}): { content: string; routedAgent: string | null } => { +}): { content: string; routedAgent: string | null; replyStatus: ReplyRouteStatus } => { const { content, replyToTgMessageId, relayMap } = opts; - if (!replyToTgMessageId || !Array.isArray(relayMap)) { - return { content, routedAgent: null }; + if (!replyToTgMessageId) { + return { content, routedAgent: null, replyStatus: 'not-a-reply' }; + } + // A reply with no usable map is still a reply that failed to route — an + // integration whose relayMap was never written looks identical, from here, + // to one whose entry aged out. + if (!Array.isArray(relayMap)) { + return { content, routedAgent: null, replyStatus: 'unmatched' }; } const hit = relayMap.find((e) => String(e.tgMessageId) === String(replyToTgMessageId)); - if (!hit || !hit.agentUsername) return { content, routedAgent: null }; + if (!hit || !hit.agentUsername) { + return { content, routedAgent: null, replyStatus: 'unmatched' }; + } const mention = `@${hit.agentUsername}`; if (content.toLowerCase().includes(mention.toLowerCase())) { - return { content, routedAgent: hit.agentUsername }; + return { content, routedAgent: hit.agentUsername, replyStatus: 'routed' }; } - return { content: `${mention} ${content}`, routedAgent: hit.agentUsername }; + return { content: `${mention} ${content}`, routedAgent: hit.agentUsername, replyStatus: 'routed' }; }; const findLiveIntegration = async (podId: unknown): Promise => { @@ -185,7 +209,7 @@ export const relayTelegramMessageToPod = async (opts: { reply_to_message_id?: number; reply_to_message?: { message_id?: number }; }; -}): Promise<{ relayed: boolean; routedAgent?: string | null }> => { +}): Promise<{ relayed: boolean; routedAgent?: string | null; replyStatus?: ReplyRouteStatus }> => { const { integration, telegramMessage } = opts; const rawText = (telegramMessage.text || telegramMessage.caption || '').trim(); if (!rawText) return { relayed: false }; @@ -242,11 +266,23 @@ export const relayTelegramMessageToPod = async (opts: { const replyToTgMessageId = telegramMessage.reply_to_message?.message_id ?? telegramMessage.reply_to_message_id ?? null; - const { content: routedText, routedAgent } = routeReplyContent({ + const { content: routedText, routedAgent, replyStatus } = routeReplyContent({ content: rawText, replyToTgMessageId: replyToTgMessageId != null ? String(replyToTgMessageId) : null, relayMap: integration.config?.relayMap, }); + if (replyStatus === 'unmatched') { + // Loud, because the pod row itself will carry no trace: it lands with no + // mention and reads exactly like a message nobody was replying to. + const mapSize = Array.isArray(integration.config?.relayMap) + ? integration.config.relayMap.length + : 'absent'; + console.warn( + '[tg-bridge] quote-reply did not route — ' + + `tgMessageId=${replyToTgMessageId} not in relayMap ` + + `(entries=${mapSize}, cap=${RELAY_MAP_CAP}); relaying unaddressed`, + ); + } const senderName = [ telegramMessage.from?.first_name, @@ -346,7 +382,7 @@ export const relayTelegramMessageToPod = async (opts: { console.warn('[tg-bridge] socket emit failed:', (socketErr as Error).message); } - return { relayed: true, routedAgent }; + return { relayed: true, routedAgent, replyStatus }; }; module.exports = {