Skip to content

feat: add --bind, --pid-file, --quiet, --log-file to headless mode#123

Merged
eitsupi merged 17 commits intomainfrom
feat/headless-daemon-options
Mar 20, 2026
Merged

feat: add --bind, --pid-file, --quiet, --log-file to headless mode#123
eitsupi merged 17 commits intomainfrom
feat/headless-daemon-options

Conversation

@eitsupi
Copy link
Copy Markdown
Owner

@eitsupi eitsupi commented Mar 20, 2026

Summary

  • Add --bind <path> to specify custom IPC socket path (Unix) or named pipe path (Windows) instead of the default PID-based path
  • Add --pid-file <path> to write server PID to a file for process management (removed on clean shutdown)
  • Add --quiet to suppress status messages on stderr (IPC path, ready, shutdown)
  • Add --log-file <path> to redirect log output to a file instead of stderr via env_logger::Target::Pipe

Test plan

  • test_headless_bind_custom_socket — verifies custom socket path works and IPC functions correctly
  • test_headless_pid_file — verifies PID file creation, content, and cleanup on shutdown
  • test_headless_quiet_mode — verifies stderr status messages are suppressed
  • test_headless_log_file — verifies log file is created and status messages stay on stderr
  • All 21 headless integration tests pass
  • cargo fmt and cargo clippy clean

🤖 Generated with Claude Code

eitsupi and others added 2 commits March 20, 2026 07:07
… 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]>
@eitsupi eitsupi changed the title feat: add --bind, --pid-file, --quiet, --log-file to headless mode feat: add --bind, --pid-file, --quiet, --log-file to headless mode Mar 20, 2026
@eitsupi eitsupi requested a review from Copilot March 20, 2026 07:21
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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_server unconditionally calls remove_file(&socket_path) even when socket_path comes 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 (using symlink_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]>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 --bind option, socket_path can 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]>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

eitsupi and others added 2 commits March 20, 2026 08:27
- --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]>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

eitsupi and others added 2 commits March 20, 2026 08:38
- 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]>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

eitsupi and others added 2 commits March 20, 2026 09:07
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]>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

eitsupi and others added 2 commits March 20, 2026 09:19
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]>
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@eitsupi eitsupi merged commit 6b78737 into main Mar 20, 2026
14 checks passed
@eitsupi eitsupi deleted the feat/headless-daemon-options branch March 20, 2026 09:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants