On bad output, see guides/skill-recovery.md

⚠️ Standalone invocation only — skip if dispatched by an orchestrator

If invoked directly by the user (not via code-review or another orchestrator
that prepends a goal preamble with
User goal: / Current task: / Success criterion: lines), do this before
producing the critique:

1. Capture the user's goal in 1-2 sentences. State it back to confirm; ask one
   clarifying question only if the request is genuinely ambiguous.
2. Record it verbatim at the top of the report as a **User goal:** line, alongside
   the Commit: <hash> metadata line at the top of the saved artifact. The User-goal
   anchor must persist in the saved file so downstream readers and tools see what frame
   the review was produced under.

When an orchestrator has already supplied the goal preamble, skip this section entirely — the User-goal anchor is already pinned upstream.

Security Code Review

Reviewing code changes for security vulnerabilities. The point is not to find issues
a linter or SAST tool would catch — those are handled elsewhere. Apply
security-specific reasoning to find design-level flaws, implicit trust assumptions, and
patterns that create exploitable conditions even when each line looks correct in
isolation.

What follows is a set of cognitive moves. Not all apply to every
diff — exercise judgment based on what the code does.

Scoping

By default, review files changed on the current branch relative to main:

git diff main...HEAD

If the user provides an explicit scope (file list, directory, or PR number), use that instead.
For each changed file, also read enough surrounding context to understand trust boundaries,
callers, and data flow — a diff alone is rarely sufficient for security analysis.

Dependency manifest changes

If the diff includes changes to dependency manifests (package.json, requirements.txt,
go.mod, Cargo.toml, pyproject.toml, Gemfile, pom.xml, build.gradle, etc.) or
their lockfiles (package-lock.json, yarn.lock, poetry.lock, Cargo.lock, go.sum,
Gemfile.lock, etc.), apply cognitive move #10 (Review dependency changes) in addition to
any other applicable moves. This is not a full supply chain audit — it is a focused check
for the most common dependency-related risks that appear in diffs.

Using the Code Fact-Check Report

If provided a code-fact-check report alongside the diff, treat it as your
foundation for understanding what the code actually does.

Instead of re-verifying behavior:

• Reference the fact-check findings where relevant. If a comment claims "input is sanitized
  by middleware" and the fact-check says that's stale, that's a security-relevant finding you
  should build on.
• Focus on security implications of fact-check findings. A "mostly accurate" claim about
  thread safety might be fine for correctness but critical for security.
• Prioritize your cognitive moves, which are what this skill uniquely provides.

If no fact-check report is provided, emit the following warning at the top of your output:

⚠️ No code fact-check report provided. Claims about security properties in comments and
documentation have not been independently verified. For full verification, run the
code-fact-check skill first or use the code-review orchestrator.

Then proceed with security analysis based on reading the actual code.

The Cognitive Moves

1. Trace the trust boundaries

Before analyzing individual code, map where trust transitions happen. Every system has
boundaries where data moves from less-trusted to more-trusted context: user input entering
the application, data crossing service boundaries, values read from external storage,
deserialized objects, URL parameters, HTTP headers, environment variables.

For each changed file, identify: what enters from outside? What leaves to somewhere else?
What assumptions does this code make about the trustworthiness of its inputs? The most
dangerous vulnerabilities happen when code on the trusted side of a boundary treats
untrusted data as safe — and the diff may be adding, moving, or removing a boundary
without making that explicit.

Two classification rules govern every source you map:

• Trust is per-consequence, not per-source. A source is classified toward a sink
  class, never globally — the same value can be trusted for availability decisions and
  untrusted for fetch/exec/deserialize/HTML sinks. A boundary map that stamps a source
  "trusted" in general has chosen the wrong grain: name the sink class each
  classification applies to. Measured driver: a benchmark review classified an
  admin-configured feed URL as trusted-in-general, analyzed the open() injection
  primitive for two sibling paths, and never applied it to the one call that fetched
  that URL unguarded — the diff's only Critical (SSRF) went unformed
  (docs/working/fn-trace-skill-levers-2026-08-21.md, lever 5).
