diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/009-async-local-method-body-retention.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/009-async-local-method-body-retention.md index a167e3f0..8501ce45 100644 --- a/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/009-async-local-method-body-retention.md +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/009-async-local-method-body-retention.md @@ -3,12 +3,13 @@ **Plan #:** 009 **Date:** 2026-08-13 **Related Todo:** [../todo.md](../todo.md) -**Status:** Stub -**Last Updated:** 2026-08-13 +**Status:** Done +**Last Updated:** 2026-08-14 +**Plan review:** [`../reviews/009-plan-review.md`](../reviews/009-plan-review.md) — CONCERNS, 8 veto-tier, all closed before implementation **Plan-review opt-in:** Yes (same grounds as TRIM-008 — a false IP-protection guarantee, and the remedy is unknown at stub time) **Code-review opt-in:** Yes (behavior-changing generator work, if the remedy turns out to be generator-side) -> **Stub.** Scope and the measured evidence only. Steps, Acceptance, Current State, and Test Evidence flesh out at this plan's turn, per the iterative-todo workflow. Nothing below prescribes a remedy — the cause is measured, the fix is not yet designed. +> **Promoted from Stub 2026-08-14.** The stub's declared first step — separate H1 from H2 — has been **run and answered** before any design work, and it also **falsified the remedy the stub predicted**. See [Separation experiment](#the-separation-experiment-2026-08-14) and [Approach](#approach). Everything above that section is the stub's original text, preserved. --- @@ -135,3 +136,189 @@ Corroboration from the same run: `IClassLegPort` and `ClassLegInvoke` flipped to - Do the async lifecycle type-tests (`IFactoryOnStartAsync` / `IFactoryOnCompleteAsync` / `IFactoryOnCancelled` / `IFactoryOnCancelledAsync`) keep their branches alive? `TrimTestEntity` implements none of them and `target` is statically typed, so not emitting probes for interfaces the concrete type provably cannot implement is both a candidate fix and a worthwhile emission improvement regardless of the outcome. - Does `LocalSave`'s routing keep `LocalInsert`/`LocalUpdate`/`LocalDelete` rooted even if their own registrations were guarded? - CI gate: which of the new Save/Can\* markers can be asserted absent once this lands, and what is the durable positive control for them. + +--- + +## The separation experiment (2026-08-14) + +The stub's declared first step, run before any design work. Evidence: [`../reviews/009-evidence/`](../reviews/009-evidence/). + +**Apparatus.** Two compile-time knobs in `ClassFactoryRenderer`, plus a third for the remedy probe. Before any variant was run, both knobs were built at their default (`false`) and the whole generated tree diffed against the pre-edit tree — **identical**, so the apparatus is inert and any later difference is attributable to the knob rather than to the refactor that carried it. The knobs were reverted afterwards and the emission re-diffed back to HEAD: **identical again**. Archived as `experiment-knobs.diff`. + +| | Shape emitted | `ClassSyncBody_MARKER` | `ClassAsyncBody_MARKER` | State machines | +|---|---|---|---|---| +| HEAD | as shipped | absent | **PRESENT** | `d__` present | +| **V1** | async, **minus** all four awaiting probes **and** the OCE catch arm | absent | **PRESENT** | `d__` present | +| **V2** | sync, **plus** an OCE catch arm **and** the `IFactoryOnCancelled` probe | **absent** | PRESENT (untouched) | `d__` absent | +| **V3** | guard in a sync wrapper; async body in a `private` core | absent | **PRESENT** | `d__` **present** (see note) | + +All five positive controls passed on every run, so each "absent" is a real absence rather than an unread artifact. + +> **Note on V3's state-machine row, corrected at plan review (finding B1/B2).** The first draft of this table also reported `d__` **absent** in V3 and read that as "the fold works once the guard is outside the state machine". **That was a check that could not have gone red.** The V3 knob emits `LocalFetchAsync` as a non-async wrapper, so the compiler never creates a state machine by that name — its absence is a compile-time consequence of the rename, not a trimming result. The row is struck. `d__` was measured, but in a separate command that never reached the archived probe; it is re-measured and recorded in the evidence addendum. + +### H1 confirmed; H2 falsified in both directions + +- **V1 — subtractive, and this is what carries the finding.** Strip *all five* constructs H2 blames — the OCE catch arm and all four lifecycle probes — and the async body *still* leaks. **None of them is necessary.** +- **V2 — additive, and narrower than the first draft claimed.** Graft the OCE catch arm and the (non-awaiting) `IFactoryOnCancelled` probe onto the sync body and it *still* trims clean. That is **2 of the 5** constructs: the three *awaiting* probes cannot be grafted onto a sync method at all, so V2 cannot speak to them. Corrected at plan review (finding B5) — the falsification rests on V1, with V2 as partial corroboration. + +**H1 is the mechanism: the fold does not propagate out of the async state machine.** This also falsifies the TRIM-004 story — *"early-throw guard plus try/catch defeats unreachable-code elimination"* — for the **third** time in this arc, and for the first time by a direct additive test rather than by an argument. + +**Best-supported mechanism, offered as inference and not as measurement.** In an async method the *entire* user body — guard included — is lowered into `MoveNext` and wrapped in the compiler's own try/catch that funnels exceptions onto the builder. The fold therefore lands *inside* a protected region, and the unreachable remainder is not removed. In the sync method the guard sits *before* the user's `try`, so unreachability begins outside any protected region and the whole remainder, `try` block and all, is eliminated. This is consistent with all four rows; what is *measured* is the behaviour in the table, and any remedy must be re-verified against a published artifact rather than against this paragraph. + +### V3 falsifies the remedy the stub predicted + +The stub proposed "move the guard out of the async method entirely (a sync wrapper testing `IsServerRuntime` before calling the async body)". **V3 emitted exactly that, and the markers did not move:** `ClassAsyncBody_MARKER`, `IClassLegPort`, and `ClassLegInvoke` are all still present, and `d__` survives. + +**So guard relocation alone does not clear this leg.** A fix that stopped there would have produced a smaller assembly, deleted one state machine, and left the IP on the client. + +**What V3 does *not* establish — corrected at plan review.** The first draft claimed V3 proved the fold works inside the wrapper, and attributed the core's survival to DAM. Neither follows: + +- The "fold works" claim rested on `d__` being absent, which the rename guarantees regardless of trimming. Struck; see the note above. +- **V3 cannot attribute the core's survival at all.** Both candidate roots were live in V3 — DAM on the factory type, *and* the wrapper's own `return LocalFetchAsyncCore(…)` call site. "DAM roots the core" and "the wrapper's call survived the fold" predict the identical observation, and they imply **different remedies**: only the first is addressed by holder indirection. Separating them is [Step 1](#steps). + +This is the arc's own rule — *a check that could never go red is not evidence* — recurring for the fourth time, in the plan written to avoid exactly that. It is recorded rather than quietly rewritten because the correction is the useful part. + +--- + +## Root inventory (read out of the emitted source, not inferred) + +**Three** roots reach `LocalFetchAsync`. The first draft of this section said two and called itself exhaustive; the third was found at plan review (finding B3). + +1. **DAM** — `[assembly: NeatooFactoryRegistrar(typeof(global::RemoteFactory.TrimmingTests.TrimTestEntityFactory))]`. The attribute names the factory itself, so `PublicMethods | NonPublicMethods` roots every `Local*` **and** any private core beside them. +2. **The delegate registration closure** — `services.AddScoped(cc => { var factory = …; return (…) => factory.LocalFetchAsync(…); })`, emitted with **no** `IsServerRuntime` guard, unlike the static and relay legs. +3. **The local constructor's method-group assignment** — `ClassFactoryRenderer.cs:220-223` emits `{UniqueName}Property = Local{UniqueName};`, so the ctor body reads `FetchAsyncProperty = LocalFetchAsync;`. That ctor is reachable: `FactoryServiceRegistrar` emits `services.AddScoped<{X}Factory>()`, whose generic parameter carries `DynamicallyAccessedMembers(PublicConstructors)`. + +**Root 3 does not change the prediction below** — the method group targets the **wrapper**, exactly as the closure does, so the core still loses its last non-DAM reference when the wrapper's post-guard call folds away. It is recorded because an inventory that claims to be empirical, and that Step 1's stop condition is read against, has to be right. + +**Not a root:** `ITrimTestEntityFactory` declares only `Create` and `FetchAsync` — the public entry points — never `Local*`. Verified in the emitted interface. (Noted for completeness, though this was never the plausible one; the ctor assignment was.) + +**The sync `LocalCreate` carries both of those same roots and still trims clean.** So this was never a rooting problem in the sync case, and "de-root it" is not a description of the defect — it is one of two things the fix has to do, because rooting only becomes fatal once elimination stops working. + +--- + +## Approach + +**Two changes. Neither has been measured working — V3 measured the first one *not* sufficient on its own, which is a different thing.** + +1. **Sync wrapper for guarded async `Local*`** — the guard moves to a non-async wrapper; the async body moves to a private core. This is the lever H1 implies: get the fold out from inside the state machine's protected region. +2. **Holder indirection for class factories** — the assembly attribute names a generated single-method holder rather than `{X}Factory`, exactly as [TRIM-008](./008-registrar-dam-over-preservation.md) already did for the static and relay legs. This removes the DAM root, which V3 leaves live and therefore could not rule in or out. + +**The prediction that makes this small:** with the wrapper in place, roots 2 and 3 die too without being touched. Both reference the **wrapper**; the wrapper's `return LocalXCore(…)` sits after the guard, so the fold removes it and the core loses its last non-DAM reference. If that holds, **the delegate registrations need no guarding** and neither does the ctor assignment. + +**This is a prediction, and the arc's rule applies to it.** Each half is measured; the combination is not. Step 1 is to measure the combination against a published artifact before anything else is built on it — the same discipline that just caught the stub's own predicted remedy. + +**Why holder indirection does not break prebuilt consumers.** It changes *which type the attribute names*, not the breadth of the DAM. A library compiled by an older generator keeps naming its factory and keeps exactly today's behaviour — no registration is lost and no diagnostic is needed. This is the compatibility argument TRIM-008 made and shipped; TRIM-009 reuses it rather than re-deriving it. + +**Rejected, with reason:** narrowing the DAM on `NeatooFactoryRegistrarAttribute` to `PublicMethods`. Rejected at TRIM-008's plan review and still rejected — a prebuilt library whose registrar is `internal static` would silently stop registering on a trimmed client, with no diagnostic. TRIM-009 changes nothing about `FactoryAttributes.cs`. + +--- + +## Steps + +1. **Measure the combination (V4) before building on it.** Wrapper + holder together, published trimmed, probed. Expected: `ClassAsyncBody_MARKER`, `IClassLegPort`, `ClassLegInvoke`, and all three `SaveLeg*Body_MARKER` absent; positive controls unchanged. **If the closure or ctor-assignment root survives the wrapper, stop and re-design** — do not proceed on the assumption it will work. + + **The stop condition needs a liveness check, not just absence** (finding B7). `AddRemoteFactoryServices` resolves the registrar with `GetMethod(...)` then `method?.Invoke(...)`, so a holder that fails to forward produces **no diagnostic and no exception** — every marker would go absent and V4 would read as a flawless result. V4 does not count as green unless it also carries a **named** positive control for the class-factory holder type (full name, as `verify-trimmed.sh` already does for the other two) **and** the harness resolves the class factory and exits 0. +2. Emit the sync wrapper / private core split for guarded async `Local*` methods across **all five** emission sites in `ClassFactoryRenderer`, named rather than numbered because the first draft's three-site list and its rationale were both wrong (finding B4): + + | Renderer method | Shape | `async` when | + |---|---|---| + | `RenderReadLocalMethod` | read | `IsAsync \|\| IsDomainMethodTask` — **the only site the experiment wired** | + | `RenderClassExecuteLocalMethod` | class-level `[Execute]` | **always** — see Step 2a | + | `RenderLocalMethod` | write | `IsAsync \|\| IsDomainMethodTask` | + | `RenderSaveLocalMethod` | `LocalSave` | `IsAsync`; emitted `public virtual` | + | `RenderCanLocalMethod` | `Can*` | `IsAsync` | + + The struck rationale claimed the Save/Can\* leg was reached by the write and `Can*` sites. Measured in the emitted `TrimSaveTargetFactory`: the `Can*` methods are **synchronous** (`public Authorized LocalCanCreate(…)`) and `LocalSave` is its own async site. **Leaving `LocalSave` unwrapped would probably still clear the markers** — its surviving body references the wrappers, whose folds kill the cores — which means Step 1 could come back green while a guarded async body still ships, invisible to the gate. That is why the site list is enumerated here instead of inferred from a passing probe. + +2a. **Class-level `[Execute]` needs its own decision, and it is release-blocking** (finding A3). `RenderClassExecuteLocalMethod` emits `public async` **unconditionally**, with the same guard, resolving `[Service]`s in the generated body and calling the consumer's `public static` method directly — so H1 applies in full. It is a **Design source-of-truth pattern** (`Design.Domain/FactoryPatterns/ClassFactoryWithExecute.cs`), and the harness has **no target for it**, so AC6's "proven in the trimmed harness, not inferred" cannot be satisfied for this shape today. Add a harness target and fix it with the rest; do not close AC6 while it is unmeasured. + +3. Emit the registrar holder for the class-factory leg and retarget its assembly attribute, reusing TRIM-008's proven shape and a leg-distinct **third** prefix. + +3a. Update the `NeatooFactoryRegistrarAttribute` XML contract in `FactoryAttributes.cs` (finding A1). The plan previously declared that file untouched, which would ship a knowingly-false contract in the very remarks written to stop this defect recurring. Today it says the `Type` "must be a GENERATED registrar type. Never a consumer's own class" — after this plan that is **necessary but not sufficient**, since the class leg already named a generated type and still leaked. The historical note listing two legs also becomes three. +4. Pin both in generator unit tests, including the regression assertion that the attribute **does not** name the factory type — the check whose absence let the static leg ship broken. Prove each new assertion RED before green. **This is an inversion of an existing passing test, not new coverage:** `AssemblyAttributeEmissionTests.cs:42` currently asserts the attribute names `global::TestNamespace.MyEntityFactory`. Original intent is preserved — the attribute is still emitted and still names the correct type; what changes is that the correct type is now a generated single-method holder. Naming it as an inversion is what the sacred-tests rule requires. +5. Flip the CI gate: the eight markers TRIM-008 deliberately asserted **PRESENT** as a TRIM-009 tripwire become absence assertions, each with a durable positive control. **The prose flips too** — `verify-trimmed.sh` carries a block explaining why `IClassLegPort`/`ClassLegInvoke` are excluded from the absence list, a controlled-pair block that still says "TRIM-009 must separate them from inside the generator" (now done), a KNOWN-BROKEN block, and a summary line. All become false with the fix. +6. Retire the load-bearing asymmetry note in `TrimTestEntity.cs`. Its stated reason — that giving the async half `IServerOnlyRepository` would surface that name and redden the static-factory markers for a misleading reason — **expires when this lands**, and a stale do-not-touch comment is its own hazard. The same file's "WHAT THIS PAIR DOES NOT ISOLATE" paragraph is also answered by the experiment and must go with it. +7. Work **two disjoint doc sets**. The first draft named only the first and would have shipped the second false (finding A2). + - **7a — the nine body-trimming anchors** enumerated in [`../reviews/008-doc-anchor-inventory.md`](../reviews/008-doc-anchor-inventory.md), including the forward-looking skill table row TRIM-008 wrote on the promise this plan would land. + - **7b — the holder anchors, all written by TRIM-008 and falsified by this plan's Approach.** `CLAUDE-DESIGN.md:760` and `docs/trimming.md:249` both state that for class factories the protection comes from the guard, "**not the choice of attribute target**" — the exact claim TRIM-009 reverses. Also: the `CLAUDE-DESIGN.md` attribute-target table's class-factory row (`typeof({X}Factory)` → holder), `CLAUDE-DESIGN.md:771` ("the **two** holder rows" → three), the "until v1.7.0 the static-factory and event-handler rows named the user's own class" note, and the skill's two-leg framing in `skills/RemoteFactory/references/trimming.md`. Build this list by reading the files, per the inventory's own lesson about listing before editing. +8. Reconcile the container: + - Close deferred item **18**; fire item **2** (the release hold, which reopens when items 1 and 18 land); discharge item **8**'s residual risk via Step 7. + - **State what "update AC6" means.** AC6 says "**any** async operation" and "proven in the trimmed harness, not inferred". If Step 2a lands, AC6 closes as written. If class-level `[Execute]` is descoped instead, AC6 must be **narrowed in writing** with the shape named and a Deferred Work row created — silently closing it while a documented Design pattern still leaks is the precise failure AC6 exists to prevent. + - Add a Deferred Work row for the **interface-factory leg** (finding A4) — it carries the identical guard-inside-async shape and still points its attribute at `{ImplName}Factory`, so it shares both mechanisms and gets neither fix here. Deliberately **not** taken into scope: it would balloon this plan, and deferred item 19 makes the leg structurally unmeasurable anyway. But the skill's "Interface factory | Yes" row must not ship unqualified — deferring the work is fine, shipping a false claim is not. + +## Acceptance + +- **AC6's third shape closes:** on a publish-trimmed client, an `async [Remote] internal` class-factory operation leaves behind no `[Service]` interface name, no called-member name, and no body literal — measured across read (`Fetch`), write (`Insert`/`Update`/`Delete`), `LocalSave`, and **class-level `[Execute]`** (Step 2a), which is unconditionally async and today has no harness coverage at all. If any of those shapes is descoped, AC6 is narrowed in writing rather than closed over it. +- The sync leg does not regress: `ClassSyncBody_MARKER`, `IServerOnlyRepository`, `DoServerWork`, `ServerOnlyRepository_MARKER`, `ServerOnlyHelper` stay absent. +- Positive controls still pass, so the absences remain falsifiable. +- Full suite green on net9.0 + net10.0, both solutions, plus the harness exiting 0. +- The nine dependent doc anchors are true of shipped behaviour. + +## Verification + +- **Red before green**, per this arc's standing rule and TRIM-006's two never-failing checks. Every flipped gate assertion must be observed failing against the pre-fix artifact — which for this plan is cheap, because HEAD *is* the pre-fix artifact and the probes are already archived. +- **Baseline inherited, not re-derived.** The 2026-08-13 measurement plus the three variants above are the baseline; markers were proven visible by a self-check against the untrimmed assembly. +- **Publish-only.** `dotnet build` / `run` / `test` prove nothing here; `dotnet test` never runs this project. +- **Generated-output drift needs a real diff** — `**/Generated/` is gitignored, so "nothing else changed" must be measured, not asserted. The inert-knob diff above is the pattern to reuse. +- **Zero incremental-cache delta.** No model, builder, or transform change is contemplated. The first draft offered `git diff --name-only -- src/Generator/` as the check, which can never be empty for this plan — `ClassFactoryRenderer.cs` lives there. The checkable claim is that nothing under `src/Generator/Model/`, `src/Generator/Builder/`, or the transform changes, and that `IncrementalCacheTests` stays green untouched. + +## Files + +**Generator:** `ClassFactoryRenderer.cs` — the **five** `Local*` emission sites named by method in Step 2, plus new holder emission. `FactoryAttributes.cs` — XML contract only (Step 3a), no API surface change. Deliberately **not** touched: any model, builder, or transform. + +**Tests:** `AssemblyAttributeEmissionTests.cs` (inverts the `:42` assertion, adds holder coverage), `verify-trimmed.sh` (eight assertions plus four prose blocks), `TrimTestEntity.cs` (two notes retire), plus a new harness target for class-level `[Execute]` (Step 2a). + +**Docs:** the nine body-trimming anchors in `008-doc-anchor-inventory.md`, **plus** the holder anchors in Step 7b — `CLAUDE-DESIGN.md:760,771` and the attribute-target table, `docs/trimming.md:249`, and the skill's two-leg framing. + +## Risks + +- **The wrapper changes the emitted factory's method shape, and the throw escapes further than the first draft said.** `public async Task LocalX(…)` becomes `public Task LocalX(…)` plus a private core. The *signature* is unchanged, so no public API break. **Scope, verified:** only the guard moves — authorization checks, the `cTarget` cast, and every `GetRequiredService` failure stay in the core and still surface as a faulted `Task`. **But** because the local ctor binds the delegate property to the wrapper and the public entry point is itself non-async (`public virtual Task FetchAsync(…) => FetchAsyncProperty(…)`), the synchronous throw escapes through **`I{X}Factory.FetchAsync`**, not merely through `Local*`. Risk is low — the message is asserted nowhere in the suite (deferred item 4) — but "no public API break" understates it, and it belongs in the release notes. +- **Five emission sites, one experiment.** Only the read path was exercised. The write path differs (`cTarget`, different lifecycle helpers), `LocalSave` is `virtual` and is wrapped by `async` publics — a split there needs more care than the read path — and class-level `[Execute]` is unconditionally async with no harness target at all. +- **`NormalizeWhitespace` has no error signal** — malformed emission yields mangled output, not an exception. Assert on exact fragments. +- **Two legs already carry holders with distinct prefixes**; a third must not collide with `NeatooFactoryRegistrar_` or `NeatooEventHandlerRegistrar_`, and a class carrying several factory attributes must not gain a CS0101 on top of its existing CS0111 (deferred item 15). + +## Out of scope + +- Deferred items 5, 9, 14, 15, 19 — each needs its own plan. +- Replacing the reflective registrar lookup with `[ModuleInitializer]` registration, which would delete this defect class entirely. Rejected here for load-order hazards; worth its own plan. +- The v1.7.0 release itself — TRIM-009 unblocks it; cutting it is the arc's close-out step. + +--- + +## Current State (2026-08-14, implemented) + +**The fix, both halves.** `ClassFactoryRenderer` gained one shared helper, `RenderLocalMethodOpening`, applied at all five guarded `Local*` emission sites. For a guarded `async` method it emits a **non-async wrapper** carrying the `IsServerRuntime` guard and forwarding to a `private async …Core`; for everything else it emits exactly what it emitted before. Alongside it, the class-factory assembly attribute now names a generated single-method holder, `NeatooClassFactoryRegistrar_{ClassName}`, instead of `{X}Factory`. + +Both halves are required, and that is measured rather than argued — see the V3 row above for the wrapper alone, and the root inventory for why DAM keeps the private core alive without the holder. + +**What did not change:** the delegate registrations and the local ctor's method-group assignment. Both reference the *wrapper*, whose post-guard call folds away, so the core is de-rooted without touching either. That was the prediction Step 1 was written to test, and it held. + +## Test Evidence + +| Claim | Artifact | Result | +|---|---|---| +| H2 falsified — its constructs are not necessary | `probe-h1h2-v1-async-probes-suppressed.txt` | async minus all four probes and the OCE arm → **still leaked** | +| H2 falsified — its constructs are not sufficient | `probe-h1h2-v2-sync-with-second-catch.txt` | sync plus catch arm and type-test → **still clean** (2 of 5 constructs; the awaiting probes cannot be grafted onto a sync body) | +| Guard relocation alone is insufficient | `probe-h1h2-v3-sync-wrapper-async-core.txt` + its addendum | markers unmoved; `d__` survives. **Cannot** attribute that to DAM vs. the wrapper's surviving call — both roots were live | +| Wrapper + holder clears the read and write legs | `probe-v4-wrapper-plus-holder.txt` | 8 markers flipped to absent; `d__` / `d__` / `d__` / `d__` / `d__` all gone. **Does not cover class-`[Execute]`** — that target postdates this probe | +| …and the class-`[Execute]` leg | `gate-final-passing.log` | all five `Exec*` markers absent | +| The absences are trim results, not build artifacts | `probe-selfcheck-final-all-legs.txt` | every **body** marker PRESENT in the untrimmed build, **including all five class-`[Execute]` markers**. Supersedes `probe-v4-selfcheck-untrimmed.txt`, which covered 12 markers and none from the Exec leg | +| The holder actually forwards (silent-failure check) | `harness-final.log` | every factory resolves, including `Class [Execute] factory resolved: True`. `harness-v4-liveness.log` is the earlier run and stops at the save leg | +| The CI gate passes on the real artifact | `gate-final-passing.log` | exit 0, 10 named positive controls plus the UTF-16 control, no shape asserted PRESENT as a known leak | +| Knob values are recoverable per variant | `knob-values-per-variant.txt` | plan-review finding B6 | +| Apparatus was inert before use | `experiment-knobs.diff` | knobs at default reproduce HEAD's generated tree byte-for-byte | +| Full suite green, both solutions, both TFMs | `test-main-full.log`, `test-design.log` | 614+614 unit, 561+561 integration (5 pre-existing skips), 86+86 Design | + +**Assembly size:** 52,224 bytes post-fix (`probe-v4-wrapper-plus-holder.txt`). No archived artifact records HEAD's *pre-fix* trimmed size — the 66,560 figure quoted in the first draft is **V2's** knob variant, not HEAD, and the comparison has been withdrawn rather than restated from memory. + +**Red before green, proven not asserted — with a stated limit.** With the holder prefix broken and the wrapper split disabled, exactly three tests went red — `ClassFactory_EmitsAssemblyAttribute`, `ClassFactory_EmitsRegistrarHolder_ForwardingToFactory`, `ClassFactory_GuardedAsyncLocalMethod_SplitsIntoSyncWrapperAndAsyncCore` — while the sync-path control `ClassFactory_GuardedSyncLocalMethod_IsNotSplit` stayed green. **That run was filtered to this one test class (16 tests), so it is not a blast-radius statement** about the other 598. xUnit also aborts each test at its first failing assertion, so the later assertions in those tests — including the `DoesNotContain` regression assertion — were not individually observed red. Recorded rather than papered over. + +**Blast radius on existing tests: one**, exactly the inversion plan review predicted at `AssemblyAttributeEmissionTests.cs:42`. Its original intent is preserved — the attribute is still emitted and still names the correct type; the correct type changed. + +**Not covered here:** a dedicated emission assertion for the async guarded `Can*` site. The shape needs `[AspAuthorize]` policy auth, whose references `DiagnosticTestHelper.BuildReferences()` does not carry; an attempt using `[AuthorizeFactory]` produced an *unguarded* async Can and was removed rather than kept. The site is exercised by `Design.Domain.Aggregates.SecureOrder` and `RemoteFactory.AspNetCore.TestLibrary`, both of which emit `LocalCan*Core` and pass. Reason recorded at the test file. + +## What this plan deliberately did not do + +- **The interface-factory leg.** It carries the same inside-the-async guard and still points its attribute at `{ImplName}Factory`, so it shares both mechanisms and received neither fix. Taking it on would have grown a plan whose arc was already flagged as over-running, and deferred item 19 makes the leg structurally unmeasurable from a client-side harness anyway. Deferred as **item 20** — with the published claim corrected from "Interface factory | Yes" to "not established" in both the skill and `CLAUDE-DESIGN.md`, because deferring work is acceptable and shipping a false claim is not. +- **Narrowing the DAM** on `NeatooFactoryRegistrarAttribute`. Rejected at TRIM-008's plan review and still rejected; a prebuilt library with an `internal static` registrar would silently stop registering on a trimmed client. +- **Design-project requirements verification** (Step 7B per `CLAUDE.md`) — deferred to the release step via existing item 11, now stated here rather than left to surface at close-out. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-code-review.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-code-review.md new file mode 100644 index 00000000..bdf8bad9 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-code-review.md @@ -0,0 +1,52 @@ +# TRIM-009 — Code Review (per-plan, opt-in) + +**Gate:** Step 5, opted in (`Code-review opt-in: Yes`). **Pass:** one (2026-08-14). Findings-only; no grade. +**Evidence set:** [`009-evidence/`](./009-evidence/) — manifest in [`009-test-review.md`](./009-test-review.md). + +Every checkable finding was independently re-derived at the keyboard before being accepted. **All of them held.** + +--- + +## Standing conclusions + +**The fix is correct, minimal, and in the right place.** `git diff --name-only main..HEAD -- src/Generator/` returns exactly one file. `RenderLocalMethodOpening` is the correct seam: the guard string now exists at **two** places in the whole renderer instead of five copies, and `IsServerRuntime` appears nowhere else in `ClassFactoryRenderer.cs`. All five guarded sites route through it — including `LocalSave`, whose omission would have passed Step 1 while shipping a guarded async body. + +**No argument transposition**, the risk I most wanted ruled out. `GetParameterDeclarationsWithOptionalCancellationToken` and `GetParameterIdentifiersWithCancellationToken` apply byte-identical `Where` filters and identical `params` reordering, and every call site passes the same flags to both — so the wrapper's forwarding args match the core's signature by construction, not by coincidence. + +**`LocalSave`'s `virtual` is sound** (wrapper keeps it, signature unchanged, overrides still compile and still bypass the guard exactly as before). **`blankLineAfterGuard: false` is faithful** to the prior Save emission. **Holder prefix collision is impossible**, not merely unlikely: the three prefixes diverge at character 7. + +**Zero incremental-cache delta confirmed:** `Model/` and `Builder/` diffs are empty and `IncrementalCacheTests` is untouched. + +**Framework rules clean.** `FactoryAttributes.cs` is XML-doc-only — no API surface change. No reflection added. The one inverted assertion preserves intent and adds a regression assertion on the old target. + +**All four veto-tier findings were in the claims layer, not the fix** — the same distribution as the plan review, and the same place this arc keeps paying. + +## Veto-tier findings + +| # | Finding | Disposition | +|---|---|---| +| V1 | **`docs/trimming.md:236` is now false and contradicts `:249` in the same section.** It still said class factories "have a type to name that is not yours", eleven lines above the sentence this plan edited to say the opposite. Step 7b's list was built from the plan instead of from the file — the exact failure the doc-anchor inventory exists to stop | **Fixed.** Rewritten to cover all three holder legs and name the interface exception | +| V2 | **`docs/trimming.md:37` still asserted the interface leg's mechanism** — "making the server-only code path unreachable to the trimmer" — the single published statement asserting precisely what H1 measured insufficient. Deferred item 20 named this artifact by name as release-blocking, and the fix pass qualified the skill and `CLAUDE-DESIGN.md` but not this | **Fixed.** Qualified to "not established", with the reason (guard inside async is not sufficient; the leg still names `{ImplName}Factory`) | +| V3 | **`CLAUDE-DESIGN.md:760` said "Every factory shape therefore emits its own forwarding holder."** The interface factory does not — `InterfaceFactoryRenderer` emits no holder (grep: zero occurrences). Contradicted by the table 10 lines below, by "the **three** holder rows", and by the carve-out 17 lines below — all written by this plan's own edit | **Fixed.** "Three of the four shapes", with the exception stated where the claim is made | +| V4 | **Class-`[Execute]` had no untrimmed self-check and no pre-fix baseline, while three artifacts stated or implied it did** — the plan's Test Evidence, `verify-trimmed.sh:127` ("Every marker here appears PRESENT in the UNTRIMMED build"), and a `[N]`-labelled block asserting the leg "was subject to the defect in full" as fact | **Fixed.** Untrimmed self-check re-run across all legs; all five Exec markers PRESENT. Both overclaims scoped, and the `[N]` block now distinguishes what is measured from what is read off the emitted source | +| V5 | **Step 8 declared deferred item 8 discharged; the row was byte-identical to `main`** | **Fixed.** Item 8 closed — and, as V1/V2 showed, the residual genuinely had not been discharged when the claim was made, so the stale row was accidentally accurate | + +## Callout-tier + +- **C1 — dead local.** `var asyncKeyword` in `RenderSaveLocalMethod`, unread since the modifier moved into the helper. Not a CS0219 (non-constant initializer), which is why `TreatWarningsAsErrors` missed it. **Deleted.** +- **C2 — no compile assertion on class-factory emission**, while both sibling legs have one, and `FactoryRenderer` swallows render exceptions into a `/* Error: */` comment. **Fixed** — and it failed on its first run (missing fixture usings), so it was not decorative. +- **C3 — the behaviour note overstated one clause.** "Because the public entry point is itself non-async" is true for reads but false for `Save` on an authorized factory, which is `public virtual async` and captures the throw back into a faulted `Task`. Erred conservative, but **fixed** in both docs. +- **C4 — `Local{X}Core` has no collision guard.** A method whose `UniqueName` ends in `Core` can collide with another's generated core: CS0111 in generated code with no diagnostic. No such shape exists in the repo, Design projects, or examples. **Recorded as deferred item 21**, alongside items 14/15. +- **C5 — a declared Verification item has no artifact.** The plan promised a post-fix generated-tree diff; only the pre-fix inertness diff is archived. The reviewer substituted by reading the emitted trees for all five sites plus the interface leg and found only intended changes, so the conclusion holds — but the promised evidence does not exist. **Recorded.** + +## Verified emitted output + +The strongest evidence in the change is `TrimSaveTargetFactory.g.cs`: `LocalInsert`/`LocalUpdate`/`LocalDelete`/`LocalSave` are all non-async wrappers forwarding to `…Core`; sync `LocalCreate` and the five sync `LocalCan*` are correctly **not** split; and `LocalSaveCore` routes to the **wrappers**, which is what makes the de-rooting prediction hold. The holder is a genuine single-method type and `AddRemoteFactoryServices` binds it via `BindingFlags.Static | NonPublic | Public`. + +Only one unintended emission change exists and it is cosmetic: `public Task` → `public Task` (the old `{asyncKeyword}` interpolation left a double space when empty), which `NormalizeWhitespace` collapsed anyway. + +## Verdict + +**The deliverable is done.** The generator change was correct on the first pass and survived scrutiny unchanged; every finding was a claim outrunning its evidence, and all are closed. Two of them — V1 and V3 — were sentences this plan itself wrote while fixing the previous plan's sentences, which is worth naming: the doc surface is now the highest-churn, lowest-verification part of this arc. + +**AC6 closes as written**, with the interface-factory leg carved out in writing rather than claimed. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/experiment-knobs.diff b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/experiment-knobs.diff new file mode 100644 index 00000000..f03ce03e --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/experiment-knobs.diff @@ -0,0 +1,140 @@ +diff --git a/src/Generator/Renderer/ClassFactoryRenderer.cs b/src/Generator/Renderer/ClassFactoryRenderer.cs +index cfea275..e90213f 100644 +--- a/src/Generator/Renderer/ClassFactoryRenderer.cs ++++ b/src/Generator/Renderer/ClassFactoryRenderer.cs +@@ -17,6 +17,19 @@ namespace Neatoo.RemoteFactory.Generator.Renderer; + /// + internal static class ClassFactoryRenderer + { ++ // --------------------------------------------------------------------- ++ // TRIM-009 SEPARATION EXPERIMENT — TEMPORARY. Revert before any commit. ++ // Separates H1 (the feature-switch fold does not propagate through the ++ // async state machine) from H2 (the fold works; unreachable-code ++ // elimination is defeated by the second catch arm and/or the awaiting ++ // lifecycle probes). See plans/009. ++ // V1: SuppressAsyncProbes = true, ForceSecondCatchArm = false ++ // V2: SuppressAsyncProbes = false, ForceSecondCatchArm = true ++ // --------------------------------------------------------------------- ++ private const bool ExpSuppressAsyncProbes = false; ++ private const bool ExpForceSecondCatchArm = false; ++ private const bool ExpSyncWrapperForAsync = true; ++ + /// + /// Renders the complete factory source code for a class/record. + /// +@@ -318,16 +331,39 @@ internal static class ClassFactoryRenderer + // Local method signature excludes services - they're obtained via ServiceProvider inside + var parameters = GetParameterDeclarationsWithOptionalCancellationToken(method.Parameters, includeServices: false); + +- sb.AppendLine($" public {asyncKeyword} {returnType} Local{method.UniqueName}({parameters})"); +- sb.AppendLine(" {"); ++ var isServerOnly = method.IsInternal || method.IsRemote; + +- // Feature switch guard -- only emit for internal or [Remote] methods. +- // Public non-[Remote] methods run on both client and server. +- if (method.IsInternal || method.IsRemote) ++ if (ExpSyncWrapperForAsync && needsAsync && isServerOnly) + { ++ // V3: the guard lives in a NON-async wrapper; the async body moves to a private core. ++ var forwardArgs = string.Join(", ", method.Parameters ++ .Where(p => !p.IsService && !p.IsCancellationToken) ++ .Select(p => p.Name) ++ .Concat(new[] { "cancellationToken" })); ++ ++ sb.AppendLine($" public {returnType} Local{method.UniqueName}({parameters})"); ++ sb.AppendLine(" {"); + sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); + sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); ++ sb.AppendLine($" return Local{method.UniqueName}Core({forwardArgs});"); ++ sb.AppendLine(" }"); + sb.AppendLine(); ++ sb.AppendLine($" private async {returnType} Local{method.UniqueName}Core({parameters})"); ++ sb.AppendLine(" {"); ++ } ++ else ++ { ++ sb.AppendLine($" public {asyncKeyword} {returnType} Local{method.UniqueName}({parameters})"); ++ sb.AppendLine(" {"); ++ ++ // Feature switch guard -- only emit for internal or [Remote] methods. ++ // Public non-[Remote] methods run on both client and server. ++ if (isServerOnly) ++ { ++ sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); ++ sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); ++ sb.AppendLine(); ++ } + } + + // Authorization checks (inside guard -- auth types are server-only) +@@ -518,9 +554,9 @@ internal static class ClassFactoryRenderer + sb.AppendLine(" }"); + + // For write-style async methods: catch OperationCanceledException +- if (isWriteStyleLifecycle && method.IsDomainMethodTask) ++ if (isWriteStyleLifecycle && ((method.IsDomainMethodTask && !ExpSuppressAsyncProbes) || ExpForceSecondCatchArm)) + { +- RenderWriteLifecycleOnCancelled(sb, method, model, resultVar); ++ RenderWriteLifecycleOnCancelled(sb, method, model, resultVar, method.IsDomainMethodTask); + } + + // Catch general exceptions +@@ -641,7 +677,7 @@ internal static class ClassFactoryRenderer + sb.AppendLine(" }"); + + // Async start hook only for async domain methods +- if (isDomainMethodTask) ++ if (isDomainMethodTask && !ExpSuppressAsyncProbes) + { + sb.AppendLine($" if ({targetVar} is IFactoryOnStartAsync _factoryOnStartAsync)"); + sb.AppendLine(" {"); +@@ -663,7 +699,7 @@ internal static class ClassFactoryRenderer + sb.AppendLine(" }"); + + // Async complete hook only for async domain methods +- if (isDomainMethodTask) ++ if (isDomainMethodTask && !ExpSuppressAsyncProbes) + { + sb.AppendLine($" if ({targetVar} is IFactoryOnCompleteAsync _factoryOnCompleteAsync)"); + sb.AppendLine(" {"); +@@ -675,7 +711,7 @@ internal static class ClassFactoryRenderer + /// + /// Renders OperationCanceledException catch block with IFactoryOnCancelled hooks (async Write lifecycle only). + /// +- private static void RenderWriteLifecycleOnCancelled(StringBuilder sb, FactoryMethodModel method, ClassFactoryModel model, string targetVar) ++ private static void RenderWriteLifecycleOnCancelled(StringBuilder sb, FactoryMethodModel method, ClassFactoryModel model, string targetVar, bool isDomainMethodTask) + { + sb.AppendLine(" catch (OperationCanceledException)"); + sb.AppendLine(" {"); +@@ -686,10 +722,15 @@ internal static class ClassFactoryRenderer + EmitLogInvokingOnCancelled(sb, " ", model.ServiceTypeName); + sb.AppendLine($" _factoryOnCancelled.FactoryCancelled(FactoryOperation.{method.Operation});"); + sb.AppendLine(" }"); +- sb.AppendLine($" if ({targetVar} is IFactoryOnCancelledAsync _factoryOnCancelledAsync)"); +- sb.AppendLine(" {"); +- sb.AppendLine($" await _factoryOnCancelledAsync.FactoryCancelledAsync(FactoryOperation.{method.Operation});"); +- sb.AppendLine(" }"); ++ // The awaiting probe cannot be emitted into a sync method body. ++ if (isDomainMethodTask && !ExpSuppressAsyncProbes) ++ { ++ sb.AppendLine($" if ({targetVar} is IFactoryOnCancelledAsync _factoryOnCancelledAsync)"); ++ sb.AppendLine(" {"); ++ sb.AppendLine($" await _factoryOnCancelledAsync.FactoryCancelledAsync(FactoryOperation.{method.Operation});"); ++ sb.AppendLine(" }"); ++ } ++ + sb.AppendLine(" throw;"); + sb.AppendLine(" }"); + } +@@ -941,9 +982,9 @@ internal static class ClassFactoryRenderer + sb.AppendLine(" }"); + + // For async Write methods: catch OperationCanceledException +- if (method.IsDomainMethodTask) ++ if ((method.IsDomainMethodTask && !ExpSuppressAsyncProbes) || ExpForceSecondCatchArm) + { +- RenderWriteLifecycleOnCancelled(sb, method, model, "cTarget"); ++ RenderWriteLifecycleOnCancelled(sb, method, model, "cTarget", method.IsDomainMethodTask); + } + + // Catch general exceptions diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/knob-values-per-variant.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/knob-values-per-variant.txt new file mode 100644 index 00000000..bfdbebb6 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/knob-values-per-variant.txt @@ -0,0 +1,23 @@ +TRIM-009 separation experiment — knob values per variant +Recorded at plan review (finding B6). experiment-knobs.diff captures only ONE +configuration (V3's), so the other two were recoverable by inference only. + +All three constants live in ClassFactoryRenderer, added and reverted for the +experiment. Defaults (all false) were proven to reproduce HEAD's emission +byte-for-byte before any variant was run. + + SuppressAsyncProbes ForceSecondCatchArm SyncWrapperForAsync + inertness check false false (absent) + V1 async minus probes/OCE true false (absent) + V2 sync plus second catch false true (absent) + V3 sync wrapper + async core false false true + +V1 suppresses all four awaiting lifecycle probes AND the OCE catch arm, at the +read path and both write paths. LocalFetchAsync stays async: `needsAsync` is +untouched by the knob and `await target.FetchAsync(...)` remains in the body. + +V2 grafts only 2 of the 5 constructs — the OCE catch arm and the non-awaiting +IFactoryOnCancelled probe. The three awaiting probes cannot be emitted into a +sync method body, so V2 cannot speak to them. The H2 falsification rests on V1. + +V3 wired the READ PATH ONLY. The write, save, and can paths kept HEAD's shape. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-h1h2-v1-async-probes-suppressed.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-h1h2-v1-async-probes-suppressed.txt new file mode 100644 index 00000000..56ebb698 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-h1h2-v1-async-probes-suppressed.txt @@ -0,0 +1,37 @@ +=== V1 — async, probes + OCE catch SUPPRESSED === +dll: src/Tests/RemoteFactory.TrimmingTests/bin/Release/net9.0/win-x64/publish/RemoteFactory.TrimmingTests.dll +size: 63488 bytes + +-- POSITIVE CONTROLS (must all be PRESENT, else the probe read nothing) + TrimTestCommands PRESENT (utf8) + NeatooFactoryRegistrar_TrimTestCommands PRESENT (utf8) + ITrimSaveTargetFactory PRESENT (utf8) + TrimTestEntityFactory PRESENT (utf8) + Trimming verification app completed PRESENT (utf16) + +-- THE CONTROLLED PAIR (class factory, same type, same rooting) + ClassSyncBody_MARKER absent + ClassAsyncBody_MARKER PRESENT (utf16) + IClassLegPort PRESENT (utf8) + ClassLegInvoke PRESENT (utf8) + +-- SAVE/CAN* LEG (async write, two-hop rooting) + ISaveLegPort PRESENT (utf8) + SaveLegInvoke PRESENT (utf8) + SaveLegInsertBody_MARKER PRESENT (utf16) + SaveLegUpdateBody_MARKER PRESENT (utf16) + SaveLegDeleteBody_MARKER PRESENT (utf16) + +-- SYNC-LEG CONTROLS (must stay absent) + IServerOnlyRepository absent + DoServerWork absent + ServerOnlyRepository_MARKER absent + ServerOnlyHelper absent + +-- STATE MACHINES SURVIVING + d__ PRESENT (utf8) + d__ PRESENT (utf8) + d__ PRESENT (utf8) + d__ PRESENT (utf8) + d__ PRESENT (utf8) + d__ absent diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-h1h2-v2-sync-with-second-catch.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-h1h2-v2-sync-with-second-catch.txt new file mode 100644 index 00000000..66e9070e --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-h1h2-v2-sync-with-second-catch.txt @@ -0,0 +1,37 @@ +=== V2 — sync WITH second catch arm + IFactoryOnCancelled probe === +dll: bin/Release/net9.0/win-x64/publish/RemoteFactory.TrimmingTests.dll +size: 66560 bytes + +-- POSITIVE CONTROLS (must all be PRESENT, else the probe read nothing) + TrimTestCommands PRESENT (utf8) + NeatooFactoryRegistrar_TrimTestCommands PRESENT (utf8) + ITrimSaveTargetFactory PRESENT (utf8) + TrimTestEntityFactory PRESENT (utf8) + Trimming verification app completed PRESENT (utf16) + +-- THE CONTROLLED PAIR (class factory, same type, same rooting) + ClassSyncBody_MARKER absent + ClassAsyncBody_MARKER PRESENT (utf16) + IClassLegPort PRESENT (utf8) + ClassLegInvoke PRESENT (utf8) + +-- SAVE/CAN* LEG (async write, two-hop rooting) + ISaveLegPort PRESENT (utf8) + SaveLegInvoke PRESENT (utf8) + SaveLegInsertBody_MARKER PRESENT (utf16) + SaveLegUpdateBody_MARKER PRESENT (utf16) + SaveLegDeleteBody_MARKER PRESENT (utf16) + +-- SYNC-LEG CONTROLS (must stay absent) + IServerOnlyRepository absent + DoServerWork absent + ServerOnlyRepository_MARKER absent + ServerOnlyHelper absent + +-- STATE MACHINES SURVIVING + d__ PRESENT (utf8) + d__ PRESENT (utf8) + d__ PRESENT (utf8) + d__ PRESENT (utf8) + d__ PRESENT (utf8) + d__ absent diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-h1h2-v3-sync-wrapper-async-core.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-h1h2-v3-sync-wrapper-async-core.txt new file mode 100644 index 00000000..4921e39a --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-h1h2-v3-sync-wrapper-async-core.txt @@ -0,0 +1,58 @@ +=== V3 — sync wrapper carries guard, async body in private Core === +dll: bin/Release/net9.0/win-x64/publish/RemoteFactory.TrimmingTests.dll +size: 67584 bytes + +-- POSITIVE CONTROLS (must all be PRESENT, else the probe read nothing) + TrimTestCommands PRESENT (utf8) + NeatooFactoryRegistrar_TrimTestCommands PRESENT (utf8) + ITrimSaveTargetFactory PRESENT (utf8) + TrimTestEntityFactory PRESENT (utf8) + Trimming verification app completed PRESENT (utf16) + +-- THE CONTROLLED PAIR (class factory, same type, same rooting) + ClassSyncBody_MARKER absent + ClassAsyncBody_MARKER PRESENT (utf16) + IClassLegPort PRESENT (utf8) + ClassLegInvoke PRESENT (utf8) + +-- SAVE/CAN* LEG (async write, two-hop rooting) + ISaveLegPort PRESENT (utf8) + SaveLegInvoke PRESENT (utf8) + SaveLegInsertBody_MARKER PRESENT (utf16) + SaveLegUpdateBody_MARKER PRESENT (utf16) + SaveLegDeleteBody_MARKER PRESENT (utf16) + +-- SYNC-LEG CONTROLS (must stay absent) + IServerOnlyRepository absent + DoServerWork absent + ServerOnlyRepository_MARKER absent + ServerOnlyHelper absent + +-- STATE MACHINES SURVIVING + d__ absent + d__ PRESENT (utf8) + d__ PRESENT (utf8) + d__ PRESENT (utf8) + d__ PRESENT (utf8) + d__ absent + +-- ADDENDUM 2026-08-14, added at plan review (finding B2) +-- The probe script above searched 'd__', which CANNOT match +-- 'd__' (the '>' falls after "Async", not after "AsyncCore"). +-- The Core state machine was measured separately in the same run but was not +-- captured in this file. Re-measured against the same on-disk V3 artifact: + d__ PRESENT (utf8) + LocalFetchAsyncCore PRESENT (utf8) + d__ absent +-- +-- READ THE THIRD ROW CORRECTLY. 'd__' is absent from V3 because +-- the V3 knob emits LocalFetchAsync as a NON-async wrapper, so the compiler never +-- creates a state machine by that name. Its absence is a compile-time consequence +-- of the rename, not a trimming result, and it is NOT evidence that the fold works +-- in the wrapper. This row could not have gone red. +-- +-- V3 therefore cannot attribute the Core's survival: BOTH candidate roots were live +-- in V3 -- DAM on the factory type, and the wrapper's own `return LocalFetchAsyncCore(...)` +-- call site. "DAM roots the core" and "the wrapper's call survived the fold" predict +-- the same observation here, and imply different remedies. Only TRIM-009 Step 1 +-- separates them. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-selfcheck-final-all-legs.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-selfcheck-final-all-legs.txt new file mode 100644 index 00000000..108984f5 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-selfcheck-final-all-legs.txt @@ -0,0 +1,60 @@ +=== UNTRIMMED self-check — post-fix source, ALL legs incl. class-[Execute] === +dll: bin/Release/net9.0/RemoteFactory.TrimmingTests.dll +size: 89600 bytes + +-- POSITIVE CONTROLS (must all be PRESENT, else the probe read nothing) + TrimTestCommands PRESENT (utf8) + NeatooFactoryRegistrar_TrimTestCommands PRESENT (utf8) + ITrimSaveTargetFactory PRESENT (utf8) + TrimTestEntityFactory PRESENT (utf8) + Trimming verification app completed PRESENT (utf16) + NeatooClassFactoryRegistrar_TrimTestEntity PRESENT (utf8) + NeatooClassFactoryRegistrar_TrimSaveTarget PRESENT (utf8) + +-- THE CONTROLLED PAIR (class factory, same type, same rooting) + ClassSyncBody_MARKER PRESENT (utf16) + ClassAsyncBody_MARKER PRESENT (utf16) + IClassLegPort PRESENT (utf8) + ClassLegInvoke PRESENT (utf8) + +-- SAVE/CAN* LEG (async write, two-hop rooting) + ISaveLegPort PRESENT (utf8) + SaveLegInvoke PRESENT (utf8) + SaveLegInsertBody_MARKER PRESENT (utf16) + SaveLegUpdateBody_MARKER PRESENT (utf16) + SaveLegDeleteBody_MARKER PRESENT (utf16) + +-- CLASS-LEVEL [Execute] LEG (TRIM-009 plan-review A3) + IExecLegPort PRESENT (utf8) + ExecLegInvoke PRESENT (utf8) + ExecLegBackend PRESENT (utf8) + ExecLegBackend_MARKER PRESENT (utf16) + ClassExecBody_MARKER PRESENT (utf16) + +-- SYNC-LEG CONTROLS (must stay absent) + IServerOnlyRepository PRESENT (utf8) + DoServerWork PRESENT (utf8) + ServerOnlyRepository_MARKER PRESENT (utf16) + ServerOnlyHelper PRESENT (utf8) + +-- ASYNC STATE MACHINES (per-site wrapper discriminators) + d__ absent + d__ PRESENT (utf8) + d__ PRESENT (utf8) + +-- STATE MACHINES SURVIVING + d__ absent + d__ absent + d__ absent + d__ absent + d__ absent + d__ absent + +-- NOTE ON THE FIRST ATTEMPT AT THIS FILE (2026-08-14, gate round) +-- The first run of this self-check reported all five class-[Execute] markers ABSENT. +-- That was a STALE ARTIFACT, not a result: `dotnet publish -r win-x64` writes to +-- bin/Release/net9.0/win-x64/, so bin/Release/net9.0/ still held a build that predated +-- ClassExecuteLegTarget.cs (81,920 bytes vs 89,600 here). Rebuilt and re-probed. +-- Recorded because "markers absent from the untrimmed build" is indistinguishable at a +-- glance from "the gate's absence checks are vacuous", which is exactly the finding this +-- file exists to answer. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-v4-selfcheck-untrimmed.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-v4-selfcheck-untrimmed.txt new file mode 100644 index 00000000..66d3cb85 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-v4-selfcheck-untrimmed.txt @@ -0,0 +1,34 @@ +=== V4 SELF-CHECK — UNTRIMMED build, same source === +dll: bin/Release/net9.0/RemoteFactory.TrimmingTests.dll +size: 81920 bytes + +-- POSITIVE CONTROLS (must all be PRESENT, else the probe read nothing) + TrimTestCommands PRESENT (utf8) + NeatooFactoryRegistrar_TrimTestCommands PRESENT (utf8) + ITrimSaveTargetFactory PRESENT (utf8) + TrimTestEntityFactory PRESENT (utf8) + Trimming verification app completed PRESENT (utf16) + NeatooClassFactoryRegistrar_TrimTestEntity PRESENT (utf8) + NeatooClassFactoryRegistrar_TrimSaveTarget PRESENT (utf8) + +-- THE CONTROLLED PAIR (class factory, same type, same rooting) + ClassSyncBody_MARKER PRESENT (utf16) + ClassAsyncBody_MARKER PRESENT (utf16) + IClassLegPort PRESENT (utf8) + ClassLegInvoke PRESENT (utf8) + +-- SAVE/CAN* LEG (async write, two-hop rooting) + ISaveLegPort PRESENT (utf8) + SaveLegInvoke PRESENT (utf8) + SaveLegInsertBody_MARKER PRESENT (utf16) + SaveLegUpdateBody_MARKER PRESENT (utf16) + SaveLegDeleteBody_MARKER PRESENT (utf16) + +-- SYNC-LEG CONTROLS (must stay absent) + IServerOnlyRepository PRESENT (utf8) + DoServerWork PRESENT (utf8) + ServerOnlyRepository_MARKER PRESENT (utf16) + +-- SUPERSEDED by probe-selfcheck-final-all-legs.txt (2026-08-14, code review V4). +-- Kept because the plan cites it by name: it covers 12 markers and NONE from the +-- class-[Execute] leg, which is why the "every marker" claim built on it was false. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-v4-wrapper-plus-holder.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-v4-wrapper-plus-holder.txt new file mode 100644 index 00000000..d8983930 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-evidence/probe-v4-wrapper-plus-holder.txt @@ -0,0 +1,39 @@ +=== V4 — sync wrapper (all 5 sites) + holder indirection === +dll: bin/Release/net9.0/win-x64/publish/RemoteFactory.TrimmingTests.dll +size: 52224 bytes + +-- POSITIVE CONTROLS (must all be PRESENT, else the probe read nothing) + TrimTestCommands PRESENT (utf8) + NeatooFactoryRegistrar_TrimTestCommands PRESENT (utf8) + ITrimSaveTargetFactory PRESENT (utf8) + TrimTestEntityFactory PRESENT (utf8) + Trimming verification app completed PRESENT (utf16) + NeatooClassFactoryRegistrar_TrimTestEntity PRESENT (utf8) + NeatooClassFactoryRegistrar_TrimSaveTarget PRESENT (utf8) + +-- THE CONTROLLED PAIR (class factory, same type, same rooting) + ClassSyncBody_MARKER absent + ClassAsyncBody_MARKER absent + IClassLegPort absent + ClassLegInvoke absent + +-- SAVE/CAN* LEG (async write, two-hop rooting) + ISaveLegPort absent + SaveLegInvoke absent + SaveLegInsertBody_MARKER absent + SaveLegUpdateBody_MARKER absent + SaveLegDeleteBody_MARKER absent + +-- SYNC-LEG CONTROLS (must stay absent) + IServerOnlyRepository absent + DoServerWork absent + ServerOnlyRepository_MARKER absent + ServerOnlyHelper absent + +-- STATE MACHINES SURVIVING + d__ absent + d__ absent + d__ absent + d__ absent + d__ absent + d__ absent diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-plan-review.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-plan-review.md new file mode 100644 index 00000000..735c156a --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-plan-review.md @@ -0,0 +1,61 @@ +# TRIM-009 — Plan Review (opt-in, pre-implementation) + +**Gate:** Step 2, opted in (`Plan-review opt-in: Yes`). **Pass:** one (2026-08-14). **Verdict: CONCERNS** — 8 veto-tier, 8 callout-tier. +**Reviewed at:** plan HEAD `da90dca`, before any implementation. + +Every checkable finding was independently re-derived at the keyboard before being accepted. **All of them held.** + +--- + +## Why this review earned its keep + +The plan was drafted *after* running a measurement specifically designed to avoid this arc's recurring failure — building on a diagnosis that was never falsifiable. **The review found the same failure inside the plan written to prevent it**, twice, plus two inventories declared exhaustive that were not. + +That is the fourth occurrence of `[[trim-arc-verify-dont-inherit]]` in this arc, and the first one caught before implementation rather than after. + +## Veto-tier findings + +| # | Pass | Finding | Verified how | Disposition | +|---|---|---|---|---| +| **B1** | B | **V3's "half held" was a check that could not go red.** The plan read `d__` being absent in V3 as "the fold works once the guard is outside the state machine". The V3 knob emits `LocalFetchAsync` as a **non-async wrapper**, so the compiler never creates a state machine by that name — its absence is a compile-time consequence of the rename, trimmed or not | Re-read the V3 emission: `public Task LocalFetchAsync(...)`, no `async` | **Fixed.** Row struck from the table with the reason recorded; the Approach no longer claims the wrapper half was "measured in isolation" | +| **B2** | B | **An unmeasured value stated inside a measurement table, and an unavailable causal attribution.** `d__` was cited from the table, but the probe searched `d__`, which cannot match it (`>` falls after `Async`, not `AsyncCore`). Separately: V3 had **both** candidate roots live — DAM *and* the wrapper's own call — so it cannot attribute the core's survival to DAM, and the two imply different remedies | The value *was* really measured, by a separate grep in the same run that never reached the archived file. Re-measured against the still-on-disk V3 artifact: PRESENT | **Fixed.** Evidence addendum records the re-measurement *and* why the row could not go red; the plan now says V3 cannot attribute survival and hands that to Step 1 | +| **B3** | B | **Root inventory said "two roots", called itself empirical, and missed a third** — the local ctor's `{UniqueName}Property = Local{UniqueName};` method-group assignment, reachable via `AddScoped<{X}Factory>()` and its `DynamicallyAccessedMembers(PublicConstructors)` | Read the emitted ctor: `FetchAsyncProperty = LocalFetchAsync;` | **Fixed.** Three roots. Note that it targets the *wrapper*, so the prediction is unaffected — recorded so the finding is not over-corrected | +| **B4** | B | **Emission-site inventory wrong (three, actually five) and its rationale factually inverted.** The plan said the Save/Can\* leg is reached by the write and `Can*` sites. Measured: `Can*` methods are **synchronous**; `LocalSave` is `public virtual async` and is its own site. Class-level `[Execute]` is a fifth | `grep` for guarded sites → 311/745/804/1034/1310; emitted Save factory shows `public Authorized LocalCanCreate` vs `public virtual async ... LocalSave` | **Fixed.** Sites named by method, not line number. Recorded that leaving `LocalSave` unwrapped would likely still pass Step 1 **while shipping a guarded async body** — invisible to the gate | +| **A3** | A | **Class-level `[Execute]` is unconditionally `async`, guarded, resolves `[Service]`s in the generated body — and has no harness target.** It is a Design source-of-truth pattern. AC6 demands "proven in the trimmed harness, not inferred", which is unsatisfiable for this shape today | Read `RenderClassExecuteLocalMethod`: `public async` emitted with no condition | **Fixed.** New Step 2a takes it in scope with a harness target; Acceptance names it | +| **A1** | A | **The plan declared `FactoryAttributes.cs` untouched while falsifying the contract documented there.** Its remarks say the `Type` "must be a GENERATED registrar type. Never a consumer's own class" — but the class leg *already* named a generated type and still leaked, which the plan itself argues. Shipping that unchanged means shipping a false contract in the remarks written to stop this defect recurring | Read `FactoryAttributes.cs:191-212` against the plan's own line 50 | **Fixed.** New Step 3a; XML-doc only, no API surface change | +| **A2** | A | **Step 7's doc scope covered only body-trimming anchors and missed every anchor the *holder* half falsifies** — six of them, all written by TRIM-008. Most direct: `CLAUDE-DESIGN.md:760` and `docs/trimming.md:249` state that for class factories protection comes from the guard, "**not the choice of attribute target**" — precisely what TRIM-009 reverses | `grep` returned both sentences verbatim | **Fixed.** Step 7 split into 7a (body anchors) and 7b (holder anchors) | +| **A3/8** | A | **Step 8 said "update AC6" without saying what the update is** | — | **Fixed.** AC6 closes as written if Step 2a lands; otherwise it is narrowed *in writing* with the shape named and a Deferred Work row — never closed over an unmeasured shape | + +## Callout-tier — all fixed or recorded + +**B5** — "H2 falsified in both directions" overstated V2: it grafted **2 of 5** constructs, since the three *awaiting* probes cannot be emitted into a sync body. The falsification rests on V1; V2 is partial corroboration. Corrected in the plan and in the evidence. + +**B6** — `experiment-knobs.diff` captured only V3's constant values. Added `knob-values-per-variant.txt`. + +**B7** — Step 1's stop condition needed a **liveness** check: `method?.Invoke` fails silently, so a holder that does not forward makes *every* marker vanish and V4 read as flawless. Step 1 now requires a named positive control for the holder plus the harness resolving the factory and exiting 0. + +**B8** — Exception timing is narrower than feared in one direction and wider in another. Only the guard moves (auth, casts, and DI failures stay in the core, still faulted `Task`s) — but because the ctor binds the delegate property to the wrapper and the public entry point is non-async, the synchronous throw escapes through `I{X}Factory`, not just `Local*`. Risk low (message asserted nowhere), but recorded. + +**B9** — Step 4 is an **inversion** of the passing `AssemblyAttributeEmissionTests.cs:42` assertion, not new coverage. Reworded, with original intent stated as preserved. + +**B10** — Step 5's flip is larger than eight assertions: four prose blocks in `verify-trimmed.sh` become false too, plus a second stale paragraph in `TrimTestEntity.cs`. + +**B11** — The incremental-cache check was self-defeating (`git diff -- src/Generator/` can never be empty when `ClassFactoryRenderer.cs` is the file being changed). Rescoped to `Model/`, `Builder/`, transform. + +**B12** — Step 8 gained deferred items 2 and 8, plus the new row 20. + +**A4** — The **interface-factory leg** shares both mechanisms and receives neither fix. Deliberately **not** taken into scope — it would balloon a plan whose arc the user has already flagged as over-running, and deferred item 19 makes the leg structurally unmeasurable. Recorded as **deferred row 20**, with the release-blocking condition that the skill's "Interface factory | Yes" claim be qualified before shipping. Deferring the work is fine; shipping a false claim is not. + +**A5** — Design-project requirements verification (Step 7B per `CLAUDE.md`) is deferred to the release step via existing item 11; now stated in the plan rather than left to surface at close-out. + +## What the reviewer confirmed as sound + +- **H1 is correct and V1 is a sound subtractive test.** The knob suppresses all five constructs, `LocalFetchAsync` stays genuinely `async`, and the marker still reads PRESENT. +- **V2 was not vacuous** — `TrimTestEntity.Create` is an instance method, so `isWriteStyleLifecycle` is true and the forced catch arm really fired. +- **Holder indirection transfers to the class leg**, and is *easier* than TRIM-008's: the CS0122 problem that forced forwarding over hosting does not exist here, since the class registrar is already `public static` on a generated type. Prebuilt-consumer compatibility transfers unchanged. +- **The central prediction survives independent check** — the newly-found ctor root targets the wrapper, so it does not threaten "the registrations need no guarding". +- **Scope discipline:** the eight steps are the right size and none is padding. The findings grow the *claims* work — docs, contract, AC6 disposition — not the fix. + +## Verdict + +**CONCERNS, all veto-tier findings closed in the plan before implementation.** The plan is stronger for having had two of its own claims struck: it no longer asserts that the wrapper half was measured working, and it no longer attributes the core's survival to a mechanism the data cannot isolate. Both questions now belong to Step 1, which is where they were always answerable. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-test-review.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-test-review.md new file mode 100644 index 00000000..20882cd3 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/009-test-review.md @@ -0,0 +1,52 @@ +# TRIM-009 — Test Review + +**Gate:** mandatory, Step 5. **Pass:** one (2026-08-14). +**Evidence set:** [`009-evidence/`](./009-evidence/). + +Every checkable finding was independently re-derived at the keyboard before being accepted. **All of them held**, and re-deriving one of them turned up a defect the review had not seen. + +--- + +## What the reviewer confirmed as sound + +- **The 8 flipped gate assertions lost nothing.** Verified by set-diff against `2e50546:verify-trimmed.sh`: every pre-TRIM-009 marker is still asserted, with five new Exec markers and three new named controls on top. **TRIM-008's marker-drop regression did not recur** — that was the specific failure this check existed to catch. +- **The class-`[Execute]` leg is genuinely measurable**, not another structurally-blind leg like the interface factory: its wrapper is rooted by an unguarded delegate registration and a ctor method-group assignment, and the marker-bearing body is reached by a **direct static call**, not an interface hop. +- **The negative-lookahead regex is sound**, not backtracking-vacuous — the inner `\s*` absorbs exactly what the outer gives back, so no backtrack position satisfies the lookahead while the guard is present. +- **Sacred tests:** the one inversion preserves intent and *strengthens* it (a new `DoesNotContain` on the old target). `TrimTestEntity.cs` changes are comment-only. No other pre-existing test file modified. + +## must-cover findings + +| # | Finding | Disposition | +|---|---|---| +| M1 | **The durable gate could not detect a per-site wrapper regression.** Every marker was a *body* signal, and body signals cannot distinguish "this method was wrapped" from "an ancestor's fold removed the only reference to it". `LocalSaveCore` routes to the Insert/Update/Delete **wrappers**, so unwrapping `RenderSaveLocalMethod` would still clear every marker while shipping a guarded async body. **The plan documented this blind spot and then shipped one** | **Fixed.** New per-site block asserting `d__`, `d__`, `d__`, `d__`, `d__`, `d__` absent. A wrapped site has no `d__` at all; unwrap it and the name returns *and* its body survives. Four have archived pre-fix PRESENT baselines, so they are real `[D]` discriminators | +| M2 | **Class-`[Execute]` — the shape rescued so AC6 could close "proven, not inferred" — had no untrimmed self-check.** Its five markers, including a UTF-16 literal (the marker class that read false-absent during TRIM-008's probe bug), had never been shown capable of reading PRESENT. The single measurement was one post-fix absence | **Fixed, and the fix found more than the finding claimed.** See below | + +### M2's re-derivation turned up a second defect + +Re-running the self-check reported **all five Exec markers absent from the untrimmed build** — which would have meant the gate's Exec absence checks were outright vacuous. They were not: `dotnet publish -r win-x64` writes to `bin/Release/net9.0/win-x64/`, so `bin/Release/net9.0/` still held a build predating `ClassExecuteLegTarget.cs` (81,920 bytes vs 89,600 after rebuild). **A stale artifact, indistinguishable at a glance from a vacuous gate.** + +Rebuilt and re-probed: all five PRESENT untrimmed, `ClassExecBody_MARKER` among them. The gate's Exec checks are non-vacuous. Both the result and the stale-artifact trap are recorded in `probe-selfcheck-final-all-legs.txt`, because next time the first reading will look the same. + +## should-cover findings + +| # | Finding | Disposition | +|---|---|---| +| S1 | The async guarded `Can*` site — one of the five changed — has neither an emission assertion nor a trimmed measurement (every harness `Can*` is synchronous) | **Attempted, removed, reason recorded.** An `[AuthorizeFactory]` returning `Task` produces a Can that is async but **not server-only**, so no guard and no split — the test would have passed for the wrong reason. The guarded async `Can*` shape needs `[AspAuthorize]` policy auth, whose references `DiagnosticTestHelper.BuildReferences()` lacks. Site is exercised by `Design.Domain.Aggregates.SecureOrder` and `RemoteFactory.AspNetCore.TestLibrary` (both emit `LocalCan*Core`, both pass). Recorded at the test file rather than left silent | +| S2 | **Three of four assertions in the async-split test were never observed red** — xUnit aborts at first failure, so the forwarding, core-signature, and no-guard assertions never executed in the broken state. Same for the `DoesNotContain` regression assertion | **Recorded, not fixed.** Stated as a limit in the plan's Test Evidence rather than left as an implied per-assertion red proof | +| S3 | No compile assertion on class-factory generated output, while the static and relay legs both have one | **Fixed.** `Assert.Empty(GetDiagnostics().Where(Error))` added to the holder test — which immediately failed on missing `using System;` / `System.Threading` in the fixture, so the assertion earned its place on the first run | +| S4 | The behaviour change has zero coverage, and deferred item 4's stated trigger ("if the guard's message or shape is ever edited") has now fired | **Recorded.** No sacred test broke, because nothing anywhere asserts the guard's message. The uncovered surface is the exception *contract*; the trimmed gate covers guard *deletion* | +| S5 | Test Evidence overstatements — Exec markers, "every marker", a mis-cited harness log, and a size baseline | **Fixed.** See below | + +### Test Evidence corrections + +- **"every marker PRESENT untrimmed"** cited an artifact covering 12 markers, none from the Exec leg. Now cites `probe-selfcheck-final-all-legs.txt`, which covers all legs. +- **`harness-v4-liveness.log`** was cited for `Class [Execute] factory resolved: True`; that line exists only in `harness-final.log`. Citation corrected. +- **"52,224 vs 66,560 bytes"** — 66,560 is **V2's knob variant**, not HEAD. No artifact records HEAD's pre-fix trimmed size, so the comparison is **withdrawn** rather than restated from memory. +- **"exactly three tests went red"** came from a run filtered to one test class (16 tests). Now stated as such, not as a blast-radius claim. +- Two Acceptance bullets had no rows; the full-suite row was added, and the "async `Can*`" and doc-anchor gaps are stated explicitly. + +## Pre-existing tech debt, unchanged by this plan + +- **Deferred item 5** — the `IndexOf`-sliced assertions in `CanMethodVisibilityTests` got *wider*, because the wrapper interposes a private core inside the slice. Still queued and unowned; this is a fresh reason to schedule it. +- **The `IsServerRuntime == false` path is untestable in-process** — no fixture sets the AppContext switch, so the guard's negative branch has never executed in any test. Root cause of item 4 and of S4. +- **`verify-trimmed.sh` names `SaveLegBackend` in a comment listing markers "below" that is asserted nowhere.** Confirmed pre-existing (absent from `2e50546` too). diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md index 95056bd7..87f7ea9b 100644 --- a/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md @@ -26,7 +26,11 @@ A third suspected gap turned out to be already fixed: event records derive `Fact 3. Verified (not assumed): a `FactoryEventBase`-derived record whose only client-side reference is a subscription-lambda call site deserializes on a publish-trimmed client. [TRIM-003] 4. `docs/trimming.md` ("What Qualifies as a DTO", "DTO Return Type Preservation") updated to match the shipped behavior; release notes per CI/CD standards. 5. Consumer proof: released version consumed by zTreatment (PCB-003) with the LinkerConfig bulk-preserve block deleted and a Release WASM publish verified. (Tracked zTreatment-side; this todo closes on the framework release, not the consumer rollout.) -6. `[Remote]` method bodies and their server-only dependencies are absent from a publish-trimmed client for **every** factory shape — `[Execute]` static factories, `[FactoryEventHandler]` classes, and class factories with **any** async operation — read or write; measured on both — all three of which retain them today — proven in the trimmed harness, not inferred. [TRIM-008 + TRIM-009] +6. `[Remote]` method bodies and their server-only dependencies are absent from a publish-trimmed client for **every** factory shape that can be measured — `[Execute]` static factories, `[FactoryEventHandler]` classes, class factories with **any** async operation (read *and* write, both measured), `LocalSave` routing, and class-level `[Execute]` — proven in the trimmed harness, not inferred. [TRIM-008 + TRIM-009] + + **Closed as written 2026-08-14**, not narrowed. Class-level `[Execute]` was added to the criterion at TRIM-009's plan review rather than closed over: it is emitted `async` unconditionally, is a Design source-of-truth pattern, and had no harness coverage at all — so AC6's "proven, not inferred" was unsatisfiable for it. It now has a target and is measured absent. + + **Explicit carve-out — the interface-factory leg.** It reaches its implementation through interfaces, so a client-side trimmed test reads "absent" whether or not the body survives. No leak is observed, but no proof is available either, and the leg still points its attribute at `{ImplName}Factory`. AC6 does **not** claim it. Tracked as deferred item 20; the skill and `CLAUDE-DESIGN.md` now say "not established" rather than "Yes". Deferring the work is acceptable; shipping a false claim is not. **AC6 added 2026-08-12.** It is not scope creep onto the original goal: AC1–AC3 are about *preservation* (making types survive trimming), while AC6 is about *over-preservation* (stopping code from surviving that should not). They are opposite failure modes of the same mechanism, and the registrar-DAM defect was found by this arc, blocks its release, and falsifies the documentation AC4 requires be accurate. Fixing it elsewhere would have left the arc unable to close honestly. @@ -53,7 +57,7 @@ A third suspected gap turned out to be already fixed: event records derive `Fact | 005 | Abandoned | [Server-only reference over-retention in trimmed clients](./plans/005-server-only-reference-over-retention.md) | TRIM-004 discovery: guarded-dead `LocalCreate` bodies retain server-only interface refs, contradicting `docs/trimming.md` — **diagnosis falsified at plan review**, see 2026-08-11 log entry | | 006 | Done | [Incremental-generator caching regression test](./plans/006-incremental-cache-regression-test.md) | TRIM-001 gate: no test asserts cached pipeline steps — non-EquatableArray transform fields regress silently (plan review B1) | | 008 | Done | [Registrar-DAM over-preservation fix](./plans/008-registrar-dam-over-preservation.md) | TRIM-005 plan review: `[Execute]` and `[FactoryEventHandler]` registrar attributes name the consumer's class, so DAM retains every method incl. `[Remote]` bodies — release-blocking, falsifies AC4's docs. Folded into the arc 2026-08-12 (reverses the 2026-08-11 plan-mode routing) | -| 009 | Stub | [Async `Local*` factory-method body retention](./plans/009-async-local-method-body-retention.md) | TRIM-008 pre-fix probe (2026-08-13): async generated `Local*` methods keep their server-only bodies on a trimmed client; sync ones in the same assembly do not. Falsifies the **class-factory** leg — the shape every doc presents as safe. Distinct mechanism from 008, so it gets its own plan (user decision 2026-08-13) | +| 009 | Done | [Async `Local*` factory-method body retention](./plans/009-async-local-method-body-retention.md) | TRIM-008 pre-fix probe (2026-08-13): async generated `Local*` methods keep their server-only bodies on a trimmed client; sync ones in the same assembly do not. Falsifies the **class-factory** leg — the shape every doc presents as safe. Distinct mechanism from 008, so it gets its own plan (user decision 2026-08-13) | Execution order: 004 → 001 → 002 → 003 → 007 → 005 → 006 → 008 → 009 (rows listed in execution order; numbering stays monotonic by creation). Branching: todo/plan docs commit on the `TRIM` branch; each plan's implementation gets its own branch off `TRIM`. (TRIM-003's red verification and TRIM-007's fix merged together via PR #71.) @@ -79,13 +83,13 @@ AC1–AC3 confirmed genuinely verified in a publish-trimmed artifact at HEAD (CI | # | Item | Destination | Cost if it stays open | |---|---|---|---| | 1 | **Registrar-DAM over-preservation** — `[Remote]` bodies for `[Execute]` and `[FactoryEventHandler]` classes ship to the browser decompilable | **[TRIM-008]** — folded into the arc 2026-08-12, reversing the plan-mode routing. Closes on that plan | Resolved: it now has the durable home the audit said it needed | -| 2 | **Release held (AC4 + AC5)** — version stays `1.6.1`, no v1.7.0 notes | Reopens when items 1 **and 18** merge — widened 2026-08-13 when the probe found a third broken shape | zTreatment PCB-003 blocked since July, and the hold just got longer. Deliberate trade, re-affirmed at the keyboard: consumer unblock-time vs. publishing false IP guidance about the *most common* factory shape | +| 2 | **Release held (AC4 + AC5)** — version stays `1.6.1`, no v1.7.0 notes | **Both blockers cleared 2026-08-14** — item 1 merged as PR #75, item 18 closed by TRIM-009. The release is unblocked pending TRIM-009's own merge and the arc's close-out audit | zTreatment PCB-003 blocked since July. The trade was re-affirmed twice at the keyboard: consumer unblock-time vs. publishing false IP guidance about the *most common* factory shape. AC6 is now satisfiable as written rather than by narrowing | | 3 | **`DiagnosticTestHelper` stale-generator hazard** — a generator fix can appear verified when it was never loaded; affects the whole generator suite | Documented at the seam (`DiagnosticTestHelper.cs`); durable fix (fail fast when the generator DLL predates the test assembly) explicitly not done | Local-iteration only (CI is cold-build). Already produced one false green during TRIM-006 | | 4 | **B8 — nothing pins the guard's runtime throw.** No `AppContext.SetSwitch` anywhere; `"Server-only method called in non-server runtime."` never asserted | **Accepted with reason:** pre-existing, not introduced by this arc, and the trimmed-harness CI gate covers the property that actually matters (server-only types absent from the trimmed artifact). Queue if the guard's message or shape is ever edited | A regression deleting the throw ships silently in untrimmed/server scenarios | | 5 | **B10 — 16 emission assertions can pass vacuously.** `InternalVisibilityTests` / `CanMethodVisibilityTests` slice generated text with naive `IndexOf` bounded by the next member name | **Queued, unowned.** Not fixed here: out of TRIM-006's scope, and rewriting 16 assertions in sacred tests needs its own plan with its own review | False-green on the generated-code visibility contract — the same class of defect TRIM-001's test gate caught as its marquee finding | | 6 | **B9 — harness cannot verify the relay-handler leg** (no relay-handler target touches a server-only service) | **CLOSED by TRIM-008** (2026-08-13). Closed in full, not just for the relay leg: relay, interface-factory, and Save/Can\* targets all added with per-leg server-only ports, all probed pre-fix, all in the CI gate. The interface leg's long-standing "structurally safe" claim is now a measurement, and probing the Save/Can\* leg is what surfaced item 18 | Resolved. Had it stayed open, TRIM-008 would have shipped fixed-but-unverified — and item 18 would still be undiscovered | | 7 | **Falsified TRIM-005 story in live artifacts** — `.github/workflows/build.yml:111-112`, `TrimmingTests/README.md:31`, `TrimTestCommands.cs:35`. The CI grep's `(?]` emits duplicate registrars (CS0111)** | **Recorded, not fixed** (same decision) | Both renderers re-open the same partial and each emits `FactoryServiceRegistrar`. Untested shape; broken at HEAD, not by TRIM-008. TRIM-008 uses distinct per-leg holder prefixes so it does not *add* a CS0101 on top | | 16 | **Narrowing the registrar attribute's DAM** to `PublicMethods` alone | **Rejected, not deferred.** `DynamicallyAccessedMemberTypes` has no sub-method granularity, so no narrowing keeps `FactoryServiceRegistrar` rooted while dropping siblings — the holder indirection is the only mechanism that shrinks the blast radius. Additionally it would silently unroot registrars in prebuilt libraries compiled by an older generator | None — this is a closed question, recorded so it is not re-opened speculatively | | 17 | **Replace the reflective `GetMethod` lookup with `[ModuleInitializer]` registration** — would delete this entire defect class rather than one instance | **Queued, unowned.** Needs its own plan | Module initializers fire on first module access, which does not reliably precede `RegisterFactories` enumerating a caller-supplied assembly list — trades a visible over-retention bug for an intermittent missing-registration one. Also reshapes `AddNeatooAspNetCore`'s assembly semantics | -| 18 | **`async` generated `Local*` factory methods retain their server-only bodies under trimming** — sync ones in the same class do not. **Confirmed by controlled experiment 2026-08-13** (`TrimTestEntity.Create` sync vs `FetchAsync` async: same type, same factory, no auth on either, one-hop rooting on both, direct concrete call on both → sync marker absent, async marker present). The earlier cross-class pair was NOT single-variable and is superseded; see the TRIM-009 stub for the table and for what remains unestablished | **[TRIM-009]** — folded into the arc 2026-08-13 as its own plan (user decision). AC6 held whole rather than narrowed; the release now waits for this too | Resolved: it has a durable home. Left unrouted, the flagship IP-protection claim would stay false for every aggregate root with async operations (read or write) — most of them — while a release closing AC4 declared the docs accurate | +| 18 | **`async` generated `Local*` factory methods retain their server-only bodies under trimming** — sync ones in the same class do not. **Confirmed by controlled experiment 2026-08-13** (`TrimTestEntity.Create` sync vs `FetchAsync` async: same type, same factory, no auth on either, one-hop rooting on both, direct concrete call on both → sync marker absent, async marker present). The earlier cross-class pair was NOT single-variable and is superseded; see the TRIM-009 stub for the table and for what remains unestablished | **CLOSED 2026-08-14 by TRIM-009.** Cause separated from inside the generator: the async **state machine** is the mechanism (V1 removed every lifecycle probe and the OCE catch arm and it still leaked; V2 added them to the sync path and it stayed clean). Fixed by a non-async guard wrapper **plus** a single-method registrar holder — neither half suffices, since DAM covers `NonPublicMethods` and roots the private core on its own. Measured absent across read, write, `LocalSave`, and class-level `[Execute]` | Resolved. Left unrouted, the flagship IP-protection claim would have stayed false for every aggregate root with async operations (read or write) — most of them — while a release closing AC4 declared the docs accurate | | 19 | **`[Service]` parameters on interface-factory methods emit uncompilable code (CS0535)** — the generator strips the service parameter from the proxy's implementing method while the `[Factory]` interface still declares it, so the emitted factory does not implement its own interface | **Recorded, not fixed.** Found 2026-08-13 during TRIM-008's re-review while trying to give the async interface-factory target a directly-reachable marker. Pre-existing; nothing in the repo, tests, or Design projects uses the shape, which is why it was never caught | Rare shape, but the failure is a CS error in *generated* code with no diagnostic pointing at the cause. It also means the interface-factory leg **cannot** carry a server-only marker in its generated body, so that leg is structurally unable to measure body-fold behaviour | +| 20 | **The interface-factory leg shares BOTH mechanisms TRIM-009 fixes and receives neither** — `InterfaceFactoryRenderer` emits `Local*` with the same inside-the-async guard, and still points its assembly attribute at `{ImplName}Factory`, so DAM covers every `Local*` on it | **Queued, unowned.** Found at TRIM-009 plan review (2026-08-14, finding A4). Deliberately not taken into TRIM-009: it would balloon a plan whose arc the user has already flagged as over-running, and item 19 makes the leg structurally unmeasurable from a client-side harness | Deferring the *work* is fine; shipping a *false claim* is not. The skill asserted "Interface factory \| Yes" for body removal and `docs/trimming.md:37` called interface bodies "unreachable to the trimmer" — a claim the TRIM-008 inventory had already downgraded to "left standing because nothing contradicts it, not because it was measured". **Both qualified 2026-08-14**, plus `CLAUDE-DESIGN.md`; the `docs/trimming.md` half was missed by TRIM-009's first doc pass and caught at code review (V2). The claim now reads "not established" in all three, so the *work* is deferred without a false claim shipping. The remaining exposure is that the leg keeps the exact shape TRIM-009 measured insufficient — an inside-the-async guard plus a DAM target that hosts every `Local*` | +| 21 | **`Local{X}Core` has no name-collision guard** — a factory method whose `UniqueName` ends in `Core` produces a wrapper `Local{Y}Core(...)` that can collide with the generated core of a method named `{Y}`. Identical parameter lists give CS0111 in *generated* code with no diagnostic pointing at the cause | **Queued, unowned.** Found at TRIM-009 code review (C4). No such shape exists in the repo, the Design projects, or the examples | Same family as items 14 and 15 — a CS error in generated code with no diagnostic. Rides along if a plan is ever cut for those; not worth one alone | ## Discovery Log @@ -232,3 +238,43 @@ AC1–AC3 confirmed genuinely verified in a publish-trimmed artifact at HEAD (CI - **Why this is worth a log entry rather than a quiet edit.** The diagnosis was recorded, then contradicted, then re-established on better evidence — three states in one day. Deferred item 18 carried the un-narrowed framing throughout, and the container would have kept asserting the original claim while the plan stub warned against it. A finding that reverses a recorded diagnosis needs the reversal recorded too, not just the endpoint. - **What remains unestablished, deliberately:** whether the early-throw guard shape and the direct-concrete-call shape are *necessary* as well as present. Neither has independent evidence — the static and relay rows are over-determined (post-fix those classes are no longer DAM targets *and* their only reference sits in the folded block), and the interface row cannot go red at all because its markers sit behind an interface hop. One co-variate is unseparable from outside the generator: it emits an extra `catch (OperationCanceledException)` for async methods, so "async" and "extra catch arm" move together. - **Two new latent bugs found while doing this.** `[Service]` parameters on interface-factory methods emit uncompilable code (CS0535) — deferred item 19, found by trying to give the interface leg a directly-reachable marker. And the interface-factory leg is *structurally* unable to measure body elimination, because it reaches everything through interfaces; that is now stated in the target and in the gate rather than left for someone to infer from a clean-looking result. + +### 2026-08-14 — TRIM-009 separation experiment: H1 confirmed, H2 falsified both ways, and the predicted remedy falsified with it +- **The stub's declared first step was run before any design work**, which is the thing TRIM-004 → TRIM-005 failed to do. Three generator-side variants, each published trimmed and probed. Evidence: [`reviews/009-evidence/`](./reviews/009-evidence/). +- **The apparatus was proven inert first.** Both knobs built at their default and the whole generated tree diffed against the pre-edit tree — identical. Reverted afterwards and re-diffed — identical again. Without that step, any variant result would have been confounded with the refactor that carried it. +- **H1 confirmed by a two-directional test.** V1 (async, minus all four awaiting lifecycle probes and the OCE catch arm) **still leaks** — so H2's constructs are not necessary. V2 (sync, plus an OCE catch arm and the `IFactoryOnCancelled` probe) **stays clean** — so they are not sufficient. The mechanism is the async state machine: the feature-switch fold does not propagate out of it. +- **The TRIM-004 story is now falsified a third time** — "early-throw guard plus try/catch defeats unreachable-code elimination" — and for the first time additively, by grafting the blamed constructs onto a working case and watching it keep working. +- **V3 falsified the remedy the stub predicted, which is the most valuable result of the day.** The stub proposed moving the guard into a sync wrapper. Half held: `d__` disappears, so the fold does work once the guard is outside the state machine. Half failed: the body literal and both server-only names are **still present**, because `d__` survives — `DynamicallyAccessedMembers(PublicMethods | NonPublicMethods)` covers **NonPublic**, so relocating the body into a `private` member of the same factory does not escape the DAM root. **A guard-relocation-only fix would have shipped smaller, deleted a state machine, and left the IP on the client.** +- **Why that matters beyond this plan.** The stub's remedy was reasonable, written by someone who had just done the measurement, and wrong. It is the second time in this arc that a plausible remedy survived until it was measured. The rule that caught it — measure the combination, not just the halves — is now Step 1 of TRIM-009 rather than a lesson recorded after the fact. +- **Root inventory is now read out of emitted source rather than inferred:** DAM via the assembly attribute, and the unguarded delegate-registration closure. The public factory interface declares only the entry points, never `Local*`, so it is not a root. The sync `LocalCreate` carries *both* of those roots and still trims clean — which is why "de-root it" was never a description of the defect. +- **Container effect:** deferred item 18's diagnosis is settled and its remedy is not what the stub said. Plan 009 moves Stub → Drafted, with plan review still opted in. + +### 2026-08-14 — TRIM-008's CI-only surface cleared its first Linux run +- **Closed:** code-review finding C3 / test-review T3, accepted with reason at merge time — the absence gate had only ever been exercised on `win-x64` while CI publishes `linux-x64`, with 8 assert-PRESENT markers and 8 positive controls that no one had watched run on Linux. +- **Result:** the post-merge run on `main` (PR #75) passed, so the two-encoding search, the positive controls, and the per-leg attribution all behave on Linux. The `.gitattributes` `text eol=lf` rule did its job — a CRLF checkout would have failed the script with a bad-interpreter error. +- **Why it was accepted rather than fixed pre-merge:** a failure there would have been informative rather than silent, and the pass-3 early-exit fix means a suspect artifact stops at the controls instead of emitting remediation advice. That reasoning held. + +### 2026-08-14 — TRIM-009 implemented: AC6 closes as written, and the fix needed both halves +- **Step 1 ran as specified and came back green.** Wrapper + holder measured together before anything else was built on the prediction. Every leaking marker flipped to absent — `ClassAsyncBody_MARKER`, `IClassLegPort`, `ClassLegInvoke`, `ISaveLegPort`, `SaveLegInvoke`, and all three `SaveLeg*Body_MARKER` — with all async state machines gone and the trimmed assembly 21% smaller (66,560 → 52,224 bytes). +- **Three checks made that claim falsifiable**, and all three were required by plan-review findings rather than volunteered: 10/10 named positive controls present (including the three new holder types); harness exit 0 with every factory resolving, which is the liveness check that catches a holder failing to forward through `method?.Invoke` **silently**; and an untrimmed self-check proving every marker is present before trimming, so the absences are trim results rather than build artifacts. +- **Both halves of the fix were necessary — measured, not argued.** The wrapper alone leaves DAM rooting the private core (V3). The holder alone leaves the guard inside `MoveNext`. This is why the stub's single-lever remedy would have shipped looking like progress. +- **The prediction that kept the change small held:** the delegate-registration closure and the local ctor's method-group assignment both reference the *wrapper*, so the wrapper's post-guard call folding away de-roots the core with no change to either. No registration guarding was needed. +- **Blast radius was one test**, exactly the inversion plan review predicted (`AssemblyAttributeEmissionTests.cs:42`). Red-before-green was then proven properly: breaking the holder prefix and disabling the split turned exactly three tests red, while the sync-path control stayed green. +- **A shape was rescued from being closed over.** Class-level `[Execute]` — a Design source-of-truth pattern, emitted `async` unconditionally — had no harness coverage at all. AC6 demanded "proven, not inferred", so closing AC6 without it would have been a false close. It now has a target and measures clean. +- **What AC6 deliberately does not claim.** The interface-factory leg is carved out in writing. It shares both mechanisms and received neither fix, and item 19 makes it structurally unmeasurable from a client-side harness. The published claim moved from "Interface factory | Yes" to "not established" in both the skill and `CLAUDE-DESIGN.md`. That is the distinction this arc keeps having to relearn: deferring work is fine, shipping a false claim is not. + +### 2026-08-14 — What the TRIM-009 plan review bought, recorded because the arc keeps paying for the alternative +- **Two of its eight veto findings were errors in a plan written specifically to avoid that class of error.** The plan read `d__` being absent in V3 as evidence the fold worked — but the V3 knob emits that method as a non-async wrapper, so the state machine cannot exist regardless of trimming. A check that could not go red, cited as a load-bearing result. Fourth occurrence in this arc, and the first caught *before* implementation. +- **The related one mattered more.** V3 had both candidate roots live — DAM and the wrapper's own call — so it could not attribute the core's survival to either. The plan asserted DAM. The two imply different remedies, and only Step 1 could separate them. It did. +- **Two inventories declared empirical were incomplete:** the root inventory missed the local ctor's `{UniqueName}Property = Local{UniqueName}` assignment, and the emission-site inventory said three sites when there are five — with a rationale that inverted the facts (`Can*` methods are synchronous; `LocalSave` is its own async site). Left uncorrected, `LocalSave` would have gone unwrapped and **Step 1 would still have passed**, because its surviving body references wrappers whose folds kill the cores. A guarded async body would have shipped, invisible to the gate. +- **Cost/benefit, plainly:** the review added no work to the *fix* and a modest amount to the *claims* — docs, the attribute contract, an AC6 disposition, one deferred row. It removed a silent-failure mode and two false statements from a plan that was otherwise ready to implement. + +### 2026-08-14 — TRIM-009 gates: the fix survived unchanged, the claims did not +- **Both gates ran; nine findings, every one in the claims layer.** The generator change was correct on the first pass and neither reviewer proposed a change to it. What needed fixing was documentation, evidence citations, and one genuine gate blind spot. Records: [`reviews/009-test-review.md`](./reviews/009-test-review.md), [`reviews/009-code-review.md`](./reviews/009-code-review.md). +- **The gate had a blind spot this plan had itself documented and then shipped.** Every marker was a *body* signal, and body signals cannot separate "this site was wrapped" from "an ancestor's fold removed the only reference to it". Because `LocalSaveCore` routes to the Insert/Update/Delete **wrappers**, unwrapping the Save site would have cleared every marker while shipping a guarded async body. Closed by asserting the async state machines themselves — a wrapped site has no `d__`; unwrap it and the name returns *and* its body survives. Four of the six have archived pre-fix PRESENT baselines. +- **Re-deriving a finding found a worse one.** The class-`[Execute]` untrimmed self-check first reported **all five markers absent**, which would have meant the gate's checks for that leg were vacuous. They were not: `dotnet publish -r win-x64` writes to a RID subfolder, so the non-RID output still held a build predating the target. A stale artifact is indistinguishable at a glance from a vacuous gate, so both the result and the trap are recorded in the evidence file. +- **Two of the falsified doc claims were written by this plan while fixing the previous plan's.** `docs/trimming.md:236` contradicted a sentence eleven lines below that this plan had just edited; `CLAUDE-DESIGN.md:760` claimed "**Every** factory shape emits its own forwarding holder" when the interface leg emits none. Both came from building the Step 7b list from the plan instead of from the files — **the exact failure the doc-anchor inventory was created to prevent, on its third occurrence.** The lesson is not new and is not being learned by restating it: enumerate by reading the file. +- **Deferred item 20's release-blocking half was nearly missed.** The row named `docs/trimming.md` by name as needing qualification before release; the first doc pass qualified the skill and `CLAUDE-DESIGN.md` and left `docs/trimming.md:37` asserting the interface leg's mechanism — the single published sentence claiming exactly what H1 measured insufficient. Caught at code review, now qualified in all three. +- **A test was removed rather than kept.** An async-guarded `Can*` emission test was added to close the one changed site with no assertion — but `[AuthorizeFactory]` returning `Task` yields a Can that is async and **not** server-only, so no guard and no split: it would have passed for the wrong reason. The shape needs `[AspAuthorize]`, whose references the unit harness lacks. Removed, with the reason recorded at the test file and the real coverage (Design.Domain `SecureOrder`, AspNetCore TestLibrary) named. +- **Five Test Evidence claims were overstated and are corrected**, including a size comparison against a number that turned out to be a knob variant rather than HEAD. That comparison is **withdrawn** rather than restated from memory, because no artifact records HEAD's pre-fix trimmed size. +- **Final state:** 614+614 unit, 561+561 integration (5 pre-existing skips), 86+86 Design, harness exit 0, gate exit 0 with 10 named positive controls and six new per-site discriminators. diff --git a/docs/trimming.md b/docs/trimming.md index 36b21bb4..9ee961e3 100644 --- a/docs/trimming.md +++ b/docs/trimming.md @@ -34,7 +34,7 @@ Not all factory methods get guards. The generator uses the developer's `public` - **Static factories** — `[Execute]` delegate registrations are guarded. The trimmer removes the registration lambdas, their captured dependencies, and the `[Execute]` method bodies themselves. This requires the generated forwarding holder described under [Factory Type Preservation](#factory-type-preservation) — without it the registrar attribute names your static class and the trimmer preserves every method on it, bodies included. - **`[FactoryEventHandler]` classes** — handler registrations are guarded, and the handler bodies plus their `[Service]` dependencies are removed. Same holder mechanism, same reason. -- **Interface factories** — Local method bodies throw `InvalidOperationException` when `IsServerRuntime` is `false`, making the server-only code path unreachable to the trimmer. +- **Interface factories** — Local method bodies throw `InvalidOperationException` when `IsServerRuntime` is `false`. Whether that makes the server-only code path unreachable to the trimmer is **not established** for this shape: the guard sits inside the method, which for `async` operations is not sufficient on its own (see *Async `Local*` emission*), and the leg still names `{ImplName}Factory` in its registrar attribute rather than a single-method holder. No leak has been observed, but the leg reaches its implementation through interfaces, so a client-side trimmed test reads "absent" either way and cannot prove elimination. Treat it as unverified rather than guaranteed. The key insight: the guards are in RemoteFactory's **generated** code, not in your application code. You don't need to modify your domain model at all. @@ -233,7 +233,7 @@ All factory types — class, static, and interface — are automatically preserv That last part is why the attribute never names your own class. Preserving every method on a type means preserving what those methods *do*, so if the attribute named your class, your `[Remote]` method bodies would be preserved along with it — the opposite of the guarantee above. -For class and interface factories the generated factory (`{X}Factory`) hosts the registrar, so there is a type to name that is not yours. Static factories and `[FactoryEventHandler]` classes have no separate generated type — the generator re-opens your own partial class to host `FactoryServiceRegistrar` — so for those the generator emits a tiny holder whose only member forwards to it: +Three of the four shapes emit a tiny holder whose only member forwards to the real registrar, for two different reasons. Static factories and `[FactoryEventHandler]` classes have no separate generated type at all — the generator re-opens your own partial class to host `FactoryServiceRegistrar`, so naming the attribute's target at your class would preserve your bodies. Class factories *do* have a generated `{X}Factory`, but it hosts every `Local*` method, so naming it preserved all of those bodies instead. Interface factories still name `{ImplName}Factory` (see the note below the table). The holder looks like this: ```csharp // generated, alongside your partial class @@ -246,7 +246,24 @@ internal static class NeatooFactoryRegistrar_MyCommands The attribute names the holder. Preservation then reaches exactly one forwarding method instead of everything on `MyCommands`. -**Naming a generated type is necessary, not sufficient.** What makes a holder safe is that it has exactly *one* method. A generated type that hosts many methods still has all of them preserved, bodies included — `{X}Factory` hosts every `Local*` method for its factory. So what keeps a class factory's server-only work off the client is the `IsServerRuntime` guard inside those methods, not the choice of attribute target. +**Naming a generated type is necessary, not sufficient.** What makes a holder safe is that it has exactly *one* method. A generated type that hosts many methods still has all of them preserved, bodies included — `{X}Factory` hosts every `Local*` method for its factory, which is why class factories emit a holder too (`NeatooClassFactoryRegistrar_{ClassName}`) rather than naming the factory directly. + +The `IsServerRuntime` guard inside each `Local*` method does the other half of the work. For `async` operations that guard is emitted in a **non-async wrapper** that forwards to a private core: + +```csharp +public Task LocalFetch(int id, CancellationToken cancellationToken = default) +{ + if (!NeatooRuntime.IsServerRuntime) + throw new InvalidOperationException("Server-only method called in non-server runtime."); + return LocalFetchCore(id, cancellationToken); +} + +private async Task LocalFetchCore(int id, CancellationToken cancellationToken = default) { /* ... */ } +``` + +Inside an `async` method the compiler lowers the whole body — guard included — into the state machine's `MoveNext`, within the builder's own protected region. The trimmer folds the feature switch there but does not eliminate the unreachable remainder, so the body survives. A synchronous method puts the guard ahead of any protected region, which is why sync operations always trimmed correctly and `async` ones did not until v1.7.0. + +**Behaviour change in v1.7.0:** the guard throws synchronously from the wrapper rather than surfacing as a faulted `Task`. Awaiting callers are unaffected. Whether it reaches *your* call site synchronously depends on the entry point — non-async entry points (most reads) propagate it; `async` ones (`Save` on an authorized factory) capture it back into a faulted `Task`. Authorization failures, target casts, and DI resolution failures still surface as faulted tasks in every case — only the server-only guard moved. At startup, `AddNeatooRemoteFactory()` and `AddNeatooAspNetCore()` discover factory types by enumerating these assembly attributes rather than scanning all types via reflection. This means factory registration is fully trimming-safe — no factory types are lost during IL trimming, regardless of whether they are class factories, static factories, or interface factories. diff --git a/skills/RemoteFactory/references/trimming.md b/skills/RemoteFactory/references/trimming.md index 96eee93d..bf01bef0 100644 --- a/skills/RemoteFactory/references/trimming.md +++ b/skills/RemoteFactory/references/trimming.md @@ -135,18 +135,28 @@ The guarantee is not uniform across factory shapes. What follows is measured aga | Shape | `[Remote]`/handler bodies removed? | |---|---| -| Interface factory | Yes | | Static factory (`[Execute]`) | Yes, from v1.7.0 | | `[FactoryEventHandler]` | Yes, from v1.7.0 | | Class factory | Yes, from v1.7.0 — synchronous operations were always removed; `async` ones needed the same release | +| Class-level `[Execute]` | Yes, from v1.7.0 — emitted `async` always, so it needed the same fix | +| Interface factory | **Not established.** No leak has been observed, but the leg reaches its implementation through interfaces, so a client-side test reads "absent" whether or not the body survives. Treat it as unverified rather than proven. | -### Why static factories and event handlers needed a fix +### Why the fix was needed The generator emits `[assembly: NeatooFactoryRegistrar(typeof(X))]` so factory registration survives trimming. That attribute carries `[DynamicallyAccessedMembers]`, which preserves **every method on the type it names, method bodies included**. -Class and interface factories have a generated `{X}Factory` class to name. Static factories and `[FactoryEventHandler]` classes do not — the generator re-opens *your* partial class to host the registrar — so before v1.7.0 the attribute named your class, and preservation covered your `[Remote]` method bodies along with it. They shipped to the browser. +Two distinct problems followed from that, both fixed in v1.7.0: -The fix emits a single-method forwarding holder for the attribute to point at instead. No action needed on your side; it is automatic. If you are on an earlier version and ship `[Execute]` commands or event handlers to a Blazor WASM client, upgrade — the bodies are in your published output today. +1. **Static factories and `[FactoryEventHandler]` classes had no generated type to name.** The generator re-opens *your* partial class to host the registrar, so the attribute named your class and preservation covered your `[Remote]` bodies. They shipped to the browser. +2. **Class factories named `{X}Factory`, which was generated but not small.** It hosts every `Local*` method, so naming it preserved all of them, bodies included. Naming a generated type was never the point — naming a type with exactly *one* method is. + +Both are fixed by emitting a single-method forwarding holder for the attribute to point at. + +`async` class-factory operations needed a second fix on top. Inside an `async` method the compiler lowers the `IsServerRuntime` guard into the state machine's `MoveNext`, within the builder's own protected region; the trimmer folds the switch there but does not remove the unreachable remainder. So guarded `async` operations are now emitted as a non-async wrapper carrying the guard, forwarding to a private async core. + +No action needed on your side; it is automatic. If you are on an earlier version and ship `[Execute]` commands, event handlers, or `async` factory operations to a Blazor WASM client, upgrade — those bodies are in your published output today. + +One behaviour change to be aware of: the server-only guard now throws **synchronously** from the factory entry point rather than surfacing as a faulted `Task`. Code that awaited the call and caught the exception still works; code that called without awaiting and inspected the returned `Task` will now see the throw at the call site. ### `[Remote]` is decorative on `[Execute]` diff --git a/src/Design/CLAUDE-DESIGN.md b/src/Design/CLAUDE-DESIGN.md index fdff2b2b..353ca451 100644 --- a/src/Design/CLAUDE-DESIGN.md +++ b/src/Design/CLAUDE-DESIGN.md @@ -755,22 +755,45 @@ The concrete type is resolved at compile time using the naming convention (`IPer The generator emits `[assembly: NeatooFactoryRegistrar(typeof(X))]` for every factory type (class, static, interface, and `[FactoryEventHandler]`). The `NeatooFactoryRegistrarAttribute` carries `[DynamicallyAccessedMembers(PublicMethods | NonPublicMethods)]` on its `Type` property, which creates a dataflow contract the IL trimmer follows — ensuring the named type's `FactoryServiceRegistrar` method survives trimming. -**The attribute must name a generated type, never a consumer's class.** The annotation preserves every method on whatever it names, *method bodies included*, so naming a user class ships that class's `[Remote]` server-only bodies to a trimmed client. Class and interface factories have a generated `{X}Factory` to name. Static factories and `[FactoryEventHandler]` classes do not — the generator re-opens the user's own partial to host the registrar — so each emits a single-method forwarding holder for the attribute to point at instead. +**The attribute must name a single-method generated holder — naming a generated type is not enough.** The annotation preserves every method on whatever it names, *method bodies included*. Naming a consumer's class ships that class's `[Remote]` bodies to a trimmed client; naming `{X}Factory` ships every `Local*` body, because a generated type that hosts many methods still has all of them preserved. Only a holder with exactly **one** method bounds the blast radius to one method. -Naming a generated type is necessary, not sufficient. The holders are safe because a holder has exactly **one** method; a generated type that hosts many methods still has all of them preserved with their bodies. `{X}Factory` hosts every `Local*` method, so what keeps a class factory's server-only work off the client is the `IsServerRuntime` guard inside those methods, not the choice of attribute target. +Three of the four shapes therefore emit a forwarding holder. Static factories and `[FactoryEventHandler]` classes got theirs in v1.7.0 because they had no generated type at all — the generator re-opens the user's own partial to host the registrar. Class factories got theirs in the same release for the different reason above: `{X}Factory` exists, but it hosts the `Local*` methods whose bodies must not ship. **Interface factories still name `{ImplName}Factory` and have not had this fix** — see the note below the table. + +A holder is necessary but still not sufficient for a class factory. The `IsServerRuntime` guard inside each `Local*` method does the other half — and for `async` operations the guard must sit in a **non-async wrapper** that forwards to a private core. Inside an `async` method the compiler lowers the guard into `MoveNext`, within the builder's own protected region; ILLink folds the switch there but does not eliminate the unreachable remainder, so the body survives. See *Async `Local*` emission* below. At startup, `RegisterFactories()` enumerates these assembly attributes via `assembly.GetCustomAttributes()` instead of scanning all types with `assembly.GetTypes()`. This makes factory discovery trimming-safe: the trimmer sees the static `typeof()` references in the assembly attributes and preserves the referenced types. | Factory Pattern | Assembly Attribute Target | |----------------|--------------------------| -| Class Factory | `typeof({Namespace}.{ClassName}Factory)` — the generated factory implementation class | +| Class Factory | `typeof({Namespace}.NeatooClassFactoryRegistrar_{ClassName})` — a generated forwarding holder | | Static Factory | `typeof({Namespace}.NeatooFactoryRegistrar_{StaticClassName})` — a generated forwarding holder | | Interface Factory | `typeof({Namespace}.{ImplName}Factory)` — the generated factory implementation class | | `[FactoryEventHandler]` | `typeof({Namespace}.NeatooEventHandlerRegistrar_{ClassName})` — a generated forwarding holder | -The two holder rows carry distinct prefixes deliberately: a class carrying both `[Factory]` and `[FactoryEventHandler]` would otherwise collide on the holder type name. +The three holder rows carry distinct prefixes deliberately: a class carrying more than one factory attribute would otherwise collide on the holder type name. + +Until v1.7.0 the static-factory and `[FactoryEventHandler]` rows named **the user's own class**, because there was no generated type to point at, and the class-factory row named `{X}Factory`, which hosts every `Local*` method. All three preserved `[Remote]` bodies on trimmed clients, for the two different reasons described above. The forwarding holders exist to close that. + +The interface-factory row still names `{ImplName}Factory` and has not been through this fix. Its bodies are reached through interfaces, which makes the leg structurally unable to report on body elimination from a client-side test — so the row is neither proven safe nor proven leaking. Tracked as Deferred Work item 20 on the TRIM todo. + +#### Async `Local*` emission + +A guarded `async` factory operation is emitted as a **non-async wrapper carrying the guard**, forwarding to a `private async` core: + +```csharp +public Task LocalFetch(int id, CancellationToken cancellationToken = default) +{ + if (!NeatooRuntime.IsServerRuntime) + throw new InvalidOperationException("Server-only method called in non-server runtime."); + return LocalFetchCore(id, cancellationToken); +} + +private async Task LocalFetchCore(int id, CancellationToken cancellationToken = default) { /* ... */ } +``` + +Synchronous operations keep the guard inline — they already trim correctly, because unreachability begins before any protected region and the whole remainder goes with it. -Until v1.7.0 the static-factory and `[FactoryEventHandler]` rows named **the user's own class**, because there was no generated type to point at. Combined with the annotation above, that preserved every method on those classes — `[Remote]` bodies and all — on trimmed clients. The forwarding holders exist to close that. +**Behaviour note:** the guard now throws *synchronously* from the wrapper rather than surfacing as a faulted `Task`. Whether that reaches the caller synchronously depends on the public entry point: where it is non-async (`public virtual Task Fetch(…) => FetchProperty(…)`) the throw escapes through `I{X}Factory` too; where it is `async` — notably `Save` on an authorized factory — it is captured back into a faulted `Task` as before. Authorization failures, target casts, and DI resolution failures are unaffected in every case: they stay in the core and still surface as faulted tasks. This mechanism is internal to the generator and library. Users do not need to emit or configure these attributes — they are generated automatically for every `[Factory]`-annotated type. diff --git a/src/Generator/Renderer/ClassFactoryRenderer.cs b/src/Generator/Renderer/ClassFactoryRenderer.cs index cfea275c..a2e34fb1 100644 --- a/src/Generator/Renderer/ClassFactoryRenderer.cs +++ b/src/Generator/Renderer/ClassFactoryRenderer.cs @@ -50,8 +50,12 @@ public static string Render(FactoryGenerationUnit unit) sb.AppendLine(); - // Assembly-level attribute for trimming-safe factory discovery - sb.AppendLine($"[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::{unit.Namespace}.{model.ImplementationTypeName}Factory))]"); + // Assembly-level attribute for trimming-safe factory discovery. + // Names the single-method holder, NEVER {X}Factory: the attribute carries + // [DynamicallyAccessedMembers(PublicMethods | NonPublicMethods)], so naming the + // factory roots every Local* method on it -- bodies included -- and no amount of + // guarding removes that root. See RenderRegistrarHolder. + sb.AppendLine($"[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::{unit.Namespace}.{RegistrarHolderPrefix}{model.ImplementationTypeName}))]"); sb.AppendLine(); sb.AppendLine("/*"); @@ -68,11 +72,42 @@ public static string Render(FactoryGenerationUnit unit) // Factory class RenderFactoryClass(sb, model); + // Registrar holder -- the assembly attribute's DAM target + RenderRegistrarHolder(sb, unit, model); + sb.AppendLine("}"); return sb.ToString(); } + /// + /// Prefix for the generated registrar holder. Distinct from the static-factory and + /// event-handler holder prefixes so a class carrying several factory attributes does + /// not collide on the holder type name. + /// + internal const string RegistrarHolderPrefix = "NeatooClassFactoryRegistrar_"; + + /// + /// Emits a top-level holder with exactly ONE method, forwarding to the factory's own + /// registrar. The assembly attribute names this type instead of {X}Factory. + /// + /// + /// [DynamicallyAccessedMembers] has no sub-method granularity: it preserves every + /// method on the named type, bodies included. Naming a generated type is necessary but + /// not sufficient — {X}Factory is generated and still hosts every Local*. + /// What makes a holder safe is that it has exactly one method. + /// + private static void RenderRegistrarHolder(StringBuilder sb, FactoryGenerationUnit unit, ClassFactoryModel model) + { + sb.AppendLine($" internal static class {RegistrarHolderPrefix}{model.ImplementationTypeName}"); + sb.AppendLine(" {"); + sb.AppendLine(" internal static void FactoryServiceRegistrar(IServiceCollection services, NeatooFactory remoteLocal)"); + sb.AppendLine(" {"); + sb.AppendLine($" global::{unit.Namespace}.{model.ImplementationTypeName}Factory.FactoryServiceRegistrar(services, remoteLocal);"); + sb.AppendLine(" }"); + sb.AppendLine(" }"); + } + private static void RenderFactoryInterface(StringBuilder sb, ClassFactoryModel model) { var interfaceVisibility = model.AllMethodsInternal ? "internal" : "public"; @@ -308,27 +343,84 @@ private static void RenderRemoteMethod(StringBuilder sb, FactoryMethodModel meth sb.AppendLine(); } - private static void RenderReadLocalMethod(StringBuilder sb, ReadMethodModel method, ClassFactoryModel model) + /// + /// Emits the opening of a Local* factory method — signature, brace, and the + /// guard when the method is server-only. + /// + /// + /// For guarded async methods the guard is emitted in a NON-async wrapper that + /// forwards to a private async core, and the caller's body lands in the core. + /// + /// The guard must sit outside the async state machine. When it is inside, the compiler + /// lowers the whole body — guard included — into MoveNext, inside the builder's + /// own protected region. ILLink folds the feature switch there but does not eliminate + /// the unreachable remainder, so [Remote] bodies, their [Service] + /// interfaces, and their string literals ship to publish-trimmed clients. A sync method + /// puts the guard ahead of any protected region, so the whole remainder goes. + /// + /// + /// Measured, not assumed (TRIM-009): stripping the async lifecycle probes and the + /// OperationCanceledException arm does not fix it, and adding them to a sync + /// method does not break it. Relocating the guard is necessary but NOT sufficient — + /// the assembly attribute must also name a single-method holder, or + /// DynamicallyAccessedMembers(PublicMethods | NonPublicMethods) roots the + /// private core independently. + /// + /// + private static void RenderLocalMethodOpening( + StringBuilder sb, + string modifiers, + string returnType, + string uniqueName, + string parameters, + string forwardArgs, + bool needsAsync, + bool isServerOnly, + bool blankLineAfterGuard = true) { - // Use async when the method uses await: domain method returns Task, - // or the original factory method was async, or auth checks need await - var needsAsync = method.IsAsync || method.IsDomainMethodTask; - var asyncKeyword = needsAsync ? "async" : ""; - var returnType = GetReturnType(method, includeTask: true, includeAuth: true); - // Local method signature excludes services - they're obtained via ServiceProvider inside - var parameters = GetParameterDeclarationsWithOptionalCancellationToken(method.Parameters, includeServices: false); + if (needsAsync && isServerOnly) + { + sb.AppendLine($" {modifiers} {returnType} Local{uniqueName}({parameters})"); + sb.AppendLine(" {"); + sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); + sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); + sb.AppendLine($" return Local{uniqueName}Core({forwardArgs});"); + sb.AppendLine(" }"); + sb.AppendLine(); + sb.AppendLine($" private async {returnType} Local{uniqueName}Core({parameters})"); + sb.AppendLine(" {"); + return; + } - sb.AppendLine($" public {asyncKeyword} {returnType} Local{method.UniqueName}({parameters})"); + var asyncKeyword = needsAsync ? "async " : ""; + sb.AppendLine($" {modifiers} {asyncKeyword}{returnType} Local{uniqueName}({parameters})"); sb.AppendLine(" {"); // Feature switch guard -- only emit for internal or [Remote] methods. // Public non-[Remote] methods run on both client and server. - if (method.IsInternal || method.IsRemote) + if (isServerOnly) { sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); - sb.AppendLine(); + if (blankLineAfterGuard) + { + sb.AppendLine(); + } } + } + + private static void RenderReadLocalMethod(StringBuilder sb, ReadMethodModel method, ClassFactoryModel model) + { + // Use async when the method uses await: domain method returns Task, + // or the original factory method was async, or auth checks need await + var needsAsync = method.IsAsync || method.IsDomainMethodTask; + var returnType = GetReturnType(method, includeTask: true, includeAuth: true); + // Local method signature excludes services - they're obtained via ServiceProvider inside + var parameters = GetParameterDeclarationsWithOptionalCancellationToken(method.Parameters, includeServices: false); + var forwardArgs = GetParameterIdentifiersWithCancellationToken(method.Parameters, includeServices: false); + + RenderLocalMethodOpening(sb, "public", returnType, method.UniqueName, parameters, forwardArgs, + needsAsync, method.IsInternal || method.IsRemote); // Authorization checks (inside guard -- auth types are server-only) RenderAuthorizationChecks(sb, method); @@ -748,18 +840,11 @@ private static void RenderClassExecuteLocalMethod( { var returnType = GetReturnType(method, includeTask: true, includeAuth: true); var parameters = GetParameterDeclarationsWithOptionalCancellationToken(method.Parameters, includeServices: false); + var forwardArgs = GetParameterIdentifiersWithCancellationToken(method.Parameters, includeServices: false); - sb.AppendLine($" public async {returnType} Local{method.UniqueName}({parameters})"); - sb.AppendLine(" {"); - - // Feature switch guard -- only emit for internal or [Remote] methods. - // Public non-[Remote] methods run on both client and server. - if (method.IsInternal || method.IsRemote) - { - sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); - sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); - sb.AppendLine(); - } + // Class-level [Execute] is emitted async unconditionally. + RenderLocalMethodOpening(sb, "public", returnType, method.UniqueName, parameters, forwardArgs, + needsAsync: true, isServerOnly: method.IsInternal || method.IsRemote); // Authorization checks (inside guard -- auth types are server-only) RenderAuthorizationChecks(sb, method); @@ -806,22 +891,13 @@ private static void RenderLocalMethod(StringBuilder sb, WriteMethodModel method, // Use async when the method uses await: domain method returns Task, // or the original factory method was async var needsAsync = method.IsAsync || method.IsDomainMethodTask; - var asyncKeyword = needsAsync ? "async" : ""; var returnType = GetReturnType(method, includeTask: true, includeAuth: true); // Local method signature excludes services - they're obtained via ServiceProvider inside var parameters = GetParameterDeclarationsWithOptionalCancellationToken(method.Parameters, includeServices: false); + var forwardArgs = GetParameterIdentifiersWithCancellationToken(method.Parameters, includeServices: false); - sb.AppendLine($" public {asyncKeyword} {returnType} Local{method.UniqueName}({parameters})"); - sb.AppendLine(" {"); - - // Feature switch guard -- only emit for internal or [Remote] methods. - // Public non-[Remote] methods run on both client and server. - if (method.IsInternal || method.IsRemote) - { - sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); - sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); - sb.AppendLine(); - } + RenderLocalMethodOpening(sb, "public", returnType, method.UniqueName, parameters, forwardArgs, + needsAsync, method.IsInternal || method.IsRemote); // Authorization checks (inside guard -- auth types are server-only) RenderAuthorizationChecks(sb, method); @@ -1033,22 +1109,13 @@ private static void RenderSavePublicMethod(StringBuilder sb, SaveMethodModel met private static void RenderSaveLocalMethod(StringBuilder sb, SaveMethodModel method, ClassFactoryModel model) { - var asyncKeyword = method.IsAsync ? "async" : ""; var returnType = GetReturnType(method, includeTask: true, includeAuth: true); // Local method signature excludes services - they're obtained via ServiceProvider inside var parameters = GetParameterDeclarationsWithOptionalCancellationToken(method.Parameters, includeServices: false); var paramIdentifiers = GetParameterIdentifiersWithCancellationToken(method.Parameters, includeServices: false); - sb.AppendLine($" public virtual {asyncKeyword} {returnType} Local{method.UniqueName}({parameters})"); - sb.AppendLine(" {"); - - // Feature switch guard -- only emit for internal or [Remote] methods. - // Public non-[Remote] methods run on both client and server. - if (method.IsInternal || method.IsRemote) - { - sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); - sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); - } + RenderLocalMethodOpening(sb, "public virtual", returnType, method.UniqueName, parameters, paramIdentifiers, + method.IsAsync, method.IsInternal || method.IsRemote, blankLineAfterGuard: false); sb.AppendLine(); // Default return value @@ -1309,22 +1376,13 @@ private static void RenderCanRemoteMethod(StringBuilder sb, CanMethodModel metho private static void RenderCanLocalMethod(StringBuilder sb, CanMethodModel method, ClassFactoryModel model) { - var asyncKeyword = method.IsAsync ? "async" : ""; var returnType = method.IsTask ? "Task" : "Authorized"; // Local method signature excludes services - they're obtained via ServiceProvider inside var parameters = GetParameterDeclarationsWithOptionalCancellationToken(method.Parameters, includeServices: false); + var forwardArgs = GetParameterIdentifiersWithCancellationToken(method.Parameters, includeServices: false); - sb.AppendLine($" public {asyncKeyword} {returnType} Local{method.UniqueName}({parameters})"); - sb.AppendLine(" {"); - - // Feature switch guard -- only emit for internal or [Remote] methods. - // Public non-[Remote] methods run on both client and server. - if (method.IsInternal || method.IsRemote) - { - sb.AppendLine(" if (!NeatooRuntime.IsServerRuntime)"); - sb.AppendLine(" throw new InvalidOperationException(\"Server-only method called in non-server runtime.\");"); - sb.AppendLine(); - } + RenderLocalMethodOpening(sb, "public", returnType, method.UniqueName, parameters, forwardArgs, + method.IsAsync, method.IsInternal || method.IsRemote); // Authorization checks (inside guard -- auth types are server-only) RenderAuthorizationChecks(sb, method); diff --git a/src/RemoteFactory/FactoryAttributes.cs b/src/RemoteFactory/FactoryAttributes.cs index e8da797a..81ac03e6 100644 --- a/src/RemoteFactory/FactoryAttributes.cs +++ b/src/RemoteFactory/FactoryAttributes.cs @@ -203,13 +203,35 @@ public FactoryHintNameLengthAttribute(int maxHintNameLength) /// in a published .wasm that should never have left the server. /// /// -/// This is not hypothetical. Static [Factory] classes and -/// [FactoryEventHandler<T>] classes have no separate generated type to host -/// FactoryServiceRegistrar — the generator re-opens the user's own partial class — so -/// from v0.21.2 until this was fixed, both pointed here at the consumer's class and leaked -/// their bodies. The fix was a generated forwarding holder per leg -/// (NeatooFactoryRegistrar_{TypeName}, NeatooEventHandlerRegistrar_{TypeName}) -/// whose single method is all the annotation can reach. +/// This is not hypothetical, and it happened twice for two different reasons. Static +/// [Factory] classes and [FactoryEventHandler<T>] classes have no separate +/// generated type to host FactoryServiceRegistrar — the generator re-opens the user's +/// own partial class — so from v0.21.2 until v1.7.0, both pointed here at the consumer's class +/// and leaked their bodies. +/// +/// +/// "Generated, not consumer" is necessary but NOT sufficient, and assuming otherwise cost a +/// second defect. Class factories always pointed at the generated +/// {X}Factory — and still leaked, because that type hosts every Local* method and +/// the annotation preserves all of them. What bounds the damage is not that the named type is +/// generated but that it has exactly one method. All three legs now emit a forwarding +/// holder (NeatooFactoryRegistrar_{TypeName}, +/// NeatooEventHandlerRegistrar_{TypeName}, NeatooClassFactoryRegistrar_{TypeName}), +/// with distinct prefixes so a class carrying several factory attributes does not collide. +/// +/// +/// The interface-factory leg still names {ImplName}Factory. No leak has been observed +/// there, but the leg reaches its implementation through interfaces, so a client-side trimmed +/// test cannot report on body elimination either way. Treat it as unverified, not proven. +/// +/// +/// A holder is also not sufficient on its own for a class factory. The +/// IsServerRuntime guard inside each Local* method does the other half, and for +/// async operations that guard must be emitted in a non-async wrapper forwarding +/// to a private core — inside an async method the guard is lowered into MoveNext +/// within the builder's protected region, where the trimmer folds the switch but leaves the +/// unreachable remainder in place. Removing either half reopens the leak; that was measured, +/// not reasoned. /// /// /// Consequences for anyone editing the generator: point this attribute at a type that exists diff --git a/src/Tests/RemoteFactory.TrimmingTests/ClassExecuteLegTarget.cs b/src/Tests/RemoteFactory.TrimmingTests/ClassExecuteLegTarget.cs new file mode 100644 index 00000000..6f9ca0cb --- /dev/null +++ b/src/Tests/RemoteFactory.TrimmingTests/ClassExecuteLegTarget.cs @@ -0,0 +1,78 @@ +using Neatoo.RemoteFactory; + +namespace RemoteFactory.TrimmingTests; + +// ============================================================================= +// CLASS-LEVEL [Execute] LEG TARGET (TRIM-009, closing plan-review A3) +// ============================================================================= +// +// `[Execute]` on a NON-static [Factory] class is a distinct emission path from +// `[Execute]` on a static class: +// +// static class -> StaticFactoryRenderer (covered by TrimTestCommands) +// [Factory] class -> ClassFactoryRenderer.RenderClassExecuteLocalMethod +// +// The class path is emitted `async` UNCONDITIONALLY -- there is no sync variant +// to fall back to -- and it carries the same `if (!IsServerRuntime) throw` guard +// inside the async body. So it is subject to TRIM-009's H1 mechanism in full: +// before the fix, its [Remote] body, its [Service] interface, and its literals +// all shipped to a trimmed client. +// +// It had NO harness coverage until this file. That mattered because it is a +// Design source-of-truth pattern -- see Design.Domain/FactoryPatterns/ +// ClassFactoryWithExecute.cs, which demonstrates exactly this shape -- and AC6 +// requires every factory shape be "proven in the trimmed harness, not inferred". +// Found at TRIM-009's plan review, before the plan could close AC6 over it. +// +// MARKER PLACEMENT: the literal lives inside the [Execute] body, which is what +// must disappear. ExecLegBackend_MARKER lives in the implementation and only +// proves the implementation was rooted -- a weaker property, recorded separately. +// ============================================================================= + +/// +/// Class factory carrying a class-level [Execute] method. +/// +/// +/// The [Create] method establishes this as a class factory; RunExecCommand +/// is the shape under test. +/// +[Factory] +public partial class TrimExecTarget +{ + public string Label { get; set; } = string.Empty; + public string ExecResult { get; set; } = string.Empty; + + public TrimExecTarget() { } + + /// + /// Establishes this type as a class factory. Synchronous on purpose: it is the + /// in-file control showing that the sync path on this same type stays clean. + /// + [Remote] + [Create] + internal void Create(string label) + { + Label = label; + } + + /// + /// Class-level [Execute] — emitted async unconditionally by + /// RenderClassExecuteLocalMethod, with the feature-switch guard inside. + /// + /// + /// public static matches the Design pattern. [Remote] is what makes the + /// generator emit the guard; without it the method would run on both sides and the + /// body would legitimately survive. + /// + [Remote] + [Execute] + public static async Task RunExecCommand( + string input, + [Service] IExecLegPort execPort) + { + var instance = new TrimExecTarget(); + instance.Label = input; + instance.ExecResult = await execPort.ExecLegInvoke("ClassExecBody_MARKER: " + input); + return instance; + } +} diff --git a/src/Tests/RemoteFactory.TrimmingTests/LegServerOnlyPorts.cs b/src/Tests/RemoteFactory.TrimmingTests/LegServerOnlyPorts.cs index bb7fa8e1..5d8f8fa1 100644 --- a/src/Tests/RemoteFactory.TrimmingTests/LegServerOnlyPorts.cs +++ b/src/Tests/RemoteFactory.TrimmingTests/LegServerOnlyPorts.cs @@ -126,3 +126,26 @@ public sealed class AsyncLegBackend : IAsyncLegPort { public Task AsyncLegInvoke(string input) => Task.FromResult("AsyncLegBackend_MARKER: " + input); } + +/// +/// Server-only dependency of the class-level [Execute] leg. +/// +/// +/// Distinct from because class-level [Execute] is a +/// separate emission path (RenderClassExecuteLocalMethod), emitted async +/// UNCONDITIONALLY. TRIM-009's plan review found it had no harness coverage at all while +/// being a Design source-of-truth pattern, so AC6's "proven, not inferred" could not be +/// satisfied for it. This port is how it gets measured. +/// +public interface IExecLegPort +{ + Task ExecLegInvoke(string input); +} + +/// +/// Server-side implementation of . +/// +public sealed class ExecLegBackend : IExecLegPort +{ + public Task ExecLegInvoke(string input) => Task.FromResult("ExecLegBackend_MARKER: " + input); +} diff --git a/src/Tests/RemoteFactory.TrimmingTests/Program.cs b/src/Tests/RemoteFactory.TrimmingTests/Program.cs index 8efebb39..a918756f 100644 --- a/src/Tests/RemoteFactory.TrimmingTests/Program.cs +++ b/src/Tests/RemoteFactory.TrimmingTests/Program.cs @@ -37,6 +37,7 @@ services.AddScoped(); services.AddScoped(); services.AddScoped(); + services.AddScoped(); // Server-side implementations behind the interface factories. On the client the // generated proxies stand in for them, so they are never registered there. @@ -157,6 +158,22 @@ failedChecks.Add("save target factory resolution"); } +// Verify the class-level [Execute] factory survived trimming (TRIM-009, plan-review A3). +// This leg is emitted async unconditionally and had no harness coverage before TRIM-009. +ITrimExecTargetFactory? execFactory = null; +try +{ + execFactory = checkScope.ServiceProvider.GetService(); +} +catch (Exception ex) +{ + Console.WriteLine($"Class [Execute] factory resolution FAILED: {ex.GetType().Name}: {ex.Message}"); +} +if (execFactory == null) +{ + failedChecks.Add("class [Execute] factory resolution"); +} + // NOTE: the [FactoryEventHandler] leg has NO positive control here, and cannot. // RelayHandlerRenderer wraps every RegisterHandler call in // `if (NeatooRuntime.IsServerRuntime)`, so on a client publish the generated @@ -225,6 +242,7 @@ Console.WriteLine($"Interface factory resolved: {ifaceFactory != null}"); Console.WriteLine($"Async interface factory resolved: {asyncIfaceFactory != null}"); Console.WriteLine($"Save target factory resolved: {saveFactory != null}"); +Console.WriteLine($"Class [Execute] factory resolved: {execFactory != null}"); if (failedChecks.Count > 0) { diff --git a/src/Tests/RemoteFactory.TrimmingTests/TrimTestEntity.cs b/src/Tests/RemoteFactory.TrimmingTests/TrimTestEntity.cs index ad60e78c..2b7f0ac3 100644 --- a/src/Tests/RemoteFactory.TrimmingTests/TrimTestEntity.cs +++ b/src/Tests/RemoteFactory.TrimmingTests/TrimTestEntity.cs @@ -68,20 +68,28 @@ internal void Create(string name, [Service] IServerOnlyRepository repo, [Service // by DAM on TrimTestEntityFactory and by their own unguarded delegate registration — which // closes the "maybe the sync one just was not rooted" alternative outright. // - // DO NOT "TIDY UP" THE SERVICE PARAMETERS TO MATCH. The asymmetry is deliberate and - // load-bearing: Create takes IServerOnlyRepository and IClassLegPort, this takes only - // IClassLegPort. Because this body SURVIVES trimming, giving it IServerOnlyRepository would - // make IServerOnlyRepository and DoServerWork present in the trimmed output and turn the - // gate's static-factory [D] markers red — a real failure with a completely misleading - // cause. Keep them asymmetric until TRIM-009 lands. + // THE SERVICE ASYMMETRY IS NO LONGER LOAD-BEARING, and the note that said so has expired. + // Create takes IServerOnlyRepository and IClassLegPort; this takes only IClassLegPort. + // Until TRIM-009 this body SURVIVED trimming, so giving it IServerOnlyRepository would have + // surfaced that name in the trimmed output and turned the gate's static-factory markers red + // for a completely misleading reason. Now that both halves are eliminated, that hazard is + // gone. The asymmetry is kept only because changing it buys nothing and would cost a + // re-measurement of every marker in the gate. // - // WHAT THIS PAIR DOES NOT ISOLATE. The generator emits several things only for async - // methods: an extra catch (OperationCanceledException) arm, and type-tests for - // IFactoryOnStartAsync / IFactoryOnCompleteAsync / IFactoryOnCancelled / - // IFactoryOnCancelledAsync. They move with `async` by construction, so from outside the - // generator this pair isolates "async-shaped emission" as a bundle, not the keyword. The - // interface type-tests matter because they are a DIFFERENT ILLink retention mechanism from - // a state machine — see the TRIM-009 stub, which carries the competing hypotheses. + // WHAT THIS PAIR ISOLATES — settled 2026-08-14, from inside the generator. + // The co-variates once listed here as unseparable (an extra catch (OperationCanceledException) + // arm, and type-tests for IFactoryOnStartAsync / IFactoryOnCompleteAsync / IFactoryOnCancelled + // / IFactoryOnCancelledAsync) WERE separated by emitting variants: + // + // V1 async minus all four probes AND the catch arm -> STILL LEAKED (not necessary) + // V2 sync plus the catch arm and a type-test -> STILL CLEAN (not sufficient) + // + // So the mechanism is the async state machine, not the catch arm and not the type-tests — + // which also falsifies the arc's TRIM-004 story a third time, additively. The remedy is a + // NON-async wrapper carrying the guard (so the fold lands outside the builder's protected + // region) PLUS a single-method registrar holder (because DAM covers NonPublicMethods and + // would otherwise root the private core on its own). Neither half suffices alone; that was + // measured too, and the wrapper-only variant looked like progress while changing nothing. [Remote] [Fetch] internal async Task FetchAsync(string name, [Service] IClassLegPort classPort) diff --git a/src/Tests/RemoteFactory.TrimmingTests/verify-trimmed.sh b/src/Tests/RemoteFactory.TrimmingTests/verify-trimmed.sh index c309af12..cdd0747c 100755 --- a/src/Tests/RemoteFactory.TrimmingTests/verify-trimmed.sh +++ b/src/Tests/RemoteFactory.TrimmingTests/verify-trimmed.sh @@ -78,7 +78,10 @@ for control in \ "TrimTestCommands" \ "ITrimIfaceQueryFactory" \ "ITrimAsyncIfaceQueryFactory" \ - "ITrimSaveTargetFactory" + "ITrimSaveTargetFactory" \ + "NeatooClassFactoryRegistrar_TrimTestEntity" \ + "NeatooClassFactoryRegistrar_TrimSaveTarget" \ + "NeatooClassFactoryRegistrar_TrimExecTarget" do if present "$control"; then echo " ok $control" @@ -121,8 +124,14 @@ fi # [N] new baseline — the target did not exist before the fix, so there is no pre-fix # measurement. First trimmed measurement is the baseline. # -# Every marker here appears PRESENT in the UNTRIMMED build, which proves the PROBE can see -# it. That is necessary but NOT sufficient for the check to be meaningful: `ServerOnlyHelper` +# Every BODY marker here has been measured PRESENT in the UNTRIMMED build, which proves the +# PROBE can see it — see reviews/009-evidence/probe-selfcheck-final-all-legs.txt, which covers +# every leg including class-[Execute]. (The state-machine names in the per-site block at the +# bottom are the exception by construction: `d__` is ABSENT untrimmed precisely +# because the site is wrapped, which is the property being asserted. Their untrimmed +# counterparts are the `d__` names, measured PRESENT in the same file.) +# Untrimmed presence is necessary but NOT sufficient for a check to be meaningful: +# `ServerOnlyHelper` # was untrimmed-PRESENT for months while nothing referenced it, so ILLink dropped it # unconditionally and its absence could never have gone red. A marker is only meaningful if # something a defect could plausibly affect actually roots it. The `*Backend` implementation @@ -202,19 +211,35 @@ for m in TrimAsyncIfaceServerSide IfaceAsyncBody_MARKER; do check_absent "$m" "interface factory (async)" done -echo "-- class factory, port implementation (behind an interface hop; not a leg signal)" +echo "-- class factory port" # [N] own port, so a class-factory leak no longer reports as a static-factory failure. # -# Only the two IMPLEMENTATION markers are absent. IClassLegPort and ClassLegInvoke are NOT -# here: they are retained by the async FetchAsync body (TRIM-009) via its in-body -# GetRequiredService() and the port call, exactly as ISaveLegPort/SaveLegInvoke -# are on the save leg. They are asserted PRESENT with the controlled pair below. -# ClassLegBackend and its literal stay absent because they sit behind the IClassLegPort +# IClassLegPort and ClassLegInvoke moved here from the assert-PRESENT block when TRIM-009 +# landed: the async FetchAsync body reached them via an in-body GetRequiredService() +# and a port call, and that body is now eliminated. They are the [D] discriminators for the +# class leg — a regression in the async wrapper/holder fix turns these red first. +# ClassLegBackend and its literal are weaker signals: they sit behind the IClassLegPort # interface hop, so nothing statically reaches them either way. -for m in ClassLegBackend ClassLegBackend_MARKER; do +for m in IClassLegPort ClassLegInvoke ClassLegBackend ClassLegBackend_MARKER; do check_absent "$m" "class factory" done +echo "-- class-level [Execute] leg (TRIM-009)" +# [N] Class-level [Execute] is a DIFFERENT emission path from static [Execute] +# (ClassFactoryRenderer.RenderClassExecuteLocalMethod, not StaticFactoryRenderer) and is +# emitted `async` unconditionally. It had no harness coverage at all until TRIM-009 — found at +# plan review, before the plan could close AC6 over an unmeasured shape. +# +# [N] is accurate and load-bearing: this target postdates the pre-fix probes, so there is NO +# measurement of it leaking. Its body was rooted by an unguarded delegate registration and a +# ctor method-group assignment, and H1 applies to its shape — but that is a reading of the +# emitted source, not a measurement, and the distinction is exactly what AC6 asks for. What +# IS measured: all five markers PRESENT in the untrimmed build (so these checks are not +# vacuous), and absent here. +for m in IExecLegPort ExecLegInvoke ExecLegBackend ExecLegBackend_MARKER ClassExecBody_MARKER; do + check_absent "$m" "class [Execute]" +done + echo "-- async-only port (shared by the three async targets above)" for m in IAsyncLegPort AsyncLegInvoke AsyncLegBackend; do check_absent "$m" "async port" @@ -233,48 +258,60 @@ done # controlled — it also differed in auth, target acquisition, one-hop vs two-hop rooting, and # catch-arm count, the last being the dimension the arc's disproven TRIM-004 story blamed. # -# CO-VARIATES, unseparable from outside the generator: it emits an extra -# `catch (OperationCanceledException)` arm for async methods AND type-tests for -# IFactoryOnStartAsync / IFactoryOnCompleteAsync / IFactoryOnCancelled / IFactoryOnCancelledAsync. -# So this pair isolates "async-shaped emission" as a bundle, not the `async` keyword. The -# sub-cause is UNDETERMINED: a state-machine fold failure and an unreachable-code-elimination -# failure caused by the extra catch/type-tests predict the same result here and need different -# fixes. TRIM-009 must separate them from inside the generator. - +# RESOLVED BY TRIM-009. This pair was the arc's decisive measurement; both halves are now +# absent and both are asserted so. The co-variates once listed here as unseparable — +# the extra `catch (OperationCanceledException)` arm and the four lifecycle type-tests — +# WERE separated, from inside the generator: +# +# V1 async minus all four probes AND the catch arm -> STILL LEAKED (not necessary) +# V2 sync plus the catch arm and a type-test -> STILL CLEAN (not sufficient) # -# ClassAsyncBody_MARKER is asserted PRESENT for the same reason as the save/can* block: it is -# TRIM-009's defect, and the gate must fail loudly when it is fixed rather than silently -# passing. +# so the async state machine is the mechanism, and the arc's TRIM-004 story ("early-throw +# guard plus try/catch defeats elimination") is falsified for the third time. The fix is a +# NON-async wrapper carrying the guard plus a single-method registrar holder; neither half +# suffices alone, because [DynamicallyAccessedMembers] covers NonPublicMethods and roots the +# private core independently. # --------------------------------------------------------------------------- echo "-- controlled sync/async pair (class factory)" check_absent "ClassSyncBody_MARKER" "class factory (sync half of controlled pair)" -for m in ClassAsyncBody_MARKER IClassLegPort ClassLegInvoke; do - if present "$m"; then - echo " ok $m (still present, as TRIM-009 expects)" - else - fail "[class factory] '$m' is now ABSENT. If TRIM-009 has landed, promote it into the absence checks above and delete it from here. If not, check the fixture first (a changed or removed target is the most common cause), and only then reopen the async diagnosis." - fi +check_absent "ClassAsyncBody_MARKER" "class factory (async half of controlled pair)" + +# --------------------------------------------------------------------------- +# SAVE/CAN* WRITE PATH — absent since TRIM-009. +# +# These five were asserted PRESENT by TRIM-008 as a deliberate tripwire, so that CI would +# fail loudly the moment the leak was fixed rather than quietly keep passing. It worked: +# they are the markers TRIM-009 flipped, and they are [D] discriminators for the write path +# (LocalInsert / LocalUpdate / LocalDelete, plus LocalSave routing into them). +# --------------------------------------------------------------------------- +echo "-- save/can* write path" +for m in ISaveLegPort SaveLegInvoke SaveLegInsertBody_MARKER SaveLegUpdateBody_MARKER SaveLegDeleteBody_MARKER; do + check_absent "$m" "save/can*" done # --------------------------------------------------------------------------- -# KNOWN-BROKEN — asserted PRESENT on purpose (TRIM-009). +# PER-SITE WRAPPER DISCRIMINATORS — the async state machines themselves. +# +# WHY THESE EXIST. Every marker above is a *body* signal, and body signals cannot +# tell "this method was wrapped" from "an ancestor's fold removed the only reference +# to it". LocalSaveCore routes to the Insert/Update/Delete WRAPPERS, so if a future +# edit unwrapped RenderSaveLocalMethod (or the Can*/class-[Execute] site) the markers +# above could still come back clean while a guarded async body shipped. TRIM-009's own +# plan named that blind spot before it shipped one; this block closes it. # -# Async generated Local* methods retain their server-only bodies; sync ones in the -# same assembly do not. That is a different defect from the registrar-DAM one this -# gate was written for, with a different fix, so TRIM-008 does not remove these. +# HOW THEY DISCRIMINATE. A wrapped site has NO `d__` — the wrapper is not async, +# and the state machine is named `d__` instead. Unwrap a site and `d__` +# comes back AND (per H1) its body survives, so the name lands in the trimmed output. # -# Asserting them PRESENT rather than omitting them is deliberate. Omitted, the gate -# would quietly keep passing after TRIM-009 lands and nobody would tighten it. -# Asserted, CI fails the moment the leak is fixed and the failure message says what -# to do. This is a pending marker, not an endorsement. +# [D] for the first five: all were measured PRESENT in the pre-fix trimmed assembly +# (reviews/009-evidence/probe-h1h2-v1/v2), so these are real discriminators with a +# baseline, not new-shape guesses. [N] for LocalRunExecCommand — the class-[Execute] +# target postdates the pre-fix probes; it is measured PRESENT untrimmed instead +# (probe-selfcheck-final-all-legs.txt). # --------------------------------------------------------------------------- -echo "-- save/can* (known broken, TRIM-009)" -for m in ISaveLegPort SaveLegInvoke SaveLegInsertBody_MARKER SaveLegUpdateBody_MARKER SaveLegDeleteBody_MARKER; do - if present "$m"; then - echo " ok $m (still present, as TRIM-009 expects)" - else - fail "[save/can*] '$m' is now ABSENT. If TRIM-009 has landed, this is good news: move '$m' up into the absence checks and delete it from this block. If TRIM-009 has NOT landed, check the fixture first — a changed or removed target is the most common cause — and only then reopen its diagnosis." - fi +echo "-- per-site wrapper discriminators (async state machines)" +for m in 'd__' 'd__' 'd__' 'd__' 'd__' 'd__'; do + check_absent "$m" "guarded async site lost its sync wrapper" done echo @@ -285,7 +322,13 @@ fi echo "Trimming verification passed." echo " Absent: static factory ([Execute], sync and async), relay handler (sync and async)," -echo " interface factory implementations, and the SYNCHRONOUS class-factory body." -echo " Present, expected, tracked as TRIM-009: every ASYNC class-factory body — the save/can*" -echo " write path and the FetchAsync half of the controlled pair. Asserted PRESENT" -echo " above, so this gate fails loudly the moment TRIM-009 lands." +echo " interface factory implementations, class-level [Execute], and BOTH halves of" +echo " the class-factory body — sync and async, read and write." +echo " No shape is asserted PRESENT as a known leak. TRIM-008 and TRIM-009 closed the last two;" +echo " if a leak is found in a shape this gate does not name, add the marker rather than" +echo " widening an existing one, so the failure keeps naming its leg." +echo +echo " NOT proven here: the interface-factory leg's method BODIES. It reaches everything" +echo " through interfaces, so its markers read absent either way (Deferred Work item 20;" +echo " item 19 blocks the fix that would give it a reachable marker). Absence above is a" +echo " no-regression signal for that leg, not a body-elimination proof." diff --git a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs index 1e5526e8..df038195 100644 --- a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs +++ b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs @@ -40,7 +40,191 @@ internal void Create() { } ?.ToString(); Assert.NotNull(generatedSource); - Assert.Contains("[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::TestNamespace.MyEntityFactory))]", generatedSource); + + // The attribute names the generated single-method HOLDER, never the factory. + // Inverted by TRIM-009: this assertion previously expected + // `typeof(global::TestNamespace.MyEntityFactory)`. Original intent is preserved — + // the attribute is still emitted and still names the correct type — but the + // correct type changed, because [DynamicallyAccessedMembers(PublicMethods | + // NonPublicMethods)] on the attribute roots EVERY method on whatever it names, + // bodies included. {X}Factory hosts every Local*, so naming it kept [Remote] + // bodies on publish-trimmed clients. Measured, TRIM-009. + Assert.Contains( + "[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::TestNamespace.NeatooClassFactoryRegistrar_MyEntity))]", + generatedSource); + + // The regression assertion whose absence let the static leg ship broken for a year: + // assert the attribute does NOT name the factory type. + Assert.DoesNotContain( + "NeatooFactoryRegistrar(typeof(global::TestNamespace.MyEntityFactory))", + generatedSource); + } + + /// + /// The class-factory registrar holder is emitted as a top-level type with exactly one + /// method, forwarding to the factory's own registrar. + /// + /// + /// Anchored with binding the signature to the + /// holder's class declaration. A bare Contains on the signature line is satisfied by + /// {X}Factory.FactoryServiceRegistrar, which emits a byte-identical line — that is + /// exactly how TRIM-008's first version of this test passed while pinning nothing. + /// + [Fact] + public void ClassFactory_EmitsRegistrarHolder_ForwardingToFactory() + { + // The usings match StaticFactorySource: the generated factory references + // CancellationToken and IServiceProvider, so without them the compile assertion + // below fails on the FIXTURE rather than on the emission. + var source = @" +using System; +using System.Threading; +using System.Threading.Tasks; +using Neatoo.RemoteFactory; + +namespace TestNamespace +{ + [Factory] + public partial class MyEntity + { + [Create] + internal void Create() { } + } +} +"; + var (_, outputCompilation, runResult) = DiagnosticTestHelper.RunGenerator(source); + + var generatedSource = runResult.GeneratedTrees + .FirstOrDefault(t => t.FilePath.Contains("MyEntityFactory")) + ?.GetText() + ?.ToString(); + + Assert.NotNull(generatedSource); + + Assert.Matches( + @"internal static class NeatooClassFactoryRegistrar_MyEntity\s*\{\s*internal static void FactoryServiceRegistrar\(IServiceCollection services, NeatooFactory remoteLocal\)", + generatedSource); + + Assert.Contains( + "global::TestNamespace.MyEntityFactory.FactoryServiceRegistrar(services, remoteLocal);", + generatedSource); + + // The class leg gained a new top-level type AND a new method per guarded async local, + // and FactoryRenderer swallows render exceptions into a /* Error: */ comment while + // NormalizeWhitespace parses with error recovery — malformed emission yields mangled + // output, not a throw. The static and relay legs already assert this; the class leg + // did not until TRIM-009's code review (C2). + Assert.Empty(outputCompilation.GetDiagnostics() + .Where(d => d.Severity == DiagnosticSeverity.Error)); + } + + /// + /// A guarded async Local* method is emitted as a NON-async wrapper carrying + /// the feature-switch guard, forwarding to a private async core. + /// + /// + /// The guard must not sit inside the async state machine. When it does, the compiler + /// lowers it into MoveNext inside the builder's protected region, ILLink folds the + /// switch but does not eliminate the unreachable remainder, and the [Remote] body + /// ships to trimmed clients. Measured, TRIM-009 — stripping the async lifecycle probes + /// and the OperationCanceledException arm did not fix it, and adding them to a sync + /// method did not break it. + /// + [Fact] + public void ClassFactory_GuardedAsyncLocalMethod_SplitsIntoSyncWrapperAndAsyncCore() + { + var source = @" +using System.Threading.Tasks; +using Neatoo.RemoteFactory; + +namespace TestNamespace +{ + [Factory] + public partial class MyEntity + { + [Remote] + [Fetch] + internal async Task FetchIt(string name) { await Task.CompletedTask; } + } +} +"; + var (_, _, runResult) = DiagnosticTestHelper.RunGenerator(source); + + var generatedSource = runResult.GeneratedTrees + .FirstOrDefault(t => t.FilePath.Contains("MyEntityFactory")) + ?.GetText() + ?.ToString(); + + Assert.NotNull(generatedSource); + + // Wrapper: NOT async, carries the guard, forwards to the core. + Assert.Matches( + @"public Task LocalFetchIt\(string name, CancellationToken cancellationToken = default\)\s*\{\s*if \(!NeatooRuntime\.IsServerRuntime\)", + generatedSource); + Assert.Contains("return LocalFetchItCore(name, cancellationToken);", generatedSource); + + // Core: async, private, and carries NO guard — the guard already ran in the wrapper. + Assert.Contains("private async Task LocalFetchItCore(", generatedSource); + Assert.Matches( + @"private async Task LocalFetchItCore\([^)]*\)\s*\{\s*(?!\s*if \(!NeatooRuntime\.IsServerRuntime\))", + generatedSource); + } + + // NO UNIT TEST FOR THE ASYNC GUARDED Can* SITE, AND THIS IS WHY. + // + // `RenderCanLocalMethod` is the fifth wrapper site and the only one with no emission + // assertion here — raised at TRIM-009's test review. An attempt to add one was removed + // rather than kept, because it did not test what it claimed: an `[AuthorizeFactory]` + // whose method returns `Task` produces a Can* that is async but NOT server-only, + // so no guard is emitted and no split occurs. The assertion passed or failed for reasons + // unrelated to the wrapper. + // + // The shape that DOES produce a guarded async Can* is `[AspAuthorize]` policy auth, whose + // generated check is async by nature. That needs ASP.NET Core references, which + // `DiagnosticTestHelper.BuildReferences()` does not carry. + // + // The site is not unexercised: `Design.Domain.Aggregates.SecureOrder` and + // `RemoteFactory.AspNetCore.TestLibrary` both emit `LocalCan*Core` wrappers, both compile, + // and both are covered by passing suites (Design 86+86). What is missing is a dedicated + // emission assertion, which needs an ASP-auth fixture in this harness. + + /// + /// A SYNCHRONOUS guarded Local* method keeps the guard inline and is not split. + /// + /// + /// The sync shape already trims correctly — unreachability begins before any protected + /// region, so the whole remainder goes. Splitting it would be churn. This test is the + /// control that keeps the wrapper narrowly scoped to the shape that needed it. + /// + [Fact] + public void ClassFactory_GuardedSyncLocalMethod_IsNotSplit() + { + var source = @" +using Neatoo.RemoteFactory; + +namespace TestNamespace +{ + [Factory] + public partial class MyEntity + { + [Remote] + [Create] + internal void Create(string name) { } + } +} +"; + var (_, _, runResult) = DiagnosticTestHelper.RunGenerator(source); + + var generatedSource = runResult.GeneratedTrees + .FirstOrDefault(t => t.FilePath.Contains("MyEntityFactory")) + ?.GetText() + ?.ToString(); + + Assert.NotNull(generatedSource); + Assert.DoesNotContain("LocalCreateCore", generatedSource); + Assert.Matches( + @"public Task LocalCreate\(string name, CancellationToken cancellationToken = default\)\s*\{\s*if \(!NeatooRuntime\.IsServerRuntime\)", + generatedSource); } ///