Repository navigation
feat(adf): compile preserves retained sections (#318 5/6) - #358
Conversation
#303) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
stackbilt-admin
left a comment
There was a problem hiding this comment.
CHANGES_REQUESTED
Adversarial review of #358 (PR 5/6 of #318, closes #303) at 2756951. I built it from a detached worktree and compared it with an origin/main (67f3c28) build. I ran 24 CP-1 variants, 27 data-loss and block variants, 23 pointer and unmanaged flows, restore round trips, 7 circular-import load orders, and --check JSON parity. Harness: rv358/{h,cp1,dl,ptr,rs,circ}.cjs in the session scratchpad.
The core design is sound and CP-1 holds for every ordinary repo. It holds for all 4 targets, with and without weights, with CRLF, with or without a trailing newline, with extra trailing blank lines, with pre-#207 index paths, with a stale ## VENDORS, with stale ## PROTOCOL and ## SESSION_PROTOCOL _(load-bearing)_ sections, with lowercase ## environment, ### Environment, stray edits, and with no ROLE or ON_DEMAND. In all of these PR --write is byte-identical to main, --write is idempotent, and --check exits 0 after one --write. The compiler banner and title text have not changed since #203, so files compiled by older Charter versions produce no hidden diff. The three items below must be fixed (or, for item 1, explicitly signed off) before merge.
Required changes
-
Text from a
.ai/value is carried into the block when.ai/changes or deletes it. This breaks CP-1 and INV-P5-4. Location:packages/cli/src/commands/adf-retained.ts:130-136(legacySectionsdedupe).- The case: a
.ai/text value that itself contains a retained heading. For exampleNOTES:with the lines## Session notes/- keep tabs, or## Environment/Use WSL.. The dedupe (fresh.includes(text)) handles only the case where.ai/is unchanged. - What happens: when the user later edits that value (
Use WSL.→Use Docker.) or removes the module, the existing compiled file still holds the old text. That old text no longer matches the fresh render, so it is taken as user content and moved into a newcharter:retainedblock. - Result: the file carries both the new and the old instruction, or only the old one after a removal, permanently. Each later
--writecopies the block. Reproduced on all 4 targets, three variants (cp1.cjs: "then .ai edited", "then .ai removed it", "## Environment, .ai edited"). - Why it matters: this repo has no user-written retained section, yet its bytes differ from main. It is the #296 class ("deleted rules come back") through a new route.
- Fix, one of:
- (a) In legacy mode, do not adopt a retained H2 that sits directly inside a rendered section's body. The renderer's text bodies have no blank line before an embedded
##line, while a hand-added section follows a blank line. Pin it with tests. - (b) Do not adopt a legacy heading line that appears, dedented, in any current
.ai/module. This covers edits only, so it must be combined with (c) for removals. - (c) An explicit operator CP sign-off that accepts this carve-out, plus a CHANGELOG and cli-reference sentence and a test that pins the behaviour.
- (a) In legacy mode, do not adopt a retained H2 that sits directly inside a rendered section's body. The renderer's text bodies have no blank line before an embedded
- My recommendation: (a), or (b) plus (c). Do not merge it silently.
- The case: a
-
The file grows without bound when a
.ai/text value contains the start marker line. Location:adf-retained.ts:167(lines.findIndex(l => l.trim() === RETAINED_START)).- The case: the rendered
.ai/text contains<!-- charter:retained:start -->. That line is found first and treated as the block start, so the "block" is everything from it to EOF. That includes generated sections and the module index. It is then appended to the next render. - Result: CLAUDE.md went 445 → 735 → 995 → 1255 → 1515 bytes over 5
--writeruns.--checkstays stale forever, and each run duplicates generated content. - Fix: skip marker lines that the fresh render itself contains, for example by matching only after the last line that
freshaccounts for, or by counting occurrences infresh. Add a test. An end-only marker in.ai/text is already stable (429 bytes on every run).
- The case: the rendered
-
Section parsing ignores code fences, so part of a "kept" section is dropped. This breaks INV-P5-2 (and W4 under
--force). Location:adf-retained.ts:79(isH2) and:85-100(sectionsOf).- The bug: every line whose trimmed form starts with
##starts a new section, even inside a fenced code block or an indented code block. - Example: a legacy
## Environmentthat holds a fenced code block with a## install depscomment line in it:## Environment ```sh ## install deps pnpm i- after fence
The section is cut at the comment. On `--write`, `## install deps`, `pnpm i` and `- after fence` are deleted, and the run prints `Written CLAUDE.md (kept ## Environment)` with exit 0. A `<!-- agents-modules -->` line inside a fence also cuts the section, and so does a 4-space-indented ` ## x` (CommonMark treats that as code, not a heading). - Where it shows up:
- Legacy compiled files.
--mode compiled --forceover unmanaged files.retainedSectionsandkeptSectionsreporting: restore reported"keptSections": ["## Environment", "## install"]for such a block.
- Pointer conversion is protected by
refused-bloated. - Fix: track
```and~~~fences insectionsOf, and treat only lines matching/^ {0,3}## /as H2. Add tests for all three shapes. - Main deletes the whole section, so this is not a regression. But it is a new promise ("kept") that the code does not keep, and the fix is small.
- The bug: every line whose trimmed form starts with
Rulings on the deviations
- (a) Accept. Copying the existing block verbatim, note line and blank lines included, is what makes C1 / INV-P5-5 hold (an edit inside the block, or removing the note, stays
fresh, as tested). The note is re-emitted only when the block is rebuilt. - (b) Accept. Merging sections from outside the block into it, rather than "first source wins", loses nothing and is deterministic. Verified: a retained section after the block is merged, and the next run is idempotent.
- (c) Accept, subject to item 1. Stopping at the module-index fence is correct. The "already rendered" dedupe cannot drop a user's only copy, because the text is by construction present in the output. The risk is the reverse (item 1). Inside a block nothing is deduped, so a user who wants a duplicate kept can keep it there.
- (d) Accept. A stray end marker falls back to legacy; the section is kept and the marker is dropped (tested).
- (e) Accept. Collision matching ignores the weight suffix:
## CONTEXTnext to## CONTEXT _(load-bearing)_is the same visible collision. - (f) Accept.
retained-unfencedwins over content-stale. The remedy (--write) fixes both causes, and the more specific reason is the more useful one. - (g) Accept: no working flow breaks.
- Unchanged: every pristine pointer template (CLAUDE.md ×2, AGENTS.md, .cursorrules, GEMINI.md), LF and CRLF, under
--mode compiled, gives the same bytes and exit code as main. So doesbootstrap --yesfollowed bycompile --target all --write --mode compiled, and so does a pristine recorded-compiled pointer under plain--write. - Newly refused (exit 1; main wrote with exit 0): only a pointer-looking file holding bloat or stray lines, either under
--mode compiledor recordedcompiledunder plain--write. On main those were the silent-loss cases. - No other command calls the compile write path (only
adf.tsdispatches it). - Doc nit: cli-reference.md:567 says "Converting a pointer refuses". It should also cover a recorded-compiled file that is still a pointer under plain
--write.
- Unchanged: every pristine pointer template (CLAUDE.md ×2, AGENTS.md, .cursorrules, GEMINI.md), LF and CRLF, under
- (h) Accept.
retained-divergentdisappearing fromRestoreRefusalis a value that is no longer emitted, and the CHANGELOG says so under Changed. It is not re-exported frompackages/cli/src/index.ts.
Verified OK
- Idempotency:
--writetwice is byte-identical in every non-failing scenario across all targets. CRLF input produces LF output. - Block edits:
- An edit inside the block survives, and so does a non-retained heading inside it.
- Deleting a section inside the block, or deleting the whole block, sticks.
- A start marker with no end runs to EOF and the end is re-added.
- A block moved into the middle of the file is moved to the end.
- A marker line with trailing whitespace is handled.
--check:- JSON rows equal main's for pristine, notes, bloated, unmanaged and absent CLAUDE.md, apart from the added
staleReasonkey. The PR2 pointer rules are unchanged. - A legacy section reports
retained-unfenced, and so does an unfenced section together with a.ai/change.--writethen makes the file fresh.
- JSON rows equal main's for pristine, notes, bloated, unmanaged and absent CLAUDE.md, apart from the added
- Restore:
- A block or legacy section is carried into the pointer,
keptSectionsis correct, andcompiled-differsis false when only the block differs. - Doctor's
adf vendor bloatpasses on the restored pointer. - The pointer → compiled round trip loses nothing.
- A block or legacy section is carried into the pointer,
- One shared function (K1):
--write(adf-compile.ts:420/462),--check(:622) and restore (compiledMatchesFresh,composeCompiled) all go throughcomposeCompiled, with no duplicated logic.score.tsis untouched (K4). - INV-P5-10 (circular imports):
adf-retained.jstouches its imports only inside functions (dist lines 48, 158, 166, 192). Loading the module first, oradf-compile,adf-vendor,adf-tidy,adf,adf-migrateorindex, all compose correctly. - Docs: the CHANGELOG puts CP-1/2/7 under Changed and #303 under Fixed (Unreleased). Both READMEs and cli-reference (retained block,
--check, statuses, restore) are accurate apart from the (g) nit. - This repo:
adf compile --target all --checkexits 0 on the branch build (4 × pointer-ok). - Tests: the CLI suite gives 688 passed and 3 failed. The 3 failures are Windows path-separator tests (bootstrap #307, context-refresh, serve-context) and fail identically on main.
Non-blocking notes
-
Restore carries a non-retained heading the user put inside the block (for example
## My Notes) into the pointer as a plain H2. The pointer is then tidy-bloat:tidy --dry-runextracts it tocore.adf, and the reverse--mode compiledreportsrefused-bloated. Nothing is lost, but consider listing such headings in the restore output or the docs. -
Two cases are dropped silently, as on main:
- a non-retained section after the block;
- a second
charter:retainedblock's non-retained content.
Consider merging every block.
-
stripCharterSentinelsaccepts<!--\s*charter:retained:start\s*-->, butcomposeCompiledneeds the exact trimmed text. A user who retypes it as<!--charter:retained:start-->falls back to legacy, so non-retained headings inside are dropped. Consider sharing the regex. -
An end marker inside a fenced code block in the block ends the block early, so the lines after it are dropped. This matches
stripCharterSentinelssemantics, but it is worth one doc sentence. -
User edits to
## Session Start,## Module Indexor the preamble of a compiled file are still dropped, as on main and by design (contract §1). The messaging is fine.
Closes #303
Refs #318
Summary
adf compile --writekeeps the user's retained sections (## Environmentand the other headingsadf tidyretains) in a fenced block at the end of each compiled file, instead of deleting them (compiled re-compile, the #303 hole) or refusing on them (pointer conversion, #295).packages/cli/src/commands/adf-retained.ts:composeCompiled(fresh, existing, file)is the one function--write,--checkandvendor restoreshare. It reads fromblock,legacy,pointer,unmanagedornone. AlsolinesConvertWouldDropandcompiledMatchesFresh. It never importsadf.ts(adf migrate writes the generic pointer for GEMINI.md/AGENTS.md: POINTER_TEMPLATES built during a circular import #355 class).adf-compile.ts:--writewrites the composed output.refused-retainedis retired. New refusals for converting a pointer:refused-bloated(remedyadf tidy --source Ffirst, then--force, which discards N item(s)) andrefused-would-drop. Rows addretainedSectionsandretainedSource.--checkcompares against the composed output and addsstaleReason(content|retained-unfenced).adf-vendor.ts: restore'scompiled-differsignores the block and legacy retained sections, and restore carries them into the pointer (keptSections). Theretained-divergentrefusal is retired.packages/adf(compileAdf),score.ts,adf.ts,adf-migrate.ts,index.ts, tidy, doctor, hooks.Before → after
## Environment--writedeletes it, exit 0--checkretained-unfenced) until one--write, then fresh--mode compiledrefused-retained--mode compiledrefused-bloated, tidy remedy firstrefused-would-drop;--forcediscards--mode compiled --forceover a hand-authored fileretained-divergentSign-offs (Gate 2)
--write. Repos without them stay byte-identical, pinned by thecompile-no-retained.main-67f3c28.jsonfixture snapshot, generated by main's build.stale(retained-unfenced) under--checkuntil one--write.refused-retainedis no longer emitted (CHANGELOG: JSON consumers no longer see it), and neither is restore'sretained-divergent..cursorrulesincluded. CP-4: carried under--mode compiled --force. CP-5: on a heading collision both are kept, with a warning. CP-6: score keeps the pure compile.replacedPointersis not added (previousModecovers it).Invariants and where they are tested (
adf-retained.test.tsunless noted)regression and structure › … byte-identically to main @ 67f3c28. Fixture scenarios: absent, hand-edited re-compile with a stale## PROTOCOL, CRLF re-compile, clean pointers. The e2e repeated this against a main build.#303 holeCLI test; conversion tests for pointer,--forceand unmanaged--force;adf-compile.test.ts--force still carries….is idempotent for every source, andtwo --write runs are byte-identical for every target … LF and CRLF.never resurrects a generated heading from a legacy file.copies an existing block verbatim…, plus the edit step of the CLI adf compile: preserve retained sections instead of only refusing on them #303 test.compiled-differstest).stdout mode prints the pure render even when the file has a block.refused-retained):never emits refused-retained…, plus the checkstaleReasonassertions and the updated rows inadf-vendor-consumers.test.ts.adf.tsimport; works when the CLI entry loads first):adf-retained.ts never imports adf.ts…(resets modules and imports../indexfirst).Mutation-checked: each core guard (verbatim block copy, generated-heading filter, empty-means-no-block, check and write compose, conversion guards, restore differs and carry, embedded-render guard, fence boundary, guard-by-markers) was broken in turn, and at least one test failed each time.
The sentinel claim was verified:
stripCharterSentinelsmatches<!-- charter:retained:start|end -->(tidy'sextractBeyondPointer, doctor's bloat scan, and the markdown parser used by migrate). Compiled files are skipped by the banner in tidy, doctor and migrate. Score reads the file text or the pure compile, so it needs no stripping.NOT in PR5
isRetainedHeading. A hash-style sentinel for.cursorrules.--check(still strict bytes). Banner text change. Legacyagents.md(PR6).Implemented via constellation-loop against a locked blueprint.
🤖 Generated with Claude Code