Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
188 changes: 149 additions & 39 deletions src/grippy/github_review.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

from __future__ import annotations

import hashlib
import json
import os
import re
Expand Down Expand Up @@ -172,8 +173,11 @@ def _sanitize_comment_text(text: str) -> str:
"LOW": "\U0001f535",
}

# Marker format: <!-- grippy:file:category:line -->
_GRIPPY_MARKER_RE = re.compile(r"<!-- grippy:(?P<file>[^:]+):(?P<category>[^:]+):(?P<line>\d+) -->")
# Marker format: <!-- grippy:file:category:line:hash8 -->
_GRIPPY_MARKER_RE = re.compile(
r"<!-- grippy:(?P<file>[^:]+):(?P<category>[^:]+):(?P<line>\d+)"
r"(?::(?P<disc>\w+))? -->"
)


def _sanitize_path(path: str) -> str:
Expand All @@ -182,10 +186,17 @@ def _sanitize_path(path: str) -> str:
return re.sub(r"[^a-zA-Z0-9_./ -]", "", path)


def _disc_hash(finding: Finding) -> str:
"""Build an 8-char discriminator hash from title + severity + category."""
key = f"{finding.title}:{finding.severity.value}:{finding.category.value}"
return hashlib.sha256(key.encode()).hexdigest()[:8]


def _finding_marker(finding: Finding) -> str:
"""Build an HTML comment marker for dedup — keyed on file, category, line."""
"""Build an HTML comment marker for dedup — keyed on file, category, line, hash."""
safe_file = _sanitize_path(finding.file)
return f"<!-- grippy:{safe_file}:{finding.category.value}:{finding.line_start} -->"
disc = _disc_hash(finding)
return f"<!-- grippy:{safe_file}:{finding.category.value}:{finding.line_start}:{disc} -->"


def build_review_comment(finding: Finding) -> dict[str, str | int]:
Expand All @@ -201,19 +212,26 @@ def build_review_comment(finding: Finding) -> dict[str, str | int]:
title = _sanitize_comment_text(finding.title)
description = _sanitize_comment_text(finding.description)
suggestion = _sanitize_comment_text(finding.suggestion)
evidence = _sanitize_comment_text(finding.evidence)
grippy_note = _sanitize_comment_text(finding.grippy_note)
body_lines = [
f"#### {emoji} {finding.severity.value}: {title}",
f"Confidence: {finding.confidence}%",
"",
description,
"",
f"**Suggestion:** {suggestion}",
"",
f"*\u2014 {grippy_note}*",
"",
_finding_marker(finding),
]
if evidence.strip():
body_lines.extend([f"```\n{evidence}\n```", ""])
body_lines.extend(
[
description,
"",
f"**Suggestion:** {suggestion}",
"",
f"*\u2014 {grippy_note}*",
"",
_finding_marker(finding),
]
)
return {
"path": _sanitize_path(finding.file),
"body": "\n".join(body_lines),
Expand All @@ -225,19 +243,24 @@ def build_review_comment(finding: Finding) -> dict[str, str | int]:
# --- GitHub comment fetching ---


def _parse_marker(body: str) -> tuple[str, str, int] | None:
"""Extract (file, category, line) from a grippy marker in comment body."""
def _parse_marker(body: str) -> tuple[str, str, int, str | None] | None:
"""Extract (file, category, line, disc) from a grippy marker in comment body."""
match = _GRIPPY_MARKER_RE.search(body)
if match:
return (match.group("file"), match.group("category"), int(match.group("line")))
return (
match.group("file"),
match.group("category"),
int(match.group("line")),
match.group("disc"),
)
return None


def fetch_grippy_comments(
*,
repo: str,
pr_number: int,
) -> dict[tuple[str, str, int], ThreadRef]:
) -> dict[tuple[str, str, int, str | None], ThreadRef]:
"""Fetch existing Grippy review threads from a PR via GraphQL.

Queries the ``reviewThreads`` connection on the pull request, extracting
Expand All @@ -249,8 +272,8 @@ def fetch_grippy_comments(
pr_number: Pull request number.

Returns:
Dict mapping (file, category, line) to a ThreadRef with thread
node_id and first comment body.
Dict mapping (file, category, line, disc) to a ThreadRef with thread
node_id and first comment body. disc is None for old-format markers.
"""
import json

Expand All @@ -266,7 +289,7 @@ def fetch_grippy_comments(
" } } } }"
)

result: dict[tuple[str, str, int], ThreadRef] = {}
result: dict[tuple[str, str, int, str | None], ThreadRef] = {}
cursor: str | None = None

for _page in range(20): # safety limit: max 20 pages
Expand Down Expand Up @@ -342,19 +365,24 @@ def format_summary_comment(
head_sha: str,
pr_number: int,
diff_truncated: bool = False,
summary_only_findings: list[Finding] | None = None,
policy_bypassed: bool = False,
display_capped_count: int = 0,
summary_only_count: int = 0,
) -> str:
"""Format the compact summary dashboard as an issue comment.

Args:
score: Overall review score (0-100).
verdict: PASS, FAIL, or PROVISIONAL.
finding_count: Total findings this round.
finding_count: Inline findings count.
new_count: Genuinely new findings posted this round.
resolved_count: Prior findings resolved this round.
off_diff_findings: Findings outside diff hunks (shown inline here).
head_sha: Commit SHA for this review.
pr_number: PR number for marker scoping.
diff_truncated: Whether the diff was truncated to fit context limits.
summary_only_count: Number of summary-only findings (scored but not inline).

Returns:
Formatted markdown comment body.
Expand All @@ -368,9 +396,23 @@ def format_summary_comment(
lines: list[str] = []
lines.append(f"## {status_emoji} Grippy Review \u2014 {verdict}")
lines.append("")
lines.append(f"**Score: {score}/100** | **Findings: {finding_count}**")
if summary_only_count > 0:
total = finding_count + summary_only_count
lines.append(
f"**Score: {score}/100** | **Findings: {total} total**"
f" ({finding_count} inline, {summary_only_count} summary-only)"
)
else:
lines.append(f"**Score: {score}/100** | **Findings: {finding_count}**")
lines.append("")

if policy_bypassed:
lines.append(
"> \u26a0\ufe0f **Output policy was bypassed due to an internal error."
" Findings are unfiltered.**"
)
lines.append("")
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 MEDIUM: Malformed summary-only findings rendering code style

Confidence: 90%

#### {sev_emoji} {f.severity.value}: {f_title}

description, suggestion only -- omits confidence/grippy_note

The rendering logic for 'summary_only_findings' in the summary comment diverges from the format used for normal findings and omits key fields (e.g. confidence or grippy_note). This may cause confusion/inconsistency for consumers or break downstream automation expecting uniform markdown structure.

Suggestion: Make the rendering structure for summary-only findings match the normal finding comment (including confidence % and grippy_note). Consider extracting a shared formatting helper.

— Consistency in report formatting helps prevent confusion and bugs.


if diff_truncated:
lines.append(
"> \u26a0\ufe0f **Notice:** Diff was truncated to fit context limits."
Expand All @@ -388,6 +430,10 @@ def format_summary_comment(
lines.append(f"**Delta:** {' \u00b7 '.join(parts)}")
lines.append("")

if display_capped_count > 0:
lines.append(f"> {display_capped_count} additional finding(s) omitted for brevity.")
lines.append("")

# Off-diff findings
if off_diff_findings:
lines.append("<details>")
Expand All @@ -408,6 +454,29 @@ def format_summary_comment(
lines.append("</details>")
lines.append("")

# Summary-only findings (scored but not inline-eligible)
if summary_only_findings:
lines.append("<details>")
lines.append(
f"<summary>Summary-only findings ({len(summary_only_findings)})"
" \u2014 scored but not inline-eligible</summary>"
)
lines.append("")
for f in summary_only_findings:
sev_emoji = _SEVERITY_EMOJI.get(f.severity.value, "\u26aa")
f_title = _sanitize_comment_text(f.title)
f_description = _sanitize_comment_text(f.description)
f_suggestion = _sanitize_comment_text(f.suggestion)
lines.append(f"#### {sev_emoji} {f.severity.value}: {f_title}")
lines.append(f"\U0001f4c1 `{_sanitize_path(f.file)}:{f.line_start}`")
lines.append("")
lines.append(f_description)
lines.append("")
lines.append(f"**Suggestion:** {f_suggestion}")
lines.append("")
lines.append("</details>")
lines.append("")

lines.append("---")
lines.append(f"<sub>Commit: {head_sha[:7]}</sub>")
lines.append("")
Expand Down Expand Up @@ -497,6 +566,9 @@ def post_review(
score: int,
verdict: str,
diff_truncated: bool = False,
summary_only_findings: list[Finding] | None = None,
policy_bypassed: bool = False,
display_capped_count: int = 0,
) -> None:
"""Post Grippy review as inline comments + summary dashboard.

Expand Down Expand Up @@ -526,23 +598,62 @@ def post_review(
existing = fetch_grippy_comments(repo=repo, pr_number=pr_number)

# 2. Classify: which current findings already have comments?
# New-format markers use exact 4-field match (file, category, line, disc).
# Old-format markers (disc=None) match if exactly one current finding
# shares the same (file, category, line) — ambiguous old markers are
# treated as new to avoid incorrect dedup.
new_findings: list[Finding] = []
for finding in findings:
key = (finding.file, finding.category.value, finding.line_start)
if key not in existing:
new_findings.append(finding)

# 3. Identify stale: use GitHub's isOutdated tracking instead of marker-key diff.
# A thread is stale if GitHub marked it outdated (line moved/removed) AND
# its marker key is no longer in the current findings.
current_keys = {(f.file, f.category.value, f.line_start) for f in findings}
absent_comments = [comment for key, comment in existing.items() if key not in current_keys]
fkey = (finding.file, finding.category.value, finding.line_start, _disc_hash(finding))
if fkey in existing:
continue
# Conservative old-marker match: disc=None at same (file, cat, line)
old_key = (finding.file, finding.category.value, finding.line_start, None)
if old_key in existing:
# Old marker — only match if this is the sole candidate at that position
candidates = [
f
for f in findings
if f.file == finding.file
and f.category.value == finding.category.value
and f.line_start == finding.line_start
]
if len(candidates) == 1:
continue # unambiguous match
new_findings.append(finding)

# 3. Identify stale: a thread is stale if its marker key is no longer in
# the current findings (the finding was suppressed or disappeared).
# Resolve all absent, unresolved threads — not just GitHub-outdated ones.
# Summary-only findings are still active concerns — don't resolve them.
all_active = list(findings)
if summary_only_findings:
all_active.extend(summary_only_findings)
current_keys: set[tuple[str, str, int, str | None]] = {
(f.file, f.category.value, f.line_start, _disc_hash(f)) for f in all_active
}
absent_comments = []
for key, comment in existing.items():
if key in current_keys:
continue
if key[3] is None:
# Old marker — don't auto-resolve if multiple candidates exist
candidates = [
f
for f in all_active
if f.file == key[0] and f.category.value == key[1] and f.line_start == key[2]
]
if len(candidates) == 1:
continue # matched to the sole candidate
if len(candidates) > 1:
continue # ambiguous — leave the thread alone
absent_comments.append(comment)
resolved_comments: list[Any] = []
if absent_comments:
thread_states = fetch_thread_states([c.node_id for c in absent_comments])
for comment in absent_comments:
state = thread_states.get(comment.node_id, {})
if state.get("isOutdated", False) and not state.get("isResolved", False):
if not state.get("isResolved", False):
resolved_comments.append(comment)

# Detect fork PR — GITHUB_TOKEN is read-only for forks
Expand Down Expand Up @@ -637,6 +748,10 @@ def post_review(
head_sha=head_sha,
pr_number=pr_number,
diff_truncated=diff_truncated,
summary_only_findings=summary_only_findings,
policy_bypassed=policy_bypassed,
display_capped_count=display_capped_count,
summary_only_count=len(summary_only_findings) if summary_only_findings else 0,
)

# Upsert: edit existing summary or create new
Expand Down Expand Up @@ -676,16 +791,11 @@ def fetch_thread_states(thread_ids: list[str]) -> dict[str, dict[str, bool]]:
"... on PullRequestReviewThread { id isOutdated isResolved } } }"
)
try:
# gh -F doesn't parse JSON arrays — use --input with stdin
input_payload = json.dumps({"query": _nodes_query, "variables": {"ids": thread_ids}})
result = subprocess.run(
[
"gh",
"api",
"graphql",
"-f",
f"query={_nodes_query}",
"-F",
f"ids={json.dumps(thread_ids)}",
],
["gh", "api", "graphql", "--input", "-"],
input=input_payload,
capture_output=True,
text=True,
timeout=30,
Expand Down
13 changes: 13 additions & 0 deletions src/grippy/mcp_response.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,19 @@ def serialize_audit(
"status": review.verdict.status.value,
"merge_blocking": review.verdict.merge_blocking,
},
"summary_only_findings": [
{
"id": f.id,
"file": f.file,
"line": f.line_start,
"severity": f.severity.value,
"category": f.category.value,
"title": f.title,
"description": f.description,
"confidence": f.confidence,
}
for f in review.summary_only_findings
],
"rule_findings": [_serialize_rule_finding(r) for r in (rule_findings or [])],
"metadata": {
"model": review.model,
Expand Down
2 changes: 1 addition & 1 deletion src/grippy/mcp_server.py
Original file line number Diff line number Diff line change
Expand Up @@ -155,7 +155,7 @@ def _run_audit(scope: str = "staged", profile: str = "security") -> str:
)

try:
review = run_review(agent, user_message)
review = run_review(agent, user_message, mode=mode, diff=diff)
except ReviewParseError as exc:
return _json_error(f"Review failed after {exc.attempts} attempts")
except Exception as exc:
Expand Down
Loading
Loading