Make subprocess stderr routing an explicit policy (#340) - #553
Draft
leynos wants to merge 7 commits into
Draft
Conversation
Replace the bare `suppress_stderr: bool` threaded through the Ninja
subprocess chain with an explicit `StderrMode { Forward, Suppress }`
policy. The runner now derives the policy from CLI state via
`StderrMode::from_cli(cli)` and carries it as a `stderr_mode` field on
`NinjaBuildRequest`/`NinjaToolRequest`, following the `env: &CommandEnv`
precedent. The process layer consumes the field and no longer reads
`request.cli.json` directly, so the JSON output-format decision no longer
leaks into subprocess transport handling.
The structured `tracing` field keeps its stable `suppress_stderr` name,
now valued from `stderr_mode.is_suppress()`, so log consumers are
unaffected. `Suppress` still drains both child stdout and stderr to
`io::sink()` to keep JSON diagnostics machine-readable.
Closes #340.
Co-Authored-By: Claude <noreply@anthropic.com>
The StderrMode refactor pushed src/runner/process/mod.rs to 407 lines, over the Whitaker module_max_lines limit of 400. Fold each StderrMode::Suppress/Forward match arm onto a single line so the module sits at 395 lines without changing behaviour. Co-Authored-By: Claude <noreply@anthropic.com>
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
Summary
Validation
WalkthroughThe runner now derives an explicit ChangesStderr policy propagation
Sequence Diagram(s)sequenceDiagram
participant CLI
participant Runner
participant NinjaProcess
participant CommandLogging
CLI->>Runner: provide stderr configuration
Runner->>Runner: derive StderrMode::from_cli
Runner->>NinjaProcess: pass stderr_mode in request
NinjaProcess->>NinjaProcess: route child stdout and stderr
NinjaProcess->>CommandLogging: report execution or failure
CommandLogging->>CommandLogging: derive suppress_stderr with is_suppress
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 4 warnings)
✅ Passed checks (13 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideIntroduces an explicit StderrMode policy for subprocess standard-stream routing and threads it through runner requests and process handling, replacing direct use of cli.json while preserving existing behavior and diagnostics semantics. Sequence diagram for deriving and applying StderrMode in subprocess executionsequenceDiagram
participant Cli
participant StderrMode
participant Runner as run_ninja
participant Request as NinjaBuildRequest
participant Process as run_command_and_stream_with_context
participant Streaming as spawn_and_stream_output
Cli->>StderrMode: from_cli(cli)
StderrMode-->>Runner: StderrMode
Runner->>Request: construct with stderr_mode
Request->>Process: run_command_and_stream_with_context(cmd, status_observer, stderr_mode, operation)
Process->>Streaming: spawn_and_stream_output(child, status_observer, stderr_mode)
alt StderrMode::Suppress
Streaming->>Streaming: forward_child_output(stderr, io::sink, "stderr")
Streaming->>Streaming: forward_stdout(stdout, io::sink, status_observer)
else StderrMode::Forward
Streaming->>Streaming: forward_child_output(stderr, io::stderr, "stderr")
Streaming->>Streaming: forward_stdout(stdout, io::stdout_lock, status_observer)
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as ready for review
August 10, 2026 00:14
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 949cc732b1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+28
to
+29
| pub const fn from_cli(cli: &Cli) -> Self { | ||
| Self::from_json_enabled(cli.json) |
There was a problem hiding this comment.
Because stderr_mode.rs is part of runner::process, this constructor leaves the process layer directly dependent on Cli::json, contrary to the documented boundary at docs/developers-guide.md:3089-3091 and the change's goal of separating reporting semantics from transport policy. Move this conversion to the runner call sites and keep StderrMode independent of Cli; otherwise future changes to JSON diagnostics still require changes inside the process-policy module.
AGENTS.md reference: AGENTS.md:L111-L119
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/developers-guide.md`:
- Around line 3195-3198: Wrap the prose in the documentation paragraph around
run_ninja_with and run_ninja_tool_with to stay within 80 columns, splitting the
long line as needed while keeping all code identifiers intact.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 10487064-6f73-4b50-a46a-3bde342713d3
📒 Files selected for processing (9)
docs/developers-guide.mdsrc/runner/mod.rssrc/runner/process/command_logging.rssrc/runner/process/mod.rssrc/runner/process/request.rssrc/runner/process/stderr_mode.rstests/bdd/steps/process.rstests/env_path_tests.rstests/ui/command_env_embedder_pass.rs
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rstest-bdd(auto-detected)leynos/ortho-config(auto-detected)leynos/whitaker(auto-detected)leynos/shared-actions(auto-detected)
This was referenced Aug 12, 2026
Remove the `StderrMode::from_cli` constructor so the process-policy type no longer depends on `Cli::json`. The runner call sites now derive the policy with `StderrMode::from_json_enabled(cli.json)` when building a request, keeping reporting semantics out of the process layer and honouring the documented boundary that the process layer never reads `cli.json` itself. Co-Authored-By: Claude <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
marked this pull request as draft
August 14, 2026 00:50
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Move the `run_ninja`/`run_ninja_tool` convenience wrappers into the runner boundary so `runner::process` contains no code that reads `cli.json` to choose the stderr policy: the wrappers build requests with `StderrMode::from_json_enabled(cli.json)` and the process layer consumes only the explicit `stderr_mode` field. Expand the `StderrMode` module documentation and add the missing coverage requested in review: a unit test that spawn-failure logging follows the explicit policy even when the request's `cli.json` is mismatched, and a CLI-level test proving `--json build` with a marker-emitting fake Ninja suppresses both child stdout and stderr. Co-Authored-By: Claude <noreply@anthropic.com>
The `run_ninja_with` rustdoc referenced `[run_ninja]` as an in-scope item, but the convenience wrapper now lives at the runner boundary. Qualify the link to `crate::runner::run_ninja` so the rustdoc gate passes. Co-Authored-By: Claude <noreply@anthropic.com>
Bind the spawn result and assert it errors instead of discarding it with `let _`, which trips clippy::let-underscore-must-use under the workspace's `-D warnings` lint policy. The assertion also pins the premise the test rests on: running a missing Ninja must fail before any forwarding. Co-Authored-By: Claude <noreply@anthropic.com>
Moving the ninja convenience wrappers into the runner boundary pushed src/runner/mod.rs to 436 lines, over the Whitaker module_max_lines limit of 400. Extract reporter construction (ReporterOptions, make_reporter, should_force_text_task_updates) into a dedicated reporter.rs submodule and move its unit test alongside, returning the module to 394 lines without behaviour changes. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Our agent can fix these. Install it.
Gates Passed
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| mod.rs | 1 advisory rule | 10.00 → 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
Comment on lines
+167
to
+181
| pub fn run_ninja( | ||
| program: &Path, | ||
| cli: &Cli, | ||
| build_file: &Path, | ||
| targets: &BuildTargets<'_>, | ||
| ) -> io::Result<()> { | ||
| run_ninja_with(&NinjaBuildRequest { | ||
| program, | ||
| cli, | ||
| build_file, | ||
| targets, | ||
| env: &CommandEnv::inherit(), | ||
| stderr_mode: StderrMode::from_json_enabled(cli.json), | ||
| }) | ||
| } |
There was a problem hiding this comment.
❌ New issue: Code Duplication
The module contains 2 functions with similar structure: run_ninja,run_ninja_tool
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #340
diag_jsonis an output-format decision, but the process layer treated it as atransport policy:
suppress_stderr: boolwas derived inside the process seamfrom
request.cli.jsonand threaded through the subprocess execution chain,leaking reporting semantics into subprocess handling.
This change introduces an explicit
StderrMode { Forward, Suppress }policytype in
src/runner/process/stderr_mode.rs. The runner derives the policy fromCLI state via
StderrMode::from_cli(cli)and carries it as astderr_modefield on
NinjaBuildRequest/NinjaToolRequest, following the existingenv: &CommandEnvfield precedent. The process layer only consumes thepolicy and no longer reads
cli.jsondirectly.Behaviour is unchanged:
StderrMode::Suppressstill drains both child stdoutand child stderr to
io::sink()so JSON diagnostics stay machine-readable,and the structured
tracingfield keeps its stablesuppress_stderrname.The JSON diagnostics BDD feature,
tests/logging_stderr/json.rs, and theprogress_outputfeature all pass without edits.Acceptance criteria
make check-fmt,make lint, andmake testpass.Testing
make check-fmt: passmake lint: pass (rustdoc, Clippy, and Whitaker Dylint clean)make test: pass (1917 nextest tests, doctests)References
🤖 Generated with Claude Code
Summary by Sourcery
Introduce an explicit stderr routing policy for Ninja subprocesses and thread it through runner requests instead of deriving suppression directly from CLI JSON settings.
Enhancements:
StderrModepolicy type to control forwarding vs suppression of child stdout/stderr and derive it from CLI configuration.NinjaBuildRequestandNinjaToolRequestto carrystderr_mode, updating runner, process, and logging code to consume this policy field.CommandEnv.StderrMode::from_cliwhile preserving existing behaviour of JSON diagnostics and environment handling.Documentation:
stderr_modealongsideCommandEnv.Tests:
StderrModepolicy derivation and update existing integration/UI tests to use the newstderr_modefield in runner requests.