OTF encoder failure-mode fixes vs hand-audited households reference (DEV-1478) - #4
Merged
ZmeiGorynych merged 12 commits intoMay 27, 2026
Conversation
- _STYLE_GUIDE constant injected into both KB / setup encoder prompts (STRING-NORM, CROSS-COLUMN REFERENCES, HOST-CHOICE, PEER-KB DEDUP, INLINE-DON'T-NAME); value-illustration ownership rewritten as a decision table (CASE A/B/C); defensive defer when a child is "NOT encoded"; LITERAL-EXISTS cross-references the new validator. - New literal-existence pre-save validator (sqlglot-based, peels LOWER/TRIM wrappers, joins-aware single-hop column resolution, prefers Column.sampled_values with conservative fallback on Column.sampled — skips when values contain commas to avoid false positives). Combined SQL + literal feedback in one tool result; fail-OPEN on harness errors. - New _format_existing_kb_tagged_entities_block helper rendered into both encoder prompts so PEER-KB DEDUP has a concrete signal (canonical lower-id KB wins via the topo sort). - CLAUDE.md: Opus/Sonnet households smoke recipe + eyeball assertions for the audit's regression targets. SLayer-side dependency (Column.sampled_values) tracked in DEV-1480 but not required to ship: the validator reads sampled_values when present, otherwise falls back to a comma-safe parse of sampled. 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:
📝 WalkthroughWalkthroughThis PR implements literal-existence validation to prevent agents from proposing non-existent column values in SQL predicates, combined with peer-KB deduplication that renders already-tagged entities into encoder prompts. A shared style guide unifies rules across both KB and setup encoders. ChangesDEV-1478: Literal-Existence Validation and Peer-KB Deduplication
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 unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/bird_interact_agents/agents/pydantic_ai_otf_encode/factories.py (1)
799-800: ⚡ Quick winMake same-
kb_idordering deterministic in the rendered block.Line 799 currently sorts only by
kb_id, so entities sharing akb_idcan appear in storage-dependent order. That can make prompt text nondeterministic across runs.Proposed fix
- rows.sort(key=lambda r: r[0]) + rows.sort(key=lambda r: (r[0], r[1])) return "\n".join(line for _, line in rows)🤖 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/pydantic_ai_otf_encode/factories.py` around lines 799 - 800, The current sorting uses rows.sort(key=lambda r: r[0]) which groups by kb_id but leaves items with the same kb_id in storage-dependent order; change the sort to a deterministic multi-key sort (e.g., sort by kb_id then by the entity-specific field or the rendered line) so identical kb_id groups are consistently ordered — update the sort call (the lambda used in rows.sort and the tuple elements used later when joining) to use a tuple key like (r[0], r[1]) or an explicit stable secondary key derived from the row to guarantee deterministic output prior to the return "\n".join(...) step.
🤖 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/agent.py`:
- Around line 491-540: The current logic in _collect_literal_problems returns
early if storage.get_model(host_name) is None, which skips validation for
create_model because the new model won't exist yet; modify
_collect_literal_problems (and related helper code) so that when
_host_model_name(...) indicates "create_model" and storage.get_model(...) yields
None, you synthesize a lightweight host model from tool_args (e.g., use
tool_args["columns"] / their datasource/column names) instead of returning,
allowing the existing literal-walking logic to resolve columns against that
synthetic model; implement a small helper (e.g., _build_model_from_create_args)
and use it in _collect_literal_problems so create_model validations run even
when the model isn't persisted yet.
- Around line 626-636: The except block around calling _collect_literal_problems
currently swallows earlier SQL validation failures (sql_problems) and proceeds
to perform the write; change it so that on exception you still log via
logger.exception but preserve and act on existing sql_problems: if sql_problems
is non-empty return/raise the same failure behavior used when problems exist
(i.e., do not call call_tool), otherwise fall back to calling call_tool(name,
tool_args, None); update the except to check sql_problems before calling
call_tool so known-invalid SQL is not bypassed.
---
Nitpick comments:
In `@src/bird_interact_agents/agents/pydantic_ai_otf_encode/factories.py`:
- Around line 799-800: The current sorting uses rows.sort(key=lambda r: r[0])
which groups by kb_id but leaves items with the same kb_id in storage-dependent
order; change the sort to a deterministic multi-key sort (e.g., sort by kb_id
then by the entity-specific field or the rendered line) so identical kb_id
groups are consistently ordered — update the sort call (the lambda used in
rows.sort and the tuple elements used later when joining) to use a tuple key
like (r[0], r[1]) or an explicit stable secondary key derived from the row to
guarantee deterministic output prior to the return "\n".join(...) step.
🪄 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: f74cc91d-3fff-4359-8fbd-ead442b4df39
📒 Files selected for processing (10)
CLAUDE.mdsrc/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/factories.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/prompts.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/setup_encoder.pytests/test_literal_existence_validation.pytests/test_pydantic_ai_otf_encode_agent.pytests/test_pydantic_ai_otf_encode_kb_encoder_prompt.pytests/test_pydantic_ai_otf_encode_kb_to_slayer.pytests/test_pydantic_ai_otf_encode_setup_encoder.py
- create_model literal validation gap (CodeRabbit thread 1): synthesize a lightweight host from `tool_args` when `storage.get_model(<new>)` returns None, so literal-existence checks still fire on brand-new models (the previous early-return silently skipped them). - Preserve SQL-gate failures on literal-collector exception (CodeRabbit thread 2 + Codex): drop only the literal-gate results in the except; the `if problems:` branch below now correctly blocks the write when the SQL gate already flagged invalid SQL — a literal-collector harness bug no longer turns into a bypass. - Deterministic same-kb_id sort in the existing-kb-tagged-entities block (CodeRabbit nitpick): tie-break on the rendered line so two entities sharing a kb_id (R-FILTER's Column + ModelMeasure pair) appear in stable order across rebuilds; prevents prompt drift. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- create_model(query=[...]) backing-query gap: extract the inner literal walker into `_check_sql_literals`, add a per-stage walker that resolves each query stage's `filters` / `measures.formula` / `dimensions[].filter` against the STAGE's own `source_model` (not the outer create_model's name). Covers R-MULTISTAGE and R-EXISTS payloads — the remaining DEV-1478 surface that slipped through. - _format_deps_block now appends the deferred child's `notes` (truncated to ~200 chars) so the defensive-defer prompt has a real ambiguity statement to quote, not just the status word `deferred`. Lets the dependent KB's notes distinguish "child deferred because ambiguous" from "child errored during encode" as the prompt promised. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…-1478) Codex follow-up (PR #4, second-pass): the existing-kb-tagged-entities block was listing ALL tagged entities regardless of kb_id. But setup encoders run concurrently via asyncio fan-out, so completion order doesn't match topo-id order — a higher-id KB whose LLM call finishes first could appear in storage before a lower-id KB's encoder runs, and the lower-id KB would then see the higher-id entity and (per the prompt's "lower-id is canonical" rule) might defer as a duplicate of the non-canonical higher id. Add a `current_kb_id` kwarg to `_format_existing_kb_tagged_entities_block` that filters entries to `kb_id < current_kb_id` — strictly less-than so same-kb_id sibling writes from concurrent encoders don't leak in either. Both call sites (setup_encoder + factories' task-time runner) thread the current KB id. Regression test pins the strict-less-than filter for {3, 10, 15} stored with current_kb_id=10. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
J — query-stage measures accept both dict and string form:
SlayerQuery measures may be `{"formula": "..."}` OR a bare
`"col:sum"` / `"CASE WHEN ... END:sum"` string. `_iter_query_stage_targets`
now walks both shapes so literal-bearing string measures inside
`create_model(query=[...])` stages don't slip through.
K — thread data_source through storage.get_model lookups:
`_collect_literal_problems` now scopes every `storage.get_model(name)`
call to `tool_args["data_source"]` so a same-named model in another
datasource (YAMLStorage's priority resolution) can't shadow the right
one. When data_source is absent (the corner case the existing
fallback test pins), falls back to an unscoped lookup. Same number of
get_model calls as before — no count regression.
L — drop the sampled-text naive-split fallback entirely:
The `\d,\d{3}` heuristic caught numeric thousands separators but
NOT values with non-numeric commas (e.g. "Springfield, IL"). Naive
split-on-`", "` would mis-fragment those and false-positive legitimate
literals. The fallback is now: when `Column.sampled_values` is None,
SKIP the check. DEV-1480 ships `sampled_values` and makes the check
effective without the false-positive risk; until then, the validator
correctly skips rather than incorrectly rejecting valid writes.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.py (1)
330-333: 💤 Low valueDead code:
_OVERFLOW_PATTERNand_RANGE_PATTERNare defined but never used.These regex patterns were presumably intended for parsing the human-readable
Column.sampledtext, but the current_sampled_setimplementation (lines 421-440) correctly skips whensampled_valuesisNoneand doesn't fall back to parsingsampledtext. The patterns serve no purpose.🧹 Remove unused patterns
-# Overflow / range patterns in the `sampled` text — when present, the -# structured list isn't available so the literal-existence check skips. -_OVERFLOW_PATTERN = re.compile(r"^>\s*\d+\s*distinct\b", re.IGNORECASE) -_RANGE_PATTERN = re.compile(r"\.\.") # "min .. max"🤖 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/pydantic_ai_otf_encode/agent.py` around lines 330 - 333, Remove the unused regex constants _OVERFLOW_PATTERN and _RANGE_PATTERN from the module: they are dead code and not referenced by the _sampled_set function or anywhere else; update the file by deleting the definitions for _OVERFLOW_PATTERN and _RANGE_PATTERN and run tests/lint to ensure no remaining references, keeping _sampled_set and Column.sampled logic unchanged.tests/test_literal_existence_validation.py (1)
736-758: 💤 Low valueTest docstrings are misleading: overflow/range patterns are dead code.
These tests pass correctly because
sampled_valuesisNone(not set viaobject.__setattr__), so_sampled_setreturnsNoneand the check is skipped. However, the test names and docstrings imply the overflow/range regex patterns are being matched, when in fact_OVERFLOW_PATTERNand_RANGE_PATTERNare never used in the implementation.The tests document the correct behavior (skip when structured data is unavailable), but the names suggest pattern-based detection that doesn't exist.
📝 Consider clarifying test names/docstrings
`@pytest.mark.parametrize`("sampled", [ "> 20 distinct", "> 50 distinct", "> 100 distinct", ]) -async def test_sampled_overflow_skip(tmp_path, sampled): - """Overflow marker → cannot check membership → skipped.""" +async def test_sampled_values_absent_skip_even_with_overflow_marker(tmp_path, sampled): + """sampled_values=None → membership check skipped (sampled text ignored)."""🤖 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_literal_existence_validation.py` around lines 736 - 758, Rename and update the test to reflect that the skip is due to sampled/structured data being unavailable, not regex matching: change the test name test_sampled_overflow_skip to something like test_sampled_unavailable_skip (or test_sampled_none_skips) and update the docstring to state "sampled_values is None (structured data unavailable) → check skipped" and/or assert that _collect_literal_problems (the function under test) returns [] when sampled values are not set; optionally simplify the parametrization (you can keep or remove the overflow strings like "> 20 distinct" since the patterns _OVERFLOW_PATTERN/_RANGE_PATTERN are not used).
🤖 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/agents/pydantic_ai_otf_encode/agent.py`:
- Around line 330-333: Remove the unused regex constants _OVERFLOW_PATTERN and
_RANGE_PATTERN from the module: they are dead code and not referenced by the
_sampled_set function or anywhere else; update the file by deleting the
definitions for _OVERFLOW_PATTERN and _RANGE_PATTERN and run tests/lint to
ensure no remaining references, keeping _sampled_set and Column.sampled logic
unchanged.
In `@tests/test_literal_existence_validation.py`:
- Around line 736-758: Rename and update the test to reflect that the skip is
due to sampled/structured data being unavailable, not regex matching: change the
test name test_sampled_overflow_skip to something like
test_sampled_unavailable_skip (or test_sampled_none_skips) and update the
docstring to state "sampled_values is None (structured data unavailable) → check
skipped" and/or assert that _collect_literal_problems (the function under test)
returns [] when sampled values are not set; optionally simplify the
parametrization (you can keep or remove the overflow strings like "> 20
distinct" since the patterns _OVERFLOW_PATTERN/_RANGE_PATTERN are not used).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 352a278e-cf5e-4e05-85d9-d458fbdf2234
📒 Files selected for processing (5)
src/bird_interact_agents/agents/pydantic_ai_otf_encode/agent.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/factories.pysrc/bird_interact_agents/agents/pydantic_ai_otf_encode/setup_encoder.pytests/test_literal_existence_validation.pytests/test_pydantic_ai_otf_encode_setup_encoder.py
🚧 Files skipped from review as they are similar to previous changes (2)
- src/bird_interact_agents/agents/pydantic_ai_otf_encode/setup_encoder.py
- tests/test_pydantic_ai_otf_encode_setup_encoder.py
…V-1478) M (Codex major): `LOWER(TRIM(col)) = 'lit'` predicates would false-positive when sampled values carry surrounding whitespace (e.g. SQLite's underlying ` yes `). Apply `.strip().casefold()` to both sides of the membership check so the validator mirrors what SQLite's `LOWER(TRIM(<col>)) = '<lit>'` does at runtime. Regression test exercises both EQ and IN with surrounding-whitespace sampled values. N (CodeRabbit nitpick): `_OVERFLOW_PATTERN` / `_RANGE_PATTERN` are dead code after Fix L dropped the sampled-text naive-split fallback. `_sampled_set` now only reads `Column.sampled_values`. Deleted the unused regex constants and the now-unused `import re`. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
agent.py now imports sqlglot at module load for the literal-existence
pre-save validator. But sqlglot was only declared in the `dev` extra,
so a runtime install with the `pydantic-ai`/`slayer` extras would
fail with `ModuleNotFoundError: sqlglot` when first importing the
agent module. The test suite path goes through `[dev]`, masking the
break.
Move sqlglot to the top-level `dependencies` list (where it really
always belonged — sar_audit/compare.py also imports it). Drop it from
`dev`. Resynced uv.lock. Verified the runtime-only install path:
uv run --no-dev --extra pydantic-ai --extra slayer python -c \\
"from bird_interact_agents.agents.pydantic_ai_otf_encode import agent"
imports cleanly.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
SLayer 0.6.10 (DEV-1480) ships Column.sampled_values — the structured top-N sample list the OTF encoder's literal-existence validator already reads defensively via getattr at agent.py::_sampled_set. With the pin lifted, the validator now has authoritative ground truth to reject KB-text predicates that don't match the actual column values (the KB 21 / 42 broken-predicate regression target from this PR). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Re-comparing OTF vs hand-audited households showed the prior bird-interact-cloud
defaults silently picked the wrong target: patience=3 cuts user-sim retries
much shorter than the audited-gold reference runs were measured at (500),
and use_audited_gold_sql=false evaluates against an un-audited gold that
includes KB-described predicates the agent has no way to ground.
New defaults:
--patience 500 (was 3)
--use-audited-gold-sql True (was False; BooleanOptionalAction +
--no-use-audited-gold-sql opt-out)
--require-audited-gold True (new; BooleanOptionalAction)
`--require-audited-gold` fails submit at the CLI layer when any passed
instance_id has no row in `<audited_gold>/<db>/<db>_audited.jsonl` — the
prior fallback to the original gold mid-cloud-run was a foot-gun (the
cluster came up, encoded ~10 min, and only THEN logged the warning).
The check itself lives in
`src/bird_interact_agents/cloud/_audited_gold_check.py` so the cli stays
slim and the helper is independently testable.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…EV-1478) After the audited-gold + patience-500 default flip closed most of the OTF-vs-hand-audited households gap (8 → 7 / 15), the remaining failures matched the original audit's cross-ref hygiene modes (correlated subqueries instead of declared joins, host-choice that adds undeclared joins, missing sampled-value caveats). User pushed back hard on my initial "inline composite chains" proposal: SLayer's value is in the DAG of cross-refs; the hand-audited inlining is a pre-0.6.9 DEV-1410 workaround. Post-0.6.9 layered named refs are first-class. Code: * `_collect_peer_kb_entries` (new, factories.py): typed `PeerKbEntry` collector that walks every kb-tagged Column / Measure / Aggregation / Model in storage and computes a per-peer reach map via BFS over the declared join graph (composite `join_pairs` count as one edge; multi- hop chains return shortest-distance per host; cycles short-circuit via the BFS visited set). * `_format_existing_kb_tagged_entities_block` (factories.py): refactored to delegate to the collector + render each peer as a single line with labeled markers `entity_ref=<db>.<model>[.<leaf>]` and `reachable_from_host: host(hops), host(hops), ...`. The legacy leading segment is byte-identical so the existing same-kb_id deterministic-order + concurrency-filter tests are preserved. Prompt (`_STYLE_GUIDE`, injected into both KB_ENCODER and SETUP_ENCODER): * CROSS-MODEL ACCESS: explicit ban on `SELECT…FROM…WHERE` subqueries and `EXISTS(SELECT…)` inside Column.sql; cites the peer-KB block's new `entity_ref=` + `reachable_from_host:` markers as the source. Worked WRONG/RIGHT example mined from the KB 15 correlated-subquery audit finding. * NO INVENTED JOINS: never add a join inside Column.sql; if a peer isn't reachable from the chosen host, CHANGE the host. R-JOIN is explicitly exempted (it's the recipe whose job is to register a schema-level ModelJoin via `edit_model(joins=[...])`). * HOST-CHOICE: KB-natural-granularity first, then min-joins through the declared graph. Worked example for KB 11 (Household Density) explaining why properties wins over households. * SAMPLED-VALUE CAVEATS: prepend a 1-2 sentence caveat ABOVE the [kb=N] block when actual sampled values diverge from KB-described enums. Three exemplars mined verbatim from the hand-audited reference (Tenure_Type mixed case; Income_Bracket R$-ranges vs ordinal labels; Dwelling_Class mixed case). * STRING-NORM: reinforced with a Yes/No-flag worked example. * The dropped INLINE-DON'T-NAME rule and the corresponding "inlines or references" wording in VALUE-ILLUSTRATION OWNERSHIP Case A are replaced with explicit "REFERENCES those leaves by entity_ref" + "Do NOT inline the leaves' CASE expressions" — keeping the DAG. * `entity_ref` distinction: documentation form `<db>.<model>.<leaf>` vs raw-SQL alias form `target_alias.col` / `path__alias.col` made explicit in CROSS-MODEL ACCESS, VALUE-ILLUSTRATION OWNERSHIP, and both R-RESOLVE recipe entries. Tests (18 new in `test_pydantic_ai_otf_encode_peer_kb_entityref.py`): collector returns correct entity_ref + exact reach maps for columns, measures, aggregations, and model entities; multi-hop chains; composite `join_pairs` count once; renderer emits labeled markers per-peer (not a global reach map); strict-less-than concurrency filter at collector level; same-kb_id multiplicity (Column + Measure sharing kb_id survive). Codex reviewed tests in 3 passes (collapsed-by-kb_id bug; exact-map assertions; per-peer chunking) and the impl in 2 passes (entity_ref qualifier confusion; R-JOIN exemption; VALUE-ILLUSTRATION wording). CLI test updates: `tests/cloud/test_cli.py` cases using fake `db_a_1` ids opt out of the new `--require-audited-gold` guard with `--no-require-audited-gold`. Added explicit tests for the new defaults (patience 500, use_audited_gold_sql=True, require_audited_gold=True). Full non-integration suite: 1181 passed, 92 skipped, 19 deselected (was 1158 before changes; +23 new tests). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…DE.md After commit 3a05b06 (S1/S2/S4/S5/S6 cross-ref hygiene + sampled-value caveats), the full 15-instance households eval (Opus encoder, Sonnet user-sim, patience 500, audited gold, on-the-fly setup, fresh encode) scored P1=9/15 — beats the hand-audited reference's 8/15 on the same set with the same models and config. Run id: 20260527t1922-pydanti-slayer-45372d. Updated the DEV-1478 smoke section's eyeball assertions to add the post-S1-S5 contract (sampled-value caveats on KB 1/2/9; peer-KB block entity_ref + reachable_from_host markers). Added a "Full eval baseline" subsection with the four runs side-by-side as the canonical reference for future encoder changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two defensive-guard hardenings surfaced by codex review against the DEV-1478 follow-up commits. * `agent.py::_collect_literal_problems` — only casefold + strip the membership-check comparison when the LHS was actually wrapped by LOWER/UPPER/TRIM at the SQL level. The STYLE_GUIDE *encourages* `LOWER(TRIM(<col>))` but it is an LLM-level convention, not a runtime guarantee; silently normalising the validator's comparison would let an unwrapped `col = 'Yes'` against sampled `['yes', 'no']` slip through even though SQLite's runtime predicate returns zero rows. `_peel_wrapper` now returns `(inner_node, was_wrapped)`; the literal walker only normalises both sides when `was_wrapped is True`. Original Codex finding M (whitespace-tolerance on `LOWER(TRIM(<col>)) = 'yes'` against sampled `' yes '`) still holds. * `cloud/_audited_gold_check.py::missing_audited_gold_ids` — the guard now also fails the row when `audit_status` is `edited` / `unrecoverable` but `audited_sol_sql` is missing / empty / not a list. Previously the helper passed any row whose status string was in the accepted set; `apply_audited_gold_overlay()` requires `isinstance(audited_sol_sql, list)` and `bool(audited_sol_sql)` to swap the gold for those statuses, so a corrupt sidecar row would silently fall back to the un-audited gold mid-cloud-run, defeating the submit-time `--require-audited-gold` guard. `_load_db_audit_index` now returns `(status, has_audited_sql)` per row; the guard rejects edited/unrecoverable rows lacking usable SQL. `clean` rows pass unconditionally because the overlay deliberately leaves `sol_sql` untouched for clean rows — the original IS the audited gold by design. Tests: * `tests/test_literal_existence_validation.py` — 4 new cases covering unwrapped case-mismatch rejection, unwrapped exact-case acceptance, wrapped LOWER+TRIM whitespace tolerance, wrapped LOWER+TRIM rejection-when-truly-missing. * `tests/cloud/test_audited_gold_check.py` — 11 cases covering clean rows without audited_sol_sql, edited/unrecoverable rows with / without / with-empty / with-non-list audited_sol_sql, missing sidecar, missing row, unknown id, and a mixed batch. Full non-integration suite: 1196 passed, 92 skipped, 19 deselected (was 1181 pre-changes; +15 from new test cases). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
6 tasks
This was referenced May 28, 2026
ZmeiGorynych
added a commit
that referenced
this pull request
Jun 4, 2026
- Use paths.*_root() helpers in migrate_data_dirs.py (CodeRabbit #1) - Guard src_mi.rmdir() after partial annotation dir merge (CodeRabbit #2) - Use paths.benchmark_data_root() in ray_app.py + ray_app_annotator.py (CodeRabbit #3, #4) - Add BIRD_AUDITED_GOLD_ROOT to isolation fixture in test_paths.py (CodeRabbit #5) - Assert BIRD_GATED_GOLD_ROOT in download_benchmark_data test (CodeRabbit nitpick) - Add Benchmark.select_full_run_count; pass through load_benchmark_tasks so livesqlbench-base-full / livesqlbench-large (one_shot=True) don't hit the hardcoded 180-assertion (Codex #2) - _build_original_sql_index: use _auto_discover_gold instead of hardcoded livesqlbench filename — works for all gold_required benchmarks (Codex #3) - _check_gold_present helper in driver.py: fail at submit time if gold_required but no *.jsonl found locally (Codex #1, right layer for this check) - Remove Path import from ray_app_annotator.py (no longer used) - Merge conflict resolved: FakeSubmitArgs + _fake_annotate_args in test_driver.py Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Summary
householdsaudit vs hand-audited reference): broken predicates, composite-of-composites skipped, mis-IDed duplicates, missing string normalisation, inconsistent column refs, redundant joins.agent.pythat catches<col> = '<literal>'/<col> IN (...)predicates whose literals never occur in the column's stored distinct values — the structural fix for the KB 21 / KB 42 broken-predicate bug. WalksLOWER/UPPER/TRIMwrappers; resolves single-hop<alias>.<col>via host modeljoins; prefersColumn.sampled_valueswith a comma-safe fallback onColumn.sampled.EXISTING_KB_TAGGED_ENTITIESblock rendered from storage and injected into both encoder prompts, giving PEER-KB DEDUP a concrete signal (canonical lower-id KB wins by topo sort).NOT encodedin the deps block.CLAUDE.md.The upstream SLayer-side
Column.sampled_valuesstructured field is tracked separately as DEV-1480 — this PR works either way (validator falls back to the existingColumn.sampledcomma-joined string, skipping conservatively when values themselves contain commas).Test plan
env -u SSH_AUTH_SOCK uv run --extra all --extra dev --extra pydantic-ai pytest— 1141 passed, 92 skipped, 19 deselected (47s).tests/test_literal_existence_validation.py: 33 tests covering EQ + IN extraction; wrapper-peel (LOWER / UPPER / TRIM / nested); single-hop joins-aware column resolution;sampled_valuespreferred +sampledtext fallback + conservative-skip on\d,\d{3}patterns;sampled_values=[](all-NULL column) emits a defer hint; combined SQL + literal feedback in one tool result; fail-OPEN on harness errors; out-of-scope numeric literals / unparseable SQL / unknown aliases all silent-skip.feedback_no_prompt_content_tests.md):{existing_kb_tagged_entities_block}placeholder exists in bothKB_ENCODER_PROMPTandSETUP_ENCODER_PROMPT; verbatim substitution; format-arg coverage / no-leftover-braces invariants._format_existing_kb_tagged_entities_block(columns / measures / aggregations / sort-by-kb_id / untagged exclusion / empty(none)); end-to-end seam tests for both setup and task-time runners proving the block is computed from storage and threaded into the formatted prompt.CLAUDE.mdrecipe — running in parallel with this PR's CI.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests