Skip to content

[code-review] #142 fix in unchecked_assert_check.go/return_count_check.go uses position strings as 'content fingerprints' — line shifts re-flag existing issues #157

Description

@topcheer

File/Line

  • internal/agent/unchecked_assert_check.go L171-184 (findUncheckedAsserts / assertFingerprint)
  • internal/agent/return_count_check.go L51-95 (checkExcessiveReturns delta logic)

Problem

The #142 fix replaced count-based deltas with set-based fingerprints, but the fingerprint is token.Position.String() — a file:line:col location string, not content:

// unchecked_assert_check.go L174
results = append(results, uncheckedAssertInfo{line: p.Line, expr: p.String()}) // expr = "/path/file.go:42:1"

The fix's own comment claims the opposite of the behavior: "Uses the expression text so the same assertion at different line numbers is recognized as the same issue" — with a position key, the same assertion at a different line is always a NEW issue.

Trigger scenario

  1. helper() at L100 has 8 returns (pre-existing, over threshold); two unchecked assertions exist at L10/L15
  2. Agent inserts 5 lines at the top of the file for an unrelated fix
  3. Old fingerprint file.go:100:1 vs new file.go:105:1 — mismatch
  4. The untouched function/assertions are flagged as newly-introduced → false warnings

Net effect: any edit above existing problem sites re-flags every pre-existing issue below it — the same false-positive class #142 was meant to eliminate, relocated from count drift to line drift.

Expected vs actual

  • Expected: unchanged issues survive line shifts (comment's stated intent)
  • Actual: line shift = new fingerprint = false "introduced" warning

Fix

  • unchecked_assert: fingerprint on the assertion expression text (e.g. x.(int))
  • return_count: fingerprint on the function name
  • Correct or delete the misleading comment

Severity

High — deterministic false positives on a routine edit pattern (inserting lines above existing code); fires on every write-integrity pass.

Verified by independent review subagent (sa-3) with concrete old/new interleaving; ctxkey_check.go compared as reference.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions