Add native macOS menu bar app; fix SIGTERM graceful shutdown - #1
Conversation
Menu bar app (Swift/AppKit, SwiftPM): status icon with live health check, start/stop the Python bridge as a child process (posix_spawn, own process group), open logs, reveal log file, copy port. Builds with 'swift build' in macos/MenuBarApp; macOS 13+. Fix graceful shutdown: serve_forever now runs on a background thread so the SIGTERM handler's server.shutdown() cannot deadlock on the main thread. Regression test starts the real server and asserts SIGTERM stops it within 8s.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughAdds a native macOS menu bar app that controls a local proxy, manages logs, and displays health status. Updates proxy shutdown to avoid SIGTERM deadlock and adds an integration test for clean termination. ChangesProxy lifecycle
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant AppDelegate
participant ProxyController
participant uvx as uvx proxy
AppDelegate->>ProxyController: Start or stop proxy
ProxyController->>uvx: Spawn or terminate process group
ProxyController->>uvx: Poll health endpoint
uvx-->>ProxyController: Return health and liveness
ProxyController-->>AppDelegate: Publish proxy state
AppDelegate-->>AppDelegate: Refresh menu status
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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: 6
🤖 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 `@macos/MenuBarApp/README.md`:
- Around line 26-29: Update the release instructions in the README to document
the executable path produced by swift build -c release, and remove the cp
command that implies complete app bundling. Do not present OpenCodeGoMenuBar.app
creation as valid unless a complete bundle with its required metadata is
actually provided.
In `@macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift`:
- Around line 42-51: Update the descriptor setup around the `logFD` and `errFD`
`open()` calls so a descriptor opened before the other fails is closed before
returning through `failStart`. Install cleanup before the validity guard or
explicitly close each descriptor whose value is nonnegative, while preserving
the existing deferred cleanup for successful startup.
- Around line 85-89: Keep the menu bar process alive until the child process
group exits: have ProxyController’s spawnInGroup()/monitorChild() report
shutdown completion after SIGTERM or the SIGKILL fallback, and update
AppDelegate’s quitApp()/applicationWillTerminate() flow to observe that
completion before calling NSApp.terminate(nil). In ProxyController.swift lines
85-89, preserve the delayed fallback while ensuring it can execute; in
AppDelegate.swift lines 113-120, defer termination until the reported child
shutdown, using applicationShouldTerminate if appropriate.
- Around line 58-65: Update the uvx source entry in the argv construction to pin
git+https://github.com/kartikkabadi/opencode-go-proxy to the reviewed immutable
commit SHA, using uvx’s `@ref` syntax. Keep the existing proxy command arguments
and configuration unchanged.
In `@src/opencode_go_proxy/app.py`:
- Around line 643-647: Reorder the startup sequence around serve_thread and
server so serve_thread.start() runs before registering the SIGTERM handler; keep
serve_thread.join() afterward and preserve the existing server.shutdown
callback.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9292f771-aec8-4d73-91a1-5d776f3cd476
📒 Files selected for processing (9)
CHANGELOG.mdmacos/MenuBarApp/.gitignoremacos/MenuBarApp/Package.swiftmacos/MenuBarApp/README.mdmacos/MenuBarApp/Sources/OpenCodeGoMenuBar/AppDelegate.swiftmacos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swiftmacos/MenuBarApp/Sources/OpenCodeGoMenuBar/main.swiftsrc/opencode_go_proxy/app.pytests/test_integration.py
| ```bash | ||
| swift build -c release | ||
| cp -R .build/release/OpenCodeGoMenuBar OpenCodeGoMenuBar.app/Contents/MacOS/ | ||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Do not describe this as a complete app-bundling procedure.
Line 28 fails when OpenCodeGoMenuBar.app/Contents/MacOS does not exist. A valid macOS app bundle also needs metadata such as Info.plist. Replace this section with the release binary path, or provide a packaging script that creates a complete bundle.
Proposed documentation fix
- To bundle it as a .app:
-
- ```bash
- swift build -c release
- cp -R .build/release/OpenCodeGoMenuBar OpenCodeGoMenuBar.app/Contents/MacOS/
- ```
+ A release build places the executable at:
+
+ ```bash
+ swift build -c release
+ .build/release/OpenCodeGoMenuBar
+ ```📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ```bash | |
| swift build -c release | |
| cp -R .build/release/OpenCodeGoMenuBar OpenCodeGoMenuBar.app/Contents/MacOS/ | |
| ``` | |
| A release build places the executable at: | |
🤖 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 `@macos/MenuBarApp/README.md` around lines 26 - 29, Update the release
instructions in the README to document the executable path produced by swift
build -c release, and remove the cp command that implies complete app bundling.
Do not present OpenCodeGoMenuBar.app creation as valid unless a complete bundle
with its required metadata is actually provided.
| let logFD = open(logPath, O_WRONLY | O_CREAT | O_APPEND, 0o644) | ||
| let errFD = open(errPath, O_WRONLY | O_CREAT | O_APPEND, 0o644) | ||
| guard logFD >= 0, errFD >= 0 else { | ||
| failStart("Could not open log files under \(logDir.path)") | ||
| return | ||
| } | ||
| defer { | ||
| close(logFD) | ||
| close(errFD) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Close any descriptor that opened before a later open fails.
If one open() succeeds and the other fails, Line 46 returns before the defer block. The successful descriptor remains open. Close both valid descriptors before failStart, or install cleanup before the guard.
🤖 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 `@macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift` around
lines 42 - 51, Update the descriptor setup around the `logFD` and `errFD`
`open()` calls so a descriptor opened before the other fails is closed before
returning through `failStart`. Install cleanup before the validity guard or
explicitly close each descriptor whose value is nonnegative, while preserving
the existing deferred cleanup for successful startup.
| let argv: [String] = [ | ||
| uvx, | ||
| "--from", "git+https://github.com/kartikkabadi/opencode-go-proxy", | ||
| "opencode-go-proxy", | ||
| "--bind", "127.0.0.1", | ||
| "--port", "\(state.port)", | ||
| "--chat-base-url", "https://opencode.ai/zen/go/v1", | ||
| ] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Replace with the reviewed commit selected for the release.
git ls-remote https://github.com/kartikkabadi/opencode-go-proxy.git <reviewed-commit-sha>Repository: kartikkabadi/opencode-go-proxy
Length of output: 227
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Repo files around ProxyController:\n'
git ls-files | rg '(^|/)ProxyController\.swift$|macos/MenuBarApp/Sources/OpenCodeGoMenuBar' || true
printf '\nProxyController outline:\n'
ast-grep outline macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift --view expanded || true
printf '\nProxyController relevant lines:\n'
cat -n macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift | sed -n '1,140p'
printf '\nSearch for UVX uvx usage across repo:\n'
rg -n 'uvx|opencode-go-proxy|ProxyController|proxy API|API key|proxy' . || true
printf '\nInspect dependency metadata for uv lock/version if present:\n'
git ls-files | rg '(^|/)(uv\.lock|requirements.*\.txt|pyproject\.toml|Package\.swift|Package\.resolved|Info\.plist)$' || trueRepository: kartikkabadi/opencode-go-proxy
Length of output: 19797
🌐 Web query:
astral uv uvx --from git repository branch resolves default branch
💡 Result:
When using the --from option with a Git repository in uvx, if no specific reference (such as a branch, tag, or commit) is provided after the repository URL, uv defaults to the repository's default branch [1][2][3]. To specify a particular branch, you must append the branch name to the repository URL using the @ symbol [1][3]. The syntax for this is: uvx --from git+https://github.com/user/repo@branch-name package-name For example, to run a tool from the main branch of a repository, you would use: uvx --from git+https://github.com/user/repo@main package-name If you do not specify a branch, tag, or commit hash with the @ symbol, uv will resolve the dependency using the repository's default branch [1][2][4].
Citations:
- 1: https://docs.astral.sh/uv/guides/tools/
- 2: https://mcginniscommawill.com/posts/2026-04-01-uvx-run-from-github/
- 3: https://docs.bswen.com/blog/2026-03-05-uvx-git-branch/
- 4: https://engineered.at/articles/uvx-from-run-any-python-tool-directly-from-github
Pin the uvx source to an immutable commit.
uvx --from git+https://github.com/kartikkabadi/opencode-go-proxy resolves the repository default branch without a @ ref. Use a reviewed commit SHA so changed or rewritten default-branch code cannot execute under the proxy API-key environment.
Proposed fix
- "--from", "git+https://github.com/kartikkabadi/opencode-go-proxy",
+ "--from", "git+https://github.com/kartikkabadi/opencode-go-proxy@<reviewed-commit-sha>",🤖 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 `@macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift` around
lines 58 - 65, Update the uvx source entry in the argv construction to pin
git+https://github.com/kartikkabadi/opencode-go-proxy to the reviewed immutable
commit SHA, using uvx’s `@ref` syntax. Keep the existing proxy command arguments
and configuration unchanged.
| DispatchQueue.global().asyncAfter(deadline: .now() + 5) { | ||
| if kill(-pid, 0) == 0 { | ||
| kill(-pid, SIGKILL) | ||
| } | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files of interest:"
git ls-files | rg '(^|/)ProxyController\.swift$|(^|/)AppDelegate\.swift$|\.xcodeproj|package\.swift$' || true
echo
echo "ProxyController outline:"
ast-grep outline 'macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift' || true
echo
echo "ProxyController relevant section:"
cat -n 'macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift' | sed -n '1,220p'
echo
echo "AppDelegate relevant section:"
cat -n 'macos/MenuBarApp/Sources/OpenCodeGoMenuBar/AppDelegate.swift' | sed -n '90,150p'Repository: kartikkabadi/opencode-go-proxy
Length of output: 10094
🌐 Web query:
GNU libc POSIX ignore SIGTERM SIGKILL kill -pgid SIGKILL queued signal
💡 Result:
Under POSIX standards and the GNU C Library (glibc) implementation, the signals SIGTERM and SIGKILL have fundamentally different behaviors [1][2][3]. SIGTERM vs. SIGKILL - SIGTERM: This is a generic termination signal that a process can catch, block, or ignore [1][2][4]. It is intended to be a polite request for a process to terminate, allowing it to perform cleanup tasks like saving state or closing files before exiting [1][4]. - SIGKILL: This signal is used for immediate process termination and cannot be caught, blocked, or ignored [1][2][5]. It is always fatal, and any attempt to register a handler or set it to be ignored will fail [4][6][7]. If SIGKILL fails to terminate a process, it is typically considered an operating system bug [1][3]. Queuing Behavior - Standard Signals: Under Linux and standard POSIX-conforming implementations, standard signals (including SIGTERM) generally do not queue [5][8]. If multiple instances of a standard signal are generated while that signal is currently blocked, only one instance is marked as pending [8]. - Real-time Signals: Multiple instances of real-time signals can be queued [8]. - SIGKILL: Because SIGKILL cannot be blocked, it does not wait in a queue for a process to unblock it; it is processed immediately [1][5]. When sent to a process, the kernel enforces its termination [1][3]. If a process is in a state where it cannot process signals (e.g., certain uninterruptible sleep states), it may appear unresponsive, but the signal remains a强制 (forced) termination mechanism [1]. In summary, a process can ignore SIGTERM if it explicitly programs its signal handlers to do so (e.g., using sigaction with SIG_IGN), but it is impossible for a process to ignore SIGKILL [1][4][9]. Because SIGKILL cannot be blocked, it is never queued behind a block mask; it causes immediate termination upon delivery [1][5].
Citations:
- 1: https://snapshots.sourceware.org/glibc/trunk/2026-07-21_14-36_1784644561/manual/html_node/Termination-Signals.html
- 2: https://sourceware.org/glibc/manual/2.31/html_node/Termination-Signals.html
- 3: https://sourceware.org/glibc/manual/2.39/html_node/Termination-Signals.html
- 4: https://sourceware.org/glibc/manual/2.42/html_node/Basic-Signal-Handling.html
- 5: https://man7.org/linux/man-pages/man7/signal.7.html
- 6: https://man.openbsd.org/OpenBSD-7.9/signal.3
- 7: https://docs.python.org/3/library/signal.html
- 8: https://stackoverflow.com/questions/58772075/linux-does-not-implement-posix-signal-queuing
- 9: https://stackoverflow.com/questions/62342802/why-sigterm-does-not-kill-some-processes-in-linux
Keep the menu bar process alive until the child process group exits.
ProxyController.stop() posts SIGTERM to the child process group, schedules a SIGKILL fallback, and then returns. quitApp() and applicationWillTerminate() then terminate the menu bar process immediately, so the delayed fallback may be delivered to an already-shut-down process rather than the target child group. Report and observe shutdown completion from spawnInGroup()/monitorChild() before calling NSApp.terminate(nil), for example via applicationShouldTerminate.
📍 Affects 2 files
macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift#L85-L89(this comment)macos/MenuBarApp/Sources/OpenCodeGoMenuBar/AppDelegate.swift#L113-L120
🤖 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 `@macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift` around
lines 85 - 89, Keep the menu bar process alive until the child process group
exits: have ProxyController’s spawnInGroup()/monitorChild() report shutdown
completion after SIGTERM or the SIGKILL fallback, and update AppDelegate’s
quitApp()/applicationWillTerminate() flow to observe that completion before
calling NSApp.terminate(nil). In ProxyController.swift lines 85-89, preserve the
delayed fallback while ensuring it can execute; in AppDelegate.swift lines
113-120, defer termination until the reported child shutdown, using
applicationShouldTerminate if appropriate.
| serve_thread = threading.Thread(target=server.serve_forever, daemon=True) | ||
| signal.signal(signal.SIGTERM, lambda *_: server.shutdown()) | ||
| try: | ||
| server.serve_forever() | ||
| serve_thread.start() | ||
| serve_thread.join() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Start the server thread before installing the SIGTERM handler.
A SIGTERM between Lines 644 and 646 calls server.shutdown() before serve_forever() starts. shutdown() then waits indefinitely. Start serve_thread before registering the handler.
Proposed fix
serve_thread = threading.Thread(target=server.serve_forever, daemon=True)
-signal.signal(signal.SIGTERM, lambda *_: server.shutdown())
try:
serve_thread.start()
+ signal.signal(signal.SIGTERM, lambda *_: server.shutdown())
serve_thread.join()🤖 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 `@src/opencode_go_proxy/app.py` around lines 643 - 647, Reorder the startup
sequence around serve_thread and server so serve_thread.start() runs before
registering the SIGTERM handler; keep serve_thread.join() afterward and preserve
the existing server.shutdown callback.
| env = dict(os.environ, OPENCODE_GO_API_KEY="test-key") | ||
| proc = subprocess.Popen( | ||
| [sys.executable, "-m", "opencode_go_proxy", "--bind", "127.0.0.1", "--port", "8799"], | ||
| env=env, | ||
| stdout=subprocess.DEVNULL, | ||
| stderr=subprocess.DEVNULL, | ||
| ) | ||
| try: | ||
| deadline = time.monotonic() + 10 | ||
| while time.monotonic() < deadline: | ||
| try: | ||
| with socket.create_connection(("127.0.0.1", 8799), timeout=1): | ||
| break | ||
| except OSError: | ||
| time.sleep(0.2) | ||
| else: | ||
| raise AssertionError("proxy did not start listening on 8799") | ||
|
|
||
| proc.send_signal(signal.SIGTERM) | ||
| proc.wait(timeout=8) | ||
| finally: | ||
| if proc.poll() is None: | ||
| proc.kill() | ||
| proc.wait(timeout=5) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use an isolated port and assert clean shutdown.
Line 432 uses fixed port 8799. Another local process can satisfy the listener probe while this subprocess fails to bind. The test also accepts any exit status and does not verify port release.
Allocate a free test port, assert proc.returncode == 0, and bind that port again after wait().
🧰 Tools
🪛 ast-grep (0.45.0)
[error] 430-435: Command coming from incoming request
Context: subprocess.Popen(
[sys.executable, "-m", "opencode_go_proxy", "--bind", "127.0.0.1", "--port", "8799"],
env=env,
stdout=subprocess.DEVNULL,
stderr=subprocess.DEVNULL,
)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
Mechanical cleanup only, no behavior change: collapse nested with statements, sort imports, yield from, explicit subprocess check flag, and noqa on the two intentional defensive crash catches. 45 tests still pass.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
8 issues found and verified against the latest diff
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="macos/MenuBarApp/Sources/OpenCodeGoMenuBar/AppDelegate.swift">
<violation number="1" location="macos/MenuBarApp/Sources/OpenCodeGoMenuBar/AppDelegate.swift:115">
P1: Quitting can leave the proxy process group running because app termination prevents `ProxyController.stop()`'s five-second SIGKILL fallback from executing. Keep the app alive until that fallback has run (or make `stop` provide a completion) before terminating.</violation>
</file>
<file name="macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift">
<violation number="1" location="macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift:44">
P2: If `logFD` opens successfully but `errFD` open fails, the `guard` returns without closing `logFD` because the `defer { close(logFD); close(errFD) }` is registered only after the guard. The leaked descriptor stays open until the process exits. This is the first thing every Start attempt does, so a repeated half-open state (e.g. err log path unwritable) leaks an fd per attempt. Close each descriptor in the error path before returning.</violation>
<violation number="2" location="macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift:60">
P2: The `uvx --from git+https://github.com/kartikkabadi/opencode-go-proxy` invocation has no `@<ref>` pin, so it always resolves and executes the repository's current default-branch HEAD rather than a reviewed version. This lets unreviewed or rewritten default-branch code run under the process's environment (including the proxy's API key). Consider pinning to a specific reviewed commit SHA or tag.</violation>
<violation number="3" location="macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift:82">
P2: Clicking Stop then immediately clicking Start can spawn a second proxy while the first one is still shutting down. `stop()` resets `childPID = -1` and flips the state to Stopped before the child actually dies (SIGTERM is async, and the 5s SIGKILL fallback runs on a background queue), while `start()` only guards on `childPID < 0` and `!state.isStarting`. So a fast Stop→Start falls through to a new `uvx` spawn that can hit "Address already in use" on 127.0.0.1:8787; the new child then exits and the app shows a brief/perpetual "Starting" state. Consider tracking the shutting-down pid separately and refusing start (or waiting for the old group to be reaped) until the previous process group is gone.</violation>
<violation number="4" location="macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift:134">
P3: `monitorChild` runs `waitpid(pid, &status, 0)` on `DispatchQueue.global().async`, which blocks that shared concurrent-queue thread for the entire lifetime of the proxy child (until it exits). Because the child is a long-running server, this permanently consumes one global-queue worker thread per start, and it un-bounds the pool for the 5s asyncAfter force-kill and any other global work. Little is gained by waiting on a background thread; you could reap asynchronously on main (waitpid is non-blocking when WNOHANG is set, or use a dedicated thread).</violation>
<violation number="5" location="macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift:161">
P1: Starting the proxy can read past the environment array and fail or pass arbitrary memory as environment entries because `envp` lacks its required null terminator. Append `nil` before calling `posix_spawn`.</violation>
</file>
<file name="src/opencode_go_proxy/app.py">
<violation number="1" location="src/opencode_go_proxy/app.py:642">
P2: The SIGTERM fix is sound for steady-state shutdown, but there's a narrow startup race reintroducing the deadlock it's meant to eliminate. The handler (`server.shutdown()`) is registered before the serve thread is started. `shutdown()` blocks on `BaseServer.__is_shut_down`, which is only set after `serve_forever` has run — so if SIGTERM arrives in the tiny window between `signal.signal(...)` (line 644) and the serve thread entering `serve_forever`, the main thread blocks forever inside the handler and never reaches `serve_thread.start()`, leaving the event unset and the process unkillable by SIGTERM. Consider starting the thread first so `serve_forever`/its shutdown event are guaranteed live before exposing the signal handler, or have the handler set a flag that the main loop turns into `server.shutdown()` once the thread is up.</violation>
</file>
<file name="macos/MenuBarApp/README.md">
<violation number="1" location="macos/MenuBarApp/README.md:28">
P3: This bundling snippet copies the binary into `OpenCodeGoMenuBar.app/Contents/MacOS/` but doesn't create that directory structure or an `Info.plist`, so following these steps as written produces an invalid/incomplete app bundle (and the `cp` fails if the directories don't exist). Consider replacing this with the plain release binary path, or provide a real packaging script that creates a complete bundle.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
|
|
||
| @objc private func quitApp() { | ||
| proxy.stop() | ||
| NSApp.terminate(nil) |
There was a problem hiding this comment.
P1: Quitting can leave the proxy process group running because app termination prevents ProxyController.stop()'s five-second SIGKILL fallback from executing. Keep the app alive until that fallback has run (or make stop provide a completion) before terminating.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/MenuBarApp/Sources/OpenCodeGoMenuBar/AppDelegate.swift, line 115:
<comment>Quitting can leave the proxy process group running because app termination prevents `ProxyController.stop()`'s five-second SIGKILL fallback from executing. Keep the app alive until that fallback has run (or make `stop` provide a completion) before terminating.</comment>
<file context>
@@ -0,0 +1,121 @@
+
+ @objc private func quitApp() {
+ proxy.stop()
+ NSApp.terminate(nil)
+ }
+
</file context>
| posix_spawnattr_setflags(&attributes, Int16(POSIX_SPAWN_SETPGROUP)) | ||
|
|
||
| var pid: pid_t = 0 | ||
| let envp = env.map { strdup($0) } |
There was a problem hiding this comment.
P1: Starting the proxy can read past the environment array and fail or pass arbitrary memory as environment entries because envp lacks its required null terminator. Append nil before calling posix_spawn.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift, line 161:
<comment>Starting the proxy can read past the environment array and fail or pass arbitrary memory as environment entries because `envp` lacks its required null terminator. Append `nil` before calling `posix_spawn`.</comment>
<file context>
@@ -0,0 +1,199 @@
+ posix_spawnattr_setflags(&attributes, Int16(POSIX_SPAWN_SETPGROUP))
+
+ var pid: pid_t = 0
+ let envp = env.map { strdup($0) }
+ defer { envp.forEach { free($0) } }
+ let result = posix_spawn(&pid, cArgs[0], &fileActions, &attributes, &cArgs, envp)
</file context>
| let envp = env.map { strdup($0) } | |
| var envp = env.map { strdup($0) } | |
| envp.append(nil) |
| # serve_forever in a background thread: shutdown() from a signal handler | ||
| # running on the main thread would otherwise deadlock (both need the main | ||
| # thread), leaving the process unkillable via SIGTERM. | ||
| serve_thread = threading.Thread(target=server.serve_forever, daemon=True) |
There was a problem hiding this comment.
P2: The SIGTERM fix is sound for steady-state shutdown, but there's a narrow startup race reintroducing the deadlock it's meant to eliminate. The handler (server.shutdown()) is registered before the serve thread is started. shutdown() blocks on BaseServer.__is_shut_down, which is only set after serve_forever has run — so if SIGTERM arrives in the tiny window between signal.signal(...) (line 644) and the serve thread entering serve_forever, the main thread blocks forever inside the handler and never reaches serve_thread.start(), leaving the event unset and the process unkillable by SIGTERM. Consider starting the thread first so serve_forever/its shutdown event are guaranteed live before exposing the signal handler, or have the handler set a flag that the main loop turns into server.shutdown() once the thread is up.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/opencode_go_proxy/app.py, line 643:
<comment>The SIGTERM fix is sound for steady-state shutdown, but there's a narrow startup race reintroducing the deadlock it's meant to eliminate. The handler (`server.shutdown()`) is registered before the serve thread is started. `shutdown()` blocks on `BaseServer.__is_shut_down`, which is only set after `serve_forever` has run — so if SIGTERM arrives in the tiny window between `signal.signal(...)` (line 644) and the serve thread entering `serve_forever`, the main thread blocks forever inside the handler and never reaches `serve_thread.start()`, leaving the event unset and the process unkillable by SIGTERM. Consider starting the thread first so `serve_forever`/its shutdown event are guaranteed live before exposing the signal handler, or have the handler set a flag that the main loop turns into `server.shutdown()` once the thread is up.</comment>
<file context>
@@ -637,9 +637,14 @@ def main(argv: list[str] | None = None) -> None:
+ # serve_forever in a background thread: shutdown() from a signal handler
+ # running on the main thread would otherwise deadlock (both need the main
+ # thread), leaving the process unkillable via SIGTERM.
+ serve_thread = threading.Thread(target=server.serve_forever, daemon=True)
signal.signal(signal.SIGTERM, lambda *_: server.shutdown())
try:
</file context>
| guard logFD >= 0, errFD >= 0 else { | ||
| failStart("Could not open log files under \(logDir.path)") | ||
| return | ||
| } | ||
| defer { | ||
| close(logFD) |
There was a problem hiding this comment.
P2: If logFD opens successfully but errFD open fails, the guard returns without closing logFD because the defer { close(logFD); close(errFD) } is registered only after the guard. The leaked descriptor stays open until the process exits. This is the first thing every Start attempt does, so a repeated half-open state (e.g. err log path unwritable) leaks an fd per attempt. Close each descriptor in the error path before returning.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift, line 44:
<comment>If `logFD` opens successfully but `errFD` open fails, the `guard` returns without closing `logFD` because the `defer { close(logFD); close(errFD) }` is registered only after the guard. The leaked descriptor stays open until the process exits. This is the first thing every Start attempt does, so a repeated half-open state (e.g. err log path unwritable) leaks an fd per attempt. Close each descriptor in the error path before returning.</comment>
<file context>
@@ -0,0 +1,199 @@
+ let errPath = logDir.appendingPathComponent("opencode-go-proxy.err").path
+ let logFD = open(logPath, O_WRONLY | O_CREAT | O_APPEND, 0o644)
+ let errFD = open(errPath, O_WRONLY | O_CREAT | O_APPEND, 0o644)
+ guard logFD >= 0, errFD >= 0 else {
+ failStart("Could not open log files under \(logDir.path)")
+ return
</file context>
| guard logFD >= 0, errFD >= 0 else { | |
| failStart("Could not open log files under \(logDir.path)") | |
| return | |
| } | |
| defer { | |
| close(logFD) | |
| var logFD = open(logPath, O_WRONLY | O_CREAT | O_APPEND, 0o644) | |
| var errFD = open(errPath, O_WRONLY | O_CREAT | O_APPEND, 0o644) | |
| guard logFD >= 0, errFD >= 0 else { | |
| if logFD >= 0 { close(logFD) } | |
| if errFD >= 0 { close(errFD) } | |
| failStart("Could not open log files under \(logDir.path)") | |
| return | |
| } |
| func stop() { | ||
| guard childPID > 0 else { return } | ||
| let pid = childPID | ||
| childPID = -1 |
There was a problem hiding this comment.
P2: Clicking Stop then immediately clicking Start can spawn a second proxy while the first one is still shutting down. stop() resets childPID = -1 and flips the state to Stopped before the child actually dies (SIGTERM is async, and the 5s SIGKILL fallback runs on a background queue), while start() only guards on childPID < 0 and !state.isStarting. So a fast Stop→Start falls through to a new uvx spawn that can hit "Address already in use" on 127.0.0.1:8787; the new child then exits and the app shows a brief/perpetual "Starting" state. Consider tracking the shutting-down pid separately and refusing start (or waiting for the old group to be reaped) until the previous process group is gone.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift, line 82:
<comment>Clicking Stop then immediately clicking Start can spawn a second proxy while the first one is still shutting down. `stop()` resets `childPID = -1` and flips the state to Stopped before the child actually dies (SIGTERM is async, and the 5s SIGKILL fallback runs on a background queue), while `start()` only guards on `childPID < 0` and `!state.isStarting`. So a fast Stop→Start falls through to a new `uvx` spawn that can hit "Address already in use" on 127.0.0.1:8787; the new child then exits and the app shows a brief/perpetual "Starting" state. Consider tracking the shutting-down pid separately and refusing start (or waiting for the old group to be reaped) until the previous process group is gone.</comment>
<file context>
@@ -0,0 +1,199 @@
+ func stop() {
+ guard childPID > 0 else { return }
+ let pid = childPID
+ childPID = -1
+ kill(-pid, SIGTERM)
+ // Grace period, then force-kill the process group if it survives.
</file context>
|
|
||
| let argv: [String] = [ | ||
| uvx, | ||
| "--from", "git+https://github.com/kartikkabadi/opencode-go-proxy", |
There was a problem hiding this comment.
P2: The uvx --from git+https://github.com/kartikkabadi/opencode-go-proxy invocation has no @<ref> pin, so it always resolves and executes the repository's current default-branch HEAD rather than a reviewed version. This lets unreviewed or rewritten default-branch code run under the process's environment (including the proxy's API key). Consider pinning to a specific reviewed commit SHA or tag.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift, line 60:
<comment>The `uvx --from git+https://github.com/kartikkabadi/opencode-go-proxy` invocation has no `@<ref>` pin, so it always resolves and executes the repository's current default-branch HEAD rather than a reviewed version. This lets unreviewed or rewritten default-branch code run under the process's environment (including the proxy's API key). Consider pinning to a specific reviewed commit SHA or tag.</comment>
<file context>
@@ -0,0 +1,199 @@
+
+ let argv: [String] = [
+ uvx,
+ "--from", "git+https://github.com/kartikkabadi/opencode-go-proxy",
+ "opencode-go-proxy",
+ "--bind", "127.0.0.1",
</file context>
| private func monitorChild(_ pid: pid_t) { | ||
| DispatchQueue.global().async { | ||
| var status: Int32 = 0 | ||
| waitpid(pid, &status, 0) |
There was a problem hiding this comment.
P3: monitorChild runs waitpid(pid, &status, 0) on DispatchQueue.global().async, which blocks that shared concurrent-queue thread for the entire lifetime of the proxy child (until it exits). Because the child is a long-running server, this permanently consumes one global-queue worker thread per start, and it un-bounds the pool for the 5s asyncAfter force-kill and any other global work. Little is gained by waiting on a background thread; you could reap asynchronously on main (waitpid is non-blocking when WNOHANG is set, or use a dedicated thread).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/MenuBarApp/Sources/OpenCodeGoMenuBar/ProxyController.swift, line 134:
<comment>`monitorChild` runs `waitpid(pid, &status, 0)` on `DispatchQueue.global().async`, which blocks that shared concurrent-queue thread for the entire lifetime of the proxy child (until it exits). Because the child is a long-running server, this permanently consumes one global-queue worker thread per start, and it un-bounds the pool for the 5s asyncAfter force-kill and any other global work. Little is gained by waiting on a background thread; you could reap asynchronously on main (waitpid is non-blocking when WNOHANG is set, or use a dedicated thread).</comment>
<file context>
@@ -0,0 +1,199 @@
+ private func monitorChild(_ pid: pid_t) {
+ DispatchQueue.global().async {
+ var status: Int32 = 0
+ waitpid(pid, &status, 0)
+ DispatchQueue.main.async {
+ guard self.childPID == pid else { return }
</file context>
|
|
||
| ```bash | ||
| swift build -c release | ||
| cp -R .build/release/OpenCodeGoMenuBar OpenCodeGoMenuBar.app/Contents/MacOS/ |
There was a problem hiding this comment.
P3: This bundling snippet copies the binary into OpenCodeGoMenuBar.app/Contents/MacOS/ but doesn't create that directory structure or an Info.plist, so following these steps as written produces an invalid/incomplete app bundle (and the cp fails if the directories don't exist). Consider replacing this with the plain release binary path, or provide a real packaging script that creates a complete bundle.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At macos/MenuBarApp/README.md, line 28:
<comment>This bundling snippet copies the binary into `OpenCodeGoMenuBar.app/Contents/MacOS/` but doesn't create that directory structure or an `Info.plist`, so following these steps as written produces an invalid/incomplete app bundle (and the `cp` fails if the directories don't exist). Consider replacing this with the plain release binary path, or provide a real packaging script that creates a complete bundle.</comment>
<file context>
@@ -0,0 +1,38 @@
+
+```bash
+swift build -c release
+cp -R .build/release/OpenCodeGoMenuBar OpenCodeGoMenuBar.app/Contents/MacOS/
+```
+
</file context>
kartikkabadi
left a comment
kartikkabadi
left a comment
There was a problem hiding this comment.
Two-pass review of exact head cab3427b8bf2278fe481ea704c72dd3d0f039f67: first the menu-bar lifecycle and process-spawn paths, then shutdown ordering, descriptor cleanup, environment construction, and the Python SIGTERM change.
Blocking findings
P1 — Quit terminates the app before the proxy’s asynchronous stop completes
AppDelegate.quitApp() calls proxy.stop() and immediately calls NSApp.terminate(nil). stop() sends SIGTERM and schedules the SIGKILL fallback asynchronously, so terminating the app can cancel the controller before the fallback runs and leave the proxy process group alive. This contradicts the menu item’s “Quit (stops the child proxy first)” contract and can leave port 8787 occupied after the app exits.
Minimal fix: make stop completion-based (or use applicationShouldTerminate/NSApplication.TerminateReply) and terminate only after the child has been reaped or the SIGKILL fallback has completed. Add an integration test that starts a real child process, invokes the quit path, and asserts the process group is gone before application termination.
P1 — envp passed to posix_spawn is not null-terminated
The environment array is built from environment.map { strdup($0) } and passed to posix_spawn without an explicit trailing nil. posix_spawn requires a null-terminated char *const envp[]; without it, the child can read past the allocated array and receive arbitrary memory as environment entries or fail nondeterministically. Append nil to the pointer array and add a test that exercises the actual spawn path.
Additional correctness findings
P2 — Stop → Start can overlap two proxy lifecycles
stop() resets childPID/state before the child has actually exited, while start() only checks childPID < 0 and !state.isStarting. A fast Stop followed by Start can spawn a second proxy while the first still owns port 8787; the replacement may fail to bind and leave the UI in a misleading starting state. Track a stopping PID/state and refuse or queue start until the old process group is reaped.
P2 — Partial log-file open leaks the descriptor that succeeded
If logFD opens and errFD fails, the guard returns before the deferred close is installed, leaking logFD on every failed start attempt. Close each nonnegative descriptor on the failure path or install cleanup before the validity check.
P2 — The menu-bar app executes mutable default-branch code
The uvx --from git+https://github.com/kartikkabadi/opencode-go-proxy invocation has no commit/tag pin, so Start resolves whatever the repository default branch contains at runtime. That means the app can execute code that was not part of the reviewed commit, with access to the proxy environment and API key. Pin the source to an immutable reviewed ref and test the generated argv.
Validation
The reported Python/Swift validation was not independently rerun in this connector-only pass. The findings above are based on the exact-head patch and the lifecycle invariants of posix_spawn, asynchronous shutdown, and file-descriptor ownership.
Review #139
Automated
Reviewed by ChatGPT - Sol
1 participant
What
Adds a native macOS menu bar app for the opencode-go-proxy bridge, and fixes a real shutdown bug found during the audit.
Menu bar app (
macos/MenuBarApp/, Swift/AppKit, SwiftPM, macOS 13+):/healthpolling every 3s: Running / Starting / Stopped.posix_spawn, own process group, explicit HOME+PATH), open logs, reveal the log file, copy the port.swift build -c releaseinmacos/MenuBarApp.SIGTERM fix (
src/opencode_go_proxy/app.py):serve_forevernow runs on a background thread. Previouslyserver.shutdown()from the main-thread signal handler deadlocked (both need the main thread), leaving the process unkillable via SIGTERM.Verification
/healthOK -> SIGTERM -> clean exit, port released, zero orphans.Notes
macos/MenuBarApp/README.md).uvx --from git+...fetches published HEAD, so the SIGTERM fix only lands once this ships; the app's SIGKILL fallback covers Stop either way.Summary by cubic
Adds a native macOS menu bar app to control the
opencode-go-proxyand fixes a SIGTERM shutdown deadlock so the proxy stops cleanly from launchd and the menu bar app. Also cleans up ruff violations to keep CI green.New Features
macos/MenuBarApp(Swift/AppKit, macOS 13+)./healthchecks every 3s (Running/Starting/Stopped).posix_spawnin its own process group.~/.codex/logs; Open/Reveal logs, Copy Port, Quit stops the child first.uvxwith explicitHOME/PATH; default port 8787.Bug Fixes
serve_foreveron a background thread soserver.shutdown()can’t deadlock on the main thread; regression test asserts SIGTERM stops within 8s.Written for commit cab3427. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation