Fix #32 snapshot deletion semantics - #42
Conversation
The stable snapshot exists to keep the system prompt byte-identical between turns, but it refreshed on long-term writes and day rollover, and stamped a second-precision timestamp plus a reason word into the injected header. Both rewrite the prompt tail and void the prefix cache for the whole conversation - on the most common in-session event. - refresh only when no snapshot exists (session_start / after compact) - drop the volatile "Snapshot <reason> at <hh:mm:ss>" caveat line - deliver deletions/restores as an injected session message instead of re-rendering the block - add PI_MEMORY_SNAPSHOT=refresh for the previous behaviour Signed-off-by: RaduAdumitroaei <radueugen84@yahoo.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 682900399f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const today = todayStr(); | ||
| const needsRefresh = memorySnapshot === null || snapshotDirty || snapshotTakenOnDate !== today; | ||
| if (needsRefresh) { | ||
| const stale = mode === "refresh" && (snapshotDirty || snapshotTakenOnDate !== today); |
There was a problem hiding this comment.
With the default stable mode, a memory_write using target: "long_term" and mode: "overwrite" still sets snapshotDirty, but this condition now ignores it. Consequently, every fact removed by the overwrite remains in the injected system prompt until compaction or a new session, even though it no longer exists in MEMORY.md; the new tool arguments do not explicitly retract those higher-priority stale facts. Treat overwrites as authority-changing operations and refresh the snapshot as is done for memory_forget.
Useful? React with 👍 / 👎.
| snapshotDirty = true; | ||
| // Restore changes which durable facts are authoritative, so refresh the | ||
| // snapshot instead of persisting restored content in a correction message. | ||
| refreshMemorySnapshot("memory_restore"); |
There was a problem hiding this comment.
This changes memory_restore from lazy dirty-marking to an immediate snapshot rebuild, but the new snapshot regression covers only memory_forget; existing restore tests verify disk contents without calling before_agent_start. Add a test that forgets and restores an injected entry and verifies the next stable prompt contains the restored content without a correction message, as repository policy requires regression coverage for every bug fix.
AGENTS.md reference: AGENTS.md:L40-L42
Useful? React with 👍 / 👎.
| |----------|--------|---------|-------------| | ||
| | `PI_MEMORY_DIR` | path | `~/.pi/agent/memory` | Override the memory storage directory | | ||
| | `PI_MEMORY_SNAPSHOT` | `stable`, `per-turn` | `stable` | `stable` snapshots memory at checkpoints for KV cache stability; `per-turn` rebuilds every turn (legacy behavior) | | ||
| | `PI_MEMORY_SNAPSHOT` | `stable`, `refresh`, `per-turn` | `stable` | `stable` snapshots once at session start and never re-renders it (deletions append a correction); `refresh` also re-renders on long-term writes and day rollover; `per-turn` rebuilds every turn (legacy behavior) | |
There was a problem hiding this comment.
The configuration table says stable-mode deletions append a correction, but this commit removes correction messages and immediately rebuilds the snapshot in memory_forget and memory_restore; the preceding behavior section also says they refresh. Update this row so users are not given contradictory cache and persistence semantics.
Useful? React with 👍 / 👎.
2 participants
Follow-up to #32. Keeps stable snapshots for normal in-session writes, but refreshes the snapshot for explicit memory_forget/memory_restore operations. This removes the 20-correction correctness cap and avoids copying forgotten content into persisted session messages.