Skip to content

Make the one-construction-path pin real: migrate 7 spec helpers, then scan IL (SPEC F193.4) #828

Description

@genwave-radio

🎯 What the pin promises

SPEC F193.4 pins that the Orchestrator has one construction path:

Fitness pin: new Orchestrator( occurs only in OrchestratorBuilder and AddOrchestration
(text scan over src/ + tests/).

The property matters because it is most of why the gh-#401 split was safe to do at all: when the
Orchestrator's constructor changes, there is exactly one place to fix. STORY-451 (v5.10.0) bought
that property by replacing every scattered construction with OrchestratorBuilder.

📍 What it actually enforces

tests/GenWave.Architecture.Tests/Support/ConstructorCallScan.cs matches the literal string
new Orchestrator(. It finds 2 files. But 9 sites construct an Orchestrator — the other 7 use
target-typed return new(…) inside a static Orchestrator BuildOrchestrator(…) helper, which the
text scan cannot see:

Site Form
src/GenWave.Orchestration/OrchestrationServiceCollectionExtensions.cs:198 explicit ✅ seen
tests/GenWave.TestSupport/OrchestratorBuilder.cs:306 explicit ✅ seen
tests/GenWave.Orchestration.Tests/Specs/Gh254_BoundaryFitSelection.cs target-typed ❌ invisible
tests/GenWave.Orchestration.Tests/Specs/Gh300_DeclineTheFinalUnit.cs target-typed ❌ invisible
tests/GenWave.Orchestration.Tests/Specs/Story197_SpeechBoundaryDeferral.cs target-typed ❌ invisible
tests/GenWave.Orchestration.Tests/Specs/Story198_BoundaryAwareSelection.cs target-typed ❌ invisible
tests/GenWave.Orchestration.Tests/Specs/Story303_StraddleHandoff.cs target-typed ❌ invisible
tests/GenWave.Orchestration.Tests/Specs/Story320_BoundaryRespectsBacklog.cs target-typed ❌ invisible
tests/GenWave.Orchestration.Tests/Specs/Story388_AdCadenceAndPipeline.cs target-typed ❌ invisible

So the pin is fail-open: an 8th return new(…) helper sails straight through a green gate. And
the 7 invisible sites are precisely the expensive ones — each had to be hand-edited when
BreakDelivery became a required constructor parameter at PLAN T536.

SPEC F193.4 now states the textual claim honestly and names this issue as the path back to the
semantic one.

🛠️ Phase 1 — migrate the 7 helpers onto OrchestratorBuilder

This is the part that makes the property true. Each helper hand-wires ~14 constructor
arguments; OrchestratorBuilder already exposes 34 With* seams covering every one of them
(WithIdentity, WithScope, WithCadence, WithRotation, WithMusicSelectionPolicy,
WithPersonaAccessor, WithLogger, WithDeferralQueue, WithTime/WithNow,
WithBoundaryBias/WithLookahead, WithPatterEstimator, WithRenderBudget, WithPersonaStore,
WithEvents, WithPlanObserver, …). So this is a mechanical rewrite, not a builder-extension
project.

Per helper:

  1. Replace the hand-wired return new(…) with a new OrchestratorBuilder().With…().Build() chain.
  2. The one thing to check each time: does the helper assert on a collaborator instance that the
    Build() result does not expose? If so, widen the returned chain record rather than reaching
    back around the builder.
  3. Full solution green — these are behaviour-neutral rewrites of test arrangement, so every
    assertion in the file must still pass untouched. An assertion that has to change is a signal the
    rewrite drifted, not a licence to edit it.

One PR, or one per helper if review surface gets uncomfortable. At the end, OrchestratorBuilder
and AddOrchestration are genuinely the only two constructions in the tree.

🔬 Phase 2 — scan IL instead of text

Must follow phase 1 — run against today's tree it finds 9 and goes red on arrival, which would
just mean a second debt baseline.

Replace ConstructorCallScan's text match with a real call-site scan over the compiled assemblies.
GenWave.Architecture.Tests already owns the machinery, and this is the house idiom for L7/L8:

  • Support/MemberCallSiteScan.cs — matches a named member on a named type through raw
    System.Reflection.Metadata tables, reporting every distinct hit;
  • Support/IlTokenWalker.cs — the IL-walking and attribution mechanics it shares with
    HttpClientMetadataScan;
  • Support/IlOperandTable.cs — operand decoding.

A constructor is just a member named .ctor, so the scan becomes: every call site of
GenWave.Orchestration.Orchestrator..ctor across every production and test assembly, asserted to
be exactly the two expected methods. That form sees target-typed new(…), Activator-free
reflection-lite construction, and calls inside compiler-generated async state machines — the class
of bypass MemberCallSiteScan's own remarks were written for.

Then reword SPEC F193.4 to the semantic claim:

F193.4 Fitness pin: the Orchestrator constructor is called only from OrchestratorBuilder
and AddOrchestration (IL call-site scan over the built assemblies).

🔗 Overlap

Phase 1 is BD-0 in gh-#827: it drops the Orchestrator's own call sites
from 9 to 2 and should be done before that issue's BD-1. Do phase 1 once, bank it for both.

Related: SPEC F193.4 · SPEC F184 (STORY-451, v5.10.0) · PLAN T537 · gh-#401

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions