fix(mcp): keep typed dependency failures retryable through chat prevalidation - #426
fix(mcp): keep typed dependency failures retryable through chat prevalidation#426audichuang wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d03233b9f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (error instanceof LocalDaemonAvailabilityError) { | ||
| return batchFailure(error.code, error.message, error.retryable, item.label); |
There was a problem hiding this comment.
Keep retryable batch failures outside the finished operation
When local prevalidation returns DAEMON_BUSY, this stores the item as terminally failed; finishOperationWhenEveryItemIsTerminal then finishes the Operation, and findMatchingRetry returns that finished snapshot on every retry. This contradicts DAEMON_BUSY_MESSAGE, which instructs the caller to reuse the same operationId, so the prompt can never be retried after the daemon recovers. A retryable dependency failure must remain recoverable under that ID or abort before accepting the batch rather than becoming a terminal item.
Useful? React with 👍 / 👎.
…lidation session_create already rethrows LocalDaemonAvailabilityError from its prevalidation catch, and session_create_many already returns it per item with the producer's own code and retryability. The two chat entry points did not. Chat prevalidation raises both typed dependency errors, not just the sync one. validateSessionChatTarget calls syncWorkspaceMetaForRead, which raises WorkspaceSyncUnavailableError, and then ensureTargetMachineOnline, which for a target on the machine answering the MCP call routes through ensureLocalRuntimeAvailable -> dispatchLocalControl -> classifyLocalDaemonIpcError and raises LocalDaemonAvailabilityError with code DAEMON_BUSY and retryable: true. Only the first was passed through. The second was flattened into COMMAND_REJECTED (single) or INVALID_ITEM (batch), both with retryable: false, so the retryability the CLI had just computed was destroyed one frame later and a caller honouring `retryable` gave up on a condition that clears on its own. COMMAND_REJECTED also asserts the command was evaluated and refused on its merits, which is not what an unreachable local daemon is. Batch items carry the failure per item rather than aborting the batch, matching session_create_many for the same error type: daemon availability is per target machine, while a workspace sync failure is workspace-wide and still aborts. Untyped errors are deliberately left alone. Both the local-project Flock sync on the create path and the remote-target branch of ensureTargetMachineOnline throw plain Errors, so they still reach the COMMAND_REJECTED fallback; classifying those is the separate transport-classification question LodyAI#400 raises first. Refs LodyAI#400 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Model: claude-opus-5[1m]
9d03233 to
458525d
Compare
Related issue
Refs #400
Stated plainly so it is not discovered in review: this PR changes none of the
four outputs #400 reproduces. Defect 2's example payload comes from
syncMachineFlockDocsForRead(session.ts:979-991), which propagates the plainErrorthrown one frame deeper bysyncFlockDocOrThrow(
apps/cli/src/lib/loro/doc.ts:973-976) on thesession_createpath — untyped, so it still lands in theCOMMAND_REJECTEDfallback, and PR #411 is removing that producer at source for#398. A network outage also cannot produce the condition this PR fixes, because
LocalDaemonAvailabilityErrororiginates from local unix-socket IPC.What this PR does is apply defect 2's rule — an unavailable dependency must not
be reported as a command refusal — to the one chat prevalidation producer that
already carries typed retryability, bringing the chat catches in line with the
create ones. The Issue stays open.
Problem / pressure
session_create(lody-mcp-server.ts:2726) already rethrowsLocalDaemonAvailabilityError, andsession_create_many(:3253) alreadyreturns it per item with the producer's own code and retryability. The two chat
entry points did not, even though chat prevalidation raises it.
validateSessionChatTargethas two typed dependency-failure producers:syncWorkspaceMetaForRead(session.ts:3175) raisesWorkspaceSyncUnavailableErrorensureTargetMachineOnline(session.ts:3187) routes a target on the machineanswering the MCP call into
ensureLocalRuntimeAvailable→dispatchLocalControl→
classifyLocalDaemonIpcError, which returnsLocalDaemonAvailabilityErrorwith
DAEMON_BUSY/DAEMON_NOT_RUNNING/DAEMON_PROTOCOL_ERRORand theretryability that goes with each
Only the first was passed through. The second was flattened to
COMMAND_REJECTED(single) orINVALID_ITEM(batch), bothretryable: false,so a caller honouring
retryablegives up on a condition that clears on its own.COMMAND_REJECTEDalso states the command was evaluated and refused on itsmerits, which an unreachable local daemon is not.
Reachable triggers, since the obvious one is not the likely one: the daemon
exiting between the invocation-context RPC and
machine/status(
DAEMON_NOT_RUNNING, not retryable); a 5xx, a 408, a 429, or a stalledmachine/statuswhile other RPCs answer (DAEMON_BUSY, retryable); or anon-retryable protocol answer — any other 4xx, or a 2xx whose body fails
validation (
DAEMON_PROTOCOL_ERROR). A daemonunresponsive for the whole call never reaches this frame —
readActiveInvocationContextconverts its
IpcTimeoutErrorinto a plainErrorone frame earlier and itsurfaces as
INTERNAL_ERROR, which is #400's defect 1 and out of scope here.Summary
startSessionChatOperation: rethrowLocalDaemonAvailabilityErroralongsidethe existing
WorkspaceSyncUnavailableErrorpassthrough.mcpErrorResultalready maps it via
toLodyError, so no mapping change was needed.startSessionChatManyOperation: return the failure on the item viabatchFailure(error.code, error.message, error.retryable, item.label)—identical to what
session_create_manyalready does for this type. Daemonavailability is per target machine; a workspace sync failure is workspace-wide
and still aborts the batch.
boundary of both entry points.
Untyped errors are deliberately left alone — see Deliberately not done.
Before / after
lody_session_chatwhen the daemon has exited, stallsmachine/status, or answers a protocol error:COMMAND_REJECTED,retryable: falseDAEMON_NOT_RUNNING/DAEMON_BUSY/DAEMON_PROTOCOL_ERROR) with its ownretryablelody_session_chat_manymarks the itemINVALID_ITEM,retryable: falseretryableretryableabandons a transient local-daemon conditionTest plan
pnpm --filter lody run typecheck— passed.cd apps/cli && pnpm exec vitest run --no-file-parallelism— 250 files, 2495tests passed, 1 skipped, 0 failed.
cd apps/cli && pnpm exec vitest run src/mcp/lody-mcp-server-chat-sync.test.ts— 5 passed.
git show origin/main:apps/cli/src/mcp/lody-mcp-server.ts > apps/cli/src/mcp/lody-mcp-server.ts,re-run the test file, and both new tests fail reporting
COMMAND_REJECTEDandINVALID_ITEM;git checkout HEAD -- apps/cli/src/mcp/lody-mcp-server.tsrestores them. Verified.
pnpm lint— 0 errors, and the warning count is identical on this branch and onits merge base (9822 locally on both). The absolute number differs between a local
run and CI's clean checkout; the delta is what this claims.
pnpm check:public-boundary— passed (4111 files, 22 manifests on this branch;mainhas grown since, so a post-rebase run reports a larger count).pnpm --filter lody run format— no changes outside the two edited files.Not run: no live reproduction. The producer chain was established by reading
origin/main; the tests inject the error type that chain returns rather thanprovoking it end to end.
Context handoff
Instructions for reviewing agents
lody-mcp-server.ts.The new branch is reachable because
validateSessionChatTargetcallsensureTargetMachineOnlineunconditionally (session.ts:3187), unlike the createsites which pass
skipMachineAvailabilityCheck: true.aborting — copied from
session_create_many:3253, but a batch whose items alltarget this machine now yields N identical failures. (2) Leaving untyped errors in
the
COMMAND_REJECTEDfallback instead of widening the fix.is narrow, and the more common local-daemon failure surfaces earlier as
INTERNAL_ERRORthroughreadActiveInvocationContext.Authoring context
reviewable change, and avoid scope that would need a maintainer design decision.
apps/cli/src/commands/session.ts—PR fix(cli): allow local session creation on stale Flock sync #411 is rewriting the local-project resolution call sites there for [Bug] Session create hard-fails on a best-effort Flock freshness sync when the network is degraded #398, with a
different resolution (make the freshness sync best-effort) than reclassifying. No
new error codes, no shared schema changes, no transport-level classification.
retryability instead of
INVALID_ITEM/false. No shape change — the item alreadyfailed with
INVALID_ITEM; onlycodeandretryablediffer.change; failed batch items were already persisted as failures.
producers. Defect 1 needs a decision about where transport failures get classified —
listWorkspacesForToken,postSessionControl, or thenormalizeMcpErrorfunnel —and whether a dependency-unavailable code should exist; the same decision governs the
plain
Errors from the local-project Flock sync and from the remote-target branch ofensureTargetMachineOnline(session.ts:1867), both of which this PR leaves in thefallback. Defects 3 and 4 turn on whether
WorkspaceSyncUnavailableError.toLodyError()should return
this.messagerather than the shared constant — today it discards theinstance message, including the
--offlinehint built inbuildOfflineHint. All ofthese look like maintainer calls rather than something to fold in here.
to consumer and independently re-derived before opening. Lower on how much the fixed
frame matters in practice, which is why the reachable triggers are spelled out above
rather than implied.