From 5a5b369e06bb14b5422229e85a32c6035d7b3d39 Mon Sep 17 00:00:00 2001 From: henleda Date: Wed, 29 Jul 2026 21:06:12 -0500 Subject: [PATCH 1/2] feat(ci): K2 scan a pull request's diff and comment the band-aid MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `.github/actions/vpcopilot-scan/` (composite action), `.github/workflows/pr-review.yml`, `src/vpcopilot/ci.py`, `vpcopilot ci-review`, and `docs/CI.md`. Scans a PR's diff against the merge base and leaves one comment carrying, per finding above a severity threshold, the F5 XC control triage routed it to and the generated policy name. A PR with nothing above the threshold posts nothing. Measured live on the Nimbus fixture: a deliberately vulnerable /api/refund route produced three findings (critical negative-amount, critical SQLi, high missing authorization) and four policies across service_policy, waf, malicious_user and rate_limit, in 76 s against a three-minute budget. The acceptance criterion contradicted itself and this resolves it honestly. It asked for one comment carrying the policy AND the simulation result while also requiring never writes to XC from CI. G2's blast-radius measurement creates a throwaway policy, attaches it to a load balancer, replays traffic through it and deletes it — three tenant writes — and G1, an offline evaluator, was deliberately deferred. So a would-block count is not computed in CI. The comment reports blast radius only from a real tenant run's simulation.json, and otherwise says that no measurement was made and what it would take. Neither silence nor 0% would do: both read as measured-and-safe. Never writes to XC by construction: ci.py imports no xc, apply, refiner or simulate, pinned by a test that reads the module's own source. The action declares no XC inputs. The defect that would have shipped a silent all-clear: `git diff` answers relative to the repository root while the scanner matches relative to `repo-path`, and this project's app fixture lives eight levels down — so nothing matched, nothing was scanned, and the PR was told it was clean. `rebase_onto` translates the paths and the comment discloses what fell outside. Additive plumbing only: `collect_files(..., only=)` and `run_pipeline(..., only_files=)` filter the existing walk, so the existing call path is unchanged and a changed file that is vendored, unsupported or oversized is still excluded and reported exactly as in a full scan. 735 tests pass offline, ruff clean, coverage 79%. Co-Authored-By: Claude Opus 5 (1M context) --- .github/actions/vpcopilot-scan/action.yml | 198 ++++++ .github/workflows/pr-review.yml | 61 ++ ROADMAP.md | 102 ++- bench/fixtures/ci/README.md | 27 + bench/fixtures/ci/refund-route.js | 15 + docs/CI.md | 169 +++++ docs/USAGE.md | 19 + src/vpcopilot/ci.py | 423 ++++++++++++ src/vpcopilot/cli.py | 72 +++ src/vpcopilot/pipeline.py | 7 +- src/vpcopilot/repo_scan.py | 13 +- tests/test_ci.py | 751 ++++++++++++++++++++++ 12 files changed, 1848 insertions(+), 9 deletions(-) create mode 100644 .github/actions/vpcopilot-scan/action.yml create mode 100644 .github/workflows/pr-review.yml create mode 100644 bench/fixtures/ci/README.md create mode 100644 bench/fixtures/ci/refund-route.js create mode 100644 docs/CI.md create mode 100644 src/vpcopilot/ci.py create mode 100644 tests/test_ci.py diff --git a/.github/actions/vpcopilot-scan/action.yml b/.github/actions/vpcopilot-scan/action.yml new file mode 100644 index 0000000..4bfbba2 --- /dev/null +++ b/.github/actions/vpcopilot-scan/action.yml @@ -0,0 +1,198 @@ +name: "Virtual Patch Copilot — PR scan" +description: >- + Scan a pull request's diff for vulnerabilities and comment the F5 Distributed Cloud virtual patch + that would hold each one closed while the code fix ships. Never touches an XC tenant. +author: "virtual-patch-copilot contributors" +branding: + icon: shield + color: blue + +inputs: + repo-path: + description: "Directory within the checkout to scan (default: the repository root)." + required: false + default: "." + base: + description: >- + Branch the PR targets. The diff is taken against the MERGE BASE of this and the head, so + commits that landed on the base since the branch point are not counted as the PR's changes. + required: false + default: "" + min-severity: + description: "Report findings at or above this severity: critical, high, medium or low." + required: false + default: "high" + min-confidence: + description: "Drop verified findings below this confidence (0-1)." + required: false + default: "0.5" + max-files: + description: >- + Cap on changed files scanned. A PR diff is small and each file costs model calls, so this is + deliberately much lower than a full scan's default of 200. + required: false + default: "40" + comment: + description: "Post (or update) the review comment on the pull request." + required: false + default: "true" + fail-on-findings: + description: >- + Fail the check when anything is reported. Default false: the comment is the deliverable, and a + red check on a finding the team has decided to accept is how a useful bot gets switched off. + required: false + default: "false" + anthropic-api-key: + description: "Model credential for the scan. Pass a secret; never hardcode." + required: true + github-token: + description: "Token used to comment. The default GITHUB_TOKEN needs pull-requests: write." + required: false + default: ${{ github.token }} + python-version: + description: "Python to run under." + required: false + default: "3.12" + simulation-json: + description: >- + Optional path to a `simulation.json` from a REAL tenant run. Blast radius cannot be measured in + CI — it requires attaching a policy to a load balancer — so supply one here to have the + would-block numbers appear in the comment. Without it the comment says no measurement was made, + which is the honest answer rather than an implied all-clear. + required: false + default: "" + +outputs: + reported: + description: "How many findings were reported at or above the threshold." + value: ${{ steps.review.outputs.reported }} + comment-path: + description: "Path to the rendered comment markdown." + value: ${{ steps.review.outputs.comment-path }} + +runs: + using: composite + steps: + - name: Set up Python + uses: actions/setup-python@v5 + with: + python-version: ${{ inputs.python-version }} + + - name: Install vpcopilot + shell: bash + run: | + # This is a LOCAL action: it installs vpcopilot from the checkout it ships in, so the + # version reviewing a PR is the version in that PR. `github.action_path` is + # `.github/actions/vpcopilot-scan`, three levels below the root — resolved rather than passed + # to pip with `..` segments, which pip accepts inconsistently across versions. + root="$(cd "${{ github.action_path }}/../../.." && pwd)" + echo "installing vpcopilot from ${root}" + python -m pip install --quiet -e "${root}[deploy]" + + - name: Check out enough history for a merge base + shell: bash + run: | + # `base` is documented as a BRANCH NAME, but `origin/main` is the natural thing to pass (it is + # the CLI's own default), and blindly prefixing would ask git for `origin/origin/main`. Strip + # a leading remote prefix so both forms work. + BASE_REF="${BASE_REF_RAW#origin/}" + echo "BASE_REF=${BASE_REF}" >> "$GITHUB_ENV" + # actions/checkout fetches a single commit by default, which leaves `git merge-base` + # with nothing to find. Deepen rather than demand fetch-depth: 0 in every caller. + git -C "${{ github.workspace }}" rev-parse --verify HEAD >/dev/null + if ! git -C "${{ github.workspace }}" merge-base HEAD "origin/${BASE_REF}" >/dev/null 2>&1; then + echo "deepening the checkout so a merge base exists" + git -C "${{ github.workspace }}" fetch --quiet --deepen=200 origin "${BASE_REF}" || true + git -C "${{ github.workspace }}" fetch --quiet origin "${BASE_REF}" || true + fi + env: + BASE_REF_RAW: ${{ inputs.base != '' && inputs.base || github.event.pull_request.base.ref || github.event.repository.default_branch }} + + - name: Stage the simulation result, if one was supplied + if: inputs.simulation-json != '' + shell: bash + env: + SIM_JSON: ${{ inputs.simulation-json }} + run: | + # Through env, not interpolated into the command: a `${{ }}` expansion inside `run:` is + # substituted as TEXT before bash sees it, so any input carrying shell metacharacters is a + # script-injection vector. These inputs come from the workflow author rather than a PR, but + # the safe form costs nothing and the habit is what matters. + mkdir -p out-ci + cp "$SIM_JSON" out-ci/simulation.json + echo "blast-radius numbers will come from $SIM_JSON" + + - name: Review the diff + id: review + shell: bash + working-directory: ${{ github.workspace }} + env: + ANTHROPIC_API_KEY: ${{ inputs.anthropic-api-key }} + GITHUB_TOKEN: ${{ inputs.github-token }} + BASE_REF_RAW: ${{ inputs.base != '' && inputs.base || github.event.pull_request.base.ref || github.event.repository.default_branch }} + PR_NUMBER: ${{ github.event.pull_request.number }} + # Every input reaches bash through the environment rather than a `${{ }}` expansion inside + # `run:`, which is substituted as TEXT before bash parses the line. + REPO_PATH: ${{ inputs.repo-path }} + MIN_SEVERITY: ${{ inputs.min-severity }} + MIN_CONFIDENCE: ${{ inputs.min-confidence }} + MAX_FILES: ${{ inputs.max-files }} + WANT_COMMENT: ${{ inputs.comment }} + FAIL_ON_FINDINGS: ${{ inputs.fail-on-findings }} + GH_REPO_SLUG: ${{ github.repository }} + run: | + set -o pipefail + # Self-contained rather than relying on the previous step's GITHUB_ENV: `origin/main` is the + # natural thing to pass for `base` (it is the CLI's own default) and would otherwise become + # `origin/origin/main`. + BASE_REF="${BASE_REF_RAW#origin/}" + args=(--repo "$REPO_PATH" + --base "origin/${BASE_REF}" + --out out-ci + --min-severity "$MIN_SEVERITY" + --min-confidence "$MIN_CONFIDENCE" + --max-files "$MAX_FILES" + --comment-out out-ci/pr-comment.md) + if [ "$WANT_COMMENT" = "true" ] && [ -n "${PR_NUMBER}" ]; then + args+=(--post --pr-repo "$GH_REPO_SLUG" --pr "${PR_NUMBER}") + fi + # ci-review exits 1 when it reports a finding, which is information rather than a failure — + # `fail-on-findings` decides whether the check goes red. Exit 2 is a real error and always + # fails. + set +e + vpcopilot ci-review "${args[@]}" + rc=$? + set -e + if [ "$rc" -ge 2 ]; then + echo "ci-review failed (exit $rc)" + exit "$rc" + fi + echo "reported=$([ "$rc" -eq 1 ] && echo 1 || echo 0)" >> "$GITHUB_OUTPUT" + echo "comment-path=out-ci/pr-comment.md" >> "$GITHUB_OUTPUT" + if [ "$rc" -eq 1 ] && [ "$FAIL_ON_FINDINGS" = "true" ]; then + echo "failing the check because fail-on-findings is true" + exit 1 + fi + exit 0 + + - name: Summary + if: always() + shell: bash + env: + # `if: always()` means this also runs when the review CRASHED. Without knowing which, it + # wrote "no findings at or above the threshold" for a review that never completed — a clean + # bill of health for something that was never looked at, in the one place a human reads. + REVIEW_OUTCOME: ${{ steps.review.outcome }} + run: | + if [ -f out-ci/pr-comment.md ]; then + cat out-ci/pr-comment.md >> "$GITHUB_STEP_SUMMARY" + elif [ "$REVIEW_OUTCOME" = "success" ]; then + echo "No findings at or above the threshold — nothing was posted." >> "$GITHUB_STEP_SUMMARY" + else + { + echo "## ⚠️ The review did not complete" + echo + echo "\`vpcopilot ci-review\` exited \`${REVIEW_OUTCOME}\` and produced no comment." + echo "**This is not a clean bill of health** — the diff was not reviewed. See the step log." + } >> "$GITHUB_STEP_SUMMARY" + fi diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml new file mode 100644 index 0000000..67452e1 --- /dev/null +++ b/.github/workflows/pr-review.yml @@ -0,0 +1,61 @@ +name: pr-review + +# K2 — scan the diff on a pull request and comment the proposed band-aid. +# +# Deliberately NOT part of `ci.yml`: that workflow is the test gate and must stay green, fast and +# free of model credentials. This one spends model calls and needs write access to comment, so it is +# its own workflow with its own permissions and can be disabled without touching the tests. +# +# `pull_request` (not `pull_request_target`) is the safe trigger: it runs with a read-only token +# against the merge commit and cannot be used by a fork to exfiltrate secrets. The consequence is +# that PRs from forks get no secrets and therefore no review — the right trade, and stated in +# docs/CI.md rather than discovered. + +on: + pull_request: + paths: + # Only worth spending a model call when code actually changed. + - "**/*.py" + - "**/*.js" + - "**/*.ts" + - "**/*.jsx" + - "**/*.tsx" + - "**/*.go" + - "**/*.rb" + - "**/*.java" + workflow_dispatch: + +permissions: + contents: read + pull-requests: write # to leave the review comment, and nothing else + +concurrency: + # A pushed branch cancels its own in-flight review rather than racing two comments. + group: pr-review-${{ github.event.pull_request.number || github.ref }} + cancel-in-progress: true + +jobs: + review: + # A fork PR has no access to secrets, so the scan would fail for want of a model key. Skip + # loudly-by-omission rather than fail a check nobody can fix from a fork. + if: github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'workflow_dispatch' + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + with: + # `git merge-base` needs history on both sides; the default single-commit fetch has none. + fetch-depth: 0 + + - name: Review the diff + uses: ./.github/actions/vpcopilot-scan + with: + # The demo app this repo carries. Point this at your own source directory. + repo-path: bench/fixtures/nimbus-vuln-lab/app/src/app/api + min-severity: high + max-files: 40 + comment: "true" + # The comment is the deliverable; a red check on an accepted finding is how a useful bot + # gets switched off. + fail-on-findings: "false" + anthropic-api-key: ${{ secrets.ANTHROPIC_API_KEY }} + github-token: ${{ github.token }} diff --git a/ROADMAP.md b/ROADMAP.md index a617d7b..f9082cc 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -876,12 +876,102 @@ sees the trail. J1–J4 are the open `BACKLOG.md` evidence entries, scheduled; * - Note: pairs with the vendor's own Distributed Cloud MCP server effort. Keep them independent. This one exposes the pipeline, not the tenant — `mcp.py` never imports `xc`, pinned by a test. -- [ ] **K2** GitHub Action. (M, P2) Depends on G2. - Scan the diff on a pull request and comment each new finding above a severity threshold - with the proposed band-aid and its would-block count. - - Acceptance: a PR introducing a known flaw gets one comment carrying the policy and the - simulation result; a PR with no new findings posts nothing; runs in under three minutes - on the Nimbus repo; never writes to XC from CI. +- [x] **K2** GitHub Action. (M, P2) — **DONE:** `.github/actions/vpcopilot-scan/` (a composite + action), `.github/workflows/pr-review.yml`, `src/vpcopilot/ci.py`, `vpcopilot ci-review`, and + `docs/CI.md`. Scans a pull request's diff against the **merge base** and leaves one comment + carrying, per finding above a threshold, the F5 XC control triage routed it to and the generated + policy name. Verified live against the Nimbus fixture. 40 tests. + - **Acceptance, as met:** a PR introducing a known flaw gets **one** comment carrying the policy ✅ + (live: a deliberately vulnerable `/api/refund` route → three findings, four policies across + `service_policy`, `waf`, `malicious_user` and `rate_limit`); a PR with no findings above the + threshold posts **nothing** ✅ — silence is the correct output, because a bot that says "all + clear" on every PR trains people to stop reading it; **75–77 s** on the Nimbus repo against a + three-minute budget ✅; never writes to XC ✅, structurally (below). + - **The acceptance criterion contradicted itself, and this is the resolution.** It asked for one + comment carrying the policy *and the simulation result* while also requiring *never writes to XC + from CI*. Both cannot hold: G2's blast-radius measurement creates a throwaway policy object, + **attaches it to a load balancer**, replays recorded traffic through it and deletes it — three + tenant writes — and there is no offline evaluator to fall back on, because G1 was deliberately + deferred. So a would-block count is **not computed in CI**. The comment reports blast radius only + from a `simulation.json` produced by a real tenant run (committed, or passed via the + `simulation-json` input), and otherwise says in as many words that no measurement was made and + what it would take. Neither silence nor `0%` would do: both read as *measured and safe*. Same + precedent as G2's own rewritten criterion, G4's reproducibility, and I2's conflict criterion. + - **The defect that would have shipped a silent all-clear.** `git diff --name-only` answers relative + to the **repository root**; `collect_files` matches relative to the directory it is given; and this + project's own app fixture lives eight levels down. Compared directly the two never match — zero + files scanned, no findings, and the pull request told it is clean. Catching it requires noticing + the *absence* of an error, which is the hardest kind of bug to see in a review. `rebase_onto()` + translates the paths and returns a count of changed files that fall outside the scanned directory, + which the comment discloses. + - **Never writes to XC, by construction rather than by care.** `ci.py` imports no `xc`, no `apply`, + no `refiner`, no `simulate` and no `promotion_gate`, so there is no code path from CI to a load + balancer — pinned by a test that reads the module's own source, which is the only version of that + guarantee that survives someone adding a convenient import later. The action declares no XC + inputs, so there is nothing to pass one through, and `ci-review` says so out loud if it finds an + XC credential in its environment, because it has no use for one. + - **Every boundary is disclosed rather than left to be assumed** — the theme of H2 and H3, applied + to a comment a developer reads in ten seconds: changed files outside the scanned directory, files + over the size cap or beyond `--max-files`, findings held back by the threshold (counted when + nothing is reported, so "we did not report this" never reads as "there was nothing"), deleted + files, and the absent blast radius. The all-clear branch carries the unscanned remainder too. + - **Decisions.** *Additive plumbing, not a new scan path*: `collect_files(..., only=)` and + `run_pipeline(..., only_files=)` filter the existing walk, so a changed file that is vendored, + unsupported or oversized is still excluded and reported exactly as in a full scan, and the + existing call path is untouched. *No cure drafting* — the developer is editing the file by hand + right now; the band-aid and the finding are what CI can add, and drafting is the expensive half. + *One comment, updated in place*, anchored on a hidden marker, because a branch pushed ten times + should not produce ten comments. *`fail-on-findings` defaults to false* — the comment is the + deliverable, and a red check on a finding the team has decided to accept is how a useful bot gets + switched off. *`pull_request`, never `pull_request_target`*: the latter runs trusted workflow code + with secrets against untrusted head code, which is the standard way a repository leaks its + secrets. The consequence — fork PRs get no review — is documented, not discovered. + - **Found by adversarial review, before shipping** (21 raised across four failure dimensions; 12 + verified — 2 confirmed and 10 refuted, again mostly because they had already been fixed while the + review ran — plus 9 lower-severity ones triaged after). Almost every real finding was one shape: + **a failure rendering as an all-clear.** + - **Refuted candidates were being reported as findings.** `findings.json` is the *discover* + contract — every candidate, including the ones `verify` refuted as false positives — and the + filter compared that set against itself, so it was a no-op. Triage runs only over the verified + set, so a triage decision is what "survived verification" looks like on disk. Putting refuted + false positives in front of a developer with a band-aid attached is the fastest way to teach a + team to ignore the bot. + - **A diff with nothing scannable produced an *empty* comment**, so `--comment-out` wrote no file + and the action's step summary fell through to "no findings at or above the threshold" — a clean + bill of health for a diff that was never analysed. This was reachable by default: the shipped + workflow triggers on any `**/*.py` change while scanning one fixture directory. Now a comment + that says *nothing was reviewed*, and says it is not a clean bill of health. Still posts nothing + to the PR, because there is nothing to report. + - **Truncating an oversized comment deleted exactly the disclosures.** GitHub caps a body at + 65,536 characters; cutting from the end removed the "N files were not scanned" and "N sit + outside the scanned directory" lines — the sentences that stop a partial review reading as a + complete one. The finding list is cut instead and the disclosures always survive. + - **A crash exited 1, which the action reads as "findings reported".** So a review that never + completed looked like a completed one, and the workflow went on to publish a comment file that + did not exist. Unexpected failures now exit 2, and the summary distinguishes a crash from a + clean review by consulting the step outcome. + - **Three more ways a meaningless blast-radius number read as safe**, each carried through from + G2 rather than flattened into a rate: a simulation that could not confirm the edge was + *enforcing* the policy (G2's own first live defect — an unenforced policy blocked 0 of 200 and + looked harmless); one that evaluated *zero* requests because they all failed in transit; and one + replayed against XC access logs, which carry **no request bodies**, so a `body_matcher` policy + matches nothing and scores a perfect 0% that `simulate` itself had declared unjudgeable. + - Plus: git **quotes** non-ASCII paths (`"caf\303\251.py"`), so its suffix read as `.py"`, no + extension matched, and the file was neither scanned nor counted as outside — it vanished, and + the PR was told it was clean (`-z` output is unquoted); a caller passing the natural + `base: origin/main` got `origin/origin/main` and no merge base; every action input reached bash + through a `${{ }}` text substitution rather than the environment; `--min-severity` was + unvalidated and fell through to `high` while the comment stated the value the caller asked for; + the header's "scanned N of M" used the post-filter count as its denominator, so a fourteen-file + diff read as "1 of 1"; and the "not scanned" line named only the caps when the count also + includes vendored directories and unsupported file types. + - **The fixture lives in `bench/fixtures/ci/`, not in the Nimbus app, and that is not tidiness.** + `bench` scans `bench/fixtures/nimbus-vuln-lab/app/src/app/api` against `answer_key.yaml`, and a + new vulnerable route there produces findings listed in neither `expected` nor `bonus` — which + `bench.py` scores as **noise**. Committing the fixture inside the scan target would have silently + degraded the precision column of G4's committed scorecard and `BASELINE.md`: a benchmark + regression caused by a test fixture. Pinned by a test, and the reason is in the fixture's README + so the next person does not helpfully move it back. - Surfaces: `.github/actions/vpcopilot-scan/`, `docs/CI.md` (every file in `docs/` is uppercase). diff --git a/bench/fixtures/ci/README.md b/bench/fixtures/ci/README.md new file mode 100644 index 0000000..3322252 --- /dev/null +++ b/bench/fixtures/ci/README.md @@ -0,0 +1,27 @@ +# K2 CI-review fixture + +`refund-route.js` is a deliberately vulnerable Next.js route — a negative-amount hole, a SQL +injection, and a missing ownership check — used to exercise the pull-request review end to end +against a real model. + +**It lives here, outside `nimbus-vuln-lab/`, on purpose.** The benchmark scans +`bench/fixtures/nimbus-vuln-lab/app/src/app/api` against `bench/answer_key.yaml`, and a new +vulnerable file inside that tree would produce findings listed in neither `expected` nor `bonus` — +which `bench.py` counts as **noise**. That would silently degrade the precision column of G4's +committed scorecard and `BASELINE.md`: a benchmark regression caused by a test fixture. + +To reproduce the K2 live measurement, copy it into a branch and review the diff: + +```sh +git checkout -b k2-demo +mkdir -p bench/fixtures/nimbus-vuln-lab/app/src/app/api/refund +cp bench/fixtures/ci/refund-route.js \ + bench/fixtures/nimbus-vuln-lab/app/src/app/api/refund/route.js +git add -A && git commit -m "introduce a vulnerable refund route" + +vpcopilot ci-review --repo bench/fixtures/nimbus-vuln-lab/app/src/app/api --base main +``` + +Measured 2026-07-29: 77 s wall clock, three findings (critical negative-amount, critical SQLi, high +missing-authorization), four generated policies. Delete the branch afterwards so the answer key stays +the ground truth it claims to be. diff --git a/bench/fixtures/ci/refund-route.js b/bench/fixtures/ci/refund-route.js new file mode 100644 index 0000000..a2da066 --- /dev/null +++ b/bench/fixtures/ci/refund-route.js @@ -0,0 +1,15 @@ +import { NextResponse } from "next/server"; +import { db } from "@/lib/db"; + +// K2 fixture: a deliberately vulnerable endpoint added in a pull request, so the CI review has a +// known flaw to find in a diff. Mirrors the /api/pay negative-amount hole. +export async function POST(req) { + const { accountId, amount, reason } = await req.json(); + + // No lower bound on `amount`: a negative refund credits the attacker's account. + const rows = await db.query( + `UPDATE accounts SET balance = balance + ${amount} WHERE id = '${accountId}' RETURNING *` + ); + + return NextResponse.json({ ok: true, account: rows[0], reason }); +} diff --git a/docs/CI.md b/docs/CI.md new file mode 100644 index 0000000..32902e0 --- /dev/null +++ b/docs/CI.md @@ -0,0 +1,169 @@ +# CI — scan a pull request's diff and comment the band-aid (K2) + +The shift left: a developer sees the F5 Distributed Cloud virtual patch that would hold their new +hole closed **in the review where they introduced it**, before it is anyone's incident. + +``` +.github/actions/vpcopilot-scan/action.yml the action +.github/workflows/pr-review.yml an example workflow that uses it +vpcopilot ci-review the CLI the action shells out to +``` + +--- + +## 1. What it does + +On a pull request it takes the diff **against the merge base**, scans only the changed code files, +and leaves one comment carrying, per finding above a severity threshold: + +- the finding, its severity, its file and line; +- the F5 XC control triage routed it to — or `no_bandaid` with the residual risk, when a load + balancer cannot see the problem at all; +- the generated policy name; +- the blast radius, **if and only if** a real measurement exists (see §4). + +A pull request with no findings above the threshold gets **no comment at all**. A bot that says "all +clear" on every PR trains people to stop reading it. + +Re-running updates the same comment rather than adding another — it is anchored on a hidden marker, +so a branch pushed ten times has one comment that keeps up instead of ten that argue. + +## 2. Use it + +Already wired for this repo in `.github/workflows/pr-review.yml`. For your own source tree, change +one line: + +```yaml + - uses: actions/checkout@v4 + with: + fetch-depth: 0 # git merge-base needs history on both sides + - uses: ./.github/actions/vpcopilot-scan + with: + repo-path: src/api # <- your source directory + min-severity: high + anthropic-api-key: ${{ secrets.ANTHROPIC_API_KEY }} +``` + +| input | default | what it does | +|---|---|---| +| `repo-path` | `.` | directory within the checkout to scan | +| `base` | the PR's base branch | branch to diff against, via the **merge base** | +| `min-severity` | `high` | report at or above this: `critical`/`high`/`medium`/`low` | +| `min-confidence` | `0.5` | drop verified findings below this confidence | +| `max-files` | `40` | cap on changed files scanned — a PR diff is small and each file costs model calls, so this is far below a full scan's 200 | +| `comment` | `true` | post/update the comment | +| `fail-on-findings` | `false` | whether a finding turns the check red | +| `simulation-json` | — | a `simulation.json` from a real tenant run, to get blast-radius numbers (§4) | +| `anthropic-api-key` | *required* | model credential | +| `github-token` | `github.token` | needs `pull-requests: write` | + +Locally, the same thing without GitHub: + +```sh +vpcopilot ci-review --repo src/api --base origin/main # prints the comment +vpcopilot ci-review --repo src/api --base origin/main --post --pr-repo owner/name --pr 42 +``` + +Exit codes: `0` nothing reported · `1` at least one finding reported (information, not failure — +`fail-on-findings` decides whether the check goes red) · `2` a real error. + +## 3. It never writes to XC, structurally + +The acceptance criterion is *never writes to XC from CI*, and it is enforced by construction rather +than by care: `ci.py` imports no tenant client, no apply path, no refiner and no simulate, so **there +is no code path from CI to a load balancer**. A test reads the module's own source and fails if any +of them appears — the only version of this guarantee that survives someone adding a convenient import +later. The action declares no XC inputs, so there is nothing to pass one through. + +An XC credential sitting in a CI environment is a credential every workflow on the repo can reach. +If `ci-review` finds one in its environment it says so and carries on, because it has no use for it. + +The band-aid in the comment is a **proposal**. Nothing is created, attached, or enabled. + +## 4. Blast radius: the acceptance criterion contradicted itself + +K2's acceptance asked for "one comment carrying the policy **and the simulation result**" while also +requiring "never writes to XC from CI". Those cannot both hold. G2's blast-radius measurement works by +creating a throwaway policy object, **attaching it to a load balancer**, replaying recorded traffic +through it, and deleting it — three writes to a live tenant. There is no offline evaluator to fall +back on: G1 was deferred, deliberately, in 2026-07. + +So a would-block count is **not computed in CI**, and the comment says exactly that: + +> **Blast radius: not measured.** A would-block count requires attaching this policy to a load +> balancer and replaying recorded traffic through it — three writes to the tenant, which this job +> deliberately cannot make. Run `vpcopilot simulate` against a spare LB before promoting it. + +Neither silence nor `0%` would do: both read as *measured and safe*. + +To get real numbers into the comment, measure them where measuring is legitimate — against a spare +LB — and hand the artifact to the action: + +```sh +vpcopilot simulate --logs traffic.jsonl --lb vpcopilot-lab --out out # on a workstation, once +``` + +```yaml + with: + simulation-json: evidence/simulation.json +``` + +Each policy then reports `would block N of M recorded requests (R%)` and whether it tripped the +threshold. A number carried forward from an earlier replay is labelled as such (`carried_from`), so a +stale measurement is never presented as this policy's fresh result. + +## 5. Fork pull requests get no review + +The workflow uses `pull_request`, not `pull_request_target`. That is the safe choice: it runs against +the merge commit with a read-only token and cannot be induced by a fork to leak secrets. The +consequence is that a fork PR has no model credential and therefore no review, and the job skips +rather than failing a check nobody can fix from a fork. + +`pull_request_target` would fix that by running trusted workflow code with secrets against untrusted +head code — which is the standard way repositories leak their secrets. Not worth a review comment. + +## 6. What it does not cover, and says so + +Every boundary is disclosed in the comment rather than left for the reader to assume: + +- **Changed files outside the scanned directory** — counted and named as not looked at. This one is + load-bearing: `git diff` answers relative to the repository root while the scanner matches relative + to `repo-path`, and getting that wrong produces a *clean-looking review that scanned nothing*. +- **Files the collector declined** — over `--max-bytes`, beyond `--max-files`, in a vendored + directory, or an unsupported file type — reported as not covered, including on an otherwise-clean + comment, because an all-clear that hides an unscanned remainder is the one output this feature must + never produce. Truncation of an oversized comment cuts the *finding list*, never these lines. +- **The whole diff size** — the header's denominator is every changed file, not just the ones the + scanner supports, so "scanned 1 of 1" cannot stand in for a fourteen-file change. +- **Findings below the threshold** — counted when nothing is reported, so "we did not report this" + never reads as "there was nothing". +- **A diff with nothing scannable in it** — gets a comment saying *nothing was reviewed*, and saying + that it is not a clean bill of health. Nothing is posted to the PR (there is nothing to report), but + the step summary a human reads must distinguish "we looked and it is fine" from "we did not look". +- **A review that crashed** — the step summary says so explicitly. `if: always()` means it runs after + a failure too, and it used to write "no findings at or above the threshold" for a diff that was + never analysed. +- **Deleted files** — excluded; there is nothing left to scan. +- **The cure** — `ci-review` does not draft code fixes. The developer is editing the file by hand + right now; the band-aid and the finding are what CI can add. + +**Only findings that survived verification are reported.** `findings.json` is the *discover* contract — +every candidate, including the ones `verify` refuted as false positives — so the comment is built from +the findings that have a triage decision. Putting refuted false positives in front of a developer with +a band-aid attached is the fastest way to teach a team to ignore the bot. + +**A blast-radius number is only shown when it means something.** Beyond the not-measured case above, +two states from G2 are carried through rather than flattened into a rate: a simulation that could not +confirm the edge was *enforcing* the policy before counting (G2's first live defect — an unenforced +policy blocked 0 of 200 and looked harmless), and one that evaluated *zero* requests because they all +failed in transit. Both say so. Neither is a low block rate. + +## 7. Cost and time + +Measured on this repo's Nimbus fixture, one changed file introducing three real flaws (a negative +amount, a SQL injection, and a missing ownership check): **77 seconds wall clock**, 69 s of it +pipeline, against the acceptance budget of three minutes. It produced four policies across +`service_policy`, `waf`, `malicious_user` and `rate_limit`. + +Cost scales with **changed** files, not repository size, which is the point of diffing. `max-files` +is the ceiling; raise it deliberately. diff --git a/docs/USAGE.md b/docs/USAGE.md index 2a76d68..65d7b29 100644 --- a/docs/USAGE.md +++ b/docs/USAGE.md @@ -505,6 +505,25 @@ lifetime and writes frames to a private handle, so a stray `print` anywhere bene defaults `log=print`, and `rprint` is used throughout the CLI — lands on stderr, which the MCP spec reserves for logging, instead of corrupting the message stream. +## 9. Pull-request review in CI (K2) + +Scan the diff on a pull request and comment the proposed band-aid on it, so a developer sees the +virtual patch in the review where they introduced the hole. + +```sh +vpcopilot ci-review --repo src/api --base origin/main # prints the comment +vpcopilot ci-review --repo src/api --base origin/main --post --pr-repo owner/name --pr 42 +``` + +Ships as a composite action (`.github/actions/vpcopilot-scan/`) with an example workflow. Scans only +what the branch changed, against the merge base; posts nothing when there is nothing above the +threshold; and **never touches an XC tenant** — `ci.py` imports no tenant client at all, so a CI job +needs a GitHub token and nothing else. The blast-radius number cannot be produced in CI (measuring it +means attaching a policy to a load balancer), so the comment reports it only from a real tenant run's +`simulation.json` and otherwise says plainly that no measurement was made. + +Full reference: **[CI.md](CI.md)**. + **Run settings** — the collapsible bar shown on the action steps (**Mitigate / Cure / Retire**): LB · validate URL · PR repo · base · path prefix, plus **dry-run** (on by default), **refine** + attempts, **keep live**, and **allow protected LB**. Its summary line spells out the mode you're diff --git a/src/vpcopilot/ci.py b/src/vpcopilot/ci.py new file mode 100644 index 0000000..45f59f3 --- /dev/null +++ b/src/vpcopilot/ci.py @@ -0,0 +1,423 @@ +"""K2 — scan a pull request's diff and comment the proposed band-aid on it. + +The point is the shift left: a developer sees the F5 XC virtual patch that would hold their new hole +closed, in the review where they introduced it, before it is anyone's incident. + +**This module never touches the tenant, and that is structural rather than promised.** It imports no +`xc`, no `apply`, no `refiner` and no `simulate`, so there is no code path from CI to a load +balancer — pinned by a test that reads this module's own source. CI gets a `GITHUB_TOKEN` to comment +and nothing else; an XC credential in a CI environment is a credential that can be exfiltrated by +any workflow, and the band-aid is a proposal at this stage, not a deployment. + +**The acceptance criterion contradicted itself, and this is the honest resolution.** It asked for +"one comment carrying the policy **and the simulation result**" while also requiring "never writes to +XC from CI" — but G2's blast-radius measurement works by creating a throwaway policy, ATTACHING it to +a load balancer, replaying traffic through it and deleting it. That is three XC writes. A would-block +count cannot be produced in CI at all. So the comment reports the blast radius **only when a +`simulation.json` from a real tenant run is available** (committed, or restored as a CI artifact), and +otherwise says in as many words that no measurement was made and why. Reporting a number nobody +measured would be worse than reporting none; rendering its absence as if the policy were known-safe +would be worse still. +""" +from __future__ import annotations + +import json +import os +import subprocess +from collections.abc import Callable +from pathlib import Path + +from .repo_scan import CODE_EXT + +# The anchor that makes a re-run update its comment instead of adding another. A pushed branch gets +# many runs, and a bot that appends every time is a bot people mute. +MARKER = "" +SEVERITY_RANK = {"critical": 0, "high": 1, "medium": 2, "low": 3} + + +def rebase_onto(changed: set[str], scan_root: str, git_root: str) -> tuple[set[str], int]: + """Translate git-root-relative diff paths into paths relative to the directory being scanned. + + This is the difference between a working review and a silent all-clear. `git diff --name-only` + answers relative to the repository root, `collect_files` matches relative to the directory it is + given, and this project's own app fixture lives eight levels down + (`bench/fixtures/nimbus-vuln-lab/app/src/app/api`). Compared directly, the two never match: zero + files are scanned, no findings are produced, and the pull request is told it is clean. Any + reviewer of this feature would have to notice the absence of an error to catch it. + + Returns the rebased set plus a count of changed files that fall OUTSIDE the scanned directory, so + the comment can say that part of the diff was not looked at rather than implying it was. + """ + root, scan = Path(git_root).resolve(), Path(scan_root).resolve() + inside: set[str] = set() + outside = 0 + for rel in changed: + p = (root / rel).resolve() + try: + inside.add(str(p.relative_to(scan))) + except ValueError: + outside += 1 + return inside, outside + + +def changed_files(repo: str = ".", base: str = "origin/main", head: str = "HEAD", + *, log: Callable = print) -> tuple[set[str], str, int]: + """The changed scannable paths, the merge base compared against, and the RAW diff size. + + `git diff --name-only ...` — three dots, so the comparison is against the MERGE BASE + rather than the tip of `base`. With two dots, every commit landing on main between the branch + point and now reads as a change in this PR, so a busy main makes every PR look like it touched + half the tree and the scan stops being about the diff at all. + + Deleted files are excluded (`--diff-filter=d`): there is nothing left to scan, and passing a + path that no longer exists to the collector would report it as skipped rather than as gone. + + **`-z`, because git quotes paths by default.** With plain `--name-only`, a non-ASCII filename + comes back as `"caf\\303\\251.py"` — C-style octal escapes, surrounded by literal quotes. Its + suffix then reads as `.py"`, which is not a known code extension, so the file was dropped from + the scan set *and* not counted as outside it: it vanished, and the pull request was told it was + clean. NUL-separated output is not quoted at all, which also settles spaces and newlines. + """ + def git(*args) -> str: + return subprocess.check_output(["git", "-C", repo, *args], text=True) + + try: + merge_base = git("merge-base", base, head).strip() + except subprocess.CalledProcessError as e: + raise CIError(f"cannot find a merge base between {base!r} and {head!r}: {e}. In CI, fetch " + "enough history — actions/checkout defaults to a single commit " + "(fetch-depth: 0).") from e + try: + out = git("diff", "--name-only", "-z", "--diff-filter=d", f"{merge_base}..{head}") + except subprocess.CalledProcessError as e: + raise CIError(f"git diff failed: {e}") from e + names = {n for n in out.split("\0") if n.strip()} + code = {n for n in names if Path(n).suffix in CODE_EXT} + log(f"diff {base}...{head} (merge-base {merge_base[:9]}): {len(names)} file(s) changed, " + f"{len(code)} scannable") + # The RAW total travels with the filtered set. Without it the comment's "scanned N of M" used the + # post-filter count as its denominator, so a diff of eleven files with one scannable read as + # "1 of 1" — a complete-looking review of a diff it mostly did not support. + return code, merge_base, len(names) + + +class CIError(RuntimeError): + """A precondition CI could not establish. Raised rather than degraded, because a review that + silently scanned nothing is indistinguishable from a review that found nothing.""" + + +def _blast_radius(out_dir: str) -> tuple[dict[str, dict], dict]: + """Per-policy would-block numbers from a PREVIOUS tenant run, plus the run-level metadata that + says whether those numbers can be believed. + + Never measured here — see the module docstring. Keyed by policy name. + + The metadata is returned rather than dropped because of one case in particular: + `bodies_available` is false when the traffic sample came from XC access logs, which capture no + request body. A `body_matcher` service policy then matches nothing during replay and scores a + perfectly clean **0%** — a rate `simulate` itself declared unjudgeable, rendered by us as + "under the threshold". Keeping only the inner dicts threw away the one field that says so.""" + p = Path(out_dir) / "simulation.json" + if not p.is_file(): + return {}, {} + try: + doc = json.loads(p.read_text()) + except (OSError, json.JSONDecodeError): + return {}, {} + per_policy = {pol["policy_name"]: pol + for pol in doc.get("policies") or [] if pol.get("policy_name")} + meta = {"bodies_available": doc.get("bodies_available", True), + "caveats": doc.get("caveats") or [], "records": doc.get("records"), + "source": doc.get("source", ""), "ts": doc.get("ts", "")} + return per_policy, meta + + +def render_comment(findings: list[dict], triage: dict, policies: list[dict], *, + min_severity: str, changed: int, sim: dict | None = None, + scanned: int = 0, base: str = "", truncated: int = 0, + outside: int = 0, diff_total: int | None = None, + sim_meta: dict | None = None) -> str: + """The comment body. Deterministic — code, not a model. + + An agent drafting this would be an agent free to round a would-block rate or soften a severity, + and these are the numbers a developer decides with.""" + sim = sim or {} + sim_meta = sim_meta or {} + floor = SEVERITY_RANK.get(min_severity, 1) + shown = [f for f in findings if SEVERITY_RANK.get(f.get("severity", "low"), 3) <= floor] + shown.sort(key=lambda f: SEVERITY_RANK.get(f.get("severity", "low"), 3)) + pol_by_finding: dict[str, list[dict]] = {} + for p in policies: + pol_by_finding.setdefault(p.get("finding_id", ""), []).append(p) + + # The denominator is the RAW diff size when we know it, not the post-filter count: "1 of 1" for a + # diff of eleven files reads as a complete review of the whole change. + scope = (f"Scanned **{scanned}** of {changed} scannable file(s)" + + (f" ({diff_total} changed in total)" if diff_total and diff_total != changed else "") + if changed else f"Scanned **{scanned}** changed file(s)") + L = [MARKER, "## 🩹 Virtual Patch Copilot", + "", scope + (f" in this diff against `{base[:9]}`" if base else " in this diff") + ".", ""] + if not shown: + held = len(findings) - len(shown) + L += [f"**No findings at or above `{min_severity}`.**"] + if held: + L += ["", f"_{held} lower-severity finding(s) were found and not reported here — " + f"raise the threshold below `{min_severity}` to see them._"] + # An all-clear that hides an unscanned remainder is the one output this feature must never + # produce: it is the difference between "we looked and it is fine" and "we did not look". + if truncated: + L += ["", f"_{truncated} changed file(s) were **not scanned** — over `--max-bytes`, beyond " + "`--max-files`, in a vendored directory, or an unsupported file type — so " + "this is not a clean bill of health for them._"] + if outside: + L += ["", f"_{outside} changed file(s) sit outside the scanned directory and were not " + "looked at._"] + return "\n".join(L) + + L += [f"**{len(shown)} finding(s) at or above `{min_severity}`.** Each row is the band-aid that " + "would hold the hole closed at the edge while the code fix ships — a proposal, not a " + "deployment: this job has no tenant credentials and changes nothing.", ""] + + for f in shown: + fid = f.get("id", "") + sev = f.get("severity", "?") + loc = f.get("file") or f.get("source") or "" + line = f.get("line") + where = f"`{loc}`" + (f":{line}" if line else "") + L += [f"### `{sev}` — {f.get('title', fid)}", "", f"{where} · `{fid}`", ""] + if f.get("description"): + L += [str(f["description"]).strip(), ""] + d = triage.get(fid) or {} + bandaids = d.get("bandaids") or [] + if d.get("no_bandaid"): + L += ["**No band-aid.** " + (d.get("residual_risk") or + "Nothing observable in a request to block — this one needs the code fix."), ""] + elif bandaids: + names = ", ".join(f"`{b['control']}`" + ("" if b.get("recommended") else " (alt)") + for b in bandaids) + L += [f"**Proposed control(s):** {names}", ""] + for pol in pol_by_finding.get(fid, []): + name = pol.get("policy_name", "") + L += [f"**Generated policy:** `{pol.get('control')}` / `{name}`"] + s = sim.get(name) + if s is None: + # Not "0%" and not silence. Both would read as "measured and safe". + L += ["", "> **Blast radius: not measured.** A would-block count requires attaching " + "this policy to a load balancer and replaying recorded traffic through it " + "— three writes to the tenant, which this job deliberately cannot make. " + "Run `vpcopilot simulate` against a spare LB before promoting it."] + elif not s.get("enforcement_confirmed", True): + # G2's FIRST live defect: attaching a policy and replaying immediately counted an + # UNENFORCED policy as harmless — 0 of 200 blocked, including the exploit. A rate + # measured before the edge was confirmed enforcing measures nothing, so it must not + # be shown as a rate at all. + L += ["", "> **Blast radius: unconfirmed.** The simulation could not confirm the " + "edge was enforcing this policy before counting, so its numbers measure " + "an unenforced policy and not this one. Re-run `vpcopilot simulate`."] + elif not s.get("evaluated"): + # G2 treats zero evaluated as NO EVIDENCE, explicitly not as safe — every replayed + # request failed in transit, so there is no rate to report. Rendering + # "would block 0 of 0 (0.0%) — under the threshold" would turn a measurement that + # failed into the most reassuring line in the comment. + L += ["", "> **Blast radius: no evidence.** A simulation ran but evaluated **zero** " + "requests" + + (f" — {s['reason']}" if s.get("reason") else "") + + ". That is not a low block rate; nothing was measured."] + else: + rate = s.get("block_rate") + verdict = ("⚠️ **over the blast-radius threshold**" if s.get("blocked_promotion") + else "under the threshold") + L += ["", f"> **Blast radius** (from a previous tenant run): would block " + f"**{s.get('would_block')}** of {s.get('evaluated')} recorded requests" + + (f" (**{rate:.1%}**)" if isinstance(rate, (int, float)) else "") + + f" — {verdict}."] + if not sim_meta.get("bodies_available", True): + # G2 sets this false for XC access-log samples, which carry no request body. A + # `body_matcher` policy matches nothing against them and scores a clean 0% — + # a rate the simulation itself declared unjudgeable. + L += ["> _The replayed sample carried **no request bodies** (XC access logs), so " + "a policy that matches on the body could not be judged from it. Treat a " + "low rate here as unmeasured, not safe._"] + if s.get("carried_from"): + L += [f"> _Carried forward from an earlier replay ({s['carried_from']}); this " + "number was not measured by the most recent simulation._"] + L += [""] + L += ["---", ""] + + # The disclosures and the footer are assembled SEPARATELY from the finding list, because + # truncation drops the tail — and the tail is exactly where "we did not look at these files" + # lives. Cutting the finding list while keeping these is the whole point. + tail: list[str] = [] + if truncated: + tail += [f"_{truncated} changed file(s) were not scanned — over `--max-bytes`, beyond " + "`--max-files`, in a vendored directory, or an unsupported file type. They are " + "not covered by this review._", ""] + if outside: + tail += [f"_{outside} changed file(s) sit outside the scanned directory and were not looked " + "at._", ""] + tail += ["Band-aid buys time; the cure is the code fix. " + "`vpcopilot scan` locally for the full run."] + return _fit("\n".join(L), "\n".join(tail), len(shown)) + + +# GitHub rejects an issue-comment body over 65536 characters. A PR touching forty files could reach +# it, and the failure mode is the whole comment being lost to an API error — the review silently not +# happening. Truncated with the count of what was dropped, so the comment is never a partial list +# passing itself off as the whole one. +MAX_BODY = 65_536 + + +def _fit(body: str, tail: str, n_findings: int) -> str: + """Keep the body under GitHub's cap while PRESERVING the disclosures. + + The naive version truncated the whole comment from the end — which is where the "N changed files + were not scanned" and "N sit outside the scanned directory" lines are, so exactly the sentences + that stop a partial review reading as a complete one were the first thing dropped. The finding + list is cut instead, and the disclosures plus a count of what is hidden are always kept.""" + full = body + "\n" + tail + if len(full) <= MAX_BODY: + return full + notice_room = 400 + len(tail) + head = body[:MAX_BODY - notice_room] + # cut at a finding boundary so the last entry is not left half-rendered + if "\n---\n" in head: + head = head[:head.rfind("\n---\n") + 1] + hidden = n_findings - head.count("\n### ") + return (head + + f"\n---\n\n> **This comment was truncated.** {hidden} of {n_findings} finding(s) are " + "not shown — GitHub caps a comment at 65,536 characters. Run `vpcopilot ci-review` " + "locally, or raise `min-severity`, to see the rest.\n\n" + + tail) + + +def upsert_comment(repo_slug: str, pr_number: int, body: str, *, token: str | None = None, + dry_run: bool = False, log: Callable = print) -> dict: + """Post the comment, or update the one this tool left last time. + + Keyed on `MARKER`, so a branch pushed ten times has one comment that keeps up rather than ten + that argue with each other.""" + from .pr import _resolve_token + if dry_run: + log(f"[dry-run] would upsert a {len(body)} char comment on {repo_slug}#{pr_number}") + return {"posted": False, "dry_run": True, "chars": len(body)} + try: + from github import Github + except ImportError as e: + raise CIError("PyGithub is not installed — `pip install -e '.[deploy]'`") from e + gh = Github(_resolve_token(token)) + pr = gh.get_repo(repo_slug).get_pull(int(pr_number)) + for c in pr.get_issue_comments(): + # STARTSWITH, not `in`. Our comments always begin with the marker; a human quoting the bot in + # a reply produces `> `, which `in` would match — and we would + # then overwrite that person's comment with our review. + if (c.body or "").startswith(MARKER): + c.edit(body) + log(f"updated comment {c.id} on {repo_slug}#{pr_number}") + return {"posted": True, "updated": True, "comment_id": c.id} + c = pr.create_issue_comment(body) + log(f"created comment {c.id} on {repo_slug}#{pr_number}") + return {"posted": True, "updated": False, "comment_id": c.id} + + +def review(repo: str = ".", *, base: str = "origin/main", head: str = "HEAD", + out_dir: str = "out-ci", min_severity: str = "high", min_confidence: float = 0.5, + max_files: int = 40, max_bytes: int = 60_000, config_path: str | None = None, + repo_slug: str | None = None, pr_number: int | None = None, + post: bool = False, dry_run: bool = False, token: str | None = None, + log: Callable = print) -> dict: + """Scan the diff, build the comment, and post it when asked to. + + `draft_code_fixes` is off: a PR review wants the band-aid and the finding, and drafting the cure + is the expensive half of a scan for something the developer is already editing by hand. + """ + if os.environ.get("XC_API_TOKEN") or os.environ.get("XC_API_URL"): + # Not a machine veto — a refusal to be handed something it should never have. CI needs a + # GitHub token and nothing else; an XC credential present in a CI environment is one any + # workflow on the repo can reach, and this path has no use for it. + log(" ⚠ XC credentials are present in this environment — this review never uses them and " + "no CI job should carry them") + + changed, merge_base, diff_total = changed_files(repo, base, head, log=log) + try: + git_root = subprocess.check_output(["git", "-C", repo, "rev-parse", "--show-toplevel"], + text=True).strip() + except (subprocess.CalledProcessError, OSError) as e: + raise CIError(f"{repo!r} is not inside a git working tree: {e}") from e + changed, outside = rebase_onto(changed, repo, git_root) + if outside: + log(f" {outside} changed file(s) are outside {repo!r} and were NOT scanned") + if not changed: + why = ("no scannable files changed" if not outside else + f"every changed file is outside {repo!r} — nothing in the scanned directory changed") + log(f"{why} — nothing to review") + # A comment body even though nothing will be POSTED. The early return used to hand back + # `comment: ""`, so `--comment-out` wrote nothing and the action's step summary fell through + # to "no findings at or above the threshold" — a clean bill of health for a diff that was + # never reviewed. Nothing is posted to the PR (there is nothing to report), but the surface a + # human actually reads has to say which of the two happened. + body = "\n".join([ + MARKER, "## 🩹 Virtual Patch Copilot", "", + f"**Nothing was reviewed.** {why}.", "", + f"Of {diff_total} file(s) changed in this diff, {len(changed)} were scannable" + + (f" and {outside} sit outside `{repo}`" if outside else "") + ".", "", + "_This is **not** a clean bill of health — no code was analysed._"]) + return {"changed": 0, "scanned": 0, "findings": [], "candidates": 0, "refuted": 0, + "reported": 0, "comment": body, "posted": False, "outside": outside, + "diff_total": diff_total, "reason": why} + + from .pipeline import run_pipeline + summary = run_pipeline(repo, out_dir=out_dir, config_path=config_path, + min_confidence=min_confidence, max_files=max_files, max_bytes=max_bytes, + draft_code_fixes=False, only_files=changed, log=log) + + def rj(name, default): + p = Path(out_dir) / name + try: + return json.loads(p.read_text()) if p.is_file() else default + except (OSError, json.JSONDecodeError): + return default + + candidates = rj("findings.json", []) + triage = {d.get("finding_id"): d for d in rj("triage.json", [])} + policies = rj("policies.json", []) + metrics = rj("metrics.json", {}) + scanned = (metrics.get("discovery") or {}).get("files", 0) + truncated = max(0, len(changed) - scanned) + + # `findings.json` is the DISCOVER contract — every candidate, including the ones `verify` + # refuted as false positives. Triage runs only over the verified set, so a triage decision is + # what "survived verification" looks like on disk. Reporting straight from findings.json put + # refuted false positives in front of a developer with a proposed band-aid attached, which is the + # fastest way to teach a team to ignore this bot. + findings = [f for f in candidates if f.get("id") in triage] + refuted = len(candidates) - len(findings) + if refuted: + log(f" {refuted} candidate(s) were refuted by verify and are not reported") + + floor = SEVERITY_RANK.get(min_severity, 1) + reported = [f for f in findings + if SEVERITY_RANK.get(f.get("severity", "low"), 3) <= floor] + + sim, sim_meta = _blast_radius(out_dir) + body = render_comment(findings, triage, policies, min_severity=min_severity, + changed=len(changed), sim=sim, scanned=scanned, + base=merge_base, truncated=truncated, outside=outside, + diff_total=diff_total, sim_meta=sim_meta) + res = {"changed": len(changed), "scanned": scanned, "findings": findings, + "candidates": len(candidates), "refuted": refuted, "diff_total": diff_total, + "reported": len(reported), "comment": body, "posted": False, + "outside": outside, "summary": summary, "out": out_dir} + + if not reported: + # "A PR with no new findings posts nothing." Silence is the correct output: a bot that + # comments "all clear" on every PR trains people to stop reading it. + log(f"no findings at or above {min_severity} — posting nothing") + res["reason"] = f"no findings at or above {min_severity}" + return res + + if post: + if not (repo_slug and pr_number): + raise CIError("posting needs both --repo (owner/name) and --pr (number)") + res.update(upsert_comment(repo_slug, int(pr_number), body, token=token, + dry_run=dry_run, log=log)) + return res diff --git a/src/vpcopilot/cli.py b/src/vpcopilot/cli.py index 1192723..2d19017 100644 --- a/src/vpcopilot/cli.py +++ b/src/vpcopilot/cli.py @@ -849,6 +849,78 @@ def console(host: str = typer.Option("127.0.0.1", help="bind host"), uvicorn.run("vpcopilot.console.app:app", host=host, port=port, log_level="warning") +@app.command(name="ci-review") +def ci_review( + repo: str = typer.Option(".", help="repository working tree to scan"), + base: str = typer.Option("origin/main", "--base", help="branch the PR targets; compared against the MERGE BASE"), + head: str = typer.Option("HEAD", "--head", help="branch/commit under review"), + out: str = typer.Option("out-ci", help="run directory for this review's artifacts"), + min_severity: str = typer.Option("high", "--min-severity", help="report findings at or above this (critical|high|medium|low)"), + min_confidence: float = typer.Option(0.5, "--min-confidence", help="drop verified findings below this confidence"), + max_files: int = typer.Option(40, "--max-files", help="cap on changed files scanned — a PR diff is small, and CI has a budget"), + max_bytes: int = typer.Option(60_000, "--max-bytes", help="cap on bytes read per file"), + pr_repo: str = typer.Option(None, "--pr-repo", help="GitHub slug owner/name, to post the comment"), + pr: int = typer.Option(None, "--pr", help="pull request number to comment on"), + post: bool = typer.Option(False, "--post", help="actually post/update the PR comment (needs --pr-repo and --pr)"), + dry_run: bool = typer.Option(False, "--dry-run", help="with --post, render and report without calling GitHub"), + comment_out: str = typer.Option(None, "--comment-out", help="also write the rendered comment to this path"), +): + """K2: scan a pull request's diff and comment the proposed band-aid on it. + + Scans only what the branch changed, against the merge base. Never touches an F5 XC tenant: this + path imports no tenant client at all, so a CI job needs a GitHub token and nothing else. A + would-block count cannot be produced here — measuring blast radius means attaching a policy to a + load balancer — so the comment reports it only from a previous tenant run's `simulation.json`, and + otherwise says plainly that no measurement was made. + + Prints the comment when `--post` is absent, so it composes in a pipeline. Exits 1 when it reports + at least one finding, so a workflow can choose to fail the check.""" + from .ci import CIError, review + + load_dotenv() + if min_severity not in ("critical", "high", "medium", "low"): + # Unvalidated, an unknown value fell through `SEVERITY_RANK.get(..., 1)` to `high` — and the + # comment then stated a threshold the run had not actually applied. + rprint(f"[red]--min-severity must be critical, high, medium or low (got {min_severity!r})[/red]") + raise typer.Exit(code=2) + try: + res = review(repo, base=base, head=head, out_dir=out, min_severity=min_severity, + min_confidence=min_confidence, max_files=max_files, max_bytes=max_bytes, + config_path=_active_config_path(), repo_slug=pr_repo, pr_number=pr, + post=post, dry_run=dry_run, log=lambda m: rprint(f"[dim]{m}[/dim]")) + except CIError as e: + rprint(f"[red]{e}[/red]") + raise typer.Exit(code=2) from None + except Exception as e: # noqa: BLE001 + # EXIT 2, not 1. Exit 1 means "findings were reported" and the action reads it that way, so + # letting a crash exit 1 would make a review that never completed look like a completed one + # with findings — and the workflow would go on to publish a comment file that does not exist. + rprint(f"[red]ci-review failed: {type(e).__name__}: {e}[/red]") + raise typer.Exit(code=2) from None + # Written even when nothing is reported: the action's step summary reads this file, and its + # absence used to be rendered as "no findings at or above the threshold" — including for a diff + # that was never reviewed at all. + if comment_out and res["comment"]: + Path(comment_out).write_text(res["comment"]) + rprint(f"wrote [bold]{comment_out}[/bold]") + rprint(Panel.fit( + f"[bold]changed[/bold]: {res['changed']} [bold]scanned[/bold]: {res['scanned']} " + f"[bold]reported[/bold]: {res['reported']}" + + (f"\n[dim]{res['reason']}[/dim]" if res.get("reason") else "") + + (f"\n[dim]comment {'updated' if res.get('updated') else 'created'}[/dim]" + if res.get("posted") else ""), + title="ci-review")) + if not post and res["reported"]: + print(res["comment"]) + if res["reported"]: + raise typer.Exit(code=1) + + +def _active_config_path() -> str | None: + """The config a CI run should use: whatever VPCOPILOT_CONFIG names, or the default.""" + return os.environ.get("VPCOPILOT_CONFIG") or None + + @app.command() def mcp(write: bool = typer.Option(None, "--write/--no-write", help="expose the mutating tools (apply, pr, retire, reconcile, " diff --git a/src/vpcopilot/pipeline.py b/src/vpcopilot/pipeline.py index f529fb8..eeb4187 100644 --- a/src/vpcopilot/pipeline.py +++ b/src/vpcopilot/pipeline.py @@ -71,6 +71,7 @@ def run_pipeline( min_severity: str = "high", # H2: floor for reaching the resolve agent max_advisories: int = 25, # H2: cap on the agent stage (0 = no cap) include_dev: bool = False, # H2: dev/test-scoped dependencies too + only_files: set[str] | None = None, # K2: restrict the walk to a PR's changed files ) -> dict: # H1 — exactly one input. Deliberately a hard error rather than a silent no-op: today # `run_pipeline("/does/not/exist")` completes and writes a full set of empty artifacts, and @@ -125,9 +126,11 @@ def run_pipeline( discover_s = time.perf_counter() - t0 else: if repo_path: - files, skipped = collect_files(repo_path, max_bytes=max_bytes, max_files=max_files) + files, skipped = collect_files(repo_path, max_bytes=max_bytes, max_files=max_files, + only=only_files) log(f"scanning {len(files)} files (caps: --max-files {max_files}, --max-bytes {max_bytes}; " - f"{len(skipped)} skipped)") + f"{len(skipped)} skipped)" + + (f"; restricted to {len(only_files)} changed file(s)" if only_files is not None else "")) for reason in ("max-files-reached", "too-large"): n = sum(1 for _, r in skipped if r == reason) if n: diff --git a/src/vpcopilot/repo_scan.py b/src/vpcopilot/repo_scan.py index 3b3730a..4b3a4ff 100644 --- a/src/vpcopilot/repo_scan.py +++ b/src/vpcopilot/repo_scan.py @@ -16,7 +16,16 @@ } -def collect_files(root: str, max_bytes: int = 60_000, max_files: int = 200): +def collect_files(root: str, max_bytes: int = 60_000, max_files: int = 200, + only: set[str] | None = None): + """Every scannable file under `root`, subject to the caps. + + `only` (K2) restricts the walk to a set of repo-relative paths — the files a pull request + actually changed. It filters rather than replaces the walk, so every other rule still applies: + a changed file that is a vendored dependency, an unsupported extension or over `max_bytes` is + excluded and reported exactly as it would be in a full scan. `None` walks everything, so the + existing call path is unchanged. + """ root = Path(root) files: list[Path] = [] skipped: list[tuple[str, str]] = [] @@ -25,6 +34,8 @@ def collect_files(root: str, max_bytes: int = 60_000, max_files: int = 200): continue if any(part in SKIP_DIRS for part in p.relative_to(root).parts): continue + if only is not None and str(p.relative_to(root)) not in only: + continue if p.suffix not in CODE_EXT: continue if p.stat().st_size > max_bytes: diff --git a/tests/test_ci.py b/tests/test_ci.py new file mode 100644 index 0000000..89716a6 --- /dev/null +++ b/tests/test_ci.py @@ -0,0 +1,751 @@ +"""K2 — scan a PR's diff and comment the band-aid. Offline: a real local git repo for the diff, a +faked pipeline, and no GitHub call.""" +from __future__ import annotations + +import json +import subprocess + +import pytest + +from vpcopilot import ci + + +def _git(repo, *args): + return subprocess.check_output(["git", "-C", str(repo), *args], text=True).strip() + + +@pytest.fixture +def repo(tmp_path): + """A real two-branch repo, so the merge-base logic is exercised rather than mocked.""" + r = tmp_path / "repo" + r.mkdir() + _git(r.parent, "init", "-q", str(r)) if False else subprocess.run( + ["git", "init", "-q", str(r)], check=True) + _git(r, "config", "user.email", "t@example.com") + _git(r, "config", "user.name", "t") + (r / "app.py").write_text("def ok():\n return 1\n") + (r / "README.md").write_text("readme\n") + _git(r, "add", "-A") + _git(r, "commit", "-qm", "base") + _git(r, "branch", "-M", "main") + _git(r, "checkout", "-q", "-b", "feature") + (r / "pay.py").write_text("def pay(amount):\n return amount\n") + (r / "notes.txt").write_text("not code\n") + _git(r, "add", "-A") + _git(r, "commit", "-qm", "add pay") + return r + + +# ============================================================ the diff +def test_only_the_branchs_own_changed_code_files_are_scanned(repo): + changed, base, total = ci.changed_files(str(repo), base="main", head="HEAD", log=lambda m: None) + assert changed == {"pay.py"} # notes.txt is not a code extension + assert base and len(base) == 40 + + +def test_the_diff_is_against_the_merge_base_not_the_tip_of_base(repo): + """Three dots, not two. With two, every commit that landed on the base branch since the branch + point reads as a change in this PR — so a busy main makes every PR look like it touched half the + tree and the review stops being about the diff.""" + _git(repo, "checkout", "-q", "main") + (repo / "unrelated.py").write_text("# landed on main after the branch point\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "unrelated main work") + _git(repo, "checkout", "-q", "feature") + + changed, _, _total = ci.changed_files(str(repo), base="main", head="HEAD", log=lambda m: None) + assert changed == {"pay.py"}, "a commit on main leaked into the PR's diff" + + +def test_a_deleted_file_is_not_offered_for_scanning(repo): + (repo / "app.py").unlink() + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "delete app") + changed, _, _total = ci.changed_files(str(repo), base="main", head="HEAD", log=lambda m: None) + assert "app.py" not in changed and changed == {"pay.py"} + + +def test_a_shallow_checkout_declines_with_the_fix_named(tmp_path): + """actions/checkout fetches one commit by default, which leaves `git merge-base` nothing to find. + The failure has to name the remedy, because it is the first thing anyone hits in CI.""" + r = tmp_path / "shallow" + subprocess.run(["git", "init", "-q", str(r)], check=True) + with pytest.raises(ci.CIError, match="fetch-depth"): + ci.changed_files(str(r), base="origin/main", head="HEAD", log=lambda m: None) + + +# ============================================================ scoping the scan +def test_collect_files_restricted_to_a_set_still_applies_every_other_rule(tmp_path): + """`only` filters the walk rather than replacing it: a changed file that is too large, vendored or + an unsupported extension is excluded and reported exactly as in a full scan.""" + from vpcopilot.repo_scan import collect_files + (tmp_path / "a.py").write_text("x = 1\n") + (tmp_path / "big.py").write_text("y = 1\n" * 5000) + (tmp_path / "n.txt").write_text("not code\n") + (tmp_path / "node_modules").mkdir() + (tmp_path / "node_modules" / "v.py").write_text("vendored\n") + + files, skipped = collect_files(str(tmp_path), max_bytes=100, + only={"a.py", "big.py", "n.txt", "node_modules/v.py"}) + assert [f.name for f in files] == ["a.py"] + assert ("big.py", "too-large") in skipped # still reported, not silently dropped + + +def test_collect_files_without_only_is_unchanged(tmp_path): + """No regression: the existing call path walks everything.""" + from vpcopilot.repo_scan import collect_files + (tmp_path / "a.py").write_text("x = 1\n") + (tmp_path / "b.py").write_text("y = 1\n") + assert {f.name for f in collect_files(str(tmp_path))[0]} == {"a.py", "b.py"} + + +def test_the_pipeline_passes_only_files_through(monkeypatch, tmp_path): + from vpcopilot import repo_scan + seen = {} + orig = repo_scan.collect_files + + def spy(root, **kw): + seen.update(kw) + return orig(root, **kw) + monkeypatch.setattr("vpcopilot.pipeline.collect_files", spy) + (tmp_path / "a.py").write_text("x = 1\n") + from vpcopilot.harness import Harness + monkeypatch.setattr(Harness, "__init__", lambda self, cp=None: None) + monkeypatch.setattr(Harness, "warmup", lambda self: None) + from vpcopilot.agents import discover + monkeypatch.setattr(discover, "run", lambda *a, **k: (_ for _ in ()).throw(RuntimeError("stop"))) + from vpcopilot.pipeline import run_pipeline + try: + run_pipeline(str(tmp_path), out_dir=str(tmp_path / "o"), only_files={"a.py"}, + log=lambda m: None) + except Exception: # noqa: BLE001 — discovery is stubbed to stop early + pass + assert seen.get("only") == {"a.py"} + + +# ============================================================ the comment +FINDINGS = [ + {"id": "neg-pay-001", "title": "Negative amount accepted", "severity": "critical", + "file": "pay.py", "line": 2, "description": "No lower bound on amount."}, + {"id": "low-001", "title": "Verbose error", "severity": "low", "file": "pay.py"}, +] +TRIAGE = { + "neg-pay-001": {"finding_id": "neg-pay-001", "no_bandaid": False, + "bandaids": [{"control": "service_policy", "recommended": True}, + {"control": "api_schema", "recommended": False}]}, + "low-001": {"finding_id": "low-001", "no_bandaid": True, "residual_risk": "cosmetic"}, +} +POLICIES = [{"finding_id": "neg-pay-001", "control": "service_policy", + "policy_name": "deny-negative-pay-amount"}] + + +def test_the_comment_carries_the_finding_the_control_and_the_policy(): + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=3, scanned=1) + assert ci.MARKER in body + assert "neg-pay-001" in body and "pay.py" in body + assert "`service_policy`" in body and "`api_schema` (alt)" in body + assert "deny-negative-pay-amount" in body + assert "low-001" not in body # below the threshold + assert "1 lower-severity finding" not in body # only said when NOTHING is reported + + +def test_an_unmeasured_blast_radius_says_so_rather_than_implying_safety(): + """The acceptance asked for the simulation result AND for never writing to XC. Measuring blast + radius means attaching a policy to a load balancer — three tenant writes — so it cannot happen + here. Neither silence nor "0%" is acceptable: both read as measured-and-safe.""" + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, scanned=1) + assert "Blast radius: not measured" in body + assert "cannot make" in body or "deliberately cannot" in body + assert "vpcopilot simulate" in body # names how to get the number + + +def test_a_supplied_simulation_result_is_reported_with_its_verdict(): + sim = {"deny-negative-pay-amount": {"policy_name": "deny-negative-pay-amount", "evaluated": 200, + "would_block": 1, "block_rate": 0.005, + "blocked_promotion": False}} + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, scanned=1, + sim=sim) + assert "would block **1** of 200" in body and "0.5%" in body + assert "under the threshold" in body + assert "not measured" not in body + + +def test_an_overbroad_simulation_result_is_flagged(): + sim = {"deny-negative-pay-amount": {"policy_name": "deny-negative-pay-amount", "evaluated": 200, + "would_block": 154, "block_rate": 0.77, + "blocked_promotion": True}} + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, scanned=1, + sim=sim) + assert "over the blast-radius threshold" in body and "77.0%" in body + + +def test_a_carried_forward_number_is_labelled_as_stale(): + """The G2/K1 `carried_from` marker has to survive into the comment: a number measured by an + earlier replay must not be presented as this policy's fresh result.""" + sim = {"deny-negative-pay-amount": {"policy_name": "deny-negative-pay-amount", "evaluated": 200, + "would_block": 1, "block_rate": 0.005, + "blocked_promotion": False, + "carried_from": "2026-07-29T10:00:00Z"}} + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, scanned=1, + sim=sim) + assert "Carried forward" in body and "2026-07-29T10:00:00Z" in body + + +def test_a_no_bandaid_finding_says_the_code_fix_is_the_only_answer(): + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="low", changed=1, scanned=1) + assert "No band-aid" in body and "cosmetic" in body + + +def test_findings_held_back_by_the_threshold_are_counted_when_none_are_reported(): + """"We did not report this" must not read as "there was nothing": a clean-looking comment on a PR + that had two medium findings is the confusion this codebase exists to avoid.""" + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="critical", changed=1, + scanned=1) + assert "neg-pay-001" in body # critical is at the floor + + only_low = [f for f in FINDINGS if f["severity"] == "low"] + body2 = ci.render_comment(only_low, TRIAGE, [], min_severity="high", changed=1, scanned=1) + assert "No findings at or above `high`" in body2 + assert "1 lower-severity finding(s) were found and not reported" in body2 + + +def test_unscanned_changed_files_are_disclosed(): + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=50, + scanned=40, truncated=10) + assert "10 changed file(s) were not scanned" in body + assert "not covered by this review" in body + + +def test_the_comment_is_deterministic(): + """Rendered by code, not a model: the same inputs must produce the same bytes, or a re-run + rewrites the comment for no reason and nobody can diff two reviews.""" + a = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=3, scanned=1) + b = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=3, scanned=1) + assert a == b + + +# ============================================================ review orchestration +def _fake_run(out, findings, triage, policies, files=1): + def run_pipeline(repo_path=None, **kw): + o = kw["out_dir"] + from pathlib import Path + Path(o).mkdir(parents=True, exist_ok=True) + (Path(o) / "findings.json").write_text(json.dumps(findings)) + (Path(o) / "triage.json").write_text(json.dumps(list(triage.values()))) + (Path(o) / "policies.json").write_text(json.dumps(policies)) + (Path(o) / "metrics.json").write_text(json.dumps({"discovery": {"files": files}})) + return {"candidates": len(findings), "verified": len(findings)} + return run_pipeline + + +def test_a_pr_with_no_scannable_change_scans_nothing_and_posts_nothing(repo, monkeypatch, tmp_path): + _git(repo, "checkout", "-q", "-b", "docs-only", "main") + (repo / "DOC.md").write_text("docs\n") + _git(repo, "add", "-A") + _git(repo, "commit", "-qm", "docs") + called = {"ran": False} + from vpcopilot import pipeline + monkeypatch.setattr(pipeline, "run_pipeline", + lambda *a, **k: called.update(ran=True) or {}) + res = ci.review(str(repo), base="main", out_dir=str(tmp_path / "o"), log=lambda m: None) + assert res["reported"] == 0 and res["posted"] is False + assert not called["ran"], "the pipeline ran for a diff with no scannable files" + assert "no scannable files" in res["reason"] + + +def test_a_pr_with_no_findings_posts_nothing(repo, monkeypatch, tmp_path): + """Acceptance: "a PR with no new findings posts nothing". A bot that comments "all clear" on + every PR trains people to stop reading it.""" + from vpcopilot import pipeline + monkeypatch.setattr(pipeline, "run_pipeline", _fake_run(tmp_path, [], {}, [])) + posted = {"n": 0} + monkeypatch.setattr(ci, "upsert_comment", lambda *a, **k: posted.update(n=posted["n"] + 1)) + res = ci.review(str(repo), base="main", out_dir=str(tmp_path / "o"), post=True, + repo_slug="o/n", pr_number=1, log=lambda m: None) + assert res["reported"] == 0 and posted["n"] == 0 + assert "No findings at or above" in res["comment"] + + +def test_a_pr_with_a_known_flaw_gets_one_comment(repo, monkeypatch, tmp_path): + """Acceptance: one comment carrying the policy. One, not one per finding.""" + from vpcopilot import pipeline + monkeypatch.setattr(pipeline, "run_pipeline", _fake_run(tmp_path, FINDINGS, TRIAGE, POLICIES)) + calls = [] + monkeypatch.setattr(ci, "upsert_comment", + lambda slug, num, body, **k: calls.append((slug, num, body)) or + {"posted": True, "updated": False}) + res = ci.review(str(repo), base="main", out_dir=str(tmp_path / "o"), post=True, + repo_slug="o/n", pr_number=7, log=lambda m: None) + assert res["reported"] == 1 and len(calls) == 1 + assert calls[0][:2] == ("o/n", 7) + assert "deny-negative-pay-amount" in calls[0][2] + + +def test_posting_needs_both_the_slug_and_the_number(repo, monkeypatch, tmp_path): + from vpcopilot import pipeline + monkeypatch.setattr(pipeline, "run_pipeline", _fake_run(tmp_path, FINDINGS, TRIAGE, POLICIES)) + with pytest.raises(ci.CIError, match="--pr"): + ci.review(str(repo), base="main", out_dir=str(tmp_path / "o"), post=True, + repo_slug="o/n", log=lambda m: None) + + +def test_the_review_never_drafts_code_fixes(repo, monkeypatch, tmp_path): + """The band-aid and the finding are the deliverable; drafting the cure is the expensive half of a + scan, for a file the developer is editing by hand right now.""" + from vpcopilot import pipeline + seen = {} + + def spy(repo_path=None, **kw): + seen.update(kw) + return _fake_run(tmp_path, [], {}, [])(repo_path, **kw) + monkeypatch.setattr(pipeline, "run_pipeline", spy) + ci.review(str(repo), base="main", out_dir=str(tmp_path / "o"), log=lambda m: None) + assert seen["draft_code_fixes"] is False + assert seen["only_files"] == {"pay.py"} + + +def test_a_previous_simulation_on_disk_is_picked_up(repo, monkeypatch, tmp_path): + from vpcopilot import pipeline + o = tmp_path / "o" + o.mkdir() + (o / "simulation.json").write_text(json.dumps({"policies": [ + {"policy_name": "deny-negative-pay-amount", "evaluated": 200, "would_block": 1, + "block_rate": 0.005, "blocked_promotion": False}]})) + monkeypatch.setattr(pipeline, "run_pipeline", _fake_run(tmp_path, FINDINGS, TRIAGE, POLICIES)) + res = ci.review(str(repo), base="main", out_dir=str(o), log=lambda m: None) + assert "would block **1** of 200" in res["comment"] + + +def test_a_corrupt_simulation_file_does_not_become_a_fake_number(repo, monkeypatch, tmp_path): + from vpcopilot import pipeline + o = tmp_path / "o" + o.mkdir() + (o / "simulation.json").write_text("{ corrupt") + monkeypatch.setattr(pipeline, "run_pipeline", _fake_run(tmp_path, FINDINGS, TRIAGE, POLICIES)) + res = ci.review(str(repo), base="main", out_dir=str(o), log=lambda m: None) + assert "Blast radius: not measured" in res["comment"] + + +# ============================================================ never writes to XC +def test_the_ci_path_cannot_reach_the_tenant(): + """Structural, not promised: this module imports no tenant client and no apply path, so there is + no code path from CI to a load balancer. A test that reads the source is the only version of this + guarantee that survives someone adding a convenient import later.""" + import inspect + src = inspect.getsource(ci) + for forbidden in ("from .xc", "import xc", "XC(", "apply_from_scan", "refine_apply", + "simulate_policies", "promotion_gate", "retire_finding"): + assert forbidden not in src, f"ci.py references {forbidden}" + + +def test_xc_credentials_in_the_environment_are_warned_about(repo, monkeypatch, tmp_path): + """An XC credential present in CI is one any workflow on the repo can reach, and this path has no + use for it. Said out loud rather than silently tolerated.""" + from vpcopilot import pipeline + monkeypatch.setenv("XC_API_TOKEN", "should-not-be-here") + monkeypatch.setattr(pipeline, "run_pipeline", _fake_run(tmp_path, [], {}, [])) + lines = [] + ci.review(str(repo), base="main", out_dir=str(tmp_path / "o"), log=lines.append) + assert any("XC credentials are present" in ln for ln in lines) + + +def test_the_action_passes_no_xc_secrets(): + """The action definition is the other half of the guarantee: no XC input exists to pass one.""" + from pathlib import Path + y = Path(__file__).resolve().parents[1] / ".github/actions/vpcopilot-scan/action.yml" + text = y.read_text() + assert "XC_API_TOKEN" not in text and "XC_API_URL" not in text and "XC_NAMESPACE" not in text + assert "anthropic-api-key" in text and "github-token" in text + + +def test_the_action_declares_the_inputs_the_cli_supports(): + from pathlib import Path + y = (Path(__file__).resolve().parents[1] / ".github/actions/vpcopilot-scan/action.yml").read_text() + for inp in ("min-severity", "max-files", "fail-on-findings", "simulation-json", "base"): + assert f"{inp}:" in y, inp + # the merge-base problem is the first thing anyone hits, so the action deepens the checkout itself + assert "merge-base" in y and "deepen" in y + + +# ============================================================ path relativity +def test_a_subdirectory_scan_matches_the_diff_paths(tmp_path): + """The bug that would have shipped a silent all-clear. `git diff --name-only` answers relative to + the REPOSITORY ROOT; `collect_files` matches relative to the directory it is given. This project's + own app fixture lives eight levels down, so compared directly the two never match: zero files + scanned, no findings, and the PR told it is clean. Catching that needs noticing the absence of an + error, which is why it is pinned.""" + from vpcopilot.repo_scan import collect_files + r = tmp_path / "repo" + sub = r / "app" / "src" + sub.mkdir(parents=True) + (sub / "pay.py").write_text("def pay(a):\n return a\n") + (r / "top.py").write_text("x = 1\n") + + changed = {"app/src/pay.py", "top.py"} + inside, outside = ci.rebase_onto(changed, str(sub), str(r)) + assert inside == {"pay.py"} # rebased onto the scanned dir… + assert outside == 1 # …and the rest is counted, not forgotten + assert [f.name for f in collect_files(str(sub), only=inside)[0]] == ["pay.py"] + + +def test_a_diff_entirely_outside_the_scanned_directory_says_so(repo, monkeypatch, tmp_path): + """"Nothing in the directory I scan changed" is a different answer from "nothing is wrong", and + the reason has to survive into the result.""" + from vpcopilot import pipeline + ran = {"n": 0} + monkeypatch.setattr(pipeline, "run_pipeline", lambda *a, **k: ran.update(n=ran["n"] + 1) or {}) + only = repo / "elsewhere" + only.mkdir() + res = ci.review(str(only), base="main", out_dir=str(tmp_path / "o"), log=lambda m: None) + assert res["reported"] == 0 and ran["n"] == 0 + assert res["outside"] == 1 and "outside" in res["reason"] + + +def test_an_all_clear_discloses_an_unscanned_remainder(): + """An all-clear that hides what it did not scan is the one output this feature must never + produce.""" + body = ci.render_comment([], {}, [], min_severity="high", changed=50, scanned=40, + truncated=10, outside=3) + assert "No findings at or above" in body + assert "not scanned" in body and "not a clean bill of health" in body + assert "outside the scanned directory" in body + + +def test_the_pr_review_workflow_is_safe_and_self_consistent(): + """`pull_request_target` would run trusted workflow code with secrets against untrusted head + code — the standard way a repository leaks its secrets. Not worth a review comment. And the + checkout must be deep, or `git merge-base` has nothing to find.""" + from pathlib import Path + + import yaml + root = Path(__file__).resolve().parents[1] + wf = yaml.safe_load((root / ".github/workflows/pr-review.yml").read_text()) + # YAML 1.1 reads a bare `on:` key as the BOOLEAN True, so a workflow's triggers land under + # `True` rather than `"on"` with pyyaml. Accept either. + triggers = wf.get("on", wf.get(True)) + # assert on the parsed TRIGGERS, not the raw text — the file names pull_request_target in a + # comment explaining why it is not used, which is exactly the documentation we want to keep + assert "pull_request_target" not in triggers + assert "pull_request" in triggers + assert wf["permissions"]["pull-requests"] == "write" + assert wf["permissions"]["contents"] == "read" + steps = wf["jobs"]["review"]["steps"] + checkout = next(s for s in steps if str(s.get("uses", "")).startswith("actions/checkout")) + assert checkout["with"]["fetch-depth"] == 0 + # the test gate must not gain a model credential or a comment permission + ci_yml = yaml.safe_load((root / ".github/workflows/ci.yml").read_text()) + assert "review" not in ci_yml["jobs"] + + +def test_docs_ci_states_the_criterion_conflict_rather_than_papering_over_it(): + """The project's rule: where an acceptance criterion is unachievable, say so and explain why.""" + from pathlib import Path + txt = (Path(__file__).resolve().parents[1] / "docs/CI.md").read_text() + assert "contradicted itself" in txt + assert "never writes to XC" in txt and "three writes" in txt + assert "pull_request_target" in txt # the fork trade-off is documented, not hidden + + +def test_the_ci_fixture_stays_out_of_the_benchmark_scan_target(): + """The K2 fixture is a deliberately vulnerable route. Inside + `bench/fixtures/nimbus-vuln-lab/app/src/app/api` — the directory `bench` scans against + `answer_key.yaml` — its findings appear in neither `expected` nor `bonus`, and `bench.py` counts + anything else as NOISE. A test fixture would then silently degrade the precision column of the + committed scorecard and BASELINE.md. It lives in `bench/fixtures/ci/` instead.""" + from pathlib import Path + root = Path(__file__).resolve().parents[1] + target = root / "bench/fixtures/nimbus-vuln-lab/app/src/app/api" + assert (root / "bench/fixtures/ci/refund-route.js").is_file() + assert not (target / "refund").exists() + # the README says why, so the next person does not helpfully move it back + readme = (root / "bench/fixtures/ci/README.md").read_text() + assert "answer_key.yaml" in readme and "noise" in readme + + +# ============================================================ review findings, pinned +def test_a_zero_evaluated_simulation_is_no_evidence_not_a_low_rate(): + """Found by adversarial review. G2 treats zero evaluated as NO EVIDENCE, explicitly not as safe — + every replayed request failed in transit. Rendering "would block 0 of 0 (0.0%) — under the + threshold" turned a measurement that failed into the most reassuring line in the comment.""" + sim = {"deny-negative-pay-amount": { + "policy_name": "deny-negative-pay-amount", "evaluated": 0, "would_block": 0, + "block_rate": 0.0, "blocked_promotion": False, + "reason": "zero evidence: every replayed request failed in transit"}} + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, scanned=1, + sim=sim) + assert "no evidence" in body and "nothing was measured" in body + assert "under the threshold" not in body + assert "would block **0** of 0" not in body + + +def test_a_comment_over_githubs_cap_is_truncated_with_the_count_dropped(): + """GitHub rejects an issue-comment body over 65536 characters, and the failure mode is the whole + comment being lost to an API error — the review silently not happening.""" + many = [{"id": f"f-{i}", "title": f"Finding {i}", "severity": "critical", "file": "a.py", + "description": "x" * 3000} for i in range(60)] + triage = {f["id"]: {"finding_id": f["id"], + "bandaids": [{"control": "service_policy", "recommended": True}]} + for f in many} + pols = [{"finding_id": f["id"], "control": "service_policy", "policy_name": f"p-{i}"} + for i, f in enumerate(many)] + body = ci.render_comment(many, triage, pols, min_severity="high", changed=1, scanned=1) + assert len(body) <= ci.MAX_BODY + assert "comment was truncated" in body + assert "of 60 finding(s) are not shown" in body + + +def test_a_short_comment_is_not_truncated(): + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, scanned=1) + assert "truncated" not in body + + +def test_the_marker_is_matched_at_the_start_so_a_quoted_reply_is_not_overwritten(): + """A human quoting the bot produces `> `. Matching with `in` would + find that and overwrite the person's comment with the review.""" + import inspect + src = inspect.getsource(ci.upsert_comment) + assert ".startswith(MARKER)" in src + assert "if MARKER in" not in src + # and our own comments really do start with it, or the anchor never matches at all + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, scanned=1) + assert body.startswith(ci.MARKER) + + +def test_the_action_normalises_an_origin_prefixed_base(): + """`base: origin/main` is the natural thing to pass — it is the CLI's own default — and blindly + re-prefixing asks git for `origin/origin/main`, which has no merge base.""" + from pathlib import Path + y = (Path(__file__).resolve().parents[1] + / ".github/actions/vpcopilot-scan/action.yml").read_text() + assert 'BASE_REF="${BASE_REF_RAW#origin/}"' in y + + +def test_the_action_passes_inputs_through_env_not_shell_interpolation(): + """A `${{ }}` expansion inside `run:` is substituted as TEXT before bash parses the line, which + is the standard GitHub Actions script-injection vector.""" + import re + + from pathlib import Path + + import yaml + d = yaml.safe_load((Path(__file__).resolve().parents[1] + / ".github/actions/vpcopilot-scan/action.yml").read_text()) + for s in d["runs"]["steps"]: + for ln in (s.get("run") or "").splitlines(): + if ln.lstrip().startswith("#"): + continue # a comment explaining the hazard is not the hazard + for m in re.findall(r"\$\{\{\s*[\w.\-]+[^}]*\}\}", ln): + assert any(k in m for k in ("github.action_path", "github.workspace")), \ + f"{s.get('name')} interpolates {m} into a shell command" + + +def test_a_non_ascii_or_spaced_filename_is_not_silently_dropped(tmp_path): + """Found while verifying the review. Git QUOTES paths by default: a non-ASCII filename comes back + from `--name-only` as `"caf\\303\\251.py"` — C-style octal escapes inside literal quotes — so its + suffix reads as `.py"`, no known code extension matches, and the file is dropped from the scan set + *and* not counted as outside it. It vanishes, and the pull request is told it is clean. `-z` + output is not quoted at all, which settles spaces and newlines too.""" + from vpcopilot.repo_scan import collect_files + r = tmp_path / "repo" + r.mkdir() + subprocess.run(["git", "init", "-q", str(r)], check=True) + _git(r, "config", "user.email", "t@e.com") + _git(r, "config", "user.name", "t") + (r / "base.py").write_text("x = 1\n") + _git(r, "add", "-A") + _git(r, "commit", "-qm", "base") + _git(r, "branch", "-M", "main") + _git(r, "checkout", "-q", "-b", "feature") + for name in ("café.py", "my file.py", "日本語.py"): + (r / name).write_text("def f():\n pass\n") + _git(r, "add", "-A") + _git(r, "commit", "-qm", "odd names") + + changed, _, _total = ci.changed_files(str(r), base="main", log=lambda m: None) + assert changed == {"café.py", "my file.py", "日本語.py"} + assert not any('"' in n or "\\3" in n for n in changed) # nothing arrived quoted + inside, outside = ci.rebase_onto(changed, str(r), str(r)) + assert outside == 0 + assert {f.name for f in collect_files(str(r), only=inside)[0]} == changed + + +def test_only_verified_findings_are_reported_never_refuted_candidates(repo, monkeypatch, tmp_path): + """Found by adversarial review. `findings.json` is the DISCOVER contract — every candidate, + including the ones `verify` refuted as false positives — and the old filter compared that set + against itself, so it was a no-op. Triage runs only over the verified set, so a triage decision + is what "survived verification" looks like on disk. Putting refuted false positives in front of a + developer with a band-aid attached is the fastest way to teach a team to ignore this bot.""" + from vpcopilot import pipeline + o = tmp_path / "o" + candidates = FINDINGS + [{"id": "refuted-001", "title": "Not real", "severity": "critical", + "file": "pay.py"}] + monkeypatch.setattr(pipeline, "run_pipeline", + _fake_run(tmp_path, candidates, TRIAGE, POLICIES)) + res = ci.review(str(repo), base="main", out_dir=str(o), log=lambda m: None) + assert res["candidates"] == 3 and res["refuted"] == 1 + assert {f["id"] for f in res["findings"]} == {"neg-pay-001", "low-001"} + assert res["reported"] == 1 # only the critical verified one + assert "refuted-001" not in res["comment"] + assert "Not real" not in res["comment"] + + +def test_truncation_keeps_the_disclosures_it_used_to_delete(): + """The naive truncation cut from the end — which is exactly where "N files were not scanned" and + "N sit outside the scanned directory" live. The sentences that stop a partial review reading as a + complete one were the first thing dropped.""" + many = [{"id": f"f-{i}", "title": f"Finding {i}", "severity": "critical", "file": "a.py", + "description": "x" * 3000} for i in range(60)] + triage = {f["id"]: {"finding_id": f["id"], + "bandaids": [{"control": "service_policy", "recommended": True}]} + for f in many} + body = ci.render_comment(many, triage, [], min_severity="high", changed=90, scanned=40, + truncated=12, outside=5) + assert len(body) <= ci.MAX_BODY + assert "comment was truncated" in body + assert "12 changed file(s) were not scanned" in body # survived the cut + assert "5 changed file(s) sit outside" in body # survived the cut + assert body.rstrip().endswith("") # footer intact + + +def test_an_unconfirmed_enforcement_is_not_reported_as_a_rate(): + """G2's FIRST live defect: attaching a policy and replaying immediately counted an UNENFORCED + policy as harmless — 0 of 200 blocked, including the exploit. A rate measured before the edge was + confirmed enforcing measures nothing.""" + sim = {"deny-negative-pay-amount": { + "policy_name": "deny-negative-pay-amount", "evaluated": 200, "would_block": 0, + "block_rate": 0.0, "blocked_promotion": False, "enforcement_confirmed": False}} + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, scanned=1, + sim=sim) + assert "Blast radius: unconfirmed" in body + assert "under the threshold" not in body + assert "would block **0** of 200" not in body + + +def test_a_confirmed_enforcement_still_reports_the_rate(): + """No regression: a confirmed simulation reports its numbers as before. Absent + `enforcement_confirmed` also reports, so an older simulation.json is not treated as unconfirmed.""" + for extra in ({"enforcement_confirmed": True}, {}): + sim = {"deny-negative-pay-amount": { + "policy_name": "deny-negative-pay-amount", "evaluated": 200, "would_block": 1, + "block_rate": 0.005, "blocked_promotion": False, **extra}} + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, + scanned=1, sim=sim) + assert "would block **1** of 200" in body, extra + assert "unconfirmed" not in body, extra + + +def test_an_unexpected_failure_exits_2_not_1(repo, monkeypatch, tmp_path): + """Exit 1 means "findings were reported" and the action reads it that way, so a crash exiting 1 + would make a review that never completed look like a completed one with findings — and the + workflow would publish a comment file that does not exist.""" + from typer.testing import CliRunner + + from vpcopilot import cli + monkeypatch.setattr("vpcopilot.ci.review", + lambda *a, **k: (_ for _ in ()).throw(RuntimeError("model API down"))) + r = CliRunner().invoke(cli.app, ["ci-review", "--repo", str(repo), "--base", "main", + "--out", str(tmp_path / "o")]) + assert r.exit_code == 2, r.output + + +def test_an_unknown_min_severity_is_rejected_rather_than_defaulted(repo, tmp_path): + """Unvalidated, it fell through `SEVERITY_RANK.get(..., 1)` to `high` — and the comment then + stated a threshold the run had not applied.""" + from typer.testing import CliRunner + + from vpcopilot import cli + r = CliRunner().invoke(cli.app, ["ci-review", "--repo", str(repo), "--base", "main", + "--min-severity", "urgent", "--out", str(tmp_path / "o")]) + assert r.exit_code == 2 and "must be critical, high, medium or low" in r.output + + +def test_the_action_summary_distinguishes_a_crash_from_a_clean_review(): + """`if: always()` means the summary also runs when the review crashed. Without knowing which, it + wrote "no findings at or above the threshold" for a diff that was never reviewed.""" + from pathlib import Path + y = (Path(__file__).resolve().parents[1] + / ".github/actions/vpcopilot-scan/action.yml").read_text() + assert "steps.review.outcome" in y + assert "did not complete" in y and "not a clean bill of health" in y + + +def test_a_diff_outside_the_scanned_directory_produces_a_comment_that_says_so(repo, monkeypatch, + tmp_path): + """Found by adversarial review — the mirror image of the rebase bug. The early return handed back + `comment: ""`, so `--comment-out` wrote nothing and the action's step summary fell through to "no + findings at or above the threshold": a clean bill of health for a diff that was never reviewed. + Nothing is POSTED (there is nothing to report), but the surface a human reads must say which of + the two happened.""" + from vpcopilot import pipeline + monkeypatch.setattr(pipeline, "run_pipeline", lambda *a, **k: {}) + only = repo / "elsewhere" + only.mkdir() + res = ci.review(str(only), base="main", out_dir=str(tmp_path / "o"), log=lambda m: None) + assert res["reported"] == 0 and res["posted"] is False + assert res["comment"], "an empty comment renders downstream as a clean review" + assert "Nothing was reviewed" in res["comment"] + assert "not** a clean bill of health" in res["comment"] + assert res["comment"].startswith(ci.MARKER) + + +def test_the_header_denominator_is_the_whole_diff_not_just_the_supported_files(): + """"Scanned 1 of 1" for a diff of eleven files reads as a complete review of the whole change.""" + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, scanned=1, + diff_total=11) + assert "of 1 scannable file(s)" in body and "11 changed in total" in body + + +def test_the_not_scanned_disclosure_names_every_cause_not_just_the_caps(): + """`truncated` counts every file the collector declined for ANY reason — vendored directories and + unsupported extensions included — so naming only the caps was wrong, and telling the reader to + raise them was advice that could not work.""" + for kw in ({"truncated": 3}, {"truncated": 3, "min_severity": "critical"}): + body = ci.render_comment( + FINDINGS if "min_severity" not in kw else [], TRIAGE, POLICIES, + min_severity=kw.get("min_severity", "high"), changed=10, scanned=7, + truncated=kw["truncated"]) + assert "vendored directory" in body and "unsupported file type" in body, kw + + +def test_a_repo_outside_a_git_worktree_declines_with_a_reason(tmp_path): + """Previously an unguarded `rev-parse --show-toplevel` propagated a raw CalledProcessError.""" + outside = tmp_path / "not-a-repo" + outside.mkdir() + with pytest.raises(ci.CIError): + ci.changed_files(str(outside), base="main", log=lambda m: None) + + +def test_a_rate_measured_against_bodyless_traffic_is_qualified(tmp_path): + """Found by adversarial review, and the third instance of the same shape in one review (after + not-measured, unconfirmed and zero-evaluated). `bodies_available` is false when the sample came + from XC access logs, which capture no request body — so a `body_matcher` service policy matches + nothing during replay and scores a perfectly clean 0%. Keeping only the per-policy dicts threw + away the one field that says the rate is unjudgeable.""" + (tmp_path / "simulation.json").write_text(json.dumps({ + "ts": "2026-07-29T10:00:00Z", "bodies_available": False, "records": 500, + "caveats": ["Records came from XC access logs, which capture no request body"], + "policies": [{"policy_name": "deny-negative-pay-amount", "evaluated": 500, + "would_block": 0, "block_rate": 0.0, "blocked_promotion": False, + "enforcement_confirmed": True}]})) + sim, meta = ci._blast_radius(str(tmp_path)) + assert meta["bodies_available"] is False and meta["caveats"] + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, scanned=1, + sim=sim, sim_meta=meta) + assert "no request bodies" in body + assert "unmeasured, not safe" in body + + +def test_a_body_carrying_sample_is_not_qualified(tmp_path): + """No regression: a HAR sample carries bodies, so its rate stands unqualified.""" + (tmp_path / "simulation.json").write_text(json.dumps({ + "bodies_available": True, + "policies": [{"policy_name": "deny-negative-pay-amount", "evaluated": 200, + "would_block": 1, "block_rate": 0.005, "blocked_promotion": False}]})) + sim, meta = ci._blast_radius(str(tmp_path)) + body = ci.render_comment(FINDINGS, TRIAGE, POLICIES, min_severity="high", changed=1, scanned=1, + sim=sim, sim_meta=meta) + assert "would block **1** of 200" in body and "no request bodies" not in body + + +def test_an_absent_simulation_returns_two_empty_dicts(tmp_path): + assert ci._blast_radius(str(tmp_path)) == ({}, {}) From b33a7ab602acb776a92032b3c17dbfdba8a4fc6f Mon Sep 17 00:00:00 2001 From: henleda Date: Wed, 29 Jul 2026 21:52:45 -0500 Subject: [PATCH 2/2] fix(ci): make the moved G2 gate work without tenant credentials, and fix the action manifest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two defects the PR's own CI found that a local run could not. 1. The moved blast-radius gate needed XC credentials to refuse. Both apply paths constructed XC() before reaching it, and XC.__init__ raises when XC_API_URL / XC_API_TOKEN are unset — so on a runner without credentials "this policy is too broad" came back as "XC_API_URL not set". The test passed locally only because the developer's .env happened to have them, which is a test depending on developer-local state. Every refusal that needs no tenant now precedes XC(), which is also better behaviour: an over-broad policy should be refused whether or not a tenant is reachable. Pinned by tests that unset the variables. 2. A comment inside the action broke the action. The runner parses Actions expression syntax anywhere in a run block, comments included, so a comment spelling out an empty expression while explaining the script-injection hazard failed the manifest to load with "An expression was expected". pyyaml cannot see it and the guard test skipped comment lines — exactly where the text was. The test no longer skips them. Also, found while fixing those: the repo has no secrets configured, so secrets.ANTHROPIC_API_KEY is empty, and `required: true` on an action input is not enforced against an empty value. The action would have scanned nothing and read as clean — an open review finding. It now fails loudly and names the cause, the workflow skips with an explicit "not a clean bill of health" summary rather than going permanently red, and the paths filter is scoped to the directory the action actually scans instead of every code file in the repo. 738 tests pass with XC_API_URL/XC_API_TOKEN/XC_NAMESPACE unset and no .env. Co-Authored-By: Claude Opus 5 (1M context) --- .github/actions/vpcopilot-scan/action.yml | 27 ++++++++++++- .github/workflows/pr-review.yml | 46 +++++++++++++++++------ ROADMAP.md | 25 ++++++++++++ src/vpcopilot/apply.py | 31 +++++++++------ src/vpcopilot/refiner.py | 8 +++- tests/test_ci.py | 23 ++++++++++++ tests/test_simulate.py | 28 ++++++++++++++ 7 files changed, 163 insertions(+), 25 deletions(-) diff --git a/.github/actions/vpcopilot-scan/action.yml b/.github/actions/vpcopilot-scan/action.yml index 4bfbba2..e68f13b 100644 --- a/.github/actions/vpcopilot-scan/action.yml +++ b/.github/actions/vpcopilot-scan/action.yml @@ -73,6 +73,26 @@ outputs: runs: using: composite steps: + - name: Check the model credential is actually present + shell: bash + env: + KEY: ${{ inputs.anthropic-api-key }} + run: | + # `required: true` on an input is not enforced by the runner when the caller passes an empty + # value — and `secrets.ANTHROPIC_API_KEY` is the empty string when the secret is not set. The + # scan would then fail deep inside the pipeline, or worse, discover nothing and read as a + # clean review. Fail here, where the message can name the cause. + if [ -z "${KEY}" ]; then + { + echo "## ⚠️ The review did not run" + echo + echo "\`anthropic-api-key\` is empty — the repository secret is probably not set." + echo "**This is not a clean bill of health**: no code was analysed." + } >> "$GITHUB_STEP_SUMMARY" + echo "::error::anthropic-api-key is empty — set the ANTHROPIC_API_KEY repository secret" + exit 1 + fi + - name: Set up Python uses: actions/setup-python@v5 with: @@ -114,10 +134,15 @@ runs: env: SIM_JSON: ${{ inputs.simulation-json }} run: | - # Through env, not interpolated into the command: a `${{ }}` expansion inside `run:` is + # Through env, not interpolated into the command: an Actions expression inside `run:` is # substituted as TEXT before bash sees it, so any input carrying shell metacharacters is a # script-injection vector. These inputs come from the workflow author rather than a PR, but # the safe form costs nothing and the habit is what matters. + # + # This comment deliberately does NOT spell that expression out. The runner parses expression + # syntax anywhere in a run block, comments included, so writing an empty one here failed the + # whole action to load with "An expression was expected" — which pyyaml cannot see, because + # it parses YAML and not Actions templates. mkdir -p out-ci cp "$SIM_JSON" out-ci/simulation.json echo "blast-radius numbers will come from $SIM_JSON" diff --git a/.github/workflows/pr-review.yml b/.github/workflows/pr-review.yml index 67452e1..b744378 100644 --- a/.github/workflows/pr-review.yml +++ b/.github/workflows/pr-review.yml @@ -13,16 +13,15 @@ name: pr-review on: pull_request: + # Scoped to the directory the action below actually SCANS, not to every code file in the repo. + # A `**/*.py` filter fired on any change — including this repo's own `src/vpcopilot/**` — and + # then scanned zero files, because `repo-path` points at the demo app. That spends a model + # credential and a runner to review nothing. Point both at the same place, or the trigger and + # the scan disagree about what this workflow is for. paths: - # Only worth spending a model call when code actually changed. - - "**/*.py" - - "**/*.js" - - "**/*.ts" - - "**/*.jsx" - - "**/*.tsx" - - "**/*.go" - - "**/*.rb" - - "**/*.java" + - "bench/fixtures/nimbus-vuln-lab/app/src/app/api/**" + - ".github/workflows/pr-review.yml" + - ".github/actions/vpcopilot-scan/**" workflow_dispatch: permissions: @@ -36,17 +35,40 @@ concurrency: jobs: review: - # A fork PR has no access to secrets, so the scan would fail for want of a model key. Skip - # loudly-by-omission rather than fail a check nobody can fix from a fork. - if: github.event.pull_request.head.repo.full_name == github.repository || github.event_name == 'workflow_dispatch' + # Two reasons to skip rather than fail. A fork PR has no access to secrets, so the scan could not + # run; and a repository that has not set ANTHROPIC_API_KEY has nothing to run it with. Neither is + # something the PR author can fix, and a permanently red check is a check people learn to ignore. + # Skipping is safe here only because the action itself fails loudly on an empty key — so the + # combination cannot produce a green check for a review that did not happen. + if: >- + (github.event.pull_request.head.repo.full_name == github.repository + || github.event_name == 'workflow_dispatch') + && github.event.repository.fork == false runs-on: ubuntu-latest + env: + # Read once at job level so both the skip note and the review step key off the same value. + MODEL_KEY: ${{ secrets.ANTHROPIC_API_KEY }} steps: + - name: Say so when there is no model credential to scan with + if: env.MODEL_KEY == '' + run: | + { + echo "## The PR review was skipped" + echo + echo "\`ANTHROPIC_API_KEY\` is not set for this repository, so there is nothing to scan" + echo "with. **This is not a clean bill of health** — the diff was not reviewed." + echo + echo "Set the secret, or run \`vpcopilot ci-review\` locally (see docs/CI.md)." + } >> "$GITHUB_STEP_SUMMARY" + - uses: actions/checkout@v4 + if: env.MODEL_KEY != '' with: # `git merge-base` needs history on both sides; the default single-commit fetch has none. fetch-depth: 0 - name: Review the diff + if: env.MODEL_KEY != '' uses: ./.github/actions/vpcopilot-scan with: # The demo app this repo carries. Point this at your own source directory. diff --git a/ROADMAP.md b/ROADMAP.md index f9082cc..85d6e21 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -965,6 +965,31 @@ sees the trail. J1–J4 are the open `BACKLOG.md` evidence entries, scheduled; * the header's "scanned N of M" used the post-filter count as its denominator, so a fourteen-file diff read as "1 of 1"; and the "not scanned" line named only the caps when the count also includes vendored directories and unsupported file types. + - **Two defects only CI could find, and both are worth reading.** The suite passed locally and the + PR went red immediately. + - **The moved G2 gate needed tenant credentials to refuse.** `refine_apply_service_policy` and + `apply_from_scan` both constructed `XC()` before reaching the gate, and `XC.__init__` raises when + `XC_API_URL`/`XC_API_TOKEN` are unset — so on a runner without credentials "this policy is too + broad" came back as "XC_API_URL not set". The test passed locally *only because the developer's + `.env` happened to have them*: a test that quietly depended on developer-local state, which is + precisely what "tests run offline against fakes" exists to prevent. Every refusal that needs no + tenant now precedes `XC()`, which is also better behaviour — an over-broad policy should be + refused whether or not a tenant is reachable. Reproducing CI locally is one `env -u` away and is + now part of the check. + - **A comment inside the action broke the action.** The runner parses Actions expression syntax + anywhere in a `run:` block — **comments included** — so a comment that spelled out an empty + expression while explaining the script-injection hazard failed the whole action to load with + "An expression was expected". Neither local check could see it: `pyyaml` parses YAML rather than + Actions templates, and the test that scanned for interpolations skipped comment lines, which is + exactly where the offending text was. The test no longer skips them, because the runner does not. + - **Found while fixing those:** the repository has **no secrets configured**, so + `secrets.ANTHROPIC_API_KEY` is the empty string — and `required: true` on an action input is not + enforced against an empty value. The action would have scanned nothing and produced a review that + read as clean, which was also a review finding left open. It now fails loudly on an empty key and + names the cause, and the workflow skips with a stated "this is not a clean bill of health" summary + rather than going permanently red for something no PR author can fix. The `paths` filter was also + scoped to the directory the action actually scans — it fired on any `**/*.py` change and then + scanned zero files, spending a credential and a runner to review nothing. - **The fixture lives in `bench/fixtures/ci/`, not in the Nimbus app, and that is not tidiness.** `bench` scans `bench/fixtures/nimbus-vuln-lab/app/src/app/api` against `answer_key.yaml`, and a new vulnerable route there produces findings listed in neither `expected` nor `bonus` — which diff --git a/src/vpcopilot/apply.py b/src/vpcopilot/apply.py index 39ba51a..d9837cd 100644 --- a/src/vpcopilot/apply.py +++ b/src/vpcopilot/apply.py @@ -376,7 +376,12 @@ def apply_from_scan(artifact_path: str, lb: str, target_url: str, *, name: str | `lb_snapshot.json` and a fresh `snapshots/-.json` on every call and `self_test()` always PUTs, so "reports no_change and writes nothing" has to short-circuit earlier than the mutation. `force=True` re-applies anyway (to re-validate a policy that is already attached).""" - xc = XC() + # Everything down to the `XC()` below is a pure disk read, and it is ordered that way on purpose: + # `XC.__init__` raises when `XC_API_URL`/`XC_API_TOKEN` are unset, so constructing the client + # first made every refusal here depend on having tenant credentials. On a CI runner that turned + # "this policy is too broad" into "XC_API_URL not set", which is a different answer to a different + # question. A refusal that needs no tenant should not require one. + # # The protected-LB guardrail, unconditionally. It used to be reached only via # `apply_service_policy`, which `create_only` returns before ever calling — so # `apply_from_scan(lb="nimbus-www", create_only=True)` wrote a real policy object into the @@ -400,18 +405,10 @@ def apply_from_scan(artifact_path: str, lb: str, target_url: str, *, name: str | raise RuntimeError(f"no policy name for {artifact_path}; pass name=...") if policy_name in PROTECTED_POLICIES: raise RuntimeError(f"refusing to create/overwrite protected policy '{policy_name}'") - body = {"metadata": {"name": policy_name, "namespace": xc.ns}, "spec": spec} - for k in ("labels", "annotations", "description", "disable"): - if src_meta.get(k) is not None: - body["metadata"][k] = src_meta[k] - # I2 — the pre-apply gate runs before the CREATE, not just before the attach: a run that is - # about to be refused should not leave a stray policy object behind in the tenant. It also has - # to precede `ApplyContext.load()`, which writes a snapshot on every call, so "no_change writes - # nothing" is true of the run directory as well as the LB. `create_only` makes no attachment, - # so there is nothing for it to gate. # G2 — the blast-radius gate, now in the module so the CLI, the console and the MCP server - # cannot disagree about it. Runs before the CREATE for the same reason the drift check does. + # cannot disagree about it. Runs before the CREATE for the same reason the drift check does, and + # before `XC()` for the reason given at the top of this function. from .simulate import promotion_gate # The audit record's join key: unlike the refiner, this path never resolves `finding_id` from the # ledger, so an override recorded here would lose the one field that ties it to a finding. @@ -422,6 +419,18 @@ def apply_from_scan(artifact_path: str, lb: str, target_url: str, *, name: str | promotion_gate(out_dir, policy_name, allow_overbroad=allow_overbroad, dry_run=dry_run, finding_id=_fid, lb=lb, log=log) + xc = XC() + body = {"metadata": {"name": policy_name, "namespace": xc.ns}, "spec": spec} + for k in ("labels", "annotations", "description", "disable"): + if src_meta.get(k) is not None: + body["metadata"][k] = src_meta[k] + + # I2 — the pre-apply gate runs before the CREATE, not just before the attach: a run that is + # about to be refused should not leave a stray policy object behind in the tenant. It also has + # to precede `ApplyContext.load()`, which writes a snapshot on every call, so "no_change writes + # nothing" is true of the run directory as well as the LB. `create_only` makes no attachment, + # so there is nothing for it to gate. + if not dry_run and not create_only: from .drift import preflight d = preflight(lb, policy_name, out_dir=out_dir, force=force, xc=xc, log=log, spec=spec, diff --git a/src/vpcopilot/refiner.py b/src/vpcopilot/refiner.py index a085965..03b0e32 100644 --- a/src/vpcopilot/refiner.py +++ b/src/vpcopilot/refiner.py @@ -89,7 +89,6 @@ def refine_apply_service_policy(artifact_path: str, lb: str, target_url: str, *, this (not `apply_from_scan`) is the default path for both the CLI's `--from-scan` and the console's Mitigate button. A guard the primary UX skips is not a guard. `force=True` bypasses it.""" - xc = XC() if lb in _protected_lbs() and not allow_protected: raise RuntimeError(f"refusing to mutate protected LB '{lb}'. Pass allow_protected=True to override.") max_refine = max_refine or refine_attempts_default() @@ -108,10 +107,17 @@ def refine_apply_service_policy(artifact_path: str, lb: str, target_url: str, *, # G2 — this is the DEFAULT path for both the CLI's `--from-scan` and the console's Mitigate # button, so the blast-radius gate has to be here as well as in `apply_from_scan`; a guard the # primary UX skips is not a guard. Same reasoning as the drift preflight below. + # + # BEFORE `XC()`, and CI is what proved that matters. Every refusal above this line is a pure disk + # read, but `XC.__init__` raises when `XC_API_URL`/`XC_API_TOKEN` are unset — so constructing the + # client first meant an over-broad policy was refused for the wrong reason on any machine without + # tenant credentials, which is every CI runner. The cheap, credential-free refusals belong first: + # an over-broad policy should be refused whether or not a tenant is reachable. from .simulate import promotion_gate promotion_gate(out_dir, policy_name, allow_overbroad=allow_overbroad, finding_id=finding_id, lb=lb, log=log) + xc = XC() from .drift import preflight d = preflight(lb, policy_name, out_dir=out_dir, force=force, xc=xc, log=log, spec=spec, exploit=(probe or {}).get("exploit"), refine=True) diff --git a/tests/test_ci.py b/tests/test_ci.py index 89716a6..f9f8941 100644 --- a/tests/test_ci.py +++ b/tests/test_ci.py @@ -749,3 +749,26 @@ def test_a_body_carrying_sample_is_not_qualified(tmp_path): def test_an_absent_simulation_returns_two_empty_dicts(tmp_path): assert ci._blast_radius(str(tmp_path)) == ({}, {}) + + +def test_the_action_contains_no_actions_expression_inside_any_run_block(): + """CI caught this and both local checks missed it. The runner parses expression syntax anywhere in + a `run:` block — COMMENTS INCLUDED — so a comment that spelled out an empty expression failed the + whole action to load with "An expression was expected". pyyaml cannot see it (it parses YAML, not + Actions templates), and the earlier version of this test skipped comment lines, which is exactly + where the offending text was. Comments are not skipped here, because the runner does not skip + them.""" + import re + + from pathlib import Path + + import yaml + d = yaml.safe_load((Path(__file__).resolve().parents[1] + / ".github/actions/vpcopilot-scan/action.yml").read_text()) + for s in d["runs"]["steps"]: + for i, ln in enumerate((s.get("run") or "").splitlines(), 1): + for m in re.findall(r"\$\{\{.*?\}\}", ln): + assert any(k in m for k in ("github.action_path", "github.workspace")), \ + f"{s.get('name')!r} line {i} contains {m!r} inside a run block" + # an unterminated or empty expression is the specific shape that broke the load + assert "${{ }}" not in ln, f"{s.get('name')!r} line {i}: empty Actions expression" diff --git a/tests/test_simulate.py b/tests/test_simulate.py index 1fa42a8..6f7bba1 100644 --- a/tests/test_simulate.py +++ b/tests/test_simulate.py @@ -422,3 +422,31 @@ def test_a_full_replay_still_replaces_everything_it_measured(tmp_path): assert len(doc["policies"]) == 1 and doc["policies"][0]["blocked_promotion"] is False assert simulate.promotion_block(str(tmp_path), "A") is None assert not doc.get("caveats") + + +def test_the_gate_refuses_without_any_tenant_credentials(monkeypatch, tmp_path): + """CI caught this and local runs could not. Every refusal before `XC()` is a pure disk read, but + `XC.__init__` raises when `XC_API_URL`/`XC_API_TOKEN` are unset — so constructing the client + first turned "this policy is too broad" into "XC_API_URL not set" on any machine without tenant + credentials, which is every CI runner. The test passed locally only because the developer's `.env` + happened to have them: a test that depended on developer-local state, which is the invariant + `tests/` exists to keep. A refusal that needs no tenant must not require one.""" + from vpcopilot import apply as A + for var in ("XC_API_URL", "XC_API_TOKEN", "XC_NAMESPACE"): + monkeypatch.delenv(var, raising=False) + _flagged(tmp_path, "wide") + art = _artifact(tmp_path, "wide") + with pytest.raises(RuntimeError, match="allow overbroad"): + A.apply_from_scan(art, "lab", "http://lab.test", out_dir=str(tmp_path), + log=lambda m: None) + + +def test_the_refiner_gate_also_refuses_without_credentials(monkeypatch, tmp_path): + from vpcopilot import refiner as R + for var in ("XC_API_URL", "XC_API_TOKEN", "XC_NAMESPACE"): + monkeypatch.delenv(var, raising=False) + _flagged(tmp_path, "wide") + art = _artifact(tmp_path, "wide") + with pytest.raises(RuntimeError, match="allow overbroad"): + R.refine_apply_service_policy(art, "lab", "http://lab.test", out_dir=str(tmp_path), + log=lambda m: None)