From e41a79e33ceb204bda1091b70fd712b434397fcd Mon Sep 17 00:00:00 2001 From: Ed_ Date: Sun, 5 Jul 2026 13:14:26 -0400 Subject: [PATCH] =?UTF-8?q?conductor(track):=20superpowers=20review=20sect?= =?UTF-8?q?ion=204=20=E2=80=94=20test-driven-development=20(deep-dive)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../superpowers_review_20260619/report.md | 59 ++++++++++++++++++- 1 file changed, 58 insertions(+), 1 deletion(-) diff --git a/conductor/tracks/superpowers_review_20260619/report.md b/conductor/tracks/superpowers_review_20260619/report.md index 4fd675f9..8a4cc9e8 100644 --- a/conductor/tracks/superpowers_review_20260619/report.md +++ b/conductor/tracks/superpowers_review_20260619/report.md @@ -186,7 +186,64 @@ The project follows the discipline with one architectural difference (4-tier hie ## 4. Test-Driven Development - +## 4. Test-Driven Development + +### 4.1 What the skill prescribes + +The `test-driven-development` skill is rigid — explicitly classified as "Rigid (TDD, debugging): Follow exactly. Don't adapt away discipline" per the skill's self-classification. The Iron Law is unambiguous: "NO PRODUCTION CODE WITHOUT A FAILING TEST FIRST." Violating the letter of the rules is violating the spirit of the rules. The Red-Green-Refactor cycle is the only acceptable flow: RED (write failing test) → Verify RED (watch it fail correctly) → GREEN (minimal code to pass) → Verify GREEN (all green) → REFACTOR (clean up, keep tests green) → Next. The skill's "common rationalizations" table is exhaustive ("Too simple to test", "I'll test after", "Deleting X hours is wasteful", "TDD is dogmatic, being pragmatic means adapting") with explicit counters. The verification checklist is mandatory: every function has a test, watched each test fail, minimal code to pass, all tests pass, output pristine, real code (not mocks), edge cases covered. + +### 4.2 Mapping to the project's existing pattern + +The project has TDD as a load-bearing discipline with extensive elaboration. The project's TDD enforcement is multi-layered: + +| TDD skill rule | Project equivalent | Where | +|---|---|---| +| Iron Law: no production code without failing test | `conductor/workflow.md` §"Task Workflow" step 4 "Write Failing Tests (Red Phase)": "CRITICAL: Run the tests and confirm that they fail as expected. This is the 'Red' phase of TDD. Do not proceed until you have failing tests." | `conductor/workflow.md` step 4 | +| Red-Green-Refactor cycle | Same — step 4 = Red, step 5 = "Implement to Pass Tests (Green Phase)", step 6 = Refactor (optional but recommended) | `conductor/workflow.md` steps 4-6 | +| "Watch it fail" | Explicit: "Run the tests and confirm that they fail as expected" | `conductor/workflow.md` step 4 | +| "No exceptions without partner permission" | `AGENTS.md` Critical Anti-Patterns: "Do not skip TDD - write failing tests before implementing functionality" | `AGENTS.md` "Critical Anti-Patterns" | +| "Delegation: tier-3 worker writes the test" | `conductor/workflow.md` step 4 + step 5: "**Delegate Test Creation:** Do NOT write test code directly. Spawn a Tier 3 Worker via the OpenCode Task tool with `subagent_type: 'tier3-worker'` and a **surgical prompt** specifying WHERE (file:line range), WHAT (test to create), HOW (which assertions/fixtures to use), and SAFETY (thread constraints if applicable)." | `conductor/workflow.md` steps 4-5 | +| "Mocking anti-patterns" | `conductor/product-guidelines.md` "Testing Requirements" → "Structural Testing Contract": "Ban on arbitrary core mocking with `unittest.mock.patch` (unless explicitly authorized for a specific boundary test). All integration and end-to-end testing must use the `live_gui` fixture." | `conductor/product-guidelines.md` "Testing Requirements" | +| "Verification Checklist" (test fails for expected reason, etc.) | `conductor/product-guidelines.md` "Quality Gates" + the Phase Completion Verification and Checkpointing Protocol | `conductor/workflow.md` "Phase Completion Verification" | +| "Skip markers are documentation, not avoidance" | `conductor/workflow.md` "Skip-Marker Policy" — explicit policy with review checklist; `AGENTS.md` "Do not use `@pytest.mark.skip` as an excuse to AVOID fixing the underlying bug" | `conductor/workflow.md` "Skip-Marker Policy" + `AGENTS.md` | +| "Debugging Integration: bug → write failing test reproducing it" | `conductor/workflow.md` "systematic-debugging" reference + the verification-before-completion pattern | `conductor/workflow.md` + `AGENTS.md` | +| "Live GUI testing" (the project's elaboration) | `tests/conftest.py` defines 7 conftest fixtures (`isolate_workspace`, `reset_paths`, `reset_ai_client`, `vlogger`, `kill_process_tree`, `mock_app`, `live_gui`); the `live_gui` fixture is session-scoped and uses the Hook API for end-to-end testing | `tests/conftest.py` + `docs/guide_testing.md` | + +### 4.3 Where the project already follows the discipline + +- **Every track on the project follows the Red-Green-Refactor cycle.** Examples from the chronology: `mcp_architecture_refactor_20260606` (Phase 1 = audit tests pass = baseline; Phase 2 = write new tests; Phase 3 = refactor; etc.); `data_oriented_error_handling_20260606` (Phase 1 = baseline green; subsequent phases add tests before refactoring); `default_layout_install_20260629` (Phase 1 = failing test for "panels invisible on empty INI"; subsequent phases = minimal fix). +- **The "don't keep code as reference" rule is enforced.** The user's `cruft_elimination_20260627` track explicitly removed legacy code without TDD justification; the `default_layout_install_followup_20260629` track added failing tests for stale INI handling. +- **The "no mocks for core infrastructure" rule is enforced** via the audit script `scripts/audit_main_thread_imports.py` (main-thread-purity invariant) and the Structural Testing Contract (`live_gui` standard). +- **The "verification before completion" discipline** is enforced via the Phase Completion Verification and Checkpointing Protocol (must run automated tests + API hooks + user confirmation before checkpoint). +- **The 251 test files** (per `docs/guide_testing.md`) are evidence of the project's TDD commitment. + +### 4.4 Where the project doesn't follow the discipline + +- **"Throwaway prototypes" exception** is acknowledged in the skill but the project doesn't have an explicit "prototype" affordance. The TDD skip is rare in the project; when it happens (e.g., the `cruft_elimination_20260627` track's audit-script work), it's documented as an exception. +- **"Generated code" exception** — n/a for this project (no code generation from LLMs to runtime code). +- **"Configuration files" exception** — partially followed. TOML config changes (`presets.toml`, `personas.toml`, `manual_slop.toml`) are made without TDD in some tracks (e.g., `presets_ai_settings_ux_20260311`); the convention is that config changes don't need new tests if the existing config-loading tests still pass. +- **"No exceptions without partner permission"** — partially followed. The TDD skill's exception list is small ("Throwaway prototypes", "Generated code", "Configuration files") and each requires partner permission. The project has more flexible exception usage; the partner is the user, but the user doesn't have to give explicit permission for each config-file change. +- **The skill's "Test passes immediately → you're testing existing behavior, fix test"** rule has surfaced in the project as the `isolation-pass verification fallacy` (per `conductor/workflow.md` §"Isolated-Pass Verification Fallacy"): tests that pass in isolation but fail in batch. The project has a dedicated rule for this; the skill does not. + +### 4.5 Recommendations summary + +The project follows the TDD discipline strictly. The four elaborations (tier-3-worker delegation; live_gui standard; skip-marker policy; isolated-pass verification rule) go *beyond* the skill. The deferred rebuild may want to: + +- **LOW:** Add a note to `conductor/workflow.md` §"Task Workflow" step 4 + step 5 that the "tier-3 worker dispatches the failing test" rule is the project's elaboration of the skill's "write the test first" rule. This is documentation, not a behavior change. +- **LOW:** Codify the "configuration files" exception explicitly. Currently the project applies this exception case-by-case; codifying the rule would reduce ambiguity. + +**Verdict.** + +| Field | Value | +|---|---| +| **Primary** | `PARITY` | +| **Integration tag** | `INTEGRATED` | +| **Section size** | deep-dive | +| **Cross-refs** | nagent_review_20260608 §10 ("durable tests" — nagent's principle that tests are durable artifacts); fable_review_20260617 §13 ("Genuinely Useful Patterns" includes the project's testing discipline); intent_dsl_survey_20260612 §6 (the survey's "AI-agent properties" include the project's testing rigor) | + +**Rationale.** The project's 12-step task workflow + AGENTS.md "Critical Anti-Patterns" + Structural Testing Contract is a faithful re-encoding of the TDD skill with four project-specific elaborations. The iron law is enforced via step 4's "Do not proceed until you have failing tests" rule. The Red-Green-Refactor cycle is the workflow steps 4-6. + +**Recommended change.** *(blank — no rebuild action. The project is in alignment.)* ## 5. Verification Before Completion