fix(doctor): classify standing warnings as advisory

openwiki/cli-and-operations.md

@@ -51,34 +51,39 @@ is missing or unusable is still counted, under `(unnamed)`, rather than dropped.
 `listDegradedChecks` in `src/lib/doctor.js` is the rule — read it rather than
 this paragraph if the two ever disagree.
 
-`advisory` is decided **per warn, not per check**. It marks a warn describing a
-standing condition the operator cannot act on at the moment they read the
-report. Today exactly one warn carries it: `queue.row_invariant`'s
-row-invariant/malformed-row warn, because the offending rows are already written
-and already being rendered. The *same check* also emits a `queue unreadable`
-warn, and that one is **not** advisory — an unreadable queue is new, actionable,
-and plausibly means ingestion has stopped. An advisory warn is still a full
-`warn` in `checks` and still counts in `summary.warn`; the report is not
-quieter, the alert signal is narrower. Not opting in is the default, so a new
-warn is alert-worthy unless it argues otherwise.
-
-That distinction is the point of the field: an earlier version counted every
+`advisory` is decided **per emitted warn, not per check id**. It marks a standing
+condition that cannot be cleared by operator action at report time. An advisory
+warn remains in `checks` and `summary.warn`; only `degraded` and
+`degraded_checks` ignore it. The opt-in is the literal boolean `true`, and only
+for status `warn`: a fail always degrades, malformed or missing ids are named
+`(unnamed)`, and every new/unclassified warn is alert-worthy by default.
+
+The current warning-path audit is:
+
+| Check and emitted condition | Classification | Reason |
+| --- | --- | --- |
+| `browser.opener`: headless/session environment | Advisory | Headlessness is a standing host/session property; `--no-open` or manually opening the URL does not clear it. |
+| `browser.opener`: `open`/`xdg-open` missing on a non-headless host | Actionable | Install the opener (`xdg-utils` on Linux) or deliberately use `--no-open`. |
+| `notify.configured`: no supported notify/hook integration configured | Advisory | The integrations are optional and provider-specific; `init` skips absent provider configs, and passive readers do not require hooks. This aggregate check has no fail path. |
+| `queue.row_invariant`: parseable rows violating the column invariant | Advisory | The rows are already on disk and already rendered; the warning remains visible for diagnosis. |
+| `queue.row_invariant`: unparseable lines | Actionable | Local API readers skip malformed lines, so their usage is absent; corruption or a partial write must degrade the report. |
+| `queue.row_invariant`: queue unreadable | Actionable | Permissions, disk, or file-type problems can stop ingestion. The shared check id does not inherit the advisory flag. |
+| `ingest.transcript_suppressed`: process list could not be read | Actionable | Doctor could not verify whether unrecordable Claude sessions are running. |
+| `ingest.transcript_suppressed`: one or more `--no-session-persistence` sessions | Actionable | Those sessions write no transcript, so their token usage cannot be recorded. |
+| `fs.tracker_dir`: tracker directory missing | Actionable | Initialization can create the required local state. Permission and type errors are fails, not warns. |
+| `fs.config_json`: config missing | Actionable | Initialization can create it. Invalid or unreadable config is a fail, not a warn. |
+
+The remaining doctor checks have no warning path: `runtime.node_version` and
+`cli.entrypoint` fail closed; `runtime.dashboard_url`,
+`runtime.http_timeout_ms`, and `runtime.debug` are informational `ok` checks.
+`buildDiagnosticsChecks` currently emits only `notify.configured`. A check that
+cannot run on an unsupported platform is omitted rather than reported as `ok`.
+
+This distinction is the point of the field: an earlier version counted every
 warn, and on a machine carrying one standing row-invariant warning it read
 `true` on a healthy day. An alert that can never clear and an alert that never
 fires are the same defect.
 
-**Known limit — `degraded` still pins on a headless host.** `browser.opener`
-warns permanently where no browser can be opened (`CI=true`, or Linux with no
-`DISPLAY`/`WAYLAND_DISPLAY`), and that warn is not marked advisory. On such a
-machine `degraded` reads `true` on a healthy day, which is the very failure the
-field was narrowed to remove — so the alert-wiring advice above holds on a
-desktop and not yet on a headless server. Only the queue warn has been
-classified so far; auditing the remaining checks for standing conditions is
-tracked as its own piece of work rather than guessed at here.
-
-A check that could not be run on this platform is **omitted** from `checks`
-rather than reported as `ok`.
-
 ## Service and release operations
 
 Local service installers live under `scripts/`. The authoritative release and

