Serve many tenants: a tenant per agent request, and none by default by cachafla · Pull Request #193 · validmind/atryum

cachafla · 2026-11-10T06:24:54Z

For Agent Authority's multi-org phase 2. v0.8.0 (#189) put a tenant on every table. This lets one Atryum serve more than one of them. Nothing changes for a program that sets neither new option, standalone Atryum included.

Why

Atryum authenticates agent requests before the embedding program sees them, and builds the HTTP server itself, so an embedder has no way to set the tenant first. In AA every agent call ran as default: a second org's agent keys were unknown under row-level security, and its calls would have been recorded and decided as default's. Three more gaps:

The config file's [[upstreams]] fallback ignored the tenant. A tenant with no server of a given name fell back to the config's, with the config's credentials.
Managed Agents resumed and discovered only default's sessions.
A context with no tenant quietly acted for default.

What changes

WithRequestTenant(func(*http.Request) (string, error)). Every agent route (/mcp/, /api/v1/invocations, /api/v1/external/*, /api/v1/agent/*) asks it first. It runs before the auth middleware, so the agent key lookup already runs as that tenant, and the decider, event hook and upstream call follow. A request it returns an error or "" for gets 401 unknown credential, before any lookup. For a JWT, the embedder can read the issuer from the unverified token, because Atryum verifies it next.
A missing tenant is an error once WithRequestTenant is set. After migrations, a statement whose context names no tenant fails with store.ErrNoTenant instead of acting for default. Main's own startup work (agent sync, seeding [[upstreams]], server statuses, identity warnings, Managed Agents start) names default explicitly. The new tenant.Lookup(ctx) reports whether a context names one.
WithTenants(func(ctx) ([]string, error)). Managed Agents sessions are resumed at start and discovered every minute in each tenant it lists, as that tenant. Without it, only default is covered. Watchers are keyed by tenant and session, so one tenant's session never replaces another tenant's watcher.
Config [[upstreams]] belong to default. Resolving, listing and seeding use them only for default. Another tenant sees only the servers stored for it.
MCP client info is kept per tenant. The name and version an initialize leaves are keyed by tenant. Before, two tenants' clients from the same address and User-Agent (behind an ingress) shared an entry, so one tenant's client could label another tenant's calls.
The hook's 401 keeps the sign-in challenge. With an IdP configured, it carries the WWW-Authenticate header MCP clients follow to find the authorization server. A hook that panics answers 401 too.
PostgreSQL only. Main refuses WithRequestTenant on SQLite, which has no row-level security.

Tests

Each new test fails with its change reverted:

TestRequestTenantScopesAgentRoutes: a key resolves only under the tenant the hook names, and an unplaced request gets 401.
TestPostgresRequireTenant
TestConfigUpstreamsAreTheDefaultTenants
TestStartResumesEveryTenantsSessions: one watcher per tenant for the same session ID, each acting as its tenant.
TestClientInfoIsPerTenant
TestRequestTenantRefusalKeepsTheChallenge

go test ./... passes locally with ATRYUM_POSTGRES_TESTS=1. CI doesn't run the Postgres tests: no workflow sets that variable, so TestPostgresRequireTenant and the other TestPostgres* tests ran locally only.

Self-review

Two passes. Pass 1 found one blocking issue: the client info cache was shared across tenants, which a Postgres probe reproduced. It also flagged the missing sign-in challenge, SQLite, and a doc that said the operator routes "assume one tenant" when they fail. All four are fixed in 5e0fd8d. Pass 2 found nothing blocking. Its two small items (a panicking hook, and when it's safe to read the issuer) are fixed in the last commit.

The passes checked these and found nothing:

every agent route is wrapped;
with the strict check on, an agent request keeps its tenant end to end on Postgres, including a held call released through ResolvePending;
upstream sessions, the tool catalog and decision waiters are keyed by tenant;
no other in-memory map is keyed by a value the client chooses;
the strict check doesn't break pings, session resets, LISTEN or startup.

Docs

The changelog (Added, Security, Fixed) and the tenants section of docs/architecture.md.

Not in this PR

The operator API, UI and OAuth connect flow stay single-tenant. AA doesn't mount them.
Per-tenant Managed Agents accounts. [[managed_agents]] accounts are still process-wide, so two tenants binding the same Claude agent on a shared account would both watch its sessions. That's a config question for AA's phase 3.
AA adopting this (setting both options, naming the tenant on its own startup and background work) is AA's next PR, after a release.

🤖 Generated with Claude Code

For Agent Authority's multi-org phase 2 (SC-19505). Atryum authenticates agent requests before the embedding program sees them, so every agent call ran as the default tenant, and a second tenant's agent keys were unknown. - WithRequestTenant: every agent route asks the embedder which tenant a request acts for before anything authenticates it, and runs the request as that tenant. A request it can't place gets 401. With it set, a query whose context names no tenant fails (ErrNoTenant) instead of acting for default. - WithTenants: Managed Agents sessions are resumed and discovered in every tenant, as that tenant; watchers are keyed by tenant and session. - The config file's [[upstreams]] belong to the default tenant. Another tenant no longer falls back to them, with the config's credentials. - tenant.Lookup reports whether a context names a tenant. Main's startup work names the default tenant. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

From the self-review of #193: - The MCP client info remembered from initialize is keyed by tenant, so one tenant's client never labels another tenant's calls when both come from the same address and User-Agent (behind an ingress). - The request-tenant 401 keeps the WWW-Authenticate challenge when an IdP is configured, so an MCP client can still find where to sign in. - WithRequestTenant refuses SQLite, which has no row-level security. - The docs say the operator API, UI and OAuth connect flow fail with it set. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

…safe From the second self-review pass on #193: a panicking hook now answers 401 like a request it can't place, instead of dropping the connection, and the WithRequestTenant doc says reading the issuer is safe only with an IdP. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

panchicore

Merge with recommendations · 0 blocking · 1 small fix · no questions for you

Every agent route is placed by the hook before authentication, every statement on the tenant pool goes through the tenant check, and in-memory state (client info, catalog, upstream sessions, Managed Agents watchers) is keyed by tenant. The Postgres tests CI skips pass on a fresh Postgres 18, and reverting the config-upstream scoping or the strict check makes their tests fail.

Small fix: panchicore:F1, say in WithRequestTenant's doc that the strict check also binds the embedder's own queries. The WithDatabase hook already runs with it on, and WithRoutes handlers aren't placed by the hook. AA's startup work runs as no tenant today, so it would stop at startup once AA sets the option.

Ready for your approval, @panchicore.

_{calibrated-review v2 · review · run by @panchicore · head 15f2545}

Agent handoff · calibrated-review v2 · review · 15f2545

calibrated-review: 2
mode: review
pr: validmind/atryum#193
head: 15f2545f2f282d6288606f52b2a46030f1936174
merge_base: c4461dfe5818cfb4d3fc82a27a1fc3f72213ca88
run_by: panchicore
verdict: merge-with-recs
downgraded: 1 # in-memory keys (catalog, upstream sessions, client info) still use tenant.From, which falls back to default under the strict check; no request path reaches them without first querying the tenant pool, which fails closed, so no failure constructed
items:
  - id: panchicore:F1
    class: fix-now
    type: hygiene
    direction: safe
    title: WithRequestTenant's doc doesn't say the strict check binds the embedder's own queries and routes
    scenario: >-
      runServer calls store.RequireTenant() right after migrate and before the
      WithDatabase hooks run, and requireTenant is a package-global read by
      every tenantConn, so the embedder's own queries on Atryum's pool are
      strict too. WithRoutes handlers (extraRoutes) are not wrapped by
      withRequestTenant. AA's WithDatabase hook (cmd/vmaa/main.go:183-222 on
      AA main) runs authn.New, decide.New, dec.Listen and record.RunMaintenance
      with context.Background(), so with the option set it would fail with
      ErrNoTenant and log.Fatal at startup. Fails closed, at startup: not
      blocking. The doc says only that "Atryum's own startup and background
      work name tenant.Default", which reads as if the embedder's work were
      exempt.
    evidence:
      - pkg/atryum/atryum.go:259-268 RequireTenant() before `for _, hook := range o.databaseHooks`
      - internal/store/tenant.go requireTenant is a package-level atomic.Bool read in tenantConn.use
      - internal/api/handlers.go:1199-1201 extraRoutes registered without agentRuntimeHandler
      - pkg/atryum/options.go WithRequestTenant doc (added in this PR)
    fix: >-
      Add to WithRequestTenant's doc (and the CHANGELOG entry): the strict check
      holds for every query on Atryum's pool, the embedder's included, from the
      WithDatabase hook on; routes added with WithRoutes are not placed by f, so
      the embedder sets tenant.With on them and on its own background work.
    done_when: the doc states both, so an embedder adopting the option knows its startup and routes must name a tenant first
accepted_risks:
  - checked: every agent route is placed before authentication
    why: decider mode (handlers.go:1192-1201) and full mode (1213-1248) wrap /mcp/, /api/v1/invocations, /api/v1/external/*, /api/v1/agent/rules in agentRuntimeHandler, whose outermost tenant step is withRequestTenant; the remaining wrapper (atr_ refusal when keys are off) does no DB work
  - checked: no statement on the tenant pool skips the tenant check
    why: Exec, Query, prepared statements and BeginTx all call use(); no caller uses sql.Conn.Raw; LISTEN uses its own pgx connection and only wakes waiters, which re-read under their own tenant
  - checked: work no request starts
    why: Main's startup work uses defaultCtx; Managed Agents Start and the discovery loop go through eachTenant, and each watcher's context keeps its tenant; the only other ticker is the operator review stream
  - checked: in-memory state shared across tenants
    why: client info, tool catalog, upstream sessions and watchers are keyed by tenant; JWKS and userinfo caches are by issuer and hold public data; decision waiters are by invocation ID and only trigger a tenant-scoped re-read
  - checked: config upstreams
    why: Lookup returns the store error (ErrNoTenant included) before falling back, and the fallback is empty for any tenant but default
  - checked: impact on AA when it bumps to this tag without setting the options
    why: tenant.From still falls back to default and requireTenant stays off, so AA#163's behavior is unchanged; adoption is declared as AA's next PR
  - checked: Managed Agents accounts stay process-wide
    why: declared non-goal (AA phase 3)
verification:
  - run: ATRYUM_POSTGRES_TESTS=1 ATRYUM_POSTGRES_TEST_DSN=<fresh throwaway PG18> go test -count=1 ./...
    result: ok in all 12 packages; TestPostgresRequireTenant, TestPostgresTenantIsolation, TestPostgresTenantPreparedStatement, TestPostgresHeldCallWakesOnAnotherReplica and the other TestPostgres* PASS (not skipped)
  - run: gofmt -l ./cmd ./internal ./pkg; go vet ./...
    result: clean
  - run: revert Resolver.configured's tenant check, then TestConfigUpstreamsAreTheDefaultTenants
    result: FAIL (another tenant resolved the config upstream with config-secret)
  - run: revert the ErrNoTenant return in tenantConn.use, then TestPostgresRequireTenant
    result: FAIL (no tenant -> nil, want ErrNoTenant)
  - run: gh pr checks 193; git merge-tree origin/main 15f2545
    result: Go, Baseline and CLA pass, live harnesses skipped; merges cleanly, main unchanged since the merge-base
  - claim: an agent request keeps its tenant end to end on Postgres, including ResolvePending (PR body self-review)
    result: NOT CHECKED end to end; checked by reading the request path and the store-level Postgres tests

kam-validmind

Merge with recommendations · 0 blocking · 1 small fix · no questions for you

Every agent route is placed before authentication, and a held call keeps its tenant through approval with the strict check on. All tests pass with the Postgres tests running.

Small fix: kam-validmind:F1, refuse to start when the database login owns the tables or is a superuser. Row-level security doesn't bind such a login, so WithRequestTenant isolates nothing: in a probe, tenant larkspur resolved tenant acme's server and its credential. Main refuses only SQLite today, and it migrates with its serving login, so one DSN on a fresh database gives this. AA is unaffected: its service login isn't the owner.

Still open from the earlier review: panchicore:F1 (doc).

Ready for your approval, @kam-validmind.

_{calibrated-review v3 · review · run by @kam-validmind · head 15f2545}

Agent handoff · calibrated-review v3 · review · 15f2545

calibrated-review: 3
mode: review
pr: validmind/atryum#193
head: 15f2545f2f282d6288606f52b2a46030f1936174
merge_base: c4461dfe5818cfb4d3fc82a27a1fc3f72213ca88
run_by: kam-validmind
verdict: merge-with-recs
downgraded: 3
downgraded_notes:
  - >-
    kam-validmind:F1 was a blocking candidate (cross-tenant read, reproduced).
    Demoted at gate 2: it needs a serving role that owns the tables or bypasses
    row-level security, which docs/architecture.md:349-351 tells a multi-tenant
    embedder not to use, and the only embedder (Agent Authority) logs in as a
    non-owner member of aa_app.
  - >-
    Dropped: a Managed Agents watcher keeps running for a tenant WithTenants
    stops listing, until restart. No failing scenario constructed.
  - >-
    Dropped: Atryum never checks that a verified token's issuer belongs to the
    tenant the hook named. That mapping is the hook's contract (options.go
    RequestTenant doc), and no embedder places requests by anything but the
    credential.
items:
  - id: kam-validmind:F1
    class: fix-now
    type: correctness
    direction: risky
    title: WithRequestTenant isolates nothing when the serving role owns the tables or is a superuser, and Main does not refuse it
    scenario: >-
      An embedder sets WithRequestTenant and gives Main one PostgreSQL DSN on a
      fresh database. Main migrates with that same connection, so the serving
      role owns every table. Migration 034 enables row-level security but does
      not force it, so it does not bind the owner (nor a superuser or a
      BYPASSRLS role). The strict check passes (every context names a tenant),
      yet every tenant sees every row: an agent placed in tenant larkspur
      calling /mcp/bank resolves tenant acme's stored server "bank" with acme's
      credential, and one tenant's agent key resolves under another tenant.
      Main refuses SQLite for exactly this reason but starts silently here.
      Not blocking: the state contradicts docs/architecture.md:349-351 and
      Agent Authority's service login is not the owner.
    evidence:
      - pkg/atryum/atryum.go:259-265 refuses only a non-Postgres dialect before store.RequireTenant()
      - pkg/atryum/atryum.go:143 and :256 migrate on the serving pool, so on a fresh database the serving role owns the tables
      - pkg/migrations/034_tenant_scope.go:128 ENABLE ROW LEVEL SECURITY, with no FORCE
      - pkg/atryum/options.go:306-307 the option doc says PostgreSQL row-level security keeps tenants apart and names only the SQLite refusal
      - >-
        probe at head, role NOSUPERUSER NOBYPASSRLS that owns the database, store.InitDBWithDialect
        then store.RequireTenant(): CreateServer as acme (AuthToken acme-secret), then
        mcp.NewResolver(servers, config.Config{}).ResolveContext as larkspur returned
        url=http://acme.internal token=acme-secret err=nil, and CountServers as larkspur returned 1
    repro: >-
      In package invocation_test: as a superuser CREATE ROLE probe_owner LOGIN
      NOSUPERUSER NOBYPASSRLS and CREATE DATABASE probe_owner OWNER probe_owner;
      store.OpenDatabase as probe_owner; store.InitDBWithDialect; store.RequireTenant();
      store.NewServerRepoWithDialect(db, dialect).CreateServer(tenant.With(ctx, "acme"), mcp.Upstream{Name: "bank", Mode: "http", BaseURL: "http://acme.internal", AuthToken: "acme-secret", Enabled: true});
      then mcp.NewResolver(servers, config.Config{}).ResolveContext(tenant.With(ctx, "larkspur"), "bank") must fail and does not.
    fix: >-
      In the block at pkg/atryum/atryum.go:259-265, after the dialect check, ask
      PostgreSQL whether row-level security binds this login and return an error
      if it does not: the role is a superuser or has BYPASSRLS
      (SELECT rolsuper OR rolbypassrls FROM pg_roles WHERE rolname = current_user),
      or it owns a tenant table (pg_class.relowner, or membership in the owner,
      for a table without relforcerowsecurity). Say so in the WithRequestTenant
      doc next to the SQLite sentence. Alternative: FORCE ROW LEVEL SECURITY in a
      new migration, which also binds the owner but not a superuser.
    done_when: >-
      Main returns an error at startup with WithRequestTenant when the login is
      the table owner, a superuser or BYPASSRLS, and a Postgres test covers the
      owner case.
arch:
  - area: concurrency
    applies: 'yes'
    checked: >-
      requireTenant is an atomic.Bool set once before serving; the watcher map
      is read and written under s.mu with the tenant in the key; Start finishes
      its per-tenant resume before the discovery goroutine starts; the client
      info cache key now leads with the tenant. Discovery's known-then-upsert
      check is unchanged by this PR.
    findings: []
  - area: messaging
    applies: 'no'
    checked: no producer or consumer changed; the decision NOTIFY carries an invocation ID and the waiter re-reads under its own tenant
    findings: []
  - area: security
    applies: 'yes'
    checked: >-
      Every agent route in both route sets is wrapped by withRequestTenant
      outside the auth middleware; lookups by client-supplied IDs are scoped by
      row-level security, which is what F1 is about; the validator picks the
      issuer config from the token and verifies the signature against that
      issuer's keys, so the doc's claim about reading the unverified issuer holds.
    findings: ['kam-validmind:F1']
  - area: database
    applies: 'yes'
    checked: >-
      tenantConn.use returns ErrNoTenant before any statement, so a pooled
      session left on another tenant never serves a context with none;
      Exec, Query, prepared statements and BeginTx all pass through it; no
      production code uses a context-less db call or sql.Conn.Raw; RequireTenant
      is set after migrate.
    findings: []
  - area: scalability
    applies: 'yes'
    checked: discovery runs two queries per listed tenant per minute, serially; no unbounded result set added
    findings: []
  - area: api
    applies: 'yes'
    checked: no new endpoint; the hook runs before authentication on every agent request, and what it costs is the embedder's code
    findings: []
  - area: frontend
    applies: 'no'
    checked: no UI files in the diff
    findings: []
  - area: caching
    applies: 'yes'
    checked: client info, tool catalog and upstream session caches are keyed by tenant; no new cache
    findings: []
  - area: distributed-systems
    applies: 'no'
    checked: no event, stream or cross-service workflow changed
    findings: []
sweeps:
  - {id: S1, hits: 0, checked: 0, findings: []}
  - {id: S2, hits: 0, checked: 0, findings: []}
  - {id: S3, hits: 0, checked: 0, findings: []}
  - {id: S4, hits: 0, checked: 0, findings: []}
  - {id: S5, hits: 0, checked: 0, findings: []}
  - {id: S6, hits: 0, checked: 0, findings: []}
  - {id: S7, hits: 0, checked: 0, findings: []}
  - {id: S8, hits: 0, checked: 0, findings: []}
  - {id: S9, hits: 0, checked: 0, findings: []}
  - {id: S10, hits: 0, checked: 0, findings: []}
  - {id: S11, hits: 0, checked: 0, findings: []}
  - {id: S12, hits: 0, checked: 0, findings: []}
  - {id: S13, hits: 0, checked: 0, findings: []}
  - {id: S14, hits: 0, checked: 0, findings: []}
  - {id: S15, hits: 0, checked: 0, findings: []}
  - {id: S16, hits: 0, checked: 0, findings: []}
  - {id: S17, hits: 0, checked: 0, findings: []}
sweeps_note: >-
  sweeps.md in this checkout is written for Agent Authority's layout
  (internal/record, internal/store/migrations, connect/, web/). The greps that
  apply to any Go diff (S2, S6, S7, S11, S12, S13, S16) ran over this diff with
  no hits; the rest name paths this repository does not have. This repository
  has no AGENTS.md, CLAUDE.md or CONTRIBUTING.md at the head.
accepted_risks:
  - checked: every agent route is placed before authentication
    why: both route sets in Handler.Routes wrap /mcp/, /api/v1/invocations, /api/v1/external/* and /api/v1/agent/rules in agentRuntimeHandler, whose tenant step runs outside the auth middleware
  - checked: a held call keeps its tenant end to end with the strict check on
    why: >-
      probe on Postgres 18 with store.RequireTenant(): a call by tenant acme to
      its own stored server was held, ResolvePending with no tenant returned
      ErrNoTenant, ResolvePending as acme in the embedder's transaction woke the
      holder through LISTEN, the upstream was called once, the result was
      stored, and nothing logged a lost tenant
  - checked: reading the issuer from the unverified token
    why: Validator.verify matches the config by the token's iss and aud, then verifies the signature with that issuer's keys (internal/auth/validator.go:260-322)
  - checked: no IdP, or auth debug skip_verify
    why: the middleware then verifies only agent keys, or nothing; the option doc says to place requests by keys without an IdP, and skip_verify already disables authentication
  - checked: panchicore:F1 (the strict check also binds the embedder's own queries and WithRoutes handlers)
    why: still open at this head, no address post; not re-reported here
  - checked: Managed Agents accounts stay process-wide
    why: declared non-goal
verification:
  - run: ATRYUM_POSTGRES_TESTS=1 ATRYUM_POSTGRES_TEST_DSN=<throwaway Postgres 18> go test -count=1 ./...
    result: ok in all 12 packages; the six new tests and every TestPostgres* test PASS, none skipped (checked with -v)
  - run: gofmt -l cmd internal pkg; go vet ./...
    result: clean
  - run: drop the tenant prefix in sessionKeyFromRequest, then TestClientInfoIsPerTenant
    result: "FAIL (bank found acme's client info)"
  - run: gh pr checks 193
    result: Go, Baseline and CLA pass; live harnesses skipped
  - claim: 'PR body: each new test fails with its change reverted'
    result: checked for TestClientInfoIsPerTenant here and for two more by panchicore; the other three NOT CHECKED

kam-validmind

Merge with recommendations · 0 blocking · 2 small fixes · no questions for you

A second, independent pass that looked only for new problems. The two items from the earlier reviews still stand and aren't repeated here.

Small fixes:

kam-validmind:F2: a broken tenant hook fails silently. If the embedding program's hook panics or errors, every agent call gets 401 unknown credential and nothing is logged outside debug mode. A probe confirmed it: a panic gave a 401 and an empty log. Before the last commit, Go's server logged the panic with its stack.
kam-validmind:F3: the docs list what fails under WithRequestTenant but leave out /agent_ids, /invocations/ and /models/, which now return 500.

Ready for your approval, @kam-validmind.

_{calibrated-review v3 · review · run by @kam-validmind · head 15f2545}

Full review: what changed, how it works, what we checked

What this PR does

Atryum used to run every agent request as the default tenant (a tenant is one customer org whose rows Postgres keeps apart). This PR lets the embedding program, Agent Authority, name the tenant for each request before Atryum authenticates it. It also makes a database query that names no tenant fail, instead of quietly acting for default. Nothing changes for a program that sets neither new option.

How it works

flowchart LR
  req["Agent request"] --> hook["Request-tenant hook"]
  hook -->|"tenant named"| auth["Auth middleware, key lookup as that tenant"]
  hook -->|"error, panic or empty"| deny["401 unknown credential"]
  auth --> route["Agent route, decider, upstream call"]
  route --> db["Postgres, row-level security per tenant"]

What to notice: the hook runs first, so even the agent-key lookup runs inside the right tenant. The refusal branch on the right is where both findings sit.

Findings

🟡 kam-validmind:F2: a broken tenant hook leaves no trace in the logs

What goes wrong: the embedding program's hook has a bug, such as a write to a nil map, or its own tenant lookup fails. Every agent request gets 401 unknown credential. The log stays empty unless ATRYUM_MCP_DEBUG is set. The operator sees agents "failing to authenticate" and nothing pointing at the hook.
Why it matters: this is an outage of every agent call that looks like bad credentials. The auth middleware next to it logs every 401 it answers. Before the last commit (15f2545), Go's HTTP server recovered the panic itself and logged it with its stack. The new recover drops that stack.
Where: internal/api/handlers.go:1316 (logs only when debug is on), internal/api/handlers.go:1334 (recovers the panic), internal/auth/middleware.go:259 (the middleware's log, which has no debug check).

Fix: log the panic with debug.Stack() and the hook error as an [auth] line, whatever the debug setting. Keep the 401.

defer func() {
    if p := recover(); p != nil {
        log.Printf("[auth] request tenant hook panicked: %v\n%s", p, debug.Stack())
        id, err = "", fmt.Errorf("request tenant panicked: %v", p)
    }
}()

How we know: I ran a probe test at the head and removed it afterwards.

Hook does	Response	Log
panics (nil map write)	`401 unknown credential`	empty
returns an error	`401 unknown credential`	empty
same panic in a plain Go HTTP handler	connection closed	`http: panic serving …: assignment to entry in nil map`

🟡 kam-validmind:F3: the docs leave three API-key routes out of the list of what fails

What goes wrong: with WithRequestTenant set and the full route set mounted, /agent_ids, /invocations/{agent_id} and /models/{cuid} query with no tenant. They answer 500 with the "context names no tenant" error. The CHANGELOG and the option's doc name only the operator API, the UI and the OAuth connect flow.
Why it matters: an embedder that relies on those routes finds out in production. Agent Authority isn't affected: it uses WithDecider, which doesn't mount them.
Where: internal/api/handlers.go:1253-1255, internal/api/handlers.go:2059, pkg/atryum/options.go:307.
Fix: name the API-key routes in both places, or say "every route outside the agent routes".
How we know: I read the route table, the handler, and the strict check in internal/store/tenant.go:70-74.

What we checked and found safe

What we checked	Why it is safe here
Every agent route places the tenant before authentication	Both route sets wrap `/mcp/`, `/api/v1/invocations`, `/api/v1/external/*` and `/api/v1/agent/rules` in the hook.
Background work keeps its tenant	The only request-path goroutine that touches the database inherits the request context through `context.WithoutCancel`, which keeps the tenant. Managed Agents watchers carry their tenant.
No in-memory state crosses tenants	Client info, the tool catalog, upstream MCP sessions and watchers all key on the tenant first. The agent-key resolver has no cache.
Cache keys built from client headers can't collide across tenants	Go's HTTP server rejects control characters in headers, so the NUL separator can't be forged.
The tenant can't inject SQL	It reaches Postgres only as a bind parameter to `set_config`.
The strict check covers every database path	Statements, prepared statements and `BEGIN` all check the tenant first, and switching tenants inside a transaction is refused.
A hook error maps to `401 invalid_token`	The auth middleware already does the same when the key store errors, so this isn't new behavior.
Discovery runs tenants one after another	A slow Anthropic call stalls every tenant, but the accounts are shared by all tenants anyway. Per-tenant accounts are out of scope.
Rollout	Both options are off by default. With neither set, behavior is unchanged. SQLite is refused.

Terms used here

Tenant: one customer org. Postgres row-level security shows each tenant only its own rows.
Hook: the function the embedding program passes to WithRequestTenant. It names a request's tenant.
Strict check: with the option set, a query whose context names no tenant fails with ErrNoTenant.
Panic / recover: a Go runtime crash inside a function, and the code that catches it.

Agent handoff · calibrated-review v3 · review · 15f2545

calibrated-review: 3
mode: review
pr: validmind/atryum#193
head: 15f2545f2f282d6288606f52b2a46030f1936174
merge_base: c4461dfe5818cfb4d3fc82a27a1fc3f72213ca88
run_by: kam-validmind
verdict: merge-with-recs
scope_note: >-
  Second, independent pass at the same head, asked for new problems only.
  panchicore:F1 and kam-validmind:F1 still stand and are not repeated here.
  IDs continue kam-validmind's sequence.
downgraded: 4
downgraded_notes:
  - >-
    Dropped: a hook error answers 401 invalid_token, so an MCP client may
    discard a good token during a transient outage of the embedder's tenant
    lookup. The auth middleware already does the same when the agent key
    store errors (internal/auth/middleware.go:75-78), so this is not new
    behavior.
  - >-
    Dropped: tenant and session are joined with a NUL byte in the client
    info, watcher and upstream session keys, so a crafted pair could collide.
    Not reachable: net/http refuses control characters in header values, and
    the embedder chooses the tenant IDs.
  - >-
    Dropped: discovery visits tenants one after another, and the Anthropic
    HTTP client has no timeout, so a hung call stalls discovery for every
    tenant. The accounts are process-wide, so a hang hits every tenant anyway,
    with or without this PR (per-tenant accounts are a declared non-goal).
  - >-
    Dropped: --init-servers tests only the default tenant's servers at
    startup. It is an opt-in status check, and nothing gates a call on it.
items:
  - id: kam-validmind:F2
    class: fix-now
    type: hygiene
    direction: safe
    title: A request-tenant hook that panics or errors is never logged, so a broken hook looks like bad credentials
    scenario: >-
      An embedder's RequestTenant hook has a bug (a nil map write, say) or its
      tenant lookup fails. Every agent request on /mcp/ and /api/v1/* gets
      401 unknown credential, and nothing reaches the log unless
      ATRYUM_MCP_DEBUG is set. Before 15f2545, net/http recovered the panic and
      logged it with its stack. The auth middleware logs every failure it
      answers with logAuthFailure, with no debug gate. Fails closed, so not
      blocking, but operators have no signal for an outage of every agent.
    evidence:
      - internal/api/handlers.go:1316-1318 logs the hook error only when h.debug
      - internal/api/handlers.go:1014 h.debug is set only by ATRYUM_MCP_DEBUG
      - internal/api/handlers.go:1334-1342 placeRequest recovers the panic and drops the stack
      - internal/auth/middleware.go:259-265 logAuthFailure logs every 401, with no debug gate
      - 'probe at head, hook panics (nil map write): status=401, log=""'
      - 'probe at head, hook returns an error: status=401, log=""'
      - 'probe, same panic in a plain net/http handler: log="http: panic serving ...: assignment to entry in nil map"'
    repro: >-
      In package api: log.SetOutput(&buf); h := &Handler{keyResolver: keyTenants{}};
      h.SetRequestTenant(func(*http.Request) (string, error) { var m map[string]string; m["x"] = "y"; return "", nil });
      serve POST /mcp/bank with Bearer atr_valid through h.agentRuntimeHandler; buf stays empty.
    fix: >-
      In placeRequest, log the panic with debug.Stack() whatever the debug
      setting. In withRequestTenant, log a hook error through the same
      [auth] line as logAuthFailure (method, path, remote, the error), not
      behind h.debug. Keep the 401. Extend TestRequestTenantScopesAgentRoutes
      to assert the panic case logs.
    done_when: a panicking or failing hook writes one log line with the cause, with ATRYUM_MCP_DEBUG unset, and a test checks it
  - id: kam-validmind:F3
    class: fix-now
    type: hygiene
    direction: safe
    title: The docs list what fails under WithRequestTenant but leave out the API-key routes /agent_ids, /invocations/ and /models/
    scenario: >-
      An embedder runs Atryum's full route set (not WithDecider) with
      WithRequestTenant. /agent_ids, /invocations/{agent_id} and /models/{cuid}
      authenticate with the static API key, are not placed by the hook, and
      query with r.Context(), which names no tenant. Each now answers 500
      with the text of store.ErrNoTenant. The CHANGELOG and the
      WithRequestTenant doc name only the operator API, UI and OAuth connect
      flow as failing. Agent Authority uses WithDecider, which does not mount
      these routes, so it is unaffected.
    evidence:
      - internal/api/handlers.go:1253-1255 mounts the three routes behind apiKeyMW only, in full mode
      - internal/api/handlers.go:2059-2061 agentIDs queries with r.Context() and returns 500 with err.Error()
      - internal/store/tenant.go:70-74 returns ErrNoTenant for a context with no tenant once RequireTenant is on
      - pkg/atryum/options.go:307-309 and the CHANGELOG Added entry list only the operator API, UI and OAuth connect flow
    fix: >-
      Add the API-key routes to the list in the WithRequestTenant doc and the
      CHANGELOG entry (or say "every route outside the agent routes").
    done_when: both docs name the API-key routes, or say that every route outside the agent routes fails
sweeps:
  - {id: S2, hits: 0, checked: 0, findings: []}
  - {id: S6, hits: 0, checked: 0, findings: []}
  - {id: S7, hits: 1, checked: 1, findings: []}
  - {id: S11, hits: 0, checked: 0, findings: []}
  - {id: S12, hits: 0, checked: 0, findings: []}
  - {id: S13, hits: 0, checked: 0, findings: []}
sweeps_note: >-
  sweeps.md is written for Agent Authority's layout. Only the sweeps that
  apply to any Go diff ran (S2's db.Exec grep, S6, S7, S11, S12, S13). S7's
  hit is the debug log of the hook error, which carries the embedder's error
  text, not a token. S1, S3-S5, S8-S10 and S14-S17 name paths this
  repository does not have.
arch:
  - topic: Concurrency
    applies: 'yes'
    checked: >-
      requireTenant is an atomic set once before serving; watchers map under
      s.mu, keyed by tenant and session; Start resumes every tenant before
      discovery starts; per-tenant discovery is serial
    findings: []
    safe_because: >-
      no shared state is written outside a lock, and a slow tenant only delays
      discovery, which hits every tenant anyway on the shared accounts
  - topic: Tenant isolation and access
    applies: 'yes'
    checked: >-
      every agent route in both route sets; every tenant.With, From and Lookup
      call; in-memory maps in api, mcp, invocation and managedagents; goroutines
      and context.Background in internal and pkg; key resolver caching
    findings: []
    safe_because: >-
      all agent routes are placed before authentication; goroutines that do DB
      work inherit the request or watcher context (context.WithoutCancel keeps
      the tenant); the key resolver has no cache; in-memory keys lead with the
      tenant
  - topic: Security
    applies: 'yes'
    checked: >-
      the tenant reaches Postgres only through set_config with a bind
      parameter; the new 401 path; header-controlled cache keys
    findings: []
    safe_because: no string-built SQL, and the hook's error text is logged only in debug mode
  - topic: API
    applies: 'yes'
    checked: >-
      status codes on the new refusal path; the WWW-Authenticate challenge;
      routes that newly fail under the option
    findings: ['kam-validmind:F3']
  - topic: Database
    applies: 'yes'
    checked: >-
      tenantConn.use under the strict check, inside and outside a
      transaction; the LISTEN connection; prepared statements
    findings: []
    safe_because: >-
      every statement path calls use() before running, a tenant change inside
      a transaction is refused, and LISTEN runs on its own pgx connection
      that reads no tenant rows
  - topic: Messaging and integrations
    applies: 'yes'
    checked: >-
      Managed Agents discovery and resume per tenant; the decision NOTIFY;
      Anthropic API calls
    findings: []
    safe_because: >-
      each watcher's context carries its tenant, and a woken waiter re-reads
      under its own tenant
  - topic: Performance and scale
    applies: 'yes'
    checked: work added to every agent request; discovery cost per tenant
    findings: []
    safe_because: >-
      the hook adds one function call per request; discovery adds two queries
      per tenant per minute
  - topic: Configuration and rollout
    applies: 'yes'
    checked: >-
      both options default off; behavior with neither set; SQLite refusal;
      an embedder that bumps the tag without setting them
    findings: []
    safe_because: >-
      with neither option set, tenant.From still falls back to default and
      requireTenant stays off, so nothing changes
  - topic: Frontend
    applies: 'no'
  - topic: Errors and observability
    applies: 'yes'
    checked: the hook refusal path; Managed Agents start and discovery errors
    findings: ['kam-validmind:F2']
verification:
  - run: go test -count=1 ./internal/api ./internal/managedagents ./internal/mcp ./internal/store ./pkg/...
    result: ok; the TestPostgres* tests skipped (no ATRYUM_POSTGRES_TESTS here). None of the findings depend on them, and both earlier reviews ran them on Postgres 18
  - run: probe test in internal/api, removed after the run
    result: 'hook panic -> 401 and an empty log; hook error -> 401 and an empty log; net/http logs the same panic with its stack'
  - claim: PR body says a panicking hook answers 401 like a request it can't place
    result: true, and the panic is no longer logged (kam-validmind:F2)

kam-validmind

Merge with recommendations · 0 blocking · 1 fix-later · no questions for you

Third pass, new problems only; earlier items still stand.

Fix later: kam-validmind:F4, one tenant can erase another tenant's MCP client info. The cache now keys by tenant, but it still holds 1,024 entries for the whole process. An agent that sends 1,024 initialize calls with different User-Agents, or enough tenants' agents together, pushes out other tenants' entries. Their tool calls and plans are then stored with no client name or version. A probe confirmed it: one bank entry, then 1,024 acme entries, and bank's was gone. It loses audit detail, nothing leaks, and no decision reads it.

Ready for your approval, @kam-validmind.

_{calibrated-review v3 · review · run by @kam-validmind · head 15f2545}

Full review: what changed, how it works, what we checked

Findings

🔵 kam-validmind:F4: one shared cap lets one tenant evict another tenant's client info

What goes wrong: Atryum remembers each MCP client's name and version from initialize, so later tool calls can record them. Atryum never assigns an Mcp-Session-Id, so the cache key is the tenant plus the caller's address and User-Agent. The cache holds 1,024 entries for every tenant together and evicts the oldest when full. An agent in tenant acme that varies its User-Agent, or ordinary load across many tenants, pushes out tenant bank's entry.
Why it matters: bank's later calls and plans are stored with no client name or version, so the record loses which harness made them. Nothing leaks, and no decision reads the field. The comment on the cap says it is sized for "a dev / single-tenant instance", and this PR is what makes one Atryum serve many tenants.
Where: internal/api/handlers.go:5247-5250 (the cap), internal/api/handlers.go:5292-5300 (the key), internal/api/handlers.go:1910 (where tool calls read it).

Fix: cap each tenant on its own, and evict only within the tenant whose insert overflowed.

byTenant := h.clientInfoCache[tid] // map[string]map[string]clientInfoSnapshot
if byTenant == nil { byTenant = map[string]clientInfoSnapshot{}; h.clientInfoCache[tid] = byTenant }
byTenant[key] = snap
if len(byTenant) > 1024 { evictOldest(byTenant) }

How we know: a probe test at the head recorded one bank entry, then 1,024 acme entries. Afterwards lookupClientInfo for bank returned nothing, and the cache held 1,024 entries.

What we checked and found safe

What we checked	Why it is safe here
Row-level security on every tenant table	Migration 034 puts the tenant policy on all 17 tables, and every upsert's conflict key starts with `tenant_id`.
Approving a held call in the embedding program's transaction	The connection refuses to switch tenants mid-transaction, so the approval can't act for another tenant.
Client-chosen IDs	Invocation and plan IDs are generated by the server.
Work that leaves the request goroutine	The summarizer copies the request context with `context.WithoutCancel`, which keeps the tenant. The upstream credential callback gets the request context.
Managed Agents bindings	The bindings adapter passes each tenant's context through, so discovery reads only that tenant's bindings.

Terms used here

Client info: the name and version an MCP client reports when it connects, such as "Claude Code 2.1".
Evict: drop an entry from a full cache to make room for a new one.
Tenant: one customer org. Postgres row-level security shows each tenant only its own rows.

Agent handoff · calibrated-review v3 · review · 15f2545

calibrated-review: 3
mode: review
pr: validmind/atryum#193
head: 15f2545f2f282d6288606f52b2a46030f1936174
merge_base: c4461dfe5818cfb4d3fc82a27a1fc3f72213ca88
run_by: kam-validmind
verdict: merge-with-recs
scope_note: >-
  Third pass at the same head, new problems only. Adds kam-validmind:F4.
  panchicore:F1 and kam-validmind:F1-F3 still stand and are not repeated.
downgraded: 3
downgraded_notes:
  - >-
    Dropped: within one tenant, two agents behind one address and
    User-Agent still share a client info entry. Before this PR, and not
    cross-tenant.
  - >-
    Dropped: agent routes answer no CORS preflight. No CORS handling exists
    anywhere, before or after this PR.
  - >-
    Dropped: the summarizer goroutine copies the request context with
    context.WithoutCancel. That keeps the tenant, so nothing is lost.
items:
  - id: kam-validmind:F4
    class: fix-later
    type: correctness
    direction: safe
    title: The client info cache is capped at 1024 entries for the whole process, so one tenant's initializes evict another tenant's
    scenario: >-
      Atryum never sets Mcp-Session-Id, so clients send none and the cache key
      is tenant + remote address + User-Agent. An agent in tenant acme sends
      1024 initialize requests with different User-Agent values, or the
      fleets of many tenants together pass 1024 distinct clients. Each insert
      past the cap evicts the oldest entry of any tenant. Tenant bank's next
      tools/call and plan submit find no entry, so the invocation and plan
      are stored with no client_name or client_version. That is lost audit
      metadata, not a leak, and no decision reads it.
    evidence:
      - internal/api/handlers.go:5247-5250 one cap of 1024 for every tenant, commented "plenty for a dev / single-tenant instance"
      - internal/api/handlers.go:5292-5300 with no Mcp-Session-Id the key is remote address + User-Agent, both chosen by the client
      - internal/api/handlers.go:1818 initialize sets no Mcp-Session-Id on the response
      - internal/api/handlers.go:1910 and :1991 tools/call and plan submit take ClientName from the cache
      - 'probe at head: 1 bank entry, then 1024 acme entries -> bank entry gone, cache size 1024'
    repro: >-
      In package api: h := &Handler{clientInfoCache: map[string]clientInfoSnapshot{}};
      recordInitializeClientInfo for tenant bank, then 1024 times for tenant acme
      with distinct Mcp-Session-Id or User-Agent; lookupClientInfo for bank returns false.
    fix: >-
      Cap per tenant: map[tenant]map[key]clientInfoSnapshot with 1024 entries
      each, evicting only within the tenant whose insert overflowed. Add a test
      that fills one tenant and checks another's entry survives.
    done_when: filling one tenant's cache never evicts another tenant's entry, and a test checks it
sweeps:
  - {id: S2, hits: 0, checked: 0, findings: []}
  - {id: S6, hits: 0, checked: 0, findings: []}
  - {id: S7, hits: 1, checked: 1, findings: []}
  - {id: S11, hits: 0, checked: 0, findings: []}
  - {id: S12, hits: 0, checked: 0, findings: []}
  - {id: S13, hits: 0, checked: 0, findings: []}
sweeps_note: same sweeps as the previous pass at this head; the diff has not changed
arch:
  - topic: Concurrency
    applies: 'yes'
    checked: watcher map, client info cache lock, discovery loop
    findings: []
    safe_because: shared maps are written under their mutex, and keys lead with the tenant
  - topic: Tenant isolation and access
    applies: 'yes'
    checked: >-
      migration 034 policies and the ON CONFLICT keys; ResolvePending in the
      embedder's transaction; the upstream credential callback; the
      summarizer goroutine; shared in-memory caps
    findings: ['kam-validmind:F4']
  - topic: Security
    applies: 'yes'
    checked: invocation and plan IDs are server-generated; the tenant reaches SQL only as a bind parameter
    findings: []
    safe_because: no client-chosen ID crosses tenants, and no SQL is built from the tenant
  - topic: API
    applies: 'yes'
    checked: no new endpoint in this pass
    findings: []
    safe_because: covered in the previous pass at this head (kam-validmind:F3)
  - topic: Database
    applies: 'yes'
    checked: every tenant table has the tenant policy; every upsert conflict key leads with tenant_id
    findings: []
    safe_because: >-
      a tenant switch inside a transaction is refused, so ResolvePending in the
      embedder's transaction cannot act for another tenant
  - topic: Messaging and integrations
    applies: 'yes'
    checked: Managed Agents bindings adapter and watcher context
    findings: []
    safe_because: the adapter passes the per-tenant context through, and each watcher keeps its tenant
  - topic: Performance and scale
    applies: 'yes'
    checked: client info eviction scans the map under the lock
    findings: ['kam-validmind:F4']
  - topic: Configuration and rollout
    applies: 'no'
  - topic: Frontend
    applies: 'no'
  - topic: Errors and observability
    applies: 'yes'
    checked: no change since the previous pass
    findings: []
    safe_because: covered in the previous pass at this head (kam-validmind:F2)
verification:
  - run: probe test in internal/api, removed after the run
    result: 'bank entry present after 1024 acme initializes: false (cache size 1024)'

kam-validmind · 2026-11-10T16:45:38Z

Review summary · approved · 0 blocking · 4 small fixes · 1 for later

Three passes at 15f2545, nothing blocking. Every agent route is placed before authentication, and the Postgres tests pass on a real database.

Small fixes, best in this PR:

kam-validmind:F1: refuse to start when the database login owns the tables, is a superuser or can bypass row-level security. Otherwise tenants can read each other's rows.
kam-validmind:F2: log a hook that panics or errors. Today every agent call gets a 401 and nothing is logged.
panchicore:F1 and kam-validmind:F3: say what the strict check breaks, including the embedding program's own queries and /agent_ids, /invocations/, /models/.

Later: kam-validmind:F4, cap the client-info cache per tenant.

_{calibrated-review v3 · summary · run by @kam-validmind · head 15f2545}

Agent handoff · calibrated-review v3 · summary · 15f2545

calibrated-review: 3
mode: summary
pr: validmind/atryum#193
head: 15f2545f2f282d6288606f52b2a46030f1936174
run_by: kam-validmind
verdict: merge-with-recs
blocking: []
items:
  - id: 'kam-validmind:F1'
    class: fix-now
    title: refuse WithRequestTenant when the login owns the tables, is a superuser or has BYPASSRLS
    source: pullrequestreview-5843788020
  - id: 'kam-validmind:F2'
    class: fix-now
    title: log a request-tenant hook panic, with its stack, and a hook error, without the debug gate
    source: pullrequestreview-5845637722
  - id: 'panchicore:F1'
    class: fix-now
    title: WithRequestTenant doc says the strict check binds the embedder's own queries and WithRoutes handlers
    source: pullrequestreview-5840357468
  - id: 'kam-validmind:F3'
    class: fix-now
    title: docs name /agent_ids, /invocations/ and /models/ among the routes that fail under the option
    source: pullrequestreview-5845637722
  - id: 'kam-validmind:F4'
    class: fix-later
    title: cap the client info cache per tenant
    ticket: owed
    source: pullrequestreview-5845716993

…ode breaks - With WithRequestTenant, Main refuses to start as a superuser, a BYPASSRLS role or an owner of the tables, or a member of one: row-level security binds none of them (kam-validmind:F1). - A request-tenant hook that errors or panics is logged, with the panic's stack; one that returns "" for an unknown credential isn't (kam-validmind:F2). - The docs say the missing-tenant error binds the embedding program's own queries and the routes no request tenant reaches, /agent_ids, /invocations/ and /models/ included (panchicore:F1, kam-validmind:F3). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

cachafla · 2026-11-10T19:07:45Z

Thanks @panchicore and @kam-validmind. All four small fixes are in a6c1453.

kam-validmind:F1 fixed. With WithRequestTenant set, Main refuses to start as a superuser, a BYPASSRLS role or an owner of the tables, or as a member of any of these (NOINHERIT included, since that login can still SET ROLE). It's the same check AA's vmaa serve makes. TestPostgresCheckRowSecurityBinds refuses the test superuser and accepts the tenant login.
kam-validmind:F2 fixed. A hook that errors or panics is logged with the method and path, and a panic with its stack. A hook that returns "" for an unknown credential isn't logged, because any caller can send one. TestRequestTenantScopesAgentRoutes checks both; its error and panic cases fail against the previous handler.
panchicore:F1 and kam-validmind:F3 fixed in WithRequestTenant's doc, the CHANGELOG and docs/architecture.md. The missing-tenant error binds the embedding program's own queries (WithDatabase hooks, WithRoutes and WithAuthenticatedRoutes handlers, its startup and background work). It also binds the routes no request tenant reaches: the operator API, UI, OAuth connect flow, and /agent_ids, /invocations/ and /models/, which answer 500. AA names its tenant on all of these when it adopts the option in its PR 3.
kam-validmind:F4 (one process-wide cap on the client-info cache) is deferred, as you suggested. It loses audit detail but leaks nothing, and it becomes reachable only once AA serves a second org.

The full suite passes, with the Postgres tests run against a local Postgres.

🤖 Generated with Claude Code

kam-validmind

Merge with recommendations · 0 blocking · 1 small fix · 1 deferred · no questions for you

All four fixes in a6c1453 hold. Atryum now refuses a database login that row-level security doesn't bind: a Postgres 18 probe refused the table owner, its members and BYPASSRLS, and accepted a plain login. A failing hook is logged; the new test fails on the old handler.

Small fix: kam-validmind:F5, the login test covers a superuser but not a table owner.

Deferred, accepted: kam-validmind:F4 (ticket owed).

Ready for your approval, @kam-validmind.

_{calibrated-review v3 · recheck · run by @kam-validmind · head a6c1453}

Full review: what changed, how it works, what we checked

What this commit does

a6c1453 answers the review. With WithRequestTenant set, Atryum checks its database login at startup and refuses one that Postgres row-level security (the per-tenant row filter) does not bind. A request-tenant hook that errors or panics is now logged. The docs say everything the missing-tenant error breaks.

How it works

flowchart LR
  start["Main starts with WithRequestTenant"] --> mig["Run migrations"]
  mig --> chk["Check the database login"]
  chk -->|"superuser, BYPASSRLS, owner or member of one"| stop["Refuse to start"]
  chk -->|"plain login"| strict["Turn on the missing-tenant error"]
  strict --> serve["Serve agent requests"]

What to notice: the check runs before the strict mode and before any request is served, so a misconfigured login never serves a tenant.

Items answered

Item	Answer	How it was verified
kam-validmind:F1	fixed	Probe on Postgres 18: owner, member of owner, `NOINHERIT` member and `BYPASSRLS` refused; plain login accepted. `Main` calls the check before enabling strict mode.
kam-validmind:F2	fixed	With the old handler restored, the extended test fails on the error and panic cases; at the head it passes. No debug gate, and a panic logs its stack.
panchicore:F1	fixed	The option doc and CHANGELOG name the embedding program's hooks, routes, startup and background work.
kam-validmind:F3	fixed	The option doc and CHANGELOG name `/agent_ids`, `/invocations/` and `/models/` and their `500`.
kam-validmind:F4	deferred	Accepted: it loses audit detail and leaks nothing. No ticket yet.

Findings

🟡 kam-validmind:F5: the login test doesn't cover the table owner

What goes wrong: TestPostgresCheckRowSecurityBinds refuses the test's admin login, which is a superuser. The clause that refuses a non-superuser table owner, the case F1 was about, is never exercised. Someone can delete it and every test stays green.
Why it matters: the owner case is the likely misconfiguration (one database URL on a fresh database). The code is right today; only the guard against a later regression is missing.
Where: internal/store/tenant.go:72-74, internal/store/tenant_postgres_test.go:178-195.
Fix: in the test, create a non-superuser role that owns the test database's tables, open a pool as it, and assert the check refuses it.
How we know: I read the test, and a probe at the head showed the clause working: owner, member and NOINHERIT member refused, plain login accepted.

What we checked and found safe

What we checked	Why it is safe here
The login check can't lock out a correct setup	A non-owner login passes the migration step on an up-to-date schema, and `atryum.Migrate` runs migrations as the owner in a separate step.
The check's SQL	It takes no input and reads only system catalogs.
The new log line	It prints the method, path and the hook's error, never a credential. The auth middleware already logs the path the same way.
Log volume	A line is written only when the hook errors, not for an unknown credential, which any caller can send.
Rollout	Without the option nothing changes. With it, a bad login fails at startup with a message that names the login and the fix.
The whole suite	Every package passes with the Postgres tests running on Postgres 18, none skipped.

Terms used here

Row-level security: a Postgres feature that shows a connection only the rows its tenant setting allows. It does not apply to a superuser, a BYPASSRLS role or a table's owner.
Hook: the function the embedding program passes to WithRequestTenant. It names a request's tenant.
Probe: a throwaway test written for this review and removed after the run.

Agent handoff · calibrated-review v3 · recheck · a6c1453

calibrated-review: 3
mode: recheck
pr: validmind/atryum#193
head: a6c1453db094e4d7f08c54e1ac4cfd5be503c0e3
previous_head: 15f2545f2f282d6288606f52b2a46030f1936174
merge_base: c4461dfe5818cfb4d3fc82a27a1fc3f72213ca88
run_by: kam-validmind
verdict: merge-with-recs
answers_checked: issuecomment-6776643176
downgraded: 2
downgraded_notes:
  - >-
    Dropped: the new log line prints r.URL.Path, which the caller chooses, so
    a path with an encoded newline can forge a log line when the hook errors.
    logAuthFailure in the auth middleware already logs the path the same way
    for every 401, so this is not new.
  - >-
    Dropped: the login check refuses the table owner, and Main migrates on
    the serving login. Checked that this does not make the option unusable:
    a non-owner login passes InitDBWithDialect on an up-to-date schema, and
    atryum.Migrate exists for a separate owner step.
items:
  - id: 'kam-validmind:F1'
    status: verified
    evidence: >-
      probe on Postgres 18 at a6c1453, CheckRowSecurityBinds per login: table
      owner (NOSUPERUSER NOBYPASSRLS) refused, member of the owner refused,
      NOINHERIT member refused, BYPASSRLS refused, plain login accepted;
      pkg/atryum/atryum.go:264 calls it before store.RequireTenant()
  - id: 'kam-validmind:F2'
    status: verified
    evidence: >-
      with internal/api/handlers.go from 15f2545, TestRequestTenantScopesAgentRoutes
      fails (atr_other logged "", atr_panic logged ""); at a6c1453 it passes.
      The log has no debug gate and a panic carries debug.Stack()
  - id: 'panchicore:F1'
    status: verified
    evidence: >-
      pkg/atryum/options.go WithRequestTenant doc and the CHANGELOG now say the
      missing-tenant error binds WithDatabase hooks, WithRoutes and
      WithAuthenticatedRoutes handlers and the program's own startup and
      background work
  - id: 'kam-validmind:F3'
    status: verified
    evidence: the option doc and the CHANGELOG name /agent_ids, /invocations/ and /models/ and their 500
  - id: 'kam-validmind:F4'
    status: accepted
    evidence: >-
      deferred by the author; safe direction (lost audit metadata, no leak)
      and fix-later in the review. No ticket yet (ticket owed)
  - id: 'kam-validmind:F5'
    class: fix-now
    type: hygiene
    direction: safe
    title: TestPostgresCheckRowSecurityBinds pins only the superuser branch, so the table-owner branch can be deleted with every test green
    scenario: >-
      The test refuses the admin login, a superuser, which the first EXISTS
      (rolsuper OR rolbypassrls) catches. The second EXISTS (pg_class.relowner
      membership) is what refuses a non-superuser owner, the case F1 was about
      (one DSN on a fresh database). Nothing exercises it: removing that clause
      keeps the suite green and reopens F1. The clause works today (probe).
    evidence:
      - internal/store/tenant.go:72-74 the two EXISTS clauses
      - internal/store/tenant_postgres_test.go:178-195 the test opens only the tenant login and the superuser admin
      - 'probe at a6c1453: owner refused=true, member refused=true, noinh refused=true, bypass refused=true, plain refused=false'
    repro: >-
      Delete the second EXISTS in CheckRowSecurityBinds and run
      ATRYUM_POSTGRES_TESTS=1 go test ./internal/store -run TestPostgresCheckRowSecurityBinds
    fix: >-
      In the test, create a NOSUPERUSER NOBYPASSRLS role, make it the owner of
      the test database before migrating (or ALTER TABLE ... OWNER TO), open a
      pool as it and assert CheckRowSecurityBinds refuses it.
    done_when: the test fails when the relowner clause is removed
sweeps:
  - {id: S2, hits: 0, checked: 0, findings: []}
  - {id: S6, hits: 0, checked: 0, findings: []}
  - {id: S7, hits: 1, checked: 1, findings: []}
  - {id: S11, hits: 1, checked: 1, findings: []}
  - {id: S12, hits: 0, checked: 0, findings: []}
  - {id: S13, hits: 0, checked: 0, findings: []}
sweeps_note: >-
  Run over the delta 15f2545..a6c1453 only. S7 is the new log.Printf of the
  hook error (method, path, the embedder's error text, no token). S11 is
  context.Background() in the startup login check, which runs before
  RequireTenant and reads only catalogs. The other sweeps name Agent
  Authority paths this repository does not have.
arch:
  - topic: Concurrency
    applies: 'no'
  - topic: Tenant isolation and access
    applies: 'yes'
    checked: the login check's SQL against five kinds of login; where Main calls it
    findings: []
    safe_because: >-
      every login row-level security does not bind, or that can SET ROLE to
      one, is refused before the strict check is turned on and before serving
  - topic: Security
    applies: 'yes'
    checked: what the new log line prints; the fixed SQL string
    findings: []
    safe_because: the query has no input, and the log carries method, path and the hook's error, no credential
  - topic: API
    applies: 'no'
  - topic: Database
    applies: 'yes'
    checked: the check runs on the serving pool after migrate; a non-owner login on an up-to-date schema
    findings: []
    safe_because: >-
      InitDBWithDialect as the non-owner login returns nil on a migrated
      database, and atryum.Migrate runs migrations as the owner in a separate step
  - topic: Messaging and integrations
    applies: 'no'
  - topic: Performance and scale
    applies: 'yes'
    checked: one catalog query at startup; one log line per failing hook call
    findings: []
    safe_because: the query runs once, and the log line is written only when the hook errors, not for an unknown credential
  - topic: Configuration and rollout
    applies: 'yes'
    checked: a deployment that sets WithRequestTenant with an owner or superuser login
    findings: []
    safe_because: it now fails at startup with a message naming the login and the fix, and nothing changes without the option
  - topic: Frontend
    applies: 'no'
  - topic: Errors and observability
    applies: 'yes'
    checked: hook error and panic logging; the startup refusal message
    findings: ['kam-validmind:F5']
verification:
  - run: ATRYUM_POSTGRES_TESTS=1 ATRYUM_POSTGRES_TEST_DSN=<throwaway Postgres 18> go test -count=1 ./...
    result: ok in every package; all seven TestPostgres* tests in internal/store PASS, none skipped (checked with -v)
  - run: gofmt -l cmd internal pkg; go vet ./internal/... ./pkg/...
    result: clean
  - run: gh pr checks 193
    result: Go, Baseline and CLA pass; live harnesses skipped
  - run: probe tests in internal/store and a revert of internal/api/handlers.go, all removed after the run
    result: see the items above

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

cachafla · 2026-11-10T19:15:47Z

@kam-validmind kam-validmind:F5 is fixed in 03f332b. TestPostgresCheckRowSecurityBinds now also connects as a login that's a member of a non-superuser role owning mcp_servers, and that login must be refused. With the ownership clause removed from the check, the case fails. kam-validmind:F4 stays deferred.

panchicore

Merge with recommendations · 0 blocking · panchicore:F1 verified · 2 small fixes

The strict-mode doc now says it binds the embedder's own queries and which routes answer 500, in the option's doc, the CHANGELOG and the architecture page. With WithRequestTenant, Main refuses an unbound login after migrating. The full suite passes with the Postgres tests on a fresh Postgres 18.

Small fixes, both in the new login-check test:

panchicore:F2: it still passes with the BYPASSRLS clause removed. The owner case is now covered.
panchicore:F3: it leaves its two roles in the cluster after every run, one of them a login with a known password. The cleanup uses a connection the test has already closed, so the drop fails silently. Four runs left eight roles.

Ready for your approval, @panchicore.

_{calibrated-review v2 · recheck · run by @panchicore · head 03f332b}

Agent handoff · calibrated-review v2 · recheck · 03f332b

calibrated-review: 2
mode: recheck
pr: validmind/atryum#193
head: 03f332bad1830faa005bd3623ca12f680b7d8113
previous_head: 15f2545f2f282d6288606f52b2a46030f1936174
run_by: panchicore
verdict: merge-with-recs
downgraded: 0
items:
  - id: panchicore:F1
    status: verified
    evidence: pkg/atryum/options.go WithRequestTenant doc, CHANGELOG.md and docs/architecture.md name WithDatabase hooks, WithRoutes/WithAuthenticatedRoutes handlers, startup/background work, and the operator API, UI, OAuth connect, /agent_ids, /invocations/, /models/ (500)
  - id: panchicore:F2
    class: fix-now
    type: hygiene
    direction: safe
    title: TestPostgresCheckRowSecurityBinds has no BYPASSRLS case
    scenario: >-
      CheckRowSecurityBinds refuses a superuser, a BYPASSRLS role or a table
      owner (directly or through membership). The test covers the tenant login,
      the superuser and, since 03f332b, a member of a table's owner. Dropping the
      BYPASSRLS clause still passes.
    evidence:
      - mutation on a throwaway Postgres 18 (ATRYUM_POSTGRES_TESTS=1) at 03f332b, "(r.rolsuper OR r.rolbypassrls)" -> "r.rolsuper": PASS
      - mutation, the pg_class relowner clause removed: FAIL "a member of a table's owner: <nil>, want refused" (covered now)
      - the query matches agent-authority's store.CheckServiceRole, whose test covers BYPASSRLS (AA#166)
    fix: add a LOGIN BYPASSRLS member of the tenant role that must be refused, named per run and dropped in cleanup
    done_when: the BYPASSRLS mutation fails the test
  - id: panchicore:F3
    class: fix-now
    type: hygiene
    direction: safe
    title: The owner-member case leaks its roles into the cluster on every run
    scenario: >-
      The test opens onDB with `defer onDB.Close()` and registers a t.Cleanup
      that uses onDB to reset mcp_servers' owner and DROP ROLE the two roles.
      Deferred calls run when the test function returns, before t.Cleanup, so
      the cleanup runs on a closed pool and its errors are ignored. Each run
      leaves <db>_owner and <db>_owner_member (LOGIN PASSWORD 'app').
    evidence:
      - internal/store/tenant_postgres_test.go at 03f332b, defer onDB.Close() then t.Cleanup(onDB.Exec(...)) with the errors dropped
      - after 4 runs of the store package on a fresh cluster, pg_roles has 8 atryum_tenant_<n>_owner / _owner_member roles
    fix: >-
      Close onDB inside the t.Cleanup after the drops (not with defer), and
      check the Exec errors with t.Error so a failed cleanup shows.
    done_when: running the store Postgres tests twice leaves no *_owner roles
verification:
  - run: ATRYUM_POSTGRES_TESTS=1 ATRYUM_POSTGRES_TEST_DSN=<fresh pg18> GOWORK=off go test ./... -count=1
    result: ok in all 12 packages at a6c1453; store package ok at 03f332b
  - run: gh pr checks 193
    result: Go, Baseline, CLA pass; live harnesses skipped (secrets)
  - claim: kam-validmind:F2 hook error/panic cases fail against the previous handler (author)
    result: NOT CHECKED both ways; passes at head

kam-validmind

Merge with recommendations · 0 blocking · 1 small fix · 1 deferred · no questions for you

kam-validmind:F5 is verified: the login test now covers a table owner, and it fails when the ownership check is removed. The new commit only changes that test.

Small fix: kam-validmind:F6, the new test case leaves two roles behind in the test Postgres on every run (4 after two runs). Test-only.

Deferred, accepted: kam-validmind:F4 (ticket owed).

Ready for your approval, @kam-validmind.

_{calibrated-review v3 · recheck · run by @kam-validmind · head 03f332b}

Full review: what changed, how it works, what we checked

What this commit does

03f332b adds one case to TestPostgresCheckRowSecurityBinds. It makes a non-superuser role the owner of one table, logs in as a member of that role, and expects the startup login check to refuse it. No production code changed.

Items answered

Item	Answer	How it was verified
kam-validmind:F5	fixed	The test passes on Postgres 18. With the table-owner clause removed from the check, it fails with "a member of a table's owner: nil, want refused".
kam-validmind:F4	deferred	Accepted, as before. No ticket yet.

Findings

🟡 kam-validmind:F6: the new test case leaks two database roles per run

What goes wrong: the case creates two roles and registers a cleanup to drop them. The cleanup uses a connection pool that a defer has already closed, because Go runs deferred calls before test cleanups. Both statements fail, the errors are ignored, and the roles stay in the Postgres cluster after the test database is dropped.
Why it matters: a developer's local Postgres collects two stray roles on every run. Nothing in production is affected, and CI doesn't run the Postgres tests.
Where: internal/store/tenant_postgres_test.go:210 (the defer), internal/store/tenant_postgres_test.go:212-215 (the cleanup).
Fix: close onDB in a t.Cleanup instead of a defer, and drop the two roles on the admin connection after the test database is dropped.
How we know: after two runs on a fresh Postgres 18, pg_roles held four atryum_tenant_*_owner and *_owner_member roles.

What we checked and found safe

What we checked	Why it is safe here
The new case really pins the owner clause	Removing the clause from the check makes the test fail.
The whole suite at the new head	Every package passes with the Postgres tests running, none skipped. Formatting and vet are clean, and CI is green.
Production behavior	Unchanged: the commit touches one test file.

Terms used here

Role: a Postgres account or group. A table's owner is a role, and row-level security does not apply to it.
Cleanup / defer: two ways a Go test runs code at the end. Deferred calls run first, when the function returns; cleanups run after.

Agent handoff · calibrated-review v3 · recheck · 03f332b

calibrated-review: 3
mode: recheck
pr: validmind/atryum#193
head: 03f332bad1830faa005bd3623ca12f680b7d8113
previous_head: a6c1453db094e4d7f08c54e1ac4cfd5be503c0e3
merge_base: c4461dfe5818cfb4d3fc82a27a1fc3f72213ca88
run_by: kam-validmind
verdict: merge-with-recs
answers_checked: the author's comment of 2026-11-10T19:15:47Z
delta: test only, internal/store/tenant_postgres_test.go (+39)
downgraded: 0
items:
  - id: 'kam-validmind:F5'
    status: verified
    evidence: >-
      at 03f332b TestPostgresCheckRowSecurityBinds passes on Postgres 18; with
      the pg_class.relowner clause removed from CheckRowSecurityBinds it fails
      ("a member of a table's owner: <nil>, want refused")
  - id: 'kam-validmind:F4'
    status: accepted
    evidence: still deferred by the author; safe direction; ticket owed
  - id: 'kam-validmind:F6'
    class: fix-now
    type: hygiene
    direction: safe
    title: The new test case leaves two roles behind in the test Postgres on every run
    scenario: >-
      The case creates <db>_owner and <db>_owner_member and registers a
      t.Cleanup that reassigns mcp_servers and drops them through onDB. But
      onDB is closed by `defer onDB.Close()` when the test function returns,
      which is before any t.Cleanup runs, so both statements fail and their
      errors are ignored. The database is then dropped by the helper's cleanup
      and the two roles stay in the cluster. Test-only: nothing in production
      is affected, and CI does not run the Postgres tests.
    evidence:
      - internal/store/tenant_postgres_test.go:210 defer onDB.Close()
      - internal/store/tenant_postgres_test.go:212-215 the cleanup uses onDB and discards the errors
      - 'after two runs on a fresh Postgres 18: pg_roles holds 4 roles named atryum_tenant_*_owner and *_owner_member'
    repro: >-
      ATRYUM_POSTGRES_TESTS=1 go test ./internal/store -run TestPostgresCheckRowSecurityBinds,
      then SELECT rolname FROM pg_roles WHERE rolname LIKE 'atryum_tenant_%'
    fix: >-
      Drop the roles through the admin pool after the database is gone: replace
      `defer onDB.Close()` with a t.Cleanup that closes onDB, and register a
      cleanup before openTenantTestDB's (so it runs after the DROP DATABASE)
      that runs DROP ROLE IF EXISTS for both roles on the admin connection.
      Dropping the database removes the ownership that blocks DROP ROLE.
    done_when: pg_roles has no atryum_tenant_* role after the test run
sweeps:
  - {id: S2, hits: 0, checked: 0, findings: []}
  - {id: S6, hits: 0, checked: 0, findings: []}
  - {id: S7, hits: 0, checked: 0, findings: []}
  - {id: S11, hits: 0, checked: 0, findings: []}
  - {id: S12, hits: 0, checked: 0, findings: []}
  - {id: S13, hits: 0, checked: 0, findings: []}
sweeps_note: the delta a6c1453..03f332b changes one test file, and the sweeps exclude test files
arch:
  - topic: Concurrency
    applies: 'no'
  - topic: Tenant isolation and access
    applies: 'yes'
    checked: the new case pins the owner-membership clause of the login check
    findings: []
    safe_because: removing the clause now fails the test
  - topic: Security
    applies: 'no'
  - topic: API
    applies: 'no'
  - topic: Database
    applies: 'yes'
    checked: what the test leaves in the test cluster
    findings: ['kam-validmind:F6']
  - topic: Messaging and integrations
    applies: 'no'
  - topic: Performance and scale
    applies: 'no'
  - topic: Configuration and rollout
    applies: 'no'
  - topic: Frontend
    applies: 'no'
  - topic: Errors and observability
    applies: 'yes'
    checked: errors discarded in the new cleanup
    findings: ['kam-validmind:F6']
verification:
  - run: ATRYUM_POSTGRES_TESTS=1 ATRYUM_POSTGRES_TEST_DSN=<throwaway Postgres 18> go test -count=1 ./...
    result: ok in every package; all seven TestPostgres* tests PASS, none skipped (checked with -v)
  - run: gofmt -l cmd internal pkg; go vet ./internal/store
    result: clean
  - run: gh pr checks 193
    result: Go, Baseline and CLA pass; live harnesses skipped

The test refuses a BYPASSRLS login too (panchicore:F2), gives its roles a random password, and drops them before its connection closes; the cleanup ran on a closed connection, so every run left two roles (panchicore:F3, kam-validmind:F6). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

cachafla · 2026-11-10T19:22:48Z

@panchicore @kam-validmind fixed in 7416a8c.

panchicore:F2 TestPostgresCheckRowSecurityBinds now also refuses a BYPASSRLS login that isn't a superuser. It fails with the BYPASSRLS clause removed from the check.
panchicore:F3 / kam-validmind:F6 the cleanup ran on a connection the test had already closed. It now runs before that connection closes, and it reports a failed drop. The roles also get a random password. A run leaves no roles behind now, and I dropped the ones my earlier runs had left in my local cluster.

kam-validmind:F4 stays deferred.

kam-validmind

Merge with recommendations · 0 blocking · 0 small fixes · 1 deferred · no questions for you

Both fixes in 7416a8c hold. The login test now leaves no roles behind, after a passing or a failing run (kam-validmind:F6, panchicore:F3). It also fails when the BYPASSRLS check is removed (panchicore:F2). The commit changes only that test.

Deferred, accepted: kam-validmind:F4, a per-tenant cap on the client-info cache (ticket owed).

Ready for your approval, @kam-validmind.

_{calibrated-review v3 · recheck · run by @kam-validmind · head 7416a8c}

Full review: what changed, how it works, what we checked

What this commit does

7416a8c changes one test, TestPostgresCheckRowSecurityBinds. It adds a login with BYPASSRLS (a Postgres attribute that skips the per-tenant row filter) and expects the startup check to refuse it. It also drops the roles the test creates before the test's connection closes. No production code changed.

Items answered

Item	Answer	How it was verified
kam-validmind:F6 / panchicore:F3	fixed	Two runs of the Postgres tests on a fresh Postgres 18 left no `atryum_tenant_*` role. A deliberately failing run left none either. The roles now get a random password.
panchicore:F2	fixed	With the `BYPASSRLS` clause removed from the check, the test fails with "a BYPASSRLS login: nil, want refused". At the head it passes.
kam-validmind:F4	deferred	Accepted, as before. No ticket yet.

What we checked and found safe

What we checked	Why it is safe here
The cleanup order	The drop is a deferred call registered after the connection's close, so Go runs it first, while the connection is still open.
A cleanup that fails	A failed `ALTER` or `DROP` now fails the test and prints the statement.
The login check is pinned on all three branches	Superuser, `BYPASSRLS` and table-owner membership each have a case that fails when its clause is removed.
The whole suite at the new head	Every package passes with the Postgres tests running, none skipped. Formatting and vet are clean, and CI is green.
Production behavior	Unchanged: the commit touches one test file.

Terms used here

Role: a Postgres account or group. Roles belong to the whole database server, not one database, so dropping a test database does not remove them.
Deferred call: code a Go function schedules to run when it returns. The last one scheduled runs first.

Agent handoff · calibrated-review v3 · recheck · 7416a8c

calibrated-review: 3
mode: recheck
pr: validmind/atryum#193
head: 7416a8ccb3035cb1a6ebc4e36169d9443365fe75
previous_head: 03f332bad1830faa005bd3623ca12f680b7d8113
merge_base: c4461dfe5818cfb4d3fc82a27a1fc3f72213ca88
run_by: kam-validmind
verdict: merge-with-recs
answers_checked: the author's comment of 2026-11-10T19:22:48Z
delta: test only, internal/store/tenant_postgres_test.go
downgraded: 0
items:
  - id: 'kam-validmind:F6'
    status: verified
    evidence: >-
      at 7416a8c, after two runs of the TestPostgres* tests on a fresh
      Postgres 18, pg_roles holds no atryum_tenant_* role; also none after a
      run where the test fails. The drop is now a defer registered after
      onDB.Close's defer, so it runs first, and a failed drop is a t.Errorf
  - id: 'panchicore:F3'
    status: verified
    evidence: the same defect as kam-validmind:F6; the roles now get a rand.Text() password
  - id: 'panchicore:F2'
    status: verified
    evidence: >-
      with rolbypassrls removed from CheckRowSecurityBinds the test fails
      ("a BYPASSRLS login: <nil>, want refused"); at 7416a8c it passes
  - id: 'kam-validmind:F4'
    status: accepted
    evidence: still deferred by the author; safe direction; ticket owed
sweeps:
  - {id: S2, hits: 0, checked: 0, findings: []}
  - {id: S6, hits: 0, checked: 0, findings: []}
  - {id: S7, hits: 0, checked: 0, findings: []}
  - {id: S11, hits: 0, checked: 0, findings: []}
  - {id: S12, hits: 0, checked: 0, findings: []}
  - {id: S13, hits: 0, checked: 0, findings: []}
sweeps_note: the delta 03f332b..7416a8c changes one test file, and the sweeps exclude test files
arch:
  - topic: Concurrency
    applies: 'no'
  - topic: Tenant isolation and access
    applies: 'yes'
    checked: the test now pins the superuser, BYPASSRLS and owner-membership clauses of the login check
    findings: []
    safe_because: removing the BYPASSRLS clause fails the test, as removing the owner clause did at 03f332b
  - topic: Security
    applies: 'yes'
    checked: the test's login roles and their passwords
    findings: []
    safe_because: the roles get a random password and are dropped in the same test, on pass and on failure
  - topic: API
    applies: 'no'
  - topic: Database
    applies: 'yes'
    checked: what the test leaves in the test cluster; the order of the deferred drop and the pool close
    findings: []
    safe_because: no role is left after a passing or a failing run
  - topic: Messaging and integrations
    applies: 'no'
  - topic: Performance and scale
    applies: 'no'
  - topic: Configuration and rollout
    applies: 'no'
  - topic: Frontend
    applies: 'no'
  - topic: Errors and observability
    applies: 'yes'
    checked: the cleanup's errors
    findings: []
    safe_because: a failed ALTER or DROP in the cleanup now fails the test with the statement and the error
verification:
  - run: ATRYUM_POSTGRES_TESTS=1 ATRYUM_POSTGRES_TEST_DSN=<throwaway Postgres 18> go test -count=1 ./...
    result: ok in every package; all seven TestPostgres* tests PASS twice with -count=2, none skipped (checked with -v)
  - run: gofmt -l cmd internal pkg; go vet ./internal/store
    result: clean
  - run: gh pr checks 193
    result: Go, Baseline and CLA pass; live harnesses skipped

panchicore

Merge · 0 blocking · 0 open · panchicore:F2 and F3 verified

The login-check test now fails when either the BYPASSRLS clause or the owner clause is removed. After one run, and after the store package twice, the cluster holds no test roles.

Ready for your approval, @panchicore.

_{calibrated-review v2 · recheck · run by @panchicore · head 7416a8c}

Agent handoff · calibrated-review v2 · recheck · 7416a8c

calibrated-review: 2
mode: recheck
pr: validmind/atryum#193
head: 7416a8ccb3035cb1a6ebc4e36169d9443365fe75
previous_head: 03f332bad1830faa005bd3623ca12f680b7d8113
run_by: panchicore
verdict: merge
downgraded: 0
items:
  - id: panchicore:F2
    status: verified
    evidence: on a fresh Postgres 18 at 7416a8c, "(r.rolsuper OR r.rolbypassrls)" -> "r.rolsuper" fails "a BYPASSRLS login: <nil>, want refused"; the relowner clause removed fails "a member of a table's owner: <nil>, want refused"; head passes
  - id: panchicore:F3
    status: verified
    evidence: non-system roles on the fresh cluster 0 before, 0 after TestPostgresCheckRowSecurityBinds, 0 after go test ./internal/store -count=2
delta: 03f332b..7416a8c, internal/store/tenant_postgres_test.go only

cachafla · 2026-11-10T20:07:20Z

kam-validmind:F4 deferred to sc-19653: cap the client-info cache per org. It's needed before AA serves a second org.

cachafla and others added 3 commits November 9, 2026 23:24

cachafla marked this pull request as ready for review November 10, 2026 07:10

cachafla requested review from kam-validmind and panchicore November 10, 2026 07:11

panchicore reviewed Nov 10, 2026

View reviewed changes

panchicore approved these changes Nov 10, 2026 •

edited

Loading

View reviewed changes

kam-validmind reviewed Nov 10, 2026

View reviewed changes

kam-validmind approved these changes Nov 10, 2026

View reviewed changes

Test that a member of a table's owner is refused (kam-validmind:F5)

03f332b

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

panchicore reviewed Nov 10, 2026

View reviewed changes

panchicore approved these changes Nov 10, 2026

View reviewed changes

kam-validmind reviewed Nov 10, 2026

View reviewed changes

panchicore reviewed Nov 10, 2026

View reviewed changes

panchicore approved these changes Nov 10, 2026

View reviewed changes

cachafla merged commit 68b25cd into main Nov 10, 2026
4 checks passed

cachafla deleted the andres/sc-19505/request-tenant branch November 10, 2026 20:14

cachafla mentioned this pull request Nov 10, 2026

release: changelog for 0.9.0 #194

Merged

Conversation

cachafla commented Nov 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changes

Tests

Self-review

Docs

Not in this PR

Uh oh!

panchicore left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kam-validmind left a comment

Choose a reason for hiding this comment

Uh oh!

kam-validmind left a comment

Choose a reason for hiding this comment

What this PR does

How it works

Findings

🟡 kam-validmind:F2: a broken tenant hook leaves no trace in the logs

🟡 kam-validmind:F3: the docs leave three API-key routes out of the list of what fails

What we checked and found safe

Terms used here

Uh oh!

kam-validmind left a comment

Choose a reason for hiding this comment

Findings

🔵 kam-validmind:F4: one shared cap lets one tenant evict another tenant's client info

What we checked and found safe

Terms used here

Uh oh!

kam-validmind commented Nov 10, 2026

Uh oh!

cachafla commented Nov 10, 2026

Uh oh!

kam-validmind left a comment

Choose a reason for hiding this comment

What this commit does

How it works

Items answered

Findings

🟡 kam-validmind:F5: the login test doesn't cover the table owner

What we checked and found safe

Terms used here

Uh oh!

cachafla commented Nov 10, 2026

Uh oh!

panchicore left a comment

Choose a reason for hiding this comment

Uh oh!

kam-validmind left a comment

Choose a reason for hiding this comment

What this commit does

Items answered

Findings

🟡 kam-validmind:F6: the new test case leaks two database roles per run

What we checked and found safe

Terms used here

Uh oh!

cachafla commented Nov 10, 2026

Uh oh!

kam-validmind left a comment

Choose a reason for hiding this comment

What this commit does

Items answered

What we checked and found safe

Terms used here

Uh oh!

panchicore left a comment

Choose a reason for hiding this comment

Uh oh!

cachafla commented Nov 10, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

cachafla commented Nov 10, 2026 •

edited

Loading