Skip to content

fix(server): thread.turn.start bootstrap parity between websocket and http dispatch - #7996

Open
pablospe wants to merge 6 commits into
pingdotgg:mainfrom
pablospe:orc/http-bootstrap-parity
Open

fix(server): thread.turn.start bootstrap parity between websocket and http dispatch#7996
pablospe wants to merge 6 commits into
pingdotgg:mainfrom
pablospe:orc/http-bootstrap-parity

fix(server): http bootstrap failures report the rolled-back thread

a4ec449
Select commit
Loading
Failed to load commit list.
MacroscopeApp / Macroscope - Effect Service Conventions succeeded Aug 23, 2026 in 1m 34s

All clear

All clear

Details

Note

Your check run agent prompt is: .macroscope/check-run-agents/effect-service-conventions.md
More information about how Check Run Agents work can be found in our Help Center.


🔍 Agent activity — 6 tool calls

💭 Checking the current state of the PR since the prior review flagged an issue with projectSetupScriptCompatibilityDetail at an earlier commit—looking at the diff now, it seems TurnStartBootstrap.ts has since added exhaustive error handling with a proper fallback case, so that concern appears resolved. Now I'm scanning the new file for convention adherence: import style looks consistent, and service definitions follow the expected order of imports, types, Context.Service tag, then make and layer. I notice a schema predicate defined as a private constant rather than an exported one, but since it's pre-existing code just moved from elsewhere and not wrapped in a redundant function, it likely isn't worth flagging. I also check the mapper that passes through existing domain errors, which is legitimate normalization, and the dependency acquisition pattern using yield* for services followed by Layer.effect. In ws.ts, I see an already-acquired service instance re-provided into a child layer via Layer.succeed, which is proper context propagation rather than fake dependency injection. I do notice the error wrapper derives its message from cause.message, which technically violates the rule about deriving messages only from stable structural attributes -- but since this is pre-existing moved code, I need to think about whether that's still worth flagging. The similar concern about detail copying cause.message into the activity payload is still present through setupFailureDescription, but that's also pre-existing behavior meant to be preserved. Since the guidance says not to demand unrelated cleanup and to preserve messages during a structural refactor, I'm inclined not to flag either issue since they're verbatim moves rather than new violations. Now I'm checking http.ts and TurnStartBootstrap to confirm error-channel handling is exhaustive and that the bootstrap layer is provided wherever needed. Both places use full-channel error catching correctly, so I want to verify the file at head for ordering, naming, and any remaining references to ws.ts.

1. browse_code — apps/server/src/orchestration/TurnStartBootstrap.ts

2. browse_code — apps/server/src/orchestration/http.ts

💭 I should check ws.ts to verify there's nothing leftover unused, particularly the bootstrap layer usage, and whether the dispatch command error helpers are still referenced there despite the duplication being acceptable for now.

3. git_tools — TurnStartBootstrap|unexpectedCompatibilityError|isOrchestrationDispatchCommandError|toDispatchCommandError|nowIso

💭 I'm noting this follows an established re-provisioning pattern already used in ws.ts. Then I'm checking convention concerns: the duplicated error-type helpers between ws.ts and TurnStartBootstrap.ts appear to be a pre-existing pattern rather than a new violation, and I'm starting to examine the error-catching logic in the setup script recording function. Looking further, the service structure with Crypto acquired from the environment, helper function placement relative to the tag and make, and unused let bindings all seem like minor issues rather than real convention violations. The one thing worth verifying is whether TurnStartBootstrap.layer is actually being provided to the orchestration HTTP layer in server.ts, so I want to check its placement there.

4. browse_code — apps/server/src/server.ts

💭 Good, that's a shared instance. The bin.test.ts using a mock layer is fine as a test seam. Checking convention violations, I notice the log warning copies an arbitrary cause message verbatim into the log and activity payload — something I'd already flagged in a prior pass. The compile-time exhaustiveness check was addressed, but the static-message part wasn't changed — though this looks deliberate, since the convention says to preserve UI-visible error messages during structural refactors, so re-flagging would just be a duplicate. I'll let it stand. The catchCause and Cause.squash usage look fine too, and now I'm checking http.ts's error handling. I'm scanning the remaining files — bin.test.ts, TurnStartBootstrap.ts, server.test.ts, server.ts, ws.ts, and environmentHttp.ts — to confirm nothing else needs attention. Layer.mock in bin.test.ts looks fine, environmentHttp.ts's added literal reason is fine, and the ws.ts changes with options being passed for sub-commands look consistent with the client-origin dispatch pattern already in place. Given this PR changes real backend dispatch behavior rather than just refactoring mechanically, the added focused tests are justified per the conventions. Now I want to verify whether Crypto was removed from ws.ts but serverEventId is still referenced somewhere, so I'm checking where serverCommandId might still be used in that file.

5. git_tools — serverCommandId|serverEventId|EventId|Crypto|randomUUID

💭 Checking whether TurnStartBootstrap.ts's placement and import style follow the codebase convention of importing local service modules as a namespace at service boundaries — this one uses a named import instead, which seems worth flagging.

6. git_tools — from "./Services/OrchestrationEngine.ts"|from "../orchestration/Services/OrchestrationEngine.ts"

💭 I don't find any new convention violations, and the prior finding was addressed. There's a tension between the instruction to output exactly "All clear" and the requirement to follow the JSON schema, so I'll reconcile by setting the state to success with "All clear" as the title and summary while keeping the JSON format intact.


Agent Credits: 64 credits