Skip to content

fix(store): derive comment sessionId from its surface#94

Closed
benvinegar wants to merge 1 commit into
mainfrom
fix/sqlstore-comment-session-id
Closed

fix(store): derive comment sessionId from its surface#94
benvinegar wants to merge 1 commit into
mainfrom
fix/sqlstore-comment-session-id

Conversation

@benvinegar

Copy link
Copy Markdown
Member

What

createComment stored the caller's sessionId verbatim instead of deriving it from the surface the comment attaches to. A comment could land in a session that doesn't own its surface, breaking listComments joins and the unread/aggregation logic.

The HTTP/MCP flow happened to pass surface.sessionId, so it was safe today; any future caller of the Store interface could split them.

Fix

Both JsonFileStore and SqlStore now resolve the surface first and derive sessionId from it, falling back to input.sessionId only when no surface (or an unknown one) is provided.

Test

Added a store contract test (a comment's session follows its surface, not the caller's sessionId) that creates a surface in session A, then posts a comment with session B's id and the surface's id. Asserts the comment is filed under A, not B. Fails on both stores before the fix; passes after.

Validation

  • npm test — 184 pass
  • npm run typecheck — clean
  • npm run lint — clean

createComment stored the caller's sessionId verbatim instead of
deriving it from the surface the comment attaches to. A comment could
land in a session that doesn't own its surface, breaking listComments
joins and the unread/aggregation logic. The HTTP/MCP flow happened to
pass surface.sessionId, so it was safe today; any future caller of the
Store interface could split them.

Both JsonFileStore and SqlStore now resolve the surface first and
derive sessionId from it, falling back to input.sessionId only when no
surface (or an unknown one) is provided.
@benvinegar

Copy link
Copy Markdown
Member Author

Closing for now. Verified the change is correct, but it's defensive hardening of an unreachable path rather than a live bug: the only caller of store.createComment (the flow function in server/app.ts, which the HTTP route and MCP path both funnel through) always passes sessionId: surface.sessionId, so input.sessionId and the surface's session are already identical in every real call path. The mismatch the test exercises can't be constructed via any HTTP/MCP/CLI entrypoint. Happy to revisit if we ever add a caller that sets them independently.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant