cc-warehouse - architecture review board

FRESH REVIEW 2026-08-23 · snapshot: master 4824098 (257 commits) · rendered from SOURCE.md by /architecture

This review supersedes the 2026-07-24 one at 1517bba, which was not decayed but DESTROYED - the repository was deleted and re-created on 2026-08-10 (ticket 28.20), so that commit no longer exists anywhere and could not be diffed against. Every file:line below was RE-DERIVED at 4824098 and re-verified first-hand, so the decay banner this page carried is retired.

Method: the named review skill is not enabled in this session, so 5 parallel read-only agents substituted for it - the same lens split as before, plus a new lens for archive.py, a 900+ line module that did not exist at the last review. Two findings turned out to be live bugs, not architecture debt, and were fixed the same session (see C12 and C6).

Build state: v1 is CLOSED. Since the last review, a major archive-first rewrite landed (tickets 19/21/25/27/29/30) that this board had never looked at until now.

module seam duplication / leak deep module
PROPOSED TICKETED-nn BUILT Strong Worth exploring Speculative tiers: VERIFIED first-hand · AGENT-REPORTED · CONTRACT

The contract in contract/ is locked and is never edited by this board. A lens report is EVIDENCE to reconcile, never a settled finding - the two consequential claims below were independently re-read from source, not trusted from an agent report alone.

In plain English.

This page lists the code changes most worth making, ranked by how much they'd help. This time, the review found something better fixed than filed: the same small bug (a saved file quietly not getting updated when it should) had already been caught and fixed once earlier the same day, in one of three near-identical copies of the same logic. The review found the SAME unfixed bug hiding in a second copy - and fixed it on the spot, along with a separate real gap where a safety check that exists for one command was missing on another. Both are done. The bigger, still-open idea both point at: stop keeping three copies of the same rule in the first place.

Top recommendation (re-ranked this review)

C12 - One replace-if-larger primitive, not three copies PROPOSED Strong

NEW this review, and it moved straight to 1st because it is not argued, it is PROVEN: the same defect (equal size silently read as equal content, F1) was found and fixed once in write_session_folder earlier this session, then found AGAIN, independently, still live in the sibling write_subagent - worse there, since a sub-agent has no manifest to even record the loss. Both specific bugs are now fixed; the reason a second copy could still be broken - the rule lives in three places kept in agreement by hand - is not.

The board - re-ranked 2026-08-23

1st · C12 (NEW) - One replace-if-larger primitive, not three copies

PROPOSED Strong

archive.py

write_session_folder

equal-size bug FIXED ticket 30 (earlier today)

write_subagent

SAME bug found live, FIXED this review (no manifest to record it in - F6)

write_source

left as-is: write_session_folder always runs after it on the same bytes and decides what renders

Problem. Three inline copies of "larger replaces, smaller is refused, equal size means equal content." That last clause was WRONG (R1/F1: size only answers ordering, never identity) and got fixed in one copy by ticket 30 earlier the same session. This review found the identical unfixed defect independently in a second copy, write_subagent - worse there, since sub-agents have no manifest.json, so the loss was not mis-rendered, it was invisible.

Fixed this session, both specific bugs.

write_subagent now compares bytes at equal size (a new SubagentResult.refused_equal_size field). write_source's stale comment, which claimed to follow write_session_folder's rule "exactly," was corrected. The CANDIDATE itself - one shared rule instead of three copies - is not done.

Solution direction. One shared _replace_if_larger(path, data) -> ReplaceOutcome all three callers use, so a future fourth defect in this rule is fixed once, not remembered and reapplied three times under pressure - which is exactly what nearly happened here.

Contract ties: R1 (as amended), R5, F1, F6 · Tier: VERIFIED at 4824098 (both bugs independently re-read from source, not trusted from the agent report)

2nd · C6 - cli.py: verbs hand back results, not cursors

PROPOSED Strong one sub-finding BUILT

cli.py · build.py · registry.py · relocate.py · share.py

Problem (a, b, d unchanged; c had a live consequence). (a) render no-row-vs-superseded policy stranded in _render_session (cli.py:863-920). (b) project-exists SELECTs inline in _run_project (cli.py:1036-1097) though registry.py owns the edits. (d) already fixed by an earlier, unrelated change - relocate's halted-run recovery now uses a typed accessor, confirmed by reading current code. (c) _out_under_warehouse (cli.py:933-949) guards write_projection at exactly one of five call sites - the two in share.py (both force=True) had none.

This was live: ccw share --out could write into the warehouse store (F9).

Confirmed by running it: ccw share s:<key> --out <warehouse>/objects/x wrote the file, no error. An unguarded, force=True path into content-addressed storage.

Fixed this session.

_run_share now calls the same _out_under_warehouse guard before writing. The structural problem - the guard lives at the CLI layer, not inside write_projection itself, so a SIXTH call site could reintroduce the same gap - is not fixed, only this session's instance of it.

Solution direction. Verbs own their reads and guards; the write-guard moves INTO write_projection itself so every caller, present or future, gets it automatically.

Contract ties: F9 · overlaps C2/C4/C12 · Tier: VERIFIED at 4824098 (the live gap independently confirmed by execution)

3rd · C3 - Make the risky transforms public and I/O-free

PROPOSED Strong

share.py · relocate.py

share.py (private)

_scan_secrets :279
_is_generic_secret :261
_redact_tree :202
_redact_value :168
_redaction_patterns :159

relocate.py (private)

_form_patterns :169
_sub_tree :204
_tree_matches :228
_references :242
_rewrite_bytes :263

4 public entry points now (share/prepare/commit/discard), all still I/O-only

Cost, confirmed still real. Zero in-process private-helper calls across 8 test files now (5 for relocate, 3 for share, including the newer --EXPOSED path). Every exercise of this logic is a run_ccw subprocess.

Solution direction. Promote thin public transforms - redact_payload(text, patterns), scan_secrets(text), rewrite_bytes(path, text, patterns) - so tests assert against the transform directly.

Tier: VERIFIED at 4824098 · Why 3rd (was 1st): unchanged Strong reasoning, but C12/C6 both now have a PROVEN live consequence this review; C3's cost is real but has not (yet) caught a live defect.

4th · C1 - Parse once: one parse product

PROPOSED Strong, worse

render.py · build.py · archive.py

flowchart LR
  A[one payload] --> M[render_markdown 1185-1199]
  A --> H[render_html 2187-2204]
  A --> B[build_manifest 2238-2295]
  A --> W[write_session_folder 603/652/663]
  M --> P1[build_conversation + parse_session]
  H --> P2[build_conversation + parse_session]
  B --> P3[build_conversation]
  W --> P4[parse_session x2 + sha256 again]
  W -.iter_projection_files.-> M
            

Problem, grown since last review. render.py's own redundancy is unchanged (build_conversation x3, parse_session x2, sha256 x3 per write). NEW: archive.py's write_session_folder parses/hashes again on top of that, and build.build() can call BOTH write_projection and write_session_folder for the same payload when both trees are configured - up to build_conversation x6, parse_session x5, sha256 x7 per head. Multiplicative, not additive.

Solution direction. Hoist parsing to the orchestrator; build models once, pass into every emitter and into archive.py's own decision points.

Contract ties: R8, R9; DESIGN section 4 · Tier: VERIFIED at 4824098

5th · C2 - Deepen the batch-verb protocol

PROPOSED Strong, worse

reports.py · cli.py · sweep.py · build.py · migrate.py · relocate.py · import_tree.py · archive.py

sweep.py

:381, :421-422

migrate.py

:114, :133-134

build.py

:612, :654-655

relocate.py

:972, :978-979

import_tree.py (new)

:196, :219-220

archive.py (new)

:749, :756-757 - different report shape too

Problem, grown from 4 to 6 copies. Two new modules since the last review each added their own lock-acquire shell instead of reusing one. cli.py's report/tally half also grew from 4 to 6 instances, plus a 7th partial in _run_reindex.

Solution direction. Unchanged - a with_lock(root, name) -> BatchReport runner plus a shared render_batch(report) -> exit_code in reports.py.

Contract ties: R9, R10; R14 untouched · Tier: AGENT-REPORTED at 4824098

6th · C7 - Type the Block discriminant

PROPOSED Worth exploring

parser.py · render.py

Concrete proof, not a hypothesis.

thinking_withheld (parser.py:641, ticket 20) is a real Block kind, handled correctly in render.py:576 - but was never added to the Block docstring's enumeration (parser.py:439-446). The exact drift this card warns about, already happened.

Fan-out is larger than described: beyond the ~7 originally-named sites, a census found kind comparisons at roughly 15+ sites, clustered in _phase_meta (render.py:694-761) alone.

Solution direction. A typed discriminant (Literal or enum, stdlib, R7-safe) driving dispatch through mappings, so a missing arm is a type error.

Contract ties: F6, R7 · Tier: AGENT-REPORTED at 4824098

7th · C13 (NEW) - folder_is_current: pure decision logic welded to the filesystem

PROPOSED Worth exploring NEW 2026-08-23

archive.py

Problem. _current_manifest (archive.py:454-499) fuses a file read with three PURE dict comparisons (source_hash, config, renderer_version) that decide "is this folder current." No way to unit-test a renderer_version mismatch without writing 5 real files first - the same shape C11 already names, in a module every incremental build/migrate call goes through.

Solution direction. A pure _manifest_matches(manifest: dict, source_hash, options) -> bool taking an already-loaded dict.

Same shape as C11 · Tier: AGENT-REPORTED at 4824098 · archive.py's first appearance on this board

8th · C4 - Give the head concept one home: a catalog read seam

PROPOSED, mostly resolved downgraded from Strong

build.py · catalog.py · share.py

The headline finding is CLOSED - not by this board, by ticket 29 mechanism 1 (2026-08-20).

The head predicate, join, and ranking are now ONE shared fragment, _HEAD_RANK_CTE (build.py:323-334), used by both _heads() and head_for_short(). The false "shares the join" docstring the last review flagged is now TRUE.

What remains: a small cosmetic duplication (each function's own final SELECT + row-unpacking, ~15-20 lines). New, unrelated: archive._session_rows (:835-861) overlaps the head query on two columns - already self-flagged in its own docstring, not deduplicated.

Contract ties: R6, R8, R9 (the R8/F6 overclaim risk is now resolved) · Tier: VERIFIED at 4824098

9th · C5 - One turn walker, two serializers

PROPOSED Worth exploring

render.py

Problem. The turn walk is still written twice: markdown via _turn_body (:999-1015) + _user_md (:1018-1036); HTML via _claude_inner (:1984-1997) + _turn_html (:2000-2072). Leaf rendering confirmed still single-owner via _render_block. Correction: the old citation _render_turn no longer exists by that name.

Solution direction. One turn-walk yielding typed events, each emitter supplying only leaf rendering.

Contract ties: R9 · pairs with C1 · Tier: AGENT-REPORTED at 4824098

10th · C9 - One source walker, two policies

PROPOSED Strong

sweep.py · migrate.py · relocate.py

Good news: the worried-about THIRD copy did not happen. migrate._walk was made public as migrate.walk_jsonl specifically so ticket 25.4's import_tree.py could reuse it - confirmed, no new os.walk there.

Problem, unchanged. sweep.py and migrate.py's walker scaffolds are still near-verbatim duplicates. New adjacent finding: relocate._scan_content (:298-413) has its own THIRD walker of the same general shape, serving a different job.

Solution direction. One walk_transcripts(root, *, classify) owning the scaffold.

Tier: AGENT-REPORTED at 4824098

11th · C14 (NEW) - No owned 3-way session/subagent/not-a-session discriminant

PROPOSED Worth exploring NEW 2026-08-23

archive.py · import_tree.py

Problem. Only two booleans exist (is_subagent, is_session); the one caller needing the 3-way classification, import_tree._kind (:84-101), reinvents it as a plain str compared by literal equality - the C7 shape, at lower stakes (one caller today).

Solution direction. A small classify(data) -> Literal[...] combinator any future second caller can share.

Same shape as C7 · Tier: AGENT-REPORTED at 4824098

12th · C10 - A catalog read-scope for relocate

TICKETED-12b

relocate.py

Unchanged verdict, re-verified. The O(N)-per-candidate churn is still gone (_encoded_moves, :464-517). What remains is O(1) per-apply churn in _project_for_cwd/_encoded_owner, called from _preflight with no live connection available to hand it - constant per apply, not a scaling bug.

Contract ties: F5; ticket 12b list · Tier: VERIFIED + CONTRACT at 4824098

13th · C11 - Split the short-key band math from its query

PROPOSED Worth exploring

catalog.py

Unchanged. _short_key (catalog.py:146-163) still welds the pure prefix-band math to the conn.execute collision query in the same loop iteration.

Solution direction. Split the pure "choose a length" function from the collision lookup. Same shape as the new C13.

Contract ties: F5 · Tier: VERIFIED at 4824098

In plain English.

Thirteen ideas, two of them brand new, one mostly finished by an unrelated fix, and two real bugs already patched along the way. The clearest theme: this codebase has three or four places where the exact same small rule gets copy-pasted, and every time that has happened, at least one copy has eventually drifted or been forgotten.

Verified healthy - the deep end, leave it alone

Not on the board - owned elsewhere

The remaining hand-rolled flag scanners in cli.py

Ticket 13 closed WITHOUT removing them; two survive deliberately (_sweep_source, _render_flags) and now document why.

config.toml parsed in three modules - RESOLVED 2026-07-24

config.py is now the only module in src/ importing tomllib.

Relocate's catalog connection churn

Card C10 above (partially closed).