Skip to content

Add opt-in data fields to get_document_content - #9

Merged
jaowcz merged 2 commits into
mainfrom
jaowcz/fetch-document-data-fields
Oct 12, 2026
Merged

jaowcz merged 2 commits into
mainfrom
jaowcz/fetch-document-data-fields

Conversation

@jaowcz

@jaowcz jaowcz commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

get_document_content gets an include_data_fields flag that also fetches the document's data fields from GET v3/documents/:id, in parallel with the markdown. If only that request fails or takes more than 10s after the text arrives, the tool still returns the text, with data_fields: null and a data_fields_error. The data fields request is cancelled as soon as the markdown request fails, and MCP client cancellation aborts both requests. Shared changes: get_template uses the same data field formatter (so it now includes formatting, passed through as-is), all tools share errorMessage, non-JSON HTTP error bodies are capped at 500 characters and error responses keep their status when the body is malformed JSON, and a caller's abort signal now combines with the 30s timeout instead of replacing it. Tests cover partial failure, cancellation, the timeout and malformed responses, with new hang/body options and unused-route tracking in the fakeFetch helper.

🤖 Generated with Claude Code

get_document_content accepts include_data_fields, which fetches the
document's data fields from GET v3/documents/:id alongside the markdown.
If only the data fields request fails or times out (10s after the content
arrives), the text is still returned with data_fields: null and
data_fields_error. The data fields request is cancelled as soon as the
markdown request fails, and both are cancelled when the MCP client cancels.

Also:
- share the data field formatter with get_template, which now outputs
  number formatting too
- share errorMessage across tools
- truncate HTTP error bodies to 500 characters and keep the HTTP status
  when an error response has a malformed JSON body
- combine a caller-supplied signal with the 30s request timeout instead
  of replacing it
- treat a markdown response that is not a document as an error rather
  than empty content

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@dkanas

dkanas commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

A few things before merging:

1. formatting drops the date format (src/tools/shared.ts:57)
The allowlist keeps only grouping/fractional, but date fields store formatting: {date: "D MMM YYYY"}, so they come out as formatting: {}. Pass formatting through as-is instead of picking keys, and add date?: string | null to TemplateDataFieldFormatting. Formatting is display-only: the API takes and returns raw values (dates must pass Date.from_iso8601, and markdown emits the raw value). In both the get_document_content and get_template descriptions, replace the "formatting: number display settings…" text with: "Values are raw: dates are YYYY-MM-DD, numbers are plain. formatting is how Contractbook displays them; use it when showing a value to the user, never when setting one." Add a date field to the formatting test cases.

2. Error truncation hits validation errors (src/contractbook/base-client.ts:32)
The 500-char cap applies to every tool. A 422 from create_document_from_template listing several bad fields gets cut before the field names, and the model can't fix its input. Truncate only non-JSON bodies (HTML error pages), which is the case this was meant for.

3. Grace timeout / cancellation (src/tools/get-document-content.ts:81)

  • The 30s HttpClient timeout starts with the request, so the real grace is min(10s after content, 30s from start). Keep the behavior; just reword the DATA_FIELDS_GRACE_MS description to say it's capped by the request timeout.
  • If the client cancels after the content has arrived, the handler still returns a normal result with data_fields_error: "This operation was aborted". After awaiting dataFieldsRequest, if cancelled?.aborted, throw cancelled.reason instead of building a result. Add a test: content resolves, data fields hang, cancel, expect a rejection.

4. Code comments
New comments in get-document-content.ts (11, 42, 111), client.ts (239, 272–273), base-client.ts (81), and JSDoc in tests/helpers/fake-fetch.ts. We don't add these unless asked; please remove them.

5. ToolContext duplicates the SDK type (src/tools/get-document-content.ts:43)
@modelcontextprotocol/server exports ServerContext with mcpReq.signal. With the local all-optional type, a rename in a later beta still compiles and cancellation silently stops working. Type the handler's ctx as ServerContext (optional) and delete ToolContext. In the tests, build the context with a cast ({ mcpReq: { signal } } as unknown as ServerContext), since the real type has required fields.

6. Spread instead of Object.assign (src/tools/get-document-content.ts:81)
const result: Record<string, unknown> = content plus Object.assign mutates content and widens it to Record<string, unknown>, losing its type. Drop result and serialize { ...content, ...formatDataFields(await dataFieldsRequest) } when data fields were requested, content otherwise. Don't annotate it; let TypeScript infer the type from formatContent and formatDataFields. The spread copies only top-level references, so there's no meaningful memory cost.

- pass data field formatting through as-is (keeps date formats) and tell
  the model values are raw and formatting is display-only
- truncate only non-JSON HTTP error bodies so validation errors stay whole
- reject instead of returning a result when the MCP client cancels while
  waiting for data fields
- type the handler context as the SDK's ServerContext
- build the result with a spread instead of mutating the content
- remove code comments added in this PR

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@jaowcz

jaowcz commented Oct 2, 2026

Copy link
Copy Markdown
Contributor Author

Thanks, addressed in eb0b31e:

  1. formatting: passed through as-is, date added to the type, and both tool descriptions now use your wording ("Values are raw: … never when setting one."). The get_template and get_document_content tests include a date field with formatting: {date: "D MMM YYYY"}.
  2. Truncation: only string (non-JSON) bodies are truncated now. New test: a 422 with 30 field errors from create_document_from_template comes through whole.
  3. Grace / cancellation: reworded the DATA_FIELDS_GRACE_MS comment to say the request's 30s timeout still applies. If the client cancels after the content arrives, the handler now throws cancelled.reason. New test: content resolves, data fields hang, cancel, expect rejection.
  4. Comments: removed all the ones you listed plus the JSDoc in fake-fetch.ts and the test helper. The only one left is the DATA_FIELDS_GRACE_MS comment, since point 3 asked for it to be reworded. Happy to drop it if you'd rather not keep it.
  5. ServerContext: ToolContext is gone; ctx?: ServerContext, and the tests build it with the cast you suggested.
  6. Spread: no more result/Object.assign; the response is { ...content, ...formatDataFields(dataFields) } when data fields were requested, content otherwise, with inferred types.

Typecheck, lint, format and all 87 tests pass. I also checked that removing the new cancellation throw makes its test fail.

@jaowcz
jaowcz merged commit 3cb9cb9 into main Oct 12, 2026
1 check passed
@jaowcz
jaowcz deleted the jaowcz/fetch-document-data-fields branch October 12, 2026 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants