fix(observe): auto-create session row + surface session/start HTTP errors (closes #522)#576
Open
rohitg00 wants to merge 1 commit into
Open
fix(observe): auto-create session row + surface session/start HTTP errors (closes #522)#576rohitg00 wants to merge 1 commit into
rohitg00 wants to merge 1 commit into
Conversation
…rors Closes a silent data-loss path: when an agent hook calls POST /agentmemory/observe before (or instead of) a successful POST /agentmemory/session/start, observations were written to KV.observations(sessionId) but KV.sessions never got the parent row. GET /agentmemory/sessions then returned an empty list even though the underlying observation files were on disk. Two complementary changes: 1. Server-side (src/functions/observe.ts) — when observe lands and the session row is missing, insert it inline using the project / cwd / timestamp / first prompt already present on the payload. session/start remains the fast path (it also returns the inject-context block) but is no longer load-bearing for durability. 2. Hook-side (src/hooks/session-start.ts) — explicit stderr write when session/start returns a non-2xx status, on both the fire-and-forget and inject-context paths. Network / timeout errors stay quiet (common on cold-start); HTTP errors surface so wiring problems (wrong URL, auth, etc.) become visible to the user. The compiled plugin/scripts/session-start.mjs is regenerated by tsdown. Tests (1081) + build pass.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThis PR fixes session creation and error visibility. When observations arrive without a corresponding session, the observe function now auto-creates the session row with metadata from the observation context. Additionally, HTTP failures from session/start requests are now logged to stderr in hook and plugin execution paths, surfacing errors that were previously silent. ChangesSession auto-creation and error observability
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/functions/observe.ts (1)
45-58:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winAdd validation for
payload.projectandpayload.cwd.The auto-create session logic (lines 232-233) uses
payload.projectandpayload.cwdwithout validation. If these fields are missing, undefined, or non-string, the session row will be corrupted. As per coding guidelines, REST endpoints must perform input validation at system boundaries.🛡️ Proposed fix to validate project and cwd
if ( !payload?.sessionId || typeof payload.sessionId !== "string" || !payload.hookType || typeof payload.hookType !== "string" || !payload.timestamp || - typeof payload.timestamp !== "string" + typeof payload.timestamp !== "string" || + (payload.project !== undefined && typeof payload.project !== "string") || + (payload.cwd !== undefined && typeof payload.cwd !== "string") ) { return { success: false, error: - "Invalid payload: sessionId, hookType, and timestamp are required", + "Invalid payload: sessionId, hookType, and timestamp are required; project and cwd must be strings if present", }; }🤖 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/functions/observe.ts` around lines 45 - 58, The input validation block in observe.ts currently checks sessionId, hookType, and timestamp but misses payload.project and payload.cwd which are later used by the auto-create session logic (the code that creates a new session using payload.project and payload.cwd); update the same validation to also require payload.project and payload.cwd to be present and of type string, and adjust the returned error message to mention project and cwd (e.g., "Invalid payload: sessionId, hookType, timestamp, project, and cwd are required") so the endpoint rejects bad requests before the session-creation code runs.
🧹 Nitpick comments (2)
src/functions/observe.ts (2)
206-214: ⚡ Quick winExtract firstPrompt normalization logic to avoid duplication.
The firstPrompt normalization logic (whitespace collapse, trim, slice to 200 chars) is duplicated between the session-update path and the session-create path. Extract it to a helper function or local variable.
♻️ Proposed refactor to eliminate duplication
+// Helper: normalize user prompt for firstPrompt storage +const normalizeFirstPrompt = (prompt: unknown): string | undefined => { + if (typeof prompt !== "string") return undefined; + const trimmed = prompt.replace(/\s+/g, " ").trim(); + return trimmed.length > 0 ? trimmed.slice(0, 200) : undefined; +}; + const session = await kv.get<{ observationCount?: number; firstPrompt?: string }>( KV.sessions, payload.sessionId, ); if (session) { const updates: Array<{ type: "set"; path: string; value: unknown }> = [ { type: "set", path: "updatedAt", value: new Date().toISOString() }, { type: "set", path: "observationCount", value: (session.observationCount || 0) + 1, }, ]; - if (!session.firstPrompt && typeof raw.userPrompt === "string") { - const trimmed = raw.userPrompt.replace(/\s+/g, " ").trim(); - if (trimmed.length > 0) { - updates.push({ - type: "set", - path: "firstPrompt", - value: trimmed.slice(0, 200), - }); - } + const normalized = normalizeFirstPrompt(raw.userPrompt); + if (!session.firstPrompt && normalized) { + updates.push({ + type: "set", + path: "firstPrompt", + value: normalized, + }); } await kv.update(KV.sessions, payload.sessionId, updates); } else { - const firstPrompt = - typeof raw.userPrompt === "string" - ? raw.userPrompt.replace(/\s+/g, " ").trim().slice(0, 200) - : undefined; + const firstPrompt = normalizeFirstPrompt(raw.userPrompt); await kv.set(KV.sessions, payload.sessionId, { id: payload.sessionId, project: payload.project, cwd: payload.cwd, startedAt: payload.timestamp, status: "active", observationCount: 1, ...(firstPrompt ? { firstPrompt } : {}), }); }Also applies to: 226-229
🤖 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/functions/observe.ts` around lines 206 - 214, The whitespace collapse/trim/length-check/slice(0,200) logic for creating firstPrompt is duplicated; extract it into a small helper (e.g., normalizePrompt or normalizeUserPrompt) and call it from both places where you set session.firstPrompt (the session-update branch checking session.firstPrompt and raw.userPrompt and the session-create branch that also handles raw.userPrompt), returning either a normalized string or null/empty if nothing remains; then replace the inline logic with a call to the helper and push the existing update object only when the helper returns a non-empty value.
218-225: 💤 Low valueSimplify comment to focus on intent rather than implementation.
The comment explains implementation details and history that are better captured in the PR description or commit message. As per coding guidelines, avoid comments explaining WHAT — use clear naming instead. Focus on the essential rationale.
📝 Suggested simplification
-// Auto-create the session row when an observation arrives before -// (or instead of) a successful session/start call. The hook -// scripts call session/start as fire-and-forget; on transient -// server unavailability that POST is silently dropped, leaving -// observations orphaned from GET /agentmemory/sessions even -// though they're present on disk. Re-creating here closes the -// data-loss hole permanently; session/start remains the fast -// path (it also returns the injected context). +// Auto-create session if observe arrives before session/start succeeds.🤖 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/functions/observe.ts` around lines 218 - 225, Replace the long explanatory comment that begins with "// Auto-create the session row when an observation arrives..." with a concise intent-focused line stating why the code creates a session row (e.g., "Ensure a session row exists when observations arrive without a prior session/start to prevent orphaned observations"). Remove historical/implementation details and references to transient server behavior; keep only the essential rationale for the creation behavior so the code intent is clear.
🤖 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/functions/observe.ts`:
- Around line 230-238: The auto-create branch that calls kv.set for KV.sessions
(key payload.sessionId) omits updatedAt, causing inconsistent session records;
update the object passed to kv.set in observe.ts (the auto-create block that
sets id, project, cwd, startedAt, status, observationCount, and optionally
firstPrompt) to include updatedAt (use payload.timestamp or the same timestamp
used for startedAt) so newly-created sessions have the same schema as the update
path.
---
Outside diff comments:
In `@src/functions/observe.ts`:
- Around line 45-58: The input validation block in observe.ts currently checks
sessionId, hookType, and timestamp but misses payload.project and payload.cwd
which are later used by the auto-create session logic (the code that creates a
new session using payload.project and payload.cwd); update the same validation
to also require payload.project and payload.cwd to be present and of type
string, and adjust the returned error message to mention project and cwd (e.g.,
"Invalid payload: sessionId, hookType, timestamp, project, and cwd are
required") so the endpoint rejects bad requests before the session-creation code
runs.
---
Nitpick comments:
In `@src/functions/observe.ts`:
- Around line 206-214: The whitespace collapse/trim/length-check/slice(0,200)
logic for creating firstPrompt is duplicated; extract it into a small helper
(e.g., normalizePrompt or normalizeUserPrompt) and call it from both places
where you set session.firstPrompt (the session-update branch checking
session.firstPrompt and raw.userPrompt and the session-create branch that also
handles raw.userPrompt), returning either a normalized string or null/empty if
nothing remains; then replace the inline logic with a call to the helper and
push the existing update object only when the helper returns a non-empty value.
- Around line 218-225: Replace the long explanatory comment that begins with "//
Auto-create the session row when an observation arrives..." with a concise
intent-focused line stating why the code creates a session row (e.g., "Ensure a
session row exists when observations arrive without a prior session/start to
prevent orphaned observations"). Remove historical/implementation details and
references to transient server behavior; keep only the essential rationale for
the creation behavior so the code intent is clear.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5d3b23a5-0d27-4108-ac5e-a93616898a1f
📒 Files selected for processing (3)
plugin/scripts/session-start.mjssrc/functions/observe.tssrc/hooks/session-start.ts
Comment on lines
+230
to
+238
| await kv.set(KV.sessions, payload.sessionId, { | ||
| id: payload.sessionId, | ||
| project: payload.project, | ||
| cwd: payload.cwd, | ||
| startedAt: payload.timestamp, | ||
| status: "active", | ||
| observationCount: 1, | ||
| ...(firstPrompt ? { firstPrompt } : {}), | ||
| }); |
There was a problem hiding this comment.
Include updatedAt field when auto-creating sessions.
The existing session update path sets updatedAt (line 199), but the new auto-create path omits it. This creates inconsistent session records where some have updatedAt and others don't, complicating queries and violating the principle of uniform schema.
🔧 Proposed fix to add updatedAt
+const now = new Date().toISOString();
await kv.set(KV.sessions, payload.sessionId, {
id: payload.sessionId,
project: payload.project,
cwd: payload.cwd,
startedAt: payload.timestamp,
+ updatedAt: now,
status: "active",
observationCount: 1,
...(firstPrompt ? { firstPrompt } : {}),
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| await kv.set(KV.sessions, payload.sessionId, { | |
| id: payload.sessionId, | |
| project: payload.project, | |
| cwd: payload.cwd, | |
| startedAt: payload.timestamp, | |
| status: "active", | |
| observationCount: 1, | |
| ...(firstPrompt ? { firstPrompt } : {}), | |
| }); | |
| const now = new Date().toISOString(); | |
| await kv.set(KV.sessions, payload.sessionId, { | |
| id: payload.sessionId, | |
| project: payload.project, | |
| cwd: payload.cwd, | |
| startedAt: payload.timestamp, | |
| updatedAt: now, | |
| status: "active", | |
| observationCount: 1, | |
| ...(firstPrompt ? { firstPrompt } : {}), | |
| }); |
🤖 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/functions/observe.ts` around lines 230 - 238, The auto-create branch that
calls kv.set for KV.sessions (key payload.sessionId) omits updatedAt, causing
inconsistent session records; update the object passed to kv.set in observe.ts
(the auto-create block that sets id, project, cwd, startedAt, status,
observationCount, and optionally firstPrompt) to include updatedAt (use
payload.timestamp or the same timestamp used for startedAt) so newly-created
sessions have the same schema as the update path.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #522.
Silent data-loss path: when a client posts to
POST /agentmemory/observebefore (or instead of) a successfulPOST /agentmemory/session/start, observations land inKV.observations(sessionId)butKV.sessionsnever gets the parent row.GET /agentmemory/sessionsthen returns an empty list even though the underlying observation files are on disk.Root cause from the issue + confirmed by @huyzhui222:
session-start.mjsPOSTs/agentmemory/session/startas fire-and-forget (fetch(...).catch(() => {})).observestill writes its observation, leaving orphaned rows.Two complementary changes
1. Server-side:
observeupserts the session row if missingsrc/functions/observe.ts— whenobservelands andkv.get(KV.sessions, sessionId)returns nothing, insert it inline using theproject/cwd/timestamp/ first-prompt fields already present on the payload.session/startstays the fast path (it also returns the inject-context block) but is no longer load-bearing for durability.2. Hook-side: surface HTTP errors
src/hooks/session-start.ts— explicitprocess.stderr.writewhensession/startreturns a non-2xx status, on both the fire-and-forget and inject-context paths. Network/timeout errors stay quiet (common on cold-start); HTTP errors surface so wiring problems (wrong URL, auth, etc.) become visible. The compiledplugin/scripts/session-start.mjsis regenerated by tsdown.Diff
src/functions/observe.ts—+22server-side auto-upsert in theelsebranch where session is missingsrc/hooks/session-start.ts—+24/-3.then(res => res.ok || log)on both pathsplugin/scripts/session-start.mjs—+3/-1regenerated by tsdownTotal 46 insertions, 4 deletions across 3 files.
Validation
npm test→ 97/97 test files, 1081/1081 tests passnpm run build→ bundle cleanBackwards compatibility
session/startkeeps returning the same response shape.elsebranch (session missing) gains behavior.Summary by CodeRabbit
Bug Fixes
Improvements