From 3024c62622583006b3113b04671a013a1617c3a8 Mon Sep 17 00:00:00 2001 From: ohm Date: Tue, 4 Aug 2026 09:20:40 +0700 Subject: [PATCH 01/14] docs: add repo cleanup design spec Covers removal of 641 lines of unreachable code (including the unused src/services scaffolding), a Vitest harness for the pure networking and routing logic, and six correctness fixes led by the routing lab's tied-cost route mislabelling. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 --- .../specs/2026-08-04-repo-cleanup-design.md | 278 ++++++++++++++++++ 1 file changed, 278 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-04-repo-cleanup-design.md diff --git a/docs/superpowers/specs/2026-08-04-repo-cleanup-design.md b/docs/superpowers/specs/2026-08-04-repo-cleanup-design.md new file mode 100644 index 0000000..44cb59f --- /dev/null +++ b/docs/superpowers/specs/2026-08-04-repo-cleanup-design.md @@ -0,0 +1,278 @@ +# Repo cleanup: dead code removal and correctness fixes + +Date: 2026-08-04 +Status: approved + +## Problem + +A sweep of the repository found two distinct classes of issue. + +**Dead code.** 641 of 9,153 source lines (7%) are unreachable. The largest block is +`src/services/` — 12 files of placeholder contracts for auth, payments, RBAC, audit, +database, quiz, progress, analytics, rate limiting and session rotation, none of which +is imported anywhere. AckLab is a static Next.js site with no API routes, no database +and no authentication. The directory is documented as intentional in `README.md` and +`docs/architecture.md`, but its contracts (`FutureUser`, `PaymentCheckoutRequest`) are +guesses that would be rewritten when real requirements arrive. Git history preserves +them if they are ever wanted back. + +**Bugs.** Two are user-visible, and both sit in explanatory text — the part of a +teaching tool that most needs to be correct. A third header is inert security theatre. + +Neither `pnpm lint` nor `pnpm typecheck` catches any of this; both pass clean today. +The repository has no test framework at all, so the routing fix — subtle comparison +logic — would otherwise land with no safety net. + +## Goals + +- Remove all verified-unreachable code, and update the docs that describe it. +- Fix the confirmed bugs, test-first where behaviour changes. +- Leave behind a test harness so the routing bug cannot silently return. + +## Non-goals + +- No CSP nonce middleware. Hardening `script-src` past `'unsafe-inline'`/`'unsafe-eval'` + needs per-request nonces and careful handling of Next's bootstrap and Framer Motion's + style injection. That is a project of its own. +- No component or DOM testing. Pure functions only. +- No unrelated refactoring. `findShortestPath` re-sorts its unvisited set each iteration + (O(V² log V)); it is correct and fine at six nodes, and is left alone. + +## Approach + +Three phases, in order: delete, then add the test harness, then fix. + +Deleting first means the harness is written against the final shape of the codebase — +otherwise we would write a spec file for `routing-utils.ts`, which is itself dead. The +deletions are mechanical and verified-unused, so they review quickly and do not obscure +the small logic diffs that follow. + +Each phase is its own commit, so a subtle tie-break change never shares a revert +boundary with a 641-line deletion. + +## Phase 1 — Deletions + +### Files removed entirely + +All verified to have zero importers. + +| Path | Lines | +| -------------------------------------------------------- | ------- | +| `src/services/**` (12 files) | 184 | +| `src/data/platform.ts` | 156 | +| `src/components/visualizations/event-timeline.tsx` | 63 | +| `src/components/visualizations/packet-inspector.tsx` | 58 | +| `src/components/visualizations/network-background.tsx` | 53 | +| `src/components/visualizations/bit-grid.tsx` | 41 | +| `src/components/visualizations/protocol-state-panel.tsx` | 27 | +| `src/components/shared/metric-card.tsx` | 21 | +| `src/data/routing.ts` | 16 | +| `src/utils/format.ts` | 3 | +| `src/features/routing/lib/routing-utils.ts` | 1 | +| **Subtotal** | **623** | + +The remaining 18 lines of the 641 total are `page-header.tsx`, removed by the collapse +described below. + +`src/data/routing.ts` is superseded by `src/features/routing/data/routing-topology.ts`. +`src/utils/format.ts` exports `formatNumber`, which nothing calls — +`subnet-visualizer.tsx:173` uses `.toLocaleString()` directly. Removing it empties +`src/utils/`, so that directory goes too. + +### Pass-through collapsed + +`src/components/shared/page-header.tsx` forwards its four props to `LearningPageHeader` +and adds nothing. Eleven of the twelve routes import the wrapper — every page except the +root `app/page.tsx` — and nothing imports the wrapped component. + +- Delete `page-header.tsx`. +- Rename the export in `learning-page-header.tsx` from `LearningPageHeader` to + `PageHeader`, and rename the file to `page-header.tsx`. +- Repoint the 11 importing pages under `app/`. + +Net effect: one component with one name, and the import path every page already uses. + +### Configuration + +`src/config/env.ts` loses the four env slots that exist only for the deleted services: +`AUTH_SECRET`, `DATABASE_URL`, `STRIPE_SECRET_KEY`, `ANALYTICS_WRITE_KEY`. `.env.example` +and `.env` lose the same keys. The schema retains `NODE_ENV` and `NEXT_PUBLIC_APP_URL`. + +### Documentation + +- `docs/architecture.md`: delete the services section (lines 20–35). +- `README.md`: delete the `src/services/` and `src/utils/` lines from the architecture + tree, and the closing "Future infrastructure concerns are represented by interface + contracts in `src/services/`" paragraph. +- The README sentence stating the product is "intentionally local-mock only" stays — + it remains true and is not tied to the services directory. + +## Phase 2 — Test harness + +Vitest with `environment: "node"`. Nothing under test touches React or the DOM, so no +jsdom and no testing-library. + +- One new dev dependency: `vitest`. +- The `@/` alias is declared directly in `vitest.config.ts`, avoiding a + `vite-tsconfig-paths` dependency. +- `describe`/`it`/`expect` are imported explicitly rather than enabling globals, which + keeps `tsc --noEmit` clean without a types entry in `tsconfig.json`. +- `package.json` gains `"test": "vitest run"`. +- CI gains a Test step between Typecheck and Build in `.github/workflows/ci.yml`. +- `precommit` becomes `lint-staged && pnpm typecheck && pnpm test`. Pure-function specs + run in milliseconds, so commits stay responsive. + +Five spec files, colocated with their subjects: + +| Spec | Covers | +| -------------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------- | +| `src/features/routing/lib/path-candidates.test.ts` | tie-break puts selected first; delta-0 wording; selected survives `slice(0, limit)`; unreachable yields a single `unavailable` candidate | +| `src/features/routing/lib/shortest-path.test.ts` | happy path; disabled links excluded; unreachable pair; `start === end` | +| `src/lib/networking/ipv4.test.ts` | valid and invalid parses; leading zeros rejected; `ipv4ToInt`/`intToIpv4` round-trip with a first octet ≥ 128 | +| `src/features/subnet/lib/subnet-utils.test.ts` | `/24`, `/0`, and the `/31` and `/32` special cases | +| `src/features/binary/lib/binary-utils.test.ts` | `binaryToDecimal` valid, over-length, non-binary | + +`path-candidates`, `ipv4` and `binary-utils` specs are written to fail against current +code, then made to pass in Phase 3. `shortest-path` and `subnet-utils` are +characterization tests pinning behaviour that is _not_ changing, so the deletions and +refactors cannot disturb them unnoticed. + +## Phase 3 — Fixes + +### 3.1 Routing tie-break — `src/features/routing/lib/path-candidates.ts` + +Confirmed against the real topology in the default state, before the user interacts: + +``` +dijkstra path: client->r1->r3->server cost: 7 + [alternative] cost=7 client->r1->r3->r4->server :: Costs 0 more than selected. + [selected ] cost=7 client->r1->r3->server :: Lowest total cost. + [alternative] cost=8 client->r2->r4->server :: Costs 1 more than selected. +``` + +Two defects. `enumerateSimplePaths` sorts by cost alone, so an equally-cheap route +outranks the one Dijkstra chose — a panel headed "Why this route wins" leads with a +route that did not win. And the reason string is a bare subtraction with no zero case, +producing "Costs 0 more than selected." + +Three coupled changes: + +1. Secondary sort key, so the selected route wins its cost tier: + + ```ts + .sort((a, b) => + a.cost - b.cost || + Number(samePath(b.path, selectedPath)) - Number(samePath(a.path, selectedPath))) + ``` + + Because Dijkstra's path is always minimum-cost, this also places it at index 0, + so it always survives `.slice(0, limit)`. The "selected route absent from the list" + case becomes unreachable rather than special-cased. + +2. Hoist `selectedCost` out of the `map` — it is loop-invariant and currently + recomputed once per candidate (line 47). + +3. Guard becomes `selectedCost !== undefined` instead of a falsy check, so a legitimate + cost of 0 stops falling through to the wrong branch (line 56). The reason string gains + the zero case ("Same cost, different path.") and a negative case, so a non-optimal + `selectedPath` from a future caller cannot produce "Costs -3 more". + +### 3.2 Binary converter NaN — `src/features/binary/components/binary-converter.tsx` + +`register("decimal", { valueAsNumber: true })` yields `NaN` for an empty number input. +`values.decimal ?? 0` catches only `null`/`undefined`, so `NaN` reaches line 117 and the +user sees `NaN = 00000000`. Line 51 guards the conversion but not the display. + +Derive one clamped octet and render both sides from it: + +```ts +const octet = clampOctet(Number.isFinite(values.decimal) ? values.decimal! : 0); +``` + +Decimal and binary then cannot disagree. Typing `300` reads `255 = 11111111` — +self-consistent — while Zod still surfaces the out-of-range error. Today it reads +`300 = 11111111`, a smaller lie of the same kind. + +### 3.3 ARP step index — `src/features/arp/hooks/use-arp-simulation.ts` + +`stepIndex >= 5` happens to align with the `cache-update` step today. Inserting or +reordering a step in `arp-steps.ts` moves the cache update to the wrong moment, with no +type or test to catch it. The same block hardcodes `"192.168.1.25"` and +`"BB:BB:BB:BB:BB:25"`, duplicating `arpHosts` — which already carries both fields for +`host-b` — while `initialCache` ten lines above derives its IP from that same source. + +Replace with a module-level `arpSteps.findIndex((step) => step.id === "cache-update")`, +and build the learned entry from `arpHosts.find((host) => host.id === "host-b")`. + +### 3.4 IPv4 leading zeros — `src/lib/networking/ipv4.ts` + +`/^\d+$/` accepts `192.168.001.1` and parses it as `192.168.1.1`. Real stacks reject +leading zeros or read them as octal. Tighten to `/^(0|[1-9]\d*)$/`. + +This is a deliberate behaviour change: input a learner is mid-way through typing may now +be rejected where it previously was not. Accepted because the app teaches IPv4 +addressing, and being quietly wrong about address parsing matters more here than the +transient input case. Both consumers (`subnet-visualizer`, `binary-converter`) already +render a validation message on parse failure, so the failure mode is a visible error, +not a blank panel. + +### 3.5 CSP enforcement — `next.config.ts` + +The policy ships as `Content-Security-Policy-Report-Only` with no `report-uri` or +`report-to` directive. Report-only does not block, and with no endpoint nothing is +collected — the header does nothing at all. + +Rename the key to `Content-Security-Policy`, directives unchanged. Because `script-src` +already permits `'unsafe-inline'` and `'unsafe-eval'`, Next's inline bootstrap keeps +working. The gain is real enforcement of `frame-ancestors`, `object-src`, `base-uri`, +`form-action` and `connect-src` on a site that loads zero external resources. The weak +`script-src` is a known limitation, recorded under Non-goals. + +### 3.6 env.ts static replacement — `src/config/env.ts` + +`safeParse(process.env)` reads the environment object dynamically. Next inlines +`NEXT_PUBLIC_*` only by statically replacing the literal expression, so from a client +component this would parse `{}`, hit the `.default()`, and silently yield +`http://localhost:3000` in production. + +Not a live bug — `appConfig` is imported only by `app/layout.tsx`, a server component. +Fixed anyway because it fails silently rather than loudly the moment someone crosses +that boundary. Read `process.env.NEXT_PUBLIC_APP_URL` as a literal expression so the +value survives static replacement. + +## Verification + +After each phase: `pnpm lint && pnpm typecheck && pnpm test && pnpm build`. + +Tests prove nothing about the CSP change, which only manifests at runtime. After Phase 3, +run the production build and load all 12 routes, confirming a clean console with no CSP +violations: + +`/`, `/learn`, `/learn/start`, `/learn/[module]`, `/tools`, `/tools/binary`, +`/tools/subnet`, `/visualizers/osi-model`, `/visualizers/dns-flow`, +`/visualizers/arp-broadcast`, `/visualizers/routing`, `/visualizers/tcp-handshake`. + +The routing lab gets a manual check in its default state: the comparison panel must list +`Client -> R1 -> R3 -> Server` first, marked `selected`, with the equal-cost +`Client -> R1 -> R3 -> R4 -> Server` below it reading "Same cost, different path." + +## Commits + +Conventional commits, matching existing history: + +1. `chore: remove unused service scaffolding and dead code` +2. `test: add vitest coverage for networking and routing logic` +3. `fix(routing): rank tied-cost routes behind the selected path` +4. `fix(binary): render a consistent octet for empty and out-of-range input` +5. `refactor(arp): derive cache-update step and host data from source` +6. `fix(networking): reject IPv4 octets with leading zeros` +7. `fix(security): enforce CSP instead of report-only` + +Commits 3, 4 and 6 each carry their failing-test-then-fix pair. + +## Expected outcome + +- 641 lines removed; roughly 8,500 source lines remain. +- Five spec files and a `pnpm test` step in CI where there was no test framework before. +- Two user-visible explanatory bugs fixed, one inert security header made real, and two + latent traps (ARP index coupling, env static replacement) closed. From fe68876977c06f8bc98cd9d23b7fe4e4aec5662d Mon Sep 17 00:00:00 2001 From: ohm Date: Tue, 4 Aug 2026 09:24:43 +0700 Subject: [PATCH 02/14] docs: correct and expand repo cleanup spec after self-review Fixes three internal contradictions: the verification gate ran pnpm test before Phase 2 creates it, the approach section claimed one commit per phase against a seven-commit list, and the env.ts fix (3.6) had no commit assigned. Corrects an overstated claim: the falsy-guard and negative-delta changes in 3.1 are defensive only. The cost slider is min=1 and endpoints cannot be equal, so neither case is reachable today. Adds a live bug found while reviewing: z.number() accepts 1.5, and octetToBinary(1.5) returns "000001.1", so the converter silently renders a fractional binary string with no validation error. clampOctet moves to binary-utils.ts so the pure-logic spec can cover it. Also removes src/styles/ (a lone README, nothing imports it) for the same reason src/utils/ goes, defines clampOctet rather than leaving it dangling, narrows with typeof instead of a non-null assertion, and notes that .env is gitignored so that edit stays local. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 --- .../specs/2026-08-04-repo-cleanup-design.md | 97 ++++++++++++++----- 1 file changed, 73 insertions(+), 24 deletions(-) diff --git a/docs/superpowers/specs/2026-08-04-repo-cleanup-design.md b/docs/superpowers/specs/2026-08-04-repo-cleanup-design.md index 44cb59f..d279ce6 100644 --- a/docs/superpowers/specs/2026-08-04-repo-cleanup-design.md +++ b/docs/superpowers/specs/2026-08-04-repo-cleanup-design.md @@ -47,8 +47,9 @@ otherwise we would write a spec file for `routing-utils.ts`, which is itself dea deletions are mechanical and verified-unused, so they review quickly and do not obscure the small logic diffs that follow. -Each phase is its own commit, so a subtle tie-break change never shares a revert -boundary with a 641-line deletion. +Phases 1 and 2 are one commit each. Phase 3 splits into one commit per fix, so a subtle +tie-break change never shares a revert boundary with a 641-line deletion. Seven commits +in total; see the Commits section. ## Phase 1 — Deletions @@ -79,6 +80,11 @@ described below. `subnet-visualizer.tsx:173` uses `.toLocaleString()` directly. Removing it empties `src/utils/`, so that directory goes too. +`src/styles/` is the same case already in progress: it holds a lone `README.md` and no +stylesheet, and nothing imports it — all styling lives in `app/globals.css`. It is removed +for consistency. `src/hooks/README.md` stays, because that directory still holds +`use-hydrated.ts` and `use-learning-progress.ts`. + ### Pass-through collapsed `src/components/shared/page-header.tsx` forwards its four props to `LearningPageHeader` @@ -98,6 +104,12 @@ Net effect: one component with one name, and the import path every page already `AUTH_SECRET`, `DATABASE_URL`, `STRIPE_SECRET_KEY`, `ANALYTICS_WRITE_KEY`. `.env.example` and `.env` lose the same keys. The schema retains `NODE_ENV` and `NEXT_PUBLIC_APP_URL`. +`.env` is gitignored, so that edit is local-only and will not appear in the diff. Both +files must still be changed together, or local and documented configuration drift apart. + +Phase 1 also carries the §3.6 env.ts change, since both edit the same file and separating +them would mean touching `env.ts` twice for one logical cleanup. + ### Documentation - `docs/architecture.md`: delete the services section (lines 20–35). @@ -130,7 +142,7 @@ Five spec files, colocated with their subjects: | `src/features/routing/lib/shortest-path.test.ts` | happy path; disabled links excluded; unreachable pair; `start === end` | | `src/lib/networking/ipv4.test.ts` | valid and invalid parses; leading zeros rejected; `ipv4ToInt`/`intToIpv4` round-trip with a first octet ≥ 128 | | `src/features/subnet/lib/subnet-utils.test.ts` | `/24`, `/0`, and the `/31` and `/32` special cases | -| `src/features/binary/lib/binary-utils.test.ts` | `binaryToDecimal` valid, over-length, non-binary | +| `src/features/binary/lib/binary-utils.test.ts` | `binaryToDecimal` valid, over-length, non-binary; `clampOctet` on `NaN`, negatives, `>255`, and fractions (`1.5 → 1`) | `path-candidates`, `ipv4` and `binary-utils` specs are written to fail against current code, then made to pass in Phase 3. `shortest-path` and `subnet-utils` are @@ -172,23 +184,53 @@ Three coupled changes: 2. Hoist `selectedCost` out of the `map` — it is loop-invariant and currently recomputed once per candidate (line 47). -3. Guard becomes `selectedCost !== undefined` instead of a falsy check, so a legitimate - cost of 0 stops falling through to the wrong branch (line 56). The reason string gains - the zero case ("Same cost, different path.") and a negative case, so a non-optimal - `selectedPath` from a future caller cannot produce "Costs -3 more". +3. The reason string gains the zero case ("Same cost, different path."), which is the + live defect — it is what produces "Costs 0 more than selected." in the default state. + + Alongside it, two defensive changes that fix nothing reachable today but remove traps: + the guard becomes `selectedCost !== undefined` rather than a falsy check, and a + negative delta is handled rather than rendering "Costs -3 more". Neither case can occur + in the current app — the cost slider is `min={1}` (`routing-control-panel.tsx:150`) and + source and destination cannot be equal, so a path cost of 0 is unreachable, and + `selectedPath` always arrives from Dijkstra and so is always optimal. They are worth + doing because both assumptions are invisible at the call site, but they should not be + mistaken for bug fixes. ### 3.2 Binary converter NaN — `src/features/binary/components/binary-converter.tsx` -`register("decimal", { valueAsNumber: true })` yields `NaN` for an empty number input. -`values.decimal ?? 0` catches only `null`/`undefined`, so `NaN` reaches line 117 and the -user sees `NaN = 00000000`. Line 51 guards the conversion but not the display. +Two live defects, same root cause: the displayed decimal and the displayed binary are +derived independently, so they can disagree. -Derive one clamped octet and render both sides from it: +**Empty input.** `register("decimal", { valueAsNumber: true })` yields `NaN` for an empty +number input. `values.decimal ?? 0` catches only `null`/`undefined`, so `NaN` reaches +line 117 and the user sees `NaN = 00000000`. Line 51 guards the conversion but not the +display. + +**Fractional input.** `z.number().min(0).max(255)` accepts `1.5`, so no validation error +is raised, and `octetToBinary(1.5)` returns `"000001.1"` — `(1.5).toString(2)` is `"1.1"`, +which `padStart` pads rather than rejects. The user sees `1.5 = 000001.1`, and +`BitGroupVisualizer` is handed that string as if it were eight bits. Found while +reviewing this spec, not in the original sweep. + +Both are fixed by deriving one clamped integer octet and rendering both sides from it. +`Number.isFinite` is not a TypeScript type guard and cannot narrow `number | undefined`, +so the check leads with `typeof`, which does narrow and avoids a non-null assertion: ```ts -const octet = clampOctet(Number.isFinite(values.decimal) ? values.decimal! : 0); +// src/features/binary/lib/binary-utils.ts +export function clampOctet(value: number) { + return Math.min(Math.max(Math.trunc(value), 0), 255); +} + +// in the component +const raw = values.decimal; +const octet = clampOctet(typeof raw === "number" && Number.isFinite(raw) ? raw : 0); ``` +`clampOctet` lives in `binary-utils.ts` rather than the component so the pure-logic spec +can cover it directly — component tests are out of scope. It replaces the inline +`Math.min(Math.max(...))` at line 52. + Decimal and binary then cannot disagree. Typing `300` reads `255 = 11111111` — self-consistent — while Zod still surfaces the out-of-range error. Today it reads `300 = 11111111`, a smaller lie of the same kind. @@ -242,7 +284,9 @@ value survives static replacement. ## Verification -After each phase: `pnpm lint && pnpm typecheck && pnpm test && pnpm build`. +After Phase 1: `pnpm lint && pnpm typecheck && pnpm build` — `pnpm test` does not exist +until Phase 2 adds it. After Phase 2 and after each Phase 3 commit: +`pnpm lint && pnpm typecheck && pnpm test && pnpm build`. Tests prove nothing about the CSP change, which only manifests at runtime. After Phase 3, run the production build and load all 12 routes, confirming a clean console with no CSP @@ -260,19 +304,24 @@ The routing lab gets a manual check in its default state: the comparison panel m Conventional commits, matching existing history: -1. `chore: remove unused service scaffolding and dead code` -2. `test: add vitest coverage for networking and routing logic` -3. `fix(routing): rank tied-cost routes behind the selected path` -4. `fix(binary): render a consistent octet for empty and out-of-range input` -5. `refactor(arp): derive cache-update step and host data from source` -6. `fix(networking): reject IPv4 octets with leading zeros` -7. `fix(security): enforce CSP instead of report-only` +| # | Commit | Covers | +| --- | ----------------------------------------------------------------------- | ----------------------- | +| 1 | `chore: remove unused service scaffolding and dead code` | Phase 1, including §3.6 | +| 2 | `test: add vitest coverage for networking and routing logic` | Phase 2 | +| 3 | `fix(routing): rank tied-cost routes behind the selected path` | §3.1 | +| 4 | `fix(binary): render a consistent octet for empty and fractional input` | §3.2 | +| 5 | `refactor(arp): derive cache-update step and host data from source` | §3.3 | +| 6 | `fix(networking): reject IPv4 octets with leading zeros` | §3.4 | +| 7 | `fix(security): enforce CSP instead of report-only` | §3.5 | -Commits 3, 4 and 6 each carry their failing-test-then-fix pair. +Commits 3, 4 and 6 each carry their failing-test-then-fix pair. §3.6 rides in commit 1 +because it edits `env.ts`, which Phase 1 is already rewriting. ## Expected outcome -- 641 lines removed; roughly 8,500 source lines remain. +- 641 lines removed; roughly 8,500 source lines remain. Two orphan directories + (`src/utils/`, `src/styles/`) go with them. - Five spec files and a `pnpm test` step in CI where there was no test framework before. -- Two user-visible explanatory bugs fixed, one inert security header made real, and two - latent traps (ARP index coupling, env static replacement) closed. +- Three user-visible bugs fixed — the routing tie-break label, and the empty and + fractional octet displays — plus one inert security header made real, and two latent + traps (ARP index coupling, env static replacement) closed. From 8d40f38684c00ce14db56e7dc4b08a95b117ee11 Mon Sep 17 00:00:00 2001 From: ohm Date: Tue, 4 Aug 2026 09:33:19 +0700 Subject: [PATCH 03/14] docs: add repo cleanup implementation plan Seven tasks matching the spec's seven commits, each with exact file paths, complete code, and verified expected values. All test expectations were confirmed against the real implementations before being written down, and the routing fix was prototyped to confirm the post-fix output the tests assert. Corrects two spec errors found while writing the plan: the PageHeader collapse needs no page edits because all 11 pages already import from the wrapper path, and the documentation list omitted the tsconfig path aliases, docs/deployment.md, and four README locations. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 --- .../plans/2026-08-04-repo-cleanup.md | 1389 +++++++++++++++++ 1 file changed, 1389 insertions(+) create mode 100644 docs/superpowers/plans/2026-08-04-repo-cleanup.md diff --git a/docs/superpowers/plans/2026-08-04-repo-cleanup.md b/docs/superpowers/plans/2026-08-04-repo-cleanup.md new file mode 100644 index 0000000..ff4bee7 --- /dev/null +++ b/docs/superpowers/plans/2026-08-04-repo-cleanup.md @@ -0,0 +1,1389 @@ +# AckLab Repo 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:** Remove 641 lines of unreachable code, add a Vitest harness for the pure logic, and fix six correctness defects — three of them user-visible. + +**Architecture:** Three phases in strict order. Phase 1 deletes dead code so later phases work against the final file layout. Phase 2 adds Vitest and characterization tests that pin behaviour we are _not_ changing. Phase 3 fixes defects one commit at a time, each led by a failing test. Seven commits total. + +**Tech Stack:** Next.js 16 App Router, React 19, TypeScript 5.9 (strict), Zod 4, React Hook Form, Tailwind 4, pnpm 11, Vitest (new). + +## Global Constraints + +- Branch is `chore/repo-cleanup`, already created. Do not create another branch. +- Spec of record: `docs/superpowers/specs/2026-08-04-repo-cleanup-design.md`. +- Package manager is **pnpm**. Never run `npm` or `yarn`. +- A Husky pre-commit hook runs `lint-staged && pnpm typecheck` (Phase 2 adds `&& pnpm test`). Every `git commit` therefore runs those checks. If the hook fails, the commit does not happen — fix the cause, do not pass `--no-verify`. +- Prettier reformats staged files on commit. Files may look different after committing; that is expected, not a problem to fix. +- Commit messages use Conventional Commits, matching existing history (`feat:`, `fix:`, `chore:`, `refactor:`, `test:`, `docs:`). +- Every commit message ends with these two trailer lines, verbatim: + ``` + Co-Authored-By: Claude Opus 5 + Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 + ``` +- `pnpm test` does not exist until Task 2. Do not run it in Task 1. +- Never use `--no-verify`, `git push --force`, or `git rebase`. + +--- + +## File Structure + +**Deleted in Task 1** (all verified to have zero importers across `.ts`, `.tsx`, `.css`, `.md`): + +| Path | Lines | +| -------------------------------------------------------- | ----- | +| `src/services/` (whole directory, 12 files) | 184 | +| `src/data/platform.ts` | 156 | +| `src/components/visualizations/event-timeline.tsx` | 63 | +| `src/components/visualizations/packet-inspector.tsx` | 58 | +| `src/components/visualizations/network-background.tsx` | 53 | +| `src/components/visualizations/bit-grid.tsx` | 41 | +| `src/components/visualizations/protocol-state-panel.tsx` | 27 | +| `src/components/shared/metric-card.tsx` | 21 | +| `src/components/shared/learning-page-header.tsx` | 18 | +| `src/data/routing.ts` | 16 | +| `src/utils/format.ts` (empties `src/utils/`) | 3 | +| `src/styles/README.md` (empties `src/styles/`) | 1 | +| `src/features/routing/lib/routing-utils.ts` | 1 | + +**Created:** + +| Path | Responsibility | +| -------------------------------------------------- | ------------------------------------------ | +| `vitest.config.ts` | Vitest config: node environment, `@` alias | +| `src/features/routing/lib/shortest-path.test.ts` | Characterization of Dijkstra | +| `src/features/subnet/lib/subnet-utils.test.ts` | Characterization of subnet maths | +| `src/features/routing/lib/path-candidates.test.ts` | Tie-break ranking and reason strings | +| `src/features/binary/lib/binary-utils.test.ts` | `binaryToDecimal` and `clampOctet` | +| `src/lib/networking/ipv4.test.ts` | IPv4 parsing and integer round-trip | +| `src/features/arp/data/arp-steps.test.ts` | Guards the derived cache-update index | + +**Modified:** `src/components/shared/page-header.tsx`, `src/config/env.ts`, `tsconfig.json`, `.env`, `.env.example`, `README.md`, `docs/architecture.md`, `docs/deployment.md`, `package.json`, `.github/workflows/ci.yml`, `src/features/routing/lib/path-candidates.ts`, `src/features/binary/lib/binary-utils.ts`, `src/features/binary/components/binary-converter.tsx`, `src/features/arp/data/arp-steps.ts`, `src/features/arp/hooks/use-arp-simulation.ts`, `src/lib/networking/ipv4.ts`, `next.config.ts`. + +### Two corrections to the spec, discovered from the files + +1. **The `PageHeader` collapse needs no page edits.** All 11 pages already import `PageHeader` from `@/components/shared/page-header`. The plan overwrites that file's _body_ and deletes `learning-page-header.tsx`. The spec's "repoint the 11 importing pages" step is unnecessary. +2. **The spec's documentation list was incomplete.** It missed `tsconfig.json` path aliases, `docs/deployment.md:17-22`, and four further README locations. Task 1 covers all of them. The spec also says to remove a `src/utils/` line from the README architecture tree; no such line exists. + +The spec lists five spec files; this plan writes six, adding `arp-steps.test.ts` so the derived index in Task 5 has a real guard. + +--- + +## Task 1: Remove dead code, orphan directories, and config slots + +Delivers spec Phase 1 plus §3.6. No behaviour changes — every deleted symbol is unreferenced, so the app must render identically. + +**Files:** + +- Delete: the 13 paths in the File Structure table above +- Modify: `src/components/shared/page-header.tsx`, `src/config/env.ts`, `tsconfig.json`, `.env`, `.env.example`, `README.md`, `docs/architecture.md`, `docs/deployment.md` + +**Interfaces:** + +- Consumes: nothing +- Produces: `PageHeader({ eyebrow, title, description, children })` from `@/components/shared/page-header` — the import path all 11 pages already use, unchanged. `publicEnv.appUrl: string` from `@/config/env`, unchanged. + +- [ ] **Step 1: Confirm every deletion target is unreferenced** + +Run this before deleting anything. It searches all file types, excluding each symbol's own defining file: + +```bash +for n in NetworkBackground BitGrid EventTimeline PacketInspector ProtocolStatePanel \ + MetricCard formatNumber featureHighlights learningPaths visualizerCards \ + toolCards routingNodes routingEdges isPathEdge LearningPageHeader; do + hits=$(grep -rl "\b$n\b" app src public docs README.md 2>/dev/null \ + | grep -vE "visualizations/(network-background|bit-grid|event-timeline|packet-inspector|protocol-state-panel)|shared/metric-card|shared/learning-page-header|utils/format|data/platform|data/routing|routing/lib/routing-utils|docs/superpowers" \ + | tr '\n' ' ') + echo "$n -> ${hits:-CLEAR}" +done +``` + +Expected: every line ends in `CLEAR`. If any symbol reports a file, **stop** — the spec's premise is wrong for that symbol. Do not delete it; report the finding. + +- [ ] **Step 2: Delete the dead files and directories** + +```bash +git rm -r src/services +git rm src/data/platform.ts src/data/routing.ts +git rm src/components/visualizations/event-timeline.tsx \ + src/components/visualizations/packet-inspector.tsx \ + src/components/visualizations/network-background.tsx \ + src/components/visualizations/bit-grid.tsx \ + src/components/visualizations/protocol-state-panel.tsx +git rm src/components/shared/metric-card.tsx +git rm src/components/shared/learning-page-header.tsx +git rm src/utils/format.ts +git rm src/styles/README.md +git rm src/features/routing/lib/routing-utils.ts +``` + +- [ ] **Step 3: Verify the two directories are now empty and gone** + +```bash +ls src/utils src/styles 2>&1 +``` + +Expected: `No such file or directory` for both. `git rm` removes directories left empty. If either still exists, remove it with `rmdir`. + +- [ ] **Step 4: Replace the `PageHeader` body** + +The file `src/components/shared/page-header.tsx` currently forwards to the now-deleted `LearningPageHeader`. Replace its **entire contents** with the real implementation: + +```tsx +import type { ReactNode } from "react"; + +import { Badge } from "@/components/ui/badge"; + +interface PageHeaderProps { + eyebrow: string; + title: string; + description: string; + children?: ReactNode; +} + +export function PageHeader({ eyebrow, title, description, children }: PageHeaderProps) { + return ( +
+
+ + {eyebrow} + +

+ {title} +

+

{description}

+
+ {children ?
{children}
: null} +
+ ); +} +``` + +The markup is copied verbatim from the deleted `learning-page-header.tsx`, so rendering is unchanged. No page imports need editing. + +- [ ] **Step 5: Rewrite `src/config/env.ts`** + +This drops the four dead slots **and** applies spec §3.6. The key change is that `NEXT_PUBLIC_APP_URL` is now read as a literal `process.env.NEXT_PUBLIC_APP_URL` expression, which Next can statically replace, instead of parsing the whole `process.env` object dynamically. + +Replace the **entire contents** with: + +```ts +import { z } from "zod"; + +const serverEnvSchema = z.object({ + NODE_ENV: z.enum(["development", "test", "production"]).default("development"), + NEXT_PUBLIC_APP_URL: z.url().default("http://localhost:3000") +}); + +// Each value is read as a literal `process.env.X` expression so Next can inline +// NEXT_PUBLIC_ values at build time. Parsing `process.env` as an object breaks that. +const parsedEnv = serverEnvSchema.safeParse({ + NODE_ENV: process.env.NODE_ENV, + NEXT_PUBLIC_APP_URL: process.env.NEXT_PUBLIC_APP_URL +}); + +if (!parsedEnv.success) { + console.error("Invalid environment configuration", z.treeifyError(parsedEnv.error)); + throw new Error("Invalid environment configuration"); +} + +export const env = parsedEnv.data; + +export const publicEnv = { + appUrl: env.NEXT_PUBLIC_APP_URL +} as const; +``` + +- [ ] **Step 6: Remove the dead path aliases from `tsconfig.json`** + +Delete these three lines from the `"paths"` block — they point at directories that no longer exist: + +```json + "@/services/*": ["./src/services/*"], + "@/styles/*": ["./src/styles/*"], + "@/utils/*": ["./src/utils/*"] +``` + +Take care with trailing commas: `"@/types/*": ["./src/types/*"]` becomes the final entry and must **not** have a trailing comma. + +- [ ] **Step 7: Trim `.env.example` and `.env`** + +Both files must end up with exactly this content. `.env` is gitignored, so its change will not appear in the diff — change it anyway, or local config drifts from the documented example. + +``` +NEXT_PUBLIC_APP_URL=http://localhost:3000 + +# Public variables are bundled into browser JavaScript. +# Only use NEXT_PUBLIC_ for values that are safe for every visitor to see. +``` + +- [ ] **Step 8: Update `docs/architecture.md`** + +Delete line 20 (`- \`src/services/\` contains future service contracts only.`) and the entire `## Future Expansion Boundaries`section — the heading on line 22, the ten bullets on lines 24-33, and the closing paragraph on line 35. The file ends after the`## Current Runtime` list. + +- [ ] **Step 9: Update `docs/deployment.md`** + +Delete lines 17-22 — the `Private placeholders:` heading and its four bullets (`AUTH_SECRET`, `DATABASE_URL`, `STRIPE_SECRET_KEY`, `ANALYTICS_WRITE_KEY`). Keep line 24 (`Do not prefix secrets with NEXT_PUBLIC_...`); it is still good advice. + +- [ ] **Step 10: Update `README.md` in three places** + +**10a.** In the architecture code block, delete this line: + +``` +src/services/ future auth/API/payment/progress contracts only +``` + +**10b.** Delete the paragraph that follows the code block: + +``` +Future infrastructure concerns are represented by interface contracts in `src/services/`, but no network calls or server-side integrations are active. +``` + +**10c.** In `## Environment Variables`, delete the heading and four bullets: + +``` +Private placeholders for future integrations: + +- `AUTH_SECRET` +- `DATABASE_URL` +- `STRIPE_SECRET_KEY` +- `ANALYTICS_WRITE_KEY` +``` + +Leave the `## Roadmap` section untouched. It describes future intent rather than claiming code exists, so it remains accurate. The README scripts list, CI list, and security note are updated in Tasks 2 and 7 respectively. + +- [ ] **Step 11: Verify lint, types, and build** + +`pnpm test` does not exist yet — do not run it. + +```bash +pnpm lint && pnpm typecheck && pnpm build +``` + +Expected: `ESLint: No issues found`, no TypeScript errors, and a successful build ending in a route table. A `Module not found` error means a deletion target was still referenced — return to Step 1. + +- [ ] **Step 12: Commit** + +```bash +git add -A +git commit -F - <<'EOF' +chore: remove unused service scaffolding and dead code + +Deletes 641 lines with no importers: the src/services placeholder +contracts, five unused visualization components, src/data/platform.ts, +src/data/routing.ts, metric-card, format.ts, and routing-utils.ts. + +Collapses the PageHeader pass-through into a single component, drops the +now-empty src/utils and src/styles directories along with their tsconfig +path aliases, and removes the four env slots that only existed for the +deleted services. + +Also reads NEXT_PUBLIC_APP_URL as a literal process.env expression so the +value survives Next's static replacement if env.ts is ever imported from +a client component. + +Co-Authored-By: Claude Opus 5 +Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 +EOF +``` + +--- + +## Task 2: Add the Vitest harness and characterization tests + +Delivers spec Phase 2. The two spec files here are **characterization tests** — they assert what the code already does, so they pass immediately. Their job is to catch accidental damage in Tasks 3-7. + +**Files:** + +- Create: `vitest.config.ts`, `src/features/routing/lib/shortest-path.test.ts`, `src/features/subnet/lib/subnet-utils.test.ts` +- Modify: `package.json`, `.github/workflows/ci.yml`, `README.md` + +**Interfaces:** + +- Consumes: `findShortestPath(nodes, links, start, end): RoutingPathResult` from `@/features/routing/lib/shortest-path`; `calculateSubnet(ip: string, cidr: number): SubnetResult | null` from `@/features/subnet/lib/subnet-utils`; `initialRoutingNodes`, `initialRoutingLinks` from `@/features/routing/data/routing-topology` +- Produces: a working `pnpm test` command that Tasks 3-7 depend on + +- [ ] **Step 1: Install Vitest** + +```bash +pnpm add -D vitest +``` + +- [ ] **Step 2: Create `vitest.config.ts`** + +The `@` alias must be declared here — Vitest does not read `tsconfig.json` paths on its own, and adding `vite-tsconfig-paths` would be a second dependency for one line of config. + +```ts +import { fileURLToPath } from "node:url"; + +import { defineConfig } from "vitest/config"; + +export default defineConfig({ + test: { + environment: "node", + include: ["src/**/*.test.ts"] + }, + resolve: { + alias: { + "@": fileURLToPath(new URL("./src", import.meta.url)) + } + } +}); +``` + +- [ ] **Step 3: Add the `test` script and extend `precommit`** + +In `package.json`, add a `test` script after `typecheck` and append `&& pnpm test` to `precommit`: + +```json + "typecheck": "next typegen && tsc --noEmit", + "test": "vitest run", + "precommit": "lint-staged && pnpm typecheck && pnpm test", +``` + +- [ ] **Step 4: Write the shortest-path characterization test** + +Create `src/features/routing/lib/shortest-path.test.ts`. Every expected value below was verified against the current implementation. + +```ts +import { describe, expect, it } from "vitest"; + +import { initialRoutingLinks, initialRoutingNodes } from "@/features/routing/data/routing-topology"; +import { findShortestPath } from "@/features/routing/lib/shortest-path"; + +describe("findShortestPath", () => { + it("finds the lowest-cost path across the default topology", () => { + const result = findShortestPath(initialRoutingNodes, initialRoutingLinks, "client", "server"); + + expect(result.reachable).toBe(true); + expect(result.cost).toBe(7); + expect(result.path).toEqual(["client", "r1", "r3", "server"]); + }); + + it("routes around a disabled link", () => { + const links = initialRoutingLinks.map((link) => + link.id === "r3-server" ? { ...link, disabled: true } : link + ); + const result = findShortestPath(initialRoutingNodes, links, "client", "server"); + + expect(result.path).toEqual(["client", "r1", "r3", "r4", "server"]); + expect(result.cost).toBe(7); + }); + + it("reports an unreachable destination", () => { + const links = initialRoutingLinks.map((link) => + ["client-r1", "client-r2"].includes(link.id) ? { ...link, disabled: true } : link + ); + const result = findShortestPath(initialRoutingNodes, links, "client", "server"); + + expect(result.reachable).toBe(false); + expect(result.path).toEqual([]); + expect(result.cost).toBe(Number.POSITIVE_INFINITY); + }); + + it("returns a zero-cost single-node path when start equals end", () => { + const result = findShortestPath(initialRoutingNodes, initialRoutingLinks, "client", "client"); + + expect(result.reachable).toBe(true); + expect(result.cost).toBe(0); + expect(result.path).toEqual(["client"]); + }); +}); +``` + +- [ ] **Step 5: Write the subnet characterization test** + +Create `src/features/subnet/lib/subnet-utils.test.ts`. All values verified against the current implementation — note `/0` yields usable range `0.0.0.1`–`255.255.255.254`. + +```ts +import { describe, expect, it } from "vitest"; + +import { calculateSubnet } from "@/features/subnet/lib/subnet-utils"; + +describe("calculateSubnet", () => { + it("computes a /24 network", () => { + const result = calculateSubnet("192.168.10.42", 24); + + expect(result).not.toBeNull(); + expect(result?.subnetMask).toBe("255.255.255.0"); + expect(result?.networkAddress).toBe("192.168.10.0"); + expect(result?.broadcastAddress).toBe("192.168.10.255"); + expect(result?.firstUsableHost).toBe("192.168.10.1"); + expect(result?.lastUsableHost).toBe("192.168.10.254"); + expect(result?.totalHosts).toBe(256); + expect(result?.usableHosts).toBe(254); + }); + + it("treats /31 as a two-address point-to-point link", () => { + const result = calculateSubnet("10.0.0.0", 31); + + expect(result?.totalHosts).toBe(2); + expect(result?.usableHosts).toBe(2); + expect(result?.firstUsableHost).toBe("10.0.0.0"); + expect(result?.lastUsableHost).toBe("10.0.0.1"); + }); + + it("treats /32 as a single host route", () => { + const result = calculateSubnet("10.0.0.5", 32); + + expect(result?.totalHosts).toBe(1); + expect(result?.usableHosts).toBe(1); + expect(result?.networkAddress).toBe("10.0.0.5"); + expect(result?.broadcastAddress).toBe("10.0.0.5"); + }); + + it("computes /0 as the whole address space", () => { + const result = calculateSubnet("10.0.0.5", 0); + + expect(result?.subnetMask).toBe("0.0.0.0"); + expect(result?.networkAddress).toBe("0.0.0.0"); + expect(result?.broadcastAddress).toBe("255.255.255.255"); + expect(result?.totalHosts).toBe(4294967296); + expect(result?.usableHosts).toBe(4294967294); + }); + + it("rejects an out-of-range prefix", () => { + expect(calculateSubnet("10.0.0.5", 33)).toBeNull(); + }); + + it("rejects a malformed address", () => { + expect(calculateSubnet("10.0.0", 24)).toBeNull(); + }); +}); +``` + +- [ ] **Step 6: Run the tests** + +```bash +pnpm test +``` + +Expected: PASS, 2 test files, 10 tests. These are characterization tests, so they must pass on the **first** run. A failure means an expectation is wrong — fix the test, not the source. No source file changes in this task. + +- [ ] **Step 7: Add the Test step to CI** + +In `.github/workflows/ci.yml`, insert between the Typecheck and Build steps: + +```yaml +- name: Test + run: pnpm test +``` + +Also update the job name on the line reading `name: Lint, Typecheck, Build`: + +```yaml +name: Lint, Typecheck, Test, Build +``` + +- [ ] **Step 8: Update the README scripts and CI lists** + +In the `## Scripts` code block, add after the `pnpm typecheck` line: + +``` +pnpm test # run Vitest unit tests +``` + +In the `## CI/CD` bullet list, add `- test` between `- typecheck` and `- build`. + +- [ ] **Step 9: Verify everything** + +```bash +pnpm lint && pnpm typecheck && pnpm test && pnpm build +``` + +Expected: all four pass. + +- [ ] **Step 10: Commit** + +```bash +git add -A +git commit -F - <<'EOF' +test: add vitest coverage for networking and routing logic + +Adds Vitest with a node environment and no jsdom, since the functions +under test are pure. Characterization tests pin the current behaviour of +findShortestPath and calculateSubnet so the fixes that follow cannot +disturb them unnoticed. + +Wires pnpm test into CI and the pre-commit hook. + +Co-Authored-By: Claude Opus 5 +Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 +EOF +``` + +--- + +## Task 3: Fix the routing tie-break + +Delivers spec §3.1. **This is the most substantive fix in the plan.** + +The routing lab's comparison panel is headed "Why this route wins", yet in the **default state, before the user touches anything**, it lists an equally-cheap route _above_ the selected one and labels it "Costs 0 more than selected." + +**Files:** + +- Create: `src/features/routing/lib/path-candidates.test.ts` +- Modify: `src/features/routing/lib/path-candidates.ts:30-31` (sort), `:45-62` (map body), plus a new helper at end of file + +**Interfaces:** + +- Consumes: `findRouteCandidates({ links, nodes, selectedPath, sourceId, destinationId, limit? }): RouteCandidate[]`; `RouteCandidate` is `{ id: string; path: string[]; cost: number; status: "selected" | "alternative" | "unavailable"; reason: string }` +- Produces: unchanged signature; only ordering and `reason` text change + +- [ ] **Step 1: Write the failing test** + +Create `src/features/routing/lib/path-candidates.test.ts`: + +```ts +import { describe, expect, it } from "vitest"; + +import { initialRoutingLinks, initialRoutingNodes } from "@/features/routing/data/routing-topology"; +import { findRouteCandidates } from "@/features/routing/lib/path-candidates"; +import { findShortestPath } from "@/features/routing/lib/shortest-path"; + +function candidatesForDefaultTopology() { + const route = findShortestPath(initialRoutingNodes, initialRoutingLinks, "client", "server"); + + return findRouteCandidates({ + links: initialRoutingLinks, + nodes: initialRoutingNodes, + selectedPath: route.path, + sourceId: "client", + destinationId: "server" + }); +} + +describe("findRouteCandidates", () => { + it("ranks the selected route first even when another route ties on cost", () => { + const candidates = candidatesForDefaultTopology(); + + expect(candidates[0].status).toBe("selected"); + expect(candidates[0].path).toEqual(["client", "r1", "r3", "server"]); + expect(candidates[0].cost).toBe(7); + }); + + it("describes a tied route as equal cost rather than '0 more'", () => { + const candidates = candidatesForDefaultTopology(); + const tied = candidates.find( + (candidate) => candidate.cost === 7 && candidate.status === "alternative" + ); + + expect(tied).toBeDefined(); + expect(tied?.path).toEqual(["client", "r1", "r3", "r4", "server"]); + expect(tied?.reason).toBe("Same cost, different path."); + }); + + it("reports how much more a dearer alternative costs", () => { + const candidates = candidatesForDefaultTopology(); + const dearer = candidates.find((candidate) => candidate.cost === 8); + + expect(dearer?.reason).toBe("Costs 1 more than the selected route."); + }); + + it("keeps the selected route within the candidate limit", () => { + const route = findShortestPath(initialRoutingNodes, initialRoutingLinks, "client", "server"); + const candidates = findRouteCandidates({ + links: initialRoutingLinks, + nodes: initialRoutingNodes, + selectedPath: route.path, + sourceId: "client", + destinationId: "server", + limit: 1 + }); + + expect(candidates).toHaveLength(1); + expect(candidates[0].status).toBe("selected"); + }); + + it("returns a single unavailable candidate when no route exists", () => { + const links = initialRoutingLinks.map((link) => + ["client-r1", "client-r2"].includes(link.id) ? { ...link, disabled: true } : link + ); + const candidates = findRouteCandidates({ + links, + nodes: initialRoutingNodes, + selectedPath: [], + sourceId: "client", + destinationId: "server" + }); + + expect(candidates).toHaveLength(1); + expect(candidates[0].status).toBe("unavailable"); + expect(candidates[0].cost).toBe(Number.POSITIVE_INFINITY); + }); +}); +``` + +- [ ] **Step 2: Run the test to verify it fails** + +```bash +pnpm test src/features/routing/lib/path-candidates.test.ts +``` + +Expected: FAIL. Specifically the first test fails with `expected 'alternative' to be 'selected'` (the tied route currently sorts first), and the second fails with `expected 'Costs 0 more than selected.' to be 'Same cost, different path.'`. The third, fourth and fifth tests may already pass — that is fine. + +- [ ] **Step 3: Add the secondary sort key** + +In `src/features/routing/lib/path-candidates.ts`, replace: + +```ts + .sort((left, right) => left.cost - right.cost) + .slice(0, limit); +``` + +with: + +```ts + .sort( + (left, right) => + left.cost - right.cost || + Number(samePath(right.path, selectedPath)) - Number(samePath(left.path, selectedPath)) + ) + .slice(0, limit); +``` + +Cost still decides first. On a tie, the selected path sorts ahead. Because Dijkstra always returns a minimum-cost simple path, this also places the selected route at index 0, so it always survives `.slice(0, limit)`. + +- [ ] **Step 4: Hoist `selectedCost` and rewrite the reason logic** + +Replace the whole `return paths.map(...)` block at the end of `findRouteCandidates`: + +```ts +return paths.map((candidate, index) => { + const selected = samePath(candidate.path, selectedPath); + const selectedCost = paths.find((path) => samePath(path.path, selectedPath))?.cost; + + return { + id: candidate.path.join("-"), + path: candidate.path, + cost: candidate.cost, + status: selected ? "selected" : "alternative", + reason: selected + ? "Lowest total cost." + : selectedCost + ? `Costs ${candidate.cost - selectedCost} more than selected.` + : index === 0 + ? "Lowest available route." + : "More expensive than the best available route." + }; +}); +``` + +with: + +```ts +const selectedCost = paths.find((path) => samePath(path.path, selectedPath))?.cost; + +return paths.map((candidate, index) => { + const selected = samePath(candidate.path, selectedPath); + + return { + id: candidate.path.join("-"), + path: candidate.path, + cost: candidate.cost, + status: selected ? "selected" : "alternative", + reason: selected + ? "Lowest total cost." + : selectedCost === undefined + ? index === 0 + ? "Lowest available route." + : "More expensive than the best available route." + : describeDelta(candidate.cost - selectedCost) + }; +}); +``` + +Three changes: `selectedCost` moves out of the loop (it was recomputed per candidate), the guard becomes an explicit `=== undefined` check rather than a falsy one, and the arithmetic moves into a helper. + +- [ ] **Step 5: Add the `describeDelta` helper** + +Append to the end of `src/features/routing/lib/path-candidates.ts`, alongside the other module-private helpers: + +```ts +function describeDelta(delta: number) { + if (delta === 0) { + return "Same cost, different path."; + } + + return delta > 0 + ? `Costs ${delta} more than the selected route.` + : `Costs ${Math.abs(delta)} less than the selected route.`; +} +``` + +The zero branch is the live fix. The negative branch is defensive — `selectedPath` always arrives from Dijkstra today and so is always optimal, but the assumption is invisible at the call site and a future caller passing a hand-picked path would otherwise render "Costs -3 more". + +- [ ] **Step 6: Run the tests to verify they pass** + +```bash +pnpm test +``` + +Expected: PASS, 3 test files, 15 tests. + +- [ ] **Step 7: Verify the whole project** + +```bash +pnpm lint && pnpm typecheck && pnpm test && pnpm build +``` + +- [ ] **Step 8: Commit** + +```bash +git add -A +git commit -F - <<'EOF' +fix(routing): rank tied-cost routes behind the selected path + +The candidate list sorted on cost alone, so a route tying with the one +Dijkstra chose sorted above it, and the reason string had no zero case. +In the lab's default state the panel headed "Why this route wins" led +with an unselected route reading "Costs 0 more than selected." + +Adds a secondary sort key so the selected route wins its cost tier, and +replaces the bare subtraction with a helper covering the equal and +cheaper cases. Also hoists selectedCost out of the map, where it was +recomputed once per candidate. + +Co-Authored-By: Claude Opus 5 +Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 +EOF +``` + +--- + +## Task 4: Fix the binary converter octet display + +Delivers spec §3.2 — two live defects sharing one root cause: the displayed decimal and the displayed binary are derived independently and can disagree. + +- Clearing the decimal field renders `NaN = 00000000`, because `values.decimal ?? 0` does not catch `NaN`. +- Typing `1.5` renders `1.5 = 000001.1` with **no validation error** — `z.number()` accepts floats, and `(1.5).toString(2)` is `"1.1"`, which `padStart` pads rather than rejects. + +**Files:** + +- Modify: `src/features/binary/lib/binary-utils.ts` (add `clampOctet`), `src/features/binary/components/binary-converter.tsx:48-53` and `:117` +- Test: `src/features/binary/lib/binary-utils.test.ts` (create) + +**Interfaces:** + +- Consumes: `octetToBinary(octet: number): string`, `binaryToDecimal(value: string): number | null` from `@/features/binary/lib/binary-utils` +- Produces: `clampOctet(value: number): number` — clamps to 0-255 and truncates to an integer + +- [ ] **Step 1: Write the failing test** + +Create `src/features/binary/lib/binary-utils.test.ts`: + +```ts +import { describe, expect, it } from "vitest"; + +import { binaryToDecimal, clampOctet, octetToBinary } from "@/features/binary/lib/binary-utils"; + +describe("binaryToDecimal", () => { + it("converts a valid binary octet", () => { + expect(binaryToDecimal("10101100")).toBe(172); + }); + + it("accepts fewer than eight digits", () => { + expect(binaryToDecimal("101")).toBe(5); + }); + + it("rejects more than eight digits", () => { + expect(binaryToDecimal("101011001")).toBeNull(); + }); + + it("rejects non-binary input", () => { + expect(binaryToDecimal("10201100")).toBeNull(); + expect(binaryToDecimal("")).toBeNull(); + }); +}); + +describe("clampOctet", () => { + it("passes an in-range integer through", () => { + expect(clampOctet(172)).toBe(172); + }); + + it("clamps below zero and above 255", () => { + expect(clampOctet(-8)).toBe(0); + expect(clampOctet(300)).toBe(255); + }); + + it("truncates fractions so the binary string stays eight bits", () => { + expect(clampOctet(1.5)).toBe(1); + expect(octetToBinary(clampOctet(1.5))).toBe("00000001"); + }); + + it("falls back to zero for NaN", () => { + expect(clampOctet(Number.NaN)).toBe(0); + }); +}); +``` + +- [ ] **Step 2: Run the test to verify it fails** + +```bash +pnpm test src/features/binary/lib/binary-utils.test.ts +``` + +Expected: FAIL at import — `clampOctet` is not exported. The `binaryToDecimal` tests would pass but cannot run yet. + +- [ ] **Step 3: Add `clampOctet`** + +Append to `src/features/binary/lib/binary-utils.ts`: + +```ts +export function clampOctet(value: number) { + if (!Number.isFinite(value)) { + return 0; + } + + return Math.min(Math.max(Math.trunc(value), 0), 255); +} +``` + +`Math.trunc` is what stops `octetToBinary` producing a fractional string such as `"000001.1"`. The `Number.isFinite` guard makes the function total, so callers cannot reintroduce the `NaN` path. + +It lives here rather than in the component so this pure-logic spec can cover it — component tests are out of scope. + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +pnpm test src/features/binary/lib/binary-utils.test.ts +``` + +Expected: PASS, 8 tests. + +- [ ] **Step 5: Use `clampOctet` in the component** + +In `src/features/binary/components/binary-converter.tsx`, add `clampOctet` to the existing import from `@/features/binary/lib/binary-utils`: + +```ts +import { + binaryToDecimal, + clampOctet, + octetToBinary, + parseIpv4Address +} from "@/features/binary/lib/binary-utils"; +``` + +Then replace these lines: + +```ts +const decimalValue = values.decimal ?? 0; +const binaryValue = values.binary ?? ""; +const ipValue = values.ip ?? ""; +const decimalBinary = Number.isFinite(Number(decimalValue)) + ? octetToBinary(Math.min(Math.max(Number(decimalValue), 0), 255)) + : "00000000"; +``` + +with: + +```ts +// Derive one clamped octet so the decimal and the binary can never disagree. +const decimalValue = clampOctet(typeof values.decimal === "number" ? values.decimal : 0); +const binaryValue = values.binary ?? ""; +const ipValue = values.ip ?? ""; +const decimalBinary = octetToBinary(decimalValue); +``` + +`typeof values.decimal === "number"` is what narrows `number | undefined`; `Number.isFinite` is not a TypeScript type guard and would force a non-null assertion. `clampOctet` then handles the `NaN` that React Hook Form produces for an empty number input. + +Line 117 already renders `{decimalValue} = {decimalBinary}` and needs no edit — `decimalValue` is now always a clamped integer, so the two sides agree. + +- [ ] **Step 6: Verify the whole project** + +```bash +pnpm lint && pnpm typecheck && pnpm test && pnpm build +``` + +Expected: all pass, 4 test files, 23 tests. + +- [ ] **Step 7: Commit** + +```bash +git add -A +git commit -F - <<'EOF' +fix(binary): render a consistent octet for empty and fractional input + +The decimal and binary readouts were derived independently, so they could +disagree. Clearing the field showed "NaN = 00000000", because ?? 0 does +not catch NaN. Typing 1.5 showed "1.5 = 000001.1" with no validation +error, because z.number() accepts floats and padStart pads "1.1" rather +than rejecting it. + +Adds clampOctet to binary-utils, which truncates and clamps to 0-255, and +derives both readouts from it. Placing it in the lib keeps it covered by +the pure-logic spec. + +Co-Authored-By: Claude Opus 5 +Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 +EOF +``` + +--- + +## Task 5: Derive the ARP cache-update step + +Delivers spec §3.3. `stepIndex >= 5` happens to align with the `cache-update` step today. Inserting or reordering a step in `arp-steps.ts` moves the cache update to the wrong moment silently. The same block hardcodes addresses that `arpHosts` already carries. + +**Files:** + +- Modify: `src/features/arp/data/arp-steps.ts` (append export), `src/features/arp/hooks/use-arp-simulation.ts:8-32` +- Test: `src/features/arp/data/arp-steps.test.ts` (create) + +**Interfaces:** + +- Consumes: `arpSteps: ArpStep[]`, `arpHosts: ArpHost[]` from `@/features/arp/data/arp-steps`; `ArpHost` has `{ id, label, ipAddress, macAddress, role }` +- Produces: `cacheLearnedStepIndex: number` from `@/features/arp/data/arp-steps` + +- [ ] **Step 1: Write the failing test** + +Create `src/features/arp/data/arp-steps.test.ts`: + +```ts +import { describe, expect, it } from "vitest"; + +import { arpHosts, arpSteps, cacheLearnedStepIndex } from "@/features/arp/data/arp-steps"; + +describe("cacheLearnedStepIndex", () => { + it("resolves to the cache-update step", () => { + expect(cacheLearnedStepIndex).toBeGreaterThan(0); + expect(arpSteps[cacheLearnedStepIndex].id).toBe("cache-update"); + }); + + it("leaves at least one step after the cache is learned", () => { + expect(cacheLearnedStepIndex).toBeLessThan(arpSteps.length - 1); + }); +}); + +describe("arpHosts", () => { + it("carries the target addresses the cache table renders", () => { + const target = arpHosts.find((host) => host.id === "host-b"); + + expect(target?.ipAddress).toBe("192.168.1.25"); + expect(target?.macAddress).toBe("BB:BB:BB:BB:BB:25"); + }); +}); +``` + +The first assertion is the important one. If someone renames or removes the `cache-update` step, `findIndex` returns `-1`, and `stepIndex >= -1` would be true from the very first step — the cache would appear populated before the lookup happens. This test makes that failure loud. + +- [ ] **Step 2: Run the test to verify it fails** + +```bash +pnpm test src/features/arp/data/arp-steps.test.ts +``` + +Expected: FAIL at import — `cacheLearnedStepIndex` is not exported. + +- [ ] **Step 3: Export the derived index** + +Append to the end of `src/features/arp/data/arp-steps.ts`, after the `arpSteps` array: + +```ts +export const cacheLearnedStepIndex = arpSteps.findIndex((step) => step.id === "cache-update"); +``` + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +pnpm test src/features/arp/data/arp-steps.test.ts +``` + +Expected: PASS, 3 tests. + +- [ ] **Step 5: Use the derived index in the hook** + +In `src/features/arp/hooks/use-arp-simulation.ts`, update the import: + +```ts +import { + arpFrames, + arpHosts, + arpSteps, + cacheLearnedStepIndex +} from "@/features/arp/data/arp-steps"; +``` + +Replace the `initialCache` constant: + +```ts +const initialCache: ArpCacheEntry[] = [ + { + ipAddress: arpHosts.find((host) => host.id === "host-b")?.ipAddress ?? "192.168.1.25", + macAddress: "unknown", + state: "missing" + } +]; +``` + +with both cache states derived from the same source: + +```ts +const targetHost = arpHosts.find((host) => host.id === "host-b"); + +const initialCache: ArpCacheEntry[] = [ + { + ipAddress: targetHost?.ipAddress ?? "192.168.1.25", + macAddress: "unknown", + state: "missing" + } +]; + +const learnedCache: ArpCacheEntry[] = [ + { + ipAddress: targetHost?.ipAddress ?? "192.168.1.25", + macAddress: targetHost?.macAddress ?? "BB:BB:BB:BB:BB:25", + state: "learned" + } +]; +``` + +Then replace the `cache` expression inside the hook: + +```ts +const cache: ArpCacheEntry[] = + stepIndex >= 5 + ? [ + { + ipAddress: "192.168.1.25", + macAddress: "BB:BB:BB:BB:BB:25", + state: "learned" + } + ] + : initialCache; +``` + +with: + +```ts +const cache: ArpCacheEntry[] = stepIndex >= cacheLearnedStepIndex ? learnedCache : initialCache; +``` + +- [ ] **Step 6: Verify the whole project** + +```bash +pnpm lint && pnpm typecheck && pnpm test && pnpm build +``` + +Expected: all pass, 5 test files, 26 tests. + +- [ ] **Step 7: Commit** + +```bash +git add -A +git commit -F - <<'EOF' +refactor(arp): derive cache-update step and host data from source + +The learned-cache cutover was a bare `stepIndex >= 5`, coupling behaviour +to an array position, and it duplicated the target IP and MAC that +arpHosts already carries. Reordering the steps would have moved the cache +update silently. + +Exports cacheLearnedStepIndex from the step data and derives both cache +states from the host record. A test asserts the index resolves to the +cache-update step, so removing that step fails loudly rather than making +the cache appear populated from the first step. + +Co-Authored-By: Claude Opus 5 +Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 +EOF +``` + +--- + +## Task 6: Reject IPv4 octets with leading zeros + +Delivers spec §3.4. `/^\d+$/` accepts `192.168.001.1` and parses it as `192.168.1.1`. Real stacks reject leading zeros or read them as octal. + +This is a deliberate behaviour change, approved in the spec: input a learner is part-way through typing may now be rejected. Both consumers already render a validation message on parse failure, so the failure mode is a visible error, not a blank panel. + +**Files:** + +- Modify: `src/lib/networking/ipv4.ts:9` +- Test: `src/lib/networking/ipv4.test.ts` (create) + +**Interfaces:** + +- Consumes: `parseIpv4Address(value: string): number[] | null`, `ipv4ToInt(octets: number[]): number`, `intToIpv4(value: number): string`, `octetToBinary(octet: number): string` from `@/lib/networking/ipv4` +- Produces: no signature change; `parseIpv4Address` becomes stricter + +- [ ] **Step 1: Write the failing test** + +Create `src/lib/networking/ipv4.test.ts`: + +```ts +import { describe, expect, it } from "vitest"; + +import { intToIpv4, ipv4ToInt, octetToBinary, parseIpv4Address } from "@/lib/networking/ipv4"; + +describe("parseIpv4Address", () => { + it("parses a valid address", () => { + expect(parseIpv4Address("192.168.1.1")).toEqual([192, 168, 1, 1]); + }); + + it("accepts a single zero octet", () => { + expect(parseIpv4Address("10.0.0.0")).toEqual([10, 0, 0, 0]); + }); + + it("trims surrounding whitespace", () => { + expect(parseIpv4Address(" 10.0.0.1 ")).toEqual([10, 0, 0, 1]); + }); + + it("rejects octets with leading zeros", () => { + expect(parseIpv4Address("192.168.001.1")).toBeNull(); + expect(parseIpv4Address("010.0.0.1")).toBeNull(); + expect(parseIpv4Address("10.0.0.00")).toBeNull(); + }); + + it("rejects an octet above 255", () => { + expect(parseIpv4Address("256.1.1.1")).toBeNull(); + }); + + it("rejects the wrong number of octets", () => { + expect(parseIpv4Address("1.2.3")).toBeNull(); + expect(parseIpv4Address("1.2.3.4.5")).toBeNull(); + }); + + it("rejects non-numeric octets", () => { + expect(parseIpv4Address("a.b.c.d")).toBeNull(); + expect(parseIpv4Address("192.168..1")).toBeNull(); + }); +}); + +describe("ipv4ToInt and intToIpv4", () => { + it("round-trips an address whose first octet exceeds 127", () => { + expect(ipv4ToInt([203, 0, 113, 10])).toBe(3405803786); + expect(intToIpv4(3405803786)).toBe("203.0.113.10"); + }); + + it("round-trips the broadcast address", () => { + expect(ipv4ToInt([255, 255, 255, 255])).toBe(4294967295); + expect(intToIpv4(4294967295)).toBe("255.255.255.255"); + }); + + it("round-trips zero", () => { + expect(ipv4ToInt([0, 0, 0, 0])).toBe(0); + expect(intToIpv4(0)).toBe("0.0.0.0"); + }); +}); + +describe("octetToBinary", () => { + it("pads to eight bits", () => { + expect(octetToBinary(0)).toBe("00000000"); + expect(octetToBinary(5)).toBe("00000101"); + expect(octetToBinary(255)).toBe("11111111"); + }); +}); +``` + +The round-trip cases cover the sign boundary — `203 << 24` is negative before `>>> 0`, so these would catch a regression in the bit arithmetic. + +- [ ] **Step 2: Run the test to verify it fails** + +```bash +pnpm test src/lib/networking/ipv4.test.ts +``` + +Expected: FAIL on "rejects octets with leading zeros" — `expected [ 192, 168, 1, 1 ] to be null`. Every other test passes. + +- [ ] **Step 3: Tighten the octet pattern** + +In `src/lib/networking/ipv4.ts`, inside `parseIpv4Address`, replace: + +```ts + if (!/^\d+$/.test(part)) { +``` + +with: + +```ts + // Reject leading zeros: real stacks treat "010" as octal or refuse it outright. + if (!/^(0|[1-9]\d*)$/.test(part)) { +``` + +`0` stays valid; `00`, `010` and `001` do not. The existing 0-255 range check is unchanged. + +- [ ] **Step 4: Run the test to verify it passes** + +```bash +pnpm test src/lib/networking/ipv4.test.ts +``` + +Expected: PASS, 11 tests. + +- [ ] **Step 5: Verify the whole project** + +```bash +pnpm lint && pnpm typecheck && pnpm test && pnpm build +``` + +Expected: all pass, 6 test files, 37 tests. Watch the subnet characterization test in particular — it shares `parseIpv4Address` and must still pass. + +- [ ] **Step 6: Commit** + +```bash +git add -A +git commit -F - <<'EOF' +fix(networking): reject IPv4 octets with leading zeros + +parseIpv4Address accepted "192.168.001.1" and silently read it as +192.168.1.1. Real stacks reject leading zeros or interpret them as octal, +and this app teaches IPv4 addressing, so quietly normalising them +misleads. + +Both callers already surface a validation message on parse failure, so +the stricter pattern produces a visible error rather than a blank panel. + +Co-Authored-By: Claude Opus 5 +Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 +EOF +``` + +--- + +## Task 7: Enforce the Content Security Policy + +Delivers spec §3.5. The policy ships as `Content-Security-Policy-Report-Only` with no `report-uri` or `report-to` directive. Report-only does not block, and with no endpoint nothing is collected — the header does nothing at all. + +**Files:** + +- Modify: `next.config.ts:30`, `README.md` (Security Defaults) + +**Interfaces:** + +- Consumes: nothing +- Produces: an enforced CSP response header + +**This task has no unit test.** A CSP violation only manifests in a browser at runtime, so Step 3 is a manual verification and must not be skipped. + +- [ ] **Step 1: Rename the header key** + +In `next.config.ts`, replace: + +```ts + key: "Content-Security-Policy-Report-Only", +``` + +with: + +```ts + key: "Content-Security-Policy", +``` + +The directives themselves are unchanged. Because `script-src` already permits `'unsafe-inline'` and `'unsafe-eval'`, Next's inline bootstrap and Framer Motion's style injection keep working. The gain is real enforcement of `frame-ancestors`, `object-src`, `base-uri`, `form-action` and `connect-src` on a site that loads zero external resources. + +- [ ] **Step 2: Update the README security note** + +In `## Security Defaults`, replace: + +``` +- A report-only Content Security Policy placeholder is included for tightening before production enforcement. +``` + +with: + +``` +- An enforced Content Security Policy is configured in `next.config.ts`. `script-src` still allows `'unsafe-inline'` and `'unsafe-eval'` because Next's bootstrap requires them; tightening that needs per-request nonces. +``` + +- [ ] **Step 3: Verify the header and load every route** + +Build and start the production server: + +```bash +pnpm build && pnpm start +``` + +In a second terminal, confirm the header is now enforcing rather than reporting: + +```bash +curl -sI http://localhost:3000 | grep -i "content-security-policy" +``` + +Expected: a single `content-security-policy:` line. There must be **no** `-report-only` suffix. + +Then open each of these 12 routes in a browser with DevTools console open, and confirm no `Refused to ...` CSP violations appear: + +``` +/ /tools/subnet +/learn /visualizers/osi-model +/learn/start /visualizers/dns-flow +/learn/communication-basics /visualizers/arp-broadcast +/tools /visualizers/routing +/tools/binary /visualizers/tcp-handshake +``` + +`/learn/communication-basics` exercises the dynamic `/learn/[module]` segment. The +segment takes a **module id**, not a category name — `generateStaticParams` maps over +`curriculumModules`, and `ModulePage` calls `notFound()` for an unknown id, so a category +name such as `/learn/foundations` returns a 404 rather than a page. + +Pay particular attention to `/visualizers/routing` and `/visualizers/tcp-handshake` — they use Framer Motion most heavily, so if injected styles are going to trip `style-src`, it shows there first. + +If a violation appears, **stop and report it**. Do not add `'unsafe-*'` directives to silence it; that would defeat the change. + +Stop the server with Ctrl-C when done. + +- [ ] **Step 4: Confirm the routing fix in the running app** + +While the production server is still running, open `/visualizers/routing` and check the Route comparison panel in its default state: + +- First row: `Client -> R1 -> R3 -> Server`, badge `selected`, cost `7`, reason "Lowest total cost." +- Second row: `Client -> R1 -> R3 -> R4 -> Server`, badge `alternative`, cost `7`, reason "Same cost, different path." +- Third row: `Client -> R2 -> R4 -> Server`, cost `8`, reason "Costs 1 more than the selected route." + +Then open `/tools/binary`, clear the "Decimal octet" field, and confirm the readout shows `0 = 00000000` rather than `NaN = 00000000`. Type `1.5` and confirm it shows `1 = 00000001` rather than `1.5 = 000001.1`. + +- [ ] **Step 5: Final full verification** + +```bash +pnpm lint && pnpm typecheck && pnpm test && pnpm build +``` + +Expected: all pass, 6 test files, 37 tests. + +- [ ] **Step 6: Commit** + +```bash +git add -A +git commit -F - <<'EOF' +fix(security): enforce CSP instead of report-only + +The policy shipped as Content-Security-Policy-Report-Only with no +report-uri or report-to directive, so it neither blocked anything nor +collected reports. + +Enforcing the existing directives is low risk because script-src already +permits 'unsafe-inline' and 'unsafe-eval', so Next's bootstrap is +unaffected. It makes frame-ancestors, object-src, base-uri, form-action +and connect-src real on a site that loads no external resources. + +Verified against a production build across all 12 routes with no console +violations. + +Co-Authored-By: Claude Opus 5 +Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 +EOF +``` + +--- + +## Completion + +After Task 7, verify the whole branch: + +```bash +git log --oneline main..HEAD +pnpm lint && pnpm typecheck && pnpm test && pnpm build +``` + +Expected: nine commits (two spec commits plus the seven above), and all four checks green. + +**Expected end state:** + +- 641 lines removed; roughly 8,500 source lines remain. `src/utils/`, `src/styles/` and `src/services/` are gone, along with their tsconfig path aliases. +- Six spec files and 37 tests, with `pnpm test` wired into CI and the pre-commit hook, where no test framework existed before. +- Three user-visible bugs fixed — the routing tie-break label, and the empty and fractional octet displays. +- One inert security header made real, and two latent traps closed (ARP index coupling, env static replacement). + +Then use `superpowers:finishing-a-development-branch` to decide how to integrate. From e7d346401497736eb068579c8d6a0441ea9e3ef8 Mon Sep 17 00:00:00 2001 From: ohm Date: Tue, 4 Aug 2026 09:46:48 +0700 Subject: [PATCH 04/14] chore: remove unused service scaffolding and dead code Deletes lines with no importers: the src/services placeholder contracts, five unused visualization components, tool-card.tsx, types/platform.ts, src/data/routing.ts, metric-card, format.ts, and routing-utils.ts. Trims src/data/platform.ts from 156 to 8 lines, keeping only roadmapItems, which app/page.tsx still renders. Collapses the PageHeader pass-through into a single component, drops the now-empty src/utils and src/styles directories along with their tsconfig path aliases, and removes the four env slots that only existed for the deleted services. Also reads NEXT_PUBLIC_APP_URL as a literal process.env expression so the value survives Next's static replacement if env.ts is ever imported from a client component. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01GLSyPAWXYtaKPQQNpWjo24 --- .env.example | 7 - README.md | 10 -- docs/architecture.md | 16 -- docs/deployment.md | 7 - .../plans/2026-08-04-repo-cleanup.md | 56 +++++-- .../shared/learning-page-header.tsx | 32 ---- src/components/shared/metric-card.tsx | 21 --- src/components/shared/page-header.tsx | 17 +- src/components/shared/tool-card.tsx | 38 ----- src/components/visualizations/bit-grid.tsx | 41 ----- .../visualizations/event-timeline.tsx | 63 -------- .../visualizations/network-background.tsx | 53 ------- .../visualizations/packet-inspector.tsx | 58 ------- .../visualizations/protocol-state-panel.tsx | 27 ---- src/config/env.ts | 21 +-- src/data/platform.ts | 149 ------------------ src/data/routing.ts | 16 -- src/features/routing/lib/routing-utils.ts | 1 - src/services/analytics/analytics-service.ts | 10 -- src/services/api/api-client.ts | 11 -- src/services/audit/audit-service.ts | 12 -- src/services/auth/auth-service.ts | 21 --- src/services/contracts.ts | 35 ---- src/services/database/database-service.ts | 10 -- src/services/payments/payment-service.ts | 12 -- src/services/progress/progress-service.ts | 16 -- src/services/quiz/quiz-service.ts | 13 -- src/services/rbac/rbac-service.ts | 16 -- src/services/security/rate-limit-service.ts | 14 -- src/services/security/session-service.ts | 14 -- src/styles/README.md | 5 - src/types/platform.ts | 19 --- src/utils/format.ts | 3 - tsconfig.json | 5 +- 34 files changed, 64 insertions(+), 785 deletions(-) delete mode 100644 src/components/shared/learning-page-header.tsx delete mode 100644 src/components/shared/metric-card.tsx delete mode 100644 src/components/shared/tool-card.tsx delete mode 100644 src/components/visualizations/bit-grid.tsx delete mode 100644 src/components/visualizations/event-timeline.tsx delete mode 100644 src/components/visualizations/network-background.tsx delete mode 100644 src/components/visualizations/packet-inspector.tsx delete mode 100644 src/components/visualizations/protocol-state-panel.tsx delete mode 100644 src/data/routing.ts delete mode 100644 src/features/routing/lib/routing-utils.ts delete mode 100644 src/services/analytics/analytics-service.ts delete mode 100644 src/services/api/api-client.ts delete mode 100644 src/services/audit/audit-service.ts delete mode 100644 src/services/auth/auth-service.ts delete mode 100644 src/services/contracts.ts delete mode 100644 src/services/database/database-service.ts delete mode 100644 src/services/payments/payment-service.ts delete mode 100644 src/services/progress/progress-service.ts delete mode 100644 src/services/quiz/quiz-service.ts delete mode 100644 src/services/rbac/rbac-service.ts delete mode 100644 src/services/security/rate-limit-service.ts delete mode 100644 src/services/security/session-service.ts delete mode 100644 src/styles/README.md delete mode 100644 src/types/platform.ts delete mode 100644 src/utils/format.ts diff --git a/.env.example b/.env.example index 92d9f4f..ceb53d6 100644 --- a/.env.example +++ b/.env.example @@ -2,10 +2,3 @@ NEXT_PUBLIC_APP_URL=http://localhost:3000 # Public variables are bundled into browser JavaScript. # Only use NEXT_PUBLIC_ for values that are safe for every visitor to see. - -# Private server-only variables for future integrations. -# These are placeholders only; AckLab does not use them in the local mock MVP. -# AUTH_SECRET= -# DATABASE_URL= -# STRIPE_SECRET_KEY= -# ANALYTICS_WRITE_KEY= diff --git a/README.md b/README.md index 531329a..991880d 100644 --- a/README.md +++ b/README.md @@ -28,7 +28,6 @@ src/components/layout/ shell, nav, command menu, theme provider src/components/shared/ reusable product UI src/components/visualizations/ src/features/ domain-owned interactive tools and visualizers -src/services/ future auth/API/payment/progress contracts only src/data/ local mock data src/types/ shared TypeScript contracts src/config/ app and environment configuration @@ -36,8 +35,6 @@ src/constants/ navigation and constants docs/ architecture and deployment documentation ``` -Future infrastructure concerns are represented by interface contracts in `src/services/`, but no network calls or server-side integrations are active. - ## Local Setup Use pnpm through Corepack: @@ -82,13 +79,6 @@ Public variables: - `NEXT_PUBLIC_APP_URL`: browser-visible app URL. -Private placeholders for future integrations: - -- `AUTH_SECRET` -- `DATABASE_URL` -- `STRIPE_SECRET_KEY` -- `ANALYTICS_WRITE_KEY` - Do not expose secrets through `NEXT_PUBLIC_`. Any value with that prefix can be bundled into client-side JavaScript. ## Docker diff --git a/docs/architecture.md b/docs/architecture.md index cb0e90e..af1da71 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -17,19 +17,3 @@ AckLab is organized as a feature-oriented Next.js App Router project. The curren - `src/components/visualizations/` contains shared visual building blocks. - `src/data/` contains local mock data. - `src/lib/` contains shared utilities and networking primitives. -- `src/services/` contains future service contracts only. - -## Future Expansion Boundaries - -- Auth: `src/services/auth` -- RBAC: `src/services/rbac` -- API client: `src/services/api` -- Database: `src/services/database` -- Payments and subscriptions: `src/services/payments` -- Progress tracking: `src/services/progress` -- Quiz engine: `src/services/quiz` -- Analytics: `src/services/analytics` -- Rate limiting and secure sessions: `src/services/security` -- Audit logging: `src/services/audit` - -These placeholders are intentionally minimal. Add real adapters behind these contracts only when the product introduces server-side infrastructure. diff --git a/docs/deployment.md b/docs/deployment.md index 97ddf41..b18f1fc 100644 --- a/docs/deployment.md +++ b/docs/deployment.md @@ -14,13 +14,6 @@ Public variables: - `NEXT_PUBLIC_APP_URL`: safe browser-visible site URL. -Private placeholders: - -- `AUTH_SECRET` -- `DATABASE_URL` -- `STRIPE_SECRET_KEY` -- `ANALYTICS_WRITE_KEY` - Do not prefix secrets with `NEXT_PUBLIC_`. Values with that prefix are bundled into browser JavaScript. ## Local Development diff --git a/docs/superpowers/plans/2026-08-04-repo-cleanup.md b/docs/superpowers/plans/2026-08-04-repo-cleanup.md index ff4bee7..c62bd58 100644 --- a/docs/superpowers/plans/2026-08-04-repo-cleanup.md +++ b/docs/superpowers/plans/2026-08-04-repo-cleanup.md @@ -33,19 +33,26 @@ | Path | Lines | | -------------------------------------------------------- | ----- | | `src/services/` (whole directory, 12 files) | 184 | -| `src/data/platform.ts` | 156 | | `src/components/visualizations/event-timeline.tsx` | 63 | | `src/components/visualizations/packet-inspector.tsx` | 58 | | `src/components/visualizations/network-background.tsx` | 53 | | `src/components/visualizations/bit-grid.tsx` | 41 | +| `src/components/shared/tool-card.tsx` | 38 | | `src/components/visualizations/protocol-state-panel.tsx` | 27 | | `src/components/shared/metric-card.tsx` | 21 | +| `src/types/platform.ts` | 19 | | `src/components/shared/learning-page-header.tsx` | 18 | | `src/data/routing.ts` | 16 | | `src/utils/format.ts` (empties `src/utils/`) | 3 | | `src/styles/README.md` (empties `src/styles/`) | 1 | | `src/features/routing/lib/routing-utils.ts` | 1 | +**Trimmed in Task 1** (partially dead — one export is live): + +| Path | Before | After | Kept export | +| ---------------------- | ------ | ----- | --------------------------------------- | +| `src/data/platform.ts` | 156 | 8 | `roadmapItems` (used by `app/page.tsx`) | + **Created:** | Path | Responsibility | @@ -58,12 +65,13 @@ | `src/lib/networking/ipv4.test.ts` | IPv4 parsing and integer round-trip | | `src/features/arp/data/arp-steps.test.ts` | Guards the derived cache-update index | -**Modified:** `src/components/shared/page-header.tsx`, `src/config/env.ts`, `tsconfig.json`, `.env`, `.env.example`, `README.md`, `docs/architecture.md`, `docs/deployment.md`, `package.json`, `.github/workflows/ci.yml`, `src/features/routing/lib/path-candidates.ts`, `src/features/binary/lib/binary-utils.ts`, `src/features/binary/components/binary-converter.tsx`, `src/features/arp/data/arp-steps.ts`, `src/features/arp/hooks/use-arp-simulation.ts`, `src/lib/networking/ipv4.ts`, `next.config.ts`. +**Modified:** `src/data/platform.ts` (trimmed), `src/components/shared/page-header.tsx`, `src/config/env.ts`, `tsconfig.json`, `.env`, `.env.example`, `README.md`, `docs/architecture.md`, `docs/deployment.md`, `package.json`, `.github/workflows/ci.yml`, `src/features/routing/lib/path-candidates.ts`, `src/features/binary/lib/binary-utils.ts`, `src/features/binary/components/binary-converter.tsx`, `src/features/arp/data/arp-steps.ts`, `src/features/arp/hooks/use-arp-simulation.ts`, `src/lib/networking/ipv4.ts`, `next.config.ts`. -### Two corrections to the spec, discovered from the files +### Corrections to the spec, discovered from the files 1. **The `PageHeader` collapse needs no page edits.** All 11 pages already import `PageHeader` from `@/components/shared/page-header`. The plan overwrites that file's _body_ and deletes `learning-page-header.tsx`. The spec's "repoint the 11 importing pages" step is unnecessary. 2. **The spec's documentation list was incomplete.** It missed `tsconfig.json` path aliases, `docs/deployment.md:17-22`, and four further README locations. Task 1 covers all of them. The spec also says to remove a `src/utils/` line from the README architecture tree; no such line exists. +3. **`src/data/platform.ts` is not fully dead.** Its `roadmapItems` export is imported by `app/page.tsx:20` and rendered at `:143` — the original sweep checked `featureHighlights`, `learningPaths`, `visualizerCards`, and `toolCards` but never checked `roadmapItems`. Discovered when Task 1's implementer ran the verification loop and the build failed on a missing module. The file is trimmed to just `roadmapItems`, not deleted. That cascades two levels: `toolCards`/`visualizerCards` were the only callers of the `ToolCard` component (`src/components/shared/tool-card.tsx`, 38 lines, verified zero importers), and `ToolCard`/`VisualizerCard`/`CardStatus` in `src/types/platform.ts` (19 lines) were only consumed by `tool-card.tsx` and `platform.ts` — both now gone, so the whole types file is dead too, verified zero importers elsewhere. The spec lists five spec files; this plan writes six, adding `arp-steps.test.ts` so the derived index in Task 5 has a real guard. @@ -71,11 +79,12 @@ The spec lists five spec files; this plan writes six, adding `arp-steps.test.ts` ## Task 1: Remove dead code, orphan directories, and config slots -Delivers spec Phase 1 plus §3.6. No behaviour changes — every deleted symbol is unreferenced, so the app must render identically. +Delivers spec Phase 1 plus §3.6. No behaviour changes — every deleted symbol is unreferenced, so the app must render identically. `src/data/platform.ts` is trimmed rather than deleted (see below) because one of its exports is live. **Files:** -- Delete: the 13 paths in the File Structure table above +- Delete: the 14 paths in the "Deleted in Task 1" table above +- Trim: `src/data/platform.ts` down to its `roadmapItems` export only - Modify: `src/components/shared/page-header.tsx`, `src/config/env.ts`, `tsconfig.json`, `.env`, `.env.example`, `README.md`, `docs/architecture.md`, `docs/deployment.md` **Interfaces:** @@ -90,21 +99,24 @@ Run this before deleting anything. It searches all file types, excluding each sy ```bash for n in NetworkBackground BitGrid EventTimeline PacketInspector ProtocolStatePanel \ MetricCard formatNumber featureHighlights learningPaths visualizerCards \ - toolCards routingNodes routingEdges isPathEdge LearningPageHeader; do + toolCards ToolCard VisualizerCard CardStatus routingNodes routingEdges \ + isPathEdge LearningPageHeader; do hits=$(grep -rl "\b$n\b" app src public docs README.md 2>/dev/null \ - | grep -vE "visualizations/(network-background|bit-grid|event-timeline|packet-inspector|protocol-state-panel)|shared/metric-card|shared/learning-page-header|utils/format|data/platform|data/routing|routing/lib/routing-utils|docs/superpowers" \ + | grep -vE "visualizations/(network-background|bit-grid|event-timeline|packet-inspector|protocol-state-panel)|shared/metric-card|shared/learning-page-header|shared/tool-card|utils/format|data/platform|data/routing|routing/lib/routing-utils|types/platform|docs/superpowers" \ | tr '\n' ' ') echo "$n -> ${hits:-CLEAR}" done ``` -Expected: every line ends in `CLEAR`. If any symbol reports a file, **stop** — the spec's premise is wrong for that symbol. Do not delete it; report the finding. +Expected: every line ends in `CLEAR`. If any symbol reports a file, **stop** — the spec's premise is wrong for that symbol. Do not delete it; report the finding. (`roadmapItems` is deliberately not in this list — it is expected to report `app/page.tsx`, which is why `platform.ts` is trimmed rather than deleted below.) - [ ] **Step 2: Delete the dead files and directories** ```bash git rm -r src/services -git rm src/data/platform.ts src/data/routing.ts +git rm src/data/routing.ts +git rm src/components/shared/tool-card.tsx +git rm src/types/platform.ts git rm src/components/visualizations/event-timeline.tsx \ src/components/visualizations/packet-inspector.tsx \ src/components/visualizations/network-background.tsx \ @@ -125,6 +137,22 @@ ls src/utils src/styles 2>&1 Expected: `No such file or directory` for both. `git rm` removes directories left empty. If either still exists, remove it with `rmdir`. +- [ ] **Step 3a: Trim `src/data/platform.ts` to its live export** + +`roadmapItems` is imported by `app/page.tsx`; the other four exports (`featureHighlights`, `learningPaths`, `visualizerCards`, `toolCards`) have zero importers, confirmed in Step 1. Replace the file's **entire contents** with: + +```ts +export const roadmapItems = [ + "Quiz engine and adaptive concept checks", + "User progress and course path persistence", + "Admin dashboard for content operations", + "Secure auth, RBAC, subscriptions, and payment workflows", + "Analytics, audit logging, and rate limited API integrations" +] as const; +``` + +Do not `git rm` this file — only its content changes, from 156 lines to 8. Its `ToolCard`/`VisualizerCard` import (`import type { ToolCard, VisualizerCard } from "@/types/platform"`) is dropped along with the exports that used it. + - [ ] **Step 4: Replace the `PageHeader` body** The file `src/components/shared/page-header.tsx` currently forwards to the now-deleted `LearningPageHeader`. Replace its **entire contents** with the real implementation: @@ -269,9 +297,11 @@ git add -A git commit -F - <<'EOF' chore: remove unused service scaffolding and dead code -Deletes 641 lines with no importers: the src/services placeholder -contracts, five unused visualization components, src/data/platform.ts, -src/data/routing.ts, metric-card, format.ts, and routing-utils.ts. +Deletes lines with no importers: the src/services placeholder +contracts, five unused visualization components, tool-card.tsx, +types/platform.ts, src/data/routing.ts, metric-card, format.ts, and +routing-utils.ts. Trims src/data/platform.ts from 156 to 8 lines, +keeping only roadmapItems, which app/page.tsx still renders. Collapses the PageHeader pass-through into a single component, drops the now-empty src/utils and src/styles directories along with their tsconfig @@ -1381,7 +1411,7 @@ Expected: nine commits (two spec commits plus the seven above), and all four che **Expected end state:** -- 641 lines removed; roughly 8,500 source lines remain. `src/utils/`, `src/styles/` and `src/services/` are gone, along with their tsconfig path aliases. +- Roughly 690 lines removed (543 from full deletions, 148 trimmed from `src/data/platform.ts`); roughly 8,450 source lines remain. `src/utils/`, `src/styles/` and `src/services/` are gone, along with their tsconfig path aliases. - Six spec files and 37 tests, with `pnpm test` wired into CI and the pre-commit hook, where no test framework existed before. - Three user-visible bugs fixed — the routing tie-break label, and the empty and fractional octet displays. - One inert security header made real, and two latent traps closed (ARP index coupling, env static replacement). diff --git a/src/components/shared/learning-page-header.tsx b/src/components/shared/learning-page-header.tsx deleted file mode 100644 index 4aee096..0000000 --- a/src/components/shared/learning-page-header.tsx +++ /dev/null @@ -1,32 +0,0 @@ -import type { ReactNode } from "react"; - -import { Badge } from "@/components/ui/badge"; - -interface LearningPageHeaderProps { - eyebrow: string; - title: string; - description: string; - children?: ReactNode; -} - -export function LearningPageHeader({ - eyebrow, - title, - description, - children -}: LearningPageHeaderProps) { - return ( -
-
- - {eyebrow} - -

- {title} -

-

{description}

-
- {children ?
{children}
: null} -
- ); -} diff --git a/src/components/shared/metric-card.tsx b/src/components/shared/metric-card.tsx deleted file mode 100644 index 048cef1..0000000 --- a/src/components/shared/metric-card.tsx +++ /dev/null @@ -1,21 +0,0 @@ -import { Card, CardContent } from "@/components/ui/card"; - -interface MetricCardProps { - label: string; - value: string | number; - hint?: string; -} - -export function MetricCard({ label, value, hint }: MetricCardProps) { - return ( - - -

- {label} -

-

{value}

- {hint ?

{hint}

: null} - - - ); -} diff --git a/src/components/shared/page-header.tsx b/src/components/shared/page-header.tsx index 6f4758d..d27593f 100644 --- a/src/components/shared/page-header.tsx +++ b/src/components/shared/page-header.tsx @@ -1,6 +1,6 @@ import type { ReactNode } from "react"; -import { LearningPageHeader } from "@/components/shared/learning-page-header"; +import { Badge } from "@/components/ui/badge"; interface PageHeaderProps { eyebrow: string; @@ -11,8 +11,17 @@ interface PageHeaderProps { export function PageHeader({ eyebrow, title, description, children }: PageHeaderProps) { return ( - - {children} - +
+
+ + {eyebrow} + +

+ {title} +

+

{description}

+
+ {children ?
{children}
: null} +
); } diff --git a/src/components/shared/tool-card.tsx b/src/components/shared/tool-card.tsx deleted file mode 100644 index 5b1c5bd..0000000 --- a/src/components/shared/tool-card.tsx +++ /dev/null @@ -1,38 +0,0 @@ -import { ArrowRight, LockKeyhole } from "lucide-react"; -import Link from "next/link"; - -import { Badge } from "@/components/ui/badge"; -import { Card, CardContent, CardHeader, CardTitle } from "@/components/ui/card"; -import type { ToolCard as ToolCardType } from "@/types/platform"; - -export function ToolCard({ tool }: { tool: ToolCardType }) { - const live = tool.status === "Live"; - - return ( - - - -
- {tool.status} - {live ? ( - - ) : ( - - )} -
- {tool.title} - - -

{tool.description}

-

- {tool.category} -

- - - - ); -} diff --git a/src/components/visualizations/bit-grid.tsx b/src/components/visualizations/bit-grid.tsx deleted file mode 100644 index f3d223d..0000000 --- a/src/components/visualizations/bit-grid.tsx +++ /dev/null @@ -1,41 +0,0 @@ -"use client"; - -import { motion } from "framer-motion"; - -import { cn } from "@/lib/utils"; - -interface BitGridProps { - octets: string[]; - cidr?: number; -} - -export function BitGrid({ octets, cidr = 32 }: BitGridProps) { - const bits = octets.join("").split(""); - - return ( -
- {bits.map((bit, index) => { - const networkBit = index < cidr; - return ( - - {bit} - - ); - })} -
- ); -} diff --git a/src/components/visualizations/event-timeline.tsx b/src/components/visualizations/event-timeline.tsx deleted file mode 100644 index 994e70d..0000000 --- a/src/components/visualizations/event-timeline.tsx +++ /dev/null @@ -1,63 +0,0 @@ -import type { NetworkEvent } from "@/lib/simulation/types"; -import { cn } from "@/lib/utils"; - -interface EventTimelineProps { - events: NetworkEvent[]; - activePacketId?: string; - onSelectPacket?: (packetId: string) => void; -} - -export function EventTimeline({ events, activePacketId, onSelectPacket }: EventTimelineProps) { - return ( -
-
-

Event timeline

- {events.length} events -
- - {events.length === 0 ? ( -

- Network events will appear here as you connect, send data, and close the simulated socket. -

- ) : ( -
    - {events.map((event) => { - const active = event.packetId === activePacketId; - const content = ( - <> - - {event.timestampLabel} - - - {event.title} - - - {event.description} - - - ); - - return ( -
  1. - {event.packetId && onSelectPacket ? ( - - ) : ( -
    {content}
    - )} -
    2. - ); - })} -
- )} -
- ); -} diff --git a/src/components/visualizations/network-background.tsx b/src/components/visualizations/network-background.tsx deleted file mode 100644 index f1f3d03..0000000 --- a/src/components/visualizations/network-background.tsx +++ /dev/null @@ -1,53 +0,0 @@ -"use client"; - -import { motion } from "framer-motion"; - -const points = [ - { x: "12%", y: "28%" }, - { x: "28%", y: "62%" }, - { x: "48%", y: "35%" }, - { x: "66%", y: "68%" }, - { x: "84%", y: "30%" } -]; - -export function NetworkBackground() { - return ( -