fix: Termux/Android support — PRoot scroll, SIGSYS sandbox, build docs#509
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR extends TUI keyboard navigation and PRoot detection, adds related tests and Termux docs, and replaces sandbox helper discovery with manual executable lookup. ChangesTUI navigation and Termux support
Sandbox executable discovery
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/tui/mouse.go (1)
11-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNo direct test coverage for
isRunningUnderPRoot.The function reads a hardcoded path (
/proc/self/status), making it hard to unit-test the parsing logic (missing prefix, malformedTracerPidvalue, empty file, etc.) directly. Extracting the parsing into a small helper that accepts the file contents (or anio.Reader) as a parameter would let tests exercise the TracerPid-parsing branch without depending onsync.OnceValue's process-wide caching or actual/proccontents.♻️ Suggested refactor for testability
+func parseTracerPid(data []byte) bool { + for _, line := range strings.Split(string(data), "\n") { + if strings.HasPrefix(line, "TracerPid:") { + pid := strings.TrimSpace(line[10:]) + return pid != "0" + } + } + return false +} + var isRunningUnderPRoot = sync.OnceValue(func() bool { data, err := os.ReadFile("/proc/self/status") if err != nil { return false } - for _, line := range strings.Split(string(data), "\n") { - if strings.HasPrefix(line, "TracerPid:") { - pid := strings.TrimSpace(line[10:]) - return pid != "0" - } - } - return false + return parseTracerPid(data) })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/tui/mouse.go` around lines 11 - 23, The parsing logic inside isRunningUnderPRoot is hard to test because it is tied to os.ReadFile and sync.OnceValue caching. Extract the TracerPid parsing into a small helper that accepts the file contents or an io.Reader, and keep isRunningUnderPRoot focused on reading /proc/self/status and calling that helper. This will let tests cover missing TracerPid, malformed values, and empty input directly without depending on real /proc data.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/tui/mouse.go`:
- Around line 11-23: The current isRunningUnderPRoot check in mouse.go uses
TracerPid from /proc/self/status, which also matches debuggers and other ptrace
tracers. Either replace this with a more specific PRoot detection in the same
helper or, if the broader fallback is intended, add a short inline note
documenting that gdb, strace, and dlv will also trigger CellMotion behavior.
---
Nitpick comments:
In `@internal/tui/mouse.go`:
- Around line 11-23: The parsing logic inside isRunningUnderPRoot is hard to
test because it is tied to os.ReadFile and sync.OnceValue caching. Extract the
TracerPid parsing into a small helper that accepts the file contents or an
io.Reader, and keep isRunningUnderPRoot focused on reading /proc/self/status and
calling that helper. This will let tests cover missing TracerPid, malformed
values, and empty input directly without depending on real /proc data.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 02a88ebc-018f-4b19-8157-09485034df0e
📒 Files selected for processing (3)
internal/tui/model.gointernal/tui/mouse.gointernal/tui/permission_prompt_test.go
|
Thanks for coming back with the Before we go further, we want to understand the actual use case: what's the reason to run Zero inside PRoot at all, given Termux already works natively (#455)? PRoot exists to solve problems like missing packages, FHS path assumptions, or glibc requirements — none of which apply to Zero, since it's a single self-contained Go binary with no runtime dependency on any of that. Genuinely asking — if there's a real workflow reason (something specific PRoot gives you that native Termux doesn't, for running Zero specifically), we'd like to hear it, since that changes the calculus here. If the answer turns out to be "no strong reason, just how I had my environment set up," then we'd lean toward not carrying this: Separately, worth fixing regardless of the outcome above: this PR references #507, which is about One thing that stands on its own regardless: the Keeping this open for your take on the use-case question above. |
|
The use case for running Zero inside PRoot: Hermes Agent (an AI agent framework by Nous Research) uses Zero as its terminal UI. Hermes runs inside a PRoot/Alpine sandbox on Termux because:
So the PRoot detection is just one part of a larger set of fixes. |
|
can you address coderabbit comments @PatrickNoFilter ? |
- Replace Termux env var detection with isRunningUnderPRoot() via /proc/self/status TracerPid, cached with sync.OnceValue - Mouse mode falls back to CellMotion (not AllMotion) under PRoot to fix broken touch scrolling - Add Shift+Up/Shift+Down before plain KeyDown/KeyUp for transcript scroll with permission/askuser cursor + composer guard - Add Ctrl+U (scroll up) and Ctrl+D (scroll down) with proper direction: Ctrl+U=move up, Ctrl+D=move down - Add guards for active modals (wizard, picker, spec review, etc.) to avoid intercepting their Ctrl+U/D keybindings - Tests: TestPermissionCursorCtrlU/D (direction regression), TestShiftUp/DownComposerGuard (composer guard) - References issue Gitlawb#507
PatrickNoFilter
force-pushed
the
fix/tui-scroll-prroot
branch
from
1306ab5 to
73d69ea
Compare
|
@kevincodex1 — addressed both CodeRabbit items on 1. TracerPid matches debuggers — Added an inline comment in 2. Testability — Extracted All existing mouse + permission tests continue to pass. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verified this locally since CI is gated on fork PRs — build's clean and the tui suite passes. The fix is careful.
The PRoot detection is the good kind of thoughtful. TracerPid via /proc/self/status is more robust than sniffing Termux env vars, and you pre-empted the obvious objection right in the comment: it detects any ptrace tracer (gdb/strace/dlv too), not PRoot specifically, and that's fine because CellMotion-under-a-tracer only costs hover highlighting. parseTracerPid taking []byte instead of reading the file is exactly right for testing, and sync.OnceValue keeps it to one read.
Directions all check out against the existing convention (positive = up, from PgUp): Shift+Up = scrollChat(1), Shift+Down = scrollChat(-1), Ctrl+U = +page (up), Ctrl+D = -page (down). The guard order is right too — shifted arrows and Ctrl+U/D are handled before the plain-arrow/composer path so they aren't swallowed, with proper fallthrough to the permission/ask-user cursors, the wizards, the picker, and a composer guard for multiline navigation.
Two small things, neither blocking:
- The Ctrl+U/D comments say "half a page," but
chatPageScrollLines()is a full page (height-8), so Ctrl+U/D scroll the same amount as PgUp/PgDn. Either make them half or fix the comment — vim/less users expect Ctrl+U/D to be a half page. - Heads up that this needs a maintainer to approve the workflow run: the Go CI is gated on fork PRs and hasn't executed yet, so I ran build + the tui suite locally to stand in for it.
On process: the bug issue #505 got closed "completed" at the same moment #503 closed, but #503 never merged, so the bug is still live in main — this is the actual fix. I've reopened #505 and marked it issue-approved.
Good work. Approving.
Replace exec.LookPath with lookupExecutable that uses os.Stat instead of the faccessat2 syscall. The faccessat2 syscall (439) is blocked by the Samsung seccomp filter on Android, causing SIGSYS termination. os.Stat uses the newfstatat/statx syscalls which are universally permitted. This is part of broader Termux/Android support for Zero.
Document how to build Zero for native Android/Termux: GOOS=android to avoid the SIGSYS crash, proot bind-mount for DNS resolution, PRoot scroll fix, and free-tier provider setup.
PatrickNoFilter
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/sandbox/manager.go`:
- Around line 125-153: The PATH lookup logic in lookupExecutable needs to handle
Windows executables correctly and avoid treating empty PATH entries as the
current directory. Update the PATH-discovery branch in lookupExecutable to use a
platform-aware executable check for Windows instead of relying on
fi.Mode()&0o111, and ensure empty PATH segments are skipped so
selectPlatformBackend and its lookupExecutable fallback do not reintroduce
dot-path resolution.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 713b0a6b-7f27-4862-9fa1-9a7cf2e892b9
📒 Files selected for processing (3)
.gitignoredocs/INSTALL.mdinternal/sandbox/manager.go
✅ Files skipped from review due to trivial changes (2)
- docs/INSTALL.md
- .gitignore
kevincodex1
left a comment
kevincodex1
left a comment
There was a problem hiding this comment.
looks good to me! thanks
|
A few updates now that more of this has resolved: Syscall fix: good catch, but it actually cuts against the PRoot case, not for it — PRoot is the standard workaround for exactly the syscall-blocking problem this fixes. Once Zero stops making the blocked call, that's one less real reason to need PRoot for Zero specifically. Hermes Agent: asked twice, got the same unsubstantiated claim back twice. Need something checkable this time — a repo link, docs, a config, anything showing Hermes Agent actually embedding Zero as its terminal UI. If it's not shareable, that's a fine answer too, but it changes what we merge. Process: PR still says "Ref #507" (the unrelated Given all that, let's split it: Ctrl+U/D + Shift+Up/Down + the syscall fix + the Termux docs stand on their own merit and don't need the PRoot question answered — that can merge once the half-page comment @Vasanthdev2004 flagged is fixed. |
|
Update: this PR was actually merged into main as For anyone running Zero on Termux/Android: build with |
4 participants
Summary
Three Termux/Android portability fixes for Zero:
1. PRoot scroll fix (TUI mouse mode)
Replace Termux env-var detection with
isRunningUnderPRoot()via/proc/self/statusTracerPid, cached withsync.OnceValue. Under PRoot, falls back toCellMotioninstead ofAllMotion— fixing broken touch-gesture scrolling while keeping wheel events, clicks, and drag working.Includes non-controversial TUI scroll fixes:
KeyUp/KeyDownwith permission/ask-user cursor and composer guard2. Sandbox
faccessat2avoidanceReplace
exec.LookPathwith alookupExecutablehelper that usesos.Statinstead of thefaccessat2syscall (syscall 439). This syscall is blocked by Samsung's seccomp filter on Android, causing SIGSYS termination.os.Statuses thenewfstatat/statxsyscalls which are universally permitted.3. Termux build documentation
Added
docs/INSTALL.mdsection covering:GOOS=androidto avoid SIGSYS (Go 1.26+ stdlib fix)prootbind-mount for/etc/resolv.confCommits
1306ab5 fix(tui): TracerPid PRoot detection, Ctrl+U/D direction, composer guard
c8f899d fix(sandbox): avoid faccessat2 for Termux/Android portability
9696920 docs(install): add Termux/Android build and configuration guide
Notes
main(f401c66), not rebased from the closed PR fix(tui): Termux/Android touch-scroll via CellMotion + alternative scroll keys #503Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests