refactor(daemon-client): one socket round-trip primitive, budgets as caller values (6gd)#101
Merged
Merged
Conversation
…caller values (6gd) Sheriff finding #6: the connect-with-timeout + frame round-trip was implemented three times (client.ts 50/150ms, client-stats.ts and client-debug.ts 200/500ms), and the stats/debug copies had drifted from client.ts's outcome classification. [LAW:one-type-per-behavior] src/daemon/client-transport.ts is now the single implementation: connect, one framed round-trip, socket teardown, and the transient/permanent failure classification (moved verbatim from client.ts, the maintained post-2l6 copy). The three clients pass their timeout budgets and payload projection as VALUES — render/click keep 50/150/200, stats/debug keep 200/500. No flags, no modes. [LAW:single-enforcer] PROTOCOL_VERSION is stamped on every outbound request inside the transport; callers cannot mis-stamp it. ClientOutcome becomes RoundTripOutcome<string>; stats/debug inherit the typed classification, so the "daemon may not be running" hint now appears only on transient failures (on permanent ones the daemon is demonstrably up). check-protocol anchors (CONNECT_TIMEOUT_MS / TOTAL_BUDGET_MS in client.ts) stay in place; all 12 mirrored constants verified.
There was a problem hiding this comment.
Z.ai Coding Agent Review
This refactoring extracts the duplicated socket round-trip (connect/send/classify/destroy) from three files into a single requestOutcome primitive in client-transport.ts. Callers pass their timeout budgets and payload projector as values — no caller-identity branching in the transport layer. This is a textbook application of [LAW:one-type-per-behavior] and [LAW:dataflow-not-control-flow].
No must-change issues found. Specifics examined:
- Type relocation:
TransientOutcome,PermanentOutcome, andFailureOutcomemoved fromclient.tstoclient-transport.ts. All consumers updated.client.tsre-exportsClientOutcome = RoundTripOutcome<string>. Clean. - Field rename: ok-variant
output→value(generic name for the parameterizedRoundTripOutcome<T>). All access sites updated (outcome-plan.ts, tests). No stale references. - Protocol check script:
check-protocol.mjsanchors onCONNECT_TIMEOUT_MSandTOTAL_BUDGET_MSinclient.ts, which still exist as named consts. No breakage. - Behavioral fix (good):
client-debug.tsandclient-stats.tspreviously always showed the "daemon may not be running" hint via.catch(), even on permanent failures. NowdescribeFailureconditionally includes the hint only for transient outcomes — the correct behavior, matching the render path's kick-vs-no-kick semantics. describeFailureexhaustiveness: The switch overPermanentOutcome.causehas nodefaultbranch, so TypeScript enforces exhaustiveness at compile time. Adding a new cause is a compile error.asyncremoval on forwarding functions:tryRenderViaDaemonandtryClickViaDaemonno longer useasyncsince they just forward the promise fromrequestOutcome. Correct — avoids an unnecessary microtask tick.- Tests: New
test/client-transport.test.tscoversdescribeFailure's contract (hint only on transient, version formatting, message surfacing). Existingdaemon-version-mismatch.test.tscovers the outcome typing andplanOutcome.
Pre-existing note (not from this diff): interpretException classifies OS errors by substring-matching error messages (message.includes("ECONNREFUSED") etc.). This is a known limitation — Node.js doesn't provide structured error codes for all socket errors. The ProtocolError/SyntaxError paths use structural matching (instanceof) as the comment explains.
✅ Approved
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
2 participants
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.
Summary
Closes sheriff finding #6 (ticket
brandon-daemon-client-6gd, epic 58c): the daemon socket round-trip was implemented three times —client.ts(connectWithTimeout, 50/150ms),client-stats.ts(connect, 200/500ms),client-debug.ts(connect, 200/500ms) — and the stats/debug copies had drifted fromclient.ts's outcome classification.Locked decision (from the ticket): one connect+round-trip primitive parameterized by
(connectMs, budgetMs); the three callers pass their budgets as VALUES. Render keeps 50/150, stats/debug keep 200/500.What changed
src/daemon/client-transport.ts— the single round-trip implementation: connect with timeout, one framed request/response, socket teardown, and the transient/permanent failure classification (moved verbatim fromclient.ts, the maintained post-2l6 copy). Callers differ only in values: request,RoundTripBudgets, and a payload projector (output/stats/debug).[LAW:one-type-per-behavior][LAW:dataflow-not-control-flow]PROTOCOL_VERSIONstamped at one boundary — the transport stampsvon every outbound request (UnversionedRequest); callers cannot mis-stamp or omit it.[LAW:single-enforcer]client.tsshrinks to the render/click vocabulary: budget consts (check-protocol anchors untouched — all 12 mirrored constants verified),ClientOutcome = RoundTripOutcome<string>, and the output projector.client-stats.ts/client-debug.tsdrop their privateconnectcopies and ad-hoc thrown errors, inheriting the typed classification they had drifted away from. Observable improvement: the "daemon may not be running" hint now prints only on transient failures — previously it printed even onVERSION_MISMATCH, where the daemon is demonstrably running.RoundTripOutcome<T>to avoid colliding with the provider-fetchOutcome<T>insrc/utils/outcome.ts— the two unions carry genuinely different failure semantics (kick-vs-no-kick vs absent-vs-failed) and stay distinct types.Verification
pnpm typecheck,pnpm lint,pnpm check:protocol(12/12 mirrored constants) all cleantest/client-transport.test.tspinning the hint-only-on-transient contract)net.createConnectionand onesendOnecallsite remain on the client side