tn-review

name	tn-review
description	Code Review & Security Analysis Skill

Code Review & Security Analysis Skill

Use this skill when asked to review code, audit files, analyze PRs, or perform security analysis. Trigger on: "review", "audit", "analyze", "code review", "PR review", "security check", "look at this code", "check this module", "what do you think of this". Use this skill whenever the user asks you to examine code for quality, safety, or correctness — even if they don't explicitly say "review" or "audit".

Project Context

Telcoin-network is a Rust blockchain node combining Narwhal/Bullshark DAG-based BFT consensus with EVM execution via Reth. In this system, correctness equals safety: consensus bugs can halt the network, EVM state transition mismatches can lose funds, and concurrency bugs in async networking can corrupt shared state. The review categories and severity calibrations throughout this skill are tuned for this threat profile — a "Medium" finding in a typical web service may be "Critical" here if it can cause validator disagreement.

Process

Phase 1: Scope & Read

Determine the review scope from the user's request:

• PR diff: Run git diff for the relevant range. Read every changed file in full (not just the diff hunks — surrounding context catches issues the diff alone hides). Also read files that import or are imported by changed files when the change touches a public interface.
• Module/crate: Read all .rs files in the crate. Start with lib.rs or main.rs to understand the module's public surface, then read internal modules. For large crates (20+ files), read lib.rs first to understand structure, then prioritize files touching consensus, state, crypto, or networking.
• Specific files: Read them plus their direct dependents if the change touches public APIs.

While reading, note the file path, line numbers, and any concerns across these categories:

Rust & Systems

• Memory & Safety — unsafe blocks without safety comments, raw pointer manipulation, transmute, missing bounds checks, buffer handling, use-after-free patterns
• Concurrency — data races, deadlock potential (lock ordering), Send/Sync bound issues, missing or incorrect use of Arc/Mutex/RwLock, unbounded channel/queue growth, task cancellation safety (what happens when a tokio task is dropped mid-await?)
• Error Handling — unwrap()/expect() on fallible operations in non-test code (these panic and crash the node), _ catch-all in match arms that silently swallow new variants, error chains that lose context via .map_err(|_| ...)

Consensus & Blockchain

• Determinism — any operation whose output could vary across validators breaks consensus. Watch for: HashMap iteration order, floating point, system time, thread-dependent ordering, randomness without deterministic seeding
• Consensus Safety — incorrect quorum calculations, missing signature verification on messages, accepting messages from wrong rounds/epochs, equivocation handling, certificate validation gaps
• State & Funds — EVM state transitions that don't match Ethereum semantics, incorrect gas accounting, missing balance checks, storage writes without corresponding reads for validation, batch execution ordering assumptions
• Fork Safety — code that behaves differently at different block heights without explicit fork-gating, upgrade paths that could split the network

General

• Security — access control bypasses, unvalidated external input (network messages, RPC params, CLI args), cryptographic misuse (nonce reuse, weak randomness, missing constant-time comparison), resource exhaustion vectors (unbounded allocations from untrusted input)
• Bugs — logic errors, off-by-one, incorrect error propagation, silent failures, type confusion between similar newtypes
• Architecture — layering violations (consensus code reaching into execution internals or vice versa), missing abstraction boundaries, god objects, inconsistent patterns across similar code
• Optimization — unnecessary allocations in hot paths, redundant computation, runtime work that could be compile-time (const fn, type-level computation), inefficient data structures for the access pattern

Phase 2: Document Findings

Write findings to report.md in the project root (or as specified by the user).

Report structure:

# Code Review: [scope description]
Date: [date]
Scope: [what was reviewed — PR number, crate name, or file list]

## Summary
[1-2 sentences on overall assessment]
| # | Title | Severity | Category | Status |
|---|-------|----------|----------|--------|

## Findings

### [N]. [Title]
- **Severity (initial)**: Critical / High / Medium / Low / Informational
- **Category**: [from list above]
- **Location**: `file_path:line_number`
- **Claim**: [standalone factual assertion — what is wrong, NO reasoning chain]
- **Key Question**: [the specific thing a verifier must answer — e.g., "Can this unwrap panic if a validator sends a malformed certificate?"]
- **Relevant Files**: [files that must be read to verify this finding]
- **Source**: tn-review

Severity guide — calibrated for a blockchain node:

• Critical — funds at risk, consensus break/halt, remote code execution, state corruption that persists across restarts
• High — denial of service against the node, privilege escalation, incorrect state transitions that are recoverable, panics in production paths that crash the node
• Medium — silent failures affecting correctness that don't immediately break consensus, missing validation at system boundaries (RPC, network messages), economic impact under specific conditions
• Low — suboptimal patterns, minor inefficiencies, weak diagnostics, unwrap() in paths that are practically safe but not provably so
• Informational — style, naming, dead code, compile-time improvements, idiomatic Rust suggestions

Phase 3: Verify and Present

After documenting all findings in Phase 2, invoke the findings-verifier agent via the Agent tool to verify each finding independently, produce the final report, and present confirmed results.

Pass to the agent:

1. The full contents of report.md (all documented findings in canonical schema)
2. The review scope context (what was reviewed — PR number, crate name, or file list)
3. The list of all changed files

The findings-verifier agent handles:

• Independent subagent verification of each finding (anti-confirmation bias — verifiers never see your original reasoning)
• Tiered verification (CRITICAL/HIGH individually, MEDIUM batched 2-3, LOW batched 3-5, INFO skipped)
• Proposed fixes for confirmed findings using the remediation decision tree
• Updating report.md with verification results
• Presenting confirmed findings in the conversation with verification stats

Do not present findings to the user before findings-verifier completes. Unverified findings waste time.

Rules

• Every finding goes through subagent evaluation before being presented as confirmed. Unverified findings waste time.
• Read code before commenting on it. Speculation produces false positives.
• Include file_path:line_number references for every finding.
• Propose fixes, not just problems. A finding without a solution is half-finished work.
• Group subagent work to keep each focused (2-3 concerns max per agent).
• Calibrate severity honestly. A style issue is Informational, not Medium. An unwrap() on a Some that's guaranteed by the previous line is not High.
• For state transitions and fund flows, verify complete accounting (inputs = outputs + fees).
• Check both the Rust code layer and any contract/protocol-level validation before concluding a finding is valid.
• debug_assert! is not a production guard — it compiles to nothing in release builds. If correctness depends on a debug_assert!, that's a real finding.
• For consensus code: if two honest validators could produce different results from the same input, that's at minimum High severity.