Two changes in one file (kept together to avoid hunk surgery):
- probe honesty (plan 75 §A3): os.getloadavg failure produced
(0, 0, 0) — a fake zero load that also silenced the load threshold
check. Now (None, None, None) and the threshold check is skipped
when undetermined.
- consolidation (plan 75 §F4): get/save_config join the ConfigStore
conversion (a3fe9bf5), the last of the five byte-identical copies.
Two dishonesty directions, one rule (plan 75 §A3/A4): a probe answers a
value or None, never 0 when undetermined — and a real 0 must survive.
Live paths normalized every missing reading to 0 while the history path
stored NULL for the same probe: agent counters an LXC agent can't
determine rendered as a perfectly idle box; load average on unsupported
platforms read 0.00 (and silenced the load alert); CPU freq read 0 MHz;
an empty history window summarized as '0.0 avg'. The mobile summary
called a method that does not exist and rendered 0/0/0 permanently.
One unavailable network counter raised uncaught and froze the entire
live broadcast.
Reverse direction: falsy traps (x if x else None) collapsed a genuinely
idle 0.0 into 'no data' in rollups, averages, and the history model.
Fleet: KPI tiles read '0.0 ms latency' / '100% success' with no data;
never-probed servers exported serverkit_server_up 0 to external
alertmanagers — the up-series now emits only for determined
connectivity. get_all_metrics is per-section fault-tolerant so one
failed probe can't blank the other six.
Five services shipped byte-identical save_config copies and as many the
same get_config skeleton — the next fix (an atomic write, a better
error) would have had to land in every one. load_json_config /
save_json_config own the mechanics; the path, the default, and any
behaviour around the write stay per-service.
Byte-compatible with the copies: plain open(..., 'w'), no locking, no
atomic rename — adding those here would be a behaviour change smuggled
in with a consolidation; if they're ever wanted they now land once.
Divergent services (template cache invalidation, storage secret
merging, email_relay's DB backing, build_service's leaner return dict)
keep their own methods. monitoring_service converts separately — it is
mid-edit on another branch of this plan.
The static sbin guard reads literal argv lists — 164 of 288 raw
subprocess sites (57%). The other 43% (variables, f-strings,
concatenation) is where plan 74's outage class can still hide: a call
that works in dev (PATH has sbin) and dies under the panel unit (PATH
has none).
An autouse conftest fixture now routes every subprocess.Popen the suite
makes through GuardedPopen, which sees the actual argv at call time and
refuses a bare name that resolves under the current PATH but not under
one with the sbin dirs removed. A binary simply absent on the test
machine still raises the honest FileNotFoundError — the guard judges
PATH shape, never machine contents. The refusal is a BaseException:
this bug class ships because blanket except handlers convert exec
failures into false facts, so an AssertionError would be swallowed by
exactly the handlers it hunts.
Coverage is 'whatever the suite exercises' — honest, measurable, and
growing for free with every new test. Full suite green with the guard
live (3791 passed; 2 pre-existing load flakes pass in isolation).
Every model with a JSON-in-Text column hand-rolled the same try/except
json.loads pair — get_details/get_context/get_headers/get_payload and a
dozen more, each a place the next fix would have to land N times. The
mixin owns the mechanics (empty-is-default, corrupt-is-default so a
hand-edited row never 500s an endpoint, optional shape check,
falsy-on-write); the per-model parts — accessor name, column, default —
stay per-model.
Conversion is behaviour-preserving: identical fallbacks, identical
on-write empty forms (NULL vs '[]' vs '{}'). Deliberately NOT converted:
accessors that let json.loads raise on corrupt data (collapsing those
would be a behaviour change, not a consolidation), and inline parses in
to_dict (per-entity serialization choice). A handful of 'except
Exception' reads narrow to the dominant (JSONDecodeError, TypeError) —
the only exceptions json.loads raises for str input.
The static scan sees literal argv lists only — 164 of 288 raw subprocess
sites (57%), and only the hardcoded names in SBIN_COMMANDS. A green run
was easy to read as 'no bare sbin calls exist'; it never meant that. Say
so in the module header and in the failure message itself, and point at
the runtime layer (popen_guard) that watches actual argv.
_backup_unverified_check returned ok for any verify_level that was not
literally 'none', so a checksum mismatch — positive evidence a backup may
not restore — rendered as 'Latest backup verified (listed).', and an
unreadable archive read as merely 'not verified yet'. verify_run_tier1
was already honest; the summary boundary was not, the same shape as the
drill badge. fail is now reserved for verification that ran and came
back bad, warn for 'couldn't check', and ok is positively earned.
plan 75 §A1 — and it was a live bug, not the hardening pass I expected.
_drill_badge() returned 'ok' for any status that was not literally 'failed'. A
drill skipped for lack of scratch space records 'skipped_no_space' AND stamps
last_drill_at, so it fell through every branch and Doctor reported:
"A recent restore drill proved this backup restores."
about a drill that never executed. It also self-perpetuated: each skip
refreshed last_drill_at, so a policy permanently short of scratch space could
never even go stale. It would report proven forever, having proven nothing —
and "short of scratch space" is the normal state of a full disk, which is
exactly when you most need the backup.
The drill service was careful (its own comment reads "loud skip, never a silent
pass") and the frontend renders an amber pill. The honesty was lost in the one
function that summarises it, which is the one Doctor reads.
Fixes:
- 'ok' must now be positively earned: only last_drill_status == 'success'
reaches it. Adds 'skipped' and 'unknown' states, so an unrecognised future
status cannot reintroduce this by falling through.
- staleness is evaluated only for a drill that actually succeeded, since it is
a property of proof rather than of attempts.
- Doctor renders both new states as warn with what was and was not proven.
- the skip path now notifies. BackupAlertService.on_drill_result already mapped
skipped_no_space to a failed outcome, but only the exception path ever called
it, so that branch was unreachable and a skipped drill told nobody. A log
line is not a notification.
Mutation-checked: with the fix reverted, the 5 bug-targeting tests fail and the
9 pinning existing behaviour still pass. 307 green across backup/doctor/restore
(a 422 in test_raw_infra_authz did not reproduce on re-run and is absent from
the baseline selection — the documented intermittent, not this change).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
plan 75 §B1. Two helpers did overlapping jobs with different semantics, and the
one that says nothing about privilege sat one autocomplete from the one that
does. nginx_advanced_service reached for it, so `nginx -t` ran unprivileged and
by bare name — it could neither read root-owned config nor find /usr/sbin/nginx,
and reported every config as invalid.
Renamed rather than folded. Folding into run_privileged(privileged=False) would
change sudo semantics at all six call sites — `sudo -n dig` fails where plain
dig worked, if sudoers does not permit it — and the failure this fixes was
choosing the wrong helper, not the helper's behaviour. The rename removes the
ambiguity with zero behavioural risk; the return type (dict, not
CompletedProcess) stays as callers expect.
`run_unprivileged(['nginx', '-t'])` reads as obviously wrong. `run_command` did
not. The old name was ambiguous in a second way: PythonService has an unrelated
method of the same name, which keeps it.
Six call sites, three test modules. Two patch targets were written as strings
('app.utils.system.run_command', setattr(mod, 'run_command')) so a word-boundary
rename skipped them and they would have failed at runtime, not import.
No alias: an alias leaves the second door open, which is the thing being closed.
run_command was never exported through plugins_sdk, so there is no public SDK
surface to break; a third-party extension importing it gets a loud ImportError
rather than silent misbehaviour. A guard test asserts the name stays gone.
417 tests green across monitor, ssl, dns, nginx, firewall and the guards.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
plan 75 §C. Bundled extensions exist twice on disk: builtin-extensions/<ext>/
backend is the source, backend/app/plugins/<ext> is what the loader imports. A
fix applied to one and not the other ships a repo that reads correct and a panel
that runs the old code.
Not hypothetical — the plan-74 dovecot fix had to be made twice, caught only by
remembering to look.
Written while drift is zero (21 file pairs), so this locks in a clean invariant
rather than repairing a dirty one. It only stays this cheap while that is true.
Compares by CRLF-normalised hash, since the repo has mixed line endings, and
only pairs present on BOTH sides: cloudflare-ops and localkit ship source with
no materialised live copy and serverkit-git uses a different layout, all
legitimate. Most live copies are gitignored, so a fresh clone sees fewer pairs
than a dev machine (serverkit-email's predates the ignore rule and is tracked).
test_drift_check_is_not_vacuous stops that from quietly reducing the
parametrised check to zero cases while still reporting green.
Also fails when a file exists on only one side — drift before anyone edits it.
Both assertions mutation-checked: appending a byte to the live dovecot copy
fails the hash test, adding a one-sided file fails the membership test, and the
tree is green again once reverted.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
plan 75 §F1. A duplication audit over 4103 functions found 73 structurally
identical groups; the sharpest instance was self-inflicted.
capacity_service.format_size renders bytes for sentences ('needs about 512 MB'):
'unknown' for None, adaptive precision. host_inventory_service carried a
byte-for-byte copy of it, added in the plan-74 round, written to avoid an import
cycle that cannot exist — app/utils/formatting imports nothing and is already
used by 9 modules.
Moved it to that leaf module. capacity_service re-exports the name so existing
callers keep working; host_inventory_service imports it.
The audit's headline was misleading and worth recording: of the seven other
_format_size methods, six are one-line adapters to the canonical format_bytes
and system_service.get_size is another. Only models/db_snapshot.py holds a real
hand-rolled reimplementation. "Four competing implementations" was a tool
artifact; reading the code gives one hand-rolled copy, one prose variant, and
one duplicate of that variant.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The enable() and remove_rule() guards reason about ports. These two paths never
touch the SSH rule and lock the operator out anyway:
block_ip(<your own address>) the rule is untouched; your source
address is rejected before reaching it
set_default_zone(<zone without ssh>) firewalld re-homes every interface with
no explicit zone; the rules did not
change, the zone around them did
check_ip_block() refuses when the target covers a live SSH peer, the caller's
own HTTP client address, or loopback — nginx forwards to 127.0.0.1, so blocking
it takes the panel down with the session. CIDRs are matched with `ipaddress`,
because 73.244.0.0/16 is the same lockout wearing a wider mask.
Peers come from `ss -Htn state established`, filtered to the sshd ports; `who`
is empty for sessions with no utmp entry. Verified against the live host, whose
only established SSH connection was 146.190.213.37:22 <- 73.244.95.52:59085:
active_ssh_peers() -> {'73.244.95.52'}
block 73.244.95.52 -> refused
block 73.244.0.0/16 -> refused (covers the same peer)
block 127.0.0.1 -> refused (loopback)
block 198.51.100.7 -> allowed
caller_ip is read in the API layer from request.remote_addr and passed in, so
the service stays free of request context.
check_default_zone() asks whether the target zone permits ssh via
--list-services / --list-ports before switching. A failed `ss` probe yields no
peers rather than fabricating conflicts, so a broken probe cannot block every
request. Consistent with the existing guards: undeterminable SSH port refuses,
an unenforcing firewall is a no-op, force=true is the operator accepting it.
/block-ip and /zones/default answer 409 with the conflicting addresses.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
enable() guards the moment the firewall comes up; this guards every moment
after it. With ufw active and denying by default, deleting the rule that admits
SSH severs the very session issuing the delete, and there is no way back in.
Deleting by NUMBER is the sharp edge: the caller passes an opaque index, so the
only way to know what is about to go is to resolve it against
`ufw status numbered` first. Removal by port spec is checked too.
Coverage-aware rather than a blanket block — a duplicate SSH rule may still be
deleted, because another allow rule keeps the port open. Application profiles
(`[1] OpenSSH`) are resolved through `ufw app info`, verified against a real
host: _ports_in('OpenSSH') -> {22}, and '22/tcp (v6)' -> {22} rather than
parsing the address-family suffix as port 6.
deny_port() is implemented as remove_rule(), so it inherits the guard —
denying the SSH port and deleting its allow rule are the same outcome.
Consistent with the enable() guard: an undeterminable SSH port refuses instead
of assuming 22, an inactive firewall is a no-op (nothing is being enforced and
enable() preflights separately), DEFAULT_INPUT_POLICY=ACCEPT short-circuits,
and force=true is the operator accepting the risk.
firewalld: removing service=ssh or the SSH port is checked against
--list-services / --list-ports so other coverage still permits the removal.
DELETE /rules and POST /deny-port answer 409 with the offending rule;
POST /rules/removal-preflight lets the UI warn before the click.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`ufw enable` normally prompts "Command may disrupt existing ssh connections.
Proceed?". The service must use `--force enable` because nothing here runs on a
terminal — and --force suppresses exactly that prompt. So the panel had removed
ufw's only safeguard and offered a one-click lockout of a remote box.
Found on a live host: ufw installed and inactive with one staged rule,
`ufw allow 25/tcp`, and sshd on 22. ufw defaults to deny-incoming, so enabling
would have severed the only way back in. Verified against that host:
ssh_ports() = [22]
ufw allowed ports = [25]
enable('ufw') -> blocked_by ssh_lockout
check_ssh_lockout() reads the live port from `sshd -T` (authoritative: resolves
Includes, Match blocks and compiled-in defaults) and compares it against
`ufw show added`, the rules that will exist the moment it comes up. Named
application profiles are resolved via `ufw app info` rather than assumed.
An undeterminable SSH port REFUSES rather than falling back to 22 — guessing in
the permissive direction is the lockout this exists to prevent. A DEFAULT_INPUT
_POLICY of ACCEPT short-circuits it, since nothing can be locked out then.
enable(force=True) is the operator accepting the risk; never set by default.
POST /firewall/enable answers 409 (well-formed request, conflicting state) with
the offending port and remediation. GET /firewall/ssh-preflight lets the UI warn
before the click instead of reporting the lockout after it.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The ufw fix repaired every run_privileged caller centrally, but three sites
reached subprocess by other routes and were broken the same way. All are
"$PATH omits sbin + the panel runs as root, so nothing prepends sudo to
resolve it".
- run_command() got the same argv[0] resolution as privileged_cmd. It does no
privilege escalation, so nothing else would ever find these commands.
- nginx_advanced_service.test_config() ran `nginx -t` (/usr/sbin/nginx) via
run_command, so it raised FileNotFoundError and reported every config
invalid. It also returned valid=True unconditionally, ignoring the exit code
entirely — so when it DID run, a broken config passed. Now run_privileged and
the returncode is checked. reload_nginx() likewise reports a failed reload
instead of swallowing it.
- dovecot_service.get_status() detected dovecot with `which`, which searches
$PATH only and false-negatives on /usr/sbin/dovecot, then read the version by
bare name under a shared except. Now is_command_available() + a separate try,
so a version read that fails cannot unsay "installed". Patched in both the
builtin-extensions source and the app/plugins live copy.
- SecurityService.get_lynis_status() inferred presence solely from exec'ing
`lynis --version`; presence and version are now separate questions.
Adds test_sbin_command_guard.py so this cannot come back: an AST scan fails CI
on any raw subprocess call using a known sbin-resident command by bare name
(it caught the dovecot site), plus per-command proof that both helpers resolve
under the exact PATH the panel unit ships.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
UFW installed cleanly, dpkg said ii, the install notification said so — and
/security/firewall still rendered "No Firewall Installed". fail2ban worked on
the same box, which is the clue: fail2ban-client lives in /usr/bin, ufw in
/usr/sbin, and the panel unit ships
PATH=<venv>/bin:/usr/local/bin:/usr/bin:/bin
with no sbin. subprocess resolves argv[0] through $PATH alone, so `ufw status`
raised FileNotFoundError. _check_ufw wrapped both questions in one try/except,
so that exception discarded an `installed` already computed as True and
returned False. Measured on the affected host:
is_command_available('ufw') = True
PackageManager.is_installed('ufw') = True
get_status() -> ufw {'installed': False}, any_installed False
FirewallTab.jsx gates its empty state on any_installed. The Enable button was
broken identically — same bare-name exec.
Three fixes, each independent:
- resolve_command() gives argv[0] an absolute path, but ONLY when $PATH cannot
resolve it, so working commands pass through byte-identical.
- probe failure can no longer unsay "installed" (same split for firewalld).
- the unit template gains /usr/local/sbin, /usr/sbin and /sbin. update.sh
re-renders the unit, so existing installs pick this up on upgrade.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A VPS resized from 1 vCPU/1 GB to 4 vCPU/8 GB with a 60 GB volume attached,
and the panel never mentioned any of it. Not a stale cache: a resize forces a
power-off, so the process and its in-memory spec cache die and the next boot
reads the truth. The gap is that nothing ever persisted the PREVIOUS numbers,
so there was nothing to compare against — and the reboot that causes the
change is what destroys the only copy of the baseline.
Adds host_snapshots: capture specs + per-filesystem inventory at boot, diff
against the last row, and emit host.specs_changed / host.storage_advisory on a
real delta. Advisories fire on state transitions only, so an unfixed fstab
entry does not nag on every restart.
Repairs a divergence this exposed: resource_tier_service measured the first of
/var/lib/serverkit,/var/lib/docker,/ while capacity_service hardcoded /. Both
now use host_inventory_service.data_path(), and local headroom reports which
mountpoint it measured.
Notifications render in the existing bell UI, so no frontend change is needed.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment explaining why scripts/ is copied into the image read
"/api/v1/servers/install.sh and /install.ps1", which reads as though the Windows
installer is served at the domain root. It is not — both live under
/api/v1/servers/. A comment whose whole job is to stop the next person hunting
for a wrong path should not contain one.
Raised by a reviewer on the PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
test_internal_error_handler_is_registered read app.error_handler_spec, a Flask
internal whose shape has moved between releases, and proved only that something
was registered. A handler can be registered and still return HTML or log
nothing, so the test could not fail for either way the guarantee actually breaks.
Now driven through app.handle_exception(), Flask's own entry point for an
unhandled exception, asserting the response is JSON and that the request was
logged. Not done by registering a route that raises: the `app` fixture is
function-scoped but wraps a session-scoped Flask object, so the route would leak
into every later test and collide on re-registration. PROPAGATE_EXCEPTIONS is
forced off for the call because TESTING=True turns it on, which re-raises before
any handler runs.
The log assertion matches this handler's own wording rather than just the path.
Flask's log_exception() already emits "Exception on /path [GET]" before any
handler runs, so a path-only assertion passes even with the handler's logging
deleted — confirmed by mutation, which is how the first version of this test was
caught being vacuous. Both mutations (return HTML, drop the log) now fail, on
different assertions.
Raised by a reviewer on the PR.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Picks up serverkit-agent 219e75e, which points the usage and --help examples at
/api/v1/servers/install.{sh,ps1} instead of the domain root. Produced by
scripts/sync-agent-installers.sh, not hand-edited — scripts/test/check-installer-drift.sh
fails on any divergence and confirms these match the canonical copies again.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
User-facing half of the registry-mirror work: an offline panel (or one with the
registry disabled) was missing LocalKit Bridge from the Marketplace because the
bundled fallback had fallen behind the published index.
The sync script and the test guard are tooling, so they stay in git log --
except for the one line about the new check, which is the part a reader of this
file would care about.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A `bundled: true` entry promises an extension that ships inside the panel, so it
carries no source/sha256 -- which means every other check in this file skips it.
test_matches_published_index only proves the catalog agrees with the registry,
not that either agrees with the code, so a bundled entry could name a version
the panel does not ship, or an extension it does not ship at all, and nothing
would notice.
Checks each bundled entry against builtin-extensions/<slug>/plugin.json. Offline
and deterministic, so unlike the published-index comparison it fails on the PR
that introduces the mismatch rather than needing the network.
Only asserted in this direction: a builtin deliberately absent from the catalog
is a product decision, not rot.
Verified non-vacuous by mutation -- claiming a wrong version and naming a
non-existent bundled slug each fail, on different assertions.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
backend/app/data/registry_index.json is a mirror of serverkit-extensions'
index.json, and nothing automated the mirror -- so every registry publish left
the panel's offline fallback stale until CI complained. That is how
`serverkit-localkit` sat missing from it: an offline panel offered a different
extension set than an online one.
Same shape as scripts/sync-agent-installers.sh: prefer a sibling checkout so it
works offline and before the registry PR is merged, else the published branch,
resolving the default branch from the API rather than hardcoding a name.
Two things it is deliberately careful about:
- It vendors the RAW index, never https://serverkit.ai/ext/index.json. The
panel fetches through that proxy at runtime and the proxy rewrites every
relative `logo` path to an absolute serverkit.ai URL, while the bundled
loader resolves relative paths itself. Vendoring the proxied copy would bake
in the wrong paths, and the tuple the tests compare (version/source/sha256)
is identical either way -- nothing would have caught it.
- It keeps the local `note` (which does not exist upstream) and writes with
indent=2/ensure_ascii=True/trailing newline, which round-trips the existing
file byte-for-byte. An in-sync run is a true no-op, not a reformat.
It refuses to write an index with no entries, so a broken or truncated canonical
source cannot blank the Marketplace fallback.
Verified: no-op when in sync; reproducing the real bug (dropping the localkit
entry) and re-running restores the file byte-identical to HEAD; repins are
reported; the empty-canonical guard exits non-zero leaving the file untouched.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`app/data/registry_index.json` is the Marketplace's offline fallback and a
mirror of serverkit-extensions/index.json on main. The registry gained
`serverkit-localkit` (v0.2.0, bundled) and the mirror was never updated, so
an offline panel would have offered a different extension set than an online
one -- and test_registry_bundled_index::test_matches_published_index failed
on every run.
Copied the entry verbatim into its published position. The rest of the file
already matched published field-for-field; this is the whole delta.
`builtin-extensions/serverkit-localkit/` ships in the panel at the same
version, so `bundled: true` is correct and the entry carries no source/sha256.
Nothing automates this mirror, so it will drift again on the next registry
publish.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The vendoring and drift-check scripts hardcoded a branch name for the fallback
fetch. That was wrong twice in one day: first `main` (the agent repo used
`master`, so the fetch 404'd), then `master` (which is being renamed to `main`).
A hardcoded branch turns any rename into a 404 that surfaces as "cannot reach
the canonical copy" rather than "wrong branch name" — a confusing way to fail
for a check whose whole job is to be believed.
They now ask the GitHub API for the repo's declared default_branch, falling back
to probing main then master if the API is unreachable or rate-limited. AGENT_REF
still pins an explicit ref (a tag, say) when you want one. Verified: the API
currently reports `master`, and it will report `main` on its own once the rename
lands — no follow-up edit here.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The vendoring and drift-check scripts defaulted AGENT_REF to `main`, which this
repo uses but the agent repo does not -- it has no `main` branch at all.
Verified: raw.githubusercontent.com/jhd3197/serverkit-agent/main/install.sh is
404, /master/install.sh is 200.
The bug was invisible locally because both scripts prefer a sibling
../serverkit-agent checkout and only fall back to fetching. It would have shown
up first in nightly CI, as "could not fetch the canonical copy" -- which reads
as an outage rather than a wrong branch name, so it is worth the comment.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
scripts/install.sh and scripts/install.ps1 are now VENDORED copies. The agent
repo owns them; this repo carries a copy only because the panel serves them off
its own disk at GET /api/v1/servers/install.sh|.ps1 and cannot link to another
repository.
Two copies of one file is what caused the second half of #101. When the agent
moved out of this monorepo it took its installer with it, and from then on there
were two -- actually four: the agent repo also carried a third, older pair under
its own scripts/ that nothing referenced. Each looked fine in isolation, nothing
ever compared them, and the panel went on serving one that downloaded from
`agent-v*` tags which exist in no repository. Unit tests are structurally blind
to this: they exercise whichever copy lives in their own repo and have no idea
another exists.
So:
scripts/sync-agent-installers.sh re-vendors from the agent repo (a sibling
checkout if present, else the published
branch), normalising to LF so this repo's
.gitattributes can apply its own EOL rule
scripts/test/check-installer-drift.sh fails, with the diff and the fix, if a
vendored copy differs from canonical
(EOL-insensitive: *.ps1 is checked out
CRLF here and LF there, which is not drift)
The drift check runs nightly alongside the release-chain verification, since
both need to reach GitHub. Both scripts run locally against a sibling checkout,
so this works offline and before an agent change is published.
Both installers carry a provenance header naming the canonical location, so
anyone who opens the vendored copy to patch it is told where to patch instead.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Also corrects the scope note, which said the agent is tagged agent-vX.Y.Z and
linked to agent/README.md -- a path that no longer exists. The agent ships from
jhd3197/serverkit-agent and is tagged vX.Y.Z there.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Issue #101 shipped because release.yml went straight from `build` to `push`
with nothing in between. The image was missing scripts/, so every containerised
panel 404'd its own enrollment URL, while the entire test suite stayed green --
pytest runs against the source tree, where the file always exists. Only booting
the artifact catches that class of bug.
scripts/test/smoke-docker-image.sh builds (or takes) an image, asserts the files
the panel reads at request time are present, boots it, and requests every route
that serves a file off disk. release.yml now builds amd64, runs it, and only
then pushes multi-arch (layers are cached, so the push reuses the build).
release-smoke.yml gets the same job on dev and PRs, and its path filters now
include Dockerfile, .dockerignore and scripts/ -- none of which used to trigger
it.
scripts/test/verify-agent-release-chain.sh covers the other half, which unit
tests structurally cannot: they stub curl, so they only ever prove the installer
is self-consistent, never that the URL it builds resolves to a real file. This
one reads the coordinates back out of the script, asks GitHub, downloads the
archive, verifies the checksum and confirms the tarball entry name the installer
moves. It runs nightly, and is also the tripwire for the reverse case -- the
agent repo changing its release naming and breaking the panel from the other
side of the repo split.
Both are plain scripts so the same check runs locally:
bash scripts/test/smoke-docker-image.sh
bash scripts/test/verify-agent-release-chain.sh
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Tracing issue #101 turned up that a panel failing to serve its own agent
installer left no diagnostic trail at all. The backend had exactly one error
handler -- the 404/SPA one -- no 500 or HTTPException handler, and nothing
anywhere configured logging, while only 2 of 100 app/api modules ever call
logger.error.
Adds:
- an errorhandler(500) that logs the unhandled exception with its method and
path and returns JSON. Flask routes here only when it is not propagating, so
pytest and the dev server still re-raise with a full traceback.
- an errorhandler(HTTPException) so framework-generated errors under /api/
(405 from the router, 413, abort()) return JSON instead of Werkzeug's HTML
page to clients that are parsing JSON. Non-API paths keep the HTML page, and
the more specific 404/500 handlers still win for those codes.
- _configure_logging(): a stderr handler with a timestamp/level/logger-name
formatter, level from LOG_LEVEL. Services across the codebase log through
logging.getLogger(__name__); with no root handler those records fell through
to logging's last-resort handler -- WARNING and above only, unformatted, INFO
dropped on the floor.
Two things the container run caught while writing this: Flask's default handler
has to come off app.logger or every record prints twice (once as %(module)s via
Flask's handler, once as %(name)s via ours, both reaching stderr through
propagation), and the level must not key off DEBUG alone -- TestingConfig sets
DEBUG=True, which would have moved the whole test suite to DEBUG logging as a
side effect. Both are pinned by tests.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The command the panel prints for enrolling a server was
`curl -fsSL <url> | sudo bash -s -- …`. In a pipeline the shell reports bash's
exit status, not curl's, so a failed download exits 0: the operator sees one
line of curl noise, the shell reports success, and nothing was installed.
Measured against a 404 -- `curl -fsSL <404> | bash -s -- --token TOK; echo $?`
prints 0; with `set -o pipefail` it prints 22.
Download to a file first and run it on success, so a failed fetch stops the
command and surfaces curl's non-zero exit. Applied to the Add Server drawer, the
server-detail connection modal and the Downloads page.
The Windows one-liner is left alone: `irm` writes an error and the pipeline
stops, so its failure is already visible.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Three changes to the endpoints that serve the fleet-agent installers.
A missing installer answered 404. That is what sent issue #101's reporter
looking for a wrong URL -- they filed it as "incorrect paths in setup" -- when
the file had simply never been shipped into the image. The status code produced
the misdiagnosis. It is now 503, the response names the path that was searched,
and the same path is written to the panel log, because the response body does
not survive the `curl -f` the enrollment one-liner uses: curl discards it and
prints only "The requested URL returned error: 404". For an endpoint consumed by
curl, the status code and the server log are the only diagnostics that reach
anyone.
The panel's release lookup pointed at jhd3197/ServerKit and filtered for
`agent-v*` tags, which exist nowhere since the agent moved to its own repo, so
/api/v1/servers/agent/version returned 503 to every agent polling for an update.
It now uses AGENT_GITHUB_REPO (env SERVERKIT_AGENT_GITHUB_REPO) and a ^v(\d…)
tag filter that skips the agent repo's malformed "v" release.
_load_installer() is shared by the .sh and .ps1 routes so they cannot drift, and
SERVERKIT_SCRIPTS_DIR overrides the resolved directory for unusual layouts.
New tests cover the endpoints, the release lookup, and -- the one that would
have caught the original bug -- a packaging invariant that parses the
Dockerfile's COPY sources and asserts every installer the panel serves is
actually copied into the image. No endpoint test could have caught it: pytest
runs against the source tree, where the file always exists.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
When the Go agent moved out of this monorepo into jhd3197/serverkit-agent, the
installers did not follow it. They still asked jhd3197/ServerKit for `agent-v*`
tags -- coordinates that exist in no repository: jhd3197/ServerKit has zero
agent-v* releases, and the binaries are published as vX.Y.Z in the agent repo.
Version discovery therefore failed outright, and the download URL was a 404.
Everything downstream was already correct and is unchanged: the asset name
serverkit-agent-<ver>-linux-<arch>.tar.gz, the entry inside the tarball
(serverkit-agent-linux-<arch>) and the checksums.txt entry names all match the
real v1.2.0 release. Only the repo and the tag prefix had rotted.
Version discovery now requires a digit after the "v". The agent repo carries a
malformed release tagged literally "v", which would otherwise parse to an empty
version and build .../releases/download/v/serverkit-agent--linux-amd64.tar.gz.
test_agent_install.sh encoded the dead scheme in its fixtures (agent-v0.3.2), so
it stayed green throughout. It now pins the release coordinates as a contract:
the repo, the /v${VERSION}/ URL scheme, the absence of any live agent-v
reference, and the checksums URL exercised through the curl stub. All four fail
against the pre-fix script.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The panel serves the fleet-agent installers at /api/v1/servers/install.sh and
/install.ps1 by reading them off its own filesystem, resolving <tree root>/scripts
four dirnames up from app/api/servers.py. In the image that is /app/scripts --
and the Dockerfile only ever copied backend/, VERSION and frontend/dist, so the
directory did not exist. Every containerised panel answered the enrollment
one-liner printed in its own Add Server dialog with a 404, and no server could
join the fleet. Verified against the published image: `ls /app` has no scripts,
and a booted container returns 404 while /api/v1/system/health returns 200.
Bare metal was unaffected -- install.sh git-clones the whole repo, and the
release tarball excludes only scripts/test/output.
Ship just the two installers: both are standalone (they source nothing from
scripts/lib), so scripts/lib, keys, the test harnesses and stage-remote.sh stay
out of the image. The .dockerignore comment claiming the build "only needs
frontend/, backend/, VERSION" was wrong and authoritative, which is a large part
of why this went unnoticed; it now states the real list.
Closes#101 (packaging half).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
app/data/registry_index.json is what the Marketplace falls back to when the
live registry is unreachable, and what the test suite reads. Seven of its ten
entries carried a sha256 that does not match the artifact their own source URL
resolves to, at the same version: serverkit-gpu's zip hashes 3872d71f..., the
bundled copy claimed c861dafa.... A panel that fell back to this file would
download the correct, untampered release and reject it as tampered, on exactly
the panels already cut off from the network. It also sat nine entries and one
version behind (WordPress 1.0.0 against a published 1.0.1).
Nothing caught it because nothing checked it: the file was hand-maintained,
had no generator, and no test referenced it. A wrong-but-well-formed digest is
indistinguishable from a right one without fetching the artifact or the
published index.
Regenerated as a straight mirror of serverkit-extensions/index.json, and the
note now says it is a mirror and must not be hand-edited. The new test compares
the pinned (version, source, sha256) tuples against the published index and
skips only when the index is unreachable, so offline is not a failure but a
reachable mismatch is. Verified it fails against the old file, naming all seven
digests and the WordPress version, and passes against the regenerated one.
Also corrects the signed-releases roadmap line I marked done earlier today: the
verification path and the consent UX shipped, but no published release actually
carries a signature and the registry schema did not even document the field, so
every first-party install still takes the unsigned-consent path.
Four boxes were unchecked for things that have shipped. Signed extension
releases with install consent landed with plan 55's ed25519 signing service
and the marketplace consent modal. WordPress is out of the tree and installs
from the Marketplace like any other extension. The Minecraft extension, the
reusable gamekit framework and the connect card are all published at v1.0.0
and carried in the registry index.
SRV-record domain support is split out as the one game-server item that is
genuinely still open — grep finds no SRV handling in the extension — rather
than leaving it hidden inside a bullet whose other half was done. The
built-ins bullet now names only what is left, since the WordPress flagship it
called out has been extracted.
Juan Denis