docs(decisions): record conn:AsyncConnection|None union as deliberate#142
Merged
Conversation
Architecture review candidate #5 flagged the `conn: AsyncConnection | None` parameter on AbstractOutboxClient as a "type lie". On inspection it is a documented, deliberate widening: `conn is None` iff the client is the fake (no engine), the subscriber genuinely produces None on that path, and the real client guards with TypeError. The proposed narrowing is unsound (a cast would lie harder); the only honest fix is moving connection-acquisition behind the seam, a sizable refactor not justified today. Record the rationale + a revisit trigger so future reviews don't re-flag it. No code change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Records a design decision (no code change) in response to architecture-review candidate #5, which flagged the
conn: AsyncConnection | Noneparameter onAbstractOutboxClientas a "type lie."On close inspection it is a deliberate, documented widening (
client.py:69-74), not an accidental lie, and the review's proposed narrowing doesn't hold:conn is None ⟺ engine is None ⟺ fake client. The subscriber genuinely producesNoneon the fake path (_open_worker_resourcesyieldswriter_conn=Nonewhenengine is None); the real client always gets a live connection.Nonein. Acast(AsyncConnection, None)would replace an honest union with a cast that lies at runtime.acquire_writer()/acquire_reader()handle) is a sizable refactor of the worker loop, fetch loop, both adapters, and the lease-token write path — disproportionate today.The decision file captures this with a revisit trigger (a third adapter, or moving the connection lifecycle behind the seam for another reason) so future reviews don't re-litigate it.
Verification
just check-planning—planning: OK(decision indexed)🤖 Generated with Claude Code