fix: address remaining code review issues from PR #39#43
Conversation
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>
| return registeredMcpTools.has(name) | ||
| } | ||
|
|
||
| export const Resource = z |
There was a problem hiding this comment.
praise: Much better than the startsWith("mcp__") heuristic. A maintained Set of actually-registered tool names is the correct approach — no naming convention assumptions.
| return template.messages | ||
| .map((message) => (message.content.type === "text" ? message.content.text : "")) | ||
| .join("\n") | ||
| }) |
There was a problem hiding this comment.
praise: Replacing the new Promise(async ...) anti-pattern with a clean .then() chain is the right fix. The explicit error on undefined template prevents silent blank prompts.
| @@ -796,6 +808,7 @@ | |||
There was a problem hiding this comment.
thought: beforeExit fires when the event loop drains but NOT on SIGINT/SIGTERM/process.exit(). If the main concern is Ctrl+C, you may also need a SIGINT handler. However, if the normal session flow already emits session_end in the finally block, this beforeExit handler mainly covers edge cases where the process exits without going through the session loop — which is reasonable.
There was a problem hiding this comment.
Good observation. The session loop registers an AbortController on SIGINT, so Ctrl+C triggers abort.aborted → loop breaks → finally block runs → session_end. The beforeExit handler specifically covers the edge case where the process drains its event loop without entering the session loop (e.g. early initialization failure). process.exit() is a gap but we'd need process.on('exit', ...) with a sync flush for that — not worth the complexity right now.
jontsai
left a comment
jontsai
left a comment
There was a problem hiding this comment.
PR #43 Review Tour Guide
File Order
| # | File | +/- | Purpose |
|---|---|---|---|
| 1 | src/mcp/index.ts |
+12/-3 | MCP tool type detection + timeout on listPrompts/listResources |
| 2 | src/command/index.ts |
+11/-15 | Fix Promise anti-pattern in MCP prompt template getter |
| 3 | src/session/processor.ts |
+6/-5 | Use MCP.isMcpTool(), fix error truncation, remove cache_write from tokens |
| 4 | src/session/prompt.ts |
+13/-0 | session_end on beforeExit, cleanup handler |
| 5 | src/telemetry/index.ts |
+17/-2 | Dropped events counter, session_id fallback, cloud.role fix |
| 6 | src/session/compaction.ts |
+5/-1 | Clean up compactionAttempts map on completion/error |
| 7 | src/tool/truncation.ts |
+6/-1 | try/catch around Identifier.timestamp() |
| 8 | src/tool/project-scan.ts |
+4/-1 | Populate skill_count from Skill.all() |
| 9-10 | docs | +2/-1 | Config path fix, telemetry field in schema table |
Stop 1: MCP Tool Detection — isMcpTool() (mcp/index.ts) — CRITICAL FIX
The old startsWith("mcp__") heuristic was broken because MCP tool names are prefixed with the sanitized client name, not "mcp__". A maintained Set<string> of registered tool names is the correct approach. The set is cleared and rebuilt on each tools() call.
Review: ✅ Correct fix. No naming convention assumptions.
Stop 2: MCP Timeout on listPrompts/listResources (mcp/index.ts) — CRITICAL FIX
Wrapped client.listPrompts() and client.listResources() in withTimeout(_, 30s). Without this, a stalled MCP server would hang the CLI indefinitely during command loading.
Review: ✅ Correct. 30s default is reasonable.
Stop 3: Promise Anti-Pattern Fix (command/index.ts)
Replaced new Promise(async (resolve, reject) => { ... }) with clean .then() chain. Now throws on undefined template instead of resolving with empty string.
Review: ✅ Textbook fix. The old pattern could swallow errors.
Stop 4: Error Truncation Standardization (processor.ts)
Changed (value.error as any).toString() to value.error instanceof Error ? value.error.message : String(value.error). .toString() on Error includes the stack trace; .message does not.
Review: ✅ Important for privacy — stack traces can contain file paths.
Stop 5: Context Utilization Fix (processor.ts)
Removed cache_write from total token count. Cache writes are an API billing concept, not context window consumption.
Review: ✅ Correct — context utilization should reflect actual window usage.
Stop 6: session_end on beforeExit (prompt.ts)
Added process.once("beforeExit", handler) with cleanup in finally. See inline comment about SIGINT coverage.
Review: ✅ Reasonable for edge cases. Main path already covered by finally block.
Stop 7: Telemetry Improvements (telemetry/index.ts)
"startup"fallback for empty session_id"altimate"cloud role (was"altimate-code")- Dropped events counter + overflow error event on flush
Review: ✅ All good improvements. Dropped events tracking is nice for observability.
Stop 8: Defensive Cleanup (compaction.ts, truncation.ts, project-scan.ts)
compactionAttempts.delete()prevents memory leak on long-running sessions- try/catch in
Truncate.cleanup()prevents one bad filename from aborting cleanup skill_countpopulated fromSkill.all()
Review: ✅ All defensive improvements, no functional risk.
Summary Checklist
| Area | Status | Notes |
|---|---|---|
| MCP tool detection | ✅ Fixed | Set-based lookup replaces broken prefix check |
| MCP timeout | ✅ Fixed | 30s timeout prevents CLI hang |
| Promise anti-pattern | ✅ Fixed | Clean .then() chain, explicit error |
| Error privacy | ✅ Fixed | .message instead of .toString() |
| Token counting | ✅ Fixed | cache_write excluded |
| Memory cleanup | ✅ Fixed | compactionAttempts map cleaned up |
| Docs | ✅ Fixed | Config path corrected |
Thorough follow-up addressing all remaining review items. LGTM, ship it! 🚢
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>
* 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>
2 participants
Summary
Addresses all remaining issues from the comprehensive code review on PR #39. PR #40 fixed some issues; this PR covers the rest.
Critical (1)
listPrompts()/listResources()now wrapped inwithTimeout()— prevents CLI hang when MCP server stallsMajor (3)
MCP.isMcpTool()with a maintained Set, replacing brokenstartsWith("mcp__")prefix check that caused all MCP tools to be tracked as "standard"new Promise(async ...)anti-pattern with.then()chain; now throws on undefined template instead of silently returning blankTruncate.cleanup()— wrappedIdentifier.timestamp()per-entry in try/catch so a single malformed filename no longer aborts entire cleanupMinor (8)
session_idin early events — fallback to"startup"instead of empty stringsession_endon early termination — addedprocess.once("beforeExit")handlercompactionAttemptsunbounded growth — delete session entry on completion/errorai.cloud.roletag — changed from"altimate-code"to"altimate".message(not.toString()) to avoid leaking stack tracesskill_count— populated fromSkill.all()instead of hardcoded0cache_writefrom token count (not part of context window consumption)Docs
~/.config/altimate/config.json→~/.config/altimate-code/altimate-code.json)telemetryfield to config schema table in config docsAlready fixed in PR #40 (verified)
ALTIMATE_TELEMETRY_DISABLEDalready implementedTest plan
bun teston telemetry, command-resilience, truncation, compaction)"mcp"type (not"standard")listPrompts()/listResources()timeout after 30s on unresponsive serversession_endfires on Ctrl+C🤖 Generated with Claude Code