Skip to content

spec.md

Latest commit

 

History

History
143 lines (101 loc) · 16.7 KB

spec.md

File metadata and controls

143 lines (101 loc) · 16.7 KB
Raw

Spec: NL2SQL (LLM → SQL → Execute Retry Loop)

Type: Forward (behavior contract for FIX-004 integration) Status: Draft Last synced with code: 2026-04-13 Hexagonal scope: Application (pipeline subgraph) + Presentation Parent module: ../spec.md Related plan: ./plan.md

1. Context & Purpose

Natural-language-to-SQL flow that lets a user ask "give me the 10 jurisdictions with the highest spending in 2025" and receive actual rows, generated by the LLM, validated by the sandbox, and handed to the analyst as a standard DataResult.

Historically (until 2026-04-10) the entire NL2SQL loop lived inlined inside the sandbox connector (application/pipeline/connectors/sandbox.py, ~170 lines of imperative code) even though a proper LangGraph subgraph sketch already existed at application/pipeline/subgraphs/nl2sql.py. Two implementations of the same thing — one live, one dead. FIX-004 consolidates them into a single stateful subgraph that owns the full retry + fallback behavior, and lets the sandbox connector shrink to a thin table-discovery + state-construction wrapper.

Scope of this spec: the subgraph's WHAT and WHY. The exact file paths, node builder calls, and state field types belong in plan.md. Keep-it-simple (constitution §0) governs every decision here: no speculative generalization, no tool-calling, no multi-model dispatch, no parallel branches. The subgraph stays a stateful loop with a handful of fallback hops.

2. Ubiquitous Language

Term Definition
NL2SQL The act of converting a natural-language question to an executable SQL query against the cache_* schema.
Self-correction Retrying SQL generation with the previous error as context, up to max_attempts times.
Last-resort fallback A deterministic SELECT * FROM first_table LIMIT 10 query tried once after all self-corrections fail, so the analyst gets something instead of a hard error.
INDEC fallback Escape hatch that hits the live INDEC API when the sandbox returned zero rows and the user asked about INDEC-shaped data (inflation, IPC, EMAE, etc.).
Save-success Background persistence of a successful (question, SQL) pair into the few-shot cache, used to prime future prompts.
Table discovery The step that happens before this subgraph runs: listing cached tables, resolving planner hints, filtering by year, enriching with catalog metadata. Owned by the sandbox connector, not by this subgraph.
used_fallback flag State marker that tells downstream nodes "we landed here via last-resort, not via the LLM's own query" — used to redact the SQL from save-success and mark the metadata.

3. User Stories

US-001 (P1) — NL2SQL answers the question

As a chat user, I want to ask "give me the 10 jurisdictions with the highest spending in 2025" and receive rows with numbers, so that the analyst can build a table or chart without my having to write SQL.

  • Happy path: question → SQL generated → executes clean → rows returned.
  • Edge: the first SQL is broken → self-corrects once or twice → succeeds.
  • Edge: all self-corrections fail → last-resort fallback returns 10 rows from the first discovered table → analyst sees degraded data with a used_fallback: true marker.

US-002 (P1) — Degradation is visible, not silent

As the analyst (downstream consumer) and the user, when NL2SQL couldn't produce the query I asked for, I want the DataResult to explicitly say so instead of silently returning 10 arbitrary rows as if they were my answer.

US-003 (P2) — Empty INDEC queries fall back to the live API

As a chat user asking about inflation / IPC / EMAE / unemployment, when the sandbox has no cached rows for my specific timeframe, I want the system to automatically hit the live INDEC API instead of returning an empty result.

US-004 (P2) — Successful queries improve future answers

As the operator, I want every successful (question, SQL) pair to be persisted as a future few-shot example, so that the prompt quality improves over time without manual curation.

US-005 (P3) — SQL is never leaked to production users

As a security-conscious operator, I want the generated_sql field of the response metadata to be omitted in production (APP_ENV=prod) to prevent schema enumeration through error replay, while keeping it in local/dev for debugging.

4. Functional Requirements

Core loop

  • FR-001: The subgraph MUST generate SQL by calling the LLM exactly once with the nl2sql prompt template and the user's natural-language question as user message.
  • FR-002: The subgraph MUST execute every generated SQL against ISQLSandbox.execute_readonly. It MUST NOT execute SQL any other way.
  • FR-003: On SQL execution error, the subgraph MUST invoke the sql_fixer prompt with the failed SQL + error message + top ~2000 chars of the tables context, and re-execute the fixed SQL.
  • FR-004: Self-correction MUST be bounded by max_attempts (default 2). After max_attempts failed fixes, the subgraph MUST stop retrying and proceed to the last-resort fallback.
  • FR-005: The subgraph MUST NOT loop forever. Any exit — success, give-up after retries, last-resort failure — MUST reach format_result exactly once before END.

