Conversation
Review — Spec 0036 (design only), part 1 of 2Read all seven ADRs, Claims I verified mechanically
1. Stale cross-reference in three amended ADRs (cheap fix before merge)
The set ships seven ADRs. Three of the four amendments under-count it. 2. FR-27.3's "only for" is now false, and nothing records thatFR-27.3 ( The outcome is right and was anticipated: C-14 and FR-19 require exactly this, and ADR 0072:182 explains how the two meet. But the rule was never amended, and ADR 0075:66 reinforces the misreading — "The two subscriber brackets sit on the subscriber path and nowhere else" is true of the two subscriber brackets and easy to read as true of suppression generally. An implementer working T5.4 from FR-27.3 alone has a defensible case that the pump bracket is out of spec. Suggest amending FR-27.3 to carve out the pump flow explicitly, cross-referencing C-14/FR-19. 3. Seven
|
Review — Spec 0036, part 2 of 26. FR-22 rule 2 fails startup for applications that work todayC-18 records this honestly: an app with The blast radius is bounded by 7. "Not opting in changes nothing" — the PR body overstates what AC-14 assertsAC-14 is titled precisely — "Not opted in ⇒ adoption behaviour identical to today" — and it goes on to exclude three named tests that must change under D3/FR-20. The PR body's bullet ("Not opting in changes nothing. AC-14 exists specifically to assert that") drops the qualifier. FR-11 spells the distinction out at Small in isolation, but it's the sentence most likely to be lifted verbatim into 8. FR-25 has no clause for consumer-side inertnessFR-25.5 covers 9. ADR 0072 ladder row 8 is untested (already acknowledged)
10. Smaller items
On the four questions you asked reviewers to push on1. Is 2. ADR 0073's package boundary. Right place. NFR-2 is guarded mechanically by AC-22 clause 2 rather than by convention, which is what makes it hold. The ADR is also honest that 3. ADR 0075's suppression rule. I agree with the decision. C-1 (MS DI scopes don't nest) plus ADR 0039 leaves no third option: either each subscriber is its own unit of work or it isn't, and adoption without suppression would silently make it not. The surprise is real, and FR-25.5 plus C-4's "the answer is the outbox" is the right framing. The half I'd worry about is the pump — findings 2 and 8. It's the more surprising suppression and the one with the least documentation weight behind it. 4. The negative ACs. I checked the ones whose failure would be silent, and they assert what they claim. AC-11's third branch (a provider that violates FR-10 by returning an ambient for an Nothing here blocks the design. Findings 1, 2 and 7 are cheap text fixes I'd take before merge; 3, 5 and 6 are judgement calls worth a decision one way or the other; the rest are optional. 🤖 Generated with Claude Code |
iancooper
changed the title
Not an ADR commit. Spec 0036's readability item S3 was answered with a reusable principle, and it belongs where every future change can see it rather than only in the ADR that provoked it — the same treatment round 6's decision 18 gave the package-boundary visibility rule (54df521). The owner's words, deciding whether ADR 0071 should keep a resolution path that only tests reach: "tests that exercise non-production paths lack value, and we should re-write to exercise the production path." Added as a new section to .agent_instructions/testing.md, between "Test Scope and Isolation" and "No InternalsVisibleTo". Those two say WHAT surface a test may drive; this says WHICH PATH it must drive when production code offers more than one, which neither covered. The load-bearing clause is the third: when a design change removes a path only tests were exercising, rewrite the tests, do not keep the path. That is the inversion the principle exists to stop — a code path preserved because deleting it would fail a test, rather than because a user depends on it. Deliberately no code example. The natural exemplar is the six tests/Paramore.Brighter.Extensions.Tests/ files that drive ServiceProviderHandlerFactory.Create with a hand-rolled TestLifetimeScope, and TODAY that is a production path — it only becomes a non-production path when ADR 0071 lands. An instruction file may cite only code that exists and means what the file says it means (decision 18's lesson, 54df521). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
Phase 2 is now half done: S1+S2 (d6502de) and S3 (bd44be1, plus 9fcfa28 for the principle). Two calls remain, S5 and S6. §3's located-items table: the S3 row carries where it actually landed — FOUR ADRs and the derived index, against the one site the row named. §7 gains decision row 6, the owner's answer in full, and the "Phase 2 is therefore four calls" line is re-ticked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
The owner's review item asked why the six scope-configuration rules are not ISpecification<T> families the core validator pulls in, proposing an IAmAValidationProvider discovered by reflection over loaded assemblies. The dig found the pull seam already ships and already crosses an assembly boundary: Paramore.Brighter.ServiceActivator defines four ISpecification<Subscription> rules, its DI package registers them (ServiceCollectionExtensions.cs:201-228) and ValidatePipelines() harvests them with GetServices (BrighterPipelineValidationExtensions.cs:79) into PipelineValidator's consumerSpecs parameter (PipelineValidator.cs:57). So rules travel; what cannot travel is the ENTITY TYPE, because core declares the collection and ISpecification<TData> (Specification.cs:35) admits no variance and has no non-generic base. Every T in the repo today — HandlerPipelineDescription, Publication, Subscription — is a core type for that reason. This ADR's entities carry ServiceLifetime and cannot be. The owner's call: take Alternative 1 — the pull moves up one level, from specifications to validators. ScopeConfigurationValidator is registered ALONGSIDE the core validator rather than decorating it, and both hosted services resolve IEnumerable<IAmAPipelineValidator> and PipelineValidationResult.Combine the results. Chosen on open/closed: a decorator makes every later contributor wrap the last, with an order, an ownership graph and a disposal cascade that a flat list does not have. 0074 (the decision, and everything downstream of it): - Decision, mechanism prose and sequence diagram; Where the pieces live diagram and its reading paragraph - Key Components: the coordinator role becomes rule evaluation only; the Reporting role gains the resolve-and-combine half; new ValidationMapperRegistry role - The evaluation site section retitled and rewritten, with the registration shape and the one-line host change - Contract: Validate() returns its own findings; ScopeConfigurationValidator has no Dispose at all — ValidationMapperRegistry.Dispose() replaces the cascade row - Technology Choices: :367 rewritten to give the real reason (the entity type, not the dependency direction) with the consumerSpecs precedent; new entry on why a second registration rather than a decorator - Implementation Approach: step 5 loses the decorator, new step 5b for the two hosts and the shared registry; step 5a's registry ownership re-plumbed through ValidationMapperRegistry; steps 6 and 7 follow - Where each type is touched: two NEW rows for the two hosted services, which leave the "unchanged" list. Neither host is in Paramore.Brighter, so the "core gains nothing" claim survives intact - Consequences: the both-hosts-untouched Positive becomes open-to-extension; the Negative is now the narrowed escape hatch (an application-registered validator no longer replaces Brighter's wholesale) rather than what the interface resolves to; nine new types becomes ten - Risks: both decorator rows replaced - Alternatives: 1 is now the Decision, so the decorator takes its place as the rejected alternative; 7's "one validator, one result, one throw" corrected 0070 (the other end, same commit per the both-ends rule): - :405's release-note ledger item restated for what actually breaks Deliberate sub-decisions inside the owner's call, both recorded in the ADR: - The escape hatch is NARROWED, not preserved. Registering neither of Brighter's validators when a foreign descriptor is present was available and declined: a seam whose point is that registrations compose should not have a silent branch in which one cancels the others. It is a release-note item. - ValidationMapperRegistry is the tenth new type and exists only because two independently registered validators must not build two MessageMapperRegistry instances — the one thing the decorator got for free. Its Interlocked-guarded shared disposal keeps step 5a's core XML-doc amendment exactly as it was. Verification: - claim inventory before/after (§20.4): ZERO requirement tokens lost, ZERO file:line citations lost - every new citation opened against source; two were wrong when first written and were corrected before landing — RegisterConsumerValidationSpecs is :199 with the four registrations at :201-228 (not :198-215), and BrighterValidationHostedService validates at :76 with StartAsync at :71 (not :75) - blast-radius greps over all seven for decorat/ScopeConfigurationValidator/ IAmAPipelineValidator/PipelineValidator/Combine: 0071 and 0075's hits are all ATTRIBUTE decorators and were verified and left alone; the byte-identical sibling description of 0074 is mechanism-neutral and needed nothing - index regenerated in the same commit — the frontmatter summary changed, and exactly one row moved Branch-2 / branch-3 list for this session: EMPTY. No requirements change: requirements.md:320 hands which component evaluates the rules to design under C-13, and no AC asserts the hosts are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
… call Ticks readability-plan.md §3 (the S5 row) and §7 (a new answered row 7), and records that Phase 2 is now 3 of 4 with only S6 left. The owner's call: take Alternative 1 — the pull moves up one level, from ISpecification<T> to IAmAPipelineValidator. ScopeConfigurationValidator is registered alongside the core validator rather than decorating it, and both hosted services resolve IEnumerable<IAmAPipelineValidator> and Combine. Chosen on open/closed: a decorator makes every later contributor wrap the last. The dig's finding, which reframed the item: the RULES already travel across assemblies — ServiceActivator ships four ISpecification<Subscription> rules that ValidatePipelines() harvests with GetServices into consumerSpecs. What cannot travel is the ENTITY TYPE, because core declares the collection, ISpecification admits no variance and has no non-generic base, and every T in the repo is a core type. 0074:367's rejection was right and argued the wrong thing. The ADR edits landed separately in bbb04d6, under the four-bucket rule. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
…t itself NOT a substantive change. No requirement, criterion, constraint or count moves. Revisions 19 (`c944dcc1a`) and 20 (`350e4cd8a`) both landed their CONTENT during round 5 — FR-9(ii)'s false disjunct is gone and AC-51 exists — but neither bumped the document's own bookkeeping. `requirements.md:7` still said "Revision: 18" and the revision-history table's newest row was 18, so the document contradicted `.requirements-approved`, which has recorded both correctly since they landed. Transcribed from `.requirements-approved`, which is the authority here: - header `:7` 18 -> 20 - two new revision-history rows, 20 and 19, above the existing 18 Revision 20 is recorded as PENDING, not approved, exactly as `.requirements-approved` states, together with the owner's process change that carries it to the end-of-phase true-up rather than a round of its own. Separated from the substantive S6 commit that follows deliberately: round 5 mixed a substantive correction into a mechanical pass and it produced both of round 6's Criticals. Revision 21 cannot sit on a table whose newest row is 18, so this had to go first, but it goes on its own. Found by isolating a fact while preparing S6 — branch-3 sighting, PROMPT.md 20.6. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
…16c, C-21, AC-52 + ADR 0072)
Phase 2 call 4 of 4, bucket S. Owner's call: this is a real design gap, and both
ends move in one commit because changing only one would trip up a reviewer.
THE QUESTION ASKED, AND ITS ANSWER. S6 asked whether Brighter's registrations —
many of them factory functions — still resolve from an ambient scope borrowed
from a non-Brighter parent. THEY DO, and the set already argued why at
`0072:416`. Every registration in both DI packages is Singleton or Transient;
Singletons are built in the ROOT scope whatever scope asked, so borrowing cannot
change what any of Brighter's infrastructure sees, and every factory-function
registration takes the provider as a PARAMETER and captures none. The artefact
types are Transient descriptors in the same IServiceCollection the borrowed
scope's container was built from (C-17). That half needed no change.
THE GAP IT EXPOSED IS A DIFFERENT ONE. `grep -ni "transaction"` over all seven
ADRs returns FIVE sites and EVERY ONE IS NEGATIVE — C-4 and OOS-10, both about
`Publish` subscribers. The POSITIVE case a `Send` handler depends on was stated
nowhere, and neither was the mechanism that defeats it today:
- a relational transaction provider wraps a `DbContext` taken by constructor
injection (`MsSqlEntityFrameworkCoreTransactionProvider.cs:18`, same shape in
the MySql, Sqlite, PostgreSql and MongoDb providers), and is registered
Transient by default over a Scoped `DbContext` — so WHICH transaction it
hands out is decided entirely by the scope that resolved the pair;
- every non-Singleton handler resolution is made from a scope Brighter created
(`ServiceProviderHandlerFactory.cs:67-68`, `:85-86`);
- so under AlwaysNew a handler's `DepositPost` writes on a different connection
inside a different transaction from the caller's own writes, and NOTHING
REPORTS IT. The deposit succeeds; only atomicity is lost.
The owner notes that reports of bugs around the flow of a transaction to an
Outbox may have this as their cause. NOT VERIFIED — the mechanism above is
verified against source; no specific report was checked.
REQUIREMENTS (revision 21, 27/10/52/123, PENDING):
- FR-16(c) — a Scoped dependency the CALLER already resolved is the same
instance a handler reached by `Send` resolves. Clause (b) extended across the
call boundary, not a second mechanism; stated separately because it is the
clause applications depend on and the only one whose failure is silent.
- C-21 — the mechanism, the silence, and three bounds on the fix: the outbox
mediator is a Singleton and never borrows (`:484`); the three container-side
transaction-provider resolutions (`:431`, `:487`, `:648`) are TYPE DISCOVERY
only; and `DepositPost` without an explicit provider still passes `null`
(`CommandProcessor.cs:795`), so adoption does not make an outbox write
transactional on its own.
- AC-52 — measures ATOMICITY through a ROLLBACK, not a commit, plus an
AlwaysNew negative control asserting today's behaviour AND ITS SILENCE, so an
implementation cannot satisfy the criterion by reporting the case instead of
fixing it.
ADR 0072: the registration answer stated as an answer rather than left to be
inferred from the container-provenance paragraph; the transaction consequence as
a Positive consequence bullet — the first place in the set to state it positively
— and the boundary that adoption changes the provider instance THE HANDLER HOLDS
and nothing the mediator does.
CLAIM INVENTORY (PROMPT.md 20.4), 0072 before/after: ZERO requirement tokens,
ZERO sibling references and ZERO `file:line` citations removed. Five citations
added, all opened and verified.
COUNTER-CASES VERIFIED AND LEFT ALONE. `0072:481`, `0074:433`, `0075:340` and
`0075:355` all state the limitation scoped to `Publish` subscribers and stay true
under FR-8. `0073:30`, `0073:329` and `0076:423` describe FR-16's case as a
handler and the controller sharing a `DbContext` — now precisely clause (c) — but
none mentions a transaction, so the new "first place in the set" claim holds and
adding a clause letter would be a readability edit riding a substantive commit.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
Tracker only; no ADR and no requirement moves. `fa937a739` is the edit. §3 row: S6 answered and landed. ⚠ The row's "one line in the set — `0072:480`" was WRONG: `:410`, `:416` and `:420` all bear on the item and `:416` largely answers it. §3 "S6 has almost no footing" subsection: both of its premises are falsified and are KEPT, with the correction beneath them, because the lesson is worth more than the tidy. `0072:416` answers the design question and was missed for an exact reason — it uses NONE of the item's words, being a paragraph about container provenance. Grep for the ANSWER's vocabulary, not the QUESTION's. §7 row 5: the plan predicted "a §18.8 true-up row rather than an ADR fix". The answer was BOTH, in one commit, and the gap was in territory the item never named — the transaction consequence, which the set stated negatively five times and positively not once. §7 closing: all four Phase 2 calls are done; Phase 2 is CLOSED and Phase 3 is next, worst-first 0072, 0071, 0070. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
Phase 3 branch-3 item, found by the pre-rewrite claim inventory and landed in
its own commit ahead of the 0072 rewrite (the S6 precedent, `6883f589f` ahead
of `fa937a739`). A stale count must not ride a diff in which every line moved.
Three unscoped sites said 0075 owns two brackets:
- `0072:33` (Scope) "the flag, both brackets and the reasoning"
- `0072:336` (touched table) "the flag, both brackets, the reasoning"
- `0074:433` (Risks row 5) "suppression and its two brackets"
0075 owns THREE — the two publish brackets and the pump-flow bracket of its
step 4a, which S1+S2 (`d6502deb5`) kept. The authorities are `0075:259` and
`0075:338` ("Three brackets, five places to get wrong"), and 0072 already said
so twice in its own body: `:110` "all three of its brackets" and `:112` "ADR
0075's first two brackets" plus "that ADR's third bracket in `Performer.Run()`".
Both ends in one commit under the §19.8 rule: fixing 0072 alone would have left
0074 asserting the old count, which is the cross-ADR split that produced round
6's two Criticals.
Counter-cases, verified and left alone — every remaining "two/both brackets" in
the set is correctly SCOPED to 0075's two PUBLISH brackets, which is the `set #5`
discriminator (a count is right or wrong against the scope stated beside it):
`0072:112` ("first two"), and `0075:72`, `:141`, `:306`, `:317`, `:348`, `:373`,
`:396`, all of which are about the resolution-time/execution-time pair.
No frontmatter moved, so `docs/adr/index.md` is unaffected.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
Phase 3's first session. One ADR, one batch. No design decision moves; the
bracket-count correction that the pre-rewrite inventory surfaced landed ahead of
this in `dd83163e1`, so nothing substantive rides this diff.
WHAT CHANGED STRUCTURALLY
- `## Context` is now four plain sentences naming no interface (house style D1's
row). The old opener led with `CreatePipelineScope()` and `IAmAScope`.
- NEW `### Scope` heading between `## Context` and `### Where this ADR sits`,
per D1. The 900-word narrative paragraph and its four-naming-questions
successor become three lists: **In scope** (one bullet per FR, each naming the
mechanism that discharges it), **Contributed to here, discharged elsewhere**
(FR-13's disposal-failure clause, FR-27.3) and **Out of scope** (one bullet
per boundary, each naming the owning ADR).
- `### Key Components` takes D2's column rename: Role / Type / **Responsibilities**
/ **Responsibility classifier** / **Collaborators**, replacing Role / Type /
Stereotype / Responsibility. Collaborators is a new column and is populated for
all eight roles. The References entry for Wirfs-Brock now says "role and
responsibility vocabulary" rather than "role/stereotype vocabulary", which is
the same correction D2 makes.
- TWO NEW DIAGRAMS, both rendered (all three blocks render clean):
* a `sequenceDiagram` at the head of *The mechanism, end to end* — builder →
factory → policy → provider → probe → handle. This is the review's
"ScopeAffinityPolicy's role is unclear" and "AmbientScopeProbe is hard to
parse" items: both objections are about a thread of calls that no artefact
showed.
* a `classDiagram` at the head of *Key Components* — what implements what.
A first draft carried the collaboration edges too and rendered as an
unreadable 1800px sprawl that duplicated the new Collaborators column; it was
cut back to the type hierarchy alone. Sequence = who calls whom, class = what
implements what, table = who is responsible for what.
- The decision-ladder table now LEADS its section; the "every path that is not
borrowed" paragraph reads off it rather than preceding it. Five invariants
follow, each with one bold lead sentence.
- Long run-on passages became lists where the content was already a list: the
`GetAmbient` error column (three requirements), the never-null invariant
(three facts making a null argument reachable), the faulted-entry removal
(three constraints), the three ways out in *Technology Choices*, the probe's
two resolutions, the two facts settling registration, and the "Unchanged"
paragraph (nine items).
- The pair-matching removal moved from a table cell into a fenced `csharp` block.
- `_scopeProvider is resolved once` moved from inside the residue discussion to
immediately after the pseudo-code it belongs to, and the FR-24.3 registration
model got its own bolded block instead of trailing a paragraph about disposal
windows.
READABILITY ITEMS CLOSED (Phase 0 filed five R items against this ADR)
- `0072:104` "This is a re-reading of FR-27.1's own words" — argument restated as
an assertion. The FACT is unchanged and still FUTURE-TENSE, as §19.9 row 4
requires: an amendment to FR-27.1 and AC-46 is owed and the true-up carries it.
- `0072:112` "Two kinds of flow reach that line" — now leads with the assertion
("Neither a `Publish` subscriber nor a consumer pipeline ever adopts") and the
flows follow. The AC-20/FR-19/C-14 correction stays future-tense (§19.9 row 5).
- `0072:131/:167/:242/:247/:249/:332/:336` `ScopeAffinityPolicy`'s role — the
section now opens with what the object is FOR in one sentence before the
requirement argument, and the sequence diagram shows where it sits.
- `0072:238` "ignored, not rejected" — rewritten to name its subject first (an
ambient a provider OFFERED, not a handle a caller passed).
- `0072:410` `AmbientScopeProbe` — broken into what it resolves, what fails, and
which providers can reach it.
⚠ THE §20.6 TRAP HELD: `0071:237` and `0072:238` ARE NOT HARMONISED. They decide
different questions about different objects — 0071 rejects an unrecognised
`IAmALifetime.PipelineScope` HANDLE, 0072 declines an unrecognised ambient SCOPE
— and the rewrite now says so in terms, so a later reviewer cannot re-file them
as twins.
CLAIM INVENTORY (§20.4) — before at `dd83163e1`, after here. Six greps.
Requirement tokens, `file:line` citations, sibling-ADR references and backticked
identifiers were extracted as sorted sets and diffed. FOUR things disappeared and
all four are deliberate:
1. `FR-16a` -> `FR-16(a)` | the unparenthesised spellings. `requirements.md`
2. `FR-16b` -> `FR-16(b)` | labels FR-16's clauses `(a)`, `(b)`, `(c)`, and
3. `FR-16c` -> `FR-16(c)` | 0072 used both forms. One term per concept.
All three parenthesised forms were already in the document, so no set member
is new; the counts move 5/1/1 + 4/2/2 -> 7/1/3.
4. The inline `((ICollection<...>)_cache).Remove(...)` run — same text, now in a
fenced `csharp` block, so it is no longer a single backtick run.
FIFTH, and it is a COUNT rather than a token: "FR-19's **two** diagnostics are
bounded to hosts where an ambient source is registered" is now "the diagnostics
FR-19 names are bounded to…". The bounding argument is what row 3's silence
rests on and is unchanged; the numeral is scheduled to change, because §19.9
row 5(c) records that FR-19's own "exactly two" is falsified by the pump-flow
bracket. ⚠ ADD `0072`'s row-3 note TO §19.9 ROW 5's RE-TENSE LIST.
Two tokens were RESTORED after the first inventory diff caught them missing —
`ConsumersOptions` (ADR 0076 supplies the affinity property's inheritance onto
it) and `AC-14` (0071 records the FR-27.1 amendment on the same footing as its
AC-14 designation change). AC-14 is now also in the References requirement list,
where it had been missing while the body cited it.
Nothing else moved: 0 citations lost, 0 sibling references lost, 0 other
backticked identifiers lost.
CHECKS RUN
- All three mermaid blocks render (`mmdc` exit 0, non-empty SVG). The two new
ones were also rendered to PNG and looked at; the classDiagram was rewritten
after the first look.
- `grep -c '<\|>\|&'` = 0.
- Whole-document re-read start to finish, per `documentation.md`'s check. It
found the FR-24.3 bullet reading as fully discharged under an "In scope"
heading (now says explicitly that the rule is split with 0074), and the
decline-count phrase below.
- The set-level shapes are UNTOUCHED and verified: the *Where this ADR sits*
rows are byte-identical to 0070's modulo bolding, and the unifying sentence is
present verbatim. Those move only in X1's own commit.
- Every cited source line was opened and verified against source before the
rewrite carried it forward — all 55 exact, including the six S6 added.
FRONTMATTER: the `summary` was one 70-word sentence; it is now four. That stales
the derived index, so `docs/adr/index.md` is REGENERATED IN THIS COMMIT — one
row, 110, and `_99 ADRs indexed._` is unchanged.
BRANCH-2 / BRANCH-3 LIST (§20.5) — reported even when empty; it is not.
- Branch 2: NONE. No fact needed its longer form kept.
- Branch 3, RESOLVED: "both brackets" at `:33` and `:336` (plus `0074:433`).
Owner call taken, landed in its own commit `dd83163e1` ahead of this one.
- Branch 3, OPEN: the `ScopedArtefactCache` decline said it "puts the decline
where the other **three** decline points already are". This ADR counts THREE
decline points in total — ladder rows 5, 8 and 9 — and the cache-supply decline
IS row 9, so "the other three" cannot be right; "the other two" would be. The
rewrite neutralises the numeral ("where the ladder's other declines already
are") so the ADR asserts nothing false, and the row is recorded in §20.6 for
the owner rather than being silently renumbered.
METRICS: 15,895 -> 16,590 words (+4.4%, "prefer a slightly longer document"),
bold runs 317 -> 210 (-34%), ⚠ markers 5 -> 0, mermaid diagrams 1 -> 3.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
Tracker only; no ADR and no requirement moves. `f30358c5e` is the rewrite and `dd83163e1` the branch-3 correction that preceded it. PROMPT.md - header and ▶ RESUME: HEAD, commit count, and "Phase 3 is 1 of 7 done, next is 0071". - ▶▶ work order rewritten for 0071, carrying forward the three things that worked on 0072: verify every cited source line before writing, act on the inventory diff, and LOOK at every rendered diagram. - §20.3 Phase-3 row: in progress, 0072 landed, what it cost. - §20.6: TWO new rows. Row 5 — the bracket count, resolved by owner call in its own commit ahead of the rewrite. Row 6 — the decline count, OPEN, neutralised in the rewrite rather than renumbered, ruling owed. - §20.6 trap paragraph: records that the trap HELD, and how 0072 now defends against a reviewer re-filing `0071:237` and `0072:238` as twins. - Two new lessons: the inventory diff caught two losses a careful read had missed, and a COUNT can disappear without a TOKEN disappearing — which only the numerals grep sees. - New work-order item 6: §19.9 row 5 owes a re-tense entry for 0072's ladder row-3 note, which dropped FR-19's falsified "two". readability-plan.md - §4 phase table: Phase 3 is 1 of 7. - §7 closing: all four Phase 2 calls done and the phase closed (the old text still said "ONE remains — S6"), plus what Phase 3's first session landed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
Branch-3 row 6 (PROMPT.md §20.6), open since Phase 3's first session and now ruled by the owner. Its own commit, ahead of 0071's rewrite, on the four-bucket rule and the dd83163 precedent. THE DEFECT The `ScopedArtefactCache` section closed with "puts the decline where the other **three** decline points already are". The ADR counts **three** decline points in TOTAL, not three besides this one: - `:175` "A declined ambient is never disposed, at rows 5, 8 and 9 alike" - `:676` "which the protocol states at each of its three decline points" and the cache-supply decline IS row 9 — `:571` says so in terms ("A failed probe declines at ladder row 9"), and a `null` answer to the `ScopedArtefactCache` ask is exactly a failed probe. So "the other three" counted itself. "The other two" is rows 5 and 8. WHAT LANDED f30358c neutralised the numeral rather than renumbering it — "where the ladder's other declines already are" — which asserted nothing false but left the arithmetic unstated. The owner's ruling restores the numeral and names the row the decline lands on, so the sentence now says both which row this decline is and how many others there are: "...puts the decline at ladder row 9, where the other two decline points already are (C-7)." COUNTER-CASES — verified and left alone Both other count statements are correct as written and are NOT touched: `:175`'s "rows 5, 8 and 9 alike" and `:676`'s "each of its three decline points" are counts of the WHOLE set, where this one is a count of the REST. Same lesson as `set #5` (a85b18c): a count is right or wrong against the scope stated beside it, so grep the numeral and read each hit for its qualifier. One ADR, one line. No sibling states this count — `grep "decline point"` over all seven returns only 0072.
Phase 3's second session. One ADR, one batch. No design decision moves; the
branch-3 correction the owner ruled on landed ahead of this in `a12e93f9a`, so
nothing substantive rides this diff.
WHAT CHANGED STRUCTURALLY
- `## Context` is now four plain sentences naming no interface (house style D1).
The old opener led with `IAmAScope`.
- NEW `### Scope` heading between `## Context` and `### Where this ADR sits`,
per D1. The 490-word narrative paragraph and its 102-word successor become
three lists: **In scope** (one bullet per requirement, each naming the
mechanism), **Contributed to here, discharged elsewhere** (FR-13's
borrowed-scope carve-out, FR-27.1's Transient rule) and **Out of scope** (one
bullet per boundary, each naming the owning ADR).
- `### Key Components` takes D2's column rename: Role / Type / **Responsibilities**
/ **Responsibility classifier** / **Collaborators**. Collaborators is new and
is populated for all five roles. The References entry for Wirfs-Brock now says
"role and responsibility vocabulary", the same correction D2 makes.
- TWO NEW DIAGRAMS, both rendered and looked at (all five blocks render clean):
* a `flowchart` under a new `#### What a Transient handler pipeline gets` —
three subgraphs, one per configured lifetime, showing that under `Transient`
the handle scopes the SET of per-resolution inner scopes rather than giving
one artefact per pipeline. This is the review's "it might help to have a
mermaid diagram of how transient works" item.
* a `classDiagram` at the head of *Key Components*. A first render left
`IAmAHandlerFactorySync`/`Async` as bare unlabelled boxes because they were
referenced in edges without being declared; they now carry their own
`Create`/`Release` members, which is what makes alternative 6's
"one declaration on the base, not two on the twins" visible.
- The 250-word contract cell and the 312-word ordering paragraph become a cell
plus prose, and prose plus bullets. NO paragraph in the document now exceeds
200 words (there were 10); paragraphs over 90 words: 54 -> 39.
- Long run-ons became lists where the content was already a list: the six
`PipelineBuilder` threading methods, `Release`'s two reasons for losing its
body, step 1's implementation counts, step 2's three grounds for never
throwing, step 2's three criteria, step 6's 26-fact split, the two routes that
reach the handle, AC-24's two owed amendments, and the nine-item "Unchanged"
paragraph.
READABILITY ITEMS CLOSED (Phase 0 filed seven R items and one M item)
- `0071:30` "FR-13 divides by family rather than by clause" — restated in the
reviewer's own words: 0070 records the decisions that make FR-13 true for
mapper and transform pipelines, this ADR records the ones for handler
pipelines. ⚠ 0070 CARRIES THE SAME PHRASE at `:32` and `:34`; the Phase 0 row
names those two sites and they are absorbed into 0070's OWN rewrite next
session, not edited here.
- `0071:104` "Transient is not only Scoped's poor relation" — the forces bullet
is now three sentences pointing at the new diagram section.
- `0071:108` NFR-4 / the `ConcurrentDictionary` — promoted out of a 224-word
forces bullet into its own `#### What replaces the dictionary's atomicity`,
because the review is right that convention-over-restriction is a decision
this ADR makes rather than a note. Confinement / immutability /
single-issue disposal are now three named bullets.
- `0071:209` "The member's shape is ADR 0070's" — split into what transfers
(shape, create-failure) and what does not (the null rule), which is the whole
point of the paragraph and was buried mid-sentence.
- `0071:234` contract row — see the M item below.
- `0071:237` "ignored, not rejected" (flagged THREE times) — the passage now
leads with its own answer, and the 0072 contrast names its subject first.
- `0071:295` AC-33/AC-51/AC-7 — the review's worked example of the pattern to
fix. Now: state the rule in prose, then a three-bullet list of the criteria,
one per bullet, AC-7 included as the one that is NOT the criterion.
THE M ITEM, AND THE OWNER'S RULING
Phase 0 filed `0071:234`'s FR-27.1/AC-46 amendment as **M** — "tracking for an
issue, not a decision… could be tracked in PROMPT.md instead of inlined here".
Removing it outright would have falsified 0072 and orphaned §19.9 row 4's
re-tense anchors, so it was raised. The owner ruled SPLIT:
- the contract CELL keeps only the design facts — a `Transient` handler
pipeline's handle is non-null, and AC-46's instrument is the ambient
recorder, not this property's nullness;
- the amendment record moves OUT of the table into one paragraph beneath it.
⚠ THAT REACHED A SECOND ADR AND BOTH ENDS ARE IN THIS COMMIT. `0072:161` said
ADR 0071 "records the same amendment ON THE CONTRACT ROW"; it now says "beneath
the contract table". The amendment stays deliberately FUTURE-TENSE at both ends,
per §19.9 row 4.
⚠ THE §20.6 TRAP HELD AGAIN, FROM THE OTHER END. `0071:237` still answers
REJECT and was NOT reached for 0072's wording. The new passage names the objects
apart — 0072 declines an ambient a PROVIDER OFFERED, 0071 rejects a handle a
CALLER PASSED — and states the rule covering both: decline where a fallback
exists, throw where none does.
CLAIM INVENTORY (§20.4) — before at `bd4ee4b7a`, after here. Six greps.
Requirement tokens, `file:line` citations, bare `:NNN` citations, sibling-ADR
references and backticked identifiers were extracted as sorted sets and diffed.
ZERO set members disappeared from any of the five. Counts move (AC-7 7->5,
FR-13 11->16) but no token, citation or identifier was lost.
⚠ THE SIXTH GREP — NUMERALS AS COUNTS — CAUGHT THE ONLY REAL LOSS, AND IT WAS
A CROSS-ADR OWNERSHIP FACT. The old Scope said FR-7's ownership "sits here
rather than with ADR 0070, which touches no handler pipeline and names FR-7 as
*served*". The rewrite dropped it, and `grep served` over the new file returned
nine hits that were all "preserved"/"observed". `0070:30` says in terms "FR-7
is served here, not discharged here… its owning ADR is ADR 0071", and `0070:32`
makes it a set-level rule — so 0071 alone would no longer have explained why two
ADRs both name FR-7. RESTORED to the FR-7 bullet.
Three further count phrases changed shape deliberately, none losing a fact:
1. "the sync and async twins of each of three" -> a three-row list of pairs,
which shows the same arithmetic structurally.
2. "Twenty-two test doubles… 22 test files in all" -> "Twenty-two test files".
Both numerals were 22 and both true (one double per file); one noun kept.
3. "one seam to build on instead of two" -> stated in `## Context` and again in
the divergence section and the *Positive* bullet.
Two ADDITIONS to the References requirement list, both closing real gaps the
inventory exposed: **FR-5**, cited seven times in the body and absent from
References, and **FR-12**, newly cited by the Scope's carve-out bullet.
BOLD — the honest number
Total bold runs 220 -> 225, which is flat. The distribution is what moved:
runs that OPEN a list item 25 -> 59, and INLINE emphasis inside prose 195 -> 166.
The review's objection was to inline bold "used to draw out the key parts of the
text"; that is the number that fell. Prose paragraphs carrying 3+ inline bold
runs: 4, and each is a lead sentence plus two working marks.
CHECKS RUN
- All five mermaid blocks render (`mmdc` exit 0, non-empty SVG). Both new blocks
were rendered to PNG at 1600px and looked at; the classDiagram was corrected
after the first look.
- `grep -c '<\|>\|&'` = 0.
- Whole-document re-read start to finish, per `documentation.md`'s check. It
found "The same three things happen at the same three moments: the dictionary
is gone…" — a colon promising the same things and then listing the CHANGED
ones. Split into two sentences.
- Every `file:line` citation was opened and verified against source BEFORE the
rewrite carried it forward — all 31 file-qualified and all 41 bare `:NNN`
exact, on a file S3 edited since Phase 0. The derived counts were re-derived
rather than trusted: 26 facts (25 `[Fact]` + 1 `[Theory]`), 22 test files
(16 factory doubles + 6 lifetime doubles), 5 `src/` factory implementations,
32 `src/` declarations taking an `IAmALifetime`, 4 existing `Debug` members,
3 existing constructors. All correct as written.
- The set-level shapes are UNTOUCHED and verified: the *Where this ADR sits*
table is byte-identical to 0070's modulo bolding, the unifying sentence is
verbatim, and the `## References` sibling list diffs clean. Those move only in
X1's own commit, which is still owed.
FRONTMATTER: the `summary` was one 190-word sentence; it is now six, and gains
FR-7, which the body has always discharged. That stales the derived index, so
`docs/adr/index.md` is REGENERATED IN THIS COMMIT — one row, 109, and
`_99 ADRs indexed._` is unchanged.
Ticks `readability-plan.md` for `3537c68cd`. PROMPT.md is gitignored and is updated in the working tree; MEMORY.md likewise. - §4's phase table: Phase 3 is 2 of 7, 5 to go. - §3's located-items table: all seven of 0071's R rows and its one M row are ticked with the commit that closed them. ⚠ The "FR-13 divides by family" row keeps `0070:32` and `0070:34` OPEN — an R item is absorbed into its own ADR's rewrite, so those two belong to 0070's session. - §7: the session-2 record, and both branch-3 rows now RULED (§20.6 rows 6 and 7), so no ruling is owed going into 0070. The lesson worth carrying: four of the six inventory greps came back completely clean and the numerals-as-counts grep still found a real cross-ADR loss — 0071 had dropped the reciprocal of `0070:30`'s "FR-7 is served here, not discharged here". A clean run on the other five is not evidence that nothing was lost.
Phase 3's third session. One ADR, one batch. No design decision moves and no
substantive change rides this diff — S4, the one bucket-S item filed against
0070, resolved to NO DESIGN CHANGE (see below), so unlike the 0072 and 0071
sessions there is no branch-3 commit ahead of this one.
WHAT CHANGED STRUCTURALLY
- `## Context` is now four plain sentences naming no interface (house style D1).
- NEW `### Scope` heading between `## Context` and `### Where this ADR sits`,
per D1. The two narrative paragraphs become four lists: a lead naming the
three things that ARE the core, **In scope** (one bullet per requirement, each
naming the mechanism and its guard), **Contributed to here, discharged
elsewhere** (FR-7, FR-27.1, FR-13's borrowed carve-out, FR-13's handler
instance) and **Out of scope** (one bullet per boundary, each naming the
owning ADR).
- `### Key Components` takes D2's column rename: Role / Type / **Responsibilities**
/ **Responsibility classifier** / **Collaborators**. Collaborators is new and
is populated for all five roles. The References entry for Wirfs-Brock now says
"role and responsibility vocabulary", the same correction D2 makes.
- TWO NEW DIAGRAMS, both rendered and looked at (all four blocks render clean):
* a `classDiagram` at the head of *Key Components* — the handle hierarchy,
who holds it and who owns the container scope behind it. Every type named in
an edge is declared, which is the defect the 0071 session found by looking
at the PNG rather than at the exit code.
* a `sequenceDiagram` in step 4b, showing the surfacing disposal path against
the terminal-teardown swallow it does not inherit. This is the review's
"'surfaces its inner disposal failure' is particularly hard to reason about
and perhaps needs a sequence diagram" item.
- NO paragraph in the document now exceeds 200 words (there were 10; the worst
was 417). Long run-ons became lists where the content was already a list: the
three-phase mechanism, the eight log messages, the two failed-build branches,
the four blocking-release sites, the AC-24 two-count arithmetic, the nine
undocumented breaks, the mixed-lifetime cases and the nine-item "Unchanged"
paragraph.
READABILITY ITEMS CLOSED (Phase 0 filed seven R items and one M item)
- `0070:30`/`:34` Scope leads with Defect 1b, `IAmAScope` and FR-13 as the core,
which is the review's ask in terms.
- `0070:32`/`:34` "FR-13 divides by family rather than by clause" — restated in
the reviewer's own words, matching what `0071:30` already says: 0070 records
the decisions that make FR-13 true for mapper and transform pipelines, 0071
those for handler pipelines. ⚠ THIS IS THE ITEM 0071's SESSION DELIBERATELY
LEFT FOR THIS ONE; both of its sites are now closed and the R row is fully
discharged across the two ADRs.
- `0070:91` NFR-8 / the `IAmALifetime` distinction — promoted out of a forces
note into its own bolded question in *Technology Choices*, because why the
transform family cannot reuse `IAmALifetime` is a decision this ADR makes.
The forces bullet keeps the NFR and points at it.
- `0070:285` "What this cache does and does not give" — moved under `## Decision`
as `### What one scope per pipeline gives, and what it does not`, because the
review is right that the why belongs with the decision. No sibling cites the
old heading; checked.
- `0070:335` and `0070:407` argument-as-record — "Raising those seven was
rejected" and "A new criterion enumerating thirteen `Then`s was rejected" now
state the decision, and the rejections move to alternatives 11 and 12.
- `0070:481` more argument into `## Alternatives Considered` — FOUR alternatives
added, 10 to 13, all APPENDED so that 0072's citation of "ADR 0070's
Alternative 2" still resolves.
THE M ITEM
`0070:32`'s "How the set treats non-functional requirements and constraints,
stated once here because this is the first ADR" is agent scaffolding, and the
review asks for it in PROMPT.md. It is REMOVED, and it needed no owner call
because nothing depends on it: `grep` over the other six found no citation of
it, and the concrete facts it carried are kept where they belong — C-19 and
C-8's disposal half are now Scope bullets, and the serve-and-name-the-owner
practice survives where 0070 and 0076 each apply it in terms.
S4 — THE BUCKET-S ITEM, AND WHY IT MOVES NOTHING
Phase 0 filed "we should promote the exception to public" as substantive.
⚠ THE PLAN'S OWN JUSTIFICATION MISREAD ITS QUOTE: `readability-plan.md` §7
answers it with "the exception is what a caller catches", reading *exception*
as an exception TYPE. It is not — "the single internal exception" means the one
class in the DI package that is not public. The owner ruled NO DESIGN CHANGE, on
these facts:
- nothing outside the DI package consumes `ServiceProviderLifetimeScope` — its
callers are the five factories in its own assembly, no test names it, and
this solution contains no `InternalsVisibleTo` anywhere;
- `design_principles.md` makes `internal` correct on exactly that test, and
prefers a public type with an internal constructor, which is what
`ServiceProviderPipelineScope` already is;
- `0074:187` already ruled the identical CS0051 question the same way.
So the decision stands and only the PROSE changes: *Technology Choices* keeps
the decision and the count `0072:336` cites it for, and the argument becomes
alternative 10.
CLAIM INVENTORY (§20.4) — before at `345e8fe0e`, after here. Six greps.
Requirement tokens, `file:line` citations, bare `:NNN` citations, sibling-ADR
references and backticked identifiers were extracted as sorted sets and diffed.
Bare citations and sibling references lost NOTHING. Five requirement tokens
disappeared and ALL FIVE were confined to the deleted M paragraph — AC-22,
FR-15, FR-17, NFR-2, NFR-9 — each an example of how the SET distributes
requirements rather than a claim 0070 makes. Each is carried by its owner:
FR-15 by 0073 and 0076, FR-17 by 0073/0074/0076, NFR-2 by 0073 (9 mentions),
NFR-9 by 0074, AC-22 across four siblings. Verified, not assumed.
⚠ THE SIXTH GREP — NUMERALS AS COUNTS — CAUGHT THE ONLY OTHER REAL LOSS, FOR
THE THIRD SESSION RUNNING. The S4 trim dropped "the TWO type tests that do name
the class — step 6 and ADR 0071 step 4 — are both inside the DI package", a
count plus a cross-ADR anchor that no other grep reported. RESTORED to
alternative 10, with 0072's `IAmAServiceProviderScope` section pointer.
ONE CITATION WAS STALE AND IS CORRECTED. AC-24's verifier is at
`requirements.md:714`, not `:700`; `:700` is one of AC-24's own `And` clauses.
Every other citation was verified against source BEFORE the rewrite carried it
forward — all 49 file-qualified and all 85 bare `:NNN` exact.
THREE REFERENCE GAPS CLOSED. FR-12, FR-23 and OOS-5 are each cited in the body
and were absent from the `## References` requirement list. Added.
DERIVED COUNTS RE-DERIVED RATHER THAN TRUSTED, all correct as written: 12
classes in `src/` (a one-line class regex misses `SimpleMessageTransformerFactory`,
whose base list is on the following line), 70 test doubles = 64 factory doubles
in 37 files plus 6 registry doubles in 3 files, one with no factory double, so
38 test files; 17 of 18 DI-package classes public.
BOLD — the honest number
Total bold runs 287 -> 230. Runs that OPEN a list item 54 -> 78; INLINE emphasis
inside prose 233 -> 152. The review's objection was to inline bold "used to draw
out the key parts of the text", and that is the number that fell, by 35%.
CHECKS RUN
- All four mermaid blocks render (`mmdc` exit 0, non-empty SVG). Both new blocks
were rendered to PNG at 1600px and looked at.
- `grep -c '<\|>\|&'` = 0.
- Whole-document re-read start to finish, per `documentation.md`'s check. It
found "Everything above that line then works as step 4a describes" — a
referent the newly inserted diagram destroyed — plus "this step's own count"
in an alternative that is not a step, and a characterisation of the seven
messages in alternative 11 that did not match how the body describes them.
All three fixed.
- Every internal pointer re-resolved: steps 1-10 including 4a, 4b, 7a and 9a all
survive under their own numbers, and alternatives 1-9 keep theirs.
- The set-level shapes are UNTOUCHED and verified: the *Where this ADR sits*
table is byte-identical to 0071's modulo bolding, the unifying sentence is
verbatim, and the `## References` sibling list diffs clean. Those move only in
X1's own commit, which is still owed.
FRONTMATTER: the `summary` was two long sentences and a fragment; it is now six,
and it now names Defect 1 and Defect 1b by name. That stales the derived index,
so `docs/adr/index.md` is REGENERATED IN THIS COMMIT — one row, 108, and
`_99 ADRs indexed._` is unchanged.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
`readability-plan.md` §3 marks the seven R rows and the M row against 0070 as closed by `fb27c81bb`, §4 records 0070 done with four to go, and §7 records the session. ⚠ §7 decision 3 (S4) is REWRITTEN rather than merely ticked, because its recommendation was falsified. It answered "promote the exception to public" with "the exception is what a caller catches" — reading *exception* as an exception TYPE, where the ADR means the one CLASS in the DI package that is not public. The old text is kept inline, marked as falsified, because the lesson is §20.6 row 3's in a new place: read the review's quoted sentence against the ADR before trusting the plan's gloss of it. The answer itself is NO DESIGN CHANGE, so S4 took no commit of its own — the first bucket-S item in the programme to move nothing. PROMPT.md is gitignored and is updated in the same session: §20.3 (3 of 7 done), §20.6 (new row 8 and the session report), §20.7 (S4 answered), the header, the ▶▶ work order for `0074`, and a NEW §20.10 holding the paragraph the review asked to be moved out of `0070:32` — which is the M item's actual destination, not merely its deletion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
Phase 3, session 4 of the readability programme. One ADR per session, worst-first: 0072, 0071 and 0070 are done; this is 0074, the second-largest file in the set. `docs/adr/index.md` is regenerated in this commit because the frontmatter `summary` was rewritten from one 90-word sentence into four. Bucket R items closed - `Scope` is now a statement of scope, not a narrative: parent requirement, an In-scope bullet per requirement naming the mechanism that discharges it and the criterion that guards it, a contributed-to-here list, and an Out-of-scope bullet per boundary naming the ADR that owns it. - The argument-as-record item (Phase 0 anchor `0074:401`, drifted to `:421`): "The alternative ... was rejected because ..." now states the decision, and the rejection moved to an alternative. - The two alternatives that were argued in prose between numbered items 2 and 3 are now numbered and each explicitly rejected. - The cross-reference pattern the review gave as its worked example: dense criterion-threading sentences are now a design point in prose followed by a bullet list of the FRs and ACs it satisfies. - D2's roles-table rename applied: Role / Type / Responsibilities / Responsibility classifier / Collaborators. Collaborators are new, per P1. - Two diagrams added, both forms the review asked for: a `classDiagram` for the ten new types and how they relate, and a `flowchart` for FR-22.3's candidate-to-finding funnel. All four diagrams rendered and looked at. - Bolded paragraph-leads promoted to `#####` headings where they were doing a heading's job; the review asked for the ideas not to be lost in the detail rather than picked out typographically. - Decision no longer carries a `file:line`; the citation lives in Technology Choices, where it was already duplicated. - `<`/`>` in the sequence diagram replaced with prose per the house style's no-escaped-markdown rule. Alternatives are APPENDED as 9-12, not interleaved. Round 6's record cites "0074 alternative 5" by number, so 1-8 keep their numbers. Claim inventory, six greps, before and after - requirement tokens: CLEAN, zero lost. - `file:line` citations: one changed, deliberately - see below. - bare `:NNN` citations: CLEAN. - sibling-ADR references: none lost. ADR 0014 falls 4 -> 3, a duplicate mention removed when the mirror-enum argument moved wholly into alternative 3, which still carries both ADR 0014 and NFR-7. - backticked identifiers: CLEAN apart from the corrected citation. - numerals used as counts: CLEAN, both as (numeral, noun) pairs and as raw frequency. Every numeral is level or higher. First session in four in which this grep finds nothing. One stale citation corrected: `PipelineValidator.cs:57` -> `:58` `:57` is the `subscriptions` parameter; `consumerSpecs` is `:58`. The citation was written by `bbb04d688` (the S5 call) three sessions ago and was off by one from the day it landed. No sibling carries it - checked - so this is a single-ADR fix and rides the rewrite. All 28 other file-qualified citations and all bare ones verified exact against `src/`. Readability - prose paragraphs/bullets over 200 words: 4 -> 2. - mid-prose bold runs: 143 -> 129. Bullet-lead bold rises 45 -> 69 and table bold is unchanged at 69, so the total rises 257 -> 268: emphasis moved out of sentences and into structure rather than being deleted. - 533 -> 794 lines for 16,961 -> 17,663 words, which is the house style's "prefer a slightly longer document to a terse one". Set-level shapes deliberately untouched and verified: the *Where this ADR sits* table diffs byte-identical to 0071's modulo the self-row, the unifying sentence is verbatim, and the `## References` sibling list diffs clean. X1 is still owed and is still not folded in here. Branch-2 / branch-3 list for this session: EMPTY. The rewrite surfaced no fact that had to keep a longer form, and none that looked wrong once isolated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
readability-plan.md §3 and §4: - the argument-as-record row: 0074:401 closed in e3ed130, and the anchor is recorded as having drifted to :421 because S5 rewrote the file after Phase 0 located it. Four of the original nine lines remain, in 0071, 0073 and 0076. - the move-argument-into-alternatives row: closed for 0074, appended as 9-12 rather than interleaved, because round 6's record cites "0074 alternative 5" by number. - §4's Phase 3 row: 4 of 7 done, three to go — 0075, 0076, 0073. PROMPT.md is gitignored and is updated in the working tree only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
Every sibling list and every *Where this ADR sits* table described ADR 0075 as suppression "for a `Publish` subscriber" only. The pump-flow bracket that landed with round 6's group A made that description incomplete: the same flag takes a third bracket around the consumer pump's own flow in `Performer.Run`, so a consumer pipeline owns its scope unconditionally. Both forms are byte-identical across all seven by design, so one change is thirteen lines in seven files, all in one commit: - seven *Where this ADR sits* rows (0075's own carries *(this one)*) - six `## References` sibling-list entries (0075 does not list itself) New text, both forms: how a `Publish` subscriber and the consumer pump suppress adoption, for themselves and every pipeline created beneath them Verified after the edit: the seven tables still diff byte-identical modulo the self-row, and the unifying sentence beneath each is untouched. Scope held deliberately. The ADR's own title and slug still name only the `Publish` subscriber; retitling would move a slug the other six cite by name and is not part of this correction. `docs/adr/index.md` carries each ADR's frontmatter `summary`, which no line here touches, so no regeneration is owed. Bucket S under the four-bucket rule (readability-plan.md §2): its own commit, never folded into a readability commit. Tracked as PROMPT.md §20.7 row X1. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
Phase 3, session 5 of the readability programme (readability-plan.md §4). Fifth of seven; 0072, 0071, 0070 and 0074 are done, 0076 and 0073 remain. The review's item 15 is a statement about METHOD: statement-level correction is what produced the prose being objected to, so the fix is a whole-document rewrite against a house style written down first. No fact is changed. WHAT THE HOUSE STYLE ASKED FOR, AND WHAT IT GOT D1 `### Scope` as a statement of scope. The narrative `**Scope**:` paragraphs become Parent requirement / In scope / Contributed to here, discharged elsewhere / Out of scope, one bullet per requirement, each naming the mechanism that discharges it and the ACs that guard it. D2/P1 Key Components. `Stereotype` -> `Responsibility classifier`, and the new `Collaborators` column P1 asks for. Responsibilities pluralised. Diagrams 2 -> 3. The publish sequence diagram said in terms that the pump bracket "is not drawn here"; it now is, as its own `sequenceDiagram` under `#### The pump-flow bracket`. Step 5a's four-cell claim about which restore is load-bearing on which twin becomes a table. STE and emphasis. Blocks over 200 words 6 -> 0 (worst was 359), over 150 11 -> 1, over 100 32 -> 16. Mid-prose bold 116 -> 113 while bullet-lead bold rose 73 -> 101, so the total rose 189 -> 214: emphasis moved into structure rather than being deleted. Words 10,673 -> 11,255, per the review's "prefer a slightly longer document than terseness". Alternatives 6-9 APPENDED, not interleaved. Four arguments carried in the body are now numbered rejections: the added `PipelineBuilder` overload, the per-task async bracket FR-9(b) permits, detecting a bracket disposed on the wrong flow, and an injected suppression role. Appending is forced — review-design.md cites "0075 alternative 3a" four times, "alternative 5" three times and "its alternative 4", and 0073:84 cites "ADR 0075's third alternative", so 1-5 and 3a keep their numbers. Step numbers are likewise pinned: 0072:169 cites "ADR 0075 step 4a" and PROMPT.md cites step 5a. CLAIM INVENTORY (readability-plan.md §6) — six greps, before and after Requirement tokens, `file:line` citations, bare `:NNN` citations and sibling-ADR references all diffed COMPLETELY CLEAN. Backticked identifiers: none lost, four added. One numeral fell, deliberately: "in the same shape ADR 0074 owns FR-25 while this ADR supplies two of its clauses" — the analogy drawn between the FR-25 arrangement and the FR-19 one. Both arrangements are now stated explicitly as adjacent bullets under one heading whose lead says each has exactly one owner and it is not this ADR, so the parallel is structural rather than asserted. The count itself survives: step 7 still says two pieces come from here, and 0074:48 still says "Two families of FR-25 clause come from ADR 0075". CITATIONS VERIFIED AGAINST SOURCE BEFORE WRITING — all 19 file-qualified citations exact, including the ones S1+S2 (`d6502deb5`) wrote into this file, and all 18 bare ones. The counted claims verify too: `tests/` has 69 `PipelineBuilder` constructions, 21 of them single-argument (describe-only), leaving 48; `CommandProcessor` has four dispatch-constructor sites; AC-13 does count five decisions with three of them the subscribers'; AC-24's four clauses do not reach the constructor note; ADR 0070 step 7a does say nine interfaces. THE RE-READ PASS caught one defect no grep can see. The pump-bracket bullet said a mixed host's shared `ConsumersOptions` is "the subject of the next paragraph"; restructuring put another bullet in between, so the pointer aimed at the wrong block. It now names the paragraph. Six ambiguous `it`/`that` referents and two consecutive paragraphs opening "The break" were fixed in the same pass, and one stale pointer (the overload "was declined in Technology Choices" — it is now Alternative 6). All three mermaid diagrams render; the new one was rendered to PNG and read. `grep -c '<|>|&'` is 0. Set-level shapes deliberately untouched and verified after the edit: all seven *Where this ADR sits* tables still hash identical modulo the self-row, every sibling-list entry still appears in exactly six files byte-identical, and the unifying sentence is verbatim. The frontmatter `summary` did not change, so unlike the four earlier rewrites this one owes no `index.md` regeneration — checked, the index carries the summary verbatim already. Branch-2 / branch-3 list for this session: EMPTY (readability-plan.md §6). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01YJjvJaUupNvDeTpEPp73as
|
Claude finished @iancooper's task in 2m 9s —— View job Claude is reviewing this PR
|
…f stall Adding Task to --allowed-tools didn't fix the stall (retested on BrighterCommand#4282 after merging the Task fix: still stalls after "Gather context", permission_denials still double digits). The job log only shows the init and final result JSON today, nothing in between, so there's no way to see which tool call is actually being denied. Turn on show_full_output to get the turn-by-turn stream before guessing again. Temporary - remove once the real cause is found and fixed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzFDNrg2Q18Zkm9MpR5TXv
…he stall show_full_output (BrighterCommand#4369) found the actual cause: every sub-agent's first move was `git fetch origin master` to diff against it, denied every time (8/8 denials on PR BrighterCommand#4282's retest were this one command in various forms) since raw git isn't in --allowed-tools. The review stalled retrying a command it could never be allowed to run, never reaching `gh pr diff`, which was allowed the whole time. Task was a real, necessary fix (confirmed working - it spawned five sub-agents on the retest) but not a sufficient one on its own; the comment explaining it is corrected here rather than left claiming it was the whole fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzFDNrg2Q18Zkm9MpR5TXv
|
Claude finished @iancooper's task in 1m 40s —— View job Claude is reviewing this PR
|
|
Claude finished @iancooper's task in 10m 15s —— View job Review — Spec 0036, implementation passTodo list
Note: the design (7 ADRs, requirements, tasks) already received a thorough review in earlier comments on this PR — this pass is implementation-only, and reads the actual files on disk ( Findings (posted inline, most severe first)
Areas checked with no solid defect found
Test coverageCoverage in |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| //dispose the pipeline scope handle last and unconditionally, holding any failure | ||
| try | ||
| { | ||
| _pipelineScope?.Dispose(); |
There was a problem hiding this comment.
Handler pipeline scope is never disposed asynchronously, unlike its mapper/transform sibling.
IAmAScope (returned by IAmAHandlerFactory.CreatePipelineScope()) is declared IDisposable, IAsyncDisposable, but HandlerLifetimeScope only implements IDisposable (IAmALifetime is IDisposable-only, IAmALifetime.cs:40), and Dispose() here always calls _pipelineScope?.Dispose() synchronously. PipelineBuilder<T> (PipelineBuilder.cs:293) is likewise IDisposable-only and is used via using var builder = ... even inside CommandProcessor's async SendAsync/PublishAsync (CommandProcessor.cs:394,578), so there is no code path that ever reaches IAmAScope.DisposeAsync() for a handler pipeline.
Compare with TransformPipelineAsync (TransformPipelineAsync.cs:64-99), which was given a full DisposeAsync/finalizer pair specifically because — per its own comments — releasing a scope holding an IAsyncDisposable-only service through the synchronous path can throw (MS DI's ServiceProviderEngineScope.Dispose() throws InvalidOperationException when it hits a service that implements only IAsyncDisposable, partway through disposing the rest of the scope's tracked disposables).
Concretely: a container-backed IAmAHandlerFactory whose pipeline scope resolves a Scoped dependency that implements only IAsyncDisposable (e.g. many async HTTP/gRPC client wrappers) will throw when the handler pipeline completes. That throw is caught here (line 123) and only logged as FailedToDisposePipelineScope — but by the time it's thrown, the underlying container scope's Dispose() loop has already aborted partway through, so some already-tracked disposables in that scope are never released: a silent per-pipeline resource leak, not just a log line.
Was this asymmetry with the mapper/transform side intentional, or is IAmALifetime/HandlerLifetimeScope missing the same async-disposal treatment?
| }, | ||
| c => | ||
| { | ||
| var affinity = ((ScopeAffinityOverride)c.AffinityOverrideRegistrations.Last().ImplementationInstance!).Affinity; |
There was a problem hiding this comment.
Unhandled NullReferenceException when a ScopeAffinityOverride is registered by factory delegate.
DefeatedOptIn()'s error factory force-reads AffinityOverrideRegistrations.Last().ImplementationInstance!. That's safe for the built-in path (AddBrighterRequestScope registers via services.AddSingleton(new ScopeAffinityOverride(affinity)) — an instance, see BrighterAspNetCoreExtensions.cs:57), but an application can register a ScopeAffinityOverride directly via a factory delegate (services.AddSingleton<ScopeAffinityOverride>(sp => ...)), leaving ImplementationInstance null. That's exactly the shape UnreadableOverride() (below, line ~214-228) is written to detect and handle gracefully — but DefeatedOptIn()'s predicate (lines 108-115) doesn't check for it, so if such an override then gets defeated (e.g. the app also registers its own IBrighterOptions), the predicate fails, the error factory runs, and (ScopeAffinityOverride)null! casts fine but .Affinity throws NullReferenceException.
Specification<T>.EvaluateSimple (Specification.cs:194-195) doesn't wrap the error-factory call in try/catch, so this propagates out of ScopeConfigurationValidator.Validate() and crashes BrighterValidationHostedService.StartAsync with a raw NRE instead of surfacing the intended, friendly validation message — a validation-error path that turns into an unhandled startup crash.
Narrow to trigger (requires bypassing AddBrighterRequestScope and registering the override via factory delegate), but worth a null-check/guard here mirroring UnreadableOverride()'s treatment.
| if (scopeProvider.GetService(typeof(ScopedArtefactCache)) is ScopedArtefactCache registered) | ||
| return registered; | ||
|
|
||
| return LazyInitializer.EnsureInitialized(ref _ownedFallbackCache, () => new ScopedArtefactCache())!; |
There was a problem hiding this comment.
LazyInitializer.EnsureInitialized here can construct ScopedArtefactCache more than once under a race, leaking the losers.
This is the 3-argument EnsureInitialized(ref T, Func<T>) overload with no syncLock, whose documented contract is that under concurrent callers the factory may run more than once, and only one result is published to _ownedFallbackCache — the others are simply discarded, not disposed. ScopedArtefactCache is IDisposable and tracks a static s_liveCount (incremented in its constructor, decremented in Dispose(), ScopedArtefactCache.cs:47-95) that's used elsewhere as a leak-detection instrument (per the PR description, a "positive control" retained-count assertion). A discarded, never-disposed instance here permanently inflates LiveCount.
This path is only reached on a hand-built host that never ran AddBrighter/BrighterHandlerBuilder (per the doc comment above), so a registered ScopedArtefactCache normally short-circuits it — but when it is reached, concurrent first resolutions on the same owned lifetime scope will leak. If exactly-once construction matters here (as it seems to, given the leak-detection use elsewhere), consider the lock-based overload (EnsureInitialized(ref T, ref bool, ref object)) or a CompareExchange-and-dispose-the-loser pattern, matching EnsureRootScopePublished's own comment just below this method about exactly that hazard.
…views Two consecutive runs on BrighterCommand#4282 after the gh-pr-diff fix (BrighterCommand#4370) showed zero permission denials and real sub-agent progress (one had read a real file, made 14 tool calls, run 31s) - then the log truncated mid-tool-call at ~100-140s wall clock, with no result event, right as the "clear the label" step started. That timing is consistent for two runs with very different amounts of sub-agent work done, which points at a fixed internal timeout rather than a turn or budget limit. Claude Code's MCP_TOOL_TIMEOUT env var caps how long a single tool call may run, and sub-agent tool calls are specifically exempt from the automatic call-backgrounding the main conversation gets - making them more exposed to this than a top-level call. Setting it to ten minutes gives a sub-agent room to actually finish reviewing its slice of a 462-file diff. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzFDNrg2Q18Zkm9MpR5TXv
DefeatedOptIn()'s error factory force-unwrapped AffinityOverrideRegistrations.Last().ImplementationInstance with '!', which threw a NullReferenceException when the override was registered by factory delegate (no ImplementationInstance) rather than as a constructed instance, and the opt-in was separately defeated by an app registering its own IBrighterOptions ahead of AddBrighter. DefeatedOptIn now declines to fire in that shape, since UnreadableOverride() already reports it as a Warning and this rule has no readable value to name in its own message. PR #4282 review finding #2. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzFDNrg2Q18Zkm9MpR5TXv
ResolveOwnedArtefactCache's fallback path (reached only on a hand-built host that never registered ScopedArtefactCache itself, i.e. never ran AddBrighter/BrighterHandlerBuilder) used LazyInitializer.EnsureInitialized's no-syncLock overload, whose documented contract allows the factory to run more than once under concurrent first callers - only one result publishes; the rest were silently discarded and never disposed, permanently inflating ScopedArtefactCache's own live-instance counter. Now publishes with Interlocked.CompareExchange and disposes the loser, mirroring EnsureRootScopePublished's own race-handling in this same file. PR #4282 review finding #3. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzFDNrg2Q18Zkm9MpR5TXv
IAmALifetime now extends IAsyncDisposable, mirroring IAmAScope. HandlerLifetimeScope gains DisposeAsync() (plus a finalizer, matching TransformPipelineAsync's pattern) that awaits its pipeline scope's DisposeAsync() instead of blocking on the synchronous Dispose() - closing the asymmetry with the mapper/transform side, which already had this. PipelineBuilder<T> gains its own DisposeAsync(), and CommandProcessor's SendAsync/PublishAsync now `await using` their builder instead of `using`, so an async pipeline's own IAmAScope handle is genuinely awaited rather than blocked on. Individual handler releases stay synchronous - neither IAmAHandlerFactorySync nor IAmAHandlerFactoryAsync offers an async Release for one handler - only the pipeline scope's own disposal becomes truly async. Verified empirically before this fix that Brighter's own built-in ServiceProviderHandlerFactory/ServiceProviderPipelineScope does not actually throw or deadlock today for a captive Scoped IAsyncDisposable-only dependency (ServiceProviderLifetimeScope.DisposeScope already routes through a blocking-wait-on-DisposeAsync for any IAsyncDisposable scope) - the real gap this fix closes is that the async Send/Publish path blocked a thread on that wait rather than truly awaiting, and a custom IAmAScope implementation with only genuine async cleanup had no clean path. Seven duplicated TestLifetimeScope test doubles across Extensions.Tests needed a DisposeAsync() implementation to keep implementing IAmALifetime. PR #4282 review finding #1 (rescoped after empirical verification - see the commit message above and PROMPT.md for the correction). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzFDNrg2Q18Zkm9MpR5TXv
|
@claude Please re-review this PR. Since your last review (2026-09-16), 3 new commits (
Each fix has its own regression test (linked in the commit). Please verify each fix actually addresses its finding, and re-check the rest of the diff for anything new. |
|
Claude finished @iancooper's task in 5m 20s —— View job Re-review — Spec 0036, verification of the three fixesTodo list
Note on verification method: Verdict on each fixFix 1 — I traced the whole path rather than taking the commit message's word for it:
Checked specifically:
Fix 2 — Fix 3 — New findings1.
|
…verride The previous fix for finding #2 (fd77dce) made DefeatedOptIn() decline to fire entirely when the affinity override was registered by factory delegate, to avoid force-unwrapping a null ImplementationInstance. That over-corrected: it also suppressed the FR-22.4 Error for a defeat that genuinely happened, so with throwOnError:true an application whose JoinAmbient opt-in was defeated would now start up successfully instead of failing validation. DefeatedOptIn now keeps firing on a genuine defeat regardless of whether the override's value is readable, and only omits the affinity-value interpolation (falling back to a value-free message) when the instance isn't a ScopeAffinityOverride. The UnreadableOverride Warning and the DefeatedOptIn Error are not duplicates - they report two different problems (the override's value can't be read; the opt-in never took effect) that happen to co-occur. This also closes review finding #2 (the gap between DefeatedOptIn's and UnreadableOverride's predicates) by construction, since DefeatedOptIn no longer special-cases unreadability at all. PR #4282 re-review findings #1 and #2 (2026-09-18). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzFDNrg2Q18Zkm9MpR5TXv
Commit d4ac64b made the public IAmALifetime interface also extend IAsyncDisposable (a source break on netstandard2.0 - seven in-repo test doubles needed a DisposeAsync() to keep implementing it), but the release-notes breaking-change catalogue only documented the earlier PipelineScope property addition. Added a fourteenth catalogue item and extended IAmALifetime's per-interface migration bullet to cover it. PR #4282 re-review finding #3 (2026-09-18). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzFDNrg2Q18Zkm9MpR5TXv
|
Thanks for the re-review — response to each finding:
All three regression suites ( |
|
@claude Please do one more re-review pass — likely the last one for this PR. Since your previous re-review, 2 more commits were pushed addressing findings #1 and #3 from that pass (findings #2, #4 and #5 were resolved without a code change - see the comment above for details):
Please verify fix #1 actually closes the gap (an app whose opt-in is genuinely defeated should still fail startup validation when |
|
Claude finished @iancooper's task in 5m 51s —— View job Final re-review — Spec 0036Todo list
Verification method: static analysis only — Fix #1 —
|
dbe70e0 fixed a resolver-vs-resolver race on ResolveOwnedArtefactCache's fallback path (the very first Scoped resolution on a hand-built host that never registered ScopedArtefactCache itself), but a second, different race remained: resolver-vs-Dispose. All four teardown paths read _ownedFallbackCache directly in their finally block rather than claiming it atomically the way _scope already is, so a resolver that read the field as null, was preempted, and a concurrent Dispose() ran to completion in that window (seeing the field still null, so its own cleanup was a no-op) would then publish its own cache into a field nothing would ever drain again. ResolveOwnedArtefactCache now re-checks _disposed after winning the publish and reclaims-and-disposes if a concurrent Dispose already ran, mirroring EnsureRootScopePublished's existing pattern for _scope. All four teardown paths now claim _ownedFallbackCache with the same Interlocked.Exchange, so exactly one side ever disposes a given cache instance - ScopedArtefactCache. Dispose() is not idempotent (it decrements a static counter unconditionally), so a double-dispose would under-count rather than no-op. Regression test drives the race entirely through public surface (a hand-built ServiceCollection with no AddBrighter, N resolver threads and one Dispose thread released simultaneously via a Barrier) and reproduced the leak in 3-12 of 200 trials against the unfixed code across three separate runs. PR #4282 re-review finding #2 (final pass, 2026-09-18). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzFDNrg2Q18Zkm9MpR5TXv
Two docs-only fixes from PR #4282's final re-review pass: - docs/guides/lifetimes-and-scoping.md section 6: the "Defeated opt-in" and "Unreadable override" entries no longer matched the rule they document after ac46633 gave DefeatedOptIn a second, value-free message form and made the two findings co-occur. "Defeated opt-in" now notes the affinity-omitting variant; "Unreadable override" no longer unconditionally claims the override "still takes effect" - it now points at the Defeated opt-in entry for the case where that claim doesn't hold. - ContainerRegistrationSnapshot's <remarks> still said "Nothing calls this type yet", a stale note from when it landed inert in T7.0a. It is now constructed by BrighterPipelineValidationExtensions and read by every rule in ScopeConfigurationRules. PR #4282 re-review findings #1 and #3 (final pass, 2026-09-18). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzFDNrg2Q18Zkm9MpR5TXv
|
All three findings from the final re-review addressed:
All three regression suites ( That's everything from this review pass. Thanks for the thorough look across all three rounds — moving to merge next. |
1 participant
Closes #4256.
Implementation is complete — all 82 tasks in
tasks.mdare done. This was in draft while the design (7 ADRs, requirements, task breakdown) was visible during implementation; it now contains the full implementation, test suite and documentation and is ready for review.The problem
ServiceLifetime.Scopeddid not mean per-pipeline for message mappers and transforms — in practice aScopedmapper was cached for the life of the process. This madeScopedbehave differently for Brighter artefacts than a user's intuition from ASP.NET Core, and gave no way for a pipeline to join a caller's existing scope, so a handler and a mapper in the same HTTP request couldn't share aScopedDbContext.What changed
HandlerLifetime,MapperLifetimeandTransformerLifetimenow govern a pipeline-scoped DI scope: aScopedparticipant resolves from one DI scope shared by everyScopedparticipant on that pipeline, disposed when the pipeline ends.Paramore.Brighter.Extensions.AspNetCorepackage lets an ASP.NET Core pipeline adopt an ambient request scope instead of creating its own — one line,AddBrighterRequestScope(). Not opting in changes nothing.ValidatePipelines()gained seven new startup checks for common lifetime/scope-registration mistakes (three errors, four warnings), each pointing at a new guidance page:docs/guides/lifetimes-and-scoping.md— the lifetime model, the adoption truth table, a decision guide for choosing a lifetime triple, and a troubleshooting entry for each validation message.Publishsubscribers and the consumer pump deliberately never adopt an ambient scope, preserving ADR 0039's per-subscriber isolation.release_notes.mdentry — the headline one isMapperLifetime.Scopedno longer caching across messages (no compatibility flag; migration isMapperLifetime = Singleton). Six interfaces gain members — the four mapper/transformer factories plus the two mapper registries — source- and binary-breaking onnetstandard2.0, since no default interface member can absorb it, so every implementation in the repo moves in one commit.Design (7 ADRs, all
Accepted)Read in this order — compile-dependency order, not numeric:
Publishsubscribers and the consumer pumprequirements.md— 27 FRs, 10 NFRs, 55 acceptance criteria, the contract every test asserts against.Review process already applied
The design and task breakdown went through adversarial review rounds before implementation started, and every task went through TDD (test written and reviewed before implementation, full regression run after):
AcceptedScope of the diff
462 files under
src//tests/(24,453 insertions), plus specs/ADRs/docs — 510 files total againstmaster.Regression status
Core.Tests,Extensions.TestsandExtensions.AspNetCore.Tests, both net9.0/net10.0, run clean except two pre-existing flakes unrelated to this work (both confirmed passing in isolation, both predate this branch):When_configuring_json_serialisation.Should_preserve_existing_options(Extensions.Tests) — a parallel-test-execution race on a shared staticJsonSerializerOptions.PackageReferencedButExtensionNotCalledTests(Extensions.AspNetCore.Tests) — aWebApplicationFactoryparallel-hosts disposal race.Where a reviewer's attention is most valuable
Scoped-means-per-pipeline the right default, given it's a breaking change? (docs/adr/0070-per-pipeline-di-scope-for-mapper-and-transform-factories.md)Publishsubscribers and the consumer pump deliberately do not adopt a caller's scope. The decision most likely to surprise.ScopeConfigurationRules.cs) — each names the guidance page;docs/guides/lifetimes-and-scoping.md§6 is meant to be self-sufficient for fixing any of them without reading source.release_notes.md— one entry, thirteen items; flag if anything is missing or mis-stated.🤖 Generated with Claude Code
https://claude.ai/code/session_01WzFDNrg2Q18Zkm9MpR5TXv