fix(worker): preserve delivered MCP errors - #1369
Conversation
Signed-off-by: wargloom <wargloom@gmail.com>
f6797fe to
8fc04c0
Compare
|
Reviewed this one closely — thank you, it is a genuinely good change and the reasoning in the description is the part I appreciated most. Separating the two layers is the right call: the worker's process status describes transport health, while the serialized payload describes tool success or failure. Collapsing those meant a perfectly valid MCP error response was thrown away and reported to the user as A few things I checked rather than assumed:
One entirely non-blocking nit: the failure path exits with Same question as on #1368: this is still a draft, so I have not promoted it myself. Mark it ready (or say so here) and I will merge — CI is 28/28 green. If you are still polishing, please do; this one changes worker exit semantics, so I would much rather have the version you consider finished than the one that happens to be green tonight. |
|
Thank you for marking this ready and for preserving the delivered MCP error instead of replacing it with a transport-level fallback. I have routed it at high priority in |
|
Thank you — this is exactly the right separation of layers: the process status speaks for transport health, the serialized payload speaks for the tool result, and conflating them is what made the supervisor invent 'crashed on a file' out of a perfectly delivered error. Tying worker exit 0 to the response actually being written (and hard-failing when the write itself fails) makes the protocol strictly more honest in both directions. The shell regression covering both the direct worker transport and the user-facing supervisor behavior is much appreciated — as is the honest note about the daemon blocking your local run. Great to have you as a repeat contributor! |
What changed
A supervised indexing worker now exits successfully when it has written a
complete MCP response, even when that response is an application-level tool
error. The parent can therefore read and return the real error instead of
replacing it with a generic
exit_nonzero/ "crashed on a file" diagnosis.Response-file open, write, or close failures still produce a nonzero worker
exit. A focused shell regression covers both the direct worker transport and
the user-facing supervisor behavior.
Why
The worker process status represents transport health, while the serialized MCP
payload represents tool success or failure. Reusing the CLI presentation exit
code for both layers caused the supervisor to discard valid error payloads.
This fixes the false-crash classification described in #1211 and complements
#1258, which addresses that issue's project-name-to-root-path resolution case.
Validation
CCACHE_DISABLE=1 ./scripts/build.sh— passedgit diff --check— passedbash -n tests/test_worker_error_response.sh— passedshellcheck tests/test_worker_error_response.sh— passedwas blocked before the assertion by an already-running daemon from a different
build cohort, which was deliberately not terminated
Checklist