hyp daemon status cleans what it reads out of status.json, plus the etag and probe-error residue from #681 (#776)

src/core/commands/daemon.js

@@ -5,6 +5,7 @@ import { parseCommandArgv } from '../cli/verb_codec.js'
 import process from 'node:process'
 
 import { readObservabilityEnv } from '../observability/env.js'
+import { sanitizeLabel } from '../util/json_util.js'
 
 /**
  * @import { CommandRunContext } from '../../../hypaware-plugin-kernel-types.js'
@@ -56,6 +57,66 @@ export async function runDaemonRun(argv, ctx) {
   }
 }
 
+/**
+ * How much of an `error=` line a hostile status file may spend. Wider than a
+ * label's 120, because unlike a name this carries a real error message -
+ * typically an fs error naming a full path - and the clamp exists to stop the
+ * line being bloated, not to bound an identifier.
+ */
+const MAX_ERROR_CHARS = 400
+
+/**
+ * A value out of `status.json` on its way to the terminal, made safe to print.
+ *
+ * `hyp status` cleans what it reads back out of this same file at the last
+ * point before render, for a reason that is a property of the file and not of
+ * that command: `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). Nothing validates a field
+ * on read, so a raw value can carry an escape sequence that repaints the
+ * operator's screen or a newline that forges a plausible extra status line.
+ *
+ * Cleaning happens here rather than in `readStatusFile` because the reader
+ * also feeds `--json`, which is the machine copy and must stay byte-exact:
+ * escaping is a render's job, never a read's (LLP 0225).
+ *
+ * @param {unknown} value
+ * @param {number} [max]
+ * @returns {string}
+ * @ref LLP 0164#status-reads-it-from-the-status-file [constrained-by]: what core reads back out of status.json is cleaned at the last point before render, on every surface that prints it
+ */
+function printable(value, max) {
+  return sanitizeLabel(value, max) ?? ''
+}
+
+/**
+ * A field `DaemonStatus` types as a number, printed as itself when the file
+ * really holds one and cleaned as a label when it does not. `String` is safe
+ * over anything `JSON.parse` produced, so a wrong-typed field still shows
+ * *something* rather than vanishing into an empty column.
+ *
+ * @param {unknown} value
+ * @returns {string}
+ */
+function printableNumber(value) {
+  if (typeof value === 'number' && Number.isFinite(value)) return String(value)
+  return printable(String(value))
+}
+
+/**
+ * A `sources` / `sinks` entry list out of the status file. Anything that is
+ * not an array is no list at all: the walk below would throw straight out
+ * through the CLI, which is the same raw-stack failure the read is guarded
+ * against.
+ *
+ * @param {unknown} value
+ * @returns {Record<string, unknown>[]}
+ */
+function entryList(value) {
+  if (!Array.isArray(value)) return []
+  return value.filter((entry) => !!entry && typeof entry === 'object')
+}
+
 /**
  * @param {string[]} argv
  * @param {CommandRunContext} ctx
@@ -65,8 +126,23 @@ export async function runDaemonStatus(argv, ctx) {
   const { readStatusFile } = await import('../daemon/status.js')
   const { readPidFile, processIsAlive } = await import('../daemon/pid.js')
   const stateDir = readObservabilityEnv(ctx.env).stateDir
-  const status = readStatusFile(stateDir)
-  const pidEntry = readPidFile(stateDir)
+  /** @type {ReturnType<typeof readStatusFile>} */
+  let status
+  /** @type {ReturnType<typeof readPidFile>} */
+  let pidEntry
+  try {
+    status = readStatusFile(stateDir)
+    pidEntry = readPidFile(stateDir)
+  } catch (err) {
+    // Both readers throw on a file they cannot make sense of, and
+    // `JSON.parse`'s message quotes an excerpt of the input verbatim. Left
+    // uncaught that reached the operator as a raw stack trace carrying the
+    // file's own bytes: useless as a diagnosis, and the one path on which the
+    // file reached the terminal entirely unfiltered.
+    const message = err instanceof Error ? err.message : String(err)
+    ctx.stderr.write(`hyp daemon status: ${printable(message, MAX_ERROR_CHARS)}\n`)
+    return 1
+  }
   const running = !!(pidEntry && processIsAlive(pidEntry.pid))
   if (!status) {
     if (json) {
@@ -84,26 +160,31 @@ export async function runDaemonStatus(argv, ctx) {
     ctx.stdout.write(JSON.stringify(payload, null, 2) + '\n')
     return 0
   }
-  ctx.stdout.write(`daemon: ${status.state}${running ? '' : ' (no live process)'}\n`)
-  ctx.stdout.write(`  pid:        ${status.pid}\n`)
-  ctx.stdout.write(`  startedAt:  ${status.startedAt}\n`)
-  if (status.healthyAt) ctx.stdout.write(`  healthyAt:  ${status.healthyAt}\n`)
-  if (status.stoppedAt) ctx.stdout.write(`  stoppedAt:  ${status.stoppedAt}\n`)
-  ctx.stdout.write(`  uptime_ms:  ${liveUptimeMs}\n`)
+  // Everything below this line came out of the file and is going to a
+  // terminal, so everything below this line is cleaned on the way.
+  ctx.stdout.write(`daemon: ${printable(status.state)}${running ? '' : ' (no live process)'}\n`)
+  ctx.stdout.write(`  pid:        ${printableNumber(status.pid)}\n`)
+  ctx.stdout.write(`  startedAt:  ${printable(status.startedAt)}\n`)
+  if (status.healthyAt) ctx.stdout.write(`  healthyAt:  ${printable(status.healthyAt)}\n`)
+  if (status.stoppedAt) ctx.stdout.write(`  stoppedAt:  ${printable(status.stoppedAt)}\n`)
+  ctx.stdout.write(`  uptime_ms:  ${printableNumber(liveUptimeMs)}\n`)
+  const sources = entryList(status.sources)
+  const sinks = entryList(status.sinks)
   ctx.stdout.write('  sources:\n')
-  if (status.sources.length === 0) {
+  if (sources.length === 0) {
     ctx.stdout.write('    (none)\n')
   } else {
-    for (const source of status.sources) {
-      ctx.stdout.write(`    - ${source.name} (${source.plugin}): ${source.state}${source.error ? ' - ' + source.error : ''}\n`)
+    for (const source of sources) {
+      const error = printable(source.error, MAX_ERROR_CHARS)
+      ctx.stdout.write(`    - ${printable(source.name)} (${printable(source.plugin)}): ${printable(source.state)}${error ? ' - ' + error : ''}\n`)
     }
   }
   ctx.stdout.write('  sinks:\n')
-  if (status.sinks.length === 0) {
+  if (sinks.length === 0) {
     ctx.stdout.write('    (none)\n')
   } else {
-    for (const sink of status.sinks) {
-      ctx.stdout.write(`    - ${sink.instance} (${sink.plugin}, ${sink.kind})\n`)
+    for (const sink of sinks) {
+      ctx.stdout.write(`    - ${printable(sink.instance)} (${printable(sink.plugin)}, ${printable(sink.kind)})\n`)
     }
   }
   return 0

src/core/commands/status.js

@@ -310,35 +310,43 @@ export function renderStatusJson({ report, clientNames, datasets, cacheRoot }) {
 }
 
 /**
- * How much of the daemon line's `error=` a hostile status file may spend.
- * Wider than a label's 120, because unlike a name this carries a real error
- * message - typically an fs error naming a full path - and the clamp exists
- * to stop the line being bloated, not to bound an identifier. Cutting a path
- * short is the one way this cleaning could cost a reader an answer.
+ * How much of an `error` line a hostile file may spend. Wider than a label's
+ * 120, because unlike a name this carries a real error message - typically an
+ * fs or parser error naming a full path - and the clamp exists to stop the
+ * line being bloated, not to bound an identifier. Cutting a path short is the
+ * one way this cleaning could cost a reader an answer.
  */
-const MAX_DAEMON_ERROR_CHARS = 400
+const MAX_ERROR_CHARS = 400
 
 /**
  * A string on its way into the text surface, made safe to print.
  *
- * `renderStatusText` is the last point before a terminal, and several of the
- * strings it interpolates were read back out of `status.json`
- * (`daemon.state`, `daemon.mode`, and - with no runtime attached - every
- * `sources[]` / `sinks[]` entry). That file is a *file*: core cannot assume
- * the daemon that wrote it was this version, this build, or well behaved, so
- * a raw value from it can carry an escape sequence that repaints the
- * operator's screen or a newline that forges a plausible extra status line.
+ * `renderStatusText` is the last point before a terminal, and not everything
+ * it interpolates was written by this build. Several of these strings were
+ * read back out of `status.json` (`daemon.state`, `daemon.mode`, and - with
+ * no runtime attached - every `sources[]` / `sinks[]` entry), and that file
+ * is a *file*: core cannot assume the daemon that wrote it was this version,
+ * this build, or well behaved. The remote-config block prints etags authored
+ * by whatever server the install joined, read back through a local state file
+ * that nothing validates; the client block prints an attach probe's error,
+ * which for a settings file that is not valid JSON is `JSON.parse`'s message
+ * and therefore quotes an excerpt of that file verbatim. Any of them can
+ * carry an escape sequence that repaints the operator's screen or a newline
+ * that forges a plausible extra status line.
  *
- * Cleaning happens *here* rather than in the collector because these values
- * are not display-only: `sources[].name` and `sinks[].instance` are identity
- * keys and part of the `--json` contract, which a consumer escapes for
- * itself. Cleaning at the interpolation closes the terminal path and leaves
- * both intact - including the raw values the provenance lookups below match
- * on.
+ * Cleaning happens *here* rather than in the collector because `--json`
+ * renders the same values for a program, which escapes for itself and must
+ * receive what was actually recorded (LLP 0225): escaping is a render's job,
+ * and only of the render a person reads. That is not merely a display
+ * nicety, either - `sources[].name` and `sinks[].instance` are identity keys
+ * as well as `--json` contract, and cleaning at the interpolation closes the
+ * terminal path while leaving both intact, including the raw values the
+ * provenance lookups below match on.
  *
- * @param {string | undefined} value
+ * @param {unknown} value
  * @param {number} [max]
  * @returns {string}
+ * @ref LLP 0225#decision [implements]: the text surface escapes what a person reads; --json stays byte-exact
  * @ref LLP 0164#status-reads-it-from-the-status-file [constrained-by]: what core reads back out of status.json is cleaned at the last point before render, whichever field it came from
  */
 function printable(value, max) {
@@ -421,7 +429,11 @@ export function renderStatusText({ report, clientNames, datasets, cacheRoot, std
       // @ref LLP 0229#status-derives-by-the-same-gate [implements]: the clients row says attach n/a, not "not attached", for a probe-less client
       state.push(c.attachable === false ? 'attach n/a' : c.attached ? 'attached' : 'not attached')
       stdout.write(`    - ${c.name}  [${state.join(', ')}]${provenanceTag(report.layered, isCentralPlugin(report.layered, c.plugin))}\n`)
-      if (c.error) stdout.write(`        error: ${c.error}\n`)
+      // The probe's error is not this build's prose: a settings file that is
+      // not valid JSON surfaces here as `JSON.parse`'s message, which echoes
+      // an excerpt of the file back. The client wrote that file, so it is
+      // cleaned on the way into the line.
+      if (c.error) stdout.write(`        error: ${printable(c.error, MAX_ERROR_CHARS)}\n`)
     }
     for (const name of clientNames) {
       if (seen.has(name)) continue
@@ -536,16 +548,21 @@ export function renderStatusText({ report, clientNames, datasets, cacheRoot, std
   // show. A never-joined install keeps the V1 status surface.
   const rc = report.remoteConfig
   if (rc && (rc.runningEtag || rc.probation || rc.lastRollback || rc.badEtag)) {
+    // Every value in this block is remote-authored or read back out of a
+    // local state file that nothing validates, and all of it is display-only
+    // here, so all of it is cleaned at the interpolation. An etag is whatever
+    // the joined server put in the header; a `reason` and a timestamp are
+    // this build's own vocabulary only if this build wrote the file.
     stdout.write('  remote config:\n')
-    if (rc.runningEtag) stdout.write(`    running etag:  ${rc.runningEtag}\n`)
+    if (rc.runningEtag) stdout.write(`    running etag:  ${printable(rc.runningEtag)}\n`)
     if (rc.probation) {
-      stdout.write(`    probation:     ${rc.probation.etag} until ${rc.probation.until}\n`)
+      stdout.write(`    probation:     ${printable(rc.probation.etag)} until ${printable(rc.probation.until)}\n`)
     }
     if (rc.lastRollback) {
-      stdout.write(`    last rollback: ${rc.lastRollback.etag} at ${rc.lastRollback.at} (${rc.lastRollback.reason})\n`)
+      stdout.write(`    last rollback: ${printable(rc.lastRollback.etag)} at ${printable(rc.lastRollback.at)} (${printable(rc.lastRollback.reason)})\n`)
     }
     if (rc.badEtag) {
-      stdout.write(`    bad etag:      ${rc.badEtag.etag} (${rc.badEtag.reason})\n`)
+      stdout.write(`    bad etag:      ${printable(rc.badEtag.etag)} (${printable(rc.badEtag.reason)})\n`)
     }
   }
 
@@ -705,7 +722,7 @@ function describeDaemon(daemon) {
   if (daemon.state) parts.push(`state=${printable(daemon.state)}`)
   if (daemon.pid) parts.push(`pid=${daemon.pid}`)
   if (daemon.mode) parts.push(`mode=${printable(daemon.mode)}`)
-  if (daemon.error) parts.push(`error=${printable(daemon.error, MAX_DAEMON_ERROR_CHARS)}`)
+  if (daemon.error) parts.push(`error=${printable(daemon.error, MAX_ERROR_CHARS)}`)
   return parts.join(', ')
 }
 

src/core/daemon/status.js

@@ -1139,10 +1139,31 @@ export async function collectHypAwareStatus(opts = {}) {
     remoteConfig = readConfigControlStatus({ stateRoot })
   } catch { /* best-effort probe */ }
   if (remoteConfig?.lastRollback) {
+    // The three values in this message are display-only, and none of them is
+    // this build's own: the etag is whatever the joined server served, and
+    // all three come back through `config-control/state.json`, which is read
+    // with no validation beyond "is an object". A diagnostic message is prose
+    // assembled for a person, so its components are cleaned here, where the
+    // prose is assembled, rather than in `readConfigControlStatus`.
+    //
+    // Be precise about what that does and does not mean. The message is
+    // assembled once, here, so the cleaned prose is also what `--json` carries
+    // at `diagnostics[].message`: a machine consumer of *that string* reads
+    // the sanitized line, not the file's bytes, with each component stripped
+    // and clamped to `sanitizeLabel`'s 120. That is accepted, not overlooked.
+    // The prose line is not a parsing surface on either render, and the
+    // unedited values stay one key away and structured, at
+    // `remote_config.last_rollback`, which is where a program reads them and
+    // which stays byte-exact. Cleaning at the single `${d.message}`
+    // interpolation in `renderStatusText` instead would keep the prose raw for
+    // `--json` too, but it would have to pick one clamp width that holds for
+    // every diagnostic kind, not just this one.
+    // @ref LLP 0225#decision [implements]: assembled prose is cleaned where it is assembled; the machine copy of the same values, `remote_config.last_rollback`, stays byte-exact
+    const rolledBack = remoteConfig.lastRollback
     diagnostics.push({
       severity: 'warning',
       kind: 'remote_config_rolled_back',
-      message: `remote config ${remoteConfig.lastRollback.etag} rolled back at ${remoteConfig.lastRollback.at} (${remoteConfig.lastRollback.reason})`,
+      message: `remote config ${sanitizeLabel(rolledBack.etag) ?? ''} rolled back at ${sanitizeLabel(rolledBack.at) ?? ''} (${sanitizeLabel(rolledBack.reason) ?? ''})`,
       repair: ['fix the central config revision; the gateway re-applies when the served etag changes'],
     })
   }