Skip to content

feat: native tallyman_core.catalog store (drop xorq catalog package) - #103

Merged
paddymul merged 27 commits into
mainfrom
plan/native-catalog-store
Jun 17, 2026
Merged

paddymul merged 27 commits into
mainfrom
plan/native-catalog-store

Conversation

@paddymul

@paddymul paddymul commented Jun 16, 2026 •

Copy link
Copy Markdown
Contributor

What

Replaces xorq's catalog repo manager with a tallyman-native, content-addressed recipe store, and decomposes the monolithic catalog.yaml into per-concern git-tracked files. Design + investigation record: plans/native-catalog-store.md.

Why

xorq's catalog/ package is the source of the recurring catalog pain: assert_consistency runs on every add (even --no-sync) and rejects any extra tracked file (#48), and the add subprocess commits outside tallyman's flock, racing index.lock. Dropping the package removes the constraint by construction — tallyman already owns the catalog model; the subprocess was the last tendril.

What shipped (two cuts)

Key decisions

  • Recipe zip archives the allowlist {expr.py, xorq_build/, schema.json, manifest.json}, deterministic; the per-expression wheel is dropped (~178KB → ~6KB).
  • Checkpoint owns zip creation under the project lock.
  • All mutable state → tracked files that roll back with git reset.
  • User recipes stay on import xorq.api as xo (catalog-free guarantee scoped to src/); the authoring-surface switch is a separate codegen-validated follow-up.

Review hardening (PR #103)

A multi-agent review pass landed on top of the implementation: a third assert_catalog_consistent guard (decomposed sidecars must name a hash in the durable recipe set), a shared atomic_write_text across every tracked-surface writer, entry-dir cleanup on every failed-build path, a loud notebook.load on malformed JSONL, and added reset/guard coverage. See the review thread for the per-finding trail.

plans/native-catalog-store.md carries the full investigation findings, risk register, module boundary, and the ordered TDD implementation sequence.

🤖 Generated with Claude Code

@paddymul

Copy link
Copy Markdown
Contributor Author

Code review (Claude)

Docs-only PR: a design plan (plans/native-catalog-store.md) plus a 3-line .gitignore addition. I verified the load-bearing technical claims against the actual tree before reviewing the prose.

Verification — the claims hold exactly:

  • import xorq loads 0 catalog modules; import xorq.api loads 12 ✓
  • xorq.expr.api.connect is xorq.api.connect → False, with incompatible signatures (connect(session_config=None) vs. connect(resource, **kwargs)), so Risk T-33: Memory-scaling integration test for React embed #3's "resolves the symbol, fails at call time" is real ✓
  • pyproject.toml:77 deselects integration/cache_lab/perf (and perf_tier_a) ✓
  • Every cited file:line (aliases.py:24/129/155, build.py:335/367/477-478, catalog_state.py:383-388/433, source_cache.py:133/152, server.py:219) is accurate ✓
  • All four zip-allowlist members (expr.py, xorq_build/, schema.json, manifest.json) are co-located at the entry-dir top level, and schema.json persists across the same-hash early-return ✓
  • All five existing tests slated for delete/rewrite exist at the cited locations ✓

Unusually rigorous plan. Findings are about implementation sequencing and a few concrete gaps, not the direction.

1. The 1a/1b split sits in tension with the TDD rule; two "failing-first" tests are really fix-induced breaks (High)

The plan splits failing-first into two commits. Stub symbols bundled with their tests (1b) are legitimate scaffolding. But test_prompt_history_survives_reset and test_two_entries_union_after_reset go red only because 1b pre-stages the .gitignore that ignores entries/*/ — on today's tree both pass (bullpen / tracked entry dir preserve the loose prompts.jsonl and expr.py across reset). Their lifecycle is green → red (when .gitignore lands) → green (when durable restore lands): not "test the existing bug then fix it," but "introduce the new storage model, which creates the regression, then add durability." The .gitignore is a behavior change, not an inert stub, and landing it in a failing-first commit risks turning existing green reset/bullpen tests red there too. Recommend deciding: (a) collapse 1a+1b into one failing-first commit but keep .gitignore out; or (b) treat the two .gitignore-dependent durability tests as fix-induced breaks and move them (with .gitignore) into commit 3.

2. The plan's own "real assertion, never collection/import error" bar isn't met by some 1b reds (Med)

test_two_entries_union_after_reset with a no-op write_recipe_zip + gitignored entries/*/ goes red because expr.py is gone (missing-file/import error mid-act), not the "Multiple backends found" RuntimeError that motivates it, and not a clean assert. Cheap fix: assert presence first (assert (entry_dir/"expr.py").exists() post-reset) so the red is an assertion.

3. assert_catalog_consistent's allowlist must include .gitignore and the full tracked set (Med)

The check is deny-by-default over git ls-files. The .gitignore introduced in commit 3 is tracked at the repo root but isn't in the plan's tracked-pattern enumeration — a strict allowlist would reject it and fail every checkpoint. Cover .gitignore, pyproject.toml, uv.lock, and each glob. Add a positive assertion (a normal checkpoint of every tracked kind passes), since test_unlisted_path_not_tracked only checks the negative.

4. Zip byte-stability depends on xorq_build/ determinism, not just pinned ZipInfo (Med)

test_recipe_zip_byte_stable_across_rebuilds leans on content_hash equality, which guarantees the hash is stable, not that every byte in xorq_build/ is identical run-to-run. An embedded non-project-root absolute path or unordered YAML dump churns the zip while the hash matches. Have the test diff the expanded xorq_build/ tree across two rebuilds so a non-deterministic build file is attributed to the builder, not the zip writer.

5. Minor

  • test_recipe_zip_members_are_allowlist must prefix-match xorq_build/** while exact-matching the three top-level files — not a literal 4-element set compare.
  • The plan says CI deselects "integration/cache_lab/perf"; addopts also deselects perf_tier_a (immaterial; enumeration is one short).
  • The .gitignore addition (.claude/, .claude-worktrees/) is unrelated to the plan doc — tiny and already committed, just noting the scope mix.

Bottom line

Technically sound, internally consistent, verified-accurate — approvable as a plan. The one thing worth settling before writing code is finding #1 (1a/1b structure + where .gitignore lives), since it collides with the project's TDD rule. #3 and #4 are concrete traps to bake into the commit-1 tests now rather than discover at step 7.

🤖 Generated with Claude Code

paddymul and others added 16 commits June 16, 2026 21:20
Plan for replacing xorq's catalog repo manager with a tallyman-native
content-addressed recipe store, and decomposing catalog.yaml into
git-tracked per-concern files (JSONL for list state, one JSON per nested
blob), now that dropping xorq removes assert_consistency's
extra-tracked-file constraint (#48).

Two cuts, one PR: the WRITE cut (native store + catalog.yaml decomposition,
checkpoint owns zip creation) and the RUNTIME cut (replace import xorq.api
with narrow catalog-free imports + a datafusion connect shim, since
import xorq.api transitively loads the whole xorq.catalog package).

Captures the investigation findings, resolved design decisions (D1-D4),
module boundary, risk register, and the ordered TDD implementation sequence.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses four findings from a code-investigation review of the plan:

- prompts.jsonl: it is append-mutable (grows on a re-run with no rebuild)
  AND runtime-read (entry-detail API -> React "N prompts seen"), yet the
  plan omitted it entirely, so it would be lost on reset/restore (entry
  dirs are gitignored; manifest keeps only the first prompt). Make it a
  seventh tracked decomposition file prompts/<hash>.jsonl, not a zip
  member. Wired through tracked tree, recipe-store exclusions, the
  decomposition section, D3, decomposition-scope, and steps 1/4.
- Cut B: state "8 runtime imports" and note the 9th grep hit
  (server.py:219) is the codegen-guidance docstring, retained per D4.
- Staging: git add -A is allow-by-default; assert_catalog_consistent (#52)
  now enforces the tracked surface as a path allowlist (deny-by-default).
  Risk row 1, module boundary, and steps 1/3 updated.
- Deterministic archive: pin ZIP_DEFLATED compresslevel.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ve-catalog plan

Folds five review findings into plans/native-catalog-store.md:

- F1: xorq.expr.api exports a connect, but it is ibis's connect(resource,
  **kwargs), not the no-arg datafusion xorq.api.connect — a naive swap fails
  at call time, not import. Correct Cut B prose + Risk #3.
- F2: relocate the notebook (project_dir/notebooks/) under catalog/ alongside
  post_processing/ and stats/; note chart_specs/ and display_configs/ already
  live under catalog_dir (no move).
- F3: strip aliases.py's module-level import xorq_catalog + the
  _xcat.add_alias/remove_alias calls in set_alias/remove_alias in the same
  commit that deletes xorq_catalog.py, else the package is un-importable.
- F5: xorq api.py catalog import is at line 62 (was cited :65). ~7.7k LOC is
  correct (non-test).
- F6: deterministic content-addressed zip is stored once by git; dropping the
  embedded wheel saves blob + working-tree size, not per-checkpoint churn.

F4 (catalog.yaml r/w + reset_to.materialize teardown) tracked as #105.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Adversarial verification of the prior fixes confirmed F1/F3/F5/F6 clean but
the consistency critic found the F2 notebook relocation was not propagated
from the Decomposition prose to the two places that enumerate it concretely:

- Implementation-sequence commit 4 said "move post_processing/+stats/ under
  the repo" — now also relocates the notebook (project_dir/notebooks/).
- Tracked-tree block annotated post_processing/stats as "(moved under the
  repo)" but not notebook.jsonl — now annotated.

Also fixes two cosmetic citations the verifiers caught: the 2nd xo.connect()
call is source_cache.py:152 (not :151), and the aliases.py import is spelled
`from tallyman_core import xorq_catalog as _xcat` (not a bare import).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The wheel + requirements.txt are xorq's per-expression reproducibility
mechanism (WheelPackager/PackagedRunner in ibis_yaml/packager.py: bundle a
built wheel + pinned requirements so `xorq run` re-executes an entry in its
own isolated uv env). The prior "nothing reads them" rationale undersold the
tradeoff.

tallyman doesn't use that path: it pins the env once (xorq==0.3.26 + uv.lock)
and runs every expression in one shared .venv via in-process build_expr
(build.py:317), never a per-expression uv env (the slow/expensive thing it
avoids). The in-process build output never held the wheel anyway — only the
catalog zip did, and nothing reads it back (fact 1). So the drop forfeits
xorq's per-entry env reconstruction (unused here), not tallyman's
reproducibility, which rides on the pinned project env.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… uv.lock

Decision: capture reproducibility at catalog-repo granularity, not per
expression. Track a single pyproject.toml + uv.lock in the catalog repo (the
reproducibility unit); rebuild = recreate one venv per project from the lock
and re-run the recipes. Per-expression venvs are deferred (the slow/expensive
xorq model we don't want yet).

- Tracked tree: add pyproject.toml + uv.lock.
- Wheel-drop bullet: reproducibility now rides on the tracked per-repo lock,
  not the ambient tallyman dev env.
- Scope boundary: disambiguate this from "no change to tallyman's own
  pyproject" (fact 5) — they are different files.
- Open/rebuild: env half settled (recreate venv from tracked lock); remaining
  open is orchestration + when the lock is snapshotted/refreshed.

Also records (chat finding, not yet in doc): xorq's decompiler is ibis's
generic one and is lossy for deferred reads (drops the path, emits a
non-compiling hyphenated identifier), so reconstructing recipes by decompiling
xorq_build/ is not viable — keep persisting expr.py.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…d scope

Records the runtime per-project venv change as an explicit deferred follow-on:
this plan settles the reproducibility record (tracked pyproject.toml + uv.lock)
only; building each project inside its own venv recreated from that lock is the
larger architecture change tracked in #106.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Settles the "why not regenerate expr.py from xorq_build/" question under D3
with evidence, so it is not re-proposed:

- The from_catalog chaining path re-execs expr.py so a cheap entry's recompute
  expression roots on the shared in-process backend; two entries union/join on
  one backend (#75). Reconstructing the way xorq's catalog does (load_expr per
  entry) gives each its own backend -> "Multiple backends found" (verified:
  two load_expr results won't union; two re-exec'd sources do). xorq never hits
  this because it runs one entry at a time and never composes.
- Both source-regeneration escape routes were tested and rejected: the ibis
  decompiler drops the read path (non-compiling source), and expr.unbind()
  composes but strips the path and can't execute without re-registering each
  source by name. Both reimplement, more fragilely, what the already-written
  expr.py (build.py:367) gives for free.

Conclusion unchanged: keep expr.py in the D3 allowlist.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ing state)

These are Claude Code's local session/tooling state (~1.6G) and agent
worktrees (~3.3G), neither of which belongs in the repo. Ignore them so they
never get swept into a commit.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Audit of the embedded TDD section found three load-bearing claims with no
named test (recipe-zip byte-stability, recipe-zip member allowlist, Cut-B
connect-shim determinism-equivalence) and D3's chaining-after-reset durability
untested in the gating suite. The failing-first commit also mixed assertion-red
tests with ones that would fail by collection error or false-green.

- Add named tests: test_recipe_zip_byte_stable_across_rebuilds,
  test_recipe_zip_members_are_allowlist, test_recipe_zip_contains_expr_py,
  test_connect_shim_snapshot_is_byte_identical, test_two_entries_union_after_reset
  (+ contrapositive), test_src_has_no_xorq_catalog_argv, test_display_config_survives_reset.
- Split commit 1 into 1a (assertion-red today) / 1b (lands catalog.py + .gitignore
  stubs in-commit); build-path no-subprocess guard asserts spy.call_count == 0,
  scoped past the cp -c clone.
- Step 2 now deletes test_catalog_xorq_integration.py in the same commit as
  xorq_catalog.py (module-level import would collection-error the suite).
- Step 6 reclassifies fix-induced breaks (delete vs re-target).
- Risk register rows 11-15; design callouts bind each invariant to its test;
  rebuild-path open item carries xorq's execution-equivalence lesson.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…zip (failing-first)

Commit 1a of the native-catalog-store cut (plans/native-catalog-store.md):
the failing-first tests that are assertion-red against today's xorq-subprocess
tree, using only existing public surfaces (build_and_persist, checkpoint_catalog,
git ls-files).

- test_uses_no_subprocess_alias: set_alias shells out to `xorq catalog
  add-alias` today (aliases.py:129, no try/except) — must not.
- test_uses_no_subprocess_build: a full build invokes `xorq catalog add`; the
  spy asserts call_count==0 (not "did not raise" — build.py swallows the
  registration exception into catalog_registered=False).
- test_zip_staged_not_committed_until_checkpoint (D1): xorq commits the recipe
  zip inline at build, outside the flock; the checkpoint must own it.
- test_failed_build_no_orphan_pointer (D1/Risk #7): a load_expr failure after
  target.mkdir leaves a partial entry dir → orphan pointer; the build must
  clean up.

Two planned 1a guards (n-builds-track-n-zips, build-dir-files-not-staged) were
verified GREEN today — the merged #48 fix already registers every entry in a
fresh project, and checkpoint uses an explicit `git add catalog.yaml` pathspec
(not `git add -A`). Per the TDD rule they ship with their fix commit, not here.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…(Cut A)

Stand up tallyman_core/catalog.py as the native recipe store and delete the
xorq catalog subprocess. The build now persists a complete entry dir (and
cleans it up on failure); the checkpoint, under the project lock, is the sole
zip writer and sole git transaction.

- catalog.py: write_recipe_zip (deterministic, allowlisted {expr.py, xorq_build/,
  schema.json, manifest.json} under one <hash>/ prefix), tracked_recipe_hashes,
  zip_pending_entries, assert_catalog_consistent (tracked-surface path allowlist
  + pointer/recipe agreement, #52), and the catalog-repo .gitignore.
- catalog_state.py: ensure_catalog_repo writes the .gitignore; checkpoint zips
  pending entries then `git add -A` (was `git add catalog.yaml`); reset_to
  delegates consistency to catalog.assert_catalog_consistent. Drops the inline
  _tracked_recipe_hashes/_assert_catalog_consistent/_TRACKED.
- build.py: drop the `xorq catalog add` subprocess; catalog_registered now means
  "entry dir persisted" (D2). rmtree the entry dir on a load/execute failure so
  a partial build leaves no orphan pointer (D1/Risk #7).
- aliases.py: strip the module-level xorq_catalog import and the add_alias/
  remove_alias subprocess calls (alias state stays in catalog.yaml for now;
  decomposition to aliases.jsonl is the next commit).
- Delete xorq_catalog.py. Delete the all-@integration test_catalog_xorq_integration.py
  (built on Catalog.from_kwargs / alias symlinks) and port its still-valid
  durability guards into test_native_catalog_store.py as fast in-process tests.

The four commit-1a failing-first tests now pass; added allowlist / byte-stable /
expr.py-member / no-xorq-argv / consistency / salt-naming / reset-durability
coverage. The catalog repo's first .gitignore also ignores the #104 migration
marker so `git add -A` stays honest. Full fast suite: 512 passed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Every piece of mutable catalog state is now its own git-tracked file the native
store allows, written by its own mutator and rolled back by reset_to's
`git reset --hard` directly — no catalog.yaml round-trip.

- aliases.py: alias map/history move from catalog.yaml keys to a tracked
  aliases.jsonl (one line per alias: {alias, latest, history}); aliases.py no
  longer reaches into catalog_state.
- notebook.py: from a single project_dir/notebooks/default.json object to a
  tracked catalog/notebook.jsonl (one cell per line).
- paths.py: post_processing/ and stats/ move under the catalog repo (were
  artifacts/ siblings); add prompts_dir/prompts_path; drop the vestigial
  notebooks/ from the project layout.
- build.py/app.py: per-entry prompt history moves from a loose (now gitignored)
  entries/<hash>/prompts.jsonl to a tracked prompts/<hash>.jsonl, so the re-run
  history survives a reset/clone. read_prompts/_append_prompt take
  (project, content_hash).
- catalog_state.py: catalog.yaml + capture/materialize of the six sections are
  gone. capture/read/write_tallyman_state shrink to the two pointer files
  (entries.jsonl + compute_cache.jsonl) for the untracked artifacts the bullpen
  reconciles; prune/restore read those; reset_to drops the materialize step.
- catalog.py: drop catalog.yaml from the tracked-surface allowlist.

Tests: delete the obsolete capture/materialize and catalog.yaml-coexistence
tests (machinery removed); update read_prompts call sites and the path-layout
assertions; add test_prompt_history_survives_reset + test_display_config_survives_reset
so net reset coverage doesn't regress. Full fast suite: 506 passed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace tallyman's own `import xorq.api as xo` runtime sites with narrow imports
that don't transitively load the xorq.catalog package: xorq.expr.api
(deferred_read_parquet / memtable / to_sql), xorq.ibis_yaml.compiler (build_expr),
xorq.caching (ParquetSnapshotCache).

- backend.py (new): catalog-free no-arg datafusion connect shim — Backend();
  do_connect(None), the exact body of xorq.api.connect. The naive narrowing
  xorq.expr.api.connect is ibis's connect(resource, **kwargs), a different
  function that would fail at call time or build the wrong backend (Risk #3).
- source_cache.py: ParquetSnapshotCache(source=connect()) via the shim.
- io.py / result_cache.py / build.py / app.py: narrow read/memtable/to_sql/
  build_expr imports.
- post_processing.py / summary_stats.py: narrow the xo.memtable validate/run
  sites; KEEP the full `import xorq.api` bind in _restricted_globals — that is
  the user-authored stat/post-processing exec surface (D4), like recipes.

Scope (D4 / Risk #2): the guarantee is that src/ imports no catalog package,
enforced by test_src_has_no_xorq_catalog_argv — NOT a sys.modules assertion. A
recipe's expr.py still does `import xorq.api as xo`, so re-execing one (build,
cached_result_expr) loads the catalog package; switching the recipe authoring
surface is a separate codegen-validated follow-up.

test_connect_shim_matches_xorq_api_connect pins the shim equivalence: the
shim-built and xorq.api.connect-built ParquetSnapshotCache derive the same
content-addressed snapshot key and bake the same data (raw parquet bytes aren't
stable — datafusion group-by order isn't deterministic). Full fast suite: 507 passed.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Non-functional cleanup of references the native store made obsolete:

- aliases.py / paths.py / catalog_state.py: rewrite the module docstrings and
  the project-layout diagram to the decomposed tree (aliases.jsonl,
  notebook.jsonl, prompts/, post_processing+stats under catalog/, the two
  pointer JSONLs) — no more catalog.yaml / xorq assert_consistency framing.
- paths.py: delete catalog_repo_path(), dead since the `xorq catalog --path`
  subprocess was removed.
- test_reset_to_revision.py: retarget test_catalog_repo_branch_is_main's
  docstring (branch pin is now for deterministic, host-config-independent
  layout, not xorq compat); the main-branch assertion stays.

The fix-induced test breaks themselves (capture/materialize round-trips,
catalog.yaml coexistence, read_prompts signature, path-layout assertions) were
already resolved with the commits that caused them. 98 affected tests pass.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…mented

- test_tracked_tree_is_the_decomposed_surface: build an entry, touch every
  decomposed section (alias, chart, display config, post-processing, stat,
  notebook, prompt), checkpoint, and assert `git ls-files` equals exactly the
  native decomposed surface — no catalog.yaml, no build dirs, no xorq
  metadata/symlinks. The whole-cut regression guard.
- plans/native-catalog-store.md: status → implemented, with the deviations from
  the plan as written (merged commits 2+3, the diff.py phantom Cut-B site, the
  full xorq.api bind kept for user-exec globals, key+data equivalence instead of
  byte-identical snapshots, the green-today 1a tests folded into fix commits, and
  the deferred corpus rebuild).

Verified: fast suite 508 passed; integration 6 passed; cache_lab 16 passed
(off mode) with the content-identity test passing under salt. A manual
genesis→build×2→checkpoint→reset roundtrip produces the expected tree and
rolls every section back.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@paddymul
paddymul force-pushed the plan/native-catalog-store branch from b441372 to 62fe9d0 Compare June 17, 2026 05:31
paddymul and others added 7 commits June 17, 2026 01:51
…first)

Four bug-hunt regressions, each seen failing before the fix:

- assert_catalog_consistent's allowlist uses fnmatch, whose * spans /, so a
  stray nested under an allowlisted dir prefix (post_processing/sub/leak.py,
  chart_specs/deep/leak.vl.json, prompts/_disabled/leak.jsonl, ...) slips past
  the deny-by-default guard. A companion test pins that the legitimate
  two-level *_disabled members must still pass.
- An interrupted atomic-write *.tmp (aliases._write / write_recipe_zip) is
  staged by the checkpoint's git add -A and durably wedges a later reset_to.
- A checkpoint over a manifest-less mid-build entry dir records a pointer with
  no durable recipe zip (capture_tallyman_state and zip_pending_entries
  disagree), producing a self-inconsistent step reset_to permanently rejects.
- cached_result_expr memoised the snapshot existence check, so an expensive
  entry's snapshot evicted after a warm read dangled on a stale
  deferred_read_parquet ("At least one path is required").

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- assert_catalog_consistent matched the allowlist with fnmatch, whose * spans
  /, so a stray nested under an allowlisted dir prefix passed the
  deny-by-default guard (and entries/sub/x.zip slipped past entries/*.zip). Add
  a path-segment-aware _surface_match (equal segment count + per-segment
  fnmatchcase) so * never crosses /.
- Add *.tmp to the repo .gitignore so an interrupted atomic-write temp
  (aliases._write, write_recipe_zip, the JSONL writers) is never staged by the
  checkpoint's git add -A and cannot wedge a later reset_to.
- capture_tallyman_state recorded every entry dir as a pointer, including a
  manifest-less mid-build dir that zip_pending_entries skips — committing a
  pointer with no durable recipe zip, a self-inconsistent step reset_to
  permanently rejects. Apply zip_pending_entries' manifest filter to capture so
  the two agree on the recorded set.

Updates two reset tests whose premises the manifest filter changes: the
back-then-forward fixture now gives its synthetic entry a manifest (real
entries always have one); the pointer-without-recipe defense-in-depth test now
injects the divergence directly, since capture no longer creates it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The whole function was @lru_cache-memoised, including the snapshot
existence-check + self-heal, so once an expensive entry was warm in the LRU,
evicting its baked snapshot left the cached deferred_read_parquet pointing at a
now-missing path — the next .execute() raised "At least one path is required".
The cold self-heal (after cache_clear) worked, masking the warm gap.

Split the expensive reconstruction (recipe import + rewrite_for_build +
snapshot-path resolution) into a memoised _resolve_result_plan, and make
cached_result_expr a thin non-memoised wrapper that re-checks path.exists() and
self-heals on every call. The perf-namespace cold-read tag still fires once per
genuine reconstruct; .cache_clear/.cache_info are re-exposed on the public name
so existing callers (companion app, conftest, cache tests) keep working.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… recipes

The plan's open item (#52, 'Rebuild + verify'): scripts/rebuild_native_catalog.py
re-executes each entry's expr.py into the native content-addressed store, the
single-user 'rebuild, don't migrate' path off the old xorq-catalog layout
(catalog.yaml + metadata/ sidecars + aliases.json).

Re-exec does not reproduce the old content hashes (the build pipeline + xorq
serialization have moved, and the hash is sensitive to the absolute source
path), so the rebuild treats hashes as not preserved: it topologically sorts the
from_catalog graph, re-points alias refs at rebuilt parents before their
children build, rewrites literal parent-hash refs old->new, expands the
persisted  placeholder back to the real path, and remaps
charts + reconstructs alias history with the new hashes. Reads either
aliases.jsonl (native) or aliases.json + alias_history.json (old), so it
round-trips a native catalog and upgrades an old one through one code path.
--dry-run prints the plan without writing.

Validated by a native round-trip test (a same-path rebuild is hash-stable, so it
asserts exact entry/alias-history/chart/prompt preservation) and end-to-end
against a real old-format project.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…arness

The native cut (#52) removed catalog.yaml / aliases.json / alias_history.json /
the metadata sidecars, so the perf integration harness's alias discovery
returned {} against a native corpus: _diff_pairs found no revision pairs and a
TALLYMAN_PERF_ENTRIES=<alias> subset always skipped the run. Read the native
aliases.jsonl first (latest + history per line), falling back to the legacy
files so an old corpus still runs, and link the native decomposed surface in the
overlay alongside the legacy names. Marker-gated (perf), local-only, never CI.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…m open item

Four in-scope regressions found by the post-implementation bug-hunt and fixed
test-first (allowlist nesting hole, interrupted-.tmp commit, manifest-less
pointer, warm-evict self-heal), the out-of-scope pre-existing findings filed for
separate decision, and the corpus-rebuild open item closed by
scripts/rebuild_native_catalog.py (the eight real projects remain the user's to
run).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…, in-process reuse

Stress-testing the rebuild against the real corpus (a disposable copy of the
15-entry parking_ticket_analysis_1m) surfaced three gaps, all fixed:

- Most real projects are catalog.yaml-era, not aliases.json-era: aliases live in
  alias_map/alias_history and charts/post_processing/stats/notebook are embedded
  in catalog.yaml with no decomposed files. read_old_catalog now reads each
  section from its decomposed file if present, else from catalog.yaml, and
  reconstructs the notebook (alias-keyed cells, no remap).
- A self-chaining revise (from_catalog of one's own alias, #74) made the topo
  sort see a false self-cycle. parse_deps now resolves a self-alias-ref to the
  PRIOR revision in history (the build-time parent), and the rebuild re-points an
  alias at each revision as it is rebuilt, oldest-first.
- An in-process rebuild left the process-global cached_result_expr memo populated
  with pre-wipe plans, so a from_catalog child re-execed against a stale parent.
  Clear the memo after wiping the catalog (a fresh-process CLI run is unaffected).

Documents the remaining build-time-alias-revision limitation: a recipe chains a
parent by alias with no version pin, so an alias revised after a child was built
(with a schema change) re-points to its latest revision and the child can miss a
column; pinning cross-alias refs to manifest.parents (#84) is the next step.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@paddymul

Copy link
Copy Markdown
Contributor Author

Tier-B perf integration run + catalog integrity audit

Ran the perf harness against the PR and audited the catalog's referential integrity (prompted by "what if a chart references an expression/alias that isn't in the catalog").

Perf: pytest -m perf tests/test_perf_integration.py

1 passed in 8.59s. Run against parking_ticket_analysis_1m (the post-#73 truncated corpus — the default parking_ticket_analysis still embeds per-entry result.parquet, so it's pre-#73 and won't self-heal). The harness's catalog.yaml-era alias discovery worked — it resolved all 9 aliases and built the diff pairs. The measurements split into two groups.

6 source-rooted entries — clean numbers:

entry rows exec cold exec warm cold stats warm stats 1st page
09d21553 1M 0.74s 0.60s 0.94s 0.10s 0.01s
4e90d0b9 1M 0.60s 0.59s 0.48s 0.09s 0.01s
7a3f2dac 1M 0.16s 0.16s 0.25s 0.04s 0.01s
a89f5e91 1M 0.60s 0.60s 0.49s 0.09s 0.01s
bd654ec5 1M 0.16s 0.15s 0.39s 0.05s 0.01s
cf52068f 1M 0.14s 0.13s 0.22s 0.04s 0.01s

Signal: warm stats are 5–10× faster than cold (e.g. 0.94→0.10s), with heal_s≈0.0 and load_s≈0.02s. The on-disk stat cache survives a fresh dataflow — the #34 empty-cache failure (warm≈cold) is not occurring on this branch.

9 from_catalog entries + all 4 diff pairs — errored, single root cause:

from_catalog('all_violations' | 'tickets_per_day' | 'habitual_speeder_stats' | 'camera_select_columns')
→ ProjectDataNotFound: catalog entry not found in project 'parking_ticket_analysis_1m'

Not a regression. from_catalog resolves an alias through get_alias → aliases.jsonl only (io.py:99, aliases.py:37); the 1m corpus is catalog.yaml-era with no aliases.jsonl (aliases live in catalog.yaml's alias_map). The harness discovers those aliases via the catalog.yaml fallback, but the native runtime can't resolve them at execute time. No native-format corpus exists on disk yet, so a fully-green from_catalog/diff run needs scripts/rebuild_native_catalog.py first (the "8 real projects still need rebuilding" item). The 6 source-rooted rows above are the real measurement this run produced.

Integrity audit: referential checks are partial and asymmetric

Recipe→parent refs fail loudly; chart/alias/display refs to a missing entry are unchecked and dangle silently. The perf errors above are a live demo of the loud half — from_catalog to a missing alias raises rather than corrupting state.

  • assert_catalog_consistent (catalog.py:182) is not referential integrity. It's a deny-by-default tracked-surface allowlist (rejects stray files git add -A would commit) plus pointer↔recipe agreement. It says nothing about whether a chart/alias target hash exists.
  • from_catalog(<hash|alias>) is checked at build time and every reconstruction (io.py:97-99) — a missing target raises ProjectDataNotFound. Loud, every time.

Chart → missing entry/alias (the original question):

  • At creation via MCP it's guarded — catalog_chart checks entry_dir(...).exists() then get_alias(...), returning {"error": "no entry for ..."} otherwise (server.py:689-692).
  • But core set_chart (charts.py:32) has no check — writes a .vl.json for any hash.
  • And no cleanup: remove_chart exists but has zero call sites in any entry-removal/prune path. Attach a chart to entry A, evict/prune A, and A's chart spec dangles silently in chart_specs/. get_chart still returns it (keyed on the chart file, not the entry); the browser falls through to "no chart."

Same silent-dangle gap on the other hash-keyed surfaces, none validating the target: aliases (latest/history, set_alias), display_configs (display_configs/<hash>.json), notebook cells (cell→alias). Post-processing and summary stats carry no entry refs (N/A). No test covers any dangling case.

Gap: there is no proactive integrity check — "scan the catalog, flag any chart/alias/display/notebook ref pointing at a missing entry." A catalog doctor pass (or extending assert_catalog_consistent with a referential check) would close it.

@paddymul

Copy link
Copy Markdown
Contributor Author

Code review — native tallyman_core.catalog (drop xorq catalog package)

Reviewed ea06845..40f09aa (28 files, +2383/−1002) via a multi-agent pass: cluster finders across the changed surface → adversarial verification of every finding (each reproduced or refuted against the code at 40f09aa) → completeness critic. Severities below are the verifier-adjusted values (several downgraded after reproduction).

Scope: findings for the manual rebuild CLI (scripts/rebuild_native_catalog.py) are tracked separately and excluded here. This covers the native store, checkpoint/reset, the build.py rework, the Cut-B import change, the catalog.yaml decomposition, companion/cache touch points, tests, and PR hygiene. No back-compat/migration concerns are raised (single-user repo).

Result: 14 confirmed (2 medium, 12 low) + 2 gaps; 1 dismissed false positive. No high-severity findings in scope.


TL;DR — worth addressing before merge

  1. [MEDIUM] The consistency guard is asymmetric (M1). assert_catalog_consistent validates only entries.jsonl ↔ entries/*.zip. Aliases (latest+history), charts, display-configs, and notebook hashes are never checked against the live recipe set, so a step whose alias/chart/display/notebook points at a non-existent hash passes silently and reset_to reports clean.
  2. [MEDIUM] Test-coverage regression on a carrier this PR changed (M2). Alias-state rollback-on-reset lost its only test in the deleted test_catalog_xorq_integration.py with no native replacement, even though this PR moves aliases into the standalone tracked aliases.jsonl.

Everything else is low: a consistent non-atomic-write pattern across several tracked-surface writers, a couple of stale comments/doc text, and the docs:-titled-but-actually-a-feat PR metadata.


MEDIUM

M1 — Consistency guard ignores aliases/charts/display/notebook · src/tallyman_core/catalog.py:182-212 · integrity · high → medium
assert_catalog_consistent enforces only (1) the TRACKED_SURFACE path allowlist and (2) entries/<hash>.zip set == entries.jsonl pointers. No cross-check that aliases.jsonl, chart_specs/<hash>.vl.json, display_configs/<hash>.json, or notebook hashes resolve to a durable recipe. reset_to (catalog_state.py:345) calls only this guard. Reproduced: committed set_alias("ghost","ffff…") + an orphan chart/display for that hash; guard passed, reset_to succeeded. Realistic triggers: a roll-back/prune leaving a sidecar, or a setter called with a no-entry hash (set_alias/set_chart/set_display_config do no existence check). Impact bounded — orphan chart/display are inert; a dangling alias-history hash surfaces as a loud build error on the self-chaining from_catalog path, not silent wrong rows.
Fix: extend the guard (or a sibling reset_to calls) to verify every aliases.jsonl latest+history hash, chart/display hash, and notebook hash resolves to a tracked recipe; raise listing orphans.

M2 — Lost coverage: alias-state rollback on reset has no native replacement · tests/test_reset_to_revision.py:178-195 (gap) · test-coverage · medium
The deleted test_catalog_xorq_integration.py had test_reset_rolls_back_alias_state (builds 2 entries, reset_to(1), asserts load_aliases == {"agg0"}). This PR moves aliases into the tracked aliases.jsonl (ec0320d) — exactly the carrier such a test guards — but no native test resets and asserts alias state. test_reset_roundtrip_over_every_kind covers pp/stats/charts/notebook and omits aliases; test_aliases.py has zero reset/checkpoint coverage. The PR's own test_display_config_survives_reset establishes the standard that each decomposed section gets a survives-reset test; aliases now occupies that gap.
Fix: add test_alias_state_survives_reset (two aliases across two steps, reset to step 1, assert only step-1; reset forward, assert both).


LOW

L1 — assert_catalog_consistent never detects orphaned chart_specs/display_configs · catalog.py:182-212 · integrity · medium → low
Guard 2 checks only the recipe-zip set; chart_specs/display_configs/prompts are validated for path shape only, never live-entry membership. remove_chart is only re-exported (never called by prune/reset); remove_display_config is never called anywhere — nothing GCs these. Narrower surface of M1. MCP catalog_chart does gate writes to live entries, so the realistic vector is roll-back/prune. Fix: the M1 guard, or document + test the tolerated-orphan stance.

L2 — Chart / display-config writers are non-atomic · catalog.py:81 (comment) + charts.py:47, display_configs.py:29 · integrity · low
The GITIGNORE_LINES comment claims *.tmp covers atomic writers, but set_chart/set_display_config (and notebook._save) do bare write_text. A crash mid-write leaves a truncated file the next git add -A commits; get_chart/get_display_config do unguarded json.loads. Fix: tmp + Path.replace, matching aliases._write.

L3 — Build's completeness sentinel manifest.json is written non-atomically · manifest.py:58-61 · concurrency · medium → low
The "manifest = complete entry dir" design gates on .is_file(), but write_manifest is a plain write_text — .is_file()-true the instant it's truncated. build_and_persist holds no lock; the companion (separate process) checkpoints on a threadpool while the MCP server is mid-build, so a checkpoint can zip a truncated manifest member. Currently latent — nothing reads the zip's manifest member back, and the capture path never json.loads it; violates the byte-identical-rebuild promise but doesn't break reads today. Fix: atomic write_manifest (tmp + replace); consider the same for schema.json at build.py:479.

L4 — Orphan entry-dir cleanup covers only the load_expr/execute exception paths · build.py:423-536 · correctness · low
shutil.rmtree(target) runs on exactly two failure paths (453, 468). The xorq_build copy loop, make_portable_inplace, and the expr.py/schema.json/manifest writes have no cleanup — an exception there leaves a partial entries/<hash>/ dir. Durability is preserved (the manifest filter skips incomplete dirs), so it's gitignored junk, but the D1 "no partial dirs" invariant is only partially realized and the no-orphan test exercises only the path that already cleans up. Fix: wrap the whole entry-dir population in a try/except that rmtrees when this call created the dir (or drop the ad-hoc rmtrees and document the manifest filter as the sole guard); add a test for a non-load_expr failure site.

L5 — notebook._save rewrites notebook.jsonl non-atomically · notebook.py:50-58 · integrity · medium → low
The one JSONL list-rewriter in the decomposition that isn't atomic; a torn write can be committed by git add -A. Bounded — cells are one-per-line and load() skips bad lines, so worst case is dropping the trailing cell(s), and it's git-recoverable. Fix: mirror aliases._write (tmp + replace), keeping the empty-notebook unlink branch.

L6 — notebook.load silently drops malformed JSONL lines · notebook.py:40-47 · integrity · medium → low
On JSONDecodeError, load() does continue; the next _save() makes the loss permanent. Inconsistent with aliases._read/read_prompts, which raise loudly. (The PR actually reduced blast radius vs the old loader, which dropped the whole notebook on one bad line.) Fix: let json.loads raise, or log + preserve the raw line so _save round-trips it.

L7 — Orphan chart_specs/display_configs/prompts files caught by neither guard, no test · catalog.py:182-212 · test-coverage · low
Test-coverage twin of L1/M1. test_nested_stray_under_allowlisted_prefix_is_rejected covers strays nested under the prefixes, not top-level orphan-hash files. By-design lenience (sidecars roll back via git reset, read by hash so inert) — the gap is that it's incidental, not pinned. Fix: decide intent — reject (guard + failing test) or allow (test documenting an orphan-hash sidecar passes).

L8 — assert_catalog_consistent's orphan-recipe branch (tracked - pointers) is untested · catalog.py:205-212 · test-coverage · medium → low
Guard 2 raises in both directions, but only missing = pointers - tracked is tested. No test stages a tracked zip absent from pointers, so the orphan branch never executes — a refactor dropping that half passes green. Defense-in-depth backstop (manifest filter prevents the divergence arising). Fix: commit an orphan zip and assert the guard raises matching "tracked recipes with no pointer".

L9 — api_delete_result_cache comment + cache_clear() left stale by the ee0a90a self-heal fix · app.py:1102-1109 · convention · low
ee0a90a made cached_result_expr re-check path.exists() and self-heal on every read. The endpoint's comment still claims cache_clear() is what prevents a dangling deref — now false (proven by test_cached_result_expr_self_heals_after_warm_then_evict, which self-heals with no cache_clear). Same stale rationale in tests/test_cache_inspector.py:133-137. Fix: update both comments.

L10 — PR title (docs:) and body ("design plan (no code)") contradict the contents · PR metadata · pr-hygiene · medium → low
The PR ships a multi-thousand-line implementation (deletes xorq_catalog.py, adds the native store, Cut B, the decomposition, new tests) and the plan doc says Status: implemented, yet the title is docs: and the body opens "A design plan (no code)…". Fix: retitle to feat:/refactor: and drop the "design plan (no code)" framing (the detailed sections can stay).

L11 — Plan doc "Open / not yet grilled" section is stale · plans/native-catalog-store.md:592-608 · convention · low
Still lists "the exact .gitignore contents" as open, but the catalog .gitignore is now concrete (GITIGNORE_LINES, catalog.py:72-82, written by write_gitignore). Fix: drop the .gitignore item; narrow what's genuinely open to the build-time-alias-revision limit (#84).

L12 — Risk register / Cut B cite a connect-shim test name that doesn't match · plans/native-catalog-store.md:437, 509 · convention · low
The doc names test_connect_shim_snapshot_is_byte_identical ("byte-identical"), but the shipped test is test_connect_shim_matches_xorq_api_connect and asserts key+data equivalence, not byte equality (datafusion group-by order isn't deterministic). The deviation is disclosed in "Implementation notes" but the risk-register rows weren't updated. Fix: update the name + wording.


Additional gaps (low)

C1 — post_processing/stats writers are non-atomic too · post_processing.py:200, summary_stats.py:183 · integrity. Same plain-write_text hazard as L2 on the tracked *.py surfaces. Fix: one shared atomic-write helper for all of these.

C2 — No test for a checkpoint racing the build's out-of-lock prompt append · build.py:409, 537 · concurrency. _append_prompt writes tracked prompts/<hash>.jsonl outside the project lock (only the checkpoint holds it); the re-run path appends while a dispatch-boundary checkpoint runs git add -A. Same class as L3, untested. Fix: atomic append, or a concurrency test; at minimum document the ordering requirement.


Dismissed (false positive)

test_api_projects_new_creates_and_switches asserts only dirs, not git/genesis init — genesis is covered by test_genesis_on_companion_project_new (tests/test_reset_to_revision.py:400-406), which POSTs the same route and asserts step-000 is tagged. The new behavior is tested.


Cross-cutting themes

  1. The integrity guard is the recurring weak point. M1/L1/L7/L8 all trace to assert_catalog_consistent checking only recipe↔pointer. One third guard (every aliases/chart/display/notebook hash resolves to a tracked recipe) closes M1 and L1 at once; L7/L8 become the tests that pin it.
  2. Non-atomic writes are a consistent pattern. L2/L3/L5 + C1 + C2 are the same gap on several writers (manifest, notebook, charts, display, pp, stats, prompts) — only aliases._write and write_recipe_zip got tmp+replace in the hardening pass. A shared atomic_write(path, body) helper across the tracked-surface writers resolves all of them and matches the convention this PR already established.

🤖 Generated with Claude Code — multi-agent review; every confirmed finding independently reproduced or refuted against the code at 40f09aa.

Bundles the failing-first tests for the verifiable behavioral findings from
the PR #103 review, ahead of their fixes:

- M1/L1 — assert_catalog_consistent must reject decomposed sidecars whose
  content hash names no durable recipe: a dangling alias latest hash, a
  dangling alias *history* hash, an orphan chart hash, and an orphan
  display-config hash. The guard currently validates only entries.jsonl ↔
  entries/*.zip, so all four DID NOT RAISE.
- L4 — a build that fails after the entry dir is populated but past the
  load_expr/execute paths (here make_portable_inplace) must leave no orphan
  entry dir. Only the two ad-hoc rmtree paths clean up today, so the dir
  lingers. (Drops the misleading monkeypatch.undo() that the existing
  no-orphan test also carries — undo() reverts isolated_home's TALLYMAN_HOME
  setenv on the shared monkeypatch, pointing entries_dir at the real home and
  making the check vacuous.)
- L6 — notebook.load must fail loudly on a malformed JSONL line (matching
  aliases._read / read_prompts) rather than silently dropping it.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@paddymul

Copy link
Copy Markdown
Contributor Author

Addendum from the pack01 sonnet run on this branch — one in-scope build-path finding the review missed

Reran pack01 verbatim against this PR (40f09aa, sonnet driver, isolated home; notes in tallyman-prompt-packs/runs/2026-06-17-pack01-sonnet/). One build.py correctness issue surfaced that the review above doesn't cover — its build.py findings are L3 (manifest atomicity) and L4 (orphan-dir cleanup on exception paths); this is neither.

[MEDIUM — arguably high] A cheap build never exercises the row-level projection — a recipe that throws on every read is committed and aliased as a successful build · src/tallyman_xorq/build.py:463-471 · correctness/integrity

For a cheap entry (no Aggregate/Join/Sort/window/UDF) the build's only execution is:

arrow_schema = loaded.schema().to_pyarrow()   # logical schema, no data eval
row_count = int(loaded.count().execute())     # pushdown count, prunes the row projection

DataFusion satisfies count() from source/parquet metadata without evaluating the projection, and schema() is logical, so a row-level evaluation error in the recipe (a failing cast, arithmetic, a UDF) is never exercised at build. The surrounding try/except (build.py:467) only catches what count/schema themselves raise. A worthy entry bakes a full result and would catch it; a cheap mutate escapes.

Result: the entry is persisted, registered in entries.jsonl, and aliased, with a manifest row_count and a successful tool response (is_error:false). Every later read throws. The author gets no signal at author time.

Reproduced (pack01 T1 — a mixed-format date column parsed with a plain .cast("date")):

camera v2 (e69c6232a7f9): build OK, row_count 23,925,000, execute_seconds 0.013, cache_worthy=false ("cheap")
full scan: ValueError: Arrow error: Cast error: Cannot cast string '2020-20-07' to value of Date32 type   (0.8s)

The driver's T1 transcript shows 10 tallyman calls and 0 in-band errors — it reported "camera is now date32" and moved on. The error only surfaced at T2, when the first worthy build reading camera forced materialization (registry error_id 67ab20a581ef).

Impact — bounded but worse than a build failure: the broken entry is committed + aliased and invisible at author time, and it sits at the head of an alias that downstream from_catalog recipes chain on. It surfaces loudly on the first materializing read (viewer, a worthy downstream build, diff, marimo export), so it is silent-broken-state rather than silent-wrong-rows. The pre-native build did a full to_parquet and rejected the same cast at author time with nothing persisted — this PR's cheap path trades that validation for the metadata-only count.

Fix options (the tension: the cheap path exists precisely to avoid writing a parquet):

  • Exercise the projection at build without persisting — a streaming execute() that discards rows, or a temp to_parquet that's dropped — restoring author-time validation at the cost of the cheap-path scan. Cleanest for correctness.
  • Or classify a projection containing fallible row ops (cast / arithmetic / UDF) as worthy so it materializes; leave pure rename/select mutates on the cheap path.
  • A bounded .limit(N).execute() probe is not sufficient — the offending row is rare and won't be in the first N.

Same run also produced F2 (stacked window functions fail to plan; the WindowShift error misleads authors into thinking lag is unsupported) → tallyman-side hint filed as #107, and F3 (the resulting row_number()+self-join over-correction silently mis-orders tied dates, 0.92% of sprees) — F3 is a model-authored-recipe wrong-data consequence prevented by the #107 hint, not a separate defect in this PR, so it's not filed standalone. Details for both in the run notes.

Resolves the confirmed findings from the PR #103 review.

Integrity guard (M1/L1): assert_catalog_consistent gains a third guard — every
decomposed sidecar keyed by content hash (aliases.jsonl latest+history,
chart_specs/<hash>, display_configs/<hash>) must name a hash in the durable
recipe set, so a committed step with a dangling alias/chart/display ref fails
loudly instead of resetting clean. Charts/display are read from `git ls-files`
(the committed tree) so a transient untracked sidecar isn't flagged. Notebook
cells key on alias name, not hash, so they stay out of scope.

Atomic writes (L2/L3/L5/C1/C2): new fsutil.atomic_write_text (tmp + os.replace)
routes every tracked-surface writer — charts, display configs, notebook._save,
manifest, post_processing/stats sources, the build's schema.json, the prompt
append, and aliases._write (consolidated onto the shared helper). A crash or a
cross-process checkpoint `git add -A` can no longer stage a torn file.

Build cleanup (L4): the entry-dir population is wrapped so any failure past
target.mkdir removes the partial dir, not just the load_expr/execute paths that
had ad-hoc rmtrees (now dropped). The target.exists() early-return guarantees we
created the dir, so cleanup can't clobber a prior entry.

notebook.load (L6): malformed JSONL now raises (matching aliases._read /
read_prompts) rather than silently dropping the line; with atomic _save a torn
line is genuine corruption worth surfacing.

Stale comments (L9): the result-cache delete endpoint and its test no longer
claim cache_clear() averts the dangling-deref 500 — since ee0a90a
cached_result_expr self-heals on every read; cache_clear is defense-in-depth.

Coverage (M2/L8): test_alias_state_survives_reset (alias map+history roll back
with git reset) and test_consistency_guard_surfaces_orphan_recipe (guard 2's
orphan branch). Dropped the misleading monkeypatch.undo() from the no-orphan
test — it reverted isolated_home's TALLYMAN_HOME and made the check vacuous.

Plan doc (L11/L12): drop the settled .gitignore open item; fix the connect-shim
test name/wording (test_connect_shim_matches_xorq_api_connect, key+data
equivalence, not the never-shipped *_byte_identical).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@paddymul paddymul changed the title docs: plan native tallyman_core.catalog (drop xorq catalog package) feat: native tallyman_core.catalog store (drop xorq catalog package) Jun 17, 2026
@paddymul

Copy link
Copy Markdown
Contributor Author

Review findings addressed

Implemented following the repo's TDD rule: failing tests first in b7acc10 (CI red), fixes in e4e5527 (CI green). Per-finding trail below.

Medium

  • M1 — asymmetric consistency guard → e4e5527. assert_catalog_consistent gains a third guard: every decomposed sidecar keyed by content hash (aliases.jsonl latest+history, chart_specs/<hash>, display_configs/<hash>) must resolve to a tracked recipe; otherwise it raises listing the orphans by surface. Pinned by test_consistency_guard_surfaces_dangling_alias_hash / ..._history_hash (b7acc10). One deliberate scope cut: notebook cells key on alias name, not hash, so they're out of scope — enforcing notebook→alias existence would break the legitimate "cell for an alias not yet created" pattern (test_reset_roundtrip_over_every_kind).
  • M2 — lost alias-rollback coverage → e4e5527. Added test_alias_state_survives_reset (two aliases across two steps, reset back asserts only step-1 state + history, reset forward asserts both), mirroring the display-config/prompt survives-reset tests.

Low

  • L1 — orphan chart/display not detected → e4e5527. Same third guard covers chart + display hashes. I took the reject option (over document-and-allow). Charts/display are read from git ls-files (the committed tree) so a transient untracked sidecar isn't false-flagged. Pinned by test_consistency_guard_surfaces_orphan_chart_hash / ..._display_config_hash (b7acc10).
  • L2 / L3 / L5 / C1 — non-atomic writers → e4e5527. New tallyman_core/fsutil.atomic_write_text (tmp + os.replace); routed charts, display configs, notebook._save, write_manifest, the build's schema.json, post-processing/stats sources, and consolidated aliases._write onto it. The GITIGNORE_LINES comment now references the shared helper.
  • L4 — partial entry-dir cleanup → e4e5527. The entry-dir population is wrapped in one try/except that rmtrees on any failure past target.mkdir; the two ad-hoc rmtrees are dropped. The target.exists() early-return guarantees we created the dir, so cleanup can't clobber a prior entry. New test_failed_build_after_copy_leaves_no_orphan_dir (b7acc10) induces a failure at make_portable_inplace. While here I also fixed a latent bug in the existing no-orphan test: its monkeypatch.undo() reverted isolated_home's TALLYMAN_HOME setenv (shared instance), pointing entries_dir at the real home and making the check vacuous — removed from both.
  • L6 — silent malformed-line drop → e4e5527. notebook.load now lets json.loads raise (matching aliases._read / read_prompts); with the now-atomic _save, a torn line is genuine corruption worth surfacing. Pinned by test_load_raises_on_malformed_line (b7acc10).
  • L7 — orphan sidecar intent unpinned → e4e5527. Resolved as part of L1 (reject + tests pin it).
  • L8 — untested orphan-recipe branch → e4e5527. Added test_consistency_guard_surfaces_orphan_recipe.
  • L9 — stale cache_clear rationale → e4e5527. Updated the delete-endpoint comment and test_cache_inspector.py comment: since ee0a90a, cached_result_expr self-heals on every read, so cache_clear() is defense-in-depth, not what averts the 500. Kept the call (cheap).
  • L10 — PR title/body → retitled to feat: and the body now describes the shipped change (dropped the "design plan (no code)" framing).
  • L11 — stale .gitignore open item → e4e5527. Dropped from the plan's open list (it's now concrete in GITIGNORE_LINES); narrowed to the genuinely-open catalog: restore the cross-entry DAG by recording resolved from_catalog parents in the manifest ({hash, ref, follow}) — catalog_parents went dark under #74 #84 limitation.
  • L12 — wrong connect-shim test name → e4e5527. Risk register / Cut-B / sequence rows now name test_connect_shim_matches_xorq_api_connect and say "key + data equivalence," not the never-shipped *_byte_identical.

Additional gaps

  • C1 — folded into the atomic-write pass above.
  • C2 — out-of-lock prompt append race → e4e5527. _append_prompt is now atomic read-modify-write (tmp + replace) so the file is always whole JSONL. Pinned by test_append_prompt_is_atomic_and_complete.

Dismissed

  • The test_api_projects_new_creates_and_switches genesis-coverage false positive — agreed, no action.

Full suite green locally (531 fast + 6 integration) and on CI.

paddymul and others added 2 commits June 17, 2026 12:37
A cheap entry (pure projection — no Aggregate/Join/Sort/window/UDF) only ran
schema() + count() at build. DataFusion satisfies count() from source metadata
and prunes the row projection, so a failing row-level op (cast / arithmetic /
UDF) never evaluated at build: the broken entry committed + aliased as a
successful build and threw only on the first materialising read — silent broken
state at the head of an alias chain (pack01 finding, PR #103 comment).

test_cheap_build_validates_row_projection builds a cheap mutate with an
out-of-range date cast and asserts the build raises. Red today: the build does
not raise (DID NOT RAISE BuildError). Also pins the valid-cast positive case so
the streaming row-count path is exercised, not just the error path.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
pack01 finding (PR #103 comment): a cheap entry (no Aggregate/Join/Sort/window/
UDF) only ran schema() + count() at build, and DataFusion satisfies count() from
source metadata — pruning the row projection — so a failing cast/arithmetic/UDF
never evaluated at author time. The broken entry committed + aliased as a
successful build and threw only on the first materialising read.

build.py now classifies before executing and, for the cheap path, streams the
full result via to_pyarrow_batches() and discards it — forcing row-level
evaluation at build (constant memory; the one pass also yields the exact row
count). A worthy entry is unchanged: baking its result-cache snapshot already
evaluates every row, so count() there both validates and counts. Pinned by
test_cheap_build_validates_row_projection (4f7f78b), with a valid-cast positive
case covering the streaming row-count path.

Also fixes three review doc-nits:
- backend.py cited a nonexistent test (test_connect_shim_snapshot_is_byte_
  identical) and the wrong "byte-identical" claim; now names the shipped
  test_connect_shim_matches_xorq_api_connect (key + data equivalence).
- build.py's failed-build cleanup comment overclaimed "we created this dir";
  reworded — created_target is also set for a manifest-less mid-build leftover,
  which is still safe to rmtree (never a complete, durable entry).
- assert_catalog_consistent guard-3 docstring omitted the hash-keyed
  prompts/<hash>.jsonl; now states why it (append-only, inert when orphaned) and
  the notebook are out of scope.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@paddymul

Copy link
Copy Markdown
Contributor Author

pack01 cheap-build finding + three doc-nits addressed

Followed the repo TDD cadence: failing test in 4f7f78b (CI red on test_cheap_build_validates_row_projection — DID NOT RAISE BuildError, 1 failed / 531 passed), fix in 8e686f8 (CI green — 532 fast + 6 integration).

[MEDIUM] Cheap build never exercised the row-level projection → 8e686f8

Confirmed the reproduction with a minimal in-process case: a cheap t.mutate(dd=t.d.cast("date")) over a parquet with one out-of-range date (2020-20-07) built successfully — count() is satisfied from source metadata and prunes the projection, so the cast never evaluated at author time.

The fix splits the build's execution by the structural verdict, which is now computed before execute (classify_build reads serialized YAML, no eval):

  • worthy entry (Aggregate/Join/Sort/window/UDF): unchanged — baking its result-cache snapshot already evaluates every row, so count().execute() both validates and counts.
  • cheap entry: streams the full result via loaded.to_pyarrow_batches() and discards it, forcing row-level evaluation at build. Constant memory, and the one pass also yields the exact row_count (no separate count()).

This restores the author-time validation the pre-native full-to_parquet build had, at the cost of a scan on cheap builds — the tradeoff the finding favored (option 1). The .limit(N) probe was rejected for the reason given (rare offending row). test_cheap_build_validates_row_projection pins both the error path and a valid-cast positive case (so the streaming row-count is covered, not just the raise).

Note on the same-run items: F2/F3 are tracked separately (#107 + run notes) and aren't changes to this PR, so they're out of scope here.

Doc-nits from the review pass → 8e686f8

  • backend.py cited a test that doesn't exist (test_connect_shim_snapshot_is_byte_identical) and the wrong "byte-identical" claim. Now names the shipped test_connect_shim_matches_xorq_api_connect and says key + data equivalence (datafusion group-by order isn't deterministic). This was the source-code survivor of the earlier L12 (the plan doc was already corrected).
  • build.py failed-build cleanup comment overclaimed "the target.exists() early-return guarantees we created this dir." Reworded: created_target is also set for a manifest-less mid-build leftover dir, which is still safe to rmtree (never a complete, durable entry — those early-return before the flag is set).
  • catalog.py guard-3 docstring said "every decomposed file keyed by content hash" but silently omitted the hash-keyed prompts/<hash>.jsonl. Now states why it (append-only provenance, inert when orphaned) and the notebook (keyed by alias name) are out of scope.

🤖 Generated with Claude Code

@paddymul
paddymul merged commit dddf4e1 into main Jun 17, 2026
4 checks passed
paddymul added a commit that referenced this pull request Jun 17, 2026
PR #103 (native tallyman_core.catalog store) and PR #104 (removed the
on-demand result.parquet layer) merged to main; merge main in and update
the reactive plan set to match the current tree.

- adr-reactive-catalog-recalc.md: add dedicated "Reconciliation with PR
  #104" and "PR #103" sections; correct the deleted ensure_result /
  result.parquet-survives, salt-mode, and catalog_parents-regex claims;
  re-point io.py / result_cache.py / lineage.py / aliases.py citations.
- staged-catalog-perf-reactive.md: Stage 1 bug cluster dissolved-by-
  construction; rebuild script OPEN -> PARTLY DONE; disk-usage/inspector
  follow-ups closed; split the parents-DAG prerequisite (capture done,
  reactive consumer remains); refresh the dependency diagram.
- adr-result-cache-cost-rubric.md / adr-source-identity-content-hash.md:
  strike ensure_result, fix the now-false "from_catalog reads
  result.parquet" claim, repoint drifted line numbers.

Attribution corrected after git archaeology: the manifest `parents` edge
+ parent_capture (#84) shipped via PR #91, and the cache-admission
instrumentation + compute_cache disk counting (#87) via PR #94 -- both
ahead of #103, not part of it. #103 only makes the parents edge durable
across reset (manifest.json in the recipe-zip member set).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
paddymul added a commit that referenced this pull request Jun 18, 2026
…corpus decision

Verified the #103/#104 reconciliation against the merged tree (all code
citations accurate bar one drift, fixed: execute_seconds is manifest.py:38,
not :47-50) and brought the plan current with what shipped since:

- Deliverable 1: prose lifecycle model + refreshed cache map already shipped as
  docs/expression-lifecycle.md + docs/caching.md (PR #47); fix the stale
  "docs/caching-architecture branch" reference.
- Deliverable 2: record #59/#64 closed and both perf tiers shipped; remaining =
  regression-gate promotion (#45), #82 deep-chain measurement (gates Stage 3).
- Stage 3: determinism lint shipped (#88) -> mark PARTIAL not open; note the
  #80/#96 dangling-path failure is mitigated by the per-call path.exists
  self-heal; name epic #89; add the recompute-substrate holes the consumer
  inherits (#75/#76/#77/#97/#79/#81).

Corpus: per the project no-migration rule, build the stage-2 corpus FRESH in
native format from the source parquets, not by converting the pre-#103 on-disk
catalogs. rebuild_native_catalog.py is a migration convenience, not on the
critical path; its alias-revision pinning limit is out of scope.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
paddymul added a commit that referenced this pull request Jun 18, 2026
PR #103 (native tallyman_core.catalog store) and PR #104 (removed the
on-demand result.parquet layer) merged to main; merge main in and update
the reactive plan set to match the current tree.

- adr-reactive-catalog-recalc.md: add dedicated "Reconciliation with PR
  #104" and "PR #103" sections; correct the deleted ensure_result /
  result.parquet-survives, salt-mode, and catalog_parents-regex claims;
  re-point io.py / result_cache.py / lineage.py / aliases.py citations.
- staged-catalog-perf-reactive.md: Stage 1 bug cluster dissolved-by-
  construction; rebuild script OPEN -> PARTLY DONE; disk-usage/inspector
  follow-ups closed; split the parents-DAG prerequisite (capture done,
  reactive consumer remains); refresh the dependency diagram.
- adr-result-cache-cost-rubric.md / adr-source-identity-content-hash.md:
  strike ensure_result, fix the now-false "from_catalog reads
  result.parquet" claim, repoint drifted line numbers.

Attribution corrected after git archaeology: the manifest `parents` edge
+ parent_capture (#84) shipped via PR #91, and the cache-admission
instrumentation + compute_cache disk counting (#87) via PR #94 -- both
ahead of #103, not part of it. #103 only makes the parents edge durable
across reset (manifest.json in the recipe-zip member set).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
paddymul added a commit that referenced this pull request Jun 18, 2026
…corpus decision

Verified the #103/#104 reconciliation against the merged tree (all code
citations accurate bar one drift, fixed: execute_seconds is manifest.py:38,
not :47-50) and brought the plan current with what shipped since:

- Deliverable 1: prose lifecycle model + refreshed cache map already shipped as
  docs/expression-lifecycle.md + docs/caching.md (PR #47); fix the stale
  "docs/caching-architecture branch" reference.
- Deliverable 2: record #59/#64 closed and both perf tiers shipped; remaining =
  regression-gate promotion (#45), #82 deep-chain measurement (gates Stage 3).
- Stage 3: determinism lint shipped (#88) -> mark PARTIAL not open; note the
  #80/#96 dangling-path failure is mitigated by the per-call path.exists
  self-heal; name epic #89; add the recompute-substrate holes the consumer
  inherits (#75/#76/#77/#97/#79/#81).

Corpus: per the project no-migration rule, build the stage-2 corpus FRESH in
native format from the source parquets, not by converting the pre-#103 on-disk
catalogs. rebuild_native_catalog.py is a migration convenience, not on the
critical path; its alias-revision pinning limit is out of scope.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant