Repository navigation
Add Hermes Agent hook support - #3585
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Hermes Agent end-to-end: asset/localization, CLI hook format and install/uninstall, sanitized launch policy and environment, SQLite-backed Hermes index and transcript loading, session indexing/resume integration, UI/feed updates, and comprehensive tests. ChangesHermes Agent Integration
sequenceDiagram
participant CLI as CLI/cmux
participant AgentLaunch as CMUXAgentLaunch
participant Vault as CMUXAgentVault (SQLite)
participant IndexStore as SessionIndexStore
participant UI as SessionIndexView / Feed
CLI->>AgentLaunch: install/uninstall hermes-agent hooks (generate events, write config, update allowlist)
AgentLaunch->>AgentLaunch: sanitize launch args (HERMES_HOME, hermesAgentPolicy)
UI->>IndexStore: request session search/load
IndexStore->>Vault: HermesAgentIndex.loadSessions / loadTranscript (snapshot DB)
Vault-->>IndexStore: sessions/transcript turns
IndexStore-->>UI: mapped SessionEntry / SessionTranscriptTurn
CLI->>UI: hook decision outcomes routed to hermesAgentBlock (permission/plan flows)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (9 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Greptile SummaryThis PR integrates Hermes Agent as a restorable agent in cmux, adding
Confidence Score: 4/5Safe to merge with one fix: the noFolderScope post-filter in SessionIndexStore silently discards all non-Hermes agent sessions from every global search until removed. The noFolderScope post-filter introduced alongside Hermes indexing incorrectly restricts global search results to sessions with an empty cwd, making the session index show only Hermes sessions whenever no folder is selected. All other agent history disappears silently. This is a straightforward one-line fix (remove the filter block), and everything else in the PR — hook install/uninstall, YAML block management, allowlist passthrough, transcript loading, sanitizer policy, and resume command building — looks correct. Sources/SessionIndexStore.swift — the noFolderScope filter at lines 1287–1289 needs to be removed. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as SessionIndexView
participant Store as SessionIndexStore
participant Merge as mergedAgentEntries
participant HI as HermesAgentIndex
participant DB as state.db (snapshot)
UI->>Store: search(needle, scope: .directory(nil))
Store->>Store: "noFolderScope = true, cwdFilter = nil"
Store->>Merge: mergedAgentEntries(cwdFilter: nil)
Merge->>HI: loadSessions(cwdFilter: nil)
HI->>DB: snapshot + SQL query
DB-->>HI: Hermes sessions (cwd: nil)
HI-->>Merge: HermesAgentIndexResult
Merge->>Merge: also loads Claude/Codex/OpenCode/RovoDev (cwd non-nil)
Merge-->>Store: all sessions
Store->>Store: filter cwd.isEmpty — drops all non-Hermes
Store-->>UI: Hermes-only results
Reviews (5): Last reviewed commit: "Keep Hermes hook installer split out" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "backup", | ||
| "config", | ||
| "cron", | ||
| "curator", | ||
| "debug", | ||
| "doctor", | ||
| "gateway", | ||
| "help", | ||
| "hooks", | ||
| "import", | ||
| "kanban", | ||
| "logs", | ||
| "mcp", | ||
| "model", | ||
| "plugins", | ||
| "setup", | ||
| "sessions", | ||
| "skills", | ||
| "status", | ||
| "tools", | ||
| "uninstall", | ||
| "update", | ||
| "version" | ||
| ] | ||
|
|
||
| static let hermesAgentPolicy = Policy( | ||
| valueOptions: [ | ||
| "--api-key", | ||
| "--base-url", | ||
| "--image", | ||
| "--max-turns", | ||
| "--model", | ||
| "-m", | ||
| "--profile", | ||
| "-p", | ||
| "--provider", | ||
| "--resume", | ||
| "-r", | ||
| "--skills", | ||
| "-s", | ||
| "--source", | ||
| "--toolsets", | ||
| "-t" | ||
| ], | ||
| optionalValueOptions: [ | ||
| "--continue", | ||
| "-c" | ||
| ], | ||
| variadicOptions: [ | ||
| "--skills", | ||
| "-s" | ||
| ], | ||
| nonRestorableCommands: hermesAgentNonRestorableCommands, | ||
| droppedOptions: [ | ||
| "--api-key", | ||
| "--continue", | ||
| "-c", | ||
| "--image", | ||
| "--resume", | ||
| "-r", | ||
| "--source", | ||
| "--verbose", | ||
| "-v", | ||
| "--worktree", | ||
| "-w" | ||
| ], | ||
| droppedOptionPrefixes: [ | ||
| "--api-key=", | ||
| "--continue=", |
There was a problem hiding this comment.
--tui flag absent from hermesAgentPolicy
--tui is not declared in valueOptions, droppedOptions, or anywhere else in hermesAgentPolicy. SessionEntry.resumeCommand shows that --tui is the actual Hermes binary flag for TUI mode (it appends it directly based on source == "tui"), yet the sanitizer policy has no entry for it. If preserveOptions uses a whitelist approach—keeping only flags it recognises—then any session originally launched as hermes --tui would be resumed without --tui, silently switching the user to CLI mode on every subsequent restore via AgentResumeCommandBuilder.
There was a problem hiding this comment.
I verified this against the current sanitizer. is intentionally not in because it is a boolean flag, and boolean flags pass through by default unless explicitly rejected or dropped. The existing test covers ; I added a short comment on the Hermes policy to make that default explicit.
— Claude Code
There was a problem hiding this comment.
Clarifying the previous reply: --tui is intentionally not in valueOptions because it is a boolean flag. Boolean flags pass through by default unless explicitly rejected or dropped. The existing preservesHermesInheritedFlagsWithoutReplayingStartupOnlyInput test covers --tui, and 04bace9 adds a policy comment for that default.
— Claude Code
| ) -> SessionTranscriptTurn? { | ||
| switch agent { | ||
| case .claude: | ||
| return parseClaudeLine(object, id: id) |
There was a problem hiding this comment.
.hermesAgent branches in file-based parse helpers
loadSession returns early via loadHermesAgentSynchronously for every .hermesAgent entry (all of which have fileURL: nil), so the three additions of .hermesAgent to parseLineByAgent, shouldCoalesce, and firstAssistantRole are dead code — the file-based parse path is never reached for this agent. They won't cause a runtime failure today, but they silently imply that parseGenericLine / the generic coalesce / assistant-role logic is correct for Hermes, which may mislead future readers who add a fileURL to HermesAgentIndexedSession without realising these branches then activate with the wrong schema.
There was a problem hiding this comment.
Fixed in 04bace9. Hermes transcripts now stay on the Hermes SQLite loader path; the file-based parser helpers return nil/false for instead of trying to parse it as generic JSONL.
— Claude Code
There was a problem hiding this comment.
Clarifying the previous reply: fixed in 04bace9. Hermes transcripts now stay on the Hermes SQLite loader path. The file-based parser helpers return nil or false for .hermesAgent instead of parsing it as generic JSONL.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 `@CLI/cmux.swift`:
- Around line 15741-15749: The Hermes feed hook set is missing the
"post_approval_response" event in the feedHookEvents array, preventing
approval-response notifications from being delivered; update the feedHookEvents
constant (the array shown with "pre_tool_call", "post_tool_call",
"pre_approval_request") to include "post_approval_response" (matching the Hermes
handler name) so the existing post_approval_response case in the Hermes hook
implementation will be registered and invoked.
- Around line 19124-19130: The classifier switch that returns ("SessionStart",
false) for "on_session_start" and handles "on_session_end"/"on_session_finalize"
but defaults to ("PreToolUse", false) is missing an explicit case for
"on_session_reset"; add a case "on_session_reset" that returns ("SessionStart",
false) so reset hooks (registered with cmuxSubcommand "session-start") are
classified and routed the same as on_session_start. Locate the switch/case block
that currently handles "on_session_start"/"on_session_end"/"on_session_finalize"
and insert the "on_session_reset" branch to match behavior.
- Around line 19426-19435: The current branch for source == "hermes-agent"
builds and returns a block payload via hermesAgentBlock(body), but the
pre_llm_call hook expects either plain text or a JSON context object (e.g.,
{"context":"..."}); update the code in the source == "hermes-agent" path to
return the user's answer as plain text or as a {"context": body} payload instead
of calling hermesAgentBlock; replace the hermesAgentBlock(...) return with the
appropriate context return so pre_llm_call receives the answer for injection
into the next user message.
In `@CLI/CMUXCLI`+DocsSettings.swift:
- Line 94: This file exceeds the repo's Swift file-length budget; either split
its non-core content (e.g., docs reference table or helper functions) out of
CMUXCLI+DocsSettings.swift into a new companion Swift file (create a new file,
move the helper/table declarations, preserve access levels and any
extensions/imports and update any references/imports) or update the repository’s
file-length budget configuration to include this new file (modify the
file-length budget tracking config to add CMUXCLI+DocsSettings.swift or raise
the threshold). Ensure after the change the original file compiles and all moved
symbols remain reachable.
In
`@Packages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerAdditionalPolicies.swift`:
- Around line 445-468: The dropped flags "--worktree" and "-w" are currently
listed in droppedOptions/droppedOptionPrefixes but not in the value-taking set,
causing split-form invocations ("--worktree /path" or "-w /path") to leave the
path as a positional arg; update the sanitizer policy in
AgentLaunchSanitizerAdditionalPolicies.swift by adding "--worktree" and "-w" to
the valueOptions collection (the same place where other flags like
"--source"/"-r" are marked as value-taking) so both "--worktree=..." and
"--worktree ..." forms are consumed and not left as positional arguments.
In
`@Packages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentHookConfig.swift`:
- Around line 34-45: The loop uses a captured map existingEvents of line indexes
but inserts into lines in-place causing later indexes to shift; fix by
collecting matched (event, eventIndex) pairs from existingEvents and processing
them in descending eventIndex order before calling lines.insert(contentsOf:at:),
referencing directEventLineIndexes(in:hooksIndex:), the events array, the lines
variable, and hookListBlock(events:itemIndent:); this ensures earlier-line edits
won't invalidate later indexes. Also add a unit test that installs hooks for two
events whose YAML keys already exist (e.g., PreToolUse and PostToolUse) to
verify the descending-order insertion behavior.
In
`@Packages/CMUXHermesAgentIndex/Sources/CMUXHermesAgentIndex/HermesAgentIndex.swift`:
- Line 168: The SQL WHERE clause currently hardcodes s.source IN ('cli', 'tui')
which will silently omit future source kinds; introduce a single-source-of-truth
by adding a private static let knownSources = ["cli", "tui"] on the
HermesAgentIndex type, replace the literal list in the query builder with a
join/interpolation of HermesAgentIndex.knownSources, and add a short comment
above knownSources explaining we filter to sources resumeCommand knows how to
launch; update any reference to the clause (e.g., in the method that builds the
session query) to use knownSources so new source kinds won’t be accidentally
missed.
- Around line 82-98: Add documentation to explain the silent early-return when
callers pass a non-nil cwdFilter: update the doc comment for the
loadSessions(needle:cwdFilter:offset:limit:stateDBPath:) method to state that
Hermes sessions do not contain cwd metadata and therefore any non-nil cwdFilter
will always return an empty HermesAgentIndexResult (no sessions, no errors).
Also add a brief doc comment on the HermesAgentIndexedSession type noting that
instances carry no cwd information and cannot be filtered by working directory.
Keep the wording short and explicit so future callers understand this contract
without inspecting the implementation.
In `@Sources/Feed/FeedPanelView.swift`:
- Line 1121: The file-length CI fails because FeedPanelView.swift exceeds the
allowed lines; extract a small, self-contained helper/view/type from
FeedPanelView to a new Swift file and import it from FeedPanelView to reduce
length. Identify a compact unit such as the color mapping for the agent case
(the switch including "case .hermesAgent") or a small SwiftUI subview used only
by FeedPanelView (e.g., a HermesAgentBadge or AgentColorProvider), cut that
type/function (preserve access level and any used dependencies) into the new
file, update FeedPanelView to reference the moved symbol (e.g., HermesAgentBadge
or AgentColorProvider.colorForAgent), and run tests/CI to confirm the original
file is now under the file-length threshold.
In `@Sources/SessionIndexStore.swift`:
- Around line 144-153: Extract the Hermes resume command construction into an
internal helper on SessionEntry in the HermesAgentIndex file and delegate the
switch case to it: create e.g. internal static func
hermesResumeCommand(sessionId: String, source: String?, model: String?) ->
String in the HermesAgentIndex extension (implement the same parts logic: start
with "hermes", append "--tui" when source == "tui", append "--resume
\(shellQuote(...))", and optionally "--model \(shellQuote(...))"), ensure the
shellQuote helper used in SessionIndexStore is made accessible (change
Self.shellQuote to an internal static shellQuote or copy a minimal
shellQuoteForHermes into HermesAgentIndex), then replace the .hermesAgent branch
in SessionIndexStore to return SessionEntry.hermesResumeCommand(sessionId:
sessionId, source: source, model: model).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 197aa7ad-6820-47dc-b12a-a11b544ed253
⛔ Files ignored due to path filters (1)
Assets.xcassets/AgentIcons/HermesAgent.imageset/HermesAgent.svgis excluded by!**/*.svg
📒 Files selected for processing (30)
Assets.xcassets/AgentIcons/HermesAgent.imageset/Contents.jsonCLI/CMUXCLI+DocsSettings.swiftCLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchEnvironmentPolicy.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizer.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerAdditionalPolicies.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentHookConfig.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentSessionResolver.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchSanitizerTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/HermesAgentHookConfigTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/HermesAgentSessionResolverTests.swiftPackages/CMUXHermesAgentIndex/Package.swiftPackages/CMUXHermesAgentIndex/Sources/CMUXHermesAgentIndex/HermesAgentIndex.swiftPackages/CMUXHermesAgentIndex/Tests/CMUXHermesAgentIndexTests/HermesAgentIndexTests.swiftPackages/CMUXWorkstream/Sources/CMUXWorkstream/WorkstreamSource.swiftResources/Localizable.xcstringsSources/Feed/FeedPanelView.swiftSources/Feed/FeedPermissionActionPolicy.swiftSources/HermesAgentIndex.swiftSources/RestorableAgentSession.swiftSources/RestorableAgentTypes.swiftSources/SessionAgentPresentation.swiftSources/SessionIndexStore.swiftSources/SessionIndexView.swiftSources/TaskManagerSnapshot.swiftcmuxTests/FeedCoordinatorTests.swiftcmuxTests/RestorableAgentHookProviderResumeTests.swiftcmuxTests/RestorableAgentNonInteractiveTests.swiftcmuxTests/SessionPersistenceTests.swift
👮 Files not reviewed due to content moderation or server errors (4)
- cmuxTests/SessionPersistenceTests.swift
- Sources/SessionIndexView.swift
- Packages/CMUXHermesAgentIndex/Tests/CMUXHermesAgentIndexTests/HermesAgentIndexTests.swift
- cmuxTests/RestorableAgentHookProviderResumeTests.swift
| ) AS preview | ||
| FROM sessions s | ||
| LEFT JOIN messages m ON m.session_id = s.id | ||
| WHERE s.source IN ('cli', 'tui') |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Hardcoded s.source IN ('cli','tui') will silently drop future source kinds.
If upstream Hermes Agent introduces additional source values (e.g., a daemon/api transport), those sessions disappear from the index without any error. Consider promoting the allowed list to a private static let knownSources = ["cli", "tui"] so it's a single point of maintenance, and add a brief comment explaining why we filter at all (presumably to match what resumeCommand knows how to launch).
🤖 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/CMUXHermesAgentIndex/Sources/CMUXHermesAgentIndex/HermesAgentIndex.swift`
at line 168, The SQL WHERE clause currently hardcodes s.source IN ('cli', 'tui')
which will silently omit future source kinds; introduce a single-source-of-truth
by adding a private static let knownSources = ["cli", "tui"] on the
HermesAgentIndex type, replace the literal list in the query builder with a
join/interpolation of HermesAgentIndex.knownSources, and add a short comment
above knownSources explaining we filter to sources resumeCommand knows how to
launch; update any reference to the clause (e.g., in the method that builds the
session query) to use knownSources so new source kinds won’t be accidentally
missed.
| } | ||
| } | ||
|
|
||
| public enum HermesAgentHookAllowlist { | ||
| public static func installing(events: [HermesAgentHookConfig.Event], in existing: Data?, approvedAt: Date = Date()) throws -> Data { | ||
| var object = try decode(existing) | ||
| let approvals = object["approvals"] as? [[String: Any]] ?? [] | ||
| var keyed: [String: [String: Any]] = [:] | ||
| for approval in approvals { | ||
| guard let event = approval["event"] as? String, | ||
| let command = approval["command"] as? String else { | ||
| continue | ||
| } | ||
| keyed[key(event: event, command: command)] = approval | ||
| } | ||
|
|
||
| let iso = ISO8601DateFormatter().string(from: approvedAt) | ||
| for event in events { | ||
| keyed[key(event: event.name, command: event.command)] = [ |
There was a problem hiding this comment.
installing builds keyed only from entries that have both a string event and a string command, silently skipping everything else. Then object["approvals"] = keyed.values.sorted { … } is written back, permanently deleting any skipped entry. By contrast, uninstalling correctly keeps non-conforming entries via return true.
Concrete failure: if Hermes or a third-party tool ever writes an allowlist approval with a different schema (e.g. a numeric command, an extra top-level key with no event, or a format revision), a single cmux hooks hermes-agent install call irreversibly removes those entries. The fix is to collect non-conforming entries into a separate array and merge them back before serializing, the same way uninstalling passes them through.
There was a problem hiding this comment.
Fixed in 04bace9. now preserves non-conforming approval entries as passthrough data while adding cmux-owned approvals, and covers the regression.
— Claude Code
There was a problem hiding this comment.
Clarifying the previous reply: fixed in 04bace9. HermesAgentHookAllowlist.installing now preserves non-conforming approval entries as passthrough data while adding cmux-owned approvals, and allowlistInstallPreservesNonConformingApprovals covers the regression.
— Claude Code
There was a problem hiding this comment.
Fixed earlier by preserving non-conforming allowlist approvals in a passthrough array before writing cmux approvals, with regression coverage in HermesAgentHookConfigTests. The current code keeps third-party/non-schema entries instead of dropping them.
— Claude Code
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/SessionIndexStore.swift (1)
1284-1290:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUnknown-folder directory search currently returns entries from all folders
Line 1284 maps nil/empty directory scope to
cwdFilter = nil, and Line 1288 fetches merged unfiltered results, but this branch never applies the no-folder post-filter (($0.cwd ?? "").isEmpty). That breaks theSearchScope.directory(nil/"")contract and leaks unrelated sessions into the “(no folder)” bucket.💡 Suggested fix
case .directory(let path): - let cwdFilter = (path?.isEmpty == false) ? path : nil + let noFolderScope = (path == nil) || ((path ?? "").isEmpty) + let cwdFilter = noFolderScope ? nil : path // Multi-agent merge: fetch the union of (offset+limit) per agent so the // merge-sort can produce a stable global ordering, then slice. let target = offset + limit - let merged = await Self.mergedAgentEntries(needle: needle, cwdFilter: cwdFilter, limit: target, errorBag: bag) + var merged = await Self.mergedAgentEntries( + needle: needle, + cwdFilter: cwdFilter, + limit: target, + errorBag: bag + ) + if noFolderScope { + merged = merged.filter { ($0.cwd ?? "").isEmpty } + } let sorted = merged.sorted { $0.modified > $1.modified } entries = Array(sorted.dropFirst(offset).prefix(limit))🤖 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 `@Sources/SessionIndexStore.swift` around lines 1284 - 1290, The directory-search branch maps nil/empty scope to cwdFilter = nil which causes mergedAgentEntries(...) to return results from all folders and never applies the “no-folder” post-filter; update the code in SessionIndexStore where merged is assigned to apply a post-filter when cwdFilter is nil: compute merged = await Self.mergedAgentEntries(...) as before, then if cwdFilter == nil replace merged with merged.filter { ($0.cwd ?? "").isEmpty } before performing the sort, dropFirst(offset) and prefix(limit) slicing so SearchScope.directory(nil/"") only returns sessions with empty cwd; keep the existing target (offset+limit) behavior but apply the empty-cwd filter prior to sorting/slicing.
🤖 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 `@CLI/CMUXCLI`+DocsSettings.swift:
- Around line 91-96: The docs commands array in CMUXCLI+DocsSettings.swift
currently lists "cmux hooks hermes-agent install" but omits the uninstall entry;
update the commands array (the commands property/variable in
CMUXCLI+DocsSettings) to also include "cmux hooks hermes-agent uninstall"
alongside the other entries so the docs output for `cmux docs agents` advertises
both install and uninstall for Hermes.
In `@GhosttyTabs.xcodeproj/project.pbxproj`:
- Around line 373-374: The project contains a duplicate HermesAgentIndex.swift
in the target Sources that redeclares public types already provided by the
CMUXHermesAgentIndex package (types: HermesAgentIndexedSession,
HermesAgentIndexResult, HermesAgentTranscriptTurn, HermesAgentIndexError,
HermesAgentIndex), causing symbol collisions; to fix, remove the in-target
Sources/HermesAgentIndex.swift entry (and its PBXBuildFile reference) and update
any files that referenced the removed local declarations to import
CMUXHermesAgentIndex and use its types, or alternatively convert the local
HermesAgentIndex.swift into a thin facade that re-exports the package types via
import CMUXHermesAgentIndex and typealiases rather than redeclaring them.
In `@Sources/SessionIndexView.swift`:
- Around line 1231-1235: transcriptRole(from:) currently wins over toolName,
causing nonstandard roles with a toolName to render as .event; change the role
determination so that presence of a non-empty turn.toolName forces .tool before
falling back to transcriptRole(from:) or .event (i.e. compute role by checking
turn.toolName first, then transcriptRole(from: turn.role) ?? .event) and keep
the subsequent toolName-based text composition using turn.toolName and
turn.content.
- Around line 1229-1241: Hermes path currently returns exactly the capped turns
from HermesAgentIndex.loadTranscript(sessionId:limit:) so the UI gets no
indication the preview was truncated; after building the compactMap result (the
array of SessionTranscriptTurn constructed with truncatedText(...)), detect if
the original turns array had count == maxPreviewTurns and, if so, append one
additional SessionTranscriptTurn (use a sentinel id like the existing preview
logic uses, role = .event or same marker role) whose text is the same truncation
marker string ("sessionIndex.preview.truncated") so the popover shows that the
preview was cut off; make this change near the code that maps turns ->
SessionTranscriptTurn and ensure you reference HermesAgentIndex.loadTranscript,
maxPreviewTurns, truncatedText, and SessionTranscriptTurn.
---
Outside diff comments:
In `@Sources/SessionIndexStore.swift`:
- Around line 1284-1290: The directory-search branch maps nil/empty scope to
cwdFilter = nil which causes mergedAgentEntries(...) to return results from all
folders and never applies the “no-folder” post-filter; update the code in
SessionIndexStore where merged is assigned to apply a post-filter when cwdFilter
is nil: compute merged = await Self.mergedAgentEntries(...) as before, then if
cwdFilter == nil replace merged with merged.filter { ($0.cwd ?? "").isEmpty }
before performing the sort, dropFirst(offset) and prefix(limit) slicing so
SearchScope.directory(nil/"") only returns sessions with empty cwd; keep the
existing target (offset+limit) behavior but apply the empty-cwd filter prior to
sorting/slicing.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2c5a9071-5373-4aa0-b0bc-783b48b89178
📒 Files selected for processing (7)
CLI/CMUXCLI+DocsSettings.swiftGhosttyTabs.xcodeproj/project.pbxprojSources/Feed/FeedPanelView.swiftSources/SessionIndexStore.swiftSources/SessionIndexView.swiftSources/SessionTranscriptTypes.swiftcmuxTests/RestorableAgentHookProviderHermesTests.swift
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 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 `@CLI/cmux.swift`:
- Around line 16354-16356: Guard block prints the hardcoded default
def.configDir instead of the actual resolved configDir, causing misleading
messages when HERMES_HOME is customized; update the print statement inside the
guard that checks fm.fileExists(atPath: configDir) to interpolate the resolved
configDir variable (and keep def.displayName) so the message shows the real path
(e.g., use configDir instead of def.configDir in the string).
In
`@Packages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentHookConfig.swift`:
- Around line 203-206: The current decode and installing/uninstalling paths
silently replace malformed allowlist JSON with an empty approvals array;
instead, modify HermesAgentHookConfig.decode to throw an error if the root JSON
is not a dictionary (do not return ["approvals": []]) and update
installing(events:in:approvedAt:) and the corresponding uninstall paths (the
blocks around lines 230-233 and 244-250) to validate that object["approvals"] is
actually a [[String: Any]] and throw a descriptive validation error when it is
missing or malformed rather than defaulting to an empty list so the CLI surfaces
the problem.
- Around line 158-184: The directEventLineIndexes(in:hooksIndex:) helper
currently only treats events with an empty suffix or comment as existing (e.g.,
"pre_tool_call:") but ignores inline-empty forms like "pre_tool_call: []" or
"pre_tool_call: {}", causing duplicate keys to be appended; update the logic in
directEventLineIndexes to also recognize suffixes that are empty collections by
checking if suffix starts with "[" or "{" (after trimming) and treat those as
existing by setting indexes[name] = index, and add a regression test that feeds
a hooks block containing an inline-empty event (e.g., "pre_tool_call: []" and
"pre_tool_call: {}") to ensure the installer converts or preserves it instead of
appending a duplicate block.
In
`@Packages/CMUXAgentVault/Sources/CMUXAgentVault/Providers/HermesAgent/HermesAgentIndex.swift`:
- Around line 96-102: The early-return on overflow (result of
offset.addingReportingOverflow(limit)) and the existing cwdFilter guard
currently return an empty HermesAgentIndexResult(sessions: [], errors: []) with
no indication why; add brief explanatory comments above both guards: for the
overflow guard note that an offset+limit overflow is intentionally treated as
"no rows" (not a DB error) and reference this behavior so future
readers/debuggers understand it, and for the cwdFilter guard add a short comment
pointing to the documented cwd-less behavior; keep behavior unchanged and do not
add error entries, just the clarifying comments near the
offset.addingReportingOverflow(limit) check and the cwdFilter check.
- Around line 318-329: Add a short clarifying comment in the withDatabase(_:_:)
helper above the sqlite3_busy_timeout call explaining that the 50 ms busy
timeout is intentional because this helper is used with a private snapshot DB
copied into a fresh temp dir (no contention expected), and warn that if
withDatabase is ever used against the live state.db (e.g., bypassing
makeSnapshot in HermesAgentIndex) the timeout may be too short and should be
increased; reference the function name withDatabase(_:,_:), the
sqlite3_busy_timeout call, and the makeSnapshot usage in HermesAgentIndex in the
comment so future maintainers understand the assumption and risk.
- Around line 206-213: The code currently binds likePattern five times using a
hardcoded range (1...5) which is brittle; update the binding to derive the
required parameter count from the prepared statement or switch to a single named
parameter. Specifically, in HermesAgentIndex (where stmt, likePattern and
destructor are used) replace the hardcoded loop with either: (a) use
sqlite3_bind_parameter_count(stmt) to get the actual parameter count and loop
from 1...Int(count) calling sqlite3_bind_text, or (b) change the SQL to use a
single named parameter (e.g., :needle) for all LIKE occurrences and bind that
named parameter once via sqlite3_bind_text with the named index; ensure error
handling still throws HermesAgentIndexError.sqlite(sqliteMessage(db) ?? "bind
failed") on bind failures.
In `@Sources/HermesAgentIndex.swift`:
- Around line 61-72: hermesResumeCommand currently rebuilds the resume
invocation without preserving a custom HERMES_HOME, so sessions started with a
non-default home will resume against the wrong location; update SessionEntry to
store the captured Hermes home (e.g., a property like hermesHome or
hermesHomePath) and modify static func hermesResumeCommand(sessionId: String,
source: String?, model: String?, hermesHome: String?) -> String (or use the
instance property if you change the method to non-static) to prefix the command
with "HERMES_HOME=\(Self.shellQuote(hermesHome))" when hermesHome is
non-nil/non-empty, while preserving existing --tui, --resume and --model
behavior and continuing to use Self.shellQuote for safe quoting.
In `@Sources/SessionIndexView.swift`:
- Around line 1229-1244: The code assumes turns.count == maxPreviewTurns implies
more rows, but HermesAgentIndex.loadTranscript(sessionId:limit:) returns up to
the limit so an exact-size transcript falsely shows a truncation marker; change
the call to request one extra row (use
HermesAgentIndex.loadTranscript(sessionId: sessionId, limit: maxPreviewTurns +
1)) then compute previewTurns from the first maxPreviewTurns entries, and only
call appendTurnLimitMarker(to:id:) when turns.count > maxPreviewTurns; update
any related trimming logic around previewTurns (variables: turns, previewTurns,
maxPreviewTurns, appendTurnLimitMarker) and adjust function signature if
loadTranscript cannot accept limit+1.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a8763f91-85d3-4bdf-beae-e88c5c966fd0
📒 Files selected for processing (13)
CLI/CMUXCLI+DocsSettings.swiftCLI/cmux.swiftGhosttyTabs.xcodeproj/project.pbxprojPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerAdditionalPolicies.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentHookConfig.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchSanitizerTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/HermesAgentHookConfigTests.swiftPackages/CMUXAgentVault/Package.swiftPackages/CMUXAgentVault/Sources/CMUXAgentVault/Providers/HermesAgent/HermesAgentIndex.swiftPackages/CMUXAgentVault/Tests/CMUXAgentVaultTests/Providers/HermesAgent/HermesAgentIndexTests.swiftSources/HermesAgentIndex.swiftSources/SessionIndexStore.swiftSources/SessionIndexView.swift
| let (_, overflow) = offset.addingReportingOverflow(limit) | ||
| guard !overflow else { | ||
| return HermesAgentIndexResult(sessions: [], errors: []) | ||
| } | ||
| guard cwdFilter == nil else { | ||
| return HermesAgentIndexResult(sessions: [], errors: []) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Overflow and cwdFilter early returns silently drop user intent.
offset.addingReportingOverflow(limit) overflowing and cwdFilter != nil both fall through to an empty result with no entry in errors. The cwdFilter path is now documented (line 4/85) so callers know Hermes is intentionally cwd-less; consider mirroring that for the overflow guard with a brief comment so a future debugger doesn't think the DB returned zero rows. The behavior itself is fine.
🤖 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/CMUXAgentVault/Sources/CMUXAgentVault/Providers/HermesAgent/HermesAgentIndex.swift`
around lines 96 - 102, The early-return on overflow (result of
offset.addingReportingOverflow(limit)) and the existing cwdFilter guard
currently return an empty HermesAgentIndexResult(sessions: [], errors: []) with
no indication why; add brief explanatory comments above both guards: for the
overflow guard note that an offset+limit overflow is intentionally treated as
"no rows" (not a DB error) and reference this behavior so future
readers/debuggers understand it, and for the cwdFilter guard add a short comment
pointing to the documented cwd-less behavior; keep behavior unchanged and do not
add error entries, just the clarifying comments near the
offset.addingReportingOverflow(limit) check and the cwdFilter check.
| if hasNeedle { | ||
| let likePattern = "%\(trimmedNeedle)%" | ||
| let destructor = unsafeBitCast(OpaquePointer(bitPattern: -1), to: sqlite3_destructor_type.self) | ||
| for index in 1...5 { | ||
| guard sqlite3_bind_text(stmt, Int32(index), likePattern, -1, destructor) == SQLITE_OK else { | ||
| throw HermesAgentIndexError.sqlite(sqliteMessage(db) ?? "bind failed") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Brittle magic number 1...5 for placeholder binding.
The loop binds five copies of likePattern in lockstep with the five LIKE ? placeholders in the dynamic SQL above (lines 177–186). If the WHERE clause grows or shrinks a placeholder, this loop won't catch the mismatch until SQLite reports a runtime bind error. Derive the count from the SQL structure (or use named parameters like ?1 and bind once) to make the coupling explicit.
♻️ Suggested fix using a named parameter (single bind)
if hasNeedle {
sql += """
AND (
- LOWER(s.id) LIKE ?
- OR LOWER(COALESCE(s.title, '')) LIKE ?
- OR LOWER(COALESCE(s.model, '')) LIKE ?
+ LOWER(s.id) LIKE ?1
+ OR LOWER(COALESCE(s.title, '')) LIKE ?1
+ OR LOWER(COALESCE(s.model, '')) LIKE ?1
OR EXISTS (
SELECT 1
FROM messages needle_messages
WHERE needle_messages.session_id = s.id
AND (
- LOWER(COALESCE(needle_messages.content, '')) LIKE ?
- OR LOWER(COALESCE(needle_messages.tool_name, '')) LIKE ?
+ LOWER(COALESCE(needle_messages.content, '')) LIKE ?1
+ OR LOWER(COALESCE(needle_messages.tool_name, '')) LIKE ?1
)
LIMIT 1
)
)
"""
}
@@
if hasNeedle {
let likePattern = "%\(trimmedNeedle)%"
let destructor = unsafeBitCast(OpaquePointer(bitPattern: -1), to: sqlite3_destructor_type.self)
- for index in 1...5 {
- guard sqlite3_bind_text(stmt, Int32(index), likePattern, -1, destructor) == SQLITE_OK else {
- throw HermesAgentIndexError.sqlite(sqliteMessage(db) ?? "bind failed")
- }
+ guard sqlite3_bind_text(stmt, 1, likePattern, -1, destructor) == SQLITE_OK else {
+ throw HermesAgentIndexError.sqlite(sqliteMessage(db) ?? "bind failed")
}
}🤖 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/CMUXAgentVault/Sources/CMUXAgentVault/Providers/HermesAgent/HermesAgentIndex.swift`
around lines 206 - 213, The code currently binds likePattern five times using a
hardcoded range (1...5) which is brittle; update the binding to derive the
required parameter count from the prepared statement or switch to a single named
parameter. Specifically, in HermesAgentIndex (where stmt, likePattern and
destructor are used) replace the hardcoded loop with either: (a) use
sqlite3_bind_parameter_count(stmt) to get the actual parameter count and loop
from 1...Int(count) calling sqlite3_bind_text, or (b) change the SQL to use a
single named parameter (e.g., :needle) for all LIKE occurrences and bind that
named parameter once via sqlite3_bind_text with the named index; ensure error
handling still throws HermesAgentIndexError.sqlite(sqliteMessage(db) ?? "bind
failed") on bind failures.
| private static func withDatabase<T>(_ path: String, _ body: (OpaquePointer) throws -> T) throws -> T { | ||
| var db: OpaquePointer? | ||
| let openResult = sqlite3_open_v2(path, &db, SQLITE_OPEN_READONLY, nil) | ||
| guard openResult == SQLITE_OK, let db else { | ||
| let message = sqliteMessage(db) ?? "open failed with code \(openResult)" | ||
| sqlite3_close(db) | ||
| throw HermesAgentIndexError.sqlite(message) | ||
| } | ||
| defer { sqlite3_close(db) } | ||
| _ = sqlite3_busy_timeout(db, 50) | ||
| return try body(db) | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Confirm 50 ms sqlite3_busy_timeout is intentional for the snapshot.
The snapshot DB is private (copied into a fresh temp dir per call), so contention is essentially impossible and 50 ms is fine in practice. Just flagging that this is unusually short compared to typical sqlite3_busy_timeout values (often seconds) — if this helper is ever reused for the live state.db path (i.e., bypassing makeSnapshot), 50 ms could surface spurious SQLITE_BUSY under concurrent Hermes writes. A code comment explaining the snapshot assumption would prevent accidental misuse.
📝 Suggested comment
defer { sqlite3_close(db) }
+ // 50 ms is sufficient because callers always operate on a private
+ // temp-dir snapshot (see makeSnapshot); no concurrent writers exist.
_ = sqlite3_busy_timeout(db, 50)🤖 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/CMUXAgentVault/Sources/CMUXAgentVault/Providers/HermesAgent/HermesAgentIndex.swift`
around lines 318 - 329, Add a short clarifying comment in the withDatabase(_:_:)
helper above the sqlite3_busy_timeout call explaining that the 50 ms busy
timeout is intentional because this helper is used with a private snapshot DB
copied into a fresh temp dir (no contention expected), and warn that if
withDatabase is ever used against the live state.db (e.g., bypassing
makeSnapshot in HermesAgentIndex) the timeout may be too short and should be
increased; reference the function name withDatabase(_:,_:), the
sqlite3_busy_timeout call, and the makeSnapshot usage in HermesAgentIndex in the
comment so future maintainers understand the assumption and risk.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
Sources/SessionIndexView.swift (1)
1242-1244:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFalse truncation marker when transcript has exactly
maxPreviewTurnsturns.
HermesAgentIndex.loadTranscript(sessionId:limit:)caps results atlimit, soturns.count == maxPreviewTurnsdoesn't actually prove there are more rows beyond the cap. A Hermes session with exactly 500 turns will render a "Preview truncated" marker even when nothing was cut off. Probe for one extra row instead.🛠️ Suggested fix
- let turns = try HermesAgentIndex.loadTranscript(sessionId: sessionId, limit: maxPreviewTurns) - var previewTurns: [SessionTranscriptTurn] = turns.enumerated().compactMap { index, turn -> SessionTranscriptTurn? in + let rawTurns = try HermesAgentIndex.loadTranscript(sessionId: sessionId, limit: maxPreviewTurns + 1) + let didHitTurnLimit = rawTurns.count > maxPreviewTurns + let turns = Array(rawTurns.prefix(maxPreviewTurns)) + var previewTurns: [SessionTranscriptTurn] = turns.enumerated().compactMap { index, turn -> SessionTranscriptTurn? in let role: SessionTranscriptRole = (turn.toolName?.isEmpty == false) ? .tool : (transcriptRole(from: turn.role) ?? .event) let text: String if role == .tool, let toolName = turn.toolName, !toolName.isEmpty { text = [toolName, turn.content].joined(separator: "\n\n") } else { text = turn.content } let trimmed = text.trimmingCharacters(in: .whitespacesAndNewlines) guard !trimmed.isEmpty else { return nil } return SessionTranscriptTurn(id: index, role: role, text: truncatedText(trimmed, role: role)) } - if turns.count == maxPreviewTurns { + if didHitTurnLimit { appendTurnLimitMarker(to: &previewTurns, id: previewTurns.count) }🤖 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 `@Sources/SessionIndexView.swift` around lines 1242 - 1244, The current logic uses turns.count == maxPreviewTurns to decide to append the "Preview truncated" marker, but HermesAgentIndex.loadTranscript(sessionId:limit:) caps results at the provided limit so equality doesn't prove there are more rows; change the load/query to request maxPreviewTurns + 1 items (call to HermesAgentIndex.loadTranscript(sessionId:limit:)) and then only take the first maxPreviewTurns into previewTurns, and append the turn-limit marker only when the fetched turns.count is greater than maxPreviewTurns (use the existing appendTurnLimitMarker(to:id:) and previewTurns collection as before).Packages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentHookConfig.swift (2)
158-188:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInline-empty event suffixes (
[]/{}) still treated as missing.
directEventLineIndexescontinues to recognize an event line only when the suffix is empty or a comment (line 182). If a user’sconfig.yamlhas e.g.pre_tool_call: []orpost_tool_call: {}, the installer falls into themissingEventsbranch and appends a secondpre_tool_call:key underhooks:, producing an invalid YAML mapping with duplicate keys. Treating[]/{}as existing-but-empty (and rewriting to block form before insertion) avoids the duplicate-key foot-gun.🤖 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/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentHookConfig.swift` around lines 158 - 188, directEventLineIndexes currently treats an event as "missing" unless the inline suffix is empty or a comment (the suffix.isEmpty || suffix.hasPrefix("#") check); update this logic in directEventLineIndexes(in:hooksIndex:) to also recognize inline-empty collection literals (e.g. "[]" and "{}") as existing-but-empty so they are added to the indexes map instead of being considered missing; specifically, after producing suffix (the trimmed substring after the colon) normalize/remove trailing inline comments and treat suffix values like "[]" or "{}" (or whitespace-wrapped variants) as valid existing entries so they get indexes[name] = index and later will be rewritten to block form before insertion.
247-255:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
decodestill silently rewrites a malformed allowlist into{ "approvals": [] }.When
state/shell-hooks-allowlist.jsonis parseable but not a dictionary at the root (e.g. an array, or an unexpected JSON value),decodereturns["approvals": []](line 252) and the nextinstallingwrite replaces the user’s file wholesale rather than surfacing the inconsistency. Throwing here would let the CLI report the bad file and avoid clobbering it. Theapprovalscast at lines 205 and 236 has the same silent-fallback shape and would benefit from the same treatment, while still allowing the explicit passthrough behavior covered byallowlistInstallPreservesNonConformingApprovals(which exercises malformed individual approvals, not a malformed root).🤖 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/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentHookConfig.swift` around lines 247 - 255, The decode(_:) function currently silently returns ["approvals": []] when the JSON root is parseable but not a dictionary; change decode(_ existing: Data?) to throw a decoding/validation error if JSONSerialization.jsonObject(with:) yields a non-[String: Any] root (rather than returning a default approvals dict), so the CLI can surface the bad file; likewise replace the silent optional casts of "approvals" (the places that currently do something like let approvals = object["approvals"] as? [[String: Any]] at the sites around the approvals handling and in allowlistInstallPreservesNonConformingApprovals) with guarded checks that throw when the root structure is not the expected dictionary (but still allow individual approval entries to be malformed if and only if allowlistInstallPreservesNonConformingApprovals is true), so only per-approval validation is relaxed while a malformed top-level JSON file causes an error instead of being clobbered.
🤖 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/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerAdditionalPolicies.swift`:
- Around line 389-415: The current hermesAgentNonRestorableCommands Set in
AgentLaunchSanitizerAdditionalPolicies.swift is a denylist and is incomplete;
change the approach to an allowlist so only the subcommand "chat" or no
subcommand is permitted: replace or deprecate hermesAgentNonRestorableCommands
usage and implement a check (in AgentLaunchSanitizer or wherever that Set is
consulted) that accepts only an empty first positional token or "chat" and
rejects every other first token (including fallback, webhook, dump, checkpoints,
whatsapp, slack, etc.); update any error/deny messages accordingly and ensure
tests or callers using hermesAgentNonRestorableCommands are adjusted to use the
new allowlist behavior.
- Around line 444-447: The variadicOptions array currently includes "--skills"
and "-s" which duplicates their presence in valueOptions and causes the
sanitizer's optionWidth logic to greedily consume following tokens; remove the
"--skills" and "-s" entries from the variadicOptions definition so those flags
remain only in valueOptions and are treated as single-value (repeatable) options
by the sanitizer (see variadicOptions, valueOptions and the optionWidth handling
in the sanitizer).
---
Duplicate comments:
In
`@Packages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentHookConfig.swift`:
- Around line 158-188: directEventLineIndexes currently treats an event as
"missing" unless the inline suffix is empty or a comment (the suffix.isEmpty ||
suffix.hasPrefix("#") check); update this logic in
directEventLineIndexes(in:hooksIndex:) to also recognize inline-empty collection
literals (e.g. "[]" and "{}") as existing-but-empty so they are added to the
indexes map instead of being considered missing; specifically, after producing
suffix (the trimmed substring after the colon) normalize/remove trailing inline
comments and treat suffix values like "[]" or "{}" (or whitespace-wrapped
variants) as valid existing entries so they get indexes[name] = index and later
will be rewritten to block form before insertion.
- Around line 247-255: The decode(_:) function currently silently returns
["approvals": []] when the JSON root is parseable but not a dictionary; change
decode(_ existing: Data?) to throw a decoding/validation error if
JSONSerialization.jsonObject(with:) yields a non-[String: Any] root (rather than
returning a default approvals dict), so the CLI can surface the bad file;
likewise replace the silent optional casts of "approvals" (the places that
currently do something like let approvals = object["approvals"] as? [[String:
Any]] at the sites around the approvals handling and in
allowlistInstallPreservesNonConformingApprovals) with guarded checks that throw
when the root structure is not the expected dictionary (but still allow
individual approval entries to be malformed if and only if
allowlistInstallPreservesNonConformingApprovals is true), so only per-approval
validation is relaxed while a malformed top-level JSON file causes an error
instead of being clobbered.
In `@Sources/SessionIndexView.swift`:
- Around line 1242-1244: The current logic uses turns.count == maxPreviewTurns
to decide to append the "Preview truncated" marker, but
HermesAgentIndex.loadTranscript(sessionId:limit:) caps results at the provided
limit so equality doesn't prove there are more rows; change the load/query to
request maxPreviewTurns + 1 items (call to
HermesAgentIndex.loadTranscript(sessionId:limit:)) and then only take the first
maxPreviewTurns into previewTurns, and append the turn-limit marker only when
the fetched turns.count is greater than maxPreviewTurns (use the existing
appendTurnLimitMarker(to:id:) and previewTurns collection as before).
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: dbbc1f8e-8c8b-4116-9866-330ccdbd70fe
📒 Files selected for processing (4)
Packages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerAdditionalPolicies.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentHookConfig.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/HermesAgentHookConfigTests.swiftSources/SessionIndexView.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/SessionIndexStore.swift (1)
711-721:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe
(no folder)path paginates the wrong universe.At the UI boundary
nilmeans “unknown cwd only”, but inside each agent loadercwdFilter == nilmeans “no filter”. The current workaround fetches only the firstoffset + limitunfiltered rows per agent and drops non-empty cwd rows afterward, so older unknown-folder sessions never make it intomergedonce an agent has enough newer sessions with a real cwd. This will undercount and truncate the(no folder)section in both search and snapshot mode. Please thread a distinct per-agent filter for “cwd is missing” instead of post-filtering after the limit.Also applies to: 1281-1288
🤖 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 `@Sources/SessionIndexStore.swift` around lines 711 - 721, The code treats cwd == nil as a request for the "(no folder)" bucket but then passes cwdFilter == nil to mergedAgentEntries where nil means "no filter", causing per-agent loaders to return unfiltered rows and post-filtering to drop older missing-cwd entries; fix by threading an explicit per-agent "cwd-is-missing" signal instead of reusing nil: add a boolean or enum (e.g., cwdIsMissing) and when noFolderScope is true call Self.mergedAgentEntries with that signal (or a distinct sentinel filter) so each agent loader performs "cwd IS NULL OR cwd == ''" at the source rather than fetching unfiltered rows and post-filtering; update mergedAgentEntries and the per-agent loader call sites (also apply the same change around the similar block at the other location noted) so pagination/filtering operate on the correct universe.
🤖 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 `@CLI/cmux.swift`:
- Around line 16346-16389: installHermesAgentHooks currently only checks
fm.fileExists(atPath: configDir) so if configDir is a regular file the
subsequent write fails opaquely; update installHermesAgentHooks to mirror
installAgentHooks by using FileManager.fileExists(atPath:isDirectory:) (or an
equivalent isDirectory check) and if the path exists but is not a directory
throw or print the same diagnostic string "\"\(configDir) exists but is not a
directory…\"" before proceeding to read/write the hook files (referencing
installHermesAgentHooks, configDir, readAgentHookConfig, and
newString.write(toFile:)).
In
`@Packages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentHookConfig.swift`:
- Around line 29-31: The install path is converting inline-empty YAML like
"hooks: []" / "pre_tool_call: {}" into bare headers and uninstall only strips
markers, losing the original inline-empty form; update the logic in
hooksLineIndex(in:), inlineEmptyHooksLine(_:), and leadingWhitespace(_) handling
so the installer preserves enough metadata (for example embed a reversible
comment or use the marker content to record the original inline form next to the
cmux markers) so uninstall can restore the exact original line instead of
"hooks:" / "pre_tool_call:"; also add a unit/regression test that runs
install→uninstall on files containing inline-empty hooks/events and asserts the
file content is identical to the original.
---
Outside diff comments:
In `@Sources/SessionIndexStore.swift`:
- Around line 711-721: The code treats cwd == nil as a request for the "(no
folder)" bucket but then passes cwdFilter == nil to mergedAgentEntries where nil
means "no filter", causing per-agent loaders to return unfiltered rows and
post-filtering to drop older missing-cwd entries; fix by threading an explicit
per-agent "cwd-is-missing" signal instead of reusing nil: add a boolean or enum
(e.g., cwdIsMissing) and when noFolderScope is true call Self.mergedAgentEntries
with that signal (or a distinct sentinel filter) so each agent loader performs
"cwd IS NULL OR cwd == ''" at the source rather than fetching unfiltered rows
and post-filtering; update mergedAgentEntries and the per-agent loader call
sites (also apply the same change around the similar block at the other location
noted) so pagination/filtering operate on the correct universe.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1dea2687-f9f2-4603-9110-17bc314022a1
📒 Files selected for processing (7)
CLI/cmux.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/HermesAgentHookConfig.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/HermesAgentHookConfigTests.swiftSources/HermesAgentIndex.swiftSources/SessionIndexStore.swiftSources/SessionIndexView.swiftcmuxTests/RestorableAgentHookProviderHermesTests.swift
| private func installHermesAgentHooks(_ def: AgentHookDef) throws { | ||
| let fm = FileManager.default | ||
| let configDir = def.resolvedConfigDir() | ||
| let filePath = "\(configDir)/\(def.configFile)" | ||
| let allowlistPath = "\(configDir)/shell-hooks-allowlist.json" | ||
| let skipConfirm = ProcessInfo.processInfo.arguments.contains("--yes") | ||
| || ProcessInfo.processInfo.arguments.contains("-y") | ||
|
|
||
| guard fm.fileExists(atPath: configDir) else { | ||
| print("\(configDir) does not exist. Install \(def.displayName) first.") | ||
| return | ||
| } | ||
|
|
||
| let events = hermesAgentEvents(def: def) | ||
| let oldString = try readAgentHookConfig(filePath: filePath, displayName: def.displayName) | ||
| let newString = HermesAgentHookConfig.installing(events: events, in: oldString) | ||
|
|
||
| if oldString != newString { | ||
| if !skipConfirm { | ||
| Self.printInstallPreview( | ||
| path: filePath, | ||
| oldContent: oldString, | ||
| newContent: newString, | ||
| fallbackContent: newString | ||
| ) | ||
| print("\nProceed? [y/N] ", terminator: "") | ||
| guard readLine()?.lowercased().hasPrefix("y") == true else { | ||
| print("Aborted.") | ||
| return | ||
| } | ||
| } | ||
| try newString.write(toFile: filePath, atomically: true, encoding: .utf8) | ||
| print("\(def.displayName) hooks installed at \(filePath)") | ||
| } else { | ||
| print("\(def.displayName) hooks already up to date at \(filePath)") | ||
| } | ||
|
|
||
| let oldAllowlist = fm.contents(atPath: allowlistPath) | ||
| let newAllowlist = try HermesAgentHookAllowlist.installing(events: events, in: oldAllowlist) | ||
| if oldAllowlist != newAllowlist { | ||
| try newAllowlist.write(to: URL(fileURLWithPath: allowlistPath), options: .atomic) | ||
| print("Approved \(def.displayName) cmux shell hooks in \(allowlistPath)") | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add an isDirectory check on configDir for parity with the generic install path.
The generic installAgentHooks path explicitly throws "\(configDir) exists but is not a directory…" (Line 16252) before writing. installHermesAgentHooks only checks fm.fileExists(atPath: configDir), so if HERMES_HOME resolves to a regular file, the user gets an opaque write failure from newString.write(toFile:) instead of the actionable diagnostic the other agents produce. Worth keeping the two paths symmetric.
♻️ Proposed fix
- guard fm.fileExists(atPath: configDir) else {
+ var isDir: ObjCBool = false
+ guard fm.fileExists(atPath: configDir, isDirectory: &isDir) else {
print("\(configDir) does not exist. Install \(def.displayName) first.")
return
}
+ guard isDir.boolValue else {
+ throw CLIError(message: "\(configDir) exists but is not a directory. Move it aside before installing \(def.displayName) hooks.")
+ }🤖 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 `@CLI/cmux.swift` around lines 16346 - 16389, installHermesAgentHooks currently
only checks fm.fileExists(atPath: configDir) so if configDir is a regular file
the subsequent write fails opaquely; update installHermesAgentHooks to mirror
installAgentHooks by using FileManager.fileExists(atPath:isDirectory:) (or an
equivalent isDirectory check) and if the path exists but is not a directory
throw or print the same diagnostic string "\"\(configDir) exists but is not a
directory…\"" before proceeding to read/write the hook files (referencing
installHermesAgentHooks, configDir, readAgentHookConfig, and
newString.write(toFile:)).
| let target = offset + limit | ||
| async let c = Self.timedAgent( | ||
| needle: needle, agent: .claude, cwdFilter: cwdFilter, | ||
| offset: 0, limit: target, errorBag: bag | ||
| ) | ||
| async let x = Self.timedAgent( | ||
| needle: needle, agent: .codex, cwdFilter: cwdFilter, | ||
| offset: 0, limit: target, errorBag: bag | ||
| ) | ||
| async let o = Self.timedAgent( | ||
| needle: needle, agent: .opencode, cwdFilter: cwdFilter, | ||
| offset: 0, limit: target, errorBag: bag | ||
| ) | ||
| async let r = Self.timedAgent( | ||
| needle: needle, agent: .rovodev, cwdFilter: cwdFilter, | ||
| offset: 0, limit: target, errorBag: bag | ||
| ) | ||
| let merged = (await c) + (await x) + (await o) + (await r) | ||
| var merged = await Self.mergedAgentEntries(needle: needle, cwdFilter: cwdFilter, limit: target, errorBag: bag) | ||
| if noFolderScope { | ||
| merged = merged.filter { ($0.cwd ?? "").isEmpty } | ||
| } | ||
| let sorted = merged.sorted { $0.modified > $1.modified } | ||
| entries = Array(sorted.dropFirst(offset).prefix(limit)) | ||
| } |
There was a problem hiding this comment.
noFolderScope post-filter silently drops all existing agent sessions
When path is nil or empty, the new merged.filter { ($0.cwd ?? "").isEmpty } keeps only entries with a nil/empty cwd. Claude, Codex, OpenCode, and RovoDev sessions all carry a non-empty cwd, so they are completely excluded from results whenever the user searches without a folder filter. Before this PR, passing cwdFilter: nil returned the full merged result without any post-filter; only Hermes sessions (which always set cwd: nil) survive the new guard.
The intent is likely the inverse: Hermes's nil cwd should not pollute directory-scoped views when a path is specified. The guard is already correct there because HermesAgentIndex.loadSessions short-circuits with an empty result for any non-nil cwdFilter. The additional post-filter on the noFolderScope branch is the part that should be removed.
Superseded by later commits; CodeRabbit status is passing on the current head.
Superseded by later commits; CodeRabbit status is passing on the current head.
| if noFolderScope { | ||
| merged = merged.filter { ($0.cwd ?? "").isEmpty } | ||
| } |
There was a problem hiding this comment.
noFolderScope filter drops all folder-scoped sessions from global search
When noFolderScope is true (no folder selected, path is nil or empty), mergedAgentEntries is called with cwdFilter: nil and returns sessions from every agent. The subsequent merged.filter { ($0.cwd ?? "").isEmpty } then keeps only entries whose cwd is nil or blank. Because every Claude, Codex, OpenCode, and RovoDev session carries a non-empty cwd, they are all removed from the results — only Hermes sessions (whose cwd is always nil) survive. Any global search in the "All" or unscoped session-index view returns a Hermes-only list, silently hiding the rest of the user's history.
The intent appears to be the inverse: Hermes sessions (with no cwd metadata) should be excluded from directory-scoped views when a specific path is set. That side is already correct — HermesAgentIndex.loadSessions short-circuits and returns empty for any non-nil cwdFilter. The post-filter on the noFolderScope branch is the incorrect half and should be removed.
This branch was successfully deployed
1 participant
Summary
cmux hooks hermes-agent install/uninstallfor Hermes Agent YAML shell hooks andshell-hooks-allowlist.jsonapprovals.state.db, plus session resume support.Upstream checked
Testing
swift test --package-path Packages/CMUXAgentLaunchswift test --package-path Packages/CMUXHermesAgentIndexswift test --package-path Packages/CMUXWorkstreamxcodebuild -project GhosttyTabs.xcodeproj -scheme cmux -configuration Debug -derivedDataPath /tmp/cmux-hermes-build CODE_SIGNING_ALLOWED=NO buildxcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -configuration Debug -derivedDataPath /tmp/cmux-hermes-tests CODE_SIGNING_ALLOWED=NO -only-testing:cmuxTests/RestorableAgentHookProviderResumeTests -only-testing:cmuxTests/RestorableAgentNonInteractiveTests -only-testing:cmuxTests/FeedCoordinatorTests -only-testing:cmuxTests/SessionPersistenceTests test./scripts/reload.sh --tag hermesHERMES_HOMEsmoke test forcmux hooks hermes-agent install --yesandcmux hooks hermes-agent uninstallSummary by cubic
Adds Hermes Agent hook support with session indexing, transcript previews, and reliable resume so
cmuxcan install hooks, surface Hermes sessions, and coordinate feed permissions end-to-end. Uninstall preserves inline Hermes YAML and launch sanitization drops startup-only input to prevent unsafe resumes.New Features
cmux hooks hermes-agent install|uninstallwrites cmux‑tagged YAML into~/.hermes/config.yaml(honorsHERMES_HOME) and managesshell-hooks-allowlist.json; uninstall removes only the cmux block and keeps other inline YAML; supportson_session_start,pre_llm_call,post_llm_call,on_session_end/finalize/reset, and Feed hookspre_tool_call,post_tool_call,pre_approval_request,post_approval_response.state.db(SQLite) to list CLI/TUI sessions with previews; transcript loader decodes Hermes\0json:content; added Hermes icon/label and a teal source chip in the UI.--tui,--model, andHERMES_HOME; drops startup‑only/sensitive flags (e.g.--continue/-c,--image,--resume,--api-key) and blocks one‑shot commands; maps Hermes events to Feed types, disables persistent/bypass permission modes, and gatesterminaltool calls.Migration
hermesis on PATH; setHERMES_HOMEif not using~/.hermes.cmux hooks hermes-agent install --yes. Uninstall:cmux hooks hermes-agent uninstall.Written for commit 6ea205f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests