Skip to content

feat(control): PeerSoftware type and an always-present software field on peerStatus - #4

Merged
MichaelTaylor3d merged 5 commits into
mainfrom
feat/2215-peer-software
Aug 6, 2026
Merged

feat(control): PeerSoftware type and an always-present software field on peerStatus#4
MichaelTaylor3d merged 5 commits into
mainfrom
feat/2215-peer-software

Conversation

@MichaelTaylor3d

@MichaelTaylor3d MichaelTaylor3d commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Define PeerSoftware — the one place a gossip handshake's software_version string becomes meaning.

Refs DIG-Network/dig_ecosystem#2215.

Change

#[serde(tag = "kind", rename_all = "snake_case")]
pub enum PeerSoftware {
    Unknown,
    Reported { product: String, version: semver::Version, raw: String },
}
  • No Ord/PartialOrd, no Default. Unknown has no position on a version line, and most peers are Unknown today — an ordering would silently become a verdict about the live network. Comparison is reachable only after destructuring Reported.
  • "", "0.0.0", product/0.0.0, and anything unparseable all map to Unknown, as a named and tested mapping.
  • control.peerStatus gains an always-present software member per peer entry. No new control methodcontrol.status.version already covers self, and the snapshot covers the point lookup and the census together.
  • SPEC §4.1 states the mapping table, the always-present rule, the no-Ord/no-Default rule, and the fingerprinting trade-off.

Why the sentinel mapping is the load-bearing part

Every dig-node running today advertises the literal "0.0.0" — three of dig-gossip's four handshake send sites hardcoded it. A parser mapping only "" to Unknown would read the entire live fleet as software version 0.0.0, making the "treated as ancient" failure guaranteed rather than hypothetical.

Scope note — peerStatus stays proxied

SPEC §4.1 says proxied results including control.peerStatus "carry the underlying source's shape verbatim ... consumers MUST NOT freeze a struct over them." So this PR pins the ONE member the contract owns (software) in situ, inside a representative connected array, rather than freezing a PeerStatusPeer struct. The always-present rule is normative prose in SPEC plus a KAT assertion, not a struct field — deliberately, to avoid contradicting the existing rule.

Blast radius checked

gitnexus is disabled in this loop; radius established by grep + direct read. Purely additive: a new public enum, a new pub use, one new dependency (semver, serde feature), one doc-string change on ControlMethod::PeerStatus. No existing type, method name, params struct, result struct, or error code is touched. Every pre-existing test still passes unmodified (46 total, 13 pre-existing in results, the KAT suite intact).

SemVer: 0.3.0 → 0.4.0 (minor, additive).

Evidence

12 new tests. Mutation battery over PeerSoftware::parse, with non-compiling mutants classified separately from survivors — an earlier run of this battery misclassified three KILLS as non-compiling because "error: test failed" matched the compile-error pattern, so the classifier now keys on could not compile specifically, and every mutated line is read back before the run.

Mutation Result
drop the product/0.0.0 sentinel check KILLED
drop the empty-product check KILLED
drop trim() KILLED
rsplit_oncesplit_once KILLED
sentinel constant "0.0.0""0.0.1" KILLED
unparseable version accepted as 0.0.0 instead of Unknown KILLED
derive Ord on PeerSoftware KILLED (by the trait-absence probe)
raw re-renders the parsed parts instead of recording the advertisement SURVIVED — see below

Two findings the battery produced, both fixed or disclosed rather than papered over:

  1. A genuinely redundant clause. The original parse had if raw.is_empty() || raw == LEGACY_UNVERSIONED_SENTINEL at the top, and dropping the sentinel half survived — because bare "0.0.0" contains no / and already fell through the no-separator branch to Unknown. The clause was unkillable dead code. parse was restructured so the sentinel is checked exactly once, in the position where it IS load-bearing (product/0.0.0); the no-separator branch now carries a comment naming the bare-sentinel case it absorbs. The mapping is still explicit and tested.

  2. trim() was uncovered. Dropping it survived, because the only whitespace fixture was " " — which maps to Unknown either way. Added surrounding_whitespace_is_trimmed_before_parsing with " dig-node/1.2.3\t", which distinguishes the two. CON-008 strips Cc/Cf but not spaces, so a padded advertisement is a real wire input.

