Skip to content

Commit 6671ee4

Browse files
anandgupta42claude
anandgupta42
and
claude
authored
fix: address remaining code review issues from PR #39 (#43)
* fix: address remaining code review issues from PR #39 Fixes 12 issues identified in the comprehensive code review of PR #39: Critical: - Wrap MCP listPrompts/listResources in withTimeout() to prevent CLI hangs Major: - Fix MCP tool type detection: add MCP.isMcpTool() instead of broken startsWith("mcp__") prefix check - Fix MCP prompt template getter: replace Promise constructor anti-pattern with .then() chain, throw on undefined template - Fix Truncate.cleanup: wrap Identifier.timestamp() per-entry in try/catch to prevent single malformed filename from aborting cleanup Minor: - Fix ai.cloud.role tag: "altimate-code" → "altimate" - Fix error truncation: use .message instead of .toString() to avoid leaking stack traces with file paths - Fix skill_count: populate from Skill.all() instead of hardcoded 0 - Fix context utilization: remove cache_write from token count - Fix buffer overflow: add dropped events counter for telemetry - Fix compactionAttempts: delete session entry on completion/error - Fix empty session_id: use "startup" fallback for early events - Fix session_end: add beforeExit handler for early process termination Docs: - Fix config path in telemetry docs (was ~/.config/altimate/config.json) - Add telemetry field to config schema table Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * fix: improve session_end coverage for process.exit() and add dedup guard Address review comment about beforeExit limitations: - Add process.once("exit") handler to cover process.exit() calls - Add dedup flag to prevent double-firing when finally block also runs - Document why SIGINT/SIGTERM are NOT handled (abort controller already triggers loop exit → finally block → normal session_end) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com>
1 parent ed6d5b3 commit 6671ee4

10 files changed

Lines changed: 86 additions & 29 deletions

File tree

‎docs/docs/configure/config.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -57,6 +57,7 @@ Configuration is loaded from multiple sources, with later sources overriding ear
5757
| `skills` | `object` | Skill paths and URLs |
5858
| `plugin` | `string[]` | Plugin specifiers |
5959
| `instructions` | `string[]` | Glob patterns for instruction files |
60+
| `telemetry` | `object` | Telemetry settings (see [Telemetry](telemetry.md)) |
6061
| `compaction` | `object` | Context compaction settings (see [Context Management](context-management.md)) |
6162
| `experimental` | `object` | Experimental feature flags |
6263

‎docs/docs/configure/telemetry.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -47,7 +47,7 @@ Telemetry helps us:
4747

4848
## Disabling Telemetry
4949

50-
To disable all telemetry collection, add this to your configuration file (`~/.config/altimate/config.json`):
50+
To disable all telemetry collection, add this to your configuration file (`~/.config/altimate-code/altimate-code.json`):
5151

5252
```json
5353
{

‎packages/altimate-code/src/command/index.ts‎

Lines changed: 11 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -118,21 +118,17 @@ export namespace Command {
118118
source: "mcp",
119119
description: prompt.description,
120120
get template() {
121-
// since a getter can't be async we need to manually return a promise here
122-
return new Promise<string>(async (resolve, reject) => {
123-
const template = await MCP.getPrompt(
124-
prompt.client,
125-
prompt.name,
126-
prompt.arguments
127-
? // substitute each argument with $1, $2, etc.
128-
Object.fromEntries(prompt.arguments?.map((argument, i) => [argument.name, `$${i + 1}`]))
129-
: {},
130-
).catch(reject)
131-
resolve(
132-
template?.messages
133-
.map((message) => (message.content.type === "text" ? message.content.text : ""))
134-
.join("\n") || "",
135-
)
121+
return MCP.getPrompt(
122+
prompt.client,
123+
prompt.name,
124+
prompt.arguments
125+
? Object.fromEntries(prompt.arguments.map((argument, i) => [argument.name, `$${i + 1}`]))
126+
: {},
127+
).then((template) => {
128+
if (!template) throw new Error(`Failed to load MCP prompt: ${prompt.name}`)
129+
return template.messages
130+
.map((message) => (message.content.type === "text" ? message.content.text : ""))
131+
.join("\n")
136132
})
137133
},
138134
hints: prompt.arguments?.map((_, i) => `$${i + 1}`) ?? [],

‎packages/altimate-code/src/mcp/index.ts‎

Lines changed: 12 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,12 @@ export namespace MCP {
2929
const log = Log.create({ service: "mcp" })
3030
const DEFAULT_TIMEOUT = 30_000
3131

32+
const registeredMcpTools = new Set<string>()
33+
34+
export function isMcpTool(name: string): boolean {
35+
return registeredMcpTools.has(name)
36+
}
37+
3238
export const Resource = z
3339
.object({
3440
name: z.string(),
@@ -215,7 +221,7 @@ export namespace MCP {
215221

216222
// Helper function to fetch prompts for a specific client
217223
async function fetchPromptsForClient(clientName: string, client: Client) {
218-
const prompts = await client.listPrompts().catch((e) => {
224+
const prompts = await withTimeout(client.listPrompts(), DEFAULT_TIMEOUT).catch((e) => {
219225
log.error("failed to get prompts", { clientName, error: e.message })
220226
return undefined
221227
})
@@ -237,7 +243,7 @@ export namespace MCP {
237243
}
238244

239245
async function fetchResourcesForClient(clientName: string, client: Client) {
240-
const resources = await client.listResources().catch((e) => {
246+
const resources = await withTimeout(client.listResources(), DEFAULT_TIMEOUT).catch((e) => {
241247
log.error("failed to get prompts", { clientName, error: e.message })
242248
return undefined
243249
})
@@ -683,6 +689,7 @@ export namespace MCP {
683689
}),
684690
)
685691

692+
registeredMcpTools.clear()
686693
for (const { clientName, client, toolsResult } of toolsResults) {
687694
if (!toolsResult) continue
688695
const mcpConfig = config[clientName]
@@ -691,7 +698,9 @@ export namespace MCP {
691698
for (const mcpTool of toolsResult.tools) {
692699
const sanitizedClientName = clientName.replace(/[^a-zA-Z0-9_-]/g, "_")
693700
const sanitizedToolName = mcpTool.name.replace(/[^a-zA-Z0-9_-]/g, "_")
694-
result[sanitizedClientName + "_" + sanitizedToolName] = await convertMcpTool(mcpTool, client, timeout)
701+
const toolName = sanitizedClientName + "_" + sanitizedToolName
702+
registeredMcpTools.add(toolName)
703+
result[toolName] = await convertMcpTool(mcpTool, client, timeout)
695704
}
696705
}
697706
return result

‎packages/altimate-code/src/session/compaction.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -298,8 +298,12 @@ When constructing the summary, try to stick to this template:
298298
},
299299
})
300300
}
301-
if (processor.message.error) return "stop"
301+
if (processor.message.error) {
302+
compactionAttempts.delete(input.sessionID)
303+
return "stop"
304+
}
302305
Bus.publish(Event.Compacted, { sessionID: input.sessionID })
306+
compactionAttempts.delete(input.sessionID)
303307
return "continue"
304308
}
305309

‎packages/altimate-code/src/session/processor.ts‎

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -16,6 +16,7 @@ import { SessionCompaction } from "./compaction"
1616
import { PermissionNext } from "@/permission/next"
1717
import { Question } from "@/question"
1818
import { Telemetry } from "@/telemetry"
19+
import { MCP } from "@/mcp"
1920

2021
export namespace SessionProcessor {
2122
const DOOM_LOOP_THRESHOLD = 3
@@ -212,7 +213,7 @@ export namespace SessionProcessor {
212213
attachments: value.output.attachments,
213214
},
214215
})
215-
const toolType = match.tool.startsWith("mcp__") ? "mcp" as const : "standard" as const
216+
const toolType = MCP.isMcpTool(match.tool) ? "mcp" as const : "standard" as const
216217
Telemetry.track({
217218
type: "tool_call",
218219
timestamp: Date.now(),
@@ -241,14 +242,14 @@ export namespace SessionProcessor {
241242
state: {
242243
status: "error",
243244
input: value.input ?? match.state.input,
244-
error: (value.error as any).toString(),
245+
error: (value.error instanceof Error ? value.error.message : String(value.error)).slice(0, 1000),
245246
time: {
246247
start: match.state.time.start,
247248
end: Date.now(),
248249
},
249250
},
250251
})
251-
const errToolType = match.tool.startsWith("mcp__") ? "mcp" as const : "standard" as const
252+
const errToolType = MCP.isMcpTool(match.tool) ? "mcp" as const : "standard" as const
252253
Telemetry.track({
253254
type: "tool_call",
254255
timestamp: Date.now(),
@@ -261,7 +262,7 @@ export namespace SessionProcessor {
261262
duration_ms: Date.now() - match.state.time.start,
262263
sequence_index: toolCallCounter,
263264
previous_tool: previousTool,
264-
error: (value.error as any).toString().slice(0, 500),
265+
error: (value.error instanceof Error ? value.error.message : String(value.error)).slice(0, 500),
265266
})
266267
toolCallCounter++
267268
previousTool = match.tool
@@ -345,7 +346,7 @@ export namespace SessionProcessor {
345346
duration_ms: Date.now() - stepStartTime,
346347
})
347348
// Context utilization tracking
348-
const totalTokens = usage.tokens.input + usage.tokens.output + usage.tokens.cache.read + usage.tokens.cache.write
349+
const totalTokens = usage.tokens.input + usage.tokens.output + usage.tokens.cache.read
349350
const contextLimit = input.model.limit?.context ?? 0
350351
if (contextLimit > 0) {
351352
const cacheRead = usage.tokens.cache.read

‎packages/altimate-code/src/session/prompt.ts‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -303,6 +303,27 @@ export namespace SessionPrompt {
303303
const session = await Session.get(sessionID)
304304
await Telemetry.init()
305305
Telemetry.setContext({ sessionId: sessionID, projectId: Instance.project?.id ?? "" })
306+
let emergencySessionEndFired = false
307+
const emergencySessionEnd = () => {
308+
if (emergencySessionEndFired) return
309+
emergencySessionEndFired = true
310+
Telemetry.track({
311+
type: "session_end",
312+
timestamp: Date.now(),
313+
session_id: sessionID,
314+
total_cost: sessionTotalCost,
315+
total_tokens: sessionTotalTokens,
316+
tool_call_count: toolCallCount,
317+
duration_ms: Date.now() - sessionStartTime,
318+
})
319+
}
320+
// beforeExit covers event-loop drain without entering the session loop.
321+
// exit covers process.exit() calls (sync only — track() buffers, flush is best-effort).
322+
// SIGINT/SIGTERM are NOT handled here: the abort controller already triggers
323+
// loop exit → finally block → normal session_end. Adding signal handlers
324+
// would interfere with the default termination behavior.
325+
process.once("beforeExit", emergencySessionEnd)
326+
process.once("exit", emergencySessionEnd)
306327
try {
307328
while (true) {
308329
SessionStatus.set(sessionID, { type: "busy" })
@@ -796,6 +817,8 @@ export namespace SessionPrompt {
796817
}
797818
SessionCompaction.prune({ sessionID })
798819
} finally {
820+
process.removeListener("beforeExit", emergencySessionEnd)
821+
process.removeListener("exit", emergencySessionEnd)
799822
const outcome: "completed" | "abandoned" | "error" = abort.aborted
800823
? "abandoned"
801824
: sessionHadError

‎packages/altimate-code/src/telemetry/index.ts‎

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -298,6 +298,7 @@ export namespace Telemetry {
298298
let sessionId = ""
299299
let projectId = ""
300300
let appInsights: AppInsightsConfig | undefined
301+
let droppedEvents = 0
301302
let initPromise: Promise<void> | undefined
302303
let initDone = false
303304

@@ -345,9 +346,9 @@ export namespace Telemetry {
345346
time: new Date(timestamp).toISOString(),
346347
iKey: cfg.iKey,
347348
tags: {
348-
"ai.session.id": sid,
349+
"ai.session.id": sid || "startup",
349350
"ai.user.id": userEmail,
350-
"ai.cloud.role": "altimate-code",
351+
"ai.cloud.role": "altimate",
351352
"ai.application.ver": Installation.VERSION,
352353
},
353354
data: {
@@ -439,6 +440,7 @@ export namespace Telemetry {
439440
buffer.push(event)
440441
if (buffer.length > MAX_BUFFER_SIZE) {
441442
buffer.shift()
443+
droppedEvents++
442444
}
443445
}
444446

@@ -447,6 +449,18 @@ export namespace Telemetry {
447449

448450
const events = buffer.splice(0, buffer.length)
449451

452+
if (droppedEvents > 0) {
453+
events.push({
454+
type: "error",
455+
timestamp: Date.now(),
456+
session_id: sessionId,
457+
error_name: "TelemetryBufferOverflow",
458+
error_message: `${droppedEvents} events dropped due to buffer overflow`,
459+
context: "telemetry",
460+
} as Event)
461+
droppedEvents = 0
462+
}
463+
450464
const controller = new AbortController()
451465
const timeout = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS)
452466
try {
@@ -475,6 +489,7 @@ export namespace Telemetry {
475489
enabled = false
476490
appInsights = undefined
477491
buffer = []
492+
droppedEvents = 0
478493
sessionId = ""
479494
projectId = ""
480495
initPromise = undefined

‎packages/altimate-code/src/tool/project-scan.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import path from "path"
66
import { Telemetry } from "@/telemetry"
77
import { Config } from "@/config/config"
88
import { Flag } from "@/flag/flag"
9+
import { Skill } from "../skill"
910

1011
// --- Types ---
1112

@@ -572,6 +573,8 @@ export const ProjectScanTool = Tool.define("project_scan", {
572573
if (Flag.ALTIMATE_CLI_ENABLE_EXA) enabledFlags.push("exa")
573574
if (Flag.ALTIMATE_CLI_ENABLE_QUESTION_TOOL) enabledFlags.push("question_tool")
574575

576+
const skillCount = await Skill.all().then(s => s.length).catch(() => 0)
577+
575578
Telemetry.track({
576579
type: "environment_census",
577580
timestamp: Date.now(),
@@ -585,7 +588,7 @@ export const ProjectScanTool = Tool.define("project_scan", {
585588
dbt_test_count_bucket: dbtManifest ? Telemetry.bucketCount(dbtManifest.test_count) : "0",
586589
connection_sources: connectionSources,
587590
mcp_server_count: mcpServerCount,
588-
skill_count: 0,
591+
skill_count: skillCount,
589592
os: process.platform,
590593
feature_flags: enabledFlags,
591594
})

‎packages/altimate-code/src/tool/truncation.ts‎

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,12 @@ export namespace Truncate {
3737
const entries = await fs.readdir(DIR).catch(() => [] as string[])
3838
for (const entry of entries) {
3939
if (!entry.startsWith("tool_")) continue
40-
if (Identifier.timestamp(entry) >= cutoff) continue
40+
try {
41+
if (Identifier.timestamp(entry) >= cutoff) continue
42+
} catch {
43+
// Skip malformed IDs (e.g. legacy format or descending IDs)
44+
continue
45+
}
4146
await fs.unlink(path.join(DIR, entry)).catch(() => {})
4247
}
4348
}

0 commit comments

Comments
 (0)