diff --git a/.superpowers/sdd/2026-08-26-housekeeping-completion/task-9-report.md b/.superpowers/sdd/2026-08-26-housekeeping-completion/task-9-report.md index 20af223c92..941916d4ac 100644 --- a/.superpowers/sdd/2026-08-26-housekeeping-completion/task-9-report.md +++ b/.superpowers/sdd/2026-08-26-housekeeping-completion/task-9-report.md @@ -138,3 +138,97 @@ src/features/housekeeping/route-handlers.ts - Mutation check: dynamic-name normalization removal, regex-style static matching, static-priority removal, decoded-separator acceptance, domain mismatch acceptance, skipped route ACL, context reload inside the handler, missing handler lookup, placeholder handler addition, and empty-domain redirect each break a focused test. - The catch-all route imports only housekeeping foundation/manifests/handlers and retains the existing forbidden database/auth/permissions/actions/legacy-page boundary audit. - `apply_patch` created all new files, but the Windows sandbox helper repeatedly failed to read existing files with `apply deny-read ACLs`. Existing-file edits therefore used controller-approved exact-anchor/full-file fallbacks only after resolving absolute paths and validating every target under `E:\Users\simol\Desktop\EpicNext-cms`. + +## Fix Round 1 — deterministic encoded route matching + +### Status and scope + +- Fix base: `e5c230ba35aec2b4c471608d32b8a16ecc1ee382`. +- Only the two Important matcher blockers were addressed. +- The three parked Minor findings remain unchanged; no changed line required an adjustment to them. +- Commit message: `fix(housekeeping): make route matching deterministic`. +- `.remember/` remained untouched. No worktree, push, PR/MR, database operation, deployment, or Task 10 work was performed. + +### Root-cause evidence + +1. The catch-all receives decoded Next segments and re-encodes them with `encodeURIComponent`. The matcher decoded the request path into `decodedSegments` but compared static route text against `rawSegments`. Therefore literal `a+b[1]` did not equal `a%2Bb%5B1%5D`; the competing `:id` route captured the request and could change the selected capability/handler. +2. Candidate sorting used only total dynamic-segment count. Intersecting patterns `/:kind/settings` and `/users/:id` have the same count, so stable sort preserved manifest order and allowed registration order to decide dispatch. + +### RED + +Exact command after test setup was validated: + +```text +pnpm exec vitest run --coverage.enabled=false src/features/housekeeping/foundation/routing/match-route.test.ts src/features/housekeeping/foundation/registry.test.ts src/features/housekeeping/route-handlers.test.ts src/features/housekeeping/foundation/preview-route-contract.test.ts +``` + +Observed exit 1: + +```text +Test Files 2 failed | 2 passed (4) +Tests 2 failed | 95 passed (97) + +catch-all encoded literal: +Expected routeId people.literal-tool with params {} +Received routeId people.tool-detail with params { id: "a+b[1]" } + +equal-count specificity: +Expected routeId people.user-detail with params { id: "settings" } +Received routeId people.kind-settings with params { kind: "users" } +``` + +The registration-order table exercises both orders. Before production changes the general-first order failed while the reverse order passed, proving that order was the deciding variable. + +An earlier RED attempt exposed a test-table setup error (`manifest.routes is not iterable`); the table was changed from spread array rows to named `{ routes }` rows, then rerun to obtain the behavioral RED above before any production edit. + +### Fix + +- A single `decodeCanonicalSegment` boundary now normalizes request segments and static route-pattern segments exactly once. +- Invalid percent encoding, empty values, decoded `/` or `\`, and decoded `.` / `..` remain fail closed. +- Dynamic markers retain their parameter names and receive the already-decoded request segment. +- Candidate specificity is compared left-to-right. At the earliest static/dynamic difference, the static segment wins; manifest order no longer selects among intersecting patterns. +- Existing static-over-dynamic behavior and registry duplicate-shape rejection remain unchanged. + +### GREEN and pre-commit verification + +Targeted command above, exit 0: + +```text +Test Files 4 passed (4) +Tests 97 passed (97) +``` + +Full housekeeping suite, exit 0: + +```text +pnpm test:housekeeping +Test Files 38 passed (38) +Tests 352 passed (352) +``` + +TypeScript, exit 0: + +```text +pnpm typecheck +$ tsc --noEmit +``` + +The only output note remained the existing Node warning: local `26.7.0`, package request `>=26.8.1 <27`. + +Exact changed-file Biome, exit 0: + +```text +pnpm exec biome check --formatter-enabled=false src/features/housekeeping/foundation/routing/match-route.ts src/features/housekeeping/foundation/routing/match-route.test.ts src/features/housekeeping/foundation/preview-route-contract.test.ts +Checked 3 files in 25ms. No fixes applied. +``` + +`git diff --check` exited 0 before the report update. Cached and committed-tree checks are final staging/commit gates. + +### Exact fix files + +```text +src/features/housekeeping/foundation/routing/match-route.ts +src/features/housekeeping/foundation/routing/match-route.test.ts +src/features/housekeeping/foundation/preview-route-contract.test.ts +.superpowers/sdd/2026-08-26-housekeeping-completion/task-9-report.md +``` \ No newline at end of file diff --git a/src/features/housekeeping/foundation/preview-route-contract.test.ts b/src/features/housekeeping/foundation/preview-route-contract.test.ts index 63c678eead..5470bbdd88 100644 --- a/src/features/housekeeping/foundation/preview-route-contract.test.ts +++ b/src/features/housekeeping/foundation/preview-route-contract.test.ts @@ -121,6 +121,24 @@ vi.mock("@/features/housekeeping/manifests", async (importOriginal) => { slugs: ["admin.bans.view"], }, }, + { + id: "people.tool-detail", + labelKey: "pages.housekeeping.domains.people.title", + href: "/ase/people/tools/:id" as const, + capability: { + mode: "any" as const, + slugs: ["admin.users.view"], + }, + }, + { + id: "people.literal-tool", + labelKey: "pages.housekeeping.domains.people.title", + href: "/ase/people/tools/a+b[1]" as const, + capability: { + mode: "any" as const, + slugs: ["admin.users.view"], + }, + }, ], } : manifest, @@ -135,6 +153,14 @@ vi.mock("@/features/housekeeping/route-handlers", () => ({ render: routeMocks.renderHousekeepingRoute, }, { routeId: "people.bans", render: routeMocks.renderHousekeepingRoute }, + { + routeId: "people.tool-detail", + render: routeMocks.renderHousekeepingRoute, + }, + { + routeId: "people.literal-tool", + render: routeMocks.renderHousekeepingRoute, + }, ], })); @@ -631,6 +657,30 @@ describe("/ase-next/[domain]/[[...segments]] page", () => { }); }); + it("keeps catch-all encoding from changing the literal route handler", async () => { + const context = capabilityContext([PERMS.USERS_VIEW]); + routeMocks.getHousekeepingCapabilityContext.mockResolvedValue(context); + + await renderRoute( + AdminNextDomainPage({ + params: Promise.resolve({ + domain: "people", + segments: ["tools", "a+b[1]"], + }), + }), + ); + + expect(routeMocks.renderHousekeepingRoute).toHaveBeenCalledWith({ + context, + match: { + routeId: "people.literal-tool", + domain: "people", + params: {}, + canonicalHref: "/ase/people/tools/a%2Bb%5B1%5D", + }, + }); + }); + it("returns 404 for an inaccessible matched route without invoking it", async () => { routeMocks.getHousekeepingCapabilityContext.mockResolvedValue( capabilityContext([PERMS.MOD_CFH_VIEW]), diff --git a/src/features/housekeeping/foundation/routing/match-route.test.ts b/src/features/housekeeping/foundation/routing/match-route.test.ts index 2c12b009a3..31baf1790b 100644 --- a/src/features/housekeeping/foundation/routing/match-route.test.ts +++ b/src/features/housekeeping/foundation/routing/match-route.test.ts @@ -102,6 +102,35 @@ describe("matchHousekeepingRoute", () => { ).toBeNull(); }); + it.each([ + { + routes: [ + route("people.kind-settings", "/ase/people/:kind/settings"), + route("people.user-detail", "/ase/people/users/:id"), + ], + }, + { + routes: [ + route("people.user-detail", "/ase/people/users/:id"), + route("people.kind-settings", "/ase/people/:kind/settings"), + ], + }, + ])( + "prefers the earliest static segment regardless of registration order", + ({ routes }) => { + const orderedRegistry = createHousekeepingRegistry([ + peopleManifest(routes), + ]); + + expect( + matchHousekeepingRoute(orderedRegistry, "/ase/people/users/settings"), + )?.toMatchObject({ + routeId: "people.user-detail", + params: { id: "settings" }, + }); + }, + ); + it.each([ "/ase/people/unknown", "/ase/economy/users", diff --git a/src/features/housekeeping/foundation/routing/match-route.ts b/src/features/housekeeping/foundation/routing/match-route.ts index ce795ce19a..37df2dee74 100644 --- a/src/features/housekeeping/foundation/routing/match-route.ts +++ b/src/features/housekeeping/foundation/routing/match-route.ts @@ -10,15 +10,18 @@ export interface HousekeepingRouteMatch { } interface ParsedCanonicalPath { - rawSegments: readonly string[]; - decodedSegments: readonly string[]; + segments: readonly string[]; +} + +interface RoutePatternSegment { + literal: string | null; + parameterName: string | null; } interface RouteCandidate { routeId: string; domain: HousekeepingDomainId; - patternSegments: readonly string[]; - dynamicSegments: number; + patternSegments: readonly RoutePatternSegment[]; } export function matchHousekeepingRoute( @@ -30,24 +33,16 @@ export function matchHousekeepingRoute( const candidates = registry.domains .flatMap((domain) => - domain.routes - .filter((route) => - routeBelongsToDomain(route.href, domain.canonicalHref), - ) - .map((route) => { - const patternSegments = route.href.slice(1).split("/"); + domain.routes.flatMap((route) => { + if (!routeBelongsToDomain(route.href, domain.canonicalHref)) return []; - return { - routeId: route.id, - domain: domain.id, - patternSegments, - dynamicSegments: patternSegments.filter((segment) => - segment.startsWith(":"), - ).length, - }; - }), + const patternSegments = parseRoutePattern(route.href); + return patternSegments + ? [{ routeId: route.id, domain: domain.id, patternSegments }] + : []; + }), ) - .sort((left, right) => left.dynamicSegments - right.dynamicSegments); + .sort(compareSpecificity); for (const candidate of candidates) { const params = matchSegments(candidate, path); @@ -81,28 +76,71 @@ function parseCanonicalPath(canonicalPath: string): ParsedCanonicalPath | null { return null; } - const decodedSegments: string[] = []; + const segments: string[] = []; for (const segment of rawSegments) { - let decoded: string; - try { - decoded = decodeURIComponent(segment); - } catch { - return null; - } - - if ( - !decoded || - decoded === "." || - decoded === ".." || - decoded.includes("/") || - decoded.includes("\\") - ) { - return null; - } - decodedSegments.push(decoded); + const decoded = decodeCanonicalSegment(segment); + if (decoded === null) return null; + segments.push(decoded); } - return { rawSegments, decodedSegments }; + return { segments }; +} + +function parseRoutePattern( + routeHref: CanonicalHousekeepingHref, +): readonly RoutePatternSegment[] | null { + const segments: RoutePatternSegment[] = []; + + for (const segment of routeHref.slice(1).split("/")) { + if (segment.startsWith(":")) { + const parameterName = segment.slice(1); + if (!parameterName) return null; + segments.push({ literal: null, parameterName }); + continue; + } + + const literal = decodeCanonicalSegment(segment); + if (literal === null) return null; + segments.push({ literal, parameterName: null }); + } + + return segments; +} + +function decodeCanonicalSegment(segment: string): string | null { + let decoded: string; + try { + decoded = decodeURIComponent(segment); + } catch { + return null; + } + + return !decoded || + decoded === "." || + decoded === ".." || + decoded.includes("/") || + decoded.includes("\\") + ? null + : decoded; +} + +function compareSpecificity( + left: RouteCandidate, + right: RouteCandidate, +): number { + const sharedLength = Math.min( + left.patternSegments.length, + right.patternSegments.length, + ); + + for (let index = 0; index < sharedLength; index += 1) { + const leftDynamic = left.patternSegments[index]?.parameterName !== null; + const rightDynamic = right.patternSegments[index]?.parameterName !== null; + if (leftDynamic === rightDynamic) continue; + return leftDynamic ? 1 : -1; + } + + return left.patternSegments.length - right.patternSegments.length; } function routeBelongsToDomain( @@ -120,18 +158,18 @@ function matchSegments( candidate: RouteCandidate, path: ParsedCanonicalPath, ): Record | null { - if (candidate.patternSegments.length !== path.rawSegments.length) return null; + if (candidate.patternSegments.length !== path.segments.length) return null; const params: Record = {}; for (const [index, patternSegment] of candidate.patternSegments.entries()) { - if (!patternSegment.startsWith(":")) { - if (patternSegment !== path.rawSegments[index]) return null; + const pathSegment = path.segments[index] ?? ""; + if (patternSegment.parameterName === null) { + if (patternSegment.literal !== pathSegment) return null; continue; } - const parameterName = patternSegment.slice(1); - if (!parameterName || parameterName in params) return null; - params[parameterName] = path.decodedSegments[index] ?? ""; + if (patternSegment.parameterName in params) return null; + params[patternSegment.parameterName] = pathSegment; } return params;