Repository navigation
__tmux-compat: short tmux format codes (#S, #I, #W, ...) are not substituted, breaking cmux omc - #3228
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe tmux-format rendering in the CLI now recognizes and expands shorthand placeholders ( Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes 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 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8418b2e0d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "D": "pane_id", | ||
| ] | ||
| for (shortAlias, key) in shortAliases where context.keys.contains(key) { | ||
| rendered = rendered.replacingOccurrences(of: "#\(shortAlias)", with: context[key] ?? "") |
There was a problem hiding this comment.
The new short-alias loop does global replacingOccurrences on the already-rendered string, so substitutions can run again inside values that were just inserted. For example, if a window title is literally #P, formatting #W first inserts #P and then the later #P pass rewrites it to the pane index, which is incorrect (tmux should return the title text as-is). This causes wrong output for user-controlled names/titles containing short alias patterns.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR fixes
Confidence Score: 3/5Not safe to merge as-is: the guard condition causes unresolved short codes to remain as literal tokens in output. One P1 logic bug (unresolved short codes leak instead of being stripped) and one P2 policy violation (single-commit regression test). The P1 affects real users: any format string using
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["tmuxRenderFormat(format, context, fallback)"] --> B["Replace long-form #{key} tokens from context"]
B --> C["For each shortAlias in table #S/#I/#W/#P/#T/#D"]
C --> D{"context contains mapped key?"}
D -- "yes" --> E["Replace #X with context value"]
D -- "no" --> F["⚠️ Leave literal #X in string (not cleaned up)"]
E --> G["Regex cleanup: remove unmatched #{...} tokens"]
F --> G
G --> H["Trim whitespace"]
H --> I{"trimmed empty?"}
I -- "yes" --> J["Return fallback"]
I -- "no" --> K["Return rendered string"]
Reviews (1): Last reviewed commit: "Address https://github.com/manaflow-ai/c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (shortAlias, key) in shortAliases where context.keys.contains(key) { | ||
| rendered = rendered.replacingOccurrences(of: "#\(shortAlias)", with: context[key] ?? "") | ||
| } |
There was a problem hiding this comment.
#X instead of being stripped
When a context key is absent (e.g. no active pane, so pane_index/pane_title/pane_id are never set), the where context.keys.contains(key) guard skips the substitution and leaves the raw token — e.g. #P, #T, #D — in the rendered string. Long-form codes like #{pane_index} are cleaned up by the #\{[^}]+\} regex pass that follows, but there is no equivalent cleanup for short codes. The result is that callers who pass a format string that includes short codes for keys not present in the current context get back a string with literal #P / #T / #D artifacts, while the parallel long-form codes are silently removed.
A minimal fix is to always replace — using an empty string (matching long-form cleanup behaviour) when the key is absent:
for (shortAlias, key) in shortAliases {
rendered = rendered.replacingOccurrences(of: "#\(shortAlias)", with: context[key] ?? "")
}| print("PASS") | ||
|
|
||
|
|
||
| def test_display_short_format_aliases(cli: str) -> None: | ||
| """Short tmux format aliases match long-form output.""" | ||
| print(" test_display_short_format_aliases ... ", end="", flush=True) | ||
| long_form = _run_tmux_compat(cli, ["display", "-p", "#{session_name}:#{window_index}:#{window_name}"]) | ||
| _must(long_form.returncode == 0, f"display with long-form failed: {long_form.stderr}") | ||
| long_output = long_form.stdout.strip() | ||
| _must(long_output, f"Expected long-form output, got empty: {long_form.stdout!r}") | ||
|
|
||
| short_form = _run_tmux_compat(cli, ["display", "-p", "#S:#I:#W"]) | ||
| _must(short_form.returncode == 0, f"display with short-form failed: {short_form.stderr}") | ||
| short_output = short_form.stdout.strip() | ||
| _must(short_output == long_output, f"Short-form output mismatch: short={short_output!r} long={long_output!r}") | ||
|
|
||
| paneid_form = _run_tmux_compat(cli, ["display", "-p", "#S:#I %<paneid>"]) | ||
| _must(paneid_form.returncode == 0, f"display with paneid token failed: {paneid_form.stderr}") | ||
| paneid_output = paneid_form.stdout.strip() | ||
| _must(long_output.split(":")[0] in paneid_output and long_output.split(":")[1] in paneid_output, f"paneid form should include expanded short fields: {paneid_output!r}") | ||
| _must("%<paneid>" in paneid_output, f"Expected literal %<paneid> to remain: {paneid_output!r}") | ||
| print("PASS") | ||
|
|
||
|
|
||
| def test_multi_pane_geometry(cli: str, c: cmux) -> None: | ||
| """After splitting, two panes have different pane_left values and halved widths.""" |
There was a problem hiding this comment.
CLAUDE.md requires a two-commit structure for every regression-test/bug-fix pair so CI can prove the test actually catches the bug before the fix is applied:
- Commit 1 – Add the failing test only (no fix). CI should go red.
- Commit 2 – Add the fix. CI should go green.
This PR squashes both the new test_display_short_format_aliases test and the shortAliases implementation into one commit, so there is no CI evidence that the test would have failed against the pre-fix code.
Context Used: CLAUDE.md (source)
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 11563-11576: The shorthand expansion currently mutates rendered
after long-form replacements, allowing substituted values to be reprocessed and
leaving unresolved short tokens; instead, scan the original template string (not
rendered) for short placeholders using the shortAliases map and apply
replacements into rendered only where those short placeholders actually existed
in the original template; use the same presence logic as the long-form fallback
(the "#\\{[^}]+\\}" cleanup) to clear any unresolved short tokens so no raw "#X"
remains; refer to shortAliases, rendered, context and the existing long-form
replacement call when making the change.
In `@tests_v2/test_tmux_compat_geometry.py`:
- Around line 161-179: Update test_display_short_format_aliases to also exercise
the newly added short aliases (`#P`, `#T`, `#D`) instead of only `#S/`#I/#W: build a
mapping of short tokens to their long-form tokens, request tmux display once
using the long-form tokens joined by a delimiter unlikely to appear in fields
(e.g. ASCII unit separator) and once using the corresponding short tokens joined
by the same delimiter, then assert the two outputs are equal; replace the
brittle long_output.split(":") checks with this delimiter-based equality check
and apply the same approach to the paneid_form check (use the delimiter and
verify the expected literal %<paneid> behavior) so the test covers `#P/`#T/#D and
avoids colon-splitting fragility while keeping checks around long_form,
short_form, and paneid_form.
🪄 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: 7c517833-5cd6-40a4-9fc3-2b58ee401e1b
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (2)
CLI/cmux.swifttests_v2/test_tmux_compat_geometry.py
| let shortAliases: [String: String] = [ | ||
| "S": "session_name", | ||
| "I": "window_index", | ||
| "W": "window_name", | ||
| "P": "pane_index", | ||
| "T": "pane_title", | ||
| "D": "pane_id", | ||
| ] | ||
| for (shortAlias, key) in shortAliases where context.keys.contains(key) { | ||
| rendered = rendered.replacingOccurrences(of: "#\(shortAlias)", with: context[key] ?? "") | ||
| } | ||
| rendered = rendered.replacingOccurrences( | ||
| of: "#\\{[^}]+\\}", | ||
| with: "", |
There was a problem hiding this comment.
Keep shorthand expansion from reprocessing substituted values.
This pass runs on rendered after the long-form replacements, so any literal #S/#I sequences inside a session name, window name, or pane title can be expanded a second time. The where context.keys.contains(key) guard also means an unresolved shorthand token can survive as a raw #X instead of being cleared like the long-form fallback.
Please expand shorthand tokens while scanning the original template, or otherwise confine replacement to actual format placeholders only.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@CLI/cmux.swift` around lines 11563 - 11576, The shorthand expansion currently
mutates rendered after long-form replacements, allowing substituted values to be
reprocessed and leaving unresolved short tokens; instead, scan the original
template string (not rendered) for short placeholders using the shortAliases map
and apply replacements into rendered only where those short placeholders
actually existed in the original template; use the same presence logic as the
long-form fallback (the "#\\{[^}]+\\}" cleanup) to clear any unresolved short
tokens so no raw "#X" remains; refer to shortAliases, rendered, context and the
existing long-form replacement call when making the change.
| def test_display_short_format_aliases(cli: str) -> None: | ||
| """Short tmux format aliases match long-form output.""" | ||
| print(" test_display_short_format_aliases ... ", end="", flush=True) | ||
| long_form = _run_tmux_compat(cli, ["display", "-p", "#{session_name}:#{window_index}:#{window_name}"]) | ||
| _must(long_form.returncode == 0, f"display with long-form failed: {long_form.stderr}") | ||
| long_output = long_form.stdout.strip() | ||
| _must(long_output, f"Expected long-form output, got empty: {long_form.stdout!r}") | ||
|
|
||
| short_form = _run_tmux_compat(cli, ["display", "-p", "#S:#I:#W"]) | ||
| _must(short_form.returncode == 0, f"display with short-form failed: {short_form.stderr}") | ||
| short_output = short_form.stdout.strip() | ||
| _must(short_output == long_output, f"Short-form output mismatch: short={short_output!r} long={long_output!r}") | ||
|
|
||
| paneid_form = _run_tmux_compat(cli, ["display", "-p", "#S:#I %<paneid>"]) | ||
| _must(paneid_form.returncode == 0, f"display with paneid token failed: {paneid_form.stderr}") | ||
| paneid_output = paneid_form.stdout.strip() | ||
| _must(long_output.split(":")[0] in paneid_output and long_output.split(":")[1] in paneid_output, f"paneid form should include expanded short fields: {paneid_output!r}") | ||
| _must("%<paneid>" in paneid_output, f"Expected literal %<paneid> to remain: {paneid_output!r}") | ||
| print("PASS") |
There was a problem hiding this comment.
Tighten the alias regression coverage.
This only exercises #S, #I, and #W, so it can still miss regressions in the newly added #P, #T, and #D mappings. The split(":") check is also brittle if tmux output ever contains : in a field.
♻️ Suggested test shape
def test_display_short_format_aliases(cli: str) -> None:
"""Short tmux format aliases match long-form output."""
- print(" test_display_short_format_aliases ... ", end="", flush=True)
- long_form = _run_tmux_compat(cli, ["display", "-p", "#{session_name}:#{window_index}:#{window_name}"])
- _must(long_form.returncode == 0, f"display with long-form failed: {long_form.stderr}")
- long_output = long_form.stdout.strip()
- _must(long_output, f"Expected long-form output, got empty: {long_form.stdout!r}")
-
- short_form = _run_tmux_compat(cli, ["display", "-p", "#S:`#I`:`#W`"])
- _must(short_form.returncode == 0, f"display with short-form failed: {short_form.stderr}")
- short_output = short_form.stdout.strip()
- _must(short_output == long_output, f"Short-form output mismatch: short={short_output!r} long={long_output!r}")
-
- paneid_form = _run_tmux_compat(cli, ["display", "-p", "#S:`#I` %<paneid>"])
- _must(paneid_form.returncode == 0, f"display with paneid token failed: {paneid_form.stderr}")
- paneid_output = paneid_form.stdout.strip()
- _must(long_output.split(":")[0] in paneid_output and long_output.split(":")[1] in paneid_output, f"paneid form should include expanded short fields: {paneid_output!r}")
- _must("%<paneid>" in paneid_output, f"Expected literal %<paneid> to remain: {paneid_output!r}")
+ cases = [
+ ("#S", "#{session_name}"),
+ ("#I", "#{window_index}"),
+ ("#W", "#{window_name}"),
+ ("#P", "#{pane_index}"),
+ ("#T", "#{pane_title}"),
+ ("#D", "#{pane_id}"),
+ ]
+
+ for short_fmt, long_fmt in cases:
+ short_form = _run_tmux_compat(cli, ["display", "-p", short_fmt])
+ long_form = _run_tmux_compat(cli, ["display", "-p", long_fmt])
+ _must(short_form.returncode == 0, f"display with short-form failed: {short_form.stderr}")
+ _must(long_form.returncode == 0, f"display with long-form failed: {long_form.stderr}")
+ _must(
+ short_form.stdout.strip() == long_form.stdout.strip(),
+ f"Short-form output mismatch for {short_fmt}: short={short_form.stdout!r} long={long_form.stdout!r}",
+ )
+
+ paneid_form = _run_tmux_compat(cli, ["display", "-p", "#S:`#I` %<paneid>"])
+ _must(paneid_form.returncode == 0, f"display with paneid token failed: {paneid_form.stderr}")
+ paneid_output = paneid_form.stdout.strip()
+ _must(paneid_output.endswith(" %<paneid>"), f"Expected literal %<paneid> to remain: {paneid_output!r}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests_v2/test_tmux_compat_geometry.py` around lines 161 - 179, Update
test_display_short_format_aliases to also exercise the newly added short aliases
(`#P`, `#T`, `#D`) instead of only `#S/`#I/#W: build a mapping of short tokens to their
long-form tokens, request tmux display once using the long-form tokens joined by
a delimiter unlikely to appear in fields (e.g. ASCII unit separator) and once
using the corresponding short tokens joined by the same delimiter, then assert
the two outputs are equal; replace the brittle long_output.split(":") checks
with this delimiter-based equality check and apply the same approach to the
paneid_form check (use the delimiter and verify the expected literal %<paneid>
behavior) so the test covers `#P/`#T/#D and avoids colon-splitting fragility while
keeping checks around long_form, short_form, and paneid_form.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Addresses #3190
Generated by cmux-loader.
Codex final message:
Done.
Summary
Implemented minimal
__tmux-compatformat substitution support for short tmux aliases intmuxRenderFormatby mapping short codes to existing long-form context keys before fallback cleanup:#S -> session_name#I -> window_index#W -> window_name#P -> pane_index#T -> pane_title#D -> pane_idAdded a behavior-level regression test that validates short-form rendering:
#S:#I:#Wagainst#{session_name}:#{window_index}:#{window_name}output%<paneid>remains literal in mixed format usageVerification performed
./scripts/reload --tag loaderError: zig is not installed. Install via: brew install zigPR URL
No PR was opened from this environment because GitHub CLI access requires authentication (
GH_TOKEN) and it is not set here.Summary by cubic
Enable short tmux format aliases in
__tmux-compatso#S,#I,#W, and others render correctly, fixing brokencmux omcoutput. Fixes #3190 and refreshes the Swift file length budget.tmuxRenderFormat:#S→session_name,#I→window_index,#W→window_name,#P→pane_index,#T→pane_title,#D→pane_id.#S:#I:#Wto long-form and keeping%<paneid>literal.Written for commit 7c793c9. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Release Notes
New Features
Tests