XERK-259: hold the whole agent record to the shape clients type - #435
Merged
Conversation
`/api/agents` decodes atomically on Android, so one host's wrong-typed value
throws for the entire array. The hub coerced three blocks by hand, which was
never the boundary that matters: a field is decode-fatal the moment a client
TYPES it, and Android types nearly all of AgentInfo. `repoUsage: [null]` — four
bytes from one host — made the app refuse to sign in at all ("Could not reach
the hub"), because the login probe decodes that payload.
turma/wire-shape.js is a table mirroring Models.kt plus the walker that applies
it, run from normalizeRecord (so both the heartbeat ingest and the state.json
restore get it). Two rules it exists to enforce: a list's ELEMENTS are as fatal
as its type, and `typeof [] === "object"`, so element tests go through
isPlainObject.
It coerces IN PLACE, touching only the keys it names, so it is not a whitelist:
a sub-key a newer agent adds rides through untouched rather than being dropped
fleet-wide until the table catches up. Values become the "can't tell you" one
every client already handles — "" / false / 0 / null / [] — and a nullable field
becomes null, never a zero that reads as a real measurement.
Its own module because server.js restores state.json at module init: a const
declared below that point is in its temporal dead zone, and the ReferenceError
dies in the restore's own `catch {}`, leaving records half-coerced with nothing
logged. Ordering inside normalizeRecord IS load-bearing and is documented: the
sweep runs last, after normalizeModelUsage rewrites an old agent's bare
model-name strings.
Tests: the hub suite pins the four reported shapes, element drops in every typed
list, the can't-tell values, map values (no coerceInputValues there), blocks with
a non-string discriminator, and that untyped keys survive. One test parses
Models.kt and walks it, so typing a field there without adding it here fails CI
rather than waiting for a phone to stop signing in. AgentDecodeTest pins the
client half — each shape is asserted decode-fatal, and the record the hub now
serves decodes beside a healthy host.
An adversarial QA pass failed the first commit on its own exemption. The sweep skipped `limits` on the grounds that `normalizeLimits` rebuilds it — but a rebuild is only as good as its own gates, and that one gated both epoch fields on `Number.isFinite`. Kotlin's `Long` takes no fractional literal, so `resetsAt: 1.5` from one host was served raw and threw for the whole `/api/agents` array: the same whole-fleet symptom this ticket exists to close, sitting inside the function meant to prevent it. Reproduced with one curl against the built image and confirmed with the real Kotlin decoder. - `normalizeLimits` now bounds `resetsAt`/`capturedAt` with `Number.isSafeInteger` (`usedPct` is a Double and keeps the looser check). A bad stamp drops that field, and an unusable `capturedAt` nulls the block — its existing degradation. - `limits` and `localModel` are in the shape table too, so they get both the type backstop and the Models.kt drift test. Nothing agent-authored is exempt now. The drift test also had two vacuous-pass modes, which is worse than the hole: it is the thing that stops this class of gap recurring. Its per-line field regex skipped every field with NO default — the dangerous kind, since those are required on the wire — and saw only the first field of a one-line `data class` (the transcript blocks). It now splits the constructor on top-level commas after stripping comments, and asserts field counts that a mis-parse drops first, so a parser that silently sees nothing can no longer read as "all covered". Re-ran the sabotage matrix: both escapes are now caught, as is dropping `limits` from the table. Also fixed inline, both found by the same pass: nothing gated `turma/Dockerfile`'s hand-maintained COPY list, so a future module omitted from it would be a container that boots to MODULE_NOT_FOUND — now a test over server.js's own local requires. And AgentDecodeTest pins the client half of the limits shape.
Second QA pass: the product fixes hold (limits dead across ingest, restore, a real container and 2000 re-fuzzed mutants — zero decode failures), but two of the guards I added to stop this class of bug recurring could still be walked through. A regression net with a hole in it is the thing that let XERK-259 exist. - The Models.kt splitter counts bracket depth and does not know string literals, so a default containing a bare `>`/`(`/`[` desynchronised it and folded the rest of the constructor into one param. The field-count FLOOR masked that only at today's count: QA proved a class grown by four fields — an ordinary paired change — then passes while silently not checking a field. Replaced the floor with a per-class count equality against the `val`s in the same comment-stripped body, which cannot drift as the class grows. - `data class X` with its `(` on the next line was invisible to the class regex, and a field of that type then skipped sub-shape validation. `\s*` around it. - The Dockerfile test matched only double-quoted, top-level, same-file requires, so a nested path, a single-quoted require, and one two hops out all escaped — each a container that boots to MODULE_NOT_FOUND with a green suite. It now walks the whole require graph from server.js and matches either quote style, a path segment, and ignores names appearing only in a Dockerfile comment. Verified by re-running every case that escaped: all six caught, the original control still caught, and a legitimate paired addition (field added to Models.kt AND the table) still green — a guard that fails on correct changes gets deleted.
Third QA pass: N1/N2/N3 are closed and the count-equality assert survived 18 engineered attempts to defeat it, but the fixes had introduced the mirror image of the bug — neither guard knew a string literal from code, so both now rejected LEGAL input. A loud failure on a correct change is what gets a guard deleted, which is the same way this class of bug gets back in. - The Models.kt parser read a default's CONTENTS as syntax: `"https://x"` hid a comment, `"/*"` opened one, `">"` broke the bracket depth, and `"use val x: Int"` invented a field. Each failed a paired, correct change with a message accusing the reader of breaking the parser. One length-preserving pass now blanks string/char/raw-string bodies and comment bodies before anything reads it as syntax, so offsets stay valid and defaults are text. - Two of the parse canaries were exact field counts, and both rot: a third field on TextBlock is an ordinary change, and CreateTicketRequest is not even on the agent record. The per-class count equality already proves every field of every class is visible, generically, so these are back to "was this class seen at all". - The require walk read prose as code: this repo's comments name modules constantly, so a stale note about a deleted one failed CI claiming it was required but missing. Sources are comment-stripped now, the existence assert is gone (a genuinely missing module takes the suite down at its own require, so that assert could only fire on a false positive), and a statically-written template-literal require counts. Verified both directions: 11 legitimate-change cases that used to fail are green (five string-noise defaults, the two rotted counts, prose in a comment and in a string), and all 9 catches still bite (M2, M14, T2, N2, dropping limits from the table, and all four Dockerfile shapes). Unrelated flake seen once and worth naming: "normalizeLocalModel bounds the name BEFORE spreading it" asserts wall-clock < 60ms and failed once under load, then passed 3/3. It belongs to #430, and this branch is not the cause — the sweep costs 0.01ms on that record, measured.
Fourth QA pass: P1/P2/P3 closed and no silent loss survives anywhere — every
Kotlin edge that desynchronises the parser now fails loudly through the
count-equality assert. Two shapes still mis-read a literal, one of them realistic
enough to redden CI on an ordinary edit.
- `blankNonCode` had no regex-literal branch, so a quote inside a character class
opened a phantom string that ran to the next matching quote — usually EOF —
leaving every comment in between unblanked and re-arming the exact P3 false
positive this was meant to kill. Not hypothetical: `/[.,;:!?'"]+$/` exists
verbatim at turma/public/chat.js:182, and adding `'` to server.js:194's
character class would have been a one-character edit that disarmed
comment-blanking for the rest of the file. Fixed without a regex-vs-division
heuristic: neither language lets a `'`/`"` literal span a line, so one that
reaches a newline unclosed was never a string and is emitted as code.
- A Kotlin raw string may END with a quote (`"""ab""""`), and closing at the
FIRST `"""` of the run left a stray quote that opened another phantom string.
The closer is now the last three of the run.
The third finding — a require written inside a string still reads as a real one —
is inherent to needing string contents to read the path out, so it is documented
as a bound beside the pattern rather than papered over.
Verified: QA's three regex shapes plus the server.js:194-with-apostrophe variant
are green as ordinary edits, while a real COPY omission is still caught WITH a
regex present; the raw-string field is green paired and caught unpaired; 21
unit shapes over blankNonCode hold (escapes, nested comments, `${}` templates,
char literals, multi-line templates); and all four real hub sources plus
Models.kt still blank to zero unblanked comment lines with length, newlines and
all 380 `val`s preserved.
QA's final pass returned PASS after driving the real Android client on an emulator — the app signs in and lists every host against this branch, and shows "Could not reach the hub" against the same tree with the coercion stubbed. Its one remaining LOW was a backtick inside a JS regex re-opening the P3 false positive: backticks are exempt from the newline rule (template literals may legitimately span lines), so `` /`([^`]+)`/g `` — which glasses and veiller both contain verbatim — opened a phantom literal that swallowed the comments after it. The suggested one-liner (treat an unclosed backtick at EOF as code) does not actually fix it: the phantom literal closes on the next REAL backtick in the file, not at EOF. Verified before discarding it. So `blankNonCode` now lexes regex literals properly, JS-only (Kotlin has none, and `a / b / c` would read as one), deciding regex-vs-division on the previous significant token and scanning to the closing `/` on the same line, stepping over `[...]` classes. That turned up a second live mis-blank on the way: `/\//g` in push.js:48 — a regex whose escaped slash is followed by its closing one — read as a `//` comment and blanked the rest of that line. Harmless there, but the same shape one line further up would have hidden a require. Verified the differential rather than assuming it: every character that now blanks differently sits inside a regex literal, nothing spills across a newline, and the requires recovered from all four hub sources still equal ground truth exactly. Eight end-to-end shapes green (three backtick regexes, the Q1 quote regex, a regex containing `//`, plain division, a multi-line template) with a real COPY omission still caught alongside a backtick regex.
# Conflicts: # CLAUDE.md
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.
Fixes XERK-259.
Stacked on #430 (
XERK-246-android), which is wherenormalizeRecord/normalizeSessionslive. Reviewgit diff origin/XERK-246-android...HEAD; this retargets tomainwhen #430 merges.The problem
/api/agentsdecodes atomically on Android: one host's wrong-typed value throws for the whole array. The hub coerced three blocks by hand, which was never the boundary that matters — a field is decode-fatal the moment a client TYPES it, and Android types nearly all ofAgentInfo.repoUsage: [null]— four bytes from one host — made the app refuse to sign in at all, reporting "Could not reach the hub — check the URL", because the login probe decodes that payload. XERK-246 had just rewritten a non-arrayrepoUsageto[]while leaving its elements raw: the shape nobody sends fixed, the one that breaks the phone served.The ticket asks for the sweep its "Wider rule" section implies, not the one field — this is the second time the same bug has been filed one field over.
What this does
turma/wire-shape.js(new) — a table mirroringModels.ktplus the walker that holds the agent record to it, run fromnormalizeRecord, so it covers both the heartbeat ingest and thestate.jsonrestore.Two rules it exists to enforce, both learned the hard way:
typeof [] === "object", so an array element slips through that test — everything goes throughisPlainObject.Design decisions worth reviewing:
""/false/0/null/[]— and a nullable field becomesnull, never a zero.work.pushed: falsewould read as "definitely not pushed";aheadOfBase: 0as "nothing to lose".server.jsrestoresstate.jsonat module init: aconstdeclared beside the walker is in its temporal dead zone then, and the ReferenceError dies in the restore's owncatch {}. My first draft did exactly that and the existing "the restore actually RUNS" test caught it, half-coerced.normalizeRecordIS load-bearing and now says so: the sweep runs last, afternormalizeModelUsagerewrites an old agent's bare model-name strings (sweeping first would drop them and silently zero that host's usage page).Coercion targets were established empirically, not guessed: a throwaway Kotlin probe through the real decoder under this project's
Jsonconfig. An object/array into aStringis fatal but a number is not (lenient);Intis 32-bit andLongrefuses a fraction; a null map value is fatal where a null field is not (coerceInputValuesdoes not reach map values); a block with a non-stringtreaches neither a known block nor theUnknownBlockfallback.Found while doing it — and fixed here
limitswas still whole-fleet decode-fatal (pre-existing, XERK-247).normalizeLimitsgated both epoch fields onNumber.isFinite, but Kotlin'sLongtakes no fractional literal, soresetsAt: 1.5was served raw and threw for the whole payload — inside the function meant to prevent exactly that. Now bounded withNumber.isSafeInteger; a bad stamp drops that field and an unusablecapturedAtnulls the block (its existing degradation).limitsandlocalModelare in the table too rather than exempted: a rebuild is only as good as its own gates.Nothing gated
turma/Dockerfile's COPY list (pre-existing). It is hand-maintained, and this change adds the first new hub module in a while — a future one omitted from that line is a container that boots toMODULE_NOT_FOUND. Now a test overserver.js's own localrequires.Tests
normalizeModelUsage.Models.ktand walks it against the table (360 fields), so typing a field there without coercing it here fails CI rather than waiting for a phone to stop signing in.blankNonCode): a comment, string, or regex literal is never read as code. Four of the six commits here are that hardening, and the reason it earned the space is that every one of its bugs was a false positive on an ordinary edit — a default of"https://x", a regex holding a quote or a backtick — and a guard that fails on correct changes is one the next person deletes. It also turned up a live mis-blank:/\//gatturma/push.js:48read as a//comment.AgentDecodeTest.ktpins the client half: each shape asserted decode-fatal with the realTurmaJson, and the record the hub now serves decoding beside a healthy host.Verification
QA verdict: PASS (
~/.claude/agents/qa.md, five passes; the first three returned FAIL/PARTIAL and are what the later commits answer).The pass that settles it drove the real Android client on a real emulator: APK built by
:app:assembleDebug, installed on a headless Pixel 6 AVD, driven through sign-in withadb shell input, against a hub fed the poisoned heartbeats. Against this branch the app signs in and lists all four hosts; against the identical tree withnormalizeRecordstubbed out it shows "Could not reach the hub — check the URL." NoSerializationExceptioninlogcaton the passing run. That is the ticket, reproduced on a device and then eliminated.Also confirmed, with measurements:
/api/agentsbodies:atomic OK, badHosts=0on all four payloads.hub-agent.py's ownrepo_usage_reportandread_limits_snapshot) is served with 0 diffs sent-vs-served, untyped extras included. The web dashboard renders 832 hostile hosts across all four pages with zero console errors.413, previous record restored, containerrestarts=0 oom=falseat 89 MiB of 256 MiB. Growth is ≤1.14× on a pathological record, which the second gate catches.JSON.stringifyof it (9–10 ms), which the ingest already does twice. Real records are ~0.30 MiB.Object.prototypeown-key delta 0, including__proto__keys insideusage.days), no stack overflow on a 200,000-deep untyped subtree, 3,000,000-element list compacted in 16.8 ms with array identity preserved (so a concurrently-queued command is not orphaned).Models.ktsabotages, 12/12 table sabotages, 6/6 require omissions, 13/13 parser-blindness probes — and 6/6 legitimate-growth changes stay green, which matters as much: a guard that fails on correct changes gets deleted.Not verified (carried verbatim from the QA report):
/api/eventsSSE — per-agent frames go through the sameserializeAgentso the coercion applies, but no stream was opened and decoded; theglasses/andveiller/clients, which consume the same record and are untouched here; migration, archive, history, search, live-tail, multi-host concurrency; CI on a pushed branch (thecode-scan.ymlcommand was run locally three times, 1084/0); Android beyond sign-in and the dashboard list; and four Kotlin constructs the drift test does not parse (whereclauses, a genericdata class Foo<T>(, typealiases, a data class nested in a class body — none exists today, and a field of one absent from the table would still be caught).Unrelated flake, for #430's owner:
normalizeLocalModel bounds the name BEFORE spreading it(turma/tests/server.test.js:6644, fromf9e85a2) asserts wall-clockms < 60. Measured here: idle p50 9.9 ms / p99 24.4 ms; with 32 cores busy, p50 19.4 ms and max 92.3 ms — 1 run in 30 over the line. This branch's sweep on that record is 0.002 ms p50, ~0.003% of the budget. It is contention, not a regression. Best-of-N or a same-process ratio would make it deterministic; the structural assertion already in that test carries the real invariant with no timing at all.Known bounds, audited and accepted:
queueCommandcan push intoagents[key].commandsbetween beats, so for up to one beat interval/api/agentscan serve a command the sweep has not seen. All 30 call sites were audited —typeis a literal,cmdIdis hub-minted,sessionIdcomes from a URL path segment, and the one body-suppliedrepois regex-validated — so this pays no per-request sweep cost. The bound holds because every writer validates, so a future route queuing an unvalidated field re-opens it. The Dockerfile guard likewise cannot see../,require.resolveor a computed path, and reads a require written inside a string as a real one.Parity: no user-facing change, so no
android/PARITY.mdentry; the Android edit is test-only.CI — read this before merging
GitHub runs zero checks on this PR, and that is structural, not a failure:
code-scan.ymlandandroid-ci.ymlare bothpull_request: branches: [main], and this PR targetsXERK-246-android. The checks will run for real when #430 merges and this retargets tomain.So every gate was run locally against the merged tree (
c5fcad4), using the same commands the workflows use:p/default,p/secrets,p/dockerfile, same four--exclude-rules,--error)--failure-threshold erroron both Dockerfilesagent/entrypoint.sh.claude/rules/agent.mdat 35,074;CLAUDE.md29,646agent/tests)assembleDebugglasses-ci.ymlis path-filtered toglasses/**, which this diff does not touch.Merged with the base
origin/XERK-246-androidmoved (XERK-246's eleventh/twelfth passes, XERK-256, XERK-257, XERK-264, plusmain), so it is merged in atc5fcad4. One conflict, inCLAUDE.md, where both sides edited the same bullet: resolved keeping both — this branch'swire-shape.jsbullets and the base's XERK-264 hub-refusal bullet. The base's blanket sentence "Anormalize*is a WHITELIST" is not restored verbatim because it is no longer true as written (the sweep is deliberately not a whitelist); its accurate residue is the sub-bullet namingnormalizeLimits/normalizeLocalModel, the two that really do rebuild.One duplication left deliberately for #430's owner to settle: that branch independently added
objectish()inserver.js, which is semantically identical towire-shape.js'sisPlainObject. Both are correct and both are one line; unifying them would mean rewriting a rationale comment written on an in-flight branch inside a merge commit, so it is flagged here rather than done. Worth collapsing to one exported predicate once #430 lands.