The surviving mutant is disclosed, not hidden. raw re-rendering format!("{product}/{version}") cannot be killed, because the accepted grammar is lossless: semver::Version re-renders every string it accepts byte-identically (probed across canonical, pre-release, and build-metadata forms). No fixture can distinguish the field from that expression today. raw is retained deliberately and the field's doc-comment now states this invariant, so the next reader does not mistake it for tested state — it becomes load-bearing the moment the grammar accepts anything non-canonical. If the gate would rather drop raw than carry a documented untestable field, say so and I will.

The trait-absence probes each carry a control on a type that DOES implement the trait (u32: Ord, String: Default); without the control, a probe broken to always answer false would pass while proving nothing.

The peerStatus KAT vector carries one REPORTED and one UNKNOWN peer together — a vector with only a reported peer would pass against an implementation that omits software whenever it is Unknown, which is exactly the bug the always-present rule exists to prevent.

Gates: 46 tests green, cargo clippy --all-targets -- -D warnings clean, cargo fmt --check clean, cargo doc adds no new warnings, git diff --shortstat identical to --ignore-cr-at-eol.

Also in this PR — SoftwareVersionDetail, the coarsening dial

The brief asked for the full | minor | off knob to be built now, because its off path IS the back-compat test. The config knob belongs in dig-node (the design says so), but the rendering belongs here beside the parsing: a node that hand-rolled its own product/version string would re-implement half the format contract and drift from it — the failure #2214 exists to repair.

pub enum SoftwareVersionDetail { Full, Minor, Off }   // wire: "full" | "minor" | "off", default Full
pub fn render(self, product: &str, version: &semver::Version) -> String;

Building it surfaced a defect in the design's own spelling. The design specified minor -> dig-node/0.99. A bare two-part version is not valid semver, so PeerSoftware::parse("dig-node/0.99") returns Unknown — the coarse setting would collapse to "tell them nothing", which is what off is for, and would silently become a second confusing spelling of it. minor therefore renders MAJOR.MINOR.0 (dig-node/0.99.0), readable while hiding the patch level. A test pins exactly that distinction.

minor also strips pre-release and build metadata: a nightly identifier (-nightly.20260805+sha.abc123) is more precisely identifying than the patch number beside it, so retaining it would coarsen nothing for exactly the builds that most want it.

Mutation battery on the renderer — 5 mutants, 5 killed:

Mutation Result
minor renders a bare MAJOR.MINOR KILLED
minor keeps the pre-release identifier KILLED
minor is a no-op (renders full) KILLED
off leaks the product name KILLED
default flipped from Full to Off KILLED

The round-trip fixture uses 0.99.1 — non-zero minor AND non-zero patch — because that is the only shape where Full and Minor differ; a 1.0.0 fixture would let a renderer that ignores the mode entirely pass.

52 tests green.

dig-node adoption (separate unit of work)

Depend on 0.4.0. Add the advertise_software_version config field typed as SoftwareVersionDetail and advertise detail.render("dig-node", &version) — do NOT hand-roll the string. Call PeerSoftware::parse(..) on each string from dig-gossip's connected_pool_peers_with_software(), and emit it as the software member of every control.peerStatus entry — including for stub/nat peers, which report "" and therefore Unknown.


Gate round 1 — addressed (1a04458)

All three findings here were one shape, as the gate said: a rule implemented over a literal whose own rationale is stated over a class. Holding that lens found a fourth instance in dig-gossip, fixed in its PR.

1. The probe guarded Ord; the hazard is PartialOrd

Ord: PartialOrd, so a one-word #[derive(PartialOrd)] satisfied an Ord-only probe while Unknown < Reported(..) still compiled and evaluated true — sorting Unknown below every real version, which is the verdict-about-the-live-network the contract forbids. SPEC §4.1 names all three traits; two were pinned.

A PartialOrdProbe now pins the weakest of them, which subsumes Ord. It carries two controls: f64 (PartialOrd but NOT Ord — without this the probe could not prove it sees the gap between the two traits) and u32 (fully ordered).

Mutation: #[derive(..., PartialOrd, ...)] on PeerSoftwareKILLED.

2. The sentinel was matched as a string, not as version zero

dig-node/0.0.0+build, dig-node/0.0.0-rc.1 and x/0.0.0-0 all yielded Reported { version: 0.0.0 } — the reading all three prose statements forbid.

The comparison moved after the parse, onto the major/minor/patch triple, ignoring pre-release and build metadata. The LEGACY_UNVERSIONED_SENTINEL string constant is replaced by an is_version_zero(&Version) predicate. All three prose statements plus the SPEC table were swept to state the class.

Two tests, deliberately paired: version_zero_is_unknown_however_it_is_decorated (5 decorated forms) and a_nonzero_version_near_zero_is_still_reported (0.0.1, 0.1.0, 0.0.1-rc.1) — without the second, a parser mapping everything below 0.1.0 to Unknown would pass the first.

Mutations: revert to the string compare → KILLED; is_version_zero ignoring patch → KILLED; is_version_zero also requiring an empty pre-release (re-introducing the decorated leak) → KILLED.

3. A stated invariant that was false

render's doc promised every result is empty or reads back as Reported. Minor.render("p", 0.0.7)"p/0.0.0"Unknown — the same Minor-collapses-into-Off defect as the two-part spelling, through the other door, and the 1.4.7 fixture could not see it.

Minor of a 0.0.x build now renders the empty string, deliberately: hiding the patch of a 0.0.x version leaves version zero, which the wire reserves as the "unknown" sentinel, and there is no coarser representable value — so it advertises nothing rather than advertising the sentinel as if it were a report. This differs from the 0.99 case, where a representable coarse value existed and the wrong spelling was chosen.

The fixture is now the class the invariant is stated over: every_rendering_is_empty_or_readable sweeps all three modes across ten versions including 0.0.1, 0.0.7, 0.0.99 and 0.0.1-rc.1, asserting the invariant directly rather than assertions about particular strings.

Mutation: guard removed (read back as // MUTANT: guard removed before running) → KILLED, by both the invariant test and the targeted one.

5. raw — kept, with the tripwire

The gate's reasoning is better than the question I asked: unprovability is a property of the current grammar, not of the field, and re-adding it later breaks a published contract. raw_is_still_reconstructible_from_the_parsed_parts now asserts raw == format!("{product}/{version}") across six accepted forms, documented as "when this test FAILS, raw has become load-bearing — do not fix it by deleting the field."

Noted, not actioned

The always-present KAT asserting against a json! literal it wrote itself is acknowledged as non-gating; the real guard belongs in the dig-node adoption PR, and I have said so there.

Re-gate scope

Changed legs: PeerSoftware::parse, is_version_zero, SoftwareVersionDetail::render, the trait probes, and the sentinel prose in doc-comments + SPEC §4.1. The JSON wire shapes, the KATs, and SoftwareVersionDetail's tokens/default are unchanged. 58 tests green, clippy -D warnings clean, cargo doc adds no new warnings, git diff --shortstat identical to --ignore-cr-at-eol.

…andshake

PeerSoftware maps a peer's advertised software_version string to a build, once, at the
control boundary. It implements neither Ord nor Default: Unknown has no position on a
version line, and every peer built before this contract advertises the legacy "0.0.0"
sentinel, so an ordering would silently rank most of the live network as ancient.

control.peerStatus gains an always-present `software` member on each peer entry. No new
control method: control.status.version already reports this node's own build, and the
snapshot covers both the point lookup and the census.

Refs dig_ecosystem#2215
…t a node advertises

Rendering lives beside PeerSoftware's parsing because they are two halves of one
format; a node that hand-rolled its own product/version string would re-implement half
the contract and drift from it.

Minor renders MAJOR.MINOR.0, not a bare MAJOR.MINOR: two-part versions are not valid
semver, so the coarse setting would be read as Unknown and become a second spelling of
Off. It also strips pre-release and build metadata, since a nightly identifier is more
precisely identifying than the patch number beside it.

Refs dig_ecosystem#2215

@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.

VERDICT: CHANGES-REQUIRED

(Recorded as a comment review: the review token shares the PR author identity, so GitHub rejects self-request-changes with 422. The verdict below is the gate result.)

Reviewed at head 858e4cc. This is careful work and the disclosure discipline is exactly right. I re-ran your corrected mutation battery myself in a detached worktree rather than taking it on report, and it reproduces:

Mutant My result
drop the product/0.0.0 sentinel half of the clause KILLED (unknown_covers_...)
drop the empty-product half KILLED (unknown_covers_...)
drop trim() KILLED (surrounding_whitespace_...)
rsplit_once -> split_once KILLED (product_is_split_at_the_last_separator)
sentinel "0.0.0" -> "0.0.1" KILLED (unknown_covers_...)
unparseable accepted as 0.0.0 KILLED (unknown_covers_...)
raw re-rendered from parsed parts SURVIVED — confirmed

A note on method, since you flagged the misclassification: my own first attempt at the sentinel mutant silently never applied (a sed s-expression broke on the ||), and the tree reported a clean 52-passed that I would have read as a survivor. I caught it only because I echoed the mutated line back. Your read-back-every-mutated-line correction is the right one and it is what saved this run too.

I also verified independently:

  • The Unknown mapping is complete for the live fleet. I ran the parser over 16 inputs: "", "0.0.0", " ", " 0.0.0 ", product, product/, /1.2.3, 1.2.3, product/not-a-version, product/v1.2.3, product/1.2, product/0.0.0 — all Unknown. The highest-consequence behaviour in the diff is correct.
  • The trait-absence probe is genuinely falsifiable. Control u32 -> true, PeerSoftware -> false, and a copy with Ord derived -> true. The probe is real, not a probe that always answers false. (But see N1 — it guards the wrong trait.)
  • raw really is unkillable. I confirmed byte-identical re-rendering across canonical, pre-release, build-metadata, multi-slash-product, and numeric-pre-release forms.
  • No dep edge to dig-gossip, no git deps, semver is the only addition, --shortstat matches --ignore-cr-at-eol, and there is no qualified Closes on the epic. All correct.

Three gating findings, all in the same shape: a rule stated over a literal where its own rationale is stated over a class. Inline.

Ruling on the raw question you asked the gate to decide

Keep it. Do not drop the field. My reasoning, on the merits rather than deferred:

  1. The unprovability is a property of the current grammar, not of the field. raw is untestable because the accepted grammar happens to be lossless today. Deleting the field to make the test suite tidy fixes the symptom by removing the sensor.
  2. The grammar is the part most likely to move. A v prefix, a two-part version, a vendor suffix, a date-stamped build — all plausible loosenings, and each makes raw load-bearing the moment it lands. Re-adding it then is a breaking change to a published wire contract (it is in your SPEC §4.1 JSON and in the KAT vector); carrying it now costs one string.
  3. The right discipline for an unprovable-today field is not deletion, it is a tripwire on the premise. You cannot test raw, but you can test the reason you cannot: add a test asserting that for every input the parser accepts, raw == format!("{product}/{version}"), with a doc-comment saying when this test fails, raw has become load-bearing and needs a fixture of its own. That converts "a documented untestable field" into "a tested invariant with an alarm on it" — and it is the only form of this that survives a future reader who does not read your doc-comment.

That tripwire is a recommendation, not a gate. The field as-merged is defensible; the three findings below are what block.

@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.

The three gating findings, ranked.

Comment thread src/results.rs
Comment thread src/results.rs Outdated
Comment thread src/results.rs Outdated
@MichaelTaylor3d

Copy link
Copy Markdown
Contributor Author

NON-GATING notes (posted as PR comments, not review threads, so they cannot block merge)

1. src/kats.rs — the always-present KAT asserts against a vector this test wrote itself.

peer_status_software_member_golden_vector builds snapshot as a json! literal containing software on both entries, then loops over that literal asserting software is present. Nothing but an edit to the literal can make that arm fail — no implementation is on the other side of it.

Your scope note explains why the envelope is not frozen (SPEC §4.1 forbids it for a proxied result), and I agree with that call. The parts of this KAT that carry real weight — the decode/re-encode byte-stability, and assert_ne!(reported, unknown) — are genuine. It is specifically the always-present loop that is a claim rather than evidence, because the rule it guards is a rule about dig-node's serializer, which does not exist yet.

No change needed here. Flagging it so the dig-node adoption PR carries the real guard: a test over dig-node's actual peerStatus construction asserting a stub/nat peer (which reports "" -> Unknown) still emits a software member. That is where the always-present rule can actually be violated, and where omitting-when-Unknown is the natural bug.

2. The raw tripwire. Per the ruling in my summary review — keep the field, and add a test pinning the premise rather than the field:

/// `raw` is currently reconstructible: the accepted grammar is lossless. This test pins that
/// premise, NOT the field. When it fails, the grammar has loosened and `raw` has become
/// load-bearing state that needs a fixture of its own.

asserting raw == format!("{product}/{version}") across canonical, pre-release, build-metadata and multi-slash-product inputs. I verified all four of those re-render byte-identically today, so the test will be green on merge and will go red exactly when the field starts mattering.

Neither of these blocks. N1/N2/N3 in the inline threads are the gate.

…classes

Three gate findings, all one shape — a rule implemented over a literal whose rationale
is stated over a class.

The version-zero sentinel was matched as the string "0.0.0", so 0.0.0+build, 0.0.0-rc.1
and 0.0.0-0 were reported as real builds at version zero. It is now matched on the
parsed major/minor/patch triple, after the parse, ignoring pre-release and build
metadata.

The trait-absence probe guarded Ord alone, but Ord: PartialOrd — a one-word
derive(PartialOrd) satisfied the probe while Unknown < Reported(..) still compiled and
evaluated. A PartialOrd probe now subsumes it, with controls on a PartialOrd-but-not-Ord
type and on a fully ordered one.

render's stated invariant was false: Minor of a 0.0.x build coarsened to version zero,
which reads as Unknown. Minor now renders the empty string there, because hiding the
patch of a 0.0.x build leaves no coarser representable value and the sentinel must never
be advertised. The invariant is now tested over the class it is stated over.

Also adds the raw tripwire: raw is asserted equal to the parsed parts across the
accepted grammar, so the test goes red exactly when raw becomes load-bearing.

Refs dig_ecosystem#2215

@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.

PASS — round 2, reviewed at head 1a04458665f91a9c27c7dc5bc87328dfc863800d

All three round-1 gating threads are addressed and resolved, each with a mutant I applied and ran myself. Given the lane's disclosure that two of its own mutants this round were malformed, I treated its mutation evidence as a claim and re-derived it rather than accepting it. Every mutant below was confirmed to have actually landed (git diff --numstat) before the run, and the tree was restored between each.

# Mutant (true revert of the fix) Result
1 #[derive(PartialOrd)] on PeerSoftware killed by peer_software_is_not_partially_ordered_either; ..._is_not_ordered stays green
1b probe bound PartialOrd -> Ord (src/results.rs:736) killed only by the f64 assertion at :900
2 is_version_zero(&version) -> version.to_string() == "0.0.0" killed by version_zero_is_unknown_however_it_is_decorated
2b is_version_zero -> major == 0 && minor == 0 (everything below 0.1.0 is Unknown) decorated-forms test passes; killed by a_nonzero_version_near_zero_is_still_reported
4 delete the version-zero guard in render (:85-87) killed by every_rendering_is_empty_or_readable and minor_of_a_zero_zero_build_...

Baseline: 58 lib tests green.

Two results are worth calling out because they are the ones that could have been asserted rather than shown.

The f64 control is load-bearing (1b). Under a probe silently drifted to guarding Ord, the u32 control still passes and only f64 fails. It is the single assertion in the suite pinning the probe to the weaker trait. Claim confirmed.

The lane's finding-2 rationale is exactly right (2b). With the parser weakened to map everything below 0.1.0 to Unknown, version_zero_is_unknown_however_it_is_decorated passes on its own. a_nonzero_version_near_zero_is_still_reported is what discriminates — it names the property rather than restating the outcome. It also took down every_rendering_is_empty_or_readable and raw_is_still_reconstructible_from_the_parsed_parts, so the widened 10-version fixture is not decoration.

Finding 4 — the design call, judged on its merits

Rendering the empty string for Minor of a 0.0.x build is right. dig-node/0.0.0 reads back as Unknown regardless, so it buys no readability while advertising the sentinel as a report; falling back to Full leaks what the operator asked to hide; nothing coarser is representable. The failure direction is "revealed less than asked", the safe direction for a privacy dial, and it carries no denial consequence — dig-gossip#56 proves an empty advertisement connects normally. The affected class is only a product before its first minor. Documented on the enum, on render, and as its own row in SPEC.md §4.1.

Coherence

SPEC.md §4.1 agrees with the code on all four points I checked: the mode table including the 0.0.x row, the MAJOR.MINOR.0 MUST, the every-rendering invariant, and the no-Ord/PartialOrd/Default rule. I swept for surviving phrasing of the superseded rules rather than spot-checking the touched lines and found none. method.rs:290's PeerStatus description and the lib.rs module doc both match.

The peer_status_software_member_golden_vector KAT is right to pin the member in situ rather than freezing the proxied envelope, and its two-entry vector (one reported, one unknown) is what stops an implementation that omits software when Unknown from passing.

Non-gating note (resolved by me, not a blocker)

The SPEC.md mode table is hand-maintained beside the enum it describes — the shape that normally drifts unwatched. Low risk here: render's match self is exhaustive, so a new variant cannot be added without a compile error, and the enum is not #[non_exhaustive], so adding one is a deliberate major event. Worth a mechanical row-per-variant guard if the enum ever grows; not worth blocking a chain-blocking publish over a four-row table that cannot silently gain a row.

dig-constants (hard rule, asked on every PR)

Neither question is met. Belongs there? No — PRODUCT_VERSION_SEPARATOR, the version-zero sentinel and the full/minor/off tokens are a format grammar whose render and parse halves are deliberately co-located in this one crate, which prevents drift more strongly than a shared constant (a shared separator would not stop a second hand-rolled renderer). Should be using it? No — dig-constants publishes chain/network/crypto values only and nothing about software advertisement; "dig-node" appears here solely in doc and test examples.

@MichaelTaylor3d
MichaelTaylor3d marked this pull request as ready for review August 6, 2026 03:21
@MichaelTaylor3d
MichaelTaylor3d merged commit a97889e into main Aug 6, 2026
9 checks passed
@MichaelTaylor3d
MichaelTaylor3d deleted the feat/2215-peer-software branch August 6, 2026 03:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant