fix： normalize windows portable naming#109
Merged
zouyonghe merged 6 commits intoAstrBotDevs:mainfrom Apr 1, 2026
Merged
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The
LEGACY_NIGHTLY_VERSION_REpattern uses a very loose(?P<version>.+?)prefix; consider tightening this to the expected semver (or whatever format is actually used) so non-nightly versions with-nightly-like substrings don't get accidentally normalized. - When deriving
portable_executable_namefromproject_config.product_name, make sureproduct_nameis guaranteed not to contain characters invalid in Windows filenames and does not already include an.exesuffix, or add a small normalization/validation layer here.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `LEGACY_NIGHTLY_VERSION_RE` pattern uses a very loose `(?P<version>.+?)` prefix; consider tightening this to the expected semver (or whatever format is actually used) so non-nightly versions with `-nightly`-like substrings don't get accidentally normalized.
- When deriving `portable_executable_name` from `project_config.product_name`, make sure `product_name` is guaranteed not to contain characters invalid in Windows filenames and does not already include an `.exe` suffix, or add a small normalization/validation layer here.
## Individual Comments
### Comment 1
<location path="scripts/ci/test_package_windows_portable.py" line_range="30-36" />
<code_context>
"AstrBot_4.29.0_windows_amd64_portable_nightly_deadbeef.zip",
)
+ def test_installer_to_portable_name_normalizes_legacy_nightly_windows_name(self):
+ self.assertEqual(
+ MODULE.installer_to_portable_name(
+ "AstrBot_4.29.0-nightly.20260401.deadbeef_aarch64-setup.exe"
+ ),
+ "AstrBot_4.29.0_windows_arm64_portable_nightly_deadbeef.zip",
+ )
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add more coverage for legacy nightly variants that the regex is intended to handle
The new test covers one legacy nightly pattern (`4.29.0-nightly.20260401.deadbeef`). Since `LEGACY_NIGHTLY_VERSION_RE` accepts multiple separators (`[._-]`) around the date and SHA, please add parameterized cases (or a few additional tests) for variants like `4.29.0-nightly_20260401_deadbeef` and `4.29.0-nightly-20260401-deadbeef`, and for each supported arch alias (`x64`/`amd64`/`arm64`/`aarch64`) to lock in coverage for all supported legacy forms.
```suggestion
def test_installer_to_portable_name_normalizes_legacy_nightly_windows_name(self):
arch_cases = [
("x64", "amd64"),
("amd64", "amd64"),
("arm64", "arm64"),
("aarch64", "arm64"),
]
separators = [".", "_", "-"]
for arch_input, arch_output in arch_cases:
for sep in separators:
with self.subTest(arch=arch_input, sep=sep):
installer_name = (
f"AstrBot_4.29.0-nightly{sep}20260401{sep}deadbeef_{arch_input}-setup.exe"
)
expected_portable_name = (
f"AstrBot_4.29.0_windows_{arch_output}_portable_nightly_deadbeef.zip"
)
self.assertEqual(
MODULE.installer_to_portable_name(installer_name),
expected_portable_name,
)
```
</issue_to_address>
### Comment 2
<location path="scripts/ci/test_package_windows_portable.py" line_range="311" />
<code_context>
- self.assertTrue((destination_root / "astrbot-desktop-tauri.exe").is_file())
+ self.assertTrue((destination_root / "AstrBot.exe").is_file())
+ self.assertFalse((destination_root / "astrbot-desktop-tauri.exe").exists())
self.assertTrue((destination_root / "WebView2Loader.dll").is_file())
self.assertTrue(
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting the copied executable name via project_config.product_name to guard against future renames
The current checks are correct, but to make this test resilient to future product renames, derive the expected `.exe` name from `project_config.product_name` (e.g. `f"{project_config.product_name}.exe"`) instead of hardcoding `AstrBot.exe`.
Suggested implementation:
```python
)
executable_name = f"{project_config.product_name}.exe"
self.assertTrue((destination_root / executable_name).is_file())
self.assertFalse((destination_root / "astrbot-desktop-tauri.exe").exists())
self.assertTrue((destination_root / "WebView2Loader.dll").is_file())
self.assertTrue(
```
This edit assumes that `project_config` is already defined in the scope of this test method (which appears likely from `project_config=MODULE.load_project_config_from(script_path),` above). If it is instead only passed into another function and not bound as a local variable, you will need to:
1. Assign the result of `MODULE.load_project_config_from(script_path)` to a local `project_config` variable before this block, and
2. Pass that `project_config` into any existing calls that currently construct it inline.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| self.assertTrue((destination_root / "astrbot-desktop-tauri.exe").is_file()) | ||
| self.assertTrue((destination_root / "AstrBot.exe").is_file()) | ||
| self.assertFalse((destination_root / "astrbot-desktop-tauri.exe").exists()) |
There was a problem hiding this comment.
suggestion (testing): Consider asserting the copied executable name via project_config.product_name to guard against future renames
The current checks are correct, but to make this test resilient to future product renames, derive the expected .exe name from project_config.product_name (e.g. f"{project_config.product_name}.exe") instead of hardcoding AstrBot.exe.
Suggested implementation:
)
executable_name = f"{project_config.product_name}.exe"
self.assertTrue((destination_root / executable_name).is_file())
self.assertFalse((destination_root / "astrbot-desktop-tauri.exe").exists())
self.assertTrue((destination_root / "WebView2Loader.dll").is_file())
self.assertTrue(This edit assumes that project_config is already defined in the scope of this test method (which appears likely from project_config=MODULE.load_project_config_from(script_path), above). If it is instead only passed into another function and not bound as a local variable, you will need to:
- Assign the result of
MODULE.load_project_config_from(script_path)to a localproject_configvariable before this block, and - Pass that
project_configinto any existing calls that currently construct it inline.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the Windows portable packaging script to correctly handle legacy nightly version strings by extracting the version and SHA for the output filename. Additionally, it renames the packaged executable to match the project's product name and includes corresponding test updates to verify these changes. I have no feedback to provide.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The ValueError message in
resolve_product_name('productName contains characters invalid Windows filename characters') is a bit awkward and redundant; consider rephrasing it to something like'productName contains characters invalid in Windows filenames'for clarity.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The ValueError message in `resolve_product_name` (`'productName contains characters invalid Windows filename characters'`) is a bit awkward and redundant; consider rephrasing it to something like `'productName contains characters invalid in Windows filenames'` for clarity.
## Individual Comments
### Comment 1
<location path="scripts/ci/package_windows_portable.py" line_range="47" />
<code_context>
PORTABLE_RUNTIME_MARKER_RELATIVE_PATH = (
pathlib.Path("src-tauri") / "windows" / "portable-runtime-marker.txt"
)
+WINDOWS_FILENAME_INVALID_CHARS_RE = re.compile(r'[<>:"/\\|?*]')
</code_context>
<issue_to_address>
**suggestion:** Consider tightening Windows filename validation beyond just the classic invalid characters.
The current regex only filters reserved characters, but Windows also rejects names with trailing spaces/dots and reserved device names like `CON`, `NUL`, `PRN`, `COM1`, etc. Since `product_name` becomes a filename, consider extending this validation (or adding a second check) to cover these cases so we fail fast instead of generating an unusable archive.
Suggested implementation:
```python
WINDOWS_FILENAME_INVALID_CHARS_RE = re.compile(r'[<>:"/\\|?*]')
WINDOWS_FILENAME_INVALID_TRAILING_RE = re.compile(r"[ .]+$")
WINDOWS_RESERVED_DEVICE_NAMES = {
"CON",
"PRN",
"AUX",
"NUL",
*{f"COM{i}" for i in range(1, 10)},
*{f"LPT{i}" for i in range(1, 10)},
}
def validate_windows_filename(name: str) -> None:
"""Validate that `name` is a safe Windows filename (without path components).
Raises ValueError if the name is not valid on Windows.
"""
if not name or name in {".", ".."}:
raise ValueError(f"Invalid Windows filename: {name!r}")
if WINDOWS_FILENAME_INVALID_CHARS_RE.search(name):
raise ValueError(
f"Invalid Windows filename {name!r}: contains reserved characters "
r'[<>:"/\\|?*]'
)
if WINDOWS_FILENAME_INVALID_TRAILING_RE.search(name):
raise ValueError(
f"Invalid Windows filename {name!r}: trailing spaces or dots are not allowed"
)
# Windows reserves device names like CON, NUL, PRN, AUX, COM1..COM9, LPT1..LPT9
stem = name.split(".")[0].upper()
if stem in WINDOWS_RESERVED_DEVICE_NAMES:
raise ValueError(
f"Invalid Windows filename {name!r}: {stem!r} is a reserved device name"
)
```
To fully apply this validation to `product_name` (and any other filenames derived from it), also:
1. Locate where `product_name` is used to build the archive filename(s) in `scripts/ci/package_windows_portable.py` (e.g. something like `f"{product_name}-{version}.zip"` or similar).
2. Before constructing any filenames, call:
```python
validate_windows_filename(product_name)
```
If `product_name` is used directly as the filename (without extra suffixes), this is sufficient.
3. If `product_name` is combined into a larger filename (e.g. `f"{product_name}-{version}.zip"`), consider validating the final base name as well:
```python
archive_name = f"{product_name}-{version}.zip"
validate_windows_filename(archive_name)
```
4. If there are other user- or config-controlled pieces that become Windows filenames in this script, call `validate_windows_filename` on those as well to ensure they fail fast when invalid.
</issue_to_address>
### Comment 2
<location path="scripts/ci/test_package_windows_portable.py" line_range="30-51" />
<code_context>
"AstrBot_4.29.0_windows_amd64_portable_nightly_deadbeef.zip",
)
+ def test_installer_to_portable_name_normalizes_legacy_nightly_windows_name(self):
+ arch_cases = [
+ ("x64", "amd64"),
+ ("amd64", "amd64"),
+ ("arm64", "arm64"),
+ ("aarch64", "arm64"),
+ ]
+ separators = [".", "_", "-"]
+
+ for arch_input, arch_output in arch_cases:
+ for separator in separators:
+ with self.subTest(arch=arch_input, separator=separator):
+ installer_name = f"AstrBot_4.29.0-nightly{separator}20260401{separator}deadbeef_{arch_input}-setup.exe"
+ expected_name = f"AstrBot_4.29.0_windows_{arch_output}_portable_nightly_deadbeef.zip"
+ self.assertEqual(
+ MODULE.installer_to_portable_name(installer_name),
+ expected_name,
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for malformed legacy nightly versions that should *not* be treated as nightly builds
To better exercise `LEGACY_NIGHTLY_VERSION_RE`, please add cases where the `version` segment looks nightly-like but doesn’t actually match the regex (e.g. wrong SHA length, invalid date, or missing components). These should follow non-nightly behavior and yield `..._portable.zip` without the `_nightly_<sha>` suffix. You can add these as extra `subTest`s here or as a sibling test asserting the suffix is omitted when the pattern doesn’t fully match.
```suggestion
def test_installer_to_portable_name_normalizes_legacy_nightly_windows_name(self):
arch_cases = [
("x64", "amd64"),
("amd64", "amd64"),
("arm64", "arm64"),
("aarch64", "arm64"),
]
separators = [".", "_", "-"]
for arch_input, arch_output in arch_cases:
for separator in separators:
with self.subTest(arch=arch_input, separator=separator):
installer_name = f"AstrBot_4.29.0-nightly{separator}20260401{separator}deadbeef_{arch_input}-setup.exe"
expected_name = f"AstrBot_4.29.0_windows_{arch_output}_portable_nightly_deadbeef.zip"
self.assertEqual(
MODULE.installer_to_portable_name(installer_name),
expected_name,
)
def test_installer_to_portable_name_treats_malformed_legacy_nightly_as_non_nightly(
self,
):
arch_cases = [
("x64", "amd64"),
("amd64", "amd64"),
("arm64", "arm64"),
("aarch64", "arm64"),
]
separators = [".", "_", "-"]
# Cases that look "nightly-like" but should not match LEGACY_NIGHTLY_VERSION_RE
malformed_fragments = [
# Wrong SHA length (too short)
("nightly{sep}20260401{sep}deadbee", "short_sha"),
# Invalid date (13th month)
("nightly{sep}20261301{sep}deadbeef", "invalid_date"),
# Missing SHA component entirely
("nightly{sep}20260401", "missing_sha"),
]
for arch_input, arch_output in arch_cases:
for separator in separators:
for fragment_template, case_name in malformed_fragments:
fragment = fragment_template.format(sep=separator)
with self.subTest(
arch=arch_input, separator=separator, malformed_case=case_name
):
installer_name = (
f"AstrBot_4.29.0-{fragment}_{arch_input}-setup.exe"
)
# Malformed legacy nightly patterns should be treated as non-nightly
expected_name = (
f"AstrBot_4.29.0_windows_{arch_output}_portable.zip"
)
self.assertEqual(
MODULE.installer_to_portable_name(installer_name),
expected_name,
)
def test_installer_to_portable_name_rejects_noncanonical_nightly_suffix_length(
self,
):
self.assertEqual(project_config.binary_name, "astrbot-desktop-tauri")
self.assertEqual(project_config.portable_marker_name, "portable.flag")
```
</issue_to_address>
### Comment 3
<location path="scripts/ci/test_package_windows_portable.py" line_range="150-159" />
<code_context>
+ def test_load_project_config_from_rejects_product_name_with_invalid_windows_chars(
</code_context>
<issue_to_address>
**suggestion (testing):** Broaden invalid Windows filename tests to cover more disallowed characters
This test currently only checks `:` in the product name to exercise `WINDOWS_FILENAME_INVALID_CHARS_RE`. To better protect against regex regressions, please parameterize over several disallowed characters from `[<>:"/\\|?*]` (e.g., `AstrBot<Dev>`, `AstrBot?`, `AstrBot|Beta`) and assert they all raise the same `ValueError`.
Suggested implementation:
```python
def test_load_project_config_from_rejects_product_name_with_invalid_windows_chars(
self,
):
invalid_product_names = [
"AstrBot<Dev>",
"AstrBot>Dev",
'AstrBot:"Test"',
"AstrBot/Dev",
r"AstrBot\Dev",
"AstrBot|Beta",
"AstrBot?",
"AstrBot*",
]
for product_name in invalid_product_names:
with self.subTest(product_name=product_name):
with tempfile.TemporaryDirectory() as tmpdir:
project_root = Path(tmpdir)
script_path = (
project_root / "scripts" / "ci" / "package_windows_portable.py"
)
tauri_config_path = project_root / "src-tauri" / "tauri.conf.json"
cargo_toml_path = project_root / "src-tauri" / "Cargo.toml"
marker_path = (
project_root / MODULE.PORTABLE_RUNTIME_MARKER_RELATIVE_PATH
)
```
`).
Here are the edits:
<file_operations>
<file_operation operation="edit" file_path="scripts/ci/test_package_windows_portable.py">
<<<<<<< SEARCH
def test_load_project_config_from_rejects_product_name_with_invalid_windows_chars(
self,
):
with tempfile.TemporaryDirectory() as tmpdir:
project_root = Path(tmpdir)
script_path = (
project_root / "scripts" / "ci" / "package_windows_portable.py"
)
tauri_config_path = project_root / "src-tauri" / "tauri.conf.json"
cargo_toml_path = project_root / "src-tauri" / "Cargo.toml"
marker_path = project_root / MODULE.PORTABLE_RUNTIME_MARKER_RELATIVE_PATH
=======
def test_load_project_config_from_rejects_product_name_with_invalid_windows_chars(
self,
):
invalid_product_names = [
"AstrBot<Dev>",
"AstrBot>Dev",
'AstrBot:"Test"',
"AstrBot/Dev",
r"AstrBot\Dev",
"AstrBot|Beta",
"AstrBot?",
"AstrBot*",
]
for product_name in invalid_product_names:
with self.subTest(product_name=product_name):
with tempfile.TemporaryDirectory() as tmpdir:
project_root = Path(tmpdir)
script_path = (
project_root / "scripts" / "ci" / "package_windows_portable.py"
)
tauri_config_path = project_root / "src-tauri" / "tauri.conf.json"
cargo_toml_path = project_root / "src-tauri" / "Cargo.toml"
marker_path = (
project_root / MODULE.PORTABLE_RUNTIME_MARKER_RELATIVE_PATH
)
>>>>>>> REPLACE
</file_operation>
</file_operations>
<additional_changes>
To fully implement the behavior described in your review comment, you should also:
1. Locate where the test currently writes the product name into `tauri.conf.json` (or any other config) in this test. It likely uses a hard-coded string similar to `"AstrBot:Dev"` or equivalent.
2. Replace that hard-coded product name with the loop variable `product_name` so each subtest actually exercises a different invalid character:
- For example, change something like:
- `config["package"]["productName"] = "AstrBot:Dev"`
- to:
- `config["package"]["productName"] = product_name`
3. Ensure that the `ValueError` assertion (e.g., `with self.assertRaises(ValueError): MODULE.load_project_config_from(project_root)`) is inside the `with self.subTest(...)` block so it runs for each invalid product name and all of them must raise the same exception.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+30
to
51
| def test_installer_to_portable_name_normalizes_legacy_nightly_windows_name(self): | ||
| arch_cases = [ | ||
| ("x64", "amd64"), | ||
| ("amd64", "amd64"), | ||
| ("arm64", "arm64"), | ||
| ("aarch64", "arm64"), | ||
| ] | ||
| separators = [".", "_", "-"] | ||
|
|
||
| for arch_input, arch_output in arch_cases: | ||
| for separator in separators: | ||
| with self.subTest(arch=arch_input, separator=separator): | ||
| installer_name = f"AstrBot_4.29.0-nightly{separator}20260401{separator}deadbeef_{arch_input}-setup.exe" | ||
| expected_name = f"AstrBot_4.29.0_windows_{arch_output}_portable_nightly_deadbeef.zip" | ||
| self.assertEqual( | ||
| MODULE.installer_to_portable_name(installer_name), | ||
| expected_name, | ||
| ) | ||
|
|
||
| def test_installer_to_portable_name_rejects_noncanonical_nightly_suffix_length( | ||
| self, | ||
| ): |
There was a problem hiding this comment.
suggestion (testing): Add coverage for malformed legacy nightly versions that should not be treated as nightly builds
To better exercise LEGACY_NIGHTLY_VERSION_RE, please add cases where the version segment looks nightly-like but doesn’t actually match the regex (e.g. wrong SHA length, invalid date, or missing components). These should follow non-nightly behavior and yield ..._portable.zip without the _nightly_<sha> suffix. You can add these as extra subTests here or as a sibling test asserting the suffix is omitted when the pattern doesn’t fully match.
Suggested change
| def test_installer_to_portable_name_normalizes_legacy_nightly_windows_name(self): | |
| arch_cases = [ | |
| ("x64", "amd64"), | |
| ("amd64", "amd64"), | |
| ("arm64", "arm64"), | |
| ("aarch64", "arm64"), | |
| ] | |
| separators = [".", "_", "-"] | |
| for arch_input, arch_output in arch_cases: | |
| for separator in separators: | |
| with self.subTest(arch=arch_input, separator=separator): | |
| installer_name = f"AstrBot_4.29.0-nightly{separator}20260401{separator}deadbeef_{arch_input}-setup.exe" | |
| expected_name = f"AstrBot_4.29.0_windows_{arch_output}_portable_nightly_deadbeef.zip" | |
| self.assertEqual( | |
| MODULE.installer_to_portable_name(installer_name), | |
| expected_name, | |
| ) | |
| def test_installer_to_portable_name_rejects_noncanonical_nightly_suffix_length( | |
| self, | |
| ): | |
| def test_installer_to_portable_name_normalizes_legacy_nightly_windows_name(self): | |
| arch_cases = [ | |
| ("x64", "amd64"), | |
| ("amd64", "amd64"), | |
| ("arm64", "arm64"), | |
| ("aarch64", "arm64"), | |
| ] | |
| separators = [".", "_", "-"] | |
| for arch_input, arch_output in arch_cases: | |
| for separator in separators: | |
| with self.subTest(arch=arch_input, separator=separator): | |
| installer_name = f"AstrBot_4.29.0-nightly{separator}20260401{separator}deadbeef_{arch_input}-setup.exe" | |
| expected_name = f"AstrBot_4.29.0_windows_{arch_output}_portable_nightly_deadbeef.zip" | |
| self.assertEqual( | |
| MODULE.installer_to_portable_name(installer_name), | |
| expected_name, | |
| ) | |
| def test_installer_to_portable_name_treats_malformed_legacy_nightly_as_non_nightly( | |
| self, | |
| ): | |
| arch_cases = [ | |
| ("x64", "amd64"), | |
| ("amd64", "amd64"), | |
| ("arm64", "arm64"), | |
| ("aarch64", "arm64"), | |
| ] | |
| separators = [".", "_", "-"] | |
| # Cases that look "nightly-like" but should not match LEGACY_NIGHTLY_VERSION_RE | |
| malformed_fragments = [ | |
| # Wrong SHA length (too short) | |
| ("nightly{sep}20260401{sep}deadbee", "short_sha"), | |
| # Invalid date (13th month) | |
| ("nightly{sep}20261301{sep}deadbeef", "invalid_date"), | |
| # Missing SHA component entirely | |
| ("nightly{sep}20260401", "missing_sha"), | |
| ] | |
| for arch_input, arch_output in arch_cases: | |
| for separator in separators: | |
| for fragment_template, case_name in malformed_fragments: | |
| fragment = fragment_template.format(sep=separator) | |
| with self.subTest( | |
| arch=arch_input, separator=separator, malformed_case=case_name | |
| ): | |
| installer_name = ( | |
| f"AstrBot_4.29.0-{fragment}_{arch_input}-setup.exe" | |
| ) | |
| # Malformed legacy nightly patterns should be treated as non-nightly | |
| expected_name = ( | |
| f"AstrBot_4.29.0_windows_{arch_output}_portable.zip" | |
| ) | |
| self.assertEqual( | |
| MODULE.installer_to_portable_name(installer_name), | |
| expected_name, | |
| ) | |
| def test_installer_to_portable_name_rejects_noncanonical_nightly_suffix_length( | |
| self, | |
| ): | |
| self.assertEqual(project_config.binary_name, "astrbot-desktop-tauri") | |
| self.assertEqual(project_config.portable_marker_name, "portable.flag") |
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The Windows filename validation logic (regexes, reserved names set, and
validate_windows_filename) is embedded directly inpackage_windows_portable.py; consider moving this into a small shared helper/module so any future Windows-specific filename checks can reuse a single implementation. - The legacy nightly parsing now has three regexes plus date validation; to keep this logic maintainable, consider extracting a dedicated helper (e.g.
parse_legacy_nightly_version(version_str) -> (base_version, nightly_sha | None)) that encapsulates all matching and error handling in one place.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Windows filename validation logic (regexes, reserved names set, and `validate_windows_filename`) is embedded directly in `package_windows_portable.py`; consider moving this into a small shared helper/module so any future Windows-specific filename checks can reuse a single implementation.
- The legacy nightly parsing now has three regexes plus date validation; to keep this logic maintainable, consider extracting a dedicated helper (e.g. `parse_legacy_nightly_version(version_str) -> (base_version, nightly_sha | None)`) that encapsulates all matching and error handling in one place.
## Individual Comments
### Comment 1
<location path="scripts/ci/package_windows_portable.py" line_range="169" />
<code_context>
version = legacy_match.group("version")
arch = normalize_arch(legacy_match.group("arch"))
- return f"{name}_{version}_windows_{arch}_portable.zip"
+ nightly_suffix = ""
+ nightly_match = LEGACY_NIGHTLY_VERSION_RE.fullmatch(version)
+ if nightly_match and is_valid_nightly_date(nightly_match.group("date")):
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting the nightly version normalization and portable executable naming into small helpers so the main functions focus only on assembling final values.
You can localize most of the added complexity by introducing small, focused helpers instead of branching directly in your main paths.
### 1. Extract nightly normalization into a helper
Right now `installer_to_portable_name` is doing regex matching, date validation, and malformed handling inline. You can move all of that into a single helper that returns `(base_version, nightly_suffix)` and keep the existing regexes/behavior unchanged.
```python
def normalize_legacy_nightly_version(version: str) -> tuple[str, str]:
"""
Returns (normalized_version, nightly_suffix),
where nightly_suffix is '' or '_nightly_<sha>'.
"""
nightly_match = LEGACY_NIGHTLY_VERSION_RE.fullmatch(version)
if nightly_match and is_valid_nightly_date(nightly_match.group("date")):
base_version = nightly_match.group("version")
nightly_suffix = f"_nightly_{nightly_match.group('sha')}"
return base_version, nightly_suffix
malformed_match = MALFORMED_LEGACY_NIGHTLY_VERSION_RE.fullmatch(version)
if malformed_match:
return malformed_match.group("version"), ""
return version, ""
```
Then the legacy block in `installer_to_portable_name` becomes:
```python
legacy_match = WINDOWS_LEGACY_INSTALLER_RE.fullmatch(installer_name)
if legacy_match:
name = legacy_match.group("name")
version, nightly_suffix = normalize_legacy_nightly_version(
legacy_match.group("version")
)
arch = normalize_arch(legacy_match.group("arch"))
return f"{name}_{version}_windows_{arch}_portable{nightly_suffix}.zip"
```
This keeps all current regex behavior and date validation, but flattens `installer_to_portable_name` so it’s only responsible for assembling the final name.
---
### 2. Centralize the `.exe` naming in one helper
To avoid coupling between `resolve_product_name` and `populate_portable_root`, you can introduce a tiny helper that makes the executable name derivation explicit and reusable:
```python
def portable_executable_name(project_config: ProjectConfig) -> str:
# product_name is already normalized/validated to be a stem
return f"{project_config.product_name}.exe"
```
Use it in `populate_portable_root`:
```python
main_executable_path = resolve_main_executable_path(bundle_dir, project_config)
destination_root.mkdir(parents=True, exist_ok=True)
shutil.copy2(
main_executable_path,
destination_root / portable_executable_name(project_config),
)
```
This doesn’t change behavior, but reduces the mental load: all `.exe` naming logic is in one place, and `populate_portable_root` no longer needs to know how `product_name` was normalized.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The test cases that construct a temporary project layout for
load_project_config_from(repeating the sameTemporaryDirectory+scripts/src-tauri+ marker setup) could be refactored into a shared helper to reduce duplication and make future changes to the directory structure or config files easier to maintain.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The test cases that construct a temporary project layout for `load_project_config_from` (repeating the same `TemporaryDirectory` + `scripts/src-tauri` + marker setup) could be refactored into a shared helper to reduce duplication and make future changes to the directory structure or config files easier to maintain.
## Individual Comments
### Comment 1
<location path="scripts/ci/package_windows_portable.py" line_range="28" />
<code_context>
WINDOWS_LEGACY_INSTALLER_RE = re.compile(
r"(?P<name>.+?)_(?P<version>.+?)_(?P<arch>x64|amd64|arm64|aarch64)-setup\.exe$"
)
+LEGACY_NIGHTLY_BASE_VERSION_PATTERN = r"[0-9A-Za-z.+]+(?:-[0-9A-Za-z.+]+)*"
+LEGACY_NIGHTLY_VERSION_RE = re.compile(
+ rf"^(?P<version>{LEGACY_NIGHTLY_BASE_VERSION_PATTERN})-nightly[._-](?P<date>[0-9]{{8}})[._-](?P<sha>{SHORT_SHA_PATTERN})$"
</code_context>
<issue_to_address>
**issue (complexity):** Consider replacing the regex-heavy nightly version parsing with a linear string-based parser and inlining the portable exe naming to reduce unnecessary indirection and branching.
You can simplify the nightly parsing and drop some regex/branching while keeping the same behavior, and also remove the extra indirection for the portable exe name.
### 1. Simplify `normalize_legacy_nightly_version` (drop extra regexes/branches)
You don’t really need `LEGACY_NIGHTLY_BASE_VERSION_PATTERN`, `LEGACY_NIGHTLY_VERSION_RE`, `MALFORMED_LEGACY_NIGHTLY_VERSION_RE`, and `is_valid_nightly_date()` to get the same outcome. A linear string-based parser keeps the “strict valid nightly vs malformed nightly vs non-nightly” behavior but is easier to follow.
You can replace:
```python
LEGACY_NIGHTLY_BASE_VERSION_PATTERN = r"[0-9A-Za-z.+]+(?:-[0-9A-Za-z.+]+)*"
LEGACY_NIGHTLY_VERSION_RE = re.compile(
rf"^(?P<version>{LEGACY_NIGHTLY_BASE_VERSION_PATTERN})-nightly[._-](?P<date>[0-9]{{8}})[._-](?P<sha>{SHORT_SHA_PATTERN})$"
)
MALFORMED_LEGACY_NIGHTLY_VERSION_RE = re.compile(
rf"^(?P<version>{LEGACY_NIGHTLY_BASE_VERSION_PATTERN})-nightly(?:[._-].*)?$"
)
def is_valid_nightly_date(date_value: str) -> bool:
try:
datetime.strptime(date_value, "%Y%m%d")
except ValueError:
return False
return True
def normalize_legacy_nightly_version(version: str) -> tuple[str, str]:
nightly_match = LEGACY_NIGHTLY_VERSION_RE.fullmatch(version)
if nightly_match and is_valid_nightly_date(nightly_match.group("date")):
base_version = nightly_match.group("version")
nightly_suffix = f"_nightly_{nightly_match.group('sha')}"
return base_version, nightly_suffix
malformed_nightly_match = MALFORMED_LEGACY_NIGHTLY_VERSION_RE.fullmatch(version)
if malformed_nightly_match:
return malformed_nightly_match.group("version"), ""
return version, ""
```
with a more linear parser:
```python
from datetime import datetime
import re
def normalize_legacy_nightly_version(version: str) -> tuple[str, str]:
# Non-nightly versions: return unchanged
if "-nightly" not in version:
return version, ""
base_version, _, nightly_part = version.partition("-nightly")
nightly_part = nightly_part.lstrip("._-")
# Missing nightly details -> treat as malformed, keep base version only
if not nightly_part:
return base_version, ""
# Split on common separators, but we only care about first two pieces
date_value, sep, rest = re.split(r"[._-]", nightly_part, maxsplit=1) + ["", ""][:0]
if not sep or not rest:
return base_version, ""
sha, *_ = re.split(r"[._-]", rest, maxsplit=1)
# Strict date validation as before
try:
datetime.strptime(date_value, "%Y%m%d")
except ValueError:
return base_version, ""
# Strict SHA validation as before
if not re.fullmatch(SHORT_SHA_PATTERN, sha):
return base_version, ""
return base_version, f"_nightly_{sha}"
```
Key points:
- Still only emits a `_nightly_<sha>` suffix when both date and SHA are strictly valid.
- Any malformed `…-nightly` form (missing date/SHA, invalid date, invalid SHA) collapses to the base version with an empty nightly suffix, as before.
- Removes the need for `LEGACY_NIGHTLY_*` regexes and the extra malformed pattern/branch, reducing cognitive load.
(If you prefer, you can use a simpler `.split` instead of `re.split`; the main win is the single linear flow rather than two compiled patterns and three branches.)
### 2. Inline `portable_executable_name` to avoid extra indirection
Right now:
```python
def portable_executable_name(project_config: ProjectConfig) -> str:
return f"{project_config.product_name}.exe"
# ...
shutil.copy2(
main_executable_path,
destination_root / portable_executable_name(project_config),
)
```
Since this is just `product_name + ".exe"` and you’re already normalizing/validating `product_name`, you can inline it and drop the function without losing the new behavior of naming the portable exe after `product_name`:
```python
shutil.copy2(
main_executable_path,
destination_root / f"{project_config.product_name}.exe",
)
```
This keeps the new naming behavior but removes a level of indirection until there’s a concrete need for additional logic around the portable exe name.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey - I've found 2 issues
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="scripts/ci/package_windows_portable.py" line_range="127-129" />
<code_context>
+ if not nightly_part:
+ return base_version, ""
+
+ parts = re.split(r"[._-]", nightly_part, maxsplit=2)
+ if len(parts) < 2:
+ return base_version, ""
+
</code_context>
<issue_to_address>
**suggestion:** Consider tightening handling of extra nightly version components instead of silently ignoring them.
With `maxsplit=2`, a value like `20240301.abcd123.extra` yields three parts, but only the first two are used, so `parts[2]` is silently dropped and the string is still treated as a valid nightly. To avoid normalizing malformed inputs, either reject `len(parts) == 3` or explicitly validate/encode the third component instead of ignoring it.
```suggestion
parts = re.split(r"[._-]", nightly_part, maxsplit=2)
# Expect exactly "<date>.<short-sha>" (with '.', '_' or '-' as separators).
# If we get any extra components (len(parts) != 2), treat this as a malformed
# nightly tag and avoid normalizing it.
if len(parts) != 2:
return version, ""
```
</issue_to_address>
### Comment 2
<location path="scripts/ci/test_package_windows_portable.py" line_range="250-255" />
<code_context>
+ with self.assertRaisesRegex(ValueError, "trailing spaces or dots"):
+ MODULE.load_project_config_from(layout["script_path"])
+
+ def test_load_project_config_from_strips_exe_suffix_from_product_name(self):
+ layout = self.make_project_layout(product_name="AstrBot.exe")
+
+ project_config = MODULE.load_project_config_from(layout["script_path"])
+
+ self.assertEqual(project_config.product_name, "AstrBot")
def test_load_cargo_package_name_supports_inline_comments(self):
</code_context>
<issue_to_address>
**suggestion (testing):** Extend .exe suffix stripping tests to cover case-insensitivity and extra whitespace
Since the implementation is case-insensitive and tolerates trailing whitespace, please add a few variants here to lock in that behavior, e.g.:
- `"AstrBot.EXE"` / `"AstrBot.ExE"`
- `"AstrBot.exe "` (trailing spaces)
This will help catch regressions if the normalization logic is refactored later.
```suggestion
def test_load_project_config_from_strips_exe_suffix_from_product_name(self):
for raw_name in ("AstrBot.exe", "AstrBot.EXE", "AstrBot.ExE", "AstrBot.exe "):
with self.subTest(product_name=raw_name):
layout = self.make_project_layout(product_name=raw_name)
project_config = MODULE.load_project_config_from(layout["script_path"])
self.assertEqual(project_config.product_name, "AstrBot")
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+250
to
+255
| def test_load_project_config_from_strips_exe_suffix_from_product_name(self): | ||
| layout = self.make_project_layout(product_name="AstrBot.exe") | ||
|
|
||
| project_config = MODULE.load_project_config_from(layout["script_path"]) | ||
|
|
||
| self.assertEqual(project_config.product_name, "AstrBot") |
There was a problem hiding this comment.
suggestion (testing): Extend .exe suffix stripping tests to cover case-insensitivity and extra whitespace
Since the implementation is case-insensitive and tolerates trailing whitespace, please add a few variants here to lock in that behavior, e.g.:
"AstrBot.EXE"/"AstrBot.ExE""AstrBot.exe "(trailing spaces)
This will help catch regressions if the normalization logic is refactored later.
Suggested change
| def test_load_project_config_from_strips_exe_suffix_from_product_name(self): | |
| layout = self.make_project_layout(product_name="AstrBot.exe") | |
| project_config = MODULE.load_project_config_from(layout["script_path"]) | |
| self.assertEqual(project_config.product_name, "AstrBot") | |
| def test_load_project_config_from_strips_exe_suffix_from_product_name(self): | |
| for raw_name in ("AstrBot.exe", "AstrBot.EXE", "AstrBot.ExE", "AstrBot.exe "): | |
| with self.subTest(product_name=raw_name): | |
| layout = self.make_project_layout(product_name=raw_name) | |
| project_config = MODULE.load_project_config_from(layout["script_path"]) | |
| self.assertEqual(project_config.product_name, "AstrBot") |
Member
Author
|
@sourcery-ai review |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
AstrBot.exeso it matches the user-facing Windows app nameProblem
After the earlier Windows portable work merged, the generated portable zip names were still awkward for nightly releases. A legacy nightly installer name could flow through to a portable artifact like
AstrBot_4.22.2-nightly.20260401.4d2791aa_windows_arm64_portable_nightly_4d2791aa.zip, which duplicated the nightly hash and looked inconsistent next to the rest of the release assets.The executable inside the portable zip also kept the Rust binary filename
astrbot-desktop-tauri.exe, which did not match the user-facing Windows app name shown by the installer builds.Root Cause
The portable packager normalized canonical nightly installer names correctly, but legacy nightly installer names still carried their embedded nightly version string into the generated portable filename before an additional canonical nightly suffix was appended. That produced the duplicated hash and mixed naming formats.
Separately, the portable packager copied the built release executable using its Rust package/binary name directly instead of renaming it to the Windows product name used for the installed application experience.
Fix
This change teaches the portable packaging script to recognize legacy nightly installer names, strip the embedded nightly payload from the version segment, and emit the same canonical portable filename format used elsewhere:
AstrBot_<version>_windows_<arch>_portable[_nightly_<sha>].zip.It also renames the executable copied into the portable package root to
AstrBot.exe, so the extracted portable app looks the same to users as the installed Windows app.Validation
python3 -m unittest discover -s scripts/ci -p 'test_*.py'python3 -m compileall -q scriptsnode --test $(find scripts -type f -name '*.test.mjs' | sort)Summary by Sourcery
Normalize Windows portable packaging naming and enforce valid Windows product executable names.
Bug Fixes:
Enhancements: