AltimateAI / AltimateAI/altimate-code
SessionProcessor closure state claims session-scope but resets per-step (warning false-positive + doom-loop detector silently degraded)
- Lingua principale
- TypeScript
- Stelle
- 811
- Fork
- 134
- Merge medio
- 3g 2h
- PR unite (30g)
- 50
Descrizione
## Symptom
State declared in `SessionProcessor.create()`'s closure scope is documented as "session-scoped, accumulates across `process()` invocations within a session" (see `processor.ts:207-210`), but `loop()` in `session/prompt.ts:485` creates a fresh `SessionProcessor` *every step*. The closure variables therefore reset on each step, not each session.
This is a documentation/implementation mismatch — the comments describe an intent that the code does not deliver.
## Known victims
1. **`plan_no_tool_generation` warning** — `sessionToolCallsMade` (`processor.ts:57`). On a multi-step plan session that runs tools across early steps and produces a final text-only step, the warning fires on the last step even though the agent did its job correctly. Patched as a targeted workaround in #888 (PR for #887) by also scanning `streamInput.messages` for prior assistant tool-call content at warning-evaluation time. Not a structural fix.
2. **Doom-loop detector** — `toolCallCounts` (`processor.ts:43`, used in 207-219). The comment explicitly states "cross-turn accumulation catches slow-burn loops that stay under the threshold per-turn but add up over the session" — but because the counter resets per step, slow-burn loops that cross step boundaries cannot trigger the threshold. The detector still catches *within-step* hot loops (e.g. todowrite 2,080x in a single step) and is presumably what saved us from realising sooner, but the documented cross-turn behavior is broken. Unpatched in #888.
There may be other consumers of the same closure variables (e.g. `toolcalls` map at line 41) where per-step vs session scoping matters for correctness — worth an audit.
## Proper fix (options)
The targeted workaround in #888 only addresses victim 1, and only by reading from the conversation history (which the warning happens to have access to). It does not generalize to the doom-loop detector, whose state has no equivalent representation in the message stream.
1. **Move `SessionProcessor.create()` outside the per-step `while (true)` body in `loop()`** so a single processor instance handles all steps of a session. This is the change that most closely matches the comments' intent. Risk: anything else in `create()`'s closure that *should* be per-step would silently break. Needs an audit of all closure variables before flipping.
2. **Lift the two affected counters to a session-keyed `Map` at module scope.** Smaller blast radius than #1, but adds memory-leak surface (need explicit cleanup when sessions end / abort).
3. **Keep the per-step semantics and update the comments + variable names** to match reality. Then re-derive the cross-turn doom-loop signal from the message stream the same way the plan-no-tool workaround does (count `tool-call` content parts across the session). Risk: every consumer that thinks it's session-scoped has to be migrated individually.
I'd lean toward option 1 *if* the closure audit comes back clean. Otherwise option 3 is the safest; it doesn't pretend the bug is fixed and forces each consumer to make an explicit choice.
## Acceptance criteria
- [ ] Closure variables in `SessionProcessor.create()` are audited; each one is documented as either *intentionally per-step* or *session-wide*.
- [ ] Variables that should be session-wide actually behave that way at runtime.
- [ ] `toolCallCounts` cross-turn behavior matches its comment, OR the comment is rewritten to match the implementation.
- [ ] `plan_no_tool_generation` warning is driven by whatever the canonical session-wide signal is (not the workaround scan added in #888).
- [ ] Regression test for cross-turn doom-loop detection.
Guida per i contributori
Apri la guida per i contributori
Valutazione
Questa issue non è ancora stata valutata.