Skip to content

fix(stream): passthrough watch commands#1547

Closed
nkootstra wants to merge 5 commits into
rtk-ai:developfrom
nkootstra:fix/streaming-watch-passthrough-develop
Closed

fix(stream): passthrough watch commands#1547
nkootstra wants to merge 5 commits into
rtk-ai:developfrom
nkootstra:fix/streaming-watch-passthrough-develop

Conversation

@nkootstra

@nkootstra nkootstra commented Apr 26, 2026

Copy link
Copy Markdown

Summary

Fixes watch/dev-style commands that previously looked like they were streaming but still buffered through RTK filtering paths until the child process exited.

This routes known long-running invocations through passthrough execution so stdout/stderr stay attached to the terminal, while still keeping normal filtered commands on the existing buffered/streaming pipeline. It also updates unsupported go and dotnet passthrough paths to actually stream instead of capturing via .output().

Changes

  • Detect watch/dev/follow/codegen-style invocations centrally before applying filtered execution.
  • Preserve gh pr checks --watch detection when extra args are present.
  • Stream interactive Playwright, unsupported Go, and unsupported Dotnet passthrough routes instead of buffering them.
  • Include small clippy cleanups required by the current develop branch.

Verification

  • smll cargo fmt --all
  • smll cargo clippy --all-targets
  • smll cargo test --all (1691p 0f)

Closes #1545

@CLAassistant

CLAassistant commented Apr 26, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@pszymkowiak pszymkowiak added bug Something isn't working effort-medium 1-2 jours, quelques fichiers filter-quality Filter produces incorrect/truncated signal labels Apr 26, 2026
@pszymkowiak

Copy link
Copy Markdown
Collaborator

[w] wshm · Automated triage by AI

📊 Automated PR Analysis

🐛 Type bug-fix
🟡 Risk medium

Summary

Fixes watch/dev-style commands (e.g. --watch, dev, serve, codegen) that were being buffered through RTK's filtered execution pipeline instead of streaming directly to the terminal. Adds centralized detection of long-running/streaming invocations in the runner module and routes them through passthrough execution, while also converting unsupported Go and Dotnet passthrough paths from buffered capture to proper streaming.

Review Checklist

  • Tests present
  • Breaking change
  • Docs updated

Linked issues: #1545


Analyzed automatically by wshm · This is an automated analysis, not a human review.

@pszymkowiak

pszymkowiak commented Apr 28, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the very thorough fix — the diagnosis in #1545 and the centralized is_streaming_invocation() helper are exactly the right shape.

Looks great:

  • Centralized detection in runner.rs before the filtered branch, with unit tests for watch / follow / dev / finite cases ✅
  • --follow / -f correctly gated on docker | kubectl | tail | journalctl to avoid false positives like git log -f
  • go run and unsupported dotnet routes now actually stream instead of capturing via .output()
  • gh pr checks --watch detection preserved when extra args are present ✅

After a deeper pass (with help from a code-review pass on my side), here's what I'd want addressed before merge.


🔴 Blocking — -w is treated as a universal watch flag

if args.iter().any(|arg| matches!(*arg, "--watch" | "--watchAll" | "-w")) {
    return true;
}

-w is overloaded outside the JS test ecosystem:

  • --word-regexp in grep / rg
  • --width in cargo test and wc
  • not actually documented for Jest (Jest uses --watch); the existing test is_streaming_invocation("jest", "run -w") asserts a behavior that's already shaky for Jest itself

The current grep_cmd.rs happens to bypass runner::run_filtered, so the live blast radius is small today — but any future filter route that accepts -w will silently switch to passthrough and skip filtering. Suggest scoping -w to a known allowlist (jest, vitest, webpack, nodemon) the same way --follow is scoped to log tools.

🟡 Test bug to fix while you're in there

assert!(!is_streaming_invocation("go test", "./..."));

This passes only because tool_name.split_whitespace().next() drops "test" from the tool name. Real callers in main.rs pass tool_name="go" and args_display="test ./...". Recommend rewriting as ("go", "test ./...") so the test reflects the actual call shape.

🟡 Coverage scope is undocumented

The check lives only inside the RunMode::Filtered arm — RunMode::Streamed ignores it entirely. A future contributor adding a filter via run_streamed won't get watch-mode protection. A short doc comment on is_streaming_invocation describing (a) which RunMode it guards and (b) the contract on args_display (structured subcommand tokens only, never user-controlled prose like PR titles) would prevent surprises.

🟡 Missing test cases

While we're here:

  • is_streaming_invocation("cargo", "test -w") should be false — currently returns true
  • pnpm run dev (pnpm has its own filter module, worth confirming the same path is exercised)

My earlier 3 concerns — resolved on second look

  1. Bundled clippy cleanups. The develop branch runs stricter clippy (-D warnings + dead-code lint), which actually requires the signal cast, collapse_if, match → map, and the six #[allow(dead_code)] annotations to compile. They belong in this PR — only the PR description should call this out explicitly so future reviewers don't burn time on it.

  2. stream.rs RAW_CAP boundary. Recomputed on all 5 sites: old condition allows the buffer to reach exactly RAW_CAP; new condition caps at RAW_CAP - 1. One byte more conservative against a multi-MiB cap — invisible in practice, no regression. Worth a one-line commit message clarifying the intent.

  3. Cargo.lock 0.36.0 → 0.34.3. This is the self-referential rtk package version from your fork's Cargo.toml. master is at 0.37.2 and develop is ahead of you — this will conflict on merge. A rebase onto current develop and a fresh cargo build should regenerate the lockfile cleanly.


Edge-case note (no action needed, just visible): the bare-token list watch | dev | serve | start | codegen | show-report applies to all tools positionally on args_display. As long as args_display is composed of structured subcommand tokens (which all current callers respect), this is safe. Documenting that contract on the helper is enough.

Once the -w scoping is tightened and the lockfile is rebased, happy to approve. Thanks again — this is a high-quality fix.

Resolve conflicts in:
- src/core/stream.rs: take upstream #[allow(dead_code)] attribute
- src/discover/lexer.rs: take upstream #[cfg(test)] for strip_quotes
- src/discover/registry.rs: take upstream call signature with
  transparent_prefixes for shell prefix builtins
- src/hooks/constants.rs: take upstream split constants
  (CONFIG_DIR/OPENCODE_SUBDIR/PLUGIN_SUBDIR/OPENCODE_PLUGIN_FILE)
…tch-passthrough-develop

# Conflicts:
#	src/cmds/dotnet/dotnet_cmd.rs
@nkootstra nkootstra closed this Jul 12, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working effort-medium 1-2 jours, quelques fichiers filter-quality Filter produces incorrect/truncated signal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Streaming/watch-mode commands still buffer or suppress stdout outside proxy mode

3 participants