feat: add --bind, --pid-file, --quiet, --log-file to headless mode#123
feat: add --bind, --pid-file, --quiet, --log-file to headless mode#123
--bind, --pid-file, --quiet, --log-file to headless mode#123Conversation
… mode Add daemon-friendly CLI options to `arf headless`: - --bind <path>: custom IPC socket/pipe path instead of PID-based default - --pid-file <path>: write PID to file, removed on clean shutdown - --quiet: suppress status messages (IPC path, ready, shutdown) on stderr - --log-file <path>: redirect log output to file via env_logger Target::Pipe Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- Use nth(1) instead of any() for headless subcommand detection to avoid false positives when "headless" appears as a non-subcommand argument - Add integration test for --log-file option Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
--bind, --pid-file, --quiet, --log-file to headless mode
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR extends arf headless to be more deployment-friendly by adding CLI options for IPC binding, process management, quieter operation, and log redirection.
Changes:
- Add headless CLI flags:
--bind,--pid-file,--quiet, and--log-file. - Allow IPC server startup to accept an optional custom bind path.
- Add integration tests covering the new headless flags and update the changelog.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| crates/arf-console/src/cli.rs | Adds new headless CLI options (--bind, --pid-file, --quiet, --log-file). |
| crates/arf-console/src/main.rs | Implements headless logging redirection, PID file writing/cleanup, and passes new args into headless mode / IPC startup. |
| crates/arf-console/src/ipc/mod.rs | Extends ipc::start_server API to accept an optional bind path. |
| crates/arf-console/src/ipc/server.rs | Uses the optional bind path when choosing the socket/pipe path. |
| crates/arf-console/src/repl/meta_command.rs | Updates :ipc start to call the new start_server(None) signature. |
| crates/arf-console/tests/headless_tests.rs | Adds tests for --bind, --pid-file, --quiet, and --log-file. |
| CHANGELOG.md | Documents the newly added headless flags. |
Comments suppressed due to low confidence (1)
crates/arf-console/src/ipc/server.rs:56
- On Unix,
start_serverunconditionally callsremove_file(&socket_path)even whensocket_pathcomes from user-provided--bind. This can silently delete an arbitrary existing file at that path (not just a stale socket), which is dangerous. Safer behavior would be: if the path exists, verify it’s a socket (usingsymlink_metadata+FileTypeExt::is_socket) before removing; otherwise return an error indicating the bind path is already in use.
let pid = std::process::id();
let socket_path = match bind {
Some(path) => path.to_string(),
None => get_socket_path(pid),
};
// Remove stale socket file if it exists
#[cfg(unix)]
{
let _ = std::fs::remove_file(&socket_path);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Move logger initialization after Cli::parse() and extract log_file from the parsed Headless command. This avoids the fragile pre-parse detection that could miss global options before the subcommand (e.g. arf --config foo headless). Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
crates/arf-console/src/ipc/server.rs:56
- On Unix this unconditionally calls
remove_file(socket_path)before binding. With the new--bindoption,socket_pathcan be user-supplied, so this can delete an arbitrary existing file if the user passes the wrong path. Consider only auto-removing stale sockets for the default PID-based path, or verify the existing path is a Unix domain socket (and ideally within the sessions dir) before removing; otherwise return an error if the path already exists.
// Remove stale socket file if it exists
#[cfg(unix)]
{
let _ = std::fs::remove_file(&socket_path);
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Stop IPC server and remove partial PID file if write_pid_file fails, preventing stale socket/session from being left behind - Fix test comments to match actual readiness detection mechanism (ipc status probe, not session file polling) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- --bind: only remove existing path if it is a Unix socket; reject non-socket files to prevent accidental deletion - --pid-file: use create_new to fail if file already exists, preventing overwrite of unrelated files or symlink-following attacks - --log-file: restrict permissions to 0600 on Unix - Quiet-mode readiness probe: use actual RPC (ipc eval) instead of ipc status to ensure R is fully initialized before returning Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- --bind: probe existing socket with connect before removing; reject if another process is actively listening - --pid-file: do not remove PID file on write failure since create_new may have failed before creating it (e.g. AlreadyExists) - Quiet-mode timeout: include server stderr and last probe error in timeout message for easier CI debugging Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
- symlink_metadata: only ignore NotFound, propagate other errors (e.g. EACCES) instead of silently falling through to bind - UnixStream::connect: only treat ConnectionRefused as stale socket, propagate other errors (e.g. EACCES) with descriptive message Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- PID file test: poll for non-empty content instead of just file existence to avoid reading between create and write - Log file: set_permissions(0o600) after open to cover existing files - Move trap handler registration after logger init so handlers can log Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Use file.set_permissions() (fchmod) instead of path-based std::fs::set_permissions() to prevent symlink race between open and chmod. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Set Unix socket permissions to 0600 after bind to restrict access regardless of parent directory umask (custom --bind paths) - Use O_NOFOLLOW when opening log files to prevent symlink-based log injection attacks in daemon deployments Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Use libc::fchmod on the listener's raw fd instead of path-based std::fs::set_permissions, consistent with the log file approach. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Add --timeout 500ms to IPC eval probe in tests to prevent hangs - Treat NotFound in addition to ConnectionRefused as stale socket (race between symlink_metadata and connect) Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Temporarily set umask to 0177 before bind() so the socket is created with 0600 permissions, eliminating the brief window where it could be accessible via the parent directory's default umask. The fchmod call is retained as a defensive fallback. Also add a comment explaining why --bind uses FilePath hint on all platforms despite Windows named pipes not being filesystem paths. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
umask is process-global, so mutating it in the IPC server thread can affect concurrent file creations in other threads (e.g. when starting IPC via :ipc start from the REPL). The fd-based fchmod after bind is sufficient to restrict socket permissions. Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary
--bind <path>to specify custom IPC socket path (Unix) or named pipe path (Windows) instead of the default PID-based path--pid-file <path>to write server PID to a file for process management (removed on clean shutdown)--quietto suppress status messages on stderr (IPC path, ready, shutdown)--log-file <path>to redirect log output to a file instead of stderr viaenv_logger::Target::PipeTest plan
test_headless_bind_custom_socket— verifies custom socket path works and IPC functions correctlytest_headless_pid_file— verifies PID file creation, content, and cleanup on shutdowntest_headless_quiet_mode— verifies stderr status messages are suppressedtest_headless_log_file— verifies log file is created and status messages stay on stderrcargo fmtandcargo clippyclean🤖 Generated with Claude Code