fix(doctor): count only non-advisory warns toward degraded - #136
Conversation
`degraded` was `summary.warn > 0 || summary.fail > 0`, which made it useless on the machine it was written for. That box carries a standing `queue.row_invariant` warning about two malformed rows out of 6482, so `degraded` read true on a perfectly healthy day. An alert wired to it could never clear, and an alert that never clears fails the same way as one that never fires — which is the exact failure #128 was about. A check may now set `advisory: true` to say its warn is a standing condition the operator cannot act on at the moment they read the report. `queue.row_invariant` is the only one that does, and it earns it on reasoning already written above `queueCheck`: the rows are on disk and already being rendered, which is why that check warns rather than fails in the first place. `degraded` is now "at least one non-advisory warn or fail". `degraded_checks` lists the ids that put it there, sorted — without it, `degraded: true` is unactionable, since a consumer would have to re-walk `checks` to find out why and a human reading the JSON cannot tell a new problem from the standing one. An advisory check is unchanged everywhere else: still `warn` in `checks`, still counted in `summary.warn`. The report is not quieter; only the alert signal is narrower. Opting out is explicit, so a check that forgets the flag stays alert-worthy — a problem that reads as silence is the thing being prevented. Not a breaking change for anyone: `degraded` has never shipped. `git show v0.39.44:src/lib/doctor.js | grep -c degraded` is 0, and npm `latest` is still 0.39.44 — the field landed on main in #129 and that merge deliberately did not publish. Narrowing it now costs no consumer anything. Verified on the machine that reported #128, running the built CLI rather than a fixture: ok: True | degraded: True degraded_checks: ['ingest.transcript_suppressed'] summary: {'ok': 9, 'warn': 2, 'fail': 0, 'critical': 0} WARN queue.row_invariant | advisory: True WARN ingest.transcript_suppressed | advisory: False Two warns still reported, one reaches the alert signal, and `degraded_checks` names which. Before this change both would have been indistinguishable behind a single boolean. Both mutation points were checked by breaking them, not by watching tests pass: neutralising the `advisory` filter in `listDegradedChecks` fails 3 tests; restoring the old `summary.warn > 0` predicate fails 1. `ci:local` exit 0 with 1002 node tests and 41 dashboard test files.
A Fable review of the first cut returned correct-but-incomplete. All three of its substantive findings held up against the source, and the worst one was mine entirely. 1. `advisory` was stamped per check id, not per warn. `queueCheck` hard-coded `advisory: true` on everything it returned, and `checkQueueRows` uses it for two different warns: the row-invariant one (standing — the rows are already written and rendered) and `queue unreadable` (a non-ENOENT read error: permissions, disk). The second is new, actionable, and plausibly means ingestion has stopped — the #128 class this field exists to surface — and it was going out as `warn` in `checks` with `degraded: false` on the wire. I had written "a problem that reads as silence is the thing being prevented" in the same file and then opened that exact path. `advisory` is now an explicit per-call-site argument defaulting to false. 2. `listDegradedChecks` dropped checks with a missing or malformed `id`, and `degraded` derives from the length of that list, so an id typo removed a genuine warn from the alert signal altogether — `[WARN] unknown` to a human (`src/commands/doctor.js` tolerates a missing id) and nothing at all to automation. Such a check is now counted under `UNNAMED_CHECK_ID`. 3. The filter suppressed advisory `fail`s as well as advisory warns. The rationale for the flag is about standing warnings; nothing argues for muting a fail on the same id. Now scoped to warns. Each fix has a test that fails without it, checked one at a time by reverting just that fix: queueCheck hard-codes advisory again -> "an unreadable queue is actionable" fails id map drops non-strings again -> "a warn with a missing id still degrades" fails advisory suppresses fail too -> "advisory never suppresses a fail" fails .sort() removed -> "degraded_checks is sorted" fails all restored -> 20/20 That last row is also a review finding: the docs claimed sorted output and no test pinned it, because the only multi-element expectation was already in sorted input order. The unreadable-queue fixture uses a directory where a file is expected (EISDIR) rather than chmod, because CI runs as root and an unreadable-by-permission file is still readable there. The review's remaining finding is real and is NOT fixed here: `browser.opener` warns permanently on a headless host, so `degraded` still pins there. Reproduced rather than reasoned about: $ node bin/tracker.js doctor --json | jq -c '{degraded, degraded_checks}' {"degraded":true,"degraded_checks":["ingest.transcript_suppressed"]} $ CI=true node bin/tracker.js doctor --json | jq -c '{degraded, degraded_checks}' {"degraded":true,"degraded_checks":["browser.opener","ingest.transcript_suppressed"]} Classifying that check means a severity call on code I did not write, with no coverage for the headless path, so openwiki now states the limit instead of implying the alert-wiring advice is general. Tracked in #137. openwiki also had two sentences that described the intent rather than the mechanism: it said the flag is per check when it is per warn, and omitted the id rule. Both corrected. ci:local exit 0 — 1006 node tests, 41 dashboard test files, OpenWiki fact check 0 findings.
Independent Fable review returned correct-but-incomplete. Three findings fixed here; the fourth is filed.Reviewer: a read-only Fable agent (
1 — HIGH, and I missed it completely:
|
| warn | standing? |
|---|---|
row-invariant / malformed rows (doctor.js:526) |
yes — rows already written and already rendered |
queue unreadable (doctor.js:502, any non-ENOENT read error) |
no — permissions, disk, new and actionable |
So a queue that lost read permission, or a disk throwing EIO, went out as warn in checks with
degraded: false on the wire. Ingestion plausibly broken, alert silent. I had written "a problem
that reads as silence is the thing being prevented" into the same file and then opened that exact
path a few lines away.
advisory is now an explicit per-call-site argument defaulting to false.
The reviewer was careful to separate this from the settled "classify only one check" decision, and it
was right to: my reasoning was per-condition while my flag was per-id. That is a mis-scoping inside
the one check I did classify, not a re-litigation of scope.
2 — MEDIUM: id filter was fail-open
listDegradedChecks mapped to check.id then dropped non-strings, and degraded derives from that
list's length. A non-advisory warn with a typo'd or missing id therefore contributed nothing —
[WARN] unknown to a human (src/commands/doctor.js tolerates a missing id) and nothing at all to
automation. The old summary.warn > 0 predicate would have caught it, so the change narrowed more
than the docs or the comments said.
Now counted under UNNAMED_CHECK_ID ("(unnamed)"), so degraded and degraded_checks stay in
agreement.
I had spotted this one myself before the review came back and said so on record rather than claiming
it afterwards. The review found it independently.
3 — LOW/MED: advisory suppressed fail too
The filter ran after the status filter, so an advisory check with status: "fail" was also excluded.
The flag is argued for standing warnings; nothing argues for muting a fail on the same id. Scoped
to warns.
Mutation matrix — one fix reverted at a time
| reverted | test that fails |
|---|---|
queueCheck hard-codes advisory again |
an unreadable queue is actionable, so it degrades the report |
| id map drops non-strings again | a warn with a missing or malformed id still degrades, under a placeholder |
| advisory suppresses fail too | advisory never suppresses a fail |
.sort() removed |
degraded_checks is sorted regardless of check order |
| all restored | 20/20 |
That last row was also a review finding: the docs claimed sorted output and nothing pinned it,
because the only multi-element expectation happened to be in sorted input order already.
The unreadable-queue fixture uses a directory where a file is expected (EISDIR) rather than a chmod,
because CI runs as root and an unreadable-by-permission file is still readable there.
4 — NOT fixed here, filed as #137: degraded still pins on a headless host
browser.opener warns permanently when no browser can be opened. The reviewer argued this from
reading; I reproduced it:
$ node bin/tracker.js doctor --json | jq -c '{degraded, degraded_checks}'
{"degraded":true,"degraded_checks":["ingest.transcript_suppressed"]}
$ CI=true node bin/tracker.js doctor --json | jq -c '{degraded, degraded_checks}'
{"degraded":true,"degraded_checks":["browser.opener","ingest.transcript_suppressed"]}
On a headless server — the population most likely to wire automation to --json — degraded reads
true on a healthy day, which is the failure this PR set out to remove, surviving in a different
check. Its own doc text says an alert that can never clear and one that never fires are the same
defect, so that is a fair hit.
Not fixed here because classifying browser.opener means a severity call on code I did not write,
under review pressure, with no coverage for the headless path. openwiki now states the limit
explicitly instead of implying the alert-wiring advice is general, and #137 asks for the per-warn
audit across all nine checks.
5 — docs described intent, not mechanism
Two sentences I wrote: the flag is per warn, not per check as stated, and the id rule was omitted.
Both corrected, plus the headless caveat added.
State
ci:local SUCCESS on the PR. 1006 node tests, 41 dashboard test files, OpenWiki fact check 0
findings. Verified on the reporting machine that degraded_checks is still
['ingest.transcript_suppressed'] with summary.warn at 2 — the report did not get quieter, only
the alert signal is specific.
Still no human review. The reviewer above is a different model family with no sight of my reasoning,
which is the strongest independence available here, but it is not a person.
Two gaps a review found in the previous commit. The `-x` scoping was only ever demonstrated on macOS, where `ps` is BSD and `-x` plainly means "own user, tty restriction lifted". Linux `ps` is procps and parses dash-prefixed options as UNIX-style, where `-x` is not an option — and `isProcessListSupported` returns true for every platform except win32, so Linux runs this argv. Had procps rejected it, `listProcessLines` would have returned `process_list_failed` on every Linux host: a permanent non-advisory warn, pinning `degraded` for a whole platform, which is the failure #136 exists to remove. Checked on a real Debian 12 / procps-ng 4.0.2 host rather than reasoned from the manual: `ps -x -o pid=,command=` exits 0 and reports one user across 22 lines, while `-ax` on the same box reports seven users across 39. procps accepts it as the BSD `x`, so the argv is correct on both supported platforms and #129's shipped code is correct too. Recorded in the comment so nobody has to re-derive it from a manpage. The array is now frozen. Two modules share it, and the claim written in the previous commit — that the two scans "cannot drift apart" — was only true of editorial drift. Importing one constant does not stop `PS_ARGS.push("-a")`; freezing does, and the test asserts the mutation throws rather than asserting the flag alone. Refs #132
…user (#138) * fix(usage-limits): scope the Antigravity process scan to the current user `detectAntigravityProcess` ran `/bin/ps -ax`, which walks every account on the box, and attached to whichever Antigravity language server matched first. Tracing what that could expose settles the question #132 left open: the CSRF token and the pid never reach an HTTP response. `processInfo` is read field by field and is never spread into a returned object — the pid goes to `listAntigravityPorts`, the token becomes a request header, and all four return shapes of `fetchAntigravityLimits` carry neither. What the token *fetches* does reach the response. `normalizeAntigravityResponse` returns `account_email` and `account_plan`, `finalize` spreads them into the result, and `getUsageLimits` serves that at `/functions/tokentracker-usage-limits`. On a shared host this displayed another person's email, plan and quota as the local user's own, and `writeAntigravityLimitsCache` persisted the address to disk, where the not-configured branch kept serving it after their process exited. So: not credential exposure, but cross-account PII — on multi-user hosts only, which is why no observed output could have caught it. The scan now uses the PS_BINARY / PS_ARGS already exported by process-list.js rather than its own inline argv. Sharing the constant is the point: #129 fixed the other scan, and two scans that must both stay own-user should not be able to drift apart. The regression test captures the literal argv from an injected commandRunner, because a real `ps` run on a single-user machine returns identical lines either way. Verified as a guard rather than a restatement: restoring `-ax` fails it. Anyone who has run this on a shared host may have another user's account_email cached in ~/.tokentracker/tracker/usage-limits-cache.json. It is a cache; deleting the file is the whole remedy. Closes #132 * fix(process-list): verify `-x` on Linux and freeze the shared argv Two gaps a review found in the previous commit. The `-x` scoping was only ever demonstrated on macOS, where `ps` is BSD and `-x` plainly means "own user, tty restriction lifted". Linux `ps` is procps and parses dash-prefixed options as UNIX-style, where `-x` is not an option — and `isProcessListSupported` returns true for every platform except win32, so Linux runs this argv. Had procps rejected it, `listProcessLines` would have returned `process_list_failed` on every Linux host: a permanent non-advisory warn, pinning `degraded` for a whole platform, which is the failure #136 exists to remove. Checked on a real Debian 12 / procps-ng 4.0.2 host rather than reasoned from the manual: `ps -x -o pid=,command=` exits 0 and reports one user across 22 lines, while `-ax` on the same box reports seven users across 39. procps accepts it as the BSD `x`, so the argv is correct on both supported platforms and #129's shipped code is correct too. Recorded in the comment so nobody has to re-derive it from a manpage. The array is now frozen. Two modules share it, and the claim written in the previous commit — that the two scans "cannot drift apart" — was only true of editorial drift. Importing one constant does not stop `PS_ARGS.push("-a")`; freezing does, and the test asserts the mutation throws rather than asserting the flag alone. Refs #132 --------- Co-authored-by: itarun.p <itarun.p@somapait.com>
1 participant
PR Goal (one sentence)
Make
degradedmean "something needs attention" instead of "something is imperfect", so it can bethe alert signal
openwikialready claims it is — closes #130.Why
degradedshipped in #129 assummary.warn > 0 || summary.fail > 0. On the machine it was writtenfor that is permanently true: the box carries a standing
queue.row_invariantwarning about twomalformed rows out of 6482, and that warning is deliberate —
queueCheckwarns rather than failsprecisely because the rows are already on disk and already being rendered, so there is nothing the
operator can do at the moment they read the report.
So an operator who wires an alert to
degradedgets one that can never clear. That fails the sameway as an alert that never fires, and "an alert nobody can act on gets ignored" is how the green
ok 9 | warn 1 | fail 0 | critical 0in #128 kept its authority for two days.What changed
A check may set
advisory: true.degradedis now "at least one non-advisory warn or fail", anda new
degraded_checksarray lists the ids that put it there, sorted.queue.row_invariantis the only check that opts in. Opting out is explicit, so a check that forgetsthe flag stays alert-worthy — the failure mode being prevented is a problem that reads as silence, so
the default has to be loud.
An advisory check is unchanged everywhere else: still
warninchecks, still counted insummary.warn. The report is not quieter; only the alert signal is narrower.degraded_checksis there becausedegraded: truealone is unactionable — a consumer would have tore-walk
checksto find out why, and a human reading the JSON cannot tell a new problem from thestanding one.
src/lib/doctor.jslistDegradedChecks(8 lines, exported for test);degradedderives from it;degraded_checksadded;queueChecksetsadvisory: truetest/doctor.test.jsopenwiki/cli-and-operations.mdNot a breaking change —
degradedhas never shippedThe field landed on
mainin #129, and that merge deliberately carried no version bump so it wouldnot publish (#135). No released version has
degradedat all, so narrowing it costs no consumeranything. This is the cheapest moment this fix will ever be available.
Verification
Run against the built CLI on the machine that reported #128, not a fixture:
Two warns still reported. One reaches the alert signal.
degraded_checksnames which. Before thischange the two were indistinguishable behind one boolean.
Both mutation points were checked by breaking them, not by watching tests pass:
advisoryfilter inlistDegradedCheckssummary.warn > 0 || summary.fail > 0predicateci:localexit 0 — 1002 node tests, 41 dashboard test files, all six validators, OpenWiki factcheck 0 findings.
Scope
src/)dashboard/) — nothing readsdegraded;grep -rn degraded dashboard/srcis emptyChecklist
npm run ci:localpassescopy.csvchange needed)0.39.44in lockstep, so merging this does not publish eitherKnown gaps
advisoryclassifies exactly one check. The other eight keep current behaviour by default, whichis the intended direction but means nobody has audited whether any of them is also a standing
condition. That audit is deliberately not in this PR.
--json: someone whoreads
degraded: falsewhilesummary.warnis 1 has to notice theadvisoryflag to understandwhy.
degraded_checksplus the openwiki paragraph are the mitigation; a CLI-side hint was notattempted.