Multi-Model Code Review — PR #35: feat/context-management-improvements

Reviewed by 8 models: Claude, Codex, Gemini, Kimi, Grok, MiniMax, GLM-5, Qwen. Convergence: 1 round.

Verdict: REQUEST CHANGES

• Critical: 1
• Major: 2
• Minor: 3
• Nit: 1

🔴 Critical

1. tokens.cache access crashes on sessions created before this PR

Location: packages/altimate-code/src/session/compaction.ts:83, processor.ts:421-423
Flagged by: Gemini (CRITICAL), MiniMax (MAJOR)

isOverflow() and context_overflow_recovered telemetry both access tokens.cache.read and tokens.cache.write unconditionally. Sessions created before this PR stored tokens as { input, output, reasoning } — no cache field. When an existing user upgrades and resumes a session, input.tokens.cache is undefined and accessing .read throws TypeError: Cannot read properties of undefined. This will crash any active session loop for existing users.

Fix:

// compaction.ts:83
const count =
  input.tokens.total ||
  input.tokens.input + input.tokens.output +
  (input.tokens.cache?.read ?? 0) + (input.tokens.cache?.write ?? 0)

// processor.ts:421-423
tokens.total ||
tokens.input + tokens.output + (tokens.cache?.read ?? 0) + (tokens.cache?.write ?? 0)

Also make cache optional in the Zod schema for AssistantMessage and StepFinishPart in message-v2.ts:

cache: z.object({ read: z.number(), write: z.number() }).optional().default({ read: 0, write: 0 })

🟠 Major

2. Text part time.start overwritten at text-end — duration always 0ms

Location: packages/altimate-code/src/session/processor.ts:376-380
Flagged by: Claude (MAJOR)

At text-end, currentText.time is assigned { start: Date.now(), end: Date.now() }. This overwrites the start timestamp set at text-start (line 341). Every text part will show 0ms duration. Compare: reasoning-end at line 107-109 correctly uses { ...part.time, end: Date.now() }.

Fix:

currentText.time = { ...currentText.time, end: Date.now() }

3. Telemetry is global singleton — concurrent sessions share state

Location: packages/altimate-code/src/telemetry/index.ts:120-128
Flagged by: Codex (MAJOR)

sessionId, projectId, and buffer are module-level variables. In a multi-session server environment (altimate-code serve), concurrent SessionPrompt.loop() calls overwrite each other's context via Telemetry.setContext(). Events from session B can be flushed under session A's session_id. Additionally, Telemetry.shutdown() called by one session clears the buffer globally, silently dropping in-flight events from all other sessions.

Suggestion: Short-term: document the limitation. Medium-term: pass sessionId/projectId into each track() call directly instead of via shared state.

🟡 Minor

4. isOverflow() can return negative usable for very small models

Location: packages/altimate-code/src/session/compaction.ts:87-89

If reserved + maxOutput > limit.input, usable goes negative and compaction triggers on every turn infinitely.

Fix: const usable = Math.max(0, ...) and guard if (usable === 0) return false.

5. Non-null assertion on findLast() — crash risk

Location: packages/altimate-code/src/session/compaction.ts:163

input.messages.findLast((m) => m.info.id === input.parentID)!.info throws if no message matches parentID. Should check for undefined and return early.

6. CJK and emoji content causes inaccurate token estimation

Location: packages/altimate-code/src/util/token.ts:21

input.length returns JavaScript code units. Emoji are 2 code units but ~1-2 tokens (2x over-estimation). Worth documenting as a known limitation of the heuristic.

⚪ Nit

7. "manual" trigger type is dead code

Location: packages/altimate-code/src/telemetry/index.ts:100

The "manual" value in the trigger union type is never emitted anywhere. Either implement it or remove it from the type definition.

✅ What's done well

1. isOverflow() fix for limit.input models is correct — the new formula properly reserves headroom. BUG regression tests at compaction.test.ts:128-194 are well-structured.
2. Loop protection state machine is well-designed — the Sentry fix (compactionAttempts = 0 on "continue") correctly prevents false terminations. 20+ test scenarios validate all state transitions.
3. Overflow detection is comprehensive — 14 provider-specific patterns covering Anthropic, OpenAI, Bedrock, Gemini, xAI, Groq, OpenRouter, DeepSeek, GitHub Copilot, llama.cpp, LM Studio, MiniMax, Kimi, Azure.
4. createObservationMask() is well-designed — format [Tool output cleared — tool(args) returned N lines, X bytes — "first line"] gives agents useful context. Surrogate-pair-safe truncation is good defensive code.
5. Telemetry is properly fail-safe — catch {} in flush() ensures telemetry never crashes the CLI. Timer .unref() prevents blocking process exit.
6. Test coverage is strong — boundary conditions, edge cases, and the state machine are all well-tested.

Missing Tests

1. No test for text-part time.start preservation across text-start → text-end cycle
2. No test for tokens.cache undefined crash (backward compat regression)
3. No test for findLast() returning undefined in SessionCompaction.process()
4. No test for isOverflow() with limit.input < reserved + maxOutput (negative usable)

Reviewed by 8 models: Claude, Codex, Gemini, Kimi, Grok, MiniMax, GLM-5, Qwen. Convergence: 1 round. 6 false positives dismissed (JSON regex, system prompt leak, PRUNE_PROTECT design, hardcoded "3", compaction loop counter, switch(true) pre-existing code).

Thanks for the thorough multi-model review! Here's the triage and resolution:

Fixed (3df53b5)

Not a real bug

Out of scope (pre-existing / other PRs)

All 1366 tests pass.