feat(cogos-harness): memory-janitor skill + Stop-hook trigger (v0.4.0) - #25
feat(cogos-harness): memory-janitor skill + Stop-hook trigger (v0.4.0)#25chazmaniandinkle wants to merge 1 commit into
Conversation
Adds a Stop hook + skill pair that keeps a project's auto-memory index (MEMORY.md) from growing unbounded, without ever asking the pilot to compact it inline mid-turn. The loop: the Stop hook blocks a turn once when the current project's MEMORY.md crosses a size threshold, then instructs the pilot to invoke the memory-janitor skill. The skill dispatches one bounded background Haiku lane against a standing directive (target file and the trigger/ target/floor numbers passed as pointers, never quoted or summarized), waits for its own harness's completion notification, then grades the lane's output deterministically with a bundled checker: size window with an overtrim floor, link-target identity including per-target occurrence counts, bullet inventory including linkless bullets, section headers, and per-entry clause retention. Defects come back as a repair-pointer list the skill re-dispatches with, up to two rounds total, and the final verdict is relayed to the operator either way. Every graded round appends a row to the project's own ledger (directive version vs. outcome); one-off fixture/test invocations tag their row and land it next to the fixture files instead, never in a real project's ledger. The knobs: COG_JANITOR_TRIGGER / COG_JANITOR_TARGET / COG_JANITOR_FLOOR set the byte-size thresholds (trigger arms the hook's block; target and floor bound the checker's grading window); COG_JANITOR_FILE and COG_JANITOR_STATE_DIR override the target file and state-directory paths for non-default layouts or testing. Both the hook and the checker validate floor < target <= trigger before acting on it -- the hook fails open with a stderr warning and skips the block rather than arming on a broken config, and the checker refuses to grade rather than guess. The hook's own bookkeeping (baseline snapshot, pending marker, ledger row) is best-effort throughout: any failure degrades to a warning appended to the block reason or a stderr note, never costing the block itself, the graded verdict, or the exit code. If the state directory is unwritable, the pending marker falls back to a system-temp-dir location so cooldown dedupe still works. Also bumps the cogos-harness plugin to 0.4.0, updates its manifest descriptions and README to document the new hook and its output shapes, and extends the membrane canary with direct coverage of the new hook against both a healthy and a forced-over-threshold fixture.
chazmaniandinkle
left a comment
chazmaniandinkle
left a comment
There was a problem hiding this comment.
Review seat verdict: CHANGES-REQUESTED (independent review seat; a formal GitHub "Approve" is not possible from this account, so this comment is the review record.)
Reviewed as a skeptical peer against a clone-fresh worktree at origin/feat/memory-janitor-skill (8c06926). The architecture is sound and the hook half is genuinely well built: fail-open on bad config, best-effort bookkeeping that never costs the block, and a real temp-dir fallback for the cooldown marker. My findings are almost entirely in check.py, and they cluster in one place: the checker does not actually enforce the directive it claims to grade. Three of the five checks behave differently than the README, CHANGELOG and the checker's own comments say they do, and all three are reachable with four-line fixtures.
Findings
1. BLOCKING — skills/memory-janitor/check.py:114-126: entry_key keys a linkless bullet on the exact text the directive orders the lane to trim, producing a false "bullet deleted" defect for compliant work.
For a linkless bullet the key is body[:colon], or body[:30] when there is no colon before that cut. directive.md:16 mandates the opposite behavior: "This applies to linkless bullets too ... shorten its tail, never remove it." Shortening a tail that begins inside the first 30 characters changes the key, and check.py:279 then reports the bullet as deleted.
baseline: - a linkless bullet with a long trailing narrative tail that can be trimmed
current: - a linkless bullet, tail trimmed
=> DEFECTS: entries: 1 bullet(s) deleted (keyed "a linkless bullet with a long" (linkless bullet deleted))
Control, same edit on a bullet whose colon lands before char 30, passes:
baseline: - Research/theory: a long trailing narrative tail that can be trimmed freely
current: - Research/theory: trimmed
=> PASS
This is not hypothetical for the corpus this was built against. Of the three linkless bullets in a live MEMORY.md I checked, one is in the fragile class: - **Voice/Mod3 build/eval/link-feed:** ... has its first colon at index 34, past the 30-character cut, so it is keyed on trimmable text.
check.py:294-295 asserts the invariant the code does not provide: "Linkless bullets are exempt ... their tails remain freely trimmable." They are exempt from clause-retention but not from identity keying, so the comment is true of one check and false of the other.
Consequence is worse than a spurious line of output. Round 2's repair pointer says "restore each as a line", which instructs the lane to undo a trim it was correctly told to make, and restoring the long tail can then re-trip the size check. That burns the entire two-round budget on a checker bug. It also corrupts the ledger, which the PR description sells as "training data for directive evolution": the directive gets charged with a defect the checker invented.
Two fixes consistent with the existing design: either compare linkless bullets by count rather than identity (matching the exemption the clause-retention scan already grants them), or key them on a prefix that survives tail trimming.
2. NON-BLOCKING — check.py:279: fabricated bullets are never flagged. lost_counter = base_counts - cur_counts uses Counter.__sub__, which drops non-positive counts, so additions are invisible. The links check has an explicit "invented" branch at check.py:270; the entries check has no counterpart.
baseline: - [k](k.md) - tail
current: - [k](k.md) - tail
- a fabricated bullet the lane invented out of nothing
=> PASS
For a grader whose job is to keep a small model honest while it rewrites a memory index, silently accepting invented content is a real gap. It is bounded only by the size window.
3. NON-BLOCKING — check.py:288-291 vs directive.md:17: entry order is unenforced. The directive's hard constraint is "Keep all section headers, emoji markers, and entry order exactly." Headers get an exact ordered list compare; entries get order-insensitive Counter comparison. A full reorder passes:
baseline: - [a](a.md) - one / - [b](b.md) - two / - [c](c.md) - three
current: - [c](c.md) - three / - [a](a.md) - one / - [b](b.md) - two
=> PASS
So one of the directive's four hard constraints ships with no grader, while the README and CHANGELOG both present the five checks as the deterministic grade of the directive.
4. NON-BLOCKING — hooks/memory-janitor.py:98-100: the thresholds are absolute byte counts tuned to one corpus, and the PR's Portability section does not cover threshold portability. Path derivation is handled thoroughly; the numbers are not. 18800/16500/15500 are meaningful only for an index already near that size. Consider a fresh installer whose index is larger. I built a 400-entry index already at its irreducible minimum, every tail removed, every bullet and link intact, and there is no legal edit left:
19219 B, no tails remaining
=> DEFECTS: size: 19219 B still over target 16500 — trim further (longest tails first)
That pointer cannot be satisfied without violating directive.md:16. The loop then goes DEFECTS, DEFECTS, stop, and the block re-arms after the 6-hour cooldown, indefinitely, because there is no terminal "unachievable" verdict distinct from "the lane did badly". For a marketplace-published plugin this is the case a stranger hits first. Worth either deriving the target relatively (a fraction of the size observed at first trigger) or teaching the checker to recognize an irreducible floor and say so.
5. NON-BLOCKING — skills/memory-janitor/SKILL.md:77: no rollback after a failed loop. Step 7 is "relay the final checker line to the operator and stop". After two DEFECTS rounds, MEMORY.md is left in the lane's last-written state, never graded as good, and the only pristine copy is janitor/baseline, which the skill never names as a restore path. For a tool that rewrites the memory index in place, the terminal step should offer restoring from the baseline it already captured.
6. NIT — check.py:203-211: an unconditional blocking stdin read whose result is discarded. When both --baseline and --current are passed without --project-dir, all three return values of _default_paths() at line 207 are overridden or unused, yet the call still runs _resolve_cwd() into _cwd_from_stdin() and a sys.stdin.read() guarded only by isatty(). A non-tty stdin that never delivers EOF blocks there; the hook is covered by hooks.json's timeout: 5, check.py has no equivalent guard. I could not reproduce an actual hang, it completed promptly in every configuration I instrumented, so I am filing this from code reading rather than a repro. Cheap fix: skip the call when both paths are explicit.
7. NIT — declared residuals 2 and 4 are both one-line fixes that belong in this PR. README.md:94 says fixture rows land "next to --current"; verified that a lone --baseline anchors on the baseline instead. canary/run_canary.py:470 still says SessionEnd "runs LAST in this cycle" while the new step 7 follows it. Shipping sentences you already know are wrong costs more than fixing them here.
On the five declared residuals
1, 3 and 5 are honestly LOW and I would ship them. Residual 1 in particular is a bundled read-only file in the plugin cache, so the exposure is negligible. Residual 5 is defensible: I confirmed the marker-based dedupe works, and reading stop_hook_active is belt to that suspenders rather than the load-bearing guard. 2 and 4 are honest about impact but are in-PR one-liners, per finding 7.
The list's problem is completeness, not calibration. Findings 1 through 3 are undeclared, and finding 1 is more consequential than any of the five. Three adversarial rounds converged on link preservation, which is well covered; nothing appears to have tested trimming a linkless bullet, adding a bullet, or reordering bullets, which are precisely the three cases that fail.
Verified clean
Version bump is coherent: 0.4.0 in both marketplace.json and plugins/cogos-harness/.claude-plugin/plugin.json, descriptions byte-identical, and feat/threads-registry-v0.3.0 does hold 0.3.0 for this same plugin, so the merge-order advisory is factually correct. hooks.json is purely additive, no existing entry touched. The two unmerged branches edit hooks.json in disjoint regions (this adds a top-level Stop key; threads adds to the UserPromptSubmit array plus a new PreToolUse key), so it should auto-merge, but whichever lands second should re-validate the merged JSON rather than trust that. README's "36 skills" matches an actual count of 36 SKILL.md files.
Verified myself
py_compileonhooks/memory-janitor.py,skills/memory-janitor/check.py,canary/run_canary.py: all clean.json.toolon.claude-plugin/marketplace.json,plugins/cogos-harness/.claude-plugin/plugin.json,plugins/cogos-harness/hooks/hooks.json: all valid.- Hook, healthy fixture piped a Stop payload: silent, exit 0.
- Hook, forced over threshold: exactly one line of
{"decision": "block", ...}, exit 0, reason carrying all five resolved absolute values. - Hook, second forced Stop inside the cooldown: silent, exit 0;
baselineandpendingboth written. - Hook, inconsistent trio (floor=50 target=20 trigger=30): failed open with a stderr warning, no block, no state directory created.
- Checker PASS fixture: exit 0. Checker DEFECTS fixture: exit 1 with repair pointers.
- Checker CHECKER-ERROR paths: inconsistent thresholds and missing file both exit 2.
- Ledger routing with a lone
--baseline: row landed next to the baseline, confirming residual 2. - Findings 1 through 4 each reproduced with the minimal fixtures quoted above.
- Diff and commit message scanned for operator-local paths, machine names and attribution trailers: zero hits.
Fixtures were created inside a scratch directory in the review worktree and removed; the worktree is clean at 8c06926 and has been torn down.
1 participant
What
A new maintenance seat for the membrane: automatic memory-index (MEMORY.md) compaction, structured so the pilot is never conscripted into janitorial work mid-turn.
The loop: a deterministic Stop hook (
hooks/memory-janitor.py) checks the current project's memory index at end-of-turn. When it crosses the size threshold it snapshots a baseline and blocks once, handing the pilot resolved absolute paths and live numbers. The pilot's involvement is one nod: dispatch a background Haiku subagent against the standing directive (skills/memory-janitor/directive.md). Its output is then machine-graded (skills/memory-janitor/check.py: size window with overtrim floor, link-target multiset, bullet inventory incl. linkless bullets, headers, per-entry clause retention); defects come back as repair pointers for a bounded re-dispatch (max 2 rounds). Every round is ledgered per-project with the directive's sha — training data for directive evolution.Portability
CLAUDE_PROJECT_DIR→os.getcwd(), sanitized; silent no-op for projects without auto-memory.~/.claude/projects/<proj>/janitor/(never inside content-scannedmemory/), with a temp-dir pending fallback so an unwritable state dir cannot block-loop.floor < target <= trigger) validated at both the arming point (fail-open, no block) and the checker (CHECKER-ERROR, exit 2). Env knobs:COG_JANITOR_{FILE,TRIGGER,TARGET,FLOOR,STATE_DIR}.Review history
Three adversarial rounds before this PR: 17 issues (round 1) and 10 issues (round 2) all verified closed in code by independent reviewers who re-ran reproductions; final verify pass ruled READY. Five known LOW residuals are intentionally deferred and declared here:
check.pyreads the directive sha between verdict computation and print — an unreadable directive can still cost a computed verdict (same class as the fixed N2, different line).--baselinenow anchors on the baseline).stop_hook_activefrom the Stop payload; loop-safety rests on the marker files (with temp-dir fallback).Note: the canary will report the new hook as missing until this merges — it validates published state by design.
Merge-order advisory
Main is at cogos-harness 0.2.0. This branch takes 0.4.0 because
feat/threads-registry-v0.3.0(chronologically first, unmerged) owns 0.3.0. If this lands first, threads-registry should rebase its version to 0.5.0; if threads-registry lands first, this is already sequenced correctly.