Skip to content

Upstream names in the idle-gateway warning are sanitized and capped like every other status-file label (#680) - #681

Merged
philcunliffe merged 6 commits into
masterfrom
fix/issue-680
Aug 17, 2026
Merged

Upstream names in the idle-gateway warning are sanitized and capped like every other status-file label (#680)#681
philcunliffe merged 6 commits into
masterfrom
fix/issue-680

Conversation

@philcunliffe

@philcunliffephilcunliffe commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

gateway_idle_no_upstreams reads the dropped upstreams' names out of
status.json and joins them straight into the warning it prints, filtering
only for "non-empty string" (src/core/daemon/status.js, the named
parenthetical). recentEntrypointsFromSources reads the same file, in the same
module, on the way to the same terminal, and passes every label through
sanitizeLabel and a count cap first - deliberately duplicating what the
gateway already did on the way in, because status.json is a file and core
cannot assume the daemon that wrote it was this version, this build, or well
behaved (LLP 0164#status-reads-it-from-the-status-file). Two paths reading one
file should not disagree about whether it is trusted.

What this changes

  • printableUpstreamNames cleans each name through sanitizeLabel (strips
    control, line-breaking, bidi and zero-width characters; clamps to 120 chars
    with a truncation marker) and caps the printed list at 8.
  • The names it holds back are counted in the line (, +N more), so a sampled
    list never reads as a complete one.
  • The count still comes off the raw list, before either filter. It is what an
    older status file's missing upstreams_configured falls back to, and it is
    the whole signal separating a dropped upstream from a legitimately
    upstream-less (hermes-only) gateway, so cleaning bounds what is printed and
    never what was configured.

Evidence

Three new tests in test/core/status-gateway-idle.test.js fail on master and
pass with the fix, one per way a name can be hostile:

  • a hostile upstream name cannot drive the terminal from the warning - before
    the fix, a raw ESC [ 2 K erase-line sequence and an embedded newline reach
    diag.message, which is enough to forge a plausible extra status line.
  • an unbounded upstream name is clamped in the warning - before the fix, all
    5000 characters are printed.
  • an unbounded number of upstream names is capped, and the rest counted -
    before the fix, all 50 names are spelled out.

A fourth (an older status file counts every name it holds, capped or not) is a
guard, green either way: it pins that neither the cap nor the sanitizer may
revise the count.

npm test (3784 pass, 0 fail) and npm run typecheck are clean.

Deliberately not fixed here

This is based on master, not on the #678 / #658 stack, so it can land on its
own. The other two items in #680 are not master-fixable and are left:

Once the stack merges, gatewayDroppedUpstreams (which replaces the reader
touched here) should pick up printableUpstreamNames in place of its bare
stringList(details.upstreams); the sanitize-on-read then covers the
droppedUpstreamConsequence message too.

Fixes#680

…rust (#680)
`gateway_idle_no_upstreams` lifts the dropped upstreams' names out of
`status.json` and joins them into the warning it prints, filtering only
for "non-empty string". `recentEntrypointsFromSources` reads the same
file, in the same module, on the way to the same terminal, and runs
every label through `sanitizeLabel` and a count cap first, because
`status.json` is a file and core cannot assume the daemon that wrote it
was this version, this build, or well behaved (LLP 0164).
Give the upstream names the same treatment: each one cleaned through
`sanitizeLabel`, the printed list capped at 8, and the names held back
counted in the line (`, +N more`) so a sampled list never reads as a
complete one. The count keeps coming off the raw list, before either
filter, since it is what an older status file's missing
`upstreams_configured` falls back to and it is the whole signal that
separates a dropped upstream from a legitimately upstream-less gateway:
cleaning bounds what is printed, never what was configured.
Co-Authored-By: Claude <noreply@anthropic.com>
…g, and the count guard only half pinned the count
Two findings from reviewing this PR's own claim.
1. `listen_fallback_from` is the same defect, twenty lines up.
`gatewayIdleWithConfiguredUpstreams` was not the only reader in this
module joining an unsanitized `status.json` string into a diagnostic:
`gatewaySourceDetails` filters `details.listen_fallback_from` for
"non-empty string" and nothing else, and `gateway_port_fallback`
interpolates it into both its message and its repair line. It has the
same provenance as an upstream `name` (config-authored, arriving
through a file this build did not necessarily write) and the same
destination (a terminal), so an ESC erase-line sequence, a newline, or
5000 characters reach the operator's screen from it. The PR's own
stated principle - two paths reading one file should not disagree
about whether it is trusted - reaches three paths, so the third is
cleaned through `sanitizeLabel` too, and a value that sanitizes away
entirely falls back to the generic antecedent exactly as an absent one
already did.
`host`, read two lines above it, is deliberately left alone: it is not
display-only (it composes the endpoint `attach` writes into client
settings files), so clamping it could turn a long-but-legal host into
a truncated wrong one. That is a separate change with a wider blast
radius, not this one.
2. The count guard pinned the cap and not the sanitizer. `an older status
file counts every name it holds, capped or not` held 20 well-formed
names, so it caught a `total` taken after the cap but not one taken
after `sanitizeLabel`: deriving the count from printable names only
left all 14 tests green. Two of the 20 names now sanitize away, so the
test fails for either mutation, which is what it claims to pin.
npm test: 3787 pass, 0 fail. npm run typecheck: clean.
Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Verdict: findings (2 actionable, both fixed on the branch), 2 left with a reason

The change is right and the reasoning behind it holds. Verified independently
rather than taken on trust: with only src/core/daemon/status.js reverted to
its pre-fix content and the new tests kept, the three claimed tests fail
(50 !== 8 on the cap, the raw erase-line sequence and newline reaching
diag.message, the whole 5000-character name printing) and the fourth stays
green either way, as claimed.

sanitizeLabel is sufficient for what is asked of it. UNSAFE_LABEL_CHARS
covers all of C0/DEL/C1 in one range, so 0x1b (ESC), CR, BS and the 8-bit
CSI/OSC/APC introducers (0x9b/0x9d/0x9f) are all stripped, not only the
one sequence the test drives, alongside U+2028/U+2029, bidi controls and
zero-width formatting. Nothing that can open an escape sequence or start a
second line survives it.

Cap semantics check out at the boundaries. The names.length === MAX test sits
at the top of each iteration, so exactly 8 are collected; hidden = raw.length - names.length counts cap-dropped and sanitize-dropped names in one
number, so at 8 names hidden is 0 and no , +0 more appears, at 9 it is 1,
and at 50-with-2-unprintable it is still 42. Deriving total from the raw list
is correct, and is a faithful preservation of the pre-fix fallback, which was
also the count of non-empty strings.

Finding 1 (medium, fixed): the same module prints a second raw status-file string

src/core/daemon/status.js:122 (pre-fix) - gatewaySourceDetails filtered
details.listen_fallback_from for "non-empty string" and nothing else, and
gateway_port_fallback interpolated it into both its message
(src/core/daemon/status.js:705) and its repair line
(src/core/daemon/status.js:706). Same provenance as an upstream name
(config-authored, arriving through a file this build did not necessarily
write), same destination (a terminal). So the fix as submitted was incomplete
against its own stated principle: two paths reading one file should not
disagree about whether it is trusted, and there were three.

Fixed: listen_fallback_from now goes through sanitizeLabel, and a value that
sanitizes away entirely falls back to the generic "its default listen address"
antecedent exactly as an absent one already did. Three tests added in
test/core/status-gateway-fallback.test.js (hostile, unbounded,
sanitizes-away); all three fail against the pre-fix status.js and pass now.

Finding 2 (low, fixed): the count guard pinned the cap but not the sanitizer

test/core/status-gateway-idle.test.js:296 - the guard held 20 well-formed
names. Mutating printableUpstreamNames to derive total from the sanitizable
names rather than the raw ones left all 14 tests green, so half of what the
guard claims to pin ("neither the cap nor the sanitizer may revise the count")
was in fact unpinned. Fixed: two of the 20 names are now zero-width runs that
sanitize away, and the test additionally asserts +12 more, so it fails for
either mutation. Re-ran the same mutation against the strengthened test: it now
fails, as it should.

Left, with a reason

  • details.host (informational, src/core/daemon/status.js:119). Also raw
    off the status file, and it composes the endpoint hyp status prints. Left
    alone deliberately: unlike the two above it is not display-only, it feeds
    endpointFromListen and the endpoint attach writes into client settings
    files, so sanitizeLabel's 120-character clamp could turn a long-but-legal
    host into a truncated wrong one. Bounding it is a separate change with a
    wider blast radius. Noted in a comment beside the fix so it does not read as
    an oversight.
  • daemonStatusFile.sources (informational, src/core/daemon/status.js:641).
    With no runtime attached, raw SourceSnapshots go from the status file into
    report.sources and are printed at src/core/commands/status.js:327 as
    - ${s.name} (${s.plugin}) [${s.state}]. Same class of exposure, but
    name/plugin are identity keys as well as display strings (they index
    runtime.sources.started(name)) and are part of the --json contract, so
    cleaning them is its own decision rather than a widening of this one. Worth a
    follow-up issue.

On the @ref

LLP 0164#status-reads-it-from-the-status-file exists as an explicit
<a id="..."> anchor, and its body says what the gloss claims: the sanitizing
and the cap "are deliberately duplicated from the gateway rather than delegated
to it: status.json is a file, and core cannot assume the daemon that wrote
it was this version, this build, or well behaved ... all three ways a label can
be hostile are answered at that last point before render." [constrained-by]
is the right relation, the gloss is honest, and no LLP edit is needed: this
applies a settled principle to a further read path rather than revising it. The
Finding 1 fix cites the same anchor for the same reason.

Merge base

The PR is behind origin/master by one commit (#679, 4fb95dc), which does
touch src/core/daemon/status.js - but only the LLP 0200 folderAsk read at
line ~877, nowhere near anything here. git merge origin/master on the branch
is clean. Flagged for the merge-base rung, not as a review finding.

Checks

npm test: 3787 pass, 0 fail, 6 skipped. npm run typecheck: clean. All 9 CI
checks green on 2b799c1.

…g a JSON parse error that quotes the file back
The sanitize-on-read this PR applies to the idle warning's upstream names,
and round 1 applied to `listen_fallback_from`, is a property of *reading
status.json* rather than of any one detail. Enumerating every route from that
file to the terminal turns up four more that were still raw, all of them in
the text renderer rather than the collector:
- `daemon.state` and `daemon.mode`, taken straight off the snapshot
(`collectHypAwareStatus` reads `mode` from it whenever the pid file did not
already supply one) and printed as `state=` / `mode=`.
- `sources[]` and `sinks[]`, which with no runtime attached come off the file
verbatim and are printed as `- name (plugin) [state]` and
`- instance (plugin, kind)`. Round 1 left `sources[]` on the grounds that
`name` / `plugin` are identity keys and part of the `--json` contract. That
reason is real, but it argues against cleaning them *in the collector*, not
against cleaning them on the way into a terminal - and it does not reach
`state`, `kind`, or `sinks[]` at all, which are display and nothing else.
- `daemon.error`, which is the sharpest of the four: a `status.json` that is
not valid JSON reaches the line as `JSON.parse`'s own message, and V8 quotes
an excerpt of its input back verbatim. A file whose first bytes are an
erase-line sequence and a newline puts both on the terminal.
All four are cleaned at the interpolation in `renderStatusText` /
`describeDaemon` rather than in the collector, so `--json` stays byte for byte
what it was (a consumer escapes for itself, and `sources[].name` /
`sinks[].instance` stay usable as keys), and the raw values the provenance
lookups match on are untouched. `error=` gets a wider bound than a label's
120: it carries a real message naming a full path, and truncating that is the
one way this cleaning could cost a reader an answer.
Four tests in `test/core/status-text-status-file-labels.test.js`, each failing
against the pre-fix renderer. They assert over the whole C0/DEL/C1 range
rather than the one sequence each case drives, and pin the forged newline
directly: one snapshot entry must render as one line.
Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Verdict: findings (1 actionable, fixed on the branch, 33c3934)

Round 1 fixed a third path after the author fixed the first. It was right that
the fix was incomplete, and right about sanitizeLabel being sufficient and
about the cap arithmetic. But it never enumerated the module systematically -
it spot-checked, found listen_fallback_from, and stopped. A full enumeration
turns up four more routes from status.json to the terminal that were
still raw, one of which is sharper than anything found so far.

Path enumeration: status.json to terminal

Every field of DaemonStatus that collectHypAwareStatus lifts into the
report, and what the text surface does with it. This is the evidence the sweep
is now complete.

#FieldRoute to terminalBefore this roundNow
1sources[].details.recent_entrypoints[].entrypoint / .client_namerecentEntrypointsFromSources to recent clientssanitized + capped 32 (pre-existing, LLP 0164)unchanged
2sources[].details.upstreams[]gateway_idle_no_upstreams messagefixed by this PR (sanitize + cap 8 + +N more)unchanged
3sources[].details.listen_fallback_fromgateway_port_fallback message and repair linefixed in round 1unchanged
4statedaemon: line, state=RAWprintable()
5modedaemon: line, mode= (when the pid file supplied none)RAWprintable()
6(the file's own bytes)daemon: line, error=, via JSON.parse's messageRAWprintable(_, 400)
7sources[].name / .plugin / .statesources: line, with no runtime attachedRAW (round 1 left it)printable()
8sinks[].instance / .plugin / .kindsinks: line, with no runtime and no configured sinksRAW (round 1 never enumerated it)printable()
9sources[].details.hostendpointFromListen, not a display stringleft (see below)unchanged
10sources[].details.port, .listening, .listen_fallback, .upstreams_configurednumeric/boolean, type-checked at the readn/an/a
11runId--jsonrun_id only, never textn/an/a
12sinks[].lastTickAt / .lastSuccessAt--json only, never textn/an/a
13pid (pid file, not status.json)daemon: line, pid=readPidFile rejects a non-number (pid.js:68)n/a

Nothing else in DaemonStatus reaches renderStatusText. Rows 4-8 are the
finding.

Finding 1 (medium, fixed): four more status-file strings printed raw

src/core/commands/status.js:327 (sources), :336 (sinks), :574 / :576 /
:577 (describeDaemon), all pre-fix line numbers.

Reproduced, not reasoned about. Writing a status.json whose sources[0].name
is gw + an ESC erase-line sequence + a newline + hyp: all good, then
rendering the report through renderStatusText, puts four output lines
carrying a raw ESC-[2K and an embedded newline
on stdout - the exact
forged-status-line attack this PR fixes for upstream names, from the same file,
in the same command.

Row 6 is the sharpest and was invisible to everyone so far, because it is not a
field at all. A status.json that is not valid JSON never becomes a
DaemonStatus; it surfaces as daemon.error, which is JSON.parse's message

  • and V8 quotes an excerpt of its input back verbatim. With a status file
    whose bytes are x + ESC-[2K + newline + hyp: all good:
daemon.error: "Unexpected token 'x', \"x<ESC>[2K\nhyp: all good\" is not valid JSON"
daemon: not installed, running, pid=..., mode=foreground, error=Unexpected token 'x', "x<ESC>[2K
hyp: all good" is not valid JSON

So a status file needs no valid structure whatsoever to reach the terminal. It
just has to be malformed in the right place.

On round 1's stated reason for leaving sources[]. It said name /
plugin are identity keys and part of the --json contract, so sanitizing
them is its own decision. That is a real constraint and I kept it - but it
argues against cleaning them in the collector, not against cleaning them on
the way into a terminal, and the two are separable. It also does not reach
state, kind, or sinks[] at all, which are display strings and nothing
else. So the reasoning was sound as far as it went and stopped one step short
of the fix it implied.

The fix cleans at the interpolation in renderStatusText /
describeDaemon, not in the collector:

  • --json is byte for byte what it was. A consumer escapes for itself, and
    sources[].name / sinks[].instance stay usable as keys. A test asserts
    the raw value still comes out of renderStatusJson.
  • The raw values isCentralPlugin / isCentralSink match on are untouched,
    so provenance tags cannot break.
  • error= gets a 400-char bound rather than a label's 120. It carries a real
    message naming a full path, and truncating that is the one way this cleaning
    could cost a reader an answer - the same argument round 1 used to leave
    host alone, applied where it actually bites.

Four tests in test/core/status-text-status-file-labels.test.js. All four
fail against the pre-fix renderer and pass after
(verified by reverting only
src/core/commands/status.js: # pass 0 # fail 4, then restoring:
# pass 4 # fail 0). They assert over the whole C0/DEL/C1 range rather than
the one sequence each case drives, and they pin the forged newline directly -
one snapshot entry must render as one line - rather than only asserting the
absence of a byte.

Focus items checked and found sound

Round 1's generic antecedent is not misleading.listen_fallback_from
only reaches the ?? 'its default listen address' branch when it sanitizes
away entirely, i.e. the value was nothing but control/invisible characters.
Any address with one printable character survives (an address followed by an
ESC sequence renders as the address plus the sequence's printable residue), so
there is no case where a real address exists and the message claims a default.
The repair line says "free its default listen address", which is generic but
not false.

Informational, pre-existing, not fixed: the generic branch produces
"the gateway's default listen its default listen address was taken at
boot", which is garbled. The ?? is on master unchanged (status.js:655
there), so this PR did not introduce it - but it did widen when the branch
fires, and the new test at status-gateway-fallback.test.js:159 now pins the
garbled phrasing. Rewording is LLP 0114's message and belongs in its own change.

Round 1's three fallback tests do not have the blind spot F2 fixed. They
drive genuinely hostile input (an erase-line sequence plus newline, a
5000-char value, a pure zero-width run) and assert over the whole control
range, not the one sequence. They are pinning what they claim.

The cap arithmetic holds at every boundary. Probed directly rather than
read:

raw namesprintedrendered
exactly 88no +0 more
exactly 98+1 more
12, of which 5 zero-width, interleaved7+5 more
3, all zero-width03 upstreams are configured, no parenthetical
0 names, upstreams_configured: 202 upstreams are configured

hidden = raw.length - names.length adds up in every case, and no path prints
a sampled list that reads as complete.

The all-sanitize-away row deserves a note, since the code comment at
status.js:731 says the withheld names "are counted rather than dropped
silently" and here the +3 more does not appear (named is gated on
names.length > 0). This is benign, not a bug: with no list printed there
is no sampled list to misread, and the three names are already counted - they
are the 3 that leads the sentence, because total comes off the raw list.
Nothing is under-reported. Left as is.

Left, with a reason

  • details.host (src/core/daemon/status.js:119). Round 1's reasoning is
    correct and I kept it: host feeds endpointFromListen, and the endpoint
    attach writes into client settings files, so sanitizeLabel's clamp could
    turn a long-but-legal host into a truncated wrong one. It is not
    display-only. Note the row-6 fix above adopts the same reasoning for
    error=.
  • Everything reached from files other than status.json.activePlugins
    and layered.drops (config), remoteConfig etags and rollback reasons
    (config-control/), clientActions reasons and request keys (the marker
    store) all print raw too. Same class of exposure, different files, and on a
    joined host some of it is remote-authored - which makes it a wider decision
    than "what core reads back out of status.json", the principle this PR and
    LLP 0164 are about. Deliberately out of scope; worth its own issue.

On the @ref and the LLP

The new printable() cites LLP 0164#status-reads-it-from-the-status-file
[constrained-by], the same anchor (verified present as an explicit
<a id>) for the same reason as rounds 0 and 1: this applies a settled
principle to further read paths rather than revising it. No LLP edit needed,
and none made - LLP 0164 is Accepted.

Checks

npm test: 3791 pass, 0 fail, 6 skipped. npm run typecheck: clean. All 9 CI
checks green on 33c3934. No semicolons, no em dashes, no @typedef, no
inline import('...') types; the new test file uses a root-anchored @import.

Merge base

Unchanged from round 1: behind origin/master by #679 (4fb95dc), which
touches status.js only at the folderAsk read. Clean merge. The
merge-base rung's business, not a review finding.

This was round 2; the ladder caps here.

@philcunliffephilcunliffe added the neutral:stuck neutral attempted this but cannot complete it autonomously — needs a human label Aug 8, 2026
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Triage: held (neutral:stuck), the sanitization sweep is not complete

Neutral was triaging this PR after the two-round review ladder capped: verify
every review finding is resolved at head 33c3934, judge whether the
status-file-to-terminal sweep is credibly complete, and either clear the PR
with a follow-up issue or hold it. The verification passed; the completeness
judgement did not.

What checked out (verified against the committed tree, not the prose)

Every finding from both review rounds is resolved at head:

  • Round 0/1: printableUpstreamNames (sanitize + cap 8 + +N more, count off
    the raw list) at src/core/daemon/status.js:187-202;
    listen_fallback_from through sanitizeLabel at
    src/core/daemon/status.js:130; the strengthened count guard in
    test/core/status-gateway-idle.test.js.
  • Round 2: printable() at every text interpolation in
    src/core/commands/status.js (sources line 364, sinks 373,
    state/mode/error at 619-622 with the 400-char error= bound);
    --json untouched; all four tests in
    test/core/status-text-status-file-labels.test.js present. All 9 CI checks
    green on head.

Round 2's DaemonStatus enumeration is also internally sound: the fields its
table omits (warnings, startedAt, healthyAt, stoppedAt, uptimeMs)
are never lifted into the report by collectHypAwareStatus, report.configPath
comes from env resolution (src/core/daemon/status.js:379), not the status
file, and the status file's sources[].error reaches only --json. As a claim
about renderStatusText, "nothing else reaches it" is true.

The blocker: hyp daemon status is a whole surface no round enumerated

Round 2's table proves completeness for hyp status. But hyp daemon status
(runDaemonStatus, src/core/commands/daemon.js:62-108, registered at
src/core/cli/core_commands.js:490) reads the same status.json
(readStatusFile, which is a bare JSON.parse plus an is-object check and a
cast, no field validation, src/core/daemon/status.js:87-96) and prints it to
the same terminal, raw:

  • src/core/commands/daemon.js:86: status.state raw.
  • :87-90: status.pid, startedAt, healthyAt, stoppedAt raw
    (pid is unvalidated too: a string pid carrying ESC bytes prints verbatim).
  • :80 + :91: status.uptimeMs raw when no live process (unvalidated, so a
    hostile string prints verbatim).
  • :97: sources[].name, .plugin, .state, and .error raw. .error is
    free text, the widest single field on either surface, and this PR's own
    hyp status deliberately keeps it out of its text surface.
  • :105: sinks[].instance, .plugin, .kind raw.
  • :67: readStatusFile's throw is uncaught, so a status.json that is not
    valid JSON surfaces via src/core/cli/dispatch.js:479
    (hyp daemon-status: ${err.message}) as JSON.parse's message, and V8
    quotes the file's bytes back verbatim. That is exactly the round-2 "row 6"
    vector, still live one command over.

Why this is a production risk rather than residue: it is the same file, the
same forged-status-line / erase-line terminal-injection vector, and the same
victim terminal this PR exists to close, reachable through a sibling command
of the one being fixed. The stated principle of the fix, "two paths reading
one file should not disagree about whether it is trusted", is precisely what
still fails: after merge, hyp status treats status.json as untrusted while
hyp daemon status trusts it completely. Both prior rounds classified this
identical exposure (medium) as actionable when they found it on other paths,
and fixed it; the ladder is capped, so triage cannot send it back for a
round 3, and merging on a "sweep complete" statement would hand the human a
security claim that is not true of the terminal, only of one command.

Completeness judgement, stated plainly for the merge decision: the sweep is
not credibly complete.
Round 2's enumeration was systematic but scoped to
renderStatusText; the fourth look found another whole surface, continuing
the pattern (each of three prior looks also found more). No claim of
completeness should be relied on until hyp daemon status is either swept
into the same printable() treatment or explicitly ruled out of scope by a
human.

Non-blocking residue (would go to a follow-up issue on clearing)

  • details.host (src/core/daemon/status.js:119): deliberately deferred by
    both rounds with sound reasoning (not display-only, it composes the endpoint
    attach writes into client settings; clamping could corrupt a legal host).
    Annotated in code. Not a blocker.
  • Strings from files other than status.json (config-layer drops, remote
    config etags/reasons, client-action reasons, and the client attach probe
    error printed at src/core/commands/status.js:398): same exposure class,
    different files, some remote-authored on a joined host. Round 2 scoped these
    out with a reason; a follow-up issue is the right home.
  • The garbled generic-antecedent phrasing in gateway_port_fallback
    ("default listen its default listen address"): pre-existing on master,
    pinned by a test, wording belongs to LLP 0114's message. Preference.

Decision needed

One of:

  1. Extend this PR (or stack one commit on it) to route
    runDaemonStatus's interpolations through the same
    sanitizeLabel/printable() treatment and catch the readStatusFile
    throw (bounded like error=), with the same style of hostile-input tests.
    Then this triage clears.
  2. A human explicitly rules hyp daemon status out of this PR's scope,
    accepts the "complete for hyp status only" framing, and merges with a
    follow-up issue covering it plus the residue above.

Reply on this PR or push to fix/issue-680; neutral monitors the thread and
re-engages next tick.

@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Decision on the triage question: option 2.

Ruling hyp daemon status out of this PR's scope. Accept the "complete for hyp status only" framing and merge, with a follow-up issue covering:

  • sweeping runDaemonStatus's raw interpolations (state, pid, timestamps, uptimeMs, sources/sinks fields, and the uncaught readStatusFile throw) through the same sanitizeLabel/printable() treatment with hostile-input tests, and
  • the non-blocking residue already listed in the triage note (strings from files other than status.json, including remote-authored config etags/reasons and the client attach probe error).

Please file that follow-up issue as part of clearing this.

@philcunliffephilcunliffe removed the neutral:stuck neutral attempted this but cannot complete it autonomously — needs a human label Aug 14, 2026
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Re-engaging with your guidance - taking option 2: hyp daemon status is out of this PR's scope, the "complete for hyp status only" framing is accepted, and the follow-up you asked for is filed as #776 (covering the runDaemonStatus interpolation sweep with hostile-input tests, the uncaught readStatusFile throw, and the non-blocking residue from the triage note).

The stuck label is removed. Neutral re-runs the rung at the current head on its next tick with this decision as input.

master reworked the same warning while this branch was open: the idle-only
`gatewayIdleWithConfiguredUpstreams` became `gatewayDroppedUpstreams`, which
also covers a bound gateway's partial loss and splits the dropped names into
"covered by a preset" and "silent" against `registered_presets` (#676/#678).
Both intents kept. The names are still read out of a file core does not
trust, so they are still cleaned through `sanitizeLabel` and capped at 8 with
the remainder counted, but the cleaning moved to each point one is rendered
rather than to the collector. `gatewayDroppedUpstreams` now hands back the
file's raw names on purpose: the configured/dropped counts and the
preset intersection are decided off them, and cleaning or capping there
would silently change which names `attributeDroppedUpstreams` calls silent.
The same rendering helper covers master's new bound-gateway sentences, which
were printing status-file names to the terminal raw.
test/core/status-gateway-idle.test.js was an append collision on both sides;
master's file plus this branch's four cases, unchanged.
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Triage: cleared at 3a2944e (Path B, residue tracked in #776)

The maintainer's option 2 ruling is in effect: hyp daemon status is out of
this PR's scope, the "complete for hyp status only" framing is accepted, and
the follow-up it required exists as #776 (open, neutral:fix, with PR #777
already implementing it). #776 covers exactly what the ruling listed: the
runDaemonStatus raw interpolations (state, pid, timestamps, uptimeMs,
sources/sinks fields), the uncaught readStatusFile throw, hostile-input
tests, and the strings-from-other-files residue (remote config etags/reasons,
client attach probe error). Nothing unresolved on this PR is a production
defect within its ruled scope.

Every review finding verified at head, in the committed tree

  • Round 0/1, printableUpstreamNames: present at
    src/core/daemon/status.js:193 (sanitize, cap 8, +N more, count off the
    raw list). Four idle-branch tests in test/core/status-gateway-idle.test.js
    including the strengthened count guard (20 names, 2 zero-width, asserts
    +12 more).
  • Round 1, listen_fallback_from: through sanitizeLabel at
    src/core/daemon/status.js:132; the three tests in
    test/core/status-gateway-fallback.test.js are present.
  • Round 2, the four raw renderStatusText / describeDaemon routes:
    printable() at the sources line (src/core/commands/status.js:367), sinks
    line (:376), and state / mode / error (:636-639, with the 400-char
    error= bound); --json untouched. All four tests in
    test/core/status-text-status-file-labels.test.js are present.
  • Triage blocker (the hyp daemon status surface): resolved by the scope
    ruling, tracked in Sweep hyp daemon status interpolations through sanitizeLabel, plus #681's non-blocking residue #776; src/core/commands/daemon.js is untouched by this
    PR, as ruled.

npm test at head: 4094 pass, 0 fail, 1 skipped. npm run typecheck: clean.
All 9 CI checks green on 3a2944e.

The merge extension, judged on its own

The conflict resolution with master's gatewayDroppedUpstreams rework made
two decisions that were never separately reviewed. Both were verified here by
reading the head tree and by driving hostile status files through the real
collector, not from the prose:

  1. The collector stays raw; cleaning moved to render time. Correct, and
    necessary rather than stylistic: attributeDroppedUpstreams gates on
    names.length === dropped (src/core/daemon/status.js:321) and intersects
    the names with details.registered_presets, so sanitizing or capping
    inside the collector would silently change which upstreams report as
    silent vs covered. Probed directly: a dropped name carrying ESC bytes that
    is also (byte for byte) a registered preset still attributes as covered,
    and the rendered sentence is clean.
  2. Cleaning extended to master's bound-gateway sentences (the
    (dropped: ...) label, the silent list, the covered list). Correct:
    these print the same file's names to the same terminal, and leaving them
    raw would reintroduce this PR's exact bug one branch over. Verified: an
    ESC-erase-line-plus-newline name renders with the control bytes stripped
    and the printable residue kept, a zero-width-only name is withheld and
    counted (+1 more), and every grammar choice (name vs names, is vs are)
    comes off the file's sets (silent.length, covered.length,
    names.length), never the printed list, so cleaning cannot make a plural
    set read singular. The counts leading each sentence are the file's own.

The N unprintable copy fires only when every name in a list sanitizes to
nothing. Probed through the real path: the idle line renders
3 upstreams (3 unprintable) are configured and the silent clause renders
under the names 2 unprintable ... those names, both with the true count and
plural grammar intact. Slightly awkward in the silent clause, but unambiguous,
never misleading, and strictly better than the empty list (which would read as
"no names in the file") or the old silent omission. Acceptable.

Non-blocking residue, for the record

  • Tracked in Sweep hyp daemon status interpolations through sanitizeLabel, plus #681's non-blocking residue #776: the runDaemonStatus sweep, the readStatusFile throw,
    and the strings-from-other-files exposure. A duplicate issue was not filed.
  • details.host (src/core/daemon/status.js): deliberate non-fix, annotated
    in code; it feeds endpointFromListen and the endpoint attach writes, so
    clamping could corrupt a legal host. A settled decision, not a deferral.
  • The garbled generic-antecedent phrasing in gateway_port_fallback
    ("default listen its default listen address"): pre-existing on master,
    pinned by a test, wording belongs with LLP 0114's message. Preference.
  • Test-coverage note: the bound-branch call sites of printableUpstreamNames
    and the N unprintable copy have no committed hostile-input test (the four
    committed tests drive the idle branch only). The shared mechanism is
    pinned, master's existing bound-branch tests pin grammar-from-raw, and this
    triage verified the extension empirically, so this is a preference; a
    natural home is alongside Sweep hyp daemon status interpolations through sanitizeLabel, plus #681's non-blocking residue #776's hostile-input tests if desired.

No blocker remains. Marker references #776 per Path B; no merge action taken
by neutral.

@philcunliffephilcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 14, 2026
philcunliffe pushed a commit that referenced this pull request Aug 17, 2026
One conflict, in src/core/commands/status.js: both sides added an import
line at the same spot. #792 (proxy-mode trust) added `ENV_VAR_NAME` from
`../daemon/launchd_env.js`; this branch added `sanitizeLabel` from
`../util/json_util.js`. Both kept.
The `printable()` collision this PR anticipated did not happen: #681 has
not landed, so `src/core/commands/status.js` still holds exactly one
`printable()` and one `MAX_ERROR_CHARS` (400), and `src/core/commands/
daemon.js` holds its own single pair. Label sites use `sanitizeLabel`'s
120 default, error sites pass 400.
Both intents survive unchanged: this branch's cleaning of every
interpolation it targets, including the collector-assembled
`remote_config_rolled_back` message whose cleaned prose reaches `--json`
while `remote_config.last_rollback` beside it stays byte-exact, and
#792's `proxy trust` text block plus tri-state `proxy_trust` JSON key,
which stays raw and absent off-darwin exactly as it landed.
Conflict: the import block in `src/core/commands/status.js`. This branch
added `sanitizeLabel` (the render-time cleaning for the status-file strings
the text surface interpolates); master's #792 added `ENV_VAR_NAME` for the
`proxy trust` block's launchd line. Independent, so both imports stay.
Everything else auto-merged and was checked rather than assumed. The
collector stays raw: `gatewayDroppedUpstreams` still reads
`details.upstreams` and `details.upstreams_dropped_names` through
`stringList` alone, so `attributeDroppedUpstreams` still intersects
unsanitized names with `registered_presets` and its
`names.length === dropped` precondition still counts the file's names.
`printableUpstreamNames` still derives `+N more` from the raw list's
length, and each message's singular/plural still comes off the raw
`silent` / `covered` arrays, so cleaning cannot make a plural set read
singular. #792's `proxy trust` block, its tri-state `null`-when-unprobed
fields, and its absence off darwin or with no CA are untouched.
npm test: 4194 tests, 0 fail (baseline origin/master: 4183, 0 fail).
npm run typecheck: clean on both.
Co-Authored-By: Claude <noreply@anthropic.com>
@philcunliffephilcunliffe removed the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 17, 2026
@philcunliffe

Copy link
Copy Markdown
ContributorAuthor

Re-triage after the master merge: cleared at 8ec0a8c (Path A, residue still #776)

Master gained #789 and #792 (and #782) since the 3a2944e clearance, and the
conflict was resolved by merge commit 8ec0a8c. This re-triage verified the
resolution against ground truth rather than trusting the resolver's report.

The merge itself, verified mechanically

The load-bearing invariants, confirmed from the head tree

  • Collector stays raw.gatewayDroppedUpstreams
    (src/core/daemon/status.js:263) builds names and droppedNames via
    stringList with no sanitizing, and attributeDroppedUpstreams (:323)
    still gates on names.length !== dropped (:331) and intersects the
    file's own bytes with registered_presets (:333). Sanitizing before this
    point would silently flip silent vs covered; it does not happen.
  • Attribution orientation is unflipped.covered is the preset-name
    intersection, silent the complement (:334-335), and the renderer says
    "nothing is proxied or captured" for silent (:415) and "in the routing
    table only as the adapter preset" for covered (:421). Correct on both
    sides.
  • +N more counts the file's names.printableUpstreamNames (:197)
    computes hidden = list.length - printed.length off the raw list, so both
    the 8-name cap and a name that sanitizes away entirely are counted, and an
    all-unprintable list renders N unprintable rather than empty.
  • Plural grammar reads the raw arrays. Each grammar choice comes off
    silent.length / covered.length / names.length / dropped
    (:400-421, :948-950), never the printed list.
  • hyp status reports proxy-mode trust and the launchd env (LLP 0237/0239) #792 survives intact.collectProxyTrust (:1279) returns null off
    darwin or with no CA, and trusted / launchdEnvSet stay tri-state
    (null when the probe could not run). The proxy trust text block and the
    proxy_trust JSON key with its null-not-omitted contract are as hyp status reports proxy-mode trust and the launchd env (LLP 0237/0239) #792
    landed them.

Evidence

  • CI at head 8ec0a8c: all 9 checks green (test and typecheck on node 22 and
    24, duplicate-numbers).
  • Locally in a clean worktree at head: the status test files
    (status-gateway-idle, status-gateway-fallback,
    status-text-status-file-labels, status-proxy-trust) all pass.
  • Body re-read after the marker edit: the prior 3a2944e marker, the new
    8ec0a8c marker, and the Fixes #680 trailer are all present.

Merge-ordering note for the maintainer (not a finding)

The printable() and error-cap helper in src/core/commands/status.js at
this head is #681's own (round 3). Sibling PR #777 (implementing #776) also
carries a printable() and a MAX_ERROR_CHARS = 400 in the same file after
its own conflict resolution. Both PRs are open; whichever merges second will
conflict on that block and the resolver must keep exactly one copy of the
helper. This is expected duplication between two open siblings, not a defect
in either.

No new finding beyond what #776 already tracks. Path A: marker appended for
8ec0a8c referencing #776; no merge action taken by neutral.

@philcunliffephilcunliffe added the neutral:approved neutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030) label Aug 17, 2026
@philcunliffe
philcunliffe merged commit fea9efe into masterAug 17, 2026
9 checks passed
@philcunliffe
philcunliffe deleted the fix/issue-680 branch August 17, 2026 21:44
philcunliffe added a commit that referenced this pull request Aug 18, 2026
…tag and probe-error residue from #681 (#776) (#777)
* hyp daemon status cleans what it reads out of status.json, and the two files hyp status still printed raw (#776)
`hyp daemon status` interpolated every field of `status.json` straight into
the terminal: `state`, `pid`, the timestamps, `uptime_ms`, and each
`sources[]` / `sinks[]` entry. `hyp status` already cleans what it reads back
out of that same file at the last point before render (LLP 0164), for a
reason that is a property of the file and not of one command: `status.json`
is a *file*, and core cannot assume the daemon that wrote it was this
version, this build, or well behaved. Nothing validates a field on read, so
every one of them was a way to repaint the operator's screen or forge a
plausible extra status line.
The same read also threw straight out through the CLI. A `status.json` that
is not valid JSON reached `JSON.parse`, whose message quotes an excerpt of
the input verbatim, and surfaced as a raw stack trace: useless as a
diagnosis, and the one path on which the file reached the terminal entirely
unfiltered. A well-formed file of the wrong shape got a `TypeError` off
`status.sources.length` instead.
Cleaning happens at each interpolation, never in the reader: the same read
feeds `--json`, which is the machine copy and stays byte-exact (LLP 0225).
Two more files reach `hyp status`'s text surface and were printed raw:
`config-control/state.json` and its etag sidecar (remote-authored etags, plus
reasons and timestamps that are this build's vocabulary only if this build
wrote the file), and a client's own settings file by way of the attach
probe's `error`, which for an unparseable settings file is `JSON.parse`
quoting that file back. Both are display-only where they are printed, so
both are cleaned at the render and left alone under `--json`.
Co-Authored-By: Claude <noreply@anthropic.com>
* Say what the rollback diagnostic actually does to --json, and pin it
Review round 1 found the PR's stated invariant false. The
`remote_config_rolled_back` message is assembled in the collector out of
`sanitizeLabel`'d components, and `renderStatusJson` maps that message
straight through, so `--json` carries the cleaned prose too: the
newline in a hostile `reason` closes up, and each component is clamped
at 120. The claim that all of it is "cleaned there and left alone under
`--json`" held for the block beside it, not for the diagnostic.
The behaviour is kept as it is. The security posture is the same either
way, the prose line is not a parsing surface on either render, and the
unedited values sit one key away at `remote_config.last_rollback`, which
stays byte-exact. Moving the clean to the single `${d.message}`
interpolation in `renderStatusText` would keep the prose raw for
`--json`, but it needs one clamp width that holds for every diagnostic
kind, which is a wider decision than #776.
So: correct the comment to state what the code does, and close the test
gap that let the false claim through. The existing `--json` guard seeds
`probation`, which only ever reaches the text render, so it could never
have caught this. Two new tests seed `last_rollback` and assert what
`--json` really contains, prose and values. Both fail on master.
Co-Authored-By: Claude <noreply@anthropic.com>
---------
Co-authored-by: test <test@example.com>
Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: test <test@test.com>
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

neutral:approvedneutral reviewed this and holds it for a maintainer merge (own or adopted PR; LLP 0025/0030)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Follow-up: deferred review findings from PR #678

1 participant

@philcunliffe