ACE-045: per-process model cache + single SQL parse + shared guard indices - #103
Merged
Merged
Conversation
…once Thread an optional GuardContext (one parsed sqlglot tree + the 4 model indices) through the _model_safety guard battery, so the SQL is parsed once and each index built once instead of 6x parse + per-guard index rebuild (audit P2). Backward-compatible: ctx=None keeps the standalone callers (cli.py) byte-identical; rebuild ctx after an auto-rewrite (SQL changes). Synced the bundled lib mirror (execute_sql.py). Tests: parse=1/index-build=1 counters, verdict-parity with/without ctx, unparseable degrade-to-allow. Full gate green (1413 tests). Spec: ACE-045
Cache the Organization in the long-lived parent, keyed (org_id, datasource, model_version), so the 2-3 loads/query collapse to 1 and it serves warm across queries + users. org_id comes from the OrgResolver seam via a _current_org_ctx ContextVar the HTTP server sets per request (mcp_http handle_mcp), falling back to AGAMI_ORG_ID/'local' (stdio/single-tenant) -> tenant-safe by construction, no F14 dependency, no downstream fork. Route _resolve_units/_resolve_receipt/ tool_get_datasource_schema through it. Bypass the cache when model_version is None (never serve a stale model). conftest isolates the module-global cache between tests. Full gate green. Spec: ACE-045
There was a problem hiding this comment.
Pull request overview
This PR implements ACE-045 performance hardening by (1) introducing a per-invocation GuardContext to avoid repeated SQL parsing and repeated semantic-model index builds across the safety-guard battery, and (2) adding a per-process semantic-model cache (get_cached_org) scoped by (org_id, datasource, model_version) so long-lived servers reuse warm models across queries while remaining tenant-safe via a request-scoped ContextVar.
Changes:
- Add
GuardContext/build_guard_contextand threadctx=through guard functions to reuse a single parsed SQL tree and shared indices. - Add per-process org/model caching in
tools.py, plus request-scoped org-id propagation in the HTTP MCP transport to keep caching org-scoped. - Add tests covering guard-context reuse/parity and org-cache warm hits, version invalidation, and org scoping.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_org_cache.py | Adds Slice 2 tests validating per-process org/model caching behavior and org scoping. |
| tests/test_guard_context.py | Adds Slice 1 tests ensuring single-parse/single-index-build and parity with/without ctx. |
| tests/conftest.py | Adds a global autouse fixture to isolate module-global org cache state between tests. |
| packages/agami-core/src/semantic_model/runtime.py | Introduces GuardContext and updates guards to optionally reuse parsed SQL + indices. |
| packages/agami-core/src/execute_sql.py | Builds guard context once and threads it through the safety battery; refreshes after rewrite. |
| plugins/agami/lib/execute_sql.py | Syncs the same guard-context threading change into the bundled lib/ mirror. |
| packages/agami-core/src/tools.py | Adds (org, datasource, version) model caching and uses it in tool paths that previously reloaded models. |
| packages/agami-core/src/mcp_http.py | Sets _current_org_ctx per request so the per-process model cache remains tenant-scoped. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…lglot, unify pre_flight parity, drop dup cache fixture - build_guard_context returns None when sqlglot unavailable (guards short-circuit to allow before touching ctx, so index-building was wasted work) - pre_flight_check non-ctx path routes through _parse_sql so ctx/non-ctx are byte-identical for unparseable SQL (was: 'unparseable; skipped' vs 'no SELECT; skipped') - remove per-file _isolate_cache fixture; conftest._reset_org_cache already covers it
Contributor
Author
|
Thanks @copilot — all three addressed:
Gate green (1417 passed). Pushed. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Behaviour-preserving perf hardening (audit finding P2, feature F12-runtime-scalability). The semantic model is parsed/indexed far more than needed per query. Two independent wins, same answers:
_model_safetyguard battery re-parsed the SQL 6× and rebuilt each model index per guard. AGuardContextnow parses once + builds each index once, threaded through the guards.(org_id, datasource, model_version), served warm across queries + users.Changes
semantic_model/runtime.py—GuardContext+build_guard_context; optionalctx=on the 6 guards +_preflight_select/_check_aggregation_semantics.ctx=Nonekeeps the standalone (cli.py) path byte-identical.execute_sql.py— build the context once in_model_safety, thread it; rebuild after an auto-rewrite (SQL changes).tools.py—get_cached_org+_current_org_ctx/_current_org_id; route_resolve_units/_resolve_receipt/tool_get_datasource_schema. Bypass the cache whenmodel_versionis None (never serve a stale model).mcp_http.py— set_current_org_ctxper request inhandle_mcpfrom the resolved org (mirrors_actor_ctx). Tenant-safe by construction: the cache key is org-scoped via theOrgResolverseam — N=1 today, per-request under a multi-tenant resolver, no fork, no F14 dependency.tests/—test_guard_context.py,test_org_cache.py,conftest.py(isolates the module-global cache).lib/mirror.Test plan / acceptance
ctxacross 6 SQL cases; unparseable→allow.local.uv run dev.py check).Out of scope (per spec)
Cross-query caching on the subprocess/file path (needs ACE-028); the multi-tenant resolver (agami-hosted F5) — this only consumes the
OrgResolverport.Spec: ACE-045