Skip to content

fix(cli): the CLI/service/updater batch — six user-visible dead ends - #458

Merged
MichaelTaylor3d merged 8 commits into
mainfrom
loop/batch-cli
Aug 31, 2026
Merged

fix(cli): the CLI/service/updater batch — six user-visible dead ends#458
MichaelTaylor3d merged 8 commits into
mainfrom
loop/batch-cli

Conversation

@MichaelTaylor3d

@MichaelTaylor3d MichaelTaylor3d commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

What this is

The dig-node CLI / service / updater batch. Six tickets that are each a user-visible lie or dead end on a real machine, which is why they group.

# Outcome
#303 dign network-info documented but absent fixed — the verb added; the node always served the data
#304 zero peers told to upgrade fixed — the two empty states told apart
#305 .deb upgrade left the old binary running fixedtry-restart on upgrade, verified in the package
#407 IO_ERROR in the restart window fixed — new exit code 12 NODE_UNREACHABLE
#403 root-owned control token decision + the packaging-adjacent fix; the dig-app half remains
#392 silent wallet sidecar partly — the logging-degrade half only

Closes #303
Closes #304
Closes #305
Closes #407

(Outside code spans deliberately — inside backticks GitHub does not parse the keyword, so the issues would not autoclose.) #403 and #392 stay open deliberately — see below; closing them would be the paperwork lie this batch is about.

The #403 shape decision, and why

The token is the master capability, not merely "control": it authorizes pairing administration (mint/list/revoke) and chiaPeers.add, which grants chain authority over the node's wallet replica.

  • A dig group at 0640 hands that to every member, permanently, and outlives the app that motivated it.
  • A --user unit changes what the node is on a server — no boot start, per-login lifetime — for the one shape where an always-on node matters most.

So: do not widen it. The node already has the right primitive for a client that cannot read a file — pairing (pairing.rs), built for exactly this and used today by the MV3 extension. It yields a scoped, revocable per-client token that cannot mint or revoke pairings and cannot grant chain authority. The principal admitted is one operator-approved client on this host with mutation rights minus the master tier; strictly less than a group grant, and revocable without touching a file mode. The token stays 0600 root:root.

What lands here is the half that was in dig-node's hands: the remedy text told the operator to "reinstall so the service grants your account read access". True on Windows (the installer keeps an explicit read grant for the interactive install-user), false on Ubuntu — the .deb leaves 0700/0600 and reinstalling changes nothing, so the operator loops. The platform is now a function argument, not a cfg! branch, so both sentences are asserted on any host.

Blast radius

.gitnexus's registered index points at the primary checkout and is ~300 commits behind, so a query would return a false-safe zero. This was done with grep + direct read, and is stated as such.

symbol callers effect
format_status 1 (peers::run) rendering only
call_async 2, both in-file signature widened to Option<&str>; call_control byte-identical
ExitCode::from_io_error entrypoint, seed_export_cli, spend_audit_cli new ConnectionRefused arm
control_token_remedy_for server.rs (log + 401 body), CLI error message only
postinst packaging no Rust caller

The one contract change, called out so the gate can disagree with the bump: an unreachable node now exits 12 where it exited 6. Additive to the catalogue, but a script matching 6 sees a change. Bumped 0.189.0 → 0.190.0 (minor: new network-info capability).

Why 12 and not 7 — the occupancy was measured, not assumed

NODE_UNREACHABLE was first assigned 7 by reading this CLI's own table, where 7 genuinely was the next number. 7 is NOT_CONNECTED in dig-app's diga, whose outcome.rs states in its own doc comment that it deliberately shares dig-node's numbering — and 8–11 are taken as well. This is the identical reasoning that already cost a yank in this repo's JSON-RPC error space, where -32015 was taken as "the next free code" from the owning crate's own list and collided with a released METADATA_TOO_LARGE.

Measured across dig-node, dig-app, dig-installer, dig-updater and this repo's scripts and e2e harnesses. The installer / digstore / example std::process::exit() calls are separate binaries with their own namespaces, not this shared catalogue, and are excluded on that basis.

Code dign diga
0 OK OK
1 NOT_SERVING
2 USAGE USAGE
3 PERMISSION_DENIED
4 SERVICE_FAILED
5 BIND_FAILED
6 IO_ERROR IO_ERROR
7 NOT_CONNECTED
8 ENGINE_ERROR
9 LOCKED
10 NOT_FOUND
11 DENIED
12 NODE_UNREACHABLE (this PR)

0–11 were therefore taken before this CLI added a code, and 12 is the first free number. The map is now written into SPEC.md §8.4 so the next person does not re-derive it, and no_exit_code_collides_with_the_dig_app_gateway_numbering fails if a shared number ever carries two meanings.

Evidence — what was measured, and where

Test counts checked, not just exit statuses.

Stated as unverified

Gate round 2 — the six findings, and what each fix is proved by

# Finding Fix Test that catches a revert
1 The new Unix remedy named sudo dign pair as the approving verb; a bare pair maps to PairAction::List. The test asserted contains("dign pair"), satisfied by the wrong string as its own substring. The remedy names sudo dign pair approve <pairing_id>, and says the bare verb only lists. the_unreadable_token_remedy_offers_a_scoped_credential_not_a_wider_file now asserts the full command, plus a one-sided row that fails on the false attribution `sudo dign pair` approves. New the_bare_pair_verb_lists_and_only_approve_approves pins the mapping against the parser via Cli::try_parse_from, so the claim is measured against behaviour rather than against itself.
2 Exit 7 is NOT_CONNECTED in diga; 8–11 taken too. Releasing 0.190.0 would freeze the collision. Renumbered to 12; the occupancy map written into SPEC.md §8.4. no_exit_code_collides_with_the_dig_app_gateway_numbering walks every ExitCode against the transcribed diga map and fails if a shared number carries two meanings.
3 SPEC's "anything else → 6" was false in the commit that added the 7 row, and already silently wrong about InvalidInput → 2. Every arm of from_io_error is now listed, in match order. Docs. No test can see this class of drift — stated rather than papered over.
4 network-info reached no doc, including §8.1's normative subcommand list, and §8.6 asserts the opposite token property. New §8.8 documents the verb and states it is the one deliberate exception to §8.6's token rule (it reads the open, already-public dig.getNetworkInfo body); §8.6 cross-references it; §8.1 lists it; README gains a section. Docs. No test can see this — stated.
5 The logging change had zero tests, and shipped two runs of ~22 literal spaces from a lost string continuation. Decision extracted as the pure degrade_announcement; text as FILE_LOGGING_DEGRADED, built with concat!. Four: a_live_file_sink_announces_no_degrade (the silent control, so an unconditional warner cannot pass), a_failed_file_sink_announces_and_names_the_reason (asserts the reason by value), the_announcement_says_an_empty_directory_means_denied_not_quiet (the ambiguity the message exists to resolve), the_announcement_text_has_no_lost_string_continuation.
6 Release-script tests runs on ubuntu-latest but is not a required context, so #305's only executing proof cannot block a merge. try-restart under the #317 marker was reasoned, not tested. The context is now required on main. A marked-upgrade case added to the existing stub-systemctl harness. Three rows: no unconditional starter under the marker, try-restart still emitted (one-sided in the opposite direction, so neither is satisfied by a postinst that does nothing), and a daemon-reload control proving the configure branch actually ran.

The whitespace test earned itself within the hour

The first repair of finding 5 used a -continued string literal. cargo fmt rejoined the lines and materialised the leading indentation straight back into the string — reintroducing the exact defect, silently, between writing the fix and running it. the_announcement_text_has_no_lost_string_continuation failed and caught it. The constant now uses concat!, which has no whitespace a formatter can reinterpret.

Revert-proofs — committed first, reverted by file copy

Three fixes reverted independently — the pairing verb, the exit code, the whitespace. 660 passed, 3 failed, exactly one failure per test, each for its own reason. Restored afterwards; the tree hashes identical to the commit.

  • Full lib suite after restore: 663 passed, 0 failed, 0 filtered out.
  • cargo fmt --all --check clean.

Branch protection was changed

Release-script tests was added to main's required contexts — now Lint commit messages, Check version increment, Rustfmt, Clippy, Test + coverage, Release-script tests — and read back to verify. The job already ran on every PR to main, so live sibling PRs already report the context and are not blocked by this.

Still stated as unverified

  • The parser test for finding 1 is a guard, not a revert-proof of a fix. It asserts pre-existing parser behaviour, so it fails if the parser changes, not if the remedy text does. The one-sided text row is what covers the text.
  • dpkg-deb is unavailable on this Windows host, so deb-contents.test.sh SKIPs locally (confirmed: it prints the SKIP) and runs in CI, which is now required. The new marked-upgrade rows have not executed anywhere yet — CI is their first run.
  • Findings 3 and 4 are documentation corrections with no mechanical guard. A future edit can silently re-introduce either.

Branch loop/batch-cli, head a4e436292d96dcafa983c62a187215919bd81844.

MichaelTaylor3d and others added 4 commits August 31, 2026 06:41
Co-Authored-By: Claude <noreply@anthropic.com>
…inary is the one running

`systemctl enable --now` is a no-op on a unit that is already enabled and running, so a
`dpkg`/`apt` upgrade replaced /usr/bin/dig-node and left the OLD process serving. The failure was
silent in both directions an operator checks: `dig-node --version` reads the on-disk image and
reports the new version at once, and `systemctl is-active` reports active because the old process
is genuinely healthy. Only MainPID moved. A security fix shipped through the .deb therefore did
not take effect on upgrade.

postinst now runs `systemctl try-restart` when dpkg passes a previously-configured version ($2),
i.e. on an upgrade only. `try-restart` rather than `restart` so a node the operator deliberately
stopped -- including one held back by the #317 no-autostart marker -- stays stopped.

macOS needs no change: its postinstall already does bootout + bootstrap + kickstart -k.

Refs #305

Co-Authored-By: Claude <noreply@anthropic.com>
…to upgrade

Two user-visible lies on the CLI's network surface.

`network-info` was documented and expected but never existed as a subcommand, so it presented
as empty output on three healthy fleet boxes -- what an unrecognised subcommand looks like once
a shell has swallowed the usage text. The node has always ANSWERED the question: `dig.getNetworkInfo`
serves peer id, network + genesis, advertised candidates (IPv6-first, §5.2), reachability and the
relay reservation. Only the verb was missing, so this adds the verb rather than the data.

It reads through a new token-free `control_client::call_open`. Every field it prints is already
published to any peer that dials this node, so the control token would buy no confidentiality
while costing real availability: on a .deb install the master token is 0600 root:root (#501), and
an ordinary user asking "what is my node's address" would be told to elevate for a read the
network performs for free. `call_control` is untouched and still carries the token.

`peers` conflated two different empty states behind one message: a node with zero peers was told
"a per-peer list needs a newer node" -- on 0.138.0, the newest build that existed. The advice was
false and pointed at a release that does not exist, and it appears on first run, when a user has
zero peers and is least able to tell. The connected count distinguishes them: zero connected is
stated plainly with something to try; peers counted but not enumerable keeps the version note,
which is the only state that actually implies one.

Refs #303, #304

Co-Authored-By: Claude <noreply@anthropic.com>
…lure

`dign updater check-now` probing inside the restart window reported IO_ERROR, which reads to an
operator exactly like an update that broke the node. It is the opposite: a successful pass installs
new bytes and cycles the service, so the restart is the normal END of the thing that succeeded.
Same class as dig-updater#77, on the surface #77's fix does not reach.

IO_ERROR says an I/O operation was attempted and failed -- a claim about the request. An
unreachable node is a failure to MEASURE, not a measured failure, and sharing one encoding is what
trains an operator to ignore the surface, which is how the one real failure gets ignored too. That
matters more under the silent-staged-install policy (dig_ecosystem#3180), where the status surface
is all an operator has.

So the loopback control client's ConnectionRefused class now resolves to a new exit code
7 NODE_UNREACHABLE, catalogued in cli.rs, README, SPEC and USER_JOURNEY. Its message is restated
at the one choke point in control_cli::run: `control.updater.*` gets the restart explanation
because it has a specific expected cause, everything else gets the general statement. Only the
message changes and only for that kind -- a node that ANSWERED and declined has measured
something, and its own words pass through untouched.

No timeout was widened: that converts a wrong answer into a slower wrong answer.

Refs #407

Co-Authored-By: Claude <noreply@anthropic.com>
MichaelTaylor3d and others added 2 commits August 31, 2026 07:22
#403 -- the dead end was the ADVICE as much as the permission. The unreadable-token remedy told
the operator to "reinstall the current dig-node so the service grants your account read access".
On Windows that is true: the installer keeps an explicit read grant for the interactive
install-user. On Ubuntu it is false -- the .deb leaves /var/lib/dig-node at 0700 root:root and the
token at 0600, so reinstalling changes nothing and the operator loops.

The decision, recorded in full on the ticket: do NOT widen the token. It is the MASTER capability
-- it authorizes pairing administration (mint/list/revoke) and chiaPeers.add, which grants chain
authority over the wallet replica. A `dig` group at 0640 hands that to every member permanently
and outlives the app that motivated it. The node already has the right primitive for a client that
cannot read a file: pairing, built for exactly this and used today by the MV3 extension. It yields
a scoped, revocable per-client token that cannot mint or revoke pairings and cannot grant chain
authority. So the Unix remedy now names `sudo dign pair` and its revocation.

The platform is a function ARGUMENT rather than a cfg! branch, so both sentences are asserted on
any host -- a cfg-gated assertion is untested on exactly the platform this defect was found on.

#392 (the logging half) -- the file-sink degrade was RECORDED on control.status but never
ANNOUNCED. A non-admin run denied C:\ProgramData\DigNetwork\logs came up console-only in silence,
so an empty log directory could not be told apart from a quiet node. The console layer is
installed on that path, which is why the warning reaches someone.

Refs #403, #392

Co-Authored-By: Claude <noreply@anthropic.com>
Minor. New capability: the `network-info` subcommand. The exit-code catalogue gains 7
NODE_UNREACHABLE, which is additive to the table but does CHANGE the code a script sees when the
node is unreachable (was 6 IO_ERROR); called out here and in the PR body so the gate can weigh it.

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Gate — IN PROGRESS, not the verdict. Head read: de237fc24aa67e3c5a0ed207600f6ce45b74edc5. .gitnexus index is ~300 commits stale and points at the primary checkout, so this was done with grep + direct read in a private worktree.

Cleared so far — the call_open surface (priority check 1).

  1. call_open is client-side only and adds no server-side reachability. crates/dig-node-service/src/server.rs is not in the diff; every plane's gate (control.* prefix at server.rs:1134, wallet at :1212, cache.* at :1267, chat.* at :1298, and the WS parity gate at :1556) is keyed on the method name server-side, not on whether a token was presented. A call_open call naming a gated method gets -32030, not service. Nothing previously gated became reachable.
  2. No silent adoption. grep -rn call_open crates/ returns exactly three hits: the definition (control_client.rs:62), the import and the single call site network_info.rs:29, with a literal "dig.getNetworkInfo". No other verb switched off the token.
  3. The claim that the node already served it holds, on the loopback path specifically — not just on the peer channel. crates/dig-node-core/src/seams/dig_rpc/dispatch.rs:280 returns node.network_info() from the same handle_rpc_as fall-through the loopback HTTP handler reaches at server.rs:1335, after all four gates decline it. Ungated by construction.
  4. The renderer's field names match the real payload, so the unit fixtures are not a self-consistent invention: lib.rs:4082-4095 emits peer_id, network_id, genesis, listen_addr, candidate_addresses, reachability, relay — all seven read by format_network_info (network_info.rs:39-76). This was the likeliest false-green in the batch and it is not one.

Still open in this pass: the Closes set, the exit-code 6→7 blast radius outside this crate, the postinst package proof, and an independent revert spot-check.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Gate — IN PROGRESS, not the verdict (head de237fc2). Second batch of established findings.

GATING — SPEC.md:2626 still states the OLD mapping rule, one line under the table this PR updated.

I/O-error mapping: `PermissionDenied` → 3; `AddrInUse`/`AddrNotAvailable` → 5; anything else → 6.

ConnectionRefused is now → 7 (cli.rs:116), so the normative sentence is false in the commit that added the row above it. This is the contract-split shape, not a doc nit: the table and the prose in the same section of the same SPEC now describe two different mappings, and a reimplementer building from the prose ships 6. (The same sentence has been silently wrong about InvalidInput → 2 since that arm landed, which is how it survived — the sweep should fix both clauses, not only the new one.) The fix must restate the rule over the set, e.g. PermissionDenied → 3; InvalidInput → 2; AddrInUse/AddrNotAvailable → 5; ConnectionRefused → 7; anything else → 6.

GATING — network-info is absent from the SPEC's normative subcommand list. SPEC.md:2399-2404 (§8.1) enumerates the verb set; the new verb is in entrypoint.rs:190, README.md's exit table gained a row, and no doc gained the command. §4.2 makes §8.1 the authoritative surface an independent implementation is built against, and this is precisely the hand-maintained-table-beside-the-code surface that drifts with no test watching: the CONTROL_METHODS drift test at control_cli.rs:1580 cannot see it, because network-info deliberately is not a control method. Add it to §8.1, and state the property that makes it different — it reads the OPEN dig.getNetworkInfo surface and presents no token — because that is a security-relevant contract clause, not an implementation detail, and §8.6's opening paragraph currently asserts the opposite for the whole control-parity set ("presenting the MASTER control token").

NON-GATING — the Closes keywords are inside inline code spans. Closes #303, ... are written as `Closes #303` in the PR body. GitHub does not resolve issue references inside code spans, so on merge these four very likely do not autoclose — which is the exact manual-close gap the two-queue split exists to remove. Unbacktick them before merge. I have verified the intent is right: no closing keyword anywhere in the body or in any of the six commit messages touches #403 or #392, so neither can close by accident.

CLEARED — #305, in the package. packaging/linux/build-deb.sh:130-148: the try-restart is inside the command -v systemctl block, guarded by [ -n "${2:-}" ], and dpkg passes the previously-configured version as $2 only on upgrade — so first install is untouched and cannot fight the installer, which is what the priority check asked. try-restart over restart is the right primitive and the test asserts the semantics rather than the spelling (deb-contents.test.sh:180), so a later simplification to restart fails. The control at :189 — a first install emitting no restart at all — is what makes the upgrade assertion load-bearing, and it is a real control rather than a restatement.

One correction to the PR body's evidence claim: try-restart under the #317 marker is reasoned, not tested. MARKED_LOG (:144) is a first-install run, so there is no marker-plus-upgrade fixture anywhere. The reasoning is sound (try-restart no-ops on a stopped unit, so a held-back node stays down), and I am not gating on it — but the body states it as measured and it is not. Either add the fourth fixture (one line: run_postinst "$MARKED_ROOT" "0.1.0") or drop the claim.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Gate — IN PROGRESS, not the verdict (head de237fc2). The #403 half.

The decision itself: I agree, and would not widen the token. The reasoning holds on inspection rather than on assertion — control::requires_master_token is the tier the server actually enforces (server.rs:1218-1230), and it does cover pairing administration and chiaPeers.add/.remove, so the token really is the master capability and not merely "control". A dig group at 0640 is a permanent, unrevocable, transitive grant of chain authority over the wallet replica, bought for one app's convenience and outliving it; a paired token is scoped, revocable and per-client. The right call, and the right primitive. Making the platform a real argument (remedy_for_unreadable_token(path, dir, unix), control.rs:175) instead of a cfg! branch is also right, and the test asserts both branches on any host including the Windows control clause — that is a genuine control, not a restatement.

GATING — but the new Unix remedy names a command that does not do what the sentence says it does.

crates/dig-node-service/src/control.rs:182:

sudo dign pair approves the client's pending request

It does not. entrypoint.rs:851 maps None | Some(PairCommand::List) => PairAction::Listbare dign pair LISTS. Approving is dign pair approve <pairing_id> (entrypoint.rs:852pair.rs:45). So an operator on Ubuntu who follows this new advice runs sudo dign pair, gets a listing, nothing is approved, and they are stuck at the same wall — which is the identical failure mode as the reinstall loop this PR exists to remove, one sentence further along. The ticket is about a remedy that sends the operator round a loop that cannot succeed; replacing it with a remedy that names the wrong verb does not close it.

The test cannot catch this and should be strengthened in the same change: control.rs:213 asserts only unix.contains("dign pair"), which the correct string and the wrong one satisfy identically. Assert the verb — contains("dign pair approve") — so the remedy is pinned to a command that performs the action the sentence promises.

Two things the fix must NOT do: do not delete the pairing route (it is the correct answer), and do not restore the reinstall clause on Unix (it is the false one). The whole change is the missing word approve, plus the assertion that keeps it.

While there: the sentence also under-describes the flow. Approval presupposes a pending request, which the client raises through the open pairing.request method (server.rs:1118); an operator who has not started the client has nothing to approve and sudo dign pair will show an empty list. One clause — "start the client so it requests pairing, then sudo dign pair to see it and sudo dign pair approve <id>" — makes the remedy followable end to end instead of at the second attempt.

Still running: the --lib suite and an independent revert spot-check.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Gate — IN PROGRESS, not the verdict (head de237fc2). The #392 logging half.

GATING — the logging change has NO test, and the evidence line counts pre-existing ones as if it did. The PR body reports #403 + logging 6/6. logging.rs contains exactly five tests (:185, :193, :201, :213, :222) and all five predate this PR — they exercise health(...), the 0.2.0 status field, which is precisely the surface the new comment says was already there and was not the problem. The new behaviour — if let Some(reason) = guard.file_error() { tracing::warn!(...) } at logging.rs:92-99 — can be deleted in full and every one of those six stays green. §2.1 does not permit a behaviour change with no failing-test-first, and the property here is the one the ticket is about: that something is said at the moment the degrade happens.

The cheap fix is to make the announcement testable rather than to test init: lift the sentence into a pure fn file_degrade_warning(dir: &Path, reason: &str) -> String and assert it names the directory, names the reason, and says nothing is being written — then warn! renders it. That also removes the second problem below. (A tracing capture-subscriber test would work too, but it is more machinery for the same assertion.)

NON-GATING — that warning's text is mangled and ships to operators that way. logging.rs:98 is a single-line literal containing two runs of ~22 literal spaces, from a line-continuation that was lost:

"the rolling log FILE could not be opened; this run logs to the console ONLY.                      Nothing is being written to that directory -- an empty log directory here                      means logging was denied, not that the node was quiet."

cat -A shows the runs are real spaces in the source, not diff rendering. It compiles, it is not caught by fmt or clippy, and the operator reads a message with two gaps in it. Fixing it falls out of the extraction above; if the string stays inline, use \-continuations and check the literal back with cat -A rather than by eye.

CLEARED — #304 distinguishes all three states, and by cause rather than symptom. peers.rs:196-207: not running → its own sentence (:184); running with connected == 0 → "no peers connected yet" plus a named action (dign peers connect <address>); running with peers it cannot enumerate → the version-gap note it was actually written for. The count that separates them is read from the payload the node already sends. The two tests are a genuine pair — :713 and the pre-existing version-gap test vary only the connected count against the same shape, so an unconditional reword fails one of them. The peers: [] versus absent-key case (:732) is the right third fixture: two spellings of "no peers" that must not diverge into two messages.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Gate — IN PROGRESS, not the verdict (head de237fc2). The #407 blast radius, measured outside this repo.

GATING — exit code 7 is already taken, by the sibling CLI that exists to share this numbering.

modules/apps/dig-app/crates/dig-app-core/src/gateway/outcome.rs:28:

/// 7 — no identity-authenticated session to the engine (dig-app not attached / engine down).
NotConnected,

and :14 states the intent in its own words: "The overlapping codes (OK, USAGE, IO_ERROR) share the engine CLI's numbers so the two command lines agree." dig-app's diga deliberately mirrors dig-node's catalogue, and it occupies 7 = NOT_CONNECTED, 8 ENGINE_ERROR, 9 LOCKED, 10 NOT_FOUND, 11 DENIED. As of this PR, dign says 7 = NODE_UNREACHABLE and diga says 7 = NOT_CONNECTEDone number, two symbols, across two command lines whose contract is that they agree, and dig-node's own SPEC.md:2627 says numbers and names are a stable contract.

The PR body's compatibility analysis reads "additive to the catalogue" and considers only consumers matching on 6. That framing missed the collision, because the risk was never only "who matches 6" — it is "who else owns this number space". I searched for the 6-matchers as asked and found none: no script in this repo, nothing in dig-installer, and dig-app references IO_ERROR only in this same file. So the break the lane flagged is genuinely low-risk; the one it did not flag is the real one.

This is not fatal to the design — the two meanings are nearly the same event (the engine is not answering), which is why it is worth resolving deliberately rather than by coincidence. The fix is a decision, and it belongs in this PR because a released 0.190.0 makes the number immutable:

  • Preferred: align on the sibling's symbol. NOT_CONNECTED at 7 in both, with dig-node's rationale doc-comment kept. One number, one name, and diga's claim stays true.
  • Otherwise: take the next number free in BOTH catalogues (12+, since 8-11 are also occupied) and say in the SPEC that 7-11 are reserved by diga.

Whichever is chosen, the shared numbering needs a home so this cannot recur — the canonical skill is where a cross-repo value like this belongs (§2.0 append-on-discovery), and neither catalogue currently points at the other. What the fix must NOT do is silently renumber diga: that catalogue is published and SPEC.md:2627's no-renumbering rule binds it equally.

CLEARED on the mechanism itself. explain_unreachable (control_cli.rs:266) preserves e.kind(), so the exit-code resolution is unaffected by the message rewrite, and its three tests are a real triple: the updater sentence, a genuine Other decline that must pass through untouched (:316 — the control that stops "treat every updater failure as a restart" from passing), and a non-updater verb getting the general statement. The from_io_error pair at cli.rs:82/:102 asserts the distinction rather than the outcome, with Other → IO_ERROR as the control, which is the right shape.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Gate — IN PROGRESS, not the verdict (head de237fc2). CI sufficiency for #305.

Baseline suite reproduced. cargo test -p dig-node-service --lib on this head: 657 tests ran, 657 passed, 0 failed, 0 filtered out — counted, not read off the exit status.

GATING (cheap) — "gated in CI" is not true as written. scripts/tests/deb-contents.test.sh does run for real in CI rather than skipping: the scripts job is runs-on: ubuntu-latest (ci.yml:38) and iterates every scripts/tests/*.test.sh (:51-56), so dpkg-deb is present and the #305 assertions execute. That half of the question is fine.

The problem is that the job does not gate anything. Branch protection's required contexts on main are exactly:

Lint commit messages · Check version increment · Rustfmt · Clippy · Test + coverage

"Release-script tests" is not among them. So the only executing proof of #305 anywhere — locally it exits 0 after printing SKIP: dpkg-deb unavailable, which I reproduced — sits in a job whose failure does not block a merge. A packaging regression would go red in a place nothing reads, which is the same shape as an action_required check: not red, just not counted.

Two acceptable outcomes, and this PR should pick one rather than leave the sentence standing: add Release-script tests to the required contexts (branch protection is superproject-owned, so this is an orchestrator action, not a lane one), or amend the PR body to say the packaging proof runs unblocking. I would take the first — a fix whose entire evidence is one shell test deserves that test to be a gate.

Independent revert spot-check running now.

@MichaelTaylor3d MichaelTaylor3d left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CHANGES-REQUIRED

Head read: de237fc24aa67e3c5a0ed207600f6ce45b74edc5 (resolved from the remote; it did not move during this review). Reviewed in a private worktree — the primary checkout and the seven live sibling worktrees were not touched. .gitnexus is ~300 commits stale and registered against the primary checkout, so a query there would return a false-safe zero; this was done with grep + direct read, and where I could not reach something I say so at the end.

GitHub rejects a verdict-bearing review event from the PR author's identity with 422, so this is recorded as a comment review. The six inline threads are what bar the merge, via required_conversation_resolution.

The batch is largely right, and the parts that matter most are right

I want to be plain about this, because five of the six findings are about the edges rather than the core:

  • call_open does not widen anything. server.rs is not in the diff, and every plane's gate — control.* at :1134, wallet at :1212, cache.* at :1267, chat.* at :1298, WS parity at :1556 — keys on the method name server-side, never on whether a token was presented. A call_open call naming a gated method gets -32030. No previously-gated read became reachable. grep -rn call_open crates/ returns exactly three hits: the definition, the import, and one call site with a literal method name (network_info.rs:29). Nothing silently adopted it.
  • The claim that the node already served it holds on the loopback path specifically, not merely on the peer channel: seams/dig_rpc/dispatch.rs:280 returns node.network_info() through the same handle_rpc_as fall-through the HTTP handler reaches at server.rs:1335, after all four gates decline it.
  • The renderer's fixtures are faithful, not self-consistent inventions. lib.rs:4082-4095 emits peer_id, network_id, genesis, listen_addr, candidate_addresses, reachability, relay — all seven read by format_network_info. This was the likeliest false green in the batch and it is not one.
  • #304 distinguishes all three states by cause, and the tests are a real pair: peers.rs:713 and the pre-existing version-gap test vary only the connected count against the same shape, so an unconditional reword fails one of them.
  • #305 is scoped correctly in the package. build-deb.sh:145 guards on [ -n "${2:-}" ], which dpkg supplies only on upgrade, so a first install cannot fight the installer. try-restart over restart is the right primitive and deb-contents.test.sh:180 asserts the semantics rather than the spelling. The first-install control at :189 is a genuine control.
  • I agree with the #403 decision. requires_master_token is the tier the server actually enforces (server.rs:1218-1230) and it does cover pairing administration and chiaPeers.add/.remove, so the token really is the master capability. A dig group at 0640 is a permanent, unrevocable, transitive grant of chain authority over the wallet replica, bought for one app's convenience and outliving it. Pairing is the right primitive: scoped, per-client, revocable. Making the platform a real argument rather than a cfg! branch is right too, and the Windows clause is a truthful control rather than a restatement.

Revert-proof, verified independently. Baseline: cargo test -p dig-node-service --lib = 657 ran, 657 passed, 0 filtered out (count checked, not exit status). With two mutations applied by file copy — if connected == 0 forced to if false, and the ConnectionRefused => ExitCode::NodeUnreachable arm deleted — 657 ran, 654 passed, 3 failed, and each failed for its own reason: an_unreachable_node_is_distinguished_from_a_measured_io_failure (left: IoError, right: NodeUnreachable), plus both peers tests printing the restored version-gap sentence. Files restored; git status --porcelain empty.

The findings, ranked

  1. control.rs:182 — the new Unix remedy names a command that does not do what the sentence says. sudo dign pair lists; approving is dign pair approve <id>. An operator following it is stuck at the same wall, one sentence further along. The test at :213 asserts only contains("dign pair"), so it cannot tell the right string from this one.
  2. cli.rs:47 — exit code 7 is already taken by diga, the sibling CLI whose own doc-comment says it shares this numbering. dig-app .../gateway/outcome.rs:28 holds 7 = NOT_CONNECTED (and 8-11). One number, two symbols, under a SPEC clause forbidding renumbering, about to be frozen by a release. The 6-matchers the body worried about are genuinely clean — I searched this repo, dig-installer and dig-app and found none.
  3. SPEC.md:2626 — the mapping sentence is false in the commit that added the row above it. "anything else -> 6" while ConnectionRefused -> 7. Also silently wrong about InvalidInput -> 2 already; sweep both.
  4. SPEC.md:2404network-info reaches no doc at all, including the normative subcommand list, and §8.6 currently asserts the opposite token property for the parity set. No test can see this drift.
  5. logging.rs:92 — the new behaviour has zero tests (all five in the file predate this PR and exercise health(...)), and its message ships with two runs of ~22 literal spaces from a lost continuation.
  6. deb-contents.test.sh — "gated in CI" is not true. It does run for real on ubuntu-latest, but Release-script tests is not a required context, so #305's only executing proof cannot block a merge. Plus one evidence claim to correct: try-restart under the #317 marker is reasoned, not tested — there is no marker-plus-upgrade fixture.

Non-gating, and I am not opening threads for these

  • The Closes keywords are inside inline code spans (`Closes #303`). GitHub does not resolve issue references inside code spans, so on merge these four very likely do not autoclose — the exact manual-close gap the two-queue split exists to remove. Unbacktick them. The intent is verified correct: no closing keyword anywhere in the body or in any of the six commit messages touches #403 or #392, so neither can close by accident. The Closes set is exactly #303, #304, #305, #407.
  • The 0.189.0 -> 0.190.0 minor bump is right for a new CLI capability.

What I could not reach

  • Windows service upgrade (#305's third platform) — not checked, as the lane says. It runs through dig-updater's install path and nothing in this diff touches it.
  • #392's primary finding is genuinely unfixed and correctly left open; proving it needs a real out-of-service run under DIG_WALLET_PORT/LOCALAPPDATA.
  • A live dign network-info against a running node. I verified the dispatch path and the field names statically; nobody has yet watched the verb print on a real machine, which is the §2.6 bar. Worth one run before release rather than before merge.

Comment thread crates/dig-node-service/src/control.rs Outdated
Comment thread crates/dig-node-service/src/cli.rs
Comment thread SPEC.md Outdated
Comment thread SPEC.md Outdated
Comment thread crates/dig-node-service/src/logging.rs Outdated
Comment thread scripts/tests/deb-contents.test.sh
…degrade

Six gate findings on #458, each a claim the branch made that its own tests could
not see.

The Unix token remedy named `sudo dign pair` as the approving verb. It is not:
`entrypoint.rs` maps a bare `pair` to `PairAction::List`, so the sentence added to
break a reinstall loop was itself a dead end one step further along. Its test
asserted `contains("dign pair")`, which the wrong string satisfies as its own
substring. The remedy now names `sudo dign pair approve <pairing_id>`, the test
asserts that command in full plus a one-sided row against the false attribution,
and a new parser test pins the bare/approve mapping so the phrase is measured
against the parser rather than against itself.

`NODE_UNREACHABLE` took exit 7 by reading this CLI's own table, where 7 was
genuinely next. dig-app's `diga` holds 7 = NOT_CONNECTED and says in its own doc
that it shares dig-node's numbering; 8-11 are taken too. This is the reasoning
that already cost a yank in the JSON-RPC error space. Renumbered to 12, the first
number free ecosystem-wide, with the measured occupancy table written into SPEC.md
and a test that fails if any shared number carries different meanings.

SPEC's I/O-error mapping claimed "anything else -> 6" while omitting InvalidInput
-> 2 and the new arm; both are now listed. `network-info` reached no doc at all
and is token-free by design, contradicting 8.6's rule, so it gets 8.8 stating that
exception and a README section.

The logging change had no tests and shipped two runs of ~22 literal spaces from a
lost string continuation. The decision is extracted as a pure function and the
text as a constant; four tests cover announce, stay-silent, the ambiguity the
message must resolve, and the whitespace. The whitespace row immediately earned
itself: the first repair used a backslash continuation and `cargo fmt` rejoined it
and materialised the indentation straight back into the string. `concat!` has no
whitespace a formatter can reinterpret.

The #317 marker surviving upgrades was reasoned in a comment and never exercised.
The stub-systemctl harness now runs a marked upgrade and asserts both directions:
nothing unconditional starts, and try-restart still cycles a running unit.

Co-Authored-By: Claude <noreply@anthropic.com>
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Correction to the "still unverified" list

The body says the new marked-upgrade rows in deb-contents.test.sh "have not executed anywhere yet — CI is their first run." That run has now happened: Release-script tests passed on a4e4362 (job 99542144279). The script fails loudly under CI when dpkg-deb is absent rather than skipping, so a pass means the rows genuinely executed.

Stated precisely, because the two are not the same claim: the check is green; I have not read its log. gh run view --log still refuses while the parent run is in progress, so "the three new rows each printed ok:" is an inference from the pass, not a read of the output. Worth one look once the run completes.

The exit-code occupancy was corroborated by a second, independent sweep

A slower full-tree grep I had abandoned in favour of ripgrep completed afterwards and was checked against the map rather than discarded. It surfaced no occupant the map is missing, and it did surface two dig-node integration test files my first filter had not covered — tests/cli.rs and tests/cli_dign_alias.rs. Both assert only exit 1 (NOT_SERVING); neither hardcodes 7, so the renumber does not break them.

Two other hits are different namespaces and are not rival exit-code numberings:

  • dig-chrome-extension's WALLET_WS_ERR.NOT_CONNECTED = -33001 is a JSON-RPC error code.
  • dig-app's cli_session/engine_proxy.rs and client.rs only use the ErrorCode symbols defined in gateway/outcome.rs; they define no numbers of their own.

Noting the -33001 because it means the ecosystem has at least three separate numbering spaces (process exit codes, dig-node's JSON-RPC errors, the extension's wallet-WS errors) and only one of them now has a written occupancy map.

@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

Verified fixed at head a4e436292d96dcafa983c62a187215919bd81844 — checked in the tree, not from the fix report:

  1. dign pair approve <pairing_id> now named, and the assertion is the full command rather than contains("dign pair") (which the wrong string satisfied as its own substring). A parser-driven test pins the verb mapping.
  2. Exit 7 is gone; NODE_UNREACHABLE is 12. No occurrence of 7 remains as an exit code, and no_exit_code_collides_with_the_dig_app_gateway_numbering now fails if any number carries two meanings across dign and diga. The occupancy map is written into SPEC.md §8.4 — the first of the ecosystem's three number spaces to have one.
  3. All five I/O arms listed in match order, including the previously missing InvalidInput → 2.
  4. network-info documented (§8.8, cross-referenced from §8.6 and listed in §8.1).
  5. Logging decision extracted and tested; concat! replaces the backslash continuations — which mattered, because cargo fmt silently reintroduced the mangled string once between writing and running.
  6. Release-script tests is now a required context (6, was 5), so dpkg upgrade does not restart the service — users keep running the old binary silently #305's only executing proof can block a merge.

Thanks — finding 2 was the valuable one and I would have merged the collision. It is the third instance of "free in my own table" in this ecosystem, so it is now filed as its own problem with an owner and a guard: https://github.com/DIG-Network/dig_ecosystem/issues/3189.

@MichaelTaylor3d
MichaelTaylor3d marked this pull request as ready for review August 31, 2026 17:47
@MichaelTaylor3d
MichaelTaylor3d merged commit d41cf6f into main Aug 31, 2026
21 checks passed
@MichaelTaylor3d
MichaelTaylor3d deleted the loop/batch-cli branch August 31, 2026 17:47
MichaelTaylor3d added a commit that referenced this pull request Aug 31, 2026
Six lanes each computed "main is at 0.189.0, so I take 0.190.0" while open
concurrently. #458 merged first and took that number, so every sibling's bump
became a no-op against the new main -- the rebase was clean, no conflict, no
`dropping` line, and the commit log still reads as a bump.

Only the file on disk showed it. Member crates are unaffected: #458 moved the
workspace version alone, so dig-node-core 0.65.0 and dig-wallet 0.44.0 are still
ahead of main.
MichaelTaylor3d added a commit that referenced this pull request Aug 31, 2026
Clean rebase, zero conflicts, no `dropping` line -- and the bump commit still
reads correctly in the log while being a no-op against the new main. Only the file
on disk shows it. 0.191.0 and 0.192.0 are claimed by sibling lanes still in
flight.
MichaelTaylor3d added a commit that referenced this pull request Aug 31, 2026
Clean rebase, no conflict, no `dropping` -- the bump commit survives and simply
stops meaning anything against the new main. Read from the file, not the log.
0.191-0.193 are claimed by siblings still in flight.
MichaelTaylor3d added a commit that referenced this pull request Aug 31, 2026
…ing input surfaces (#453)

* chore(secpeer): open lane for the peer/network-facing security batch

Batch: #349 #352 #348 #285 #282 -- five defects where a stranger's input
decides something, each fixed by correcting the failure direction.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(relay,config): fail closed on an unreadable relay endpoint and share one off-token

#285: relay endpoint parsing accepted any scheme and turned an unparsable port
into None, which relay_socket_addr resolved with .unwrap_or(443) -- so one
malformed string made the node silently dial 443 at a host that may not be the
one the operator wrote. Replaced with a fail-closed parse transcribed from
dig_nat::relay::parse_relay_endpoint (the designated survivor, still private in
the published 0.21.0), so adopting the export later is a deletion.

#282/#352: the three network-reaching isolation knobs each carried a private
off-token predicate reading a different vocabulary, so DIG_PEER_NETWORK=OFF left
the peer network running while DIG_RELAY_URL=OFF disabled the relay. One shared
is_off_token now serves all three, and DIG_RELAY_URL= means no relay rather than
the compiled-in public one. Bootstrap resolution announces which branch it took.

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(peers,reservation): withhold non-destination addresses; hold a possibly-in-flight bundle

#349: dig.getPeers served the wildcard address to REMOTE peers as a dial
candidate, because is_usable_contact had been adopted call site by call site and
pool_peers was the fifth site nobody looked at. The check now lives in a
ContactAddr type whose construction IS the question and whose address_json is the
only renderer of the wire entry. The ADDRESS is withheld, never the peer: the row
survives with an empty addresses array, the shape both consumers already handle.

#348: reservation was gated on an untrusted 'accepted' alone, so an unexplained
denial or a post-transmit transport failure freed coins of a bundle that may be in
flight. Anything short of a STATED mempool rejection now holds to the TTL.

Co-Authored-By: Claude <noreply@anthropic.com>

* docs(spec): state the getPeers address rule and the possibly-in-flight reservation rule

Both behaviours are now normative rather than incidental: a peer row with no
dialable destination keeps the row and empties the addresses ARRAY (#349), and a
push reserves its inputs unless the mempool DEFINITIVELY refused it (#348).

Co-Authored-By: Claude <noreply@anthropic.com>

* chore(release): 0.190.0 -- minor, two accepted-input sets narrow observably

Root 0.189.0 -> 0.190.0, dig-node-core 0.64.0 -> 0.65.0, dig-wallet 0.43.0 ->
0.44.0 (both touched). Minor rather than patch because two behaviours narrow in
ways a consumer can observe: parse_relay_host is pub and now refuses a
scheme-less endpoint it used to accept, and DIG_RELAY_URL= changes meaning from
'the compiled-in public relay' to 'no relay'. Not major -- a 0.x line, and
nothing left the public surface but the private relay_port.

Cargo.lock carries the three version lines and no dependency churn.

Co-Authored-By: Claude <noreply@anthropic.com>

* style(peers): rustfmt the ContactAddr type-boundary comment

Pure reflow of lines the type-boundary change made over-width. No behaviour
change; `git diff -w` against the prior head is empty.

Refs #349

* fix(wallet): a bare verdict is not a stated rejection, so the #348 hold can fire

`is_definitive_rejection` (`sage/rpc.rs`) frees a bundle's inputs back into
selection only for a refusal the mempool STATED, and holds them to the TTL
otherwise -- because a peer that relayed the bundle and then answered with a bare
verdict may still have put it in flight.

`ChainTransport::push` manufactured a reason from `status.status`, so `rejection`
was `Some(..)` on EVERY non-admitted answer, the guard was true every time, and
the hold could never fire. The fix read as shipped and was vacuous; the
double-select window #348 exists to close was untouched.

SPEC.md §18.7 was already correct -- "a bare denial carrying no reason ... MUST be
treated as POSSIBLY IN FLIGHT" -- so this is the code being brought to the spec,
not a spec change.

The decision is extracted as `stated_rejection` and asserted directly, because an
end-to-end assertion on a held reservation passes for many reasons and only one of
them is this mapping being right. Revert-proved: restoring the manufactured reason
fails it on `left: Some("PENDING"), right: None`.

Also corrects two comments that claimed more than the code does: the off-token is
shared by the three ISOLATION knobs, not by every network-reaching `DIG_*` knob
(four others read a narrower vocabulary -- #459); and `ContactAddr` closes the
`dig.getPeers` emitter, not the class, since `provider_json` still relays
remote-supplied candidates unchecked.

Refs #348 #349 #459

Co-Authored-By: Claude <noreply@anthropic.com>

* fix(wallet): carry the mempool's verdict label beside the stated reason

Narrowing `rejection` to a STATED reason is what lets #348's hold fire, but taken
alone it left an operator debugging a bare `PENDING` with `accepted:false`,
`transaction_id:null`, `rejection:null` and no label at all. The response did not
lie -- "not accepted, no reason given" is true -- but it said strictly less than
before, which is its own regression on a money path.

`PushOutcome` gains `verdict`, always present. The two fields answer different
questions and only one of them may drive the hold, which is why they are separate
rather than one string: folding the label back into `rejection` would re-break the
guard, and dropping it re-blinds the operator.

`control.wallet.broadcast` surfaces it, and the doc above it no longer claims a
refusal always answers `{accepted:false, rejection}` -- a shape that stopped
always holding when `rejection` narrowed.

Caught by the security re-gate as a LOW observability finding on my own fix.

Refs #348

Co-Authored-By: Claude <noreply@anthropic.com>

* chore(release): 0.191.0 -- rebased past #458, which took 0.190.0

Six lanes each computed "main is at 0.189.0, so I take 0.190.0" while open
concurrently. #458 merged first and took that number, so every sibling's bump
became a no-op against the new main -- the rebase was clean, no conflict, no
`dropping` line, and the commit log still reads as a bump.

Only the file on disk showed it. Member crates are unaffected: #458 moved the
workspace version alone, so dig-node-core 0.65.0 and dig-wallet 0.44.0 are still
ahead of main.

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment