-
Notifications
You must be signed in to change notification settings - Fork 0
docs(review-pr): Pass C v1.1 — fenced-body heuristic + doc refinements (4.5.0) #44
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from 19 commits
Commits
Show all changes
22 commits
Select commit
Hold shift + click to select a range
de76536
chore(reviewer-rigor): BP audit + release SKILL YAML fix + version 4.2.0
claude ae2bdda
feat(schema): add call_tree category + CALL- id prefix for reviewer c…
claude 22bdcae
Merge branch 'feat/reviewer-rigor-housekeeping' into feat/reviewer-rigor
claude e6975ee
feat(reviewer-rigor): call-tree inspection methodology + ephemeral-ID…
claude b76bf72
fix(reviewer-rigor): address Copilot review on PR #41
claude f10308d
feat(git-and-github): PR bodies lead with "Why this PR exists" rationale
claude ead92c6
fix(report-pipeline): derive severity on-the-fly + schema 3.1.0 addit…
claude a631c98
fix(report-pipeline): renderer on-the-fly severity + permalink @{u} +…
claude aa89655
style(consolidate): drop unused build_severity_stats import (ruff F401)
claude c9f0ccc
feat(review-pr): Pass C v1.1 doc heuristics + regression tests (4.5.0)
claude d7c93c8
Merge branch 'main' into feat/report-pipeline-severity
claude c107c11
fix(report-pipeline): address Copilot review on PR #42
claude ec9ad84
fix(report-pipeline): address second Copilot pass on PR #42
claude e33231d
Merge branch 'feat/report-pipeline-severity' into feat/pr-why-template
claude 67c1ce7
Merge remote-tracking branch 'origin/main' into feat/pr-why-template
claude e24a823
style(test): black-format test_pr_body_template.py
claude 81e54fc
Merge branch 'feat/pr-why-template' into feat/passc-v1.1
claude 2cf5ce8
style(test): ruff E741 + black on test_review_pr_passc.py
claude 5447c29
Merge remote-tracking branch 'origin/main' into feat/passc-v1.1
claude b941251
fix(review-pr): correct Pass C informational-finding severity floats …
claude f4dfd62
docs(changelog): correct PR-body-unparseable finding band INFO -> LOW
claude a9cfcf3
docs(review-pr): correct band-math wording in Pass C scope note + tes…
claude File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,46 @@ | ||
| # Pass C fixture — fully-fenced PR body | ||
|
|
||
| Exercises the **fenced-body unwrap** heuristic: the entire PR body is wrapped in | ||
| a single code fence, so the column-0-anchored Summary/Out-of-scope regexes match | ||
| nothing until the outer fence is stripped and the content dedented. | ||
|
|
||
| ## PR Title | ||
|
|
||
| ``` | ||
| feat(resolver): add LRU caching layer | ||
| ``` | ||
|
|
||
| ## PR Body (raw — wholly fenced) | ||
|
|
||
| The body as received from the API is a single fenced block. The `## Summary` | ||
| and `## Out of scope` headers below sit INSIDE the fence: | ||
|
|
||
| ``` | ||
| # Add caching layer to the resolver | ||
|
|
||
| ## Summary | ||
|
|
||
| - Add an LRU cache to `Resolver::lookup` | ||
| - Expose `CacheConfig` with a configurable capacity | ||
|
|
||
| ## Out of scope | ||
|
|
||
| - Distributed cache backends (separate PR) | ||
| ``` | ||
|
|
||
| ## Expected Pass C behaviour | ||
|
|
||
| After the fenced-body unwrap (strip outer fence + dedent), the `## Summary` | ||
| header becomes visible at column 0 and the two bullets are extracted normally; | ||
| the out-of-scope item is enforced against the diff. No unparseable-body finding | ||
| is emitted because the dedent exposes a real Summary header. The | ||
| `expected_finding_count` below reflects the documented dedent rule, not an | ||
| executed audit (no diff is supplied). | ||
|
|
||
| <!-- expected: { | ||
| "expected_finding_count": 0, | ||
| "title_alignment": "aligned", | ||
| "summary_alignment": "aligned", | ||
| "out_of_scope": "aligned", | ||
| "required_sections": ["## PR Title", "## PR Body (raw — wholly fenced)"] | ||
| } --> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,168 @@ | ||
| """Doc-regression tests for review-pr Pass C v1.1 (TODOs 8c17d019 + 4463333d). | ||
|
|
||
| Two flavours of test live here: | ||
|
|
||
| 1. **Grep-assert** that ``skills/review-pr/SKILL.md`` still documents each Pass C | ||
| v1.1 rule. These pin the *wording* so a future SKILL edit can't silently drop | ||
| a rule without a failing test. | ||
| 2. **Parser-mirror** that re-implements the documented fenced-body unwrap exactly | ||
| as the SKILL describes and checks that, against a fully-fenced fixture, the | ||
| unwrap exposes the ``## Summary`` header that the column-0 regex would | ||
| otherwise miss. Mirrors how ``tests/test_check_pr_comments_permalink.py`` | ||
| mirrors a documented producer rule. | ||
| """ | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import re | ||
| from pathlib import Path | ||
|
|
||
| ROOT = Path(__file__).resolve().parent.parent | ||
| SKILL = ROOT / "skills" / "review-pr" / "SKILL.md" | ||
| FIXTURES = Path(__file__).resolve().parent / "fixtures" / "pr-promises" | ||
|
|
||
|
|
||
| def _skill_text() -> str: | ||
| return SKILL.read_text(encoding="utf-8") | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Grep-assert: documented rules survive | ||
| # --------------------------------------------------------------------------- | ||
| class TestSkillDocumentsPassCRules: | ||
| def test_fenced_body_dedent_step(self): | ||
| text = _skill_text() | ||
| assert "Fenced-body unwrap" in text | ||
| assert "dedent" in text | ||
|
|
||
| def test_pr_body_unparseable_fallback(self): | ||
| text = _skill_text() | ||
| assert "PR body unparseable" in text | ||
|
|
||
| def test_clean_pass_empty_findings_plus_info(self): | ||
| text = _skill_text() | ||
| assert "findings: []" in text | ||
| assert "PR self-description verified" in text | ||
|
|
||
| def test_undocumented_change_keyword_overlap(self): | ||
| text = _skill_text() | ||
| assert "keyword overlap" in text | ||
| # The 50-LOC trigger is retained alongside the precise definition. | ||
| assert "50 LOC" in text | ||
|
|
||
| def test_summary_heading_precedence_order(self): | ||
| text = _skill_text() | ||
| assert "Summary-heading precedence" in text | ||
| # Precedence order is documented as Summary > ### Summary > What changed. | ||
| idx_h2 = text.find("`## Summary` > `### Summary` > `## What changed`") | ||
| assert idx_h2 != -1 | ||
|
|
||
| def test_compound_title_majority_hits(self): | ||
| text = _skill_text() | ||
| assert "Majority-hits rule" in text | ||
|
|
||
| def test_section_verdict_wiring(self): | ||
| text = _skill_text() | ||
| assert "finding_section.verdict" in text | ||
| for kw in ("PASS", "FAIL", "NEEDS_REVIEW"): | ||
| assert kw in text | ||
|
|
||
| def test_language_cross_ref_not_hardcoded(self): | ||
| text = _skill_text() | ||
| # The hard-coded `language: "diff"` instruction is gone; a cross-ref to | ||
| # report-format's code_snippets field reference takes its place. | ||
| assert 'Use `language: "diff"`' not in text | ||
| assert "report-format` §code_snippets" in text | ||
|
|
||
| def test_report_type_pr_audit_documented(self): | ||
| text = _skill_text() | ||
| assert 'metadata.report_type: "pr_audit"' in text | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Parser-mirror: the documented fenced-body unwrap exposes ## Summary | ||
| # --------------------------------------------------------------------------- | ||
| SUMMARY_RE = re.compile( | ||
| r"^##+\s+(?:Summary|What changed)\b", re.IGNORECASE | re.MULTILINE | ||
| ) | ||
|
|
||
| _FENCE_RE = re.compile(r"^(```+|~~~+)") | ||
|
|
||
|
|
||
| def _unwrap_fenced_body(body: str) -> str: | ||
| """Reference implementation of the documented "Fenced-body unwrap" step. | ||
|
|
||
| If the body (after leading blank lines) opens with a code fence whose | ||
| matching closing fence is the last non-blank line, strip the outer fence | ||
| and dedent the enclosed lines by their longest common leading whitespace. | ||
| Otherwise return the body unchanged. | ||
| """ | ||
| lines = body.splitlines() | ||
| # Find first non-blank line. | ||
| start = 0 | ||
| while start < len(lines) and lines[start].strip() == "": | ||
| start += 1 | ||
| if start >= len(lines): | ||
| return body | ||
| open_m = _FENCE_RE.match(lines[start]) | ||
| if not open_m: | ||
| return body | ||
| fence = open_m.group(1) | ||
| # Find last non-blank line; it must be a closing fence of the same kind. | ||
| end = len(lines) - 1 | ||
| while end > start and lines[end].strip() == "": | ||
| end -= 1 | ||
| if end <= start or not lines[end].strip().startswith(fence[0] * 3): | ||
| return body | ||
|
lklimek marked this conversation as resolved.
Outdated
|
||
| inner = lines[start + 1 : end] | ||
| # Dedent by longest common leading whitespace over non-blank lines. | ||
| indents = [len(ln) - len(ln.lstrip()) for ln in inner if ln.strip()] | ||
| pad = min(indents) if indents else 0 | ||
| return "\n".join(ln[pad:] if len(ln) >= pad else ln for ln in inner) | ||
|
|
||
|
|
||
| # A PR body that is wholly wrapped in one indented code fence — the exact shape | ||
| # the "Fenced-body unwrap" step targets. The `## Summary` header lives inside the | ||
| # fence and is indented, so the column-0 SUMMARY_RE matches nothing until the | ||
| # outer fence is stripped and the lines dedented. | ||
| WHOLLY_FENCED_BODY = ( | ||
| "```\n" | ||
| " # Add caching layer to the resolver\n" | ||
| "\n" | ||
| " ## Summary\n" | ||
| "\n" | ||
| " - Add an LRU cache to `Resolver::lookup`\n" | ||
| " - Expose `CacheConfig` with a configurable capacity\n" | ||
| "\n" | ||
| " ## Out of scope\n" | ||
| "\n" | ||
| " - Distributed cache backends (separate PR)\n" | ||
| "```" | ||
| ) | ||
|
|
||
|
|
||
| class TestFencedUnwrapExposesSummary: | ||
| def test_fenced_body_hides_header_before_unwrap(self): | ||
| # Before unwrap: the body opens with a fence and SUMMARY_RE finds nothing | ||
| # (the header is indented inside the fence). | ||
| assert WHOLLY_FENCED_BODY.lstrip().startswith("```") | ||
| assert SUMMARY_RE.search(WHOLLY_FENCED_BODY) is None | ||
|
|
||
| def test_unwrap_exposes_summary_header(self): | ||
| unwrapped = _unwrap_fenced_body(WHOLLY_FENCED_BODY) | ||
| assert not unwrapped.lstrip().startswith("```") | ||
| # After strip + dedent the header is at column 0 and SUMMARY_RE matches. | ||
| assert SUMMARY_RE.search(unwrapped) is not None | ||
|
|
||
| def test_unwrap_is_noop_for_unfenced_body(self): | ||
| # An ordinary (non-wholly-fenced) body is returned unchanged. | ||
| plain = "## Summary\n\n- did a thing\n" | ||
| assert _unwrap_fenced_body(plain) == plain | ||
|
|
||
| def test_fenced_fixture_present_and_self_describing(self): | ||
| # The committed fixture demonstrates the same wholly-fenced shape and | ||
| # carries the standard `<!-- expected -->` annotation (enforced in full | ||
| # by tests/test_pr_promises_fixtures.py). | ||
| text = (FIXTURES / "synthetic-fenced.md").read_text(encoding="utf-8") | ||
| assert "## Summary" in text | ||
| assert "<!-- expected:" in text | ||
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.