💭 Looking at the changed file, I'm evaluating whether the error class conventions are being followed. The TerminalSubprocessCheckError removes the terminalPid field and derives the message from command alone, which acts as a discriminator between powershell and ps. The snapshotFailure helper seems to be building an error object, though the thought cuts off before I can fully assess whether it violates the guideline about not creating helpers that just wrap error construction.… Looking at whether snapshotFailure violates conventions — it's a helper that constructs an error with a formatted message and encodes structural data like exit codes and timeouts into the message string rather than as proper attributes. This runs counter to the principle of not creating helpers that just wrap error construction, and also conflicts with the guidance against manufacturing errors solely to populate a cause field. The fix is straightforward: add exitCode, timedOut, and truncated as actual fields on TerminalSubprocessCheckError instead of embedding them in a fabricated error message. On the imports side, the test file pulls ChildProcessSpawner as a named import from "effect/unstable/process", but the convention expects subpath imports to come in as namespaces — so it should be import * as UnstableProcess from "effect/unstable/process" and then reference it as UnstableProcess.ChildProcessSpawner.… The test imports look standard across the codebase, so that's fine. The real issue is how snapshotFailure is creating an Error object just to use as a cause, which violates the principle of not manufacturing errors for that purpose. Instead, I should be capturing the actual domain context—like the operation or stage—in a normalized, serializable way.… So Effect.catch properly handles the entire channel, which is acceptable. The error being logged is the actual error object, and its cause is a synthetic Error with exit code information—following the convention of keeping direct error attributes safe without copying raw wire data or command arguments. The only real issue I'm seeing is that snapshotFailure creates a fabricated Error cause with an unstructured message, and I'm weighing whether that's a clear enough convention violation to warrant failing the check. The convention explicitly forbids manufacturing an Error just to populate cause, and says to capture context as structural attributes instead. Yeah, I think I should flag this with an inline comment. The fix would involve adding fields to the error class and removing the fabricated cause—that's a multi-hunk change across the same file, so I'll describe it rather than provide a diff. I'm also considering whether the helper function itself should be removed since it only wraps the error construction, but it's used twice and builds the message, so that's worth thinking through once the class gets proper fields. Now I need to figure out the diff line numbers in…

1. modify_pr — One convention finding on the new snapshot failure path in apps/server/src/terminal/Manager.ts: the snapshot failure is modeled by fabricating an Error whose message carries the structural context, instead of putting that context on TerminalSubprocessCheckError as attributes.