Skip to content

feat: unify model lists, add workspace overrides to runner type gates#794

Merged
Gkrumbach07 merged 14 commits intomainfrom
feat/unify-models-runner-gates
Mar 4, 2026
Merged

feat: unify model lists, add workspace overrides to runner type gates#794
Gkrumbach07 merged 14 commits intomainfrom
feat/unify-models-runner-gates

Conversation

@maskarb
Copy link
Contributor

@maskarb maskarb commented Mar 4, 2026

Summary

Eliminates duplicate model lists and restores feature-flag gating for models. models.json is now the single source of truth for all models across all providers — no hardcoded fallback lists anywhere. Adds workspace-scoped override support to runner type feature gates and cross-provider model/runner validation.

Problem: PR #788 added hardcoded model lists to the agent registry. The create-session dialog read models from the registry instead of the /api/models endpoint, bypassing all Unleash feature flag and workspace override logic. The featureGate field on runner types only checked global Unleash — no workspace-level control. Nothing prevented using a Gemini model with a Claude runner.

Solution: Models belong in models.json, not the agent registry. Each runner now has a provider field, and the models API filters by provider. Model/runner provider mismatches are rejected at the API boundary — including for default models.

What changed

Backend

  • handlers/models.go — Added ?provider= query param filter. isModelAvailable validates model/runner provider match (requiredProvider param) for all models including defaults. Returns 503 when manifest unavailable (no hardcoded fallbacks). Returns empty defaultModel when no models pass filtering.
  • handlers/runner_types.go — Removed Models/DefaultModel/ModelOption, added Provider. New GetRunnerTypesGlobal (admin). Updated GetRunnerTypes to project-scoped with workspace overrides via isRunnerEnabledWithOverrides.
  • handlers/sessions.go — Resolves runner provider from registry, passes to isModelAvailable. Degrades gracefully when registry unavailable (skips provider matching, still validates against manifest).
  • cmd/sync_flags.goFlagsFromManifest skips provider default models (from providerDefaults map) in addition to the global default.

Operator

  • internal/handlers/registry.go — Synced AgentRuntimeSpec: removed DefaultModel/Models/ModelEntry, added Provider.

Manifests

  • agent-registry-configmap.yaml — Removed models/defaultModel, added provider field per runner.
  • models.json — Added Gemini models. Added providerDefaults for per-provider default model resolution.

Frontend

  • create-session-dialog.tsx — Models fetched from API filtered by selectedRunner?.provider. No hardcoded fallback lists. Static DEFAULT_MODEL for form init, useEffect updates from API.
  • services/api/runner-types.ts — Removed models/defaultModel/RunnerModel/requiredSecretKeys. Added provider. Two API functions: project-scoped + global.
  • services/queries/use-models.ts — Provider in query key with conditional spread (no undefined).
  • app/api/projects/[name]/runner-types/route.ts — New proxy route.
  • All consumers updated to pass projectName or use global variant.

Data flow

User selects runner type (e.g., "Gemini CLI", provider="google")
  → useModels(projectName, open, "google")
    → GET /api/projects/:name/models?provider=google
      → Backend filters models.json by provider
      → Checks Unleash flags + workspace overrides
      → Returns only enabled Google models (or empty defaultModel if none)

User creates session with model
  → Backend resolves runner provider from registry
  → isModelAvailable checks (in order):
    1. Model exists in manifest and is available
    2. Model provider matches runner provider (rejects cross-provider)
    3. Default models skip flag check but still enforce provider match
    4. Non-default models pass feature flag check (workspace override → Unleash)
  → Mismatches rejected with 400

Manifest unavailable (cold start)
  → Try ConfigMap volume mount
  → Try in-memory cache
  → 503 Service Unavailable (no hardcoded fallbacks)

Tests

Test Coverage
Provider filtering (3 tests) ?provider=anthropic, ?provider=google, no filter
Provider mismatch (3 tests) Wrong provider → false, matching provider → true, provider-default with wrong provider → false
isRunnerEnabledWithOverrides (4 tests) Override true/false, no override, nil overrides
GetRunnerTypesGlobal (2 tests) Returns ungated runners, no auth required
FlagsFromManifest (2 tests) Skips global default, provider defaults, unavailable
503 on manifest unavailable (2 tests) Missing file, malformed JSON
isModelAvailable (10 tests) Empty, default, available, unavailable, unknown, fail-open, workspace overrides, provider match/mismatch

Checklist

  • gofmt clean, go vet clean, golangci-lint 0 issues
  • npm run build — 0 errors, 0 warnings
  • eslint src/ --quiet — 0 errors
  • Pre-commit hooks pass
  • All new tests pass (1 pre-existing session test failure on main)
  • Select Claude runner → see only Anthropic models
  • Select Gemini runner → see only Google models
  • Toggle model flag in Feature Flags admin → model list updates
  • Enable runner.gemini-cli.enabled workspace override → Gemini runner appears
  • POST session with mismatched model/runner → 400 rejection
  • POST session with provider-default model on wrong runner → 400 rejection

🤖 Generated with Claude Code

maskarb and others added 2 commits March 4, 2026 12:00
…unner type gates

- Remove models/defaultModel from agent registry, add provider field
- Add Gemini models to models.json (single source of truth)
- Add ?provider= filter to GET /api/projects/:name/models
- Move runner-types to project-scoped route with workspace override support
- Keep global /api/runner-types for admin page (no workspace overrides)
- Frontend fetches models by provider based on selected runner type
- Runner type feature gates now check workspace ConfigMap overrides

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
When the models API is filtered by provider (e.g. ?provider=google),
it now returns the correct provider-specific default model instead of
the global default. This ensures the UI shows "gemini-2.5-flash" as
the default when the Gemini runner is selected, rather than an
Anthropic model.

- Add providerDefaults map to models.json (version 2)
- Resolve effective default in ListModelsForProject based on provider
- Treat provider defaults as always-available in isModelAvailable
- Update test fixtures with Gemini models and providerDefaults

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@maskarb maskarb force-pushed the feat/unify-models-runner-gates branch from 1767e42 to 795f49b Compare March 4, 2026 17:00
@github-actions

This comment has been minimized.

…r struct

- Delete defaultModelsResponse() — models.json is the single source of
  truth. Return 503 when manifest unavailable instead of stale hardcoded
  list.
- Delete fallbackModels in create-session-dialog — show empty dropdown
  while loading instead of a stale list.
- Fix temporal dead zone: useForm referenced defaultModel/modelsData
  before their const declarations. Use static DEFAULT_MODEL for form
  init, useEffect updates when API responds.
- Fix query key: use conditional spread to avoid undefined in cache key.
- Sanitize providerFilter query param.
- Sync operator AgentRuntimeSpec: remove DefaultModel/Models, add
  Provider (matches backend struct).

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions

This comment has been minimized.

maskarb and others added 2 commits March 4, 2026 12:35
…ctName args, gofmt

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…es, GetRunnerTypesGlobal

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions

This comment has been minimized.

…kip provider defaults in flag sync

- Add requiredProvider param to isModelAvailable — rejects model/runner
  provider mismatches (e.g., Gemini model with Claude runner) at the API
  boundary instead of failing at runtime
- Return empty defaultModel when no models pass filtering (prevents
  frontend setting a non-existent default in the form)
- Embed models.json at compile time via go:embed as fallback for cold
  start before ConfigMap volume is mounted
- Skip provider default models in FlagsFromManifest — default models
  for each provider don't need Unleash flags

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions

This comment has been minimized.

The go:embed approach required copying models.json into the backend
directory at build time, adding complexity for a marginal benefit
(seconds of cold start before ConfigMap mounts). The 503 is cleaner.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

test comment

@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

hello

@github-actions

This comment has been minimized.

maskarb and others added 2 commits March 4, 2026 14:36
…match tests

- When GetRuntime fails (registry ConfigMap not mounted), log warning
  and skip provider matching instead of returning 400. Model is still
  validated against the manifest.
- Add test: provider mismatch (google model + anthropic runner) → false
- Add test: provider match (anthropic model + anthropic runner) → true

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…t mismatch test

Default and provider-default models previously bypassed the
requiredProvider check via early return. Now provider matching runs
before the default-model early return — using gemini-2.5-flash with
a Claude runner correctly returns false.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

…d empty model state

- Provider default loop now verifies the provider key matches
  requiredProvider, not just the model ID value
- Submit button disabled while models are loading (prevents submitting
  stale DEFAULT_MODEL before API responds)
- Empty model dropdown shows "No models available for this runner"
  instead of a blank list

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions

This comment has been minimized.

…r switch

- Don't fetch models until runner types have loaded (prevents wasteful
  unfiltered fetch and model list flicker on dialog open)
- Use form.resetField instead of form.setValue to clear both value AND
  dirty state when switching runners, so the useEffect correctly applies
  the new provider's default model

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions

This comment has been minimized.

maskarb and others added 2 commits March 4, 2026 15:11
…nnerTypesError

- Invalidate ["runner-types", projectName] after saving feature flag
  overrides so the runner type dropdown refreshes without a page reload
- Gate models fetch on !runnerTypesError (prevents unfiltered fetch
  when runner types fail to load)
- Initialize models slice to empty (not nil) for consistent JSON []
- Document fail-open provider validation skip on cold start

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@ambient-code
Copy link

ambient-code bot commented Mar 4, 2026

Merge Readiness — Blockers Found

Check Status Detail
CI pass
Merge conflicts pass
Review comments FAIL Has comments — agent to evaluate
Jira hygiene warn No Jira reference found
Staleness pass
Diff overlap risk FAIL Line overlap with #640, #778 on components/frontend/src/app/admin/runtimes/page.tsx, components/frontend/src/components/create-session-dialog.tsx

This comment is auto-generated by the PR Overview workflow and will be updated when the PR changes.

…e submit on model error

Make isModelAvailable reject unknown models when requiredProvider is
set but the manifest is unavailable, rather than failing open. This
ensures cross-provider validation is enforced even during cold start.
Fail-open is preserved only when both manifest and registry are
unavailable (requiredProvider is empty).

Disable the Create Session submit button when the models API returns
an error and no models are loaded, preventing users from submitting
with a stale default model that may not match the selected runner.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@github-actions
Copy link
Contributor

github-actions bot commented Mar 4, 2026

Claude Code Review

Summary

PR #794 unifies model lists under models.json as a single source of truth, adds per-provider model filtering, introduces workspace-scoped runner-type gating, and enforces cross-provider model/runner validation at the API boundary. This is a well-structured refactor with broad test coverage and correct auth patterns throughout. A handful of minor issues are worth addressing before merge.

Issues by Severity

Blocker Issues

None

Critical Issues

None

Major Issues

1. Missing test: provider-default happy path in isModelAvailable
components/backend/handlers/models_test.go

The new isModelAvailable tests cover negative cases well (provider-default rejected for wrong provider, non-default rejected for wrong provider) but the key positive path is untested: a provider-default model accepted when requiredProvider matches its own provider.

Example: gemini-2.5-flash with requiredProvider="google" should return true. Execution path:

  1. entry.Available passes
  2. Provider match passes (entry.Provider == "google" == requiredProvider)
  3. Not global default ("gemini-2.5-flash" != "claude-sonnet-4-5")
  4. Provider-defaults loop: modelID == pd && ("google" == "google")return true

Without this test, a future refactor of step 4 could silently break Gemini session creation.

Suggested addition to the isModelAvailable context:

It("should accept provider-default model when requiredProvider matches", func() {
    writeManifestFile(validManifest)
    setupK8sWithOverrides()
    // gemini-2.5-flash is the google provider default
    result := isModelAvailable(context.Background(), K8sClient, "gemini-2.5-flash", "google", "test-ns")
    Expect(result).To(BeTrue())
})

Minor Issues

1. Redundant condition in provider-defaults loop
components/backend/handlers/models.go (~line 251)

