diff options
| author | Craig Jennings <c@cjennings.net> | 2026-05-23 12:59:31 -0500 |
|---|---|---|
| committer | Craig Jennings <c@cjennings.net> | 2026-05-23 12:59:31 -0500 |
| commit | 7f2aea1e022c93f3eb463e5222bdb0d8ae6288b9 (patch) | |
| tree | cbbd70275d9384d102d1825e441a114b0e84ca36 /.ai/workflows/spec-review.org | |
| parent | 2d34e9ac3a9cec1f625df75f08890ee85836afa3 (diff) | |
| download | rulesets-7f2aea1e022c93f3eb463e5222bdb0d8ae6288b9.tar.gz rulesets-7f2aea1e022c93f3eb463e5222bdb0d8ae6288b9.zip | |
feat(workflows): add spec-review and spec-response workflow pair
These two workflows form a reviewer side and an author side for taking a design spec to implementation-ready. spec-review judges a spec against a readiness gate, reads the code before critiquing, evaluates across dimensions, assigns a rubric (Ready / Ready-with-caveats / Not-ready / Needs-research), and writes a <spec>-review.org file when it isn't ready. spec-response consumes that file: it decides accept / modify / reject for every recommendation, weaves the accepts into the spec body, records the modifies and rejects with reasons in a "Review dispositions" section, and reconciles tensions when coupled specs get reviewed together. The review file is the contract between them.
Both were validated by a real run on 2026-05-23 before landing here. I then reviewed them against established practice and tightened five things. The readiness gate now covers security/privacy and observability, since a spec shouldn't pass with those undefined. Phase 1 is a fast triage and Phase 3 is the authoritative gate after the code read. Finding severity maps to blocking power. A rejection goes back to the reviewer instead of standing as a unilateral call. And the response loop has an explicit termination condition.
I added both to INDEX.org under a new "Specs and design" section with trigger phrases, cross-linked as a pair, and kept the canonical and mirror copies identical.
Diffstat (limited to '.ai/workflows/spec-review.org')
| -rw-r--r-- | .ai/workflows/spec-review.org | 208 |
1 files changed, 208 insertions, 0 deletions
diff --git a/.ai/workflows/spec-review.org b/.ai/workflows/spec-review.org new file mode 100644 index 0000000..2bdcf73 --- /dev/null +++ b/.ai/workflows/spec-review.org @@ -0,0 +1,208 @@ +#+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. *Deferred work is logged* to =todo.org= (v1 = =[#B]=, vNext/someday = =[#D]=), not left only in chat. + +*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. + +* 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 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. + +** 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]] |
