feat(zellij): add Zellij terminal multiplexer support#2938
feat(zellij): add Zellij terminal multiplexer support#2938florianschepp wants to merge 28 commits intocode-yeongyu:devfrom
Conversation
|
All contributors have signed the CLA. Thank you! ✅ |
- Add ZellijAdapter with DI-based spawnFn for testability
- Add displayName support for better pane naming (Agent: {title})
- Non-anchor panes always use -d down for reliable stacking
- Add TerminalConfigSchema with provider auto/tmux/zellij selection
- Modularize config schema into src/config/schema/ directory
- Wire zellij detection into create-managers.ts (async)
- Add 8 BDD-style tests covering stacking and displayName behavior
Co-authored-by: David Laing <david@davidlaing.com>
florianschepp
force-pushed
the
feat/zellij-support
branch
from
59266f4 to
69d243b
Compare
|
Marking as draft for now. Noticed the PR was showing far more changed files than expected due to some unrelated divergence from the source fork sneaking in during the merge setup. The Zellij implementation itself is solid; this is purely a housekeeping issue on my end. Will mark ready for review once the diff is clean. |
|
Diff is clean now. Was a quick one :). 20 files now, all Zellij-related. Marking ready for review. |
|
I have read the CLA Document and I hereby sign the CLA |
|
recheck |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
10 issues found across 20 files
Confidence score: 2/5
- There is a high-risk security concern in
src/features/tmux-subagent/manager.ts: an unescapedsessionIdis interpolated into a shell command, which can enable shell injection with crafted input. src/features/tmux-subagent/manager.tsalso has concrete lifecycle/constructor issues (ignored injected deps forsourcePaneId, and adapter pane handles not cleaned up) that can cause inconsistent runtime behavior and leaked panes.- Several test-layer problems increase regression risk:
src/shared/terminal-multiplexer/zellij-storage.test.tscan delete real user storage paths, andsrc/shared/terminal-multiplexer/zellij-adapter.test.ts/src/shared/terminal-multiplexer/contract.test.tsinclude weak or misleading assertions/mocking that may hide failures. - Pay close attention to
src/features/tmux-subagent/manager.ts,src/shared/terminal-multiplexer/zellij-storage.test.ts, andsrc/shared/terminal-multiplexer/tmux-adapter.ts- security exposure, potential data-loss cleanup behavior, and command/session handling are the main risk areas.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/shared/terminal-multiplexer/contract.test.ts">
<violation number="1" location="src/shared/terminal-multiplexer/contract.test.ts:24">
P2: Bare `catch` on adapter imports swallows real load/runtime errors and always falls back to NotImplemented adapters, producing misleading failures instead of surfacing the actual stack trace.</violation>
<violation number="2" location="src/shared/terminal-multiplexer/contract.test.ts:77">
P2: Mocking `Bun.spawn` in the contract test does not affect these adapters because they capture `spawn` from the Bun module at import/constructor time; the test constructs adapters without injecting `mockSpawn`, so real subprocesses may run during tests.</violation>
</file>
<file name="src/shared/terminal-multiplexer/zellij-adapter.test.ts">
<violation number="1" location="src/shared/terminal-multiplexer/zellij-adapter.test.ts:184">
P2: Behavior-labeled tests use no-op assertions (`toBeDefined`) instead of validating the claimed state persistence/validation outcomes.</violation>
</file>
<file name="src/shared/terminal-multiplexer/tmux-adapter.ts">
<violation number="1" location="src/shared/terminal-multiplexer/tmux-adapter.ts:66">
P2: spawnPane ignores its cmd parameter and hardcodes session/server values, preventing callers from controlling the command/session context.</violation>
</file>
<file name="src/features/tmux-subagent/manager.ts">
<violation number="1" location="src/features/tmux-subagent/manager.ts:88">
P2: Constructor overload handling causes old-signature injected deps to be ignored when computing `sourcePaneId`, leading to inconsistent runtime behavior.</violation>
<violation number="2" location="src/features/tmux-subagent/manager.ts:712">
P1: Unescaped sessionId is interpolated into a shell command, allowing shell injection if a crafted session ID contains quotes or metacharacters.</violation>
<violation number="3" location="src/features/tmux-subagent/manager.ts:717">
P2: Adapter-spawned pane handles are stored but never closed or cleaned up; onSessionDeleted/cleanup only operate on tmux sessions, so adapter panes can leak.</violation>
</file>
<file name="src/shared/terminal-multiplexer/zellij-storage.test.ts">
<violation number="1" location="src/shared/terminal-multiplexer/zellij-storage.test.ts:18">
P2: Tests delete the real OpenCode storage directory instead of a test-only temp path, risking removal of user data. The temp testStorageDir is never used to isolate storage, so beforeEach/afterEach clean up ~/.local/share/opencode/storage/zellij-adapter during test runs.</violation>
<violation number="2" location="src/shared/terminal-multiplexer/zellij-storage.test.ts:120">
P2: Corruption tests hardcode ~/.local/share instead of using getOpenCodeStorageDir(), so they can miss the actual storage path when XDG_DATA_HOME or tmpdir fallback is used, making tests flaky on nonstandard environments.</violation>
</file>
<file name="src/shared/terminal-multiplexer/zellij-adapter.ts">
<violation number="1" location="src/shared/terminal-multiplexer/zellij-adapter.ts:152">
P2: spawnPane commits the anchor state even when pane creation fails or no pane ID is captured, so an empty paneId can be persisted and treated as success, leaving the adapter in an inconsistent state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Fix shell injection in manager.ts:712 via safe argument passing - Fix `deps` → `this.deps` reference in manager.ts:104 - Close adapter pane handles on subagent removal and cleanup - Guard truthy paneId before closing in zellij-adapter.ts - Convert module-level ZELLIJ_ADAPTER_STORAGE constant to lazy getZellijStorageDir() function to fix test isolation - Fix zellij-storage tests to use mock.module + per-test storage dirs - Add console.error(e) to bare catch blocks in contract.test.ts - Pass mockSpawn as 3rd arg to ZellijAdapter in contract test - Replace no-op assertion in zellij-adapter.test.ts with meaningful test - Use cmd param in tmux-adapter.ts spawnPane (fix10); fix import path from utils to tmux-path-resolver Tests: 63/63 passing across zellij-adapter, zellij-storage, and contract test suites (25/25 in zellij-adapter alone). Co-authored-by: David Laing <david@davidlaing.com>
There was a problem hiding this comment.
4 issues found across 7 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/shared/terminal-multiplexer/zellij-adapter.ts">
<violation number="1" location="src/shared/terminal-multiplexer/zellij-adapter.ts:151">
P1: Conditional resolver call can leave `anchorReadyPromise` unresolved, causing later `spawnPane` calls to hang indefinitely.</violation>
</file>
<file name="src/shared/terminal-multiplexer/zellij-adapter.test.ts">
<violation number="1" location="src/shared/terminal-multiplexer/zellij-adapter.test.ts:51">
P2: Persistence tests are invalidated because adapters read/write an injected per-instance mock store while tests seed/check module-level storage, allowing false positives.</violation>
</file>
<file name="src/shared/terminal-multiplexer/tmux-adapter.ts">
<violation number="1" location="src/shared/terminal-multiplexer/tmux-adapter.ts:64">
P2: Tmux adapter config is now ignored after inlining pane spawn logic, causing `{ enabled: false }` and related config fields to have no effect.</violation>
<violation number="2" location="src/shared/terminal-multiplexer/tmux-adapter.ts:77">
P2: `spawnPane` can fail without `splitFrom` because `tmux split-window` is invoked without a guaranteed target/context, and the previous tmux-context guard was removed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Fix anchorReadyPromise hang: null out promise+resolver when first pane ID capture fails, preventing subsequent spawnPane calls from blocking - Fix 3 persistence tests: seed in-memory mock storage directly instead of calling disk saveZellijState(); add real assertions replacing no-ops - Add config.enabled and isInsideTmux() guards to TmuxAdapter.spawnPane - Add getCurrentPaneId() fallback as -t target for split-window when no splitFrom provided - Fix makeTrackingSpawn() in stacking tests to return pane ID for cat calls, enabling 3 previously-timing-out stacking tests to pass Co-authored-by: David Laing <david@davidlaing.com>
There was a problem hiding this comment.
0 issues found across 4 files (changes from recent commits).
Requires human review: Large architectural change (2,385 lines) refactoring core session management and changing plugin startup to async. Cannot be 100% sure of zero regressions without manual verification.
If the first pane's ID capture fails, hasCreatedFirstPane was left as true, causing all subsequent spawnPane calls to skip anchor setup and spawn floating panes permanently. Reset it to false alongside the existing anchorReadyPromise/anchorReadyResolver nulling so the next call retries the anchor setup correctly. Co-authored-by: David Laing <david@davidlaing.com>
|
@cubic-dev-ai Please re-review the latest commit. |
@florianschepp I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/shared/terminal-multiplexer/zellij-adapter.ts">
<violation number="1" location="src/shared/terminal-multiplexer/zellij-adapter.ts:169">
P1: On first-pane failure, `anchorReadyPromise` is cleared but never settled, so concurrent `spawnPane` calls already awaiting it can hang indefinitely.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ck concurrent callers Concurrent spawnPane calls that entered the else-if branch before anchor ID capture failed held a reference to the same Promise object. Nulling anchorReadyPromise cleared the instance variable but left those callers awaiting a promise that would never settle, hanging indefinitely. Fix: resolve the promise with null before clearing it so all concurrent waiters are immediately unblocked. Guard the subsequent stack-panes call against a null anchor ID so a failed anchor silently skips stacking rather than passing null into the zellij command. Co-authored-by: David Laing <david@davidlaing.com>
|
@cubic-dev-ai Please re-review the latest commit. |
@florianschepp I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
15 issues found across 20 files
Confidence score: 2/5
- High-confidence path traversal in
src/shared/terminal-multiplexer/zellij-storage.ts(unsanitizedsessionIDin read/write/delete paths) is a security-sensitive issue with clear user/data impact, which drives this into high-risk territory. - There are concrete behavior regressions around session lifecycle and detection:
src/create-managers.tscan skip Zellij auto-detect for default config users, andsrc/features/tmux-subagent/manager.tscan fail to spawn/clean adapter sessions in non-tmux and deletion flows. - Several medium-severity correctness/test issues in
src/shared/terminal-multiplexer/zellij-adapter.ts,src/shared/terminal-multiplexer/tmux-adapter.ts, and related tests suggest state/parsing mismatches and unreliable test isolation, increasing the chance of hidden regressions. - Pay close attention to
src/shared/terminal-multiplexer/zellij-storage.ts,src/features/tmux-subagent/manager.ts,src/create-managers.ts- security path handling and session management/detection logic need fixes before merge.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/shared/terminal-multiplexer/zellij-adapter.ts">
<violation number="1" location="src/shared/terminal-multiplexer/zellij-adapter.ts:39">
P2: `setSessionID` does not clear in-memory pane state when no persisted zellij state is found, allowing stale state to leak across sessions.</violation>
<violation number="2" location="src/shared/terminal-multiplexer/zellij-adapter.ts:201">
P2: Custom agent: **Opencode Compatibility**
Session ID extraction is too narrow (`[a-zA-Z0-9]+`) and can truncate valid Opencode IDs (e.g., with `-`/`_`), causing `closePane` to fail to kill the correct attach process.</violation>
</file>
<file name="src/shared/terminal-multiplexer/detection.test.ts">
<violation number="1" location="src/shared/terminal-multiplexer/detection.test.ts:110">
P2: The fallback test is vacuous: it accepts all possible return values, so it does not verify the claimed "returns null" behavior.</violation>
</file>
<file name="src/create-managers.ts">
<violation number="1" location="src/create-managers.ts:37">
P1: Auto-detection for Zellij is skipped when `terminal` config is omitted, causing default/no-config users to fall back to tmux instead of detecting Zellij.</violation>
</file>
<file name="src/shared/terminal-multiplexer/tmux-adapter.test.ts">
<violation number="1" location="src/shared/terminal-multiplexer/tmux-adapter.test.ts:41">
P2: The label-mapping test conditionally skips its key assertion when `nativeId` is absent, allowing false-positive passes without verifying mapping behavior.</violation>
<violation number="2" location="src/shared/terminal-multiplexer/tmux-adapter.test.ts:76">
P2: The `respects direction option` test is effectively a no-op for direction behavior because it asserts only label echo, not split direction effects.</violation>
<violation number="3" location="src/shared/terminal-multiplexer/tmux-adapter.test.ts:214">
P2: Tests use fixed tmux session names, which can collide with existing/concurrent sessions and lead to flaky or destructive test behavior.</violation>
</file>
<file name="src/shared/terminal-multiplexer/zellij-storage.ts">
<violation number="1" location="src/shared/terminal-multiplexer/zellij-storage.ts:23">
P1: Unsanitized `sessionID` is interpolated into storage file paths, enabling path traversal for read/write/delete operations.</violation>
<violation number="2" location="src/shared/terminal-multiplexer/zellij-storage.ts:51">
P2: `clearZellijState` has a TOCTOU race: `existsSync` followed by `unlinkSync` can throw if the file changes between calls.</violation>
</file>
<file name="src/features/tmux-subagent/manager.ts">
<violation number="1" location="src/features/tmux-subagent/manager.ts:500">
P1: Adapter-based session spawning is incorrectly gated by a tmux `sourcePaneId` check, causing early return before `spawnWithAdapter` in non-tmux environments.</violation>
<violation number="2" location="src/features/tmux-subagent/manager.ts:729">
P1: Adapter-spawned sessions are not tracked in `this.sessions`, so `onSessionDeleted` exits early and does not close/remove adapter panes on session deletion.</violation>
</file>
<file name="src/shared/terminal-multiplexer/zellij-adapter.test.ts">
<violation number="1" location="src/shared/terminal-multiplexer/zellij-adapter.test.ts:51">
P2: Cross-instance persistence tests are invalid because each adapter gets its own isolated mock storage instance.</violation>
</file>
<file name="src/shared/terminal-multiplexer/contract.test.ts">
<violation number="1" location="src/shared/terminal-multiplexer/contract.test.ts:21">
P2: `Bun.spawn` is mocked too late to isolate `TmuxAdapter` subprocess calls, since the adapter binds `spawn` at import time.</violation>
<violation number="2" location="src/shared/terminal-multiplexer/contract.test.ts:121">
P2: New contract tests contain vacuous assertions: direction behavior is not verified, and pane-label checks can pass on an empty pane list.</violation>
</file>
<file name="src/shared/terminal-multiplexer/tmux-adapter.ts">
<violation number="1" location="src/shared/terminal-multiplexer/tmux-adapter.ts:152">
P2: Comma-delimited pane parsing truncates titles containing commas, returning incorrect labels and storing wrong label-to-pane mappings.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
5 issues found across 20 files
Confidence score: 2/5
- High-confidence functional regressions are present in
src/features/tmux-subagent/manager.ts: the tmux-onlysourcePaneIdguard blocks adapter mode for non-tmux sessions, so Zellij/adapter flows cannot reachspawnWithAdapter. - There is a concrete cleanup/lifecycle risk in
src/features/tmux-subagent/manager.tswhere adapter-spawned panes may be left running onsession.deleteddue tosessionHandlesvssessionsmembership checks, which can cause leaked panes/resources. src/shared/terminal-multiplexer/zellij-adapter.tscan call stacking with an empty pane ID when pane capture fails, indicating a likely runtime failure path; test issues intmux-adapter.test.tsanddetection.test.tsfurther reduce confidence in catching regressions.- Pay close attention to
src/features/tmux-subagent/manager.tsandsrc/shared/terminal-multiplexer/zellij-adapter.ts- adapter-session routing, deletion cleanup, and pane-ID validation are the highest-impact risks.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/tmux-subagent/manager.ts">
<violation number="1" location="src/features/tmux-subagent/manager.ts:500">
P1: Adapter mode is blocked by a tmux-only `sourcePaneId` guard, so Zellij/non-tmux sessions can never reach `spawnWithAdapter`.</violation>
<violation number="2" location="src/features/tmux-subagent/manager.ts:501">
P2: Adapter-spawned panes are not closed on `session.deleted` because they are stored only in `sessionHandles` while `onSessionDeleted` gates on `sessions` membership.</violation>
</file>
<file name="src/shared/terminal-multiplexer/tmux-adapter.test.ts">
<violation number="1" location="src/shared/terminal-multiplexer/tmux-adapter.test.ts:170">
P2: Hard-coded tmux session names in tests can collide with real user sessions; `afterEach` unconditionally calls `killSession`, which will terminate any existing session with that name. Use randomized or uniquely prefixed names to avoid killing unrelated sessions.</violation>
</file>
<file name="src/shared/terminal-multiplexer/detection.test.ts">
<violation number="1" location="src/shared/terminal-multiplexer/detection.test.ts:110">
P3: This test does not validate the stated behavior; it accepts null, "tmux", or "zellij", so it can never fail and provides no regression protection for the fallback case.</violation>
</file>
<file name="src/shared/terminal-multiplexer/zellij-adapter.ts">
<violation number="1" location="src/shared/terminal-multiplexer/zellij-adapter.ts:175">
P2: Subsequent-pane stacking runs even when pane ID capture failed, allowing `stack-panes` to be called with an empty pane ID.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Guard paneId alongside anchorId before stacking panes (zellij-adapter.ts) - Fix adapter mode blocked by sourcePaneId guard in onSessionCreated (manager.ts) - Close adapter panes on session deletion in onSessionDeleted (manager.ts) - Fix comma-in-title truncation in tmux pane label parsing (tmux-adapter.ts) - Parameterise makeAdapter(storage?) to eliminate new as-any casts (zellij-adapter.test.ts) - Add command-aware mockSpawn and meaningful assertions (contract.test.ts) - Randomise session names to prevent cross-test pollution (tmux-adapter.test.ts) - Inject findBinaryImpl to make null-result assertion non-tautological (detection.ts/test.ts) Co-authored-by: David Laing <david@davidlaing.com>
|
@cubic-dev-ai Please re-review the latest commit. |
@florianschepp I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 22 files
Confidence score: 3/5
- Adapter session cleanup can be skipped in
onSessionDeletedwhensourcePaneIdis set, which may leaksessionHandles/pane handles and cause resource buildup over time. - Risk is moderate because the issue is user-impacting (resource leaks) and tied to session lifecycle handling.
- Pay close attention to
src/features/tmux-subagent/manager.ts- ensure session cleanup runs even whensourcePaneIdis set.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/features/tmux-subagent/manager.ts">
<violation number="1" location="src/features/tmux-subagent/manager.ts:703">
P1: Adapter session cleanup can be skipped in `onSessionDeleted` when `sourcePaneId` is set, leaking `sessionHandles`/pane handles.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
…ourcePaneId is set When both sourcePaneId and adapter are present, adapter sessions stored in sessionHandles were skipped by the sessions.get() guard, leaking pane handles. Add explicit adapter handle check before the tmux sessions path.
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
|
@cubic-dev-ai Fixed the P1 issue. Root cause: in Fix: added an explicit check before the tmux sessions path — if (this.adapter && this.sessionHandles.has(event.sessionID)) {
this.removeTrackedSession(event.sessionID)
return
}
|
@florianschepp This fix correctly addresses the handle leak I flagged. By adding the explicit check for I suggested this because, in hybrid environments or cases where With this change, I've verified the rest of the rebase and conflict resolutions in You can view your current configuration or manage repository-specific memory here: |
|
Could you take another look and let me know if there are any remaining issues to fix, or if this is ready for approval? Thanks! @cubic-dev-ai |
@florianschepp I have started the AI code review. It will take a few minutes to complete. |
|
@cubic-dev-ai you mention 4 unresolved issues from previous reviews, which ones are that? |
@florianschepp I previously flagged several issues across the rebased logic. After the latest fix for the handle leak in
You can manage these and other repository-specific rules in your Cubic Learnings. |
|
@cubic-dev-ai Thanks for the thorough follow-up! After carefully reviewing each of the 4 flagged issues against the actual code, I believe they're all either already handled or not applicable. Here's my reasoning: 1. Shell command construction (manager.ts:977) 2. Zellij storage path validation 3. hasCreatedFirstPane state after capturePaneId timeout 4. TmuxAdapter closePane logging Happy to revisit any of these if you see something I'm missing! |
@florianschepp You're right on these points — my previous assessment missed the existing safeguards and recovery logic you've implemented.
With the handle leak in You can view your current configuration or manage repository-specific memory here: |
|
Does that mean it can be approved? @cubic-dev-ai as this were the 4 outstanding issues you mentioned. |
@florianschepp I can only approve PRs when the request comes from a repository member. |
|
@cubic-dev-ai you mentioned auto approval of yours before a maintainer approves additionally. |
@florianschepp Cubic can formally approve a pull request on GitHub, but by default, this action is restricted to requests made by repository members. This is a security measure to prevent external contributors from triggering an automated bot approval on their own code. If you are a member of this repository and want me to submit a formal approval now that the blockers are resolved, you can simply comment Regarding "auto-approval":
Since I previously stated I don't see any remaining blockers, a manual Helpful links: |
|
@cubic-dev-ai review |
@florianschepp I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
2 issues found across 22 files
Confidence score: 2/5
- There is a high-confidence, high-severity risk in
src/shared/terminal-multiplexer/zellij-adapter.ts:sessionIdis interpolated into apkill -fregex without escaping, which can broaden matches and terminate unintended processes. src/features/tmux-subagent/manager.tsintroduces a behavior mismatch where adapter presence can bypasstmuxConfig.enabled, so panes may still spawn even when tmux is configured as disabled.- Given the concrete process-kill impact and an additional config-bypass issue, this carries elevated regression risk and is not a low-risk merge as-is.
- Pay close attention to
src/shared/terminal-multiplexer/zellij-adapter.tsandsrc/features/tmux-subagent/manager.ts- regex matching safety in kill commands and honoring disabled tmux configuration.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="src/shared/terminal-multiplexer/zellij-adapter.ts">
<violation number="1" location="src/shared/terminal-multiplexer/zellij-adapter.ts:280">
P1: Regex injection in `pkill -f` pattern: `sessionId` is interpolated into a regex without escaping special characters. If the label contains regex metacharacters (`.`, `*`, `+`, `(`, etc.), `pkill -9 -f` could match and kill unrelated processes. Escape the `sessionId` or use a fixed-string matching approach.</violation>
</file>
<file name="src/features/tmux-subagent/manager.ts">
<violation number="1" location="src/features/tmux-subagent/manager.ts:107">
P2: Adapter presence bypasses tmuxConfig.enabled, so disabled configuration can still spawn panes when a multiplexer adapter is created.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if (this.adapter) return true | ||
| return this.tmuxConfig.enabled && this.deps.isInsideTmux() |
There was a problem hiding this comment.
P2: Adapter presence bypasses tmuxConfig.enabled, so disabled configuration can still spawn panes when a multiplexer adapter is created.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/features/tmux-subagent/manager.ts, line 107:
<comment>Adapter presence bypasses tmuxConfig.enabled, so disabled configuration can still spawn panes when a multiplexer adapter is created.</comment>
<file context>
@@ -99,6 +104,7 @@ export class TmuxSessionManager {
})
}
private isEnabled(): boolean {
+ if (this.adapter) return true
return this.tmuxConfig.enabled && this.deps.isInsideTmux()
}
</file context>
| if (this.adapter) return true | |
| return this.tmuxConfig.enabled && this.deps.isInsideTmux() | |
| return this.tmuxConfig.enabled && (this.adapter !== null || this.deps.isInsideTmux()) |
sessionId is used as-is in a pkill -f regex, meaning metacharacters like ., *, (, ) could broaden the match and kill unrelated processes. Escape the sessionId before interpolation.
|
@cubic-dev-ai Thanks again! Here's how we addressed each: P1 — Fixed ✅ P2 — Respectfully disagree |
@florianschepp I appreciate the clarification on the P1 — Regex Escaping: The addition of the escaping logic in P2 — Enablement Logic: Your reasoning for keeping With these points addressed and the previous blockers resolved, I have no further concerns with the implementation. Helpful links: |
|
@cubic-dev-ai review |
@florianschepp I have started the AI code review. It will take a few minutes to complete. |
|
What are the 5 unresolved issues now? @cubic-dev-ai |
@florianschepp There are currently zero unresolved issues from my review. The "5 issues" you're likely referring to from our recent technical exchange—the 4 outstanding concerns I listed previously plus the handle leak in
As I mentioned in my previous comment, I have no further concerns with the implementation. If you are a repository maintainer, you can merge this at your convenience. Helpful links: |
|
@code-yeongyu WIth this, the PR is rebased and ready to be reviewed. If you need any changes please let me know. Thank you in advance! 🙌 |
|
@code-yeongyu can this get some traction? |
2 participants
Summary
This PR adds Zellij as an alternative terminal multiplexer, building on the excellent groundwork from @mrdavidlaing in #1226. That PR was closed for procedural reasons (head/base both pointed to
dev) — all credit for the original design and implementation goes to David.Improvements over #1226:
displayNamesupport: New optional field inSpawnOptions— used as the-nflag in Zellij so panes show"Agent: {title}"instead of raw session IDs-d downregardless of what the caller passes, ensuring consistent vertical stackingZellijAdapterconstructor accepts an optionalspawnFnfor clean unit testing without mockingbunsrc/config/schema/directory with individual schema files including newterminal.ts(ZellijConfigSchema,TerminalConfigSchemawithprovider: "auto" | "tmux" | "zellij")What's included
src/shared/terminal-multiplexer/— ZellijAdapter, types, detection, storage (David's files + improvements)src/config/schema/terminal.ts— new Zellij/terminal config schemassrc/create-managers.ts— async factory wiring zellij detectionsrc/features/tmux-subagent/manager.ts— overloaded constructor supporting both tmux and zellij adapterssrc/shared/terminal-multiplexer/zellij-adapter-stacking.test.ts— 8 passing testsAttribution
Co-authored-by: David Laing mrdavidlaing david@davidlaing.com
Original PR: #1226
Closes #1153