Repository navigation
fix(frontend): retire four dead settings API paths, stop a fifth calling admin-gated settings from non-admin surfaces (#16481) - #16484
Merged
Conversation
Closed
4 tasks done
Contributor
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This was referenced Sep 12, 2026
Contributor
6 tasks done
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
This was referenced Sep 12, 2026
Merged
Merged
Merged
Owner
Author
|
Review: approved. It merges on a settled green after #16487.
Follow-up, outside this diff: |
This was referenced Sep 12, 2026
This was referenced Sep 13, 2026
Merged
6 of 7 tasks
This was referenced Sep 14, 2026
5 tasks
mrveiss
added a commit
that referenced
this pull request
Sep 23, 2026
… it on three leftovers (#16912, #16513, #16494, #16704) (#17321) * fix(frontend-tooling): give a worktree a resolvable node_modules and stop a skipped check reading as a pass (#16912) A frontend change made in a worktree had no local verification at all: no node_modules to resolve, so neither vitest nor vue-tsc could run, and pre-push said "skipping" and pushed anyway. Four causes, all four fixed: 1. post-checkout now links autobot-frontend/ and autobot-slm-frontend/ node_modules to the main checkout's install when it runs in a linked worktree -- the same shape as the backend symlink it already restores. Nothing is installed, and a real install in the worktree always wins. 2. autobot-slm-frontend/.gitignore pinned `node_modules/`, a directory-only pattern that does not match a symlink, so the link above showed up as an untracked file. Verified with git status before and after. 3. pre-push reports both frontend checks through could_not_run() when node_modules does not resolve, so "could not look" is no longer spelled the same way as "looked and found nothing" (MEASUREMENT_DISCIPLINE). The vitest branch had no else at all and fell through silently. 4. dependency-floor-reporter.ts could never load: it imported createLogger from @/utils/debugUtils, which re-exports from @autobot/ui, whose index pulls in .vue SFCs that fail to compile in the reporter-loading context, and it default-exported an instance where vitest calls `new`. Either one takes the whole run down at startup, and vitest.config.ts only registers this reporter when CI is unset -- so it broke local runs exclusively. Both fixed; the banner now prints for the first time. Evidence, all from inside a worktree: vitest 4.1.9 and vue-tsc 5.9.3 resolve; `npx vitest run` (the exact invocation pre-push uses) starts and passes 13/13; vue-tsc --noEmit over autobot-frontend reports 0 errors; the hook was run for real with the links removed first and recreated both. * fix(frontend): clear the three leftovers from the settings-path and SecretVault retirements (#16513) - useAsyncOperation.examples.md showed api.saveSettings() in two examples, an API #16484 removed (api.ts:247 records the removal). Both now use apiClient.saveSettings() from utils/ApiClient, which is live, and the AFTER example gains the import a reader would copy. - security.secretsManager.scopeUser / scopeSession went unreferenced when #16486 dropped those scopes from the Scope select to match ChatSecretScope. Removed from all 11 locales, brace-matched to the secretsManager block so the unrelated secrets.vault.scopeSession survives -- asserted per file, as is each file re-parsing as JSON. No dynamic key construction reaches them: every reference in the component is a literal $t('...scopeGeneral') form. - SecretsManager's virtual-list wiring had no test. The new spec covers the seam that was uncovered -- the FILTERED set reaching displayedSecrets, list mode positioning each row from the virtual item's offset, grid mode rendering the same set unpositioned -- not useVirtualList's own maths. 3/3 pass locally (worktree vitest, now that #16912 makes that possible). * fix(frontend): gate the three admin-only surfaces the UI still offered everyone (#16494) Each of these let a non-admin see and attempt an action the backend already refuses. Verified against the backend before changing anything, because it sets the severity: api/feature_flags.py has Depends(require_admin) on 8 of 8 routes, POST /api/settings/telemetry has Depends(check_admin_permission) (the GET is public on purpose), and api/themes.py gates install and uninstall the same way. So these are confusing dead-ends, not privilege escalation -- no data was reachable, only 403s. - SettingsView: the Feature Flags tab button and its section, gated on userStore.isAdmin. - TelemetrySettingsPanel: the toggle only. The state it shows comes from the public GET, so everyone keeps seeing whether telemetry is on; only an admin is offered the control that changes it. - ThemeManagerView: upload and uninstall. The listing GETs are open, so the installed-theme list stays visible. The issue's fourth item needed no change: #16442 merged and #16426 is closed, and SecretsManager.vue already gates both the infrastructure-hosts category and its quick-add button on userStore.isAdmin. v-if with the existing userStore.isAdmin pattern, matching App.vue and UsageView; no v-permission, since each of these is a plain admin/non-admin binary. No new UI strings, so no locale changes. vue-tsc --noEmit over autobot-frontend: 0 errors. * refactor(slm-frontend): finish #16239's consolidation — move ToolsView onto useFleetTools (#16704) ToolsView is the view FleetToolsTab was extracted from, and it never adopted the composable its own extraction produced: #16702's merge-conflict reconciliation kept main's independent copy of this one file. The split was an artifact, not a decision -- nothing anywhere records an intent for these two views to diverge on shared logic. - activeTool/loading/error/result, the tool-specific state, nodes and selectedNodeDetails, selectTool/closeTool now come from useFleetTools(), with the two names this view has always used renamed at the destructure (shellCommand: ansibleCommand, runShellCommand: runAnsibleCommand) so FleetToolsTab and the composable are untouched. - The local runRedisCommand/runAnsibleCommand bodies are gone; they were the composable's own implementations inlined, down to the redis-node fallback and the message keys. - runNetworkTest, runHealthCheck, serviceAction and getServiceLogs stay here -- this view alone offers them -- rebuilt on the shared runTool/requireInput the way FleetToolsTab already builds getServiceLogs. 666 -> 545 lines. The three remaining rawRequest calls are exactly the three tools this view alone offers. serviceAction keeps a local normalisation: its fallback message interpolates {action} and runTool's fallback key cannot take a parameter, so a non-Error throw still reads as it did before. Verification, stated exactly: NOT run locally. autobot-slm-frontend's node_modules in this machine's main checkout is mode drw-r--r-- -- no execute bit, so nothing inside it is reachable and neither vitest nor vue-tsc can run for the SLM app from any tree. The existing src/views/ToolsView.test.ts and FleetToolsTab.logsError.test.ts therefore run in CI only for this change. The SLM duplication count drops with this change. SLM_MAX_DUP_LINES ('2945') lives in .github/workflows/duplication-guard.yml, which this PR does not own; the gate passes when under the pin and prints the figure to lower it to. * fix(frontend-tooling): select changed .spec.ts files for the pre-push vitest run too (#16912) A fifth cause of the same defect, found while pushing this branch: the hook selected changed test files with `grep -E '\.test\.ts$'`, so a changed `.spec.ts` was never run. 51 of autobot-frontend's 246+51 test files are named that way -- including the SecretsManager virtual-list spec added in this branch, which is exactly how it surfaced. A test file that is silently not selected is the same failure as a check that could not run: nothing in the output distinguishes it from a clean pass. * docs(changelog): add the fragment for the admin-only UI gates (#16494) * test(frontend): mount ThemeManagerView with its store, and cover the admin gate it gained (#16494, #16912) The #16494 gate made this view call useUserStore(), and its existing test mounted it with no pinia active: "[pinia]: getActivePinia() was called but there was no active Pinia". That is my regression and CI caught it -- but it was reproduced locally in seconds rather than through a CI round trip, which is what #16912 in this same branch is for. The store is mocked with a per-test admin flag rather than backed by a real pinia, matching the pattern the SecretsManager spec uses, and the regression is turned into the coverage the gate did not have: the listing stays visible to a non-admin while the file input and the uninstall button do not, and an admin sees both. Full autobot-frontend suite after this fix: 297 files, 3408 passed, 1 skipped, 0 failed, in 149s -- run in the worktree.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Sub-issue of #16465 (see #16481 for why this is a separate issue rather than a PR branched directly off #16465: a branch made via
gh issue developauto-closes its issue on merge regardless of the PR body's wording, and #16465 as a whole isn't fully delivered until #16245 resolvesSettingsPanel.vue).#16240 made the core backend
/api/settingsroutes admin-only. Five frontend paths into those routes turned out to have zero real callers (#16465). This PR covers the four fully retirable with parity evidence. Per dead-code-review discipline, checked git history + exhaustive caller grep for each:useApi.ts'suseSettingsApi— already marked@deprecatedfile-wide with a GH#7446 audit note ("zero active callers... do not add new callers"). Superseded: rebasing onto 2b founduseApi.tsitself already retired whole-file by chore(frontend): retire useApi.ts, the deprecated useApiWithState composable family (#15025) #16424 (batch 2b); this PR's own hunk against it is dropped, nothing left to do here.SystemRepository.ts'sgetSettings/updateSettings/getBackendSettings/saveBackendSettings— zero callers anywhere (grepped bothautobot-frontendandautobot-slm-frontendfor the/backendpiece specifically; no equivalent exists for it anywhere). Retired.services/api.ts'sgetSettings/updateSettings/saveSettings— retired with parity evidence:utils/ApiClient.ts'sgetSettings()/saveSettings()are the live, mounted, superseding implementation (AgentSettingsPanel.vue,BatchApiService.ts).services/CacheService.ts'swarmup()— zero callers, and never functional in the first place (only ever wrote a placeholder into the local cache, never actually fetched any of itscommonEndpoints).Not touched:
components/settings/SettingsPanel.vue. Unlike the other four, its capability isn't fully served elsewhere — #16245 (open) already owns relocating its detailed system-health and Redis-service-management parts into the SLM before removing this component's entry points. Added a doc comment cross-referencing #16245/#16465/#16481 rather than force a premature retirement.Investigating the
services/api.tsparity evidence surfaced a live violation of #16465's own AC2 ("No frontend path calls the core/api/settingsroutes from a surface a non-admin can reach"):BatchApiService.ts'sfallbackChatInitialization()calledApiClient.getSettings()unconditionally on every/chatpage load — reachable by any signed-in user — and the result was never read anywhere by its only real caller (ChatInterface.vue'sinitializeChatInterface). Fetched only to be discarded, and always failing for non-admins since #16240.Checked the other plausible violation (
AgentSettingsPanel.vue, at/agents/registry, also not an admin-gated route) and found it's not actually a violation:AgentRegistryView.vue:354already gates<AgentSettingsPanel />behindv-if="isAdmin"client-side, so it never mounts (and never calls the endpoint) for a non-admin.What Changed
useApi.ts'suseSettingsApi, was already retired whole-file by chore(frontend): retire useApi.ts, the deprecated useApiWithState composable family (#15025) #16424 before this branch rebased past it) with cross-referenced comments explaining why.SettingsPanel.vue: added a doc comment cross-referencing tech-debt(frontend): move SettingsPanel's detailed system health and Redis service management into the SLM #16245/tech-debt(frontend): five settings API paths into the now admin-only /api/settings routes have no callers #16465/retire useSettingsApi/SystemRepository/services-api.ts/CacheService.ts dead settings paths #16481, left otherwise untouched.BatchApiService.ts: removed thegetSettings()call fromfallbackChatInitialization()(its result was never consumed) and the now-unusedsettingsfield fromFallbackResults; removedloadChatInitData()entirely (a separate, unrelated dead method with the same admin-gated call, zero real callers beyond its own test's mock).SystemRepository.shape.test.ts(settings-methods-removed assertions, matching the file's existing#5214precedent for this exact pattern),api.integration.test.ts(removed the now-obsolete "Settings API Integration" describe block),ChatInterface.test.ts(removed the orphanedloadChatInitDatamock stub and thesettings: { data: {} }field from everymockInitializeChatInterface.mockResolvedValue()call, matching the realFallbackResultsshape).Closes #16481
Refs #16465
Verification
autobot-slm-frontendfor the/backend-specific claim), confirming zero remaining references outside the files changed here.AgentRegistryView.vue'sv-if="isAdmin"gate directly to rule out a false-positive AC2 violation before touching anything there.BatchApiService.ts'ssettings/user_preferencesresult all the way toChatInterface.vue'sinitializeChatInterfaceto confirm it was never read, before removing the call rather than rerouting it.node_modules) — CI is authoritative for frontend type-checking and tests.Model Used
Claude Sonnet 5