fix(sandbox): avoid faccessat2 for Termux/Android + build docs#519
fix(sandbox): avoid faccessat2 for Termux/Android + build docs#519PatrickNoFilter wants to merge 11 commits into
Conversation
- mouseModeForTermEnv(): detect Termux/PRoot/Android env → use CellMotion (CellMotion is reliable through PRoot unlike AllMotion/1003 tracking) - Add Ctrl+U/D half-page scroll keys (Termux-friendly, no PgUp/PgDn needed) - Add Shift+Up/Down line scroll keys - Update keybinding help overlay with new shortcuts
- Add focused-surface guards to Ctrl+U/D and Shift+Up/Down scroll keys (pendingPermission, pendingAskUser, providerWizard, mcpAddWizard, mcpManager, picker, suggestionsActive) - Remove unreliable PROOT_CWD and CONTAINER from Termux detection; use only TERMUX_VERSION, PREFIX, and ANDROID_ROOT
…tion - Move Ctrl+U/D and Shift+Up/Down scroll key cases to after KeyDown and KeyUp focused-surface handlers in the inner switch, using return-through-handler pattern instead of break. - Fix PREFIX detection to check for /data/data/com.termux/files/usr prefix instead of any non-empty PREFIX env var. - Add strings import to mouse.go for strings.HasPrefix.
…itignore - Move Shift+Up/Shift+Down before plain KeyUp/KeyDown so shifted-arrow shortcuts are reachable (were swallowed by plain-arrow keyIs match) - Swap Ctrl+U/Ctrl+D deltas for permission and ask-user focused prompts: Ctrl+U (up) now moves cursor UP (-1), Ctrl+D (down) moves DOWN (+1) - Add TestPermissionCursorCtrlU/D and TestAskUserCursorCtrlU/D regression tests verifying the direction fix - Remove unrelated zero-linux-sandbox entry from .gitignore
- Add composer-empty guard to Shift+Up/Down before scrollChat so shifted arrows navigate multiline text instead of scrolling - Add TestShiftUp/DownWithComposerDoesNotScroll regression tests - Add mouseModeForTermEnv tests: default/desktop, TERMUX_VERSION, PREFIX (Termux and unrelated), ANDROID_ROOT (/system and unrelated), and all Termux signals combined — each with per-test env cleanup
…t setup - transcriptScrollOffset -> chatScrollOffset (actual field name) - int != comparison instead of nonexistent .Equal() - AltScreen: true + width/height + transcript rows so tests actually exercise the scroll path and would catch the guard regression
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.
WalkthroughAdds Termux/Android build support (ignore entries, install docs, environment-aware mouse mode), replaces ChangesTermux/Android support
TUI keyboard scrolling
Estimated code review effort: 3 (Moderate) | ~25 minutes 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 (2)
internal/tui/permission_prompt_test.go (1)
333-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen composer-guard tests to confirm the key actually reaches the input field.
Both tests only assert the transcript doesn't scroll and no command is returned, but don't verify the key was actually forwarded to
m.inputfor multiline navigation (e.g., input cursor line/position change). As written, a bug that simply swallows the key entirely (does nothing) would also pass these assertions.🤖 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/permission_prompt_test.go` around lines 333 - 376, The composer-guard tests in TestShiftUpWithComposerDoesNotScroll and TestShiftDownWithComposerDoesNotScroll only check that chatScrollOffset stays unchanged and cmd is nil, so a bug that drops the key entirely would still pass. Update both tests to also assert the Shift+Up/Shift+Down key is forwarded into m.input for multiline navigation by checking an input state change such as cursor position/line movement after m.Update(testKeyShift(...)). Use the existing model and input symbols (m.Update, m.input, chatScrollOffset) to verify the key reaches the composer rather than being swallowed.internal/sandbox/manager.go (1)
125-169: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit tests for the new lookup helpers.
isExecutable/lookupExecutableare new, non-trivial cross-platform logic (permission bits, extension checks, PATH parsing, empty-segment skipping) but aren't covered by tests in this diff.internal/sandbox/manager_test.goalready exists as the natural place for this coverage. Want me to draft table-driven tests for both functions?🤖 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/sandbox/manager.go` around lines 125 - 169, The new helper logic in isExecutable and lookupExecutable is not covered by tests, so add table-driven cases in internal/sandbox/manager_test.go for Unix permissions, Windows-style executable extensions, PATH scanning, empty PATH segment skipping, and direct-path checks. Use the isExecutable and lookupExecutable symbols to verify regular files vs non-regular files, executable vs non-executable modes, and ensure lookupExecutable returns the expected path or error for both PATH-based and explicit-file lookups.
🤖 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 `@docs/INSTALL.md`:
- Around line 195-198: Reword the GOOS=android note in the INSTALL docs so it
does not imply the workaround only starts with Go 1.26+; the issue is the
version claim, and the fix is to state that Termux/Android builds require
GOOS=android because it makes os/exec.findExecutable avoid faccessat2 on Android
and fall back to permission-bit checks, regardless of toolchain version. Keep
the explanation tied to the Android/GOOS=android behavior and remove the
misleading version-specific wording.
---
Nitpick comments:
In `@internal/sandbox/manager.go`:
- Around line 125-169: The new helper logic in isExecutable and lookupExecutable
is not covered by tests, so add table-driven cases in
internal/sandbox/manager_test.go for Unix permissions, Windows-style executable
extensions, PATH scanning, empty PATH segment skipping, and direct-path checks.
Use the isExecutable and lookupExecutable symbols to verify regular files vs
non-regular files, executable vs non-executable modes, and ensure
lookupExecutable returns the expected path or error for both PATH-based and
explicit-file lookups.
In `@internal/tui/permission_prompt_test.go`:
- Around line 333-376: The composer-guard tests in
TestShiftUpWithComposerDoesNotScroll and TestShiftDownWithComposerDoesNotScroll
only check that chatScrollOffset stays unchanged and cmd is nil, so a bug that
drops the key entirely would still pass. Update both tests to also assert the
Shift+Up/Shift+Down key is forwarded into m.input for multiline navigation by
checking an input state change such as cursor position/line movement after
m.Update(testKeyShift(...)). Use the existing model and input symbols (m.Update,
m.input, chatScrollOffset) to verify the key reaches the composer rather than
being swallowed.
🪄 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: 04d7d66e-6540-453d-85f0-272007e663d3
📒 Files selected for processing (9)
.gitignoredocs/INSTALL.mdinternal/sandbox/manager.gointernal/tui/ask_user_test.gointernal/tui/keybinding_help.gointernal/tui/model.gointernal/tui/mouse.gointernal/tui/mouse_test.gointernal/tui/permission_prompt_test.go
| > **Why `GOOS=android`?** Go 1.26+ detects `runtime.GOOS == "android"` and skips | ||
| > the `faccessat2` syscall inside `os/exec.findExecutable`, falling back to | ||
| > permission-bit checks. Without this flag, Android's seccomp sends SIGSYS and | ||
| > kills the process whenever Zero looks up a binary on `PATH` (git, sh, etc.). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
When was the runtime.GOOS == "android" check added to syscall.Faccessat in the Go standard library to skip faccessat2?
💡 Result:
The check runtime.GOOS != "android" was added to syscall.Faccessat in the Go standard library to skip faccessat2 as part of the commit titled "syscall: don't use faccessat2 on android" [1]. This change was submitted and reviewed on December 20, 2022 (Change-Id: I8d4e12a6f46cea5642d3b5b5a02c682529882f29) [1] and subsequently merged into the Go source tree [2]. The modification was implemented to address issue #57393 [3], as the Android seccomp policy does not allow the faccessat2 system call, which previously caused programs to crash with a SIGSYS signal when attempting to use it [1][2]. By checking the operating system and excluding Android, the library avoids faccessat2 on that platform and falls back to the standard faccessat syscall [1].
Citations:
- 1: https://git.jordan.im/go/commit/?h=go1.20.13&id=58f6022eee95f43b4e0dc640b012bb3f574898f1
- 2: https://groups.google.com/g/golang-checkins/c/oWsfgb-1mZ0
- 3: syscall: Faccessat crashes because faccessat2 is blocked on Android golang/go#57393
Go 1.26+ is misleading — the Android faccessat2 skip has existed since 2022, so GOOS=android works on older Go toolchains too. Reword this to say the flag is required for Termux/Android builds without implying the workaround starts at Go 1.26.
🤖 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 `@docs/INSTALL.md` around lines 195 - 198, Reword the GOOS=android note in the
INSTALL docs so it does not imply the workaround only starts with Go 1.26+; the
issue is the version claim, and the fix is to state that Termux/Android builds
require GOOS=android because it makes os/exec.findExecutable avoid faccessat2 on
Android and fall back to permission-bit checks, regardless of toolchain version.
Keep the explanation tied to the Android/GOOS=android behavior and remove the
misleading version-specific wording.
1 participant
Summary
Two independent fixes for Termux/Android support, split from PR #509 per maintainer request.
1. Sandbox: avoid faccessat2 syscall
Replace
exec.LookPathwith alookupExecutablehelper usingos.Statinstead of thefaccessat2syscall (syscall 439). This syscall is blocked by Samsung's seccomp filter on Android, causing SIGSYS termination.os.Statusesnewfstatat/statxwhich are universally permitted.Also adds:
isExecutable()for cross-platform executable detection (Unix permission bits + Windows.exe/.com/.bat/.cmd)2. Termux build documentation
Added a
### Termux (Android)section todocs/INSTALL.mdcovering:GOOS=androidto avoid SIGSYS (Go 1.26+ stdlib fix)prootbind-mount for/etc/resolv.confCommits
Ref #505
Summary by CodeRabbit
New Features
Bug Fixes
Chores