feat(control-plane): add indexed cursor-paginated request history - #1004
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds a rebuildable SQLite index over ChangesRequest History
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant ManagementAPI
participant RequestHistoryRoutes
participant RequestHistoryIndexer
participant UsageLedger
CLI->>RequestHistoryIndexer: rebuild index or read index status
ManagementAPI->>RequestHistoryRoutes: request history query
RequestHistoryRoutes->>RequestHistoryIndexer: validate filters and query cursor
RequestHistoryIndexer->>UsageLedger: read canonical usage.jsonl tail
RequestHistoryIndexer-->>RequestHistoryRoutes: page and index metadata
RequestHistoryRoutes-->>ManagementAPI: history response
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7efb6e8428
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@devlog/_plan/260804_router_intelligence/000_master_plan.md`:
- Around line 393-398: Update the RI-10 acceptance criterion to require
documentation in all six locale files—en, de, ja, ko, ru, and zh—matching the
locale set defined earlier in the plan.
- Around line 252-256: Use one canonical rebuild timestamp key across the plan:
update the rebuild audit requirement near the indexer behavior description to
use the existing schema_meta key built_at_ms, and ensure the indexer, status
API, and tests reference that same field and millisecond unit instead of
rebuilt_at.
- Around line 186-187: Fix the RouteCandidateTrace schema formatting by
replacing the nested inline code spans with one valid Markdown code span or a
fenced code block. Preserve the full schema fields and exclusions constraint
while ensuring markdownlint MD038 passes.
In `@devlog/_plan/260804_router_intelligence/001_pr_stack_status.md`:
- Around line 40-45: Synchronize the RI-02 entry in the stack status table with
the completed implementation and verification recorded in the acceptance log:
replace the pending head SHA and PR fields with their finalized values and
update the status from “in progress” to the verified state. Keep the RI-02 base
reference unchanged and ensure the corresponding final commit and PR fields in
the acceptance log are updated consistently.
In `@src/router.ts`:
- Around line 538-557: Make trace construction optional in routeModel by adding
a trace opt-out option that preserves tracing by default. Only assign
route.routeDecision through buildRouteDecisionTrace when tracing is enabled,
while keeping routing behavior unchanged. Update the probe calls in
canDecryptUnreadableAgentTask to disable tracing since they only inspect the
selected provider.
- Around line 389-421: Update comboRouteCandidates to determine configured
providers with hasOwnProvider, matching the ownership check used by
routeModelInternal and getCombo. Only read provider.disabled after confirming
the provider is an own property, preserving the existing enabled, cooldown, and
candidate-exclusion behavior.
In `@src/routing/history/indexer.ts`:
- Around line 445-450: Coordinate rebuildRequestHistoryIndex across processes so
the CLI cannot drop and repopulate the index while the server serves requests;
prefer routing the rebuild through the existing management API via logs’
runtimeRequest flow, or otherwise add a config-directory advisory lock shared by
readers and rebuilders. Ensure rebuild-index preserves consistent
request-history reads during the full rebuild.
- Around line 500-515: Update the request-history refresh flow so the validated
database handle is returned alongside metadata: have refreshLocked return both
handle and meta on every exit, add internal openRequestHistoryIndexHandle
returning that RefreshResult, and keep openRequestHistoryIndex as a
metadata-only wrapper. Change queryRequestHistory and requestHistoryRowById to
use the returned handle instead of re-reading the mutable db singleton after
awaiting.
- Around line 181-197: Replace the whole-file buffering in readCompleteTail and
its ingestSourceTail integration with bounded chunk reads that preserve any
partial trailing line between chunks. Ensure chunks without a newline expand the
read window and never stall on an oversized line; ingest and commit each chunk
incrementally while advancing indexedOffset until the source is exhausted,
avoiding full-ledger buffers and split arrays.
- Around line 295-318: Update destroyAndRecreate to delete the database WAL and
SHM sidecars alongside path within the existing retry cleanup loop, before
creating the replacement database. Update indexDbPath to register both
corresponding -wal and -shm paths with recordOwnedConfigPath so ownership
cleanup removes them as well.
- Around line 523-528: Update the nextCursor construction in queryRequestHistory
to avoid re-parsing the final row’s row_json; select the indexed timestamp and
request_id ordering columns with each page row and use those values directly in
encodeHistoryCursor. Preserve the existing hasMore and non-empty page guard, and
ensure damaged JSON remains skipped by hydrateRow without causing pagination to
throw.
- Around line 487-498: Expose the implementation currently referenced as
normalizeUsageEntryForTest from usage/log.ts under the production name
normalizeUsageEntry, and update hydrateRow in indexer.ts to call that production
symbol. Retain normalizeUsageEntryForTest only as a deprecated compatibility
alias if existing tests require it, while preserving the current hydration and
validation behavior.
- Around line 128-133: Update sourceIdentityMatches to compare only
revision.path, revision.dev, revision.ino, and revision.birthtimeMs against the
stored identity fields, while leaving size and mtime checks to the
offset/truncation logic. In openIndexDb, initialize those identity fields in the
fresh-database branch using the existing metadata flow so the first refresh does
not rebuild unnecessarily.
In `@src/routing/history/schema.ts`:
- Around line 70-72: Update indexDbPath in the history indexer to import and
call historyIndexPath instead of constructing the database path inline with
HISTORY_DB_FILENAME. Preserve the existing directory input while reusing
historyIndexPath’s trailing-separator normalization.
In `@src/routing/trace.ts`:
- Around line 322-338: Update enforceByteBudget to measure serialized trace size
in UTF-8 bytes rather than UTF-16 code units. Replace both serialized.length
checks with TextEncoder-based encoded byte-length checks, preserving the
existing truncation and candidate-reduction flow.
- Around line 474-478: Update the evidence object construction around
parseCapability, parseHealth, parseQuota, parseCost, and parseScore so each
parser result is computed once per candidate, stored in a local binding, and
reused for both the presence check and property value. Preserve the existing
omission behavior for falsy or absent parsed results while eliminating the
duplicate parser calls.
- Around line 275-279: Replace the requirements-overflow assignment in the
requirements normalization block so it sets a dedicated truncated.requirements
flag instead of truncated.candidates. Add the corresponding flag to the
truncated shape, and update normalizeRouteDecisionTrace to preserve it during
defensive parsing and persisted trace round trips while leaving candidate
truncation behavior unchanged.
In `@src/server/management/request-history-routes.ts`:
- Around line 92-96: Update the GET request-history route around requestId
decoding to catch malformed URI decoding and treat it as an unknown request,
returning the existing 404 response instead of propagating the URIError.
Preserve the current empty-ID and slash validation, and add a regression test in
the request-history index tests for GET /api/request-history/% asserting a 404
response.
- Around line 23-27: Update parseOptionalInt to distinguish absent parameters
from present-but-invalid values, including treating trimmed empty strings as
invalid, then update each request-history validation path to return a stable 400
for the invalid marker before applying defaults or filters. Preserve undefined
only for genuinely absent parameters, and add regression coverage in the
existing validation tests for invalid status, limit, and from query values.
In `@src/server/request-log.ts`:
- Around line 255-267: Update normalizeRouteDecisionTraceForLog to return the
normalized result directly and omit routeDecision when
normalizeRouteDecisionTrace rejects the persisted trace, rather than falling
back to the original entry. Align the request-log hydration and DTO mapping flow
with the existing rejection behavior in usage logging, while preserving valid
normalized traces.
In `@src/server/responses/core.ts`:
- Line 856: Update handleComboResponses so promoting child logs via
Object.assign does not overwrite the parent alias-level routeDecision with the
child’s explicit-provider trace. Preserve the existing combo routeDecision, or
rebuild it as a combo trace from the selected target, on both successful and
exhausted paths. Add persistence coverage for both outcomes, verifying
route_kind and decision_json retain the combo trace.
In `@tests/request-history-index.test.ts`:
- Around line 247-259: Extend the existing invalid-filter test after the
badLimit and badStatus requests to assert error codes invalid_limit and
invalid_status, not only HTTP 400. Add a from=2&to=1 request and assert HTTP 400
with error code invalid_range, preserving the existing invalid_cursor assertion.
- Around line 270-280: Strengthen the test around rebuildRequestHistoryIndex by
reading the canonical JSONL ledger entries after appendUsageEntry, ordering them
according to the defined request-history order, and comparing the rebuilt
queryRequestHistory rows against those canonical entries rather than comparing
two identical rebuilt projections. Keep the indexedRows count assertion and
place the focused regression coverage alongside the existing request-history
tests.
In `@tests/route-decision-trace.test.ts`:
- Around line 125-141: Update the test “combo candidates are capped and the
selected candidate survives truncation” to select a candidate beyond
MAX_TRACE_CANDIDATES by making the early targets ineligible or invoking
buildRouteDecisionTrace directly. Assert the truncated trace retains the
selected final candidate and that selected.candidateIndex is adjusted to its
final index.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 6dade0d3-9678-4e4c-9bd9-9e9602eb8b5c
📒 Files selected for processing (19)
devlog/_plan/260804_router_intelligence/000_master_plan.mddevlog/_plan/260804_router_intelligence/001_pr_stack_status.mdsrc/cli/observe.tssrc/router.tssrc/routing/history/cursor.tssrc/routing/history/indexer.tssrc/routing/history/schema.tssrc/routing/trace.tssrc/server/chat-completions.tssrc/server/claude-messages.tssrc/server/management-api.tssrc/server/management/request-history-routes.tssrc/server/request-log.tssrc/server/responses/compact.tssrc/server/responses/core.tssrc/server/search.tssrc/usage/log.tstests/request-history-index.test.tstests/route-decision-trace.test.ts
7efb6e8 to
85d3b24
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (8)
src/routing/history/indexer.ts (5)
295-318: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
destroyAndRecreatestill leaves the WAL sidecars on disk.Line 312 enables
PRAGMA journal_mode = WAL, so SQLite maintainsrouting-history.sqlite-walandrouting-history.sqlite-shm. Line 304 unlinks onlypath. A stale-walfrom the discarded database survives next to the replacement database, andindexDbPath(Lines 84-88) registers only the main file withrecordOwnedConfigPath, so config-ownership cleanup never removes the sidecars.🐛 Proposed fix
const fresh = new Database(path, { create: true }); + // WAL mode keeps `-wal`/`-shm` sidecars; a stale WAL must not outlive its database. + for (const suffix of ["-wal", "-shm"]) { + try { unlinkSync(`${path}${suffix}`); } catch { /* absent */ } + }Move the sidecar removal above
new Database(path, ...), and register both sidecar paths inindexDbPath.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routing/history/indexer.ts` around lines 295 - 318, Update destroyAndRecreate to remove the SQLite WAL and SHM sidecar files, alongside the main database path, before constructing the replacement Database. Update indexDbPath to register both sidecar paths with recordOwnedConfigPath so ownership cleanup removes them as well.
128-133: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
sourceIdentityMatchesstill forces a full rebuild after every normal append.
refreshLockedcallsensureSchemaAndIdentity(Line 398) before it reaches the incremental offset check.ingestSourceTailstores the size and mtime observed at ingest time (Lines 257-258). One new ledger row changesrevision.size, so Lines 131-132 report a mismatch,ensureSchemaAndIdentitycallsdestroyAndRecreate(Line 364), andrefreshLockedrunsfullRebuild. The incremental branch at Lines 417-419 therefore never runs after an append, and everyGET /api/request-historyreingests the whole ledger.The check also ignores
sourcePath,sourceDev,sourceIno, andsourceBirthtimeMs, althoughrecordSourceMetastores them at Lines 277-284. A replacement file with the same size and mtime passes the check and causes tail ingestion from a stale offset.Compare identity fields only, and leave size and mtime to the offset and truncation logic at Lines 411-419.
🐛 Proposed fix
function sourceIdentityMatches(dbHandle: Database, revision: UsageLogRevision | null): boolean { - const stored = readIndexedMeta(dbHandle); - if (revision === null) return stored.sourceSize === 0; - return stored.sourceSize === Number(revision.size) - && stored.sourceMtimeMs === Number(revision.mtimeMs); + if (revision === null) return readIndexedMeta(dbHandle).sourceSize === 0; + return metaValue(dbHandle, HISTORY_META_KEYS.sourcePath) === revision.path + && metaValue(dbHandle, HISTORY_META_KEYS.sourceDev) === String(revision.dev) + && metaValue(dbHandle, HISTORY_META_KEYS.sourceIno) === String(revision.ino) + && metaValue(dbHandle, HISTORY_META_KEYS.sourceBirthtimeMs) === String(revision.birthtimeMs); }Also record the identity fields in the fresh-database branch of
openIndexDb(Lines 329-337) so the first refresh does not rebuild without cause.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routing/history/indexer.ts` around lines 128 - 133, Update sourceIdentityMatches to compare only source identity fields—sourcePath, sourceDev, sourceIno, and sourceBirthtimeMs—while leaving sourceSize and sourceMtimeMs to the offset/truncation logic in refreshLocked. Ensure the fresh-database branch of openIndexDb records these identity fields via the existing metadata mechanism so the initial refresh does not trigger an unnecessary rebuild.
523-528: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winThe
nextCursorpath still re-parsesrow_jsonwithout a guard and reads non-indexed values.
hydrateRowwrapsJSON.parsein try/catch and skips a damaged row (Lines 489-497). Line 526 parses the samerow_jsonagain with no guard. If the last row of a page holds invalid JSON,JSON.parsethrowsSyntaxError,queryRequestHistorydoes not catch it, andhandleRequestHistoryRoutescatches onlyInvalidCursorError, so the client receives a 500 instead of a page.Second defect in the same lines:
parsed.timestampandparsed.requestIdcome from the JSON blob, but pagination orders and filters on the indexedtimestampandrequest_idcolumns. If those values ever disagree, the cursor points outside the keyset window and the client skips or repeats rows.Select the ordering columns and use them directly.
🐛 Proposed fix
const rows = handle.query( - `SELECT row_json FROM requests${whereSql} ORDER BY timestamp DESC, request_id DESC LIMIT ?`, - ).all(...values, limit + 1) as Array<{ row_json: string }>; + `SELECT row_json, timestamp, request_id FROM requests${whereSql} ORDER BY timestamp DESC, request_id DESC LIMIT ?`, + ).all(...values, limit + 1) as Array<{ row_json: string; timestamp: number; request_id: string }>;if (hasMore && pageRows.length > 0) { const last = pageRows[pageRows.length - 1]!; - const parsed = JSON.parse(last.row_json) as PersistedUsageEntry; - nextCursor = encodeHistoryCursor({ t: parsed.timestamp, i: parsed.requestId }); + nextCursor = encodeHistoryCursor({ t: last.timestamp, i: last.request_id }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routing/history/indexer.ts` around lines 523 - 528, Update the nextCursor logic in queryRequestHistory to avoid re-parsing pageRows[last].row_json and instead use the row’s indexed timestamp and request_id values directly when calling encodeHistoryCursor. Preserve the existing hasMore and non-empty page guard so malformed JSON skipped by hydrateRow cannot cause pagination to throw.
181-197: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
readCompleteTailstill buffers the whole ledger synchronously on the request thread.On a full rebuild
ingestSourceTailpassesfromOffset = 0(Line 389), solengthat Line 183 equals the wholeusage.jsonlsize. Peak memory holds three copies: theBuffer(Line 184), the string from.toString("utf-8")(Line 194), and the line array fromtext.split(/\r?\n/)(Line 217).queryRequestHistoryawaits this path, so a singleGET /api/request-historyperforms a synchronous whole-file read plus every insert on the server request thread. Combined with the rebuild-on-append defect at Lines 128-133, this cost is paid on each query.Read and ingest in bounded chunks, and carry the partial trailing line across chunks so peak memory stays fixed. A chunk that contains no newline must grow the read window, otherwise a line longer than the chunk size stalls progress.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routing/history/indexer.ts` around lines 181 - 197, Refactor readCompleteTail and the ingestSourceTail path to read the ledger in bounded chunks instead of allocating the full file and splitting it at once. Preserve partial trailing-line data across chunks, emit complete lines for ingestion, and ensure newline-free chunks expand or otherwise advance the read window so lines longer than the chunk size do not stall. Update the request-history flow to use this incremental ingestion while preserving offsets and existing rebuild behavior.
500-515: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
db!is still read after anawait, so a concurrent close dereferencesnull.
openRequestHistoryIndexreturns meta only; it does not hand back the handle. Line 514 re-reads the module singletondband asserts non-null.closeRequestHistoryIndex(Lines 437-443) setsdb = null. Between theawaitat Line 505 and the read at Line 514, a shutdown path or a test teardown can null it, and Line 515 throws insidequeryRows.handleRequestHistoryRoutescatches onlyInvalidCursorError, so the caller receives a 500.requestHistoryRowByIdhas the same defect at Line 534.Fix the root cause: return the validated handle from the refresh.
🐛 Proposed fix
+interface RefreshResult { handle: Database; meta: RequestHistoryIndexMeta } + +async function openRequestHistoryIndexHandle(): Promise<RefreshResult> { /* single-flight refresh returning { handle, meta } */ }- const meta = await openRequestHistoryIndex(); + const { handle, meta } = await openRequestHistoryIndexHandle(); ... - const handle = db!; const { rows, total } = queryRows(handle, filters, cursor, limit);Keep
openRequestHistoryIndexas a meta-only wrapper for the existing public contract.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routing/history/indexer.ts` around lines 500 - 515, Update the request-history index refresh flow so it returns the validated database handle alongside its metadata, then use that returned handle in queryRequestHistory and requestHistoryRowById instead of re-reading the nullable db singleton after await. Preserve openRequestHistoryIndex as the existing metadata-only public wrapper, while ensuring both callers use the handle obtained during refresh.src/routing/history/schema.ts (1)
70-72: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
historyIndexPathis still bypassed by the indexer.
historyIndexPathnormalizes trailing separators before it appendsHISTORY_DB_FILENAME.indexDbPathinsrc/routing/history/indexer.ts(Lines 84-88) still builds${dir}/${HISTORY_DB_FILENAME}inline, twice, and skips that normalization. Two constructions of the same path can drift, andrecordOwnedConfigPaththen records a path that other callers do not use.Import
historyIndexPathin the indexer and call it fromindexDbPath.♻️ Proposed change in src/routing/history/indexer.ts
function indexDbPath(): string { const dir = getConfigDir(); - recordOwnedConfigPath(dir, `${dir}/${HISTORY_DB_FILENAME}`); - return `${dir}/${HISTORY_DB_FILENAME}`; + const path = historyIndexPath(dir); + recordOwnedConfigPath(dir, path); + return path; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/routing/history/schema.ts` around lines 70 - 72, Update indexDbPath in the history indexer to import and use historyIndexPath for both index-path constructions instead of interpolating HISTORY_DB_FILENAME directly. Preserve the existing directory inputs and recordOwnedConfigPath behavior while ensuring all callers use the normalized path helper.src/server/management/request-history-routes.ts (2)
92-96: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
decodeURIComponentstill throws on a malformed percent sequence, producing a 500.Line 93 decodes the raw path remainder with no guard.
GET /api/request-history/%or/api/request-history/%zzmakesdecodeURIComponentthrowURIError: URI malformed. Thetry/catchat Line 84 covers only the collection route.handleManagementAPIinsrc/server/management-api.ts(Lines 152-166) handles onlymanagementBodyTooLargeResponse,OAuthMutationBusyError, andCatalogGatherBusyError, and rethrows everything else. A malformed identifier should return 404, not 500.🐛 Proposed fix
- const requestId = decodeURIComponent(url.pathname.slice("/api/request-history/".length)); - if (!requestId || requestId.includes("/")) { + const rawId = url.pathname.slice("/api/request-history/".length); + let requestId: string; + try { + requestId = decodeURIComponent(rawId); + } catch { + // Malformed percent-encoding is an unknown request, not a server fault. + return jsonResponse({ error: { code: "not_found", message: "unknown request" } }, 404, req, config); + } + if (!requestId || requestId.includes("/")) { return jsonResponse({ error: { code: "not_found", message: "unknown request" } }, 404, req, config); }Add a regression test for
GET /api/request-history/%intests/request-history-index.test.tsasserting 404.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/management/request-history-routes.ts` around lines 92 - 96, Guard the requestId decoding in the GET request-history route so malformed percent-encoded path segments are treated as unknown requests and return the existing 404 response instead of throwing. Update the route logic around requestId and add a regression test in the request-history index tests for GET /api/request-history/% asserting status 404.
23-27: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
parseOptionalIntstill maps invalid input toundefined, so bad filters are dropped instead of rejected.The function cannot distinguish "absent" from "present but not an integer". Both return
undefined, and every caller then treats the parameter as absent.Reachable cases through
GET /api/request-history:
?status=abc→undefined. The guard at Line 35 is skipped,queryRequestHistoryreceives no status filter, and the response is unfiltered history with a 200.?from=abc&to=abc→ bothundefined. The range check at Line 40 is skipped and the time window is dropped silently.?limit=abc→undefined. The guard at Line 45 is skipped and the default page size applies.The PR objectives state that invalid filters return a stable 400. This code returns 200 with the wrong result set. Empty strings behave inconsistently too, because
Number("")is0:?status=returns 400, but?from=applies a realtimestamp >= 0filter.🐛 Proposed fix
-function parseOptionalInt(raw: string | null): number | undefined { - if (raw === null) return undefined; - const value = Number(raw.trim()); - return Number.isInteger(value) ? value : undefined; -} +const INVALID = Symbol("invalid"); + +/** `undefined` means absent; `INVALID` means present but not an integer. */ +function parseOptionalInt(raw: string | null): number | undefined | typeof INVALID { + if (raw === null) return undefined; + const trimmed = raw.trim(); + if (trimmed.length === 0) return INVALID; + const value = Number(trimmed); + return Number.isInteger(value) ? value : INVALID; +}- if (status !== undefined && (status < 100 || status > 599)) { + if (status === INVALID || (status !== undefined && (status < 100 || status > 599))) {Reject
INVALIDforfrom,to, andlimitin the same way. Line 44 can then callparseOptionalInt(url.searchParams.get("limit"))directly, because the function already handlesnull.Add regression tests for
?status=abc,?limit=abc, and?from=abcnext to the existing validation tests intests/request-history-index.test.ts, asserting 400. As per path instructions, a behavior change insrc/should come with a focused regression test near the existing tests for that subsystem.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/server/management/request-history-routes.ts` around lines 23 - 27, Update parseOptionalInt to distinguish absent, valid, and present-but-invalid values, returning the established INVALID result for non-integer or empty-string input while preserving undefined for null. Update the GET /api/request-history validation for status, from, to, and limit to reject INVALID with a stable 400, and pass limit directly through parseOptionalInt. Add focused regression tests in the existing request-history validation tests for invalid status, limit, and from query parameters, each asserting 400.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cli/observe.ts`:
- Around line 84-90: Update rebuildIndex and indexStatus to call the running
server’s management API through runtimeRequest, matching the logs handler,
instead of importing and invoking the history indexer directly. Preserve their
existing argument parsing and output behavior while passing deps through the
request path; this ensures the server owns index writes. If standalone execution
is intentionally required, instead remove the unused deps parameters and
document that constraint.
In `@src/routing/history/indexer.ts`:
- Around line 452-485: Rename the `queryRows` return property and its local
usage from `total` to `fetched`, including the return object and the
`queryRequestHistory` consumer, while preserving its meaning as the number of
rows retrieved by the `limit + 1` probe used for `hasMore`.
In `@tests/request-history-index.test.ts`:
- Around line 282-291: Add a focused pagination test near “cursor stays stable
while appends arrive between pages” that seeds more rows than one page with
identical timestamps, pages through them using a small limit and successive
nextCursor values, and asserts every expected requestId appears exactly once
without skips or duplicates.
- Around line 199-213: Expand the filters test around queryRequestHistory to add
distinct fixture rows and focused assertions for model, requestedModel,
inboundProtocol, apiKeyId, profileId, and fallback, ensuring each query selects
only its intended row. Keep the existing provider, status, conversationId,
surface, and date-range checks, and use the request-history filter fields
forwarded by the route.
---
Duplicate comments:
In `@src/routing/history/indexer.ts`:
- Around line 295-318: Update destroyAndRecreate to remove the SQLite WAL and
SHM sidecar files, alongside the main database path, before constructing the
replacement Database. Update indexDbPath to register both sidecar paths with
recordOwnedConfigPath so ownership cleanup removes them as well.
- Around line 128-133: Update sourceIdentityMatches to compare only source
identity fields—sourcePath, sourceDev, sourceIno, and sourceBirthtimeMs—while
leaving sourceSize and sourceMtimeMs to the offset/truncation logic in
refreshLocked. Ensure the fresh-database branch of openIndexDb records these
identity fields via the existing metadata mechanism so the initial refresh does
not trigger an unnecessary rebuild.
- Around line 523-528: Update the nextCursor logic in queryRequestHistory to
avoid re-parsing pageRows[last].row_json and instead use the row’s indexed
timestamp and request_id values directly when calling encodeHistoryCursor.
Preserve the existing hasMore and non-empty page guard so malformed JSON skipped
by hydrateRow cannot cause pagination to throw.
- Around line 181-197: Refactor readCompleteTail and the ingestSourceTail path
to read the ledger in bounded chunks instead of allocating the full file and
splitting it at once. Preserve partial trailing-line data across chunks, emit
complete lines for ingestion, and ensure newline-free chunks expand or otherwise
advance the read window so lines longer than the chunk size do not stall. Update
the request-history flow to use this incremental ingestion while preserving
offsets and existing rebuild behavior.
- Around line 500-515: Update the request-history index refresh flow so it
returns the validated database handle alongside its metadata, then use that
returned handle in queryRequestHistory and requestHistoryRowById instead of
re-reading the nullable db singleton after await. Preserve
openRequestHistoryIndex as the existing metadata-only public wrapper, while
ensuring both callers use the handle obtained during refresh.
In `@src/routing/history/schema.ts`:
- Around line 70-72: Update indexDbPath in the history indexer to import and use
historyIndexPath for both index-path constructions instead of interpolating
HISTORY_DB_FILENAME directly. Preserve the existing directory inputs and
recordOwnedConfigPath behavior while ensuring all callers use the normalized
path helper.
In `@src/server/management/request-history-routes.ts`:
- Around line 92-96: Guard the requestId decoding in the GET request-history
route so malformed percent-encoded path segments are treated as unknown requests
and return the existing 404 response instead of throwing. Update the route logic
around requestId and add a regression test in the request-history index tests
for GET /api/request-history/% asserting status 404.
- Around line 23-27: Update parseOptionalInt to distinguish absent, valid, and
present-but-invalid values, returning the established INVALID result for
non-integer or empty-string input while preserving undefined for null. Update
the GET /api/request-history validation for status, from, to, and limit to
reject INVALID with a stable 400, and pass limit directly through
parseOptionalInt. Add focused regression tests in the existing request-history
validation tests for invalid status, limit, and from query parameters, each
asserting 400.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 40b03b6d-416d-471f-bc7e-b70de4f735bc
📒 Files selected for processing (9)
devlog/_plan/260804_router_intelligence/001_pr_stack_status.mdsrc/cli/observe.tssrc/routing/history/cursor.tssrc/routing/history/indexer.tssrc/routing/history/schema.tssrc/server/management-api.tssrc/server/management/request-history-routes.tstests/request-history-index.test.tstests/route-decision-trace.test.ts
[GD] Verdict: approve-commentTLDR
Full verdictSemantic propagation
Bugs / correctness
SimplifyApplied trivial rename; declined streaming rebuild, cross-process lock, and shared |
|
[GD] Merged — why it helps: RI-02 adds a rebuildable SQLite projection over usage.jsonl with cursor-paginated /api/request-history and CLI index commands (ocx logs rebuild-index, index-status). This keeps usage.jsonl canonical while enabling efficient history queries without mutating the ledger. |
1 participant
Summary
RI-02 of the Router Intelligence / Routing Control Plane programme
(
devlog/_plan/260804_router_intelligence/000_master_plan.md). Adds arebuildable SQLite projection of the canonical
usage.jsonlledger plus acursor-paginated request-history API.
usage.jsonlremains the canonical, append-only request evidence. The index(
routing-history.sqlitein the config dir, Bun's built-inbun:sqlite) is aderived query index that can be deleted, corrupted, or stale at any time and
is automatically rebuilt from the ledger without losing history (ADR-1/ADR-8).
Scope
src/routing/history/schema.ts- versioned schema (schema_meta+requestswith filter indexes),HISTORY_SCHEMA_VERSION = 1.src/routing/history/cursor.ts- opaque base64url keyset cursor(
timestamp DESC, request_id DESC),InvalidCursorError.src/routing/history/indexer.ts- open/append/rebuild/repair:PRAGMA quick_check, source file identity(size/mtime), byte-offset incremental append;
JSONL line (skipped until it completes), file replacement/truncation
(identity change -> rebuild), duplicate replay (
INSERT OR IGNORE),crash during indexing (transactional 500-row batches, WAL);
queryRequestHistory,requestHistoryRowById,rebuildRequestHistoryIndex.src/server/management/request-history-routes.ts-GET /api/request-history- filters:provider,model,requestedModel,status,conversationId,surface,inboundProtocol,apiKeyId,profileId,fallback,from,to;limit(1..100),cursor; response carriesentries,nextCursor,hasMore, and anindexstatus block (schemaVersion, indexedRows,sourceSize, sourceMtimeMs, builtAtMs, lastError).
GET /api/request-history/:requestId- one canonical row (404 unknown).ocx logs rebuild-index,ocx logs index-status(run directlyagainst the ledger; no running server needed).
Compatibility
usage.jsonluntouched;/api/logscontract unchanged.ocx logs rebuild-indexforces one explicitly.
Privacy / security
secret surface (same config dir,
0o600semantics, local file).by RI-01 and the usage-log normalizer); no prompts or credentials.
400with stable error codes.bun run privacy:scanpasses.Dependency
feat/ri-01-route-decision-traceshead
b5a8e7c4c. The PR target isdev; while feat(routing): record durable route decision traces #1003 is open the diff alsocontains RI-01's commits, which shrink automatically after feat(routing): record durable route decision traces #1003 merges.
Non-goals
/api/logsor the in-memory ring buffer.Local verification (exact)
bun x tsc --noEmit-> PASSED (0 errors)bun run test tests/request-history-index.test.ts-> 16/16 pass(1574 assertions) covering: empty/missing/corrupt/old-schema/partial-line/
replacement/truncation/duplicate-replay/cursor-stability/invalid-cursor/
page-bounds/rebuild-equivalence/filters/row-by-id
combo-management-api, codex-routing, codex-account-namespaces) -> 269/269
bun run privacy:scan-> passedupstream/devruns out-of-band; resultrecorded in the stack ledger.
Notes for reviewers
LIMIT/INSERT silently mis-bind (positional parameters used everywhere);unfinalized prepared statements keep the DB locked on Windows after close.
Summary by CodeRabbit
New Features
Bug Fixes