85df18a0

Add corrections section to waystty implementation plan

a73x 2026-04-08 05:40

Commit message

Add corrections section to waystty implementation plan

Addresses review feedback without rewriting the plan:

Blockers fixed (C1-C2, C7-C10, C12-C13):
- Zig 0.15 stdlib API sweep (GPA, ArrayList, io.getStdOut, fcntl flags)
- Task 0.2/0.3 ordering (build.zig must exist before zig fetch --save)
- @cImport moved out of function body
- mmap defer-before-null-check fixed
- String literal casts for fontconfig/xkbcommon C APIs
- FT_Bitmap null check + negative pitch handling
- execve plumbing via std.posix.execveZ
- Task 2.1 path discovery via ls instead of find

Missing coverage (C3-C6, C11):
- New Task 2.8: wire effect callbacks (WRITE_PTY, TITLE_CHANGED, etc.)
- New Task 3.2: SIGCHLD / child-exit detection (terminal now exits
  when shell exits)
- New Task 7.6: cursor rendering (previously no visible cursor)
- sRGB color math documented
- Key repeat carries current modifier state

Structure (C14-C15):
- Guidance to split 6.7, 6.8, 7.2, 7.3 into sub-tasks ad-hoc
- Drop unused protocol bindings from Task 5.1

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

docs/superpowers/plans/2026-04-07-waystty-implementation.md

@@ @@ -26,6 +26,232 @@ When in doubt, consult these external references: @@
+---
+## Corrections (apply while executing)
+This plan was reviewed after writing. The following corrections apply throughout — read them before starting, and apply them when you reach the relevant task.
+### C1. Zig 0.15 stdlib API sweep
+Multiple tasks use pre-0.15 Zig idioms. Apply these substitutions **everywhere**:
+- **`std.heap.GeneralPurposeAllocator(.{}){}`** → use `std.heap.DebugAllocator(.{}){}` for debug builds, or `std.heap.smp_allocator` for release. The GPA was renamed to `DebugAllocator` in 0.15.
+- **`std.io.getStdOut().writer()`** → use `std.fs.File.stdout().writer(&buf)` with an explicit `[4096]u8` buffer. Writergate changed the writer API in 0.15. (Affects Task 3.1.)
+- **`std.ArrayList(T).init(alloc)`** → ArrayList is unmanaged by default in 0.15. Use `std.ArrayList(T){}` or `.empty` as the initial value. Methods take an allocator: `list.append(alloc, item)`, `list.deinit(alloc)`. (Affects Task 5.4, 5.5, 7.3.)
+- **`std.posix.fcntl(..., flags | @as(u32, @bitCast(std.posix.O{...})))`** → the `flags` returned by fcntl is wider than `u32` on 64-bit. Use `std.posix.O.NONBLOCK` via the typed wrapper, or bitcast through `@as(@TypeOf(flags), ...)`. (Affects Task 1.1.)
+### C2. Task 0.2 / 0.3 ordering is wrong
+`zig fetch --save` requires a valid `build.zig` to exist. The order must be:
+. **Task 0.2 (revised)**: Write `build.zig.zon` with an empty `.dependencies = .{}` block and placeholder fingerprint `0x0`. Do NOT run `zig fetch` yet.
+. **Task 0.3**: Write `build.zig` and `src/main.zig`.
+. **Task 0.3 extra step**: Run `zig build` — it will fail with a fingerprint error. Paste the printed fingerprint value back into `build.zig.zon`. Run `zig build` again; it should succeed and print `waystty`.
+. **Task 0.2b (new step)**: Run the `zig fetch --save` commands to populate dependencies. Each will update `build.zig.zon` with real hashes.
+. **Task 0.2c**: Pin ghostty to a specific commit hash by editing the URL to `git+https://github.com/ghostty-org/ghostty#<commit-sha>`. The spec requires pinning.
+### C3. Add Task 2.8 — wire effect callbacks
+The spec lists six required effect callbacks. The plan omits this step, which means `vim` cursor positioning will break (needs `WRITE_PTY`), titles won't update, and DA queries won't respond. Add this task after Task 2.7:
+**Task 2.8: Wire effect callbacks**
+- [ ] Register `WRITE_PTY` callback that writes bytes to a provided `*Pty` via a context pointer. **Mandatory** — without this, the terminal cannot respond to DSR or cursor-position queries.
+- [ ] Register `DEVICE_ATTRIBUTES` callback returning VT220 conformance.
+- [ ] Register `SIZE` callback that returns current cols/rows.
+- [ ] Register `XTVERSION` callback returning `"waystty"`.
+- [ ] Register `TITLE_CHANGED` callback that stores the new title in a shared string. `main.zig` reads this after each frame and calls `xdg_toplevel.setTitle` when it changes.
+- [ ] Register `COLOR_SCHEME` callback as a stub returning `false`.
+The exact Zig API for registering these callbacks must be determined from `example/zig-vt/src/main.zig`. The C API uses `ghostty_terminal_set(term, OPT, callback)`; the Zig module likely exposes these as fields on an options struct passed to `Terminal.init`, or as individual setter methods.
+Add a test that feeds a DSR query (`\x1b[6n`) and verifies the write-pty callback fires with a cursor-position response.
+### C4. Add Task 3.2 — SIGCHLD / child-exit detection
+The plan's main loop checks `p.child_pid > 0`, but `child_pid` never changes after spawn — the loop runs forever after the shell exits. Fix:
+**Task 3.2: Detect child exit**
+- [ ] Add a `waitpidNonblocking` helper to `pty.zig`:
+```zig
+pub fn isChildAlive(self: *Pty) bool {
+    var status: c_int = 0;
+    const rc = c.waitpid(self.child_pid, &status, c.WNOHANG);
+    if (rc == 0) return true;  // still running
+    if (rc == self.child_pid) return false;  // exited
+    return true;  // error or no state change
+}
+```
+- [ ] In the main loop (Task 7.3), replace `while (!window.should_close and p.child_pid > 0)` with `while (!window.should_close and p.isChildAlive())`.
+- [ ] When `Pty.read` returns `0` (not `error.WouldBlock` — a clean `0` means EOF), break out of the loop. Add this check in both Task 3.1's `runHeadless` (already handles it) and Task 7.3's main loop (currently doesn't).
+### C5. Add Task 7.6 — cursor rendering
+The plan never draws a cursor. libghostty's render state exposes cursor position and visibility. Add after Task 7.5:
+**Task 7.6: Render cursor**
+- [ ] Query cursor x/y/visible from `render_state` after each `update`.
+- [ ] If visible, append one extra `Instance` to the instance buffer at the cursor cell with inverted fg/bg colors (or a hardcoded bright color). Use a special UV rect pointing to an all-1.0 region of the atlas (reserve a 1x1 white pixel at atlas `(0,0)` during `Atlas.init`).
+- [ ] Draw the cursor instance as part of the same draw call.
+Add the white-pixel reservation to Task 4.3: after `atlas.pixels` is zeroed, set `atlas.pixels[0] = 255` and reserve cursor UV coordinates at `(0, 0)` to `(1/width, 1/height)`.
+### C6. sRGB color correctness (Task 6.5 + 6.7)
+Mixing sRGB-encoded colors with linear glyph alpha produces visually wrong text. Pick one:
+- **Option A (recommended)**: Change swapchain format from `.b8g8r8a8_srgb` to `.b8g8r8a8_unorm`, and manually linearize cell colors before passing to the shader (`pow(x, 2.2)` approximation, or a proper sRGB→linear function), then the shader's `mix` works in linear space. Output also needs to be gamma-corrected before write.
+- **Option B (simpler, slightly wrong)**: Keep sRGB swapchain. Do `mix` in sRGB space (visually acceptable for terminal text, even if mathematically wrong). Document the choice.
+The plan originally assumed B implicitly; the review correctly flagged it. Choose B for v1 simplicity and document the limitation.
+### C7. Task 7.3 — fix `@cImport` in function body
+`@cImport` inside a function body is a compile error. Move the dlopen wrapper to module scope:
+```zig
+// At top of main.zig (not inside main)
+const dl = @cImport({
+    @cInclude("dlfcn.h");
+});
+var vk_lib_handle: ?*anyopaque = null;
+fn vkGetInstanceProcAddr(instance: anytype, name: [*:0]const u8) ?*const fn () callconv(.C) void {
+    if (vk_lib_handle == null) {
+        vk_lib_handle = dl.dlopen("libvulkan.so.1", dl.RTLD_NOW);
+    }
+    const handle = vk_lib_handle orelse return null;
+    const sym = dl.dlsym(handle, "vkGetInstanceProcAddr") orelse return null;
+    const get_proc: *const fn (@TypeOf(instance), [*:0]const u8) callconv(.C) ?*const fn () callconv(.C) void =
+        @ptrCast(@alignCast(sym));
+    return get_proc(instance, name);
+}
+```
+The exact signature vulkan-zig expects must still be matched; consult its examples. Key fix: cImport and `vk_lib_handle` live at module scope.
+### C8. Keyboard event queue
+Task 5.4 uses `ArrayList(KeyboardEvent).init(alloc)`. Apply C1 (unmanaged ArrayList), plus:
+- Pre-reserve capacity of 64 at init so `append` rarely allocates in the event callback.
+- If `append` fails (OOM or otherwise), log via `std.log.err` rather than silently dropping — this is the only path for user input.
+### C9. Task 5.4 mmap defer ordering + xkbcommon casts
+```zig
+.keymap => |k| {
+    if (k.format != .xkb_v1) return;
+    const map_mem = c.mmap(
+        null,
+        k.size,
+        c.PROT_READ,
+        c.MAP_PRIVATE,
+        k.fd,
+,
+    );
+    if (map_mem == c.MAP_FAILED) {
+        _ = c.close(k.fd);
+        return;
+    }
+    defer _ = c.munmap(map_mem, k.size);
+    defer _ = c.close(k.fd);
+    const new_keymap = c.xkb_keymap_new_from_string(
+        kb.xkb_ctx,
+        @ptrCast(@alignCast(map_mem.?)),
+        c.XKB_KEYMAP_FORMAT_TEXT_V1,
+        c.XKB_KEYMAP_COMPILE_NO_FLAGS,
+    ) orelse return;
+    // ...
+},
+```
+Also cast modifier name strings explicitly:
+```zig
+.ctrl = c.xkb_state_mod_name_is_active(state, @ptrCast("Control"), c.XKB_STATE_MODS_EFFECTIVE) > 0,
+.shift = c.xkb_state_mod_name_is_active(state, @ptrCast("Shift"), c.XKB_STATE_MODS_EFFECTIVE) > 0,
+```
+Same for all `FcPatternAddString` string literals in Task 4.1.
+### C10. Task 4.2 — FT_Bitmap nullability and negative pitch
+```zig
+if (w > 0 and h > 0) {
+    const buffer = bitmap.buffer orelse return error.FtBitmapNull;
+    const pitch: i32 = bitmap.pitch;
+    const abs_pitch: u32 = @intCast(@abs(pitch));
+    const top_down = pitch > 0;
+    var y: u32 = 0;
+    while (y < h) : (y += 1) {
+        const src_y = if (top_down) y else (h - 1 - y);
+        const src_row = buffer + @as(usize, src_y) * abs_pitch;
+        const dst_row = pixels.ptr + @as(usize, y) * w;
+        @memcpy(dst_row[0..w], src_row[0..w]);
+    }
+}
+```
+### C11. Task 5.5 — repeat events must carry modifiers
+`tickRepeat` creates synthetic events with `modifiers = .{}`. This breaks `Ctrl-c` repeat. Change `Keyboard` to track `current_mods: Modifiers` (updated in the `.modifiers` handler) and use it when constructing the synthetic repeat event.
+### C12. Task 7.3 — child process `execve` plumbing
+Use `std.posix.execveZ` instead of `std.c.execve` directly — it handles the type plumbing correctly:
+```zig
+if (pid == 0) {
+    _ = c.setenv("TERM", "xterm-256color", 1);
+    var argv = [_:null]?[*:0]const u8{ @ptrCast(opts.shell.ptr), null };
+    std.posix.execveZ(@ptrCast(opts.shell.ptr), &argv, std.c.environ) catch {};
+    std.process.exit(1);
+}
+```
+Assume the caller passes a `[:0]const u8` shell path (tighten `SpawnOptions.shell` to `[:0]const u8`).
+### C13. Task 2.1 — path discovery
+Instead of `find`, use the deterministic path:
+```bash
+ls .zig-cache/p/ | grep ghostty
+```
+If the ghostty dep is lazy and hasn't been fetched yet, run `zig build` first (Task 0.4 triggers it). Then:
+```bash
+ls .zig-cache/p/<ghostty-hash>/example/zig-vt/src/
+```
+### C14. Split the huge tasks (6.7, 6.8, 7.2, 7.3)
+Each of these should be split into 4-6 sub-tasks with a build check after each sub-task. For example, Task 6.7 splits into:
+- 6.7a: Shader module creation
+- 6.7b: Vertex input description + binding
+- 6.7c: Rasterizer + multisampling + color blend
+- 6.7d: Pipeline layout + push constants
+- 6.7e: Final `createGraphicsPipelines` call and smoke-compile
+Task 7.2's Context is even larger — split into init (with one subsystem per sub-task: instance, device, swapchain, render pass, pipeline, descriptors, commands, sync, buffers, atlas) and deinit.
+When executing, the implementer should perform this split ad-hoc rather than following the monolithic steps as-written.
+### C15. Drop unused protocol bindings (nit)
+Task 5.1 binds `wp_cursor_shape_manager_v1`, `wp_fractional_scale_manager_v1`, `wp_viewporter`, `wl_output`. The plan never uses them. For a truly minimal v1, drop them from the scanner block and re-add when actually implementing cursor shape / DPI scaling.
+---
+## Phase 0: Project Scaffolding
+### Task 0.1: Initialize project and git