Address review comments and verify tests (six-stage progress)#266
Open
Address review comments and verify tests (six-stage progress)#266
Conversation
Add a detailed ExecPlan document for roadmap 3.9.1 specifying design, constraints, risks, and staged work for integrating indicatif::MultiProgress reporting with six stages and localized progress feedback. This plan lays out progress configuration, stage model expansion, reporter implementation, manifest refactoring, testing, and documentation updates. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
Reviewer's GuideImplements a six-stage, localized progress reporting model using indicatif::MultiProgress for standard mode, extends manifest and runner pipelines to expose those stages, adds a configurable progress toggle, and updates tests and docs accordingly while preserving accessible static output. Sequence diagram for six-stage pipeline progress reportingsequenceDiagram
actor User
participant Cli
participant Runner as RunnerModule
participant Manifest as ManifestModule
participant Status as StatusReporter
participant Indicatif as IndicatifReporter
User->>Cli: Parse_args_with_progress_and_accessible
Cli->>Runner: run(cli)
alt Accessible_mode
Runner->>Runner: resolve_output_mode(Accessible)
Runner->>Status: create AccessibleReporter
else Standard_mode_with_progress_true
Runner->>Runner: resolve_output_mode(Standard)
Runner->>Indicatif: IndicatifReporter::new()
Indicatif-->>Runner: boxed_as_StatusReporter
Runner->>Status: use IndicatifReporter
else Standard_mode_with_progress_false
Runner->>Status: create SilentReporter
end
User->>Runner: Command_build_or_manifest
Runner->>Runner: generate_ninja(cli, reporter, tool_key)
Runner->>Runner: resolve_manifest_path(cli)
Runner->>Runner: ensure_manifest_exists_or_error(cli, reporter, manifest_path)
alt Manifest_missing
Runner->>Status: report_pipeline_stage(ManifestIngestion)
Runner-->>Cli: Err ManifestNotFound
else Manifest_exists
Runner->>Runner: cli.network_policy()
Runner->>Manifest: from_path_with_policy_and_stage_callback(path, policy, on_stage)
loop Manifest_stage_callbacks
Manifest-->>Runner: on_stage(ManifestLoadStage)
Runner->>Status: report_pipeline_stage(mapped_PipelineStage)
end
Manifest-->>Runner: NetsukeManifest
Runner->>Status: report_pipeline_stage(IrGenerationValidation)
Runner->>Runner: BuildGraph::from_manifest
Runner->>Status: report_pipeline_stage(NinjaSynthesisAndExecution, tool_key)
Runner->>Runner: ninja_gen::generate
Runner-->>Cli: NinjaContent
end
opt Build_or_tool_execution
Runner->>Runner: run_ninja_or_run_ninja_tool
end
Runner->>Status: report_complete(tool_key)
Runner-->>User: Ok
Class diagram for six-stage progress model and reportersclassDiagram
direction LR
class StageNumberError {
<<enum>>
OutOfRange(value u32)
}
class StageNumber {
- value u32
+ new(value u32) Result_StageNumber_StageNumberError
+ get() u32
}
class StageDescription {
- value String
+ new(value String)
+ as_str() &str
}
class LocalizationKey {
- value &'static str
+ new(value &'static str)
+ as_str() &'static str
}
class StatusReporter {
<<interface>>
+ report_stage(current StageNumber, total StageNumber, description StageDescription) void
+ report_complete(tool_key LocalizationKey) void
}
class AccessibleReporter {
+ report_stage(current StageNumber, total StageNumber, description StageDescription) void
+ report_complete(tool_key LocalizationKey) void
}
class SilentReporter {
+ report_stage(current StageNumber, total StageNumber, description StageDescription) void
+ report_complete(tool_key LocalizationKey) void
}
class IndicatifState {
- progress MultiProgress
- bars Vec~ProgressBar~
- descriptions Vec~StageDescription~
- running_index Option~usize~
- completed bool
- is_hidden bool
}
class IndicatifReporter {
- state Mutex~IndicatifState~
+ new() IndicatifReporter
+ report_stage(current StageNumber, total StageNumber, description StageDescription) void
+ report_complete(tool_key LocalizationKey) void
- set_stage_state(state IndicatifState, index usize, status_key LocalizationKey, finish_line bool) void
}
class PipelineStage {
<<enum>>
ManifestIngestion
InitialYamlParsing
TemplateExpansion
FinalRendering
IrGenerationValidation
NinjaSynthesisAndExecution
+ ALL [PipelineStage; 6]
+ index() StageNumber
+ from_index(index u32) Option~PipelineStage~
+ description(tool_key Option~LocalizationKey~) StageDescription
}
class ManifestLoadStage {
<<enum>>
ManifestIngestion
InitialYamlParsing
TemplateExpansion
FinalRendering
}
class ManifestModule {
+ from_path_with_policy(path Path, policy NetworkPolicy) NetsukeManifest
+ from_path_with_policy_and_stage_callback(path Path, policy NetworkPolicy, on_stage FnMut_ManifestLoadStage_) NetsukeManifest
+ from_str_named(yaml &str, name &ManifestName, stdlib_config Option~StdlibConfig~) NetsukeManifest
+ from_str_named_with_stage_callback(yaml &str, name &ManifestName, stdlib_config Option~StdlibConfig~, on_stage FnMut_ManifestLoadStage_) NetsukeManifest
}
class RunnerModule {
+ run(cli &Cli) Result
+ handle_build(cli &Cli, args &BuildArgs, reporter &StatusReporter) Result
+ handle_clean(cli &Cli, reporter &StatusReporter) Result
+ handle_graph(cli &Cli, reporter &StatusReporter) Result
+ handle_ninja_tool(cli &Cli, tool &str, tool_key LocalizationKey, reporter &StatusReporter) Result
+ generate_ninja(cli &Cli, reporter &StatusReporter, tool_key Option~LocalizationKey~) NinjaContent
+ ensure_manifest_exists_or_error(cli &Cli, reporter &StatusReporter, manifest_path &Utf8PathBuf) Result
+ load_manifest_with_stage_reporting(manifest_path &Utf8PathBuf, policy NetworkPolicy, reporter &StatusReporter) NetsukeManifest
}
StatusReporter <|.. AccessibleReporter
StatusReporter <|.. SilentReporter
StatusReporter <|.. IndicatifReporter
IndicatifReporter *-- IndicatifState
IndicatifState o-- MultiProgress
IndicatifState o-- ProgressBar
IndicatifState o-- StageDescription
PipelineStage --> StageNumber
PipelineStage --> StageDescription
ManifestModule --> ManifestLoadStage
ManifestModule --> PipelineStage
RunnerModule --> StatusReporter
RunnerModule --> PipelineStage
RunnerModule --> ManifestModule
RunnerModule --> LocalizationKey
StageDescription --> LocalizationKey
StageNumber --> StageNumberError
Flow diagram for progress configuration and reporter selectionflowchart LR
A[Start_run_cli] --> B[Resolve_output_mode]
B --> C{Accessible_mode?}
C -- Yes --> D[Use_AccessibleReporter]
C -- No --> E[Resolve_progress_setting]
subgraph Progress_config_precedence
E --> F[Read_config_file_progress]
F --> G[Read_env_NETSUKE_PROGRESS]
G --> H[Read_CLI_flag_progress]
H --> I{Final_progress_value}
end
I -- true --> J[Use_IndicatifReporter_with_MultiProgress]
I -- false --> K[Use_SilentReporter]
D --> L[Execute_command_build_clean_graph_manifest]
J --> L
K --> L
L --> M[generate_ninja_with_stage_reporting]
M --> N[Manifest_ingestion]
N --> O[Initial_YAML_parsing]
O --> P[Template_expansion]
P --> Q[Final_rendering]
Q --> R[IR_generation_and_validation]
R --> S[Ninja_synthesis_and_execution]
S --> T[Reporter_emits_stage_summaries]
T --> U[Reporter_report_complete]
U --> V[End_run]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 Summary by CodeRabbit
WalkthroughIntroduce a six‑stage, localisable real‑time progress pipeline: add tri‑state CLI Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (flags/options)
participant Runner as Runner
participant Manifest as ManifestLoader
participant Reporter as StatusReporter
participant UI as ProgressUI
CLI->>Runner: start (progress, accessible)
Runner->>Runner: resolve OutputMode & progress
Runner->>Reporter: make_reporter(mode, progress)
Reporter->>UI: init MultiProgress / prepare output
Runner->>Manifest: load_manifest_with_stage_reporting(path, policy)
Manifest->>Reporter: report_stage(1/6, "Manifest Ingestion")
Reporter->>UI: update stage 1
Manifest->>Reporter: report_stage(2/6, "Initial YAML Parsing")
Reporter->>UI: update stage 2
Manifest->>Reporter: report_stage(3/6, "Template Expansion")
Reporter->>UI: update stage 3
Manifest->>Reporter: report_stage(4/6, "Final Rendering")
Reporter->>UI: update stage 4
Runner->>Reporter: report_stage(5/6, "IR Generation & Validation")
Reporter->>UI: update stage 5
Runner->>Reporter: report_stage(6/6, "Ninja Synthesis & Execution")
Reporter->>UI: update stage 6
Runner->>Reporter: report_complete(tool_key)
Reporter->>UI: finalise and emit completion message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
…atif - Integrate indicatif::MultiProgress to display persistent progress summaries for six pipeline stages. - Provide localized, stateful labels for pipeline stages: pending, in progress, done, failed. - Support CLI flag, env var, and config file options to enable or disable standard progress summaries. - Accessible mode produces static, labelled status lines and takes precedence over standard mode. - Add manifest loading stages with explicit callbacks for fine-grained progress reporting. - Update documentation and localization files for progress reporting and new CLI flag `--progress`. - Add behavioral tests for progress output, including localization and failure scenarios. - Replace silent reporter with indicatif reporter when progress summaries are enabled in standard mode. - Ensure accurate stage reporting and graceful fallback on failures. This enhancement improves user feedback during builds with real-time, accessible, and localized progress summaries. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
…-progress - Integrated indicatif::MultiProgress for standard terminal progress reporting. - Implemented six-stage progress model with localized stage/state labels. - Added fallback for non-TTY stderr: emit localized summary lines deterministically. - Configured via OrthoConfig layering with support for CLI args & env variables. - Expanded rstest and rstest-bdd coverage for reporter logic, stages, and failure paths. - Updated documentation and marked roadmap item 3.9.1 as done. - Improved testing helpers to normalize Fluent bidi isolate characters. This enables persistent interactive progress summaries on TTYs while providing stable, testable summary lines in CI and non-interactive environments. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Primitive Obsessionsrc/status.rs: What lead to degradation?In this module, 32.4% of all function arguments are primitive types, threshold = 30.0% Why does this problem occur?Code that uses a high degree of built-in, primitives such as integers, strings, floats, lacks a domain language that encapsulates the validation and semantics of function arguments. Primitive Obsession has several consequences: 1) In a statically typed language, the compiler will detect less erroneous assignments. 2) Security impact since the possible value range of a variable/argument isn't retricted. How to fix it?Primitive Obsession indicates a missing domain language. Introduce data types that encapsulate the details and constraints of your domain. For example, instead of int userId, consider User clicked. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# primitive-obsession.ts
-// Problem: It's hard to know what this function does, and what values are valid as parameters.
-function getPopularRepositories(String baseURL, String query, Integer pages, Integer pageSize, String sortorder): Json {
- let pages == null ? 10 : pages
- let pageSize == null ? 10 : pageSize
- return httpClient.get(`${baseURL}?q=${query}&pages=${pages}&pageSize=${pageSize}&sortorder=${sortorder}`)
+// Refactoring: extract the pagination & API logic into a class, and it will
+// attract validation and other logic related to the specific query. It's now
+// easier to use and to maintain the getPopularRepositories function!
+function getPopularRepositories(query: PaginatedRepoQuery): Json {
+ return httpClient.get(query.getURL())
.map(json => json.repositories)
.filter(repository => repositry.stargazersCount > 1000)
} |
This comment was marked as resolved.
This comment was marked as resolved.
- Replaced raw integers and string keys related to pipeline stages with strongly typed structs: `StageNumber`, `StageDescription`, and `LocalizationKey`. - Moved `PipelineStage` enum and related reporting functions to a new module `status_pipeline.rs`. - Updated all `StatusReporter` trait methods and implementations to use the new types instead of raw primitives. - Improved error handling and validation around stage numbers. - Added tests for the new stage modeling types in `status_tests.rs`. - Enhanced localization usage consistency by wrapping keys in `LocalizationKey` type. - Changed calls in `runner` to pass `LocalizationKey` instances instead of plain static string keys. This refactor enhances type safety, readability, and maintainability of the pipeline status reporting subsystem. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 16 issues, and left some high level feedback:
- The
normalize_fluent_isolateshelper is duplicated across multiple test modules (localization_tests.rs,manifest_command.rs,tests/bdd/helpers/assertions.rs); consider consolidating this into a shared test utility to avoid drift and keep the normalization logic consistent. - The stage indexing model currently has multiple sources of truth (
PIPELINE_STAGE_COUNT,PipelineStage::ALL,StageNumber(Stage as u32)andPIPELINE_STAGE_TOTAL = StageNumber(PIPELINE_STAGE_COUNT)); it may be safer to derive the count andStageNumbersolely fromPipelineStage::ALL(or similar) so changes to the enum cannot silently desynchronize the validated range.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `normalize_fluent_isolates` helper is duplicated across multiple test modules (`localization_tests.rs`, `manifest_command.rs`, `tests/bdd/helpers/assertions.rs`); consider consolidating this into a shared test utility to avoid drift and keep the normalization logic consistent.
- The stage indexing model currently has multiple sources of truth (`PIPELINE_STAGE_COUNT`, `PipelineStage::ALL`, `StageNumber(Stage as u32)` and `PIPELINE_STAGE_TOTAL = StageNumber(PIPELINE_STAGE_COUNT)`); it may be safer to derive the count and `StageNumber` solely from `PipelineStage::ALL` (or similar) so changes to the enum cannot silently desynchronize the validated range.
## Individual Comments
### Comment 1
<location> `src/manifest/tests/stages.rs:19-28` </location>
<code_context>
+ Ok(())
+}
+
+#[test]
+fn stage_callback_stops_after_parse_failure() -> Result<()> {
+ let dir = tempfile::tempdir().context("create temp workspace for stage failure test")?;
+ let manifest_path = dir.path().join("Netsukefile");
</code_context>
<issue_to_address>
**suggestion (testing):** Cover additional failure modes in manifest stage callbacks (template expansion and final rendering)
These tests cover the happy path and an early YAML-parse failure, but there are other failure points in the manifest pipeline: template expansion and final rendering/deserialization. Please add tests where (a) template expansion fails and (b) final rendering fails, and assert that the stage sequence stops at TemplateExpansion or FinalRendering respectively and does not emit later stages, to fully exercise the callback contract across all error paths.
Suggested implementation:
```rust
let manifest = manifest::from_path_with_policy_and_stage_callback(
&manifest_path,
NetworkPolicy::default(),
|stage| stages.push(stage),
)?;
assert_eq!(
stages,
vec![
manifest::Stage::Discovery,
manifest::Stage::Parsing,
manifest::Stage::TemplateExpansion,
manifest::Stage::FinalRendering
]
);
Ok(())
}
#[test]
fn stage_callback_stops_after_parse_failure() -> Result<()> {
let dir = tempfile::tempdir().context("create temp workspace for stage failure test")?;
let manifest_path = dir.path().join("Netsukefile");
std::fs::write(
&manifest_path,
"this: is: not: valid: yaml: [", // invalid YAML to force parse failure
)
.with_context(|| format!("write {}", manifest_path.display()))?;
let mut stages = Vec::new();
let result = manifest::from_path_with_policy_and_stage_callback(
&manifest_path,
NetworkPolicy::default(),
|stage| stages.push(stage),
);
assert!(result.is_err());
assert_eq!(
stages,
vec![
manifest::Stage::Discovery,
manifest::Stage::Parsing,
]
);
Ok(())
}
#[test]
fn stage_callback_stops_after_template_expansion_failure() -> Result<()> {
let dir =
tempfile::tempdir().context("create temp workspace for stage template expansion failure")?;
let manifest_path = dir.path().join("Netsukefile");
// Manifest that parses as valid YAML but fails during template expansion.
// This assumes that using an unknown template function causes expansion failure.
let manifest_contents = concat!(
"netsuke_version: \"1.0.0\"\n",
"rules:\n",
" - name: touch\n",
" command: \"touch ${{ unknown_func() }}\"\n",
"targets:\n",
" - name: out.txt\n",
" rule: touch\n",
);
std::fs::write(&manifest_path, manifest_contents)
.with_context(|| format!("write {}", manifest_path.display()))?;
let mut stages = Vec::new();
let result = manifest::from_path_with_policy_and_stage_callback(
&manifest_path,
NetworkPolicy::default(),
|stage| stages.push(stage),
);
assert!(result.is_err());
assert_eq!(
stages,
vec![
manifest::Stage::Discovery,
manifest::Stage::Parsing,
manifest::Stage::TemplateExpansion,
]
);
Ok(())
}
#[test]
fn stage_callback_stops_after_final_rendering_failure() -> Result<()> {
let dir =
tempfile::tempdir().context("create temp workspace for stage final rendering failure")?;
let manifest_path = dir.path().join("Netsukefile");
// Manifest that parses and expands templates but fails during final rendering / deserialization.
// This assumes that an invalid type for `command` will cause the final rendering to fail.
let manifest_contents = concat!(
"netsuke_version: \"1.0.0\"\n",
"rules:\n",
" - name: touch\n",
" command: [1, 2, 3]\n", // invalid type for command which should be a string
"targets:\n",
" - name: out.txt\n",
" rule: touch\n",
);
std::fs::write(&manifest_path, manifest_contents)
.with_context(|| format!("write {}", manifest_path.display()))?;
let mut stages = Vec::new();
let result = manifest::from_path_with_policy_and_stage_callback(
&manifest_path,
NetworkPolicy::default(),
|stage| stages.push(stage),
);
assert!(result.is_err());
assert_eq!(
stages,
vec![
manifest::Stage::Discovery,
manifest::Stage::Parsing,
manifest::Stage::TemplateExpansion,
manifest::Stage::FinalRendering,
]
);
Ok(())
}
```
1. Adjust the `manifest::Stage` variants (`Discovery`, `Parsing`, `TemplateExpansion`, `FinalRendering`) to match the actual enum names used in your codebase.
2. The concrete ways of triggering template-expansion and final-rendering failures (`unknown_func()` in a template, list value for `command`) may need to be updated to match your real template engine failure conditions and deserialization rules. Adapt the manifest snippets so that:
- The template-expansion test produces a failure during template expansion (not during parsing or final rendering).
- The final-rendering test produces a failure during the last rendering/deserialization step (after templates have been successfully expanded).
3. If the "happy path" test already asserts on `stages` and returns `Ok(())` elsewhere in the file, deduplicate or align the assertion and `Ok(())` with existing conventions.
</issue_to_address>
### Comment 2
<location> `tests/localization_tests.rs:48-51` </location>
<code_context>
+/// directionality when interpolating values. They are invisible to users but
+/// can make plain substring assertions fail.
+#[must_use]
+fn normalize_fluent_isolates(text: &str) -> String {
+ text.chars()
+ .filter(|ch| *ch != '\u{2068}' && *ch != '\u{2069}')
+ .collect()
+}
+
</code_context>
<issue_to_address>
**suggestion:** Deduplicate normalize_fluent_isolates helper across test modules for maintainability
This helper is now defined in multiple test modules (localization_tests.rs, bdd/steps/manifest_command.rs, bdd/helpers/assertions.rs). Please extract it into a shared test utility and reference it from each caller to keep the normalization logic consistent and reduce maintenance overhead if Fluent’s behavior changes.
Suggested implementation:
```rust
#[rstest]
```
To complete the deduplication across the codebase, you’ll also need to:
1. Extract `normalize_fluent_isolates` into a shared test utility module (for example, `tests/common/mod.rs` or `tests/support/fluent.rs`, depending on your existing test structure):
```rust
// e.g. tests/common/mod.rs
#[must_use]
pub fn normalize_fluent_isolates(text: &str) -> String {
text.chars()
.filter(|ch| *ch != '\u{2068}' && *ch != '\u{2069}')
.collect()
}
```
2. Import and use this shared helper in all test modules that currently define their own version:
- `tests/localization_tests.rs` (this file): add an appropriate `use` statement at the top, e.g. `use crate::common::normalize_fluent_isolates;` or `use crate::support::fluent::normalize_fluent_isolates;` depending on where you put it.
- `tests/bdd/steps/manifest_command.rs`
- `tests/bdd/helpers/assertions.rs`
3. Remove the duplicated `normalize_fluent_isolates` function definitions from those other modules, mirroring the edit pattern shown here.
You may need to adjust module paths (`crate::common`, `crate::support::fluent`, etc.) to match your existing test support layout.
</issue_to_address>
### Comment 3
<location> `src/status.rs:10` </location>
<code_context>
- /// Emit a status line for the given pipeline stage.
- fn report_stage(&self, current: u32, total: u32, description: &str);
+/// Total count of user-visible pipeline stages.
+pub const PIPELINE_STAGE_COUNT: u32 = 6;
+
+/// Validation error when constructing a [`StageNumber`].
</code_context>
<issue_to_address>
**issue (complexity):** Consider simplifying the status-reporting types by removing unnecessary wrappers and error handling around stage indices and descriptions, especially in the `IndicatifReporter` path.
You can keep all the new functionality and significantly reduce the structural complexity by trimming the over-abstractions around stage indices and descriptions, especially in the `IndicatifReporter` path.
### 1. Drop `StageNumberError` and make `StageNumber` a thin, infallible wrapper
All current uses already assume valid indices (e.g. `PipelineStage::index` and `set_stage_state` deriving from a length-checked `Vec`). The fallible constructor just adds dead error branches and conversions.
You can keep the newtype (for clarity) but remove the error machinery and early returns:
```rust
#[derive(Debug, Copy, Clone, PartialEq, Eq, PartialOrd, Ord)]
pub struct StageNumber(u32);
impl StageNumber {
/// Build a stage number without runtime validation.
#[must_use]
pub const fn new_unchecked(value: u32) -> Self {
Self(value)
}
#[must_use]
pub const fn get(self) -> u32 {
self.0
}
}
pub const PIPELINE_STAGE_TOTAL: StageNumber =
StageNumber::new_unchecked(PIPELINE_STAGE_COUNT);
```
Then simplify the call sites that currently do redundant validation:
```rust
impl PipelineStage {
#[must_use]
pub const fn index(self) -> StageNumber {
StageNumber::new_unchecked(self as u32)
}
}
// In IndicatifReporter::set_stage_state:
fn set_stage_state(
state: &mut IndicatifState,
index: usize,
status_key: LocalizationKey,
finish_line: bool,
) {
let current = StageNumber::new_unchecked((index + 1) as u32);
let description = state
.descriptions
.get(index)
.cloned()
.unwrap_or_else(|| StageDescription::new(String::new()));
let message = stage_summary(status_key, current, PIPELINE_STAGE_TOTAL, &description);
// ... rest unchanged ...
}
```
This keeps the type-level clarity (`StageNumber`) but eliminates the unnecessary `Result` and early returns.
### 2. Avoid owning `StageDescription` in the trait and state where a `&str` is enough
Right now `StageDescription` is just a newtype over `String`, and `StatusReporter::report_stage` takes it by value, forcing allocations/clones into `IndicatifState.descriptions`.
You can keep the helper type for internal helpers (`stage_label`, `stage_summary`) but make the reporting path operate on `&str` and store plain `String` internally. That reduces type churn while preserving the new formatting helpers.
Change the trait to borrow the description:
```rust
pub trait StatusReporter {
fn report_stage(&self, current: StageNumber, total: StageNumber, description: &str);
fn report_complete(&self, tool_key: LocalizationKey);
}
```
Update the helpers to accept `&str` instead of `StageDescription`:
```rust
fn stage_label(current: StageNumber, total: StageNumber, description: &str) -> String {
localization::message(keys::STATUS_STAGE_LABEL)
.with_arg("current", current.get().to_string())
.with_arg("total", total.get().to_string())
.with_arg("description", description)
.to_string()
}
fn stage_summary(
state_key: LocalizationKey,
current: StageNumber,
total: StageNumber,
description: &str,
) -> String {
let state = localization::message(state_key.as_str()).to_string();
let label = stage_label(current, total, description);
localization::message(keys::STATUS_STAGE_SUMMARY)
.with_arg("state", state)
.with_arg("label", label)
.to_string()
}
```
Then, in `IndicatifReporter`, store `Vec<String>` instead of `Vec<StageDescription>` and avoid extra wrapping:
```rust
#[derive(Debug)]
struct IndicatifState {
progress: MultiProgress,
bars: Vec<ProgressBar>,
descriptions: Vec<String>,
running_index: Option<usize>,
completed: bool,
is_hidden: bool,
}
impl IndicatifReporter {
pub fn new() -> Self {
// ...
let mut descriptions = Vec::with_capacity(PipelineStage::ALL.len());
for stage in PipelineStage::ALL {
let description = stage.description(None);
let current = stage.index();
let bar = progress.add(ProgressBar::new(1));
bar.set_style(style.clone());
bar.set_message(stage_summary(
LocalizationKey::new(keys::STATUS_STATE_PENDING),
current,
PIPELINE_STAGE_TOTAL,
&description,
));
bars.push(bar);
descriptions.push(description);
}
// ...
}
fn set_stage_state(
state: &mut IndicatifState,
index: usize,
status_key: LocalizationKey,
finish_line: bool,
) {
let current = StageNumber::new_unchecked((index + 1) as u32);
let description = state
.descriptions
.get(index)
.map(|s| s.as_str())
.unwrap_or("");
let message = stage_summary(status_key, current, PIPELINE_STAGE_TOTAL, description);
// ... unchanged ...
}
}
impl StatusReporter for IndicatifReporter {
fn report_stage(&self, current: StageNumber, _total: StageNumber, description: &str) {
let Ok(index) = usize::try_from(current.get().saturating_sub(1)) else {
return;
};
let mut state = self
.state
.lock()
.unwrap_or_else(std::sync::PoisonError::into_inner);
if index >= state.bars.len() {
return;
}
if let Some(existing) = state.descriptions.get_mut(index) {
*existing = description.to_owned();
}
// ... rest unchanged ...
}
}
```
This keeps the new multi-line summaries and `indicatif` integration exactly as-is but flattens a lot of the wrapper types and conversions that the reviewer flagged, especially around stage indices and descriptions.
</issue_to_address>
### Comment 4
<location> `src/status_pipeline.rs:42` </location>
<code_context>
+
+ /// Map a raw stage index to a stage enum variant.
+ #[must_use]
+ pub const fn from_index(index: u32) -> Option<Self> {
+ match index {
+ 1 => Some(Self::ManifestIngestion),
</code_context>
<issue_to_address>
**issue (complexity):** Consider deriving the total stage count from the enum definition and dropping the unused index-mapping helper to streamline the pipeline stage API and its maintenance.
You can reduce some of the new complexity while keeping the centralization benefits by:
1. **Eliminating the duplicated “total stages” source of truth**
Instead of hard‑coding `PIPELINE_STAGE_TOTAL` in `status.rs`, derive it from `PipelineStage::ALL` in this module and re‑export it:
```rust
// pipeline_stage.rs
pub const PIPELINE_STAGE_TOTAL: u32 = PipelineStage::ALL.len() as u32;
```
Then in `status.rs` (or wherever it’s used), import from this module instead of defining a separate constant:
```rust
// status.rs
use crate::pipeline_stage::PIPELINE_STAGE_TOTAL;
```
This way, adding/removing a stage only requires touching `PipelineStage::ALL`.
2. **Removing the unused `from_index` helper for now**
`PipelineStage::from_index` isn’t used, so it’s just extra surface area to maintain and keep in sync with the enum:
```rust
impl PipelineStage {
// Remove this until you have a concrete caller:
// pub const fn from_index(index: u32) -> Option<Self> { ... }
}
```
You can always reintroduce it when there’s a real consumer, or generate it via a derive macro if you go that route later.
These two changes keep the centralized, localized stage descriptions and the reporting helper, but remove a duplicated constant and an unused mapping function.
</issue_to_address>
### Comment 5
<location> `src/manifest/mod.rs:121` </location>
<code_context>
+ from_str_named_with_stage_callback(yaml, name, stdlib_config, &mut no_op)
+}
+
+fn from_str_named_with_stage_callback<F>(
+ yaml: &str,
+ name: &ManifestName,
</code_context>
<issue_to_address>
**issue (complexity):** Consider collapsing the separate `*_with_stage_callback` and non-callback functions into a single implementation that takes an optional callback and exposes only one rich public API with a thin convenience wrapper.
You can keep all current behavior while simplifying the API surface and the generics by collapsing the dual function families into a single implementation that takes an optional callback, and only keeping the `*_with_stage_callback` variant public at the top level.
One concrete approach:
1. **Collapse `from_str_named` and `from_str_named_with_stage_callback` into one function with an optional callback**, using a trait object to avoid threading a generic type parameter:
```rust
fn from_str_named(
yaml: &str,
name: &ManifestName,
stdlib_config: Option<StdlibConfig>,
mut on_stage: Option<&mut dyn FnMut(ManifestLoadStage)>,
) -> Result<NetsukeManifest> {
if let Some(cb) = on_stage.as_deref_mut() {
cb(ManifestLoadStage::InitialYamlParsing);
}
let mut doc: ManifestValue = serde_saphyr::from_str(yaml).map_err(|e| /* ... */)?;
// ... Jinja setup ...
if let Some(cb) = on_stage.as_deref_mut() {
cb(ManifestLoadStage::TemplateExpansion);
}
register_manifest_macros(&doc, &mut jinja)?;
expand_foreach(&mut doc, &jinja)?;
if let Some(cb) = on_stage.as_deref_mut() {
cb(ManifestLoadStage::FinalRendering);
}
let manifest: NetsukeManifest = serde_json::from_value(doc).map_err(|e| /* ... */)?;
render_manifest(manifest, &jinja)
}
```
2. **Make only one “main” public API that exposes the callback**, and keep the no-callback version as a thin convenience wrapper that doesn’t introduce another generic:
```rust
pub fn from_path_with_policy(
path: impl AsRef<Path>,
policy: NetworkPolicy,
) -> Result<NetsukeManifest> {
from_path_with_policy_and_stage_callback(path, policy, None)
}
pub fn from_path_with_policy_and_stage_callback(
path: impl AsRef<Path>,
policy: NetworkPolicy,
mut on_stage: Option<&mut dyn FnMut(ManifestLoadStage)>,
) -> Result<NetsukeManifest> {
if let Some(cb) = on_stage.as_deref_mut() {
cb(ManifestLoadStage::ManifestIngestion);
}
let path_ref = path.as_ref();
let data = fs::read_to_string(path_ref).with_context(|| /* ... */)?;
let name = ManifestName::new(path_ref.display().to_string());
let config = stdlib_config_for_manifest(path_ref, policy)?;
from_str_named(&data, &name, Some(config), on_stage)
}
```
This way:
- Only one internal implementation (`from_str_named`) knows about stages.
- Only one public “rich” API (`from_path_with_policy_and_stage_callback`) carries the callback; the simple variant is a trivial wrapper.
- You eliminate the extra `_with_stage_callback` internal function and the extra generic `F: FnMut` plumbing, while preserving all existing stage emission points and behavior.
</issue_to_address>
### Comment 6
<location> `src/runner/mod.rs:14` </location>
<code_context>
use crate::localization::{self, keys};
use crate::output_mode::{self, OutputMode};
use crate::status::{
- AccessibleReporter, PipelineStage, SilentReporter, StatusReporter, report_pipeline_stage,
+ AccessibleReporter, IndicatifReporter, LocalizationKey, PipelineStage, SilentReporter,
</code_context>
<issue_to_address>
**issue (complexity):** Consider hiding `LocalizationKey` behind friendlier APIs and consolidating manifest loading and stage reporting into a single helper to keep `runner` simpler and more focused.
You can reduce the added complexity without losing functionality by:
1. **Hiding `LocalizationKey` at the runner boundary**
2. **Collapsing manifest-stage emission into a single helper**
---
### 1. Avoid `LocalizationKey` boilerplate at call sites
Right now `runner` has to construct `LocalizationKey` everywhere:
```rust
reporter.report_complete(LocalizationKey::new(keys::STATUS_TOOL_BUILD));
handle_ninja_tool(
cli,
"graph",
LocalizationKey::new(keys::STATUS_TOOL_GRAPH),
reporter,
);
let ninja = generate_ninja(
cli,
reporter,
Some(LocalizationKey::new(keys::STATUS_TOOL_BUILD)),
)?;
```
You can keep the new `LocalizationKey`-based machinery but make the *signature* friendlier so `runner` mostly deals with `&'static str`.
**Concrete change:**
In the status/localization layer, introduce `From<&'static str>` and generic params on the reporter API:
```rust
// status/localization side
impl From<&'static str> for LocalizationKey {
fn from(s: &'static str) -> Self {
LocalizationKey::new(s)
}
}
pub trait StatusReporter {
fn report_complete(&self, key: impl Into<LocalizationKey>);
// if needed:
fn report_stage(
&self,
stage: PipelineStage,
key: Option<impl Into<LocalizationKey>>,
);
}
```
Then update the `runner` side to pass literals/consts and only convert at the boundary:
```rust
fn handle_build(cli: &Cli, args: &BuildArgs, reporter: &dyn StatusReporter) -> Result<()> {
let ninja = generate_ninja(cli, reporter, Some(keys::STATUS_TOOL_BUILD))?;
// ...
reporter.report_complete(keys::STATUS_TOOL_BUILD);
Ok(())
}
fn handle_ninja_tool(
cli: &Cli,
tool: &str,
tool_key: &'static str,
reporter: &dyn StatusReporter,
) -> Result<()> {
let ninja = generate_ninja(cli, reporter, Some(tool_key))?;
// ...
reporter.report_complete(tool_key);
Ok(())
}
fn handle_clean(cli: &Cli, reporter: &dyn StatusReporter) -> Result<()> {
handle_ninja_tool(cli, "clean", keys::STATUS_TOOL_CLEAN, reporter)
}
```
And in `generate_ninja` you do the conversion once:
```rust
fn generate_ninja(
cli: &Cli,
reporter: &dyn StatusReporter,
tool_key: Option<&'static str>,
) -> Result<NinjaContent> {
let manifest_path = resolve_manifest_path(cli)?;
// ...
report_pipeline_stage(
reporter,
PipelineStage::NinjaSynthesisAndExecution,
tool_key.map(LocalizationKey::from),
);
// ...
}
```
This keeps all the new progress behavior but removes repeated `LocalizationKey::new(...)` noise and the need for `LocalizationKey` in every runner signature.
---
### 2. Make manifest stage reporting single-responsibility
Currently manifest handling is split between:
- `generate_ninja`
- `ensure_manifest_exists_or_error`
- `load_manifest_with_stage_reporting`
- plus the `manifest` module’s callback
You can fold the existence check and stage reporting into `load_manifest_with_stage_reporting` so `generate_ninja` has one clear entry point.
**Concrete refactor:**
```rust
fn load_manifest_with_stage_reporting(
cli: &Cli,
manifest_path: &Utf8PathBuf,
policy: crate::stdlib::NetworkPolicy,
reporter: &dyn StatusReporter,
) -> Result<crate::ast::NetsukeManifest> {
// Existence check + ManifestIngestion stage in one place
if !manifest_path.as_std_path().exists() {
report_pipeline_stage(reporter, PipelineStage::ManifestIngestion, None);
// existing error construction moved here from ensure_manifest_exists_or_error
let manifest_name = manifest_path.file_name().ok_or_else(|| {
anyhow!(
"{}",
localization::message(keys::RUNNER_MANIFEST_PATH_MISSING_NAME)
.with_arg("path", manifest_path.as_str())
)
})?.to_owned();
let directory = if cli.directory.is_some() {
let parent = manifest_path
.parent()
.map_or_else(|| manifest_path.as_str(), camino::Utf8Path::as_str);
localization::message(keys::RUNNER_MANIFEST_DIRECTORY)
.with_arg("directory", parent)
.to_string()
} else {
localization::message(keys::RUNNER_MANIFEST_CURRENT_DIRECTORY).to_string()
};
let message = localization::message(keys::RUNNER_MANIFEST_NOT_FOUND)
.with_arg("manifest_name", manifest_name.as_str())
.with_arg("directory", &directory);
return Err(RunnerError::ManifestNotFound {
manifest_name,
directory,
path: manifest_path.to_path_buf().into_std_path_buf(),
message,
help: localization::message(keys::RUNNER_MANIFEST_NOT_FOUND_HELP),
}
.into());
}
// Normal staged load path
manifest::from_path_with_policy_and_stage_callback(
manifest_path.as_std_path(),
policy,
|stage| match stage {
manifest::ManifestLoadStage::ManifestIngestion => {
report_pipeline_stage(reporter, PipelineStage::ManifestIngestion, None);
}
manifest::ManifestLoadStage::InitialYamlParsing => {
report_pipeline_stage(reporter, PipelineStage::InitialYamlParsing, None);
}
manifest::ManifestLoadStage::TemplateExpansion => {
report_pipeline_stage(reporter, PipelineStage::TemplateExpansion, None);
}
manifest::ManifestLoadStage::FinalRendering => {
report_pipeline_stage(reporter, PipelineStage::FinalRendering, None);
}
},
)
.with_context(|| {
localization::message(keys::RUNNER_CONTEXT_LOAD_MANIFEST)
.with_arg("path", manifest_path.as_str())
})
}
```
Then `generate_ninja` becomes simpler and is the only place that calls this helper:
```rust
fn generate_ninja(
cli: &Cli,
reporter: &dyn StatusReporter,
tool_key: Option<&'static str>,
) -> Result<NinjaContent> {
let manifest_path = resolve_manifest_path(cli)?;
let policy = cli
.network_policy()
.context(localization::message(keys::RUNNER_CONTEXT_NETWORK_POLICY))?;
let manifest = load_manifest_with_stage_reporting(cli, &manifest_path, policy, reporter)?;
if tracing::enabled!(tracing::Level::DEBUG) {
// unchanged debug block
}
report_pipeline_stage(reporter, PipelineStage::IrGenerationValidation, None);
let graph = BuildGraph::from_manifest(&manifest)
.context(localization::message(keys::RUNNER_CONTEXT_BUILD_GRAPH))?;
report_pipeline_stage(
reporter,
PipelineStage::NinjaSynthesisAndExecution,
tool_key.map(LocalizationKey::from),
);
let ninja = ninja_gen::generate(&graph)
.context(localization::message(keys::RUNNER_CONTEXT_GENERATE_NINJA))?;
Ok(NinjaContent::new(ninja))
}
```
This preserves:
- The new manifest progress stages
- The more detailed error for missing manifests
- The tool-stage reporting
while making `runner` easier to read and reducing the amount of `LocalizationKey` wiring it has to carry.
</issue_to_address>
### Comment 7
<location> `src/runner/mod.rs:97` </location>
<code_context>
- let reporter: Box<dyn StatusReporter> = match mode {
- OutputMode::Accessible => Box::new(AccessibleReporter),
- OutputMode::Standard => Box::new(SilentReporter),
+ let reporter: Box<dyn StatusReporter> = match (mode, cli.progress.unwrap_or(true)) {
+ (OutputMode::Accessible, _) => Box::new(AccessibleReporter),
+ (OutputMode::Standard, true) => Box::new(IndicatifReporter::new()),
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This three-way match on `(mode, cli.progress.unwrap_or(true))` should be extracted into a predicate/helper to keep the conditional out of the main flow.
The instructions ask to move conditionals with more than two branches into a predicate function. Here you have three branches over `(mode, cli.progress.unwrap_or(true))`.
Consider extracting the reporter selection into a small helper, e.g.:
```rust
fn make_reporter(mode: OutputMode, progress: Option<bool>) -> Box<dyn StatusReporter> {
match (mode, progress.unwrap_or(true)) {
(OutputMode::Accessible, _) => Box::new(AccessibleReporter),
(OutputMode::Standard, true) => Box::new(IndicatifReporter::new()),
(OutputMode::Standard, false) => Box::new(SilentReporter),
}
}
```
and then call `let reporter = make_reporter(mode, cli.progress);` from `run` to stay within the guideline.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.rs`
**Instructions:**
Move conditionals with >2 branches into a predicate function.
</details>
</issue_to_address>
### Comment 8
<location> `docs/users-guide.md:608` </location>
<code_context>
+
+In standard mode, Netsuke uses persistent progress summaries for the six-stage
+pipeline. The summaries are localized and remain visible as stages complete.
+When stderr is not a TTY (for example, in CI logs), Netsuke emits deterministic
+summary lines instead of animated redraws.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym “CI” is used without being defined on first use.
Consider expanding this to something like “continuous integration (CI) logs)” the first time it appears.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 9
<location> `docs/users-guide.md:611` </location>
<code_context>
+When stderr is not a TTY (for example, in CI logs), Netsuke emits deterministic
+summary lines instead of animated redraws.
+
+Progress summaries are enabled by default in standard mode. You can force them
+on or off through layered configuration:
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This sentence uses the second-person pronoun “you”, which the style guidelines forbid.
Maybe rephrase as “Progress summaries are enabled by default in standard mode and can be forced on or off through layered configuration.”
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we").
</details>
</issue_to_address>
### Comment 10
<location> `docs/users-guide.md:606` </location>
<code_context>
-animated progress indicators for standard mode terminals.
+### Real-time progress summaries
+
+In standard mode, Netsuke uses persistent progress summaries for the six-stage
+pipeline. The summaries are localized and remain visible as stages complete.
+When stderr is not a TTY (for example, in CI logs), Netsuke emits deterministic
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Subsequent text on this feature uses “localized”, which does not follow en-GB spelling (“localised”).
Please update “localized”/“localization” in this section (for example, “The summaries are localized…”, “localized state prefixes”) to “localised”/“localisation” to match en-GB spelling.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 11
<location> `docs/users-guide.md:598` </location>
<code_context>
+Stage 1/6: Reading manifest file
+Stage 2/6: Parsing YAML document
+Stage 3/6: Expanding template directives
+Stage 4/6: Deserializing and rendering manifest values
+Stage 5/6: Building and validating dependency graph
+Stage 6/6: Synthesizing Ninja plan and executing Build
</code_context>
<issue_to_address>
**suggestion (review_instructions):** “Deserializing” uses US spelling; en-GB would be “deserialising”.
The same change is needed in the later example block as well to keep spelling consistent across the document.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 12
<location> `docs/users-guide.md:600` </location>
<code_context>
+Stage 3/6: Expanding template directives
+Stage 4/6: Deserializing and rendering manifest values
+Stage 5/6: Building and validating dependency graph
+Stage 6/6: Synthesizing Ninja plan and executing Build
Build complete.
```
</code_context>
<issue_to_address>
**suggestion (review_instructions):** “Synthesizing” uses US spelling; en-GB would be “synthesising”.
There is another occurrence in the later example block; that one should be updated as well.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 13
<location> `docs/netsuke-design.md:2043` </location>
<code_context>
enumeration in unit tests and via Cucumber steps for behavioural coverage.
+Real-time stage reporting now uses a six-stage model in `src/status.rs` backed
+by `indicatif::MultiProgress` for standard terminals. The reporter keeps one
+persistent summary line per stage and updates each line through localized state
+labels (`pending`, `in progress`, `done`, `failed`) plus localized stage text.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** Later in this paragraph “localized state labels” uses US spelling instead of en-GB (“localised”).
Please change “localized state labels” to “localised state labels” to align with the required en-GB spelling.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 14
<location> `docs/execplans/3-9-1-integrate-indicatif-multi-progress.md:7` </location>
<code_context>
+`Risks`, `Progress`, `Surprises & Discoveries`, `Decision Log`, and
+`Outcomes & Retrospective` must be kept up to date as work proceeds.
+
+Status: DONE
+
+No `PLANS.md` file exists in this repository.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** This document repeatedly uses “localized”/“localization”, which do not follow en-GB spelling (“localised”/“localisation”).
For example: “localized labels”, “localized help text”, “Localization keys”, “localization tests”, and similar should be updated to “localised”/“localisation” throughout this ExecPlan.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-GB-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 15
<location> `docs/execplans/3-9-1-integrate-indicatif-multi-progress.md:118` </location>
<code_context>
+ inside one function, so stage-level reporting needs extraction.
+- Standard mode currently uses `SilentReporter`; no `indicatif` dependency is
+ present.
+- No project-memory MCP resources were available in this environment during
+ planning, so repository docs were used as the authoritative source.
+- The runtime `manifest` command path needed an explicit completion call after
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym “MCP” is introduced without being defined.
Consider expanding this to something like “Model Context Protocol (MCP) resources” at first mention.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>
### Comment 16
<location> `docs/execplans/3-9-1-integrate-indicatif-multi-progress.md:47` </location>
<code_context>
+- Localize all newly introduced user-facing strings in both:
+ `locales/en-US/messages.ftl` and `locales/es-ES/messages.ftl`.
+- Add/adjust unit tests using `rstest`.
+- Add/adjust behavioural tests using `rstest-bdd` v0.5.0.
+- Cover happy paths, unhappy paths, and edge cases.
+- Update `docs/users-guide.md` for user-visible behaviour changes.
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The acronym “BDD” is used without being defined on first use in this document.
Suggest spelling this out on first mention, for example “behaviour-driven development (BDD) tests”.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Define uncommon acronyms on first use.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![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: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
docs/execplans/3-8-1-add-accessible-output-mode.md (1)
175-194:⚠️ Potential issue | 🟡 MinorWrap long bullet lines to 80 columns.
Reflow the bullets in the Outcomes section to comply with wrapping rules.As per coding guidelines, "Markdown paragraphs and bullet points must be wrapped at 80 columns."Proposed fix
-- The `resolve_with` dependency-injection pattern for environment variable - lookup made both unit tests and BDD tests clean and deterministic. -- The `OutputMode` enum and `StatusReporter` trait provide a clean - abstraction that future spinner work (roadmap 3.9) can plug into. -- The plan's staged approach (A: detection, B: CLI, C: status, D: testing) - worked well with checkpoint validation between stages. +- The `resolve_with` dependency-injection pattern for environment variable + lookup made both unit tests and BDD tests clean and deterministic. +- The `OutputMode` enum and `StatusReporter` trait provide a clean + abstraction that future spinner work (roadmap 3.9) can plug into. +- The plan's staged approach (A: detection, B: CLI, C: status, D: testing) + worked well with checkpoint validation between stages. @@ -- `clippy::print_stderr` is denied globally, requiring `writeln!(io::stderr())` - instead of `eprintln!`. The `drop()` wrapper follows the `main.rs` pattern. -- The BDD `Given the environment variable` step already existed in - `manifest/mod.rs` and sets real process env vars. Using "simulated" - env vars in the TestWorld avoided test interference. +- `clippy::print_stderr` is denied globally, requiring `writeln!(io::stderr())` + instead of `eprintln!`. The `drop()` wrapper follows the `main.rs` pattern. +- The BDD `Given the environment variable` step already existed in + `manifest/mod.rs` and sets real process env vars. Using "simulated" + env vars in the TestWorld avoided test interference. @@ -- 4 new files created, 10 files modified. -- 12 unit tests (output_mode), 7 BDD scenarios (accessible_output). -- ~300 net new lines of code (well within the 800-line tolerance). +- 4 new files created, 10 files modified. +- 12 unit tests (output_mode), 7 BDD scenarios (accessible_output). +- ~300 net new lines of code (well within the 800-line tolerance).src/runner/mod.rs (1)
1-419:⚠️ Potential issue | 🟠 MajorSplit this module to bring it under 400 lines.
Decompose
src/runner/mod.rsby moving helper types and/or manifest-stage reporting helpers into submodules so the file length cap is met.
🤖 Fix all issues with AI agents
In `@Cargo.toml`:
- Line 30: The Cargo.toml currently pins the indicatif crate to "0.17.11";
update the dependency declaration for indicatif to the latest stable release
"0.18.3" (replace indicatif = "0.17.11" with indicatif = "0.18.3"), then run
cargo update / cargo build to ensure the new version resolves and tests compile;
verify any API changes in code that references indicatif
(symbols/types/functions used) and adjust calls if necessary.
In `@docs/execplans/3-9-1-integrate-indicatif-multi-progress.md`:
- Around line 334-346: Replace the indented command examples with fenced code
blocks that include a language identifier: wrap the first gate sequence (lines
containing "set -o pipefail" and the three make commands: "make check-fmt",
"make lint", "make test") in a ```shell ... ``` block, and wrap the
documentation-gates sequence (the second "set -o pipefail" and "make fmt", "make
markdownlint", "make nixie") in its own ```shell ... ``` block; ensure there are
no stray leading "+" characters and use 'shell' as the language identifier (use
'plaintext' only if any block is non-command text).
- Around line 37-40: The stage name list uses non-Oxford spelling for the
deserialisation stage; update the six-stage pipeline string that currently reads
"Manifest Ingestion, Initial YAML Parsing, Template Expansion, Deserialisation &
Final Rendering, IR Generation & Validation, and Ninja Synthesis & Execution" to
use the -ize spelling ("Deserialization & Final Rendering") so it conforms to
the en-GB-oxendict guideline; search for the exact stage phrase and replace
"Deserialisation" with "Deserialization" wherever the six-stage pipeline is
defined or documented.
In `@docs/netsuke-design.md`:
- Around line 2042-2051: The paragraph describing the new progress reporter is
not wrapped to 80 columns; reflow the text in docs/netsuke-design.md so each
line is <=80 characters while preserving the original content and terminology
(keep references to src/status.rs, indicatif::MultiProgress, the six-stage
model, localized state labels `pending`/`in progress`/`done`/`failed`, stderr
TTY fallback, accessible output behavior, and configuration via OrthoConfig with
`progress: Option<bool>` / `--progress` / `NETSUKE_PROGRESS`). Ensure the
paragraph remains a single Markdown paragraph and follows the project wrapping
rules.
In `@docs/users-guide.md`:
- Around line 606-616: Rewrite the paragraph that begins "Progress summaries are
enabled by default in standard mode." to remove second‑person pronouns and wrap
text to 80 columns; keep the meaning and retain the configuration examples (CLI
flag `--progress true`/`false`, env `NETSUKE_PROGRESS=true`, config `progress =
true`), phrasing it in neutral/impersonal voice (e.g., "Progress summaries are
enabled by default. They can be forced on or off via layered configuration:
...") and ensure all lines in the paragraph and bullet points do not exceed 80
characters.
In `@src/manifest/tests/stages.rs`:
- Around line 19-62: Extract the duplicated tempdir + manifest path setup into
an rstest fixture by adding a function annotated with #[fixture] that creates a
tempfile::TempDir and returns (TempDir, PathBuf) for the Netsukefile path; then
convert the two tests stage_callback_reports_expected_order_for_valid_manifest
and stage_callback_stops_after_parse_failure to #[rstest] and accept the fixture
as a parameter, keeping each test's specific std::fs::write call (the file
contents) inside the test body; ensure you still call
manifest::from_path_with_policy_and_stage_callback with the fixture's PathBuf
and update imports to include rstest::{rstest, fixture} as needed.
In `@src/runner/mod.rs`:
- Around line 109-119: The doc/example for generate_ninja is stale: update every
example call site (e.g., the snippet shown around generate_ninja usage and other
occurrences at lines referenced) to include the new third parameter tool_key
(pass None if you don't have one) so the example signature matches fn
generate_ninja(cli: &Cli, reporter: &dyn StatusReporter, tool_key:
Option<LocalizationKey>) -> Result<NinjaContent>; locate uses of generate_ninja
in docs and tests and change calls like generate_ninja(&cli, &SilentReporter) to
generate_ninja(&cli, &SilentReporter, None).
In `@src/status_pipeline.rs`:
- Around line 6-53: Add a compile-time check to prevent the enum variants,
PipelineStage::ALL, PIPELINE_STAGE_COUNT/PIPELINE_STAGE_TOTAL, and from_index()
from drifting: define a const PIPELINE_STAGE_COUNT: usize =
PipelineStage::ALL.len() and then add a const equality assertion like const
_PIPELINE_STAGE_CHECK: [(); PIPELINE_STAGE_TOTAL as usize] = [();
PIPELINE_STAGE_COUNT]; also change from_index() (or add a small wrapper) to rely
on PIPELINE_STAGE_TOTAL/PIPELINE_STAGE_COUNT (e.g., reject indices >
PIPELINE_STAGE_COUNT) so the match/create logic and the ALL array length are
statically tied together.
In `@src/status_tests.rs`:
- Around line 39-41: In the test function
pipeline_stage_count_matches_stage_array replace the
u32::try_from(...).unwrap_or(0) call with
u32::try_from(PipelineStage::ALL.len()).expect(...) so the conversion failure
surfaces in test output; update the expect message to something descriptive
(e.g., "failed to convert PipelineStage::ALL.len() to u32") and keep the rest of
the assertion against PIPELINE_STAGE_COUNT unchanged.
In `@src/status.rs`:
- Around line 124-143: Clippy warns about drop_non_drop where
drop(writeln!(...)) is used; update each occurrence to use let _ = writeln!(...)
instead so write errors are ignored without calling drop on a Result.
Specifically, in AccessibleReporter::report_stage and
AccessibleReporter::report_complete replace drop(writeln!(io::stderr(),
"{message}")) with let _ = writeln!(io::stderr(), "{message}"), and make the
same change for the other similar methods in this file (the other report_*
blocks noted in the comment) to eliminate the lint.
- Around line 250-269: The Drop impl for IndicatifReporter contains a no-op
borrow `let _ = &state.progress;` (and a similar occurrence later) that should
be removed or explicitly justified; update the drop path in the Impl Drop for
IndicatifReporter to either delete these accidental no-op borrows or replace
them with a short comment explaining the required side-effect (e.g., keeping
borrow alive for ordering or triggering destructor) so the intent is
clear—locate the `drop` method on `IndicatifReporter` and the comparable block
around the other occurrence and remove the meaningless `let _ =
&state.progress;` lines or add a one-line comment explaining why the borrow must
remain.
In `@tests/bdd/helpers/assertions.rs`:
- Around line 10-20: Make normalize_fluent_isolates public (pub fn
normalize_fluent_isolates) in the assertions helper and delete the duplicate
implementations elsewhere; then update the other modules that currently
reimplement it (the manifest_command and localization test modules) to import
and call the single exported normalize_fluent_isolates instead of their local
copies. Ensure the original signature and behavior are preserved and adjust
imports (use ...) in those modules to reference the helper’s
normalize_fluent_isolates.
In `@tests/bdd/steps/manifest_command.rs`:
- Around line 32-37: The test file defines a local normalize_fluent_isolates
function that duplicates the helper in tests/bdd/helpers/assertions.rs; remove
the local function and import/reuse the shared helper instead (export the helper
from assertions.rs if it isn't public) so tests use the single canonical
normalize_fluent_isolates implementation; ensure the test module references the
helper by its exported name and delete the duplicate function in
manifest_command.rs.
Refactor status reporting interfaces and implementations to replace the StageDescription wrapper with plain &str or String parameters. This simplifies the pipeline stage reporting code, removes unnecessary wrapper types, and improves clarity. Impacts include: - Remove StageDescription struct and associated error type. - Change StatusReporter trait methods to accept &str for stage description. - Update implementations (AccessibleReporter, SilentReporter, IndicatifReporter) accordingly. - Adjust PipelineStage::description to return String directly. - Simplify related test code. This refactor retains previous behavior but cleans up types and improves interface consistency for pipeline stage status reporting. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docs/users-guide.md`:
- Around line 606-623: The documentation uses US spelling and incorrect acronym
casing: replace the word "localized" with British en-GB spelling "localised" in
the standard-mode progress paragraph and the state-prefix line, and change
"continuous integration (CI)" to "Continuous Integration (CI)" (ensuring the
acronym is expanded on first use exactly as "Continuous Integration (CI)").
Update any other occurrences of "localized" or lowercase "continuous
integration" in the shown standard-mode section so they follow en-GB-oxendict
spelling and proper acronym casing.
Owner
Author
|
@coderabbitai Have the following now been resolved? |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain
🏁 Script executed: Length of output: 1139 🏁 Script executed: Length of output: 337 🏁 Script executed: Length of output: 558 🏁 Script executed: Length of output: 538 🏁 Script executed: Length of output: 292 🏁 Script executed: Length of output: 446 🏁 Script executed: Length of output: 469 🏁 Script executed: Length of output: 850 🏁 Script executed: Length of output: 667 🏁 Script executed: Length of output: 47 🏁 Script executed: Length of output: 545 🏁 Script executed: Length of output: 809 🏁 Script executed: Length of output: 188 🏁 Script executed: Length of output: 297 🏁 Script executed: Length of output: 233 🏁 Script executed: Length of output: 585 🏁 Script executed: Length of output: 1309
✅ Resolved (10 items)Fully resolved:
Partially resolved: Overall comment 1 (normalize_fluent_isolates duplication): ✅ Resolved ❌ Not Resolved (8 items)Code structure:
Documentation spelling (en-GB required): Overall comment 2 (stage indexing sources of truth): Partially addressed but not fully resolved due to Summary: Significant progress on code structure (especially test coverage and consolidation), but all en-GB spelling issues remain unresolved in documentation files, and a few complexity-reduction suggestions (reporter helper, manifest API collapse, PIPELINE_STAGE_TOTAL location) have not been implemented. |
Moved manifest path resolution, output path resolution, and manifest existence validation into a new module `runner::path_helpers` to improve code organization and separation of concerns. Additionally, streamlined runner module by delegating these responsibilities to the helper functions. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
leynos
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/status_pipeline.rs`:
- Around line 37-38: Update the doc comment for the enum variant
NinjaSynthesisAndExecution to use en-GB spelling: change "synthesize" to
"synthesise" so the comment reads "Stage 6: synthesise Ninja and execute command
intent." Keep the identifier NinjaSynthesisAndExecution unchanged; only edit the
preceding comment text.
…ath string conversion - Changed map_string_or_list from using str::to_string to str::to_owned to avoid unnecessary allocations. - Updated get_target_display_name to use explicit closure and Utf8PathBuf to_string call for clarity. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
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
Changes
docs/execplans/3-9-1-integrate-indicatif-multi-progress.md(now aligned with implemented six-stage model).IndicatifReporter) and wired it into the runner when standard output is active and progress is enabled.PipelineStageandManifestLoadStagewith persistent stage lines.--progressflag and integrated OrthoConfig precedence for progress rendering.indicatif = 0.17.11to Cargo.toml and corresponding lock entries.Rationale
Plan of work (high level)
progresssetting and localized CLI/help keys.indicatif::MultiProgressstandard reporter; preserve accessible reporter for accessibility mode.rstest).rstest-bdd) for standard/accessible paths and localization.Acceptance criteria
Risks & mitigations
Validation and evidence
Rollback / recovery
--progresswith a guarded default.Context and references
indicatif.docs/netsuke-design.md,docs/users-guide.md, and existing3-8-1accessibility plan.◳ Generated by DevBoxer ◰
Generated by DevBoxer
📎 Task: https://www.devboxer.com/task/0e25281d-067a-4ec0-b836-5b8b4f47f10d
Summary by Sourcery
Implement a localized six-stage pipeline progress model with an Indicatif-based standard reporter and configurable progress output across CLI, runner, and manifest loading.
New Features:
--progress), and environment, with accessible mode taking precedence.Enhancements:
Build:
indicatifcrate dependency for multi-progress rendering.Documentation:
Tests:
📎 Task: https://www.devboxer.com/task/b559494e-78a2-4194-8a26-6cb42cb93d1c