cli: collapse filter_run visit flags and GitResult bool pair into enums

[robobun]

Two small type-tightening refactors that replace dependent bool pairs with enums so impossible states are unrepresentable. No behavior change.

ProcessHandle.visited / visiting → VisitState

has_cycle() in src/runtime/cli/filter_run.rs is a textbook DFS cycle check that set both flags on entry and cleared only visiting on exit, so visiting && !visited was unreachable. Replaced with:

enum VisitState { Unvisited, Visiting, Visited }

and an exhaustive match in has_cycle.

GitResult.ok / spawn_failed → tri-variant enum

In src/runtime/cli/test/ChangedFilesFilter.rs, every run_git constructor produced exactly one of {spawn_failed, !ok, ok}; ok && spawn_failed was impossible and every caller checked spawn_failed first, then ok. Replaced with:

enum GitResult {
    SpawnFailed,
    ExitError { stderr: Vec<u8> },
    Ok { stdout: Vec<u8> },
}

Each variant now carries only the output stream the caller actually reads on that path (stderr on error, stdout on success), and call sites became exhaustive matches. Net -9 lines.

Verification

• test/cli/run/filter-workspace.test.ts: 56 pass (incl. ignore dependency order on cycle, detect cycle of length > 2, respect dependency order)
• test/cli/test/test-changed.test.ts: 20 pass (incl. errors helpfully outside a git repo, --changed=<ref> paths, subdirectory/untracked paths)

There is no new test because there is no behavior to distinguish: each run_git call site maps one-to-one from the old {ok, spawn_failed} pair to a variant (spawn failure, nonzero exit, success), and has_cycle performs the same DFS with the same detection condition (back-edge to a node on the current stack). The suites above exercise every touched path, matching the approach taken in #36762.

[coderabbitai]

Warning

Review limit reached

You’ve reached a temporary PR review limit under our Fair Usage Limits Policy.

Your recent review volume is higher than typical usage, so adaptive limits are currently applied.

Next review available in: 59 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: e632f379-dd4d-432c-a17f-6e7f4ade2c0a

📥 Commits

Reviewing files that changed from the base of the PR and between 7354c5e and 35af1c2.

📒 Files selected for processing (2)

• src/runtime/cli/filter_run.rs
• src/runtime/cli/test/ChangedFilesFilter.rs

---

_{Comment @coderabbitai help to get the list of available commands.}

[robobun]

Status: both test suites pass locally (filter-workspace.test.ts 56 pass, test-changed.test.ts 20 pass). Self-review found no surviving concerns.

CI build 87671: the only hard failure is test/js/bun/s3/s3.test.ts with S3Error: ServiceUnavailable from Cloudflare R2 across several lanes (external outage, unrelated to this diff). The tests covering the changed code (filter-workspace.test.ts, test-changed.test.ts) are green. Ready for review.

[claude]

LGTM — mechanical bool-pair → enum refactor with identical control flow.

What was reviewed:

• Traced has_cycle DFS: the old (visited, visiting) state pairs map exactly onto Unvisited/Visiting/Visited, including the outer-loop re-entry on already-visited nodes and the early-return-on-cycle path.
• Traced all six run_git call sites: each SpawnFailed/ExitError/Ok arm matches the prior spawn_failed → ok cascade, including the silent ExitError {..} => {} fallthroughs for unstaged/staged/ls-files.
• Confirmed GitResult and the visit fields have no consumers outside these two files; dropping the unread stdout/stderr per variant is safe.

Extended reasoning...

Overview

Two self-contained type-tightening refactors in CLI code:

1. src/runtime/cli/filter_run.rs: replaces the visited: bool / visiting: bool pair on ProcessHandle with a tri-state VisitState enum. The only consumer is has_cycle(), a standard DFS three-color cycle detector. I verified the state mapping is exact: old (false,false) → Unvisited, (true,true) → Visiting, (true,false) → Visited; the fourth combination was already unreachable. The outer loop that re-invokes has_cycle on already-processed handles behaves identically (temporarily re-marks Visiting, deps are all Visited, restores Visited).

2. src/runtime/cli/test/ChangedFilesFilter.rs: replaces the GitResult { ok, spawn_failed, stdout, stderr } struct with a three-variant enum carrying only the stream each caller reads. I walked every call site (rev-parse --show-toplevel, diff HEAD, fallback diff + diff --cached, diff <since>, ls-files --others) and each match arm is behaviorally identical to the old if spawn_failed / if ok cascade — same error messages, same early returns, same silent fallthroughs on non-fatal exit errors. run_git itself now constructs the variant directly instead of populating unused fields.

Security risks

None. No new inputs, no new external calls, no changes to path handling or validation — the git argv, cwd, and output parsing are untouched.

