Skip to content

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

Open
huklaa wants to merge 1 commit into
circlefin:mainfrom
huklaa:fix-quake-semver-build-metadata
Open

fix(quake): parse semver build metadata in image tags#278
huklaa wants to merge 1 commit 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.

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