refactor: detectSessionNeed single source; run-targets trust unification; perf + UX

[guohai]

Summary

Closes the remaining improvement backlog from the tier-targeting review rounds (#121–#123):

1. detectSessionNeed() — one detector everywhere. New pure helper in auth-session (parse setup → owner scope → evaluate against brokered secrets) consumed by the run route, run-targets, schedule create/PATCH, run-now, and the scheduler tick. stampOwnerSession takes the precomputed requirement, killing the duplicate getBrokeredSecretNames query per scheduler tick / run-now.
2. run-targets trust unification: needsSession now comes from the same detector the run route enforces with (a misconfigured pair no longer mislabels tiers — dispatch fails with the real misuse error); agents.mine is gated on session trust so a stranger on a credential-injected workflow isn't offered tokens the targeted branch would 403 (shared listings stay — the sanctioned attestation+consent path).
3. Indexable region filter fast-path: eq(target_region) covers pooled rows (stamped at creation, never cleared at claim); the collation-safe regex remains only for site-pinned targeted/legacy rows.
4. Test hygiene: the Organization Roles suite cleans up the admin-owner org it creates — closing the cross-suite pollution race that produced false failures in three gate runs.
5. Latent client bug: workflow-detail's Job History treated the {data,total} response as a bare array (silently broken table).
6. UX: the tier auto-hop announces itself with a toast.

Test plan

• tsc clean; 331 passing across the seven affected suites (+ site-id-wire 7/7 isolated — known parallel wf[0] race); run-your-own E2E 13/0.
• Behavior-preserving by construction at every consolidated site (same detector, same predicates); the only intentional behavior changes are the agents.mine gating, the misconfigured-pair tier labeling, and the toast.

Generated with SMT smt@agora.build

[github-actions]

🤖 Claude Code Review

Reviewed the merge ref (4246e86, diff vs a3324a6). The core refactor is sound: detectSessionNeed genuinely collapses five hand-rolled parsePlatformSetup + sessionScopeForWorkflow + evaluateSessionRequirement + getBrokeredSecretNames chains into one, all five call sites pass the same workflow object they later act on, and the precomputedReq threading in stampOwnerSession preserves the "pure detect before side-effecting stamp" ordering that the pool gates depend on. I verified sessionTrusted in /run-targets is genuinely equivalent to the run route's ownerTrusted gate for tokens in mine (since ownTokens are all createdBy === user.id, owner.organizationId collapses to user.organizationId), and that emptying mine doesn't hide a user's own shared listing — listDispatchable ignores its userId arg and returns all active listings, so the client dedupe just stops removing it and it surfaces under pickerShared.

Findings below, roughly by severity.

tests/api.test.ts:4221 — cleanup hardcodes id = 1 and runs unconditionally

await pool.query('UPDATE users SET organization_id = NULL, org_role = NULL WHERE id = 1');

Nothing guarantees the admin is user 1 — the file resolves admin by ADMIN_EMAIL (line 14, overridable via TEST_ADMIN_EMAIL), never by id. On a dev DB where the admin isn't id 1, this silently strips org membership from an unrelated account, and the leak this afterAll exists to fix persists. It also fires even when the suite never created an org. Key it off the same identity the suite logs in with:

await pool.query('UPDATE users SET organization_id = NULL, org_role = NULL WHERE email = $1', [ADMIN_EMAIL]);

server/routes.ts:4256 — needsSession now returns false for misconfigured workflows

Old: referencedSecrets.some(brokerType != null) → true for a split-class pair or a brokered secret referenced outside platform.setup. New: kind === "need" → false for both. Those workflows now advertise private/team/public as available: true, and the run route 400s. The comment argues the misuse error is more actionable, which is defensible — but the client change in this same PR makes the consequence worse: the auto-hop effect will now silently move the user onto a tier that is guaranteed to fail, without a toast (since the current tier reads as available). Consider a third state (available: false, reason: "misconfigured-credentials") so the dialog can surface the real problem instead of a dead tier. Not blocking, but worth a decision rather than a comment.

server/routes.ts:4219 + 4254 — duplicate full-scope secret fetch per request

detectSessionNeed(workflow) calls getBrokeredSecretNames(scope), which loads every secret row for the scope; classifyReferencedSecrets(scope, …) twenty lines later loads the exact same rows again. Two identical queries on every run-dialog open, in a PR whose title claims perf. Cheapest fix is an optional pre-fetched-rows param on one of them, or deriving the brokered-name set from the rows classifyReferencedSecrets already has. (The getEvalAgentToken N+1 in the shared-listing loop just above is pre-existing — out of scope, but it's the bigger cost on this route.)

server/storage.ts:1193-1204 — correct, but the comment oversells and is now half-stale

The rewrite from or(regex, and(isNull(siteId), eq(targetRegion))) to or(eq(targetRegion), regex) is behaviorally equivalent, but only under the invariant that a pooled row's claim-time siteId always lands inside its targetRegion. That invariant does hold — permissions.ts:177 refuses a claim when token.region !== job.targetRegion, and every creation site (routes.ts:4158-4160, routes.ts:2444, index.ts:450, routes-api-v1.ts:397-398) sets siteId and targetRegion mutually exclusively. Worth a one-line comment pinning that dependency, since the new form breaks loudly if anyone ever re-queues a pooled job across regions.

Two comment problems:

• "indexable predicate" is only true for pending rows. The sole index on target_region is eval_jobs_pending_pool_idx … WHERE status = 'pending' (migration 0034), so the common case here — the job-history list with no status filter — still seq-scans. And the route fetches all matching rows and paginates in JS (visibleJobs.slice, routes.ts:4376-4377), so the predicate isn't where the time goes. Either add a plain target_region index or drop the perf claim.
• The paragraph above the new one still explains the removed isNull(siteId) arm, and the prefix-collision/collation rationale is now stated twice. Trim the first block.

server/auth-session.ts:208-238 — stampOwnerSession's doc comment is now orphaned

detectSessionNeed and its own JSDoc were inserted between stampOwnerSession's doc block and the function. Two consecutive block comments now stack on detectSessionNeed, and stampOwnerSession — the function whose in-place mutation and fire-and-forget mint the comment carefully documents — has none in hover/tooling. Move detectSessionNeed (with its own doc) above the OwnerSessionStampResult type or above the orphaned block.

Client changes

console-workflow-detail.tsx:81-83 is a real bug fix — /api/eval-jobs returns { data, total } (routes.ts:4400-ish), so the Job History table was being handed an object. The ?? [] guard is right. console-eval-jobs.tsx unwraps it separately, so no other caller needs the same change.

The toast additions are fine: toast comes from the module-level singleton in use-toast.ts:186, so adding it to the dep array is a no-op and won't re-fire the effect. No double-toast risk outside StrictMode, since setTargetTier makes current.available true on the next pass. Duplicating the label() map inline in both pages is the one thing I'd push back on lightly — it's four strings, but the two copies will drift when a tier is renamed; the tier labels already appear elsewhere in both dialogs.

Not flagged

sessionDispatchAllowed = sessionTrusted and teamBlockedBySession still mirror sessionPoolViolation exactly (permissions.ts:110-126) — private always allowed, team requires the workflow to belong to the dispatcher's org, public always refused. The scheduler's de-nesting in index.ts:422-431 keeps schedSessionReq correctly scoped per loop iteration and preserves the detect-before-stamp ordering, so a schedule about to be disabled still doesn't burn a mint. Removed imports there are genuinely unused.