Plan 76 milestone A, the third row. 23 handlers decided admin access inline,
all in two files, in two shapes.
18 were the mid-route gate this milestone already built a door for:
current_user_id = get_jwt_identity()
user = User.query.get(current_user_id)
if user.role != 'admin':
return jsonify({'error': 'Admin access required'}), 403
which is four lines to say require_admin_user(), and carried the two-line
identity lookup with it. Migrating them closes both rows at once: the identity
census drops 87 -> 69 and the controller-boundary baseline loses 18 persistence
crossings (528 -> 510), with no new entries.
It also fixes a latent 500: where the check omitted the `not user or` guard, a
token naming a deleted user hit `None.role` instead of answering 403.
The other 5 are `not user.is_admin and app.user_id != user.id` — admin OR
owner, which is a different question. Those keep their shape and only converge
the truth source from `role != 'admin'` to `is_admin`; routing them through
can_access_app would silently widen access, since that also honours resource
grants.
The migration initially left `current_user_id` referenced but unbound in four
places. Two were caught by tests; the other two (create_manual_app,
upload_app_archive) have no coverage and were caught by an AST scan for names
used-but-never-assigned. Worth remembering that a mechanical removal needs the
scan, not just the suite.
Gate: -k "system or apps or app_ or host_snapshot or manual_app or upload",
304 passed / 13 skipped; all seven ratchets green (52 passed).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone G. 114 top-level class definitions share 50 names across
partials, so those classes have no owner: every definition applies, merged
per-property in main.scss import order, and editing one file changes only the
properties that file happens to win.
`.empty-state` is the worked example the audit named. Three partials define it —
components/_empty-state.scss (flex column, min-height 200px), _cards.scss (card
background, solid border, radius), _users.scss (padding, colour) — and the
compiled sheet carries five competing top-level rules. What renders takes its
background and *dashed* border from one, its padding and display from another,
and its colour from a third. No author wrote that element.
Ratcheted rather than merged, on purpose. Reproducing the cascade's computed
result has no byte-identical-output proof available (unlike the @keyframes
sweep, where deleting seven dead copies left the CSS unchanged to the byte), so
it needs eyes on the affected pages — plan invariant 9, no repo-wide mechanical
rewrite without behavioural tests.
The ratchet counts DEFINITIONS, not names: counting names would let an
already-shared class spread to a fourth and fifth partial without moving the
number, which is the failure mode worth stopping. Mutation-checked both ways —
adding a definition of an existing shared class and introducing a brand-new
shared name each fail it, and both pass on revert.
Wired into `npm run lint`.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone G. A @keyframes name is global and last-definition-wins across
every partial main.scss pulls in, so a duplicate name is not wasted bytes — one
copy is silently dead, and which one depends on import order.
`spin` was defined 8 times. The build already emitted exactly one, so seven were
dead: deleting them left the compiled CSS byte-for-byte identical (same content
hash, 958878 bytes), which is the proof rather than an argument. Three of the
seven also differed — `to`-only rather than `from`/`to` — so anyone editing
those was changing nothing.
`pulse-dot` was the one that actually mattered: components/_uptime defined it
with a scale (1 -> 1.2) and pages/_services defined it opacity-only, and
_services is imported later, so the uptime dot has been running the services
animation. Renamed the uptime one to `uptime-pulse-dot`; service-detail keeps
the shared `pulse-dot` it already got.
**Visual change**: the uptime status dot now scales as its own stylesheet always
said it should. Nothing else moves.
`pulse` (identical twice) and `drawer-fade`/`drawer-slide` (identical in
_docker and _file-manager) are consolidated into base/_utilities.scss, so
neither page owns the other page's animation.
Ratchet: the boundary check now scans every .scss partial and fails on any
@keyframes name defined more than once. 66 unique names, zero duplicates.
Verified non-vacuous: an injected second `spin` fails it, revert passes.
Gate: npm run build clean, npm run lint 0 errors / 953 warnings, boundary check
green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone E2, continuing 0bfcf69c. Ratchet 16 sites / 12 files -> 11 / 10.
Dashboard's remote-metrics poll and its socket-down HTTP fallback, ImportWizard's
active-import poll, and TestSandbox's run poll plus its open-log fan-out.
TestSandbox's log refresher fanned out one request per open log per tick and
returned nothing, so a tick reported done before any of them had come back. It
now returns a single Promise.all, which is what the in-flight guard needs to
mean anything for a fan-out.
That leaves 11 raw setInterval sites, and they are deliberate:
- 5 are clock ticks with nothing to overlap and nothing to fetch — an elapsed
timer, a countdown, a board refresh pulse, two wall clocks.
- 3 are the socket-fallback pollers in useMetrics, useContainerStatus and
useDeployJobStream, driven by imperative startPolling()/stopPolling() from
socket handlers with a 3s grace period. Expressing that as a declarative
`enabled` is a real restructure of each hook and they only poll when the
socket is already down.
- 2 belong to the serverkit-gui extension, whose source is the sibling repo;
its frame loop already carries its own in-flight guard.
- 1 is ServerGui's user-pausable capture interval.
The ratchet keeps all of them at exact counts, so the reasoning above has to be
revisited rather than forgotten if one changes.
Gate: npm run build clean, npm run lint 0 errors / 953 warnings, npm test 72
passed, boundary check green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone E2, continuing 89c3896e. Ratchet 22 sites / 15 files -> 16 / 12.
DoctorPanel's fleet sweep and ScannerTab's malware scan were imperative
pollers: a function started an interval, and the interval cleared itself once
the job reached a terminal status, with a second clearInterval in an unmount
cleanup to catch the case where it never did. Both now hold the job id in state
and let usePolling own the schedule —
const pollSweep = useCallback((jobId) => setSweepJobId(jobId), []);
usePolling(tick, SWEEP_POLL_MS, { enabled: Boolean(sweepJobId) });
which is what makes the visibility pause and the in-flight guard apply to them
too. A job poller is the shape that most wants both: it runs while someone
watches a spinner, and its endpoint is doing real work.
Also ContainersTab's stats poll and log tail, and OnboardingWizard's status
poll (its 1s elapsed-time clock stays a plain interval — nothing to overlap).
Two `useRef`s and one `clearInterval` in an unmount cleanup went with them.
Gate: npm run build clean, npm run lint 0 errors / 953 warnings, npm test 72
passed, boundary check green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone E2, continuing f9657d38. Ratchet 27 sites / 18 files -> 22 / 15.
Terminal's three tab components (file logs, journal, processes), ServicesTab's
unit log tail, and the GUI extension's synthetic desktop. All were the
auto-refresh tail shape: poll only while the toggle is on, never fetch on mount
from the interval effect.
These are the sites where a hidden tab mattered most — a log tail at 3s left
open in a background tab is the single most wasteful poller in the panel, and
every one of them also re-ran on `lineCount`/`priority`/`sort` changes via a
dependency array with `// eslint-disable-line` on it. That silent restart is now
explicit: the tail's own load effects still handle those, the poll handles the
repeat.
Four `intervalRef`s and one `logIntervalRef` went with them.
Gate: npm run build clean, npm run lint 0 errors / 953 warnings, npm test 72
passed, boundary check green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone E2, continuing 3ac566cb. Ratchet 31 sites / 22 files -> 27 / 18.
QueueDetail and QueueOperations each fired two requests per tick. Their ticks
now return a single awaited Promise.all, because the in-flight guard only knows
a tick has finished when the promise it was handed settles — a bare `.then()`
inside the callback reports done immediately and lets the next tick start on top
of a request still in flight, which is the exact overlap the hook exists to
prevent.
ServerDetail and LinkPanelForm keep their mount-time load as its own effect;
the poll is `immediate: false` so nothing fetches twice.
Gate: npm run build clean, npm run lint 0 errors / 953 warnings, npm test 72
passed, boundary check green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone E2, continuing f21abe7a. Poller ratchet 41 sites / 31 files
-> 31 / 22.
Migrated: Jobs, Monitors, MonitorDetail, DeliveryLog, Deployments, Databases,
VulnerabilityTab, and the service-detail Logs/Metrics/Overview tabs.
The trap flagged in the plan bit on four of them, so it is worth restating: the
interval effect's dependency array was doing double duty as the "reload because
the query changed" trigger —
useEffect(() => {
load(); // <- reload on filter change
pollRef.current = setInterval(load, POLL_MS);
return () => clearInterval(pollRef.current);
}, [isAdmin, load]); // <- load() is a useCallback of the filters
usePolling holds the callback in a ref precisely so it does not restart on every
render, which means a naive swap silently drops that reload and nothing fails to
compile. Each of those four keeps an explicit `useEffect(() => load(), [load])`
alongside the poll, with `immediate: false` so mount does not fetch twice.
Six magic interval literals became named constants while their lines were being
touched (LOG_REFRESH_MS, METRICS_REFRESH_MS, SCAN_POLL_MS, DEPLOY_POLL_MS,
ENGINE_POLL_MS).
Not migrated, and not mechanical: useMetrics, useContainerStatus and
useDeployJobStream are socket-fallback pollers driven by imperative
startPolling()/stopPolling() calls from socket event handlers, with a 3s grace
period before falling back. Expressing that as a declarative `enabled` flag is a
real restructure of each hook, and they only poll when the socket is already
down. Left for when they are next touched.
Gate: npm run build clean, npm run lint 0 errors / 953 warnings (one below the
954 baseline — four orphaned useRef imports removed), npm test 72 passed,
boundary check green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone F3 (the blob half) plus the C4-interim dead-file deletion the
audit called out.
The six-line "save this to disk" anchor ritual was pasted into 14 components,
and the copies had drifted in two ways that decide whether the download
actually happens:
- only 3 of 14 appended the anchor to the document. Chrome dispatches a click
on a detached anchor; Firefox has not always done so, which makes "the
download button does nothing" a browser-specific bug that never reproduces
for whoever wrote it.
- most revoked the object URL on the very next line after click(). The download
starts asynchronously, so revoking immediately can cancel it — showing up as
a flaky empty-or-missing file rather than a clean failure.
utils/downloadBlob.js does both correctly (append, click, remove, revoke on a
later task) and accepts either a string or an existing Blob, which is what
FleetCapacityPanel needs since its CSV arrives from the API already blobbed.
Also deletes src/services/wordpress.js: 494 lines, zero importers, and a stale
duplicate of the live src/services/api/wordpress.js that the ApiService barrel
actually wires in. Verified by import graph and a clean production build.
Ratchet: the boundary check now fails on any `.download =` assignment outside
the helper — a zero baseline, not a shrinking one, since every site is
migrated. Verified non-vacuous: an injected anchor fails it, revert passes.
Gate: npm run build clean, npm run lint 0 errors / 954 warnings (unchanged),
npm test 72 passed, boundary check green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
a7d5a0b9 replaced hand-shaped 500s in docker.py and notifications.py with
unexpected_response(), which rolls the session back itself, so those two routes
no longer call db.session.rollback() directly. Controller-boundary persistence
crossings 519 -> 517; no new entries.
Caught by the full per-file sweep rather than the focused gate, which did not
include this ratchet.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Names why the sniffing row is ratcheted rather than migrated: file_service owns
12 of the 23 sites and has eight callers, so converting it to raise ahead of the
envelope decision would leave two contracts on one service — the failure mode
the plan exists to remove.
Adds a status table for the five drift/bug rows and the five ratchets now
holding them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone C, drift/bug row 3 — the ratchet only. 23 sites choose an
HTTP status by searching an error message:
status = 403 if 'denied' in result.get('error', '').lower() else 400
That is what `{'success': False, 'error': '<prose>'}` forces on a caller: the
service knew the failure was a denial, discarded it, and the route rebuilds it
from the sentence. The status is then coupled to the wording — rephrasing
"Access denied: path not in allowed directories" to "You do not have access to
this path" turns a 403 into a 400 and nothing fails, not a test, not a lint
rule.
The migration is NOT done here on purpose. Converging these means services
raising typed errors instead of returning result dicts, and file_service alone
(12 of the 23 sites) has eight callers. Converting it to raise while its other
callers still expect a dict leaves two contracts, which is the disease rather
than the cure — that is milestone C's envelope decision, sequenced after A and
B by the plan's own execution order.
So the ratchet lands first, per second-wave rule 1, to stop the pattern
spreading in the meantime. Ceiling 23. Counts a conditional choosing between
two HTTP status codes whose test inspects string content (`in`, `.lower()`,
`.startswith()`); a status chosen from a typed exception, an enum, or a boolean
is not counted, since that is the thing being asked for. Verified non-vacuous:
an injected site fails it and it passes again on revert.
Gate: tests/test_status_sniffing_ratchet.py, 8 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Corrects the audit's framing of the fix: the in-flight guard is not what stops
the schedule stacking — arming the next tick from the previous run's completion
is. Also flags the trap for the remaining 41 migrations: several sites use the
interval effect's dependency array as an out-of-band reload trigger, which
usePolling drops silently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone E2, drift/bug row 4. 45 hand-rolled setInterval sites across
35 files, with `visibilitychange` handled in exactly one of them and an
in-flight guard in exactly one of them — the documented poller-stampede shape.
Two failures every hand-rolled `setInterval(load, 5000)` shares:
- it starts a request every 5s whether or not the previous one returned. A slow
endpoint does not slow the poller down, it makes it stack; each extra request
makes the server slower, which makes the next overlap more likely. This is
what a DevTools capture of /domains showed as a wall of pending rows.
- it keeps polling a tab nobody is looking at, where browsers throttle timers
unevenly anyway, so it is not even a reliable refresh.
utils/pollScheduler.js holds the rules as a plain object so they are testable
without a DOM (the repo has no jsdom — this follows formState.js/useForm).
hooks/usePolling.js is the React binding, and useServerQuery gains
`refetchInterval` built on the same scheduler, so there is one implementation
rather than a second one for query users.
Two design points worth stating, since both differ from what the sites did:
- the next tick is armed by the previous run *finishing*, not by setInterval.
With setInterval the gap is measured tick-to-tick regardless of how long the
work took, so slow work silently turns a 5s gap into no gap. The in-flight
guard therefore protects the out-of-band paths — refresh() and the visibility
catch-up — rather than the schedule, and the test says so instead of implying
a guard the schedule can no longer exercise.
- the callback is held in a ref, so an inline arrow re-created each render does
not restart the timer. That is the usual reason a hand-rolled poller runs far
faster than its stated interval.
Migrated four as reference implementations, per the plan's "migrate a
representative flow before broader adoption": DeployPill (which had worked all
of this out by hand and loses ~20 lines), MonitorsSummary, appdetail/LogsTab,
and ProcessListPanel. MonitorsSummary keeps its `refreshKey` prop as an explicit
out-of-band refresh rather than folding it into the schedule.
Ratchet: check-frontend-boundaries.mjs gains LEGACY_POLLERS with exact per-file
counts, now 41 sites in 31 files, so every future cleanup must shrink it and a
new bare setInterval fails lint. Genuine non-fetching timers (clock ticks,
countdowns) are listed too rather than pattern-matched away — they are harmless
but the exact count is what makes the ratchet trustworthy.
Gate: npm test (72 passed, 11 of them new), npm run lint (0 errors; 954
warnings, one fewer than the 955 baseline), npm run build clean, boundary check
green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Keeps the three ways the audit's measurement was off, and names the 56
swallow-and-answer-200 handlers as still open so B cannot read as finished.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone B, drift/bug row 4. 43 API handlers answered HTTP 500 from
their own `except Exception`, so the global 500 handler never ran — and that
handler was the ONLY caller of error_log_service.record_error(source='backend').
Every one of those crashes was absent from /monitoring/errors, and the page read
empty precisely because someone had "handled" them.
35 of the 43 also returned str(exc) to the caller. Two returned the full
traceback (templates.py, which also print()ed it), and several logged with a
locally-minted uuid4 'ref' that correlated with nothing but a log line.
The recording side of the 500 handler moves to app/error_reporting.py, so it is
importable rather than reachable only by not-catching:
record_unexpected(exc) -> logs, rolls the session back, records, returns
the request id
unexpected_response(exc) -> the above plus the documented crash body
The 500 handler now calls the same pair, so there is one recorder rather than
one recorder and 43 routes that quietly opted out of it.
Caller-visible change on those endpoints: the body is now the documented
{'error': 'Internal server error', 'status': 500, 'code': 'internal_error',
'request_id': ...} instead of the exception text. That is the point — an
unexpected exception's text is written for whoever reads the log and routinely
carries connection strings, absolute paths, and SQL. The request id is the part
a caller can act on, and it matches the row in the error log. No frontend code
read the 'ref' or 'trace' keys this drops (checked).
Two sites keep their own wording and call record_unexpected instead: sso.py's
login failures should not read as a generic crash on the login page.
docker.py's bulk endpoint keeps its per-item `results` — the caller's half-done
work survives, only the exception text is dropped.
Ratchet: tests/unreported_crash_census.py + its test, held at **0**, not a
legacy baseline to shrink. It counts blanket handlers that answer 500 without
calling either reporter, and is scoped to app/api — a service catching an
exception and returning a value is making a domain decision, not declaring an
HTTP crash. Verified non-vacuous: an injected handler fails it, and it passes
again on revert.
Measured and deliberately still open: 56 API handlers swallow a crash and
answer 200. A failure reported as success is worse, but converging it is
milestone C's response-envelope work, not this row.
Gate: tests/test_unexpected_error_reporting.py (11, including the two that pin
why the old shape lost the crash), test_unreported_crash_ratchet.py (9), and
the 66 test files covering every touched module: 1105 passed, 7 skipped, 1
pre-existing Windows-only failure (test_fleet_doctor's socket.gaierror).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
fe02ce8e removed queue_bus.py's private _current_user(), which was the only
code using the model. The automatic dead-import sweep kept it because the word
"User" also appears in a docstring two lines into _ensure_group_accessible.
Gate: -k queue_bus, 26 passed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Also records the one non-mechanical part of the remaining F3 work: a
.then(onOk, onErr) chain cannot be repointed at copyToClipboard as-is, because
the helper resolves false instead of rejecting and the error callback would
silently stop running.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone F3, drift/bug row 5. 33 call sites in 25 files reached for
navigator.clipboard.writeText directly, against 4 files using the shared
copyToClipboard/useClipboard helpers.
This is a fix rather than a tidy-up: navigator.clipboard is undefined in an
insecure context, and ServerKit's SSL is optional by policy, so on an
HTTP-served panel every one of those copy buttons did nothing. The failure was
silent at most sites — a bare fire-and-forget writeText, or a `.catch` that
swallowed. utils/clipboard.js already carries the execCommand fallback that
makes them work; it just had almost no callers.
Two of the bypasses were local functions *named* copyToClipboard
(ServerSettingsTab, Downloads) — the same shadowing shape as the backend's
private get_current_user() helpers, and the reason the call sites read as
though they were already correct. ServerSettingsTab's was a hand-rolled
useClipboard (copy + toast) and now uses the hook; Downloads' is renamed
copyCommand since it also owns per-command copied state.
Behaviour preserved per site, with two deliberate improvements the helper's
contract allows:
- state that was set unconditionally (`copied` flags, keyed copy markers) is
now set only when the copy actually succeeded
- CreateTableModal's "this browser will not give the page clipboard access"
notice is gone; the fallback is what it was warning about
`.then(onOk, onErr)` chains became `.then(ok => ...)` because copyToClipboard
resolves false instead of rejecting — the second callback would never have run.
The existing frontend boundary ratchet held an exact per-file legacy baseline;
it is now empty and documented as needing to stay that way, so a new direct
call fails `npm run lint`.
Gate: node scripts/check-frontend-boundaries.mjs (0 clipboard files ratcheted),
npm run lint (0 errors, 955 pre-existing warnings — unchanged), npm test
(61 passed), npm run build (clean).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Both drift/bug rows of milestone A are closed. The record keeps the four ways
the audit's own measurements were wrong, since the plan's premise is that these
numbers are the baselines everything else ratchets against:
- the API-key failure is a raised RuntimeError (a 500), not a None
- the inline population has a second, larger two-line shape (87 sites)
- a sixth _require_admin() lives in an extension with no installed live copy
- 12 of 19 private helpers were named get_current_user(), shadowing the door
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone A, drift/bug row 2 — scheduled as a fix, not a refactor,
because it is a behaviour difference shipping today.
rbac.get_current_user() is the only lookup that knows about both auth
mechanisms: it returns g.api_key_user for an X-API-Key request and falls back
to the JWT. 73 call sites answered "who is calling" with
`User.query.get(get_jwt_identity())` instead, behind 19 near-identical private
helpers (12 named get_current_user(), shadowing the real door).
The audit that scheduled this row predicted those return None for API-key
callers. Measured, it is worse: auth_required() deliberately skips
verify_jwt_in_request() once the key middleware has authenticated, so there is
no JWT context and get_jwt_identity() *raises*. Nine handlers reached through
an API-key-capable policy decorator therefore returned 500 to every API-key
caller:
bandwidth.get_app_bandwidth @viewer_required
servers.set_server_workspace @developer_required
servers.create_server @developer_required
sso.get_sso_config / update_provider_config / update_general_settings
source_connections.update_{github,gitlab,bitbucket}_admin_config
source_connections.complete_github_app_manifest
databases.execute_{mysql,pg,sqlite,docker}_query (see below)
Three service helpers had the same lookup inside a blanket `except Exception`,
which converted the raise into a silent user_id=None — the write landed and
the audit trail lost the actor (shared_resource_service, cf_ops_change_service,
secrets_webhooks).
The four databases.execute_*_query sites were a second admin check *inside* an
already-@admin_required route: dead code that always passed for JWT callers and
crashed for API-key ones. They now call require_admin_user() rather than
re-deriving the answer.
Also folds in a sixth _require_admin() found in the cloudflare-ops extension
source (29 gates, the same inverted contract as jobs.py), and fixes
serverkit-gui's blueprint at its sibling-repo source rather than in this repo's
live copy.
Ratchet (second-wave rule 1 — a migration without one regrows):
tests/identity_door_census.py counts both bypass shapes by AST, direct and the
two-line `uid = get_jwt_identity(); User.query.get(uid)` variant. The ceiling is
87, all of them on @jwt_required()-only routes where reading the JWT is correct
today; the ratchet's third assertion holds the API-key-capable population at
exactly zero, so migrating one of those routes to a policy decorator without
fixing its lookup now fails. Verified non-vacuous: an injected bypass fails both
assertions and passes again on revert.
The first wave's controller-boundary baseline is regenerated — this removes 74
persistence crossings (593 -> 519), a debt reduction with no new entries.
Gate: tests/test_api_key_identity.py (7, each red before the fix),
test_identity_door_ratchet.py (10), test_api_controller_boundaries.py (4), plus
the 65 test files covering every touched module: 1024 passed, 10 skipped.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 76 milestone A, drift/bug row 3. Five API modules had grown a private
_require_admin() with *three mutually incompatible* return contracts:
jobs.py -> returns the user; None means DENIED
dns_cutover.py -> returns None on success; a response tuple means denied
waf.py -> same as dns_cutover
manifests.py -> (user, None) on success, (None, response) on denial
themes.py -> same as manifests
jobs.py's is the exact inverse of dns_cutover.py's, so a guard copied between
the two reads identically at the call site and fails open. Nothing in the code
told you which contract you were looking at.
Replace all five with rbac.require_admin_user(), which raises a typed
PermissionDeniedError the global ApplicationError handler already renders. A
gate that raises has no return contract to get backwards.
Two convergences ride along, both toward the stricter shared behaviour:
- manifests.py gated on `user.role != 'admin'` where the others used
`is_admin`. These are the same predicate today (is_admin is defined as
role == ROLE_ADMIN), so no route changes status, but there is now one
truth source rather than two.
- none of the five checked is_active, so a deactivated admin still passed
them; @admin_required has always checked it. The shared door checks it, so
these 25 call sites now reject deactivated admins like every other
admin route.
The unknown-user case keeps its 403 'Admin access required' rather than
becoming a 401/404: a non-admin caller should not learn from the status code
whether the account behind the token still exists.
Gate: tests/test_jobs.py, test_themes.py, test_waf.py, test_dns_cutover_api.py
(82 passed) and -k manifest (152 passed).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G1, the remaining 33 files. Every raw subprocess call site in
backend/app that can go through a door now does. What is left is what
should be:
8 app/utils/system.py the doors themselves
8 database_service mysqldump|gzip, gunzip|mysql, pg_dump|gzip,
gunzip|psql — a pipe is two processes
7 build/docker/db_sync/ftp streaming output, gzip pipes
1 log_service tail -f
2 serverkit-analytics sibling repo; see below
Extensions were migrated in BOTH trees. builtin-extensions/ is the source
and backend/app/plugins/ the live copy, compared by normalised hash;
test_builtin_extension_drift stays green. serverkit-email has both sides
tracked, serverkit-ftp and serverkit-cloud-provision only the source.
serverkit-analytics (2 calls) is deliberately untouched: it has no source
in builtin-extensions/ at all, and backend/app/plugins/serverkit-analytics
is a gitignored materialised copy. Editing it would be a change that
cannot be committed and is overwritten on the next sync — that fix belongs
in the serverkit-analytics repo.
Exit-code semantics are preserved wherever they carry meaning, with a
comment at each site, because they are the thing a mechanical migration
destroys: clamscan 1 = INFECTED, grype 1 = vulnerabilities FOUND, yara
1 = matches, cron_runner 127 = never launched. Those read
result['returncode'], never result['success'].
run_checked gained two things real call sites needed: it yields when the
caller directs the streams itself (writing straight to a log file), rather
than adding capture_output on top and raising ValueError; and text/errors
pass through, so site_importers/ssh keeps decoding with errors='replace'
instead of gaining a UnicodeDecodeError on a bad byte.
A regression this migration introduced, caught by the suite and fixed:
install_profile_service._docker_usable() is deliberately TRI-STATE —
None means the probe could not run, because inside LXC the docker client
is usually present while the daemon never started, and callers "must not
treat a broken probe as proof of absence". Writing it as
run_checked(...)['success'] turned an exec failure into False, i.e. "docker
is not usable" — a fact nobody established. That is the §A violation this
plan exists to remove, reintroduced by the migration meant to prevent it.
test_probe_honesty_doctor caught it. Every other run_checked(...)['success']
caller was then audited; the rest are genuinely two-state, and their
originals also returned False on a missing binary.
Seven more slips of the same shape were caught before running by an AST
check for `x = run_checked(...)` followed by `x.stdout/.stderr/.returncode`
— a dict read as an object, which raises only on the failure path, so
happy-path tests stay green.
api_controller_boundary_baseline.json: pure deletion. Routing api/docker.py
through DockerService removed the last 6 subprocess crossings, so the
`subprocess` category is gone from the API layer entirely.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G1, second migration. docker_service was the heaviest file in the
census: 49 raw subprocess call sites -> 2. Census 258 -> 211.
DockerService.run now SITS ON run_checked instead of re-implementing it, so
G3's runner and G1's door are one thing rather than two. Adds
DockerService.run_compose() for the compose subcommands, whose argv head is
whatever _get_compose_cmd() resolved (`docker compose` v2 vs `docker-compose`
v1) and therefore cannot go through run().
The 2 survivors are the genuine streaming Popen sites — the live container
log stream and the compose-up transcript. A captured call cannot do those.
run_checked gained two things this migration needed:
- `stderr` (the raw stream) as a separate key from `error` (the verdict, None
on success). git and `docker exec` write to stderr and still succeed, so
folding the stream into the verdict would invent failures. exec_command
needs the stream; every other caller wants the verdict.
- merge_stderr=True, for `docker logs` and `compose logs`. A container writes
its log to both streams; the `result.stdout + result.stderr` concatenation
this replaces loses the interleaving, so a crash tail landed above the lines
that led to it.
TIMEOUTS ARE UNCHANGED, deliberately. run_checked defaults to 60s and every
docker call in this file is unbounded today; inheriting the default would cap
`compose up --build`, `docker pull` and `docker build` at a minute and turn
working deploys into timeouts. Every migrated site keeps exactly what it has,
with timeout=None passed explicitly at the long ones and six tests pinning it
(mutation-checked: removing it from pull_image fails the test). Bounding
docker's long operations is a real improvement and a deliberate behaviour
change — its own commit, not a side effect of this one.
Two regressions caught by the suite and fixed here:
- Dropping the try/except around the compose-version probe let an exception
escape into the caller, surfacing as `compose up` exit_code -1 instead of a
v1 fallback. run_checked converts the exec failures it knows about, but this
probe runs before everything else in the compose path and must not be able
to raise at all. Guard restored, with a test.
- test_docker_protection hand-rolled a CompletedProcess stand-in with only
returncode and stdout. Once list_containers went through run_checked,
reading .stderr off it raised AttributeError, the caller's blanket except
swallowed it, and the test saw an EMPTY CONTAINER LIST rather than an error.
Retargeted onto the §G7 kit. A private stub is only correct until something
reads the attribute it forgot.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds the 2026-08-18 duplication-audit table to plan 76: for each door the
first wave built, how much legacy traffic still walks around it, measured
rather than estimated.
The rows marked drift/bug are the reason this is not filed as tidy-up —
they are behaviour differences shipping today, not repetition. Inline
`User.query.get(get_jwt_identity())` returns None for API-key callers
because only rbac checks g.api_key_user; locally-swallowed exceptions skip
the 500 handler and never reach the error tracker; status codes chosen by
sniffing error strings; pollers with no in-flight guard; raw
navigator.clipboard on HTTP-served panels, where SSL being optional by
policy means those copy buttons simply do not work.
Two rules recorded with it: every row gets a ratchet before its migration
starts (a migration without one regrows), and the drift/bug rows are
scheduled like fixes rather than "migrate when touched".
Scope boundary noted so the three plans do not overlap: the services-layer
subprocess/docker/nginx/SQL-exec doors are plan 75 Round 2 (§G); model
mixins, secret handling, status vocabulary, realtime convergence and
shared infra utilities are plan 77.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G1 — the door and the ratchet. The 33-file migration is NOT
done; see below.
run_checked(cmd, *, privileged=False, timeout=60, input=None). Around
backend/app's raw subprocess calls sit 220 copies of capture_output/text,
56 hand-rolled TimeoutExpired handlers, 19 FileNotFoundError handlers and
1,164 literal {'success': False, 'error': ...} dicts. Each is a place an
exec failure becomes a false fact.
It keeps the distinction those handlers lose: returncode is None when the
command never ran. Exit 1 means the command answered "no"; no exit code
at all means nobody answered. A caller rendering "not installed" can
finally tell them apart — plan 74's outage in one field. Unprivileged
calls get the same argv[0] resolution as privileged ones, because the
sbin outage is not something that only happens when you ask for root.
The ratchet is tests/raw_subprocess_census.py + RAW_SUBPROCESS_CEILING,
wired into the existing "Audit Raw Subprocess Patterns" CI job and
enforced by test_raw_subprocess_ratchet.py. The grep steps already there
check eight hand-listed files for three known-bad patterns, so a new file
was invisible to the audit entirely; the census counts every call site
under backend/app. It is AST-based because a grep for subprocess.run also
matches comments, docstrings and `except subprocess.SubprocessError`, and
a miscounting ratchet reads green either way. test_the_ceiling_is_not_stale
fails if the ceiling drifts more than 10 above reality — a stale ceiling
silently re-authorises everything a migration removed.
First migration: cron_service, 13 raw calls -> 3. Its `crontab -l` read
and Popen-with-stdin `crontab -` install were written out five times each
and are now two helpers over run_checked. Census 267 -> 258.
The other 32 files are untouched: docker_service (49), database_service
(24), git_service (18), security_service (14), python_service (13) are
the bulk. The door plus the ratchet are what make that incremental
instead of one unreviewable diff.
test_utils_system.py loads system.py under its own importlib spec, so
@patch('app.utils.system.<name>') binds to whichever module object is in
sys.modules at patch time — not necessarily the one run_checked closes
over. shutil/subprocess are singletons so the file's existing patches are
safe; these are the first tests to patch a module-level function of
system.py, and they use patch.object(_module, ...) instead.
BASELINE_COUNT 3383 -> 4081. Most of that is pre-existing drift (the
ratchet only fails on a drop, so an un-bumped floor never complains), not
tests added here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G3. Three services kept private `_docker()` wrappers around the
same `subprocess.run(['docker', ...])` call, and the three had drifted
into three different return contracts:
- db_admin_sso_service returned a dict and mapped FileNotFoundError to
"Docker not found";
- db_config_tuner_service returned a dict that collapsed every failure
into str(e), so a caller could not tell "install docker" from "the
daemon is wedged";
- image_update_service returned a raw CompletedProcess, so a caller
reading result['success'] against it got a TypeError, not an answer.
DockerService.run(args, timeout=60) is now that call, with one shape —
{'success', 'output', 'error'} — that keeps the three distinctions §A
cares about apart: docker said no, docker is absent, docker did not answer
in time. DockerService.available() asks the daemon rather than the binary,
because "CLI installed, daemon down" is the state the callers check for.
The three named seams stay (each service's tests stub them); only the
bodies go. image_update_service's two digest probes move to the dict
shape, and drop the blanket try/except that was there to absorb the
CompletedProcess path's exceptions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G5 + §G6.
§G5 — app/services/db_exec.py. engine × host dispatch (mysql/postgres
crossed with native/docker) was written three times. Only
managed_db_user_service delegated fully to DatabaseService's executors;
the other two grew private copies that had already drifted —
db_process_service's docker-postgres branch built its own psql argv and
never set ON_ERROR_STOP=1, so psql exited 0 for a statement the server
had rejected and the caller was told it worked.
db_exec owns the dispatch and nothing else; the four executors stay in
DatabaseService. The three _exec_sql seams remain (tests stub them) and
now delegate. An unrecognised engine is reported, never quietly routed to
mysql, where it would surface as a syntax error blamed on the caller.
docker_mysql_execute/docker_pg_execute gain machine_readable, which is
the single flag db_config_tuner_service was keeping a whole private copy
of the dispatcher to add. The MYSQL_PWD env block, pasted six times in
database_service, becomes _mysql_env(): it carries a real rule — the
password goes in the environment, never on the argv — and a rule in six
copies only holds in the copies someone remembered.
§G6 — adopting doors Round 1 built:
- config_store's write is now atomic (tmp + os.replace). The §F4
consolidation deliberately kept the copies' truncating write and noted
atomicity would land here once; this is that once. A write that dies
mid-dump leaves a truncated file, and every load path treats a corrupt
config as "use the default" — a config silently reset.
- uptime_service._save_history was `except Exception: pass` over that
truncating write, so a full disk destroyed the history and reported
nothing while the panel kept rendering a figure from a file that had
stopped updating. It now returns and logs the failure.
- unit_is_active(): `systemctl is-active` was re-inlined in four services
and every copy collapsed "could not ask" into "not running". The shared
probe is tri-state; None is not False.
- slug: preview_service and saved_view_service re-implemented slugify's
regex, and saved_view re-implemented unique_slug's collision loop too.
unique_slug gains start= so saved_view keeps generating the handles it
always has.
Checked rather than assumed: the plan lists file_integrity_service as
silently losing data on a full disk. It does not — it already used tmp +
os.replace and does not swallow the error. It is the pattern followed
here, not a victim; only uptime_service had the silent loss.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G2 + §G4, together because they are the same physical files and
G4 is built on G2's helper.
§G2 — write_privileged_file(). The write-a-root-owned-file sequence was
pasted 15+ times in two competing forms: run_privileged(['tee', path],
input=...) and a raw subprocess.run(['sudo', 'tee', ...]). The second
hardcodes sudo instead of asking _needs_sudo(), so it fails on a panel
already running as root in a container with no sudo binary — plan 74's
outage shape. All core-service tee sites now go through one helper that
owns the sudo decision, the result shape, and honest failure: a missing
tee or a timeout reports the failure instead of being swallowed (§A).
§G4 — NginxService.write_vhost()/read_vhost(). environment_domain_service
re-implemented write → symlink → nginx -t → reload with raw sudo calls,
and nginx_advanced_service wrote the vhost with a plain unprivileged
open() — PermissionError against a root-owned sites-available, with no
config test and no reload. Both now go through NginxService.
The rollback is the part no copy had: a vhost that fails nginx -t used to
stay on disk and symlinked, so the next unrelated reload (a cert renewal,
another site going live) failed and the blame landed on whatever
triggered it. write_vhost restores the previous content, or removes a
file that did not exist before, and never reloads a config that failed
the test.
SITES_AVAILABLE/SITES_ENABLED were independently declared in four files,
so an NGINX_CONF_DIR override — the env var NginxService honours, and what
tests redirect — silently did not apply to the other three. One
declaration now.
Mutation-checked: disabling the rollback fails two tests.
Also lands unit_is_active() in app/utils/system.py; its adopters are in
the following commit (§G6).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Plan 75 §G7. The suite's hand-rolled stubs were entrenching the very
duplication the plan removes: _FakeProc pasted byte-identical in four
modules, ~25 fake_run closures re-implementing argv dispatch, and zero
tests patching the wrappers — every stub targeted raw subprocess inside
one service module, so each new service test recreated the raw seam.
tests/subprocess_stub.py is that seam. Tests script argv → result; when
G1 moves services onto run_checked() the retarget happens here once
instead of in twelve files.
Two properties are deliberate:
- An unscripted command raises instead of inventing returncode=0. It is a
BaseException for the same reason SbinPathError is — services wrap
subprocess in `except Exception`, so an AssertionError would be eaten
by exactly the handlers the kit exists to test.
- Stubbed execs are still sbin-guarded. Patching subprocess removes the
§B2 runtime guard, so the kit re-applies it; popen_guard grows a public
guard_argv() as the one door for that check.
Matching normalises away a sudo prefix and argv[0] absolutisation, so one
scripted rule matches on a root box, a sudo box and a dev box alike.
Mutation-checked: removing the guard call fails the guard test.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
