diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 00000000..08b84b69 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,5 @@ +# Shell scripts must keep LF endings regardless of the contributor's platform. +# CI runs them on Linux; a CRLF checkout puts a stray \r on the shebang line and +# bash fails with "bad interpreter: /usr/bin/env bash^M" — an error that reads like +# a missing interpreter rather than a line-ending problem. +*.sh text eol=lf diff --git a/.github/workflows/build.yml b/.github/workflows/build.yml index 86878f0d..1f42d6bc 100644 --- a/.github/workflows/build.yml +++ b/.github/workflows/build.yml @@ -106,16 +106,21 @@ jobs: PUBLISH_DIR="src/Tests/RemoteFactory.TrimmingTests/bin/Release/net9.0/linux-x64/publish" - # Server-only IMPLEMENTATION types must be trimmed out of the published - # assembly. The IServerOnlyRepository interface name is expected to remain - # (referenced from guarded-dead LocalCreate bodies the trimmer keeps — - # tracked as TRIM-005); the implementations must not. - if grep -aq "ServerOnlyDirect" "$PUBLISH_DIR/RemoteFactory.TrimmingTests.dll" \ - || grep -aqP '(?snippet source | anchor +**Trimming:** the generated local registration is guarded by `NeatooRuntime.IsServerRuntime`, so a client published with the feature switch set to `false` drops the method body, its `[Service]` dependencies, and their transitive references. See [IL Trimming](trimming.md). + +Note that `[Remote]` is **decorative** on `[Execute]` methods — static factories are exempt from the NF0105 `[Remote] public` check, and the generator emits both remote and local registrations regardless, guarding only the local one. What makes the body trimmable is the guard, not `[Remote]`. Keeping `[Remote]` on the method is still worthwhile as intent, and matches how the same marker behaves on class factories. + ### [FactoryEventHandler\] Class-level attribute that marks a class as a **server-side** static handler for factory events of type `T` (where `T : FactoryEventBase`). The source generator finds one matching `static` method by signature and registers it with `FactoryEventHandlerRegistry`. See [Factory Events](factory-events.md) for the full pattern. @@ -209,6 +213,8 @@ public static partial class OrderAuditHandler } ``` +**Trimming:** handler registrations are wrapped in `NeatooRuntime.IsServerRuntime`, so a client published with the feature switch set to `false` drops the handler bodies and their `[Service]` dependencies. Because those registrations are entirely server-guarded, there is nothing left on a trimmed client to resolve — handler registration cannot be verified from a client-side test, only from server-side or untrimmed ones. See [IL Trimming](trimming.md). + Runs in the caller's DI scope via `FactoryEventHandlerRegistry`, triggered by `IFactoryEvents.Raise` during a factory method. All handlers for the event type run sequentially, awaited, sharing the caller's `DbContext` and transaction. A throwing handler aborts the chain and propagates to the caller. For fire-and-forget work that should not participate in the caller's transaction, compose a manual `Task.Run` + `IServiceScopeFactory.CreateScope()` pattern inside the factory method (see the [v1.5.0 release notes](release-notes/v1.5.0.md)). > **Instance-method handlers are not supported.** Declaring a non-`static` matching method inside a `[FactoryEventHandler]` class emits **NF0503 (Warning)** and is silently skipped at runtime. Client-side reception is handled by implementing `IFactoryEventRelay` on your own class and registering it in DI — see [Factory Events — Client-Side Relay](factory-events.md#client-side-relay-consumer-implements-ifactoryeventrelay) and the [`IFactoryEventRelay`](interfaces-reference.md#ifactoryeventrelay) interface reference. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/008-registrar-dam-over-preservation.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/008-registrar-dam-over-preservation.md new file mode 100644 index 00000000..f3594469 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/008-registrar-dam-over-preservation.md @@ -0,0 +1,211 @@ +# TRIM-008 — Registrar-DAM over-preservation fix + +**Plan #:** 008 +**Date:** 2026-08-12 +**Related Todo:** [../todo.md](../todo.md) +**Status:** Done +**Last Updated:** 2026-08-13 +**Plan-review opt-in:** Yes (generator emission contract change; security-relevant — this is the plan that makes a false IP-protection guarantee true; mirrors TRIM-007, whose review caught a veto that would have broken every consumer build) +**Code-review opt-in:** Yes (behavior-changing generator work) + +--- + +## Scope + +Make RemoteFactory's IP-protection guarantee true for the two shapes where it is false: `[Execute]` static factories and `[FactoryEventHandler]` classes. Their generated `[assembly: NeatooFactoryRegistrar(typeof(T))]` names **the consumer's own class**, and the attribute's `DynamicallyAccessedMembers(PublicMethods | NonPublicMethods)` makes ILLink retain every method on it — so `[Remote]` server-only bodies ship to the browser decompilable. Fix by emitting a top-level generated holder type that forwards to the existing registrar, and pointing the attribute at the holder. + +Also in scope, because they are inseparable from the fix: correcting the ~40 documentation anchors the defect falsified (the docs are only false while the code is broken, so they must not be separately mergeable); giving the trimming harness the targets it lacks, including closing plan-review finding **B9** in full; and re-deriving the CI absence gate, whose current exemption is justified by a diagnosis this arc disproved. + +**Not** in scope: narrowing the DAM itself; the two latent generator bugs surfaced during design; replacing the reflective lookup with `[ModuleInitializer]` registration; cutting the v1.7.0 release. + +**Scope correction, 2026-08-13.** The line above originally read that this plan unblocks the release. It no longer does on its own. This plan's own pre-fix probe found a **third** broken shape — class factories with async write operations, a different mechanism — now carried as [TRIM-009](./009-async-local-method-body-retention.md). AC6 is held whole across both plans (user decision), so v1.7.0 unblocks when TRIM-009 also lands. TRIM-008 delivers two of AC6's three shapes and must not be closed out as delivering AC6. + +--- + +## Intent + +- The guarantee RemoteFactory advertises becomes the guarantee it delivers, for every factory shape rather than two of four. +- The documentation stops asserting something demonstrably false about what ships to a user's browser — including the distributable skill, which travels without the repo. +- The trimming harness gains the ability to *see* this class of defect at all. Today it cannot: the relay-handler leg has no target whatsoever, and the static leg has targets but no assertion. +- The registrar attribute gains a written contract — "the `Type` must be a generated registrar type, never a consumer type" — so the next person cannot reintroduce this by pointing it somewhere convenient. + +--- + +## Framework & Architectural Alignment + +- The remedy mirrors TRIM-007's shape (`EventPreservationRenderer.cs:71,83-85`): a top-level `internal static` holder whose only member is `FactoryServiceRegistrar`, so the DAM blast radius is one method. Precisely what CI verifies at HEAD is that such a holder **registers correctly under trimming** — it does *not* verify DAM narrowing, because that holder has one method and nothing to narrow. The shapes are analogous, not identical: TRIM-007 emits one holder **per assembly** in an assembly-derived namespace; TRIM-008 emits one **per type** in the user's namespace. +- It deliberately does **not** use the nested-type variant floated at TRIM-005's review, which depends on an ILLink property (DAM not extending to nested types) that this codebase has never verified. TRIM-005 was abandoned for building on an unverified inherited diagnosis; that lesson applies here. +- The holder **forwards** rather than hosts, because `[Execute]` methods are `private static` by the repo's own design convention (`src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs:362-368`) and the registrar body calls them. This also means no generated member is removed or renamed. +- No model, builder, or transform changes — the incremental-cache surface stays at zero delta, honoring what TRIM-006 just repaired. +- Absence assertions live in CI, not in the harness process: string absence is only observable from outside the process (`plans/004-trimming-harness-ci-gate.md:119`). + +--- + +## Constraints & Invariants + +- `FactoryAttributes.cs:148-170` is unchanged apart from XML docs. Narrowing the DAM would silently break consumers referencing a prebuilt library compiled by an older generator — its registrar would no longer be rooted and its factories would fail to register on a trimmed client, with no diagnostic. +- No generated member is removed, renamed, or has its accessibility changed. This is `fix:`, not `feat!:`. +- Class-factory, interface-factory, and event-preservation emission is byte-identical. Static and relay emission changes by design. +- `RelayHandlerModel` and `TypeInfo` gain no fields; `IncrementalCacheTests` stays green. +- Existing tests are not gutted. `AssemblyAttributeEmissionTests.cs:142` pins the defective behavior and is in-scope to retarget, but its intent — "the registrar attribute is emitted and names the correct type" — must be preserved. +- **The holder's method must remain exactly `FactoryServiceRegistrar`.** `AddRemoteFactoryServices.cs:168-170` looks it up by that literal string and calls `method?.Invoke`, so any rename silently stops registration for that type with no diagnostic and no exception. +- The holder name must **not** be a prefix-extension of the user's type. `{TypeName}NeatooFactoryRegistrar` leaves `global::Ns.{TypeName}` a substring of the holder's FQN, which makes the "must not name the consumer's type" assertion a false red and an unclosed `Contains` a false green. Hence the `NeatooFactoryRegistrar_{TypeName}` prefix form, and a distinct prefix per leg so the two never collide on a class carrying both attributes. +- **Design-surface disposition for AC6:** accept-with-reason, mirroring deferred item 11. Over-preservation is no more observable in untrimmed Design tests than preservation was — `RemoteFactory.TrimmingTests` is the verification surface. `CLAUDE-DESIGN.md` is still corrected as documentation (Step 9); what is accepted is the absence of a *demonstrating example*. +- Full suite green on net9.0 and net10.0 across both solutions, plus the trimmed harness exiting 0. + +--- + +## Steps + +**Order matters and is not negotiable: every harness target must exist and be probed BEFORE the leg that fixes it.** A marker measured for the first time after the fix cannot distinguish "the fix removed it" from "it was never rooted" — plan review B1, and the third appearance of this arc's signature failure. + +1. Give the harness the targets it lacks, before any generator change: a `[FactoryEventHandler]` class calling a server-only service (that leg has no coverage at all), plus interface-factory and Save/Can* targets, closing B9. Each server-only dependency gets a leg-distinct marker so a failure names the culprit. +2. Probe publish-trimmed with the new targets and the generator **unfixed**, recording which markers are present. This is the baseline the later "absent" claims are measured against; without it those claims are unfalsifiable. +3. Emit the forwarding holder for the static-factory leg and retarget its assembly attribute, changing nothing else in that renderer's output. *(Already done and measured — the pre-existing `_DoWork`/`_ProcessRecord` targets gave this leg a valid baseline.)* +4. Do the same for the relay-handler leg, with a distinct holder name shape, and fix its long-standing missing `global::` prefix at the same time. +5. Give the registrar attribute a written contract in its XML doc: the `Type` must be a generated registrar type, never a consumer type, because the DAM retains every method on it, bodies included. +6. Pin the new emission for both legs — including a regression assertion that the attribute does **not** name the user's type, the check whose absence let this ship — and add the relay-handler emission tests that have never existed. Relay tests must assert the output compilation is error-free, not merely that strings appear: relay output bypasses `NormalizeWhitespace` and has no parse-error signal. +7. Re-probe publish-trimmed and compare against step 2, so every marker has a present-before / absent-after pair. Confirm registration still works on both legs — absence assertions pass more easily when registration is silently dead. +8. Tighten the CI gate to what steps 2 and 7 proved — per-pattern messages, a durable positive control so a missing or renamed DLL cannot pass silently, and removal of the `(?]` class are absent from a publish-trimmed client assembly. `[trimmed-harness]` +- [ ] Both legs' assembly attributes name a generated holder, and provably do not name the consumer's type. `[unit]` +- [ ] The relay-handler assembly attribute is `global::`-qualified. `[unit]` +- [x] Class-factory, interface-factory, and event-preservation emission is byte-identical before and after. Evidence is an **expected-delta-set equality** check — enumerate the files expected to change, then assert the actual delta set *equals* it. `[explicit-skip: one-off recursive emission diff — `Generated/` is gitignored so `git status` cannot detect drift. Unlike TRIM-006's zero-delta diff, this one has a nonzero expected delta, so "inspect the diff" would not discriminate]` + **Measured 2026-08-13.** Full emission tree captured twice over both solutions with `--no-incremental`, once with the generator reverted to `25ac975` (pre-TRIM-008) and once at HEAD, using an identical collection procedure. **732 files emitted; 40 changed (20 relay-handler, 20 static-factory); 692 byte-identical.** No class-factory or interface-factory output appears in the delta. Discharged one level stronger than the bullet asked: rather than only comparing the delta *set* against an expectation, every changed line in every changed file was matched against the two permitted shapes (registrar-attribute retarget, holder block) — **0 lines fell outside them**. Non-vacuity: the delta list held 40 entries, so the "zero unexpected" result came from inspecting real diffs rather than an empty loop. +- [ ] The CI gate fails when either leg regresses, names which leg, and carries a **durable positive control** so a missing or renamed artifact cannot pass silently (confirmed real today: `grep -aq` on an absent path returns non-zero, the `if` is false, and the step prints success). `[explicit-skip: gate is shell in YAML, not unit-testable]` +- [ ] Factory registration still works through the retargeted holder on both legs. `[trimmed-harness]` for static (`TrimmingTests/Program.cs` resolves the delegate and the harness exits non-zero on failure); `[integration]` for relay, which structurally cannot be covered in the trimmed harness because `RelayHandlerRenderer.cs:82` guards every `RegisterHandler` behind `IsServerRuntime` — name the untrimmed suite that covers it. Needed because `AddRemoteFactoryServices.cs:170` uses `method?.Invoke`, so a misnamed holder method fails **silently**, and every other bullet here is an absence assertion that passes *more* easily when registration is dead. +- [ ] Finding B9 is closed: every new harness target has a **recorded pre-fix measurement**, and each leg's post-fix result is stated against it — not merely that the target files exist. Measured 2026-08-13, so the required outcome now differs per leg and is fixed in advance rather than read off the result: **relay** present pre-fix → must be absent post-fix; **interface-factory** absent pre-fix → must stay absent (no regression); **Save/Can\*** present pre-fix → **expected to stay present**, because its cause is [TRIM-009](./009-async-local-method-body-retention.md) and not this plan's defect. A post-fix Save/Can\* absence would falsify TRIM-009's diagnosis and must reopen it, not be recorded as a win. `[trimmed-harness]` +- [ ] No documentation in the repo — including the distributable skill — asserts the IP guarantee for **the two shapes this plan delivers** (`[Execute]` static factories, `[FactoryEventHandler]` classes). `[explicit-skip: documentation]` + **Scoped 2026-08-13 (code review V3).** The bullet was written unqualified and was therefore false at HEAD by this plan's own inventory: eight class-factory anchors remain accurate-only-after-TRIM-009, deliberately deferred rather than edited-then-reverted. Left absolute, the close-out audit could only tick it dishonestly or veto it. The residue is carried in [`../reviews/008-doc-anchor-inventory.md`](../reviews/008-doc-anchor-inventory.md) as a release-blocking table, and the release is already gated on TRIM-009 by AC6 and deferred item 2. +- [ ] Full solution build/test green (net9.0 + net10.0), both solutions, harness exits 0. `[explicit-skip: build/test gates]` + +--- + +## Current State (Pre-Flight) + +Walked 2026-08-12 on branch `TRIM` (`25ac975`). Diagnosis verified at the keyboard against a published trimmed artifact — the explicit lesson from TRIM-005. + +- **The defect.** `StaticFactoryRenderer.cs:41` and `RelayHandlerRenderer.cs:32` point the registrar attribute at the user's own class; `ClassFactoryRenderer.cs:54` and `InterfaceFactoryRenderer.cs:48` correctly point at the generated `{X}Factory`. The DAM (`FactoryAttributes.cs:148-170`, on **both** ctor param and `Type` property) then retains every method on the target. `attr.Type` is used for exactly one `GetMethod` and nothing else (`AddRemoteFactoryServices.cs:160-173`). +- **Why the two legs differ.** Class/interface factories get a separate generated type to host `FactoryServiceRegistrar`. Static factories (`StaticFactoryRenderer.cs:53,65,88-90`) and relay handlers (`RelayHandlerRenderer.cs:42,45`) instead re-open the **user's own partial** — there was no other type to name. Introduced in v0.21.2 (2026-03-08); its plan records the intent but not this consequence. +- **Trimmed-artifact probe (as of the 2026-08-12 walk, before any fix):** `_DoWork`, `_ProcessRecord`, `IServerOnlyRepository`, `DoServerWork` **present**; `ServerOnlyDirect`, `ServerOnlyHelper`, `ServerOnlyRepository` absent. *(Superseded by measurement, kept as the pre-flight baseline: after Step 3 all four of the "present" names went absent. The 2026-08-13 probe is the current picture.)* +- **The remedy cannot be "move the registrar."** `[Execute]` methods are `private static` by convention (`AllPatterns.cs:362-368`; `TrimTestCommands.cs:26,39`) and the registrar body calls them (`StaticFactoryRenderer.cs:193`), so a sibling holder that *hosts* the registrar is CS0122. The relay leg applies no accessibility filter (`FactoryGenerator.RelayHandler.cs:74-114`) despite its doc comment claiming "non-private", so private handlers compile today. Design work surfaced this as a **second, unstated reason** the TRIM-005 advisory may have reached for a nested type; the only written record of that advisory gives a DAM-breadth rationale, and no claim is made here about what its author actually intended. Forwarding satisfies the accessibility constraint without depending on unverified ILLink behavior — that is the load-bearing point, independent of anyone's intent. +- **Harness cannot see either defect.** `[Execute]` targets exist but nothing asserts their absence; `IServerOnlyRepository` is explicitly exempted by the CI grep. There is **no `[FactoryEventHandler]` class in the harness at all** — that leg is unexercised, not merely unasserted. +- **The CI exemption is unjustified.** `.github/workflows/build.yml:109-112` cites "guarded-dead `LocalCreate` bodies the trimmer keeps — tracked as TRIM-005", a cause this arc disproved. Three other artifacts repeat or contradict it: `TrimmingTests/README.md:30-31`, `TrimTestCommands.cs:35`, and `ServerOnlyTypes.cs:4-6` — the last says the interface *should be absent*, contradicting the other three. The deferred-work table listed only three of the four. +- **Doc inventory: ~40 anchors.** Verified do-NOT-touch (class/interface-scoped, still true): `docs/trimming.md:25,27,36` (under "### Class Factories — Conditional Guards" at `:19` and the interface half of `:33`), `skills/.../class-factory.md:318,333-334` (under "## Internal Visibility for Child Entities"), `advanced-patterns.md:227`. The deferred table over-listed the skill entries; abandoned TRIM-005 targeted `:36` while missing `:35`, the actual falsehood. +- **Latent bugs found during design, recorded not fixed:** nested `[Factory]` static classes and nested handler classes emit uncompilable code (simple-name FQN plus a namespace-scope re-declaration of the user's class); a class carrying both `[Factory]`(static) and `[FactoryEventHandler]` emits duplicate registrars (CS0111). +- **`NormalizeWhitespace` has no error signal** (`FactoryRenderer.cs:100-108`, `:55-58`): malformed emission yields mangled output, not an exception. Relay output bypasses normalization entirely (`FactoryGenerator.cs:104`). + +--- + +## Test Evidence + +Filled 2026-08-13, after implementation, before the gates. + +| Acceptance bullet (short) | Tier declared | Test method | Tier confirmed | +|---|---|---|---| +| `[Execute]` bodies absent from trimmed client | `[trimmed-harness]` | `verify-trimmed.sh` "static factory" block. **Discriminators [D]:** `_DoWork`, `_ProcessRecord`, `IServerOnlyRepository`, `DoServerWork` — all four recorded PRESENT in the pre-fix walk (this plan's Current State) and absent in the current probe. `IServerOnlyRepository`'s flip is the evidence that justifies deleting the `(?]`; it cannot deliver the class-factory shape, which measurement moved from "believed safe" to "known broken". **User decision 2026-08-13: AC6 is held whole, not narrowed.** Item 18 becomes [TRIM-009](./009-async-local-method-body-retention.md) and the release waits for both plans. TRIM-008's own scope is unchanged — it continues at Step 4 — but it no longer closes AC6 by itself, and its close-out must not claim to. +- **Acceptance bullet for B9 rewritten** to fix each leg's required post-fix outcome in advance. Left as "present pre-fix → absent post-fix" it would have been *failed* by the interface leg (correctly absent both times) and would have quietly accepted whatever the Save/Can\* leg did. + +--- + +## Abandonment / Retirement Reason + + + +--- + +## Notes + +- Folded into the arc by user decision 2026-08-12, reversing the 2026-08-11 decision to fix it in plan mode outside any todo. The close-out audit had flagged that it was release-blocking with no durable home; this gives it one. +- Adds **AC6** to the todo. This plan is what reopens AC4/AC5 and unblocks zTreatment PCB-003. +- Retires deferred items 1, 6 (B9), 7, and 8 when it lands. Items 3, 5, 9, 10 stay open; items 11, 12, 13 stay accepted-with-reason. +- The verification design pass for this plan died on an API error and returned nothing; the verification approach here is unreviewed by a second party. The plan-review gate is the compensating control. + +**2026-08-13 — Steps 5 and 8 executed.** + +- **Step 5 (attribute contract).** `NeatooFactoryRegistrarAttribute` now carries the rule that was only ever implicit: the `Type` must be a generated registrar type, never a consumer's class, because the DAM retains every method on it, bodies included. Documented on the type, the ctor parameter, and the `Type` property, with the reason narrating the actual defect rather than stating a rule with no motive. Also records the two things a future editor would otherwise re-litigate: why the DAM is deliberately not narrowed (no sub-method granularity; narrowing unroots prebuilt-library registrars), and that the method name is looked up as a literal and invoked null-conditionally, so a rename fails silently. +- **Step 8 (CI gate) — the exemption is gone.** `(?` XML docs, `attributes-reference.md`, the skill's static-factory and trimming pages, `AllPatterns.cs`). Bucket 4's two anchors rewritten so "preserve all methods on the referenced type" now says which type that is and why it is never the consumer's. +- **A verification trap was documented for users, not just for us.** `docs/trimming.md` and the skill both told readers to `grep -a` the published DLL for server-only names. That works for type and method names (UTF-8 metadata) and silently fails for string literals (UTF-16), which is how our own probe nearly reported a broken leg as clean. Both pages now carry the `tr -d '\000'` step and the instruction to prove the check against an untrimmed build first. +- **Design-surface disposition honored.** `CLAUDE-DESIGN.md` and `AllPatterns.cs` are corrected as documentation; the absence of a *demonstrating Design test* is accepted with reason, recorded in `AllPatterns.cs` itself so the next reader sees why rather than assuming an oversight. +- **Step 10 container reconciliation:** deferred items 6 and 7 CLOSED, item 8 partially closed with its premise corrected. Items 1 and 2 remain open pending merge and TRIM-009. +- **Verification:** main solution 611+611 unit / 561+561 integration green on net9.0 and net10.0, nothing filtered; Design solution 86+86 green; trimmed harness exits 0; `verify-trimmed.sh` passes on the fixed artifact and fails on both archived pre-fix artifacts. `FactoryEventRelayTests` was separately proven a pre-existing flake (identical failures with this branch's changes stashed) but was NOT excluded from the gate runs and passes in them. + +**Gates: both closed.** [`../reviews/008-test-review.md`](../reviews/008-test-review.md) and [`../reviews/008-code-review.md`](../reviews/008-code-review.md), three passes each, all findings closed or accepted with reason. Evidence archived in [`../reviews/008-evidence/`](../reviews/008-evidence/). This plan delivers two of AC6's three shapes and must not be closed out as delivering AC6. + +**2026-08-13 — gate findings addressed.** `test-reviewer` (4 must-cover, 7 should-cover) and `code-reviewer` (3 veto, 8 callout) both ran; every checkable finding was independently re-verified at the keyboard before being accepted, and all of them held. + +**The measurement that changed a diagnosis.** Closing P2/S7 — adding async targets to three legs — produced a result that contradicts TRIM-009's stated cause. Async `[Execute]`, async relay handlers, and an async interface-factory method **all trim clean**; only the class-factory write path leaks. It narrows rather than falsifies: none of the three clean shapes is the leaking shape (static/relay use a *wrapping* guard inside a non-async registrar, and the interface leg reaches its implementation through an interface so no body is statically reachable). The leak needs an early-throw guard **and** `async` **and** a direct call on a concrete type. TRIM-009's stub now carries the five-shape table and an explicit instruction not to treat "async is the cause" as settled. + +**Veto-tier:** +- **V1 — the rewritten gate had silently dropped a marker the old gate carried.** `grep -F "IServerOnlyRepository"` cannot match the bare implementation name that `(?`/`IServiceCollection`/`ILogger` were previously *error types* now bind, so the generator's semantic model sees different symbols suite-wide. Strengthening, not weakening (it is why `StaticFactorySource` needed its usings), but every pre-TRIM-008 green was obtained under a narrower compilation. +- **C7** — nested types, global namespace, and generic containing types were **traced** in the generator rather than assumed: name derivation is unchanged in kind, the holder binds exactly where the old attribute target did, and no new error is added to the already-broken shapes (deferred items 14/15). +- **Structural corroboration for byte-identity:** `git diff --name-only 25ac975..HEAD -- src/Generator/` returns exactly the two renderers. Because `Generated/` is gitignored, the 732/40/692 measurement is not re-derivable by a later reader; this one-command check is, and it makes the constraint structural rather than observational. 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 new file mode 100644 index 00000000..a167e3f0 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/009-async-local-method-body-retention.md @@ -0,0 +1,137 @@ +# TRIM-009 — Async `Local*` factory-method body retention + +**Plan #:** 009 +**Date:** 2026-08-13 +**Related Todo:** [../todo.md](../todo.md) +**Status:** Stub +**Last Updated:** 2026-08-13 +**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. + +--- + +## Scope + +Make the class-factory leg deliver RemoteFactory's IP-protection guarantee for **async** factory operations. Today it does not: `[Remote]` method bodies reached from an `async` generated `Local*` method — and with them their `[Service]` interfaces, their called member names, and their string literals — survive on a publish-trimmed client and are decompilable. + +This is the shape `docs/trimming.md`, the distributable skill, and `CLAUDE-DESIGN.md` all present as the one that trims correctly, so the doc surface is in scope alongside whatever the code fix turns out to be. + +**Not** in scope: the registrar-DAM defect on `[Execute]` and `[FactoryEventHandler]` shapes — that is [TRIM-008](./008-registrar-dam-over-preservation.md), which lands first and does not reach this. + +--- + +## Measured evidence (2026-08-13, pre-fix probe on branch `TRIM-008-registrar-dam-over-preservation`) + +Recorded here so this plan starts from a measurement rather than an inherited story — the arc has lost a plan to the latter once already (TRIM-005). + +**The observation.** In the harness's Save/Can\* leg (`TrimSaveTarget`), a publish-trimmed client retains `ISaveLegPort`, `SaveLegInvoke`, and all three body literals `SaveLegInsertBody_MARKER` / `SaveLegUpdateBody_MARKER` / `SaveLegDeleteBody_MARKER`. + +**The controlled comparison.** Within the *same* assembly, `TrimTestEntityFactory.LocalCreate` and `TrimSaveTargetFactory.LocalInsert/Update/Delete` differ in exactly one respect — `async`: + +| | `LocalCreate` | `LocalInsert` / `LocalUpdate` / `LocalDelete` | +|---|---|---| +| Guard | `if (!NeatooRuntime.IsServerRuntime) throw` | identical | +| Server-only reach | `GetRequiredService()` | `GetRequiredService()` | +| Rooted by | unguarded `AddScoped` closure in `FactoryServiceRegistrar` | unguarded `AddScoped` closure | +| DAM-preserved | yes | yes | +| `async` | **no** | **yes** | +| Post-guard body after trimming | **eliminated** (`IServerOnlyRepository`, `DoServerWork` absent) | **retained** | + +**Corroboration.** Surviving state machines in the trimmed DLL: `d__15`, `d__16`, `d__17`, `d__21`. `d__` does not exist — it is not an async method. The feature-switch fold happens inside `MoveNext`, and the remainder is not eliminated there. + +**What is deliberately NOT claimed.** No assertion is made here about *why* ILLink treats the two cases differently. The statement that survives scrutiny is the empirical one in the table. Any causal story about ILLink internals must be re-derived against the artifact before it is built on. + +**Why the TRIM-008 remedy does not apply — corrected 2026-08-13 after code review.** + +The first draft of this paragraph argued: *"the attribute correctly names the generated `TrimSaveTargetFactory`"*, as though naming a generated type settled it, and cited the static-leg fix leaving these markers unchanged as confirmation. **Both halves were wrong**, and are recorded here rather than quietly rewritten because building on them would have cost a cycle. + +- `TrimSaveTargetFactory` is *itself* the type that hosts the leaking bodies — `LocalInsert`, `LocalUpdate`, `LocalDelete`, `LocalSave` are all its members. Its assembly attribute carries `DynamicallyAccessedMembers(PublicMethods | NonPublicMethods)`, so **DAM is live on this leg and is rooting those bodies**. "Generated, not consumer" is not the operative distinction; what makes TRIM-008's holders safe is that a holder has exactly *one* method. +- The static-leg measurement could not confirm anything about this leg. Fixing `TrimTestCommands`'s attribute has no bearing on `TrimSaveTargetFactory`'s attribute, which still names the factory. That was a non-sequitur. + +**What actually supports the conclusion**, verified in the emitted code: `ClassFactoryRenderer` emits its delegate registrations with **no `IsServerRuntime` guard** (unlike `StaticFactoryRenderer`), producing an `AddScoped` closure that captures the factory and calls `LocalSave`, which routes to `LocalInsert`. That is an independent root chain. So a forwarding holder alone would not clear these markers — **but the holder indirection is plausibly part of the eventual fix rather than orthogonal to it**, because DAM is rooting them too. A guard-only remedy would ship with DAM still holding `LocalInsert`. + +--- + +## The "async" hypothesis, narrowed by measurement (2026-08-13) + +The original framing — "async generated `Local*` methods retain their server-only bodies; sync ones do not" — was drawn from a two-case comparison. Three async targets were then added to the harness and **all trim clean**: + +| Shape | Guard | Reaches server-only code via | `async` | Body rooted at all? | Result | +|---|---|---|---|---|---| +| static `[Execute]` | `if (IsServerRuntime) { … }` wrapping, in a non-async registrar | direct static call | yes | **no** — fold deletes the registration | absent (moot) | +| relay handler | same wrapping guard | direct static call | yes | **no** — same | absent (moot) | +| interface factory | `if (!IsServerRuntime) throw` early | **the interface** | yes | yes, but markers are behind the interface hop | **not measurable** | +| class factory `Create` | `if (!IsServerRuntime) throw` early | direct call on the concrete type | **no** | yes | clean | +| class factory `Insert`/`Update`/`Delete` | `if (!IsServerRuntime) throw` early | direct call on the concrete type | **yes** | yes | **LEAKS** | + +**Read the first three rows carefully — they are weaker than they look**, and the first draft of this table overstated them as "clean". + +- **Static and relay** put their guard in a *wrapping* block inside the non-async `FactoryServiceRegistrar`, so the fold deletes the whole registration and the async body is never rooted. Their markers being absent is real evidence that TRIM-008's fix generalizes to async `[Execute]` (`_DoAsyncWork` is a method on the consumer's class, exactly what the DAM used to retain) — but it says nothing about async fold behaviour, because that mechanism is never reached. +- **Interface factory cannot measure this property at all.** Its markers sit on the implementation, which the generated body reaches through an interface, so they read absent whether or not the body survives. This is structural: an interface factory reaches everything through interfaces. The direct fix — a `[Service]` parameter putting `GetRequiredService()` into the generated body — was attempted and **does not compile** (Deferred Work item 19). + +**So the wider hypothesis is narrowed, not confirmed** — and the cross-class pair originally offered as "single-variable" was not. `TrimTestEntityFactory.LocalCreate` vs `TrimSaveTargetFactory.LocalInsert` differ in at least four further ways: an `[AuthorizeFactory]` block, target-from-DI vs target-from-parameter, **one-hop vs two-hop rooting** (there is no `InsertDelegate`; `Insert` is reached through `SaveDelegate` → `LocalSave`), and an extra catch arm plus lifecycle probes. That last one matters most, because the arc's own disproven TRIM-004 story blamed exactly *"early-throw guard + try/catch defeats unreachable-code elimination"*. + +## The controlled experiment (2026-08-13) — `async` confirmed + +Run at TRIM-008's re-review rather than deferred to this plan, because it decides this plan's scope. `TrimTestEntity` gained an `async [Remote][Fetch] FetchAsync` beside its existing sync `[Remote][Create] Create`, each writing its own literal into its own body: + +| | `ClassSyncBody_MARKER` | `ClassAsyncBody_MARKER` | +|---|---|---| +| Body | `TrimTestEntity.Create` | `TrimTestEntity.FetchAsync` | +| Declaring type / generated factory / registrar | identical | identical | +| `[AuthorizeFactory]` | none | none | +| Rooting | one hop, own delegate | one hop, own delegate | +| Reached by | direct call on the concrete type | direct call on the concrete type | +| Literal position | in the domain body | in the domain body | +| `async` | **no** | **yes** | +| Untrimmed | PRESENT | PRESENT | +| **Trimmed** | **absent** | **PRESENT** | + +Every confound listed above is controlled, and both halves are rooted **twice and identically** — by DAM on `TrimTestEntityFactory` (the assembly attribute names the generated factory, which hosts both methods) and by their own unguarded delegate registration. That closes the "maybe the sync one simply was not rooted" alternative outright. + +**`async`-shaped emission is the operative variable for the class-factory leg**, and this plan's scope is correct. + +### But the sub-cause is undetermined, and it changes the fix + +Say "async-shaped emission", not "the `async` keyword". Five constructs appear in the async body and not the sync one, all emitted *because* the method is async and therefore inseparable from outside the generator: + +1. an extra `catch (OperationCanceledException)` arm +2. `if (target is IFactoryOnStartAsync) await …` +3. `if (target is IFactoryOnCompleteAsync) await …` +4. `if (target is IFactoryOnCancelled) …` (inside the OCE arm) +5. `if (target is IFactoryOnCancelledAsync) await …` (inside the OCE arm) + +Items 2–5 are **interface type-tests**, a mechanically different ILLink retention path from a state machine: a type-test against a rooted type can keep its branch alive independently of any `MoveNext` fold. So two hypotheses survive the experiment and it cannot separate them: + +- **H1 — the fold does not propagate through the state machine.** The switch folds inside `MoveNext` and the remainder survives. +- **H2 — the fold works, and unreachable-code elimination is defeated** by the second catch arm and/or the async lifecycle type-tests. **This is the disproven TRIM-004 story returning in async-only form** — the arc set that story aside for the wrong reason and never re-tested it. + +**They imply different remedies.** Under H1 the guard must move out of the async method entirely (a sync wrapper testing `IsServerRuntime` before calling the async body). Under H2 the state machine is innocent and the fix is to restructure catch/probe emission — and note `TrimTestEntity` implements **none** of those four interfaces while `target` is statically typed `TrimTestEntity`, so simply not emitting probes for interfaces the concrete type provably cannot implement may be the whole fix. + +**Separate them before choosing a remedy.** From inside the generator it is easy: emit a *sync* `Local*` carrying a second catch arm, or an *async* one without the probes, and re-probe. That is this plan's first step, not its design conclusion. + +**One remedy is ruled out already:** de-rooting. DAM on `typeof(TrimTestEntityFactory)` will always root `LocalFetchAsync` regardless of registration changes, so no amount of guarding the delegate registrations removes the root. + +**Free variable, controlled and harmless:** the sync half resolves two server-only services and the async half one. It cuts the safe way — the sync body has strictly *more* server-only reach and still folds clean. It is also load-bearing for the harness: giving the async half `IServerOnlyRepository` would surface that name in the trimmed output and turn the gate's static-factory `[D]` markers red for a misleading reason. The asymmetry must stay until this plan lands. + +Corroboration from the same run: `IClassLegPort` and `ClassLegInvoke` flipped to PRESENT once `FetchAsync` existed — retained by its in-body `GetRequiredService()` — mirroring `ISaveLegPort`/`SaveLegInvoke` on the save leg. The gate caught that flip on its first run after the target landed, which is the per-leg attribution working. + +**Still open:** whether the early-throw guard shape and the direct-concrete-call shape are *necessary* as well. Neither has independent evidence — the static/relay rows cannot discriminate (over-determined) and the interface row cannot go red at all. Do not present them as established conditions. + +**For the design turn:** do not treat "async is the cause" as established. The controlled pair says `async` is the differing variable *within the class-factory leg*; whether the operative mechanism is the async state machine, the early-throw guard shape, or their combination is not settled, and the remedy differs for each. Re-derive it against the artifact before building on it. + +**Baseline inheritance.** This plan does **not** need its own pre-fix probe. The 2026-08-13 measurement above *is* the baseline, captured before any fix to this leg existed, with markers proven visible by a self-check against the untrimmed assembly. Harness targets, per-leg ports, and the probe script all land with TRIM-008. + +--- + +## Open questions for the design turn + +- Is the remedy generator-side (guard the delegate registrations, restructure the emitted guard so the fold is not inside `MoveNext`, keep server-only work out of async `Local*` bodies) or configuration-side (an ILLink feature/substitution the generator emits)? Unknown; do not assume. Whichever it is, check whether the DAM root also has to be addressed — see the corrected paragraph above. +- ~~Does the same retention affect **async `[Execute]`** and **async relay handlers**?~~ **Answered 2026-08-13: no.** Async targets were added to the harness for both legs plus an async interface-factory method, and all trim clean. See the table above for why none of them is the leaking shape. +- Would changing the emitted guard from `if (!IsServerRuntime) throw …` to a *wrapping* `if (IsServerRuntime) { … }` block fix this without touching DAM or the registrations? Worth trying because it is cheap, **but note that no measurement points at it**: the static and relay legs use that shape and are clean, yet their cleanliness is over-determined (post-TRIM-008 they are no longer DAM targets *and* their only reference sits inside the folded block), so they cannot show the guard shape is what does the work. Treat this as an untested idea, not as something the evidence suggests. +- **First step, before any design work: separate H1 from H2.** Emit a *sync* `Local*` with a second catch arm, or an *async* one without the lifecycle type-tests, and re-probe. The controlled experiment cannot do this from outside the generator; from inside it is cheap. Choosing a remedy before separating them is how TRIM-004 → TRIM-005 was lost. +- 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. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-code-review.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-code-review.md new file mode 100644 index 00000000..dbcbb75a --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-code-review.md @@ -0,0 +1,66 @@ +# TRIM-008 — Code Review (per-plan, opt-in) + +**Gate:** Step 5, opted in (`Code-review opt-in: Yes`). **Passes:** three (2026-08-13). Findings-only; no grade. +**Evidence set:** [`008-evidence/`](./008-evidence/) — manifest in [`008-test-review.md`](./008-test-review.md). + +Every checkable finding was independently re-derived at the keyboard before being accepted. **All of them held.** + +--- + +## Standing conclusions (unchanged across all three passes) + +**The generator change is correct and minimal.** `git diff --name-only 25ac975..HEAD -- src/Generator/` returns exactly two files: `StaticFactoryRenderer.cs` and `RelayHandlerRenderer.cs`. No model, builder, transform, or dispatch change. That is a **structural** argument for the byte-identity constraint — stronger than the 732/40/692 measurement, because `Generated/` is gitignored and the measurement is not re-derivable by a later reader while this one-command check is. It also makes the incremental-cache constraint literally true: `RelayHandlerModel`/`TypeInfo` gain nothing and `IncrementalCacheTests` is untouched. + +**The holder shape is right.** Top-level `internal static`, one forwarding method, distinct prefix per leg — reusing the proven in-tree `EventPreservationRenderer` shape rather than the nested-type variant that rested on unverified ILLink behaviour. Forwarding rather than hosting is load-bearing: `[Execute]` methods are `private static` and the registrar body calls them, so a hosting sibling would be CS0122. `AddRemoteFactoryServices` reaches the `internal static` holder method via `BindingFlags.Static | NonPublic | Public`. + +**`FactoryAttributes.cs` is XML-doc-only**, verified member by member: no new members, no signature, accessibility, or attribute-argument change. + +**All five plan-review vetoes were genuinely addressed**, not merely claimed — including the B1 step reordering, visible in commit order. + +**No new trim-analysis surface:** 25 `IL2xxx` warnings in the publish log, none naming either holder; all pre-existing library code. + +--- + +## Pass 1 — 3 veto, 8 callout + +| # | Finding | Disposition | +|---|---|---| +| V1 | **The rewritten gate silently dropped a marker the old gate carried.** `grep -F "IServerOnlyRepository"` cannot match the bare implementation name that `(?`/`IServiceCollection`/`ILogger` were previously *error types* now bind. Strengthening, not weakening — but every pre-TRIM-008 green was obtained under a narrower compilation. +- **C7 — nested types, global namespace, generic containing types** were *traced* in the generator rather than assumed. Name derivation is unchanged in kind, the holder binds where the old attribute target did, and the already-broken shapes (deferred items 14/15) gain no new error. +- **N4 (relay registration) — the holder *type* is covered by a full-name control, but DAM roots that type regardless.** What remains uncovered is whether the handler *registration* fires, which a client-side trimmed harness structurally cannot signal. Covered untrimmed by the integration suite. + +## Verdict + +**The deliverable is done.** The generator change has been stable and correct across three passes; the harness, gate, and tests are now the strongest artifacts in this arc. What blocked Done at pass 3 was bookkeeping — a plan contradicting its own artifact, an understated acceptance criterion, and two missing review records — all closed here. + +**TRIM-008 delivers two of AC6's three shapes and must not be closed out as delivering AC6.** The third is TRIM-009, on which the v1.7.0 release remains blocked. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-doc-anchor-inventory.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-doc-anchor-inventory.md new file mode 100644 index 00000000..d912584a --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-doc-anchor-inventory.md @@ -0,0 +1,79 @@ +# TRIM-008 — Documentation Anchor Inventory (Step 9 working list) + +**Built:** 2026-08-13, by reading the files rather than copying the plan's list. +**Why it exists:** plan review A4. This inventory has been wrong twice in this arc — once by *over-listing* (skill class-factory entries flagged as falsified when they described a leg that worked) and once by *under-listing* (abandoned TRIM-005 targeted `docs/trimming.md:36`, the true statement, while missing `:35`, the false one). Enumerating before editing is the correction. + +--- + +## Inventory revision: the plan's "do NOT touch" list is now partly wrong + +The plan's Files section lists these as *verified do-NOT-touch, class/interface-scoped, still true*: +`docs/trimming.md:25,27,36`, `skills/.../class-factory.md:318,333-334`, `advanced-patterns.md:227`. + +**The 2026-08-13 pre-fix probe falsified the class-factory half of that.** Async generated `Local*` methods retain their server-only bodies (TRIM-009), so every anchor asserting that `[Remote] internal` class-factory bodies are trimmed is **false today** for any aggregate root with async operations — reads as well as writes, both measured 2026-08-13 — which is nearly all of them. + +**Disposition: do not edit them in TRIM-008.** TRIM-009 makes them true again, and nothing ships until both land (AC6 held whole; deferred item 2 gates the release on both). Editing them to say "leaks" and reverting after TRIM-009 is churn that would also publish a scarier claim than the shipped state ever has. + +**What makes that safe rather than a repeat of this arc's habit:** the anchors are enumerated below, the dependency is named, and the release is already blocked on TRIM-009. If TRIM-009 is ever abandoned or descoped, this table is the checklist that must be worked before any release. + +### TRIM-009-dependent anchors (deferred, tracked, release-blocking) + +| Anchor | Claim | Status | +|---|---|---| +| `docs/trimming.md:25` | `[Remote] internal` → "Method body trimmed" | False for async ops (read AND write — measured 2026-08-13) | +| `docs/trimming.md:27` | `internal` (no `[Remote]`) → "Method body trimmed. Server-only." | Same | +| `src/Design/CLAUDE-DESIGN.md:648,650` | Same visibility/guard table | Same | +| `src/Design/CLAUDE-DESIGN.md:653` | "`[Remote]` requires `internal` so the IL trimmer can remove method bodies from client assemblies" | Same | +| `skills/RemoteFactory/references/trimming.md:10` | "method bodies trimmed on client" | Same | +| `skills/RemoteFactory/references/trimming.md:11` | Child entity methods "removed from client" | Same | +| `skills/RemoteFactory/references/trimming.md:14` | "no server-only logic, no server-only dependencies, no IP exposure" | Same — the strongest claim in the set | +| `docs/client-server-architecture.md:133` | "removes server-only method bodies … and the decompilable business logic" | Same | +| `skills/RemoteFactory/references/trimming.md` — "What Gets Trimmed, By Factory Shape" table, class-factory row | **Added by TRIM-008.** Says class-factory bodies are removed "from v1.7.0 — synchronous operations were always removed; `async` ones needed the same release" | **True only once TRIM-009 lands.** This is the one place TRIM-008 wrote a forward-looking claim rather than deferring, because the table's whole purpose is a shape-by-shape guarantee and omitting the row would be its own silence. It is release-blocking: if TRIM-009 is descoped, this row must be rewritten before shipping. | + +--- + +## Bucket 1 — Falsified by the defect, made true by TRIM-008 (edit now) + +| Anchor | Problem | +|---|---| +| `docs/trimming.md:35` | Static factories: "The trimmer removes the registration lambdas and their captured dependencies." Was false — bodies were DAM-retained. True after the fix, but the stated mechanism omits why it now works. | +| `docs/trimming.md:13` | "method bodies, server-only types, and their transitive dependencies all disappear" — stated unconditionally for all shapes. | +| `src/Design/CLAUDE-DESIGN.md:763-766` | Attribute-target table: Static Factory row says `typeof({Namespace}.{StaticClassName})` — **the consumer's class**. Now factually wrong about emitted output, and it documents the defect as intended design. No relay-handler row at all. | +| `skills/RemoteFactory/references/trimming.md` (new section) | Skill never mentions that `[Execute]`/`[FactoryEventHandler]` have their own preservation story. | + +## Bucket 2 — Falsified TRIM-005 artifacts (4, not the 3 the deferred table listed) + +| Anchor | Problem | Status | +|---|---|---| +| `.github/workflows/build.yml:109-112` | `(?` XML doc | No trimming content | +| `src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs` — `ExampleCommands` | No trimming commentary despite being the Design source of truth for static factories | + +## Bucket 4 — Accurate today, made inaccurate BY the fix + +The bucket the first draft missed. These correctly describe "preserve all methods on the referenced type" — precisely the property the fix removes for two of four shapes. + +| Anchor | Problem | +|---|---| +| `docs/trimming.md:222` | "instruct the trimmer to preserve all methods on the referenced type" — still true of the *mechanism*, but the referenced type is now a generated holder, and the sentence reads as though it is the factory. | +| `src/Design/CLAUDE-DESIGN.md:756` | "ensuring each factory type's `FactoryServiceRegistrar` method (and all other methods) survive trimming" — "and all other methods" is exactly the defect, stated as a feature. | + +## Verified do-NOT-touch (still true after measurement) + +| Anchor | Why it stands | +|---|---| +| `docs/trimming.md:26` | `public` non-`[Remote]` bodies survive — correct, and unaffected | +| `docs/trimming.md:36` | Interface factories unreachable to the trimmer — all iface markers absent, sync and async. **But the leg cannot in principle report on body elimination:** it reaches its implementation through the interface, so those markers are absent by fixture shape either way (deferred item 19 — the `[Service]` fix that would give it a reachable marker does not compile). Left standing because nothing contradicts it, not because it was measured. Corrected 2026-08-13; this row previously read "measured true", which the leg cannot deliver | +| `docs/trimming.md:42` | Feature-switch mechanism — accurate | +| `src/RemoteFactory/NeatooRuntime.cs:5-9` | Describes the switch, claims nothing about which shapes benefit | diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-baseline-prefix.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-baseline-prefix.txt new file mode 100644 index 00000000..4d7c1b48 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-baseline-prefix.txt @@ -0,0 +1,10 @@ +-- positive controls +::error::Positive control 'NeatooFactoryRegistrar_TrimTestCommands' is MISSING from the trimmed assembly. Either registration was silently removed, or this gate is not reading a real trimmed artifact — do not trust the absence results below. +::error::Positive control 'NeatooEventHandlerRegistrar_TrimRelayHandlers' is MISSING from the trimmed assembly. Either registration was silently removed, or this gate is not reading a real trimmed artifact — do not trust the absence results below. +::error::Positive control 'NeatooEventHandlerRegistrar_TrimAsyncRelayHandlers' is MISSING from the trimmed assembly. Either registration was silently removed, or this gate is not reading a real trimmed artifact — do not trust the absence results below. + ok TrimTestCommands +::error::Positive control 'ITrimIfaceQueryFactory' is MISSING from the trimmed assembly. Either registration was silently removed, or this gate is not reading a real trimmed artifact — do not trust the absence results below. +::error::Positive control 'ITrimAsyncIfaceQueryFactory' is MISSING from the trimmed assembly. Either registration was silently removed, or this gate is not reading a real trimmed artifact — do not trust the absence results below. +::error::Positive control 'ITrimSaveTargetFactory' is MISSING from the trimmed assembly. Either registration was silently removed, or this gate is not reading a real trimmed artifact — do not trust the absence results below. + ok +::error::6 positive control(s) failed. This artifact is not a trustworthy trimmed build of the current harness — absence and presence results are NOT being reported, because they would be meaningless and their remediation advice actively misleading. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-missing.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-missing.txt new file mode 100644 index 00000000..dde9799f --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-missing.txt @@ -0,0 +1 @@ +::error::Trimmed assembly not found at 'C:/Users/keith/AppData/Local/Temp/claude/C--Users-keith-source-repos-neatoodotnet-RemoteFactory/f1851286-87dc-40c8-a14e-6c005df7aef1/scratchpad/nope.dll'. The gate read nothing, so it proved nothing. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-nofold.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-nofold.txt new file mode 100644 index 00000000..9d1ce28c --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-nofold.txt @@ -0,0 +1,57 @@ +-- positive controls + ok NeatooFactoryRegistrar_TrimTestCommands + ok NeatooEventHandlerRegistrar_TrimRelayHandlers + ok NeatooEventHandlerRegistrar_TrimAsyncRelayHandlers + ok TrimTestCommands + ok ITrimIfaceQueryFactory + ok ITrimAsyncIfaceQueryFactory + ok ITrimSaveTargetFactory + ok +-- static factory ([Execute]) +::error::[static factory] '_DoWork' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[static factory] '_ProcessRecord' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[static factory] 'DoServerWork' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[static factory] 'IServerOnlyRepository' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[static factory] 'ServerOnlyRepository_MARKER' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[static factory] 'ServerOnlyDirect' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[static factory] 'ServerOnlyHelper' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[static factory] 'ServerOnlyHelper_MARKER' found in the trimmed assembly. Server-only code is shipping to clients. +-- static factory, async body +::error::[static factory (async)] '_DoAsyncWork' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[static factory (async)] 'StaticAsyncBody_MARKER' found in the trimmed assembly. Server-only code is shipping to clients. +-- relay handler ([FactoryEventHandler]) +::error::[relay handler] 'RelayLegHandlerBody' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[relay handler] 'IRelayLegPort' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[relay handler] 'RelayLegInvoke' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[relay handler] 'RelayLegHandlerBody_MARKER' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[relay handler] 'RelayLegBackend' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[relay handler (async)] 'AsyncRelayHandlerBody' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[relay handler (async)] 'RelayAsyncBody_MARKER' found in the trimmed assembly. Server-only code is shipping to clients. +-- interface factory +::error::[interface factory] 'TrimIfaceServerSide' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[interface factory] 'IIfaceLegPort' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[interface factory] 'IfaceLegInvoke' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[interface factory] 'IfaceLegBackend' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[interface factory] 'IfaceLegServerBody_MARKER' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[interface factory (async)] 'TrimAsyncIfaceServerSide' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[interface factory (async)] 'IfaceAsyncBody_MARKER' found in the trimmed assembly. Server-only code is shipping to clients. +-- class factory, port implementation (behind an interface hop; not a leg signal) +::error::[class factory] 'ClassLegBackend' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[class factory] 'ClassLegBackend_MARKER' found in the trimmed assembly. Server-only code is shipping to clients. +-- async-only port (shared by the three async targets above) +::error::[async port] 'IAsyncLegPort' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[async port] 'AsyncLegInvoke' found in the trimmed assembly. Server-only code is shipping to clients. +::error::[async port] 'AsyncLegBackend' found in the trimmed assembly. Server-only code is shipping to clients. +-- controlled sync/async pair (class factory) +::error::[class factory (sync half of controlled pair)] 'ClassSyncBody_MARKER' found in the trimmed assembly. Server-only code is shipping to clients. + ok ClassAsyncBody_MARKER (still present, as TRIM-009 expects) + ok IClassLegPort (still present, as TRIM-009 expects) + ok ClassLegInvoke (still present, as TRIM-009 expects) +-- save/can* (known broken, TRIM-009) + ok ISaveLegPort (still present, as TRIM-009 expects) + ok SaveLegInvoke (still present, as TRIM-009 expects) + ok SaveLegInsertBody_MARKER (still present, as TRIM-009 expects) + ok SaveLegUpdateBody_MARKER (still present, as TRIM-009 expects) + ok SaveLegDeleteBody_MARKER (still present, as TRIM-009 expects) + +::error::Trimming verification FAILED (30 check(s)). diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-prefix-relayleg.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-prefix-relayleg.txt new file mode 100644 index 00000000..426ebb3e --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/gate-red-prefix-relayleg.txt @@ -0,0 +1,10 @@ +-- positive controls + ok NeatooFactoryRegistrar_TrimTestCommands +::error::Positive control 'NeatooEventHandlerRegistrar_TrimRelayHandlers' is MISSING from the trimmed assembly. Either registration was silently removed, or this gate is not reading a real trimmed artifact — do not trust the absence results below. +::error::Positive control 'NeatooEventHandlerRegistrar_TrimAsyncRelayHandlers' is MISSING from the trimmed assembly. Either registration was silently removed, or this gate is not reading a real trimmed artifact — do not trust the absence results below. + ok TrimTestCommands + ok ITrimIfaceQueryFactory +::error::Positive control 'ITrimAsyncIfaceQueryFactory' is MISSING from the trimmed assembly. Either registration was silently removed, or this gate is not reading a real trimmed artifact — do not trust the absence results below. + ok ITrimSaveTargetFactory + ok +::error::3 positive control(s) failed. This artifact is not a trustworthy trimmed build of the current harness — absence and presence results are NOT being reported, because they would be meaningless and their remediation advice actively misleading. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/probe-prefix.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/probe-prefix.txt new file mode 100644 index 00000000..c4c45b05 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/probe-prefix.txt @@ -0,0 +1,47 @@ +=== PRE-FIX PROBE: relay leg UNFIXED, new B9 targets first measurement === +dll: src\Tests\RemoteFactory.TrimmingTests\bin\Release\net9.0\win-x64\publish\RemoteFactory.TrimmingTests.dll +size: 55808 bytes + +-- relay leg (UNFIXED at this probe) + RelayLegHandlerBody PRESENT (utf8,utf16) + IRelayLegPort PRESENT (utf8) + RelayLegInvoke PRESENT (utf8) + RelayLegHandlerBody_MARKER PRESENT (utf16) + +-- interface-factory leg (B9, never measured before) + TrimIfaceServerSide absent + IIfaceLegPort absent + IfaceLegInvoke absent + IfaceLegBackend absent + IfaceLegServerBody_MARKER absent + +-- save/can* leg (B9, never measured before) + ISaveLegPort PRESENT (utf8) + SaveLegInvoke PRESENT (utf8) + SaveLegBackend absent + SaveLegInsertBody_MARKER PRESENT (utf16) + SaveLegUpdateBody_MARKER PRESENT (utf16) + SaveLegDeleteBody_MARKER PRESENT (utf16) + +-- pre-existing absent-expected (regression) + _DoWork absent + _ProcessRecord absent + IServerOnlyRepository absent + DoServerWork absent + ServerOnlyDirect absent + ServerOnlyHelper absent + +-- positive controls (must be PRESENT) + TrimTestCommands PRESENT (utf8) + TrimRecordResult PRESENT (utf8,utf16) + ITrimIfaceQueryFactory PRESENT (utf8) + ITrimSaveTargetFactory PRESENT (utf8) + NeatooFactoryRegistrar_ PRESENT (utf8) + NeatooEventPreservationRegistrar PRESENT (utf8) + +-- measured, not asserted + TrimRelayHandlers PRESENT (utf8) + TrimSaveAuthRules PRESENT (utf8) + TrimRelayHandlerEvent PRESENT (utf8,utf16) + RelayLegBackend absent + diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/probe-selfcheck3.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/probe-selfcheck3.txt new file mode 100644 index 00000000..6ffaefee --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/probe-selfcheck3.txt @@ -0,0 +1,73 @@ +=== SELF-CHECK v3: UNTRIMMED, every marker must be PRESENT === +dll: src\Tests\RemoteFactory.TrimmingTests\bin\Release\net9.0\RemoteFactory.TrimmingTests.dll +size: 80896 bytes + +-- relay leg + RelayLegHandlerBody PRESENT (utf8,utf16) + IRelayLegPort PRESENT (utf8) + RelayLegInvoke PRESENT (utf8) + RelayLegHandlerBody_MARKER PRESENT (utf16) + +-- interface-factory leg (B9, never measured before) + TrimIfaceServerSide PRESENT (utf8) + IIfaceLegPort PRESENT (utf8) + IfaceLegInvoke PRESENT (utf8) + IfaceLegBackend PRESENT (utf8) + IfaceLegServerBody_MARKER PRESENT (utf16) + +-- save/can* leg (B9, never measured before) + ISaveLegPort PRESENT (utf8) + SaveLegInvoke PRESENT (utf8) + SaveLegBackend PRESENT (utf8) + SaveLegInsertBody_MARKER PRESENT (utf16) + SaveLegUpdateBody_MARKER PRESENT (utf16) + SaveLegDeleteBody_MARKER PRESENT (utf16) + +-- ASYNC bodies (S7/P2 - inference converted to measurement) + _DoAsyncWork PRESENT (utf8) + StaticAsyncBody_MARKER PRESENT (utf16) + AsyncRelayHandlerBody PRESENT (utf8) + RelayAsyncBody_MARKER PRESENT (utf16) + TrimAsyncIfaceServerSide PRESENT (utf8) + IfaceAsyncBody_MARKER PRESENT (utf16) + IAsyncLegPort PRESENT (utf8) + AsyncLegInvoke PRESENT (utf8) + AsyncLegBackend PRESENT (utf8,utf16) + +-- class-factory leg (S8 - own port, was sharing with static) + IClassLegPort PRESENT (utf8) + ClassLegInvoke PRESENT (utf8) + ClassLegBackend PRESENT (utf8,utf16) + ClassLegBackend_MARKER PRESENT (utf16) + +-- CONTROLLED PAIR: sync vs async in ONE class factory (V5c) + ClassSyncBody_MARKER PRESENT (utf16) + ClassAsyncBody_MARKER PRESENT (utf16) + +-- pre-existing absent-expected (regression) + _DoWork PRESENT (utf8) + _ProcessRecord PRESENT (utf8) + IServerOnlyRepository PRESENT (utf8) + DoServerWork PRESENT (utf8) + ServerOnlyDirect PRESENT (utf8,utf16) + ServerOnlyHelper PRESENT (utf8,utf16) + ServerOnlyRepository_MARKER PRESENT (utf16) + ServerOnlyHelper_MARKER PRESENT (utf16) + +-- positive controls (must be PRESENT) + TrimTestCommands PRESENT (utf8) + TrimRecordResult PRESENT (utf8,utf16) + ITrimIfaceQueryFactory PRESENT (utf8) + ITrimAsyncIfaceQueryFactory PRESENT (utf8) + ITrimSaveTargetFactory PRESENT (utf8) + NeatooFactoryRegistrar_ PRESENT (utf8) + NeatooEventHandlerRegistrar_ PRESENT (utf8) + NeatooEventPreservationRegistrar PRESENT (utf8) + Trimming verification app completed PRESENT (utf16) + +-- measured, not asserted + TrimRelayHandlers PRESENT (utf8) + TrimSaveAuthRules PRESENT (utf8) + TrimRelayHandlerEvent PRESENT (utf8,utf16) + RelayLegBackend PRESENT (utf8) + diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/probe-v5c.txt b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/probe-v5c.txt new file mode 100644 index 00000000..d504c1f6 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-evidence/probe-v5c.txt @@ -0,0 +1,73 @@ +=== V5c: sync vs async within ONE class factory === +dll: src\Tests\RemoteFactory.TrimmingTests\bin\Release\net9.0\win-x64\publish\RemoteFactory.TrimmingTests.dll +size: 66560 bytes + +-- relay leg (UNFIXED at this probe) + RelayLegHandlerBody absent + IRelayLegPort absent + RelayLegInvoke absent + RelayLegHandlerBody_MARKER absent + +-- interface-factory leg (B9, never measured before) + TrimIfaceServerSide absent + IIfaceLegPort absent + IfaceLegInvoke absent + IfaceLegBackend absent + IfaceLegServerBody_MARKER absent + +-- save/can* leg (B9, never measured before) + ISaveLegPort PRESENT (utf8) + SaveLegInvoke PRESENT (utf8) + SaveLegBackend absent + SaveLegInsertBody_MARKER PRESENT (utf16) + SaveLegUpdateBody_MARKER PRESENT (utf16) + SaveLegDeleteBody_MARKER PRESENT (utf16) + +-- ASYNC bodies (S7/P2 - inference converted to measurement) + _DoAsyncWork absent + StaticAsyncBody_MARKER absent + AsyncRelayHandlerBody absent + RelayAsyncBody_MARKER absent + TrimAsyncIfaceServerSide absent + IfaceAsyncBody_MARKER absent + IAsyncLegPort absent + AsyncLegInvoke absent + AsyncLegBackend absent + +-- class-factory leg (S8 - own port, was sharing with static) + IClassLegPort PRESENT (utf8) + ClassLegInvoke PRESENT (utf8) + ClassLegBackend absent + ClassLegBackend_MARKER absent + +-- CONTROLLED PAIR: sync vs async in ONE class factory (V5c) + ClassSyncBody_MARKER absent + ClassAsyncBody_MARKER PRESENT (utf16) + +-- pre-existing absent-expected (regression) + _DoWork absent + _ProcessRecord absent + IServerOnlyRepository absent + DoServerWork absent + ServerOnlyDirect absent + ServerOnlyHelper absent + ServerOnlyRepository_MARKER absent + ServerOnlyHelper_MARKER absent + +-- positive controls (must be PRESENT) + TrimTestCommands PRESENT (utf8) + TrimRecordResult PRESENT (utf8,utf16) + ITrimIfaceQueryFactory PRESENT (utf8) + ITrimAsyncIfaceQueryFactory PRESENT (utf8) + ITrimSaveTargetFactory PRESENT (utf8) + NeatooFactoryRegistrar_ PRESENT (utf8) + NeatooEventHandlerRegistrar_ PRESENT (utf8) + NeatooEventPreservationRegistrar PRESENT (utf8) + Trimming verification app completed PRESENT (utf16) + +-- measured, not asserted + TrimRelayHandlers PRESENT (utf8) + TrimSaveAuthRules PRESENT (utf8) + TrimRelayHandlerEvent PRESENT (utf8,utf16) + RelayLegBackend absent + diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-plan-review.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-plan-review.md new file mode 100644 index 00000000..3d59b4f4 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-plan-review.md @@ -0,0 +1,92 @@ +# TRIM-008 — Plan Review + +**Date:** 2026-08-12 +**Reviewed:** `plans/008-registrar-dam-over-preservation.md` (Draft) +**Branch:** `TRIM-008-registrar-dam-over-preservation` at `06ea1f5`, working tree dirty with the Step-6 measurement spike +**Verdict:** **CONCERNS** — 5 veto-tier, 8 callout-tier + +Two earlier attempts at this review died on API errors mid-response. The second left one usable lead (non-`[Remote]` `[Execute]` regression), which was chased down separately and ruled out — see below. + +--- + +## Scope: split NOT recommended + +The reviewer explicitly rejected splitting the plan. The doc pass is inseparable — the docs are false only while the code is broken, so a separately-mergeable doc plan would either publish a lie or publish a correction to code that has not landed. B9 closure is the *evidence* for the fix, not adjacent work. Nine steps is inside the cap. + +Flagged instead: Steps 8 (docs) and 9 (container) are the parts most likely to be under-executed at the end of a long plan, and are exactly what the close-out audit must re-verify. + +--- + +## Veto-Tier + +### B1 — New harness targets get no pre-fix baseline; "absent" would prove nothing + +Steps run fix (1–2) → tests (4) → **add targets (5)** → measure (6). The relay-handler, interface-factory, and Save/Can* targets **do not exist yet**, so the first and only trimmed measurement of their markers happens with the fix already applied. An "absent" result is then satisfied equally by *"the fix removed it"* and by *"the marker was never rooted in the first place."* + +This is the arc's signature failure for the third time: TRIM-001's gate caught a harness that passed with emission disabled; the close-out audit's V1 caught a fixture whose guarded collections were never constructed. + +**Accepted.** Step order changes: targets land and get a pre-fix probe **before** the leg that fixes them. Each new marker must have a recorded present-before / absent-after pair. + +Note the static leg is unaffected — its baseline was captured against pre-existing targets (`_DoWork`, `_ProcessRecord`), so that measurement stands. + +### B2 — Nothing asserts registration still works, and the runtime fails silently + +`AddRemoteFactoryServices.cs:168-170` uses `method?.Invoke(...)`. A holder whose forwarding method is missing or misnamed produces **no diagnostic and no exception** — registration simply stops for that type and surfaces later as an unrelated DI failure. The change introduces a new name-coupling (generated holder ↔ the hard-coded `"FactoryServiceRegistrar"` string) that did not previously exist. + +Every Acceptance bullet is an *absence* or *generated-text* assertion, and **absence assertions pass more easily when registration is dead.** The counter-signal is missing. + +Partial mitigation exists but was unnamed: `TrimmingTests/Program.cs:83,133` resolves `TrimTestCommands.DoWork` and the harness exits non-zero on failure, so the **static leg has a real positive control**. The **relay leg structurally cannot** have one in the trimmed harness — `RelayHandlerRenderer.cs:82` wraps every `RegisterHandler` in `if (NeatooRuntime.IsServerRuntime)`, so on a client publish there is nothing to resolve. Relay registration correctness rests on the untrimmed integration suite, which is nowhere stated and is known-flaky (deferred item 10). + +**Accepted.** Adding a registration-works Acceptance bullet naming the signal for each leg, and a Constraint pinning the holder method name. + +### B3 — Byte-identity evidence has a nonzero expected delta and no partition + +TRIM-006's diff was conclusive because the expected delta was **zero**. Here the static and relay legs change by design, so the diff *will* be nonzero, and the plan gives no way to distinguish "only the two legs moved" from "the two legs moved and something drifted with them." As written the evidence degrades to eyeballing a nonzero diff — the same non-discriminating class the close-out audit vetoed as V2. + +The property itself is achievable: `FactoryRenderer.Render` dispatch, `CleanupSource`, and `NormalizeWhitespace` are untouched shared code, and each remaining leg has its own renderer. + +**Accepted.** Evidence becomes an *expected-delta-set equality* check — enumerate the files expected to change, assert the actual set equals it. + +### A1 — `src/Design/` is absent from the plan, and Step 8 has no bucket for "true today, false after the fix" + +Repo `CLAUDE.md` names the Design projects the single source of truth. `CLAUDE-DESIGN.md:756` is a falsified anchor listed in the todo's own Discovery Log, yet `src/Design/` appears nowhere in the plan's Scope, Steps, Acceptance, or Constraints. + +Worse, Step 8's three buckets (false claims / TRIM-005 artifacts / silences) miss a **fourth**: statements that are *accurate today and become inaccurate after the fix*. Two live anchors sit in it — `docs/trimming.md:222` and `CLAUDE-DESIGN.md:756`, both of which accurately describe "preserve all methods on the referenced type", which is precisely the property the fix removes for two of four shapes. + +**Accepted.** Fourth bucket added; `src/Design/` named explicitly. + +### A2 — AC6 has no Design-surface disposition before a release that closes AC4 + +Deferred item 11 accepts the Design demonstration gap for TRIM-002/007 on the grounds that preservation is unobservable in untrimmed Design tests. That reason extends to AC6 — over-preservation is equally unobservable — but the plan never says so. + +**Accepted.** Explicit accept-with-reason recorded for AC6. + +--- + +## Callout-Tier + +- **B4 — Holder name was a prefix-extension of the user's type.** `MyCommandsNeatooFactoryRegistrar` keeps `global::Ns.MyCommands` as a substring, making the "does not name the consumer's type" assertion a false red and an unclosed `Contains` a false green — the same naive-substring class as deferred item 5. **Fixed in code**: holder is now `NeatooFactoryRegistrar_{TypeName}` (prefix), which breaks the namespace-qualified substring outright. Relay will use a distinct prefix. +- **B5 — Relay output bypasses `NormalizeWhitespace` and has no parse-error signal**, yet the plan adds a top-level type to it. `DiagnosticTestHelper.RunGenerator` returns `OutputCompilation`, which `AssemblyAttributeEmissionTests` discards. **Accepted:** new relay emission tests assert zero `DiagnosticSeverity.Error` on the output compilation, not just string containment. +- **B6 — Deferred item 15 changes error signature** from CS0111 to CS0111 + CS0101 if both legs share a holder name shape. Addressed by distinct per-leg prefixes. +- **B7 — "verified in CI at HEAD" overstates.** CI verifies a top-level holder *registers correctly under trimming*; it does not verify DAM narrowing, because TRIM-007's holder has one method and nothing to narrow. The shapes are analogous, not identical (per-assembly vs per-type). **Accepted** — wording tightened. +- **B8 — The plan asserted intent about the TRIM-005 advisory as fact** ("*This is why* it proposed a nested type — it was solving accessibility, not DAM breadth"), in the same paragraph indicting the arc for that habit. The Discovery Log's framing — "design work found a *second*, unstated reason" — is the defensible one. **Accepted**, restored. +- **B9 — Acceptance bullet 7 is fixture-presence, not behavioural.** "The harness carries targets" is satisfied by adding files. Folded into B1's present-before/absent-after requirement. +- **B10 — CI-gate bullet claims the wrong thing.** It cites a one-time keyboard red-before-green where Step 7 promises a *durable* positive control. Vacuity confirmed real: `build.yml:113-114` greps a path that, if absent, makes `grep -aq` return non-zero, the `if` false, and the step print success. Tier is right; wording is wrong. +- **B11 — Two orphan deferrals.** "Narrowing the DAM" and "`[ModuleInitializer]` registration" are out-of-scope with no Deferred Work row. Also notes the Constraints rationale for not narrowing is directionally right but imprecise — `DynamicallyAccessedMemberTypes` has no sub-method granularity, so no narrowing keeps `FactoryServiceRegistrar` rooted while dropping siblings. +- **A3 — AC6 says "every factory shape"; Acceptance covers two.** Needs one sentence on where class/interface evidence comes from. +- **A4 — The ~40 anchors are not enumerated anywhere the implementer can work from.** This set has been mis-inventoried twice already. Enumerate before the doc pass. + +--- + +## Confirmed by the reviewer (independent check) + +- `attr.Type` is consumed at exactly one site; retargeting is behaviour-preserving at the only consumption point. +- Forwarding rather than hosting is correct — the holder reaches `internal static FactoryServiceRegistrar`, not the `private static` `[Execute]` methods, so CS0122 is avoided without depending on unverified ILLink behaviour. +- No `[FactoryEventHandler]` target exists in the harness; the string appears only in a comment. +- The CI exemption and its falsified rationale are exactly where the plan says. +- `docs/trimming.md:35` is the static bullet (false) and `:36` the interface bullet (true) — the do-not-touch list is correct. +- No transcription smell; code-level detail is confined to Current State, its sanctioned home. + +## Ruled out separately + +**Non-`[Remote]` `[Execute]` regression.** `ExecuteDelegateModel` has no `IsRemote` field; `StaticFactoryRenderer.cs:141-160` emits both a remote and a local registration for *every* delegate with no `[Remote]` check, and only the local one is guarded by `IsServerRuntime`. Such bodies are already unreachable on a trimmed client and merely DAM-retained, so the fix strips something already dead. Incidental finding: **`[Remote]` is decorative on `[Execute]` static methods** — `FactoryModelBuilder.cs:168` exempts static factories from NF0105 — yet NF0105's message and several doc pages present `[Remote]` as the trimming-enabling marker. Belongs in the Step 8 corrections. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-test-review.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-test-review.md new file mode 100644 index 00000000..0eddb0a8 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/008-test-review.md @@ -0,0 +1,68 @@ +# TRIM-008 — Test Review + +**Gate:** mandatory, Step 5. **Passes:** three (2026-08-13). +**Evidence set:** [`008-evidence/`](./008-evidence/) — see the manifest at the bottom. + +Findings are recorded with their verification status. Every checkable finding was independently re-derived at the keyboard before being accepted; **all of them held**, across all three passes. + +--- + +## Pass 1 — 4 must-cover, 7 should-cover + +| # | Finding | Disposition | +|---|---|---| +| P1 | **Holder method name not pinned.** `Assert.Contains("internal static void FactoryServiceRegistrar(...)")` is satisfied by the *user's* partial class, which emits a byte-identical line — so the assertion passed regardless of the holder's method name. The test's own remarks and the plan's Constraint both claimed it was pinned. This is the one failure mode that is silent (`method?.Invoke`) | **Fixed.** Both tests now `Assert.Matches` an anchor binding the signature to the holder's class declaration. Proven by renaming *only* the holder's emitted method and observing RED, then restoring | +| P2 | **Interface leg measured sync-only.** `IsAsync` is driven by auth methods; the target had no auth, so only the sync branch was exercised | **Fixed, then superseded.** An async variant was added — and the result contradicted the then-current diagnosis. See pass 2 | +| P3 | **`ServerOnlyHelper` could never go red.** Zero references anywhere, so ILLink dropped it unconditionally, while it sat in the gate under a header claiming every marker was measured present-before/absent-after | **Fixed.** Wired into `ServerOnlyRepository.DoServerWork`, so the transitive-removal property its comment claimed is now genuinely tested | +| P4 | **The present-before/absent-after claim was wrong for 8 of 16 markers**, including `RelayLegBackend`, which the probe recorded absent *both* times yet the Test Evidence folded into the fix evidence | **Fixed.** `[D]`/`[R]` labels introduced (later `[N]` added) | +| S5–S11 | Compile test passed on zero generated trees; no private-handler relay test; sync-only async inference; shared markers broke per-leg attribution; both-attributes claim untested; stale counts; red gate runs uncaptured | **All fixed** | + +## Pass 2 — 1 must-cover, 6 should-cover + +| # | Finding | Disposition | +|---|---|---| +| N1 | **The async interface-factory target could not go red for the property it was cited as measuring.** Its markers sit behind the `GetRequiredService()` interface hop; verified zero occurrences of any probed marker in the generated factory | **Resolved as structural.** The proposed one-parameter fix (`[Service]` on the interface method) **does not compile** — CS0535, recorded as deferred item 19. So the leg is *structurally* unable to measure body elimination. Now stated at the target, in the gate, and in the inventory rather than left to be inferred from a clean-looking result | +| N2–N7 | Fixture-drift guard on the `Replace`-built private-handler source; async-iface resolution check; `[D]`/`[R]` correction not propagated to three rows; shared async port; evidence-citation nits | **All fixed** | + +## Pass 3 — 1 hold-the-gate, 5 should/nice + +| # | Finding | Disposition | +|---|---|---| +| Q1 | **"Only `async` differs" is not accurate.** Five constructs are async-only in the generated body: the extra `catch (OperationCanceledException)` **and four interface type-tests** (`IFactoryOnStartAsync`, `IFactoryOnCompleteAsync`, `IFactoryOnCancelled`, `IFactoryOnCancelledAsync`). Type-tests are a *different* ILLink retention mechanism from a state machine, so two hypotheses survive and the data cannot separate them — **including the disproven TRIM-004 story returning in async-only form** | **Fixed.** Conclusion restated as "async-shaped emission"; H1/H2 and their differing remedies handed to TRIM-009 as its first step. Also recorded: DAM roots `LocalFetchAsync` via `typeof(TrimTestEntityFactory)`, so **de-rooting is not an available fix** | +| Q2 | Five Test Evidence drifts, including a direct self-contradiction with the script on `[D]`/`[R]` | **Fixed**, and artifacts are now cited by role against a manifest rather than by filename in eight places | +| Q3 | **The gate ran its assert-PRESENT blocks after positive controls failed**, emitting eight "reopen the diagnosis" instructions on an artifact where the target never existed | **Fixed.** The gate now exits immediately when a control fails | +| T1 | **The controlled pair's service asymmetry is load-bearing and undocumented** — giving the async half `IServerOnlyRepository` would surface that name and turn the static-factory `[D]` markers red for a misleading reason | **Fixed.** Documented as do-not-tidy at the target | +| T2 | Deferred item 19 has no pinning test, unlike items 14/15 | **Recorded, not fixed.** Deliberate: the same treatment items 14/15 got until one was pinned; queued with them | +| T4, T5 | Harness summary line for the async delegate; third cause in the assert-PRESENT message | **T5 fixed; T4 recorded** | + +--- + +## What the gate is judged to have achieved + +**The controlled experiment is the strongest evidence this arc has produced** — the first measurement in it that isolates a variable rather than narrating one. Its conclusion holds; only its scope was overstated, and that is corrected. + +Both reviewers independently reached the same verdict on the third pass: **the deliverable is done; the residual risk sits in bookkeeping, not in code or tests.** The `[D]`/`[R]`/`[N]` taxonomy and the "untrimmed-present is necessary but not sufficient" reasoning are durable improvements that outlive this plan. + +## Standing tech debt, unchanged by this plan + +- Deferred item 5 — 16 `IndexOf`-sliced emission assertions that can pass vacuously. Still queued and unowned. Pass 1 noted the pattern reproducing in *new* code (P1), which strengthens the case for giving it its own plan. +- Deferred item 3 — `DiagnosticTestHelper` stale-generator fail-fast. This plan edited that file without closing it. Local-iteration exposure only; CI is cold-build. + +## Evidence manifest + +Artifacts in [`008-evidence/`](./008-evidence/), all regenerated at the end of pass 3 against HEAD: + +| Artifact | Role | +|---|---| +| `build-main.log`, `build-design.log` | Both solutions, 0 errors | +| `test-main-full.log` | 611+611 unit, 561+561 integration, 0 failed, nothing filtered | +| `test-design.log` | 86+86 | +| `publish-trimmed.log` | Trimmed publish | +| `trim-harness.log` | Harness exit 0, six resolution checks | +| `trim-gate.log` | Absence gate, passing run | +| `probe-selfcheck3.txt` | **Untrimmed** control — 53/53 markers PRESENT, proving the probe can see every marker before any absence result is trusted | +| `probe-v5c.txt` | The controlled sync-vs-async experiment | +| `probe-prefix.txt` | Pre-fix baseline | +| `gate-red-nofold.txt` | **The per-leg naming demonstration.** Current code published with the feature switch left ON, so nothing folds: all 6 positive controls pass and the gate names every leg across 30 errors | +| `gate-red-prefix-relayleg.txt`, `gate-red-baseline-prefix.txt` | Archived pre-fix artifacts. These now stop at the positive controls by design — they predate the current targets, so their absence results would be meaningless | +| `gate-red-missing.txt` | Missing-path branch fails loudly | diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md index 5d47b00c..95056bd7 100644 --- a/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md @@ -26,6 +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] + +**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. + +**AC6 held whole 2026-08-13** (user decision) after TRIM-008's pre-fix probe found a *third* broken shape by a different mechanism. The alternative was to narrow AC6 to the two shapes TRIM-008 owns and ship v1.7.0 sooner. Rejected: the class-factory leg is the shape the docs, the skill, and the release notes all hold up as the one that works, so a release closing AC4 while that claim stays false would repeat exactly what AC6 exists to prevent. AC6 now spans two plans and the release waits for both. ## Out of Scope @@ -47,8 +52,10 @@ A third suspected gap turned out to be already fixed: event records derive `Fact | 007 | Done | [Subscribe-only event preservation fix](./plans/007-subscribe-only-event-preservation-fix.md) | TRIM-003 finding: inherited DAM doesn't flow to derived types under ILLink; fixed via generator-emitted per-assembly event-preservation registrar | | 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) | -Execution order: 004 → 001 → 002 → 003 → 007 → 005 → 006 (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.) +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.) ## Skipped Steps @@ -71,19 +78,25 @@ 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 | Built-in plan mode (user decision 2026-08-11); **release-blocking**, so it cannot be silently dropped | Highest-cost item. Its only record is a Discovery Log entry that archives when this todo closes — give it a durable home if the todo closes before the fix lands | -| 2 | **Release held (AC4 + AC5)** — version stays `1.6.1`, no v1.7.0 notes | Reopens when item 1 merges | zTreatment PCB-003 blocked since July. Deliberate trade: consumer unblock-time vs. publishing false IP guidance | +| 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 | | 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) | Tied to item 1 | Item 1 would ship fixed but unverified | -| 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 `(?` spurious `Register>` emission** | **Accepted**, TRIM-002 Amendment — idempotent and harmless; removal needs its own trimmed verification | Cosmetic registrar noise | | 13 | **Interface-factory implementation classes get no property walk** | **Accepted by design**, TRIM-002 Constraint; documented at `docs/trimming.md:285` | A consumer serializing state off such a class still needs manual preservation | +| 14 | **Nested `[Factory]` static / `[FactoryEventHandler]` classes emit uncompilable code** — simple-name FQN in the assembly attribute plus a namespace-scope re-declaration of the user's class | **Recorded, not fixed** (user decision 2026-08-12). Surfaced during TRIM-008 design; pre-existing | Rare shape, but the failure is a confusing cascade of CS errors in *generated* code. A cheap NF01xx diagnostic (`IsNested` is already computed) would make it one clear message | +| 15 | **A class carrying both `[Factory]`(static) and `[FactoryEventHandler]` 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 | +| 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 | ## Discovery Log @@ -172,3 +185,50 @@ AC1–AC3 confirmed genuinely verified in a publish-trimmed artifact at HEAD (CI - **Decision:** **Hold the release** (user decision 2026-08-12) until the registrar-DAM fix lands in plan mode, then ship one version carrying both the trimming-preservation work and the over-retention fix, with the docs true again on publication. The alternative considered and rejected was to narrow the doc claims and release immediately. - **Consequence, stated plainly:** zTreatment PCB-003 stays blocked for the duration — it has been waiting since the framework-side goal was met in July. This is a deliberate trade of consumer unblock-time for not publishing false security guidance. - **Follow-up:** AC4 (release notes + version bump) and AC5 (consumer proof) stay open; the todo does **not** close on this audit. Reopen the release step once the registrar-DAM fix merges. + +### 2026-08-12 — Registrar-DAM fix folded into the arc as TRIM-008 (reverses the plan-mode routing) +- **Finding:** Three things made the 2026-08-11 "fix it in plan mode, not as a todo" routing untenable. The defect is **release-blocking**, so the arc cannot close without it. It **falsifies the documentation AC4 requires be accurate**, so AC4 cannot close honestly while it stands. And the close-out audit found its only record was a Discovery Log entry that archives when the todo closes — i.e. it would have lost its tracker at exactly the moment it still mattered. Added **AC6** to make the guarantee a named requirement rather than an implicit one. +- **Decision:** Fold in as TRIM-008 (user decision 2026-08-12). AC6 added. Deferred items 1, 6, 7, 8 now route to that plan. +- **Note on scope:** AC6 is not scope creep. AC1–AC3 are about *preservation* — making types survive trimming. AC6 is about *over-preservation* — stopping code surviving that should not. Opposite failure modes of the same mechanism. + +### 2026-08-12 — The TRIM-005 nested-type advisory was solving accessibility, not DAM breadth +- **Finding:** The remedy floated at TRIM-005's plan review — put `FactoryServiceRegistrar` on a *nested* type inside the user's partial — was recorded with the rationale "DAM on a type does not cover its nested types". That rationale is unverified in this codebase, and the TRIM-007 precedent it appeals to uses a **top-level** holder, so it proves nothing about the nested case. But design work found a *second*, unstated reason the advisory chose nesting: `[Execute]` methods are `private static` by the repo's own convention (`AllPatterns.cs:362-368`), and the registrar body calls them, so a sibling top-level holder that *hosts* the registrar is CS0122. The relay leg is equally exposed — `FactoryGenerator.RelayHandler.cs:74-114` applies no accessibility filter despite its doc comment claiming "non-private". A holder that **forwards** to a registrar left on the user's class satisfies both constraints and depends on no unverified ILLink behavior. +- **Decision:** TRIM-008 uses the forwarding holder. Recorded because an advisory's stated reason was incomplete, and the missing half was the load-bearing one — the same class of hazard as an inherited diagnosis. + +### 2026-08-12 — Two latent generator bugs (recorded, deliberately not fixed) +- **Finding:** (1) A **nested** `[Factory] static partial class` or nested `[FactoryEventHandler]` class emits uncompilable code today: `model.TypeName`/`model.ClassName` are simple names with no containing-type walk, so the assembly attribute names a nonexistent `Ns.Inner`, *and* the renderer re-declares the class at namespace scope. `IsNested` is already computed (`FactoryGenerator.Types.cs:85`) but consulted only for ordinal converters. (2) A class carrying **both** `[Factory]`(static) and `[FactoryEventHandler]` emits duplicate `FactoryServiceRegistrar` members — CS0111. Both pre-existing, both surfaced during TRIM-008's design, neither caused by it. +- **Decision:** Record only (user decision 2026-08-12). TRIM-008 is already large; these hit rare shapes and are independent. A cheap NF01xx diagnostic on nested types would convert a cascade of CS errors in generated code into one actionable message — the recommended shape if either is ever picked up. +- **Follow-up:** Carried in the Deferred Work table as items 14 and 15. + +### 2026-08-13 — B9 harness targets landed and probed pre-fix; the Save/Can* leg came back BROKEN +- **What was done:** TRIM-008 Steps 1–2. Three harness legs added that never existed — `[FactoryEventHandler]` (`TrimRelayHandlers`), interface factory (`ITrimIfaceQuery`), and Save/Can* (`TrimSaveTarget`, `IFactorySaveMeta` + `[AuthorizeFactory]` → `Save`/`CanCreate`/`CanInsert`/`CanUpdate`/`CanDelete`/`CanSave`) — each with its own server-only port so a marker names one leg. Then probed publish-trimmed **with the relay leg still unfixed**, per plan-review B1: a marker measured for the first time *after* a fix cannot distinguish "the fix removed it" from "it was never rooted". +- **Relay leg (expected):** all four markers PRESENT pre-fix. The baseline the post-fix "absent" claim will be measured against now exists. +- **Interface-factory leg (expected):** all five markers absent. The leg the arc has always *claimed* is safe is now *measured* safe. B9's first half closes with evidence rather than structural argument. +- **Save/Can\* leg (NOT expected):** `ISaveLegPort`, `SaveLegInvoke`, and all three body literals `SaveLegInsertBody_MARKER` / `SaveLegUpdateBody_MARKER` / `SaveLegDeleteBody_MARKER` **PRESENT** in the trimmed client. This is the **class-factory** leg — the shape the docs, the skill, and the release notes all present as the one that trims correctly. +- **Cause, from a single-variable comparison inside one assembly:** `TrimTestEntityFactory.LocalCreate` (**sync**) and `TrimSaveTargetFactory.LocalInsert/Update/Delete` (**async**) are otherwise identical — same `if (!NeatooRuntime.IsServerRuntime) throw` guard, same `ServiceProvider.GetRequiredService()`, both rooted the same way by an **unguarded** delegate registration in `FactoryServiceRegistrar`, both DAM-preserved. The sync one's post-guard body is eliminated (`IServerOnlyRepository`, `DoServerWork` absent); the async ones' are not. Surviving state machines confirm it: `d__15`, `d__16`, `d__17`, `d__21` are all in the trimmed DLL; `d__` does not exist. The feature-switch fold happens inside `MoveNext`, and the remainder is not eliminated there. +- **Why this is NOT TRIM-008's defect:** TRIM-008 is "the assembly attribute names a *consumer* type". Here the attribute correctly names the generated `TrimSaveTargetFactory`. And DAM is not even the only root — the registrar's own unguarded `AddScoped` closure roots `LocalSave` independently, so the forwarding-holder fix would not remove these bodies. Different mechanism, different fix, out of TRIM-008's scope. +- **What it changes for TRIM-008:** the Save/Can* markers **cannot** be asserted absent after the relay fix, and the Step-8 CI gate must not encode that they are. AC6 as written ("every factory shape") is no longer satisfiable by this plan — it needs a disposition. Recorded as deferred item 18 pending the user's call. +- **Method note:** the probe's own self-check caught a defect in the probe before any result was trusted. Run first against the **untrimmed** assembly — where all 30 markers must be PRESENT — it reported all five string-literal markers "absent", because it decoded UTF-16 only from byte offset 0 and every literal starting at an odd offset was invisible to it. Uncorrected, every literal marker would have read "absent" in the trimmed DLL too, and the Save/Can* break would have been reported as a clean pass. `[[trim-arc-verify-dont-inherit]]`, third instance: an absence check that has never been shown capable of reporting presence is not evidence. + +### 2026-08-13 — AC6 held whole; the async-body leak becomes TRIM-009 +- **Decision (user):** do not narrow AC6 to the two shapes TRIM-008 owns. Deferred item 18 is folded into the arc as **TRIM-009**, and the v1.7.0 release waits for both plans. +- **What was traded away:** the fastest path to unblocking zTreatment PCB-003, blocked since July. Narrowing AC6 would have let v1.7.0 ship as soon as TRIM-008 landed, with release notes disclosing that async write operations still leak. +- **Why the slower path won:** the leaking shape is the **class factory** — the one `docs/trimming.md`, the distributable skill, and `CLAUDE-DESIGN.md` all hold up as the shape that works. A release that closes AC4 ("documentation matches shipped behavior") while the flagship claim stays false is the precise failure AC6 was created to stop. Shipping the fix for two obscure shapes while the common one leaks would also read, to a consumer, as the problem being solved. +- **Why TRIM-009 is a separate plan and not more steps on TRIM-008:** different mechanism (async state-machine bodies surviving the feature-switch fold, rooted by unguarded delegate-registration closures — not an attribute naming a consumer type), so a different fix, different tests, and a different CI assertion. TRIM-008 is already ten steps, and its own plan review flagged the tail steps as the likeliest to be under-executed. A third option — folding item 18 into TRIM-008 — was offered and declined. +- **Consequence for TRIM-008:** unchanged in scope. It continues at Step 4. Its Save/Can\* harness markers are now **expected to stay present** through its post-fix probe; TRIM-009 is what turns them absent, and TRIM-009 inherits the pre-fix baseline captured on 2026-08-13 rather than needing its own. + +### 2026-08-13 — TRIM-008 doc pass: rebuilding the inventory falsified the deferred table's own premise +- **What was done:** Steps 9 and 10. The ~40-anchor inventory was rebuilt by reading the files rather than copying the plan's list (plan review A4), and recorded at [`reviews/008-doc-anchor-inventory.md`](./reviews/008-doc-anchor-inventory.md). Four buckets worked: falsified claims, the four falsified-TRIM-005 artifacts, the silences, and the "accurate today, inaccurate after the fix" bucket the first draft missed. +- **The inventory contradicted deferred item 8.** That row asserted `skills/.../class-factory.md:318,333-334` and `advanced-patterns.md:227` had been **over-listed** as falsified "because class factories trim correctly." Measurement says otherwise — class factories do not trim correctly for async operations (item 18). So the anchors the table called safe are false today, and the correction is the reverse of the one recorded. +- **Disposition: deferred to TRIM-009, not edited.** TRIM-009 makes them true, and nothing ships before it lands. Editing them to say "leaks" and reverting afterwards would churn published docs and, in the interim, publish a scarier claim than any shipped version has warranted. What makes this legitimate rather than a repeat of this arc's habit: the eight anchors are enumerated, the dependency is named, and the release is already blocked on TRIM-009 by AC6 and deferred item 2. If TRIM-009 is ever descoped, that table is the checklist that must be worked first. +- **One forward-looking claim was written deliberately.** The skill's new shape-by-shape trimming table has a class-factory row that is true only once TRIM-009 lands. Omitting the row would have been its own silence — the table exists precisely to say what each shape does and does not remove. It is registered in the inventory as release-blocking. +- **`ServerOnlyTypes.cs:4-6` needed no change.** It was the fourth falsified-TRIM-005 artifact only in the sense that it *contradicted* the other three: it said the interface should be absent, which measurement now confirms. It was right the whole time and the surrounding artifacts were wrong. +- **Two silences worth naming, both now documented:** `[Remote]` is decorative on `[Execute]` (static factories are exempt from NF0105, and both registrations are emitted regardless — the `IsServerRuntime` guard is what makes the body trimmable), and the `[FactoryEventHandler]` leg structurally cannot be verified from a client-side test because every registration is server-guarded. +- **Container reconciliation:** deferred items 6 and 7 closed, item 8 partially closed with its premise corrected. Items 1 and 2 stay open until TRIM-008 merges and TRIM-009 lands respectively. + +### 2026-08-13 — Gate re-review: an async measurement corrected a diagnosis, then a controlled experiment restored it +- **What happened, in order.** TRIM-008's test review said the async claim was inferred rather than measured. Adding async targets to three legs then produced a result that *contradicted* the diagnosis: async `[Execute]`, async relay handlers, and an async interface-factory method all trim clean. The code review then showed the fallback explanation ("the leak needs an early-throw guard AND async AND a direct concrete call") was itself unsupported — two of its three conjuncts rested on rows that cannot discriminate, and the class-factory pair offered as single-variable actually differed in four further ways: an auth block, target-from-DI vs from-parameter, one-hop vs two-hop rooting, and an extra catch arm. That last is the dimension the arc's disproven TRIM-004 story blamed. +- **The experiment that settled it.** `TrimTestEntity` gained an `async [Remote][Fetch]` beside its sync `[Remote][Create]`, each with its own literal in its own body — same type, same generated factory, same registrar, no auth on either, one-hop rooting on both, direct concrete call on both. Trimmed: sync marker **absent**, async marker **PRESENT**. Both PRESENT untrimmed. **`async` is confirmed as the operative variable**, now on a controlled comparison instead of a confounded one. +- **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. diff --git a/docs/trimming.md b/docs/trimming.md index 73947058..36b21bb4 100644 --- a/docs/trimming.md +++ b/docs/trimming.md @@ -30,9 +30,10 @@ Not all factory methods get guards. The generator uses the developer's `public` `public` non-`[Remote]` methods like `Create(string name)` or `CanCreate()` have no guard because they are designed to run on the client. Marking child entity factory methods as `internal` (without `[Remote]`) also makes them trimmable. -### Static and Interface Factories +### Static, Interface, and Event Handler Factories -- **Static factories** — `[Execute]` delegate registrations are guarded. The trimmer removes the registration lambdas and their captured dependencies. +- **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. 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. @@ -212,6 +213,15 @@ grep -aob "YourRepositoryClassName" bin/Release/net9.0/publish/YourApp.dll ilspycmd bin/Release/net9.0/publish/YourApp.dll ``` +**Grepping for a string literal needs a second step.** Type and method *names* are UTF-8 in the assembly's metadata, so `grep -a` finds them. String *literals* from method bodies are UTF-16, so `grep -a` never matches them and reports "absent" for text that is demonstrably in the file. If you are looking for a literal — a connection string fragment, a SQL keyword, a distinctive message — strip the null bytes first: + +```bash +# Literals: collapse UTF-16 to ASCII before searching +tr -d '\000' < bin/Release/net9.0/publish/YourApp.dll | grep -c "SELECT * FROM" +``` + +Verify your check can actually find things before trusting a clean result: run it against the **non-published** build output, where the server-only code definitely still exists. If it reports "absent" there too, the check is broken, not the code. + If server-only type names still appear in the output, check that: 1. `TrimMode` is set to `full` (not `partial` or omitted) 2. The `RuntimeHostConfigurationOption` has `Trim="true"` @@ -219,7 +229,24 @@ If server-only type names still appear in the output, check that: ## Factory Type Preservation -All factory types — class, static, and interface — are automatically preserved from trimming. The source generator emits `[assembly: NeatooFactoryRegistrar(typeof(X))]` for every factory, creating a static reference that the IL trimmer follows. The `NeatooFactoryRegistrarAttribute` carries `[DynamicallyAccessedMembers]` annotations that instruct the trimmer to preserve all methods on the referenced type, including the internal `FactoryServiceRegistrar` method used for DI registration. +All factory types — class, static, and interface — are automatically preserved from trimming. The source generator emits `[assembly: NeatooFactoryRegistrar(typeof(X))]` for every factory, creating a static reference that the IL trimmer follows. The `NeatooFactoryRegistrarAttribute` carries `[DynamicallyAccessedMembers]` annotations that instruct the trimmer to preserve **all methods on the referenced type, method bodies included**. + +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: + +```csharp +// generated, alongside your partial class +internal static class NeatooFactoryRegistrar_MyCommands +{ + internal static void FactoryServiceRegistrar(IServiceCollection services, NeatooFactory remoteLocal) + => MyCommands.FactoryServiceRegistrar(services, remoteLocal); +} +``` + +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. 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/static-factory.md b/skills/RemoteFactory/references/static-factory.md index d4fd67c4..3572a476 100644 --- a/skills/RemoteFactory/references/static-factory.md +++ b/skills/RemoteFactory/references/static-factory.md @@ -84,6 +84,8 @@ private static Task _SendNotification(...) { } The generator creates the public method. Your code provides the private implementation. +`private static` also matters for trimming: the generated local registration is guarded by `NeatooRuntime.IsServerRuntime`, so on a Blazor WASM client published with the feature switch set to `false`, the method body, its `[Service]` dependencies, and their transitive references are removed. Note that `[Remote]` is decorative here — static factories are exempt from the NF0105 `[Remote] public` check, and what makes the body trimmable is the guard, not the attribute. Keep `[Remote]` for intent. See `references/trimming.md`, which documents what each factory shape does and does not remove. + ### [Execute] must return `Task`, not `Task` ```csharp diff --git a/skills/RemoteFactory/references/trimming.md b/skills/RemoteFactory/references/trimming.md index 547afdf0..96eee93d 100644 --- a/skills/RemoteFactory/references/trimming.md +++ b/skills/RemoteFactory/references/trimming.md @@ -129,6 +129,29 @@ The `Trim="true"` attribute on the `RuntimeHostConfigurationOption` is critical - **.NET 9 or later** — `[FeatureSwitchDefinition]` was introduced in .NET 9 - **`dotnet publish`** — Trimming runs during publish, not during `dotnet build` or `dotnet run` +## What Gets Trimmed, By Factory Shape + +The guarantee is not uniform across factory shapes. What follows is measured against a published trimmed assembly, not inferred from the mechanism. + +| 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 | + +### Why static factories and event handlers needed a fix + +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. + +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. + +### `[Remote]` is decorative on `[Execute]` + +Static factories are exempt from the NF0105 `[Remote] public` check, and the generator emits both remote and local registrations regardless, guarding only the local one with `IsServerRuntime`. Trimming of an `[Execute]` body follows from that guard, not from `[Remote]`. Keep `[Remote]` for intent — it reads consistently with class factories — but do not rely on it as the thing that makes the body trimmable. + ## Verifying Results After publishing, confirm server-only types were removed: @@ -141,6 +164,14 @@ dotnet publish -c Release grep -aob "YourRepositoryClassName" bin/Release/net9.0/publish/YourApp.dll ``` +**Searching for a string literal takes an extra step.** Type and method *names* are UTF-8 in assembly metadata, so `grep -a` finds them. String *literals* from method bodies are UTF-16, so `grep -a` cannot match them — it reports "absent" for text that is sitting in the file. Strip the nulls first: + +```bash +tr -d '\000' < bin/Release/net9.0/publish/YourApp.dll | grep -c "SELECT * FROM" +``` + +Before trusting a clean result, run the same check against the **non-published** build output, where the server-only code definitely still exists. If it reports "absent" there too, your check is broken rather than your code being clean. + If server-only type names still appear: 1. Confirm `TrimMode` is `full` (not `partial` or omitted) 2. Confirm `RuntimeHostConfigurationOption` has `Trim="true"` diff --git a/src/Design/CLAUDE-DESIGN.md b/src/Design/CLAUDE-DESIGN.md index d81324d8..fdff2b2b 100644 --- a/src/Design/CLAUDE-DESIGN.md +++ b/src/Design/CLAUDE-DESIGN.md @@ -753,15 +753,24 @@ The concrete type is resolved at compile time using the naming convention (`IPer #### Trimming-Safe Factory Registration -The generator emits `[assembly: NeatooFactoryRegistrar(typeof(X))]` for every factory type (class, static, and interface). The `NeatooFactoryRegistrarAttribute` carries `[DynamicallyAccessedMembers(PublicMethods | NonPublicMethods)]` on its `Type` property, which creates a dataflow contract the IL trimmer follows — ensuring each factory type's `FactoryServiceRegistrar` method (and all other methods) survive trimming. +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. + +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. 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 | -| Static Factory | `typeof({Namespace}.{StaticClassName})` — the static class itself | +| 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. + +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. 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/Design/Design.Domain/FactoryPatterns/AllPatterns.cs b/src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs index 865dd02f..c3eed607 100644 --- a/src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs +++ b/src/Design/Design.Domain/FactoryPatterns/AllPatterns.cs @@ -353,6 +353,26 @@ public static partial class ExampleCommands /// GENERATOR BEHAVIOR: For this method, the generator creates: /// - Delegate: Execute_ExampleCommands_SendNotification(string, string) /// - Public method: ExampleCommands.SendNotification(string, string) + /// - A forwarding holder, NeatooFactoryRegistrar_ExampleCommands, which the + /// assembly-level [NeatooFactoryRegistrar] attribute points at + /// + /// TRIMMING: the holder is not incidental. The registrar attribute carries + /// [DynamicallyAccessedMembers], which preserves every method on the type it + /// names -- method BODIES included. Static factories have no separate generated + /// factory class (the generator re-opens this partial to host + /// FactoryServiceRegistrar), so before v1.7.0 the attribute named + /// ExampleCommands itself and _SendNotification's body shipped to trimmed + /// Blazor WASM clients, decompilable. The holder gives the attribute a + /// single-method type to preserve instead. + /// + /// Note [Remote] is decorative on [Execute]: static factories are exempt from + /// the NF0105 [Remote] public check, and both remote and local registrations + /// are emitted regardless, with only the local one guarded by IsServerRuntime. + /// The guard is what makes the body trimmable, not the attribute. + /// + /// Not demonstrated by a Design test: preservation and over-preservation are + /// only observable in a publish-trimmed artifact, and Design.Tests run + /// untrimmed. RemoteFactory.TrimmingTests is the verification surface. /// /// Usage from client: /// var success = await ExampleCommands.SendNotification("recipient@example.com", "Hello!"); diff --git a/src/Generator/Renderer/RelayHandlerRenderer.cs b/src/Generator/Renderer/RelayHandlerRenderer.cs index 3f79da54..1b1105e7 100644 --- a/src/Generator/Renderer/RelayHandlerRenderer.cs +++ b/src/Generator/Renderer/RelayHandlerRenderer.cs @@ -28,8 +28,17 @@ public static string Render(RelayHandlerModel model) sb.AppendLine(); - // Assembly-level attribute for trimming-safe factory discovery - sb.AppendLine($"[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof({model.Namespace}.{model.ClassName}))]"); + // Assembly-level attribute for trimming-safe factory discovery. + // Targets the generated registrar holder, NEVER the user's own class: the + // attribute's [DynamicallyAccessedMembers] preserves every method on whatever + // type it names, bodies included, so naming the user's class would ship their + // handler bodies — and the server-only services those bodies reach — to a + // trimmed client (TRIM-008). + // + // global:: added here at the same time. Its absence was a latent bug: a + // consumer namespace that shadows the first segment of this one would bind + // the attribute argument to the wrong type. + sb.AppendLine($"[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::{model.Namespace}.{RegistrarHolderPrefix}{model.ClassName}))]"); sb.AppendLine(); sb.AppendLine("/*"); @@ -53,11 +62,60 @@ public static string Render(RelayHandlerModel model) sb.AppendLine(" }"); sb.AppendLine(" }"); + + // Registrar holder — the assembly attribute's DAM target. Exists so the DAM + // blast radius is this one forwarding method instead of every method on the + // user's handler class. Top-level rather than nested, matching the proven + // in-tree shape (EventPreservationRenderer) and StaticFactoryRenderer's holder. + sb.AppendLine(); + RenderRegistrarHolder(sb, model); sb.AppendLine("}"); return sb.ToString(); } + /// + /// Prefix for the generated relay-handler registrar holder type. + /// + /// + /// Deliberately a PREFIX, for the same reason as + /// : a suffixed name leaves + /// global::Ns.MyHandlers a substring of the holder's own fully-qualified name, + /// which turns the "attribute must not name the consumer's type" regression assertion + /// into a false red and an unclosed Contains into a false green. + /// + /// Deliberately DISTINCT from the static-factory prefix. A class carrying both + /// [Factory] and [FactoryEventHandler<T>] already emits duplicate + /// FactoryServiceRegistrar members (CS0111 — Deferred Work item 15, broken at + /// HEAD). A shared holder prefix would stack a CS0101 duplicate-type error on top, + /// changing that shape's failure signature for the worse. + /// + /// + internal const string RegistrarHolderPrefix = "NeatooEventHandlerRegistrar_"; + + /// + /// Emits the holder the assembly attribute points at. It does nothing but forward + /// to the real registrar on the user's partial class — the indirection exists purely + /// so the attribute's [DynamicallyAccessedMembers] has a single-method type to + /// preserve instead of the user's whole handler class. + /// + /// + /// Forwarding rather than hosting: the registrar body invokes the handler methods, + /// and FactoryGenerator.RelayHandler.cs applies no accessibility filter, so + /// private handler methods compile today. A sibling holder that hosted the registrar + /// would be CS0122 for those. + /// + private static void RenderRegistrarHolder(StringBuilder sb, RelayHandlerModel model) + { + sb.AppendLine($" internal static class {RegistrarHolderPrefix}{model.ClassName}"); + sb.AppendLine(" {"); + sb.AppendLine(" internal static void FactoryServiceRegistrar(IServiceCollection services, NeatooFactory remoteLocal)"); + sb.AppendLine(" {"); + sb.AppendLine($" global::{model.Namespace}.{model.ClassName}.FactoryServiceRegistrar(services, remoteLocal);"); + sb.AppendLine(" }"); + sb.AppendLine(" }"); + } + /// /// Server-side: register into FactoryEventHandlerRegistry. Handlers run in the /// caller's DI scope (shared DbContext/transaction), sequentially, awaited. diff --git a/src/Generator/Renderer/StaticFactoryRenderer.cs b/src/Generator/Renderer/StaticFactoryRenderer.cs index fb0bb54a..4be6cc19 100644 --- a/src/Generator/Renderer/StaticFactoryRenderer.cs +++ b/src/Generator/Renderer/StaticFactoryRenderer.cs @@ -37,8 +37,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.TypeName}))]"); + // Assembly-level attribute for trimming-safe factory discovery. + // Targets the generated registrar holder, NEVER the user's own class: the + // attribute's [DynamicallyAccessedMembers] preserves every method on whatever + // type it names, bodies included, so naming the user's class would ship their + // [Remote] server-only bodies to a trimmed client (TRIM-008). + sb.AppendLine($"[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::{unit.Namespace}.{RegistrarHolderPrefix}{model.TypeName}))]"); sb.AppendLine(); sb.AppendLine("/*"); @@ -61,10 +65,20 @@ public static string Render(FactoryGenerationUnit unit) sb.AppendLine(); - // FactoryServiceRegistrar + // FactoryServiceRegistrar. Stays on the user's partial class deliberately — + // its body calls the [Execute] domain methods, which are `private static` by + // convention (see Design AllPatterns.cs), so a sibling type cannot reach them. RenderFactoryServiceRegistrar(sb, model); sb.AppendLine(" }"); + + // Registrar holder — the assembly attribute's DAM target. Exists so the DAM + // blast radius is this one forwarding method instead of every method on the + // user's class. Top-level rather than nested: the proven in-tree shape + // (EventPreservationRenderer) is top-level, and DAM's behavior toward nested + // types is not something this repo has verified. + sb.AppendLine(); + RenderRegistrarHolder(sb, unit, model); sb.AppendLine("}"); return sb.ToString(); @@ -85,6 +99,40 @@ private static void RenderDelegate(StringBuilder sb, ExecuteDelegateModel del) sb.AppendLine($" public delegate Task<{del.ReturnType}> {del.DelegateName}({paramDecl});"); } + /// + /// Prefix for the generated registrar holder type. Mirrored as a literal in + /// AssemblyAttributeEmissionTests so a rename fails loudly rather than silently + /// repointing the attribute. + /// + /// + /// Deliberately a PREFIX, not a suffix. A suffixed name (MyCommandsNeatooFactoryRegistrar) + /// is a prefix-extension of the user's type, so global::Ns.MyCommands remains a + /// substring of the holder's fully-qualified name — which makes the "attribute must not + /// name the consumer's type" regression assertion a false red, and an unclosed + /// Contains("typeof(global::Ns.MyCommands") a false green. Prefixing breaks the + /// namespace-qualified substring outright, so the assertion means what it says. + /// The relay leg uses a distinct prefix so the two never collide on a class carrying + /// both attributes (see Deferred Work item 15). + /// + internal const string RegistrarHolderPrefix = "NeatooFactoryRegistrar_"; + + /// + /// Emits the holder the assembly attribute points at. It does nothing but forward + /// to the real registrar on the user's partial class — the indirection exists + /// purely so the attribute's [DynamicallyAccessedMembers] has a single-method type + /// to preserve instead of the user's whole class. + /// + private static void RenderRegistrarHolder(StringBuilder sb, FactoryGenerationUnit unit, StaticFactoryModel model) + { + sb.AppendLine($" internal static class {RegistrarHolderPrefix}{model.TypeName}"); + sb.AppendLine(" {"); + sb.AppendLine(" internal static void FactoryServiceRegistrar(IServiceCollection services, NeatooFactory remoteLocal)"); + sb.AppendLine(" {"); + sb.AppendLine($" global::{unit.Namespace}.{model.TypeName}.FactoryServiceRegistrar(services, remoteLocal);"); + sb.AppendLine(" }"); + sb.AppendLine(" }"); + } + private static void RenderFactoryServiceRegistrar(StringBuilder sb, StaticFactoryModel model) { sb.AppendLine(" internal static void FactoryServiceRegistrar(IServiceCollection services, NeatooFactory remoteLocal)"); diff --git a/src/RemoteFactory/FactoryAttributes.cs b/src/RemoteFactory/FactoryAttributes.cs index 0ecf85ac..e8da797a 100644 --- a/src/RemoteFactory/FactoryAttributes.cs +++ b/src/RemoteFactory/FactoryAttributes.cs @@ -86,6 +86,28 @@ public sealed class DeleteAttribute : FactoryOperationAttribute public DeleteAttribute() : base(FactoryOperation.Delete) { } } +/// +/// Marks a static method on a [Factory] static class as a request-response command. +/// The generator emits a delegate type and its DI registration. +/// +/// +/// +/// Trimming: mark the method private static (the convention used throughout +/// the Design projects) and the generated local registration is guarded by +/// NeatooRuntime.IsServerRuntime, so on a client published with the feature switch +/// set to false the method body, its [Service] dependencies, and their +/// transitive references are removed from the output. +/// +/// +/// [Remote] is decorative on [Execute] methods. Static factories are +/// exempt from the NF0105 [Remote] public check, and the renderer emits both a remote +/// and a local registration for every delegate regardless of whether [Remote] is +/// present — only the local one is feature-switch guarded. Trimming of the body therefore +/// depends on the guard, not on [Remote]. This differs from class factories, where +/// [Remote] internal is what drives the guard, and several documentation pages +/// present [Remote] as the trimming-enabling marker generally. +/// +/// public sealed class ExecuteAttribute : FactoryOperationAttribute { public ExecuteAttribute() : base(FactoryOperation.Execute) { } @@ -106,6 +128,21 @@ public ExecuteAttribute() : base(FactoryOperation.Execute) { } /// Client-side event consumers implement to bridge /// relayed events to their own event aggregator. /// +/// +/// Trimming: every generated handler registration is wrapped in +/// NeatooRuntime.IsServerRuntime, so on a client published with the feature switch +/// set to false the handler bodies and their [Service] dependencies are +/// removed from the output. This works because the generator points the assembly's +/// at a generated forwarding holder rather +/// than at the handler class itself — the attribute preserves every method on whatever it +/// names, bodies included, so naming the handler class would ship the handler bodies to the +/// browser. Fixed in v1.7.0; before that, it did. +/// +/// +/// One consequence for consumers: because the registrations are server-guarded, there is +/// nothing on a trimmed client to resolve, and handler registration cannot be verified from +/// a client-side test. Coverage for it lives in server-side/untrimmed tests. +/// /// /// The event type (must inherit from ). [System.AttributeUsage(AttributeTargets.Class, Inherited = false, AllowMultiple = true)] @@ -151,9 +188,58 @@ public FactoryHintNameLengthAttribute(int maxHintNameLength) /// enumerates these to discover FactoryServiceRegistrar methods /// in a trimming-safe way (replaces the trim-unsafe assembly.GetTypes() scan). /// +/// +/// +/// CONTRACT: the must be a GENERATED registrar type. Never a consumer's +/// own class. +/// +/// +/// The [DynamicallyAccessedMembers] annotation below preserves every method on the +/// named type, method bodies included. Naming a consumer's class therefore ships that +/// class's [Remote] server-only method bodies — SQL, business rules, credentials, +/// whatever they contain — to a trimmed Blazor WebAssembly client, where they are +/// decompilable. That is the exact opposite of what RemoteFactory promises, and it is +/// invisible: everything compiles, every test passes, and the only symptom is code sitting +/// 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. +/// +/// +/// Consequences for anyone editing the generator: point this attribute at a type that exists +/// only to forward, keep its surface to the one FactoryServiceRegistrar method, and do +/// not "simplify" it away by naming the class that already has the method. Pointing it +/// somewhere convenient is what caused the defect. +/// +/// +/// The annotation is deliberately not narrowed to PublicMethods. +/// DynamicallyAccessedMemberTypes has no sub-method granularity, so no narrowing can +/// keep FactoryServiceRegistrar rooted while dropping siblings — the holder indirection +/// is the only mechanism that shrinks the blast radius. Narrowing would also silently unroot +/// the registrars of any prebuilt library compiled by an older generator, whose registrar is +/// internal static: its factories would stop registering on a trimmed client with no +/// diagnostic and no exception. +/// +/// +/// The method this attribute exists to reach is looked up by the literal name +/// "FactoryServiceRegistrar" and invoked with a null-conditional call, so a holder whose +/// method is renamed or missing produces no diagnostic and no exception — registration +/// simply stops for that type and surfaces later as an unrelated DI resolution failure. +/// +/// [System.AttributeUsage(AttributeTargets.Assembly, Inherited = false, AllowMultiple = true)] public sealed class NeatooFactoryRegistrarAttribute : Attribute { + /// + /// The generated registrar holder type. Must never be a consumer-authored class — see the + /// contract on the type-level documentation for why. + /// public NeatooFactoryRegistrarAttribute( [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers( System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicMethods | @@ -162,6 +248,11 @@ public NeatooFactoryRegistrarAttribute( Type = type; } + /// + /// The generated registrar holder whose FactoryServiceRegistrar method is invoked + /// during registration. Every method on this type is preserved under trimming, bodies + /// included, so it must be a generated forwarding type and never a consumer's class. + /// [System.Diagnostics.CodeAnalysis.DynamicallyAccessedMembers( System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.PublicMethods | System.Diagnostics.CodeAnalysis.DynamicallyAccessedMemberTypes.NonPublicMethods)] diff --git a/src/Tests/RemoteFactory.TrimmingTests/InterfaceFactoryLegTarget.cs b/src/Tests/RemoteFactory.TrimmingTests/InterfaceFactoryLegTarget.cs new file mode 100644 index 00000000..84f0b3ba --- /dev/null +++ b/src/Tests/RemoteFactory.TrimmingTests/InterfaceFactoryLegTarget.cs @@ -0,0 +1,156 @@ +using Neatoo.RemoteFactory; + +namespace RemoteFactory.TrimmingTests; + +// ============================================================================= +// INTERFACE-FACTORY LEG TARGET (TRIM-008, closing plan-review B9) +// ============================================================================= +// +// The interface-factory leg is the one the arc has always CLAIMED is safe, on +// the structural grounds that its assembly attribute names the GENERATED proxy +// (TrimIfaceQueryFactory) rather than any user type — so DAM has nothing of the +// consumer's to over-retain. +// +// That claim was never measured. This target measures it, which is the whole +// point of B9: an arc that asserts two legs are safe while only ever probing one +// of them is asserting, not verifying. +// +// Note the shape difference from the broken legs. There is no [Remote] method +// body here to strip — an interface factory's client side is a proxy that +// serializes calls. The server-only IP lives in the IMPLEMENTATION class +// (TrimIfaceServerSide), which carries no factory attribute at all and is +// registered only behind the harness's IsServerRuntime guard. If the interface +// leg is genuinely clean, nothing roots that class and its body is absent. +// +// NF0106 TRAP: operation attributes ([Fetch], [Execute], ...) on an interface- +// factory member are a diagnostic, not a no-op — the member degrades and the +// generated factory silently loses it. No operation attributes below; [Factory] +// on the interface is the whole contract. +// ============================================================================= + +/// +/// Interface factory. The generator emits ITrimIfaceQueryFactory and a +/// TrimIfaceQueryFactory proxy; the assembly attribute names the proxy. +/// +[Factory] +public interface ITrimIfaceQuery +{ + Task LookupAsync(string key); +} + +/// +/// Server-side implementation. Deliberately carries NO factory attribute — the +/// [Factory] on the interface is sufficient, and adding one here would cause +/// duplicate registration. +/// +/// +/// Every name in this class is an absence marker: the type name, its +/// dependency, and the literal in the body. All three +/// should be gone from a trimmed client publish. +/// +public sealed class TrimIfaceServerSide : ITrimIfaceQuery +{ + private readonly IIfaceLegPort port; + + public TrimIfaceServerSide(IIfaceLegPort port) + { + this.port = port; + } + + public Task LookupAsync(string key) + { + // Concatenated, not interpolated — see the note in RelayHandlerLegTarget. + return Task.FromResult(port.IfaceLegInvoke("IfaceLegServerBody_MARKER: " + key)); + } +} + +// ============================================================================= +// ASYNC INTERFACE-FACTORY VARIANT +// ============================================================================= +// +// The target above measures only the SYNCHRONOUS emission branch. +// InterfaceFactoryRenderer emits `async` for a method only when the model's IsAsync is set, +// and FactoryModelBuilder sets that from +// method.AuthMethodInfos.Any(m => m.IsTask) || method.AspAuthorizeCalls.Any() +// — so an interface factory with no authorization can never produce an async Local* method. +// +// That distinction is not cosmetic here. TRIM-009 found that async generated Local* methods +// retain their server-only bodies while sync ones do not. Claiming "the interface leg is +// clean" off a sync-only measurement generalizes across exactly the boundary that broke the +// class-factory leg. This variant carries a Task-returning auth method so the generated +// LocalQueryAsync really is async, and gets its own marker. +// ============================================================================= + +/// +/// Authorization contract whose method returns Task<bool>, which is what makes the +/// generated interface-factory method async. +/// +public interface ITrimAsyncIfaceAuth +{ + [AuthorizeFactory(AuthorizeFactoryOperation.Execute)] + Task CanQuery(); +} + +/// +/// Trivial auth implementation — no server-only reach, for the reason recorded in +/// SaveCanLegTarget.cs (auth registrations are emitted unguarded). +/// +public sealed class TrimAsyncIfaceAuth : ITrimAsyncIfaceAuth +{ + public Task CanQuery() => Task.FromResult(true); +} + +/// +/// Interface factory whose generated local method is async. +/// +/// +/// WHAT THIS TARGET CAN AND CANNOT MEASURE — read before citing its result. +/// +/// Its markers live on , which the generated +/// LocalQueryAsync reaches only through the interface +/// (GetRequiredService<ITrimAsyncIfaceQuery>() then target.QueryAsync(...)). +/// So those markers are absent by fixture shape whether or not the generated body survives +/// trimming. Their absence is **not** evidence that the feature-switch fold eliminated +/// anything on this leg. It is a no-regression check that the implementation stays off the +/// client, which is worth having and is all it is. +/// +/// +/// This is structural, not a fixture defect that could be tidied up. An interface factory +/// reaches everything through interfaces by design, so no server-only *implementation* name +/// can appear directly in its generated local body. The obvious fix — a [Service] +/// parameter, which would put GetRequiredService<IAsyncLegPort>() straight into +/// the body — was tried and does not compile: the generator strips the service parameter from +/// the proxy's implementing method while the interface still declares it, so the emitted +/// factory fails CS0535. Recorded as Deferred Work item 19; nothing else in the repo uses +/// that shape, which is why it was never caught. +/// +/// +/// Consequence: the async-interface result contributes nothing to the question of whether +/// async bodies fold. That question rests on the static and relay async targets and on the +/// class-factory sync-vs-async pair. +/// +/// +[Factory] +[AuthorizeFactory] +public interface ITrimAsyncIfaceQuery +{ + Task QueryAsync(string key); +} + +/// +/// Server-side implementation for the async interface-factory variant. +/// +public sealed class TrimAsyncIfaceServerSide : ITrimAsyncIfaceQuery +{ + private readonly IAsyncLegPort port; + + public TrimAsyncIfaceServerSide(IAsyncLegPort port) + { + this.port = port; + } + + public Task QueryAsync(string key) + { + return port.AsyncLegInvoke("IfaceAsyncBody_MARKER: " + key); + } +} diff --git a/src/Tests/RemoteFactory.TrimmingTests/LegServerOnlyPorts.cs b/src/Tests/RemoteFactory.TrimmingTests/LegServerOnlyPorts.cs new file mode 100644 index 00000000..bb7fa8e1 --- /dev/null +++ b/src/Tests/RemoteFactory.TrimmingTests/LegServerOnlyPorts.cs @@ -0,0 +1,128 @@ +namespace RemoteFactory.TrimmingTests; + +// ============================================================================= +// PER-LEG SERVER-ONLY DEPENDENCIES (TRIM-008) +// ============================================================================= +// +// The harness previously had ONE server-only dependency (IServerOnlyRepository) +// shared by the class-factory and static-factory targets. That is enough to say +// "something leaked" and not enough to say WHICH leg leaked it — the CI gate's +// failure message could only name the marker, not the culprit. +// +// Each factory shape now gets its own port. A marker in the trimmed output names +// exactly one leg. +// +// NAMING IS LOAD-BEARING. Every name below is chosen so that no marker is a +// substring of another marker, and no marker is a substring of a name that is +// EXPECTED to survive. The harness's original naming failed this: grepping for +// `ServerOnlyRepository` also matched `IServerOnlyRepository`, which is why +// build.yml carries a `(? +/// Server-only dependency of the [FactoryEventHandler<T>] leg. +/// Absent from a trimmed client publish iff the relay-handler body was eliminated. +/// +public interface IRelayLegPort +{ + Task RelayLegInvoke(string payload); +} + +/// +/// Server-side implementation of . Registered only +/// inside the harness's if (NeatooRuntime.IsServerRuntime) block. +/// +public sealed class RelayLegBackend : IRelayLegPort +{ + public Task RelayLegInvoke(string payload) => Task.CompletedTask; +} + +/// +/// Server-only dependency of the interface-factory leg. +/// +public interface IIfaceLegPort +{ + string IfaceLegInvoke(string input); +} + +/// +/// Server-side implementation of . +/// +public sealed class IfaceLegBackend : IIfaceLegPort +{ + public string IfaceLegInvoke(string input) => input; +} + +/// +/// Server-only dependency of the Save/Can* leg. +/// +public interface ISaveLegPort +{ + Task SaveLegInvoke(string operation); +} + +/// +/// Server-side implementation of . +/// +public sealed class SaveLegBackend : ISaveLegPort +{ + public Task SaveLegInvoke(string operation) => Task.CompletedTask; +} + +/// +/// Server-only dependency of the CLASS-factory read path (). +/// +/// +/// Added by TRIM-008's test review. `TrimTestEntity` previously shared +/// with the static-factory target, so a leak in either +/// leg produced the same marker and the CI gate filed both under "static factory". That +/// defeats the per-leg attribution this file exists for — and it matters now, because +/// TRIM-009 works on the class-factory leg and its regressions would have been reported +/// as static-factory failures. +/// +public interface IClassLegPort +{ + string ClassLegInvoke(string input); +} + +/// +/// Server-side implementation of . +/// +public sealed class ClassLegBackend : IClassLegPort +{ + public string ClassLegInvoke(string input) => "ClassLegBackend_MARKER: " + input; +} + +/// +/// Server-only dependency reached ONLY from `async` bodies. +/// +/// +/// Every leg TRIM-008 proved clean was measured with a SYNCHRONOUS server-only body, and the +/// one leg with async bodies (Save/Can*) came back leaking. That made "static and relay are +/// clean" an inference of exactly the class TRIM-009 falsified. These markers convert it into +/// a measurement: an async `[Execute]` method and an async relay handler each reach this port. +/// +public interface IAsyncLegPort +{ + Task AsyncLegInvoke(string input); +} + +/// +/// Server-side implementation of . +/// +public sealed class AsyncLegBackend : IAsyncLegPort +{ + public Task AsyncLegInvoke(string input) => Task.FromResult("AsyncLegBackend_MARKER: " + input); +} diff --git a/src/Tests/RemoteFactory.TrimmingTests/Program.cs b/src/Tests/RemoteFactory.TrimmingTests/Program.cs index 76d30661..8efebb39 100644 --- a/src/Tests/RemoteFactory.TrimmingTests/Program.cs +++ b/src/Tests/RemoteFactory.TrimmingTests/Program.cs @@ -28,6 +28,20 @@ if (NeatooRuntime.IsServerRuntime) { services.AddScoped(); + + // Per-leg server-only dependencies (TRIM-008). Same guard, same reason: a + // registration outside it would root the implementation from the DI graph + // and mask whatever the generator's own preservation is doing. + services.AddScoped(); + 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. + services.AddScoped(); + services.AddScoped(); } // Every named check appends to failedChecks; the process exits non-zero if any @@ -91,6 +105,83 @@ failedChecks.Add("static factory delegate resolution"); } +// Verify the interface-factory leg still registers after trimming (TRIM-008). +// This is the positive control for that leg: the DAM-preservation question is +// asked with absence greps, and an absence grep passes just as well when the +// feature is dead. Resolving the generated proxy proves it is not. +ITrimIfaceQueryFactory? ifaceFactory = null; +try +{ + ifaceFactory = checkScope.ServiceProvider.GetService(); +} +catch (Exception ex) +{ + Console.WriteLine($"Interface factory resolution FAILED: {ex.GetType().Name}: {ex.Message}"); +} +if (ifaceFactory == null) +{ + failedChecks.Add("interface factory resolution"); +} + +// The async interface-factory variant needs its own resolution check. Its type name survives +// via the assembly attribute's DAM whether or not registration works, so asserting it PRESENT +// in the CI gate without resolving it here would be the "absence assertions pass more easily +// when registration is dead" hazard (plan review B2) applied to a positive control. +ITrimAsyncIfaceQueryFactory? asyncIfaceFactory = null; +try +{ + asyncIfaceFactory = checkScope.ServiceProvider.GetService(); +} +catch (Exception ex) +{ + Console.WriteLine($"Async interface factory resolution FAILED: {ex.GetType().Name}: {ex.Message}"); +} +if (asyncIfaceFactory == null) +{ + failedChecks.Add("async interface factory resolution"); +} + +// Verify the Save/Can* leg still registers after trimming (TRIM-008). +// Positive control for the write half of the class-factory shape. +ITrimSaveTargetFactory? saveFactory = null; +try +{ + saveFactory = checkScope.ServiceProvider.GetService(); +} +catch (Exception ex) +{ + Console.WriteLine($"Save target factory resolution FAILED: {ex.GetType().Name}: {ex.Message}"); +} +if (saveFactory == null) +{ + failedChecks.Add("save target 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 +// registrar body folds away entirely — there is no service, delegate, or registry +// entry left to resolve. Its registration counter-signal lives in the untrimmed +// integration suite (FactoryEventHandlerTargets), not in this harness. + +// The async [Execute] delegate needs its own resolution check for the same reason as the +// async interface factory: without it, _DoAsyncWork's markers could go absent because the +// delegate stopped generating rather than because trimming removed the body, and the gate +// would stay green. +TrimTestCommands.DoAsyncWork? doAsyncWorkDelegate = null; +try +{ + doAsyncWorkDelegate = checkScope.ServiceProvider.GetService(); +} +catch (Exception ex) +{ + Console.WriteLine($"DoAsyncWork delegate resolution FAILED: {ex.GetType().Name}: {ex.Message}"); +} +if (doAsyncWorkDelegate == null) +{ + failedChecks.Add("async static factory delegate resolution"); +} + // Direct feature switch test: verifies that the trimmer constant-folds // NeatooRuntime.IsServerRuntime and removes dead code. if (!DirectFeatureSwitchTest.Run()) @@ -131,6 +222,9 @@ Console.WriteLine($"IsServerRuntime: {NeatooRuntime.IsServerRuntime}"); Console.WriteLine($"Class factory resolved: {factory != null}"); Console.WriteLine($"Static factory delegate resolved: {doWorkDelegate != null}"); +Console.WriteLine($"Interface factory resolved: {ifaceFactory != null}"); +Console.WriteLine($"Async interface factory resolved: {asyncIfaceFactory != null}"); +Console.WriteLine($"Save target factory resolved: {saveFactory != null}"); if (failedChecks.Count > 0) { diff --git a/src/Tests/RemoteFactory.TrimmingTests/README.md b/src/Tests/RemoteFactory.TrimmingTests/README.md index c2779207..55e57669 100644 --- a/src/Tests/RemoteFactory.TrimmingTests/README.md +++ b/src/Tests/RemoteFactory.TrimmingTests/README.md @@ -26,11 +26,22 @@ comes with a non-zero exit — new checks must follow that contract (append to # Clean and publish with trimming (net9.0) dotnet publish -c Release -r win-x64 --self-contained true -# Search for server-only IMPLEMENTATION types in output (should return nothing). -# The IServerOnlyRepository interface name is expected to remain — it is referenced -# from guarded-dead LocalCreate bodies the trimmer retains (tracked as TRIM-005). -grep -aob "ServerOnlyDirect" bin/Release/net9.0/win-x64/publish/RemoteFactory.TrimmingTests.dll -grep -aobP '(?] target — the +// string appeared only in a comment. The leg was therefore never measured under +// trimming, which is how the registrar-DAM defect survived on it undetected. +// +// The generator re-opens THIS class to host FactoryServiceRegistrar and (at the +// time of writing) points the assembly attribute at it: +// +// [assembly: NeatooFactoryRegistrar(typeof(RemoteFactory.TrimmingTests.TrimRelayHandlers))] +// +// The attribute's [DynamicallyAccessedMembers(PublicMethods | NonPublicMethods)] +// retains every method on the named type, BODIES INCLUDED. So RelayLegHandlerBody +// — and with it IRelayLegPort, RelayLegInvoke, and the marker literal below — is +// retained on a trimmed client that can never legally call it. +// +// WHY THERE IS NO POSITIVE CONTROL FOR THIS LEG +// +// RelayHandlerRenderer wraps every RegisterHandler call in +// `if (NeatooRuntime.IsServerRuntime)`. On a client publish the registrar body +// folds to nothing, so there is no service, delegate, or registry entry to +// resolve — nothing this harness can assert PRESENT to prove registration still +// works. That counter-signal has to come from the untrimmed integration suite +// (FactoryEventHandlerTargets), not from here. Stated rather than papered over: +// an absence-only check passes just as happily when the feature is dead. +// +// NF0502 TRAP: two static handler methods matching the same event type on one +// class is an ambiguous match — the transform reports and skips, yielding ZERO +// entries and an empty generated registrar. A fixture shaped that way tests +// nothing. Exactly one handler method here, for exactly one event type. +// ============================================================================= + +/// +/// Event carried by the relay-handler leg. A +/// descendant, so the per-assembly event-preservation registrar (TRIM-007) +/// preserves it — it is expected to be PRESENT after trimming. +/// +public record TrimRelayHandlerEvent(int Id, string Message) : FactoryEventBase; + +/// +/// Static server-side handler for . +/// +[FactoryEventHandler] +public static partial class TrimRelayHandlers +{ + /// + /// Server-only work. Reaches , which is registered + /// only behind the harness's IsServerRuntime guard — so on a trimmed client + /// this body is unreachable and every name it mentions should be gone. + /// + internal static Task RelayLegHandlerBody( + TrimRelayHandlerEvent relayEvent, + [Service] IRelayLegPort port, + CancellationToken cancellationToken) + { + // Plain concatenation, not interpolation: an interpolated string can be + // lowered to a DefaultInterpolatedStringHandler call sequence, which + // splits the literal. Concatenation leaves "RelayLegHandlerBody_MARKER: " + // intact in the user-string heap, where a trimmed-DLL grep can find it. + return port.RelayLegInvoke("RelayLegHandlerBody_MARKER: " + relayEvent.Message); + } +} + +/// +/// Second handler class, for a distinct event, whose handler body is async. +/// +/// +/// A separate class rather than a second method on : two static +/// handlers matching the same event type on one class is the NF0502 ambiguous-match shape, and +/// even for distinct event types keeping them apart means a failure names one handler. +/// +/// Exists because every leg TRIM-008 proved clean was measured with a synchronous body, while +/// the one async leg leaked. This converts "async relay handlers are also clean" from inference +/// into measurement. +/// +/// +public record TrimAsyncRelayEvent(int Id, string Message) : FactoryEventBase; + +[FactoryEventHandler] +public static partial class TrimAsyncRelayHandlers +{ + internal static async Task AsyncRelayHandlerBody( + TrimAsyncRelayEvent relayEvent, + [Service] IAsyncLegPort port, + CancellationToken cancellationToken) + { + await port.AsyncLegInvoke("RelayAsyncBody_MARKER: " + relayEvent.Message).ConfigureAwait(false); + } +} diff --git a/src/Tests/RemoteFactory.TrimmingTests/SaveCanLegTarget.cs b/src/Tests/RemoteFactory.TrimmingTests/SaveCanLegTarget.cs new file mode 100644 index 00000000..9b2794d6 --- /dev/null +++ b/src/Tests/RemoteFactory.TrimmingTests/SaveCanLegTarget.cs @@ -0,0 +1,102 @@ +using Neatoo.RemoteFactory; + +namespace RemoteFactory.TrimmingTests; + +// ============================================================================= +// SAVE / CAN* LEG TARGET (TRIM-008, closing plan-review B9) +// ============================================================================= +// +// The harness's existing class-factory target (TrimTestEntity) has a single +// [Remote, Create]. That covers the read half of the class-factory shape and +// nothing else. Save routing (IFactorySaveMeta -> Insert/Update/Delete) and the +// generated Can* methods are a materially different emission path — write +// operations, an auth interface, and a Save() dispatcher — and were never +// measured under trimming. +// +// This target carries the write half. The server-only IP lives in the three +// [Remote] write bodies; each gets its own marker literal so a leak names the +// operation, not just the leg. +// +// AUTH IMPLEMENTATION IS DELIBERATELY DEPENDENCY-FREE +// +// The generator emits `services.TryAddTransient()` +// into FactoryServiceRegistrar WITHOUT an IsServerRuntime guard (see any +// generated *Factory.g.cs for a class carrying [AuthorizeFactory]). An auth +// implementation with a server-only constructor dependency would therefore be +// rooted on the client and would fail ValidateOnBuild — a DIFFERENT preservation +// question from the registrar-DAM one under test here, and conflating the two +// would make this target's result unreadable. TrimSaveAuthRules stays trivial; +// whether it survives trimming is recorded as a measurement, not asserted. +// ============================================================================= + +/// +/// Authorization contract for . Present so the +/// generator emits the Can* surface (CanCreate / CanSave / CanDelete) that this +/// leg exists to measure. +/// +public interface ITrimSaveAuth +{ + [AuthorizeFactory(AuthorizeFactoryOperation.Read | AuthorizeFactoryOperation.Write)] + bool HasAccess(); + + [AuthorizeFactory(AuthorizeFactoryOperation.Delete)] + bool CanDeleteTarget(); +} + +/// +/// Trivial auth implementation — no constructor dependencies, no server-only +/// reach. See the file header for why. +/// +public sealed class TrimSaveAuthRules : ITrimSaveAuth +{ + public bool HasAccess() => true; + + public bool CanDeleteTarget() => true; +} + +/// +/// Class factory exercising Save routing and the generated Can* methods. +/// Implements so the generator emits Save(), +/// which dispatches to Insert / Update / Delete on IsNew and IsDeleted. +/// +[Factory] +[AuthorizeFactory] +public partial class TrimSaveTarget : IFactorySaveMeta +{ + public int Id { get; set; } + + public string? Label { get; set; } + + public bool IsNew { get; set; } = true; + + public bool IsDeleted { get; set; } + + [Remote] + [Create] + internal void Create(string label) + { + Label = label; + } + + [Remote] + [Insert] + internal Task Insert([Service] ISaveLegPort port) + { + IsNew = false; + return port.SaveLegInvoke("SaveLegInsertBody_MARKER: " + Label); + } + + [Remote] + [Update] + internal Task Update([Service] ISaveLegPort port) + { + return port.SaveLegInvoke("SaveLegUpdateBody_MARKER: " + Label); + } + + [Remote] + [Delete] + internal Task Delete([Service] ISaveLegPort port) + { + return port.SaveLegInvoke("SaveLegDeleteBody_MARKER: " + Label); + } +} diff --git a/src/Tests/RemoteFactory.TrimmingTests/ServerOnlyTypes.cs b/src/Tests/RemoteFactory.TrimmingTests/ServerOnlyTypes.cs index 0ba0f557..e27ce819 100644 --- a/src/Tests/RemoteFactory.TrimmingTests/ServerOnlyTypes.cs +++ b/src/Tests/RemoteFactory.TrimmingTests/ServerOnlyTypes.cs @@ -18,14 +18,24 @@ public class ServerOnlyRepository : IServerOnlyRepository { public string DoServerWork(string input) { - return "ServerOnlyRepository_MARKER: " + input; + // Reaches ServerOnlyHelper so the transitive-removal property is genuinely exercised + // rather than merely asserted — see the remarks on ServerOnlyHelper. + return "ServerOnlyRepository_MARKER: " + new ServerOnlyHelper().ProcessData(input); } } /// -/// Another server-only type to verify transitive dependency removal. -/// This is used by ServerOnlyRepository to test whether transitive types are also trimmed. +/// Another server-only type, reached only transitively — +/// is its sole caller. Verifies that removing a server-only body also removes the types that +/// body's callees drag in. /// +/// +/// Its doc comment used to claim exactly this while nothing referenced it at all, so ILLink +/// dropped it unconditionally and its absence proved nothing — it could not have gone red for +/// any defect. It was nevertheless carried in the CI gate under a header asserting every marker +/// there was measured present-before/absent-after. Wired up for real by TRIM-008's test review; +/// the transitive property it names is now actually under test. +/// public class ServerOnlyHelper { public static string HelperMarker => "ServerOnlyHelper_MARKER"; diff --git a/src/Tests/RemoteFactory.TrimmingTests/TrimTestCommands.cs b/src/Tests/RemoteFactory.TrimmingTests/TrimTestCommands.cs index 445f6ae1..ce74a762 100644 --- a/src/Tests/RemoteFactory.TrimmingTests/TrimTestCommands.cs +++ b/src/Tests/RemoteFactory.TrimmingTests/TrimTestCommands.cs @@ -30,10 +30,19 @@ private static Task _DoWork(string input, [Service] IServerOnlyRepositor // Positional records as [Execute] return type (with a nested record) and as a // non-service parameter — the zTreatment StartVisitResultV2 shape (TRIM-001). - // DTO discovery is signature-based, so the body deliberately never constructs - // the records: a `new TrimRecordResult(...)` here would root the ctor from the - // (retained, guarded-dead) method body and make RecordDtoSmokeTest pass even - // without the generator's PreserveType emission — a vacuous check. + // + // DTO discovery is signature-based, so the body deliberately never constructs the + // records. A `new TrimRecordResult(...)` here would root the ctor from this method + // body and make RecordDtoSmokeTest pass even without the generator's PreserveType + // emission — a vacuous check. + // + // This comment used to justify that by calling the body "retained, guarded-dead", + // per the TRIM-005 story that the trimmer keeps guarded-dead bodies. That story was + // disproven, and since TRIM-008 this body is measurably GONE from the trimmed client + // (_ProcessRecord is one of the gate's absence markers). The precaution still stands + // on its own footing though: the fixture must not depend on trimming behavior to stay + // non-vacuous, because a change that started retaining bodies again would silently + // re-root the ctor and turn RecordDtoSmokeTest green for the wrong reason. [Remote] [Execute] private static Task _ProcessRecord(TrimRecordCommand command, [Service] IServerOnlyRepository repo) @@ -41,4 +50,15 @@ private static Task _DoWork(string input, [Service] IServerOnlyRepositor repo.DoServerWork(command.Reason); return Task.FromResult(null); } + + // ASYNC [Execute]. Every leg TRIM-008 proved clean was measured with a synchronous + // server-only body; the one leg with async bodies came back leaking (TRIM-009). Without + // this target, "the static leg is clean" generalizes from sync to async by inference — + // the same inference TRIM-009 falsified for class factories. + [Remote] + [Execute] + private static async Task _DoAsyncWork(string input, [Service] IAsyncLegPort port) + { + return await port.AsyncLegInvoke("StaticAsyncBody_MARKER: " + input).ConfigureAwait(false); + } } diff --git a/src/Tests/RemoteFactory.TrimmingTests/TrimTestEntity.cs b/src/Tests/RemoteFactory.TrimmingTests/TrimTestEntity.cs index c1a83267..ad60e78c 100644 --- a/src/Tests/RemoteFactory.TrimmingTests/TrimTestEntity.cs +++ b/src/Tests/RemoteFactory.TrimmingTests/TrimTestEntity.cs @@ -35,11 +35,58 @@ public class TrimTestEntity public TrimEntityCarriedInfo? Info { get; set; } public TrimEntityCarriedBanner? Banner { get; set; } + // Uses IClassLegPort, not the shared IServerOnlyRepository. Sharing a port with the + // static-factory target meant a leak in either leg produced the same marker, so the CI + // gate filed both under "static factory" — defeating per-leg attribution exactly when + // TRIM-009 starts changing this leg. Retains the IServerOnlyRepository dependency too, + // so the pre-existing markers keep their meaning. [Remote] [Create] - internal void Create(string name, [Service] IServerOnlyRepository repo) + internal void Create(string name, [Service] IServerOnlyRepository repo, [Service] IClassLegPort classPort) { Name = name; - ServerResult = repo.DoServerWork(name); + // ClassSyncBody_MARKER is the sync half of the controlled pair below. It must be a + // literal in THIS body: ClassLegBackend_MARKER lives on the port implementation, which + // is reached through IClassLegPort, so it is behind an interface hop and cannot report + // on whether this body survived. + ServerResult = repo.DoServerWork(name) + classPort.ClassLegInvoke("ClassSyncBody_MARKER: " + name); + } + + // THE CONTROLLED ASYNC COMPARISON. + // + // The earlier sync-vs-async pair (this class's sync Create vs TrimSaveTarget's async + // Insert) was presented as isolating `async`. It does not: those two differ in at least + // four other ways — an [AuthorizeFactory] block, target-from-DI vs target-from- + // parameter, one-hop vs two-hop rooting (there is no InsertDelegate; Insert is reached + // through SaveDelegate -> LocalSave), and an extra catch arm plus lifecycle probes. That + // last difference matters most, because the arc's disproven TRIM-004 story blamed exactly + // "early-throw guard + try/catch defeats unreachable-code elimination". + // + // This method controls all of it. Same class, same factory type, same registrar, same + // absence of auth, same one-hop delegate rooting, same direct concrete call, marker literal + // in the domain body on both sides. Both halves are also rooted TWICE and identically — + // 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. + // + // 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. + [Remote] + [Fetch] + internal async Task FetchAsync(string name, [Service] IClassLegPort classPort) + { + Name = name; + ServerResult = await Task.FromResult(classPort.ClassLegInvoke("ClassAsyncBody_MARKER: " + name)); } } diff --git a/src/Tests/RemoteFactory.TrimmingTests/verify-trimmed.sh b/src/Tests/RemoteFactory.TrimmingTests/verify-trimmed.sh new file mode 100755 index 00000000..c309af12 --- /dev/null +++ b/src/Tests/RemoteFactory.TrimmingTests/verify-trimmed.sh @@ -0,0 +1,291 @@ +#!/usr/bin/env bash +# +# Trimming absence gate. Asserts that server-only code is gone from a +# publish-trimmed client assembly, per factory shape. +# +# Usage: verify-trimmed.sh +# +# WHY THIS IS A SCRIPT AND NOT SHELL-IN-YAML +# +# String absence is only observable from outside the harness process, so this +# check has to live in CI. But a gate that only ever runs in CI is a gate nobody +# can prove works. Every previous version of this check was written straight into +# build.yml and never executed against a known-bad artifact — which is how it came +# to carry an exemption justified by a diagnosis this repo later disproved, and how +# `grep -aq` on a missing path came to print success. As a script it can be run +# against an archived pre-fix DLL and observed FAILING before it is trusted. +# +# TWO ENCODINGS, OR THE CHECK CANNOT FAIL +# +# .NET metadata names (types, methods, parameters) are UTF-8 in the #Strings heap. +# String literals from method bodies are UTF-16LE in the #US heap. A plain +# `grep -a "SomeLiteral_MARKER"` searches raw bytes and therefore NEVER matches a +# UTF-16 literal — it reports "absent" for something sitting right there, and an +# absence gate built on it passes unconditionally. +# +# `tr -d '\000'` collapses UTF-16LE ASCII text down to plain ASCII, giving a second +# view to search. Metadata names contain no NULs, so they survive that view intact +# and both kinds of marker are reachable. This was not a hypothetical: the probe +# used to produce this gate's expectations had exactly this bug, and reported five +# literal markers absent from an assembly that provably contained them. + +set -euo pipefail + +DLL="${1:-}" +if [ -z "$DLL" ]; then + echo "usage: $0 " >&2 + exit 2 +fi + +if [ ! -f "$DLL" ]; then + echo "::error::Trimmed assembly not found at '$DLL'. The gate read nothing, so it proved nothing." + exit 1 +fi + +NONUL="$(mktemp)" +trap 'rm -f "$NONUL"' EXIT +tr -d '\000' < "$DLL" > "$NONUL" + +# Searches both the raw bytes (UTF-8 metadata names) and the NUL-stripped view +# (UTF-16 string literals). +present() { + grep -aqF -- "$1" "$DLL" || grep -aqF -- "$1" "$NONUL" +} + +failures=0 + +fail() { + echo "::error::$1" + failures=$((failures + 1)) +} + +# --------------------------------------------------------------------------- +# POSITIVE CONTROLS — must be PRESENT. +# +# Without these the gate is unfalsifiable: every absence check below passes +# trivially against a truncated, wrong, or unreadable file. The named-type controls are +# UTF-8-only and the message control is UTF-16-only, so a failure in EITHER extraction path +# is caught rather than silently turning that half of the gate into a no-op. +# --------------------------------------------------------------------------- +echo "-- positive controls" +# Named in full, not by prefix. `NeatooEventHandlerRegistrar_` alone is satisfied by the sync +# handler class, so the async handler class could stop generating entirely — taking its markers +# absent for the wrong reason — while the gate stayed green. +for control in \ + "NeatooFactoryRegistrar_TrimTestCommands" \ + "NeatooEventHandlerRegistrar_TrimRelayHandlers" \ + "NeatooEventHandlerRegistrar_TrimAsyncRelayHandlers" \ + "TrimTestCommands" \ + "ITrimIfaceQueryFactory" \ + "ITrimAsyncIfaceQueryFactory" \ + "ITrimSaveTargetFactory" +do + if present "$control"; then + echo " ok $control" + else + fail "Positive control '$control' is MISSING from the trimmed assembly. Either registration was silently removed, or this gate is not reading a real trimmed artifact — do not trust the absence results below." + fi +done + +# UTF-16-only control. Proves the NUL-stripped view was built and searched; if +# this is the sole failure, every literal-marker check below is a no-op. +if present "Trimming verification app completed"; then + echo " ok " +else + fail "UTF-16 positive control missing. The NUL-stripped view is not working, so every string-literal absence check in this gate is vacuous." +fi + +# STOP HERE if any control failed. Printing "do not trust the results below" and then +# printing them anyway is not good enough: the assert-PRESENT blocks emit the most +# action-directing text in this gate ("the async diagnosis is wrong and must be reopened"), +# and a reader scanning ::error:: lines will act on it. Running against an artifact that +# predates those targets produced eight such instructions — advice to reopen a diagnosis, +# generated by a DLL where the target simply did not exist yet. +if [ "$failures" -gt 0 ]; then + echo "::error::$failures positive control(s) failed. This artifact is not a trustworthy trimmed build of the current harness — absence and presence results are NOT being reported, because they would be meaningless and their remediation advice actively misleading." + exit 1 +fi + +# --------------------------------------------------------------------------- +# ABSENCE CHECKS — per leg, so a failure names the culprit. +# +# NOT every marker here is a fix-discriminator. Mixing the two and calling them all +# "measured present before the fix, absent after" is the claim-beyond-evidence this +# arc keeps repeating, so the two kinds are labelled: +# +# [D] discriminator — measured PRESENT pre-fix and ABSENT post-fix. Going red means +# the fix regressed. +# [R] no-regression — absent both before and after. Going red means something NEW +# started leaking. Real value, but it is not evidence that any +# fix worked. +# [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` +# 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 +# markers below (ClassLegBackend, AsyncLegBackend, IfaceLegBackend, SaveLegBackend, +# RelayLegBackend) are reachable only from DI registrations inside the harness's own +# IsServerRuntime block, so they can only go red if the feature switch stops folding +# altogether — which ServerOnlyDirect already covers. They are kept as cheap breadth, not +# relied on as leg-specific signals. +# --------------------------------------------------------------------------- +check_absent() { + local marker="$1" leg="$2" + if present "$marker"; then + fail "[$leg] '$marker' found in the trimmed assembly. Server-only code is shipping to clients." + else + echo " ok $marker" + fi +} + +echo "-- static factory ([Execute])" +# [D] _DoWork, _ProcessRecord, IServerOnlyRepository, DoServerWork — all four measured PRESENT +# pre-fix (plan Current State, 2026-08-12 walk) and absent after. IServerOnlyRepository's +# flip is the headline evidence that the old (?])" +# [D] RelayLegHandlerBody, IRelayLegPort, RelayLegInvoke, RelayLegHandlerBody_MARKER. +# [R] RelayLegBackend — absent pre-fix too (it is DI-registered only behind the harness's +# own IsServerRuntime guard, so nothing ever rooted it). Kept, but NOT fix evidence. +for m in RelayLegHandlerBody IRelayLegPort RelayLegInvoke RelayLegHandlerBody_MARKER; do + check_absent "$m" "relay handler" +done +check_absent "RelayLegBackend" "relay handler" +for m in AsyncRelayHandlerBody RelayAsyncBody_MARKER; do + check_absent "$m" "relay handler (async)" +done + +echo "-- interface factory" +# [R] all of them — absent pre-fix as well, since this leg never had the defect. +# NOTE what these do and do not prove. An interface factory reaches its implementation +# through the INTERFACE (GetRequiredService() then target.LookupAsync), +# so the implementation body is never statically reachable from generated code and its +# absence follows from that indirection — NOT from feature-switch folding. Useful as a +# no-regression check on the implementation staying off the client; not evidence about +# guard elimination. +for m in TrimIfaceServerSide IIfaceLegPort IfaceLegInvoke IfaceLegBackend IfaceLegServerBody_MARKER; do + check_absent "$m" "interface factory" +done +# The async variant carries the SAME caveat, and it is structural rather than fixable here: +# an interface factory reaches everything through interfaces, so no server-only implementation +# name can appear directly in its generated local body. These markers cannot report on whether +# the async body folded — they are no-regression checks only. See the remarks on +# ITrimAsyncIfaceQuery, and Deferred Work item 19 for why the obvious fix does not compile. +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)" +# [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 +# interface hop, so nothing statically reaches them either way. +for m in ClassLegBackend ClassLegBackend_MARKER; do + check_absent "$m" "class factory" +done + +echo "-- async-only port (shared by the three async targets above)" +for m in IAsyncLegPort AsyncLegInvoke AsyncLegBackend; do + check_absent "$m" "async port" +done + +# --------------------------------------------------------------------------- +# THE CONTROLLED PAIR — sync vs async inside ONE class factory. +# +# ClassSyncBody_MARKER lives in TrimTestEntity.Create (sync) -> expected ABSENT +# ClassAsyncBody_MARKER lives in TrimTestEntity.FetchAsync (async) -> expected PRESENT +# +# Same class, same generated factory, same registrar, neither carrying [AuthorizeFactory], +# both one-hop rooted by their own delegate registration, both reached by a direct call on the +# concrete type, both literals in the domain body rather than behind an interface hop. +# The earlier sync/async comparison (this class's Create vs TrimSaveTarget's Insert) was NOT +# 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. + +# +# 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. +# --------------------------------------------------------------------------- +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 +done + +# --------------------------------------------------------------------------- +# KNOWN-BROKEN — asserted PRESENT on purpose (TRIM-009). +# +# 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. +# +# 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. +# --------------------------------------------------------------------------- +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 +done + +echo +if [ "$failures" -gt 0 ]; then + echo "::error::Trimming verification FAILED ($failures check(s))." + exit 1 +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." diff --git a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs index b8a9778a..1e5526e8 100644 --- a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs +++ b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/AssemblyAttributeEmissionTests.cs @@ -1,3 +1,4 @@ +using Microsoft.CodeAnalysis; using RemoteFactory.UnitTests.TestContainers; namespace RemoteFactory.UnitTests.FactoryGenerator; @@ -108,15 +109,14 @@ internal void Create() { } #region Static Factory - /// - /// Static factory generated source contains the assembly-level NeatooFactoryRegistrar - /// attribute with the fully-qualified static class type name. - /// - [Fact] - public void StaticFactory_EmitsAssemblyAttribute() - { - var source = @" + // The usings are required, not decorative: without System.Threading.Tasks the fixture's + // Task is an error type, and StaticFactory_RegistrarHolder_ForwardsToUserClass — + // which asserts the generated output actually compiles — fails on the fixture rather than + // on the emission. String-containment assertions never noticed. + private const string StaticFactorySource = @" using Neatoo.RemoteFactory; +using System.Threading; +using System.Threading.Tasks; namespace TestNamespace { @@ -131,7 +131,75 @@ private static Task _DoWork(string input) } } "; - var (_, _, runResult) = DiagnosticTestHelper.RunGenerator(source); + + /// + /// Static factory generated source contains the assembly-level NeatooFactoryRegistrar + /// attribute naming the correct type. + /// + /// + /// Retargeted by TRIM-008. The original pinned typeof(global::TestNamespace.MyCommands) + /// — the user's own class — which is the defect: the attribute's + /// [DynamicallyAccessedMembers(PublicMethods | NonPublicMethods)] retains every method on + /// whatever type it names, bodies included, so that shipped _DoWork's server-only + /// body to trimmed clients. The test's intent is unchanged and deliberately preserved: + /// "the registrar attribute is emitted, and it names the correct type". Only what counts + /// as correct has changed, from the consumer's class to the generated forwarding holder. + /// + [Fact] + public void StaticFactory_EmitsAssemblyAttribute() + { + var (_, _, runResult) = DiagnosticTestHelper.RunGenerator(StaticFactorySource); + + var generatedSource = runResult.GeneratedTrees + .FirstOrDefault(t => t.FilePath.Contains("MyCommands")) + ?.GetText() + ?.ToString(); + + Assert.NotNull(generatedSource); + Assert.Contains("[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::TestNamespace.NeatooFactoryRegistrar_MyCommands))]", generatedSource); + } + + /// + /// The static factory's registrar attribute must NOT name the consumer's own class. + /// This is the assertion whose absence let the defect ship. + /// + /// + /// The closing paren in the expected substring is load-bearing. Without it, + /// typeof(global::TestNamespace.MyCommands is a prefix of any name that merely + /// starts with the user's type, and the assertion would pass or fail for the wrong + /// reason. The holder is prefixed rather than suffixed for the same reason — a suffixed + /// MyCommandsNeatooFactoryRegistrar would keep the user's FQN a substring of the + /// holder's, making this a false red. + /// + [Fact] + public void StaticFactory_AssemblyAttribute_DoesNotNameConsumerType() + { + var (_, _, runResult) = DiagnosticTestHelper.RunGenerator(StaticFactorySource); + + var generatedSource = runResult.GeneratedTrees + .FirstOrDefault(t => t.FilePath.Contains("MyCommands")) + ?.GetText() + ?.ToString(); + + Assert.NotNull(generatedSource); + Assert.DoesNotContain("NeatooFactoryRegistrar(typeof(global::TestNamespace.MyCommands))", generatedSource); + } + + /// + /// The holder forwards to the registrar that stays on the user's partial class. + /// + /// + /// Forwarding, not hosting: [Execute] methods are private static by the repo's own + /// convention and the registrar body calls them, so a sibling holder that hosted the + /// registrar would be CS0122. The method name is pinned because + /// AddRemoteFactoryServices looks it up by that literal string and calls + /// method?.Invoke — a rename stops registration silently, with no diagnostic + /// and no exception. + /// + [Fact] + public void StaticFactory_RegistrarHolder_ForwardsToUserClass() + { + var (_, outputCompilation, runResult) = DiagnosticTestHelper.RunGenerator(StaticFactorySource); var generatedSource = runResult.GeneratedTrees .FirstOrDefault(t => t.FilePath.Contains("MyCommands")) @@ -139,7 +207,21 @@ private static Task _DoWork(string input) ?.ToString(); Assert.NotNull(generatedSource); - Assert.Contains("[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::TestNamespace.MyCommands))]", generatedSource); + + // The method signature must be asserted INSIDE the holder, not merely present in the + // file. The user's partial class emits a byte-identical FactoryServiceRegistrar + // signature, so a bare Contains(...) for it is satisfied by that one and would still + // pass if the holder's method were renamed — which is the single failure mode that is + // silent, because AddRemoteFactoryServices looks it up by literal name and calls + // method?.Invoke. Anchoring the signature to the holder's class declaration is what + // actually pins plan Constraint "the holder's method must remain exactly + // FactoryServiceRegistrar". + Assert.Matches( + @"internal static class NeatooFactoryRegistrar_MyCommands\s*\{\s*internal static void FactoryServiceRegistrar\(IServiceCollection services, NeatooFactory remoteLocal\)", + generatedSource); + Assert.Contains("global::TestNamespace.MyCommands.FactoryServiceRegistrar(services, remoteLocal);", generatedSource); + + Assert.Empty(outputCompilation.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error)); } #endregion @@ -177,4 +259,225 @@ public interface IMyService } #endregion + + #region Relay Handler + + // The [FactoryEventHandler] leg had NO emission tests before TRIM-008 — which is + // how its registrar attribute went four releases naming the consumer's own class, + // and unqualified, without anything noticing. + // + // NF0502 TRAP: two static handlers matching the same event type on one class is an + // ambiguous match. The transform reports and `continue`s without adding an entry, and + // FactoryGenerator returns early on Entries.Count == 0 — so the fixture would generate + // NOTHING and every assertion below would fail for a reason unrelated to what it tests. + // Exactly one handler, one event type. + private const string RelayHandlerSource = @" +using Neatoo.RemoteFactory; +using System.Threading; +using System.Threading.Tasks; + +namespace TestNamespace +{ + public record MyEvent(int Id) : FactoryEventBase; + + public interface IMyPort + { + Task Send(string message); + } + + [FactoryEventHandler] + public static partial class MyHandlers + { + internal static Task Handle(MyEvent evt, [Service] IMyPort port) + { + return port.Send(""handled""); + } + } +} +"; + + /// + /// Relay-handler generated source contains the assembly-level NeatooFactoryRegistrar + /// attribute naming the generated holder, global::-qualified. + /// + [Fact] + public void RelayHandler_EmitsAssemblyAttribute() + { + var (_, _, runResult) = DiagnosticTestHelper.RunGenerator(RelayHandlerSource); + + var generatedSource = runResult.GeneratedTrees + .FirstOrDefault(t => t.FilePath.Contains("MyHandlers")) + ?.GetText() + ?.ToString(); + + Assert.NotNull(generatedSource); + Assert.Contains("[assembly: Neatoo.RemoteFactory.NeatooFactoryRegistrar(typeof(global::TestNamespace.NeatooEventHandlerRegistrar_MyHandlers))]", generatedSource); + } + + /// + /// The relay-handler registrar attribute must NOT name the consumer's own handler class. + /// + /// + /// Naming the handler class made ILLink retain every method on it, so the handler body + /// and the server-only service it reaches shipped to trimmed clients. Measured present + /// before this fix and absent after, in the publish-trimmed harness. + /// + [Fact] + public void RelayHandler_AssemblyAttribute_DoesNotNameConsumerType() + { + var (_, _, runResult) = DiagnosticTestHelper.RunGenerator(RelayHandlerSource); + + var generatedSource = runResult.GeneratedTrees + .FirstOrDefault(t => t.FilePath.Contains("MyHandlers")) + ?.GetText() + ?.ToString(); + + Assert.NotNull(generatedSource); + Assert.DoesNotContain("NeatooFactoryRegistrar(typeof(global::TestNamespace.MyHandlers))", generatedSource); + + // The pre-fix emission was also missing global::. Pin its absence explicitly: + // an unqualified argument binds to the wrong type when a consumer namespace + // shadows the first segment of this one. + Assert.DoesNotContain("NeatooFactoryRegistrar(typeof(TestNamespace.", generatedSource); + } + + /// + /// The relay-handler holder forwards to the registrar on the user's partial class. + /// + /// + /// The holder prefix differs from the static-factory leg's on purpose. A class carrying + /// both [Factory] and [FactoryEventHandler<T>] already emits duplicate + /// FactoryServiceRegistrar members (CS0111, broken at HEAD, Deferred Work item 15); + /// a shared prefix would stack a CS0101 duplicate-type error on top of it. + /// + [Fact] + public void RelayHandler_RegistrarHolder_ForwardsToUserClass() + { + var (_, _, runResult) = DiagnosticTestHelper.RunGenerator(RelayHandlerSource); + + var generatedSource = runResult.GeneratedTrees + .FirstOrDefault(t => t.FilePath.Contains("MyHandlers")) + ?.GetText() + ?.ToString(); + + Assert.NotNull(generatedSource); + + // Anchored to the holder's class declaration for the same reason as the static leg: + // the user's partial class emits an identical signature, so an unanchored assertion + // would still pass with the holder's method renamed — and that rename fails silently + // at runtime (method?.Invoke). + Assert.Matches( + @"internal static class NeatooEventHandlerRegistrar_MyHandlers\s*\{\s*internal static void FactoryServiceRegistrar\(IServiceCollection services, NeatooFactory remoteLocal\)", + generatedSource); + Assert.Contains("global::TestNamespace.MyHandlers.FactoryServiceRegistrar(services, remoteLocal);", generatedSource); + + // Distinct from the static-factory prefix, so the two never collide. + Assert.DoesNotContain("NeatooFactoryRegistrar_MyHandlers", generatedSource); + } + + /// + /// The relay-handler output compiles without errors. + /// + /// + /// Not redundant with the string assertions above. Relay output bypasses + /// NormalizeWhitespace entirely, and FactoryRenderer parses with error + /// recovery and swallows throws into a /* Error: */ comment — so malformed + /// emission yields MANGLED OUTPUT, NOT AN EXCEPTION. String containment can pass on + /// source that does not compile. TRIM-008 adds a whole new top-level type to this + /// unnormalized output, which is exactly the change that could produce that. + /// + [Fact] + public void RelayHandler_GeneratedOutputCompilesWithoutErrors() + { + var (_, outputCompilation, runResult) = DiagnosticTestHelper.RunGenerator(RelayHandlerSource); + + // Without this, the test passes on ZERO generated trees: if the fixture ever hits + // NF0502 or any transform early-out, the generator emits nothing, the input + // compilation is clean, and Assert.Empty(errors) is trivially satisfied. The other + // relay tests would fail in that state; this one would not. + Assert.NotNull(runResult.GeneratedTrees.FirstOrDefault(t => t.FilePath.Contains("MyHandlers"))); + + var errors = outputCompilation.GetDiagnostics() + .Where(d => d.Severity == DiagnosticSeverity.Error) + .ToList(); + + Assert.Empty(errors); + } + + /// + /// A private static handler compiles — the reason the holder forwards rather than hosts. + /// + /// + /// FactoryGenerator.RelayHandler filters only on IsStatic, with no accessibility + /// gate, so private handlers are legal today. That is precisely why a sibling holder cannot + /// *host* the registrar (it could not reach them — CS0122) and must forward to the user's + /// partial instead. The static leg already exercises this via its `private static` fixture; + /// the relay leg did not. + /// + [Fact] + public void RelayHandler_PrivateHandler_GeneratedOutputCompilesWithoutErrors() + { + var source = RelayHandlerSource.Replace( + "internal static Task Handle(", + "private static Task Handle("); + + // If the shared fixture's text drifts, Replace silently no-ops and this test quietly + // becomes a duplicate of the internal-handler test — green, and testing nothing. + Assert.NotEqual(RelayHandlerSource, source); + Assert.Contains("private static Task Handle(", source); + + var (_, outputCompilation, runResult) = DiagnosticTestHelper.RunGenerator(source); + + Assert.NotNull(runResult.GeneratedTrees.FirstOrDefault(t => t.FilePath.Contains("MyHandlers"))); + Assert.Empty(outputCompilation.GetDiagnostics().Where(d => d.Severity == DiagnosticSeverity.Error)); + } + + /// + /// A class carrying BOTH [Factory] (static) and [FactoryEventHandler<T>] + /// still fails with CS0111 only — the per-leg holder prefixes must not add CS0101 on top. + /// + /// + /// This shape is broken at HEAD and deliberately not fixed (Deferred Work item 15): both + /// renderers re-open the same partial and each emits FactoryServiceRegistrar, which is + /// CS0111 duplicate-member. TRIM-008 asserted in three places that distinct holder prefixes + /// keep it at CS0111 rather than compounding it with CS0101 duplicate-type, and tested it + /// nowhere. This pins that claim so a future prefix change cannot quietly worsen an already + /// broken shape. + /// + [Fact] + public void BothAttributes_EmitsDuplicateMemberOnly_NotDuplicateType() + { + var source = @" +using Neatoo.RemoteFactory; +using System.Threading; +using System.Threading.Tasks; + +namespace TestNamespace +{ + public record MyEvent(int Id) : FactoryEventBase; + + [Factory] + [FactoryEventHandler] + public static partial class MyBoth + { + [Execute] + private static Task _DoWork(string input) => Task.FromResult(input); + + internal static Task Handle(MyEvent evt) => Task.CompletedTask; + } +} +"; + var (_, outputCompilation, _) = DiagnosticTestHelper.RunGenerator(source); + + var errorIds = outputCompilation.GetDiagnostics() + .Where(d => d.Severity == DiagnosticSeverity.Error) + .Select(d => d.Id) + .Distinct() + .ToList(); + + Assert.Contains("CS0111", errorIds); + Assert.DoesNotContain("CS0101", errorIds); + } + + #endregion } diff --git a/src/Tests/RemoteFactory.UnitTests/TestContainers/DiagnosticTestHelper.cs b/src/Tests/RemoteFactory.UnitTests/TestContainers/DiagnosticTestHelper.cs index d18b7855..7d476a1b 100644 --- a/src/Tests/RemoteFactory.UnitTests/TestContainers/DiagnosticTestHelper.cs +++ b/src/Tests/RemoteFactory.UnitTests/TestContainers/DiagnosticTestHelper.cs @@ -102,15 +102,36 @@ private static List BuildReferences() { MetadataReference.CreateFromFile(typeof(object).Assembly.Location), MetadataReference.CreateFromFile(typeof(Task).Assembly.Location), - MetadataReference.CreateFromFile(remoteFactoryAssembly.Location) + MetadataReference.CreateFromFile(remoteFactoryAssembly.Location), + + // Generated factory code declares FactoryServiceRegistrar(IServiceCollection, ...) + // and calls AddScoped / TryAddTransient / GetRequiredService; class factories also + // reach for ILogger. Without these, EVERY generated tree fails to compile in this + // harness for reasons that have nothing to do with the code under test. + // + // Added by TRIM-008. Their absence was not costing correctness — nothing asserted on + // OutputCompilation — but it silently capped what this helper could ever detect: a + // generator change that emits uncompilable source was undetectable here, and the + // relay-handler renderer in particular has no other error signal (it bypasses + // NormalizeWhitespace, and FactoryRenderer swallows parse failures into a + // /* Error: */ comment rather than throwing). + MetadataReference.CreateFromFile(typeof(Microsoft.Extensions.DependencyInjection.IServiceCollection).Assembly.Location), + MetadataReference.CreateFromFile(typeof(Microsoft.Extensions.DependencyInjection.ServiceCollection).Assembly.Location), + MetadataReference.CreateFromFile(typeof(Microsoft.Extensions.Logging.ILogger<>).Assembly.Location) }; - // Add System.Runtime reference + // Add System.Runtime, plus System.ComponentModel — which is where IServiceProvider + // is surfaced for reference purposes. Generated registrars and factories take an + // IServiceProvider, so without it the output compilation reports CS0012 on every + // generated tree. var runtimeAssemblyPath = Path.GetDirectoryName(typeof(object).Assembly.Location); - var systemRuntimePath = Path.Combine(runtimeAssemblyPath!, "System.Runtime.dll"); - if (File.Exists(systemRuntimePath)) + foreach (var name in new[] { "System.Runtime.dll", "System.ComponentModel.dll" }) { - references.Add(MetadataReference.CreateFromFile(systemRuntimePath)); + var path = Path.Combine(runtimeAssemblyPath!, name); + if (File.Exists(path)) + { + references.Add(MetadataReference.CreateFromFile(path)); + } } return references;