fix: address critical and high review findings

- Fix /reset silent failure: deactivate session before vault save (C1)
- Wrap saveSessionToObsidian body in top-level try/catch so it never throws (C1)
- Fix computeMemoryPath: only replace slashes, not dots, to match Claude CLI (H1)
- Fix ActiveAdapters: derive from z.infer<typeof adaptersSchema> instead of duplicate interface (H2)
- Fix _retryDepth: use typed InternalHandleMessageContext, remove unsafe type casts (H3)
- Fix session expiry: remove dangerously broad string-match condition (H4)
- Fix retry limit path: send user-facing message and return instead of falling through (H4)
- Fix handleCompact: guard fallback AI call in its own try/catch (H6)
- Fix handleCompact JSDoc: summary goes to Obsidian; continuity comes from MEMORY.md (H10)
- Add tests: /compact no-session sends correct message (H7)
- Add tests: /setproject found/not-found paths (H7)
- Add tests: Telegram forum topic getConversationId with/without message_thread_id (H9)
This commit is contained in:
Rasmus Widing 2026-04-15 10:28:46 +03:00
parent 0016664362
commit aab0bb809e
4 changed files with 144 additions and 57 deletions

View file

@ -212,6 +212,26 @@ describe('TelegramAdapter', () => {
expect(() => adapter.getConversationId(ctx)).toThrow('No chat in context');
});
test('should return "chatId:threadId" for forum topic messages', () => {
const adapter = new TelegramAdapter('fake-token-for-testing');
const ctx = {
chat: { id: -1001234567890 },
message: { message_thread_id: 42 },
} as unknown as import('telegraf').Context;
expect(adapter.getConversationId(ctx)).toBe('-1001234567890:42');
});
test('should return plain chatId for messages without message_thread_id', () => {
const adapter = new TelegramAdapter('fake-token-for-testing');
const ctx = {
chat: { id: -1001234567890 },
message: {},
} as unknown as import('telegraf').Context;
expect(adapter.getConversationId(ctx)).toBe('-1001234567890');
});
});
describe('ensureThread', () => {

View file

@ -85,6 +85,11 @@ export interface OrchestratorCommands {
projectRegistration: ProjectRegistration | null;
}
/** Internal extension of HandleMessageContext that carries recursion depth for auto-compact retry. */
interface InternalHandleMessageContext extends HandleMessageContext {
readonly _retryDepth?: number;
}
// ─── Command Parsing ────────────────────────────────────────────────────────
/**
@ -885,8 +890,7 @@ export async function handleMessage(
// Auto-compact on expired session: save summary from messages, reset, and retry
const isSessionExpired =
err.message.includes('No conversation found with session ID') ||
err.message.includes('not a valid UUID') ||
(err.message.includes('session') && err.message.includes('not found'));
err.message.includes('not a valid UUID');
if (conversation && isSessionExpired) {
getLog().info({ conversationId }, 'session.expired_auto_compacting');
try {
@ -905,16 +909,19 @@ export async function handleMessage(
);
// Retry once (guard against infinite recursion)
const retryDepth =
((context as Record<string, unknown> | undefined)?._retryDepth as number | undefined) ??
0;
const retryDepth = (context as InternalHandleMessageContext | undefined)?._retryDepth ?? 0;
if (retryDepth > 0) {
getLog().error({ conversationId, retryDepth }, 'session.auto_compact_retry_limit');
await platform.sendMessage(
conversationId,
'Session expired and auto-recovery failed. Please use /reset to start a fresh session.'
);
return;
} else {
await handleMessage(platform, conversationId, message, {
...context,
_retryDepth: retryDepth + 1,
} as HandleMessageContext);
} as InternalHandleMessageContext);
return;
}
} catch (compactError) {
@ -1452,12 +1459,13 @@ async function saveSessionLogToVault(
/**
* Compute the path to Claude Code's per-project memory directory.
* CLI encodes the CWD by replacing '/' and spaces with '-' as the project folder name.
* CLI encodes the CWD by replacing '/' with '-' as the project folder name.
* Dots and spaces are preserved to match the Claude CLI exactly.
* Example: /Users/anton/Claude workspace/ai-ofm
* ~/.claude/projects/-Users-anton-Claude-workspace-ai-ofm/memory/
* ~/.claude/projects/-Users-anton-Claude workspace-ai-ofm/memory/
*/
export function computeMemoryPath(cwd: string): string {
const encoded = cwd.replace(/[/. ]/g, '-');
const encoded = cwd.replace(/\//g, '-');
const home = process.env.HOME ?? '';
return join(home, '.claude', 'projects', encoded, 'memory');
}
@ -1516,42 +1524,47 @@ async function persistConversationMessages(
* Returns the vault path on success, null if no messages or on failure.
*/
async function saveSessionToObsidian(conversation: Conversation): Promise<string | null> {
const messages = await messageDb.listMessages(conversation.id, 50);
if (messages.length === 0) return null;
const codebase = conversation.codebase_id
? await codebaseDb.getCodebase(conversation.codebase_id)
: null;
if (!codebase) return null;
const transcript = messages.map(m => `[${m.role}]: ${m.content.slice(0, 500)}`).join('\n\n');
const aiClient = getAgentProvider(conversation.ai_assistant_type);
const cwd = conversation.cwd ?? getArchonWorkspacesPath();
let summary = '';
try {
for await (const chunk of aiClient.sendQuery(
`Summarize this conversation transcript concisely. Include: key decisions, current state of work, important context, and pending items. Output ONLY the summary, no preamble.\n\n---\n\n${transcript}`,
cwd,
undefined,
{ nodeConfig: { allowed_tools: [] } }
)) {
if (chunk.type === 'assistant') summary += chunk.content;
const messages = await messageDb.listMessages(conversation.id, 50);
if (messages.length === 0) return null;
const codebase = conversation.codebase_id
? await codebaseDb.getCodebase(conversation.codebase_id)
: null;
if (!codebase) return null;
const transcript = messages.map(m => `[${m.role}]: ${m.content.slice(0, 500)}`).join('\n\n');
const aiClient = getAgentProvider(conversation.ai_assistant_type);
const cwd = conversation.cwd ?? getArchonWorkspacesPath();
let summary = '';
try {
for await (const chunk of aiClient.sendQuery(
`Summarize this conversation transcript concisely. Include: key decisions, current state of work, important context, and pending items. Output ONLY the summary, no preamble.\n\n---\n\n${transcript}`,
cwd,
undefined,
{ nodeConfig: { allowed_tools: [] } }
)) {
if (chunk.type === 'assistant') summary += chunk.content;
}
} catch (error) {
getLog().warn({ err: error as Error }, 'session.summary_generation_failed');
return null;
}
if (!summary.trim()) return null;
return await saveSessionLogToVault(
getProjectSlug(codebase),
summary.trim(),
conversation.platform_type,
conversation.title
);
} catch (error) {
getLog().warn({ err: error as Error }, 'session.summary_generation_failed');
getLog().warn({ err: error as Error }, 'session.obsidian_save_failed');
return null;
}
if (!summary.trim()) return null;
return saveSessionLogToVault(
getProjectSlug(codebase),
summary.trim(),
conversation.platform_type,
conversation.title
);
}
/**
@ -1569,10 +1582,17 @@ async function handleResetWithSessionLog(
return;
}
// Save session log to Obsidian before resetting
const vaultPath = await saveSessionToObsidian(conversation);
// Deactivate session first so /reset never silently fails if vault save throws
await sessionDb.deactivateSession(session.id, 'reset-requested');
// Save session log to Obsidian (best-effort — vault save must not block reset)
let vaultPath: string | null = null;
try {
vaultPath = await saveSessionToObsidian(conversation);
} catch (vaultError) {
getLog().warn({ err: toError(vaultError), conversationId }, 'session.obsidian_save_failed');
}
const logNote = vaultPath ? `\nSession log saved to Obsidian: ${vaultPath}` : '';
await platform.sendMessage(
conversationId,
@ -1582,8 +1602,8 @@ async function handleResetWithSessionLog(
/**
* Handle /compact command.
* Summarizes the current conversation via AI, saves the summary, and resets the session.
* Next message will include the summary as context for continuity.
* Summarizes the current conversation via AI, saves the summary to the Obsidian vault, and resets
* the session. Context continuity on the next message comes from MEMORY.md, not from this summary.
*/
async function handleCompact(
platform: IPlatformAdapter,
@ -1629,12 +1649,18 @@ async function handleCompact(
const fallbackPrompt = `Summarize this conversation transcript. Include: key decisions, current state of work, important context, and pending items. Be concise but complete. Output ONLY the summary.\n\n---\n\n${transcript}`;
for await (const chunk of aiClient.sendQuery(fallbackPrompt, cwd, undefined, {
nodeConfig: { allowed_tools: [] },
})) {
if (chunk.type === 'assistant') {
summary += chunk.content;
try {
for await (const chunk of aiClient.sendQuery(fallbackPrompt, cwd, undefined, {
nodeConfig: { allowed_tools: [] },
})) {
if (chunk.type === 'assistant') {
summary += chunk.content;
}
}
} catch {
getLog().warn({ conversationId }, 'session.compact_fallback_failed');
await platform.sendMessage(conversationId, 'Failed to generate summary. Session not reset.');
return;
}
}

View file

@ -1493,4 +1493,52 @@ describe('orchestrator-agent handleMessage', () => {
expect(mockGenerateAndSetTitle).not.toHaveBeenCalled();
});
});
// ─── /compact ─────────────────────────────────────────────────────────────
describe('/compact', () => {
test('sends "No active session to compact." when no active session exists', async () => {
mockGetActiveSession.mockResolvedValueOnce(null);
await handleMessage(platform, 'chat-456', '/compact');
expect(platform.sendMessage).toHaveBeenCalledWith(
'chat-456',
'No active session to compact.'
);
});
});
// ─── /setproject ──────────────────────────────────────────────────────────
describe('/setproject', () => {
test('updates conversation when codebase is found', async () => {
mockListCodebases.mockResolvedValueOnce([mockCodebase]);
mockParseCommand.mockReturnValueOnce({ command: 'setproject', args: ['test-project'] });
await handleMessage(platform, 'chat-456', '/setproject test-project');
expect(mockUpdateConversation).toHaveBeenCalledWith(
mockConversation.id,
expect.objectContaining({ codebase_id: mockCodebase.id })
);
expect(platform.sendMessage).toHaveBeenCalledWith(
'chat-456',
expect.stringContaining('test-project')
);
});
test('sends error containing "not found" when project name does not match', async () => {
mockListCodebases.mockResolvedValueOnce([mockCodebase]);
mockParseCommand.mockReturnValueOnce({ command: 'setproject', args: ['unknown-project'] });
await handleMessage(platform, 'chat-456', '/setproject unknown-project');
expect(mockUpdateConversation).not.toHaveBeenCalled();
expect(platform.sendMessage).toHaveBeenCalledWith(
'chat-456',
expect.stringContaining('not found')
);
});
});
});

View file

@ -861,14 +861,7 @@ const getUpdateCheckRoute = createRoute({
* Register all /api/* routes on the Hono app.
*/
/** Which platform adapters are currently active (instantiated and started). */
export interface ActiveAdapters {
slack: boolean;
telegram: boolean;
discord: boolean;
github: boolean;
gitea: boolean;
gitlab: boolean;
}
export type ActiveAdapters = z.infer<typeof adaptersSchema>;
export function registerApiRoutes(
app: OpenAPIHono,