Skip to content

feat: add featureGated field to models, cleanup stale flags, sort and group flags in UI#808

Merged
maskarb merged 4 commits intomainfrom
feat/feature-gated-models-cleanup
Mar 5, 2026
Merged

feat: add featureGated field to models, cleanup stale flags, sort and group flags in UI#808
maskarb merged 4 commits intomainfrom
feat/feature-gated-models-cleanup

Conversation

@maskarb
Copy link
Contributor

@maskarb maskarb commented Mar 4, 2026

Summary

  • Archive stale Unleash feature flags when models are marked featureGated: false in models.json
  • Sort /models API response alphabetically by label for consistent frontend dropdown ordering

Changes

Stale flag cleanup (sync_flags.go, main.go)

  • StaleFlagsFromManifest() — identifies flag names for non-gated models that should be removed from Unleash
  • CleanupStaleFlags() — checks if each flag exists in Unleash and archives it via the Admin API DELETE endpoint; silently skips flags that don't exist (idempotent)
  • archiveFlag() — calls DELETE /api/admin/projects/{project}/features/{name}, treats 404 as success
  • SyncAndCleanupAsync() — runs sync then cleanup in a background goroutine with retries; cleanup failure is non-fatal (logged but doesn't trigger retries or block startup)
  • Wired into both entry points: SyncModelFlagsFromFile (CLI subcommand) and server startup in main.go

Sorted models response (models.go)

  • ListModelsForProject now sorts the filtered model list by Label before returning, so the frontend dropdown is always alphabetically ordered regardless of manifest order or flag state

Tests (sync_flags_test.go)

  • TestStaleFlagsFromManifest_ReturnsNonGatedModels — verifies non-gated models produce stale flag names
  • TestStaleFlagsFromManifest_EmptyWhenAllGated — empty result when all models are gated
  • TestCleanupStaleFlags_ArchivesExistingFlags — mocks Unleash, verifies DELETE called with correct path
  • TestCleanupStaleFlags_SkipsNonExistentFlags — 404 from GET → no DELETE
  • TestCleanupStaleFlags_SkipsWhenEnvNotSet — graceful no-op when Unleash not configured
  • TestCleanupStaleFlags_EmptyList — early return for nil input

Test plan

  • cd components/backend && go vet ./... && go test -tags test ./cmd/
  • Verify stale flags are archived on server startup when UNLEASH_ADMIN_URL and UNLEASH_ADMIN_TOKEN are set
  • Verify models dropdown is sorted alphabetically in create-session dialog
  • Verify sync-model-flags CLI subcommand archives stale flags after sync

🤖 Generated with Claude Code

maskarb and others added 2 commits March 4, 2026 17:16
…category

Add `featureGated` boolean to models.json so established models
(haiku-4-5, opus-4-5, defaults) are always available without needing
Unleash flags, while newer models require explicit flag enablement.
The model discovery script sets featureGated=true for newly discovered
models so they're disabled by default.

Group and alphabetically sort feature flags in the workspace UI by
their prefix category (Models, Runners, Frameworks, etc.) with
section headers and counts for better organization.

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

Add CleanupStaleFlags to archive Unleash feature flags for models
that have been marked featureGated=false in models.json. Runs after
flag sync at both server startup and via the sync-model-flags CLI.
Cleanup failures are non-fatal to avoid blocking startup.

Sort the /models API response alphabetically by label for consistent
ordering in the frontend dropdown.

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

This comment has been minimized.

Flag names are API identifiers built from model IDs, not user input.
Wrapping them in sanitizeLogString could cause name mismatches between
FlagsFromManifest (creation) and StaleFlagsFromManifest (cleanup) if
a model ID ever contained characters the function strips. Keep
sanitizeLogString only on the Description field which is user-facing.

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

This comment has been minimized.

Add http.StatusNoContent to archiveFlag success cases to handle
proxy layers that normalize Unleash's 202 response to 204.

Add inline comment explaining why new models default to
featureGated=true in the discovery script.

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

github-actions bot commented Mar 5, 2026

Claude Code Review

Summary

PR 808 adds a featureGated field to ModelEntry, wires stale-flag cleanup into the Unleash sync pipeline, sorts the /models API response alphabetically, and groups feature flags by category in the UI. Well-structured with solid test coverage. No blocking or critical issues found.

Issues by Severity

Blocker Issues: None

Critical Issues: None

Major Issues: None

Minor Issues:

M1. StaleFlagsFromManifest includes unavailable models that never had flags
File: components/backend/cmd/sync_flags.go lines 40-49

The function iterates all non-gated models regardless of Available. Models with available=false were filtered out by the old FlagsFromManifest so no Unleash flag was ever created for them. Cleanup handles this gracefully (existence check before DELETE), but it produces unnecessary API round-trips. Fix: add the Available filter to the skip condition to mirror FlagsFromManifest.

M2. respBody is read unconditionally in archiveFlag on success paths
File: components/backend/cmd/sync_flags.go line 154

io.ReadAll is called before the switch statement, but respBody is only used in the default error branch. On 200/202/204 paths the buffer is allocated and discarded. Minor IO waste; not a correctness issue.

M3. sort.Slice is not stable
File: components/backend/handlers/models.go line 403

Current model labels are unique so instability will not manifest in practice, but sort.SliceStable would be semantically cleaner for a deterministic API response.

M4. Cleanup-only-after-successful-sync behaviour is undocumented
File: components/backend/cmd/sync_flags.go lines 132-144

If SyncFlags fails on every retry (e.g. Unleash unreachable at startup), CleanupStaleFlags is never called. Intentional design trade-off, but not captured in the SyncAndCleanupAsync godoc. A one-line comment would help future readers.

Positive Highlights

  • Comprehensive test coverage: Six new test cases cover the happy path, idempotency (flag already gone), env-var skip, and empty-list early return.
  • Idempotent cleanup design: flagExists check before DELETE and treating 404 as success in archiveFlag make the operation safe to replay across deployments.
  • Non-fatal async cleanup: CleanupStaleFlags failure is logged but does not trigger retries or block server startup.
  • Clean TypeScript: New frontend code uses type over interface, zero any types, and GroupRows is correctly colocated in the same file.
  • url.PathEscape applied to both project and flagName in archiveFlag -- no path-injection risk.
  • useMemo on groupedFlags prevents redundant grouping work on every render.

Recommendations

  1. Add the Available filter to StaleFlagsFromManifest to avoid unnecessary Unleash round-trips (M1).
  2. Add a godoc note to SyncAndCleanupAsync documenting the cleanup-after-successful-sync-only behaviour (M4).
  3. Defer io.ReadAll to the error branch in archiveFlag (M2) -- lowest priority.

@maskarb maskarb marked this pull request as ready for review March 5, 2026 01:04
@ambient-code ambient-code bot added this to the Merge Queue milestone Mar 5, 2026
@maskarb maskarb merged commit c270847 into main Mar 5, 2026
24 checks passed
@maskarb maskarb deleted the feat/feature-gated-models-cleanup branch March 5, 2026 17:50
@ambient-code ambient-code bot removed this from the Merge Queue milestone Mar 5, 2026
@ambient-code
Copy link

ambient-code bot commented Mar 5, 2026

Merge Readiness — Blockers Found

Check Status Detail
CI pass
Merge conflicts pass
Review comments pass Has comments — agent to evaluate
Jira hygiene warn No Jira reference found
Staleness pass
Diff overlap risk FAIL Line overlap with #811 on scripts/model-discovery.py

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

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