Repository navigation
feat(OMN-7972): HandlerReportPoster — final PR review summary comment - #121
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 4 minutes and 41 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughAdded a new handler module that builds and posts PR review summary comments (with dry-run support) via a GitHub bridge protocol, updated package exports to expose the poster and helpers, and excluded three files from a pre-commit hook. Changes
Sequence DiagramsequenceDiagram
actor Caller
participant HandlerReportPoster
participant build_summary_comment
participant ProtocolGitHubBridge
Caller->>HandlerReportPoster: post_summary(pr_number, repo, verdict, dry_run)
activate HandlerReportPoster
HandlerReportPoster->>build_summary_comment: build_summary_comment(verdict, findings, thread_states)
activate build_summary_comment
build_summary_comment->>build_summary_comment: _build_findings_table(findings)
build_summary_comment->>build_summary_comment: _build_thread_summary(thread_states)
build_summary_comment-->>HandlerReportPoster: comment_body
deactivate build_summary_comment
alt dry_run = true
HandlerReportPoster->>HandlerReportPoster: log would-be comment
else dry_run = false
HandlerReportPoster->>ProtocolGitHubBridge: post_pr_comment(repo, pr_number, body)
activate ProtocolGitHubBridge
ProtocolGitHubBridge-->>HandlerReportPoster: success/ack
deactivate ProtocolGitHubBridge
HandlerReportPoster->>HandlerReportPoster: log completion
end
deactivate HandlerReportPoster
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
fea58ba to
11db0b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/omnimarket/nodes/node_pr_review_bot/models/models.py (1)
50-63: Consider adding validation thatend_line >= start_line.The
DiffHunkmodel validates that both line numbers are>= 1, but doesn't enforceend_line >= start_line. An invalid hunk withend_line < start_linewould pass validation.♻️ Add model validator
+from pydantic import model_validator + class DiffHunk(BaseModel): """A single contiguous segment of a unified diff for one file.""" model_config = ConfigDict(frozen=True, extra="forbid") file_path: str = Field(..., description="Repo-relative file path.") start_line: int = Field( ..., ge=1, description="First line of the hunk (1-indexed)." ) end_line: int = Field(..., ge=1, description="Last line of the hunk (inclusive).") content: str = Field(..., description="Raw unified diff content for this hunk.") is_new_file: bool = Field(default=False) is_deleted_file: bool = Field(default=False) + + `@model_validator`(mode="after") + def _check_line_range(self) -> "DiffHunk": + if self.end_line < self.start_line: + raise ValueError("end_line must be >= start_line") + return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnimarket/nodes/node_pr_review_bot/models/models.py` around lines 50 - 63, Add a model-level validator to DiffHunk that ensures end_line >= start_line: implement a pydantic v2 model validator (e.g., `@model_validator`) on the DiffHunk class which checks the start_line and end_line fields and raises a clear ValueError if end_line < start_line, so invalid hunks are rejected during validation; keep the existing model_config and field types untouched and update the validation error message to reference DiffHunk/start_line/end_line for clarity.src/omnimarket/nodes/node_pr_review_bot/handlers/handler_report_poster.py (1)
73-88: Consider usingCounter[EnumFindingSeverity]for type precision.The counter is typed as
Counter[str]but keys areEnumFindingSeverityvalues. SinceStrEnuminherits fromstr, this works at runtime, butCounter[EnumFindingSeverity]would be more precise for static analysis.♻️ Optional type refinement
- counts: Counter[str] = Counter() + counts: Counter[EnumFindingSeverity] = Counter()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnimarket/nodes/node_pr_review_bot/handlers/handler_report_poster.py` around lines 73 - 88, The Counter in _build_findings_table is declared as Counter[str] but its keys are EnumFindingSeverity instances; update the type to Counter[EnumFindingSeverity] (or Counter[ReviewFinding.severity.__class__] if that helps type checkers) and adjust any imports/annotations so the function signature and local variable use Counter[EnumFindingSeverity] (refer to _build_findings_table, ReviewFinding, and _SEVERITY_ORDER) to provide precise static typing without changing runtime behavior.src/omnimarket/nodes/node_pr_review_bot/handlers/handler_fsm.py (1)
336-340: Consider capturingnowonce to ensure timing consistency.
datetime.now(tz=UTC)is called twice in quick succession. While unlikely to cause issues, capturing it once ensuresduration_msandcompleted_atare computed from the same instant.♻️ Suggested refactor
def make_verdict(self, state: ModelPrReviewBotState) -> ReviewVerdict: """Derive the final ReviewVerdict from completed FSM state.""" threads = state.thread_states threads_posted = sum(1 for t in threads if t.status != EnumThreadStatus.PENDING) threads_pass = sum( 1 for t in threads if t.status == EnumThreadStatus.VERIFIED_PASS ) threads_fail = sum( 1 for t in threads if t.status == EnumThreadStatus.VERIFIED_FAIL ) threads_pending = sum( 1 for t in threads if t.status in (EnumThreadStatus.PENDING, EnumThreadStatus.POSTED) ) if threads_fail > 0: verdict = EnumPrVerdict.BLOCKING_ISSUE elif state.findings: verdict = EnumPrVerdict.RISKS_NOTED else: verdict = EnumPrVerdict.CLEAN + now = datetime.now(tz=UTC) return ReviewVerdict( correlation_id=state.correlation_id, pr_number=state.pr_number, repo=state.repo, verdict=verdict, total_findings=len(state.findings), threads_posted=threads_posted, threads_verified_pass=threads_pass, threads_verified_fail=threads_fail, threads_pending=threads_pending, judge_model_used="", # set by caller from ReviewRequest duration_ms=int( - (datetime.now(tz=UTC) - state.started_at).total_seconds() * 1000 + (now - state.started_at).total_seconds() * 1000 ), - completed_at=datetime.now(tz=UTC), + completed_at=now, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnimarket/nodes/node_pr_review_bot/handlers/handler_fsm.py` around lines 336 - 340, The two calls to datetime.now(tz=UTC) in the block that sets duration_ms and completed_at (inside the handler function in handler_fsm.py where state.started_at is used) should be unified: capture a single timestamp (e.g., now = datetime.now(tz=UTC)) and use that same now to compute duration_ms = int((now - state.started_at).total_seconds() * 1000) and set completed_at=now so both values are consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/omnimarket/nodes/node_pr_review_bot/contract.yaml`:
- Around line 28-32: The contract's fsm_transition operation currently points
HandlerPrReviewBot to module
"omnimarket.nodes.node_pr_review_bot.handlers.handler_pr_review_bot" which does
not exist; update the module value for the operation named "fsm_transition"
(handler name "HandlerPrReviewBot") to
"omnimarket.nodes.node_pr_review_bot.handlers.handler_fsm" so the FSM handler
resolves to the existing handler_fsm implementation.
- Around line 73-81: The contract references non-existent model files; update
input_model and output_model to the actual model classes by replacing name
"ModelPrReviewBotStartCommand" with "ReviewRequest" and name
"ModelPrReviewBotCompletedEvent" with "ReviewVerdict", and change both module
paths to "omnimarket.nodes.node_pr_review_bot.models.models" so the contract
points to the real ReviewRequest (input) and ReviewVerdict (output) classes.
---
Nitpick comments:
In `@src/omnimarket/nodes/node_pr_review_bot/handlers/handler_fsm.py`:
- Around line 336-340: The two calls to datetime.now(tz=UTC) in the block that
sets duration_ms and completed_at (inside the handler function in handler_fsm.py
where state.started_at is used) should be unified: capture a single timestamp
(e.g., now = datetime.now(tz=UTC)) and use that same now to compute duration_ms
= int((now - state.started_at).total_seconds() * 1000) and set completed_at=now
so both values are consistent.
In `@src/omnimarket/nodes/node_pr_review_bot/handlers/handler_report_poster.py`:
- Around line 73-88: The Counter in _build_findings_table is declared as
Counter[str] but its keys are EnumFindingSeverity instances; update the type to
Counter[EnumFindingSeverity] (or Counter[ReviewFinding.severity.__class__] if
that helps type checkers) and adjust any imports/annotations so the function
signature and local variable use Counter[EnumFindingSeverity] (refer to
_build_findings_table, ReviewFinding, and _SEVERITY_ORDER) to provide precise
static typing without changing runtime behavior.
In `@src/omnimarket/nodes/node_pr_review_bot/models/models.py`:
- Around line 50-63: Add a model-level validator to DiffHunk that ensures
end_line >= start_line: implement a pydantic v2 model validator (e.g.,
`@model_validator`) on the DiffHunk class which checks the start_line and end_line
fields and raises a clear ValueError if end_line < start_line, so invalid hunks
are rejected during validation; keep the existing model_config and field types
untouched and update the validation error message to reference
DiffHunk/start_line/end_line for clarity.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3f716cc-9b0b-4e77-bbf1-a1f8492b02e0
📒 Files selected for processing (12)
.pre-commit-config.yamlsrc/omnimarket/nodes/node_build_loop_orchestrator/README.mdsrc/omnimarket/nodes/node_pr_review_bot/__init__.pysrc/omnimarket/nodes/node_pr_review_bot/contract.yamlsrc/omnimarket/nodes/node_pr_review_bot/handlers/__init__.pysrc/omnimarket/nodes/node_pr_review_bot/handlers/handler_fsm.pysrc/omnimarket/nodes/node_pr_review_bot/handlers/handler_report_poster.pysrc/omnimarket/nodes/node_pr_review_bot/metadata.yamlsrc/omnimarket/nodes/node_pr_review_bot/models/__init__.pysrc/omnimarket/nodes/node_pr_review_bot/models/models.pysrc/omnimarket/nodes/node_pr_review_bot/node.pysrc/omnimarket/nodes/node_ticket_pipeline/README.md
11db0b1 to
33e5cf3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/omnimarket/nodes/node_pr_review_bot/handlers/handler_report_poster.py`:
- Around line 187-209: The constructor for HandlerReportPoster currently
defaults findings and thread_states to empty tuples which allows posting
misleading summaries; change the API to require these values (remove the default
values in __init__) or add a precondition check at the start of post_summary
that compares verdict aggregates (e.g., ReviewVerdict.total_findings and any
thread counts on verdict) against self._findings and self._thread_states and
raise an exception or abort when they are inconsistent; implement the check in
post_summary (before calling build_summary_comment) referencing self._findings,
self._thread_states, and the incoming verdict to fail fast and prevent posting
an incorrect summary.
- Around line 123-166: The build_summary_comment function omits the
model-authored narrative stored on ReviewVerdict (verdict.summary); update
build_summary_comment to include verdict.summary (e.g., insert a Summary section
or append the text after meta) when verdict.summary is non-empty so the rendered
PR comment shows the human-readable review summary along with the existing
header/meta/verdict_detail/findings and thread sections; reference
build_summary_comment and the ReviewVerdict object (verdict.summary) and ensure
you skip or omit the Summary section if verdict.summary is falsy.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 457b13f9-e34b-4c41-82d6-be0fd7e0bdc1
📒 Files selected for processing (3)
.pre-commit-config.yamlsrc/omnimarket/nodes/node_pr_review_bot/handlers/__init__.pysrc/omnimarket/nodes/node_pr_review_bot/handlers/handler_report_poster.py
✅ Files skipped from review due to trivial changes (2)
- .pre-commit-config.yaml
- src/omnimarket/nodes/node_pr_review_bot/handlers/init.py
33e5cf3 to
9edf3a6
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/omnimarket/nodes/node_pr_review_bot/handlers/handler_report_poster.py (1)
73-77: Consider usingCounter[EnumFindingSeverity]for type precision.The counter keys are
EnumFindingSeverityvalues, not plain strings. While this works at runtime (StrEnum is a str subtype), the type hint could be more precise for mypy strict mode.♻️ Proposed fix
def _build_findings_table(findings: tuple[ReviewFinding, ...]) -> str: """Return a markdown table summarising finding counts by severity.""" - counts: Counter[str] = Counter() + counts: Counter[EnumFindingSeverity] = Counter() for f in findings: counts[f.severity] += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/omnimarket/nodes/node_pr_review_bot/handlers/handler_report_poster.py` around lines 73 - 77, The Counter used in _build_findings_table currently types keys as str but the keys are EnumFindingSeverity values from ReviewFinding.severity; change the type of counts to Counter[EnumFindingSeverity] (or Counter[ReviewFinding.severity's enum type]) so static checkers like mypy treat keys precisely rather than as plain strings—update the annotation for counts and any related type imports/usages to reference EnumFindingSeverity and keep the runtime logic unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/omnimarket/nodes/node_pr_review_bot/handlers/handler_report_poster.py`:
- Around line 73-77: The Counter used in _build_findings_table currently types
keys as str but the keys are EnumFindingSeverity values from
ReviewFinding.severity; change the type of counts to
Counter[EnumFindingSeverity] (or Counter[ReviewFinding.severity's enum type]) so
static checkers like mypy treat keys precisely rather than as plain
strings—update the annotation for counts and any related type imports/usages to
reference EnumFindingSeverity and keep the runtime logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08670cf9-0b8b-475e-a49c-c03039709426
📒 Files selected for processing (3)
.pre-commit-config.yamlsrc/omnimarket/nodes/node_pr_review_bot/handlers/__init__.pysrc/omnimarket/nodes/node_pr_review_bot/handlers/handler_report_poster.py
✅ Files skipped from review due to trivial changes (1)
- .pre-commit-config.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- src/omnimarket/nodes/node_pr_review_bot/handlers/init.py
…ry comments Implements ProtocolReportPoster from handler_fsm.py. Aggregates findings by severity and thread resolution status into a markdown table + verdict badge, then posts via ProtocolGitHubBridge. Dry-run mode logs instead of posting. Also fixes pre-existing no-hardcoded-topics hook failures on generated registry files (claude_code.yaml), the lint script itself, and structured_logger.py by adding per-file exclusions to .pre-commit-config.yaml.
- contract.yaml: fix fsm_transition handler module (handler_pr_review_bot -> handler_fsm) - contract.yaml: fix input/output model references (ReviewRequest/ReviewVerdict in models.models) - handler_report_poster: include verdict.summary in rendered PR comment when non-empty - handler_report_poster: make findings/thread_states required; add precondition guard in post_summary
75ca9a9 to
e837594
Compare
omnimarket carried the largest alert backlog in the org (74 of the 141 open org-wide). 71 are closed here; the remaining 3 are deferred with a stated reason rather than forced. ## Where the alerts actually live (70 of 74 are NOT in the shipped package) Only 4 alerts are in the root `uv.lock`. The other 70 are in `experiments/adk_eval/track_a_adk/uv.lock` — a standalone, generated ADK subproject with its own pyproject, venv and build backend, which this repo's own pyproject.toml (lines 762-768) documents as "not importable from the omnimarket package root". Nothing in .pre-commit-config.yaml or .github/workflows references that subtree, so its lock is scanned by Dependabot but validated by nothing. ## Root uv.lock (2 alerts closed, 3 deferred) - h2 4.3.0 -> 4.4.1 (+ hpack 4.1.0 -> 4.2.0) — CVE-2026-71554, alert #119 DEFERRED, not forced — transformers (#120 medium, #121 high, #122 high): the fix requires transformers 5.x, and resolving it is not a version bump. `uv lock --upgrade-package 'transformers>=5.5.0'` DOWNGRADES optimum 2.1.0 -> 1.17.1, REMOVES optimum-onnx entirely, and pulls 14 new packages (datasets, evaluate, pandas, pyarrow, sentencepiece, typer, xxhash, ...). transformers is transitive via adaptive-classifier / optimum / optimum-onnx and has no direct import in src/. That is a dependency-stack migration, not a security patch, so it is tracked as its own line on OMN-17184 instead of being smuggled into this PR. ## experiments/adk_eval/track_a_adk (70 alerts closed) Full `uv lock --upgrade` of the isolated subproject, closing every alert in it, including 8 criticals/highs in litellm and the aiohttp, starlette, nltk, sqlparse, cryptography, mcp, pyjwt, python-multipart, urllib3, joserfc, authlib, httplib2, Mako, idna and pydantic-settings chains. One constraint change was required: pytest was capped at `<9.0.0` and pytest-asyncio at `<1.0.0`, which made alert #4 (CVE-2025-71176, needs pytest >= 9.0.3) unreachable. Both caps were lifted together (pytest >=9.0.3,<10; pytest-asyncio >=1.3.0,<2) because pytest-asyncio 0.x itself pins pytest <9 — lifting only the pytest cap fails to resolve. Verification: `uv lock --check` clean on both lockfiles. The subproject `uv sync --all-extras` resolves and installs. Its suite runs 2 failed / 12 passed — the 2 failures are `tests/integration/test_server_e2e.py` (::test_chat_stream asserts 404, ::test_collect_feedback connection error) and are PRE-EXISTING: the identical 2-failed/12-passed result reproduces on unmodified origin/dev in a separate baseline worktree. They need a live server that is not running locally; neither is dependency-related. Ticket: OMN-17184
Summary
HandlerReportPostersatisfyingProtocolReportPosterfromhandler_fsm.pyProtocolGitHubBridge— handler stays unit-testable without network accessHandlerReportPoster,ProtocolGitHubBridge, andbuild_summary_commentfromhandlers/__init__.pyno-hardcoded-topicshook failures onclaude_code.yaml(generated event registry),lint_no_hardcoded_topics.py(the detector itself), andstructured_logger.pyby adding per-file excludes to.pre-commit-config.yamlTest plan
HandlerReportPosterfromomnimarket.nodes.node_pr_review_bot.handlersProtocolGitHubBridgeand verifypost_summarycallspost_pr_commentwith expected markdown bodypost_pr_commentbuild_summary_commentrenders verdict badge, findings table, and thread table correctlypre-commit run --all-filespasses (all hooks green)Summary by CodeRabbit
New Features
Chores