From 9a9023431ac779320a754b2c3063443f47d5719f Mon Sep 17 00:00:00 2001 From: Lily Shen <115414357+lilyshen0722@users.noreply.github.com> Date: Thu, 3 Sep 2026 06:15:55 -0700 Subject: [PATCH 1/2] fix(hardening): make six silent fallbacks distinguishable from success (TASK-099) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every site here returned, on failure, a value that is ALSO an ordinary success value — so the failure was unobservable by construction and no amount of logging at the throw site would have let a caller see it. That is the row's rule ("fails LOUD or fails CLOSED with a log, never a template") sharpened into a predicate: a fallback is a silent failure iff the value it returns is reachable on the SUCCESS path without being the documented fallback. - pg/Message.findActivityHint: `{count: 0}` is what a quiet pod returns. Adds `unavailable: true`, and schedulerService.buildHeartbeatActivityHint now reports `hasRecentActivity: null` (unknown) rather than `false` — this hint is shipped verbatim into the heartbeat prompt, so the old value told every agent in the pod that a Postgres outage was silence. A positive signal from the post arm still reads true; only a zero is unknowable. - skillsCatalogService.loadCatalog: `{items: []}` is what an empty catalog returns, and what the two "no catalog configured" guards above already return. Now throws on an unreadable or malformed file, which the route's existing handler turns into a 500 instead of 200 "no skills". - agentsRuntime context budget: `maxContextTokens: 0` means UNCAPPED to PodContextService, and had THREE producers — a config-store outage, an unconfigured contextLimit, and a malformed `?maxContextTokens=`. All three removed the budget and returned the context untrimmed, which is fail-OPEN in exactly the direction this row forbids. Malformed input is now a 400, the outage is logged (mirroring llmService, which logs the identical call), and the response carries `contextBudget: { applied, source }`. - telegramBridgeService.findLiveIntegration: `null` also means "no bridge for this pod", the modal case. Stays fail-closed; the swallow now logs. - agentAvatarService.parseDesignDescription: the only fully silent catch in that file. Logs, and tags the design so `metadata.designFallbackReason` distinguishes a parsed design from the hardcoded default — `fallbackUsed` could not, being true on the success path too. - agentInstallationCleanupService: `{marked: 0}` was a lower bound reported as a total. Both steps now return their failure counts. Tests pair each failure with its success TWIN and assert they differ; a test exercising only the failure arm would pass against the code being replaced. Verified by reverting the three service fixes: the four failure assertions go red, the success-twin control stays green. Inventory and site numbering: @sprint-review on TASK-099. Sites 4 (summaries 503), 7 (routeReplyContent, a control-flow miss with no catch) and 8 (13 floating `void` calls, which want the lint gate on TASK-121) are deliberately not in this PR. Co-Authored-By: Claude Opus 5 --- .../models/pg/message.activityHint.test.js | 21 ++- .../silentFailure.distinguishable.test.js | 160 ++++++++++++++++++ backend/models/pg/Message.ts | 9 +- backend/routes/agentsRuntime.ts | 46 ++++- backend/services/agentAvatarService.ts | 17 +- .../agentInstallationCleanupService.ts | 37 ++-- backend/services/schedulerService.ts | 22 ++- backend/services/skillsCatalogService.ts | 22 ++- backend/services/telegramBridgeService.ts | 8 +- 9 files changed, 311 insertions(+), 31 deletions(-) create mode 100644 backend/__tests__/unit/services/silentFailure.distinguishable.test.js diff --git a/backend/__tests__/unit/models/pg/message.activityHint.test.js b/backend/__tests__/unit/models/pg/message.activityHint.test.js index 2e15b0026..ac8694540 100644 --- a/backend/__tests__/unit/models/pg/message.activityHint.test.js +++ b/backend/__tests__/unit/models/pg/message.activityHint.test.js @@ -63,6 +63,25 @@ describe('Message.findActivityHint', () => { it('a query failure degrades to a zero hint rather than throwing', async () => { // The heartbeat's pod-selection pass must not die because the hint did. pool.query.mockRejectedValueOnce(new Error('connection terminated')); - await expect(Message.findActivityHint(POD, SINCE)).resolves.toEqual({ count: 0, lastAt: null }); + await expect(Message.findActivityHint(POD, SINCE)) + .resolves.toEqual({ count: 0, lastAt: null, unavailable: true }); + }); + + // TASK-099. Degrading is right; degrading INDISTINGUISHABLY is the defect. + // `count: 0` is exactly what a genuinely quiet pod returns, so without a + // discriminator the caller — and, through the heartbeat prompt, every agent — + // reads a Postgres outage as "nothing happened here". + it('a real zero and a failed zero are distinguishable', async () => { + pool.query.mockResolvedValueOnce({ rows: [{ count: '0', last_at: null }] }); + const quiet = await Message.findActivityHint(POD, SINCE); + + pool.query.mockRejectedValueOnce(new Error('connection terminated')); + const broken = await Message.findActivityHint(POD, SINCE); + + expect(quiet.count).toBe(0); + expect(broken.count).toBe(0); + expect(quiet.unavailable).toBeUndefined(); + expect(broken.unavailable).toBe(true); + expect(quiet).not.toEqual(broken); }); }); diff --git a/backend/__tests__/unit/services/silentFailure.distinguishable.test.js b/backend/__tests__/unit/services/silentFailure.distinguishable.test.js new file mode 100644 index 000000000..8c4155e0d --- /dev/null +++ b/backend/__tests__/unit/services/silentFailure.distinguishable.test.js @@ -0,0 +1,160 @@ +/** + * TASK-099 — silent-failure sweep. + * + * The row's rule: every fallback that hides an error fails LOUD or fails + * CLOSED with a log, never a template. + * + * The discriminator these tests encode, one level sharper than "does it + * log": a fallback is a SILENT FAILURE iff the value it returns is reachable + * on the SUCCESS path without being the documented fallback. `count: 0`, + * `items: []`, `marked: 0`, `null` and `maxContextTokens: 0` are all ordinary + * success values somewhere in this codebase, so returning one on failure + * makes the failure unobservable by construction — no amount of logging at + * the throw site changes what the CALLER can see. + * + * So each case below pairs the failure with its success TWIN and asserts the + * two differ. A test that only exercised the failure arm would pass against + * the very code these fixes replace. + */ + +const path = require('path'); + +describe('TASK-099 — a failed fallback is distinguishable from its success twin', () => { + describe('skillsCatalogService.loadCatalog', () => { + const CATALOG = '/tmp/task099-catalog.json'; + let fs; + let loadCatalog; + let invalidateCache; + + beforeEach(() => { + jest.resetModules(); + jest.doMock('fs', () => ({ + existsSync: jest.fn(), + statSync: jest.fn(), + readFileSync: jest.fn(), + })); + process.env.SKILLS_CATALOG_PATH = CATALOG; + fs = require('fs'); + ({ loadCatalog, invalidateCache } = require('../../../services/skillsCatalogService')); + invalidateCache(); + }); + + afterEach(() => { + delete process.env.SKILLS_CATALOG_PATH; + jest.dontMock('fs'); + }); + + it('returns an empty catalog when the file is legitimately absent', () => { + fs.existsSync.mockReturnValue(false); + expect(loadCatalog()).toEqual({ source: 'awesome', updatedAt: null, items: [] }); + }); + + it('THROWS on an unreadable file instead of returning that same empty catalog', () => { + fs.existsSync.mockReturnValue(true); + fs.statSync.mockReturnValue({ mtimeMs: 1 }); + fs.readFileSync.mockImplementation(() => { throw new Error('EACCES'); }); + // Previously this returned `{ items: [] }` and GET /api/skills/catalog + // answered 200 "no skills" for a catalog nobody could read. + expect(() => loadCatalog()).toThrow(/unreadable/i); + }); + + it('THROWS on malformed JSON too — a parse failure is not an empty catalog', () => { + fs.existsSync.mockReturnValue(true); + fs.statSync.mockReturnValue({ mtimeMs: 2 }); + fs.readFileSync.mockReturnValue('{ not json'); + expect(() => loadCatalog()).toThrow(/unreadable/i); + }); + }); + + describe('telegramBridgeService.findLiveIntegration', () => { + it('logs when the lookup THROWS, because null also means "no bridge here"', async () => { + jest.resetModules(); + const Integration = { findOne: jest.fn(), findByIdAndUpdate: jest.fn() }; + jest.doMock('../../../models/Integration', () => Integration); + jest.doMock('../../../services/telegramService', () => ({ sendMessage: jest.fn() })); + + const bridge = require('../../../services/telegramBridgeService'); + const errSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + + // Success twin: no live bridge for this pod. Must stay quiet. + Integration.findOne.mockReturnValue({ lean: () => Promise.resolve(null) }); + await bridge.relayAgentMessageToTelegram({ + podId: 'p1', agentUsername: 'a', displayName: 'A', content: '[DECISION] x', + }); + const quietCalls = errSpy.mock.calls.filter((c) => String(c[0]).includes('findLiveIntegration')); + expect(quietCalls).toHaveLength(0); + + // Failure arm: the store threw. Same `null`, but it must be audible. + Integration.findOne.mockReturnValue({ lean: () => Promise.reject(new Error('mongo down')) }); + await bridge.relayAgentMessageToTelegram({ + podId: 'p1', agentUsername: 'a', displayName: 'A', content: '[DECISION] x', + }); + const loudCalls = errSpy.mock.calls.filter((c) => String(c[0]).includes('findLiveIntegration')); + expect(loudCalls).toHaveLength(1); + + errSpy.mockRestore(); + }); + }); + + describe('agentAvatarService.parseDesignDescription', () => { + it('tags the default design so a parse failure is not reported as a clean SVG', () => { + jest.resetModules(); + const AgentAvatarService = require('../../../services/agentAvatarService'); + const svc = AgentAvatarService.default || AgentAvatarService; + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + + const parsed = svc.parseDesignDescription('noise {"style":"abstract","colors":["#111"]} tail'); + expect(parsed.fallbackReason).toBeUndefined(); + expect(warn).not.toHaveBeenCalled(); + + const fell = svc.parseDesignDescription('there is no json here at all'); + expect(fell.fallbackReason).toBe('design-parse-failed'); + expect(warn).toHaveBeenCalled(); + + warn.mockRestore(); + }); + }); + + describe('schedulerService.buildHeartbeatActivityHint', () => { + // This hint is shipped VERBATIM into the heartbeat payload the agent reads, + // so the pg arm's failure value does not stop at the service boundary — it + // becomes a sentence in a prompt. `hasRecentActivity: false` asserted from a + // failed read is the platform lying to every agent in the pod. + const buildHint = ({ pgHint, posts = [] }) => { + jest.resetModules(); + jest.doMock('../../../services/agentEventService', () => ({ enqueue: jest.fn() })); + jest.doMock('../../../models/pg/Message', () => ({ + findActivityHint: jest.fn().mockResolvedValue(pgHint), + })); + jest.doMock('../../../models/Post', () => ({ + aggregate: jest.fn().mockResolvedValue(posts), + })); + const instance = require('../../../services/schedulerService'); + const SchedulerService = instance.constructor; + return SchedulerService.buildHeartbeatActivityHint({ podId: 'p1', now: new Date() }); + }; + + it('a genuinely quiet pod reads false', async () => { + const hint = await buildHint({ pgHint: { count: 0, lastAt: null } }); + expect(hint.hasRecentActivity).toBe(false); + expect(hint.messageCountUnavailable).toBe(false); + }); + + it('an unreadable message store reads null — unknown, not quiet', async () => { + const hint = await buildHint({ pgHint: { count: 0, lastAt: null, unavailable: true } }); + expect(hint.hasRecentActivity).toBeNull(); + expect(hint.messageCountUnavailable).toBe(true); + }); + + it('a positive signal from the OTHER arm still reads true despite the failure', async () => { + // Only a zero is unknowable. Posts answered, so activity is established + // even though the message count is missing. + const hint = await buildHint({ + pgHint: { count: 0, lastAt: null, unavailable: true }, + posts: [{ _id: null, count: 3, lastAt: new Date() }], + }); + expect(hint.hasRecentActivity).toBe(true); + expect(hint.messageCountUnavailable).toBe(true); + }); + }); +}); diff --git a/backend/models/pg/Message.ts b/backend/models/pg/Message.ts index 281429e15..b86683676 100644 --- a/backend/models/pg/Message.ts +++ b/backend/models/pg/Message.ts @@ -41,6 +41,13 @@ interface FormattedMessage extends MessageRow { interface ActivityHintResult { count: number; lastAt: unknown; + /** + * True when the query FAILED and `count: 0` is therefore not a measurement. + * Without this the caller cannot tell a Postgres outage from a quiet pod — + * they return byte-identical values, and the quiet-pod reading is shipped + * into every agent's heartbeat prompt (schedulerService.buildHeartbeatActivityHint). + */ + unavailable?: boolean; } interface PodActivityEntry { @@ -477,7 +484,7 @@ class Message { } catch (error) { const e = error as { message?: string }; console.error('Error in findActivityHint:', e.message); - return { count: 0, lastAt: null }; + return { count: 0, lastAt: null, unavailable: true }; } } diff --git a/backend/routes/agentsRuntime.ts b/backend/routes/agentsRuntime.ts index 1dfb469e5..fe0f01d50 100644 --- a/backend/routes/agentsRuntime.ts +++ b/backend/routes/agentsRuntime.ts @@ -1491,16 +1491,42 @@ router.get('/pods/:podId/context', agentRuntimeAuth, async (req: any, res: any) return clamp(parsed, 1, max); }; - // Resolve context token budget from model config + // Resolve context token budget from model config. + // + // `maxContextTokens: 0` means UNCAPPED to PodContextService (its guard is + // `if (maxContextTokens > 0)`), so 0 must never also be a failure value — + // it previously had three producers that all landed there and all meant + // something different: a config-store outage, an unconfigured contextLimit, + // and a malformed `?maxContextTokens=`. Each removed the budget entirely + // and returned the whole context untrimmed, which is fail-OPEN. let maxContextTokens = 0; + let contextBudgetSource: 'query' | 'model-config' | 'unconfigured' | 'config-unavailable' = 'unconfigured'; if (req.query.maxContextTokens) { + // A caller who ASKED for a budget and mistyped it must not silently get + // no budget at all. parseLimit's NaN fallback is 0 = uncapped here. + const requested = Number.parseInt(req.query.maxContextTokens as string, 10); + if (Number.isNaN(requested)) { + return res.status(400).json({ error: 'maxContextTokens must be an integer' }); + } maxContextTokens = parseLimit(req.query.maxContextTokens, 0, 200000); + contextBudgetSource = 'query'; } else { - const modelConfig = await GlobalModelConfigService.getConfig().catch(() => null); - const contextLimit = modelConfig?.llmService?.contextLimit || 0; - // Reserve 25% of model context for system prompt + output - if (contextLimit > 0) { - maxContextTokens = Math.floor(contextLimit * 0.75); + const modelConfig = await GlobalModelConfigService.getConfig().catch((configErr: Error) => { + // Mirrors llmService.generateText, which logs the identical failure of + // the identical call. Silent here meant an outage was indistinguishable + // from "no contextLimit configured". + console.warn('[agents-runtime] Failed to load model config for context budget:', configErr?.message); + return null; + }); + if (!modelConfig) { + contextBudgetSource = 'config-unavailable'; + } else { + const contextLimit = modelConfig?.llmService?.contextLimit || 0; + // Reserve 25% of model context for system prompt + output + if (contextLimit > 0) { + maxContextTokens = Math.floor(contextLimit * 0.75); + contextBudgetSource = 'model-config'; + } } } @@ -1518,7 +1544,13 @@ router.get('/pods/:podId/context', agentRuntimeAuth, async (req: any, res: any) maxContextTokens, }); - return res.json(context); + // PodContextService attaches tokenEstimate/tokenBudget to `stats` only when + // a budget applied, so an absent budget was invisible to the caller AND + // unattributable. Say which of the four states produced it. + return res.json({ + ...(context as Record), + contextBudget: { applied: maxContextTokens > 0, source: contextBudgetSource }, + }); } catch (error: any) { let statusCode = 500; if (error.status) statusCode = error.status; diff --git a/backend/services/agentAvatarService.ts b/backend/services/agentAvatarService.ts index 137dca52a..719c2153f 100644 --- a/backend/services/agentAvatarService.ts +++ b/backend/services/agentAvatarService.ts @@ -159,6 +159,9 @@ class AgentAvatarService { source: 'svg', model: null, fallbackUsed: true, + // null when the model's design actually parsed. `fallbackUsed` alone + // could not distinguish these: it is set to true on this path either way. + designFallbackReason: (avatarDesign as { fallbackReason?: string })?.fallbackReason || null, }, }; } catch (error: any) { @@ -306,7 +309,7 @@ class AgentAvatarService { return this.parseDesignDescription(designDescription); } catch (error) { console.error('Error generating avatar design:', error); - return this.getDefaultDesign(style, colorScheme); + return { ...this.getDefaultDesign(style, colorScheme), fallbackReason: 'design-generation-failed' }; } } @@ -411,8 +414,16 @@ Keep it suitable for a clean vector avatar (flat shapes, gradients, and simple d } throw new Error('No JSON found in response'); } catch (error) { - // Fallback to default design - return this.getDefaultDesign('banana', 'vibrant'); + // Fallback to default design. This catch was previously the only fully + // silent one in this file: it never logged, and its throw never reached + // the outer catch at generateAvatar, so a hardcoded banana was returned + // under `metadata.fallbackUsed: true` — the same value a successful SVG + // generation sets. Log it, and TAG the design so the caller can say which. + console.warn( + '[agent-avatar] could not parse design description, using default:', + (error as Error).message, + ); + return { ...this.getDefaultDesign('banana', 'vibrant'), fallbackReason: 'design-parse-failed' }; } } diff --git a/backend/services/agentInstallationCleanupService.ts b/backend/services/agentInstallationCleanupService.ts index ac9b0c5e3..b70aae6fe 100644 --- a/backend/services/agentInstallationCleanupService.ts +++ b/backend/services/agentInstallationCleanupService.ts @@ -107,13 +107,13 @@ function hasRecentTokenUse( */ export async function markStaleInstallations( daysSinceLastEvent: number = DEFAULT_STALENESS_EVENT_DAYS, -): Promise<{ marked: number }> { +): Promise<{ marked: number; evaluationFailures: number }> { if (!Number.isFinite(daysSinceLastEvent) || daysSinceLastEvent <= 0) { console.warn( '[installation-cleanup] invalid daysSinceLastEvent, skipping mark step (value=%s)', daysSinceLastEvent, ); - return { marked: 0 }; + return { marked: 0, evaluationFailures: 0 }; } const cutoff = new Date(Date.now() - daysSinceLastEvent * 24 * 60 * 60 * 1000); @@ -125,7 +125,7 @@ export async function markStaleInstallations( .lean(); if (!activeInstalls.length) { - return { marked: 0 }; + return { marked: 0, evaluationFailures: 0 }; } // Dedup the (agentName, instanceId) pairs so we only do one User lookup and @@ -141,6 +141,10 @@ export async function markStaleInstallations( } const stalePairs = new Set(); + // A per-item swallow makes `marked` a LOWER BOUND reported as a total: a run + // that failed to evaluate every pair returns `{ marked: 0 }`, byte-identical + // to a clean run with nothing to do. Count the misses so the two differ. + let evaluationFailures = 0; for (const { agentName, instanceId } of uniquePairs.values()) { try { @@ -178,11 +182,12 @@ export async function markStaleInstallations( instanceId, (err as Error).message, ); + evaluationFailures += 1; } } if (!stalePairs.size) { - return { marked: 0 }; + return { marked: 0, evaluationFailures }; } // Build the OR filter once and updateMany — touches all pods of each stale @@ -207,7 +212,7 @@ export async function markStaleInstallations( ); const marked = result?.modifiedCount ?? result?.nModified ?? 0; - return { marked }; + return { marked, evaluationFailures }; } /** @@ -217,13 +222,13 @@ export async function markStaleInstallations( */ export async function pruneStaleInstallations( minStaleAgeDays: number = DEFAULT_PRUNE_AFTER_STALE_DAYS, -): Promise<{ deleted: number }> { +): Promise<{ deleted: number; failures: number }> { if (!Number.isFinite(minStaleAgeDays) || minStaleAgeDays <= 0) { console.warn( '[installation-cleanup] invalid minStaleAgeDays, skipping prune step (value=%s)', minStaleAgeDays, ); - return { deleted: 0 }; + return { deleted: 0, failures: 0 }; } const cutoff = new Date(Date.now() - minStaleAgeDays * 24 * 60 * 60 * 1000); @@ -236,10 +241,13 @@ export async function pruneStaleInstallations( .lean(); if (!prunable.length) { - return { deleted: 0 }; + return { deleted: 0, failures: 0 }; } let deleted = 0; + // Same reason as markStaleInstallations: `deleted: 0` must not mean both + // "nothing to prune" and "every prune threw". + let failures = 0; for (const inst of prunable) { try { // Remove the agent user from the pod's members array so it stops @@ -261,10 +269,11 @@ export async function pruneStaleInstallations( inst.instanceId, (err as Error).message, ); + failures += 1; } } - return { deleted }; + return { deleted, failures }; } export async function runCleanup(): Promise { @@ -291,9 +300,15 @@ export async function runCleanup(): Promise { `[installation-cleanup] running: mark stale after ${eventDays}d inactivity, prune after ${pruneDays}d stale`, ); const markResult = await markStaleInstallations(eventDays); - console.log(`[installation-cleanup] mark done: marked ${markResult.marked} install(s) stale`); + console.log( + `[installation-cleanup] mark done: marked ${markResult.marked} install(s) stale` + + `, ${markResult.evaluationFailures} pair(s) could not be evaluated`, + ); const pruneResult = await pruneStaleInstallations(pruneDays); - console.log(`[installation-cleanup] prune done: deleted ${pruneResult.deleted} stale install(s)`); + console.log( + `[installation-cleanup] prune done: deleted ${pruneResult.deleted} stale install(s)` + + `, ${pruneResult.failures} failed`, + ); } catch (err) { // Swallow so cron keeps running — never crash the host process from a // cleanup failure. Next run will retry. diff --git a/backend/services/schedulerService.ts b/backend/services/schedulerService.ts index 55e6e5034..9f1eda371 100644 --- a/backend/services/schedulerService.ts +++ b/backend/services/schedulerService.ts @@ -75,7 +75,15 @@ interface HeartbeatActivityHint { messageCount: number; postCount: number; totalSignals: number; - hasRecentActivity: boolean; + /** + * `null` means WE DO NOT KNOW, not "no activity". The chat-message arm reads + * Postgres and returns `count: 0` on failure, which is the same value a quiet + * pod returns; this hint is shipped verbatim into the heartbeat payload the + * agent reads, so a store outage used to tell every agent the pod was quiet. + */ + hasRecentActivity: boolean | null; + /** True when the message-count arm failed, so `messageCount` is not a measurement. */ + messageCountUnavailable: boolean; lastMessageAt: string | null; lastPostAt: string | null; generatedAt: string; @@ -884,7 +892,7 @@ class SchedulerService { const since = new Date(now.getTime() - (lookbackMinutes * 60 * 1000)); const [pgMsgHint, postStats]: [ - { count: number; lastAt?: string | Date }, + { count: number; lastAt?: string | Date; unavailable?: boolean }, Array<{ _id: unknown; count: number; lastAt?: Date }> ] = await Promise.all([ PGMessage.findActivityHint(podId, since), @@ -895,16 +903,24 @@ class SchedulerService { ]); const messageCount = pgMsgHint.count; + const messageCountUnavailable = pgMsgHint.unavailable === true; const postCount = Number(postStats?.[0]?.count || 0); const totalSignals = messageCount + postCount; + // A positive signal is still positive even if the other arm failed; only a + // ZERO is unknowable, because we did not manage to look. + const hasRecentActivity = totalSignals > 0 + ? true + : (messageCountUnavailable ? null : false); + return { lookbackMinutes, since: since.toISOString(), messageCount, postCount, totalSignals, - hasRecentActivity: totalSignals > 0, + hasRecentActivity, + messageCountUnavailable, lastMessageAt: pgMsgHint.lastAt ? new Date(pgMsgHint.lastAt).toISOString() : null, lastPostAt: postStats?.[0]?.lastAt ? new Date(postStats[0].lastAt).toISOString() : null, generatedAt: now.toISOString(), diff --git a/backend/services/skillsCatalogService.ts b/backend/services/skillsCatalogService.ts index 20bd56977..56d9f91ef 100644 --- a/backend/services/skillsCatalogService.ts +++ b/backend/services/skillsCatalogService.ts @@ -97,8 +97,15 @@ export const loadCatalog = (source = DEFAULT_SOURCE): Catalog => { }); return catalog; } catch (error) { - console.warn(`[skills-catalog] Failed to read ${catalogPath}:`, (error as Error).message); - return { source, updatedAt: null, items: [] }; + // FAIL LOUD. `{ items: [] }` is exactly what a legitimately empty catalog + // returns (and what the two guards above return for "no path configured" + // and "file absent"), so swallowing here answered `GET /api/skills/catalog` + // with 200 "no skills" for an unreadable or malformed catalog file. The + // route's existing catch turns this throw into a 500, which is the honest + // answer: we do not know what skills exist. + const message = (error as Error).message; + console.error(`[skills-catalog] Failed to read ${catalogPath}:`, message); + throw new Error(`skills catalog unreadable at ${catalogPath}: ${message}`); } }; @@ -108,8 +115,15 @@ export const loadCatalog = (source = DEFAULT_SOURCE): Catalog => { * "Last updated X minutes ago" indicator on the frontend. */ export const getLastRefreshedAt = (source = DEFAULT_SOURCE): { localRefreshedAt: string | null; upstreamRefreshedAt: string | null } => { - // Force a cache warm-up so we pick up the current on-disk values. - loadCatalog(source); + // Force a cache warm-up so we pick up the current on-disk values. This is a + // best-effort refresh of a display timestamp, not the catalog read itself — + // loadCatalog now throws on an unreadable file and that must surface from the + // CATALOG call, not from here, or a broken file would be reported twice. + try { + loadCatalog(source); + } catch { + // already logged inside loadCatalog; fall through to whatever is cached + } const cached = catalogCache.get(source); if (!cached) { return { localRefreshedAt: null, upstreamRefreshedAt: null }; diff --git a/backend/services/telegramBridgeService.ts b/backend/services/telegramBridgeService.ts index 603e03f7c..2ad26a776 100644 --- a/backend/services/telegramBridgeService.ts +++ b/backend/services/telegramBridgeService.ts @@ -112,7 +112,13 @@ const findLiveIntegration = async (podId: unknown): Promise Date: Thu, 3 Sep 2026 06:48:56 -0700 Subject: [PATCH 2/2] test(hardening): cover the two fixes that shipped unobservable (TASK-099) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit @sprint-review gated #1519 at `9a902343` and found by mutation that two of the six sites had no test asserting their failure is observable: deleting the new 400 on a malformed `maxContextTokens`, and stopping the cleanup sweep's failure counter, both left the suite fully green. That is this PR's own thesis one level up — a fallback nobody can observe, in the coverage rather than the code. Verified rather than taken on report: no test anywhere reached either site (the existing cleanup suite only ever destructures `marked`), so both mutations survived by construction. - `agentsRuntime.contextBudget.test.js` — 5 cases mounting the real router. The malformed param is a 400 and never reaches PodContextService; and all four budget states are pinned by the `contextBudget` field, including the pair that matters: an unconfigured `contextLimit` and a config-store OUTAGE both send `maxContextTokens: 0`, so the response field is the only thing that separates them. - `agentInstallationCleanup.failureCount.test.js` — 3 cases pairing a clean sweep (nothing stale) against one that threw on every pair. Both return `marked: 0`; only `evaluationFailures` tells them apart. Third case pins the partial, so the count is not rounded to either end. Re-ran both of their mutations against the new tests: each reddens, and the success-twin control in each file stays green. 20 suites / 133 tests green; `tsc --noEmit` still 51, the origin/main baseline. Co-Authored-By: Claude Opus 5 --- .../agentsRuntime.contextBudget.test.js | 113 ++++++++++++++++++ ...ntInstallationCleanup.failureCount.test.js | 91 ++++++++++++++ 2 files changed, 204 insertions(+) create mode 100644 backend/__tests__/unit/routes/agentsRuntime.contextBudget.test.js create mode 100644 backend/__tests__/unit/services/agentInstallationCleanup.failureCount.test.js diff --git a/backend/__tests__/unit/routes/agentsRuntime.contextBudget.test.js b/backend/__tests__/unit/routes/agentsRuntime.contextBudget.test.js new file mode 100644 index 000000000..2f639fb75 --- /dev/null +++ b/backend/__tests__/unit/routes/agentsRuntime.contextBudget.test.js @@ -0,0 +1,113 @@ +/** + * TASK-099 site 9 — `maxContextTokens: 0` means UNCAPPED to PodContextService, + * so it must not also be a failure value. + * + * These close a gap @sprint-review found by mutation on #1519: the new 400 and + * the new `contextBudget` field shipped with no test, so deleting either left + * the suite fully green. That is exactly the defect the PR is about — a + * fallback nobody can observe — one level up, in the coverage rather than the + * code. Each case below pairs the failure with the success value it collided + * with, the same shape as the service tests. + */ + +jest.mock('jsonwebtoken', () => ({ sign: jest.fn(), verify: jest.fn(), decode: jest.fn() })); + +jest.mock('../../../middleware/agentRuntimeAuth', () => (req, res, next) => { + req.agentUser = { _id: 'bot-1' }; + req.agentInstallations = [{ podId: 'pod-1', status: 'active', agentName: 'a', instanceId: 'i' }]; + req.agentAuthorizedPodIds = ['pod-1']; + next(); +}); +jest.mock('../../../middleware/auth', () => (req, res, next) => next()); +jest.mock('../../../middleware/apiTokenScopes', () => ({ + requireApiTokenScopes: () => (req, res, next) => next(), +})); + +jest.mock('../../../services/agentEventService', () => ({})); +jest.mock('../../../services/agentIdentityService', () => ({ + buildAgentUsername: jest.fn((a) => a), + getOrCreateAgentUser: jest.fn().mockResolvedValue({ _id: 'agent-user-1' }), + ensureAgentInPod: jest.fn().mockResolvedValue(undefined), +})); +jest.mock('../../../services/agentMessageService', () => ({ getRecentMessages: jest.fn() })); +jest.mock('../../../services/agentThreadService', () => ({})); +jest.mock('../../../services/podContextService', () => ({ + getPodContext: jest.fn().mockResolvedValue({ _status: 'success', stats: {} }), +})); +jest.mock('../../../services/globalModelConfigService', () => ({ getConfig: jest.fn() })); +jest.mock('../../../services/socialPolicyService', () => ({})); +jest.mock('../../../integrations', () => ({ get: jest.fn() })); +jest.mock('../../../models/Activity', () => ({})); +jest.mock('../../../models/User', () => ({ findById: jest.fn() })); +jest.mock('../../../models/Post', () => ({ findById: jest.fn() })); +jest.mock('../../../models/Pod', () => ({ find: jest.fn() })); +jest.mock('../../../services/dmService', () => ({ getOrCreateAgentDM: jest.fn() })); +jest.mock('../../../models/Integration', () => ({ find: jest.fn(), findOne: jest.fn() })); +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { findOne: jest.fn(), find: jest.fn() }, +})); + +const express = require('express'); +const request = require('supertest'); +const GlobalModelConfigService = require('../../../services/globalModelConfigService'); +const PodContextService = require('../../../services/podContextService'); +const router = require('../../../routes/agentsRuntime'); + +const app = express(); +app.use(express.json()); +app.use('/api/agents/runtime', router); + +const get = (qs = '') => request(app).get(`/api/agents/runtime/pods/pod-1/context${qs}`); +const budgetPassedToService = () => PodContextService.getPodContext.mock.calls[0][0].maxContextTokens; + +describe('GET /pods/:podId/context — the context budget (TASK-099 site 9)', () => { + beforeEach(() => { + jest.clearAllMocks(); + PodContextService.getPodContext.mockResolvedValue({ _status: 'success', stats: {} }); + }); + + it('a malformed maxContextTokens is a 400, not a silent uncap', async () => { + // parseLimit's NaN fallback is 0, and 0 means UNCAPPED downstream — so the + // pre-fix behaviour was to honour a typo by removing the budget entirely. + const res = await get('?maxContextTokens=abc'); + expect(res.status).toBe(400); + expect(res.body.error).toMatch(/maxContextTokens/); + expect(PodContextService.getPodContext).not.toHaveBeenCalled(); + }); + + it('a well-formed maxContextTokens is honoured and attributed to the query', async () => { + const res = await get('?maxContextTokens=8000'); + expect(res.status).toBe(200); + expect(budgetPassedToService()).toBe(8000); + expect(res.body.contextBudget).toEqual({ applied: true, source: 'query' }); + }); + + it('a configured contextLimit reserves 25% and says so', async () => { + GlobalModelConfigService.getConfig.mockResolvedValue({ llmService: { contextLimit: 100000 } }); + const res = await get(); + expect(res.status).toBe(200); + expect(budgetPassedToService()).toBe(75000); + expect(res.body.contextBudget).toEqual({ applied: true, source: 'model-config' }); + }); + + it('an UNCONFIGURED contextLimit is uncapped and named as unconfigured', async () => { + GlobalModelConfigService.getConfig.mockResolvedValue({ llmService: {} }); + const res = await get(); + expect(budgetPassedToService()).toBe(0); + expect(res.body.contextBudget).toEqual({ applied: false, source: 'unconfigured' }); + }); + + it('a config-store OUTAGE is uncapped too, and is distinguishable from unconfigured', async () => { + // Both send `maxContextTokens: 0`, which is the whole problem: the response + // field is the only thing that can tell an operator which one happened. + GlobalModelConfigService.getConfig.mockRejectedValue(new Error('mongo down')); + const warn = jest.spyOn(console, 'warn').mockImplementation(() => {}); + + const res = await get(); + expect(budgetPassedToService()).toBe(0); + expect(res.body.contextBudget).toEqual({ applied: false, source: 'config-unavailable' }); + expect(warn).toHaveBeenCalled(); + + warn.mockRestore(); + }); +}); diff --git a/backend/__tests__/unit/services/agentInstallationCleanup.failureCount.test.js b/backend/__tests__/unit/services/agentInstallationCleanup.failureCount.test.js new file mode 100644 index 000000000..6967f4e7d --- /dev/null +++ b/backend/__tests__/unit/services/agentInstallationCleanup.failureCount.test.js @@ -0,0 +1,91 @@ +/** + * TASK-099 site 3 — `{ marked: 0 }` must not mean both "nothing was stale" and + * "every staleness check threw". + * + * The per-item catch logs and continues, so the returned count is a LOWER BOUND + * reported as a total. That is loud at the log and silent at the return value, + * and the return value is what the runner prints and what a caller could gate on. + * + * Closes a gap @sprint-review found by mutation on #1519: stopping the failure + * counter left the suite green, because the existing staleness suite only ever + * destructures `marked`. + */ + +jest.mock('node-cron', () => ({ schedule: jest.fn() })); + +jest.mock('../../../models/AgentRegistry', () => ({ + AgentInstallation: { find: jest.fn(), updateMany: jest.fn(), deleteMany: jest.fn() }, +})); +jest.mock('../../../models/AgentEvent', () => ({ findOne: jest.fn() })); +jest.mock('../../../models/User', () => ({ findOne: jest.fn() })); +jest.mock('../../../models/Pod', () => ({ updateOne: jest.fn() })); +jest.mock('../../../services/agentIdentityService', () => ({ + buildAgentUsername: (name, instanceId) => `${name}__${instanceId}`, +})); + +const { AgentInstallation } = require('../../../models/AgentRegistry'); +const AgentEvent = require('../../../models/AgentEvent'); +const User = require('../../../models/User'); +const { markStaleInstallations } = require('../../../services/agentInstallationCleanupService'); + +const givenActiveInstalls = (n) => { + const rows = Array.from({ length: n }, (_, i) => ({ + _id: `i${i}`, agentName: 'telegram-app', instanceId: `inst-${i}`, podId: 'p1', + })); + AgentInstallation.find.mockReturnValue({ select: () => ({ lean: async () => rows }) }); +}; + +// A live token: unexpired, so the pair is spared and nothing is marked. This is +// the SUCCESS TWIN of the failure case — both return `marked: 0`. +const givenLiveToken = () => { + User.findOne.mockReturnValue({ + select: () => ({ lean: async () => ({ agentRuntimeTokens: [{ expiresAt: new Date(Date.now() + 864e5) }] }) }), + }); +}; + +beforeEach(() => { + jest.clearAllMocks(); + AgentInstallation.updateMany.mockResolvedValue({ modifiedCount: 0 }); + AgentEvent.findOne.mockReturnValue({ select: () => ({ sort: () => ({ lean: async () => null }) }) }); +}); + +describe('markStaleInstallations — a failed sweep differs from a clean one', () => { + it('a clean sweep with nothing stale reports zero failures', async () => { + givenActiveInstalls(2); + givenLiveToken(); + const result = await markStaleInstallations(7); + expect(result).toEqual({ marked: 0, evaluationFailures: 0 }); + }); + + it('a sweep that threw on every pair reports the same marked:0 AND its failures', async () => { + givenActiveInstalls(2); + const err = jest.spyOn(console, 'error').mockImplementation(() => {}); + User.findOne.mockImplementation(() => { throw new Error('mongo down'); }); + + const result = await markStaleInstallations(7); + + // The count alone is indistinguishable from the clean sweep above — which is + // why the second field has to exist. + expect(result.marked).toBe(0); + expect(result.evaluationFailures).toBe(2); + expect(err).toHaveBeenCalledTimes(2); + + err.mockRestore(); + }); + + it('a PARTIAL failure is reported as partial, not rounded to either end', async () => { + givenActiveInstalls(2); + const err = jest.spyOn(console, 'error').mockImplementation(() => {}); + let call = 0; + User.findOne.mockImplementation(() => { + call += 1; + if (call === 1) throw new Error('mongo down'); + return { select: () => ({ lean: async () => ({ agentRuntimeTokens: [{ expiresAt: new Date(Date.now() + 864e5) }] }) }) }; + }); + + const result = await markStaleInstallations(7); + + expect(result.evaluationFailures).toBe(1); + err.mockRestore(); + }); +});