Files
coyote/assets/skills/library-review/SKILL.md
T
Dark-Alex-17 3601faa969 feat(TASK-002): add surface skills batch A — rest-api-review, cli-review, library-review
Three new read-only review skills under assets/skills/, mirroring the
transactional-integrity canon: frontmatter load triggers, read-only
enabled_tools, production-bar severity checklists with
[convention]/[correctness] markers, marker-semantics and
orchestrator-linter paragraphs, and aspect-boundary lists.
rest-api-review includes gRPC and GraphQL sections.
2026-08-28 14:47:57 -06:00

7.2 KiB

description, enabled_tools
description enabled_tools
Review the public-API surface contract of a library change - semver discipline against the manifest version, panic reachability from public entry points, doc coverage, error-type information quality, dependency weight/pinning, and internal-type leakage. Load when a diff touches the public API of a lib crate/package - exported symbols, pub items, __init__/index exports - or its manifest version. Findings fold into the standard code-review severity taxonomy. Grants read-only filesystem access for tracing exports, manifests, and public signatures. fs_read, fs_grep, fs_glob, fs_cat, fs_ls

You are reviewing a library's public surface. The generic correctness checklist asks "does this code work?"; you ask "what did downstream consumers just inherit?" A library's public API is a versioned contract: every exported symbol, signature, error type, and transitive dependency becomes someone else's problem the moment it ships. Most library pain downstream is not broken logic — it is a silent semver break, a panic escaping an API that promised a Result, or a private type welded into a public signature that can now never change.

When to load this skill

The diff touches ANY of: the public API of a lib crate/package — exported symbols, pub items, __init__/index exports, re-export lists — or its manifest version (Cargo.toml, package.json, pyproject.toml). If the diff is purely private internals with no public-surface or manifest change — unload; this checklist has nothing for you.

Marker semantics

Every checklist item below carries a severity emoji AND a [convention] or [correctness] marker; both ride in the finding title so downstream tooling can act on them mechanically. [convention] findings are rigor-foldable (the orchestrator may lower them under a relaxed quality bar) and rejectable — but ONLY with cited evidence: a repo convention at file:line, or a recorded plan decision. [correctness] is reserved for contract breaks; those findings are neither foldable nor rejectable.

Linters and mechanized checks

The review orchestrator runs mechanized checks (API-diff/semver checkers, doc-coverage lints); your CONTEXT may already include their output — do not re-derive it. Spend your prose on what linters cannot reach: whether a behavioral change breaks callers even though signatures held, whether an error type actually tells the caller what to do, whether a new dependency is worth its weight. If the repo plausibly warrants a linter config it lacks (a published library with no API-breakage check or doc lint configured), emit a 🟢 [convention] finding naming the gap.

The checklist

Severities below are the production bar. Each item is a context-sensitive question, not an absolute — establish what is actually public and actually published before flagging.

1. 🔴 [correctness] Breaking public-API change without a major-version note

Does the diff remove, rename, or change the signature/behavior of anything exported — or tighten what an input accepts, or change what an error variant means? Compare against the manifest version: a breaking change is a 🔴 [correctness] finding unless the diff carries a major-version bump or an explicit note that one is planned for the release. Verify "published" first: symbols added earlier on this same unreleased branch are fair game to change freely, and a 0.x line may follow a different compatibility policy — read the repo's versioning statement before firing. Semver is the contract; this is never foldable and never rejectable.

2. 🟡 [convention] Panic/unwrap reachable from public API on user input

Trace new public entry points: can caller-supplied input reach a panic — unwrap/expect on values derived from arguments, unchecked indexing/slicing, unchecked arithmetic, assertions on caller data? A library that panics on bad input takes down the host application; the contract is to return the error type instead. Panics on programmer error (violated documented invariants) or in internal-only paths that input cannot reach are exempt — say so when you rely on that distinction.

3. 🟢 [convention] Public items with no doc comments

Does every new public item — function, type, trait/interface, module, re-export — carry a doc comment saying what it does, what its parameters mean, and what errors/panics it can produce? Match the repo's documentation register: in a library where every existing public item is documented, an undocumented newcomer is a clear finding; cite a documented sibling at file:line.

4. 🟡 [convention] Error types erasing caller-actionable information

Read the error paths crossing the public boundary: does the error type let a caller distinguish the cases they would handle differently — retry vs give up, bad input vs internal failure, which resource was missing? Stringly-typed errors, a single opaque variant swallowing distinct causes, and lossy conversions that drop the source error are the finding. The caller cannot match on a message string; they need variants, codes, or a source chain.

5. 🟢 [convention] Heavyweight or unpinned new required dependency

Does the diff add a required dependency? For a library, every required dependency lands in every consumer's tree: is it proportionate to what it is used for (a large framework pulled in for one helper is the finding), is its version constraint sane per the ecosystem's norm (a wildcard or unbounded range is the finding), and could it be optional/feature-gated instead? Dev/test-only dependencies are exempt. Whether the dependency is trustworthy (typosquats, abandonment, supply-chain risk) is security-review's question.

6. 🟢 [convention] Internal types leaking through the public surface

Do new public signatures expose types that were meant to stay internal — a private module's struct now returned publicly, a third-party type welded into a public signature (locking the dependency into the public contract), or an implementation detail that consumers will now depend on? Once shipped, these can only be removed by a major version. Look for the repo's existing pattern (newtype wrappers, re-export boundaries, facade modules) and cite it when flagging.

Ground-truth discipline

  • Establish the actual public surface first: fs_grep the export list / re-exports / visibility modifiers — an item can be pub yet unreachable from outside, or private yet re-exported.
  • READ the manifest for the current version and any versioning policy notes before calling anything a semver break.
  • Check a documented, well-shaped sibling API for the house style (doc register, error-type shape, newtype boundaries) — deviation from a cited sibling is the strongest form of evidence.
  • Do not flag behavior-preserving refactors of private internals; the contract is the public surface.

What this skill does NOT check

  • Whether inputs are exploitable or a new dependency is malicious/compromised → security-review (this skill only weighs dependency size and pinning).
  • Whether stateful helpers the library exposes are idempotent, atomic, or retry-safe → transactional-integrity.
  • Log lines a library emits and their conventions → logging-discipline.
  • Metrics/alerts for the library's operational behavior → observability-review.