Skip to content

Commit 9975880

Browse files
authored
refactor: apply low-effort structural cleanup (items 1, 2, 3, 6) (#20)
* refactor: apply low-effort structural cleanup items 1, 2, 3, 6 - Move TARGET_TO_CLAUDE_CLI into model_mappings.js; import in claude_cli_launcher.js so target definitions have one source of truth - Extract ensureFile/readStore/writeStore into src/switchboard/fs-utils.js; session_store.js and route_context.js now import shared helpers - Define ANTHROPIC_TARGETS_PATH once in paths.js; remove duplicate local definitions and dead __filename/__dirname boilerplate from workflow.js and claude_cli_launcher.js - Rename deriveLegacyTurnFields -> extractTurnFields in route_context.js All 89 tests pass. * fix(review): restore path import in claude_cli_launcher; rename legacy variable - Re-add 'node:path' import dropped during item 3 cleanup; appendLog still calls path.dirname() and would have thrown at runtime - Rename local variable 'legacy' -> 'turnFields' in saveRouteContext for consistency with the extractTurnFields rename from item 6 Addresses Copilot review comments on PR #20. * docs(refactoring): refresh completed-item references in plan - Update item 1 mapping reference to current source location - Replace stale item 3 file/line snippets with centralized paths constant - Update item 6 naming from deriveLegacyTurnFields to extractTurnFields --------- Co-authored-by: Hanna Rosengren <4538260+hannasoderstromdev@users.noreply.github.com>
1 parent 7bb7a55 commit 9975880

8 files changed

Lines changed: 148 additions & 61 deletions

File tree

docs/REFACTORING-PLAN.md

Lines changed: 104 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,104 @@
1+
# Refactoring Plan
2+
3+
Findings from the May 2026 architecture review. Work on these one at a time. Mark items as **Done** when complete.
4+
5+
---
6+
7+
## 1. Unify target mappings — LOW effort / HIGH value
8+
9+
**Problem:** Two parallel, unconnected mapping structures had to be kept in sync manually.
10+
11+
- `src/adapters/model_mappings.js``targetId → profile → model` (used by SDK adapters)
12+
- `src/adapters/model_mappings.js``TARGET_TO_CLAUDE_CLI` (`targetId → { model, effort }`) (used by CLI path)
13+
14+
Adding or renaming a target required updates in both places with no guardrail.
15+
16+
**Status:** Done
17+
18+
---
19+
20+
## 2. Extract shared fs-utils — LOW effort
21+
22+
**Problem:** `session_store.js` and `route_context.js` each define nearly identical private helpers:
23+
24+
```js
25+
function ensureFile(storePath) { ... }
26+
function readStore(storePath) { ... }
27+
function writeStore(storePath, data) { ... }
28+
```
29+
30+
Divergence risk: if one is fixed, the other silently stays broken.
31+
32+
**Status:** Done
33+
34+
---
35+
36+
## 3. Hard-code `ANTHROPIC_TARGETS_PATH` in one place — LOW effort
37+
38+
**Problem:** The same path was independently resolved in two files:
39+
40+
```js
41+
// now centralized in src/switchboard/paths.js
42+
export const ANTHROPIC_TARGETS_PATH = path.join(__dirname, "..", "router", "data", "targets.anthropic.json");
43+
```
44+
45+
**Status:** Done
46+
47+
---
48+
49+
## 4. Extract `explain` command from `workflow.js` — MEDIUM effort
50+
51+
**Problem:** `workflow.js` handles session loading, routing coordination, log writing, context-package building, attribution construction, serialization helpers, the `explain` command, and continuity probes. It is both orchestrator and utility module.
52+
53+
The `explain` command is conceptually separate — it reads past logs rather than driving a new turn — so it's the most obvious thing to split out.
54+
55+
**Fix:** Create `src/switchboard/explain.js` that owns `explainLatestSwitchboardTurn`, `reconstructReasoning`, and related read-only log helpers. Import from `workflow.js` if needed, then remove the implementation there.
56+
57+
**Status:** Not started
58+
59+
---
60+
61+
## 5. Separate prompt classifier from router — MEDIUM effort
62+
63+
**Problem:** `router.js` mixes two distinct concerns:
64+
65+
- `classifyPrompt` — keyword-matching heuristics that produce `taskType` / `proposedMode` signals
66+
- `routePrompt` — policy evaluation, hard/soft constraints, continuity cost, target selection
67+
68+
The classifier produces signals; the router consumes them. They have different change rates and different test surfaces.
69+
70+
**Fix:** Extract `classifyPrompt` (and its supporting constants like `TASK_TYPE_TO_ADDITIONAL_REQUIREMENTS`) into `src/router/classifier.js`. Keep `router.js` as the policy engine that imports from the classifier.
71+
72+
**Status:** Not started
73+
74+
---
75+
76+
## 6. Remove or promote `extractTurnFields` — LOW effort
77+
78+
**Problem:** `route_context.js` had a function named `deriveLegacyTurnFields`. The "legacy" label signaled it had been kept alive longer than intended and imposed a cognitive tax on every reader.
79+
80+
**Status:** Done
81+
82+
---
83+
84+
## 7. Align file naming convention — LOW effort
85+
86+
**Problem:** Source files use `snake_case` (`anthropic_claude_adapter.js`, `session_store.js`); test files use `kebab-case` (`claude-cli-launcher.test.js`, `thread-session-store.test.js`). Pairing a source file with its test requires a mental translation step.
87+
88+
**Fix:** Decide on one convention (kebab-case is more common in the JS ecosystem) and rename files accordingly. Update all imports.
89+
90+
**Status:** Not started
91+
92+
---
93+
94+
## Order of execution
95+
96+
| # | Item | Effort | Notes |
97+
|---|------|--------|-------|
98+
| 1 | Unify target mappings | Low | Do first — prevents divergence bugs |
99+
| 2 | Shared fs-utils | Low | Quick win, reduces duplication |
100+
| 3 | Paths constant | Low | Quick win, no logic changes |
101+
| 6 | Legacy field | Low | Audit first to confirm safe to delete |
102+
| 4 | Extract explain | Medium | Biggest readability gain in workflow.js |
103+
| 5 | Separate classifier | Medium | Makes router independently testable |
104+
| 7 | File naming | Low | Do last — touches many files at once |

src/adapters/model_mappings.js

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,12 @@ export const PROFILE_MODEL_MAP = {
3434
}
3535
};
3636

