Skip to content

feat: add reconciliation supervisor foundation#240

Merged
munezaclovis merged 6 commits into
mainfrom
feat/reconciliation-supervisor-foundation
May 26, 2026
Merged

feat: add reconciliation supervisor foundation#240
munezaclovis merged 6 commits into
mainfrom
feat/reconciliation-supervisor-foundation

Conversation

@munezaclovis

@munezaclovis munezaclovis commented May 24, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add typed exact reconciliation scopes, drop-safe queue ownership, coalesced job attachment, and debounce primitives for M3
  • add watcher-to-debouncer wiring so linked Project config changes enqueue project-scoped reconciliation jobs
  • harden generic port allocation, process supervision ownership/adoption, readiness deadlines, process cleanup, and atomic file writes

Roadmap

Implements PR 5: PV-030, PV-031, PV-032, PV-033, PV-034, PV-035, PV-036.

Verification

  • cargo fmt --all -- --check
  • cargo nextest run -p daemon --no-capture
  • cargo nextest run -p state --no-capture
  • cargo nextest run --workspace --no-capture
  • cargo clippy --workspace --all-targets --all-features --locked -- -D warnings
  • cargo shear
  • git diff --check

Summary by CodeRabbit

  • New Features

    • Queued, coalescing reconciliation with debouncing, serialized execution and enqueue/queued semantics.
    • Process supervision with lifecycle management, ownership/adoption and TCP/HTTP readiness probes.
    • Project config watcher that enqueues reconciliations on config changes.
    • Persistent port allocator with fallback, reuse and release; secure atomic file helpers.
    • Public API expanded to expose reconciliation and supervisor types.
  • Bug Fixes / Improvements

    • Clearer timeout and missing-process error reporting; readiness errors include last probe details.
  • Tests

    • Expanded integration and unit tests for reconciliation, supervision, readiness, port allocation and file helpers.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 24, 2026

Copy link
Copy Markdown

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a process supervisor with readiness checks, a scoped reconciliation queue + debouncer wired into job handling and server, a SQLite-backed port allocator and secure filesystem helpers, and tests exercising reconciliation, supervision, readiness, and port allocation.

Changes

Process Supervision Infrastructure

Layer / File(s) Summary
Cargo & daemon error variants
crates/daemon/Cargo.toml, crates/daemon/src/error.rs
Workspace deps updated (camino, rustix, time), dev deps adjusted, and DaemonError extended with readiness/timestamp/format variants.
Supervisor types and public API
crates/daemon/src/supervisor.rs
Adds ProcessSpec, OwnedRuntime, AdoptedProcess, and ProcessSupervisor with verify/adopt APIs and runtime-metadata persistence.
Process control, termination, and readiness
crates/daemon/src/supervisor.rs
ManagedProcess stop/escalation, process-group signaling, live-process verification, and wait_for_readiness/wait_for_custom_readiness (TCP/HTTP checks).
Secure filesystem helpers
crates/state/src/fs.rs
write_sensitive_file, open_append_file, read_to_string with atomic writes, temp-file generation, strict perms, and UTF‑8 mapping; includes a unit test for temporary filename behavior.
Supervisor and readiness tests
crates/daemon/tests/supervisor_foundation.rs
TCP/HTTP/custom readiness tests and process supervision integration tests exercising metadata persistence, adoption, forced-persistence failures, and snapshot helpers.

Reconciliation Queue and Job Orchestration

Layer / File(s) Summary
Reconciliation types and queue
crates/daemon/src/reconciliation.rs
ReconciliationScope (system/project/resource) with Display/FromStr, ReconciliationQueue, QueuedReconciliation, RunningReconciliation, and job handle lifecycles.
Debouncer and unit tests
crates/daemon/src/reconciliation.rs
ReconciliationDebouncer batches scopes over a delay and calls a handler per drained scope; unit tests cover coalescing and lifecycle semantics.
Job handler reconciliation flow
crates/daemon/src/jobs.rs
run_job routes reconcile kinds through the queue; adds background/foreground reconciliation runners, streaming/stub helpers, invalid-scope handling, and reconciliation streaming/failure tests.
Server wiring and re-exports
crates/daemon/src/server.rs, crates/daemon/src/lib.rs
serve creates a shared ReconciliationQueue, spawns background reconciliation, clones queue per connection, passes queue into handle_connection/run_job; crate root declares reconciliation module and re-exports reconciliation and supervisor types; runtime builder enables tokio timers.
Watcher and daemon reconciliation tests
crates/daemon/src/watcher.rs, crates/daemon/tests/daemon_foundation.rs
ProjectConfigWatcher polls project config modified times, tolerates unreadable paths, and requests debounced project reconciliations; tests submit reconcile jobs (valid and invalid scopes) and assert streamed stub completion and persisted job records.

Port Allocation System

Layer / File(s) Summary
Port types and constants
crates/state/src/database.rs
PORT_CANDIDATE_LIMIT; public PortRequest, PortOwner, PortAssignment types and internal row mapping.
Port management and DB operations
crates/state/src/database.rs
assign_port (reuse-or-allocate with candidate iteration and availability predicate, upsert via ON CONFLICT), release_port, and assigned_ports with SQL helpers.
Port error, re-exports, and tests
crates/state/src/error.rs, crates/state/src/lib.rs, crates/state/tests/state_foundation.rs
Adds StateError::NoAvailablePort and owner-kind errors; widens state re-exports to include port types; tests validate persistence/reuse/candidate-cap behavior with normalized timestamps.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • prvious/pv#239: Implements earlier daemon job/stub logic that this PR extends into a scope-typed reconciliation queue and broader server wiring.

Poem

🐰 I stitched the queues so scopes hop in line,
Daemons guard processes, keep readiness fine,
Ports find soft burrows in SQLite's small den,
Files written atomic, permissions held then,
I nibble a carrot, and code dreams again.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.45% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main addition: a reconciliation supervisor foundation with typed scopes, queues, and debouncing primitives.
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.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/reconciliation-supervisor-foundation

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 6

🧹 Nitpick comments (1)
crates/state/src/lib.rs (1)

32-33: ⚡ Quick win

Use a top-level import instead of a fully-qualified call in testing::read_to_string.

