DEV-1671: KB-materialization checker + reference-first store-cleanup recipe - #87
Merged
ZmeiGorynych merged 16 commits intoJul 15, 2026
Conversation
…recipe Add a deterministic checker that reports whether a saved edited-model store is in a "passing state": every surviving agent-added entity is used by the winning query, relevant KB items are encoded as entities, entities reference each other (clean DAG, not inlined), and the query does not do kb/concept work inline that should be an encoded column/concept. Recommendations only — all lineage is resolved through SLayer's own helpers (column_dependency / enrichment / core.formula), never a hand-rolled parser. - store_kb_checker.py: check_models / check_store + relevant_kb_closure. Findings UNUSED_AGENT_ENTITY, INLINE_QUERY_WORK, NON_KB_ENTITY, INLINED_KB_DEF, plus advisory EXPECTED_KB_NOT_MATERIALIZED / DEFERRED_RELEVANT_KB / ORPHAN_KB_ENTITY. Resolves cross-model (join-reachable) refs, nested multi-stage queries, and filter/measure references; join-key + PK columns are structural scaffolding. - store_cleanup.py: identical-result comparison (positional, headers-ignored, Decimal precision, NULL-aware) + materialize_store / verify_and_repack scaffolding. The cleanup agent drives the REAL SLayer edit tools directly (create/edit/delete_model); these helpers only back the correctness gate. - scripts/check_edited_model_stores.py: CLI scanner over the saved stores. - .claude/skills/clean-edited-model-store: reference-first, add-missing-encodings, delete-last cleanup recipe. - tests: 29 checker + 11 cleanup unit tests (pure, no postgres). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DEV-1671 Clean up 3 saved edited-model stores: hoist inline KB defs into models + prune unused agent additions
GoalPost-hoc clean up the saved edited-model stores for exactly 3 livesqlbench-large tasks so that the reused (apply) store contains only legitimate, reusable KB-derived definitions — not one-off inline query scaffolding and not broken/unused leftovers. This is a manual, after-the-fact fix for these 3 tasks; wiring the same discipline into the model-writing agent is deferred (see "Later"). This spun out of DEV-1670 (token consumption of lean+readonly apply-saved-models runs). Investigation there found that the worst token-thrash tasks in the apply-readonly run reuse stores that either (a) never encoded the KB definition the answer needs (so the agent re-derives it inline, repeatedly, hitting slayer-DSL join/grouping errors), or (b) contain a broken/unused derived model saved next to the winner that the reusing agent wastes turns on. The 3 tasks (livesqlbench-large)
(Paths are in the MAIN checkout HARD CONSTRAINT — do NOT cheatDo NOT encapsulate the winning query as a model. Baking "the answer" (e.g. the final aggregation/filters that produce the graded result) into a stored model or measure is cheating and is forbidden. What you MAY store is KB definitions — the reusable semantic building blocks that the knowledge base defines (a KB formula, a KB-defined derived quantity, a KB enum normalization). The query that combines/aggregates/filters those building blocks into the actual answer must still be written at query time. The litmus test: if a definition is a verbatim/paraphrased KB entry (tagged The cleanup recipe (per task)
Per-task starting facts (from the DEV-1670 investigation)
NB: because all three winning queries mostly reference STORED columns, the (B) prune path likely dominates and the (A) hoist path may be a no-op for some — verify per task, don't assume. Execution plan (requested)
Sources / how to reproduce the investigation
RelatedDEV-1670 (token-consumption parent — this is the "why"), DEV-1668 (unified inspect / branch this forks from), DEV-1666 (lean/readonly flags), DEV-1609 ( |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds edited-model store validation and cleanup tooling, hides expanded raw JSONB columns across pipeline and archive paths, adds readonly prompt guidance, and broadens filename mode parsing with regression tests. ChangesEdited-model store validation
Expanded JSONB visibility
Readonly prompt guidance
Filename mode parsing
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bird_interact_agents/agents/claude_sdk_otf_v1/agent.py (1)
294-310: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSynchronize prompt instructions with the filtered tool surface.
Default lean mode removes
inspect_modelwhile the static prompt still mentions it. Likewise, readonly mode’s inline-only requirement reaches only the log, not the model. Gate or replace unavailable-tool instructions during prompt construction; otherwise runs can waste turns calling tools that were deliberately removed.Also applies to: 373-378, 536-540, 585-588
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/agents/claude_sdk_otf_v1/agent.py` around lines 294 - 310, Update _build_prompt and the corresponding prompt construction at the referenced locations so instructions match the filtered tool surface: when lean_introspection is enabled, remove or replace static inspect_model guidance, and when readonly_mode is enabled, explicitly communicate the inline-only requirement to the model rather than only logging it. Ensure unavailable-tool instructions are not emitted in any one-shot prompt path.
🧹 Nitpick comments (6)
scripts/check_edited_model_stores.py (1)
45-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAd-hoc path concatenation off
_repo_root()forresults/andgated_gold/data.
_repo_root() / reland_repo_root() / "results" / benchmarkbuild gitignored/output paths via string concatenation rather than a dedicatedpaths.*_root()helper, even though this same file already funnels the OTF cache and main-checkout paths throughpaths.slayer_otf_cache_root()/paths.main_checkout_root(). This file is underscripts/, outside the literalsrc/**/*.pyguideline pattern, but the same convention seems intended for consistency.Please check whether
bird_interact_agents.pathsalready exposes dedicated roots forresults/and the gated-gold jsonl directory that this script should use instead of_repo_root() / "results"/_repo_root() / rel.Also applies to: 70-71, 93-96
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@scripts/check_edited_model_stores.py` around lines 45 - 51, Update _load_external_knowledge and the related results-loading logic to use existing dedicated roots from bird_interact_agents.paths for gated-gold data and results instead of constructing paths with _repo_root() / rel or _repo_root() / "results". Verify the appropriate helper names in paths, then preserve the current benchmark and instance lookup behavior while routing all affected path construction through those helpers.src/bird_interact_agents/agents/_pre_encoded_prompts.py (1)
311-324: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExport the new public builders consistently.
The new
build_slayer_pre_encoded_one_shot*functions are public-looking APIs, but the module’s__all__list does not include them. Add both names if wildcard imports are part of the supported surface, or explicitly keep them internal.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/agents/_pre_encoded_prompts.py` around lines 311 - 324, Update the module’s __all__ export list to include both public builders, build_slayer_pre_encoded_one_shot and build_slayer_pre_encoded_one_shot_v1, so wildcard imports expose them consistently with their public API naming.src/bird_interact_agents/memory_store_io.py (1)
24-30: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffReliance on slayer's private (underscore) storage internals.
_save_memory_row,_md_to_memory, and_memory_to_mdare underscore-prefixed — not part of slayer's public contract. The module docstring gives a solid rationale (id/created_atdeterminism thatsave_memorydoesn't support), and thepyproject.tomlfloor pin (>=0.9.6) mitigates immediate risk, but a future slayer patch/minor release could rename or remove these without notice, silently breaking this "single choke point" for KB memory I/O.Consider whether slayer's maintainers would accept a public row-writer API (e.g. an
id/created_at-preserving variant ofsave_memory) to remove this dependency on private symbols, or at minimum add a smoke test asserting these three names still exist on import — cheaper failure mode than a silent behavior change.🔎 Optional: guard against silent private-API drift
# In a small existing test module (e.g. tests/test_memory_store_io.py) def test_slayer_private_storage_symbols_still_exist(): from slayer.storage.yaml_storage import YAMLStorage, _md_to_memory, _memory_to_md assert hasattr(YAMLStorage, "_save_memory_row")Also applies to: 62-69, 72-85
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/memory_store_io.py` around lines 24 - 30, The memory I/O implementation relies on slayer’s private storage symbols, risking silent breakage from upstream changes. Prefer a public slayer API that preserves memory id and created_at values and update the relevant memory read/write flow to use it; if no public API is available, add a smoke test covering YAMLStorage._save_memory_row, _md_to_memory, and _memory_to_md.src/bird_interact_agents/agents/_shared_otf_prompts.py (1)
1621-1642: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDefault
lean_introspection=Falsehere diverges from the system-wide default ofTrueelsewhere.
run.py'smake_runner/_make_runner/run_evaluationand the CLI (--no-lean) all defaultlean_introspection=True(lean introspection ON by default). These two public builders default it toFalse, matching the FROZEN legacy constant contract (documented and correct for that purpose), but it's an easy trap for a future caller that omits the kwarg expecting the "normal" project default.♻️ Optional: make the divergence explicit
def build_slayer_otf_one_shot_v0( - *, lean_introspection: bool = False, readonly_mode: bool = False + *, lean_introspection: bool = False, readonly_mode: bool = False, + # NOTE: intentionally opposite of run.py's system-wide default (True) — + # False/False reproduces the frozen legacy SLAYER_OTF_ONE_SHOT_V0 byte-for-byte. ) -> str:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/agents/_shared_otf_prompts.py` around lines 1621 - 1642, Make the default behavior of build_slayer_otf_one_shot_v0 and build_slayer_otf_ainteract_v0 explicit so callers understand that lean_introspection=False intentionally preserves the frozen legacy template contract, despite the system-wide default being True. Update the public builders’ documentation without changing their default values or generated output.src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py (1)
433-442: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
effective_native_toolsinstead of re-deriving the lean drop-set inline.
claude_sdk_otf/agent.pyalready exposeseffective_native_tools(eval_mode, lean_introspection=...)that does exactly this filtering. Re-implementing it here risks the two agent flavors silently diverging if the drop-set logic changes in only one place.♻️ Proposed refactor
- tools = _select_tools(eval_mode) - if self.lean_introspection: - tools = [t for t in tools if t.name not in LEAN_DROP_NATIVE_KB] + tools = effective_native_tools( + eval_mode, lean_introspection=self.lean_introspection + )And add
effective_native_toolsto the import fromclaude_sdk_otf.agent(dropping the now-unusedLEAN_DROP_NATIVE_KB/_select_toolsimports if unused elsewhere in this file).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py` around lines 433 - 442, Update the tool selection in the agent flow around _build_prompt to use effective_native_tools(eval_mode, lean_introspection=self.lean_introspection) instead of calling _select_tools and applying LEAN_DROP_NATIVE_KB inline. Import effective_native_tools from claude_sdk_otf.agent and remove the drop-set or selector imports only if they are unused elsewhere in the file.src/bird_interact_agents/slayer_otf/store_cleanup.py (1)
206-225: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid reaching into
_tool_managerinternals.query/query_nestedare the exposed tools, butmcp._tool_manager._tools[...]ties this helper to an internal server shape. Keep themcp._slayer_engine.aclose()teardown; that hook is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/slayer_otf/store_cleanup.py` around lines 206 - 225, Update run_store_query to invoke the exposed query and query_nested tools through the MCP server’s public interface instead of accessing mcp._tool_manager._tools. Preserve the existing single-dict versus nested-list dispatch, format propagation, and intentional mcp._slayer_engine.aclose() teardown.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLAUDE.md`:
- Around line 126-132: Update both polling examples in CLAUDE.md to invoke
`bird-interact-cloud list <run-id>` through the required `env -u SSH_AUTH_SOCK
uv run` wrapper. Preserve the run-id argument and surrounding polling behavior
so each command remains copy-pasteable and consistent with the documented setup.
In `@README.md`:
- Around line 42-48: Update the later README instructions that require `--mode
one-shot` or `--mode a-interact` to describe these values as optional explicit
overrides, consistent with the recommendation to omit `--mode` and rely on
benchmark-specific defaults. Preserve guidance for passing `--mode` when
deliberately selecting a non-default mode.
In `@src/bird_interact_agents/slayer_otf/cache.py`:
- Around line 576-580: Update the docstring and nearby references for
_materialise_cache_memories to describe persistence through per-ID
memories/<id>.md files via the slayer storage layer, replacing references to the
obsolete flat memories.yaml format. Keep the documented cache contract aligned
with the existing persist_memories behavior.
In `@src/bird_interact_agents/slayer_otf/store_kb_checker.py`:
- Around line 387-407: The stage usage analysis currently skips dictionary-based
inline models because _collect_used_models (or the enclosing stage traversal)
sets root to None. Update this flow to resolve the inline model’s source_name to
its underlying store model or prior-stage root, then process that model’s
reachable dependencies and include references from the inline columns in the
seed closure. Preserve existing handling for string source_model values and
continue recording the stage root and used models consistently.
In `@tests/test_slayer_otf_smoke.py`:
- Around line 177-181: Update the memory-file assertion in the smoke test to
require exact set equality between mem_stems and the expected
{f"{db}_kb_{int(r['id'])}" for r in kb_rows} values, preserving coverage that
storage contains one and only one memory file per KB row.
---
Outside diff comments:
In `@src/bird_interact_agents/agents/claude_sdk_otf_v1/agent.py`:
- Around line 294-310: Update _build_prompt and the corresponding prompt
construction at the referenced locations so instructions match the filtered tool
surface: when lean_introspection is enabled, remove or replace static
inspect_model guidance, and when readonly_mode is enabled, explicitly
communicate the inline-only requirement to the model rather than only logging
it. Ensure unavailable-tool instructions are not emitted in any one-shot prompt
path.
---
Nitpick comments:
In `@scripts/check_edited_model_stores.py`:
- Around line 45-51: Update _load_external_knowledge and the related
results-loading logic to use existing dedicated roots from
bird_interact_agents.paths for gated-gold data and results instead of
constructing paths with _repo_root() / rel or _repo_root() / "results". Verify
the appropriate helper names in paths, then preserve the current benchmark and
instance lookup behavior while routing all affected path construction through
those helpers.
In `@src/bird_interact_agents/agents/_pre_encoded_prompts.py`:
- Around line 311-324: Update the module’s __all__ export list to include both
public builders, build_slayer_pre_encoded_one_shot and
build_slayer_pre_encoded_one_shot_v1, so wildcard imports expose them
consistently with their public API naming.
In `@src/bird_interact_agents/agents/_shared_otf_prompts.py`:
- Around line 1621-1642: Make the default behavior of
build_slayer_otf_one_shot_v0 and build_slayer_otf_ainteract_v0 explicit so
callers understand that lean_introspection=False intentionally preserves the
frozen legacy template contract, despite the system-wide default being True.
Update the public builders’ documentation without changing their default values
or generated output.
In `@src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py`:
- Around line 433-442: Update the tool selection in the agent flow around
_build_prompt to use effective_native_tools(eval_mode,
lean_introspection=self.lean_introspection) instead of calling _select_tools and
applying LEAN_DROP_NATIVE_KB inline. Import effective_native_tools from
claude_sdk_otf.agent and remove the drop-set or selector imports only if they
are unused elsewhere in the file.
In `@src/bird_interact_agents/memory_store_io.py`:
- Around line 24-30: The memory I/O implementation relies on slayer’s private
storage symbols, risking silent breakage from upstream changes. Prefer a public
slayer API that preserves memory id and created_at values and update the
relevant memory read/write flow to use it; if no public API is available, add a
smoke test covering YAMLStorage._save_memory_row, _md_to_memory, and
_memory_to_md.
In `@src/bird_interact_agents/slayer_otf/store_cleanup.py`:
- Around line 206-225: Update run_store_query to invoke the exposed query and
query_nested tools through the MCP server’s public interface instead of
accessing mcp._tool_manager._tools. Preserve the existing single-dict versus
nested-list dispatch, format propagation, and intentional
mcp._slayer_engine.aclose() teardown.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 4f37cb68-7ede-4b54-9dd2-83fc0b4bd0cb
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (103)
.claude/skills/clean-edited-model-store/SKILL.mdCLAUDE.mdREADME.mdpyproject.tomlscripts/check_edited_model_stores.pyscripts/verify_kb_coverage.pyslayer_models/households/memories.yamlslayer_models/households/memories/1.mdslayer_models/households/memories/10.mdslayer_models/households/memories/11.mdslayer_models/households/memories/12.mdslayer_models/households/memories/13.mdslayer_models/households/memories/14.mdslayer_models/households/memories/15.mdslayer_models/households/memories/16.mdslayer_models/households/memories/17.mdslayer_models/households/memories/18.mdslayer_models/households/memories/19.mdslayer_models/households/memories/2.mdslayer_models/households/memories/20.mdslayer_models/households/memories/21.mdslayer_models/households/memories/22.mdslayer_models/households/memories/23.mdslayer_models/households/memories/24.mdslayer_models/households/memories/3.mdslayer_models/households/memories/4.mdslayer_models/households/memories/5.mdslayer_models/households/memories/6.mdslayer_models/households/memories/7.mdslayer_models/households/memories/8.mdslayer_models/households/memories/9.mdsrc/bird_interact_agents/agents/_host_discovery_playbook.pysrc/bird_interact_agents/agents/_pre_encoded_prompts.pysrc/bird_interact_agents/agents/_prompt_builders.pysrc/bird_interact_agents/agents/_shared_otf_prompts.pysrc/bird_interact_agents/agents/_slayer_tool_surface.pysrc/bird_interact_agents/agents/claude_sdk/agent.pysrc/bird_interact_agents/agents/claude_sdk/partition.pysrc/bird_interact_agents/agents/claude_sdk/prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf/prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract/prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract_raw_v1/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf_encode/prompts.pysrc/bird_interact_agents/agents/claude_sdk_otf_encode/setup_encoder.pysrc/bird_interact_agents/agents/claude_sdk_otf_raw_v1/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_v1/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_v1/prompts.pysrc/bird_interact_agents/cloud/cli.pysrc/bird_interact_agents/cloud/driver.pysrc/bird_interact_agents/cloud/ray_app.pysrc/bird_interact_agents/eval/annotation_schema.pysrc/bird_interact_agents/eval/autopsy.pysrc/bird_interact_agents/eval/versioning.pysrc/bird_interact_agents/hard8_preprocessor.pysrc/bird_interact_agents/memory_store_io.pysrc/bird_interact_agents/run.pysrc/bird_interact_agents/slayer_otf/cache.pysrc/bird_interact_agents/slayer_otf/reference_build.pysrc/bird_interact_agents/slayer_otf/runtime.pysrc/bird_interact_agents/slayer_otf/store_cleanup.pysrc/bird_interact_agents/slayer_otf/store_kb_checker.pytests/_edited_models_fixtures.pytests/data/dev1603/slayer_ainteract_v0.golden.txttests/data/dev1603/slayer_one_shot_v0.golden.txttests/scripts/test_export_slayer_models.pytests/scripts/test_verify_kb_coverage.pytests/test_claude_sdk_otf_ainteract_v1_agent.pytests/test_claude_sdk_otf_ainteract_v1_run_wiring.pytests/test_claude_sdk_otf_disallowed_slayer_tools.pytests/test_claude_sdk_otf_raw_v1_run_wiring.pytests/test_claude_sdk_otf_v1_agent.pytests/test_claude_sdk_otf_v1_run_wiring.pytests/test_dev1534_query_wrapper.pytests/test_dev1546_distinct_dim_values.pytests/test_dev1581_agent_wiring.pytests/test_dev1586_pre_encoded.pytests/test_dev1589_setup_encoder.pytests/test_dev1591_force_compact_search.pytests/test_dev1602_run_subscription_flag.pytests/test_dev1609_claude_sdk_otf_encode.pytests/test_dev1623_filter_and_submit_mandates.pytests/test_dev1666_lean_readonly_flags.pytests/test_dev1668_unified_inspect.pytests/test_edited_models.pytests/test_hard8_preprocessor.pytests/test_memory_store_io.pytests/test_pydantic_ai_otf_encode_kb_loader.pytests/test_pydantic_ai_otf_encode_kb_to_slayer.pytests/test_pydantic_ai_otf_encode_kb_to_slayer_storage.pytests/test_pydantic_ai_otf_encode_memory_annotation.pytests/test_pydantic_ai_otf_encode_run_wiring.pytests/test_run_framework_dispatch.pytests/test_shared_otf_prompts.pytests/test_slayer_otf_cache.pytests/test_slayer_otf_reference_build.pytests/test_slayer_otf_smoke.pytests/test_slayer_tool_surface.pytests/test_store_cleanup.pytests/test_store_kb_checker.py
💤 Files with no reviewable changes (1)
- slayer_models/households/memories.yaml
ZmeiGorynych
changed the base branch from
main
to
egor/dev-1668-bird-agents-adopt-unified-inspect-drop-models_summary
July 13, 2026 17:53
…no-re-derive rule
Two encoder/prompt-level levers for the apply-edited-models+readonly token
thrash (DEV-1670), both independent of DEV-1671 store cleanup.
Cause 2 — hide raw JSON columns (they add no query capability; the flat
leaves already cover every documented path, resolving to the underlying
TABLE column):
- jsonb.hide_expanded_jsonb_columns(model): hide any meta.jsonb col owning
>=1 derived leaf (case-insensitive, persisted-state, idempotent, >=1-leaf
guard so leaf-less cols stay discoverable).
- _phase3_jsonb wires it in at build time.
- cache._PHASE3_IMPL_TOKEN feeds the IMPL fingerprint only, so warm caches
rebuild-with-hidden while saved edited-model archives (full fp unchanged)
stay valid on apply.
- slayer_otf/hide_jsonb_stores.{hide_store_dir,hide_archive}: propagate to
OTF reference stores + saved archives (archive repack preserves cache_fp).
- edited_models.materialize_from_saved_store: apply-time self-heal (best-
effort, before the baseline) so an un-migrated archive still queries hidden.
- scripts/migrate_hide_raw_jsonb.py: operator CLI over the primitives.
Cause 1 — stern "don't re-derive an already-encoded quantity" rule
(_NO_REDERIVE_READONLY), spliced ONLY into the v0 OTF readonly branch
(one-shot + a-interact); names/overrides the help.intro ModelExtension nudge
while preserving the deferred-KB escape hatch. v1 readonly wiring deferred.
Full non-integration suite: 4467 passed.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…lback - cache._impl_fingerprint_of docstring now lists the phase-3 token component and notes it is impl-fp-only, excluded from the full _cache_fp.txt (CodeRabbit). - hide_jsonb_stores._extract_all pre-3.11.4 fallback now validates members (reject absolute paths / .. escapes / links) before extracting, mirroring edited_models._safe_extractall — inlined to avoid a circular import (Codex). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
….4 fallback Brings the hide_archive fallback to parity with the filter="data" path it emulates (which rejects device/fifo members) (CodeRabbit). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
- --benchmark is now required + validated against the canonical hyphenated names (choices); the old default "mini_interact" (underscore) crashed the paths validators mid-run. Fails fast at argparse with the valid list. - _sweep_archives is scoped to runs/<benchmark>/ instead of walking all of runs/, so a benchmark-targeted migration no longer rewrites other benchmarks' archives or the runs/_edited_models_backups/ copies. - New tests/scripts/test_migrate_hide_raw_jsonb.py pins both the benchmark scoping and the instance-id filter. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The token forced warm OTF caches to rebuild so they'd pick up the hidden-column change. It was unnecessary — the apply-time self-heal and the migration's --cache flag already hide columns without any rebuild — and it caused a regression: forcing a rebuild re-stamps _cache_fp.txt, and when the recomputed fingerprint differs from a saved edited-model archive's stamp (e.g. inputs drifted since the archive was saved), --apply-edited-models silently falls back to a fresh cache. Cause-2 hiding is now delivered by: the encoder (fresh builds), apply-time self-heal (applied stores), and the migration (existing stores, incl. --cache for warm caches). Fingerprint logic reverts to unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mode_from_filename only matched the hyphen-delimited cloud format (-raw-/ -slayer-), so every underscore-delimited local run (…_claude_sdk_raw.json, optionally local_-prefixed) was silently dropped — collect_latest_per_task returned zero local raw/slayer runs. Anchor the mode slot on -/_ with a trailing -/_/./end so the trailing-slug local form is matched too. Regression test covers both formats + the trajectory sidecar. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The code half of the token removal (the prior commit only dropped its test). Reverts _PHASE3_IMPL_TOKEN + its _impl_fingerprint_of hash line + docstring in cache.py, and scrubs the "token forces a rebuild" narrative from hide_jsonb_stores.py / migrate_hide_raw_jsonb.py. Cause-2 hiding is delivered by the encoder (fresh builds) + apply-time self-heal + migration (incl. --cache). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…oken-thrash-stop-inline-re-derivation-of DEV-1672: cut slayer-query token thrash — hide raw JSON cols + stern no-re-derive rule
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/bird_interact_agents/slayer_otf/edited_models.py`:
- Around line 314-327: Update the exception handling around hide_store_dir in
the apply-time self-healing flow to remove the partially prepared scratch store
and return None immediately when self-healing fails. Do not call
write_baseline_manifest after the failure; preserve baseline creation only for
successfully healed stores so the documented fresh-cache fallback is triggered.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: faec8d6a-e021-46c1-a0d2-902e57dcda13
📒 Files selected for processing (12)
scripts/cascade_for_combo.pyscripts/migrate_hide_raw_jsonb.pysrc/bird_interact_agents/agents/_shared_otf_prompts.pysrc/bird_interact_agents/slayer_otf/edited_models.pysrc/bird_interact_agents/slayer_otf/hide_jsonb_stores.pysrc/bird_interact_agents/slayer_pipeline/jsonb.pysrc/bird_interact_agents/slayer_pipeline/orchestrator.pytests/scripts/test_cascade_for_combo.pytests/scripts/test_migrate_hide_raw_jsonb.pytests/slayer_otf/test_dev1672_hide_jsonb_stores.pytests/slayer_pipeline/test_dev1672_hide_raw_jsonb.pytests/test_dev1672_readonly_rederive_prompt.py
Checker fixes from the Codex review of PR #87 — all reduce false UNUSED: - resolve cross-model (join-reachable) references and ModelExtension source_name roots; count time_dimensions and order-by `col:agg` refs; recognise list-form join_pairs as join keys; expand measure-of-measure via named_measures. - INLINE_QUERY_WORK recommendation (JSON extraction / CASE / multi-column arithmetic / inline model in a query) — Case B of the reference-first model. - recommendation model: findings are advisory (no hard error); unused [kb]/[concept] entities read as "reference if useful, else remove", untagged as scratch-to-delete; StoreCheckReport.ok = no substantive finding. Cleanup helpers rewritten to drive the REAL SLayer tools (no ops re-encoding): - store_cleanup: materialize_store (backup+untar) + verify_and_repack (identical- result gate + checker + repack); results_identical uses a string-aware JSON parse. The cleanup agent calls create/edit/delete_model directly. - CLI: pick the latest slayer attempt (tie-break on attempt file); use the current OTF cache as baseline even on a cosmetic cache_fp mismatch. Full non-integration suite green (4437 passed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A source_queries-backed (grain-bridge) model projects its output from a nested query that consumes base KB definitions on OTHER models. Column.sql traversal cannot see those references, so the base defs were falsely flagged UNUSED when a bridge's output was the only consumer (e.g. mental_healths_10 joins pfis_fac / tes_fac2 / … over pfis/tes/tar/mar base KB columns). Expand a used bridge's source_queries (dimensions + measure-formula refs) into the closure so those load-bearing base defs count as used. Genuine scratch (duplicate bridges, broken measures, stale projections) still flags correctly. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…anup Cleaning the last dirty stores exposed four checker false positives; each is fixed with a regression test, and all 44 livesqlbench-large stores now scan OK. - Far-side join keys: a join is declared on the PARENT (facilities.joins -> target bridge on [fac_key, bridge_col]); the bridge's own joins are empty, so its far-side join column read as UNUSED scratch. Exempt far-side join columns on the target model too. - source_queries-bridge projection columns: a trivial-base passthrough column of a source_queries-backed bridge is the compiler's projection of the backing query (a ratio-of-aggregates measure surfaces its component columns, e.g. mar = miss_appt:sum / pat_ref:count_distinct). Not independently-removable scratch — treat as structural. - Window-function measure refs: _measure_ref_names now recurses into the .inner of window/scalar TransformFields so a column used only inside rank(x:max, ...) counts as used. - Inline-model work detection: an inline source_model is only INLINE_QUERY_WORK when it does real work (a measure or a non-constant column sql). A constant-only inline column (sql="'sprint'") is not encodable work; handle both the source_name/columns and base_model/extra_columns inline forms. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…dited-model-stores-hoist-inline-kb-defs' into egor/dev-1671-clean-up-3-saved-edited-model-stores-hoist-inline-kb-defs
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/bird_interact_agents/slayer_otf/store_kb_checker.py (1)
435-449: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winHandle both inline ModelExtension key shapes in
_query_closure. The dict branch only acceptssource_name; thebase_model/extra_columnsform used by the inline-work tests still drops toroot = None, so any referenced base-model columns can be missed and later misreported as UNUSED. Accept either key here and add a regression test for thebase_modelshape.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/slayer_otf/store_kb_checker.py` around lines 435 - 449, Update the dict handling in _query_closure to resolve inline ModelExtension objects using either source_name/columns or base_model/extra_columns, mapping the selected base model through stage_root as currently done. Preserve existing string and invalid-source behavior, and add a regression test proving referenced columns from the base_model shape are not reported as UNUSED.
🧹 Nitpick comments (1)
src/bird_interact_agents/slayer_otf/store_kb_checker.py (1)
596-622: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFar-side join-key resolution duplicates
_join_key_columns's attr-matching pattern.This block re-implements the same "try a list of candidate attribute names on the join-pair object" pattern already used in
_join_key_columns(Lines 195-199), just withtarget_*names instead ofsource_*names. Extracting a shared helper (e.g._join_pair_names(pair) -> tuple[str|None, str|None]returning(source_name, target_name)) would remove the duplication and reduce the risk of the two attribute lists silently diverging again.♻️ Sketch of a shared helper
+def _join_pair_names(pair) -> tuple[str | None, str | None]: + if isinstance(pair, (list, tuple)): + src = pair[0] if pair and isinstance(pair[0], str) else None + tgt = pair[1] if len(pair) >= 2 and isinstance(pair[1], str) else None + return src, tgt + src = next((getattr(pair, a, None) for a in + ("source_column", "source_field", "from_column", "left", "source", "column") + if isinstance(getattr(pair, a, None), str)), None) + tgt = next((getattr(pair, a, None) for a in + ("target_column", "target_field", "to_column", "right", "target", "column") + if isinstance(getattr(pair, a, None), str)), None) + return src, tgt🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/bird_interact_agents/slayer_otf/store_kb_checker.py` around lines 596 - 622, Extract the join-pair attribute resolution shared by `_join_key_columns` and the far-side join-key loop into a helper such as `_join_pair_names(pair)`, returning source and target column names. Update `_join_key_columns` and the `join_keys_by_model` population block to use this helper, preserving their existing handling of sequence pairs and object pairs while keeping the candidate attribute mappings centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/bird_interact_agents/slayer_otf/store_kb_checker.py`:
- Around line 435-449: Update the dict handling in _query_closure to resolve
inline ModelExtension objects using either source_name/columns or
base_model/extra_columns, mapping the selected base model through stage_root as
currently done. Preserve existing string and invalid-source behavior, and add a
regression test proving referenced columns from the base_model shape are not
reported as UNUSED.
---
Nitpick comments:
In `@src/bird_interact_agents/slayer_otf/store_kb_checker.py`:
- Around line 596-622: Extract the join-pair attribute resolution shared by
`_join_key_columns` and the far-side join-key loop into a helper such as
`_join_pair_names(pair)`, returning source and target column names. Update
`_join_key_columns` and the `join_keys_by_model` population block to use this
helper, preserving their existing handling of sequence pairs and object pairs
while keeping the candidate attribute mappings centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2c80cc70-8077-4bf1-9a94-411db93ecf6d
📒 Files selected for processing (5)
scripts/check_edited_model_stores.pysrc/bird_interact_agents/slayer_otf/store_cleanup.pysrc/bird_interact_agents/slayer_otf/store_kb_checker.pytests/test_store_cleanup.pytests/test_store_kb_checker.py
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_store_cleanup.py
- scripts/check_edited_model_stores.py
- src/bird_interact_agents/slayer_otf/store_cleanup.py
…ME doc - CodeRabbit nitpick: extract `_join_pair_names(pair)` shared by `_join_key_columns` and the far-side join-key exemption (single place for the list/object pair forms). - Codex #1: dotted `join_pairs` (e.g. `["a.local_col", "b.remote_col"]`, which SLayer supports) were compared against bare `Column.name`s, so the join key was not exempted. `_bare_column` strips the qualifier on both near and far sides (+test). - Codex #2: `_CONST_SQL_RE` rejected escaped-quote string literals (`'can''t'`), so a constant-only inline column was falsely flagged INLINE_QUERY_WORK. Allow doubled single-quotes (+test). - README: document the edited-model store cleanup process (the /clean-edited-model-store skill + scripts/check_edited_model_stores.py) so it is discoverable post-merge, not only in SKILL.md. Full suite 4486 passed; all 44 livesqlbench-large stores still scan OK. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Around line 290-296: Update the Clean bullet in README.md to scope the cleanup
guarantee to the winning query: replace the broad claim that cleanup can never
change what the store answers with wording that explicitly states the winning
query’s result is preserved. Keep the existing verification and checker-clean
documentation unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 54f50725-c39f-4c23-a3d6-d54e535c8951
📒 Files selected for processing (3)
README.mdsrc/bird_interact_agents/slayer_otf/store_kb_checker.pytests/test_store_kb_checker.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/bird_interact_agents/slayer_otf/store_kb_checker.py
…nsforms `_measure_ref_names` recursed into `sub_transforms` only when it was a dict, but SLayer's `MixedArithmeticField.sub_transforms` is a `list[tuple]` of (alias, TransformField) — so a column used only inside a mixed transform (e.g. `rank(ts:max) / tot:sum`) was missed and could false-flag UNUSED. Handle the list/tuple form (recurse into non-alias members) + regression test. Full suite 4487 passed; all livesqlbench-large stores still scan OK (47/47). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_store_kb_checker.py (1)
534-538: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse iterable unpacking instead of list concatenation.
This triggers Ruff RUF005. Construct the fixture with
[*base_models(), ...]to preserve behavior and remove the lint warning.Suggested fix
- store = base_models() + [ + store = [ + *base_models(), tbl_model("m", [col("acct", "acct", type="TEXT"), col("ts", "ts_raw + 1", kb=4), col("tot", "tot_raw + 1", kb=0)], table="t"), ]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_store_kb_checker.py` around lines 534 - 538, Update the fixture construction around base_models() to use iterable unpacking within the list literal instead of list concatenation, preserving the existing tbl_model entry and resulting store contents while resolving Ruff RUF005.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_store_kb_checker.py`:
- Around line 534-538: Update the fixture construction around base_models() to
use iterable unpacking within the list literal instead of list concatenation,
preserving the existing tbl_model entry and resulting store contents while
resolving Ruff RUF005.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e0e4ed68-03eb-4867-bcba-661d0d46ceb2
📒 Files selected for processing (2)
src/bird_interact_agents/slayer_otf/store_kb_checker.pytests/test_store_kb_checker.py
…DME scope
- CodeRabbit (Major): `_query_closure` resolved the inline source_model dict only
via `source_name`; the `base_model`/`extra_columns` shape dropped to `root=None`,
so base-model columns referenced by the query were missed and false-flagged
UNUSED. Resolve the base model from either key (mirrors `_inline_model_does_work`)
+ regression test.
- CodeRabbit (Minor): scope the README cleanup guarantee to the winning query
("cannot change the winning query's result", not "what the store answers").
Full suite 4488 passed; all livesqlbench-large stores still scan OK (47/47).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ZmeiGorynych
merged commit Jul 15, 2026
40fb274
into
egor/dev-1668-bird-agents-adopt-unified-inspect-drop-models_summary
1 check passed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
A deterministic checker for saved edited-model stores + a reference-first cleanup recipe (skill) and its scaffolding helpers, plus a CLI scanner. Spun out of DEV-1670 (token thrash in lean+readonly apply-saved-models runs): the worst stores reuse scratch the winning query never uses, or inline KB/concept work that should be encoded and referenced.
Checker (
store_kb_checker.py)Reports whether a store is in a passing state — recommendations only, no hard gate:
UNUSED_AGENT_ENTITY— agent entity not referenced by the (nice) query. Untagged → likely scratch (delete);[kb]/[concept]→ consider referencing if useful, else remove (never auto-delete a reusable encoding).INLINE_QUERY_WORK— the query does JSON extraction / CASE / multi-column arithmetic / defines an inline model inline, when it should encode a column/concept and reference it.NON_KB_ENTITY,INLINED_KB_DEF, and advisoryEXPECTED_KB_NOT_MATERIALIZED/DEFERRED_RELEVANT_KB/ORPHAN_KB_ENTITY.All lineage is resolved through SLayer's own helpers (
column_dependency,enrichment.parse_filter,core.formula) — cross-model (join-reachable) refs, nested multi-stage queries, filter/measure references. No hand-rolled parser. Pure over YAML/SLayer objects → unit-tested without postgres.Cleanup (
.claude/skills/clean-edited-model-store+store_cleanup.py)Reference-first, add-missing-encodings, delete-last, gated by an identical-result check. The agent drives the real SLayer edit tools (
create_model/edit_model/delete_model) directly against the unpacked store;store_cleanuponly provides non-MCP scaffolding:materialize_store(backup + untar),results_identical(positional, Decimal, NULL-aware),verify_and_repack(identical-result gate + checker + repack).Scanner (
scripts/check_edited_model_stores.py)Walks
runs/<benchmark>/<db>/<iid>/edited_models.tar.gz, resolves the winning query from the latest slayer attempt (or the store's_winning_query.jsonsidecar), and reports per-store findings.Data side-effects (gitignored, not in this PR)
3 stores already cleaned end-to-end and verified (
reverse_logistics_4,residential_data_4,fake_account_6); backups underruns/_edited_models_backups/. A 44-store scan shows 20 clean / 24 with recommendations — the follow-up cleanup of those 24 is separate.Tests
29 checker + 11 cleanup unit tests (pure, no postgres). Full non-integration suite green (4432 passed).
🤖 Generated with Claude Code
Summary by CodeRabbit