Conversation
Percent-encoding leaves "." and ".." untouched (both are unreserved), and the URL parser resolving /api/agents/<host>/... then collapses the dot segment: "." drops the segment entirely and ".." climbs a level. A host reporting either name heartbeats and shows online on the dashboard while every command, spawn, kill and migration relay against it 404s, with nothing pointing at the name as the cause. _usable_hostname() now rejects both, so the agent falls through to its next naming source instead of registering under a name the hub cannot address, and logs why -- the fallback name is otherwise unexplained. The DEVICE_NAME/COMPUTERNAME override deliberately outranks the other placeholder rules (an operator naming a host "localhost" means it), but a dot segment is unaddressable no matter who chose it, so that one check applies there too. Only the bare segments are affected; "...", ".hidden", "a.b" and "HOST.local." percent-encode and route fine, as do the rest of the device-name matrix from XERK-266.
…side
QA found the first commit half-applied the fix. agent/tunnel-agent.js
carries the other copy of device_name()/_usable_hostname() -- a parity
pair CLAUDE.md names -- and was not updated.
That split the host identity rather than closing it. entrypoint.sh only
calls `--print-device` when DEVICE_NAME is unset, so an operator-set
DEVICE_NAME="." reached both processes unvalidated: the manager fell
through to its next source while the tunnel kept ".", which works there
because it registers by query param (no parser collapses those). Since
openChannel keys controlChannels by the name, the hub then never reaches
the tunnel at all -- trading "every command 404s" for "commands work,
terminal and live tail dead", plus a ghost card. Measured: /term/<sess>
-> 502 "agent tunnel offline", and GET /api/agents lists both the ghost
(online=false, terminalOnline=true) and the real host.
So tunnel-agent.js gets the same check in both places, and the pair is
now parity-tested -- tunnel-agent.test.js had no coverage of either
function, which is why this shipped green.
Also guards it hub-side, one clause on the existing XERK-235 device-key
allowlist in turma/server.js. The agent-side fix protects nothing against
agents not yet upgraded, and Watchtower updates hosts independently, so a
mixed fleet is the steady state. Without it a pre-fix agent's dot name is
a card that reads online, refuses every command, and cannot be removed --
DELETE /api/agents/. is itself one of the routes that 404s, and prune()
only drops a record after 7 days without a beat. A 400 in the agent's own
log beats a 7-day ghost.
Names that merely contain dots ("...", ".hidden", "a.b", "HOST.local.")
are unaffected and covered by tests on both sides.
…usals
QA's second pass: the guard shipped only on the heartbeat ingest, so a
hub whose state.json already carried a "." key restored the ghost at boot
and it was still uncommandable and undeletable. That is the exact gap
CLAUDE.md warns about in the section beside the one I edited -- a
coercion belongs where BOTH the ingest and the restore reach it, because
a restart is when one ships and the restore is the first thing served.
So the key check is now isPlainHostKey(), called from both, covering the
XERK-235 prototype keys as well. The restore drops a bad key rather than
carrying it (a live host re-registers on its next beat), and says so.
server.test.js already asserted by source inspection that the ingest and
the restore share ONE coercion function; that assertion is extended to
the key guard, which is what would have caught this.
Exact match is deliberate. The padded forms (" . ", ".\n") are genuinely
addressable -- the padding percent-encodes to %20/%0A and no parser
collapses those -- so refusing them would over-scope the guard. Verified
by booting a hub on a hand-written state.json: the "." and ".." records
are dropped with a log line while "HOST.local." survives and still
answers DELETE with 200.
Also makes the refusal legible. The first commit's comment claimed "a
loud 400 in its log beats a 7-day ghost" and it was not loud: the hub
logged nothing, and urllib's HTTPError stringifies without the body, so
the agent's own log shows a bare "HTTP Error 400" and the host just
vanishes from the dashboard. The hub now names the refused key, which is
the only place that reason is recoverable. The agent-side opacity is
pre-existing and filed separately.
Also blanks PATH around the new tunnel-agent deviceName test: falling
through reaches dockerHostName()'s execFileSync, whose 15s timeout would
be paid three times against a possibly-cold daemon on a CI runner.
QA's third pass, both findings acted on. The refusal log I added last commit was an unbounded write of an attacker-controlled string. sanitizeHeartbeat does not cap `device` -- only HEARTBEAT_MAX (32 MiB) does -- so two refused beats wrote 9 MiB into the hub log, synchronously, on the request path, blocking every other host's beat while it ran. Every agent host shares one TURMA_AGENT_TOKEN, so one buggy host could fill the hub's log and the /data volume. Keys now log through hostKeyLabel(), which truncates at 80 chars and reports the real length. The restore drop had no behavioral test. It was protected only by a source regex asserting isPlainHostKey(key) appears in the loader, and QA showed two mutations that keep that string, keep the suite green at 1079/1079, and restore the ghost bug in full: removing the `delete` still PRINTS "dropping restored agent under unusable device name" while dropping nothing, and continue->break makes survival depend on JSON key order. The first is what makes this worth fixing rather than arguing -- the boot log I used to check the fix by hand is not evidence the drop happened. So the loop is now dropUnusableHostKeys(), exported and tested directly, with refused keys first, last and in the middle of the fixture. Both mutants now fail. The source-inspection assertion stays, since it is what guards ingest/restore parity, but it is no longer the only thing. Also anchored that test's source slice on the section header rather than a statement -- rewording the parse silently sliced nothing and passed the whole block vacuously, which it did when I made the change below. Also guards the parsed shape, which QA raised as unproven and which reproduces: state.json containing `"hello"` restored "5 agents" (the string's character indices) and then threw into the silent catch. The parse now lands in a local, is shape-checked, and is only then assigned, so a throw cannot leave junk in `agents`. Also documents that the restore reaches isPlainHostKey and dropUnusableHostKeys only through function hoisting -- as consts the TDZ error would land in that swallowing catch and the hub would boot with zero agents and print nothing.
Self-review with the mutants QA taught me to run: removing the shape
check on load, and assigning `agents` before checking it, both survived
the suite. The unit test for dropUnusableHostKeys cannot reach either --
they live in the module-init restore, which only a real load exercises.
server.test.js already had a child-process boot harness for exactly this
("the restore actually RUNS"), so both cases now boot the real module
against a fixture state file: dot-segment and prototype keys must not
appear in the restored `agents`, and a corrupt file ("hello", [], 12,
null, true) must restore nothing rather than a registry of character
indices.
Two things this surfaced that are worth keeping:
The log line is captured and asserted separately from the drop, because
console.warn goes to stderr, which that harness discards -- so the
assertion I first wrote passed vacuously against a mutant that reported
without deleting. The key list is the evidence; the warn count only
checks the operator is told at all.
The prototype fixture is raw JSON, not JSON.stringify of a literal.
`"__proto__":` in an object literal sets the prototype and creates no
key, so the stringified form silently omitted the one case XERK-235 is
about; JSON.parse does make it an own property, which is the real hazard.
Caught because the warn count came back 2 instead of 3.
Mutation sweep now 7/7: remove the delete, continue->break, drop either
shape guard, log the key raw, assign before checking, drop the ingest
guard.
The shape check I added two commits ago throws a message nothing ever
printed: the restore's `catch {}` swallows everything, so a corrupt state
file was indistinguishable from a first boot. QA flagged the dead
diagnostic; this finishes it.
ENOENT stays silent, since that IS the ordinary case (first boot, or no
volume mounted). Anything else means a state file exists and could not be
restored, which is worth a line -- and it is the diagnostic whoever hits
a corrupt file will need, since the hub otherwise boots cheerfully empty.
Verified both directions: the corrupt-file boot test now asserts the
warning is emitted, and a boot with no state file at all prints nothing.
`typeof null` is "object", so the one shape with no other diagnosis read "state file is object, not an object" -- the opposite of a useful message, in a commit whose whole point was making a corrupt file diagnosable. Reported by QA as cosmetic; taken inline since it undercuts the change it sits in.
QA's post-merge pass. `--agent-token .` minted a real, valid credential
for a host that XERK-269 now guarantees can never register, and the
resulting failure is the worst shape available: the agent renames itself
to its next naming source, the token stops matching, and the tunnel
reconnect-loops forever without ever mentioning the name.
[tunnel-agent] ignoring $DEVICE_NAME=".": a URL dot segment is ...
[tunnel-agent] starting; hub=... name=truenas
[tunnel-agent] control channel error: connection failed
[tunnel-agent] control channel closed; reconnecting in 1s <- forever
That interaction is created by this branch's own rename, which is why it
lands here rather than in a follow-up: before the rename, such a host
registered (uselessly) rather than looping.
The check goes in hostAgentToken, not just the CLI, so every mint path is
covered -- it sits beside the two refusals already there (non-strings,
names that don't survive a UTF-8 round trip), and is the same rule: do
not derive a credential for a name that is not a host. The CLI now says
why and exits non-zero instead of printing an empty line, which sent the
operator looking at the master.
Names that merely contain dots ("...", ".hidden", "..host",
"HOST.local.") still mint, since they register fine.
XERK-272 (bound the agent registry) reworked the same state.json restore
block this branch guards. Both conflicts are in that block; both sides'
intent is kept.
Restore body — the two guards compose, in this order:
1. XERK-272's size check, BEFORE the file is opened. It has to stay
first: readFileSync + JSON.parse materialize the whole file, so a
flooded state.json OOMs the hub at init and no later check runs.
2. XERK-269's parse-into-a-local + shape check, so a blob that is not
a registry never reaches `agents`.
3. XERK-269's dropUnusableHostKeys, then normalizeRecord, then
XERK-272's trimRestoredAgents. Dropping unaddressable keys BEFORE
the trim matters: otherwise a ghost record consumes part of the
registry budget and can evict a real host to make room for one that
is about to be deleted anyway.
The two shape guards are not redundant. XERK-272's catch now clears
`agents` on any non-ENOENT failure, which covers every shape that
THROWS. It does not cover `[]`, `12` or `true`: those throw nowhere, so
the catch never fires and only the shape check keeps them out of the
registry. Verified both ways on a real boot.
Catch clause — took origin/main's wholesale. It does everything this
branch's version did (report a non-ENOENT failure rather than letting a
corrupt file read as first boot) and additionally resets a
partially-built registry, and its wording is what registry-restore.test.js
asserts. This branch's console.warn version is fully subsumed; the only
thing carried over is a mention of the non-registry shape in the comment.
Test fixup: the XERK-269 boot harness captured console.warn only, and the
merged restore reports a refused file through console.error. It now
captures both, and asserts main's "state restore skipped" wording.
Suites: 1184 python OK; node 1143/1144.
The one node failure is `registry-restore.test.js` "when it CANNOT move
the file" and is NOT from this merge — it fails identically on pristine
origin/main on this host. It makes a directory read-only to force the
rename to fail, and this box runs as uid 0, which ignores the permission
bits, so the rename succeeds and the message names the .oversized file it
did create. CI runs non-root, where it passes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A host reporting a device name of
.or..was silently unreachable on every/api/agents/<host>/...route. It heartbeat normally and showed online and healthy on the dashboard, while every command, spawn, kill and migration relay against it 404'd — including theDELETEthat would have removed it.Root cause
urllib.parse.quote(name, safe='')leaves.and..unescaped (both are unreserved), and the URL parser resolving the path then collapses the dot segment. Confirmed before fixing:Only the bare segments collapse. Everything else in the XERK-266 device-name matrix survives.
What changed
The ticket named one function. The value reaches four places, and a guard in one of them is not a fix — three of the five commits here exist because I learned that the hard way, via QA.
agent/hub-agent.py—_usable_hostname()refuses both names sodevice_name()falls through to its next source, and logs why. TheDEVICE_NAME/COMPUTERNAMEoverride bypasses_usable_hostname()by design (an operator naming a hostlocalhostmeans it), but a dot segment is unaddressable whoever chose it, so that one check applies there too.agent/tunnel-agent.js— the mirror. CLAUDE.md names this pair as a parity contract andtunnel-agent.test.jshad no coverage of either function, so the first commit shipped green while making things worse:entrypoint.shexports an operator-setDEVICE_NAMEto both processes unvalidated, the tunnel registers by query param (which no parser collapses) and so kept., andopenChannelkeyscontrolChannelsby name. Result was commands working while the terminal and live tail were dead, plus a ghost card. Now mirrored and parity-tested.turma/server.js, ingest —isPlainHostKey(), folding in the XERK-235 prototype/type/length checks. Agent-side alone protects nothing against un-upgraded agents, and Watchtower updates hosts independently, so a mixed fleet is the steady state.turma/server.js,state.jsonrestore — the same guard. A hub whose state file already carried a.key restored the ghost at boot;prune()needed 7 days to clear it. This is the rule CLAUDE.md states in the section beside the one I edited — a coercion belongs where both the ingest and the restore reach it — so the check is one shared function, not two copies.Plus two things the above turned up, both in code these commits touch:
sanitizeHeartbeatdoesn't capdevice, so two refused beats wrote 9 MiB into the hub log, synchronously, on the request path. All hosts share oneTURMA_AGENT_TOKEN, so one bad host could fill the log and/data. Keys now log throughhostKeyLabel()(80 chars + true length).state.jsoncontaining"hello"loaded as "5 agents" — the string's character indices. The parse now lands in a local, is shape-checked, and is only then assigned; an unusable file is reported rather than being indistinguishable from first boot.QA
Five passes by the
qaagent (~/.claude/agents/qa.md), each against real processes and a real hub — no stubs across the agent↔hub seam.tunnel-agent.jsmirror missing — the fix split the host identitymainEvidence at the tip:
Mutation sweep 15/16 across the branch (the 16th is an equivalent mutant QA identified as unreachable, not an escape). Suites: 1171 Python, 1083 Node, all passing.
Exact-match is deliberate.
" . ",".\n","\t..\n"are genuinely addressable — the padding percent-encodes to%20/%0A, which no parser collapses — so refusing them would over-scope the guard.Not verified
entrypoint.shis inspection plus a native--print-devicerun. Both new log lines are prefixed, so itssed -n 's/^DEVICE_NAME=//p'is unaffected.veiller/not built (no SDK/gradle cache on the box). No parity impact: this is agent/hub plumbing with no user-facing surface, so noPARITY.mdline is needed. Android's one exposure — Retrofit throwing on a dot@Path— is closed in practice now that no dot-named host can enter the fleet list.EACCESon the state file couldn't be arranged; it takes the identical branch as theEISDIRandELOOPcases that were driven.Merged main mid-review
Main moved while this was in QA. Merging it brought XERK-268 (per-host agent tokens), which binds the heartbeat to a per-host credential — landing on the same handler this change guards. QA's sixth pass covered the interaction specifically:
agentPresented(pre-body) →isPlainHostKey→agentHostRefusal. Any caller holding a valid agent credential gets the name 400, never an auth-flavoured refusal; only "no credential" (401) and "master under strict" (403) answer first, which is correct precedence..separator, sotokenHost's split stays unambiguous for every name, and tokens travel in headers or query params, never path segments.--agent-token .minted a valid credential for a host that can now never register, so the agent renamed itself, the token stopped matching, and the tunnel reconnect-looped forever without naming the cause. Fixed inhostAgentToken(covering every mint path, beside the two refusals already there) with the CLI now saying why and exiting non-zero.XERK-294, filed earlier in this review, turned out to be fixed on main by XERK-268 and is closed.
Filed, not fixed here
hub-agent.py:10742,:10759), same class as the above.nullrecord instate.jsonblanks the entire fleet, reporting"payload too large"for what is aTypeError. Pre-existing, reproduces onmain.