feat(config): add dir and env fields for daemons#227
Conversation
…nostics The `pitchfork status` command was silently succeeding (exit 0) when a daemon wasn't found, making it impossible for callers to distinguish "not found" from "found with status". Now it returns a non-zero exit code with an error message. Also improves the `wait_for_status` test helper to be more resilient under CI load: - Increased timeout from 5s to 10s - Added direct state file reading as fallback (bypasses CLI) - On failure, dumps full diagnostics: stderr, stdout, exit code, and raw state file contents for easier debugging of flaky CI failures Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR adds working directory and environment variable configuration for daemons in Pitchfork. The changes support custom working directories (via dir field) and environment variables (via env field) for daemon processes, addressing feature requests #226 and #211.
Changes:
- Added
dirfield to specify custom working directories (relative topitchfork.tomlor absolute paths) - Added
envfield to pass environment variables as key-value pairs to daemon processes - Implemented
resolve_daemon_dir()helper function to resolve directory paths consistently across the codebase
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
src/pitchfork_toml.rs |
Added dir and env fields to PitchforkTomlDaemon struct with proper serialization |
src/daemon.rs |
Added env field to Daemon and RunOptions structs |
src/supervisor/state.rs |
Updated UpsertDaemonOpts to include env field and propagate it |
src/supervisor/lifecycle.rs |
Applied custom environment variables when spawning daemon processes |
src/supervisor/retry.rs |
Ensured env is passed when retrying failed daemons |
src/supervisor/watchers.rs |
Applied dir and env for cron and file-watch triggered restarts |
src/supervisor/autostop.rs |
Applied dir and env for boot-start daemons |
src/ipc/batch.rs |
Added resolve_daemon_dir() helper and updated daemon run options |
src/cli/status.rs |
Changed daemon-not-found handling from warning to error |
src/cli/config/add.rs |
Initialized new fields in daemon creation |
src/tui/app.rs |
Initialized new fields in editor state |
src/daemon_list.rs |
Initialized env field in daemon list builder |
src/deps.rs |
Initialized new fields in test helper |
tests/test_pitchfork_toml.rs |
Added 8 unit tests for config parsing and serialization |
tests/test_e2e.rs |
Added 5 E2E tests for dir and env functionality |
tests/common/mod.rs |
Enhanced test diagnostics and increased retry timeout |
docs/reference/configuration.md |
Added documentation for dir and env configuration fields |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Summary of ChangesHello @jdx, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly enhances daemon configuration by introducing Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces dir and env configuration fields for daemons, allowing for custom working directories and environment variables. The changes are well-implemented, propagating the new options through all daemon lifecycle paths, including start, retry, cron, and file-watching. The documentation has been updated clearly, and the new functionality is thoroughly covered by both unit and end-to-end tests. I've found one minor opportunity to simplify the code, which I've detailed in a specific comment. Overall, this is a great addition to the project.
src/ipc/batch.rs
Outdated
| Some(d) => { | ||
| let p = PathBuf::from(d); | ||
| if p.is_absolute() { p } else { base_dir.join(p) } | ||
| } |
There was a problem hiding this comment.
The logic for handling absolute and relative paths in this match arm can be simplified. The Path::join method already handles absolute paths correctly by replacing the base path, so the explicit is_absolute check is redundant. You can directly use base_dir.join(d).
Some(d) => base_dir.join(d),|
bugbot run |
Add two new configuration fields to daemon definitions: - `dir`: Set a custom working directory (relative to pitchfork.toml location or absolute). Addresses discussion #226. - `env`: Pass environment variables as key-value pairs to daemon processes. Addresses discussion #211. Example config: ```toml [daemons.frontend] run = "npm run dev" dir = "frontend" env = { NODE_ENV = "development", PORT = "3000" } ``` Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
37eb7d6 to
7c57529
Compare
|
bugbot run |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
src/supervisor/state.rs:1
- The
envmerge logic keeps the previous env whenopts.envisNone. For start/restart flows where “no env configured” is represented asNone, this causes stale env vars from an earlier run to persist even after config changes removeenv. Prefer makingenvoverwrite (i.e., “source of truth” from the current run/config) rather than falling back to existing state, or introduce a tri-state to distinguish “not provided” vs “explicitly none”.
//! State access layer for the supervisor
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pub fn resolve_daemon_dir(dir: Option<&str>, config_path: Option<&Path>) -> PathBuf { | ||
| let base_dir = config_path | ||
| .and_then(|p| p.parent()) | ||
| .map(|p| p.to_path_buf()) | ||
| .unwrap_or_default(); | ||
| match dir { | ||
| Some(d) => { | ||
| let p = PathBuf::from(d); | ||
| if p.is_absolute() { p } else { base_dir.join(p) } | ||
| } | ||
| None => base_dir, | ||
| } | ||
| } |
There was a problem hiding this comment.
unwrap_or_default() makes the “no config_path” default directory an empty path (""). Passing that into Command::current_dir commonly fails (e.g., chdir("") → ENOENT), which can break ad-hoc runs or any path where config_path is None. Consider defaulting base_dir to the process CWD when config_path is missing (and ensure relative dir values resolve against that), rather than returning an empty PathBuf.
| pt.daemons = daemons; | ||
| pt.write()?; | ||
|
|
There was a problem hiding this comment.
These assertions are brittle because they can fail on unrelated substrings (e.g., daemon names/values containing dir/env, future comments, formatting changes). A more robust check is to parse the written TOML and assert the dir/env keys are absent under [daemons.test] (or to assert the serialized representation of that specific daemon table doesn’t include those keys).
| let toml_content = format!( | ||
| r#" | ||
| [daemons.dir_test] | ||
| run = "bash -c 'pwd > {} && sleep 60'" | ||
| dir = "mysubdir" | ||
| ready_delay = 1 | ||
| "#, | ||
| marker.display() | ||
| ); |
There was a problem hiding this comment.
The marker path is interpolated into a shell redirection unquoted (pwd > {}), so the test can break if the path contains spaces or other shell-significant characters. Quoting/escaping the path in the shell snippet (similarly for the echo ... > {} cases) will make these E2E tests more robust across environments.
The TUI editor's to_daemon_config() was hardcoding dir: None and env: None, silently dropping these values when saving edits. Now EditorState captures dir and env from the original config on edit and carries them through to the saved output. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Replace the silent preservation of dir/env with actual editable form fields. The dir field uses optional text input, and env uses a comma-separated KEY=VALUE string list format. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| config.env = None; | ||
| } else { | ||
| let mut map = indexmap::IndexMap::new(); | ||
| for entry in v { | ||
| if let Some((k, val)) = entry.split_once('=') { | ||
| map.insert(k.trim().to_string(), val.trim().to_string()); | ||
| } | ||
| } | ||
| config.env = if map.is_empty() { None } else { Some(map) }; |
There was a problem hiding this comment.
The TUI silently drops invalid env entries (anything without =) and also drops the whole env if all entries are invalid, which can cause surprising data loss for users editing config. Consider validating input and surfacing an error (or preserving invalid entries in the form until corrected), and optionally rejecting empty keys after trim() to avoid inserting "" as an environment variable name.
| config.env = None; | |
| } else { | |
| let mut map = indexmap::IndexMap::new(); | |
| for entry in v { | |
| if let Some((k, val)) = entry.split_once('=') { | |
| map.insert(k.trim().to_string(), val.trim().to_string()); | |
| } | |
| } | |
| config.env = if map.is_empty() { None } else { Some(map) }; | |
| // An explicitly empty list clears the environment. | |
| config.env = None; | |
| } else { | |
| let mut map = indexmap::IndexMap::new(); | |
| for entry in v { | |
| if let Some((k, val)) = entry.split_once('=') { | |
| let key = k.trim(); | |
| // Skip entries that would result in an empty environment variable name. | |
| if key.is_empty() { | |
| continue; | |
| } | |
| map.insert(key.to_string(), val.trim().to_string()); | |
| } | |
| } | |
| // Only update env if there is at least one valid entry. | |
| if !map.is_empty() { | |
| config.env = Some(map); | |
| } |
| // Wait a moment for the file to be written | ||
| env.sleep(Duration::from_millis(500)); | ||
|
|
||
| // Verify the daemon ran in the correct directory | ||
| let pwd = std::fs::read_to_string(&marker).unwrap(); |
There was a problem hiding this comment.
These new e2e tests can be flaky: the daemon is configured with ready_delay = 1 (1s), but the test only waits 500ms before reading the marker file and unwrap()s immediately. This can intermittently fail under CI load. Consider polling for the marker file contents with a timeout (or waiting >= ready_delay) before asserting.
| // Wait a moment for the file to be written | |
| env.sleep(Duration::from_millis(500)); | |
| // Verify the daemon ran in the correct directory | |
| let pwd = std::fs::read_to_string(&marker).unwrap(); | |
| // Wait for the marker file to be written, with a timeout to avoid flakiness. | |
| let start = std::time::Instant::now(); | |
| let pwd = loop { | |
| match std::fs::read_to_string(&marker) { | |
| Ok(contents) => break contents, | |
| Err(err) => { | |
| if start.elapsed() >= Duration::from_secs(3) { | |
| panic!("Timed out waiting for marker file {:?}: {}", marker, err); | |
| } | |
| env.sleep(Duration::from_millis(100)); | |
| } | |
| } | |
| }; | |
| // Verify the daemon ran in the correct directory |
tests/test_pitchfork_toml.rs
Outdated
| assert!(!raw.contains("dir"), "dir should not appear when None"); | ||
| assert!(!raw.contains("env"), "env should not appear when None"); |
There was a problem hiding this comment.
Using raw.contains("dir") / raw.contains("env") can produce false positives if those substrings appear elsewhere (e.g., in values, comments, or other keys). It’s more robust to assert on the serialized keys themselves (e.g., lines containing "\ndir =" / "\nenv =", or parse the TOML and assert the keys are absent).
| assert!(!raw.contains("dir"), "dir should not appear when None"); | |
| assert!(!raw.contains("env"), "env should not appear when None"); | |
| assert!( | |
| !raw.contains("\ndir ="), | |
| "dir should not appear when None" | |
| ); | |
| assert!( | |
| !raw.contains("\nenv ="), | |
| "env should not appear when None" | |
| ); |
- Simplify resolve_daemon_dir: use Path::join which handles absolute paths natively, removing redundant is_absolute check - Fix empty path fallback: use CWD instead of empty string when config_path is None, preventing ENOENT on spawn - Fix brittle test: parse TOML and check fields instead of substring matching on serialized output - Quote marker paths in E2E shell commands for robustness with spaces in paths Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
## 🤖 New release * `pitchfork-cli`: 1.3.0 -> 1.4.0 <details><summary><i><b>Changelog</b></i></summary><p> <blockquote> ## [1.4.0](v1.3.0...v1.4.0) - 2026-02-11 ### Added - *(config)* add `dir` and `env` fields for daemons ([#227](#227)) ### Fixed - *(status)* return error when daemon not found and improve test diagnostics ([#224](#224)) </blockquote> </p></details> --- This PR was generated with [release-plz](https://github.com/release-plz/release-plz/). <!-- CURSOR_SUMMARY --> --- > [!NOTE] > **Low Risk** > Release metadata-only change (version bump and changelog updates) with no functional code modifications in this PR. > > **Overview** > Updates the crate version from `1.3.0` to `1.4.0` (including `Cargo.lock`) and adds a new `CHANGELOG.md` entry for the `v1.4.0` release. > > The release notes call out new daemon config fields (`dir`, `env`) and a `status` fix to error when a daemon is not found (plus improved test diagnostics). > > <sup>Written by [Cursor Bugbot](https://cursor.com/dashboard?tab=bugbot) for commit 09bdfb8. This will update automatically on new commits. Configure [here](https://cursor.com/dashboard?tab=bugbot).</sup> <!-- /CURSOR_SUMMARY --> --------- Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Summary
dirconfig field to set a custom working directory for daemons (relative topitchfork.tomlor absolute). Closes Support for setting working directory #226.envconfig field to pass environment variables to daemon processes as key-value pairs. Closes Pass environment variables to processes #211.Example config
Test plan
resolve_daemon_dir()(6 tests covering relative, absolute, nested, and no-config-path cases)dirandenv(8 tests covering inline/table formats, serialization roundtrip, skip-when-none)dirsets working directory (relative and absolute paths)envvars are available to daemon processes (single and multiple vars)dirandenvwork togethercargo fmt --checkcleancargo clippyclean (lib+bins)🤖 Generated with Claude Code
Note
Medium Risk
Touches process-spawn and state persistence paths (start/retry/cron/watch/boot), so mis-resolution of working directories or env merging could break daemon launches or restarts. Coverage is strong with new unit/config/e2e tests, but runtime behavior changes are broad.
Overview
Adds new daemon config fields
dir(working directory, resolved relative to thepitchfork.tomllocation) andenv(key/value environment variables), including JSON schema and configuration docs updates.Propagates
dir/envthrough the runtime lifecycle by extendingPitchforkTomlDaemon,RunOptions, and daemon state, centralizing directory resolution viaresolve_daemon_dir(), and applying env vars during process spawn; this covers normal starts, boot-start, retries, cron triggers, file-watch restarts, and ad-hoc restarts.Updates the TUI editor to edit/persist
dirandenv, tweaksstatusto error when a daemon is missing, and adds/expands unit + TOML roundtrip + e2e tests (including more robust status polling/diagnostics in CI).Written by Cursor Bugbot for commit ba80785. This will update automatically on new commits. Configure here.