docs: define refactoring review remediation
This commit is contained in:
@@ -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<void>
|
||||
```
|
||||
|
||||
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.
|
||||
Reference in New Issue
Block a user