Feature request: Add structured memory primitives for better context engineering#791
Feature request: Add structured memory primitives for better context engineering#791mchockal wants to merge 10 commits intocloudflare:mainfrom
Conversation
|
commit: |
deathbyknowledge
left a comment
deathbyknowledge
left a comment
There was a problem hiding this comment.
This is not a final review yet but I'm struggling to see the bigger picture with this, it seems quite opinionated and not sure if the benefits outweigh the negatives.
Could you update one or two of the existing examples (or make a new one) to use this approach and see how if feels?
| /** | ||
| * Token limit processor - ensures context fits within token limits | ||
| */ | ||
| export const tokenLimitRequestProcessor: RequestProcessor< |
There was a problem hiding this comment.
This relies on estimateTokenCount which just guesses tokens. Since there's quite some variation in tokenizers depending on the model at use, I'd prefer if we remove this one and let users provide their own.
Maybe we can add "character" truncation instead?
There was a problem hiding this comment.
True. Ideally the choice of this processor implementation would be something left to the user. That would be step 0. But it could potentially be something to include at a later stage when context compaction to keep long-conversation going becomes more common, and if there's benefits to including it in the sdk at that point. The entire processors.ts can be part of a later PR.
| */ | ||
| export class Session { | ||
| readonly metadata: SessionMetadata; | ||
| readonly events: Event[]; |
There was a problem hiding this comment.
Do we want to use the DO sqlite to handle storage here? Or perhaps storing events directly, kind of like an event store?
There was a problem hiding this comment.
Good call. sqlite storage would be better, and fits better with the existing pattern. A cf_agents_session table might be a better approach to store user's session. Primary key will be a session_id, and events can be stored in order for easy retrieval of last 'N' user-agent turns in a given session, which would serve as the working-context for the current request. What are your thoughts?
(Apologies about the late response. I missed the email notification thread for this set of comments, and only noticed earlier today )
There was a problem hiding this comment.
yeah do sqlite is fine. might need to work around 2mb row limitation, just consider that.
|
I'll review further on wednesday |
|
Some thoughts: (that opus cleaned up for me) Thanks for putting this together - the architecture works and the problem space (context management, compaction, model-agnostic event logging) is real. I think this is worth shipping as an experimental feature with some adjustments. Overall TakeThe Session/WorkingContext/ProcessorPipeline split is reasonable. For users building long-running conversations, multi-agent systems, or agents that need audit trails, this provides useful primitives that don't exist in the SDK today. That said, most agents-sdk users are building simpler things and won't need this level of abstraction yet. So I'd recommend shipping this as explicitly experimental - opt-in for power users who want to explore these patterns. Maybe under agents/experimental/sessions? Before Merging1. Add clear experimental labeling The exports and docs should make it obvious this is experimental and the API may change:
2. Add at least one complete example The PR description has good code snippets, but there's no end-to-end example showing how to wire this into an actual agent. Something in
Without this, developers won't know how to connect the pieces. 3. Document known limitations Be explicit about what this doesn't do (yet):
Code-Level ItemsA few things to clean up: Token counting — The
Event ID generation — Tests — Would be good to have basic coverage for:
SummaryShip it as experimental with:
We'll learn more from real usage than from more design iteration. Just make sure the "experimental" signal is loud enough that people don't accidentally depend on API stability. |
|
Thanks for detailed feedback! I'll update the PR with the changes over the next 2-3 days and have it ready for review by Friday latest. |
|
/bonk review latest changes here |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: Experimental Memory Primitives
The overall architecture (Session / WorkingContext / Adapters split) is well-thought-out and the implementation is clean. The separation between RPC-safe methods and local-only WorkingContext helpers is a good design choice. The test coverage for the pure utility functions is solid.
However, there are two blocking issues (a syntax error in build.ts that will break the build, and the test scripts being rewritten in a way that breaks the existing vitest workspace setup) plus several smaller items that should be addressed before merging.
Blocking
build.tssyntax error — Missing comma between entry points will fail the build for the entireagentspackage.- Test scripts rewrite breaks existing setup — The PR replaces the vitest
--projectworkspace-based test commands with-rdirectory-based commands and dropstest:x402entirely. The repo has avitest.config.tsat the package root that defines projects; the new scripts bypass it.
Important
destroy()does not clean up session tables — The baseAgent.destroy()dropscf_agents_state,cf_agents_queues, etc. but doesn't know aboutcf_agents_sessionsorcf_agents_events.SessionAgentshould overridedestroy()to drop its tables first, then callsuper.destroy().appendEventsis not atomic — Individual INSERT statements in a loop without an explicit transaction mean a failure mid-batch leaves partial data. The baseAgentusesthis.sqlfor single statements, but batch operations should be wrapped in a transaction.- Missing changeset — The bot flagged this. Changes to
packages/agents/package.jsonexports need a changeset. - Example does not follow repo conventions — Per
examples/AGENTS.md, examples should be full-stack (frontend + backend), use Vite with@cloudflare/vite-plugin, includevite.config.ts,index.html, Kumo UI, etc. The current example is backend-only with curl-based usage. compatibility_dateis stale —wrangler.jsoncuses2025-09-27; the repo standard is2026-01-28.
Minor / Suggestions
loadEventsquery construction — The branching logic forsince/actions/tailcombinations produces 6 code paths. A query-builder approach or parameterized construction would be more maintainable.- Unsafe type assertions in
hydrateEvent— The metadata JSON is parsed and fields are cast withaswithout validation. Malformed metadata would produce runtime errors downstream. onStartin example uses sync methods —listSessions()andcreateSession()are synchronous (SQLite), butonStartisasync. This is fine but potentially confusing to readers.- Test config
includepath is relative to repo root — The vitest config for memory tests usesinclude: ["src/experimental/memory/__tests__/*.test.ts"]which works only when vitest is invoked from the package root, not the config file's directory. This may cause issues depending on how vitest resolves the path.
| "src/mcp/x402.ts", | ||
| "src/observability/index.ts", | ||
| "src/codemode/ai.ts", | ||
| "src/experimental/memory/index.ts" |
There was a problem hiding this comment.
Build-breaking bug: Missing comma between array elements. This will cause a syntax error when running npm run build.
| "src/experimental/memory/index.ts" | |
| "src/experimental/memory/index.ts", |
packages/agents/package.json
Outdated
| "test": "npm run test:workers && npm run test:react && npm run test:cli && npm run test:memory", | ||
| "test:cli": "vitest -r src/cli-tests", | ||
| "test:react": "vitest -r src/react-tests", | ||
| "test:e2e": "vitest run src/e2e/e2e.test.ts --sequence.concurrent", | ||
| "test:memory": "vitest run --config src/experimental/memory/__tests__/vitest.config.ts", | ||
| "test:workers": "vitest -r src/tests" | ||
| } |
There was a problem hiding this comment.
Breaking change to existing test infrastructure. The original scripts use vitest --project <name> which references the workspace config in vitest.config.ts (defines projects: src/tests/vitest.config.ts, src/react-tests/vitest.config.ts, src/cli-tests/vitest.config.ts, src/x402-tests/vitest.config.ts).
This PR:
- Replaces
--projectwith-r(changes how vitest resolves configs) - Drops
test:x402entirely - Changes
testfromvitest --run(which uses the workspace) to a sequential chain
Please keep the existing scripts intact and only add the new test:memory script. Then add the memory vitest config to the projects array in the root vitest.config.ts:
| "test": "npm run test:workers && npm run test:react && npm run test:cli && npm run test:memory", | |
| "test:cli": "vitest -r src/cli-tests", | |
| "test:react": "vitest -r src/react-tests", | |
| "test:e2e": "vitest run src/e2e/e2e.test.ts --sequence.concurrent", | |
| "test:memory": "vitest run --config src/experimental/memory/__tests__/vitest.config.ts", | |
| "test:workers": "vitest -r src/tests" | |
| } | |
| "test": "vitest --run", | |
| "test:cli": "vitest --project cli", | |
| "test:react": "vitest --project react", | |
| "test:e2e": "vitest run src/e2e/e2e.test.ts --sequence.concurrent", | |
| "test:memory": "vitest run --config src/experimental/memory/__tests__/vitest.config.ts", | |
| "test:workers": "vitest --project workers", | |
| "test:x402": "vitest --project x402" |
Alternatively, add the memory tests as another project in the workspace config so vitest --run picks them up automatically.
| `; | ||
|
|
||
| nextSeq++; | ||
| } |
There was a problem hiding this comment.
The loop of individual INSERT statements is not wrapped in a transaction. If the Durable Object fails mid-batch (e.g., on the 3rd of 5 events), you'll have partial data with a gap in seq values. Consider wrapping the batch in a transaction:
this.ctx.storage.sql.exec("BEGIN");
try {
for (const event of events) { ... }
this.ctx.storage.sql.exec("COMMIT");
} catch (e) {
this.ctx.storage.sql.exec("ROLLBACK");
throw e;
}Note: The base Agent's single-row operations don't need this, but batch inserts do.
| const events = newMessages.map((msg) => messageToEvent(sessionId, msg)); | ||
| this.appendEvents(sessionId, events); | ||
| } | ||
| } |
There was a problem hiding this comment.
SessionAgent creates cf_agents_sessions and cf_agents_events tables, but the base Agent.destroy() method only drops the tables it knows about (cf_agents_state, cf_agents_queues, cf_agents_schedules, cf_agents_mcp_servers, cf_agents_workflows).
Please override destroy() to clean up the session tables:
async destroy() {
this.sql`DROP TABLE IF EXISTS cf_agents_events`;
this.sql`DROP TABLE IF EXISTS cf_agents_sessions`;
await super.destroy();
}|
|
||
| const meta: Record<string, unknown> = row.metadata | ||
| ? JSON.parse(row.metadata) | ||
| : {}; |
There was a problem hiding this comment.
The metadata JSON is parsed with JSON.parse and then fields are accessed with bare as casts (e.g., meta.toolCallId as string, meta.toolCalls as ToolCall[]). If the stored metadata is malformed or from a schema migration, this will silently produce undefined values that propagate as runtime errors later.
Consider at minimum adding fallback checks, or documenting that metadata is trusted (since only dehydrateEvent writes it). For toolCallId, the downstream code assumes it's a non-empty string, so an explicit check would be safer:
toolCallId: (meta.toolCallId as string) ?? "",
toolName: (meta.toolName as string) ?? "",| if (since !== null && actions !== null && actions.length > 0) { | ||
| const placeholders = actions.map(() => "?").join(", "); | ||
| const inner = `SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ? AND created_at >= ? AND action IN (${placeholders}) | ||
| ORDER BY seq ${innerOrder} LIMIT ?`; | ||
| const query = tail | ||
| ? `SELECT * FROM (${inner}) sub ORDER BY seq ASC` | ||
| : inner; | ||
| rows = [ | ||
| ...this.ctx.storage.sql.exec(query, sessionId, since, ...actions, limit) | ||
| ] as unknown as StoredEvent[]; | ||
| } else if (since !== null) { | ||
| if (tail) { | ||
| rows = this.sql<StoredEvent>` | ||
| SELECT * FROM ( | ||
| SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ${sessionId} AND created_at >= ${since} | ||
| ORDER BY seq DESC LIMIT ${limit} | ||
| ) sub ORDER BY seq ASC | ||
| `; | ||
| } else { | ||
| rows = this.sql<StoredEvent>` | ||
| SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ${sessionId} AND created_at >= ${since} | ||
| ORDER BY seq ASC LIMIT ${limit} | ||
| `; | ||
| } | ||
| } else if (actions !== null && actions.length > 0) { | ||
| const placeholders = actions.map(() => "?").join(", "); | ||
| const inner = `SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ? AND action IN (${placeholders}) | ||
| ORDER BY seq ${innerOrder} LIMIT ?`; | ||
| const query = tail | ||
| ? `SELECT * FROM (${inner}) sub ORDER BY seq ASC` | ||
| : inner; | ||
| rows = [ | ||
| ...this.ctx.storage.sql.exec(query, sessionId, ...actions, limit) | ||
| ] as unknown as StoredEvent[]; | ||
| } else { | ||
| if (tail) { | ||
| rows = this.sql<StoredEvent>` | ||
| SELECT * FROM ( | ||
| SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ${sessionId} | ||
| ORDER BY seq DESC LIMIT ${limit} | ||
| ) sub ORDER BY seq ASC | ||
| `; | ||
| } else { | ||
| rows = this.sql<StoredEvent>` | ||
| SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ${sessionId} | ||
| ORDER BY seq ASC LIMIT ${limit} | ||
| `; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The loadEvents method has 6 distinct code paths for the combinations of since, actions, and tail. This is hard to maintain and test — adding a new filter option would double the branches.
Consider a query-builder approach that conditionally appends WHERE clauses:
let conditions = ["session_id = ?"];
let params: unknown[] = [sessionId];
if (since !== null) {
conditions.push("created_at >= ?");
params.push(since);
}
if (actions !== null && actions.length > 0) {
conditions.push(`action IN (${actions.map(() => "?").join(", ")})`);
params.push(...actions);
}
const where = conditions.join(" AND ");
const inner = `SELECT ... FROM cf_agents_events WHERE ${where} ORDER BY seq ${innerOrder} LIMIT ?`;
params.push(limit);
const query = tail ? `SELECT * FROM (${inner}) sub ORDER BY seq ASC` : inner;
rows = [...this.ctx.storage.sql.exec(query, ...params)] as unknown as StoredEvent[];This eliminates the combinatorial branching entirely.
| "name": "agents-session-agent", | ||
| "main": "src/index.ts", | ||
| "compatibility_date": "2025-09-27", | ||
| "compatibility_flags": ["nodejs_compat"], |
There was a problem hiding this comment.
The compatibility_date should be "2026-01-28" per the repo convention (see root AGENTS.md).
| "compatibility_flags": ["nodejs_compat"], | |
| "compatibility_date": "2026-01-28", |
| /** | ||
| * @experimental | ||
| * Build a WorkingContext from a list of session events. This is an example, | ||
| * Keep this outside of the Agent class to allow handling multiple independent threads in parallel. |
There was a problem hiding this comment.
Nit: This comment says "This is an example" — should probably say something like "Pure function — keep outside the Agent class to allow use from Workflows/Workers/tests."
| test: { | ||
| environment: "node", | ||
| clearMocks: true, | ||
| include: ["src/experimental/memory/__tests__/*.test.ts"] |
There was a problem hiding this comment.
This include path is relative to the repo root, not to the config file's directory. This works when running vitest run --config src/experimental/memory/__tests__/vitest.config.ts from the packages/agents/ directory, but may be fragile.
Consider using a path relative to the config file, or use root in the config to make it explicit:
export default defineConfig({
test: {
environment: "node",
clearMocks: true,
root: "../../../..",
include: ["src/experimental/memory/__tests__/*.test.ts"]
}
});Or simply:
include: ["./*.test.ts"]if vitest resolves relative to the config file location (check vitest docs for your version).
| if (msg.role === "user") { | ||
| return { ...base, action: EventAction.USER_MESSAGE, content: msg.content }; | ||
| } | ||
| // TOOL_CALL_REQUEST are also stored as 'assistant' messages, eventhough it is a separate event, hence this extra check |
There was a problem hiding this comment.
Nit: typo in comment — eventhough → even though.
| // TOOL_CALL_REQUEST are also stored as 'assistant' messages, eventhough it is a separate event, hence this extra check | |
| // TOOL_CALL_REQUEST are also stored as 'assistant' messages, even though it is a separate event, hence this extra check |
|
Review submitted successfully. Here's a summary of the feedback: Blocking issues (2)
Important issues (5)
Minor items (4)
|
…apters, tail-mode loadEvents
|
/bonk review again please |
There was a problem hiding this comment.
Review: Experimental Memory Primitives (v2)
The architecture is well-designed. The Session/WorkingContext/Adapter split is clean, the separation between RPC-safe methods and local-only helpers is thoughtful, and the pure utility functions are well-tested. The code addresses the previous round of feedback (from threepointone and deathbyknowledge) effectively — the ProcessorPipeline and token estimation were dropped, SQL-backed storage was adopted, and the module is properly labeled as experimental.
I have two blocking issues, several important items, and a few minor suggestions.
Blocking
-
package-lock.jsonis massively out of sync — The fork's lockfile has ~25,000 lines of diff against main, downgrading nearly every dependency (e.g.,@ai-sdk/anthropicfrom^3.0.45to^3.0.8,@cloudflare/workers-typesfrom^4.20260219.0to^4.20260103.0, adding@biomejs/biome, removing@cloudflare/kumo,oxfmt,oxlint, etc.). This PR should not modifypackage-lock.jsonbeyond what's needed for the newexamples/session-agentworkspace. Please rebase on the latestmainand runnpm installto regenerate. -
Missing changeset — This PR adds a new export path (
agents/experimental/memory) topackages/agents/package.json. Per repo conventions, changes topackages/that affect the public API need a changeset. Runnpx changesetand describe the new experimental memory module.
Important
-
SessionAgentshould overridedestroy()— The baseAgent.destroy()drops the five tables it creates (cf_agents_state,cf_agents_queues,cf_agents_schedules,cf_agents_mcp_servers,cf_agents_workflows) and then callsdeleteAll(). However, it doesn't know aboutcf_agents_sessionsorcf_agents_events. WhiledeleteAll()clears storage, the explicit table drops are part of the pattern for clean teardown and should be followed consistently. -
appendEventsshould use a transaction — The loop of individual INSERT statements at lines 254-265 can leave partial data if the DO fails mid-batch. A single failed insert after 3 of 5 events would leave orphaned rows with gaps inseqvalues. The base Agent uses individualthis.sqlstatements but only for single-row operations. Batch inserts should be wrapped in a transaction. -
deleteSessionshould also use a transaction — Lines 138-139 execute two separate DELETE statements. If the first succeeds but the second fails, you'd have a session row with no events, but the session itself would still exist (or vice versa). -
Example doesn't follow repo conventions — Per
examples/AGENTS.md, all examples must be full-stack (frontend + backend) with Vite,@cloudflare/vite-plugin,index.html, React client, Kumo UI, etc. The current example is backend-only with curl-based usage. It's missingvite.config.ts,index.html,src/client.tsx, and the required shared components (PoweredByAgents,ConnectionIndicator,ModeToggle,ThemeProvider). I understand this is extra work, but consistency matters for user-facing learning material. -
compatibility_dateis stale — The example'swrangler.jsoncuses2025-09-27; the repo standard is2026-01-28.
Minor
- See inline comments below.
| const events = newMessages.map((msg) => messageToEvent(sessionId, msg)); | ||
| this.appendEvents(sessionId, events); | ||
| } | ||
| } |
There was a problem hiding this comment.
SessionAgent creates cf_agents_sessions and cf_agents_events tables, but the base Agent.destroy() only drops the tables it knows about. Override destroy() to clean up:
| } | |
| async destroy() { | |
| this.sql`DROP TABLE IF EXISTS cf_agents_events`; | |
| this.sql`DROP TABLE IF EXISTS cf_agents_sessions`; | |
| await super.destroy(); | |
| } | |
| } |
| for (const event of events) { | ||
| // Override seq with the correct monotonic value | ||
| const withSeq: SessionEvent = { ...event, seq: nextSeq, sessionId }; | ||
| const row = dehydrateEvent(withSeq); | ||
|
|
||
| this.sql` | ||
| INSERT INTO cf_agents_events (id, session_id, seq, action, content, metadata, created_at) | ||
| VALUES (${row.id}, ${row.session_id}, ${row.seq}, ${row.action}, ${row.content}, ${row.metadata}, ${row.created_at}) | ||
| `; | ||
|
|
||
| nextSeq++; | ||
| } |
There was a problem hiding this comment.
This loop of individual INSERTs is not atomic. If the DO fails mid-batch, you'll have partial data with gaps in seq values. Wrap in a transaction:
this.ctx.storage.sql.exec("BEGIN");
try {
for (const event of events) {
const withSeq: SessionEvent = { ...event, seq: nextSeq, sessionId };
const row = dehydrateEvent(withSeq);
this.sql`
INSERT INTO cf_agents_events (id, session_id, seq, action, content, metadata, created_at)
VALUES (${row.id}, ${row.session_id}, ${row.seq}, ${row.action}, ${row.content}, ${row.metadata}, ${row.created_at})
`;
nextSeq++;
}
this.ctx.storage.sql.exec("COMMIT");
} catch (e) {
this.ctx.storage.sql.exec("ROLLBACK");
throw e;
}Note: Durable Object SQLite supports transactions via BEGIN/COMMIT/ROLLBACK.
| deleteSession(sessionId: string): void { | ||
| this.sql`DELETE FROM cf_agents_events WHERE session_id = ${sessionId}`; | ||
| this.sql`DELETE FROM cf_agents_sessions WHERE id = ${sessionId}`; | ||
| } |
There was a problem hiding this comment.
Same atomicity concern here — if the first DELETE succeeds but the second fails (or vice versa), you'll have inconsistent state. Consider wrapping in a transaction:
| deleteSession(sessionId: string): void { | |
| this.sql`DELETE FROM cf_agents_events WHERE session_id = ${sessionId}`; | |
| this.sql`DELETE FROM cf_agents_sessions WHERE id = ${sessionId}`; | |
| } | |
| deleteSession(sessionId: string): void { | |
| this.ctx.storage.sql.exec("BEGIN"); | |
| try { | |
| this.sql`DELETE FROM cf_agents_events WHERE session_id = ${sessionId}`; | |
| this.sql`DELETE FROM cf_agents_sessions WHERE id = ${sessionId}`; | |
| this.ctx.storage.sql.exec("COMMIT"); | |
| } catch (e) { | |
| this.ctx.storage.sql.exec("ROLLBACK"); | |
| throw e; | |
| } | |
| } |
| if (since !== null && actions !== null && actions.length > 0) { | ||
| const placeholders = actions.map(() => "?").join(", "); | ||
| const inner = `SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ? AND created_at >= ? AND action IN (${placeholders}) | ||
| ORDER BY seq ${innerOrder} LIMIT ?`; | ||
| const query = tail | ||
| ? `SELECT * FROM (${inner}) sub ORDER BY seq ASC` | ||
| : inner; | ||
| rows = [ | ||
| ...this.ctx.storage.sql.exec(query, sessionId, since, ...actions, limit) | ||
| ] as unknown as StoredEvent[]; | ||
| } else if (since !== null) { | ||
| if (tail) { | ||
| rows = this.sql<StoredEvent>` | ||
| SELECT * FROM ( | ||
| SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ${sessionId} AND created_at >= ${since} | ||
| ORDER BY seq DESC LIMIT ${limit} | ||
| ) sub ORDER BY seq ASC | ||
| `; | ||
| } else { | ||
| rows = this.sql<StoredEvent>` | ||
| SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ${sessionId} AND created_at >= ${since} | ||
| ORDER BY seq ASC LIMIT ${limit} | ||
| `; | ||
| } | ||
| } else if (actions !== null && actions.length > 0) { | ||
| const placeholders = actions.map(() => "?").join(", "); | ||
| const inner = `SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ? AND action IN (${placeholders}) | ||
| ORDER BY seq ${innerOrder} LIMIT ?`; | ||
| const query = tail | ||
| ? `SELECT * FROM (${inner}) sub ORDER BY seq ASC` | ||
| : inner; | ||
| rows = [ | ||
| ...this.ctx.storage.sql.exec(query, sessionId, ...actions, limit) | ||
| ] as unknown as StoredEvent[]; | ||
| } else { | ||
| if (tail) { | ||
| rows = this.sql<StoredEvent>` | ||
| SELECT * FROM ( | ||
| SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ${sessionId} | ||
| ORDER BY seq DESC LIMIT ${limit} | ||
| ) sub ORDER BY seq ASC | ||
| `; | ||
| } else { | ||
| rows = this.sql<StoredEvent>` | ||
| SELECT id, session_id, seq, action, content, metadata, created_at | ||
| FROM cf_agents_events | ||
| WHERE session_id = ${sessionId} | ||
| ORDER BY seq ASC LIMIT ${limit} | ||
| `; | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
The combinatorial branching for since/actions/tail produces 6 code paths. This is hard to maintain and will double with each new filter. Consider a query-builder approach:
const conditions = ["session_id = ?"];
const params: (string | number)[] = [sessionId];
if (since !== null) {
conditions.push("created_at >= ?");
params.push(since);
}
if (actions !== null && actions.length > 0) {
conditions.push(`action IN (${actions.map(() => "?").join(", ")})`);
params.push(...actions);
}
const where = conditions.join(" AND ");
const cols = "id, session_id, seq, action, content, metadata, created_at";
const inner = `SELECT ${cols} FROM cf_agents_events WHERE ${where} ORDER BY seq ${innerOrder} LIMIT ?`;
params.push(limit);
const query = tail
? `SELECT * FROM (${inner}) sub ORDER BY seq ASC`
: inner;
rows = [...this.ctx.storage.sql.exec(query, ...params)] as unknown as StoredEvent[];This eliminates the branching entirely and is easier to extend.
| ...base, | ||
| action: EventAction.TOOL_RESULT, | ||
| content: row.content ?? "", | ||
| toolCallId: meta.toolCallId as string, |
There was a problem hiding this comment.
The as casts on metadata fields (meta.toolCallId as string, meta.toolCalls as ToolCall[]) will silently produce undefined if metadata is malformed or from a future schema migration. Since only dehydrateEvent writes these fields, the risk is low in practice, but defensive defaults would be safer:
| toolCallId: meta.toolCallId as string, | |
| toolCallId: (meta.toolCallId as string) ?? "", |
Similarly for toolName on line 66 and toolCalls on line 57.
| if (msg.role === "user") { | ||
| return { ...base, action: EventAction.USER_MESSAGE, content: msg.content }; | ||
| } | ||
| // TOOL_CALL_REQUEST are also stored as 'assistant' messages, eventhough it is a separate event, hence this extra check |
There was a problem hiding this comment.
Typo: eventhough should be even though.
| // TOOL_CALL_REQUEST are also stored as 'assistant' messages, eventhough it is a separate event, hence this extra check | |
| // TOOL_CALL_REQUEST are also stored as 'assistant' messages, even though it is a separate event, hence this extra check |
| /** | ||
| * @experimental | ||
| * Build a WorkingContext from a list of session events. This is an example, | ||
| * Keep this outside of the Agent class to allow handling multiple independent threads in parallel. |
There was a problem hiding this comment.
This comment says "This is an example" — it should describe the function's purpose, e.g.:
| * Keep this outside of the Agent class to allow handling multiple independent threads in parallel. | |
| * Pure function for building a WorkingContext from session events. | |
| * Keep this outside of the Agent class to allow handling multiple independent threads in parallel. |
| "name": "agents-session-agent", | ||
| "main": "src/index.ts", | ||
| "compatibility_date": "2025-09-27", | ||
| "compatibility_flags": ["nodejs_compat"], |
There was a problem hiding this comment.
Should be 2026-01-28 per repo convention.
| "compatibility_flags": ["nodejs_compat"], | |
| "compatibility_date": "2026-01-28", |
| test: { | ||
| environment: "node", | ||
| clearMocks: true, | ||
| include: ["src/experimental/memory/__tests__/*.test.ts"] |
There was a problem hiding this comment.
This include path is relative to the working directory, not the config file location. It works when invoked via vitest run --config <path> from the packages/agents/ directory, but is fragile. Consider using a relative path from the config file:
| include: ["src/experimental/memory/__tests__/*.test.ts"] | |
| include: ["./*.test.ts"] |
Or explicitly set root if vitest needs the package root:
root: "../../../..",
include: ["src/experimental/memory/__tests__/*.test.ts"]
examples/session-agent/src/index.ts
Outdated
| } | ||
|
|
||
| // Public wrapper — _buildWorkingContext is protected, | ||
| // but we need it from our own helper methods (?) |
There was a problem hiding this comment.
The (?) in this comment reads like uncertainty. If this is the intended pattern for subclasses (wrapping _buildWorkingContext as a public method), document it clearly:
| // but we need it from our own helper methods (?) | |
| // Public wrapper — _buildWorkingContext is protected to prevent RPC misuse, | |
| // but subclasses need to expose it for their own request handlers. |
Alternatively, consider whether _buildWorkingContext should just be public on SessionAgent since every subclass will need to wrap it anyway. The RPC-safety concern is valid, but protected doesn't actually prevent RPC calls — it only restricts TypeScript-level access.
|
Review submitted successfully. Here's a summary of my review: Review SummaryI posted a comprehensive review on PR #791 with 2 blocking issues, 5 important items, and 8 inline comments. Blocking
Important
Minor (inline)
I also corrected the previous bonk review's incorrect claims about a "missing comma in build.ts" (already fixed) and "test scripts being rewritten" (only |
…tion, query-builder, nits
…tion, make buildWorkingContext public
5 participants
feat(memory): Add Session, WorkingContext,
and Processor PipelineprimitivesSummary
This PR introduces structured memory primitives for building context-aware agents, based on the tiered memory architecture principles from Google's ADK whitepaper, but adapted to agents sdk.
Update 2/16: Added experimental session primitives to agents/experimental/memory — SQL-backed sessions, append-only event storage, ephemeral WorkingContext. This is a complete redesign of the original v0 memory primitives based on PR feedback, focusing just on the Session and Working Context first. A separate class
SessionAgentis used to keep these changes purely experimental and independent of theAgentclass.Why?
Currently, agent developers must manually manage conversation history, handle context window limits, and implement their own compaction strategies. This leads to:
These primitives provide a model-agnostic foundation that separates the ground truth (Session) from the computed view (WorkingContext), enabling clean abstractions for context management and (later)compaction.
What's Added
SessionAgent — Agent subclass with session/event SQL tables
Extends Agent and creates cf_agents_sessions and cf_agents_events tables in the constructor (matching the Agent pattern for cf_agents_state, cf_agents_queues, etc.).
RPC-safe methods (plain serializable objects, callable from Workflows/Workers):
Local-only methods (class instances don't survive RPC serialization):
WorkingContext — Ephemeral in-memory context
Built per-request from session events. Accumulates messages during an agentic loop, tracks which are new via _initialCount. Thrown away after the request.
buildWorkingContext() — Pure function
For use outside the Agent (Workflows, Workers, tests). Takes SessionEvent[] from loadEvents() RPC and builds a WorkingContext locally.
workersAIAdapter — Model format adapter
Stateless adapter converting WorkingContext messages to Workers AI chat completions format. Handles system instructions, structured toolCalls, and tool results. Support for other frameworks will be added as new adapters.
Pure utility functions
SQL Schema
Example: examples/session-agent/
A complete working example with:
Tests
Tests run via npm run test:memory (added to the CI test pipeline).
Known Limitations
b). spend one inference call with output_tokens limited to 1 to obtain the input-only token_usage_count
Files Changed / Added