Skip to content

fix: derive fetch url truncation label - #1017

Open
mrinalg297 wants to merge 1 commit into
Nano-Collective:mainfrom
mrinalg297:fix-fetch-url-size-label
Open

fix: derive fetch url truncation label#1017
mrinalg297 wants to merge 1 commit into
Nano-Collective:mainfrom
mrinalg297:fix-fetch-url-size-label

Conversation

@mrinalg297

@mrinalg297 mrinalg297 commented Aug 28, 2026

Copy link
Copy Markdown

Description

Brief description of what this PR does

Fix the hardcoded 100KB truncation warning in fetch-url by deriving the displayed size from MAX_URL_CONTENT_BYTES using the existing formatSize utility. Also updates the related formatter test to verify the dynamic value.

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Changeset

  • Added a changeset (pnpm changeset) describing this change for the changelog

Docs-only or internal chores need no changeset (or run pnpm changeset --empty to note that intentionally).

Testing

Automated Tests

  • [] New features include passing tests in .spec.ts/tsx files
  • All existing tests pass (pnpm test:all completes successfully)
  • Tests cover both success and error scenarios

Manual Testing

  • Tested with Ollama
  • Tested with OpenRouter
  • Tested with OpenAI-compatible API
  • Tested MCP integration (if applicable)

Checklist

  • If this was for an open issue, I was assigned to it
  • Code follows project style guidelines
  • Self-review completed
  • Documentation updated (if needed)
  • No breaking changes (or clearly documented)
  • Appropriate logging added using structured logging (see CONTRIBUTING.md)

@github-actions

Copy link
Copy Markdown
Contributor

No changeset found

This PR does not add a changeset, so it will not appear in the changelog or trigger a release.

If the change is user-facing, add one:

pnpm changeset

Pick a bump (patch / minor / major) and write the changelog entry in our usual voice ("Added X... Thanks to @you. Closes #123."), then commit the generated .changeset/*.md file.

If this PR is docs-only or a chore that needs no release note, you can ignore this - or run pnpm changeset --empty to record that intentionally.

@will-lamerton will-lamerton left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this. The instinct is right, the label shouldn't be hardcoded separately from the constant, but the chosen formatter changes the user-visible string in a way I don't think we want.

1. The rendered label becomes 97.7 KB

MAX_URL_CONTENT_BYTES is 100_000 (decimal, per the // ~100 KB comment), but formatSize divides by 1024, so the warning now reads ⚠ Content was truncated to 97.7 KB. Users never configured 97.7 KB, and the one-decimal figure implies a precision we don't have: the truncation at fetch-url.tsx:39 compares content.length (UTF-16 code units), not bytes.

Two options that both keep the value derived from the constant:

  • Say what the code actually measures: Content was truncated to ${MAX_URL_CONTENT_BYTES.toLocaleString()} characters. This also lines up with the handler's own note (original size was ${content.length} characters), which currently disagrees with the formatter's "KB" wording.
  • Or keep KB with the decimal divisor the constant was written in: ${MAX_URL_CONTENT_BYTES / 1000}KB, which renders the same 100KB as today.

2. The updated test no longer asserts anything

const expectedLabel = `Content was truncated to ${formatSize(MAX_URL_CONTENT_BYTES)}`;
t.regex(output!, new RegExp(expectedLabel.replace('.', '\\.')));

This builds the expectation from the same constant and the same function the source uses, so it passes for any value of either, including a broken formatSize. The old hardcoded assertion was the stronger test because it pinned the user-visible output. Please assert the literal expected string so a change to the constant surfaces as a failure someone has to consciously accept.

Also, String.replace with a string pattern only replaces the first match, so the escaping is incomplete by construction (it works here only because there's exactly one dot). Since no pattern matching is needed, t.true(output!.includes(expectedLabel)) is simpler and correct.

3. Missing changeset

The PR body has the changeset box checked, but the diff is only the two source files. This is a user-visible string change, so it needs one.

Happy to merge once the label stays round and honest about its unit, the test pins the literal output, and the changeset is added.

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