Skip to content

feat: customize stop signal and refactor config types#406

Merged
jdx merged 3 commits into
jdx:mainfrom
gaojunran:feat-signal
May 3, 2026
Merged

feat: customize stop signal and refactor config types#406
jdx merged 3 commits into
jdx:mainfrom
gaojunran:feat-signal

Conversation

@gaojunran

Copy link
Copy Markdown
Contributor

No description provided.

@gaojunran gaojunran marked this pull request as draft May 2, 2026 09:13
@greptile-apps

greptile-apps Bot commented May 2, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces customizable stop signals via the new StopSignal/StopConfig types, and refactors several previously inline config types (Retry, PortBump, PortConfig, WatchMode, CronRetrigger, Dir, etc.) into a dedicated src/config_types.rs module. The --auto-bump-port CLI flag is renamed to --bump and the previously broken optional-value form is fixed by adding num_args = 0..=1.

Confidence Score: 5/5

Safe to merge — one P2 style cleanup in pitchfork.toml, no functional issues.

All P1 issues from previous review rounds are addressed. Core logic for signal dispatch, serialization, and config migration is sound.

pitchfork.toml — still mixes deprecated and new port config fields for the docs daemon

Important Files Changed

Filename Overview
src/config_types.rs New file extracting config value types (StopSignal, StopConfig, Retry, PortBump, PortConfig, etc.) from pitchfork_toml.rs — clean design with BoolOrU32 and StringOrStruct traits, correct serde and JsonSchema impls.
pitchfork.toml docs daemon retains deprecated top-level fields alongside the new [daemons.docs.port] section; triggers deprecation warning at every start.
src/supervisor/lifecycle.rs stop() now reads configured stop_signal from daemon state and passes it through kill_process_group_async; logic is correct.
src/cli/config/add.rs auto-bump-port renamed to --bump; num_args = 0..=1 added so bare --bump now maps correctly to Some(None).
src/procs.rs kill_process_group and kill now accept a configurable stop_signal and optional per-daemon timeout, replacing the hardcoded SIGTERM.
src/pitchfork_toml.rs Re-exports config types from new config_types.rs module; stop_signal properly threaded through read/write paths.
src/supervisor/watchers.rs Watcher restart path now reads stop_signal from daemon state and uses it for kill_process_group_async.

Reviews (8): Last reviewed commit: "[autofix.ci] apply automated fixes" | Re-trigger Greptile

Comment thread src/cli/supervisor/mod.rs Outdated
@gaojunran gaojunran changed the title feat: customize stop signal feat: customize stop signal and refactor config types May 2, 2026
@gaojunran gaojunran force-pushed the feat-signal branch 6 times, most recently from 9d6f81b to aff8842 Compare May 3, 2026 07:19
@gaojunran gaojunran marked this pull request as ready for review May 3, 2026 09:32
@gaojunran

Copy link
Copy Markdown
Contributor Author

@jdx ready for a review!

@jdx jdx merged commit 307990b into jdx:main May 3, 2026
3 checks passed
@mise-en-dev mise-en-dev mentioned this pull request May 3, 2026
jdx pushed a commit that referenced this pull request May 3, 2026
this is a stacked PR and should wait for #406 to be merged.
jdx pushed a commit that referenced this pull request May 3, 2026
## 🤖 New release

* `pitchfork-cli`: 2.8.0 -> 2.9.0

<details><summary><i><b>Changelog</b></i></summary><p>

<blockquote>

## [2.9.0](v2.8.0...v2.9.0)
- 2026-05-03

### Added

- keep ANSI by default and impl PTY mode
([#408](#408))
- customize stop signal and refactor config types
([#406](#406))
- add hook `on_output`
([#399](#399))
</blockquote>


</p></details>

---
This PR was generated with
[release-plz](https://github.com/release-plz/release-plz/).
jdx added a commit that referenced this pull request May 3, 2026
## Summary

The v2.9.0 release pipeline failed on both Windows targets (run
[25281536062](https://github.com/endevco/pitchfork/actions/runs/25281536062))
due to two recently-merged PRs that didn't cfg-gate Unix-only code:

- **#408** added `src/supervisor/pty.rs` (uses `std::os::fd`) and a bare
`mod pty;` in `supervisor/mod.rs` — `std::os::fd` doesn't exist on
Windows.
- **#406** introduced `libc::SIG*` references in `src/config_types.rs`,
but `libc` is gated to `[target.'cfg(unix)']` in `Cargo.toml`.

This PR:
- Gates `mod pty;` behind `cfg(unix)`.
- Replaces `libc::SIG*` lookups in `StopSignal` with platform-portable
constants — `libc` on Unix (where signal numbers vary across BSD/Linux),
hardcoded POSIX-typical values on Windows. The Windows constants are
only used for parsing/Display; `procs::kill` ignores `stop_signal` on
Windows and uses `TerminateProcess` via `sysinfo`.
- Cleans up the incidental Windows-only warnings in the same touched
files (unused imports, unused vars, dead-code variant, needless return).
- Adds a `windows-build` CI job running `cargo check --lib --bins
--all-features` on `windows-latest`. Integration tests under `tests/`
use `lsof`/`pkill` and are Unix-only, so we limit the check to lib +
bins. This catches the regression in PR CI rather than the release
pipeline.

A follow-up patch release will be needed since v2.9.0 shipped with no
Windows binaries.

## Test plan

- [x] `cargo check --all-targets --all-features` (Linux) clean
- [x] `cargo check --target x86_64-pc-windows-gnu --all-features` clean
- [x] `cargo clippy --all-targets --all-features -- -D warnings` (Linux)
clean
- [x] `cargo clippy --lib --bins --target x86_64-pc-windows-gnu -- -D
warnings` clean
- [ ] New `windows-build` CI job passes on this PR

🤖 Generated with [Claude Code](https://claude.com/claude-code)

<!-- CURSOR_SUMMARY -->
---

> [!NOTE]
> **Low Risk**
> Low risk: changes are primarily platform `cfg` gating and CI
configuration to catch Windows-only compile issues; runtime behavior is
unchanged except for stop-signal parsing/display constants on Windows.
> 
> **Overview**
> Restores Windows compatibility by `cfg`-gating Unix-only
modules/imports (e.g. `supervisor::pty`, Unix signal handling) and
fixing Windows-only warnings/unused code paths.
> 
> Makes `StopSignal` platform-portable by using `libc` signal numbers on
Unix and POSIX-typical constants on Windows, and adjusts Windows
process-kill code to ignore unused stop-signal parameters.
> 
> Adds a new GitHub Actions `windows-build` job that runs `cargo check
--lib --bins --all-features` on `windows-latest` to catch Windows
compile regressions in PR CI.
> 
> <sup>Reviewed by [Cursor Bugbot](https://cursor.com/bugbot) for commit
d3c3830. Bugbot is set up for automated
code reviews on this repo. Configure
[here](https://www.cursor.com/dashboard/bugbot).</sup>
<!-- /CURSOR_SUMMARY -->

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants