Skip to content

RBAC + chat visibility + integrations polish + templates sync#4

Merged
undeemed merged 11 commits into
mainfrom
combined-pr
May 26, 2026
Merged

RBAC + chat visibility + integrations polish + templates sync#4
undeemed merged 11 commits into
mainfrom
combined-pr

Conversation

@undeemed

@undeemed undeemed commented May 26, 2026

Copy link
Copy Markdown
Member

Summary

Combines PRs #2 and #3 into a single PR. Supersedes both.

Operator/Guest RBAC (from #2)

  • Non-allowlisted users become "guests" with read-only template access (no longer blocked)
  • Every Convex mutation in templates.ts and patternExtractions.ts enforces assertOperator()
  • RoleProvider + useIsOperator() context gates client-side editing controls
  • TopNav/ProfileMenu adapt to role; middleware simplified
  • Allowlist read queries (list, listWithRescue) now require operator session
  • Dedicated loadTemplateData() for guest pages (no founderHours leak)

Chat visibility model (from #3)

  • agent_conversations gains visibility (personal/shared) + owner_user_id
  • Every conversation mutation/query auth-gated via safeGetAuthUser
  • bindSession hardened with per-turn write-token + race-guard
  • Hermes session prefix split for personal vs shared conversations

Integrations polish (from #3)

  • GitHub template sync via Composio (/api/templates/sync-github)
  • TemplateGithubCandidates component for operator template discovery
  • Cron job for daily templates sync

Review fixes applied

  • Greptile: least-privilege nav default (?? false), valid pnpm allowBuilds
  • CodeRabbit: guard allowlist reads, narrow guest data loader, empty-state tags UX

Test plan

  • Operator sign-in → full console, all nav links, edit controls
  • Guest sign-in → redirected to /templates, read-only view
  • Guest direct URL to /overview → redirected to /templates
  • Chat visibility toggle (personal ↔ shared)
  • GitHub template sync via candidates component
  • Convex mutations reject for non-operators

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Multi-file attachments in messages with upload UI and attachment chips
    • Regenerate the latest assistant response for a conversation
    • Sync GitHub repos as template candidates (cron-enabled)
  • UI

    • Per-conversation visibility (personal vs shared) and split sidebar
    • Role-aware gating across controls and navigation (operator vs guest)
    • Improved connections rail (logos/status), timestamps, transcript navigation, and chat drafts
  • Chores

    • Added scheduled template-sync cron and deployment config

Review Change Stack

coderabbitai Bot and others added 8 commits May 26, 2026 02:33
Fixed 5 file(s) based on 5 unresolved review comments.

Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
…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>
… get full console

- Extract assertOperator into convex/lib/assertOperator.ts (shared across modules)
- Add RoleProvider context + useIsOperator hook for client-side role gating
- getCurrentUser now returns { ...user, isOperator } instead of null for non-allowlisted
- Auth hook no longer throws on non-allowlisted sign-in (guests allowed through)
- Middleware requires session for all routes (no public "/" exception)
- All Convex mutations in templates.ts / patternExtractions.ts enforce assertOperator
- Controls render read-only for guests, editable for operators
- TopNav shows operator links vs guest links based on role
- ProfileMenu hides settings link for guests
- Guests redirected to /templates from operator-only pages

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

- TopNav: default isOp to false (not true) while query loads, so guests
  never briefly see operator links
- pnpm-workspace.yaml: replace placeholder strings with boolean values

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t data, empty-state UX

- emailAllowlist: add assertOperatorRead to list/listWithRescue so
  guests can't enumerate operator emails
- load-overview: replace skipAuth escape hatch with dedicated
  loadTemplateData() that omits founderHours
- template-tags-input: show "—" dash for empty tags in guest view
  instead of blank cell

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

# Conflicts:
#	app/templates/page.tsx
@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 10:57pm

@coderabbitai

coderabbitai Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: db4a415a-9422-4740-9470-8baab94ccdf0

📥 Commits

Reviewing files that changed from the base of the PR and between db533c1 and 6cd0c22.

📒 Files selected for processing (3)
  • convex/agentMessages.ts
  • convex/lib/conversationAuth.ts
  • convex/templates.ts
🚧 Files skipped from review as they are similar to previous changes (2)
  • convex/templates.ts
  • convex/agentMessages.ts

📝 Walkthrough

<review_stack_artifact_start>
<stack_title>Roles, conversation visibility, attachments, regeneration, and templates</stack_title>
<stack_summary>Adds operator/guest role helpers, conversation visibility/ownership, message attachments and regeneration flows, GitHub template discovery, Convex schema/authorization updates, Hermes kickoff framing, and coordinated frontend role-gated UI and hooks.</stack_summary>

Main cohesive set: backend Convex auth/conversation gates, schema/migrations, agent turns/messages mutations, Next.js API routes for agent and template sync, Hermes server updates, and frontend role/attachment/regeneration UX. Next.js route handlers for agent lifecycle (start/cancel/regenerate) and the new templates sync-github cron/operator endpoint, including auth-forwarding, kickoff token creation, Hermes POST with timeout, and failure marking. range_d331c62f4d5f range_5a99f70e7847 range_63ca3aafb8ad range_ac190b613ec9 range_c030e0c36491 range_6959402205f8 range_d9b3c47b4647 range_65a03e33ce8c range_e228013900eb range_72e766d9d6d2 range_893d6eb833ad range_9485725ee78e range_517a56c115b1 range_5b0f14f866a5 range_c19f0a5433c0 range_e478dce7c626 range_9485725ee78e range_5b0f14f866a5 range_c19f0a5433c0 range_5b0f14f866a5 range_5b0f14f866a5 range_5b0f14f866a5 range_5b0f14f866a5 New operator assertion and conversation auth utilities: isOperator, assertOperator, assertOperatorRead, requireUser, requireConversation, tryConversation, emailToSlug, hermesSessionName used to gate and normalize access across Convex handlers. range_918b20695806 range_2314455190ca range_366185bf58cb range_82896f4e19bf range_39c6496fc387 range_7085a1bacac3 range_81a4adaa26c8 range_8beca5a004c8 range_6bd3c3e5882e range_88121d7c5359 range_81a4adaa26c8 range_81a4adaa26c8 range_81a4adaa26c8 range_81a4adaa26c8 range_81a4adaa26c8 range_81a4adaa26c8 Schema updates and one-shot migrations: template_github_candidates table, agent_conversations visibility/owner_user_id, agent_messages attachments; migrations to stamp defaults and claim unowned conversations. range_38d5b855fdd0 range_9ff15806b63e range_00c8eb9ac7be range_59871962cc97 range_51b60c600ae1 range_88121d7c5359 Split personal/shared conversation listing, owner-aware createConversation, append supports attachments and server-derived actor_slug, bindSession turned into write-token-validated binder; startTurn accepts attachments and returns kickoff metadata; cancel/failFromRoute use conversation auth; regenerateLast recreates queued turns. range_c283e4748803 range_c31459e1b2c1 range_4a3985cb092c range_f91339238ad8 range_32d618c4aca7 range_713809ff2368 range_7f9c645e748d range_a179ff35bc95 range_8b85864fefce range_27e56dc65f81 range_246e7129a8fe range_cd7f1e994ec4 range_440a52b0402e range_918b20695806 Add operator authorization checks to template mutations and implement list/upsert/dismiss/markCandidatePromoted for GitHub candidates; frontend candidate UI hooks into sync route. range_1616f4e73a5a range_85112040bed0 range_74b1d74a5ba3 range_f87ba86699eb range_370ea08ff82e range_c235ec2fdb65 range_ca30a415d0fb range_c7ec7b2f9fb8 range_d65d8e6ee767 range_dce9438ed6f3 range_643edb6d1a1a range_0641f2ff2dbd range_9f729bcf8d56 range_6b203d4a50d1 range_72e766d9d6d2 range_72e766d9d6d2 docker/hermes/serve.py updated to parse visibility and attachments, mint/bind Hermes sessions back to Convex, and stream framed prompt content including visibility preamble and attachments directory. range_61407324e100 range_9babbf27e996 range_d90d99454a3d range_d866dbd38c69 range_0a71d21a949e range_7d791930f882 range_cacedbdf0dfa range_063712f2dc7f range_6ef070c90d72 range_4fd9947db717 range_24d2a70c5716 range_6fbe4588145b range_fa8244c0a7a4 range_62d65fd6140b range_c666b62399dd useHermesChat and ChatContext extended to support attachments and regeneration; added useChatDraft and chat UI changes for multi-file attachments, uploads, persistent drafts, sticky scroll, regenerate controls, timestamps, and attachment chips. range_02327cc668bd range_aacdd8f90769 range_c278bb6f46a8 range_ae10fedc608f range_24984faea8ed range_d577e0ab15fb range_d53f7ffdd2bc range_db20566f3726 range_073c8b0db9ab range_d9bc9c2251a6 range_aff4a4c83b68 range_8de030e8ac1d range_116c77d0deb4 range_abaa24de5783 range_e597a819b7a4 range_dc1bba72c6e8 range_9402b400cc9b range_4cdd9e4e1ee6 range_a64a1e146381 range_e1ae6985ca3a range_e8afe17ffbad range_173e6aca46f2 range_664751fd9376 range_807e009faa02 range_073c8b0db9ab RoleProvider/useIsOperator, requireSignedIn/loadTemplateData loaders, pages updated to redirect non-operators, templates pages wrapped with RoleProvider, TopNav and ProfileMenu conditional operator UI. range_8bc197be8559 range_9e4dc2e48d49 range_97c992a58146 range_3d168fa01062 range_c6523d82f115 range_e24680f47efd range_5a7b70042b02 range_5dab21e5bc25 range_83619f0d290d range_87b4299df9c0 range_acebd49be201 range_4052eb500573 range_53970c9c55b8 range_63281a4d3026 range_37db6f366826 range_2a9e57af0db9 range_63281a4d3026 ChatSidebar split into Personal/Shared with claim migration, many control components gated by useIsOperator to render read-only for guests, ProfileMenu operator-only link, and per-conversation create/delete flows. range_52f975588781 range_bb41d69db418 range_a9377d745375 range_440a52b0402e range_93b2df790391 range_37424f20f876 range_baf214e9dd45 range_b272cf58ccb3 range_244ee09361b3 range_3b4c24424dc9 range_4bbfe7ef8f6e range_783ec6a6c54a range_e7ab0272cdf4 range_b8c652d2a2c8 range_253f9c25950d range_b96be3f0aa19 range_c0b1748f44df range_0e20792f6a28 range_1de54ff244f8 range_0530dbe87537 range_fe102e01d77e range_32d618c4aca7 range_9700a246790c range_e39272814970 range_d2045af0154f range_cc7b19c245d1 range_b5a307c86832 range_58edc6af9c3f range_ca3c1c4167c0 range_59ea073a57ec range_5ac462f374f3 range_5f38a27bd2be range_130b0f7c2406 range_ac9e6c58c468 range_7af41439bc32 range_e4c85ff204cb range_0530dbe87537 Connections decorated with toolkit catalog name/logo, TemplateGithubCandidates UI, UI polish (pluralization, motion-safe, delta sign), pnpm/workspace and vercel.json cron config, and middleware docs adjustments. range_1b27d6e56096 range_64ea1149b53d range_04c26cded4b0 range_768a0d49d09a range_43f5e7eae251 range_284496f64b76 range_bb870ce13c5c range_b6a6d113ed40 range_c14f7e2e4f38 range_0fd6916e37c0 range_38dfdc7e4fdf range_ca820007c0e2 range_d78933350813 range_38dfdc7e4fdf range_ca820007c0e2 range_d78933350813 range_38dfdc7e4fdf range_ca820007c0e2 range_d78933350813 range_38dfdc7e4fdf range_38dfdc7e4fdf range_38dfdc7e4fdf range_38dfdc7e4fdf range_38dfdc7e4fdf range_38dfdc7e4fdf range_ca820007c0e2 range_d78933350813 range_38dfdc7e4fdf range_38dfdc7e4fdf range_38dfdc7e4fdf range_38dfdc7e4fdf range_ca820007c0e2 range_d78933350813 range_38dfdc7e4fdf range_38dfdc7e4fdf range_38dfdc7e4fdf range_38dfdc7e4fdf range_38dfdc7e4fdf range_ca820007c0e2 range_d78933350813 range_38dfdc7e4fdf range_38dfdc7e4fdf
✨ 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 combined-pr

@greptile-apps

greptile-apps Bot commented May 26, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR combines two prior PRs to introduce RBAC (operator vs. guest roles), a per-conversation visibility model (personal/shared), file attachments in chat, GitHub template sync via Composio, and a regenerate-turn feature. The auth surface is substantially hardened: every Convex mutation now enforces assertOperator, conversation access is gated through a new conversationAuth library, and the Vercel routes switch from unauthenticated ConvexHttpClient calls to fetchAuthMutation with identity forwarding.

  • RBAC + guest pass-through: Non-allowlisted users receive a valid session and are redirected to /templates (read-only); assertOperator in every write mutation enforces the boundary server-side; RoleProvider/useIsOperator gate client-side controls.
  • Chat visibility: agent_conversations gains visibility and owner_user_id; conversationAuth.ts provides requireConversation/tryConversation helpers with legacy actor_slug fallback; bindSession is hardened with a per-turn HMAC write-token and a race guard for mid-turn visibility flips.
  • Attachments + regenerate: Files are uploaded to Convex storage (operator-only), forwarded as signed URLs to Hermes, and rendered as chips; a new /api/agent/regenerate route deletes the stale turn and re-queues against the same memory.

Confidence Score: 4/5

The change is broadly solid but the reworked auth gates in lib/load-overview.ts have a concrete defect: the catch blocks are no-ops that rethrow unconditionally, losing the old redirect-to-sign-in fallback when fetchAuthQuery throws.

Both requireSignedIn and requireOperator now catch errors only to immediately rethrow them — the isRedirectError branch and the fallback throw err are identical in effect. In the old code any thrown error from fetchAuthQuery resulted in a clean redirect to /sign-in; now it surfaces as an unhandled 500. This is a regression on the operator auth-gate path exercised on every operator page load when Convex has a transient issue or when a session token is rejected rather than returned as null.

lib/load-overview.ts — both requireSignedIn and requireOperator need the fallback redirect reinstated after the isRedirectError guard.

Important Files Changed

Filename Overview
lib/load-overview.ts Introduces requireSignedIn and reworks requireOperator; both have identical no-op catch blocks that rethrow unconditionally, losing the old redirect-to-sign-in fallback for auth errors.
convex/agentMessages.ts Adds conversation visibility model, attachment support, and setVisibility; contains a dead isShared variable in a security-sensitive mutation and attachmentUrl is accessible to any authenticated user including guests.
convex/lib/conversationAuth.ts New auth helpers for conversation access control; canAccess correctly implements legacy actor_slug fallback and operator check for shared visibility.
convex/lib/assertOperator.ts Centralises the operator/allowlist check previously duplicated across modules; clean extraction with no issues.
convex/agentTurns.ts Removes client-passed actor_slug in favour of auth-derived identity; adds regenerateLast mutation with proper conversation ownership guard; clean overall.
convex/templates.ts Adds assertOperator to all write mutations and assertOperatorRead to listGithubCandidates; upsertGithubCandidate deliberately left unprotected for cron path.
convex/auth.ts Allows non-allowlisted users through sign-in as guests; getCurrentUser now returns { ...user, isOperator } for role-aware clients.
convex/migrations.ts Adds backfill mutations for visibility/owner stamping; stampDefaultsForConversations iterates all rows in a single mutation which may time out on large tables.
app/api/templates/sync-github/route.ts New GitHub org sync route; operator-gated for manual calls and CRON_SECRET-gated for scheduled calls.
middleware.ts Simplified to require a session cookie for all routes; removes the public / exemption; clean.
docker/hermes/serve.py Adds visibility-aware preamble injection, attachment URL forwarding to Hermes, and the bindSession call for fresh sessions; kickoff token covers the full body hash including new fields.
app/api/agent/start/route.ts Removes unauthenticated ConvexHttpClient in favour of fetchAuthMutation; actor identity now derived server-side from auth context.
app/api/agent/regenerate/route.ts New regenerate endpoint; mirrors start/route.ts auth pattern, mints kickoff+write tokens, handles Railway kickoff failures correctly.
lib/role-context.tsx Minimal React context for operator/guest role; defaults to guest which is the least-privilege choice.

Sequence Diagram

sequenceDiagram
    participant Browser
    participant Middleware
    participant RequireOperator as requireOperator / requireSignedIn
    participant Convex
    participant VercelRoute as /api/agent/start
    participant Railway as Hermes (Railway)

    Browser->>Middleware: GET /overview (or /templates)
    Middleware->>Middleware: check session cookie presence
    alt No cookie
        Middleware-->>Browser: redirect /sign-in
    else Cookie present
        Middleware-->>Browser: pass through
        Browser->>RequireOperator: Server Component load
        RequireOperator->>Convex: fetchAuthQuery(getCurrentUser)
        alt Convex error
            Convex-->>RequireOperator: throws
            RequireOperator-->>Browser: 500 (catch is no-op — should redirect /sign-in)
        else user null
            Convex-->>RequireOperator: null
            RequireOperator-->>Browser: redirect /sign-in
        else guest on operator page
            Convex-->>RequireOperator: isOperator false
            RequireOperator-->>Browser: redirect /templates
        else operator
            Convex-->>RequireOperator: isOperator true
            RequireOperator-->>Browser: render page
        end
    end

    Browser->>VercelRoute: "POST /api/agent/start {conversationId, text, attachments}"
    VercelRoute->>Convex: fetchAuthMutation(agentTurns.startTurn)
    Convex->>Convex: requireConversation (ownership + visibility check)
    Convex->>Convex: insert user_msg + assistant_placeholder + agent_turn
    Convex-->>VercelRoute: "{turn_id, hermes_session, actor_slug, attachment_links}"
    VercelRoute->>VercelRoute: mint kickoff token (HMAC over body hash)
    VercelRoute->>Railway: "POST /agent/start {kickoffToken, writeToken, visibility, attachments}"
    Railway->>Railway: verify kickoff token
    Railway->>Convex: mutation agentTurns.claimTurn (queued to running)
    Railway->>Railway: load or mint Hermes session
    alt New session minted
        Railway->>Convex: mutation agentMessages.bindSession (write_token + race guard)
    end
    Railway->>Railway: prepend visibility preamble + attachment block
    Railway->>Railway: stream prompt to Hermes ACP
    Railway->>Convex: mutation agentTurns.complete (write_token)
Loading

Reviews (4): Last reviewed commit: "fix: address new Greptile/CodeRabbit P1 ..." | Re-trigger Greptile

Comment thread convex/templates.ts
Comment thread convex/templates.ts
Comment thread convex/agentMessages.ts
Comment thread convex/agentMessages.ts

@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: 17

Caution

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

⚠️ Outside diff range comments (3)
components/connections-rail.tsx (1)

23-35: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Reset rail state immediately when actorSlug changes to avoid stale cross-user rows.

Line 23 should clear prior rows and set loading before firing the next request; otherwise old actor data can remain visible until the new fetch resolves.

Suggested fix
   useEffect(() => {
     if (!actorSlug) {
       setLoaded(true);
       setRows([]);
       return;
     }
+    setLoaded(false);
+    setRows([]);
     let cancelled = false;
-    listConnections(actorSlug).then((r) => {
+    listConnections(actorSlug).then((r) => {
       if (!cancelled) {
         setRows(r);
         setLoaded(true);
       }
     });
🤖 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/connections-rail.tsx` around lines 23 - 35, When actorSlug changes
we should immediately clear prior rows and flip loading state before issuing
listConnections to avoid showing stale data; inside the useEffect that
references actorSlug, call setRows([]) and setLoaded(false) right after the
actorSlug truthy check (before creating cancelled and invoking listConnections),
then proceed with the existing cancelled handling and setRows(r)/setLoaded(true)
in the promise resolution; keep the early-return branch for falsy actorSlug that
still setsRows([]) and setLoaded(true).
app/api/agent/start/route.ts (1)

42-46: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Allow attachment-only turns through this guard.

The composer now permits sends with files and no text, but this check still rejects any request whose text trims to empty. That makes the new attachment flow fail before it ever reaches Convex/Hermes.

Suggested fix
-  if (!body.conversationId || !body.text?.trim()) {
+  const hasText = typeof body.text === "string" && body.text.trim().length > 0;
+  const hasAttachments =
+    Array.isArray(body.attachments) && body.attachments.length > 0;
+  if (!body.conversationId || (!hasText && !hasAttachments)) {
     return Response.json(
-      { error: "conversationId + text required" },
+      { error: "conversationId and text or attachments required" },
       { status: 400 },
     );
   }
🤖 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 `@app/api/agent/start/route.ts` around lines 42 - 46, The current guard in
route.ts rejects requests where body.text trims to empty even when an attachment
is present; update the validation in the request handler so it requires
body.conversationId and either non-empty trimmed body.text OR a non-empty
body.files array (or equivalent attachments field used by the composer), and
update the error message accordingly; look for the conditional that checks
body.conversationId and body.text (the if block that returns the 400 Response)
and change it to allow requests with attachments (e.g., body.files?.length > 0)
to pass.
lib/use-hermes-chat.ts (1)

242-250: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Handle non-2xx cancel responses instead of swallowing them.

stop() ignores res.ok, so a 4xx/5xx from /api/agent/cancel is treated as success and the UI never surfaces the failure. This path should mirror sendMessage/regenerate and set sendError on HTTP errors.

Suggested patch
   const stop = useCallback(async () => {
     if (!active || !actorSlug) return;
+    setSendError(null);
     try {
-      await fetch("/api/agent/cancel", {
+      const res = await fetch("/api/agent/cancel", {
         method: "POST",
         headers: { "Content-Type": "application/json" },
         body: JSON.stringify({
           turnId: active.turn._id,
         }),
       });
+      if (!res.ok) {
+        const detail = await res.json().catch(() => ({}));
+        throw new Error(
+          (detail as { error?: string }).error ??
+            `Cancel route failed: ${res.status}`,
+        );
+      }
     } catch (err) {
-      console.error("[chat] cancel failed:", err);
+      setSendError(err instanceof Error ? err.message : "Cancel failed.");
     }
   }, [active, actorSlug]);
🤖 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 242 - 250, The cancel path in stop()
currently ignores non-2xx responses; modify the fetch call in use-hermes-chat.ts
(the stop() implementation that posts to "/api/agent/cancel" with turnId =
active.turn._id) to capture the Response, check res.ok, and on failure parse the
error body (json/text) and call setSendError(...) with a descriptive message
(mirroring sendMessage/regenerate behavior) before returning/throwing; keep the
existing console.error but ensure HTTP errors are surfaced via sendError so the
UI can show the failure.
🧹 Nitpick comments (6)
components/agent-panel.tsx (1)

39-51: ⚡ Quick win

Derive the existing conversation id instead of setting it in the effect.

The synchronous setConversationId(conversations[0]._id) here is what ESLint is flagging on Line 47. Let the query result provide the existing id directly, and keep the effect creation-only.

♻️ One way to make the effect creation-only
-  const [conversationId, setConversationId] =
+  const [createdConversationId, setCreatedConversationId] =
     useState<Id<"agent_conversations"> | null>(null);
+  const conversationId =
+    createdConversationId ?? conversations?.[0]?._id ?? null;
@@
   useEffect(() => {
     if (!actorSlug || !conversations) return;
     if (conversationId) return;
-    if (conversations.length > 0) {
-      setConversationId(conversations[0]._id);
-    } else if (open) {
-      create({ visibility: "personal" }).then(setConversationId);
-    }
+    if (!open || conversations.length > 0) return;
+    void create({ visibility: "personal" }).then(setCreatedConversationId);
   }, [actorSlug, conversations, conversationId, open, create]);
🤖 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/agent-panel.tsx` around lines 39 - 51, The effect currently
mutates state synchronously via setConversationId(conversations[0]._id); instead
derive the existing id from the query result and make the effect creation-only:
remove the synchronous setConversationId call from the useEffect block and
instead compute the effective id elsewhere (e.g., const effectiveConversationId
= conversationId ?? conversations?.[0]?._id or via a memo/selector) so the
effect (useEffect) only calls create({ visibility: "personal" }) when open and
no conversation exists; keep references to actorSlug, conversations,
conversationId, open, create to locate the code.
components/controls/capability-editor.tsx (1)

27-45: ⚡ Quick win

Skip the actor lookup in the guest path.

The read-only branch on Line 73 returns before any mutation can run, but the api.fdes.getBySlug query on Lines 37-40 still fires for guests. Passing "skip" when !op removes an unnecessary Convex request from the template detail page.

Suggested fix
   const actor = useQuery(
     api.fdes.getBySlug,
-    actorSlug ? { slug: actorSlug } : "skip",
+    op && actorSlug ? { slug: actorSlug } : "skip",
   ) as { _id: string } | null | undefined;

Also applies to: 73-92

🤖 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/controls/capability-editor.tsx` around lines 27 - 45, The actor
lookup fires even for guests; change the useQuery call that fetches
api.fdes.getBySlug to pass "skip" when the user is not an operator
(useIsOperator -> op) or when actorSlug is falsy so the Convex request is
omitted for the guest/read-only path; update the call that currently uses
actorSlug ? { slug: actorSlug } : "skip" to additionally check op (e.g., op &&
actorSlug ? { slug: actorSlug } : "skip") so mutations in the operator path
still work and guests don’t trigger api.fdes.getBySlug.
components/controls/inline-name.tsx (1)

31-41: ⚡ Quick win

Skip the unused Convex lookups for guests.

When !op, this component renders a plain <span> from current, so the entity and actor queries on Lines 32-40 do no useful work. Skipping them trims two requests from the guest template header path.

Suggested fix
-  const entity = useQuery(API[kind].get, { slug }) as
+  const entity = useQuery(
+    API[kind].get,
+    op ? { slug } : "skip",
+  ) as
     | { _id: string }
     | null
     | undefined;
@@
   const actor = useQuery(
     api.fdes.getBySlug,
-    actorSlug ? { slug: actorSlug } : "skip",
+    op && actorSlug ? { slug: actorSlug } : "skip",
   ) as { _id: string } | null | undefined;

Also applies to: 74-76

🤖 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/controls/inline-name.tsx` around lines 31 - 41, The component
performs Convex lookups unconditionally even for guests; when useIsOperator()
(op) is falsy these queries are unused. Modify the code so that entity =
useQuery(API[kind].get, { slug }) and actor = useQuery(api.fdes.getBySlug,
actorSlug ? { slug: actorSlug } : "skip") and the run =
useRunMutation(API[kind].update) call are only invoked/initialized when op is
true (e.g., return "skip" or null/undefined for queries and avoid calling
useRunMutation for guests), and apply the same conditional change to the second
occurrence referenced around lines 74-76 to prevent unnecessary Convex requests
for non-operators.
lib/load-overview.ts (1)

78-80: ⚡ Quick win

Avoid the second auth round-trip in template loaders.

loadTemplateData() re-runs requireSignedIn(), but both template pages already call requireSignedIn() to choose the RoleProvider role. That adds a second auth fetch to every template request for no extra protection.

🤖 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/load-overview.ts` around lines 78 - 80, loadTemplateData currently calls
requireSignedIn again, causing a redundant auth round-trip; remove the second
auth check by deleting the requireSignedIn() call inside loadTemplateData and
rely on the callers' prior requireSignedIn() (the template pages that set
RoleProvider). Update any tests or callers that expect loadTemplateData to
enforce auth, and ensure loadOverviewUnchecked and loadTemplateData remain
documented to assume the caller already authenticated; references:
loadTemplateData, requireSignedIn, loadOverviewUnchecked.
components/controls/github-repo-input.tsx (1)

23-33: ⚡ Quick win

Don't fetch template and actor records for the guest view.

The guest branch on Line 65 renders entirely from current, but the getBySlug and actor lookups on Lines 24-32 still run first. That turns every read-only repo field into two unused Convex requests.

Suggested fix
-  const tpl = useQuery(api.templates.getBySlug, { slug: templateSlug }) as
+  const tpl = useQuery(
+    api.templates.getBySlug,
+    op ? { slug: templateSlug } : "skip",
+  ) as
     | { _id: string }
     | null
     | undefined;
@@
   const actor = useQuery(
     api.fdes.getBySlug,
-    actorSlug ? { slug: actorSlug } : "skip",
+    op && actorSlug ? { slug: actorSlug } : "skip",
   ) as { _id: string } | null | undefined;

Also applies to: 65-79

🤖 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/controls/github-repo-input.tsx` around lines 23 - 33, The template
and actor Convex queries and the run mutation are executed even for the guest
view; change those hooks to no-op when not an operator: call
useQuery(api.templates.getBySlug, op ? { slug: templateSlug } : "skip") for tpl,
call useQuery(api.fdes.getBySlug, op && actorSlug ? { slug: actorSlug } :
"skip") for actor (leave useActorSlug as-is), and set run to op ?
useRunMutation(api.templates.setGithubRepo) : null so no Convex reads/mutation
hooks run for guests; reference the existing symbols useIsOperator,
templateSlug, actorSlug, api.templates.getBySlug, api.fdes.getBySlug,
useRunMutation, and api.templates.setGithubRepo when making these changes.
components/controls/template-tags-input.tsx (1)

111-117: ⚡ Quick win

Keep the guest path read-only all the way through.

The new branch renders only selected, but the component still builds autocomplete state from the global tags list first. That extra fetch isn't needed for guests and widens the metadata exposed beyond the current template. Splitting out a read-only guest view here would keep the guest path narrow.

🤖 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/controls/template-tags-input.tsx` around lines 111 - 117, The
guest-readonly branch is rendered after the component still constructs
autocomplete state from the global tags list; avoid that by short-circuiting the
guest path before any autocomplete setup or global tag fetch. Move the existing
if (!op) return (selected ? <TemplateTagsChips tags={selected} /> : read-only
placeholder) to the top of the TemplateTagsInput component (or add a guard that
returns immediately), and ensure related effects/logic that build autocomplete
state or call the global tags list (e.g., any useEffect, buildAutocomplete,
fetchTags, or state initialization for autocomplete) are not executed when op is
falsy so guests never trigger the fetch or expose extra metadata.
🤖 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 47-62: The manual-auth branch currently only checks that a user
exists (me) but does not verify operator privileges; after calling
fetchAuthQuery(api.auth.getCurrentUser, {}) and assigning me, explicitly check
me?.isOperator (or the appropriate boolean property returned) and return a 403
Response.json error if it is false/undefined; keep the existing unauthenticated
catch and only allow proceeding when cron is true or when me exists and
me.isOperator is truthy.
- Around line 162-166: Currently GET is aliased to POST which allows
state-changing sync via a safe verb; change the GET export so it no longer
directly mutates state unless the request is cron-authenticated: replace "export
const GET = POST" with an explicit GET handler that checks the cron auth (e.g.,
verify the Vercel cron header or your existing cron auth helper) and only
delegates to POST when authenticated, otherwise return a 405/Forbidden response;
reference the GET and POST exports in app/api/templates/sync-github/route.ts and
reuse your existing cron-auth check routine (or add one) to centralize the
authorization logic.

In `@components/chat-landing.tsx`:
- Around line 245-255: The submit() handler currently sends only attachments
that finished uploading and allows sending while other attachments are still
uploading, causing orphaned chips; update submit (and the other call site around
lines 469-475) to check attachment upload state before sending—either by adding
a per-attachment flag (e.g., attachment.uploading) or maintaining a
pendingUploadsCount state—and return / disable the send action when any
attachment.uploading === true or pendingUploadsCount > 0; ensure sendMessage is
only called when all attachments have finished uploading and include reference
to pendingAttachments and submit() so reviewers can locate where to add the
check and UI disable.

In `@components/chat-sidebar.tsx`:
- Around line 65-68: The merged conversations array created by useMemo
(allConversations from personal and shared) isn't sorted, so selecting
allConversations[0] can pick an older personal chat; sort the combined list by
the updated_at timestamp descending before returning it so the newest
conversation is first. Update the useMemo that builds allConversations (and any
analogous merge logic around the auto-select code at lines referenced near
73-78) to perform a stable sort comparing item.updated_at (or item.updatedAt) in
descending order, then use the first element for auto-selection.

In `@components/controls/template-author-select.tsx`:
- Around line 61-67: The guest/read‑only branch in template-author-select.tsx
(the if (!op) path) currently relies on the full fdes list (fdes and
api.fdes.list) to resolve currentFdeSlug, which exposes operator-only data;
instead stop using fdes there and use a guest-safe single-name source: accept
and use a preloaded currentFdeName (or currentAuthorName) passed from the page
loader, or call a guest-safe single-record API (e.g., an api.fdes.getPublic or
getBySlugSafe) to resolve currentFdeSlug; update the component to read
currentFdeName when op is falsy and remove dependence on fdes/api.fdes.list in
that branch so guests never trigger the full directory fetch.

In `@components/controls/template-origin-select.tsx`:
- Around line 63-72: The guest/readonly branch in template-origin-select.tsx
currently reads the full customers list (customers) to render the display name,
which leaks data and can fail when list is operator-only; change the branch so
it does not rely on customers: read the single display value via a guest-safe
source (e.g. accept a currentCustomerName in the page props or call a guest-safe
single-record lookup like customers.getBySlug or customers.findOneGuestSafe) and
render currentCustomerName (instead of resolving currentCustomer from customers
using currentCustomerSlug); update any callers to pass currentCustomerName when
rendering the read-only path and remove the dependence on the customers list in
the !op branch.

In `@components/sections/template-overview.tsx`:
- Line 59: The JSX always renders "reuses" regardless of u.reuseCount; update
the markup around the span that displays reuse count (the element referencing
u.reuseCount in components/sections/template-overview.tsx) to conditionally
choose the label based on the numeric value, e.g. use a ternary on u.reuseCount
(u.reuseCount === 1 ? 'reuse' : 'reuses') so "1" shows "1 reuse" and other
counts show "reuses".

In `@components/template-github-candidates.tsx`:
- Around line 106-109: The onClick handler calls dismiss({ id: c._id }) and
unconditionally shows toast.success, so failures give no feedback; wrap the
await dismiss(...) call in a try/catch inside the onClick async function, call
toast.success("Dismissed.") only on success, and call toast.error(...) in the
catch block (optionally including the error message) to surface failures to the
user; locate the anonymous onClick handler in
components/template-github-candidates.tsx where dismiss and toast.success are
used.

In `@convex/agentMessages.ts`:
- Around line 35-63: The shared-conversation branches are currently accessible
to any signed-in user; restrict them to operators by invoking the existing
operator check (e.g., call requireOperator(ctx) or use the module's
requireOperator helper) before exposing or mutating any rows with visibility ===
"shared" inside listConversations and the other shared-handling handlers (the
functions that read or write shared rows referenced around the other diffs). For
read paths, only include shared rows after requireOperator(ctx) succeeds; for
create/update paths, requireOperator(ctx) before allowing visibility to be set
to "shared" or flipping a personal conversation to shared.

In `@convex/agentTurns.ts`:
- Around line 350-360: The regenerate path currently returns only user_text so
attachments are dropped; update regenerateLast (in convex/agentTurns.ts) to
fetch the original user message attachments (same as startTurn does) and include
an attachment_links (or attachmentLinks) field in the returned object alongside
user_text and user_message_id so /api/agent/regenerate forwards the same
attachment_links to Hermes; locate the userMsg retrieval in regenerateLast and
copy the attachment resolution logic used by startTurn (using
latest.user_message_id and userMsg) to populate the attachment_links output.

In `@convex/lib/conversationAuth.ts`:
- Around line 51-64: The canAccess function currently allows any authenticated
user to access visibility === "shared"; update AuthUser to carry operator state
(e.g., an isOperator or role field populated by requireUser/convex/auth.ts),
then change canAccess to only return true for shared conversations when the user
is an operator (check the new AuthUser flag inside canAccess) while preserving
the existing owner_user_id check for personal conversations and the legacy
unowned behavior; ensure requireUser/convex/auth.ts sets that flag for operator
accounts so the check is meaningful.

In `@convex/migrations.ts`:
- Around line 61-76: The current migration claims unowned agent_conversations
rows solely by user.slug (local-part), which can collide across domains; change
the claim logic in the requireUser -> rows loop so we do NOT claim based only on
user.slug: require a stronger identity signal (e.g., user.email and
user.emailVerified or a row-level owner_email/owner_identity_verified field)
before calling ctx.db.patch on row._id, otherwise skip the row (leave ambiguous
rows unclaimed) or mark them for manual/backfill processing; update the
condition that checks (row.visibility ?? "personal") and the subsequent patch to
only run when the stronger identity check passes.

In `@convex/templates.ts`:
- Around line 24-84: The public Convex endpoints for GitHub candidates are
currently unprotected; add authorization checks at the start of each handler: in
listGithubCandidates' handler call assertOperatorRead(ctx) before any DB access,
and in upsertGithubCandidate, dismissGithubCandidate, and markCandidatePromoted
handlers call assertOperator(ctx) before performing inserts/patches (or convert
the sync-only write path to an internal-only function if it should not be
client-callable). Ensure you import/require the assertOperator and
assertOperatorRead helpers and place the checks at the top of the respective
handlers (listGithubCandidates, upsertGithubCandidate, dismissGithubCandidate,
markCandidatePromoted).

In `@lib/load-overview.ts`:
- Around line 43-49: The catch is currently converting any exception from
fetchAuthQuery (called with api.auth.getCurrentUser) into a redirect to
"/sign-in", hiding real errors; change the try/catch so that you do not
catch-all exceptions — let unexpected errors rethrow — and only perform redirect
when the call succeeds but returns a falsy user (i.e., check the user variable
after await fetchAuthQuery and call redirect("/sign-in") only when user is
null/undefined). Update both places that call fetchAuthQuery (the block around
fetchAuthQuery(api.auth.getCurrentUser, {}) and the similar block at lines
60–68) so the catch is removed or rethrows the error and the redirect occurs
solely based on the resolved user value.

In `@lib/use-chat-draft.ts`:
- Around line 13-15: The draft localStorage key in useChatDraft is only
conversation-scoped (key = `castle:chat-draft:${conversationId}`) and must be
scoped by the authenticated user as well; change useChatDraft to accept a stable
authenticated user identifier (e.g., userId) from the caller and include it in
the key (for example `castle:chat-draft:${userId}:${conversationId}`), handle
null/anonymous cases consistently (skip persistence or use an explicit "anon"
token), and update any callers to pass the stable user identifier instead of
relying on actorSlug so drafts are isolated per user+conversation.

In `@lib/use-hermes-chat.ts`:
- Around line 196-203: The send/cancel guard incorrectly blocks operations when
actorSlug is null even though conversationId/turnId suffice; remove the
actorSlug check from the early-return condition(s) that include (!trimmed &&
!hasAttachments) || sending || !actorSlug || !conversationId so that only
trimmed/hasAttachments, sending, and missing conversationId/turnId block
actions; update both occurrences (the send guard and the cancel guard around the
same boolean expression) and run tests to confirm the backend calls that use
conversationId/turnId still function when actorSlug is null.

In `@pnpm-workspace.yaml`:
- Around line 1-8: The entries for sharp and unrs-resolver conflict because they
appear in both allowBuilds and ignoredBuiltDependencies; decide whether you want
to build them locally or ignore their prebuilt binaries and then update the
configuration accordingly: either remove "sharp" and "unrs-resolver" from the
ignoredBuiltDependencies list if you want local builds (keep allowBuilds), or
remove them from allowBuilds if you intend to continue using ignored prebuilt
binaries, and if keeping both is intentional add a clear inline comment
explaining the special case; references: allowBuilds, ignoredBuiltDependencies,
package names sharp and unrs-resolver.

---

Outside diff comments:
In `@app/api/agent/start/route.ts`:
- Around line 42-46: The current guard in route.ts rejects requests where
body.text trims to empty even when an attachment is present; update the
validation in the request handler so it requires body.conversationId and either
non-empty trimmed body.text OR a non-empty body.files array (or equivalent
attachments field used by the composer), and update the error message
accordingly; look for the conditional that checks body.conversationId and
body.text (the if block that returns the 400 Response) and change it to allow
requests with attachments (e.g., body.files?.length > 0) to pass.

In `@components/connections-rail.tsx`:
- Around line 23-35: When actorSlug changes we should immediately clear prior
rows and flip loading state before issuing listConnections to avoid showing
stale data; inside the useEffect that references actorSlug, call setRows([]) and
setLoaded(false) right after the actorSlug truthy check (before creating
cancelled and invoking listConnections), then proceed with the existing
cancelled handling and setRows(r)/setLoaded(true) in the promise resolution;
keep the early-return branch for falsy actorSlug that still setsRows([]) and
setLoaded(true).

In `@lib/use-hermes-chat.ts`:
- Around line 242-250: The cancel path in stop() currently ignores non-2xx
responses; modify the fetch call in use-hermes-chat.ts (the stop()
implementation that posts to "/api/agent/cancel" with turnId = active.turn._id)
to capture the Response, check res.ok, and on failure parse the error body
(json/text) and call setSendError(...) with a descriptive message (mirroring
sendMessage/regenerate behavior) before returning/throwing; keep the existing
console.error but ensure HTTP errors are surfaced via sendError so the UI can
show the failure.

---

Nitpick comments:
In `@components/agent-panel.tsx`:
- Around line 39-51: The effect currently mutates state synchronously via
setConversationId(conversations[0]._id); instead derive the existing id from the
query result and make the effect creation-only: remove the synchronous
setConversationId call from the useEffect block and instead compute the
effective id elsewhere (e.g., const effectiveConversationId = conversationId ??
conversations?.[0]?._id or via a memo/selector) so the effect (useEffect) only
calls create({ visibility: "personal" }) when open and no conversation exists;
keep references to actorSlug, conversations, conversationId, open, create to
locate the code.

In `@components/controls/capability-editor.tsx`:
- Around line 27-45: The actor lookup fires even for guests; change the useQuery
call that fetches api.fdes.getBySlug to pass "skip" when the user is not an
operator (useIsOperator -> op) or when actorSlug is falsy so the Convex request
is omitted for the guest/read-only path; update the call that currently uses
actorSlug ? { slug: actorSlug } : "skip" to additionally check op (e.g., op &&
actorSlug ? { slug: actorSlug } : "skip") so mutations in the operator path
still work and guests don’t trigger api.fdes.getBySlug.

In `@components/controls/github-repo-input.tsx`:
- Around line 23-33: The template and actor Convex queries and the run mutation
are executed even for the guest view; change those hooks to no-op when not an
operator: call useQuery(api.templates.getBySlug, op ? { slug: templateSlug } :
"skip") for tpl, call useQuery(api.fdes.getBySlug, op && actorSlug ? { slug:
actorSlug } : "skip") for actor (leave useActorSlug as-is), and set run to op ?
useRunMutation(api.templates.setGithubRepo) : null so no Convex reads/mutation
hooks run for guests; reference the existing symbols useIsOperator,
templateSlug, actorSlug, api.templates.getBySlug, api.fdes.getBySlug,
useRunMutation, and api.templates.setGithubRepo when making these changes.

In `@components/controls/inline-name.tsx`:
- Around line 31-41: The component performs Convex lookups unconditionally even
for guests; when useIsOperator() (op) is falsy these queries are unused. Modify
the code so that entity = useQuery(API[kind].get, { slug }) and actor =
useQuery(api.fdes.getBySlug, actorSlug ? { slug: actorSlug } : "skip") and the
run = useRunMutation(API[kind].update) call are only invoked/initialized when op
is true (e.g., return "skip" or null/undefined for queries and avoid calling
useRunMutation for guests), and apply the same conditional change to the second
occurrence referenced around lines 74-76 to prevent unnecessary Convex requests
for non-operators.

In `@components/controls/template-tags-input.tsx`:
- Around line 111-117: The guest-readonly branch is rendered after the component
still constructs autocomplete state from the global tags list; avoid that by
short-circuiting the guest path before any autocomplete setup or global tag
fetch. Move the existing if (!op) return (selected ? <TemplateTagsChips
tags={selected} /> : read-only placeholder) to the top of the TemplateTagsInput
component (or add a guard that returns immediately), and ensure related
effects/logic that build autocomplete state or call the global tags list (e.g.,
any useEffect, buildAutocomplete, fetchTags, or state initialization for
autocomplete) are not executed when op is falsy so guests never trigger the
fetch or expose extra metadata.

In `@lib/load-overview.ts`:
- Around line 78-80: loadTemplateData currently calls requireSignedIn again,
causing a redundant auth round-trip; remove the second auth check by deleting
the requireSignedIn() call inside loadTemplateData and rely on the callers'
prior requireSignedIn() (the template pages that set RoleProvider). Update any
tests or callers that expect loadTemplateData to enforce auth, and ensure
loadOverviewUnchecked and loadTemplateData remain documented to assume the
caller already authenticated; references: loadTemplateData, requireSignedIn,
loadOverviewUnchecked.
🪄 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 Plus

Run ID: 5dd67525-123e-463b-82dd-dfe02eaf4ba5

📥 Commits

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

⛔ Files ignored due to path filters (1)
  • convex/_generated/api.d.ts is excluded by !**/_generated/**
📒 Files selected for processing (48)
  • 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/page.tsx
  • app/templates/[slug]/page.tsx
  • app/templates/page.tsx
  • components/agent-panel.tsx
  • components/chat-landing.tsx
  • components/chat-sidebar.tsx
  • components/connections-rail.tsx
  • components/controls/capability-editor.tsx
  • components/controls/extraction-row-delete.tsx
  • components/controls/github-repo-input.tsx
  • components/controls/inline-name.tsx
  • components/controls/live-url-input.tsx
  • components/controls/template-author-select.tsx
  • components/controls/template-category-menu.tsx
  • components/controls/template-origin-select.tsx
  • components/controls/template-tags-input.tsx
  • components/profile-menu.tsx
  • components/sections/attention.tsx
  • components/sections/extraction-timeline.tsx
  • components/sections/mrr-chart.tsx
  • components/sections/template-overview.tsx
  • components/template-github-candidates.tsx
  • components/top-nav.tsx
  • convex/agentMessages.ts
  • convex/agentTurns.ts
  • convex/auth.ts
  • convex/emailAllowlist.ts
  • convex/lib/assertOperator.ts
  • convex/lib/conversationAuth.ts
  • convex/migrations.ts
  • convex/patternExtractions.ts
  • convex/schema.ts
  • convex/templates.ts
  • docker/hermes/serve.py
  • lib/chat-context.tsx
  • lib/load-overview.ts
  • lib/role-context.tsx
  • lib/use-chat-draft.ts
  • lib/use-hermes-chat.ts
  • middleware.ts
  • pnpm-workspace.yaml
  • vercel.json
💤 Files with no reviewable changes (1)
  • app/api/agent/route.ts

Comment thread app/api/templates/sync-github/route.ts Outdated
Comment thread app/api/templates/sync-github/route.ts Outdated
Comment thread components/chat-landing.tsx
Comment thread components/chat-sidebar.tsx
Comment thread components/controls/template-author-select.tsx
Comment thread convex/templates.ts
Comment thread lib/load-overview.ts
Comment thread lib/use-chat-draft.ts
Comment on lines +13 to +15
export function useChatDraft(conversationId: string | null) {
const key = conversationId ? `castle:chat-draft:${conversationId}` : null;
const [draft, setDraftState] = useState("");

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.

⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

Scope draft persistence by authenticated user, not just conversation.

The localStorage key is only conversation-scoped. With shared chats in this PR, signing into the same browser as a different account and reopening that conversation will hydrate the previous user's unsent draft.

Suggested direction
-export function useChatDraft(conversationId: string | null) {
-  const key = conversationId ? `castle:chat-draft:${conversationId}` : null;
+export function useChatDraft(
+  conversationId: string | null,
+  userScope: string | null,
+) {
+  const key =
+    conversationId && userScope
+      ? `castle:chat-draft:${userScope}:${conversationId}`
+      : null;

Then pass a stable authenticated user identifier from the caller. actorSlug is not sufficient here because shared conversations can be used by multiple users.

📝 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
export function useChatDraft(conversationId: string | null) {
const key = conversationId ? `castle:chat-draft:${conversationId}` : null;
const [draft, setDraftState] = useState("");
export function useChatDraft(
conversationId: string | null,
userScope: string | null,
) {
const key =
conversationId && userScope
? `castle:chat-draft:${userScope}:${conversationId}`
: null;
const [draft, setDraftState] = useState("");
🤖 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-chat-draft.ts` around lines 13 - 15, The draft localStorage key in
useChatDraft is only conversation-scoped (key =
`castle:chat-draft:${conversationId}`) and must be scoped by the authenticated
user as well; change useChatDraft to accept a stable authenticated user
identifier (e.g., userId) from the caller and include it in the key (for example
`castle:chat-draft:${userId}:${conversationId}`), handle null/anonymous cases
consistently (skip persistence or use an explicit "anon" token), and update any
callers to pass the stable user identifier instead of relying on actorSlug so
drafts are isolated per user+conversation.

Comment thread lib/use-hermes-chat.ts
Comment thread pnpm-workspace.yaml Outdated
Security (P1/Critical):
- templates.ts: guard listGithubCandidates, upsertGithubCandidate,
  dismissGithubCandidate, markCandidatePromoted with assertOperator
- conversationAuth.ts: shared conversations now require operator
  status (guests can't read/write shared chats)
- agentMessages.ts: generateAttachmentUploadUrl requires operator,
  setVisibility only allows original owner to change visibility

Auth hardening (Major):
- sync-github route: check isOperator (not just session presence)
- GET handler restricted to cron-authenticated calls only (no CSRF)

Minor fixes:
- template-overview: fix "1 reuses" → "1 reuse" pluralization
- template-github-candidates: wrap dismiss in try/catch with error toast
- pnpm-workspace.yaml: remove contradictory ignoredBuiltDependencies

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

@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.

Caution

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

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

41-48: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift

This operator check breaks the cron sync path.

/api/templates/sync-github intentionally calls upsertGithubCandidate through an unauthenticated ConvexHttpClient when Vercel Cron runs. With assertOperator(ctx) here, every cron upsert now throws, so the daily sync will discover repos but never persist them. Keep the client-facing mutation operator-only, but give cron a separate server-only/internal write path or a trusted operator identity.

🤖 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/templates.ts` around lines 41 - 48, The current assertOperator(ctx) in
the mutation upsertGithubCandidate blocks unauthenticated cron calls from
/api/templates/sync-github (which uses an unauthenticated ConvexHttpClient);
remove the operator-only guard from this client-facing mutation and instead
expose a separate server-only/internal write path that the cron job can call
(e.g., create an internal function or mutation like
internalUpsertGithubCandidate or a server-only handler invoked by the cron
route) or accept a trusted operator identity for cron; update the cron caller to
use that server-only function and keep upsertGithubCandidate as client-facing
(or vice versa) so operator checks don't prevent the scheduled sync from
persisting discovered repos.
♻️ Duplicate comments (1)
convex/agentMessages.ts (1)

40-63: ⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Shared conversation flows are still reachable by guests.

requireConversation is operator-aware now, but these handlers still treat any signed-in user as eligible for shared chat access. In this PR guests can authenticate, so they can still list all shared conversations, create a new shared conversation, or promote their own personal conversation to shared. Add the operator gate on every visibility === "shared" read/write branch here as well.

Also applies to: 86-95, 98-119, 207-228

🤖 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 40 - 63, listConversations currently
returns all records with visibility === "shared" to any authenticated user; add
an operator gate so only operators can read shared conversations. After
obtaining user via requireUser(ctx) check operator privileges (e.g. call
requireOperator(ctx) or assert user.is_operator) before querying
agent_conversations with visibility "shared" and skip the shared-query/merge if
not operator. Apply the same pattern to every other read/write branch that uses
visibility === "shared" (the handlers referenced at the other ranges) so any
access or promotion to shared is only allowed for operators.
🤖 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.

Outside diff comments:
In `@convex/templates.ts`:
- Around line 41-48: The current assertOperator(ctx) in the mutation
upsertGithubCandidate blocks unauthenticated cron calls from
/api/templates/sync-github (which uses an unauthenticated ConvexHttpClient);
remove the operator-only guard from this client-facing mutation and instead
expose a separate server-only/internal write path that the cron job can call
(e.g., create an internal function or mutation like
internalUpsertGithubCandidate or a server-only handler invoked by the cron
route) or accept a trusted operator identity for cron; update the cron caller to
use that server-only function and keep upsertGithubCandidate as client-facing
(or vice versa) so operator checks don't prevent the scheduled sync from
persisting discovered repos.

---

Duplicate comments:
In `@convex/agentMessages.ts`:
- Around line 40-63: listConversations currently returns all records with
visibility === "shared" to any authenticated user; add an operator gate so only
operators can read shared conversations. After obtaining user via
requireUser(ctx) check operator privileges (e.g. call requireOperator(ctx) or
assert user.is_operator) before querying agent_conversations with visibility
"shared" and skip the shared-query/merge if not operator. Apply the same pattern
to every other read/write branch that uses visibility === "shared" (the handlers
referenced at the other ranges) so any access or promotion to shared is only
allowed for operators.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 54f9a347-de05-4663-9e41-ac2052ac55b6

📥 Commits

Reviewing files that changed from the base of the PR and between c7ae255 and 7ec7274.

📒 Files selected for processing (7)
  • app/api/templates/sync-github/route.ts
  • components/sections/template-overview.tsx
  • components/template-github-candidates.tsx
  • convex/agentMessages.ts
  • convex/lib/conversationAuth.ts
  • convex/templates.ts
  • pnpm-workspace.yaml
💤 Files with no reviewable changes (1)
  • pnpm-workspace.yaml

Comment on lines +126 to +135
let skipped = 0;
for (const r of repos) {
if (r.archived) {
skipped++;
continue;
}
if (!r.full_name || !r.name) {
skipped++;
continue;
}

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 Cron path silently fails after assertOperator was added to upsertGithubCandidate

The cron branch creates a plain, unauthenticated ConvexHttpClient and calls upsertGithubCandidate, which now unconditionally calls await assertOperator(ctx). Because there is no session on the unauthenticated client, safeGetAuthUser returns null, assertOperator throws "unauthorized", and the mutation is caught by the surrounding try/catchskipped++. Every repo is silently skipped; the route still returns { ok: true, found: N, inserted: 0, skipped: N } and the Vercel cron job has no idea something went wrong. The daily template-sync feature is effectively broken.

The cleanest fix is to expose the upsert as a Convex internalMutation (callable only from trusted server-side actions/mutations, not from the browser client) so the cron can call it without needing an operator session, while the browser path continues to call the public mutation guarded by assertOperator.

Comment thread convex/agentMessages.ts
Comment thread convex/lib/conversationAuth.ts Outdated
…, UX

Security:
- fdes.ts, customers.ts: guard list queries with assertOperatorRead
- template-author-select, template-origin-select: skip list queries
  for guests, use server-passed currentName prop instead

Correctness:
- agent/start route: allow attachment-only turns (no text required)
- use-hermes-chat: remove stale actorSlug guard from send/cancel
  (route no longer needs it, was blocking valid sends)
- agentTurns.regenerateLast: resolve and return attachment_links so
  regeneration preserves original file context
- agent/regenerate route: forward attachment_links to Hermes
- load-overview: don't mask non-auth errors as sign-in redirects
  (let Convex outages propagate instead of silent redirect)

UX:
- chat-landing: block submit while uploads still in progress
- connections-rail: clear stale rows immediately on actor switch
- chat-sidebar: sort merged conversation list by updated_at before
  auto-selecting most recent

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread convex/agentMessages.ts
- upsertGithubCandidate: remove assertOperator (breaks cron path);
  access is gated at the HTTP route level instead
- listShared: guard with assertOperatorRead (was leaking shared
  conversations to guests)
- createConversation: guard with assertOperator (guests could create
  conversations and use the chat by calling Convex directly)
- canAccess: restore legacy actor_slug fallback for unowned
  conversations (pre-migration chats were inaccessible)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment thread lib/load-overview.ts
Comment on lines +44 to +53
try {
const { fetchAuthQuery } = await import("@/lib/auth-server");
user = await fetchAuthQuery(api.auth.getCurrentUser, {});
} catch (err) {
if (isRedirectError(err)) throw err;
throw err;
}
if (!user) redirect("/sign-in");
return { isOperator: user.isOperator ?? false };
}

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 The catch blocks in both requireSignedIn and requireOperator are functionally no-ops: the if (isRedirectError(err)) throw err branch and the fallback throw err both rethrow unconditionally. The old requireOperator redirected to /sign-in on any error from fetchAuthQuery; now a transient Convex failure, auth system error, or invalid-but-present session cookie all surface as a raw 500 page rather than a clean sign-in redirect. The standard Next.js pattern requires a fallback after the isRedirectError guard.

Suggested change
try {
const { fetchAuthQuery } = await import("@/lib/auth-server");
user = await fetchAuthQuery(api.auth.getCurrentUser, {});
} catch (err) {
if (isRedirectError(err)) throw err;
throw err;
}
if (!user) redirect("/sign-in");
return { isOperator: user.isOperator ?? false };
}
try {
const { fetchAuthQuery } = await import("@/lib/auth-server");
user = await fetchAuthQuery(api.auth.getCurrentUser, {});
} catch (err) {
if (isRedirectError(err)) throw err;
redirect("/sign-in");
}
if (!user) redirect("/sign-in");
return { isOperator: user.isOperator ?? false };
}

@undeemed undeemed merged commit c6c5a67 into main May 26, 2026
3 checks passed
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