# Memory Write Fix Plan

**Goal:** prevent `memory_write` from polluting `MEMORY.md` with empty entries (orphan date markers) and content-level duplicates. Centralize the write logic in `core/memory.js#writeMemory` so all callers (the tool + compaction-side auto-extract) get the same guards.

---

## §1 — Files affected

| File | Change |
|---|---|
| `core/memory.js` | Add empty-content rejection; add idempotent dedup (skip if entry already present). Both happen INSIDE `writeMemory` so all callers benefit. Return value gains a status enum. |
| `tools/memory_write.js` | Stop reimplementing append+timestamp. Delegate to `writeMemory`. Map its return value to a tool-style string. |
| `core/compaction.js:82` | No call-site change needed — already calls `writeMemory`. New guards apply automatically. |

No schema changes. No new files. No migration.

---

## §2 — `core/memory.js#writeMemory` — exact behavior

### 2.1 Function signature stays the same

```js
function writeMemory({ cwd, agentName, scope = 'agent', content, maxLines = MAX_LINES_DEFAULT })
```

### 2.2 Return value changes from `void` to a status object

```js
return { status: 'written' | 'skipped_empty' | 'skipped_duplicate', filePath };
```

- `'written'` — entry was appended (and possibly rotated).
- `'skipped_empty'` — content was null / whitespace-only after `.trim()`. No file I/O.
- `'skipped_duplicate'` — trimmed content already substring-present in the existing `MEMORY.md`. No file I/O.

Why an enum: the tool wrapper can map each branch to the right user-facing string. Compaction-side caller can ignore the result (current call doesn't read return value, so this is non-breaking).

### 2.3 Empty-content rejection

```js
const trimmed = (content || '').trim();
if (!trimmed) return { status: 'skipped_empty', filePath: null };
```

- Runs BEFORE `mkdirSync`, BEFORE any file read/write. No side effects when rejected.
- `filePath: null` because we don't compute the path on the empty branch.

### 2.4 Substring dedup

```js
fs.mkdirSync(dir, { recursive: true });
const filePath = path.join(dir, 'MEMORY.md');

let existing = '';
if (fs.existsSync(filePath)) {
  existing = fs.readFileSync(filePath, 'utf8');
}
if (existing.includes(trimmed)) {
  return { status: 'skipped_duplicate', filePath };
}
```

Substring match (not normalized hash). Tradeoffs explicitly accepted:

- New entry that's a substring of older content → rejected. Acceptable: agent should write distinctive content.
- Older content that's a substring of new entry → not detected by this check; new entry is appended. Acceptable: this case is rarer and the new entry strictly supersedes the old (bigger).
- Whitespace differences inside the body → treated as different. Acceptable: agent prompts shouldn't rely on whitespace coincidence.

### 2.5 Append + rotation (unchanged)

After dedup passes:

```js
const timestamp = new Date().toISOString().slice(0, 10);
const entry = `\n\n<!-- ${timestamp} -->\n${trimmed}`;
fs.appendFileSync(filePath, entry, 'utf8');
// existing rotation logic stays
```

Note: use `trimmed` (not `content.trim()`) to avoid the double-trim. Behaviorally identical, just less wasteful.

---

## §3 — `tools/memory_write.js` — delegation

Replace the body with:

```js
const { writeMemory } = require('../core/memory');

async function execute({ content, scope = 'agent', _cwd, _agent }) {
  const result = writeMemory({
    cwd: _cwd,
    agentName: _agent.name,
    scope,
    content,
  });

  switch (result.status) {
    case 'written':
      return `Memory written to ${scope} scope (${result.filePath})`;
    case 'skipped_empty':
      return 'Error: VALIDATION_ERROR — memory_write content cannot be empty';
    case 'skipped_duplicate':
      return `Memory entry already present (no-op) — ${result.filePath}`;
    default:
      return `Memory write returned unknown status: ${result.status}`;
  }
}
```

### Removed from the tool

- The local `path` import (delegating means no path manipulation here).
- The local `fs` import.
- The local timestamp + entry construction.
- The local `mkdirSync` + `appendFileSync` calls.

### Schema unchanged

`content` (required string) + `scope` (optional 'agent' | 'global'). Backwards compatible.

---

## §4 — Tests

Add to `test/integration/` or a new standalone smoke (the existing 09-memory-analyst test already covers happy-path append; we add dedup + empty cases). Three new assertions:

1. **Empty content rejected:** call `writeMemory({ cwd, agentName: 'a', scope: 'agent', content: '' })` → returns `{status: 'skipped_empty'}`. Check that the file does NOT exist (or did not grow).
2. **Whitespace-only rejected:** same as above with `content: '   \n\t  '` → `skipped_empty`.
3. **Duplicate skipped:** two consecutive calls with identical content → first returns `'written'`, second returns `'skipped_duplicate'`. Check file size is the same after both calls.

Tests live in a single self-contained file: `test/memory-write-guards.js` (standalone, no server, similar pattern to `test/phase-3-suite.js`). Cleans up its own scratch dir.

---

## §5 — Acceptance criteria

| # | Check |
|---|---|
| 1 | `writeMemory({ content: '' })` returns `{status:'skipped_empty', filePath:null}` and creates no file. |
| 2 | `writeMemory({ content: 'abc' })` twice in a row produces one entry; second returns `{status:'skipped_duplicate'}`. |
| 3 | `writeMemory({ content: 'abc' })` then `writeMemory({ content: 'def' })` produces two distinct entries. |
| 4 | `tools/memory_write.js` no longer imports `fs` or `path` directly. |
| 5 | `tools/memory_write.js` calls `writeMemory` (not its own append logic). |
| 6 | `node test/memory-write-guards.js` runs green. |
| 7 | `node test/phase-3-suite.js` still runs green (no regression). |

---

## §6 — Out of scope

- Hash-based dedup, content-canonicalization, name/key replacement semantics — explicit non-goals for this fix. Substring is enough for the observed bug.
- Cleaning up agent-level `MEMORY.md` files (`.veil/memory/agents/*/MEMORY.md`) — not touched.
- Refactoring `core/memory.js`'s rotation logic — works as-is.
- `memory_search.js` matching HTML date markers — surfaced by criticizer earlier but separate fix.