Skip to content

fix(cli): #1295 follow-ups — semver ordering, manual-update policy, telemetry#1296

Open
branarakic wants to merge 1 commit into
mainfrom
fix/pr1295-followups
Open

fix(cli): #1295 follow-ups — semver ordering, manual-update policy, telemetry#1296
branarakic wants to merge 1 commit into
mainfrom
fix/pr1295-followups

Conversation

@branarakic

Copy link
Copy Markdown
Contributor

Follow-up to the merged #1295 (per-node auto-update channel), addressing review round 4 + 2 refinements from an adversarial self-review. None were active on the live 10.0.0 fleet — each requires build-metadata versions, manual RC installs, garbage dist-tags, or a misconfigured channel.

Fixes

  • P1 — compareSemver mis-parsed build metadata. It detected the prerelease via the raw string's -, so a STABLE 10.0.0+mainnet-build.1 (hyphen in +build) sorted below 10.0.0-rc.19 and read as "not newer" → the forward-only gate would refuse the update. Now strips build metadata before parsing (semver §10), matching isPrerelease. (feat(cli): per-node auto-update channel (decouple testnet from latest) #1295 round-3 patched isPrerelease but missed compareSemver.)
  • P3 — candidate masking. The default [latest,dev,beta,next] path sorted before filtering valid semver, so a single garbage tag (latest="garbage") could sort ahead of and mask a valid one (beta="9.0.0-beta.4"). Now filters to valid semver before sorting.
  • P2 — manual-update policy bypass. dkg update 10.1.0-rc.1 / dkg update latest (when latest points at an RC) bypassed allowPrerelease:false (only the no-arg registry check applied it). Now gated via a tested, dep-injected resolveExplicitUpdateTarget: refuse unless --allow-prerelease, resolving a dist-tag to its concrete version first, and failing closed on a transient registry error (matching the no-arg path).
  • P2 — GELF telemetry. versionStatus reported "latest" whenever upToDate, hiding a channelTargetMissing node from central monitoring. Now emits "channel-missing".

Tests

compareSemver hyphenated-build ordering; garbage-tag-doesn't-mask-valid; +build-over-rc end-to-end via the mainnet channel; full resolveExplicitUpdateTarget matrix (stable/prerelease/dist-tag→stable/dist-tag→rc/registry-error-fail-closed/unknown-tag).

Verification

Full Bura: cli suite green locally (2109 passed, 0 failed); tsc --noEmit clean. Adversarially self-reviewed — the compareSemver rewrite was executed against a real DKG version matrix with no ordering regression.

Deferred (team's call)

Surfacing updateChannelTargetMissing in dkg status CLI output + node-ui (P2/P3) remains the tracked follow-up.

🤖 Generated with Claude Code

…, telemetry

Follow-up to merged #1295 (per-node auto-update channel). Bugs from review round 4
+ 2 refinements from an adversarial self-review. None active on the live 10.0.0
fleet (each needs build-metadata versions, manual RC installs, garbage tags, or
channel misconfig):

P1 — compareSemver mis-parsed build metadata: it detected prerelease via the raw
string's `-`, so a STABLE `10.0.0+mainnet-build.1` (hyphen in +build) sorted BELOW
`10.0.0-rc.19` and read as "not newer". Now strips build metadata before parsing
(semver §10), matching isPrerelease. (Round-3 patched isPrerelease but missed this.)

P3 — default candidate selection sorted before filtering valid semver, so a single
garbage tag (latest="garbage") could sort ahead of and mask a valid one
(beta="9.0.0-beta.4"). Now filters to valid semver before sorting.

P2 — explicit `dkg update <rc>` / `dkg update latest` (when latest is an RC)
bypassed allowPrerelease:false. Now gated via a tested, dep-injected
`resolveExplicitUpdateTarget` (refuse unless --allow-prerelease; dist-tags
resolved to a concrete version first; FAILS CLOSED on a transient registry error,
matching the no-arg path).

P2 — GELF telemetry versionStatus reported "latest" whenever upToDate, hiding a
channelTargetMissing node from central monitoring. Now emits "channel-missing".

Tests: compareSemver hyphenated-build ordering; garbage-tag-does-not-mask-valid;
+build-over-rc end-to-end; full resolveExplicitUpdateTarget matrix incl. fail-closed.

Deferred (team's call): surfacing updateChannelTargetMissing in `dkg status` +
node-ui (P2/P3) remains the tracked follow-up.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@otReviewAgent otReviewAgent left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Operational Notice: Review Agent could not complete this review.

Business logic reviewer failed: retry_exhausted

// A pinned channel with no acceptable target is NOT healthy "latest" —
// surface it distinctly so central monitoring can flag an unpublished /
// rejected channel instead of seeing a falsely-current node.
if (daemonState.lastUpdateCheck.channelTargetMissing) return "channel-missing";

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟡 Issue: This new telemetry state is not covered by the added tests. The existing deriveUpdateCheckState tests prove channelTargetMissing can be set, but nothing exercises the versionStatus callback passed to LogPushWorker, so removing or reordering this branch would still leave the suite green while central monitoring reports latest instead of channel-missing. Add a focused test, or extract this status mapping into a pure helper, that covers autoUpdateEnabled=true, checkedAt set, and channelTargetMissing=true.

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.

2 participants