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

CHANGELOG.md

@@ -19,13 +19,16 @@ below corresponds to one such version.
   through `Adapters.statement_limits` or `tools.set_statement_limits_provider`. A missing, `None` or
   unusable value (not a positive whole number, or a provider that raises) falls back to
   `AGAMI_SQL_MAX_ROWS` / `AGAMI_SQL_TIMEOUT_S`, which stay the deployment default, with a warning in
-  the log. There is no ceiling. A time limit too large for the platform to arm a timer on (at or
-  above Python's `threading.TIMEOUT_MAX`, counting the supervisor's 60-second slack) is treated as
-  unusable, from the provider and from `AGAMI_SQL_TIMEOUT_S` alike.
+  the log. There is no policy ceiling, only what the engines can represent: a time limit whose native
+  setting — the limit plus the executor's 5-second skew — would pass seven days (604,800 seconds,
+  Snowflake's own maximum and the smallest among the supported engines; so 604,795 is the largest
+  usable limit) 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.
   - An evaluation run scores both statements of each case under the named organisation's limits.
   - `tools.statement_limit_is_usable(key, value)` is the rule a provider's values are held to (a
-    positive whole number, and a timeout the platform can arm), public so a settings screen can
-    refuse at save time what the executor would otherwise decline on every statement.
+    positive whole number within those two bounds), public so a settings screen can refuse at save
+    time what the executor would otherwise decline on every statement.
   - The limits are resolved once per `execute_sql` call and held for the whole call, and the forked
     child is handed the same two numbers in its environment, so the watchdog, the native bound, the
     outer bound and the supervisor still derive from one budget on both sides of the fork.
@@ -45,6 +48,16 @@ below corresponds to one such version.
 
 ### Fixed
 
