path: root/.ai/workflows/spec-review.org

#+TITLE: Spec-Review Workflow
#+AUTHOR: Craig Jennings
#+DATE: 2026-05-23
#+STARTUP: showall

* Overview

The spec-review workflow evaluates a feature/specification document before implementation and decides one thing: can an engineer implement it confidently, test it thoroughly, and ship behavior that matches the user's mental model? If yes, say so and stop. If no, write a review file next to the spec naming every blocking gap and the concrete change that closes it.

This is the *reviewer* side of a pair. Its counterpart is the spec-response workflow, which the spec's author runs to fold a review back in. The contract between them is the review file: =<spec-basename>-review.org= (e.g. =docs/issue-query-spec.org= → =docs/issue-query-spec-review.org=). spec-review produces it; spec-response consumes it.

The goal is not to prove the spec is clever. It is to leave the implementer with *fewer* hidden decisions, not more prose.

* Problem We're Solving

Without a disciplined review before implementation:

- *Specs look finished but force implementers to invent product behavior.* Ownership, conflict, empty-state, and error semantics get discovered (and decided ad hoc) mid-implementation, where they're expensive to change.
- *API assumptions ship unverified.* A spec quietly depends on a field/mutation/enum shape that turns out wrong, and the work stalls at integration.
- *Reviews are vague or sprawling.* "Add tests," "consider edge cases," or a full rewrite of the spec — none of which an implementer can act on.
- *Deferred decisions live only in chat.* The next session re-litigates them.

*Impact:* viability and consistency problems that should surface in review instead surface during implementation, when they cost the most.

* Exit Criteria

A review is complete when:

