Handle OMX extended-keys tmux probes in cmux compat layer#2675
Open
kenryu42 wants to merge 6 commits intomanaflow-ai:mainfrom
Open
Handle OMX extended-keys tmux probes in cmux compat layer#2675kenryu42 wants to merge 6 commits intomanaflow-ai:mainfrom
kenryu42 wants to merge 6 commits intomanaflow-ai:mainfrom
Conversation
OMX now probes tmux extended-keys state during startup. Add a regression check that expects show-options/set-option round-tripping so CI can prove current tmux-compat behavior is insufficient before the implementation fix lands. Constraint: Regression-test policy requires a failing test commit before the fix commit Rejected: Ship the implementation without a proving test commit | would violate repo regression policy Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep tmux compatibility coverage focused on observable CLI behavior, not implementation details Tested: python3 -m py_compile tests_v2/test_tmux_compat_matrix.py Not-tested: End-to-end execution against a running cmux instance; local test runs are deferred to CI/VM by repo policy
Teach the tmux compatibility shim to answer the show-options and set-option traffic OMX now uses for its extended-keys lease flow. Persist option state in the existing compat store so startup probes and restoration commands see a consistent value instead of falling through to unsupported-command errors. Constraint: OMX launches through cmux __tmux-compat, not a real tmux server Rejected: Minimal empty-string show-options stub only | fixes the immediate crash but does not model the show/set round-trip OMX performs Confidence: high Scope-risk: narrow Reversibility: clean Directive: If future tmux consumers rely on more options, extend the compat store instead of adding one-off no-op cases Tested: swiftc -parse CLI/cmux.swift Tested: ./scripts/reload.sh --tag fix-omx-extended-keys (blocked by unrelated Ghostty build panic) Not-tested: Full app build and runtime validation; Ghostty submodule setup currently panics on tagged-release version parsing
|
@kenryu42 is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
📝 WalkthroughWalkthroughAdds tmux-compat support for Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client (e.g., omx)
participant CLI as CLI/cmux
participant Store as TmuxCompatStore
participant FS as FileSystem
Client->>CLI: tmux show-options -sv extended-keys
CLI->>CLI: parse flags (-s/-v/-q), normalize name
CLI->>Store: load store & merge defaults (case-insensitive)
Store->>FS: read JSON store file
FS-->>Store: stored data
Store-->>CLI: resolved option value or error
CLI-->>Client: print value (honor -v/-q)
Client->>CLI: tmux set-option -sq extended-keys always
CLI->>CLI: validate args, apply flags (-u/-o/-a)
CLI->>Store: update options map
Store->>FS: write updated JSON store
FS-->>Store: write ack
Store-->>CLI: confirm update
CLI-->>Client: optional output / exit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR implements Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/consistency issues that do not affect correctness for the OMX use case. Both findings are P2. The prior P1 concern about show-options without a name argument (previous thread) is now addressed. No blocking issues remain. CLI/cmux.swift — minor output case inconsistency and dead error-path Important Files Changed
Sequence DiagramsequenceDiagram
participant OMX as Oh My Codex
participant CLI as cmux CLI (compat layer)
participant Store as TmuxCompatStore (JSON)
OMX->>CLI: show-options -sv extended-keys
CLI->>Store: loadTmuxCompatStore()
Store-->>CLI: options: {}
CLI->>CLI: merge defaults (extended-keys=off)
CLI-->>OMX: off
OMX->>CLI: set-option -sq extended-keys always
CLI->>Store: store.options["extended-keys"] = "always"
CLI->>Store: saveTmuxCompatStore()
OMX->>CLI: show-options -sv extended-keys
CLI->>Store: loadTmuxCompatStore()
Store-->>CLI: options: {extended-keys: always}
CLI-->>OMX: always
OMX->>CLI: set-option -u extended-keys
CLI->>Store: store.options.removeValue(extended-keys)
CLI->>Store: saveTmuxCompatStore()
Greploops — Automatically fix all review issues by running Reviews (2): Last reviewed commit: "Close the remaining tmux option parity g..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _run_cli(cli, ["set-option", "-sq", "extended-keys", "always"]) | ||
| show_extended_keys = _run_cli(cli, ["show-options", "-sv", "extended-keys"]) | ||
| _must(show_extended_keys.stdout.strip() == "always", f"set-option should persist extended-keys, got {show_extended_keys.stdout!r}") | ||
| _run_cli(cli, ["set-option", "-sq", "extended-keys", "off"]) |
Contributor
There was a problem hiding this comment.
-u to unset instead of re-setting to the default value
The teardown sets extended-keys back to "off" via set-option, which persists the entry in the store. Using set-option -u extended-keys would cleanly remove the key, restoring the store to its pre-test state and matching OMX's actual lease-release behavior (which calls -u to relinquish ownership).
Suggested change
| _run_cli(cli, ["set-option", "-sq", "extended-keys", "off"]) | |
| _run_cli(cli, ["set-option", "-u", "extended-keys"]) |
Author
There was a problem hiding this comment.
current OMX restores extended-keys with set-option -sq ..., not -u
CLI/cmux.swift
Outdated
Comment on lines
+12238
to
+12241
| guard let optionName = parsed.positional.first? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !optionName.isEmpty else { | ||
| throw CLIError(message: "\(command) requires an option name") |
Contributor
There was a problem hiding this comment.
show-options without a name argument errors instead of listing all options
In real tmux, show-options (or show-options -g) with no positional argument lists every option's current value. The guard here throws a CLIError when no name is supplied, so any caller that enumerates all options will get an error rather than the options map. Consider returning the full store.options merged with built-in defaults when no positional name is provided.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests_v2/test_tmux_compat_matrix.py (1)
160-166: Ensureextended-keyscleanup runs on failure paths.On Line 162–165, if the
"always"assertion fails, the reset to"off"is skipped. Wrap the mutate/verify block intry/finallyto avoid leaking option state across later checks.Proposed refactor
show_extended_keys = _run_cli(cli, ["show-options", "-sv", "extended-keys"]) _must(show_extended_keys.stdout.strip() == "off", f"show-options should default extended-keys to off, got {show_extended_keys.stdout!r}") - _run_cli(cli, ["set-option", "-sq", "extended-keys", "always"]) - show_extended_keys = _run_cli(cli, ["show-options", "-sv", "extended-keys"]) - _must(show_extended_keys.stdout.strip() == "always", f"set-option should persist extended-keys, got {show_extended_keys.stdout!r}") - _run_cli(cli, ["set-option", "-sq", "extended-keys", "off"]) + try: + _run_cli(cli, ["set-option", "-sq", "extended-keys", "always"]) + show_extended_keys = _run_cli(cli, ["show-options", "-sv", "extended-keys"]) + _must(show_extended_keys.stdout.strip() == "always", f"set-option should persist extended-keys, got {show_extended_keys.stdout!r}") + finally: + _run_cli(cli, ["set-option", "-sq", "extended-keys", "off"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_tmux_compat_matrix.py` around lines 160 - 166, The test mutates tmux option extended-keys via _run_cli(["set-option", "-sq", "extended-keys", "always"]) but if the assertion on show_extended_keys fails the cleanup _run_cli(... "off") is skipped; wrap the mutate + verification (the call to _run_cli that sets "always" and the subsequent show-options/_must assertion that expects "always") in a try/finally and move the reset call (_run_cli(cli, ["set-option", "-sq", "extended-keys", "off"])) into the finally block so extended-keys is always reset even on assertion failures; reference the show_extended_keys variable, the _run_cli calls for "set-option" and "show-options", and the _must call when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 12257-12285: The current code always overwrites
store.options[normalizedName] but parseTmuxArguments included flags for "-o"
(only-set) and "-a" (append); update the set-option handling to honor these
flags: after computing normalizedName and value, if parsed.hasFlag("-o") then
only set store.options[normalizedName] if it does not already exist; if
parsed.hasFlag("-a") then read existing = store.options[normalizedName] and set
store.options[normalizedName] = existing.isEmpty ? value :
"\(existing),\(value)" (or similar list-append logic), otherwise perform the
current overwrite; finally call try saveTmuxCompatStore(store) as before.
---
Nitpick comments:
In `@tests_v2/test_tmux_compat_matrix.py`:
- Around line 160-166: The test mutates tmux option extended-keys via
_run_cli(["set-option", "-sq", "extended-keys", "always"]) but if the assertion
on show_extended_keys fails the cleanup _run_cli(... "off") is skipped; wrap the
mutate + verification (the call to _run_cli that sets "always" and the
subsequent show-options/_must assertion that expects "always") in a try/finally
and move the reset call (_run_cli(cli, ["set-option", "-sq", "extended-keys",
"off"])) into the finally block so extended-keys is always reset even on
assertion failures; reference the show_extended_keys variable, the _run_cli
calls for "set-option" and "show-options", and the _must call when making this
change.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47e749ff-8c6d-4e9a-b2ba-4d265c244d9c
📒 Files selected for processing (2)
CLI/cmux.swifttests_v2/test_tmux_compat_matrix.py
Comment on lines
+12257
to
+12285
| case "set-option", "set", "set-window-option", "setw": | ||
| let parsed = try parseTmuxArguments( | ||
| commandArgs, | ||
| valueFlags: [], | ||
| boolFlags: ["-F", "-a", "-g", "-o", "-p", "-q", "-s", "-u", "-w"] | ||
| ) | ||
| guard let optionName = parsed.positional.first? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !optionName.isEmpty else { | ||
| throw CLIError(message: "\(command) requires an option name") | ||
| } | ||
|
|
||
| var store = loadTmuxCompatStore() | ||
| let normalizedName = optionName.lowercased() | ||
| if parsed.hasFlag("-u") { | ||
| store.options.removeValue(forKey: normalizedName) | ||
| try saveTmuxCompatStore(store) | ||
| return | ||
| } | ||
|
|
||
| let value = parsed.positional.dropFirst().joined(separator: " ") | ||
| .trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard !value.isEmpty else { | ||
| throw CLIError(message: "\(command) requires an option value") | ||
| } | ||
|
|
||
| store.options[normalizedName] = value | ||
| try saveTmuxCompatStore(store) | ||
|
|
There was a problem hiding this comment.
Implement parsed -o / -a behavior in set-option.
Line 12261 parses -o and -a, but Lines 12283-12284 always overwrite the value. That diverges from tmux semantics and can break scripts expecting conditional set or append behavior.
🔧 Suggested patch
case "set-option", "set", "set-window-option", "setw":
let parsed = try parseTmuxArguments(
commandArgs,
valueFlags: [],
boolFlags: ["-F", "-a", "-g", "-o", "-p", "-q", "-s", "-u", "-w"]
)
@@
if parsed.hasFlag("-u") {
store.options.removeValue(forKey: normalizedName)
try saveTmuxCompatStore(store)
return
}
@@
- store.options[normalizedName] = value
+ if parsed.hasFlag("-o"), store.options[normalizedName] != nil {
+ return
+ }
+
+ if parsed.hasFlag("-a"), let existing = store.options[normalizedName] {
+ store.options[normalizedName] = existing + value
+ } else {
+ store.options[normalizedName] = value
+ }
try saveTmuxCompatStore(store)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 12257 - 12285, The current code always
overwrites store.options[normalizedName] but parseTmuxArguments included flags
for "-o" (only-set) and "-a" (append); update the set-option handling to honor
these flags: after computing normalizedName and value, if parsed.hasFlag("-o")
then only set store.options[normalizedName] if it does not already exist; if
parsed.hasFlag("-a") then read existing = store.options[normalizedName] and set
store.options[normalizedName] = existing.isEmpty ? value :
"\(existing),\(value)" (or similar list-append logic), otherwise perform the
current overwrite; finally call try saveTmuxCompatStore(store) as before.
Add regression coverage for two follow-up tmux-compat behaviors: listing options with bare show-options and honoring set-option -o/-a semantics for stored values. These expectations should fail until the CLI implementation is extended to match them. Constraint: Regression-test policy requires a failing test commit before the fix commit Rejected: Fold the new expectations directly into the implementation commit | would weaken the regression proof required by repo policy Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep tmux option parity checks behavior-focused so future compat rewrites can still reuse them Tested: python3 -m py_compile tests_v2/test_tmux_compat_matrix.py Not-tested: End-to-end execution against a running cmux instance; local test runs are deferred to CI/VM by repo policy
Extend the new tmux option compatibility layer so bare show-options lists known values and set-option now honors the parsed -o and -a flags. This keeps the new behavior internally consistent and resolves the follow-up review notes without changing the OMX-specific happy path. Constraint: Follow-up review fixes must preserve the OMX startup behavior already covered by this branch Rejected: Leave the parsed -o/-a flags as no-ops | would keep newly added compat behavior internally inconsistent Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep tmux option semantics minimal-but-coherent; add broader tmux parity in focused follow-ups if more flags become necessary Tested: swiftc -parse CLI/cmux.swift Tested: python3 -m py_compile tests_v2/test_tmux_compat_matrix.py Not-tested: Full app build and runtime validation; Ghostty submodule setup currently panics on tagged-release version parsing
Author
|
@greptileai pls review |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux.swift (1)
11721-11736:⚠️ Potential issue | 🟠 MajorPersist tmux option state per socket and under a lock.
extended-keysis a server-scoped option, but this stores it in one flatstore.optionsmap shared by every tmux-compat invocation. Tagged/debug sockets will now see and overwrite each other’s value, and the unlocked load/mutate/save path can also drop concurrent updates from another compat command. This needs a socket-scoped bucket plus a locked write path (theClaudeHookSessionStore.withLockedStatepattern earlier in this file would fit).Also applies to: 12289-12313
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLI/cmux.swift` around lines 11721 - 11736, The store currently puts server-scoped tmux options (like the extended-keys entry) into a single flat options map (`options` / `store.options`) which causes tagged/debug sockets to clobber each other and allows racey loss on concurrent mutates; change the persistence to use a socket-scoped bucket (keyed by the socket identifier used by your tmux-compat invocations) instead of a single map and wrap all load/mutate/save accesses in the existing locked pattern (`ClaudeHookSessionStore.withLockedState`) so reads/writes acquire the same lock; update the code paths that read/write `options` (and related per-socket state such as `mainVerticalLayouts`/`lastSplitSurface` if applicable) to use the socket-keyed submap and the locked write path to avoid cross-socket pollution and race conditions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 12297-12301: The code currently builds `value` from
`parsed.positional.dropFirst().joined(separator: " ")` then trims and rejects
empty strings, which strips significant leading/trailing whitespace and forbids
explicit empty option values; change it to use the joined string as-is (do not
call `trimmingCharacters(in:)`) and stop throwing `CLIError` when `value` is
empty—only throw if there are no positional tokens at all (e.g.
`parsed.positional.count <= 1`); update the logic around `value` and the guard
so `set-option foo ""` and whitespace-significant values are preserved.
- Around line 2697-2703: dispatchSubcommandHelp() only shows help for commands
present in subcommandUsage(_:), so add entries for the new tmux-compat commands
to that function: include "show-option", "show-options", "set", "set-option",
"set-window-option", "set-buffer", and "setw" in subcommandUsage(_:) so that
cmux <command> --help routes to the proper help text; update the same additions
where subcommandUsage(_:) is duplicated (also applies to the other occurrence
referenced in the comment).
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 11721-11736: The store currently puts server-scoped tmux options
(like the extended-keys entry) into a single flat options map (`options` /
`store.options`) which causes tagged/debug sockets to clobber each other and
allows racey loss on concurrent mutates; change the persistence to use a
socket-scoped bucket (keyed by the socket identifier used by your tmux-compat
invocations) instead of a single map and wrap all load/mutate/save accesses in
the existing locked pattern (`ClaudeHookSessionStore.withLockedState`) so
reads/writes acquire the same lock; update the code paths that read/write
`options` (and related per-socket state such as
`mainVerticalLayouts`/`lastSplitSurface` if applicable) to use the socket-keyed
submap and the locked write path to avoid cross-socket pollution and race
conditions.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: f83350fc-bd47-4e78-bc5c-3a1af77e113f
📒 Files selected for processing (2)
CLI/cmux.swifttests_v2/test_tmux_compat_matrix.py
✅ Files skipped from review due to trivial changes (1)
- tests_v2/test_tmux_compat_matrix.py
Add behavioral coverage for two remaining tmux-compat gaps on this branch: new show/set subcommands should surface help via --help, and set-option should preserve explicit empty strings plus leading or trailing whitespace in stored values. Constraint: Regression-test policy requires a failing test commit before the fix commit Rejected: Bundle these assertions into the implementation commit | would weaken the regression proof required by repo policy Confidence: high Scope-risk: narrow Reversibility: clean Directive: Keep tmux-compat regression cases focused on user-visible CLI behavior rather than implementation details Tested: python3 -m py_compile tests_v2/test_tmux_compat_matrix.py Not-tested: End-to-end execution against a running cmux instance; local test runs are deferred to CI/VM by repo policy
Close the remaining review feedback on this branch by wiring show/set tmux-compat commands into subcommand help and by storing option values exactly as passed. That keeps quoted empty strings and whitespace-significant values round-trippable while making Unknown command 'show-options'. Run 'cmux help' to see available commands. and friends behave like other CLI subcommands. Constraint: The fix must preserve the OMX startup behavior already covered by this branch Rejected: Trim and reject empty option values | would keep the new tmux-compat layer unable to round-trip exact values Confidence: high Scope-risk: narrow Reversibility: clean Directive: Treat tmux-compat option values as exact user input unless tmux semantics require transformation Tested: swiftc -parse CLI/cmux.swift Tested: python3 -m py_compile tests_v2/test_tmux_compat_matrix.py Tested: ./scripts/reload.sh --tag fix-tmux-option-help (blocked by unrelated Ghostty build panic) Not-tested: Full app build and runtime validation; Ghostty submodule setup currently panics on tagged-release version parsing
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests_v2/test_tmux_compat_matrix.py (1)
162-197: Make option-state cleanup failure-safe to avoid cross-run test pollution.This test mutates persisted compat options. If execution fails/interruption occurs before teardown lines, later runs can inherit stale values (notably
extended-keys), causing cascading failures. Prefertry/finallycleanup.♻️ Suggested refactor (failure-safe cleanup)
- show_extended_keys = _run_cli(cli, ["show-options", "-sv", "extended-keys"]) - _must(show_extended_keys.stdout.strip() == "off", f"show-options should default extended-keys to off, got {show_extended_keys.stdout!r}") - _run_cli(cli, ["set-option", "-sq", "extended-keys", "always"]) - show_extended_keys = _run_cli(cli, ["show-options", "-sv", "extended-keys"]) - _must(show_extended_keys.stdout.strip() == "always", f"set-option should persist extended-keys, got {show_extended_keys.stdout!r}") - _run_cli(cli, ["set-option", "-sq", "extended-keys", "off"]) - show_all_options = _run_cli(cli, ["show-options"]) - _must("extended-keys off" in show_all_options.stdout, f"show-options should list default options, got {show_all_options.stdout!r}") - - custom_option = f"@compat_probe_{stamp}" - _run_cli(cli, ["set-option", custom_option, "alpha"]) - _run_cli(cli, ["set-option", "-o", custom_option, "beta"]) - custom_value = _run_cli(cli, ["show-options", "-v", custom_option]) - _must(custom_value.stdout.strip() == "alpha", f"set-option -o should keep the existing value, got {custom_value.stdout!r}") - _run_cli(cli, ["set-option", "-a", custom_option, "gamma"]) - custom_value = _run_cli(cli, ["show-options", "-v", custom_option]) - _must(custom_value.stdout.strip() == "alphagamma", f"set-option -a should append to the existing value, got {custom_value.stdout!r}") - show_all_options = _run_cli(cli, ["show-options"]) - _must(f"{custom_option} alphagamma" in show_all_options.stdout, f"show-options should list custom options, got {show_all_options.stdout!r}") - _run_cli(cli, ["set-option", "-u", custom_option]) + custom_option = None + empty_option = None + spaced_option = None + try: + show_extended_keys = _run_cli(cli, ["show-options", "-sv", "extended-keys"]) + _must(show_extended_keys.stdout.strip() == "off", f"show-options should default extended-keys to off, got {show_extended_keys.stdout!r}") + _run_cli(cli, ["set-option", "-sq", "extended-keys", "always"]) + show_extended_keys = _run_cli(cli, ["show-options", "-sv", "extended-keys"]) + _must(show_extended_keys.stdout.strip() == "always", f"set-option should persist extended-keys, got {show_extended_keys.stdout!r}") + _run_cli(cli, ["set-option", "-sq", "extended-keys", "off"]) + show_all_options = _run_cli(cli, ["show-options"]) + _must("extended-keys off" in show_all_options.stdout, f"show-options should list default options, got {show_all_options.stdout!r}") + + custom_option = f"@compat_probe_{stamp}" + _run_cli(cli, ["set-option", custom_option, "alpha"]) + _run_cli(cli, ["set-option", "-o", custom_option, "beta"]) + custom_value = _run_cli(cli, ["show-options", "-v", custom_option]) + _must(custom_value.stdout.strip() == "alpha", f"set-option -o should keep the existing value, got {custom_value.stdout!r}") + _run_cli(cli, ["set-option", "-a", custom_option, "gamma"]) + custom_value = _run_cli(cli, ["show-options", "-v", custom_option]) + _must(custom_value.stdout.strip() == "alphagamma", f"set-option -a should append to the existing value, got {custom_value.stdout!r}") + show_all_options = _run_cli(cli, ["show-options"]) + _must(f"{custom_option} alphagamma" in show_all_options.stdout, f"show-options should list custom options, got {show_all_options.stdout!r}") + + empty_option = f"@empty_probe_{stamp}" + _run_cli(cli, ["set-option", empty_option, ""]) + empty_value = _run_cli(cli, ["show-options", "-v", empty_option]) + _must(empty_value.stdout == "\n", f"set-option should preserve explicit empty values, got {empty_value.stdout!r}") + + spaced_option = f"@space_probe_{stamp}" + spaced_value = " padded value " + _run_cli(cli, ["set-option", spaced_option, spaced_value]) + shown_spaced_value = _run_cli(cli, ["show-options", "-v", spaced_option]) + _must(shown_spaced_value.stdout == spaced_value + "\n", f"set-option should preserve leading/trailing whitespace, got {shown_spaced_value.stdout!r}") + finally: + _run_cli(cli, ["set-option", "-sq", "extended-keys", "off"], expect_ok=False) + for opt in (custom_option, empty_option, spaced_option): + if opt: + _run_cli(cli, ["set-option", "-u", opt], expect_ok=False)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests_v2/test_tmux_compat_matrix.py` around lines 162 - 197, The test mutates persisted options (calls to _run_cli with "set-option" for "extended-keys", custom_option, empty_option, spaced_option) without guaranteeing cleanup on failure; wrap the section that changes options in a try/finally so all mutated state is reverted in the finally block (restore original extended-keys value using show-options -sv before changing it, and ensure you call _run_cli(..., ["set-option", "-u", <custom_option>]) / ["set-option", "-u", <empty_option>] / ["set-option", "-u", <spaced_option>] even if an assertion via _must or an exception occurs). Use the existing helpers (_run_cli, _must) and reference the variables extended-keys, custom_option, empty_option, spaced_option to locate the changes to revert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests_v2/test_tmux_compat_matrix.py`:
- Around line 162-197: The test mutates persisted options (calls to _run_cli
with "set-option" for "extended-keys", custom_option, empty_option,
spaced_option) without guaranteeing cleanup on failure; wrap the section that
changes options in a try/finally so all mutated state is reverted in the finally
block (restore original extended-keys value using show-options -sv before
changing it, and ensure you call _run_cli(..., ["set-option", "-u",
<custom_option>]) / ["set-option", "-u", <empty_option>] / ["set-option", "-u",
<spaced_option>] even if an assertion via _must or an exception occurs). Use the
existing helpers (_run_cli, _must) and reference the variables extended-keys,
custom_option, empty_option, spaced_option to locate the changes to revert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ce75d70-e553-43c1-9d4e-d2b3f0936fc1
📒 Files selected for processing (2)
CLI/cmux.swifttests_v2/test_tmux_compat_matrix.py
✅ Files skipped from review due to trivial changes (1)
- CLI/cmux.swift
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
1 participant
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
show-options/set-optionextended-keys startup flowshow-optionsand persistset-optionstateextended-keystooffso OMX can acquire and release its tmux lease without unsupported-command errorsFixes #2671
Why this PR
PR #2672 fixes the immediate reachability issue with a minimal
show-optionsstub. This PR goes a step further and models theshow-options+set-optionround-trip that Oh My Codex now performs during startup.Testing
python3 -m py_compile tests_v2/test_tmux_compat_matrix.pyswiftc -parse CLI/cmux.swift./scripts/reload.sh --tag fix-omx-extended-keys(blocked by unrelated Ghostty build panic: tagged releases must be in vX.Y.Z format matching build.zig)Notes
.gitignorechanges out of this PRSummary by CodeRabbit
New Features
Tests