From 4cc337de8e02e5f54bbb9240c34c6466f10500a0 Mon Sep 17 00:00:00 2001 From: Kadir Eren Altintas Date: Mon, 21 Sep 2026 13:39:31 +0300 Subject: [PATCH] fix: publishing planner no longer omits high-confidence correctness/security findings by severity alone Production evidence for PR #57 head 44b9ec0: the reviewer accepted a real, critic-verified finding (category=correctness, confidence=high, severity=low) for the PR #57 swapped-counts fixture, but publication still reported status=skipped_no_findings with omitted=1 and wrote nothing to GitHub. Root cause: PublicationPlanner.build_plan's severity-threshold gate (step 1 of the funnel) omitted any finding below config.min_severity outright, before ever attempting diff mapping or summary placement -- with the production default min_severity=MEDIUM, a real, high-confidence finding assigned severity=LOW never even entered the mapping/summary funnel, regardless of how strong its evidence was. Fix: severity and confidence are orthogonal signals (severity = how bad, confidence = how sure). A finding in one of the two specialist categories (correctness, security) with Confidence.HIGH now bypasses the severity floor and enters the normal funnel like any other eligible finding -- it can still land inline (if mappable), summary-only (otherwise), or get capped to omitted by max_inline_comments/ max_summary_findings exactly like before. This is a narrow, category+confidence-scoped carve-out, not a change to min_severity itself -- every other finding (lower confidence, or outside the two specialist categories) is still governed by the configured threshold exactly as before, so existing noise controls and spam protections are unchanged. Also adds structured, safe per-omitted-finding logging (review_publish_finding_omitted: finding id, severity, category, omission reason only -- never title/message/reasoning/suggested_fix) so a future silent-omission gap is observable without a DB query. Regression coverage: a planner-level fixture reproducing the exact PR #57 finding shape, plus an end-to-end dry-run test through the real Phase 5 pipeline, both asserting the finding lands inline or summary-only, never omitted by severity alone. Two scope-guard tests confirm the bypass stays narrow (still omitted at medium confidence, still omitted for a high-confidence non-specialist category). Co-Authored-By: Claude Sonnet 5 --- patchfrog/publishing/planner.py | 45 ++++++++- patchfrog/publishing/service.py | 27 ++++++ .../test_publishing_service_dry_run.py | 33 +++++++ tests/unit/test_publishing_planner.py | 95 +++++++++++++++++++ 4 files changed, 197 insertions(+), 3 deletions(-) diff --git a/patchfrog/publishing/planner.py b/patchfrog/publishing/planner.py index 545de62..e65a0d2 100644 --- a/patchfrog/publishing/planner.py +++ b/patchfrog/publishing/planner.py @@ -12,7 +12,24 @@ Selection funnel, most to least preferred: -1. Findings below ``config.min_severity`` are omitted outright. +1. Findings below ``config.min_severity`` are omitted outright -- + *unless* the finding is high-confidence evidence from one of the two + specialist roles (:class:`~patchfrog.analysis.domain.FindingCategory.CORRECTNESS` + or ``SECURITY``) with :class:`~patchfrog.analysis.domain.Confidence.HIGH`. + Severity and confidence are orthogonal (see + :class:`patchfrog.analysis.domain.Confidence`'s own docstring): severity + is how *bad* an issue is, confidence is how *sure* the detection is. + ``min_severity`` exists to cut low-priority noise, never to discard a + proposal that has already survived reviewer proposal, deterministic + validation, confidence aggregation (:mod:`patchfrog.review.confidence` + -- critic-downgraded when a critic verdict exists, reviewer-reported + otherwise, e.g. when the critic call itself failed and fell back per + :mod:`patchfrog.review.orchestration`'s documented fail-open policy) + and dedup as a proven defect (see + :meth:`~patchfrog.publishing.planner.PublicationPlanner.build_plan`'s + ``_bypasses_severity_floor`` use below) -- such a finding still goes + through steps 2-4 exactly like any other eligible finding, so it can + still end up ``OMITTED`` by a cap, just never by severity alone. 2. Findings that map to a real diff line (see :mod:`patchfrog.publishing.diff_mapper`) are ranked (severity desc, confidence desc, then stable path/line) and the top @@ -34,7 +51,7 @@ from collections.abc import Mapping, Sequence from uuid import UUID -from patchfrog.analysis.domain import Confidence, Severity +from patchfrog.analysis.domain import Confidence, FindingCategory, Severity from patchfrog.diff.models import DiffFile from patchfrog.domain.pull_request import ChangedFile from patchfrog.publishing.body import ( @@ -74,6 +91,28 @@ def _meets_min_severity(severity: Severity, *, minimum: Severity) -> bool: return _SEVERITY_RANK[severity] >= _SEVERITY_RANK[minimum] +#: The two specialist roles (:mod:`patchfrog.review.agents.roles`) whose +#: findings represent hard, reviewer-proposed-then-critic-verified +#: evidence of an actual defect rather than a style/maintainability +#: opinion -- see the module docstring's funnel step 1. +_HARD_EVIDENCE_CATEGORIES = frozenset({FindingCategory.CORRECTNESS, FindingCategory.SECURITY}) + + +def _bypasses_severity_floor(finding: PublishableFinding) -> bool: + """A finding that survived reviewer proposal, validation, confidence + aggregation, and dedup with :class:`Confidence.HIGH` in one of the two + specialist categories is proven evidence of a real defect, not noise -- + ``config.min_severity`` must never be the sole reason it is discarded + with zero visibility (see the funnel docstring above). This is a + narrow, category+confidence-scoped carve-out, not a change to the + configured threshold itself: everything else is still governed by + ``config.min_severity`` exactly as before, and a bypassing finding is + still fully subject to steps 2-4 (mapping, caps) below -- it can still + end up ``OMITTED`` by a cap, just never by severity alone.""" + + return finding.confidence is Confidence.HIGH and finding.category in _HARD_EVIDENCE_CATEGORIES + + class PublicationPlanner: """Stateless; safe to reuse across calls.""" @@ -176,7 +215,7 @@ def build_plan( ) ) continue - if not _meets_min_severity(finding.severity, minimum=config.min_severity): + if not _meets_min_severity(finding.severity, minimum=config.min_severity) and not _bypasses_severity_floor(finding): omitted.append(self._to_comment(finding, snapshot, PublicationDisposition.OMITTED, position=None, reason="below minimum severity threshold")) continue eligible.append(finding) diff --git a/patchfrog/publishing/service.py b/patchfrog/publishing/service.py index 423ba38..2e53a97 100644 --- a/patchfrog/publishing/service.py +++ b/patchfrog/publishing/service.py @@ -61,6 +61,7 @@ from patchfrog.publishing.config import PublicationConfig from patchfrog.publishing.domain import ( DiffSide, + PublishableFinding, ReviewInputSnapshot, ReviewPublicationComment, ReviewPublicationMode, @@ -235,6 +236,7 @@ async def publish( ), ) + self._log_omitted_findings(publication_id, findings, plan) await self._persist_plan_comments(publication_id, plan) if plan.status is ReviewPublicationStatus.STALE: @@ -325,6 +327,31 @@ async def publish( ) return self._result_from_model(model, reconciled=False, errors=()) + @staticmethod + def _log_omitted_findings( + publication_id: uuid.UUID, findings: list[PublishableFinding], plan: ReviewPublicationPlan + ) -> None: + """One structured log line per finding the planner discarded to + ``OMITTED`` -- finding id, severity/category, and the planner's own + fixed-vocabulary omission reason only (e.g. "below minimum + severity threshold", "exceeded max_summary_findings cap"). Never + the finding's title/message/reasoning_summary/suggested_fix + (repository/provider-derived free text) -- see + :class:`patchfrog.publishing.domain.PublishableFinding`.""" + + if not plan.omitted: + return + category_by_id = {f.finding_id: f.category.value for f in findings} + for comment in plan.omitted: + logger.info( + "review_publish_finding_omitted", + publication_id=str(publication_id), + finding_id=str(comment.finding_id), + severity=comment.severity.value, + category=category_by_id.get(comment.finding_id, "unknown"), + omission_reason=comment.reason, + ) + async def _persist_plan_comments(self, publication_id: uuid.UUID, plan: ReviewPublicationPlan) -> None: all_comments: list[ReviewPublicationComment] = [ *plan.inline_comments, *plan.summary_only, *plan.omitted, *plan.already_reported, diff --git a/tests/integration/test_publishing_service_dry_run.py b/tests/integration/test_publishing_service_dry_run.py index 4fbbc80..3dc2205 100644 --- a/tests/integration/test_publishing_service_dry_run.py +++ b/tests/integration/test_publishing_service_dry_run.py @@ -79,6 +79,39 @@ async def test_dry_run_plans_but_never_writes_to_github( assert comments[0].github_comment_id is None +async def test_dry_run_high_confidence_low_severity_correctness_finding_is_published_not_omitted( + session_factory: async_sessionmaker[AsyncSession], tmp_path: Path +) -> None: + """Regression: PR #57 production evidence showed a real, critic- + verified, high-confidence correctness finding (severity=low) being + discarded outright by the default min_severity=MEDIUM floor before + ever being considered for inline/summary placement -- GitHub received + nothing despite the review run itself reporting accepted_count=1. See + patchfrog.publishing.planner._bypasses_severity_floor.""" + + reviewed = await setup_reviewed_pull_request( + session_factory, + full_name="test/pr-57-severity-floor", + changed_lines=[14], + response_factory=lambda req: scripted_findings_response( + [finding_json(severity="low", confidence="high", category="correctness")] + ), + tmp_root=tmp_path, + ) + assert len(reviewed.findings) == 1 + + publisher = FakeReviewPublisher( + pull_request=_pr_metadata(number=reviewed.pull_request_number, head_sha=reviewed.commit_sha), + changed_files=reviewed.changed_files, + ) + service = ReviewPublicationService(session_factory=session_factory, publisher=publisher) + result = await service.publish(review_run_id=reviewed.review_run_id, mode=ReviewPublicationMode.DRY_RUN) + + assert result.status is ReviewPublicationStatus.DRY_RUN + assert result.omitted == 0 + assert result.planned_inline + result.summary_only == 1 + + async def test_dry_run_with_no_findings_is_skipped( session_factory: async_sessionmaker[AsyncSession], tmp_path: Path ) -> None: diff --git a/tests/unit/test_publishing_planner.py b/tests/unit/test_publishing_planner.py index 97c6440..d5b8306 100644 --- a/tests/unit/test_publishing_planner.py +++ b/tests/unit/test_publishing_planner.py @@ -235,6 +235,101 @@ def test_max_summary_findings_cap_produces_omitted() -> None: assert all("cap" in c.reason for c in plan.omitted) +def test_high_confidence_correctness_finding_below_severity_floor_is_not_silently_omitted() -> None: + """A finding that already survived reviewer proposal + critic + verification + dedup with Confidence.HIGH in a specialist category + (correctness/security) must not be discarded by config.min_severity + alone -- see patchfrog.publishing.planner's module docstring, funnel + step 1, and _bypasses_severity_floor.""" + + finding = _finding(line=5, severity=Severity.LOW, confidence=Confidence.HIGH) + plan = _build([finding], config=PublicationConfig()) # production default: min_severity=MEDIUM + + assert plan.omitted == () + assert len(plan.inline_comments) + len(plan.summary_only) == 1 + + +def test_medium_confidence_finding_below_severity_floor_is_still_omitted() -> None: + """The severity-floor bypass is scoped to Confidence.HIGH only -- + noise controls for lower-confidence findings are unaffected.""" + + finding = _finding(line=5, severity=Severity.LOW, confidence=Confidence.MEDIUM) + plan = _build([finding], config=PublicationConfig()) + + assert len(plan.omitted) == 1 + assert plan.omitted[0].reason == "below minimum severity threshold" + + +def test_high_confidence_non_specialist_category_below_severity_floor_is_still_omitted() -> None: + """The bypass only applies to the two specialist roles (correctness, + security) -- a high-confidence style/maintainability nit is still + governed by config.min_severity exactly as before.""" + + finding = PublishableFinding( + finding_id=uuid.uuid4(), + title="nit", + message="nit message", + category=FindingCategory.STYLE, + severity=Severity.LOW, + confidence=Confidence.HIGH, + file_path="src/billing.py", + start_line=5, + end_line=5, + reasoning_summary="because", + ) + plan = _build([finding], config=PublicationConfig()) + + assert len(plan.omitted) == 1 + + +def test_pr_57_swapped_counts_finding_is_not_silently_omitted() -> None: + """Regression fixture: production evidence for PR #57 head + 44b9ec0a0c68c1073f62e92b4369619bc84e46e7 showed the reviewer accepting + a real, critic-verified finding (category=correctness, confidence=high, + severity=low) for the intentional swapped inline/summary-only counts in + format_summary_body -- but the default min_severity=MEDIUM floor + discarded it outright before it was ever considered for inline/summary + placement, so GitHub received nothing despite review_run_completed + reporting accepted_count=1 (status=skipped_no_findings, omitted=1). + An accepted, high-confidence correctness finding must land inline or + summary-only, never omitted by severity alone.""" + + changed_files, diff_files = _whole_file_changed_files(path="patchfrog/publishing/body.py") + finding = PublishableFinding( + finding_id=uuid.uuid4(), + title="Inline/summary-only finding counts are swapped in the review summary body", + message=( + "format_summary_body labels len(summary_only_findings) as " + "'Published inline' and len(inline_findings) as 'Summary-only' -- the two are transposed." + ), + category=FindingCategory.CORRECTNESS, + severity=Severity.LOW, + confidence=Confidence.HIGH, + file_path="patchfrog/publishing/body.py", + start_line=9, + end_line=9, + reasoning_summary="the two len() calls are swapped relative to their labels", + suggested_fix=None, + ) + + plan = PublicationPlanner().build_plan( + publication_id=uuid.uuid4(), + snapshot=_snapshot(), + findings=[finding], + changed_files=changed_files, + diff_files=diff_files, + config=PublicationConfig(), # production defaults, including min_severity=MEDIUM + mode=ReviewPublicationMode.DRY_RUN, + current_head_sha=_HEAD_SHA, + ) + + assert plan.omitted == () + assert plan.status in (ReviewPublicationStatus.DRY_RUN, ReviewPublicationStatus.PLANNED) + assert len(plan.inline_comments) + len(plan.summary_only) == 1 + placed = plan.inline_comments[0] if plan.inline_comments else plan.summary_only[0] + assert placed.finding_id == finding.finding_id + + def test_selection_prefers_higher_severity_under_inline_cap() -> None: low = _finding(line=2, severity=Severity.LOW, title="low") critical = _finding(line=3, severity=Severity.CRITICAL, title="critical")