diff --git a/.github/actions/vpcopilot-scan/action.yml b/.github/actions/vpcopilot-scan/action.yml new file mode 100644 index 0000000..e68f13b --- /dev/null +++ b/.github/actions/vpcopilot-scan/action.yml @@ -0,0 +1,223 @@ +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: 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: + 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: 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" + + - 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..b744378 --- /dev/null +++ b/.github/workflows/pr-review.yml @@ -0,0 +1,83 @@ +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: + # 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: + - "bench/fixtures/nimbus-vuln-lab/app/src/app/api/**" + - ".github/workflows/pr-review.yml" + - ".github/actions/vpcopilot-scan/**" + 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: + # 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. + 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..85d6e21 100644 --- a/ROADMAP.md +++ b/ROADMAP.md @@ -876,12 +876,127 @@ 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. + - **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 + `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/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/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/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/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..f9f8941 --- /dev/null +++ b/tests/test_ci.py @@ -0,0 +1,774 @@ +"""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)) == ({}, {}) + + +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)