Report source lines in security preflight findings

[justin808]

Summary

• derive suspicious added-line source coordinates from unified-diff hunk headers
• retain flattened diff-output positions as secondary diagnostics and preserve fallback coordinates for malformed or missing hunk metadata
• close a detection gap where an added payload beginning with ++ renders as +++ and could be mistaken for a file header; file headers are now recognized only during the pre-hunk metadata phase
• cover multi-hunk mapping, malformed hunk headers, ++ added payloads (including after a -- deleted payload), and raw-content redaction

Validation

• focused preflight suite — 110 runs, 941 assertions, 0 failures
• Ruby syntax, git diff --check, and focused RuboCop — clean; 2 files inspected, no offenses
• exact-head bin/validate — passed; RuboCop inspected 112 files with no offenses
• synthetic combined-tip bin/validate against current main — passed; RuboCop inspected 114 files with no offenses
• hosted validate and claude-review — completed successfully at 3ced9f8a11010f400c6ec34d0935420021098fb8

Summary by CodeRabbit

• Bug Fixes

  • Improved suspicious change detection by reporting the affected file and source line when available.
  • Added reliable line tracking across multiple diff sections.
  • Added fallback locations when diff metadata is incomplete or malformed.

• Tests

  • Added coverage for multi-section diffs, source-line mapping, and fallback reporting.

[coderabbitai]

Walkthrough

The security preflight now tracks Git diff file names and new-file line numbers. It parses unified diff hunks, reports source-aware suspicious locations, and falls back to diff output lines for malformed headers. Tests cover multi-hunk and fallback cases.

Changes

Suspicious diff location tracking

Layer / File(s)	Summary
Parse and track source locations skills/pr-batch/bin/pr-security-preflight	The scanner parses hunk headers, tracks new-file lines across additions and context, ignores deletions and metadata, and reports file and source-line locations.
Validate mapping and fallback behavior skills/pr-batch/bin/pr-security-preflight-test.rb	Fixtures and tests cover multi-hunk line mapping and fallback reporting for malformed hunk headers.

Estimated code review effort: 3 (Moderate) | ~20 minutes

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: adding source-line reporting to security preflight findings.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch jg-codex/preflight-source-lines

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@skills/pr-batch/bin/pr-security-preflight`:
- Line 730: Update the added_line detection in the diff scanner to treat every
remaining line beginning with “+” as an addition, relying on the existing
file-header handling at lines 718-722 to exclude “+++ ” headers. Add a
regression fixture covering suspicious content whose added payload begins with
“++”.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bfb4a198-d2a4-49ba-83ff-da2f83c0dc9c

📥 Commits

Reviewing files that changed from the base of the PR and between e2106eb and 7fdf4ec.

📒 Files selected for processing (2)

• skills/pr-batch/bin/pr-security-preflight
• skills/pr-batch/bin/pr-security-preflight-test.rb

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7fdf4ec159

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Review

Overview

This PR improves pr-security-preflight's reporting for suspicious added lines: instead of only reporting the raw line offset within the fetched diff text, it now derives the actual source file line number from the unified-diff hunk headers (@@ -a,b +c,d @@), while preserving the flattened diff-output offset as a secondary diagnostic and falling back gracefully when a hunk header is malformed or absent.

Correctness

I traced the new state machine in suspicious_added_lines (and its helpers unified_diff_hunk_new_start / suspicious_diff_location) by hand against the new fixtures (multi-hunk-warning-diff, malformed-hunk-warning-diff):

• current_file/current_new_line are correctly reset on diff --git (new file section) and +++ (file header) lines, preventing stale file/line attribution from leaking across files in a multi-file diff — a nice incidental correctness improvement over the prior code, which never reset current_file between files lacking a +++ b/ line.
• The new-line counter increments only for context ( ) and added (+) lines, holds steady for removed (-) and no-newline-marker (\) lines, and resets to nil on anything else (e.g. a malformed hunk header) — this matches unified diff semantics.
• source_line is captured before the counter increments, so it reflects the added line's own position, not the next line's.
• The malformed-hunk fallback correctly leaves current_new_line as nil, so suspicious_diff_location degrades to the pre-existing "file (diff output line N)" format rather than printing a wrong line number.
• The existing warning-diff fixture (no hunk header at all) still exercises the old fallback path unchanged, confirming backward compatibility.
• warnings_not_already_blocking's location-based de-dup (comparing blocking vs. warning findings from two separate passes over the same diff) is unaffected since both passes compute identical, deterministic locations for the same input.

I did not find any correctness bugs. The detection predicate itself (added_line && line.match?(pattern)) is behaviorally identical to the prior line.start_with?("+") && !line.start_with?("+++") check, so no suspicious-content detection is weakened — this is a pure diagnostics/UX improvement.

Security

No regressions. The location-string changes are cosmetic/diagnostic only; the actual blocking/warning decision logic is untouched. No raw diff content is echoed for malformed input, matching the PR's stated goal.

Minor observations (non-blocking)

• Left an inline nit: Integer(match[1], exception: false) is slightly over-defensive since the capture group is already constrained to \d+; match[1].to_i would be equivalent.
• Pre-existing (not introduced by this PR) theoretical edge case: a source line whose added content is literally ++ b/<path> would render in the diff as +++ b/<path> and be indistinguishable from a real +++ file-header line by the line.start_with?("+++ ") check. Extremely unlikely in practice and doesn't affect detection, only possible location misattribution — not worth blocking on.

Test coverage

The two new tests (multi-hunk across two hunks, and malformed-hunk fallback) directly exercise the new branch logic and assert exact file:line (diff output line N) strings, which is good — they'd catch off-by-one regressions in the counter arithmetic. The shell-side warning_diff_line/warning_diff_line_2 fixtures follow the existing pattern of split-string construction to avoid the security scanner flagging the test file itself.

Overall: solid, well-tested, low-risk diagnostic improvement. No blocking issues found.

[claude]

Review

Overview

This PR improves suspicious_added_lines in pr-security-preflight to derive the source file line number of a suspicious added line from unified-diff hunk headers (@@ -a,b +c,d @@), instead of only reporting the raw diff-output line offset. It keeps the flattened diff-output line as a secondary diagnostic and falls back gracefully when hunk headers are missing/malformed. Two new tests cover multi-hunk tracking and malformed hunk headers.

Correctness

I hand-traced the parser against all four fixtures (single-hunk, multi-hunk, malformed-hunk, and the +++-content edge case) and the line-number math checks out exactly against the new assertions (:12, :13, :31 etc. matching the expected added-line positions across two hunks).

Key points verified:

• current_file/current_new_line/in_hunk are correctly reset on every diff --git boundary, so state can't leak across files in a multi-file diff.
• The hunk-header regex is anchored (\A...\z) and only matches unprefixed @@ ... @@ lines, so it can't be spoofed by added file content that happens to look like a hunk header (diff-prefixed content always carries a leading +/-/ character, which breaks the \A@@ anchor).
• Gating the +++ file-header check on !in_hunk fixes a real bypass in the old code — see inline comment. The old condition !line.start_with?("+++") would have silently skipped pattern-matching on any added line whose content began with ++, letting suspicious content evade detection. This is now correctly handled and is explicitly tested via plusplus_warning_diff_line. Good catch, though it's a security-relevant behavior change worth calling out explicitly in the PR description rather than leaving implicit.

Minor notes (left inline)

• The "new file line ..." fallback branch in suspicious_diff_location (current_file nil but source_line present) doesn't look like it's exercised by the new tests — worth a small test or a one-line comment on when it's expected to fire.

Security

No concerns — the change only affects where a finding is reported, not whether one is raised, and (per the point above) actually closes a detection gap rather than opening one. Suspicious raw content is still never printed, only location metadata, consistent with the existing security posture.

Performance

Single pass over diff.lines, same O(n) complexity as before — no concerns.

Test coverage

Good — new tests cover multi-hunk tracking and malformed hunk header fallback, and the fake gh harness changes are scoped correctly to the new modes. Only gap is the untested fallback branch noted above.

Overall: solid, well-tested change with an incidental security fix. No blocking issues.

[claude]

Review

Overview
This PR improves the pr-security-preflight script's diagnostics: instead of only reporting a raw "diff output line N" for suspicious added content, it now parses unified-diff hunk headers (@@ -a,b +c,d @@) to track and report the actual source file line number (file:N), while retaining the flattened diff-output line as a secondary diagnostic and falling back gracefully when hunks are missing or malformed.

Code quality / correctness

• The state machine (current_file, current_new_line, in_hunk, file_header_allowed, awaiting_new_file_header) is a bit dense but each transition is deliberate and correctly handles the tricky cases: multi-hunk diffs, malformed @@ headers, no-trailing-newline markers, and content lines that happen to start with +++/@@-like text (verified these are only recognized as real diff markup when unprefixed by +/-/space, so injected content can't spoof a header).
• Traced the new/updated tests (multi-hunk-warning-diff, malformed-hunk-warning-diff) by hand against the state machine and the line-number math checks out.
• Critically for a security-focused script: the change only affects the location string attached to a match, not the matching logic itself (pattern matching against line.match?(pattern) is untouched, and added_line detection still fires independent of whether hunk-header parsing succeeded). So a malformed/missing hunk header degrades location precision but does not cause suspicious content to go unflagged (confirmed via the malformed-hunk-warning-diff test, which still flags both suspicious lines, just without a line number). This is the right fail-safe direction for a security gate.
• Backward compatible: existing tests asserting the looser "file (diff output line N)" substring still pass because the pre-existing test fixtures don't include real @@ headers (they go through the same fallback path as before).

Minor issues (left as inline comments)

• suspicious_diff_location's "new file line N" fallback label is misleading, it fires whenever the filename couldn't be determined (e.g. a deleted file's dev-null hunk), not only for genuinely new files. This fallback branch also isn't exercised by the new tests.
• Integer(match[1], exception: false) in unified_diff_hunk_new_start is unnecessarily defensive since the regex already guarantees a digits-only capture (match[1].to_i would suffice).

Test coverage
Good, new tests cover multi-hunk tracking, malformed headers, and confirm the suspicious findings themselves are still detected (not just relocated). Nice touch verifying full_diff_call_count stays at 1 (no extra API calls introduced).

Security
No concerns, this only changes diagnostic output for an existing detection mechanism; detection patterns (BLOCKING_SUSPICIOUS_PATTERN/WARNING_SUSPICIOUS_PATTERN) and match logic are unchanged.

Overall: solid, well-tested change with only cosmetic nits.

[justin808]

Address-review summary

Scan scope: full PR history; no prior valid address-review summary cutoff.

Mattered

• Addressed and resolved the ++-prefixed added-payload detection bypass. The parser now recognizes file headers only during pre-hunk metadata, including the malformed-hunk ---/+++ intersection; focused tests are 110/941 green and exact-head plus combined-tip validation passed. CodeRabbit thread, Codex thread
• Updated the PR description to explicitly document the security-relevant behavior change and current validation evidence, then resolved the metadata request. Thread

Optional

• Declined the two equivalent Integer(..., exception: false) → to_i conversion nits at the final-candidate debounce; the current strict conversion is harmless and explicit. Earlier thread, exact-head thread
• Declined the no-path fallback wording/comment/fixture suggestions as late optional test-shape work; the fallback intentionally preserves a unified-diff new-side coordinate and no security or coordinate regression was demonstrated. Coverage thread, wording thread

Skipped

• Two formal review summaries and four issue-level bot review/status comments were status-only, boilerplate, or duplicated the inline threads above; no separate action was required. CodeRabbit reported SUCCESS but its latest incremental attempt was rate-limited and produced no new inline finding; the prior actionable CodeRabbit thread is resolved. Current-head claude-review independently completed successfully.

Deferred-work tracking: none; all optional outcomes were explicitly declined with rationale and their conversations were resolved.

Next default scan starts after this comment. Say check all reviews to rescan the full PR.