for provider, pd := range manifest.ProviderDefaults {
    if modelID == pd && (requiredProvider == "" || provider == requiredProvider) {

By the time execution reaches this loop, the entry.Provider != requiredProvider guard above has already rejected any provider mismatch. The provider == requiredProvider sub-condition is therefore always satisfied at this point (or requiredProvider == ""). A brief comment explaining this invariant would prevent future confusion about what the redundant check is guarding.


2. defaults[""] edge case in sync_flags.go
components/backend/cmd/sync_flags.go line 13

defaults := map[string]bool{manifest.DefaultModel: true}

If manifest.DefaultModel is the zero value (empty string), "" is inserted as a map key, which would silently skip any model with an empty ID. Risk is negligible (empty IDs are invalid), but it's easy to guard with a non-empty check before inserting.


3. Hardcoded Content-Type in new proxy route
components/frontend/src/app/api/projects/[name]/runner-types/route.ts

return new Response(data, {
  status: response.status,
  headers: { "Content-Type": "application/json" },
});

This forces application/json even when the backend returns a non-JSON error body (e.g., a 503 with plain text). Consider forwarding response.headers.get("content-type") ?? "application/json". (The models route follows the same pre-existing pattern.)


4. Overly complex submit-disabled condition
components/frontend/src/components/create-session-dialog.tsx (~line 1416)

disabled={... || modelsLoading || (modelsError && models.length === 0)}

When modelsError (isError: boolean) is true, modelsData is undefined and models is always []. The models.length === 0 guard is always satisfied, making (modelsError && models.length === 0) equivalent to just modelsError. Simplifying removes dead logic and makes the intent clearer.


Positive Highlights

  • Correct auth pattern in GetRunnerTypes: The new project-scoped handler properly calls GetK8sClientsForRequest and returns 401 on failure before proceeding — textbook correct per project auth standards.
  • 503 instead of hardcoded fallbacks: Returning ServiceUnavailable when the manifest is unavailable is a meaningful correctness improvement; the removed defaultModelsResponse was a genuine maintenance hazard.
  • Provider in query key (use-models.ts): ['models', projectName, provider?] ensures full cache isolation per provider — no risk of stale cross-provider model lists.
  • form.resetField on runner switch: Clearing dirty state before the API re-fetches cleanly allows the useEffect to set the new provider's default without fighting user input state.
  • Runner-types cache invalidation after flag save (feature-flags-section.tsx): Invalidating ["runner-types", projectName] after a workspace override save is a subtle but important correctness fix — gate changes now reflect immediately in the runner selector.
  • Comprehensive test matrix: 10 isModelAvailable tests, 4 isRunnerEnabledWithOverrides tests, 3 provider-filter tests, and 2 503-manifest tests give strong coverage of the new logic.
  • No any types in TypeScript changes; all new types use type over interface; sanitizeParam correctly applied to the new ?provider= query parameter.

Recommendations

  1. (Major) Add the provider-default happy-path test: gemini-2.5-flash + requiredProvider="google"true. This is the key positive path for cross-provider enforcement and is currently untested.
  2. (Minor) Add a comment clarifying the invariant in the ProviderDefaults loop, or simplify the redundant condition.
  3. (Minor) Guard against empty-string DefaultModel in sync_flags.go defaults map initialization.

Generated with Claude Code


🔍 View AI decision process (logs available for 90 days)

📋 View memory system files loaded (click to expand)

What Amber Loaded for Code Review

Amber automatically loaded these repository standards from the memory system:

  1. CLAUDE.md - Master project instructions, development standards
  2. backend-development.md - Go backend, K8s integration patterns
  3. frontend-development.md - NextJS, Shadcn UI, React Query patterns
  4. security-standards.md - Auth, RBAC, token handling
  5. k8s-client-usage.md - User token vs service account patterns
  6. error-handling.md - Consistent error patterns
  7. react-query-usage.md - Data fetching patterns

Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines.

@Gkrumbach07 Gkrumbach07 merged commit a288b33 into main Mar 4, 2026
24 checks passed
@Gkrumbach07 Gkrumbach07 deleted the feat/unify-models-runner-gates branch March 4, 2026 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants