Skip to content

fix(quake): parse semver build metadata in image tags - #278

Open
huklaa wants to merge 3 commits into
circlefin:mainfrom
huklaa:fix-quake-semver-build-metadata
Open

fix(quake): parse semver build metadata in image tags#278
huklaa wants to merge 3 commits into
circlefin:mainfrom
huklaa:fix-quake-semver-build-metadata

Conversation

@huklaa

@huklaa huklaa commented Aug 21, 2026

Copy link
Copy Markdown

Fix Quake image-version parsing for valid SemVer tags containing build metadata such as v0.6.0+build.1. Previously these tags were treated as unparsable, which could skip version-specific CLI compatibility rewrites. Adds regression coverage around the v0.5.0 and v0.6.0 compatibility boundaries.

@osr21

osr21 commented Aug 21, 2026

Copy link
Copy Markdown

Reviewed the logic at 3ed7404 — the fix is correct, including for the dotted +build.1 case in the PR title, and the added tests are genuine regression guards. Details, since the correctness here is a bit non-obvious:

Why the fix works even though +build.1 contains a dot. parse_image_semver splits the whole version on ., so v0.6.0+build.1 → strip v"0.6.0+build.1"split('.')["0","6","0+build","1"]four parts, not three. This only survives because the length guard is if parts.len() < 3 (tolerant of extra segments), not != 3. Then the new parts[2].split(|c| c == '-' || c == '+') turns "0+build" into "0". Both pieces have to cooperate: had the guard been != 3, this fix would silently fail for dotted build metadata and the +build.1 tests would exercise the None fallback instead. Worth a one-line comment near the length check noting it's intentionally < 3 so nobody "tightens" it to != 3 later and quietly reintroduces this bug.

It also handles combined prerelease+build correctly (v0.6.0-rc1+build.5parts[2] = "0-rc1+build" → first segment before either delimiter → "0"), since split breaks on whichever of -/+ comes first.

The new tests are meaningful, not tautological. supports_cli_flags(t) = check_cli_version(t) != RequiresConfigToml, and MIN_CLI_FLAGS_VERSION = (0,5,0). So:

  • v0.4.0+build.1: before the fix, patch "0+build" fails parse::<u64>()Nonecheck_cli_version returns Assumedsupports_cli_flags = true. After, it parses to (0,4,0) → below boundary → RequiresConfigTomlfalse. So assert!(!supports_cli_flags(Some("v0.4.0+build.1"))) genuinely fails without this patch — it's a real guard.

This is why the bug mattered (and the PR body slightly undersells it). The failure was in the unsafe direction: an old image below the CLI-flags boundary that happened to carry build metadata (v0.4.0+build.1) parsed to None, and apply_version_compat's None branch passes flags through unchanged — i.e., it assumes the target supports every flag and skips the v0.5.0/v0.6.0 compat rewrites. So a pre-flags binary could be handed flags it doesn't understand. The fix makes those versions parse and get the correct compat treatment.

One thing to flag for the merger: the Rust test job is skipped on this head (check-runs: 1 success + 4 skipped, legacy status empty) — same as the other open PRs in this batch. So these new regression assertions aren't actually being executed by CI here. This is the one PR in the batch that changes real logic rather than comments, so it's the one where that matters: please make sure the test suite actually runs (approve/trigger workflows) before merge, rather than merging on the skipped-but-green appearance. I verified the logic by hand — no Rust toolchain in my environment — and it should pass, but hand-verification isn't the suite. mergeable=true, mergeable_state=blocked (maintainer-side review/branch protection).

Optional, non-blocking: since this is fundamentally semver parsing, the semver crate's Version::parse would handle prerelease/build per spec for free — but the hand-rolled parser is deliberately lenient (tolerates extra segments and non-semver junk by design), so pulling in a dep probably isn't worth it. Fine as-is.

Solid fix — LGTM once CI actually exercises the new tests.

@osr21

osr21 commented Aug 25, 2026

Copy link
Copy Markdown

Reviewed ab6b6e6 — these are the right cases to add. The two commits before this covered prerelease and build metadata separately, but never composed; v0.5.0-rc1+build.1 / v0.6.x-rc1+build.1 exercise the full MAJOR.MINOR.PATCH-pre+build form. Traced it through the parser: 0.5.0-rc1+build.1 splits on . into ["0","5","0-rc1+build","1"], the patch segment truncates at the first -/+ to 0, giving (0,5,0) — and the !supports_cli_flags("v0.4.0-rc1+build.1") negative keeps the 0.5.0 boundary honest from below. The doc comment on parse_image_semver was already updated to [-...][+...] in 3ed7404, so docs and tests now agree.

Two small observations, take or leave:

  1. The new comment above parts.len() < 3 is accurate but understates the case: dot-separated prerelease identifiers (v0.6.0-rc.1) are the other — and in registry tags arguably more common — source of extra segments, and they were what the lax < 3 lower bound was already silently carrying. They take the same path (0-rc0) but no test pins them. One line per table ("v0.6.0-rc.1" and friends) would cover it, and the comment could read "prerelease and build metadata are dot-separated identifier sequences (SemVer §9–10)" so it explains both.

  2. Build metadata may also contain hyphens — v0.6.0+2026-08-26 is valid SemVer and plausible for CI-stamped images. It parses correctly today only because the patch segment splits at the first of -/+; that's incidental to the current implementation rather than guaranteed by a test. A case in the "supported" table would keep it from regressing if the truncation logic is ever rewritten.

Neither blocks — the fix plus this coverage already handles everything the scenario TOMLs are likely to pin.

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