feat(security-review): automated per-PR security-review pipeline (EXP-440)#1940
Open
gvladika wants to merge 54 commits into
Open
feat(security-review): automated per-PR security-review pipeline (EXP-440)#1940gvladika wants to merge 54 commits into
gvladika wants to merge 54 commits into
Conversation
Distills audit report PDFs into a structured knowledge corpus under audit/knowledge/ (findings.json + lessons.md + by-area/*.md). Output feeds the LI.FI-customized PR-time security review agent (EXP-483). Single-PDF mode in v1; batch processing handled in EXP-479. Skip list excludes gas / naming / NatSpec / code-quality informational findings without a security path. Idempotent via composite source_id (<audit_id>::<auditor_label>). Validated against audit20250305 (Chainflip Facet) — 7 raw findings → 3 kept (1 high, 2 low). Parent: EXP-440 — LI.FI-customized security review agent. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…(EXP-478) CodeRabbit flagged a schema inconsistency: step 6 referenced status_history[].source_id, but the schema only defines source_id at the top level. Reconstruct the composite <audit_id>::<source_label> from each status_history entry instead — same intent, correct field references. This also clarifies that the check matches either the top-level source_id OR any historical appearance, which is the behaviour the corpus needs for re-audits. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
… (EXP-479)
Ran /extract-audit-knowledge across 81 of 83 audits in audit/auditLog.json
(2 PDFs unprocessable — corruption — see lessons.md "Coverage gaps"). The
parallel extraction was executed via 8 sub-agents, each handling a
load-balanced slice of the corpus.
Result:
- 145 findings (0 critical, 4 high, 18 medium, 67 low, 56 info-security)
- 42 audits contributed findings; 39 read but produced none (genuine 0-issue
reports or all findings filtered by the skip list)
- Distribution: facets 91, periphery 43, libraries 8, security 2,
cross-cutting 1, helpers 0
- 92 fixed, 49 acknowledged, 3 mitigated, 1 raised
Files:
- findings.json — canonical machine-readable source of truth, keyed by LF-NNN
- lessons.md — totals + coverage gaps + flat index of all findings
- by-area/{facets,periphery,libraries,security,cross-cutting}.md —
per-folder context loaded by the PR-time security review skill
- semgrep/lf-{001,002,003,008,029,046,053,054}-*.yml — 7 hand-authored
seed rules deriving from the clearest structural patterns. All pass
`semgrep --validate`. Precision is intentionally lower for some
(LF-029, LF-046, LF-008) — EXP-484's baseline measurement will tune
these.
Two PDFs in audit/reports/ are corrupted and cannot be processed without
re-obtaining from the auditor:
- audit20251208 (Patcher v1.0.1)
- audit20251229 (WhitelistManagerFacet v1.1.0)
Branched from feature/exp-478-build-extract-audit-knowledge-skill so the
(fixed) skill is available locally; when EXP-478 merges, this branch
rebases cleanly onto main with only the audit/knowledge/ delta.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…(EXP-479) Fan-out 5 parallel sub-agents to extract findings from the 39 audits the original EXP-479 batch run missed (out of 41 — 2 PDFs are permanently corrupted in repo state and excluded). Result: 2 new findings extracted (147 total, was 145): - LF-146 INFO LidoWrapper — wrap/unwrap entrypoints lack minAmountOut slippage parameter. Acknowledged by LI.FI. - LF-147 INFO LiFiIntentEscrowFacet — validateBridgeData modifier omits zero-check on destinationChainId. Acknowledged. The other 37 audits were either zero-finding reports or had only gas-opt/code-quality items the skip-list correctly drops. The real shape of the "41 missing" was: 39 of the missing audits had nothing keepable. The original EXP-479 batch run failed to RECORD this — findings.json had no concept of "processed but empty" — so the coverage check kept flagging them as drift. Schema bumped 1.0 → 1.1: - Added top-level `processed_audits: string[]` array recording every audit_id considered (with or without findings). - Coverage script updated to treat presence in processed_audits OR in any status_history as "covered". - /extract-audit-knowledge skill updated: new Step 8 appends to processed_audits idempotently; documented in the schema example; added to the validation checklist. Coverage check final state: 81/83 audits covered. Remaining 2 are the known-corrupted PDFs (audit20251208 Patcher v1.0.1, audit20251229 WhitelistManagerFacet v1.1.0) — re-fetch from auditor required, tracked as session debt. Per-batch results (all 5 sub-agents reported honestly): - batch 1 (8 audits, early-2025 hotfixes): 0 kept - batch 2 (8 audits, mid-early 2025): 0 kept - batch 3 (8 audits, mid-2025): 1 kept (LF-146) - batch 4 (8 audits, late 2025): 0 kept - batch 5 (7 audits, late 2025 + 2026): 1 kept (LF-147) Cost: ~$3.50 in Anthropic API tokens (5 parallel Sonnet sub-agents, ~60K total input each, low output). Wall time ~10 min in parallel. Files touched: M audit/knowledge/findings.json (+2 findings, +processed_audits) M audit/knowledge/lessons.md (totals + index updated) M audit/knowledge/by-area/periphery.md (+ LF-146) M audit/knowledge/by-area/facets.md (+ LF-147) M script/tasks/checkAuditKnowledgeCoverage.ts (honors processed_audits) M .agents/commands/extract-audit-knowledge.md (Step 8 + schema 1.1) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…XP-480)
Adds .github/workflows/security-review.yml that runs three SAST tools in
parallel on every non-draft PR touching src/** or audit/knowledge/**:
- Slither (crytic/slither-action@v0.4.2) → slither.sarif
- Aderyn (Cyfrin v0.6.8, installed at runtime) → aderyn.sarif
- Semgrep (pip-installed v1.117.0) with audit/knowledge/semgrep/*.yml
from the EXP-479 seed rules → semgrep.sarif
Each job uploads its SARIF to GitHub Code Scanning with a per-tool
category, so findings appear inline in the PR diff and aggregate in the
Security tab. Status check is published but NOT required (advisory only,
per EXP-480) — enforcement comes in EXP-485 after EXP-484 baseline tuning.
continue-on-error: true on all three so a single tool flake (pip rate
limit, slither dep failure, etc.) does not mask findings from the others.
Concurrency group dedupes in-flight runs per PR.
Per-job permissions: contents: read + security-events: write (scoped
narrowly per zizmor recommendation; workflow-level only has contents: read).
Validated locally:
- actionlint → 0 findings
- zizmor 1.23.1 → 0 findings
Stacks on top of EXP-478 (skill) and EXP-479 (corpus + seed rules).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PR-review feedback on #1885: Aderyn was included in the original Stage 1 design but the dry-run measurements in EXP-483 (#1887) showed it contributes ~1 finding per PR beyond Slither's coverage on the same code. That marginal value did not justify the supply-chain attack surface of Aderyn's installer step or the extra CI job. This commit removes the Aderyn job from .github/workflows/security- review.yml on the same PR where it was originally introduced, so PR #1885 ships the final Stage 1 design (Slither + Semgrep). The EXP-484 follow-up PR (#1888) previously did this removal — it becomes a no-op after this lands, which is the intended outcome when stacked PRs merge in order. If a second-opinion static-analysis pass is wanted later, it belongs in EXP-486 (nightly deep scan), not per-PR Stage 1. Validated: - actionlint → 0 findings - zizmor 1.23.1 → 0 findings - Workflow grep confirms no remaining Aderyn references
…P-483) Wires the AI triage layer for the security-review pipeline. On every non-draft PR touching src/** or audit/knowledge/**, the new lifi-pr-review job runs after Stage 1 (Slither/Aderyn/Semgrep) and: 1. Downloads the three SARIF artifacts produced by Stage 1. 2. Builds the PR diff context (git diff base...head + changed src/ files). 3. Invokes anthropics/claude-code-action@v1.0.99 with a prompt that runs the /lifi-pr-review skill from .claude/skills/lifi-pr-review/. 4. The skill applies Pashov's 4-gate FP filter to Stage 1 findings (Track A) AND reviews the PR diff for new risks (Track B), using audit/knowledge/ as cached context. Emits curated.sarif + summary.md. 5. Workflow uploads curated.sarif to Code Scanning (category "ai-triage") and posts/updates a sticky PR summary comment via actions/github-script. Other additions: - `.agents/commands/lifi-pr-review.md` — full skill spec (source of truth, symlinked from `.claude/skills/lifi-pr-review/SKILL.md` per repo DRY convention) - `audit/findings/waived.yml` — explicit FP-waiver schema (initially empty; commented example shows the required fields incl. expires date) - `docs/security-review.md` — operational manual: setup steps, how to read output, waiver workflow, override path for security emergencies, custom-rule authoring, cost expectations, troubleshooting Security: - ANTHROPIC_API_KEY accessed via dedicated `security-review` environment (zizmor recommendation, isolates from regular repo secrets and enables per-environment review gates on forks) - All action refs SHA-pinned per `[CONV:ACTIONS-IMMUTABLE]`: - actions/upload-artifact@v7.0.1, actions/download-artifact@v8.0.1 - actions/github-script@v9.0.0 - anthropics/claude-code-action@v1.0.99 - continue-on-error: true keeps the workflow advisory until EXP-485 flips the gate Validated locally: - actionlint → 0 findings - zizmor 1.23.1 → 0 findings - markdownlint → 0 errors on new docs Stacks on top of EXP-482 baseline. Three follow-up items already noted in the docs/security-review.md and the skill: - EXP-484 tunes the skip list + Semgrep rule precision once first CI runs surface real data - EXP-485 flips advisory → enforcing - Repo admin must create the `security-review` environment + add the Anthropic secret before first run (see docs/security-review.md § Setup) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
The Build PR diff context step was emitting the full PR diff (including
test/, script/, docs/, etc.) into ${DIFF_FILE} while ${PR_FILES} was
already scoped to src/. On test-heavy PRs this trips the skill's
5000-line guardrail and causes Stage 2 to self-skip even though the
src/ surface is small.
Surfaced by the EXP-483 dry-run on PR #1550 (AcrossV4SwapFacet): full
diff was 7820 lines while the src/-only diff was 1102 lines, well under
the guardrail.
Fix scopes the git diff invocation to `-- src/` so DIFF_FILE matches
PR_FILES. Also updates the skill spec table to reflect the new shape.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…(EXP-483)
The original EXP-440 design intent was for /lifi-pr-review to be a thin
LI.FI integration layer over open-source security skills (Trail of Bits
+ Pashov), with the LF-NNN audit corpus as the LI.FI-specific value-add.
That intent was lost during context compaction mid-session — the resulting
skill re-implemented Track A (4-gate FP filter) and Track B (diff review)
from scratch, duplicating logic that ToB already ships at
github.com/trailofbits/skills as Claude Code plugins.
This commit corrects that, reusing the ToB skills as designed:
ToB skills now vendored as a pinned-SHA git submodule:
.claude/vendor/tob-skills/ (CC-BY-SA-4.0; see NOTICE)
pinned to: a56045e9 (2026-05-05)
Exposed to Claude Code via symlinks (controlled exposure of 5 plugins):
.claude/plugins/audit-context-building
.claude/plugins/differential-review
.claude/plugins/fp-check
.claude/plugins/variant-analysis
.claude/plugins/semgrep-rule-creator
Per-PR workflow (new shape):
Step 0 load LF-NNN corpus + waivers + skip-list (LI.FI)
Step 1 cost guardrail (LI.FI)
Step 2 invoke audit-context-building with corpus as baseline (ToB)
Step 3 invoke differential-review on the diff (ToB; replaces our Track B)
Step 4 invoke fp-check on each Stage 1 SARIF survivor + diff finding
(ToB; replaces our 4-gate Track A — 6 gates vs 4, with
Standard/Deep routing and bug-class-specific verification)
Step 5 invoke variant-analysis on each fp-check TRUE POSITIVE to hunt
siblings elsewhere in src/ (ToB; new capability)
Step 6 normalize all findings → curated.sarif + summary.md (LI.FI)
LI.FI value-add stays in our code (MIT/LGPL):
audit/knowledge/ — LF-NNN corpus (input to audit-context-building)
audit/knowledge/semgrep/— LI.FI custom rules
audit/findings/skip-list.yml — rule-level FP suppression (extracted from
prose in baseline doc to a maintained
artifact; 11 entries, 5 slither + 6 aderyn)
audit/findings/waived.yml — finding-level FP suppression
.agents/commands/lifi-pr-review.md — orchestrator (this refactor)
.github/workflows/security-review.yml — CI plumbing + SARIF→Code Scanning
Other changes:
- NOTICE at repo root with CC-BY-SA-4.0 attribution per license terms
- .gitattributes marks .claude/vendor/** as linguist-vendored
- workflow's Stage 2 checkout now uses submodules: recursive
- workflow's claude-code-action prompt updated to direct the skill to
invoke the vendored plugins (rather than reimplement)
- docs/security-review.md: new architecture diagram showing LI.FI layer
over ToB layer; added "Bumping the ToB skills" procedure; updated
cost estimate (higher due to per-finding fp-check invocations,
offset by ~70% prompt caching)
What was removed:
- The previous skill file's Track A 4-gate inline rubric (superseded by
fp-check's 6 gates)
- The previous skill file's Track B 3-question inline rubric (superseded
by differential-review's 7-phase workflow)
- The previous skill file's inline skip-list prose (extracted to
audit/findings/skip-list.yml as a maintained artifact)
Validated locally:
- actionlint → 0 findings
- zizmor 1.23.1 → 0 findings
- markdownlint → 0 errors on docs/security-review.md + skill file
- All 5 plugin symlinks resolve to valid CCP plugin.json manifests
- skip-list.yml parses with all required fields (11 entries)
End-to-end re-run against PR #1550 deferred — requires Claude Code
session with the new plugins loaded, which is what the live CI run
does. The previous dry-run (commit e94658b3 era) validated the LI.FI-
side behavior; this refactor changes only the delegation layer.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Follow-on to PR #1885's commit 041e524 which removed the Aderyn job from Stage 1. PR 5's branch was created when PR 3 still had Aderyn, so the branch state still contains: - The Aderyn job inherited from PR 3's original commit - Stage 2 `needs: [slither, aderyn, semgrep]` from cda4a89 - aderyn.sarif references in the claude-code-action prompt - aderyn references in the lifi-pr-review skill input table + summary This commit aligns PR 5's branch with the no-Aderyn end state shipped in PR 3 (#1885). When the stack merges in order, PR 6's c263f42 commit (which originally removed Aderyn) becomes a clean no-op — Aderyn is already gone. Files updated: - .github/workflows/security-review.yml: header comment, full Aderyn job block, needs: list, claude-code-action prompt - .agents/commands/lifi-pr-review.md: SARIF inputs table, summary template Validated: - actionlint → 0 findings - zizmor 1.23.1 → 0 findings - markdownlint → 0 errors on the skill file - grep -i aderyn → no remaining references in workflow or skill
…EXP-484) CodeRabbit (local) on PR #1887 flagged cross-layer inconsistency: major @ .github/workflows/security-review.yml:170-321 "The workflow defines only two Stage 1 jobs (slither, semgrep), but docs/security-review.md (lines 29, 32, 58) and audit/findings/skip-list.yml (lines 77-118) both reference Aderyn as a third Stage 1 tool." Fixed by aligning the two referenced documents with the workflow state: - docs/security-review.md • Architecture diagram now shows Slither + Semgrep only (no Aderyn box, no aderyn.sarif arrow). • Removed "Slither/Aderyn config tuning" prose; replaced with Slither- specific guidance. - audit/findings/skip-list.yml • Removed all 6 aderyn entries (todo, unused-import, state-variable-could-be-immutable, state-variable-could-be-constant, centralization-risk, push-zero-opcode). • Schema line tool: enum collapsed from "slither | aderyn | semgrep" to "slither | semgrep". • Aderyn section header kept as an explanatory comment block documenting WHY the tool was removed — answers the predictable reviewer question of "why isn't Aderyn here?" without re-introducing the entries. Validated: - bunx markdownlint-cli2 docs/security-review.md → 0 errors - skip-list.yml parses cleanly via js-yaml - grep confirms only comment-block mentions of Aderyn remain
PR 4 (EXP-482 / docs/security-review-baseline.md) was dropped from the EXP-440 stack — the snapshot was a one-shot research artifact, not living documentation. The methodology + tool-flag rationale that justified --exclude-informational --exclude-low lives inline in skip-list.yml and in docs/security-review.md § "Cost & runtime"; the dated finding counts have no long-term value once src/ changes. Removes: - .agents/commands/lifi-pr-review.md — 2 baseline-doc references in the "Files we maintain" and "Reference" sections. - audit/findings/skip-list.yml — "Sourced from … EXP-482 noise baseline" header line; the tuning anchor (EXP-484) is preserved. - docs/security-review.md — "EXP-482" row in the Related section and the in-prose mention under "Bumping the Trail of Bits skills"; replaced with a tool-agnostic "re-measure Stage 1 noise on recent PRs" phrasing.
…tuning rationale (EXP-484) EXP-484 follow-up surfaced by the EXP-483 dry-run on PRs #1715 + #1731: 1. Aderyn dropped from Stage 1 Marginal value: ~1 finding per PR beyond what Slither already catches. The supply-chain attack surface of the installer step doesn't pay for itself. If a second-opinion pass is wanted, it belongs in EXP-486 (nightly deep scan), not per-PR. 2. Slither tuning was already correct — workflow already had --exclude-informational --exclude-low (line 68 in v0.4.2 of crytic/slither-action invocation). The earlier dry-run report was based on running Slither locally WITHOUT those flags, which inflated the numbers. Real per-PR numbers with workflow flags: PR #1715 (6 src/ files, ~130 line feat): 4 Slither findings PR #1731 (1 src/ file, +5 line add): 1 Slither finding No workflow change needed for Slither tuning. 3. skip-list.yml shrunk from 11 entries to 0 All previously-listed Slither entries (naming-convention, too-many-digits, reentrancy-events, assembly, low-level-calls) are now dropped at the tool level by --exclude-informational --exclude-low. The skip-list mechanism is kept for future skill-level suppression of Medium/High Slither findings that turn out to be systematically FP for LI.FI patterns. Currently no entries needed. Open EXP-484 tuning items moved to comments in skip-list.yml: - missing-zero-check (Medium): often FP on LI.FI ctor params - divide-before-multiply (Medium): matches our (a*BASE)/b idiom - Semgrep lf-046 / lf-029 / lf-053: rules need pattern tightening (these are Semgrep rule precision issues, fixed in audit/knowledge/semgrep/ not here) Cost impact (per-PR Stage 2 cost estimate, updated): Small PR (<5 files, 1-5 findings) $0.30 - 0.50 Medium PR (6-15 files, 5-15 findings) $0.50 - 1.00 Large PR (16-30, 15-50 findings) $1.00 - 2.20 These are 40-60% below the previous estimate because the actual fp-check input set is much smaller than the unfiltered SARIF suggested. Validated locally: - actionlint → 0 findings - zizmor 1.23.1 → 0 findings - markdownlint → 0 errors - skip-list.yml parses (now 0 entries; all guidance in comments) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Three rules surfaced as systematically false-positive in the EXP-483 dry-runs on PR #1715, #1731, #1711. Fixed: lf-046-bytes32-nonevm-receiver Problem: `metavariable-regex` was a SIBLING of `pattern-either` at the top level of the rule, which semgrep silently ignored. Every `bytes32 ...` declaration in scoped paths was matching, including NAMESPACE constants, EIP-712 typehashes, request IDs, etc. Fix: wrap `pattern-either` and `metavariable-regex` in a `patterns:` block so AND-conjunction is correctly applied. Also tightened the regex to only match names containing "nonEvm/btc/bitcoin" + "receiver" — removed "destination" (matched callers/sentinels) and "solana" (Solana addresses safely fit in 32 bytes per protocol). lf-029-shared-modifier-parallel-fields Problem: matched ANY `$SD.receiver` read, including `_bridgeData. receiver` (the canonical parent) inside `keccak256(abi.encode(...))` for EIP-712 signature binding (good practice), inside if/else branches (`if (_bridgeData.receiver == NON_EVM_ADDRESS)`), and inside the validation guard itself. Fix: pattern-not-inside excludes keccak256/abi.encode/abi.encodePacked blocks, equality/inequality comparisons (== / !=), and the LI.FI canonical guard `LibUtil.validateReceiver(...)`. metavariable-regex on $SD excludes `_bridgeData` so the rule only flags bridge-SPECIFIC structs (the actual LF-029 class). lf-053-loop-msg-value Problem: fired on every `{value: msg.value}(...)` invocation, including single-swap entry points where there's no loop and msg.value can't be spent multiple times. Fix: add `pattern-inside: for/while/do-while` constraint. Only fires when the call is inside an iterating construct. Validation (re-run on the 3 dry-run PRs): PR before tune after tune delta ───────── ───────────── ──────────── ────── #1715 1 0 −100% #1731 9 2 −78% #1711 9 1 −89% TOTAL 19 3 −84% The 3 remaining findings are legitimate concerns routed to fp-check: - PR #1731 L43: bytes32 nonEVMReceiver (TP class) - PR #1731 L114: IERC20(USDC).approve (likely FP per fp-check; owner- gated one-time init) - PR #1711 L64: bytes32 nonEVMReceiver (TP class) Documented the tuning rationale inline in each rule's `metadata.# EXP-484 tuning notes:` comment so future maintainers see why the structure looks the way it does. Updated audit/findings/skip-list.yml to remove the "open EXP-484 candidate" notes that are now addressed. Validated locally: - `semgrep --validate --config audit/knowledge/semgrep` → 0 errors, 8 rules loaded - Isolated test fixtures confirm intended matches/non-matches Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…tector) LI.FI's lf-053-loop-msg-value rule is a straight duplicate of Slither's upstream-maintained `msg-value-loop` detector (Medium impact, ships since Slither 0.10.x). Both Stage 1 (Slither) and Stage 1 (Semgrep with this rule) were firing on the same pattern. Removing the rule: - No coverage loss — Slither's detector already catches the same pattern and runs in the same Stage 1 step. - Reduces our maintenance surface (one fewer rule pattern to keep precise as Solidity / facet patterns evolve). - Validates the audit corpus principle: ship LI.FI Semgrep rules ONLY for patterns Slither structurally cannot catch. The LF-053 LF-NNN entry stays in audit/knowledge/findings.json — that is historical knowledge of the finding shape. Only the Semgrep rule file is removed. Surfaced by PR review of #1884 (audit knowledge corpus seed): reviewer questioned why we ship a Semgrep rule for a pattern Slither already flags. The other 7 LI.FI Semgrep rules were verified to NOT have a precise Slither equivalent (lf-002 partially overlaps with unchecked-transfer but has a different framing; lf-008 partially overlaps with missing-events-* but is broader; the rest are LI.FI-specific). Followup: audit lf-002 and lf-008 against unchecked-transfer and missing-events-* more rigorously to confirm they cover patterns Slither cannot. Tracked as EXP-484 work.
Keep audit/knowledge/findings.json in lock-step with
audit/auditLog.json so the EXP-483 security-review pipeline doesn't
silently lose access to prior-audit findings when a new audit lands.
Two layers — local automation + CI safety net:
1. /add-audit Step 9 (.agents/commands/add-audit.md)
After saving the PDF and updating auditLog.json, /add-audit now
chains into /extract-audit-knowledge <audit_id> automatically.
Skippable via /add-audit --skip-extract for two-step workflows.
Failure handling preserves the auditLog entry and surfaces the
error for retry.
2. Coverage check (script/tasks/checkAuditKnowledgeCoverage.ts)
TypeScript per rule 000 (no Python). Two modes:
--new-audits <id1,id2> (CI use — delta-only)
# absolute (no args) (local use — informational)
Delta-only is deliberate: pre-existing drift (41 audits as of
2026-05-22; from EXP-479 batch run that didn't complete coverage)
is backfill work and not blocked here. Every passing PR would
otherwise be blocked until full backfill — too much friction.
3. CI workflow (.github/workflows/audit-knowledge-coverage.yml)
Triggers on PRs touching audit/auditLog.json, audit/reports/**,
or audit/knowledge/findings.json. Computes the PR-added audit IDs
via `git diff base..head` against auditLog.json (jq, not text),
then runs the coverage check in --new-audits mode. Blocks merge
if any newly-added audit lacks corpus entries.
Linting clean:
- actionlint → 0 findings
- zizmor 1.23.1 → 0 findings (template-injection avoided by
routing NEW_AUDITS through env)
Docs (docs/security-review.md § "Audit-knowledge coverage") explain
the two layers, the delta-only design rationale, and the --skip-
extract escape hatch.
Real-world validation: running the check against the live repo state
reveals that the EXP-479 batch extraction only landed corpus entries
for 42 of 83 audits. This is real drift, surfaced as informational
(absolute-mode exit 1, advisory). The CI guard only fails NEW
introductions, so existing repo state continues to pass.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Reviewer feedback: the CI workflow added in f34cda55 was over-engineered. The audit/knowledge/ corpus is advisory context for the EXP-483 security agent — a missing audit degrades recall, it doesn't break correctness. Gating PRs on it is too strong; the /add-audit Step 9 chain already handles the canonical sync path. Changes: - Removed .github/workflows/audit-knowledge-coverage.yml entirely. - Reframed script/tasks/checkAuditKnowledgeCoverage.ts as a local diagnostic (header comment + --new-audits docstring updated). - Updated docs/security-review.md "Audit-knowledge coverage" section to describe only Layer 1 (the /add-audit auto-chain) and the manual diagnostic, removed the "two layers" framing. - Updated .agents/commands/add-audit.md to drop CI-gate references in the --skip-extract reminder and the validation checklist. The TS script stays — useful for triaging the EXP-479 backfill queue (42/83 audits currently in the corpus) and for sanity-checking after a manual /extract-audit-knowledge run. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…EXP-481) Picks up the script changes from 9fcc48d6 that were dropped when PR 2 (EXP-479 backfill) cherry-picked the schema 1.1 bump but couldn't modify a file (script/tasks/checkAuditKnowledgeCoverage.ts) that doesn't exist yet at that point in the stack. This commit: - Adds processed_audits to the IFindingsCorpus type so the check treats schema 1.1+ corpora correctly. - Unions processed_audits into the "covered" set alongside status_history-derived audit IDs. - Refactors type aliases to interfaces with I-prefix per [200-typescript.md] naming convention (was pre-existing technical debt in f34cda55). Validated: - bunx tsc-files --noEmit → 0 errors - bunx eslint → 0 findings - bun script/tasks/checkAuditKnowledgeCoverage.ts → runs cleanly
…ck 6/7] (#1888) * refactor(security-review): drop Aderyn from Stage 1; tighten Slither tuning rationale (EXP-484) EXP-484 follow-up surfaced by the EXP-483 dry-run on PRs #1715 + #1731: 1. Aderyn dropped from Stage 1 Marginal value: ~1 finding per PR beyond what Slither already catches. The supply-chain attack surface of the installer step doesn't pay for itself. If a second-opinion pass is wanted, it belongs in EXP-486 (nightly deep scan), not per-PR. 2. Slither tuning was already correct — workflow already had --exclude-informational --exclude-low (line 68 in v0.4.2 of crytic/slither-action invocation). The earlier dry-run report was based on running Slither locally WITHOUT those flags, which inflated the numbers. Real per-PR numbers with workflow flags: PR #1715 (6 src/ files, ~130 line feat): 4 Slither findings PR #1731 (1 src/ file, +5 line add): 1 Slither finding No workflow change needed for Slither tuning. 3. skip-list.yml shrunk from 11 entries to 0 All previously-listed Slither entries (naming-convention, too-many-digits, reentrancy-events, assembly, low-level-calls) are now dropped at the tool level by --exclude-informational --exclude-low. The skip-list mechanism is kept for future skill-level suppression of Medium/High Slither findings that turn out to be systematically FP for LI.FI patterns. Currently no entries needed. Open EXP-484 tuning items moved to comments in skip-list.yml: - missing-zero-check (Medium): often FP on LI.FI ctor params - divide-before-multiply (Medium): matches our (a*BASE)/b idiom - Semgrep lf-046 / lf-029 / lf-053: rules need pattern tightening (these are Semgrep rule precision issues, fixed in audit/knowledge/semgrep/ not here) Cost impact (per-PR Stage 2 cost estimate, updated): Small PR (<5 files, 1-5 findings) $0.30 - 0.50 Medium PR (6-15 files, 5-15 findings) $0.50 - 1.00 Large PR (16-30, 15-50 findings) $1.00 - 2.20 These are 40-60% below the previous estimate because the actual fp-check input set is much smaller than the unfiltered SARIF suggested. Validated locally: - actionlint → 0 findings - zizmor 1.23.1 → 0 findings - markdownlint → 0 errors - skip-list.yml parses (now 0 entries; all guidance in comments) Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * fix(semgrep): tighten lf-046 / lf-029 / lf-053 rules (EXP-484) Three rules surfaced as systematically false-positive in the EXP-483 dry-runs on PR #1715, #1731, #1711. Fixed: lf-046-bytes32-nonevm-receiver Problem: `metavariable-regex` was a SIBLING of `pattern-either` at the top level of the rule, which semgrep silently ignored. Every `bytes32 ...` declaration in scoped paths was matching, including NAMESPACE constants, EIP-712 typehashes, request IDs, etc. Fix: wrap `pattern-either` and `metavariable-regex` in a `patterns:` block so AND-conjunction is correctly applied. Also tightened the regex to only match names containing "nonEvm/btc/bitcoin" + "receiver" — removed "destination" (matched callers/sentinels) and "solana" (Solana addresses safely fit in 32 bytes per protocol). lf-029-shared-modifier-parallel-fields Problem: matched ANY `$SD.receiver` read, including `_bridgeData. receiver` (the canonical parent) inside `keccak256(abi.encode(...))` for EIP-712 signature binding (good practice), inside if/else branches (`if (_bridgeData.receiver == NON_EVM_ADDRESS)`), and inside the validation guard itself. Fix: pattern-not-inside excludes keccak256/abi.encode/abi.encodePacked blocks, equality/inequality comparisons (== / !=), and the LI.FI canonical guard `LibUtil.validateReceiver(...)`. metavariable-regex on $SD excludes `_bridgeData` so the rule only flags bridge-SPECIFIC structs (the actual LF-029 class). lf-053-loop-msg-value Problem: fired on every `{value: msg.value}(...)` invocation, including single-swap entry points where there's no loop and msg.value can't be spent multiple times. Fix: add `pattern-inside: for/while/do-while` constraint. Only fires when the call is inside an iterating construct. Validation (re-run on the 3 dry-run PRs): PR before tune after tune delta ───────── ───────────── ──────────── ────── #1715 1 0 −100% #1731 9 2 −78% #1711 9 1 −89% TOTAL 19 3 −84% The 3 remaining findings are legitimate concerns routed to fp-check: - PR #1731 L43: bytes32 nonEVMReceiver (TP class) - PR #1731 L114: IERC20(USDC).approve (likely FP per fp-check; owner- gated one-time init) - PR #1711 L64: bytes32 nonEVMReceiver (TP class) Documented the tuning rationale inline in each rule's `metadata.# EXP-484 tuning notes:` comment so future maintainers see why the structure looks the way it does. Updated audit/findings/skip-list.yml to remove the "open EXP-484 candidate" notes that are now addressed. Validated locally: - `semgrep --validate --config audit/knowledge/semgrep` → 0 errors, 8 rules loaded - Isolated test fixtures confirm intended matches/non-matches Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * chore(semgrep): delete lf-053-loop-msg-value (duplicate of Slither detector) LI.FI's lf-053-loop-msg-value rule is a straight duplicate of Slither's upstream-maintained `msg-value-loop` detector (Medium impact, ships since Slither 0.10.x). Both Stage 1 (Slither) and Stage 1 (Semgrep with this rule) were firing on the same pattern. Removing the rule: - No coverage loss — Slither's detector already catches the same pattern and runs in the same Stage 1 step. - Reduces our maintenance surface (one fewer rule pattern to keep precise as Solidity / facet patterns evolve). - Validates the audit corpus principle: ship LI.FI Semgrep rules ONLY for patterns Slither structurally cannot catch. The LF-053 LF-NNN entry stays in audit/knowledge/findings.json — that is historical knowledge of the finding shape. Only the Semgrep rule file is removed. Surfaced by PR review of #1884 (audit knowledge corpus seed): reviewer questioned why we ship a Semgrep rule for a pattern Slither already flags. The other 7 LI.FI Semgrep rules were verified to NOT have a precise Slither equivalent (lf-002 partially overlaps with unchecked-transfer but has a different framing; lf-008 partially overlaps with missing-events-* but is broader; the rest are LI.FI-specific). Followup: audit lf-002 and lf-008 against unchecked-transfer and missing-events-* more rigorously to confirm they cover patterns Slither cannot. Tracked as EXP-484 work. --------- Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
…ools<81) slither: install via pip and let crytic-compile drive the pinned foundry instead of crytic/slither-action — the Docker action runs npm install (repo peer-dep conflict, ERESOLVE) and its ignore-compile path can't parse foundry 1.7.1 build-info. --fail-none keeps the step advisory. semgrep: pin setuptools<81 (>=81 removed pkg_resources, which semgrep imports; py3.12 no longer bundles it) so the install step stops failing.
…fi-pr-review-skill
With Stage 2 now present, Stage 1 SARIF feeds the AI triage as an artifact rather than going to Code Scanning directly — drop the raw Slither/Semgrep uploads (and the now-unused security-events: write on those jobs) so only curated.sarif surfaces. Add id-token: write to the lifi-pr-review job (claude-code-action fetches an OIDC token at startup). Align docs: Stage 1 is input-only, and note Stage 2's first run is post-merge (workflow parity).
…tmodules) CodeRabbit (critical): branch = main lets `git submodule update --remote` drift the vendored security tooling off the pinned commit (a56045e9), defeating the supply-chain pin. Other submodules carry no branch directive; this aligns tob-skills with them and enforces strict SHA pinning.
Remove the variant-analysis ToB skill from the /lifi-pr-review orchestration: the Step 5 variant-hunt phase, the plugin symlink, the workflow prompt's plugin list, NOTICE attribution, docs diagram/cost notes, and the curated.sarif/summary.md/status.json schema references. Step 6 (emit deliverables) becomes Step 5. fp-check + differential-review remain the per-PR review engine. The vendored ToB submodule is untouched (still pinned at a56045e9); variant-analysis simply is no longer wired into the per-PR run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CodeRabbit: github.rest.issues.listComments returns only the first page (default 30). On a PR with >30 comments the sticky marker comment could be on a later page, so the finder would miss it and post a duplicate instead of updating. Use github.paginate(..., per_page: 100) to scan all pages. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ecurity-review-pipeline
…ecurity-review.yml:297)
CodeRabbit (Critical): the 'Upload curated SARIF to Code Scanning' step
gates on hashFiles(format('{0}/curated.sarif', env.OUT_DIR)), but
hashFiles() only evaluates files under $GITHUB_WORKSPACE. With
OUT_DIR=/tmp/output the glob always returns '', so the condition is
never true and Stage 3a (Code Scanning upload) is skipped on every run.
Move OUT_DIR under the workspace so the guard works.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-without-event.yml:20)
CodeRabbit: the bare 'function ... onlyOwner ... { ... }' pattern matched
EVERY admin function, including those with proper event emission — a 100%
false-positive rate. Add a pattern-not for an emit in the body so only
admin-gated functions with no emit are flagged. Verified against a fixture:
emit-bearing admin fn is now excluded.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… (lf-003-eth-transfer-fixed-gas.yml:18) CodeRabbit: the comment promised an IERC20 exclusion 'below' that never followed, and the metavariable-pattern (payable($Y) | $Y) was a no-op. ERC20 transfer/send are 2-arg, so the single-arg $X.transfer($AMT) / $X.send($AMT) forms already match only native-asset payouts. Remove the dead block and replace the comment with the arity rationale. Verified: 2-arg IERC20 transfer no longer matches. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🛡️ Security ReviewStage 2 (AI triage) did not produce a summary. Raw Stage 1 (Slither/Semgrep) output is attached as a workflow-run artifact; see the run logs. |
Collaborator
Test Coverage ReportLine Coverage: 90.10% (3424 / 3800 lines) |
…used coverage script Rename the lifi-pr-review skill to lifi-security-review so the name reflects that this is a targeted security review, not a generic PR review. Updated across the command file, skill symlink, workflow (job id, name, comment marker, output artifact, SARIF driver name), docs, and NOTICE. Remove script/tasks/checkAuditKnowledgeCoverage.ts and its doc references. It was an advisory local diagnostic never wired into CI. Add extract-audit-knowledge and lifi-security-review to the .agents/README command table. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
🛡️ Security ReviewStage 2 (AI triage) did not produce a summary. Raw Stage 1 (Slither/Semgrep) output is attached as a workflow-run artifact; see the run logs. |
- lf-002: nest metavariable-regex under `patterns:` so the $TOKEN name
constraint actually applies (it was a silently-ignored top-level sibling
of pattern-either); keep the `.*` regex prefix required by re.match
anchoring so names like `inputToken` still match
- Trigger only on src/** changes — knowledge-only PRs no longer launch a
paid Stage 2 review against an empty diff
- permissions: {} at the workflow level (default-deny; each job grants its own)
- slither: init lib/ submodules only, skipping the Stage-2-only tob-skills
- lf-029: drop dead src/Facets/Helpers/** exclude (helpers live in src/Helpers/)
- route src/Helpers/ to cross-cutting.md (helpers.md was never produced)
- document the tob-skills submodule init for the local dry-run
- README: refresh the add-audit row (--skip-extract + corpus refresh)
- delete lf-003 and lf-008 — unreliable detectors (one-arg transfer/send
over-match; generic mode can't parse Solidity modifiers)
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The keyed lifi-security-review job holds ANTHROPIC_API_KEY plus comment/Code-Scanning write access and ingests the PR diff + SARIF. GitHub withholds secrets from direct fork PRs, but convertForkedPRsToInternal.yml re-pushes a fork onto a same-repo fork-pr-<N> branch and opens an internal PR that would receive the secret, exposing the agent to prompt injection from attacker-controlled input (key/comment abuse or a false "all clear"). Gate the keyed job to skip direct fork PRs and converted fork-pr-* branches, and have the conversion workflow stamp an external-fork label as a drift-proof secondary signal. Stage 1 (Slither/Semgrep) is unprivileged and still runs on external PRs. EXP-440 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CR: metavariable-regex is start-anchored (re.match), so the prior regex missed prefixed names like _nonEvmReceiver, bridgeBtcReceiver, dstBitcoinReceiver — silent false negatives. Prepend `.*` and group the alternation. Verified on a fixture: 4 non-EVM receivers match, feeReceiver and solanaReceiver still excluded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CR: the message documents IERC20(t).balanceOf(address(this)) used as a transfer amount, but the only token-balance pattern was the WETH-specific withdraw form, so IERC20(token).transfer(to, IERC20(token).balanceOf(address(this))) passed Stage 1 unnoticed. Add transfer/transferFrom/safeTransfer/safeTransferFrom forms. Verified on a fixture: 3 full-balance calls match, a scoped-amount call does not. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CR: standalone `pattern-not-inside: abi.encode(...)` / `abi.encodePacked(...)` suppressed any encoded receiver read, hiding unchecked receivers packed into bridge calldata / protocol payloads — the LF-029 bug class. The EIP-712 case is keccak256(abi.encode(...)), already covered by `pattern-not-inside: keccak256(...)`; drop the two over-broad exclusions. Verified on a fixture: a direct read and an abi.encode-into- calldata read both match, while keccak256 binding, the parent _bridgeData.receiver, and an equality guard stay excluded. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CR: the rule fired on ERC20 selector calls even inside an explicit
non-native branch, adding noise that can drown out real LF-054
regressions. Add `pattern-not-inside: if (!LibAsset.isNativeAsset(...))
{ ... }`. Other guard forms (else-branch of a positive check, early
return, sentinel comparison) are not expressible as pattern-not-inside and
are left to Stage 2 fp-check triage, as documented in the rule. Verified on
a fixture: an unguarded call matches, the wrapping-guarded call does not.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CR: the message implied SafeERC20 handles fee-on-transfer, but it only wraps return-value and allowance behavior. Drop fee-on-transfer from the "what direct calls break" list and add an explicit note that deflationary tokens still require balance-delta accounting. Message-only change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
CR: the title said ">= 484 instead of > 484" but the entry's own root_cause and fix_summary describe a `< 484` check that should be `<= 484` (484 bytes = empty inner SwapData wrongly accepted). Align the title with the documented semantics. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…it-knowledge.md:117) CR: Step 7 "Existing finding" appended a status_history row unconditionally, so a rerun on an already-processed audit added a duplicate row every time, contradicting the idempotency claim. Only append when the (audit_id, source_label) pair is new; otherwise skip the mutation. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…251) CR: the comments promise missing SARIF artifacts are tolerated, but the download step had no non-fatal setting; if both analyzers fail before upload, the step could abort Stage 2 before the diff-only review runs. Add continue-on-error: true. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…review.yml:121) CR: the workspace is checked out at PR head, then Stage 1 consumes the Semgrep corpus and Stage 2 consumes the LI.FI/ToB skill logic, audit knowledge, waivers, and the ToB submodule — all PR-controlled. A PR touching src/** could weaken or redirect its own secret-bearing review. Restore these from the trusted base revision before running: - semgrep job: fetch-depth 0 + checkout audit/knowledge/semgrep from base. - lifi-security-review job: checkout .agents (skill source of truth), the .claude skill/plugin symlinks, audit/knowledge, .gitmodules and the ToB submodule from base; restore or drop audit/findings/waived.yml. The PR diff is captured to DIFF_FILE first, so the diff review still sees the PR's src/ changes; only the controls revert. New controls added by a PR take effect on the next PR after merge. base.sha is an ancestor of HEAD so the restore is offline (no auth dependency). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.agents/commands/lifi-security-review.md:
- Around line 161-164: Make ${OUT_DIR}/curated.sarif mandatory even when the
large-PR guardrail exits early, since Stage 3 only uploads curated.sarif and
otherwise Code Scanning gets nothing. Update the guardrail path in
lifi-security-review so it still produces a valid SARIF file at
${OUT_DIR}/curated.sarif (using the existing curated SARIF shape with
tool.driver.name = "lifi-security-review") before exiting, even if it only
contains the summary/findings available at that point.
- Around line 99-100: The by-area mapping table in the security review command
guide is missing an entry for src/Interfaces, so interface-only changes fall
through without the right Stage 2 context. Update the mapping in the table
alongside the existing src/Helpers and cross-cutting entries so src/Interfaces
points to audit/knowledge/by-area/cross-cutting.md, or the appropriate dedicated
interfaces page if one exists, keeping the routing consistent with how PR_FILES
is used by security-review.yml.
In @.github/workflows/security-review.yml:
- Around line 251-260: The checkout step in the security review workflow is
fetching submodules too early, before the trusted base restore has run. Update
the checkout logic around the Checkout step so it only fetches the superproject
at this point, and defer initializing `.claude/vendor/tob-skills` until after
the `base.sha` restore/hardening step. Use the existing workflow step names and
the `actions/checkout` configuration to keep the PR-head `.gitmodules` and
gitlink from being trusted prematurely.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: efd5a656-c54f-4a04-86e1-46b2acf48271
📒 Files selected for processing (15)
.agents/README.md.agents/commands/add-audit.md.agents/commands/extract-audit-knowledge.md.agents/commands/lifi-security-review.md.claude/skills/lifi-security-review/SKILL.md.github/workflows/convertForkedPRsToInternal.yml.github/workflows/security-review.ymlNOTICEaudit/knowledge/findings.jsonaudit/knowledge/semgrep/lf-001-balance-of-this-as-amount.ymlaudit/knowledge/semgrep/lf-002-direct-ierc20-approve.ymlaudit/knowledge/semgrep/lf-029-shared-modifier-parallel-fields.ymlaudit/knowledge/semgrep/lf-046-bytes32-nonevm-receiver.ymlaudit/knowledge/semgrep/lf-054-native-sentinel-into-call.ymldocs/security-review.md
✅ Files skipped from review due to trivial changes (6)
- .claude/skills/lifi-security-review/SKILL.md
- .github/workflows/convertForkedPRsToInternal.yml
- NOTICE
- .agents/commands/add-audit.md
- docs/security-review.md
- .agents/commands/extract-audit-knowledge.md
🚧 Files skipped from review as they are similar to previous changes (5)
- audit/knowledge/semgrep/lf-054-native-sentinel-into-call.yml
- audit/knowledge/semgrep/lf-002-direct-ierc20-approve.yml
- audit/knowledge/semgrep/lf-029-shared-modifier-parallel-fields.yml
- audit/knowledge/semgrep/lf-001-balance-of-this-as-amount.yml
- audit/knowledge/semgrep/lf-046-bytes32-nonevm-receiver.yml
….md:98) CR: security-review.yml feeds every changed src/ path into PR_FILES, but the by-area table had no src/Interfaces/ row, so interface-only PRs loaded no area corpus beyond lessons.md. Route src/Interfaces/ to cross-cutting.md. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…-review.md:164) CR: Stage 3 uploads only curated.sarif (Stage 1 raw SARIF is an artifact, not a Code Scanning upload), so the large-PR guardrail writing just a summary left the PR with no Code Scanning coverage. Make the guardrail emit a valid zero-result curated.sarif before exiting, document the deliverable as always-written, and correct the Step 1 text that wrongly claimed Stage 1 findings stay visible in Code Scanning. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…259) CR: the checkout did `submodules: recursive` against the PR-head .gitmodules + gitlink, so a same-repo PR could make this secret-bearing job fetch an arbitrary submodule repo/commit before the base-restore step reset it. Drop submodule init from checkout; the restore step already inits .claude/vendor/tob-skills from the trusted base revision (git submodule sync + update --init from base). Nothing between checkout and the restore uses the submodule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mirooon
reviewed
Jul 1, 2026
mirooon
reviewed
Jul 1, 2026
mirooon
reviewed
Jul 1, 2026
…EXP-440) Restrict the claude-code-action toolset to Read,Grep,Glob,Write so an injection via the PR diff cannot exfiltrate ANTHROPIC_API_KEY or reach the network (drops Bash/WebFetch; keeps Write for OUT_DIR). Add an untrusted-input rule to the invocation prompt and mirror it into the skill's Hard rules so diff/SARIF/file content is treated as attacker-influenceable and cannot change the verdict. Correct the docs setup note: forks no longer reach Stage 2, so the residual path is internal-PR content, contained by the toolset + prompt rule. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
mirooon
approved these changes
Jul 2, 2026
…ity-review-pipeline # Conflicts: # .agents/README.md
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Which Linear task belongs to this PR?
EXP-440 — automated per-PR security-review pipeline.
Rolls up the work previously tracked as EXP-478, EXP-479, EXP-480, EXP-483, EXP-484, and EXP-481.
Why did I implement it this way?
This is the EXP-440 security-review pipeline, consolidated into one PR. It was
originally developed as a 5-PR stack (#1883 → #1884 → #1885 → #1887 → #1889);
those are superseded by this single PR and closed. All of their content is
included here unchanged except for the two deltas noted below.
What this delivers
A per-PR security review that runs in CI on every non-draft PR touching
src/**oraudit/knowledge/**:Stage 1 tool output is an input to Stage 2 triage (downloaded as an
artifact), not a standalone Code Scanning check — raw, untriaged tool output is
exactly the noise Stage 2 filters. Only the curated SARIF reaches the Security
tab.
LI.FI's value-add stays in our code (MIT/LGPL):
.agents/commands/lifi-security-review.md— the orchestrator skillaudit/knowledge/— the LF-NNN corpus (39-audit backfill) fed as cached contextaudit/findings/{skip-list,waived}.yml— rule-level + per-finding suppressionsaudit/knowledge/semgrep/lf-*.yml— custom rules derived from past findingsThe reasoning machinery (gate reviews, diff analysis, attacker modeling) is
vendored from Trail of Bits' open-source skills at
.claude/vendor/tob-skills/(submodule pinned to
a56045e9, 2026-05-05), exposed via symlinks at.claude/plugins/{audit-context-building,differential-review,fp-check,semgrep-rule-creator}.We wrap, we don't fork —
NOTICEcarries the full CC-BY-SA-4.0 attribution.Deltas vs. the original stack
variant-analysisfrom the per-PR run (the ToB skill, its pluginsymlink, the Step 5 variant-hunt phase, and all schema/doc/NOTICE references).
fp-check + differential-review remain the review engine. The vendored
submodule is untouched; the skill simply no longer wires that plugin in.
security-review.yml) so a PR withFork-PR / lethal-trifecta hardening
The keyed Stage 2 job (
ANTHROPIC_API_KEY+pull-requests:write+security-events:write, ingesting the PR diff/SARIF) holds all three legs ofthe lethal trifecta. GitHub withholds secrets from direct fork PRs, but
convertForkedPRsToInternal.ymlre-pushes a fork onto a same-repofork-pr-<N>branch and opens an internal PR that would receive the secret.Two changes close this:
lifi-security-reviewjob isgated to skip direct fork PRs (
head.repo != repo) and converted fork PRs(
fork-pr-*branch prefix orexternal-forklabel, the latter now stampedby the converter). The OR-gate fails closed for external code. Stage 1
(Slither/Semgrep) is unprivileged and still runs on forks for deterministic
feedback. (CodeRabbit's bot-identity hardening was considered and declined:
it would make the external-skip fail open if the bot login were ever
misconfigured, against the "never run on external" priority. The
if-without-status-function note was left as-is — Stage 1'scontinue-on-errorsatisfiesneedsin the common case.)restore the Semgrep corpus, LI.FI/ToB skill logic (
.agents), auditknowledge, FP waivers, and the ToB submodule from
base.sha, so a PRtouching
src/**can't weaken the controls that review it. The PR diff iscaptured first, so the diff review still sees the PR's
src/changes.Validation: dry-runs against real PRs
PR #1711's Medium (swap-amount / signed-amount divergence in
swapAndStartBridgeTokensViaLayerSwap) is the class of bug static toolsstructurally cannot find — what this pipeline is designed to surface.
Operational prerequisites (blocks live runs)
Repo admin must:
security-review(Settings → Environments).ANTHROPIC_API_KEYas an environment secret (not a plain repo secret).Stage 2's
claude-code-actiononly runs once this workflow file is on thedefault branch (its built-in guard against a PR editing the review workflow to
exfiltrate the API key), so the first real Stage 2 run is post-merge. Stage 1
and the sticky comment run on any branch. The workflow stays advisory
(
continue-on-error: true) until EXP-485 flips the gate.Deferred review findings (acknowledged, follow-up)
/pr-ready(local CodeRabbit) on the consolidated branch surfaced 11 findings.Five were fixed in dedicated
pr-ready:commits (the CriticalhashFiles/OUT_DIRbug that skipped the Stage 3 SARIF upload; the lf-008 100%-FP rule; the lf-003
misleading no-op pattern; a draft-PR doc contradiction; and TS input-shape
validation). The following four are deferred as enhancements / nuance, not bugs:
lifi-security-review.md—upstream_skillrecords the verifier (fp-check /differential-review) but not the originating Stage 1 tool for confirmed
Slither/Semgrep survivors. Provenance enhancement; revisit with the SARIF schema.
extract-audit-knowledge.md— "higher severity" merge precedence isunspecified; spell out an explicit ranking.
extract-audit-knowledge.md— lists ahelpersarea with no downstreamprojection/consumer; align the area enum.
add-audit.md— intake overview still says "parse a pasted PDF"; rewordaround locating the referenced file on disk.
A subsequent high-effort review fixed further items in-branch: the lf-002
metavariable-patternwas a silently-ignored top-level sibling (the nameconstraint now applies); the workflow triggers on
src/**only, soknowledge-only PRs no longer launch a paid empty-diff Stage 2 run; workflow
permissions are now default-deny (
permissions: {}); the Slither job initsonly
lib/submodules (skipping the Stage-2-only tob-skills); the deadsrc/Facets/Helpers/**exclude and thehelpers.mdrouting pointer werecorrected; and the low-precision
lf-003/lf-008rules were removed.The
/pr-readyround alongside the fork-gate change surfaced 11 findings; 9were fixed in dedicated
pr-ready:commits and the local re-run is clean:lf-046 receiver regex was start-anchored (missed prefixed names); lf-001 only
caught the WETH-specific balance read, not ERC20
balanceOf-as-amount; lf-029'sstandalone
abi.encodeexclusion hid receivers packed into calldata; lf-054fired inside
!isNativeAssetguards; lf-002's message overstated SafeERC20 vsfee-on-transfer; the LF-011 finding title contradicted its own root_cause;
extract-audit-knowledgereruns appended duplicatestatus_historyrows; theStage 1 SARIF download wasn't
continue-on-error; and controls are now loadedfrom the base revision (above). The two
if:-gate findings are intentionallynot applied (see "Fork-PR / lethal-trifecta hardening").
Known pre-existing lint issue (not in this diff):
convertForkedPRsToInternal.ymldoes not parse inactionlint/zizmor— theCOMMENT_BODYmulti-line string in the "Comment and close original PR" step hascolumn-0 continuation lines that terminate the YAML block scalar. This predates
this PR and is out of scope; a separate fix should rebuild the body with
printf(matching the PR-body construction in the step above).Checklist before requesting a review
/pr-ready(local CodeRabbit) on this branch and resolved (or explicitly documented) all findings — see.agents/commands/pr-ready.mdChecklist for reviewer (DO NOT DEPLOY and contracts BEFORE CHECKING THIS!!!)