refactor: eliminate all Tier 1 architectural violations (reducer-effect pipeline)#138
Merged
refactor: eliminate all Tier 1 architectural violations (reducer-effect pipeline)#138
Conversation
Previously handleSystemSignal() called emitEvent() directly for CONSUMER_CONNECTED and CONSUMER_DISCONNECTED signals, bypassing the reducer-effect pipeline. Enrich signals with consumerCountAfter and identity before reducer call, then produce EMIT_EVENT effects from the pure reducer. Runtime post-reducer hook now only manages the WebSocket handle map (non-serializable state).
…ONSUMER effect (Tier 1 #4 + P3) Add SEND_TO_CONSUMER effect type so targeted per-consumer errors go through the reducer-effect pipeline instead of being sent inline. Tier 1 #4: MessageQueueHandler.sendTo() calls replaced with QUEUE_ERROR signal dispatches; reducer produces SEND_TO_CONSUMER effects. P3 fix: handleInboundCommand() post-reducer user_message and set_adapter rejections now append SEND_TO_CONSUMER effects to the effect list instead of calling broadcaster.sendTo() directly.
Update docs/architecture.md: - Replace Tier 1 table with resolved summary (#1–4 all fixed) - Update Tier 2 #6 (pendingInitialize now set in correct post-reducer hook) - Update Tier 3 #14 (handleInboundCommand rejections fixed via SEND_TO_CONSUMER) - Update Tier 3 #16 (trySendRawToBackend now runtime-internal only) Also narrow SLASH_LOCAL_RESULT signal source type to "emulated" | "cli" to match ConsumerMessage contract (caught by typecheck).
…ND_TO_CONSUMER test) - Guard CAPABILITIES_INIT_REQUESTED against no_backend result so the pending-initialize timer is only armed after a successful send - Add ws !== null guards in handleInboundCommand extraEffects to avoid producing SEND_TO_CONSUMER effects for programmatic (null ws) calls - Log a warning on CONSUMER_DISCONNECTED when the socket is unregistered - Add sendTo to makeDeps() in effect-executor.test.ts and cover SEND_TO_CONSUMER with a dedicated test - Update effect-types.ts module header to document the SEND_TO_CONSUMER exception to the "plain data, no dependencies" contract
Contributor
Summary of ChangesHello @teng-lin, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors core session management logic to align with the established reducer-effect architectural principles. By routing various direct I/O operations and event emissions through a standardized signal-reducer-effect pipeline, the changes enhance the system's testability, predictability, and overall architectural integrity. The introduction of new system signals and an effect type for targeted consumer messages streamlines error handling and event propagation, ensuring a more consistent and robust flow of information within the application. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This is an excellent and thorough refactoring that successfully eliminates the Tier 1 architectural violations by moving side effects into a proper reducer-effect pipeline. The changes are consistent across the codebase, including production code, tests, and documentation, which significantly improves the architectural purity and maintainability. The minor suggestion to simplify a newly added logic block for improved readability remains relevant.
Comment on lines
+318
to
+350
| // Augment reducer effects with SEND_TO_CONSUMER effects for rejection cases. | ||
| // These require the live ws handle, which the pure reducer cannot access. | ||
| // ws is null as never for programmatic sendUserMessage() / sendPermissionResponse() calls — | ||
| // skip the targeted error in those cases (the caller checks the boolean return value instead). | ||
| const extraEffects: import("./effect-types.js").Effect[] = []; | ||
| if (msg.type === "user_message" && nextData === prevData && ws !== null) { | ||
| // Reducer no-op means closed/closing session — send targeted error. | ||
| extraEffects.push({ | ||
| type: "SEND_TO_CONSUMER", | ||
| ws, | ||
| message: { | ||
| type: "error", | ||
| message: "Session is closing or closed and cannot accept new messages.", | ||
| }, | ||
| }); | ||
| } else if (msg.type === "set_adapter" && ws !== null) { | ||
| // Rejected for active sessions — send targeted error to the requesting consumer only. | ||
| extraEffects.push({ | ||
| type: "SEND_TO_CONSUMER", | ||
| ws, | ||
| message: { | ||
| type: "error", | ||
| message: | ||
| "Adapter cannot be changed on an active session. Create a new session with the desired adapter.", | ||
| }); | ||
| break; | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| executeEffects( | ||
| extraEffects.length > 0 ? [...effects, ...extraEffects] : effects, | ||
| this.session, | ||
| this.effectDeps(), | ||
| ); |
Contributor
There was a problem hiding this comment.
For improved readability and to reduce verbosity, the logic for augmenting the effects array can be simplified. Instead of creating a separate extraEffects array and then conditionally merging it, you can directly push the new effects onto the effects array returned by the reducer. Since effects is a mutable local variable, this is a safe and more direct approach.
// Augment reducer effects with SEND_TO_CONSUMER effects for rejection cases.
// These require the live ws handle, which the pure reducer cannot access.
// ws is null for programmatic sendUserMessage() / sendPermissionResponse() calls —
// skip the targeted error in those cases (the caller checks the boolean return value instead).
if (msg.type === "user_message" && nextData === prevData && ws !== null) {
// Reducer no-op means closed/closing session — send targeted error.
effects.push({
type: "SEND_TO_CONSUMER",
ws,
message: {
type: "error",
message: "Session is closing or closed and cannot accept new messages.",
},
});
} else if (msg.type === "set_adapter" && ws !== null) {
// Rejected for active sessions — send targeted error to the requesting consumer only.
effects.push({
type: "SEND_TO_CONSUMER",
ws,
message: {
type: "error",
message:
"Adapter cannot be changed on an active session. Create a new session with the desired adapter.",
},
});
}
executeEffects(effects, this.session, this.effectDeps());
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.
Summary
SessionReducer→SLASH_LOCAL_RESULT/SLASH_LOCAL_ERRORsignals instead of calling broadcaster/emitter directly fromSlashCommandChainSessionRuntimepost-reducer hook triggered by the newCAPABILITIES_INIT_REQUESTEDsignal, replacing direct I/O inCapabilitiesPolicyCONSUMER_CONNECTED/CONSUMER_DISCONNECTEDconsumer event emissions into reducerEMIT_EVENTeffects; runtime enriches signals with counts and identity before the reducer callSEND_TO_CONSUMEReffect type; routeQUEUE_ERRORsignals andhandleInboundCommandrejections through the effect pipeline instead of callingbroadcaster.sendTodirectlyCAPABILITIES_INIT_REQUESTEDagainstno_backendresult (prevented phantom capability timeouts); addws !== nullguards for programmatic sendUserMessage calls; log warning on unregistered socket disconnect; addSEND_TO_CONSUMEReffect test +sendTotomakeDeps(); documentSEND_TO_CONSUMERexception to the plain-data contract ineffect-types.tsTest Plan
pnpm test— all 2923 unit + integration tests passpnpm typecheck— no TypeScript errorspnpm check:fix— no lint errorsCAPABILITIES_INIT_REQUESTEDonly arms the pending-initialize timer after a successful backend send (not onno_backend/unsupported)QUEUE_ERRORpath: queueing a second message while one is already queued sends an error only to the requesting consumer sockethandleInboundCommandrejections (session closing, adapter lock) send targeted errors viaSEND_TO_CONSUMEReffect