fix: Set final statusMessage before storeTaskResult (#638)#680
fix: Set final statusMessage before storeTaskResult (#638)#680jirispilka wants to merge 14 commits intomasterfrom
Conversation
storeTaskResult() has no statusMessage parameter, so the last intermediate progress message (e.g. "Starting the crawler.") persisted as the final statusMessage on completed/failed tasks. Call updateTaskStatus() with a descriptive final message right before each storeTaskResult() call. https://claude.ai/code/session_01ABYjPsZxtCdib54jfGcx3f
The third test asserted that a stale intermediate statusMessage is preserved through storeTaskResult — which tests InMemoryTaskStore internals, not the bug. The two remaining cases are sufficient regression coverage.
ad5d65e to
9a234ab
Compare
The MCP SDK's requestStream() only calls getTaskResult() for 'completed' tasks — 'failed' tasks yield a generic "Task X failed" error and discard the stored result. This broke x402 auto-pay retry (client never saw the payment payload) and lost error details for soft-fail results. - Revert task status from 'failed' back to 'completed' for all result paths - Add [error] prefix to statusMessage so tasks/list shows the real outcome - Add "… (truncated)" suffix when statusMessage is clipped to 200 chars - Document the workaround and upstream fixes needed in res/task_status_workaround.md Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
@claude can you review again |
There was a problem hiding this comment.
Pull request overview
Fixes stale statusMessage for terminal tasks by ensuring a final message is written immediately before persisting the task result, along with documenting (and testing) the SDK-driven workaround for result delivery.
Changes:
- Added
storeTaskResultWithMessage()helper to update finalstatusMessagethen store the result. - Updated
executeToolAndUpdateTask()to set outcome-specific final messages and to mark signal aborts ascancelled. - Added unit + integration regression tests and documentation for the “store errors as completed” workaround.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/mcp/utils.ts |
Adds storeTaskResultWithMessage() helper for final statusMessage + result persistence. |
src/mcp/server.ts |
Uses helper to set final messages; adjusts cancellation messaging and terminal handling. |
tests/unit/task.statusMessage.test.ts |
New unit regression coverage for final statusMessage behavior. |
tests/unit/server.task_execution.test.ts |
Verifies task-mode soft-fail payloads are stored/delivered. |
tests/integration/suite.ts |
Integration regression test ensuring success ends with : completed. |
res/task_status_workaround.md |
Documents why errors are stored as completed due to SDK requestStream() behavior. |
res/index.md |
Index entry for the new workaround doc. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot apply changes based on the comments in this thread |
Agent-Logs-Url: https://github.com/apify/apify-mcp-server/sessions/b6debe5d-50b7-4b92-9b85-8ab365c2df09 Co-authored-by: jirispilka <19406805+jirispilka@users.noreply.github.com>
Done in 558070b. I applied the thread feedback: narrowed |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.
Comments suppressed due to low confidence (1)
src/mcp/server.ts:1147
finishTaskTrackinglogstoolName: tool.name, while status messages and telemetry initialization usetoolFullName(canonical tool/actor name). This can make task-mode logs inconsistent with non-task mode (e.g. actor tools logging the encoded tool name rather thanapify/<actor>). Consider passingtoolFullNameintologToolCallAndTelemetryhere for consistency.
const finishTaskTracking = (status: ToolStatus, diagnostics?: CallDiagnostics) => {
this.logToolCallAndTelemetry({
toolName: tool.name,
mcpSessionId,
toolStatus: status,
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export async function storeTaskResultWithMessage( | ||
| taskStore: TaskStore, | ||
| taskId: string, | ||
| // Always 'completed' — the SDK's requestStream() only delivers results for 'completed' tasks; | ||
| // 'failed' tasks yield a generic error and discard the stored result. See res/task_status_workaround.md. | ||
| status: 'completed', | ||
| result: Result, |
There was a problem hiding this comment.
storeTaskResultWithMessage requires callers to pass a status: 'completed' argument even though the type only permits 'completed' and the function comment states it’s always completed. Dropping the parameter (or renaming it to something like completedStatus is unnecessary) would reduce call-site noise and prevent the API from looking more flexible than it is.
| process.exit(1); | ||
| } | ||
|
|
||
| log.setLevel(process.env.LOG_LEVEL === 'info' ? log.LEVELS.INFO : log.LEVELS.DEBUG); |
There was a problem hiding this comment.
This LOG_LEVEL handling only distinguishes 'info' vs “everything else → debug”. Since the repo already defines LOG_LEVEL_MAP (debug/info/notice/…/emergency), consider reusing it here so LOG_LEVEL=error (etc.) behaves as expected in dev mode, and default to a sensible level when unset.
| log.setLevel(process.env.LOG_LEVEL === 'info' ? log.LEVELS.INFO : log.LEVELS.DEBUG); | |
| const configuredLogLevel = (process.env.LOG_LEVEL ?? 'debug').trim().toUpperCase() as keyof typeof log.LEVELS; | |
| const resolvedLogLevel = configuredLogLevel in log.LEVELS | |
| ? log.LEVELS[configuredLogLevel] | |
| : log.LEVELS.DEBUG; | |
| log.setLevel(resolvedLogLevel); |
Summary
Fixes #638 —
tasks/listshows stalestatusMessage(e.g. "Starting the crawler.") for completed tasks becausestoreTaskResult()doesn't accept astatusMessageparameter.Changes:
storeTaskResultWithMessage()helper insrc/mcp/utils.ts— encapsulates the SDK workaround (updateTaskStatus+storeTaskResultin sequence)statusMessageper outcome inexecuteToolAndUpdateTask:"tool: completed""[error] tool: Actor not found…"(with[error]prefix)"[error] tool: payment required""[error] tool: payment required"+isTaskCancelledguard"[error] tool: error text… (truncated)"if >200 charscancelled(was incorrectly stored ascompletedwith empty{}result)"apify/rag-web-browser: cancelled by client")SDK workaround — all results stored as
'completed':The MCP SDK's
requestStream()only callsgetTaskResult()for'completed'tasks. For'failed'tasks it yields a generic"Task X failed"error and discards the stored result. This means:We work around this by storing all results as
'completed'and using[error]prefix instatusMessageto signal the real outcome. Full analysis inres/task_status_workaround.md.Verified with mcpc
isErrortasks-liststatusstatusMessagenullcompletedcall-actor: completedtruecompleted[error] call-actor: Actor not found…truecompleted[error] call-actor: Input validation failed…cancelledCancelled by clientcancelledapify/google-search-scraper: cancelled by clientFollow up issues
Test plan
npm run type-check— zero errorsnpm run lint— zero errorsnpm run test:unit— 488 tests passstatusMessage: "call-actor: completed"isError: true,statusMessagehas[error]prefixcancelledstatus with tool prefix when availablenpm run test:integration🤖 Generated with Claude Code