review: security-auth docstring-contract rule, workflow-security hunts, and their corpus cases - #295
review: security-auth docstring-contract rule, workflow-security hunts, and their corpus cases#295ioanacrant wants to merge 10 commits into
Conversation
…seam A consuming repo may now define .github/aw/review/lenses/<lens>.md for any of the eleven specialist lenses, plus lenses/correctness.md for the always-on correctness-reviewer. Each file is runtime-imported (optional form) into a new 'Repo-specific rules and hunts' section of the matching reviewer prompt, carrying that repo's surface-specific rules and extra tri-state hunts. Lens names stay generic and shared; only payloads vary per repo. Behavior-neutral for every current consumer: no repo carries a payload file yet, and gh-aw's runtime resolves a missing optional import to nothing. correctness-checks.md stays imported as a deprecated alias for lenses/correctness.md (frontend carries one today); a repo should migrate the file and carry at most one of the two. Also in this change: - Eval alignment: resolveRuntimeImports now matches production for the optional form (missing resolves to empty, not the '(not configured for this eval case)' note), so corpus case trees can carry payloads. The required-form fallback note is unchanged and remains the one documented deviation (production fails the run). - disciplines.test.ts pins the payload import in every specialist lens section so the seam cannot be silently dropped. - README documents the lenses/ surface and the three-way contribution rule (shared skeleton vs lens payload vs skills), and fixes two stale claims in the consumer-config section: correctness-checks.md was never documented, and the optional import form was said to have been dropped. First planned payload use: migrating the #271 unbounded-reads hunt from the shared skeleton to server repos' correctness/caching payloads, and frontend's client-surface lenses/security-auth.md.
…, inert-payload warnings, precedence) Addresses the four non-blocking reviewer findings on #281: - Pin the correctness payload imports: disciplines.test.ts now asserts the correctness-reviewer section carries both the lenses/correctness.md import and its deprecated correctness-checks.md alias, so neither can be dropped silently (the specialist-lens assertion covered only the eleven lenses). - Warn on silently-inert payloads: new lib/lens-payloads.ts computes fixed-format warnings for a lenses/ entry matching no imported payload (typo or unknown name), a specialist payload no ROUTING rule routes, and the correctness alias carried alongside its replacement. runCli readdirs the payload dir and appends these to routingConfig.warnings, the existing channel the orchestrator surfaces in the review body's note lines. Split into its own module to respect the max-lines cap on router.ts; the specialist roster is a parameter to avoid an import cycle. - State payload precedence in the prompts: the eleven lens payload sections and the correctness framing now say payload rules are additive and never relax or override the shared rules, which win on conflict. README states the same, plus the alias's removal point (next major release). - FsLike gains readdirSync; the router test fake answers it (and directory existsSync) from its input map.
…ound-two feedback) Addresses the second-round reviewer findings on #281: - ENOTDIR crash: existsSync is true for a regular file at .github/aw/review/lenses, and readdirSync would then throw before routing.json is written, failing every review run in that repo. The CLI now degrades that misconfiguration to a routing warning; the test fake's readdirSync mirrors node:fs and throws on a file path to prove it. - Alias-alone deprecation nudge: an unmigrated repo (frontend today) would go silently inert when the correctness-checks.md import is dropped at the next major. lensPayloadWarnings now emits a rename nudge whenever the alias is carried without lenses/correctness.md, so the cliff is signposted on every review until the file moves. - runCli wiring for the alias check is pinned by a fakeFs case carrying both correctness files; the both-files warning cannot silently drop out. - README: the alias row no longer implies the alias is ignored when both files exist (both import; the router warns), and the warning list names the nudge and the dir-as-file degrade. - Import order nit in router.test.ts fixed. The additive/never-override contract stays prose-level by design: payloads are consumer-owned trusted config in the same class as risk-classification.md (restored from the base branch, authored under normal code review), and a phrase blocklist or size cap would misfire on legitimate rule text. Answered on the thread.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: be03052 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Review GuidanceOther risky files (1 file)
Common patterns11 files: Two new incident corpus cases — each a 2 files: New changeset entries bumping the Excluded from review (11 files)Not individually reviewed — generated, formatting-only, or fully explained by a common pattern above:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| - **`injection-sink`** — trace user-controlled input to a SQL/HTML/path/URL/shell/ | ||
| deserialization sink without validation or parameterization. `found` on an unguarded | ||
| sink. | ||
| - **`pwn-request`** — when `.github/workflows/*.yml` changes: a workflow |
There was a problem hiding this comment.
suggestion (non-blocking): All four new hunt gates read .github/workflows/*.yml, but GitHub also executes .yaml workflow files, so a lens following the gate literally would mark the hunts not-applicable for a .yaml-only change and miss the incident class they target. The same gap applies to composite actions under .github/actions/**/action.yml, where the pwn-request fixture itself stages the untrusted code the job runs. Widen the gate in all four bullets.
| - **`pwn-request`** — when `.github/workflows/*.yml` changes: a workflow | |
| - **`pwn-request`** — when `.github/workflows/*.{yml,yaml}` changes: a workflow |
Low-confidence notes (3)
workflows/review/review.md:2717— over-scoped-secret hunt ships with no eval corpus case; consider adding one. Note the pwn-request fixture is not a clean example — itscontents/pull-requests/issues: writescopes are each used by a step (git push, comment posting).workflows/review/review.md:2722— unpinned-action overlaps deterministic linters (actionlint/zizmor/Dependabot); worth deciding whether a model hunt is the right layer, especially with no corpus case measuring its precision/recall.workflows/review/review.md:2715— the push-ref-race discriminator ("a ref name rather than the head commit SHA") may read as firing on most checkout-branch-then-push auto-commit workflows; consider stating the compliant compare-and-swap pattern (--force-with-leasepinned to the checked-out SHA).
| { | ||
| "id": "incident-workflow-pwn-request", | ||
| "tags": [ | ||
| "live" |
There was a problem hiding this comment.
nitpick (non-blocking): This incident-repro case tags only ["live"], while its sibling added in this same PR carries ["incident", "live"] and every other case under corpus/incidents/ carries "incident". No harness code filters on the tag today, so nothing breaks, but the inconsistency would silently drop this case from any future incident-tag selection.
| "live" | |
| "incident", | |
| "live" |
| deserialization sink without validation or parameterization. `found` on an unguarded | ||
| sink. | ||
| - **`pwn-request`** — when `.github/workflows/*.yml` changes: a workflow | ||
| combining a `pull_request`-family trigger, write permissions |
There was a problem hiding this comment.
thought (non-blocking): security-auth is a path-gated specialist lens, so these hunts run only when a consumer's ROUTING maps .github/workflows/** to security-auth — the pwn-request corpus case has to inject routerConfig.lensRules for the hunt to fire at all. Unless a default routing rule ships or the required ROUTING entry is documented, the hunts stay dormant in production even though the eval reports them caught.
| (`contents:write` / `pull-requests:write`), checkout of the PR head, and any | ||
| step executing untrusted code (install/build/lint that runs or mutates files). | ||
| `found` on the full combination — treat as blocking. | ||
| - **`push-ref-race`** — when `.github/workflows/*.yml` changes: a workflow |
There was a problem hiding this comment.
suggestion (non-blocking): push-ref-race ships with no eval corpus case, so it is untested detection logic — in this repo the corpus cases are the regression tests for prompt behavior. The pwn-request fixture's bare git push embodies the pattern, but its case asserts only the pwn-request finding. Consider adding a dedicated case so a future edit that stops the reviewer emitting push-ref-race fails a gate rather than regressing silently. (Same applies to over-scoped-secret — see the note on the pwn-request hunt above.)
Review live A/BBaseline: Ruler: matcher deterministic+arbiter; corpus cc11988c0918 (9 cases).
Adversarial hard gate: PASSED on the candidate arm. Single-run-stable rows: recall, verdict agreement, regressions, adversarial gate. Judge quality and noise are not: they jitter run-to-run at this corpus size, and a regressed reviewer can score HIGHER on judge quality (fewer, surer comments each read better). Recall against the labeled specs is the load-bearing metric. Measured noise floor (identical arms, run 29069228968, 2026-07-10, 6 arm-samples, full corpus x3, pre-arbiter; budget skips left the samples on unequal case sets, so these v1 bands also carry case-mix variance): must-catch recall 54%-86% (sd 10%), verdict agreement 75%-100% (sd 9%), noise (unmatched posted) 50%-60% (sd 3%), judge mean quality 82%-86% (sd 2%). A single-run delta whose arms both sit inside a band is indistinguishable from run-to-run wobble; use |
ioanacrant
changed the title
Issue: SEC-624 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 participants
Summary:
Stack-generic additions to the shared
security-authlens, plus the eval corpuscases that exercise them — one PR so the powered eval can run straight off this
ref. Stacked on #281; the consumer-surface counterparts live in that seam's
payload.
Prompt changes
security or privacy property ("without answers", "without PII") is a contract
clients trust; a resolver that contradicts it means the schema lies. The rule
fires when a symbol's documented guarantee disagrees with what the changed
code actually assigns to it.
pwn-request,push-ref-race,over-scoped-secret,unpinned-action, gated on.github/workflows/*.ymlchanges. Placed in
security-authrather thandeploy-infra-config, whichowns rollout safety (flag defaults, plaintext config secrets, destructive IaC)
and nothing in workflow-execution semantics — happy to move if you disagree.
for
over-scoped-secret(the validator's failure-scenario bar).Corpus cases
Both cases are synthetic. They carry no real service code, paths, identifiers, or
author names — this repo is public, and every other case in the corpus is
synthetic for the same reason. Both are sized in line with existing cases (146
and 137 lines of tree across four files each).
incidents/incident-docstring-contract-parity— a GraphQL schema documentsSimpleQuizItem.contentas "the rendered content of the item without answers",and the changed resolver assigns it the output of a fetcher whose own docstring
says it returns the item JSON "complete with answers". The PR description in the
fixture defers stripping to a future frontend change, which would not stop the
answers leaving the server. One
mustCatchSpecanchored on a diff-added line,with
altLocationson the in-diff fetcher and the deleted answer-stripped path.The unchanged schema file cannot carry a spec (the loader requires spec paths to
appear in
changedFiles), so it is staged as tree context alongside the contentclient — together they are the discoverability chain from the documented promise
to the runtime behavior.
incidents/incident-workflow-pwn-request— a new workflow triggered onpull_request_target, which runs in the base repository's context with fullsecret access and a write-capable token for every PR including fork-authored
ones. It checks out the attacker-controlled PR head (
head.sha) with a bottoken, then executes code from it (
pnpm installlifecycle scripts via thesetup action, and a fixer script from the PR branch) before pushing back. Any
GitHub user can fork, open a PR, and obtain arbitrary code execution on a
trusted runner: every secret in scope is exfiltratable and the bot token can
push to protected branches. The fixture's comment rationalizes the trigger
choice as a workaround for fork runs getting no secrets, so the case also tests
whether the lens reasons about the trigger rather than accepting the author's
stated justification.
Single-spec by design. An earlier revision of this case used the plain
pull_requesttrigger, where GitHub withholds secrets from fork runs — theattacker needs push access first, which makes the pattern a defensible tradeoff
and "must block" contested ground truth. It now uses
pull_request_targetsothe expected verdict is unambiguous. There is deliberately no unpinned-action
spec: the
unpinned-actionhunt ships without a dedicated corpus case for nowrather than with fabricated ground truth.
One changeset (minor), covering the lens changes and the corpus together.
Test plan:
both cases and the whole-corpus must-catch recall check replaying them to
REQUEST_CHANGES), plus
tsc --noEmitand eslint clean.staged tree, and every anchor and evidence-trace line resolves to the content it
claims.
prompt-injection hard gate must stay green). Single-run deltas are noise per the
eval README.
(
--force-arms, 3 repeats) before any band claim, then a powered repeated evalpricing the prompt changes against them.
Known CI failure
The
PR Reviewercheck fails on this PR by hitting the agent step'stimeout-minutes: 20ceiling — the orchestrator fans out ten subagents and doesnot finish inside the cap. It is not caused by these changes: the same step failed
identically on the preceding commit, and this PR's latest push reduced the diff
by ~3,600 lines without changing the runtime. Filed separately from this review.
Open questions:
pwn-requesthunt text says "apull_request-family trigger" and nevernames
pull_request_targetorworkflow_run. The corpus case now turnsentirely on
pull_request_target, andworkflow_runis the other classicprivileged-trigger vector (artifact download, then execute). Worth naming both
explicitly in the hunt?
ROUTINGenables no specialist lenses, so
pwn-requestcannot fire in the repo thatships actions into other repos' CI. Route
.github/workflows/**tosecurity-authhere as a follow-up?Issue: SEC-624