feat(slots): reject stale slot overwrites, keep an undo copy, report headroom - #1175
feat(slots): reject stale slot overwrites, keep an undo copy, report headroom#1175sbynode-ux wants to merge 1 commit into
Conversation
…headroom A slot shared by several lanes can only be emptied by rewriting the whole thing, and mem::slot-replace had neither a concurrency guard nor an undo. Two sessions that both read a slot and then wrote it back produced a silent clobber of whichever lane wrote first, and once a slot approached its sizeLimit the only escape was exactly that operation — so the safest-looking action and the most dangerous one were the same call. Three additions, all opt-in and backward compatible: - mem::slot-replace accepts expectedRev / expectedHash from the read the caller merged from and rejects the write when the slot has moved on, returning the current rev and hash so the caller can re-read and merge. Callers that pass neither behave exactly as before. - every append and replace snapshots the previous content to mem:slots:history (most recent AGENTMEMORY_SLOT_HISTORY per label, default 20). mem::slot-history lists them and, with restore:true, returns the replace call that undoes the last write. History is best-effort: a write is never failed because its undo copy could not be stored. - slot reads and writes return size / sizeLimit / free / pctUsed / rev / contentHash, plus a warning at 70% and 90%, so a caller can see the slot filling up before an append starts failing. The MCP dispatcher rebuilds the slot-replace payload field by field, so the new parameters are forwarded there and declared in the tool schema; without that the guard exists server-side and never fires for the agents that actually use it. Signed-off-by: sbynode-ux <sby.node@gmail.com>
|
@sbynode-ux is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughSlot APIs now return size, capacity, revision, hash, and warning statistics. Mutations support optimistic concurrency checks and preserve configurable snapshots. A new history function lists snapshots and generates restore instructions. MCP replacement inputs expose the new checks. ChangesSlot lifecycle enhancements
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant MCPClient
participant memory_slot_replace
participant mem_slot_replace
participant KVStore
MCPClient->>memory_slot_replace: label, content, expectedRev or expectedHash
memory_slot_replace->>mem_slot_replace: forward replacement payload
mem_slot_replace->>KVStore: read current slot and validate expectation
mem_slot_replace->>KVStore: snapshot prior content
mem_slot_replace->>KVStore: write content and increment revision
mem_slot_replace-->>MCPClient: updated statistics or stale-write rejection
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (2)
src/functions/slots.ts (2)
15-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove new comments that restate implementation details.
src/functions/slots.ts#L15-L20: remove comments that restate constant purpose.src/functions/slots.ts#L42-L46: remove theslotStats()implementation explanation.src/functions/slots.ts#L72-L75: remove thesnapshotSlot()implementation explanation.src/types.ts#L230-L231: remove therevbehavior explanation.src/mcp/server.ts#L1216-L1217: remove the payload-forwarding implementation explanation.As per coding guidelines,
src/**/*.tsmust not add comments that explain what code does; use clear naming instead.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/functions/slots.ts` around lines 15 - 20, Remove the implementation-detail comments without changing behavior: in src/functions/slots.ts lines 15-20, 42-46, and 72-75, delete the comments around SLOT_HISTORY_SCOPE/DEFAULT_SLOT_HISTORY/SLOT_WARN_PCT/SLOT_URGENT_PCT, slotStats(), and snapshotSlot(); in src/types.ts lines 230-231, delete the rev behavior explanation; and in src/mcp/server.ts lines 1216-1217, delete the payload-forwarding explanation. Keep the constants, functions, types, and payload handling unchanged.Source: Coding guidelines
97-99: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winDelete pruned snapshots in parallel.
Each
kv.delete()is independent aftermineis calculated. UsePromise.all()so pruning does not add one persistence round trip per expired snapshot.As per coding guidelines, run independent KV writes in parallel where possible.
Proposed change
- for (const old of mine.slice(0, Math.max(0, mine.length - keep))) { - await kv.delete(SLOT_HISTORY_SCOPE, old.id).catch(() => {}); - } + await Promise.all( + mine + .slice(0, Math.max(0, mine.length - keep)) + .map((old) => kv.delete(SLOT_HISTORY_SCOPE, old.id).catch(() => {})), + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/functions/slots.ts` around lines 97 - 99, Update the snapshot-pruning loop in the slots history flow to issue independent kv.delete calls concurrently with Promise.all, while preserving the existing SLOT_HISTORY_SCOPE, old.id, and swallowed-error behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/functions/slots.ts`:
- Around line 488-492: Update the restore instruction generation around
restoreWith and mem::slot-replace to read the slot’s effective current state and
include its revision as the expectedRev precondition. Preserve the existing
label and content values, and add a test covering a slot change between history
lookup and restore that prevents a stale overwrite.
- Around line 22-30: Update src/functions/slots.ts lines 22-30 by adding a scope
field to SlotHistoryEntry, then update the history lookup around lines 479-481
to resolve the effective slot scope and filter entries by both label and scope,
preventing project and global slot histories from being mixed.
- Line 57: Update the contentHash assignment in the slot creation flow to use
fingerprintId("slot", slot.content) instead of the truncated SHA-1 createHash
expression, while leaving the surrounding slot data unchanged.
- Around line 425-450: Update the slot replacement flow surrounding
withKeyedLock and readSlot so concurrency is enforced across workers, not only
within one process. Either route each slot label to a single owner through
worker sharding, or use a storage-level compare-and-set that validates
expectedRev/expectedHash together with the write; preserve the existing conflict
responses when validation fails.
In `@src/mcp/server.ts`:
- Around line 1223-1224: Validate supplied expectedRev and expectedHash inputs
at the MCP boundary before constructing the replacement payload. Require
expectedRev to be a non-negative safe integer and expectedHash to be a non-empty
string; return HTTP 400 for invalid present values rather than omitting them,
while preserving omission when either field is absent. Update the payload
construction near the expectedRev/expectedHash spread expressions.
In `@src/mcp/tools-registry.ts`:
- Around line 917-932: The replace-slot tool description incorrectly claims
previous content is snapshotted for every attempt. Update the description near
the replace operation to state that successful replacements snapshot the
pre-write content, while preserving the existing stale-revision and size-limit
failure behavior.
---
Nitpick comments:
In `@src/functions/slots.ts`:
- Around line 15-20: Remove the implementation-detail comments without changing
behavior: in src/functions/slots.ts lines 15-20, 42-46, and 72-75, delete the
comments around
SLOT_HISTORY_SCOPE/DEFAULT_SLOT_HISTORY/SLOT_WARN_PCT/SLOT_URGENT_PCT,
slotStats(), and snapshotSlot(); in src/types.ts lines 230-231, delete the rev
behavior explanation; and in src/mcp/server.ts lines 1216-1217, delete the
payload-forwarding explanation. Keep the constants, functions, types, and
payload handling unchanged.
- Around line 97-99: Update the snapshot-pruning loop in the slots history flow
to issue independent kv.delete calls concurrently with Promise.all, while
preserving the existing SLOT_HISTORY_SCOPE, old.id, and swallowed-error
behavior.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a8684e8b-48b4-4cb4-b672-d7e5294ce7a6
📒 Files selected for processing (5)
src/functions/slots.tssrc/mcp/server.tssrc/mcp/tools-registry.tssrc/types.tstest/slots-concurrency.test.ts
| export interface SlotHistoryEntry { | ||
| id: string; | ||
| label: string; | ||
| operation: "append" | "replace"; | ||
| content: string; | ||
| size: number; | ||
| rev: number; | ||
| at: string; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Separate slot history by scope. Project and global slots can share a label, but history stores and filters only by label. A project slot can therefore list or restore a global slot snapshot and overwrite project content.
src/functions/slots.ts#L22-L30: addscopetoSlotHistoryEntry.src/functions/slots.ts#L479-L481: resolve the effective slot scope and filter history by bothlabelandscope.
📍 Affects 1 file
src/functions/slots.ts#L22-L30(this comment)src/functions/slots.ts#L479-L481
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/functions/slots.ts` around lines 22 - 30, Update src/functions/slots.ts
lines 22-30 by adding a scope field to SlotHistoryEntry, then update the history
lookup around lines 479-481 to resolve the effective slot scope and filter
entries by both label and scope, preventing project and global slot histories
from being mixed.
| free: Math.max(0, sizeLimit - size), | ||
| pctUsed, | ||
| rev: slot.rev ?? 0, | ||
| contentHash: createHash("sha1").update(slot.content).digest("hex").slice(0, 12), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 3 '\bfingerprintId\s*\(' src testRepository: rohitg00/agentmemory
Length of output: 5816
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- src/functions/slots.ts outline ---'
ast-grep outline src/functions/slots.ts
printf '%s\n' '--- relevant slots.ts sections ---'
cat -n src/functions/slots.ts | sed -n '1,150p'
printf '%s\n' '--- hash and slot revision usages ---'
rg -n -C 4 'createHash|contentHash|expectedHash|fingerprintId|generateId|revision' src/functions/slots.ts src/state
printf '%s\n' '--- slot function callers ---'
rg -n -C 3 'slots|appendSlot|replaceSlot|restore|contentHash' src testRepository: rohitg00/agentmemory
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- slotStats and mutation handlers ---'
cat -n src/functions/slots.ts | sed -n '40,66p;390,475p'
printf '%s\n' '--- slot tests involving hashes and revisions ---'
rg -n -C 8 'expectedHash|contentHash|expectedRev|rev' test/slots*.test.ts
printf '%s\n' '--- fingerprintId imports and call sites ---'
rg -n -C 3 'import .*fingerprintId|fingerprintId\(' src --glob '*.ts'
printf '%s\n' '--- public slot documentation ---'
rg -n -C 4 'expectedHash|contentHash|expectedRev|slot-replace|slot-append' README.md docs src test --glob '!src/viewer/index.html'Repository: rohitg00/agentmemory
Length of output: 39460
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from hashlib import sha1, sha256
samples = ["", "base", "updated content", "a" * 2000]
for value in samples:
sha1_hash = sha1(value.encode()).hexdigest()[:12]
fingerprint = f"slot_{sha256(value.encode()).hexdigest()[:16]}"
print({"content": repr(value), "sha1_12": sha1_hash, "fingerprintId": fingerprint})
PY
printf '%s\n' '--- mutation precondition semantics ---'
sed -n '400,465p' src/functions/slots.ts
printf '%s\n' '--- hash-related tests ---'
rg -n -C 6 'expectedHash|contentHash|expectedRev' test/slots*.test.tsRepository: rohitg00/agentmemory
Length of output: 9812
Use fingerprintId() for contentHash.
expectedHash is an alternative stale-write precondition. The current 48-bit SHA-1 value can collide and allow a stale replacement to overwrite the slot. Use fingerprintId("slot", slot.content).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/functions/slots.ts` at line 57, Update the contentHash assignment in the
slot creation flow to use fingerprintId("slot", slot.content) instead of the
truncated SHA-1 createHash expression, while leaving the surrounding slot data
unchanged.
Sources: Coding guidelines, Linters/SAST tools
| return withKeyedLock(`slot:${label}`, async () => { | ||
| const { slot, scope } = await readSlot(kv, label); | ||
| if (!slot) return { success: false, error: "slot not found (use mem::slot-create first)" }; | ||
| if (slot.readOnly) return { success: false, error: "slot is read-only" }; | ||
| if (data.content.length > slot.sizeLimit) { | ||
| if (content.length > slot.sizeLimit) { | ||
| return { | ||
| success: false, | ||
| error: `content exceeds sizeLimit (${data.content.length} > ${slot.sizeLimit})`, | ||
| error: `content exceeds sizeLimit (${content.length} > ${slot.sizeLimit})`, | ||
| sizeLimit: slot.sizeLimit, | ||
| }; | ||
| } | ||
| const updated: MemorySlot = { ...slot, content: data.content, updatedAt: nowIso() }; | ||
| const current = slotStats(slot); | ||
| if (data.expectedRev !== undefined && Number(data.expectedRev) !== current.rev) { | ||
| return { | ||
| success: false, | ||
| error: `slot changed since you read it (expectedRev ${data.expectedRev}, current ${current.rev}) — re-read the slot and merge, do not overwrite`, | ||
| current, | ||
| }; | ||
| } | ||
| if (typeof data.expectedHash === "string" && data.expectedHash !== current.contentHash) { | ||
| return { | ||
| success: false, | ||
| error: `slot changed since you read it (expectedHash ${data.expectedHash}, current ${current.contentHash}) — re-read the slot and merge, do not overwrite`, | ||
| current, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 4 'withKeyedLock|expectedRev|expectedHash|state::update|compare.?and.?set|conditional' src test
fd -HI -t f 'Dockerfile*' . -o -t f 'docker-compose*.yml' . -o -t f '*deployment*.yml' .Repository: rohitg00/agentmemory
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- keyed mutex ---'
cat -n src/state/keyed-mutex.ts
echo '--- KV wrapper ---'
cat -n src/state/kv.ts
echo '--- slot write path ---'
sed -n '330,535p' src/functions/slots.ts
echo '--- state update registrations/usages ---'
rg -n -C 5 'state::update|register.*state|function_id.*state|StateKV|kv\.set|kv\.update' src --glob '*.ts' | head -n 500
echo '--- deployment/config files ---'
find . -type f \( -iname 'Dockerfile*' -o -iname 'docker-compose*.yml' -o -iname '*deployment*.yml' -o -iname '*iii*.json' -o -iname '*iii*.yml' -o -iname '*iii*.yaml' \) -printRepository: rohitg00/agentmemory
Length of output: 38736
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in iii-config.yaml iii-config.docker.yaml docker-compose.yml deploy/coolify/docker-compose.yml deploy/render/Dockerfile deploy/railway/Dockerfile deploy/fly/Dockerfile deploy/coolify/Dockerfile package.json README.md; do
if [ -f "$f" ]; then
echo "--- $f ---"
cat -n "$f" | sed -n '1,260p'
fi
done
echo '--- process/worker/runtime terms ---'
rg -n -i -C 3 'worker|process|replica|replicas|instance|scale|cluster|pm2|node .*cluster|concurr|serial|queue|lock|sqlite|state' \
iii-config.yaml iii-config.docker.yaml docker-compose.yml deploy README.md package.json .github 2>/dev/null | head -n 500Repository: rohitg00/agentmemory
Length of output: 50378
🌐 Web query:
iii-engine v0.11.2 state::update KV adapter conditional compare and set concurrency semantics
💡 Result:
As of iii-engine v0.11.2, the KV adapter and the broader state management system do not support conditional compare-and-set (CAS) operations [1][2]. The system exposes six state functions: state::get, state::set, state::delete, state::update, state::list, and state::list_groups [3][4]. The state::update function allows for atomic partial updates (such as set, merge, append, increment, decrement, and remove) on a record [5], but it does not support precondition checks or version-based optimistic concurrency [2]. Similarly, state::set is an unconditional overwrite [2]. Because there is no native CAS primitive, developers are advised against implementing "fake" CAS patterns—such as a client-side get followed by a conditional set—because these are vulnerable to race conditions where an intervening mutation can cause a stale write [1][2]. Official guidance for managing concurrency in environments requiring record-level atomicity is to use worker sharding to ensure only one owning instance processes a given state partition [2]. A feature request for native conditional single-key CAS functionality has been identified as a required prerequisite for true multi-writer concurrency [1][2].
Citations:
- 1: Proposal: conditional single-key compare-and-set for state iii-hq/iii#2011
- 2: https://github.com/iii-hq/workers/blob/main/workflow/README.md
- 3: https://github.com/iii-hq/workers/tree/main/state
- 4: https://workers.iii.dev/workers/state
- 5: https://github.com/iii-hq/iii/blob/d51a06dabf41d8121d99e622dc351f1e2ebb7810/skills/references/state-management.py
🌐 Web query:
iii-engine worker configuration multiple workers process concurrency state kv file_based adapter documentation
💡 Result:
In the iii engine, workers are configured within the workers: block of the config.yaml file [1][2]. Each worker entry typically contains a name and an optional config block that serves as a one-time first-boot seed for runtime settings [2][3]. For workers requiring persistence or distributed state, such as iii-state or iii-queue, the configuration includes an adapter definition [1][4]. File-based Adapter Configuration The kv adapter supports a file_based storage method, which persists data to disk [5][6]. It is configured within the worker's config block [5][7]: workers: - name: iii-state config: adapter: name: kv config: store_method: file_based file_path:./data/state_store save_interval_ms: 5000 - store_method: Set to file_based to enable disk persistence (alternatively in_memory) [5][6]. - file_path: Specifies the directory path where state files are stored [5][6]. - save_interval_ms: Controls the persistence flush cadence in milliseconds; it defaults to 5000ms [5][6]. Worker Process Concurrency For the queue worker (iii-queue), concurrency is managed within the queue_configs block, which allows you to define processing behavior per queue [1][7]: workers: - name: iii-queue config: queue_configs: default: max_retries: 5 concurrency: 5 type: standard adapter: name: builtin config: store_method: file_based file_path:./data/queue_store In this context, the concurrency field sets the number of concurrent jobs processed by the worker [1][7]. The iii engine also allows for adapters to be swapped (e.g., using Redis instead of the built-in file-based adapter) without altering the application code, providing flexibility for different deployment environments [4].
Citations:
- 1: https://iii.dev/docs/how-to/configure-engine
- 2: https://iii.dev/docs/next/using-iii/engine
- 3: https://iii.dev/docs/using-iii/workers
- 4: https://docs.iii.dev/docs/0-11-0/advanced/adapters
- 5: https://workers.iii.dev/workers/iii-state
- 6: https://iii.dev/docs/0-11-0/workers/iii-state
- 7: https://iii.dev/docs/0-11-0/how-to/configure-engine
Make slot replacement atomic across workers
withKeyedLock() is process-local, while iii v0.11.2 provides no conditional KV write. If multiple workers or replicas serve a slot, enforce single-owner sharding or add storage-level compare-and-set; otherwise concurrent replacements can overwrite updates despite expectedRev or expectedHash.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/functions/slots.ts` around lines 425 - 450, Update the slot replacement
flow surrounding withKeyedLock and readSlot so concurrency is enforced across
workers, not only within one process. Either route each slot label to a single
owner through worker sharding, or use a storage-level compare-and-set that
validates expectedRev/expectedHash together with the write; preserve the
existing conflict responses when validation fails.
| restoreWith: { | ||
| function_id: "mem::slot-replace", | ||
| label, | ||
| content: target.content, | ||
| }, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Include a stale-write precondition in generated restores.
restoreWith creates a destructive mem::slot-replace call without expectedRev or expectedHash. If the slot changes after history is read, executing this instruction silently overwrites the newer content.
Read the effective slot when generating the instruction. Include its current revision in restoreWith. Add a test where the slot changes between history lookup and restore.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/functions/slots.ts` around lines 488 - 492, Update the restore
instruction generation around restoreWith and mem::slot-replace to read the
slot’s effective current state and include its revision as the expectedRev
precondition. Preserve the existing label and content values, and add a test
covering a slot change between history lookup and restore that prevents a stale
overwrite.
| ...(typeof args.expectedRev === "number" ? { expectedRev: args.expectedRev } : {}), | ||
| ...(typeof args.expectedHash === "string" ? { expectedHash: args.expectedHash } : {}), |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Reject invalid preconditions instead of dropping them.
If a caller supplies expectedRev as a string, a negative number, or a fractional number, this code omits it from the payload. The replacement then runs without concurrency protection. The same omission occurs for a non-string expectedHash.
If either field is present, validate it before constructing the payload. Require expectedRev to be a non-negative safe integer. Require expectedHash to be a non-empty string. Return HTTP 400 when validation fails.
As per coding guidelines, validate inputs at MCP system boundaries.
Proposed change
+ if (
+ args.expectedRev !== undefined &&
+ (!Number.isSafeInteger(args.expectedRev) || (args.expectedRev as number) < 0)
+ ) {
+ return { status_code: 400, body: { error: "expectedRev must be a non-negative integer" } };
+ }
+ if (
+ args.expectedHash !== undefined &&
+ (typeof args.expectedHash !== "string" || !args.expectedHash.trim())
+ ) {
+ return { status_code: 400, body: { error: "expectedHash must be a non-empty string" } };
+ }
const result = await sdk.trigger({📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ...(typeof args.expectedRev === "number" ? { expectedRev: args.expectedRev } : {}), | |
| ...(typeof args.expectedHash === "string" ? { expectedHash: args.expectedHash } : {}), | |
| if ( | |
| args.expectedRev !== undefined && | |
| (!Number.isSafeInteger(args.expectedRev) || (args.expectedRev as number) < 0) | |
| ) { | |
| return { status_code: 400, body: { error: "expectedRev must be a non-negative integer" } }; | |
| } | |
| if ( | |
| args.expectedHash !== undefined && | |
| (typeof args.expectedHash !== "string" || !args.expectedHash.trim()) | |
| ) { | |
| return { status_code: 400, body: { error: "expectedHash must be a non-empty string" } }; | |
| } | |
| const result = await sdk.trigger({ | |
| ...(typeof args.expectedRev === "number" ? { expectedRev: args.expectedRev } : {}), | |
| ...(typeof args.expectedHash === "string" ? { expectedHash: args.expectedHash } : {}), |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp/server.ts` around lines 1223 - 1224, Validate supplied expectedRev
and expectedHash inputs at the MCP boundary before constructing the replacement
payload. Require expectedRev to be a non-negative safe integer and expectedHash
to be a non-empty string; return HTTP 400 for invalid present values rather than
omitting them, while preserving omission when either field is absent. Update the
payload construction near the expectedRev/expectedHash spread expressions.
Source: Coding guidelines
| description: | ||
| "Replace slot content in place. Fails if content exceeds sizeLimit. Pass expectedRev (or expectedHash) from your memory_slot_get read so a concurrent write by another session is rejected instead of silently clobbered; the previous content is snapshotted to slot history either way.", | ||
| inputSchema: { | ||
| type: "object", | ||
| properties: { | ||
| label: { type: "string", description: "Slot label" }, | ||
| content: { type: "string", description: "New full content" }, | ||
| expectedRev: { | ||
| type: "number", | ||
| description: | ||
| "rev returned by memory_slot_get. If it no longer matches, the replace is rejected — re-read and merge.", | ||
| }, | ||
| expectedHash: { | ||
| type: "string", | ||
| description: "contentHash returned by memory_slot_get; alternative to expectedRev.", | ||
| }, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the snapshot behavior description.
A stale or oversized replacement returns before snapshotSlot() runs. The description says that the previous content is snapshotted “either way.” State that successful replacements snapshot the pre-write content.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp/tools-registry.ts` around lines 917 - 932, The replace-slot tool
description incorrectly claims previous content is snapshotted for every
attempt. Update the description near the replace operation to state that
successful replacements snapshot the pre-write content, while preserving the
existing stale-revision and size-limit failure behavior.
1 participant
Problem
Slots are the one part of agentmemory that several sessions write to by hand, and
mem::slot-replaceis a whole-object write with no guard:memory_slot_getand thenmemory_slot_replaceproduce a silent clobber — the second write wins and the first lane's content is gone, with no error and no copy anywhere. The audit row records only lengths.sizeLimitis only felt when an append fails, and by then the only fix is a full replace — i.e. the risky operation above.Change
Backward compatible; a caller that passes nothing behaves exactly as today.
1. Optimistic concurrency.
mem::slot-replaceacceptsexpectedRev(orexpectedHash) from the read it merged from. If the slot has moved on, the write is rejected with the current rev and hash instead of overwriting:revstarts at 0 for existing slots and increments on every append/replace.2. Write history. Every append and replace snapshots the previous content to
mem:slots:history, keeping the most recentAGENTMEMORY_SLOT_HISTORYper label (default 20).mem::slot-historylists entries;restore: truereturns the exactmem::slot-replacecall that puts the old content back. Snapshotting is best-effort — a slot write is never failed because its undo copy could not be stored.3. Headroom on every read and write.
size,sizeLimit,free,pctUsed,rev,contentHash, pluswarningat ≥70% and ≥90%:{ "size": 11596, "sizeLimit": 12000, "free": 404, "pctUsed": 97, "warning": "slot is 97% full (404 chars free) — compact it now; the next append of this size will fail" }MCP plumbing.
src/mcp/server.tsrebuilds the slot-replace payload field by field, soexpectedRev/expectedHashare forwarded there and declared in the tool schema. Without that the guard exists server-side and never fires for the agents that actually call it — which is how I first "verified" it working when it wasn't.Verification
test/slots-concurrency.test.ts(new, 6 cases): headroom + warning on read; staleexpectedRevrejected and content untouched; current rev accepted and bumped;expectedHashboth ways; history records both operations in order and hands back the undo; a replace with no expectation still succeeds.Full suite green (1602 passed, 1 skipped) and
npm run buildclean.Note for anyone reproducing: run the suite with an isolated
HOME. On a machine with a real~/.agentmemory/.env, config merging pulls the developer's own keys and flags into 14 unrelated tests.Summary by CodeRabbit