feat(coordinator): redesign internals — persistence, IPC, structured logs (#476)

[avihut]

Abstract

The hooks-jobs coordinator (daft __coordinator) backs every background
hook job daft runs and is the source of truth for job status, logs, and
cancel semantics. Until this PR its internals had grown organically:
durability lived in scattered JSON sidecars, the IPC was loose \n-delimited
JSON without versioning, log content went into a single output.log with
no schema, and crashed jobs were invisible until the next manual cleanup.

This PR redesigns those internals around three pillars plus a
hexagonal architecture (store / ports / adapters / domain) that future
DB-backed features land into:

1. Persistence. A per-repo SQLite database at
   <state>/jobs/<repo>/coordinator.db (WAL mode) replaces filesystem
   sidecars (meta.json, repo-policy.json) as the system of record.
   Writes are atomic via SQLite transactions. A reconcile pass on
   coordinator boot flips rows for processes whose PIDs are gone to
   Crashed, surfacing failures the user could previously only spot by
   inspecting state directories. Critically, the new store also fixes
   the cross-process-access limitation of the design's first iteration
   (see "Migration notes" below).
2. Wire protocol. Length-prefixed framed JSON envelopes with a
   stable v / ErrorCode taxonomy replace the old line-delimited
   exchange. A TailLogs { follow, since_seq } streaming variant lets
   the CLI live-tail structured records over the socket. The 8 MiB
   frame ceiling bounds local IPC.
3. Logs. Per-job output.jsonl records ({seq, ts, kind: stdout|stderr|status, ...}) replace the legacy output.log. A
   per-job log.sampling_every_nth knob drops records without breaking
   sequence numbering. daft hooks jobs logs gains --stdout / --stderr / --status / --seq / --since-seq / --since / --follow flags.

Beyond the three pillars, the PR also lands the user-visible payoff for
the new persistence layer: daft hooks jobs ls / logs and the tab-
completion hot path now read from SQLite, so the new Crashed status is
finally visible from the CLI and stays visible while a coordinator is
actively writing (WAL allows concurrent readers).
A CancelMatching { hook, worktree, tag, invocation_prefix, older_than_secs } IPC variant and matching CLI flags collapse what used
to be three special-case helpers into a single predicate.

Three over-grown internal APIs were also tightened with context structs
(JobAdapterContext, HookExecutionContext, extended
JobInvocationContext) — all three lose their
#[allow(clippy::too_many_arguments)].

Closes #476.

Migration notes

The early iteration of this PR used redb for
persistence — pure-Rust, single-file, atomic transactions. During
review the cross-process access pattern surfaced as a hard wall: redb
takes a process-exclusive OS file lock on open, so the CLI could not
read job state while the coordinator was alive. The Tier-1 design
papered over this with a meta.json dual-write that the CLI fell back
to. Removing that workaround revealed the real issue and led to a swap
from redb to SQLite via rusqlite (bundled, WAL mode):

• Cross-process concurrency model matches daft's actual access pattern
  (many readers + one writer across processes on the same host) — no
  workaround needed.
• Fully safe Rust API honors daft's forbid(unsafe_code) policy
  without carveouts. LMDB via heed was evaluated and rejected: its
  Env::open is unsafe fn.
• SQL fits the trust registry's pattern-match queries and the planned
  repo catalog's relational shape — future DB-backed features land in
  the same store layer rather than each one introducing a new DB.

Legacy files left over from earlier builds — coordinator.redb,
repo-policy.json, and per-job meta.json sidecars under
~/.local/state/daft/jobs/*/<inv>/<job>/ — are removed on first
coordinator startup by SqliteJobsStore::for_repo_base_with_wipe,
with a one-line stderr banner per file. The completion / CLI-reader
path uses a non-wiping constructor so the banner cannot leak into the
user's terminal mid-Tab press.

Architectural changes

Persistence: JSON sidecars → SQLite

flowchart LR
    subgraph Before["Before — JSON sidecars"]
        direction TB
        DM1["meta.json (per job)"]
        DR1["repo-policy.json"]
        DL1["output.log"]
    end

    subgraph After["After — SQLite + JSONL"]
        direction TB
        DB[("coordinator.db<br/>(WAL mode)")]
        DB --> T1["jobs"]
        DB --> T2["invocations"]
        DB --> T3["repo_policy"]
        DL2["output.jsonl (per job)"]
    end

    Before -- "legacy file sweep on coordinator startup" --> After

Loading

Writes go through the JobsStorePort trait (typed upsert_job,
upsert_invocation, write_repo_policy with field-merge semantics)
implemented by SqliteJobsStore. The adapter scrubs *_TOKEN /
*_SECRET / *_KEY / *_PASSWORD env vars at the persistence
boundary so no future caller can accidentally leak secrets to disk.

SQLite is the sole source of truth for job metadata. LogStore::clean
now gathers its retention snapshot via the port (lifted into a pure
coordinator/domain/retention.rs FCIS function), so meta.json no
longer flows through any production write or read path. Legacy
sidecars from earlier builds are swept on first coordinator startup.

Store / Ports / Adapters / Domain pattern

flowchart TB
    APP["Application<br/>(commands/hooks/jobs,<br/>IPC dispatch, complete.rs)"]
    DOMAIN["Domain<br/>(pure logic — no SQL, no syscalls)<br/>coordinator/domain/reconcile.rs"]
    PORTS["Ports (trait declarations)<br/>JobsStorePort · Clock · ProcessControl"]
    ADAPTERS["Adapters (impls)<br/>SqliteJobsStore · SystemClock · UnixProcess"]
    STORE["Store (data layer)<br/>connection · pool · migrate · repos · env_scrub"]

    APP --> DOMAIN
    APP --> ADAPTERS
    DOMAIN --> PORTS
    ADAPTERS --> PORTS
    ADAPTERS --> STORE

Loading

Hard rules (recorded in CLAUDE.md):

• Domain code never imports rusqlite, nix, or std::env.
• Store code never imports domain types — returns its own typed row
  structs (JobRow, InvocationRow, RepoPolicyRow).
• Adapters are the only translation layer; cross-cutting concerns
  (env scrub, status enum ↔ TEXT conversion, policy merge) live there.

coordinator/domain/reconcile.rs is the canonical example — pure
logic, six unit tests against fake adapters. Future PRs will lift
more functions from process.rs (cancel, lifecycle) into the
domain layer using the same pattern.

Security baked into the store layer

Every consumer goes through src/store/connection.rs::bring_up, so
the security defaults are non-bypassable:

Concern	Measure
SQL injection	All queries parameterized; no format! allowed in src/store/repos/
File perms	0600 on file, 0700 on parent dir; verified once per process
Path safety	Always derived from daft_state_dir(); canonicalized; prefix-asserted
Schema version	Refuse-open if PRAGMA user_version > CURRENT
App identity	PRAGMA application_id check refuses foreign SQLite files
WAL hygiene	wal_autocheckpoint = 1000
Concurrent writes	Writer pool size 1 + busy_timeout = 5000
Concurrent reads	Reader pool SQLITE_OPEN_READ_ONLY + busy_timeout = 300
Secrets in env	env_scrub::scrub strips *_TOKEN/_SECRET/_KEY/_PASSWORD before persisting JobRow.env
Migration safety	rusqlite_migration-driven; refuses to open user_version > binary's current

Wire protocol: loose JSON → framed envelopes

sequenceDiagram
    participant CLI as daft CLI
    participant Coord as daft __coordinator

    Note over CLI,Coord: Length-prefixed (u32 BE) framed JSON envelopes.<br/>Every frame carries `v` (PROTOCOL_VERSION) and a tagged body.

    CLI->>Coord: RequestEnvelope { v:1, req:"TailLogs",<br/>payload:{job, follow:true, since_seq:42} }
    loop until terminal Status or EOF
        Coord-->>CLI: ResponseEnvelope { v:1, kind:"StreamFrame",<br/>payload: LogRecord {...} }
    end
    Coord-->>CLI: ResponseEnvelope { v:1, kind:"StreamEnd" }

    Note over CLI,Coord: On schema mismatch the coordinator emits<br/>Error { code: SchemaMismatch, message } instead of<br/>silently parse-failing — the typed ErrorCode taxonomy<br/>also covers JobNotFound / NoMatch / KillFailed / Internal.

Loading

Frames cap at 8 MiB so a misbehaving peer can't exhaust memory.
Streaming requests (currently just TailLogs) get many StreamFrame
envelopes terminated by exactly one StreamEnd. Single-response
requests exchange one envelope each way.

Coordinator lifecycle: spawn-self + crash recovery

flowchart TB
    A["Hook fires<br/>(worktree-post-create, ...)"] --> B{"Background jobs<br/>declared?"}
    B -- "no" --> Z["Foreground exec only<br/>(BufferingLogSink<br/>writes JobRow to SQLite)"]
    B -- "yes" --> C["spawn_coordinator()"]

    C --> C1["serialize state → 0600 tempfile"]
    C1 --> C2["current_exe().canonicalize()"]
    C2 --> C3["Command::new(canonical).args(__coordinator, tempfile)<br/>Stdio::null x3"]

    C3 --> D["child: daft __coordinator"]
    D --> D1["read + unlink state tempfile"]
    D1 --> D2["nix::setsid()"]
    D2 --> D3["SqliteJobsStore::for_repo_base(...)<br/>auto-wipe legacy coordinator.redb<br/>+ repo-policy.json"]
    D3 --> D4["coordinator::domain::reconcile_active_jobs()<br/>via UnixProcess + SystemClock adapters<br/>flips dead Running rows → Crashed"]
    D4 --> D5["run_all_with_cancel()<br/>each job in its own process_group(0)<br/>upserts JobRow at start + terminal"]
    D5 --> D6["process::exit"]

Loading

spawn_coordinator is the canonical pattern; fork() is not an
option (libc::fork is unsafe fn, conflicts with
forbid(unsafe_code)). The reconcile pass on boot is what makes the
"my last terminal crashed" case visible: the previous coordinator's
process group is gone, so its Running rows get flipped to Crashed
with finished_at = now, and the next daft hooks jobs ls shows them.

Cancel matching

flowchart LR
    subgraph CLI
        F1["--hook"]
        F2["--worktree"]
        F3["--tag"]
        F4["--inv"]
        F5["--older-than"]
        Fall["--all"]
    end

    F1 & F2 & F3 & F4 & F5 -- "AND" --> CM["CancelMatching {<br/>hook, worktree, tag,<br/>invocation_prefix, older_than_secs<br/>}"]
    Fall -- "no filter required" --> CA["CancelAll"]

    CM --> S["filter_matching_jobs over<br/>SqliteJobsStore::list_active_jobs(repo)"]
    CA --> S

    S --> K["killpg(pgid, SIGTERM)<br/>for each match"]
    K --> R["Cancelled { count, names }"]

Loading

Dependencies

• Add rusqlite = "0.39" with bundled (compiles SQLite from source
  into the binary, ~+700 KB on release builds, +C compiler at build
  time).
• Add rusqlite_migration = "2.5", r2d2 = "0.8", r2d2_sqlite = "0.33".
• Drop redb = "4.1", bincode = "1.3".
• All within the 7-day cooldown policy; no .dep-age-allowlist
  entries needed.

Release binary size went from 9.3 MB (master) to 10 MB on this branch.

Test plan

• cargo test --lib — 1790 unit tests pass (90 added under
  store::*, coordinator::adapters::*,
  coordinator::domain::reconcile::*, plus a regression test for
  the legacy coordinator.redb / repo-policy.json sweep)
• mise run clippy — zero warnings
• mise run fmt:check — clean
• mise run test:manual -- --ci — full YAML scenario sweep
• Pre-push hook (mise run test:unit + build-check) — clean
• Manual smoke: coordinator.db, -wal, -shm files appear in
  per-repo state dir; concurrent reader works while writer alive;
  legacy coordinator.redb / repo-policy.json auto-wiped with
  stderr banner

Tab completion benchmark

Per the original migration plan, daft __complete is on the
documented hot path (fires every Tab keypress) and the SQLite open
adds work the meta.json scan didn't pay. Measured on the daft project
itself (298 invocation directories on disk, ~30 worktrees) with
hyperfine, comparing release binaries:

binary	mean	range
master (meta.json hot path)	122 ms	78 ms – 177 ms
this branch (SQLite hot path)	165 ms	97 ms – 217 ms

Branch is ~43 ms slower (1.35×). The --version-only baseline for
both binaries is ~16 ms, so essentially all of the gap is the
SQLite open + PRAGMA bring-up + first reader checkout. The dominant
cost on either binary is the filesystem invocation scan that powers
the worktree drill-down, which is unchanged. 165 ms total stays
under the typical "feels sluggish" threshold (~200 ms).

The advisor flagged the ≤5 ms budget from the original plan as
speculative — order-of-magnitude is the real signal, and 1.35× is
not a regression that warrants the cache-file mitigation from the
plan's Risks section. If user feedback indicates otherwise, a
follow-up can switch the completion path off the pool onto a single
direct Connection::open_with_flags(... READ_ONLY) and skip the
bootstrap/migration check (we know the schema is current — the
binary was built with it).

Architectural direction

This PR also lands ARCHITECTURE.md at the repo
root, pinning the strategic direction the hexagonal coordinator
proves out:

• Hexagonal at subsystem boundaries. The redb→SQLite cutover here
  was a single-adapter-file replacement; that's the property worth
  preserving as new subsystems land.
• Functional core inside domain modules. reconcile is the
  worked example — pure function over a snapshot, no port callbacks
  during computation. Future domain extractions (cancel,
  lifecycle) follow this shape.
• Vertical slice at the CLI command layer. Already the de facto
  pattern in src/commands/; explicitly not extended into hexagonal
  ceremony.
• Gitoxide-style crate decomposition (long-term). One concern per
  crate, narrow public surface, independent versioning. Not a
  near-term refactor.
• Native agentic IDE spin-out (long-term). The concrete second
  consumer that justifies lifting ports from inside-one-crate to
  at-a-crate-edge. daft-coordinator + daft-hooks + daft-trust
  become reusable libraries with IDE-supplied adapters for storage,
  process control, filesystem.

CLAUDE.md's Architecture section gets a landpost paragraph pointing
at the new doc, so agents and contributors hit the strategic direction
before any operational detail. The follow-ups below are all concrete
steps toward this direction.

Known follow-ups

• The invocations SQL table is forward-looking but not yet
  populated by production code (Tier-1 added the schema). The FK
  from jobs.invocation_id → invocations.invocation_id is
  intentionally deferred to the migration that adds the population
  path; declaring it now would refuse every job insert.
• Lift the remaining business logic in process.rs (cancel,
  lifecycle) into coordinator/domain/ using the
  reconcile_active_jobs pattern. Domain-level unit tests already
  cover the lifted function; the surrounding scaffold can come over
  incrementally.
• Migrate the trust registry from JSON to src/store/ (new
  trust.db in ~/.config/daft/).
• Implement the repo catalog on src/store/ (new repos.db).

🤖 Generated with Claude Code

[avihut]

/claude review

[claude]

Claude finished @avihut's task in 5m 4s —— View job

---

Review: feat(coordinator): redesign internals (#476)

This is a large, well-structured PR. The spawn pattern is correctly followed, canonicalize() on current_exe() is present, unsafe is not reintroduced, and the schema-version sentinel and frame-size limit are good protocol discipline. Test coverage for the store layer is thorough. The following are actionable findings in descending priority.

---

Issues

1. --inv alone silently falls through in cancel — likely bug

src/commands/hooks/jobs.rs:537-557

let has_filter =
    hook.is_some() || worktree.is_some() || tag.is_some() || older_than.is_some();
if has_filter {
    cancel_matching(hook…, inv.as_deref(), …)  // inv passed here
} else if all { … }
else if let Some(j) = job { … }
else {
    anyhow::bail!("cancel requires a job address, --all, or at least one filter …");
}

inv is accepted by the Cancel subcommand and forwarded to cancel_matching, but it is not included in has_filter. A user running daft hooks jobs cancel --inv abc123 hits the final bail! instead of filtering by invocation prefix. Either add || inv.is_some() to has_filter, or document that --inv is only a secondary filter that must accompany another predicate.

Fix this →

---

2. : in job_name can corrupt key comparisons

src/coordinator/store/types.rs:116-118

pub fn job_key(repo_hash: &str, invocation_id: &str, job_name: &str) -> String {
    format!("{repo_hash}:{invocation_id}:{job_name}")
}

rsplitn(3, ':') in JobAddress::parse handles : in worktree names, but the redb key format doesn't escape : in job_name. A hook YAML with a job named build:test produces key {repo}:{inv}:build:test, which parses back as repo=repo, inv=inv, job=build:test and round-trips correctly for exact-match get_job. The range scan in list_jobs_for_repo uses {repo_hash}: as prefix and ; as upper bound — still safe. So this is not exploitable today.

However, invocation_id is a 16-hex string (safe) and repo_hash is a UUID (safe). Only job_name is user-controlled via YAML name:. A comment confirming this invariant ("job names must not contain :") or a validation site at YAML parse time would close this permanently.

---

3. reconcile_active_jobs comment misstates the probe semantics

src/coordinator/process.rs:62-66

// `signal::kill` with `None` is the POSIX existence probe:
// returns Ok(()) if the process group exists (regardless of permission)
let pid = Pid::from_raw(pgid as i32);
matches!(signal::kill(pid, None), Ok(()))

signal::kill(pid, None) sends signal 0 to PID pgid, not to the process group with that PGID. To probe a process group you'd use killpg(pgid, 0) (i.e. Pid::from_raw(-(pgid as i32))). In practice, since PID == PGID for these jobs via process_group(0), the probe lands on the right process. But the comment says "process group exists" when it really means "process group leader exists". A PID-reused process would also satisfy the probe — this is a known best-effort limitation, worth documenting inline rather than leaving the misleading comment.

Fix this →

---

4. send_response double-serde is fragile and silently drops errors

src/coordinator/process.rs:1099-1114

fn send_response(stream: &std::os::unix::net::UnixStream, response: &CoordinatorResponse) {
    let env = ResponseEnvelope {
        v: PROTOCOL_VERSION,
        body: match serde_json::to_value(response).and_then(serde_json::from_value) {
            Ok(b) => b,
            Err(_) => return,  // client hangs waiting for a response
        },
    };

The to_value→from_value round-trip exists to avoid needing CoordinatorResponse: Clone, per the comment. But serde_json::Value already implements Clone, so #[derive(Clone)] on CoordinatorResponse (which only has String, Vec, usize, and Value fields) would work and remove the round-trip entirely. As written, a serialization bug causes a silent return — the client hangs on read_frame indefinitely because no StreamEnd or error frame is sent.

Fix this →

---

5. fork_coordinator alias should be removed, not deferred

src/coordinator/process.rs:1356-1365

/// Backwards-compatible alias for callers still using the old name.
/// Functionally identical to [`spawn_coordinator`]; kept thin so the
/// rename across … can land in a follow-up.
pub fn fork_coordinator(state: CoordinatorState, store: LogStore) -> Result<…> {
    spawn_coordinator(state, store)
}

Both call sites (src/hooks/yaml_executor/mod.rs:405 and src/commands/hooks/jobs.rs:1850) are in this same PR. The alias was intended to let the rename land separately, but the rename didn't happen. This is dead API surface with a misleading name (fork_ implies fork()). Removing the alias and updating both call sites to spawn_coordinator is a one-line change per site.

Fix this →

---

6. follow=true tail loop has no shutdown escape hatch on the coordinator side

src/coordinator/process.rs:1186-1201

if n == 0 {
    if !follow { break; }
    let pos = match file.stream_position() { Ok(p) => p, Err(_) => break };
    std::thread::sleep(std::time::Duration::from_millis(200));
    if file.seek(SeekFrom::Start(pos)).is_err() { break; }
    reader = BufReader::new(&file);
    continue;
}

If the log writer thread panics before emitting a terminal Status::Finished|Signaled|Crashed record, handle_tail_logs will spin at 200ms/cycle forever — the connection read-timeout was cleared for follow=true at line 849, so neither side times out. The client will spin too since its stream_request iterator also cleared its read timeout. A shutdown signal check or a wall-clock deadline (e.g. "bail after N seconds at EOF with no new data after the coordinator PID file is gone") would close this.

---

7. Sampling inconsistency between foreground and background sinks

src/coordinator/process.rs:551 vs src/executor/log_sink.rs:122

// process.rs — background:
!seq.is_multiple_of(n as u64)

// log_sink.rs — foreground:
seq % (n as u64) != 0

Both expressions are semantically equivalent (is_multiple_of stabilised Rust 1.87, MSRV is 1.93). The inconsistency is cosmetic but signals the two paths diverged. Unifying to one form makes it easier to verify both paths sample identically, especially since is_multiple_of is clearer.

---

8. job_count() is pub with #[allow(dead_code)]

src/coordinator/store/mod.rs:201-207

A pub function should not need #[allow(dead_code)]. Either demote it to #[cfg(test)] + pub(crate), or add it to an existing test so the annotation is unnecessary.

---

What's done well

• Spawn pattern is correct throughout — canonicalize() on current_exe() is present and load-bearing; the comment in CLAUDE.md is accurately reflected in the code.
• forbid(unsafe_code) preserved — no unsafe blocks introduced; nix safe wrappers used correctly for setsid() and killpg().
• Schema-version sentinel — forward-compat refusal ("older binaries refuse to write data they don't fully understand") is correct and tested.
• Frame size limit (8 MiB) — bounds local IPC without being absurdly small.
• Protocol version on every envelope — SchemaMismatch error code surfaced instead of silent JSON parse failures.
• Crash-recovery reconciliation — reconcile_marks_dead_running_as_crashed and reconcile_leaves_alive_running_intact tests are solid. Spawning-and-waiting true to get a definitely-dead PID is a clean approach.
• Tempfile at 0600 — coordinator state passed through a tempfile with default 0600 permissions; comment confirms this.
• Fallback when redb lock can't be acquired — load_redb_job_meta_index returning None and falling back to meta.json is correctly documented as a known limitation with a test.
• Hot-path is clean — shell-init, completions, and __* background spawns are all in skip_startup_tasks_for; no new commands on the hot path.
• invocation_id_from_seed hash distribution — the ns+PID hash fixing the prefix-collision bug is clearly motivated and tested.
• TOCTOU in write_repo_policy is benign in practice because the coordinator holds the exclusive redb lock, and is documented.

---

[avihut]

Thanks for the thorough review. Addressed in 782272a + 41c9820 (numbering matches the original review list):

• R1 --inv falls through silently — fixed. inv.is_some() now counts toward has_filter. Extracted cancel_has_filter() with a regression test.
• R2 : in job_name key correctness — documented the invariant inline on job_key: repo_hash and invocation_id are :-free by construction; the key format remains safe via rsplitn(3, ':') on read and ; upper-bound on range scan even when job_name contains :.
• R3 reconcile probe comment — rewrote: signal::kill(pid, None) probes the process-group leader's PID (PID == PGID because jobs are launched with process_group(0)). Added the PID-reuse caveat.
• R4 send_response double-serde / silent error drop — replaced the to_value/from_value round-trip with derive(Clone) on CoordinatorResponse + JobInfo. On serialization failure we now emit a sentinel Error { code: Internal, message } frame so the client's read_frame doesn't hang.
• R5 fork_coordinator alias — removed. Both call sites now reference spawn_coordinator.
• R6 follow-loop shutdown escape — documented the limitation inline (writer panic before terminal Status would spin at 200ms/cycle indefinitely). Closing requires either a wall-clock idle deadline gated on stale-mtime + row status no longer Running, or a writer-liveness channel — tracking as a follow-up. Not load-bearing in practice.
• R7 sampling expression — unified: src/executor/log_sink.rs now uses !seq.is_multiple_of(n) matching process.rs.
• R8 pub job_count() with #[allow(dead_code)] — deleted (no callers anywhere) along with the now-unused ReadableTableMetadata import.

Also unblocked the Windows build that broke on this PR — refactored follow_logs to early-return through the coordinator path on Unix and fall back to the file-read otherwise; the Unix-only tail_logs IPC call now lives in a #[cfg(unix)] helper. Gated anyhow::Context import for cfg(unix) since spawn_coordinator (its sole user) is Unix-only.

The PR description was also expanded with an abstract and mermaid diagrams covering the four architectural shifts (persistence, IPC, lifecycle, cancel).

[avihut]

/claude review

[claude]

Claude finished @avihut's task in 4m 50s —— View job

---

Review: feat(coordinator): redesign internals (#476)

This is a well-executed redesign. The hexagonal layering is clean, the spawn pattern follows CLAUDE.md precisely, forbid(unsafe_code) is preserved throughout, and the previous review's findings are all addressed. The following are new actionable findings.

---

Issues

1. verify_perms is defined but never called from production code

src/store/paths.rs:68–96

The PR description's security table lists "File perms: 0600 on file, 0700 on parent dir; verified once per process." This is only half-true. tighten_perms is called on fresh DB creation (correct — src/store/connection.rs:80), but verify_perms is never invoked from any production path — only from the module's own unit tests. Subsequent Pool::open calls on an existing DB go through bring_up → verify_application_id, but never verify_perms. A replacement attack (symlink a world-readable DB over the original) is not caught on re-open.

The fix is to call paths::verify_perms(db_path)? inside bring_up for non-read-only connections on existing DBs (the is_fresh_db = false branch). Or update the security table to say "set once at creation" rather than "verified once per process."

Fix this →

---

2. wipe_legacy_files prints to stderr on the tab-completion hot path

src/coordinator/adapters/sqlite_store.rs:57–75

complete.rs calls SqliteJobsStore::for_repo_base (lines 822, 1169, 1263) which always calls wipe_legacy_files. If a user has coordinator.redb or repo-policy.json on disk (upgrading from the previous design), the first tab press after the upgrade runs eprintln!("daft: removed legacy …"). CLAUDE.md explicitly calls out: "Stderr output is also problematic — eval only captures stdout … The same applies to the __complete tab-completion helper, which fires on every Tab keypress." The banner would appear mid-typing in the user's terminal.

The wipe is one-time (files are gone after first deletion), but the banner appearing on a tab press is confusing UX. The mitigation is to move the legacy wipe out of for_repo_base into a dedicated first-boot-or-coordinator-startup path (e.g., only call it from run_coordinator, not from every for_repo_base call). A flag file in the repo state dir could gate this.

Fix this →

---

3. format! in src/store/repos/jobs.rs violates the stated invariant

src/store/repos/jobs.rs:144–155

The CLAUDE.md security table records "All queries parameterized; no format! allowed in src/store/repos/." list_by_repo_and_statuses uses format! to build the placeholder list:

let placeholders = (2..=statuses.len() + 1)
    .map(|i| format!("?{i}"))
    .collect::<Vec<_>>()
    .join(", ");
let sql = format!(
    "… WHERE repo_hash = ?1 AND status IN ({placeholders}) …"
);

This is not a SQL injection risk — only the count comes from statuses.len() and the values are bound via params_vec. But it violates the "no format!" invariant that future contributors will rely on as a clear signal that queries are parameterized. The hardcoded callers are always two known statuses ("running" and "cancelling"), so a clean fix is to hardcode the two-placeholder form: WHERE repo_hash = ?1 AND status IN (?2, ?3). If a more general form is genuinely needed later, the rule should be updated in CLAUDE.md first.

Fix this →

---

4. for_repo_base bypasses the path-safety check that for_repo_under provides

src/coordinator/adapters/sqlite_store.rs:41–47

pub fn for_repo_base(base: &Path) -> Result<Self> {
    wipe_legacy_files(base);
    let db_path = base.join(crate::store::paths::COORDINATOR_DB);
    // ↑ no canonicalize(), no starts_with(base_dir) assertion
    let pool = Pool::open(&db_path)…;
}

paths::for_repo_under canonicalizes the parent and asserts it stays under base before returning the DB path — this is the symlink-escape guard (for_repo_under_rejects_symlink_escape test). for_repo_base skips this check. All production callers pass a store.base_dir that is itself already within the state dir, so this doesn't create a practical exploit today. But the layering is inconsistent: the paths module has a guard, and the adapter silently routes around it. The safe fix is to call paths::for_repo_under(base, "") or equivalent, or to document why the guard is intentionally skipped here.

---

5. Socket listener swallows unexpected errors without logging

src/coordinator/process.rs:743–749

Err(ref e) if e.kind() == std::io::ErrorKind::WouldBlock => {
    std::thread::sleep(…);
}
Err(_) => {
    // Unexpected error; break the loop.
    break;   // ← no eprintln!, no log
}

When an unexpected accept() error fires (file-descriptor exhaustion, protocol error), the socket listener exits silently. From that point on, cancel, tail-logs, and list-jobs IPC requests all fail at the client with "connection refused." This is hard to diagnose because no signal in the coordinator's output explains why the socket disappeared mid-flight. A one-line eprintln!("daft coordinator: socket listener error: {e}") before the break would surface this in whatever terminal collects the coordinator's output.

Fix this →

---

Minor observations

• JobMeta.pid is set to the coordinator's own PID on the initial meta.json write (process.rs:468: pid: Some(std::process::id())), but the SQLite JobRow correctly uses child_pid = None for the initial write (via job_row_from_meta(..., child_pid = None)). So SQLite is correct; only the transitional meta.json sidecar carries the misleading coordinator PID. This is self-correcting once the meta.json path is retired in the follow-up.

• stale_running_after_seconds from RepoPolicy is not used by the reconciler — clean_policy.rs:83 has fn stale_running_threshold consuming this field, but reconcile_active_jobs uses only live/dead PID probing. The field is wired up but the reconciler doesn't consult it. No bug (PID probe is sufficient), but the policy field has no effect today.

• Pool::open in complete.rs runs migration validation on every tab press — The 165ms benchmark is acceptable, but migrate::run checks user_version on every Pool::open. Since the schema never changes at runtime, the completion path could eventually use a single Connection::open_with_flags(READ_ONLY) + skip the migration check, as the PR description's risks section notes. Not a blocker now.

• wipe_legacy_files runs on every for_repo_base call, adding two stat() syscalls to the hot path — Once the legacy files are gone, the overhead is two p.exists() checks per completion-path open. Negligible but worth tracking for the eventual cleanup.

---

What's done well

• Previous review findings all addressed — R1 (cancel_has_filter + regression test), R2 (: key safety comment), R3 (reconcile probe comment), R4 (derive(Clone) + error frame fallback), R5 (fork_coordinator alias removed), R7 (sampling unified to is_multiple_of), R8 (job_count dead code removed). Clean response.
• Hexagonal ports/adapters/domain pattern is properly enforced — domain code has no rusqlite/nix/std::env imports; adapters are the only translation layer; the reconcile unit tests use in-memory fakes.
• forbid(unsafe_code) fully preserved — no unsafe blocks, nix safe wrappers for setsid()/killpg().
• canonicalize() on current_exe() is correct and load-bearing — the comment in spawn_coordinator explaining why this is necessary (symlink multicall dispatch) is accurate and important.
• Frame size limit (8 MiB) with tests — write_rejects_oversize_payload and read_rejects_oversize_header both present.
• Env scrub at the adapter boundary — upsert_job scrubs before any DB write; no caller can accidentally skip this. Tests confirm GH_TOKEN doesn't survive the round-trip.
• Schema forward-compat sentinel — run refuses to apply migrations when user_version > current_version(), tested.
• Concurrent access test replaces the redb lock test — concurrent_open_allowed directly demonstrates the property that motivated the redb→SQLite cutover.
• cancel_has_filter extracted with regression test — clean fix for R1 and the function itself is now independently testable.
• ACTIVE_STATUSES constant — shared between the fake store in tests and the adapter, so the reconciler tests are always consistent with the production filter.

---

[avihut]

Thanks for the second-pass review. Most of the findings are legitimate
and are addressed in be764d7d
… 387ed12f.
One Minor observation is a false alarm; tracing below.

Issues (addressed)

• Feature: Shorthand Remote URL Support #1 verify_perms defined but never called from production —
  fixed in f2e7b57c
  (fix(store): verify file perms once per pool open). Pool::open
  now calls paths::verify_perms(db_path)? once after migrations
  finish, both for fresh and pre-existing DBs. Tightened the security
  claim to actually hold. Regression test
  pool_rejects_world_readable_db chmods a freshly-created DB to
  0o644 and asserts the second open fails with PermissionsTooOpen.

• Feature: Root Level .git Dir #2 wipe_legacy_files prints to stderr on the tab-completion hot
  path — fixed in be764d7d
  (refactor(coordinator): move legacy-file wipe off the hot path).
  for_repo_base split into two constructors:

  • for_repo_base — no wipe; used by completion / CLI readers.
  • for_repo_base_with_wipe — wipes legacy files and emits the
    stderr banner; called only from run_coordinator.
    Regression test for_repo_base_does_not_wipe_legacy_files plants
    legacy files and asserts the completion-path constructor leaves
    them alone.

• Install Through brew #3 format! in src/store/repos/jobs.rs violates the
  no-format! invariant — fixed in 46c0921f
  (fix(store): hardcode IN-list placeholders ...). list_by_repo_and_statuses(&[&str])
  → list_by_repo_and_two_statuses(status_a, status_b) with
  hardcoded SQL WHERE repo_hash = ?1 AND status IN (?2, ?3). The
  caller (SqliteJobsStore::list_active_jobs) always passes exactly
  two statuses, so the API shrink is lossless. If we ever need three,
  a sibling method is the right shape — re-introducing dynamic
  placeholders would weaken the CI grep-gate's signal.

• Command Shortcuts #4 for_repo_base bypasses the path-safety check — addressed
  via the same commit (be764d7d)
  by documenting the trust boundary on for_repo_base. The full
  paths::for_repo_under check (canonicalize + prefix-assert
  against state dir) can't be applied uniformly without coupling the
  adapter to daft_state_dir() (which would break every test that
  uses a tempdir). The docstring now records that production callers
  derive base from LogStore::base_dir, which is itself constructed
  via <state_dir>/jobs/<repo_hash> — the chain is upstream-trusted.
  Per the review's own framing, "or to document why the guard is
  intentionally skipped" is a valid resolution; that's what's in tree.

• Shell Completions #5 Socket listener swallows unexpected errors without logging
  — fixed in 658545d2
  (fix(coordinator): surface socket-listener errors before exiting).
  One-line eprintln! before the break in the Err(_) arm.

Follow-up from the previous review (now closed)

The LogStore::clean → meta.json cutover that was previously
flagged as a deferred follow-up landed in
6535ef57
and 387ed12f:

• B1 lifts retention into coordinator/domain/retention.rs as a pure
  FCIS function (10 unit tests, no fake-adapter mocking) and rewrites
  LogStore::clean as the imperative shell that gathers a snapshot
  via JobsStorePort::list_jobs_for_repo + filesystem stat, calls
  retention(input), and applies the returned RetentionDecision.
• B2 drops the three production write_meta calls in
  coordinator/process.rs, the write_meta inside
  LogStore::write_job_record_jsonl, and the meta.json read fallback
  in commands/hooks/jobs.rs (read_job_meta_redb_first →
  lookup_job_meta, Result → Option). meta.json is added to
  the legacy-file sweep so post-upgrade installs get cleaned up the
  same way the redb-era files are.

Minor observations

• Minor Feature: Shorthand Remote URL Support #1 (JobMeta.pid mismatch on initial meta.json write) —
  auto-resolved by B2: that initial write no longer happens.

• Minor Feature: Root Level .git Dir #2 (stale_running_after_seconds claimed unused) — this
  is a false alarm. The field IS consulted by production code. End-
  to-end trace:

  1. src/coordinator/clean_policy.rs:82-85 —
     RepoPolicy::stale_running_after_resolved(&self) -> i64
     returns the field with an 86_400s default.
  2. src/coordinator/log_store.rs::clean (post-B1 shell) /
     src/coordinator/domain/retention.rs::retention (pure core) —
     consumes the resolved value as the stale-Running threshold
     (Duration::try_seconds(...)).
  3. Production callers of clean: src/log_clean.rs:120
     (run_clean_logs, fires on every daft invocation via the
     once-per-24h background sweep) and
     src/commands/hooks/jobs.rs::prune_jobs (the user-facing
     daft hooks jobs prune subcommand).
  4. The stale_running_marked_when_socket_missing and
     stale_running_skipped_when_socket_alive domain tests directly
     exercise the threshold path.

  Recommendation: the bot's index missed the indirection through
  stale_running_after_resolved. No change to make.

• Minor Install Through brew #3 (per-tab-press migration check in Pool::open) —
  acknowledged in the PR body's tab-completion benchmark section.
  165 ms total is acceptable for now; the Connection::open_with_flags(... READ_ONLY) + skip-migration path is a follow-up if real user
  feedback indicates the cost matters.

• Minor Command Shortcuts #4 (extra stat() syscalls in the hot path from
  wipe_legacy_files) — auto-resolved by Feature: Root Level .git Dir #2's fix: the completion
  path no longer calls wipe_legacy_files, so the per-keystroke
  exists() checks are gone.

Diff summary

Commit	Subject
be764d7d	refactor(coordinator): move legacy-file wipe off the hot path
f2e7b57c	fix(store): verify file perms once per pool open
46c0921f	fix(store): hardcode IN-list placeholders in list_by_repo_and_two_statuses
658545d2	fix(coordinator): surface socket-listener errors before exiting
6535ef57	refactor(coordinator): lift retention into FCIS domain + route through SQLite
387ed12f	chore(coordinator): drop meta.json from production paths

mise run test:unit clean (1802 tests); mise run clippy clean;
mise run fmt:check clean. CI on the pushed branch will tell us if
anything regresses against the integration sweep.

[avihut]

/claude review

[claude]

Claude finished @avihut's task in 7m 30s —— View job

---

Review: feat(coordinator): redesign internals (#476) — Pass 3

All previous findings from passes 1 and 2 are correctly addressed. Four new items follow, two moderate and two minor.

---

Issues

1. load_redb_job_meta_index / redb_index are misleading misnomers

src/commands/hooks/jobs.rs:654–671

fn load_redb_job_meta_index(
    repo_hash: &str,
    log_store_base: &Path,
) -> Option<HashMap<(String, String), JobMeta>> {
    let store = SqliteJobsStore::for_repo_base(log_store_base).ok()?;
    let rows = store.list_jobs_for_repo(repo_hash).ok()?;
    …
}

The function opens a SQLite store and maps rows from it — it has nothing to do with redb. The name survives from the pre-cutover dual-write era when it read from redb and fell back to meta.json. Every call site passes the result as redb_index (lines 723, 885, 942, 1249, 1281, 1641, …), which now describes a SQLite-populated HashMap. Future contributors reading this file will look for a redb dependency that no longer exists.

The fix is a rename: load_redb_job_meta_index → load_sqlite_job_meta_index and redb_index → sqlite_index at every site. No logic change.

Fix this →

---

2. The no format! CI grep-gate is documented but does not exist

src/store/repos/mod.rs:6 and src/store/repos/jobs.rs:136–137

// mod.rs:6
//! All SQL is parameterized — no `format!` into queries — and a
//! CI grep-gate enforces that in `src/store/repos/`.

// jobs.rs:136-137
/// Fixed at exactly two statuses so the SQL stays parameterized with
/// hardcoded placeholders — the `no format!` invariant in
/// `src/store/repos/` is enforced by a CI grep-gate, …

No grep-gate exists in any workflow file (.github/workflows/) or mise task. The invariant is currently satisfied — format! genuinely does not appear in production query code in src/store/repos/ — but it is enforced by convention only, not tooling. A future contributor can add format! without any CI signal.

Two clean resolutions:

• Add the gate: a CI step or mise task running ! grep -rPn 'format!' src/store/repos/*.rs | grep -v '^\s*//' would make the claim true.
• Correct the comments: change "enforced by a CI grep-gate" to "enforced by convention" so the claim accurately reflects the current state.

Fix this →

---

3. Two stale "redb" doc comments in process.rs and prune.rs

src/coordinator/process.rs:338,355 and src/core/worktree/prune.rs:835

// process.rs:338
/// Optional dual-write target for the redb-backed `JobRow` lifecycle
//              ^^^^^^^^^^ should be "SQLite-backed"

// process.rs:355
/// Project a `JobMeta` into a redb `JobRow`. …
//                             ^^^^ should be "SQLite"

// prune.rs:835
/// filters its redb-recorded active jobs in one round trip …
//              ^^^^ should be "SQLite-recorded"

Redb is gone from the codebase. These three comments still use the old terminology. No logic impact, but they create false signal for future readers (and tooling like grep redb src/ which was being used to verify the cutover was complete).

---

4. meta.pid = Some(std::process::id()) is a dead assignment

src/coordinator/process.rs:468

let mut meta = JobMeta {
    …
    pid: Some(std::process::id()),  // coordinator's own PID — never used
    …
};

job_row_from_meta ignores meta.pid entirely (it uses the child_pid parameter directly for JobRow.pid and JobRow.pgid). The meta.json write path that previously consumed meta.pid was removed in B2 (chore(coordinator): drop meta.json from production paths). This assignment now has no effect, but it looks meaningful — a reader would reasonably assume the coordinator's PID is being persisted into the initial SQLite row, when in fact None is written.

Fix: either remove the assignment (pid: None) or add a comment explaining it's inert (// not propagated to SQLite — overridden by child_pid in job_row_from_meta).

---

What's done well

• All pass-2 findings correctly addressed. verify_perms is now called from Pool::open (with regression test). wipe_legacy_files is split into for_repo_base (no wipe, safe on the completion hot path) vs. for_repo_base_with_wipe (coordinator-only). format! is actually absent from src/store/repos/ production code. cancel_has_filter includes inv.is_some() with a dedicated regression test. fork_coordinator alias is gone.
• Domain purity holds end-to-end. src/coordinator/domain/reconcile.rs and retention.rs have zero imports of rusqlite, nix, or std::env. Confirmed by grep.
• process_group_alive now uses killpg correctly. nix::sys::signal::killpg(Pid::from_raw(pgid as i32), None) probes the process group — the rewrite from pass-2 fixed the semantic mismatch the first review flagged.
• forbid(unsafe_code) preserved. The unsafe { libc::getpgrp() } in unix_process.rs:48 is inside #[cfg(test)], which the #![cfg_attr(not(test), forbid(unsafe_code))] gate in lib.rs allows.
• Legacy-file sweep regression tests. for_repo_base_with_wipe_removes_legacy_files and for_repo_base_does_not_wipe_legacy_files both exist and cover the expected post-conditions precisely.
• Condition-skipped jobs persist to SQLite. src/hooks/yaml_executor/mod.rs:306–354 writes both the JSONL record and the JobRow for jobs filtered out before execution. The regression commit a45d5a1 adds test coverage.

---