src/lib/doctor.js

@@ -67,16 +67,14 @@ async function buildDoctorReport({
     // It counts every warn and fail except a warn whose check marked itself
     // `advisory`. A first version counted every warn, which made it useless on the
     // one machine it was written for: that box carries a standing
-    // `queue.row_invariant` warn about two malformed rows, so `degraded` read true
+    // `queue.row_invariant` warn about two parseable invariant violations, so
+    // `degraded` read true
     // on a perfectly healthy day and an alert wired to it could never clear. An
     // always-on alert and an alert that never fires fail the same way.
     // `listDegradedChecks` holds the exact rule including its two fail-closed
-    // clauses; `queueCheck` shows what earns the flag and what does not.
-    //
-    // Not yet true of every standing warn: `browser.opener` still warns
-    // permanently on a headless host, so `degraded` pins there. Left as-is
-    // deliberately rather than guessed at — auditing the remaining checks is
-    // tracked separately.
+    // clauses. Advisory is assigned at the individual warning return site: a
+    // standing condition can opt out without muting an actionable warning from
+    // the same check id.
     degraded: degradedChecks.length > 0,
     // Which checks put it there. Without this, `degraded: true` is unactionable —
     // a consumer has to re-derive the reason by walking `checks` itself, and a
@@ -89,11 +87,10 @@ async function buildDoctorReport({
 }
 
 // A check is advisory when its warn describes a standing condition the operator
-// cannot act on in the moment — true of the queue row invariant, whose rows are
-// already written and already being rendered. Such a check still reports `warn`
-// and still appears in `summary.warn`: the report does not become quieter, only
-// the alert signal becomes specific. Anything that does not opt in counts, so a
-// new check is alert-worthy by default and has to argue its way out.
+// cannot act on in the moment. Such a check still reports `warn` and still appears
+// in `summary.warn`: the report does not become quieter, only the alert signal
+// becomes specific. Anything that does not opt in counts, so a new check is
+// alert-worthy by default and has to argue its way out.
 //
 // Two rules here are deliberately fail-CLOSED, because the failure this field
 // exists to prevent is a real problem reading as silence, and both were live
@@ -111,9 +108,17 @@ async function buildDoctorReport({
 function listDegradedChecks(checks = []) {
   return checks
     .filter((check) => check && (check.status === "warn" || check.status === "fail"))
-    .filter((check) => !(check.status === "warn" && check.advisory === true))
+    // A malformed id overrides advisory suppression. Advisory is an explicit
+    // classification made at a known warning call site; if that identity is
+    // lost, fail closed under the placeholder rather than silently dropping it.
+    .filter((check) => !(
+      check.status === "warn"
+      && check.advisory === true
+      && typeof check.id === "string"
+      && check.id.trim().length > 0
+    ))
     .map((check) =>
-      typeof check.id === "string" && check.id.length > 0 ? check.id : UNNAMED_CHECK_ID,
+      typeof check.id === "string" && check.id.trim().length > 0 ? check.id : UNNAMED_CHECK_ID,
     )
     .sort();
 }
@@ -194,11 +199,14 @@ function buildNodeVersionCheck(nodeVersion) {
 async function buildBrowserOpenerCheck({ platform = process.platform, env = process.env, commandExists }) {
   const headless = isHeadlessEnvironment({ platform, env });
   if (headless) {
+    // Standing environment property: neither --no-open nor opening the printed
+    // URL manually can make a headless session acquire a browser opener.
     return {
       id: "browser.opener",
       status: "warn",
       detail: "headless/session environment detected; use --no-open or open the printed URL manually",
       critical: false,
+      advisory: true,
       meta: { platform, command: null, headless: true },
     };
   }
@@ -444,14 +452,21 @@ function buildDiagnosticsChecks(diagnostics) {
     notify.claude_hook_configured ||
     notify.gemini_hook_configured ||
     notify.opencode_plugin_configured ||
-    notify.openclaw_hook_configured,
+    notify.openclaw_hook_configured ||
+    notify.openclaw_session_plugin_configured ||
+    notify.grok_hook_configured,
   );
 
+  // This aggregate describes an optional integration preference, not whether
+  // passive log ingestion works. `init` also skips hooks for providers whose
+  // config is absent, so "none configured" can be a stable, intentional state.
+  // Keep only that warn advisory; this check currently has no fail path.
   checks.push({
     id: "notify.configured",
     status: notifyConfigured ? "ok" : "warn",
     detail: notifyConfigured ? "notify configured" : "notify not configured",
     critical: false,
+    ...(notifyConfigured ? {} : { advisory: true }),
     meta: { configured: notifyConfigured },
   });
 
@@ -495,13 +510,10 @@ const QUEUE_VIOLATIONS_SHOWN = 5;
 // PER CALL SITE, not once for this check id, because the two warns this check can
 // emit are not the same kind of thing:
 //
-//   - the row-invariant / malformed-row warn IS advisory, on the same reasoning
-//     the comment above gives for warning rather than failing: the rows are
-//     already written and already being rendered, so there is nothing the
-//     operator can do at the moment they read the report.
-//   - "queue unreadable" is NOT. A queue that cannot be read is new, actionable
-//     (permissions, disk), and plausibly means ingestion has stopped — exactly
-//     the #128 class this whole field exists to surface.
+//   - a parseable row-invariant violation IS advisory: the row is already written
+//     and rendered, so there is nothing the operator can do at report time.
+//   - an unparseable line or unreadable queue is NOT. Those conditions omit usage
+//     or can stop ingestion and are actionable (corruption, permissions, disk).
 //
 // An earlier version of this function stamped `advisory: true` on everything it
 // returned, which silenced the unreadable case: `warn` in `checks`,
@@ -550,10 +562,10 @@ async function checkQueueRows(queuePath) {
   const parts = [];
   if (violations.length > 0) parts.push(`${violations.length} row problem(s)`);
   if (malformed > 0) parts.push(`${malformed} unparseable line(s)`);
-  // Advisory: the offending rows are already on disk and already aggregated into
-  // what the dashboard renders, so this warn tells the operator something true
-  // that they cannot act on in this moment. It is the one standing condition in
-  // this check.
+  // Parseable invariant violations are already on disk and already aggregated
+  // into what the dashboard renders, so they are advisory. Malformed lines are
+  // skipped by local-api readers and their usage is absent; corruption or a
+  // partial write is actionable and must degrade the report.
   return queueCheck(
     "warn",
     `${parts.join(", ")} in ${rows.length + malformed} line(s)`,
@@ -564,7 +576,7 @@ async function checkQueueRows(queuePath) {
       violations: violations.length,
       examples: violations.slice(0, QUEUE_VIOLATIONS_SHOWN),
     },
-    { advisory: true },
+    { advisory: malformed === 0 },
   );
 }
 

test/doctor.test.js

@@ -68,6 +68,58 @@ test("doctor warns for headless browser opener even when Node is supported", asy
   assert.equal(openerCheck.status, "warn");
   assert.equal(openerCheck.meta.headless, true);
   assert.match(openerCheck.detail, /--no-open/);
+  assert.equal(openerCheck.advisory, true);
+  assert.equal(report.summary.warn, 1, "the advisory opener stays visible in summary.warn");
+  assert.equal(report.degraded, false);
+  assert.deepEqual(report.degraded_checks, []);
+});
+
+test("doctor degrades when a non-headless browser opener is missing", async () => {
+  const report = await buildDoctorReport({
+    runtime: {},
+    system: {
+      nodeVersion: "v20.11.1",
+      platform: "linux",
+      env: { PATH: "/usr/bin", DISPLAY: ":0" },
+      commandExists: async () => false,
+    },
+  });
+  const openerCheck = report.checks.find((c) => c.id === "browser.opener");
+
+  assert.equal(openerCheck.status, "warn");
+  assert.equal(openerCheck.meta.headless, false);
+  assert.notEqual(openerCheck.advisory, true, "a missing opener is actionable");
+  assert.equal(report.summary.warn, 1);
+  assert.equal(report.degraded, true);
+  assert.deepEqual(report.degraded_checks, ["browser.opener"]);
+});
+
+test("doctor keeps an unconfigured notify preference visible without degrading", async () => {
+  const report = await buildDoctorReport({
+    runtime: {},
+    diagnostics: { notify: {} },
+  });
+  const notifyCheck = report.checks.find((c) => c.id === "notify.configured");
+
+  assert.equal(notifyCheck.status, "warn");
+  assert.equal(notifyCheck.advisory, true);
+  assert.equal(report.summary.warn, 1, "the advisory preference stays visible in summary.warn");
+  assert.equal(report.degraded, false);
+  assert.deepEqual(report.degraded_checks, []);
+});
+
+test("doctor recognizes every supported notify integration as configured", async () => {
+  for (const field of ["openclaw_session_plugin_configured", "grok_hook_configured"]) {
+    const report = await buildDoctorReport({
+      runtime: {},
+      diagnostics: { notify: { [field]: true } },
+    });
+    const notifyCheck = report.checks.find((c) => c.id === "notify.configured");
+
+    assert.equal(notifyCheck.status, "ok", `${field} is a supported integration`);
+    assert.notEqual(notifyCheck.advisory, true, "an ok result must not carry advisory");
+    assert.equal(report.degraded, false);
+  }
 });
 
 test("doctor marks invalid config.json as critical", async () => {
@@ -289,9 +341,9 @@ test("doctor reports a clean suppression check and stays undegraded", async () =
 // --- #130: `degraded` counts non-advisory warns only -------------------------
 //
 // The first version counted every warn. On the machine that reported #128 that
-// made it useless: a standing `queue.row_invariant` warning about two malformed
-// rows meant `degraded` read true on a healthy day, so an alert wired to it could
-// never clear. These pin the narrowed contract.
+// made it useless: a standing `queue.row_invariant` warning about two parseable
+// invariant violations meant `degraded` read true on a healthy day, so an alert
+// wired to it could never clear. These pin the narrowed contract.
 
 test("listDegradedChecks counts a plain warn or fail and names it", () => {
   assert.deepEqual(
@@ -366,6 +418,25 @@ test("a queue row violation warns and is counted, but does not degrade the repor
   }
 });
 
+test("a malformed queue line is actionable because it is omitted from dashboard totals", async () => {
+  const tmp = await fs.mkdtemp(path.join(os.tmpdir(), "tt-degraded-malformed-"));
+  const queuePath = path.join(tmp, "queue.jsonl");
+  try {
+    await fs.writeFile(queuePath, "{not valid json}\n", "utf8");
+
+    const report = await buildDoctorReport({ runtime: {}, paths: { queuePath } });
+    const check = report.checks.find((c) => c.id === "queue.row_invariant");
+
+    assert.equal(check.status, "warn");
+    assert.equal(check.meta.malformed, 1);
+    assert.notEqual(check.advisory, true, "unparseable usage is not already rendered");
+    assert.equal(report.degraded, true);
+    assert.deepEqual(report.degraded_checks, ["queue.row_invariant"]);
+  } finally {
+    await fs.rm(tmp, { recursive: true, force: true });
+  }
+});
+
 // --- the three fail-open holes an independent review found in the first cut ----
 
 // The advisory rationale is about a STANDING condition. An unreadable queue is
@@ -399,7 +470,7 @@ test("an unreadable queue is actionable, so it degrades the report", async () =>
 // typo removed a genuine warn from `degraded` entirely — `[WARN] unknown` to a
 // human, nothing at all to automation.
 test("a warn with a missing or malformed id still degrades, under a placeholder", () => {
-  for (const id of [undefined, null, "", 42, {}]) {
+  for (const id of [undefined, null, "", "   ", 42, {}]) {
     const result = listDegradedChecks([{ id, status: "warn" }]);
     assert.deepEqual(
       result,
@@ -409,6 +480,16 @@ test("a warn with a missing or malformed id still degrades, under a placeholder"
   }
 });
 
+test("a malformed warning id overrides advisory suppression", () => {
+  for (const id of [undefined, null, "", "   ", 42, {}]) {
+    assert.deepEqual(
+      listDegradedChecks([{ id, status: "warn", advisory: true }]),
+      [UNNAMED_CHECK_ID],
+      `advisory cannot hide malformed id=${JSON.stringify(id)}`,
+    );
+  }
+});
+
 // `advisory` is argued for standing WARNINGS. Nothing argues for muting a fail on
 // the same id, and the first version's filter muted both.
 test("advisory never suppresses a fail", () => {