fix: sql_pre_validation telemetry review followups

[anandgupta42]

What does this PR do?

Follow-up to #643. Fixes review findings from multi-model consensus code review.

Critical bugs (would corrupt 2-week shadow telemetry data):

• C1 — validationResult.success was ignored. Dispatcher failures ({success: false, data: {}}) evaluated as isValid=true, recording engine crashes as outcome="passed", reason="valid". Now checks .success first and emits outcome="error", reason="dispatcher_failed".
• C2 — database name was dropped from schema context keys. Multi-database warehouses collapsed DB1.PUBLIC.USERS and DB2.PUBLIC.USERS into one entry, and qualified queries got false-positive blocked events. Keys now use database.schema.table; columns deduped per table to defend against residual collisions.

Major fixes:

• M1 (event-loop blocking): preValidateSql now yields via setImmediate before the synchronous bun:sqlite scan, so concurrent work isn't blocked while listColumns runs on large warehouses.
• M2 (correlation fields): sql_pre_validation event now carries warehouse_type, query_type, and masked_sql_hash (SHA-256 prefix of masked SQL). Shadow outcomes can be joined to sql_execute_failure events per-warehouse / per-query-type.
• m5 (stale test list): sql_pre_validation added to ALL_EVENT_TYPES completeness check in telemetry.test.ts; count bumped 42 → 43.

Minor fixes:

• m3 (fragile matching): widened structural-error regex with word boundaries + relation|identifier variants.
• m4 (dead code): removed errorOutput construction — caller discards it in shadow mode. Comment documents where to rebuild when blocking mode is enabled later.

Type of change

• Bug fix
• New feature
• Test coverage
• Documentation
• Refactoring
• Infrastructure

Issue for this PR

Closes #650

How did you verify your code works?

• Typecheck passes (full turbo)
• 130 telemetry tests pass (including updated ALL_EVENT_TYPES assertion)
• Marker guard clean for changed files

Checklist

• Typecheck passes
• Marker guard passes
• No user-facing behavior change (shadow mode)
• Telemetry event type updated with new fields
• Event-type completeness test updated

---

Summary by cubic

Fixes critical bugs in SQL pre-validation telemetry and adds richer context so shadow data is accurate and easy to analyze. Also reduces event-loop blocking during schema scans.

• Bug Fixes

  • Treat dispatcher failures as error with reason "dispatcher_failed" instead of "passed/valid".
  • Build schema context with fully qualified database.schema.table keys and dedupe columns to avoid multi-database collisions.
  • Broaden structural-error matching to include view/relation/identifier variants with word boundaries.

• New Features

  • Add warehouse_type, query_type, and masked_sql_hash to sql_pre_validation events for correlation with sql_execute_failure.
  • Yield via setImmediate before the sync bun:sqlite scan to prevent event-loop blocking on large caches.
  • Add sql_pre_validation to the event-type completeness test (count 42 → 43).

^{Written for commit 4283169. Summary will update on new commits.}

Summary by CodeRabbit

• Bug Fixes

  • Improved error detection for SQL queries with stricter pattern matching.
  • Enhanced handling of validation failures to allow execution to continue safely.

• Chores

  • Enhanced telemetry instrumentation for SQL validation events to better track warehouse type and query classification.
  • Improved correlation capabilities for debugging validation failures.

[claude]

Claude Code Review

This repository is configured for manual code reviews. Comment @claude review to trigger a review and subscribe this PR to future pushes, or @claude review once for a one-time review.

_{Tip: disable this comment in your organization's Code Review settings.}

[coderabbitai]

📝 Walkthrough

Walkthrough

This PR addresses critical correctness bugs in the sql_pre_validation telemetry feature. Changes include fixing validator failure detection logic, resolving schema key collisions by using fully-qualified identifiers, adding event-loop yielding to prevent blocking, introducing telemetry correlation fields (warehouse_type, query_type, masked_sql_hash), improving error detection with regex pattern matching, handling dispatcher-level validation failures, and updating telemetry schema and tests accordingly.

Changes

Cohort / File(s)	Summary
Telemetry Schema packages/opencode/src/altimate/telemetry/index.ts	Added new required fields to sql_pre_validation event: warehouse_type, query_type, masked_sql_hash. Updated reason documentation to include dispatcher_failed value for validation failures.
SQL Execution & Validation Logic packages/opencode/src/altimate/tools/sql-execute.ts	Fixed critical bugs: (1) corrected validator failure detection by checking validationResult.success, (2) resolved schema key collisions by using fully-qualified database.schema.table format, (3) added setImmediate() yield to prevent event-loop blocking, (4) introduced TrackCtx object threading warehouse/query/hash correlation fields through telemetry, (5) switched error detection from substring matching to word-boundary regex, (6) added explicit dispatcher validation failure handling with dispatcher_failed reason, (7) modified shadow mode blocking behavior to return { blocked: false } instead of enforcing block.
Test Coverage packages/opencode/test/telemetry/telemetry.test.ts	Added sql_pre_validation to ALL_EVENT_TYPES list and updated event completeness assertion from 42 to 43.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

• feat: add core_failure telemetry with PII-safe input signatures #245 — Both PRs modify telemetry schema and SQL execution pipeline to add/thread warehouse_type, query_type, and masked_sql_hash correlation fields for event correlation.
• feat: add telemetry intelligence signals for debugging and improvements #564 — Both PRs enhance SQL telemetry system by updating event types, adding fingerprint/hash fields, and threading context through sql-execute tooling.

Suggested labels

contributor

Suggested reviewers

• mdesmet

Poem

🐰 A rabbit hops through telemetry fields,
Fixing schemas where collision yields—
No more blocking loops, the event flows free,
With warehouse_type and query_type key,
Dispatcher's voice heard, correlation complete! 🎉

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly summarizes the main change: a follow-up fix addressing telemetry review findings from the previous pr (#643).
Linked Issues check	✅ Passed	Code changes fully address all objectives from issue #650: C1 checks validationResult.success, C2 uses database.schema.table keys, M1 adds setImmediate, M2 adds correlation fields, and m3-m5 make minor improvements.
Out of Scope Changes check	✅ Passed	All changes are scoped to sql_pre_validation telemetry improvements across three files. No unrelated modifications detected; changes align precisely with stated objectives.
Description check	✅ Passed	PR description is comprehensive and well-structured, including detailed sections on critical bugs, major fixes, minor fixes, verification steps, and checklist items, though it deviates from the template format.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/sql-pre-validation-review-followups

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cubic-dev-ai]

No issues found across 3 files