Skip to content

config: c_get returns window-padding-x/-y (on 9d8d40319) - #251

Merged
lawrencecchen merged 2 commits into
mainfrom
cmux-config-get-window-padding
Sep 30, 2026
Merged

lawrencecchen merged 2 commits into
mainfrom
cmux-config-get-window-padding

Conversation

@lawrencecchen

@lawrencecchen lawrencecchen commented Sep 30, 2026 •

Copy link
Copy Markdown

ghostty_config_get returned false for window-padding-x and window-padding-y: WindowPadding is a non-packed struct without cval. cmux-next needs the user's value to put its tab icons on the terminal's first text column.

  • WindowPadding.C (extern {top_left, bottom_right} u32 points) and cval; ghostty.h declares ghostty_config_window_padding_s.
  • window-padding-balance already comes back as its tag name; the test pins it.
  • Based on 9d8d40319 (the 9961d09be pin of cmux feat-cmux-next plus the styled-blank-row test fix, without which build-ghosttykit.yml stops before building). Merge with a merge commit so the branch head stays an ancestor of main for the submodule pointer.

Two commits: the failing test c_get: window padding, then the fix. Red/green in cmux build-ghosttykit.yml (new test-filter step): red https://github.com/manaflow-ai/cmux/actions/runs/36783657036, green (also publishes the xcframework) https://github.com/manaflow-ai/cmux/actions/runs/36783660554.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented Sep 30, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: b06d0fae-9fda-443d-aa78-ad3c60e1949c

📥 Commits

Reviewing files that changed from the base of the PR and between 4330ffa and a1026ef.

📒 Files selected for processing (3)
  • include/ghostty.h
  • src/config/Config.zig
  • src/config/c_get.zig

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The public interface now defines a C-compatible type for window padding values. The Zig configuration type converts to this representation, and a test checks padding and balance retrieval.

Changes

Window padding retrieval

Layer / File(s) Summary
Expose and verify window padding values
include/ghostty.h, src/config/Config.zig, src/config/c_get.zig
The public header and Zig configuration type represent padding as top_left and bottom_right unsigned integers. A test checks retrieval of horizontal and vertical padding pairs and the "true" balance value.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Suggested reviewers: mitchellh

Merge Risk: ⚪ Minimal · up to a1026

The new padding representation and conversion align. The vertical test could be more discriminating, but no current failure is established, so the change appears mergeable.

Security Architecture Review

Security architecture risk: ⚪ Minimal · up to a1026

The change exposes two scalar configuration values through an existing read-only API. The C and Zig representations match, and the reviewed path introduces no new privileges, sensitive-data access, or trust-boundary bypass.

Retained concerns
No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The reviewed exposure is limited to native callers retrieving padding from a configuration object they supply. This path does not introduce cross-tenant access, credential authority, or a new remote entrypoint.

Trust Boundaries and Controls

  • observed — The getter remains a native C caller boundary: callers supply the configuration, key bytes, and output memory. Unrecognized keys are rejected, but output capacity is not checked; callers must use the documented result type and valid memory. The new padding conversion uses this pre-existing pointer contract rather than adding a separate write mechanism.

Resilience and Maintainability Implications

  • inferred — The new conversion is a synchronous scalar copy with no allocation, reservation, persistent mutation, or ownership transfer. It introduces no multi-step transition requiring cleanup or recovery; repeated retrieval replaces the caller's result without changing configuration state.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly identifies the main change: c_get now returns window-padding-x and window-padding-y. The commit reference is extra but does not make the title misleading.
✨ Finishing Touches
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR
  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Autopilot is currently an internal CodeRabbit preview.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

lawrencecchen and others added 2 commits September 30, 2026 15:06
cmux aligns its pane chrome to the terminal's first column and needs the
user's window-padding-x. ghostty_config_get returns false for it:
WindowPadding is a non-packed struct without cval. The test also pins
window-padding-balance, which c_get already returns as its tag name.

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

WindowPadding gets a C mirror and cval, so ghostty_config_get fills a
{top_left, bottom_right} pair of u32 points instead of returning false.
cmux reads it to put its tab icons on the terminal's first text column.
window-padding-balance already returns its tag name. ghostty.h declares
ghostty_config_window_padding_s.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@lawrencecchen
lawrencecchen force-pushed the cmux-config-get-window-padding branch from a1026ef to b1a49b6 Compare September 30, 2026 22:06
lawrencecchen added a commit to manaflow-ai/cmux that referenced this pull request Sep 30, 2026
…dow-padding-x

Follow-up to 541b77d (coordinator decision): the terminal host must put
the first text column on Metrics.paneContentInset with the user's real
window-padding-x, not Ghostty's default. GhosttyRuntime.terminalPadding
reads window-padding-x, window-padding-y and window-padding-balance through
ghostty_config_get; TerminalHostView subtracts each side's padding and
re-lays out on GhosttyRuntime.configDidChange (a config reload).

Red: the pinned libghostty (9961d09be) returns false for window-padding-x
and -y (WindowPadding has no C value), so the read falls back to 2 pt.
PaneAlignmentTests.theTerminalPaddingIsReadFromTheGhosttyConfig fails;
the GhosttyKit bump with manaflow-ai/ghostty#251 makes it pass.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@lawrencecchen lawrencecchen changed the title config: c_get returns window-padding-x/-y (on 9961d09be) config: c_get returns window-padding-x/-y (on 9d8d40319) Sep 30, 2026
@lawrencecchen
lawrencecchen merged commit dcb1d69 into main Sep 30, 2026
154 checks passed
lawrencecchen added a commit to manaflow-ai/cmux that referenced this pull request Sep 30, 2026
…dow-padding-x

Follow-up to 541b77d (coordinator decision): the terminal host must put
the first text column on Metrics.paneContentInset with the user's real
window-padding-x, not Ghostty's default. GhosttyRuntime.terminalPadding
reads window-padding-x, window-padding-y and window-padding-balance through
ghostty_config_get; TerminalHostView subtracts each side's padding and
re-lays out on GhosttyRuntime.configDidChange (a config reload).

Red: the pinned libghostty (9961d09be) returns false for window-padding-x
and -y (WindowPadding has no C value), so the read falls back to 2 pt.
PaneAlignmentTests.theTerminalPaddingIsReadFromTheGhosttyConfig fails;
the GhosttyKit bump with manaflow-ai/ghostty#251 makes it pass.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
lawrencecchen added a commit to manaflow-ai/cmux that referenced this pull request Sep 30, 2026
…hosttyKit b1a49b601)

Bumps the ghostty submodule from 9961d09be to b1a49b601
(manaflow-ai/ghostty#251, merged as dcb1d6965; b1a49b601 is an ancestor of
the fork's main). ghostty_config_get now fills
ghostty_config_window_padding_s for window-padding-x and -y, so
GhosttyRuntime.terminalPadding reads the real values (with
window-padding-balance) and TerminalHostView puts the first text column on
Metrics.paneContentInset for any padding; a padding wider than the inset
leaves the surface at the host edge. The base 9d8d40319 adds only the
styled blank row test fix on top of 9961d09be.

- scripts/ghosttykit-checksums.txt pins the published artifact
  (build-ghosttykit run 36783660554, SHA-256 740cd227...43a9).
- build-ghosttykit.yml runs the Ghostty test "c_get: window padding"
  (red run 36783657036 at the test-only commit a230c2b25).
- docs/ghostty-fork.md records the change.

Green: PaneAlignmentTests.theTerminalPaddingIsReadFromTheGhosttyConfig.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@lawrencecchen
lawrencecchen deleted the cmux-config-get-window-padding branch September 30, 2026 22:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant