From 6a47fbbfa12c2306367465b42ee8e05eec28b51a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 11 May 2026 12:58:10 +0000 Subject: [PATCH 1/2] Initial plan From 744208c549804f67e88edd90d2b70791121a565a Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 11 May 2026 13:05:24 +0000 Subject: [PATCH 2/2] Fix three high-priority correctness issues in envoy.github.release - Fix 8.3: Replace `r"v(\w+)"` with `r"^v(\d+\.\d+\.\d+.*)$"` in manager.py and use fullmatch+group(1) instead of sub() to correctly parse versions like v1.19.0 (previously \w didn't match . so the regex was incorrect) - Fix 4.2: Check HTTP status before streaming download body in assets.py so 404/error responses are not written to disk before the error is reported - Fix 6.1: Use `raise e.args[0] from e` instead of bare `raise e.args[0]` in release.py to preserve the ConcurrentError exception chain Add corresponding tests for each fix: - Updated test_release_manager_parse_version to use fullmatch semantics - Added test_release_manager_parse_version_regex integration test - Added test_release_manager_latest_minors integration test - Updated test_fetcher_save to assert stream.writer not called on non-200 - Updated test_release_push to assert __cause__ is set on ConcurrentError Agent-Logs-Url: https://github.com/envoyproxy/toolshed/sessions/a7bfdad5-6b3e-4180-ae1b-33134aa0554c Co-authored-by: phlax <454682+phlax@users.noreply.github.com> --- .../envoy/github/release/assets.py | 7 +- .../envoy/github/release/manager.py | 8 +- .../envoy/github/release/release.py | 2 +- py/envoy.github.release/tests/test_assets.py | 25 +++--- py/envoy.github.release/tests/test_manager.py | 83 ++++++++++++++++--- py/envoy.github.release/tests/test_release.py | 5 +- 6 files changed, 99 insertions(+), 31 deletions(-) diff --git a/py/envoy.github.release/envoy/github/release/assets.py b/py/envoy.github.release/envoy/github/release/assets.py index 1192e91fb6..88c00b92ed 100644 --- a/py/envoy.github.release/envoy/github/release/assets.py +++ b/py/envoy.github.release/envoy/github/release/assets.py @@ -59,15 +59,16 @@ async def save( download: aiohttp.ClientResponse) -> dict[ str, str | pathlib.Path]: outfile = self.path.joinpath(asset_type, name) - outfile.parent.mkdir(exist_ok=True) - async with stream.writer(outfile) as f: - await f.stream_bytes(download) result: dict[str, str | pathlib.Path] = dict( name=name, outfile=outfile) if download.status != 200: result["error"] = self.fail( f"Failed downloading, got response:\n{download}") + return result + outfile.parent.mkdir(exist_ok=True) + async with stream.writer(outfile) as f: + await f.stream_bytes(download) return result diff --git a/py/envoy.github.release/envoy/github/release/manager.py b/py/envoy.github.release/envoy/github/release/manager.py index b9b2cb3ea0..5452d05621 100644 --- a/py/envoy.github.release/envoy/github/release/manager.py +++ b/py/envoy.github.release/envoy/github/release/manager.py @@ -22,7 +22,7 @@ @abstracts.implementer(AGithubReleaseManager) class GithubReleaseManager: - _version_re = r"v(\w+)" + _version_re = r"^v(\d+\.\d+\.\d+.*)$" _version_format = "v{version}" async def __aenter__(self) -> AGithubReleaseManager: @@ -101,10 +101,10 @@ def format_version( def parse_version( self, version: str) -> packaging.version.Version | None: - _version = self.version_re.sub(r"\1", version) - if _version: + m = self.version_re.fullmatch(version) + if m: try: - return packaging.version.Version(_version) + return packaging.version.Version(m.group(1)) except packaging.version.InvalidVersion: pass self.log.warning(f"Unable to parse version: {version}") diff --git a/py/envoy.github.release/envoy/github/release/release.py b/py/envoy.github.release/envoy/github/release/release.py index 7e6e60a223..06cb5d4784 100644 --- a/py/envoy.github.release/envoy/github/release/release.py +++ b/py/envoy.github.release/envoy/github/release/release.py @@ -198,7 +198,7 @@ async def push( response["assets"].append(result) self.log.info(f"Release file uploaded {result['name']}") except ConcurrentError as e: - raise e.args[0] + raise e.args[0] from e if not response["errors"]: self.log.success(f"Assets uploaded: {self.version}") return response diff --git a/py/envoy.github.release/tests/test_assets.py b/py/envoy.github.release/tests/test_assets.py index 2866e112aa..da5441d08e 100644 --- a/py/envoy.github.release/tests/test_assets.py +++ b/py/envoy.github.release/tests/test_assets.py @@ -205,24 +205,27 @@ async def test_fetcher_save(patches, status): m_fail.call_args == [(f"Failed downloading, got response:\n{download}", ), {}]) expected["error"] = m_fail.return_value + # stream.writer must NOT be called on error + assert not m_stream.writer.called + assert not outfile.parent.mkdir.called else: assert not m_fail.called + assert ( + outfile.parent.mkdir.call_args + == [(), dict(exist_ok=True)]) + writer = m_stream.writer + assert ( + writer.call_args + == [(outfile, ), {}]) + stream_bytes = writer.return_value.__aenter__.return_value.stream_bytes + assert ( + stream_bytes.call_args + == [(download, ), {}]) assert result == expected assert ( m_path.return_value.joinpath.call_args == [('ASSET TYPE', 'NAME'), {}]) - assert ( - outfile.parent.mkdir.call_args - == [(), dict(exist_ok=True)]) - writer = m_stream.writer - assert ( - writer.call_args - == [(outfile, ), {}]) - stream_bytes = writer.return_value.__aenter__.return_value.stream_bytes - assert ( - stream_bytes.call_args - == [(download, ), {}]) def test_pusher_constructor(patches): diff --git a/py/envoy.github.release/tests/test_manager.py b/py/envoy.github.release/tests/test_manager.py index 0f69922b36..fed87e83f0 100644 --- a/py/envoy.github.release/tests/test_manager.py +++ b/py/envoy.github.release/tests/test_manager.py @@ -52,7 +52,7 @@ def test_release_manager_constructor( assert releaser._github == github assert releaser._session == session - assert releaser._version_re == r"v(\w+)" + assert releaser._version_re == r"^v(\d+\.\d+\.\d+.*)$" async def test_release_manager_async_contextmanager(patches): @@ -335,11 +335,11 @@ def test_release_manager_format_version(): == [(), dict(version="VERSION")]) -@pytest.mark.parametrize("version", [None, 0, "", "1.2.3"]) +@pytest.mark.parametrize("matched", [None, "1.2.3"]) @pytest.mark.parametrize( "raises", [None, BaseException, packaging.version.InvalidVersion]) -def test_release_manager_parse_version(patches, version, raises): +def test_release_manager_parse_version(patches, matched, raises): releaser = GithubReleaseManager("PATH", "REPOSITORY") patched = patches( "packaging.version.Version", @@ -348,33 +348,94 @@ def test_release_manager_parse_version(patches, version, raises): prefix="envoy.github.release.manager") with patched as (m_packaging, m_log, m_version): - m_version.return_value.sub.return_value = version + if matched: + m_match = MagicMock() + m_match.group.return_value = matched + m_version.return_value.fullmatch.return_value = m_match + else: + m_version.return_value.fullmatch.return_value = None if raises: m_packaging.side_effect = raises() - if version and raises == BaseException: + if matched and raises == BaseException: with pytest.raises(BaseException): releaser.parse_version("VERSION") else: assert ( releaser.parse_version("VERSION") == (None - if not version or raises + if not matched or raises else m_packaging.return_value)) assert ( - m_version.return_value.sub.call_args - == [(r"\1", "VERSION"), {}]) - if version: + m_version.return_value.fullmatch.call_args + == [("VERSION", ), {}]) + if matched: + assert ( + m_match.group.call_args + == [(1, ), {}]) assert ( m_packaging.call_args - == [(m_version.return_value.sub.return_value, ), {}]) + == [(m_match.group.return_value, ), {}]) else: assert not m_packaging.called - if not version or raises and raises != BaseException: + if not matched or raises and raises != BaseException: assert ( m_log.return_value.warning.call_args == [("Unable to parse version: VERSION", ), {}]) else: assert not m_log.called + + +@pytest.mark.parametrize( + "version,expected", + [("v1.19.0", packaging.version.Version("1.19.0")), + ("v0.0.1", packaging.version.Version("0.0.1")), + ("v1.2.3-rc1", packaging.version.Version("1.2.3rc1")), + ("v1.2.3.dev0", packaging.version.Version("1.2.3.dev0")), + ("v1", None), + ("notaversion", None)]) +def test_release_manager_parse_version_regex(patches, version, expected): + """Integration test: parse_version with the real version_re regex.""" + releaser = GithubReleaseManager("PATH", "REPOSITORY") + patched = patches( + ("GithubReleaseManager.log", dict(new_callable=PropertyMock)), + prefix="envoy.github.release.manager") + + with patched as (m_log, ): + result = releaser.parse_version(version) + + assert result == expected + if expected is None: + assert ( + m_log.return_value.warning.call_args + == [(f"Unable to parse version: {version}", ), {}]) + else: + assert not m_log.called + + +async def test_release_manager_latest_minors(patches): + """Integration test: latest dict uses correct per-minor bucketing.""" + releaser = GithubReleaseManager("PATH", "REPOSITORY") + patched = patches( + ("GithubReleaseManager.releases", dict(new_callable=PropertyMock)), + prefix="envoy.github.release.manager") + + _versions = [ + dict(tag_name=v) + for v in ("v1.18.0", "v1.19.0", "v1.19.1", "v2.0.0")] + + with patched as (m_releases, ): + m_releases.side_effect = AsyncMock(return_value=_versions) + result = await releaser.latest + + assert result == { + "1.18.0": packaging.version.Version("1.18.0"), + "1.18": packaging.version.Version("1.18.0"), + "1.19.0": packaging.version.Version("1.19.0"), + "1.19.1": packaging.version.Version("1.19.1"), + "1.19": packaging.version.Version("1.19.1"), + "2.0.0": packaging.version.Version("2.0.0"), + "2.0": packaging.version.Version("2.0.0"), + } diff --git a/py/envoy.github.release/tests/test_release.py b/py/envoy.github.release/tests/test_release.py index 3d005ca35c..82ba85cc63 100644 --- a/py/envoy.github.release/tests/test_release.py +++ b/py/envoy.github.release/tests/test_release.py @@ -514,7 +514,7 @@ async def _pusher(path): BaseException if raises == BaseException else SomeError) - with pytest.raises(exception): + with pytest.raises(exception) as exc_info: await release.push(artefacts) else: assert await release.push(artefacts) == expected @@ -528,6 +528,9 @@ async def _pusher(path): m_pusher.return_value.call_args_list == [[(release, 'ARTEFACTS0'), {}]]) assert not m_log.return_value.info.called + if raises == tasks.ConcurrentError: + assert isinstance( + exc_info.value.__cause__, tasks.ConcurrentError) else: assert ( m_pusher.return_value.call_args_list