Add Vault Pi agent restore support - #3582
Merged
Merged
Conversation
Add a persistence-level regression that models a Pi pane snapshot carrying a concrete JSONL session path through Vault autosave and reload. The current code cannot decode the pi agent kind, so this commit is expected to fail before the implementation commit. Constraint: Regression test commit must precede the fix so CI can show the behavior gap. Confidence: high Scope-risk: narrow Directive: Keep Pi restore targeted on --session; do not regress to --continue for pane-specific restoration. Tested: Not run locally per repository policy. Not-tested: Local XCTest execution intentionally skipped.
Pi exposes persistent JSONL sessions and a targeted `--session` resume path, so Vault now treats non-built-in agents as registered restore providers instead of adding another closed enum-only branch. The default registry includes Pi, and cmux.json can add or override registrations with detection rules, session id sources, cwd policy, and a resume-command template. Constraint: Pi should resume with `pi --session <path-or-id>`, not `pi --continue` Constraint: Local tests are not run for this repo task; validation is limited to static parsing and JSON checks before CI Rejected: Add Pi as a hardcoded sixth Vault-only agent | it would repeat the same work for the next JSONL-backed coding agent Rejected: Preserve Pi's original launch argv on resume | stale `--continue` or previous `--session` flags can reopen the wrong session Confidence: medium Scope-risk: moderate Directive: Keep new Vault agents data-driven through `vault.agents`; do not add provider-specific restore branches unless the CLI cannot be expressed by a template Tested: swiftc -frontend -parse on touched Swift files; python3 -m json.tool on Resources/Localizable.xcstrings and web/data/cmux.schema.json; git diff --check Not-tested: XCTest and app build, per task instruction to use CI and final reload.sh only
|
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 a Vault agent registration subsystem: schema and cmux config support, a registry and process scanner for registered agents (including Pi), session indexing/loading for registered entries, resume-command templating and custom resume flow, UI and localization changes, tests, docs, and Xcode project wiring. ChangesVault Agent Registration System
Sequence DiagramsequenceDiagram
participant User
participant VaultPanel as Vault Panel
participant Registry as Vault Registry
participant ProcScanner as Process Scanner
participant SessionStore as Session Store
participant CmdBuilder as Resume Command Builder
User->>VaultPanel: Open Vault / view sessions
VaultPanel->>Registry: Load registrations (built-in + local)
Registry-->>VaultPanel: Registrations list
VaultPanel->>ProcScanner: Enumerate running processes
ProcScanner->>ProcScanner: Parse argv & env (kern proc args)
loop for each process
ProcScanner->>Registry: Ask matches for process
alt matches registration
Registry-->>ProcScanner: Matching registration
ProcScanner->>Registry: Resolve sessionId (argv/pi file)
Registry-->>ProcScanner: sessionId
ProcScanner-->>VaultPanel: Emit detected snapshot
end
end
VaultPanel->>SessionStore: Merge detected snapshots with persisted sessions
alt user resumes session
User->>SessionStore: Request resume
SessionStore->>CmdBuilder: Build resume command
CmdBuilder->>Registry: Load registration template
CmdBuilder->>CmdBuilder: Expand placeholders ({{sessionId}}, {{cwd}}, ...)
CmdBuilder-->>SessionStore: Resume command
SessionStore-->>VaultPanel: Execute resume
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 1 warning, 1 inconclusive)
✅ Passed checks (7 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR adds config-driven Vault agent registration (new
Confidence Score: 4/5Safe to merge with one area worth a follow-up look: non-autosave saveSessionSnapshot paths in AppDelegate may still call CmuxVaultAgentRegistry.load() synchronously on the main actor. The autosave tick, resume command construction, and process detection are now correctly off-main. The remaining concern is that saveSessionSnapshot at AppDelegate line 3598 still invokes RestorableAgentSessionIndex.load() without a pre-loaded registry on window-close, app-termination, and several other save paths — if that load() still calls CmuxVaultAgentRegistry.load(), config files are parsed on the main thread on every state-change save. Sources/AppDelegate.swift (non-autosave saveSessionSnapshot fallback at line 3598) and Sources/SessionIndexModels.swift (over 400 lines with mixed responsibilities). Important Files Changed
Sequence DiagramsequenceDiagram
participant AD as AppDelegate
participant RASI as SessionIndex
participant Scanner as ProcessScanner
participant Reg as VaultRegistry
participant SIS as SessionIndexStore
participant UI as SessionIndexView
AD->>RASI: await loadIncludingProcessDetectedSnapshots
Note over RASI: Task.detached off main
RASI->>Reg: load workingDirectory
Reg-->>RASI: CmuxVaultAgentRegistry
RASI->>Scanner: processDetectedSnapshots
Scanner->>Scanner: sysctl KERN_PROCARGS2 per process
Scanner-->>RASI: keyed snapshots
RASI-->>AD: RestorableAgentSessionIndex
UI->>SIS: search registered agent
Note over SIS: Task.detached utility
SIS->>Reg: load for cwdFilter
SIS->>SIS: loadRegisteredAgentEntries
SIS-->>UI: SessionEntry array
UI->>UI: resumeCommandWithCwd via registrationOverride
Reviews (19): Last reviewed commit: "Keep merged Swift files within guard bud..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 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 `@cmuxTests/SessionPersistenceTests.swift`:
- Around line 1004-1085: The new test function
testPiVaultAgentSnapshotRoundTripBuildsTargetedSessionCommand pushes the file
past the Swift length budget; move that test into a new test file (e.g.,
SessionPersistencePiVaultTests.swift) by copying the entire test function into a
new XCTestCase subclass (ensure you include import XCTest and any required test
helpers or makeSnapshot/SessionPersistenceStore visibility), remove the original
function from cmuxTests/SessionPersistenceTests.swift, and run tests to confirm
compilation and that loadedAgent assertions (kind, sessionId, resumeCommand)
still pass.
In `@Sources/CmuxTopSnapshot.swift`:
- Around line 47-50: Move the new proc-args parser and its helper type out of
CmuxTopSnapshot.swift into its own file under Sources to satisfy the file-length
budget: create a new Swift source file (e.g., CmuxTopProcessArguments.swift)
containing the CmuxTopProcessArguments struct plus the related parser code and
any helper functions currently between lines 348–462, and update visibility
(internal) so CmuxTopProcessSnapshot and CmuxTopProcessScope can still reference
it; ensure there are no behavior changes and remove the moved code from
CmuxTopSnapshot.swift.
In `@Sources/RestorableAgentSession.swift`:
- Around line 607-630: The matches(_:) implementation in
CmuxVaultAgentDetectRule should require all configured detect clauses to be true
(conjunctive), not return early when processName matches; change matches(_:) to
compute a Boolean for processNameMatch and a Boolean for argvContainsMatch and
return their conjunction (or just argvContainsMatch if argvContains is empty).
Also change the argvContains matching logic in matches(_:) so each needle is
treated as a substring match (use argument.range(of: needle, options:
[.caseInsensitive, .literal]) != nil or search the joinedArguments for needles
with "/" or " "), instead of only doing exact/token equality—ensure you still
handle lastPathComponent by applying substring search to the lastPathComponent
rather than equality. Ensure these changes are made in the
CmuxVaultAgentDetectRule.matches(_ process: VaultObservedAgentProcess) method.
In `@Sources/RestorableAgentTypes.swift`:
- Around line 134-505: Extract the CmuxVault* declarations
(CmuxVaultConfigDefinition, CmuxVaultAgentRegistration,
CmuxVaultAgentDetectRule, CmuxVaultAgentSessionIDSource,
CmuxVaultAgentCWDPolicy, CmuxVaultAgentRegistry and any helpers like
decodeConfig/configPaths/findLocalConfig) into a new Swift source file and
remove them from Sources/RestorableAgentTypes.swift so the original file falls
under the CI length budget; ensure you add explicit access control
(public/internal/private) for the moved types and their initializers/props as
appropriate rather than relying on implicit internal visibility, keep the same
Codable/Sendable conformances and preserve method names (init(from:), isValidID,
defaultExecutable, load, registration, findLocalConfig, decodeConfig,
configPaths) so callers compile, and update any imports or module-level
references if necessary.
- Around line 183-189: The ID validation in RestorableAgentTypes (the guard
using Self.isValidID(id) in the init/from-decoder path) must also reject IDs
that collide with built-in agent kinds; update the guard to additionally ensure
RestorableAgentKind(rawValue: id) yields nil (or is not a built-in case) and
throw the same DecodingError.dataCorruptedError with the existing
debugDescription if it does collide. Apply the same check to the other
decoding/validation block referenced around lines 227-230 so both decoding paths
consistently reject built-in names.
- Around line 328-358: The decoder currently checks for a non-empty argvOption
first which lets argvOption override an explicit type; change the logic to
decode the container and the required "type" first (using
decoder.container(keyedBy: CodingKeys.self) and let type = ...), then branch on
type (.piSessionFile / .argvOption). For the .argvOption branch decode and
validate argvOption is present and non-empty (throw
DecodingError.dataCorruptedError if blank/missing), and for the .piSessionFile
branch ensure argvOption is not non-empty (if argvOption exists and
.trimmingCharacters... is not empty, throw a dataCorruptedError to reject
conflicting config). Ensure the default case still throws an unknown type error.
In `@Sources/SessionAgentPresentation.swift`:
- Around line 10-14: The current special-case in the switch branch for case
.registered(let id) returns a hardcoded localized "Pi" when id == "pi",
preventing any user-provided vault.agents[].name override; change the logic to
prefer the registry-provided name from
CmuxVaultAgentRegistry.load().registration(id: id)?.name first (for all ids or
at least for "pi") and only fall back to the localized "Pi" (or id) if no
registry name exists so that vault overrides are honored.
In `@Sources/SessionIndexStore.swift`:
- Around line 1446-1651: Move the entire registered-agent block into a new file
as an extension on SessionIndexStore (e.g.
Sources/SessionIndexStore+RegisteredAgents.swift): create `extension
SessionIndexStore { ... }` and paste the declarations
RegisteredAgentJSONLMetadata, loadRegisteredAgentEntries,
registeredSessionRoots, enumerateRegisteredJSONLCandidates,
extractRegisteredJSONLMetadata, firstString, and piCWDInferred there, preserving
the nonisolated and static behavior; change access-control so any extension
members that need to be callable from the original file are not marked private
(remove `private` so they are internal), while keeping types/vars that are only
used inside the new file private; do not alter logic, only move code and adjust
access modifiers to internal where cross-file access is required.
- Around line 1641-1651: The piCWDInferred(from:) helper builds a candidate path
by decoding a directory like "--body--" but currently returns it without
verifying it exists on disk, which can misattribute CWDs when segments contain
hyphens; modify nonisolated private static func piCWDInferred(from url: URL) to
construct the candidate ("/" + body.replacingOccurrences(of: "-", with: "/"))
and then call FileManager.default.fileExists(atPath:isDirectory:) (with a Bool
var) to ensure the candidate exists and is a directory—only return the candidate
when the check succeeds, otherwise return nil (mirroring the
decodeClaudeProjectDir pattern).
- Around line 711-716: defaultAgentOrder() currently calls
CmuxVaultAgentRegistry.load() without passing the project working directory,
which omits project-scoped agents; change defaultAgentOrder() to accept a
workingDirectory parameter (e.g. defaultAgentOrder(workingDirectory: URL?)) and
call CmuxVaultAgentRegistry.load(workingDirectory: workingDirectory) inside it,
then update all callers that have a cwd (notably loadDirectorySnapshot(cwd:),
searchSessions(.directory) branch, and any scanAll() path that can supply a cwd)
to pass their cwd through; keep the existing behavior for callers without a cwd
by passing nil so built-in-only behavior remains.
- Around line 1539-1555: registeredSessionRoots and
extractRegisteredJSONLMetadata currently special-case the "pi" id; instead add a
data-driven dispatch on CmuxVaultAgentRegistration (e.g., a new enum/property
like directoryEncoding or cwdFromPathPolicy on CmuxVaultAgentRegistration or
extend sessionIdSource) and implement a protocol/strategy (e.g.,
PiSessionLocator-conforming type) that exposes methods used now
(projectDirectoryName(for:) and piCWDInferred(from:)). Replace the id == "pi"
checks in registeredSessionRoots and extractRegisteredJSONLMetadata with calls
to registration.directoryEncoding.resolveProjectDirectory(for:cwdFilter, root:)
or registration.cwdPolicy.inferCWD(from:path) so behavior is driven by
registration metadata rather than hardcoded ids, and update registration
parsing/fixtures to set the new policy for Pi registrations.
- Around line 188-208: The fallback when RestorableAgentKind(rawValue:
registration.id) succeeds but AgentResumeCommandBuilder.resumeShellCommand(...)
returns nil must not synthesize a hardcoded "<defaultExecutable> --session
<sessionId>" but instead expand registration.resumeCommand using the same
template expansion used by customResumeArguments in RestorableAgentSession.swift
(substitute {{sessionId}}, {{sessionPath}}, {{executable}}, {{cwd}},
{{sessionDir}} etc.) so that registration.resumeCommand is honored; replace the
hardcoded return in that case with a call to the existing template expansion
helper (or extract a small helper that performs the template substitution if
needed) and return its result. Also remove inline provider-specific checks
(registration.id == "pi") from registeredSessionRoots and
extractRegisteredJSONLMetadata by converting that behavior into registration
metadata (e.g., projectDirectoryEncoding) or a Pi-specific helper and dispatch
based on metadata rather than literal id. Finally split the registered-agent
related types/functions (RegisteredAgentJSONLMetadata,
loadRegisteredAgentEntries, registeredSessionRoots,
enumerateRegisteredJSONLCandidates, extractRegisteredJSONLMetadata, firstString,
piCWDInferred) into a new file to satisfy the file-length budget and update
imports/usages accordingly.
In `@web/data/cmux.schema.json`:
- Around line 86-107: The schema for sessionIdSource should mirror the runtime
decoder CmuxVaultAgentSessionIDSource.init(from:) by requiring the "type"
property and enforcing "argvOption" when type == "argvOption"; update the
sessionIdSource object definition to include "required": ["type"] and add an
"if": { "properties": { "type": { "const": "argvOption" } } } with a
corresponding "then": { "required": ["argvOption"] } so invalid objects like {}
or { "type": "argvOption" } are rejected at validation time (keep existing
additionalProperties: false and the "type"/"argvOption" property specs).
🪄 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: 40f40bdf-e2c5-4c23-b38b-67c7e381138a
📒 Files selected for processing (11)
Resources/Localizable.xcstringsSources/CmuxConfig.swiftSources/CmuxTopSnapshot.swiftSources/RestorableAgentSession.swiftSources/RestorableAgentTypes.swiftSources/SessionAgentPresentation.swiftSources/SessionIndexStore.swiftSources/SessionIndexView.swiftcmuxTests/SessionPersistenceTests.swiftdocs/vault.mdweb/data/cmux.schema.json
The extensible Vault agent support had grown several already-large files, so this splits the new registry, process scanning, model, and JSONL indexing code into focused files while preserving the same runtime paths. Constraint: CI enforces Swift file-length budgets and the budget file should not be raised for this feature. Rejected: Refresh the Swift file-length budget | would accept avoidable debt instead of isolating coherent code. Confidence: high Scope-risk: narrow Directive: Keep future Vault agent integrations in the registry/scanner/index helper files instead of growing SessionIndexStore or RestorableAgentSession again. Tested: swiftc -frontend -parse on changed Swift surfaces; JSON schema/localization validation; project.pbxproj lint; Swift file-length budget guard; git diff --check Not-tested: Local XCTest execution per repository policy
Review feedback exposed a few places where the new registration path still behaved like a Pi-specific patch. This keeps detection, display names, session directory scoping, and resume generation driven by registration metadata while avoiding synchronous display-path config reads. Constraint: Local XCTest is disabled by repository policy, so this round uses static validation and CI for behavioral execution. Rejected: Keep processName as an early-success detector | it ignored configured argv constraints and broadened matches unexpectedly. Rejected: Reconstruct Pi cwd from encoded path without checking the filesystem | hyphenated project names make that inference lossy. Confidence: medium Scope-risk: moderate Directive: New Vault agents should add registration metadata or sessionIdSource behavior instead of literal agent-id checks in session indexing. Tested: swiftc -frontend -parse on changed Swift surfaces; JSON schema/localization validation; Swift file-length budget guard; git diff --check Not-tested: Local XCTest execution per repository policy
CircleCI failed without exposing logs through the public status script, and the previous review-fix commit introduced a static mutable display-name cache. This moves the cache to the existing shared-instance plus NSLock pattern used elsewhere in the app so Swift concurrency checks do not see unsafely shared mutable static state. Constraint: CircleCI log output is not available through the PR check helper or public CircleCI output endpoint. Rejected: Leave static mutable dictionary behind a static lock | Swift 6 still treats shared mutable static state as unsafe. Confidence: medium Scope-risk: narrow Directive: Keep synchronous display-name lookup in memory-only caches; registry disk reads belong off the main actor. Tested: swiftc -frontend -parse on affected Swift files; Swift file-length budget guard; git diff --check Not-tested: Local XCTest execution per repository policy
The tagged CI build showed RestorableAgentSession and SessionIndexStore could not see methods moved into the new Vault extension files. The build phase had entries, but the matching PBXFileReference records were missing, so Xcode omitted those files from compilation. Constraint: The only available failure evidence was the CI reload log; local app builds are reserved for the final mandated reload. Rejected: Move the extension methods back into the large files | that would regress the file-length guard fix. Confidence: high Scope-risk: narrow Directive: When splitting Swift files in the Xcode project, add both PBXBuildFile and PBXFileReference records before relying on project lint. Tested: project.pbxproj lint; git diff --check Not-tested: Local build/test per task constraints
Process detection already receives the global Vault registry, so it can merge project-local registrations lazily per working directory instead of re-reading every config for every observed process. The scanner now keeps a per-cwd registry cache and the registry exposes a focused project-config merge path. Constraint: Review feedback flagged synchronous registry loads inside the process scan loop. Rejected: Reload the complete registry for each process | preserves the latency issue and repeats global config reads. Confidence: high Scope-risk: narrow Directive: Keep process scanning on preloaded registries plus bounded per-cwd project overrides; do not add per-process config disk reads back to the loop. Tested: swiftc -frontend -parse for Sources and cmuxTests Swift files; JSON lint for Localizable.xcstrings and cmux.schema.json; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; swift_file_length_budget.py; git diff --check. Not-tested: Local XCTest and app launch before final mandated reload, per task instructions.
The per-cwd registry cache helper used the same name as the registry parameter, which made Swift resolve member accesses against the local function instead of the loaded registry. Renaming the helper keeps the cached project-config merge behavior and restores compilation. Constraint: CI compile failed in VaultAgentProcessScanner after the review-performance fix. Rejected: Revert the cache | would reintroduce repeated registry disk reads from the review finding. Confidence: high Scope-risk: narrow Directive: Avoid helper names that shadow high-value parameters in scanner code. Tested: swiftc -frontend -parse for Sources and cmuxTests Swift files; JSON lint for Localizable.xcstrings and cmux.schema.json; plutil -lint GhosttyTabs.xcodeproj/project.pbxproj; swift_file_length_budget.py; git diff --check. Not-tested: Local XCTest and local app build, per task instructions.
Review found that registered Vault agent support still had a few UI-path hazards: process detection ran synchronously from autosave, display names used a shared lock-backed cache, and registered session searches reread the registry per agent. This moves process detection behind a detached async load for autosave, carries registered display names as value data, reuses the already-loaded registry during search, and keeps JSONL parsing alive long enough to collect branch metadata. Constraint: Local tests are not run in this repository; CI owns XCTest and UI coverage. Rejected: Keep the display-name singleton with NSLock | leaves actor isolation unverifiable and was the current review blocker. Rejected: Make every session snapshot save async | broader call-site migration than needed for the autosave hot path. Confidence: high Scope-risk: moderate Directive: Do not put Vault process scans or registry config reads back into SwiftUI render or autosave main-actor paths. Tested: xcrun swiftc -frontend -parse on touched Swift files; git diff --check; python3 scripts/swift_file_length_budget.py; ./scripts/reload.sh --tag issue-3575-vault-followups. Not-tested: Local XCTest per repository policy.
austinywang
force-pushed
the
issue-3575-vault-pi-agent-support
branch
from
May 6, 2026 05:53
05435db to
670d0a5
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
3434-3503:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRevalidate autosave preconditions after the awaited index load.
The old flow checked the typing quiet-period and termination state immediately before saving. Now there's an
awaitin between, so the user can type again or shutdown can start whileloadIncludingProcessDetectedSnapshots()is in flight, and this method will still proceed with a scrollback-less autosave using a stalenow. That can defeat the quiet-period and race the shutdown snapshot.💡 Suggested fix
- let now = Date() `#if` DEBUG let fingerprintStart = ProcessInfo.processInfo.systemUptime `#endif` let restorableAgentIndex = await RestorableAgentSessionIndex.loadIncludingProcessDetectedSnapshots() + guard Self.shouldRunSessionAutosaveTick(isTerminatingApp: isTerminatingApp) else { + return + } + if let remainingQuietPeriod = remainingSessionAutosaveTypingQuietPeriod() { +#if DEBUG + cmuxDebugLog( + "session.save.skipped reason=typing_recent includeScrollback=0 source=\(source) " + + "retryMs=\(Int((remainingQuietPeriod * 1000).rounded()))" + ) +#endif + scheduleDeferredSessionAutosaveRetry(after: remainingQuietPeriod) + return + } + let now = Date() let autosaveFingerprint = sessionAutosaveFingerprint( includeScrollback: false, restorableAgentIndex: restorableAgentIndex )🤖 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/AppDelegate.swift` around lines 3434 - 3503, In finishSessionAutosaveTick, after awaiting RestorableAgentSessionIndex.loadIncludingProcessDetectedSnapshots() and before calling saveSessionSnapshot, revalidate the autosave preconditions by updating now = Date(), recomputing autosaveFingerprint via sessionAutosaveFingerprint(includeScrollback: false, restorableAgentIndex: restorableAgentIndex), and re-running Self.shouldSkipSessionAutosaveForUnchangedFingerprint(...) (using lastSessionAutosaveFingerprint, lastSessionAutosavePersistedAt, isTerminatingApp, etc.); if it returns true, log the skip (cmuxDebugLog as above) and return instead of proceeding to saveSessionSnapshot; preserve/update debug timing variables (fingerprintMs/saveMs) accordingly.
🤖 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 `@Sources/RestorableAgentSession.swift`:
- Around line 229-237: The current template expansion uses
templateParts.compactMap which drops any part that expands to an empty string,
causing option flags to be left without values; update the logic in
RestorableAgentSession.swift (the block using templateParts, replacements and
producing resolved) to fail-fast: iterate/map over templateParts applying
replacements and trimming, and if any resulting trimmed part is empty then
return nil for the whole expansion instead of removing that part; otherwise
return the full array of resolved parts (i.e., do not use compactMap that
filters out empty elements).
- Line 212: Change the function signature that currently returns an optional
collection (-> [String]?) to a non-optional array (-> [String]) and replace any
returns of nil inside that function with an empty array ([]); update any call
sites handling an optional (notably where resumeArguments is passed into
resumeShellCommand) to treat an empty array as the same "no args" case (remove
optional unwrapping or nil-coalescing). Ensure the function declaration and all
internal return paths use [String] and that callers like
resumeArguments/resumeShellCommand rely on empty-array semantics rather than
optionality.
In `@Sources/SessionIndexModels.swift`:
- Around line 217-236: In the .registered case do not fall back to returning the
raw template string registration.resumeCommand if
AgentResumeCommandBuilder.resumeShellCommand(...) fails; instead propagate the
failure (e.g., return nil or throw so the caller surfaces the validation error).
Update the case handling around AgentResumeCommandBuilder.resumeShellCommand (in
SessionIndexModels.swift) to remove the fallback to registration.resumeCommand
and ensure the caller of this switch (or the function) properly handles the
nil/throw result so invalid registrations don't produce literal templates passed
to the shell.
- Around line 73-89: The Codable implementation for SessionAgent currently
encodes only rawValue which loses RegisteredSessionAgent.name on round-trips;
update encode(to:) and init(from:) in the SessionAgent Codable conformance so
that when the enum case is .registered(RegisteredSessionAgent), you encode an
object containing both id and name (e.g., a keyed container with "id" and
optional "name") and when decoding reconstruct .registered by decoding that
keyed representation (falling back to RegisteredSessionAgent(id: decodedId,
name: nil) if name is absent); ensure the init(from:) still supports legacy
plain-string rawValue decoding to remain backward compatible with other cases.
In `@Sources/SessionIndexRegisteredAgents.swift`:
- Around line 67-84: The code currently hardcodes candidate.url.path as the
session id when creating SessionEntry, which breaks agents that supply a native
session id via sessionIdSource (e.g. argvOption). Change the builder to derive a
sessionId from the metadata/sessionIdSource (e.g. let sessionId =
metadata.sessionId ?? candidate.url.path) and then use that sessionId in both
the SessionEntry id string ("\(registration.id):\(sessionId)") and the sessionId
field instead of candidate.url.path so registered agents resume with their
native id.
In `@Sources/VaultAgentRegistry.swift`:
- Around line 337-345: The decodeConfig(at:fileManager:) currently swallows any
JSONC preprocessing or JSON decoding errors and returns nil; change it to catch
and surface errors instead of silently returning nil by wrapping
JSONCParser.preprocess and JSONDecoder().decode calls in do/catch, and on
failure log a warning that includes the path and the caught error (use the
project's logging utility or os_log/print if none), then return nil; reference
decodeConfig, JSONCParser.preprocess, JSONDecoder().decode, and CmuxConfigFile
when locating where to add the do/catch and the warning message.
- Around line 63-71: The DecodingError thrown when validating resumeCommand is
misleading because the guard accepts either "{{sessionId}}" or "{{sessionPath}}"
but the debugDescription only mentions "{{sessionId}}"; update the
debugDescription in the DecodingError.dataCorruptedError call (forKey:
.resumeCommand, in: container) to reference both accepted placeholders (e.g.,
mention "{{sessionId}} or {{sessionPath}}") so the error accurately reflects the
allowed templates for resumeCommand.
---
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 3434-3503: In finishSessionAutosaveTick, after awaiting
RestorableAgentSessionIndex.loadIncludingProcessDetectedSnapshots() and before
calling saveSessionSnapshot, revalidate the autosave preconditions by updating
now = Date(), recomputing autosaveFingerprint via
sessionAutosaveFingerprint(includeScrollback: false, restorableAgentIndex:
restorableAgentIndex), and re-running
Self.shouldSkipSessionAutosaveForUnchangedFingerprint(...) (using
lastSessionAutosaveFingerprint, lastSessionAutosavePersistedAt,
isTerminatingApp, etc.); if it returns true, log the skip (cmuxDebugLog as
above) and return instead of proceeding to saveSessionSnapshot; preserve/update
debug timing variables (fingerprintMs/saveMs) accordingly.
🪄 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: 7f7df1b3-164e-46d4-873f-0f4e27cff014
📒 Files selected for processing (17)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/CmuxConfig.swiftSources/CmuxTopProcessArguments.swiftSources/CmuxTopSnapshot.swiftSources/RestorableAgentSession.swiftSources/RestorableAgentTypes.swiftSources/SessionAgentPresentation.swiftSources/SessionIndexModels.swiftSources/SessionIndexRegisteredAgents.swiftSources/SessionIndexStore.swiftSources/VaultAgentProcessScanner.swiftSources/VaultAgentRegistry.swiftcmuxTests/PiVaultAgentPersistenceTests.swiftcmuxTests/SessionPersistenceTests.swiftdocs/vault.mdweb/data/cmux.schema.json
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (7)
Sources/VaultAgentRegistry.swift (2)
65-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDecode error mentions only
{{sessionId}}but the guard also accepts{{sessionPath}}.The guard accepts either placeholder, but the
debugDescriptiononly mentions{{sessionId}}, so users get misleading feedback when their template uses{{sessionPath}}.🛠️ Proposed fix
throw DecodingError.dataCorruptedError( forKey: .resumeCommand, in: container, - debugDescription: "Vault agent resumeCommand must include {{sessionId}}" + debugDescription: "Vault agent resumeCommand must include {{sessionId}} or {{sessionPath}}" )🤖 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/VaultAgentRegistry.swift` around lines 65 - 72, The DecodingError debug message for the resumeCommand guard is misleading because the guard allows either "{{sessionId}}" or "{{sessionPath}}" but the DecodingError.dataCorruptedError debugDescription only mentions "{{sessionId}}"; update the error text in the throw inside the guard (the DecodingError.dataCorruptedError call forKey: .resumeCommand, in: container) to reference both placeholders (e.g., require "{{sessionId}}" or "{{sessionPath}}") so users receive accurate feedback when the resumeCommand uses "{{sessionPath}}".
337-345:⚠️ Potential issue | 🟠 Major | ⚡ Quick winSilently swallowing config decode errors makes broken
cmux.jsonfiles invisible.Both
JSONCParser.preprocessandJSONDecoder().decodefailures are mapped tonilhere. For a config-driven restore path, a typo incmux.jsonwill silently make custom Vault registrations disappear with no diagnostic. Catch the errors and surface a warning that includes the file path so misconfiguration is debuggable.🛠️ Proposed fix
private static func decodeConfig(at path: String, fileManager: FileManager) -> CmuxConfigFile? { guard fileManager.fileExists(atPath: path), let data = fileManager.contents(atPath: path), - !data.isEmpty, - let sanitized = try? JSONCParser.preprocess(data: data) else { + !data.isEmpty else { return nil } - return try? JSONDecoder().decode(CmuxConfigFile.self, from: sanitized) + do { + let sanitized = try JSONCParser.preprocess(data: data) + return try JSONDecoder().decode(CmuxConfigFile.self, from: sanitized) + } catch { + NSLog("[Vault] Failed to decode cmux config at %@: %@", path, String(describing: error)) + return nil + } }🤖 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/VaultAgentRegistry.swift` around lines 337 - 345, The decodeConfig(at:fileManager:) helper is swallowing both JSONCParser.preprocess and JSONDecoder().decode errors and returning nil, which hides malformed cmux.json; update decodeConfig to explicitly do-try-catch around JSONCParser.preprocess and JSONDecoder().decode (referencing JSONCParser.preprocess, JSONDecoder().decode and CmuxConfigFile) and when either throws log a warning that includes the path and the caught error details so misconfigured files are visible, then return nil on error; keep the existing file existence/data checks and only attempt parsing inside the do-catch blocks.Sources/SessionIndexRegisteredAgents.swift (1)
73-86:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
sessionIdis hardcoded tocandidate.url.pathand ignoresregistration.sessionIdSource.Storing the JSONL file path as the session id is only correct for
piSessionFile-style sources. For a registration whosesessionIdSource == .argvOption(...), the native session id should be parsed from the JSONL contents (e.g. anid/sessionId/session_idfield) and used both here and in theSessionEntry.idcomposite. As written, any non-Pi registered agent will resume with the JSONL path passed where its CLI expects a real session id.🛠️ Sketch
Add a metadata field for the native session id and prefer it over the file path:
private struct RegisteredAgentJSONLMetadata { var title: String = "" var cwd: String? var branch: String? + var sessionId: String? }Then in
extractRegisteredJSONLMetadata, capturefirstString(in: object, keys: ["sessionId", "session_id", "id"])and:- matches.append(SessionEntry( - id: "\(registration.id):\(candidate.url.path)", + let resolvedSessionId = metadata.sessionId ?? candidate.url.path + matches.append(SessionEntry( + id: "\(registration.id):\(resolvedSessionId)", agent: .registered(RegisteredSessionAgent(registration: registration)), - sessionId: candidate.url.path, + sessionId: resolvedSessionId,🤖 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/SessionIndexRegisteredAgents.swift` around lines 73 - 86, The SessionEntry is using candidate.url.path as sessionId and in the composite id, which ignores registration.sessionIdSource; update the code to prefer a native session id extracted from the JSONL metadata (add a metadata field like nativeSessionId in whatever struct extractRegisteredJSONLMetadata fills) and use that nativeSessionId when present for both SessionEntry.sessionId and the SessionEntry.id composite (fall back to candidate.url.path only if nativeSessionId is nil). Modify extractRegisteredJSONLMetadata to call firstString(in:object, keys:["sessionId","session_id","id"]) and populate the new nativeSessionId, and ensure RegisteredSessionAgent/SessionEntry construction uses that new field instead of always using candidate.url.path.Sources/RestorableAgentSession.swift (2)
229-237:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
compactMapsilently drops empty-expanded template parts, producing malformed argv.For a template like
pi --session {{sessionId}} --session-dir {{sessionDir}}, whensessionDiris unset (noregistration.sessionDirectory),{{sessionDir}}collapses to""and gets filtered out, leaving["pi", "--session", "<id>", "--session-dir"]— an option flag missing its required value, then concatenated and sent to the shell.Fail closed when any placeholder yields an empty value rather than emitting a corrupt command.
🛠️ Proposed fix
- let resolved = templateParts.compactMap { part -> String? in + var resolved: [String] = [] + for part in templateParts { var value = part for (key, replacement) in replacements { value = value.replacingOccurrences(of: "{{\(key)}}", with: replacement) } let trimmed = value.trimmingCharacters(in: .whitespacesAndNewlines) - return trimmed.isEmpty ? nil : trimmed + // If a template part contained a placeholder that resolved to "", + // abort entirely rather than emitting an option flag without its value. + guard !trimmed.isEmpty else { return nil } + resolved.append(trimmed) } return resolved.isEmpty ? nil : resolved🤖 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/RestorableAgentSession.swift` around lines 229 - 237, The code in the template expansion (using templateParts.compactMap) silently drops parts that expand to empty strings, corrupting argv; change the logic in the expansion loop (the block that builds resolved from templateParts and replacements) to use map (or keep all parts) and detect any part that contained a placeholder but trimmed to empty — if any replacement yields an empty trimmed value, return nil (fail closed) rather than filtering it out; reference the templateParts/replacements loop and the resolved return so the function returns nil when any placeholder expansion is empty.
212-214: 🧹 Nitpick | 🔵 Trivial | 💤 Low valuePrefer
[String]over[String]?to satisfydiscouraged_optional_collection.SwiftLint flags this signature, and an empty array and
nilare semantically equivalent here (both makeresumeArguments→resumeShellCommandreturnnilvia the!argv.isEmptyguard).🛠️ Proposed fix
- private static func customResumeArguments( + private static func customResumeArguments( registration: CmuxVaultAgentRegistration, sessionId: String, launchCommand: AgentLaunchCommandSnapshot? - ) -> [String]? { + ) -> [String] { let templateParts = splitShellWords(registration.resumeCommand) - guard !templateParts.isEmpty else { return nil } + guard !templateParts.isEmpty else { return [] } ... - return resolved.isEmpty ? nil : resolved + return resolved }🤖 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/RestorableAgentSession.swift` around lines 212 - 214, Change the function signature from returning [String]? to a non-optional [String] and return an empty array instead of nil; specifically, in the function that calls splitShellWords(registration.resumeCommand) replace "guard !templateParts.isEmpty else { return nil }" with a non-optional-friendly return (e.g., "guard !templateParts.isEmpty else { return [] }" or simply return templateParts) and update the signature to -> [String]; ensure call sites like resumeArguments and resumeShellCommand (which already gate on !argv.isEmpty) continue to work with the empty-array semantics so no further behavioral changes are needed.Sources/SessionIndexModels.swift (2)
217-237:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't fall back to the unresolved template string.
When
AgentResumeCommandBuilder.resumeShellCommandreturnsnil, line 236 returnsregistration.resumeCommand— the raw template (e.g.pi --session {{sessionId}}). That string then getscd && ...wrapped byresumeCommandWithCwdand shipped to a real shell, turning a bad registration / missing context into a broken resume rather than surfacing the failure.🛠️ Proposed fix
case .registered(let registration): - if let command = AgentResumeCommandBuilder.resumeShellCommand( + return AgentResumeCommandBuilder.resumeShellCommand( kind: .custom(registration.id), sessionId: sessionId, launchCommand: AgentLaunchCommandSnapshot( launcher: registration.id, executablePath: nil, arguments: [registration.defaultExecutable], workingDirectory: cwd, environment: nil, capturedAt: nil, source: "vault" ), workingDirectory: nil, registrationOverride: registration, includeWorkingDirectoryPrefix: false - ) { - return command - } - return registration.resumeCommand + ) ?? "" }(Or, better, propagate the failure by making
resumeCommandoptional so callers can skip emitting a broken resume entirely.)🤖 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/SessionIndexModels.swift` around lines 217 - 237, The current code falls back to returning the unresolved template string registration.resumeCommand when AgentResumeCommandBuilder.resumeShellCommand(...) returns nil, which produces a broken resume command; change the control flow so that when resumeShellCommand returns nil you do NOT return registration.resumeCommand but instead propagate the failure (e.g., make the containing method return an optional and return nil, or make registration.resumeCommand optional and return nil) so callers can skip emitting a broken resume; update all callers to handle the optional result; relevant symbols: AgentResumeCommandBuilder.resumeShellCommand, registration.resumeCommand, and the switch/case that currently returns registration.resumeCommand.
87-90:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEncoding only
rawValuedropsRegisteredSessionAgent.nameacross Codable round-trips.
encode(to:)writes justrawValue(the agent id), andinit(rawValue:)always reconstructs.registered(RegisteredSessionAgent(id: value))withname == nil. Any persistence/IPC round-trip of a non-built-in registered agent loses the configured display name and the UI falls back to the raw id.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/SessionIndexModels.swift` around lines 87 - 90, encode(to:) currently writes only rawValue (the agent id), which drops RegisteredSessionAgent.name during Codable round-trips; update the Codable implementation so encode(to:) encodes both the id and optional name (e.g., via a keyed container like keys "id" and "name" or by encoding RegisteredSessionAgent directly) and update init(from:) to decode those same keys and reconstruct .registered(RegisteredSessionAgent(id: decodedId, name: decodedName)). Ensure uses of rawValue remain for built-ins but preserve RegisteredSessionAgent.name when present (referenced symbols: encode(to:), init(from:), RegisteredSessionAgent, rawValue, name).
🤖 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 `@Sources/RestorableAgentSession.swift`:
- Around line 27-32: The current logic always calls
CmuxVaultAgentRegistry.load(...) when registrationOverride is nil, causing
unnecessary filesystem walks for built-in kinds; change it to only call
CmuxVaultAgentRegistry.load when a custom agent ID might be needed by first
checking kind.customAgentID (or registrationOverride) before invoking load.
Concretely, update the construction of registry/customRegistration so that you
defer or guard the call to CmuxVaultAgentRegistry.load(...) — e.g., compute
whether kind.customAgentID != nil (or registrationOverride == nil &&
kind.customAgentID != nil) and only then call CmuxVaultAgentRegistry.load(...)
to populate registry and resolve customRegistration (referencing
registrationOverride, kind.customAgentID, CmuxVaultAgentRegistry.load, and
customRegistration).
In `@Sources/SessionAgentPresentation.swift`:
- Around line 22-24: The registered case currently returns the hardcoded
"AgentIcons/OpenCode" which mislabels vault-registered agents (e.g., Pi); update
the data flow so the UI resolves an icon from the registration instead of always
using OpenCode: add an iconAssetName (or similar) property to
CmuxVaultAgentRegistration and change SessionAgentPresentation's assetName
resolution for the .registered case to read registration.iconAssetName (falling
back to a neutral generic asset like "person.crop.circle" or a shipped Pi asset
only when id == "pi" and no user override exists); ensure the registry lookup
path that yields a CmuxVaultAgentRegistration is updated to populate
iconAssetName so assetName no longer returns the hardcoded
"AgentIcons/OpenCode".
In `@Sources/SessionIndexModels.swift`:
- Around line 24-26: Replace the localized string usage for the Pi brand with a
direct literal: instead of returning String(localized: "sessionIndex.agent.pi",
defaultValue: "Pi") when id == "pi", return the brand literal "Pi" directly so
the agent displayName is not treated as translatable text; update the branch
that checks id == "pi" (the conditional containing the String(localized:) call)
to return "Pi".
In `@Sources/SessionIndexStore.swift`:
- Around line 1067-1078: When handling .agent(let a) where a is .registered, you
must fetch the vault registry and perform the search with the caller's
current/project working directory instead of passing workingDirectory: nil and
cwdFilter: nil; update the branch that creates registry via
Self.vaultAgentRegistry(workingDirectory: nil) to use the appropriate
currentDirectory (or the entry's cwd) and pass that directory through as
cwdFilter to the call to
Self.searchAgent(needle:agent:cwdFilter:offset:limit:errorBag:registry:) so
project-scoped registrations are found (mirror how the .directory branch threads
cwdFilter into defaultAgentOrder/loadRegisteredAgentEntries).
---
Duplicate comments:
In `@Sources/RestorableAgentSession.swift`:
- Around line 229-237: The code in the template expansion (using
templateParts.compactMap) silently drops parts that expand to empty strings,
corrupting argv; change the logic in the expansion loop (the block that builds
resolved from templateParts and replacements) to use map (or keep all parts) and
detect any part that contained a placeholder but trimmed to empty — if any
replacement yields an empty trimmed value, return nil (fail closed) rather than
filtering it out; reference the templateParts/replacements loop and the resolved
return so the function returns nil when any placeholder expansion is empty.
- Around line 212-214: Change the function signature from returning [String]? to
a non-optional [String] and return an empty array instead of nil; specifically,
in the function that calls splitShellWords(registration.resumeCommand) replace
"guard !templateParts.isEmpty else { return nil }" with a non-optional-friendly
return (e.g., "guard !templateParts.isEmpty else { return [] }" or simply return
templateParts) and update the signature to -> [String]; ensure call sites like
resumeArguments and resumeShellCommand (which already gate on !argv.isEmpty)
continue to work with the empty-array semantics so no further behavioral changes
are needed.
In `@Sources/SessionIndexModels.swift`:
- Around line 217-237: The current code falls back to returning the unresolved
template string registration.resumeCommand when
AgentResumeCommandBuilder.resumeShellCommand(...) returns nil, which produces a
broken resume command; change the control flow so that when resumeShellCommand
returns nil you do NOT return registration.resumeCommand but instead propagate
the failure (e.g., make the containing method return an optional and return nil,
or make registration.resumeCommand optional and return nil) so callers can skip
emitting a broken resume; update all callers to handle the optional result;
relevant symbols: AgentResumeCommandBuilder.resumeShellCommand,
registration.resumeCommand, and the switch/case that currently returns
registration.resumeCommand.
- Around line 87-90: encode(to:) currently writes only rawValue (the agent id),
which drops RegisteredSessionAgent.name during Codable round-trips; update the
Codable implementation so encode(to:) encodes both the id and optional name
(e.g., via a keyed container like keys "id" and "name" or by encoding
RegisteredSessionAgent directly) and update init(from:) to decode those same
keys and reconstruct .registered(RegisteredSessionAgent(id: decodedId, name:
decodedName)). Ensure uses of rawValue remain for built-ins but preserve
RegisteredSessionAgent.name when present (referenced symbols: encode(to:),
init(from:), RegisteredSessionAgent, rawValue, name).
In `@Sources/SessionIndexRegisteredAgents.swift`:
- Around line 73-86: The SessionEntry is using candidate.url.path as sessionId
and in the composite id, which ignores registration.sessionIdSource; update the
code to prefer a native session id extracted from the JSONL metadata (add a
metadata field like nativeSessionId in whatever struct
extractRegisteredJSONLMetadata fills) and use that nativeSessionId when present
for both SessionEntry.sessionId and the SessionEntry.id composite (fall back to
candidate.url.path only if nativeSessionId is nil). Modify
extractRegisteredJSONLMetadata to call firstString(in:object,
keys:["sessionId","session_id","id"]) and populate the new nativeSessionId, and
ensure RegisteredSessionAgent/SessionEntry construction uses that new field
instead of always using candidate.url.path.
In `@Sources/VaultAgentRegistry.swift`:
- Around line 65-72: The DecodingError debug message for the resumeCommand guard
is misleading because the guard allows either "{{sessionId}}" or
"{{sessionPath}}" but the DecodingError.dataCorruptedError debugDescription only
mentions "{{sessionId}}"; update the error text in the throw inside the guard
(the DecodingError.dataCorruptedError call forKey: .resumeCommand, in:
container) to reference both placeholders (e.g., require "{{sessionId}}" or
"{{sessionPath}}") so users receive accurate feedback when the resumeCommand
uses "{{sessionPath}}".
- Around line 337-345: The decodeConfig(at:fileManager:) helper is swallowing
both JSONCParser.preprocess and JSONDecoder().decode errors and returning nil,
which hides malformed cmux.json; update decodeConfig to explicitly do-try-catch
around JSONCParser.preprocess and JSONDecoder().decode (referencing
JSONCParser.preprocess, JSONDecoder().decode and CmuxConfigFile) and when either
throws log a warning that includes the path and the caught error details so
misconfigured files are visible, then return nil on error; keep the existing
file existence/data checks and only attempt parsing inside the do-catch blocks.
🪄 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: 1638625a-a642-4864-8569-6dd0ef14eb2d
📒 Files selected for processing (8)
Sources/AppDelegate.swiftSources/RestorableAgentSession.swiftSources/SessionAgentPresentation.swiftSources/SessionIndexModels.swiftSources/SessionIndexRegisteredAgents.swiftSources/SessionIndexStore.swiftSources/VaultAgentRegistry.swiftcmuxTests/PiVaultAgentPersistenceTests.swift
CircleCI macOS unit tests failed before reaching XCTest because the Zig tarball download was reset by the remote peer. The shared install-zig command now uses curl retries for both the archive and signature downloads while preserving minisign verification.
Constraint: CircleCI failed in dependency bootstrap with curl exit 56 before code or tests ran
Rejected: Empty retry commit | would retrigger CI without improving the flaky bootstrap path
Confidence: high
Scope-risk: narrow
Directive: Keep signature verification after download retries; retries should not bypass minisign
Tested: git diff --check
Tested: ruby YAML.load_file('.circleci/config.yml')
Not-tested: local Xcode build/tests per repository policy and user xcodebuild restriction
Both required CI surfaces failed before build/test execution because the Zig tarball download was reset by the remote peer. Direct Zig downloads across GitHub workflows now retry transient curl failures and resume partial archives; CircleCI uses the same resume behavior inside its shared install-zig command. Constraint: Local xcodebuild is prohibited and the failures occurred in remote dependency bootstrap Rejected: Patch activation only | would leave the same reset-prone Zig download in other CI entrypoints Confidence: high Scope-risk: narrow Directive: Keep retry/resume flags on every direct ziglang.org tarball download unless replacing them with a shared installer Tested: git diff --check Tested: ruby YAML.load_file for changed workflow configs Not-tested: local Xcode build/tests per repository policy and user xcodebuild restriction
The retry/resume path kept activation alive but the transfer stayed stuck inside Install zig for an extended period. Direct Zig downloads now treat sustained sub-1KB/s transfers as transient failures, allowing curl to retry and resume instead of holding macOS runners indefinitely. Constraint: Activation CI remained in Install zig after the reset-prone download path was hardened Rejected: Wait for runner timeout | provides no new code evidence and delays the same stalled transfer Confidence: high Scope-risk: narrow Directive: Keep speed-limit, retry, and continue-at flags together for direct Zig tarball downloads Tested: git diff --check Tested: ruby YAML.load_file for changed workflow configs Not-tested: local Xcode build/tests per repository policy and user xcodebuild restriction
Custom Vault resume templates now replace placeholders by scanning the original token once instead of mutating through dictionary iteration. Placeholder-looking text inside session ids, cwd values, or other replacement data remains literal and cannot be expanded by a later replacement pass. Constraint: Cursor review found dictionary iteration could make replacement expansion order-dependent Rejected: Ordered dictionary iteration only | deterministic but still allows replacement values to be recursively expanded Confidence: high Scope-risk: narrow Directive: Do not reintroduce sequential replacingOccurrences over a mutable template token for registered resume templates Tested: git diff --check Tested: added XCTest coverage for literal placeholder text inside sessionId replacement values Not-tested: local XCTest/build per repository policy and user xcodebuild restriction
The direct curl flags still allowed macOS CI to lose a nearly-complete Zig archive when ziglang.org reset the connection. A shared downloader now bounds each attempt, resumes partial output with --continue-at, and retries explicitly so GitHub Actions and CircleCI use the same recovery behavior. Constraint: Activation was cancelled in Install zig after 30 minutes and CircleCI release reset at roughly 95% of the Zig archive Rejected: Rely on curl --retry alone | observed CircleCI did not recover the reset transfer Confidence: high Scope-risk: moderate Directive: Keep direct Zig archive downloads routed through scripts/download-with-retry.sh unless replacing the installer with a cached artifact Tested: bash -n scripts/download-with-retry.sh Tested: git diff --check Tested: ruby YAML.load_file for changed workflow configs Not-tested: local Xcode build/tests per repository policy and user xcodebuild restriction
The PR branch was merged with origin/main before the next update. YAML workflow conflicts were resolved by taking origin/main exactly as requested; the remaining Swift test conflict keeps main's Pi environment case and the branch's empty environment grouping for cursor, rovodev, factory, and custom agents. Constraint: User required merging origin/main before PR updates Constraint: YAML conflicts must take origin/main and no YAML files may be manually edited Confidence: high Scope-risk: moderate Tested: git diff --name-only --diff-filter=U returned no paths Tested: conflicted YAML files match origin/main Tested: git diff --cached --check Not-tested: local XCTest/build per repository policy and user xcodebuild restriction
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 204025a. Configure here.
origin/main added Pi as a native restorable kind while this branch intentionally keeps Pi registry-owned for Vault discovery and project overrides. The merged enum therefore needed to keep direct .pi values encodable without adding Pi to the hook-kind allCases list. Constraint: Direct xcodebuild is forbidden; CI activation uses the tagged reload script. Constraint: YAML files must not be edited in this iteration. Rejected: Add Pi back to RestorableAgentKind.allCases | that would bypass the registry-owned Pi hook path and block project overrides. Confidence: high Scope-risk: narrow Directive: Keep Pi out of RestorableAgentKind.allCases while Vault owns the default Pi registration. Tested: git diff --check Not-tested: Local build/tests, per repo policy and no direct xcodebuild constraint.
Resume command construction should not discover Vault config by walking the filesystem on demand. Restorable snapshots now carry the registration resolved at registry load or process-detection time, so custom resume templates remain available without main-thread disk reads. Constraint: Review feedback flagged synchronous registry loading during restore and production NSLog usage. Constraint: YAML files are off-limits for this iteration. Rejected: Keep lazy CmuxVaultAgentRegistry.load in resumeShellCommand | preserves legacy fallback but keeps disk IO in the restore hot path. Confidence: high Scope-risk: moderate Directive: Custom restorable agents must receive a registration before resume command construction; do not reintroduce fallback disk reads in AgentResumeCommandBuilder. Tested: git diff --check Not-tested: Local build/tests, per repo policy and no direct xcodebuild constraint.
origin/main adds Hermes Agent as a built-in provider while this branch keeps Pi and custom agents registry-owned through Vault. The merge keeps Hermes in the built-in session/search/resume switches, keeps dynamic Vault agents on the registered path, and restores explicit pi decoding so legacy direct Pi values do not collapse into custom registrations. Constraint: PR branch must be merged with current origin/main before updating the PR. Constraint: YAML workflow files were not edited during this merge. Rejected: Treat Hermes as a Vault registration | Hermes landed on main as a built-in provider with dedicated index and hook support. Confidence: medium Scope-risk: moderate Directive: Keep registry-owned agents out of built-in allCases when project config must be able to override them. Tested: git diff --cached --check Not-tested: Local tests per repository policy; CI will validate after push.
The autosave tick sets its in-flight guard before crossing into an async MainActor task. Keeping the AppDelegate alive through that handoff makes the task always reach finishSessionAutosaveTick, where the existing defer clears the guard in debug and release builds. Constraint: Greptile flagged the weak-self early return as a path that could strand sessionAutosaveTickInFlight. Rejected: Add a guard-else reset | there is no instance to mutate when weak self is nil; removing the nil path preserves the invariant directly. Confidence: high Scope-risk: narrow Tested: git diff --check Not-tested: Local tests per repository policy; CI will validate after push.
origin/main now adds settings, event, process-detail, and test-target changes. The only manual conflict resolution was in the Xcode project test target membership, where both PiVaultAgentPersistenceTests and AgentSessionAutoResumeSettingsTests need to stay in the cmuxTests group and sources phase. Constraint: Merge origin/main into issue-3575-vault-pi-agent-support before continuing PR work. Constraint: No .yml/.yaml conflicts were edited. Rejected: Taking either side of project.pbxproj test membership | would drop a valid test file from one branch. Confidence: high Scope-risk: moderate Tested: git diff --cached --check Not-tested: Local tests per repository policy; final verification uses the required reload script.
The merge with origin/main pushed GhosttyTerminalView and AppDelegate over the CI file-length guard. Move TerminalSurface debug metadata accessors into the existing small support file and compact the autosave task handoff so the guard passes without editing workflow YAML or increasing the budget. Constraint: CI failed workflow-guard-tests on scripts/swift_file_length_budget.py. Constraint: Do not edit .yml/.yaml files or xcodebuild directly. Rejected: Refreshing .github/swift-file-length-budget.tsv | accepts debt instead of reducing the over-budget files. Confidence: high Scope-risk: narrow Tested: python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv Tested: git diff --check Not-tested: Local app/test build per repository policy; final verification uses CI and required reload script.
This was referenced May 12, 2026
Merged
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
pi --session <path-or-id>Fixes #3575.
Verification
Local tests were not run per repository task instruction; CI should validate the regression test/fix pair.
Note
Medium Risk
Adds new Vault agent registration, process-detection, and resume-command generation paths that affect session discovery and restore behavior; failures could prevent resuming sessions or mis-detect running agents.
Overview
Adds config-driven Vault agent registrations via new
vault.agentsincmux.json(schema +docs/vault.md), with Pi registered by default and localized.Vault can now detect running registered agents from cmux-scoped processes (reading argv/env via
sysctl) and persist snapshots with the resolved registration so restores can generate targeted resume commands.Extends the session index to list and resume registered/JSONL-backed agent sessions, including optional icons and safer UI behavior (only show copy/resume when a command is available), plus small concurrency cleanup around autosave and drag registry.
Reviewed by Cursor Bugbot for commit afe4fea. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds a config-driven Vault agent registry with Pi enabled by default and targeted
--sessionrestores for JSONL-backed agents. Improves detection, indexing, and resume flows while keeping Hermes Agent built-in. Addresses #3575.New Features
vault.agentsincmux.json(schema +docs/vault.md) with placeholders{{sessionId}},{{sessionPath}},{{executable}},{{cwd}},{{sessionDir}}; Pi registered by default.--session-dir, project folders), build resume commands from templates, and list JSONL sessions with names/icons; copy/resume only when a valid command exists.Refactors
SessionAgent.registered,RestorableAgentKind.custom); snapshots carry resolved registrations; templates are non-recursive; process scans run async off autosave and reuse per-CWD registries; autosave guard preserved across async handoff.Written for commit afe4fea. Summary will update on new commits.
Summary by CodeRabbit
New Features
Localization
Documentation
Tests
Bug Fixes