feat: implement cleanup on daemon exit and enhance file path handling

[aeroxy]

Summary by CodeRabbit

• New Features

  • Daemon IPC files now use user-specific $TMPDIR locations to avoid cross-user conflicts.
  • Added graceful shutdown handling for Unix termination signals and Windows Ctrl-C.
  • kill-daemon now detects and stops older “legacy” daemons when present.

• Bug Fixes

  • Improved best-effort cleanup of daemon socket/PID/lock artifacts on normal exit, panics, and shutdown scenarios.
  • Reduced risk of stale daemon files after failures and lock contention.

• Documentation

  • Updated daemon architecture/IPC details and cleanup behavior.
  • Added “Pattern 16: Headless Chrome (No Login, No Human Approval)” to the Chrome DevTools CLI guide.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: fa06d4ef-30da-4f6a-a830-f1087ab3298a

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Daemon IPC files now use per-user paths, daemon cleanup is lock-guarded and unwind-safe, signal handling enables clean shutdown, and kill-daemon removes legacy Unix daemon files. Documentation adds the updated paths, cleanup behavior, and headless workflow.

Changes

Daemon files and lifecycle

Layer / File(s)	Summary
Per-user daemon paths src/protocol.rs	Socket, address, PID, and lock paths include a user suffix; Unix legacy path helpers preserve compatibility with older daemon files.
Locked cleanup and signal-aware daemon shutdown src/daemon.rs	Startup and cleanup use cross-process locking and CleanupGuard; signal-aware shutdown exits the accept loop while preserving idle timeouts.
Legacy daemon stop and documented workflows src/lib.rs, AGENTS.md, README.md, skill/chrome-devtools/SKILL.md	Unix kill-daemon validates and handles legacy PID/socket files, while documentation describes updated daemon behavior and headless Chrome usage.

Estimated code review effort: 4 (Complex) | ~40 minutes

Possibly related PRs

• aeroxy/chrome-devtools-cli#8: Both modify Unix daemon stop logic in src/lib.rs.
• aeroxy/chrome-devtools-cli#15: Both modify kill-daemon handling and Unix cleanup behavior.

Poem

A bunny found a socket with a uid upon its name,
No shared /tmp burrow could ever be the same.
Signals tap the daemon: “Time to hop away!”
Guards sweep the files at night and brighten up the day.
Legacy paths get tucked in bed—
Thump, thump, clean shutdown ahead!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main changes: daemon exit cleanup and improved daemon file path handling.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Clean up daemon exits and isolate runtime files per user

🐞 Bug fix ✨ Enhancement 🕐 20-40 Minutes

AI Description

• Clean daemon files after signals, errors, panics, idle timeouts, and normal exits.
• Prevent stale daemons from deleting files owned by replacement processes.
• Isolate Unix daemon files by user to avoid shared temporary-directory collisions.

Diagram

sequenceDiagram
    actor C as CLI Client
    participant P as Path Resolver
    participant D as Daemon
    participant L as Accept Loop
    actor O as OS Signal
    participant G as Cleanup Guard
    participant F as Runtime Files
    C->>P: Resolve user path
    D->>P: Resolve user path
    D->>F: Write PID and endpoint
    D->>G: Arm cleanup
    D->>L: Accept requests
    O->>L: Request shutdown
    L-->>D: Exit after request
    D->>G: Drop guard
    G->>F: Verify and remove

Loading

High-Level Assessment

The RAII cleanup guard combined with signal-aware loop termination is the preferred approach. Explicit cleanup at every return would be error-prone, while a process-global exit hook would not integrate as cleanly with Tokio signals or Rust unwinding; PID ownership verification also appropriately protects replacement daemons.

Files changed (2) +108 / -24

Enhancement (1) +16 / -3

protocol.rsNamespace Unix daemon paths by user ID +16/-3

Namespace Unix daemon paths by user ID

• Adds a platform-specific user suffix and applies it consistently to socket, address, and PID filenames. Unix paths now include the effective user ID to prevent collisions in shared temporary directories, while Windows filenames remain unchanged.

src/protocol.rs

Bug fix (1) +92 / -21

daemon.rsAdd signal-aware, ownership-safe daemon cleanup +92/-21

Add signal-aware, ownership-safe daemon cleanup

• Adds Unix and Windows shutdown futures to stop the accept loop cleanly after any in-flight request. Introduces a drop guard that removes socket/address and PID files on normal, error, signal, or panic exits only when the PID file still belongs to the current process.

src/daemon.rs

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 11 rules

1. Stale PID kills process ✓ Resolved 🐞 Bug ≡ Correctness

Description

The legacy sweep sends SIGTERM to any valid PID in the old unsuffixed file without verifying that
the process is a legacy daemon. If a daemon left stale files and its PID was reused, kill-daemon
terminates an unrelated user-owned process.

Code

src/lib.rs[1010]

+                    let ret = unsafe { libc::kill(pid as libc::pid_t, libc::SIGTERM) };

Relevance

●●● Strong

Prior kill-daemon reviews consistently accepted safeguards preventing unsafe PID signaling and
stale-file hazards.

PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The sweep validates only that the legacy PID is nonzero and fits in i32, then signals it directly.
The daemon cleanup documentation confirms that non-catchable termination can leave stale PID files,
allowing the recorded PID eventually to identify another process.

src/lib.rs[993-1017]
src/daemon.rs[90-92]
PR-#8

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The legacy cleanup flow signals a PID based only on stale file contents. PID reuse can therefore cause `kill-daemon` to terminate an unrelated process.

## Issue Context
Numeric range checks prevent process-group signaling but do not establish process identity. Confirm the process through the legacy socket/protocol or validate durable identity information such as executable and process start time before sending SIGTERM; otherwise leave the process untouched and report/remove only safely identified stale files.

## Fix Focus Areas
- src/lib.rs[993-1027]
- src/protocol.rs[89-100]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Cleanup ownership check races ✓ Resolved 🐞 Bug ☼ Reliability

Description

cleanup() checks PID ownership once, then removes the endpoint and PID separately; a replacement
daemon starting between those operations can have its newly bound files deleted. The replacement
remains alive but unreachable, causing subsequent clients to spawn still more daemons.

Code

src/daemon.rs[R38-46]

+    let owns_files = std::fs::read_to_string(pid_path())
+        .ok()
+        .and_then(|s| s.trim().parse::<u32>().ok())
+        == Some(std::process::id());
+    if !owns_files {
+        return;
+    }
+    #[cfg(unix)]
+    let _ = std::fs::remove_file(socket_path());

Relevance

●●● Strong

Team previously accepted daemon file-deletion fixes preventing live daemons becoming unreachable.

PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The ownership decision and deletions are separate filesystem operations. Startup writes the shared
PID before replacing and binding the shared endpoint, while any failed client connection can spawn a
replacement, permitting the interleaving: old daemon reads its PID, replacement writes/binds, then
old daemon removes the replacement's files.

src/daemon.rs[37-49]
src/daemon.rs[123-155]
src/lib.rs[1035-1043]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The cleanup ownership check is not atomic with removing the daemon files. If a replacement daemon writes its PID and binds after the old daemon's check, the old daemon can delete the replacement's endpoint and PID file.

## Issue Context
The cleanup guard expands this race to signal, panic, and early-error exits. Use a process-lifetime exclusive lock or another ownership protocol that prevents startup/rebinding while the previous owner is cleaning up; merely repeating the PID read still leaves a TOCTOU window.

## Fix Focus Areas
- src/daemon.rs[37-49]
- src/daemon.rs[123-155]
- src/lib.rs[1035-1043]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Wrong UID for ownership ✓ Resolved 🐞 Bug ☼ Reliability ⭐ New

Description

The new PID/lock file ownership checks compare metadata uid to libc::getuid() (real UID), but file
creation/ownership is based on the effective UID; when real!=effective (e.g., setuid wrappers or
other credential-changing launchers), the daemon can reject the files it just created and fail to
start or clean up. The same UID choice is also used for the per-user filename suffix, compounding
the mismatch.

Code

src/daemon.rs[R116-123]

+    // SAFETY: getuid() is a pure kernel query with no preconditions; it is
+    // thread-safe and cannot fail.
+    if !md.is_file() || md.uid() != unsafe { libc::getuid() } {
+        anyhow::bail!(
+            "Daemon PID path {} is not a regular file owned by the current user; refusing to write it",
+            path.display()
+        );
+    }

Relevance

●●● Strong

Team has accepted similar Unix PID/lock hardening; fixing real/effective UID mismatch prevents
self-rejection under wrappers.

PR-#8
PR-#13

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

write_pid_file_checked rejects a PID file unless md.uid() equals getuid(), and
read_pid_file_checked applies the same rule; meanwhile the daemon’s per-user temp filenames are
derived from getuid() as well. In real!=effective UID scenarios, file ownership will reflect the
effective UID, so these checks can self-reject.

src/daemon.rs[79-128]
src/lib.rs[511-570]
src/protocol.rs[51-89]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Daemon IPC artifact ownership checks use `libc::getuid()` (real UID) to validate that PID/lock files are owned by the “current user”. On Unix, file ownership is determined by the *effective* UID, so in configurations where real and effective UIDs differ (setuid binaries, credential-transition launchers), the daemon can create a file and then immediately reject it as “not owned by current user”, breaking startup/cleanup.

### Issue Context
This PR introduced stricter hostile-path/ownership validation for PID/lock files and also introduced UID-based filename suffixing. The UID identity used for suffixing and ownership validation should be consistent with the identity used by the OS for file ownership.

### Fix Focus Areas
- src/daemon.rs[79-128]
- src/lib.rs[511-570]
- src/protocol.rs[51-89]

### Suggested fix
- Replace `libc::getuid()` with `libc::geteuid()` in the PID/lock/PID-read ownership checks.
- Make the per-user filename suffix use the same identity (`geteuid`) so the suffix and ownership checks align.
- (Optional) If running with differing real/effective IDs is intentionally unsupported, add an explicit early check (real!=effective) with a clear error message, rather than failing later with “not owned by current user”.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

4. Cleanup races Chrome shutdown ✓ Resolved 🐞 Bug ☼ Reliability

Description

The EXIT trap sends SIGTERM to Chrome and immediately deletes its profile without waiting for the
process to exit. Because termination is asynchronous, Chrome can remain running while its active
profile is removed and can survive after the script exits.

Code

skill/chrome-devtools/SKILL.md[R397-398]

+  kill "$CHROME_PID" 2>/dev/null
+  rm -rf "$PROFILE"

Relevance

●●● Strong

Team accepts deterministic cleanup-order fixes, especially preventing artifact deletion before
process shutdown completes.

PR-#8
PR-#13

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The recipe describes the trap as required cleanup, but the function proceeds directly from kill to
rm -rf without wait, a bounded process-exit check, or escalation.

skill/chrome-devtools/SKILL.md[392-400]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The documented cleanup sends SIGTERM to Chrome but deletes its profile immediately, racing asynchronous browser shutdown and potentially leaving Chrome running.

## Issue Context
The recipe promises cleanup on every exit path. Cleanup should remain bounded rather than waiting indefinitely for a browser that ignores SIGTERM.

## Fix Focus Areas
- skill/chrome-devtools/SKILL.md[392-400]

Wait for `CHROME_PID` to exit before removing `PROFILE`, using a bounded timeout and SIGKILL escalation if graceful termination does not complete.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. read_pid_file_checked lacks context ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

read_pid_file_checked returns std::io::Result and propagates open, metadata, and read failures
with bare ? operators. This loses operation-specific context required for diagnosing which
PID-file step failed.

Code

src/lib.rs[515]

+fn read_pid_file_checked(path: &std::path::Path) -> std::io::Result<String> {

Relevance

●●● Strong

Explicit repository rule applies; contextual I/O error propagation has accepted precedent.

PR-#13

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1983364 requires fallible I/O functions to return anyhow::Result and attach
descriptive context at ? propagation sites. The helper instead returns std::io::Result<String>
and uses bare ? for open, metadata, and read_to_string.

Rule 1983364: Use anyhow::Result with contextual error messages in functions
src/lib.rs[515-535]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`read_pid_file_checked` performs several fallible filesystem operations but returns `std::io::Result<String>` and propagates failures without contextual messages.

## Issue Context
Convert the helper to `anyhow::Result<String>` and add path-aware context to opening the PID file, reading metadata, and reading its contents. Preserve the callers' ability to distinguish `NotFound`, such as by downcasting the error or handling absence separately.

## Fix Focus Areas
- src/lib.rs[515-535]
- src/lib.rs[1037-1043]
- src/lib.rs[1113-1132]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (13)

6. PID truncation remains accepted ✓ Resolved 🐞 Bug ≡ Correctness

Description

read_pid_file_checked reads only the first 64 bytes without detecting additional content, so an
oversized file whose truncated prefix is a valid PID followed by whitespace is accepted.
kill-daemon can consequently signal that prefix despite unvalidated trailing file contents.

Code

src/lib.rs[535]

+    f.take(64).read_to_string(&mut s)?;

Relevance

●●● Strong

Team consistently accepts PID-file validation fixes preventing unsafe signaling from malformed
contents.

PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The function documentation characterizes large PID files as hostile, but take(64) cannot
distinguish an exactly 64-byte file from a longer one; the returned truncated string is subsequently
parsed and passed to libc::kill.

src/lib.rs[506-536]
src/lib.rs[1047-1064]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The PID reader caps memory use but silently truncates oversized files rather than rejecting them, allowing a valid truncated prefix to reach PID parsing.

## Issue Context
The ownership check limits cross-user exploitation, but strict validation remains important because the resulting PID is passed to `kill`.

## Fix Focus Areas
- src/lib.rs[506-536]
- src/lib.rs[1047-1064]

Read up to 65 bytes and reject when a 65th byte exists, or validate the file length before parsing. Add a test covering a valid PID and whitespace in the first 64 bytes followed by extra content.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. Lock deletion breaks serialization ✓ Resolved 🐞 Bug ☼ Reliability

Description

The kill instructions now permit deleting the lock file, but on Unix deleting it during startup or
cleanup allows another process to create and lock a different inode. Startup and cleanup can then
overlap, leaving active daemons unreachable or their PID/socket files corrupted.

Code

README.md[227]

+- **Kill**: `chrome-devtools kill-daemon` (or delete the socket + PID file; the lock file may also be deleted)

Relevance

●●● Strong

Clear documentation contradiction exposes the exact lock race the implementation prevents; team
accepts daemon-file safety fixes.

PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

README.md documents that the lock must remain persistent because deletion reintroduces the race. The
implementation opens that path for both startup and cleanup synchronization, so unlinking it can
split participants across different locked files.

README.md[222-227]
src/protocol.rs[83-89]
src/daemon.rs[73-105]
src/daemon.rs[122-161]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The README permits manually deleting the daemon lock file. On Unix, deleting a lock pathname while a daemon-related process has the old inode open allows another process to create and lock a replacement inode, defeating startup/cleanup serialization.

## Issue Context
The implementation intentionally retains the lock file permanently. Remove the lock-file deletion recommendation, or state that deletion is safe only when no daemon startup, shutdown, or cleanup process can still be active.

## Fix Focus Areas
- README.md[227-227]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. metadata() lacks error context ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

open_lock_file returns std::io::Result and propagates f.metadata() with a bare ?, so
metadata failures receive only the caller's misleading “open daemon lock file” context. This
obscures which lock-file operation failed.

Code

src/daemon.rs[50]

+        let md = f.metadata()?;

Relevance

●●● Strong

Repository precedent favors anyhow results with operation-specific context for filesystem failures.

PR-#13

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1983364 requires non-trivial fallible functions to use anyhow::Result and attach
descriptive context to propagated errors. open_lock_file uses std::io::Result and directly
propagates the metadata call at line 50 without metadata-specific context.

Rule 1983364: Use anyhow::Result with contextual error messages in functions
src/daemon.rs[37-50]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`open_lock_file` propagates lock-file metadata errors without accurate operation-specific context.

## Issue Context
The function performs fallible filesystem operations but returns `std::io::Result`; its caller labels every resulting error as an open failure, including failures from `f.metadata()`.

## Fix Focus Areas
- src/daemon.rs[37-50]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Legacy startup files deleted ✓ Resolved 🐞 Bug ☼ Reliability

Description

The Ok(false) branch considers a missing or refused socket proof that both legacy files are stale,
although daemon startup writes its PID before binding the socket. A concurrent kill-daemon can
therefore delete the PID during that window, leaving the legacy daemon running without a usable PID
record.

Code

src/lib.rs[1079]

+                    (_, Ok(false)) => {

Relevance

●●● Strong

Close kill-daemon precedent requires preserving files whenever daemon liveness remains possible.

PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The probe maps NotFound and ConnectionRefused directly to Ok(false), after which the branch
removes both files without synchronization or a liveness recheck. Daemon startup writes the PID
before removing and binding the socket, creating exactly such an observable interval.

src/lib.rs[521-530]
src/lib.rs[1079-1084]
src/daemon.rs[242-253]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Legacy cleanup deletes the PID and socket immediately after a failed connection, which can race the PID-write-before-bind startup sequence and orphan a running daemon.

## Issue Context
Older daemons do not participate in the new lock protocol. Treat an absent endpoint conservatively when the PID names a live process; retry or preserve the files unless process absence and file ownership establish that they are stale.

## Fix Focus Areas
- src/lib.rs[521-530]
- src/lib.rs[1079-1084]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. Lock file enables squatting ✓ Resolved 🐞 Bug ⛨ Security

Description

Daemon startup now requires opening and blocking on a predictable lock file without validating its
ownership or file type. On shared /tmp, another user can precreate or symlink that path to force
startup failure, or hold its advisory lock to block startup indefinitely.

Code

src/daemon.rs[R48-51]

+    let f = open_lock_file()
+        .with_context(|| format!("Failed to open daemon lock file {}", lock_path().display()))?;
+    f.lock()
+        .with_context(|| format!("Failed to lock daemon lock file {}", lock_path().display()))?;

Relevance

●●● Strong

Security flaw contradicts this PR’s user-isolation intent; daemon-file safety fixes are historically
accepted.

PR-#8
PR-#13

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

lock_daemon_files treats opening and acquiring the lock as mandatory and uses blocking
File::lock. The lock path is deterministically derived from the victim's UID under temp_dir,
which the protocol module identifies as shared /tmp on Linux, and the open path performs no
ownership or file-type validation.

src/daemon.rs[32-52]
src/protocol.rs[51-56]
src/protocol.rs[81-87]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The mandatory startup lock uses an attacker-precreatable path in shared temporary storage and follows normal path resolution. Place the lock in a private per-user runtime directory, or securely open it without following symlinks and validate ownership and file type before taking the lock.

## Issue Context
A precreated non-writable file causes immediate startup failure; a writable attacker-owned file with a held advisory lock causes the blocking `lock()` call to wait indefinitely.

## Fix Focus Areas
- src/daemon.rs[32-52]
- src/protocol.rs[51-56]
- src/protocol.rs[81-87]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. Polling comment restates loop ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The comment Wait for Chrome to publish its debug port merely paraphrases the immediately following
polling loop. It provides no rationale or constraint beyond what the code already expresses.

Code

skill/chrome-devtools/SKILL.md[392]

+# 3. Wait for Chrome to publish its debug port

Relevance

●●● Strong

Explicit comment rule and trivial documentation cleanup favor acceptance.

PR-#14

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1983290 prohibits comments that merely restate associated code behavior. The cited
comment says to wait for DevToolsActivePort, while the next line directly implements that exact
wait.

Rule 1983290: Comments must capture intent or rationale, not restate code behavior
skill/chrome-devtools/SKILL.md[392-393]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The shell comment above the `DevToolsActivePort` polling loop only restates the loop's observable behavior.

## Issue Context
Either remove the redundant comment or replace it with useful rationale, such as why this file is the reliable Chrome-readiness signal or why polling is necessary.

## Fix Focus Areas
- skill/chrome-devtools/SKILL.md[392-393]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

12. probe_legacy_daemon hides I/O errors ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

probe_legacy_daemon performs socket I/O but returns bool, converting connection, write, read,
and timeout failures into an uncontextualized false. This prevents callers from distinguishing an
absent legacy daemon from operational failures.

Code

src/lib.rs[R509-511]

+async fn probe_legacy_daemon() -> bool {
+    let Ok(mut stream) =
+        tokio::net::UnixStream::connect(protocol::legacy_socket_path()).await

Relevance

●●● Strong

Team previously accepted preserving operational errors instead of silently collapsing failures.

PR-#13
PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1983364 requires non-trivial fallible functions to return anyhow::Result and
provide contextual messages. The cited function performs Unix socket connection and protocol I/O but
returns only bool, discarding all underlying errors.

Rule 1983364: Use anyhow::Result with contextual error messages in functions
src/lib.rs[509-525]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`probe_legacy_daemon` performs fallible socket operations but collapses every error into `false` instead of returning contextualized `anyhow::Result` errors.

## Issue Context
Expected absence conditions may still map to `Ok(false)`, but unexpected connection, protocol write/read, and timeout failures should retain descriptive operational context. Update the command handler to propagate or deliberately handle the resulting error.

## Fix Focus Areas
- src/lib.rs[509-525]
- src/lib.rs[1041-1041]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

13. Legacy probe can hang ✓ Resolved 🐞 Bug ☼ Reliability

Description

probe_legacy_daemon applies its two-second timeout only to reading the response, leaving socket
connection and request transmission unbounded. A process controlling the predictable legacy socket
can stall the compatibility probe and prevent kill-daemon from returning.

Code

src/lib.rs[R510-515]

+    let Ok(mut stream) =
+        tokio::net::UnixStream::connect(protocol::legacy_socket_path()).await
+    else {
+        return false;
+    };
+    if protocol::write_msg(&mut stream, b"\"probe\"").await.is_err() {

Relevance

●●● Strong

Unbounded daemon probe I/O matches the team’s accepted timeout and async reliability priorities.

PR-#15
PR-#3

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The function awaits UnixStream::connect and write_msg before entering tokio::time::timeout;
only read_msg receives the two-second bound. The legacy endpoint is a predictable unsuffixed
socket path.

src/lib.rs[509-525]
src/protocol.rs[96-100]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The legacy probe's timeout covers only response reading, while connection establishment and request writing happen outside it. Wrap the complete connect/write/read sequence in a single timeout or apply deadlines to every operation.

## Issue Context
The affected socket has a predictable unsuffixed name and may be controlled by another local process on systems using shared temporary storage.

## Fix Focus Areas
- src/lib.rs[509-525]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

14. lock_daemon_files suppresses errors ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

lock_daemon_files and the related cleanup locking logic convert file creation, opening, or lock
acquisition failures into None, causing daemon startup to proceed without synchronization while
cleanup skips removal. Inaccessible or unsupported locking can therefore hide the cause of
cross-process lock failure, leave stale endpoint/PID files, and restore the startup-cleanup race the
lock is intended to prevent.

Code

src/daemon.rs[R37-44]

+fn lock_daemon_files() -> Option<std::fs::File> {
+    let f = std::fs::OpenOptions::new()
+        .create(true)
+        .write(true)
+        .truncate(false)
+        .open(lock_path())
+        .ok()?;
+    f.lock().ok()?;

Relevance

●●● Strong

Team previously accepted propagating swallowed filesystem errors and distinguishing PID-file I/O
failures.

PR-#13
PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 1983364 requires non-trivial fallible functions to return anyhow::Result and add
operation-specific context, but both lock helpers discard open and lock errors through optional
return values and .ok()?. Startup writes the PID after receiving an optional lock, so it can run
unlocked, while cleanup immediately returns on any try-lock failure without distinguishing genuine
WouldBlock contention from lock-file I/O or unsupported-lock errors, leaving daemon files behind.

Rule 1983364: Use anyhow::Result with contextual error messages in functions
src/daemon.rs[37-44]
src/daemon.rs[37-59]
src/daemon.rs[70-87]
src/daemon.rs[175-180]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Daemon startup silently proceeds when its correctness lock cannot be created, opened, or acquired, while cleanup silently skips file removal on equivalent failures. This suppresses useful error causes, defeats synchronized ownership changes and exit cleanup, and can leave stale endpoint/PID files.

## Issue Context
Return detailed lock failures as `anyhow::Result` with descriptive, operation-specific context, including the lock path where appropriate, rather than collapsing errors into `Option::None`. Update startup to fail explicitly when synchronization cannot be established, and make cleanup distinguish genuine `WouldBlock` contention from lock-file I/O or unsupported-lock errors so each failure is reported and handled deliberately.

## Fix Focus Areas
- src/daemon.rs[37-59]
- src/daemon.rs[70-75]
- src/daemon.rs[175-180]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

15. Extract legacy PID parsing ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The kill-daemon handler performs a multi-step pure conversion from PID-file text to a validated
i32 directly inside the command body. Keeping this logic inline duplicates the existing PID guards
and makes validation harder to test and reuse consistently.

Code

src/lib.rs[R1003-1008]

+                let legacy_pid = pid_str
+                    .trim()
+                    .parse::<u32>()
+                    .ok()
+                    .filter(|&p| p != 0)
+                    .and_then(|p| i32::try_from(p).ok());

Relevance

●●● Strong

Small deterministic extraction matches the repository’s accepted parsing and command-handler cleanup
patterns.

PR-#14
PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Compliance rule 1983333 requires pure conversion logic in command handlers to be delegated to
standalone functions. These lines directly reshape a PID string into a filtered Option<i32>
through a multi-line parsing and validation chain inside run.

Rule 1983333: Extract pure conversion/formatting logic into standalone functions
src/lib.rs[1003-1008]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Legacy PID parsing and validation are implemented inline within the `kill-daemon` command handler.

## Issue Context
Move the trim, parse, nonzero validation, and `i32` conversion into a standalone pure function that accepts the PID text and returns the validated value or an appropriate result. Reuse it from the command handler and keep I/O outside the helper.

## Fix Focus Areas
- src/lib.rs[1003-1008]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

16. shutdown_signal comment restates behavior ✓ Resolved 📘 Rule violation ⚙ Maintainability

Description

The doc comment only paraphrases the function name and observable behavior without explaining why
the abstraction exists or documenting a non-obvious constraint. This violates the requirement that
comments capture intent or rationale.

Code

src/daemon.rs[91]

+/// Resolves when the daemon should shut down due to a signal.

Relevance

●●● Strong

Trivial comment cleanup aligns with repository precedent for correcting low-value documentation.

PR-#10

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

PR Compliance ID 1983290 requires comments to add intent or rationale rather than restating code
behavior. The comment Resolves when the daemon should shut down due to a signal is trivially
derivable from the shutdown_signal function name and body.

Rule 1983290: Comments must capture intent or rationale, not restate code behavior
src/daemon.rs[91-91]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The `shutdown_signal` doc comment merely restates what the function does and provides no non-obvious intent or rationale.

## Issue Context
Revise the comment to document a meaningful design reason or platform-related constraint, or remove it if the function is already self-explanatory.

## Fix Focus Areas
- src/daemon.rs[91-91]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

17. Lock timeout exceeds deadline ✓ Resolved 🐞 Bug ☼ Reliability

Description

The daemon always waits up to two seconds for its file lock even when DAEMON_WAIT_TIMEOUT_SECS
configures the client to give up sooner. Under contention, the client can begin direct execution
while its spawned daemon later acquires the lock and becomes available, allowing overlapping direct
and daemon sessions.

Code

src/daemon.rs[86]

+const LOCK_WAIT_TIMEOUT: Duration = Duration::from_secs(2);

Relevance

●● Moderate

Plausible concurrency race, but no close timeout-configuration precedent establishes likely team
behavior.

PR-#8
PR-#15

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The lock timeout is fixed at two seconds and its comment reasons only about the five-second default,
while the client accepts any u64 timeout and falls back directly without stopping the spawned
daemon.

src/daemon.rs[79-86]
src/client.rs[23-29]
src/lib.rs[1253-1267]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The fixed daemon lock timeout is only shorter than the default client timeout, not every value accepted through `DAEMON_WAIT_TIMEOUT_SECS`.

## Issue Context
When the client deadline expires, the child daemon is not cancelled; the caller falls back to direct execution while the daemon may continue starting.

## Fix Focus Areas
- src/daemon.rs[79-86]
- src/client.rs[23-29]
- src/lib.rs[1253-1267]

Derive or constrain the lock deadline so it is always shorter than the effective client deadline, or terminate the spawned daemon before direct fallback.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

18. Legacy daemon becomes unreachable ✓ Resolved 🐞 Bug ≡ Correctness

Description

After an upgrade, a daemon launched by the previous binary remains on the unsuffixed paths, but the
new client and kill-daemon inspect only UID-suffixed paths. The new client therefore spawns a
duplicate daemon and cannot stop the legacy process through the CLI.

Code

src/protocol.rs[67]

+    std::env::temp_dir().join(format!("chrome-devtools-daemon{}.sock", user_suffix()))

Relevance

●● Moderate

Compatibility concern is plausible, but migration behavior is semantic and lacks a close precedent.

PR-#8

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The protocol exposes only the new UID-suffixed paths, all connection attempts use those helpers,
failed connection attempts trigger daemon spawning, and kill-daemon reads only the new PID helper.
The removed diff lines show that prior binaries continue using the old unsuffixed names.

src/protocol.rs[51-78]
src/client.rs[12-20]
src/lib.rs[917-925]
src/lib.rs[1035-1043]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Changing Unix daemon filenames without a compatibility path makes an already-running daemon from the previous version undiscoverable and unkillable by the upgraded CLI.

## Issue Context
This occurs during upgrades or mixed-version operation. Before spawning, safely probe or migrate the legacy endpoint/PID files, or provide an explicit upgrade cleanup mechanism while preserving the per-user isolation goal.

## Fix Focus Areas
- src/protocol.rs[51-78]
- src/client.rs[12-20]
- src/lib.rs[917-925]
- src/lib.rs[1035-1043]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

19. Daemon path documentation stale ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The implementation now creates UID-suffixed Unix files, while the README still documents the
unsuffixed paths and recommends deleting them manually. Following that recovery instruction leaves
the actual daemon files untouched.

Code

src/protocol.rs[67]

+    std::env::temp_dir().join(format!("chrome-devtools-daemon{}.sock", user_suffix()))

Relevance

●●● Strong

Team consistently accepts documentation updates when examples or instructions diverge from behavior.

PR-#5
PR-#14

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The path helpers create suffixed socket and PID filenames, but the user-facing daemon details list
the old exact filenames and tell users to delete them; the contributor architecture documentation
also retains the old socket name.

src/protocol.rs[64-78]
README.md[216-223]
AGENTS.md[63-67]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
User and contributor documentation still names the old unsuffixed daemon paths after the implementation changed them to include the Unix UID.

## Issue Context
Update the platform-specific examples and manual cleanup instructions to describe the UID suffix and actual canonical paths.

## Fix Focus Areas
- README.md[216-223]
- AGENTS.md[63-67]
- src/protocol.rs[64-78]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[aeroxy]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[aeroxy]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 421eca5

[coderabbitai]

Actionable comments posted: 3

🤖 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 `@README.md`:
- Around line 218-221: Update README.md lines 218-221 to separate Unix and
Windows daemon transports and cleanup guarantees: document the UID-suffixed Unix
socket and signal handling, the Windows loopback TCP listener with the
unsuffixed address file, and best-effort Ctrl-C cleanup. Qualify the Unix-socket
architecture diagram at README.md line 44 or add the Windows transport. Qualify
the UID-suffixed socket and signal-cleanup statements in AGENTS.md lines 65-68
as Unix-specific.

In `@src/daemon.rs`:
- Around line 32-45: Update lock_daemon_files to return
anyhow::Result<std::fs::File> and propagate descriptive errors from opening and
locking the daemon lifecycle lock instead of converting failures to None. Make
daemon startup handle this result and fail with the reported context, while
preserving best-effort unlocked behavior only in the cleanup path that handles
lock contention.

In `@src/lib.rs`:
- Around line 999-1017: Update the legacy PID handling in the kill-daemon flow
around legacy_pid_path so it does not call libc::kill based solely on the parsed
PID. Verify that the PID belongs to this daemon using an available
daemon-specific identity check before sending SIGTERM; when identity cannot be
confirmed, avoid signaling and only remove files when staleness is safely
established, otherwise leave them for manual intervention.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 1e8d53b9-443c-4b8f-8692-c7a1d3cba527

📥 Commits

Reviewing files that changed from the base of the PR and between 8297c24 and 421eca5.

📒 Files selected for processing (5)

• AGENTS.md
• README.md
• src/daemon.rs
• src/lib.rs
• src/protocol.rs

[aeroxy]

/agentic_review

[aeroxy]

@coderabbitai full review

[coderabbitai]

✅ Action performed

Full review finished.

[qodo-code-review]

Code review by qodo was updated up to the latest commit 42f60b9

[aeroxy]

/agentic_review

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (3)

README.md (1)

217-226: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Lock file not mentioned in daemon file inventory.

This cohort introduces a persistent lock_path() file (never removed, per its doc comment in protocol.rs) used to serialize startup/cleanup, but "Daemon details" and the manual-cleanup note ("delete the socket + PID file") don't mention it. A user manually cleaning up daemon artifacts wouldn't know it exists.

🤖 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 `@README.md` around lines 217 - 226, Update the “Daemon details” file inventory
and manual cleanup guidance to include the persistent lock file created by
lock_path(). State its platform-specific location if applicable, note that it is
intentionally not removed automatically, and include it in the artifacts users
should delete during manual cleanup.

src/lib.rs (2)

488-500: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add unit tests for parse_pid_file_contents.

This is a pure conversion function (trim/parse/validate) with no accompanying tests — worth covering edge cases ("0", negative-after-cast values, overflow beyond i32, non-numeric, whitespace-only).

As per coding guidelines, src/**/*.rs: "Extract pure conversion and formatting logic into testable functions" and "Place tests in #[cfg(test)] mod tests within the same source file."

✅ Example test module

#[cfg(test)]
mod tests {
    use super::*;

    #[test]
    fn rejects_zero() {
        assert_eq!(parse_pid_file_contents("0"), None);
    }

    #[test]
    fn rejects_overflow() {
        assert_eq!(parse_pid_file_contents("4294967295"), None);
    }

    #[test]
    fn accepts_valid_pid() {
        assert_eq!(parse_pid_file_contents(" 1234 \n"), Some(1234));
    }

    #[test]
    fn rejects_garbage() {
        assert_eq!(parse_pid_file_contents("abc"), None);
    }
}

🤖 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/lib.rs` around lines 488 - 500, Add a #[cfg(test)] mod tests in
src/lib.rs using super::* to cover parse_pid_file_contents: reject zero,
negative-after-cast inputs, values exceeding i32, non-numeric and
whitespace-only strings, and accept trimmed valid PIDs. Keep the tests focused
on this pure conversion function.

Source: Coding guidelines

---

957-991: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider reusing parse_pid_file_contents here instead of duplicating pid-validation logic.

This block re-implements the same "reject 0 / must fit i32" checks that parse_pid_file_contents (lines 494-500) now centralizes for the legacy path. Consolidating avoids two independently-maintained copies of the same safety check, though you'd want to keep this path's more descriptive per-failure error messages.

🤖 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/lib.rs` around lines 957 - 991, The PID-reading flow around the Unix
signal handling should reuse parse_pid_file_contents for the shared PID 0 and
i32-range validation instead of duplicating those checks. Preserve this path’s
descriptive error context by mapping or augmenting parser errors with pid_path
details, then continue using the validated PID for libc::kill.

🤖 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 `@src/daemon.rs`:
- Line 1: Declare the minimum supported Rust version/toolchain for the file_lock
crate at the project configuration level, setting it to Rust 1.89 or newer to
match the std::fs::File::lock/try_lock and std::fs::TryLockError APIs used by
the daemon lock path. Update the appropriate Cargo.toml or toolchain
configuration rather than changing the daemon implementation.

In `@src/lib.rs`:
- Around line 1041-1057: Update the legacy daemon branch around
probe_legacy_daemon and libc::kill so failures other than ESRCH are surfaced,
especially EPERM, with a diagnostic indicating the daemon may still be running
and could not be signaled. Preserve cleanup for successful termination or ESRCH,
and keep the existing stopped-daemon message unchanged.

---

Nitpick comments:
In `@README.md`:
- Around line 217-226: Update the “Daemon details” file inventory and manual
cleanup guidance to include the persistent lock file created by lock_path().
State its platform-specific location if applicable, note that it is
intentionally not removed automatically, and include it in the artifacts users
should delete during manual cleanup.

In `@src/lib.rs`:
- Around line 488-500: Add a #[cfg(test)] mod tests in src/lib.rs using super::*
to cover parse_pid_file_contents: reject zero, negative-after-cast inputs,
values exceeding i32, non-numeric and whitespace-only strings, and accept
trimmed valid PIDs. Keep the tests focused on this pure conversion function.
- Around line 957-991: The PID-reading flow around the Unix signal handling
should reuse parse_pid_file_contents for the shared PID 0 and i32-range
validation instead of duplicating those checks. Preserve this path’s descriptive
error context by mapping or augmenting parser errors with pid_path details, then
continue using the validated PID for libc::kill.

🪄 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: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 956a0e72-6461-455d-868f-90ab242230b6

📥 Commits

Reviewing files that changed from the base of the PR and between 8297c24 and 42f60b9.

📒 Files selected for processing (6)

• AGENTS.md
• README.md
• skill/chrome-devtools/SKILL.md
• src/daemon.rs
• src/lib.rs
• src/protocol.rs

[qodo-code-review]

Code review by qodo was updated up to the latest commit 517354b

[aeroxy]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 79270e2

[aeroxy]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 5e8ed26

[aeroxy]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 420f17b

[aeroxy]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 48dba36

[aeroxy]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 7f8e85a

[aeroxy]

/agentic_review

[qodo-code-review]

Code review by qodo was updated up to the latest commit 93adb70