-
Notifications
You must be signed in to change notification settings - Fork 41
sql_pre_validation telemetry: correctness bugs from code review of #643 #650
Description
Follow-up to #643 (merged). Multi-model code review surfaced bugs that corrupt the shadow-mode telemetry this feature collects.
Critical (would corrupt the 2-week analysis data)
C1 — Validator failures recorded as 'passed/valid'
packages/opencode/src/altimate/tools/sql-execute.ts ignores validationResult.success. When Dispatcher.call("altimate_core.validate") returns {success: false, data: {}, error: "..."} (the fail() shape in native/altimate-core.ts), the code computes isValid = (undefined !== false) && (0 === 0) = true. Engine crashes are silently recorded as outcome="passed", reason="valid", inflating the passed bucket.
C2 — Schema key collision from dropped database name
Keys are built as schema_name.table or table, with no database/catalog component. In multi-database warehouses (Snowflake, BigQuery), DB1.PUBLIC.USERS and DB2.PUBLIC.USERS collapse into the same entry, and qualified queries like DEV_DB.PUBLIC.USERS trigger false-positive outcome="blocked" events.
Major
M1 — Event-loop blocking on hot path
Bun's bun:sqlite is synchronous. After await getCache(), cache.cacheStatus() and cache.listColumns(500_000) block the event loop for the entire sync duration before the next await. Delays concurrent work in the process for large warehouses.
M2 — Missing correlation fields on sql_pre_validation event
Event has session_id (session-level joins) but lacks warehouse_type, query_type, and any join key to sql_execute_failure. Cross-event analysis per-warehouse / per-query can't be done.
Minor
m3 — Fragile substring matching (msg.includes("column")) misses variants like unknown identifier, unresolved column, and false-matches on table names containing column.
m4 — errorOutput construction runs in shadow mode but the caller discards it via .catch(() => {}). Dead code.
m5 — ALL_EVENT_TYPES in test/telemetry/telemetry.test.ts not updated with the new event — completeness check stale.
Reviewers
Multi-model consensus review (Claude + GPT 5.4 + Gemini 3.1 Pro) — all 3 flagged C2 independently; C1 flagged by Gemini.
Without C1+C2 fixed, the 2-week shadow telemetry window will yield corrupted data that can't justify enabling blocking mode.