diff --git a/openwiki/cli-and-operations.md b/openwiki/cli-and-operations.md index 783f0d5d..d00b4dd8 100644 --- a/openwiki/cli-and-operations.md +++ b/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 diff --git a/src/lib/doctor.js b/src/lib/doctor.js index df50f5a6..0b5cfa33 100644 --- a/src/lib/doctor.js +++ b/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 }, ); } diff --git a/test/doctor.test.js b/test/doctor.test.js index 37f93100..7559fdb0 100644 --- a/test/doctor.test.js +++ b/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", () => {