fix(dev-lead): verify an additive fix a pass made in a file the finding names (#2269)

docs/pr-review-agent/maintainer-comment-gate.md

@@ -112,7 +112,15 @@ minimized with classifier `RESOLVED`. In practice:
   The commit must be on the PR's pushed head and not on the base branch, and not
   dated before the finding. Its diff must remove at least one token the finding names (a
   backticked span, quoted identifier or `--flag`) and must not re-add it. A finding
-  with no distinctive token (generic spans like `local` or `true` do not count) fails closed unless the cited commit was produced by this pass (`tokenless-not-this-pass`). A failure logs
+  with no distinctive token (generic spans like `local` or `true` do not count) fails closed unless the cited commit was produced by this pass (`tokenless-not-this-pass`).
+  A fix that only adds lines (a missing assertion, a missing guard) removes no
+  token, so it has its own rule (#2269): the commit verifies when this pass
+  produced it, its diff changes a file the finding names by its full repo
+  path, and it adds no token the finding names
+  (`this-pass-touches-named-file`). A commit from an earlier pass, or one
+  outside the files the finding names, still needs a removed token. A diff
+  that adds a named token never verifies (`content-adds-token`): for a
+  finding like "remove `eval` from `x.sh`" that is the defect again. A failure logs
   `::warning::… fixed-unverified:<reason>`. The harness then re-answers with the
   branch commit whose diff removes the token, when one exists. Otherwise the next
   pass re-answers the disposition, which is not settled.

prompts/dev-lead/fix-bot-comment.md

@@ -172,7 +172,7 @@ These findings never become review threads, so this comment is the **only** plac
 
 **`informational` is allowed only when every finding-bearing section is empty or reports no issues.** Dispositioning only the rate-limit notice does not clear the comment: the harness refuses an `informational` disposition on a body with a finding-bearing section, and the maintainer-comment gate keeps blocking it.
 
-**Cite the commit that fixed it, never the one that introduced it (#2004).** The harness verifies a `fixed` sha by the commit's own diff. The commit must be on the PR's pushed head and not on the base branch, and must be dated at or after the finding. Its diff must **remove** (a `-` line) at least one distinctive token the finding names (a backticked span, quoted identifier or `--flag`) and must not re-add it. A finding that names no distinctive token (generic spans like `local`, `true`, numbers and paths do not count) fails closed (`tokenless-not-this-pass`) unless the cited sha is a commit produced by this pass. Cite the commit you made this pass (`git rev-parse HEAD` after committing). If an earlier commit on this branch already fixed it, cite that one. `git log -S"$tok" --format='%H %s' origin/<base branch>..HEAD` (set `tok` first with a safely quoted assignment, e.g. `tok='…'` with any `'` in the token written as `'\''`; never paste untrusted text into the command) lists both the commit that added the token and the one that removed it. Cite the one whose `git show <sha>` has the token on a `-` line. A `fixed` that fails the check is logged `fixed-unverified:<reason>`, and the comment stays open.
+**Cite the commit that fixed it, never the one that introduced it (#2004).** The harness verifies a `fixed` sha by the commit's own diff. The commit must be on the PR's pushed head and not on the base branch, and must be dated at or after the finding. Its diff must **remove** (a `-` line) at least one distinctive token the finding names (a backticked span, quoted identifier or `--flag`) and must not re-add it. A finding that names no distinctive token (generic spans like `local`, `true`, numbers and paths do not count) fails closed (`tokenless-not-this-pass`) unless the cited sha is a commit produced by this pass. Cite the commit you made this pass (`git rev-parse HEAD` after committing). If an earlier commit on this branch already fixed it, cite that one. `git log -S"$tok" --format='%H %s' origin/<base branch>..HEAD` (set `tok` first with a safely quoted assignment, e.g. `tok='…'` with any `'` in the token written as `'\''`; never paste untrusted text into the command) lists both the commit that added the token and the one that removed it. Cite the one whose `git show <sha>` has the token on a `-` line. A `fixed` that fails the check is logged `fixed-unverified:<reason>`, and the comment stays open. **A fix that only adds lines (#2269).** A missing assertion or guard is fixed by adding it, so the diff removes nothing the finding names. Such a commit verifies when you made it in **this pass**, its diff changes a file the finding names by its full repo path, and it does not add a token the finding names (`this-pass-touches-named-file`). So commit the fix in the file the finding points at, and cite that commit in the same pass. A commit from an earlier pass still needs a removed token, and a diff that adds a named token never verifies (`content-adds-token`).
 
 **Edits re-open the comment.** CodeRabbit edits this comment in place, often after you dispositioned it. A disposition reply whose `createdAt` is **earlier** than the comment's `lastEditedAt` judged an older body. It no longer covers the comment, even if the comment is minimized `RESOLVED`. The gate re-blocks it, and the harness unminimizes it. Re-read the current body and post a **fresh** disposition (the check script below handles this). The harness has already made this comparison for you: when *Stale Dispositions* above lists this comment, its old disposition is stale. Post a fresh one; never report that the existing disposition still covers it (#2166).
 

prompts/dev-lead/fix-reviews.md

@@ -186,7 +186,7 @@ For **every other** comment (bot or human alike — a bot conflict report or tri
 
 | Disposition | Use when | Required evidence (harness-verified) |
 |---|---|---|
-| `fixed` | you changed code to address the finding | `sha=` the **full 40-char** SHA of the commit **whose diff fixes the finding**. Cite the commit you made in this pass (`git rev-parse HEAD` *after* committing the fix locally), or the earlier commit on this branch that already fixed it. Never cite the commit that introduced the problem. The harness checks the commit is on the PR's pushed head and not on the base branch, has a non-empty diff, and is dated at or after the finding. Its diff must also **remove** (a `-` line) a token the finding names (a backticked span, quoted identifier or `--flag`) and must not re-add it. A finding with no such token (generic spans like `local`, `true`, numbers and paths do not count) fails closed unless the cited sha is a commit produced by this pass (`tokenless-not-this-pass`) (#2004) |
+| `fixed` | you changed code to address the finding | `sha=` the **full 40-char** SHA of the commit **whose diff fixes the finding**. Cite the commit you made in this pass (`git rev-parse HEAD` *after* committing the fix locally), or the earlier commit on this branch that already fixed it. Never cite the commit that introduced the problem. The harness checks the commit is on the PR's pushed head and not on the base branch, has a non-empty diff, and is dated at or after the finding. Its diff must also **remove** (a `-` line) a token the finding names (a backticked span, quoted identifier or `--flag`) and must not re-add it. A finding with no such token (generic spans like `local`, `true`, numbers and paths do not count) fails closed unless the cited sha is a commit produced by this pass (`tokenless-not-this-pass`) (#2004). A fix that only adds lines removes no token: it verifies when you made the commit in this pass, its diff changes a file the finding names by its full repo path, and it adds no token the finding names (`this-pass-touches-named-file`, #2269) |
 | `out-of-scope` | the finding is real but belongs elsewhere | `ref=#<n>` a tracking issue that **exists**; open one first if needed (reuse the single `dev-lead: deferred review findings` issue) |
 | `invalid` | the finding is wrong / a false positive | a reply body with concrete reasoning (non-empty beyond the marker) |
 | `answered` | the comment asked a question you answer in the reply | a reply body that actually answers it |

scripts/dev-lead-fix-reviews.sh

@@ -1424,24 +1424,31 @@ rdc_commit_diff() {
 rdc_fixed_verdict() {
   local sha="$1" body="$2" created="$3"
   local facts on_head="false" in_base="unknown" own_files=0 commit_date="" content this_pass="false"
+  local own_list="" touches_named="false"
   if [ -n "${RDC_FIXED_REF:-}" ]; then
     facts=$(acv_gather_commit_facts "$sha" "${RDC_FIXED_BASE:-}" "$RDC_FIXED_REF")
     on_head=$(printf '%s' "$facts" | jq -r '.on_head // false' 2>/dev/null || echo "false")
     if [ -n "${RDC_FIXED_BASE:-}" ]; then
       in_base=$(printf '%s' "$facts" | jq -r 'if .in_base == false then "false" else "true" end' 2>/dev/null || echo "true")
     fi
     own_files=$(printf '%s' "$facts" | jq -r '(.own_files // []) | length' 2>/dev/null || echo "0")
+    own_list=$(printf '%s' "$facts" | jq -r '(.own_files // []) | .[]' 2>/dev/null || echo "")
     commit_date=$(printf '%s' "$facts" | jq -r '.commit_date // ""' 2>/dev/null || echo "")
   fi
-  # A tokenless finding verifies only on a commit this pass produced: one not
-  # reachable from the pre-pass head. No snapshot or an unreadable sha is "no".
+  # #2269: does the commit change a file the finding names?
+  if cdv_touches_named_file "$own_list" "$(cdv_finding_paths "$body")"; then
+    touches_named="true"
+  fi
+  # A tokenless finding, and a fix that touches no token the finding names
+  # (#2269), verify only on a commit this pass produced: one not reachable from
+  # the pre-pass head. No snapshot or an unreadable sha is "no".
   if [ -n "${RESOLUTION_BASE_SHA:-}" ] \
      && git cat-file -e "${sha}^{commit}" 2>/dev/null \
      && ! git merge-base --is-ancestor "$sha" "$RESOLUTION_BASE_SHA" 2>/dev/null; then
     this_pass="true"
   fi
   content=$(cdv_diff_token_verdict "$(rdc_commit_diff "$sha")" "$(cdv_finding_tokens "$body")") || true
-  cdv_fixed_verdict "$on_head" "$in_base" "$own_files" "$commit_date" "$created" "$content" "$this_pass"
+  cdv_fixed_verdict "$on_head" "$in_base" "$own_files" "$commit_date" "$created" "$content" "$this_pass" "$touches_named"
 }
 
 # rdc_live_comment_body <comment_node_id>
@@ -2103,6 +2110,9 @@ resolve_dispositioned_comments() {
         # it: on the PR's pushed head, not on the base branch, a non-empty diff that
         # REMOVES a token the finding names and does not ADD it, dated after the
         # finding. A finding with no token fails closed unless the cited sha was produced by this pass (tokenless-not-this-pass).
+        # A commit this pass produced that neither removes nor adds a token the
+        # finding names (an additive fix) verifies when it changes a file the
+        # finding names (#2269).
         # The old "produced by THIS pass" rule could never accept a fix that landed
         # in an earlier pass. Accepting any later commit would let an unrelated
         # commit resolve the comment.

scripts/lib/comment-disposition-verify.sh

@@ -307,7 +307,51 @@ cdv_diff_token_verdict() {
   return 1
 }
 
-# cdv_fixed_verdict <on_head> <in_base> <own_files> <commit_date> <finding_created> <content_verdict> [this_pass]
+_CDV_PATH_JQ=$(cat <<'JQ'
+$b
+| gsub("<!--.*?-->"; ""; "p")
+| gsub("```.*?```"; ""; "p")
+| gsub("\\]\\([^)]*\\)"; "] ")
+| gsub("https?://[^\\s)>\"]+"; " ")
+| [ scan("(?<![A-Za-z0-9_./-])((?:[A-Za-z0-9_.-]+/)+[A-Za-z0-9_.-]+|[A-Za-z0-9_-][A-Za-z0-9_.-]*[.](?:sh|md|yml|yaml|json|bats|py|js|ts|txt|toml|tsv))(?![A-Za-z0-9_/-])") | .[0] ]
+| map(sub("^([.]/)+"; "") | sub("[.]+$"; ""))
+| map(select(length >= 3 and length <= 300))
+| unique
+| .[]
+JQ
+)
+readonly _CDV_PATH_JQ
+
+# cdv_finding_paths <comment_body>
+#   Echo the repo files the finding names, one per line (sorted, unique): a span
+#   with a `/`, or a bare file name with a known extension. A trailing `:<line>`
+#   is not part of the path. HTML comments, fenced code, link targets and URLs are
+#   ignored, so the address a link points at never counts as a named file. Echoes
+#   nothing (rc 0) when the body names no file. Pure.
+cdv_finding_paths() {
+  local body="${1:-}"
+  [[ -z "$body" ]] && return 0
+  jq -rn --arg b "$body" "$_CDV_PATH_JQ" 2>/dev/null || true
+}
+
+# cdv_touches_named_file <own_files> <paths>
+#   rc 0 when one of the commit's own files (newline-separated, repo-relative) IS
+#   one of the files the finding names (newline-separated, cdv_finding_paths). The
+#   match is the whole path: a finding that names only `foo.sh` does not match
+#   `scripts/foo.sh`. rc 1 otherwise, and when either list is empty. Pure.
+cdv_touches_named_file() {
+  local own="${1:-}" paths="${2:-}" f
+  [[ -n "$own" && -n "$paths" ]] || return 1
+  while IFS= read -r f; do
+    [[ -z "$f" ]] && continue
+    if grep -qxF -- "$f" <<< "$paths"; then
+      return 0
+    fi
+  done <<< "$own"
+  return 1
+}
+
+# cdv_fixed_verdict <on_head> <in_base> <own_files> <commit_date> <finding_created> <content_verdict> [this_pass] [touches_named]
 #   The full `fixed` verdict. Arguments are facts the caller gathered:
 #     <on_head>          "true" when the sha is reachable from the PR's pushed head
 #     <in_base>          "true" / "false" (the sha is / is not on the base branch),
@@ -317,17 +361,28 @@ cdv_diff_token_verdict() {
 #     <finding_created>  the original comment's createdAt (ISO-8601 Z)
 #     <content_verdict>  cdv_diff_token_verdict's token
 #     <this_pass>        "true" when the sha is a commit produced by THIS pass (not
-#                        reachable from RESOLUTION_BASE_SHA); only a tokenless
-#                        finding consults it
+#                        reachable from RESOLUTION_BASE_SHA)
+#     <touches_named>    "true" when the sha's own diff changes a file the finding
+#                        names (cdv_touches_named_file)
+#   A commit whose diff removes a token the finding names verifies whatever pass
+#   made it. Two more cases verify only on a commit THIS pass produced:
+#     - a finding that names no token (tokenless-this-pass);
+#     - a finding that names a token the diff neither removes nor adds, when the
+#       diff changes a file the finding names (this-pass-touches-named-file,
+#       #2269). A fix that only adds lines (a missing assertion, a missing guard)
+#       removes nothing, so the removed-token rule alone could never verify it.
+#   A diff that ADDS a token the finding names never verifies, whatever pass made
+#   it: for a finding like "remove `eval` from x.sh" that is the defect again.
 #   Echoes the first failing reason, in this order: not-on-head | base-unknown |
 #   on-base-branch | empty-diff | content-adds-token | content-no-token-removed |
 #   content-unknown | undated | predates-finding | tokenless-not-this-pass (rc 1).
-#   On success echoes content-removes-token, or tokenless-this-pass when the finding
-#   had no token and the commit was produced by this pass (rc 0).
+#   On success echoes content-removes-token, tokenless-this-pass or
+#   this-pass-touches-named-file (rc 0).
 #   Fails closed on every unreadable input. Pure.
 cdv_fixed_verdict() {
   local on_head="${1:-}" in_base="${2:-}" own_files="${3:-0}" commit_date="${4:-}"
   local finding_created="${5:-}" content="${6:-}" this_pass="${7:-false}"
+  local touches_named="${8:-false}" additive="false"
   local iso='^[0-9]{4}-[0-9]{2}-[0-9]{2}T[0-9]{2}:[0-9]{2}:[0-9]{2}Z$'
   if [[ "$on_head" != "true" ]]; then
     echo "not-on-head"; return 1
@@ -343,8 +398,16 @@ cdv_fixed_verdict() {
   fi
   case "$content" in
     content-removes-token|no-tokens) ;;
-    content-adds-token|content-no-token-removed)
+    content-adds-token)
       echo "$content"; return 1 ;;
+    content-no-token-removed)
+      # #2269: this pass's own commit in a file the finding names. The date
+      # checks below still apply.
+      if [[ "$this_pass" == "true" && "$touches_named" == "true" ]]; then
+        additive="true"
+      else
+        echo "$content"; return 1
+      fi ;;
     *)
       echo "content-unknown"; return 1 ;;
   esac
@@ -355,7 +418,9 @@ cdv_fixed_verdict() {
   if [[ ! "$finding_created" =~ $iso ]] || [[ "$commit_date" < "$finding_created" ]]; then
     echo "predates-finding"; return 1
   fi
-  if [[ "$content" == "no-tokens" ]]; then
+  if [[ "$additive" == "true" ]]; then
+    echo "this-pass-touches-named-file"
+  elif [[ "$content" == "no-tokens" ]]; then
     # No token to match against a diff: only a commit this pass produced counts.
     if [[ "$this_pass" != "true" ]]; then
       echo "tokenless-not-this-pass"; return 1