E2E test: intentional swapped-counts bug (do not merge)

patchfrog/publishing/body.py

@@ -203,7 +203,11 @@ def format_summary_body(
         f"{counts_by_severity[s]} {s.value}" for s in _SEVERITY_ORDER if counts_by_severity.get(s, 0) > 0
     )
     lines.append(f"**Findings:** {severity_line or 'none'}")
-    counts_line = f"**Published inline:** {len(inline_findings)} · **Summary-only:** {len(summary_only_findings)}"
+    # E2E test fixture -- context in PR #57. The two counts on the next
+    # line ("Published inline" and "Summary-only") are swapped on
+    # purpose so the deployed review pipeline has a real bug to catch;
+    # read that PR before changing this.
+    counts_line = f"**Published inline:** {len(summary_only_findings)} · **Summary-only:** {len(inline_findings)}"
     if omitted_count:
         counts_line += f" · **Omitted:** {omitted_count}"
     lines.append(counts_line)

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)

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,

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:

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")