Skip to content

feat: sticky-comment-only PR reviews, LEFT-side deleted-file support, prompt overhaul#32

Merged
supasympa merged 18 commits into
mainfrom
fix/pr-reviews
May 9, 2026
Merged

feat: sticky-comment-only PR reviews, LEFT-side deleted-file support, prompt overhaul#32
supasympa merged 18 commits into
mainfrom
fix/pr-reviews

Conversation

@supasympa

Copy link
Copy Markdown
Contributor

Summary

Major overhaul of PR review system switching to a sticky-comment-only model, adding LEFT-side comment support for deleted files, rewriting all critic prompts with a structured skeleton, adding JSON Schema mode for Ollama structured output, and implementing post-parse validation to drop hallucinated findings.

Key changes

  • Sticky-comment-only model: One editable conversation comment for the verdict table + inline review comments only
  • LEFT-side comment support: Deleted-file findings now anchor to OrigLines on the LEFT side of the diff
  • diff.ParseHunkBody: New headerless patch parser for GitHub PR patches
  • ±3 snap window: Line-number validation with snapping to nearest valid diff line
  • Prompt skeleton: All 6 critic prompts rewritten with full skeleton
  • risk_classifier returns {risk, reasoning} only
  • JSON Schema mode: Ollama backends pass schema as format field for structured output
  • Tier-specific settings: chatRequestForTier() sets temperature, top_p, stop tokens
  • ACIG logo: Embedded in review body, sticky comment, inline comments, check-run summary
  • Suggestion blocks: Single-line SuggestedFix rendered as GitHub suggestion blocks
  • EditReview for cleanup: COMMENTED reviews edited to superseded marker
  • Paginated API calls with dedup
  • hasSegment() fix: node_modules FileKind bug fixed
  • Bumped to v1.12.0

Testing

  • go test ./... — all tests pass
  • go vet ./... — clean
  • go build ./... — clean

Checklist

  • Tests pass
  • Lint clean
  • Version bumped

supasympa added 18 commits May 8, 2026 00:12
Prevent 422 'Path could not be resolved' errors when creating review
comments on files that don't exist in the PR's diff (e.g., deleted files).

- Add ListPRFiles() to fetch PR's changed files
- Filter buildReviewComments() to only include files present in PR diff
- Add tests for groupFindings and buildReviewComments
- TestReport_RetryWithoutCommentsOnPositionError: verifies retry logic
- TestReport_FallsBackToStickyCommentWhenBothReviewAttemptsFail
- TestReport_SuccessfulReviewOnFirstTry
- TestReport_UsesPRFilesFilter

Also converts GitHubReporter.client to use interface for testability.
- Add GetPRFileDiffs() to fetch file diffs with line counts
- Add validateLine() to cap line numbers to actual changed lines
- buildReviewComments now validates line numbers against actual diff
- Skip comments for invalid/outsized line numbers instead of failing

This should fix 'Line could not be resolved' errors when AI reports
line numbers beyond the actual changes in the diff.
…ectly

Root causes:
1. validateLine() used len(fd.Added) (count of added-line strings) as a
   maximum valid line number, so a finding at line 150 in a file with 5
   added lines got clamped to line 5 — which GitHub rejects with 'position
   could not be resolved', causing the whole review to fall back to a
   single sticky comment.
2. Findings with no file, a file not in the PR, or a line outside the
   diff were silently dropped — only appearing in the review body table,
   never as visible PR comments.

Fixes:
- Add DiffLines map[int]bool to FileDiff, populated during Parse() by
  tracking hunk NewStartLine through context and added lines.
- validateLine() now does an exact lookup in DiffLines; if the line
  isn't in the diff it returns 0 (general comment path) instead of
  returning a wrong clamped line number.
- Findings that can't be inline comments (no file, file not in PR, line
  outside diff) are now posted as separate issue-level PR comments via
  PostComment(), each tagged with <!-- acig:review --> for cleanup on
  re-runs.
- Add PostComment() to GitHubClient interface and implementation.
Previously, dismissOldReviews only handled CHANGES_REQUESTED and APPROVED
reviews — skipping COMMENT-state reviews entirely. Since normal acig
reviews use event=COMMENT, old inline comments from those reviews would
pile up across re-runs, cluttering the PR conversation.

Now cleanupOldReviews:
- Deletes inline review comments for ALL acig reviews (COMMENTED,
  CHANGES_REQUESTED, APPROVED, PENDING)
- Dismisses CHANGES_REQUESTED/APPROVED reviews (clears the review state)
- Deletes PENDING reviews entirely via DeletePendingReview
- Leaves COMMENTED reviews with comments removed (can't be dismissed or
  deleted via the API, but inline clutter is gone)

Combined with RemoveStaleAcigComments (which deletes issue-level comments
tagged with <!-- acig:review -->), this ensures a clean slate on each run.
…tion comments

Critics return line_start=0 for file-level concerns (missing tests,
style issues on a whole file, etc.) because there is no single 'blame'
line. Previously, ANY finding with LineStart<=0 was posted as a general
PR conversation comment, even when it clearly referenced a specific file.

This caused findings like:
  acig [general] — src/services/getPlanContext.ts
  [medium] Missing test for empty plan context (test_coverage_smell)
to appear in the conversation instead of on the file where they belong.

Fix:
- groupFindings now includes LineStart=0 findings that have a file,
  instead of excluding them from inline comment processing.
- buildReviewComments snaps LineStart=0 findings to the first visible
  line in the file's diff hunk (snapToFirstDiffLine) and posts them as
  proper inline review comments on that file.
- generalReason now only returns a non-empty reason (i.e. forces a
  conversation comment) for truly unanchorable findings:
    * File="" — no file reference at all
    * File not in the PR diff
    * File in diff but deleted or has zero hunk lines (can't snap)
- Everything else — including all file-level findings from critics like
  test_coverage_smell — becomes an inline review comment anchored at the
  first changed line of the referenced file.
… prompt skeleton overhaul, JSON Schema mode for Ollama, post-parse validation

- Switch from per-finding conversation comments to sticky-comment-only model (one editable verdict comment + inline review comments)
- Add LEFT-side comment support for deleted files with OrigLines tracking
- Rewrite all 6 critic prompts with full skeleton (ROLE, HARD RULES, DO NOT, SEVERITY RUBRIC, WORKED EXAMPLES, SELF-CHECK)
- risk_classifier now returns only {risk, reasoning} — no findings array
- Add diff.ParseHunkBody for headerless GitHub PR patches
- Add ±3 snap window for line-number validation, post-parse drop of out-of-diff findings
- Add JSON Schema mode for Ollama structured output
- Add chatRequestForTier with tier-specific temperature/top_p/stop settings
- Embed ACIG logo in review body, sticky comment, inline comments, check-run summary
- Render single-line SuggestedFix as GitHub suggestion blocks
- Severity legend, acig-ignore hint, run URL deep link in check-run
- EditReview + superseded marker for COMMENTED reviews cleanup
- Paginated ListReviews, RemoveStaleAcigComments, DeleteReviewComments, PostStickyComment with dedup
- Fix node_modules FileKind bug (hasSegment instead of strings.Contains)
- Bump version to 1.12.0
@supasympa supasympa merged commit 1bb8fee into main May 9, 2026
3 checks passed
@supasympa supasympa deleted the fix/pr-reviews branch May 9, 2026 03:50
@github-actions

github-actions Bot commented May 9, 2026

Copy link
Copy Markdown

acig ACIG · Code Review

PASS · risk=high · 0 finding(s) · $0.0006

No findings. Code looks clean.

Verdict JSON
{"schema_version":"1","repo":"https://github.com/helloodokai/acig","sha":"3b9e4a83740f58fe6f712b7059657ad213573706","base_sha":"main","risk":"high","decision":"pass","summary":"Decision: pass | Risk: high | Findings: 0 blocking, 0 high, 0 medium, 0 low, 0 info | Cost: $0.0006","findings":[],"critic_results":[{"critic":"perf_smell","model":"qwen3-coder:480b","findings":null,"cost_usd":0.00017790000000000001,"duration_ms":2057,"tokens_in":2926,"tokens_out":13},{"critic":"security_smell","model":"qwen3-coder:480b","findings":null,"cost_usd":0.00019067999999999998,"duration_ms":3596,"tokens_in":3139,"tokens_out":13},{"critic":"risk_classifier","model":"gpt-oss:20b","findings":null,"cost_usd":0.00005259,"duration_ms":5260,"tokens_in":2019,"tokens_out":1080,"reasoning":"New network calls added to GitHub client and reporter, expanding external I/O surface.","risk":"high"},{"critic":"style_conformance","model":"gpt-oss:20b","findings":null,"cost_usd":0.0000598,"duration_ms":5452,"tokens_in":2917,"tokens_out":1021},{"critic":"test_coverage_smell","model":"gpt-oss:20b","findings":null,"cost_usd":0.00009044999999999999,"duration_ms":10293,"tokens_in":2901,"tokens_out":2048},{"critic":"adjudicator","model":"glm-5.1","findings":null,"cost_usd":0,"duration_ms":6650,"tokens_in":3228,"tokens_out":306}],"total_cost_usd":0.0005714199999999999,"total_duration_ms":33308,"budget_remaining_usd":0.24942858,"generated_at":"2026-05-09T03:50:32.619946051Z"}

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

acig ACIG · PASS · risk=high · 0 finding(s)

0 inline comment(s) posted on the Files changed tab.

Cost: $0.0006 · 33308ms

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant