Skip to content

[grug] Elder should know a MOVE is a move - suppress/downgrade findings on byte-identical relocated files #772

Description

@quadseven

Why

quadseven/infra#1988 vendored three standalone repos into the monorepo via
git subtree add. The file contents were byte-identical to their sources -
verified by SHA-256 across all 86 files, 0 mismatches. Nothing was authored; the
paths changed.

Elder reviewed it as if all 6,424 lines were new code and produced 11 findings,
all against the relocated files, none against anything written for the move.

Every finding was about pre-existing code. That is not wrong exactly - the
code IS now in this repo and the findings were mostly real - but it is the wrong
frame, and it has three costs:

  1. It buries the actual diff. The move PR also carried a genuine
    security fix (XSS via attacker-controlled SSID) and a docs change. Those were
    the parts a reviewer needed to look at, and they were 11 findings deep in
    noise about code that had not changed.
  2. It creates a bad incentive. If relocating code triggers a full findings
    dump, the rational move is to avoid consolidation - exactly the opposite of
    what you want. The three repos being merged had zero CI and zero review;
    consolidating them was a strict improvement and Elder made it feel like a
    penalty.
  3. It makes the "answer every finding" rule expensive. House policy is that
    every bot thread gets a reply before merge. 11 threads on code nobody wrote in
    that PR is ~11 replies of pure ceremony.

What

Give Elder move/rename awareness. When a file in the diff is a pure relocation -
old path deleted, new path added, content hash unchanged - then:

  • Suppress complexity findings entirely. Complexity of unchanged code is not a
    property of this change.
  • Downgrade robustness/silent-failure findings to informational, or better,
    emit them ONCE as a single rollup comment ("this relocated code carries N
    pre-existing findings, see ") rather than N inline threads.
  • Keep full severity for any file where content actually changed, including a
    file that moved AND was edited.

Git already computes this - git log --find-renames / --find-copies and the
similarity index in the diff header give it directly. For git subtree add
specifically the merge shape is recognisable too.

Suggested UX

A rollup comment is better than silence. Something like:

This PR relocates 86 files with unchanged content. Elder found 10 pre-existing
findings in them, not introduced here. Filed as a single follow-up rather than
inline threads:

That preserves the value (the findings were genuinely useful - they became
quadseven/infra#1990, a real blocker on deploying that code) while keeping the
PR review focused on what the PR actually did.

Acceptance criteria

  • A PR that only moves files with identical content produces no inline
    complexity findings.
  • Robustness/silent-failure findings on such files roll up into one comment.
  • A file that is moved AND modified still gets full inline treatment.
  • Fixture: a git subtree add-shaped PR is correctly recognised as a move.

Related

Size: M

Refs #707

Metadata

Metadata

Assignees

No one assigned

    Labels

    enhancementNew feature or request

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions