fixup! fix(cloudflare): Filter CREATE INDEX spans on cf_-prefixed tables

JPeer264 · JPeer264 · commit de796113ffdf · 2026-07-27T16:15:35.000+02:00
diff --git a/packages/cloudflare/test/instrumentSqlStorage.test.ts b/packages/cloudflare/test/instrumentSqlStorage.test.ts
@@ -159,31 +159,129 @@ describe('instrumentSqlStorage', () => {
       expect(result).toBe(mockCursor);
     });
 
-    it('still creates a span for user queries', () => {
-      const startSpanSpy = vi.spyOn(sentryCore, 'startSpan');
-      const mockSql = createMockSqlStorage();
-      const instrumented = instrumentSqlStorage(mockSql);
+    describe('internal tables (cf_ prefix) are skipped', () => {
+      it.each([
+        ['SELECT', 'SELECT * FROM cf_agents_state WHERE id = ?'],
+        ['INSERT', 'INSERT INTO cf_agents_fibers (id, callback) VALUES (?, ?)'],
+        ['DELETE', 'DELETE FROM cf_agents_schedules WHERE id = ?'],
+        ['UPDATE', 'UPDATE cf_agent_tool_runs SET output_json = ? WHERE id = ?'],
+        ['CREATE TABLE', 'CREATE TABLE IF NOT EXISTS cf_agents_workflows (id TEXT PRIMARY KEY NOT NULL)'],
+        ['ALTER TABLE', 'ALTER TABLE cf_agents_queues ADD COLUMN retry_options TEXT'],
+        ['DROP TABLE', 'DROP TABLE cf_agents_state'],
+        ['cf_agent_ prefix', 'SELECT * FROM cf_agent_identity'],
+        ['cf_ai_ prefix', 'INSERT INTO cf_ai_chat_stream_chunks (id) VALUES (?)'],
+        ['cf_mcp_ prefix', 'SELECT * FROM cf_mcp_agent_event'],
+        ['schema version', 'SELECT version FROM cf_schema_version'],
+        // SQLite upsert forms used by the agents framework for state/schedule/MCP persistence
+        ['INSERT OR REPLACE', 'INSERT OR REPLACE INTO cf_agents_state (id, state) VALUES (?, ?)'],
+        [
+          'INSERT OR REPLACE with column list',
+          `INSERT OR REPLACE INTO cf_agents_mcp_servers ( id, name, server_url, client_id, auth_url,
+            callback_url, server_options )
+          VALUES ( ?, ?, ?, ?, ?, ?, ? )`,
+        ],
+        ['INSERT OR IGNORE', 'INSERT OR IGNORE INTO cf_agents_sub_agents (class, name) VALUES (?, ?)'],
+        ['REPLACE INTO', 'REPLACE INTO cf_agents_queues (id, payload) VALUES (?, ?)'],
+        ['UPDATE OR REPLACE', 'UPDATE OR REPLACE cf_agents_state SET state = ? WHERE id = ?'],
+        // The summary of a CREATE INDEX carries the index name, not the indexed table — the cf_
+        // target only exists in the ON clause of the full statement.
+        [
+          'CREATE INDEX (framework statement)',
+          `create index if not exists idx_ai_chat_agent_tool_request_id
+            on cf_ai_chat_agent_tool_runs(request_id)`,
+        ],
+        ['CREATE INDEX (uppercase)', 'CREATE INDEX idx_agents_state_id ON cf_agents_state (id)'],
+        ['CREATE UNIQUE INDEX', 'CREATE UNIQUE INDEX idx_agents_state_id ON cf_agents_state (id)'],
+        [
+          'CREATE INDEX (without IF NOT EXISTS)',
+          'CREATE INDEX idx_chunks_stream ON cf_ai_chat_stream_chunks (stream_id)',
+        ],
+        [
+          'JOIN between internal tables',
+          `SELECT f.fiber_id, f.status
+             FROM cf_agents_fibers f
+             LEFT JOIN cf_agents_runs r ON r.id = f.fiber_id
+             WHERE f.status IN ('pending', 'running')`,
+        ],
+        // `.some()` — any internal table present means the query is framework-driven noise.
+        ['JOIN with a user table', 'SELECT * FROM cf_agents_state s JOIN users u ON u.id = s.id'],
+        ['lowercase keywords and prefix', 'select * from CF_AGENTS_STATE'],
+      ])('skips %s', (_label, query) => {
+        expect(execCreatesSpan(query)).toBe(false);
+      });
+    });
 
-      instrumented.exec('SELECT * FROM users WHERE id = ?', 1);
+    describe('user queries stay instrumented', () => {
+      it.each([
+        ['SELECT', 'SELECT * FROM users WHERE id = ?'],
+        ['INSERT', 'INSERT INTO orders (id, total) VALUES (?, ?)'],
+        ['UPDATE', 'UPDATE products SET price = ? WHERE id = ?'],
+        ['DELETE', 'DELETE FROM sessions WHERE expired = 1'],
+        ['CREATE TABLE', 'CREATE TABLE users (id TEXT PRIMARY KEY)'],
+        ['CREATE INDEX', 'CREATE INDEX idx_name ON users (name)'],
+        ['table with cf in the middle', 'SELECT * FROM my_cf_table'],
+        ['table starting with cfg', 'SELECT * FROM cfg_settings'],
+        ['INSERT OR REPLACE', 'INSERT OR REPLACE INTO users (id, name) VALUES (?, ?)'],
+        ['REPLACE INTO', 'REPLACE INTO sessions (id, token) VALUES (?, ?)'],
+        ['UPDATE OR IGNORE', 'UPDATE OR IGNORE products SET price = ? WHERE id = ?'],
+        // No resolvable table target — safe default is to instrument.
+        ['no-table SELECT', 'SELECT 1'],
+        ['PRAGMA', 'PRAGMA foreign_keys = ON'],
+        ['bare operation', 'BEGIN'],
+        ['empty query', ''],
+      ])('instruments %s', (_label, query) => {
+        expect(execCreatesSpan(query)).toBe(true);
+      });
+    });
 
-      expect(startSpanSpy).toHaveBeenCalledTimes(1);
+    describe('durableObjectSqlSpanAllowlist (opt a cf_ table back into instrumentation)', () => {
+      it.each([
+        ['exact string', 'SELECT * FROM cf_my_table', ['cf_my_table']],
+        ['regex', 'SELECT * FROM cf_reports_daily', [/^cf_reports_/]],
+        ['upsert target', 'INSERT OR REPLACE INTO cf_my_table (id) VALUES (?)', ['cf_my_table']],
+        ['CREATE INDEX target', 'CREATE INDEX idx_mine ON cf_my_table (id)', ['cf_my_table']],
+      ])('instruments an allowlisted table matched by %s', (_label, query, allowlist) => {
+        expect(execCreatesSpan(query, allowlist)).toBe(true);
+      });
+
+      it.each([
+        // Substring matches must not opt a table back in, otherwise `cf_` would allowlist everything.
+        ['a string entry only matches exactly', 'SELECT * FROM cf_agents_state', ['cf_agents']],
+        ['a non-matching entry leaves internal tables skipped', 'SELECT * FROM cf_agents_state', ['cf_my_table']],
+        [
+          'an internal table joined with an allowlisted table is still skipped',
+          'SELECT * FROM cf_my_table t JOIN cf_agents_state s ON s.id = t.id',
+          ['cf_my_table'],
+        ],
+        ['an empty allowlist is ignored', 'SELECT * FROM cf_agents_state', []],
+      ])('%s', (_label, query, allowlist) => {
+        expect(execCreatesSpan(query, allowlist)).toBe(false);
+      });
     });
+  });
+});
 
-    it('creates a span for a cf_ table on the durableObjectSqlSpanAllowlist', () => {
-      const startSpanSpy = vi.spyOn(sentryCore, 'startSpan');
-      vi.spyOn(sentryCore, 'getClient').mockReturnValue({
-        getOptions: () => ({ durableObjectSqlSpanAllowlist: ['cf_my_table'] }),
-      } as unknown as ReturnType<typeof sentryCore.getClient>);
+/**
+ * Runs a query through the real `instrumentSqlStorage` proxy and reports whether it produced a
+ * `db.query` span, so the filtering matrix exercises the actual code path rather than a
+ * reimplementation of it.
+ */
+function execCreatesSpan(query: string, allowlist?: Array<string | RegExp>): boolean {
+  const startSpanSpy = vi.spyOn(sentryCore, 'startSpan');
 
-      const mockSql = createMockSqlStorage();
-      const instrumented = instrumentSqlStorage(mockSql);
+  if (allowlist) {
+    vi.spyOn(sentryCore, 'getClient').mockReturnValue({
+      getOptions: () => ({ durableObjectSqlSpanAllowlist: allowlist }),
+    } as unknown as ReturnType<typeof sentryCore.getClient>);
+  }
 
-      instrumented.exec('SELECT * FROM cf_my_table WHERE id = ?', 1);
+  const mockSql = createMockSqlStorage();
+  instrumentSqlStorage(mockSql).exec(query);
 
-      expect(startSpanSpy).toHaveBeenCalledTimes(1);
-    });
-  });
-});
+  expect(mockSql.exec).toHaveBeenCalledWith(query);
+
+  return startSpanSpy.mock.calls.length > 0;
+}
 
 function createMockCursor() {
   return {
diff --git a/packages/cloudflare/test/utils/internalSqlQuery.test.ts b/packages/cloudflare/test/utils/internalSqlQuery.test.ts
@@ -1,145 +1,25 @@
-import { _INTERNAL_getSqlQuerySummary, _INTERNAL_sanitizeSqlQuery } from '@sentry/core';
 import { describe, expect, it } from 'vitest';
 import { targetsCloudflareInternalTable } from '../../src/utils/internalSqlQuery';
 
-// Runs the same sanitize -> summarize -> filter pipeline as `instrumentSqlStorage`, so the tests
-// exercise the real detection path rather than hand-written summaries.
-const check = (query: string, allowlist?: Array<string | RegExp>): boolean => {
-  const sanitized = _INTERNAL_sanitizeSqlQuery(query);
-  return targetsCloudflareInternalTable(_INTERNAL_getSqlQuerySummary(sanitized), allowlist, sanitized);
-};
-
+// Behavioural coverage of the filter lives in `instrumentSqlStorage.test.ts`, which drives real
+// queries through the instrumented `exec`. What remains here are the signature-level contracts that
+// call path cannot reach: an absent summary, and an absent `queryText`.
 describe('targetsCloudflareInternalTable', () => {
-  describe('internal queries (cf_ tables)', () => {
-    it.each([
-      ['SELECT', 'SELECT * FROM cf_agents_state WHERE id = ?'],
-      ['INSERT', 'INSERT INTO cf_agents_fibers (id, callback) VALUES (?, ?)'],
-      ['DELETE', 'DELETE FROM cf_agents_schedules WHERE id = ?'],
-      ['UPDATE', 'UPDATE cf_agent_tool_runs SET output_json = ? WHERE id = ?'],
-      ['CREATE TABLE', 'CREATE TABLE IF NOT EXISTS cf_agents_workflows (id TEXT PRIMARY KEY NOT NULL)'],
-      ['ALTER TABLE', 'ALTER TABLE cf_agents_queues ADD COLUMN retry_options TEXT'],
-      ['DROP TABLE', 'DROP TABLE cf_agents_state'],
-      ['cf_agent_ prefix', 'SELECT * FROM cf_agent_identity'],
-      ['cf_ai_ prefix', 'INSERT INTO cf_ai_chat_stream_chunks (id) VALUES (?)'],
-      ['cf_mcp_ prefix', 'SELECT * FROM cf_mcp_agent_event'],
-      ['schema version', 'SELECT version FROM cf_schema_version'],
-      // SQLite upsert forms used by the agents framework for state/schedule/MCP persistence
-      ['INSERT OR REPLACE', 'INSERT OR REPLACE INTO cf_agents_state (id, state) VALUES (?, ?)'],
-      [
-        'INSERT OR REPLACE with column list',
-        `INSERT OR REPLACE INTO cf_agents_mcp_servers ( id, name, server_url, client_id, auth_url,
-          callback_url, server_options )
-        VALUES ( ?, ?, ?, ?, ?, ?, ? )`,
-      ],
-      ['INSERT OR IGNORE', 'INSERT OR IGNORE INTO cf_agents_sub_agents (class, name) VALUES (?, ?)'],
-      ['REPLACE INTO', 'REPLACE INTO cf_agents_queues (id, payload) VALUES (?, ?)'],
-      ['UPDATE OR REPLACE', 'UPDATE OR REPLACE cf_agents_state SET state = ? WHERE id = ?'],
-    ])('returns true for %s on internal tables', (_label, query) => {
-      expect(check(query)).toBe(true);
-    });
-
-    // The summary of a CREATE INDEX carries the index name, not the indexed table — the cf_
-    // target only exists in the ON clause of the full statement.
-    it.each([
-      [
-        'framework statement',
-        `create index if not exists idx_ai_chat_agent_tool_request_id
-          on cf_ai_chat_agent_tool_runs(request_id)`,
-      ],
-      ['uppercase', 'CREATE INDEX idx_agents_state_id ON cf_agents_state (id)'],
-      ['UNIQUE', 'CREATE UNIQUE INDEX idx_agents_state_id ON cf_agents_state (id)'],
-      ['without IF NOT EXISTS', 'CREATE INDEX idx_chunks_stream ON cf_ai_chat_stream_chunks (stream_id)'],
-    ])('returns true for CREATE INDEX (%s) on an internal table', (_label, query) => {
-      expect(check(query)).toBe(true);
-    });
-
-    it('returns true for an internal JOIN', () => {
-      const query = `
-        SELECT f.fiber_id, f.status
-        FROM cf_agents_fibers f
-        LEFT JOIN cf_agents_runs r ON r.id = f.fiber_id
-        WHERE f.status IN ('pending', 'running')
-      `;
-      expect(check(query)).toBe(true);
-    });
-
-    it('returns true when an internal table is joined with a user table', () => {
-      // `.some()` — any internal table present means the query is framework-driven noise.
-      expect(check('SELECT * FROM cf_agents_state s JOIN users u ON u.id = s.id')).toBe(true);
-    });
-
-    it('handles case-insensitive keywords and prefixes', () => {
-      expect(check('select * from CF_AGENTS_STATE')).toBe(true);
-    });
+  it.each([
+    ['undefined', undefined],
+    ['empty', ''],
+  ])('returns false for a %s summary', (_label, summary) => {
+    expect(targetsCloudflareInternalTable(summary)).toBe(false);
   });
 
-  describe('user queries (must be instrumented)', () => {
-    it.each([
-      ['SELECT', 'SELECT * FROM users WHERE id = ?'],
-      ['INSERT', 'INSERT INTO orders (id, total) VALUES (?, ?)'],
-      ['UPDATE', 'UPDATE products SET price = ? WHERE id = ?'],
-      ['DELETE', 'DELETE FROM sessions WHERE expired = 1'],
-      ['CREATE TABLE', 'CREATE TABLE users (id TEXT PRIMARY KEY)'],
-      ['CREATE INDEX', 'CREATE INDEX idx_name ON users (name)'],
-      ['table with cf in the middle', 'SELECT * FROM my_cf_table'],
-      ['table starting with cfg', 'SELECT * FROM cfg_settings'],
-      ['INSERT OR REPLACE', 'INSERT OR REPLACE INTO users (id, name) VALUES (?, ?)'],
-      ['REPLACE INTO', 'REPLACE INTO sessions (id, token) VALUES (?, ?)'],
-      ['UPDATE OR IGNORE', 'UPDATE OR IGNORE products SET price = ? WHERE id = ?'],
-    ])('returns false for %s on user tables', (_label, query) => {
-      expect(check(query)).toBe(false);
-    });
+  it('falls back to the summary when no queryText is passed', () => {
+    expect(targetsCloudflareInternalTable('SELECT cf_agents_state')).toBe(true);
+    expect(targetsCloudflareInternalTable('SELECT users')).toBe(false);
   });
 
-  describe('allowlist (opt a cf_ table back into instrumentation)', () => {
-    it('returns false for an allowlisted table matched by exact string', () => {
-      expect(check('SELECT * FROM cf_my_table', ['cf_my_table'])).toBe(false);
-    });
-
-    it('returns false for an allowlisted table matched by regex', () => {
-      expect(check('SELECT * FROM cf_reports_daily', [/^cf_reports_/])).toBe(false);
-    });
-
-    it('returns false for an allowlisted table targeted by an upsert', () => {
-      expect(check('INSERT OR REPLACE INTO cf_my_table (id) VALUES (?)', ['cf_my_table'])).toBe(false);
-    });
-
-    it('returns false for CREATE INDEX on an allowlisted table', () => {
-      expect(check('CREATE INDEX idx_mine ON cf_my_table (id)', ['cf_my_table'])).toBe(false);
-    });
-
-    it('requires an exact match for string entries', () => {
-      // Substring matches must not opt a table back in, otherwise `cf_` would allowlist everything.
-      expect(check('SELECT * FROM cf_agents_state', ['cf_agents'])).toBe(true);
-    });
-
-    it('still skips genuine internal tables that are not allowlisted', () => {
-      expect(check('SELECT * FROM cf_agents_state', ['cf_my_table'])).toBe(true);
-    });
-
-    it('still skips when an internal table is joined with an allowlisted table', () => {
-      expect(check('SELECT * FROM cf_my_table t JOIN cf_agents_state s ON s.id = t.id', ['cf_my_table'])).toBe(true);
-    });
-
-    it('ignores an empty allowlist', () => {
-      expect(check('SELECT * FROM cf_agents_state', [])).toBe(true);
-    });
-  });
-
-  describe('summaries without a resolvable table target (safe default: instrument)', () => {
-    it.each([
-      ['no-table SELECT', 'SELECT 1'],
-      ['PRAGMA', 'PRAGMA foreign_keys = ON'],
-      ['bare operation', 'BEGIN'],
-    ])('returns false for %s', (_label, query) => {
-      expect(check(query)).toBe(false);
-    });
-
-    it.each([
-      ['undefined', undefined],
-      ['empty', ''],
-    ])('returns false for a %s summary', (_label, summary) => {
-      expect(targetsCloudflareInternalTable(summary)).toBe(false);
-    });
+  // Without queryText a CREATE INDEX summary carries the index name, so the cf_ table in the ON
+  // clause is invisible and the query is instrumented — the caller must pass queryText to filter it.
+  it('cannot resolve a CREATE INDEX target from the summary alone', () => {
+    expect(targetsCloudflareInternalTable('CREATE INDEX idx_agents_state_id')).toBe(false);
   });
 });
