Merge pull request #1969 from vyos/yuriy/ai-validation-cr-followups-rolling

ci(ai-validation): address 4 CR findings on the merged #1957

1 files changed, 61 insertions, 12 deletions

diff --git a/.github/workflows/ai-validation.yml b/.github/workflows/ai-validation.yml
index f75081dc..d5f1160f 100644
--- a/.github/workflows/ai-validation.yml
+++ b/.github/workflows/ai-validation.yml
@@ -59,7 +59,7 @@ jobs:
       has_md_changes: ${{ steps.changes.outputs.has_md_changes }}
     steps:
       - name: Checkout PR merge ref (NO credentials)
-        uses: actions/checkout@v6
+        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
         with:
           ref: refs/pull/${{ github.event.number }}/merge
           persist-credentials: false
@@ -80,6 +80,30 @@ jobs:
           # in changed-md.txt (which drives the bundling step).
           git diff "$BASE...HEAD" --name-only --diff-filter=ACMRT -z -- 'docs/**/*.md'  > changed-md.z
           git diff "$BASE...HEAD" --name-only --diff-filter=ACMRT -z -- 'docs/**/*.rst' > changed-rst.z
+          # Reject paths containing line-disrupting control bytes (LF, CR,
+          # other 0x01-0x1F + 0x7F) before generating the newline-delimited
+          # *.txt manifests. NUL itself can't appear in a git pathname
+          # (it's the on-disk tree-entry terminator), so it stays out of
+          # the rejection class and remains the legitimate record delimiter
+          # for `git diff -z` — `grep -z` honors that contract.
+          #
+          # POSIX filesystems generally allow LF/CR in filenames and git
+          # stores them fine; the hazard is purely in our line-delimited
+          # downstream tooling. Without this guard, `tr '\0' '\n'` on a
+          # path like `docs/foo\nbar.md` would split it into two logical
+          # lines — downstream consumers reading line-by-line would miss
+          # validation coverage on the real file (or worse, act on a
+          # synthetic path). Fail fast at this seam.
+          #
+          # An earlier `tr -d '\0\n\r' | grep [\x00-\x1F\x7F]` form
+          # stripped the very bytes it was meant to reject before the
+          # grep ran — defeating the guard.
+          for z in changed-md.z changed-rst.z; do
+            if LC_ALL=C grep -zPq '[\x01-\x1F\x7F]' "$z"; then
+              echo "::error::Refusing to bundle: path in $z contains a control character. Reject the offending file name in the PR."
+              exit 1
+            fi
+          done
           tr '\0' '\n' < changed-md.z  > changed-md.txt
           tr '\0' '\n' < changed-rst.z > changed-rst.txt
           git diff "$BASE...HEAD" -- 'docs/**/*.md' > diff-md.patch
@@ -110,8 +134,14 @@ jobs:
                || "$path" == "../"* \
                || "$path" == *"/.." \
                || "$path" == ".." ]]; then
-              echo "::warning::Skipping unsafe path with traversal/absolute prefix: $path"
-              continue
+              # Fail-fast (don't `continue`) so an unsafe path can't silently
+              # bypass Pass 1 (no file copied into _changed_md/) and Pass 2
+              # (LLM tools see no content) — same reasoning as the non-regular
+              # tree-entry check below: maintainers must explicitly decide to
+              # land such a path. Visible failure > silent skip on inputs that
+              # warrant the closest look.
+              echo "::error::Refusing to bundle: path has traversal/absolute prefix: $path"
+              exit 1
             fi
             # Refuse to bundle non-regular tree entries (symlinks mode 120000,
             # submodules 160000, etc). Skipping silently would let a PR that
@@ -145,7 +175,7 @@ jobs:
           fi
 
       - name: Upload PR input artifact
-        uses: actions/upload-artifact@v4
+        uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2
         with:
           name: pr-input
           path: |
@@ -164,8 +194,16 @@ jobs:
     runs-on: ubuntu-latest
     permissions:
       contents: read
+      # pull-requests: write is required for inline review comments via
+      # mcp__github_inline_comment__create_inline_comment (Pass 2).
+      # issues: write is required for `gh pr comment` (the skip-notice
+      # step and Pass 2's top-level summary comment) — `gh pr comment`
+      # posts via POST /repos/{owner}/{repo}/issues/{number}/comments,
+      # which the issues scope governs. Granting both keeps every
+      # comment path working on repos where the default GITHUB_TOKEN
+      # permissions split issue and PR scopes.
       pull-requests: write
-      id-token: write
+      issues: write
     steps:
       # Pass secrets via env: rather than inlining ${{ secrets.X }} into the
       # shell script. GitHub Actions template-expands ${{ ... }} BEFORE bash
@@ -209,7 +247,7 @@ jobs:
 
       - name: Download PR input
         if: steps.secrets-check.outputs.skip != 'true'
-        uses: actions/download-artifact@v4
+        uses: actions/download-artifact@d3f86a106a0bac45b974a628896c90dbdf5c8093 # v4.3.0
         with:
           name: pr-input
 
@@ -235,7 +273,7 @@ jobs:
 
       - name: Sparse-checkout branches.json from reviewer
         if: steps.secrets-check.outputs.skip != 'true'
-        uses: actions/checkout@v6
+        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
         with:
           repository: VyOS-Networks/vyos-docs-opus-reviewer
           ref: ${{ env.REVIEWER_REF }}
@@ -266,7 +304,7 @@ jobs:
 
       - name: Checkout vyos-1x at mapped branch
         if: steps.secrets-check.outputs.skip != 'true'
-        uses: actions/checkout@v6
+        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
         with:
           repository: vyos-networks/vyos-1x
           ref: ${{ steps.branch.outputs.vyos1x }}
@@ -285,7 +323,11 @@ jobs:
         uses: robinraju/release-downloader@28fc21f50d76778e7023361aa1f863e717d3d56f # v1.13
         with:
           repository: VyOS-Networks/vyos-docs-opus-reviewer
-          latest: true
+          # Pin the DB to the same release tag as REVIEWER_REF so a future
+          # reviewer-v1.x.x release with a schema change cannot be silently
+          # picked up while the pinned reviewer code still expects the old
+          # schema. Reproducibility > recency for this artifact.
+          tag: ${{ env.REVIEWER_REF }}
           fileName: reference-db-${{ steps.branch.outputs.vyos1x }}.tar.gz
           out-file-path: .reference-db
           token: ${{ steps.app.outputs.token }}
@@ -300,7 +342,14 @@ jobs:
         if: steps.secrets-check.outputs.skip != 'true'
         run: |
           set -euo pipefail
-          if [ -s changed-md.txt ] && [ ! -d .reference-db/extracted ]; then
+          # Gate on diff-md.patch (unfiltered) rather than changed-md.txt
+          # (--diff-filter=ACMRT). The job-level `if: has_md_changes` already
+          # ensures we only reach here when there are MD-related changes —
+          # including deletion-only PRs (where changed-md.txt is empty by
+          # design but diff-md.patch isn't). Using changed-md.txt here would
+          # silently skip the fail-closed check on those PRs, masking a
+          # missing reference DB from the reviewer.
+          if [ -s diff-md.patch ] && [ ! -d .reference-db/extracted ]; then
             echo "::error::Reference DB missing for vyos-1x branch '${{ steps.branch.outputs.vyos1x }}'. Pass 1 cannot run. Re-trigger rebuild-reference.yml in the reviewer repo and re-run."
             exit 1
           fi
@@ -330,7 +379,7 @@ jobs:
       # reviewer-src/.git/config where the Pass 2 LLM step could read it.
       - name: Checkout reviewer package source (pinned to REVIEWER_REF)
         if: steps.secrets-check.outputs.skip != 'true'
-        uses: actions/checkout@v6
+        uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2
         with:
           repository: VyOS-Networks/vyos-docs-opus-reviewer
           ref: ${{ env.REVIEWER_REF }}
@@ -359,7 +408,7 @@ jobs:
 
       - name: Pass 2 — Claude review
         if: steps.secrets-check.outputs.skip != 'true'
-        uses: anthropics/claude-code-action@v1
+        uses: anthropics/claude-code-action@476e359e6203e73dad705c8b322e333fabbd7416 # v1.0.119
         with:
           anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }}
           # claude-code-action@v1 removed the top-level `model` input; CLI