Repository navigation
fix(frontend): make a worktree able to verify frontend work, then use it on three leftovers (#16912, #16513, #16494, #16704) - #17321
Merged
Conversation
…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.
…ecretVault 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).
…d 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.
…w 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.
… 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.
Contributor
|
Warning Review limit reachedNext included review available in 32 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: mrveiss/AutoBot-AI/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (23)
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 |
Contributor
✅ SSOT Configuration Compliance: Passing🎉 No new hardcoded values of either class — Known backlog in |
7 tasks done
…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 was referenced Sep 23, 2026
frontend: three leftovers from the settings-path and SecretVault retirements (#16484, #16486)
#16513
Closed
mrveiss
added a commit
that referenced
this pull request
Sep 23, 2026
…#17142, #16324) Rebased onto 3131039. Three merges landed within the hour (#17314, #17321, #17328) and moved every one of these. REACH FLOORS. hooks-path-override 6536 -> 6538 (measured 6938) and audio-extension-allowlist 5450 -> 5752 (measured 6152). The first is the sixteenth re-pin and 6536 was measured correctly barely an hour ago -- it did not survive three merges. The second had never tripped before today and trips now for the same reason. That is #17142's argument as a data point rather than an argument: a 400-file allowance on a ~6900-file tree is spent faster than a branch can be reviewed, so a floor pinned correctly at measurement time is already stale at merge time. DUPLICATION, MAIN SCOPE. 11723 -> 11707. Sixteen lines of slack, and the gate exits 0 while it sits there, so a fall in duplication is invisible and a later PR can spend it. That is #16324. DUPLICATION, SLM SCOPE: DELIBERATELY UNCHANGED, and this is the part worth reading. A figure of 2918 was reported for it from the guard's run on #17321's merged head. That run predated #17314, which had already lowered the pin from 2945 to 2831. Applying the reported number would have moved the pin from 2831 to 2918 -- RAISING a ratchet, which is the one direction it must never go, and it would have re-licensed 87 lines of duplication while looking like housekeeping. Re-measured here instead, on the actual merged tree: main scope: 11707 duplicated lines, 672 clones, 4288 files -> pin 11707 SLM scope: 2831 duplicated lines, 123 clones, 798 files -> pin 2831, exact Both figures come from running the detector with the workflow's own flags, not from reading a report. The report was accurate about its own tree and wrong about this one, which is the entire hazard of carrying a measurement across a merge. Refs #17317, #17142, #16324
mrveiss
added a commit
that referenced
this pull request
Sep 23, 2026
…t carries (#17317) (#17318) * fix(provision): stop a long ansible line from replacing the failure it carries (#17317) Reported from a live provisioning run: backend : Install filtered backend requirements fatal: [...]: FAILED! => {"changed": false, "cmd": [".../pip3", "install", ...], "msg": "\n:stderr: ER Error: Separator is found, but chunk is longer than limit Two failures stacked and the second hid the first. `pip install` failed, and ansible reported it the way it reports everything: ONE `fatal:` line holding the whole task result as JSON, with pip's entire stderr inside it. That line was past asyncio's default 64 KiB StreamReader limit, so `readline()` raised `ValueError: Separator is found, but chunk is longer than limit`. Nothing caught it, the generator died mid-run, and that ValueError stood in for the pip error it had just swallowed. The operator was shown the reader's failure instead of the provisioning failure, and the real cause was never written down anywhere. THE PART THAT DECIDES THE FIX: `readline()` DESTROYS the line before raising. CPython's implementation clears `self._buffer` on LimitOverrunError and re-raises as a bare ValueError, so catching it recovers nothing however careful the handler is -- verified directly, `read()` afterwards returns 0 bytes. My first attempt did exactly that and its own test caught it. So the reader now uses `readuntil(b"\n")`, which raises LimitOverrunError with the buffer INTACT and `consumed` pointing at how much is readable. The head is kept, the remainder of that line is discarded so the next read starts on a clean boundary, and the yielded line carries a marker so a cut line is never mistaken for a whole one. An over-long line is truncated and reported -- never dropped, never fatal. A line too big to read is still evidence. Also: the spawn now passes `limit=PIPE_LINE_LIMIT` (10 MiB, env-backed and clamped). Raising the constant without passing it to create_subprocess_exec would have left the 64 KiB default in place, which is the version of this fix that looks right and changes nothing. Mutation-verified, each against the real stdlib rather than a mock -- the bug lives in what asyncio raises when its buffer is exceeded, so the buffer has to be exceeded: back to readline() -> 4 FAILED drop limit= from the spawn -> FAILED ..._spawned_with_that_limit skip the discard -> FAILED ..._no_fragment_..._leaks_as_its_own_line That last one is worth its own note. Removing the discard does not crash and does not spin -- it yields the unread tail as an extra blank line between the truncated line and the next real one, quietly feeding the progress parser a line ansible never emitted. Only a shape assertion catches it, and my first pass at these tests did not. WHAT THIS DOES NOT DO: it does not fix the pip failure. That error is still unknown, because it was destroyed before anyone could read it. This change is what makes the next run say what actually went wrong. The reader moved to its own module, services/playbook_output_stream.py. playbook_executor.py is at its grandfathered ceiling and may not grow, and the rule is split rather than raise -- it came out at 1402 against a 1406 ceiling, so the ceiling drops to 1402 in both places that record it. It is also the better seam: this is stream decoding and the executor is orchestration. Closes #17317 Refs #17038 * chore(ratchets): re-pin four counters against the merged tree (#17317, #17142, #16324) Rebased onto 3131039. Three merges landed within the hour (#17314, #17321, #17328) and moved every one of these. REACH FLOORS. hooks-path-override 6536 -> 6538 (measured 6938) and audio-extension-allowlist 5450 -> 5752 (measured 6152). The first is the sixteenth re-pin and 6536 was measured correctly barely an hour ago -- it did not survive three merges. The second had never tripped before today and trips now for the same reason. That is #17142's argument as a data point rather than an argument: a 400-file allowance on a ~6900-file tree is spent faster than a branch can be reviewed, so a floor pinned correctly at measurement time is already stale at merge time. DUPLICATION, MAIN SCOPE. 11723 -> 11707. Sixteen lines of slack, and the gate exits 0 while it sits there, so a fall in duplication is invisible and a later PR can spend it. That is #16324. DUPLICATION, SLM SCOPE: DELIBERATELY UNCHANGED, and this is the part worth reading. A figure of 2918 was reported for it from the guard's run on #17321's merged head. That run predated #17314, which had already lowered the pin from 2945 to 2831. Applying the reported number would have moved the pin from 2831 to 2918 -- RAISING a ratchet, which is the one direction it must never go, and it would have re-licensed 87 lines of duplication while looking like housekeeping. Re-measured here instead, on the actual merged tree: main scope: 11707 duplicated lines, 672 clones, 4288 files -> pin 11707 SLM scope: 2831 duplicated lines, 123 clones, 798 files -> pin 2831, exact Both figures come from running the detector with the workflow's own flags, not from reading a report. The report was accurate about its own tree and wrong about this one, which is the entire hazard of carrying a measurement across a merge. Refs #17317, #17142, #16324 * fix(provision): route the second playbook reader through the same guard (#17317) Review of this PR found the extraction had left a live twin. api/infrastructure.py `_stream_process_output` ran the identical unguarded `readline()` against a pipe spawned without a `limit=`, so the default 64 KiB applied. It is reached from POST /api/execute and runs the same pip-heavy provisioning playbooks -- setup-ai-stack.yml, setup-npu-worker.yml, provision-fleet-roles.yml -- that produce the oversized `fatal:` JSON line this PR exists to survive. Its failure mode was the worse of the two. `readline` clears the buffer and raises a bare ValueError; `_run_playbook`'s broad `except Exception` catches it and reports "Internal server error". So the endpoint answered with strictly less than the original bug report, which at least surfaced the ValueError text. Both halves are needed and neither is sufficient: the shared iterator cannot salvage a line the spawn already capped at 64 KiB, and a raised limit changes nothing while the reader still uses `readline`. Both are asserted, and the reader assertion is written against the BEHAVIOUR (`process.stdout.readline()` absent, `iter_pipe_lines` present) rather than the helper's name, so renaming `_stream_process_output` cannot silently retire the check. Also corrects two stale references to the pre-extraction name `_iter_pipe_lines` left in a comment and a test docstring. * fix(ratchets): pin the hooks reach floor mid-window, not at its bottom (#17317, #17142) Sixteen re-pins of this floor, six in the last two days, and #17142 concluded the growth allowance is too small. The measurement says the allowance is not the cause and raising it would not have helped. The floor has two bounds. verify_floor needs population - floor <= skips + growth (401), so floor >= 6535. completed() needs floor <= what the guard finishes, and skips=1 is this file alone, so that ceiling is population - 1 = 6935. The legal window is 6535..6935, four hundred wide. Every previous re-pin used floor = population - growth, which lands on the very bottom of that window. Slack is then always exactly growth against an allowance of growth + 1, leaving ONE file of headroom no matter what growth is set to -- doubling growth to 800 would re-pin the floor 400 lower and leave the same single file. That is the treadmill, and it is a property of the formula rather than of the number. 6736 is population - 200: 201 files of headroom before the allowance is breached, 199 files of shrink before completed() is. It is also a stricter floor than 6538 rather than a looser one, because the floor asserts how much of the tree the guard actually reached; only the gap check cares about the distance. Main measures 6936, confirmed by two independent branches rather than asserted: #17323 adds 3 counted files and CI read 6939, #17330 adds 2 and read 6938. Counted additions in flight total +22, and #17327 alone (+14) would have breached the previous 6538 four times over.
5 tasks
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
Four frontend issues, one batch. I took #16912 first on purpose: it is the one that says a frontend change in a worktree has no local verification at all, and until that was false I could not honestly verify the other three. Fixing it turned the rest of this PR from "read the diff and hope" into "run it" — and then immediately paid for itself by finding two more causes of the same defect that the issue had not named.
#16912 was filed as a decision, not a task ("Directions, not a decision", three mutually exclusive options, no acceptance criteria). Direction 1 (make verification possible) plus the cheap half of direction 3 (make a skip say so) was chosen and recorded on the issue; direction 2's CI negative-control job was explicitly not built.
What Changed
#16912 — a worktree can now verify a frontend change, and a skip says it skipped. Five causes, all five fixed:
scripts/hooks/post-checkoutlinksautobot-frontend/andautobot-slm-frontend/node_modulesto 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; a real install in the worktree always wins; a dangling link is replaced.autobot-slm-frontend/.gitignorepinnednode_modules/— a directory-only pattern that does not match a symlink, so the link above showed as an untracked file. One character.tools/git-hooks/pre-pushreports both frontend checks through the existingcould_not_run()whennode_modulesdoes not resolve. The vue-tsc branch saidWARN ... skippingand exited zero; the vitest branch had no else at all and fell through in silence.src/test/dependency-floor-reporter.tscould never load — it importedcreateLoggerfrom@/utils/debugUtils, which re-exports from@autobot/ui, whose index pulls in.vueSFCs that fail to compile in the reporter-loading context; and it default-exported an instance where vitest callsnew. Either alone takes the whole run down before a single test executes.vitest.config.tsonly registers this reporter whenCIis unset, so it broke local runs exclusively.pre-pushselected changed tests withgrep -E '\.test\.ts$', so a changed.spec.tswas never run — 51 of this app's test files are named that way, including the one added in this PR. That is how it surfaced.#16513 — three leftovers.
useAsyncOperation.examples.mdmoved off the removedapi.saveSettings()onto the liveapiClient.saveSettings();security.secretsManager.scopeUser/scopeSessionremoved from all 11 locales; a test added forSecretsManager's virtual-list wiring.#16494 — three admin-only surfaces gated. Feature Flags tab and section, the telemetry toggle (not its public state display), theme install/uninstall (not the listing). Existing
v-if="userStore.isAdmin"pattern, nov-permission— each is a plain admin/non-admin binary. No new UI strings, so no locale changes.#16704 —
ToolsViewmoved ontouseFleetTools. 666 → 545 lines. The three remainingrawRequestcalls are exactly the three tools this view alone offers, now built on the sharedrunTool/requireInput.Verification
Every command below was run in a git worktree, which is the point.
npx vue-tsc --noEmit -p tsconfig.app.json(autobot-frontend)npx vitest run SecretsManager.virtualList.spec.ts dependency-floor-reporter.test.tsnpx vitest run <spec>with no--reporteroverrideCOULD NOT RUN ... this push is NOT verified, andthe 1 selected test file(s) did not run— which also proves the.spec.tsselectorvue-tsc: 0 errors,running vitest on relevant tests: src/components/security/__tests__/SecretsManager.virtualList.spec.ts,vitest: all relevant tests passbash scripts/hooks/post-checkout HEAD HEAD 1npx vitest --versionresolves through themsecurity.secretsManager,secrets.vault.scopeSessionuntouched, all 11 land on the same key countfeature_flags.py8/8 routesDepends(require_admin);POST /settings/telemetryDepends(check_admin_permission);themes.pyinstall+uninstall both gatedNot verified locally, stated rather than implied:
autobot-slm-frontend/node_modulesat modedrw-r--r--— no execute bit, so nothing inside is reachable and neither vitest nor vue-tsc can run for the SLM app from any tree.src/views/ToolsView.test.tsandFleetToolsTab.logsError.test.tsare therefore CI's to run for this change. This is machine state, not repo content, and it was left alone rather than silently repaired in a tree this PR does not own.SLM_MAX_DUP_LINES(2945) lives in.github/workflows/duplication-guard.yml, which this PR deliberately does not touch — it is held by another branch. The gate passes when under its pin and prints the figure to lower it to.Risks
pre-pushnow blocks where it used to warn. A tree wherenode_modulesdoes not resolve fails the frontend checks instead of skipping them. That is the intent, and the escape is named in the output (AUTOBOT_PREPUSH_ALLOW_UNRUNNABLE=1) rather than--no-verify, which would switch off every other check too. After change 1 the condition should be rare.npm installin the main checkout is immediately visible to every worktree — normally what you want, but it means a broken install there breaks them all at once. The hook never writes into the main checkout.console.*is introduced.secrets.vault.scopeSessionkey before writing.autobot-frontend/src/types/generated/api.tsis not touched by this PR.Model Used
Claude Opus 5 (1M context)
Issue Link
Closes #16912
Closes #16513
Closes #16494
Closes #16704
Changelog fragment
changelog/unreleased/16494-admin-only-ui-gates.md— the admin gating is the one user-visible behaviour change here; the rest is tooling, internal refactor and an i18n cleanup.Checklist
CLAUDE.mdSecretsManager's virtual list; refactor(slm-frontend): finish #16239's consolidation — move ToolsView.vue onto useFleetTools #16704 is covered by the existingToolsView.test.ts, which CI runs (see Verification)useAsyncOperationexamples now name a live API; hook behaviour is documented inline where it is implementedmain