diff --git a/.claude-plugin/plugin.json b/.claude-plugin/plugin.json index f08fda76..5012dd37 100644 --- a/.claude-plugin/plugin.json +++ b/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "name": "fosmvvm-generators", "description": "FOSMVVM architecture generators for ViewModels, Fields, DataModels, ServerRequests, Leaf Views, and ViewModel Tests", - "version": "2.30.0", + "version": "2.63.0", "author": { "name": "FOS Computer Services" }, diff --git a/.claude/CLAUDE.md b/.claude/CLAUDE.md index c66db62a..7b8a1865 100644 --- a/.claude/CLAUDE.md +++ b/.claude/CLAUDE.md @@ -2,6 +2,10 @@ This file provides guidance to Claude Code (claude.ai/code) when working with code in this repository. +## Attribution Moratorium + +No AI attribution anywhere in this project's artifacts: no AI names, model names, or AI-authorship statements in documents, specifications, code, code comments, commit messages, PR/issue text, or generated files. No `Co-Authored-By`, `Claude-Session`, "Generated with", or similar trailers/banners — in any message or file, ever. Harness-required filenames and paths (`CLAUDE.md`, `.claude/`) are not attribution and are unaffected. + ## Build & Test Commands ```bash diff --git a/.claude/docs/FOSMVVMArchitecture.md b/.claude/docs/FOSMVVMArchitecture.md index c90fd5d3..96499ddd 100644 --- a/.claude/docs/FOSMVVMArchitecture.md +++ b/.claude/docs/FOSMVVMArchitecture.md @@ -1541,6 +1541,23 @@ If a type is needed by both client and server, it belongs in the shared module. - Anything used by `MVVMEnvironment` - Anything that must be consistent across all targets +### The SPMLibraries umbrella — the Xcode-side twin + +The shared module solves agreement between targets that are *compiled together*. An Xcode project has a second version of the same problem: several Xcode targets (app, unit tests, UI tests, frameworks) each consuming the same external SPM package products. + +**When more than one Xcode target consumes SPM package products, vend them through a single `SPMLibraries` umbrella framework that every target depends on — never link the SPM products directly into each target.** `SPMLibraries` is a thin framework whose dependencies list every external package product (`FOSFoundation`, `FOSMVVM`, …); every other target depends on it. + +**Why — a generic Xcode + SPM bug, not FOS-specific.** Linking an SPM library statically into multiple targets compiles a *separate copy of its types into each target*, and Swift's mangled type name carries the linking context. The "same" type then has a different runtime identity per target, so an instance crossing a target boundary fails `is` / `as?` / `==` / `===` against the same type on the other side: **`TypeA != TypeA`**. It compiles clean and breaks at runtime, far from the cause. One umbrella *dynamic* framework means one canonical copy and one shared type identity everywhere. + +**Why it matters especially here.** FOSMVVM leans hard on comparing types — type-derived request paths, ViewModel/Request resolution, versioning. An app that skips the umbrella breaks exactly where those comparisons happen. The umbrella looks like redundant re-vending to a mainstream Xcode eye, which is why it has to be stated rather than left implicit. + +Two carve-outs, both deliberate: + +- **Testing products** (`FOSTesting`, `FOSTestingUI`, `FOSTestingVapor`) stay *out* of the umbrella and link directly into test targets. The umbrella embeds in the shipping app, and testing products must not ride along; their types are never shared across target boundaries, so the identity rule does not apply to them. (Ruled 2026-08-19.) +- **Single-embed.** The app embeds the umbrella and every local framework with sign-on-copy; every other target links without embedding, because the test host already carries the embedded copy. Embedding twice puts two copies in one bundle — the identity failure the umbrella exists to prevent, reintroduced. + +This is enforced in three places, and they must agree: the scaffolder's `project.yml` templates emit it, `fosmvvm-doctor` audits an existing project for it (rules R4a/R4b/R5), and generated projects ship a `memory/spm-libraries-settled.md` carrying the argument for the app's own future sessions. + --- ## File Organization Conventions diff --git a/.claude/skills/fosmvvm-fields-generator/SKILL.md b/.claude/skills/fosmvvm-fields-generator/SKILL.md index 10e9a2ad..6a5eb1c0 100644 --- a/.claude/skills/fosmvvm-fields-generator/SKILL.md +++ b/.claude/skills/fosmvvm-fields-generator/SKILL.md @@ -147,6 +147,8 @@ public protocol {Name}Fields: ValidatableModel, Codable, Sendable { > **Overridable-with-a-default member? Declare it as a *requirement* AND provide the default.** If you want a Fields member to have a zero-config default that a conformer can still override (a validation policy, a message source), it must be a protocol **requirement** with a default in an extension. A member defined *only* in an extension is statically dispatched — a conformer's "override" merely **shadows** it and calls through the protocol/a generic `some {Name}Fields` still hit the default. That's a silent OCP failure. See [Architecture Patterns → Requirement + Default = a Real Override](../shared/architecture-patterns.md). +> **The `{name}FieldsValidateModel(validations:fields:)` composition helper lives in the extension deliberately — it is not an override point** (ratified 2026-08-25). Its protocol-derived prefix is the point: a type adopting two Fields protocols writes one `validate(fields:validations:)` that calls `documentFieldsValidateModel(…)` *and* `otherFieldsValidateModel(…)` — a composition seam, so the requirement-plus-default rule above does not apply to it. (`ValidatableModel.validate(fields:validations:)` itself *is* a real requirement, so a `validate` default in a Fields extension is dynamically dispatched and correctly overridable.) + ### FormField Definition ```swift diff --git a/.claude/skills/fosmvvm-fluent-datamodel-generator/SKILL.md b/.claude/skills/fosmvvm-fluent-datamodel-generator/SKILL.md index cad04dae..22034d4a 100644 --- a/.claude/skills/fosmvvm-fluent-datamodel-generator/SKILL.md +++ b/.claude/skills/fosmvvm-fluent-datamodel-generator/SKILL.md @@ -270,9 +270,9 @@ final class Idea: DataModel, IdeaFields, Hashable, @unchecked Sendable { In schema: `.field("created_by", .uuid, .required, .references(User.schema, "id", onDelete: .cascade))` **When to use each pattern:** -- **Associated type** (`associatedtype User: UserFields`): Required relationships -- **Optional associated type**: Not supported - use `ModelIdType?` for optional FKs -- **Plain `ModelIdType`**: Optional FKs, external system references +- **Associated type** (`associatedtype User: UserFields`): required relationships — `@Parent` satisfies it directly. +- **Optional FK to a table in this database**: `@OptionalParent(key:)` on the model, over a nullable `.references(...)` column. (Optional associated types are not supported on the protocol side; the optional relationship lives on the DataModel only.) +- **Plain `ModelIdType`/`UUID` field**: ONLY for references *outside* this database — an external system's id, a token minted elsewhere — and only with express approval documented at the declaration site, per the firm `ModelIdType Requires Junction Tables Except for @ID` principle. The documentation names its authority (a decision, an issue, an approver) or the condition under which the exception ends — a note that merely describes the reference is not approval. A same-database reference as a raw UUID has no wall: nothing constrains what is written, Fluent cannot load the relation, and the reference dangles silently when the target row goes. ### Migrations @@ -310,6 +310,13 @@ Key points: - Test validation with `@Test(arguments:)` - Create private test struct implementing the Fields protocol +**Database-backed tests bind an ephemeral database — never an inherited one.** A migration or +schema test uses `app.databases.use(.sqlite(.memory), as: .sqlite)` (or an equally ephemeral, +test-constructed binding). Never bind a DSN read from the ambient environment (`DATABASE_URL`) — +the test's target then becomes whatever the shell says, and *Tests Must Never Modify Production +Data* is a firm principle (repo `CLAUDE.md`): tests SHALL NOT modify, delete, or corrupt +production data; isolation is constructed, not inherited. + **Test structs with associated types:** ```swift @@ -350,10 +357,28 @@ private struct TestUser: UserFields { | `Bool` | `.bool` | `BOOLEAN` | | `Date` | `.datetime` | `TIMESTAMPTZ` | | `UUID` | `.uuid` | `UUID` | -| `[UUID]` | `.array(of: .uuid)` | `UUID[]` | | Custom Enum | `.string` | `VARCHAR` (stored as raw value) | | `JSONB` | `.json` | `JSONB` | +> **Decode stored enums honestly.** A raw value read back with a coalescing fallback — `SomeEnum(rawValue: stored) ?? .someCase` — silently rewrites every historical row the current enum no longer names. Decode throwing or into an explicit `.unknown` case; never coalesce into a meaning-bearing category. + +> **No identity arrays.** `[UUID]` (`.array(of: .uuid)`) flattens a relation into a column nothing can constrain — element-level foreign keys do not exist on array columns. A many-to-many is a junction table + `@Siblings`; a design that genuinely wants the array (a subset pointer into an already-related aggregate, say) must clear the same express-approval bar as any raw identity field, naming the integrity cost it accepts. + +> **Identities hide in JSON too.** A `Codable` struct stored as `.json` whose members reference this database's tables by `UUID` carries the same integrity risk as a raw identity column, one struct-level down. Keep same-database references out of JSONB payloads; relate with wrappers and join. + +--- + +## Framework Surface Since v2.1 + +This skill's patterns predate several FOSMVVMVapor releases. Before hand-writing container loading, sorting, filtering, guarded writes, or live-refresh plumbing, check the catalog — these already exist: + +- **`ContainerDataModel` + `ContainmentRelation`** — declare a container's authorization-bearing relations from its own Fluent `@Children`/`@Siblings`/`@Parent` KeyPaths; cardinality and joins come from Fluent, never restated. +- **`SortableDataModel` + `SortMapping`**, **`FilterableDataModel`** — published sort meanings mapped to database ordering, and query-driven narrowing of container loads. +- **`DataModelWriter` + `WriteTargetProviding`** — the guarded write path the CRUD request doors (`CreateRequest`/`UpdateRequest`/`DeleteRequest` registration) require. +- **Live invalidation** — a Fluent-persisted model's committed saves already nudge `.live` clients with no model-side code; non-Fluent sources pair `registerDependency(on:)` / `invalidateProjections(of:)`. + +See [`../shared/api-catalog/FOSMVVMVapor.md`](../shared/api-catalog/FOSMVVMVapor.md) for each one's reach-for entry. + --- ## See Also @@ -376,3 +401,4 @@ private struct TestUser: UserFields { | 1.3 | 2025-12-24 | Factored out Fields layer to fields-generator skill | | 2.0 | 2025-12-26 | Renamed to fosmvvm-fluent-datamodel-generator, added Scope Guard, generalized from Kairos-specific to FOSMVVM patterns, added architecture context | | 2.1 | 2026-01-24 | Update to context-aware approach (remove file-parsing/Q&A). Skill references conversation context instead of asking questions or accepting file paths. | +| 2.2 | 2026-08-25 | Raw-identity rules aligned with the junction-table principle: `@OptionalParent` for same-database optional FKs, no `[UUID]` arrays, no same-database ids in JSONB, express-approval documentation for external references, honest enum decodes. Post-2.1 framework surface pointer (Container/Sortable/Filterable DataModel, DataModelWriter, live invalidation). | diff --git a/.claude/skills/fosmvvm-review/SKILL.md b/.claude/skills/fosmvvm-review/SKILL.md index 5cc13645..8d33e2fa 100644 --- a/.claude/skills/fosmvvm-review/SKILL.md +++ b/.claude/skills/fosmvvm-review/SKILL.md @@ -1,6 +1,6 @@ --- name: fosmvvm-review -description: Review FOSMVVM code against per-area check files. Triages changed files by area, dispatches one subagent per affected area for parallel review, emits severity-tagged report. Report-only, no auto-fix. Use when reviewing a branch before merge, sweeping the codebase periodically, or in CI. +description: Review FOSMVVM code in two tiers - the deterministic fosmvvm-doctor structural audit first (structural errors halt area review), then per-area check files dispatched one subagent per affected area. Emits one severity-tagged report covering both tiers. Report-only, no auto-fix. Use when reviewing a branch before merge, sweeping the codebase periodically, or in CI. homepage: https://swiftpackageindex.com/foscomputerservices/FOSUtilities/documentation/fosmvvm --- @@ -8,7 +8,7 @@ homepage: https://swiftpackageindex.com/foscomputerservices/FOSUtilities/documen > **Read [`shared/functional-discipline.md`](../shared/functional-discipline.md) before proceeding.** Every rule below derives from it. -Reviews FOSMVVM-area Swift files against per-area check files in `checks/`. Designed for both interactive use and CI integration. +Reviews a project in two tiers, one report. **Tier 1** is `fosmvvm-doctor` — the compiled, deterministic audit of project structure (Step 2); structural errors halt everything downstream, because area reviews assume a project shaped the way the scaffolder shapes it. **Tier 2** reviews the Swift sources against the per-area check files in `checks/`. Designed for both interactive use and CI integration. ## When to Use This Skill @@ -24,8 +24,8 @@ Parse the `args` string for these flags. Order does not matter; unknown args pro | Arg | Effect | Default | |-----|--------|---------| | (none) | Scope = branch diff vs `--base`. | Branch diff | -| `--all` | Scope = all `Sources/**/*.swift` and `Tests/**/*.swift`. | — | -| `` | Scope = `.swift` files under ``. | — | +| `--all` | Scope = all reviewable files (`.swift`, `.leaf`, `.tsx`, `.jsx`) under `Sources`, `Tests`, `Resources`. | — | +| `` | Scope = reviewable files under ``. | — | | `--base ` | Override diff base for default scope. | `main` | | `--format md\|json` | Report format. | `md` | | `--output ` | Write report to file (else stdout). | stdout | @@ -37,15 +37,15 @@ Parse the `args` string for these flags. Order does not matter; unknown args pro ### Step 1: Resolve Scope and Load Project Config -**Scope:** -- If `` given: `find -name '*.swift' -type f`. -- If `--all`: `find Sources Tests -name '*.swift' -type f` from repo root. -- Else (default): `git diff --name-only ...HEAD -- '*.swift'` where `` is `--base` value or `main`. +**Scope:** reviewable files are `*.swift`, `*.leaf`, `*.tsx`, and `*.jsx` — the view edge has three rendering surfaces, and a Swift-only scope silently exempts the Leaf and React ones. +- If `` given: `find -type f \( -name '*.swift' -o -name '*.leaf' -o -name '*.tsx' -o -name '*.jsx' \)`. +- If `--all`: the same `find` over `Sources Tests Resources` from the repo root (skip `node_modules` and `.build`). +- Else (default): `git diff --name-only ...HEAD -- '*.swift' '*.leaf' '*.tsx' '*.jsx'` where `` is `--base` value or `main`. If the resulting file list is empty: - Empty diff: print "No changes to review." Exit 0. -- Path with no `.swift` files: print "No files in scope at ``." Exit 0. -- `--all` with no files: print "No Swift files found." Exit 0. +- Path with no reviewable files: print "No files in scope at ``." Exit 0. +- `--all` with no files: print "No reviewable files found." Exit 0. **Project config:** Look for `.fosmvvm-review.yml` at the repo root. If present, parse: @@ -57,7 +57,31 @@ If the file is missing or any key is absent, use defaults. If the file is malfor Apply `excluded_paths` immediately to filter the scoped file list before triage. -### Step 2: Load Check Files +### Step 2: Tier 1 — Structural Audit (doctor) + +Before any triage, run the deterministic structural audit. `fosmvvm-doctor` checks the project against the scaffolder's rules — build settings, linkage and embedding, test plans, entitlements, deployment floors. Deterministic questions stay in compiled code: this skill never re-derives what doctor already answers. + +**How to run it** — first route that applies: + +1. The project is a Swift package whose FOSUtilities pin ships the plugin (0.15+): from the repo root, `swift package fosmvvm-doctor --json`, plus `--shape ` when the shape is known. Judge the pin from `Package.resolved` (the resolved version), not the `Package.swift` requirement — `from: "0.14.0"` can resolve past 0.15. When no `Package.resolved` exists yet, try this route and fall through on failure. +2. Otherwise — an Xcode-only project, or a pre-plugin pin — run from a FOSUtilities checkout: `swift run fosmvvm-bootstrap doctor --project --json`, plus the same `--shape` flag when the shape is known (it moves the shape-dependent rules from `unchecked` into the audit). (The checkout is only a host; nothing is written anywhere.) + +Both routes exit non-zero when doctor finds errors — capture stdout regardless of exit status (append `|| true`). +3. Neither available (no macOS, no checkout): tier 1 is **unavailable**. Record it as such below and continue to Step 3 — the absence is stated in the report, never silent. Name the enabling route (the `CreatingAProject` DocC article, § Diagnosing an existing project). + +**Parse the JSON:** `findings` (each carrying `severity` — `error` | `warning` — an optional `target`, `summary`, `remedy`), `unchecked`, and `hasErrors`. + +**The gate (ruled 2026-08-25): structural errors halt tier 2.** When `hasErrors` is true, do not dispatch any area subagent — fix structure first, so that the area reviews find what they expect where they expect. Skip to Step 7 and emit the report now, with: + +- the `structure` section carrying every doctor finding and the `unchecked` list, +- `summary.by_area.structure` and `summary.total` counting them (severity mapping: doctor `error` → `blocker`, `warning` → `warning`), +- `"tier2": "halted"` in JSON; in Markdown, a `**Tier 2: halted**` line stating that doctor reported structural errors and area review runs after they are fixed. + +When doctor reports only warnings, or nothing: record the results in the same `structure` section and continue to Step 3. + +Doctor findings are deterministic facts, not review judgments: they are exempt from `.fosmvvm-review.yml` overrides and inline suppression, and they carry no check names — each finding's `remedy` is the action. + +### Step 3: Load Check Files Read all `checks/*.md` from this skill's base directory. Parse YAML frontmatter (`area`, `generator-skill`, `where`). @@ -65,7 +89,7 @@ Apply project config: - Drop any `## Check: ` section whose name appears in `disabled_checks`. - Override `**Severity:**` lines for checks listed in `severity_overrides`. -### Step 3: Triage — Match Files to Areas +### Step 4: Triage — Match Files to Areas For each scoped file, test against each check file's `where:` globs. Build a map `area → [files]`. A file may match multiple areas (acceptable — different lenses). @@ -73,7 +97,7 @@ Always include `cross-cutting` in the dispatch list when scope is non-empty, reg Areas with no matched files (other than `cross-cutting`) are skipped. -### Step 4: Dispatch Subagents +### Step 5: Dispatch Subagents For each area in the dispatch list, dispatch a Task tool subagent (general-purpose) with the prompt template below. Run up to **4 subagents in parallel** (cap chosen to balance throughput against token usage; tune in a future plan if needed). @@ -107,8 +131,13 @@ The "right way" lives in the `{generator_skill}` skill. Treat its SKILL.md as th - `// fosmvvm-review:disable ` / `// fosmvvm-review:enable ` block markers wrapping the candidate. If the matching check is suppressed, omit the finding. If a directive matches but has no justification text after the rule name, instead emit a `suppression-without-justification` finding (defined in `cross-cutting.md`). 5. Apply Reviewer Guidance: do NOT recommend the listed anti-patterns even if they "look like" simplifications. -6. If no findings, say "No findings." -7. Do NOT fix anything. Report only. +6. **Establish the pinned FOSUtilities version before grading any check that names an API.** A check that says "use `uiTestingElement(_:)`" is not a defect report against a codebase written before that API shipped. Read the pin — and read the right one: an Xcode-project area is governed by `*.xcodeproj/project.xcworkspace/xcshareddata/swiftpm/Package.resolved`, which can disagree with the root SPM `Package.resolved`. Where the API postdates the pin, report the finding as **correct at time of writing, now fixable** and say which version lifts it, rather than as an authored defect. Where the pin's own source contradicts a comment in the code under review, the source wins — treat any comment describing framework internals as a claim to verify, not context to trust. +6. **Never invent a check name.** The names above are the complete set for this area. If you find a real violation of the generator skill that no check covers — which will happen, because several areas are thinly covered — report it under the literal check name `uncovered-{area}` and name the generator-skill rule it breaks in the explanation. Do NOT coin a plausible-sounding name: check names are a stable contract that suppression directives, `.fosmvvm-review.yml`, and CI gates all address by name, and a fabricated one silently belongs to no rule and cannot be configured, suppressed, or trusted to reappear on the next run. +7. **Grade an `uncovered-{area}` finding on the same severity scale as the named checks** — blocker when it breaks at runtime, warning when it degrades the development experience. The absence of a check is not itself a severity. +8. If no findings, say "No findings." +9. Do NOT fix anything. Report only. + +If you emitted any `uncovered-{area}` findings, end your report with a section titled `## Coverage gap` listing, one line each, the rule the check file should encode to catch them next time. This is the signal that the area needs checks written — say it plainly rather than papering over it with names that look official. Format each finding as: - **{severity}** [{check-name}] {repo-relative-path}:{line} @@ -119,13 +148,13 @@ Format each finding as: Substitute `{area}`, `{file_list}`, `{reviewer_guidance_section_or_"(none)"}`, `{generator_skill}`, and `{full_check_section_text}` from the loaded check file before dispatching. -### Step 5: Aggregate Findings +### Step 6: Aggregate Findings Collect each subagent's findings. Parse them into structured records: `{severity, area, file, line, check, message, prevention}`. If a subagent returned an error or timeout, record the area as `ERROR` with the failure message; do not abort other areas. -### Step 6: Emit Report +### Step 7: Emit Report #### Markdown format (`--format=md`, default) @@ -133,22 +162,37 @@ If a subagent returned an error or timeout, record the area as `ERROR` with the # FOSMVVM Review **Scope:** {scope description} ({N} files) -**Areas triaged:** {comma-separated areas} +**Tier 1 (doctor):** {ran | unavailable — reason and the enabling route} +**Tier 2:** (only when halted) halted — doctor reported structural errors; fix structure first, then re-run +**Areas triaged:** {comma-separated areas, or "none — tier 2 halted"} **Fail-on threshold:** {threshold} **Configuration applied:** (omit line if no config) disabled checks: {names}; severity overrides: {name=severity, ...}; excluded paths: {N} +## Structure (doctor) +(omit the section when tier 1 ran clean with nothing unchecked) +- {❌ error | ⚠️ warning} {target or (project)}: {summary} → {remedy} +- Not checked: {each unchecked entry, one line} + ## Findings by area +- structure: {N} ({Bb / Ww}) (present when doctor reported anything; doctor error → blocker, warning → warning) - {area}: {N} ({Bb / Ww / Nn}) - ... ## Blockers -{findings, grouped} +{tier-2 findings, grouped} ## Warnings -{findings, grouped} +{tier-2 findings, grouped} ## Nits -{findings, grouped} +{tier-2 findings, grouped} + +(The Blockers/Warnings/Nits sections carry **tier-2 findings only** — the markdown mirror of the JSON rule that tier-1 findings never enter `findings[]`. Doctor's full detail lives in the Structure section; its counts appear in "Findings by area" under `structure`. On a halted run the Structure section is the whole story and the tier-2 sections are empty.) + +## Coverage gaps +(omit the section when there are no `uncovered-*` findings) +Real violations no check covers — these areas need checks written: +- {area}: {N} uncovered ({M} of them blockers) → rules to encode: {one line each} ## Generator skill signals Areas with elevated findings — candidates for generator skill updates: @@ -159,11 +203,25 @@ Areas with elevated findings — candidates for generator skill updates: - {area}: {error message} ``` +`uncovered-*` findings count toward the severity totals like any other — a real +blocker is a blocker whether or not someone had written the check yet. The +separate section exists so the *gap* stays visible rather than dissolving into +the general finding list, and so a run against a thinly-covered area cannot be +mistaken for a clean one. + #### JSON format (`--format=json`) ```json { "scope": { "description": "...", "file_count": 12 }, + "tier1": "ran", + "tier2": "ran", + "structure": { + "findings": [ + { "severity": "error", "target": "SPMLibraries", "summary": "...", "remedy": "..." } + ], + "unchecked": ["entitlements match the project shape (needs --shape)"] + }, "areas_triaged": ["viewmodel", "swiftui-view", "cross-cutting"], "config": { "fail_on": "blocker", @@ -172,12 +230,18 @@ Areas with elevated findings — candidates for generator skill updates: "excluded_paths_count": 0 }, "summary": { - "by_area": { "viewmodel": { "blocker": 1, "warning": 2, "nit": 0 }, "...": {} }, - "total": { "blocker": 1, "warning": 2, "nit": 0 } + "by_area": { "structure": { "blocker": 1, "warning": 0, "nit": 0 }, "viewmodel": { "blocker": 1, "warning": 2, "nit": 0 }, "...": {} }, + "total": { "blocker": 2, "warning": 2, "nit": 0 }, + "uncovered": { "viewmodel": 3, "swiftui-app-setup": 5 } }, "findings": [ { "severity": "blocker", "area": "viewmodel", "file": "...", "line": 42, - "check": "ops-no-output-reads", "message": "...", "prevention": "fosmvvm-viewmodel-generator" } + "check": "ops-no-output-reads", "message": "...", "prevention": "fosmvvm-viewmodel-generator" }, + { "severity": "warning", "area": "viewmodel", "file": "...", "line": 88, + "check": "uncovered-viewmodel", "message": "...", "prevention": "fosmvvm-viewmodel-generator" } + ], + "coverage_gaps": [ + { "area": "viewmodel", "count": 3, "rules_to_encode": ["vmId derivation on list rows", "..."] } ], "errors": [ { "area": "ui-tests", "message": "subagent timeout" } @@ -185,14 +249,29 @@ Areas with elevated findings — candidates for generator skill updates: } ``` +`summary.uncovered` lets a CI wrapper track whether coverage is improving — +`jq '.summary.uncovered | add // 0'` trending down means checks are being +written. Gate on it only deliberately: a thinly-covered area reports a high +number through no fault of the code under review. + +There is **one summary**: doctor findings count in `summary.by_area.structure` +and in `summary.total` alongside every other area (doctor `error` → `blocker`, +`warning` → `warning`), so the CI contract — `jq '.summary.total.blocker == 0'` +— covers both tiers without forking. `tier1` is `"ran"` or +`"unavailable: "`; `tier2` is `"ran"` or `"halted"`. When halted, +`areas_triaged` is empty, `findings` carries no tier-2 records, and the +`structure` section is the whole story. The full doctor detail (targets, +remedies, unchecked) lives only in `structure` — tier-1 findings do not appear +in the `findings[]` array, which remains check-name-addressable tier-2 records. + If `--output ` given, write to file; else stdout. -### Step 7: Annotate Failure Threshold +### Step 8: Annotate Failure Threshold The skill runs inside Claude Code and cannot directly control the shell exit code. Instead, record the configured `--fail-on` threshold in the report so out-of-process wrappers can translate findings to exit codes: - **Markdown report:** include a `**Fail-on threshold:** {threshold}` line in the header. -- **JSON output:** include a top-level `"config": { "fail_on": "", ... }` field (already present per the JSON schema in Step 6). +- **JSON output:** include a top-level `"config": { "fail_on": "", ... }` field (already present per the JSON schema in Step 7). CI consumers invoke the skill via `claude -p` and parse the JSON to decide whether to fail the build: @@ -250,9 +329,19 @@ Block scope: **Justification is required.** A suppression without text after the check name produces a `suppression-without-justification` finding (warning). This forces explicit documentation of every silenced check. +## Coverage state + +The coverage ledger's register is closed — every gap it identified has a shipped check (most recently G22–G26 in 2.62.0). As of plugin 2.62.0: + +- **Covered:** `cross-cutting` (20 checks), `viewmodel` (13), `view` (9 — multi-surface: SwiftUI/Leaf/React), `serverrequest` (9), `swiftui-app-setup` (6), `datamodel` (5), `viewmodel-test` (5), `ui-tests` (4), `fields` (4), `serverrequest-test` (2). +- **Retired:** `viewmodelrequest`. The rule set names `ServerRequest`, not `ViewModelRequest` — the latter is a `ShowRequest` specialization, so its wire contract is `serverrequest`'s and the VM↔Request pairing is `viewmodel`'s (`viewmodel-request-pairing`). + +Violations no check covers still surface as `uncovered-{area}` findings rather than under invented names, so any remaining gap shows up in every report instead of hiding behind official-looking labels — an `uncovered-*` finding is now also a signal that the coverage ledger (`coverage-ledger.md`, beside the checks) may need a new entry. + ## Notes - Reports may flap slightly between runs on identical input due to subagent non-determinism. The exit code (`--fail-on` threshold) is the stable signal for CI. +- A check name is a contract. Suppression directives, `.fosmvvm-review.yml`, and CI gates all address checks by name, so names must come from the check files and nowhere else — see the `uncovered-{area}` rule in the subagent prompt. - Per-PR CI runs should use the default branch-diff scope. `--all` is reserved for daily/weekly sweeps and PRs to `main`/`master`. - The skill is report-only by design. Do not add auto-fix; review and remediation are separate concerns. diff --git a/.claude/skills/fosmvvm-review/checks/cross-cutting.md b/.claude/skills/fosmvvm-review/checks/cross-cutting.md index 2c64f984..9ff7bd9b 100644 --- a/.claude/skills/fosmvvm-review/checks/cross-cutting.md +++ b/.claude/skills/fosmvvm-review/checks/cross-cutting.md @@ -59,6 +59,32 @@ catch DataFetchError.badStatus(401) { ``` **Detection:** In non-test source, find `catch` clauses or `if`/`switch` branches keyed on a specific HTTP status (`DataFetchError.badStatus()`, `response.status == .`, raw `401`/`403`/`404` comparisons) where the branch drives business behavior (retry with new credential, navigation, user-facing state). Exempt: pure transport consequences (structured logging, cache invalidation, generic backoff without semantic branching) and test assertions of the server's transport contract. +## Check: server-calls-use-the-request-door +**Severity:** blocker for a hand-built call to the app's own server; warning for hand-rolled plumbing to external services +**What:** Client code never hand-builds an HTTP call to the app's own FOSMVVM server — every such communication is a typed `ServerRequest` through `processRequest` (**either overload**: `processRequest(mvvmEnv:)` in apps, `processRequest(baseURL:headers:session:)` in CLIs and jobs that have no `MVVMEnvironment` — both are the door). The type derives the path, the protocol the method; version headers and typed errors ride along. Calls to genuinely external services (third-party APIs) go through FOSFoundation's networking front door — `url.fetch()` / `url.send(data:)` / `url.delete(data:)` — never a hand-rolled `URLSession` dataTask plus `JSONDecoder`. Use the `errorType:` overloads when the service's error body is a type you own; a third-party service throwing arbitrary HTML/JSON is legitimately handled with the plain overload. +**Anti-pattern:** +```swift +// To the app's own server — the request door exists for exactly this +let url = URL(string: "http://localhost:8080/api/users/\(id)")! +let (data, _) = try await URLSession.shared.data(from: url) +let user = try JSONDecoder().decode(User.self, from: data) + +// To an external service — hand-rolled where url.fetch() exists +var request = URLRequest(url: weatherURL) +request.httpMethod = "GET" +let (data, _) = try await URLSession.shared.data(for: request) +``` +**Detection:** Key on the **transport, not on URL construction** — building or rewriting a URL (cache-busting, pagination) that is then handed to the front door is not a hit. Find raw transport use in client-role code: `URLSession` dataTask/data calls, hand-assembled `URLRequest`s, and app-owned socket connects (WebSocketKit/NIO dialers, `URLSessionWebSocketTask` outside the framework's own plumbing). Then resolve whose host each call targets — the tier depends on it: + +- **The app's own server** — the URL matches a `deploymentURLs` host, is derived from `MVVMEnvironment` configuration, mirrors a ServerRequest-derived route, **or comes from an injected base (a CLI flag, a channel endpoint, a config file) whose documented target is the app's server**. CLIs rarely have an `MVVMEnvironment`, so the injected-base prong is the one that decides them — read the base's documentation and derivation, not just the literal URL. **Blocker.** +- **Client role wins over target membership.** Code inside the server target that calls the server over HTTP — an installer's health poll, a self-check — is client code for this rule. (The architecture doc names health checks as `ShowRequest`'s own example, so "there is no request type for it" is the finding, not the excuse.) +- **The acknowledged-gap path.** Where the door genuinely cannot express the operation — raw byte streaming, ranged reads, an octet-stream object transfer, a socket channel — that is a framework gap. The disposition is the standard suppression directive whose justification **names the gap and the upstream report** (`// fosmvvm-review:disable:next server-calls-use-the-request-door — raw streaming; FOSUtilities issue #N`). A hand-built call with an in-file rationale but no suppression-plus-report is still the blocker — fire it once more with "report the gap upstream and suppress with the issue number" as the remedy, so the finding converges instead of re-firing forever. Never recommend widening the hand-built surface while the gap stands. +- **A genuinely external service.** Hand-rolled `URLSession`/`dataTask`/`JSONDecoder` plumbing is a **warning**: `url.fetch()`, `url.send(data:)`, `url.delete(data:)` (FOSFoundation, `URL+DataFetch.swift`) already apply the standard headers, status checking, and the library's **JSON** coding — the front door is JSON-shaped, so a raw-bytes external fetch has no front door and takes the acknowledged-gap path instead. `DataFetch.urlSessionConfiguration(forUserToken:)` covers bearer-token sessions. +- **CLI tools and background jobs are clients too** — no exception on either tier. +- **Leaf/JS templates: TBD** (ruled 2026-08-25). A template-side `fetch('/api/…')` is this rule's violation in JavaScript, and template review belongs to the view-area work — there is no detection here yet. When one is encountered anyway, report it at **warning** under this check's name and note the TBD standing. + +Not hits: FOSFoundation's own Networking internals; the framework's SSE/live-invalidation plumbing; **`URLSession.session(config:mutualTLS:)` and FOSNetworkSecurity's session factories** — pinned-TLS session construction is the framework front door, not hand-rolling; test doubles exercising the network-mocking support. Test helpers that hand-assemble method + path + query around `app.testing().test(...)` are the request-test area's business, not this check's — note them, do not grade them here. + ## Check: published-representation **Severity:** warning **What:** A sealed type's internal encoded shape (JSON keys, token format, byte/column layout) stated on a **public** surface — a DocC `///` comment, `CHANGELOG`, or `README`. Publishing the representation makes it a de-facto schema consumers parse or hand-forge, defeating the opacity and freezing an implementation detail. Public docs state the *contract* (opaque; `Codable` round-trips; stable within a major version), never the shape; pin the shape in an internal `//` comment + a forward-compat test. @@ -105,3 +131,123 @@ static func stub() -> Self { let name = user?.displayName ?? "Fred Flintstone" ``` **Detection:** Find `.stub()` call sites outside `#Preview` bodies, preview providers, and test targets. Grep scoped production sources, localization YAML values, and migrations for vocabulary markers — Flintstones names ("Flintstone", "Rubble", "Bedrock", "Slate") and 1914-era dates are strong signals anywhere; ±42 numbers are a signal only in defaults/fallbacks (42 alone is too common to flag bare). Flag each hit with which artifact it contaminates. + +## Check: stub-records-its-arguments +**Severity:** warning +**What:** A recording stub exposes both assertion points a caller needs: that the operation fired, and what data it fired with. +**Anti-pattern:** +```swift +public func run(_ command: Command, on storage: any Storing) async throws { + runCalled = true // `command` is dropped +} +``` +**Detection:** For each recording stub (a test double whose methods set `…Called` flags), find methods taking data-carrying parameters — anything beyond the output/storage target. Flag those recording only a `Bool` with no corresponding `…CalledWith`. The test that matters is usually "did the *right* verb fire", and a lone `Bool` makes three different calls indistinguishable. Methods whose only parameter is the write target are not hits. + +## Check: stubs-record-they-dont-do +**Severity:** warning +**What:** A stub Operations implementation records the call — it never performs the operation's work (viewmodel generator, ratified 2026-08-25). A UI test proves the button is *wired* to the operation, not that the operation does something; a stub that does work turns that wiring test into a timing-dependent behavior test, and can reach real infrastructure from a test. +**Anti-pattern:** A `StubOps` method whose body awaits a `processRequest`, sleeps (`Task.sleep`), spawns a `Task`, or touches network, files, or a database — instead of assigning its recorder and returning. +**Detection:** For each stub Operations conformer (resolve the trio by conformance and role, not name), read every method body. Flag real work: awaited calls beyond a trivial actor hop, `processRequest`, timers and sleeps, spawned tasks, network or storage reach. **Not hits:** recorder assignments; writing the `output` storage/binding the method is handed — that is recording's client-hosted twin, and `stub-mutates-what-it-is-handed` *requires* it; an `async throws` signature with no `await`, which the protocol forces (the same shape `ops-not-async-unless-needed` already exempts). A stub whose work reaches production-shaped infrastructure escalates to `tests-never-touch-production`'s blocker — cite that name, count the cause once. + +## Check: stub-mutates-what-it-is-handed +**Severity:** warning +**What:** A stub for a client-hosted ViewModel's Operations performs the same mutation the live implementation would, so the projection loop still runs under test. +**Anti-pattern:** A `*StubOps` method that ignores its storage parameter entirely and only sets a flag — the `@Observable` store never changes, so nothing re-projects and the View under test stays frozen. +**Detection:** Identify Operations stubs belonging to a ViewModel declared with `clientHostedFactory`. Flag methods that never touch the storage parameter they are handed. Server-hosted Operations stubs are exempt: their projection comes from a fetch, not from local mutation. Cite what a test can no longer assert — the frozen state is the cost, not the missing line. + +## Check: no-hand-rolled-framework-products +**Severity:** warning +**What:** The project does not re-implement what the framework or a generator already provides (functional-discipline: hand-rolling what a generator or the framework produces; repo CLAUDE.md's API-catalog rule — check the catalog before hand-writing a helper). Detections resolve against the plugin's api-catalog (`shared/api-catalog/`, the reach-for index) — **never against memory**: a capability is "provided" only if the catalog lists it. +**Anti-pattern:** A hand-configured `JSONEncoder()`/`JSONDecoder()` coding the app's own wire or persisted data where `toJSON()`/`fromJSON()`/`defaultEncoder` carry the wire-format date contract; a hardcoded `DateFormatter` pattern building display text where the `Localizable` date machinery exists; a hand-written async button with an error binding after the framework shipped `Button(activity:error:action:)`; a bespoke network mock where `URLSessionProtocol`/`session()` exists; a re-implemented semantic-version comparison, CSV parse, or grouping helper. +**Detection:** For each project-authored helper, utility type, or extension in the reviewed files, ask the catalog's reach-for question — does an entry already provide this? Two forms, one name: + +1. **The hand-rolled product.** A re-implementation of a catalog-listed capability. Grade by the **contract semantics the hand-roll loses**, and say so in the finding: wire-format dates on the app's own coding (drift the decoder will feel), locale correctness (a hardcoded format string is wrong in every locale but one), typed decode diagnostics, verified interaction semantics (re-entry refusal, cancellation) on async UI. A bare `JSONDecoder()` on a CLI tool's local config file is the shallow end — note the family once with its sites listed, not one finding per call. +2. **Duplicated dependency internals.** Framework internals reverse-engineered or copied downstream because the framework lacks something the project needs. The remedy is **always the upstream report, never the fork** — the finding names the gap and the report as the fix, and the acknowledged-gap suppression path applies (gap documented + upstream issue number, per `server-calls-use-the-request-door`'s precedent), so the finding converges instead of re-firing. + +**The version floor is mandatory, checked before writing any finding.** Resolve the consumer's pinned FOSUtilities version (`Package.resolved`) against the release the API shipped in — the catalog and CHANGELOG carry the floors. A pin **below** the floor means the hand-roll predates the product: that is NOT a violation — report it as an **adoption candidate** ("the framework provides this since 0.13.0; adopt on upgrade"), a different finding with a different tone. Blaming code for not using an API its pin cannot see is this check's characteristic false positive. + +**Not hits:** + +- **An external service's wire contract** legitimately demands its own coder configuration — DTO coding for a third-party API, with that service's date formats and casing, is not a hand-roll of the framework's coders. (The *transport* to such services is `server-calls-use-the-request-door`'s warning clause.) +- **Own-server calls** are `server-calls-use-the-request-door`'s blocker — network plumbing belongs to that check entirely; this one covers everything else in the catalog. +- **UI-test element helpers** keep their specialized name: `no-hand-rolled-element-helpers` (`ui-tests`). +- A helper the catalog does not list is not a finding under this name — it may be a candidate *for* the framework, which is an observation for the owner, not a violation. + +## Check: tests-never-touch-production +**Severity:** blocker +**What:** Tests never modify, delete, or corrupt production data — read-only by default (repo `CLAUDE.md`, firm governance principle). A test's isolation is **constructed, not inherited**: its server is in-process or localhost, its database is ephemeral and test-created, its stores are stubs. +**Anti-pattern:** A Fluent test binding a DSN read from the ambient environment (`DATABASE_URL`) and running migrations or writes; a test firing a write request at a real deployment URL; a cleanup sweep deleting by pattern (`test*`) against shared infrastructure; a live production store injected into a test host whose taps mutate it. +**Detection:** For each test file, resolve where its **execution edges** actually land — the server it calls, the database it binds, the stores it injects: + +- **Blocker — a mutation path that can reach non-test infrastructure.** Write requests aimed at a deployment URL (anything not in-process or localhost); a database binding inherited from the ambient environment, with or without a production-shaped default — "it's staging today" does not clear it, because the target is whatever the shell says; pattern-keyed cleanup deletes against a shared target; a live store in a test host (that shape's home is `testhost-mirrors-vm-settings`, `ui-tests` — cite it there, count it once). +- **A production URL literal is never itself the violation.** Trace it to an execution edge: a real hostname handed to a pure function (URL manipulation, parsing) is inert fixture data and not a hit. Grep-and-flag on hostnames is this check's characteristic false positive. +- **Live reads are not this blocker.** A test that live-reads an external service mutates nothing — the principle is read-only by default. Note the network dependency once, as flakiness, not under this name. +- **The conformant shapes:** the in-process typed test door (`serverrequest-test`'s harnesses), `app.databases.use(.sqlite(.memory), as: .sqlite)` or an equally ephemeral test-constructed binding, mocked sessions (`URLSessionProtocol`), and localhost `.debug` deployment URLs. + +Pairs with `deployment-urls-distinguish-environments` (`swiftui-app-setup`) — a debug/test build resolving to a production host — which stays that area's finding; this check owns the test-tree side of the same principle. + +## Check: behavioral-suite-standing +**Severity:** warning +**What:** The behavioral-test channel's **standing and isolation** — and nothing else. Behavioral suites project from requirements + ratified design in a context that never saw the implementation (execution-model's dedicated second channel; the `fosmvvm-behavioral-test-generator` skill). Review verifies that the suite exists and that its isolation held; **review NEVER judges a behavioral suite's assertions against the implementation** (ruled 2026-08-25) — a reviewer proposing to "fix" a behavioral assertion to match the code is committing exactly the contamination the channel exists to prevent. When a behavioral assertion and the code disagree, that is channel disagreement, classified upward (code defect / payload defect / ambiguous requirement) — never a review finding against the test. +**Anti-pattern:** A `*BehavioralTests.swift` suite with `@testable import` of the module under test; a behavioral suite importing an app or server target; a project whose truth layer carries requirements while no behavioral suite exists. +**Detection:** Behavioral suites are identified by the generator's conventions — `{Name}BehavioralTests.swift`, suites named `"{Name} — {REQ} behavioral"`, per-test `// REQ-nn:` traceability comments. Two clauses: + +1. **Standing.** When the project's repo carries a requirements register (specs/requirements documents in the truth layer) and no behavioral suite exists, note the standing gap once — the requirement-semantics channel is unverified. Grade it relative to what review can see: a repo with no visible requirements register gets no standing finding, because review cannot demand a projection of arguments it cannot see. +2. **Isolation, code-visible shadows only.** In each behavioral suite: `@testable import` of any module (the writer's payload carried public signatures only — internal access is a post-hoc reach the channel never had); imports of implementation modules (app targets, server targets, Factory-bearing modules) rather than the shared contract modules + testing frameworks; missing traceability markers or suite naming (form drift suggesting the suite was not projected through the channel — report as form, do not infer content judgments from it). Provenance and payload hygiene are process facts review cannot see; do not speculate about them. + +## Check: existentials-answer-the-question +**Severity:** warning +**What:** Existential types are a code smell, not a ban (repo `CLAUDE.md`, firm principle; scope ruled 2026-08-25): every existential in the flagged shapes must be able to answer the principle's own question — **was there any other way?** Passing `any XxxOperations` as a parameter is fine and out of scope. In scope: **stored** `any P` properties, **collections** of existentials (`[any P]`), and **existential returns** on the project's own API — the shapes where the erasure persists and compounds, costing dynamic dispatch, boxing, lost type identity, and `Codable` friction. +**Anti-pattern:** A stored `any P` where `P` has one production conformer and a generic (or the concrete type) would serve; `[any P]` over a closed, known set of conformers that an enum would model with exhaustive switching; a public API returning `any P` when callers immediately need the concrete type back. +**Detection:** For each in-scope existential, ask the question and read whether the code answers it. **The finding states the cost and names the alternative** — a generic parameter, a primary associated type, an enum over the closed conformer set, or the concrete type — not just "existential found." Answers that count, found in the wild: + +- **The injected-dependency seam.** A type storing several protocol-typed dependencies (`private let journal: any JournalStore`, five siblings beside it) where the generic alternative metastasizes N type parameters across every use site — that burden is exactly what the ruling exempts. Cold-path dispatch (called once per cycle) strengthens the answer. +- **Transitively, the seam's resources.** A seam protocol's method returning `any Handle`/`any Connection` inherits the seam's answer — the caller holds the seam erased, so its resources come back erased. +- **A third-party protocol's own idiom** (`(String) -> any LogHandler`, a service-lifecycle `any Service` collection) — the vendor's contract, not the project's choice. +- **A genuinely heterogeneous runtime mix** whose membership is open — the case erasure exists for. + +**Not double-reported:** a stored existential on a `@ViewModel` whose conformers include an `@Observable` class is `vm-holds-scalars-only`'s **blocker** (`viewmodel`) — this check takes only the value-typed remainder there, as the snapshot-doctrine note that check delegates. The framework's computed `operations: any XxxViewModelOperations` idiom and the platform's untyped-error convention (`Binding`, `any Error` in catch plumbing) are the framework's and platform's own answers — not hits. + +## Check: docc-serves-the-customer +**Severity:** warning +**What:** Documentation has three audiences, three homes (repo `CLAUDE.md` → Documentation & Comments; ruled into review 2026-08-25). DocC (`///`) serves the code's **customer**: lead with how they call it — nearly always an example — then why and when they care; state the contract, never implementation details or design rationale. Design rationale belongs in plan/design prose; internal `//` serves the maintainer and only for genuinely non-obvious constraints. Undocumented **and** example-free public API is debt reviewed projects inherit from the framework's own standard. +**Anti-pattern:** `/// An opaque token that wraps a namespace-derived hash…` (implementer's frame — what it *is* inside) instead of `/// Create one from a type — ModelNamespace(for: User.self)…` (customer's frame — how to call it); a `///` block explaining why the author chose this design; a `//` comment restating what the next line plainly does. +**Detection:** Three clauses, all judgment-graded; report each as an aggregate finding with representative `path:line` sites, never one finding per symbol: + +1. **Undocumented or example-free public API.** Grade by whether the symbol has *customers* — types consumed across module boundaries (ViewModels, Fields, Requests, shared utilities) carry the bar; a member that is `public` only because the target layout forces it is the shallow end. **Look above the attribute stack**: `@ViewModel`, property wrappers, and macros sit between the DocC and the declaration, so an adjacency-keyed detection reports the best-documented idiomatic types as undocumented — walk up past attributes before concluding a symbol has no `///`. +2. **Implementer's frame in DocC.** The test is the frame, not tell-words: does the sentence tell the *caller* how/when to use it, or tell a *maintainer* why it was built this way? A scope-of-contract note ("observed-only — the editor is a later arc") is customer information even when it sounds provisional; "we do X to avoid Y internally" is implementer rationale even when polished. The remedy is **relocate to the design/plan prose, not delete** — the content is valuable in its right home. +3. **Theatrical internal comments.** A `//` that proves a non-problem or restates the adjacent code is theatre — it reads as compensating for doubt and costs lifetime velocity. (A comment asserting a safety mechanism the code lacks is `comment-asserts-an-invariant-the-code-lacks`'s **blocker**; a sealed type's encoded shape stated in DocC/README is `published-representation`'s — cite those names, not this one.) + +## Check: suites-serialize-shared-state +**Severity:** warning +**What:** Swift Testing runs suites — and the tests inside a suite — in parallel by default. A suite whose tests touch **shared mutable state** carries `.serialized` (repo `CLAUDE.md` lesson, confirmed as doctrine 2026-08-25). The classic symptom is a test recording a value and asserting `0`, because another test cleared the shared state between the write and the read. +**Anti-pattern:** `@Suite struct FooTests { … Store.shared.clearState() … }` with no `.serialized` trait, in a package where another suite also drives `Store.shared`. +**Detection:** For each `@Suite` (and each implicit suite — a type holding `@Test`s), find touches of shared mutable state, then check the trait: + +- **Shared mutable state is more than `X.shared` singletons:** `static var`s, the process environment (`setenv`/`ProcessInfo` overrides), fixed-path filesystem fixtures, and process-global registries all count. A `.shared` accessor that is only *read* (a port off a running server, `URLSession.shared` as transport) is not a mutation — the mutation is the test's, not the accessor's. +- **`.serialized` protects within the suite only.** When the same state spans **multiple suites**, `.serialized` on each does not stop cross-suite interleaving. Two conformant remedies for that case, both found in the wild or the lessons: a **test-owned coordination gate** acquired around exactly the racing window (set → use → restore — finer-grained than the trait, and cross-suite safe), or **dependency injection** so the state stops being shared at all — the companion lesson names DI as the real fix and the singleton as the underlying liability. A finding on the cross-suite shape names one of these, not just the trait. + +## Check: comment-asserts-an-invariant-the-code-lacks +**Severity:** blocker +**What:** A comment does not claim a safety property the code does not implement. +**Anti-pattern:** +```swift +/// Recorded state is guarded by an `OSAllocatedUnfairLock`, so the +/// `@unchecked Sendable` conformance is HONEST. +public final class SomeStubOps: SomeOperations, @unchecked Sendable { + public private(set) var runCalled = false // no lock anywhere in the file +} +``` +**Detection:** Where a comment names a specific mechanism as the reason something is safe — a lock, a queue, an actor, a copy, a validation — verify the mechanism exists in the code it describes. Highest yield around `@unchecked Sendable`, whose whole contract is a human promise: check that the named guard is actually present and actually covers the mutable state. Flag the comment together with the state it fails to cover. This is worse than no comment: it tells the next reader, and the next reviewer, not to look. + +## Check: directives-spell-their-tool +**Severity:** warning +**What:** A tooling directive comment uses its tool's exact token, or it silences nothing while reading as governance (ruled 2026-08-25; the ledger entry is the statement of record). SwiftLint's token is `swiftlint:`, this skill's is `fosmvvm-review:` — a variant spelling (`swift lint:disable`, a stray space, a hyphenated guess) is inert prose the next reader trusts and every tool ignores: the same harm class as `suppression-without-justification`. +**Anti-pattern:** `// swift lint:disable classes_should_be_final` — wrong token, and in the observed case a rule id no tool defines either. +**Detection:** Find directive-shaped comments — `disable`/`enable`/`ignore` verbs addressed to a tool — whose token matches no tool the repo actually runs (its lint config, this skill's directives). Flag each with the correct spelling, or with removal when the underlying rule does not exist. A correctly-spelled directive is the *other* checks' business (justification, validity); this one fires only on the spelling. + +## Check: deferral-pointers-resolve +**Severity:** warning +**What:** A comment that defers work to a tracking document points at a document that exists in the repo (ruled 2026-08-25; the ledger entry is the statement of record). A dead pointer makes an untracked deferral look tracked — the reader trusts the ledger entry that was never written. +**Anti-pattern:** `// deferred follow-up (see docs/harbor-team/deferrals.md)` where no such file exists — beside a sibling comment correctly citing the real `docs/deferrals.md`. +**Detection:** Find comments that defer or reference work to an in-repo document — `see docs/…`, `→ .md`, deferral/plan/ledger citations — and verify each cited path exists. Flag dead pointers with the nearest real document when one is evident (a path-drifted spelling of an existing ledger is the common case). External URLs and issue-tracker references are out of scope; so is prose *about* documents that cites none. diff --git a/.claude/skills/fosmvvm-review/checks/datamodel.md b/.claude/skills/fosmvvm-review/checks/datamodel.md new file mode 100644 index 00000000..41dad2ef --- /dev/null +++ b/.claude/skills/fosmvvm-review/checks/datamodel.md @@ -0,0 +1,94 @@ +--- +area: datamodel +generator-skill: fosmvvm-fluent-datamodel-generator +where: + - "Sources/**/DataModels/**/*.swift" + - "Sources/**/Models/**/*.swift" + - "Sources/**/Migrations/**/*.swift" + - "Sources/**/*Migration*.swift" + - "Sources/**/database.swift" + - "Sources/**/databases.swift" +--- + +# DataModel Checks + +The positive pattern lives in the `fosmvvm-fluent-datamodel-generator` skill. The Model is the center of the architecture — the source of truth reads and writes flow through — and these checks are about identity leaking out of `@ID`, the form contract diverging from storage, and the schema drifting from the model that writes to it. + +## Reviewer Guidance + +- **Resolve types by conformance, not by name — or by prose.** A DataModel is a type conforming to `DataModel` (FOSMVVMVapor) or Fluent's `Model`, wherever it lives and whatever it is called. Trust the conformance over the DocC: a stale comment claiming "this model uses `Model` (not `DataModel`)" beside a `DataModel` conformance has been seen in the field. `ModelIdType` is a typealias for `UUID` — resolve the alias; the two spellings are one type. +- **Not every entity has Fields, and that is correct.** Session records, audit logs, and junction tables are system-only — no user form, no Fields protocol. Do NOT recommend inventing a Fields protocol for an entity no form edits. The discriminator when an entity is written by requests: does the request body carry *user-entered field values* (Fields required) or *operation parameters* (bare DataModel is correct)? A session-close request is operational; a rename request carrying a user-typed `name` is a form edit. +- **A junction table is the prescribed shape for many-to-many, not a workaround.** Do NOT recommend "simplifying" `@Siblings` + junction into a `[UUID]` array column — that is the exact defect `modelid-outside-id` exists to catch. +- **The contract-side twin lives in `fields`.** An identity requirement (`var id: ModelIdType? { get set }`) on a Fields *protocol* is `fields-carry-no-identity`'s finding, not this area's — do not re-report it here, but when you see it, say the pairing out loud: the protocol requirement is what forces adopters to carry the identity. +- **This area is macOS/Linux server code.** The pinned FOSUtilities version governs which protocols exist (`ContainerDataModel`, `SortableDataModel`, `FilterableDataModel`, `DataModelWriter` arrived across 0.4.0–0.10.0); check the pin before flagging their absence, per the dispatch prompt's version-floor rule. + +## Check: modelid-outside-id + +**Severity:** blocker +**What:** Identity appears in exactly one place on a DataModel: the `@ID(key: .id)` property. Every other identity-bearing stored field — `ModelIdType`/`UUID` (one type; resolve the alias), optionals and collections of them, and identities smuggled in other clothes (see Detection) — requires express approval and documentation at the declaration site (the repo's firm `ModelIdType Requires Junction Tables Except for @ID` principle). Relationships ride the typed wrappers — `@Parent`, `@OptionalParent`, `@Children`, `@Siblings` with a junction table — which give Fluent the join and the database the foreign-key constraint. A raw identity column has no wall: any value can be written into it, the database enforces nothing, and the reference silently dangles when the target row goes. +**Anti-pattern:** +```swift +final class ToolCallRecord: DataModel, @unchecked Sendable { + @ID(key: .id) var id: UUID? + // NOTE: relationship disabled during migration; restore later. + @OptionalField(key: "session_id") var sessionId: UUID? // same-database FK as raw UUID + @Field(key: "tag_ids") var tagIds: [UUID] // many-to-many flattened into an array +} +``` +**Detection:** For each DataModel (by conformance), list stored properties whose type is `ModelIdType`/`UUID` or an optional/collection of either. Exempt the `@ID(key: .id)` property — under either spelling. Then widen past the obvious type list, because the smuggled forms carry the same risk and escape a type-keyed scan: + +- A `Codable` struct stored as a `.json` column whose members include identities referencing tables in this database. +- A `String`-typed field or dictionary key that holds an identity (a UUID-string key set, a `conversationId: String`). + +For every hit, decide which side of the principle it is on: + +- **The express-documentation bar.** The principle allows approved, documented exceptions. Documentation that clears the bar names its authority — a decision, an issue, an approver — or states a condition under which the exception ends. A comment that merely *describes* the reference ("the session this belongs to") is not approval, and **a documented deferral whose stated milestone has passed is drift, not an exception** — a "will be restored in R2.2" note outliving R2.2's ship date is precisely the finding. +- **Legitimate shapes** are references *outside* this database: an external system's id, an opaque token minted elsewhere. Those still deserve the documentation, but the finding, if any, is the missing note — say so at warning tone inside the report text. +- **The array form** (`[UUID]`, an id-keyed dictionary) is a flattened relation. The junction table with `@Siblings` (or a child table) is the remedy; the only path past it is the same express-approval bar, and a design note claiming the exception must name the integrity cost it accepts — element-level foreign keys do not exist on array columns, so nothing constrains the members. An array with a documented design decision but no named integrity tradeoff has not cleared the bar. + +**Say what the raw column costs, from the migration — and name the right remedy site.** No `.references(...)` on the column means no integrity at all; with `.references(...)` the database constrains it, but relation loading is still unavailable because loading comes from the *wrapper* (`@Parent`/`@OptionalParent`), not from the constraint. And when the original migration runs before the referenced table exists in the migration order, the constraint cannot be added there — the remedy is a follow-up migration, so point the finding at that, not at the original file. + +## Check: datamodel-adopts-its-fields + +**Severity:** blocker +**What:** A form-backed entity's DataModel adopts its Fields protocol. The Model implements Fields and contains more — system fields, timestamps, relationships — but the user-editable subset comes from the one shared contract, so storage validates with exactly the rules the form and the RequestBody validate with. A model that restates the properties without the conformance has forked the contract; an entity edited by users with no Fields protocol anywhere has no contract at all — every `validate` on the path returns nil, and nothing anywhere bounds what a user can store. +**Anti-pattern:** A model whose DocC says its `name` is user-edited, written by an `UpdateRequest` whose RequestBody carries the user-typed string — and no `{Entity}Fields` protocol exists; both the RequestBody's and the model's `validate` return `nil`. +**Detection:** Two directions, and they need each other: + +1. **From Fields:** for each Fields protocol in the shared module, find the DataModel persisting that entity (match by the entity, not the name — the model whose schema the entity's factory and write requests use). Flag a model that stores the protocol's properties without declaring the conformance. +2. **From the model:** a DataModel with no Fields protocol is a hit only when some request body elsewhere carries *user-entered field values* for it — apply the guidance discriminator: user-entered values mean a form contract is owed; operation parameters (close, advance, retry) do not. With no user-editing surface anywhere, the entity is system-only and correct as a bare DataModel. + +When the conformance exists, spot-check that it is real: the model's stored properties satisfy the protocol requirements directly (Fluent's wrappers witness them — `@Field var content: String` satisfies `var content: String`), not through shadow computed properties copying values around. + +**Scale the remedy to the surface.** One user-editable field means a one-field Fields protocol — small contract, same principle. State the minimal shape in the finding so a single inline-rename entity is not read as demanding a full form apparatus. + +## Check: schema-matches-the-model + +**Severity:** blocker +**What:** The model and the *net* result of its migration sequence agree, and every migration is registered. Every wrapper key on the model has a column in the net schema and vice versa; a drifted key string compiles clean and fails at runtime with an opaque SQL error, and an unregistered migration means the table never exists at all. +**Anti-pattern:** `@Field(key: "user_name")` beside a migration creating `"username"`; a `{Model}+Schema.swift` never added in `database.swift`; a column created by the initial schema whose wrapper was removed, with no migration dropping it. +**Detection:** For each DataModel, collect the key strings from its property wrappers (`@ID`, `@Field`, `@OptionalField`, `@Parent(key:)`, `@OptionalParent`, `@Timestamp`, `@Enum`, `@Siblings` through its junction model). Compute the schema as the **net of the full registered migration sequence** — creates, alters, and drops, applied in registration order — not any single file. Two subtleties the field has already produced: + +- **Dialect-forked migrations fork the net schema.** A migration that guards on the SQL dialect (`postgresql`-only drops, say) leaves a different net schema on the test dialect than in production. When a column is dropped on one dialect and survives on another, that divergence is itself the finding — name both nets. +- **Data-preservation columns are exemptible at the same bar as `modelid-outside-id`** (ratified 2026-08-25). A Fluent-created column no wrapper reads, documented at its migration site as deliberately retained for existing data, is exempt *while its stated plan is live*; a retention note whose restore-or-remove milestone has passed is the drift finding, not an exemption. Raw-SQL (SQLKit) database-only columns — search vectors and the like — are deliberately invisible to the model and are not mismatches; the generator's own pattern says so. + +Then confirm each migration type is registered (`app.migrations.add(...)`, conventionally `database.swift`); flag one that never is. **Conditional registration is acceptable when it is deliberate** — seeds gated on non-release environments are the generator's own pattern; registration gated on something that looks accidental is the finding. + +## Check: migration-honors-the-fields-contract + +**Severity:** warning +**What:** Where the schema and the Fields contract describe the same property, they agree on what must exist: a field the Fields protocol requires (`.required(value: true)` on its FormField, non-optional in the protocol) is `.required` in its column, and an optional protocol property's column does not demand what the contract lets a user omit. This is agreement, not duplication — the fields area's `validation-not-duplicated-downstream` already establishes that `.required` on a column is storage integrity (correct here), while restating a *length or range* in the schema is the duplication to flag there. +**Anti-pattern:** A Fields protocol with `var value: String { get set }` (non-optional) and a `.required(value: true)` FormField — while the migration declares the `value` column without `.required`. A row can now exist that no form could have produced, and its decode into the non-optional `@Field` fails at read time. +**Detection:** For each DataModel adopting a Fields protocol, walk the protocol's non-optional properties and required FormFields against the net schema's columns: flag a required field whose column lacks `.required`, and an optional protocol property whose column is `.required` with no default (the write path fails on legitimate nil). For enum-typed Fields properties, confirm the column stores what the enum writes (raw-value type agreement). The Fields protocol is authoritative for user-editable semantics, the migration for storage shape. + +## Check: stored-enum-decodes-honestly + +**Severity:** blocker when the fallback is a meaning-bearing case; warning when it is an explicit unknown/none case +**What:** A raw value stored in a column and decoded with a coalescing fallback — `SomeEnum(rawValue: stored) ?? .someCase` — silently rewrites every historical row whose value the current enum no longer names. No decode error, no log line: the row simply *means something else now*. This is how a renamed or removed case strands data invisibly, and it is statically visible at the decode site even though the stranded rows are not. +**Anti-pattern:** +```swift +var signalType: GovernanceSignalType { + GovernanceSignalType(rawValue: signalTypeRaw) ?? .actionBias // history coerced to a real category +} +``` +**Detection:** For each DataModel storing an enum as a raw value — through `@Enum`, or a `String`/`Int` field paired with a computed decode — find the decode path and its failure posture. Flag `?? .meaningBearingCase` at blocker: unknown historical values become a live business category. An explicit `?? .unknown` (a case that exists to mean "not recognized") is the tolerant-reader pattern done honestly — warning at most, and only when nothing downstream treats `.unknown` as a real category. A throwing or optional decode that surfaces the mismatch is correct and is not a hit. Applies to every DataModel, Fields-backed or bare — the field evidence for this check came from bare models. diff --git a/.claude/skills/fosmvvm-review/checks/fields.md b/.claude/skills/fosmvvm-review/checks/fields.md index 8b6684dd..23076523 100644 --- a/.claude/skills/fosmvvm-review/checks/fields.md +++ b/.claude/skills/fosmvvm-review/checks/fields.md @@ -4,8 +4,78 @@ generator-skill: fosmvvm-fields-generator where: - "Sources/**/Fields/**/*.swift" - "Sources/**/*Fields.swift" + - "Sources/**/*FieldsMessages.swift" --- # Fields Checks -The positive pattern lives in the `fosmvvm-fields-generator` skill. No review-only checks defined yet. +The positive pattern lives in the `fosmvvm-fields-generator` skill. A Fields protocol is the **form contract**, defined once and projected into a RequestBody, a Form ViewModel, and a Model. These checks are about the contract leaking, drifting from its messages, or being defined in a way a conformer cannot actually override. + +## Reviewer Guidance + +- **A Fields protocol defines the user-editable form contract only** — validation rules, localized messages, input handling. It carries no identity. Do NOT recommend adding one "for convenience"; that is the project's `[Architecture] Fields Protocols Define Form Contracts Only` principle, and the reason is that Fields is projected into three artifacts that must not each acquire an identity of their own. +- **Do NOT recommend moving a member out of the protocol into an extension "to simplify."** A member defined only in an extension is statically dispatched, so a conformer's override merely shadows it — calls through the protocol still hit the default. That is a silent OCP failure and the opposite of a simplification. +- Validation lives on the Fields protocol, not in the View and not in the controller. A rule enforced in two places will disagree; a rule enforced only downstream is not part of the contract at all. + +## Check: fields-carry-no-identity +**Severity:** blocker +**What:** A Fields protocol declares no `ModelIdType`, `UUID`, or other identity field. +**Anti-pattern:** `var documentId: ModelIdType { get set }` on a `DocumentFields` protocol. +**Detection:** Flag requirements **typed** as identity — `ModelIdType`, `UUID`, or a typed model identifier. Do *not* flag on the name alone: a `String` holding a polymorphic reference is not identity, however it is spelled, and a name-shaped heuristic reports it while missing an identity typed under an alias. Identity belongs to the Model's `@ID()`, not to the form contract — and because Fields projects into a RequestBody, a Form ViewModel, and a Model, an identity here becomes three identities that can disagree. Per the repo's principles, a `ModelIdType` outside `@ID()` requires express approval and documentation; absent that, it is a finding. + +## Check: overridable-members-are-requirements +**Severity:** blocker +**What:** A Fields member intended to be overridable is a protocol *requirement* with a default in an extension — never extension-only. +**Anti-pattern:** +```swift +public protocol DocumentFields: ValidatableModel { + var content: String { get set } +} +extension DocumentFields { + var validationPolicy: Policy { .strict } // no requirement — a conformer can only shadow it +} +``` +**Detection:** For each Fields protocol, compare its declared requirements against members defined in its extensions — then apply the exemptions below *first*, because they cover most of what an extension legitimately holds. + +**Not hits — this is the prescribed shape.** The generator puts these in an extension by design, and flagging them would fail everything it emits: + +- `static var …Range` constants +- `static var …Field: FormField<…>` definitions +- per-field `internal func validate{Field}(_:)` helpers +- `{name}FieldsValidateModel(validations:fields:)` — the protocol-prefixed composition helper. Its prefix is the point: a type adopting two Fields protocols writes one `validate` calling `documentFieldsValidateModel` *and* `otherFieldsValidateModel`. It is a composition seam, not an override point (ratified 2026-08-25; the generator states it). + +**Hits** are members carrying policy a conformer would plausibly want to change and cannot: a validation strategy, a message source, an on/off switch, a default that is not one of the shapes above. Swift dispatches extension-only members statically, so the conformer's "override" applies only where the concrete type is known — every call through the protocol, or through a generic `some SomeFields`, still gets the default. + +This fails silently and in the confusing direction: the override works in a unit test that names the concrete type, and does nothing in the code that goes through the protocol. Say which member, and which call sites keep getting the default. + +Note that `ValidatableModel.validate(fields:validations:)` is a real protocol requirement, so a `validate` in a Fields extension is a *default for a requirement* — dynamically dispatched and correctly overridable. Confirm that upstream before grading it either way. + +## Check: every-field-has-its-messages +**Severity:** warning +**What:** Every `FormField` has the localized messages it references, and every message is reachable from a field. +**Anti-pattern:** A `FormField` whose `title:` names `messageKey: "title"` while the YAML defines only `placeholder` — or a `…RequiredMessage` on the Messages struct that no validation method ever returns. +**Detection:** Three artifacts must agree, and they drift independently: + +- the `FormField` definitions and the `messageKey`s they reference, +- the `@FieldValidationModel` Messages struct's properties, +- the YAML under `{Name}FieldsMessages:`. + +Walk all three and flag both directions: a referenced key with no YAML entry, and a Messages property no validation method returns. The first renders an empty string in the form; the second is usually a validation rule that was removed with its message left behind. + +Two further shapes worth naming when you see them, because they are the same drift wearing different clothes: a YAML file for a type that no longer exists, and two YAML files defining the *same* key with different content — where which one wins depends on load order. + +**Trace reachability, not just presence, for the second direction.** A message returned by a validator that its only caller can never reach — because the caller tests the same condition first and returns something else — is as invisible as one nothing returns. Follow the call path; the syntactic test alone misses it. + +**Watch for a fourth artifact competing with the three.** If a Form ViewModel declares its own label and placeholder `@LocalizedString`s, the `FormField` titles are dead — nothing on the rendering path asks for them, which is *why* gaps in the Fields YAML can sit unnoticed indefinitely. Report the competing source, not merely the gap; otherwise the fix looks like "add the missing keys" when it is "delete one of the two sources." + +## Check: validation-not-duplicated-downstream +**Severity:** blocker +**What:** A rule declared in Fields is enforced there, not restated in a View, a controller, or a Model. +**Anti-pattern:** `DocumentFields.validateContent` requiring 1–10,000 characters, and a `DocumentView` separately disabling its save button on `content.count > 10_000`. Equally: a template hand-typing `maxlength="200"` where `FormInputOption.rangeLength` already ships the bound; a RequestBody declining to adopt the protocol and copying its static members into a private helper enum; and — the one that drifts first — a Form ViewModel re-declaring the field's label and placeholder as its own `@LocalizedString`s. + +**Messages are part of the contract, not decoration.** Fields carries data, presentation, constraints, *and* messages. A title or placeholder re-declared outside the Fields messages struct is the same duplication as a re-declared range, and it goes wrong sooner: nobody notices two YAML files disagreeing until a user reads both spellings. +**Detection:** For each validation rule on a Fields protocol, search the downstream projections — the Form ViewModel's View, the controller handling the RequestBody, the Model's migration — for the same constraint expressed again. Flag the duplicate, naming both sites. + +**Two shapes are not hits.** A Fluent migration's `.required` column is a storage-integrity constraint that exists whether or not a form does, and carries no length — reporting it pushes toward nullable columns, which is worse. And a bare HTML `required` attribute with no `minlength`/`maxlength` is close to native form semantics and is *the correct case*: it is the absence of the range restatement. Flag the hand-typed bounds, not the requiredness. + +Fields exists so one definition projects into three artifacts. A rule restated downstream is a second definition that will drift from the first, and the drift is invisible until the two disagree about a specific value. Note which one is authoritative in the finding: the Fields declaration is, and the downstream copy is what gets deleted. diff --git a/.claude/skills/fosmvvm-review/checks/serverrequest-test.md b/.claude/skills/fosmvvm-review/checks/serverrequest-test.md new file mode 100644 index 00000000..f546a7ce --- /dev/null +++ b/.claude/skills/fosmvvm-review/checks/serverrequest-test.md @@ -0,0 +1,62 @@ +--- +area: serverrequest-test +generator-skill: fosmvvm-serverrequest-test-generator +where: + - "Tests/**/*Request*Tests.swift" + - "Tests/**/Requests/**/*.swift" + - "Tests/**/*Controller*Tests.swift" + - "Tests/**/*TestSupport*.swift" + - "Tests/**/*E2E*.swift" + - "Tests/**/*ServerTests.swift" +--- + +# ServerRequest Test Checks + +The positive pattern lives in the `fosmvvm-serverrequest-test-generator` skill. A request test's job is to prove the wire contract — that the request the client would send reaches the route the server serves, and that what comes back decodes typed. These checks are about tests that hand-assemble the wire and stop proving it. + +## Reviewer Guidance + +- **Find request tests by content, not by filename.** The wire-driving code routinely lives in shared `TestSupport` helpers rather than in `*Tests.swift` files — grep the test tree for `.testing().test(` and `processRequest` before concluding anything about coverage. +- **The typed door and what it derives.** `app.testing().test(request, locale:) { response in }` (FOSTestingVapor) takes the *request instance*: the path comes from `R.path`, the query rides as `toJSON()`, the version/locale/content headers are added, and the response decodes into `TestingServerRequestResponse` with typed `body` and `error`. Client-side E2E tests reach the same guarantee through `processRequest`. Every piece a test hand-assembles instead is a place the test can agree with the server while both disagree with the client. +- **Deliberately-malformed requests legitimately use the raw door.** A test asserting the server rejects a bad body *must* be able to send wrongness the typed door cannot express. Do NOT flag raw-door use whose purpose is sending a deliberately invalid request — the tell is an assertion on the rejection. +- **Raw Vapor routes are tested raw.** A bare route with no ServerRequest (a health endpoint, an ingest hook) has no typed door to use; testing it raw is correct — though the route itself may be another check's finding. +- **Know this area's outer boundary, and say it in reports.** These checks count coverage per request *type*; they cannot verify that a specific production call site is ever exercised. When you notice a production client path nothing drives (a `gateway.stop` no test calls), report it as the coverage note it is — never imply the suite's greenness says anything about that path. Closing that gap is E2E testing's job, not review's. +- **The async-boot trap in hand-rolled harnesses.** Vapor's `app.test()` runs only the synchronous boot path, so async lifecycle handlers (middleware registered via async startup) silently never run under a hand-rolled harness. FOSTestingVapor's shipped harnesses (`withFluentTestApp`, `withServedFluentTestApp`) handle boot correctly — at pins that have them. A hand-rolled `withTestApp` at an older pin is *correct at time of writing, now fixable*; name the version that lifts it, per the dispatch prompt's version-floor rule. + +## Check: request-test-uses-the-typed-door + +**Severity:** blocker +**What:** Request tests drive the wire through the typed door — `app.testing().test(request, locale:)` server-side, `processRequest` for E2E — never a hand-assembled method + path + query against the raw `test(.POST, "…", …)`. This is `controller-derives-its-own-route`'s test-side twin, and it fails the same way: a hand-built path that matches a hand-built route goes green while the real client fetches something else, and the 404 ships. +**Anti-pattern:** +```swift +// Type-derived components do NOT make the door — method, headers, and +// encoding are still hand-assembled, and the suffix is glued on: +try await app.testing().test(.POST, "\(StopRunController.baseURL)/destroy?\(encoded)") { … } +``` +**Detection:** In the test tree, find every drive of a ServerRequest-served route. Sort by **who derived the wire pieces**, not by how the string looks — and sort *drives*, naming a dual-purpose helper once: + +- **The typed door** — the overload taking the request *instance* (or `processRequest`): correct, not a hit. +- **Hand-assembled path, query, or verb suffix** — a glued `"/destroy?…"`, a hand-encoded query string, a string-interpolated route: **blocker**. But before framing the finding, check the server side: **when the glue faithfully mirrors a bespoke controller mount, the test is the honest witness, not the offender** — the primary finding is `controller-derives-its-own-route` (production side), the production client is the party likely broken against that mount, and converting this test to the typed door goes red until the server is remounted. Say all of that: the remedy spans both trees, and a test-only fix is not executable. +- **Framework-computed from the instance** (`request.requestURL()` and kin) — the framework derived it, a hand-rolled sliver of what the typed door already does: **warning**, not blocker; the pieces cannot drift from the framework, only from the door's header/decode behavior. +- **The raw door for something the typed door cannot express** — a deliberately malformed body, a request with a *required header omitted* (the typed door force-adds version/locale/content headers with no removal seam): legitimate, not a hit. This carve-out is scoped to inexpressibility, **not** to "asserts a rejection": a typed `ResponseError` rejection is asserted *better* through the typed door's `error` field, and a credential rejection through `credentialRejection` (0.7.0+) — a raw-door drive asserting `status == .unauthorized` is status-sniffing the field exists to eliminate, and is a **warning** (correct-at-time-of-writing below 0.7.0). +- **The raw door for an ordinary path the typed overload could express**: **warning**, with the typed door as the one-line remedy. Expect this to be voluminous with one mechanical cause — report it as one migration finding listing the sites, not as thirty findings. + +Check the pin before grading: the typed door dates to 0.1.0 (almost nothing earns a pre-floor excuse on its existence), `credentialRejection` to 0.7.0, the shipped harnesses to 0.5.0/0.6.0. + +## Check: request-test-covers-the-contract + +**Severity:** warning +**What:** Each ServerRequest has a test exercising its contract through the typed door: the success path decodes into `ResponseBody`, and where the request declares a `ResponseError`, at least one test provokes it and catches it *typed*. An error vocabulary no test ever decodes is decoration — the same area-wide inversion `controller-throws-the-declared-error` catches in production code, seen from the test side. +**Anti-pattern:** A request declaring a three-case `ResponseError` whose entire test coverage is one happy-path fetch — the error cases compile, ship, and have never once crossed a wire. +**Detection:** Enumerate `ServerRequest` conformers yourself — across every module, not from memory or a prior list — and map the test tree's drives against them. Three dispositions, not two: + +- **Absent** — a request no test drives at all, through any door. The clean hit; reserve the word for true zeros. +- **Present through the wrong door** — driven only raw. That finding belongs to `request-test-uses-the-typed-door`; here it is a *note*, never a second warning — substance-rich raw-door tests (decoded bodies, effect assertions) graded "absent" read as noise to the suite's author and double-count one mechanical cause. +- **Present** — typed-door or `processRequest` drives exist. + +Then the error leg, with its escape hatch: + +- A declared `ResponseError` (excluding `EmptyError`) that no test provokes and catches typed is the default finding. **Except decode-guard declarations:** an error declared solely so a permissive decode cannot swallow a middleware rejection is un-provokable by design — the server never throws it. For those, a *decode-contract* test (asserting the type refuses to decode from `{}` or a bare string) is the equivalent coverage. And note the pairing: at pins ≥ 0.7.0 the guard rationale itself is `no-defensive-error-for-credential-rejection`'s finding — `WireError` decodes the rejection first, so the declaration buys nothing; route the declaration question there and grade only the coverage here. +- **The invalid-body clause, for any validating write body** — Fields-adopting *or* plain `ValidatableModel`: at least one test sends an invalid body through the wire and asserts the typed rejection comes back. That one test proves the contract runs on the server, not merely compiles into it. A status-only assert (`== .badRequest`) half-proves it; say what the typed assert would add. + +Writes deserve an **effect assertion**, not only a status. For container CRUD that naturally means the response carries the container's children and the entity appears in them; for command-style writes (a stop, a mint, a replace returning a token or outcome), asserting server-side state in-process is stronger still — the requirement is the effect, not the shape. diff --git a/.claude/skills/fosmvvm-review/checks/serverrequest.md b/.claude/skills/fosmvvm-review/checks/serverrequest.md index 62a1245e..c5892b2f 100644 --- a/.claude/skills/fosmvvm-review/checks/serverrequest.md +++ b/.claude/skills/fosmvvm-review/checks/serverrequest.md @@ -4,6 +4,13 @@ generator-skill: fosmvvm-serverrequest-generator where: - "Sources/**/ServerRequests/**/*.swift" - "Sources/**/*Request.swift" + - "Sources/**/routes.swift" + - "Sources/**/*Controller.swift" + - "Sources/**/*+Factory.swift" + - "Sources/**/Factories/**/*.swift" + - "Sources/**/*+Live.swift" + - "Sources/**/LiveInvalidation/**/*.swift" + - "Sources/**/configure.swift" --- # ServerRequest Checks @@ -12,6 +19,10 @@ The positive pattern lives in the `fosmvvm-serverrequest-generator` skill. ## Reviewer Guidance +- **Anchor an area-wide finding at the instance where the loss is largest**, and list the rest in the body. Some defects here are one belief replicated across a dozen files; reporting them per file buries the diagnosis, and a finding still needs one `path:line` a reader can open. +- **The transport is not the contract.** A `ResponseError` declared on the request and an `Abort(.badRequest, reason:)` thrown in the controller are two different vocabularies; when they disagree, the declared type is decoration and the status is the real API. Check both ends before concluding an area is clean. +- **`CredentialRejectedError` is already handled and is not the request's business.** `WireError` decodes the FOS-owned surface errors strictly before the request's own `ResponseError`, so a rejection always reaches the client typed, whatever the `ResponseError` is. Do NOT recommend defensive shapes to "avoid swallowing a 401" — and treat a comment claiming that risk as a finding, not as a rationale. +- Do NOT recommend collapsing a typed error to a `String` to reduce boilerplate. The vocabulary *is* the value; a free-text field is an error a client cannot branch on. - A `ResponseError` is the operation's *semantic* error — the well-defined Swift error the operation would `throw` if it were a local function call. `ServerRequestError` exists so that throw can happen across the wire (server throws → rides the response as `Codable` → the client's `processRequest` rethrows the same typed error). It is **not** an HTTP-status mapping; HTTP statuses are transport dressing and carry no result semantics. See [Architecture Patterns → Typed Errors Are the Operation's Throw](../../shared/architecture-patterns.md). ## Check: responseerror-models-the-throw @@ -29,3 +40,139 @@ enum ErrorCode: String, Codable, Sendable { } ``` **Detection:** For each type conforming to `ServerRequestError` (excluding `EmptyError` and `ValidationError`): flag if (a) its only stored data is one or more free-text `String` fields (no `ErrorCode`-style enum, no typed associated data); or (b) its enum cases are named for HTTP statuses/transport categories (`unauthorized`, `forbidden`, `badRequest`, `notFound` with no operation noun, numeric-status suffixes) rather than operation outcomes (`duplicateContent`, `quotaExceeded`, `sessionExpired`). + + +## Check: controller-throws-the-declared-error +**Severity:** blocker +**What:** The controller throws the `ResponseError` its request declares, rather than lowering the failure into an HTTP status and prose. +**Anti-pattern:** +```swift +} catch let error as DAGValidationError { + throw Abort(.badRequest, reason: error.reason) // typed error → status + String +} +``` +**Detection:** For each request declaring a `ResponseError`, find its controller or factory and read the failure paths. Flag a handler that throws `Abort(_:reason:)` for an outcome the declared error covers, or should. + +**A reason-only `ResponseError` that is never thrown is this check's maximal instance, not an exemption from it.** Read literally, "where the declared error has a case for that outcome" lets the worst shape escape — a `{reason: String}` type has no cases at all, so nothing ever matches. That is the defect, not a reason to pass: the type was declared, the operation has real outcomes, and none of them survive the wire. + +**Infrastructure failures are correctly `Abort`s and are not hits.** `Abort(.internalServerError, reason: "SomeService unavailable")` means *this server is broken*, which is not an outcome of the operation and has no place in its vocabulary. Carve those out explicitly when you report, so the finding is not diluted — and do not let the carve-out stretch to cover a genuine operation outcome that merely arrives as a 500 — the distinction the type exists to carry (wrong role, no live connection, malformed input) then survives only as prose in a status body, and no client can branch on it. + +This is the check that catches an area-wide inversion: a codebase can declare typed errors on every request and throw none of them, in which case the declared types are decoration and HTTP is the actual contract. Say so once, at the level it is true, rather than filing the same finding per request. + +## Check: no-defensive-error-for-credential-rejection +**Severity:** warning +**What:** A `ResponseError` is not shaped, and its permissive fields are not justified, by a fear of swallowing credential rejections. +**Anti-pattern:** +```swift +/// A REQUIRED field by design: an error type whose decode accepts anything +/// (e.g. `EmptyError`) would swallow a credential-middleware 401 on the +/// FOSMVVM client, hiding the re-pull trigger. +public struct ResponseError: ServerRequestError { + public let reason: String +} +``` +**Detection:** Grep the `ResponseError` declarations and their documentation for reasoning about 401s, credential rejection, or `EmptyError` swallowing errors. Flag it: `WireError` decodes `CredentialRejectedError` strictly before the request's own error type, so the rejection is never reachable by the `ResponseError` and the defensive shape buys nothing. It also costs something — a permissive error decodes any abort body, so unrelated failures arrive wearing this operation's type. + +**Report this once for the whole area when the rationale has propagated**, listing every site in the body. A copied justification is one belief, not N defects, and filing it per request buries the fact that it spread. + +**Widen the grep past `ResponseError`.** The reasoning migrates: look for any member — including `ResponseBody` fields — whose documentation cites `EmptyError` swallowing, 401s, or credential rejection as a design reason. Once the belief is in a codebase it justifies shapes well outside the error type. + +**Say what to do instead.** Where the operation has no well-defined throw, the fix is `typealias ResponseError = EmptyError`, not an invented enum. Point at an in-repo example if one exists — most codebases with this problem have at least one request that got it right. + +Report the comment and the field together, and check whether the same block has been copied across requests: this is a rationale that propagates, and finding it once usually means finding it everywhere. Where the shape is otherwise a bare `reason: String`, `responseerror-models-the-throw` is the primary finding and this one explains why it was written that way — report both, but say which is the defect and which is the cause. + +## Check: requestbody-adopts-its-fields +**Severity:** blocker +**What:** A write request's RequestBody carrying user-entered field values adopts the entity's Fields protocol — the one contract the form, the body, and the model all validate with. The compiler forces `ValidatableModel` onto `CreateRequest`/`UpdateRequest` bodies, but it cannot force the conformance to *mean* anything: a hand-written `validate` returning `nil` satisfies the constraint and validates nothing, so invalid data rides the wire behind a green build. +**Anti-pattern:** +```swift +public struct RequestBody: ServerRequestBody, ValidatableModel { + public let name: String // user-typed value + public func validate(fields: [any FormFieldBase]?, validations: Validations) -> ValidationResult.Status? { + nil // constraint satisfied, nothing validated + } +} +``` +**Detection:** Enumerate the **whole write family**: `CreateRequest`, `UpdateRequest`, **and `ReplaceRequest`** — a peer protocol with the same `RequestBody: ValidatableModel` constraint that refines neither of the others, and the most form-like write shape there is (a PUT-upsert); a conformance scan keyed on Create/Update alone misses exactly the bodies most likely to carry user values. Add any plain `ServerRequest` whose declared `action` override is a write (`.create`, `.update`, `.replace`) — the conformance-free spelling escapes a protocol scan, and a deliberate control-channel command (the legitimate use, usually saying so in its DocC) is distinguished by the discriminator, not by skipping the file. A `RequestBody = EmptyBody` short-circuits to not-a-hit — the framework conforms `EmptyBody` to `ValidatableModel` for precisely the body-less write. Then, per body: + +- **Apply the discriminator first** (ruled 2026-08-25, shared with `datamodel-adopts-its-fields`): does the body carry *user-entered field values* — things a person typed into the requesting client's UI — or *operation parameters* — ids being acted on, verbs, flags, machine-minted or machine-assembled payloads? Operation-parameter bodies owe no Fields protocol and are not hits; `DeleteRequest`/`DestroyRequest` land there by construction. Free text is not automatically user text: a subprocess log tail in a `String?` is machine-produced; content authored upstream in a config file and submitted by an agent is not a form entry. An admin's CLI argument naming a catalog key is an id acted on, not a typed field value. +- **A user-values body adopting its Fields protocol:** confirm the wiring is real — its `validate` reaches the Fields validation helpers (the `{name}FieldsValidateModel(validations:fields:)` composition, or the per-field validators), not a parallel hand-rolled rule set. Adoption whose `validate` ignores the helpers has dropped the contract while wearing it — the same hit. +- **A user-values body with no Fields protocol anywhere:** the contract is missing wholesale. Name the minimal remedy — a one-field Fields protocol is a small contract, same principle — and note that `datamodel-adopts-its-fields` sees the same absence from the model side: anchor the finding in whichever area holds the richer evidence and cross-reference the other; do not file it twice. +- **A body that copies Fields members without adopting** is `validation-not-duplicated-downstream`'s finding (the `fields` area) — note the pairing, do not re-grade it here. +- **A DocC claiming the contract the code lacks** — "the same Fields validation applies at every layer" over a `validate` returning `nil` and no Fields protocol in the repo — is `comment-asserts-an-invariant-the-code-lacks` (`cross-cutting`) wearing request clothes; report it there and say what it will cost the next reader. + +The full composed shape is `ServerRequestBody` + Fields + `ValidatableModel` + `Stubbable`; a body missing `Stubbable` degrades request testing — say so as a note in the finding, not as this check's blocker. + +## Check: registration-uses-the-request-door +**Severity:** blocker +**What:** Requests are registered with `register(request:app:)`, mounted on middleware-only groups. +**Anti-pattern:** `try app.grouped("admin").register(request: DockPageRequest.self, app: app)` — a path-prefixing group; or a `ServerRequestController` route collection standing in for requests the request door already covers. +**Detection:** Establish first what the door can actually reach, because a controller is legitimate whenever the constraints cannot be met — and in some codebases they never can: + +- **Read door** — `register(request:app:)` requires `SR.ResponseBody: VaporResponseBodyFactory`. +- **Write doors** — `CreateRequest`/`UpdateRequest`/`DeleteRequest` additionally require `SR.RequestBody: DataModelWriter`. These are Fluent-container doors: a project with no Fluent layer cannot use them for *any* write, and every write controller is correct. + +Check the conformances before flagging. Skipping this turns every write controller in a non-Fluent project into a false blocker. + +Then flag two shapes. First, a `register(request:app:)` mounted on a group built with a path prefix — `grouped("string")` — rather than middleware only: the client derives the served URL from the request type, so a server-side prefix moves the route out from under that derivation. FOSMVVM rejects this at boot, so it is a startup failure rather than a silent one, but it is worth catching before the boot. + +Second, a `ServerRequestController` collection registered for operations `register(request:app:)` covers. Controllers are for what the request door does not reach — a `ReplaceRequest`, a multi-record operation. A collection standing in for ordinary CRUD is a parallel door, and the reason it usually exists is that someone needed to throw a status the typed error could not express, which is `controller-throws-the-declared-error` wearing a different hat. + + +## Check: controller-derives-its-own-route +**Severity:** blocker +**What:** A `ServerRequestController` mounts at the request's own path and binds the query through `VaporServerRequestMiddleware` — it does not invent either. +**Anti-pattern:** +```swift +private func path(forDestroy base: String) -> [PathComponent] { + [.constant(base), "destroy"] // client fetches /, server serves //destroy +} + +let query = try req.query.decode(RequestQuery.self) // form decoder; client sends JSON +``` +**Detection:** For each `ServerRequestController` that overrides `boot(routes:)`, check two things against the client's derivation, which is not negotiable and not visible from the server file: + +- **Path.** The client sets `urlComps.path = "/" + Self.path` with **no action suffix** (`Sources/FOSMVVM/Protocols/ServerRequest+Fetch.swift`). A hand-built `[PathComponent]` array that appends a verb — `"destroy"`, `"update"` — serves a URL no client asks for. Flag it. +- **Query.** The client sends the query as a **JSON string** (`try query?.toJSON()`), which `VaporServerRequestMiddleware` decodes. Vapor's `req.query.decode` is a URL-encoded-**form** decoder and cannot read it. Flag a bespoke `boot` that hand-decodes instead of binding through the middleware. + +This is the same guarantee `registration-uses-the-request-door` protects — client and server never independently invent a URL — but it fails far worse. A path-prefixing group is **rejected at boot**, loudly, before anything ships. A hand-built path compiles, boots, passes its own tests, and 404s in production. + +**Check the tests too, and say so.** This defect hides behind tests that hand-build the server's invented URL (`"\(Controller.baseURL)/destroy?…"`, or a query built with `URLEncodedFormEncoder`) rather than going through `processRequest`. A green suite over a bespoke `boot` is evidence of nothing; a test that calls `processRequest` would have caught it on the first run. + +## Check: request-names-follow-the-dictionary +**Severity:** warning +**What:** Request type names follow the naming dictionary (NAMES.md §1a–1c) — the entity noun leads, always: writes and semantic actions are `Request` (`UserCreateRequest`, `IdeaMoveRequest` — never `CreateUserRequest`, `MoveIdeaRequest`); a screen ViewModel's read is `Request` with **no verb** (`DocksRequest` — the read is the one canonical fetch); a raw-data read keeps an explicit noun-first `Show` (`UserShowRequest`). Noun-first keeps an entity's whole request family together — verb-first scatters it across the alphabet and buries the entity. +**Anti-pattern:** `CreateClientRequest`, `MoveIdeaRequest`, `GetDashboardRequest`, `MintAgentTokenRequest` — the dictionary's own wrong-column entries, in the wild. +**Detection:** **Enumerate `ServerRequest` conformers — never `*Request`-named types.** The suffix sweep hits domain types that merely end in the word (`PullRequest`, a GitHub wire type, is not a request and not a finding); conformance is the membership test, per this file's standing discipline. Then classify each conformer by its contract, not its name, and check the form the dictionary assigns: + +1. **Write / semantic action** (CRUD conformances — Create/Update/Delete/Replace — and write-actioned plain `ServerRequest`s, per `requestbody-adopts-its-fields`' enumeration): the leading token must be the entity, the action verb second-to-last. A leading `Create`/`Update`/`Delete`/`Get`/`Start`/`Stop`/`Mint`/`Move`/`Report`/`Accept`-style token that names the request's **own action** is the hit. +2. **Screen read** (a `ViewModelRequest` whose response is a `RequestableViewModel`): the name is the ViewModel's stem + `Request`, verbless. Resolve against the paired VM (the `viewmodel-request-pairing` walk already computes this): `DashboardViewModel` → `DashboardRequest`; `GetSidebarRequest` and `ShowXxxRequest` forms are hits. +3. **Raw read** (a read-shaped plain `ServerRequest`): `ShowRequest`, noun-first — `GetLicenseKeyRequest` is the hit form; `LicenseKeyShowRequest` is its correction. + +**The leading-token test is about the request's own action, not any verb-derived word.** A screen noun may legitimately contain one: a confirmation modal's read named after `DeleteConfirmationViewModel` correctly carries `Delete` inside the noun phrase — the request's action is a read, and the paired VM stem settles it. This is why classification precedes the token test. + +**A wholesale verb-first codebase gets one finding, not seventy — and the right framing.** Verb-first is the REST idiom's default and the default AI-authored code arrives with; a codebase written before this dictionary existed follows it uniformly, and that *dates* the code, it does not indict it. Report a single area-wide finding anchored at the largest-loss instance (per this file's guidance), listing the family and framing per the dictionary's own callout: existing verb-first names are **rename items — do not add more**. A *new* verb-first name arriving in the reviewed diff is the sharper per-instance finding, because the dictionary now exists to be followed. + +**Where the dictionary is silent, do not improvise.** A semantic the dictionary has no row for (a distinct `List` form, say) is a candidate for the owner — the finding derives the nearest dictionary-consistent spelling (a raw list read is still a read: plural noun + `Show`) and says the dictionary should rule. + +**The collision-contortion clause (NAMES.md §2), same stage, any projection type:** a display type renamed only to dodge sharing a name with its domain counterpart (`CatalogTier` display type beside `CatalogChannel.Tier`, renamed out of collision fear) is a warning — the module *is* the namespace, the collision is harmless, and a display name is chosen for **meaning**. A more-descriptive name chosen for meaning is fine; the tell is a name whose only explanation is the dodge. + +## Check: live-invalidation-is-a-pair +**Severity:** blocker +**What:** For a `@ViewModel(options: [.live])` screen, register a dependency on what the projection reads; invalidate projections of what changed — **both naming the same entity. That pairing is the live contract** (FOSMVVMArchitecture → Live Invalidation; the api-catalog states it verbatim). Plan-loaded records register automatically and Fluent-registered container commits notify automatically — the manual pair covers exactly what Fluent doesn't own: `Application`-hosted actors, computed aggregates, external feeds. +**Anti-pattern:** An `invalidateProjections(of: X)` no factory ever registers on — the emit nudges nobody, and the mutation lands invisibly; a `registerDependency(on: Y)` nothing ever invalidates — the screen renders that state once and never refreshes on its change; a hand-driven Fluent write to a live model inside a bare `database.transaction { }` — the framework cannot see the commit, stays silent (one warning), and no client refreshes, where `liveTransaction` is the wrapper. +**Detection:** Three sets, then pair them — and **enumerate every file before pairing**: registrations routinely live in `+Live.swift` factory extensions and `LiveInvalidation/` sentinel files, and a truncated sweep manufactures a dead-emit finding against a codebase that holds the contract (the verification run's own first sweep did exactly this). + +1. **The live set:** every `@ViewModel(options: [.live])` type (by attribute) and its server factory, wherever its extensions live. +2. **The registration set:** every `context.registerDependency(on: )`. **The emit set:** every `invalidateProjections(of: )`, `app`- and `req`-flavored, including those wired through composition-root closures (`onChange:` hooks in `configure`). +3. **Pair by entity expression.** An emit with no matching registration is a blocker — name the mutating source and which factory must register. A registration with no matching emit is a blocker — name the state and which mutator must emit. A live factory reading outside its plan (an `appState` snapshot, a computed aggregate) with no registration at all is the same blocker from the read side. +4. **The conformant idiom to recognize:** a sentinel `FOSMVVM.Model` projection type whose shared `static let observed` is referenced by *both* halves — one identifier holds the pairing together, and drift between the halves becomes impossible to spell. +5. **The bare-transaction clause:** a `.transaction { }` writing a registered live container model, outside `liveTransaction` and outside the framework's own write door (`DataModelWriter`), is a blocker — the refresh silently never happens (the catalog's own Don't). + +**Not hits:** `context.records(...)` reads (plan-loaded — auto-registered); writes through `DataModelWriter` or `liveTransaction` (framework-notified); non-live ViewModels, which carry no refresh contract to break; and `invalidateProjections` calls present but inert because `useLiveInvalidation` was never enabled — that is a boot-wiring observation, not a pairing defect (the call is documented as a safe no-op). + +## Check: server-installs-the-error-middleware +**Severity:** blocker +**What:** The server's boot path installs FOSMVVMVapor's `ErrorMiddleware` (ruled 2026-08-25; the api-catalog's entry is the statement — "Don't keep Vapor's stock ErrorMiddleware"). Without it, Vapor's stock middleware flattens every typed rejection — a validation failure, a thrown `ResponseError` — into a bare 500 with prose, and no client can branch on what happened. The field case: a Fields-contract validation ran correctly on the server and the client saw only `500 Internal Server Error`. +**Anti-pattern:** A `configure(_:)`/`registerServices(_:)` that registers requests and enables localization but never touches `app.middleware` — the stock middleware is silently in charge of every error. +**Detection:** In the server target's boot path, find `app.middleware.use(FOSMVVMVapor.ErrorMiddleware.default(environment:))`. **The module qualification matters**: Vapor declares its own `ErrorMiddleware`, the bare name is ambiguous beside it, and `Vapor.ErrorMiddleware.default` type-checks while installing the wrong one — verify which module's middleware is named, not merely that the line exists. Absence in any server target that declares `ServerRequest`s is the blocker; the remedy is the catalog's one-liner. A project-authored middleware demonstrably serving encodable errors typed-and-localized is a judgment call, not an automatic hit — say what it covers and what the FOS middleware would add. diff --git a/.claude/skills/fosmvvm-review/checks/swiftui-app-setup.md b/.claude/skills/fosmvvm-review/checks/swiftui-app-setup.md index 686a5b1e..d8c50ab7 100644 --- a/.claude/skills/fosmvvm-review/checks/swiftui-app-setup.md +++ b/.claude/skills/fosmvvm-review/checks/swiftui-app-setup.md @@ -3,8 +3,86 @@ area: swiftui-app-setup generator-skill: fosmvvm-swiftui-app-setup where: - "Sources/**/*App.swift" + - "Sources/**/*ResourceAccess.swift" + - "Sources/**/*ViewModels.swift" --- # SwiftUI App Setup Checks -The positive pattern lives in the `fosmvvm-swiftui-app-setup` skill. No review-only checks defined yet. +The positive pattern lives in the `fosmvvm-swiftui-app-setup` skill. This file covers the App struct as it is *maintained* — the edits it takes for the rest of the app's life, which is where these defects arrive. A scaffolded App struct starts correct; it drifts one hand-edit at a time. + +## Reviewer Guidance + +- **The App struct is generated, then hand-edited forever.** Most findings here are an edit made later — a view added without its registration, a bundle, an environment. Frame them that way: the remedy is the missing edit, not a regeneration. +- **But check the templates before assuming the scaffolder is right.** If a check appears to flag what `Sources/FOSMVVMBootstrap/Templates/*/…App.swift.tmpl` emits, stop and resolve the contradiction rather than reporting it — one of the check, the template, or the generator skill is stale, and it has been the skill before now. Say which, and report it as a finding against the framework, not against the app under review. +- **Find the bundle accessor by content, not by filename.** It is `{Module}ResourceAccess.swift` in one project shape and `{Module}.swift` in another — both shipped. Locate it with `grep -l "localizationBundle"` across the module's sources. Keying on a filename convention misses exactly the projects that drifted from it. +- **Do NOT recommend moving test-view registration out of `init()`** to "clean up" the initializer. The timing is load-bearing: `testHost()` resolves the view under test before the first render, so registration from a computed property, `.onAppear`, or `.task` arrives too late. This looks like an obvious tidy-up and breaks UI testing silently. +- **An empty `deploymentURLs` is not a missing configuration.** A wholly client-hosted app talks to no server, and `[Deployment: MVVMEnvironment.URLPackage]()` is the correct expression of that. Do NOT recommend inventing placeholder URLs to fill it. +- Project settings — targets, signing, build settings, the link and embed graph — are not this area's business. `fosmvvm-doctor` audits those. Report an App-struct concern here and leave the project file alone. + +## Check: mvvmenv-built-once +**Severity:** warning +**What:** `MVVMEnvironment` is built once by a static factory and held in `@State` — not rebuilt on every `body` pass. +**Anti-pattern:** `private var mvvmEnv: MVVMEnvironment { MVVMEnvironment(...) }` — a computed property referenced from `body`. +**Detection:** In the `@main` App struct, find the `MVVMEnvironment` declaration. Flag a **computed** form. `body` is evaluated repeatedly, so a computed property constructs a new `MVVMEnvironment` each pass and hands `.environment()` a different instance every render — churn on a value meant to be stable for the app's lifetime, plus repeated bundle and URL resolution. `@State private var mvvmEnv = makeMVVMEnvironment()` with a `@MainActor static func` factory is the shape the scaffolder emits and is **not** a hit. + +## Check: test-views-registered-in-init +**Severity:** blocker +**What:** Test-view registration happens in the App struct's `init()`. +**Anti-pattern:** `registerTestView(_:)` calls reached from a computed property, `.onAppear`, `.task`, or any point after the first render. +**Detection:** Find every `registerTestView(` call site and the path that reaches it. Flag any not reachable from the App's `init()`. `testHost()` resolves the view under test before the first render, so a later registration is simply absent when it is needed. The framework does emit a diagnostic naming the missing ViewModel — but the fix is always to move the registration earlier, never to register later still. + +## Check: all-viewmodelviews-registered +**Severity:** warning +**What:** Every `ViewModelView` in the app is registered, so it can be driven in isolation under test. +**Anti-pattern:** An app with eight `ViewModelView` conformers and six `registerTestView` calls — the two that were added last are the two nobody can test. +**Detection:** Enumerate `ViewModelView` conformers across the app's sources — they live in the View layer, outside this area's globs, so go read them — and compare against the `registerTestView(_:)` calls. + +**Resolve each conformer's `VM` associated type first; this step is mandatory, not an aside.** The registry holds one entry *per ViewModel*, not per view, so several views sharing one ViewModel are correctly represented by a single registration. Skipping this produces a false hit for every extra view in such a group. That sharing is itself a defect, but it belongs to `viewmodel-view-one-to-one` in `swiftui-view.md` — do not double-report it here. + +A conformer is a hit when it renders a ViewModel no registration covers **and** is reachable in the running app (something `bind()`s it). A conformer nothing binds is dead code — a finding, but not this one. + +**Anchor the finding at the App file's registry**, not at the conformer: the registry is the file in scope and the missing `registerTestView` line is the remedy. Name the unregistered conformer and its ViewModel in the message so the edit is obvious. + +## Check: deployment-urls-distinguish-environments +**Severity:** blocker +**What:** Each declared deployment environment resolves to the host it names. +**Anti-pattern:** +```swift +deploymentURLs: [ + .production: .init(serverBaseURL: URL(string: "https://api.example.com")!), + .debug: .init(serverBaseURL: URL(string: "https://api.example.com")!), // ← production + // .debug: .init(serverBaseURL: URL(string: "http://localhost:8080")!) ← commented out +] +``` +**Detection:** Compare the URLs across environments. `MVVMEnvironment` accepts both a `[Deployment: MVVMEnvironment.URLPackage]` and a plain `[Deployment: URL]`; the defect and the check are identical in either, so do not pattern-match on the `.init(serverBaseURL:)` spelling alone. Flag two environments resolving to the same host, and flag a commented-out local URL sitting beside a live production one — the shape of a temporary change that stayed. Every debug run and every UI-test launch then talks to production, which is a data-safety problem before it is a configuration one. An empty `deploymentURLs` is not a hit: see Reviewer Guidance. + +## Check: client-hosted-vms-need-resource-bundles +**Severity:** blocker +**What:** An app with client-hosted ViewModels wires their localization bundles into `resourceBundles`. +**Anti-pattern:** A `@ViewModel(options: [.clientHostedFactory])` in the app, and `resourceBundles: []` — or a list missing that module's accessor. +**Detection:** Find ViewModels declared with `clientHostedFactory`, resolve which module carries each one's YAML, and check that module's `localizationBundle` accessor appears in `resourceBundles`. Flag a missing one. The symptoms are `missingLocalizationStore` or `noResourcePaths` at runtime, neither of which names the absent bundle — which is why this is worth catching in review. + +## Check: resource-directory-name-matches-hosting +**Severity:** blocker +**What:** `resourceDirectoryName` matches how the bundle was built, and there are three correct answers. +**Anti-pattern:** `resourceDirectoryName: "ViewModels"` for an Xcode framework bundle — the on-disk subfolder that Xcode flattened away, so the search walks a path the built bundle does not contain. +**Detection:** Two places carry a `resourceDirectoryName`, and **both are in scope from the start** — the call sites that pass one, and the bundle accessor's documentation that tells callers which to pass. An app whose every call site is correct can still ship an accessor whose DocC instructs the next caller into `.noResourcePaths`; check the documentation as a first-class site, not as an afterthought once the call sites come back clean. + +For each localization load — `MVVMEnvironment`, `loadLocalizationStore`, `initYamlLocalization` — **and for each `localizationBundle` accessor's DocC** — establish the bundle's build system first. That step is the whole cost of this check, so do it deliberately: + +- A target listed in `Package.swift` is an **SPM** target; check its `resources:` declaration for the folder it copies. +- A target present in the `.pbxproj` and absent from `Package.swift` is an **Xcode** target. +- For a test target, the platform comes from the **test plan's** membership, not from wherever `swift test` happens to run. + +Then check the value: + +- **Xcode framework target → `""`, or the argument omitted.** Xcode flattens grouped resources into the bundle's resource root, so any subfolder name finds nothing. `nil` coalesces to `""` and recurses from the root; passing `""` explicitly and omitting it are equivalent, and neither is a hit. +- **SPM target using `.copy("Resources")` → `"Resources"`.** SPM preserves the folder. +- **SPM library with `Resources/Localizations` → `"Localizations"`.** + +One project legitimately carries more than one of these at once — a client-server app loads `""` for its Xcode client framework and `"Resources"` for its SPM server target. Do not flag inconsistency between them; flag a value that contradicts *its own* bundle. + +When the hit is in documentation rather than a call site, grade it at the severity of what it will cause, and say plainly that no live load is currently broken. + +**The test-default trap is conditional.** `loadLocalizationStore`'s `resourceDirectoryName` **defaults to `"Resources"`**, which resolves on macOS and throws `.noResourcePaths` on iOS's flat bundles. A test that omits the argument is a hit only if that test target actually builds for iOS — check the test plan before flagging. A pure-SPM, macOS-only test target omitting it is correct. diff --git a/.claude/skills/fosmvvm-review/checks/swiftui-view.md b/.claude/skills/fosmvvm-review/checks/swiftui-view.md deleted file mode 100644 index 784745ad..00000000 --- a/.claude/skills/fosmvvm-review/checks/swiftui-view.md +++ /dev/null @@ -1,35 +0,0 @@ ---- -area: swiftui-view -generator-skill: fosmvvm-swiftui-view-generator -where: - - "Sources/**/Views/**/*.swift" - - "Sources/**/*View.swift" ---- - -# SwiftUI View Checks - -The positive pattern lives in the `fosmvvm-swiftui-view-generator` skill. This file documents review-only concerns for View bodies and their interaction with VMs and the `@Observable` data store. - -## Reviewer Guidance - -- Do NOT recommend removing `@Environment(SomeAppState.self)` from a view to "simplify" by reading through `viewModel.someObservableRef.x`. Production may bind both to the same instance, but tests inject independently — collapsing the split breaks test host injection. -- Do NOT recommend collapsing env/VM read-write splits. The split is required for test host injection (TestConfiguration pattern). View reads come from the VM; mutations go through Operations to the `@Observable` store; tests inject the VM stub's state into the env to mirror this. -- The VM is the single source of truth for what Views display. If a value needs to appear in a View, expose it as a frozen scalar on the VM at projection time. Do NOT recommend reaching back into the data store from the View body for display data. - -## Check: view-reads-vm-only -**Severity:** blocker -**What:** Views read display data from the VM, never directly from `@Environment`-shadowed data store types when the VM exposes the equivalent. -**Anti-pattern:** A View reads `programmingSettings.amplitudeValue` from `@Environment(ProgrammingSettings.self)` for display when the VM already exposes `amplitudeValue` as a frozen scalar. -**Detection:** For each View, find `@Environment` declarations of `@Observable` types. For each property read off those env values in the View body, check whether the VM exposes the same property name. Flag overlapping reads — the View should be reading from the VM. - -## Check: view-no-env-mutation -**Severity:** blocker -**What:** View bodies do not mutate `@Observable` state directly. Mutations go through Operations. -**Anti-pattern:** `programmingSettings.isEnabled = true` written inline in a View body or button action closure. -**Detection:** Inside View bodies and the closures they construct, find assignments where the LHS resolves to a property of an `@Observable` env value. Flag any such assignment. (Operations dispatched from button actions are fine — they call methods on a `*ViewModelOperations` conformer, which mutates internally.) - -## Check: view-no-read-through-vm-ref -**Severity:** warning -**What:** A VM may hold a reference to its `@Observable` state for ops dispatch, but Views must not read display data through that reference. -**Anti-pattern:** `viewModel.patientPanelSettings.electrodeSettings[0].isLocked` read in a View body for display. -**Detection:** In View bodies, find chained reads through VM properties whose type is `@Observable`. Flag reads of properties that the VM could expose as frozen scalars at projection time. diff --git a/.claude/skills/fosmvvm-review/checks/ui-tests.md b/.claude/skills/fosmvvm-review/checks/ui-tests.md index a52f1480..815e5948 100644 --- a/.claude/skills/fosmvvm-review/checks/ui-tests.md +++ b/.claude/skills/fosmvvm-review/checks/ui-tests.md @@ -14,10 +14,62 @@ The positive pattern lives in the `fosmvvm-ui-tests-generator` skill. This file ## Reviewer Guidance - Do NOT recommend collapsing the env/VM split in production views to "make the test pass" or "simplify." The split is the architectural reason the test host pattern exists. The correct fix when a UI test fails because env state and VM state diverge is to thread the VM stub's state through `TestConfiguration` into the env — not to remove the env or the read/write boundary. +- **`uiTestingElement(_:)` ships in FOSUtilities 0.12.0.** Below that pin the raw accessors were the only option: report them as *correct at time of writing, now fixable*, naming the version that lifts it — not as authored defects. Read the pin from the **xcodeproj's** `project.xcworkspace/xcshareddata/swiftpm/Package.resolved`, which governs the UI-test target and can disagree with the root SPM one. +- **There may be no `TestConfiguration` type in the repo at all.** Where the checks below say "thread it through `TestConfiguration`", that is the shape to build, not a file to find — the closure form is `.testHost { testConfiguration, testView in … }`, and the payload carries the VM stub's state into the env. Say so plainly rather than pointing at something that does not exist. - Test host blocks must mirror production binding. In production, `bind(appState: .init(...))` projects the data store into the VM. In tests, `TestConfiguration` is the analogue — it must construct env state from the VM stub's settings, not from independent `.stub()` calls. ## Check: testhost-mirrors-vm-settings **Severity:** blocker **What:** Test host blocks must construct `@Observable` env state from the VM stub's settings, not from independent `.stub()` calls. The env and the VM must hold the same instance, mirroring production binding. -**Anti-pattern:** `let env = ProgrammingSettings(patientRight: .stub())` in a test host while the injected VM holds a different `PatientPanelSettings` instance — taps mutate one, the View reads the other, the test fails for an incorrect reason. +**Anti-pattern:** `let env = ProgrammingSettings(patientRight: .stub())` in a test host while the injected VM holds a different `PatientPanelSettings` instance — taps mutate one, the View reads the other, the test fails for an incorrect reason. Equally a hit, and worse: an env holding a **live production** object, `LocalDockStore(prober: LocalDockOps())`, injected at App scope with no relationship to the stub. + +**Where to look when the app uses the plain `.testHost()`.** The env-construction site is often not in a UI test file at all — it is the app's `@main`, injected on the `WindowGroup`. A reviewer searching only the test target reports "nothing found" and is wrong. The tell is structural, **graded by what the environment holds** (ruled 2026-08-25 — the seam arrives with the App State): plain `.testHost()` plus a registered test view reading a **project-authored `@Observable`** (`.environment(appState)`) is the finding — the hosted view reads app state no test can reach, and the decorator + a real `TestConfiguration` should have arrived with that injection. When the only environment is the framework's (`MVVMEnvironment`), plain `.testHost()` is the correct baseline — at most note the latent shape (a stub that ever stops discarding the env would do real work against the production URLs). Broaden past `.stub()` — the anti-pattern is *any* env construction not derived from the VM stub, and a live production object is the most dangerous form because it will do real work. A payload-free `TestConfiguration` no test constructs is the same ruling's other half: a dead seam, flagged as scaffolding noise rather than wired transport. + +**Grade latent and active differently, and say which.** A divergence where the stub ops ignores the env, and the view happens to render only from the VM, breaks nothing today — the first test that asserts on env state hits it. Report the tier: `blocker` when a test fails or does real work now, `warning` when it is latent, and in both cases name what will trip it. Do not flatten the two; whoever triages the list needs the difference. **Detection:** Find blocks named `testHost`, `setUp`, or `presentView` in UI test files. For each construction of `@Observable` env state, verify it threads through the VM stub's settings (typically via `TestConfiguration` payload). Flag env constructions that use `.stub()` independent of the VM. + +## Check: elements-reached-by-identifier +**Severity:** blocker +**What:** Tagged views are reached with `uiTestingElement(_:)`, never through XCUITest's element-type accessors. +**Anti-pattern:** `app.buttons["saveButton"].tap()` · `app.staticTexts["title"].label` · `app.otherElements["banner"].exists` +**Detection:** In UI test sources, flag reads or gestures that go through `app.buttons`, `app.staticTexts`, `app.otherElements`, `app.textFields`, and their siblings, for a view the app tags with `uiTestingIdentifier`. This includes the laundered form — a `private extension XCUIApplication` vending `var someTitle: XCUIElement { staticTexts.element(matching: .staticText, identifier: "…") }`. Wrapping the type query in a computed property does not remove the type query; it hides it from a grep and leaves the coupling in place. An accessor keyed on an element type bakes a rendering detail into the test: the test breaks when a `Button` becomes a `Menu`, which is a change with no behavioural meaning. `uiTestingElement(_:)` is keyed on the identifier alone and survives it. + +There is a second reason, and it is the one that costs afternoons. A gesture against an identifier no view carries fails the test *naming that identifier*, so a typo reads as a typo. The same typo through `app.buttons[…]` surfaces as an XCUITest snapshot error about an element that does not exist, which reads like a timing or hierarchy problem and gets debugged as one. + +A raw accessor is legitimate for something FOSMVVM does not tag — a system alert, a share sheet, a keyboard key. Do not flag those; flag the ones reaching a view the app itself tagged. + +## Check: harness-merges-every-yaml-bundle +**Severity:** blocker +**What:** A view-test harness localizing from more than one target's YAML uses the multi-bundle `setUp(bundles:)` form. +**Anti-pattern:** A harness calling the single-bundle `setUp(bundle:)` in an app whose ViewModels are localized from two places — its own YAML plus another target's — so half the strings resolve and half fall back. +**Detection:** Two failures, and they present in opposite ways. + +**Some of the bundles (N of M) → silent.** Strings from the unmerged bundle resolve to their fallback, so the test asserts against a key or an English default and passes. Read for it; running proves nothing. + +**None of them (0 of M) → loud.** When the passed bundle contains no YAML at all, `yamlStoreConfig` throws `noResourcePaths` and `setUp` dies before any test body runs. The whole harness is dead rather than quietly wrong. + +**Check that the YAML physically reaches the bundle — do not stop at the call shape.** `bundle: Bundle(for: Self.self)` is a perfectly correct call that resolves nothing if the test target copies no resources. The failure is one layer down, in the Xcode target's wiring. Open the target's `PBXResourcesBuildPhase` and its synchronized groups, and settle it definitively against the built product: `find …/SomeUITests.xctest -name '*.yml'`. That takes five seconds and is the only answer that cannot be argued with. + +Then, for each `ViewModelViewTestCase` / `ViewModelDisplayTestCase` harness, determine how many bundles carry YAML for the ViewModels it drives. In a client-server app that is routinely two: the client framework's own resources and the server-side contract's. Flag a harness passing one bundle where the ViewModels it exercises span several. + +`setUp(bundles:resourceDirectoryName:appBundleIdentifier:locales:)` merges them into one store (FOSTestingUI, 0.13.2); `loadLocalizationStore(bundles:)` is the FOSTesting equivalent. Before that release the single-bundle form was the only option and harnesses worked around the gap — a symlinked `Resources` directory in the test target is the usual tell, and is worth reporting as the workaround it now is. + +The symptom is not a failure. Strings from the unmerged bundle resolve to their fallback, so the test asserts against a key or an English default and passes — which is why this is worth catching by reading rather than by running. + + +## Check: no-hand-rolled-element-helpers +**Severity:** warning +**What:** Tests use the verified interaction APIs rather than re-implementing them. +**Anti-pattern:** +```swift +extension XCUIElement { + var text: String? { value as? String } + func typeTextAndWait(_ string: String, timeout: TimeInterval = 2) { … } + func tapMenu() { … } +} +``` +**Detection:** Flag `XCUIElement`/`XCUIApplication` extensions re-implementing what `uiTestingElement(_:)` already vends — `text` for `.value`, `typeTextAndWait`/`selectTypeTextAndWait` for `.setText(_:)`, `tapMenu` for `.tap()`, hand-rolled `waitFor…` loops for `waitForExistence()`. These are the cases the verified APIs were written to absorb, including the keyboard-occlusion and menu-dismissal handling a hand-rolled version will not have. + +**Check whether the helpers are used before grading.** An unused helper set is dead code, which lowers the severity but not the finding: it sits in the test target as a template, and the next author writes against it. + +**A hand-rolled wait is not automatically a hit.** The verified APIs poll *UI elements*. A test polling something else — a transported operations stub, an out-of-process side effect — has no verified equivalent to reach for, and that is a gap in the framework rather than a defect in the test. Say which you are looking at; if it is the framework gap, report it as one so it can be closed upstream. diff --git a/.claude/skills/fosmvvm-review/checks/view.md b/.claude/skills/fosmvvm-review/checks/view.md new file mode 100644 index 00000000..83285c6c --- /dev/null +++ b/.claude/skills/fosmvvm-review/checks/view.md @@ -0,0 +1,148 @@ +--- +area: view +generator-skill: fosmvvm-swiftui-view-generator +where: + - "Sources/**/Views/**/*.swift" + - "Sources/**/*Views/**/*.swift" + - "Sources/**/*View.swift" + - "**/Resources/Views/**/*.leaf" + - "**/*.leaf" + - "**/components/**/*.tsx" + - "**/*.tsx" + - "**/*.jsx" +--- + +# View Checks + +One area, three surfaces (ruled 2026-08-25): SwiftUI views, Leaf templates, and React components are all projections of the same edge — View ← ViewModel + ratified design — and the rules below are per-edge, with per-surface detections. This file covers View bodies/templates and their interaction with ViewModels and the `@Observable` data store. + +## Reviewer Guidance + +- **Three surfaces, three generators — cite the right one.** The frontmatter names the SwiftUI generator; for `.leaf` files the positive pattern is `fosmvvm-leaf-view-generator`, and for `.tsx`/`.jsx` it is `fosmvvm-react-view-generator`. Cite the surface's own generator in findings, whatever the dispatch prompt's default says. +- Do NOT recommend removing `@Environment(SomeAppState.self)` from a view to "simplify" by reading through `viewModel.someObservableRef.x`. Production may bind both to the same instance, but tests inject independently — collapsing the split breaks test host injection. +- Do NOT recommend collapsing env/VM read-write splits. The split is required for test host injection (TestConfiguration pattern). View reads come from the VM; mutations go through Operations to the `@Observable` store; tests inject the VM stub's state into the env to mirror this. +- The VM is the single source of truth for what Views display — on every surface. If a value needs to appear, it is a frozen scalar (or stored `Localizable`) on the VM at projection time. Do NOT recommend reaching back into the data store, the request, or the session from a view or template for display data. +- **A SwiftUI view is identified by `: View` conformance, not by its filename.** Views routinely live in files named `*Tile.swift`, `*Card.swift`, `*Row.swift`, `*Section.swift`, `*Strip.swift`, or share a file with the screen that uses them. Enumerate every `: View` conformer in the scoped files and evaluate each one; do not limit the structural checks to types whose name ends in `View`. +- **A plain `View` is legitimate when it renders no ViewModel data.** A leaf that takes only primitives, colors, or geometry (a spinner, a swatch, a divider) is a presentational component and is correctly a plain `View` — do NOT flag it. The structural checks below fire only on views that hold ViewModel-typed state. +- Do NOT recommend satisfying the 1:1 rule by deleting a sub-view and inlining its body into its parent. The fix direction is the other way: the sub-view gets its own ViewModel, projected as a child of the parent's. +- **A template-side `fetch('/api/…')` is `server-calls-use-the-request-door`'s finding** (`cross-cutting`), reported at **warning** under that check's name per its TBD ruling (2026-08-25) — JavaScript in a Leaf template is client code, and the WebApp's JS→route bridge is the sanctioned path. Note it when you see it; this area carries the eyes, that check carries the name. +- **Leaf renders `Localizable` values natively** (`LeafDataRepresentable`, FOSUtilities 0.4.0+): `#(card.createdAt)` on a `LocalizableDate` renders the localized string. A template that re-formats instead of rendering is `views-render-they-dont-shape`'s business below. +- **Leaf drift never errors — it renders silently empty.** LeafKit resolves an unknown variable to nothing and emits an unknown tag as literal text, skipped. A template reading properties its VM does not have, or using a tag that is not registered (`#set` does not exist in LeafKit's built-ins), produces a page with blank holes and no log line. The runtime will not catch what these checks catch — say so in findings, because "it renders" is not evidence. +- **Follow `