Repository navigation
Fix all Codex adversarial review findings - #7
Conversation
Address 10 issues identified across IPC safety, memory correctness, protocol compliance, and architectural concerns: - Fix IPC readMessage partial read desync on non-blocking sockets - Fix sendRequest to use writeAll for partial write safety - Fix workspace move double-free via output_name pointer aliasing - Implement command criteria execution ([class="..."] kill etc.) - Fix JSON escaping in IPC tree/output responses - Replace hardcoded get_config/get_binding_modes with real data - Raise capacity limits (layout 256, render 128/32/128, clients 4096) - Guard executeFloating/executeFullscreen against non-window containers - Fix bar JSON parsing to handle escaped quotes and nested objects - Use XCB accessor for KeyMap.keysymData instead of manual pointer math - Separate OutputData from WorkspaceData to fix type confusion - Add WM_NORMAL_HINTS min size reading and WM_TAKE_FOCUS protocol Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
CodeAnt AI is reviewing your PR. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 11 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 Walkthroughウォークスルー本PRは、ICCCM準拠のフォーカスハンドリング、基準に基づくコマンド実行、ウィンドウ・出力メタデータ構造の拡張、IPC通信の改善、JSONパース機能の強化、およびGitHub Actions CI/CDワークフローの追加を含む包括的なアップデートです。 変更内容
見積もりコード レビュー労力🎯 3 (中程度) | ⏱️ ~25分 関連の可能性があるPR
推奨ラベル
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| Secrets |  |
Apr 6, 2026 8:36p.m. | Review ↗ |
codeant-ai
Bot
added
the
size:L
Up to standards ✅🟢 Issues
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements ICCCM WM_TAKE_FOCUS support, WM_NORMAL_HINTS size constraint handling, and command criteria for targeted execution. It refactors output data structures, expands several internal buffers, and enhances IPC message handling and JSON serialization. Critical feedback identifies a potential use-after-free and logic error when restoring focus after criteria commands, a double-free risk during workspace movement, a busy-loop denial-of-service vector in IPC reading, and a memory leak vulnerability from uninitialized stack data in X11 events.
| if (original_focused) |orig| { | ||
| setFocus(ctx, orig); | ||
| } |
There was a problem hiding this comment.
Restoring the original focus here has two significant issues:
- Logic: It negates the effect of any
focuscommand executed within the criteria block (e.g.,[class="..."] focus). According to i3 behavior, if a focus command is executed via criteria, the focus should remain on the matching window. - Safety: If the command executed in the loop (such as
killor a structural change triggeringcleanupEmptySplitCon) causes the container pointed to byoriginal_focusedto be destroyed, this call results in a Use-After-Free (UAF). The checkif (original_focused)only checks for null, not for pointer validity.
| if (target_out.output_data) |od| { | ||
| // Free old output_name before replacing (skip empty string literals) | ||
| if (wsd.output_name.len > 0) ctx.allocator.free(wsd.output_name); | ||
| wsd.output_name = ctx.allocator.dupe(u8, od.name) catch od.name; |
There was a problem hiding this comment.
The fallback catch od.name creates an alias to the output's name string. Since wsd.output_name is freed in Container.destroy (via freeOwnedString), this will result in a double-free when both the workspace and the output container are destroyed, as both will attempt to free the same memory. If duplication fails, it is safer to set the name to an empty string literal.
wsd.output_name = ctx.allocator.dupe(u8, od.name) catch "";
| if (!supports_take_focus) return; | ||
|
|
||
| // Send WM_TAKE_FOCUS client message | ||
| var event_data: xcb.c.xcb_client_message_event_t = undefined; |
There was a problem hiding this comment.
Initializing event_data with undefined and then only partially filling its fields before sending it to the X server via xcb_send_event can leak sensitive information from the WM's stack memory to the client window's process. It is safer to zero-initialize the structure using std.mem.zeroes.
var event_data: xcb.c.xcb_client_message_event_t = std.mem.zeroes(xcb.c.xcb_client_message_event_t);
| if (err == error.WouldBlock) { | ||
| // Only return WouldBlock if we haven't read anything yet. | ||
| // If partial data was consumed, we must finish reading to stay in sync. | ||
| if (total_read == 0) return error.WouldBlock; | ||
| continue; // Retry — data should arrive shortly on UNIX domain socket | ||
| } |
There was a problem hiding this comment.
Using continue when error.WouldBlock is encountered after partial data has been read creates a busy loop. Since the window manager is single-threaded, this will consume 100% CPU and hang the entire WM if a client sends a partial message (e.g., just the header) and then stalls. This is a potential Denial of Service (DoS) vector.
![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: 5499895f05
ℹ️ 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".
| if (err == error.WouldBlock) { | ||
| // Only return WouldBlock if we haven't read anything yet. | ||
| // If partial data was consumed, we must finish reading to stay in sync. | ||
| if (total_read == 0) return error.WouldBlock; | ||
| continue; // Retry — data should arrive shortly on UNIX domain socket |
There was a problem hiding this comment.
readMessage now loops on error.WouldBlock after reading partial data, but IPC client fds are non-blocking and processed on the main epoll thread. If a client sends part of a header/payload and then pauses, this continue path spins in a tight loop, pegging CPU and blocking all WM event handling until more bytes arrive or the client disconnects.
Useful? React with 👍 / 👎.
| const transient_cookie = xcb.getProperty(conn, 0, window, xcb.ATOM_WM_TRANSIENT_FOR, xcb.ATOM_WINDOW, 0, 1); | ||
| const type_cookie = xcb.getProperty(conn, 0, window, atoms.net_wm_window_type, xcb.ATOM_ATOM, 0, 32); | ||
| const role_cookie = xcb.getProperty(conn, 0, window, atoms.wm_window_role, xcb.ATOM_STRING, 0, 256); | ||
| const hints_cookie = xcb.getProperty(conn, 0, window, xcb.ATOM_WM_NORMAL_HINTS, xcb.ATOM_WM_NORMAL_HINTS, 0, 18); |
There was a problem hiding this comment.
The WM_NORMAL_HINTS property’s data type is WM_SIZE_HINTS, but this call requests type WM_NORMAL_HINTS. On type mismatch, X returns no typed payload, so the later min-size parsing path never sees the hints and min_w/min_h stay unset, breaking the intended constraint handling.
Useful? React with 👍 / 👎.
| if (wsd.output_name.len > 0) ctx.allocator.free(wsd.output_name); | ||
| wsd.output_name = ctx.allocator.dupe(u8, od.name) catch od.name; |
There was a problem hiding this comment.
Falling back to od.name on allocation failure aliases wsd.output_name to memory owned by the output container, but later code still frees wsd.output_name as if it were workspace-owned. A subsequent move or container teardown can then free the same allocation twice (or free output-owned memory early), causing memory corruption in low-memory paths.
Useful? React with 👍 / 👎.
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| const len: u16 = @intCast(@min(end, 511)); | ||
| @memcpy(cfg.status_command[0..len], response[start..][0..len]); | ||
| cfg.status_command_len = len; |
There was a problem hiding this comment.
Suggestion: The parsed status_command is copied directly from JSON without unescaping, but the WM emits this field with JSON escaping (\", \\). This leaves escape backslashes in the command string and can change shell semantics (especially for backslashes/quotes), so unescape common JSON escapes while copying. [logic error]
Severity Level: Major ⚠️
- ❌ Complex `status_command` with quotes misparsed, status bar fails.
- ⚠️ Users can't safely use paths needing shell quoting.| const len: u16 = @intCast(@min(end, 511)); | |
| @memcpy(cfg.status_command[0..len], response[start..][0..len]); | |
| cfg.status_command_len = len; | |
| const raw = response[start .. start + end]; | |
| var in_i: usize = 0; | |
| var out_i: usize = 0; | |
| while (in_i < raw.len and out_i < cfg.status_command.len - 1) : (in_i += 1) { | |
| if (raw[in_i] == '\\' and in_i + 1 < raw.len) { | |
| const esc = raw[in_i + 1]; | |
| switch (esc) { | |
| '\\', '"' => { | |
| cfg.status_command[out_i] = esc; | |
| out_i += 1; | |
| in_i += 1; | |
| continue; | |
| }, | |
| else => {}, | |
| } | |
| } | |
| cfg.status_command[out_i] = raw[in_i]; | |
| out_i += 1; | |
| } | |
| cfg.status_command_len = @intCast(out_i); |
Steps of Reproduction ✅
1. Configure a non-trivial bar status command with quotes in `~/.config/zephwm/config`,
e.g. `status_command i3status --color \"#ffffff\"` (parsed into `cfg.bar.status_command`
in `src/config.zig:181-184`).
2. Start zephwm; the WM builds the bar config IPC response using `buildBarConfigJson` in
`src/main.zig:19-41`, which writes `"status_command"` via `jsonEscapeWrite` (lines 23-27),
emitting JSON-escaped content like `i3status --color \\\"#ffffff\\\"`.
3. The `zephwm-bar` process connects to IPC and calls `queryBarConfig` in
`zephwm-bar/main.zig:82-121`; the status_command parser at lines 111-118 (including the
existing code at 114-118) copies the raw JSON substring between quotes directly into
`cfg.status_command` without unescaping the `\\` and `\"` sequences.
4. Later in `zephwm-bar/main.zig:331-339`, `status_cmd = bar_cfg.getStatusCommand()` is
passed to `spawnStatusCommand` (lines 843-892), which invokes `/bin/sh -c` with this
still-escaped string; the extra backslashes alter the shell's parsing of
quotes/backslashes, so complex `status_command` values receive different arguments than
intended or may fail entirely, causing the external status program (whose output is
rendered in `drawBar` at lines 641-682) to misbehave or not show up.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** zephwm-bar/main.zig
**Line:** 115:117
**Comment:**
*Logic Error: The parsed `status_command` is copied directly from JSON without unescaping, but the WM emits this field with JSON escaping (`\"`, `\\`). This leaves escape backslashes in the command string and can change shell semantics (especially for backslashes/quotes), so unescape common JSON escapes while copying.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
| } | ||
| const len = (std.fmt.bufPrint(&path_buf, "{s}/.config/zephwm/config", .{home}) catch break :blk "").len; | ||
| if (std.fs.cwd().access(path_buf[0..len], .{})) |_| break :blk path_buf[0..len] else |_| {} | ||
| break :blk ""; |
There was a problem hiding this comment.
Suggestion: buildConfigJson only checks XDG_CONFIG_HOME/zephwm/config and ~/.config/zephwm/config, but the actual loader also supports ~/.zephwm/config and /etc/zephwm/config. This makes get_config return an empty config path even when the WM successfully loaded config from those valid locations. [logic error]
Severity Level: Major ⚠️
- ⚠️ zephwm-msg get_config prints empty config despite loaded file.
- ⚠️ Tools relying on config path misidentify active configuration.| break :blk ""; | |
| const legacy_len = (std.fmt.bufPrint(&path_buf, "{s}/.zephwm/config", .{home}) catch break :blk "").len; | |
| if (std.fs.cwd().access(path_buf[0..legacy_len], .{})) |_| break :blk path_buf[0..legacy_len] else |_| {} | |
| if (std.fs.cwd().access("/etc/zephwm/config", .{})) |_| break :blk "/etc/zephwm/config" else |_| {} | |
Steps of Reproduction ✅
1. Create a valid zephwm config only at a legacy/global path: either `~/.zephwm/config` or
`/etc/zephwm/config`, and ensure neither `$XDG_CONFIG_HOME/zephwm/config` nor
`~/.config/zephwm/config` exists (these four search paths are implemented in `fn
loadConfig` at `src/main.zig:508-542`, where it checks XDG, `~/.config/zephwm/config`,
`~/.zephwm/config`, then `/etc/zephwm/config`).
2. Start `zephwm` built from this PR so that `main()` calls `loadConfig(allocator)` at
`src/main.zig:711` (shown by the call `var config = loadConfig(allocator);`), which
successfully loads the config from `~/.zephwm/config` or `/etc/zephwm/config` and
populates `config` / `ctx.config`.
3. From the same session, invoke the IPC client `zephwm-msg` with `-t get_config` (CLI
wiring is defined in `zephwm-msg/main.zig:27-41`, mapping `"get_config"` to
`ipc.MessageType.get_config` and printing the raw response).
4. The WM handles this IPC in `handleIpcMessage` at `src/main.zig:139-163`, which calls
`buildConfigJson(ctx, &dyn_buf)` for `.get_config`; `buildConfigJson` at
`src/main.zig:343-365` only probes `$XDG_CONFIG_HOME/zephwm/config` and
`~/.config/zephwm/config` (lines 353-359) and falls back to `""`, so the IPC response is
`{"config":""}` even though the active config was loaded from `~/.zephwm/config` or
`/etc/zephwm/config`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/main.zig
**Line:** 360:360
**Comment:**
*Logic Error: `buildConfigJson` only checks `XDG_CONFIG_HOME/zephwm/config` and `~/.config/zephwm/config`, but the actual loader also supports `~/.zephwm/config` and `/etc/zephwm/config`. This makes `get_config` return an empty config path even when the WM successfully loaded config from those valid locations.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
| .name = name, | ||
| .output_name = out_name, | ||
| }; | ||
| tree_root.appendChild(output_con); |
There was a problem hiding this comment.
Suggestion: Output containers created from RandR data only populate output_data, but workspace IPC event generation still reads the parent output name from parent.workspace.output_name. Without initializing that compatibility field here, workspace focus/change events report the wrong output ("default"), breaking IPC protocol correctness. [logic error]
Severity Level: Critical 🚨
- ❌ Workspace IPC events always report output as "default".
- ⚠️ Multi-monitor bars mis-associate workspaces with physical outputs.
- ⚠️ External IPC consumers cannot reliably track per-output workspaces.| tree_root.appendChild(output_con); | |
| output_con.workspace = tree.WorkspaceData{ | |
| .name = "", | |
| .output_name = try allocator.dupe(u8, info.nameSlice()), | |
| }; |
Steps of Reproduction ✅
1. Start zephwm so that it detects real RandR outputs via `output.detectOutputs` at
`src/main.zig:174-179`, which calls `detectOutputs(conn, tree_root, allocator)` in
`src/output.zig:23-85`.
2. In `detectOutputs`, for each RandR monitor, an output container is created and only
`output_con.output_data` is populated with the RandR name at `src/output.zig:48-51`;
`output_con.workspace` remains null and its `output_name` field is never set.
3. Trigger a workspace switch through IPC or keybinding (e.g. default `Mod+1`/`Mod+2`),
which is parsed in `src/command.zig:106-112/196-201` and executed via
`event.executeCommand` at `src/event.zig:1753-1803`, dispatching to `executeWorkspace` at
`src/event.zig:47-172`.
4. Inside `executeWorkspace`, when emitting the workspace focus event JSON, it calls
`getWorkspaceOutputName(target_ws)` at `src/event.zig:159-168`, which reads
`target_ws.parent.workspace.output_name` via `getWorkspaceOutputName` at
`src/event.zig:78-88`; because `parent.workspace` was never initialised for output
containers, this function falls back to `"default"`, so all workspace IPC events report
`"output":"default"` even when the real RandR output name stored in
`parent.output_data.name` is something like `"HDMI-1"`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/output.zig
**Line:** 52:52
**Comment:**
*Logic Error: Output containers created from RandR data only populate `output_data`, but workspace IPC event generation still reads the parent output name from `parent.workspace.output_name`. Without initializing that compatibility field here, workspace focus/change events report the wrong output (`"default"`), breaking IPC protocol correctness.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.| .name = owned_name, | ||
| .output_name = owned_out_name, | ||
| }; | ||
| output_con.dirty = false; |
There was a problem hiding this comment.
Suggestion: Dynamically added outputs in updateOutputs also initialize only output_data, but downstream event serialization still reads parent.workspace.output_name. After hotplug, workspace events will carry stale/default output names instead of the actual monitor name. [logic error]
Severity Level: Major ⚠️
- ❌ Workspace events misreport output after dynamic monitor hotplug.
- ⚠️ Multi-monitor bars desynchronise when outputs are added at runtime.
- ⚠️ IPC automations cannot distinguish new output's workspaces.| output_con.dirty = false; | |
| output_con.workspace = tree.WorkspaceData{ | |
| .name = "", | |
| .output_name = try allocator.dupe(u8, name), | |
| }; |
Steps of Reproduction ✅
1. Start zephwm normally so initial outputs are detected and the event loop is running;
RandR screen-change handling is wired via `handleEvent` at `src/event.zig:123-147`, which
dispatches RandR events to `handleRandrScreenChange` at `src/event.zig:150-160`.
2. While zephwm is running, hotplug a new monitor so XRandR emits a screen change event;
`handleRandrScreenChange` calls `output.updateOutputs(ctx.conn, ctx.tree_root,
ctx.allocator)` at `src/event.zig:151-153`.
3. In `updateOutputs`, if the RandR name of the new monitor does not match any existing
output, the "New output" branch at `src/output.zig:230-239` runs: it creates a new
`.output` container, sets `output_con.output_data.name` to an allocator-owned copy of the
RandR name at `src/output.zig:233-237`, but never initialises `output_con.workspace` or
its `output_name` field.
4. After hotplug, when a workspace is focused or created on that new output via commands
executed by `executeWorkspace` at `src/event.zig:47-172` (for example through `workspace`
keybindings parsed in `src/command.zig:106-112/196-201`), the workspace focus event JSON
uses `getWorkspaceOutputName(target_ws)` at `src/event.zig:159-168`; because the new
output's `workspace` is null, `getWorkspaceOutputName` at `src/event.zig:78-88` returns
`"default"`, so workspace events for the hotplugged monitor report `"output":"default"`
instead of the actual RandR name stored in `parent.output_data.name`.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/output.zig
**Line:** 238:238
**Comment:**
*Logic Error: Dynamically added outputs in `updateOutputs` also initialize only `output_data`, but downstream event serialization still reads `parent.workspace.output_name`. After hotplug, workspace events will carry stale/default output names instead of the actual monitor name.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
| fn collectMatchingContainers(root: *tree.Container, crit: *const criteria.Criteria, out: *[64]*tree.Container) usize { | ||
| var count: usize = 0; | ||
| collectMatchingRecursive(root, crit, out, &count); | ||
| return count; | ||
| } | ||
|
|
||
| fn collectMatchingRecursive(con: *tree.Container, crit: *const criteria.Criteria, out: *[64]*tree.Container, count: *usize) void { | ||
| if (count.* >= 64) return; |
There was a problem hiding this comment.
Suggestion: Criteria execution is hard-capped to 64 containers, so commands silently stop applying after the 64th match, which breaks expected behavior on larger trees. [logic error]
Severity Level: Major ⚠️
- ⚠️ Criteria commands skip containers after first 64 matches.
- ⚠️ i3-style [criteria] kill/move can leave windows untouched.| fn collectMatchingContainers(root: *tree.Container, crit: *const criteria.Criteria, out: *[64]*tree.Container) usize { | |
| var count: usize = 0; | |
| collectMatchingRecursive(root, crit, out, &count); | |
| return count; | |
| } | |
| fn collectMatchingRecursive(con: *tree.Container, crit: *const criteria.Criteria, out: *[64]*tree.Container, count: *usize) void { | |
| if (count.* >= 64) return; | |
| const MAX_CRITERIA_MATCHES: usize = 4096; | |
| fn collectMatchingContainers(root: *tree.Container, crit: *const criteria.Criteria, out: *[MAX_CRITERIA_MATCHES]*tree.Container) usize { | |
| var count: usize = 0; | |
| collectMatchingRecursive(root, crit, out, &count); | |
| return count; | |
| } | |
| fn collectMatchingRecursive(con: *tree.Container, crit: *const criteria.Criteria, out: *[MAX_CRITERIA_MATCHES]*tree.Container, count: *usize) void { | |
| if (count.* >= MAX_CRITERIA_MATCHES) return; |
Steps of Reproduction ✅
1. User issues an i3-style criteria command like `[class="Xterm"] kill`;
`parseCriteriaPrefix()` in `src/command.zig:43-57` parses the `[class=...]` block into a
`criteria_mod.Criteria`, and `command.parse()` (e.g. branches at `src/command.zig:189-194`
or `203-211`) returns a `Command` with `.criteria` set.
2. The command is executed either from a keybinding via `handleKeyPress()` in
`src/event.zig:1154-1218` or via other callers of `executeCommand()`, which invokes
`executeCommand(ctx, cmd)` at `src/event.zig:1753-1803`.
3. At the start of `executeCommand()` (lines `1755-1758`), when `cmd.criteria` is
non-null, it declares `var matches_buf: [64]*tree.Container = undefined;` and calls
`collectMatchingContainers(ctx.tree_root, &crit, &matches_buf);` to gather all matching
containers.
4. `collectMatchingContainers()` and `collectMatchingRecursive()` in
`src/event.zig:1735-1742` fill `out`, but `collectMatchingRecursive()` returns early once
`count.* >= 64`, so if more than 64 containers match the criteria (for example, 100 Xterms
on one workspace), only the first 64 are placed into `matches_buf[0..match_count]` and
processed; the remaining matching windows are silently skipped and never see the
kill/move/etc command.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** src/event.zig
**Line:** 1735:1742
**Comment:**
*Logic Error: Criteria execution is hard-capped to 64 containers, so commands silently stop applying after the 64th match, which breaks expected behavior on larger trees.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
|
CodeAnt AI finished reviewing your PR. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 29-31: The "Format check" step currently runs zig fmt --check src/
which misses Zig files outside src (e.g., zephwm-bar/main.zig); update the
Format check step (the job named "Format check") to run zig fmt --check across
the whole repository instead of only src/ (for example by using a repo-wide
target or glob such as **/*.zig or running against the repo root) so all .zig
files are validated in CI.
In `@src/event.zig`:
- Around line 1754-1773: You are holding raw pointers to tree nodes (matches_buf
elements and original_focused from getFocusedContainer) while reentering
executeCommand which may mutate or free nodes; this causes dangling pointers for
commands like kill or scratchpad move. Fix by collecting stable identifiers
(e.g., container IDs returned by collectMatchingContainers or compute a
path/index relative to ctx.tree_root) instead of raw pointers, then for each id
re-resolve the current container from ctx.tree_root before calling
setFocus/executeCommand, and likewise re-resolve/check that the original_focused
id still belongs to the tree before calling setFocus to restore focus; update
collectMatchingContainers/getFocusedContainer usage and the loop over
matches_buf to store and re-resolve ids rather than reuse raw pointers.
- Around line 2651-2656: 現在のコード uses ctx.allocator.dupe(u8, od.name) catch
od.name which on OOM makes wsd.output_name alias od.name and causes
double-free/UAF when Container.destroy frees both; change the logic in the block
handling current_ws.workspace/target_out.output_data to call ctx.allocator.dupe
into a temporary result (e.g., var new_name = ctx.allocator.dupe(u8, od.name)
catch |err| ...), only if dupe succeeds then free the old wsd.output_name (if
len>0) and assign wsd.output_name = new_name; on dupe failure do not assign or
free the old value and propagate or handle the allocation error appropriately so
no aliasing to od.name occurs (refer to wsd.output_name, ctx.allocator.dupe,
ctx.allocator.free, target_out.output_data, Container.destroy).
- Around line 725-726: The xcb.getProperty call that reads WM_NORMAL_HINTS uses
the wrong type atom: currently both property and type are
xcb.ATOM_WM_NORMAL_HINTS which causes empty replies and leaves min_w/min_h zero;
update the call that creates hints_cookie to pass xcb.ATOM_WM_SIZE_HINTS as the
type atom (i.e., change the second atom argument from ATOM_WM_NORMAL_HINTS to
ATOM_WM_SIZE_HINTS) so the WM_SIZE_HINTS-typed data is returned and min_w/min_h
are populated.
In `@src/ipc.zig`:
- Around line 145-151: The read loop currently spins on non-blocking sockets
because when std.posix.read(fd, buf[total_read..]) returns error.WouldBlock
after a partial read the code does `continue` instead of returning control;
change the behavior so that any WouldBlock from std.posix.read returns
error.WouldBlock to the caller when total_read > 0 (same as when total_read ==
0) rather than continuing to retry, and make the identical change in the other
symmetric block (the one around lines 165-169) so partial reads propagate
WouldBlock back to the caller (referencing fd, buf, total_read, and
error.WouldBlock).
In `@src/main.zig`:
- Around line 343-365: buildConfigJson currently writes the config file path
into the "config" field instead of the actual loaded configuration text and its
search paths differ from loadConfig; update buildConfigJson to perform the same
search order as loadConfig (XDG_CONFIG_HOME/zephwm/config,
~/.config/zephwm/config, ~/.zephwm/config, /etc/zephwm/config), open the first
existing file, read its contents into the provided buffer (or a temporary
slice), and then call jsonEscapeWrite to emit the file contents into the
"config" JSON field (preserve existing error handling style and buffer usage);
keep references to buildConfigJson, loadConfig, and jsonEscapeWrite to locate
the code to change.
In `@src/output.zig`:
- Around line 49-50: 出力名の移行が不完全で、出力作成時に output_con.output_data =
tree.OutputData{ .name = name } に移した一方で getWorkspaceOutputName() はまだ
workspace.output_name を参照しているため起動直後や hotplug 後に名前が "default" のまま残ります；修正は二点で、まず
getWorkspaceOutputName()（src/event.zig 内）を新しい構造に合わせて output コンテキストの
output_data.name を参照するように更新するか、出力作成箇所（output_con.output_data / tree.OutputData
の設定がある箇所：49行付近、および指摘の 75-76, 235-236, 274-289 相当の箇所）で workspace.output_name
を必ず同期して設定すること、そして removed-output の退避処理でも各 workspace の workspace.output_name
を新しい出力名に更新するロジックを追加して古い出力名が残らないようにすること（対象シンボル: output_con.output_data,
tree.OutputData, getWorkspaceOutputName, workspace.output_name, removed-output
ハンドリングロジック）。
In `@src/xcb.zig`:
- Around line 374-379: The returned keysym slice (keysymData) can be shorter
than callers expect, so modify getKeycode to guard against out-of-bounds access:
inside getKeycode (the function that currently indexes data[base + col]) check
that base + col < data.len() (and/or that col < data.len() - base) before
accessing the slice, and handle the short-reply case safely (e.g., return an
Option/Result or a sentinel value) instead of unconditionally indexing;
reference the keysymData slice, the getKeycode function, and the base/col index
expressions when making the change.
In `@zephwm-bar/main.zig`:
- Around line 91-97: The extracted substrings from response (found via
findUnescapedQuote in the font parsing block and similar code paths used by
queryBarConfig() and parseStatusUpdate()) are still JSON-escaped; you must
unescape them before memcpy/assign so values like \" \\ \n and unicode escapes
become their real characters. Add a utility (e.g. unescapeJsonString) that
accepts the raw slice from response[start..end], decodes JSON escape sequences
(\", \\, /, b, f, n, r, t, and \uXXXX) into a temporary buffer, enforces the
existing 255-byte truncation, and returns the unescaped length; then call this
function in the font block (cfg.font / cfg.font_len) and in the places indicated
(the blocks at ~111-117, 978-995, 1066-1080) before memcpy/assign so stored
strings are unescaped and length-correct. Ensure the helper handles invalid
escapes robustly and returns an error or truncates safely rather than copying
raw escape sequences.
🪄 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
Run ID: 55acf0bc-3062-4eab-9dce-f3b4e5b36bd3
📒 Files selected for processing (10)
.github/workflows/ci.ymlsrc/event.zigsrc/ipc.zigsrc/layout.zigsrc/main.zigsrc/output.zigsrc/render.zigsrc/tree.zigsrc/xcb.zigzephwm-bar/main.zig
| - name: Format check | ||
| continue-on-error: true | ||
| run: zig fmt --check src/ |
There was a problem hiding this comment.
fmt チェックが src/ 外の Zig ファイルを見ていません。
この PR でも zephwm-bar/main.zig を変更していますが、zig fmt --check src/ だと検査対象から漏れます。整形を CI で担保するなら、repo 全体の *.zig を対象にした方が安全です。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 29 - 31, The "Format check" step
currently runs zig fmt --check src/ which misses Zig files outside src (e.g.,
zephwm-bar/main.zig); update the Format check step (the job named "Format
check") to run zig fmt --check across the whole repository instead of only src/
(for example by using a repo-wide target or glob such as **/*.zig or running
against the repo root) so all .zig files are validated in CI.
| fn buildConfigJson(ctx: *event.EventContext, buf: *[8192]u8) []const u8 { | ||
| _ = ctx; | ||
| var fbs = std.io.fixedBufferStream(buf); | ||
| const w = fbs.writer(); | ||
| // Return config file path if known, empty config otherwise (matches i3 behavior) | ||
| w.writeAll("{\"config\":\"") catch return "{}"; | ||
| // Try to find the config file path | ||
| const home = std.posix.getenv("HOME") orelse ""; | ||
| const xdg_config = std.posix.getenv("XDG_CONFIG_HOME"); | ||
| var path_buf: [512]u8 = undefined; | ||
| const config_path: []const u8 = blk: { | ||
| if (xdg_config) |xdg| { | ||
| const len = (std.fmt.bufPrint(&path_buf, "{s}/zephwm/config", .{xdg}) catch break :blk "").len; | ||
| if (std.fs.cwd().access(path_buf[0..len], .{})) |_| break :blk path_buf[0..len] else |_| {} | ||
| } | ||
| const len = (std.fmt.bufPrint(&path_buf, "{s}/.config/zephwm/config", .{home}) catch break :blk "").len; | ||
| if (std.fs.cwd().access(path_buf[0..len], .{})) |_| break :blk path_buf[0..len] else |_| {} | ||
| break :blk ""; | ||
| }; | ||
| jsonEscapeWrite(w, config_path) catch return "{}"; | ||
| w.writeAll("\"}") catch return "{}"; | ||
| return fbs.getWritten(); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
i3 IPC GET_CONFIG CONFIG reply format: does the "config" field contain loaded file contents or the config file path?
💡 Result:
In the GET_CONFIG → CONFIG reply, the "config" field contains the top-level config file contents (text) that i3 most recently loaded — not the file path. (i3wm.org)
If you need paths, use "included_configs": each entry includes "path" (absolute filename) plus "raw_contents" (file text). The first entry’s "raw_contents" is identical to "config". (i3wm.org)
Citations:
🏁 Script executed:
# Find the loadConfig function
rg -n "fn loadConfig" src/main.zigRepository: midasdf/zephwm
Length of output: 126
🏁 Script executed:
# Also search for any config path definitions or constants
rg -n "zephwm/config|\.config/zephwm|etc/zephwm" src/main.zigRepository: midasdf/zephwm
Length of output: 1127
🏁 Script executed:
# Get more context around buildConfigJson to see the full function
sed -n '343,365p' src/main.zigRepository: midasdf/zephwm
Length of output: 1214
🏁 Script executed:
# Get the loadConfig function implementation
sed -n '508,560p' src/main.zigRepository: midasdf/zephwm
Length of output: 2238
🏁 Script executed:
# Also check what readConfigFile does - is it reading file contents?
rg -n "fn readConfigFile" src/main.zig -A 20Repository: midasdf/zephwm
Length of output: 956
GET_CONFIG 応答が i3 IPC プロトコル期待と一致していません。
Lines 343-365 の buildConfigJson は "config" フィールドに設定ファイルのパスを返していますが、i3 IPC の GET_CONFIG は「最後にロードされた設定内容（テキスト）」を返すべきです（i3 IPC 仕様より）。
加えて、Lines 355-358 の探索パスが loadConfig（Lines 515-540）と一致していません。loadConfig は 4 つのパスを検索します：
XDG_CONFIG_HOME/zephwm/config~/.config/zephwm/config~/.zephwm/config← buildConfigJson では検索されていない/etc/zephwm/config← buildConfigJson では検索されていない
結果として、実際に読み込まれている設定ファイルが ~/.zephwm/config や /etc/zephwm/config にある場合、空文字が返される可能性があります。
修正には、buildConfigJson で loadConfig と同じ探索順序とパスを使用し、ファイルの内容を読み込んで返すようにしてください。
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main.zig` around lines 343 - 365, buildConfigJson currently writes the
config file path into the "config" field instead of the actual loaded
configuration text and its search paths differ from loadConfig; update
buildConfigJson to perform the same search order as loadConfig
(XDG_CONFIG_HOME/zephwm/config, ~/.config/zephwm/config, ~/.zephwm/config,
/etc/zephwm/config), open the first existing file, read its contents into the
provided buffer (or a temporary slice), and then call jsonEscapeWrite to emit
the file contents into the "config" JSON field (preserve existing error handling
style and buffer usage); keep references to buildConfigJson, loadConfig, and
jsonEscapeWrite to locate the code to change.
- Fix criteria execution use-after-free: collect window IDs instead of raw pointers, re-resolve before each command; skip focus restore for focus cmds - Fix OOM double-free in workspace output_name: only free+assign on dupe success - Fix IPC busy-loop: return null on WouldBlock after partial read - Fix WM_NORMAL_HINTS type atom: use WM_SIZE_HINTS per ICCCM spec - Fix getWorkspaceOutputName: read output_data.name instead of stale workspace field - Fix buildConfigJson: search all 4 config paths, return file contents not path - Fix getKeycode: add bounds check for short keysymData replies - Fix xcb_client_message_event_t: zero-init instead of undefined - Add JSON unescape utility to bar, apply to font/status_command/blocks parsing - Extend CI fmt check to cover zephwm-bar/ Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Resolve ci.yml conflict: keep expanded fmt check (src/ + zephwm-bar/) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b704e57b6f
ℹ️ 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".
| while (total_read < total_needed) { | ||
| const n = std.posix.read(fd, buf[total_read..total_needed]) catch |err| { | ||
| if (err == error.WouldBlock) return error.WouldBlock; | ||
| if (err == error.WouldBlock) return null; // Partial payload: drop client to avoid busy-loop |
There was a problem hiding this comment.
This branch treats error.WouldBlock after a partial payload read as a disconnect, but IPC sockets are stream-based and non-blocking reads can legitimately return partial frames. If a client’s header/payload arrives across multiple writes (or a short read happens), the server will close a healthy connection mid-message and drop the request, causing intermittent IPC failures for valid clients.
Useful? React with 👍 / 👎.
| @memcpy(cfg.font[0..len], response[start..][0..len]); | ||
| if (findUnescapedQuote(response[start..])) |end| { | ||
| const raw = response[start..][0..end]; | ||
| const len: u8 = @intCast(unescapeJsonString(&cfg.font, raw)); |
There was a problem hiding this comment.
unescapeJsonString can return 256 when the font field fills the destination buffer, but this value is cast directly to u8. That overflows the u8 range and can trap in safe builds (or wrap in unsafe builds), so a long font value in bar config can crash or corrupt parsing at startup.
Useful? React with 👍 / 👎.
User description
Summary
Changes (9 files, +278 -51)
src/ipc.zig— readMessage preserves partial state, sendRequest uses writeAllsrc/event.zig— criteria execution, container type guards, OutputData migration, WM_TAKE_FOCUS, WM_NORMAL_HINTS, client list cap raisedsrc/main.zig— JSON escaping via jsonEscapeWrite, real get_config/get_binding_modessrc/tree.zig— OutputData type, min_w/min_h in WindowDatasrc/output.zig— OutputData instead of WorkspaceData for output containerssrc/render.zig— Capacity limits raised to 128/32/128src/layout.zig— MAX_TILING_CHILDREN 64→256src/xcb.zig— XCB accessor for keysym data with length validationzephwm-bar/main.zig— findUnescapedQuote, findMatchingBrace for safe JSON parsingTest plan
zig build— compiles cleanlyzig build test— all tests pass[class="..."] kill)🤖 Generated with Claude Code
CodeAnt-AI Description
Fix output handling, IPC safety, and window focus behavior
What Changed
WM_TAKE_FOCUSto supported apps, and windows now respect minimum size hints fromWM_NORMAL_HINTS.Impact
✅ Fewer workspace move crashes✅ Clearer JSON responses for panels and IPC clients✅ Better focus handling for apps that need WM_TAKE_FOCUS💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
リリースノート
新機能
改善
チョア