Skip to content

feat(hermes): add Hermes Agent provider support - #421

Merged
tuo-lei merged 3 commits into
mainfrom
feat/hermes-provider
Aug 5, 2026
Merged

tuo-lei merged 3 commits into
mainfrom
feat/hermes-provider

Conversation

@tuo-lei

@tuo-lei tuo-lei commented Aug 5, 2026 •

Copy link
Copy Markdown
Owner

Summary

Adds a Hermes Agent provider so vibe-replay can discover, parse, and replay sessions from Hermes' local store (~/.hermes/state.db, SQLite via sql.js — no native deps). Mirrors the opencode provider (#420) structure for feature parity.

What's included

New package packages/provider-hermes/

  • discover.ts — session discovery from the sessions table, per-session prompt/tool/edit counts, per-model token usage + cost from session_model_usage, version read from ~/.hermes/.update_check, #session:<id> marker paths
  • parser.ts — OpenAI-style tool_calls pairing with role='tool' result rows, reasoning → thinking blocks, compaction handling, truncated-response counting, skillsUsed, malformed-JSON parse warnings
  • tool-mapping.ts — Hermes tool names → viewer canonical names (Bash/Edit/Write/Read/Grep/WebSearch/Agent/TodoWrite/…)
  • sqlite.ts / config.ts — sql.js WASM loading (positional ? binds, per CLAUDE.md gotcha), platform-aware ~/.hermes resolution
  • 12 unit tests (parser + discovery) with an in-memory sql.js fixture builder

Wiring (mirrors #420)

  • providers-default registry + priority + order test
  • CLI: lightweight scan path (buildLightweightHermesScanResult), badge color (#F85149), provider-contract test
  • Dashboard: badge/label/bar colors + red family, system-tool check entry
  • Feedback: hermes chat -q <prompt> -Q headless runner (quiet mode = final response only)
  • Docs: CLAUDE.md provider section

Context compaction

Hermes records compaction two ways: compacted=1 rows (full pre-compaction history, kept in the replay like claude-code) and a user row prefixed [CONTEXT COMPACTION — the parser maps the latter to subtype: "compaction-summary" so the viewer renders a dedicated scene, same as claude-code's isCompactSummary.

Verification (real data)

  • Discovered 65 local sessions across cli/telegram/cron/subagent sources
  • Full replay of a real session: 353 scenes, 64 diffs, 71 bash outputs, 94 thinking blocks, self-contained HTML (zero external requests), no secrets flagged
  • Compaction scenes render for sessions that carry the summary marker
  • pnpm lint:check / pnpm test (209) / pnpm typecheck all green

Known limitations (follow-ups)

  • Subagent runs (Hermes delegate_task → parent_session_id) are not yet linked into an Agent scene — same level as opencode today
  • _user_images blocks are parsed but images are not decoded into scenes (only 5 rows in the sample DB)
  • Old Hermes versions (< ~0.19) that don't write the [CONTEXT COMPACTION marker won't produce compaction scenes (compaction count still tracked in meta)

Summary by CodeRabbit

  • New Features

    • Added support for discovering and viewing Hermes sessions, including metadata, usage, tools, thinking, and conversation details.
    • Added Hermes feedback CLI integration with headless execution and system readiness checks.
    • Added Hermes provider badges, labels, colors, and dashboard visibility.
    • Added support for Hermes session storage locations and configurable data directories.
  • Bug Fixes

    • Improved handling of incomplete, malformed, compacted, and truncated session data.
  • Documentation

    • Documented Hermes storage, message formats, discovery behavior, and tool mappings.

Discover, parse, and replay Hermes Agent sessions from ~/.hermes/state.db
(SQLite via sql.js), matching the opencode provider's feature set:

- discovery: sessions/messages/token stats from SQLite, version from
  ~/.hermes/.update_check, #session:<id> marker paths
- parser: OpenAI-style tool_calls pairing, reasoning (thinking) blocks,
  compaction-summary scenes for [CONTEXT COMPACTION rows, truncated
  responses, skillsUsed, per-model token usage + cost
- tool mapping: hermes tool names onto the viewer vocabulary
  (Bash/Edit/Write/Read/Grep/WebSearch/Agent/TodoWrite/...)
- wiring: providers-default registry, CLI lightweight scan path + badge
  color, dashboard colors, feedback integration via `hermes chat -q -Q`,
  server tool-check, docs

Verified end-to-end against real local sessions (353-scene replay,
64 diffs, 71 bash outputs, compaction scenes) and 12 unit tests.
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

@claude

claude Bot commented Aug 5, 2026 •

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026 •

Copy link
Copy Markdown

Review Change Stack

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: 44a5448a-2750-42a6-ac65-4cf27f010cd5

📥 Commits

Reviewing files that changed from the base of the PR and between bd5ef6f and c4834a2.

📒 Files selected for processing (1)
  • packages/provider-hermes/src/hermes/sqlite.ts
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/provider-hermes/src/hermes/sqlite.ts

📝 Walkthrough

Walkthrough

Added a SQLite-backed Hermes provider with session discovery, message parsing, tool mapping, token usage, compaction handling, CLI integration, provider registration, system checks, scanning, viewer styling, tests, and documentation.

Changes

Hermes provider

Layer / File(s) Summary
SQLite storage and test fixtures
packages/provider-hermes/package.json, packages/provider-hermes/src/hermes/sqlite.ts, packages/provider-hermes/src/sql-js.d.ts, packages/provider-hermes/test/helpers/db.ts
Added the Hermes package, SQLite utilities, SQL.js declarations, and in-memory test database fixtures.
Hermes session discovery
packages/provider-hermes/src/hermes/discover.ts, packages/provider-hermes/src/hermes/config.ts, packages/provider-hermes/test/discover.test.ts
Added ordered session discovery, statistics, metadata mapping, version loading, timestamp normalization, marker paths, and pinned-session handling.
Hermes message parsing
packages/provider-hermes/src/hermes/parser.ts, packages/provider-hermes/src/hermes/tool-mapping.ts, packages/provider-hermes/src/hermes/index.ts, packages/provider-hermes/test/parser.test.ts
Added parsing for turns, thinking, text, tools, results, compaction, truncation, skills, Git metadata, token usage, and malformed JSON warnings.
Provider registry and CLI integration
packages/providers-default/*, packages/cli/src/feedback.ts, packages/cli/src/scanner.ts, packages/cli/src/server.ts, packages/cli/package.json, package.json
Registered Hermes and added feedback execution, system checks, lightweight scanning, workspace dependencies, and type checking.
Viewer presentation and documentation
packages/viewer/src/components/*, CLAUDE.md
Added Hermes system checks, display labels, red provider styling, tests, and provider documentation.

Estimated code review effort: 4 (Complex) | ~45 minutes

Sequence Diagram(s)

sequenceDiagram
  participant CLI
  participant HermesProvider
  participant HermesSQLite
  participant Viewer
  CLI->>HermesProvider: discover or parse Hermes session
  HermesProvider->>HermesSQLite: open and query database
  HermesSQLite-->>HermesProvider: session metadata and messages
  HermesProvider-->>CLI: scan or parsed session result
  Viewer->>CLI: request Hermes system check
  CLI-->>Viewer: Hermes availability and provider status
Loading

Possibly related PRs

  • tuo-lei/vibe-replay#420: Adds analogous SQLite-backed provider support across discovery, parsing, CLI, registry, and viewer integration.
🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 36.11% 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 title clearly and concisely describes the primary change: adding Hermes Agent provider support.
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.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/hermes-provider

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 9

🧹 Nitpick comments (4)
packages/provider-hermes/src/hermes/parser.ts (2)

111-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Reuse isHermesSessionId instead of repeating the pattern.

packages/provider-hermes/src/hermes/sqlite.ts line 59 exports isHermesSessionId with the same rule, and it currently has no caller. Import it here so one definition covers both sites.

♻️ Proposed refactor
-import { openHermesDb, hermesDataDir, hermesDbPath } from "./sqlite.js";
+import { isHermesSessionId, openHermesDb, hermesDataDir, hermesDbPath } from "./sqlite.js";
-    if (/^\d{8}_\d{6}_/.test(path) || path.startsWith("session_")) return path;
+    if (isHermesSessionId(path)) return path;
🤖 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 `@packages/provider-hermes/src/hermes/parser.ts` around lines 111 - 126, Update
resolveSessionId to import and reuse isHermesSessionId from sqlite.ts for the
raw path validation, removing the duplicated timestamp/session_ pattern check
while preserving the existing marker-based extraction and return behavior.

65-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Extract the duplicated sql.js row and timestamp helpers. rowValues and toIsoMs exist twice with identical bodies. Two copies of the timestamp normalization also make the durationMsEst unit fix easy to apply in only one place.

  • packages/provider-hermes/src/hermes/parser.ts#L65-L80: import rowValues, firstValue, and toIsoMs from a new internal module such as src/hermes/db-utils.ts and delete the local copies at lines 65-80 and 368-373.
  • packages/provider-hermes/src/hermes/discover.ts#L37-L47: import the same helpers and delete the local copies at lines 37-47 and 242-247.
🤖 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 `@packages/provider-hermes/src/hermes/parser.ts` around lines 65 - 80, Extract
the duplicated sql.js helpers into a shared internal db-utils module. In
packages/provider-hermes/src/hermes/parser.ts at lines 65-80 and 368-373, import
rowValues, firstValue, and toIsoMs from that module and remove the local
implementations; apply the same import and removal in
packages/provider-hermes/src/hermes/discover.ts at lines 37-47 and 242-247.
Ensure both files use the shared helpers, including a single toIsoMs
implementation for consistent timestamp normalization and durationMsEst
handling.
packages/provider-hermes/src/hermes/sqlite.ts (1)

58-61: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

isHermesSessionId has no caller, and parser.ts repeats its logic inline.

packages/provider-hermes/src/hermes/parser.ts line 123 repeats /^\d{8}_\d{6}_/ and the session_ prefix test. Two copies of the same identifier rule will drift. The fix belongs at the parser call site; see the comment there.

🤖 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 `@packages/provider-hermes/src/hermes/sqlite.ts` around lines 58 - 61, Update
the parser logic in parser.ts to call the existing isHermesSessionId helper
instead of duplicating the timestamp-pattern and session_ prefix checks inline.
Remove the redundant inline validation while preserving the parser’s current
behavior.
packages/provider-hermes/src/hermes/tool-mapping.ts (1)

9-29: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Hoist the mapping table and simplify the null branch.

mapping is rebuilt on every mapHermesToolName call. Large sessions call it once per tool call. At line 43, obj is null in that branch, so obj || {} always yields {}.

♻️ Proposed refactor
+const TOOL_NAME_MAP: Record<string, string> = {
+  terminal: "Bash",
+  read_file: "Read",
+  write_file: "Write",
+  patch: "Edit",
+  search_files: "Grep",
+  web_search: "WebSearch",
+  web_extract: "WebFetch",
+  delegate_task: "Agent",
+  clarify: "AskQuestion",
+  todo: "TodoWrite",
+  skill_view: "Skill",
+  skill_manage: "Skill",
+  skills_list: "Skill",
+  vision_analyze: "Vision",
+  computer_use: "ComputerUse",
+  execute_code: "ExecuteCode",
+  session_search: "SessionSearch",
+  text_to_speech: "TextToSpeech",
+};
+
 export function mapHermesToolName(name: string): string {
-  const mapping: Record<string, string> = {
-    terminal: "Bash",
-    ...
-  };
-  return mapping[name] || name;
+  return TOOL_NAME_MAP[name] || name;
 }
-  if (!obj) return obj || {};
+  if (!obj) return {};

Also applies to: 43-43

🤖 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 `@packages/provider-hermes/src/hermes/tool-mapping.ts` around lines 9 - 29,
Move the constant mapping table out of mapHermesToolName so it is initialized
once and reused across calls, preserving all existing mappings and fallback
behavior. In the null-object branch, replace the redundant obj || {} expression
with the direct empty-object value because obj is guaranteed null there.
🤖 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 `@CLAUDE.md`:
- Around line 58-59: Update the supported-provider summary in CLAUDE.md to
include Hermes alongside Claude Code, Cursor, and Codex. Keep the existing
provider list and surrounding documentation unchanged.

In `@packages/cli/src/feedback.ts`:
- Around line 447-450: Update the spawnTool invocation in the feedback flow so
prompt is no longer passed as the "chat" command-line argument. Send the prompt
through the spawned process's stdin or another supported non-command-line input
while preserving the existing command behavior and stdio handling.
- Around line 447-450: Update the process launch in runHermes to avoid passing
the replay prompt through a Windows shell: either write the prompt to the
spawned process’s stdin and remove it from the argument list, or invoke a
resolved executable with shell disabled. Preserve the existing chat options and
timeout while preventing cmd.exe from interpreting prompt metacharacters.

In `@packages/cli/src/scanner.ts`:
- Around line 847-854: Update the background-scan mapping in server.ts to
populate discoveryTokenUsage and discoveryCostEstimate from the discovery
session before buildLightweightHermesScanResult consumes them; if the session
contract lacks these fields, extend it accordingly. Add coverage for propagation
through the discovery-to-scan path.

In `@packages/cli/src/server.ts`:
- Around line 2418-2419: Update the no-tool error messages in the feedback
routes around the affected handlers to include hermes alongside claude, agent,
and opencode. Ensure all three messages list every supported feedback tool
consistently with the hermes entry in the tool detection configuration.

In `@packages/provider-hermes/src/hermes/discover.ts`:
- Around line 191-217: Update the duration calculation in the session mapping
return object to normalize both started_at and lastActivity through the same
timestamp-unit helper used by toIsoMs before subtracting them. Preserve the
existing positive-duration guard and return durationMsEst in milliseconds
without unconditionally multiplying a millisecond delta by 1000.

In `@packages/provider-hermes/src/hermes/parser.ts`:
- Around line 217-224: Move the `message.finish_reason === "max_tokens"`
increment before the `blocks.length === 0` early continue in the assistant-row
parsing flow, so empty truncated responses are counted while preserving the
existing skip behavior.
- Around line 137-147: Update the messages query in the Hermes history loader to
filter out rewound messages by adding the active-state predicate alongside the
existing session_id condition. Preserve the current ordering and selected
columns so only active Hermes messages are replayed.

In `@packages/provider-hermes/src/hermes/sqlite.ts`:
- Around line 40-56: Update openHermesDb to probe the opened database schema
before returning its handle, verifying the expected sessions table/layout and
returning null when the schema is missing or incompatible. Keep existing read,
initialization, and size checks, and ensure any schema-probe failure is handled
within openHermesDb so discoverHermesSessions cannot fail on a foreign state.db.

---

Nitpick comments:
In `@packages/provider-hermes/src/hermes/parser.ts`:
- Around line 111-126: Update resolveSessionId to import and reuse
isHermesSessionId from sqlite.ts for the raw path validation, removing the
duplicated timestamp/session_ pattern check while preserving the existing
marker-based extraction and return behavior.
- Around line 65-80: Extract the duplicated sql.js helpers into a shared
internal db-utils module. In packages/provider-hermes/src/hermes/parser.ts at
lines 65-80 and 368-373, import rowValues, firstValue, and toIsoMs from that
module and remove the local implementations; apply the same import and removal
in packages/provider-hermes/src/hermes/discover.ts at lines 37-47 and 242-247.
Ensure both files use the shared helpers, including a single toIsoMs
implementation for consistent timestamp normalization and durationMsEst
handling.

In `@packages/provider-hermes/src/hermes/sqlite.ts`:
- Around line 58-61: Update the parser logic in parser.ts to call the existing
isHermesSessionId helper instead of duplicating the timestamp-pattern and
session_ prefix checks inline. Remove the redundant inline validation while
preserving the parser’s current behavior.

In `@packages/provider-hermes/src/hermes/tool-mapping.ts`:
- Around line 9-29: Move the constant mapping table out of mapHermesToolName so
it is initialized once and reused across calls, preserving all existing mappings
and fallback behavior. In the null-object branch, replace the redundant obj ||
{} expression with the direct empty-object value because obj is guaranteed null
there.
🪄 Autofix

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: c6ecd4ad-de5d-457e-bdc2-a041fa2a5a79

📥 Commits

Reviewing files that changed from the base of the PR and between c67e66c and a8d90a8.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (26)
  • CLAUDE.md
  • package.json
  • packages/cli/package.json
  • packages/cli/src/feedback.ts
  • packages/cli/src/index.ts
  • packages/cli/src/scanner.ts
  • packages/cli/src/server.ts
  • packages/cli/test/provider-contract-package.test.ts
  • packages/provider-hermes/package.json
  • packages/provider-hermes/src/hermes/config.ts
  • packages/provider-hermes/src/hermes/discover.ts
  • packages/provider-hermes/src/hermes/index.ts
  • packages/provider-hermes/src/hermes/parser.ts
  • packages/provider-hermes/src/hermes/sqlite.ts
  • packages/provider-hermes/src/hermes/tool-mapping.ts
  • packages/provider-hermes/src/sql-js.d.ts
  • packages/provider-hermes/test/discover.test.ts
  • packages/provider-hermes/test/helpers/db.ts
  • packages/provider-hermes/test/parser.test.ts
  • packages/provider-hermes/tsconfig.json
  • packages/providers-default/package.json
  • packages/providers-default/src/index.test.ts
  • packages/providers-default/src/index.ts
  • packages/viewer/src/components/DashboardHome.tsx
  • packages/viewer/src/components/__tests__/dashboard-utils.test.ts
  • packages/viewer/src/components/dashboard-utils.ts

Comment thread CLAUDE.md
Comment on lines +447 to +450
const proc = spawnTool(cmd, ["chat", "-q", prompt, "-Q", "--no-restore-cwd"], {
env: { ...process.env, NO_COLOR: "1", TERM: "dumb" },
timeout: 600_000,
stdio: ["pipe", "pipe", "pipe"],

@coderabbitai coderabbitai Bot Aug 5, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- feedback.ts structure and relevant symbols ---'
ast-grep outline packages/cli/src/feedback.ts
printf '%s\n' '--- feedback.ts lines 1-180 ---'
sed -n '1,180p' packages/cli/src/feedback.ts
printf '%s\n' '--- feedback.ts lines 380-490 ---'
sed -n '380,490p' packages/cli/src/feedback.ts
printf '%s\n' '--- feedback.ts references to spawnTool, prompt, and stdin writes ---'
rg -n -C 4 'spawnTool|prompt|stdin|write\(' packages/cli/src/feedback.ts

Repository: tuo-lei/vibe-replay

Length of output: 30524


🏁 Script executed:

#!/bin/bash
set -e
node - <<'JS'
const { spawn } = require("node:child_process");
const secret = "source code credential PII marker";
const child = spawn(process.execPath, ["-e", "setTimeout(() => {}, 2000)", secret], {
  stdio: "ignore",
});
const cmdline = require("node:fs").readFileSync(`/proc/${child.pid}/cmdline`, "utf8").replace(/\0/g, " ");
console.log(JSON.stringify({ secretVisibleInProcCmdline: cmdline.includes(secret), cmdline }));
child.kill();
JS

Repository: tuo-lei/vibe-replay

Length of output: 290


Sensitive Data Exposure (CWE-200): Exposure of Sensitive Information to an Unauthorized Actor

Reachability: External · Exploitability: Moderate

Reachability path
● Entry
  packages/cli/src/index.ts:672
  startServer
│
▼
● Hop
  packages/cli/src/server.ts:586
  startServer: After replays change, sync the sources cache so existingReplay / replay stay consistent
│
▼
● Sink
  packages/cli/src/feedback.ts

Keep the Hermes prompt out of the process command line.

Pass prompt through stdin or another non-command-line input. It can contain source code, credentials, or PII.

🤖 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 `@packages/cli/src/feedback.ts` around lines 447 - 450, Update the spawnTool
invocation in the feedback flow so prompt is no longer passed as the "chat"
command-line argument. Send the prompt through the spawned process's stdin or
another supported non-command-line input while preserving the existing command
behavior and stdio handling.

🔒 Security & Privacy | 🔴 Critical | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- feedback.ts outline ---'
ast-grep outline packages/cli/src/feedback.ts
printf '%s\n' '--- relevant implementation ---'
sed -n '1,140p' packages/cli/src/feedback.ts
sed -n '400,490p' packages/cli/src/feedback.ts
printf '%s\n' '--- runHermes and spawnTool references ---'
rg -n -C 5 'spawnTool|runHermes|prompt' packages/cli/src/feedback.ts
printf '%s\n' '--- feedback entry points ---'
rg -n -C 4 'runHermes|generateFeedback|feedback|replay' packages/cli/src --glob '*.ts' --glob '*.tsx' | head -n 240

Repository: tuo-lei/vibe-replay

Length of output: 46350


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- generateFeedback implementation ---'
sed -n '920,970p' packages/cli/src/feedback.ts
printf '%s\n' '--- all generateFeedback call sites ---'
rg -n -C 8 'generateFeedback\(' packages/cli/src packages/cli/test packages/cli/tests 2>/dev/null || true
printf '%s\n' '--- executeFeedback and tool command flow ---'
sed -n '1340,1405p' packages/cli/src/feedback.ts
printf '%s\n' '--- package scripts and test files mentioning feedback ---'
rg -n -C 3 'feedback|generateFeedback|detectFeedbackTools' packages/cli/package.json packages/cli --glob '*test*' --glob '*spec*' --glob '*.json' 2>/dev/null | head -n 240

Repository: tuo-lei/vibe-replay

Length of output: 22205


🏁 Script executed:

#!/bin/bash
set -eu
printf '%s\n' '--- feedback route and surrounding middleware ---'
sed -n '2640,2765p' packages/cli/src/server.ts
printf '%s\n' '--- route registration and auth-related middleware ---'
rg -n -C 5 'feedback|ai-coach|auth|middleware|bearer|basic|session' packages/cli/src/server.ts | head -n 320
printf '%s\n' '--- loadSessionFromDisk definition and callers ---'
rg -n -C 8 'function loadSessionFromDisk|loadSessionFromDisk\(' packages/cli/src --glob '*.ts'

Repository: tuo-lei/vibe-replay

Length of output: 29491


🌐 Web query:

Node.js child_process spawn Windows shell true args cmd.exe metacharacters argument quoting official documentation

💡 Result:

When using child_process.spawn with the shell: true option on Windows, Node.js executes the command inside a shell, typically cmd.exe (as specified by process.env.ComSpec) [1][2]. Security Implications and Metacharacters Enabling shell: true significantly changes how arguments are handled and introduces severe security risks [3][4]. 1. String Concatenation: When shell: true is used, the provided command and arguments are concatenated into a single string and passed to the shell [5][6]. The shell then interprets this string [4]. 2. Command Injection: Because the shell interprets the resulting string, any input containing shell metacharacters—such as &, &&, |,;, <, >, or ^—can be used to trigger arbitrary command execution [5][4]. If user-controlled input is included in the command or arguments without rigorous sanitization, an attacker can escape the intended command and execute malicious code [3][4]. 3. False Sense of Security: The child_process.spawn interface, which accepts an array for arguments, suggests that arguments are passed safely as distinct parameters [3][4]. When shell: true is enabled, this safety mechanism is bypassed, as the array elements are simply joined with spaces before being sent to the shell [5][4]. Official Guidance and Deprecation Node.js documentation explicitly warns against passing unsanitized user input when shell: true is enabled [3]. Furthermore, passing an array of arguments (args) while using the shell option is deprecated (DEP0190) because it encourages the incorrect assumption that arguments will be handled safely [3][6]. The recommended practice is to pass a single command string when using shell: true, or to avoid the shell option entirely [6]. Quoting and Escaping When using shell: true, Node.js does not perform automatic quoting or escaping of arguments to protect against shell metacharacters [7][2]. If the executable path or arguments contain spaces, they must be quoted manually by the developer [1][2]. Given the inherent risks of command injection and the complexity of correctly quoting arguments for cmd.exe, using shell: true is generally discouraged [1][3]. For executing.bat or.cmd files on Windows without the risks associated with spawning a shell, developers are encouraged to seek safer alternatives, such as executing the script directly if possible or using established utilities that handle safe spawning [6].

Citations:


Injection (CWE-78): Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection')

Reachability: External

Reachability path
● Entry
  packages/cli/src/index.ts:672
  startServer
│
▼
● Hop
  packages/cli/src/server.ts:586
  startServer: After replays change, sync the sources cache so existingReplay / replay stay consistent
│
▼
● Sink
  packages/cli/src/feedback.ts

Do not pass the replay prompt through a Windows shell. runHermes passes replay content as an argument while spawnTool enables shell: true; cmd.exe can interpret shell metacharacters. Pass the prompt through stdin or invoke a resolved executable without a shell.

🤖 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 `@packages/cli/src/feedback.ts` around lines 447 - 450, Update the process
launch in runHermes to avoid passing the replay prompt through a Windows shell:
either write the prompt to the spawned process’s stdin and remove it from the
argument list, or invoke a resolved executable with shell disabled. Preserve the
existing chat options and timeout while preventing cmd.exe from interpreting
prompt metacharacters.

Source: Linters/SAST tools

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Good catch on argv exposure — but Hermes only accepts a non-interactive query via -q argv: the CLI reads stdin only for interactive TUI commands (guarded by _require_tty in hermes_cli/main.py), and agent/ has no stdin query path. So there is no stdin channel to move the prompt to. Mitigations already in place: the feedback digest is truncated (max ~25KB) and this is the same trust model as the other tools (claude/agent/opencode also stream session digests to a subprocess). Left as-is with a comment; happy to revisit if Hermes adds stdin query support.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧩 Analysis chain

🤖 web_search executed:

Hermes CLI hermes chat -qstdin non-interactive input_require_tty hermes_cli main.py

Length of output: 14242


🤖 web_search executed:

site:github.com/NousResearch/hermes-agent "def cmd_chat" "_require_tty" "-q" "sys.stdin"

Length of output: 15114


@tuo-lei, thank you for the Hermes CLI detail. I confirmed that hermes chat -q <prompt> is the documented headless execution path. I found no supported stdin query option. The stdin-specific remediation does not apply, so I will not require it.

The Windows injection concern remains. runHermes passes the digest in an argument, and spawnTool uses shell: true on Windows. cmd.exe can interpret metacharacters in that digest. The size limit does not prevent this behavior.

Please use a shell-free Windows launch path for Hermes, or a supported Hermes invocation that isolates the prompt from cmd.exe. The argv exposure also remains a Hermes CLI limitation.

You are interacting with an AI system.

Comment on lines +847 to +854
promptCount: input.discoveryPromptCount ?? (firstPrompt ? 1 : 0),
toolCallCount: input.discoveryToolCallCount ?? 0,
editCount: input.discoveryEditCount ?? 0,
filesModified: [],
model: input.discoveryModel,
durationMs: input.discoveryDurationMs,
tokenUsage: input.discoveryTokenUsage,
costEstimate: input.discoveryCostEstimate,

@coderabbitai coderabbitai Bot Aug 5, 2026 •

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Propagate Hermes token and cost metadata into ScanInput.

buildLightweightHermesScanResult reads input.discoveryTokenUsage and input.discoveryCostEstimate. The background-scan mapping in packages/cli/src/server.ts Lines 1121-1143 sets neither field. The Hermes scan path therefore drops token and cost data before it reaches insights. Add the producer assignments, or extend the shared session contract if discovery does not expose these fields, and cover the full discovery-to-scan path with a test.

🤖 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 `@packages/cli/src/scanner.ts` around lines 847 - 854, Update the
background-scan mapping in server.ts to populate discoveryTokenUsage and
discoveryCostEstimate from the discovery session before
buildLightweightHermesScanResult consumes them; if the session contract lacks
these fields, extend it accordingly. Add coverage for propagation through the
discovery-to-scan path.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Agreed it would be nice, but this is a pre-existing, provider-wide gap: SessionInfo (provider-contract) has no tokenUsage/costEstimate fields, and the opencode lightweight scan (#420) has the identical blind spot — the producer assignments would have to be added to the shared contract for every provider. The rich token/cost data does reach insights via the full parse path (parseSessionFromDb reads session_model_usage). Tracking as a follow-up; kept consistent with opencode for now.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@tuo-lei, understood. This is a valid provider-contract gap and is not specific to the Hermes implementation.

Keeping the Hermes lightweight scan consistent with OpenCode is reasonable for this PR. The full parse path preserves Hermes token and cost data through session_model_usage.

Would you like me to create a follow-up GitHub issue for adding token and cost metadata to SessionInfo and propagating it through lightweight scans?

You are interacting with an AI system.

Comment thread packages/cli/src/server.ts
Comment thread packages/provider-hermes/src/hermes/discover.ts
Comment thread packages/provider-hermes/src/hermes/parser.ts
Comment thread packages/provider-hermes/src/hermes/parser.ts Outdated
Comment thread packages/provider-hermes/src/hermes/sqlite.ts
- discover: normalize seconds/milliseconds timestamps through one helper
  so durationMsEst can't be 1000x off if a build stores ms
- parser: count truncated responses even for rows with no renderable blocks
- sqlite: probe for sessions/messages tables before returning the handle so a
  foreign or stale state.db degrades to 'no sessions' instead of throwing
- server: include hermes in the no-tool error message (3 routes)
- CLAUDE.md: list Hermes in the supported-provider intro

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 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 `@packages/provider-hermes/src/hermes/sqlite.ts`:
- Around line 52-60: Update the database-loading function containing the probe
query so the SQL.js handle is declared outside the try block and assigned after
new SQL.Database(buffer) succeeds. In the catch path, close the handle when it
exists before returning null, while preserving the existing probe validation
cleanup and successful return behavior.
🪄 Autofix

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: 3fec5678-e6f0-4601-a5b1-d5e9c792b2ca

📥 Commits

Reviewing files that changed from the base of the PR and between a8d90a8 and bd5ef6f.

📒 Files selected for processing (5)
  • CLAUDE.md
  • packages/cli/src/server.ts
  • packages/provider-hermes/src/hermes/discover.ts
  • packages/provider-hermes/src/hermes/parser.ts
  • packages/provider-hermes/src/hermes/sqlite.ts
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/provider-hermes/src/hermes/parser.ts
  • CLAUDE.md
  • packages/provider-hermes/src/hermes/discover.ts

Comment thread packages/provider-hermes/src/hermes/sqlite.ts
Hoist the Database handle out of the try block so the catch path can close
it, preventing WASM memory leaks during repeated discovery scans when a
foreign/corrupt state.db makes the probe throw.
@tuo-lei
tuo-lei merged commit c995fb4 into main Aug 5, 2026
7 checks passed
@tuo-lei
tuo-lei deleted the feat/hermes-provider branch August 5, 2026 04:48
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