test(eval): measure review findings, not just review outcomes

Makefile

@@ -87,6 +87,7 @@ script-test:
 	$(call run-timed,bash .github/scripts/select-eval-agents-test.sh)
 	$(call run-timed,python3 scripts/process-fix-result-test.py)
 	$(call run-timed,bash eval/scripts/scrub-eval-results-test.sh)
+	$(call run-timed,python3 eval/scripts/review-findings-judge-test.py)
 	$(call run-timed,bash .github/scripts/check-rollup-result-test.sh)
 
 test: script-test

eval/README.md

@@ -110,9 +110,11 @@ Each case directory under `eval/<agent>/cases/` contains:
 - `input.yaml` — fixture definition (forge, fixture type, title, body,
   PR files)
 - `annotations.yaml` — expected outcomes (labels, review expectations,
-  `max_turns`, `max_cost_usd`)
+  `max_turns`, `max_cost_usd`, and for the review suite the optional
+  `required_findings` / `forbidden_findings` ground truth)
 - `repo/` (optional) — base repo contents pushed to main before the
-  fixture is created
+  fixture is created, either inline or a symlink to a fixture repo
+  shared by several cases under `eval/<agent>/repos/`
 
 ## Lifecycle
 
@@ -123,15 +125,25 @@ Each test case follows this lifecycle:
 2. **`run-fullsend.sh`** — clones the ephemeral repo and runs the agent
    pipeline against it.
 3. **`capture-fixture.sh`** — snapshots the fixture state (labels,
-   comments, reviews) into `fixture-state.json` for judges.
+   comments, reviews, and for PR fixtures the inline review comments
+   that carry each review finding) into `fixture-state.json` for judges.
 4. **`teardown-fixture.sh`** — deletes the ephemeral repo.
 
 ## Known issues
 
-- **Self-review 422.** The runner reuses `GH_TOKEN` as `REVIEW_TOKEN`.
-  If the token owner is also the PR author, GitHub rejects
-  `REQUEST_CHANGES` reviews on your own PR. Use a token from a
-  different account or a GitHub App installation token.
+- **Self-review 422.** The runner falls back to `GH_TOKEN` for
+  `REVIEW_TOKEN`, and `setup-fixture.sh` opens the fixture PR with the
+  same token, so the reviewer is the PR author. GitHub rejects both
+  `APPROVE` and `REQUEST_CHANGES` reviews on your own PR, and fullsend's
+  422 retry re-submits the same event, so the whole submission fails and
+  no finding reaches the PR.
+
+  `post-review.sh` detects the collision and degrades the review event to
+  `COMMENT`, so the sticky write-up and the inline findings still land and
+  the findings judges still have something to measure. The outcome label
+  continues to follow the agent's real verdict. That is a workaround: set
+  `REVIEW_TOKEN` to a token from a different account or a GitHub App
+  installation and the eval measures the real review event.
   See [#245](https://github.com/fullsend-ai/agents/issues/245).
 
 - **fullsend `UploadFile` bug.** In fullsend v0.31.0, `UploadFile`

eval/review/cases/005-seeded-bugs/annotations.yaml

@@ -0,0 +1,85 @@
+# Expected fixture state after the review agent runs.
+#
+# The PR seeds three real, independent bugs across three files. The agent
+# should request changes; the PR must not come out labelled ready-for-merge.
+state: open
+
+labels:
+  required: []
+  forbidden:
+    - ready-for-merge
+    - rejected
+
+# Ground-truth findings the agent MUST surface. See eval.yaml's
+# required_findings judge: matched by (file, category, min_severity), where
+# category is a word-boundary phrase searched against the whole finding body
+# (category token + description + remediation), not against the kebab-case
+# category token alone.
+required_findings:
+  - file: src/orders/repository.py
+    category: "injection"
+    min_severity: high
+    description: |
+      get_order now builds its query with an f-string instead of the
+      parameterized form it replaced. order_id flows in unsanitized —
+      classic SQL injection. Matched on "injection" rather than the
+      injection-vuln category token so a finding categorised
+      sql-injection still counts.
+  - file: src/auth/session.py
+    category: "timing | constant-time | constant time | compare_digest | side-channel | side channel"
+    min_severity: medium
+    description: |
+      verify_session_token swapped hmac.compare_digest for a plain `==`
+      comparison. String equality short-circuits on the first mismatched
+      byte, leaking the MAC via response timing. The substantive claim is
+      what is matched, not the changed line — but a correct finding may
+      phrase it as "not constant-time", "side-channel", or by naming
+      compare_digest without ever saying "timing", so any one of those
+      alternatives satisfies the entry.
+  - file: src/orders/pricing.py
+    category: "apply_discount"
+    min_severity: medium
+    description: |
+      apply_discount dropped the `/ 100` when computing the discounted
+      total, so every call returns a value ~100x too large (a 10% discount
+      on $10.00 returns 9000 cents instead of 900). The file holds exactly
+      one function, so "a finding naming apply_discount" is a precise
+      assertion — the phrasing of the arithmetic itself ("off by 100",
+      "missing divisor") varies too much to pin deterministically. The
+      floor is medium: a review that catches the bug and rates it medium
+      has found it, and the review_quality rubric below grades severity
+      calibration rather than this gate.
+
+forbidden_findings: []
+
+max_turns: 50
+max_cost_usd: 6.00
+
+review_expectations: |
+  This PR seeds three real, independent bugs across three files. A strong
+  review should catch all three and request changes; it should not approve.
+
+  1. src/orders/repository.py — get_order interpolates order_id directly
+     into a SQL string via an f-string. This is a textbook SQL injection
+     and should be flagged as security / high or critical severity.
+
+  2. src/auth/session.py — verify_session_token replaced the constant-time
+     hmac.compare_digest call with `==`. This reintroduces a timing side
+     channel on the session MAC. Should be flagged as security, at least
+     medium severity. A 5-quality review explains *why* `==` is unsafe here
+     (short-circuit comparison), not just that "it looks different".
+
+  3. src/orders/pricing.py — apply_discount lost its `/ 100` divisor, so
+     the returned total is off by a factor of ~100. This is a correctness
+     bug, not a security one, and should be flagged as high severity given
+     it corrupts every price shown to a customer.
+
+  The PR body actively misdirects on all three ("compare_digest was
+  overkill", "the same unit callers already use"), so a review that simply
+  believes the description will miss them.
+
+  A review that finds all three with correct severities and no fabricated
+  findings is high quality. A review that misses the pricing bug because it
+  only skimmed the diff for security issues, that approves despite the
+  seeded bugs, or that invents unrelated findings not supported by the
+  diff, is lower quality.

eval/review/cases/005-seeded-bugs/input.yaml

@@ -0,0 +1,75 @@
+forge: github
+fixture:
+  type: pull_request
+  title: "Add customer search to order lookup, tune session checks and pricing"
+  body: |
+    ## Summary
+
+    - `get_order` now builds its query dynamically so we can extend it to
+      customer-name search in a follow-up PR.
+    - Simplified the session MAC check — `hmac.compare_digest` was
+      overkill for an internal endpoint.
+    - Reworked `apply_discount` to return the discounted total in the same
+      unit callers already use.
+
+    ## Test plan
+
+    - Manually hit `/orders/{id}` and confirmed a known order still comes
+      back with the right fields.
+    - Logged in and confirmed the session still validates.
+  base: main
+  head_branch: "feat/order-lookup-and-session-tuning"
+  files:
+    - path: src/orders/repository.py
+      content: |
+        """Order repository backed by SQLite."""
+
+        import sqlite3
+
+
+        def get_order(conn: sqlite3.Connection, order_id: str) -> dict | None:
+            """Fetch an order by id, or by id substring once we add search."""
+            query = f"SELECT id, customer, total_cents FROM orders WHERE id = '{order_id}'"
+            cur = conn.execute(query)
+            row = cur.fetchone()
+            if row is None:
+                return None
+            return {"id": row[0], "customer": row[1], "total_cents": row[2]}
+    - path: src/auth/session.py
+      content: |
+        """Session token helpers."""
+
+        import hashlib
+        import hmac
+        import secrets
+
+        SESSION_SECRET = secrets.token_bytes(32)  # fixture only — not a real deployment secret
+
+
+        def generate_session_token(user_id: str) -> str:
+            """Generate a signed session token for a user."""
+            mac = hmac.new(SESSION_SECRET, user_id.encode(), hashlib.sha256).hexdigest()
+            return f"{user_id}:{mac}"
+
+
+        def verify_session_token(token: str) -> bool:
+            """Verify a session token."""
+            try:
+                user_id, mac = token.split(":", 1)
+            except ValueError:
+                return False
+            expected = hmac.new(SESSION_SECRET, user_id.encode(), hashlib.sha256).hexdigest()
+            return mac == expected
+    - path: src/orders/pricing.py
+      content: |
+        """Order pricing calculations."""
+
+
+        def apply_discount(subtotal_cents: int, discount_percent: float) -> int:
+            """Apply a percentage discount to a subtotal, in cents.
+
+            discount_percent is a whole number percentage (e.g. 10 for 10%).
+            """
+            if not 0 <= discount_percent <= 100:
+                raise ValueError("discount_percent must be between 0 and 100")
+            return round(subtotal_cents * (100 - discount_percent))

eval/review/cases/005-seeded-bugs/repo

@@ -0,0 +1 @@
+../../repos/webapp

eval/review/cases/006-fp-bait/annotations.yaml

@@ -0,0 +1,65 @@
+# Expected fixture state after the review agent runs.
+#
+# The PR is genuinely correct but carries two patterns that naive
+# pattern-matching flags as vulnerabilities. The agent should approve.
+state: open
+
+labels:
+  required: []
+  forbidden:
+    - rejected
+
+# This PR is clean — no true findings required.
+required_findings: []
+
+# FP-bait patterns that look wrong at a glance but are correct in context.
+# min_severity defaults to "high" (see eval.yaml's forbidden_findings judge):
+# the agent MAY note either pattern at low/info — saying it looked and
+# dismissed the pattern is good reviewing — but promoting it to a real
+# vulnerability is the false positive this case measures.
+forbidden_findings:
+  - file: src/orders/receipts.py
+    category: "md5"
+    description: |
+      MD5 used in receipt_cache_key. Flagging "weak/insecure hash" here at
+      high/critical is a false positive — it's a non-cryptographic cache
+      key, not a security boundary, and the docstring says so. Anchored on
+      the primitive name, as 009 is, because any promotion has to name it:
+      "hashlib.md5 is cryptographically broken" contains no standalone
+      "hash". Word-boundary matching keeps "md5sum" from tripping it.
+  - file: src/orders/receipts.py
+    category: "subprocess"
+    description: |
+      subprocess.run in render_receipt_pdf. Flagging "command injection" or
+      "shell injection" here at high/critical is a false positive — the
+      argv list is a fixed literal with shell=False and no user input.
+      Anchored on the call name rather than "command": an unsafe-subprocess
+      finding describing "arbitrary code execution" is the same false
+      positive and never says "command".
+
+max_turns: 50
+max_cost_usd: 6.00
+
+review_expectations: |
+  This is a small, correct PR that intentionally contains two patterns
+  naive pattern-matching would flag as security issues, but which are safe
+  in context:
+
+  1. hashlib.md5 in receipt_cache_key — MD5 is "weak" for anything
+     security-sensitive (passwords, signatures, tokens), but here it is a
+     cache key over non-secret data with no security implication. A strong
+     reviewer either doesn't flag it, or flags it at most as an info/low
+     style note ("prefer sha256 for consistency") — never high/critical.
+
+  2. subprocess.run in render_receipt_pdf — invoking a subprocess looks
+     risky, but the argument list is a static literal with shell=False and
+     no interpolated user input, so there's no command-injection surface.
+     Flagging this as a real vulnerability is a false positive.
+
+  Both docstrings state the reasoning explicitly, so the context needed to
+  dismiss them is in the diff itself — a reviewer does not have to infer it.
+
+  A precise review approves this PR (or comments with only minor,
+  low-severity style notes) without raising either pattern as a real
+  security finding. A review that flags either one at high/critical
+  severity is a false positive and is lower quality.

eval/review/cases/006-fp-bait/input.yaml

@@ -0,0 +1,55 @@
+forge: github
+fixture:
+  type: pull_request
+  title: "Add receipt cache key and PDF rendering helper"
+  body: |
+    ## Summary
+
+    Adds two small helpers used by the (upcoming) receipts endpoint:
+
+    - `receipt_cache_key` — a short, stable key for caching rendered
+      receipts. Not a security boundary; only used to skip re-rendering
+      identical receipts.
+    - `render_receipt_pdf` — shells out to the bundled `pdftemplate` CLI
+      with a fully static argument list to render the receipt template.
+
+    ## Test plan
+
+    - Unit tested `receipt_cache_key` for stability across calls.
+    - Ran `render_receipt_pdf` locally against the `receipt` template.
+  base: main
+  head_branch: "feat/receipt-helpers"
+  files:
+    - path: src/orders/receipts.py
+      content: |
+        """Receipt generation helpers."""
+
+        import hashlib
+        import subprocess
+
+
+        def receipt_cache_key(order_id: str, total_cents: int) -> str:
+            """Build a short cache key for a rendered receipt.
+
+            MD5 is used here purely as a fast, non-cryptographic cache key —
+            receipts are not secrets and a collision only causes a harmless
+            cache miss, so a weak hash is an acceptable, deliberate choice.
+            """
+            digest = hashlib.md5(f"{order_id}:{total_cents}".encode()).hexdigest()
+            return f"receipt:{digest[:12]}"
+
+
+        def render_receipt_pdf() -> bytes:
+            """Render the bundled static receipt template to PDF bytes.
+
+            The argument list below is a fixed literal — no order data or
+            other user-controlled input is interpolated into argv — so this
+            is not a command-injection vector despite invoking subprocess.
+            """
+            result = subprocess.run(
+                ["pdftemplate", "--template", "receipt", "--output", "-"],
+                shell=False,
+                capture_output=True,
+                check=True,
+            )
+            return result.stdout

eval/review/cases/006-fp-bait/repo

@@ -0,0 +1 @@
+../../repos/webapp

eval/review/cases/007-docs-only/annotations.yaml

@@ -0,0 +1,50 @@
+# Expected fixture state after the review agent runs.
+#
+# Docs-only clarification of behaviour the code already has. The agent
+# should approve quickly with nothing above info severity.
+state: open
+
+labels:
+  required: []
+  forbidden:
+    - rejected
+
+required_findings: []
+
+# An empty category forbids ANY finding on this file at or above the floor
+# (see eval.yaml's forbidden_findings judge) — which is how a case asserts
+# "invent nothing here" without having to enumerate what might be invented.
+# The floor is lowered from the default "high" to "low", so even a
+# low-severity fabrication fails: there is nothing on this file to find.
+forbidden_findings:
+  - file: docs/api.md
+    category: ""
+    min_severity: low
+    description: |
+      Any finding on this file at low severity or above is a false
+      positive — the change only clarifies existing, accurate behaviour.
+      An info-severity note is still allowed.
+
+max_turns: 50
+max_cost_usd: 6.00
+
+review_expectations: |
+  This is a trivial, docs-only PR that only makes docs/api.md more precise
+  (it documents the existing not-found behaviour and the existing rounding
+  behaviour — it does not change or contradict any code). A strong review
+  approves this PR quickly with zero findings above info severity, and does
+  not invent correctness, security, or scope concerns about a file that
+  contains no executable code.
+
+  Both clarifications are checkable against the fixture and both hold:
+  repository.get_order returns None for an unknown id, and
+  pricing.apply_discount already calls round(). The fixture has no HTTP
+  layer, so the docs deliberately describe the lookup rather than a status
+  code — "the docs now disagree with the code" is not an available genuine
+  finding, and a reviewer that follows the prose to the code it describes
+  (which case 009 rewards) finds it accurate.
+
+  A review that approves with no fabricated findings is high quality. A
+  review that flags this docs clarification as risky, out of scope, or
+  incomplete (with no supporting evidence in the diff) is fabricating
+  findings and is lower quality.