fix(esptool): parse version from release tag when filename is generic (#1217)

[zackees]

Fixes #1217 (fault 1 — the version parse, which is what clears the outage). Follow-ups split out to #1219 and #1220 so they survive this closing #1217.

Problem

extract_esptool_version() only looked at the metadata URL's filename. Two URL shapes exist in the wild and they put the version in opposite places:

Publisher	URL	Version lives in
pioarduino registry	.../releases/download/0.0.1/esptoolpy-v5.3.0.zip	filename (0.0.1 is just the registry tag)
platform-espressif32 53.03.10	.../releases/download/v4.8.5/esptool.zip	release tag (filename is generic)

Only the first was handled. For the second the parse returned "unknown", so the tasmota download URL became .../releases/download/vunknown/esptool-linux-amd64.zip → 404 → resolve_esptool() warned and returned None → the linker degraded to a bare esptool PATH lookup (esp32_linker.rs:73).

That silent degradation is what broke FastLED master: every ESP32 build has failed since 2026-07-30 at elf2image, three minutes in, with

build error: build failed: esptool not found — cannot convert firmware.elf to firmware.bin.
Error: failed to spawn ["esptool", "--chip", "esp32", "elf2image", ...]: No such file or directory

...and a hint suggesting pip install esptool — even though esptool 5.1.0 was installed, just not on the daemon's PATH.

Change

Split the digit scan into dotted_version_in(), then try filename → parent path segment.

The ordering is load-bearing. The filename must keep winning, or the registry case regresses to the 0.0.1 tag. That invariant was previously only implied by a single test; it now has an explicit one (filename_version_still_wins_over_release_tag) using a URL where both components carry a dotted version.

Second commit updates two doc comments that described only the filename shape — the module-flow step and the from_metadata_url constructor. That stale doc is a good part of why the second shape was easy to miss.

Tests

soldr cargo test -p fbuild-library --lib esptool → 12 passed.

• extract_version_from_release_tag_when_filename_is_generic — new, the outage case, asserts 4.8.5
• filename_version_still_wins_over_release_tag — new, ordering guard
• extract_version_falls_back_to_unknown — unchanged and still passing; neither component of https://example.com/esptool.zip carries a dotted version, so the genuine unknown case still returns "unknown" rather than being papered over

cargo fmt --check and clippy clean.

Deliberately not in scope

Kept narrow so it's reviewable and cherry-pickable for a patch release. The rest of #1217's findings are now tracked separately:

• daemon binds tool resolution to spawn-time PATH for up to 12h — bare-name esptool/python/objcopy spawns resolve nondeterministically #1219 — the daemon binds tool resolution to spawn-time PATH for up to 12h (IDLE_TIMEOUT = 43200s). This is why the PATH fallback failed rather than merely being slow, and it affects python/objcopy too, not just esptool.
• esptool: provisioning failure degrades silently to PATH, and there is no FBUILD_ESPTOOL_PATH override #1220 — provisioning failure degrades silently to a PATH lookup, and esptool is the only provisioned tool with no FBUILD_*_PATH override (the only prefix surviving the daemon env_clear).

With this PR provisioning succeeds and the fallback is never reached, so the outage clears — but the fallback is still fragile for any future URL shape, which is what those two cover.

[coderabbitai]

📝 Walkthrough

Walkthrough

The ESPTool URL parser now detects dotted numeric versions in filenames, falls back to the parent URL segment, and returns "unknown" when neither location contains a version. Tests cover fallback and precedence.

Changes

ESPTool version extraction

Layer / File(s)	Summary
Version parser and test coverage crates/fbuild-library/src/library/esptool.rs	extract_esptool_version checks the filename before the parent URL segment and otherwise returns "unknown". Tests cover generic filenames and filename precedence.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

• FastLED/fbuild#988: Introduces the ESPTool provisioning flow that relies on this version extraction logic.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR implements [#1217] objective 1 but omits the requested provisioning error, path override, and per-request environment fixes.	Implement the remaining [#1217] fixes, or update the linked issue scope so this PR covers only release-tag parsing.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Out of Scope Changes check	✅ Passed	All changes are limited to esptool version extraction and related tests, matching the stated objective from [#1217].
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly describes parsing the esptool version from a release tag when the filename is generic.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/1217-esptool-version-from-release-tag

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@crates/fbuild-library/src/library/esptool.rs`:
- Around line 294-303: Update extract_esptool_version to remove URL query and
fragment components, or use the parsed URL path, before splitting path segments
and calling dotted_version_in. Preserve filename-version precedence and ensure
registry-tag fallback reads the actual path segment. Add regression tests
covering URLs with query and fragment data, including the
v4.8.5/esptool.zip?channel=1.2 case and from_metadata_url behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 606f7a87-50b4-4328-89b7-50d158d75b97

📥 Commits

Reviewing files that changed from the base of the PR and between 3e02dc9 and 8d70b7f.

📒 Files selected for processing (1)

• crates/fbuild-library/src/library/esptool.rs

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Parse the URL path before extracting the version.

extract_esptool_version splits the raw URL with rsplit('/'). Query and fragment data are not part of the filename. For example, .../v4.8.5/esptool.zip?channel=1.2 returns "1.2" and skips the valid release-tag fallback. from_metadata_url then stores the wrong version, which can cause provisioning to request the wrong artifact.

Strip the query and fragment, or parse the URL path before splitting it. Add regression tests for both cases.

Proposed fix

 fn extract_esptool_version(url: &str) -> String {
-    let mut segments = url.rsplit('/');
+    let path = url.split_once('?').map_or(url, |(path, _)| path);
+    let path = path.split_once('#').map_or(path, |(path, _)| path);
+    let mut segments = path.rsplit('/');

+    #[test]
+    fn extract_version_ignores_query_and_fragment() {
+        assert_eq!(
+            extract_esptool_version(
+                "https://github.com/pioarduino/esptool/releases/download/v4.8.5/esptool.zip?channel=1.2"
+            ),
+            "4.8.5"
+        );
+        assert_eq!(
+            extract_esptool_version(
+                "https://github.com/pioarduino/esptool/releases/download/v4.8.5/esptool.zip#mirror/1.2"
+            ),
+            "4.8.5"
+        );
+    }

Also applies to: 329-354

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/fbuild-library/src/library/esptool.rs` around lines 294 - 303, Update
extract_esptool_version to remove URL query and fragment components, or use the
parsed URL path, before splitting path segments and calling dotted_version_in.
Preserve filename-version precedence and ensure registry-tag fallback reads the
actual path segment. Add regression tests covering URLs with query and fragment
data, including the v4.8.5/esptool.zip?channel=1.2 case and from_metadata_url
behavior.