Conversation
|
|
Warning Review limit reached
More reviews will be available in 26 minutes and 20 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (30)
WalkthroughThis PR introduces complete agent persona management to Hermes Hub. Users can edit and save persona text (up to 20,000 characters), deploy it to a managed Hermes server via SSH, and track deployment status. The feature includes database persistence, SSH-based file writes with base64 encoding for safe transfer, audit logging, and a full React UI with loading states and error handling. ChangesAgent Persona Settings
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
No React Doctor issues found in this scan.
Generated by React Doctor. Questions? Contact founders@million.dev. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces the ability to configure and deploy a custom agent persona (saved to SOUL.md on the deployed Hermes server). It adds the hermes_settings database table, backend endpoints for saving and deploying the persona, and a frontend UI with state management and tests. The review feedback highlights two important robustness issues in server/settings.ts: a potential database query error if deployedServerId is null, and a potential TypeError if the parsed JSON payload itself is null.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| const telegramInfo = await getTelegramDeployInfo(session.user.id); | ||
| if (!telegramInfo) { | ||
| return context.json( | ||
| { | ||
| error: | ||
| "No Hermes deployment found. Deploy a Telegram bot to a server first.", | ||
| }, | ||
| 400, | ||
| ); | ||
| } |
There was a problem hiding this comment.
In the database schema, deployedServerId is a nullable field in the telegram_configs table. If a user has configured a Telegram bot but has not yet deployed it, telegramInfo will be returned but telegramInfo.deployedServerId will be null. Passing null to requireOwnedServerSshById can lead to database query errors or unexpected runtime exceptions. Adding a check to ensure deployedServerId is present prevents this issue.
| const telegramInfo = await getTelegramDeployInfo(session.user.id); | |
| if (!telegramInfo) { | |
| return context.json( | |
| { | |
| error: | |
| "No Hermes deployment found. Deploy a Telegram bot to a server first.", | |
| }, | |
| 400, | |
| ); | |
| } | |
| const telegramInfo = await getTelegramDeployInfo(session.user.id); | |
| if (!telegramInfo || !telegramInfo.deployedServerId) { | |
| return context.json( | |
| { | |
| error: | |
| "No Hermes deployment found. Deploy a Telegram bot to a server first.", | |
| }, | |
| 400, | |
| ); | |
| } |
| if (typeof payload.agentPersona !== "string") { | ||
| return context.json({ error: "Persona content is required." }, 400); | ||
| } |
There was a problem hiding this comment.
If the request payload is null (which can happen if the client sends a null JSON body), accessing payload.agentPersona will throw a TypeError: Cannot read properties of null (reading 'agentPersona'). Adding a null check for payload prevents potential unhandled exceptions and returns a clean 400 error.
| if (typeof payload.agentPersona !== "string") { | |
| return context.json({ error: "Persona content is required." }, 400); | |
| } | |
| if (!payload || typeof payload.agentPersona !== "string") { | |
| return context.json({ error: "Persona content is required." }, 400); | |
| } |
|
Note Unit test generation is a public access feature. Expect some limitations and changes as we gather feedback and continue to improve it. Generating unit tests... This may take up to 20 minutes. |
|
✅ Created PR with unit tests: #17 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
server/settings/config.test.ts (1)
5-35: ⚡ Quick winAdd boundary tests for the 20,000-character persona limit.
This suite covers required/trim behavior, but it misses the explicit size contract from this PR. Please add tests for exactly 20,000 chars (accept) and 20,001 chars (reject) to lock the API contract and prevent silent regressions.
🤖 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 `@server/settings/config.test.ts` around lines 5 - 35, Add two boundary tests for parsePersonaSaveBody to enforce the 20,000-character limit: create one test that passes a string of exactly 20,000 non-space characters (optionally with surrounding spaces to ensure trimming) and assert it returns { ok: true, content: <trimmed string> }, and another test that passes a string of 20,001 characters and assert it returns { ok: false, error: "Persona content exceeds maximum length." } (or the existing error message used by parsePersonaSaveBody). Place these tests alongside the existing parsePersonaSaveBody tests to lock the size contract.server/settings/config.ts (1)
8-21: 💤 Low valueConsider adding explicit array rejection.
The current check
!payload || typeof payload !== "object"accepts arrays (sincetypeof [] === "object"). While the subsequent type check on line 16 would correctly reject arrays (because[].agentPersonaisundefined), adding an explicitArray.isArray(payload)check would make the intent clearer and prevent potential confusion.♻️ Suggested refinement
export function parsePersonaSaveBody( payload: unknown, ): { ok: true; content: string } | { ok: false; error: string } { - if (!payload || typeof payload !== "object") { + if (!payload || typeof payload !== "object" || Array.isArray(payload)) { return { ok: false, error: "Persona content is required." }; }🤖 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 `@server/settings/config.ts` around lines 8 - 21, The payload object check in parsePersonaSaveBody currently treats arrays as objects; update the initial guard to also reject arrays (use Array.isArray(payload)) so arrays are treated as invalid input, then return the same { ok: false, error: "Persona content is required." } path; keep the rest of the function (reading (payload as { agentPersona?: unknown }).agentPersona and calling validateAgentPersona(agentPersona)) unchanged.src/features/settings/persona-settings.test.tsx (1)
56-159: ⚡ Quick winAdd test coverage for error scenarios.
The current test suite covers happy paths (successful save, successful deploy, button gating), but doesn't verify error handling. Adding tests for error responses and network failures would ensure the error UI works correctly and would have caught the missing catch blocks flagged in the production code.
📋 Suggested error test cases
Add tests after line 104 for save errors:
it("shows error message when save fails", async () => { fetchMock.mockResolvedValueOnce( new Response( JSON.stringify({ error: "Persona content cannot be empty." }), { status: 400, headers: { "content-type": "application/json" } } ) ); render(<PersonaSettings initialSettings={null} />); fireEvent.change(screen.getByLabelText(/persona content/i), { target: { value: "" }, }); fireEvent.click(screen.getByRole("button", { name: /save persona/i })); await flushAsyncWork(); expect(screen.getByText(/persona content cannot be empty/i)).toBeTruthy(); }); it("shows error message when save network fails", async () => { fetchMock.mockRejectedValueOnce(new Error("Network error")); render(<PersonaSettings initialSettings={null} />); fireEvent.change(screen.getByLabelText(/persona content/i), { target: { value: "Test" }, }); fireEvent.click(screen.getByRole("button", { name: /save persona/i })); await flushAsyncWork(); expect(screen.getByText(/network error/i)).toBeTruthy(); });Add tests after line 144 for deploy errors:
it("shows error message when deploy fails", async () => { fetchMock.mockResolvedValueOnce( new Response( JSON.stringify({ error: "No persona saved." }), { status: 400, headers: { "content-type": "application/json" } } ) ); render( <PersonaSettings initialSettings={{ agentPersona: "You are Hermes.", updatedAt: "2026-06-06T12:00:00.000Z", }} telegramDeploy={{ deployedServerHost: "1.2.3.4" }} /> ); fireEvent.click(screen.getByRole("button", { name: /deploy to hermes server/i })); await flushAsyncWork(); expect(screen.getByText(/no persona saved/i)).toBeTruthy(); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/settings/persona-settings.test.tsx` around lines 56 - 159, Add tests to persona-settings.test.tsx that cover error paths for PersonaSettings: write two save-error tests that mock fetchMock to return a 400 JSON error response and to reject (network error) and assert the error text appears after firing change + clicking the "Save persona" button and awaiting flushAsyncWork; and write a deploy-error test that mocks a 400 JSON error response for the POST to "/api/settings/persona/deploy", clicks the "Deploy to Hermes server" button on the rendered PersonaSettings (with telegramDeploy provided), awaits flushAsyncWork, and asserts the returned error message is displayed; reuse existing helpers (render, fireEvent, fetchMock, flushAsyncWork, screen) and assert via screen.getByText for the expected error strings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/settings/persona-settings-aside.tsx`:
- Around line 25-59: The handleDeploy function currently only uses try-finally
so a thrown fetch() (network/CORS) will bypass setting an error; wrap the
fetch+response parsing in a try-catch inside handleDeploy, and in the catch call
setDeployError with a user-friendly message (and optionally include
error.message), ensure setIsDeploying is still cleared in the existing finally,
and avoid setting deployResult when an error occurs; update references:
handleDeploy, setDeployError, setDeployResult, setIsDeploying and the
fetch("/api/settings/persona/deploy")/response.json parsing logic.
In `@src/features/settings/persona-settings.tsx`:
- Around line 30-60: The handleSave function currently only uses try/finally and
swallows fetch() rejections, so wrap the network call in a try/catch (inside the
existing handleSave) to catch errors from fetch() and response.json() that
bubble up, call setSaveMessage({ type: "error", text: <friendly message> }) with
the error message (or a generic "Network error while saving persona") and ensure
setIsSaving is still cleared in the existing finally; update references inside
handleSave to use the caught error when logging or setting the save message so
users see a failure if fetch throws.
---
Nitpick comments:
In `@server/settings/config.test.ts`:
- Around line 5-35: Add two boundary tests for parsePersonaSaveBody to enforce
the 20,000-character limit: create one test that passes a string of exactly
20,000 non-space characters (optionally with surrounding spaces to ensure
trimming) and assert it returns { ok: true, content: <trimmed string> }, and
another test that passes a string of 20,001 characters and assert it returns {
ok: false, error: "Persona content exceeds maximum length." } (or the existing
error message used by parsePersonaSaveBody). Place these tests alongside the
existing parsePersonaSaveBody tests to lock the size contract.
In `@server/settings/config.ts`:
- Around line 8-21: The payload object check in parsePersonaSaveBody currently
treats arrays as objects; update the initial guard to also reject arrays (use
Array.isArray(payload)) so arrays are treated as invalid input, then return the
same { ok: false, error: "Persona content is required." } path; keep the rest of
the function (reading (payload as { agentPersona?: unknown }).agentPersona and
calling validateAgentPersona(agentPersona)) unchanged.
In `@src/features/settings/persona-settings.test.tsx`:
- Around line 56-159: Add tests to persona-settings.test.tsx that cover error
paths for PersonaSettings: write two save-error tests that mock fetchMock to
return a 400 JSON error response and to reject (network error) and assert the
error text appears after firing change + clicking the "Save persona" button and
awaiting flushAsyncWork; and write a deploy-error test that mocks a 400 JSON
error response for the POST to "/api/settings/persona/deploy", clicks the
"Deploy to Hermes server" button on the rendered PersonaSettings (with
telegramDeploy provided), awaits flushAsyncWork, and asserts the returned error
message is displayed; reuse existing helpers (render, fireEvent, fetchMock,
flushAsyncWork, screen) and assert via screen.getByText for the expected error
strings.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: afc8a41a-85cc-466c-866d-108316422181
📒 Files selected for processing (27)
drizzle/0011_spicy_the_phantom.sqldrizzle/0012_pink_tattoo.sqldrizzle/meta/0011_snapshot.jsondrizzle/meta/0012_snapshot.jsondrizzle/meta/_journal.jsonserver/app.test.tsserver/app.tsserver/db/schema.tsserver/deploy.tsserver/hermes/persona.test.tsserver/hermes/persona.tsserver/hermes/telegram-deploy-context.test.tsserver/hermes/telegram-deploy-context.tsserver/settings.test.tsserver/settings.tsserver/settings/config.test.tsserver/settings/config.tsserver/settings/records.tssrc/features/providers/provider-settings-aside.tsxsrc/features/providers/provider-settings.tsxsrc/features/settings/persona-settings-aside.tsxsrc/features/settings/persona-settings.test.tsxsrc/features/settings/persona-settings.tsxsrc/features/settings/settings-page.tsxsrc/lib/load-telegram-deploy.tssrc/routes/ai-provider.tsxsrc/routes/settings.tsx
Replace the Settings stub with a persona editor that persists content in hermes_settings and deploys it to the Telegram-deployed Hermes server via SSH SOUL.md writes and gateway restarts.
Reject null JSON save payloads before property access and require a deployed Telegram server ID before resolving SSH credentials.
Extract shared Telegram deploy context resolution and route loader, drop duplicate hermes_settings deploy metadata, unify persona contracts, and replace the reducer with a simpler aside-based settings layout.
fetch() rejections bypassed the try/finally handlers, leaving users with no feedback when the request never completed. Catch network failures and show a friendly error while still clearing loading state in finally.
Add Settings endpoints to the API reference, feature bullets and route map in README, and Agent Persona glossary entry in CONTEXT.md for the persona editor introduced in issue #12. Co-Authored-By: Warp <agent@warp.dev>
- Document optional serverId on persona and MCP deploy endpoints - Replace outdated Telegram-linked deploy language with install-based targets - Add MCP presets, mcp_servers schema row, and Hermes Deploy Target glossary term - Fix duplicate audit_logs row in API reference Triggered by issue-22 features: persona editor (#16), MCP manager (#21), VPS setup check (#19), and deploy target selector. Co-authored-by: Warp <agent@warp.dev>
1 participant
What
Add a Settings-page editor for the user's durable Hermes persona, backed by
SOUL.md.hermes_settingstable and Drizzle migration (0011_spicy_the_phantom.sql)POST /api/settings/personaandPOST /api/settings/persona/deployhandlers/root/.hermes/SOUL.mdCloses #12
Why
Hermes uses
SOUL.mdas the primary agent identity and slot #1 system prompt source. Users need a durable way to define their agent persona in HermesHub and push it to their already-deployed Telegram Hermes server without manual SSH edits.How
hermes_settingsrow per user (upsert onuser_id) storesagent_personaand deploy metadata.getTelegramDeployInfo), writesSOUL.mdover SSH via base64 transfer to avoid shell injection, restarts the Hermes gateway, and records deploy metadata in a transaction.Checklist
Summary by CodeRabbit