feat(compaction): hard-compact mid-turn when token threshold is reached

[Anthony-Bible]

Previously auto-compaction only fired at the natural end of a turn (AI response with no tool calls). Long tool-using sessions could run many AI<->tool round-trips well above the configured threshold before the model finally returned without tool calls, sending the full over-threshold conversation on every call.

Now AddToolResultMessage also checks the threshold (using token usage from the prior AI response) and compacts before the next AI call. This spot is API-safe: the assistant tool_use and the user tool_result are both already in the conversation, so the pair is preserved in the summary and there is no orphaned tool_use. Worst-case overshoot is one extra tool round.

A small maybeCompact helper consolidates the threshold-check + log + swallow pattern shared between finalizeAIResponse (end-of-turn) and AddToolResultMessage (mid-turn).

[claude]

Claude finished @Anthony-Bible's task in 2m 14s —— View job

---

PR Review: feat(compaction): hard-compact mid-turn when token threshold is reached

• Gather context and read changed files
• Analyze compaction logic for correctness and edge cases
• Review test coverage
• Post comprehensive review feedback

---

Summary

The core design is sound. Moving the compaction trigger into AddToolResultMessage correctly solves the over-threshold problem for long tool-using sessions, and the PR description accurately captures the API-safety invariant (tool_use + tool_result pair is always in the conversation before compaction fires). The maybeCompact helper is a clean refactoring. Below are a few issues worth addressing.

---

Issues

1. Redundant threshold re-check in finalizeAIResponse (minor)

conversation_service.go, lines 392–395:

shouldCompact := cs.setTokensAndCheckThreshold(sessionID, response.InputTokens, response.OutputTokens)
if !hasToolCalls && shouldCompact {
    cs.maybeCompact(ctx, sessionID, conversation)
}

maybeCompact unconditionally acquires a read lock and calls getTokenUsage to re-check the threshold — which was already atomically computed by setTokensAndCheckThreshold. The value can only change if another goroutine writes to sessionTokenUsage for the same session between these two lines, which requires a concurrent AI response on the same session. That's an edge case, but the re-read is still redundant in the happy path.

For consistency with AddToolResultMessage (which calls maybeCompact with no pre-gate), consider removing the shouldCompact variable entirely and always calling maybeCompact when !hasToolCalls, matching the unconditional pattern already used mid-turn. Fix this →

2. Test mock uses tools == nil as a summary-request detector (fragile)

conversation_service_test.go, lines ~2415–2427:

// nil tools => this is the compaction summary request
if tools == nil {
    ...
}

This leaks the implementation detail that compactConversation passes nil for tools (line 1034 of conversation_service.go). If compactConversation ever passes an empty []port.ToolParam{} instead of nil, the mock's detection silently breaks and the test would pass a wrong code path without catching the regression. A more robust approach would be to inspect the message content (e.g., check if the first message contains "CONVERSATION TO SUMMARIZE"). Fix this →

3. callCount field declared but never asserted (dead code in tests)

conversation_service_test.go, line ~2404:

callCount int

callCount is incremented on every SendMessage call but no test assertion uses it. This means the number of AI round-trips is unverified — e.g., the test wouldn't catch if compaction fired twice. Either add an assertion (if provider.callCount != 3 { t.Errorf(...) }) or remove the field. Fix this →

4. Token counter reset not verified in tests (missing assertion)

TestCompactionTriggeredAfterToolResults verifies the conversation collapses to 1 summary message but doesn't assert that GetTokenUsage was reset to the summary's token count (500+500=1000). If compactConversation forgot to reset sessionTokenUsage, the next AI response would immediately re-trigger compaction (threshold thrash). Worth adding: Fix this →

tokens, threshold := svc.GetTokenUsage(sessionID)
if tokens != 1000 { // summaryResponse.InputTokens(500) + OutputTokens(500)
    t.Errorf("Expected token usage reset to 1000, got %d", tokens)
}

---

Observations (no action needed)

• fmt.Fprintf(os.Stderr, ...) vs structured logger: The CLAUDE.md guidelines prefer the port.Logger interface with With() context. However, ConversationService doesn't hold a logger field, so this would require a broader refactor. Not introduced by this PR.
• Mid-turn compaction uses prior-response token count: The comment documents this clearly. The tool result itself isn't counted until the next AI call, which is acceptable — the overshoot is at most one tool round, consistent with the PR description.
• API safety: The invariant that tool_use + tool_result are both in the conversation before compaction fires is correctly maintained. The summary will contain both sides of the pair.

---

Overall this is a solid improvement. The four items above are worth addressing but none are blockers.

[Copilot]

Pull request overview

This PR improves conversation auto-compaction behavior during tool-using turns by triggering compaction immediately after tool results are appended, preventing long multi-round tool cycles from repeatedly sending over-threshold conversation history to the model.

Changes:

• Run compaction mid-turn from AddToolResultMessage when the last recorded token usage meets the configured threshold.
• Add a maybeCompact helper to centralize the threshold-check + log + swallow pattern shared by end-of-turn and mid-turn compaction sites.
• Add tests covering (1) compaction triggering after tool results are added and (2) non-fatal behavior when compaction fails mid-turn.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.

File	Description
internal/domain/service/conversation_service.go	Adds mid-turn compaction after tool results and factors shared compaction logic into maybeCompact.
internal/domain/service/conversation_service_test.go	Adds tests validating mid-turn compaction behavior and non-fatal handling on compaction failure.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[claude]

Claude finished @Anthony-Bible's task in 2m 8s —— View job

---

PR Review: feat(compaction): hard-compact mid-turn when token threshold is reached

• Gather context and read changed files
• Analyze compaction logic for correctness and edge cases
• Review test coverage
• Post comprehensive review feedback

---

All Prior Feedback Addressed

The follow-up commit (1761d34) fully resolved all four items from the previous review:

Issue	Status
Redundant shouldCompact variable in finalizeAIResponse	✅ Removed — maybeCompact now called unconditionally when !hasToolCalls
Mock detected by tools == nil (fragile)	✅ Fixed — now checks messages[0].Content for "CONVERSATION TO SUMMARIZE"
callCount declared but never asserted	✅ Replaced with turnCalls/summaryCalls, both asserted
Token counter reset not verified	✅ Added GetTokenUsage assertion (tokens == 1000, threshold == 10000)

The finalizeAIResponse change is particularly clean: removing the shouldCompact bool and calling maybeCompact unconditionally matches the pattern already used in AddToolResultMessage and eliminates the redundant read-lock acquire.

---

Remaining Observations

1. Content-string detection still couples to buildSummaryRequest internals (very minor)

conversation_service_test.go, line ~2427:

if len(messages) > 0 && strings.Contains(messages[0].Content, "CONVERSATION TO SUMMARIZE") {

This is much better than the tools == nil heuristic, but the test is still coupled to the exact phrase in buildSummaryRequest ("=== CONVERSATION TO SUMMARIZE ===\n\n" at line 1004). If the summary prompt preamble ever changes, the mock silently stops discriminating. A constant shared between production code and test (or a dedicated interface method) would remove this coupling entirely. That said, this is an acceptable tradeoff for a test mock and not a blocking concern.

2. No guard against concurrent double-compaction (pre-existing, not introduced here)

maybeCompact checks the threshold under a read lock, then calls compactConversation outside any lock:

// conversation_service.go:354-357
if cs.getTokenUsage(sessionID) < cs.compactionThreshold {   // read lock
    return
}
cs.compactConversation(ctx, sessionID, conversation)          // no lock held

If two goroutines both pass the threshold check before either resets the counter, both will compact — clearing the conversation twice and producing two summary messages (the second summarizing a one-message conversation). In practice the current callers are serial (AddToolResultMessage batches all results into one call, ExecuteToolsInResponse loops serially), so this race is theoretical rather than real. It's pre-existing and the PR doesn't make it worse, but worth documenting for anyone who adds a parallel tool-result code path in the future. Note for future work →

---

Overall

This is a solid, well-tested improvement. The API-safety invariant (tool_use + tool_result both in conversation before compaction fires) is correctly maintained, the helper refactoring is clean, and the test coverage now verifies all the important post-compaction invariants (message count, summary content, call counts, token reset). Ready to merge.