feat: SharedStore for parallel workflow agent communication (#47)#48
Conversation
|
Nice structure, and the live path (per-run scoping, dispose timing, tool injection) checks out. But the resume/replay path — the load-bearing part of the design — has a blocking correctness bug: restore() does map.clear() + full replace, and each agent's journaled storeSnapshot is a whole-Map view captured at completion time. Parallel agents finish out of callSeq order, so on resume the restores replay in index order and the highest-index snapshot wins by replacement — wiping keys that sibling agents wrote but that snapshot never held. Net: resuming a run with a parallel fan-out can silently drop committed writes, and a downstream agent sees a store state that never existed on the original run. It reproduces with the ordinary "write result, then finish" pattern, not an edge case. Whole-Map snapshot + replace-on-restore is fundamentally incompatible with out-of-order parallel completion. Cleanest fix: journal a per-agent write delta (just the keys that agent set) and apply deltas additively in callSeq order on replay — that also makes the journaled value deterministic (today two resumes of the same run can restore different prefixes). structuredClone in snapshot() is worth adding alongside. Two more before re-review:
Happy to re-review once the snapshot/restore semantics are reworked. |
Adds SharedStore (in-memory Map per runId) with store_put/store_get tools injected into every agent in a workflow run, enabling parallel agents to share intermediate state without going through the parent. Closes QuintinShaw#47 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rdering Replace the full-map storeSnapshot/restore approach with per-agent write deltas so parallel agents finishing out of callSeq order no longer lose each other's writes on resume. Key changes: - SharedStore.trackPut(key, value, callIndex): records puts in a per-agent delta - SharedStore.commitDelta(callIndex): extracts and clears the agent's delta - SharedStore.applyDelta(delta): additive replay without clearing other keys - SharedStore.snapshot(): now returns a structuredClone (deep copy) - createAgentStoreTools(store, callIndex): per-agent tools that call trackPut - workflow.ts: use createAgentStoreTools per call, journal storeDelta, replay with applyDelta instead of storeSnapshot/restore - src/index.ts: export SharedStore and createSharedStoreTools for consumers - tests/shared-store.test.ts: unit tests for delta tracking, cross-run isolation, and resume-under-fan-out integration test Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Preparing review... |
3 similar comments
|
Preparing review... |
|
Preparing review... |
|
Preparing review... |
PR Reviewer Guide 🔍(Review updated until commit 76b130d)Here are some key observations to aid the review process:
|
- Remove unused `createSharedStoreTools` import (noUnusedImports) - Replace bracket notation with dot notation (useLiteralKeys) - Replace banned `Function` type with explicit signature (noBannedTypes) - Sort import names and index.ts exports (organizeImports) - Format test file to satisfy formatter Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
e079886 to
32dfff0
Compare
|
Persistent review updated to latest commit 32dfff0 |
1 similar comment
|
Persistent review updated to latest commit 32dfff0 |
…store_get Storing `undefined` explicitly would be treated as a missing key under the old check. Using `Map.has()` correctly distinguishes a present-but-undefined value from an absent key in both createSharedStoreTools and createAgentStoreTools. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Two tests in shared-store.test.ts were failing:
1. YAML frontmatter (`---`) was used in inline script strings but
parseWorkflowScript expects `export const meta = { ... }` as the
first statement. Acorn choked on `---` with SyntaxError.
2. The resume-replay test populated resumeJournal with ALL agent
entries including the get agents, so the gets were served as cache
hits and the mock agent never ran — leaving writeCalls.alpha undefined.
Fix: include only put agents (non-empty storeDelta) so gets run live
against the store rebuilt from deltas.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Persistent review updated to latest commit 76b130d |
|
The mechanism itself checks out — the tools are genuinely wired through
Worth documenting: writes via the public |
Resolves conflict in src/agent.ts by keeping both main's per-run modelRegistry override (systemTools field from PR QuintinShaw#48 sits alongside it).
|
Picked this up and pushed the fixes to your branch. The delta journaling is a nice design; the one real issue was the collision I flagged — a nested |
Summary
Parallel agents in a workflow run had no way to share intermediate state. This adds a
SharedStore(in-memory Map scoped per run) withstore_put/store_gettools injected into every agent, plus per-agent delta journaling so shared state replays correctly on resume.New:
src/shared-store.tsSharedStore—Map<string, unknown>withput,trackPut,get,has,snapshot,commitDelta,applyDelta,restore,disposecreateSharedStoreTools(store)— plain (non-journaled)[store_put, store_get]bound to a store; exported for host/external usecreateAgentStoreTools(store, deltaKey)— the per-agent journaled variant used internally by the runtime; attributes each write to a run-uniquedeltaKeyso it can be replayed on resumestore_putdescription documents last-write-wins semantics for parallel agentsModified:
src/workflow.tsSharedStoreper top-levelrunWorkflowcall; nestedworkflow()calls inherit it viaWorkflowRunOptions.sharedStorecreateAgentStoreTools(store, deltaKey)injected into every agent viasystemTools(bypasses thetoolNamesallowlist)storeDelta) captured in each journal entry; on cache-hit replaystore.applyDelta(delta)re-applies it additively in callSeq order, so parallel-agent writes reconstruct correctly regardless of original completion orderModified:
src/agent.tssystemTools?: ToolDefinition[]added toAgentRunOptions— tools injected after the allowlist/denylist filter, so store tools reach every agent even under a restrictiveagentTypeExported from
src/index.ts:SharedStore,createSharedStoreToolsMaintainer fixes pushed to this branch
workflow()shares the parent's store but restarts its owncallSeqat 0, so a parent agent and a nested-run agent could both land oncallIndex0 and clobber each other inagentDeltas. Deltas are now keyed by${runId}:${callIndex}— unique across the shared store's lifetime since every run (including each nested run,${runId}-nested{depth}) has a distinctrunId.agentTypestill receivesstore_put/store_get) and a nested-collision regression test.Notes / caveats
SharedStore.put()are not journaled — only the per-agent tool path (trackPut) is tracked and replayed. Host/script code callingput()directly bypasses resume.Closes #47
Test plan
store_put("key", val)→ agent B (same run) retrieves it withstore_get("key")(real-pi verified)runWorkflowcallsstore_geton missing key returnsnull(not error)agentTypeallowlist still receives the store toolsworkflow()concurrent writes don't collide in the journal