Round-24 review. One actionable finding, verified: 4.2 promised
contradiction handling "at L2/L3", but 4.2.1's detector groups atoms by
`project` and an L3 atom is scope=global with no `project` field — the
schema forbids one. Detection can never fire against L3, the one tier
that is always in context and never auto-demoted.
Scoped 4.2 to L1/L2 and split detection from rendering: the injected
`[contested]` tag stays tier-agnostic, since the schema permits
`contested` anywhere and a human can set it at adopt.
Added 9.6 rather than a loop change, because the L3 case is
underdetermined, not unbuilt. A project claim that differs from a global
one is either a correction to an over-generalised L3 atom or a
legitimate local exception, and no string-shape rule separates them.
Guessing "correction" is the expensive direction: it would tag the
persona tier as unreliable the first time any project deviates —
section 1's false-permanence failure, inverted. Records the live
consequence (5.1 injects L2 and L3 together, so two contradictory lines
can enter one context block unmarked) and three candidates, leaning
specificity-wins for v1.
Also: the "53 checks in six families" claim in 10.1 was stale by two
rounds and a reviewer quoted it back. Rather than correcting the number,
made the program own it — a final check asserts the doc cites the count
actually executed, so the sentence cannot go stale again. Documented the
working invocation in 10.1 too; the `.txt` parking means nothing in CI
gates this yet, and `python3 <(cat ...)` does not work because path
resolution walks up from __file__.
Verified: 69 checks, 0 failures; the count check fails when the doc
disagrees. derive_counters.py --check unchanged (363/89/663).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Fourth PR-review round on #948. All four reproduced first.
1. The gate was one flag away from opt-out. --waive applied to whatever
gate_refusals() returned, including G1 "no review round has been collected",
so `close --waive "no time"` exited 0 with nobody having looked at the
artifact. That is the most tempting shortcut for an agent under time
pressure and it defeats the skill's whole premise.
G1 is now unwaivable, with its own refusal message: a waiver accepts
objections a reviewer raised, it cannot manufacture a review that never
happened. Waiving a genuine objection (G2/G3/G4/G7) still works.
2. Inline `style` was unsanitized, so a reviewed draft containing
`background-image:url(https://attacker/beacon.png)` fired a request the
moment the reviewer opened the page — no script needed, and directly
contrary to the no-network property the README and manifest advertise.
Added to DROP_ATTRS. The <style> tag was already dropped, so keeping the
attribute was inconsistent as well as leaky.
3. Markdown `` never rendered. LINK's regex was not anchored against
a preceding `!`, so an image became `!<a href=...>` — and _safe_href's
image=True branch, which exists to allowlist data:image URIs, was dead code
on that path. Added an IMAGE regex ahead of LINK, negative-lookbehind on
LINK, and real <img> rendering through the same scheme allowlist. Verified a
javascript: src degrades to inert alt text.
4. An unterminated <script> silently swallowed the rest of the body — same
confusing failure shape as the void-tag bug, though it fails safe. Now emits
a named diagnostic to stderr instead of vanishing.
Nit: raw-HTML `target="_blank"` anchors get the rel="noreferrer noopener" the
Markdown path already added to its own.
Re-verified: derive_counters --check, check_plugin_json --all, checklist 6/6
PASS, description validator PASS, all three scripts --help/--sample green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Round-23 review. All three findings verified against the tree first.
Fixture drift (3.1 L1 vs the schema's L2 example, captioned "one atom's
lifecycle read side by side"): the L1 snapshot carried the L2's
last_seen as its own first_seen, putting the first sighting three weeks
*after* the promotion it precedes. A second incoherence in the same pair
went unmentioned by the review and is arguably worse: the L1
back-pointer used the L2's `source` line (#L412) where its `first_source`
(#L77) belonged, which 3.1.1 forbids — promotion strips the path prefix
and nothing else, so a line number cannot drift across it. Both fixed by
anchoring the L1 fixture on the L2's first-sighting fields.
Added family 7 to the checker: within an atom, first_seen <= promoted_at
<= last_seen; across a lifecycle group, first_seen identical (4.1.1 takes
the min), last_seen/observations monotonic, sessions a superset, and
first_source surviving with only the prefix stripped. Families 1-6 all
passed the broken fixtures because each atom was independently
well-formed and nothing compared them — a prose claim that two fixtures
are one story imposes constraints no per-atom check can see. Verified by
re-injecting the old values: 2 failures, both named.
Manifest precedent was "5 for 5" and is 4 of 5. llm-wiki uses the same
on-disk nesting but declares the bare ["./skills"] — which IS one of root
CLAUDE.md's documented forms. That inverts the maintainer follow-up: the
question is which of two shapes the repo wants, not whether to bless an
undocumented fourth one, since a documented form already covers this
layout.
10.1 cited 644 -> 645 for the counter delta while a sibling file in the
same PR cited 663 -> 664. 663 is current; 644 predates book-to-skill.
Verified: 57 -> 68 checks, 0 failures; derive_counters.py --check
unchanged (363 skills, 89 plugins, 663 tools).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Three findings from the automated review, all contract-level:
- memory_schema.json: the tier=L3 -> promoted_from_projects conditional
was one-sided, so an L1/L2 atom carrying promoted_from_projects was
accepted. Added the else branch forbidding it outside L3 — the field
is not merely unnecessary there, it is meaningless, since scope is
still `project` and the array would claim cross-project evidence the
atom does not have.
- hooks.json: SessionStart declared no timeout and fell back to the
Claude Code default. Pinned to 5s. UserPromptSubmit keeps its 1s
backstop; SessionEnd stays async.
- DESIGN.md 4.1.1: state that the L2 -> L3 merge is lexical. normalize()
collapses whitespace/case/punctuation only, so two projects holding
the same rule in different words never merge. The failure is
one-directional — L3 under-fires, the claim stays live at L2 in each
project — and widening it needs an LLM or a per-user synonym table,
neither of which belongs in v1. Named in the same style as 4.2.1's
contradiction-detector limits.
Verified: assets/validate_examples.py.txt 57 checks / 0 failures; all
three schema examples still validate; an L1 atom carrying
promoted_from_projects is now rejected; derive_counters.py --check
passes unchanged (363 skills, 89 plugins, 663 tools).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Third PR-review round on #948. All three reproduced first.
1. HIGH — every realistic HTML5 document produced zero blocks. meta, link and
base are void elements: html.parser fires handle_starttag for them but never
a matching handle_endtag. They were also in DROP_TAGS, so each bare
`<meta charset>` incremented self._skip permanently and every subsequent
starttag/endtag/data callback inside <body> hit the skip guard. Result:
empty out, empty blocks, "No reviewable blocks found", exit 2.
The documented landing-page use case therefore did not work at all. It
survived three review rounds because every HTML fixture I wrote used only
<title>/<style> in head — the sanitizer test included. Void drop-tags no
longer touch the counter.
--sample now builds BOTH fixtures (Markdown + a full DOCTYPE HTML5 doc with
bare meta/link), asserts the expected block count for each, and exits 2 on
regression, so this cannot come back silently. It also writes to a temp dir
instead of cwd — the same class of mistake that leaked a stray artifact into
an earlier commit.
2. MEDIUM — xlink:href bypassed the URL allowlist. SVG anchors still honour it,
so `<svg><a xlink:href="javascript:alert(1)">` survived the hardening added
one commit earlier. Added with xlink:role and xlink:arcrole.
3. MEDIUM — status did not mirror close. It inspected only blocking_open, so a
round with no named reviewer reported exit 0 while close refused on G3 —
directly contradicting the exit-code contract the docstring advertises.
Both now call a shared gate_refusals(), so they cannot drift: verified they
agree on 2 (G3 open) and on 0 (clean round). status also prints which rules
would refuse rather than just a count.
Re-verified: derive_counters --check, check_plugin_json --all, checklist 6/6
PASS, description validator PASS, all three scripts --help/--sample green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Twenty-first review round. One fresh finding, and its sharp half is the
round-20 failure mode again in a new place.
validate_examples.py.txt derived BASE as dirname(dirname(__file__)) —
correct only at its current depth (assets/, two below the plugin root).
§10.1 moves it to skills/agent-memory/scripts/, four below, where that
form lands on the SKILL root instead. Verified by simulating both
layouts rather than reasoning about it:
- SCHEMA still resolved, by coincidence (assets/ and scripts/ become
siblings under the skill).
- DESIGN resolved to a path that does not exist and raised.
- The repo-root CLAUDE.md size check resolved to a nonexistent path and
was SKIPPED by its own `if os.path.exists(root)` guard — so the 84 KB
claim would stop being verified with the suite still printing green.
That last one is what makes this worth more than a path fix. Round 20 was
a whitelist that made a checker excuse the author's defect; this is a
guard that makes a checker excuse its own inability to run. Both report
success while checking nothing. A check that cannot locate its input has
FAILED, not passed, so the exists() guard is now an assertion.
BASE is anchored on a marker (walk up to DESIGN.md) instead of a fixed
dirname count, so the move cannot quietly disable anything. The repo root
is found by predicate — the directory holding BOTH CLAUDE.md and
scripts/derive_counters.py — because `.claude-plugin` alone is not a
repo-root marker: engineering/ carries one too, and anchoring on it lands
one level short. My first attempt did exactly that and the new assertion
caught it, which is the argument for the assertion.
Verified in three layouts: current (57 checks, 0 failures), the simulated
post-§10.1 move with links updated (57 checks, 0 failures — identical
count, so nothing is dropped by the move), and an orphaned copy with no
repo root above it (fails loudly, exit 1).
Counters unchanged: 363 / 89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Twentieth review round. One real defect, and the worse half is mine.
examples[0] embedded a $comment inside the atom object. `examples` entries
are INSTANCE data, and the schema declares additionalProperties: false
with no $comment in properties — so under any real validator (jsonschema,
ajv) that fixture fails the very schema it exists to demonstrate. The
distinction that makes this a bug rather than a style choice: $comment is
a SCHEMA keyword. It is legal at schema top level and inside the allOf
branches — where it still lives, untouched — and illegal in an instance.
The worse half: validate_examples.py.txt computed its allowed-property set
as `set(schema["properties"]) | {"$comment"}`. I wrote that whitelist to
make my own annotation pass. That is not a missing check; it is a check
deliberately weakened to tolerate the author's defect, which is strictly
worse than the round-8 happy-path gap — that one failed to look, this one
looked and was told to ignore what it saw. It also falsified the PR's own
testing claim, which said the examples validate against
additionalProperties when what they validated against was a checker built
to excuse them.
Fixed by removing the annotation from the fixture (option b) rather than
widening the schema, because instance data must be valid instance data.
The rationale it carried — why a third, L1 fixture exists at all: it is
the only thing exercising the L1 branch of the back-pointer conditional,
the one with PII consequences — now lives in §3.1 prose, where an
annotation about the fixtures belongs.
The workaround is gone from the checker, with a comment recording why it
must not come back. Verified both directions: re-injecting a $comment into
an example now fails "declares no unknown fields" (exit 1), and an
independent pass confirms all three fixtures carry zero undeclared keys.
Counters unchanged: 363 / 89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Nineteenth review round, no blocking issues. Two acted on.
1. §4.1.1's "age >= 30 days" gate and §4.3's demotion both key off
promoted_at, but nothing required it — so an atom could reach L2
without recording when, never satisfy the age test, and sit
un-promotable forever without anything flagging it. Exactly the
silent-skip mode that made `redacted` required last round.
Now conditionally required at L2/L3. Deliberately NOT forbidden at L1,
which the reviewer's phrasing already got right and which matters more
than it looks: §4.3 demotes an expired L2 claim BACK to L1, and that
atom legitimately keeps the promoted_at from its earlier life —
forbidding it there would make every demoted atom invalid. The
asymmetry is documented in the field description rather than left for
someone to rediscover.
Added to the committed checker too, not just the schema (56 checks now,
was 54): a schema conditional nothing exercises is the same class of
gap as the rule it fixes. Verified — deleting promoted_at from the L2
fixture fails "committed tier records promoted_at", exit 1.
2. §8's citations were unverifiable to a reviewer without the upstream
repo. They are now checkable: the clone was read at commit
b44c6db5f5b1a011eed645efb1949840f99f961a (2026-08-05, tip of main at
inspection), and the Chinese source comment quoted in point 1 is
verbatim from MemoryProxy/src/agent-adapters/claude-code.ts lines 2-6.
Pinning the sha means the citation stays checkable even after upstream
moves.
Not acted on: the root CLAUDE.md pointer (same answer as last round — it
advertises the convention before it is ruled on, so it lands with the
maintainer's yes), and blessing the .py.txt parking pattern, which is the
same convention call and belongs in the same decision.
Counters unchanged: 363 / 89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
README and SKILL.md were corrected in 70c908a; the manifest still carried the
flat 'zero network requests'. The page makes no request of its own, but a
reviewed HTML artifact's own https: assets do load. Also notes the HTML
sanitization added in the same commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Eighteenth review round, no correctness bugs found. Two items acted on.
1. §6 rule 1 calls redaction "non-negotiable ... before any write", but
`redacted` was optional in the schema — so an atom that skipped the
pass entirely was schema-VALID, which is precisely the state the rule
forbids. The reviewer's framing is right: an invariant the schema
does not carry is a promise, not a guarantee.
`redacted` is now the 13th unconditionally required field. All four
fixtures already set it, so the rule is enforced immediately rather
than aspirationally, and memory_promote.py can now CHECK that
redaction ran instead of trusting every writer to remember. §3.1
separates it from the genuinely later-stage fields (contested,
contested_by, promoted_at) and says why it differs: those are set by
contradiction handling and promotion, whereas redaction has already
run by the time any atom exists on disk.
2. validate_examples.py.txt's published_normalize() did a bare
re.search(...).group(0) — a moved or renamed fence would raise a raw
AttributeError from inside a checker whose entire job is to fail
legibly. Now three explicit SystemExit paths (fence missing, fence
present but not valid Python, block ran but defined no normalize),
each naming the cause and the fix. Verified by renaming the function:
the checker prints the guidance and exits 1 rather than tracebacking.
Both regressions confirmed caught: dropping `redacted` from a fixture now
fails "has all required fields" (exit 1); the clean tree still passes 54
checks (exit 0).
Items 3 and 4 not acted on. 3 is the maintainer's convention call, now
raised by six reviewers. 4 (a pointer in root CLAUDE.md's Unreleased
section) is deliberately coupled to it — advertising this folder in the
repo's headline changelog would pre-empt the very decision I have been
declining to settle by commit, and would need reverting if the ruling
goes the other way.
Counters unchanged: 363 / 89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Second PR-review round on #948. All four reproduced before fixing.
1. HIGH — reviewed HTML executed in the review page. BlockTagger re-emitted
attributes verbatim, escaping values but never filtering attribute names or
URL schemes. The Markdown path has had _safe_href scheme-allowlisting all
along; the HTML path had nothing. Reproduced: a draft.html containing
`<img src=x onerror=...>`, `<a href="javascript:alert(1)">` and an <iframe>
passed straight into the page a reviewer opens — and reviewing a landing-page
draft is a documented use of this skill.
sanitize_attrs() drops on* handlers, srcdoc and srcset, and runs href/src/
action/formaction/poster/cite/background through the scheme allowlist.
_safe_href now strips control characters before reading the scheme (so
`java\tscript:` cannot smuggle one) and allows data:image only for image
attributes. DROP_TAGS removes iframe/object/embed/frame/base/applet as well
as script/style/head/link/meta. Verified: handlers, javascript: (plain and
tab-smuggled), and iframes all gone; https links and relative images kept.
2. MEDIUM — verify_quotes compared rendered text against raw markup. A quote
comes from window.getSelection(), which is what the browser rendered, so
selecting a sentence containing **bold**, `code` or a link never matched the
raw source. G7 had just made that blocking, so this refused legitimate
closes. Now matched against raw OR a rendered-text projection (inline markup
stripped for Markdown, tags stripped and entities unescaped for HTML). A
fabricated quote is still caught — verified both directions.
3. MEDIUM — state["waiver"] was never cleared, so after waived-close → reopen →
a clean round, close still printed the old waiver reason. For a tool whose
premise is an honest record of what was actually reviewed and waived, that is
its own integrity bug. Cleared whenever a close passes with zero refusals.
4. LOW — status returned 4 for both "no sidecar yet" and "collected, blockers
open". The blocked case now returns 2, matching close, so an agent can branch
on the exit code alone: 0 clear, 2 blocked, 3 collect, 4 nothing yet.
Also corrected an over-broad claim of my own: the page makes no network request
of its own, but a reviewed HTML artifact's own https: assets do load, as they
must for the review to be faithful. README and SKILL.md now say that precisely.
Re-verified: derive_counters --check, check_plugin_json --all, checklist 6/6
PASS, description validator PASS, all three scripts --help/--sample green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Seventeenth review round, nothing blocking. One suggestion acted on, two
already-open items left open.
The reviewer's point on automating the example<->schema check is right,
and the sharpest version of it is one I had not weighed when declining
this twice on counter grounds: my verification logic has lived only in
throwaway shell heredocs, so it dies with the session that wrote it. The
next person editing this spec inherits nothing. Drift between DESIGN.md,
the schema and the fixtures has been the DOMINANT defect class across the
review — required-field drift, a tier the examples never exercised, ids
that stopped reproducing, headings inserted out of order, a confidence
value contradicting its own lifecycle narrative. Losing the checks that
found those is the real cost.
Added assets/validate_examples.py.txt — stdlib-only, 54 checks in six
families. Two properties make it more than a linter:
1. It EXECUTES the normalize() that DESIGN.md publishes rather than
reimplementing it, so doc and fixtures cannot silently disagree.
2. It is tested against INJECTED regressions, not just the happy path.
Four deliberate defects — an unstripped back-pointer (the OS-username
leak), a broken tier->scope pair, a wrong id, a confidence downgrade
— each make it exit 1; the clean tree exits 0. A checker that only
ever passes proves nothing, which is why the happy-path-only version
of this check missed the round-8 gap.
Parked as .txt, not .py, deliberately: this PR is spec-only and a .py
here is counted by derive_counters.py (measured 663 -> 664), producing a
counted tool belonging to no plugin in a folder that deliberately has no
SKILL.md. New §10.1 records that, names it the FIRST file the
implementation PR should land — before memory_extract.py, since
everything else is written against the contract it guards — and states
the reversal condition: if a spec-stage folder may carry tooling, it
becomes a .py and three counter files move, nothing else.
Suggestions 2 and 3 (the DESIGN.md-only convention; resolving §9.2 before
merge) are maintainer decisions already surfaced in the PR body. Five
reviewers have now independently raised the first.
Counters unchanged: 363 / 89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Sixteenth review round. One real inconsistency, one scoping fix.
The §3.1 atom (atm_961f033d, L1) carries confidence "observed"; the same
id at L2 in the schema carries "stated" — and round 14 explicitly paired
those two as "one atom's lifecycle", so the pairing turned an
undocumented transition into a visible contradiction. Nothing in §4.1 or
§5.3's merge description said confidence could change at all.
This is not cosmetic: confidence selects both the session count (3/2/1)
and the distinct-days exemption, so WHEN it may change decides which gate
an atom is held to. Left unstated, two implementers would reasonably
build different machines.
Resolved by documenting the transition rather than flattening the
examples, because upgrading is correct on the merits — a claim the agent
inferred can later be stated outright or confirmed by a check, and
freezing confidence at extraction would hold the atom to a stricter gate
than its evidence warrants. New §4.1.3: total order observed < stated <
verified; merge takes max(existing, incoming); never downgrades, so a
weak re-observation cannot silently re-impose the slower gate; the gate
re-reads confidence at promotion time, not creation. §5.3's merge step
now lists it alongside observations and sessions.
Verification proves the example pair is a legal upgrade and that the L2
atom clears the gate its own confidence implies, rather than asserting
the prose is consistent.
Also scoped the fast paths to "(L1 → L2 only)" — L2 → L3 is gated on
distinct projects, not session count, so neither shortcut applies there.
Note: the heading-order linter added in round 13 caught me repeating the
round-13 mistake — §4.1.3 was inserted between 4.1.1 and 4.1.2. Fixed
before commit. That is the linter earning its keep one round after being
written.
Counters unchanged: 363 / 89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Both from PR review on #948, both reproduced before fixing.
G7 — a real hole in the skill's core promise. feedback_parser downgrades an
unrecognised severity heading to NIT and records it only as advisory prose in
`problems`, which cmd_close never read. Reproduced: a sidecar with `## BLOKCER`
carrying "No source. Do not ship this." collected as a NIT, and close exited 0 —
a reviewer's genuine blocker lost to a typo. Same gap covered EDIT items with no
`+ after:` line and quotes that do not appear in the target file.
close now refuses (exit 2) while the last collected round carries unresolved
integrity problems. Problems that already have their own rule are filtered via
GATED_ELSEWHERE so G2/G3 are not double-reported. Verified: typo'd severity,
missing EDIT replacement, and quote-not-in-file each refuse; a clean sidecar
still passes; a missing reviewer still reports G3 alone.
Stray artifact — quarterly-plan.review.html was committed at the repo root. It
came from a `review_page_builder.py --sample` run during the post-merge
verification sweep with cwd at the repo root, then got swept up by `git add -A`.
Removed, and .gitignore now covers `*.review.html` + `.human-gate/` so neither
this repo nor a user of the skill re-commits a disposable review page. The
sidecar (<artifact>.review.md) is deliberately NOT ignored — that is the
reviewer's feedback and belongs in git.
G7 documented in the script docstring, SKILL.md, README, the command, plugin.json
(description + derivation_note) and CHANGELOG. Not acted on: the reviewer's note
that cmd_status's success line is terse — they flagged it as "not a real issue"
and the JSON branch already carries blocking_open.
Re-verified: derive_counters --check passes, check_plugin_json --all 90/90,
write-a-skill checklist 6/6 PASS, description validator PASS, all three scripts
--help/--sample green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Fifteenth review round, no correctness findings. Three concrete gaps,
all worth closing before implementation starts.
1. §5.4 said a writer that cannot acquire the lock "gives up and drops
its atoms, logging the loss" — and never said where. That is the one
place in the design where data disappears silently, so an unspecified
destination made the sentence decorative. Pinned to
.memory/errors.log (gitignored, 0600, capped 200 lines), added to the
§6 layout, and explicitly NOT stderr: SessionEnd is async, so its
stderr reaches nobody, which would make "logging the loss" a fiction.
/cs:memory status surfaces recent entries — a log nobody is pointed at
is the same as no log.
2. §10's planned tree moves assets/ under skills/agent-memory/, which
silently breaks two references: DESIGN.md's relative link to the
schema, and the schema's own $id. DESIGN.md stays at the plugin root
(it documents the plugin, not the skill), so the link lengthens rather
than staying put. Both now called out in a table with their post-move
values, matching how every other forward-looking wrinkle in this doc
is handled.
3. Open decision #2 has a repo-wide cost if it resolves toward an LLM
extractor that nobody had priced. Root CLAUDE.md's anti-patterns bullet
reads "one documented, opt-in exception" and names skillopt-sleep by
file; a second LLM-calling script makes that sentence false. The
implementation PR would have to amend that bullet rather than become a
silent second carve-out. Recorded as part of the cost of (b) — it is a
real argument for the rule-based path beyond recall.
Also stated in the Status header why a design doc lives under
engineering/ rather than the gitignored documentation/ folder (nothing
there is visible in a PR), while keeping the "is this a repeatable
pattern" question explicitly open for the maintainer.
Link checking now strips code spans before resolving relative links —
the previous version flagged the illustrative post-move path in item 2's
table as a dead link, which it is not.
Counters unchanged: 363 / 89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
book-to-skill landed in dev while this branch was open. All four conflicts were
counter/registry collisions in the shared headline files — resolved by taking
dev's side, then re-deriving from the tree so both plugins are counted:
skills 363 -> 364 · tools 663 -> 666 · refs 746 -> 749
agents 103 -> 104 · commands 118 -> 119 · plugins 89 -> 90
README engineering row 85 -> 86
Also fixed two merge artifacts: the README engineering row lost its human-gate
mention (dev edited the same row for book-to-skill), and the both-sides CHANGELOG
resolution left an orphaned duplicate fable-goal header at the seam — dev had
retitled the real entry "(previous PR)".
Verified after merge: derive_counters --check passes, check_plugin_json --all 90/90
OK, human-gate 6/6 on the write-a-skill checklist, all three scripts --sample green,
no conflict markers left in the tree.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Fourteenth review round, no correctness findings. Three clarity items,
all of which were real double-takes rather than nitpicks.
1. §5.4's 60s stale-break and 5s acquire timeout read as contradictory
("5 < 60, so how does stale-break ever fire?") because the doc never
said they answer different questions. Added a table: 60s is an mtime
AGE test — older than that, break immediately, no waiting; 5s is how
long a writer waits on a lock that is live and younger than 60s. The
stale path is checked first, not gated behind the wait.
2. AGENT_MEMORY_RECALL breaks the naming symmetry its two siblings keep
(they mirror SessionStart/SessionEnd exactly). Recorded why:
AGENT_MEMORY_USERPROMPTSUBMIT is the consistent name and is rejected
on ergonomics, since this is the variable a user reaches for most
often. Marked deliberate so a later round does not "fix" it.
3. atm_961f033d appears at tier L1 in DESIGN.md and tier L2 in the
schema, which reads like copy-paste. It is the atom's lifecycle: id
hashes claim+project with no tier component, so L1->L2 keeps it, and
stability is required because merge-on-re-observation keys on the id.
Writing that callout surfaced a defect in my own first draft of it: I
claimed promotion never mints a new id, but §4.1.1 step 3 does exactly
that at L2->L3, because dropping the project component changes the
hash input. Corrected before commit, and verification now PROVES both
halves by computing them (aid(claim, project) == the L1/L2 id;
aid(claim) != it) rather than asserting the prose is right.
Counters unchanged: 363 / 89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Audits petergyang/human-review and ships a conceptual derivation that fits this
repo's stdlib-only conventions.
Audit (audit/human-review-2026-08/AUDIT.md): upstream is a well-engineered ~5,200
LOC Node app — its own test suite passes 90/90, and its security model (loopback
bind, DNS-rebinding Host check, constant-time token compare, realpath traversal
guard, inert Markdown renderer, 45-min idle shutdown) is better than most
local-server tools. It still does not fit: Node 20 + an npm runtime dependency
fails the same stdlib-only test that kept the heavier skillopt package out in
v2.11.2. Seven findings, three material — F1 (HIGH) unpinned `npx -y` executes a
newly published version on every run; F2 (MED) "do not end your turn" plus
re-poll on timeout with no headless guard or retry cap; F3 (MED) only /api/* is
token-gated.
Also: despite the name it is not a humanizer. This is human approval, not human
voice — no overlap with behuman or content-humanizer.
New plugin engineering/human-gate, three stdlib scripts, no server or socket:
- review_page_builder.py — Markdown/HTML to a single-file anchored review page
with zero network requests (~11 KB, opens over file://). Escapes before
applying inline markup, scheme-allowlists hrefs, drops script/style on HTML
input.
- feedback_parser.py — sidecar to batch.v1 JSON. BLOCKER/MAJOR/MINOR/NIT
(matching md-review) plus EDIT/NOTE/APPROVE. Verifies quotes against the real
file; strips HTML comments so a documented example cannot parse as a real
sign-off.
- human_gate.py — open/status/collect/close/reset with atomic writes and
0700/0600 state. Rules G1-G6 refuse to close on: no collected round, an open
BLOCKER/MAJOR, an unnamed reviewer, a sidecar changed after collection, an
exhausted round cap (exit 5 = escalate), or an undocumented waiver.
Loop discipline deliberately inverts upstream: no blocking poll, a headless
guard, a round cap that escalates. The sidecar is hand-writable Markdown, so the
loop closes over SSH and in CI. The optional bridge to upstream is opt-in and
always version-pinned.
Adds 3 references (7-8 sources each), a batch.v1 schema, a worked example,
cs-human-gate agent, /cs:human-gate command. SKILL.md passes the write-a-skill
6-item checklist 6/6; description validator PASS.
Counters: skills 362->363, tools 644->647, refs 741->744, agents 102->103,
commands 116->117, plugins 88->89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01233Eggb2cjSYf96X6C3pCm
Thirteenth review round. Three structural defects, all introduced by my
own earlier edits rather than present in the original draft.
1. §4.1.2 was placed physically BEFORE §4.1.1 — and §4.1.2's argument
cites the L2→L3 merge as the reason a state is unreachable, so a
reader met the citation before the section defining it. Reordered.
2. §4.1.1 and §4.1.2 were ### (h3), the same level as §4.1 itself,
while §3.1.1/§3.1.2 and §4.2.1 correctly use ####. Both bumped to
####. Both defects date to round 11, where the two sections were
spliced in without reconciling against the document's own structure.
3. "Hashing" was orphaned on its own line, residue from the round-7 edit
that replaced the text around it. Reattached.
The reviewer's framing is the right one: these undercut a document whose
pitch is self-checking rigor. So the fix is not just the three edits —
verification now includes a structural linter that derives expectations
from the numbering itself rather than checking known strings: heading
LEVEL must equal numbering depth + 1, and heading NUMBERS must ascend in
document order. Both would have failed on this state before the fix, and
they cover every section, not the three that happened to break.
Also the sixth round where a check failed on my own assertion rather than
the artifact (backticks in `scope`). Prose checks now normalize markdown
emphasis and code markers alongside case and whitespace, which is the
last of the three ways these assertions have been brittle.
Counters unchanged: 363 / 89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Fourth review on PR #947 reported two functional bugs, explicitly noting it had
not run anything. Both reproduced, and the first is severe.
1. F1 -- the blocking gate -- could be bypassed by a typo.
_check_forgetting_rule() failed only when `rule` was literally
"none"/""/"never", and otherwise inferred PASS from what the rule was *not*.
So anything unrecognized fell through to the PASS branch with an empty
mechanism list. Reproduced:
{"rule": "asdf"} -> F1=PASS "Forgetting is designed: ."
{"rule": "ttl"} (no ttl_days)-> F1=PASS "Forgetting is designed: ."
A misspelling silently passed the one check this entire skill is built
around, and the nonsensical detail string was the only hint.
The check is now allowlist-based: PASS is unreachable unless a concrete
mechanism is actually found (ttl_days > 0, max_records/max_bytes > 0, or a
decay setting). Failure messages now distinguish an unrecognized rule from a
declared-but-unconfigured one, so a typo is never mistaken for a deliberate
decision not to forget. Booleans are rejected where a number is expected,
and ttl_days=0 counts as absent.
Verified across 10 cases: all six bypass variants now FAIL at exit 4, all
four legitimate mechanisms still PASS, and the empty-mechanism string can no
longer be emitted.
2. --print-sample-spec was unreachable on all three scripts that offer it.
The flag sat outside a mutually-exclusive group declared required=True, and
argparse enforces that during parse_args() -- before any of our code runs.
So the flag alone exited 2 with a usage error, which broke the first line of
the workflow SKILL.md documents verbatim:
python scripts/memory_cost_profiler.py --print-sample-spec > workload.json
The group is now required=False with explicit post-parse validation, so
no-args still errors helpfully and names all valid entry points. Verified the
full round-trip on all three: --print-sample-spec > f.json, then feed f.json
back in.
This slipped through because the PR's own checklist covered --help, --sample
and --output json, but never ran --print-sample-spec standalone.
Also removed the identity dict in render() flagged as a nit.
Verified: 4/4 scripts help/sample/json; error paths 3/4/4; all six blocking
gates; checklist 6/6 PASS; security auditor PASS (0 critical, 0 high, 0 info).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jt1sqt5kQmopyfXu2Hhjnv
Twelfth review round.
1. Section 1 and section 9.1 both claimed root CLAUDE.md is "~40 KB".
It is 85,875 bytes — 84 KB, more than 2x the stated figure. The
argument survives (the real number strengthens the bloat case), but
the error is the embarrassing kind for this doc specifically: section
2 opens "verified by reading the code, not the docs", and this was
eyeballed. Both sites now carry the measured figure with the method
(wc -c) so it is checkable rather than asserted, and verification
recomputes it from the file instead of matching a string.
2. The promotion table read "observations >= 3 across >= 3 distinct
sessions", presenting two conditions where there is one. The schema's
own observations description says the field is informational and the
gate counts sessions; since observations is always >= len(sessions),
the extra clause adds nothing except the risk that an implementer
gates on the wrong field — the exact confusion the sessions-as-a-set
rule exists to prevent. Row now names sessions as the gate and says
explicitly that observations is not it.
Also fifth round running where a verification check failed on my own
assertion rather than the artifact (this time case: "Informational" vs
"informational"). Prose checks now normalize case and whitespace once at
the top rather than being patched per-match.
Counters verified on the merged base: 363 / 89, unchanged by this PR.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Third review on PR #947 caught a counter this branch's sync missed.
README.md line 30 read "**Reference docs** — 746 templates, checklists, and
domain-specific knowledge files". The sync pass replaced the string "746
reference guides", which is the phrasing used in CLAUDE.md and
marketplace.json, so this differently-worded line was never matched.
Root cause worth recording: `derive_counters.py --check` does not cover this
line. Verified by setting it to a deliberately wrong 111 — the gate still
passes. So this class of drift is invisible to CI, which is why a reviewer
found it and the automated gate did not. Not fixing the checker here: it is
shared infra, and widening its coverage could surface pre-existing drift in
unrelated rows and turn CI red for reasons that have nothing to do with this
PR. Flagged on the PR for the maintainer instead.
Also swept README for any other stale counter adjacent to counter vocabulary
(skills/tools/references/agents/commands/plugins, old value vs new): no other
occurrences. Badges and both prose lines now read 364 / 667 / 750 / 104 / 120 / 90.
Left untouched: CLAUDE.md line 183, which records book-to-skill's own
"refs 741 -> 746" delta. That is historically correct for that release note
and must not be re-pointed at the current total.
All six blocking gates pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jt1sqt5kQmopyfXu2Hhjnv
Second automated review on PR #947 found a real bug, verified by execution:
`pick()` looks up `tuple(sorted([winner, runner_up]))`, but two of the four
TIE_BREAKERS keys were authored in the other order, so they could never match:
('flat_rag', 'structured_rag') sorted -> matches
('structured_rag', 'agentic') sorted -> ('agentic', ...) DEAD
('long_context', 'flat_rag') sorted -> ('flat_rag', ...) DEAD
('long_context', 'structured_rag') sorted -> matches
Only 2 of 4 authored questions were reachable. The two dead ones are the
plausible near-ties (structured_rag vs agentic on a high-recall/high-mutability
workload; long_context vs flat_rag under build-budget pressure), so the tool's
most distinctive behaviour — an authored, specific tie-breaking question —
silently degraded to the generic fallback with no error.
Keys are now normalized through sorted() at import, with a collision check that
raises if two entries describe the same pair. Verified by injecting a duplicate
in the reverse order: the guard fires. This repo has no test suite, so the check
runs at import rather than living in a test.
Confirmed by execution, not inspection: all 4 keys reachable, and a constraint
set that ties structured_rag against agentic now returns the authored question
("Does your memory need to correct itself without a human in the loop?")
instead of the fallback.
Also from the review: the SKILL.md workflow block labelled steps 1-4 while the
prose referenced a step 5, so a reader skimming only the code block would not
know it existed. Added a `# 5 - No command.` line and compensated elsewhere to
stay within the checklist's 100-line limit (still 6/6 PASS).
Verified: 4/4 scripts --help/--sample/--output json; all six blocking gates.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jt1sqt5kQmopyfXu2Hhjnv
Eleventh review round. Two genuine design gaps in the state machine, both
of which would have surfaced mid-implementation.
1. A tier=L1/scope=global atom was schema-legal but could never promote.
The scope field's own description said scope "is assigned at
extraction", so an extractor could mint one; but L1->L2 requires "same
project" and a global atom has no project field, while L2->L3 is a
merge over two or more L2 atoms. Such an atom would sit at L1 until it
expired at 90 days, silently, since nothing flagged it.
Closed by making scope a function of tier rather than a free choice:
L1 and L2 are project, global exists ONLY as the product of the L2->L3
merge. Enforced with a tier/scope conditional so a wrong extractor
fails validation instead of quietly producing orphans. This is also
correct on the merits — whether a claim is global is not knowable at
extraction; it becomes global by holding in a second project, which is
exactly what the merge represents. New section 4.1.2.
2. Section 4.1 gated L1->L2 on "no contradiction open", but 4.2 defined
contradiction handling only at L2/L3 — so the gate referenced a state
nothing produced. New section 4.2.1 defines detection at L1 with two
deterministic rules (explicit negation; same-subject different-value),
run at merge time within a project. States plainly what they do NOT
catch (semantic contradiction needs meaning, not string shape) and why
that is acceptable: the miss is bounded, since 4.2's L2/L3 handling
catches it one tier later and the human gate at adopt is what actually
holds. A narrow detector claiming completeness would be worse than one
that documents its edge.
Also: merged dev (branch was cut before #941) so the "adds nothing
countable" claim is verified against the real base — derive_counters
--check passes at 363 skills / 89 plugins, up from the 362/88 the branch
was cut at. Corrected the stale productivity/handoff evidence in section
10 (7 scripts + 2 hooks = 9 counted tools, not 5 + 2); the underlying
claim that hooks/*.py count was independently verified and is unchanged.
Section 1's skill count is now "360+" so it stops drifting with dev.
Counters verified on the merged base: 363 / 89, unchanged by this PR.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Automated review on PR #947 flagged find_duplicates() undercounting. Verified
and fixed, though the diagnosis was incomplete in a way that changes the fix.
`seen.add(j)` only ever recorded the second member of a matching pair, so for
a fully-connected cluster of k mutually-duplicate records len(seen) == k-1.
That number is not meaningless -- it is exactly the count of redundant copies
you could delete. The actual defect is that it was reported under the name
`records_with_a_duplicate`, and rendered as "N records have a near-duplicate",
which describes participants (k), not redundant copies (k-1). Two valid
metrics, one reported under the other's name.
So rather than just adding `seen.add(i)`, both are now computed and reported:
participants -- every record with >= 1 near-duplicate; drives duplicate_share
and the DUPLICATE_BLOATED threshold, matching the wording
redundant -- participants minus one survivor per connected cluster
Clusters are resolved with union-find, not by counting pair endpoints: a
3-record cluster emits pairs (i,j), (i,k), (j,k), so endpoint arithmetic gets
the redundant count wrong. Verified k=2/3/4 give participants=k, redundant=k-1,
and that a 3-cluster plus an unrelated record still gives 3/2.
Sample output moves from "1 record (17%)" to "2 records (33%), of which 1
redundant" -- the reviewer's point that this could tip a real store under the
15% threshold was correct.
Also from the same review:
- Dropped the stale `-> list[dict]` hint (the function returns a tuple); the
return shape is documented in the docstring instead.
- Capped the O(n^2) scan at MAX_DUPLICATE_SCAN=2000 eligible records and made
the tool print how many were skipped, per this repo's no-silent-caps rule --
a quiet cap reads as "no duplicates found".
Verified: 4/4 scripts --help/--sample/--output json; all six blocking gates;
skill checklist 6/6 PASS; real-directory run unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jt1sqt5kQmopyfXu2Hhjnv
Keeps the branch current with dev (363 skills / 89 plugins) so the
'adds nothing countable' claim is verified against the real base rather
than the stale one the branch was cut from.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
dev moved 8 commits ahead (engineering/book-to-skill), and both branches
touched the same three headline-counter files, so marketplace.json, CLAUDE.md
and README.md all conflicted.
Resolved by taking dev's version of each file wholesale, then re-applying this
branch's additions on top — rather than hand-merging the counter arithmetic,
which is exactly the kind of edit that silently drifts:
- marketplace.json: re-inserted the memory-engineering entry next to
agent-harness (dev's 89 plugins -> 90)
- CLAUDE.md: re-applied the memory-engineering release note above the
fable-goal note
- README.md: engineering-POWERFUL row 85 -> 86 plus the plugin mention
Then re-derived every headline number from the merged tree instead of
assuming my original deltas still held — they did not, since book-to-skill
had already consumed them. True values are now skills 364, tools 667, refs
750, agents 104, commands 120, plugins 90. The release note's own delta line
was rewritten to sit on top of book-to-skill rather than claiming the stale
362->363 range.
Verified on the merged tree: all six blocking gates pass (check_plugin_json,
check_paths, check_dual_publish, smoke_scripts, smoke_json_output,
derive_counters --check), compileall clean, skill checklist still 6/6 PASS,
no conflict markers remain.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jt1sqt5kQmopyfXu2Hhjnv
Tenth review round, no blocking findings. All three folded in.
1. The examples array held only L2 and L3 atoms, both using the stripped
back-pointer form — so the L1 branch of the tier conditional, the one
the $comment calls out as having PII consequences, was never exercised
by the schema's own self-test. DESIGN.md showed an L1 atom in prose,
but the schema read standalone never demonstrated the prefixed form.
My own verification had the same hole: it hand-evaluated L1 with a
synthetic case rather than a committed fixture, so nothing in the
repo pinned it.
Added a third example at tier L1 carrying the full <cwd-slug> path.
Chose a claim with kind='failure' so it also demonstrates a third
kind (the others were constraint and preference) rather than only
patching the coverage gap. Verification now asserts the examples span
all three tiers and both scope branches, and still rejects both an
unstripped L2 pointer and a prefix-less L1.
2. The L1->L2 row read "≥ 3 distinct sessions, ≥ 2 of them on distinct
days", which invites the wrong reading (2 of which sessions?). The
intent, stated precisely later in 4.1, is that the sessions SPAN two
days. Row now reads "spanning ≥ 2 distinct calendar days (UTC)",
matching the precision used elsewhere.
3. Recorded the stale-lock TOCTOU as a deliberate acceptance rather than
leaving it to be discovered. Two writers can both judge a lock stale;
the consequence is bounded by the design already there — each still
commits via os.replace, so the loser's atoms are lost, not corrupted,
and lost L1 candidates re-observe next session. A true mutex costs
portability (fcntl semantics vary across NFS and Windows) to buy
durability this tier does not need. Noted that it should not be
"fixed" without first showing the loss is observable.
Counters unchanged (362 / 88).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Ninth review round, no blocking findings. Three of four folded in; the
fourth is a maintainer call I have deliberately not made.
1. Added section 3.1.2 — the invariants JSON Schema CANNOT enforce,
because it validates one atom at a time and a valid atom does not
imply a valid store: first_seen <= last_seen (no cross-field
comparison), id uniqueness across atoms.jsonl, and per-claim session
uniqueness across records (uniqueItems is within-array only). Each is
assigned an owner in memory_extract.py / memory_promote.py. Verified
the one thing the schema DOES enforce — within-atom sessions
uniqueness, which is what the durability gate actually depends on, so
the boundary is exact rather than hand-waved. Recorded because "the
schema validates" is easy to mistake for "the store is consistent".
2. Section 9.5 now says that if the cold-start measurement forces
outcome (c) — drop UserPromptSubmit — hooks/hooks.json must shrink
with it. Deleting the hook from the design alone would leave the
contract file asserting a hook the design no longer wants; a contract
must not outlive the decision that justified it.
3. Recorded the $id rationale as a $comment in the schema. Three
separate review rounds have now proposed conflicting changes to this
non-functional field (blob-vs-raw, dev-vs-main), so the reasoning is
written down to stop the churn: raw.githubusercontent because
github.com/blob/ serves HTML and breaks $ref dereferencing, and main
because $id identifies the published artifact — a dead link until the
dev -> main promotion lands is accepted deliberately over a URL that
goes stale the moment it does.
Not acted on: whether a DESIGN.md-only folder under a domain is a
pattern to keep or a one-off. Two reviewers have now raised it; it is a
repo-convention decision for the maintainer, not one to settle by
pushing a commit.
Counters unchanged (362 / 88).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Eighth review round. Three findings, one of which closes a gap round 7
opened.
1. The tier-dependent back-pointer rule was prose-only. Round 7 argued
hard that stripping the ~/.claude/projects/<cwd-slug>/ prefix at
promotion is "not cosmetic" — an unstripped L2/L3 back-pointer commits
a contributor's OS username to a shared CLAUDE.md. But the schema left
source/first_source as bare {"type":"string"} with a description,
while the two OTHER promotion rules (project<->scope,
promoted_from_projects<->tier) were both machine-enforced
conditionals. The one rule with PII consequences was the one relying
on prose — which contradicts round 7's own framing that a contract
must be self-checking rather than self-asserting.
Added a tier-keyed conditional: at L1, source and first_source must
match ^~/\.claude/projects/[^/]+/...\.jsonl#L[0-9]+$; at L2/L3 they
must match ^[A-Za-z0-9._-]+\.jsonl#L[0-9]+$, which no path-prefixed
value can satisfy. memory_promote.py now fails loudly on a bad
promotion instead of leaking silently.
Verification hand-evaluates the conditional (jsonschema is not
available — stdlib-only repo) against three cases beyond the fixtures:
an unstripped L2 pointer is REJECTED, the stripped form is ACCEPTED,
and a prefix-less L1 is REJECTED so the local form stays
direct-openable. Asserting the rule fires is the point; asserting only
that the fixtures pass would have missed it.
2. Section 4.3 cited section 5.3 for the L3 cap. 5.3 is SessionEnd
capture; the cap ("Budget: 2 KB L3 + 4 KB L2", truncate by last_seen)
is in 5.1, SessionStart read. Corrected, and the check now confirms
the cited section actually contains the budget rather than trusting
the number.
3. The section 3.1.1 insertion split section 3.1's field bullet list,
leaving the `confidence` bullet abutting 3.1.1's closing paragraph
where it read as a stray continuation. Moved 3.1.1 to after the
complete list, so the list stays whole and the back-pointer deep-dive
is its own uninterrupted unit.
Counters unchanged (362 / 88).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
CI gate G1 (scripts/check_paths.py) failed with 8 unresolvable references.
Both command files referenced `scripts/<tool>.py` and `assets/<file>` as if
they were relative to the command file, but commands/ sits at the plugin root
while the scripts live under skills/memory-engineering/. SKILL.md was correct
already — it sits inside the skill directory, so its bare `scripts/...` paths
resolve — which is why this only showed up in the two command files.
Rewritten to the plugin-root-relative form
(`skills/memory-engineering/scripts/...`), matching how agent-harness writes
its command paths.
check_paths.py --all now reports 0 findings across 586 files. Also re-ran the
other five blocking gates locally: check_plugin_json, check_dual_publish,
smoke_scripts, smoke_json_output, derive_counters --check — all pass, plus
compileall on the plugin.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jt1sqt5kQmopyfXu2Hhjnv
Two follow-ups after running the repo's own CI auditors locally:
- Removed `from __future__ import annotations` from all 4 scripts. The repo's
skill_validator.py counts it as an external import (script_tester.py
disagrees and reports the same files as stdlib-only, but the stricter of the
two is what CI surfaces). PEP 585 generics are native on the 3.11 CI runner,
and the single PEP 604 union annotation was the only thing needing the
import — it is now unannotated. Takes this skill from 7 validator errors to
3, matching productivity/weekly-review and better than productivity/fable-goal
(4). The 3 remaining are the legacy v2.0.0 schema checks (min-100-lines, and
Tier/Category/Features/Usage frontmatter+sections) that directly contradict
the Matt Pocock checklist CLAUDE.md declares binding for post-v2.6.0 skills —
every recently-merged skill carries the same ones.
- SKILL.md: steps 4 and 5 used a bold-number style left over from the
compression pass while steps 1-3 had become prose. Reworded to match; still
exactly 100 lines and 6/6 PASS.
Verified after the change: 4/4 scripts pass --help / --sample / --output json;
script_tester 5/5 PASS; security auditor PASS (0 critical, 0 high, 0 info).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jt1sqt5kQmopyfXu2Hhjnv
Seventh review round. Two real defects, both in the contract file.
1. The back-pointer format contradicted the de-identification rule it is
promoted through. source/first_source are required at EVERY tier, and
the format is ~/.claude/projects/<cwd-slug>/<session>.jsonl#L<line>
where <cwd-slug> is Claude Code's slugification of the ABSOLUTE working
directory — so on a real machine it reads -home-alice-work-... and
embeds the OS username. A strictly-compliant implementation of rule 4
("cite, don't invent") would therefore write a contributor's username
into a shared, git-tracked CLAUDE.md on the first promotion, violating
section 6's de-identification requirement. Rule 4 would have won,
being the more mechanical of the two.
Resolution: the format is now explicitly tier-dependent. L1
(gitignored) keeps the full local path; promotion into L2/L3 strips
the prefix to <session>.jsonl#L<line>. Nothing is lost — session ids
are globally unique, so the prefix is recoverable at read time by
globbing ~/.claude/projects/*/<session>.jsonl. Required at BOTH
promotion boundaries, since L1->L2 is the first crossing into
committed territory.
Round 3's fixture fix masked this: the placeholder read -home-user-,
which looks de-identified only because that machine's username is
literally "user". The L2/L3 fixtures now carry the portable form, so
the examples demonstrate the rule instead of hiding it.
2. Neither worked example id reproduced. atm_7f3a9c21 / atm_b41c8de0
were invented in round 1, before sha256 was pinned in round 3, and
nothing since re-derived them — in the file that calls itself the
contract. Recomputed to atm_961f033d / atm_12cc1dc2, and the exact
normalize() is now pinned in the doc (collapse whitespace, casefold,
strip trailing punctuation — order matters). Verification executes
that published function and re-derives every id, so the contract is
self-checking rather than self-asserting.
Also: stale-lock threshold quantified at 60s (the one unquantified
number in the doc), and the section 10 manifest form recorded as a
follow-up for root CLAUDE.md rather than silently changed here.
Counters unchanged (362 / 88).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
New engineering/memory-engineering/ plugin. The repo had no skill for
designing, pricing, or auditing an agent memory system: llm-wiki maintains
one vault, skillopt-sleep runs a consolidation loop, agent-harness bounds a
task loop. This bounds a store.
Four stdlib scripts, one per lens:
- memory_cost_profiler.py — construction vs query split, cost per correct
answer, amortization ratio, construction co-location warning
- memory_architecture_picker.py — scores the four paradigm families,
disqualifies on hard constraints, names the cost the choice makes you pay,
and refuses to pick when the top two tie (exit 2 + tie-breaking question)
- memory_density_auditor.py — classifies records FACT/SKILL/LOG/PROSE, finds
near-duplicates, flags staleness and time-relative wording, scores density;
runs on a real --dir or --jsonl
- forgetting_policy_linter.py — the gate. 8 checks; F1 (explicit forgetting
rule) and F4 (contradictions surfaced, never auto-merged) block at exit 4
Evidence discipline: the four-lens framing is synthesized from @N01ennn's
"How to be a Memory Engineer", but every quantitative claim is re-cited to
the primary source, and two of the article's paraphrases are corrected in the
references rather than propagated — the 47x energy figure is the spread across
ten evaluated systems (not an accuracy-matched pair), and the 97%
first-pass-error figure is Rakuten's named vendor testimonial (not a
controlled study). Per-claim confidence levels throughout.
Three classifier defects found and fixed during the build, each of which would
have produced garbage on a real repo: markdown headings inside fenced code
blocks were splitting records (258 phantom records -> 107 on a real directory);
short fragments matched trivially at 1.00 Jaccard (41 false-positive
duplicates); and signal-less prose was labeled LOG, firing LOG_HEAVY at 74% on
a documentation folder — now its own PROSE class.
Also ships 4 references (7 sources each), a seven-question forcing worksheet,
a combined example spec consumed by all three spec-taking scripts, an F1–F8
policy template, cs-memory-engineer, /cs:memory-engineering and
/cs:forgetting-audit.
Verified: all 4 scripts pass --help / --sample / --output json, stdlib-only,
correct exit codes on error paths; SKILL.md 6/6 PASS on the write-a-skill
checklist; check_plugin_json.py --all and derive_counters.py --check both pass.
Counters: skills 362->363, tools 644->648, refs 741->745, agents 102->103,
commands 116->118, plugins 88->89.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jt1sqt5kQmopyfXu2Hhjnv
Sixth review round, one substantive nit.
Section 3.1 opened "Every field is mandatory", which is false: 12 of the
18 fields are unconditionally required, and the example directly beneath
it shows two that are not (project is conditional on scope, redacted is
set by a later stage). Left as-is this would push whoever writes
memory_extract.py to over-constrain the extractor — emitting fields at
extraction time that belong to redaction, contradiction handling, or
promotion, and emitting `project` on global-scoped atoms where the schema
forbids it outright.
Replaced with the true count plus an explicit accounting of why each of
the other six is conditional or deferred, and a note that the example
carries project/redacted because of what that particular atom is, not
because either is universal. Verification now cross-checks the prose
against the schema field by field rather than pattern-matching a phrase,
so the two cannot drift apart again.
Counters unchanged (362 / 88).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Fifth review round. Three findings, all verified empirically rather than
reasoned about — two were wrong in ways that would have broken the
implementation PR's CI.
1. Section 10 claimed "tools +3", counting only scripts/*.py.
derive_counters.py counts EVERY .py outside repo-root scripts/, so the
three hooks/*.py count too. Verified by adding one file under hooks/
in this tree: python_tools moved 644 -> 645. productivity/handoff
confirms it independently — 5 scripts/ + 2 hooks/ files, documented
repo-wide as "7 stdlib-only Python tools". Corrected to +6, with the
evidence recorded so the implementation PR does not trip
derive_counters.py --check.
2. The planned layout put SKILL.md flat at the plugin root. Every
comparable agents+commands plugin nests it under skills/<name>/ —
skillopt-sleep, write-a-skill, agent-harness, handoff and llm-wiki are
5 for 5, and two of them are already cited in this doc for their hook
and staging patterns, so matching their directory shape too is the
consistent call. Tree corrected, and the plugin.json skills form
pinned to ["./skills/agent-memory"] to match.
3. Section 6 listed .memory/ as gitignored while marking adopted.log
inside it as committed, with no pattern shown. Not a contradiction but
it needed stating, because the obvious spelling does not work: a
directory-level `.memory/` ignore is never descended into, so a `!`
negation cannot re-include anything under it. Verified both spellings
in a scratch repo — `.memory/*` + negation tracks adopted.log,
`.memory/` + negation tracks nothing. Documented the working pattern
and why adopted.log is public by design (it records what already
cleared the L2/L3 bar into CLAUDE.md).
Also scoped the chmod non-negotiable to runtime-created files: git
tracks no POSIX mode beyond the executable bit, so a fresh checkout
materializes adopted.log at the cloner's umask and no in-repo
declaration changes that. Anything whose confidentiality depends on
mode bits must be gitignored, which is now stated as the reason
atoms.jsonl and staged/ are.
Counters unchanged (362 / 88) — still no SKILL.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Fourth review round. Five findings, all valid.
1. Section 4.1.1's L2->L3 merge assigned first_source but never source,
though the schema requires both — so memory_promote.py written against
this contract would have emitted atoms the schema rejects. Same class
as the promoted_from_projects gap from round 3. Merge now sets source
from the contributor with the latest last_seen, which also preserves
the field contract (first_source = oldest, source = newest) across the
merge boundary rather than only within one atom's history.
2. Concurrency was entirely unaddressed. SessionEnd is async and does a
read-modify-write on .memory/atoms.jsonl; two sessions on one repo
(several terminals, or worktrees) interleave and lose writes, and the
recall read can catch a partially-rewritten file. New section 5.4
reuses the repo's existing pattern rather than inventing one, citing
both precedents by line: agent-harness loop_controller.py:54-62 and
skillopt-sleep state.py:77. Writers take an exclusive lock then temp +
os.replace; readers take NO lock, because blocking UserPromptSubmit on
a lock held by an async SessionEnd would blow the 100ms budget for a
hook whose failure mode is meant to be "return nothing" — atomic
replacement is what makes lock-free reads safe. Writers that cannot
acquire within 5s drop their atoms and log it: losing one session's
candidates is recoverable, a wedged SessionEnd is not.
3. The aggregate collision claim was wrong. "Likelier than not across ~30
users" does not follow from a 0.29% per-file rate — 30 files give
8.4%, and even odds needs ~239. Replaced with the correct figures. The
conclusion (widen the id if the cap rises) was unaffected, but the
stated justification was false.
4. Fixtures used the real session id that generated this PR, which
section 6.5 — added last round precisely to stop fixture-realism
violations — forbids. All seven ids are now uniformly synthetic
(01SESSION<X>...), so none can be mistaken for real and the pattern is
assertable.
5. confidence:"verified" is the sharpest form of the extraction risk
section 9.2 only gestured at generally: hardest to assign lexically
(it requires recognising a check actually ran) yet lowest promotion
bar (1 observation, exempt from the distinct-days clause). Leaning
recorded: a rule-based extractor must never assign it — reserve it for
atoms minted by a tool that ran the check and can name it.
Counters unchanged (362 / 88).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Third review round. Both findings valid; the first is sharper than the
report framed it.
1. The spec violated its OWN admission policy inside its own examples.
Section 6 requires committed content be "interpreted, de-identified,
non-confidential" — and the fixtures named a repo that appears nowhere
in this public tree and embedded its local transcript path. Two costs:
the section 6 analogy was unverifiable to any reader of this repo, and
a project name that was not ours to publish shipped in a committed
file. Fixtures are committed data; the policy binds them.
Fixed by genericizing the example project and restating the admission
analogy against llm-wiki, which is in this tree and checkable. Added
rule 6.5 making the policy explicitly binding on the spec's own
examples, since the underlying mistake was treating fixture data as
exempt — that is the part that would otherwise recur.
2. The id hash was unspecified. Section 4.1 said hash() without naming a
function, which for Python is actively dangerous: the builtin hash()
is salted per process for str, so ids would differ every run and
merging — the mechanism the whole durability gate rests on — would
silently never fire. Now pinned to stdlib hashlib.sha256 truncated to
8 hex, with the salting hazard called out.
Also replaced the unstated 32-bit id-space assumption with arithmetic:
birthday collision is 0.0029% at the 500-atom cap, 0.29% at 5000, so
raising the cap requires widening the id. A collision silently merges
two unrelated claims' durability counters, which is the same failure
project-scoping was added to prevent.
Verification now asserts fixtures carry no unknown project slugs, and
recomputes the collision table rather than trusting the numbers in prose.
Counters unchanged (362 / 88).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Second review round. Two findings were real schema bugs that would have
corrupted promotion logic once memory_promote.py was written against it.
1. The schema could not represent the L2->L3 criterion it exists to gate.
Section 4.1 requires a claim hold at L2 in >= 2 distinct projects, but
`project` is a single string and the scope conditional FORBIDS it once
scope flips to "global" — so the multi-project evidence was discarded
at exactly the moment it stopped being an eligibility test and became
an audit trail. Added `promoted_from_projects`, required at L3 with a
minItems:2 conditional so the rule is enforced by the schema rather
than merely described in prose.
2. Atom identity was not project-scoped. `id` hashed claim text alone, so
two unrelated claims normalizing alike in different repos ("tests must
pass before merge") would collide and merge their `sessions` arrays
across projects — manufacturing false durability, since the L1->L2
gate requires sessions from the SAME project. Now
hash(claim + NUL + project) for project-scoped atoms.
This made a genuinely missing step visible: with project-scoped ids a
claim held in two projects is TWO atoms, so L2->L3 is a merge, not a
flag flip. That step was undocumented anywhere. New section 4.1.1
specifies it — group by the project-free hash, union sessions, sum
observations, min/max the timestamps, record contributors, retain the
contributing L2 atoms as the provenance chain.
3. `source` was a single string overwritten on every merge, so an
"anti-fabrication" field retained only the latest sighting and lost
the evidence that first justified the claim. Added `first_source`,
written once and never overwritten; both are now required.
Also fixed the $id ref dev -> main for a stable identifier. Deliberately
NOT switched to the github.com/blob/ form used by the repo's one other
schema: blob URLs serve HTML, not JSON, so that convention breaks any
tooling that resolves $id. Noted as pre-existing rather than propagated.
Verification now walks every JSON block in DESIGN.md plus the schema
examples, and asserts the L3 conditional is enforced rather than only
documented. Counters unchanged (362 / 88).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Six findings from automated review, all verified against the files before
acting. None architectural; all resolved in place.
1. Latency budget contradicted its own contract. DESIGN.md led with a
"hard 100 ms budget" while hooks.json set "timeout": 1 — the hook
timeout field is in SECONDS, so the contract permitted 10x the stated
budget. Split into two explicitly-named limits: a 100 ms internal
self-budget the script enforces against a monotonic clock, and the 1 s
hook timeout as a wedged-process backstop. States outright that
finishing under 1 s does not satisfy the spec.
2. The 100 ms budget was asserted with no mechanism to reach it. Bounded
the work: .memory/atoms.jsonl capped at 500 atoms with last_seen
eviction, single linear pass, bounded top-5 heap. Added open decision
9.5 requiring the budget be MEASURED before implementation, since
interpreter cold-start is the dominant cost and is not controllable
from inside the script — and naming "drop UserPromptSubmit entirely"
as an acceptable outcome. A recall hook that misses its budget every
prompt is worse than no recall hook.
3. The section 3.1 atom example omitted the required `tier` field, so a
reader could copy an invalid atom out of the doc that is meant to BE
the contract. Added; verification now parses every JSON block in
DESIGN.md, not only the schema's own examples, which is why this
drifted undetected.
4. Session ids were 8 chars in the schema examples and 24 in DESIGN.md.
Normalized to 24 everywhere; check asserts a single length across all
examples.
5. Schema $id was not a resolvable URL (GitHub blob path missing /blob/
<ref>/), which fails silently if tooling ever resolves it for $ref.
Now a raw.githubusercontent.com URL.
6. The "stated" fast path (2 sessions instead of 3) did not say whether
the >= 2-distinct-days clause survived, so one long working day could
have minted an L2 claim. Clause explicitly retained; "verified"
documented as the only exemption.
Counters unchanged (362 skills / 88 plugins) — still no SKILL.md.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Design spec only — no SKILL.md, no plugin.json, no Python. Repo counters
are deliberately untouched (derive_counters counts skills by SKILL.md).
Derived from an inspection of TencentCloud/TencentDB-Agent-Memory (MIT).
Borrows two design ideas — the L0->L3 memory tiering and the
ownership/visibility model — and rejects its integration mechanism. No
code vendored.
The core idea: flat CLAUDE.md has exactly one injection policy (always,
in full), which causes bloat, staleness, and false permanence. Tiering
splits memory by durability and gives each tier its own retrieval policy:
L0 transcripts never injected, L1 atoms recalled on relevance, L2
injected per-project at SessionStart, L3 always in context under a cap.
Promotion is deterministic and recurrence-based (>= 3 distinct sessions
across >= 2 days for L1->L2), not importance-based, and requires a live
L0 back-pointer — keeping it stdlib-only per the no-LLM-in-scripts rule.
Includes a grounded overlap analysis against existing skills. Notably
skillopt-sleep already implements the L0 reader (harvest.py walks
~/.claude/projects/*/*.jsonl) and the protected-marker-block write; the
delta is tiering, prompt-time recall, and a durability gate. Spec
concludes agent-memory must be a separate self-contained plugin rather
than an extension, since skillopt-sleep is a vendored copy carrying 23
re-vendor deviations, and cross-skill imports are a repo anti-pattern.
Rejects MemoryProxy (ANTHROPIC_BASE_URL interception) on four grounds:
reverse-engineered CC internals, subscription-to-metered billing change,
raw conversation persistence incompatible with the repo's compliance
posture, and zero test coverage.
Files: DESIGN.md, hooks/hooks.json (contract), assets/memory_schema.json.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EM5xmJ7AmTMg31rq68BCym
Addresses the residual TOCTOU raised in the fourth review. The reviewer called
it non-blocking; it verified as slightly worse than described, and the fix is
small, so it is closed rather than deferred.
The claim checks out: `mkdir(parents=True, exist_ok=True)` does NOT raise on a
symlink-to-directory, because its exists-branch tests `is_dir()`, which follows
symlinks. Demonstrated directly — mkdir succeeded silently on a planted link and
a subsequent write landed in the attacker's directory.
What the review did not note is why the second layer failed to catch it: a file
inside a swapped directory is an ordinary file, not a symlink, so
`_write_private`'s `is_symlink()` check could never see a directory swap. The
artifact-level guard did not back up the directory-level one at all.
Three changes:
- `resolve_workdir()` attempts `mkdir` FIRST and only inspects a path that
already existed, via `os.lstat` — which does not follow the final component.
That removes the check-then-create ordering.
- `open_workdir()` pins the directory with `O_NOFOLLOW|O_DIRECTORY`, and both
artifacts are written through that descriptor. An fd names an inode, so a
rename or symlink swap of the path afterwards cannot redirect the write.
- `_write_private()` creates with `O_CREAT|O_EXCL|O_NOFOLLOW` at mode 0600 —
no check-then-act window at all. An artifact from a previous run into the same
--workdir is unlinked first; unlink removes the link, never its target.
Verified against a live race: pin the directory, rename it away, plant a symlink
to an attacker directory, then write — data lands in the pinned inode, attacker
directory stays empty. Also verified a pre-planted `full_text.txt -> victim`
symlink leaves the victim's content intact and is replaced by a 0600 file we own,
and that re-running into the same --workdir still succeeds.
Degrades to the previous path-based checks where `dir_fd`/`O_NOFOLLOW` are
unavailable (Windows).
Recorded as deviation 25. Full regression re-run: EPUB bomb, EPUB entity,
extensionless sniff bomb, DOCX bomb, emitter symlink, rights gate and the
estimator path check all still refuse; a clean EPUB still extracts. All gates
green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zu9Gmm9S78c2t3kDLnpPX
Read the skill as a skill rather than as code, which the previous three review
rounds had not done. Four findings, all now fixed and verified.
1. The documented quick-start did not run. SKILL.md's copy-paste block referenced
$WORKDIR and $SKILLS_HOME without ever assigning them, so following it
literally produced a FileNotFoundError traceback at step 2. Both are now real
assignments, and all five steps were executed verbatim end to end as a check.
The plugin README's block had the same defect and is fixed the same way. A
quick-start that does not run is the worst kind of doc bug: it is the part a
reader trusts most.
2. A gate tool reported success for a path that was not there.
`token_budget_estimator.py --skill-dir <typo>` produced a complete,
plausible-looking budget audit — every row "missing", every cap satisfied,
exit 0 — which reads as a pass. It now refuses a missing directory, a
non-directory, and a directory with no SKILL.md (exit 2). `--full-text
<missing>` raised a bare traceback and now refuses cleanly. The other three
tools already validated their inputs; this one was the outlier.
3. Three upstream artifacts cleaned, one of them load-bearing. epub.py's
`except (KeyError, Exception)` is simply `except Exception` — it swallowed
everything including the size refusal `safe_read()` now raises, quietly
disarming deviation 17 at that call site. Narrowed so ExtractionError
propagates and only genuine parse failures fall through to the .opf glob.
utils.py emitted a dynamic {pages_label: pages} key beside a literal "pages",
colliding whenever the label was "pages"; the alias is now conditional. A
stray artifact word removed from a pdf.py comment.
4. `tool | head` no longer tracebacks. Observed once on the emitter (racy on
flush timing, 0/20 on retry) — all four CLIs now exit 141 quietly, the
standard SIGPIPE convention.
Token cost re-measured: SKILL.md 2,256 tokens resident (229 lines), references
10,216 on demand. Healthy against the ~5k practical ceiling for a resident body.
Full security regression re-run after the changes: EPUB bomb, EPUB entity,
extensionless sniff bomb, DOCX bomb, planted workdir symlink, emitter symlink
and the rights gate all still refuse; a clean EPUB still extracts.
Recorded as deviations 21-24; count synced across plugin.json, CLAUDE.md and
CHANGELOG. All gates green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zu9Gmm9S78c2t3kDLnpPX
Addresses the third automated review on PR #941.
[High] The magic-byte sniffing path bypassed the zip-bomb budget it was built
to enforce. extract_single_file() reads a `mimetype` member with a bare
zf.read() when the extension is unrecognized — the earliest attacker-controlled
point in the pipeline, running before a format is chosen and before any check in
zip_safety.py. zip_safety.py's own docstring claims "every read goes through
safe_read()"; this one did not, which makes it a documentation defect as much as
a security one. Now routed through safe_read(). Its ExtractionError sits outside
the surrounding except tuple on purpose, so a bomb reports as a bomb rather than
as a generic unsupported format.
Verified: a 200 MB / 1029x fixture with no file extension is refused at ~15 MB
peak RSS instead of being decompressed.
[Medium] --author / --author-url never reached the printed marketplace entry.
_plugin_manifest() threaded them correctly into the emitted plugin.json, but
_marketplace_entry() took no author parameter and hardcoded one name — so the
snippet whose entire purpose is preventing hand-edit mistakes contradicted the
manifest sitting next to it for anyone but the default author. Threaded through.
Verified: --author "Jane Doe" now appears in both.
[Low] Narrow TOCTOU between _assert_no_symlinks() and copytree. copytree already
runs with symlinks=True, so a link planted in that window is copied as a link
rather than dereferenced — no content leak. Now fully closed: the emitted tree
is re-walked after the copy, and the package is deleted rather than shipped if
any link appeared.
Verified with a monkeypatched guard that plants a symlink immediately after the
check passes: refused, package removed, secret content absent.
[Nit] plugin.json asserted "license": "MIT" unconditionally, with the "MIT
covers the converter, not the compiled content" caveat living only in README
prose. Added source.license_scope stating it in the manifest, so a tool reading
only the manifest sees the distinction, plus a code comment at the assignment.
Recorded as deviations 19 and 20; count synced across plugin.json, CLAUDE.md
and CHANGELOG.
All gates green: compileall, check_paths --all, check_dual_publish,
smoke_scripts (0 failed), derive_counters --check, check_plugin_json --all
(0 FAIL). All four CLIs pass --help / --sample.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zu9Gmm9S78c2t3kDLnpPX
Addresses the second automated review on PR #941. All three code findings
verified against the actual behaviour, not just patched.
[Medium] shutil.copytree dereferenced symlinks the validator never saw. The
validator checks SKILL.md, the three supporting files and chapters/*.md; the
copy then followed a link anywhere else in the tree (assets/, any subdirectory)
and baked the target's real content into a package that can go out as
--distribution shareable. _assert_no_symlinks() now walks the whole tree and
refuses, and runs BEFORE the validation branch so --skip-validation cannot
bypass it. copytree also passes symlinks=True so loosening that check later
cannot silently reintroduce dereferencing.
Verified: a symlink in assets/ pointing at a secret file is refused both with
and without --skip-validation, the secret never lands in a package, and a clean
tree still emits.
[Medium] The DOCX XXE/entity guard did not extend to EPUB's ebooklib path.
Upstream hardened DOCX only. EPUB is the same zip-of-XML shape and ebooklib —
one of the packages this skill recommends installing — parsed container.xml,
the OPF and content docs with no equivalent pre-check. The guard moved to a new
book_to_skill/zip_safety.py and now runs for both formats.
Verified: an EPUB whose OPF declares an entity is refused; a clean EPUB still
extracts and detects its chapter.
[Low] No size cap before decompressing zip members. Every archive read now goes
through safe_read(), which checks the declared uncompressed size and the
compression ratio against the central directory before decompressing, and
charges actual bytes against a per-archive budget so a lying directory cannot
get past it either.
Verified: a 200 MB / 1029x bomb is refused at ~14 MB peak RSS instead of being
materialized.
[Low] The PR body's "12 numbered items" was stale against README's list. Fixed
in the PR description; the in-repo count is synced to 18 across plugin.json,
CLAUDE.md and CHANGELOG.
Recorded as deviations 17 and 18. Counters: tools 662 -> 663 (zip_safety.py);
that module is allowlisted in smoke_exceptions.txt like its siblings.
All gates green: compileall, check_paths --all, check_dual_publish,
smoke_scripts (0 failed), derive_counters --check, check_plugin_json --all
(0 FAIL). Security auditor unchanged at 0 critical / 4 high.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zu9Gmm9S78c2t3kDLnpPX
Addresses the automated review on PR #941.
Security (the one item flagged as wanted-before-merge): upstream defaults the
extraction workdir to a fixed `<tempdir>/book_skill_work`. On a shared host that
is CWE-377/CWE-59 — any local user can pre-create the directory in a
world-writable /tmp (the sticky bit prevents deletion, not creation) and plant a
symlink named full_text.txt or metadata.json pointing at a file the victim can
write, because Path.write_text follows symlinks. Two concurrent runs also
silently clobber each other.
- Default workdir is now a fresh `tempfile.mkdtemp(prefix="book_skill_work_")`:
unpredictable name, 0700 by construction, never shared with a concurrent run.
The path is printed and carried in metadata.json's `output_text`.
- Artifacts are written 0600, and each write refuses a symlink at the target.
- An explicit --workdir / BOOK_SKILL_WORKDIR is still honoured, but is
symlink-refused, created 0700, and chmod-tightened if it already exists.
- parsers/calibre.py no longer writes its ebook-convert scratch file to the
shared directory. That also fixes a real bug the review did not name: it read
a module-level OUTPUT_DIR constant, so the scratch file ignored --workdir
entirely and escaped the directory the caller asked for.
Verified: default workdir 0700 with 0600 artifacts and a per-invocation name;
two runs get distinct directories; a 777 --workdir is tightened to 700; a
symlinked workdir is refused; and a planted `full_text.txt -> victim` symlink is
refused with the victim file left untouched.
Also from the review:
- book_skill_validator.py and token_budget_estimator.py restated the same
BUDGETS dict. Both now import SKILL_FILE_BUDGETS / CHAPTER_TOKEN_CEILING from
book_to_skill/config.py so the two gating tools cannot drift.
- Corrected the smoke_exceptions.txt rationale: the list is "modules the G8
probe trips on", not "modules that aren't CLIs". config.py, exceptions.py,
sanitize.py, parsers/__init__.py, parsers/pdf.py and parsers/text.py are
equally not CLIs and pass only because they have no argv handling.
Recorded as deviations 15 and 16; count synced in plugin.json, CLAUDE.md and
CHANGELOG. Docs updated: the workdir path is now read from the tool's output
rather than hardcoded.
All gates green: compileall, check_paths --all, check_dual_publish,
smoke_scripts (0 failed), derive_counters --check, check_plugin_json --all
(0 FAIL). Security auditor unchanged at 0 critical / 4 high (documented).
End-to-end pipeline re-run clean: extract -> verdict -> validate -> emit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_017zu9Gmm9S78c2t3kDLnpPX
