fix(graphile-llm): key agent discovery by database_id, and actually run express-context in CI - #1645
Merged
Merged
Conversation
…s-context in CI getAgentDiscovery ran an unfiltered LIMIT 1 against agent_chat_module and cached the result under a dbname, so in a schema-per-tenant serving database it resolved a neighbouring tenant's agent tables. Its bare catch also reported a dead pool as 'not provisioned'; now only 42P01/3F000 are swallowed. packages/express-context is in no CI batch, so the tenant-keying guard added in #1644 never ran. Added it to packages-core. constructive-planning#1403
Contributor
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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
Finishes the sweep started in #1644. Scanning every template literal in the repo that selects
FROM metaschema_*/routing_*/catalog_*/compute_*with aLIMIT 1and noWHEREleaves zero unkeyed lookups; the one remaining wasgraphile-llm's agent discovery:FROM metaschema_modules_public.agent_chat_module acm JOIN metaschema_public.schema s ON s.id = acm.schema_id + WHERE acm.database_id = $1 LIMIT 1Two things made it worse than the express-context copies:
It was keyed by
dbname, notdatabase_id—getAgentDiscovery(pool, dbname)cached under a database name while the query filtered on nothing at all, so in a schema-per-tenant serving database every tenant got whichever tenant's agent tables the planner reached first, memoised for 60s. The parameter is nowdatabaseId(a breaking signature change to a published export, but there are no in-repo callers, and the old contract could not return a correct answer).Its
catchwas bare, so the failure mode was invisible:A dead pool, a permission error or a malformed
databaseIdall reported "not provisioned", i.e. an API that silently loses its agent surface and answers 200. Now only the absence being probed for is swallowed — this is also what makes the newdatabaseIdcontract enforceable, since passing a dbname now raises22P02loudly instead of being eaten.packages/express-contextis in no CI batchWhich means the tenant-keying guard test #1644 added — the thing that is supposed to stop this class from coming back — has never run in CI. Added it to the
packages-coreunit batch (pure unit tests, no services).Worth a separate issue: 35 packages with tests that no CI job runs
While checking the above I enumerated the workspace against the workflow matrices. Excluding
express-context(fixed here), 34 packages / 85 test files are in no batch — includingpackages/errors, where #1643 just landedregistry-sync.test.tsspecifically to catch cross-repo drift. It passes locally; CI has never executed it. Full list:agentic/*(12),pgpm/transform(14 files),pgpm/export(9),pgpm/slice(6),graphile/graphile-upload-plugin,graphile-realtime-subscriptions,graphile-sql-expression-validator,postgres/pg-cache,packages/server-utils,packages/node-type-registry, and others. This is the same gapconstructive-planning#1410records for constructive-db. Not fixed here — some of those need services, and a few may be failing today.graphile-llm: 5 new tests (per-tenant resolution, cache keying, provisioned-vs-fatal error split, emptydatabaseId), lint and build clean.Link to Devin session: https://app.devin.ai/sessions/087553534c774929918ec4d378845881
Requested by: @pyramation