memory: gate write over an existing file, report the delta
write replaces a whole file and reported success identically whether the file grew, held, or was erased. Replacing an existing file now requires overwrite: true, and the refusal names append/edit as the likely intent. Every write returns a WriteDelta so the result carries magnitude; a write that drops a file below half its former size says so loudly. Internal callers (archivist, compaction, subconscious ledgers, REST) keep the ungated path — their replacement is structural.
This commit is contained in:
parent
d290b12441
commit
e498e72b77
2 changed files with 219 additions and 10 deletions
|
|
@ -92,7 +92,15 @@ pub enum MemoryCommand {
|
|||
/// Read a memory file by label (path relative to memory dir, .md optional).
|
||||
Read { path: String },
|
||||
/// Write content to a memory file (creates or replaces).
|
||||
Write { path: String, content: String },
|
||||
///
|
||||
/// `overwrite` is the deliberate-replacement gate: writing over an
|
||||
/// existing file requires it. Default false, so the destructive case is
|
||||
/// never the accidental one.
|
||||
Write {
|
||||
path: String,
|
||||
content: String,
|
||||
overwrite: bool,
|
||||
},
|
||||
/// Append content to a memory file.
|
||||
Append { path: String, content: String },
|
||||
/// List files in a memory directory.
|
||||
|
|
@ -112,6 +120,62 @@ pub enum MemoryCommand {
|
|||
Audit,
|
||||
}
|
||||
|
||||
/// A write shrinking a file below this fraction of its former size is
|
||||
/// reported loudly. Half is arbitrary but catches the real failure: a
|
||||
/// placeholder body replacing an accumulated file.
|
||||
const SHRINK_ALARM_RATIO: f64 = 0.5;
|
||||
|
||||
/// What a write did to a file, in bytes.
|
||||
///
|
||||
/// Reported back to the caller because silence about magnitude is what let a
|
||||
/// 22 KB state file become one line with nothing looking wrong: the success
|
||||
/// message was identical whether the file grew, held, or was erased. Size is
|
||||
/// whole-file (frontmatter included) on both sides so the numbers compare.
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub struct WriteDelta {
|
||||
/// Whether the file was already there before this write.
|
||||
pub existed: bool,
|
||||
pub old_bytes: usize,
|
||||
pub new_bytes: usize,
|
||||
}
|
||||
|
||||
impl WriteDelta {
|
||||
/// True when this write replaced most of an existing file.
|
||||
pub fn shrank_hard(&self) -> bool {
|
||||
self.existed
|
||||
&& self.old_bytes > 0
|
||||
&& (self.new_bytes as f64) < (self.old_bytes as f64) * SHRINK_ALARM_RATIO
|
||||
}
|
||||
|
||||
/// Human-facing summary — always carries the magnitude.
|
||||
pub fn summary(&self, label: &str) -> String {
|
||||
if !self.existed {
|
||||
return format!("Created memory file: {} ({} bytes)", label, self.new_bytes);
|
||||
}
|
||||
let delta = self.new_bytes as i64 - self.old_bytes as i64;
|
||||
let pct = if self.old_bytes == 0 {
|
||||
0.0
|
||||
} else {
|
||||
(delta as f64 / self.old_bytes as f64) * 100.0
|
||||
};
|
||||
let mut s = format!(
|
||||
"Wrote memory file: {} ({} bytes, was {}, {}{:.0}%)",
|
||||
label,
|
||||
self.new_bytes,
|
||||
self.old_bytes,
|
||||
if delta >= 0 { "+" } else { "" },
|
||||
pct
|
||||
);
|
||||
if self.shrank_hard() {
|
||||
s.push_str(
|
||||
"\n⚠ This replaced most of the file. If that was not intended, the previous \
|
||||
version is one commit back — every memory write is a commit.",
|
||||
);
|
||||
}
|
||||
s
|
||||
}
|
||||
}
|
||||
|
||||
// ── Git-backed Memory Repository ───────────────────────────────────────────
|
||||
|
||||
/// A git-backed memory repository for a single agent.
|
||||
|
|
@ -310,7 +374,11 @@ impl MemoryRepo {
|
|||
///
|
||||
/// Content should NOT include frontmatter — it will be added automatically.
|
||||
/// If the file exists, its frontmatter is preserved (unless changing read_only).
|
||||
pub async fn write(&self, label: &str, body: &str) -> Result<()> {
|
||||
///
|
||||
/// This is the ungated path, for callers whose replacement is structural
|
||||
/// (archivist output, compaction summaries, the REST surface). Agent-facing
|
||||
/// writes go through [`write_guarded`](Self::write_guarded).
|
||||
pub async fn write(&self, label: &str, body: &str) -> Result<WriteDelta> {
|
||||
let path = self.resolve_path(label);
|
||||
|
||||
// Ensure parent directory exists
|
||||
|
|
@ -324,8 +392,11 @@ impl MemoryRepo {
|
|||
let (supplied, body) = split_supplied_frontmatter(body);
|
||||
|
||||
// Get existing frontmatter or use default
|
||||
let base = if path.exists() {
|
||||
let mut old_bytes = 0usize;
|
||||
let existed = path.exists();
|
||||
let base = if existed {
|
||||
let existing = tokio::fs::read_to_string(&path).await?;
|
||||
old_bytes = existing.len();
|
||||
let parsed = parse_memory_file(&existing)?;
|
||||
if parsed.frontmatter.read_only.as_deref() == Some("true") {
|
||||
return Err(anyhow!("memory file is read_only: {}", label));
|
||||
|
|
@ -371,6 +442,7 @@ impl MemoryRepo {
|
|||
}
|
||||
|
||||
let rendered = render_frontmatter(&frontmatter, body);
|
||||
let new_bytes = rendered.len();
|
||||
tokio::fs::write(&path, &rendered)
|
||||
.await
|
||||
.with_context(|| format!("writing memory file: {}", label))?;
|
||||
|
|
@ -379,7 +451,41 @@ impl MemoryRepo {
|
|||
self.commit(&[label], &format!("memory write: {}", label))?;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
Ok(WriteDelta {
|
||||
existed,
|
||||
old_bytes,
|
||||
new_bytes,
|
||||
})
|
||||
}
|
||||
|
||||
/// Agent-facing write, with the existence gate.
|
||||
///
|
||||
/// `write` replaces a whole file, and its success message used to look
|
||||
/// identical whether the file grew or was erased. So the destructive case
|
||||
/// now has to be asked for: replacing an existing file requires
|
||||
/// `overwrite`. The refusal names the alternatives, because the mistake is
|
||||
/// almost always reaching for `write` when `append` or `edit` was meant.
|
||||
pub async fn write_guarded(
|
||||
&self,
|
||||
label: &str,
|
||||
body: &str,
|
||||
overwrite: bool,
|
||||
) -> Result<WriteDelta> {
|
||||
let path = self.resolve_path(label);
|
||||
if path.exists() && !overwrite {
|
||||
let old_bytes = tokio::fs::metadata(&path)
|
||||
.await
|
||||
.map(|m| m.len())
|
||||
.unwrap_or(0);
|
||||
return Err(anyhow!(
|
||||
"memory write refused: {} already exists ({} bytes), and `write` replaces the \
|
||||
whole file. Did you mean `append` (add to the end), `edit` (change one part), \
|
||||
or `write` with `overwrite: true` (replace it deliberately)?",
|
||||
label,
|
||||
old_bytes
|
||||
));
|
||||
}
|
||||
self.write(label, body).await
|
||||
}
|
||||
|
||||
/// Append content to a memory file.
|
||||
|
|
@ -1070,12 +1176,16 @@ pub async fn execute_memory_command_with_context(
|
|||
// what a rewrite would produce.
|
||||
Ok(render_frontmatter(&file.frontmatter, &file.body))
|
||||
}
|
||||
MemoryCommand::Write { path, content } => {
|
||||
repo.write(path, content).await?;
|
||||
MemoryCommand::Write {
|
||||
path,
|
||||
content,
|
||||
overwrite,
|
||||
} => {
|
||||
let delta = repo.write_guarded(path, content, *overwrite).await?;
|
||||
if let Some(c) = ctx {
|
||||
fire_memfs_commit(c, &repo, "write", &[path], 0.1);
|
||||
}
|
||||
Ok(format!("Wrote memory file: {}", path))
|
||||
Ok(delta.summary(path))
|
||||
}
|
||||
MemoryCommand::Append { path, content } => {
|
||||
repo.append(path, content).await?;
|
||||
|
|
@ -1221,7 +1331,14 @@ pub async fn handle_memory_tool_with_context(
|
|||
.and_then(|v| v.as_str())
|
||||
.unwrap_or("")
|
||||
.to_string();
|
||||
MemoryCommand::Write { path, content }
|
||||
MemoryCommand::Write {
|
||||
path,
|
||||
content,
|
||||
overwrite: input
|
||||
.get("overwrite")
|
||||
.and_then(|v| v.as_bool())
|
||||
.unwrap_or(false),
|
||||
}
|
||||
}
|
||||
"append" => {
|
||||
let path = input
|
||||
|
|
@ -1318,7 +1435,7 @@ pub fn memory_tool_definition() -> ToolDefinition {
|
|||
|
||||
Subcommands:
|
||||
read — Open a memory file. Frontmatter is handled for me — I see the body.
|
||||
write — Write to a memory file. Frontmatter is preserved or auto-generated.
|
||||
write — Create a memory file, or replace one whole. Replacing an existing file requires `overwrite: true` — `write` is not `append`.
|
||||
append — Add to a memory file without disturbing its frontmatter.
|
||||
ls — List files in a memory directory.
|
||||
status — Check my memory's git state: uncommitted changes, last commit.
|
||||
|
|
@ -1345,6 +1462,10 @@ Paths are relative to my memory directory. Frontmatter description is required o
|
|||
"type": "string",
|
||||
"description": "Content to write or append — body only, no frontmatter"
|
||||
},
|
||||
"overwrite": {
|
||||
"type": "boolean",
|
||||
"description": "For `write` only. Required to replace a file that already exists — writing over accumulated memory has to be deliberate. Default false."
|
||||
},
|
||||
"strategy": {
|
||||
"type": "string",
|
||||
"enum": ["microcompact", "sliding_window", "sliding_reflect", "summary", "cull"],
|
||||
|
|
@ -1363,6 +1484,91 @@ mod tests {
|
|||
use super::*;
|
||||
use tempfile::TempDir;
|
||||
|
||||
/// The failure this guard exists for: `write` aimed at an accumulated
|
||||
/// file, meaning `edit`. It must refuse rather than succeed quietly.
|
||||
#[tokio::test]
|
||||
async fn write_guarded_refuses_to_replace_an_existing_file() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let repo = MemoryRepo::open("test-agent", tmp.path().to_path_buf());
|
||||
repo.init().await.unwrap();
|
||||
|
||||
// Seed through the ungated path: init() already creates system/state,
|
||||
// so reaching for the gated one here would trip the very guard we are
|
||||
// about to test.
|
||||
let long_body = "accumulated state\n".repeat(500);
|
||||
repo.write("reference/accumulated", &long_body).await.unwrap();
|
||||
|
||||
let err = repo
|
||||
.write_guarded("reference/accumulated", "(rest unchanged)", false)
|
||||
.await
|
||||
.unwrap_err()
|
||||
.to_string();
|
||||
assert!(err.contains("already exists"), "got: {err}");
|
||||
assert!(err.contains("append"), "refusal must name the alternatives");
|
||||
assert!(err.contains("edit"), "refusal must name the alternatives");
|
||||
|
||||
// And the file is untouched.
|
||||
let still_there = repo.read("reference/accumulated").await.unwrap();
|
||||
assert!(still_there.body.len() > 1000);
|
||||
}
|
||||
|
||||
/// Passing the gate deliberately works, and the result carries magnitude.
|
||||
#[tokio::test]
|
||||
async fn write_guarded_with_overwrite_reports_the_shrink() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let repo = MemoryRepo::open("test-agent", tmp.path().to_path_buf());
|
||||
repo.init().await.unwrap();
|
||||
|
||||
let long_body = "accumulated state\n".repeat(500);
|
||||
repo.write("reference/accumulated", &long_body).await.unwrap();
|
||||
let delta = repo
|
||||
.write_guarded("reference/accumulated", "(rest unchanged)", true)
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
assert!(delta.existed);
|
||||
assert!(delta.shrank_hard(), "99% truncation must trip the alarm");
|
||||
let summary = delta.summary("reference/accumulated");
|
||||
assert!(summary.contains("was "), "summary must report the old size");
|
||||
assert!(summary.contains('⚠'), "hard shrink must be loud: {summary}");
|
||||
}
|
||||
|
||||
/// A new file needs no gate — the destructive case is the only gated one.
|
||||
#[tokio::test]
|
||||
async fn write_guarded_creates_without_overwrite() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let repo = MemoryRepo::open("test-agent", tmp.path().to_path_buf());
|
||||
repo.init().await.unwrap();
|
||||
|
||||
let delta = repo
|
||||
.write_guarded("journal/fresh", "a new thought", false)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(!delta.existed);
|
||||
assert!(!delta.shrank_hard());
|
||||
assert!(delta.summary("journal/fresh").contains("Created"));
|
||||
}
|
||||
|
||||
/// Growth is never alarming, and the summary still says by how much.
|
||||
#[tokio::test]
|
||||
async fn growth_is_reported_but_not_alarmed() {
|
||||
let tmp = TempDir::new().unwrap();
|
||||
let repo = MemoryRepo::open("test-agent", tmp.path().to_path_buf());
|
||||
repo.init().await.unwrap();
|
||||
|
||||
repo.write_guarded("journal/grow", "short", false)
|
||||
.await
|
||||
.unwrap();
|
||||
let delta = repo
|
||||
.write_guarded("journal/grow", &"much longer\n".repeat(100), true)
|
||||
.await
|
||||
.unwrap();
|
||||
assert!(!delta.shrank_hard());
|
||||
let summary = delta.summary("journal/grow");
|
||||
assert!(summary.contains('+'), "growth should show a + delta");
|
||||
assert!(!summary.contains('⚠'));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_parse_frontmatter() {
|
||||
let content = "---\ndescription: Test file\nread_only: false\n---\nHello world";
|
||||
|
|
|
|||
|
|
@ -281,7 +281,10 @@ impl SubconsciousInbox {
|
|||
} else {
|
||||
serde_yaml::to_string(items).context("serializing inbox items")?
|
||||
};
|
||||
self.repo.write(path, &body).await
|
||||
// Structural rewrite of a machine-owned ledger — the delta is not
|
||||
// interesting here, only that it landed.
|
||||
self.repo.write(path, &body).await?;
|
||||
Ok(())
|
||||
}
|
||||
}
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue