Fallback comment posting for findings outside the diff by hsyntax · Pull Request #163 · hsyntax/brrr

hsyntax · 2026-03-07T04:55:53Z

Summary

Findings referencing files not in the PR diff no longer hard-fail the review — they follow a fallback chain: line comment → file comment → related-file comment → standalone PR comment
Relax correctness/security review prompts so reviewers can flag issues in code affected by changes, not just changed code itself
Add DiffPositionMapper::find_nearest_file to locate a related diff file in the same or parent directory
Add FallbackContext::RelatedFile and ::Standalone variants with GitHub-rendered notes
Add SubmissionBackend::post_finding_comment for standalone PR comments (gh pr comment)

Testing

cargo fmt --all -- --check
cargo clippy --all-targets --all-features -- -D warnings
cargo nextest run
cargo nextest run --profile integration -E 'binary(cli_binary)'

🤖 Generated with Claude Code

Add emoji indicators (🔴/🟡/🔵 severity, ✅/❌ verdict, 🚧 blocked), H3 headers for inline findings, collapsible suggested fixes via <details>, GitHub alert syntax for fallback notes, and a table for category breakdown in the summary comment. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Resolve conflicts with #161 (comment formatting) and #162 (fail hard on inline submission errors), keeping this branch's target validation, file-level comment fallback, and strict error handling. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Findings referencing files not in the PR diff no longer hard-fail the review. Instead they follow a fallback chain: line comment → file comment → related-file comment → standalone PR comment. This lets security and correctness reviewers flag issues in affected code outside the changed files. - Relax prompt constraints: reviewers may inspect code affected by changes, not just changed code itself - Add DiffPositionMapper::find_nearest_file to locate a related diff file in the same or parent directory - Add FallbackContext::RelatedFile and ::Standalone variants with GitHub-rendered notes - Add SubmissionBackend::post_finding_comment for standalone PR comments - Replace InvalidReviewCommentTarget hard error with graceful fallback Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

hsyntax

Review: 4 finding(s) across the changes.

hsyntax · 2026-03-07T20:16:23Z

🔵 INFO (quality)

hunk-cursor-old-line-unused — HunkCursor.old_line is initialised and incremented but its value is never read. Only new_line drives commentable_lines.

Suggested fixes

Remove old_line from HunkCursor, its parse from the hunk-header block, and the checked_add expressions in the b' ' and b'-' match arms.

Note
This finding applies to src/diff_position_mapper.rs:48 but is shown as a file-level comment because that location is not commentable in the diff.

hsyntax · 2026-03-07T20:16:24Z

Critical trust-filtering gap in fix-loop issue-comment ingestion and duplicate-posting risks on transient failures require code changes before merge.

Verdict: ❌ needs_fix
Findings: 12 total (🔴 critical: 1, 🟡 warning: 6, 🔵 info: 5)

Category Breakdown

Category	Count
authorization	1
correctness	1
efficiency	4
quality	3
reuse	1
style	2

…t path and remove dead Line fallback

…with Arc

…itHubSubmission

… in fix parsing and dedupe standalone posting

hsyntax

Review: 5 finding(s) across the changes.

hsyntax · 2026-03-07T20:40:09Z

+        submission
+            .fetch_pr_comments(pr_number)?
+            .into_iter()
+            .map(|comment| PrReviewComment {


🔵 INFO (quality)

pr-comment-fields-dropped — When mapping PrComment to PrReviewComment in fetch_and_parse_items, created_at and author_association are silently discarded with no indication the drop is intentional.

Suggested fixes

Add a brief comment at the mapping site noting these fields are not needed for fix item parsing.

hsyntax · 2026-03-07T20:40:11Z

🟡 WARNING (reuse-and-correctness)

fallback-arm-triplication-and-dead-code — The find_nearest_file → FileReviewComment(RelatedFile) / StandaloneFinding block is copy-pasted verbatim three times in prepare_review_comments (InvalidLine inner arm L1121, FileNotFound|NoCurrentPath arm L1156, catch-all Err() arm L1186). Additionally, the final Err() catch-all arm is unreachable dead code — DiffPositionMapperError has only Parse, InvalidLine, FileNotFound, NoCurrentPath, and the preceding arms already cover InvalidLine and FileNotFound|NoCurrentPath, leaving only Parse which should be handled explicitly rather than silently falling back.

Suggested fixes

Extract a private helper fn push_nearest_or_standalone(...) containing the shared logic and call it from each arm.

Replace the catch-all Err(_) with an explicit DiffPositionMapperError::Parse arm — either propagate it as an error or add it to the FileNotFound|NoCurrentPath arm — so future variants cause a compile error.

Note
This finding applies to src/orchestrator.rs:1121 but is shown as a file-level comment because that location is not commentable in the diff.

Fixed: fallback-arm-triplication-and-dead-code: deduplicate review fallback handling

hsyntax · 2026-03-07T20:40:16Z

🔵 INFO (quality)

post-finding-comment-redundant-ok — post_finding_comment calls self.gh_post_pr_comment(pr_number, body)?; then returns Ok(()). Since gh_post_pr_comment returns Result<()>, the ?; Ok(()) is equivalent to a direct return.

Suggested fixes

Replace the body with self.gh_post_pr_comment(pr_number, body) directly.

Note
This finding applies to src/submission.rs:758 but is shown as a file-level comment because that location is not commentable in the diff.

…collecting existing finding IDs

…nto helper

…port

…review request

…e scan

…and dedupe nearest-file scan

…th validation

hsyntax

Review: 1 finding(s) across the changes.

hsyntax · 2026-03-07T21:07:34Z

 #[derive(Debug, Clone, PartialEq, Eq)]
 pub enum FallbackContext {
-    Line(u32),
    File { file: String, line: u32 },


🔵 INFO (style)

fallback-context-file-ambiguous — FallbackContext::File is ambiguous — all three variants reference files. It doesn't convey that the line was not commentable but the file is present in the diff.

Suggested fixes

Rename FallbackContext::File to FallbackContext::SameFile or FallbackContext::NotCommentable to make the intended meaning explicit.

…comments REST API review creation uses DraftPullRequestReviewComment which lacks subjectType and requires position — incompatible with file-level comments. Switch to GraphQL addPullRequestReview mutation with DraftPullRequestReviewThread which supports subjectType: FILE. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

hsyntax

Review: 2 finding(s) across the changes.

hsyntax · 2026-03-07T21:43:41Z

+    let mut comments = submission.fetch_pr_review_comments(pr_number)?;
+    comments.extend(
+        submission
+            .fetch_pr_comments(pr_number)?


🟡 WARNING (correctness)

standalone-finding-reply-always-fails — When the fix agent processes a standalone finding (posted via gh pr comment), reply_to_review_comment tries pulls/{pr}/comments/{id}/replies (404 for issue comments) then issues/comments/{id}/replies (not a valid GitHub API endpoint). Both fail silently — the reaction is added but the reply body explaining the fix outcome is lost.

Suggested fixes

In the issue-comment fallback branch of reply_to_review_comment, use POST /repos/{owner}/{repo}/issues/{pr_number}/comments to post a new issue-level comment mentioning the original finding, instead of the non-existent issues/comments/{id}/replies endpoint.

hsyntax · 2026-03-07T21:43:41Z

+                    caps.get(3).expect("regex capture group exists").as_str(),
+                    "hunk new start",
+                )?;
+                if let Some(count_match) = caps.get(2) {


🔵 INFO (quality)

discarded-parse-u32 — Hunk old-length and new-length are parsed via parse_u32 but the u32 is discarded — sole intent is validation. Misleads readers into thinking the value should be used.

Suggested fixes

Extract a fn validate_u32(value, context) -> Result<(), DiffPositionMapperError> that returns Result<()> by design, making discard explicit at the type level.

Fixed: discarded-parse-u32: make hunk length validation explicit

Revert file-level comment submission from broken GraphQL (DraftPullRequestReviewThread lacks subjectType/optional line) back to individual REST API calls with subject_type: "file". Add backtick formatting instruction for code references in finding descriptions. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

hsyntax

Review: 3 finding(s) across the changes.

hsyntax · 2026-03-07T22:08:08Z

+    body: String,
+    commit_id: String,
+    path: String,
+    subject_type: String,


🔵 INFO (style)

subject-type-field-always-string — FileReviewCommentRequest.subject_type is a String field always initialised to "file", causing a heap allocation per request inside a loop.

Suggested fixes

Change the field type to &'static str and all construction sites to subject_type: "file".

hsyntax · 2026-03-07T22:08:08Z

+        )));
+    }
+
+    let sha = String::from_utf8_lossy(&output.stdout).trim().to_string();


🔵 INFO (efficiency)

sha-cow-double-alloc — fetch_pr_head_sha uses String::from_utf8_lossy(&output.stdout).trim().to_string() which creates a Cow then allocates a new String. On the happy path (valid UTF-8) the extra allocation is unnecessary.

Suggested fixes

Use String::from_utf8(output.stdout).map(|s| s.trim().to_owned()) with a fallback for invalid UTF-8.

hsyntax · 2026-03-07T22:08:10Z

🟡 WARNING (correctness)

duplicate-posts-on-transient-failure — Two related duplicate-posting risks: (1) When fetch_pr_comments() fails at L842, the code falls back to HashSet::new(). If standalone_findings is non-empty, every standalone finding is reposted even when identical finding_ids already exist on the PR. (2) Detailed findings (inline, file-level, standalone) are posted before upsert_review_comment(). If the summary upsert fails after detailed comments are already created, the run returns Err; a retry then reposts the same detailed findings because line/file comments are not deduped.

Suggested fixes

Return an error instead of using an empty dedupe set whenever standalone_findings is non-empty and fetch_pr_comments() fails.

Move upsert_review_comment() ahead of detailed finding submission, or treat a summary failure as non-fatal after detailed comments have been posted.

Add cross-comment dedupe by finding_id for line/file review comments before retrying submission.

Note
This finding applies to src/orchestrator.rs:842 but is shown as a file-level comment because that location is not commentable in the diff.

hsyntax · 2026-03-07T22:08:11Z

🟡 WARNING (quality)

prepare-review-comments-error-arms — The three error arms in prepare_review_comments contain duplicated fallback logic and a dead arm. Err(FileNotFound | NoCurrentPath) and Err() execute byte-for-byte identical find_nearest_file/standalone code. The Err() arm is dead code since target_for never returns DiffPositionMapperError::Parse and all other variants are already covered. The Err(InvalidLine) arm also partially duplicates the nearest-file fallback path.

Suggested fixes

Extract a helper fn push_nearest_or_standalone(prepared, mapper, finding, deps) and call it from each applicable arm.

Remove the dead Err(_) arm, and collapse Err(InvalidLine) into Err(FileNotFound | NoCurrentPath) if there is no behavioral difference.

Note
This finding applies to src/orchestrator.rs:1101 but is shown as a file-level comment because that location is not commentable in the diff.

Fixed: prepare-review-comments-error-arms: deduplicate review comment fallback handling

hsyntax · 2026-03-07T22:08:12Z

🟡 WARNING (quality)

raw-pointer-hashset-dedup — find_nearest_file uses HashSet::<*const String> with Arc::as_ptr for deduplication. This relies on all aliases for the same file sharing the same Arc allocation — a correct but fragile invariant not enforced by the type system. If finalize_pending_file ever stops sharing the same Arc, dedup silently breaks.

Suggested fixes

Replace HashSet::<*const String> with HashSet::<&str> and insert current_path.as_str() instead of Arc::as_ptr(current_path).

Note
This finding applies to src/diff_position_mapper.rs:318 but is shown as a file-level comment because that location is not commentable in the diff.

Fixed: raw-pointer-hashset-dedup: dedupe nearest-file paths by value

hsyntax · 2026-03-07T22:08:13Z

🟡 WARNING (efficiency)

double-collect-dep-descriptions — Inside prepare_review_comments, for every finding a Vec of dependency_descriptions is collected and then immediately re-iterated to collect a second Vec<&str> to pass &[&str] to render_inline_finding_comment_for_github. This allocates two intermediate Vecs per finding.

Suggested fixes

Change render_inline_finding_comment_for_github to accept &[impl AsRef] or impl IntoIterator<Item = impl AsRef> so no second collection step is needed.

Alternatively, collect into Vec then convert in one pass: let refs: Vec<&str> = owned.iter().map(String::as_str).collect().

Note
This finding applies to src/orchestrator.rs:1067 but is shown as a file-level comment because that location is not commentable in the diff.

hsyntax · 2026-03-07T22:08:14Z

🟡 WARNING (reuse)

review-comment-shared-fields — InlineReviewComment and FileReviewComment share three identical diagnostic fields (finding_id, original_file, original_line) that are not part of the GitHub API payload. Keeping diagnostic metadata inside API request structs conflates concerns. The related format helpers (format_attempted_line_review_comments / format_attempted_file_review_comment) are also asymmetric in signature despite identical format strings.

Suggested fixes

Introduce a FindingOrigin { finding_id, original_file, original_line } struct and embed it in both comment structs.

Unify the two format helpers into a single fn format_attempted_comment(finding_id, original_file, original_line, kind, path) -> String.

Note
This finding applies to src/submission.rs:223 but is shown as a file-level comment because that location is not commentable in the diff.

hsyntax · 2026-03-07T22:08:14Z

🟡 WARNING (quality)

fail-inline-review-submission-missing-methods — FailInlineReviewSubmission does not override submit_file_pr_review_comments or post_finding_comment, so both silently no-op via the trait default. The test does not verify that file-level or standalone comment paths were not reached. Also, push_review_comment is a free function called from only one location and can be inlined, and two factories have unnecessary create_phase_runner overrides.

Suggested fixes

Override submit_file_pr_review_comments and post_finding_comment with panic!("unexpected call") bodies.

Inline push_review_comment into FailInlineReviewSubmission::upsert_review_comment.

Remove the create_phase_runner override from InvalidFileReviewFactory and MixedPlacementReviewFactory.

Note
This finding applies to tests/orchestrator_integration.rs:481 but is shown as a file-level comment because that location is not commentable in the diff.

hsyntax · 2026-03-07T22:08:15Z

🔵 INFO (efficiency)

arc-string-unnecessary-clones — current_path_for and find_nearest_file convert Arc to a newly heap-allocated String via .to_string(). Since the Arc already owns the data, callers could receive Arc::clone (a pointer bump). Similarly, PendingFile::aliases unconditionally clones new_path even though primary_path() already includes it on the non-rename path.

Suggested fixes

Change both return types to Option<Arc> and return Some(Arc::clone(current_path)).

Remove the unconditional aliases.insert(self.new_path.clone()) and build the alias set directly from distinct logical aliases.

Note
This finding applies to src/diff_position_mapper.rs:311 but is shown as a file-level comment because that location is not commentable in the diff.

hsyntax · 2026-03-07T22:08:16Z

🔵 INFO (efficiency)

extract-finding-id-full-deserialize — extract_finding_id deserializes the full ReviewFinding struct just to read the id field, allocating all other string fields only to drop them.

Suggested fixes

Deserialize into a minimal struct FindingIdOnly { id: String } — unknown fields are skipped by serde by default.

Note
This finding applies to src/orchestrator.rs:1241 but is shown as a file-level comment because that location is not commentable in the diff.

hsyntax · 2026-03-07T22:08:17Z

🔵 INFO (style)

post-finding-comment-trivial-wrapper — GitHubSubmission::post_finding_comment is a one-liner that calls self.gh_post_pr_comment(pr_number, body)? then Ok(()). The ? + Ok(()) is redundant since gh_post_pr_comment already returns Result<()>.

Suggested fixes

Replace the body with self.gh_post_pr_comment(pr_number, body) and return its Result directly.

Note
This finding applies to src/submission.rs:743 but is shown as a file-level comment because that location is not commentable in the diff.

…handling

…ck handling

hsyntax and others added 4 commits March 6, 2026 17:35

Merge branch 'main' into improve-review-comment-formatting

7f9e78d

fail hard on inline review submission errors

92c8004

validate review comment targets before submission

8869843

Base automatically changed from hard-fail-review-comments to main March 7, 2026 19:01

hsyntax and others added 2 commits March 7, 2026 11:19

hsyntax changed the title ~~Validate review comment targets before submission~~ Fallback comment posting for findings outside the diff Mar 7, 2026

hsyntax commented Mar 7, 2026

View reviewed changes

Comment thread src/orchestrator.rs Outdated

Comment thread src/orchestrator.rs Outdated

Comment thread src/submission.rs

Comment thread src/orchestrator.rs

hsyntax commented Mar 7, 2026

View reviewed changes

Comment thread src/diff_position_mapper.rs

hsyntax commented Mar 7, 2026

View reviewed changes

hsyntax added 4 commits March 7, 2026 12:19

err-catch-all-and-dead-line-variant: route InvalidLine through curren…

839a850

…t path and remove dead Line fallback

finalize-per-alias-repeated-clone: share commentable lines per alias …

8b03b4c

…with Arc

post-comment-gh-duplication: centralize gh pr comment invocation in G…

d4391e1

…itHubSubmission

standalone-findings-invisible-to-fix-pipeline: include issue comments…

78f4348

… in fix parsing and dedupe standalone posting

hsyntax commented Mar 7, 2026

View reviewed changes

Comment thread src/submission.rs

hsyntax commented Mar 7, 2026

View reviewed changes

Comment thread src/submission.rs

hsyntax commented Mar 7, 2026

View reviewed changes

Comment thread src/diff_position_mapper.rs

hsyntax commented Mar 7, 2026

View reviewed changes

Comment thread src/diff_position_mapper.rs

hsyntax commented Mar 7, 2026

View reviewed changes

hsyntax added 7 commits March 7, 2026 12:44

standalone-dedup-trusts-untrusted: ignore untrusted PR comments when …

ae28d7e

…collecting existing finding IDs

pr-issue-fallback-pattern: extract review comment endpoint fallback i…

99311ef

…nto helper

import-hygiene: group sync imports and remove redundant serde_json im…

635f025

…port

file-comment-one-process-per: batch file review comments into one PR …

279a646

…review request

fragile-json-parse-heuristic: prefer full-text JSON parse before brac…

410ff1a

…e scan

find-nearest-file-alias-duplication: share file paths across aliases …

f17e5e1

…and dedupe nearest-file scan

let-underscore-parse-validate: use bare parse_u32 calls for hunk leng…

ca1345b

…th validation

hsyntax commented Mar 7, 2026

View reviewed changes

hsyntax added 5 commits March 7, 2026 14:10

fallback-arm-triplication-and-dead-code: deduplicate review fallback …

961906f

…handling

raw-pointer-hashset-dedup: dedupe nearest-file paths by value

96160bb

untrusted-issue-comments-in-fix-loop: filter untrusted issue comments

b418194

prepare-review-comments-error-arms: deduplicate review comment fallba…

9d7dfff

…ck handling

discarded-parse-u32: make hunk length validation explicit

4ea9485

Conversation

hsyntax commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Testing

Uh oh!

hsyntax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🔵 INFO (quality)

Uh oh!

hsyntax commented Mar 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Category Breakdown

Uh oh!

hsyntax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🔵 INFO (quality)

Uh oh!

Choose a reason for hiding this comment

🟡 WARNING (reuse-and-correctness)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🔵 INFO (quality)

Uh oh!

hsyntax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

🔵 INFO (style)

Uh oh!

hsyntax left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

🟡 WARNING (correctness)

Uh oh!

Choose a reason for hiding this comment

🔵 INFO (quality)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hsyntax left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

🔵 INFO (style)

Uh oh!

Choose a reason for hiding this comment

🔵 INFO (efficiency)

Uh oh!

Choose a reason for hiding this comment

🟡 WARNING (correctness)

Uh oh!

Choose a reason for hiding this comment

🟡 WARNING (quality)

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

🟡 WARNING (quality)

hsyntax commented Mar 7, 2026 •

edited

Loading

hsyntax commented Mar 7, 2026 •

edited

Loading