Fix execution log download URL schema#349
Conversation
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
Co-authored-by: Rohan Gupta <thisrohangupta@users.noreply.github.com>
|
|
There was a problem hiding this comment.
Important
src/tools/harness-get.ts:32,src/registry/toolsets/logs.ts:16-28,src/tools/harness-list.ts:19-23,tests/tools/tool-handlers.test.ts:436-460: This fixes the strict-client stripping bug, but it still leaves theexecution_logcontract mis-modeled.return_download_urlis now documented only on the globalharness_getschema, whileexecution_logstill has noget.paramsSchemaand still publishesexecution_idvialistFilterFieldseven though it is get-only. Soharness_describe(resource_type="execution_log")still will not surfaceprefix/execution_id/return_download_url, andharness_listcontinues advertisingexecution_idas a global list filter for a non-listable resource. The new tests only lock the global tool schema/runtime branch, so this discovery-contract drift still passes CI. Please move these fields intoexecution_log.operations.get.paramsSchema, remove thelistFilterFieldsentry, and add a focusedharness_describe/ filter-catalog regression.
Sent by Cursor Automation: Sunil On Demand Architecture Review
| resource_scope: z.enum(["account", "org", "project"]).optional().describe("Scope to query. Use account for account-level resources and to omit org/project defaults; org injects only org; project injects org+project. Auto-detected from url."), | ||
| org_id: z.string().describe("Organization identifier (overrides default)").optional(), | ||
| project_id: z.string().describe("Project identifier (overrides default)").optional(), | ||
| return_download_url: z.boolean().optional().describe("For execution_log, return a signed log download URL instead of downloading log content."), |
There was a problem hiding this comment.
This closes the strict-client stripping bug, but it still leaves the execution_log discovery contract split across the wrong surfaces. harness_describe(resource_type="execution_log") still will not advertise prefix, execution_id, or return_download_url, because src/registry/toolsets/logs.ts has no get.paramsSchema for them and still misfiles execution_id under listFilterFields even though execution_log has no list op. That also leaks execution_id into harness_list's global filter catalog.
Per the architecture rules, get-only inputs should live on the get operation's paramsSchema, not on the generic tool schema / list-filter metadata. Please add an execution_log.operations.get.paramsSchema for prefix, execution_id, and return_download_url, remove the listFilterFields entry, and add a focused harness_describe regression so this metadata cannot drift again.
There was a problem hiding this comment.
Important
src/tools/harness-get.ts:32,src/registry/toolsets/logs.ts:16-28,src/tools/harness-list.ts:22-23,tests/tools/tool-handlers.test.ts:436-460: The runtime strict-client fix looks right, but the public discovery contract is still split across two places.return_download_urlis now documented only on the globalharness_getschema, whileexecution_logis still a get-only resource withexecution_idexposed vialistFilterFieldsand nooperations.get.paramsSchema. That meansharness_describe(resource_type="execution_log")still omitsreturn_download_urlfrom the operation metadata, andharness_listcontinues advertisingexecution_idas a global list filter for a non-listable resource. This breaks the “metadata matches operations” / “docs match implementation” rules and the new tests will not catch it because they only cover the global tool schema/runtime branch.
Open questions / assumptions
- Assuming
execution_logis intentionally get-only andharness_describeremains the source of truth for resource-specific input discovery.
Change summary
- The strict-client
return_download_urlregression itself appears fixed; the remaining gap is resource metadata alignment and focused contract coverage.
Sent by Cursor Automation: Sunil On Demand Architecture Review
| resource_scope: z.enum(["account", "org", "project"]).optional().describe("Scope to query. Use account for account-level resources and to omit org/project defaults; org injects only org; project injects org+project. Auto-detected from url."), | ||
| org_id: z.string().describe("Organization identifier (overrides default)").optional(), | ||
| project_id: z.string().describe("Project identifier (overrides default)").optional(), | ||
| return_download_url: z.boolean().optional().describe("For execution_log, return a signed log download URL instead of downloading log content."), |
There was a problem hiding this comment.
Good fix for the strict-client stripping path. One remaining architecture issue: this adds return_download_url only to the global harness_get schema, but execution_log is still modeled as a get-only resource with execution_id in listFilterFields and no operations.get.paramsSchema in src/registry/toolsets/logs.ts. That leaves harness_describe(resource_type='execution_log') without the get-param contract for execution_id / return_download_url, and it keeps polluting harness_list's global filter catalog with a get-only field. Please move those get-only inputs into get.paramsSchema, drop the listFilterFields entry, and add a focused describe/filter-catalog regression.


Description
Fixes a critical schema-surface bug in
harness_getfor execution logs. Theexecution_loghandler supportedreturn_download_url=true, but the top-levelharness_getschema did not advertise that field. Strict MCP clients could strip the flag before dispatch, causing URL-only log requests to fall back to full log download and buffering. For large logs with missing or unreliableContent-Length, that can reintroduce memory exhaustion risk.This PR adds
return_download_urlto the registeredharness_getinput schema and adds focused coverage that simulates schema-driven clients forwarding only registered fields.Type of Change
Checklist
Validation performed:
pnpm exec vitest run tests/tools/tool-handlers.test.ts -t "harness_get — execution_log"pnpm build && pnpm docs:generate && pnpm typecheck && pnpm docs:check && pnpm test