hyp daemon status cleans what it reads out of status.json, plus the etag and probe-error residue from #681 (#776) - #777
hyp daemon status cleans what it reads out of status.json, plus the etag and probe-error residue from #681 (#776)#777philcunliffe wants to merge 6 commits into
Conversation
…o 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>
Verdict: findings (1), none fixed hereReviewed One finding, and it is the exact thing focus item 1 asked me to look for: the Finding 1 (blocker for the stated invariant): the rollback diagnostic is cleaned in the collector, and that value is what
|
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>
|
Round 2 of 2. Verdict: approve, with round 1's finding resolved in this round and one follow-up deferred to triage. Reviewed Round 1's finding: confirmed at head, resolved as option (b)Reproduced against the real The message is assembled once in the collector ( Resolved the conservative way (option b): behaviour unchanged, claim corrected, contract pinned. Commit Before taking (b) I checked whether any 1. The in-source comment now says what the code does
Two incidental inaccuracies in my own first draft of that comment were corrected before commit: there is one 2. Two new tests pin the real
|
| test | asserts | on master |
|---|---|---|
hyp status --json carries the cleaned rollback prose, and the raw values beside it |
the --json message has no control byte, no zero-width run, no newline, and the stripped reason closes up (probation_expiredFORGED); remote_config.last_rollback deep-equals the raw seeded object |
fails |
a long rollback etag is clamped in the --json message but whole in the --json values |
the --json message clamps at 117 + ...; remote_config.last_rollback.etag keeps all 5000 characters |
fails |
Before/after, run in a clean origin/master worktree with only the test file copied in, then at the new head:
- against
master:# pass 1 # fail 7(the 5 the PR already claimed, plus both new ones) - at
078d762:# pass 8 # fail 0
Verification at the new head
npm test: 4099 pass / 0 fail / 1 skipped. Note the PR body's "4096 pass / 1 fail" is stale: the inheritedtwo documents numbered LLP 0223failure is gone, because Renumber the colliding prune decision from LLP 0223 to 0226 (#774) #775 landed onmasterand is in this branch's merge.npm run typecheck: clean.npm run smoke -- status_diagnostics: ok.- No em dash and no statement semicolon in any added line.
Round 1's passing items, re-verified at head
hyp daemon status --jsonis genuinely unchanged: thejsonbranch returns{ running, ...status, uptimeMs: liveUptimeMs }before anyprintablecall touches anything.hyp status --jsonstays raw for everything except the one diagnostic string above:probation.etag,last_rollback(all three fields), and the client attacherrorall come through byte-exact, now asserted by deep-equal rather than by a single field.- The collector is otherwise semantically raw;
sanitizeLabelinsrc/core/daemon/status.jsis confined torecentEntrypointsFromSources(pre-existing, LLP 0164) and this one message. - The rest of the diff's claims hold:
entryListreally does stop theTypeErroron a non-arraysources/sinks, thereadStatusFile/readPidFileguard really is the only path on which the file reached the terminal unfiltered, and the 400-character error clamp is applied at everyerrorinterpolation the PR body names.
Deferred follow-up for triage: option (a)
Not implemented here, deliberately. Move the cleaning of diagnostic messages out of the collector to the single ${d.message} interpolation in renderStatusText (src/core/commands/status.js:559), leaving --json byte-exact for diagnostics too, which is what LLP 0225 #decision states as the general rule.
The open question it needs answered first is the clamp width. A clamp at the render applies to the whole assembled sentence, for all 14 diagnostic kinds in src/core/daemon/status.js, not to each interpolated component. The longest existing template (client_attached_not_configured, src/core/daemon/status.js:964) is a ~230-character source line, so before any captured value is substituted a message is already around 200 characters of this build's own prose. That makes the existing MAX_ERROR_CHARS of 400 tight enough that a legitimate message naming a full path could be truncated, while a value-level clamp of 120 could not reach it at all. Someone has to pick that number and defend it across every kind. That is a wider decision than #776 asked for, which is why it is filed rather than done.
Classification: preference, not a blocker. The behaviour on this branch is safe (nothing hostile reaches a terminal on either surface), the values a program should read are byte-exact, and the code now describes itself accurately.
Action needed from the orchestrator, not from a reviewer
The PR body still carries the corrected claim and I cannot fix it (reviewers do not run gh pr edit). Under Item 2, the first bullet ends:
All of it is display-only at those points, so all of it is cleaned there and left alone under
--json.
That sentence is false for the remote_config_rolled_back diagnostic. Suggested replacement:
All of it is display-only at those points, so all of it is cleaned there, and the values stay byte-exact under
--json. One exception is worth stating plainly: theremote_config_rolled_backmessage is assembled in the collector out of cleaned components, so--jsoncarries that cleaned sentence atdiagnostics[].messagetoo. That is accepted rather than overlooked. The line is prose, not a parsing surface, and a program reads the unedited values one key away atremote_config.last_rollback, which is byte-exact. Cleaning instead at the single${d.message}interpolation would keep the prose raw for--json, but it needs one clamp width that holds for all 14 diagnostic kinds, which is wider than #776 and is deferred.
The Evidence section's counts are also stale twice over: test/core/status-hostile-non-status-file.test.js is now 8 tests (7 fail before, not 5 of 6), for 16 new tests across two files, 13 failing on master, and npm test is now 4099 pass / 0 fail, with the inherited LLP 0223 failure gone since #775 landed.
Triage at
|
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.
Re-triage at
|
Merge-order note: this PR and #681 each add the same
|
Re-triage at
|
hyp daemon statusinterpolated every field ofstatus.jsonstraight into theterminal, and two files other than
status.jsonreachedhyp status's textsurface raw. Both items from #776 are fixed here.
Item 1:
runDaemonStatusstate,pid,startedAt/healthyAt/stoppedAt,uptime_ms, and eachsources[]/sinks[]entry (name,plugin,state,error,instance,kind) now go through the samesanitizeLabel/printable()treatment #681applied on the
hyp statusside.hyp statuswas already cleaning what itreads back out of this 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: core
cannot assume the daemon that wrote it was this version, this build, or well
behaved, and
readStatusFilevalidates nothing beyond "is an object".pidanduptime_msare typednumberand neither is validated on read, sothey get
printableNumber: printed as themselves when the file really holds afinite number, cleaned as a label when it does not, so a wrong-typed field
still shows something rather than an empty column.
Cleaning sits at each interpolation and never in
readStatusFile, because thesame read feeds
--json, which is the machine copy and stays byte-exact(LLP 0225). This follows #681's split: sanitize where text is rendered for a
person, not where values are collected.
The uncaught
readStatusFilethrow is handled too. Astatus.jsonthat isnot valid JSON reached
JSON.parse, whose message quotes an excerpt of theinput 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. It now
exits 1 with a cleaned
hyp daemon status: <message>on stderr, the shapeevery other daemon subcommand's failures already use.
readPidFilethrows thesame way on a malformed
pid.jsonand is inside the same guard. A well-formedfile of the wrong shape used to get a
TypeErroroffstatus.sources.length;a non-array
sources/sinksnow renders(none).Item 2: the residue from #681's triage
running etag,probation,last rollbackandbad etaginrenderStatusText, plus theremote_config_rolled_backdiagnostic message built incollectHypAwareStatus. An etag is authored by whatever server the installjoined; the reasons and timestamps come back through
config-control/state.json, whichreadControlStateaccepts on "is anobject" alone, so they are this build's vocabulary only if this build wrote
the file. All of it is display-only at those points. One exception, found in review
and pinned by test: the
remote_config_rolled_backmessage is assembled oncein the collector from cleaned components, so
--jsoncarries that cleanedprose at
diagnostics[].message(stripped, each component clamped to 120).That is accepted rather than overlooked: the prose is not a parsing surface on
either render, and the structured
remote_config.last_rollbackbeside itstays byte-exact. Moving the clean to the single render interpolation would
make diagnostics byte-exact too, but needs one clamp width chosen across all
14 diagnostic kinds, so it is deferred rather than done here.
JSON,
probeClientAttachFromDescriptorreturnsJSON.parse's message, whichquotes an excerpt of the client's own file back. Cleaned at the
error:linewith the wider 400-character error clamp, so a message naming a full path is
not cut short.
Evidence
16 new tests across two files. 13 fail on
masterand pass with the fix;3 are guards, green either way. (Round 2 added two tests seeding
last_rollback, which the original guards never exercised - the gap that letthe overclaim above survive.)
test/core/daemon-status-hostile.test.js(8 tests, 6 fail before):ESC [ 2 Kand an embedded newline reach stdout, enough to forge a plausible extra status lineTypeError: Cannot read properties of undefined (reading 'length')test/core/status-hostile-non-status-file.test.js(6 tests, 5 fail before):running etag:lineremote_config_rolled_back's messageJSON.parsemessage forges an extra client rownpm test: 4096 pass / 1 fail.npm run typecheck: clean. The single failureis inherited from
master(no LLP number is claimed by two documents: twodocuments numbered LLP 0223, which #775 fixes). The
masterbaseline in thesame worktree is 4082 pass / 1 fail, the same test. Smokes run green:
daemon_foreground_start_stop,status_diagnostics,package_bin_boot,cli_bundled_plugins_activated,client_attach_idempotent,walkthrough_picker_to_first_query.Note on overlap with #681
This is based on
master, not on #681, so it can land on its own. #681 adds aprintable()helper tosrc/core/commands/status.jsfor the sources/sinks anddescribeDaemonlines; this PR adds a helper of the same name and signature tothe same file for the remote-config and client-error lines, and its own copy in
src/core/commands/daemon.js. Whichever lands second will conflict on that oneblock, and the resolution is to keep a single
printableand a single errorclamp constant (
MAX_ERROR_CHARShere,MAX_DAEMON_ERROR_CHARSthere - same400) and let both sets of call sites use them. Nothing else in the two diffs
touches the same lines.
Deliberately not changed
report.layered.drops'section/key/reason/detailin the localconfig block. They are config-authored rather than remote-authored, were not
in Upstream names in the idle-gateway warning are sanitized and capped like every other status-file label (#680) #681's triage note, and Sweep hyp daemon status interpolations through sanitizeLabel, plus #681's non-blocking residue #776 does not list them, so bringing them in would
widen the sweep past what was decided.
--jsonon either command, and thesources[].name/sinks[].instanceidentity keys the collector hands out. Cleaning stays at the render for a
person, exactly as Upstream names in the idle-gateway warning are sanitized and capped like every other status-file label (#680) #681 established.
Fixes #776