feat: shadow-mode SQL pre-validation telemetry#643
Conversation
- altimate-core NAPI binding: set `NODE_PATH` to global npm root so
`require('@altimateai/altimate-core')` resolves after `npm install -g`
- upstream branding: replace "opencode" with "altimate-code" in user-facing
`describe` strings (uninstall, tui, pr commands, config, server API docs)
- driver resolvability: set `NODE_PATH` in driver check loop and install
`duckdb` alongside the main package so at least one peer dep is present
- hardcoded CI paths: restrict grep to JS/JSON files only — compiled Bun
binaries embed build-machine paths in debug info which is unavoidable
- NAPI module exports: already had correct `NODE_PATH` in extended test;
root cause was the base test (fix 1) which is now resolved
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…rules, and error UX - Wire datafusion validation into sql_execute — catches column/table errors locally before hitting the warehouse (uses schema cache with 24h TTL) - Add sql_pre_validation telemetry event to measure catch rate and latency - Add apply_patch retry-with-re-read on verification failure — re-reads the file and retries once before giving up, with actionable error messages - Add file-not-found cache in read tool — prevents retry loops on missing paths (capped at 500 entries) - Add agent behavior rules to system prompt: act first/ask later, enforce read-before-edit, limit retries to 2 per input - Add actionable connection error guidance in warehouse_test — maps common auth failures (wrong password, missing key, SSO timeout) to fix instructions - Auto-pull schema cache in altimate_core_validate when no schema provided Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
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.
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe PR adds SQL pre-validation functionality that executes before SQL processing using cached schema information, and introduces corresponding telemetry events to track validation outcomes ( Changes
Sequence DiagramsequenceDiagram
participant SQLExec as SQL Execute Tool
participant WH as Warehouse Registry
participant Cache as Schema Cache
participant Validator as altimate_core.validate
participant Telemetry as Telemetry Tracker
SQLExec->>WH: Resolve target warehouse<br/>(with fallback)
WH-->>SQLExec: Warehouse instance
SQLExec->>Cache: Check cache availability<br/>and freshness (TTL)
Cache-->>SQLExec: Cached schema columns
SQLExec->>SQLExec: Build schema context<br/>(apply scan limit)
SQLExec->>Validator: Invoke validation<br/>with cached schema
Validator-->>SQLExec: Validation errors
alt Validation Errors Present
SQLExec->>SQLExec: Categorize as 'blocked'
else No Errors
SQLExec->>SQLExec: Categorize as 'passed'
end
SQLExec->>Telemetry: Emit pre-validation event<br/>(outcome + metadata)
Telemetry-->>SQLExec: Tracked
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
…idation-and-error-resilience # Conflicts: # test/sanity/phases/verify-install.sh
Reduces PR scope to telemetry-only based on deep analysis: the broader fixes (prompt rules, warehouse_test guidance, apply_patch retry, read file-not-found cache, altimate_core_validate auto-pull) were speculative against an 8-machine / 1-day telemetry sample. This PR now ships only what's needed to measure whether pre-execution SQL validation is worth it: - Keep: sql_pre_validation telemetry event + preValidateSql function - Change: pre-validation runs fire-and-forget (shadow mode) — emits telemetry with outcome=skipped|passed|blocked|error but never blocks sql_execute. Zero user-facing latency impact. - Revert: read.ts, apply_patch.ts, warehouse-test.ts, altimate-core-validate.ts, anthropic.txt system prompt changes — to be re-evaluated as separate PRs once real telemetry data validates need. After 2 weeks of shadow telemetry, we can decide whether the blocking behavior is worth the latency and false-positive risk.
There was a problem hiding this comment.
3 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/opencode/src/altimate/tools/sql-execute.ts">
<violation number="1" location="packages/opencode/src/altimate/tools/sql-execute.ts:120">
P2: When `warehouse` is omitted, this validates against the cache’s first warehouse instead of the warehouse `sql.execute` will actually use.</violation>
<violation number="2" location="packages/opencode/src/altimate/tools/sql-execute.ts:140">
P2: Limiting validation to 10k columns can create false structural errors on large warehouses and bias the telemetry sample.</violation>
<violation number="3" location="packages/opencode/src/altimate/tools/sql-execute.ts:229">
P1: Mask the validator error before emitting telemetry; this currently uploads raw schema identifiers into App Insights.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
- P1: mask validator error via `Telemetry.maskString()` before emitting `sql_pre_validation` telemetry. Raw schema identifiers (table/column names, paths) no longer leak to App Insights. - P2: resolve fallback warehouse via `Registry.list().warehouses[0]` (same path `sql.execute` uses) instead of the cache's first warehouse. Keeps shadow validation aligned with actual execution. - P2: raise column-scan cap from 10k to 500k and add `schema_truncated` boolean to the event. Avoids false structural errors on large warehouses and lets analysis flag biased samples.
What does this PR do?
Ships telemetry-only instrumentation to measure whether pre-execution SQL validation is worth implementing as a blocking check.
The original scope (blocking validation, apply_patch retry, file-not-found cache, prompt rules, warehouse guidance, validate auto-pull) was deferred after analysis showed the telemetry sample (8 machines, 1 day, 2-3 outlier users) didn't justify 297 LOC of production defenses.
This PR adds:
sql_pre_validationtelemetry event (outcome, reason, schema_columns, duration_ms, error_message)preValidateSql()runs fire-and-forget before everysql_execute— validates the query against the cached schema viaaltimate_core.validate, emits telemetry, and never blocks executionAfter 2 weeks of shadow telemetry we can answer:
sql_executecalls would have been blocked?Then decide whether to flip to blocking mode.
Type of change
Issue for this PR
Follow-up to telemetry analysis of 2026-03-30 (Azure AppInsights, altimate-code-os).
How did you verify your code works?
.catch(() => {})— cannot fail the sql_execute calltrackPreValidation()emits telemetry with one of 4 outcomes (skipped, passed, blocked, error)Checklist
Summary by CodeRabbit