Repository navigation
fix: surface description publication failures - #3065
IsmaelMartinez merged 2 commits into
Conversation
|
/agentic_review |
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks, and for catching the Gitea case yourself on the issue rather than stopping at the three providers that were reported.
One inline suggestion, otherwise this is ready.
- I walked all eleven implementations, not just the four you changed. The seven you leave alone already raise or propagate, and mosaico has no remote write that can fail, so the contract leaves nobody in a false-success state.
- Merged onto today's main: 3729 passed against 3723, and restoring the six source files turns all six of your new tests red.
- The suggestion is a defensive tightening of the Bitbucket status check, not a failure I reproduced.
| except: | ||
| pass | ||
| return response | ||
| if response.status_code != 200: |
There was a problem hiding this comment.
Now that this raises rather than logging at info, the strict != 200 matters in a way it did not before: a 2xx that is not exactly 200 would abort /describe and post a failure comment on a PR that did update. I have not confirmed Bitbucket returns one here, so this is defensive, matching the raise_for_status() this file already uses at :116, :124, :509 and :653.
| if response.status_code != 200: | |
| if not 200 <= response.status_code < 300: |
There was a problem hiding this comment.
Applied in a7dded9. publish_description now accepts the full 2xx range, with regression coverage for 200, 201, and 204; non-2xx responses still raise. Re-ran the complete Bitbucket provider and description lifecycle files: 63 passed. Ruff and diff checks are clean.
Code Review by QodoSorry, something went wrongWe weren't able to complete the code review on our side. Please try again manually by commenting/agentic_review on this PR.
|
IsmaelMartinez
left a comment
IsmaelMartinez
left a comment
There was a problem hiding this comment.
Thanks, the 2xx range landed as suggested and every provider now raises where it used to log. The regression tests go red on the old code.
The-PR-Agent#3065 builds PRDescription with __new__ and mocks _prepare_data, and The-PR-Agent#3098 reads self.data on the publish path, so main has failed two tests since both landed on 7 September. Co-Authored-By: Claude Fable 5.1 <[email protected]> Claude-Session: https://claude.ai/code/session_018DnpxBBKh17sPL8J6uZ5mJ
Summary
publish_description()as an exception-on-failure provider contract/describeand skip the final "updated" success messagepropagate_tool_errorslifecycle modesCloses #3057.
Validation
PYTHONPATH=. uv run pytest -q tests/unittest/test_pr_description_lifecycle.py tests/unittest/test_gitlab_provider.py tests/unittest/test_azure_devops_provider.py tests/unittest/test_bitbucket_provider.py tests/unittest/test_gitea_provider.py— 396 passeduv run ruff checkon all changed Python files — passedgit diff --check— passedThe full unit suite's first failure on this Windows host is the existing
tests/unittest/test_artifacts.py::TestResolveArtifactPath::test_root_workspace_does_not_reject_valid_paths; it reproduces unchanged on a cleanf3b385eworktree. Pre-commit could not initialize because fetchingpre-commit-hooksfailed twice with a GitHub port 443 connection error; its relevant Python checks were covered manually above.