Repository navigation
Add OMP hook installation and session restore - #5413
Merged
austinywang merged 7 commits intoJun 5, 2026
Merged
Conversation
|
@joshrzemien is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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 OMP agent support: embed OMP TypeScript extension with Swift install/uninstall and CLI wiring; register OMP in Vault and TaskManager; parse OMP session selectors and prefer OMP session roots from env; extend agent-launch sanitizer/resume argv; add unit/integration tests, docs, localization, CI, and Xcode wiring. ChangesOMP Integration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
✅ Passed checks (15 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
.github/workflows/ci.yml (1)
205-462:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd explicit least-privilege
permissionsfor the workflow token.Line 205 onward runs many jobs without a
permissions:block, soGITHUB_TOKENkeeps default broad scopes. Please pin permissions explicitly (for most jobs,contents: read) and grant write scopes only to the specific release/upload jobs that need them.Suggested hardening
name: CI +permissions: + contents: read🤖 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 @.github/workflows/ci.yml around lines 205 - 462, Summary: workflow missing explicit least-privilege permissions for GITHUB_TOKEN in the "tests" job; add a permissions block. Update the "tests" job definition to include an explicit permissions: block (e.g., permissions: contents: read) so token scope is least-privilege for the many steps run under the tests job, and then add/adjust permissions only on jobs that require write access (release/upload jobs) to grant specific write scopes (e.g., contents: write, packages: write) instead of broad defaults; locate and modify the job named "tests" and the release/upload job(s) that perform publishing to set the minimal required scopes.
🤖 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`+AgentHookDefinitions.swift:
- Around line 196-203: The AgentHookDef for "omp" currently lacks
configDirEnvOverride, causing resolvedConfigDir() to disagree with installer
behavior in CMUXCLI+OmpExtension.swift which uses PI_CODING_AGENT_DIR or
PI_CONFIG_DIR/agent; add the appropriate configDirEnvOverride to the
AgentHookDef for name "omp" (matching the env var(s) used by
CMUXCLI+OmpExtension.swift) so AgentHookDef.resolvedConfigDir() and the
installer use the same override-aware path resolution logic.
In `@CLI/CMUXCLI`+OmpExtension.swift:
- Around line 168-177: The ompAgentDirectory() resolver currently treats
PI_CODING_AGENT_DIR and PI_CONFIG_DIR as plain strings and always appends
PI_CONFIG_DIR under $HOME; update it to normalize and respect absolute/tilde
paths: in ompAgentDirectory(), when
nonEmptyEnvironmentValue("PI_CODING_AGENT_DIR", ...) returns a value, expand
tilde (use NSString().expandingTildeInPath) and if the result is an absolute
path use it directly as the agent directory; otherwise treat it as a relative
path as before. For PI_CONFIG_DIR, expand tilde and if the expanded value is
absolute, use that path instead of appending it to home; only append when the
config dir is a relative path. Refer to ompAgentDirectory() and
nonEmptyEnvironmentValue(...) when applying these fixes.
In
`@Packages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchSanitizerTests.swift`:
- Around line 416-443: Extend the
dropsOmpSessionSelectorsWithoutReplayingPrompts test to also assert that
AgentLaunchSanitizer.sanitizedLaunchArguments removes "--session" selectors
(both "--session", "--session=old-session" and combined forms) the same way it
handles "-r" and "--resume"; add expectations calling
AgentLaunchSanitizer.sanitizedLaunchArguments (with launcher: "omp",
fallbackKind: "omp") that input arrays containing "--session",
"--session=old-session" (and a case where "--session" is followed by the session
id as a separate argument) result in the sanitized array without any session
selector and without replaying the original prompt or session id.
In `@Sources/VaultAgentProcessScanner.swift`:
- Around line 1004-1008: The current sessionRoot precedence wrongly calls
ompAgentSessionsRoot(for:registration:) before honoring
registration.sessionDirectory and always synthesizes an OMP path; change the
precedence so registration.sessionDirectory is checked before calling
ompAgentSessionsRoot, and modify ompAgentSessionsRoot usage so it is only
invoked when an explicit OMP override/environment/flag is present (e.g., when
process.arguments.value(afterOption: "--session-dir") or
process.environment["PI_CODING_AGENT_SESSION_DIR"] indicates OMP should be
used); ensure sessionRoot falls back to defaultSessionsRoot() only after
registration.sessionDirectory and explicit omp override are absent, and update
any similar logic around omp handling in the same block (the code that
references sessionRoot, ompAgentSessionsRoot(for:registration:),
registration.sessionDirectory, process.arguments.value(afterOption:),
process.environment["PI_CODING_AGENT_SESSION_DIR"], and defaultSessionsRoot())
to preserve configured registration.sessionDirectory overrides.
---
Outside diff comments:
In @.github/workflows/ci.yml:
- Around line 205-462: Summary: workflow missing explicit least-privilege
permissions for GITHUB_TOKEN in the "tests" job; add a permissions block. Update
the "tests" job definition to include an explicit permissions: block (e.g.,
permissions: contents: read) so token scope is least-privilege for the many
steps run under the tests job, and then add/adjust permissions only on jobs that
require write access (release/upload jobs) to grant specific write scopes (e.g.,
contents: write, packages: write) instead of broad defaults; locate and modify
the job named "tests" and the release/upload job(s) that perform publishing to
set the minimal required scopes.
🪄 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: 2fe67c09-e701-4df5-8024-21caa93c17dd
📒 Files selected for processing (25)
.github/workflows/ci.ymlCLI/CMUXCLI+AgentHookDefinitions.swiftCLI/CMUXCLI+OmpExtension.swiftCLI/cmux.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchEnvironmentPolicy.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizer.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerPrimaryPolicies.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeArgv.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchEnvironmentPolicyTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchSanitizerTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeArgvTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeWorkingDirectoryTests.swiftResources/Localizable.xcstringsSources/TaskManagerTypes.swiftSources/VaultAgentProcessScanner.swiftSources/VaultAgentRegistry.swiftcmux.xcodeproj/project.pbxprojcmuxTests/OmpSupportTests.swiftcmuxTests/PiVaultAgentPersistenceTests.swiftcmuxTests/TaskManagerResourcesTests.swiftdocs/agent-hooks.mddocs/feed.mddocs/vault.mdtests/test_omp_extension_install.pyweb/app/[locale]/docs/session-restore/page.tsx
Contributor
Greptile SummaryThis PR adds first-class OMP support to cmux: a
Confidence Score: 4/5Safe to merge after addressing the raw error-detail leak in the install/uninstall path. The only issue in new production code is that existingOmpExtensionContents re-throws String(describing: error) verbatim into user-visible CLI output, exposing NSCocoaErrorDomain codes and UserInfo keys. Swapping to error.localizedDescription is a one-line fix. Everything else — session detection, sanitizer routing, resume argv, localization, and CI wiring — looks correct and is well covered by the new test suite. CLI/CMUXCLI+OmpExtension.swift — the existingOmpExtensionContents error path leaks raw Cocoa error internals to the user. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant OMP as OMP Process
participant Ext as cmux-omp-session.ts
participant cmux as cmux CLI
participant Vault as VaultAgentProcessScanner
participant Launch as CMUXAgentLaunch
User->>OMP: omp [--session id]
OMP->>Ext: session_start event
Ext->>cmux: hooks omp session-start (stdin JSON)
Note over Ext,cmux: sendHook - non-blocking async
OMP->>Ext: before_agent_start event
Ext->>cmux: hooks omp prompt-submit
OMP->>Ext: agent_end event
Ext->>cmux: hooks omp stop
Note over Vault: Parallel process scan
Vault->>Vault: "match processName=omp OR alternateArgvContains"
Vault->>Vault: piCompatibleSessionID via --session/--resume/-r
Vault->>Vault: ompAgentSessionsRoot via PI_CODING_AGENT_DIR or PI_CONFIG_DIR
Vault-->>Launch: SessionRestorableAgentSnapshot
User->>cmux: surface resume
cmux->>Launch: AgentResumeArgv omp sessionId
Launch-->>User: omp --session id
Reviews (8): Last reviewed commit: "fix: avoid blocking OMP stop hook dispat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+129
to
+139
| static var builtInOmp: CmuxVaultAgentRegistration { | ||
| CmuxVaultAgentRegistration( | ||
| id: "omp", | ||
| name: "OMP", | ||
| detect: CmuxVaultAgentDetectRule(processName: "omp"), | ||
| sessionIdSource: .piSessionFile, | ||
| resumeCommand: "{{executable}} --session {{sessionId}}", | ||
| cwd: .preserve, | ||
| sessionDirectory: "~/.omp/agent/sessions" | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
argvContains for bun-invoked OMP
builtInPi uses CmuxVaultAgentDetectRule(processName: "pi", argvContains: ["pi"]) so the vault process scanner also matches Pi processes launched via bun /path/to/@mariozechner/pi-coding-agent/.... builtInOmp only has processName: "omp", so when OMP runs via bun /path/to/@oh-my-pi/pi-coding-agent/src/main.ts the vault process scanner won't match it — process.name is "bun", not "omp" — and vault-based session tracking won't fire for that invocation path. Adding argvContains: ["@oh-my-pi/pi-coding-agent"] would close the gap and mirror the Pi pattern.
Contributor
There was a problem hiding this comment.
Current code already detects bun-invoked OMP via alternateArgvContains: ["@oh-my-pi/pi-coding-agent"], and cmuxTests/OmpSupportTests.swift covers that path with vaultDetectsBunInvokedOmpPackage.
— Claude Code
| foreign_path.parent.mkdir(parents=True, exist_ok=True) | ||
| foreign_path.write_text("// user extension\n", encoding="utf-8") | ||
| uninstall_foreign = subprocess.run( | ||
| [cli_path, "hooks", "omp", "uninstall", "--yes"], |
Contributor
There was a problem hiding this comment.
CMUX_AGENT_LAUNCH_ARGV_B64 encoding mismatch between test and production extension
The TypeScript base64NulSeparated function appends a NUL byte after every element including the last, producing arg1\0arg2\0arg3\0. This test uses "\0".join(launch_argv) (NUL as separator, no trailing NUL), producing arg1\0arg2\0arg3. The two encodings differ in the trailing byte. If the Swift hook handler splits strictly on NUL without filtering empty strings, the TypeScript-generated value would decode with an extra empty element at the tail, producing an argv list that differs from what this test asserts. Using b"\0".join(v.encode() for v in launch_argv) + b"\0" would match the real extension output.
Contributor
There was a problem hiding this comment.
Current production code and the regression fixture both encode CMUX_AGENT_LAUNCH_ARGV_B64 as NUL-separated argv with a trailing NUL; the test decodes and filters the terminator before comparing argv values.
— Claude Code
joshrzemien
force-pushed
the
omp-hook-session-restore
branch
2 times, most recently
from
June 5, 2026 01:38
6604f42 to
ebe8c2f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/VaultAgentProcessScanner.swift (1)
809-817:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrefer exact session-file matches before substring fallback.
Line 809 now resolves explicit
--session/--resume/-rselectors throughresolvedSessionPath(...), but that helper picks the newest.jsonlwhose basename merely contains the requested id. If bothabc.jsonlandxabcx.jsonlexist,--session abccan resolve to the wrong conversation.Suggested fix
- var newest: (url: URL, modified: Date)? + var exactNewest: (url: URL, modified: Date)? + var partialNewest: (url: URL, modified: Date)? for case let url as URL in enumerator where url.pathExtension == "jsonl" { - guard url.deletingPathExtension().lastPathComponent.contains(trimmed) else { continue } + let basename = url.deletingPathExtension().lastPathComponent + guard basename == trimmed || basename.contains(trimmed) else { continue } let values = try? url.resourceValues(forKeys: [.contentModificationDateKey, .isRegularFileKey]) guard values?.isRegularFile == true, let modified = values?.contentModificationDate else { continue } - if newest == nil || modified > newest!.modified { - newest = (url, modified) + if basename == trimmed { + if exactNewest == nil || modified > exactNewest!.modified { + exactNewest = (url, modified) + } + } else if partialNewest == nil || modified > partialNewest!.modified { + partialNewest = (url, modified) } } - return newest?.url.path + return exactNewest?.url.path ?? partialNewest?.url.pathAlso applies to: 963-997
🤖 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/VaultAgentProcessScanner.swift` around lines 809 - 817, The current resolution via PiSessionLocator.resolvedSessionPath(for: process, ...) may match any filename whose basename contains the requested piCompatibleSessionID, causing e.g. "abc" to match "xabcx.jsonl"; update resolvedSessionPath (and the other similar call sites using piCompatibleSessionID / PiSessionLocator.latestSessionPath) to first look for an exact basename match (basename == id or basenameWithoutExtension == id) and return that file, and only if no exact match exists fall back to the existing substring/contains logic; keep using the same parameters (process, registration, fileManager) and preserve behavior when no match is found by returning the original id or delegating to latestSessionPath as currently done.
🤖 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`+OmpExtension.swift:
- Around line 207-245: The code currently swallows read errors by using (try?
String(contentsOf: extensionURL, encoding: .utf8)) ?? "" which treats unreadable
or non‑UTF8 existing files as empty and allows overwrite; change this to
explicitly detect and surface read failures: use FileManager to check whether
extensionURL exists and then attempt String(contentsOf:encoding:) with a
do/catch so that if reading fails you throw a CLIError (include
extensionURL.path and the underlying error) instead of defaulting to "",
preserving the existing logic that allows empty/nonexistent files to proceed;
apply the same fix to the duplicated occurrence that uses the same try? pattern
(the second block around lines 267–278) so both places validate read success
before checking Self.ompExtensionMarker or writing Self.ompExtensionSource.
---
Outside diff comments:
In `@Sources/VaultAgentProcessScanner.swift`:
- Around line 809-817: The current resolution via
PiSessionLocator.resolvedSessionPath(for: process, ...) may match any filename
whose basename contains the requested piCompatibleSessionID, causing e.g. "abc"
to match "xabcx.jsonl"; update resolvedSessionPath (and the other similar call
sites using piCompatibleSessionID / PiSessionLocator.latestSessionPath) to first
look for an exact basename match (basename == id or basenameWithoutExtension ==
id) and return that file, and only if no exact match exists fall back to the
existing substring/contains logic; keep using the same parameters (process,
registration, fileManager) and preserve behavior when no match is found by
returning the original id or delegating to latestSessionPath as currently done.
🪄 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: c1167dac-ea31-4553-a1e7-89be0cc581aa
📒 Files selected for processing (25)
.github/workflows/ci.ymlCLI/CMUXCLI+AgentHookDefinitions.swiftCLI/CMUXCLI+OmpExtension.swiftCLI/cmux.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchEnvironmentPolicy.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizer.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerPrimaryPolicies.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeArgv.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchEnvironmentPolicyTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchSanitizerTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeArgvTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeWorkingDirectoryTests.swiftResources/Localizable.xcstringsSources/TaskManagerTypes.swiftSources/VaultAgentProcessScanner.swiftSources/VaultAgentRegistry.swiftcmux.xcodeproj/project.pbxprojcmuxTests/OmpSupportTests.swiftcmuxTests/PiVaultAgentPersistenceTests.swiftcmuxTests/TaskManagerResourcesTests.swiftdocs/agent-hooks.mddocs/feed.mddocs/vault.mdtests/test_omp_extension_install.pyweb/app/[locale]/docs/session-restore/page.tsx
joshrzemien
force-pushed
the
omp-hook-session-restore
branch
from
June 5, 2026 01:56
ebe8c2f to
47c095b
Compare
Detect OMP sessions from supported selector flags and session roots, restore OMP launches through CMUXAgentLaunch, and cover hook persistence plus direct process detection.
joshrzemien
force-pushed
the
omp-hook-session-restore
branch
from
June 5, 2026 01:59
47c095b to
5b31ba2
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/VaultAgentProcessScanner.swift (1)
819-827:⚠️ Potential issue | 🟠 Major | ⚡ Quick winFix
piCompatibleSessionIDto not treat-r/--resumeas value-carrying session selectors
Sources/VaultAgentProcessScanner.swiftmakespiCompatibleSessionIDscan the entire argv and returns the token after-r/--resumeas the session id. This breaks:
- Node-backed launches: Node’s
-r/--require <module>preloads the module before the main script, so this parser can treat the preload module path as the session id.- kiro-cli semantics: in
kiro-cli,-r/--resumeare boolean flags (no value); the value-carrying form is--resume-id <id>. Returning the next token as the session id will also misparse.Adjust the logic to only parse the actual value-carrying flags (e.g.
--resume-id/--session) and/or restrict scanning to the agent “tail” (after stripping node runtime options), similar to how OpenCode usesnodeScriptArgumentIndex.🤖 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/VaultAgentProcessScanner.swift` around lines 819 - 827, The piCompatibleSessionID logic is incorrectly treating -r/--resume as value-bearing; update the implementation in VaultAgentProcessScanner so piCompatibleSessionID only extracts session IDs from explicit value flags (e.g. --resume-id, --session, --session-id) and/or by scanning only the agent tail after runtime options (use the same nodeScriptArgumentIndex approach OpenCode uses to strip node runtime flags), instead of returning the token after -r/--resume; ensure callers like PiSessionLocator.resolvedSessionPath and PiSessionLocator.latestSessionPath keep the same behavior but receive the corrected session string.
🤖 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 @.github/workflows/ci.yml:
- Around line 10-11: The workflow-level permissions block currently only sets
"contents: read", which implicitly disables other scopes like "actions" and
breaks cache/artifact operations; update the permissions mapping in the CI
workflow's permissions block to include "actions: write" (and optionally
"actions: read" if you prefer explicit restores) so actions/cache and artifact
upload/download can succeed, i.e., add "actions: write" alongside "contents:
read" in the permissions block at the top of the CI YAML.
In `@CLI/CMUXCLI`+OmpExtension.swift:
- Around line 24-35: resolveExecutable currently returns the first PATH entry
that passes fs.accessSync(candidate, fs.constants.X_OK), but on POSIX X_OK can
be true for searchable directories so a directory named "omp" may be persisted;
update resolveExecutable to additionally stat the candidate (e.g., fs.statSync)
and only accept it if stat.isFile() (or isSymbolicLink resolved to a file) and
it is executable before returning; reference the resolveExecutable function and
perform the isFile/isSymbolicLink + permission check sequence to avoid
persisting directories.
In `@tests/test_omp_extension_install.py`:
- Around line 442-444: The test waits for only 3 non-empty lines via
wait_for_text(fake_stdin_log, 3) which can complete after two cmux invocations
(each writes two lines: payload and '---'), causing a race on the third
invocation and the `"last_assistant_message":"done"` assertion; change the wait
count to expect all three stdin payloads (e.g., wait_for_text(fake_stdin_log, 6)
or compute 2*number_of_invocations) and similarly ensure args_log/env_log
expectations match the actual number of lines written (use
wait_for_text(fake_args_log, expected_count), wait_for_text(fake_env_log,
expected_count)) so the test only proceeds after all three cmux runs complete.
---
Outside diff comments:
In `@Sources/VaultAgentProcessScanner.swift`:
- Around line 819-827: The piCompatibleSessionID logic is incorrectly treating
-r/--resume as value-bearing; update the implementation in
VaultAgentProcessScanner so piCompatibleSessionID only extracts session IDs from
explicit value flags (e.g. --resume-id, --session, --session-id) and/or by
scanning only the agent tail after runtime options (use the same
nodeScriptArgumentIndex approach OpenCode uses to strip node runtime flags),
instead of returning the token after -r/--resume; ensure callers like
PiSessionLocator.resolvedSessionPath and PiSessionLocator.latestSessionPath keep
the same behavior but receive the corrected session string.
🪄 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: 72afbeac-d6da-4509-80b6-b39b1580d7da
📒 Files selected for processing (25)
.github/workflows/ci.ymlCLI/CMUXCLI+AgentHookDefinitions.swiftCLI/CMUXCLI+OmpExtension.swiftCLI/cmux.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchEnvironmentPolicy.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizer.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerPrimaryPolicies.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeArgv.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchEnvironmentPolicyTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchSanitizerTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeArgvTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeWorkingDirectoryTests.swiftResources/Localizable.xcstringsSources/TaskManagerTypes.swiftSources/VaultAgentProcessScanner.swiftSources/VaultAgentRegistry.swiftcmux.xcodeproj/project.pbxprojcmuxTests/OmpSupportTests.swiftcmuxTests/PiVaultAgentPersistenceTests.swiftcmuxTests/TaskManagerResourcesTests.swiftdocs/agent-hooks.mddocs/feed.mddocs/vault.mdtests/test_omp_extension_install.pyweb/app/[locale]/docs/session-restore/page.tsx
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`+AgentHookDefinitions.swift:
- Around line 73-75: The special-case check if name == "omp" bypasses the
generic configDirEnvOverride logic and needs an explanatory comment and precise
behavior: add a comment above the if-block describing that OMP must resolve its
directory using CMUXCLI.resolvedOmpAgentDirectory() because OMP requires
checking both PI_CODING_AGENT_DIR (higher precedence) and PI_CONFIG_DIR
(fallback), a precedence that the generic configDirEnvOverride cannot express;
ensure the comment references the env vars PI_CODING_AGENT_DIR and PI_CONFIG_DIR
and mentions CMUXCLI.resolvedOmpAgentDirectory() implements that two-var
resolution so future maintainers understand why OMP is special-cased.
- Around line 199-206: The current special-case env-var precedence for the "omp"
AgentHookDef should be generalized: add an optional closure field to
AgentHookDef named resolveConfigDir: ((String) -> String)? or extend the
existing configDirEnvOverride to accept [String] so resolution supports multiple
env vars with precedence; update the AgentHookDef initializer and the config-dir
resolution logic to call that closure or iterate the env-var array when present,
then convert the "omp" entry to use this new field (removing the bespoke if/else
handling) so future agents can reuse the multi-env-var precedence behavior.
In `@CLI/CMUXCLI`+OmpExtension.swift:
- Around line 205-215: The catch that handles failures from String(contentsOf:
url, encoding: .utf8) incorrectly reuses the "not a cmux extension" message;
change it to throw a dedicated read-failure CLIError (e.g.,
"cli.hooks.omp.error.readFailed" or a clear literal like "Failed to read file")
and include the underlying error formatted with String(describing: error)
instead of error.localizedDescription; update the catch in
CMUXCLI+OmpExtension.swift where String(contentsOf:) is called to construct and
throw this new read-failure CLIError.
🪄 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: 0d02c38e-fe4a-4654-8e74-97e07f4de1ff
📒 Files selected for processing (25)
.github/workflows/ci.ymlCLI/CMUXCLI+AgentHookDefinitions.swiftCLI/CMUXCLI+OmpExtension.swiftCLI/cmux.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchEnvironmentPolicy.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizer.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentLaunchSanitizerPrimaryPolicies.swiftPackages/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentResumeArgv.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchEnvironmentPolicyTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentLaunchSanitizerTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeArgvTests.swiftPackages/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentResumeWorkingDirectoryTests.swiftResources/Localizable.xcstringsSources/TaskManagerTypes.swiftSources/VaultAgentProcessScanner.swiftSources/VaultAgentRegistry.swiftcmux.xcodeproj/project.pbxprojcmuxTests/OmpSupportTests.swiftcmuxTests/PiVaultAgentPersistenceTests.swiftcmuxTests/TaskManagerResourcesTests.swiftdocs/agent-hooks.mddocs/feed.mddocs/vault.mdtests/test_omp_extension_install.pyweb/app/[locale]/docs/session-restore/page.tsx
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Comment on lines
+205
to
+208
| api.on("agent_end", async (event, ctx) => { | ||
| sendHookSync("stop", ctx, { last_assistant_message: lastAssistantMessage(event) }); | ||
| }); | ||
| } |
Contributor
There was a problem hiding this comment.
sendHookSync blocks the event loop for up to 5 s at agent teardown
api.on("agent_end", async ...) declares an async handler, but the body calls sendHookSync — a synchronous spawnSync with timeout: 5000 — without await. That means every OMP session end blocks Node's event loop for up to 5 seconds waiting for the cmux subprocess. The other two hooks (session_start, before_agent_start) correctly use await sendHook(...) which resolves as soon as stdin is flushed. If OMP's extension API honours the returned promise from an async handler, replacing the call here with await sendHook("stop", ...) gives the same delivery guarantee without a wall-clock block.
Rule Used: Flag fixed sleeps, delayed dispatch, timers, polli... (source)
Contributor
There was a problem hiding this comment.
Fixed in 795243c by removing spawnSync and awaiting the same detached stdin-flush sendHook path for agent_end. The Python regression now uses a slow fake cmux command so synchronous Stop dispatch would fail the timing assertion while still proving all three payloads arrive.
— Claude Code
Merged
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
Adds first-class OMP support for cmux session restore:
cmux hooks ompcmux-omp-session.ts--session,--resume,-r,PI_CODING_AGENT_DIR, andPI_CONFIG_DIRCMUXAgentLaunchVerification
./scripts/reload.sh --tag omp-support-prxcodebuild test -project cmux.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-omp-support-pr -only-testing:cmuxTests/OmpSupportTestsswift test --package-path Packages/CMUXAgentLaunchpython3 tests/test_omp_extension_install.py./scripts/check-pbxproj.sh./scripts/lint-pbxproj-test-wiring.shgit diff --check origin/main..HEADLocalization
No new user-facing UI strings were added. Existing
cli.hooks.omp.*localization keys were audited and remain covered.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Adds first-class OMP support.
cmux hooks ompinstalls a native OMP extension that captures lifecycle events, and cmux safely restores OMP sessions; Vault, Task Manager, andCMUXAgentLaunchnow detect and resume OMP.New Features
cmux hooks omp: install/uninstall a non-blocking extension at~/.omp/agent/extensions/cmux-omp-session.tsor$PI_CODING_AGENT_DIR/extensions/cmux-omp-session.ts; refuses to delete non-cmux files; opt out viaCMUX_OMP_HOOKS_DISABLED=1.--session,--resume,-r,PI_CODING_AGENT_DIR, andPI_CONFIG_DIR(explicit selectors win); add built-inompto Vault/Task Manager with process-name and argv detection (@oh-my-pi/pi-coding-agent) and session dir~/.omp/agent/sessions; resume withomp --session <id>; per-directory CWD namespacing.CMUXAgentLaunch: treatomplikepifor argv sanitization (drop prompts and session selectors, including-r); preservePI_CODING_AGENT_DIRandPI_CONFIG_DIR;AgentResumeArgvbuildsomp --session <id>; docs, CLI help, localization, and CI tests (Swift +tests/test_omp_extension_install.py) updated.Bug Fixes
Written for commit 795243c. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores