Repository navigation
docs: totality is not liveness — narrow two overclaims from #3915 (Copilot review) - #3919
Conversation
…ew, #3915) Two accuracy defects in what this change wrote about itself: 1. The doc said "the legs are now composed through DetachedReplyOutcome.Of", but the read leg and DispatchInnerCreate carry their own explicit third arm — only three of the five use the helper. The per-leg table was already right; the introduction was not. Reworded to the true claim: every leg handles all three of Rx's terminations, by one route or the other. 2. `Of`'s summary promised one outcome "on every path" for "any source". False for the FOURTH outcome this very page has a section about: a source that never emits AND never completes reaches no arm at all, and no composition OVER a source can invent a termination it never produces. The guarantee is now stated strictly — if the source terminates, exactly one outcome — with the #2742 starvation shape named, and with the statement that a leg whose source can starve still owes its own Timeout, as InnerCreateVerdictBound and NodeOpForwardTimeout already do. Totality and liveness are different properties; this type gives only the first. The reviewer also found the upsert handler's failure surface to be English-only while its success surface is keyed. That is real, pre-existing and file-wide — `PostFail` has no key parameter and all 16 of its sites pass literals — so keying only the three messages added here would deepen the inconsistency rather than reduce it. Filed as #3917 with the scope written out, including the decision nobody has made about whether the wire field `*Response.Error` is itself localized. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01S25PJZbVeCbkWhCfMub9P7
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved wording issues remain in both reviewed files.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Documentation-only follow-up clarifying termination totality versus liveness, with no runtime behavior changes.
Changes:
- Narrows
DetachedReplyOutcome.Ofguarantees. - Corrects the upsert-leg composition description.
- Documents starvation and timeout boundaries.
File summaries
| File | Summary |
|---|---|
src/MeshWeaver.Mesh.Contract/DetachedReplyOutcome.cs |
Clarifies helper guarantees and limitations; wording about compositions inventing terminations remains unresolved. |
src/MeshWeaver.Documentation/Data/Architecture/WriteVerdictTotality.md |
Corrects upsert-leg documentation and adds liveness caveats; an overbroad claim remains unresolved. |
Review details
Suppressed comments (1)
src/MeshWeaver.Mesh.Contract/DetachedReplyOutcome.cs:69
- This revision qualifies
Of's method summary, but the class summary above still says it turns any source into exactly one outcome, and the<returns>text below still makes the same unconditional claim.Of(Observable.Never<T>())emits nothing, so the file still documents liveness that the implementation does not provide. Please qualify those remaining contracts with “if the source terminates” as well.
/// <c>value → HasValue</c>, <c>fault → Error</c>, <c>completed empty → CompletedEmpty</c> — one
/// emission for each of the three terminations Rx delivers, so a subscriber cannot fail to
/// answer a source that TERMINATES. It adds no deadline: a source that never terminates still
/// reaches nobody, and bounding that is the caller's own job (see the type's remarks).
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| "The FOURTH outcome" above — reaches no arm at all, and no composition over the source can invent | ||
| one. A liveness bound stays the leg's own responsibility and is separately present where the |
There was a problem hiding this comment.
Same finding, same fix — see the thread on DetachedReplyOutcome.cs. The doc's caveat now reads: every operator Of applies reacts to a termination and none manufactures one; Timeout manufactures one and is separately present where the source can starve (InnerCreateVerdictBound, NodeOpForwardTimeout); compose Of around that bound and both properties hold. Committed as e7ecf98a8, landing in the follow-up (this PR was already queued when the review arrived).
| /// — reaches no arm at all, and no composition OVER a source can invent a termination it never | ||
| /// produces. Read the guarantee strictly: <i>if the source terminates, exactly one outcome is |
There was a problem hiding this comment.
Correct — and it is a genuine self-contradiction, not just breadth: the same paragraph names Timeout two sentences later as the thing that supplies liveness.
Fixed, with the reason made explicit rather than the claim merely narrowed: every operator Of applies — Take(1), Select, Catch, DefaultIfEmpty — reacts to a termination; none manufactures one. Timeout manufactures one, which is exactly what makes it the right tool for the fourth outcome. So the guidance is now positive: compose Of around a Timeout and both properties hold — the timeout supplies liveness, Of turns whichever termination arrives into an answer.
Applied at all three sites you named (the type's remarks, the Of summary, and the doc page's caveat).
🚨 This PR was already in the merge queue when your review landed, and a queued branch cannot be updated — the same race that put these fixes in a follow-up rather than in #3915. The fix is committed as e7ecf98a8 and lands in the next PR, which I am opening as a draft so review precedes queueing and this loop stops repeating.
Follow-up to #3915 (merged). Copilot found two accuracy defects in what that change wrote about itself; the fixes were ready but the PR had already been enqueued, and a queued branch cannot be updated. Documentation only — no behaviour change, no
src/logic touched.1. The doc claimed a construction that covers three of five legs
It said "the legs are now composed through
DetachedReplyOutcome.Of". The read leg andDispatchInnerCreatecarry their own explicit third arm instead — only the no-op probe, the NodeType gate and the update leg use the helper. The per-leg table was already accurate; the introduction above it was not.Reworded to the true claim: "Every leg now handles all three of Rx's terminations — the read leg and the inner create by their own explicit third arm, the other three by composing through
DetachedReplyOutcome.Of."2. 🚨 Totality is not liveness, and the summary conflated them
DetachedReplyOutcome.Of's remarks promised one outcome "on every path" for "any source". That is false for the fourth outcome the very same doc page has a section about — a source that never emits and never completes (Observable.Never, a lost response, the starvation shape behind #2742) reaches no arm at all, and no composition over a source can invent a termination it never produces.The guarantee is now stated strictly:
…with the #2742 shape named, and with the explicit statement that a leg whose source can starve still owes its own
Timeout— pointing at the two that already carry one (InnerCreateVerdictBoundon the inner create,NodeOpForwardTimeoutinside the no-op probe) — and that composing through this type neither adds nor excuses that bound. The doc page carries the same caveat beside the table.This is the more important of the two: a helper documented as making silence impossible is precisely the kind of standing claim a later reader stops checking.
Not fixed here, and why
The same review found the upsert handler's failure surface English-only while its success surface is keyed. That is real, pre-existing and file-wide —
PostFailhas no key parameter and all 16 of its call sites pass literals — so keying only the three messages #3915 added would deepen the inconsistency rather than reduce it (German for "the probe produced no answer", English for "the probe faulted", in the same dialog). Filed with its scope written out as #3917, including the decision nobody has made about whether the wire field*Response.Erroris itself localized.Verification
Mesh.Contract,Documentation,Documentation.Testeach0 Error(s)in Release with-warnaserror;DocumentationLinkIntegrityTestgreen.🤖 Generated with Claude Code
https://claude.ai/code/session_01S25PJZbVeCbkWhCfMub9P7