37+
export const TARGET_TO_CLAUDE_CLI = {
38+
"anthropic-quick": { model: "haiku", effort: "low" },
39+
"anthropic-balanced": { model: "sonnet", effort: "medium" },
40+
"anthropic-coder": { model: "sonnet", effort: "high" }
41+
};
42+
3743
export function getTargetProfileMap(vendor) {
3844
return TARGET_PROFILE_MAP[vendor] || {};
3945
}

src/switchboard/claude_cli_launcher.js

Lines changed: 2 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -2,19 +2,10 @@ import { spawnSync } from "node:child_process";
22
import { randomUUID } from "node:crypto";
33
import fs from "node:fs";
44
import path from "node:path";
5-
import { fileURLToPath } from "node:url";
5+
import { TARGET_TO_CLAUDE_CLI } from "../adapters/model_mappings.js";
6+
import { ANTHROPIC_TARGETS_PATH } from "./paths.js";
67
import { routePrompt } from "../router/router.js";
78

8-
const __filename = fileURLToPath(import.meta.url);
9-
const __dirname = path.dirname(__filename);
10-
const ANTHROPIC_TARGETS_PATH = path.join(__dirname, "..", "router", "data", "targets.anthropic.json");
11-
12-
const TARGET_TO_CLAUDE_CLI = {
13-
"anthropic-quick": { model: "haiku", effort: "low" },
14-
"anthropic-balanced": { model: "sonnet", effort: "medium" },
15-
"anthropic-coder": { model: "sonnet", effort: "high" }
16-
};
17-
189
function readJson(filePath) {
1910
return JSON.parse(fs.readFileSync(filePath, "utf8"));
2011
}

src/switchboard/fs-utils.js

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
import fs from "node:fs";
2+
import path from "node:path";
3+
4+
export function ensureFile(storePath) {
5+
fs.mkdirSync(path.dirname(storePath), { recursive: true });
6+
if (!fs.existsSync(storePath)) fs.writeFileSync(storePath, "{}\n", "utf8");
7+
}
8+
9+
export function readStore(storePath) {
10+
ensureFile(storePath);
11+
return JSON.parse(fs.readFileSync(storePath, "utf8"));
12+
}
13+
14+
export function writeStore(storePath, data) {
15+
ensureFile(storePath);
16+
fs.writeFileSync(storePath, `${JSON.stringify(data, null, 2)}\n`, "utf8");
17+
}

src/switchboard/paths.js

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,13 @@
11
import os from "node:os";
22
import path from "node:path";
3+
import { fileURLToPath } from "node:url";
4+
5+
const __filename = fileURLToPath(import.meta.url);
6+
const __dirname = path.dirname(__filename);
37

48
export const DEFAULT_SWITCHBOARD_STATE_DIR = path.join(os.homedir(), ".model-switchboard");
59
export const DEFAULT_SWITCHBOARD_STORE_PATH = path.join(DEFAULT_SWITCHBOARD_STATE_DIR, "switchboard-sessions.json");
610
export const DEFAULT_SWITCHBOARD_LOG_PATH = path.join(DEFAULT_SWITCHBOARD_STATE_DIR, "switchboard-turns.ndjson");
711
export const DEFAULT_ROUTE_CONTEXT_PATH = path.join(DEFAULT_SWITCHBOARD_STATE_DIR, "switchboard-route-context.json");
812
export const DEFAULT_CLAUDE_HOOK_LOG_PATH = path.join(DEFAULT_SWITCHBOARD_STATE_DIR, "claude-hook-events.ndjson");
13+
export const ANTHROPIC_TARGETS_PATH = path.join(__dirname, "..", "router", "data", "targets.anthropic.json");

src/switchboard/route_context.js

Lines changed: 12 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -1,25 +1,9 @@
1-
import fs from "node:fs";
2-
import path from "node:path";
31
import { DEFAULT_ROUTE_CONTEXT_PATH } from "./paths.js";
2+
import { readStore, writeStore } from "./fs-utils.js";
43

54
export { DEFAULT_ROUTE_CONTEXT_PATH } from "./paths.js";
65

7-
function ensureFile(storePath) {
8-
fs.mkdirSync(path.dirname(storePath), { recursive: true });
9-
if (!fs.existsSync(storePath)) fs.writeFileSync(storePath, "{}\n", "utf8");
10-
}
11-
12-
function readStore(storePath) {
13-
ensureFile(storePath);
14-
return JSON.parse(fs.readFileSync(storePath, "utf8"));
15-
}
16-
17-
function writeStore(storePath, store) {
18-
ensureFile(storePath);
19-
fs.writeFileSync(storePath, `${JSON.stringify(store, null, 2)}\n`, "utf8");
20-
}
21-
22-
function deriveLegacyTurnFields(context = {}) {
6+
function extractTurnFields(context = {}) {
237
const sessionState = context.sessionState || null;
248
const routingDecision = context.routingDecision || null;
259
const contextPackage = context.contextPackage || null;
@@ -83,18 +67,18 @@ export function saveRouteContext({
8367

8468
const store = readStore(storePath);
8569
const existing = store[sessionId] || { turns: [] };
86-
const legacy = deriveLegacyTurnFields(context);
70+
const turnFields = extractTurnFields(context);
8771
const turn = {
88-
threadId: legacy.threadId,
72+
threadId: turnFields.threadId,
8973
claudeSessionId: sessionId,
90-
turnCount: legacy.turnCount,
91-
routeLabel: legacy.routeLabel,
92-
targetId: legacy.targetId,
93-
model: legacy.model,
94-
effort: legacy.effort,
95-
mode: legacy.mode,
96-
executionMode: legacy.executionMode,
97-
wrapperContext: legacy.wrapperContext,
74+
turnCount: turnFields.turnCount,
75+
routeLabel: turnFields.routeLabel,
76+
targetId: turnFields.targetId,
77+
model: turnFields.model,
78+
effort: turnFields.effort,
79+
mode: turnFields.mode,
80+
executionMode: turnFields.executionMode,
81+
wrapperContext: turnFields.wrapperContext,
9882
sessionState: context.sessionState || null,
9983
routingDecision: context.routingDecision || null,
10084
contextPackage: context.contextPackage || null,

src/switchboard/session_store.js

Lines changed: 1 addition & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -1,20 +1,4 @@
1-
import fs from "node:fs";
2-
import path from "node:path";
3-
4-
function ensureFile(storePath) {
5-
fs.mkdirSync(path.dirname(storePath), { recursive: true });
6-
if (!fs.existsSync(storePath)) fs.writeFileSync(storePath, "{}\n", "utf8");
7-
}
8-
9-
function readStore(storePath) {
10-
ensureFile(storePath);
11-
return JSON.parse(fs.readFileSync(storePath, "utf8"));
12-
}
13-
14-
function writeStore(storePath, data) {
15-
ensureFile(storePath);
16-
fs.writeFileSync(storePath, `${JSON.stringify(data, null, 2)}\n`, "utf8");
17-
}
1+
import { readStore, writeStore } from "./fs-utils.js";
182

193
export function loadThreadSession({ storePath, threadId }) {
204
const store = readStore(storePath);

src/switchboard/workflow.js

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2,9 +2,9 @@ import { randomUUID } from "node:crypto";
22
import { spawnSync } from "node:child_process";
33
import fs from "node:fs";
44
import path from "node:path";
5-
import { fileURLToPath } from "node:url";
65
import { planClaudeCliLaunch } from "./claude_cli_launcher.js";
76
import {
7+
ANTHROPIC_TARGETS_PATH,
88
DEFAULT_SWITCHBOARD_LOG_PATH,
99
DEFAULT_SWITCHBOARD_STORE_PATH
1010
} from "./paths.js";
@@ -23,10 +23,6 @@ export {
2323
DEFAULT_SWITCHBOARD_STORE_PATH
2424
} from "./paths.js";
2525

26-
const __filename = fileURLToPath(import.meta.url);
27-
const __dirname = path.dirname(__filename);
28-
const ANTHROPIC_TARGETS_PATH = path.join(__dirname, "..", "router", "data", "targets.anthropic.json");
29-
3026
function readJson(filePath) {
3127
return JSON.parse(fs.readFileSync(filePath, "utf8"));
3228
}

0 commit comments

Comments
 (0)