♻️ Suggested refactor
 pub mod testing {
     use rusqlite::Transaction;

     pub use crate::migrations::Migration;
-    use crate::{Database, PvPaths, StateError};
+    use crate::{Database, PvPaths, StateError, fs};
@@
     pub fn read_to_string(path: &camino::Utf8Path) -> Result<String, StateError> {
-        crate::fs::read_to_string(path)
+        fs::read_to_string(path)
     }

As per coding guidelines, "PREFER top-level imports over local imports or fully qualified names in Rust".

🤖 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 `@crates/state/src/lib.rs` around lines 32 - 33, Replace the fully-qualified
call to crate::fs::read_to_string inside the testing read_to_string function
with a top-level import and direct call: add a use crate::fs::read_to_string; at
the top of the module (or crate root as appropriate) and then call
read_to_string(path) inside the pub fn read_to_string(...) -> Result<String,
StateError> so the function uses the imported symbol instead of the
fully-qualified name.
🤖 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 `@crates/daemon/src/reconciliation.rs`:
- Around line 75-89: The enqueue/QueuedReconciliation API must be made
drop-safe: remove Clone from QueuedReconciliation and return a non-cloneable
guard that automatically releases/cleans up its queue state in Drop (so canceled
or dropped tokens cannot leave queued.front() or active stuck). Specifically,
modify enqueue to produce a guard type (replace current QueuedReconciliation)
which on Drop will (1) acquire inner.state lock, remove itself from queued if
still queued, or if it is active clear active and pop the next queued entry, and
(2) call inner.notify.notify_waiters() after modifying state; also ensure
wait_for_turn/finish become internal to the guard or are folded into an
awaitable method on the guard so explicit finish is not required. Update any
uses of QueuedReconciliation, wait_for_turn, and finish to use the new guard API
and ensure the guard is not Clone to prevent misuse.

In `@crates/daemon/src/supervisor.rs`:
- Around line 83-84: The post-spawn persistence calls (fs::write_sensitive_file
and write_runtime_metadata) can fail and currently return Err while leaving the
spawned child running; modify the start function so that if either write fails
you immediately terminate the spawned process (the Child instance named child)
and await/handle its exit before returning Err. Specifically, after spawning
(where child is created) wrap the fs::write_sensitive_file and
write_runtime_metadata calls in a fallible block and on any Err call
child.kill() (or child.kill().await for tokio::process::Child), then call
child.wait()/await to reap it, log or propagate any kill/wait errors as
appropriate, and finally return the original persistence error.
- Around line 113-119: The call to kill_process_group in stop can fail if the
target process group already exited; change the
kill_process_group(process_group, Signal::TERM).map_err(...) pattern to
explicitly handle "no such process" (ESRCH) as non-fatal: call
kill_process_group(process_group, Signal::TERM), match the Result, and if Err
map to an io::Error and check e.raw_os_error()==Some(libc::ESRCH) (or equivalent
platform-specific code) then ignore it and continue, otherwise return the mapped
error; do the same defensive handling for the later kill_process_group(...,
Signal::KILL) so stop and the child.wait() path remain resilient when the
process/group is already gone.

In `@crates/state/src/database.rs`:
- Around line 248-262: The NoAvailablePort error always reports
PORT_CANDIDATE_LIMIT even when fewer candidates were actually tried; modify the
loop in the function that iterates request.candidates() to count the real number
of candidate checks (increment a local attempts counter each iteration that
isn't skipped by assigned_ports/!is_available) and use that counter when
constructing StateError::NoAvailablePort instead of PORT_CANDIDATE_LIMIT; this
touches the loop that calls request.candidates(), the call to
self.upsert_port(request, candidate), and the StateError::NoAvailablePort
construction so the reported attempts accurately reflect how many distinct
candidates were evaluated.

In `@crates/state/src/fs.rs`:
- Line 154: The current temporary_path uses path.with_extension("tmp") which
collides for different extensions; change the temp derivation to avoid
collisions by appending a unique suffix instead of replacing the extension.
Replace the use of path.with_extension("tmp") (variable temporary_path) with a
construction that keeps the original filename and adds a non-colliding suffix
(for example: path.with_file_name(format!("{}.{}.tmp",
path.file_name().unwrap().to_string_lossy(), std::process::id())) or include a
UUID via uuid::Uuid::new_v4()), or create a temp file atomically in the same
directory using
tempfile::Builder::new().prefix(...).tempfile_in(path.parent().unwrap()) and use
its path; ensure the change is applied where temporary_path is referenced in
crates::state::fs (the variable temporary_path and the code that writes/renames
it).

---

Nitpick comments:
In `@crates/state/src/lib.rs`:
- Around line 32-33: Replace the fully-qualified call to
crate::fs::read_to_string inside the testing read_to_string function with a
top-level import and direct call: add a use crate::fs::read_to_string; at the
top of the module (or crate root as appropriate) and then call
read_to_string(path) inside the pub fn read_to_string(...) -> Result<String,
StateError> so the function uses the imported symbol instead of the
fully-qualified name.
🪄 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: 63da7c0e-6495-4817-af64-ef4ed6573131

📥 Commits

Reviewing files that changed from the base of the PR and between bb06749 and f877fda.

⛔ Files ignored due to path filters (4)
  • Cargo.lock is excluded by !**/*.lock
  • crates/daemon/tests/snapshots/daemon_foundation__valid_reconciliation_scopes_stream_stub_completion.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/supervisor_foundation__supervisor_captures_logs_and_runtime_metadata_then_stops_child.snap is excluded by !**/*.snap
  • crates/state/tests/snapshots/state_foundation__port_allocator_persists_reuses_avoids_collisions_and_releases_assignments.snap is excluded by !**/*.snap
📒 Files selected for processing (14)
  • crates/daemon/Cargo.toml
  • crates/daemon/src/error.rs
  • crates/daemon/src/jobs.rs
  • crates/daemon/src/lib.rs
  • crates/daemon/src/reconciliation.rs
  • crates/daemon/src/server.rs
  • crates/daemon/src/supervisor.rs
  • crates/daemon/tests/daemon_foundation.rs
  • crates/daemon/tests/supervisor_foundation.rs
  • crates/state/src/database.rs
  • crates/state/src/error.rs
  • crates/state/src/fs.rs
  • crates/state/src/lib.rs
  • crates/state/tests/state_foundation.rs

Comment thread crates/daemon/src/reconciliation.rs Outdated
Comment thread crates/daemon/src/supervisor.rs Outdated
Comment thread crates/daemon/src/supervisor.rs
Comment thread crates/state/src/database.rs Outdated
Comment thread crates/state/src/database.rs Outdated
Comment thread crates/state/src/fs.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 `@crates/daemon/src/watcher.rs`:
- Around line 38-41: The loop in run is currently discarding poll_once() errors
(let _result = ...), so change it to explicitly handle the Result from
self.poll_once().await: match or if let Err(e) = self.poll_once().await { ... }
and log/report the error (e.g., via the crate logger or a provided reporter)
instead of ignoring it; optionally decide whether to retry, backoff, or
return/propagate the error to the task owner rather than swallowing it. Ensure
you remove the unused underscore binding and reference the symbols run,
poll_once, and poll_interval when locating the change.

In `@crates/daemon/tests/daemon_foundation.rs`:
- Around line 287-290: The 250ms sleep before the second write in the test can
be too short for coarse mtime filesystems causing the write to appear unchanged;
update the test around tokio::time::sleep(Duration::from_millis(250)).await and
state::fs::write_sensitive_file(...) so it either sleeps long enough (e.g.,
>=1s) or actively polls file metadata (using tokio::fs::metadata and checking
modified_at/mtime) in a loop until the mtime changes before calling
wait_for_job_scope(&paths, "project:project_1").await; ensure you reference the
existing helpers (state::fs::write_sensitive_file and wait_for_job_scope) and
fail the test if a reasonable timeout is reached.

In `@crates/daemon/tests/supervisor_foundation.rs`:
- Line 100: The timing assertion using started_at.elapsed() <
Duration::from_millis(200) is too tight and can flake in CI; update both
occurrences (the assertion at started_at.elapsed() and the similar one at the
other location) to use a more lenient bound—e.g., Duration::from_millis(250) or,
better, the test's configured timeout constant—so replace
Duration::from_millis(200) with the configured timeout (or 250ms) to give
scheduling margin, or alternatively change the check to only assert
timeout/error semantics instead of strict low-latency timing.
🪄 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: 1fca4061-a7eb-49ff-a0f3-6927522345d0

📥 Commits

Reviewing files that changed from the base of the PR and between f877fda and 87de3a0.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (13)
  • crates/daemon/Cargo.toml
  • crates/daemon/src/jobs.rs
  • crates/daemon/src/lib.rs
  • crates/daemon/src/reconciliation.rs
  • crates/daemon/src/server.rs
  • crates/daemon/src/supervisor.rs
  • crates/daemon/src/watcher.rs
  • crates/daemon/tests/daemon_foundation.rs
  • crates/daemon/tests/supervisor_foundation.rs
  • crates/state/src/database.rs
  • crates/state/src/fs.rs
  • crates/state/src/lib.rs
  • crates/state/tests/state_foundation.rs

Comment thread crates/daemon/src/watcher.rs Outdated
Comment thread crates/daemon/tests/daemon_foundation.rs Outdated
Comment thread crates/daemon/tests/supervisor_foundation.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
crates/daemon/tests/daemon_foundation.rs (1)

360-367: ⚡ Quick win

Use top-level imports for Utf8Path and state::fs in write_file_after_modified_time_tick.

The helper currently uses fully-qualified camino::Utf8Path and state::fs::*; importing these at module level keeps the test consistent.

♻️ Proposed refactor
 use anyhow::{Result, anyhow};
+use camino::Utf8Path;
 use camino_tempfile::tempdir;
@@
-use state::{Database, JobRecord, PvPaths};
+use state::{Database, JobRecord, PvPaths, fs};
@@
-async fn write_file_after_modified_time_tick(path: &camino::Utf8Path, content: &str) -> Result<()> {
-    let before = state::fs::modified_at(path)?;
+async fn write_file_after_modified_time_tick(path: &Utf8Path, content: &str) -> Result<()> {
+    let before = fs::modified_at(path)?;
@@
-        state::fs::write_sensitive_file(path, content)?;
+        fs::write_sensitive_file(path, content)?;
@@
-        if state::fs::modified_at(path)? != before {
+        if fs::modified_at(path)? != before {
             return Ok(());
         }
🤖 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 `@crates/daemon/tests/daemon_foundation.rs` around lines 360 - 367, Replace the
inline fully-qualified types and module paths in
write_file_after_modified_time_tick with top-level imports: add a module-level
use for camino::Utf8Path (or Utf8Path as the name used elsewhere) and a use for
state::fs (or use state::fs::* if that matches project style), then update the
function signature and calls to use Utf8Path and
fs::modified_at/fs::write_sensitive_file (or direct function names if using glob
import) instead of camino::Utf8Path and state::fs::... so the helper matches the
test module's import style.
🤖 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 `@crates/daemon/src/watcher.rs`:
- Around line 91-92: Replace fully-qualified state:: references with top-level
imports: add state::{PvPaths, fs} to the imports and update uses so
PvPaths::for_home(...) and fs::write_sensitive_file(...) are called instead of
state::PvPaths and state::fs; specifically adjust the test code that currently
calls state::PvPaths::for_home and state::fs::write_sensitive_file to use
PvPaths and fs to keep module import style consistent.

---

Nitpick comments:
In `@crates/daemon/tests/daemon_foundation.rs`:
- Around line 360-367: Replace the inline fully-qualified types and module paths
in write_file_after_modified_time_tick with top-level imports: add a
module-level use for camino::Utf8Path (or Utf8Path as the name used elsewhere)
and a use for state::fs (or use state::fs::* if that matches project style),
then update the function signature and calls to use Utf8Path and
fs::modified_at/fs::write_sensitive_file (or direct function names if using glob
import) instead of camino::Utf8Path and state::fs::... so the helper matches the
test module's import style.
🪄 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: 2c9df7f5-9b25-44ff-b927-9824ab9ae2bc

📥 Commits

Reviewing files that changed from the base of the PR and between 87de3a0 and 6d17511.

📒 Files selected for processing (4)
  • crates/daemon/src/server.rs
  • crates/daemon/src/watcher.rs
  • crates/daemon/tests/daemon_foundation.rs
  • crates/daemon/tests/supervisor_foundation.rs

Comment thread crates/daemon/src/watcher.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/daemon/src/jobs.rs (1)

63-89: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don’t let the initial response write mask reconciliation completion failures.

If write_line for the accepted response fails, accepted_result? returns before result, so a later complete_stub_reconciliation/DB error is dropped. That can leave the job lifecycle broken while the caller only sees a transport failure.

♻️ Proposed fix
             let result = stream_started_reconciliation_job(
                 paths,
                 transport,
                 stream_is_open,
                 running.job_id(),
                 &scope_text,
             )
             .await;

             running.finish();

-            accepted_result?;
-
-            result
+            result?;
+            accepted_result?;
+            Ok(())
         }
🤖 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 `@crates/daemon/src/jobs.rs` around lines 63 - 89, The transport write result
(accepted_result) is currently propagated before awaiting/returning the
reconciliation result from stream_started_reconciliation_job, which can hide
reconciliation/DB errors; change the control flow so you await and capture
result first, then propagate errors giving precedence to the reconciliation
result: call stream_started_reconciliation_job(...).await and store its Result,
call running.finish(), then handle accepted_result and the reconciliation result
such that if reconciliation (result) is Err it is returned to the caller,
otherwise return accepted_result (or Ok) as appropriate; refer to
write_line/accepted_result, stream_started_reconciliation_job, result, and
running.finish in the change.
🤖 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 `@crates/daemon/src/supervisor.rs`:
- Line 137: The current check uses command_line.contains(spec.command.as_str())
which may match arguments instead of the real executable; update live-process
executable matching in live_process_matches_spec (and the related checks around
metadata.matches usage) to compare the actual process executable path or argv[0]
instead of substring matching: obtain the process executable via /proc/<pid>/exe
(e.g., std::fs::read_link) or parse argv[0] from the proc cmdline and compare it
exactly (or canonicalize paths) against spec.command, replacing the
contains(...) call with this exact/executable-path comparison to avoid false
positives.

In `@crates/daemon/src/watcher.rs`:
- Around line 51-63: The code currently converts every fs::modified_at error
into modified_at: None; instead, only treat explicit "unreadable/invalid path"
errors as tolerable and set WatchedConfig.modified_at = None, and let all other
errors propagate so run() can observe them. Update the Err(_error) arm in the
fs::modified_at match to inspect the io::Error (from fs::modified_at) and match
on error.kind() (e.g., NotFound, InvalidInput, PermissionDenied if desired) and
only insert current_configs.insert(watch.project_id, WatchedConfig { path:
watch.config_path, modified_at: None }) and continue for those kinds; for any
other error kind, return or propagate the error (so run() receives it) rather
than swallowing it.

---

Outside diff comments:
In `@crates/daemon/src/jobs.rs`:
- Around line 63-89: The transport write result (accepted_result) is currently
propagated before awaiting/returning the reconciliation result from
stream_started_reconciliation_job, which can hide reconciliation/DB errors;
change the control flow so you await and capture result first, then propagate
errors giving precedence to the reconciliation result: call
stream_started_reconciliation_job(...).await and store its Result, call
running.finish(), then handle accepted_result and the reconciliation result such
that if reconciliation (result) is Err it is returned to the caller, otherwise
return accepted_result (or Ok) as appropriate; refer to
write_line/accepted_result, stream_started_reconciliation_job, result, and
running.finish in the change.
🪄 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: 72594690-8b63-4b43-9488-8683b6f1cfcd

📥 Commits

Reviewing files that changed from the base of the PR and between 6d17511 and 637af8b.

⛔ Files ignored due to path filters (2)
  • crates/daemon/tests/snapshots/daemon_foundation__invalid_reconciliation_scope_reports_scope_parse_failure.snap is excluded by !**/*.snap
  • crates/daemon/tests/snapshots/supervisor_foundation__supervisor_captures_logs_and_runtime_metadata_then_stops_child.snap is excluded by !**/*.snap
📒 Files selected for processing (8)
  • crates/daemon/src/error.rs
  • crates/daemon/src/jobs.rs
  • crates/daemon/src/lib.rs
  • crates/daemon/src/reconciliation.rs
  • crates/daemon/src/supervisor.rs
  • crates/daemon/src/watcher.rs
  • crates/daemon/tests/daemon_foundation.rs
  • crates/daemon/tests/supervisor_foundation.rs

Comment thread crates/daemon/src/supervisor.rs
Comment thread crates/daemon/src/watcher.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
crates/state/src/database.rs (1)

512-523: ⚡ Quick win

Use a typed component discriminator to remove the impossible fallback path.

validate_component currently relies on string literals and a default arm for a scenario that should be impossible from call sites. Encoding the component as an enum makes this exhaustive at compile time and removes dead fallback handling.

♻️ Proposed refactor
+enum PortOwnerComponent {
+    ProjectId,
+    PhpTrack,
+    Name,
+    Track,
+}
+
 impl PortOwner {
@@
-                self.validate_component("project_id", project_id)?;
-                self.validate_component("php_track", php_track)?;
+                self.validate_component(PortOwnerComponent::ProjectId, project_id)?;
+                self.validate_component(PortOwnerComponent::PhpTrack, php_track)?;
@@
-                self.validate_component("name", name)?;
-                self.validate_component("track", track)?;
+                self.validate_component(PortOwnerComponent::Name, name)?;
+                self.validate_component(PortOwnerComponent::Track, track)?;
@@
-    fn validate_component(&self, component: &'static str, value: &str) -> Result<(), StateError> {
+    fn validate_component(
+        &self,
+        component: PortOwnerComponent,
+        value: &str,
+    ) -> Result<(), StateError> {
         if value.is_empty() {
             return Err(StateError::InvalidPortOwner {
                 owner: self.display_name(),
                 reason: match component {
-                    "project_id" => "project worker port owner project id must not be empty",
-                    "php_track" => "project worker port owner php track must not be empty",
-                    "name" => "resource port owner name must not be empty",
-                    "track" => "resource port owner track must not be empty",
-                    _ => "port owner component must not be empty",
+                    PortOwnerComponent::ProjectId => {
+                        "project worker port owner project id must not be empty"
+                    }
+                    PortOwnerComponent::PhpTrack => {
+                        "project worker port owner php track must not be empty"
+                    }
+                    PortOwnerComponent::Name => "resource port owner name must not be empty",
+                    PortOwnerComponent::Track => "resource port owner track must not be empty",
                 },
             });
         }

As per coding guidelines, "Avoid error handling for impossible scenarios in Rust".

Also applies to: 572-583

🤖 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 `@crates/state/src/database.rs` around lines 512 - 523, The code uses
validate_component(&str) with string literals and a catch-all path; change
validate_component to accept a typed enum (e.g., enum Component { ProjectId,
PhpTrack, Name, Track }) and update all call sites in the PortIdentity match
arms (the Self::Project and Self::Resource branches) to pass the appropriate
Component variant instead of string literals, so validation is exhaustive at
compile time and the impossible fallback can be removed; also update any other
callers of validate_component (the second occurrence noted in the review) to use
the new enum and adjust error messages to reference the Component variants.
🤖 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.

Nitpick comments:
In `@crates/state/src/database.rs`:
- Around line 512-523: The code uses validate_component(&str) with string
literals and a catch-all path; change validate_component to accept a typed enum
(e.g., enum Component { ProjectId, PhpTrack, Name, Track }) and update all call
sites in the PortIdentity match arms (the Self::Project and Self::Resource
branches) to pass the appropriate Component variant instead of string literals,
so validation is exhaustive at compile time and the impossible fallback can be
removed; also update any other callers of validate_component (the second
occurrence noted in the review) to use the new enum and adjust error messages to
reference the Component variants.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: b38edac0-455f-4725-96f6-f3b1b223606b

📥 Commits

Reviewing files that changed from the base of the PR and between 637af8b and b6cd4c9.

⛔ Files ignored due to path filters (1)
  • crates/state/tests/snapshots/state_foundation__port_allocator_persists_reuses_avoids_collisions_and_releases_assignments.snap is excluded by !**/*.snap
📒 Files selected for processing (4)
  • crates/state/src/database.rs
  • crates/state/src/error.rs
  • crates/state/src/sql/001_core_state_schema.sql
  • crates/state/tests/state_foundation.rs

@munezaclovis

Copy link
Copy Markdown
Contributor Author

@codex Review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b6cd4c9f7a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread crates/daemon/src/supervisor.rs Outdated
@munezaclovis
munezaclovis merged commit 29e65f2 into main May 26, 2026
2 checks passed
@munezaclovis
munezaclovis deleted the feat/reconciliation-supervisor-foundation branch May 26, 2026 01:32
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.

1 participant