Merge pull request #1004 from MyGO-Mujica/fix/smartfetch-strip-fragment-cache-keys

fix(smartfetch): strip URL fragments from cache keys and fetch URLs (#1003)

src/tools/smartfetch/cache.test.ts

@@ -34,6 +34,85 @@ describe('smartfetch/cache', () => {
     });
   });
 
+  test('URL fragments are not part of the cache key (RFC 3986)', () => {
+    const noFragment = buildCacheKey(
+      'https://example.com/docs',
+      true,
+      'auto',
+      false,
+    );
+    const sec1 = buildCacheKey(
+      'https://example.com/docs#sec1',
+      true,
+      'auto',
+      false,
+    );
+    const sec2 = buildCacheKey(
+      'https://example.com/docs#sec2',
+      true,
+      'auto',
+      false,
+    );
+    const emptyFragment = buildCacheKey(
+      'https://example.com/docs#',
+      true,
+      'auto',
+      false,
+    );
+
+    expect(sec1).toBe(noFragment);
+    expect(sec2).toBe(noFragment);
+    expect(emptyFragment).toBe(noFragment);
+  });
+
+  test('query strings still distinguish cache keys', () => {
+    const page1 = buildCacheKey(
+      'https://example.com/docs?page=1#x',
+      true,
+      'auto',
+      false,
+    );
+    const page2 = buildCacheKey(
+      'https://example.com/docs?page=2#x',
+      true,
+      'auto',
+      false,
+    );
+
+    expect(page1).not.toBe(page2);
+  });
+
+  test('option changes still produce distinct cache keys', () => {
+    const base = buildCacheKey(
+      'https://example.com/docs#sec1',
+      true,
+      'auto',
+      false,
+    );
+    const noExtract = buildCacheKey(
+      'https://example.com/docs#sec1',
+      false,
+      'auto',
+      false,
+    );
+    const alwaysLlms = buildCacheKey(
+      'https://example.com/docs#sec1',
+      true,
+      'always',
+      false,
+    );
+    const saveBinary = buildCacheKey(
+      'https://example.com/docs#sec1',
+      true,
+      'auto',
+      true,
+    );
+
+    expect(noExtract).not.toBe(base);
+    expect(alwaysLlms).not.toBe(base);
+    expect(saveBinary).not.toBe(base);
+  });
+
   test('llms.txt-shaped result is charged once for its content', () => {
     const llmsTxt = Array.from(
       { length: 64 },

src/tools/smartfetch/cache.ts

@@ -30,6 +30,9 @@ export function buildCacheKey(
   saveBinary: boolean,
 ) {
   const parsed = new URL(url);
+  // Fragments never reach the server (RFC 3986 §3.5); #sec1 and #sec2 are
+  // the same document, so they must share one cache entry.
+  parsed.hash = '';
   return JSON.stringify({
     url: parsed.toString(),
     extractMain,

src/tools/smartfetch/network.test.ts

@@ -3,6 +3,7 @@ import {
   buildAllowedOrigins,
   buildConditionalHeaders,
   fetchWithRedirects,
+  normalizeUrl,
 } from './network';
 
 describe('smartfetch/network', () => {
@@ -13,6 +14,33 @@ describe('smartfetch/network', () => {
     mock.restore();
   });
 
+  test('normalizeUrl strips the fragment from fetch URLs only', () => {
+    const normalized = normalizeUrl('https://example.com/docs#sec1');
+
+    expect(normalized.url).toBe('https://example.com/docs');
+    expect(normalized.originalUrl).toBe('https://example.com/docs#sec1');
+    expect(normalized.fallbackUrl).toBeUndefined();
+    expect(normalized.upgradedToHttps).toBe(false);
+  });
+
+  test('normalizeUrl strips fragments from the http fallback URL too', () => {
+    const normalized = normalizeUrl('http://example.com/docs#sec1');
+
+    expect(normalized.url).toBe('https://example.com/docs');
+    expect(normalized.originalUrl).toBe('http://example.com/docs#sec1');
+    expect(normalized.fallbackUrl).toBe('http://example.com/docs');
+    expect(normalized.upgradedToHttps).toBe(true);
+  });
+
+  test('normalizeUrl keeps origin and query string while dropping the fragment', () => {
+    const normalized = normalizeUrl(
+      'https://example.com/docs?page=2#anchor',
+    );
+
+    expect(normalized.url).toBe('https://example.com/docs?page=2');
+    expect(new URL(normalized.url).origin).toBe('https://example.com');
+  });
+
   test('collects unique allowed origins from permission patterns', () => {
     const origins = [
       ...buildAllowedOrigins([

src/tools/smartfetch/network.ts

@@ -27,10 +27,19 @@ export function normalizeUrl(input: string): {
   let upgradedToHttps = false;
   let fallbackUrl: string | undefined;
   if (parsed.protocol === 'http:') {
-    fallbackUrl = originalUrl;
+    fallbackUrl = parsed.toString();
     parsed.protocol = 'https:';
     upgradedToHttps = true;
   }
+  // Fragments never reach the server (RFC 3986 §3.5); strip them from the
+  // URLs actually fetched so the same document requested with different
+  // anchors issues a single request. originalUrl keeps the fragment.
+  parsed.hash = '';
+  if (fallbackUrl) {
+    const fallback = new URL(fallbackUrl);
+    fallback.hash = '';
+    fallbackUrl = fallback.toString();
+  }
   return { url: parsed.toString(), upgradedToHttps, fallbackUrl, originalUrl };
 }
 

src/tools/smartfetch/tool.test.ts

@@ -1,4 +1,5 @@
 import { afterEach, describe, expect, mock, test } from 'bun:test';
+import { CACHE } from './cache';
 import { createWebfetchTool } from './tool';
 
 function createExecutionContext() {
@@ -15,6 +16,7 @@ describe('smartfetch/tool', () => {
 
   afterEach(() => {
     globalThis.fetch = originalFetch;
+    CACHE.clear();
     mock.restore();
   });
 
@@ -57,4 +59,65 @@ describe('smartfetch/tool', () => {
     expect(ctx.ask).toHaveBeenCalledTimes(1);
     expect(ctx.metadata).not.toHaveBeenCalled();
   });
+
+  test('same document with different fragments issues a single request', async () => {
+    CACHE.clear();
+
+    const fetchMock = mock(async (input: string | URL | Request) => {
+      const url = typeof input === 'string' ? input : input.toString();
+
+      if (url === 'https://example.com/docs') {
+        return new Response('document body', {
+          status: 200,
+          headers: { 'content-type': 'text/plain' },
+        });
+      }
+
+      throw new Error(`Unexpected fetch URL: ${url}`);
+    });
+    globalThis.fetch = fetchMock as unknown as typeof fetch;
+
+    const webfetch = createWebfetchTool({ client: {} } as any);
+
+    const firstCtx = createExecutionContext();
+    const firstResult = await webfetch.execute(
+      {
+        url: 'https://example.com/docs#sec1',
+        format: 'markdown',
+        extract_main: true,
+        prefer_llms_txt: 'auto',
+        include_metadata: true,
+        save_binary: false,
+      },
+      firstCtx,
+    );
+
+    expect(fetchMock).toHaveBeenCalledTimes(1);
+    expect(firstResult).toContain(
+      'requested_url: "https://example.com/docs#sec1"',
+    );
+    expect(firstResult).toContain('cache_hit: false');
+
+    const secondCtx = createExecutionContext();
+    const secondResult = await webfetch.execute(
+      {
+        url: 'https://example.com/docs#sec2',
+        format: 'markdown',
+        extract_main: true,
+        prefer_llms_txt: 'auto',
+        include_metadata: true,
+        save_binary: false,
+      },
+      secondCtx,
+    );
+
+    // The fragment-stripped cache key collides with the first request, so
+    // the second fetch is served from cache without hitting the network,
+    // and the reported requested_url stays the URL of the current request.
+    expect(fetchMock).toHaveBeenCalledTimes(1);
+    expect(secondResult).toContain('cache_hit: true');
+    expect(secondResult).toContain(
+      'requested_url: "https://example.com/docs#sec2"',
+    );
+  });
 });