Fix round 1 review findings on the patch run service:
- advanceRun no longer runs every server's dispatch claim and failed-send
reset on the caller's shared short context; each gets its own fresh
patchCtx(), and a failed reset write is logged instead of discarded.
- The dispatch claim (queued/waiting_offline -> patching) now also requires
the run to still be status running with no cancelled_at, closing a race
where a tick that loaded the run just before CancelPatchRun wrote
cancelled_at could still dispatch.
- RecordPatchResult's write is now guarded on command_id too, so a late
result for a superseded command cannot land on a re-dispatched attempt.
Named patch_window.go (not patch_windows.go) since the _windows.go
suffix is Go's implicit GOOS build constraint and would silently
exclude the file on non-Windows builds.
Implements the patchrun package with a pure functional state machine for managing
patch runs. Contains no database dependencies - the services layer loads a run,
asks this package what should change, and writes changes guarded by expected status.
All 14 test cases pass, covering:
- Agent version parsing and support detection
- Concurrency limits and queueing
- Window deadlines and offline handling
- Result timeouts (ResultGrace, ManualTimeout, RebootTimeout)
- Reboot verification with boot time proof
- Run finalization logic
- Summary generation for alerts
The previous tests reimplemented the target-scope rule instead of calling
validateWorkflowTargetScope, so they proved nothing about CreateWorkflow
or UpdateWorkflow's actual enforcement. Split the check into a pure
decideWorkflowTargetScope (tested directly, no database) and a thin
wrapper behind an overridable listServersForScope seam, so tests can
invoke the real CreateWorkflow/UpdateWorkflow without a live database and
fail if the call sites are removed.
Close the time-of-write/time-of-fire gap: a restricted caller could
previously save target_tags matching no server today (a selector aimed
at hosts not yet provisioned or not yet tagged), pass validation on an
empty set, and have the scheduler fire on those hosts the moment they
appeared. Now a restricted caller specifying targets that resolve to
nothing is refused with the same message as an out-of-scope match; a
workflow with no targets at all, and an unrestricted caller, are
unaffected.
A database error while resolving the fleet now surfaces as an error
instead of folding into a pass.
Correct three comments that overstated what the code does: the
create/update route comment now mentions the tag-scope check, not only
validateTargetServers; the schedule route comment explains its safety
holds only for workflows written after this check existed, not for rows
already in the database under the old rule.
CreateWorkflow and UpdateWorkflow validated TargetServerIDs against the
caller's scope but never checked TargetTags, letting a restricted token
save a workflow with an empty ID list and an out-of-scope tag selector,
then reach those hosts once the scheduler fires it as the system with no
restriction of its own. Add validateWorkflowTargetScope, applying the
same all-or-nothing rule the MCP create_workflow tool already enforces:
resolve the full target set unscoped and scoped, refuse unless they
match. Update the PUT /api/workflows/:id/schedule fleetWide comment to
say why it is safe now (targets are constrained at write time) rather
than repeating the falsified claim that scheduling reaches nothing new.
TestServerScopeMapCoversEveryScopedRoute wrongly required serverScopedRoutes
to be a subset of routeScopes, which only covers the authenticated /api
group. The ESO route is registered on the root router and is exempt from
routeScopes by design, so it failed the test despite being correctly
declared. Lift the hardcoded exception in AssertScopeMapComplete into a
named routesOutsideAPIGroup set that both scopes.go and the test read.
GET /api/monitors and GET /api/monitors/:id returned Monitor.Runner
unfiltered; for an agent-pushed monitor that field is literally a server
ID, so a restricted token learned which out-of-scope server a monitor
runs on directly, not merely that one exists. services.RedactMonitorRunner
replaces Runner with models.RunnerRestricted when it names a server
outside the caller's scope, resolved once via the new
services.VisibleServerIDs rather than per monitor. The monitor itself is
still returned — a restricted operator may legitimately need to see that
it exists and is up or down — only the runner field goes neutral; omitting
the monitor entirely was considered and rejected as more surprising than
one field changing. Runner "server" (control-plane-run) is never
touched. The MCP list_monitors/get_monitor_status projections never had a
Runner field to begin with, so REST and the tool surface already agreed;
a comment now records why.
GET /api/workflows and GET /api/workflows/:id returned
Workflow.TargetServerIDs unfiltered — directly naming out-of-scope
servers, worse than a count. services.FilterVisibleServerIDs narrows the
list to what VisibleServerIDs admits and reports hidden (no count) when
at least one target was dropped; WorkflowResponse wraps *models.Workflow
with a scoped TargetServerIDs and a TargetsRestricted flag. TargetTags is
left untouched — the tag vocabulary is already ruled acceptable to
expose. The MCP list_workflows/get_workflow tools get the identical
treatment: list_workflows' target count is now based on the filtered ID
list, and get_workflow's workflowDetail carries the same
TargetsRestricted flag, so a model that sees a filtered target list and
then has run_workflow refuse the same workflow for out-of-scope targets
is not left concluding the refusal invented a problem the list never
mentioned.
All four routes recorded in serverScopedRoutes as true; none is
boot-enforced, for the same substring-filter reason as the key routes
added in the previous round.
GET /api/keys returned each key's AssignedCount as a raw
CountDocuments over every non-revoked assignment, with no scope filter —
a tag-restricted token reading the list saw a nonzero count for a key it
can see nothing assigned to in its own scope, which is enough to tell it
an assignment exists on a host it must not know about. Same class of leak
getKey's assignment-list filter closed on the detail route, surviving on
the list route through a count instead of a server object.
services.ListKeys now takes the caller's tokenScope. An unrestricted
caller (empty scope) takes the original unfiltered per-key
CountDocuments with no extra work, so the common case is not slower. A
restricted caller resolves the visible fleet once via ListServers before
the per-key loop, then counts each key's assignments with an added
server_id $in filter — one extra query total, not one per key.
ListKeys had exactly one caller (listKeys), so the parameter went there
rather than adding a second entry point.
Recorded GET /api/keys in serverScopedRoutes as true; its path, like GET
/api/keys/:id, matches none of serverTouchingRoutes' substrings, so the
entry is not boot-enforced. Deliberately did not widen the filter to
catch "keys" — that would sweep in create/delete/private-key routes with
no server data at all. The real fix for this shape of gap is the
declare-by-default inversion already recorded as a follow-up.
serverTouchingRoutes in cmd/main.go filtered on "server"/"console"/an exact
workflows-run match, which is how POST /api/keys/:id/assign reached
production with no scope check and no boot-time signal at all: its path
names neither. Widen the filter to also match ":serverId" and "assign",
and document at the filter why a substring match is the weak part of this
design — a route that acts on a server without saying so in its path stays
invisible to it — noting that inverting the model (every /api route
declares itself, with an exemption list) would be the stronger fix and is
left as a follow-up. Re-running the mechanical check against the widened
filter swept in no route beyond what serverScopedRoutes already declared.
GET /api/keys/:id also leaked out-of-scope hostnames: it returned every
assignment for a key, server attached, unfiltered by the caller's tag
restriction. getKey now drops any assignment whose server fails
services.ServerInTokenScope before returning the list — silently, so the
response carries no count of what was removed — while still returning the
key itself, since a restricted token may legitimately hold a key also
assigned outside its scope. GetAssignmentsWithServers has exactly one
caller (getKey), so the filtering is done in the handler rather than
threaded into the service. Recorded in serverScopedRoutes; its path
matches none of the filter's substrings either, so it is not boot-enforced
and is kept as a considered decision, same as the assign/revoke entries.
Every early return from a write-tool handler skipped both the tool's own
LogCall and transport.go's gated LogCall (which only fires for reads), so a
blocked mutation attempt left no audit trail. registerSDKTool now routes
every write-tool error through LogDenied (fan-out and tag-scope refusals,
by gate name) or LogFailure (everything else), keeping the successful-write
path logging its own resolved server count exactly as before.
Also close a live scope gap surfaced while reviewing this: POST
/api/keys/:id/assign called services.AssignKey with an unscoped GetServer
lookup, so a tag-restricted token could assign a key to a server outside
its restriction. The handler now resolves the target through
GetServerScoped first, matching its sibling revoke route, and the route is
recorded in serverScopedRoutes.
summariseServer compared Status against "online", a value never assigned
anywhere (the real vocabulary is pending/active/offline), so every server
misreported as offline. search_fleet's version_below used a lexicographic
comparison across dpkg/rpm/apk version schemes with no common ordering, so
it refuses that filter now and returns all matches instead of a wrong
answer. listAuditResult's Total carried the "shown" JSON tag and the
capped count; it now reports the real total alongside shown.
vantagectl/ becomes the root of gitea.hostxtra.co.uk/vantage/vantage-ctl.
The command keeps the name vantagectl; only the repository and the image
path change, to vantage/vantage-ctl.
chart-release.yml's render checks are repointed at the new image. The
chart itself has no default backup.image and fails loudly without one, so
an existing cluster keeps working until someone changes the value.
go.work stays, now with a single use ./server entry: without it a go.work
further up the developer's filesystem is picked up instead.
agent/ becomes the root of gitea.hostxtra.co.uk/vantage/vantage-agent,
with installer/ alongside it, and agent-release.yml goes with them.
Releases now come from that repository, so the six places this server
generates or reads a release URL are repointed: both install scripts,
both update scripts, and the latest-version lookup in dispatch.go. The
agent/v* tag prefix is unchanged — those scripts grep for it.
Agents built before this move have the old mrhid6/vantage path compiled
into their self-update and will 404 on the push-button update. The
remedy is the /update one-liner, which this server generates and which
therefore has to ship first.
vantage.proto documents the hand-written types in shared/grpc/pb, and
nothing compiles it. Keeping it in a different repository from the Go
types it describes meant the one rule holding them together — add the
message to both in the same commit — could not be followed at all.
server's rebuild trigger loses proto/, which it only carried as
insurance against exactly that split.
site/ and sitesvc/ become web/ and server/ in vantage-site; docsite/
becomes the root of vantage-docs. Their images move with them, to
vantage/vantage-site/{web,server} and vantage/vantage-docs.
Nothing here imported any of them, and sitesvc turned out to read no
database at all, so both cuts are clean. docker-compose.site.yml is
deleted rather than emptied: every service it held now ships with the
repository that builds it, and deploy/docker/docker-compose.yml is once
again exactly a self-hosted install.
Corrects four comments that named sitesvc for work it no longer does.
admin/ and adminsite/ are extracted with their history to
gitea.hostxtra.co.uk/vantage/vantage-admin, where they are named server/
and web/ for what they are rather than for the services they run. Their
images move with them, to vantage/vantage-admin/{server,web}.
Nothing here imported them, so the cut is clean: the only coupling was
always at runtime, through admin writing into the control plane's
database. The parts of that contract this side enforces are unchanged and
still documented here — hq-sourced users, POST /license answering 409
cloud_managed, and FREE_INSTANCE_REAP_AFTER needing to match.
LICENSE_SIGNING_KEY now appears in no compose file in this repository.
Keeping it out used to be a rule someone had to remember; it is the
repository boundary now.
docker-compose.site.yml loses both services and gains a note on how the
host composes the three files together.
go list -m -f '{{.Dir}}' reports an empty Dir and exits 0 for a module that
is not in the cache, so on a cold runner swag was handed an empty --dir and
failed several steps later with 'dir: does not exist'. Download first, and
fail loudly if the path is still not there.
shared/ is extracted to gitea.hostxtra.co.uk/vantage/vantage-shared and
pinned at v0.1.0 by server, agent, admin, sitesvc and vantagectl. The
replace directives and the ./shared entry in go.work are gone.
Every Go build now needs a credential for the private module: CI writes a
netrc per job from REGISTRY_USER + RELEASE_TOKEN and sets GOPRIVATE, and
the four Go Dockerfiles take it as a BuildKit secret rather than a build
arg, which would survive in the builder layer's history. RELEASE_TOKEN
needs read access to the vantage org.
admin, sitesvc and vantagectl now build from their own directory; only
server still needs the repository root, for default_steps/. The rebuild
triggers in server-deploy.yml lose their shared/ patterns, since a
service now moves when its own go.mod pin does.
vantagectl is a released tool, not a running service. An operator restoring a
database should be able to name the version they ran, and ":latest, rebuilt
whenever main moved" cannot be named after the fact.
The image now builds in vantagectl-release.yml on a vantagectl/v* tag, tagged
with that version as well as latest, with VERSION passed through so the binary
inside reports the tag rather than "dev". server-deploy.yml no longer builds
it and is back to seven images.
The cost is that a shared/ fix reaches the image only at the next release
rather than the next push to main. That is the intended trade and is written
down in CLAUDE.md next to the trigger table.
Vantage encrypts SSH private keys, vault secrets, SSO client secrets and
console credentials with KEY_ENCRYPTION_KEY. That key is in no backup and is
not recoverable, so restoring a database without it produces a control plane
whose every secret is permanently unreadable.
vantagectl dumps and restores a whole database and stamps a SHA-256
fingerprint of the key into every archive, so a restore refuses rather than
producing that database. The key itself is never written.
- shared/cryptobox: AES-GCM extracted so the server and the CLI share one
implementation rather than a copy that can drift
- shared/backup: manifest, tar+gzip archive with per-member checksums
verified before the first write, dump, restore, verify
- vantagectl: its own module, so cobra stays out of the server, admin and
sitesvc module graphs
- container image, tagged release workflow, CI rebuild trigger
- optional Helm CronJob, off by default
- operator documentation, reconciled with the existing backups page
mrhid6