+- **Follow-ups to per-organisation statement limits** (#334, #338):
+  - A row cap too large for the drivers to fetch (2,147,483,647 or more) and a time limit over seven
+    days now fall back to the deployment value, instead of failing every statement with an
+    `OverflowError` or a native timeout the engine rejects before the query runs.
+  - A provider mapping that raises when read falls back like a provider that raises, instead of
+    escaping the resolver.
+  - With a provider registered, the HTTP server's tool-visibility predicate runs in the request task
+    again, as `build_server` documents; only the descriptions are computed off the event loop.
+  - The `execute_sql` description no longer names "the deployment row ceiling" before stating the
+    caller's own row limit.
 - **A table outside the connection's default schema resolves when the client names it without its
   schema** (#258). A large model is served at the `summary` tier, whose area table lists carried a
   bare name, so the client wrote `FROM orders` and a warehouse keeping it in `sales_data` answered

packages/agami-core/src/execute_sql.py

@@ -1544,17 +1544,24 @@ def _pin_statement_limits(max_rows: int, timeout_s: int) -> Token[tuple[int, int
 def _row_cap_from_env() -> int:
     """The DEPLOYMENT row cap: `AGAMI_SQL_MAX_ROWS`, default 1000 when unset. An operator owns their
     availability tradeoff and may set it higher OR lower than 1000; it is NOT a hard 1000 ceiling. A
-    missing/invalid/zero env value falls back to 1000."""
+    missing/invalid/zero/unrepresentable env value falls back to 1000."""
     raw = os.environ.get("AGAMI_SQL_MAX_ROWS", "").strip()
     # `isdecimal`, not `isdigit`, for the reason `_timeout_s_from_env` gives: `isdigit` admits `²`,
     # which `int()` then refuses. `tools` now resolves this while building its registry, so a raise
     # here would stop the module importing at all rather than failing one call.
     cap = int(raw) if raw.isdecimal() else _DEFAULT_MAX_ROWS
-    if cap <= 0:
+    if cap <= 0 or not _row_cap_is_representable(cap):
         cap = _DEFAULT_MAX_ROWS  # "0" / "00" → the default, never an empty result
     return cap
 
 
+def _row_cap_is_representable(cap: int) -> bool:
+    """Whether the drivers can be asked for this cap. Not a ceiling (#329): every engine fetches
+    `cap + 1` rows in one call (`fetchmany`, psycopg2's `itersize`), and the C drivers hold that count
+    in a signed 32-bit int — past it the fetch raises `OverflowError` after the statement has run."""
+    return cap + 1 <= 2**31 - 1
+
+
 def _resolve_row_cap() -> int:
     """Effective result-row cap for THIS call: the organisation's own when a call pinned one
     (`_statement_limits`, #329), otherwise the deployment's (`_row_cap_from_env`).
@@ -1646,9 +1653,14 @@ def _timeout_is_representable(timeout_s: int) -> bool:
     wait all refuse a timeout at or above `threading.TIMEOUT_MAX` with an `OverflowError`. The outer
     bound raises it AFTER its worker has started and BEFORE the abandonment is counted, so an
     unrepresentable budget would not merely fail one call: it would leave `_MAX_ABANDONED_WORKERS`
-    bounding nothing. The largest derived bound is the supervisor's, so that is the one checked, and a
-    value failing it is treated as unusable like any other and falls back to the deployment's."""
-    return timeout_s + _SUPERVISOR_SKEW_S < threading.TIMEOUT_MAX
+    bounding nothing. The engines' native backstops are narrower still: Snowflake's
+    `STATEMENT_TIMEOUT_IN_SECONDS` stops at 604,800 (seven days), the smallest maximum among the
+    engines, and Postgres's `statement_timeout` is an int32 of milliseconds — past either, the native
+    bound fails before the query runs. Seven days is inside every one of those, so it is the one bound
+    checked, and a value past it is treated as unusable like any other and falls back to the
+    deployment's. It is checked on the NATIVE value, which is the budget plus `_NATIVE_BOUND_SKEW_S`,
+    because that is the number the engine receives — so the largest usable budget is 604,795."""
+    return timeout_s + _NATIVE_BOUND_SKEW_S <= 604_800
 
 
 def _timeout_s_from_env() -> int:
@@ -2703,9 +2715,11 @@ def _execute_bounded(
 
     The call runs inside a copy of the CALLER's context, and what that is FOR changed when the
     per-call row cap went (ACE-087). It used to carry ``_max_rows_override`` to ``_resolve_row_cap``
-    inside the worker; that override is gone. It now also carries the call's pinned organisation
-    limits (``_statement_limits``, #329), though nothing in the worker reads them today — every bound
-    is derived on the caller's side. What it chiefly carries is the *caller's* request scope — ``tools._current_org_ctx``, the resolve-once
+    inside the worker; that override is gone. It now carries the call's pinned organisation limits
+    (``_statement_limits``, #329), and that is load-bearing: the built-in executor's engine functions
+    run in this worker and call ``_resolve_timeout_s`` (watchdog, native bound) and
+    ``_resolve_row_cap`` (the fetch window) there, so without the copy they would silently enforce the
+    deployment's limits instead of the organisation's. It also carries the *caller's* request scope — ``tools._current_org_ctx``, the resolve-once
     request cache, the actor and session on the served path — into the one place a consumer's own
     code runs. That is the point of the ``Executor`` seam: a pooled / per-user-RBAC executor picks
     its connection from exactly that context, and a new thread starts with an empty one, so dropping

packages/agami-core/src/mcp_http.py

@@ -485,29 +485,32 @@ def _visible(name: str) -> bool:
         instructions = f"{instructions}\n{extra_instructions}"
     server = Server(SERVER_NAME, version=server_version(), instructions=instructions)
 
-    def _listed() -> list:
+    def _described(names: list[str]) -> list:
         return [
             # `tool_description` states execute_sql's limits for THIS caller's organisation (#329).
             # That is core describing its own tool per request, not the subtractive-only hook above
             # reshaping one: a consumer's description passes through it untouched.
             mt.Tool(
                 name=name,
-                description=tool_description(name, meta["description"]),
-                inputSchema=meta["inputSchema"],
+                description=tool_description(name, registry[name]["description"]),
+                inputSchema=registry[name]["inputSchema"],
             )
-            for name, meta in registry.items()
-            if _visible(name)
+            for name in names
         ]
 
     @server.list_tools()
     async def _list_tools() -> list:
-        # Off the loop, for ACE-048's reason: the limits provider is the consumer's code and usually a
-        # database read, and on the loop one slow read would stall every in-flight request.
-        # `run_blocking` copies the request context, so the organisation is still set in the worker.
-        # Without a provider nothing here blocks, so the hop would be a thread per listing for nothing.
+        # The visibility predicate runs HERE, in the request task, before any hop: that is the context
+        # its contract promises a consumer, who may read request-task state from it. Only the
+        # descriptions go off the loop, for ACE-048's reason: the limits provider is the consumer's
+        # code and usually a database read, and on the loop one slow read would stall every in-flight
+        # request. `run_blocking` copies the request context, so the organisation is still set in the
+        # worker. Without a provider nothing here blocks, so the hop would be a thread per listing for
+        # nothing.
+        names = [name for name in registry if _visible(name)]
         if not has_statement_limits_provider():
-            return _listed()
-        return await run_blocking(_listed)
+            return _described(names)
+        return await run_blocking(_described, names)
 
     @server.call_tool()
     async def _call_tool(name: str, arguments: dict) -> list:

packages/agami-core/src/tools.py

@@ -1951,8 +1951,10 @@ def set_statement_limits_provider(
 
     ``provider(org_id)`` returns ``{"max_rows": int | None, "timeout_s": int | None}`` or ``None``. A
     missing key, a ``None``, or a ``None`` result means "this organisation has no limit of its own" and
-    the deployment's environment value applies. There is no ceiling: an administrator may set either
-    number to any positive whole number. Called by ``mcp_http.create_app`` from
+    the deployment's environment value applies. There is no policy ceiling, only what the engines can
+    represent: either number may be any positive whole number except a row cap of ``2**31 - 1`` or
+    more and a timeout over 604,800 seconds, which fall back like any other unusable value (see
+    ``statement_limit_is_usable``). Called by ``mcp_http.create_app`` from
     ``adapters.statement_limits``; an embedder with no HTTP app may call it directly."""
     global _STATEMENT_LIMITS_PROVIDER
     if provider is not None and not callable(provider):
@@ -1962,19 +1964,17 @@ def set_statement_limits_provider(
     _STATEMENT_LIMITS_PROVIDER = provider
 
 
-_STATEMENT_LIMIT_KEYS = ("max_rows", "timeout_s")
-
-
 def statement_limit_is_usable(key: str, value: Any) -> bool:
     """Whether ``value`` is a limit the executor would enforce for ``key`` (``max_rows`` or
     ``timeout_s``) — the rule the provider's values are held to, exposed so a settings screen can refuse
     at save time what the executor would otherwise decline, with a warning, on every statement.
 
     A positive ``int``, and not a ``bool``: ``True`` reaching the row cap as ``1`` is a storage bug that
-    would look like a setting. There is no ceiling (#329), but a timeout the platform cannot arm is not
-    a setting either — see ``execute_sql._timeout_is_representable`` for how one would disable the
-    abandoned-worker cap. An unknown ``key`` raises ``ValueError``: that is the caller's bug, not a value
-    to decline."""
+    would look like a setting. There is no ceiling (#329), but a number the engines cannot represent is
+    not a setting either: a timeout over seven days (``execute_sql._timeout_is_representable``) and a
+    row cap whose ``cap + 1`` fetch overflows a 32-bit count (``execute_sql._row_cap_is_representable``).
+    The environment values are held to the same checks. An unknown ``key`` raises ``ValueError``: that
+    is the caller's bug, not a value to decline."""
     if key not in _STATEMENT_LIMIT_KEYS:
         raise ValueError(f"unknown statement limit {key!r}; expected one of {_STATEMENT_LIMIT_KEYS}")
     if not isinstance(value, int) or isinstance(value, bool) or value <= 0:
@@ -1983,7 +1983,9 @@ def statement_limit_is_usable(key: str, value: Any) -> bool:
         from execute_sql import _timeout_is_representable
 
         return _timeout_is_representable(value)
-    return True
+    from execute_sql import _row_cap_is_representable
+
+    return _row_cap_is_representable(value)
 
 
 def _provider_limit(org_id: str, key: str, value: Any) -> int | None:
@@ -2023,6 +2025,21 @@ def _effective_statement_limits(org_id: str | None) -> tuple[int, int]:
     org_id = org_id or _current_org_id()
     try:
         supplied = provider(org_id)
+        if supplied is not None and not isinstance(supplied, Mapping):
+            _LOG.warning(
+                "statement limits provider returned %r for org %s, not a mapping; using the "
+                "deployment values.",
+                type(supplied).__name__,
+                org_id,
+            )
+            supplied = None
+        # Read INSIDE the guard, and read once. Any `Mapping` is allowed, so a lazy or custom one can
+        # run the consumer's code on `get` just as the call itself does — and a raise there is the
+        # provider failing, not a bug of ours to let escape the resolver. `is not None` rather than
+        # truthiness, so its `__bool__`/`__len__` is never asked either.
+        values = (
+            {key: supplied.get(key) for key in _STATEMENT_LIMIT_KEYS} if supplied is not None else {}
+        )
     except Exception:
         # The provider is the consumer's code, usually a database read. Failing it must not fail the
         # statement: the deployment's own limits are a safe, known answer, and the log carries why.
@@ -2031,17 +2048,9 @@ def _effective_statement_limits(org_id: str | None) -> tuple[int, int]:
             org_id,
             exc_info=True,
         )
-        supplied = None
-    if supplied is not None and not isinstance(supplied, Mapping):
-        _LOG.warning(
-            "statement limits provider returned %r for org %s, not a mapping; using the deployment "
-            "values.",
-            type(supplied).__name__,
-            org_id,
-        )
-        supplied = None
+        values = {}
     for key in _STATEMENT_LIMIT_KEYS:
-        value = _provider_limit(org_id, key, (supplied or {}).get(key))
+        value = _provider_limit(org_id, key, values.get(key))
         if value is not None:
             limits[key] = value
     return limits["max_rows"], limits["timeout_s"]
@@ -3752,9 +3761,12 @@ def tool_description(name: str, description: str) -> str:
             "  {status:'refused', refusal:{reason, rule, detail, remediation}, receipt, audit_id} "
             "— OUR decision, so it always names its fix: relay the `remediation`, it says how to "
             "get an answer. SELECT-only is enforced, so DML/DDL/multi-statement arrive here, as "
+            # "The row limit", with no owner and no number: the next sentence states the number that
+            # applies to this caller, and naming "the deployment" here would advertise a second cap
+            # whenever an organisation has its own (#329).
             "do an out-of-scope table or column, a per-statement deadline, and a result larger "
-            "than the deployment row ceiling (refused rather than trimmed, so a partial answer "
-            "never arrives looking whole).\n"
+            "than the row limit (refused rather than trimmed, so a partial answer never arrives "
+            "looking whole).\n"
             # The numbers behind the two limits above (#326); see `_execute_sql_limits_sentence`.
             # Replaced per caller at list-tools time (#329); see `tool_description`.
             + _EXECUTE_SQL_LIMITS_AT_IMPORT