# 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.