ci(code-quality): scope the hardcoded-value guard to changed lines (#13950)

.github/workflows/code-quality.yml

@@ -191,7 +191,12 @@ jobs:
         echo "$files" | xargs python3 tools/lint/check_no_utcnow_isoformat.py
 
     - name: Block hardcoded value regressions (#694, #6725)
-      run: bash pipeline-scripts/check-pre-commit-hook-pr.sh pre-commit-hardcoded-values
+      # --changed-lines-only (#13950): this hook reads each changed file whole,
+      # so editing one line of a legacy module made the PR answerable for that
+      # file's entire backlog — four PRs in one session failed on violations
+      # blame attributes to other commits. Scoped to added lines here; the
+      # pre-commit stage and a direct run still scan whole files.
+      run: bash pipeline-scripts/check-pre-commit-hook-pr.sh --changed-lines-only pre-commit-hardcoded-values
 
     - name: Block hardcoded-IP fallback regressions (#6783, #6991)
       run: bash pipeline-scripts/check-pre-commit-hook-pr.sh --python tools/lint/check_no_hardcoded_ip_fallbacks.py --ext py

pipeline-scripts/check-pre-commit-hook-pr.sh

@@ -16,9 +16,21 @@
 # set for the PR/push context and invokes the hook in argv mode.
 #
 # Usage:
-#   bash pipeline-scripts/check-pre-commit-hook-pr.sh <hook-name>
+#   bash pipeline-scripts/check-pre-commit-hook-pr.sh [--changed-lines-only] <hook-name>
 #   bash pipeline-scripts/check-pre-commit-hook-pr.sh --python <validator-path> [--ext py,ts,...]
 #
+# --changed-lines-only (#13950)
+#   Report only violations on lines this PR actually added. Without it the hook
+#   reads each changed file whole, so editing one line of a legacy module makes
+#   the PR answerable for that file's entire backlog: four PRs in one session
+#   failed on violations blame attributes to other commits, and in two of them
+#   the constant the message suggested was the wrong concept (an RBAC role and a
+#   ceiling), so the mechanical fix would have gone green while encoding a
+#   different meaning. Opt-in per invocation rather than default, so hooks whose
+#   output this wrapper cannot parse keep their existing behaviour exactly.
+#   Suppressed violations are still printed, and the whole-file backlog stays
+#   visible by running the hook directly.
+#
 # Examples (in GitHub Actions steps):
 #   - run: bash pipeline-scripts/check-pre-commit-hook-pr.sh pre-commit-no-print-console
 #   - run: bash pipeline-scripts/check-pre-commit-hook-pr.sh --python tools/lint/check_no_hardcoded_ip_fallbacks.py
@@ -46,6 +58,7 @@ set -euo pipefail
 USE_PYTHON=false
 VALIDATOR_PATH=""
 HOOK_NAME=""
+CHANGED_LINES_ONLY=false
 # An ARRAY, not a string (#13880). As a string it was passed unquoted to
 # `git diff -- $EXT_GLOB`, so the SHELL expanded the globs against the cwd
 # before git saw them. In CI the cwd is the repo root, where `*.py` matches
@@ -61,6 +74,15 @@ if [ "$#" -lt 1 ]; then
     exit 2
 fi
 
+if [ "$1" = "--changed-lines-only" ]; then
+    CHANGED_LINES_ONLY=true
+    shift
+    if [ "$#" -lt 1 ]; then
+        echo "Usage: $0 --changed-lines-only <hook-name>" >&2
+        exit 2
+    fi
+fi
+
 if [ "$1" = "--python" ]; then
     USE_PYTHON=true
     if [ "$#" -lt 2 ]; then
@@ -186,8 +208,120 @@ if $USE_PYTHON; then
     echo "Running ${VALIDATOR_PATH} against $count changed file(s)..."
     # shellcheck disable=SC2086
     echo "$files" | xargs python3 "$VALIDATOR_PATH"
-else
+elif ! $CHANGED_LINES_ONLY; then
     echo "Running ${HOOK_NAME} against $count changed file(s)..."
     # shellcheck disable=SC2086
     bash "$HOOK_PATH" $files
+else
+    echo "Running ${HOOK_NAME} against $count changed file(s), reporting only added lines..."
+
+    # The set of lines this PR added, as "path:line" keys. -U0 so hunk headers
+    # bound exactly the added lines and nothing either side of them.
+    added_lines=$(
+        for f in $files; do
+            git diff -U0 "$base" "$HEAD_SHA" -- "$f" | awk -v path="$f" '
+                /^@@/ {
+                    # @@ -a,b +c,d @@   (d omitted means 1)
+                    match($0, /\+[0-9]+(,[0-9]+)?/)
+                    spec = substr($0, RSTART + 1, RLENGTH - 1)
+                    n = split(spec, parts, ",")
+                    start = parts[1] + 0
+                    len = (n > 1) ? parts[2] + 0 : 1
+                    for (i = 0; i < len; i++) print path ":" (start + i)
+                }'
+        done
+    ) || {
+        echo "FATAL: could not compute the added-line set — refusing to report clean." >&2
+        exit 1
+    }
+
+    set +e
+    hook_output=$(bash "$HOOK_PATH" $files 2>&1)
+    hook_status=$?
+    set -e
+
+    if [ "$hook_status" -eq 0 ]; then
+        printf '%s\n' "$hook_output"
+        exit 0
+    fi
+
+    # Partition the hook's violation blocks. A block starts at a VIOLATION
+    # header carrying "path:line" and runs to the next blank line.
+    filtered=$(printf '%s\n' "$hook_output" | ADDED="$added_lines" awk '
+        # is_violation distinguishes a real violation block from the hook'"'"'s
+        # banner and trailer. Counting those as kept reported "6 violations on
+        # lines this PR added" for a PR that added one clean line — the summary
+        # would have contradicted the (correct) suppression right above it.
+        function flush_block() {
+            if (block == "") return
+            if (!is_violation) { kept_text = kept_text block }
+            else if (keep)     { kept_text = kept_text block; kept++ }
+            else               { supp_text = supp_text block; suppressed++ }
+            block = ""; keep = 1; is_violation = 0
+        }
+        BEGIN {
+            split(ENVIRON["ADDED"], a, "\n")
+            for (i in a) if (a[i] != "") added[a[i]] = 1
+            keep = 1
+        }
+        {
+            plain = $0
+            gsub(/\033\[[0-9;]*m/, "", plain)
+            # The hook'"'"'s trailer counts what the hook found across whole files.
+            # In scoped mode this wrapper decides, so printing "COMMIT BLOCKED"
+            # next to "not failing the build" would contradict the verdict.
+            if (plain ~ /COMMIT BLOCKED/) next
+            if (plain ~ /VIOLATION[[:space:]]+[^[:space:]]+:[0-9]+/) {
+                flush_block()
+                seen_header++
+                is_violation = 1
+                match(plain, /[^[:space:]]+:[0-9]+/)
+                key = substr(plain, RSTART, RLENGTH)
+                keep = (key in added) ? 1 : 0
+            }
+            block = block $0 "\n"
+            if (plain ~ /^[[:space:]]*$/) flush_block()
+        }
+        END {
+            flush_block()
+            print "@@HEADERS@@" seen_header
+            print "@@KEPT@@" kept
+            print "@@SUPPRESSED@@" suppressed
+            print "@@KEPTTEXT@@"
+            printf "%s", kept_text
+            print "@@SUPPTEXT@@"
+            printf "%s", supp_text
+        }
+    ')
+
+    seen_headers=$(printf '%s\n' "$filtered" | sed -n 's/^@@HEADERS@@//p')
+    kept_count=$(printf '%s\n' "$filtered" | sed -n 's/^@@KEPT@@//p')
+    supp_count=$(printf '%s\n' "$filtered" | sed -n 's/^@@SUPPRESSED@@//p')
+
+    # Fail closed. The hook said "violations" and we parsed none, so the output
+    # format is not one this filter understands. Suppressing an unparsed
+    # failure would turn a real regression into a green build — the precise
+    # failure mode #13880 was about, reached from the other direction.
+    if [ "${seen_headers:-0}" -eq 0 ]; then
+        printf '%s\n' "$hook_output"
+        echo "FATAL: ${HOOK_NAME} failed but produced no parseable 'file:line' violations." >&2
+        echo "  --changed-lines-only cannot scope this output, and refuses to drop it." >&2
+        exit "$hook_status"
+    fi
+
+    printf '%s\n' "$filtered" | sed -n '/^@@KEPTTEXT@@$/,/^@@SUPPTEXT@@$/p' | sed '1d;$d'
+
+    if [ "${supp_count:-0}" -gt 0 ]; then
+        echo "----------------------------------------"
+        echo "${supp_count} pre-existing violation(s) on lines this PR did not touch (#13950):"
+        printf '%s\n' "$filtered" | sed -n '/^@@SUPPTEXT@@$/,$p' | sed '1d'
+        echo "Not failing the build for these. Run the hook directly for the whole-file backlog."
+    fi
+
+    if [ "${kept_count:-0}" -gt 0 ]; then
+        echo "${kept_count} violation(s) on lines this PR added."
+        exit 1
+    fi
+    echo "No violations on lines this PR added."
+    exit 0
 fi

pipeline-scripts/check-pre-commit-hook-pr_test.py

@@ -259,3 +259,107 @@ def test_python_validator_clean_exit_0(self, tmp_path: Path) -> None:
         base, head = _make_pr(tmp_path, {"src/foo.py": "x = 1\n"})
         result = _run_wrapper_python(tmp_path, validator, base, head, ext="py")
         assert result.returncode == 0
+
+
+def _amend_pr(tmp_path: Path, files: dict[str, str]) -> tuple[str, str]:
+    """A repo whose BASE already contains *files*, and whose PR edits one line.
+
+    The point of #13950 is the distinction between "in a file this PR touched"
+    and "on a line this PR added", so the fixture has to be able to express it:
+    the base carries pre-existing content and the PR appends to it.
+    """
+    subprocess.run(["git", "init", "--quiet"], cwd=tmp_path, check=True)
+    subprocess.run(["git", "config", "user.email", "t@t"], cwd=tmp_path, check=True)
+    subprocess.run(["git", "config", "user.name", "t"], cwd=tmp_path, check=True)
+    for rel, content in files.items():
+        path = tmp_path / rel
+        path.parent.mkdir(parents=True, exist_ok=True)
+        path.write_text(content, encoding="utf-8")
+        subprocess.run(["git", "add", rel], cwd=tmp_path, check=True)
+    subprocess.run(
+        ["git", "-c", "commit.gpgsign=false", "commit", "-q", "-m", "base"], cwd=tmp_path, check=True
+    )
+    base = subprocess.run(
+        ["git", "rev-parse", "HEAD"], cwd=tmp_path, capture_output=True, text=True, check=True
+    ).stdout.strip()
+    return base, ""
+
+
+def _commit_pr(tmp_path: Path) -> str:
+    subprocess.run(["git", "add", "-A"], cwd=tmp_path, check=True)
+    subprocess.run(
+        ["git", "-c", "commit.gpgsign=false", "commit", "-q", "-m", "pr"], cwd=tmp_path, check=True
+    )
+    return subprocess.run(
+        ["git", "rev-parse", "HEAD"], cwd=tmp_path, capture_output=True, text=True, check=True
+    ).stdout.strip()
+
+
+def _run_scoped(tmp_path: Path, hook_name: str, base: str, head: str) -> subprocess.CompletedProcess:
+    return subprocess.run(
+        ["bash", str(WRAPPER), "--changed-lines-only", hook_name],
+        cwd=tmp_path,
+        capture_output=True,
+        text=True,
+        env={"BASE_SHA": base, "HEAD_SHA": head, "PATH": "/usr/bin:/bin"},
+    )
+
+
+# A violation the hardcoded-values hook reliably reports: a bare port literal.
+_OFFENDING = '    URL = "http://127.0.0.1:8001/api/health"\n'
+_CLEAN = "    VALUE = compute()\n"
+
+
+@pytest.mark.skipif(not WRAPPER.exists(), reason="wrapper script not found")
+class TestChangedLinesOnly:
+    """#13950: a PR answers for the lines it added, not for the file it touched."""
+
+    def test_a_pre_existing_violation_in_a_touched_file_does_not_fail(self, tmp_path: Path) -> None:
+        """The reported symptom: four PRs red on other commits' violations."""
+        base, _ = _amend_pr(tmp_path, {"m.py": "def f():\n" + _OFFENDING})
+        (tmp_path / "m.py").write_text("def f():\n" + _OFFENDING + _CLEAN, encoding="utf-8")
+        head = _commit_pr(tmp_path)
+
+        result = _run_scoped(tmp_path, "pre-commit-hardcoded-values", base, head)
+
+        assert result.returncode == 0, f"pre-existing violation failed the build:\n{result.stdout}"
+        assert "did not touch" in result.stdout, "the suppressed violation was hidden rather than reported"
+
+    def test_a_violation_the_pr_adds_still_fails(self, tmp_path: Path) -> None:
+        """The property that must survive: this is a scope change, not an off switch.
+
+        Without this, the whole change is indistinguishable from deleting the
+        guard — which is exactly how a loosened check stops catching anything.
+        """
+        base, _ = _amend_pr(tmp_path, {"m.py": "def f():\n" + _CLEAN})
+        (tmp_path / "m.py").write_text("def f():\n" + _CLEAN + _OFFENDING, encoding="utf-8")
+        head = _commit_pr(tmp_path)
+
+        result = _run_scoped(tmp_path, "pre-commit-hardcoded-values", base, head)
+
+        assert result.returncode == 1, f"a newly added violation passed:\n{result.stdout}"
+        assert "lines this PR added" in result.stdout
+
+    def test_a_clean_pr_stays_clean(self, tmp_path: Path) -> None:
+        base, _ = _amend_pr(tmp_path, {"m.py": "def f():\n" + _CLEAN})
+        (tmp_path / "m.py").write_text("def f():\n" + _CLEAN + "    OTHER = g()\n", encoding="utf-8")
+        head = _commit_pr(tmp_path)
+
+        result = _run_scoped(tmp_path, "pre-commit-hardcoded-values", base, head)
+
+        assert result.returncode == 0
+
+    def test_the_default_mode_is_unchanged(self, tmp_path: Path) -> None:
+        """Without the flag, a pre-existing violation still fails.
+
+        The flag is opt-in precisely so every other hook keeps its behaviour;
+        if the default changed too, the blast radius would be every guard that
+        runs through this wrapper.
+        """
+        base, _ = _amend_pr(tmp_path, {"m.py": "def f():\n" + _OFFENDING})
+        (tmp_path / "m.py").write_text("def f():\n" + _OFFENDING + _CLEAN, encoding="utf-8")
+        head = _commit_pr(tmp_path)
+
+        result = _run_wrapper(tmp_path, "pre-commit-hardcoded-values", base, head)
+
+        assert result.returncode == 1