From db488afb7c89b4afd25daf8d37af88b42df72471 Mon Sep 17 00:00:00 2001 From: Aaron Sachs <898627+asachs01@users.noreply.github.com> Date: Sun, 30 Aug 2026 00:57:11 +0000 Subject: [PATCH] fix(bus): clean CLI error for kb-delete --scope private with no agent; add exec timeout deleteFromKnowledgeBase already threw a plain Error when --scope private resolved with no agent (--agent/CTX_AGENT_NAME), but the kb-delete CLI action had no pre-check for it -- unlike the sibling --org guard in the same handler -- so it bubbled up as a raw Node stack trace instead of a clean ERROR/exit(1). Mirrors the --org guard's shape exactly. Also added a 30s execFileSync timeout to deleteFromKnowledgeBase's mmrag.py call, matching queryKnowledgeBase's flat local-only timeout (delete is disk I/O only, no outbound network like ingest's Gemini calls, so it doesn't need ingest's floor/default/env-override machinery -- the simpler sibling pattern is the right fit). Follow-ups from dev's PR #157 review (task_1787750905384_03501051). --- src/bus/knowledge-base.ts | 6 +++- src/cli/bus.ts | 18 +++++++++-- tests/unit/bus/knowledge-base.test.ts | 45 ++++++++++++++++++++++++++- 3 files changed, 65 insertions(+), 4 deletions(-) diff --git a/src/bus/knowledge-base.ts b/src/bus/knowledge-base.ts index 9e7f17fa6a..305f4719bb 100644 --- a/src/bus/knowledge-base.ts +++ b/src/bus/knowledge-base.ts @@ -378,10 +378,14 @@ export function deleteFromKnowledgeBase( console.log(`Deleting from collection: ${collection}`); console.log(` Source: ${sourcePath}`); + // delete's mmrag.py path is local disk I/O only (chromadb, no outbound + // network/Gemini calls), so it doesn't need ingest's minutes-scale + // floor/default/env-override machinery — mirrors queryKnowledgeBase's + // flat 30s timeout above, the other local-only call in this file. execFileSync( pythonPath, [mmragPath, 'delete', sourcePath, '--collection', collection], - { encoding: 'utf-8', env, stdio: 'inherit' }, + { encoding: 'utf-8', env, stdio: 'inherit', timeout: 30000 }, ); } diff --git a/src/cli/bus.ts b/src/cli/bus.ts index a99fad0edc..5bbfaa2ed3 100644 --- a/src/cli/bus.ts +++ b/src/cli/bus.ts @@ -1691,10 +1691,24 @@ busCommand process.exit(1); } + const scope = (opts.scope as 'shared' | 'private') || 'shared'; + const agent = opts.agent || env.agentName; + // deleteFromKnowledgeBase throws a plain Error for this same condition, + // which — uncaught here — dumps a raw Node stack trace instead of a + // clean CLI error. Mirror the --org guard above rather than let it + // bubble: an operator running kb-delete --scope private without an + // agent resolved (no --agent, no CTX_AGENT_NAME) should see the same + // one-line ERROR/exit(1) shape every other missing-required-value case + // in this command gets. + if (scope === 'private' && !agent) { + console.error('ERROR: --agent or CTX_AGENT_NAME required for --scope private'); + process.exit(1); + } + deleteFromKnowledgeBase(sourcePath, { org, - agent: opts.agent || env.agentName, - scope: (opts.scope as 'shared' | 'private') || 'shared', + agent, + scope, frameworkRoot: env.frameworkRoot || process.cwd(), instanceId: env.instanceId, }); diff --git a/tests/unit/bus/knowledge-base.test.ts b/tests/unit/bus/knowledge-base.test.ts index c0c24e9383..cce69a79c6 100644 --- a/tests/unit/bus/knowledge-base.test.ts +++ b/tests/unit/bus/knowledge-base.test.ts @@ -37,7 +37,7 @@ vi.mock('../../../src/utils/org.js', () => ({ normalizeOrgName: (_root: string, org: string) => org, })); -const { queryKnowledgeBase, ingestKnowledgeBase } = await import('../../../src/bus/knowledge-base.js'); +const { queryKnowledgeBase, ingestKnowledgeBase, deleteFromKnowledgeBase } = await import('../../../src/bus/knowledge-base.js'); // Minimal BusPaths stub — knowledge-base.ts doesn't actually USE the paths // object at call time, just the options/env it constructs. @@ -143,6 +143,49 @@ describe('ingestKnowledgeBase — graceful missing-config', () => { }); }); +describe('deleteFromKnowledgeBase', () => { + it('missing config: warn + return cleanly, execFileSync NEVER called', () => { + mockMissingKbConfig(); + + expect(() => + deleteFromKnowledgeBase('/some/file.md', baseOptions), + ).not.toThrow(); + + expect(execFileSyncMock).not.toHaveBeenCalled(); + expect(warnLog.some((m) => m.includes('TestOrg') && /run setup/i.test(m))).toBe(true); + }); + + it('config present: execFileSync IS called with the mmrag delete args and a timeout', () => { + mockConfiguredKb(); + execFileSyncMock.mockReturnValue(''); + + deleteFromKnowledgeBase('/some/file.md', baseOptions); + + expect(execFileSyncMock).toHaveBeenCalledTimes(1); + const [pythonPath, argv, execOpts] = execFileSyncMock.mock.calls[0] as [string, string[], { timeout?: number }]; + expect(String(pythonPath)).toMatch(/python/); + expect(argv).toEqual(expect.arrayContaining(['delete', '/some/file.md'])); + // Local disk I/O only (no Gemini/network calls like ingest) — a flat + // bounded timeout, not ingest's minutes-scale floor/default/env knob. + expect(execOpts.timeout).toBe(30000); + }); + + it('--scope private with no agent throws a plain Error, not an execFileSync crash', () => { + mockConfiguredKb(); + + // This is the condition the CLI layer (src/cli/bus.ts kb-delete action) + // pre-checks with console.error+process.exit(1) before ever calling + // this function — asserting it here locks the underlying contract the + // CLI guard depends on: a clean, catchable Error, not a raw child-process + // crash from execFileSync running with an unresolvable collection name. + expect(() => + deleteFromKnowledgeBase('/some/file.md', { ...baseOptions, agent: undefined, scope: 'private' }), + ).toThrow('--agent or CTX_AGENT_NAME required for --scope private'); + + expect(execFileSyncMock).not.toHaveBeenCalled(); + }); +}); + describe('queryKnowledgeBase — graceful missing-config', () => { it('missing config: warn + return empty KBQueryResponse, execFileSync NEVER called', () => { mockMissingKbConfig();