fix: coerce stringified numeric anchor in timeline() MCP tool (#2176)

* fix: coerce stringified numeric anchor in timeline() to repair MCP anchor routing HTTP query params arrive as strings, so the typeof anchor === 'number' dispatch always missed the observation-ID branch, falling through to ISO-timestamp parsing and silently returning a wrong-epoch window with the correct anchor echoed in the header. Closes the timeline regression reported on cut-guardian. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * refactor: extract parseNumericAnchor helper and expand timeline tests Address CodeRabbit review nitpicks on PR #2176: - Extract anchor coercion into private parseNumericAnchor helper - Add whitespace-padded numeric-string anchor test case - Add explicit numeric-anchor-not-found regression test Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: assert exact ordering and rendered anchor header in timeline tests Address CodeRabbit nitpick on PR #2176: drop sort to verify chronological ordering, and assert that the rendered anchor/header text echoes the requested numeric ID and marks the anchor row. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * test: extract anchor-render helper and tighten garbage-anchor assertion Address CodeRabbit nitpicks: DRY-up the three repeated anchor header/row assertions into expectAnchorRendered(), and assert the exact "Invalid timestamp: 123abc" error in the garbage-anchor branch instead of a generic non-empty-string check. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
2026-04-27 19:40:02 -07:00
parent 5458dd2322
commit ff0793f7df
2 changed files with 318 additions and 5 deletions
@@ -471,6 +471,14 @@ export class SearchManager {
    };
  }

+  private parseNumericAnchor(anchor: unknown): number | null {
+    if (typeof anchor === 'number') return anchor;
+    if (typeof anchor === 'string' && /^\d+$/.test(anchor.trim())) {
+      return Number(anchor.trim());
+    }
+    return null;
+  }
+
  /**
   * Tool handler: timeline
   */
@@ -478,6 +486,7 @@ export class SearchManager {
    const { anchor, query, depth_before, depth_after, project } = args;
    const depthBefore = depth_before != null ? Number(depth_before) : 10;
    const depthAfter = depth_after != null ? Number(depth_after) : 10;
+    const anchorAsNumber = this.parseNumericAnchor(anchor);
    const cwd = process.cwd();

    // Validate: must provide either anchor or query, not both
@@ -550,21 +559,21 @@ export class SearchManager {
      timelineData = this.sessionStore.getTimelineAroundObservation(topResult.id, topResult.created_at_epoch, depthBefore, depthAfter, project);
    }
    // MODE 2: Anchor-based timeline
-    else if (typeof anchor === 'number') {
+    else if (anchorAsNumber !== null) {
      // Observation ID
-      const obs = this.sessionStore.getObservationById(anchor);
+      const obs = this.sessionStore.getObservationById(anchorAsNumber);
      if (!obs) {
        return {
          content: [{
            type: 'text' as const,
-            text: `Observation #${anchor} not found`
+            text: `Observation #${anchorAsNumber} not found`
          }],
          isError: true
        };
      }
-      anchorId = anchor;
+      anchorId = anchorAsNumber;
      anchorEpoch = obs.created_at_epoch;
-      timelineData = this.sessionStore.getTimelineAroundObservation(anchor, anchorEpoch, depthBefore, depthAfter, project);
+      timelineData = this.sessionStore.getTimelineAroundObservation(anchorAsNumber, anchorEpoch, depthBefore, depthAfter, project);
    } else if (typeof anchor === 'string') {
      // Session ID or ISO timestamp
      if (anchor.startsWith('S') || anchor.startsWith('#S')) {
@@ -0,0 +1,304 @@
+/**
+ * Regression coverage for SearchManager.timeline() anchor dispatch.
+ *
+ * Bug history: HTTP query params arrive as strings, so the
+ * `typeof anchor === 'number'` dispatch missed the observation-ID branch
+ * and silently fell through to ISO-timestamp parsing — returning a
+ * wrong-epoch window with the correct anchor still echoed in the header.
+ *
+ * The fix coerces stringified numerics in `SearchManager.timeline()` via
+ * `anchorAsNumber`. These tests guard that fix by exercising:
+ *   (a) numeric anchor as JS number
+ *   (b) numeric anchor as string (THE bug case)
+ *   (c) session-ID string anchor "S<n>"
+ *   (d) ISO-timestamp anchor
+ *   (e) garbage anchor (must return isError: true)
+ *
+ * Pattern source: tests/session_store.test.ts uses real SessionStore
+ * against ':memory:' SQLite. We follow the same approach (no SessionStore
+ * mocks) and additionally instantiate real SessionSearch over the same DB
+ * handle, plus real FormattingService and TimelineService. ChromaSync is
+ * passed as null (the timeline anchor branch does not require Chroma).
+ */
+import { describe, it, expect, beforeEach, afterEach, mock } from 'bun:test';
+
+// ModeManager is a global singleton that requires `loadMode()` to be
+// called before use. The formatter path inside `SearchManager.timeline()`
+// calls `ModeManager.getInstance().getTypeIcon(...)`, which throws if no
+// mode is loaded. Existing worker tests (e.g. tests/worker/search/
+// result-formatter.test.ts) follow the same pattern: stub ModeManager
+// so the unrelated config singleton does not blow up the unit under
+// test. We deliberately do NOT mock SessionStore — that's the data
+// layer the bug travelled through, and faking it would defeat the
+// regression coverage.
+mock.module('../../src/services/domain/ModeManager.js', () => ({
+  ModeManager: {
+    getInstance: () => ({
+      getActiveMode: () => ({
+        name: 'code',
+        prompts: {},
+        observation_types: [
+          { id: 'discovery', icon: 'I' },
+        ],
+        observation_concepts: [],
+      }),
+      getObservationTypes: () => [{ id: 'discovery', icon: 'I' }],
+      getTypeIcon: (_type: string) => 'I',
+      getWorkEmoji: () => 'W',
+    }),
+  },
+}));
+
+import { Database } from 'bun:sqlite';
+import { SessionStore } from '../../src/services/sqlite/SessionStore.js';
+import { SessionSearch } from '../../src/services/sqlite/SessionSearch.js';
+import { FormattingService } from '../../src/services/worker/FormattingService.js';
+import { TimelineService } from '../../src/services/worker/TimelineService.js';
+import { SearchManager } from '../../src/services/worker/SearchManager.js';
+
+const PROJECT = 'timeline-anchor-test';
+const MEMORY_SESSION_ID = 'mem-session-timeline-anchor';
+const CONTENT_SESSION_ID = 'content-timeline-anchor';
+
+interface SeededObservation {
+  id: number;
+  epoch: number;
+}
+
+function seedObservations(store: SessionStore, count: number): SeededObservation[] {
+  const sdkId = store.createSDKSession(CONTENT_SESSION_ID, PROJECT, 'initial prompt');
+  store.updateMemorySessionId(sdkId, MEMORY_SESSION_ID);
+
+  // Anchor the synthetic timeline well in the past so it cannot collide with
+  // any "recent rows" the buggy code path would otherwise return.
+  const baseEpoch = Date.UTC(2024, 0, 1, 0, 0, 0); // 2024-01-01T00:00:00Z
+  const stepMs = 60_000; // 1 minute apart, deterministic ordering
+
+  const seeded: SeededObservation[] = [];
+  for (let i = 0; i < count; i++) {
+    const epoch = baseEpoch + i * stepMs;
+    const result = store.storeObservation(
+      MEMORY_SESSION_ID,
+      PROJECT,
+      {
+        type: 'discovery',
+        title: `Synthetic observation #${i + 1}`,
+        subtitle: null,
+        facts: [],
+        narrative: `Narrative for synthetic observation ${i + 1}`,
+        concepts: [],
+        files_read: [],
+        files_modified: [],
+      },
+      i + 1,
+      0,
+      epoch
+    );
+    seeded.push({ id: result.id, epoch: result.createdAtEpoch });
+  }
+  return seeded;
+}
+
+/**
+ * Pull the observation IDs out of the timeline's formatted markdown.
+ * Each observation row renders as `| #<id> | <time> | ...` (see
+ * SearchManager.timeline() formatter, ~line 744). We only want
+ * observation IDs (rows starting with `| #` followed by a digit) — we
+ * deliberately skip session rows (`| #S...`) and prompt headers.
+ */
+function extractObservationIds(formattedText: string): number[] {
+  const ids: number[] = [];
+  const rowRegex = /^\|\s*#(\d+)\s*\|/gm;
+  let match: RegExpExecArray | null;
+  while ((match = rowRegex.exec(formattedText)) !== null) {
+    ids.push(Number(match[1]));
+  }
+  return ids;
+}
+
+function expectAnchorRendered(text: string, anchorId: number): void {
+  expect(text).toContain(`# Timeline around anchor: ${anchorId}`);
+  const anchorRow = text
+    .split('\n')
+    .find((line) => line.startsWith(`| #${anchorId} `));
+  expect(anchorRow).toBeDefined();
+  expect(anchorRow).toContain('<- **ANCHOR**');
+}
+
+describe('SearchManager.timeline() anchor dispatch', () => {
+  let db: Database;
+  let store: SessionStore;
+  let search: SessionSearch;
+  let manager: SearchManager;
+  let seeded: SeededObservation[];
+
+  beforeEach(() => {
+    // Real SQLite, shared connection between store + search (same wiring
+    // DatabaseManager uses in production at src/services/worker/DatabaseManager.ts:34-35).
+    db = new Database(':memory:');
+    db.run('PRAGMA foreign_keys = ON');
+    store = new SessionStore(db);
+    search = new SessionSearch(db);
+
+    seeded = seedObservations(store, 50);
+    manager = new SearchManager(
+      search,
+      store,
+      null, // ChromaSync intentionally null: anchor dispatch must not require it.
+      new FormattingService(),
+      new TimelineService()
+    );
+  });
+
+  afterEach(() => {
+    db.close();
+  });
+
+  it('(a) numeric anchor passed as JS number returns the 7-id window around the anchor', async () => {
+    // depth_before=3 + anchor + depth_after=3 = 7 IDs
+    const middle = seeded[24]; // 25th observation (index 24)
+    const expectedIds = seeded.slice(21, 28).map((o) => o.id);
+
+    const response = await manager.timeline({
+      anchor: middle.id, // pass as JS number
+      depth_before: 3,
+      depth_after: 3,
+    });
+
+    expect(response.isError).not.toBe(true);
+    const text: string = response.content[0].text;
+    const returnedIds = extractObservationIds(text);
+    // Exact sequence equality — chronological order matters, not just membership.
+    expect(returnedIds).toEqual(expectedIds);
+    // Header must echo the anchor ID and the anchor row must be marked.
+    expectAnchorRendered(text, middle.id);
+  });
+
+  it('(b) numeric anchor passed as STRING returns the 7-id window around the anchor (THE bug case)', async () => {
+    // This is the exact regression that motivated Phase 2's anchorAsNumber
+    // coercion. Without that fix, the response collapsed to the most
+    // recent rows because `new Date("<digits>")` produced a wrong-epoch
+    // window, while the header still echoed the requested anchor.
+    const middle = seeded[24];
+    const expectedIds = seeded.slice(21, 28).map((o) => o.id);
+
+    const response = await manager.timeline({
+      anchor: String(middle.id), // pass as STRING — what HTTP layer always sends
+      depth_before: 3,
+      depth_after: 3,
+    });
+
+    expect(response.isError).not.toBe(true);
+    const text: string = response.content[0].text;
+    const returnedIds = extractObservationIds(text);
+    expect(returnedIds).toEqual(expectedIds);
+    expectAnchorRendered(text, middle.id);
+  });
+
+  it('(b2) numeric anchor with surrounding whitespace is coerced and returns the same window', async () => {
+    const middle = seeded[24];
+    const expectedIds = seeded.slice(21, 28).map((o) => o.id);
+
+    const response = await manager.timeline({
+      anchor: `  ${middle.id}  `,
+      depth_before: 3,
+      depth_after: 3,
+    });
+
+    expect(response.isError).not.toBe(true);
+    const text: string = response.content[0].text;
+    const returnedIds = extractObservationIds(text);
+    expect(returnedIds).toEqual(expectedIds);
+    // Whitespace must be trimmed in the rendered header — the trimmed numeric ID, not the padded string.
+    expectAnchorRendered(text, middle.id);
+  });
+
+  it('(c) session-ID anchor "S<n>" routes to the timestamp branch and returns a non-error response', async () => {
+    // Look up the SDK session row id directly. The timeline session
+    // anchor branch (SearchManager.timeline ~line 576) parses the integer
+    // after the "S" and calls getSessionSummariesByIds, so we need a row
+    // in session_summaries for this id. Build one off the existing
+    // memory session.
+    // Anchor the synthetic summary on the same epoch as the middle
+    // observation so the timestamp branch lands inside the seeded range.
+    const middle = seeded[24];
+    const summaryResult = store.storeSummary(
+      MEMORY_SESSION_ID,
+      PROJECT,
+      {
+        request: 'Synthetic session for timeline anchor test',
+        investigated: '',
+        learned: '',
+        completed: '',
+        next_steps: '',
+        notes: null,
+      },
+      undefined,
+      0,
+      middle.epoch
+    );
+    const sessionDbId = summaryResult.id;
+
+    const response = await manager.timeline({
+      anchor: `S${sessionDbId}`,
+      depth_before: 3,
+      depth_after: 3,
+    });
+
+    expect(response.isError).not.toBe(true);
+    // We do not assert the exact ID set here — getTimelineAroundTimestamp
+    // returns whatever lives near the session's epoch. The invariant the
+    // bug was about (numeric coercion not stealing string anchors) is
+    // captured by the fact that this call does NOT 404 and does NOT hit
+    // the invalid-anchor branch.
+    const text: string = response.content[0].text;
+    expect(typeof text).toBe('string');
+    expect(text.length).toBeGreaterThan(0);
+  });
+
+  it('(d) ISO-timestamp anchor routes to the timestamp branch and returns a non-error response', async () => {
+    // Pick an ISO timestamp in the middle of our seeded range.
+    const middle = seeded[24];
+    const isoAnchor = new Date(middle.epoch).toISOString();
+
+    const response = await manager.timeline({
+      anchor: isoAnchor,
+      depth_before: 3,
+      depth_after: 3,
+    });
+
+    expect(response.isError).not.toBe(true);
+    const text: string = response.content[0].text;
+    // ISO branch uses a timestamp window — the seeded observation closest
+    // to the requested epoch must appear somewhere in the result.
+    const returnedIds = extractObservationIds(text);
+    expect(returnedIds).toContain(middle.id);
+  });
+
+  it('(e) garbage anchor "123abc" returns isError: true (does NOT swallow as numeric)', async () => {
+    const response = await manager.timeline({
+      anchor: '123abc',
+      depth_before: 3,
+      depth_after: 3,
+    });
+
+    expect(response.isError).toBe(true);
+    const text: string = response.content[0].text;
+    // Garbage strings must hit the ISO-timestamp branch and surface its
+    // concrete "Invalid timestamp" error — not the numeric-observation
+    // branch (which would mean `anchorAsNumber` silently coerced "123abc").
+    expect(text).toBe('Invalid timestamp: 123abc');
+  });
+
+  it('(f) numeric anchor not found returns Observation #... not found with isError', async () => {
+    const response = await manager.timeline({
+      anchor: '99999999',
+      depth_before: 3,
+      depth_after: 3,
+    });
+
+    expect(response.isError).toBe(true);
+    const text: string = response.content[0].text;
+    expect(text).toContain('Observation #99999999 not found');
+  });
+});