Decouple runner process execution from Cli (#339) - #371
Conversation
|
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
WalkthroughSeparate CLI translation from Ninja process execution. Add ChangesNinja process decoupling
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 warnings, 5 inconclusive)
✅ Passed checks (13 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideDecouples the runner subprocess layer from the Cli type by introducing a narrow NinjaProcessOptions struct and performing Cli-to-process translation at the runner orchestration boundary, keeping public runner APIs unchanged while updating internal request/command configuration plumbing. Sequence diagram for run_ninja decoupled call flowsequenceDiagram
participant Caller
participant runner as runner
participant process as process
participant cmd as Command
Caller->>runner: run_ninja(program, cli, build_file, targets)
runner->>runner: ninja_process_options(cli)
runner->>process: run_ninja(program, options, build_file, targets)
process->>cmd: configure_ninja_build_command(cmd, options, build_file, targets)
process->>process: run_command_and_stream(cmd, status_observer, options.suppress_stderr)
process-->>runner: io::Result
runner-->>Caller: io::Result
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
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 | 9.39 → 9.10 | 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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Comment on file //! Internal to `runner`; public API is defined in `runner.rs`.
use super::{BuildTargets, NINJA_PROGRAM};
use crate::cli::Cli;❌ Getting worse: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
6e0a2d7 to
9a75f64
Compare
![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.
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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. src/runner/process/command_logging.rs Comment on file );
}
/// Determine the operation label from a fully configured Ninja command.
❌ New issue: Code Duplication |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Comment on lines +235 to +238 fn run_ninja_internal<F>(
program: &Path,
options: &NinjaProcessOptions,
build_file: &Path,❌ New issue: Excess Number of Function Arguments |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Comment on lines -257 to -266 ) -> io::Result<()> {
run_ninja_internal(
NinjaInternalRequest {
program: request.program,
cli: request.cli,
status_observer,
operation: request.tool,
},
|cmd| configure_ninja_tool_command(cmd, &request),
)❌ Getting worse: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
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.
![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}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
122011a to
e0fe679
Compare
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The low-level subprocess adapter in `runner::process` accepted `&Cli` through command construction and its request structs, coupling it to the parser/config domain type and making reuse and testing harder. Introduce `NinjaProcessOptions` — the narrow execution type carrying only what the process layer needs (working directory, job count, and the stderr-suppression flag) — and translate from `Cli` once at the orchestration boundary via `runner::ninja_process_options`. The public `run_ninja`/`run_ninja_tool` entry points keep their `&Cli` signatures but now live in `runner::mod` as thin translating wrappers, so existing callers and tests are unchanged. `runner::process` no longer imports `Cli` at all; making the stderr flag a named policy type is tracked separately in #340.
Keep `runner` below the module-size limit by moving the CLI-to-process translation and public compatibility wrappers into their own runner-owned adapter module. Update the embedded-process fixture and design documentation so requests remain parser-independent while `CommandEnv` retains its distinct role.
Route build and tool invocations through one configured-command helper so streaming, stderr policy, and error propagation remain centralised. Retain the request types and verify the command argument order without spawning Ninja.
Convert CLI directories to UTF-8 process options at the runner boundary and reject invalid paths before spawning Ninja. Reuse shared rstest setup for command-configuration coverage and document the process request contract.
Assert the operation labels emitted for build and named tool execution, and ensure non-zero exits retain their structured status diagnostics.
Cover successful and failed Ninja exits plus the operation labels emitted by build and tool execution. Document the request-based execution context so adapter callers and implementation guidance remain aligned.
Remove obsolete runner wrappers and migrate remaining request fixtures to the options and stream-policy APIs. Split the focused execution-context tests into their own module so the process test module remains within the repository size limit.
NinjaProcessOptions.jobs now carries a NinjaJobCount newtype that enforces the CLI's supported 1..=64 semantics at every construction site. ninja_process_options converts the raw CLI value and rejects out-of-range counts with an InvalidInput error before a request reaches the process layer. The stderr-routing tests gain a crate-level #![cfg(unix)] gate, and their documentation now describes only the explicit StderrMode routing the tests actually construct. A new process-layer test pins working-directory propagation by having the fake Ninja record its effective current directory. Co-Authored-By: Claude <noreply@anthropic.com>
Rebasing onto origin/main surfaced weave-resolved regions in src/runner/mod.rs and src/runner/process/mod.rs whose import and module-declaration ordering drifted from rustfmt's canonical form. No semantics change; cargo fmt --all -- --check is now clean. Co-Authored-By: Claude <noreply@anthropic.com>
f61ea74 to
fce4f77
Compare
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.
|
@coderabbitai review |
✅ Action performedReview finished.
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Exercise every `usize` against the process boundary so valid counts preserve their value and invalid counts consistently return `InvalidInput`.
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved? tests/stderr_routing_tests.rs (1)
🤖 Detailed instructionsUse a wyvern agent team to verify each finding against the current code and only fix it if needed. Fix any documentation issues using a scribe agent team. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. Use a scrutineer agent for execution of tests and summarizing the results. In Apply the same fix in Source: Coding guidelines |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== current revision =='
git rev-parse --short HEAD
git status --short
printf '%s\n' '== target file outline and complete contents =='
ast-grep outline tests/stderr_routing_tests.rs --items all
cat -n tests/stderr_routing_tests.rs
printf '%s\n' '== routing terminology and implementation references =='
rg -n -A18 -B8 \
'\b(Cli|json|StderrMode|routing_worker|JOB_ENV|NinjaBuildRequest|NinjaToolRequest)\b' \
tests/stderr_routing_tests.rs src tests -g '*.rs'
printf '%s\n' '== PR checks =='
gh pr view 371 --repo leynos/netsuke \
--json headRefOid,statusCheckRollup \
--jq '{head: .headRefOid, checks: [.statusCheckRollup[] | {name, status, conclusion}]}'Length of output: 168875 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== stderr routing test source =='
sed -n '1,180p' tests/stderr_routing_tests.rs
printf '%s\n' '== target-file matches =='
rg -n -i -C2 \
'cfg\(unix\)|Cli|cli\.json|independent|StderrMode|routing_worker|JOB_ENV|routing' \
tests/stderr_routing_tests.rsLength of output: 13973
Treat this finding as resolved at PR head
Do not add a runner-boundary CLI test for this finding. These tests now correctly test the process-layer contract. You are interacting with an AI system. |
3 participants
Summary
Closes #339
The subprocess adapter in
src/runner/process/mod.rsaccepted&Cliinconfigure_ninja_base, both request structs, and the public entry points,coupling the process layer to the parser/config domain type.
Changes
src/runner/process/mod.rs: newNinjaProcessOptions(working directory,job count, stderr suppression) — the narrow execution type the issue
proposes.
configure_ninja_*,NinjaBuildRequest, andNinjaToolRequestconsume it; the module no longer imports
Cli.src/runner/mod.rs:ninja_process_options(&Cli)performs the CLI-to-processtranslation at the orchestration boundary;
run_ninja/run_ninja_toolkeeptheir public
&Clisignatures as thin wrappers, so existing behaviour,callers, and tests are unchanged.
Replacing the boolean stderr flag with an explicit policy type is
#340, designed together with
this change and stacked on it.
Validation
make check-fmt/make lint/make test— pass (37 suites; runnerbehaviour covered by existing tests, unchanged)
🤖 Generated with Claude Code
Summary by Sourcery
Decouple Ninja process execution from CLI configuration by introducing parser-independent process options while retaining existing public wrappers and behavior.
New Features:
NinjaProcessOptionsand validatedNinjaJobCountfor configuring Ninja process execution independently of CLI parsing state.Enhancements:
Cliwhile preserving the existing CLI-facing convenience wrappers.Documentation:
Tests:
Enhancements:
Introduce a NinjaProcessOptions struct encapsulating working directory, job
count, and stderr suppression for invoking Ninja processes.
Refactor process-layer Ninja build and tool invocation functions to depend on
NinjaProcessOptions instead of the Cli type, removing the parser/config
dependency from the subprocess module.
Add runner-level helpers that translate Cli into NinjaProcessOptions and
delegate to the process-layer Ninja execution functions, preserving existing
public CLI-facing APIs.
Adjust status-reporting build and tool paths to construct and reuse
NinjaProcessOptions when invoking process-layer functions.
References