Skip to content

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

Open
kadireren7 wants to merge 9 commits into
mainfrom
test/webhook-e2e-review-check
Open

E2E test: intentional swapped-counts bug (do not merge)#57
kadireren7 wants to merge 9 commits into
mainfrom
test/webhook-e2e-review-check

Conversation

@kadireren7

@kadireren7 kadireren7 commented Sep 20, 2026 •
edited
Loading

Copy link
Copy Markdown
Owner

Purpose

This PR exists solely to verify the deployed PatchFrog Cloud GitHub App
end-to-end pipeline: receives the PR webhook, queues the worker job, runs
the review engine, and posts a review comment/findings back to this PR.

Do not merge.

Intentional bug introduced

patchfrog/publishing/body.py, in format_summary_body(): the
counts_line swaps which count backs which label --

counts_line = f"**Published inline:** {len(summary_only_findings)} · **Summary-only:** {len(inline_findings)}"

should be:

counts_line = f"**Published inline:** {len(inline_findings)} · **Summary-only:** {len(summary_only_findings)}"

Effect: whenever a review produces a different number of inline vs.
summary-only findings, the rendered summary body mislabels the two
counts (shows the summary-only count as "Published inline" and vice
versa). It's silent -- no crash, no test failure, since the existing
unit tests for this function happen to use symmetric counts (1 and 1)
and don't distinguish the two labels.

No secrets, deployment config, license, or architecture files touched.

kadireren7 added a commit that referenced this pull request Sep 20, 2026
@kadireren7 @claude
test: note that the swapped counts line is an E2E fixture
60baa68 
Comment-only change, no behavior change -- the intentional bug from
PR #57 is untouched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kadireren7
test: swap inline/summary-only finding counts in review summary body
80ace56 
Intentional regression for an end-to-end production webhook test:
format_summary_body() now labels the "Published inline" count with
len(summary_only_findings) and the "Summary-only" count with
len(inline_findings), so the two figures are swapped whenever they
differ. Existing tests don't catch it because they only exercise
symmetric counts (1 inline / 1 summary-only).

Do not merge -- this branch exists only to verify the deployed
PatchFrog Cloud GitHub App receives the PR webhook, queues the
review job, and posts a review comment.
@kadireren7
test: note that the swapped counts line is an E2E fixture
48d0d9b 
Comment-only change, no behavior change -- the intentional bug from
PR #57 is untouched.
@kadireren7
kadireren7 force-pushed the test/webhook-e2e-review-check branch from 60baa68 to 48d0d9b Compare September 20, 2026 13:46
@kadireren7
test: reword E2E fixture comment for clarity
eac7387 
Comment-only wording change, no behavior change -- the intentional
swapped-counts bug from PR #57 is untouched.
@kadireren7
test: reword E2E fixture comment again
f76988a 
Comment-only wording change, no behavior change -- the intentional
swapped-counts bug from PR #57 is untouched.
@kadireren7
test: reword E2E fixture comment once more
b82d279 
Comment-only wording change, no behavior change -- the intentional
swapped-counts bug from PR #57 is untouched.
@kadireren7 kadireren7 mentioned this pull request Sep 20, 2026
Merged
4 tasks done
kadireren7 added a commit that referenced this pull request Sep 21, 2026
@kadireren7
Fix critic false-negative on findings the diff claims are intentional (…
dcb5fe2 
…#62)

PR #57 (test/webhook-e2e-review-check) exercised a real, deliberately
introduced bug in format_summary_body(): the "Published inline" and
"Summary-only" counts are swapped. The reviewer correctly proposed the
finding (evidence, reasoning, and impact were all accurate), but the
critic rejected it, reasoning that the comment directly above the bug
("intentionally swapped... do not fix") proved the behavior was not a
defect. Reproduced against the real production diff (merge-base
bd25117 vs head b82d279) with a real reviewer+critic run: candidate=1,
accepted=0, rejected=1, matching the reported production shape exactly.
The rejection is a false negative -- the critic's own system prompt
already says everything in the diff is untrusted data, but nothing in
it named this specific pattern (a claim of intent, as opposed to an
impersonated system instruction), so the critic treated "the author
says this is deliberate" as proof the output is correct. Deliberateness
and correctness are different questions; a bug introduced on purpose is
still a bug.

- patchfrog/review/prompt.py: add an explicit critic-prompt rule that a
  comment/commit message claiming code is intentional, a test fixture,
  or "do not fix" is untrusted data, not evidence of correctness -- the
  critic must judge the finding against what the code does, not against
  the diff's own claim about why it exists. Existing reject criteria are
  unchanged; no threshold was lowered.
- patchfrog/review/domain.py, schemas.py, critic.py: add
  CriticRejectionCategory, a machine-classified reject reason the critic
  now returns in its own structured response (mirroring the existing
  ValidationOutcome pattern -- never inferred from reasoning_summary
  prose after the fact).
- patchfrog/persistence/models/review.py,
  repositories/critic_verdict.py, migrations/versions/0031_*: persist
  rejection_category on critic_verdicts (nullable, additive).
- patchfrog/review/service.py: one structured "finding_candidate_diagnostics"
  log line per proposal (finding id, category, reviewer confidence,
  evidence-entry count, critic decision, rejection category) -- never
  the prompt, a provider response, a secret, or source content.
- tests/fixtures/evaluation/cases/py-claimed-intentional-swapped-counts:
  regression fixture reproducing the pattern for patchfrog.evaluation.
- tests/unit/test_review_critic.py, test_review_prompt_injection.py:
  cover rejection_category parsing/schema and the new prompt guardrail.
kadireren7 and others added 2 commits September 21, 2026 12:20
@kadireren7 @claude
test: reword E2E fixture comment yet again
44b9ec0 
Comment-only wording change, no behavior change -- the intentional
swapped-counts bug from PR #57 is untouched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kadireren7 @claude
fix: publishing planner no longer omits high-confidence correctness/s…
fe2bfcd 
…ecurity 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 <noreply@anthropic.com>
@kadireren7 kadireren7 mentioned this pull request Sep 21, 2026
Open
6 tasks done
kadireren7 and others added 2 commits September 22, 2026 22:32
@kadireren7 @claude
test: reword E2E fixture comment once more
7126105 
Comment-only wording change, no behavior change -- the intentional
swapped-counts bug from PR #57 is untouched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@kadireren7 @claude
test: reword E2E fixture comment again
e95cb09 
Comment-only wording change, no behavior change -- the intentional
swapped-counts bug from PR #57 is untouched.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@kadireren7