Fallback behavior

  • FR-006: When self-correction is exhausted, the subgraph MUST attempt one last-resort query of the form SELECT * FROM <first_discovered_table> LIMIT 10, built via safe_table_query() to reject unsafe table names.
  • FR-007: If the last-resort fallback also fails (no tables, unsafe name, execution error), the subgraph MUST emit a single error DataResult with metadata.error set to the last observed error string and records=[].
  • FR-008: When the sandbox returned zero rows AND the user query matches INDEC_PATTERN, the subgraph MUST attempt the live INDEC API via indec_live_fallback(). If it returns data, that result replaces the otherwise-empty NL2SQL result.
  • FR-009: The INDEC fallback MUST NOT run if the sandbox returned a non-empty result, regardless of the query pattern. Real data beats fallbacks.

State contract (input)

  • FR-010: The caller (sandbox connector) MUST provide the following checkpoint-safe fields in the initial state: nl_query, tables (already filtered list), tables_context (rendered string), catalog_entries (dict keyed by table name), table_descriptions (pre-computed list), few_shot_block (string, may be empty), max_attempts (int, default 2). The caller MUST NOT pre-compute indec_pattern_match — see FR-011.
  • FR-010a: Runtime service objects (llm, sandbox, embedding, semantic_cache) MUST travel outside the LangGraph state via request-scoped runtime context, not as persisted state fields. This keeps checkpoint serialization free of non-msgpack-serializable adapters such as FallbackLLMAdapter. The production caller wraps compiled_subgraph.ainvoke(...) in nl2sql_runtime(...); regression-tested by test_compiled_subgraph_runs_with_runtime_context_and_minimal_state.
  • FR-011: The subgraph MUST NOT perform table discovery, hint resolution, catalog enrichment, or table-description building itself. Those steps belong to the caller. This boundary exists to keep the subgraph testable in isolation with mock dependencies. However, the subgraph MUST compute INDEC_PATTERN.search(nl_query) internally (not as a caller-provided state field) — forcing the caller to know about INDEC would leak a connector-internal concept into the pipeline dispatch layer for zero benefit.

Output contract

  • FR-012: The subgraph MUST populate state.data_results with exactly one DataResult before reaching END, whether the outcome is success, give-up, last-resort, INDEC fallback, or total failure.
  • FR-013: The DataResult on success MUST include: source="sandbox:nl2sql", records (max 200 rows), metadata.total_records, metadata.columns, metadata.truncated, metadata.fetched_at, metadata.table_descriptions, metadata.served_table, metadata.result_kind.
  • FR-013a: metadata.served_table MUST be the table the SQL actually hit (e.g. mart.delitos_caba, mart.flujo_vehicular_peajes_caba). Primary source is the first entry of state.tables; if that drifts (the step_executor redirect path can lose it), the field is filled from the FROM/JOIN clause of state.generated_sql via _first_relation_reference. The field is consumed by (a) the finalize node's query_analytics write, and (b) the analyst's no-data fallback to deflect honestly when a real table was queried but returned empty (see FR-013c).
  • FR-013b: metadata.result_kind MUST be "aggregate" when the generated SQL contains a top-level aggregate function (COUNT/SUM/AVG/MIN/MAX) or GROUP BY, and "sample" otherwise. Computed deterministically by _classify_result_kind. The analyst reads it to avoid reporting a LIMIT n row dump as if n were a real total (LLM-001).
  • FR-013c: metadata.queried_empty_mart MUST be True when row_count == 0 AND served_table is non-empty AND generated_sql is non-empty. The analyst's no_data_fallback reads this and passes attempted_table to analyst_no_data.txt, so the deflection can name the specific dataset queried instead of falling back to the generic "OpenArg cubre…" template.
  • FR-013d: metadata.nonadditive_warning MUST be True when the generated SQL applies SUM(/AVG( to a column whose name contains tasa / indice / índice / porcentaje / promedio (heuristic detector _has_nonadditive_aggregate). The analyst reads it and hedges or recomputes (LLM-002: prevents reporting "112,93 homicidios por mil" from SUM(tasa_hechos) over departmental rows).
  • FR-014: When the result was produced via last-resort fallback, the metadata MUST include used_fallback=true so the analyst can downgrade its tone.
  • FR-015: When APP_ENV != "prod" the metadata MUST include generated_sql. When APP_ENV == "prod" the field MUST be omitted (SEC-03: prevent schema enumeration).

Save-success side effect

  • FR-016: On a successful (non-fallback) query with row_count > 0, the subgraph MUST persist the (question, SQL, table, row_count) tuple via save_successful_query() as a future few-shot example. This write is fire-and-forget — it MUST NOT block the subgraph's return.
  • FR-017: The background task MUST be held by a strong reference via the shared application/pipeline/_background_tasks.spawn_background() helper to prevent CPython's weak-ref garbage collection of unfinished tasks. That helper also attaches a done_callback that logs any unhandled exception from the task body (same helper used by finalize.py for its own memory-update path, DEBT-012 fix).
  • FR-018: Fallback results (used_fallback=true) MUST NOT be saved as few-shot examples. Last-resort SELECT * queries pollute the cache and are not worth re-suggesting.

On missing dependencies in tests: the spec intentionally does NOT define a no-op guard for the case where embedding or semantic_cache are absent from state. If a caller (or a test fixture) invokes the subgraph without those fields and the happy-path reaches save_success_node, the save_successful_query() coroutine will raise on first attribute access. That exception is automatically caught and logged by the spawn_background done_callback (FR-017) — no crash, visible warning. This is the "dumbest version that works" (constitution §0): it correctly surfaces production misconfiguration as a log warning, while letting unit tests either provide mocks or tolerate the log noise. Explicit if embedding is None: return branches would just be a duplicate of the resilience we already own at the task-registry layer.

Compilation & caching

  • FR-019: The subgraph MUST be compiled at most once per process. A get_compiled_nl2sql_subgraph() helper MUST cache the compiled instance and guard concurrent initialization with an asyncio.Lock (double-check pattern).
  • FR-020: The compiled subgraph MUST be stateless with respect to per-request data. All per-request data flows through the state dict passed to ainvoke().
  • FR-020a: The compiled subgraph MAY read request-scoped runtime dependencies from a ContextVar-backed helper, provided those dependencies are set/reset by the caller around ainvoke() and never leak into checkpointed state snapshots.

5. Success Criteria

  • SC-001: A question that maps cleanly to one table finishes in <8 seconds p95 with 0–1 self-correction retries.
  • SC-002: Self-correction resolves ≥70% of initial SQL failures on real user traffic (p50 of the retry rate).
  • SC-003: Zero orphaned background tasks — every save_successful_query spawn is registered in the module-level set, its done_callback runs, and any raised exception is logged.
  • SC-004: Zero generated_sql leaks in production — a request with APP_ENV=prod and a failing SQL never exposes the query in the response metadata.
  • SC-005: Zero silent total failures — any exit path that can't produce real data MUST emit a DataResult with metadata.error populated, never return an empty list.
  • SC-006: Behavior parity with the pre-FIX-004 inline implementation: every question that worked before MUST still work after. This is verified with a replay suite on a sample of real queries from successful_queries.

6. Assumptions & Out of Scope

Assumptions

  • Bedrock Claude Haiku 4.5 is available for both the initial generation and the sql_fixer retries.
  • The caller has already filtered tables to a manageable set (<=50) before invoking the subgraph.
  • save_successful_query() is idempotent enough that duplicate writes on the fire-and-forget path don't corrupt the cache.
  • 2 retries are empirically sufficient — deeper retry loops historically thrashed on complex schemas.
  • ThreadPoolExecutor contention at the ISQLSandbox.execute_readonly level is managed by the sandbox itself; the subgraph treats each execute as opaque.

Out of scope

  • Tool-calling analyst — the subgraph is a deterministic state machine, not an agentic loop. A future 017-agentic-analyst feature can hoist NL2SQL into a tool that the analyst calls mid-stream; that is a separate feature.
  • Parallel SQL candidates — generating multiple SQLs in parallel and picking the best is explicitly out of scope (keep-it-simple).
  • Multi-model NL2SQL (Haiku vs. Sonnet vs. Cohere) — Haiku only.
  • Write / DDL SQL — blocked at the validation layer of 010a-sql-sandbox, never reaches this subgraph.
  • Multi-turn clarification inside NL2SQL — if the question is ambiguous the planner is responsible for asking; this subgraph assumes an unambiguous question.
  • Replacing the sandbox_router.py /sandbox/ask POST endpoint — the endpoint still calls the subgraph synchronously for the admin SQL UI. Only the pipeline connector path changes.

7. Open Questions

  • [NEEDS CLARIFICATION CL-001] — Should the last-resort fallback be configurable to OFF for the /sandbox/ask admin endpoint (where an empty result is more informative than 10 arbitrary rows)? For now the subgraph always attempts it; the admin UI will need to surface used_fallback=true clearly.
  • [NEEDS CLARIFICATION CL-002] — Is 2 retries still the right default after integrating the subgraph? Measure the retry distribution on real traffic for a week before tuning.

8. Tech Debt Discovered / Closed

  • [CLOSED — FIX-004] 2026-04-11 — The NL2SQL subgraph is no longer dead code. The inline loop in connectors/sandbox.py has been removed and replaced with a call to the compiled subgraph defined in application/pipeline/subgraphs/nl2sql.py. The duplication between the two implementations is gone.
  • [DEBT-001] — 2 self-correction retries is a magic constant passed through state. It is configurable per-invocation but not exposed through settings or DI. Low priority.
  • [DEBT-002] — _route_after_format reads state.result AFTER format_result_node already consumed it. This works because state fields are preserved across nodes, but it is subtle — a future refactor that reshapes the state risks silently breaking the INDEC branch. Add an explicit needs_indec_fallback flag computed by format_result_node if the edge function grows.
  • [DEBT-003] — Runtime dependencies were threaded through LangGraph state (llm, sandbox, embedding, semantic_cache), which became unsafe once checkpointing was active because FallbackLLMAdapter and friends are not msgpack-serializable. FIXED 2026-04-13: runtime services now live in request-scoped context, while the persisted state contains only checkpoint-safe data.

End of spec.md