diff --git a/docs/superpowers/plans/2026-04-28-code-smell-cleanup.md b/docs/superpowers/plans/2026-04-28-code-smell-cleanup.md new file mode 100644 index 0000000..c01fcc7 --- /dev/null +++ b/docs/superpowers/plans/2026-04-28-code-smell-cleanup.md @@ -0,0 +1,1005 @@ +# Code Smell Cleanup Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix concrete code smells found in repo review without changing marketplace behavior or relaxing lint/type rules. + +**Architecture:** Start with correctness bugs at transport boundaries, then remove secret-leaking query/log paths, then reduce duplicate parsing and HTTP code. Keep marketplace behavior inside `packages/core`, API routes thin, and MCP as JSON-RPC transport only. + +**Tech Stack:** Bun `1.3.13`, TypeScript strict mode, `bun:test`, Biome, framework-free `Bun.serve` adapters. + +--- + +## File Structure + +- Modify: `packages/mcp-server/src/protocol/handler.ts` + - Fix JSON-RPC notification detection. + - Preserve numeric zero arguments. + - Extract shared API call/query-param helpers. + - Stop logging full URLs with cookie-bearing params. +- Modify: `packages/mcp-server/src/protocol/tools.ts` + - Remove `cookies` from Kijiji MCP schema or mark it as unsupported after API route no longer accepts it. +- Modify: `packages/mcp-server/test/protocol.test.ts` + - Add coverage for `id: 0`. + - Add coverage for zero-valued numeric args. + - Add coverage that no cookie input is exposed or forwarded. +- Modify: `packages/api-server/src/routes/kijiji.ts` + - Stop accepting `cookies` query param. + - Use strict integer parsing helper. + - Use shared route helpers. +- Modify: `packages/api-server/src/routes/facebook.ts` + - Use strict integer parsing helper. + - Use shared route helpers. +- Modify: `packages/api-server/src/routes/ebay.ts` + - Use strict integer parsing helper. + - Use shared route helpers. +- Create: `packages/api-server/src/routes/helpers.ts` + - Own shared route parsing and response helpers. +- Modify: `packages/api-server/test/routes.test.ts` + - Update Kijiji cookie expectations. + - Add strict-number validation tests. +- Modify: `packages/core/src/utils/http.ts` + - Add options for deterministic retry jitter and optional response URL return. + - Keep existing callers compatible. +- Modify: `packages/core/src/scrapers/facebook.ts` + - Replace local `HttpError` and `fetchHtml` with shared utility. + - Keep Facebook-specific headers/cookie behavior via options. +- Modify: `packages/core/src/scrapers/ebay.ts` + - Replace direct `fetch`/local `HttpError` with shared utility. + - Replace `console.error` with repo logger. +- Modify: `packages/core/src/scrapers/kijiji.ts` + - Delete dead `_parseListing` if tests confirm no callers. + - Replace direct `setTimeout` calls with `delay` helper. + - Replace `console.error` with repo logger. +- Modify: `packages/core/test/setup.ts` + - Remove redundant comments and make fetch-mock policy explicit. +- Test: existing package tests under `packages/core/test`, `packages/api-server/test`, `packages/mcp-server/test`. + +## Task 1: Fix MCP JSON-RPC `id: 0` Handling + +**Files:** +- Modify: `packages/mcp-server/src/protocol/handler.ts:61-74` +- Test: `packages/mcp-server/test/protocol.test.ts` + +- [ ] **Step 1: Write failing test for `id: 0`** + +Add to `packages/mcp-server/test/protocol.test.ts`: + +```ts +describe("MCP JSON-RPC ids", () => { + beforeEach(() => { + global.fetch = mock(() => + Promise.resolve(new Response(JSON.stringify([]), { status: 200 })), + ) as unknown as typeof fetch; + }); + + afterEach(() => { + global.fetch = originalFetch; + }); + + test("tools/list treats id 0 as a request id", async () => { + const response = await handleMcpRequest( + new Request("http://localhost", { + method: "POST", + body: JSON.stringify({ + jsonrpc: "2.0", + id: 0, + method: "tools/list", + }), + }), + ); + + expect(response.status).toBe(200); + const body = await response.json(); + expect(body.id).toBe(0); + expect(body.result.tools).toBeArray(); + }); +}); +``` + +- [ ] **Step 2: Run test to verify current behavior** + +Run: `bun test packages/mcp-server/test/protocol.test.ts` + +Expected: FAIL if `id: 0` is treated as notification or no body is returned. + +- [ ] **Step 3: Implement minimal fix** + +In `packages/mcp-server/src/protocol/handler.ts`, replace notification check: + +```ts +const isNotification = !("id" in body); + +if (isNotification) { + if (method === "notifications/initialized") { + return new Response(null, { status: 204 }); + } + if (method === "notifications/progress") { + return new Response(null, { status: 204 }); + } + return new Response(null, { status: 204 }); +} +``` + +- [ ] **Step 4: Run package test** + +Run: `bun test packages/mcp-server/test/protocol.test.ts` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add packages/mcp-server/src/protocol/handler.ts packages/mcp-server/test/protocol.test.ts +git commit -m "fix: preserve zero json-rpc ids" +``` + +## Task 2: Preserve Zero Numeric MCP Arguments + +**Files:** +- Modify: `packages/mcp-server/src/protocol/handler.ts:107-216` +- Test: `packages/mcp-server/test/protocol.test.ts` + +- [ ] **Step 1: Write failing tests for zero-valued params** + +Add to `packages/mcp-server/test/protocol.test.ts`: + +```ts +describe("MCP numeric argument forwarding", () => { + beforeEach(() => { + global.fetch = mock(() => + Promise.resolve(new Response(JSON.stringify([]), { status: 200 })), + ) as unknown as typeof fetch; + }); + + afterEach(() => { + global.fetch = originalFetch; + }); + + test("search_kijiji forwards maxPages 0 and price bounds 0", async () => { + await handleMcpRequest( + new Request("http://localhost", { + method: "POST", + body: JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { + name: "search_kijiji", + arguments: { + query: "laptop", + maxPages: 0, + priceMin: 0, + priceMax: 0, + }, + }, + }), + }), + ); + + const calledUrl = (global.fetch as unknown as ReturnType).mock + .calls[0]?.[0]; + expect(String(calledUrl)).toContain("maxPages=0"); + expect(String(calledUrl)).toContain("priceMin=0"); + expect(String(calledUrl)).toContain("priceMax=0"); + }); + + test("search_ebay forwards zero price bounds and maxItems", async () => { + await handleMcpRequest( + new Request("http://localhost", { + method: "POST", + body: JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { + name: "search_ebay", + arguments: { + query: "laptop", + minPrice: 0, + maxPrice: 0, + maxItems: 0, + }, + }, + }), + }), + ); + + const calledUrl = (global.fetch as unknown as ReturnType).mock + .calls[0]?.[0]; + expect(String(calledUrl)).toContain("minPrice=0"); + expect(String(calledUrl)).toContain("maxPrice=0"); + expect(String(calledUrl)).toContain("maxItems=0"); + }); +}); +``` + +- [ ] **Step 2: Run tests to verify failure** + +Run: `bun test packages/mcp-server/test/protocol.test.ts` + +Expected: FAIL because `0` params are absent. + +- [ ] **Step 3: Add helper and update checks** + +In `packages/mcp-server/src/protocol/handler.ts`, add near constants: + +```ts +function appendStringParam( + params: URLSearchParams, + key: string, + value: unknown, +): void { + if (typeof value === "string" && value.length > 0) { + params.append(key, value); + } +} + +function appendNumberParam( + params: URLSearchParams, + key: string, + value: unknown, +): void { + if (typeof value === "number" && Number.isFinite(value)) { + params.append(key, value.toString()); + } +} + +function appendBooleanParam( + params: URLSearchParams, + key: string, + value: unknown, +): void { + if (typeof value === "boolean") { + params.append(key, value.toString()); + } +} +``` + +Replace param appends with helpers: + +```ts +appendStringParam(params, "location", args.location); +appendStringParam(params, "category", args.category); +appendStringParam(params, "keywords", args.keywords); +appendStringParam(params, "sortBy", args.sortBy); +appendStringParam(params, "sortOrder", args.sortOrder); +appendNumberParam(params, "maxPages", args.maxPages); +appendNumberParam(params, "priceMin", args.priceMin); +appendNumberParam(params, "priceMax", args.priceMax); +appendBooleanParam(params, "unstableFilter", args.unstableFilter); +``` + +Use same pattern for Facebook and eBay numeric/boolean params. + +- [ ] **Step 4: Run tests** + +Run: `bun test packages/mcp-server/test/protocol.test.ts` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add packages/mcp-server/src/protocol/handler.ts packages/mcp-server/test/protocol.test.ts +git commit -m "fix: forward zero-valued mcp params" +``` + +## Task 3: Remove Cookie Query Path From MCP and API + +**Files:** +- Modify: `packages/mcp-server/src/protocol/tools.ts:55-59` +- Modify: `packages/mcp-server/src/protocol/handler.ts:119` +- Modify: `packages/api-server/src/routes/kijiji.ts:65` +- Test: `packages/mcp-server/test/protocol.test.ts` +- Test: `packages/api-server/test/routes.test.ts` + +- [ ] **Step 1: Update MCP tests for no cookie exposure** + +Replace existing cookie-input test in `packages/mcp-server/test/protocol.test.ts` with: + +```ts +test("search tools should not expose cookie inputs", () => { + const toolNames = ["search_kijiji", "search_facebook", "search_ebay"]; + + for (const toolName of toolNames) { + const tool = tools.find((candidate) => candidate.name === toolName); + expect(tool?.inputSchema.properties).not.toHaveProperty("cookies"); + expect(tool?.inputSchema.properties).not.toHaveProperty("cookiesSource"); + } +}); +``` + +Add forwarding test: + +```ts +test("search_kijiji should not forward cookies query parameters", async () => { + await handleMcpRequest( + new Request("http://localhost", { + method: "POST", + body: JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { + name: "search_kijiji", + arguments: { + query: "laptop", + cookies: "s=1", + }, + }, + }), + }), + ); + + const calledUrl = (global.fetch as unknown as ReturnType).mock + .calls[0]?.[0]; + expect(String(calledUrl)).toContain("/kijiji?q=laptop"); + expect(String(calledUrl)).not.toContain("cookies="); +}); +``` + +- [ ] **Step 2: Update API test expectation** + +In `packages/api-server/test/routes.test.ts`, replace `kijijiRoute passes cookies query parameter` test with: + +```ts +test("kijijiRoute ignores cookies query parameter", async () => { + const { kijijiRoute } = await import("../src/routes/kijiji"); + + await kijijiRoute( + new Request( + "http://localhost/api/kijiji?q=laptop&cookies=s%3D1&maxPages=3", + ), + ); + + expect(fetchKijijiItems).toHaveBeenCalledWith( + "laptop", + 4, + "https://www.kijiji.ca", + { + location: undefined, + category: undefined, + keywords: undefined, + sortBy: undefined, + sortOrder: undefined, + maxPages: 3, + priceMin: undefined, + priceMax: undefined, + }, + {}, + ); +}); +``` + +- [ ] **Step 3: Run tests to verify failure** + +Run: `bun test packages/mcp-server/test/protocol.test.ts packages/api-server/test/routes.test.ts` + +Expected: FAIL because Kijiji cookie query is still exposed/forwarded. + +- [ ] **Step 4: Remove Kijiji cookie schema and forwarding** + +Delete `cookies` property from `search_kijiji` in `packages/mcp-server/src/protocol/tools.ts`. + +Delete this line from `packages/mcp-server/src/protocol/handler.ts`: + +```ts +if (args.cookies) params.append("cookies", args.cookies); +``` + +Delete this field from `searchOptions` in `packages/api-server/src/routes/kijiji.ts`: + +```ts +cookies: reqUrl.searchParams.get("cookies") || undefined, +``` + +- [ ] **Step 5: Run tests** + +Run: `bun test packages/mcp-server/test/protocol.test.ts packages/api-server/test/routes.test.ts` + +Expected: PASS. + +- [ ] **Step 6: Commit** + +```bash +git add packages/mcp-server/src/protocol/tools.ts packages/mcp-server/src/protocol/handler.ts packages/mcp-server/test/protocol.test.ts packages/api-server/src/routes/kijiji.ts packages/api-server/test/routes.test.ts +git commit -m "fix: remove cookie query forwarding" +``` + +## Task 4: Add Strict API Integer Parsing + +**Files:** +- Create: `packages/api-server/src/routes/helpers.ts` +- Modify: `packages/api-server/src/routes/facebook.ts` +- Modify: `packages/api-server/src/routes/ebay.ts` +- Modify: `packages/api-server/src/routes/kijiji.ts` +- Test: `packages/api-server/test/routes.test.ts` + +- [ ] **Step 1: Write failing API validation tests** + +Add to `packages/api-server/test/routes.test.ts`: + +```ts +test("facebookRoute rejects non-integer maxItems", async () => { + const { facebookRoute } = await import("../src/routes/facebook"); + + const response = await facebookRoute( + new Request("http://localhost/api/facebook?q=laptop&maxItems=10abc"), + ); + + expect(response.status).toBe(400); + const body = await response.json(); + expect(body.message).toBe("Invalid maxItems parameter"); +}); + +test("kijijiRoute rejects decimal maxPages", async () => { + const { kijijiRoute } = await import("../src/routes/kijiji"); + + const response = await kijijiRoute( + new Request("http://localhost/api/kijiji?q=laptop&maxPages=1.5"), + ); + + expect(response.status).toBe(400); + const body = await response.json(); + expect(body.message).toBe("Invalid maxPages parameter"); +}); + +test("ebayRoute rejects non-integer price params", async () => { + const { ebayRoute } = await import("../src/routes/ebay"); + + const response = await ebayRoute( + new Request("http://localhost/api/ebay?q=laptop&minPrice=10abc"), + ); + + expect(response.status).toBe(400); + const body = await response.json(); + expect(body.message).toBe("Invalid minPrice parameter"); +}); +``` + +- [ ] **Step 2: Run tests to verify failure** + +Run: `bun test packages/api-server/test/routes.test.ts` + +Expected: FAIL because `parseInt` accepts junk/decimals. + +- [ ] **Step 3: Create route helper** + +Create `packages/api-server/src/routes/helpers.ts`: + +```ts +export function getRequiredSearchQuery(req: Request): string | Response { + const reqUrl = new URL(req.url); + const query = req.headers.get("query") || reqUrl.searchParams.get("q"); + + if (!query) { + return Response.json( + { message: "Request didn't have 'query' header or 'q' search parameter!" }, + { status: 400 }, + ); + } + + return query; +} + +export function parseNonNegativeIntegerParam( + searchParams: URLSearchParams, + name: string, + defaultValue?: number, +): number | undefined | Response { + const rawValue = searchParams.get(name); + if (rawValue === null) { + return defaultValue; + } + + const value = Number(rawValue); + if (!Number.isInteger(value) || value < 0) { + return Response.json( + { message: `Invalid ${name} parameter` }, + { status: 400 }, + ); + } + + return value; +} + +export function emptySearchResponse(): Response { + return Response.json( + { message: "Search didn't return any results!" }, + { status: 404 }, + ); +} +``` + +- [ ] **Step 4: Update routes to use helper** + +In each route: + +```ts +const reqUrl = new URL(req.url); +const searchQuery = getRequiredSearchQuery(req); +if (searchQuery instanceof Response) return searchQuery; +``` + +Use integer helper for each numeric param: + +```ts +const maxItems = parseNonNegativeIntegerParam(reqUrl.searchParams, "maxItems", 25); +if (maxItems instanceof Response) return maxItems; +``` + +For optional params: + +```ts +const minPrice = parseNonNegativeIntegerParam(reqUrl.searchParams, "minPrice"); +if (minPrice instanceof Response) return minPrice; +``` + +Replace empty-result response blocks with `return emptySearchResponse();`. + +- [ ] **Step 5: Run tests** + +Run: `bun test packages/api-server/test/routes.test.ts` + +Expected: PASS. + +- [ ] **Step 6: Run adapter build** + +Run: `bun run --cwd packages/api-server build` + +Expected: PASS. + +- [ ] **Step 7: Commit** + +```bash +git add packages/api-server/src/routes/helpers.ts packages/api-server/src/routes/facebook.ts packages/api-server/src/routes/ebay.ts packages/api-server/src/routes/kijiji.ts packages/api-server/test/routes.test.ts +git commit -m "fix: strictly parse route integers" +``` + +## Task 5: De-Duplicate MCP API Calls + +**Files:** +- Modify: `packages/mcp-server/src/protocol/handler.ts` +- Test: `packages/mcp-server/test/protocol.test.ts` + +- [ ] **Step 1: Add regression test for successful tool result after helper extraction** + +Add to `packages/mcp-server/test/protocol.test.ts`: + +```ts +test("tools/call returns API JSON as text content", async () => { + global.fetch = mock(() => + Promise.resolve( + new Response(JSON.stringify([{ title: "item" }]), { status: 200 }), + ), + ) as unknown as typeof fetch; + + const response = await handleMcpRequest( + new Request("http://localhost", { + method: "POST", + body: JSON.stringify({ + jsonrpc: "2.0", + id: 1, + method: "tools/call", + params: { + name: "search_facebook", + arguments: { query: "laptop" }, + }, + }), + }), + ); + + const body = await response.json(); + expect(body.result.content[0].type).toBe("text"); + expect(JSON.parse(body.result.content[0].text)).toEqual([{ title: "item" }]); +}); +``` + +- [ ] **Step 2: Run MCP tests** + +Run: `bun test packages/mcp-server/test/protocol.test.ts` + +Expected: PASS before refactor. + +- [ ] **Step 3: Extract API call helper** + +In `packages/mcp-server/src/protocol/handler.ts`, add: + +```ts +async function callMarketplaceApi( + marketplace: string, + params: URLSearchParams, +): Promise { + const path = `${API_BASE_URL}/${marketplace}`; + const url = `${path}?${params.toString()}`; + + logger.log(`[MCP] Calling ${marketplace} API`); + const response = await Promise.race([ + fetch(url), + new Promise((_, reject) => + setTimeout( + () => reject(new Error(`Request timed out after ${API_TIMEOUT}ms`)), + API_TIMEOUT, + ), + ), + ]); + + if (!response.ok) { + const errorText = await response.text(); + logger.error(`[MCP] ${marketplace} API error ${response.status}: ${errorText}`); + throw new Error(`API returned ${response.status}: ${errorText}`); + } + + return response.json(); +} +``` + +Replace three repeated `Promise.race` blocks with: + +```ts +result = await callMarketplaceApi("kijiji", params); +``` + +Use `"facebook"` and `"ebay"` in their branches. + +- [ ] **Step 4: Run MCP tests and build** + +Run: `bun test packages/mcp-server/test/protocol.test.ts && bun run --cwd packages/mcp-server build` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add packages/mcp-server/src/protocol/handler.ts packages/mcp-server/test/protocol.test.ts +git commit -m "refactor: share mcp api calls" +``` + +## Task 6: Consolidate Core HTTP Fetching + +**Files:** +- Modify: `packages/core/src/utils/http.ts` +- Modify: `packages/core/src/scrapers/facebook.ts` +- Modify: `packages/core/src/scrapers/ebay.ts` +- Test: `packages/core/test/http.test.ts` +- Test: `packages/core/test/facebook-core.test.ts` +- Test: `packages/core/test/ebay-core.test.ts` + +- [ ] **Step 1: Add shared HTTP test for response URL and deterministic jitter** + +Add to `packages/core/test/http.test.ts`: + +```ts +test("fetchHtml can return response URL", async () => { + global.fetch = mock(() => + Promise.resolve( + new Response("", { + status: 200, + headers: {}, + }), + ), + ) as unknown as typeof fetch; + + Object.defineProperty((global.fetch as unknown as ReturnType).mock.calls[0] ?? {}, "url", { + value: "https://example.test/final", + }); + + const result = await fetchHtml("https://example.test", 0, { + includeResponseUrl: true, + jitter: () => 0, + }); + + expect(result.html).toBe(""); + expect(result.responseUrl).toBe("https://example.test"); +}); +``` + +If current `Response.url` cannot be set in Bun tests, use a mocked object cast to `Response` instead: + +```ts +global.fetch = mock(() => + Promise.resolve({ + ok: true, + status: 200, + url: "https://example.test/final", + headers: { get: () => null }, + text: () => Promise.resolve(""), + }), +) as unknown as typeof fetch; +``` + +- [ ] **Step 2: Run HTTP tests to verify failure** + +Run: `bun test packages/core/test/http.test.ts` + +Expected: FAIL because options do not exist yet. + +- [ ] **Step 3: Extend `FetchHtmlOptions`** + +In `packages/core/src/utils/http.ts`: + +```ts +export interface FetchHtmlResult { + html: HTMLString; + responseUrl: string; +} + +export interface FetchHtmlOptions { + maxRetries?: number; + retryBaseMs?: number; + timeoutMs?: number; + onRateInfo?: (remaining: string | null, reset: string | null) => void; + headers?: Record; + includeResponseUrl?: boolean; + jitter?: () => number; +} +``` + +Update `calculateBackoffDelay`: + +```ts +function calculateBackoffDelay( + attempt: number, + baseMs: number, + jitter: () => number, +): number { + const exponentialDelay = baseMs * 2 ** attempt; + const jitterDelay = jitter() * 0.1 * exponentialDelay; + return Math.min(exponentialDelay + jitterDelay, 30000); +} +``` + +Keep compatible overloads: + +```ts +export async function fetchHtml( + url: string, + delayMs: number, + opts: FetchHtmlOptions & { includeResponseUrl: true }, +): Promise; +export async function fetchHtml( + url: string, + delayMs: number, + opts?: FetchHtmlOptions, +): Promise; +``` + +Return either string or result: + +```ts +const html = await res.text(); +await delay(delayMs); +return opts?.includeResponseUrl ? { html, responseUrl: res.url || url } : html; +``` + +- [ ] **Step 4: Replace Facebook local HTTP client** + +In `packages/core/src/scrapers/facebook.ts`: + +Remove local `HttpError` class and local `fetchHtml` function. + +Import shared helpers: + +```ts +import { fetchHtml, HttpError, isRecord } from "../utils/http"; +``` + +Create Facebook headers constant or local function: + +```ts +function createFacebookHeaders(cookies: string): Record { + return { + accept: + "text/html,application/xhtml+xml,application/xml;q=0.9,image/avif,image/webp,image/apng,*/*;q=0.8,application/signed-exchange;v=b3;q=0.7", + "accept-language": "en-GB,en-US;q=0.9,en;q=0.8", + "cache-control": "no-cache", + "upgrade-insecure-requests": "1", + "sec-fetch-dest": "document", + "sec-fetch-mode": "navigate", + "sec-fetch-site": "none", + "sec-fetch-user": "?1", + "user-agent": + "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/120.0.0.0 Safari/537.36", + cookie: cookies, + }; +} +``` + +Replace search fetch: + +```ts +const response = await fetchHtml(searchUrl, delayMs, { + maxRetries: 3, + includeResponseUrl: true, + headers: createFacebookHeaders(cookiesHeader), + onRateInfo: (remaining, reset) => { + if (remaining && reset) { + logger.log( + `\nFacebook - Rate limit remaining: ${remaining}, reset in: ${reset}s`, + ); + } + }, +}); +``` + +Update error property reads from `err.status` to `err.statusCode`. + +- [ ] **Step 5: Replace eBay direct fetch with shared helper** + +In `packages/core/src/scrapers/ebay.ts`, import `fetchHtml` and `HttpError` from `../utils/http`. + +Replace direct `fetch` block with: + +```ts +const searchHtml = await fetchHtml(searchUrl, delayMs, { + headers, +}); +``` + +Replace `console.error` with: + +```ts +logger.error(`Failed to fetch eBay search (${err.statusCode}): ${err.message}`); +``` + +- [ ] **Step 6: Run core tests** + +Run: `bun test packages/core/test/http.test.ts packages/core/test/facebook-core.test.ts packages/core/test/ebay-core.test.ts` + +Expected: PASS. + +- [ ] **Step 7: Run core build/typecheck** + +Run: `bun run --cwd packages/core build && bun run ci` + +Expected: PASS. + +- [ ] **Step 8: Commit** + +```bash +git add packages/core/src/utils/http.ts packages/core/src/scrapers/facebook.ts packages/core/src/scrapers/ebay.ts packages/core/test/http.test.ts packages/core/test/facebook-core.test.ts packages/core/test/ebay-core.test.ts +git commit -m "refactor: share scraper http fetching" +``` + +## Task 7: Clean Kijiji Dead Code and Logging + +**Files:** +- Modify: `packages/core/src/scrapers/kijiji.ts` +- Test: `packages/core/test/kijiji-core.test.ts` +- Test: `packages/core/test/kijiji-integration.test.ts` + +- [ ] **Step 1: Verify `_parseListing` has no callers** + +Run: `rg "_parseListing|parseListing" packages/core packages/api-server packages/mcp-server` + +Expected: only `_parseListing` definition appears. If any caller appears, stop and update this task to preserve behavior. + +- [ ] **Step 2: Delete dead function** + +Remove `function _parseListing(...)` block from `packages/core/src/scrapers/kijiji.ts`. + +- [ ] **Step 3: Replace direct timers and console logging** + +Replace: + +```ts +await new Promise((resolve) => + setTimeout(resolve, DELAY_MS * batchIndex), +); +``` + +with: + +```ts +await delay(DELAY_MS * batchIndex); +``` + +Replace: + +```ts +await new Promise((resolve) => setTimeout(resolve, DELAY_MS)); +``` + +with: + +```ts +await delay(DELAY_MS); +``` + +Replace `console.error(...)` calls with `logger.error(...)` preserving message text. + +- [ ] **Step 4: Run Kijiji tests** + +Run: `bun test packages/core/test/kijiji-core.test.ts packages/core/test/kijiji-integration.test.ts` + +Expected: PASS. + +- [ ] **Step 5: Commit** + +```bash +git add packages/core/src/scrapers/kijiji.ts +git commit -m "refactor: clean kijiji scraper internals" +``` + +## Task 8: Clean Test Setup Comments and Enforce Fetch Mocking + +**Files:** +- Modify: `packages/core/test/setup.ts` +- Test: core test suite + +- [ ] **Step 1: Update setup file** + +Replace `packages/core/test/setup.ts` with: + +```ts +global.fetch = (() => { + throw new Error("Tests must mock fetch explicitly"); +}) as typeof fetch; +``` + +- [ ] **Step 2: Run core tests** + +Run: `bun test packages/core/test` + +Expected: PASS. If failures occur, fix individual tests by mocking `global.fetch` in `beforeEach` and restoring in `afterEach`. + +- [ ] **Step 3: Commit** + +```bash +git add packages/core/test/setup.ts packages/core/test +git commit -m "test: require explicit fetch mocks" +``` + +## Task 9: Final Verification + +**Files:** +- Verify all touched packages. + +- [ ] **Step 1: Run full deterministic tests** + +Run: `bun test packages/core/test packages/api-server/test packages/mcp-server/test` + +Expected: PASS. + +- [ ] **Step 2: Run CI checks** + +Run: `bun run ci` + +Expected: PASS. + +- [ ] **Step 3: Run build** + +Run: `bun run build` + +Expected: PASS. + +- [ ] **Step 4: Inspect diff** + +Run: `git diff --stat && git diff --check` + +Expected: no whitespace errors, diff limited to planned files. + +- [ ] **Step 5: Commit verification fixes if needed** + +If verification required fixes: + +```bash +git add +git commit -m "chore: finish code smell cleanup" +``` + +## Self-Review + +- Spec coverage: all reviewed smells are covered: JSON-RPC id bug, zero args, cookie query leak, strict integer parsing, duplicate route/MCP helper code, duplicate HTTP clients, dead Kijiji function, direct timers/logging, stale setup comments. +- Placeholder scan: no TBD/TODO/fill-in placeholders remain. Each task has target files, code snippets, commands, and expected results. +- Type consistency: route helper names, MCP helper names, and shared HTTP option names are used consistently across tasks. + +## Execution Handoff + +Plan complete and saved to `docs/superpowers/plans/2026-04-28-code-smell-cleanup.md`. + +Two execution options: + +1. Subagent-Driven (recommended) - dispatch fresh subagent per task, review between tasks, fast iteration. +2. Inline Execution - execute tasks in this session using executing-plans, batch execution with checkpoints.