feat(zellij): per-agent placement + fresh-boot layout generation#2
Open
sealedsecurity-bot wants to merge 12 commits into
Open
feat(zellij): per-agent placement + fresh-boot layout generation#2sealedsecurity-bot wants to merge 12 commits into
sealedsecurity-bot wants to merge 12 commits into
Conversation
|
Warning Review limit reached
Next review available in: 44 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (31)
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:
📝 WalkthroughWalkthroughThis PR adds a new ChangesZellij runtime extension
Estimated code review effort: 5 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Manager
participant ZellijRuntime
participant Driver
participant zellij
Manager->>ZellijRuntime: spawn(name, spec, cwd, placement)
ZellijRuntime->>Driver: available()
ZellijRuntime->>Driver: ensureSession(session)
Driver->>zellij: attach --create-background
alt placement provided
ZellijRuntime->>Driver: goToTabNameCreate(session, tab)
ZellijRuntime->>Driver: newPane(session, argv, cwd, opts)
Driver->>zellij: new-pane
else no placement
ZellijRuntime->>Driver: openTab(session, name, argv, cwd)
Driver->>zellij: new-tab
end
Driver-->>ZellijRuntime: tab or pane id
ZellijRuntime-->>Manager: AgentHandle
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Greptile SummaryThis PR adds zellij runtime placement and cold-start layout support. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (10): Last reviewed commit: "ci(zellij): cache + SHA-256-verify the z..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
implementations/manager/src/launch.ts (1)
34-51: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider rejecting contradictory
stacked+floating(+direction) combinations at this validation boundary.Nothing here prevents a manifest from setting
stacked: trueandfloating: truetogether (or pairingdirectionwith either). Since this comment already framesplacementas the untrusted-input boundary, a.refine()catching mutually exclusive fields here would fail fast with a clear manifest error instead of pushing ambiguous precedence decisions onto the zellij driver.♻️ Suggested refine to reject conflicting placement flags
placement: z .strictObject({ tab: z .string() .min(1) .refine((s) => !s.startsWith("-") && !/[\x00-\x1f]/.test(s), { message: "placement.tab must be non-empty, not start with '-', and have no control chars", }) .optional(), stacked: z.boolean().optional(), floating: z.boolean().optional(), direction: z.enum(["right", "down"]).optional(), }) + .refine((p) => !(p.stacked && p.floating), { + message: "placement: stacked and floating are mutually exclusive", + }) .optional(),🤖 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 `@implementations/manager/src/launch.ts` around lines 34 - 51, The placement schema in launch.ts currently allows contradictory placement flags, so update the zod validation on the placement object to reject invalid combinations such as stacked with floating, and any direction value paired with floating or otherwise conflicting placement modes. Add a .refine() on the placement object near the existing placement.tab validation in the launch.ts schema so manifest errors fail fast with a clear message instead of leaving precedence ambiguous for the zellij driver.packages/core/src/runtime.ts (1)
47-60: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winUndocumented precedence when
stacked/floating/directionare combined.The downstream zellij runtime (context snippet,
spawnIntoTab) passesstacked,floating, anddirectionstraight through tozellij.newPanewithout this type constraining which one wins if a caller sets bothstacked: trueandfloating: true. A short JSDoc note on precedence (or restructuring as a discriminated shape) would prevent ambiguous manifests from silently producing unexpected pane layouts.📝 Suggested doc clarification
export interface Placement { /** Target tab by name; the tab is CREATED ON DEMAND if it doesn't exist. */ tab?: string; - /** Stacked pane within the tab (the lane default). */ + /** Stacked pane within the tab (the lane default). Mutually exclusive with `floating`/`direction` — + * if more than one is set, [document precedence rule here]. */ stacked?: boolean; /** Floating pane. */ floating?: boolean; /** Split direction when the pane is neither stacked nor floating. */ direction?: "right" | "down"; }🤖 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 `@packages/core/src/runtime.ts` around lines 47 - 60, Clarify the precedence rules for Placement when callers provide multiple layout flags at once. Update the JSDoc on Placement to document which of stacked, floating, or direction takes effect when more than one is set, and ensure the description matches how spawnIntoTab forwards these fields to zellij.newPane so ambiguous manifests are not left implicit. If needed, refactor Placement into a discriminated shape to make the valid combinations explicit and prevent conflicting values.implementations/cli/src/lib/manifest/apply.ts (1)
23-35: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCanonicalize
placementkey order inhashAgentto prevent spurious restarts.All other nested fields in the stable hash are canonicalized (arrays are
.sort()ed), butplacementis serialized as-is. SinceJSON.stringifypreserves key insertion order and Zod'sstrictObjectdoes not reorder keys, reordering placement keys in the YAML (e.g.,directionbeforetab) would change the hash and trigger an unnecessary agent restart.♻️ Proposed fix: sort placement keys before hashing
export function hashAgent(a: PreparedAgent): string { const stable = JSON.stringify({ agent: a.agentType, model: a.model ?? null, role: a.role ?? null, body: a.body ?? null, capabilities: [...a.capabilities].sort(), subscribe: [...a.policy.subscribe].sort(), allowSubscribe: [...a.policy.allowSubscribe].sort(), allowPublish: [...a.policy.allowPublish].sort(), - placement: a.placement ?? null, + placement: a.placement + ? Object.fromEntries(Object.entries(a.placement).sort(([k1], [k2]) => k1.localeCompare(k2))) + : null, }); return createHash("sha256").update(stable).digest("hex").slice(0, 16); }🤖 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 `@implementations/cli/src/lib/manifest/apply.ts` around lines 23 - 35, `hashAgent` is hashing `placement` directly, so different YAML key orders can produce different hashes and trigger unnecessary restarts. Update the `hashAgent` stable payload to canonicalize `a.placement` before `JSON.stringify`, matching the existing sorting used for `capabilities` and policy arrays. Keep the change localized to `hashAgent` in the manifest apply logic so the hash stays stable regardless of `placement` key insertion order.
🤖 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 `@extensions/zellij/src/runtime.ts`:
- Around line 90-131: The tab status check in tabHandle still relies on
tabNames(...).includes(name), which can misreport a renamed tab as exited even
when it is still running. Update status() in tabHandle to use the stable tabId
and align it with the id-based close path already used by stop(), while leaving
interrupt() name-based if no id-based focus API exists.
In `@implementations/cli/smoke/manifest.smoke.ts`:
- Around line 157-168: The unknown-runtime rejection test in fails is too weak
because the empty needle always passes and only proves a ManifestError was
thrown. Update the assertion in the manifest smoke test to use a specific
substring tied to the enum validation path, such as the runtime value or the
invalid-enum message, so the check in fails actually verifies the expected
rejection from the manifest parsing logic.
In `@implementations/manager/src/launch.ts`:
- Around line 34-51: The placement schema in launch.ts has stricter tab
validation than the manifest schema, so keep both validators aligned. Update the
tab validation in the relevant placement schema(s) to use the same constraints
as the launch path: non-empty, no leading dash, and no control characters, using
the existing placement/tab validation logic in the manifest schema and the
launch validation for consistency.
---
Nitpick comments:
In `@implementations/cli/src/lib/manifest/apply.ts`:
- Around line 23-35: `hashAgent` is hashing `placement` directly, so different
YAML key orders can produce different hashes and trigger unnecessary restarts.
Update the `hashAgent` stable payload to canonicalize `a.placement` before
`JSON.stringify`, matching the existing sorting used for `capabilities` and
policy arrays. Keep the change localized to `hashAgent` in the manifest apply
logic so the hash stays stable regardless of `placement` key insertion order.
In `@implementations/manager/src/launch.ts`:
- Around line 34-51: The placement schema in launch.ts currently allows
contradictory placement flags, so update the zod validation on the placement
object to reject invalid combinations such as stacked with floating, and any
direction value paired with floating or otherwise conflicting placement modes.
Add a .refine() on the placement object near the existing placement.tab
validation in the launch.ts schema so manifest errors fail fast with a clear
message instead of leaving precedence ambiguous for the zellij driver.
In `@packages/core/src/runtime.ts`:
- Around line 47-60: Clarify the precedence rules for Placement when callers
provide multiple layout flags at once. Update the JSDoc on Placement to document
which of stacked, floating, or direction takes effect when more than one is set,
and ensure the description matches how spawnIntoTab forwards these fields to
zellij.newPane so ambiguous manifests are not left implicit. If needed, refactor
Placement into a discriminated shape to make the valid combinations explicit and
prevent conflicting values.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a65e494-d39c-4170-9c09-6a25048ce3cc
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (26)
.changeset/config.json.changeset/zellij-runtime.md.github/workflows/ci.ymlextensions/cmux/src/runtime.tsextensions/tmux/src/runtime.tsextensions/zellij/README.mdextensions/zellij/package.jsonextensions/zellij/smoke.tsextensions/zellij/src/driver.tsextensions/zellij/src/index.tsextensions/zellij/src/layout-map.tsextensions/zellij/src/runtime.tsextensions/zellij/tsconfig.jsonimplementations/cli/smoke/manifest.smoke.tsimplementations/cli/src/lib/manifest/apply.tsimplementations/cli/src/lib/manifest/model.tsimplementations/cli/src/lib/manifest/prepare.tsimplementations/cli/src/lib/manifest/resolve.tsimplementations/cli/src/lib/manifest/schema.tsimplementations/manager/src/commands.tsimplementations/manager/src/launch.tsimplementations/manager/src/manager.tsimplementations/manager/src/runtime/pty.tspackage.jsonpackages/core/src/launch.tspackages/core/src/runtime.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
11 issues found across 27 files
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="packages/core/src/runtime.ts">
<violation number="1" location="packages/core/src/runtime.ts:51">
P2: `Placement` fields `stacked`, `floating`, and `direction` are independent optionals, but they're mutually exclusive (the driver resolves them in priority order: stacked > floating > direction). A caller passing e.g. `{ floating: true, direction: "right" }` silently ignores `direction`, which is confusing and may lead to layout bugs in manifests. Consider modeling as a discriminated union to enforce exclusivity at the type level.</violation>
</file>
<file name="implementations/manager/src/launch.ts">
<violation number="1" location="implementations/manager/src/launch.ts:38">
P3: Empty `placement: {}` passes validation but carries no actionable shape info — when tab is absent, the runtime falls back to the default per-agent-tab path, so the empty wrapper is misleading. Consider a `.refine` that requires at least one of `tab`, `stacked`, `floating`, or `direction` (or accept the trade-off explicitly).</violation>
</file>
<file name="extensions/zellij/src/runtime.ts">
<violation number="1" location="extensions/zellij/src/runtime.ts:94">
P2: A live zellij agent tab can be reported as exited after the tab is renamed because `status()` checks the mutable tab name instead of the `tabId` captured at spawn. Since the default path already stores the stable id for lifecycle, status should use an id-based existence check as well.</violation>
<violation number="2" location="extensions/zellij/src/runtime.ts:221">
P1: `zellijLayout` uses `scheduleConfirm` (tab-scoped) for all pane confirms, but after all panes open the **last-focused pane** receives the Enter keypress — the first pane's confirmation prompt is never dismissed.</violation>
</file>
<file name="implementations/manager/src/commands.ts">
<violation number="1" location="implementations/manager/src/commands.ts:319">
P1: The stock `cotal` CLI now accepts `--runtime zellij`, but the zellij runtime is not registered in the published composition root, so this option can pass validation and then fail at manager startup with an unknown-runtime error. Consider importing `@cotal-ai/zellij` (and adding it as a dependency) alongside the tmux/cmux integrations before advertising it in `RUNTIME_OVERRIDES`.</violation>
</file>
Heads up: you’re close to your included review allowance. Set a flex budget so reviews don’t pause.
Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@extensions/zellij/src/layout-map.ts`:
- Around line 81-93: The plugin-frame skip in layout-map.ts is too broad and can
incorrectly swallow real content panes that happen to have size/borderless
attributes. Update the guard around the inPluginPane handling to only enter that
path for actual plugin panes by checking for a plugin-specific marker such as a
plugin child before setting inPluginPane. Keep the existing
pluginDepth/brace-walking logic in place, but make the initial detection
stricter so valid command panes still preserve their cwd and args.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10b61ba1-80e5-47e9-a657-8846006336a2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (7)
bin/cotal.tsbin/package.jsonextensions/zellij/smoke.tsextensions/zellij/src/layout-map.tsimplementations/cli/smoke/manifest.smoke.tsimplementations/cli/src/lib/manifest/schema.tsimplementations/manager/src/launch.ts
✅ Files skipped from review due to trivial changes (2)
- bin/cotal.ts
- bin/package.json
🚧 Files skipped from review as they are similar to previous changes (3)
- implementations/manager/src/launch.ts
- implementations/cli/src/lib/manifest/schema.ts
- extensions/zellij/smoke.ts
There was a problem hiding this comment.
5 issues found across 8 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="extensions/zellij/smoke.ts">
<violation number="1" location="extensions/zellij/smoke.ts:117">
P2: The real dump-layout test includes a layout-level `cwd "/home/mattw"` alongside a relative pane `cwd="agents/workspaces/zheng"`, but only asserts the raw relative pane value. Zellij's documented `cwd` composition semantics mean that relative pane `cwd` values are resolved against their container's `cwd` (pane → tab → global cwd). The current `seedFromDump` implementation does not parse the layout-level `cwd`, `LayoutMap` has no field to store it, and `generateKdl` does not emit it. This means a round-trip through `seedFromDump` → `generateKdl` would lose the absolute base directory and the regenerated pane would resolve to a different effective working directory. Consider either (a) adding a top-level `cwd` field to `LayoutMap` and handling it in both `seedFromDump` and `generateKdl`, or (b) resolving relative pane `cwd` values against the layout-level `cwd` at parse time so they round-trip correctly.</violation>
<violation number="2" location="extensions/zellij/smoke.ts:160">
P2: The global exit cleanup hook only tears down the main smoke session, so the placement section can still leak the `cotal-zellij-smoke-placement` session and its detached client process on abrupt termination. `process.on("exit")` does not fire for SIGINT/SIGTERM, and even when it does fire (normal exit, `process.exit`, uncaught exceptions), `cleanup()` does not know about the placement session. Consider extending `cleanup()` to also delete the placement session, or add SIGINT/SIGTERM handlers, so the "no stray sessions" promise holds across all exit paths.</violation>
</file>
<file name="extensions/zellij/src/layout-map.ts">
<violation number="1" location="extensions/zellij/src/layout-map.ts:85">
P2: The plugin-frame detection heuristic (`(?:size|borderless)=`) is too broad. Zellij's layout format allows `size` and `borderless` on any pane type — a content pane like `pane size="50%" command="vim" { args "file.txt" }` would be incorrectly skipped, dropping it from the seeded layout map. Consider requiring evidence of an actual `plugin` child node (e.g., peek ahead or check for `plugin location=`) before marking a pane as a non-content frame to skip.</violation>
<violation number="2" location="extensions/zellij/src/layout-map.ts:124">
P2: The bare-pane detection branch in `seedFromDump` misclassifies structural zellij container panes (e.g. `pane split_direction="vertical" { ... }`) as empty content panes because `split_direction` is not included in the attribute denylist. This causes an extra empty shell pane to be emitted on layout regeneration. Consider adding `split_direction` (and likely any pane that opens `{` without being one of the explicitly-handled forms) to the exclusion list so that structural wrappers are not recorded as content panes.</violation>
</file>
<file name="packages/core/src/runtime.ts">
<violation number="1" location="packages/core/src/runtime.ts:51">
P2: `Placement` fields `stacked`, `floating`, and `direction` are independent optionals, but they're mutually exclusive (the driver resolves them in priority order: stacked > floating > direction). A caller passing e.g. `{ floating: true, direction: "right" }` silently ignores `direction`, which is confusing and may lead to layout bugs in manifests. Consider modeling as a discriminated union to enforce exclusivity at the type level.</violation>
</file>
<file name="implementations/manager/src/launch.ts">
<violation number="1" location="implementations/manager/src/launch.ts:38">
P3: Empty `placement: {}` passes validation but carries no actionable shape info — when tab is absent, the runtime falls back to the default per-agent-tab path, so the empty wrapper is misleading. Consider a `.refine` that requires at least one of `tab`, `stacked`, `floating`, or `direction` (or accept the trade-off explicitly).</violation>
</file>
<file name="implementations/manager/src/commands.ts">
<violation number="1" location="implementations/manager/src/commands.ts:319">
P1: The stock `cotal` CLI now accepts `--runtime zellij`, but the zellij runtime is not registered in the published composition root, so this option can pass validation and then fail at manager startup with an unknown-runtime error. Consider importing `@cotal-ai/zellij` (and adding it as a dependency) alongside the tmux/cmux integrations before advertising it in `RUNTIME_OVERRIDES`.</violation>
</file>
<file name="implementations/cli/src/lib/manifest/schema.ts">
<violation number="1" location="implementations/cli/src/lib/manifest/schema.ts:38">
P2: The `placement.tab` refine claims to reject strings with 'no control chars', but the regex `[/\x00-\x1f/]` only matches C0 controls (U+0000–U+001F). DEL (U+007F) and C1 controls (U+0080–U+009F) are still Unicode control characters and pass validation. Because tab names flow into generated KDL layout output where `escapeKdl` does not escape control characters, the schema's incomplete check provides a false guarantee. Consider using `\p{Cc}` with the `u` flag to match all Unicode control characters, or explicitly add `\x7f` and `\x80-\x9f` to the regex.</violation>
</file>
Tip: instead of fixing issues one by one fix them all with cubic
Re-trigger cubic
There was a problem hiding this comment.
4 issues found across 3 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="packages/core/src/runtime.ts">
<violation number="1" location="packages/core/src/runtime.ts:51">
P2: `Placement` fields `stacked`, `floating`, and `direction` are independent optionals, but they're mutually exclusive (the driver resolves them in priority order: stacked > floating > direction). A caller passing e.g. `{ floating: true, direction: "right" }` silently ignores `direction`, which is confusing and may lead to layout bugs in manifests. Consider modeling as a discriminated union to enforce exclusivity at the type level.</violation>
</file>
<file name="implementations/manager/src/launch.ts">
<violation number="1" location="implementations/manager/src/launch.ts:38">
P3: Empty `placement: {}` passes validation but carries no actionable shape info — when tab is absent, the runtime falls back to the default per-agent-tab path, so the empty wrapper is misleading. Consider a `.refine` that requires at least one of `tab`, `stacked`, `floating`, or `direction` (or accept the trade-off explicitly).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@extensions/zellij/smoke.ts`:
- Around line 272-277: The rename regression block still runs client-dependent
tab renaming even when ensureClient(SESSION) returns false, which can make the
smoke fail in environments without script. Update the smoke test around
ensureClient, goToTabName, and the rename-tab execFileSync call to mirror the
placement subtest: if ensureClient(SESSION) is false, skip this regression check
instead of continuing with the rename.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 27d9c247-d065-4c32-837e-432e867b6644
📒 Files selected for processing (6)
extensions/zellij/smoke.tsextensions/zellij/src/driver.tsextensions/zellij/src/layout-map.tsextensions/zellij/src/runtime.tsimplementations/cli/src/lib/manifest/schema.tsimplementations/manager/src/launch.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- implementations/manager/src/launch.ts
- implementations/cli/src/lib/manifest/schema.ts
- extensions/zellij/src/runtime.ts
- extensions/zellij/src/layout-map.ts
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
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="extensions/zellij/smoke.ts">
<violation number="1" location="extensions/zellij/smoke.ts:281">
P2: The newly added conditional skip for the rename regression means that in environments without the `script` utility, the tab is never renamed from `"smoke-agent"` to `"smoke-renamed"`. However, the later assertion `"renamed tab gone after hard stop"` still unconditionally checks for the absence of `"smoke-renamed"`, which is trivially true when the rename was skipped. This weakens the smoke test in those environments: if `handle.stop()` were to silently fail to remove the original tab, the test would still pass because it only asserts against a name that never existed. Consider tracking whether the rename was performed and matching the assertion to the actual tab name (or skipping that assertion in the skip path), so the test verifies session-level cleanup regardless of the host environment.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 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="packages/core/src/runtime.ts">
<violation number="1" location="packages/core/src/runtime.ts:51">
P2: `Placement` fields `stacked`, `floating`, and `direction` are independent optionals, but they're mutually exclusive (the driver resolves them in priority order: stacked > floating > direction). A caller passing e.g. `{ floating: true, direction: "right" }` silently ignores `direction`, which is confusing and may lead to layout bugs in manifests. Consider modeling as a discriminated union to enforce exclusivity at the type level.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Fix all with cubic | Re-trigger cubic
Spawns each agent into its own zellij tab in a shared per-space background session (env -i isolation), plus a TerminalLayout provider for cotal setup. Self-registers on import like @cotal-ai/tmux; select with --runtime zellij. Structural argv over zellij's control socket keeps secret env off dump-layout/ps. Co-Authored-By: seal <noreply@sealedsecurity.com>
Adds an optional `placement` arg to `Runtime.spawn` (target tab by name, created on demand; stacked/floating/split) so a wave lands each agent as a pane in a named lane tab; only zellij reads it, the other runtimes accept and ignore it. Threads `placement` through the manifest -> resolve -> spawn chain and adds a pure `layout-map` (`seedFromDump`/`generateKdl`) for full-session KDL boot. Co-Authored-By: seal <noreply@sealedsecurity.com>
`seedFromDump` only captured `command=`, dropping the `args "…"` child node and the inline `cwd="…"` pane attribute — so a seeded `omp --resume` regenerated as a bare `omp`. Parse both (and skip the stacked/plugin wrapper panes); add args round-trip + real-dump-grammar assertions. Co-Authored-By: seal <noreply@sealedsecurity.com>
…validation
Add `@cotal-ai/zellij` to the `cotal` binary's composition root so `--runtime zellij` resolves
instead of throwing `unknown runtime` at manager startup. Guard `seedFromDump` against `cwd`/`args`
leaking out of skipped plugin frames, make placement shape (stacked/floating/direction) mutually
exclusive and mirror the tab-name flag-injection guard into the manifest schema, and fix a
vacuous `includes("")` smoke assertion.
Co-Authored-By: seal <noreply@sealedsecurity.com>
…firm Placement pane-id ops silently no-op against a client-less background session, so a placed agent's pane never spawned and read as `exited`; `spawnIntoTab` now attaches a headless PTY client first (`ensureClient`). Status checks the stable tab id (`tabExists` via `list-tabs`) instead of the mutable tab name, so a renamed tab no longer reports exited, and multi-pane layouts route each confirm to its own pane id rather than a tab-focus that only reaches the last pane. All three verified against live zellij 0.44.3. Co-Authored-By: seal <noreply@sealedsecurity.com>
…d 3)
`ensureClient` now passes the attach command through `script`'s structural `-- <cmd>` form (no shell
interpolation of the session name), attaches an `error` listener so a missing `script` can't crash
Node on the async ENOENT, and returns a bool so `spawnIntoTab` fails loud instead of spawning a pane
that silently reads as exited. `seedFromDump` classifies panes structurally — a plugin frame needs an
actual `plugin` child (not just size/borderless, which real content panes carry), block-opening panes
(split_direction/stacked wrappers) are never leaf content, and the layout-level `cwd` round-trips.
Tab-name control-char guard widened to `\p{Cc}`; confirm-routing smoke assertion fixed to check the
lane panes, not the ad-hoc tab.
Co-Authored-By: seal <noreply@sealedsecurity.com>
The rename-under-handle smoke test needs an attached client, but ran unconditionally even when `ensureClient` returns false (no `script`), failing on client-less boxes. Gate it on the boolean like the placement subtest already does. Co-Authored-By: seal <noreply@sealedsecurity.com>
The structural `script … -- command` form needs util-linux >=2.40 and silently no-ops on older `script` (Ubuntu 24.04 / the CI runner ships 2.39), so no client attached and placement threw. Use the universally-supported `-qec "<cmd>" /dev/null` string form, validating the session name against a metacharacter-free charset (matching the runtime's agent-name guard) to keep the shell path injection-safe. Co-Authored-By: seal <noreply@sealedsecurity.com>
Tighten `scriptAttachArgv`'s charset so a session name can't start with `-` (else `zellij attach` reads it as a CLI option under the `-c` shell). Track the smoke's live tab name so the post-stop "tab gone" assertion checks the actual name in both the client and script-absent skip paths. Co-Authored-By: seal <noreply@sealedsecurity.com>
Add a Zod refine to the manifest and launch schemas rejecting an empty `placement: {}` (a no-op
wrapper) alongside the existing mutual-exclusion guard, and document on the `Placement` type that
stacked/floating/direction are exclusive with the driver's stacked>floating>direction precedence.
Co-Authored-By: seal <noreply@sealedsecurity.com>
seal-agent
force-pushed
the
zheng-zellij-runtime
branch
from
a5cd46e to
a53bdfe
Compare
T5 taught the manifest schema enum, the manager RUNTIME_OVERRIDES, and the ResolvedManifest union about zellij, but three CLI-command-level allow-lists still hardcoded [pty, tmux, cmux] — so `cotal up -f … --runtime zellij` (the design's headline invocation) and `spawn -f --runtime zellij` were rejected before the value reached the runtime, and tab-completion omitted it. Add zellij to up.ts validation, spawn-manifest RUNTIMES, and spawn.ts completion. The manifest `runtime: zellij` field already flowed through; this fixes the flag override + completion to match. Co-Authored-By: seal <noreply@sealedsecurity.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
107-115: 🔒 Security & Privacy | 🔵 TrivialConsider verifying the downloaded binary and/or caching it.
The
zellijbinary is fetched from GitHub releases on everyliverun with no integrity check, unlikenats-serverwhich is cached. Two optional hardening steps:
- Verify a pinned SHA-256 of the tarball before
installto guard against a tampered/partial download (supply-chain integrity).- Cache
zellijviaactions/cache(as done fornats-server) to cut download time and reduce dependence on the release CDN during CI.🤖 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 107 - 115, The Install zellij step currently downloads and installs the binary without integrity verification or caching; update the workflow to harden this by adding a pinned checksum check for the tarball before the install in the zellij install block, and consider adding an actions/cache entry for the downloaded zellij artifact similar to the nats-server setup. Use the existing Install zellij step and the zellij --version verification as the place to wire in the download validation/caching changes.Source: Linters/SAST tools
🤖 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 `@implementations/cli/src/commands/spawn.ts`:
- Line 182: Update the `runtime` flag spec in `spawn.ts` so the `value` hint
matches the accepted runtimes elsewhere; the current `--help` text omits
`zellij` even though `spawn-manifest.ts` validation and the completion list
already allow it. Adjust the `name: "runtime"` entry to include `zellij` in the
displayed option set so users see the full valid list.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 107-115: The Install zellij step currently downloads and installs
the binary without integrity verification or caching; update the workflow to
harden this by adding a pinned checksum check for the tarball before the install
in the zellij install block, and consider adding an actions/cache entry for the
downloaded zellij artifact similar to the nats-server setup. Use the existing
Install zellij step and the zellij --version verification as the place to wire
in the download validation/caching changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 37da87ee-b176-473b-952d-6ce6633ef49a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
.changeset/config.json.changeset/zellij-runtime.md.github/workflows/ci.ymlbin/cotal.tsbin/package.jsonextensions/cmux/src/runtime.tsextensions/tmux/src/runtime.tsextensions/zellij/README.mdextensions/zellij/package.jsonextensions/zellij/smoke.tsextensions/zellij/src/driver.tsextensions/zellij/src/index.tsextensions/zellij/src/layout-map.tsextensions/zellij/src/runtime.tsextensions/zellij/tsconfig.jsonimplementations/cli/smoke/manifest.smoke.tsimplementations/cli/src/commands/spawn-manifest.tsimplementations/cli/src/commands/spawn.tsimplementations/cli/src/commands/up.tsimplementations/cli/src/lib/manifest/apply.tsimplementations/cli/src/lib/manifest/model.tsimplementations/cli/src/lib/manifest/prepare.tsimplementations/cli/src/lib/manifest/resolve.tsimplementations/cli/src/lib/manifest/schema.tsimplementations/manager/src/commands.tsimplementations/manager/src/launch.tsimplementations/manager/src/manager.tsimplementations/manager/src/runtime/pty.tspackage.jsonpackages/core/src/launch.tspackages/core/src/runtime.ts
💤 Files with no reviewable changes (2)
- packages/core/src/launch.ts
- packages/core/src/runtime.ts
✅ Files skipped from review due to trivial changes (6)
- .changeset/config.json
- extensions/zellij/README.md
- .changeset/zellij-runtime.md
- implementations/cli/src/lib/manifest/prepare.ts
- extensions/zellij/package.json
- extensions/zellij/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (16)
- bin/package.json
- implementations/cli/src/lib/manifest/resolve.ts
- implementations/manager/src/runtime/pty.ts
- extensions/cmux/src/runtime.ts
- implementations/cli/src/lib/manifest/schema.ts
- package.json
- implementations/manager/src/manager.ts
- bin/cotal.ts
- implementations/manager/src/launch.ts
- implementations/cli/src/lib/manifest/apply.ts
- implementations/manager/src/commands.ts
- implementations/cli/smoke/manifest.smoke.ts
- extensions/tmux/src/runtime.ts
- extensions/zellij/src/index.ts
- extensions/zellij/src/runtime.ts
- extensions/zellij/src/layout-map.ts
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/ci.yml (1)
107-115: 🔒 Security & Privacy | 🔵 TrivialConsider verifying the downloaded binary and/or caching it.
The
zellijbinary is fetched from GitHub releases on everyliverun with no integrity check, unlikenats-serverwhich is cached. Two optional hardening steps:
- Verify a pinned SHA-256 of the tarball before
installto guard against a tampered/partial download (supply-chain integrity).- Cache
zellijviaactions/cache(as done fornats-server) to cut download time and reduce dependence on the release CDN during CI.🤖 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 107 - 115, The Install zellij step currently downloads and installs the binary without integrity verification or caching; update the workflow to harden this by adding a pinned checksum check for the tarball before the install in the zellij install block, and consider adding an actions/cache entry for the downloaded zellij artifact similar to the nats-server setup. Use the existing Install zellij step and the zellij --version verification as the place to wire in the download validation/caching changes.Source: Linters/SAST tools
🤖 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 `@implementations/cli/src/commands/spawn.ts`:
- Line 182: Update the `runtime` flag spec in `spawn.ts` so the `value` hint
matches the accepted runtimes elsewhere; the current `--help` text omits
`zellij` even though `spawn-manifest.ts` validation and the completion list
already allow it. Adjust the `name: "runtime"` entry to include `zellij` in the
displayed option set so users see the full valid list.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 107-115: The Install zellij step currently downloads and installs
the binary without integrity verification or caching; update the workflow to
harden this by adding a pinned checksum check for the tarball before the install
in the zellij install block, and consider adding an actions/cache entry for the
downloaded zellij artifact similar to the nats-server setup. Use the existing
Install zellij step and the zellij --version verification as the place to wire
in the download validation/caching changes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 37da87ee-b176-473b-952d-6ce6633ef49a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
.changeset/config.json.changeset/zellij-runtime.md.github/workflows/ci.ymlbin/cotal.tsbin/package.jsonextensions/cmux/src/runtime.tsextensions/tmux/src/runtime.tsextensions/zellij/README.mdextensions/zellij/package.jsonextensions/zellij/smoke.tsextensions/zellij/src/driver.tsextensions/zellij/src/index.tsextensions/zellij/src/layout-map.tsextensions/zellij/src/runtime.tsextensions/zellij/tsconfig.jsonimplementations/cli/smoke/manifest.smoke.tsimplementations/cli/src/commands/spawn-manifest.tsimplementations/cli/src/commands/spawn.tsimplementations/cli/src/commands/up.tsimplementations/cli/src/lib/manifest/apply.tsimplementations/cli/src/lib/manifest/model.tsimplementations/cli/src/lib/manifest/prepare.tsimplementations/cli/src/lib/manifest/resolve.tsimplementations/cli/src/lib/manifest/schema.tsimplementations/manager/src/commands.tsimplementations/manager/src/launch.tsimplementations/manager/src/manager.tsimplementations/manager/src/runtime/pty.tspackage.jsonpackages/core/src/launch.tspackages/core/src/runtime.ts
💤 Files with no reviewable changes (2)
- packages/core/src/launch.ts
- packages/core/src/runtime.ts
✅ Files skipped from review due to trivial changes (6)
- .changeset/config.json
- extensions/zellij/README.md
- .changeset/zellij-runtime.md
- implementations/cli/src/lib/manifest/prepare.ts
- extensions/zellij/package.json
- extensions/zellij/tsconfig.json
🚧 Files skipped from review as they are similar to previous changes (16)
- bin/package.json
- implementations/cli/src/lib/manifest/resolve.ts
- implementations/manager/src/runtime/pty.ts
- extensions/cmux/src/runtime.ts
- implementations/cli/src/lib/manifest/schema.ts
- package.json
- implementations/manager/src/manager.ts
- bin/cotal.ts
- implementations/manager/src/launch.ts
- implementations/cli/src/lib/manifest/apply.ts
- implementations/manager/src/commands.ts
- implementations/cli/smoke/manifest.smoke.ts
- extensions/tmux/src/runtime.ts
- extensions/zellij/src/index.ts
- extensions/zellij/src/runtime.ts
- extensions/zellij/src/layout-map.ts
🛑 Comments failed to post (1)
implementations/cli/src/commands/spawn.ts (1)
182-182: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the
--runtimeflag value hint to includezellij.The completion list (line 73) and validation in
spawn-manifest.tsboth acceptzellij, but the flag spec'svaluefield still shows<pty|tmux|cmux>. Users reading--helpwon't seezellijas a valid option.✏️ Proposed fix
- { name: "runtime", type: "string", value: "<pty|tmux|cmux>", description: "with -f: override the manifest's runtime" }, + { name: "runtime", type: "string", value: "<pty|tmux|cmux|zellij>", description: "with -f: override the manifest's runtime" },📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.{ name: "runtime", type: "string", value: "<pty|tmux|cmux|zellij>", description: "with -f: override the manifest's runtime" },🧰 Tools
🪛 ast-grep (0.44.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn as spawnProcess } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').(detect-child-process-typescript)
🤖 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 `@implementations/cli/src/commands/spawn.ts` at line 182, Update the `runtime` flag spec in `spawn.ts` so the `value` hint matches the accepted runtimes elsewhere; the current `--help` text omits `zellij` even though `spawn-manifest.ts` validation and the completion list already allow it. Adjust the `name: "runtime"` entry to include `zellij` in the displayed option set so users see the full valid list.
Mirror the nats-server pattern for the zellij install: cache it via actions/cache keyed on version+OS, and verify a pinned SHA-256 of the release tarball before install (supply-chain integrity). Addresses a CodeRabbit review nit on the live-runner install step. No behavior change — zellij is still on PATH for the smoke. Co-Authored-By: seal <noreply@sealedsecurity.com>
|
@coderabbitai review The ci.yml zellij-install nitpick (verify + cache the binary) is addressed in d611228 — mirrors the nats-server pattern: |
|
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
|
@coderabbitai review Head is now |
|
✅ Action performedReview finished.
|
|
@coderabbitai full review Head |
|
✅ Action performedFull review finished. Your plan includes PR reviews subject to rate limits. More reviews will be available in 44 minutes. |
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
3 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.
What
Implements the zellij runtime placement + fresh-boot layout mechanism frozen in the design record (
docs/designs/agents/zellij-runtime-placement.md, zireael#254): a wave lands each agent as a pane in a named lane tab of one shared zellij session, and a full-session layout can be regenerated for a cold restart.Builds on the base
@cotal-ai/zellijextension with the placement seam.Changes
Runtime.spawngains an optional 4th argplacement?: Placement({ tab?, stacked?, floating?, direction? },packages/core/src/runtime.ts).PtyRuntime/TmuxRuntime/CmuxRuntimeaccept and ignore it; only zellij reads it, so a manifest stays valid under any backend.goToTabNameCreate,newPane({stacked,floating,direction}),closePaneById(idempotent),paneExists, plustabExists/hasClient/ensureClient/focusedPaneIdadded in review (extensions/zellij/src/driver.ts).ZellijRuntime.spawnwithplacement.tabcreates/focuses the tab then opens a pane, building the handle off the pane id. No placement → the existing per-agent-tab path, whose handle now keysstatusoff the stable tab id.extensions/zellij/src/layout-map.ts:seedFromDump(dumpKdl)parsesdump-layoutinto aLayoutMap(command + inlinecwd+argschild, skipping plugin frames);generateKdl(map)emits a full-session KDL (+new_tab_template) that boots via top-levelzellij --layout."zellij"added toRUNTIME_OVERRIDES, both manifest allow-lists, and thebin/cotal.tscomposition root so--runtime zellijresolves instead of throwing at startup.placementthreads resolve →MeshLaunchAgent→ managerStartOpts→spawn(name, spec, cwd, placement).smoke:zellij= 57 assertions (pure layout-map units + live placement, rename-survival, per-pane confirm) + manifest-resolve/refine assertions. CI installs zellij 0.44.3 +script(util-linux).Review fixes (rounds 1–3, all bots)
--runtime zellijadvertised but@cotal-ai/zellijwasn't imported inbin/cotal.ts→ resolved-at-startup. Fixed + lockfile updated.spawnIntoTabnowensureClients a headless PTY client first. Verified live: placed panes really spawn.scheduleConfirmPane) instead of a tab-focus that only reached the last pane.seedFromDumpdroppedargs+ inlinecwd; now round-trips both, and skipscwd/argsleaking out of plugin frames.status()now checks the stable tab id (tabExists), surviving a tab rename.stacked/floating/direction) made mutually exclusive in both Zod schemas; tab-name flag-injection guard mirrored into the manifest schema; vacuousincludes("")test fixed.Placementas a discriminated union at the type level — SEA-1159, post-MVP.Verification
pnpm typecheck(all 16 workspaces) — exit 0pnpm test— exit 0 (manifest pipeline smoke ok, incl. placement resolve + refine assertions)pnpm smoke:zellij— 57 passed, 0 failed against live zellij 0.44.3; leaves no stray sessions/processes (named-target teardown only)Refs zireael#254
Co-Authored-By: seal noreply@sealedsecurity.com