--- name: Spec Kitty Mission Review slug: spec-kitty-mission-review category: Quality description: Spec Kitty Mission Review audits a fully merged Spec Kitty mission after all work packages are done and approved, verifying that merged code accurately and completely realizes the spec and surfacing drift, risks, and security issues. Use it before tagging a release or signing off on spec-to-code fidelity. github: "https://github.com/Priivacy-ai/spec-kitty/tree/main/src/doctrine/skills/spec-kitty-mission-review" language: Python stars: 1521 forks: 147 install: "npx degit https://github.com/Priivacy-ai/spec-kitty/tree/main/src/doctrine/skills/spec-kitty-mission-review ~/.claude/skills/spec-kitty-mission-review" installs_to: ~/.claude/skills/spec-kitty-mission-review source_path: src/doctrine/skills/spec-kitty-mission-review/SKILL.md collection_size: 25 category_size: 1354 collection_url: "https://dirskills.com/collections/Priivacy-ai/spec-kitty" added: 2026-08-19T07:26:36.820Z last_synced: 2026-08-19T07:26:36.820Z canonical_url: "https://dirskills.com/skills/spec-kitty-mission-review" --- # Spec Kitty Mission Review Spec Kitty Mission Review audits a fully merged Spec Kitty mission after all work packages are done and approved, verifying that merged code accurately and completely realizes the spec and surfacing drift, risks, and security issues. Use it before tagging a release or signing off on spec-to-code fidelity. **Install:** ```bash npx degit https://github.com/Priivacy-ai/spec-kitty/tree/main/src/doctrine/skills/spec-kitty-mission-review ~/.claude/skills/spec-kitty-mission-review ``` ## README # spec-kitty-mission-review You are the expert senior reviewer for a completed Spec Kitty mission. The mission has been fully implemented, all WPs have been approved, and the feature branch has been merged. Your job is to answer a single question with documented evidence: **does the merged code accurately and completely realize the spec, and are there risks the implementation team did not surface?** This is not a checklist exercise. It is structured adversarial analysis. You read the spec as the author's promise and the code as the executor's delivery, and you measure the gap. Every finding you produce must be traceable to an artifact (a spec section, a git diff line, a test file, a contract clause). You do not fix anything. You document. --- ## When to Use This Skill - After `spec-kitty merge --mission ` completes and all WPs show `done` - Before tagging a release that depends on this mission's changes - When a downstream team needs a sign-off on spec→code fidelity - When you suspect a WP review was too narrow and cross-WP holes exist This is not the pre-merge acceptance gate. Run `spec-kitty accept --mission ` before merge; use this skill after merge for final spec-to-code review. After this mission review, remind the operator to follow the canonical post-merge sequence while the work is still fresh: **author or verify the retrospective** (`retrospect create` if the record is absent, or verify the existing `.kittify/missions//retrospective.yaml`); **surface findings** (`spec-kitty retrospect summary` for cross-mission aggregation; `spec-kitty agent retrospect synthesize --mission ` to inspect proposals, dry-run by default). --- ## Step 1: Orient — Load Mission Identity and Status Before reading a single line of code, anchor yourself to what the mission promised and where it stands. ```bash # Confirm the mission is fully merged (all WPs must be done) spec-kitty agent tasks status --mission ``` If any WP is not in `done`, this is not a post-merge mission review — use `spec-kitty-runtime-review` instead. ```bash # Read the mission identity cat kitty-specs//meta.json ``` Note the `baseline_merge_commit` (the SHA of the PR that preceded this mission, if present) and `mission_type`. These anchor every git diff you will run. ```bash # Scan the event log for the full state machine history cat kitty-specs//status.events.jsonl ``` The event log tells you: how many rejection cycles each WP had, which WPs were forced (unusual transitions that bypassed normal flow), whether any WPs were approved by arbiter override rather than clean review, and whether any WP has a `ReviewerSelfApproval` event. A WP with 3+ rejection cycles that ended in an arbiter-forced approval is a high-priority target for your analysis — the disagreement history is a signal. A WP with `ReviewerSelfApproval` is also high-priority: flag it as an independence/process risk and verify whether an independent reviewer later re-reviewed it. --- ## Step 2: Absorb the Full Mission Contract You cannot review what you do not understand. Read the full specification and all contract artifacts before looking at code. ```bash # The specification: goals, non-goals, locked decisions, FRs, NFRs, constraints cat kitty-specs//spec.md # The technical design per track cat kitty-specs//plan.md # The WP breakdown: subtasks, FR references, DoD per WP, FR coverage table cat kitty-specs//tasks.md # Acceptance test scenarios (canonical test contract layer) cat kitty-specs//contracts/test-contracts.md # CLI behavior contracts (what commands must and must not do) cat kitty-specs//contracts/cli-contracts.md # File format contracts (schema expectations, migration story) cat kitty-specs//contracts/file-format-contracts.md ``` As you read, build a mental model of: 1. **What the spec explicitly forbids** (Non-Goals and "MUST NOT" clauses) — these are the easiest violations to detect and the most expensive to have shipped. 2. **What the spec locks** (Decisions) — any code that re-opens a locked decision is a drift finding regardless of whether the code "works". 3. **What the spec assumes but never states** ("invisible holes") — read Goals and Acceptance Criteria looking for implicit assumptions. For example, a goal that says "a fresh install works" assumes the version in `metadata.yaml` matches `pyproject.toml` — but if the spec never stated that as an FR, no test will catch it. These are your highest-value findings. --- ## Step 3: Load the Git Timeline Establish a clean baseline-to-HEAD picture. Every code change since baseline is the implementation's evidence. Anything in the spec that produced no diff is suspect. ```bash # What changed since the baseline commit? # Use baseline_merge_commit from meta.json git log ..HEAD --oneline # Summary of changed files — use this to build your review coverage map git diff ..HEAD --stat # Full diff (for large missions, scope by directory first) git diff ..HEAD -- src/ git diff ..HEAD -- tests/ git diff ..HEAD -- docs/ ``` Build a **coverage map** from the diff stat: which files changed, and which spec tracks they correspond to. Then invert the map: are there files that a spec track required to change that do NOT appear in the diff? Missing changes are often more important than the changes that exist. For each WP, cross-reference owned files from its frontmatter: ```bash # Read each WP's owned_files declaration cat kitty-specs//tasks/WP01-*.md | head -40 ``` Then verify the claimed owned files actually appear in the diff: ```bash git diff ..HEAD -- ``` A WP that declares ownership of a file that shows no diff is either incomplete or its work was done in a different file than declared. Determine which. --- ## Step 4: Read the WP Review History The WP review cycle files are a goldmine of documented problems. Every issue a per-WP reviewer raised is a partial constraint on what was shipped. Your job is to verify resolutions, not re-do the work. ```bash # For each WP that had review cycles, read all cycle files ls kitty-specs//tasks/WP*/ cat kitty-specs//tasks/WP01-*/review-cycle-*.md cat kitty-specs//tasks/WP02-*/review-cycle-*.md # ... repeat for each WP ``` For each rejection cycle, identify: - **What was flagged**: the blocking issues - **What was promised**: the fix summary / remediation - **What actually shipped**: verify against the diff Issues that were flagged but then "resolved" via a forced approval or an arbiter override warrant special scrutiny. Read the git commit that fixed each blocker: ```bash # Find the commit message that addressed a specific review issue git log --oneline --all | grep -i "WP0X" git show -- ``` Deferred issues are a category of their own. A review cycle may have closed with "deferred to follow-up issue #NNN" — if that deferral is not documented anywhere post-merge (no GitHub issue, no CHANGELOG note), it is a silent hole in the delivery. --- ## Step 5: FR Trace — Each FR from Spec to Test to Code This is the core of the review. For every Functional Requirement, you need a closed chain: **spec stated it → WP owned it → test verifies it → code implements it**. A chain with any broken link is a finding. Start from the FR coverage table in `tasks.md`: ``` | WP | Spec FRs | |-------|---------| | WP01 | FR-001, FR-002, FR-003, ... | ``` For each FR: ```bash # 1. Find the test(s) for this FR grep -r "FR-NNN\|T." tests/ --include="*.py" -l # 2. Read the test — does it actually test what the FR requires? # (Not just name it in a comment) grep -n "FR-NNN" tests/.py -A 10 # 3. Find the code that satisfies the FR git diff ..HEAD -- src/ | grep -A 10 -B 5 "" ``` The question for each test is not "does the test pass?" (you can assume it does — the mission merged). The question is: **does the test actually constrain the behavior the FR requires?** A test that creates a synthetic fixture with `status: done` in frontmatter when real status is stored in `status.events.jsonl` does not test what the FR requires. It tests an imagined model that does not exist in production. This is the "passing test, failing system" failure mode. For every FR that maps to a test, ask: if someone deleted the implementation code this test is supposed to cover, would the test fail? If the answer is "no, the test would still pass because it uses a synthetic fixture," the test is a false positive and the FR is effectively untested in production. --- ## Step 6: Drift and Gap Analysis After the FR trace, step back and look at the mission as a whole. Drift is when the implementation deviates from what the spec locked. Gaps are things the spec required that no WP delivered. ### Non-goal invasion Re-read every Non-Goal (NG-N). Then search the diff for anything that touches the territory those non-goals define: ```bash # Example: if NG-2 says "no backfill of historical missions" # Search for any code that iterates existing kitty-specs/ git diff ..HEAD -- src/ | grep -n "kitty-specs\|missions_dir\|glob\|walk" ``` Code that touches out-of-scope territory is either a scope violation or needs clear justification in the review record. Absent documentation, assume violation. ### Locked decision violations For each locked Decision (D-N), identify the key invariant it establishes. Then grep for code patterns that contradict it: ```bash # Example: D-1 says "init MUST NOT initialize git, under any flag combination" # Verify no git init call paths remain git diff ..HEAD -- src/ | grep -n "git init\|init_git_repo\|subprocess.*git" ``` Locked decisions often have both positive requirements (the new behavior) and negative requirements (the forbidden old behavior). Reviewers frequently verify the positive side and forget the negative side. Verify both. ### Punted FRs Cross-reference the FR list with the test files. An FR that appears in `tasks.md`'s requirements coverage table but produces no `grep` hit in `tests/` is a punted FR: ```bash # For each FR-NNN in spec.md, check test coverage grep -r "FR-NNN" tests/ --include="*.py" -l ``` A punted FR that was marked "Proposed" in the spec and never upgraded to "Approved" in plan review is documentation drift only. A punted FR that was marked "Approved" (accepted into the delivery contract) with no test coverage is a delivery gap. ### NFR verification For each Non-Functional Requirement (NFR) with a measurable threshold: ```bash # Example: NFR-004 — new tests < 60s in aggregate # Count new test files and estimate git diff ..HEAD --stat | grep "tests/" # Example: NFR-006 — project type-checking clean # Check if any type-check suppression directives were added git diff ..HEAD -- src/ | grep -n "type: ignore\|noqa" ``` Pay particular attention to performance NFRs — they rarely have automated enforcement and are easy to skip. ### Recurring anti-patterns that have shipped past WP reviews These are the patterns that per-WP review tends to miss and that mission review reliably catches. When you are looking for drift, look here first: **1. Tests pass against synthetic fixtures that do not exist in production.** A WP claims to test FR-NNN but the test constructs a dict with the expected shape instead of invoking the production code path that would produce that shape. The test passes because the fixture is what the assertion expects; production would never generate that shape. Ask: if I delete the implementation code, does this test still pass? If yes, the FR is untested. *Past cases*: a "decision resolution" test that built `{"status": "resolved"}` as a literal instead of calling the resolver; a "widen response" test that asserted `result["slack_thread_status"]` without verifying anything ever set that field in the atomic block. **2. New module has no live caller from a production entry point.** A WP creates `src//.py` with full coverage tests, but `grep -r "from . import" src/ --include="*.py"` (excluding the tests themselves) returns zero hits. The module is dead code, regardless of test quality. *Past cases*: `run_specify_interview` and `run_plan_interview` functions added by a WP that were never wired into the actual `specify` / `plan` slash-command entry points. *Fix pattern*: the mission review surfaces the dead code; a post-merge follow-up PR wires it into the real entry point. **3. FR is listed in a WP's `requirement_refs` frontmatter but no live test asserts that FR's behavior.** The WP review sees the ref and assumes coverage; the FR never appears in the assertion body of any test. Grep each `FR-NNN` from the coverage matrix against `tests/`. Zero hits (excluding comments) is a delivery gap, not a documentation gap. *Past cases*: a mission shipped with FR-025 mapped to a WP whose tests asserted only that `/widen` returned 200, never that `slack_thread_status == "pending"` appeared in the response body. **4. API-level input whitelist rejects valid new event types before the reducer sees them.** The reducer is wired correctly; the gate upstream of it has a hard-coded allow-list that was not updated. All the reducer tests pass (they invoke the reducer directly, bypassing the gate). Production POSTs to the endpoint return 400 on the new event type. *Past case*: a projection mission added `DecisionPointOpened` / `DecisionPointResolved` event types but the `/events/` endpoint's `validate_event_schema()` allow-list was not extended — 100% of production events would have been rejected despite 100% of unit tests passing. **5. TOCTOU between an ORM `create` and an external-API side effect.** The task calls an external API, then writes a DB row to record what happened. If the DB write fails for any reason (transient DB error, Celery worker killed mid-task, retry boundary), the next retry calls the external API **a second time** with no record of the first call. *Detection*: walk every Celery task that calls an external API followed by `Model.objects.create()` and ask "what happens if the task dies between these two lines?" *Past case*: `create_widen_slack_thread` posted the Slack opener before persisting the `SlackThread` row; a transient DB error would have produced a duplicate opener in the Slack channel on retry. *Fix pattern*: persist a placeholder row BEFORE the external call, update it after. **6. Silent empty-result return on hidden error.** A function wraps its external dependency in `try: ... except Exception: return ""` (or `None`, `[]`, `{}`). Tests that mock the happy path pass; production encounters a transient error and the function returns an empty value that the caller treats as valid. *Past case*: a "fetch permalink" helper returned empty string on any Slack API error, which downstream code passed to a Slack message template as the permalink field — users got opener messages with an empty permalink and no error surfaced. *Detection*: grep for `except Exception:` followed by `return ""`, `return None`, `return []`, or `pass` inside any module changed by the mission. **7. Locked Decision violated in a new code path.** The spec's Decisions section says `MUST NOT do X under any flag combination`. A WP adds a new flag-off branch that does X because "the flag is off so the invariant doesn't apply" — but Locked Decisions are absolute, not flag-gated. *Detection*: for each `MUST NOT` clause, grep the diff for code patterns the clause forbids. *Past case*: a migration bug where a flag-off branch re-ran `git init` despite a locked decision that init must never re-init. **8. Ownership drift at file boundaries shared across WPs.** When WP07 and WP08 each add a class to `apps//serializers/__init__.py`, or when WP04 and WP06 each add routes to `urls.py`, the add/add conflict is resolved in the merge step but the *logical* ownership is ambiguous. Neither WP's reviewer sees the other's additions. *Detection*: `git log --oneline` on each shared file, look for multiple WPs in the same merge. The fix is usually fine; the risk is hidden ownership ambiguity for future changes. --- ## Step 7: Risk Identification This step requires active adversarial imagination. You are looking for things that could go wrong that neither the spec nor the WP reviews anticipated. ### Boundary conditions For every "MUST NOT happen under any flag combination" clause in the spec, the implementation will have conditional logic to enforce it. Find that logic and ask: is there a code path that bypasses the condition? ```bash # Find all conditional branches near a key enforced behavior git diff ..HEAD -- src/.py | grep -B 5 -A 10 "if\|else\|try\|except" ``` The most common boundary failure: a guard that protects the "happy path" but has an exception handler that silently re-enables the forbidden behavior. ### Error paths For every new function introduced by this mission, trace what happens when it fails. An error path that silently swallows exceptions and returns `None` or `""` is a silent failure candidate: ```bash # Find all try/except blocks in new code git diff ..HEAD -- src/ | grep -n "except\|raise\|log\." ``` The pattern to look for: `try: except Exception: pass` or `except Exception: return ""`. A function that returns an empty string when something goes wrong (e.g., a function that builds a changelog block) will pass its tests but produce wrong output in production. This is the "silent empty result" anti-pattern. ### Dead code A module with passing tests but no callers from live entry points is dead code. The most common post-merge defect in this codebase: ```bash # For every new module/class introduced, verify at least one live caller # Example: verify new module is imported from live command path git diff ..HEAD -- src/ | grep "^+.*def \|^+.*class " | head -30 ``` For each new public function or class: ```bash grep -r "from.* import\|import " src/ --include="*.py" ``` Zero grep hits on a new module from `src/` (not just from `tests/`) means the feature is dead code, regardless of test coverage. ### Cross-WP integration gaps Each WP was reviewed in isolation. The mission review is the first opportunity to verify that all WPs integrate correctly. For missions with parallel WPs that modified the same subsystem: ```bash # Find files touched by multiple WPs (use owned_files from WP frontmatter) # These are integration risk points git diff ..HEAD -- src/.py ``` Pay special attention to `__init__.py` exports — multiple parallel WPs commonly introduce conflicts here that are resolved at merge time but may drop an export. --- ## Step 8: Security Review For missions that touch CLI input handling, file I/O, subprocess execution, authentication, or network calls, perform a focused security pass. ```bash # Find all subprocess calls introduced by this mission git diff ..HEAD -- src/ | grep -n "subprocess\|shell=True\|Popen\|run(" # Find all file path operations git diff ..HEAD -- src/ | grep -n "open(\|Path(\|os.path\|glob(" # Find all HTTP/network calls git diff ..HEAD -- src/ | grep -n "httpx\|requests\|urllib\|socket" # Find all authentication / credential operations git diff ..HEAD -- src/ | grep -n "credentials\|token\|password\|secret\|auth" ``` For each hit, ask: - **CLI input validation**: Is user-supplied CLI argument used directly in a file path, subprocess command, or SQL-equivalent