From abd0b6941cfdf91476129d98cbeeb18d2f8c7ab8 Mon Sep 17 00:00:00 2001 From: Keith Voels Date: Wed, 12 Aug 2026 21:30:26 -0500 Subject: [PATCH] =?UTF-8?q?fix(test):=20close-out=20audit=20findings=20?= =?UTF-8?q?=E2=80=94=20caching=20guard=20never=20covered=20EventHandlerEnt?= =?UTF-8?q?ry?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Step 7 close-out audit returned CONCERNS with four veto-tier findings. Two were defects in TRIM-006 itself, both self-reported as verified. V1 — the caching guard's fixture declared two matching handlers for the SAME event type, which is the NF0502 ambiguous-match shape: the transform reports the diagnostic and continues without adding an entry. RelayHandlerModel.Entries was therefore empty, so EventHandlerEntry.Parameters/ServiceParameters/ AllParameters — the three fields TRIM-006 converted to EquatableArray — were never constructed and never guarded. The RelayHandler red/green transition was real but driven entirely by Usings and Diagnostics. The fixture's own comment claimed the opposite. Fixed by giving the fixture two distinct event types with one matching handler each. Coverage re-proven rather than assumed: reverting EventHandlerEntry .Parameters to IReadOnlyList now turns the RelayHandler case red, which it provably did not before. Adds two fixture-health tests, because "the fixture silently stopped exercising the thing" was the actual failure mode and nothing detected it: Fixture_ProducesNoDiagnostics and Fixture_EmitsRelayHandlerOutput. The first caught a second latent defect on its first run — [Fetch] on an interface-factory member (NF0106), which had been degrading the FactoryInterface branch too. V2 — the "generated output is byte-identical" evidence was vacuous. It rested on zero git status drift under Generated/, but .gitignore:405 is **/Generated/ with zero tracked files, so git could not have reported drift regardless. Replaced with a real measurement: IntegrationTests' full generated tree emitted with the pre-fix generator and with HEAD's, diffed recursively — 256 files byte-identical, including all 16 .FactoryEventHandler.g.cs. The conclusion held; the evidence had not. V3 — TRIM-007's plan header said In Progress against a Done Index row. V4 — plan-review findings B8 and B10 had no disposition; both now carried in the Deferred Work table. Also: DtoTypeWalker's header comment still named one caller when it now has three (C7 — this file's stale header was itself a TRIM-001 finding), and the TRIM-007 log entry's "Amend" decision is corrected in place per the append-only rule. Tests: 603+603 unit, 561+561 integration, 86+86 Design, 0 failures. Co-Authored-By: Claude Opus 5 (1M context) --- .../006-incremental-cache-regression-test.md | 14 ++- ...7-subscribe-only-event-preservation-fix.md | 4 +- .../reviews/close-out-audit.md | 97 +++++++++++++++++++ .../todo.md | 39 +++++++- src/Generator/DtoTypeWalker.cs | 8 +- .../Core/IncrementalCacheTests.cs | 64 ++++++++++-- 6 files changed, 209 insertions(+), 17 deletions(-) create mode 100644 docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/close-out-audit.md diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/006-incremental-cache-regression-test.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/006-incremental-cache-regression-test.md index f14b040a..c25393d4 100644 --- a/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/006-incremental-cache-regression-test.md +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/006-incremental-cache-regression-test.md @@ -3,7 +3,7 @@ **Plan #:** 006 **Date:** 2026-08-11 **Related Todo:** [../todo.md](../todo.md) -**Status:** In Progress +**Status:** Done **Last Updated:** 2026-08-12 **Plan-review opt-in:** No (test-first plan, narrow blast radius, emitted output unchanged; the diagnosis was verified at the keyboard at draft time rather than inherited — see Current State) **Code-review opt-in:** Yes (touches generator transform-output types) @@ -87,7 +87,7 @@ Filled 2026-08-12, before the Step 5 gate. Logs in the session scratchpad; per-r | Acceptance bullet (short) | Tier declared | Test method | Tier confirmed | |---|---|---|---| | Every branch stays cached across an unrelated edit | `[unit]` | `IncrementalCacheTests.UnrelatedEdit_TransformOutputStaysCached` — `[Theory]`, 4 cases (`FactoryClass`, `FactoryInterface`, `RelayHandler`, `FactoryEvents`); non-vacuity backed by `Fixture_ExercisesEveryPipelineBranch` | `[unit]` | -| Emitted output byte-identical before/after | `[unit]` | `UnrelatedEdit_GeneratedOutputIsIdentical` covers run-to-run determinism. The *before/after-this-plan* half is **not** a test — it is verified by zero `git status` drift in the committed `Generated/` trees of both solutions after a full rebuild that re-emitted them (relay-handler file re-emitted 09:42:33, checked 09:44:01) | `[unit]` + repo-artifact diff | +| Emitted output byte-identical before/after | `[unit]` | `UnrelatedEdit_GeneratedOutputIsIdentical` covers run-to-run determinism. The *before/after-this-plan* half is **not** a test — it is a one-off measurement: `IntegrationTests`' full generated tree emitted with the pre-fix generator (`710498c^:RelayHandlerModel.cs`) into a clean directory, then with HEAD's, diffed recursively → **256 files byte-identical, including all 16 `.FactoryEventHandler.g.cs`**. (The original evidence here — zero `git status` drift under `Generated/` — was **vacuous**: `.gitignore:405` is `**/Generated/`, so git could not have reported drift. Corrected at close-out audit V2.) | `[unit]` + one-off emission diff | | Relay-handler branch caches | `[unit]` | Same `[Theory]`, `RelayHandler` case — observed `Modified` → `Unchanged` across the fix | `[unit]` | | Guard sensitivity (negative control) | `[explicit-skip]` | Performed at the keyboard, not committed — see Implementation Record. All four branches proven to go red on a reference-equality field | `[explicit-skip]` — honored | | Existing diagnostic suite unchanged | `[explicit-skip]` | Covered by full-suite run; `RunGenerator`'s signature and single-run behavior untouched (extraction of `BuildReferences` only) | `[explicit-skip]` — honored | @@ -101,7 +101,7 @@ Filled 2026-08-12, before the Step 5 gate. Logs in the session scratchpad; per-r **Red → green (Step 5).** Guard against unmodified HEAD: `RelayHandler` failed with reason `Modified`, the other three cached — pre-flight's prediction confirmed exactly, and the guard proven non-vacuous before it was made to pass. After moving `RelayHandlerModel.Usings/Entries/Diagnostics` and `EventHandlerEntry.Parameters/ServiceParameters/AllParameters` onto `EquatableArray`: 6/6 green. Constructors normalize incoming sequences rather than taking the array type, so a future call site cannot reintroduce the defect by passing a plain list. -**Negative control (Step 6).** Added a `IReadOnlyList` auto-property with a fresh-allocation initializer to `TypeInfo` (branches 1–2) and `FactoryEventInfo` (branch 4), covering the three branches that were *already* passing — the RelayHandler red above is the control for branch 3. Result: `FactoryClass`, `FactoryInterface`, `FactoryEvents` all went red with reason `Modified`; `RelayHandler` stayed `Unchanged`. Probes and the throwaway dump test removed; `git status` confirms no residue. +**Negative control (Step 6).** Added a `IReadOnlyList` auto-property with a fresh-allocation initializer to `TypeInfo` (branches 1–2) and `FactoryEventInfo` (branch 4), covering the three branches that were *already* passing — the RelayHandler red above is the control for branch 3. Result: `FactoryClass`, `FactoryInterface`, `FactoryEvents` all went red with reason `Modified`; `RelayHandler` stayed `Unchanged`. Probes and the throwaway dump test removed; `git status` confirms no residue. **This control tested transform-output *roots* only** — it did not reach nested element types, which is why the `EventHandlerEntry` gap survived it. See Plan Amendments; a fourth control at the nested level was added post-merge. **Finding — the guard can read stale generator code locally.** The first negative-control run *passed*, which was wrong. Cause: the generator is loaded once per process via `Assembly.LoadFrom` into a static `Lazy`, so rebuilding only the generator and re-running with `--no-build` let a surviving testhost serve the previously loaded assembly. A dump of all tracked steps proved the probe was live in the DLL while the guard reported green. Rebuilding the test project produced the correct red. Documented in `RunGeneratorTracked`'s remarks; CI is unaffected because every run starts cold. Recorded in the todo's Discovery Log — this will bite the next person doing a red/green cycle on any dynamically-loaded-generator test, not just this one. @@ -111,7 +111,13 @@ Filled 2026-08-12, before the Step 5 gate. Logs in the session scratchpad; per-r ## Plan Amendments -(None yet.) +**2026-08-12 — post-merge correction from the close-out audit (V1, V2).** Recorded as an amendment because Step 4 was reported met when it was not. + +- **Step 4 was unmet and reported as met.** The fixture declared two matching handlers for the *same* event type, which is the NF0502 ambiguous-match shape: the transform reports the diagnostic and `continue`s without adding an entry. `RelayHandlerModel.Entries` was therefore empty, and `EventHandlerEntry.Parameters` / `ServiceParameters` / `AllParameters` — the three fields this plan converted — were never constructed and never guarded. The `RelayHandler` red→green transition recorded below was real but was driven entirely by `Usings` and `Diagnostics`; the nested type was never in play. The fixture's own comment asserted the opposite. Fixed: two distinct event types, one matching handler each; comment corrected to name the NF0502 trap; coverage re-proven by reverting `EventHandlerEntry.Parameters` to `IReadOnlyList` and observing the `RelayHandler` case go red — coverage the guard provably did not have before. +- **Two fixture-health tests added**, because "the fixture silently stopped exercising the thing" was the actual failure mode and nothing detected it: `Fixture_ProducesNoDiagnostics` (a diagnostic means some transform took an early-out and never built the model under guard) and `Fixture_EmitsRelayHandlerOutput` (the relay output stage returns early on `Entries.Count == 0`, so an empty-entry fixture emits no file). The first caught a **second** latent defect on its first run: `[Fetch]` on an interface-factory member (NF0106), which had been degrading the `FactoryInterface` branch too. +- **The byte-identity evidence was vacuous** and is replaced — see the Test Evidence row. + +Root cause worth carrying: both defects were *self-reported as verified*. The lesson is the same one TRIM-005 produced — a claim of verification is not verification — and it recurred inside the very plan written to enforce non-vacuity. --- diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/007-subscribe-only-event-preservation-fix.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/007-subscribe-only-event-preservation-fix.md index c6731eac..b09aed69 100644 --- a/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/007-subscribe-only-event-preservation-fix.md +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/plans/007-subscribe-only-event-preservation-fix.md @@ -3,8 +3,8 @@ **Plan #:** 007 **Date:** 2026-07-13 **Related Todo:** [../todo.md](../todo.md) -**Status:** In Progress -**Last Updated:** 2026-07-13 +**Status:** Done +**Last Updated:** 2026-08-12 (status header reconciled with the Plan Index at close-out audit V3; the plan completed and merged via PR #71 on 2026-07-13) **Plan-review opt-in:** Yes (new incremental-generator pipeline branch; generator emission contract change; corrects documented behavior that currently overpromises) **Code-review opt-in:** Yes (behavior-changing generator work) diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/close-out-audit.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/close-out-audit.md new file mode 100644 index 00000000..ee493e35 --- /dev/null +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/reviews/close-out-audit.md @@ -0,0 +1,97 @@ +# TRIM — Close-Out Audit + +**Date:** 2026-08-12 +**Mode:** Step 7 close-out (whole arc, findings-only) +**Branch audited:** `TRIM` at `3aed354` (fast-forwarded to `main`, all plan PRs merged) +**Verdict:** **CONCERNS** — 4 veto-tier findings, all since addressed (see Disposition below) + +--- + +## Headline + +AC1–AC3 are genuinely verified in a publish-trimmed artifact at HEAD, with CI evidence (GitHub Actions run **31658596913** at `3aed354`, linux-x64 self-contained, `TrimMode=full`, `IsServerRuntime: False` — all four preservation smokes PASSED). This is **not** a repeat of TRIM-003, where a preservation claim was assumed and came back red. + +The veto findings concentrated in TRIM-006 — the one plan that received no independent per-plan gate, its gates having been folded into this audit. + +--- + +## Veto-Tier Findings and Disposition + +### V1 — TRIM-006's guard never exercised `EventHandlerEntry` — FIXED + +The fixture declared two matching static handlers (`Notify`, `Audit`) for the **same** event type on one `[FactoryEventHandler]` class. That is the NF0502 ambiguous-match shape: `FactoryGenerator.RelayHandler.cs:160-174` reports the diagnostic and `continue`s **without adding an entry**. + +Consequences, all confirmed independently before accepting the finding: + +- `RelayHandlerModel.Entries` was empty, so `EventHandlerEntry.Parameters` / `ServiceParameters` / `AllParameters` — the exact three fields TRIM-006 converted to `EquatableArray` — were never constructed and never guarded. +- The observed `RelayHandler` red→green transition was driven entirely by `RelayHandlerModel.Usings` and `.Diagnostics`. The nested element type was never in play. +- `FactoryGenerator.cs:101` returns early on `Entries.Count == 0`, so `RelayHandlerRenderer.Render` never ran either. +- The fixture's own doc comment asserted the opposite — a false statement in a test, worse than no comment. +- Plan Step 4 ("populate every collection-bearing field on each transform output") was unmet with no Plan Amendment recording the divergence. + +**Fix:** fixture now carries two *distinct* event types with one matching handler each, so `Entries` holds two values and each entry's three parameter collections are populated. Comment corrected to state the NF0502 trap explicitly. Two fixture-health tests added: `Fixture_ProducesNoDiagnostics` and `Fixture_EmitsRelayHandlerOutput`. + +**Verified, not assumed:** with the corrected fixture, reverting `EventHandlerEntry.Parameters` to `IReadOnlyList` turns the `RelayHandler` case **red** — coverage the guard provably did not have before. Restored after. + +**The health assertion immediately earned its keep:** it caught a *second* latent fixture defect on its first run — `[Fetch]` on an interface-factory member (NF0106; the interface *is* the boundary, so operation attributes are invalid there). That had been silently degrading the `FactoryInterface` branch too. + +### V2 — the "byte-identical output" evidence was vacuous — FIXED + +The plan cited "zero `git status` drift in the committed `Generated/` trees," explicitly framed as "verified empirically rather than by inference." + +`.gitignore:405` is `**/Generated/` and `git ls-files | grep "Generated/"` returns **zero** tracked files. Those trees are untracked and ignored, so `git status` could not report drift regardless of what the generator emitted. The check proved nothing. (`reviews/005-plan-review.md:83` had already recorded that generated files are not git-tracked.) + +**Fix:** replaced with a real measurement. Emitted `RemoteFactory.IntegrationTests`' full generated tree with the pre-fix generator (`710498c^:src/Generator/Model/RelayHandlerModel.cs`) into a clean directory, then with HEAD's generator, and diffed recursively: **256 files, byte-identical, including all 16 `.FactoryEventHandler.g.cs` files**. The conclusion was correct; the original evidence for it was not. + +### V3 — TRIM-007's plan file contradicted the Plan Index — FIXED + +`plans/007-...md:6` read `Status: In Progress` while `todo.md` showed `007 | Done` and commit `602a6d4` was titled "mark TRIM-007 Done." Header reconciled, with a note that the reconciliation is dated 2026-08-12 while the plan itself completed 2026-07-13. + +### V4 — three verified `005-plan-review.md` findings had no disposition — FIXED + +B8 and B10 were routed nowhere. Both re-verified rather than inherited: + +- **B8** — `grep -rn "AppContext.SetSwitch" src/` returns nothing, and `"Server-only method called in non-server runtime."` appears only in generator renderers, never in a test assertion. **Nothing pins the guard's runtime throw.** +- **B10** — `InternalVisibilityTests.cs:235-236,274-275,280-281,322-323` slices generated text with naive `IndexOf` arithmetic delimited by the next member name; an emission reorder mis-slices and the `DoesNotContain` assertions pass vacuously — the same false-green class TRIM-001's test gate caught as its marquee finding. + +**Fix:** both given explicit dispositions in the Deferred Work Carrying Forward table in `todo.md`. + +--- + +## Acceptance Criteria Trace + +| Criterion | Evidence | Holds? | +|---|---|---| +| **AC1** — positional record as return / parameter / nested property deserializes publish-trimmed | Bucket walk `DtoTypeWalker.WalkDtoGraph:156-197`; trimmed proof `RecordDtoSmokeTest.cs:Run:25-79` (JSON-literal deserialization, no record constructed anywhere). CI 31658596913: "Record DTO smoke PASSED" | **Yes** | +| **AC2** — DTO reachable only as a `[Factory]` entity property survives trimming | `DtoTypeWalker.WalkEntityProperties:207-214` via `FactoryGenerator.Types.cs:257-272`; trimmed proof `EntityPropertyDtoSmokeTest.cs:Run:21-67`. CI: "Entity property DTO smoke PASSED" | **Yes** | +| **AC3** — subscribe-only `FactoryEventBase` record deserializes publish-trimmed, verified not assumed | Branch 4 `FactoryGenerator.cs:113-130`; `EventPreservationRenderer.cs:27-103` (registrar targets a **generated** type at `:71`); trimmed proof `EventSubscribeOnlySmokeTest.cs:Run:74-146` (string-literal `TypeFullName`, no `typeof`, no construction). CI: "Subscribe-only event smoke PASSED" | **Yes** | +| **AC4** — `docs/trimming.md` updated + release notes | Docs half landed (`docs/trimming.md:254`, `:266-285`, `:287-314`). Release half **open**: version still `1.6.1`, no `v1.7.0.md` | **Open — deliberately held** | +| **AC5** — consumer proof via zTreatment PCB-003 | Not started; blocked on the held release | **Open** | + +--- + +## Build & Test Evidence + +- Build: `Build succeeded. 3 Warning(s), 0 Error(s)` — 2× WASM workload warnings in an unrelated example project, 1× pre-existing `CA1062`. +- Tests: **2324 passed, 0 failed, 10 skipped** — UnitTests 601+601, IntegrationTests 561+561 (5 skips × 2 TFMs). +- Design solution: 86 + 86 passed, 0 failed. +- Skipped inventory: `RelayTimingTests.cs:56,:105` (user decision, this arc) + 3 pre-existing "Optional Performance Demo" tests. +- Trimmed-artifact gate at HEAD: CI run 31658596913, all four smokes passed, "Server-only implementation types absent from trimmed assembly." One pre-existing `IL2057` from `ServiceAssemblies.FindType`, untouched by this arc. + +--- + +## Container Integrity + +7 plan files, 7 Index rows, no orphans, numbering monotonic with no duplicates. TRIM-005's Abandoned status carries a substantive Abandonment Reason naming the real seam and the successor lesson. Skipped Steps entries exist for all three non-run gates (004, 003, 006). Out of Scope holds — `git diff v1.6.1..HEAD -- src/` contains zero `IFactorySaveMeta` hits and no zTreatment-side work. + +**Sacred tests:** only comment-only edits to `FactoryEventHandlerTests.cs` and `FactoryEventBaseAttributeTests.cs`, plus two `[Fact(Skip=...)]` additions carrying user-decision reasons. No assertions removed, no expected values bent, no reflection introduced. + +--- + +## Verified Non-Findings + +Recorded because they were explicitly challenged: + +- **No other transform-output type carries a reference-equality collection at HEAD.** The full reachable graph of all three transform outputs (`TypeInfo`, `RelayHandlerModel`, `FactoryEventInfo` → `TypeFactoryMethodInfo`, `TypeAuthMethodInfo`, `MethodInfo`, `MethodParameterInfo`, `OrdinalPropertyInfo`, `AspAuthorizeInfo`, `DiagnosticInfo`, `EventHandlerEntry`, `ParameterModel`) uses `EquatableArray` throughout. +- **`ExecuteDelegateModel.ServiceParameters` is outside the cache boundary**, as suspected but unverified when flagged. It is constructed only at `Builder/FactoryModelBuilder.cs:482`, and `FactoryModelBuilder.Build` is called only from the two `RegisterSourceOutput` lambdas — output-stage, not transform-output. No defect. +- **`skills/.../class-factory.md:318,333-334` and `advanced-patterns.md:227` describe *class* factories**, which `ClassFactoryRenderer.cs:54` targets at the generated type and which the 2026-08-11 trimmed probe showed trim correctly. These were **over-listed** as falsified in the 2026-08-11 Discovery Log entry; they are arguably true as written. diff --git a/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md b/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md index b1f37252..5d47b00c 100644 --- a/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md +++ b/docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md @@ -46,7 +46,7 @@ A third suspected gap turned out to be already fixed: event records derive `Fact | 003 | Done | [Verify event-record preservation needs no consumer entries](./plans/003-verify-event-record-preservation.md) | `FactoryEventBase` DAM annotation shipped v1.4.0; consumer entries predate it, never re-tested — **verification came back RED**, re-split → TRIM-007 | | 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 | In Progress | [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) | +| 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) | 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.) @@ -54,6 +54,36 @@ Execution order: 004 → 001 → 002 → 003 → 007 → 005 → 006 (rows liste - TRIM-004 — `test-reviewer` gate skipped (test-infrastructure-only plan: every Acceptance bullet is `explicit-skip`; the harness itself is the test artifact, evidence recorded in the plan's Test Evidence table). - TRIM-003 — `test-reviewer` gate skipped (verification-only plan whose deliverable is the finding itself; non-vacuity proven by the red-trimmed / green-untrimmed / green-annotated triplet recorded in the plan's Test Evidence; the repro check lands under TRIM-007's full gate). +- TRIM-006 — per-plan `test-reviewer` and `code-reviewer` gates **folded into the Step 7 close-out audit** (user decision 2026-08-12) rather than run separately. PR #73 merged ahead of the gate; the close-out audit covers TRIM-006's code as part of the whole-arc pass. Note this is a genuine deviation, not a no-op: the per-plan `test-reviewer` pass that would have scrutinised the guard's own coverage never happened, so the close-out audit is the only independent review this plan receives. **This deviation cost something real** — the audit found the guard's fixture never exercised `EventHandlerEntry` at all (V1), a gap a per-plan test review would have been far likelier to catch before merge. Fixed post-merge; see `reviews/close-out-audit.md`. + +## Close-Out Audit + +**2026-08-12 — Verdict: CONCERNS → addressed.** Full record: [`reviews/close-out-audit.md`](./reviews/close-out-audit.md). + +AC1–AC3 confirmed genuinely verified in a publish-trimmed artifact at HEAD (CI run 31658596913 at `3aed354`) — not a repeat of TRIM-003's assumed-preservation failure. Four veto-tier findings, all concentrated in TRIM-006 (the plan whose per-plan gates were folded into this audit) plus container bookkeeping; all four fixed: + +- **V1** — the caching guard's fixture hit NF0502 (two handlers, one event), so `RelayHandlerModel.Entries` was empty and `EventHandlerEntry`'s three converted collections were never constructed *or guarded*. Fixture corrected to two distinct events; two fixture-health tests added; coverage re-proven by reverting a nested field and observing red. The new health test immediately caught a second latent defect (NF0106 `[Fetch]` on an interface-factory member). +- **V2** — the "byte-identical output" claim rested on `git status` over `**/Generated/`, which is **gitignored** — the check could not have detected drift. Replaced with a real 256-file recursive diff of pre-fix vs post-fix emission. Conclusion held; the evidence had not. +- **V3** — TRIM-007's plan header said `In Progress` against a `Done` Index row. Reconciled. +- **V4** — `005-plan-review.md` findings B8 and B10 had no disposition anywhere. Now carried in the table below. + +## Deferred Work Carrying Forward + +| # | 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 | +| 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 | ## Discovery Log @@ -89,6 +119,7 @@ Execution order: 004 → 001 → 002 → 003 → 007 → 005 → 006 (rows liste ### 2026-07-13 — TRIM-007 (gates cleared, merged) - **Finding:** Generator-emission fix landed (PR #71, CI green first run): fourth pipeline branch + per-assembly `NeatooEventPreservationRegistrar`; TRIM-003's red check green in the pure consumer shape incl. nested record. Plan review's veto (accessibility gate — the repo's own private nested test events would have broken every consumer build) folded pre-implementation. Test gate cleared (10 unit tests incl. determinism + FQN-decoy guards). Code review caught 3 veto doc findings — the falsified DAM claim surviving in `FactoryEventRelayPattern.cs`, `docs/factory-events.md`, and the smoke test's own summary — all fixed, plus skill-reference and IL2026-justification callouts. Reviews: `reviews/007-*.md`. - **Decision:** Amend. +- **Correction (2026-08-12, close-out audit C8):** "Amend" is the wrong word and no Plan Amendment was ever written, which is why `plans/007-...md` still reads "Plan Amendments — (None yet.)". What actually happened was a *pre-implementation plan revision*: the plan-review veto was folded into the Steps before any code was written, which the workflow treats as ordinary Step-2 revision, not a post-hoc amendment. Nothing is missing from the plan; the decision word here was imprecise. Entry left standing rather than rewritten, per the append-only rule. - **Follow-up:** n/a — remaining queue: TRIM-005, TRIM-006, then todo-level release step (AC4 release notes deferred there). ### 2026-07-07 — TRIM-003 (verification RED, re-split → TRIM-007) @@ -135,3 +166,9 @@ Execution order: 004 → 001 → 002 → 003 → 007 → 005 → 006 (rows liste - **Finding:** `DiagnosticTestHelper` loads `Neatoo.Generator.dll` once per process via `Assembly.LoadFrom` into a static `Lazy`. Rebuilding **only** the generator project and re-running `dotnet test --no-build` can let a surviving testhost serve the previously loaded assembly, so a test reports on code that is no longer on disk. Caught during TRIM-006's negative control: the probe field was provably present in the loaded DLL (`grep` on the binary, plus a full tracked-step dump showing `Modified`) while the guard reported 6/6 green; rebuilding the test project produced the correct red. This is not specific to the caching guard — it applies to **every** test that goes through `DiagnosticTestHelper`, which is effectively the whole generator suite. A generator fix can appear verified when it was never loaded. - **Decision:** Document at the seam rather than re-architect the loader — `RunGeneratorTracked`'s remarks now carry the warning. CI is unaffected (cold build every run), so the exposure is local iteration only. - **Follow-up:** If this bites again, the durable fix is to make the helper fail fast on a generator DLL older than the test assembly. Deliberately not done here — out of TRIM-006's scope, and the plan's Constraints forbid reworking the shared helper. + +### 2026-08-12 — Release held: shipping now would publish falsified IP-protection claims +- **Finding:** With all plans landed, the arc is functionally releasable — AC1–AC3 are met and verified in the trimmed artifact, `docs/trimming.md` and the Design projects were updated during TRIM-001/002/007, and the `feat:` commits since `v1.6.1` put the bump at 1.7.0. But the registrar-DAM defect (2026-08-11 entry) falsifies live documentation that would ship with it. `docs/trimming.md:35` claims `[Execute]` static factories' "registration lambdas and their captured dependencies" are removed by the trimmer; `:222` then accurately describes the very mechanism that makes that false ("preserve all methods on the referenced type"). The two passages contradict each other in the same published page, and `:7` sells IP protection as a headline benefit. Releasing would tell users their `[Remote]` bodies are stripped from `[Execute]` and `[FactoryEventHandler]` classes when they demonstrably are not. +- **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. diff --git a/src/Generator/DtoTypeWalker.cs b/src/Generator/DtoTypeWalker.cs index ce7d26be..27aabc54 100644 --- a/src/Generator/DtoTypeWalker.cs +++ b/src/Generator/DtoTypeWalker.cs @@ -1,7 +1,11 @@ // DtoTypeWalker.cs // Shared walker for discovering DTO types reachable from a root symbol. -// Used by the factory-signature path (MethodInfo.DiscoverDtoTypes) for both -// return types and non-service parameters. Discovered types bucket-sort by +// Three callers, all bucketing into the same two registries: +// - factory signatures (MethodInfo.DiscoverDtoTypes) — return types and +// non-service parameters +// - [Factory] entity property graphs (FactoryGenerator.Types.cs, TRIM-002) +// - FactoryEventBase descendant graphs (FactoryGenerator.Events.cs, TRIM-007) +// Discovered types bucket-sort by // constructor shape: parameterless -> DtoConstructorRegistry.Register(), // parameterized-only -> DtoConstructorRegistry.PreserveType(). diff --git a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/Core/IncrementalCacheTests.cs b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/Core/IncrementalCacheTests.cs index 790ff74c..728edc29 100644 --- a/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/Core/IncrementalCacheTests.cs +++ b/src/Tests/RemoteFactory.UnitTests/FactoryGenerator/Core/IncrementalCacheTests.cs @@ -56,8 +56,9 @@ public interface INotifier // populates DtoReturnTypes / DtoPreserveTypes on TypeInfo. public record OrderSummary(int Id, string Label); - // Branch 4: FactoryEventBase descendant. + // Branch 4: FactoryEventBase descendants. public record OrderPlacedEvent(int OrderId, string CustomerEmail) : FactoryEventBase; + public record OrderShippedEvent(int OrderId, string Carrier) : FactoryEventBase; // Branch 1: [Factory] class — multiple methods, services, DTO in the signature. [Factory] @@ -80,18 +81,25 @@ public void Create(string name) } } - // Branch 2: [Factory] interface. + // Branch 2: [Factory] interface. No operation attributes on the members — the + // interface IS the remote boundary, and [Fetch] here is NF0106. [Factory] public interface IOrderReader { - [Fetch] OrderSummary? Read(int id); } - // Branch 3: [FactoryEventHandler] — two handlers, each with non-service - // parameters, service parameters, and a CancellationToken, so Entries and all - // three parameter collections on EventHandlerEntry are non-empty. + // Branch 3: [FactoryEventHandler] — TWO DISTINCT event types, one matching + // handler each, so RelayHandlerModel.Entries holds two EventHandlerEntry values + // and each entry's Parameters / ServiceParameters / AllParameters are non-empty. + // + // The two attributes must name DIFFERENT events. Two handlers for the SAME event + // is the NF0502 ambiguous-match shape: the transform reports the diagnostic and + // `continue`s WITHOUT adding an entry, leaving Entries empty and every + // EventHandlerEntry collection unconstructed — the guard would then cover none of + // them while appearing to. Fixture_ProducesNoDiagnostics pins this. [FactoryEventHandler] + [FactoryEventHandler] public static partial class OrderHandlers { internal static async Task Notify( @@ -103,11 +111,11 @@ internal static async Task Notify( } internal static async Task Audit( - OrderPlacedEvent orderEvent, + OrderShippedEvent orderEvent, [Service] INotifier notifier, CancellationToken cancellationToken) { - await notifier.SendAsync(""audit@example.com"", ""audited""); + await notifier.SendAsync(""audit@example.com"", orderEvent.Carrier); } } } @@ -166,6 +174,46 @@ public void UnrelatedEdit_TransformOutputStaysCached(string trackingName) + "value-equatable all the way down."); } + /// + /// Fixture health. A branch can report tracked steps while its transform bailed + /// early on a diagnostic, leaving the very collections under test unconstructed — + /// the guard then covers nothing and still passes. That is not hypothetical: the + /// first version of this fixture declared two handlers for one event, hit NF0502, + /// and silently guarded an empty Entries for the whole of TRIM-006. + /// + [Fact] + public void Fixture_ProducesNoDiagnostics() + { + var (_, second) = DiagnosticTestHelper.RunGeneratorTracked(Fixture, UnrelatedAppendix); + + var nf = second.Diagnostics.Where(d => d.Id.StartsWith("NF")).ToList(); + + Assert.True( + nf.Count == 0, + "The caching fixture must generate cleanly. A diagnostic means some transform took an " + + "early-out path and never built the model the guard is supposed to protect: " + + string.Join("; ", nf.Select(d => $"{d.Id} {d.GetMessage()}"))); + } + + /// + /// Pins that the relay-handler branch actually reached emission. Complements + /// : the relay output stage returns early + /// when Entries.Count == 0, so an empty-entry fixture produces no file here. + /// + [Fact] + public void Fixture_EmitsRelayHandlerOutput() + { + var (_, second) = DiagnosticTestHelper.RunGeneratorTracked(Fixture, UnrelatedAppendix); + + var files = second.GeneratedTrees.Select(t => Path.GetFileName(t.FilePath)).ToList(); + + Assert.True( + files.Any(f => f.EndsWith(".FactoryEventHandler.g.cs")), + "No relay-handler source was emitted, so RelayHandlerModel.Entries was empty and " + + "EventHandlerEntry's collections were never constructed — the RelayHandler case of " + + $"the caching guard would be vacuous. Emitted: {string.Join(", ", files)}"); + } + [Fact] public void Fixture_ExercisesEveryPipelineBranch() {