fix(search): newest-first context, session lookup, jsts split, ID disambiguation#2065
fix(search): newest-first context, session lookup, jsts split, ID disambiguation#2065thedotmack wants to merge 1 commit intomainfrom
Conversation
…ambiguation - Fix #1917: Sort timeline and day groups newest-first so truncated 2KB SessionStart preview shows recent data instead of oldest observations - Fix #1918: Use consistent cwd fallback (process.cwd()) in session-init matching context handler; replace unsafe process.cwd() fallback in storeObservation with 'unknown-project' to prevent worker-cwd mismatch - Fix #1919: Split shared jsts tree-sitter query into separate javascript and typescript variants; JS query excludes interface_declaration, type_alias_declaration, and enum_declaration which don't exist in tree-sitter-javascript grammar - Fix #1920: Add get_sessions and get_prompts MCP tools with batch endpoints (/api/sessions/batch, /api/prompts/batch); update context footer, legend, and MCP instructions to disambiguate observation IDs, session IDs (S-prefixed), and prompt IDs Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 33 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Code ReviewOverviewThis PR bundles four targeted bug fixes: newest-first context sorting (#1917), consistent project-key resolution (#1918), JS/TS tree-sitter query split (#1919), and two new MCP batch-fetch tools for sessions and prompts (#1920). The changes are focused, well-scoped, and the motivation for each is clearly documented. Issues1. Wrong store used for prompt lookup (
// Both handlers share the same store — is this intentional?
const store = this.dbManager.getSessionStore();
const sessions = store.getSessionSummariesByIds(...); // OK
...
const store = this.dbManager.getSessionStore();
const prompts = store.getUserPromptsByIds(...); // Bug if prompts live elsewherePlease verify that 2. // store.ts
const resolvedProject = project || 'unknown-project';If the worker receives observations without a project key (e.g. during race-condition startup), they're bucketed under the shared sentinel
Minor Notes3. Duplicated batch-handler validation logic The private parseIds(raw: any, res: Response): number[] | null {
// coerce, validate, return null and set 400 on failure
}4.
5. No automated tests added The test plan is entirely manual. Given that fixes #1917 and #1919 are pure logic changes (sort order reversal, query routing), they're straightforward to unit test. Would be good to add at least:
Positives
|
Greptile SummaryThis PR closes four bugs: timeline/context output is now sorted newest-first so truncated 2KB previews surface recent data; Confidence Score: 5/5Safe to merge; all remaining findings are P2 style suggestions that do not block correct behavior. All four bug fixes are well-scoped and follow existing patterns. The only flagged issues are a missing integer guard on the No files require special attention; Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent as MCP Client (Agent)
participant MCP as mcp-server.ts
participant Worker as Worker HTTP
participant DB as SessionStore (SQLite)
Agent->>MCP: search(query)
MCP->>Worker: POST /api/search
Worker-->>MCP: results with IDs (obs emoji, S-prefixed, prompt)
MCP-->>Agent: index results
Agent->>MCP: timeline(anchor=ID)
MCP->>Worker: GET /api/timeline
Worker-->>MCP: newest-first chronological context
MCP-->>Agent: day-grouped context (newest day first)
alt Observation IDs
Agent->>MCP: get_observations([IDs])
MCP->>Worker: POST /api/observations/batch
Worker->>DB: getObservationsByIds()
DB-->>Worker: observations
Worker-->>MCP: full details
else Session IDs (S-prefixed) [NEW]
Agent->>MCP: get_sessions([IDs])
MCP->>Worker: POST /api/sessions/batch
Worker->>DB: getSessionSummariesByIds()
DB-->>Worker: session summaries
Worker-->>MCP: full details
else Prompt IDs [NEW]
Agent->>MCP: get_prompts([IDs])
MCP->>Worker: POST /api/prompts/batch
Worker->>DB: getUserPromptsByIds()
DB-->>Worker: user prompts
Worker-->>MCP: full details
end
MCP-->>Agent: ~500-1000 tokens/result
Reviews (1): Last reviewed commit: "fix(search): newest-first context, sessi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| /** | ||
| * Get SDK sessions by SDK session IDs | ||
| * POST /api/sdk-sessions/batch |
There was a problem hiding this comment.
limit not validated before SQL interpolation
Both new batch handlers (handleGetSessionSummariesByIds and handleGetPromptsByIds) extract limit from req.body and pass it straight through to the SQL query as LIMIT ${limit}. A non-integer value (e.g. "10; SELECT 1") would be interpolated into the prepared-statement template before this.db.prepare() is called. While bun:sqlite rejects multi-statement strings at prepare-time, it will also throw an unhandled error for any non-numeric value, since the handler has no integer guard on limit.
The existing handleGetObservationsByIds follows the same pattern — consider adding a consistent integer check when limit is truthy:
| * POST /api/sdk-sessions/batch | |
| if (limit !== undefined && (!Number.isInteger(Number(limit)) || Number(limit) < 1)) { | |
| this.badRequest(res, 'limit must be a positive integer'); | |
| return; | |
| } |
|
|
||
| // Guard against empty project string (race condition where project isn't set yet) | ||
| const resolvedProject = project || getProjectContext(process.cwd()).primary; | ||
| // Guard against empty project string (race condition where project isn't set yet). |
There was a problem hiding this comment.
The new fallback ('unknown-project') correctly prevents future writes from using the worker's process.cwd(), but any observations already stored under the incorrect cwd-derived project key (from before this fix) are still stranded — they won't surface in queries scoped to the user's real project key. Worth documenting a one-time migration path (or at least a note in the changelog) so users who hit #1918 before this fix know how to recover those records.
|
Closing to start fresh from main — will redo fixes isolated in Docker container. |
1 participant
Summary
Test plan
Closes #1917, closes #1918, closes #1919, closes #1920
🤖 Generated with Claude Code