Skip to content

Commit 6c4bdb4

Browse files
committed
fix(sdk-review): FP-K-01 PY-CON-01 respects PR added-lines scope
Discovered live on cloud-sdk-python PR #209 (Ricardo, agentgateway auditlog). The check was counting string occurrences from the on-disk AST — global to the file — then anchoring the finding to the first occurrence's line number. When a PR (a) shifts line numbers by adding upstream code and (b) does not touch any of the actual repeated-string lines, PY-CON-01 fires for tech debt that pre-existed the PR. Fix: read $ADDED_LINES_FILE (already exported by orchestrate.sh) inside check_con_01. Filter each literal's occurrence list to lines actually in the PR's added set. Only fire when ${filtered} is non-empty AND total occurrences (across file, unchanged) still >= threshold. Anchor line becomes the first added-line occurrence. Backward-compat: if ADDED_LINES_FILE is unset (bats tests exercising the lib directly), fall back to the legacy full-file behavior. Two new regression tests pin both branches (untouched pre-existing repetition = no fire; at least one occurrence on added line = fires with correct anchor). Bats: 81/81 green on both repos.
1 parent a436253 commit 6c4bdb4

2 files changed

Lines changed: 558 additions & 10 deletions

File tree

.claude/scripts/lib/ast_python_checks.py

Lines changed: 48 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@
1818

1919
import ast
2020
import json
21+
import os
2122
import sys
2223
from pathlib import Path
2324

@@ -229,6 +230,32 @@ def check_con_01(
229230
effective_min_len = max(min_len, 3)
230231
# FP-D-01: per-file cap
231232
MAX_PER_FILE = 3
233+
234+
# FP-K-01: load the PR's added-lines set (if orchestrate provided it) so
235+
# we only count string occurrences that the PR actually introduces. Prevents
236+
# penalizing an unrelated edit for pre-existing repetitions.
237+
added_lines_for_file: set[int] | None = None
238+
added_lines_file = os.environ.get("ADDED_LINES_FILE", "")
239+
if added_lines_file and Path(added_lines_file).is_file():
240+
added_lines_for_file = set()
241+
try:
242+
with open(added_lines_file, encoding="utf-8") as fh:
243+
for entry in fh:
244+
entry = entry.strip()
245+
if not entry or ":" not in entry:
246+
continue
247+
p, _, ln = entry.rpartition(":")
248+
if p == path:
249+
try:
250+
added_lines_for_file.add(int(ln))
251+
except ValueError:
252+
continue
253+
except OSError:
254+
added_lines_for_file = None
255+
# If the caller didn't provide a diff scope, fall back to legacy behavior
256+
# (count all occurrences). Bats tests exercise the check without an
257+
# orchestrate wrapper, so we keep backward-compat.
258+
232259
counts: dict[str, list[int]] = {}
233260
for node in ast.walk(tree):
234261
if isinstance(node, ast.Constant) and isinstance(node.value, str):
@@ -244,16 +271,27 @@ def check_con_01(
244271
for literal, lines in counts.items():
245272
if emitted >= MAX_PER_FILE:
246273
break
247-
if len(lines) >= threshold:
248-
emit(
249-
"PY-CON-01",
250-
"FLAG",
251-
path,
252-
lines[0],
253-
f"String literal {literal!r} appears {len(lines)}× — extract module-level constant",
254-
suggestion=f"e.g., _CONSTANT_NAME = {literal!r}",
255-
)
256-
emitted += 1
274+
if len(lines) < threshold:
275+
continue
276+
# FP-K-01: require at least one occurrence to live on a PR-added line.
277+
# Without this, an unrelated edit is credited for the repetition that
278+
# already existed. If we have no diff scope, allow the legacy path.
279+
if added_lines_for_file is not None:
280+
added_occurrences = [ln for ln in lines if ln in added_lines_for_file]
281+
if not added_occurrences:
282+
continue
283+
anchor_line = added_occurrences[0]
284+
else:
285+
anchor_line = lines[0]
286+
emit(
287+
"PY-CON-01",
288+
"FLAG",
289+
path,
290+
anchor_line,
291+
f"String literal {literal!r} appears {len(lines)}× — extract module-level constant",
292+
suggestion=f"e.g., _CONSTANT_NAME = {literal!r}",
293+
)
294+
emitted += 1
257295

258296

259297
# ---------- PY-PT-04: module has any Exception subclass (AST-based) ----------

0 commit comments

Comments
 (0)