# Impl Review > Use when a spec or design document exists and needs quality/security review before implementation. Triggers on: ''review spec'', ''audit spec'', ''check spec'', ''spec review'', ''design review'', ''审查spec'', ''审阅设计文档'', ''检查设计''. Also use proactively after brainstorming generates a spec, before writing-plans, or when a spec was just created and hasn''t been reviewed. - **Type:** Skill - **Install:** `agentstack add skill-shinewinew-lucideye-impl-review` - **Verified:** Yes — security-reviewed for prompt injection and unsafe behavior - **Seller:** [ShinewineW](https://agentstack.voostack.com/s/shinewinew) - **Installs:** 0 - **Category:** [Agent Skills](https://agentstack.voostack.com/c/agent-skills) - **Latest version:** 0.1.0 - **License:** MIT - **Upstream author:** [ShinewineW](https://github.com/ShinewineW) - **Source:** https://github.com/ShinewineW/lucideye/tree/main/skills/impl-review ## Install ```sh agentstack add skill-shinewinew-lucideye-impl-review ``` Requires the [AgentStack CLI](https://agentstack.voostack.com/docs/cli). Works with Claude Code, Cursor, and any MCP-compatible agent. ## About ## Constraints - **No code output.** You never write, generate, or modify code. You only analyze and edit spec documents. - **No code modification.** You never touch source files. Your allowed-tools are Read/Grep/Glob (for project context) and Edit (for spec documents only). - **Evidence-based.** Every finding must cite the exact section of the spec or project context that supports it. No speculation. - **Proportional.** Calibrate effort to spec size. A 50-line feature spec doesn't need a 6-phase formal audit. A system architecture spec does. ## When to Use - After a spec is created, before implementation planning - When a spec or design document needs review before implementation - When you want to validate that a spec is consistent, complete, and safe to build from - Before committing to a large implementation effort ## When NOT to Use - Code review - Reviewing already-implemented code against a spec - Writing or creating specs - Reviewing non-technical documents --- # Review Process ## Effort Calibration Before starting, assess the spec and choose the appropriate depth using BOTH size and risk: **Step 1 — Size baseline:** | Spec Size | Baseline Depth | |-----------|---------------| | 10 commits | | **Full** | All phases | Written, detailed | Explicit trace, all sub-phases, external verification | Always | --- ## Phase 1 — Context Gathering Collect two things: the spec itself, and the project it will live in. **Spec input:** - Read the spec document (user provides path or it's the most recent file in `docs/` or `specs/`) - Accept any format: Markdown, PDF, plain text, Notion export **Project context** (if available — skip if standalone spec): - Read the applicable project-instruction chain (such as `AGENTS.md` or `CLAUDE.md`) and agent-native rule skills for project conventions and constraints - Scan directory structure to understand existing architecture - Read relevant existing modules/interfaces the spec mentions or depends on **Domain documentation** (if present — these are first-class inputs, not optional reading): - `CONTEXT.md` at repo root (single-context project) — domain glossary, ubiquitous language, bounded context definition. Use the same vocabulary throughout the review; flag spec terms that drift from it. - `CONTEXT-MAP.md` at repo root (multi-context project) — points to per-context `CONTEXT.md` files under each module. Identify which context(s) the spec touches and load the matching `CONTEXT.md`. - `docs/adr/` directory (or per-context `docs/adr/`) — architecture decision records. Index them by ADR number/title before Phase 5. Each ADR represents a decision already debated; the spec must either align with it or explicitly propose superseding it. If these don't exist, fall back to reading what's there. Do not invent them. **Temporal gap check:** If the spec references a specific commit hash, date, or branch state, immediately check how far the codebase has moved since then. Run `git log --oneline ..HEAD` (or compare dates) to measure the gap. A spec that reviewed commit `abc1234` when HEAD is 40 commits ahead is a fundamentally different review target than a spec written against the current HEAD — every factual claim in the spec becomes a hypothesis that may have been invalidated by intervening changes. When the gap is significant (>10 commits or >1 week), treat this as a **staleness risk** that colors all subsequent phases: every claim the spec makes about "current state" must be independently verified, not taken on faith. This check takes 30 seconds and can save the entire review from being anchored on false premises. **Baseline independence:** If the spec was developed in the current conversation session, or if the spec's problem statement describes a "current state" that was analyzed earlier in the conversation, you MUST re-verify the baseline independently. Do not reuse filesystem observations, command outputs, or state assessments from earlier in the conversation — they may be wrong or outdated. Run fresh verification commands (`ls -la`, `readlink`, `cat`, etc.) as part of Phase 1, treating the spec's claims about current state as hypotheses to verify, not facts to inherit. > Example: A command may follow links and hide the filesystem property the spec relies on. Re-check with a command that exposes that property before accepting the baseline. Output: a mental model of "what the spec says" and "what the project already is." Keep this lightweight — you're gathering context, not writing a report. --- ## Phase 2 — Spec Intent Extraction (Spec-IR) This is the core analytical step. Transform the natural language spec into structured intent records. This process forces hidden assumptions to the surface — which is where most spec bugs live. For each significant claim in the spec, extract: ```yaml id: SPEC-NNN excerpt: "exact quote from spec" section: "section heading or location" type: actor | flow | invariant | constraint | assumption | security-req | interface | data-model | error-handling | dependency normalized: "what this actually means, stated precisely" confidence: 0.0-1.0 # how unambiguous is this claim? implicit_assumptions: ["assumptions the spec doesn't state but relies on"] ``` **What to extract:** - **Actors & roles**: who interacts with the system, what permissions they have - **Data flows**: what data moves where, through what boundaries - **Trust boundaries**: where trusted/untrusted transitions happen - **Invariants**: things that must always/never be true - **State transitions**: valid sequences of operations - **Security requirements**: authentication, authorization, encryption, audit - **Error conditions**: what can go wrong and what should happen - **Dependencies**: external systems, libraries, APIs the spec assumes exist - **Implicit assumptions**: things the spec takes for granted without stating For **Light** reviews, you can do this mentally without writing formal IR. For **Standard** and **Full** reviews, you **must** write out the Spec-IR records in your output — the act of writing forces precision and exposes gaps that mental analysis misses. Include at least the key records (actors, security requirements, critical flows). Omitting IR at Standard/Full depth defeats the purpose of structured analysis. --- ## Phase 3 — Self-Consistency Analysis Compare Spec-IR records against each other. Look for: ### 3a. Internal Contradictions Two records that cannot both be true. Pay special attention to **numerical consistency** — tables, counts, totals, and statistics that don't add up are a common and easy-to-miss contradiction. > Example: Section 2 says "all API calls require authentication" but Section 5 describes a public health-check endpoint without mentioning auth exemption. > Example: A classification table shows A=40, B=6, C=8, D=5 (total 59), but a footnote says "B and D don't overlap" — implying they could overlap, which would change the total. If the numbers work without overlap, the footnote is misleading; if there is overlap, the total is wrong. **Narrative vs verified reality:** After Phase 1 context gathering (which may include filesystem verification), compare the spec's title, problem statement, and core framing against what you actually observed. If the spec says "migrate from X to Y" but the system is already in state Y, the entire narrative is wrong — and the implementation plan built on that narrative will be wrong too. This is not a minor wording issue; a false premise propagates through every design decision. Concrete verification steps (do these, don't just reason about them): - If the spec claims files have specific content, **read those files** and compare - If the spec claims tests fail, **run those tests** and check the actual result - If the spec claims a deployment topology, **ls the actual directories** - If the spec cites specific line numbers, **verify those lines still contain what the spec says** - If the spec claims something is "broken" or "missing", **check if it was fixed since the spec date** > Example: A migration spec describes moving from state X to state Y, but direct verification shows the system is already in state Y. The problem statement and migration steps are therefore built on a false premise. > Example: A remediation spec cites failing tests, but running the current test suite shows they now pass after intervening changes. The remediation plan may no longer be needed. **Goals vs own caveats:** Check whether the spec's goal-state descriptions or guarantees are contradicted by its own risk, assumption, or limitation sections. Specs often write aspirational goals first ("zero data loss", "automatic for all projects") and then add caveats later ("platform assumption: may not work for new projects"). If the caveat means the goal is unachievable, the goal section must be qualified — otherwise implementers and reviewers will rely on the unqualified claim. > Example: The goal promises automatic coverage for every newly created resource, while the risk section acknowledges that resources created after initialization are not covered. The goal contradicts the acknowledged limitation. **Guarantee claims under concurrency:** When a spec makes a strong guarantee ("zero loss", "no downtime", "exactly-once", "always consistent"), don't accept the guarantee at face value. Verify it holds under ALL known concurrent actors in the system, not just the single path the spec analyzes. If the system has competing consumers, parallel workers, or async background tasks that touch the same state, check whether the guarantee accounts for their interference. > Example: A spec claims zero loss and proves it for the primary consumer, but a secondary consumer can hold an in-flight item when shutdown begins. The guarantee is overstated unless that parallel path is also covered. **Name/semantics consistency:** When a spec introduces a new field, counter, or state variable, verify that its name implies the correct lifecycle. "Lifetime" implies surviving restarts; "persistent" implies disk storage; "global" implies cross-process. If the actual mechanism is an in-memory field that resets on process restart, the name is misleading and will cause implementers to rely on guarantees the mechanism can't deliver. > Example: A field is described as a lifetime counter but is stored only in memory and resets when its runtime state is recreated. The name promises persistence the mechanism cannot deliver. ### 3b. Ambiguity Records where the same text can be reasonably interpreted two different ways, leading to different implementations. > Example: "Users can access their own data" — does "their own" mean data they created, or data about them that others created? ### 3c. Circular or Unresolvable Dependencies Component A depends on B, B depends on C, C depends on A — or a dependency that doesn't exist yet with no plan to create it. ### 3d. Completeness Gaps Things the spec *must* address given its scope but doesn't: | If the spec describes... | It should also address... | |--------------------------|--------------------------| | User-facing API | Authentication, rate limiting, error responses | | Data storage | Retention policy, backup, migration | | Multi-step workflow | Failure/retry at each step, partial completion | | External integration | Timeout, fallback, version pinning | | Concurrent access | Locking strategy, conflict resolution | | Sensitive data | Encryption at rest/transit, access audit, PII handling | | Multiple actors/roles | Each actor has a defined authentication flow | | Phased execution plan | Each phase's rollback is independent of later phases | | Downloaded binaries/deps | Integrity verification (checksum/signature) | | Polling loop / consumer loop | Every branch path yields control (await/sleep); what happens when a message is rejected mid-loop (claim→reject→retry cycle) | | Shared queue with filtered consumer | Filter mechanism (source-level query vs post-claim check); post-claim filter + retry = potential busy loop if the same message is re-claimed | | Competing consumers on shared state | Not just multi-consumer races, but single-consumer self-loops (claim→can't process→release→re-claim same item) | | Component migration/rename | All references updated (slash commands, table entries, prose text, description fields); governance checkpoints preserved; runtime dependencies still resolve; rollback covers every destructive step | | Validation/verification step | Checks actually catch the failures they claim to prevent (grep patterns match all residual forms, not just one) | | Fix/patch for a specific code path | Whether the fix interacts with parallel code paths that handle the same data differently — e.g., one path already paginates at the SQL layer while another paginates at the application layer; applying the same pagination to both causes double-application. When a spec proposes multiple alternative implementations ("approach A or approach B"), verify they are compatible; if mutually exclusive, the spec must choose one | | Decision justified by external rule/standard | Source is identified with enough specificity (full file path or URL) for a reader unfamiliar with the project to independently locate and verify the cited rule | | Error detection / catch-based recovery | The assumed error actually occurs on the described trigger path. Trace the code from trigger to catch: does the path pass through auto-creation layers (`ensureDir`, `mkdirSync({recursive})`, `CREATE IF NOT EXISTS`), retry wrappers, or fallback constructors that would swallow or prevent the error before it reaches the catch? A catch block that never fires is a safety net with no net. | | Refactoring / code extraction | When the spec extracts, reorganizes, or replaces a code region, ALL branches in the original region are accounted for — not just the ones the spec names. Read the actual source lines being refactored; specs often describe 3 of 5 branches, and the 2 unnamed ones silently break. Adjacent logic (within ~20 lines of the described region) is especially likely to be missed. | ### 3e. Vague Specification Language that gives implementers too much freedom in security-critical areas: - "appropriate security measures" — what specifically? - "should handle errors gracefully" — what does graceful mean here? - "may optionally support" — will it or won't it? - "similar to X" — in what exact ways? --- ## Phase 4 — Security Design Review (STRIDE) Apply STRIDE threat categories to the spec's design, not to code. For each significant component or data flow described in the spec, ask: | Threat | Question for the Spec | |--------|----------------------| | **Spoofing** | Does the spec define how actors prove their identity? Can one actor impersonate another given this design? | | **Tampering** | Does the spec protect data integrity in transit and at rest? Can messages/data be modified between components? | | **Repudiation** | Does the spec include audit trails? Can actors deny performing actions? | | **Information Disclosure** | Does the spec control who sees what? Are there data flows that cross trust boundaries without encryption/filtering? | | **Denial of Service** | Does the spec address resource limits? Can any actor exhaust system resources? | | **Elevation of Privilege** | Does the spec enforce least privilege? Can any actor gain permissions beyond their role? | **Also apply Sharp Edges analysis** to any API or configuration interface described in the spec: - Are defaults secure? - Can the "easy path" lead to insecurity? - Are there dangerous configuration options without validation? - Can parameters be confused or swapped? For **Light** reviews, do a quick mental STRIDE pass — mention any relevant threats in your findings but don't produce a formal table. For **Standard** and **Full** reviews, you **must** produce a STRIDE summary table in the output showing which threats apply and which are adequately add … ## Source & license This open-source skill is cataloged on AgentStack and links to its original source — we do not rehost the code. - **Author:** [ShinewineW](https://github.com/ShinewineW) - **Source:** [ShinewineW/lucideye](https://github.com/ShinewineW/lucideye) - **License:** MIT Install and usage instructions live in the source repository linked above. ## Pricing - **Free** — Free ## Security capabilities Automated source analysis of v0.1.0 — what this tool can access: - **Network access:** no - **Filesystem access:** no - **Shell / process execution:** no - **Environment & secrets:** no - **Dynamic code execution:** no *"Yes" means the capability is present in the source — more access means more to trust, not that it is unsafe.* ## Versions - **0.1.0** — security scan: passed — Imported from the upstream source. ## Links - Listing page: https://agentstack.voostack.com/l/skill-shinewinew-lucideye-impl-review - Seller: https://agentstack.voostack.com/s/shinewinew - Browse the marketplace: https://agentstack.voostack.com/browse --- Listed on AgentStack — the marketplace for AI agent skills and MCP servers. Every listing is security-reviewed. Creators keep 70%.