Address Copilot findings on per-organisation statement limits (#334, #338)

[ashwin-agami]

Addresses the Copilot review findings left open on two merged PRs: #334 (per-organisation statement limits) and #338 (statement_limit_is_usable). Includes the comments Copilot left in the collapsed "Suppressed comments" section of its reviews.

The owner's decisions, applied throughout:

• No policy ceiling on either limit. The only checks are on what the engines and drivers can represent.
• One simple bound per limit. No backstop-skipping logic per engine.
• Smallest correct change for everything else.

Findings

#	Source	Finding	What was done
1	#334, tools.py:1934 (suppressed)	Execution fetches cap + 1 rows (fetchmany, psycopg2 itersize), so a huge row cap failed the statement with OverflowError	New execute_sql._row_cap_is_representable: usable only if cap + 1 <= 2**31 - 1, the C drivers' signed 32-bit count. Applies to AGAMI_SQL_MAX_ROWS (_row_cap_from_env) and to provider values (statement_limit_is_usable). Anything past the bound falls back to the deployment value.
2	#334, execute_sql.py:1651 (both copies)	A timeout accepted by the Python-side check could still overflow an engine's own timeout setting (Postgres int32 ms statement_timeout) and fail before the query ran	_timeout_is_representable is now timeout_s <= 604_800 (seven days). That is Snowflake's documented maximum for STATEMENT_TIMEOUT_IN_SECONDS and the smallest among the engines. It also keeps Postgres's int32 milliseconds and threading.TIMEOUT_MAX safe. A longer value falls back like any other unusable value, from the provider and from AGAMI_SQL_TIMEOUT_S.
3	#334, tools.py:1985 (suppressed)	A custom mapping whose __bool__/get raises escaped _effective_statement_limits	The provider's mapping is now read once, inside the same try as the provider call, using is not None instead of truthiness. A mapping that raises falls back like a provider that raises.
4	#334, mcp_http.py:510	With a provider registered, the whole of _listed ran on a worker thread, including the consumer's visibility predicate	_list_tools now filters names with _visible in the request task. Only _described(names) (the provider and descriptions) is sent off the event loop.
5	#334, tools.py:3742	The description said "the deployment row ceiling" and then gave the organisation's own number	Changed to "the row limit". The next sentence gives the caller's number.
6	#334, execute_sql.py:2708 (both copies)	The comment said no worker reads the pinned limits, but the engine functions in that worker call both resolvers	Comment corrected: the copied context is load-bearing for the watchdog, the native timeout and the fetch window.
7	#334, mcp_harness.py:88 (suppressed)	Add an end-to-end stdio tools/list test	Not done, by owner decision (keep this PR minimal). Per-caller descriptions are already covered by tool_description and the HTTP handler tests.
8	#334, tools.py:1913 (suppressed)	The provider docstring said "any positive whole number"	Now states the two representability exceptions (row cap 2**31 - 1 or more, timeout over 604,800 s).
9	#338, tools.py:1965	_STATEMENT_LIMIT_KEYS was defined twice	The second definition is removed.
10	#338, test_statement_limit_is_usable.py:14 (suppressed)	Unnecessary pytest.importorskip("pydantic")	Removed. Verified the module passes on a bare agami-core install without pydantic (33 passed).

packages/agami-core/src/execute_sql.py was copied over plugins/agami/lib/execute_sql.py, and the two are byte-identical (cmp, plus the ACE-093 byte-identity test).

Tests

• tests/test_statement_limit_is_usable.py: one edge test per limit. Row cap 2**31 - 2 is usable and 2**31 - 1 is not. Timeout 604_800 is usable and 604_801 is not. The provider-path test also covers both edges.
• tests/test_per_org_statement_limits.py:
  • A provider returning a mapping that raises when read falls back to the deployment values.
  • With a provider registered, the visibility predicate runs in the request task, and the descriptions still carry the caller's numbers.
  • The existing "largest accepted timeout" constant is moved to the new bound.
• Both new behaviour tests fail against main's tools.py / mcp_http.py (ConnectionError escapes; the predicate runs off the task) and pass with this change.
• Targeted run: test_per_org_statement_limits, test_statement_limit_is_usable, test_ace038_timeout, test_ace044_bounded_fetch, test_execute_sql_states_limits, test_ace087_result_bound, test_mcp_http, test_mcp_harness and the four parity/byte-identity tests. Result: 481 passed, 1 skipped.
• Full suite: 39 failed, 5704 passed, 12 skipped. That matches the known baseline of exactly 39 pre-existing failures on main, and none of the 39 are in a test file this PR touches.

uvx ruff check is clean on the changed files. No existing file was reformatted; the rewritten test_statement_limit_is_usable.py passes ruff format --check.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

The timeout skew boundary and related tests/documentation need correction, and an environment-boundary regression test is missing.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR hardens per-organisation SQL statement-limit handling across executor, tooling, and MCP paths.

Changes:

• Adds row-cap and timeout representability checks.
• Hardens provider fallback and visibility filtering.
• Updates regression tests and changelog documentation.

File summaries

File	Reviewed changes
tests/test_statement_limit_is_usable.py	Tests statement-limit boundaries.
tests/test_per_org_statement_limits.py	Tests provider fallback, visibility, and timeout limits.
plugins/agami/lib/execute_sql.py	Mirrors executor validation changes.
packages/agami-core/src/tools.py	Updates provider validation and descriptions.
packages/agami-core/src/mcp_http.py	Keeps visibility filtering in the request task.
packages/agami-core/src/execute_sql.py	Adds executor representability validation.
CHANGELOG.md	Documents the follow-up changes.

Review details

Suppressed comments (3)

CHANGELOG.md:25

• This documents 604,800 seconds as the largest usable base timeout, but the executor adds a 5-second native skew before sending the setting. If 604,800 is the native maximum, the documented usable base edge is 604,795; please keep this statement aligned with the runtime predicate and tests.

  the log. There is no policy ceiling, only what the engines can represent: a time limit over seven
  days (604,800 seconds, Snowflake's own maximum and the smallest among the supported engines) and a
  row cap of 2,147,483,647 or more (the drivers fetch one row past the cap, in a 32-bit count) are
  treated as unusable, from the provider and from `AGAMI_SQL_TIMEOUT_S` / `AGAMI_SQL_MAX_ROWS` alike.

tests/test_per_org_statement_limits.py:493

• _LARGEST_ARMABLE is set to 604,800 even though the engine receives that budget plus the 5-second native skew. With a 604,800-second native maximum, the largest accepted base budget is 604,795; otherwise this acceptance test preserves the invalid boundary.

_LARGEST_ARMABLE = 604_800  # seven days: the smallest native maximum among the engines

tests/test_statement_limit_is_usable.py:37

• This edge test encodes the same incorrect boundary as the implementation: the native timeout is armed at timeout_s + 5, so a base value of 604,800 would request 604,805 and must be rejected when 604,800 is the engine maximum. After tightening the helper, make 604,795 the accepted edge and adjust the next rejected value.

def test_a_timeout_past_seven_days_is_not_usable():
    assert tools.statement_limit_is_usable("timeout_s", 604_800) is True
    assert tools.statement_limit_is_usable("timeout_s", 604_801) is False

• Files reviewed: 7/7 changed files
• Comments generated: 3
• 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 45d0db1: test_an_environment_row_cap_the_drivers_cannot_fetch_falls_back sets AGAMI_SQL_MAX_ROWS to 231-1 (falls back to the default) and 231-2 (kept).

[ashwin-agami]

Fixed in 45d0db1: _timeout_is_representable now checks the native value, timeout_s + _NATIVE_BOUND_SKEW_S <= 604_800, so 604,795 is the largest usable limit (both copies; vendored copy identical). Edge tests moved to 604,795 / 604,796.

[ashwin-agami]

Fixed in 45d0db1: _timeout_is_representable now checks the native value, timeout_s + _NATIVE_BOUND_SKEW_S <= 604_800, so 604,795 is the largest usable limit (both copies; vendored copy identical). Edge tests moved to 604,795 / 604,796.

[ashwin-agami]

Copilot's suppressed comments (CHANGELOG.md:25, tests/test_per_org_statement_limits.py:493, tests/test_statement_limit_is_usable.py:37) — the same off-by-skew boundary — are fixed in 45d0db1: the CHANGELOG now states the native setting (limit + 5 s skew) must stay within 604,800 s, _LARGEST_ARMABLE is 604_800 - _NATIVE_BOUND_SKEW_S, and the edge test accepts 604,795 and rejects 604,796.