Cut SQL retries: Redshift dialect rules, stated row/time limits, resolved datasource in the log

[ashwin-agami]

Closes #325, closes #326, closes #328.

Three changes that cut execute_sql retries — each one a warehouse round trip plus a full client turn — or make them visible in the activity log.

#325 — Redshift dialect rules on the schema response

• New flat module sql_dialect_rules.py: per-engine rules keyed by the model's StorageType; Redshift is the only entry. Read-path only.
• get_datasource_schema adds dialect_rules beside the prompt_examples pointer on both branches — inside the size-budget loop on the budgeted one, so the ~1.6k chars are measured.
• Declared on DatasourceSchemaResult; the tool description and the shared instructions tell the client to follow it.

#326 — execute_sql states the row cap and deadline

• tools.statement_limits() returns {max_rows, timeout_s} from the executor's own resolvers; _execute_sql_limits_sentence() puts both numbers, and what to do about each, into the description when the registry is built.
• Both resolvers read only the process environment, so the stated number is the enforced one. An embedder that changes AGAMI_SQL_MAX_ROWS/AGAMI_SQL_TIMEOUT_S after importing tools would see a stale description — nothing in core or the hosted entrypoint does.
• statement_limits() is public for the admin Settings screen that will show the limits in force. Per-organisation editing is Let an organisation's administrator set the row cap and statement deadline #329.

#328 — the activity log records the resolved datasource

• The three datasource-scoped handlers resolve through _resolve_call_datasource, which publishes the result on a ContextVar; reset_typed_outcome clears it and typed_outcome_overrides hands it to record_tool_call beside (not as part of) the outcome trio.
• tool_calls.datasource is now the resolved datasource; new datasource_source column (migration 025): explicit / resolved / NULL.

Decisions

• Rules on the schema response, not a pre-execution refusal. A refusal naming the rewrite still spends the retry; prevention removes it.
• Rules inside the size budget. Honest measurement; a schema near the budget may downgrade one rung sooner on Redshift.
• Resolution published before the model loads. A call naming no datasource on a deployment with none is recorded as default / resolved rather than empty — it is what the server tried.
• datasource_source derives from the argument when no handler published a value, so an embedder that states nothing keeps today's behaviour.

Tests

• test_schema_dialect_rules.py, test_execute_sql_states_limits.py, test_tool_calls_resolved_datasource.py (17 tests: rules on both branches and absent for other engines; limits from env overrides; explicit/resolved/NULL rows through migration 025; each handler publishes via a caller-owned Context; no leak into the next tool).
• Full suite: 39 failures, the identical set to origin/main at a365b10 (pre-existing).

Review

/agami-sdlc:review: 0 must-fix. Applied: Redshift ordered-aggregate and reserved-identifier rules widened; unused fixture removed. Not applied: a transport-level end-to-end test through build_server, and reading back through _TOOL_CALL_COLS — the handler test mirrors the transport's context handling and the column is exercised by insert.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Address the critical import-time parsing issue and the listed dialect-rule and datasource-provenance issues before approval.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds Redshift dialect guidance, exposes SQL row/time limits, and records resolved datasource provenance to reduce avoidable retries.

Changes:

• Adds engine-specific rules to schema responses.
• Documents effective execute_sql limits.
• Tracks datasource resolution in activity logs.

File summaries

File	Summary and findings
tests/test_tool_calls_resolved_datasource.py	Tests explicit and resolved datasource logging.
tests/test_schema_dialect_rules.py	Tests dialect rules across schema paths and engines.
tests/test_execute_sql_states_limits.py	Tests configured SQL limit descriptions.
packages/agami-core/src/tools.py	Critical: invalid Unicode digits can prevent module import during registry construction. Nit: cancellation timing is overstated for some executors.
packages/agami-core/src/sql_dialect_rules.py	Moderate: preserve empty-count semantics, SQL NULL booleans, and array-valued ARRAY_AGG results.
packages/agami-core/src/model_store.py	Moderate: add list_sessions round-trip coverage for explicit and resolved datasource provenance.
packages/agami-core/src/migrations/core/025_tool_calls_datasource_source.sql	Adds the datasource provenance column.
packages/agami-core/src/contracts.py	Extends schema and activity-log contracts.
packages/agami-core/pyproject.toml	Packages the new dialect-rules module.
CHANGELOG.md	Documents the changes.

Review details

Suppressed comments (5)

packages/agami-core/src/model_store.py:635

• The new tests only query SELECT * after insertion; none exercises model_store.list_sessions, whose _TOOL_CALL_COLS projection is the path the Activity UI reads. A regression that drops datasource_source from this SELECT would still pass the insert assertions while silently hiding the provenance from the UI. Add a round-trip assertion through list_sessions for the resolved and explicit values.

    "client_model, datasource_source"

packages/agami-core/src/sql_dialect_rules.py:22

• COUNT(*) FILTER (WHERE c) returns 0 for an empty input, but the proposed SUM(CASE ...) returns NULL there. Since this is presented as a general rewrite, it can change an ungrouped empty result; preserve the count semantics with COALESCE or explicitly limit the rule to non-empty grouped inputs.

- No FILTER on aggregates. COUNT(*) FILTER (WHERE c) -> SUM(CASE WHEN c THEN 1 ELSE 0 END); a \
filtered average -> AVG(CASE WHEN c THEN v END) (NULL, not 0, in the ELSE).

packages/agami-core/src/sql_dialect_rules.py:32

• This rewrite maps SQL NULL booleans to 'false', because a CASE WHEN col ... ELSE ... treats both false and unknown as the ELSE branch. A boolean cast/string conversion preserves NULL, so following this rule can change query results; use explicit true/false branches with an implicit NULL ELSE.

- A BOOLEAN cannot be cast to VARCHAR or passed to a string function (BTRIM, CONCAT). Use \
CASE WHEN col THEN 'true' ELSE 'false' END.

packages/agami-core/src/sql_dialect_rules.py:25

• ARRAY_AGG is not a string-aggregation rewrite: replacing it with LISTAGG changes an array-valued result into text and can change element/null semantics. A client following this blanket rule can return the wrong type even when the SQL succeeds; scope LISTAGG to STRING_AGG and explain that array results have no generic text-equivalent rewrite.

- No STRING_AGG or ARRAY_AGG. Use LISTAGG(col, ', ') WITHIN GROUP (ORDER BY col). The delimiter must \
be a constant, and every ordered aggregate in one SELECT (LISTAGG, PERCENTILE_CONT, MEDIAN) must use \
the same ordering.

packages/agami-core/src/tools.py:3201

• The description promises cancellation immediately after timeout_s for every executor, but BigQuery applies its native bound at timeout_s + 5 and injected executors are only stopped waiting at timeout_s + 10; the underlying work may continue. This makes the client-facing enforcement statement inaccurate. Describe this as the configured deadline/bound without promising cancellation, or make every executor honor cancellation at the stated value.

        f"statement still running after {limits['timeout_s']}s is cancelled. Plan for both before "

• Files reviewed: 10/10 changed files
• Comments generated: 1
• Review effort level: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[ashwin-agami]

Fixed in 14edc1f. _resolve_row_cap now uses isdecimal, matching _resolve_timeout_s, so ² / ① / abc / -5 / 0 fall back to the default instead of raising while the registry builds. The vendored plugins/agami/lib/execute_sql.py got the identical change (parity tests pass). Covered by test_an_unusable_row_cap_falls_back_instead_of_raising.

[ashwin-agami]

Addressed Copilot's suppressed comments in 14edc1f:

• COUNT(*) FILTER rewrite — now COUNT(CASE WHEN c THEN 1 END), which stays 0 on an empty input (the SUM(CASE …) form was NULL). SUM/AVG use CASE WHEN c THEN v END with no ELSE.
• Boolean to text — now CASE WHEN col THEN 'true' WHEN NOT col THEN 'false' END, so NULL stays NULL.
• ARRAY_AGG — no longer mapped to LISTAGG; the rule says Redshift has no array result and LISTAGG returns text, so use it only when a text list is wanted.
• "is cancelled" — the limits sentence now says a statement past ~Ns is refused, without promising cancellation on executors that can only stop waiting.
• _TOOL_CALL_COLS coverage — test_both_values_reach_the_activity_reader reads explicit and resolved back through list_sessions.

test_rewrites_keep_the_result_not_just_avoid_the_error pins the three rule changes. New test files, vendored-parity and result-bound suites: 209 passed.