# Module S06 — Secure Code Review Harnesses **Course**: 2A — Building AI Harnesses for Cybersecurity **Module**: S06 — Secure Code Review Harnesses **Duration**: 75 minutes **Level**: Senior Engineer and above **Prerequisites**: S00–S05 complete > *A layered pipeline that turns pull requests into verified, prioritized security findings — deterministic scanners for recall, an LLM triage layer for precision, semantic codebase memory for cross-file taint flows, and an approval-gated autofix loop that never merges without a human.* --- ## Learning Objectives 1. Design a layered code review pipeline (AST → Semgrep → CodeQL → LLM → synthesis) and defend the ordering — deterministic first, LLM last. 2. Implement LLM-powered false-positive triage with confidence scoring and a human-feedback loop, and measure precision/recall/F1 against a labeled dataset. 3. Build a semantic codebase index (function-level vector embeddings) that retrieves cross-file taint context for each finding under a bounded context budget. 4. Engineer an autofix loop (patch → verify → approve → merge) and explain why human approval is non-negotiable. --- # S06.1 — Code Review Harness Architecture A secure code review harness ingests a change — a pull request diff, a full file, a repo snapshot, or a pre-commit hook payload — and emits a structured, prioritized set of findings, each with a CWE, a severity, a confidence, and a remediation. The architecture is a layered pipeline. Each layer has a job; no single layer is trusted alone. ## The layered pipeline ``` Input (PR diff / file / snapshot / commit hook) ↓ 1. AST parsing — syntax tree, symbol resolution, cheap structural checks ↓ 2. Semgrep — fast pattern-based rules, custom + registry ↓ 3. CodeQL — deep data-flow queries, taint tracking ↓ 4. LLM semantic — context-aware reasoning, cross-file judgement, FP triage ↓ 5. Synthesis — dedup, normalize, score, route (comment / queue / block) ↓ Structured findings (file, line, CWE, severity, confidence, remediation, PR comment) ``` The order is not arbitrary. **Deterministic scanners run first** because they are fast, deterministic, and free of hallucination. Semgrep runs in seconds across a large diff; CodeQL runs in minutes on a prepared database. Both produce raw findings with known CWE mappings. The LLM runs last, on the *filtered* set of raw findings plus surrounding context, to do what deterministic scanners cannot: judge whether the finding is exploitable in *this specific usage*, and synthesize across overlapping findings from different scanners. ## Why deterministic first, LLM last Running the LLM first is an anti-pattern. An LLM scanning a raw diff at scale is slow, expensive, inconsistent across runs, and prone to both hallucinated findings and missed real ones. The LLM's strength is judgement on a *small, bounded* set of candidates — not brute-force scanning. By the time the LLM sees a finding, the deterministic layers have already established that a pattern matches a known weakness; the LLM's job is to answer "is this actually exploitable here, given the surrounding code?" This separation also gives you a measurable pipeline. Each layer has a recall budget and a precision budget. Semgrep's recall is high (it catches most pattern-matching weaknesses) but its precision on a large codebase is low (many false positives in framework-specific contexts). The LLM triage layer trades a small amount of recall for a large gain in precision. You measure both. ## The output schema Every finding, regardless of which layer produced it, is normalized to one schema: ```typescript interface Finding { id: string; file: string; start_line: number; end_line: number; cwe: string; // e.g. "CWE-89" cwe_name: string; // "SQL Injection" severity: "critical" | "high" | "medium" | "low"; confidence: "high" | "medium" | "low"; // LLM-assigned, post-triage description: string; remediation: string; source: "ast" | "semgrep" | "codeql" | "llm"; pr_comment: string; // markdown, ready to post dedup_key: string; // for cross-scanner dedup } ``` The `confidence` field is the load-bearing output of the LLM triage layer. It gates routing: high-confidence critical findings can block a PR; medium-confidence findings queue for human review; low-confidence findings are filed but do not surface as blocking comments. ## Routing: auto-comment, queue, or block The synthesis layer maps each finding to one of three actions based on severity and confidence: | Severity | Confidence | Action | | --- | --- | --- | | Critical/High | High | **Block** the PR (status check fails) | | Critical/High | Medium | **Queue** for human reviewer; inline comment | | Medium/Low | High | **Auto-comment** inline on the PR | | Any | Low | **File** silently in the dashboard; no PR noise | Blocking a PR is a strong action — it should require both a high severity and a high post-triage confidence. The goal is not to gate every PR; it is to gate the findings you are confident are real and serious. Over-blocking trains developers to click "override" without reading. --- # S06.2 — The False Positive Problem Semgrep and CodeQL are recall machines. On a large codebase, Semgrep's registry rules can produce hundreds of findings, of which a large fraction are false positives — the pattern matched, but the framework sanitizes the input, or the sink is not reachable from untrusted data, or the flagged function is used only in tests. Raw scanner output at scale is low-precision. If you surface it directly, developers ignore the bot within a week. ## LLM-powered triage For each raw finding, the harness assembles a triage prompt: the finding itself, the matched code, the surrounding function, and — from S06.3 — any retrieved cross-file context. The LLM is asked a bounded set of questions: ```python async def triage_finding(raw: RawFinding, context: str) -> TriagedFinding: prompt = f""" You are a security triage analyst. Evaluate this finding. Rule: {raw.rule_id} ({raw.cwe}) Location: {raw.file}:{raw.start_line} Matched code: ``` {raw.snippet} ``` Surrounding context: {context} Questions (answer each with evidence): 1. Is the tainted input reachable from untrusted data in THIS code path? 2. Is there a sanitizer, validator, or framework control between source and sink? 3. Is this finding a true positive or a false positive? 4. Confidence: high / medium / low? Respond as JSON: {{reachable: bool, sanitized: bool, verdict: "tp"|"fp", confidence: "high"|"medium"|"low", reasoning: str}} """ result = await triage_model.complete(prompt) return TriagedFinding(**json.loads(result), original=raw) ``` The model is not asked to *find* the vulnerability — the deterministic scanner already did that. The model is asked to *judge* it, in context. This is the same reader/actor separation from S01.3 and the model-judged triage pattern from S02.4: the system that finds a finding should not be the system that scores it. ## Confidence scoring and routing The triage model returns a confidence level. That level drives routing (see the table in S06.1). The critical design choice is the threshold for "high" confidence: too loose and false positives leak into blocking comments; too strict and real vulnerabilities get queued indefinitely. Calibrate against a labeled dataset (see below) and tune the threshold to the precision target — typically ≥90% precision at the "high" confidence level. ## The feedback loop Triage is not set-and-forget. When a human reviewer marks a finding as a false positive (or confirms a true positive the model called low-confidence), that judgement becomes a labeled example. Those examples feed back into the harness as few-shot context for future triage of similar findings: ```python def build_triage_prompt(finding, codebase_index, feedback_store): # Retrieve past human judgements on similar findings similar = feedback_store.query( embedding=embed(finding.rule_id + finding.snippet), k=3 ) examples = format_as_few_shot(similar) return f""" Prior judgements on similar findings: {examples} Now evaluate: {finding} """ ``` The feedback store is a vector index over past human verdicts. Over weeks, the triage prompt for "is this Django ORM call actually injectable?" includes three prior human judgements that said no — and the model's precision on that class of finding improves. This is the loop that turns a static ruleset into a learning harness. ## Measuring harness performance: precision, recall, F1 The harness is measurable. Against a labeled vulnerability dataset — Juliet (NIST) for C/C++/Java, the OWASP Benchmark for Java web, DVWA for PHP, or your own historically-labeled findings — you measure: - **Precision**: of findings the harness surfaced (e.g. high-confidence), how many were real? `TP / (TP + FP)`. - **Recall**: of real vulnerabilities in the dataset, how many did the harness surface? `TP / (TP + FN)`. - **F1**: the harmonic mean, `2 · (precision · recall) / (precision + recall)`. Measure at each stage. Raw Semgrep recall might be 95% but precision 40%. After LLM triage at the "high" confidence threshold, precision rises to 92% while recall drops to 85%. That trade is almost always worth it: a 92% / 85% pipeline is usable; a 40% / 95% pipeline is noise that gets muted. Run the measurement on every harness change — a new Semgrep rule or a triage prompt tweak can quietly shift the curve. A note on what "recall" means here: against a labeled dataset, recall is ground-truth knowable. In production, on your real codebase, recall is unknowable by definition (you don't know about the vulnerabilities you missed). The labeled dataset is the proxy. Treat production precision as measurable (from human feedback) and production recall as an assumption validated only periodically against fresh labeled data. --- # S06.3 — Semantic Codebase Memory Single-file analysis misses cross-file vulnerabilities. A SQL query is built in `repository.py`, the tainted input enters through `api/handler.py`, and the sink executes in `db/engine.py`. A Semgrep rule run on `repository.py` sees the query construction but not the source; a rule on `handler.py` sees the source but not the sink. The taint flow crosses file boundaries, and no single-file scan can see it whole. ## Function-level semantic indexing The harness builds a semantic index of the codebase at function-level granularity. Each function (or method, or endpoint handler) is embedded and stored in a vector database with metadata: file path, symbol name, callers, callees, and the raw source: ```typescript interface CodeUnit { id: string; // stable: file + symbol + signature hash file: string; symbol: string; // function/method name signature: string; // typed signature source: string; // the function body callers: string[]; // ids of functions that call this one callees: string[]; // ids of functions this one calls embedding: number[]; // vector for semantic retrieval } ``` The index is rebuilt incrementally on each PR — only changed functions and their callers/callees are re-embedded. For a large monorepo, a full reindex is expensive; an incremental index tied to the git diff keeps the cost bounded. ## The retrieval query For a finding at `file X, line Y`, the retrieval query asks the index for the context the triage model needs to judge reachability: 1. **The enclosing function** — always retrieved. 2. **All callers** of the enclosing function — where does the data come from? 3. **All callees** of the enclosing function — where does the data go (the sink)? 4. **Definitions of referenced symbols** — the sanitizer function, the ORM method, the framework decorator. Each retrieved unit is a `CodeUnit`. The retrieval is a mix of graph traversal (callers/callees via the static call graph) and semantic search (vector similarity for "functions that look like sanitizers"). ## Context budget management The retrieved context cannot all fit in the triage prompt — a finding deep in a call graph can pull in dozens of functions, each hundreds of lines. The harness enforces a **context budget**: a token limit on retrieved context, typically 4,000–8,000 tokens. Within the budget, retrieval is prioritized by: 1. Direct callers and callees (highest priority — they define the taint path). 2. Sanitizer-like functions (high — they resolve FP/TP). 3. Transitive callers (lower — background context). The budget is a tunable. Too small and the model misses the sanitizer two frames up the stack; too large and the prompt becomes expensive and the model's attention dilutes. Calibrate against the labeled dataset: does raising the budget from 4k to 8k tokens improve triage precision on cross-file findings enough to justify the cost? ## What this buys With cross-file context, the triage model can answer the reachability question that single-file analysis cannot: "the input comes from `request.json` in `handler.py`, passes through `validate_input()` in `validators.py` (which escapes it), and reaches the sink already sanitized — false positive." Without the index, that judgement is impossible; the finding is either surfaced as a likely true positive (noise) or suppressed by a blanket rule (risk). --- # S06.4 — Autofix with Approval Gate The final layer of a mature review harness generates a fix for each confirmed finding — not just a comment describing the problem. But a fix that a model proposes and a fix that ships to production are separated by a verification and approval gate that is non-negotiable. ## The autofix loop ``` Confirmed finding (high confidence, post-triage) ↓ LLM generates patch — targeted diff, minimal scope, preserves behavior ↓ Static verification — linter clean? Semgrep re-scan clean? no NEW findings? ↓ Test verification — existing test suite passes; coverage did not drop ↓ Draft PR opened — harness posts the patch, the finding, the verification evidence ↓ Human approval — developer reviews, requests changes, or merges ↓ Merge ``` Each step is a gate. The patch does not advance until the previous gate passes. ## Patch generation The fix prompt is tightly scoped: the finding, the vulnerable code, the remediation guidance, and the constraint to produce a minimal diff. The model is not asked to refactor, optimize, or improve — only to resolve the specific weakness: ```python async def generate_patch(finding: Finding, context: str) -> Patch: prompt = f""" Resolve this security finding with a minimal, targeted patch. Finding: {finding.cwe} — {finding.cwe_name} Location: {finding.file}:{finding.start_line} Code: ``` {finding.snippet} ``` Remediation guidance: {finding.remediation} Cross-file context: {context} Constraints: - Minimal diff. Change only what is necessary to resolve the finding. - Do not refactor, rename, or reformat unrelated code. - Preserve all existing behavior and test coverage. - Output a unified diff. """ return Patch(diff=await fix_model.complete(prompt), finding=finding) ``` ## Static and test verification A generated patch is not trusted because the model wrote it. The patch is applied to a checkout and verified: 1. **Linter clean** — the patched code passes the project's linter (eslint, ruff, golangci-lint). 2. **Semgrep re-scan clean** — the specific rule that flagged the finding no longer matches. No *new* Semgrep findings are introduced by the patch. 3. **Test suite passes** — the full test suite (or a targeted subset) passes. 4. **Coverage preserved** — the patch did not drop code coverage below the finding's location. If any gate fails, the patch is discarded or revised. The harness does not post a patch that fails verification — that wastes the developer's time and erodes trust. ## Patch quality scoring Beyond pass/fail, the harness scores patch quality: does the fix preserve the surrounding logic? Does it introduce stylistic regressions? Does it handle edge cases the original code handled? A low-scoring patch is held back even if it passes the gates — the harness opens the PR only for patches above a quality threshold, and files the rest as "fix suggested, needs human authorship." ## Why approval is non-negotiable LLM-generated patches can introduce new vulnerabilities while fixing the reported one. A model that closes a SQL injection by escaping quotes may miss a second injection in the same function; a model that adds an authorization check may place it after the sensitive operation; a model that "fixes" a path traversal by rejecting `..` may not handle URL-encoded variants. The deterministic gates catch some of these, but not all. The approval gate exists for the rest. The PR is the approval mechanism. The harness opens a **draft PR** — not a direct commit, not an auto-merge. The developer reviews the patch against the finding and the verification evidence, requests changes or approves, and merges. This is the same principle as S02.3's evidence chain: the human is in the loop on any action with production consequence. Auto-merging security fixes is an anti-pattern that will eventually ship a regression or a new vulnerability. The non-negotiable framing is deliberate. There is a constant pressure to "just auto-merge the high-confidence ones" — it saves a click, it feels like progress. Resist it. The cost of one auto-merged regression that takes down production or introduces a CVE outweighs the saved clicks across thousands of safe merges. The approval gate is the last line of defense, and it is held by a human. --- ## Anti-Patterns ### The LLM-first scanner Running the LLM over the raw diff to find vulnerabilities, skipping the deterministic layers. Slow, expensive, inconsistent, hallucination-prone. Cure: deterministic scanners first (recall), LLM last (precision and judgement). ### Surface-all-findings Posting every Semgrep finding as an inline PR comment. Precision is low; the PR is buried in yellow warnings; the developer mutes the bot. Cure: LLM triage with confidence gating; only high-confidence findings surface as comments, the rest file silently. ### The context-blind triage Asking the LLM to triage a finding with only the matched snippet, no surrounding function, no cross-file context. The model cannot judge reachability, so it defaults to "possible true positive" — re-introducing the false positive problem you built the triage layer to solve. Cure: semantic codebase memory (S06.3) feeding the triage prompt. ### Auto-merge the easy fixes Letting the harness merge high-confidence patches without human review. Saves a click until it ships a regression or a new vulnerability. Cure: the approval gate is non-negotiable. Draft PR, human merge, always. --- ## Key Terms | Term | Definition | | --- | --- | | **Layered pipeline** | AST → Semgrep → CodeQL → LLM → synthesis; deterministic first, LLM last | | **Confidence scoring** | LLM-assigned level (high/medium/low) post-triage that gates finding routing | | **Feedback loop** | Human verdicts on findings stored and retrieved as few-shot examples for future triage | | **Semantic codebase memory** | Function-level vector index with callers/callees for cross-file taint context retrieval | | **Context budget** | Token limit on retrieved cross-file context in the triage prompt | | **Approval gate** | Human review of any LLM-generated patch before merge; non-negotiable | --- ## Lab Exercise See `07-lab-spec.md`. Four labs: (1) layered pipeline (Semgrep + LLM triage) on a PR diff; (2) false-positive triage measured against DVWA; (3) semantic codebase index for cross-file context retrieval; (4) autofix loop with verification gates and a draft PR. --- ## References 1. **Semgrep** — pattern-based SAST, fast, custom rules, the first deterministic layer. 2. **CodeQL** (GitHub) — deep data-flow queries, taint tracking, the second deterministic layer. 3. **OWASP Benchmark Project** — labeled Java web vulnerability dataset for precision/recall measurement. 4. **Juliet Test Suite** (NIST) — labeled C/C++/Java dataset for static analyzer evaluation. 5. **S02.4** — model-judged triage pattern; this module applies it to code review findings. 6. **S01.3** — reader/actor separation; the triage model judges findings the scanners found. 7. **CWE** (MITRE) — Common Weakness Enumeration; the normalization taxonomy for findings across scanners.