Forty review rounds on code that had already been reviewed

Last time I wrote about skillmem — a local memory for coding agents — it was about a hole we found in it: a stranger's README could come back a day later looking like the user's own rule. Two independent reviews later, 0.10 closed it, and I believed the hole was one and it was shut.

0.11.0 shipped yesterday. Between the two releases: forty review rounds by two models on opposite sides (one Claude-side, one GPT-side), each reading the code as an adversary and required to reproduce every finding with a command, not an argument. Here is what came out, and why I no longer trust a clean review that isn't followed by another one.

The stop rule

"Review until there are no bugs" does not terminate: fresh eyes on any module always find something. We agreed on this instead: stop when two consecutive rounds produce no reproducible P1/P2 from either reviewer — data loss, wrong answer, a crossed trust boundary, a crash on realistic input. P3s go to the next release's issue.

It fired on rounds 39–40. The path there was not straight.

Three waves

Wave 1 (rounds 1–18): the audit. Six P1s, none in the Stop hook everyone had been staring at. HTTP /write could take over another agent's record; /learn wrote public skills without the permission; body files were shared between databases; trust could be granted by any process; kind was a path traversal in the exporter. Fixed; three rounds spent fixing what the fixes broke.

Wave 2 (rounds 19–23): rehearsing the release. I wrote the release procedure as a spec and handed it to the same reviewers. Rehearsing init on a copy of the config found that moving the venv doubled every hook — the recap would have run twice per session. Then, in a chain: config backups named by the second overwrote each other; they were created 0644 next to the file holding the OAuth account; the Codex backup was not byte-exact. Eleven P2s in installer code that "worked".

Wave 3 (rounds 24–40): fresh eyes on the core. The interesting part. Every time a reviewer got a module nobody had re-read, they found one to three old P2s inherited from main:

• The multi-agent HTTP server: a 409 conflict quoted the titles of another agent's private records. Backlinks on a public record named a private one's slug. /search, /list and /recall cut the page before the visibility filter — 110 of someone else's records and your own came back as an empty 200 while /get found it.
• import-vault followed a symlinked *.md out of the vault and stored the target — your ~/.zshrc, say. Attachments and packs already refused this; notes did not.
• skills rm <pack> removed every record under the pack's project, the owner's own note included.
• The Windows scheduler carried none of the environment launchd, cron and systemd did.
• My favourite: scrub, the function that redacts secrets before a write, was not idempotent. A value already rendered as [secret redacted] matched again on every re-write and grew into [secret redacted] redacted]. The content hash changed; the owner's approval was dropped. Restoring a dump over the same database silently stripped approvals.

Every second fix bred a regression

This is the lesson. The search-crowding fix took four iterations, and the next round caught each one:

1. Candidate window of 5, filtered after the limit: five hidden rows crowd yours out.
2. Window of 100: a hundred hidden rows do the same.
3. No limit, walk until N visible: without LIMIT, SQLite sorts every match before the first row comes out, and the wide SELECT dragged every body through the sort — 20 seconds per request on a 9k-row database.
4. Narrow walk, bodies only for the kept rows: 52 ms unfiltered, 73–87 filtered. Done? No: the predicate went to the master identity too, so master over HTTP ranked on an unbounded pool — a different top-5 than the CLI for five queries out of six.

Iteration five: master takes the unfiltered path, plus a test "HTTP master == S.search" that fails on the previous commit. Only then was the round clean.

Without a review after the first fix we would have shipped a 20-second /search, confident the leak was closed.

What is worth copying

• Two reviewers from different sides, not one. The models fail differently; over forty rounds the intersection of their findings was visibly smaller than the union.
• A rigid report format: P1|P2|P3 · file:line · what · evidence (command + output). "Looks racy" is a P3, not a gate.
• Reviewers don't write code. Twice in the cycle a reviewer broke read-only (an edit to pyproject.toml, a stray file beside the working clone) — check git status after every round.
• Every fix is a new round, one-liners especially.
• Name the stop rule up front, or the loop either never ends or ends where you got tired.

Cost: a day of machine time and ~350 tests instead of 221. Result, 0.11.0: the trust boundary closed on HTTP, MCP, the CLI and the hooks alike, and every line of the release notes tried with a command before it was written. What stayed P3 is in issue 0.11.1.

pip install skillmem · GitHub · Release notes

---

UPD Sep 18: this post was wrong — 0.11.0 had two more holes

The post ended on "the trust boundary is closed on HTTP, MCP, CLI and the hooks alike." A day later the reviewers showed it was not. Both holes let an agent neutralise the owner's own approved rule, and neither left a trace.

One: a rule leaves the briefing in a single call. An agent calls mem_write with a different kind — an approved feedback rule becomes a note, drops out of the SessionStart injection, keeps its approval, and writes zero history rows. The rule simply stops arriving, and nothing anywhere says why.

Two: approved text can be swapped underneath its approval. Bodies over the externalisation threshold live in a file, and nothing compared that file against the hash the row carries. One file write, no database access, and trusted_at, content_hash, updated_at and the tamper-evident chain all stay intact while the hooks serve the new words as the owner's rule.

Plus a third, Windows only: isatty() is true there for any character device, NUL included, so an agent running a command with stdin=DEVNULL passed for a person at a keyboard and reached every owner-only verb. The Windows CI jobs caught it on the release commit.

All closed in 0.11.1, each with a test that fails on 0.11.0.

What I learned about the process itself

The post praised the "two clean rounds in a row" stop rule. Over the next 26 rounds it was never met — and that matters more than the bugs.

About half the findings in the later rounds were regressions from the previous fix. Every one had one of exactly two shapes:

1. A guard placed in one caller instead of in the operation. The check went onto the MCP handler — HTTP was still open. We closed /update — /write and /learn were still open. We closed those — the vault importer was left, the eleventh caller. Only one thing worked: move the check inside the mutation every caller passes through, and make it fail closed.
2. A read, then a write, with a gap. Between reading a row and writing it, the owner can approve the record and an agent can delete or rewrite it. Nine mutations became one transaction each, and every one of those writes now carries "the row is not deleted".

I deleted the feature the release was for

0.11.1 was supposed to add mem_archive, a tool for an agent to retire a record that no longer applies. It produced 13 P1s across ten rounds and the finding curve went up. A tool that removes a record from every read while keeping its text, approval and origin works against the trust boundary it lives inside: every gate we built was one call from open — directly, then via an update, then via an export/import round trip, then via a kind change, then via the nightly sweep.

The tool is gone. Retiring a record is the owner's own terminal command, and there are nine tools again. Shipping less was the right answer, and it took me ten rounds to accept it.

And one experiment

Half the rounds were run by an unattended loop on a server: review, fix, test, commit, all night, with one rule — "if ten rounds in a row still find something, roll back to the start and begin again." It used that rule once. Result: 415 tests instead of 346, and the same two shapes in its findings as in mine. Which confirms the uncomfortable part: the problem was never the person doing the fixing.

pip install -U skillmem · 0.11.1 release