initial commit

2026-06-20 17:02:23 +01:00
commit 414140b962
16 changed files with 13568 additions and 0 deletions
@@ -0,0 +1,172 @@
+
+# Asked qwen3.6 35B about code smells
+
+Analyze the code in src/ for code smells and improvements.
+
+Qwen:
+
+# Here's my analysis of the codebase:
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ Code Smells & Improvements
+
+ ### 1. Duplicated Constants (REFACTOR_FOCI)
+
+ Location: src/state.ts and src/ui.ts
+
+ Both files define their own copy of REFACTOR_FOCI:
+
+ ```ts
+   // state.ts
+   export const REFACTOR_FOCI: RefactorFocus[] = [...];
+   // ui.ts (line 16)
+   const REFACTOR_FOCI: RefactorFocus[] = [...];
+ ```
+
+ Fix: Export the constant from state.ts and import it in ui.ts. This is a direct DRY violation.
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### 2. canTransition is Dead Code
+
+ canTransition() in state.ts is never called by any tool or event handler. The phase enforcement in index.ts is done via imperative if checks in the go-to-refactor tool and the tool_call event
+ handler. The function sits there with full test coverage but zero usage.
+
+ Options:
+ - Delete it if it's not needed — the enforcement logic is already in the tools.
+ - Use it in go-to-refactor instead of the manual if/else checks, making the tool logic cleaner and the guard centralized.
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### 3. redToGreen is Never Called Directly
+
+ redToGreen() is only called internally inside recordTestResult() — it's never invoked by any tool. It's effectively a private helper, yet it's exported and has its own test suite.
+
+ Fix: Either make it private (or just not export it) and remove its standalone tests, or keep it exported if you want it as a reusable state-machine primitive.
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### 4. Magic String "npm test"
+
+ go-to-green hardcodes const testCommand = "npm test 2>&1"; while run-tests allows a custom command. The go-to-green tool should also accept an optional command parameter for consistency.
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### 5. persistState / restoreState — Fragile Serialization
+
+ The state persistence relies on pi.appendEntry / ctx.sessionManager.getEntries() with string-based filtering (e.type === "custom" && e.customType === "tdd-cycle"). This is fragile:
+ - No versioning — if the CycleState interface changes, old entries may corrupt state.
+ - No deduplication logic beyond .pop() — if multiple entries exist, only the last one is kept (which is correct, but the intent isn't explicit).
+ - restoreState is called at the start of every tool execution, meaning state is reloaded on each call — this is fine for a TDD cycle, but worth noting.
+
+ Suggestion: Add a version field to CycleState and handle version mismatches gracefully.
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### 6. buildSteeringMessage is a Long Switch
+
+ This function has 5 if branches that could be replaced with a Record<RefactorFocus, (decision: RefactorDecision) => string> mapping. This would make it data-driven and easier to extend:
+
+ ```ts
+   const STEERING_MESSAGES: Record<RefactorFocus, (d: RefactorDecision) => string> = {
+     "next-test": () => "...",
+     audit: () => "...",
+     // etc.
+   };
+ ```
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### 7. Repeated ctx.ui.setWidget Calls
+
+ Every tool and event handler calls ctx.ui.setWidget("tdd-status", ...) with similar patterns. This could be extracted into a helper like updateStatusWidget(ctx) to reduce repetition and ensure
+ consistency.
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### 8. renderResult Type Guards are Repetitive
+
+ All four tools have the same boilerplate in renderResult:
+
+ ```ts
+   const details = result.details as { ... } | undefined;
+   if (!details) {
+     return new Text(result.content[0]?.type === "text" ? result.content[0].text : "", 0, 0);
+   }
+ ```
+
+ Fix: Extract this into a shared renderResultFallback helper.
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### 9. go-to-refactor Non-TUI Fallback is Silent
+
+ When ctx.mode !== "tui", the tool silently defaults to { focus: "next-test" } without any user confirmation. This could be confusing — the developer has no way to choose a different refactoring
+ focus.
+
+ Suggestion: In non-TUI mode, return the REFACTOR phase status without auto-advancing, and let the developer call the tool again with an explicit parameter.
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### 10. invalidate in UI Component is a No-Op
+
+ ```ts
+   return { render, invalidate: () => {}, handleInput };
+ ```
+
+ The invalidate callback does nothing. If the TUI framework expects it to trigger a re-render, this is a bug. If it's intentionally unused, document why.
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### 11. recordTestResult Returns TransitionResult but Caller Ignores valid/error
+
+ In index.ts, recordTestResult is called and its state is used, but the valid and error fields are discarded:
+
+ ```ts
+   const transition = recordTestResult(state, passed);
+   state = transition.state;  // valid/error ignored
+ ```
+
+ If recordTestResult returns invalid (e.g., called from REFACTOR phase), the state is still updated. The tool should check transition.valid and handle errors.
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### 12. go-to-green Hardcodes Timeout
+
+ Both go-to-green and run-tests use timeout: 60_000. This is fine, but consider making it configurable via a tool parameter for projects with slower test suites.
+
+ ────────────────────────────────────────────────────────────────────────────────
+
+ ### Summary Table
+
+ ┌────┬───────────────────────────────────────────┬──────────┬────────┐
+ │ #  │ Smell                                     │ Severity │ Effort │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 1  │ Duplicated REFACTOR_FOCI                  │ Low      │ 5 min  │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 2  │ Dead canTransition                        │ Medium   │ 10 min │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 3  │ Unused export redToGreen                  │ Low      │ 5 min  │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 4  │ Hardcoded "npm test" in go-to-green       │ Low      │ 5 min  │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 5  │ Fragile state persistence (no versioning) │ Medium   │ 20 min │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 6  │ Long switch → data-driven mapping         │ Low      │ 10 min │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 7  │ Repeated setWidget calls                  │ Low      │ 10 min │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 8  │ Repetitive renderResult guards            │ Low      │ 10 min │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 9  │ Silent non-TUI fallback                   │ Medium   │ 15 min │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 10 │ No-op invalidate                          │ Low      │ 5 min  │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 11 │ Ignored valid/error from recordTestResult │ High     │ 10 min │
+ ├────┼───────────────────────────────────────────┼──────────┼────────┤
+ │ 12 │ Hardcoded timeout                         │ Low      │ 5 min  │
+ └────┴───────────────────────────────────────────┴──────────┴────────┘
+
+ The highest priority fix is #11 — the recordTestResult return value's valid field is silently ignored, which means invalid state transitions (e.g., recording test results while in REFACTOR phase)
+ would silently corrupt state.