Skip to content

fix(experimental): session API better compaction helpers#1210

Merged
mattzcarey merged 23 commits into
mainfrom
fix/session-api-edge-cases
Mar 27, 2026
Merged

fix(experimental): session API better compaction helpers#1210
mattzcarey merged 23 commits into
mainfrom
fix/session-api-edge-cases

Conversation

@mattzcarey

@mattzcarey mattzcarey commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

Summary

Edge case fixes, compaction API improvements, and status broadcasting for the Session API.

Edge case fixes

1. Empty prompt not persisted

freezeSystemPrompt() used if (stored) which treats empty string as "no value". Fixed: if (stored !== null).

2. Double-prefix in compaction summaries

createCompactFunction and applyCompactions both added prefixes. Fixed: removed prefix from applyCompactions — the summary text already has context.

3. SearchResult.createdAt always empty

FTS5 can't populate created_at. Fixed: made createdAt optional on SearchResult. Also fixed Session.search() return type to match.

4. Cross-session parentId not validated

appendMessage accepted any parentId without checking session ownership. Fixed: validates with SELECT id WHERE session_id = ?, falls back to null.

5. No depth guard in recursive CTEs

Both getHistory and getPathLength CTEs had no depth limit. Fixed: WHERE p.depth < 10000.

6. previousSummary closure lost on DO eviction

createCompactFunction stored previousSummary in closure. Fixed: extracts from existing compaction messages in history.

Compaction API improvements

CompactResult replaces UIMessage[]

createCompactFunction now returns { fromMessageId, toMessageId, summary } | null instead of a UIMessage[]. Session.compact() uses it directly — no more prefix searching, message diffing, or ID convention coupling.

Single COMPACTION_PREFIX

All compaction messages use compaction_ prefix. Exported as COMPACTION_PREFIX constant with isCompactionMessage() helper. No more separate prefixes for overlays vs summaries.

onCompaction() + compactAfter() builder methods

Session.create(this)
  .onCompaction(createCompactFunction({ summarize: ... }))
  .compactAfter(100000)  // auto-compact when tokens exceed threshold

appendMessage auto-compacts when estimateMessageTokens(history) > threshold. Auto-compact failure is non-fatal — the message is still appended.

Removed needsCompaction(maxMessages)

Replaced by token-based compactAfter(tokenThreshold). Message count was a poor proxy for context window usage.

Status broadcasting

Session auto-broadcasts to connected WebSocket clients during compaction:

cf_agent_session — status updates

{ "type": "cf_agent_session", "phase": "compacting", "tokenEstimate": 1234, "tokenThreshold": 100000 }
{ "type": "cf_agent_session", "phase": "idle", "tokenEstimate": 456, "tokenThreshold": 100000, "compacted": { "tokensBefore": 1234, "tokensAfter": 456 } }

cf_agent_session_error — compaction failures

{ "type": "cf_agent_session_error", "error": "504 Gateway Timeout" }

Both added to MessageType enum. MessageType now exported from agents package.

Example updates

  • session-memory uses onCompaction() + compactAfter(100) builder API
  • Streaming chat via @callable({ streaming: true }) + streamText
  • Token usage badge in header (% of threshold or raw estimate)
  • Auto-refreshes messages after compaction to show overlay
  • Compaction uses @cf/zai-org/glm-4.7-flash (fast model)
  • Error recovery: cf_agent_session_error resets compacting state

Test plan

  • 918 tests pass (10 new: empty prompt persistence, CompactResult contract, auto-compaction triggers, broadcast events)
  • Pre-existing: 2 connection-uri failures (unrelated)

@mattzcarey

Copy link
Copy Markdown
Contributor Author

/bonk review

ask-bonk[bot]
ask-bonk Bot previously requested changes Mar 26, 2026

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good set of defensive fixes. Most are clearly correct. I have two issues — one is a bug introduced by this PR, the other is a correctness concern with the previousSummary extraction logic.

Issues

1. Session.search() return type not updated (bug)

SearchResult.createdAt was made optional in provider.ts, but the inline return type in session.ts:304-309 still declares createdAt: string (required). This means Session.search() now has a type mismatch with what AgentSessionProvider.searchMessages() actually returns (objects without createdAt). This should either be updated to createdAt?: string or the Session.search() method should use the SearchResult type directly instead of an inline type literal.

2. Fix #6previousSummary extraction uses wrong id prefix

createCompactFunction (compaction-helpers.ts:459-460) searches for m.id.startsWith("compaction_"), which matches ids created by applyCompactions in agent.ts:337 (compaction_${comp.id}). However, the summary message created by createCompactFunction itself uses a different prefix: compaction-summary-${Date.now()} (line 484).

This means on the second compaction cycle:

  • The first compaction produces a compaction-summary-* message, which compact() in session.ts:239-252 then stores as a StoredCompaction in the DB.
  • On the next getHistory(), applyCompactions replaces the compacted range with a compaction_* overlay message containing the stored summary.
  • So by the time createCompactFunction runs again, the message in the history will indeed have a compaction_ prefix — but only if the flow goes through applyCompactions.

This works in the normal Session.compact() flow, but if createCompactFunction is used standalone (as shown in its JSDoc example), the returned compaction-summary-* messages won't be found by the compaction_ prefix check. The id prefix coupling between these two separate modules is fragile. Consider either:

  • Also matching compaction-summary- prefix, or
  • Using a more robust signal (e.g., a property on the message) rather than id prefix matching.

Minor observations

  • Fixes 1-5 look correct and well-motivated.
  • Tests cover the empty prompt edge cases well.
  • The SearchResult.createdAt test (line 543-551) only checks TypeScript type compatibility, which is fine for a type change, but it's a no-op at runtime since any object would pass that check.

content: string;
createdAt: string;
createdAt?: string;
sessionId?: string;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Session.search() at session.ts:308 still has createdAt: string (required), but SearchResult.createdAt is now optional. This is a type mismatch — searchMessages() returns objects without createdAt, but this method signature promises it will always be present. Should be createdAt?: string here too, or better yet, just use the SearchResult type directly.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: Session.search() return type now has createdAt?: string matching SearchResult.

// in the message history (survives DO eviction, unlike closure state)
const existingCompaction = messages.find((m) =>
m.id.startsWith("compaction_")
);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This prefix check ("compaction_") only matches overlay messages created by applyCompactions in agent.ts. If createCompactFunction is used outside the Session.compact() flow (as its JSDoc example suggests is possible), the summary messages it produces have id compaction-summary-*, which won't match here. Consider also checking "compaction-summary-":

const existingCompaction = messages.find((m) =>
  m.id.startsWith("compaction_") || m.id.startsWith("compaction-summary-")
);

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed: Now checks both compaction_ (from applyCompactions overlays) and compaction-summary- (from createCompactFunction standalone output). Works in both the Session.compact() flow and standalone usage.

@ask-bonk

ask-bonk Bot commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

Review posted on PR #1210 with "request changes". Two issues identified:

  1. Session.search() return type mismatchSearchResult.createdAt was made optional in provider.ts, but the inline return type in session.ts:308 still declares createdAt: string (required). This is a type inconsistency that will either cause a compile error or silently produce values where createdAt is undefined but typed as string.

  2. previousSummary extraction uses fragile id prefix couplingcreateCompactFunction searches for m.id.startsWith("compaction_") to find existing compaction messages, but the compaction messages it creates use the prefix compaction-summary-. While this works in the normal Session.compact() flow (because applyCompactions re-wraps summaries with compaction_ ids), it won't work if createCompactFunction is used standalone as its JSDoc suggests.

github run

@mattzcarey
mattzcarey force-pushed the fix/session-api-edge-cases branch from a25d1b1 to 87ad03d Compare March 26, 2026 21:52
@changeset-bot

changeset-bot Bot commented Mar 26, 2026

Copy link
Copy Markdown

⚠️ No Changeset found

Latest commit: f3bcedf

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@mattzcarey
mattzcarey force-pushed the fix/session-api-edge-cases branch from 87ad03d to 1c5fdef Compare March 26, 2026 21:57

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no potential bugs to report.

View in Devin Review to see 5 additional findings.

Open in Devin Review

@pkg-pr-new

pkg-pr-new Bot commented Mar 26, 2026

Copy link
Copy Markdown

Open in StackBlitz

agents

npm i https://pkg.pr.new/agents@1210

@cloudflare/ai-chat

npm i https://pkg.pr.new/@cloudflare/ai-chat@1210

@cloudflare/codemode

npm i https://pkg.pr.new/@cloudflare/codemode@1210

hono-agents

npm i https://pkg.pr.new/hono-agents@1210

@cloudflare/shell

npm i https://pkg.pr.new/@cloudflare/shell@1210

@cloudflare/think

npm i https://pkg.pr.new/@cloudflare/think@1210

@cloudflare/voice

npm i https://pkg.pr.new/@cloudflare/voice@1210

@cloudflare/worker-bundler

npm i https://pkg.pr.new/@cloudflare/worker-bundler@1210

commit: f3bcedf

@mattzcarey
mattzcarey force-pushed the fix/session-api-edge-cases branch 3 times, most recently from fa74acc to 713d2b4 Compare March 27, 2026 10:57
devin-ai-integration[bot]

This comment was marked as resolved.

@mattzcarey
mattzcarey force-pushed the fix/session-api-edge-cases branch 4 times, most recently from f3cdc27 to b5c545e Compare March 27, 2026 11:20
devin-ai-integration[bot]

This comment was marked as resolved.

@mattzcarey
mattzcarey force-pushed the fix/session-api-edge-cases branch from b5c545e to 86c5408 Compare March 27, 2026 12:00
Matt and others added 4 commits March 27, 2026 12:03
Edge case fixes:
- freezeSystemPrompt: use !== null so empty prompts are persisted
- Remove double-prefix in compaction summaries
- SearchResult.createdAt now optional (FTS5 can't populate it)
- Session.search() return type matches SearchResult
- appendMessage validates parentId belongs to current session
- Depth guard (10000) in recursive CTEs
- createCompactFunction reads previousSummary from history not closure

Compaction API improvements:
- createCompactFunction returns CompactResult { fromMessageId, toMessageId, summary } instead of UIMessage[]
- Session.compact() uses CompactResult directly — no more prefix searching or message diffing
- Single COMPACTION_PREFIX constant + isCompactionMessage() helper
- onCompaction() builder method registers compaction function
- compactAfter(tokenThreshold) builder method for auto-compaction
- appendMessage auto-compacts when estimated tokens exceed threshold
- Removed needsCompaction(maxMessages) — replaced by token-based approach

Status broadcasting:
- Session broadcasts cf_agent_session events during compaction (phase, tokenEstimate, tokenThreshold)
- Session broadcasts cf_agent_session_error on compaction failure
- MessageType.CF_AGENT_SESSION + CF_AGENT_SESSION_ERROR added to enum
- MessageType exported from agents package
- Auto-compact failure is non-fatal — message still appended

Example updates:
- session-memory uses onCompaction() + compactAfter() builder API
- Streaming chat via @callable({ streaming: true }) + streamText
- Client shows token usage badge (% of threshold or raw estimate)
- Client auto-refreshes messages after compaction overlay appears
- Compaction uses @cf/zai-org/glm-4.7-flash (fast model for summarization)
During iterative compaction, middleMessages could include compaction
overlay messages with virtual IDs (compaction_<uuid>). This caused:

1. Virtual IDs stored as toMessageId, breaking future overlays since
   applyCompactions can't find virtual IDs in the message list
2. The previous summary appearing twice in the LLM prompt — once as
   PREVIOUS SUMMARY and again as a conversation turn in middleMessages

Fix: filter compaction overlay messages from middleMessages before
summarization. Use filtered message IDs for fromMessageId/toMessageId.
Return null if no real messages remain after filtering.
compact() extends fromMessageId for iterative compactions (using the
earliest existing compaction's start), but returned the original
CompactResult with the un-extended fromMessageId. The public API now
returns the actual persisted fromMessageId.
Set isCompacting immediately on click to disable the button before the
server broadcasts the compacting status via WebSocket. Reset on error
only — the server broadcast handles the success case.
@mattzcarey

Copy link
Copy Markdown
Contributor Author

/bonk review

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good set of edge case fixes and a clean refactor of the compaction API. The CompactResult abstraction, COMPACTION_PREFIX unification, and status broadcasting are well-designed. The freezeSystemPrompt null-vs-empty fix, cross-session parentId validation, and CTE depth guards are all clearly correct.

Two issues below — one is a real bug introduced by this PR, the other is a breaking API change that needs attention in downstream consumers.

Issue 1: appendMessage is now async but TestSessionAgent wrapper doesn't await it

Session.appendMessage() was changed from void to async ... Promise<void> (session.ts:223-242). The TestSessionAgent wrapper at tests/agents/session.ts:18-20 still declares appendMessage(): void and doesn't await the call:

appendMessage(message: UIMessage, parentId?: string): void {
  this.session.appendMessage(message, parentId);  // fire-and-forget
}

This means all integration tests calling agent.appendMessage() via RPC silently drop the auto-compaction promise. This won't surface as test failures today because the provider tests don't set a token threshold, but it means:

  1. Any test that relies on auto-compaction triggering via appendMessage against the real AgentSessionProvider will be flaky — the compaction runs detached and may not complete before assertions.
  2. If auto-compaction throws (e.g., LLM timeout), the error is swallowed without even the catch handler running in the test context.

The fix is to make the wrapper async and await the result:

async appendMessage(message: UIMessage, parentId?: string): Promise<void> {
  await this.session.appendMessage(message, parentId);
}

Issue 2: Devin's virtual-ID overlay compaction concern is valid and unmitigated

The Devin review correctly identified that during iterative compaction, createCompactFunction can produce a CompactResult where fromMessageId or toMessageId is a virtual overlay ID (e.g., compaction_<uuid>). The filtering fix at compaction-helpers.ts:479-481 handles the case where overlay messages are filtered out of middleMessages, but doesn't handle the case where the overlay is at compressStart and all real messages are in the tail — in that scenario middleMessages becomes empty and the function correctly returns null.

However, there's a subtler scenario: if there are a few real messages mixed in after the overlay within the compression window, middleMessages will be non-empty and fromMessageId will be a real message. But Session.compact() at session.ts:318-319 then overrides fromId to existing[0].fromMessageId (the first compaction's start), which is always a real message ID. And toMessageId comes from middleMessages[middleMessages.length - 1] which is filtered to real messages. So the stored compaction should always have real IDs.

I think the current code is actually correct for the normal flow, but it would be worth adding a test that exercises iterative compaction with a getHistory() that returns overlay messages to confirm the filtering works end-to-end. The unit tests currently use a stub getHistory that returns a plain message list without overlays.

ask-bonk[bot]

This comment was marked as resolved.

@ask-bonk

ask-bonk Bot commented Mar 27, 2026

Copy link
Copy Markdown
Contributor

Review posted on PR #1210 with "request changes". Here's a summary of what I found:

Issue 1 (bug): TestSessionAgent.appendMessage doesn't await the now-async Session.appendMessage

Session.appendMessage() was changed from synchronous void to async Promise<void> (to support auto-compaction), but the test wrapper at tests/agents/session.ts:18-20 still returns void without await. This means auto-compaction fires detached in integration tests — errors are silently lost and any test relying on compaction completing after appendMessage will be racy. Fix: make the wrapper async and await the call.

Issue 2 (observation): Virtual overlay ID concern from Devin review is valid but mitigated

I traced through the code path carefully. The isCompactionMessage filter at compaction-helpers.ts:479-481 correctly removes overlay messages from middleMessages, and Session.compact() overrides fromId from existing[0].fromMessageId. So stored compactions should always reference real message IDs. However, there are no integration tests exercising iterative compaction with overlay messages in the history — the unit tests all use stubs with plain message lists. A test covering this would increase confidence.

The previous reviewer comments about SearchResult.createdAt type mismatch and the dual prefix check have already been addressed in subsequent commits.

github run

@mattzcarey
mattzcarey force-pushed the fix/session-api-edge-cases branch from 55ddf5f to f272c03 Compare March 27, 2026 13:11
this._broadcastSessionError(
err instanceof Error ? err.message : String(err)
);
throw err;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

compact() broadcasts cf_agent_session_error here and then rethrows. The ChatAgent.compact() wrapper in server.ts:132-138 catches the rethrown error and returns { success: false }. This means the client receives both: (1) the error broadcast via WebSocket, and (2) a successful RPC response with { success: false }.

Consider either not rethrowing after broadcasting the error, or documenting that callers should expect both signals.

const existing = this.getCompactions();
const fromId =
existing.length > 0 ? existing[0].fromMessageId : removed[0].id;
this.addCompaction(result.summary, fromId, result.toMessageId);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onCompaction() API accepts arbitrary functions, but compact() stores result.toMessageId without validating it exists in the message history. If a custom compaction function returns a non-existent ID (typo, virtual overlay ID, wrong session), applyCompactions in agent.ts:335 will silently skip the overlay (ids.indexOf returns -1), permanently breaking compaction for that range.

Since the built-in createCompactFunction handles this correctly, this is low priority. But a defensive check here would prevent subtle data corruption from custom implementations:

const historyIds = new Set(history.map(m => m.id));
if (!historyIds.has(result.toMessageId)) {
  console.warn(`compact(): toMessageId not found in history, skipping`);
  return null;
}

@ask-bonk

ask-bonk Bot commented Mar 27, 2026

Copy link
Copy Markdown
Contributor

Review posted on PR #1210 with "request changes". Here's what I found:

Issue 1: compact() broadcasts cf_agent_session_error on LLM failure and then rethrows. The ChatAgent.compact() wrapper catches the rethrow and returns { success: false }, causing the client to receive a double error signal (WebSocket broadcast + RPC response). This is fragile — if someone removes the try/catch in the wrapper, the error surfaces twice on the client.

Issue 2: onCompaction() is a public API accepting arbitrary functions, but compact() stores result.toMessageId without validating it exists in the message history. A custom compaction function returning a non-existent ID would silently break applyCompactions permanently. The built-in createCompactFunction handles this correctly, but a defensive check would prevent data corruption from custom implementations.

All previously reported issues from ask-bonk and Devin reviews have been addressed across the 8 commits:

  • TestSessionAgent.appendMessage is now async with await
  • SearchResult.createdAt type mismatch fixed
  • Dual prefix check fixed (uses isCompactionMessage helper)
  • compact() returns correct fromMessageId
  • Overlay messages filtered from compaction range
  • Falsy check on _tokenThreshold fixed to use != null
  • Compact button has finally block with setIsCompacting(false)
  • Changeset present

github run

…oMessageId

1. compact() no longer rethrows after broadcasting cf_agent_session_error.
   Callers get null instead of a dual signal (error broadcast + exception).

2. Validate toMessageId exists in history before storing compaction.
   Prevents silent data corruption from custom compaction functions
   returning non-existent message IDs.
@mattzcarey

Copy link
Copy Markdown
Contributor Author

/bonk review

@ask-bonk ask-bonk Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approval

I've reviewed all 14 changed files, traced through the full review history (ask-bonk x3, Devin x5), and run the test suite (799 tests pass, 7 skipped). All previously reported issues have been addressed across the 10 commits. Here's my assessment:

All prior issues resolved

  1. SearchResult.createdAt type mismatch — Fixed: session.ts:398 now has createdAt?: string matching SearchResult.
  2. Dual prefix check for previousSummary — Fixed: compaction-helpers.ts:486 uses isCompactionMessage() helper, which checks COMPACTION_PREFIX. The compaction-summary- prefix is gone since createCompactFunction now returns CompactResult instead of creating messages.
  3. TestSessionAgent.appendMessage not async — Fixed: tests/agents/session.ts:18-20 properly awaits the call.
  4. Virtual overlay ID in toMessageId — Fixed: compaction-helpers.ts:479-481 filters overlay messages from middleMessages, and session.ts:316-325 validates toMessageId exists in history.
  5. compact() return value fromMessageId mismatch — Fixed: session.ts:346 returns { ...result, fromMessageId: fromId }.
  6. Compact button double-click — Fixed: client.tsx:295 sets setIsCompacting(true) immediately, with finally block at line 302.
  7. Auto-compaction failure propagation — Fixed: session.ts:235-239 wraps compact() in try/catch (non-fatal).
  8. Falsy check on _tokenThreshold — Fixed: session.ts:231 uses != null.
  9. compact() double-signal (broadcast + rethrow) — Fixed: session.ts:304 now return null instead of rethrowing, so the client only receives the WebSocket broadcast, not a second error from the RPC layer.
  10. Custom toMessageId validation — Added: session.ts:316-325 checks historyIds.has(result.toMessageId) before storing.
  11. Missing changeset — Present at .changeset/session-api-edge-cases.md, appropriately scoped as patch.

Code quality observations

  • The CompactResult abstraction is a good simplification over the previous UIMessage[] return. It cleanly separates the "what to compact" decision from the "how to store" concern.
  • The isCompactionMessage() helper and COMPACTION_PREFIX constant eliminate the fragile multi-prefix coupling between createCompactFunction and applyCompactions.
  • The _broadcastSession / _broadcastSessionError pattern with the Broadcaster interface is well-isolated — the Session class doesn't depend on Agent internals.
  • Test coverage for the new functionality is thorough: empty prompt persistence, CompactResult contract, auto-compaction triggers, broadcast events, and iterative compaction with overlay messages.

Non-blocking suggestions (for follow-up)

  1. compact() early return at history.length < 4 doesn't broadcast (session.ts:288). The client's finally block handles this, but if auto-compaction hits this path, no status is broadcast at all. Consider broadcasting phase: "idle" on this early return too, for consistency. Low priority since auto-compaction callers silently catch errors anyway.

  2. fromMessageId is not validated — only toMessageId is checked against historyIds (session.ts:316-325). A custom compaction function could return a non-existent fromMessageId, which would be stored and potentially confuse applyCompactions. The built-in createCompactFunction handles this correctly since it derives IDs from filtered real messages, so this is only relevant for custom implementations.

@ask-bonk

ask-bonk Bot commented Mar 27, 2026

Copy link
Copy Markdown
Contributor

To https://github.com/cloudflare/agents.git
! [rejected] fix/session-api-edge-cases -> fix/session-api-edge-cases (fetch first)
error: failed to push some refs to 'https://github.com/cloudflare/agents.git'
hint: Updates were rejected because the remote contains work that you do not
hint: have locally. This is usually caused by another repository pushing to
hint: the same ref. If you want to integrate the remote changes, use
hint: 'git pull' before pushing again.
hint: See the 'Note about fast-forwards' in 'git push --help' for details.

github run

@ask-bonk

ask-bonk Bot commented Mar 27, 2026

Copy link
Copy Markdown
Contributor

@mattzcarey Bonk workflow failed. Check the logs for details.

View workflow run · To retry, trigger Bonk again.

The summary prompt was structured for coding workflows — it asked for
'file paths, commands run, results obtained' and had a 'Relevant Files'
section. When the conversation wasn't about code, the LLM hallucinated
file paths and technical artifacts to fill the template.

Replaced with a content-agnostic prompt structure (Topic, Key Points,
Current State, Open Items) with explicit instruction to only include
information that was discussed in the conversation.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

View 18 additional findings in Devin Review.

Open in Devin Review

Comment on lines +332 to +346
this.addCompaction(result.summary, fromId, result.toMessageId);
await this.refreshSystemPrompt();
return removed.length;

const tokensAfter = estimateMessageTokens(this.getHistory());
this._broadcastSession({
phase: "idle",
tokenEstimate: tokensAfter,
tokenThreshold: this._tokenThreshold ?? null,
compacted: {
tokensBefore,
tokensAfter
}
});

return { ...result, fromMessageId: fromId };

@devin-ai-integration devin-ai-integration Bot Mar 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Client stuck in "Compacting..." state if addCompaction or refreshSystemPrompt throws

In Session.compact(), the "compacting" phase is broadcast at line 291, but addCompaction() (line 332) and refreshSystemPrompt() (line 333) are not wrapped in a try/catch. If either throws (e.g., a SQL error), the exception propagates to the caller without broadcasting an "idle" or "error" status. The client, having received the "compacting" broadcast, will remain stuck in the isCompacting=true state indefinitely — the Compact button stays disabled/loading, and the badge shows "Compacting..." with no recovery path.

The compaction function's own errors are correctly handled (lines 298-305 broadcast cf_agent_session_error), but these later steps are unprotected. This is especially relevant for auto-compact via appendMessage (packages/agents/src/experimental/memory/session/session.ts:236), where the error is silently swallowed and no broadcast follows.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +317 to +318
const historyIds = new Set(history.map((m) => m.id));
if (!historyIds.has(result.toMessageId)) {

@devin-ai-integration devin-ai-integration Bot Mar 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 toMessageId validation accepts virtual compaction overlay IDs that can never match in applyCompactions

In Session.compact(), the toMessageId validation at line 317-318 builds historyIds from this.getHistory(), which includes compaction overlay messages with virtual IDs (e.g., compaction_<uuid>). If a custom compaction function returns one of these overlay IDs as toMessageId, the validation passes, but applyCompactions in packages/agents/src/experimental/memory/session/providers/agent.ts:335 searches for toMessageId in the raw message path via ids.indexOf(comp.toMessageId) — overlay IDs don't exist there, so endIdx is -1 and the compaction overlay is silently ignored. The built-in createCompactFunction correctly filters these out (packages/agents/src/experimental/memory/utils/compaction-helpers.ts:463), but the validation in Session.compact() should reject overlay IDs to prevent silent failures from custom compaction functions.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 20 additional findings in Devin Review.

Open in Devin Review

The history.length < 4 guard was removed from compact() — the minimum
message check is now the compaction function's responsibility. Updated
the test to use a compaction function that returns null instead of
expecting compact() to short-circuit.
The history.length < 4 guard was removed from Session.compact() — the
check now lives in createCompactFunction (protectHead + minTailMessages).
Added tests to cover both the rejection and acceptance paths.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 2 new potential issues.

View 20 additional findings in Devin Review.

Open in Devin Review

Comment on lines +258 to +265
onDone: (final: unknown) => {
const f = final as { message?: UIMessage };
if (f.message) {
// Replace streaming message with final persisted message
setMessages((prev) =>
prev.map((m) => (m.id === streamId ? f.message! : m))
);
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 onDone handler silently drops assistant message when no text was streamed

When the LLM response consists only of tool calls (no text output), result.textStream yields nothing, so no text-delta chunks are sent and no streaming message (with id === streamId) is ever created in the messages array. The onDone handler at experimental/session-memory/src/client.tsx:262 uses .map() to replace the streaming message with the final one, but since no message matches streamId, the map returns the array unchanged and the final assistantMsg (containing tool call parts) is never added to the UI.

Trace of the issue
  1. Server streams no text chunks (tool-call-only response)
  2. Client onChunk is never invoked → no streaming message created
  3. onDone fires with { message: assistantMsg } containing tool parts
  4. prev.map(m => m.id === streamId ? f.message! : m) matches nothing → array unchanged
  5. Assistant message with tool calls is silently lost from the UI
Suggested change
onDone: (final: unknown) => {
const f = final as { message?: UIMessage };
if (f.message) {
// Replace streaming message with final persisted message
setMessages((prev) =>
prev.map((m) => (m.id === streamId ? f.message! : m))
);
}
onDone: (final: unknown) => {
const f = final as { message?: UIMessage };
if (f.message) {
// Replace streaming message with final persisted message,
// or append if no streaming message was created (e.g. tool-only responses)
setMessages((prev) => {
const hasStreaming = prev.some((m) => m.id === streamId);
if (hasStreaming) {
return prev.map((m) => (m.id === streamId ? f.message! : m));
}
return [...prev, f.message!];
});
}
},
Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Comment on lines +308 to +313
} catch (err) {
this._broadcastSessionError(
err instanceof Error ? err.message : String(err)
);
return null;
}

@devin-ai-integration devin-ai-integration Bot Mar 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 compact() error path broadcasts "compacting" but never transitions back to "idle"

In session.ts, when _compactionFn throws (lines 305-308), _emitError() is called but _emitStatus("idle") is not. Every other early-return path in compact() (lines 311, 318) correctly emits an "idle" status before returning. The client's onMessage handler in client.tsx:176-178 does handle cf_agent_session_error by setting isCompacting(false), so the primary client recovers. However, other broadcast listeners only tracking the cf_agent_session status type will see "compacting" as the last known phase until the next status-emitting operation occurs (e.g., the next appendMessage call).

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Added _broadcastTokens() helper that broadcasts cf_agent_session with
the current token estimate. Now called from updateMessage, deleteMessages,
and clearMessages in addition to appendMessage and compact(). The client
always has an up-to-date token count.
…emitError

Replace ad-hoc _broadcastSession calls with two focused helpers:
- _emitStatus(phase, extra?) — broadcasts phase + token estimate + threshold
- _emitError(error) — broadcasts error

Removes duplicated broadcast objects throughout compact(). The compacted
event now uses tokenEstimate (current) + compacted.tokensBefore instead
of redundant tokensBefore/tokensAfter fields.
The summary budget had a fixed minimum of 2000 tokens, which exceeded
the entire context window for small compactAfter thresholds (e.g. 1000).

Now the budget is capped at 50% of tailTokenBudget, with a minimum of
100 tokens. For the example config (tailTokenBudget=150), the summary
budget is ~75 tokens instead of 2000.
Previously minTailMessages overrode tailTokenBudget — when the token
budget was small, the token walk gave up quickly but the fallback
forced exactly minTailMessages in the tail regardless. With
tailTokenBudget=150, the budget was effectively ignored.

Now the tail protects whichever is larger: the token-based cut or
minTailMessages. The token budget is respected when it protects
more messages than the minimum.

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 25 additional findings in Devin Review.

Open in Devin Review

*/
export function createCompactFunction(opts: CompactOptions) {
const protectHead = opts.protectHead ?? 2;
const protectHead = opts.protectHead ?? 3;

@devin-ai-integration devin-ai-integration Bot Mar 27, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 protectHead JSDoc says default is 2 but implementation defaults to 3

The CompactOptions.protectHead JSDoc at compaction-helpers.ts:402 says (default: 2), but the implementation at compaction-helpers.ts:438 uses opts.protectHead ?? 3. This was changed in this PR (the old default was 2, matching the docs), but the doc comment was not updated. Users reading the JSDoc will incorrectly expect 2 head messages to be protected, when 3 are actually protected by default.

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

1. Send button becomes a stop button while streaming. Uses a stopped
   flag to ignore further chunks/done events from the server.

2. Tail budget edge case: if a single message at the tail exceeds the
   entire tailTokenBudget, it's still included (at least 1 tail message
   is always protected). Previously this would leave an empty tail.

3. Full README rewrite covering the complete Session API, compaction
   configuration, context blocks, WebSocket events, and truncation.
Default minTailMessages (4) is too high for a 1000-token window —
it forces 4 tail messages regardless of tailTokenBudget. Set to 1
so the token budget is actually respected. Updated README table.
The summary replaces the compressed middle — it doesn't compete with
the tail for space. Capping at 50% of tailTokenBudget was wrong for
small windows (e.g. 75 tokens for a 150-token tail budget).

Now uses the same approach as Hermes: 20% of compressed content with
a minimum of 100 tokens, no artificial ceiling.
… README

protectHead defaults to 3, minTailMessages now defaults to 2.
4 tail messages was too aggressive for small context windows.
@mattzcarey mattzcarey changed the title fix(experimental): session API edge cases fix(experimental): session API better compaction helpers Mar 27, 2026
@mattzcarey
mattzcarey merged commit 938f30f into main Mar 27, 2026
2 checks passed
@mattzcarey
mattzcarey deleted the fix/session-api-edge-cases branch March 27, 2026 15:49

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 new potential issue.

View 28 additional findings in Devin Review.

Open in Devin Review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 computeSummaryBudget JSDoc says "clamped to 2K-8K" but code has no upper clamp and min is 100

The JSDoc at compaction-helpers.ts:278-279 states the budget is "20% of the compressed content, clamped to 2K-8K tokens", but the actual implementation at line 288 is Math.max(100, budget) — a minimum of 100 with no upper bound. This means large histories could produce budgets well above 8K tokens, and the minimum is 100 not 2000. The inline comment at lines 283-286 correctly describes the logic, but the JSDoc contradicts it.

(Refers to lines 278-279)

Open in Devin Review

Was this helpful? React with 👍 or 👎 to provide feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant