Skip to content

chat: visibility model + integrations polish + templates sync + Hermes session fix#3

Closed
undeemed wants to merge 3 commits into
mainfrom
claude/pensive-gagarin-17d4ca
Closed

chat: visibility model + integrations polish + templates sync + Hermes session fix#3
undeemed wants to merge 3 commits into
mainfrom
claude/pensive-gagarin-17d4ca

Conversation

@undeemed

@undeemed undeemed commented May 26, 2026

Copy link
Copy Markdown
Member

Summary

Bundles a connected set of fixes from one user ask.

Auth + chat visibility

  • agent_conversations gains visibility (personal|shared) + owner_user_id + indexes
  • Every direct-id mutation/query auth-gated via authComponent.safeGetAuthUser
  • bindSession hardened with per-turn write-token + expected_hermes_session race-guard
  • setVisibility refuses mid-turn (the running Hermes task is bound to the OLD session via its kickoff payload; re-minting under it could leak personal-memory into a now-shared thread)
  • Hermes session prefix split (castle-personal-<owner>-… vs castle-shared-…); visibility-aware system preamble
  • Real-bug fix: Hermes wrapper now calls bindSession back to Convex when minting a new session — was the root cause of "new context every turn"
  • Personal/Shared collapsible sidebar sections + per-chat "make shared / make personal" toggle
  • Opportunistic legacy-row claim (hoisted to ChatProvider so it runs on every page, not just /)
  • deleteConversation now cascade-deletes turns, chunks, and tool events (no more orphan rows)

Integrations + composer polish

  • Right rail: real Composio toolkit logos + filled-check / pulsing-dot status icons
  • Connect-button reflow killed (CTA only attaches once the new assistant message exists past the action timestamp)
  • Per-conversation localStorage drafts
  • Hover-revealed relative timestamps + copy button
  • regenerate (new /api/agent/regenerate + agentTurns.regenerateLast) re-runs the last turn against the same Hermes session — now carries the original attachments forward too
  • File attachments: paperclip → Convex storage → URLs forwarded to Hermes via kickoff body → chips render on user messages
  • attachmentUrl gated by message_id + verifies the storageId is actually on that message (defense in depth: a caller with access to convo A can't pass A's id but ask for B's storageId)
  • Send button blocked while attachments are still uploading

Templates auto-sync

  • New template_github_candidates staging table (no relaxation of templates)
  • /api/templates/sync-github accepts both Better Auth sessions and Vercel cron's CRON_SECRET bearer header
  • Auth gates on listGithubCandidates / dismissGithubCandidate / markCandidatePromoted; upsertGithubCandidate stays open so the cron (no operator identity) can write
  • vercel.json cron: 0 7 * * *daily, not hourly (Vercel Hobby tier caps at one cron job per day)
  • "Pending review" candidates section on /templates with manual "sync from GitHub" button
  • Shared normalizeGithubRepo helper so candidates and templates dedupe consistently (Owner/Repo and owner/repo collapse)

Sticky auto-scroll

  • Distance-from-bottom listener with 80px threshold; "↓ jump to latest" pill is sticky inside the scroll container (won't collide with the 200px-max composer)

Env vars needed in Vercel

  • CRON_SECRET — gates the daily templates sync
  • CASTLE_SYSTEM_ACTOR_SLUG — actor whose Composio GitHub connection the cron borrows
  • COMPOSIO_API_KEY — required for sync (already set)

Post-deploy

Run once: npx convex run migrations:stampDefaultsForConversations to backfill visibility = "personal" on legacy rows.

Test plan

  • Sign in as user A, create personal chat. Sign in as user B (incognito) — chat does not appear; direct URL is forbidden.
  • Create shared chat as A, post messages. B sees it in Shared, can read + post.
  • Try flipping visibility on a chat with an active turn — UI shows "a turn is in flight; stop or wait for it before changing visibility".
  • Flip a personal chat to shared via header toggle. Verify confirm modal, new Hermes session minted, agent has no memory of prior turns (proves session-store split).
  • Stream a long response, scroll up — page doesn't fight you. "Jump to latest" pill re-engages auto-scroll.
  • Multi-turn memory: ask agent to "remember purple", send unrelated messages, then "what's my color?" — should answer purple (proves Hermes bindSession fix).
  • Connect rail: 5+ toolkits show real logos; connected vs pending render different icons.
  • Connect button: mid-stream send "connect github". CTA lands under the new streaming assistant message (not the previous one) and doesn't jump.
  • Compose a draft, reload page — draft persists. Switch conversations — drafts don't leak.
  • Attach a PDF, send. Chip appears on user message; clicking opens the file. Try sending while still uploading — send button is disabled.
  • Click "regenerate" on the last assistant message that had attachments — Hermes is re-prompted with the same files, not just text.
  • Click "sync from GitHub" on /templates — candidates appear (or no-op if all already linked). Dismiss a candidate — toast confirms.
  • Delete a conversation — verify no orphan turn/chunk/tool-event rows in Convex.

🤖 Generated with Claude Code

…s session fix

Lands a bundle of related fixes from one user ask: auth leak on chat,
plain integration UI, connect-button reflow bug, missing composer
features, no templates auto-sync, no auto-scroll, and "every turn feels
like a new context" (Hermes session continuity).

## auth + chat visibility (PR1)

- agent_conversations gains optional `visibility` (personal|shared) +
  `owner_user_id` + two indexes (by_owner_updated, by_visibility_updated)
- New `convex/lib/conversationAuth.ts` — `requireUser` /
  `requireConversation` / `tryConversation` using the existing
  `authComponent.safeGetAuthUser` pattern (not `ctx.auth.getUserIdentity`,
  which isn't used anywhere in this repo)
- Every direct-id mutation/query that touches a conversation now goes
  through the gate: `list`, `append`, `clear`, `rename`,
  `delete`, `bindSession` in `agentMessages`; `startTurn`, `activeFor`,
  `cancel`, `failFromRoute` in `agentTurns`. Client-passed `actor_slug`
  is ignored.
- `bindSession` hardened with per-turn write_token + an
  `expected_hermes_session` race guard — `setVisibility` re-mints the
  Hermes session while a turn is mid-flight; the running wrapper's bind
  no longer clobbers the fresh shared/personal id.
- Hermes session names: `castle-personal-<owner>-<rand>` vs.
  `castle-shared-<rand>` so the FTS5 session store is partitioned by
  visibility (no shared-chat content can bleed into a personal store).
- Hermes preamble passed per turn so the agent KNOWS the chat is shared
  or personal and can self-moderate (e.g., refuse to repeat private
  memory in a shared thread).
- Hermes wrapper now calls `agentMessages:bindSession` after minting a
  new session — this was the root cause of "new context every turn":
  load_session failed, mint_new succeeded, but the new id never got
  patched back to Convex. Fixed at docker/hermes/serve.py:881.
- `convex/migrations.ts` — `stampDefaultsForConversations` (operator-
  gated public mutation) + `claimMyUnownedConversations` (called
  opportunistically on sidebar mount). Legacy actor_slug fallback in
  canAccess is intentionally NOT used: same-local-part collision risk
  (`sam@a.com`, `sam@b.com`).
- Sidebar split into Personal/Shared collapsible sections, each with
  its own "+ new" button. Chat header shows visibility chip + "make
  shared / make personal" toggle with confirm + fresh-memory-store
  warning.
- Routes (`/api/agent/{start,cancel,regenerate}`) use
  `fetchAuthMutation`/`fetchAuthQuery` so Convex sees the Better Auth
  identity. Legacy `/api/agent/route.ts` (orphan, no callers) removed.

## integrations polish + composer (PR2)

- Right rail: real toolkit logos via Composio's `meta.logo` joined
  from the cached catalog inside `listConnections`; status icon swapped
  for filled check (connected) / pulsing dot (pending). Fallback chip
  uses the first letter of the slug.
- Connect-button reflow killed: CTA only attaches once the latest
  assistant message was created AFTER the agent_action — otherwise
  the button would dangle below the previous assistant turn then jump
  down when the new streaming response landed.
- One-shot scroll-to when a new CTA appears, even if user has scrolled
  up.
- New `lib/use-chat-draft.ts` — per-conversation localStorage drafts
  (key `castle:chat-draft:<id>`); hydrate on conversation switch,
  persist on edit, clear on submit.
- Timestamps: surfaced on hover via a `<time>` element with relative
  label + absolute tooltip. Auto-ticks every 60s.
- `regenerate` button on the last assistant message — new
  `agentTurns.regenerateLast` deletes the old assistant message +
  chunks + tool events, re-queues a fresh turn pointing at the same
  user message and Hermes session. `/api/agent/regenerate` route kicks
  Railway off again.
- `copy` button on assistant messages.
- File attachments (full flow):
  - `agent_messages.attachments` schema field (storageId + name + ct)
  - `agentMessages.generateAttachmentUploadUrl` mutation +
    `attachmentUrl` query (Convex storage)
  - Paperclip in the composer → upload to Convex storage → chips above
    the textarea → on send, IDs go to `startTurn` which resolves URLs
    and stuffs them into the kickoff body
  - Hermes wrapper builds an `[the asker attached these files]`
    preamble with name + URL per file (no OCR/preview — agent fetches
    on demand)
  - User messages render attachment chips with click-to-download links

## templates auto-sync (PR3)

- New `template_github_candidates` table — staging area, no
  relaxation of the existing `templates` schema (required FKs to
  customer + author stay intact; humans curate the promotion)
- `convex/templates.ts` — `listGithubCandidates`,
  `upsertGithubCandidate` (idempotent; skips repos already linked OR
  already on the staging table), `dismissGithubCandidate`,
  `markCandidatePromoted`
- `/api/templates/sync-github` route accepts either an allowlisted
  Better Auth session (manual "sync from GitHub" button) OR a Vercel
  cron `Authorization: Bearer ${CRON_SECRET}` header. Uses
  `CASTLE_SYSTEM_ACTOR_SLUG`'s GitHub Composio connection, falls back
  to caller slug. Both POST and GET supported (Vercel cron uses GET).
- `vercel.json` cron entry — hourly, `0 * * * *`
- `<TemplateGithubCandidates>` section on `/templates` with "sync from
  GitHub" button + per-row dismiss

## sticky auto-scroll (PR4)

- Track scroll distance via `scrollRef` listener with 80px threshold.
  Only auto-scroll on new content if user is within 80px of the bottom.
- "↓ jump to latest" pill is sticky-inside the scroll container (not
  absolute-positioned against the parent), so it can't collide with the
  composer when the textarea is expanded.

## env vars needed in Vercel for full functionality

- `CRON_SECRET` — gates the hourly templates sync route
- `CASTLE_SYSTEM_ACTOR_SLUG` — actor whose Composio GitHub connection
  the cron borrows
- `COMPOSIO_API_KEY` — already set; required for sync

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented May 26, 2026

Copy link
Copy Markdown

Deployment failed with the following error:

Hobby accounts are limited to daily cron jobs. This cron expression (0 * * * *) would run more than once per day. Upgrade to the Pro plan to unlock all Cron Jobs features on Vercel.

Learn More: https://vercel.link/3Fpeeb1

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f4696068-e885-4143-877c-fc13de414b4b

📥 Commits

Reviewing files that changed from the base of the PR and between b52ad63 and 7742f82.

📒 Files selected for processing (9)
  • app/api/templates/sync-github/route.ts
  • components/chat-landing.tsx
  • components/template-github-candidates.tsx
  • convex/agentMessages.ts
  • convex/agentTurns.ts
  • convex/templates.ts
  • docker/hermes/serve.py
  • pnpm-workspace.yaml
  • tsconfig.json

📝 Walkthrough

Walkthrough

This PR implements a conversation ownership and visibility model, adds message attachments, introduces turn regeneration, syncs GitHub templates via daily cron, and refactors the chat UI to support personal/shared conversation lists with file attachments and turn regeneration controls.

Changes

Conversation Visibility, Message Attachments, and GitHub Template Sync

Layer / File(s) Summary
Conversation Access Control & Schema
convex/lib/conversationAuth.ts, convex/schema.ts, convex/migrations.ts
Introduces AuthUser type, requireUser, requireConversation, and tryConversation helpers for access enforcement. Adds optional visibility (personal/shared) and owner_user_id fields to agent_conversations with indexes, and optional attachments array to agent_messages. Migrations stamp default visibility and claim legacy unowned conversations by actor slug.
Conversation & Message Backend
convex/agentMessages.ts
Splits conversation listing into listConversations (merged personal+shared), listPersonal, and listShared queries with access gating. Updates createConversation to derive actor from auth and accept optional visibility. Hardens bindSession with per-turn write-token validation, turn ownership checks, and conditional patching via expected_hermes_session. Tightens deleteConversation to enforce owner-only deletion for shared conversations. Adds setVisibility mutation with ownership rule updates and session reminting. Message append removes actor_slug input and adds optional attachments support. Adds generateAttachmentUploadUrl and attachmentUrl storage integration.
Turn Creation and Regeneration Mutations
convex/agentTurns.ts
Updates startTurn to derive actor/visibility from requireConversation instead of caller-supplied fields, adds optional attachments input with URL resolution, and returns visibility/actor/attachment metadata. Adds new regenerateLast mutation to delete terminal assistant turn and recreate queued turn reusing user message. Tightens cancel and failFromRoute to re-verify conversation access via requireConversation. Updates activeFor to use access-control helpers.
GitHub Template Candidates
convex/schema.ts, convex/templates.ts
Adds template_github_candidates table with repo metadata, discovery timestamp, and dismissal/promotion tracking. Implements listGithubCandidates filtering dismissed/promoted entries, idempotent upsertGithubCandidate with dedup checks, and mutations to dismiss and mark candidates as promoted.
API Route Refactoring
app/api/agent/cancel/route.ts, app/api/agent/start/route.ts, app/api/agent/regenerate/route.ts
Refactors /agent/cancel to use fetchAuthMutation with server-side re-verification; removes old streaming /agent/route.ts; updates /agent/start to derive actor/visibility from mutation result. Adds new /agent/regenerate route that calls regenerateLast, mints tokens, and POSTs kickoff to Hermes with 5s timeout and error fallback mutation.
GitHub Sync API Route
app/api/templates/sync-github/route.ts
Adds new route supporting operator authentication and Vercel cron invocation, resolves GitHub actor from environment or operator email, fetches repos via Composio, upserts non-archived repos as candidates, and exports GET alias for cron.
Hermes Session Binding & Attachment Threading
docker/hermes/serve.py
Updates /agent/start endpoint to parse and forward visibility and attachments. Tracks Hermes session minting; calls Convex bindSession mutation with expected_hermes_session when session is newly minted and differs from stored value. Prepends visibility-specific preambles and attachment URL blocks to prompts sent to Hermes.
Chat Hooks: Draft, Context, & Messages
lib/use-chat-draft.ts, lib/chat-context.tsx, lib/use-hermes-chat.ts
Introduces useChatDraft hook for per-conversation localStorage draft persistence. Extends ChatContextValue with sendMessage(text, attachments?), regenerate(), and canRegenerate. Adds ChatAttachment and expanded ChatMessage types with createdAt. Implements useHermesChat attachments support with fallback text, and adds regenerate() function and canRegenerate flag computed from active turn and message history.
Chat Landing UI
components/chat-landing.tsx
Restructures to use useChatDraft for per-conversation draft management. Adds file attachment staging/upload via Convex URLs with per-file error toasts. Implements scroll-aware "jump to latest" UI. Refactors CTA attachment logic to gate on turn creation timestamp. Adds Timestamp, AttachmentChip, and CopyButton helper components. Introduces ChatHeader for visibility toggling with confirmation and toasts. Expands Turn component with regeneration controls and user timestamps/attachment chips.
Chat Sidebar Personal/Shared Lists
components/chat-sidebar.tsx
Splits conversation queries into listPersonal and listShared, merges results for auto-selection. Adds opportunistic migration effect to claim legacy conversations. Updates newChat to accept visibility parameter. Wraps deletion in try/catch with toast feedback. Refactors UI to render collapsible "Personal" and "Shared" sections with per-visibility "+ new" buttons and empty-state hints.
Supporting Components & UI Updates
components/agent-panel.tsx, components/connections-rail.tsx, app/connections/actions.ts, components/template-github-candidates.tsx, app/templates/page.tsx
Agent panel uses listPersonal query and creates personal conversations. ConnectionRow gains optional name and logo fields; listConnections decorates rows with toolkit catalog metadata. Connections-rail renders ToolkitLogo and StatusIcon components. Adds TemplateGithubCandidates component with sync button, candidate list, dismiss mutations, and toasts. Integrates candidates into templates page.
Vercel Cron Configuration
vercel.json
Configures daily cron job at 7 AM UTC to trigger /api/templates/sync-github.

🎯 4 (Complex) | ⏱️ ~60 minutes

🐰 The threads of chat now stitch with care,

Attachments tucked and sessions paired,
Regenerated turns hop anew,
GitHub seeds in fields of blue—
A rabbit hums: "All systems paired!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 43.24% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title clearly summarizes the main themes: visibility model, integrations polish, templates sync, and Hermes session fix. It directly relates to the extensive changes across auth, chat UI, templates, and backend systems.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/pensive-gagarin-17d4ca

Comment @coderabbitai help to get the list of available commands and usage tips.

Vercel free plan only allows one cron job per day. Switch from
`0 * * * *` (hourly) to `0 7 * * *` (daily at 07:00 UTC) so the
deployment succeeds. Operators can still hit the "sync from GitHub"
button on /templates for ad-hoc syncs.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@vercel

vercel Bot commented May 26, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
castle Ready Ready Preview, Comment May 26, 2026 9:40pm

@greptile-apps

greptile-apps Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR delivers a connected set of fixes centred on conversation visibility/ownership, auth hardening, the Hermes bindSession root-cause fix, and UI polish (file attachments, regenerate, auto-scroll, per-conversation drafts, integrations rail improvements, and template GitHub sync).

  • Visibility model: agent_conversations gains visibility (personal|shared) and owner_user_id; every direct-id query and mutation now goes through requireConversation/tryConversation from the new convex/lib/conversationAuth.ts module instead of trusting a client-passed actor_slug.
  • Hermes bindSession fix: the wrapper now calls agentMessages:bindSession back to Convex when it mints a fresh session, so memory accumulates across turns instead of resetting each time.
  • Regenerate + attachments: a new /api/agent/regenerate route and agentTurns.regenerateLast mutation cleanly delete the old assistant message/chunks/events and re-queue the same user message; file attachments flow through Convex storage to Hermes as URL lists.

Confidence Score: 5/5

Safe to merge; the auth model is well-structured and consistently applied, and the Hermes bindSession fix addresses a confirmed root cause.

All direct-id queries and mutations now go through the new conversationAuth helpers, closing the old arbitrary-actor-slug vector. The bindSession hardening (write-token + expected_hermes_session race guard) is correct. The only new findings are a never-called check_canceled helper in serve.py that references the old activeFor signature, and a stale doc comment on listConversations — neither affects runtime behavior.

docker/hermes/serve.py — the dead check_canceled function should either be removed or updated before the cancel-poll is re-enabled.

Important Files Changed

Filename Overview
convex/lib/conversationAuth.ts New module providing visibility guards (requireConversation, tryConversation, requireUser) and session naming; auth model is sound and consistently applied across queries/mutations.
docker/hermes/serve.py Adds long-running turn execution path with proper bindSession fix; a check_canceled helper is defined but never called, and still references the old activeFor API shape that would break if re-enabled.
convex/agentTurns.ts startTurn/cancel/failFromRoute/activeFor all upgraded to use requireConversation auth; regenerateLast added for UI regeneration with proper cleanup of old chunks/events.
convex/agentMessages.ts Visibility mutations (setVisibility, listPersonal, listShared) added with correct auth gates; bindSession hardened with write-token and expected_hermes_session race guard.
app/api/agent/start/route.ts Replaced unauthenticated ConvexHttpClient with fetchAuthMutation; hermes_session and actor now derived server-side from the authenticated turn, closing the old arbitrary-actor-slug vector.
app/api/agent/regenerate/route.ts New route for regenerating the last assistant turn; properly uses fetchAuthMutation and propagates auth errors with appropriate HTTP status codes.
app/api/templates/sync-github/route.ts New sync route with proper auth gating for both operator and Vercel cron paths; Composio successful flag is now checked before parsing data (fixing the silent-failure from the prior review).
convex/schema.ts Adds visibility, owner_user_id, and attachment fields with supporting indexes; fields are optional to allow backfill migration without breaking existing rows.
lib/use-hermes-chat.ts Adds attachment support, regenerate hook, and removes client-passed actorSlug from route calls; canRegenerate correctly tied to active state.
convex/migrations.ts stampDefaultsForConversations and claimMyUnownedConversations added; the leading comment incorrectly claims there is an actor_slug fallback in the access check that does not exist in the actual code (flagged in a prior thread).

Sequence Diagram

sequenceDiagram
    participant UI as Browser / UI
    participant VR as Vercel Route
    participant CVX as Convex
    participant HRM as Hermes (Railway)

    Note over UI,HRM: Normal turn
    UI->>VR: POST /api/agent/start
    VR->>CVX: fetchAuthMutation agentTurns.startTurn
    CVX-->>VR: turn_id, hermes_session, visibility
    VR->>HRM: POST /agent/start with kickoff token
    HRM-->>VR: 202
    loop streaming
        HRM->>CVX: agentMessageChunks:append
        HRM->>CVX: agentTurns:heartbeat
    end
    alt new session minted
        HRM->>CVX: agentMessages:bindSession
    end
    HRM->>CVX: agentTurns:complete

    Note over UI,HRM: Regenerate
    UI->>VR: POST /api/agent/regenerate
    VR->>CVX: fetchAuthMutation agentTurns.regenerateLast
    CVX-->>VR: turn_id, user_text, hermes_session
    VR->>HRM: POST /agent/start same session
    HRM-->>VR: 202

    Note over UI,HRM: Cancel
    UI->>VR: POST /api/agent/cancel
    VR->>CVX: fetchAuthMutation agentTurns.cancel
    VR->>HRM: POST /agent/cancel/turnId
Loading

Reviews (2): Last reviewed commit: "fix: apply CodeRabbit auto-fixes" | Re-trigger Greptile

Comment thread convex/templates.ts
Comment thread convex/migrations.ts
Comment on lines +25 to +31
* Until a row is claimed, the access check in
* `convex/lib/conversationAuth.ts` falls back to matching
* `actor_slug → caller.slug`, so existing chats remain accessible to
* their original users mid-migration.
*
* Run from CLI:
* npx convex run migrations:stampDefaultsForConversations

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Migration comment contradicts actual canAccess behavior

The comment says "Until a row is claimed, the access check falls back to matching actor_slug → caller.slug." In reality, canAccess() in conversationAuth.ts (line 64) returns false immediately for any row with no owner_user_id, with no slug fallback. Post-deploy, every legacy row without an owner_user_id will be forbidden — even to the original creator — until claimMyUnownedConversations runs for that user on sidebar mount. If that mutation is slow or skipped (e.g. direct-URL navigation before the sidebar mounts), the user sees a 403 on their own conversations. The comment should be corrected; the sidebar-mount call to claimMyUnownedConversations is the real recovery path.

Comment thread convex/agentMessages.ts
Comment on lines 181 to 201
@@ -91,6 +200,36 @@ export const deleteConversation = mutation({
},
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 deleteConversation leaves orphaned turns, chunks, and tool-events

The handler deletes agent_messages rows and the conversation itself but never touches agent_turns, agent_message_chunks, or agent_tool_events. All three tables have references (conversation_id on turns, turn_id on chunks/events) that become dangling after deletion. This accumulates unbounded orphan data over time and will confuse any future cron or query that sweeps those tables expecting live references.

Comment thread app/api/templates/sync-github/route.ts
Comment thread vercel.json
Comment on lines +4 to +7
{
"path": "/api/templates/sync-github",
"schedule": "0 7 * * *"
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Cron schedule is daily, not hourly as described in the PR

"0 7 * * *" fires once a day at 07:00 UTC. The PR description, PR summary, and inline route comment all say "hourly". If hourly sync is the intended cadence, the schedule should be "0 * * * *" (or an off-minute equivalent like "7 * * * *").

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 9

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
convex/agentMessages.ts (1)

184-199: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Cascade turn state before deleting the conversation.

This only removes agent_messages and the agent_conversations row. agent_turns, agent_message_chunks, and agent_tool_events for the same conversation remain orphaned, and a wrapper still finishing the turn can keep writing against rows whose assistant message has already been deleted. Please cancel/cascade the full turn graph here.

🤖 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 `@convex/agentMessages.ts` around lines 184 - 199, The deletion only removes
agent_messages and the agent_conversations row leaving agent_turns,
agent_message_chunks, and agent_tool_events orphaned and possibly still
writable; update the delete flow in the block after requireConversation(...) to
cascade/cancel the entire turn graph for conversation id: query and mark any
active agent_turns as cancelled/finished (so no background writer continues),
then delete related rows in agent_message_chunks and agent_tool_events (and
agent_messages if not already removed), and finally delete agent_turns and the
agent_conversation row; use the existing ctx.db query/.withIndex/.collect and
ctx.db.delete calls (same pattern as msgs and ctx.db.delete(id)) and perform
these operations atomically/transactionally if supported so no leftover rows
remain.
lib/use-hermes-chat.ts (1)

182-233: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Propagate send failures back to callers.

sendMessage always resolves after a failed /api/agent/start POST and only stores the error locally. In this PR, the callers in components/chat-landing.tsx and components/agent-panel.tsx clear their composer state immediately after invoking it, so a transient failure drops the user's unsent text and attachments. Return a success flag or rethrow here so callers only clear local state after the turn is actually created.

🤖 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 `@lib/use-hermes-chat.ts` around lines 182 - 233, sendMessage currently
swallows failures and always resolves, causing callers to clear composer state
on transient errors; modify sendMessage (the useCallback that calls
fetch("/api/agent/start")) to return a success boolean (or throw) so callers can
await it before clearing UI: ensure on a successful POST you return true, on
non-OK response capture detail and call setSendError(...) then return false (or
rethrow if you prefer), and in the catch block setSendError(...) and return
false; keep setSending(false) in finally and update sendMessage's
signature/return type so callers (e.g., components/chat-landing.tsx and
components/agent-panel.tsx) can await the result before clearing
text/attachments.
🧹 Nitpick comments (1)
components/chat-sidebar.tsx (1)

73-90: ⚡ Quick win

Make the implicit auto-create visibility explicit.

This effect says it only auto-creates personal chats, but it's the one remaining createConversation call site that still relies on the backend default. Passing visibility: "personal" here keeps the client-side contract stable if the mutation default changes later.

Suggested change
-    create({}).then((id) => onSelect(id));
+    create({ visibility: "personal" }).then((id) => onSelect(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 `@components/chat-sidebar.tsx` around lines 73 - 90, The effect in useEffect
implicitly creates a conversation with create({}) which relies on backend
defaults; update the create call in the useEffect (the block that checks
actorSlug, personal, shared, allConversations, selectedId and then calls
create(...).then(...)) to pass the explicit visibility parameter, e.g. call
create({ visibility: "personal" }) so the client-side contract doesn’t depend on
server defaults.
🤖 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 `@app/api/templates/sync-github/route.ts`:
- Around line 93-101: The call to
c.tools.execute("GITHUB_LIST_ORGANIZATION_REPOSITORIES") currently ignores
result.successful and treats missing/invalid data as an empty list; update the
logic after the execute call (the result variable handling and repos assignment)
to check result.successful and, if false or missing expected data, fail the sync
by throwing an error or returning a non-200 Response (include the returned
result/error details in the message) instead of silently assigning repos = [];
ensure this guard is applied immediately after reading result so
GITHUB_LIST_ORGANIZATION_REPOSITORIES failures are surfaced.

In `@components/chat-landing.tsx`:
- Around line 245-255: The submit() path currently only checks
pendingAttachments and can send while uploads are in flight; update submit() to
also block when uploadingCount > 0 (i.e., require uploadingCount === 0) and
similarly disable/gate the send button UI so it cannot be clicked while
uploadingCount > 0; specifically, before calling sendMessage(text,
pendingAttachments.length ? pendingAttachments : undefined) in submit(), return
or await until uploadingCount === 0, and ensure the same check/guard is applied
to any other send handler referenced around the other send button code (the
alternate send handler you saw at 469-474) so clearDraft(),
setPendingAttachments([]), and sendMessage(...) only run once uploads are
finished.

In `@components/template-github-candidates.tsx`:
- Around line 105-109: The onClick handler for the button that calls dismiss({
id: c._id }) must handle rejection and show user feedback; wrap the async call
inside a try/catch inside the onClick (the handler that currently awaits dismiss
and then calls toast.success), call toast.success on success and toast.error
with a helpful message in the catch branch, and ensure any state used for
optimistic UI or disabling (if present) is restored in the failure path so there
are no unhandled promise rejections from the dismiss call.

In `@convex/agentMessages.ts`:
- Around line 308-313: The attachmentUrl query currently only calls requireUser
and returns ctx.storage.getUrl(storageId), allowing anyone with a storageId to
mint URLs; change the query signature (attachmentUrl) to accept a
conversation_id (or message_id) in args, call requireConversation(ctx,
conversation_id) (or requireMessage if available) to re-check
access/authorization before calling ctx.storage.getUrl(storageId), and only
return the signed URL after the conversation/message authorization succeeds;
update any related callers to pass the conversation_id/message_id.
- Around line 211-229: The handler for changing conversation visibility (handler
in this file, which calls requireConversation and updates via ctx.db.patch)
currently remints the session but allows in-flight turns to continue against the
old Hermes session; reject visibility flips while a turn is active by querying
the agent turns for this conversation (e.g. look up active/queued/running turns
associated with id) before computing newOwner/hermesSessionName and applying
ctx.db.patch, and throw a descriptive error (e.g. "cannot change visibility
while a turn is active") if any active turn exists; keep the existing logic for
allowed flips when no active turns are found.

In `@convex/agentTurns.ts`:
- Around line 350-361: The regenerate path currently only returns user_text from
the reused user message, losing attachments; update the object returned in the
function that builds the regenerated turn (the code using latest.user_message_id
and userMsg from ctx.db.get) to include the same attachment metadata/URLs fields
that startTurn returns (pull attachments from userMsg?.attachments or equivalent
and include them in the returned payload alongside user_text, user_message_id,
assistant_message_id, hermes_session, visibility, actor_slug, and turn_id) so
Hermes can forward attachments for regenerations.

In `@convex/templates.ts`:
- Around line 46-57: upsertGithubCandidate lowercases github_repo before lookups
but setGithubRepo persists owner/repo with original casing, causing misses;
update setGithubRepo to normalize the repo the same way upsertGithubCandidate
does (e.g., apply the existing normalize/lowercase helper) before writing
github_repo to the templates record so the templates table and
template_github_candidates use identical lowercased repo values for dedupe and
lookups.
- Around line 23-83: These handlers (listGithubCandidates,
upsertGithubCandidate, dismissGithubCandidate, markCandidatePromoted) are
public; add operator-only auth or make them internal and expose authenticated
wrappers: either insert an auth check at the top of each handler (e.g., call
ctx.auth.getUserIdentity(), validate identity/role and throw if unauthorized)
before any DB access, or convert the implementations to
internalQuery/internalMutation and create thin exported query/mutation wrappers
that call the internal versions only after verifying ctx.auth.getUserIdentity()
and operator permissions; ensure all four functions reference the same auth
logic so only authorized operators can read/modify template_github_candidates.

In `@docker/hermes/serve.py`:
- Around line 647-649: The current agent_start incoming-params validation
rejects falsy hermes_session, which breaks first-turn flows because
_run_turn_to_convex can mint a new session; update the validation so
hermes_session is not required to be truthy (e.g., remove hermes_session from
the all([...]) check or only validate other required fields turn_id,
conversation_id, actor_slug, write_token, convex_url) so agent_start accepts
empty strings for hermes_session and lets _run_turn_to_convex handle session
creation.

---

Outside diff comments:
In `@convex/agentMessages.ts`:
- Around line 184-199: The deletion only removes agent_messages and the
agent_conversations row leaving agent_turns, agent_message_chunks, and
agent_tool_events orphaned and possibly still writable; update the delete flow
in the block after requireConversation(...) to cascade/cancel the entire turn
graph for conversation id: query and mark any active agent_turns as
cancelled/finished (so no background writer continues), then delete related rows
in agent_message_chunks and agent_tool_events (and agent_messages if not already
removed), and finally delete agent_turns and the agent_conversation row; use the
existing ctx.db query/.withIndex/.collect and ctx.db.delete calls (same pattern
as msgs and ctx.db.delete(id)) and perform these operations
atomically/transactionally if supported so no leftover rows remain.

In `@lib/use-hermes-chat.ts`:
- Around line 182-233: sendMessage currently swallows failures and always
resolves, causing callers to clear composer state on transient errors; modify
sendMessage (the useCallback that calls fetch("/api/agent/start")) to return a
success boolean (or throw) so callers can await it before clearing UI: ensure on
a successful POST you return true, on non-OK response capture detail and call
setSendError(...) then return false (or rethrow if you prefer), and in the catch
block setSendError(...) and return false; keep setSending(false) in finally and
update sendMessage's signature/return type so callers (e.g.,
components/chat-landing.tsx and components/agent-panel.tsx) can await the result
before clearing text/attachments.

---

Nitpick comments:
In `@components/chat-sidebar.tsx`:
- Around line 73-90: The effect in useEffect implicitly creates a conversation
with create({}) which relies on backend defaults; update the create call in the
useEffect (the block that checks actorSlug, personal, shared, allConversations,
selectedId and then calls create(...).then(...)) to pass the explicit visibility
parameter, e.g. call create({ visibility: "personal" }) so the client-side
contract doesn’t depend on server defaults.
🪄 Autofix (Beta)

✅ Autofix completed


ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 2208d784-6401-4c1a-9f63-1e909f8d8cbe

📥 Commits

Reviewing files that changed from the base of the PR and between 8d69335 and b52ad63.

⛔ Files ignored due to path filters (1)
  • convex/_generated/api.d.ts is excluded by !**/_generated/**
📒 Files selected for processing (23)
  • app/api/agent/cancel/route.ts
  • app/api/agent/regenerate/route.ts
  • app/api/agent/route.ts
  • app/api/agent/start/route.ts
  • app/api/templates/sync-github/route.ts
  • app/connections/actions.ts
  • app/templates/page.tsx
  • components/agent-panel.tsx
  • components/chat-landing.tsx
  • components/chat-sidebar.tsx
  • components/connections-rail.tsx
  • components/template-github-candidates.tsx
  • convex/agentMessages.ts
  • convex/agentTurns.ts
  • convex/lib/conversationAuth.ts
  • convex/migrations.ts
  • convex/schema.ts
  • convex/templates.ts
  • docker/hermes/serve.py
  • lib/chat-context.tsx
  • lib/use-chat-draft.ts
  • lib/use-hermes-chat.ts
  • vercel.json
💤 Files with no reviewable changes (1)
  • app/api/agent/route.ts

Comment thread app/api/templates/sync-github/route.ts
Comment thread components/chat-landing.tsx
Comment thread components/template-github-candidates.tsx
Comment thread convex/agentMessages.ts
Comment thread convex/agentMessages.ts
Comment thread convex/agentTurns.ts Outdated
Comment thread convex/templates.ts
Comment thread convex/templates.ts
Comment thread docker/hermes/serve.py Outdated
@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Note

Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it.

Fixes Applied Successfully

Fixed 9 file(s) based on 9 unresolved review comments.

Files modified:

  • app/api/templates/sync-github/route.ts
  • components/chat-landing.tsx
  • components/template-github-candidates.tsx
  • convex/agentMessages.ts
  • convex/agentTurns.ts
  • convex/templates.ts
  • docker/hermes/serve.py
  • pnpm-workspace.yaml
  • tsconfig.json

Commit: 7742f82a600beb949799dfb595f08c21ead02315

The changes have been pushed to the claude/pensive-gagarin-17d4ca branch.

Time taken: 8m 10s

Fixed 9 file(s) based on 9 unresolved review comments.

Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
@undeemed

Copy link
Copy Markdown
Member Author

Superseded by combined PR

@undeemed undeemed closed this May 26, 2026
undeemed added a commit that referenced this pull request May 26, 2026
CodeRabbit (critical):
- Block setVisibility while a turn is active — without it, a flip
  during a running turn could publish a personal-memory response into
  a now-shared thread (operator stops/waits for the turn instead)
- attachmentUrl now takes message_id + storageId, gates via
  tryConversation, and verifies storageId actually belongs to that
  message. Stops holders of an orphan storageId minting URLs after a
  shared chat is demoted to personal
- Hermes wrapper no longer rejects empty hermesSession — _run_turn_to_convex
  already treats it as "mint new + bind back," so the previous 400
  blocked first-turn flows on brand-new conversations

CodeRabbit (major):
- regenerateLast now resolves and returns attachment URLs alongside
  user_text, and the regenerate route forwards them in the kickoff
  body — without this, regenerating a user message with attached
  files re-prompted Hermes with text only
- Composer submit + send button are gated on uploadingCount === 0;
  sending mid-upload would have stranded files in the composer for
  the next turn
- normalizeGithubRepo() shared helper now used by both
  upsertGithubCandidate and setGithubRepo so `Owner/Repo` and
  `owner/repo` dedupe correctly across the candidates+templates
  tables

CodeRabbit (minor):
- Dismiss handler on template candidates wraps the mutation in
  try/catch + toast.error so failures aren't an unhandled rejection

Greptile (P1 security):
- listGithubCandidates, dismissGithubCandidate, markCandidatePromoted
  now require an authenticated user — NEXT_PUBLIC_CONVEX_URL is public,
  so without these gates anyone could enumerate or wipe staged
  candidates. upsertGithubCandidate stays open so the Vercel cron
  (which has no operator identity) can still write
- Migration docstring rewritten to match actual canAccess behavior
  (legacy actor_slug fallback was deliberately removed; recovery is
  via opportunistic claim, now hoisted to ChatProvider so it runs on
  every page, not just /)

Greptile (P2):
- deleteConversation now cascade-deletes agent_turns,
  agent_message_chunks, and agent_tool_events — previously these
  became unbounded orphan data

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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