From a49c76b5b2137777d610376433674e054d459f83 Mon Sep 17 00:00:00 2001 From: DongHyeonka Date: Sat, 1 Aug 2026 19:43:33 +0900 Subject: [PATCH] docs: define refactoring review remediation --- ...1-refactoring-review-remediation-design.md | 322 ++++++++++++++++++ 1 file changed, 322 insertions(+) create mode 100644 docs/superpowers/specs/2026-08-01-refactoring-review-remediation-design.md diff --git a/docs/superpowers/specs/2026-08-01-refactoring-review-remediation-design.md b/docs/superpowers/specs/2026-08-01-refactoring-review-remediation-design.md new file mode 100644 index 0000000..88fa312 --- /dev/null +++ b/docs/superpowers/specs/2026-08-01-refactoring-review-remediation-design.md @@ -0,0 +1,322 @@ +# Refactoring Review Remediation Design + +## Purpose + +Complete the existing runtime-integrity work, preserve the current uncommitted +implementation snapshot, and then close the correctness and evidence gaps found +by the repository-wide module, script, schema, and test review. + +This design supplements `2026-08-01-runtime-integrity-refactor-design.md`. It +does not replace the existing release/boot or scope-owned server-state plans. +Those changes remain the baseline on which the remediation phases build. + +## Chosen approach + +Use a staged, in-place compatibility migration with test-first behavior fixes. + +- Preserve every pre-existing dirty-worktree change. Do not reset, restore, or + replace the current implementation with a clean-branch rewrite. +- Complete and verify the existing release/boot and scope-generation work + before changing its public contracts. +- Add a failing regression test before every production behavior change. +- Fix active correctness and release blockers before structural extraction. +- Keep public facades stable while removing duplicate internal authorities. +- Do not enable the Service Worker capability until its bounded marker reader, + full identity handshake, and ACTIVE fixture build are verified. + +A big-bang rewrite is rejected because query ownership, HTTP execution, +release evidence, and optional runtimes have independent failure modes. A +P0/P1-only patch is also rejected because it would leave duplicate registries +and misleading quality gates that can recreate the same defects. + +## Program phases + +The work is delivered as independently testable sub-projects in this order. + +1. Existing implementation baseline +2. Query invalidation correctness +3. Mutation intent and effect settlement +4. Immutable release promotion +5. Release/evidence contract enforcement +6. Quality and architecture gate accuracy +7. HTTP contract and layer consolidation +8. Service Worker hardening +9. Characterized adapter decomposition + +Each phase must leave type checking, linting, and its focused tests green. A +later phase may depend only on explicit interfaces produced by an earlier one. + +## Existing implementation baseline + +The current dirty snapshot contains the in-progress Release Manifest V2, exact +boot pairing, scope-generation ownership, HTTP V3 executor, and optional runtime +foundation. It is the source of truth for this program. + +Before remediation begins, run the focused suites from the existing +release/boot and scope-owned server-state plans. Resolve failures in those +plans without changing the remediation contracts below. Record browser- or +artifact-producing gates separately when they cannot run in the current +environment. + +## Query invalidation architecture + +One query-key module owns both bound keys and invalidation prefixes. Query-key +schema version 2 is: + +```text +["query", 2, namespaceId, namespaceVersion, scopeFingerprint, + definitionVersion, identityToken] +``` + +The invalidation prefix for the same namespace is exactly: + +```text +["query", 2, namespaceId, namespaceVersion] +``` + +Features declare namespace identities and topic-to-namespace edges; they never +copy TanStack key shapes. Bootstrap composes one `InvalidationRegistry`, calls +`indexInvalidationRegistry` once, and passes the resulting many-to-many index +to the cache coordinator. Cross-context messages continue to carry opaque +topics only. The receiving coordinator resolves every namespace for that +topic locally. + +The legacy flat query registry and the empty registry in `query-keys.ts` are +removed after static usage reaches zero. Query persistence is disabled, so the +key-version change requires no persisted-cache migration. + +## Mutation intent architecture + +`MutationIntent` is an application-level command identity, not an HTTP adapter +detail. The contracts layer defines its immutable shape, the application layer +defines `MutationIntentFactory`, and a browser adapter implements the factory +with `crypto.randomUUID()`. + +```ts +type MutationIntent = Readonly<{ + intentId: string; + operationId: string; + canonicalInputIdentity: string; + idempotencyKey?: string; + createdAtMonotonicMs: number; +}>; +``` + +`useApplicationMutation` creates exactly one intent after duplicate admission +and before optimistic projection. The intent travels through the bound mutation +execution context and feature input to `ContractHttpExecutor`. Every physical +retry and reconciliation of that logical submit reuses the same intent. A new +submit always creates a new intent. + +For a `KEYED` descriptor, the executor requires a bounded, non-empty +idempotency key before credential resolution or fetch admission. Missing or +invalid keys return a pre-dispatch contract violation with `NOT_STARTED` and +perform zero fetches. Query operations never emit an idempotency header. Intent +IDs and keys are forbidden in URLs, query keys, diagnostics, and telemetry. + +## Mutation effect settlement + +Command failures carry mandatory effect certainty. The mutation bridge derives +settlement before changing optimistic state: + +```text +NOT_STARTED | NOT_APPLIED -> rollback +APPLIED_CONFIRMED -> commit, then invalidate +MAYBE_APPLIED -> retain uncertain layer; do not retry or invalidate +``` + +`OptimisticLayerLease` gains an uncertain state and an explicit reconciliation +operation. An uncertain layer remains ordered with later layers and cannot be +collapsed into the base until inspection resolves it as applied or not applied. +The application async state exposes `mutation-effect-unknown`, and the +controller exposes a reconciliation action associated with the original +intent. Unknown effect never silently maps to conflict, success, or generic +retryable failure. + +## Immutable release promotion + +The release workflow has one byte-producing authority. + +1. `immutable_build` creates `dist`, build manifest, module inventory, and the + local supply-chain documents once. +2. The job publishes one immutable bundle and its `distSha256`. +3. Provider jobs scan and attest that exact digest. +4. The promotion job downloads the bundle and provider evidence, supplies + `VULNERABILITY_REPORT_PATH` and `PROVENANCE_ATTESTATION_PATH`, and verifies + schemas, signatures, and digest identity. +5. Promotion consumes the verified bundle without rebuilding it. + +The current `build:release` command is split into byte production, local +evidence generation, provider verification, and promotion verification. +Missing provider evidence remains `FAIL_UNVERIFIED`; it is never converted to a +local pass. An attestation for a different digest, or any rebuild after +attestation, fails promotion. + +This repository validates but does not fabricate external provider evidence. +The CI environment must supply a vulnerability report and signed provenance +attestation produced for the published candidate digest. Until that external +integration is configured, candidate build and local verification may pass but +promotion remains intentionally unavailable. + +## Release and evidence contracts + +One async `verifyReleaseRuntimeCoherence` policy is used by `verify-release` +and the rollback runbook. V1 verifies the legacy scalar tuple. V2 verifies the +exact package set and recomputes the contract-set digest. V2 never synthesizes +an API contract version. + +All machine-readable evidence writers call a common validated writer before +touching the destination: + +```ts +writeValidatedJsonArtifact({ path, schema, value }): Promise +``` + +Executable Zod schemas are authoritative. Checked JSON schemas are generated +views and a `--check` command fails on drift. CI maps every evidence path to an +executable schema and validates content before upload, rather than checking +existence only. + +`verifyBuildManifestOutputs` confines declared paths to their approved roots, +parses the module inventory, and compares its raw SHA-256 with +`moduleInventoryHash`. + +Repository file discovery is fail-closed. Required roots fail on absence, +permissions, or read errors; optional roots permit only explicit `ENOENT`. +Provenance and secret scanning share the tracked repository inventory so build +inputs such as `index.html`, Vite configs, TypeScript configs, `.nvmrc`, and the +provider workflow cannot be silently omitted. + +## Quality and architecture gates + +Coverage reports distinguish `selectedTotal` from `repositoryTotal`. +`repositoryTotal` contains every production TypeScript module and starts with a +non-decreasing baseline. A critical-module registry immediately includes HTTP +V3, request/response bounds, boot bounds, service-worker lifecycle, scope +generation, and release loading. A changed high-risk module must have a policy +entry or an owned, expiring waiver. + +The HTTP scenario catalog is executable input to table-driven contract tests. +Declaring a scenario without executing its status, effect, retry, fetch-count, +media-type, body-bound, and scope-fence expectations does not satisfy the gate. + +`config/ci/gates.json` is parsed by one shared schema. +`scripts/generate-ci-workflow.ts` deterministically emits the complete provider +workflow, and its `--check` mode fails when the checked-in workflow differs. +Token and regex presence checks are not authoritative. Playwright configurations inherit +`forbidOnly: true`, Vitest rejects `.only`, and fake timers are restored by the +common test setup. + +The Babel/Node resolver graph is the sole authoritative architecture analyzer +while dependency-cruiser cannot parse TypeScript 7. A zero-module graph fails. +Rules prohibit contracts from importing outer application/runtime layers and +feature adapters from importing global concrete adapters. + +## HTTP contract consolidation + +The installed contract contribution is the single source for method, path, +input/output validators, retry semantics, effect classification, deadlines, +and byte bounds. The reference DTO schema is defined once and requires a valid +datetime when `createdAt` is present. Legacy registries and codecs are generated +from the contribution during the compatibility window, then removed after +production and tests have no callers. + +Provider-neutral operation outcomes and typed operation maps live in contracts +or application ports. Feature adapters do not import `HttpExecutionOutcome` +from a concrete HTTP adapter and do not accept `operationId: string` paired with +`input: unknown`. Runtime `REQUEST_TIMEOUT_MS` is a global maximum applied on +top of descriptor deadlines. + +Raw query and mutation overloads are removed from production exports after +callers migrate to `BoundQuery` and `BoundMutation`. Test-only legacy harnesses +remain outside the production public index until their tests migrate. + +## Service Worker hardening + +Activation markers are read through a realm-safe bounded response reader. It +checks declared length, reads at most `maxBytes + 1`, cancels an oversized or +non-terminating stream, decodes fatal UTF-8, and validates the marker record. +`response.text()` is not used for bounded worker protocol data. + +Page and worker exchange a canonical identity digest covering protocol version, +cache schema version, build, release, contract set, and static asset set. This +wire-shape change increments the Service Worker protocol version to 2. Every +tuple-field mutation changes the digest and rejects activation. +The ACTIVE fixture build validates asset entries and recomputes the static set +digest without changing the product's default `null` selection. + +## Adapter decomposition + +Structural extraction begins only after behavior is characterized. + +- IndexedDB runtime and maintenance share one persisted-row schema containing + record, receipt, retention, budget types, guards, and golden fixtures. +- OPFS worker keeps its public facade while browser bootstrap, message host, + core state machine, Web Lock lease, and physical I/O move into focused files. +- Public response cache extracts manifest codec/digest and generic lock logic. +- Download delivery extracts browser-managed, picker-streaming, and object-URL + strategies behind the existing facade. + +Resumable upload, websocket, reconnect, stream coordinator, HTTP V3, and +Browser RPC remain intact unless a behavior test demonstrates an independent +change reason. File length alone is not a split criterion. + +## Error handling + +- Active correctness failures use stable failure kinds and preserve effect + certainty. +- Mandatory release evidence, file inventory, schema, or digest failures fail + closed. +- Optional diagnostics cannot change command, cache, or release outcomes. +- Cleanup continues through all participants but publishes no READY state after + any mandatory failure. +- Tool crashes, null exit status, signals, and timeouts never count as expected + negative-fixture rejection. + +## Testing and verification + +Every behavior change follows red-green-refactor. The minimum regression set +includes: + +- Actual `bindQuery` keys for local, remote, fan-out, and generation-isolated + invalidation. +- Independent runtime intents, same-submit retry identity, missing-key + pre-dispatch rejection, and secret-free diagnostics. +- Optimistic NOT_APPLIED, APPLIED_CONFIRMED, and MAYBE_APPLIED settlement, + including out-of-order layers and reconciliation. +- V1/V2 release coherence and every contract-set tamper in both verifier and + rollback drill. +- Provider evidence absence, digest mismatch, post-attestation rebuild, and a + valid immutable promotion fixture. +- Required-root and unreadable-file failures, schema drift, invalid evidence, + and module-inventory hash mismatch. +- Complete V3 response/effect scenario execution and production read/write E2E. +- Headerless oversized and non-terminating Service Worker marker streams. +- Shared persisted-row acceptance/rejection across IndexedDB runtime and + maintenance before extraction. + +Repository completion requires fresh evidence from type checking, linting, +non-browser tests, coverage, architecture, build, release verification, CI +contract checks, browser capabilities, E2E, accessibility, visual tests where +the environment supports them, schema parity, and diff hygiene. Unsupported +browser gates are reported explicitly and are never claimed as passing. + +## Acceptance criteria + +- The existing release/boot and scope-generation work passes its original + focused suites before remediation contracts are changed. +- Mutation success invalidates every matching bound query locally and remotely. +- No two independent logical commands reuse an idempotency key; missing KEYED + intent prevents network admission. +- MAYBE_APPLIED never causes arbitrary optimistic rollback, commit, retry, or + invalidation. +- Release promotion verifies and promotes the exact same immutable bytes. +- V2 rollback verification cannot bypass contract-set package or digest checks. +- Required files, artifact schemas, and manifest output hashes fail closed. +- Coverage and scenario gates measure production behavior rather than declared + subsets or source tokens. +- Production uses one executable HTTP contract registry and respects clean + architecture dependency direction. +- Service Worker activation is bounded and full-identity coherent before the + capability can become ACTIVE. +- Adapter splits preserve their public facade and pass shared golden tests.