fix: close all 2026-06-12 review findings — write-path safety, enforcement, read-path correctness#1
Merged
Merged
Conversation
The trailer amend previously committed the current index: a compound command like 'git commit -m wip && git add .' folded the newly staged files into the rewritten commit. --only with no pathspec amends the message against HEAD's tree; --no-verify stops the user's hooks from re-running (and possibly rejecting) a message-only rewrite. Tests prove the staged index survives untouched and a rejecting commit-msg hook no longer blocks the amend; README discloses both flags. (Review finding C2.) Lore-id: 28478409 Confidence: medium Supersedes: 31b909e3 Supersedes: 62f1d86e Supersedes: 1effc9ae Supersedes: 97f98290
…ior decisions Re-consolidating at the same HEAD read the existing note only when no amend happened; when one did, the new note held only the new atoms, the pre-amend note was deleted, and the trailer block was rebuilt from the new atoms alone — the first decision's reasoning survived nowhere. Now the existing note is read from the pre-amend sha unconditionally and merged into both artifacts. The trailer record is built from PRE-compaction atoms (a within-run rollup can no longer drop folded constraints from the permanent record), filtered to decision atoms (a noted rollup never leaks into Constraint: lines), and unioned with the trailer block already on the commit (the only survivor of a dream-pruned note), making the block monotone under crash-retry. Amend also gains --allow-empty so a message-only rewrite of an empty commit succeeds. Five tests pin this, including the exact data-loss repro, the flush-then-commit merge, idempotent replay, and folded-constraint survival past COMPACT_TOKEN_BUDGET. (Review finding C1.) Lore-id: 95073161 Confidence: medium Supersedes: 62c9629f Supersedes: 7ff4a524
The PostToolUse hook fired on command TEXT alone — a failed commit, a
'git commit || true' compound, or a 'cd sub && git commit' still amended
this repo's previous, unrelated commit. The gate now requires the
payload's exit code to be 0 when present, plus either git's own summary
line ('[branch sha]', first non-empty stdout line, anchored) naming
HEAD, or a fresh committer timestamp on a HEAD Cairn has not already
consolidated (.git/cairn/last-head — recency alone is not proof because
our own amend keeps the timestamp fresh). Anything else demotes to a
notes-only flush; errors fail toward notes-only.
isGitCommit exclusions (--amend/--dry-run) now apply per matched
segment with quoted spans removed, so a commit message mentioning
--amend no longer masks a real commit. committerDate uses %cI (honest
under --date/cherry-pick, unlike %aI). (Review finding C3 + minor.)
…tion survive clearJournal deleted the whole file after the model calls finished, so any entry appended in that multi-second window (a parallel hook, a second session) was destroyed unconsolidated — breaking 'a missed trigger loses nothing'. Consolidation now records the ids it read and consumeEntries re-reads the journal fresh, keeping unconsumed entries AND unparseable lines (a torn write is preserved, never deleted), written via same-directory temp+rename with a pid-suffixed temp name so concurrent consolidations cannot rename over each other. Entry ids now hash the reason and pid as well — consume-by-id is only safe when ids are unique per appended entry, and a per-process counter never increments (each hook is its own process). decisions.json writes go through temp+rename so a crash cannot corrupt the registry. (Review findings C4 + F8 + residual.) Lore-id: 1a7379eb Confidence: medium Supersedes: 8d0a94e0 Supersedes: 1dcd285f Supersedes: cda86887 Supersedes: 58650afd Supersedes: 62c9629f Supersedes: 9f3d544e Supersedes: f77e8142
…rage Five near-identical makeRepo/gitC copies collapse into tests/helpers/repo.ts; every git call now isolates the developer's global/system config so machine-level commit.gpgsign can't hang or sign test commits. CLI spawn tests run from SOURCE via tsx instead of a possibly-stale dist/cli.js. New guard tests close the review's biggest coverage gap (C6): a commit already on a remote is never amended, and a GPG/SSH-signed commit is never amended — in both cases the note alone carries the reasoning. The signed test pins a real subtlety: %G? only reports SSH signatures when gpg.ssh.allowedSignersFile is configured.
The line-regex check was bypassable: wrapped multi-line imports were
never inspected, './../store/index.js' passed the startsWith('./')
allowlist, non-literal dynamic imports were silently skipped, and
ambient globals (process, fetch, globalThis) need no import at all.
scripts/lib/engine-imports.mjs walks the real AST via the TypeScript
API, fails closed on unverifiable specifiers, flags ambient globals,
and is consumed by BOTH tests/decoupling.test.ts and scripts/verify.mjs
(the duplicated regex logic is gone). Negative self-tests prove each
bypass is caught. New companion rule, also enforced in both places:
child_process is confined to src/store/git.ts. (Review finding C5.)
Lore-id: d517f371
Confidence: medium
Supersedes: 62c9629f
Supersedes: 9f3d544e
Supersedes: 5d6e1f01
Supersedes: ab5d7987
Supersedes: 3f0e85f0
Supersedes: 51b42483
Supersedes: b465ae4c
Supersedes: 7dda1dc0
Supersedes: 8d641084
Supersedes: 356dc48c
Supersedes: 4108b9ac
Supersedes: 96a2c022
Supersedes: d25f3490
Supersedes: ab110ea4
Supersedes: 70fbce5c
Supersedes: 79d06c3a
Supersedes: 5c1ea1ca
Supersedes: 3d0b30c4
…CP layer src/mcp/graph.ts becomes src/read/graph.ts: the notes-and-trailers merge, Lore-id dedupe, and staleness/revert annotation are domain assembly, not protocol code — a future CLI surface or the planned process-level note cache plugs in at this seam without importing from mcp/. The annotate composition now exists in exactly one place (annotateEntries, stale-then-reverted) consumed by both the MCP server and the dream. The dream also gains Lore-id dedupe: duplicate notes (post-crash orphans) count once against the store budget and feed compaction once, while the prune step works from the UN-deduped entries so every physical copy of a folded atom is rewritten — a dedupe-before-prune zombie copy is structurally impossible, and a test pins the zero-or-one-copies invariant. (Review findings M1 + latent dedupe.)
…persedes values stripCairnTrailers dropped every non-'Key: value' line in the block — including RFC-822 folded continuations of FOREIGN trailers, so a re-amend silently corrupted another tool's wrapped Change-Id/Acked-by values. The filter now tracks which trailer a continuation belongs to and drops it only for Cairn's own keys. Supersedes was the one emitted value bypassing the oneLine() newline defense; values are now emitted only when they match the 8-hex content-hash shape, so a crafted note can no longer forge a trailer line through it. (Review findings M3 + minor.) Lore-id: 8cca4229 Confidence: medium Supersedes: 7ff4a524 Supersedes: 05ddfdfe Supersedes: b1e41104 Supersedes: a12ff6ad Supersedes: a642260e Supersedes: 785761a8 Supersedes: 6214d81f Supersedes: 7dda1dc0 Supersedes: 8d641084 Supersedes: 356dc48c
…put hardening recallChain used 'continue' on overflow, so a cheaper OLD atom could be kept after a newer one was dropped — a silent hole in the middle of the evolution arc, with supersedes links pointing at invisible records. Once a level-0 atom doesn't fit, all older level-0 atoms now drop too; level-1 rollups stay exempt (cheap, they ARE the compressed old arc). recent(n) reported hitting the requested n as truncated=true, which format.ts rendered as 'trimmed to stay under budget' — factually wrong under budget. RecallResult now carries limited (n-cap) separately from truncated (budget), each with its own message; budget wins when both. why/recent output now leads with a one-line preamble marking served memory as context-not-instructions (defense-in-depth against poisoned memory) and strips control characters from every user-derived value, including the file header. (Review findings M4, M5, M8.) Lore-id: 4f021f91 Confidence: medium Supersedes: 021c84c9 Supersedes: 1ac373f3 Supersedes: a628b30f Supersedes: 09d5b9a6 Supersedes: 6c626763 Supersedes: 7de20f62 Supersedes: 918d2174
…edupe
Two small fixes: consolidate() now bails with {ok:false, reason:
'no-head'} BEFORE any model call in a repo with zero commits (the
SessionStart-flush-in-a-new-repo shape), journal untouched; and
inferClusters dedupes ids within a single model-returned cluster so
sourceIds never carry duplicates and atom ids stay independent of
model repetition.
Coverage backfill for everything the review found untested: extractJson
recovery (fences, prose, braces-in-strings), transcript extraction
(tool_use filtering, corrupt tails, truncation), the no-key fallback
chain with a THROWING complete() plus an end-to-end keyless
consolidation, supersedes linking (link, trailer, render, lowest
confidence, case-insensitive rejected dedupe, threshold straddle), CLI
hook gating via spawn (garbage stdin always exits 0 with no side
effects), MCP error paths in smoke (non-repo dir, absolute path), and
the dream staleness test's tautological assertion replaced with direct
fold + provenance assertions. (Review findings M6, M7, M10, M11, F7, T5.)
Lore-id: 9f1f3ddf
Confidence: medium
Supersedes: cab1a3ed
Supersedes: c7b67612
Supersedes: 4f43d380
Supersedes: 840c839e
Supersedes: ea8d1db9
Supersedes: ad22734b
Supersedes: 3f0e85f0
Supersedes: 51b42483
Supersedes: b465ae4c
Supersedes: 6214d81f
Supersedes: 7dda1dc0
Supersedes: 8d641084
Supersedes: 5b458508
Supersedes: 356dc48c
Supersedes: f86aebd2
Supersedes: 62c9629f
Supersedes: 9f3d544e
Supersedes: ab5d7987
README: stale '17 tests' count removed (no count to re-drift), Configuration gains STORE_TOKEN_BUDGET (the dream's knob) and DEFAULT_RECENT, the MCP resolution paragraph now matches the code (CLAUDE_PROJECT_DIR with a process-cwd fallback; roots/list documented as not implemented), and the notes-push instructions carry a privacy note — session-derived text reaches notes, redaction is explicit future work. DESIGN.md gets the same roots/list correction. The 2026-05-30 verification REPORT.md is banner-marked superseded (its open findings are fixed) pointing at the 2026-06-12 review. RollupAtom.sourceIds comment now says identification-not-reconstruction. The /cairn:decision heredoc delimiter is a machine-generated random string instead of the guessable CAIRN_INTENT_EOF. (Review R15, R16 + minors.) Lore-id: 3df12c00 Confidence: medium Supersedes: 7a4c6652 Supersedes: 31b909e3 Supersedes: 62f1d86e Supersedes: 1effc9ae Supersedes: 97f98290 Supersedes: 021c84c9 Supersedes: 1ac373f3 Supersedes: 075d08cf Supersedes: a628b30f Supersedes: 5760d38f Supersedes: a821d5b1 Supersedes: 9efa8ad1
Six findings confirmed by the post-implementation review's validator wave, all applied: - Foreign trailers (Signed-off-by etc.) were demoted out of git's trailer block by the amend's blank-line join when the stripped body still ended in trailers — 'git interpret-trailers --parse' stopped seeing them; now joined into one block (empirically reproduced, test added). - A commit landing during the model window had its message silently replaced: the amend now re-reads HEAD and bails (head-moved) when it no longer matches; the note stays keyed on the pre-move sha (test with a mid-call concurrent commit). - Multi-line model constraints accumulated duplicate Constraint lines across amends (raw vs oneLine-normalized never deduped); constraints now normalize before the union (test added). - The no-head probe leaked git's 'fatal:' to the session terminal (headShaIfAny allowFail probe); a writeLastConsolidatedHead failure no longer propagates out of an otherwise-successful consolidation. - Dead cliPath() export removed; tsFiles deduplicated into the shared scanner; README Layout updated for src/read/; server.ts comment no longer implies roots/list resolution. Rejected by validation (recorded, not applied): rollup ledger duplication (unreachable — compactGraph always re-rolls), foreign 40-char Supersedes preservation (non-conformant to the Lore 8-hex subset), capture-to-read layering (plan-intended shared seam), prior.loreId supersedes filter (requires a hash collision). Lore-id: 0d6e5dfd Confidence: medium Supersedes: 7ff4a524 Supersedes: 05ddfdfe Supersedes: b42c5130 Supersedes: 38930329 Supersedes: 44a555b5 Supersedes: 507fa351 Supersedes: 85226637 Supersedes: 5f63f53f Supersedes: fce7b0ed Supersedes: ced05abc Supersedes: 0d331561 Supersedes: cab1a3ed Supersedes: 16861973 Supersedes: 5d330713 Supersedes: 6a0801f0 Supersedes: 1dcd285f Supersedes: cda86887 Supersedes: 18318f60 Supersedes: 58650afd Supersedes: 6f0bcc30 Supersedes: 7a4c6652 Supersedes: 31b909e3 Supersedes: 62f1d86e Supersedes: 1effc9ae Supersedes: 97f98290 Supersedes: 8ce81bf4 Supersedes: f86aebd2 Supersedes: 62c9629f Supersedes: 9f3d544e Supersedes: ab5d7987 Supersedes: b5e3eb63
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Implements the hardening plan closing every finding from the 2026-06-12 five-perspective codebase review, in blast-radius order.
What changed
Write-path safety (the critical core)
--amend --only --allow-empty --no-edit --no-verify) — it can never fold staged work into a commit or be blocked by a rejecting commit-msg hook. Disclosed in README..git/cairn/last-head); anything else demotes to a notes-only flush.cd sub && git commitandgit commit || truecan no longer amend the wrong commit.Enforcement & structure
verify.mjs(wrapped imports,./../escapes, non-literal dynamic imports, and ambient globals are all caught — with negative self-tests), plus a new rule:child_processconfined tostore/git.ts.src/read/(the seam the future note cache drops into); the dream consumes the same annotation composition and dedupes orphaned duplicate notes without zombie copies.Correctness & honesty
git interpret-trailers --parse; Supersedes values are shape-guarded against forgery.why()chains have no silent mid-chain holes under budget pressure;recent(n)distinguishes "hit the requested n" from budget truncation.Coverage & docs: 124 tests (was 70) — amend guards (pushed/signed), no-key fallback with a throwing model, supersedes linking, extractJson/transcript/CLI-gating/MCP-error paths, shared git-config-isolated test helpers. README/DESIGN doc drift fixed; the stale 2026-05-30 verification report is banner-marked superseded.
Testing
npm test: 124 pass, 0 fail (every fix landed with a regression test proven to fail on the old code where practical)npm run verify: all behavioral + structural checks pass (build, suite, MCP smoke over stdio, decoupling, child_process confinement, non-goals, Lore interop)Known residuals (advisory, accepted)
tests/decoupling.test.tsimports the shared.mjsscanner under@ts-ignore(tests are outside tsc); a.d.tswould close the blind spot.git -Ccommit detection remain documented accepted tradeoffs.Post-Deploy Monitoring & Validation
Local dev tool — no production runtime. Validation plan for dogfooding (owner: Andrew, window: next few days of normal use):
Lore-idblock;git diff --cachedis untouched after commits;why(file)returns merged chains after repeated consolidations at one HEAD.cairn:warning lines on hook stderr; trailers appearing on the wrong commit; journal entries vanishing without a matching note (.git/cairn/journal.jsonlvsgit notes --ref=cairn list).git logandgit notes.npm run buildin the plugin checkout) after merging, per the install setup.🤖 Generated with Claude Code