1. *The implementation-readiness gate has been evaluated* and a rubric label assigned (=Ready= / =Ready with caveats= / =Not ready= / =Needs research=).
2. *If ready:* the user is told plainly ("This spec is implementation-ready. I have no further blocking review notes."), and the review stops — no churn for its own sake.
3. *If not ready:* a =<spec>-review.org= file is written next to the spec, in the standard structure, with every finding specific and actionable (current behavior named, risk explained, change recommended, blocking-or-not stated).
4. *The spec's review history is updated* with who reviewed it, when, which iteration it was, what changed or was recommended, and why.
5. *Deferred work is logged* to =todo.org= (v1 = =[#B]=, vNext/someday = =[#D]=), not left only in chat.
6. *Implementation tasks are enumerated* — the spec's =Implementation phases= section is lifted into a drop-in =todo.org= block (one entry per phase plus a test-surface entry), or, if the spec has no phase decomposition, that gap is raised as a finding.

*Measurable validation:* after reading the review, an implementer knows exactly what to decide or change before starting, and an author running spec-response can give every finding an accept/modify/reject without guessing what it means.

* When to Use This Workflow

Trigger when:

- A spec is up for implementation and someone needs to know if it's ready.
- A spec changes materially (significant design conclusions changed → fresh review round).
- Craig says "review the spec" / "is this spec implementation-ready?" / "let's run the spec-review workflow."

Run it *early* — design review exists to catch viability problems and costly mistakes before implementation, not after.

* Precondition: spec filename

Before Phase 1, verify the file under review ends with =-spec.org=. Every design, decision, or planning document under a project's =docs/= directory carries that suffix as its identifier. The =.org= extension alone is not enough because =docs/= holds non-spec org files too (tutorials, frozen inventories, reference material).

If the file does not end with =-spec.org=, stop immediately and surface the mismatch:

#+begin_example
The file <path> does not end with -spec.org. Either the wrong file was named, or
the file should be renamed first. Spec workflows require the -spec.org suffix as a
guard against pointing the workflow at tutorial, inventory, or setup docs.
#+end_example

The user resolves the mismatch (rename the file, or point at a different one) and re-invokes the workflow. Do not proceed with the review against a misnamed file.

Anywhere the rest of this workflow refers to "the spec," "the spec file," or "the file under review," the path is the =-spec.org= file confirmed here.

* Approach: How We Work Together

** Phase 1: First-pass readiness gate

This is a fast triage on the spec's face — can it be rejected as obviously not ready before you invest in the full read? It is not the final word: several items below (architecture fit, API verification, integration points) can only be judged after Phase 2's code read, so Phase 3 re-runs this gate as the authoritative pass. Here, decide what you already can, and flag the rest to confirm after reading.

Mark the spec implementation-ready only if *all* of these hold:

- The user problem and target behavior are clear.
- Scope is explicit: v1, out-of-scope, and vNext are separated.
- All prior review questions/recommendations are answered or intentionally deferred.
- The expected UX is concrete enough to implement without guessing.
- The data model, state ownership, persistence, and sync behavior are defined.
- Error, conflict, empty-state, and partial-failure behavior are defined.
- Security and privacy behavior is defined wherever the feature touches credentials, personal or shared data, or external services — or there is nothing sensitive to define and that's stated.
- Observability is defined: how the user (or operator) sees what the feature is doing and how failures surface — or the feature is simple enough not to need it, and that's stated.
- Performance expectations and likely bottlenecks are considered.
- The architecture fits the existing codebase and names its integration points.
- The test strategy covers unit, integration/fixture, regression, and user-flow as appropriate.
- The plan can be phased without shipping broken intermediate states.
- External API assumptions are verified or explicitly listed as prerequisites.

If all true → tell the user it's ready and stop unless they ask for more. If any false → continue and write the review file. A "ready" at this phase is provisional; confirm it at Phase 3 after the code read.

** Phase 2: Required reading order

Never review a spec in isolation.

1. *Read the existing implementation first.* The code paths the spec would touch: public commands and entry points, internal helpers/boundaries, current data representation, persistence/write-back, async/sync, caching, error handling, existing tests, naming/style. Capture current-state facts with function names and file paths. Don't recommend designs that ignore how the package works today.
2. *Read related specs and task tracking.* Companion specs, relevant =todo.org= tasks, README/testing docs, prior review files. Record which tasks the spec absorbs, which stay separate, which decisions are already made, which are still open.
3. *Read the target spec end to end — twice.* First for its problem/behavior/phases/assumptions; second looking only for gaps. The second read asks: "What would an implementer still have to invent?"

** Phase 3: Re-run the gate (authoritative)

After reading code and spec, re-run the Phase 1 gate — this is the pass that counts, because now you can actually judge the items that needed the code: architecture fit, API verification, integration points. If now ready, don't manufacture churn. If not, write the review file.

** Phase 4: Evaluate across dimensions

Work the spec against these. Each is a source of concrete findings, not a box to tick.

- *User problem & mental model.* Real problem or just machinery? Does UX match how users think? Predictable names/commands/prompts/layouts? Is editable-vs-generated content obvious? Are destructive/workspace-mutating actions explicit and confirmed? For org features, be *strict about ownership* — users treat visible org text as editable unless told otherwise.
- *Scope & boundaries.* What's v1 / out-of-scope / vNext? Are deferrals captured (=todo.org=)? Does the phased plan avoid half-working states? If a decision is deferred, is v1 still coherent without it?
- *Requirements clarity.* Each requirement unambiguous, testable, traceable to a source (user ask / bug / API capability / decision)? Are "good/fast/intuitive/smart/seamless" replaced with concrete behavior? Terms used consistently? Infeasible/unverified requirements flagged?
- *Data model & ownership.* Durable IDs vs display-only names? What's local/remote/generated/cached/user-authored? Who owns each editable region? What's replaced on refresh, survives view-switch / restart? What if one remote object could appear in multiple local places? Prefer stable IDs for execution, names for display; don't overload generic org properties (=:ID:=) for app-specific remote IDs unless intentional.
- *Sync, refresh, conflict.* What triggers sync (command / save hook / state hook / timer) and refresh? One-way or bidirectional? No-op detection? Remote-conflict detection? v1 conflict behavior? Dirty-buffer behavior? API success but GraphQL-level failure? Network failure mid-pagination/mutation? For v1, simple is fine if explicit: detect, refuse, tell the user what to do next.
- *API & external dependencies.* Which fields/mutations/filters/enums are assumed, and are they verified against current schema/docs/live responses? Permission differences, rate limits, pagination, nulls, partial errors handled? Any optional external tool — what's the fallback? Don't let a spec silently depend on an unverified API shape.
- *Architecture & maintainability.* Fits current module boundaries, or should new ones precede growth? Which parts are pure vs side-effecting? Are transport/parsing/normalization/rendering/command-orchestration separated enough to test? Will it multiply special cases? Backward-compat preserved where needed? Favor small pure cores surrounded by thin IO/command layers.
- *Robustness & error handling.* Expected failure modes? Errors distinguishable from empty-but-successful results? Actionable messages? Retries? Stale/missing caches? Can malformed local files or missing properties crash commands? nil/null/missing-field behavior defined? Data-loss-by-default avoided? Anything that writes files or remote data needs a clear data-loss story.
- *Performance & scale.* Expected counts (issues/comments/labels/teams/projects/views)? Server-side filtering where possible? Bounded, visible pagination? Cached name→ID lookups? Sync calls in the command path acceptable? Could a save hook or whole-file scan make N network calls? Rendering linear? Full-file rewrites avoided? Identify UI freezes, repeated network calls, unbounded pagination — without premature optimization.
- *Security & privacy.* API keys safe? Debug logs leaking secrets or private issue text? Confirmations before mutating shared workspace objects? Personal vs shared distinguished? Local files holding sensitive descriptions/comments? Anything to redact from messages/logs? Any work-tracker integration may handle private company data.
- *UX & accessibility.* Discoverable commands? Recoverable mistakes? Prompts ordered to the task? Safe, useful defaults? Informative-not-noisy status messages? Does the UI avoid implying unsupported actions are supported? Match the upstream product's permissions/concepts? For Emacs packages, command names, completion candidates, buffer layout, and message wording *are* the UX.
- *Test strategy.* Characterization tests before behavior changes? Pure functions to unit-test? API responses needing fixtures? Command flows needing stubs? Regression tests for prior bugs? Boundary/error cases? What's covered elsewhere and shouldn't be re-tested? Which existing tests must change? Prefer tests that lock contracts: representation shape, query compilation, sync no-op, conflict refusal, pagination, dirty-buffer protection.
- *Observability & operations.* How does a user see what the package is doing? Progress messages for long ops? Useful, safe debug logging? A way to inspect/clear caches? Verify setup/API connectivity? For generated org files, headers should often carry source, filter/view name, refresh time, count, truncation state.
- *Documentation & migration.* README/config changes? New commands and defcustoms documented? Old behaviors removed/renamed/migrated? Breaking changes acceptable? Changelog note needed? Don't ship with stale examples.

** Phase 5: Write the review file

Use this structure for =<spec-basename>-review.org= unless the spec calls for something different:

#+begin_src org
,#+TITLE: Review: <Spec Title>
,#+AUTHOR: <reviewer>
,#+DATE: <date>
,#+STARTUP: showall

,* Scope reviewed
What code, tests, docs, and specs you read.

,* Implementation-readiness
Whether the spec is ready. If not, summarize the blockers.

,* Overall assessment
The short senior-engineering read: what's right, what's risky, what must be clarified.

,* High-priority findings
Concrete headings. Each: why it matters and what to change.

,* Medium-priority findings
Important improvements that shouldn't block all progress.

,* UX observations
,* Architecture observations
,* Robustness and performance observations
,* Test strategy recommendations
Specific test cases, not generic "add tests".

,* Suggested spec edits
Concrete edits to make the spec implementation-ready.

,* Agreed decisions
Answers reached during review. Omit if none.

,* Open questions
Only questions that truly block or materially affect implementation.

,* vNext candidates
Deferred features to capture in task tracking.
#+end_src

** Phase 6: Assign the rubric and update tracking

Assign one label consistently:

- =Ready= — no blocking open questions; implementation can start.
- =Ready with caveats= — can start if the caveats are accepted and tracked.
- =Not ready= — blocking ambiguity / missing decisions would force implementers to invent product behavior.
- =Needs research= — external API/library/platform assumptions must be verified first.

The most useful reviews move a spec from =Not ready= to =Ready with caveats= or =Ready= once decisions are captured.

Finding severity maps to blocking power: *high-priority findings block =Ready=* — they hold the rubric at =Not ready= (or =Ready with caveats= if the author accepts and tracks them) until dispositioned; *medium-priority findings are the author's discretion* and don't block. State the blocking status on each finding so the author running spec-response knows which ones gate the rubric.

Then update the spec's review history. Specs should carry a bottom section named =Review and iteration history= (or the nearest existing equivalent) that tracks each material author/reviewer pass. Add a concise entry for this review even when the spec is ready and no review file is written.

Each entry is an org subheading with a compound id followed by three body fields.

Heading format: =YYYY-MM-DD Day @ HH:MM:SS -ZZZZ — Contributor — Role=

The timestamp + contributor + role concatenate as an opaque id — nothing more should be read into it. Timestamp matches the project's todo-format event-log convention and gives natural temporal sort without implying any decision ordering. Contributor uses short parenthetical project context where session matters (e.g. =Claude Code (linear-emacs)=, =Claude Code (rulesets)=, =Codex=, =Craig Jennings=). Role can be compound (e.g. =reviewer + responder=) when one pass fused multiple roles. Author, reviewer, responder, verifier, and researcher are the standard role names.

Body fields:

- *What changed or was recommended:* high-signal summary, not a duplicate of the whole review.
- *Why:* the decision pressure or rationale that caused the contribution.
- *Artifacts:* links to the review file, response/disposition section, commits, task IDs, or source checks when useful.

If the spec has no such section, add it at the bottom. Keep the history short and cumulative; it is provenance for future readers, not a session transcript.

*Emit implementation tasks (drop-in for =todo.org=).* Read the spec's =Implementation phases= section and turn it into a paste-ready block in the review file, under a heading =Implementation tasks (drop-in for todo.org)=. One =** TODO= entry per phase, plus a final entry for the test surface. The point: the handoff to whoever implements is one paste, not a re-read of the spec, and a spec that can't be decomposed into phases fails this step, surfacing a shape problem before =Ready=.

Per-phase entry, following =todo-format.md= (terse heading names the phase; body holds the one-line deliverable plus a pointer back to the spec; tags on the heading):

#+begin_example
** TODO [#B] <phase name — smallest noun phrase> :feature:
<what this phase delivers, one line>. Spec: [[file:<spec path>]] (Implementation phases, phase N).
#+end_example

Final test-surface entry, mirroring the spec's =Acceptance criteria= when present:

#+begin_example
** TODO [#B] <feature> — test surface :test:
Unit: <...>. Integration: <...>. E2e / manual-verify: <acceptance criteria as checkable items>. Spec: [[file:<spec path>]] (Acceptance criteria).
#+end_example

Priority and tags follow the deferred-work rule below. Emit the block in the review file; the author pastes it into =todo.org= during spec-response, or you log it directly when you're also closing the loop. If the spec has no =Implementation phases= section, don't invent one — that absence is the finding, and the step becomes the prompt to ask the author to add a phase decomposition before the spec can be =Ready=.

Then log deferred work to =todo.org=: v1 implementation = =[#B]= (unless urgent or speculative); vNext/someday = =[#D]=. Tag =:feature:= / =:bug:= / =:refactor:= / =:test:= / =:quick:= / =:solo:= only when accurate. Don't leave important deferred decisions only in chat.

* Principles to Follow

** Read the code before critiquing the spec
A recommendation that ignores how the package works today is noise. Cite function names and file paths; ground findings in current behavior.

** Specific over vague
Name the current behavior or spec text, explain the risk, recommend the change, say whether it blocks. Avoid vague preferences, whole-spec rewrites, architecture astronautics, unbounded "consider" lists, and questions whose answer doesn't change implementation.

Model finding:
#+begin_quote
The spec says fetched comments render as subheadings but doesn't define whether editing an existing comment syncs back. That's risky because Linear only allows users to edit their own comments. V1 should treat fetched comments as remote-owned display content and support only adding new comments; editing own comments can be vNext.
#+end_quote

** Decide readiness first, critique second
The gate is the product of the review. A long critique on a spec that's actually ready just creates churn; a "looks fine" on one that isn't ships hidden decisions.

** Preserve iteration provenance
Future reviewers and implementers need to know not just the current decision, but how the spec got there: how many review/response loops happened, who contributed, what they changed or recommended, and why. Keep that record in the spec itself under =Review and iteration history= so the trail survives deleted review files, chat loss, and agent handoffs.

** Be strict about ownership
Especially for org-mode features: a user treats visible text as editable unless the representation says otherwise. Make generated-vs-editable explicit.

** Never depend on an unverified API shape
If the spec assumes fields/mutations/enums, they're verified against current schema/docs/live responses, or listed as a research prerequisite. =Needs research= is a real, useful verdict.

** Favor small pure cores and thin IO layers
Push findings toward separable, unit-testable pure functions surrounded by thin command/transport layers.

** Block only on answer-changing questions
Open questions in the review are the ones whose answer materially changes implementation. Everything else is a suggestion, not a blocker.

* Living Document

Update this workflow as review practice sharpens — new dimensions worth checking, better model findings, refinements to the readiness gate and rubric.

* Research notes behind this checklist

Combines the review process used on the linear-emacs issue query/representation specs with broader design/spec-review practice:

- Design reviews should happen early enough to find viability problems and costly mistakes before implementation (Google design-review research).
- Product/spec docs should create shared understanding — goals, assumptions, and an explicit "what we're not doing" (Atlassian PRD guidance).
- Review sessions should capture open questions and gaps so they're tracked and resolved (Atlassian design-review template).
- Requirements should be checked for correctness, ambiguity, completeness, consistency, verifiability, traceability, feasibility (SRS checklist categories).
- Design-doc review should be iterative but restart/refocus when significant conclusions change (Gerrit design-doc guidance).

Sources:

- [[https://research.google/pubs/improving-design-reviews-at-google/][Google Research: Improving Design Reviews at Google]]
- [[https://www.atlassian.com/agile/product-management/requirements][Atlassian: Product Requirements Documents]]
- [[https://www.atlassian.com/software/confluence/templates/design-review][Atlassian: Design Review Template]]
- [[https://www.geeksforgeeks.org/software-engineering/software-requirement-specification-srs-document-checklist/][GeeksforGeeks: SRS Document Checklist]]
- [[https://gerrit-review.googlesource.com/Documentation/dev-design-docs.html][Gerrit: Design Docs Review Process]]

* Review and iteration history

** 2026-05-23 Sat @ 00:00:00 -0500 — Claude Code (linear-emacs) — author
- *What:* Initial draft of the workflow file. Validated by a real 2026-05-23 run against a linear-emacs ticket before being handed off to rulesets.
- *Why:* Craig wanted to use each contributor for their value — his time goes into user scenarios and direction-setting, agents' time goes into structured technical review (gate-checking, finding-tracking, disposition rigor) where agents are more consistent than humans. spec-review formalized the reviewer-side discipline so Craig doesn't spend his time on gate-checks an agent can do better.
- *Artifacts:* Delivered to =rulesets/inbox/= on 2026-05-23 as part of a 4-file handoff (2 workflow drafts + 2 handoff notes). Inbox files since deleted after install. Heading timestamp is approximate per the opaque-id convention; no exact time was recorded.

** 2026-05-23 Sat @ 12:59:31 -0500 — Claude Code (rulesets) — reviewer + responder
- *What:* Three tightening edits to the review workflow itself before install: Phase 1 reframed as fast triage with Phase 3 the authoritative gate after the code read; readiness gate expanded from 11 to 13 items adding security/privacy and observability (each with an escape hatch for trivial features); severity-to-blocking-power mapping added in Phase 6 (high blocks Ready, medium is author discretion). Paired response-workflow edits landed in the same commit and are recorded in spec-response.org's history.
- *Why:* Three parallel research subagents (Definition-of-Ready / INVEST, requirements-quality IEEE 830 / ISO 29148, disposition-tracking peer-review practice) confirmed gaps. Close-read identified the Phase 1 vs Phase 3 ordering tension (gate items needed code-read context) and a security/observability blind spot.
- *Artifacts:* Commit =7f2aea1=, +706 lines across 6 files including INDEX entries and the paired spec-response.org edits.

** 2026-05-28 Thu @ 03:22:25 -0500 — Craig Jennings — author
- *What:* Added the Review-and-iteration-history requirement itself. New goal item ("the spec's review history is updated"); new Phase 6 sub-section with the entry shape (heading-as-compound-id + What/Why/Artifacts body); new "Preserve iteration provenance" principle.
- *Why:* Craig had no way to scan a spec under review and tell which iterations had already been processed, which were new, and which were pending — file mtime / created-time was the only signal, silly and error-prone. The iteration-history section makes processing state visible inside the spec content itself.
- *Artifacts:* Commit =55adf6e=.

** 2026-05-28 Thu @ 03:23:01 -0500 — Claude Code (rulesets) — author
- *What:* Backfilled this section with entries for the workflow file's prior iterations (Draft 1, Review-and-Fold, Requirement addition). Used commit author dates where exact; Draft 1's timestamp is an opaque-id placeholder since no exact time was recorded.
- *Why:* The iteration-history requirement landed in =55adf6e= without retroactive entries for the workflow file's own prior history. This pass closes that gap so the section starts with full provenance rather than empty.
- *Artifacts:* Working draft sources at =working/spec-workflows-iteration-history-backfill/draft-entries.org= (deleted on this commit). Spec-response cycle (two Codex reviews and three rulesets responses on the draft) validated the entries and the entry-shape before splicing.

** 2026-05-31 Sun @ 10:39:49 -0500 — Claude Code (rulesets) — author
- *What:* Added a Phase 6 step, "Emit implementation tasks (drop-in for todo.org)": lift the spec's =Implementation phases= section into a paste-ready =todo.org= block (one =[#B]= TODO per phase plus a test-surface entry mirroring =Acceptance criteria=), with the missing-phase-decomposition case raised as a finding rather than invented. Added Exit Criterion 6 to match.
- *Why:* From a pearl handoff. Phase 6 only logged deferred/v1 work in passing; this makes the implementer handoff one paste instead of a re-read, and forces specs to be decomposable into phases (a spec that resists it surfaces a shape problem before =Ready=).
- *Artifacts:* =todo.org= task ([#C] =:feature:solo:=, closed this commit). Phase 6 + Exit Criteria edits in this file.