Skip to content
Original file line number Diff line number Diff line change
Expand Up @@ -3,8 +3,8 @@
**Plan #:** 001
**Date:** 2026-07-06
**Related Todo:** [../todo.md](../todo.md)
**Status:** In Progress
**Last Updated:** 2026-07-06
**Status:** Done
**Last Updated:** 2026-07-06 (closed: PR #69 merged; test gate cleared, code review clean, CI trimming gate green on linux-x64 after one unrelated flaky-test re-run)
**Plan-review opt-in:** Yes (changes the generator's emission contract for every consumer's registrar; touches documented behavior in docs/trimming.md and CLAUDE-DESIGN.md; incremental-pipeline equality semantics involved)
**Code-review opt-in:** Yes (behavior-changing generator work)

Expand Down

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# TRIM-002 Code Review (Step 5, opt-in) — 2026-07-06

**Reviewer:** code-reviewer agent, findings-only (no grade). Range `68315e7..788f0ac`. Logs grepped, not re-run.

**Result: no veto-tier findings.** Deliverable clean; shape verified right.

## Verified

- `WalkEntityProperties` is a thin adapter over the existing `WalkProperties` → `WalkDtoGraph` machinery — no parallel discovery surface; entity root never bucketed; child-entity properties neither bucketed nor descended.
- Insertion guarded by the same `!IsInterface && !IsStatic` condition as `CollectOrdinalProperties`; merges into the existing bucket HashSets; no renderer change; no cross-renderer duplication.
- Exception-safety on unusual symbols (generic entities, type parameters, error types, nested/struct symbols): no throwing shape found.
- Plan-review compliance complete: B1 boundary documented + pinned by test; B2 LazyLoad ACCEPTED amendment with pinning test; B3 ordinal orthogonality holds.
- No reflection introduced; both relay skips faithful (bodies/assertions intact); repo rules respected (0 errors, only pre-existing WASM workload warnings).
- Logs: units 581+581 green; integration 561+561 green sequential (authoritative); trimmed harness all checks passed exit 0.

## Callout-tier findings

1. **Doc-comment misplacement in `TrimTestEntity.cs`** (cosmetic): the carried-DTO types were inserted between `TrimTestEntity`'s summary and its declaration, leaving a double-summary on `TrimEntityCarriedInfo` and none on the entity. **Fixed immediately post-review** (summary re-attached to the entity declaration).
2. **Pre-existing, record-only:** `HashSet` → `EquatableArray` emission ordering is not byte-stable across build *processes* (string hash randomization). Incremental caching is NOT affected (per-process determinism holds; cross-process runs have cold caches anyway) and registrations are order-independent/idempotent. Only relevant if byte-reproducible generated output ever becomes a requirement — would be a project-wide sort, not a TRIM-002 fix. Accepted.
3. **Record-only:** the two parallel-run relay-family failures in `002-test.log` are the already-user-accepted flaky family; sequential run green.
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
# TRIM-002 Plan Review — 2026-07-06

**Reviewer:** plan-reviewer agent (two-pass).
**Verdict: APPROVED** — no veto findings either pass. Five callouts, all folded into the draft before implementation.

## Pass A — vs documented requirements

- The one documented-boundary change (`docs/trimming.md:283` "preserve it yourself" sentence; `CLAUDE-DESIGN.md:296` FAQ row) is exactly the delta the todo's Goal + Acceptance Criterion 2 sanction, and Step 7 targets it.
- "What Qualifies as a DTO" tables not contradicted — entities still never become DTOs; only their DTO-typed properties are discovered. No conflict with entity-duality guidance.
- **A1 (callout):** Step 7 must also rewrite the nested-discovery *prose* ("properties of each discovered DTO" framing), not just the boundary sentence. Anchors: `trimming.md:279,:283`; `CLAUDE-DESIGN.md:296,:789`. → Folded into Step 7.

## Pass B — vs codebase

Reality check passed: insertion seam (`FactoryGenerator.Types.cs:236-257`), model/renderer flow ("no renderer change" holds), over-preservation trade-off confirmed real and consistent (`RegisterFactories` runs every registrar regardless of mode; emissions idempotent).

- **B1 (callout, key):** "That type's own registrar owns its graph" is over-broad — a class implementing a `[Factory]` interface without carrying the attribute is rejected by `IsDtoStructureCandidate` (`DtoTypeWalker.cs:114-120`) but never gets a TypeInfo/registrar (`ForAttributeWithMetadataName` matches direct application only, `FactoryGenerator.cs:19-29`) — its property graph is covered by nobody. Diagnosis, not a live gap: those are stateless server-only service impls, never serialized. → Invariant narrowed; Constraints line added; no machinery.
- **B2 (callout, key):** `LazyLoad<T>` suspicion corrected at the code: `T` **is** walked (descent through the public `Value` getter, `LazyLoad.cs:176-181`); the only artifact is a benign spurious `Register<LazyLoad<T>>` emission (`[JsonConstructor] public LazyLoad()` at `LazyLoad.cs:92-97` puts it in the Register bucket; it deserializes via `LazyLoadJsonConverterFactory`, never the registry). → Step 4 rewritten with the corrected diagnosis; fix-vs-accept at keyboard; queue needs a stub row.
- **B3 (callout):** Ordinal interplay clean — different outputs to different registries; entity walk skips factory-typed (ordinal-serialized) properties. → Constraints note added.
- **B4 (callout):** `System` segment-match hardening sound; no BCL namespace is System-prefixed beyond `System`/`System.*`; no test pins the old over-exclusion.
- **B5 (callout):** Child-vs-parent registrar assertions need per-tree generated text — `runResult.GeneratedTrees` per-tree `FilePath` embeds the factory hint name; the existing concatenating helper cannot distinguish registrars. → Already in Current State; carried to test design.

## Disposition

| # | Finding | Disposition |
|---|---------|-------------|
| B1 | Interface-factory impl classes outside the walk | Invariant narrowed; Constraints exception documented |
| B2 | LazyLoad diagnosis corrected | Step 4 rewritten |
| A1 | Nested-discovery prose anchors | Step 7 anchors added |
| B4 | Hardening safe | No change needed |
| B3/B5 | Ordinal orthogonality / per-tree helper | Constraints note; test-design note |

Calibration: diagnoses adopted; all prescriptions matched the code walk and were adopted as-is.
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# TRIM-002 Test Review (Step 5 Gate) — 2026-07-06

**Reviewer:** test-reviewer agent, two passes (initial + closure).
**Logs:** `002-build.log` (0 errors), `002-test.log` (full parallel run: units 581+581 green; 2 pre-existing relay-family flakes in integration), `002-test-integration-seq.log` (integration 561+561 green both TFMs, `xUnit.MaxParallelThreads=1` — the authoritative integration signal), `002-publish.log`, `002-harness-run.log` (trimmed run: all checks passed, exit 0).
**Gate result: CLEARED** — no open must-cover or should-cover findings.

## Initial pass

Evidence map verified honest (all 9 cited methods exist, load-bearing assertions at declared tier; both zTreatment failure shapes directly mirrored; per-tree child-vs-parent assertion non-vacuous; harness types confirmed never constructed repo-wide). Findings:

1. **should-cover (plan):** no test for a DTO carried on the entity's *base class* (entity-root base-chain application).
2. **should-cover (plan):** no test for a `[Factory] record` aggregate's self-walk.
3. **nice-to-have:** `System.*` exclusion half of the hardening unpinned; cross-walk dedupe untested; `FactoryTree` substring-match fragility.
4. **tech-debt (visibility):** the FactoryEventRelay integration family is parallel-load flaky *beyond* the two skipped `RelayTimingTests` members — different members flake per run, all pass isolated/sequential.
5. **process:** the trimmed-harness *run* output wasn't in the provided logs.
6. **sacred tests:** both `RelayTimingTests` skips verified faithful (bodies/assertions intact, reasons recorded) — with the honest note that the relay post-return ordering contract now has no active guard in the default parallel run.

## Response and closure

| Finding | Disposition |
|---|---|
| Base-class property | **CLOSED** — `EntityBaseClassDtoProperty_Discovered` (abstract base traversed) |
| Record entity self-walk | **CLOSED** — `RecordEntitySelfWalk_CarriedDtoDiscovered` (residual: primary-ctor-record permutation accepted as nice-to-have; walk gate is independent of the create-path) |
| `System.*` exclusion / dedupe / helper anchoring | **CLOSED** — `SystemFrameworkTypeProperty_StillExcluded`, `DtoInBothSignatureAndEntityProperty_SingleEmission`, `FactoryTree` anchored to `.{hint}.g.cs` |
| Harness run evidence | **CLOSED** — `002-harness-run.log` captured |
| Relay-family parallel flakiness | **ACCEPTED-WITH-REASON** — user declined queueing (Discovery Log records the family-wide pattern) |
| Relay skips coverage-loss | **ACCEPTED-WITH-REASON** — standing user decision; on record |

## Closing tier picture

- must-cover: none (never open).
- should-cover: both closed.
- nice-to-have: all closed.
- tech-debt: one accepted-with-reason (user decision).

13 unit tests total; negative control (entity walk disabled → carried-DTO ctor stripped → harness exit 1) verified at the keyboard per the TRIM-001 precedent.
8 changes: 7 additions & 1 deletion docs/todos/TRIM-dto-trimming-preservation-gaps/todo.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ A third suspected gap turned out to be already fixed: event records derive `Fact
| # | Status | Plan | Source |
|-----|--------|------|--------|
| 004 | Done | [Trimming harness pass/fail semantics + CI gate](./plans/004-trimming-harness-ci-gate.md) | 2026-07-06 recon: TrimmingTests outside .sln/CI, exits 0 on failure — 001–003's trimmed acceptance signals need this gate first |
| 001 | Draft | [Positional-record preservation in factory signatures](./plans/001-positional-record-signature-preservation.md) | `DtoTypeWalker.WalkFactoryReturn` `HasParameterlessCtor` gate; zTreatment cut-over `StartVisitResultV2` hotfix |
| 001 | Done | [Positional-record preservation in factory signatures](./plans/001-positional-record-signature-preservation.md) | `DtoTypeWalker.WalkFactoryReturn` `HasParameterlessCtor` gate; zTreatment cut-over `StartVisitResultV2` hotfix |
| 002 | Draft | [`[Factory]` entity property-graph DTO discovery](./plans/002-factory-entity-property-dto-discovery.md) | `WalkFactoryReturn` bails on `[Factory]` roots without descending; zTreatment `TreatmentBanner` / `DashboardContactResult` hotfixes |
| 003 | Draft | [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 |
| 005 | Draft | [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` |
Expand Down Expand Up @@ -76,13 +76,19 @@ Execution order: 004 → 001 → 002 → 003 → 005 → 006 (rows listed in exe
- **Finding:** `RelayTimingTests.Relay_FiresAfterCallerSynchronousWriteOnContinuation` (integration, event relay) failed with `TimeoutException` on net9.0 under full-suite parallel load, passed in isolation and on the next full run. Unrelated to TRIM's generator changes — timing-sensitive test.
- **Decision:** Defer.
- **Follow-up:** flagged to user — out-of-goal tech debt; queue as sibling todo or accept as known flake (not queued in TRIM).
- **Resolution (2026-07-06):** flaked again on PR #69's first CI run (green on re-run). User decision: test marked `[Fact(Skip = ...)]`, no todo. Its sibling `Relay_FiresAfterCallerContinuation_InNoSyncContextHost` flaked with the identical signature during the TRIM-002 gate run (green in isolation) — same decision applied, also skipped.

### 2026-07-06 — TRIM-001 (gate closed)
- **Finding:** Test-review gate returned zero must-cover gaps but caught false trimmed-harness coverage: the constructed-body harness design let the return/nested checks pass with the emission disabled (guarded-dead bodies root ctors — the TRIM-005 behavior). Harness redesigned so no record is ever constructed; negative controls v1+v2 now prove each shape depends on `PreserveType`. Added `record struct` + cross-method dedupe unit tests from the should-cover tier. Long form: TRIM-001 Plan Amendment + `reviews/001-test-review.md`.
- **Decision:** Amend.
- **Index changes:** add TRIM-006 (incremental-cache regression test — pre-existing tech debt, plan review B1), executed last.
- **Follow-up:** TRIM-006.

### 2026-07-06 — TRIM-002 (gate closed)
- **Finding:** Test gate CLEARED with zero must-cover; two should-covers (base-class property, `[Factory]` record self-walk) and three nice-to-haves closed with tests; harness run log captured. New visibility item: the FactoryEventRelay integration family is parallel-load flaky *beyond* the two skipped members (different members flake per run; all green isolated and with `MaxParallelThreads=1`) — user previously declined queueing, recorded here for the close-out audit. Long form: `reviews/002-test-review.md`.
- **Decision:** Amend.
- **Follow-up:** n/a.

### 2026-07-06 — TRIM-001 (code review clean)
- **Finding:** Opt-in code review returned zero veto findings (B1/B2 compliance, emission placement, semantics, docs all verified — `reviews/001-code-review.md`). One low-confidence pre-existing callout: `IsDtoStructureCandidate`'s `StartsWith("System")` prefix match would exclude a consumer namespace like `Systems.Domain` from preservation.
- **Decision:** Amend.
Expand Down
6 changes: 4 additions & 2 deletions docs/trimming.md
Original file line number Diff line number Diff line change
Expand Up @@ -278,9 +278,11 @@ If you return or accept a plain DTO class **or a positional record** through any

**Nested DTOs are automatically discovered.** The generator recursively walks public instance properties (including inherited properties) of each discovered DTO type — classes and records alike — to find nested DTOs that also need preservation. Collection properties (`List<T>`, `IReadOnlyList<T>`, arrays) and nullable properties (`T?`) are unwrapped to find the inner type. The same eligibility criteria and bucket rule apply to nested DTOs as to direct signature types. Cycle detection prevents infinite recursion from circular references.

For example, if a factory method returns `ParentDto` which has a `List<ChildDto> Children` property, both `ParentDto` and `ChildDto` are automatically registered — no additional action is needed.
**DTOs carried as `[Factory]` entity properties are automatically discovered.** Every class carrying `[Factory]` directly also walks its own public property graph (inherited properties included) during generation and emits preservation for reachable DTOs in its own `FactoryServiceRegistrar` — so a DTO or record that only rides on an aggregate (never appearing in a factory method signature itself) is still trimming-safe. The entity itself is never treated as a DTO (entities are preserved via DI registration), and entity-typed properties are not walked by the *parent* — each `[Factory]` class's own registrar covers its own graph.

If you have a DTO that is **not** returned by any factory method and **not** reachable as a property of a discovered DTO, you need to preserve it yourself. See [Microsoft's documentation on preserving dependencies](https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming#dynamicdependency).
For example, if a factory method returns `ParentDto` which has a `List<ChildDto> Children` property, both `ParentDto` and `ChildDto` are automatically registered — no additional action is needed. Likewise, if an `[Execute]`-opened aggregate carries a `Banner` record property, the record is preserved through the aggregate's own registrar.

If you have a DTO that is **not** in any factory method signature, **not** reachable as a property of a discovered DTO, and **not** reachable through a `[Factory]` entity's public property graph, you need to preserve it yourself. See [Microsoft's documentation on preserving dependencies](https://learn.microsoft.com/en-us/dotnet/core/deploying/trimming/prepare-libraries-for-trimming#dynamicdependency). (One deliberate boundary: a class that merely *implements* a `[Factory]` interface — an interface-factory service implementation — gets no property walk; those are stateless services, not serialized state.)

## Factory Event Type Preservation

Expand Down
Loading
Loading