Level of scrutiny

Low-to-medium. This is a pure refactor of internal control-flow representation with no user-facing surface change. The main risk in bool-pair → enum conversions is misreading which combination maps to which arm; I traced each one explicitly and found no divergence. Grep confirms neither GitResult nor the visit fields are referenced outside these files.

Other factors

• Both relevant test suites (filter-workspace.test.ts 56 pass, test-changed.test.ts 20 pass) were run locally per the PR description, covering cycle detection and the git error/subdirectory paths.
• The comment-cop bot flagged a long doc comment on SpawnFailed; the author tightened it to one line in 9329c01 and the thread is resolved.
• Net -9 lines, exhaustive matches replace nested conditionals — this is exactly the "simplest honest shape" the review guide asks for.

[robobun]

Closing: this is a pure type-tightening refactor with no observable behavior change (both reviews above confirm the control flow is byte-for-byte identical), so there is no test that can fail before and pass after. The existing suites already cover every changed path:

• test/cli/run/filter-workspace.test.ts exercises has_cycle via respect dependency order, ignore dependency order on cycle, detect cycle of length > 2
• test/cli/test/test-changed.test.ts exercises every run_git call site including the ExitError path (errors helpfully outside a git repo) and the Ok paths

Both pass against this branch. The branch is left in place in case a maintainer wants to land it directly.

[claude]

LGTM — mechanical bool-pair → enum refactor matching the pattern of #36762/#36764/#36765/#36766.

What was reviewed:

• Traced has_cycle DFS: old (visited, visiting) pair had exactly three reachable states; the new VisitState match preserves the check order (Visiting → cycle, Unvisited → recurse, Visited → skip) including when the outer loop re-enters an already-Visited node.
• Traced all five run_git call sites: each SpawnFailed/ExitError/Ok arm produces the identical error message and control flow as the old spawn_failed-then-ok chain; the dropped stderr-on-Ok / stdout-on-ExitError were never read.
• GitResult and visit_state have no other consumers in the tree.

Extended reasoning...

Overview

Two independent type-tightening refactors in CLI code, each replacing a pair of dependent bool fields with a small enum:

• src/runtime/cli/filter_run.rs: ProcessHandle.visited/visiting → enum VisitState { Unvisited, Visiting, Visited }, with has_cycle rewritten as an exhaustive match.
• src/runtime/cli/test/ChangedFilesFilter.rs: struct GitResult { ok, spawn_failed, stdout, stderr } → enum GitResult { SpawnFailed, ExitError { stderr }, Ok { stdout } }, with all five call sites converted to exhaustive match.

This is the same pattern landed in the last four merged PRs on main (#36762, #36764, #36765, #36766).

Behavior preservation

I walked every branch:

• has_cycle: The old code set both flags on entry and cleared only visiting on exit, so (visiting=true, visited=false) was unreachable. The old dep check tested dep.visiting first (→ cycle) then !dep.visited (→ recurse), which is exactly Visiting / Unvisited / fall-through Visited. The outer for handle in state.handles loop can call has_cycle on an already-Visited node; both old and new re-mark it in-progress, iterate (all deps Visited → no-op), re-mark done, return false. Identical.
• GitResult: For each of the five call sites (rev-parse, diff HEAD, inner diff/diff --cached, diff <since>, ls-files), the new match arms line up one-for-one with the old if spawn_failed / if !ok / if ok chains, including which paths print an error vs. silently continue vs. return GitFailed. In run_git itself, both spawn-error paths still Output::err_generic before returning SpawnFailed, preserving the "already reported" contract in the doc comment. The only data dropped is stderr on success and stdout on non-zero exit — neither was ever read.

Grep confirms GitResult and the visit-state fields are used nowhere else in the tree.

Security risks

None. This touches CLI-internal control flow for bun run --filter dependency ordering and bun test --changed git subprocess result handling. No user-input parsing, no auth, no crypto, no memory-safety changes.

Level of scrutiny

Low. Pure refactor with no observable behavior change, following an established batch pattern already merged four times this week. Both covering test suites (filter-workspace.test.ts 56 tests, test-changed.test.ts 20 tests) pass, and CI is green on the changed code paths (the only CI failure is an unrelated R2 outage in s3.test.ts).

Other factors

The comment-cop bot flag on the SpawnFailed doc comment was addressed in 9329c01 (tightened to one line) and the thread is resolved. No outstanding reviewer comments.

[robobun]

Nothing to address from this review, and there are no open threads. All test lanes covering the changed paths are green; the remaining red in the aggregate is a pre-existing worker_threads asan failure on main plus per-file flakes that passed alone or on retry, none in these files. No new test because there is no behavior to distinguish (rationale and suite runs in the description, same approach as #36762), so this is ready for a maintainer call.