Fix scrollback-limit parser to accept Ghostty K/M/G suffixes

[shaun0927]

Summary

• accept K/M/G/T byte suffixes in scrollback-limit (case-insensitive, optional trailing B), matching upstream Ghostty's own parser
• reject overflow on the suffix multiplication so a pathological literal can no longer trap
• existing plain-integer and _-grouped literal behaviour is preserved at the call site (parse, line 256)

Closes #2950

Root Cause

parseIntegerLiteral (Sources/GhosttyConfig.swift:320-328) only stripped _ digit-group separators. Upstream Ghostty's configuration parser accepts byte-typed values like 10MB, so a user copying the recommended example from upstream Ghostty docs into their ~/.config/ghostty/config silently got cmux's default. README advertises Ghostty config compatibility (Reads your existing ~/.config/ghostty/config), so the silent fallback is surprising.

Fix

parseIntegerLiteral extended to handle the same set of suffixes Ghostty itself accepts, with overflow-safe multiplication:

private static func parseIntegerLiteral(_ value: String) -> Int? {
    var normalized = value
        .trimmingCharacters(in: .whitespaces)
        .replacingOccurrences(of: "_", with: "")
        .lowercased()

    if normalized.hasSuffix("b") {
        normalized.removeLast()
    }

    let multiplier: Int
    switch normalized.last {
    case "k": multiplier = 1 << 10
    case "m": multiplier = 1 << 20
    case "g": multiplier = 1 << 30
    case "t": multiplier = 1 << 40
    default:  multiplier = 1
    }
    if multiplier > 1 {
        normalized.removeLast()
    }

    guard !normalized.isEmpty,
          let magnitude = Int(normalized),
          magnitude >= 0 else {
        return nil
    }

    let (scaled, overflow) = magnitude.multipliedReportingOverflow(by: multiplier)
    return overflow ? nil : scaled
}

The call site in parse (line 256) is untouched — the existing if let limit = Self.parseIntegerLiteral(value) keeps the silent-default behaviour for genuinely unparseable input, so any existing config that already silently fell through (typos, gibberish) still does.

Tests

cmuxTests/GhosttyConfigTests.swift — new GhosttyConfigScrollbackLimitParseTests:

• testAcceptsPlainIntegerValue — 12345 → 12345
• testAcceptsUnderscoreSeparatedInteger — 10_000_000 → 10_000_000 (regression for the existing PR Fix scrollback-limit byte handling #2927 behaviour)
• testAcceptsKilobyteSuffix — 256K → 262144
• testAcceptsKilobyteSuffixWithB — 256KB → 262144
• testAcceptsMegabyteSuffix — 10M → 10485760
• testAcceptsMegabyteSuffixCaseInsensitive — 10mb → 10485760
• testAcceptsGigabyteSuffix — 1G → 1073741824
• testRejectsGibberishKeepsDefault — abc keeps the default in place

Commits split as test first, fix second per repo policy, so the GitHub PR Commits tab shows the red→green transition.

Test plan

• new GhosttyConfigScrollbackLimitParseTests cases fail on the test-only commit, pass on the fix commit
• existing GhosttyConfigTests cases continue to pass

Notes

• Local tests were not run per repo policy.
• This PR scopes itself to the parser to keep the diff small and reviewable. The companion CHANGELOG entry covering the bytes-vs-lines semantics from Fix scrollback-limit byte handling #2927 is left to the next /release pass since CHANGELOG.md currently has no [Unreleased] section.

[vercel]

Someone is attempting to deploy a commit to the Manaflow Team on Vercel.

A member of the Team first needs to authorize it.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

[coderabbitai]

📝 Walkthrough

Walkthrough

Enhanced GhosttyConfig's integer parser to accept Ghostty-style byte-size literals (whitespace trimming, _ separators, optional B, K/M/G/T multipliers with overflow detection). Added tests covering valid suffixes, separators, invalid input, and overflow behavior for scrollback-limit.

Changes

Cohort / File(s)	Summary
Integer Literal Parser Sources/GhosttyConfig.swift	Rewrote parseIntegerLiteral(_:) to: trim whitespace, remove _ digit separators, lowercase, optionally strip trailing b/B, apply K/M/G/T multipliers via bit shifts, detect and reject arithmetic overflow, and return nil for invalid/empty inputs.
Scrollback Limit Parsing Tests cmuxTests/GhosttyConfigTests.swift	Added GhosttyConfigScrollbackLimitParseTests verifying parsing of raw integers, underscore-separated numbers, case-insensitive suffixed units (K/KB, M/MB, G, T/TB), B-only byte values, invalid inputs preservation of defaults, and overflow handling.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

• Fix scrollback-limit byte handling #2927 — Modified the same parseIntegerLiteral helper to add underscore-normalization and basic integer parsing; this PR extends that work with suffix handling and overflow checks.

Poem

🐰 I nibbled digits, stripped the underscores away,
I learned K, M, G, and T to hop and play.
No more silent defaults in the scrollback den,
Bytes counted true — hooray again! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and specifically describes the main change: fixing the scrollback-limit parser to accept Ghostty K/M/G suffixes, which is the primary objective of the pull request.
Linked Issues check	✅ Passed	The code changes fully address the primary coding-related requirements from issue #2950: accepting K/M/G/T byte suffixes (case-insensitive with optional B), using 1024 multipliers, and implementing overflow-safe multiplication.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to the parser extension and corresponding unit tests as described in the PR objectives. No extraneous changes to unrelated code are present.
Description check	✅ Passed	The pull request description is comprehensive and well-structured, covering summary, root cause, fix with code example, tests, test plan, and notes.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (2)

Sources/GhosttyConfig.swift (1)

320-354: LGTM — parser logic is sound.

The walkthrough holds for the documented shapes ("10MB", "256K", "1024", "10_000_000", "abc"), the empty/"kb"/"b"-only cases correctly return nil via the guard, and multipliedReportingOverflow gives a clean overflow path. 1 << 40 is safe on Swift's 64-bit Int on macOS.

Two small notes worth considering (non-blocking):

• Behavior change (intentional?): the magnitude >= 0 guard now rejects negative literals (e.g. "-100"), whereas the prior Int(value) path would have accepted them. Downstream clamps via max(config.scrollbackLimit, 0) anyway, so this is an improvement, but it's worth noting in the PR description / changelog since it's a small semantic shift beyond suffix support.
• Unsupported variants: Ghostty's own parser also accepts the IEC KiB/MiB/GiB spellings on some byte-typed fields. If you want full parity, the i between digit and b will currently fall into the default branch and return nil. Easy to extend later if users report it.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@Sources/GhosttyConfig.swift` around lines 320 - 354, Update the PR/changelog
to note that parseIntegerLiteral now rejects negative literals (the magnitude >=
0 guard in parseIntegerLiteral) which is a behavioral change vs the previous
Int(value) path, and optionally extend parseIntegerLiteral's suffix handling
(the switch on normalized.last and the hasSuffix/removeLast logic) to accept
IEC-style "kib/mib/gib" spellings by stripping a trailing "ib"
(case-insensitive) or treating an 'i' before 'b' as part of the suffix so
"KiB"/"MiB"/etc. parse like "K"/"M"/"G"; keep overflow semantics using
multipliedReportingOverflow unchanged.

cmuxTests/GhosttyConfigTests.swift (1)

4044-4084: Consider adding overflow and bare-B coverage.

The added cases nicely document the happy paths and the gibberish fallback. Two small additions would make the suite more defensive against future regressions in parseIntegerLiteral:

♻️ Optional extra cases

+    func testRejectsOverflowingSuffixedValueKeepsDefault() {
+        // multipliedReportingOverflow must short-circuit pathological inputs
+        // like "999999999G" instead of wrapping to a negative byte count.
+        let defaultLimit = GhosttyConfig().scrollbackLimit
+        XCTAssertEqual(parsedScrollbackLimit(from: "999999999G"), defaultLimit)
+    }
+
+    func testAcceptsBareByteSuffix() {
+        // "1024B" should be treated as 1024 bytes (no K/M/G multiplier).
+        XCTAssertEqual(parsedScrollbackLimit(from: "1024B"), 1024)
+    }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@cmuxTests/GhosttyConfigTests.swift` around lines 4044 - 4084, Add two
defensive tests to GhosttyConfigScrollbackLimitParseTests: one that verifies a
bare 'B' suffix is parsed as bytes (e.g.
XCTAssertEqual(parsedScrollbackLimit(from: "256B"), 256)) and one that verifies
an overflowing numeric literal does not change the default (use a very large
numeric string and assert it equals GhosttyConfig().scrollbackLimit). Add them
alongside the other test methods (referencing parsedScrollbackLimit and
GhosttyConfig) so parseIntegerLiteral regressions are caught.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@cmuxTests/GhosttyConfigTests.swift`:
- Around line 4044-4084: Add two defensive tests to
GhosttyConfigScrollbackLimitParseTests: one that verifies a bare 'B' suffix is
parsed as bytes (e.g. XCTAssertEqual(parsedScrollbackLimit(from: "256B"), 256))
and one that verifies an overflowing numeric literal does not change the default
(use a very large numeric string and assert it equals
GhosttyConfig().scrollbackLimit). Add them alongside the other test methods
(referencing parsedScrollbackLimit and GhosttyConfig) so parseIntegerLiteral
regressions are caught.

In `@Sources/GhosttyConfig.swift`:
- Around line 320-354: Update the PR/changelog to note that parseIntegerLiteral
now rejects negative literals (the magnitude >= 0 guard in parseIntegerLiteral)
which is a behavioral change vs the previous Int(value) path, and optionally
extend parseIntegerLiteral's suffix handling (the switch on normalized.last and
the hasSuffix/removeLast logic) to accept IEC-style "kib/mib/gib" spellings by
stripping a trailing "ib" (case-insensitive) or treating an 'i' before 'b' as
part of the suffix so "KiB"/"MiB"/etc. parse like "K"/"M"/"G"; keep overflow
semantics using multipliedReportingOverflow unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ecd073bd-f138-4436-9bca-0e7065db6236

📥 Commits

Reviewing files that changed from the base of the PR and between 3f1ce4b and b6dcdee.

📒 Files selected for processing (2)

• Sources/GhosttyConfig.swift
• cmuxTests/GhosttyConfigTests.swift

[greptile-apps]

Greptile Summary

Extends parseIntegerLiteral in GhosttyConfig.swift to accept Ghostty-style byte suffixes (K, M, G, T, with optional trailing B, case-insensitive) and adds overflow-safe multiplication, fixing silent fallback to the default when users copy configs that include values like scrollback-limit = 10MB from upstream Ghostty docs. The implementation logic is correct: the two-pass strip (b first, then k/m/g/t) handles all suffix combinations properly, overflow is guarded via multipliedReportingOverflow, and the existing if let call site preserves silent-default semantics for genuinely unparseable input.

Confidence Score: 5/5

Safe to merge; the only findings are missing test coverage for the T/TB suffix, bare-B suffix, and overflow path — none affect correctness of the shipped code.

All P0/P1 concerns are absent. The single comment is a P2 test-coverage gap for three edge cases that are implemented correctly but untested. Core logic and overflow protection are sound on the 64-bit macOS target.

cmuxTests/GhosttyConfigTests.swift — T/TB suffix, bare-B suffix, and overflow rejection are missing tests.

Important Files Changed

Filename	Overview
Sources/GhosttyConfig.swift	Extended parseIntegerLiteral to handle K/M/G/T byte-suffix parsing with overflow-safe multiplication; logic is correct for all covered cases
cmuxTests/GhosttyConfigTests.swift	New GhosttyConfigScrollbackLimitParseTests covers the main suffix variants but is missing tests for the T/TB terabyte suffix, overflow rejection, and standalone B suffix

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["parseIntegerLiteral(value)"] --> B["trim whitespace\nstrip underscores\nlowercased()"]
    B --> C{"ends with 'b'?"}
    C -->|yes| D["removeLast() — strip 'b'"]
    C -->|no| E["unchanged"]
    D --> F{"last char?"}
    E --> F
    F -->|k| G["multiplier = 1<<10"]
    F -->|m| H["multiplier = 1<<20"]
    F -->|g| I["multiplier = 1<<30"]
    F -->|t| J["multiplier = 1<<40"]
    F -->|other| K["multiplier = 1"]
    G --> L["removeLast() — strip suffix"]
    H --> L
    I --> L
    J --> L
    K --> M["no strip"]
    L --> N{"normalized empty\nor Int() nil\nor magnitude < 0?"}
    M --> N
    N -->|yes| O["return nil → keep default"]
    N -->|no| P["multipliedReportingOverflow"]
    P -->|overflow| O
    P -->|ok| Q["return scaled Int"]

Loading

_{Reviews (1): Last reviewed commit: "fix: accept Ghostty K/M/G/T byte suffixe..." | Re-trigger Greptile}

[greptile-apps]

Missing test cases for T/TB suffix, overflow, and bare B suffix

The code adds explicit support for T/TB (terabyte), B-only (bare bytes), and overflow rejection via multipliedReportingOverflow, but no test exercises any of these three paths. A few additions would close the gap:

func testAcceptsTerabyteSuffix() {
    XCTAssertEqual(parsedScrollbackLimit(from: "1T"), 1 << 40)
}

func testAcceptsTerabyteSuffixWithB() {
    XCTAssertEqual(parsedScrollbackLimit(from: "1TB"), 1 << 40)
}

func testAcceptsBareBytesSuffix() {
    XCTAssertEqual(parsedScrollbackLimit(from: "100B"), 100)
}

func testOverflowKeepsDefault() {
    let defaultLimit = GhosttyConfig().scrollbackLimit
    // 9_999_999_999G overflows Int64 → should fall back to default
    XCTAssertEqual(parsedScrollbackLimit(from: "9999999999G"), defaultLimit)
}

[cubic-dev-ai]

1 issue found across 2 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="cmuxTests/GhosttyConfigTests.swift">

<violation number="1" location="cmuxTests/GhosttyConfigTests.swift:4044">
P2: New scrollback-limit parser branches (`T` suffix and overflow fallback) are added but not tested, leaving regression-prone paths unprotected.</violation>
</file>

_{Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.}