fix(pipeline): correct dispatch correlation and log rendering - #197
Conversation
|
Warning Review limit reached
Next review available in: 35 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe CLI no longer creates unsupported manifest fields. GitHub workflow dispatch now returns and uses the exact dispatched run ID. ZIP log downloads receive validation, size limits, path-safety checks, and deterministic rendering. ChangesConfiguration and GitHub workflow behavior
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant GitHubActionsPipelineProvider
participant GitHubActionsTransport
participant GitHubActionsRESTAPI
GitHubActionsPipelineProvider->>GitHubActionsTransport: dispatch_workflow(ref, inputs)
GitHubActionsTransport->>GitHubActionsRESTAPI: POST workflow dispatch
GitHubActionsRESTAPI-->>GitHubActionsTransport: workflow_run_id
GitHubActionsTransport-->>GitHubActionsPipelineProvider: run ID
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/odoo_forge_pipeline_github/transport.py`:
- Around line 66-69: Update the response parsing exception handler in REST
transport to include UnicodeDecodeError alongside the existing JSON, key, and
type exceptions, normalizing invalid UTF-8 to the same RuntimeError. In
tests/pipeline_github/test_rest_transport.py lines 68-76, add b"\xff" to the
malformed-response test cases and verify it produces the existing RuntimeError.
In `@tests/pipeline_github/test_rest_transport.py`:
- Line 43: Update the affected tests to stop invoking or monkeypatching
GitHubActionsRestTransport._request. Mock urllib.request.urlopen at the HTTP
boundary instead, then exercise the public dispatch_workflow and get_run_logs
methods and assert their observable behavior without accessing private transport
attributes.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 7c206dab-3ac6-4e0a-85ba-3f514d09cdc6
📒 Files selected for processing (10)
src/odoo_forge_cli/commands/manifest.pysrc/odoo_forge_pipeline_github/provider.pysrc/odoo_forge_pipeline_github/transport.pytests/cli/test_configure.pytests/pipeline_github/fakes.pytests/pipeline_github/test_hermetic.pytests/pipeline_github/test_neutrality.pytests/pipeline_github/test_rest_transport.pytests/pipeline_github/test_transport_protocol.pytests/pipeline_github/test_trigger.py
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odoo_forge_pipeline_github/transport.py (1)
98-101: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winNormalize ZIP-entry read failures.
Catch
zipfile.BadZipFile,EOFError,NotImplementedError, andzlib.erroraroundarchive.read(entry). Preserve theRuntimeErrorvalues raised by size and path checks. Add a regression test for a corrupted compressed entry.🤖 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 `@src/odoo_forge_pipeline_github/transport.py` around lines 98 - 101, Update the archive-reading flow around archive.read(entry) to catch zipfile.BadZipFile, EOFError, NotImplementedError, and zlib.error and normalize them as the existing invalid-log-archive RuntimeError. Keep the RuntimeError exceptions raised by the size and path checks unchanged, and add a regression test covering a corrupted compressed entry.Source: MCP tools
🤖 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 `@tests/pipeline_github/test_rest_transport.py`:
- Around line 52-60: Add an assertion in
test_dispatch_returns_the_exact_workflow_run_id to require len(requests) == 1,
preserving the existing request method, payload, and returned workflow ID
assertions.
---
Outside diff comments:
In `@src/odoo_forge_pipeline_github/transport.py`:
- Around line 98-101: Update the archive-reading flow around archive.read(entry)
to catch zipfile.BadZipFile, EOFError, NotImplementedError, and zlib.error and
normalize them as the existing invalid-log-archive RuntimeError. Keep the
RuntimeError exceptions raised by the size and path checks unchanged, and add a
regression test covering a corrupted compressed entry.
🪄 Autofix
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: ASSERTIVE
Plan: Pro Plus
Run ID: 90274849-dff5-42b4-a466-54fb68e0c220
📒 Files selected for processing (2)
src/odoo_forge_pipeline_github/transport.pytests/pipeline_github/test_rest_transport.py
Both requested changes were addressed in commit 2a91f64; focused validation and all required checks pass.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/odoo_forge_pipeline_github/transport.py (1)
85-106: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winNormalize archive-construction errors.
zipfile.ZipFile(...)can raiseUnicodeDecodeErrorbeforearchive.infolist()runs. Catch it in the outer handler soget_run_logsraisesRuntimeError("invalid log archive"). Add a regression test through publicget_run_logs.NotImplementedErroris already handled during entry reads.🤖 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 `@src/odoo_forge_pipeline_github/transport.py` around lines 85 - 106, Update the outer exception handler around ZipFile construction in get_run_logs to also catch UnicodeDecodeError and normalize it to RuntimeError("invalid log archive"), preserving the existing entry-read handling. Add a regression test through the public get_run_logs API using an archive that triggers UnicodeDecodeError during construction and assert the normalized RuntimeError.Source: Path instructions
🤖 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.
Outside diff comments:
In `@src/odoo_forge_pipeline_github/transport.py`:
- Around line 85-106: Update the outer exception handler around ZipFile
construction in get_run_logs to also catch UnicodeDecodeError and normalize it
to RuntimeError("invalid log archive"), preserving the existing entry-read
handling. Add a regression test through the public get_run_logs API using an
archive that triggers UnicodeDecodeError during construction and assert the
normalized RuntimeError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 1eb07bc2-d9bd-4cd6-abc1-cf87bc777051
📒 Files selected for processing (2)
src/odoo_forge_pipeline_github/transport.pytests/pipeline_github/test_rest_transport.py
1 participant
Closes #196
Summary
forge configureChanges
workflow_run_idTest plan
uv run pytest— 1922 passed, 46 deselecteduv run ruff check .uv run ruff format --check .uv run mypyuv run lint-importsgit diff --checkContributor checklist
Summary by CodeRabbit