fix(knowledge): LayeredKnowledgeStore.list_chunks returns tier-tagged Chunk rows

[mabry1985]

Root cause

LayeredKnowledgeStore.list_chunks() returned tier-tagged dicts while every other store's list_chunks() yields Chunk objects — a Liskov break in the drop-in the runtime actually uses whenever a commons is configured (server/agent_init.py:541). Every consumer reads rows by attribute, so all of them broke on a layered store:

Consumer	What happened on a layered store
tools/lg_tools.py memory_list (c.domain …)	AttributeError — the tool crashed
graph/memory_facts.py:141-144 consolidate_and_store (c.id, c.content inside try/except)	dedup/supersede silently degraded to add-only; duplicate facts accumulated
graph/snapshot_op.py:779-790 collect_knowledge_seed (getattr(chunk, "content", ""))	every chunk read as "" → the snapshot knowledge seed exported empty

Found in the D4 (#3242) review. Refs #3184.

The snapshot seed had a second cause for the same empty export: with no explicit domains= (both live callers — operator_api/snapshot_routes.py:178, graph/snapshot_cli.py:258), collect_knowledge_seed discovers domains from store.stats() keys, and the layered stats() returned only tier counts (private / commons / total) — so it "discovered" private and commons as domains, listed nothing, and exported empty even with correct rows. The same shape made memory_stats and the Store view's stats show tiers instead of domains on a layered store.

Fix

• Chunk gains a non-column tier: str | None = None field (last, so positional construction elsewhere is unaffected); as_dict() carries "tier" so the console's tier badges keep working. Chunk.from_row() (D4) already ignores unknown keys, and _CHUNK_FIELDS picks the field up automatically.
• LayeredKnowledgeStore.list_chunks() stamps rows with dataclasses.replace(c, tier=…) — the backend's own row type, private first, ids per-backend as before. A custom KnowledgeBackend that yields dicts still gets a "tier" key (_tag()), so the layered store stays a drop-in for that shape too.
• LayeredKnowledgeStore.stats() now returns per-domain counts merged across both tiers plus the split: {<domain>: n, …, "total": N, "private": P, "commons": C}. Readers of the split keys keep working; readers that treat every non-count key as a domain (memory_stats, the seed's discovery, the Store view) see real domains. A domain literally named private/commons/total is shadowed by the split keys (documented).
• graph/snapshot_op.collect_knowledge_seed skips _NON_DOMAIN_STAT_KEYS = {"total", "private", "commons"} during discovery (was "total" only), kept in step with knowledge.layered._SPLIT_KEYS.
• knowledge/backend.py Protocol and LayeredKnowledgeStore.list_chunks: -> list[Chunk] | list[dict] with a one-line note (the annotation said list[dict] while every built-in store has always returned Chunk rows); the one protocol-sketch line in ADR 0031 updated to the same union, nothing else in the ADR.
• _tag() hardening: a dataclass row without a tier field (a backend's own row type — unreachable today, allowed by the Protocol) degrades to as_dict() + key instead of a TypeError from replace.
• memory_stats renders the layered split as tier private: P / tier commons: C so the model doesn't read them as domains it could memory_list; domain lines unchanged.

Design decision

The seed exports the private tier only. A snapshot is the agent's own portable knowledge (ADR 0091); the commons is host-shared curated knowledge that exists on the destination fleet independently — exporting it would make the console route (private ∪ commons) disagree with the unbooted CLI path (private only) and let commons rows eat the 2 MB cap. collect_knowledge_seed skips tier == "commons" rows; a commons-only domain is discovered but has no private rows, so it is not exported.

• operator_api/memory_routes.py _hot_chunks was already dict-tolerant (c if isinstance(c, dict) else c.as_dict() → .get("tier")) and keeps working unchanged (docstring updated); knowledge_routes.py _knowledge_row already reads tier from the dict. No consumer needed a code change for the row type — that is the point of returning the right type.

Tests

Regression tests on a real LayeredKnowledgeStore (two tmp KnowledgeStores) — five fail on main, pass here:

• test_list_chunks_returns_tier_tagged_chunk_rows — rows are Chunk, .tier / as_dict()["tier"] set, single-backend stores leave it None, filters reach both tiers
• test_list_chunks_tags_dict_rows_from_a_custom_backend — dict-yielding backends keep dicts
• test_memory_list_tool_renders_on_a_layered_store — the tool renders both tiers (crashed before)
• test_fact_consolidation_dedups_on_a_layered_store — second identical fact is skipped, not added (add-only before)
• test_snapshot_seed_exports_layered_rows — with explicit domains=, the seed carries the private row and not the commons row (empty before: dict rows)
• test_snapshot_seed_discovers_domains_on_a_layered_store — with no domains= (the live callers' shape), discovery finds the real domains, exports the private tier, drops memory domains, skips the commons-only domain, no private/commons phantom docs (empty before: stats shape)
• test_stats_merges_a_domain_present_in_both_tiers — a domain in both tiers sums; split keys always present; empty store is {"total": 0, "private": 0, "commons": 0}
• test_hot_list_on_a_real_layered_store (tests/test_memory_routes.py) — passes on main too; pins that the hot-memory route still lists the private row with its tier + injecting flag and excludes the commons row with the new row type

Existing test_promote_then_forget_from_commons updated from c["tier"] / c["id"] to attribute access; test_stats_split_by_tier updated to the merged shape.

Gates

• uv run ruff check . → All checks passed!
• uv run lint-imports → Contracts: 3 kept, 0 broken.
• focused (test_knowledge_layered, test_agent_snapshot[_import], test_memory_routes, test_knowledge_routes, test_chunking, test_knowledge_trust, test_memory_facts, test_knowledge_typed_memory, test_knowledge_lifecycle) → 313 passed after folding the stats fix (289 before)
• full suite uv run python -m pytest tests/ -q -p no:cacheprovider → 6892 passed, 16 skipped, EXIT=0 (331s)
• pre-fix proof: the five row-shape regression tests against origin/main's store.py + layered.py → 5 failed; the seed-discovery + stats-shape tests against origin/main's layered.py + snapshot_op.py → 2 failed — all as expected

🤖 Generated with Claude Code

https://claude.ai/code/session_01WEMxBi71vjtmmmziFCMcby

[coderabbitai]

Warning

Review limit reached

Next included review available in 59 minutes.

View limit details

Limit details: You’ve used the included review currently available.

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: a03d1c18-446d-4c3d-81a0-92b116f9d18c

📥 Commits

Reviewing files that changed from the base of the PR and between a32eb6b and 218c3ac.

📒 Files selected for processing (11)

• changelog.d/3245.fixed.md
• docs/adr/0031-pluggable-knowledge-backend.md
• graph/snapshot_op.py
• knowledge/backend.py
• knowledge/layered.py
• knowledge/store.py
• operator_api/knowledge_routes.py
• operator_api/memory_routes.py
• tests/test_knowledge_layered.py
• tests/test_memory_routes.py
• tools/lg_tools.py

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[protoreview]

QA panel review — WARN

code-review-structural · head 218c3ac7ba37 · formal

Low-risk change: the PR introduces a tier-key split mechanism across three modules (graph/snapshot_op.py, knowledge/layered.py, tools/lg_tools.py) to fix a domain-discovery bug where non-domain stat keys were leaking into the seed. The panel's only surviving finding is a minor maintenance-hazard note about the duplicated key set across those three files — no correctness or security issues were identified. The verifier confirmed the one finding and refuted nothing. No prior blockers/majors to disposition. Coverage note: the panel read all three touched files but produced only a single minor; the structural pass was skipped, so a deeper cross-file consistency check (e.g., whether the tier-key logic in lg_tools correctly mirrors the frozenset semantics in the other two) went unexercised.

Findings

	Severity	Location	Finding	Verified
🟡	minor	graph/snapshot_op.py:70	The tier-split key set is duplicated across three modules (byte-identical frozensets in snapshot_op and layered, plus an inline tuple in lg_tools), so a future…	confirmed

findings JSON (machine-readable)

[
  {
    "file": "graph/snapshot_op.py",
    "line": 70,
    "severity": "minor",
    "category": "conventions",
    "claim": "The tier-split key set is duplicated across three modules (byte-identical frozensets in snapshot_op and layered, plus an inline tuple in lg_tools), so a future tier key added to one but not the others will silently desync the snapshot seed's domain discovery from the layered stats \u2014 the same failure class this PR fixes.",
    "evidence": "graph/snapshot_op.py: `_NON_DOMAIN_STAT_KEYS = frozenset({\"total\", \"private\", \"commons\"})` with the comment `#: matched, and the seed exported empty. Kept in step with ``knowledge.layered._SPLIT_KEYS``.`; knowledge/layered.py: `_SPLIT_KEYS = frozenset({\"total\", \"private\", \"commons\"})` with the comment `# step with ``graph.snapshot_op._NON_DOMAIN_STAT_KEYS`` (the seed's domain discovery).`; tools/lg_tools.py: `if k in (\"private\", \"commons\"):` \u2014 three copies of the same tier-key knowledge that must be kept in manual sync.",
    "verdict": "confirmed",
    "note": "All three copies verified in the PR diff: two byte-identical frozensets (snapshot_op + layered) and one inline tuple (lg_tools, correctly omitting 'total' since that key is handled separately above). The cross-referencing comments confirm the authors are aware of the coupling. Valid maintenance-hazard observation at minor severity."
  }
]