DEV-1668: adopt slayer 0.9.6 unified inspect (drop models_summary / list_datasources / help) - #86
Merged
ZmeiGorynych merged 26 commits intoAug 11, 2026
Conversation
Tool surface (claude_sdk_otf* only): - Bump slayer floor to 0.9.6: `help` removed (-> inspect(memory:help.intro)), inspect(reference=None, entity_type=...) collection view subsumes models_summary / list_datasources. - Drop `help` unconditionally from v0/encoder allow-lists + v1 main natives (allow-listing a removed tool crashes derive_disallowed). - Lean-gate models_summary/list_datasources (add models_summary to LEAN_DROP_SLAYER_MCP; encoder exempt keeps them). Re-point the help- onboarding prompt line to inspect(memory:help.intro); re-baseline dev1603 goldens + shared-otf SHAs (help line only; legacy prose otherwise unchanged). 0.9.6 DEV-1658 integration (dragged in by the bump): - help-seeding: pass _seed_help=False at the two storage-less introspection create_mcp_server sites (metadata-only builds must not touch storage). Slayer-side fix tracked in DEV-1669. - per-id memory store: new bird_interact_agents/memory_store_io.py persists / reads / copies KB memories through slayer's memories/<id>.md storage, preserving custom ids + EPOCH created_at via _save_memory_row. Migrate all writers (runtime/cache/reference_build), the autopsy reader, the hard8 copier, the verify_kb_coverage guard, and the test fixtures off flat memories.yaml. - Migrate the one committed reference slayer_models/households/memories.yaml -> 24 per-id memories/*.md (deterministic; opens are now idempotent). Full non-integration suite green (4383 passed, 0 failed). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DEV-1668 bird-agents: adopt unified inspect — drop models_summary / list_datasources / help from the slayer surface
Follow-up to the lean_introspection/readonly_mode work (DEV-1666) and the SLayer null-ref inspect (DEV-1667). Depends on both landing in the slayer dep. GoalCollapse the last redundant introspection tools into When ready (after bumping the slayer version)
Tests
Blocked by
Worktree off main (rebase onto DEV-1666's branch if it hasn't merged yet). |
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:
📝 WalkthroughWalkthroughThis PR upgrades SLayer to 0.9.6, migrates memories to per-ID Markdown storage, replaces ChangesMemory storage, KB migration, and runtime integration
Unified inspect and SLayer 0.9.6 integration
Edited-store validation and JSONB handling
Local PostgreSQL portability
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/prompts.py (1)
74-83: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winResolve the contradictory “first call” instructions.
Line 74 requires
inspect(...)first, while Line 82 requiresask_discoveryfirst. The agent may choose the wrong ordering and either skip syntax onboarding or violate the discovery-before-build workflow. Make one ordering explicit; for example, callinspect(...)first, thenask_discovery.Proposed prompt fix
- FIRST call `ask_discovery` + After learning the tool syntax with `inspect(...)`, call `ask_discovery`🤖 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_v1/prompts.py` around lines 74 - 83, Resolve the conflicting ordering instructions in the prompt text: explicitly require the agent to call inspect(reference="memory:help.intro", entity_type="memory", compact=False) first for syntax onboarding, followed immediately by ask_discovery before any further introspection or model-building actions. Update the instructions near the tool guidance so only this sequence is described.tests/test_hard8_preprocessor.py (1)
297-301: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale docstring:
_seed_memories_and_embeddingsno longer writesmemories.yaml.The function body was migrated to
write_memories_files(per-id.md), but the docstring right above it still says"""Drop a ``memories.yaml`` + ``embeddings.db``...".📝 Proposed fix
- """Drop a ``memories.yaml`` + ``embeddings.db`` into the canonical - store so the copy paths have something to copy. The memory bodies + """Drop per-id memory files (DEV-1668) + ``embeddings.db`` into the + canonical store so the copy paths have something to copy. The memory bodies follow the kb-to-slayer-models skill convention (``KB <n> — ``). """🤖 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_hard8_preprocessor.py` around lines 297 - 301, Update the docstring for `_seed_memories_and_embeddings` to describe the current behavior: it creates per-memory `.md` files via `write_memories_files` and seeds `embeddings.db`, rather than writing `memories.yaml`.src/bird_interact_agents/slayer_otf/cache.py (1)
465-475: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winStale "
memories.yaml" docstrings after the per-id storage migration.Both
_build_async's and_materialise_cache_memories's docstrings still describe writing/overwritingmemories.yaml, but the implementation now persists per-idmemories/<id>.mdfiles viapersist_memories(DEV-1668). Left as-is, these docstrings will actively mislead anyone reading them about the current on-disk format.📝 Proposed doc fixes
- Also pre-encodes the full (no-deletion) memories.yaml and populates - embeddings.db so the per-task copy inherits both. Tasks with - ``deleted_kb_ids`` will overwrite memories.yaml + prune embeddings - rows at prepare time; tasks with no deletions reuse the cache - verbatim and pay zero embedding API cost. + Also pre-encodes the full (no-deletion) per-id memory store + (``memories/<id>.md``, DEV-1668) and populates embeddings.db so the + per-task copy inherits both. Tasks with ``deleted_kb_ids`` will + overwrite the memory store + prune embeddings rows at prepare time; + tasks with no deletions reuse the cache verbatim and pay zero + embedding API cost.- """Write ``memories.yaml`` for the no-deletion case AND populate the - embedding rows for each memory so SearchService channel 3 (dense + """Persist the no-deletion-case KB memories (DEV-1668: per-id + ``memories/<id>.md`` via the slayer storage layer) AND populate the + embedding rows for each memory so SearchService channel 3 (dense embedding similarity) can rank them.Also applies to: 552-566
🤖 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/cache.py` around lines 465 - 475, Update the docstrings for _build_async and _materialise_cache_memories to describe the current per-ID storage format: persist_memories writes memories/<id>.md files, and task preparation updates or prunes those files and corresponding embedding rows as needed. Remove all references to writing, overwriting, or inheriting memories.yaml.
🧹 Nitpick comments (2)
tests/test_slayer_otf_reference_build.py (1)
151-152: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert that every encoded memory was materialized.
any(...glob("*.md"))only proves that one Markdown file exists. This test would still pass if two of the three encoded KB memories were missing; assert the expected count or IDs instead.🤖 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_slayer_otf_reference_build.py` around lines 151 - 152, Update the assertion in the Slayer reference-build test to verify that every encoded memory was materialized, rather than only checking that one file exists. Use the expected encoded memory count or explicitly compare the discovered Markdown filenames/IDs against all expected memories, keeping the existing `memories` path and relevant test fixtures.src/bird_interact_agents/eval/autopsy.py (1)
466-470: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLog the exception when KB memory read fails.
logger.warningdoesn't include the exception, so a silent""fallback gives no trace of why the read failed.🔍 Proposed fix
except Exception: # noqa: BLE001 - logger.warning("[autopsy] failed to read memories from %s", slayer_storage_dir) + logger.warning( + "[autopsy] failed to read memories from %s", slayer_storage_dir, exc_info=True, + ) return ""🤖 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/eval/autopsy.py` around lines 466 - 470, Include the caught exception details in the warning emitted by the memories read failure handler in the autopsy logic. Update the except block around read_memories to log the traceback or exception information while preserving the existing fallback return of an empty string.
🤖 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/memory_store_io.py`:
- Around line 76-91: Update read_memories to sort memory files by their numeric
stem rather than lexicographic path order. Replace the current
sorted(mem_dir.glob("*.md")) ordering with a numeric-key sort, preserving
_md_to_memory parsing and return behavior.
In `@tests/test_dev1546_distinct_dim_values.py`:
- Around line 601-612: Update the docstring of
test_motley_slayer_floor_is_at_least_0_9_6 so every paragraph consistently
identifies motley-slayer 0.9.6 as the binding minimum; remove or revise the
stale 0.9.1 rationale and downgrade-failure references while preserving the
current 0.9.6 behavior and rationale.
In `@tests/test_dev1668_unified_inspect.py`:
- Around line 256-261: In the warning-suppression setup around
PydanticJsonSchemaWarning, replace the broad Exception handler with the specific
expected compatibility/import failure, or import the warning class directly when
supported; avoid silently swallowing unrelated errors while preserving
compatibility for versions that lack this symbol.
---
Outside diff comments:
In `@src/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/prompts.py`:
- Around line 74-83: Resolve the conflicting ordering instructions in the prompt
text: explicitly require the agent to call
inspect(reference="memory:help.intro", entity_type="memory", compact=False)
first for syntax onboarding, followed immediately by ask_discovery before any
further introspection or model-building actions. Update the instructions near
the tool guidance so only this sequence is described.
In `@src/bird_interact_agents/slayer_otf/cache.py`:
- Around line 465-475: Update the docstrings for _build_async and
_materialise_cache_memories to describe the current per-ID storage format:
persist_memories writes memories/<id>.md files, and task preparation updates or
prunes those files and corresponding embedding rows as needed. Remove all
references to writing, overwriting, or inheriting memories.yaml.
In `@tests/test_hard8_preprocessor.py`:
- Around line 297-301: Update the docstring for `_seed_memories_and_embeddings`
to describe the current behavior: it creates per-memory `.md` files via
`write_memories_files` and seeds `embeddings.db`, rather than writing
`memories.yaml`.
---
Nitpick comments:
In `@src/bird_interact_agents/eval/autopsy.py`:
- Around line 466-470: Include the caught exception details in the warning
emitted by the memories read failure handler in the autopsy logic. Update the
except block around read_memories to log the traceback or exception information
while preserving the existing fallback return of an empty string.
In `@tests/test_slayer_otf_reference_build.py`:
- Around line 151-152: Update the assertion in the Slayer reference-build test
to verify that every encoded memory was materialized, rather than only checking
that one file exists. Use the expected encoded memory count or explicitly
compare the discovered Markdown filenames/IDs against all expected memories,
keeping the existing `memories` path and relevant test fixtures.
🪄 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: 10225fb1-c1da-4928-845f-34a99168dfa3
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (70)
pyproject.tomlscripts/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/_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_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_v1/agent.pysrc/bird_interact_agents/agents/claude_sdk_otf_v1/prompts.pysrc/bird_interact_agents/eval/autopsy.pysrc/bird_interact_agents/hard8_preprocessor.pysrc/bird_interact_agents/memory_store_io.pysrc/bird_interact_agents/slayer_otf/cache.pysrc/bird_interact_agents/slayer_otf/reference_build.pysrc/bird_interact_agents/slayer_otf/runtime.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_disallowed_slayer_tools.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_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_shared_otf_prompts.pytests/test_slayer_otf_cache.pytests/test_slayer_otf_reference_build.pytests/test_slayer_tool_surface.py
💤 Files with no reviewable changes (2)
- slayer_models/households/memories.yaml
- src/bird_interact_agents/agents/claude_sdk/prompts.py
…dex) hard8 `_copy_memories_and_embeddings` pruned deleted memories' embedding rows by int-parsed `memory:<n>` suffixes only, so a store with `<db>_kb_<n>` string ids (the encode_kb_as_memories scheme) left the dropped memory's dense-embedding row behind — a ghost search hit. Prune by the DROPPED memory ids directly (`memory:<id>`, id-scheme-agnostic), mirroring runtime._prune_deleted_memory_embeddings. Adds a string-id regression test; the existing int-id test still passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… clarity - memory_store_io.read_memories: sort by natural (numeric-suffix) id so KB paragraphs join in KB order (`db_kb_2` before `db_kb_10`), not lexical (CodeRabbit). Add ordering tests. - test_slayer_otf_smoke: assert per-id `memories/*.md` instead of flat `memories.yaml` (integration test, deselected from default run) (Codex). - test_slayer_otf_reference_build: assert ALL 3 encoded KB memories, not just one file (CodeRabbit nitpick). - test_dev1668: narrow `except Exception` -> `except ImportError` (CodeRabbit). - test_dev1546: drop the stale 0.9.1 floor rationale (CodeRabbit). - ainteract_v1 prompt: reword the redundant second "FIRST" (ask_discovery) to "BEFORE building" to remove the ordering ambiguity vs the syntax-onboarding line (CodeRabbit); re-baseline the a-interact SHA. Full non-integration suite green (4386 passed). 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 `@tests/test_slayer_otf_smoke.py`:
- Around line 177-178: Update the assertion in the smoke test to collect the
memory filename stems from `(scratch / "memories").glob("*.md")` and compare
that set exactly with the IDs from `kb_rows`, rather than using `any(...)`; this
must enforce one memory file per KB row and detect missing or unexpected files.
🪄 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: 687364a5-d098-4998-a884-701b50ea1927
📒 Files selected for processing (8)
src/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/prompts.pysrc/bird_interact_agents/memory_store_io.pytests/test_dev1546_distinct_dim_values.pytests/test_dev1668_unified_inspect.pytests/test_memory_store_io.pytests/test_shared_otf_prompts.pytests/test_slayer_otf_reference_build.pytests/test_slayer_otf_smoke.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_shared_otf_prompts.py
🚧 Files skipped from review as they are similar to previous changes (6)
- tests/test_slayer_otf_reference_build.py
- src/bird_interact_agents/agents/claude_sdk_otf_ainteract_v1/prompts.py
- tests/test_dev1546_distinct_dim_values.py
- tests/test_memory_store_io.py
- src/bird_interact_agents/memory_store_io.py
- tests/test_dev1668_unified_inspect.py
Strengthen the OTF smoke layout assertion from `any(memories/*.md)` to check that every `<db>_kb_<id>` from kb_rows was materialized (CodeRabbit), matching the reference-build test. (Other round-3 CodeRabbit threads were stale — already fixed in d27dc11: the 0.9.1 docstring, the reference-build assertion, and the a-interact prompt double-"FIRST" reword.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…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>
…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
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
…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>
…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>
…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>
…dited-model-stores-hoist-inline-kb-defs DEV-1671: KB-materialization checker + reference-first store-cleanup recipe
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/bird_interact_agents/slayer_otf/store_cleanup.py (2)
120-128: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing runs-root helper instead of hand-building the backup path.
edited_models.run_edited_models_archivealready resolves the canonicalruns/root via an internal_runs_root()helper; this backup path re-derives"runs"manually offmain_checkout_root(), which can drift if the runs root ever gets an override (mirroringBIRD_OTF_CACHE_ROOT-style env overrides elsewhere inpaths.py).As per path instructions: "use
bird_interact_agents.paths.*_root()helpers... to resolve paths in the main checkout; never build these paths withrepo_root / ...".♻️ Proposed direction
- backup = (paths.main_checkout_root() / "runs" / "_edited_models_backups" / backup_label - / benchmark / db / instance_id / "edited_models.tar.gz") + backup = (paths.runs_backups_root(benchmark=benchmark) / backup_label + / db / instance_id / "edited_models.tar.gz")(Add a small
runs_backups_root()/similar helper inpaths.pyreusing the same base as_runs_root().)🤖 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 120 - 128, Update the backup path construction in the store cleanup flow to use a dedicated runs-root helper rather than appending "runs" to paths.main_checkout_root(). Add a paths.py helper such as runs_backups_root() that resolves the canonical, override-aware runs root consistently with edited_models._runs_root(), then use it when building the backup path before appending backup_label, benchmark, db, instance_id, and the archive filename.Source: Path instructions
200-219: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffFragile coupling to private
motley-slayerMCP internals.
mcp._tool_manager._tools["query"].fn/mcp._tool_manager._tools["query_nested"].fnandmcp._slayer_engineare private attributes of the MCP server object. Ifmotley-slayerchanges its internal tool-registry structure in a future release, this breaks silently at runtime (AttributeError/KeyError) rather than at review/lint time.Please check whether
motley-slayer0.9.6 exposes a public/documented way to invoke a registered tool by name (e.g. a client-facing call helper), rather than reaching into_tool_manager._tools.🤖 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 200 - 219, Update run_store_query to use the public/documented motley-slayer 0.9.6 mechanism for invoking registered tools by name, replacing direct access to mcp._tool_manager._tools for both query and query_nested. Also avoid relying on the private mcp._slayer_engine attribute; use the corresponding public cleanup API or lifecycle context provided by create_mcp_server, while preserving query arguments and resource cleanup.
🤖 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 `@scripts/check_edited_model_stores.py`:
- Around line 121-124: Validate all tar members before extraction in the
temporary-directory flow around store_dir: reject absolute paths, any normalized
path escaping tmp, symlink members, and hardlink members, then extract only
after validation succeeds. Do not rely on tarfile.extractall(filter="data"),
since the supported Python target lacks that option.
- Around line 6-7: Restore the cache_fp validation in the baseline-model flow
before creating baseline_storage: compare the store’s _edited_models_meta.json
value with the current OTF cache and pass baseline_storage=None on mismatch so
check_store skips comparison. In the tar-loading logic, replace extractall(tmp)
with the same path-safe extraction approach used by edited_models.py.
In `@scripts/migrate_hide_raw_jsonb.py`:
- Around line 71-94: Update the default sweep selection in _run so passing
--cache remains additive to the default reference and archive sweeps, while
preserving --reference, --archives, and --all selection behavior. Adjust the
do_reference and do_archives conditions to avoid disabling them solely because
args.cache is set, and keep do_cache enabled for --cache or --all.
In `@src/bird_interact_agents/slayer_otf/store_cleanup.py`:
- Around line 176-197: Update verify_and_repack() so _EM.save_edited_store(...)
is called only when rep.ok is true; preserve the existing result payload and
query-recording behavior regardless of cleanliness.
- Around line 129-131: Update the tar extraction flow around tarfile.open in the
cleanup function to use the existing guarded extraction pattern from
edited_models.py: apply filter="data" on supported interpreters and otherwise
validate archive members manually before extraction, preventing path traversal
across Python 3.11.0–3.11.3.
---
Nitpick comments:
In `@src/bird_interact_agents/slayer_otf/store_cleanup.py`:
- Around line 120-128: Update the backup path construction in the store cleanup
flow to use a dedicated runs-root helper rather than appending "runs" to
paths.main_checkout_root(). Add a paths.py helper such as runs_backups_root()
that resolves the canonical, override-aware runs root consistently with
edited_models._runs_root(), then use it when building the backup path before
appending backup_label, benchmark, db, instance_id, and the archive filename.
- Around line 200-219: Update run_store_query to use the public/documented
motley-slayer 0.9.6 mechanism for invoking registered tools by name, replacing
direct access to mcp._tool_manager._tools for both query and query_nested. Also
avoid relying on the private mcp._slayer_engine attribute; use the corresponding
public cleanup API or lifecycle context provided by create_mcp_server, while
preserving query arguments and resource cleanup.
🪄 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: 26743235-f8d0-4ce4-80ee-2320a2251a73
📒 Files selected for processing (20)
.claude/skills/clean-edited-model-store/SKILL.mdREADME.mdscripts/cascade_for_combo.pyscripts/check_edited_model_stores.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_otf/store_cleanup.pysrc/bird_interact_agents/slayer_otf/store_kb_checker.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.pytests/test_slayer_otf_smoke.pytests/test_store_cleanup.pytests/test_store_kb_checker.py
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_slayer_otf_smoke.py
Comment on lines
+6
to
+7
| * baseline models — the OTF cache ``slayer_otf_cache/<benchmark>/<db>`` (guarded | ||
| by ``cache_fp`` == the store's ``_edited_models_meta.json``) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate the existing metadata/fingerprint contract before wiring the comparison.
rg -n -C3 --type=py '\bcache_fp\b|_edited_models_meta\.json' src scripts testsRepository: MotleyAI/bird-agents
Length of output: 33472
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== outline ==\n'
ast-grep outline scripts/check_edited_model_stores.py --view expanded || true
printf '\n== relevant sections ==\n'
sed -n '1,220p' scripts/check_edited_model_stores.py | cat -nRepository: MotleyAI/bird-agents
Length of output: 10273
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== target helpers ==\n'
rg -n -C4 'extract|tarfile|baseline_storage|cache_fp|edited_models_meta|check_store' scripts/check_edited_model_stores.py src/bird_interact_agents/slayer_otf/edited_models.py src/bird_interact_agents/slayer_otf/store_kb_checker.pyRepository: MotleyAI/bird-agents
Length of output: 28991
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('scripts/check_edited_model_stores.py')
text = p.read_text()
for needle in ['cache_fp', 'extract', 'tarfile', 'baseline_storage', 'check_store']:
print(f"\n## {needle}")
for i, line in enumerate(text.splitlines(), 1):
if needle in line:
start = max(1, i-3)
end = min(len(text.splitlines()), i+5)
for j in range(start, end+1):
print(f"{j:4d}: {text.splitlines()[j-1]}")
break
PYRepository: MotleyAI/bird-agents
Length of output: 2526
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
p = Path('src/bird_interact_agents/slayer_otf/store_kb_checker.py')
text = p.read_text().splitlines()
for i, line in enumerate(text, 1):
if 'baseline_storage is None' in line or 'check_models(' in line:
start = max(1, i-4)
end = min(len(text), i+10)
print(f"\n## lines {start}-{end}")
for j in range(start, end+1):
print(f"{j:4d}: {text[j-1]}")
PYRepository: MotleyAI/bird-agents
Length of output: 2333
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '100,160p' scripts/check_edited_model_stores.py | cat -nRepository: MotleyAI/bird-agents
Length of output: 3313
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1,190p' scripts/check_edited_model_stores.py | cat -nRepository: MotleyAI/bird-agents
Length of output: 9403
Restore the cache_fp gate and use safe tar extraction.
- Compare the store’s
_edited_models_meta.jsoncache_fpagainst the current cache before creatingbaseline_storage; on mismatch, passbaseline_storage=Nonesocheck_storeskips instead of comparing against a rebuilt cache. - Replace
t.extractall(tmp)with the path-safe extraction used insrc/bird_interact_agents/slayer_otf/edited_models.py.
🤖 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 6 - 7, Restore the
cache_fp validation in the baseline-model flow before creating baseline_storage:
compare the store’s _edited_models_meta.json value with the current OTF cache
and pass baseline_storage=None on mismatch so check_store skips comparison. In
the tar-loading logic, replace extractall(tmp) with the same path-safe
extraction approach used by edited_models.py.
Comment on lines
+121
to
+124
| with tempfile.TemporaryDirectory() as tmp: | ||
| with tarfile.open(archive) as t: | ||
| t.extractall(tmp) | ||
| store_dir = Path(tmp) / db |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the repository's declared Python target supports tarfile's filter argument.
fd -H -t f '^(pyproject\.toml|\.python-version|Dockerfile.*)$' \
--exec sh -c 'echo "### $1"; rg -n "requires-python|python_version|FROM python" "$1" || true' sh {}Repository: MotleyAI/bird-agents
Length of output: 260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant script and nearby logic.
file="scripts/check_edited_model_stores.py"
echo "### outline"
ast-grep outline "$file" --view expanded || true
echo "### relevant lines"
sed -n '1,220p' "$file" | cat -nRepository: MotleyAI/bird-agents
Length of output: 10264
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import inspect
import os
import sys
import tarfile
import tempfile
from pathlib import Path
print("python", sys.version)
print("extractall signature:", inspect.signature(tarfile.TarFile.extractall))
print("filter param supported:", "filter" in inspect.signature(tarfile.TarFile.extractall).parameters)
with tempfile.TemporaryDirectory() as td:
td = Path(td)
archive = td / "traversal.tar"
outside = td / "outside.txt"
outside.write_text("original")
with tarfile.open(archive, "w") as t:
info = tarfile.TarInfo("../outside.txt")
data = b"pwned"
info.size = len(data)
t.addfile(info, io := __import__("io").BytesIO(data))
extract_dir = td / "extract"
extract_dir.mkdir()
try:
with tarfile.open(archive) as t:
t.extractall(extract_dir)
print("extractall: completed")
except Exception as e:
print("extractall: raised", type(e).__name__, e)
print("outside exists:", outside.exists())
if outside.exists():
print("outside contents:", outside.read_text())
print("extract dir contents:", sorted(str(p.relative_to(extract_dir)) for p in extract_dir.rglob("*")))
PYRepository: MotleyAI/bird-agents
Length of output: 414
Restrict tar extraction before unpacking the store. extractall() can write outside tmp via .. entries or link members. The current Python target doesn’t support filter="data", so validate each member and reject absolute paths, traversal, symlinks, and hardlinks before extracting. scripts/check_edited_model_stores.py:121-124
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 122-122: Calling extractall() on a zipfile.ZipFile or tarfile archive without validating member paths lets a crafted entry (e.g. "../../etc/passwd") write outside the destination directory (Zip Slip). Validate each member resolves inside the target directory, or pass a safe filter (tarfile: filter="data" / tarfile.data_filter).
Context: t.extractall(tmp)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(archive-extractall-path-traversal-python)
🪛 Ruff (0.15.21)
[error] 123-123: Uses of tarfile.extractall()
(S202)
🤖 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 121 - 124, Validate all
tar members before extraction in the temporary-directory flow around store_dir:
reject absolute paths, any normalized path escaping tmp, symlink members, and
hardlink members, then extract only after validation succeeds. Do not rely on
tarfile.extractall(filter="data"), since the supported Python target lacks that
option.
Source: Linters/SAST tools
Comment on lines
+71
to
+94
| async def _run(args: argparse.Namespace) -> int: | ||
| # Default: reference stores + archives (the encoder never rebuilds these). | ||
| do_reference = args.reference or args.all or not (args.cache or args.archives) | ||
| do_archives = args.archives or args.all or not (args.cache or args.reference) | ||
| do_cache = args.cache or args.all | ||
|
|
||
| ids = set(args.instance_ids.split(",")) if args.instance_ids else None | ||
| grand = 0 | ||
| if do_reference: | ||
| print(f"slayer_models_otf ({args.benchmark}):") | ||
| grand += await _sweep_store_root( | ||
| paths.slayer_models_otf_root(benchmark=args.benchmark), "reference" | ||
| ) | ||
| if do_cache: | ||
| print(f"slayer_otf_cache ({args.benchmark}):") | ||
| grand += await _sweep_store_root( | ||
| paths.slayer_otf_cache_root(benchmark=args.benchmark), "cache" | ||
| ) | ||
| if do_archives: | ||
| print(f"saved edited-model archives ({args.benchmark}):") | ||
| grand += await _sweep_archives(args.benchmark, ids) | ||
| print(f"\nDone: {grand} raw JSON column(s) newly hidden.") | ||
| return grand | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the migration script and its tests, then inspect relevant sections.
git ls-files | rg 'scripts/migrate_hide_raw_jsonb\.py|tests/.+migrate_hide_raw_jsonb|paths\.py|README|docs' || true
echo '--- script outline ---'
ast-grep outline scripts/migrate_hide_raw_jsonb.py --view expanded || true
echo '--- script lines around run() ---'
sed -n '1,180p' scripts/migrate_hide_raw_jsonb.py
echo '--- tests outline ---'
ast-grep outline tests/scripts/test_migrate_hide_raw_jsonb.py --view expanded || true
echo '--- tests file ---'
sed -n '1,240p' tests/scripts/test_migrate_hide_raw_jsonb.pyRepository: MotleyAI/bird-agents
Length of output: 8498
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect any docs/help text for the flags to compare stated semantics.
rg -n --hidden --glob '!**/.git/**' --glob '!*node_modules*' \
-- 'migrate_hide_raw_jsonb|--cache|--reference|--archives|raw JSON' scripts tests docs README* .Repository: MotleyAI/bird-agents
Length of output: 10091
--cache should not disable the default reference/archive sweep
Passing --cache alone now runs only the cache sweep, but the flag text says it “also” sweeps slayer_otf_cache/<db>, which reads as additive to the default reference+archive migration. Either decouple --cache from the default selection logic or update the flag docs if exclusive behavior is intended.
🤖 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/migrate_hide_raw_jsonb.py` around lines 71 - 94, Update the default
sweep selection in _run so passing --cache remains additive to the default
reference and archive sweeps, while preserving --reference, --archives, and
--all selection behavior. Adjust the do_reference and do_archives conditions to
avoid disabling them solely because args.cache is set, and keep do_cache enabled
for --cache or --all.
Comment on lines
+129
to
+131
| work = Path(work_dir) | ||
| with tarfile.open(archive) as t: | ||
| t.extractall(work) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
printf '\n== Relevant file ==\n'
sed -n '110,150p' src/bird_interact_agents/slayer_otf/store_cleanup.py
printf '\n== Python version constraints ==\n'
git ls-files | rg '(^|/)(pyproject.toml|setup.cfg|setup.py|tox.ini|noxfile.py|requirements.*|Pipfile|pdm\.toml|uv\.toml|README\.md)$' || true
printf '\n== Search for python-version / requires-python ==\n'
rg -n --hidden --glob '!*node_modules*' --glob '!*.git/*' 'requires-python|python_requires|python_version|python = |target-version|Supported Python|Python 3\.' .
printf '\n== tarfile usage ==\n'
rg -n 'extractall\(|tarfile\.open|filter="data"|filter=' src/bird_interact_agents || trueRepository: MotleyAI/bird-agents
Length of output: 6702
🏁 Script executed:
set -euo pipefail
printf '\n== store_cleanup.py context ==\n'
sed -n '1,190p' src/bird_interact_agents/slayer_otf/store_cleanup.py
printf '\n== edited_models.py safe extraction helper ==\n'
sed -n '1,120p' src/bird_interact_agents/slayer_otf/edited_models.py
printf '\n== edited_models.py extraction callsite ==\n'
sed -n '250,300p' src/bird_interact_agents/slayer_otf/edited_models.pyRepository: MotleyAI/bird-agents
Length of output: 15282
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
import sys
print(sys.version)
print((3, 11, 3) < (3, 11, 4))
PYRepository: MotleyAI/bird-agents
Length of output: 213
Use guarded tar extraction here filter="data" isn’t available on the project’s supported Python 3.11.0–3.11.3 range, so mirror the data_filter/manual member-validation fallback used in edited_models.py to block path traversal on older interpreters.
🧰 Tools
🪛 ast-grep (0.44.1)
[error] 130-130: Calling extractall() on a zipfile.ZipFile or tarfile archive without validating member paths lets a crafted entry (e.g. "../../etc/passwd") write outside the destination directory (Zip Slip). Validate each member resolves inside the target directory, or pass a safe filter (tarfile: filter="data" / tarfile.data_filter).
Context: t.extractall(work)
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(archive-extractall-path-traversal-python)
🪛 Ruff (0.15.21)
[error] 131-131: Uses of tarfile.extractall()
(S202)
🤖 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 129 - 131,
Update the tar extraction flow around tarfile.open in the cleanup function to
use the existing guarded extraction pattern from edited_models.py: apply
filter="data" on supported interpreters and otherwise validate archive members
manually before extraction, preventing path traversal across Python
3.11.0–3.11.3.
Source: Linters/SAST tools
Comment on lines
+176
to
+197
| nice_res = await run_store_query(str(scratch_p), nice_query) | ||
| if not results_identical(baseline_result, nice_res, round_ndigits=round_ndigits, | ||
| order_sensitive=order_sensitive): | ||
| return {"ok": False, "reason": "identical-result gate failed", | ||
| "baseline": baseline_result.splitlines()[0] if baseline_result else "", | ||
| "nice": nice_res.splitlines()[0] if nice_res else ""} | ||
|
|
||
| cache_dir = paths.slayer_otf_cache_root(benchmark=benchmark) / db | ||
| baseline_storage = YAMLStorage(base_dir=str(cache_dir)) if cache_dir.exists() else None | ||
| rep = await check_store( | ||
| store_storage=YAMLStorage(base_dir=str(scratch_p)), baseline_storage=baseline_storage, | ||
| db=db, kb_rows=kb_rows, relevant_kb_ids=relevant_kb_ids, winning_query=nice_query, | ||
| instance_id=instance_id, benchmark=benchmark) | ||
|
|
||
| if record_query: | ||
| (scratch_p / "_winning_query.json").write_text(_json.dumps(nice_query)) | ||
|
|
||
| _EM.save_edited_store( | ||
| benchmark=benchmark, db=db, instance_id=instance_id, work_dir=Path(work_dir), | ||
| scratch=scratch_p, deleted_kb_ids=meta.get("deleted_kb_ids", []), | ||
| cache_fp=meta.get("cache_fp", "")) | ||
| return {"ok": True, "clean": rep.ok, "remaining": [f.model_dump() for f in rep.findings]} |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the target function and nearby context
sed -n '120,240p' src/bird_interact_agents/slayer_otf/store_cleanup.py
# Find call sites that may rely on the returned "clean" flag
rg -n '"clean"|check_store\(|verify_and_repack|store_cleanup' src README.md -SRepository: MotleyAI/bird-agents
Length of output: 6448
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the README contract around verify_and_repack
sed -n '286,305p' README.md
# Inspect checker semantics for `clean`
sed -n '1,120p' src/bird_interact_agents/slayer_otf/store_kb_checker.py
sed -n '860,940p' src/bird_interact_agents/slayer_otf/store_kb_checker.py
# Find all call sites of verify_and_repack
rg -n "verify_and_repack\(" src README.md -SRepository: MotleyAI/bird-agents
Length of output: 7009
Gate _EM.save_edited_store(...) on rep.ok
verify_and_repack() repacks any store whose query result is identical, even when check_store() still returns findings. That lets an unclean store be written back to the edited-models archive and breaks the clean-skill contract.
🤖 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 176 - 197,
Update verify_and_repack() so _EM.save_edited_store(...) is called only when
rep.ok is true; preserve the existing result payload and query-recording
behavior regardless of cleanliness.
…+ decouple OTF cache from the pg port Part A — the sudo-free .local_pg cluster never binds :5432 (reserved for the gcloud IAP tunnel to a remote PostgreSQL) and, when its target port is busy or reserved, bumps to the next free port instead of dying with a cryptic pg_ctl bind error. An already-running cluster is adopted (singleton) rather than duplicated. New pure helpers (_port_available / running_cluster_port / resolve_port / _port_change_note) are wired into both provision_and_export() (the bird-interact path) and the setup_local_postgres admin CLI; the resolved port flows into BIRD_PG_*. start_cluster hard-fails on a running-but-unreadable port and wraps the pg_ctl bind race in a clear SystemExit. Part B — the postgres connection is now runtime-supplied, not cached: the per-task datasource reanchor (portable_connection) rewrites host:port:user from BIRD_PG_* (defaults to localhost:5432 so cloud, which does not forward BIRD_PG_* for postgres, stays correct), and persisted references are portabilised to canonical defaults. cache._impl_fingerprint_of no longer embeds pg_conn, so an auto-port change never thrashes the OTF cache. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…visioner-auto-select-a-free-port-so-the DEV-1685: auto-select a free port for the local postgres provisioner (+ decouple OTF cache from the pg port)
…ain/precedence prompts Autopsy: OAuth branch now passes api_key="" (not None) so the SDK cannot auto-load ambient ANTHROPIC_API_KEY and double-bill; subscription path with no CLAUDE_CODE_OAUTH_TOKEN raises _AutopsyAuthUnavailable and skips the post-mortem rather than falling back to the API key. Raw v0 one-shot + a-interact prompts gain the PER-RECORD-vs-DISTINCT row-grain discipline and weighted-sum precedence-parens check (transferred from slayer, no slayer vocab). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…unified-inspect-drop-models_summary' into egor/dev-1668-bird-agents-adopt-unified-inspect-drop-models_summary
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@scripts/build_local_otf_cache.sh`:
- Around line 33-39: Update the PGPORT default in
scripts/build_local_otf_cache.sh to use the non-reserved port established by the
script’s existing contract at the earlier configuration points, while still
allowing an explicit PGPORT override. Keep the surrounding cache fingerprint and
runtime connection behavior unchanged.
In `@src/bird_interact_agents/slayer_pipeline/portable_connection.py`:
- Around line 47-58: Update _rewrite_pg_netloc and all corresponding PostgreSQL
reanchoring call sites to remove connection override parameters from the query
before rebuilding the URL, including host, port, user, password, and related
libpq overrides. Preserve only non-connection query parameters, while ensuring
task preparation consistently reanchors reused connections from BIRD_PG_*
values.
🪄 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: bf3f1828-7161-4c89-b725-2f5281bb2965
📒 Files selected for processing (10)
CLAUDE.mdscripts/build_local_otf_cache.shscripts/setup_local_postgres.pysrc/bird_interact_agents/local_postgres.pysrc/bird_interact_agents/slayer_otf/cache.pysrc/bird_interact_agents/slayer_pipeline/portable_connection.pytests/scripts/test_local_postgres_helpers.pytests/slayer_pipeline/test_portable_connection.pytests/test_local_postgres_provision.pytests/test_slayer_otf_cache.py
Comment on lines
+33
to
39
| # NOTE (DEV-1685): the OTF cache impl fingerprint NO LONGER embeds the postgres | ||
| # connection (host:port:user) — the connection is runtime-reanchored from | ||
| # BIRD_PG_* and persisted references are portabilised to canonical defaults, so | ||
| # the port you build on here does not affect cache reuse across machines/cloud. | ||
| # The 5432 default below is now only about which local server this script talks | ||
| # to; set PGPORT to any free port if 5432 is taken (e.g. by the IAP tunnel). | ||
| PGPORT="${PGPORT:-5432}" |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Keep this self-hosted PostgreSQL instance off reserved port 5432.
PGPORT still defaults to 5432, contradicting Lines 14/22 and the new reserved-port contract. With the IAP tunnel or another server active, startup fails; otherwise this script occupies the tunnel’s reserved port.
Proposed fix
-# The 5432 default below is now only about which local server this script talks
-# to; set PGPORT to any free port if 5432 is taken (e.g. by the IAP tunnel).
-PGPORT="${PGPORT:-5432}"
+# Keep the self-hosted cache builder off the IAP tunnel's reserved port.
+PGPORT="${PGPORT:-5435}"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # NOTE (DEV-1685): the OTF cache impl fingerprint NO LONGER embeds the postgres | |
| # connection (host:port:user) — the connection is runtime-reanchored from | |
| # BIRD_PG_* and persisted references are portabilised to canonical defaults, so | |
| # the port you build on here does not affect cache reuse across machines/cloud. | |
| # The 5432 default below is now only about which local server this script talks | |
| # to; set PGPORT to any free port if 5432 is taken (e.g. by the IAP tunnel). | |
| PGPORT="${PGPORT:-5432}" | |
| # NOTE (DEV-1685): the OTF cache impl fingerprint NO LONGER embeds the postgres | |
| # connection (host:port:user) — the connection is runtime-reanchored from | |
| # BIRD_PG_* and persisted references are portabilised to canonical defaults, so | |
| # the port you build on here does not affect cache reuse across machines/cloud. | |
| # Keep the self-hosted cache builder off the IAP tunnel's reserved port. | |
| PGPORT="${PGPORT:-5435}" |
🤖 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/build_local_otf_cache.sh` around lines 33 - 39, Update the PGPORT
default in scripts/build_local_otf_cache.sh to use the non-reserved port
established by the script’s existing contract at the earlier configuration
points, while still allowing an explicit PGPORT override. Keep the surrounding
cache fingerprint and runtime connection behavior unchanged.
Comment on lines
+47
to
+58
| def _rewrite_pg_netloc( | ||
| connection_string: str, host: str, port: str, user: str | ||
| ) -> str: | ||
| """Rewrite the ``user@host:port`` netloc of a ``postgres(ql)://`` URL, | ||
| PRESERVING the database name (path) and any query, and DROPPING any | ||
| password (the password rides ``PGPASSWORD``, never the URL — it must not | ||
| leak into a persisted YAML or a subprocess argv).""" | ||
| parts = urlsplit(connection_string) | ||
| netloc = f"{quote(user, safe='')}@{host}:{port}" | ||
| return urlunsplit( | ||
| (parts.scheme, netloc, parts.path, parts.query, parts.fragment) | ||
| ) |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Strip PostgreSQL connection overrides from the query string.
The rewritten netloc can still be overridden by preserved libpq parameters such as ?host=...&port=...&user=...; ?password=... also remains persisted. This defeats runtime reanchoring and can connect a reused cache to the wrong server.
Proposed fix
-from urllib.parse import quote, urlsplit, urlunsplit
+from urllib.parse import parse_qsl, quote, urlencode, urlsplit, urlunsplit
+_PG_RUNTIME_QUERY_KEYS = frozenset(
+ {"host", "hostaddr", "port", "user", "password"}
+)
+
def _rewrite_pg_netloc(
connection_string: str, host: str, port: str, user: str
) -> str:
parts = urlsplit(connection_string)
netloc = f"{quote(user, safe='')}@{host}:{port}"
+ query = urlencode([
+ (key, value)
+ for key, value in parse_qsl(parts.query, keep_blank_values=True)
+ if key.lower() not in _PG_RUNTIME_QUERY_KEYS
+ ])
return urlunsplit(
- (parts.scheme, netloc, parts.path, parts.query, parts.fragment)
+ (parts.scheme, netloc, parts.path, query, parts.fragment)
)As per coding guidelines, reanchor PostgreSQL connections from BIRD_PG_* at task preparation.
Also applies to: 61-89, 114-115, 237-238
🤖 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_pipeline/portable_connection.py` around lines
47 - 58, Update _rewrite_pg_netloc and all corresponding PostgreSQL reanchoring
call sites to remove connection override parameters from the query before
rebuilding the URL, including host, port, user, password, and related libpq
overrides. Preserve only non-connection query parameters, while ensuring task
preparation consistently reanchors reused connections from BIRD_PG_* values.
Source: Coding guidelines
Non-integration tests (autopsy, regenerate_autopsies, pydantic_ai smoke, cloud driver/resubmit) mock the Anthropic client but silently depended on an ambient ANTHROPIC_API_KEY to pass _build_anthropic_client's auth-presence check. Removing the key from the env broke 34 tests. Inject an obviously- invalid dummy key via an autouse fixture so the presence check passes deterministically while a real call would 401 loudly. Function-scoped so the auth-resolution tests that delenv / set an OAuth token still win. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ZmeiGorynych
changed the base branch from
egor/dev-1591-slayer-agent-prompts-use-compacttrue-for-broad-search
to
main
August 11, 2026 15:00
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.
Adopts slayer 0.9.6's unified
inspecton theclaude_sdk_otf*agents, and absorbs the 0.9.6 DEV-1658 integration fallout the version bump dragged in. Stacked on #58 (base isegor/dev-1591-slayer-agent-prompts-use-compacttrue-for-broad-search).Closes DEV-1668.
Part A — the tool surface (the issue)
helpis gone (content moved toinspect(reference="memory:help.intro", entity_type="memory", compact=False)), andinspect(reference=None, entity_type="model"|"datasource")is the collection view subsumingmodels_summary/list_datasources.helpremoved unconditionally from the v0 + encoderSLAYER_MCP_TOOLSand the v1_MAIN_NATIVE_BARE(allow-listing a removed tool crashesderive_disallowed_slayer_tools).models_summary+list_datasourceslean-gated — addedmodels_summarytoLEAN_DROP_SLAYER_MCP(the only tool-surface edit; v0/v1 already filter through it and the disallowed set is derived from the effective lean-filtered allow). The encoder is exempt and keeps them; onlyhelpis dropped there.helpFIRST" onboarding line is re-pointed toinspect(memory:help.intro)across the OTF prompt bodies; the v1 lean discovery notes route broad inventory toinspect(reference=None, entity_type="model"). Legacy (lean=False) prose stays byte-identical except the forced help line. dev1603 goldens +test_shared_otf_promptsSHAs re-baselined accordingly.Part B — 0.9.6 DEV-1658 integration (dragged in by the bump)
The bump is mandatory for Part A but 0.9.6 also ships DEV-1658, which broke ~42 bird-agents tests unrelated to the tool surface:
create_mcp_servernow seeds help viastorage.get_memory_row(...), crashing storage-less introspection builds. Fixed by passing_seed_help=Falseat the two metadata-only sites (_slayer_tool_metadata,all_slayer_mcp_tool_names); the runtime query server still seeds soinspect(memory:help.intro)works. Upstream slayer fix tracked in DEV-1669 (worktree created in the slayer repo).memories/<id>.mdand destructively migrates any flatmemories.yamlon open. New top-levelbird_interact_agents/memory_store_io.pypersists/reads/copies KB memories through slayer's storage layer via_save_memory_row(preserving the<db>_kb_<n>ids and the encoder's EPOCHcreated_at, which the publicsave_memorycan't). Migrated every writer (runtime / cache / reference_build), the autopsy reader, the hard8 copier, theverify_kb_coveragescript guard, and the test fixtures off flatmemories.yaml.slayer_models/households/memories.yamlwas the only committed flat memory file; migrated deterministically to 24 per-idmemories/*.md(created_at preserved) so opens are idempotent. The other 27slayer_models/DBs commit no memories.Tests
tests/test_dev1668_unified_inspect.py(surface contract: help absent everywhere; lean-gating; encoder exemption; functionalinspect(reference=None)+memory:help.introreachability).tests/test_memory_store_io.py(round-trip, replace semantics, legacy-flat migration).🤖 Generated with Claude Code
Summary by CodeRabbit
inspect(reference="memory:help.intro", entity_type="memory", compact=False)and removed the deprecatedhelptool from available SLayer tool surfaces.