• Runtime-mutable ⇒ compromise-reachable. Any value changeable through the
  application's own UI/API after deploy — site settings, DB-backed config, feature
  parameters, admin-editable records — is reachable by whoever compromises a session
  with that permission, including via the very vulnerabilities under review (the XSS
  you just found steals exactly the admin session that edits the setting). Classify
  such values untrusted toward high-consequence sinks (fetch, exec, deserialize,
  raw SQL, HTML interpolation, path construction). Code constants and deploy-time-only
  values may stay trusted; "an admin set it" is a likelihood note, never a
  trust classification.

2. Find the implicit sanitization assumption

Most injection vulnerabilities don't come from missing sanitization — they come from
assumed sanitization that happens somewhere else. When code uses a value without
validation, trace backward: where is this value supposed to be cleaned? Is that actually
guaranteed to happen on every code path that reaches here?

The specific move: for each input the changed code uses, ask "what would happen if
this value contained an attacker-controlled string?" Then trace whether anything between
the input source and this usage actually prevents that. Pay special attention to:

• Values that pass through multiple functions before use (sanitization assumed but not enforced)
• Values that are validated for format but not for content (e.g., regex checks that the string
  looks like an email but doesn't prevent SQL injection)
• Values from "internal" sources that were originally external (e.g., database fields that
  store user input)

3. Check the error path, not just the happy path

Security bugs hide in error handling. When an operation fails, what state is the system in?
Are partial results cleaned up? Are error messages leaking internal details? Does a failed
authentication check fall through to the next handler instead of returning early?

For every conditional or try/catch in the diff, ask: "What happens when this fails? Is the
failure state safe?" The most critical variant: when authentication or authorization checks
throw exceptions, does the catch block deny access or does it accidentally permit it?

4. Identify time-of-check to time-of-use gaps

When code checks a condition and then acts on it, ask whether anything can change between
the check and the use. This applies to:

• File existence checks followed by file operations
• Permission checks followed by privileged actions
• Balance checks followed by transfers
• Input validation followed by processing in a different function

The check and the use don't have to be on adjacent lines — they might be in different
functions, or the check might be cached. The question: can an attacker (or concurrent
operation) change the state between when it was validated and when it was used?

5. Invert the access control model

Take whatever the code allows and ask: "what does this prevent?" Most access control
bugs come from confusion between allowlists and denylists, or from forgetting to check
at all. If the code checks that a user has role X, ask what happens for a user with no
role. If it validates that a resource belongs to the requesting user, ask what happens for
admin endpoints, system resources, or resources with null owners.

The specific move: for each authorization check, enumerate the cases it does NOT cover.
Then check whether those uncovered cases are safe by default (deny) or unsafe by default
(allow). Default-allow is almost always a bug.

6. Follow the secrets

When the diff touches credentials, tokens, API keys, session identifiers, or encryption
keys, trace their lifecycle: where are they created, stored,
transmitted, invalidated? Check for:

• Secrets logged or included in error messages
• Secrets compared with timing-unsafe equality (== instead of constant-time comparison)
• Secrets stored in places with broader access than intended (localStorage, query parameters,
  world-readable files, environment variables inherited by child processes)
• Secrets that never expire or can't be revoked
• Secrets derived with insufficient entropy or predictable seeds

7. Test the serialization boundary

Whenever data is serialized (JSON, XML, protobuf, pickle, YAML) or deserialized, there's a
security surface. On the way out: is sensitive data being included that shouldn't be? On the
way in: can an attacker craft input that causes unexpected behavior during deserialization?

Check specifically for:

• Object properties that are serialized but shouldn't be (internal IDs, password hashes,
  tokens) — often caused by serializing an entire ORM model instead of a DTO
• Deserialization of untrusted data using formats that allow code execution (pickle, YAML
  with !!python/object, Java serialization)
• Type confusion when deserializing — does the code verify the shape of what it received, or
  does it cast and hope?

8. Ask "what if there are a million of these?"

Many security issues only appear at scale. A single request is fine; a million concurrent
requests create a denial of service. A single record is fine; a million records in a response
cause memory exhaustion. Rate limiting, pagination, resource cleanup, and connection pooling
are where these issues live.

For each operation in the diff, ask: can an unauthenticated (or low-privilege) user trigger
this repeatedly? If so, what's the cost per invocation? Is there any limit? This catches:

• Missing rate limiting on expensive operations
• Unbounded queries or response sizes
• Resource allocation without corresponding cleanup
• Amplification attacks (one request triggers many downstream operations)

9. Check the cryptographic choices

When the diff involves encryption, hashing, signing, or random number generation, verify
that the choices are current and appropriate. This is not about memorizing which algorithms
are good — it's about checking that:

• Random values used for security purposes come from cryptographic RNG (not Math.random(),
  not rand(), not time-based seeds)
• Hashing of passwords uses a purpose-built password hash (bcrypt, scrypt, argon2) not a
  fast hash (SHA-256, MD5)
• Encryption uses authenticated encryption (not ECB mode, not CBC without HMAC)
• Signatures are verified before the signed data is used
• Key sizes meet current recommendations

If not confident in your cryptographic assessment, say so and flag it for expert
review rather than guessing.

10. Review dependency changes

When the diff modifies dependency manifests or lockfiles, check for supply-chain risk with
these focused checks:

1. Newly added dependencies — Is this package well-maintained (recent commits, multiple
   maintainers, significant download count)? Is it widely used for this purpose, or is it an
   obscure alternative? A new dependency is new attack surface — each one should have a clear
   reason to exist.

2. Major version bumps — Major version changes can introduce breaking changes and may
   coincide with security-relevant API changes. Check whether the bumped version has known
   CVEs (search the advisory database for the package name and version range). Flag any bump
   that skips multiple major versions, as this suggests a long-deferred update that may carry
   compounding migration risk.

3. Lockfile-only changes — When lockfile changes appear without corresponding manifest
   changes, determine whether they reflect intentional updates (e.g., npm audit fix,
   dependabot) or accidental regeneration. Unexplained lockfile churn can mask dependency
   confusion attacks or introduce unintended transitive dependency changes.

4. Removal of security-relevant dependencies — If the diff removes a dependency that
   provides security functionality (e.g., helmet, csurf, bcrypt, cors, a WAF
   middleware, or an input sanitization library), verify that its functionality has been
   replaced or is genuinely no longer needed.

This is not a full supply chain audit. Do not attempt to review every transitive dependency
or reproduce build artifacts. Focus on what is visible and actionable from the diff.

When reporting dependency findings, use the [Dependency change] prefix in the finding title
so these findings are identifiable in review output. This aids traceability when
evaluating whether the dependency trigger is surfacing risks that would otherwise go
unreviewed.

11. Enumerate bypasses for every guardrail

For every guardrail, matcher, sanitizer, or filter the diff adds or modifies — an auth
middleware matcher, an input sanitizer, a path allowlist, a CSP directive, a rate limiter —
enumerate at least 3 candidate bypass inputs before forming any opinion about it. Think
like the attacker probing it: encoding variants, boundary values, alternate code paths that
skip the guard, request shapes the matcher's pattern doesn't cover, response classes the
policy doesn't reach.

Each candidate must then be dispatched one of two ways:

• Tested — traced through the actual code, or exercised via a fixture/execution, with
  the outcome recorded.
• Listed — placed under an explicit "Untested bypass candidates" heading in the
  report, naming the input and the reason it wasn't tested.

A guardrail with untested bypass candidates may not appear in Endorsement Claims. Enumerating
what would break a guard and then declining to check is the honest state to report — it is
not grounds for a clean verdict.

12. Sweep every call site of an engaged primitive

The moment your analysis engages a dangerous primitive anywhere in the diff scope —
open/URL fetch, process exec, deserialization, raw SQL, HTML interpolation, path
join/traversal, eval-family — grep the scope for all occurrences of that primitive
and disposition each one before moving on. Render the result as a table in the report
(see "Primitive sweep" in the output format): every call site, the source label (S1…)
feeding it, guarded/unguarded, and finding-or-cleared with a one-phrase reason.

An analyzed primitive with an undisposed sibling call site is an incomplete review by
definition — the analysis that would find the bug has already been paid for; only the
enumeration is missing. Measured driver: the benchmark SSRF above was missed by a review
that enumerated Kernel#open command-injection bypasses for two sibling call paths and
skipped the third — the unguarded one (docs/working/fn-trace-skill-levers-2026-08-21.md,
lever 5). The sweep converts that omission from a silent judgment lapse into a visible
empty table row.

Critical Finding Escalation

Some findings are severe enough that they should halt the review and be surfaced to a human
immediately, before continuing with the rest of the analysis. These are not just "Critical
severity" — they are patterns where the risk is so high and the fix so urgent that burying them
in a list of findings would be irresponsible.

When any of the following patterns are detected, stop normal review flow and emit an
escalation block (format below) before continuing with the remaining cognitive moves:

1. Plaintext credentials or secrets in source — API keys, passwords, tokens, or connection
   strings committed to the repository in cleartext (not references to secret stores or
   environment variable names).
2. Missing authentication on privileged endpoints — administrative, data-deletion, or
   configuration-changing endpoints that can be reached without any authentication check.
3. SQL injection or command injection in user-facing code — unsanitized user input
   concatenated into SQL queries, shell commands, or system calls.
4. Disabled TLS or certificate verification — code that sets verify=False,
   NODE_TLS_REJECT_UNAUTHORIZED=0, InsecureSkipVerify: true, or equivalent, in
   production code paths (not test fixtures).
5. Hardcoded cryptographic keys — encryption keys, HMAC secrets, or signing keys
   embedded as literals in source code rather than loaded from secure key management.

Keep this list short. Adding patterns increases noise and escalation fatigue — only patterns
with near-certain exploitability and high blast radius belong here.

Escalation output format

When a pattern matches, emit this block at the top of your critique output, before the
Trust Boundary Map:

> 🚨 **HALT — ESCALATE TO HUMAN**
>
> **Pattern:** [Which escalation pattern matched, e.g., "Plaintext credentials in source"]
> **Location:** `path/to/file.ext:42`
> **Detail:** [1-2 sentences: what was found and why it's urgent]
>
> This finding requires immediate human attention before the remaining review is actionable.
> Do not merge or deploy until this is resolved.

If multiple escalation patterns match, emit one block per pattern. Then continue with the
normal review below. Escalated findings should still appear in the Findings section with
full detail — the escalation block is an early warning, not a replacement for the
structured finding.

How to Structure the Critique

Output your critique as a Markdown document.

Title and Header

Open with a top-level title that includes "Security Review" so the report is
discoverable. Follow with these header fields so readers know what was reviewed and when:

# Security Review — [short scope label, e.g., PR #347 or branch name]

**Scope:** [branch diff / file list / directory under review]
**Date:** [YYYY-MM-DD]

If given a fact-check report or other upstream artifact, add a **Based on:**
line naming it. Keep the header to 3–5 lines; the substance belongs in the sections below.

Trust Boundary Map

Before listing any findings, articulate where input crosses trust boundaries in the
changed code (move #1). Render the boundaries as a labeled ASCII/text diagram showing
the flow from less-trusted to more-trusted context. Use → arrows and bracketed
labels. At least two diagram lines are required for any non-trivial diff — if the
diff genuinely touches only one boundary, state that explicitly and still render it
in the diagram format.

Required format:

B1: [user input]            → [validation point]    → [trusted internal]
B2: [external API response] → [parser / validator]  → [domain model]

Each boundary gets a stable label (B1, B2, …) followed by a single line of the
form [source] → [transition point] → [destination]. If the diff adds, moves, or
removes a boundary, append (new), (moved), or (removed) to the label so the
delta is visible. Use one line per distinct boundary the diff touches.

Below the diagram, add an input-source classification table: one row per distinct
source feeding the changed code — request data, DB fields, runtime-mutable
settings/config, environment, filesystem, third-party responses, message payloads —
with a stable label (S1, S2, …), its mutability class (request-time /
runtime-mutable / deploy-time / code-constant), and its trust classification per sink
class, with justification, applying the two classification rules from move #1
(per-consequence trust; runtime-mutable ⇒ compromise-reachable):

S1: request body            — request-time    — UNTRUSTED (all sinks)
S2: SiteSetting.feed_url    — runtime-mutable — UNTRUSTED for fetch/exec/HTML sinks
                                                 (admin-editable post-deploy);
                                                 trusted for availability
S3: RETRY_LIMIT constant    — code-constant   — trusted (all sinks)

Then add 1-3 sentences of prose summarizing what enters from outside,
what crosses between trust levels, and what assumptions about trustworthiness the
diff makes. The diagram and source table are load-bearing for the rest of the review:
every finding below must cross-reference one of these boundary labels, and every
dangerous-primitive call site in the Primitive sweep names the S label feeding it.

Findings

Anchoring rule: Every finding must reference a boundary from the Trust Boundary
Map by its label (e.g., B1). Findings must not float unanchored. If a finding
genuinely does not involve a trust boundary crossing (rare — e.g., a purely
internal cryptographic-primitive choice with no input crossing involved), use
Boundary: Internal — no boundary and justify in the finding body why a security
issue exists absent any crossing. If you cannot tie a finding to a boundary or
justify its absence, the finding is either out of scope for this skill or the
Trust Boundary Map is incomplete — revise the map before listing the finding.

For each finding, use this structure:

#### [Finding title]

**Severity:** [Critical / High / Medium / Low / Informational]
**Location:** `path/to/file.ext:42-58`
**Boundary:** [B1, B2, … — label(s) from the Trust Boundary Map this finding sits on,
or `Internal — no boundary` with justification in the body]
**Move:** [Which cognitive move surfaced this]
**Confidence:** [High / Medium / Low]

[2-5 sentences: what the vulnerability is, how it could be exploited, and what the
impact would be. Be specific about the attack scenario, and name the boundary
transition where the trust assumption breaks.]

**Recommendation:** [1-3 sentences: what to do about it.]

Severity guidelines:

• Critical: Remote code execution, authentication bypass, unrestricted data access
• High: Privilege escalation, significant data leakage, injection in authenticated context
• Medium: Information disclosure, missing rate limiting, weak cryptographic choices
• Low: Minor information leak, missing security headers, overly permissive CORS
• Informational: Defense-in-depth suggestions, hardening opportunities, non-exploitable patterns

Floor rule: a finding that names a concrete mechanism by which a security property can
be violated in a reachable environment (e.g., a dev-only flag that is enforceable in
production) may not be rated below Medium. Environmental unlikelihood belongs in
Confidence, not Severity — downgrading a named mechanism to Low/Informational is how
real detections die in triage. (A scenario that requires the attacker to already control
the host does not meet the reachable-environment bar.)

Order findings by severity (critical first), then by confidence.

Endorsement Claims

This section replaces free-form praise ("What Looks Good"). It still tells the author what
doesn't need rework — but every positive statement is an atomic, falsifiable claim, not
a verdict. The reviewer never self-certifies code as good; it states exactly what it
established and hands anything stronger to execution-backed verification.

Each entry uses this structure:

- **Claim:** [one atomic, falsifiable statement about a security property of the diff]
  **Location:** `path/to/file.ext:42-58`
  **Evidence:** executed | read-static
  **Verified:** [exactly what was established — the specific code read or command run]
  **Not verified:** [the nearest unread/unexecuted hop — a specific caller, code path,
  or response class one step away, never a generic disclaimer]
  **route: code-fact-check** [only where required — see below]

Rules:

• The Verified / Not verified pair is mandatory on every entry. The Not verified
  line must name the nearest concrete hop not covered (e.g., "caller payloads on the
  non-streaming path", "documents served via prefetch"), not "everything else".
• Categorical vocabulary is banned on Evidence: read-static entries — "safe", "only",
  "never", "correct", "not a bug", "cannot", "correctly reflects", "eliminates". Reading
  code licenses scoped observations, not categorical guarantees. Reading a write signature
  never licenses "the cache contains only X" — the caller's payload is one hop away.
• Any claim that could justify a Confirmed-Good rubric row must be marked
  route: code-fact-check. These claims are submitted to the code-fact-check skill's
  ## Submitted claims intake for execution-backed verdicting; the verdict (Confirmed-Good
  or refuted) comes from there, never from this review. Minor positives that would not
  anchor a rubric row stay as scoped prose entries without the routing tag.
• A guardrail with untested bypass candidates (move #11) may not appear here at all.

When run standalone (no fact-check available to route to), route: code-fact-check entries
remain in the report marked pending execution verification — they are still claims, not
verdicts.

Primitive sweep

Required whenever move #12 engaged: one table per dangerous primitive the analysis
touched, enumerating every call site of that primitive in the diff scope — including
the ones you cleared and the ones you did not analyze:

Primitive: Kernel#open (URL fetch / command injection)
| Call site | Source | Guard | Disposition |
|---|---|---|---|
| `lib/topic_retriever.rb:14` | S1 embed_url | host allowlist | Finding 3 (redirect bypass) |
| `jobs/poll_feed.rb:32`      | S2 feed_polling_url | none | Finding 5 (SSRF) |
| `lib/disqus_import.thor:88` | S4 local path arg | `File.exist?` check | cleared — CLI-only, S4 deploy-time |

Every row's Source cell uses an S label from the input-source classification table.
A row you cannot disposition ("not analyzed") is allowed but makes the review
explicitly partial — say so in the Overall Assessment. If no dangerous primitive was
engaged, state Primitive sweep: no dangerous primitives in scope so absence is
auditable.

Summary Table

#	Finding	Severity	Boundary	Location	Confidence
1	...	Critical	B1	f:42	High

Overall Assessment

One paragraph: what's the security posture of this change? Are the issues fixable in place
or do they indicate an architectural problem? What's the single most important thing to
address?

A "safe to merge" conclusion is permitted only when every Endorsement Claim is either
Evidence: executed or explicitly scoped with its Not verified hop named. Otherwise the
assessment must say: "no findings within the code paths read; endorsement claims pending
execution verification" — not a categorical all-clear.

Output Location

When run standalone, save your critique as docs/reviews/security-review-{date}.md (e.g., security-review-2025-01-15.md) in the project root with a Commit: <hash> metadata line at the top; create docs/reviews/ if it doesn't exist.

When run via an orchestrator, the orchestrator specifies the output path — follow its
instructions.

Tone

Direct and precise. Security review is not about making the developer feel bad — it's about
finding problems before an attacker does. State what's wrong, why it matters, and how to fix
it. Don't hedge on real issues, but calibrate confidence honestly. If unsure whether
something is exploitable, say so and explain what would need to be true for the attack to work.

Important

• Read the actual implementation for every security-relevant code path. Do not rely on
  function names, type signatures, or comments as evidence of safety — read the code.
• Always read enough context beyond the diff to understand trust boundaries and data flow.
  A diff that adds input validation is only useful if you know what happens downstream.
• Do not report issues that static analysis tools reliably catch (unused variables, simple
  type mismatches). Focus on design-level and logic-level security.
• Do not suggest security theater (adding checks that cannot fail, over-validating trusted
  internal data). Every recommendation should address a realistic threat.
• When a finding depends on context you can't see (e.g., "this is safe IF the caller always
  validates"), say so explicitly rather than assuming either way. The same rule binds
  positive statements: a clean verdict extends only to the code paths actually read, and
  every endorsement names what was not read. "I read the write signature" never licenses
  "the cache contains only X" — the caller's payload is one hop away.