fix: tier-targeting follow-ups — scheduler pool re-check, region job filter, tier UX, OpenAPI

[guohai]

Summary

Closes the four non-critical findings from PR #121's code review:

1. Scheduler tier-composition re-check (medium): the workflow's secrets are mutable after schedule creation, so a public/team schedule whose workflow later gains a login-class secret would emit an unclaimable session-injected job every tick (each riding the 24h backstop). The scheduler now re-checks via a new pure sessionPoolViolation() gate (unit-tested) and disables violating schedules, mirroring the existing misconfigured-credentials handling.
2. Jobs-list region filter (medium): GET /api/eval-jobs filter moves from exact ?siteId= to ?region=<baseId> — matching claimed rows by site prefix AND pending pooled rows by targetRegion, so queued jobs no longer disappear under the filter. Console dropdown switches to region options.
3. Tier UX (low): run-targets now marks team unavailable (reason session-injected) exactly when the run route would 403 it (personal session workflow); both run dialogs hop the selected tier to the first available option once live tier data arrives, so the default action can't 403.
4. OpenAPI (low): EvalJob.siteId marked nullable with claim-stamp semantics documented; targetRegion/targetTier added.

Test plan

• New unit tests for sessionPoolViolation (public always blocked; team requires workflow∈creator's org; private never blocked).
• Filter tests migrated to region semantics incl. pending-pooled-rows-visible assertion (site-id-wire, api.test).
• tsc clean; full gate: audio + E2E PASSED; unit 1583 passed with the only 2 failing files re-verified green in isolation (documented environmental races: parallel schema teardown in credits-repo; admin-org pollution from api.test's own org suite).

Generated with SMT smt@agora.build

[github-actions]

🤖 Codex Code Review

Findings

• Medium: server/routes.ts:4259 still reports private pooled dispatch as available for session-injected workflows even when the caller is neither the workflow owner nor in the workflow org. The run route rejects that case at server/routes.ts:4077 because untargeted session dispatch is owner/org-only. With the new auto-hop in client/src/pages/run-your-own.tsx:202 and client/src/pages/console-workflow-detail.tsx:107, a non-owner running a public credential-injected workflow will move from disabled public to supposedly available private, and the default action still 403s. run-targets should mark all pooled tiers unavailable when needsSession && !isOwner && !isWorkflowOrgMember, or otherwise surface that only explicit shared-agent dispatch with consent is allowed.

Notes

• I did not run tests due the read-only/no-approval environment.
• The scheduler re-check and region filter changes otherwise look consistent with the existing claim and run-route semantics.

[github-actions]

🤖 Claude Code Review

Overall this is a solid, well-targeted follow-up — the scheduler re-check closes a real TOCTOU (mutable workflow secrets vs. a tier chosen at schedule-creation time), and the pooled-job region filter fixes a genuine "pending jobs vanish" bug. A few issues worth addressing:

Medium

1. The private tier still mis-advertises availability for session-injected workflows — server/routes.ts:4258-4266

The PR fixes the team mirror gap but leaves the same gap on private. The run route's untargeted branch gates all pooled tiers on owner-or-org for session workflows (routes.ts:4077-4083), yet run-targets always reports { tier: "private", available: true }.

Concrete failure: user X runs a public workflow owned by Y that references a login-class secret. needsSession is computed under Y's scope, so it's true → public blocked, team blocked, private "available". The new auto-hop useEffect then selects private, and the run 403s with "Credential-injected workflows can only be run untargeted by the owner or an org member" — exactly the class of bug this PR is fixing. Suggest folding the owner-or-org check in:

const sessionDispatchAllowed = !needsSession ||
  workflow.ownerId === user.id ||
  (workflow.organizationId != null && sameOrg({ organizationId: user.organizationId }, { organizationId: workflow.organizationId }));

and gating private (and team) on it. Bonus: teamBlockedBySession becomes a special case of the same predicate.

Related: if no tier is available the useEffect silently leaves the unavailable tier selected and the user still gets a 403 — worth surfacing the reason in the UI rather than relying solely on the hop.

2. sessionPoolViolation fails open on unknown tiers — server/permissions.ts:107-122

private and shared both fall through to return null. shared is explicitly "reserved" per the comment, so a session-injected job in a shared pool would be allowed by this gate. It's unreachable today (both schedule create and PATCH reject shared, and the column defaults to public), but a security predicate should be allowlist-shaped:

if (targetTier === "private") return null;
if (targetTier === "team") { ...check...; return null; }
return `credential-injected jobs cannot use the ${targetTier} pool`;

That also future-proofs it if dispatchTierEnum gains a member.

Low

3. Region LIKE filter is over-inclusive across prefix-colliding baseIds — server/storage.ts:1178-1187

like(evalJobs.siteId, ${region}-%) matches any baseId that has region as a dash-delimited prefix. Both na-us-seattle and na-us-seattle-north are valid under the admin baseId regex (routes.ts:945-950), and the codebase already acknowledges this collision — routes.ts:87 sorts locations by baseId.length descending precisely to disambiguate site→location resolution. Filtering by na-us-seattle would silently include na-us-seattle-north-01 jobs.

Sites are always <base>-NN (shared/regions.ts), so tightening the pattern fixes it, e.g. sql\${evalJobs.siteId} ~ ${'^' + escapeRegex(region) + '-\d+$'}`orlike(...) plus a length/similar toconstraint. No injection risk here (the baseId regex excludes%and_`, and the value is whitelist-validated first), so this is correctness-only.

4. needsSession (run-targets) and sessionNeed (run route) are computed by different code paths. run-targets uses referencedSecrets.some(s => s.brokerType != null) over workflow + eval-set configs; the run route uses evaluateSessionRequirement(parsePlatformSetup(stepsPrefix), ...). Where they diverge the UI and the enforcement disagree. The current direction (UI over-blocks) is the safe one, but since this PR's whole point is "mirror the run route exactly," it'd be worth deriving both from evaluateSessionRequirement.

5. Silent API contract change. GET /api/eval-jobs?siteId= went from strict-validated to silently ignored. Any API-key consumer filtering by site now gets unfiltered results rather than an error — a widening, not a narrowing. /api/eval-jobs isn't in docs/openapi.yaml, so nothing to update there, but consider 400-ing on siteId with a "use region" message for one release.

Nits

• server/index.ts:427 / permissions.ts — disabling the schedule is the right call vs. emitting unclaimable jobs, but the only signal to the owner is a server log line. Consistent with the existing misconfigured path, so fine as-is; a lastError-style field on the schedule row would close the loop later.
• Double blank line after the new useEffect in both console-workflow-detail.tsx:113 and run-your-own.tsx:209.
• The two useEffect blocks are byte-identical across the two pages — a small shared hook (useTierAutoHop(runTargets, targetTier, setTargetTier)) would keep them from drifting.

The new tests in permissions-dispatch.test.ts cover sessionPoolViolation's branches well, and the site-id-wire.test.ts update correctly asserts both halves of the pooled/claimed disjunction. I couldn't run tsc/the suite here (approval-gated), so the pre-merge ./scripts/full-tests-run.sh gate still applies.

[github-actions]

🤖 Codex Code Review

Findings

• Medium: server/index.ts:414 calls stampOwnerSession() before the new pool-composition re-check at server/index.ts:425. stampOwnerSession() has side effects: it mutates jobConfig and calls ensureSession() at server/auth-session.ts:236, which can mint/cache a login session. So a schedule that should be disabled because it now targets a disallowed public/invalid team pool still uses the workflow’s brokered credentials once before being disabled. The new gate should run before any session mint/prewarm side effect, e.g. by splitting “evaluate session requirement / build snapshot stamp” from “stamp config and ensureSession”, or by doing the tier violation check immediately after pure session evaluation and before ensureSession().

Notes

• I did not find an auth bypass or SQL injection issue in the new region filter; the route validates against stored region base IDs before the regex predicate.
• Residual performance risk: the new region filter uses a regex on siteId, so it may need query-plan validation on a large eval_jobs table, but I’m not flagging it as a concrete defect without plan data.

[github-actions]

🤖 Claude Code Review

Reviewed the full diff against merge base 670cb7d. The core changes are sound — the run-targets availability logic genuinely mirrors the run route's guards at server/routes.ts:4077-4114, sessionPoolViolation is allowlist-shaped and fails closed on shared/future enum members, the regex pattern is bound as a $n parameter (no injection), and the region filter correctly keeps pending pooled rows. Rejecting siteId with an explanatory 400 rather than silently ignoring it is the right call for a breaking param rename.

A few things worth addressing:

1. Region filter drops index usage (server/storage.ts:1178-1191)

siteId ~ '^base-[0-9]+$' cannot use eval_jobs_status_site_idx, whereas the previous eq() could. Combined with the pre-existing pattern in /api/eval-jobs of fetching all matching rows and paginating in memory (server/routes.ts:4336-4358), an unbounded-hours region query becomes a full scan as eval_jobs grows.

The route already resolves the region location and has loc.allocatedRegions in hand. Passing that list and using inArray(evalJobs.siteId, sites) is exact, index-eligible, and removes the regex plus its "carries no metacharacters" caveat entirely:

or(inArray(evalJobs.siteId, filters.sites), and(isNull(evalJobs.siteId), eq(evalJobs.targetRegion, filters.region)))

2. Legacy schedules get hard-disabled with no user-visible reason (server/index.ts:420-431)

Migration 0034 backfilled every pre-existing schedule to target_tier = 'public'. So any legacy schedule whose workflow references a login-class secret will be permanently disabled on the next tick — correct behavior (it was emitting unclaimable jobs before), but the only trace is a server log. The owner sees a paused schedule with no explanation and no way to recover except re-creating it. The same gap already exists on the misconfigured path above it, so this is arguably a pre-existing pattern, but this PR widens the blast radius to a class of schedules that previously looked "enabled". Consider persisting a disabledReason on the row and surfacing it in the schedules list.

3. Redundant disjunct in the team-tier gate (server/routes.ts:4266-4270)

teamBlockedBySession || !sessionDispatchAllowed — since !sessionDispatchAllowed is needsSession && !isOwner && !isTeam and teamBlockedBySession is needsSession && !isTeam, the first strictly subsumes the second. The || !sessionDispatchAllowed never changes the result. Harmless, but it sits right under a comment about mirroring exactly, so it reads as if it's carrying weight.

4. Disabled tiers render with no reason text (console-workflow-detail.tsx:241, run-your-own.tsx:546)

Only reason === "no-org" maps to visible text; "session-injected" and "not-pooled-yet" render as a silently greyed-out option. Since the new useEffect also moves the user off their selection without comment, a user who picked Public on a login-credential workflow gets bounced to Private with zero feedback. For a PR whose stated scope includes tier UX, mapping those two reasons to a short string would close the loop.

5. Auto-hop has no all-unavailable branch (same two files)

When needsSession and the caller is neither owner nor org-member (reachable — a public workflow is runnable by anyone per canRunWorkflow), all four tiers come back available: false. firstAvailable is undefined, targetTier stays on an unavailable value, and the Run button still submits into a 403. Worth disabling the submit action when tiers.every(t => !t.available).

6. Minor: useRegionLocationOptions has no static fallback (unlike useSiteOptions's SITES), so the jobs-page region dropdown is momentarily empty on first paint. Also note 0033 was a pure column rename with no data backfill, so any legacy site_id not shaped <base>-NN will never match the region filter — not a regression (those rows weren't reachable via ?siteId= either), just a known blind spot.

[github-actions]

🤖 Codex Code Review

Findings

• Medium: server/routes.ts:2417 still lets POST /api/eval-schedules/:id/run-now create a session-injected pooled job using the schedule’s stale targetTier. If a public/team schedule was valid when created, then the workflow later gains login-class secrets, the background tick now disables it before creating jobs, but run-now skips sessionPoolViolation() and proceeds to createEvalJob with targetRegion: schedule.region / targetTier: schedule.targetTier at server/routes.ts:2435. For public this creates an unclaimable session job; for invalid team composition it can be claimed then fail /session. Reuse the new helper before stampOwnerSession() here, and probably also during schedule re-enable/reschedule when the effective tier is unchanged.

Notes

• The region filter change looks sound: server-side whitelist validation plus parameterized regex avoids injection, and pending pooled rows are covered.
• The run-targets UX changes appear to match the run route for pooled dispatch.
• I did not run tests because this environment is read-only.

[github-actions]

🤖 Claude Code Review

The changes are sound overall — the security posture is solid (the region value is whitelist-validated against region_locations and bound as a parameter, and baseId is regex-constrained to ^[a-z0-9]+(?:-[a-z0-9]+)*$ at creation, so the constructed pattern can't carry metacharacters). The scheduler re-check ordering (pure detection before stampOwnerSession, so a doomed schedule doesn't burn a broker mint) is the right call, and sessionPoolViolation's allowlist shape correctly fails closed on shared. Findings below.

1. Region filter defeats the (status, siteId) index — server/storage.ts:1187-1192

The old predicate was eq(evalJobs.siteId, ...), which could use eval_jobs_status_site_idx (shared/schema.ts:382). The regex operator ~ cannot use a b-tree index, so this now forces a sequential scan. It compounds with two existing properties of the route: hours is optional (server/routes.ts:4328), and the handler fetches all matching rows and paginates in memory (server/routes.ts:4356-4358). GET /api/eval-jobs?region=na-us-seattle with no hours is a full-table scan plus full materialization.

A sargable formulation preserves the exactness the comment is after — range comparisons use the b-tree regardless of collation, unlike LIKE:

and(
  gte(evalJobs.siteId, `${filters.region}-`),
  lt(evalJobs.siteId, `${filters.region}-:`),   // ':' == '9' + 1
  sql`${evalJobs.siteId} ~ ${"^" + filters.region + "-[0-9]+$"}`,  // exactness, on the narrowed set
)

Or, since region_locations.nextSequence bounds the site set, inArray over the region's allocated site IDs.

2. The wire test can't catch the regression its comment describes — tests/site-id-wire.test.ts:79

The comment in storage.ts:1182-1183 says the anchor exists so na-us-seattle doesn't match na-us-seattle-north-01. But the test asserts only:

expect(job.siteId.startsWith(`${BASE_NA}-`)).toBe(true);

na-us-seattle-north-01 starts with na-us-seattle-, so a bare LIKE 'base-%' implementation passes this assertion. The guard is untested. A unit-level test asserting /^base-[0-9]+$/ against a fixture set containing a prefix-colliding baseId would actually pin the invariant.

3. sessionPoolViolation is extracted but only wired into one of four call sites

The new helper (server/permissions.ts:98-124) encodes rules that remain open-coded in three other places: server/routes.ts:4104 and :4113 (run route), :2175/:2181 (schedule create), and :2298 (schedule update). The helper's own docstring names drift as the risk it's guarding against, so leaving the copies is self-defeating. There's already a shape divergence: the helper is allowlist-based while routes.ts:4090 uses ["private","team","public"].includes(...) with a separate shared branch — equivalent today, divergent the moment the enum gains a member.

Related: server/index.ts:421-426 open-codes stampOwnerSession's internals (parsePlatformSetup → evaluateSessionRequirement → getBrokeredSecretNames) verbatim, so every enabled schedule now issues a duplicate getBrokeredSecretNames query per tick and detection logic lives in two places. Exporting a detectSessionNeed(workflow) from auth-session.ts — consumed by stampOwnerSession, the run route, and the scheduler — collapses both duplications.

4. Run button is still clickable during the run-targets fetch — console-workflow-detail.tsx:97-101, 119-121

noPoolAvailable short-circuits when runTargets is undefined, and the fallback tierOptions claims public: available: true. The query is enabled: runDialogOpen && !!runRegion && !!runEvalSetId, and the button's other guards are exactly !runRegion || !runEvalSetId — so once both are set the button enables while the query is still in flight, and a fast click on a credential-injected workflow gets the 403 the PR is trying to eliminate. Pulling isPending/isFetching off the query into the disabled expression closes it. Same shape in run-your-own.tsx:610-613.

5. ?siteId= removal isn't documented anywhere

/api/eval-jobs returning 400 for siteId (server/routes.ts:4297-4299) is a deliberate breaking change, and the explicit error beats silently ignoring it. But /api/eval-jobs isn't in docs/openapi.yaml at all, so neither the removal nor the new region param is documented — the OpenAPI additions in this PR only cover the EvalJob schema fields. Note also that siteId remains the filter name on /metrics/leaderboard and throughout routes-api-v1.ts; the two are genuinely different scopes, but a one-line note in the spec would prevent the obvious confusion.

Minor

• sessionDispatchAllowed and teamBlockedBySession (server/routes.ts:4259-4263) each recompute the same workflow.organizationId != null && sameOrg(...) expression — hoist it to one const isTeamWorkflow.
• The tier auto-hop silently overwrites an explicit user selection. The disabled SelectItem carries the reason, so it's discoverable, but a user who deliberately chose "public" gets moved without acknowledgement.
• Stray double blank line after the useEffect in both client files.
• noPoolAvailable's every() includes the always-unavailable shared tier. Harmless today (it can only ever be false, so it never blocks the guard), but it reads as if shared participates in the decision.

[github-actions]

🤖 Claude Code Review

Findings

🔴 Blocking — server/storage.ts:1178-1198: the \uFFFF upper bound is collation-dependent and silently drops rows

The region filter's "claimed site" arm is:

gte(evalJobs.siteId, `${filters.region}-`),
lte(evalJobs.siteId, `${filters.region}-\uFFFF`),
sql`${evalJobs.siteId} ~ ${"^" + filters.region + "-[0-9]+$"}`,

The sentinel-range trick only works under C/POSIX collation. Under any glibc locale collation, U+FFFF is ignorable at the primary levels, so it sorts before digits. Verified on this machine:

$ printf 'b-01\nb-\xef\xbf\xbf\nb-\nb-z\n' | LC_ALL=en_US.UTF-8 sort
b-
b-<U+FFFF>      ← sorts BEFORE b-01
b-01
b-z
$ ... | LC_ALL=C sort
b-  b-01  b-z  b-<U+FFFF>   ← sorts last, as the code assumes

Consequence: on a DB whose site_id collation is a glibc locale (e.g. en_US.UTF-8 — the default for the official Debian postgres image, which sets LANG=en_US.utf8, and for many managed PGs), site_id <= 'na-us-seattle-\uFFFF' is false for every real site id, so GET /api/eval-jobs?region=… returns only pending pooled rows and hides every claimed/running/completed job. No error, just missing data.

This won't be caught locally: docker-compose.yml:3 uses postgres:15-alpine (musl, initdb defaults to C), so tests/tier-pool-claim.test.ts passes regardless. If prod and dev collations differ, the new test is a false green.

Suggested fix — drop the range and keep the regex, which is correct under all collations (migration 0023 already RAISE EXCEPTIONs unless every site_id matches ^base-[0-9]+$, so exactness is guaranteed). If you want the bounds for sargability, pin them with COLLATE "C" on both sides — but note the index is eval_jobs_status_site_idx on (status, site_id), so a site_id range is only usable when a status filter is also present, and the console jobs tab defaults to status=all. The inline comment ("Sargable range narrows via the (status, site_id) b-tree first") overstates what the planner can do here.

🟡 The pool-composition rule now lives in three hand-maintained copies

sessionPoolViolation (permissions.ts:108), the run route's inline checks (routes.ts:4118-4132), and the run-targets availability block (routes.ts:4276-4292) all encode the same rule in different shapes. Since keeping them in sync is the entire point of this PR, the two inline copies should call the helper. Concretely: routes.ts:4118-4132's targetTier === "public" / targetTier === "team" 403s are exactly sessionPoolViolation(targetTier, workflow, user).

Related: the helper's name and doc read like the complete session gate, but it deliberately returns null for private and doesn't encode the owner-or-team dispatcher gate the run route applies separately (routes.ts:4096-4102). That's safe for the two current callers (both canScheduleWorkflow-gated, so creator == owner), but a future pooled-dispatch path calling only this helper would be under-gated. Worth saying so in the doc comment.

🟢 Smaller notes

• routes.ts:4332 — (await storage.getAllRegionLocations()).find(l => l.baseId === …) selects the whole table per request; storage.getRegionLocationByBaseId() already exists (used at routes.ts:953). Same pattern at routes.ts:4113 in the run route. Also note the jobs filter accepts inactive regions while the run route requires isActive — probably intentional for history, but useRegionLocationOptions only lists active ones, so an inactive region is unselectable from the UI anyway.
• Duplicate work in the new pre-checks: parsePlatformSetup + getBrokeredSecretNames run once in the new block and again inside stampOwnerSession (auth-session.ts:227-230) — two extra queries per due schedule per tick, and per run-now. A stampOwnerSession(workflow, cfg, precomputedReq?) overload would collapse it.
• ?siteId= now hard-400s. Fine for a session-only console endpoint that isn't in openapi.yaml's path list, and the explicit message is better than silent ignoring — just confirm no external consumer is on it, since the OpenAPI change only touched the EvalJob schema.
• disabled={… || runTargetsFetching || …} also fires on background refetches (window refocus), briefly disabling Run. Minor; isFetching is the safe choice after a region change, so probably fine as-is.
• Residual "never offer a 403" gap (pre-existing, adjacent): run-targets still returns agents.mine for a non-owner on a session-injected workflow, but the targeted branch's ownerTrusted check (routes.ts:4076-4083) 403s those. The tier list is now correct; the agent picker isn't.

What's right

• Ordering the pure evaluateSessionRequirement check before stampOwnerSession in both the scheduler tick and run-now is the correct call — it avoids burning a broker mint (ensureSession) on a dispatch that's about to be rejected.
• sessionPoolViolation's allowlist shape genuinely fails closed on shared/future enum members, and the test asserts it.
• The run-targets tier block mirrors the run route's untargeted branch exactly (owner-or-team for all pools, team additionally requiring workflow-org match, public always blocked) — I traced each arm.
• The prefix-collision test (na-us-ashburn vs na-us-ashburn-west) is the right test to have written; it just needs to run against the production collation to be meaningful.

[github-actions]

🤖 Codex Code Review

No findings.

I reviewed the PR diff with a security/logic/performance focus. The scheduler re-check, region-based job filtering, tier availability changes, and OpenAPI updates look consistent with the described intent. I didn’t see a concrete auth bypass, secret exposure, injection issue, or behavior regression in the changed code.

Residual risk: I did not run the test suite because the environment is read-only; this review is based on static inspection of the diff and surrounding context.

[github-actions]

🤖 Claude Code Review

Review

Reviewed the merge ref (670cb7d...485acfb). The core changes are sound: the sessionPoolViolation extraction is a genuine single-source-of-truth improvement, the TOCTOU re-check ordering (pure detector before stampOwnerSession, so a doomed schedule doesn't burn a broker mint) is correct, and run-targets now mirrors the run route's untargeted gates exactly (sessionDispatchAllowed ≡ isOwner || isTeam, team additionally requiring workflow.organizationId match — verified against server/routes.ts:4090-4128).

Security — no issues found.

• server/storage.ts:1178 interpolates filters.region into a regex, but it's bound as a parameter (not spliced SQL), and baseId is constrained to ^[a-z0-9]+(?:-[a-z0-9]+)*$ at creation (server/routes.ts:948), so no regex metacharacters or ReDoS surface can reach it. The route also whitelist-validates against region_locations first.
• sessionPoolViolation uses an allowlist shape and fails closed on shared/unknown tiers. evalSchedules.targetTier is notNull with a default, so the scheduler/run-now callers can't pass null and trip the catch-all.

Findings

1. server/storage.ts:1178 — region filter is no longer index-usable (perf)

site_id ~ '^base-[0-9]+$' can't use eval_jobs_status_site_idx, unlike the prior eq(). The collation rationale in the comment is correct (range sentinels are genuinely unsafe), but a sargable-and-collation-safe form exists: keep LIKE 'base-%' as a prefix predicate and keep the regex as the exact refinement. Compounding this, /api/eval-jobs fetches all matching rows and paginates in JS (server/routes.ts:4348, 4368-4369) — pre-existing, but it means the filter scans the full time window every request. Fine at current scale; worth a follow-up if eval_jobs grows.

2. server/routes.ts:4262-4272 — needsSession and the run route's sessionNeed are computed differently

run-targets sets needsSession from any referenced brokered secret across workflow + eval-set config, while the run route derives sessionNeed only from platform.setup in workflow.config.stepsPrefix. A brokered secret referenced elsewhere makes the run route return the brokered-misuse 400 (routes.ts:4008), but the UI now shows "not allowed for credential-injected workflows" on every tier and hard-disables submit via noPoolAvailable. Previously this divergence only cost the public tier; it now blocks the whole dialog behind a misleading reason. Consider deriving needsSession in run-targets from evaluateSessionRequirement (as both other callers do) and surfacing misuse separately.

3. Agent picker still offers 403-guaranteed targets (never-offer-a-403 gap)

noPoolAvailable only guards targetTokenId === "any". For a session-injected workflow you don't own, selecting one of your own agents leaves submit enabled and hits the targeted-branch 403 at routes.ts:4079. run-targets returns agents.mine unfiltered, so the client can't tell. Not a regression, but it's the same contract this PR is tightening.

4. server/index.ts:423 — duplicate secret fetch per due schedule per tick

getBrokeredSecretNames(scope) loads the scope's secrets, then stampOwnerSession immediately re-resolves them. Two round trips where one would do; only matters if many schedules fire on the same tick.

Minor

• ?siteId= on /api/eval-jobs is now a hard 400 rather than silently ignored — good, explicit failure beats a silently-unfiltered list. I found no remaining siteId= callers against this endpoint (the hits in tests/e2e/api.spec.ts and tests/api.test.ts:1277 are /api/v1/metrics/leaderboard, untouched). Worth confirming no external/API-key consumers rely on it, since this route isn't in docs/openapi.yaml.
• Auto-hop useEffect (both pages) is stable — it only sets a tier that reports available: true, so no render loop, and the all-unavailable case falls through to the noPoolAvailable submit gate with the reason visible in the select trigger.
• Test coverage is good: tests/tier-pool-claim.test.ts exercises the prefix-collision case (na-us-ashburn vs na-us-ashburn-west) that motivated the regex, and tests/permissions-dispatch.test.ts covers the fail-closed arm.

Nothing here blocks merge; items 1–3 are follow-up candidates.

[github-actions]

🤖 Claude Code Review

Review

The core refactor is sound. sessionPoolViolation is a genuine improvement — allowlist-shaped so unknown/future tiers fail closed, and the run-route substitution is behaviour-identical to the two hand-rolled checks it replaces (targetTier is already narrowed to private|team|public upstream, so the shared/default arm is unreachable-but-safe). Placing the scheduler and run-now checks before stampOwnerSession is the right call — ensureSession is fire-and-forget and would otherwise burn a broker mint for a job about to be rejected. The region regex is parameterized, and region_locations.baseId is validated at creation against ^[a-z0-9]+(?:-[a-z0-9]+)*$ (server/routes.ts:948), so the "no regex metacharacters" claim in the storage comment actually holds. No injection, authz, or credential-exposure regressions found.

Findings, roughly by importance:

1. The "single source of truth" is still triplicated (server/routes.ts:2169-2185, 2293-2308)

sessionPoolViolation was substituted into the run route only. The schedule POST and PATCH routes still carry byte-identical copies of the public-tier and team-org checks it replaces. They agree with the helper today, so this is not a bug — but it's precisely the drift the helper exists to prevent, and the scheduler now enforces the helper's version at runtime, so a divergence would surface as schedules that pass creation and get auto-disabled on first tick. Both call sites have workflow and user in scope already; the swap is mechanical.

2. run-targets computes needsSession differently from the run route (server/routes.ts:4252 vs 3992-3996)

• run-targets: referencedSecrets.some(s => s.brokerType != null) — any brokered secret anywhere in workflow or eval-set config.
• run route: the platform.setup login pair parsed from workflow.config.stepsPrefix only.

These converge today only because findBrokeredMisuse 400s any brokered secret referenced outside the login pair. Two consequences: a misconfigured workflow gets the tier labelled "not allowed for credential-injected workflows" when the real failure is a misused brokered secret, and the mirror silently breaks the moment a second brokerType exists (brokerType != null ⊋ === "auth-session"). Since the comment above the block explicitly promises "must mirror the run route exactly", consider deriving needsSession from evaluateSessionRequirement(...).kind === "need" here too.

3. The never-offer-a-403 contract doesn't cover the agent picker (server/routes.ts:4220-4224, both dialogs)

noPoolAvailable is gated on targetTokenId === "any". For a stranger on a public credential-injected workflow, the tier dropdown is now correctly all-unavailable — but agents.mine still lists every one of their own tokens, and selecting one re-enables the Run button. The targeted branch then 403s at the ownerTrusted gate (routes.ts:4076-4082). mine should be filtered (or annotated) by the same owner/org trust test when needsSession.

4. Re-enabling a schedule skips the composition re-check (server/routes.ts:2293)

The session check inside the owner-gated block fires only on tierChanged. A schedule the scheduler auto-disabled for a pool violation can be re-enabled via PATCH {isEnabled:true} and will be disabled again on the next tick — a silent flap with no user-visible signal (the scheduler only log()s). Widening the check to wantsEnable || tierChanged gives the user the actionable 403 that run-now already returns.

5. getBrokeredSecretNames now runs twice per schedule per tick (server/index.ts:426, server/routes.ts:2429)

The new guard queries the owner's/org's secrets, then stampOwnerSession immediately queries the same rows again (auth-session.ts:228-231). Cheap fix: have stampOwnerSession accept an optional precomputed SessionRequirement, or return it so the caller reuses one fetch.

6. The region filter is no longer index-usable (server/storage.ts:1178-1198)

siteId ~ '^base-[0-9]+$' can't use eval_jobs_status_site_idx, where the old eq(siteId, …) could. Compounding it, the route fetches the entire filtered set unbounded and slices in JS after the visibility pass (routes.ts:4348, 4368-4370) — so this is a full seq-scan over the hours window on every page. Pre-existing unbounded fetch, but the scan is new. Note that targetRegion is set on all pooled rows and never cleared at claim time, so or(eq(targetRegion, region), <regex for targeted/legacy rows>) would give the common case an indexable predicate while keeping the regex only for site-pinned rows. The reasoning in the comment for rejecting collation-dependent range bounds is correct and worth keeping.

Smaller notes: useRegionLocationOptions filters to isActive, but the route's region validation deliberately doesn't — so jobs in a deactivated region are filterable via API but unreachable from the UI dropdown. And the new getEvalJobs region test leaves four rows behind with no cleanup, consistent with the rest of that file but worth knowing if row counts ever get asserted.

The OpenAPI additions (siteId nullable, targetRegion/targetTier) match what the v1 handlers actually return — those endpoints serialize the raw job row.