OTF setup-encoder reverse-dep visibility — fix KB-6 mis-encoded as dwelling-type score (DEV-1466) - #1
Merged
ZmeiGorynych merged 1 commit intoMay 24, 2026
Conversation
…encoded as dwelling-type score (DEV-1466)
The setup encoder turned KB-6 (a value_illustration) into the canonical
dwelling_type_score column tagged meta.kb_id=6, while the dedicated
calculation_knowledge KB-44 deferred — mis-tagging the score for HARD-8
masking (which keys on meta.kb_id). It also encoded KB-22 with a degenerate
always-true water clause.
Fix (build-time only; the task-time encoder still has ask_user):
- reference_build._encode_all computes each KB's reverse deps (parents),
filtered to scheduled/acyclic KBs, and threads them through run_one →
_run_setup_encoder → _format_reverse_deps_block → {reverse_deps_block}.
- DUPLICATE-SCORE GUARD: a value_illustration defaults to R-DESCRIBE; it must
not materialise its scoring prose as a calc column when a referencing
calculation_knowledge parent owns that score (KB-6 → KB-44). Exception: a
parent that merely averages component scores keeps them (KB-3/4/5 → KB-13).
- LITERAL-EXISTS GUARD: confirm a categorical literal occurs in the data
before encoding a predicate on it; else defer, listing the column's values
(<20 distinct) so a later agent can map the label (fixes KB-22).
Finding (b) (named derived-col refs don't resolve) was stale: SLayer 0.6.9's
engine/column_expansion.py recursively expands same-/cross-model derived refs
at query time (verified empirically against the real sqlite).
Tests: pure-fn + _encode_all wiring (incl. cyclic-parent exclusion) + real
run_one closure + prompt mechanical contract. Full non-integration suite:
956 passed. Acceptance gate (opus re-encode of households): score tagged
kb_id 44, KB-6 R-DESCRIBE, KB-22 deferred with candidate values, KB-3/4/5
keep component scores, living_condition_score resolves.
Also tidied stale DEV-1410 baseline comments and documented the exact test
command in CLAUDE.md.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
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 (12)
📝 WalkthroughWalkthroughThis PR implements reverse-dependency tracking in the knowledge-base encoder. The setup encoder now receives data about which KBs reference the current KB (its parents), enabling more accurate ownership decisions for scoring and derived values. Changes span formatters, prompt enhancements, encoder wiring, scheduler computation, and test coverage across the encoding pipeline. ChangesReverse Dependencies Encoder Integration
🎯 3 (Moderate) | ⏱️ ~25 minutes
🚥 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 |
This was referenced May 27, 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>
ZmeiGorynych
added a commit
that referenced
this pull request
Jun 12, 2026
…ath order Codex round 3 caught three plumbing gaps that survived round 2's _compute_n1 rewrite. conditions threading (Codex round 3 #1 + #3) Round 2 added a `conditions` kwarg to `grade_submission`, but the production grading paths still dropped task-level conditions before reaching it. - `grade_in_place.grade_and_write` now accepts `conditions: Optional[dict] = None` and forwards to `grade_submission`. - `grade_in_place.grade_one_submission` reads `task_data.get("conditions")` and passes it to `grade_and_write`. - `scripts/regrade_n1_ex_base.py:_process_one` already loaded conditions from the annotation / canonical jsonl; now actually passes them to `grade_submission` so the backfill regrades ordered tasks with positional semantics. Without this, conditions={"order": True} tasks would silently fall back to set-dedup N1 from the cloud inline grader and the local regrade — even though `_compute_n1` now expects them. sys.path front-insert on every load (Codex round 3 #2) The round-1 cache-isolation fix snapshotted `sys.modules["db_utils"]` but the `sys.path.insert(0, sys_path_addition)` was conditional on absence. After loading mini-interact then livesqlbench, sys.path was `[lsb_dir, mini_dir, ...]`. A subsequent reload of mini-interact's test_utils ran a fresh `from db_utils import ...` that walked sys.path in order and bound to livesqlbench's `db_utils` (wrong driver). `_load_module_from_file` now removes any pre-existing occurrence of `sys_path_addition` and re-inserts at position 0, so the tree currently being loaded is always at the front. Tests 4 new regressions: - test_grade_and_write_forwards_conditions_to_grade_submission - test_grade_one_submission_forwards_task_data_conditions - test_regrade_script_forwards_task_conditions_to_grade_submission - test_load_module_from_file_reloading_first_tree_still_finds_own_db_utils Full non-integration suite: 2858 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ZmeiGorynych
added a commit
that referenced
this pull request
Jun 12, 2026
…etection, test isolation Codex round 4 + CodeRabbit caught three real concerns. Postgres N1 production path (Codex round 4 #1) `_compute_n1`'s file-existence guard fired for the cloud-Postgres shape (`db_path = Path(<db_name>)`, `conn = None`) and fell back to legacy `_set_equal` for every livesqlbench non-sqlite task — defeating the new ex_base dispatch in the very integration the PR advertised. The guard now only fires when the benchmark backend is SQLite; for Postgres the dispatcher trusts upstream `perform_query_on_postgresql_databases` to auto-open from the connection pool. Commented-mutation detection (Codex round 4 #2) `is_mutation_sql` only allowed whitespace before a statement-leading mutation keyword, so `-- explanation\nINSERT INTO ...` and `/* ... */ UPDATE ...` slipped past the dispatcher's mutation guard. `compare_pred_vs_gold_ex_base` then ran upstream `remove_comments` and executed the cleaned mutation against the per-task DB. `is_mutation_sql` now strips SQL comments (`-- ...`, `/* ... */`) before the regex match, mirroring upstream's pre-exec cleanup so the dispatcher and the exec path agree on what counts as statement start. Test isolation (CodeRabbit round 3) `test_regrade_script_forwards_task_conditions_to_grade_submission` mutated `sys.path` without cleanup and imported the script without reload-if-cached, leaking state to subsequent tests. The test now snapshots + restores `sys.path` and `sys.modules` and uses `importlib.reload` when the module is already cached. Same test splits the combined `and` assertion for clearer failure messages (CodeRabbit nitpick 3). Test comment accuracy (CodeRabbit nitpick 2) `test_n1_dispatch_uses_db_stem_for_postgres_benchmarks` claimed the db_path file was "presence required by the conn-fallback guard" — but `conn=MagicMock()` is truthy so the conn-is-None branch is skipped. Comment now reflects that file creation is defensive only. Tests: 5 new (4 parametrised commented-mutation cases, 1 Postgres file-existence-skip dispatch). Full non-integration suite: 2863 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ZmeiGorynych
added a commit
that referenced
this pull request
Jun 12, 2026
…o all callers Codex round 5 found two leaks the prior rounds left. Conn lifecycle in upstream shim (Codex round 5 #1) When the dispatcher passed `conn=None` (the cloud Postgres production shape), upstream `execute_queries` opened a fresh conn per call (sqlite3.connect for SQLite, pool.getconn for Postgres) and NEVER closed it. Every N1 comparison leaked at least one conn, and `_both_results_empty`'s re-execution could double-leak. Postgres eventually exhausts the pool; SQLite eventually exhausts file descriptors. `compare_pred_vs_gold_ex_base` now opens its own conn when the caller passes `conn=None` (sqlite3.connect for SQLite, _open_psycopg2_connection for Postgres) and closes it in a nested try/finally that fires on both the success path AND when upstream raises. Caller-supplied conns are left alone — that's the caller's lifecycle. `ExBaseUnavailableError` surfaces when conn-open fails so the dispatcher falls back to legacy `_set_equal` cleanly. Plumb conditions to remaining grade_submission callers (Codex round 5 #2) Round 2 added conditions to grade_submission; round 3 plumbed grade_in_place but left four direct callers still hardcoding `conditions=None` implicitly. Ordered-comparison tasks reaching N1 via those paths silently fell back to set-dedup. Updated: - src/bird_interact_agents/agents/claude_sdk_otf/agent.py (autopsy grading) - src/bird_interact_agents/agents/claude_sdk_otf_ainteract/agent.py (autopsy grading) - src/bird_interact_agents/eval/annotate.py (annotator-side grader closure) - src/bird_interact_agents/eval/regrade.py (regrade-run grader closure) Each call site now passes `conditions=task_data.get("conditions")` (or `task_row.get("conditions")` / `_row.get("conditions")`, matching the in-scope task-source dict). Tests 4 new regressions in tests/eval/: - test_compare_pred_vs_gold_ex_base_closes_owned_sqlite_conn_on_success - test_compare_pred_vs_gold_ex_base_closes_owned_sqlite_conn_on_exception - test_compare_pred_vs_gold_ex_base_does_not_close_caller_supplied_conn - test_remaining_grade_submission_callers_read_conditions_from_task_dict (grep-style across the four sites so a future caller is more likely to follow the same idiom) Full non-integration suite: 2867 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ZmeiGorynych
added a commit
that referenced
this pull request
Jun 12, 2026
…e failures Codex round 6 caught two more leak paths through the dispatch guards. CTE-prefixed mutations (Codex round 6 #1) `is_mutation_sql` matched mutation verbs only as the leading token of a statement (after `^` or `;`). SQLite + Postgres both accept CTE-prefixed mutations like WITH x AS (SELECT id FROM t WHERE v > 0) DELETE FROM t WHERE id IN (SELECT id FROM x) where the mutation verb follows a `WITH ... AS (...)` block. The statement-start regex sees `WITH` and reports "not a mutation", so the dispatcher routed the SQL to upstream's writeable exec path and the DELETE committed against the per-task DB. `is_mutation_sql` now also runs a verb-target regex (`INSERT INTO`, `UPDATE <ident> SET`, `DELETE FROM`, `CREATE TABLE/VIEW/INDEX/...`, `DROP TABLE/...`, `ALTER TABLE/...`, `TRUNCATE [TABLE] <ident>`, `REPLACE INTO`) anywhere in the cleaned SQL. The shapes are tight enough that ordinary SELECT clauses don't trip them (`SELECT INSERT_NUM` doesn't match `INSERT INTO`). tiktoken encode failures (Codex round 6 #2) `_truncate_for_embedding` caught `import tiktoken` and `encoding_for_model` failures, but `enc.encode(text)` itself raises `ValueError` when input contains disallowed special-token strings like `<|endoftext|>`. Memory text comes from arbitrary KB content, so a single offending memory could abort the whole cache build. Both `enc.encode` and `enc.decode` are now wrapped in try/except that falls back to the same logged char-cap path used when tiktoken is missing. Tests 7 new regressions: - test_is_mutation_sql_detects_cte_prefixed_mutations[*] (4 cases) - test_is_mutation_sql_negative_for_cte_select[*] (2 cases — a SELECT that happens to have a WITH clause must NOT be flagged) - test_truncate_for_embedding_special_token_text_falls_back_to_char_cap Full non-integration suite: 2874 passed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
4 tasks
ZmeiGorynych
added a commit
that referenced
this pull request
Jun 13, 2026
…bmit, query-tool hygiene Diagnosed by walking the alien_1 trajectories on both Opus/Sonnet (r5) and Kimi all-around (r4): both ran out of turns flailing on the same ambiguities the discovery subagent left in "Open ambiguities" instead of resolving, and both burned turns on partition-deny denials when reaching for `mcp__slayer__query` (discovery-only) when their actual query tool is the bird-interact-tools wrapper. * Discovery prompt: "Open ambiguities" reframed as a failure mode (must be empty in the common case; only the user-sim refusals belong there). New explicit resolution checklist a-interact discovery MUST cover: GROUPING / METRIC IDENTITY / AGGREGATION SCOPE / SORT ORDER / TIE-BREAKING / NULL HANDLING / UNITS-ROUNDING / FILTER LITERALS. * Main-agent workflow note: new verify-before-submit checklist with SORT ORDER as item #1 ("when the request is silent, ask once; never assume"). * Query-tool hygiene: every mention of `query_nested` removed from the prompts; `query` is documented as accepting a single SlayerQuery OR a list of stage objects for nested DAGs. New comment names the wrapper (`mcp__bird-interact-tools__query`) explicitly and warns that `mcp__slayer__query` is reserved for the discovery subagent — kills the instinctive misnaming that cost Opus 4 turns and Kimi more. Prompt snapshot hashes re-baselined. Full suite green (2829 passed). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
This was referenced Jul 7, 2026
ZmeiGorynych
added a commit
that referenced
this pull request
Jul 15, 2026
…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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
DEV-1466. The OTF setup-encoder turned KB-6 (a
value_illustration) into the canonicaldwelling_type_scorecolumn taggedmeta.kb_id: 6, while the dedicatedcalculation_knowledgeKB-44 ("Dwelling Type Score") deferred. Since HARD-8 masks entities bymeta.kb_id(hard8_preprocessor.py), this mis-tags the score: a task deleting KB-44 won't mask it, and a task deleting KB-6 deletes a column KB-20 depends on. Separately, KB-22 was encoded with a degenerate always-true water clause (water_access_score = 4, true for every row).Validation of the original findings
dwelling_type_score. The real defect — wrong owner/tag — holds.)engine/column_expansion.py("Closes DEV-1333") which recursively expands same-/cross-model derived refs at query time. Verified empirically: loadingslayer_models_otf/householdsagainst the real sqlite,living_condition_score/is_comfortable_householdfully expand and execute. No code change for (b).The fix (build-time encoder only; the task-time encoder still has
ask_user)reference_build._encode_allcomputes each KB's reverse deps (parents), filtered to scheduled/acyclic KBs, and threads them throughrun_one→_run_setup_encoder→ new pure helper_format_reverse_deps_block→ a new{reverse_deps_block}slot inSETUP_ENCODER_PROMPT.value_illustrationdefaults to R-DESCRIBE; it must not materialise its scoring prose as a calc column when a referencingcalculation_knowledgeparent defines that same score as its whole calculation (KB-6 → defer to KB-44). Exception: a parent that merely averages component scores keeps them (KB-3/4/5 → KB-13).sampled) before encoding a predicate on it; else defer — listing the column's values (<20 distinct) so a later agent can map the label (fixes KB-22).Tests
_format_reverse_deps_block(empty/sort/dedup/missing-field/truncate),_encode_allreverse-dep wiring (incl. cyclic-parent exclusion + transitive cycle), realrun_oneclosure threading, andSETUP_ENCODER_PROMPTmechanical contract (placeholder declared + content appears; no prose assertions per repo policy).households(8/8):dwelling_type_scoretaggedkb_id 44, KB-6 R-DESCRIBE, KB-44 owns the score (Brickwork 4 / Apartment 3 / else 1), KB-22 deferred with candidatewateraccessvalues, KB-3/4/5 keep their component scores,living_condition_scoreresolves + executes.Also
slayer_models/householdsbaseline (now fixed in 0.6.9).CLAUDE.md(incl. the--extra pydantic-ai/httpx[socks]proxy gotcha).🤖 Generated with Claude Code
Summary by CodeRabbit
Documentation
Improvements
Tests