Skip to content

Consolidate design rules in code-design guidelines + migration plan #223

Closed
Closed
Consolidate design rules in code-design guidelines + migration plan#223
Labels
user storyWork item representing a user story

Description

@rucka
rucka
opened on Jul 6, 2026

Resolution

Delivered by PR #274 (merged). Board: Done.

feat: consolidate design rules in code-design guidelines.

Story Statement

As a maintainer of pair code quality
I want design rules consolidated in the code-design guidelines (do/don't form, evidence-based) plus a migration plan for existing code
So that the AI stops generating known bad patterns ("too many ifs") and legacy code converges (R6.7)

Where: knowledge/guidelines/code-design/; loaded by implement and review

Epic Context

Parent Epic: Code Quality & Testing foundations #209
Status: Refined
Priority: P0 (Must-Have)

Status Workflow

  • Refined: Story is detailed, estimated, and ready for development
  • In Progress: Story is actively being developed
  • Done: Story delivered and accepted

Acceptance Criteria

Functional Requirements

Given-When-Then Format:

  1. Given the consolidated findings in Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199 and the R6.7 rules ("too many ifs" family)
    When the code-design guidelines are updated
    Then each rule appears in do/don't form with recognition criteria and the preferred alternative, evidence-linked to real findings

  2. Given pair-process-implement runs on any story
    When it loads project guidelines
    Then the design rules are part of its constraints (prevention at generation time)

  3. Given pair-process-review evaluates a diff violating a rule
    When the technical review phase runs
    Then the violation is reported referencing the specific rule (consistent, non-vague feedback)

  4. Given existing code violating the rules
    When the migration plan is produced
    Then violations become tech-debt Draft items (P1–P3), they do NOT block PRs

Business Rules

Edge Cases and Error Handling

  • Rule conflicts with project adoption: adoption wins; note in rule entry
  • Ambiguous match in review: report as suggestion, not violation

Definition of Done Checklist

Development Completion

  • All acceptance criteria implemented and verified
  • Guidelines follow KB conventions (markdownlint green)
  • Code review completed and approved
  • Documentation updated — docs site reflects updated guidelines

Quality Assurance

  • implement/review reference verified (skill text loads the rules) — dry-run against 5 sample fixtures (3 clear violations, 1 clean-negative set, 1 borderline), see PR #274 remediation comment
  • Migration items created and tagged

Story Sizing and Sprint Readiness

Refined Story Points

Final Story Points: 3 (M)
Confidence Level: High
Sizing Justification: consolidation of existing evidence + guideline writing + two skill-text references

Sprint Capacity Validation

Sprint Fit Assessment: yes, single sprint
Total Effort Assessment: fits — Yes

Dependencies and Coordination

Story Dependencies

Prerequisite Stories: none (input = #199 findings, already available)
Dependent Stories: #224 (scan uses the rules), migration debt items

Validation and Testing Strategy

Acceptance Testing Approach

Testing Methods: sample diff violating each rule → review flags it; implement dry-run shows rules in loaded context

Notes and Additional Context

Refinement Session Insights: D12 confirmed (no catalog); rules also feed assess-debt scan (#224)
Documentation Links: R6.7 · Spec G8 · D12, D22 · #199

Technical Analysis

Implementation Approach

Technical Strategy: extract recurring defect patterns from #199 findings → write do/don't entries in knowledge/guidelines/code-design/ (source: knowledge-hub dataset) → reference from implement/review skill texts → generate migration debt items
Key Components: code-design guideline files; implement/review SKILL.md references; migration item list
Integration Points: #224 scan (rules as detection source); review checklist

Technical Risks and Mitigation

Risk Impact Probability Mitigation Strategy
Rules too abstract to act on Medium Medium every rule carries a concrete recognition example from #199

Task Breakdown

Checklist

Dependency Graph

T1 → T2 → (T3 ∥ T4) → T5

AC Coverage

Task AC1 AC2 AC3 AC4
T1 ✓
T2 ✓
T3 ✓ ✓
T4 ✓
T5 ✓ ✓ ✓ ✓

T-1: Extract recurring patterns from #199 findings (+ R6.7 list)

Priority: P0 | Estimated Hours: 1.5h | Bounded Context: Knowledge Base Management

Summary: Identify which #199 audit findings recur as generatable code patterns (vs. one-off config/CI issues) and are worth codifying as do/don't rules.

Type: Research

Description: From the #199 audit (Codebase audit 2026-04-17), three findings were selected as structural/generatable patterns rather than config drift: P0.4 (LinkProcessor static-only class), P0.5 (copyPathOps.ts god module, generalized into a 4-instance cluster by pulling in P0.3 cli.e2e.test.ts, P2.2 dev/App.tsx, P2.3 in-memory-fs.ts), and P1.3 (movePathOps.ts MoveCtx optional-bag). These became DR-1, DR-2, DR-3 respectively in design-rules.md. The epic's R6.7 requirement (#209: "Design rules consolidation (R6.7, D12)") is the umbrella label for this entire story, not a separate rule to extract on its own — its illustrative example ("too many ifs") is addressed indirectly through DR-3, which is explicitly framed in the doc as "the concrete, type-unsafe cousin of 'too many ifs'".

Acceptance Criteria:

Implementation Approach:

Dependencies:

  • Tasks: none (first task in the chain)

Implementation Steps:

  1. Read Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199 in full, list every P0/P1/P2 finding
  2. Classify each as "structural pattern" (candidate for a DR rule) vs. "config/process" (out of scope for this story)
  3. Cluster same-shaped findings together (P0.5 + P0.3 + P2.2 + P2.3 → one god-module cluster instead of four separate rules)
  4. Hand off the shortlist (LinkProcessor static class, god-module cluster, MoveCtx optional-bag) to T2

Testing Strategy:

Notes: R6.7 itself is not a discrete pattern to extract — per epic #209 it's the requirement ID for "design rules consolidation" as a whole, satisfied by this story existing. No discrepancy there. One partial-coverage gap worth naming: #199 contains no discrete "too many ifs" / branching-complexity finding, so the epic's own illustrative example is only indirectly covered (via DR-3's framing) rather than extracted as its own rule. Not a defect — there was no #199 evidence to extract for it — but a rigorous reader could expect a literal "too many ifs" DR entry given the story statement's wording, and won't find one.

T-2: Write do/don't rules in code-design guidelines (recognition + alternative + evidence)

Priority: P0 | Estimated Hours: 3h | Bounded Context: Knowledge Base Management

Summary: Author design-rules.md with DR-1 (God Module), DR-2 (Static-Only Namespace Class), DR-3 (Optional-Bag Dispatch), each in do/don't form with recognition criteria, code examples, a preferred alternative, and an evidence citation back to #199 — plus a Migration Plan table for existing violations.

Type: Documentation

Description: Created packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/design-principles/design-rules.md (118 lines). Each of the 3 rules follows the same shape: Don't / Recognition / code snippet / Do / code snippet / Evidence. A trailing "Migration Plan" table lists 6 concrete current-code instances (DR-1 x4, DR-2 x1, DR-3 x1) with priority P1/P2 and a one-line remediation note, plus an explicit statement that no tech-debt Draft items are created by this story (deferred to #224). A "Related" section cross-links technical-debt.md, solid-principles.md, naming-conventions.md, typescript.md.

Acceptance Criteria:

  • Primary deliverable: design-rules.md with 3 rules in do/don't form + Migration Plan table
  • Quality standard: every rule has a recognition criterion, a code example for both "don't" and "do", and an evidence citation with file path + line range
  • Integration requirement: file is discoverable from design-principles/README.md's file list and "Evidence-Based Rules" section (both updated)
  • Verification method: evidence citations checked against current source (line ranges, LOC counts, method counts)

Implementation Approach:

  • Technical Design: one file, one rule per ## section, evidence line always in the format #199 <finding-id> <path>:<lines> — <detail>
  • Files to Modify/Create:
    • packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/design-principles/design-rules.md - new file, the 3 rules + migration plan
    • packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/design-principles/README.md - added design-rules.md to file list + new "Evidence-Based Rules" subsection

Dependencies:

  • Tasks: T1

Implementation Steps:

  1. Draft DR-1 (God Module) from the P0.5/P0.3/P2.2/P2.3 cluster, with copyPathOps.ts as the primary example
  2. Draft DR-2 (Static-Only Namespace Class) from P0.4 (LinkProcessor)
  3. Draft DR-3 (Optional-Bag Dispatch) from P1.3 (MoveCtx)
  4. Build the Migration Plan table from the same 6 instances, assign P1/P2 by impact/effort
  5. Wire design-rules.md into design-principles/README.md's file list and overview
  6. Follow-up commit (b7cab4f): correct DR-2's re-export line range and add the explicit "assess-* skills are output-only; record-decision is the sole adoption writer (no scan-mode) #224 defers Draft-item creation" note

Testing Strategy:

  • Unit Tests: not applicable (markdown content, no executable code)
  • Manual Testing: markdownlint run implied by DoD ("Guidelines follow KB conventions"); line-range and LOC citations re-verified against current source during this documentation pass

Notes: Independently re-verified every citation against current main source. DR-1 LOC/function counts are exact: copyPathOps.ts 705 LOC / 19 function occurrences, cli.e2e.test.ts 1507 LOC, dev/App.tsx 456 LOC, in-memory-fs.ts 429 LOC — all match. DR-3 citations are exact: movePathOps.ts:189 is the type MoveCtx = Partial<...> declaration; the non-null-assertion sites are at exactly 157-158 and 172-173. DR-2's re-export range was wrong at 409-418 in the first commit (7a96029) and was corrected to 406-421 in b7cab4f — re-verified: line 406 is the doc-comment opening the extractLinks wrapper and line 421 is the closing brace of detectLinkStyle, so 406-421 is now byte-accurate. One citation was not touched by that fix and remains imprecise: DR-2's primary range link-processor.ts:42-411 for "class LinkProcessor" — the class body actually spans lines 42-404 in current source, not 42-411; line 411 lands mid-way through the immediately-following extractLinks compat wrapper (it stops before detectLinkStyle, which ends at 421). This exact range was inherited verbatim from #199 P0.4's own text, so it isn't a new error introduced by this story, but it is still not byte-accurate against current source and slipped through the DR-2 fix pass in b7cab4f.

T-3: Reference rules in implement + review skill texts

Priority: P0 | Estimated Hours: 1h | Bounded Context: Knowledge Base Management

Summary: Wire design-rules.md into pair-process-implement's constraint-loading step and into pair-process-review's technical-review step, per AC2/AC3.

Type: Feature Implementation

Description: In implement/SKILL.md, added design-rules.md to the Step 1.1 context-loading list (alongside architecture.md, tech-stack.md, way-of-working.md) and added an explicit instruction in the TDD-discipline section not to generate a new instance of a known do/don't pattern, naming the three concrete patterns (god module, static-only namespace class, optional-bag dispatch) as examples. In review/SKILL.md, added design-rules.md as the first item checked in Step 2.2's review list, with an explicit rule for citing the rule ID (e.g. "DR-1 — God Module") on a clear match, and downgrading a partial/ambiguous match to a suggestion rather than a violation — implementing AC3's "consistent, non-vague feedback" and the story's "ambiguous match → suggestion" edge case.

Acceptance Criteria:

  • Primary deliverable: both implement/SKILL.md and review/SKILL.md link to design-rules.md at the correct relative path
  • Quality standard: the review instruction explicitly distinguishes "clear match → violation, cite rule ID" from "ambiguous match → suggestion, doesn't count toward review decision"
  • Integration requirement: relative path (../../../.pair/knowledge/guidelines/code-design/design-principles/design-rules.md) resolves correctly from both .skills/process/implement/SKILL.md and .skills/process/review/SKILL.md to the actual file location
  • Verification method: manual path resolution check + comparison against the pattern used by pre-existing links in the same files (e.g. the architecture.md link)

Implementation Approach:

  • Technical Design: minimal, surgical additions — one new bullet/link per file, no restructuring of existing skill text
  • Files to Modify/Create:
    • packages/knowledge-hub/dataset/.skills/process/implement/SKILL.md - Step 1.1 context list + TDD discipline intro, both now reference Design Rules
    • packages/knowledge-hub/dataset/.skills/process/review/SKILL.md - Step 2.2 review list + Step 2.2 verify line, now reference Design Rules and the violation/suggestion split

Dependencies:

  • Tasks: T2

Implementation Steps:

  1. Add the Design Rules link to implement/SKILL.md Step 1.1's context-loading bullet list
  2. Extend the TDD discipline intro sentence in implement/SKILL.md to require checking against Design Rules, naming the 3 concrete anti-patterns
  3. Add the Design Rules link as the first bullet in review/SKILL.md Step 2.2, with the violation-citation / ambiguous-match-is-suggestion rule spelled out inline
  4. Update review/SKILL.md Step 2.2's "Verify" line to require rule-ID referencing where applicable

Testing Strategy:

  • Unit Tests: not applicable (skill text is prose, not executable)
  • Manual Testing: traced the relative path from each SKILL.md's directory to confirm it resolves to packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/design-principles/design-rules.md, matching the depth of adjacent pre-existing links in the same files

Notes: No discrepancy found. Both edits are minimal, additive, and match AC2 (design rules loaded as implement-time constraints) and AC3 (review cites the specific rule ID; ambiguous matches downgraded to suggestions) exactly as specified. The relative path is correct in both files.

T-4: Produce migration plan → tech-debt Draft items

Priority: P1 | Estimated Hours: 1h | Bounded Context: Knowledge Base Management

Summary: Produce the Migration Plan table of current rule violations in design-rules.md, as the seed data for #224's pair-capability-assess-debt scan mode, which will convert it into actual tech-debt Draft items.

Type: Documentation

Description: The "Migration Plan (existing violations)" section of design-rules.md lists 6 concrete instances (4x DR-1, 1x DR-2, 1x DR-3) with file path, size/detail, priority (P1/P2), and a one-line remediation note. The section explicitly states this story does not create any tech-debt Draft items itself — that conversion step is deferred to #224, per the story's own "Dependent Stories: #224 (scan uses the rules)" note. This is a reasoned scope split (parsing the table into PM-tool items is a distinct capability, pair-capability-assess-debt scan mode, not this story's job) documented inline in the story text, not an unplanned gap.

Acceptance Criteria:

  • Primary deliverable: Migration Plan table in design-rules.md, one row per known violation, with rule ID, location, priority, and note
  • Quality standard: every row cites a real, currently-existing instance (verified against source, not invented)
  • Integration requirement: table is structured so #224's scan mode can consume it as seed data (consistent columns: Rule / Location / Priority / Note)
  • Verification method: each row's file path and line/LOC reference checked against current source

Implementation Approach:

  • Technical Design: table format, one row per instance, reusing the same file paths already cited in the rules' Evidence lines
  • Files to Modify/Create:
    • packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/design-principles/design-rules.md - "Migration Plan (existing violations)" section (same file as T2, different section)

Dependencies:

  • Tasks: T2 (table reuses the same evidence gathered while writing the rules)

Implementation Steps:

  1. List every concrete file instance identified while drafting DR-1/DR-2/DR-3 in T2
  2. Assign priority (P1/P2) by rough impact/effort per instance
  3. Add a one-line remediation note per row (e.g. "split per Tech-debt resolution ledger (2026-04-17 audit) — resolve findings incrementally #199 suggested execution: copy-file.ts / copy-directory.ts / copy-orchestrator.ts")
  4. Add the explicit deferral note: no Draft items created here, conversion happens in #224's scan mode (added in follow-up commit b7cab4f)

Testing Strategy:

  • Unit Tests: not applicable
  • Manual Testing: re-verified all 6 rows' file paths and line/LOC references against current source during this documentation pass — all accurate (see T2 Notes for the DR-2/DR-3 line-range spot-check)

Notes: AC4 ("violations become tech-debt Draft items... they do NOT block PRs") is only half-satisfied by this story alone. The "do NOT block PRs" half is fully satisfied (T3's review instruction explicitly treats rule matches as findings/suggestions, never a merge gate). The "become Draft items" half is intentionally deferred to #224 — the table exists as seed data but no Draft item has actually been created yet by anything on this branch. This is the known, already-documented split (story text: "Dependent Stories: #224") and is a legitimate reasoned deferral, not a new discrepancy. No tech-debt Draft items exist yet as of this branch.

T-5: Docs site update + QA (sample-diff test)

Priority: P1 | Estimated Hours: 1.5h | Bounded Context: Knowledge Base Management

Summary: Reflect the new Design Rules in the public docs site catalog, and validate end-to-end that implement loads the rules and review flags a violating sample diff.

Type: Documentation

Description: Updated apps/website/content/docs/reference/guidelines-catalog.mdx: bumped the "Last updated" date and extended the "Design Principles" row's description to mention "evidence-based do/don't design rules". Updated design-principles/README.md's "Design Philosophy" area with a new "Evidence-Based Rules" subsection summarizing design-rules.md's do/don't format, evidence-linking, and migration plan. No automated test or test fixture was added on this branch for the "sample-diff test" mentioned in the task title.

Acceptance Criteria:

  • Primary deliverable: guidelines-catalog.mdx and design-principles/README.md both reflect the new Design Rules guideline
  • Quality standard: docs site content stays consistent with the KB source (packages/knowledge-hub/dataset/.pair/knowledge/...), per the "Source:" line in the mdx
  • Integration requirement: catalog entry findable under the existing "Design Principles" topic row, no new top-level category needed
  • Verification method: sample diff violating each rule → review flags it; implement dry-run shows rules in loaded context (per story's own Validation and Testing Strategy)

Implementation Approach:

  • Technical Design: docs-only edits, no new page/route; catalog row extended in place
  • Files to Modify/Create:
    • apps/website/content/docs/reference/guidelines-catalog.mdx - "Last updated" date bump + "Design Principles" row description
    • packages/knowledge-hub/dataset/.pair/knowledge/guidelines/code-design/design-principles/README.md - new "Evidence-Based Rules" subsection (shared file with T2)

Dependencies:

  • Tasks: T3, T4 (docs summary should reflect the finished rule-referencing and migration-plan work)

Implementation Steps:

  1. Update guidelines-catalog.mdx's "Last updated" line to the date of this change
  2. Extend the "Design Principles" topic row description to mention the new rules
  3. Add the "Evidence-Based Rules" subsection to design-principles/README.md
  4. Manually dry-run implement/review against a sample violating diff to confirm the rules surface as intended (per the story's Testing Methods)

Testing Strategy:

  • Unit Tests: not applicable (docs-only change)
  • Manual Testing: per the story's own Validation strategy — "sample diff violating each rule → review flags it; implement dry-run shows rules in loaded context" — described as the intended verification method

Notes: Gap closed (2026-07-07): the sample-diff QA pass was executed by hand against 5 fixtures (3 clear violations — DR-1/DR-2/DR-3 each correctly cited by rule ID; 1 clean-negative set confirming no false positives; 1 borderline case confirming the violation-vs-suggestion split classifies an ambiguous match as "suggestion" not "violation"), plus a direct algorithm trace confirming implement/SKILL.md Step 1.1 loads design-rules.md into context. Recorded in the PR #274 remediation comment — fixtures are guideline-validation samples, not product code, so they aren't committed to the branch itself.

Metadata

Metadata

Assignees

No one assigned

    Labels

    user storyWork item representing a user story

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions