feat(wizard): #1889 the setup page names which node check failed, and what a pass actually proved - #1937
Conversation
… and an absent report is not a failed one The host already refuses to provision when a configured remote node does not answer — `preflight_remote_nodes`, called from `12-firstboot-wizard.sh:372` before anything irreversible. What the operator got was one line of error text. #1898 publishes the full report beside that refusal as `node-probe.json`; this serves it to the page as `node_probe` so the wizard and the config preview can name WHICH endpoint failed and why. This is the read half only. Nothing renders it yet, so nothing changes on screen: the render module is the next commit on this branch. The gate is NOT duplicated here — it stays the host's, which is the same side `wizard.py:160` already puts "which step this machine is on" on. THE DEFECT THIS IS SHAPED AROUND. `_spool_json` fails open to `{}`, so an ABSENT report and a FAILED one both arrive with no usable `ok`. Reading that by truthiness would have blocked every machine that runs its own nodes, plus every host older than the report. Presence is therefore checked as `isinstance(report.get("ok"), bool)` — not `is not None`, which lets `"true"` and `1` through, and not truthiness, which lets `"false"` through as a pass. Proven, not assumed: - 16 new tests in a NEW file (`test_wizard.py` is at its 974/974 ceiling, so appending was not an option; split by defect class beside `test_wizard_saved_role.py`, reusing its harness). - MUTATION CONTROL, because a green run that has not been shown able to fail is not evidence. `isinstance(...)` -> `is not None` reddens exactly the two rows that pin the mechanism (`ok` as a string, `ok` as 1); returning the report unconditionally reddens 10. Restored by sha256, not by `git checkout`. - `tests/web` 720 passed rc 0; lint-file-budget, lint-operator-strings and lint-docs-voice all green, each with its own self-test, and run AFTER staging so the new untracked file was in scope. wizard.py 654/674. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KZuzdtxNX8ae4rnDtBqEqR
… what a pass actually proved The read half landed `node_probe` in the wizard state and rendered nothing. This is the render half: a failed probe now shows one row per endpoint — which node, which port, the address dialled, and a sentence for the reason — under the host's one-line error rather than instead of it. ONE module (`nodeprobe.mjs`), because the same report reaches an operator on two surfaces. The setup screen shows it today; the Configuration view's preview will show the same report once the control channel carries a probe of its own, and seven reasons worded twice is how two surfaces come to disagree about what a failure means. That second surface is NOT in this PR: nothing serves a probe to `/api/control/preview` at any head — I checked #1888's branch as well as develop rather than assuming its preview changes covered it — so its consumer would have no producer. THREE RULES ARE LOAD-BEARING, NOT STYLISTIC. - An unrecognised reason reads "the check did not complete", never "not reached". The emitter has seven reasons today and grows a name-resolution one next; inventing a reachability claim for a value this page has not seen is exactly the defect that fix exists to end. - `missing-tool` drops the host's own `detail`. curl rc 127 is OUR binary missing, and the host keeps its generic reach sentence on that row — the one naming the operator's host, port and LAN switch, every one of them innocent. It is the ONLY reason where the host's sentence contradicts the truth, so it is the only one suppressed, with a sibling proving the same string IS shown on a `refused` row. - The module renders no control at all, and a test forbids one. The gate is the host's (`preflight_remote_nodes` refuses and hands the form back), which is the side `wizard.py:160` already puts "which step this machine is on" on. A client-side disable would be worse than a courtesy: correcting the address and submitting again is the operator's only way out of a failed probe, and a dead button takes it away. A Tari pass renders QUALIFIED. `connect` is a bare TCP dial satisfied by any socket that accepts, and there is no Tari client on the machine to ask further — the copy leans on that mechanism, not on `docs/dashboard.md`'s "cannot park a chain on a node that is not there", which overclaims it. The skipped-endpoint gap is measured as `configured - probes.length`, NOT against the report's own `probed`. In a well-formed report the two are the same number; in a malformed one the array is what is actually on screen. M7 SURVIVED THE FIRST BATTERY, AND THE FIXTURE WAS THE BUG. Relaxing the presence check from `typeof ok !== "boolean"` to `ok == null` reddened nothing, because every malformed-verdict fixture carried no probes and the all-local door answered for it — the rows were green for the wrong reason. Widening them to carry real probes (which is the shape that bites: `ok: "false"` is TRUTHY) reddens it, and a control proves the widened fixtures arm. Proven, not assumed: - 22 tests in a NEW file — `wizard.test.mjs` is at its 765/765 ceiling, so appending was not an option; split by defect class, reusing the sibling harness. - MUTATION BATTERY, each mutation asserted to have applied (a needle that does not match writes the file back unchanged) and restored by sha256, not by `git checkout`. Seven on `nodeprobe.mjs`: every one reddens exactly the row that pins its mechanism, none survives. Two on `wizard.mjs` (drop `nodeProbe` from state; drop the card from the screen) both redden the seam test. - A SEAM test, because a correct component over a wrong served value is what survived the last fix on this page. It drives the real `WizardApp` against a real `/api/wizard-state` body and reads what the setup screen paints — not props handed in by hand. - Frontend suite 601 passed, 0 failed, rc 0. lint-js, lint-docs-voice, lint-operator-strings and lint-file-budget all rc 0, run AFTER staging so the two new untracked files were in scope. - No shellcheck and no docker: `appliance` holds gouda for a KVM battery. Budgets: wizard.mjs 888/889 (4 lines spent of 5), nodeprobe.mjs 124 and wizardprobe.test.mjs 301, both under the 400-line threshold that would demand a budget row. Docs: `docs/appliance.md` gains what the operator sees at "Validate, then install" (a Monero node is two live protocol checks, a Tari node one dial that proves only the port accepted); the wizard dev doc gains the three rules above. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KZuzdtxNX8ae4rnDtBqEqR
…he read half never checked CI caught this, not me: the read half ran lint-file-budget, lint-operator-strings and lint-docs-voice and did NOT run lint-py, so a signature ruff wanted on one line shipped wrapped. `ruff format --check` is part of lint-py and the gate is per-file, so the whole Python job went red on a branch whose second commit touches no Python at all. Formatting only — `ruff format` applied, no hand editing. lint-py now rc 0 (`ruff check` all passed, 227 files already formatted), and the file's 16 tests still pass rc 0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01KZuzdtxNX8ae4rnDtBqEqR
Non-author review — PASS at
|
…stead of contradicting it
The reviewer's FINDING A, taken. The headline keyed on `s.ok` while the skipped paragraph was
computed independently, so a report `{ok: true, configured: 3, probed: 1}` rendered a green "every
node check passed" directly above a red "2 of the 3 produced no result at all" — one card telling
the operator both things at once.
Unreachable from the shipped producer, whose `ok` is `(length == want) and all(.ok)` and so already
implies `probed == configured`. It is reachable from a MALFORMED report, which is the exact case my
own comment says `skipped` exists to survive — so the guard existed and the headline did not use it.
An endpoint that produced no row was not verified, so the failure headline is true there.
THE CONSEQUENCE LINE DELIBERATELY DID NOT MOVE. A host that published `ok: true` proceeded; telling
the operator that setup had stopped would be the opposite lie. Both the defect and that
over-correction are pinned: reverting to `s.ok`, and extending `passed` to the consequence, each
redden the new test.
The reviewer's tell was right and worth recording: my own skipped-render test forced `ok: false`
into its fixture, so nothing ever exercised the pair.
Also NIT D: the module header said "seven failure reasons". Six are failures; the seventh value is
`ok`, which `PASSED`/`PASSED_DEFAULT` word instead.
Proven: frontend suite 602 passed, 0 failed, rc 0. The 7-mutation battery on this module still
reddens every named row and none survives. lint-py, lint-js, lint-docs-voice, lint-operator-strings
and lint-file-budget all rc 0, run after staging.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01KZuzdtxNX8ae4rnDtBqEqR
Non-author DELTA re-pass — head
|
| mutation | node --test wizardprobe.test.mjs |
|
|---|---|---|
| M0 | none (control) | rc 0 — 23 / 23 pass |
| M1 | :105 → const passed = s.ok; — the defect, reverted |
rc 1 — 22 pass / 1 fail, not ok 16 |
| M2 | :114 → ${!passed && children && …} — the over-correction |
rc 1 — 22 pass / 1 fail, not ok 16 |
Both redden only test 16, the new one. Nothing else in the file moves either way.
node:assert short-circuits, so "test 16 reddened" is not the finding — I pulled the failing
frame out of each stack, and they are different assertions:
- M1 →
wizardprobe.test.mjs:214—assert.doesNotMatch(out, /Every node check … passed/), the
headline. - M2 →
wizardprobe.test.mjs:218—assert.doesNotMatch(card(…, "Setup does not continue."), /Setup does not continue/), the consequence.
The claim holds exactly: each mutation lands on the assertion that pins its own mechanism. The
test's control at :219-221 is real — a clean pass still says "passed", so the headline assertion
is not satisfiable by a card that never says it.
3. NIT D — "six" is the right number, confirmed at the producer
Every NODE_PROBE_REASON= assignment at 00f7d062:lib/pithead/10-installer-preseed.sh (whole-tree
git grep, untruncated): protocol (:283, :320), timeout (:284, :310), refused
(:285, :308, :340), auth (:309), missing-tool (:311), unknown (:312) — six
failure reasons — plus ok (:289, :317, :332) and the "" declaration at :302.
nodeprobe.mjs:33-40 has exactly those six keys. ok never reaches REASONS:
summariseRow:87 consults it only on the !ok arm, and a pass is worded by PASSED (:47-50) /
PASSED_DEFAULT (:51). So the seventh value is ok, it is not a failure, and it is worded
elsewhere. "six failure reasons" is correct, and the doc's "all seven reasons" and
test_wizard_probe.py:151's "SEVEN reasons" stay correct — they count values, not failures.
4. NIT C is taken, but three figures in the body are now wrong at the tip
Measured against git diff --stat origin/develop...57784daf and the files themselves.
Right: wizard.py +21 and 654 (ceiling 674); test_wizard_probe.py +197 and 9 test functions
/ 16 collected — I counted 9 async def test_ and exactly one @pytest.mark.parametrize
(:166) with 8 params, 8 + 8 = 16; nodeprobe.mjs +131; wizard.mjs +4 and 888 (ceiling 889);
wizardprobe.test.mjs +317; docs.
Wrong — all three are pre-delta figures the delta moved:
- Tier: "22 tests in a new file" → it is 23 (
grep -c '^test('), and the body's own table
says 23 four paragraphs earlier. The body contradicts itself. - Evidence: "Frontend suite 601 passed" → 602 at this head. Not a guess: CI's own
Frontend logic testslog at57784dafprints# tests 602 / # pass 602 / # fail 0, and
containsok 445 - skipped: a gap contradicts the headline, whatever the verdict says. - Budgets: "
nodeprobe.mjs124 andwizardprobe.test.mjs301" → 131 and 317 (both
files end in0a, so nowc -lundercount). The conclusion survives — both under
TARGET_LINES=400(scripts/lint-file-budget.sh:26), neither needs afile-budget.tsvrow —
but the numbers under it are stale.
None of these changes what anyone should DO, so it is a body correction, not a blocker — same
class as NIT C. One edit fixes all three. (Also: the Evidence section's seven-mutation battery ran
at the previous head and does not cover the passed line. §2 above does, and it is now on the
record from a non-author.)
5. CI at 57784daf — what I SAW, not an assumption. Not green yet.
gh api …/commits/57784daf/check-runs → total 22: 17 success, 2 skipped, 0 failing, 3
in_progress.
- Still running:
Dashboard tests (pytest + coverage),Shell tests (shellcheck + pithead suite),Dashboard image (Docker test stage). - The job that covers this delta is GREEN:
Frontend logic tests (node --test)—success,
602/602, and I read the new test'sok 445out of the job log rather than trusting the label. Python lint + format (ruff)success; the delta touches no Python, so the pending pytest job
is inherited risk from the earlier commits, not from57784daf.
This PASS is conditional on those three going green. Whoever merges must re-read the rollup at
the head they are merging — do not carry this paragraph forward as if it said green.
Verdict: PASS at 57784daf — non-author, conditional on the three pending jobs
- The delta is exactly (a)+(b)+(c).
- FINDING A correctly taken; leaving the consequence on
!s.okis the right call and I agree with
the reasoning. - The new test is honest — both mutations applied by me, reddening different assertions, each
the one that pins its mechanism; restored by sha256 to a byte-identical file. - NIT D's "six" is correct at the producer.
- NIT E and the three stale body figures are follow-ons. Neither blocks.
Base is develop and mergeStateStatus is BEHIND, so this is not mine to merge — it goes to
the merge seat, and never to the author.
What I ran: git, gh, and node --test on the one file the review needed, in a detached
worktree at 57784daf (removed after), plus a ten-line render script for NIT E.
What I did NOT run: no make, no pytest, no shellcheck, no shfmt, no docker, no image build, no
KVM, no stack suite — a heavy job was holding the local test capacity, so CI is the authority for
everything above except the mutation battery. The producer half was read at 00f7d062, not
executed; it is an unmerged branch.
|
Author, closing the loop on the delta re-pass. This is MERGE-READY and not mine to merge. The re-pass condition is discharged — CI is fully green at
|
| body claim | was | is | how measured |
|---|---|---|---|
| tests in the new file | 22 | 23 | grep -c '^test(' |
| frontend suite | 601 | 602 | node --test dashboard/tests/frontend/, # pass 602 / # fail 0, rc=0 written by me |
nodeprobe.mjs / wizardprobe.test.mjs |
124 / 301 | 131 / 317 | wc -l |
probeSummary asserted directly |
19 of 22 | 16 of 23 | tests that never call the render helper |
The fourth row was not in the review — the justification line still read "19 of the 22". The
budget conclusion is unchanged: both files are under the 400-line threshold, so file-budget.tsv
stays untouched.
What I did NOT do
No rebase, no new commit, no merge. I did not re-run the pytest, shell or image jobs locally — CI's
own green at this head is what I am citing, and another lane holds the bench.
1 participant
The UI half of #1889. The issue stays open — item 3 (the same report in the Configuration view) has no producer yet, so no closing keyword is used here on purpose.
What the operator sees change on the appliance
Today a remote node that does not answer fails at "Validate, then install" with one line of error text. The host already knows far more than that —
preflight_remote_nodesprobes every configured endpoint and writes the full report — and none of it reached the page.Now the setup screen shows, under that same error line, one row per endpoint: which node, which port, the address it dialled, and what happened. A Monero node contributes two rows (RPC and ZMQ), a remote Tari node one.
Three things it deliberately does not do:
connectis a bare TCP dial satisfied by any socket that accepts, and there is no Tari client on the machine to ask further — so that pass reads "the port accepted a connection; what is listening behind it was not checked".missing-toolfailure. curl rc 127 is our binary missing; the host keeps its generic reach sentence on that row, naming the operator's host, port and LAN switch, all innocent. That one reason drops the host detail and says the fault is here.A machine that runs its own nodes probes nothing, so it sees no card at all — an absent report is not a failed one.
What is actually in the diff
Four commits, two halves. The body originally described only the second half — the reviewer's NIT C, taken.
dashboard/mining_dashboard/wizard.py_node_probe()servesnode-probe.jsonasnode_probe, via the existing_spool_jsonidiom. 654/674.dashboard/tests/web/test_wizard_probe.pytest_wizard.pyis at its 974/974 ceiling.dashboard/mining_dashboard/web/static/nodeprobe.mjsdashboard/mining_dashboard/web/static/wizard.mjsdashboard/tests/frontend/wizardprobe.test.mjsdocs/appliance.md,docs/dev/appliance-wizard.mdPython checks:
lint-pyrc 0 (ruff checkall passed;ruff format --check227 files already formatted) and the 16 cases pass rc 0 locally. The read half originally shipped withoutlint-pyhaving been run, which is what CI's ruff job caught ata355a1b4; fixed in64c21cff.Review
Non-author PASS from the ephemeral
reviewerat64c21cff(comment5558611434), conditional on the pending CI jobs. That pass named64c21cffand the head has since moved to57784daf, so it does not cover the tip — it needs a re-pass at the current head.57784daf: the headline keyed ons.okwhile the skipped paragraph was independent, so a malformed{ok: true, configured: 3, probed: 1}rendered a green "every check passed" above a red "2 of the 3 produced no result". The reviewer's tell was right — my own skipped-render test forcedok: falseinto its fixture, so nothing exercised the pair. The consequence line deliberately did NOT move: a host that publishedok: trueproceeded. Both the defect and that over-correction redden the new test.node-probe.jsoncan outlive the config it judged, because nothing removes it whileerror.txtis cleared on every submit. That is one line on the producer's reject arm and is blocking on fix(wizard): #1889 the node probe verifies the protocol, and reports which check it made #1898. Relayed to the lane that owns it; recorded here so it is not lost if these merge out of order.Stacking and base
origin/develop(merge-base0a4195c9). Not rebased onto any PR.fix/1889-remote-node-probe), UNMERGED. Nothing writesnode-probe.jsonondevelop, so the consumer is built against fixtures taken fromnode_probe_oneat00f7d062— the row keys, the seven reasons, and the host's owndetailsentence for each. The two land in either order: with fix(wizard): #1889 the node probe verifies the protocol, and reports which check it made #1898 unmerged the served value isnulland the screen is unchanged.nodeprobe.mjs(new),wizard.mjsand two docs; no other in-flight branch of mine editswizard.mjs.What is NOT here, and why
Item 3 of the issue — the same report in the Configuration view's preview, with apply refused while it fails — has no producer. Nothing serves a probe to
/api/control/previewat any head; I checked #1888's branch as well asdeveloprather than assuming its preview changes covered it. Building the consumer now would be a view with nothing to view. The render module is written so that surface passes its own consequence sentence and reuses the same reason vocabulary, so the two cannot come to disagree about what a failure means.Tier
Tier 1 (frontend logic + render probes), per
docs/dev/testing-strategy.md— 23 tests in a new file,dashboard/tests/frontend/wizardprobe.test.mjs. A new file rather than an append becausewizard.test.mjsis at its 765/765 ceiling; split by defect class, reusing the sibling harness.The issue also names tier 3 (a KVM provision leg against a guest that is not listening). That belongs with the host half and is not added here — and
tests/os/setup-again-leg.shnever renders a wizard screen (it spools over SSH), so wizard-screen work has no tier-4 cover either way. Recorded, not silently skipped.Evidence
git checkout. Seven onnodeprobe.mjs— unrecognised reason defaulting to "not reached";missing-toolkeeping the host's detail; a truthyokas a pass; a connect pass reading as verified; the skipped gap trustingprobed; the all-local card; presence as!= null— each reddens exactly the row that pins its mechanism, none survives. Two onwizard.mjs(dropnodeProbefrom state; drop the card from the screen) both redden the seam test.ok: "false"is truthy, which is the shape that bites) reddens it, and a control proves the widened fixtures arm.WizardAppagainst a real/api/wizard-statebody and reads what the setup screen paints, rather than props handed in by hand.lint-js,lint-docs-voice,lint-operator-strings,lint-file-budgetall rc 0, run after staging so the two new untracked files were in scope.Budgets
wizard.mjs888/889 (4 of the 5 free lines spent).nodeprobe.mjs131 andwizardprobe.test.mjs317 are both under the 400-line threshold that would demand a budget row, sofile-budget.tsvis untouched.Docs
docs/appliance.mdgains what the operator meets at "Validate, then install" — a Monero node is two live protocol checks, a Tari node one dial that proves only that the port accepted.docs/dev/appliance-wizard.mdgains the three load-bearing render rules.Over-engineering pass
Run on the merits, not by the hook — five things weighed, one kept with a caveat.
childrenrather than being hard-coded. This is the diff's thinnest justification: only one surface passes it today. Kept because it is a prop read replacing a string literal — no extra code — and because "setup does not continue" and "the apply is refused" are genuinely different sentences that should not be chosen inside the shared module.probeSummaryis exported apart from the component. Justified: 16 of the 23 tests assert the vocabulary against it directly, never rendering the card, and asserting seven reason sentences through rendered HTML would be noise.hostcan be""(jq// ""), the report is JSON written by a shell script, and the spool reader fails open to{}. None is speculative.verdict+sentence) rather than one, kept: the verdict is the scannable coloured word, the sentence is the explanation, and merging them makes the failure list unreadable at a glance.detail. Smaller, and it is precisely the defect — the host's sentence is wrong formissing-tooland absent for any reason it does not name.I am the author, so I do not merge this.
🤖 Generated with Claude Code
https://claude.ai/code/session_01KZuzdtxNX8ae4rnDtBqEqR