DEV-1478: cloud DB conn re-anchor + Mode-B filter normalization - #6
Merged
ZmeiGorynych merged 6 commits intoMay 28, 2026
Conversation
…-1478)
Failure-mode analysis of the 53-instance cloud run found every agent
(and 15-30 of each DB's ~50 setup encoders) hit "Database error: unable
to open database file" on its SLayer query tool — the dominant driver of
the apparent OTF-vs-hand-audited regression (23 vs 30), which is really a
blind-cloud-vs-self-validating-local confound plus brittle ex_base
comparator artifacts and gold defects.
Root cause: the on-the-fly deterministic cache bakes in an ABSOLUTE
sqlite path from the machine that built it (e.g.
sqlite:////home/<user>/.../mini-interact/<db>/<db>.sqlite). The OTF
storage paths re-anchored that path via to_portable→resolve or
resolve-only, both of which PASS A FOREIGN ABSOLUTE PATH THROUGH
UNCHANGED — so a locally-built cache transported into a cloud container
(DB at /data/mini-interact/...) kept the nonexistent local path. Phase-1
eval still worked (it resolves data_path_base from $BIRD_DB_PATH on a
different code path), so only the agent's + encoder's interactive query
tool was blinded. The recursive on-the-fly path was immune because
prepare_task_storage force-rewrites via an _expected_connection_string
helper.
Fix: new `reanchor_connection_string(conn, db, root)` in
slayer_pipeline.portable_connection — relative form resolves against the
root (path preserved, unchanged behaviour); absolute form (incl. the
malformed 5-slash variant) is force-rewritten to the canonical
<root>/<db>/<db>.sqlite ($BIRD_DB_PATH wins). All three OTF datasource
re-anchor sites now delegate to it:
- hard8_preprocessor.build_task_variant_storage (per-task agent storage)
- slayer_otf/runtime.py (recursive on-the-fly per-task; dedup of its
private _expected_connection_string)
- slayer_otf/reference_build.py::_resolve_datasource_for_build
(setup-encode build — Codex follow-up; the encoder's own query tool
was hitting the same error)
The commit-time _portabilise_datasource still emits the relative portable
form for the uploaded/committed reference (fetch-back re-anchors locally).
Tests (1209 passed, +13): portable_connection unit cases for
expected/reanchor (relative resolve, stale-foreign-absolute force,
$BIRD_DB_PATH override, 5-slash normalisation, non-sqlite passthrough);
hard8 + reference_build integration cases asserting a foreign-absolute
cache path is re-anchored to the current root with no foreign-path leak.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…lters (DEV-1478)
The 53-task failure analysis found case/whitespace mismatch on text
filters causing wrong_result losses (e.g. an agent's `= 'Apartment'`
missing 'apartment'/' apartment '). The NL questions carry no casing
info, so a text-equality filter semantically matches all variants;
relying on the LLM to remember LOWER(TRIM) is unreliable. Fix:
deterministically normalize text-equality FILTER predicates wherever a
SLayer query flows through this repo's tools.
New `slayer_pipeline/filter_normalization.py`:
* `normalize_mode_b_filter` — Mode-B filters are Python-expression
syntax, so reuse SLayer's own `_preprocess_sql_operators` (=→==, <>→!=,
keyword-lowercase; user owns SLayer, reuse over reinvent) then
`ast.parse`/NodeTransformer/`ast.unparse`. For in-scope comparisons
(single op Eq/NotEq/In/NotIn, RHS all string literals without `{...}`
placeholders, LHS a column ref possibly already lower/trim-wrapped):
canonicalize the LHS to `lower(trim(col))` AND lowercase the
literal(s). Idempotent (canonical form is a fixed point); fail-safe
no-op on anything out of scope (LIKE, colon/func aggregations, chained
compares, numeric/placeholder RHS, non-column LHS, unparseable).
* `normalize_query_payload` (submit path: dict | list of stages) and
`normalize_tool_filters` (facade: dispatch by tool — query→filters,
query_nested→queries, create_model→query stages, edit_model→
source_queries; deep-copied, never mutates input). Mode-A
add_filters/remove_filters left alone.
Applied at every in-repo SLayer-query surface (no SLayer changes):
* `_submit.py::submit_slayer_query` — normalize before compile (the
scored path for BOTH adapters). Recorded `submitted_query` stays the
agent's original DSL; only the compiled `submitted_sql` reflects it.
* otf_encode `_process_tool_call` — normalize tool_args before the
literal-existence validator (which then runs post-transform; its
casefold-when-wrapped branch matches the normalized filter).
* recursive adapter `_build_shared_slayer_server` — a new
process_tool_call normalize-then-forward hook, for apples-to-apples
parity with the OTF path's exploratory queries.
Mode-A SQL the encoder writes (Column.sql/filter CASE-WHENs) is the
encoder's responsibility: STYLE_GUIDE STRING-NORM is strengthened and
**scoped emphatically to FILTER predicates only** (WRONG/RIGHT
projection example — never normalize a projected/grouped/joined column).
Prompt hygiene: rephrased every real-households exemplar embedded in
the STYLE_GUIDE / SETUP_ENCODER prompt (Tenure_Type / Income_Bracket /
Dwelling_Class caveats; the host-choice, cross-model, and
value-illustration worked examples) into SYNTHETIC fictional schemas
that preserve structure — no eval-set table/column/value/KB-id in any
prompt (per feedback_prompts_synthetic_examples_only).
Tests: 33 cases in tests/slayer_pipeline/test_filter_normalization.py
(== / SQL-style = / <> / IN / not in / already-wrapped-uppercase-literal
/ partial wrap / dotted col / {placeholder} / numeric / compound /
LIKE / colon-agg / combined-agg-boundary / unparseable no-ops; per-tool
arg dispatch; deep-copy non-mutation). Full non-integration suite: 1241
passed, 92 skipped, 19 deselected (was 1209; +32). Codex-reviewed (plan
+ diff).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughDeterministic Mode-B filter normalization and portable SQLite connection re-anchoring are added, normalization is integrated into submit/encoder/recursive MCP flows, re-anchoring is applied in build/runtime/hard8 paths, prompt guidance is updated, dual-eval golds and metrics are wired, and tests cover these behaviors. ChangesFilter Normalization and Connection String Re-anchoring
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
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/agents/pydantic_ai_otf_encode/prompts.py`:
- Line 143: In the prompt text containing "PREPEND a 1–2 sentence caveat"
replace the Unicode EN DASH (–) with an ASCII hyphen (-) so the phrase becomes
"PREPEND a 1-2 sentence caveat" to satisfy Ruff RUF001; update the string
literal in prompts.py where that exact phrase appears.
🪄 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: de99ef38-32d6-4433-870a-4b54ed3de2a2
📒 Files selected for processing (13)
src/bird_interact_agents/agents/_submit.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/prompts.pysrc/bird_interact_agents/agents/pydantic_ai_recursive/agent.pysrc/bird_interact_agents/hard8_preprocessor.pysrc/bird_interact_agents/slayer_otf/reference_build.pysrc/bird_interact_agents/slayer_otf/runtime.pysrc/bird_interact_agents/slayer_pipeline/filter_normalization.pysrc/bird_interact_agents/slayer_pipeline/portable_connection.pytests/slayer_pipeline/test_filter_normalization.pytests/slayer_pipeline/test_portable_connection.pytests/test_hard8_preprocessor.pytests/test_slayer_otf_reference_build.py
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
|
Actionable comments posted: 0 |
… (DEV-1478) Cloud `collation._row_to_task_result_row` omitted the four dual-eval fields, so `results.db` showed NULL for the original-gold score on every row even though the row JSON / eval.json carried the scored values. Add the fields (mirroring the local run.py writer) and emit the dual aggregate in the cloud eval.json for parity. Also make `apply_audited_gold_overlay` set `original_sol_sql` for EVERY task (not just edited/unrecoverable), so every row dual-evaluates against the canonical gold; evaluate_dual_gold short-circuits identical golds so clean rows cost no extra evaluator call. Co-Authored-By: Claude Opus 4.7 (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 `@src/bird_interact_agents/cloud/collation.py`:
- Around line 129-133: The original-counting logic is inconsistent:
`phase1_count_original`/`p1_original` is currently computed across all rows
while `n_dual` only counts rows with non-NULL audited values, causing mismatched
denominators; change the original-pass counts to use the same eligibility filter
as audited (i.e., only rows where r.get("phase1_passed_audited") is not None).
Concretely, update `p1_original`/`phase1_count_original` to sum only over
`canonical_rows` where `phase1_passed_audited` is not None and
`phase1_passed_original` is true, keeping `n_dual`, `p1_audited`, and
`phase1_rate_original` calculations consistent; apply the same fix to the other
occurrence around the `phase1_rate_original` computation (the block referenced
at the second location).
🪄 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: fd46615f-ef19-4949-9017-7f90684adbb8
📒 Files selected for processing (4)
src/bird_interact_agents/cloud/collation.pysrc/bird_interact_agents/harness.pytests/cloud/test_collation.pytests/test_dual_eval.py
…V-1478) `_check_slayer_setup_present` used to fail-fast when the REQUIRED `slayer_otf_cache` was absent. Since that cache is free + deterministic (no LLMs), build it locally via `ensure_db_cache` and proceed instead of erroring. Non-buildable required artifacts (e.g. `slayer_models`) still fail fast. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
tests/cloud/test_driver.py (1)
707-717: ⚡ Quick winAssert
ensure_db_cachecall kwargs in the new auto-build tests.These tests currently prove that building is triggered, but not where it builds. Add assertions for
cache_root,mini_interact_root, andforce=Falseto lock the contract and catch path-root regressions early.✅ Suggested test strengthening (example pattern)
- built: list[str] = [] + built: list[str] = [] + seen_kwargs: list[dict] = [] async def _fake_cache(db, **kw): built.append(db) + seen_kwargs.append(kw) @@ assert dbs == ["db_a", "db_b"] assert sorted(built) == ["db_a", "db_b"] + assert all(kw["cache_root"] == driver.paths.slayer_otf_cache_root() for kw in seen_kwargs) + assert all(kw["mini_interact_root"] == driver.paths.mini_interact_root() for kw in seen_kwargs) + assert all(kw["force"] is False for kw in seen_kwargs)As per coding guidelines
**/*.py: “Never depend on a path insiderepo_rootfor gitignored data ... Always usebird_interact_agents.paths.*_root()helpers...”.Also applies to: 730-739, 974-983
🤖 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/cloud/test_driver.py` around lines 707 - 717, The test replaces driver.ensure_db_cache with _fake_cache but only asserts which DBs were built; update the fake to capture kwargs and assert that ensure_db_cache was called with cache_root and mini_interact_root coming from the bird_interact_agents.paths.*_root() helpers and force=False; locate the usage in this test around driver._check_slayer_setup_present and FakeSubmitArgs and add assertions that the captured kwargs include cache_root, mini_interact_root, and force==False (and apply the same pattern to the other similar test blocks at the indicated ranges).src/bird_interact_agents/cloud/driver.py (1)
245-255: ⚡ Quick winUse a single event loop for
_build_missing_otf_caches
ensure_db_cache’s locking is loop-bound (_get_lockkeysasyncio.Lockby(id(event_loop), db)), so the current per-DBasyncio.run(...)recreates locks on a new event loop each iteration (extra churn + harder reasoning). Switch_build_missing_otf_cachestoasyncio.runonce andawait ensure_db_cache(...)for alldbswithin that same loop.🤖 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/cloud/driver.py` around lines 245 - 255, The loop currently calls asyncio.run(...) per-db which creates a new event loop each iteration; change _build_missing_otf_caches to run asyncio.run once over an async helper that iterates the dbs and awaits ensure_db_cache(...) for each db so all locks from _get_lock (which keys asyncio.Lock by (id(event_loop), db)) are created on the same loop; specifically, create an async inner function (e.g., async def _run_all()) that loops over dbs and awaits ensure_db_cache(db, cache_root=cache_root, mini_interact_root=mi_root, force=False) and then call asyncio.run(_run_all()) instead of per-db asyncio.run calls.
🤖 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 `@src/bird_interact_agents/cloud/driver.py`:
- Around line 245-255: The loop currently calls asyncio.run(...) per-db which
creates a new event loop each iteration; change _build_missing_otf_caches to run
asyncio.run once over an async helper that iterates the dbs and awaits
ensure_db_cache(...) for each db so all locks from _get_lock (which keys
asyncio.Lock by (id(event_loop), db)) are created on the same loop;
specifically, create an async inner function (e.g., async def _run_all()) that
loops over dbs and awaits ensure_db_cache(db, cache_root=cache_root,
mini_interact_root=mi_root, force=False) and then call asyncio.run(_run_all())
instead of per-db asyncio.run calls.
In `@tests/cloud/test_driver.py`:
- Around line 707-717: The test replaces driver.ensure_db_cache with _fake_cache
but only asserts which DBs were built; update the fake to capture kwargs and
assert that ensure_db_cache was called with cache_root and mini_interact_root
coming from the bird_interact_agents.paths.*_root() helpers and force=False;
locate the usage in this test around driver._check_slayer_setup_present and
FakeSubmitArgs and add assertions that the captured kwargs include cache_root,
mini_interact_root, and force==False (and apply the same pattern to the other
similar test blocks at the indicated ranges).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 832c1547-8273-4df7-84d5-52f9677fce0c
📒 Files selected for processing (2)
src/bird_interact_agents/cloud/driver.pytests/cloud/test_driver.py
CodeRabbit quick-win on PR #6: lock the worktree-safe contract by asserting `ensure_db_cache` is called with the resolved `cache_root` / `mini_interact_root` and `force=False`, catching wrong-root regressions. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Actionable comments posted: 0 |
This was referenced May 29, 2026
Merged
4 tasks
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.
Follow-up to #4 (merged). Two commits that landed on the branch after #4 merged — opening a fresh PR so CI + CodeRabbit run on them.
Summary
c1c77c4— re-anchor OTF datasourceconnection_stringin the cloud. The on-the-fly deterministic cache bakes in an absolute local sqlite path; the OTF storage paths only resolved relative connection strings, so a locally-built cache transported into a cloud container kept a nonexistent/home/...path → every agent's + setup-encoder's SLayerquerytool failedunable to open database file(all 53 instances in the last full run flew blind). Newreanchor_connection_stringforce-rewrites stale-foreign-absolute paths to<root>/<db>/<db>.sqlite; wired into all three OTF storage sites (build_task_variant_storage,slayer_otf/runtime.py,reference_build.py). Phase-1 eval was unaffected (different DB path), which is why this hid behind plausible-looking scores.e224616— deterministic case/whitespace normalization of Mode-B query filters. NL questions carry no casing info, so a text-equality filter should match all variants ('Apartment'/'apartment'/' apartment '). Newslayer_pipeline/filter_normalization.pyparses Mode-B (reusing SLayer's own_preprocess_sql_operators), and for in-scope==/!=/in/not inagainst string literals wraps the column inlower(trim(...))+ lowercases the literal. Applied at every in-repo query surface:submit_slayer_query, the OTF-encodeprocess_tool_callfacade (before the literal-existence validator), and a new recursive-adapter facade. Mode-A SQL the encoder writes is governed by a strengthened STYLE_GUIDE rule scoped emphatically to filter predicates only. All in-prompt exemplars rephrased to synthetic schemas (no eval-set content).Test plan
1241 passed, 92 skipped, 19 deselectedtests/slayer_pipeline/test_portable_connection.py,tests/test_hard8_preprocessor.py,tests/test_slayer_otf_reference_build.py(connection re-anchor);tests/slayer_pipeline/test_filter_normalization.py(33 cases)wrong_resultlosses are gone🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests