Repository navigation
tty field always null in --json tree output - #3236
lawrencecchen wants to merge 2 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe shell integration scripts (bash and zsh) now preserve full TTY device paths instead of extracting just the basename. TerminalController adds a normalization helper that extracts the basename when registering TTYs with PortScanner, and test expectations are updated to match the new behavior. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 fixes the Confidence Score: 4/5Safe to merge; the fix is minimal and well-scoped with only P2 suggestions remaining. All findings are P2 (missing test coverage for the new helper and one stale fixture). The core logic — removing basename stripping in both shell integrations and re-normalising for PortScanner — is correct, and the remote-workspace codepath already had its own normalisation that is unaffected. Sources/TerminalController.swift — Important Files Changed
Sequence DiagramsequenceDiagram
participant Shell as Shell (bash/zsh)
participant Socket as Unix Socket
participant TC as TerminalController
participant WS as Workspace.surfaceTTYNames
participant PS as PortScanner
participant API as --json tree
Note over Shell: OLD: t="${t##*/}" → ttys001
Note over Shell: NEW: full path kept → /dev/ttys001
Shell->>Socket: report_tty /dev/ttys001 --tab=X
Socket->>TC: reportTTY("/dev/ttys001 --tab=X")
TC->>WS: surfaceTTYNames[panelId] = "/dev/ttys001"
TC->>TC: portScanTTYName("/dev/ttys001") → "ttys001"
TC->>PS: registerTTY(ttyName: "ttys001")
PS->>PS: ps -t ttys001 (basename, unchanged)
API->>WS: surfaceTTYNames[panelId]
WS-->>API: "/dev/ttys001" (was null before fix)
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private static func portScanTTYName(from ttyName: String) -> String { | ||
| let trimmed = ttyName.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| let candidate = trimmed.split(separator: "/").last.map(String.init) ?? trimmed | ||
| return candidate.isEmpty ? trimmed : candidate | ||
| } |
There was a problem hiding this comment.
portScanTTYName
The new portScanTTYName(from:) helper is the critical bridge that prevents a regression in port-scan behavior: it must convert /dev/ttys001 → ttys001 while leaving bare names like ttys001 untouched. It currently has no dedicated unit test. A simple parametric test covering /dev/ttys001, ttys001, an empty string, and a trailing-slash edge case (e.g. /dev/ttys001/) would lock in the invariant and prevent future accidental changes.
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
2 participants
Addresses #3098
Generated by cmux-loader.
Codex final message:
Implemented in branch
loader/issue-3098-tty-field-always-null-in-json-tree-output:Fixed terminal tty capture to preserve full device path in shell integration:
ttysNNNbasename truncation (${t##*/}) so reported tty values remain/dev/tty...for API output.Preserved API-visible tty path while keeping port scanner behavior unchanged:
portScanTTYName(from:)helper to normalize to basename only wherePortScannerregistration needs it.workspace.surfaceTTYNames[...]populated with full tty path socmux --json treenow gets populatedtty.Updated tests to match new full-path tty payloads:
Verification performed:
./scripts/reload.sh --tag loaderzigis not installed (Error: zig is not installed. Install via: brew install zig).zigis available.Pull request:
gh auth statusreports no GitHub authentication in this environment, so I couldn’t create/update a PR or push for PR creation here.If you want, I can also provide the exact PR title/body text to use when you create it from your authenticated environment.
Summary by CodeRabbit
Improvements
Summary by cubic
Fixes the
ttyfield being null incmux --json treeby preserving the full TTY device path from shell integration. The JSON now includes values like/dev/ttys001instead of null.Bug Fixes
/dev/...path.TerminalController: added a helper to pass only the basename toPortScanner, while keeping the full path in API payloads.ttyvalues.Refactors
.github/swift-file-length-budget.tsvforSources/TerminalController.swift.Written for commit e294143. Summary will update on new commits. Review in cubic