fix(guard): the observable→Task bridge guard enforced a SPELLING — make it detect the thing by rbuergi · Pull Request #2762 · Systemorph/MeshWeaver

rbuergi · 2026-08-30T11:54:32Z

What was wrong

ObservableToTaskBridgeGuard matched the literal text .ToTask( and its own remarks claimed src/ was at zero "with no escape hatch". Measured today, that claim was false as enforced, in two independent ways:

Site	Why the marker missed it
`src/MeshWeaver.Reactive.Assertions/ReactiveWait.cs`	Hand-rolls the bridge from a `TaskCompletionSource` + `Subscribe`. Not one character of `.ToTask(` in it.
`src/MeshWeaver.Mesh.Contract/Services/MeshServiceExtensions.cs`	Declares a method literally named `ToTask` and calls it as `ToTask<bool>(service.DeleteNode(path), ct)`. The marker's leading dot — documented as deliberate ("any `.ToTask(` in this repo is Rx's") — walked straight past a static call to a hand-rolled bridge of exactly the banned name.

The second was also the unsafe form (a bare new TaskCompletionSource<T>()), i.e. a live inline-resumption hazard whose remaining callers are 58 in-mesh MeshWeaver.Reinsurance layout-area sites — hub-reachable code, exactly where an inline resumption wedges a turn.

What this changes

Detection is now structural and name-independent. Rename ToTask to First and it still fires:

A TaskCompletionSource settled from inside a Subscribe callback.
A Task-returning method over an IObservable<> that builds its own completion source.
An IObserver<> implementation that settles a completion source (the same bridge with its callbacks extracted into a class).

Two tiers, deliberately different in strength:

Unsafe form (no RunContinuationsAsynchronously) → ZERO. No register, no allow file, no exemption. That flag is the LiveQueryHandoffDropTest fails post-warmup-fix (e2080c4fa) — new information per #2338's own ask #2377 defect.
Safe form → only the entries in SanctionedBridges, each machine-verified to still exist, still contain a bridge, and still be the safe form — so an entry cannot outlive its subject. Four entries: ReactiveCompletion (the one real implementation), ReactiveWait, MeshServiceExtensions (retained debt, already governed by MeshServiceHasNoTaskShimGuard) and MessageHubGrain (grain Task is contractual, and it must map an activation fault to a successful classified DeliveryFailure, which ObserveCompletion cannot express).

The one real defect found is fixed: MeshServiceExtensions.ToTask now constructs with RunContinuationsAsynchronously. No signature change, no caller impact.

🚨 Where the line is — this bans BRIDGING, never the `Task` type

Stated in the guard's remarks and pinned by its tests, because getting it wrong would make the rule unshippable:

IIoPool is the sanctioned async/IO boundary (measured: it constructs no TaskCompletionSource at all).
Orleans grain signatures are Task by contract.
A TaskCompletionSource used as a plain lifecycle signal is not a bridge — MessageHub.hasStarted is settled from Start()/FailStartup() and is correctly invisible.
A Task-returning method that merely consumes the sanctioned bridge is not hand-rolling one — HubDisposalJoin.JoinDisposalAsync passes, and the negative control asserts it passes.

What I deliberately excluded, and why

.Result — overwhelmingly a domain property here (ToolCall.Result, PatchResult.Result). A marker matching it would flag a hundred innocent reads to catch one bridge, and a rule that cries wolf gets suppressed, which is worse than not having it. Narrowing it to ".Result on a task-returning expression" was considered and rejected: the narrowing needs a call graph to be sound, and a marker that is right most of the time is the same "spelling, not the thing" failure this PR exists to end. .GetAwaiter().GetResult() is unambiguous and is matched.
.Wait(timeout) — matched only with empty parens. The bounded form is legitimate in the two places it appears: IoPool's _gate.Wait(DrainTimeout) (the SemaphoreSlim AGENTS.md sanctions inside IoPool) and HubDisposalJoin's deliberate Task.Wait(TimeSpan). The empty-paren spelling is the unbounded park.
A bare TaskCompletionSource ban — would red on MessageHub.hasStarted and on ReactiveCompletion itself, i.e. on the very fix every failure message tells authors to use.

Deviation from the brief (flagging explicitly)

The brief asked for .GetAwaiter().GetResult() and .Wait( at zero, no allow file. Production roots do not start clean: 9 sites across 5 files (compiler, Orleans test-base disposal, plugin tester, a sample). Holding them at zero today would red on merge. They get a seeded ratchet (test/ProductionBlockingBridgeSites.allow) that may only shrink. This still closes a real gap: BlockingBridgeInTestRatchetGuard scans test/ only, on the stated grounds that src/ is covered "by the reviews that enforce it" — reviews are not a gate, and that remark is corrected in this change.

Proof it can actually fail

Non-vacuity is proved, not asserted. The guard builds a real temp tree with a real production root, drops a hand-rolled bridge into it containing no .ToTask( at all, and drives the same scanner over it — asserting the unsafe fixture is caught and classified unsafe, the safe fixture is caught and classified safe, and a mere consumer is not caught.

Mutation A — restore the bare new TaskCompletionSource<T>() in MeshServiceExtensions:

NoProductionBridgeUsesTheUnsafeForm                [FAIL]
  src/MeshWeaver.Mesh.Contract/Services/MeshServiceExtensions.cs
      line 60: 'ToTask' returns a Task over an IObservable<> and builds its own TaskCompletionSource
      line 69: an IObserver<> implementation that settles a TaskCompletionSource
TheSanctionedBridgeRegisterIsStillAccurate         [FAIL]  (not the safe form)
NoProductionCodeBridgesAnObservableToATask         [PASS]  ← the OLD rule, blind to it

Mutation B — revert detection to the .ToTask( spelling alone:

NoProductionBridgeUsesTheUnsafeForm                [PASS]  ← green on a tree with 4 bridges
EveryProductionBridgeIsASanctionedImplementation   [PASS]  ← the false pass that stood before
TheScannerCatchesAHandRolledBridgeInAProductionRoot [FAIL] ← only the control catches it

Deferred, by coordinator decision

Moving MeshWeaver.Reactive.Assertions to test/ is NOT in this PR. It must land after #2750, which is rewriting test/MeshWeaver.Reactive.Assertions.Test; a rename underneath it turns a clean merge into a path-rename conflict across 343 files. A peer worktree is also editing ReactiveWait.cs right now. The register entry carries that follow-up in its reason text, and goes stale — failing loudly — the moment the move lands.

Framing note: ReactiveWait is exonerated as a design. It introduced no defect. NodeTypeCompileParkTest carried a latent ordering assumption that Rx's bridge was masking by resuming the test inline; ReactiveWait correctly queues that continuation, so the latent race became visible. Removing a bridge surfaces latent ordering bugs rather than creating them — that is now a section in the doc, and it is the strongest argument for the zero-tolerance rule. The pending move is layering, not a defect fix.

Conserved

Doc/Architecture/RemovingObservableToTaskBridges gains the spelling-vs-thing lesson, the where-the-line-is boundary, the new rule table, and the "removing a bridge surfaces latent bugs" section.

No What's New entry — deliberate. This is agent/maintainer-facing tooling with no user-visible behaviour change.

Verification

dotnet build src/MeshWeaver.Mesh.Contract -c Release -warnaserror → 0 Warning(s), 0 Error(s) (fresh assembly, not a no-op)
dotnet build test/MeshWeaver.Documentation.Test -c Release -warnaserror → 0 Warning(s), 0 Error(s)
dotnet test test/MeshWeaver.Documentation.Test → 109/109 passed, including DocumentationLinkIntegrityTest (the MeshWeaver.Documentation project rebuilt in the same run, so the edited embedded markdown was re-embedded first)

🤖 Generated with Claude Code

… thing ObservableToTaskBridgeGuard matched the literal text `.ToTask(` and claimed src/ was at zero "with no escape hatch". Measured 2026-08-30, that claim was FALSE as enforced, in two independent ways: * src/MeshWeaver.Reactive.Assertions/ReactiveWait.cs hand-rolls the bridge from a TaskCompletionSource + Subscribe — no `.ToTask(` anywhere in it. * src/MeshWeaver.Mesh.Contract/Services/MeshServiceExtensions.cs declares a method LITERALLY NAMED ToTask and calls it as `ToTask<bool>(service.DeleteNode(path), ct)`. The marker's leading `.` — documented as deliberate — walked straight past it. It was also the UNSAFE form (bare `new TaskCompletionSource<T>()`), i.e. a live inline-resumption hazard on hub-reachable in-mesh callers. Detection is now STRUCTURAL and name-independent: a TCS settled from inside a Subscribe callback; a Task-returning method over an IObservable<> that builds its own completion source; an IObserver<> implementation that settles one. Two tiers, deliberately different in strength: * UNSAFE form (no RunContinuationsAsynchronously) — ZERO, no register, no allow file. That flag IS the #2377 defect. * SAFE form — only the entries in SanctionedBridges, each machine-verified to still exist, still contain a bridge, and still be the safe form, so an entry cannot outlive its subject. Where the line is, stated in the guard and pinned by its tests: this bans BRIDGING, never the Task type. IIoPool, Orleans grain signatures, a TCS used as a plain lifecycle signal (MessageHub.hasStarted) and a mere CONSUMER of the sanctioned bridge (HubDisposalJoin.JoinDisposalAsync) all pass. Also fixes the one real defect found: MeshServiceExtensions.ToTask now constructs with RunContinuationsAsynchronously. Blocking bridges (.Wait() / .GetAwaiter().GetResult()) in production roots get a seeded ratchet rather than a zero — they did not start clean (9 sites, 5 files), and BlockingBridgeInTestRatchetGuard had never scanned production, on the stated grounds that src/ is covered "by the reviews that enforce it". Reviews are not a gate; that remark is corrected in the same change. Non-vacuity is proved, not asserted: the guard builds a real temp tree with a real production root, drops a hand-rolled bridge into it containing no `.ToTask(` at all, and drives the same scanner over it — asserting the unsafe fixture is caught AND classified unsafe, the safe one caught AND classified safe, and a consumer NOT caught. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR strengthens ObservableToTaskBridgeGuard so it detects observable→Task bridges by structure (e.g., TaskCompletionSource settled from inside Subscribe) rather than by the .ToTask( spelling, and adds a separate ratchet for blocking bridges (.Wait() / .GetAwaiter().GetResult()) in production roots.

Changes:

Reworked ObservableToTaskBridgeGuard to structurally detect hand-rolled observable→Task bridges, distinguish safe vs unsafe forms, and verify a sanctioned-bridge register.
Added a seeded ratchet allow-list for blocking bridges in production roots (test/ProductionBlockingBridgeSites.allow) and corrected the related test-only guard remarks.
Updated documentation (RemovingObservableToTaskBridges.md) to capture the “spelling vs thing” lesson, the boundary conditions, and the ratchet model.

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
test/ProductionBlockingBridgeSites.allow	Seeded allow-list for blocking-bridge sites in production roots with a “may only shrink” budget.
test/MeshWeaver.Documentation.Test/ObservableToTaskBridgeGuard.cs	Implements structural bridge detection + safe/unsafe classification + sanctioned register + production blocking ratchet.
test/MeshWeaver.Documentation.Test/BlockingBridgeInTestRatchetGuard.cs	Updates remarks to reflect production blocking-bridge scanning is now gated elsewhere.
src/MeshWeaver.Mesh.Contract/Services/MeshServiceExtensions.cs	Fixes the previously-unsafe `TaskCompletionSource` construction by using `RunContinuationsAsynchronously`.
src/MeshWeaver.Documentation/Data/Architecture/RemovingObservableToTaskBridges.md	Documents the updated guard approach and rationale (structural detection, boundaries, seeded ratchets).

Suppressed comments (1)

src/MeshWeaver.Mesh.Contract/Services/MeshServiceExtensions.cs:66

ToTask claims to complete with the first emitted value, but the subscription is never disposed after TrySetResult/TrySetException. That can keep the observable pipeline alive (and keep consuming values/errors) after the returned Task is already completed, leaking resources and potentially hiding late faults. Dispose the subscription on OnNext/OnError/cancellation using a SingleAssignmentDisposable so synchronous-on-subscribe sources are handled safely.

        var tcs = new TaskCompletionSource<T>(TaskCreationOptions.RunContinuationsAsynchronously);
        var sub = observable.Subscribe(new SingleObserver<T>(tcs));
        if (ct.CanBeCanceled)
            ct.Register(() => { tcs.TrySetCanceled(); sub.Dispose(); });
        return tcs.Task;

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

github-actions · 2026-08-30T12:01:31Z