feat(session-log): switch to local lazy dev log files - #40
Conversation
|
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 29 minutes and 1 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: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR introduces structured background trace logging with AI diagnostic data capture, adds transient error handling with session manager retry logic, and propagates AI traces through conversation routing and RAG orchestration. Session identifiers now use local DD-MM-HH-mm format, and logging payloads shift from plain strings to discriminated union objects supporting both notes and AI trace data. Changes
Sequence Diagram(s)sequenceDiagram
participant Chat as Chat Request
participant Orch as Orchestrator
participant Rewrite as Rewrite Service
participant Provider as LLM Provider
participant Trace as Trace Builder
participant Log as Session Log
Chat->>Orch: query with context
Orch->>Rewrite: attempt retrieval rewrite
Rewrite->>Provider: rewrite request
Provider-->>Rewrite: rewrite response
Rewrite->>Trace: buildRetrievalRewriteTrace(request, response)
Trace-->>Rewrite: RetrievalRewriteTrace (systemPrompt, provider, usage, apiResult)
Rewrite-->>Orch: RetrievalRewriteAttempt with trace
Orch->>Provider: answer generation request
Provider-->>Orch: provider response (text, usage)
Orch->>Trace: toAnswerGenerationAiTrace(systemPrompt, grounding, provider, usage, apiResult)
Trace-->>Orch: CatalogChatAiTrace (ai.answer_generation)
Orch->>Log: appendConversationSessionLogAiTracesSafely(aiTraces)
Log-->>Log: write bts entries with payload.kind="ai"
Orch-->>Chat: CatalogChatResult with aiTraces
sequenceDiagram
participant Boot as Bot Startup
participant Mgr as Session Manager
participant Retry as Retry Handler
participant Store as Store (Convex)
participant Logger as Logger
Boot->>Mgr: initialize
Mgr->>Retry: retryInitialSessionReconcile(reconcileManagedSessions, onRetry)
Retry->>Store: reconcile managed sessions (attempt 1)
Store-->>Retry: transient error (UND_ERR_CONNECT_TIMEOUT)
Retry->>Logger: warn bot.session.initial_reconcile_retry_scheduled (attempt 2, delay 250ms)
Retry->>Retry: sleep(250)
Retry->>Store: reconcile managed sessions (attempt 2)
Store-->>Retry: transient error (socket closed)
Retry->>Logger: warn bot.session.initial_reconcile_retry_scheduled (attempt 3, delay 500ms)
Retry->>Retry: sleep(500)
Retry->>Store: reconcile managed sessions (attempt 3)
Store-->>Retry: success
Retry-->>Mgr: resolved
Mgr-->>Boot: ready
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/core/src/conversationSessionLog.test.ts`:
- Around line 14-21: The session id produced by
createConversationSessionLogSessionId uses DD-MM-HH-mm which can collide for
sessions started in the same minute and cause logs at paths built by
createConversationSessionLogSessionPath to be commingled or overwritten; update
createConversationSessionLogSessionId to increase uniqueness (e.g., include
seconds to produce DD-MM-HH-mm-ss or append a short random suffix) and ensure
downstream code that relies on the format
(createConversationSessionLogSessionPath) still composes file names correctly or
is updated to accept the extended id format; alternatively, if
minute-granularity is intentional, add a docstring comment on
createConversationSessionLogSessionId and
createConversationSessionLogSessionPath to document the behavior.
In `@packages/core/src/conversationSessionLog.ts`:
- Line 1: When mkdir throws EEXIST and the code proceeds to appendFile, validate
that the existing file belongs to the same conversation/company by reading and
parsing its header before appending; if the header does not match the in-memory
conversationIdentity, fail closed (throw or return error) instead of appending.
Update the logic in conversationSessionLog (the code paths using mkdir,
appendFile, writeFile and the header-write routine referenced around lines 1 and
165-171) to: on EEXIST read the existing file, parse/compare its header to
conversationIdentity, and only call appendFile when it matches; otherwise abort
and surface an error.
- Around line 75-80: The session ID generator in conversationSessionLog.ts
currently builds IDs with toTwoDigits(value.getDate()),
toTwoDigits(value.getMonth() + 1), toTwoDigits(value.getHours()), and
toTwoDigits(value.getMinutes()), which can collide for sessions started in the
same minute; modify the generator (the code calling toTwoDigits and producing
the sessionId used in `${sessionId}.md`) to append higher-precision time
(seconds and/or milliseconds via value.getSeconds() and value.getMilliseconds())
or a short random suffix (e.g., 4 hex chars) while preserving the readable
DD-MM-HH-mm prefix so each sessionId is unique even on quick restarts.
In `@packages/rag/src/catalogChatAiTrace.ts`:
- Around line 13-38: The Catalog AI trace builders inconsistently include usage:
toCatalogChatAiTrace and toAnswerGenerationAiTrace always set usage (potentially
as undefined); change both factories to only include usage when defined by
conditionally spreading it like groundingContext is handled so that
CatalogChatAiTrace objects do not contain an explicit usage: undefined property
— update toCatalogChatAiTrace and toAnswerGenerationAiTrace to check trace.usage
/ input.usage and spread { usage: ... } only when present.
In `@packages/rag/src/retrievalRewriteTrace.ts`:
- Around line 10-22: The extractSystemPromptFromRequest arrow function
unnecessarily wraps its logic in an IIFE; remove the immediate-invoked function
and make the body direct, and when handling request.messages (ChatRequest) avoid
mapping non-text parts to empty strings—filter parts that have "text" first and
then join their text fields so you don't generate extra blank lines; update the
function that inspects request.messages and the conditional handling of content
accordingly.
In `@scripts/dev-session-log.ts`:
- Around line 12-16: The session file names created by
createConversationSessionLogSessionPath use the minute-level ID from
createConversationSessionLogSessionId, causing collisions for runs started
within the same minute; update the session ID generation to append a compact
unique suffix (e.g., input.now.getTime() or input.now.toISOString() millis + a
short random or process.pid/crypto.randomUUID() fragment) while preserving the
human-readable prefix produced by createConversationSessionLogSessionId, or
alter createConversationSessionLogSessionPath to tack such a unique token onto
the filename it builds so each run yields a unique <prefix>-<unique> session
path.
🪄 Autofix (Beta)
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: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4d7db13d-fa7e-4401-be94-72d1d0eb4fb2
📒 Files selected for processing (23)
CONTEXT.mdapps/bot/src/customerConversationLogHelpers.tsapps/bot/src/customerConversationRouter.test.tsapps/bot/src/customerConversationRouter.tsapps/bot/src/sessionManager.test.tsapps/bot/src/sessionManager.tsapps/bot/src/sessionManagerStartupRetry.tsapps/worker/src/pendingAssistantReconciliation.test.tsapps/worker/src/pendingAssistantSessionLog.tspackages/core/src/conversationSessionLog.test.tspackages/core/src/conversationSessionLog.tspackages/core/src/conversationSessionLogBackgroundPayload.tspackages/core/src/convexTransport.test.tspackages/core/src/convexTransport.tspackages/rag/src/catalogChat.test.tspackages/rag/src/catalogChatAiTrace.tspackages/rag/src/catalogChatLogging.tspackages/rag/src/index.tspackages/rag/src/retrievalRewrite.test.tspackages/rag/src/retrievalRewrite.tspackages/rag/src/retrievalRewriteTrace.tsscripts/dev-session-log.test.tsscripts/dev-session-log.ts
💤 Files with no reviewable changes (1)
- CONTEXT.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (2)
**/*.ts
📄 CodeRabbit inference engine (AGENTS.md)
**/*.ts: For core logic.tsfiles, do not add a new responsibility to an existing file when it should be a sibling module
No new core logic.tsfile may exceed240LOC without an explicit entry inmodularity-policy.json
If a core logic file already exceeds240LOC, it must not grow unless itsmodularity-policy.jsonentry is intentionally updated with rationale
Files classified asmust_splitinmodularity-policy.jsonare debt containers: patches are allowed, but adding unrelated responsibilities is not
Files:
scripts/dev-session-log.tsapps/bot/src/sessionManager.test.tsapps/bot/src/sessionManager.tspackages/core/src/convexTransport.test.tsapps/bot/src/customerConversationRouter.test.tsapps/bot/src/customerConversationRouter.tsscripts/dev-session-log.test.tsapps/bot/src/sessionManagerStartupRetry.tspackages/core/src/convexTransport.tspackages/rag/src/retrievalRewriteTrace.tspackages/rag/src/retrievalRewrite.test.tsapps/bot/src/customerConversationLogHelpers.tspackages/rag/src/catalogChat.test.tsapps/worker/src/pendingAssistantReconciliation.test.tspackages/rag/src/catalogChatAiTrace.tsapps/worker/src/pendingAssistantSessionLog.tspackages/rag/src/retrievalRewrite.tspackages/core/src/conversationSessionLog.tspackages/rag/src/catalogChatLogging.tspackages/core/src/conversationSessionLog.test.tspackages/rag/src/index.tspackages/core/src/conversationSessionLogBackgroundPayload.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use the
@cs/*path aliases fromtsconfig.base.jsonfor cross-package imports
Files:
scripts/dev-session-log.tsapps/bot/src/sessionManager.test.tsapps/bot/src/sessionManager.tspackages/core/src/convexTransport.test.tsapps/bot/src/customerConversationRouter.test.tsapps/bot/src/customerConversationRouter.tsscripts/dev-session-log.test.tsapps/bot/src/sessionManagerStartupRetry.tspackages/core/src/convexTransport.tspackages/rag/src/retrievalRewriteTrace.tspackages/rag/src/retrievalRewrite.test.tsapps/bot/src/customerConversationLogHelpers.tspackages/rag/src/catalogChat.test.tsapps/worker/src/pendingAssistantReconciliation.test.tspackages/rag/src/catalogChatAiTrace.tsapps/worker/src/pendingAssistantSessionLog.tspackages/rag/src/retrievalRewrite.tspackages/core/src/conversationSessionLog.tspackages/rag/src/catalogChatLogging.tspackages/core/src/conversationSessionLog.test.tspackages/rag/src/index.tspackages/core/src/conversationSessionLogBackgroundPayload.ts
🔇 Additional comments (9)
packages/core/src/convexTransport.ts (1)
1-58: LGTM — cause-chain traversal is bounded and safe.
getErrorChaincaps traversal atMAX_CAUSE_DEPTH = 6, so a cycliccausereference cannot infinite-loop. The addedUND_ERR_CONNECT_TIMEOUTcode and the three substring patterns correctly cover the undici/Convex connectivity failures that should be retried. Pattern matching is substring-based on specific, unambiguous strings, so false-positive risk is low.packages/core/src/convexTransport.test.ts (1)
11-55: LGTM — coverage matches the new transient cases.The three added cases (connectivity message,
UND_ERR_CONNECT_TIMEOUTcode, and a nestedcausechain) exercise each branch added toisTransientConvexTransportError.apps/bot/src/sessionManagerStartupRetry.ts (1)
10-36: LGTM — retry loop terminates deterministically.The
for (;;)loop is bounded in practice: onceattemptexceedsINITIAL_RECONCILE_RETRY_DELAYS_MS.length,retryDelayMsisundefinedand the caught error is rethrown. Delays[250, 500, 1000]cap startup latency at ~1.75s before falling through to the heartbeat retry path, which is a reasonable tradeoff.Minor observation (non-blocking): there is no cancellation signal, so a SIGINT during the retry window cannot short-circuit the
sleep. Given the sub-2-second worst case and that process signal handlers are installed bystartTenantSessionManageronly after this returns, this is acceptable.apps/bot/src/sessionManager.ts (1)
277-277: LGTM — bound logger and retry wiring are consistent.
sessionManagerLoggeris bound with{ runtimeOwnerId, surface: "session_manager" }and reused for the retry-scheduled warn (line 941), the final initial-reconcile error (line 953), and the ongoing reconcile failure (line 892), giving a uniform surface in logs. The explicitruntimeOwnerId/surfacefields in each payload are redundant with the bindings but preserve the existing payload contract that tests rely on.The catch block intentionally swallows the terminal error so the heartbeat loop can keep attempting reconciliation — consistent with the pre-existing "continues manager startup after the initial reconcile fails" behavior.
Also applies to: 939-961
apps/bot/src/sessionManager.test.ts (1)
967-1031: LGTM — test correctly validates the transient-retry path.The test uses
new Error("fetch failed", { cause: new Error("Connect Timeout Error ...") })to exercise the cause-chain detection added inconvexTransport.ts, asserts thebot.session.initial_reconcile_retry_scheduledwarn payload (withretryDelayMs: 250), and asserts thatbot.session.initial_reconcile_failedis not emitted on the transient path.Minor note (non-blocking): because
sleep()inretryInitialSessionReconcileuses the realsetTimeout, this test adds ~250 ms of real wall-clock latency per run. If retry-delay timing becomes a test-performance concern later, consider injecting asleep/clock intoretryInitialSessionReconcile.apps/bot/src/customerConversationLogHelpers.ts (1)
98-107: No action needed —apiResultis already properly declared and rendered in the core payload type.The
ConversationSessionLogAiPayloadinterface inpackages/core/src/conversationSessionLogBackgroundPayload.tsalready includesapiResult: unknown(line 12), and the markdown renderer already explicitly outputs it (line 66). The helper function correctly passes the field through without any type mismatch.> Likely an incorrect or invalid review comment.apps/bot/src/customerConversationRouter.ts (1)
181-181: Modularity-policy exception already exists with explicit rationale—revert this comment.The
modularity-policy.jsonentry for this file explicitly setsmaxLines: 570with documented reason: "Couples persistence, orchestration, delivery, analytics, and handoff side effects." The file is at its allowed limit, not exceeding it. Line 181 delegates AI trace persistence to a helper module (customerConversationLogHelpers), extending the existing session-log persistence concern rather than introducing a new unrelated responsibility. No further action required.> Likely an incorrect or invalid review comment.packages/core/src/conversationSessionLog.test.ts (2)
107-110: No issue found. The functionformatConversationSessionLogTimestampproperly accepts bothDateandnumbertypes, as shown by its signature(input: Date | number): stringand implementation that converts numeric inputs toDateobjects. The calls at lines 107-110 with epoch-millisecond timestamps are type-safe and correct.> Likely an incorrect or invalid review comment.
41-48: Implementation is timezone-stable.The
formatConversationSessionLogTimestampimplementation correctly derives hour/minute/second fromDate.prototype.getHours(),getMinutes(), andgetSeconds()(local time), matching howcreateConversationSessionLogSessionIdformats components. Tests will not fail across timezones.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Summary by CodeRabbit
New Features
Improvements