Skip to content

feat: keep ANSI by default and impl PTY mode#408

Merged
jdx merged 1 commit into
jdx:mainfrom
gaojunran:feat-ansi-pty
May 3, 2026
Merged

feat: keep ANSI by default and impl PTY mode#408
jdx merged 1 commit into
jdx:mainfrom
gaojunran:feat-ansi-pty

Conversation

@gaojunran

Copy link
Copy Markdown
Contributor

this is a stacked PR and should wait for #406 to be merged.

@greptile-apps

greptile-apps Bot commented May 3, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces a pty = true daemon configuration option that allocates a pseudo-terminal for daemons that rely on isatty() or produce colour output only when connected to a TTY. It also normalises ANSI handling throughout the stack: ANSI escape codes are preserved in log files, stripped before ready_output / on_output pattern matching, stripped in the web log viewer, and conditionally stripped in the logs CLI command depending on the output terminal's colour support.

Confidence Score: 5/5

Safe to merge once the stacked PR #406 lands; implementation is correct and previously flagged issues are all addressed.

No P0 or P1 findings. Previously flagged issues (saturating_pow backoff,
stripping in PTY mode, #[cfg(unix)] guards, stale port.bats assertions) are all resolved in this diff. The single P2 note is a silent behaviour change for on_output_hook scripts receiving ANSI-stripped text, which is consistent with the PR's intent but not explicitly documented.

src/supervisor/lifecycle.rs — the on_output_hook output change and the merged stdout/stderr channel are worth a second read, but both are implemented correctly.

Important Files Changed

Filename Overview
src/supervisor/lifecycle.rs Major refactor: stdout/stderr readers merged into a single channel; PTY master reader wired up in the same path; ANSI stripping added for pattern matching; backoff overflow fixed with saturating_pow; \r stripping for PTY ONLCR behaviour added. Silent behaviour change: on_output_hook hook command now receives ANSI-stripped output.
src/supervisor/pty.rs New file: wraps libc::openpty into a safe PtyPair struct; correctly acknowledges missing FD_CLOEXEC in comments but does not set it; both master and slave OwnedFd ownership is handled correctly.
src/cli/logs.rs strip_ansi parameter threaded through all output paths (pager, direct, live-stream); strip_ansi = raw
src/web/routes/logs.rs Both the initial HTML render and the SSE stream now strip ANSI codes before html_escape; correct ordering (strip then escape) prevents ANSI sequences appearing as visible HTML entities.
tests/test_e2e_pty.rs Four focused e2e tests covering ANSI preservation in logs (pipe mode and PTY mode), TTY detection with pty=true, and no-TTY default behaviour.
test/port.bats Assertions updated to match new PortConfig serialisation (port = 3000 / expect = [8080] / bump = 10); previously flagged stale assertions are now corrected.
src/pitchfork_toml.rs pty field threaded through PitchforkTomlDaemonRaw → PitchforkTomlDaemon → RunOptions; deprecation warning now fires in both the presence-only and coexistence-with-new-port cases.
src/supervisor/state.rs pty field added to UpsertDaemonOpts; Default derived so builder can use ..Default::default(), removing large boilerplate struct literal.

Reviews (11): Last reviewed commit: "feat: keep ANSI and PTY mode" | Re-trigger Greptile

Comment thread src/supervisor/lifecycle.rs
Comment thread src/supervisor/pty.rs Outdated
Comment thread src/supervisor/lifecycle.rs Outdated
@gaojunran gaojunran force-pushed the feat-ansi-pty branch 2 times, most recently from d872d05 to 6b07116 Compare May 3, 2026 09:42
Comment thread src/supervisor/lifecycle.rs Outdated
@gaojunran gaojunran force-pushed the feat-ansi-pty branch 3 times, most recently from a3dc5f9 to 0fdfd4f Compare May 3, 2026 11:30
Comment thread src/supervisor/lifecycle.rs Outdated
@gaojunran gaojunran force-pushed the feat-ansi-pty branch 3 times, most recently from 7721ae6 to 2105f3c Compare May 3, 2026 12:05
@gaojunran

gaojunran commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

@jdx ready for a review!

on_output hook payload is stripped and error-path fall-through

  • it is on purpose. we don't need to capture a line of output with ANSI, so they are always stripped. And we don't want to expose hook error

tests/test_e2e_pty.rs — fixed sleep durations

  • it's enough even for ci. I've tested it for multiple times

@gaojunran gaojunran marked this pull request as ready for review May 3, 2026 12:15
@jdx

jdx commented May 3, 2026

Copy link
Copy Markdown
Owner

could we just make this default behavior?

@gaojunran

gaojunran commented May 3, 2026

Copy link
Copy Markdown
Contributor Author

you mean keep ANSI as default or strip by default? Now default is strip @jdx

@jdx

jdx commented May 3, 2026

Copy link
Copy Markdown
Owner

I think we should keep ansi as default unless we've disabled colors, then we should strip when outputting the logs not when storing them

@gaojunran gaojunran marked this pull request as draft May 3, 2026 12:23
refactor: config type

[autofix.ci] apply automated fixes
feat: keep ANSI and pty mode
@gaojunran gaojunran marked this pull request as ready for review May 3, 2026 13:26
@gaojunran gaojunran changed the title feat: keep ansi and pty mode feat: keep ANSI by default and impl PTY mode May 3, 2026
@gaojunran

Copy link
Copy Markdown
Contributor Author
  1. log.ansi has been removed. We always store ANSI
  2. The output part (CLI, Web, TUI) decides whether to strip ANSI

@jdx jdx merged commit c0c2458 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
## 🤖 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