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