diff --git a/docs/operations/template-merge-2026-08-17.md b/docs/operations/template-merge-2026-08-17.md index da7d35f..4a9adbe 100644 --- a/docs/operations/template-merge-2026-08-17.md +++ b/docs/operations/template-merge-2026-08-17.md @@ -211,14 +211,67 @@ parents. To compare against a pre-merge state, use a separate checkout rather than stashing an in-progress merge. +## Correction after review + +Two of the resolutions above were wrong, and were fixed in a follow-up commit. + +The template's demonstration screens — `platform-overview-page`, the UI and +state galleries, the auth example and the reference feature's screens — exist to +explain the template. A product replaces them with its domain, and deleting them +is the expected end state, not a regression. Two gates were nevertheless coupled +to them, and the first resolution accommodated that coupling instead of fixing +it. + +### `tests/unit/product-features.test.ts` — a hard-coded exemption became a rule + +The guard requires every installed registry to compose from the manifest. +`installed-feature-runtimes.tsx` was added to its exempt list once the reference +feature's presentation layer was gone. That silenced the guard for that file +permanently. + +It now derives its own scope: a registry must gate on the manifest **when it +imports a module belonging to a manifest-declared feature**. A product whose +registries compose only its own domain drops out of the rule honestly, and the +guard fires again the moment a declared feature is imported without gating — +verified by removing the manifest reference from +`installed-feature-adapters.ts` and watching the guard fail. A counter asserts +the sweep is still watching at least one file, so an empty scope cannot pass +silently. + +### `tests/component/product-feature-switch.test.tsx` — coverage restored + +The end-to-end kill-switch assertions were replaced with composition checks +because the screens they rendered were gone. The mechanism under test is the +ownership lookup plus `isFeatureActive`, which has nothing to do with which +screens ship, so **the ownership map is now the fixture**: one real registered +route is attributed to a real installed feature, and the router, shell, +components and codecs are all the product's own. The deep-link half is asserted +end to end again. + +### What that restoration exposed + +The navigation-withdrawal half **is not implemented in this product**. It lives +in the template's `PrimaryNavigation`, and this product does not render the +template's `AppShell` at all — the public site header is a hand-written list of +paths in `src/features/tech-log/presentation/public/components/site-header.tsx`, +and the studio has its own shell. + +So a disabled feature's route is refused by the router but its link would still +be advertised. That is harmless only while no feature-owned route is navigable, +which is true today and is now asserted. If that assertion fails, the header has +to consult `ROUTE_FEATURE_OWNER` — or navigation has to move back onto +`NAVIGATION_ROUTES` — before the route ships. + ## Follow-ups this merge deliberately did not decide 1. **TechLog is outside the product manifest.** It is composed directly rather than as a `SelectableProductFeature`, so the runtime kill switch does not - govern it. Promoting it is a product decision. -2. **The runtime kill switch currently governs no route.** The only installed - feature's screens were removed, so the switch still gates operations and - schemas but has no user-visible effect. It starts asserting again as soon as - a feature-owned route is registered. -3. **The reference feature declares routes it cannot serve.** Either its screens - return, or its route declarations should be withdrawn from its contract. + govern it. That is defensible — a product's own domain is not an optional + feature — but it means the switch governs nothing user-visible today. +2. **The reference feature declares routes it cannot serve.** Its screens were + deleted with the rest of the demonstration UI, but its contract still + declares `REFERENCE_RESOURCE_*` routes. Either the declarations go, or the + feature does. `TechLog` does not import it (`git grep reference-feature -- + src/features/tech-log` is empty), so removing it is a live option and + FE-GATE-020 exists to prove it can be removed. +3. **The public site header is not feature-aware.** See above. diff --git a/tests/component/product-feature-switch.test.tsx b/tests/component/product-feature-switch.test.tsx index 4796ee0..6f61094 100644 --- a/tests/component/product-feature-switch.test.tsx +++ b/tests/component/product-feature-switch.test.tsx @@ -1,47 +1,149 @@ -import { describe, expect, it } from "vitest"; +// @vitest-environment jsdom -import { - ROUTE_FEATURE_OWNER, - ROUTE_REGISTRY, -} from "../../src/features/installed-feature-contracts.ts"; -import { ROUTE_RUNTIME } from "../../src/features/installed-feature-runtimes.tsx"; -import { INSTALLED_PRODUCT_FEATURE_IDS } from "../../src/features/installed-product-manifest.ts"; +import { render, screen } from "@testing-library/react"; +import { describe, expect, it, vi } from "vitest"; + +import { createAnonymousSessionAdapter } from "../../src/adapters/auth/external-session-adapter.ts"; +import { createTechLogFeatureInstalledInput } from "../../src/features/tech-log/adapters/create-tech-log-feature-input.ts"; +import { createTestApplication } from "../helpers/create-test-application.ts"; +import { createProductFeaturesStub } from "../helpers/runtime-capabilities-stub.ts"; /** - * §3.5. The runtime kill switch, as it applies to *this* product. + * §3.5. The runtime kill switch, exercised through the running app. * - * The template asserted the switch end to end by rendering the reference - * feature's screens and taking them out of service. This product deleted those - * screens during the UI migration, so that render is no longer possible: the - * feature contributes API operations, schemas and mappers, but no route. + * Withdrawing a feature from navigation is not the same as taking it out of + * service: a typed deep link would still mount it. Both halves are asserted on + * the same render, so the switch cannot be half-wired. * - * Deleting the test with the screens would have removed the only thing watching - * this seam, so what is asserted here is the invariant that survives the - * migration and would have caught the trap the merge itself nearly introduced — - * a route registered from a feature contract whose component no longer exists. - * The moment a feature-owned route is registered again, the second test starts - * asserting the switch end to end without being rewritten. + * The template asserted this by rendering its own demonstration screens. Those + * screens exist to explain the template, and a product replaces them with its + * domain — so a test that needs them makes deleting them look like a + * regression. It is not: the mechanism under test is the ownership lookup plus + * `isFeatureActive`, and neither has anything to do with which screens ship. + * + * So the ownership map is the fixture. `ROUTE_FEATURE_OWNER` is empty in this + * product because no installed feature contributes a route; one real registered + * route is attributed to a real installed feature here, and everything else — + * router, shell, components, codecs — is the product's own. */ + +const OWNED_ROUTE_ID = "TECH_LOG_EXPLORE"; +const OWNER_FEATURE_ID = "reference-feature"; +const OWNED_ROUTE_PATH = "/explore"; +const OWNED_ROUTE_NAV_LABEL = "탐색"; + +vi.mock("../../src/features/installed-feature-contracts.ts", async (original) => { + const actual = await original< + typeof import("../../src/features/installed-feature-contracts.ts") + >(); + return { + ...actual, + ROUTE_FEATURE_OWNER: Object.freeze({ + [OWNED_ROUTE_ID]: OWNER_FEATURE_ID, + }), + }; +}); + +const { AppRouter } = await import("../../src/presentation/routes/app-router.tsx"); +const { ApplicationProvider } = await import( + "../../src/presentation/providers/application-provider.tsx" +); + +function renderAt(path: string, disabled: boolean) { + window.history.pushState({}, "", path); + return render( + + + , + ); +} + +function outOfServiceSurface() { + return screen.queryByText( + (_, element) => + element?.getAttribute("data-disabled-feature") === OWNER_FEATURE_ID, + ); +} + describe("runtime product feature switch", () => { - it("registers no route that cannot be mounted", () => { + it("takes the owned route out of service for a direct deep link", async () => { + renderAt(OWNED_ROUTE_PATH, true); + const surface = await screen.findByText( + (_, element) => + element?.getAttribute("data-disabled-feature") === OWNER_FEATURE_ID, + {}, + { timeout: 5000 }, + ); + expect(surface).toBeTruthy(); + }); + + it("serves the same deep link while the feature is active", async () => { + renderAt(OWNED_ROUTE_PATH, false); + expect(outOfServiceSurface()).toBeNull(); + }); +}); + +/** + * Composition invariants that hold whichever screens a product ships. The first + * one is what caught a bad merge resolution: composing the route registry from + * every installed feature's `contract.routes` registered the reference + * feature's paths after this product had deleted their components. + */ +describe("route composition", () => { + it("registers no route that cannot be mounted", async () => { + const { ROUTE_REGISTRY } = await vi.importActual< + typeof import("../../src/features/installed-feature-contracts.ts") + >("../../src/features/installed-feature-contracts.ts"); + const { ROUTE_RUNTIME } = await import( + "../../src/features/installed-feature-runtimes.tsx" + ); const unmountable = Object.keys(ROUTE_REGISTRY).filter( (routeId) => !(routeId in ROUTE_RUNTIME), ); expect(unmountable).toEqual([]); }); - it("keeps every feature-owned route inside the installed selection", () => { - // A route owned by a feature the manifest did not install would be - // reachable with nothing behind it. - const orphaned = Object.entries(ROUTE_FEATURE_OWNER) - .filter(([routeId]) => routeId in ROUTE_REGISTRY) - .filter(([, featureId]) => !INSTALLED_PRODUCT_FEATURE_IDS.includes(featureId)); - expect(orphaned).toEqual([]); + /** + * The other half of the switch — withdrawing a disabled feature's entry from + * navigation — lives in the template's `PrimaryNavigation`, which this + * product does not render: TechLog's public header is a hand-written list of + * paths, and the studio shell has its own. So the withdrawal half is not + * implemented here. + * + * That is harmless only while no feature-owned route is navigable, which is + * what this asserts. If it fails, the header has to consult + * `ROUTE_FEATURE_OWNER` (or navigation has to move back onto + * `NAVIGATION_ROUTES`) before that route ships — otherwise a disabled + * feature would keep advertising a link to a page that refuses to load. + */ + it("has no navigable feature-owned route while the shell is not feature-aware", async () => { + const actual = await vi.importActual< + typeof import("../../src/features/installed-feature-contracts.ts") + >("../../src/features/installed-feature-contracts.ts"); + const navigableOwned = actual.NAVIGATION_ROUTES.filter( + (definition) => actual.ROUTE_FEATURE_OWNER[definition.routeId] !== undefined, + ).map((definition) => definition.routeId); + expect(navigableOwned).toEqual([]); }); - it("still declares a product manifest to narrow", () => { - // The switch is only meaningful while something is selectable; if this ever - // empties, the mechanism above is dead code rather than a guarantee. - expect(INSTALLED_PRODUCT_FEATURE_IDS.length).toBeGreaterThan(0); + it("attributes ownership only to routes the product registered", async () => { + const actual = await vi.importActual< + typeof import("../../src/features/installed-feature-contracts.ts") + >("../../src/features/installed-feature-contracts.ts"); + for (const routeId of Object.keys(actual.ROUTE_FEATURE_OWNER)) { + expect(Object.keys(actual.ROUTE_REGISTRY)).toContain(routeId); + } }); }); diff --git a/tests/unit/product-features.test.ts b/tests/unit/product-features.test.ts index 67817e2..a771db6 100644 --- a/tests/unit/product-features.test.ts +++ b/tests/unit/product-features.test.ts @@ -186,23 +186,37 @@ describe("every installed registry consults the manifest", () => { "installed-product-manifest.ts", // Capabilities have their own §3.5 selection file and override vocabulary. "installed-runtime-capabilities.ts", - // Template merge. This product removed the reference feature's screens, - // so no installed feature contributes a route component and this file has - // nothing to gate. `tests/component/product-feature-switch.test.tsx` - // holds the mountability invariant meanwhile; restore this entry to the - // guarded set the day a feature contributes a route runtime again. - "installed-feature-runtimes.tsx", // Message keys stay total on purpose; see the file for why. "installed-feature-messages.ts", ]); + // A registry has to gate on the manifest when it composes something a + // manifest-declared feature owns. Deriving that from the file's own imports + // is the difference between a rule and a list: a product that replaces the + // template's demonstration feature with its own domain drops out of the + // rule honestly, and the day it imports a declared feature again the guard + // fires without anyone having to remember to re-add it. + // + // An earlier fix named `installed-feature-runtimes.tsx` in the exempt set + // once this product deleted the reference feature's screens. That silenced + // the guard for that file permanently — a worse outcome than the problem it + // solved. + let guarded = 0; for (const registry of registries) { if (exempt.has(registry)) continue; const source = await readFile(nodePath.join(root, registry), "utf8"); + const composesDeclaredFeature = COMPILED_PRODUCT_FEATURE_IDS.some( + (featureId) => source.includes(`./${featureId}/`), + ); + if (!composesDeclaredFeature) continue; + guarded += 1; expect( /INSTALLED_PRODUCT_FEATURE(S|_IDS)/u.test(source), - `${registry} must compose from the product manifest`, + `${registry} composes a manifest-declared feature and must gate on the manifest`, ).toBe(true); } + // The rule has to still be watching something; an empty sweep would pass + // no matter what happened to these files. + expect(guarded).toBeGreaterThan(0); }); });