Support serial dependency ordering for actions and targets (#552) - #557
Open
lodyai[bot] wants to merge 19 commits into
Open
Support serial dependency ordering for actions and targets (#552)#557lodyai[bot] wants to merge 19 commits into
lodyai[bot] wants to merge 19 commits into
Conversation
Contributor
Reviewer's GuideAdds manifest-level dependency_order support, threads it through IR to Ninja generation, and implements staged Ninja dyndep bundles plus atomic sidecar materialization so serial dependency lists run in declaration order while preserving a single Ninja scheduler and parallel behaviour for other branches. Sequence diagram for serial dependency Ninja bundle generation and executionsequenceDiagram
actor User
participant Runner as runner.generate_ninja
participant NinjaGen as ninja_gen.generate_bundle
participant Dyndep as process.materialize_dyndep_files
participant Ninja
User->>Runner: netsuke build / clean / generate
Runner->>NinjaGen: generate_bundle(graph)
NinjaGen-->>Runner: GeneratedNinja (build_file, dyndep_files)
Runner->>Dyndep: materialize_dyndep_files(cli, bundle.dyndep_files())
Dyndep-->>Runner: dyndep sidecars materialized
Runner->>Ninja: invoke with bundle.build_file()
Ninja-->>User: serial deps run in order, parallel elsewhere
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.
Keep local VT Code state out of version control.
Record the staged dyndep design, generated sidecar contract, and atomic materialization boundary for ordered target and action dependencies. Define regression evidence for declaration order, shared work, failure short-circuiting, default parallelism, and serialization scope. Document the independent-reachability limit that requires approval before implementation.
Introduce the closed `DependencyOrder::{Parallel, Serial}` enum on
`Target` and carry it on every `BuildEdge`, defaulting to parallel so
existing manifests and generated Ninja remain unchanged. Copy the value
during manifest-to-IR lowering while preserving the declaration order of
`implicit_deps`, and re-export it through the `ir` module.
Cover the new surface with regressions: omission defaults to parallel,
explicit parallel and serial parse on targets and actions, unknown values
such as `sequential` are rejected, and serial targets and actions retain
their order and policy through lowering. Update every direct `BuildEdge`
literal, doctest, and fixture to compile against the widened struct.
Update the execplan progress and discoveries with the validated Ninja
dyndep loading and path-escaping semantics.
Introduce a generated-bundle API so a `serial` `deps` list can be expressed as a single Ninja invocation. `generate_bundle` lowers each multi-dependency serial edge into one phony gate and content-addressed dyndep sidecar per dependency under the reserved `.netsuke/serial` and `.netsuke/dyndep` namespaces, emits `ninja_required_version = 1.10` only when gates exist, and keeps ordinary parallel graphs byte-for-byte identical to string-only generation. String-only `generate`/`generate_into` now refuse serial graphs with a localized `DyndepFilesRequired` error before writing any output, and a user output that collides with the reserved namespace is rejected. Add localization keys for both errors across every shipped catalogue and split `src/ninja_gen.rs` into a directory module so no file exceeds the repository line ceiling. Cover the staging rules with unit and integration tests: sidecar count and ordering, version-floor gating, the no-sidecar cases for parallel and single-element serial lists, per-stage sidecars for repeated dependencies, and the reserved-path error.
Add `src/runner/process/dyndep_files.rs`, which writes each generated sidecar beneath `.netsuke/dyndep` in the effective Ninja working directory through a capability-scoped handle: a same-directory `create_new` temporary file, flushed and synced, and an atomic rename. An existing file is verified and reused; mismatched bytes are reported as corruption and a concurrent writer that wins the race is treated as success only when the surviving content matches. Route `generate_ninja` through `generate_bundle` and materialize the bundle before every build, clean, and generate invocation writes or invokes the main Ninja file, so a serial manifest never reaches Ninja without its sidecars present. Add localization keys for the materializer errors across all 35 catalogues, with U+200F direction marks in the RTL locales. Add real-Ninja runtime tests proving strict declaration order and failure short-circuiting, plus materializer unit tests for creation, reuse, corruption detection, and temporary-file cleanup. The full suite (1930 tests) passes, and an end-to-end serial build over a real manifest observed `fmt, lint, test, all` order.
Collect the per-edge sidecar list and seen-path set in a single SerialStages value so render_serial_block no longer threads five parameters and cannot diverge from the caller's bundle accumulator.
Propagate bundle formatting errors, apply capability-scoped test staging, and split focused test cases so the serial-dependency implementation meets the repository's strict Clippy and Whitaker contracts.
Keep sidecar-materialization fixtures within their temporary directory capability so they satisfy the repository filesystem policy.
Update the approved ExecPlan with implementation and validation findings, and remove surplus trailing whitespace from the affected Fluent catalogues.
lodyai
Bot
force-pushed
the
issue-552-support-serial-dependency-ordering-for-actions-and-targets
branch
from
August 11, 2026 21:48
e1cef57 to
7ed4cc8
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
|
@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/dyndep_files.rs Comment on lines +114 to +163 fn write_atomic(dir: &Dir, rel: &Utf8Path, content: &str) -> Result<()> {
let temp = unique_temp_name(rel);
let mut options = OpenOptions::new();
options.write(true).create_new(true);
let mut file = match dir.open_with(&temp, &options) {
Ok(file) => file,
Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => {
// Another process won the race for our temporary name; verify the
// final path and treat matching content as success.
return match read_verified(dir, rel, content)? {
ReadOutcome::Matching => Ok(()),
ReadOutcome::Mismatch => Err(anyhow!(
localization::message(keys::RUNNER_IO_DYNDEP_CORRUPT)
.with_arg("path", rel.as_str())
)),
ReadOutcome::Missing => Err(anyhow!(
localization::message(keys::RUNNER_IO_DYNDEP_RACE)
.with_arg("path", rel.as_str())
)),
};
}
Err(err) => {
return Err(err).with_context(|| {
localization::message(keys::RUNNER_IO_DYNDEP_WRITE).with_arg("path", rel.as_str())
});
}
};
file.write_all(content.as_bytes()).with_context(|| {
localization::message(keys::RUNNER_IO_DYNDEP_WRITE).with_arg("path", rel.as_str())
})?;
file.flush().with_context(|| {
localization::message(keys::RUNNER_IO_DYNDEP_WRITE).with_arg("path", rel.as_str())
})?;
file.sync_all().with_context(|| {
localization::message(keys::RUNNER_IO_DYNDEP_WRITE).with_arg("path", rel.as_str())
})?;
// Rename is relative to the same directory; `rename` replaces an existing
// destination, so if another process already wrote the final file, the
// atomic replace yields content identical to ours.
if let Err(err) = dir.rename(&temp, dir, rel) {
// The final file may have appeared via a concurrent writer; verify it.
if read_verified(dir, rel, content)? != ReadOutcome::Matching {
return Err(err).with_context(|| {
localization::message(keys::RUNNER_IO_DYNDEP_RENAME).with_arg("path", rel.as_str())
});
}
drop(dir.remove_file(&temp));
}
Ok(())
}❌ New issue: Bumpy Road Ahead |
Owner
|
@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 +123 to +135 fn parallel_edges_produce_no_sidecars() -> Result<()> {
let graph = graph_with_edge(parallel_edge("all", &["dep1", "dep2"]))?;
let bundle = generate_bundle(&graph)?;
ensure!(
!bundle.build_file().contains("ninja_required_version"),
"parallel bundle must not emit a version floor"
);
ensure!(
bundle.dyndep_files().is_empty(),
"parallel graph must produce no sidecars"
);
Ok(())
}❌ New issue: Code Duplication |
Owner
|
@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 +156 to +230 pub fn generate_bundle(graph: &BuildGraph) -> Result<GeneratedNinja, NinjaGenError> {
reject_reserved_paths(graph)?;
let serial_present = graph_requires_dyndep(graph);
let mut out = String::new();
if serial_present {
writeln!(out, "ninja_required_version = 1.10\n")?;
}
let mut actions: Vec<_> = graph.actions.iter().collect();
actions.sort_by_key(|(id, _)| *id);
for (id, action) in actions {
use crate::ninja_gen::NamedAction;
writeln!(out, "{}", NamedAction { id, action })?;
}
let mut edges: Vec<_> = graph.targets.values().collect();
edges.sort_by_key(|a| path_key(&a.explicit_outputs));
let mut seen: HashSet<String> = HashSet::new();
let mut stages = SerialStages::default();
for edge in edges {
let key = path_key(&edge.explicit_outputs);
if !seen.insert(key.clone()) {
continue;
}
let action =
graph
.actions
.get(&edge.action_id)
.ok_or_else(|| NinjaGenError::MissingAction {
id: edge.action_id.clone(),
message: localization::message(keys::NINJA_GEN_MISSING_ACTION)
.with_arg("id", &edge.action_id),
})?;
let requires_gates =
edge.dependency_order == DependencyOrder::Serial && edge.implicit_deps.len() > 1;
if requires_gates {
let mut added = Vec::new();
render_serial_block(edge, &mut out, &mut stages, &mut added)?;
let mut aggregate = edge.clone();
aggregate.implicit_deps = added;
aggregate.dependency_order = DependencyOrder::Parallel;
writeln!(
out,
"{}",
crate::ninja_gen::DisplayEdge {
edge: &aggregate,
action_restat: action.restat,
}
)?;
} else {
writeln!(
out,
"{}",
crate::ninja_gen::DisplayEdge {
edge,
action_restat: action.restat,
}
)?;
}
}
if !graph.default_targets.is_empty() {
let mut defs = graph.default_targets.clone();
defs.sort();
writeln!(out, "default {}", join(&defs))?;
}
Ok(GeneratedNinja {
build_file: out,
dyndep_files: stages.dyndep_files,
})
}❌ New issue: Complex Method |
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.
leynos
marked this pull request as ready for review
August 11, 2026 21:58
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Sorry @leynos, your pull request is larger than the review limit of 150000 diff characters
![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: 7ed4cc838e
ℹ️ 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
+37
to
+38
| let dir = open_effective_dir(cli)?; | ||
| dir.create_dir_all(DYNDEP_DIR).with_context(|| { |
There was a problem hiding this comment.
When a manifest has only parallel dependencies, dyndep_files is empty, but every build, clean, and generate path still opens the working directory and creates .netsuke/dyndep. Consequently, previously valid operations such as generating Ninja output from a read-only project directory now fail even though they require no dyndep artefacts. Return early when the slice is empty so existing parallel manifests retain their prior filesystem requirements.
Useful? React with 👍 / 👎.
Owner
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '== Module outline =='
ast-grep outline src/runner/process/dyndep_files.rs --items all --view expanded
printf '%s\n' '== materialize_dyndep_files implementation =='
rg -n -A80 -B8 '^\s*pub\s+fn\s+materialize_dyndep_files\s*\(' src/runner/process/dyndep_files.rs
printf '%s\n' '== Typed call sites and surrounding control flow =='
rg -n -A12 -B12 '\bmaterialize_dyndep_files\s*\(' --glob '*.rs' .
printf '%s\n' '== PR discussion thread =='
gh api graphql \
-F owner='leynos' \
-F name='netsuke' \
-F number=557 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 20) {
nodes {
id
body
url
author { login }
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .url == "https://github.com/leynos/netsuke/pull/557#discussion_r3762028952"))
| {id, isResolved, comments}'Length of output: 16320
@leynos, this comment is not resolved.
The codegraph shows that src/runner/mod.rs always calls materialize_dyndep_files. In src/runner/process/dyndep_files.rs, Line 37 opens the effective directory and Line 38 creates .netsuke/dyndep before the loop. An empty dyndep_files slice still performs these operations.
Use this AI agent prompt:
Update `src/runner/process/dyndep_files.rs`.
Add an early successful return at the start of `materialize_dyndep_files` when `dyndep_files.is_empty()`.
Place the guard before `open_effective_dir(cli)` and before `create_dir_all(DYNDEP_DIR)`. This must preserve the previous behaviour for parallel-only manifests: do not open the working directory and do not create `.netsuke/dyndep` when no dyndep sidecars exist.
Add a focused Rust unit test that passes an empty slice and verifies that the function does not create `.netsuke/dyndep`. Use the existing test helpers and project lint rules. Update the function documentation if its error behaviour changes.
You are interacting with an AI system.
Comment on lines
+120
to
+123
| Err(err) if err.kind() == std::io::ErrorKind::AlreadyExists => { | ||
| // Another process won the race for our temporary name; verify the | ||
| // final path and treat matching content as success. | ||
| return match read_verified(dir, rel, content)? { |
There was a problem hiding this comment.
When two first-time builds generate the same sidecar concurrently, both choose the fixed <digest>.dd.tmp name; the loser of create_new immediately checks the final path while the winner may still be writing, sees it missing, and fails with the race error. An interrupted writer also leaves this fixed temporary file behind and permanently causes the same failure on later runs. Use a genuinely process-unique temporary name, or safely recover and retry collisions, so concurrent builds and crash recovery work as intended.
Useful? React with 👍 / 👎.
Owner
There was a problem hiding this comment.
@coderabbitai Has this now been resolved in the latest commit?
Use codegraph analysis to determine your answer.
If this comment is now resolved, please mark it as such using the API. Otherwise, please provide an AI agent prompt for the remaining work to be done to address this comment.
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '=== Current PR revision ==='
gh pr view 557 --repo leynos/netsuke --json headRefOid --jq '.headRefOid'
echo '=== Module outline ==='
ast-grep outline src/runner/process/dyndep_files.rs --items all --view expanded
echo '=== Structural map: write_atomic and temporary-name helpers ==='
ast-grep run --lang rust \
--pattern 'fn write_atomic($$$) -> $RET { $$$ }' \
src/runner/process/dyndep_files.rs
ast-grep run --lang rust \
--pattern 'fn unique_temp_name($$$) -> $RET { $$$ }' \
src/runner/process/dyndep_files.rs
echo '=== Focused source slices ==='
rg -n -A100 -B10 'fn (write_atomic|unique_temp_name)\b' src/runner/process/dyndep_files.rs
echo '=== Call sites and definitions in the dyndep graph ==='
ast-grep run --lang rust \
--pattern 'unique_temp_name($$$)' \
src/runner/process/dyndep_files.rs
ast-grep run --lang rust \
--pattern 'materialize_dyndep_files($$$)' \
src
echo '=== Review-thread lookup ==='
gh api graphql \
-F owner='leynos' \
-F name='netsuke' \
-F number=557 \
-f query='
query($owner: String!, $name: String!, $number: Int!) {
repository(owner: $owner, name: $name) {
pullRequest(number: $number) {
reviewThreads(first: 100) {
nodes {
id
isResolved
comments(first: 20) {
nodes {
author { login }
body
url
}
}
}
}
}
}
}' \
--jq '.data.repository.pullRequest.reviewThreads.nodes[]
| select(any(.comments.nodes[]; .url == "https://github.com/leynos/netsuke/pull/557#discussion_r3762028956"))
| {id, isResolved, comments}'Length of output: 15359
@leynos, keep this thread open. The latest commit does not resolve the finding.
unique_temp_name() still returns the fixed path <sidecar-name>.tmp. When this path exists and the final sidecar is absent, handle_temp_name_collision() returns RUNNER_IO_DYNDEP_RACE. This failure still occurs for concurrent first-time writers and stale temporary files.
Use this AI agent prompt for the remaining work:
Fix the concurrent temporary-file collision in `src/runner/process/dyndep_files.rs`.
Problem:
- `unique_temp_name(rel)` currently returns the deterministic `<sidecar-name>.tmp` path.
- `create_temp_file()` uses `create_new(true)`.
- If another process is writing that temporary file, or a previous process left it behind, `handle_temp_name_collision()` checks the final sidecar.
- If the final sidecar is not present yet, the code returns `RUNNER_IO_DYNDEP_RACE`.
Required changes:
1. Generate a genuinely unique same-directory temporary path for each write attempt. Do not use only the sidecar filename plus a fixed suffix.
2. Keep the temporary path under the final sidecar’s parent directory so `Dir::rename()` remains atomic.
3. Preserve `create_new(true)` so a temporary write cannot truncate another file.
4. On a rename failure, verify the final sidecar content. If it matches, remove the exact temporary path created by the current invocation. Pass that path into the cleanup function; do not regenerate a temporary name.
5. Preserve the existing corruption check. A final sidecar with different content must still fail.
6. Add focused tests for:
- an existing stale temporary file with no final sidecar does not block materialization;
- two separately generated temporary names for the same final sidecar differ;
- a matching final sidecar still succeeds when another temporary file exists.
7. Update the temporary-name documentation. Do not describe deterministic names as unique.
8. Keep the module documentation and Rust lint rules compliant.
You are interacting with an AI system.
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.
Describe dependency_order for actions and targets, including its path-scoped serial execution guarantee, Ninja version requirement, and generated-state behaviour. Record the dyndep architecture decision and exercise the complete documented manifest through the documentation suite.
Capture the final deterministic gate evidence and the zero-finding CodeRabbit review so the Issue 552 ExecPlan remains a complete handoff.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Extract the shared no-staging assertion while retaining separate parallel and single-element serial test cases.
Keep the Issue 552 ExecPlan current with the requested test-only deduplication, commit, and validation evidence.
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.
Extract temporary creation, writing, and rename-race handling while preserving the existing atomic write and concurrent-writer protocol.
Keep the Issue 552 ExecPlan current with the atomic-write helper extraction, collision regression, and required gate results.
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.
Move sorted edge rendering and serial edge lowering out of generate_bundle while preserving the generated Ninja bundle and error behaviour.
Keep the Issue 552 ExecPlan current with the generate_bundle complexity reduction and its unchanged-output validation evidence.
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.
Return before opening the effective directory when no generated sidecars exist, so parallel-only manifests neither require the working directory nor create `.netsuke/dyndep`. Pass the existing root `dylint.toml` policy to Whitaker explicitly. This preserves its documented narrow capability exemptions and makes `make lint` reproducible.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Use distinct same-directory temporary names and retry protected creation collisions, so stale or concurrent temporary files cannot block sidecar materialization. Carry the exact created path through rename-race cleanup to avoid deleting another writer’s temporary sidecar.
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.
Collaborator
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
|
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:
Implement staged Ninja dyndep support
WalkthroughSerial dependency ordering is now configurable for actions and targets through ChangesSerial dependency ordering
Sequence Diagram(s)sequenceDiagram
participant Manifest
participant BuildGraph
participant NinjaGenerator
participant Runner
participant Ninja
Manifest->>BuildGraph: propagate dependency_order
BuildGraph->>NinjaGenerator: provide serial dependency graph
NinjaGenerator->>Runner: return Ninja build file and dyndep sidecars
Runner->>Runner: materialise sidecars atomically
Runner->>Ninja: invoke one Ninja build
Ninja->>Ninja: execute dependencies through staged gates
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (4 errors, 4 warnings, 6 inconclusive)
✅ Passed checks (6 passed)
📋 Issue PlannerLet us write the prompt for your AI agent so you can ship faster (with fewer bugs). View plan for ticket: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 30
🤖 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/adr-010-use-ninja-dyndep-for-serial-dependency-ordering.md`:
- Line 114: Update the user contract link text in the ADR to “user's guide”
while preserving its existing destination and surrounding formatting.
- Around line 25-47: Add a concise Y-statement to the ADR-010 Decision section
covering the context, forces, decision, and accepted consequences of staged
Ninja dyndep serial ordering. Preserve the existing paragraphs as supporting
implementation rationale and do not alter their technical details.
- Line 9: Update the ADR date in the document metadata from “2026-08-11.” to the
bare YYYY-MM-DD value “2026-08-11”, removing the trailing punctuation.
In
`@docs/execplans/issue-552-support-serial-dependency-ordering-for-actions-and-targets.md`:
- Around line 798-801: Remove the machine-specific absolute filesystem path from
the plan’s command example, replacing it with <repository-root> or omitting the
path while keeping the commands relative to the repository root.
- Around line 185-186: Update the test-results sentence in the execution plan to
replace “739 lib + touched integration tests pass” with clear grammatical prose
stating that 739 library tests plus the touched integration tests passed.
In `@docs/netsuke-design.md`:
- Around line 2056-2071: Update the BuildEdge IR example around the Rust snippet
and class diagram to represent the dependency_order policy used by the staged
serial-ordering section. If dependency_order is a BuildEdge field, add it
consistently to both representations; otherwise, document the actual field or
graph structure that carries this policy and align references to
BuildEdge.dependency_order accordingly.
In `@docs/users-guide.md`:
- Around line 384-385: Update the Netsuke documentation sentence to state that
generated builds containing staged serial ordering require Ninja 1.10 or newer,
replacing the ambiguous phrase “generated builds containing one” while
preserving the surrounding dyndep and dependency details.
In `@locales/ar/messages.ftl`:
- Around line 101-106: Translate all added dyndep diagnostic values in
locales/ar/messages.ftl, locales/cs/messages.ftl, locales/cy/messages.ftl,
locales/da/messages.ftl, locales/de/messages.ftl, locales/th/messages.ftl, and
locales/tr/messages.ftl at lines 101-106, and apply the same locale-specific
translations to each catalogue’s corresponding lines 171-172. Preserve every
Fluent key, the { $path } placeholder, and inline command names.
In `@locales/el/messages.ftl`:
- Around line 102-107: Translate the six runner.io.dyndep.* messages and two
ninja_gen.* messages in locales/el/messages.ftl (102-107, 172-173),
locales/es-419/messages.ftl (102-107, 172-173), locales/es-ES/messages.ftl
(102-107, 172-173), locales/fa/messages.ftl (102-107, 172-173), and
locales/fi/messages.ftl (102-107, 172-173) into their respective locales.
Preserve every Fluent key and { $path } placeholder exactly, retain technical
identifiers such as dyndep, Ninja, and command names as appropriate, and ensure
Greek wording places { $path } in a nominative sentence position.
In `@locales/en-GB/messages.ftl`:
- Around line 101-106: Update the en-GB catalogue entries to use British
spelling: change “finalize” to “finalise” in runner.io.dyndep.rename and change
“materialized” to “materialised” in the referenced entry around the
materialization messages.
In `@locales/fr/messages.ftl`:
- Around line 102-107: Translate all eight added diagnostic messages in each
affected catalogue: locales/fr/messages.ftl lines 102-107 and 172-173,
locales/gd/messages.ftl lines 101-106 and 171-172, locales/he/messages.ftl lines
101-106 and 171-172, and locales/hi/messages.ftl lines 101-106 and 171-172.
Preserve every Fluent key and the { $path } placeholder while translating the
six runner.io.dyndep.* values and two ninja_gen.* values; no direct changes are
needed elsewhere.
In `@locales/hu/messages.ftl`:
- Around line 101-106: Translate the new dyndep diagnostic values while
preserving every Fluent key and { $path } placeholder: in
locales/hu/messages.ftl lines 101-106 and 171-172 use Hungarian; in
locales/id/messages.ftl lines 101-106 and 171-172 use Indonesian; in
locales/it/messages.ftl lines 102-107 and 172-173 use Italian; and in
locales/ja/messages.ftl lines 101-106 and 171-172 use Japanese.
In `@locales/ko/messages.ftl`:
- Around line 101-106: Translate all newly added English diagnostics in
locales/ko/messages.ftl lines 101-106 and 171-172 into Korean,
locales/nb/messages.ftl lines 101-106 and 171-172 into Norwegian Bokmål, and
locales/nl/messages.ftl lines 101-106 and 171-172 into Dutch. Preserve every
Fluent key, {$path} placeholder, formatting, and command name exactly.
In `@locales/pl/messages.ftl`:
- Around line 101-107: Translate the new dyndep diagnostic values into each
target language while preserving all Fluent keys and the `{ $path }` variable:
update locales/pl/messages.ftl lines 101-107 in Polish,
locales/pt-BR/messages.ftl lines 102-107 in Brazilian Portuguese,
locales/pt-PT/messages.ftl lines 102-107 in European Portuguese,
locales/ro/messages.ftl lines 101-106 in Romanian, locales/ru/messages.ftl lines
101-106 in Russian, and locales/sv/messages.ftl lines 101-106 in Swedish.
In `@locales/uk/messages.ftl`:
- Around line 101-106: Translate every newly added dyndep diagnostic value while
preserving the existing Fluent keys and { $path } placeholders: in
locales/uk/messages.ftl lines 101-106 and 171-172 use Ukrainian; in
locales/vi/messages.ftl lines 101-106 and 171-172 use Vietnamese; in
locales/zh-Hans/messages.ftl lines 100-105 and 170-171 use Simplified Chinese;
and in locales/zh-Hant/messages.ftl lines 100-105 and 170-171 use Traditional
Chinese.
In `@src/manifest/render.rs`:
- Line 156: Update assert_rendered_target to assert that the rendered target
preserves DependencyOrder::Parallel, and add a serial case if the test is
intended to cover both enum variants. Ensure the render_manifest test fails when
dependency_order is changed or omitted.
In `@src/ninja_gen/dyndep_tests.rs`:
- Around line 179-192: Expand reserved path coverage in
reserved_output_namespace_is_rejected by parameterizing the test across
explicit_outputs, implicit_outputs, inputs, implicit_deps, and order_only_deps,
ensuring each case retains a valid non-reserved output for graph_with_edge. Add
a negative case using a path such as .netsuke-extra/x and verify it is accepted,
preserving the prefix-boundary behavior.
In `@src/ninja_gen/dyndep.rs`:
- Around line 144-148: Align generate_bundle’s action and edge emission with
generate_into by replacing the writeln! calls around NamedAction and DisplayEdge
with write!, since those display implementations already append their own
newlines. Add an equivalence test for a parallel-only graph asserting
generate_bundle(&graph).build_file() matches generate(&graph)? exactly.
- Around line 224-225: Consolidate the duplicated serial-edge predicate by
exposing one shared edge_requires_gates(edge: &BuildEdge) -> bool and removing
the local requires_gates definition and duplicate graph_requires_dyndep
implementation in src/ninja_gen/dyndep.rs. Update graph_requires_dyndep in both
modules to call the shared predicate, preserving the existing serial dependency
and implicit-dependency count conditions.
- Around line 133-134: Complete the truncated documentation sentence above the
sidecar fixture by restoring the missing intra-doc link after “rather than
through.” Use the appropriate existing symbol for the bundle-construction path
referenced by the surrounding documentation.
In `@src/ninja_gen/mod.rs`:
- Around line 233-251: Update escape_ninja_path to reject paths containing the
pipe character before generating Ninja output, rather than emitting the
unsupported $| sequence. Preserve the existing escaping behavior for spaces,
dollars, and colons, and ensure callers handle the rejection appropriately.
In `@src/ninja_gen/tests.rs`:
- Around line 1-6: Remove the duplicated “Unit tests for Ninja file generation
and rule synthesis.” module documentation line in the tests module header,
preserving the single original doc line and the remaining comments.
In `@src/runner/process/dyndep_files.rs`:
- Around line 59-69: The non-UTF-8 working-directory error in open_effective_dir
uses an untranslated literal. Add a dedicated localization key alongside
RUNNER_IO_OPEN_AMBIENT_DIR, define its Fluent string in every
locales/*/messages.ftl file, and replace the literal context with
localization::message using the new key.
- Around line 122-126: Update write_atomic and its helper error paths to remove
the temporary file whenever writing, syncing, or renaming fails. Ensure
handle_rename_failure’s existing matching-content cleanup does not cause a
double removal; centralize cleanup or guard that path while preserving the
current successful rename behavior.
- Around line 129-163: Bound the retry loop in create_unique_temp_file with a
finite attempt limit and return a clear error when all candidate names collide,
while preserving successful creation and existing I/O error propagation. In
create_temp_file, replace the handle_temp_name_collision() call with the direct
None result and remove the now-unused helper function.
- Around line 317-328: Update the no_temp_files_left_behind test around
materialize_dyndep_files to enumerate the dyndep directory and assert that no
entry name ends with ".tmp", rather than checking the nonexistent fixed
filename. After cleanup is implemented, add a failing-write case using the same
directory scan to verify temporary files are also removed on errors.
In `@tests/ir_from_manifest_tests.rs`:
- Around line 380-402: Update serial_dependency_order_survives_lowering to
accept a third case parameter containing the expected dependency list, then
assert edge.implicit_deps matches it alongside dependency_order and phony.
Extend each serial test case with the expected declaration-ordered dependencies,
preserving the existing assertions.
In `@tests/ninja_gen_integration_tests.rs`:
- Around line 272-360: Extract reusable rstest fixtures or small constructor
helpers for the repeated Action and BuildEdge literals in
serial_graph_rejected_by_string_only_generation and
bundle_generation_for_serial_graph_materializes_sidecars, parameterizing
command, implicit dependencies, and dependency order as needed. Follow the
existing action, serial_edge, and parallel_edge pattern in dyndep_tests.rs, and
keep the integration test file below the enforced 400-line limit.
In `@tests/serial_dependency_runtime_tests.rs`:
- Around line 1-7: Add a real Ninja runtime test alongside the existing ordering
and short-circuiting tests, using the bundle-generation and filesystem-marker
patterns already present in the file. Define two serial aggregates that depend
on the same shared target, run the encompassing build, and assert the shared
target’s command appends to the log exactly once, proving reuse across serial
lists.
- Line 17: Update the Ninja process-spawn error handling in the serial
dependency runtime tests to distinguish ErrorKind::NotFound from other failures,
explicitly report that ninja must be available on PATH, and preserve test
failure when it is missing. Keep other spawn errors’ existing context and
behavior unchanged.
🪄 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: 88cd048c-68a2-4244-9d43-2cb28ee8320a
📒 Files selected for processing (72)
.gitignoreMakefiledocs/adr-010-use-ninja-dyndep-for-serial-dependency-ordering.mddocs/contents.mddocs/developers-guide.mddocs/execplans/issue-552-support-serial-dependency-ordering-for-actions-and-targets.mddocs/netsuke-design.mddocs/repository-layout.mddocs/roadmap.mddocs/users-guide.mdlocales/ar/messages.ftllocales/cs/messages.ftllocales/cy/messages.ftllocales/da/messages.ftllocales/de/messages.ftllocales/el/messages.ftllocales/en-GB/messages.ftllocales/en-US/messages.ftllocales/es-419/messages.ftllocales/es-ES/messages.ftllocales/fa/messages.ftllocales/fi/messages.ftllocales/fr/messages.ftllocales/gd/messages.ftllocales/he/messages.ftllocales/hi/messages.ftllocales/hu/messages.ftllocales/id/messages.ftllocales/it/messages.ftllocales/ja/messages.ftllocales/ko/messages.ftllocales/nb/messages.ftllocales/nl/messages.ftllocales/pl/messages.ftllocales/pt-BR/messages.ftllocales/pt-PT/messages.ftllocales/ro/messages.ftllocales/ru/messages.ftllocales/sv/messages.ftllocales/th/messages.ftllocales/tr/messages.ftllocales/uk/messages.ftllocales/vi/messages.ftllocales/zh-Hans/messages.ftllocales/zh-Hant/messages.ftlsrc/ast.rssrc/graph_view/tests_support.rssrc/ir/cycle_issue322_property_tests.rssrc/ir/cycle_property_tests.rssrc/ir/cycle_tests.rssrc/ir/cycle_verification.rssrc/ir/from_manifest.rssrc/ir/graph.rssrc/ir/mod.rssrc/localization/keys.rssrc/manifest/render.rssrc/ninja_gen/dyndep.rssrc/ninja_gen/dyndep_tests.rssrc/ninja_gen/mod.rssrc/ninja_gen/tests.rssrc/ninja_gen_property_tests.rssrc/runner/mod.rssrc/runner/process/dyndep_files.rssrc/runner/process/mod.rstests/ast_tests.rstests/ast_tests/dependency_order.rstests/documentation_examples_tests.rstests/ir_from_manifest_tests.rstests/ir_tests.rstests/ninja_gen_integration_tests.rstests/ninja_gen_unit_tests.rstests/serial_dependency_runtime_tests.rs
|
|
||
| ## Date | ||
|
|
||
| 2026-08-11. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use a bare ADR date.
Change 2026-08-11. to 2026-08-11.
Triage: [type:docstyle]
Based on learnings: ADR dates must use bare YYYY-MM-DD values without trailing punctuation.
🤖 Prompt for 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.
In `@docs/adr-010-use-ninja-dyndep-for-serial-dependency-ordering.md` at line 9,
Update the ADR date in the document metadata from “2026-08-11.” to the bare
YYYY-MM-DD value “2026-08-11”, removing the trailing punctuation.
Source: Learnings
Comment on lines
+25
to
+47
| ## Decision | ||
|
|
||
| In the context of a `dependency_order: serial` manifest `deps` list, Netsuke | ||
| will use staged Ninja dyndep sidecars to reveal one direct dependency at a time | ||
| and will materialize those sidecars atomically beneath `.netsuke/dyndep` before | ||
| writing or invoking the main build file. | ||
|
|
||
| `dependency_order` is a closed `parallel`/`serial` enum on the shared action | ||
| and target AST shape. It is copied to `BuildEdge`, where it remains a logical | ||
| graph annotation. Only the Ninja generator lowers a serial list containing two | ||
| or more direct dependencies into synthetic phony gates beneath | ||
| `.netsuke/serial` and content-addressed dyndep sidecars beneath | ||
| `.netsuke/dyndep`. | ||
|
|
||
| The main generated build file declares `ninja_required_version = 1.10` only | ||
| when staged serial lowering is present. The generator exposes a complete bundle | ||
| containing main-file text and every required sidecar. String-only generation | ||
| rejects a graph requiring sidecars instead of returning an incomplete file. | ||
|
|
||
| The serial guarantee is deliberately path-scoped: each direct dependency in the | ||
| annotated list becomes schedulable only after its predecessor succeeds. A later | ||
| dependency independently reachable through another requested path remains free | ||
| to run through that other path. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add the required Y-statement.
Add a concise Y-statement in the Decision section. State the context, forces, decision, and accepted consequences. Keep the existing implementation details as supporting rationale.
Triage: [type:docstyle]
The ExecPlan requires ADR-010 to include a Y-statement.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~42-~42: Possible missing comma found.
Context: ...ly generation rejects a graph requiring sidecars instead of returning an incomplete file...
(AI_HYDRA_LEO_MISSING_COMMA)
🤖 Prompt for 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.
In `@docs/adr-010-use-ninja-dyndep-for-serial-dependency-ordering.md` around lines
25 - 47, Add a concise Y-statement to the ADR-010 Decision section covering the
context, forces, decision, and accepted consequences of staged Ninja dyndep
serial ordering. Preserve the existing paragraphs as supporting implementation
rationale and do not alter their technical details.
| [`src/ninja_gen/dyndep.rs`](../src/ninja_gen/dyndep.rs) | ||
| - Atomic sidecar materialization: | ||
| [`src/runner/process/dyndep_files.rs`](../src/runner/process/dyndep_files.rs) | ||
| - User contract: [users guide](users-guide.md#run-direct-dependencies-serially) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the guide link text.
Change [users guide] to [user's guide].
Triage: [type:grammar] [type:docstyle]
Static analysis flagged the missing apostrophe.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~114-~114: It seems likely that a singular genitive (’s) apostrophe is missing.
Context: ...cess/dyndep_files.rs) - User contract: [users guide](users-guide.md#run-direct-depend...
(AI_HYDRA_LEO_APOSTROPHE_S_XS)
🤖 Prompt for 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.
In `@docs/adr-010-use-ninja-dyndep-for-serial-dependency-ordering.md` at line 114,
Update the user contract link text in the ADR to “user's guide” while preserving
its existing destination and surrounding formatting.
Source: Linters/SAST tools
Comment on lines
+185
to
+186
| and fixture to compile. `cargo check --all-targets` and 739 lib + touched | ||
| integration tests pass. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the test-count wording.
Replace 739 lib + touched integration tests pass. with clear prose such as 739 library tests plus the touched integration tests passed.
Triage: [type:grammar] [type:docstyle]
Static analysis flagged this wording as grammatically unclear.
🧰 Tools
🪛 LanguageTool
[grammar] ~185-~185: After the number ‘739’, use a plural noun. Did you mean “libs”?
Context: ...le. cargo check --all-targets and 739 lib + touched integration tests pass. - [...
(CD_NNU)
🤖 Prompt for 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.
In
`@docs/execplans/issue-552-support-serial-dependency-ordering-for-actions-and-targets.md`
around lines 185 - 186, Update the test-results sentence in the execution plan
to replace “739 lib + touched integration tests pass” with clear grammatical
prose stating that 739 library tests plus the touched integration tests passed.
Source: Linters/SAST tools
Comment on lines
+798
to
+801
| ```plaintext | ||
| /home/leynos/.lody/repos/github---leynos---netsuke/worktrees/ | ||
| 6c498022-7fb6-49a9-94a9-56723bb7d1e1 | ||
| ``` |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Remove the machine-specific absolute path.
The committed plan exposes /home/leynos/... and is not portable to another checkout. Replace it with <repository-root> or omit the path. Keep the commands relative to the repository root.
The path exposes a developer identifier and local filesystem layout.
🤖 Prompt for 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.
In
`@docs/execplans/issue-552-support-serial-dependency-ordering-for-actions-and-targets.md`
around lines 798 - 801, Remove the machine-specific absolute filesystem path
from the plan’s command example, replacing it with <repository-root> or omitting
the path while keeping the commands relative to the repository root.
Comment on lines
+317
to
+328
| #[test] | ||
| fn no_temp_files_left_behind() -> Result<()> { | ||
| let temp = tempfile::tempdir()?; | ||
| let cli = temp_cli(temp.path()); | ||
| materialize_dyndep_files(&cli, &[sidecar(".netsuke/dyndep/x.dd", "content")])?; | ||
| let temp_file = ".netsuke/dyndep/x.dd.tmp"; | ||
| ensure!( | ||
| temp_dir(&temp)?.open(temp_file).is_err(), | ||
| "temp file left behind" | ||
| ); | ||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the vacuous assertion: this test checks a filename that is never produced.
unique_temp_name at line 232 builds {name}.{process_id}.{sequence}.tmp, so the temporary file for x.dd is named x.dd.<pid>.<seq>.tmp. Line 322 checks .netsuke/dyndep/x.dd.tmp, which the code never creates. The assertion at lines 323-326 therefore succeeds regardless of whether a temporary file was left behind, and it does not detect the leak described on write_atomic.
Enumerate the directory and assert that no entry ends with .tmp.
🧪 Proposed fix
#[test]
fn no_temp_files_left_behind() -> Result<()> {
let temp = tempfile::tempdir()?;
let cli = temp_cli(temp.path());
materialize_dyndep_files(&cli, &[sidecar(".netsuke/dyndep/x.dd", "content")])?;
- let temp_file = ".netsuke/dyndep/x.dd.tmp";
- ensure!(
- temp_dir(&temp)?.open(temp_file).is_err(),
- "temp file left behind"
- );
+ let leftovers: Vec<String> = temp_dir(&temp)?
+ .read_dir(DYNDEP_DIR)?
+ .filter_map(|entry| entry.ok()?.file_name().ok())
+ .filter(|name| name.ends_with(".tmp"))
+ .collect();
+ ensure!(
+ leftovers.is_empty(),
+ "temporary files left behind: {leftovers:?}"
+ );
Ok(())
}Extend the same check to a failing-write case once cleanup is added, so the guard covers the error path too.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[test] | |
| fn no_temp_files_left_behind() -> Result<()> { | |
| let temp = tempfile::tempdir()?; | |
| let cli = temp_cli(temp.path()); | |
| materialize_dyndep_files(&cli, &[sidecar(".netsuke/dyndep/x.dd", "content")])?; | |
| let temp_file = ".netsuke/dyndep/x.dd.tmp"; | |
| ensure!( | |
| temp_dir(&temp)?.open(temp_file).is_err(), | |
| "temp file left behind" | |
| ); | |
| Ok(()) | |
| } | |
| #[test] | |
| fn no_temp_files_left_behind() -> Result<()> { | |
| let temp = tempfile::tempdir()?; | |
| let cli = temp_cli(temp.path()); | |
| materialize_dyndep_files(&cli, &[sidecar(".netsuke/dyndep/x.dd", "content")])?; | |
| let leftovers: Vec<String> = temp_dir(&temp)? | |
| .read_dir(DYNDEP_DIR)? | |
| .filter_map(|entry| entry.ok()?.file_name().ok()) | |
| .filter(|name| name.ends_with(".tmp")) | |
| .collect(); | |
| ensure!( | |
| leftovers.is_empty(), | |
| "temporary files left behind: {leftovers:?}" | |
| ); | |
| Ok(()) | |
| } |
🤖 Prompt for 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.
In `@src/runner/process/dyndep_files.rs` around lines 317 - 328, Update the
no_temp_files_left_behind test around materialize_dyndep_files to enumerate the
dyndep directory and assert that no entry name ends with ".tmp", rather than
checking the nonexistent fixed filename. After cleanup is implemented, add a
failing-write case using the same directory scan to verify temporary files are
also removed on errors.
Comment on lines
+380
to
+402
| fn serial_dependency_order_survives_lowering( | ||
| #[case] yaml: &str, | ||
| #[case] output: &str, | ||
| #[case] expected_phony: bool, | ||
| ) -> Result<()> { | ||
| let manifest = manifest::from_str(yaml)?; | ||
| let graph = BuildGraph::from_manifest(&manifest).context("expected graph generation")?; | ||
| let edge = graph | ||
| .targets | ||
| .get(&Utf8PathBuf::from(output)) | ||
| .with_context(|| format!("expected edge for {output}"))?; | ||
| ensure!( | ||
| edge.dependency_order == DependencyOrder::Serial, | ||
| "expected serial dependency order for {output}, got {:?}", | ||
| edge.dependency_order | ||
| ); | ||
| ensure!( | ||
| edge.phony == expected_phony, | ||
| "unexpected phony flag for {output}: {}", | ||
| edge.phony | ||
| ); | ||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Assert declaration order in the serial cases.
This test checks the dependency_order enum and the phony flag, but not implicit_deps. Declaration order is the property the feature exists to preserve, and parallel_dependency_order_lowering_is_default at lines 424-428 already asserts it for the parallel path. A lowering change that reorders or de-duplicates a serial deps list passes this test.
Add the expected dependency list as a third case parameter.
🧪 Proposed addition
-), "all", false)]
+), "all", false, &["check-fmt", "lint", "test"])]-), "gate", true)]
+), "gate", true, &["fmt", "clippy"])]
fn serial_dependency_order_survives_lowering(
#[case] yaml: &str,
#[case] output: &str,
#[case] expected_phony: bool,
+ #[case] expected_deps: &[&str],
) -> Result<()> { ensure!(
edge.phony == expected_phony,
"unexpected phony flag for {output}: {}",
edge.phony
);
+ let expected: Vec<Utf8PathBuf> = expected_deps.iter().map(Utf8PathBuf::from).collect();
+ ensure!(
+ edge.implicit_deps == expected,
+ "declaration order must survive lowering for {output}: {:?}",
+ edge.implicit_deps
+ );
Ok(())
}🤖 Prompt for 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.
In `@tests/ir_from_manifest_tests.rs` around lines 380 - 402, Update
serial_dependency_order_survives_lowering to accept a third case parameter
containing the expected dependency list, then assert edge.implicit_deps matches
it alongside dependency_order and phony. Extend each serial test case with the
expected declaration-ordered dependencies, preserving the existing assertions.
Comment on lines
+272
to
+360
| #[rstest] | ||
| fn serial_graph_rejected_by_string_only_generation() -> Result<()> { | ||
| let action = Action { | ||
| recipe: Recipe::Command { | ||
| command: "echo done".into(), | ||
| }, | ||
| description: None, | ||
| depfile: None, | ||
| deps_format: None, | ||
| pool: None, | ||
| restat: false, | ||
| }; | ||
| let edge = BuildEdge { | ||
| action_id: "a".into(), | ||
| inputs: Vec::new(), | ||
| implicit_deps: vec![Utf8PathBuf::from("dep1"), Utf8PathBuf::from("dep2")], | ||
| dependency_order: netsuke::ast::DependencyOrder::Serial, | ||
| explicit_outputs: vec![Utf8PathBuf::from("all")], | ||
| implicit_outputs: Vec::new(), | ||
| order_only_deps: Vec::new(), | ||
| phony: false, | ||
| always: false, | ||
| }; | ||
| let mut graph = BuildGraph::default(); | ||
| graph.actions.insert("a".into(), action); | ||
| graph.targets.insert(Utf8PathBuf::from("all"), edge); | ||
|
|
||
| let mut out = String::new(); | ||
| let err = generate_into(&graph, &mut out) | ||
| .err() | ||
| .context("serial graph must be rejected by string-only generation")?; | ||
| ensure!( | ||
| matches!(err, NinjaGenError::DyndepFilesRequired { .. }), | ||
| "expected DyndepFilesRequired, got {err:?}" | ||
| ); | ||
| ensure!( | ||
| out.is_empty(), | ||
| "string-only generation must not write partial output" | ||
| ); | ||
| Ok(()) | ||
| } | ||
|
|
||
| #[rstest] | ||
| fn bundle_generation_for_serial_graph_materializes_sidecars() -> Result<()> { | ||
| let action = Action { | ||
| recipe: Recipe::Command { | ||
| command: "echo done".into(), | ||
| }, | ||
| description: None, | ||
| depfile: None, | ||
| deps_format: None, | ||
| pool: None, | ||
| restat: false, | ||
| }; | ||
| let edge = BuildEdge { | ||
| action_id: "a".into(), | ||
| inputs: Vec::new(), | ||
| implicit_deps: vec![Utf8PathBuf::from("check-fmt"), Utf8PathBuf::from("test")], | ||
| dependency_order: netsuke::ast::DependencyOrder::Serial, | ||
| explicit_outputs: vec![Utf8PathBuf::from("all")], | ||
| implicit_outputs: Vec::new(), | ||
| order_only_deps: Vec::new(), | ||
| phony: false, | ||
| always: false, | ||
| }; | ||
| let mut graph = BuildGraph::default(); | ||
| graph.actions.insert("a".into(), action); | ||
| graph.targets.insert(Utf8PathBuf::from("all"), edge); | ||
|
|
||
| let bundle = netsuke::ninja_gen::generate_bundle(&graph)?; | ||
| ensure!( | ||
| bundle | ||
| .build_file() | ||
| .contains("ninja_required_version = 1.10"), | ||
| "serial bundle must declare version floor" | ||
| ); | ||
| ensure!( | ||
| bundle.dyndep_files().len() == 2, | ||
| "expected two sidecars, got {}", | ||
| bundle.dyndep_files().len() | ||
| ); | ||
| for dd in bundle.dyndep_files() { | ||
| ensure!( | ||
| dd.content().starts_with("ninja_dyndep_version = 1\n"), | ||
| "sidecar must start with dyndep version header" | ||
| ); | ||
| } | ||
| Ok(()) | ||
| } |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract a shared edge fixture; the file is close to the 400-line ceiling.
The two new tests repeat the full Action and BuildEdge literals that already appear six times earlier in this file. The only variation is the recipe command, the implicit deps, and the dependency order. The file now reaches line 360, and the repository enforces a 400-line maximum for Rust files, including crate-root integration tests under tests/.
Add rstest fixtures or small constructor helpers for Action and BuildEdge, as src/ninja_gen/dyndep_tests.rs already does with action, serial_edge, and parallel_edge. That removes the duplication and keeps headroom under the ceiling.
Based on learnings, enforce the 400-line maximum for every Rust code file, including crate-root integration test files under tests/; do not treat Whitaker lint's current failure to enforce this on crate roots as an exception.
🤖 Prompt for 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.
In `@tests/ninja_gen_integration_tests.rs` around lines 272 - 360, Extract
reusable rstest fixtures or small constructor helpers for the repeated Action
and BuildEdge literals in serial_graph_rejected_by_string_only_generation and
bundle_generation_for_serial_graph_materializes_sidecars, parameterizing
command, implicit dependencies, and dependency order as needed. Follow the
existing action, serial_edge, and parallel_edge pattern in dyndep_tests.rs, and
keep the integration test file below the enforced 400-line limit.
Sources: Coding guidelines, Learnings
Comment on lines
+1
to
+7
| //! Real-Ninja runtime tests for serial dependency ordering. | ||
| //! | ||
| //! These tests drive an actual `ninja` process against a bundle generated by | ||
| //! `generate_bundle`, using filesystem markers to prove declaration order, | ||
| //! failure short-circuiting, and shared-work reuse. They deliberately assert | ||
| //! observable behaviour rather than generated text. | ||
|
|
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add the shared-dependency test that the module doc promises.
Line 5 states these tests prove "declaration order, failure short-circuiting, and shared-work reuse". The file contains two tests: ordering and short-circuiting. No test covers shared-work reuse.
Issue #552 requires that a dependency shared by several serial lists executes at most once per encompassing build. src/ninja_gen/dyndep_tests.rs checks only that a repeated dependency produces two distinct sidecars; it does not prove the underlying recipe runs once. This is exactly the property that needs a real Ninja run to confirm, because it depends on Ninja unifying the revealed dependency node across gates.
Add a test with two serial aggregates that both depend on a common target, then assert the shared command appended to the log exactly once.
Do you want me to generate that test?
Also applies to: 115-210
🤖 Prompt for 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.
In `@tests/serial_dependency_runtime_tests.rs` around lines 1 - 7, Add a real
Ninja runtime test alongside the existing ordering and short-circuiting tests,
using the bundle-generation and filesystem-marker patterns already present in
the file. Define two serial aggregates that depend on the same shared target,
run the encompassing build, and assert the shared target’s command appends to
the log exactly once, proving reuse across serial lists.
| use std::process::Command; | ||
| use tempfile::TempDir; | ||
|
|
||
| const NINJA: &str = "ninja"; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find the existing convention for tests that require an external ninja binary.
set -euo pipefail
# Locate other tests that shell out to ninja and see how they guard availability.
rg -n -C4 --type=rust '"ninja"|Command::new\(NINJA\)|ninja_available|skip' tests/ src/ | head -n 120
# Check for a shared test helper crate or module that gates on external tools.
fd -t f -e rs . tests/ --exec rg -l 'which|is_available|requires_tool' {} \;Repository: leynos/netsuke
Length of output: 9964
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target file ---'
fd -t f -a 'serial_dependency_runtime_tests.rs' . --exec sh -c 'cat -n "$1"' sh {}
printf '%s\n' '--- external-command test patterns ---'
rg -n -C3 --type rust \
'Command::new|\.output\(\)|spawn [A-Za-z0-9_-]+|which::|PATH' \
tests src | head -n 240
printf '%s\n' '--- test support modules ---'
fd -t f -a . tests | rg 'support|helper|utils|common' | head -n 120Repository: leynos/netsuke
Length of output: 28527
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all repository references to Ninja ---'
rg -n -i -C3 '\bninja\b' . \
-g '!target/**' -g '!node_modules/**' -g '!*.lock' | head -n 320
printf '%s\n' '--- integration-test registration ---'
rg -n -C4 'serial_dependency_runtime_tests|harness *=|test-support|make test|cargo test' \
Cargo.toml Makefile .github tests 2>/dev/null | head -n 240
printf '%s\n' '--- command error behaviour ---'
python3 - <<'PY'
import errno
from pathlib import Path
print("ENOENT:", errno.ENOENT, Path("/definitely/missing/ninja").exists())
PYRepository: leynos/netsuke
Length of output: 46004
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- test target and CI prerequisites ---'
sed -n '1,180p' Makefile
sed -n '1,130p' .github/workflows/ci.yml
sed -n '1,180p' Cargo.toml
printf '%s\n' '--- tool-availability and skip conventions ---'
rg -n -i -C5 \
'skip|ignored|unavailable|not installed|missing prerequisite|command_available|which\(' \
tests .github Makefile docs \
-g '*.rs' -g '*.yml' -g '*.yaml' -g 'Makefile' -g '*.md' | head -n 320
printf '%s\n' '--- target file registration ---'
rg -n -C6 'serial_dependency_runtime_tests' . \
-g '!target/**' -g '!node_modules/**'Repository: leynos/netsuke
Length of output: 44634
Report a missing ninja prerequisite explicitly.
Keep these tests failing when Ninja is unavailable because CI requires it and the repository has no skip convention. Distinguish NotFound from other spawn errors and state that ninja must be available on PATH. The current spawn ninja context names the command but does not identify the missing prerequisite.
🤖 Prompt for 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.
In `@tests/serial_dependency_runtime_tests.rs` at line 17, Update the Ninja
process-spawn error handling in the serial dependency runtime tests to
distinguish ErrorKind::NotFound from other failures, explicitly report that
ninja must be available on PATH, and preserve test failure when it is missing.
Keep other spawn errors’ existing context and behavior unchanged.
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
2 participants
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
Implements the approved staged-Ninja-dyndep design for issue #552. Actions and
targets can declare
dependency_order: serialwhile preserving one Ninjascheduler, shared-work reuse, failure short-circuiting, and unrelated-branch
concurrency.
Closes #552.
User documentation
dependency_order: parallel | serialfor actions and targets inthe users' guide, with a complete executable manifest.
depsare ordered;independently reachable and unrelated work remains concurrent.
.netsuke/serialand.netsuke/dyndepnamespaces.developer, repository-layout, roadmap, contents, and living ExecPlan records.
Review walkthrough
parallel.Validation
make check-fmt: passed.make typecheck: passed.make lint: passed, including Whitaker.make test: passed; 1,939 tests passed, one skipped, and doctests passed.make markdownlint: passed.make nixie: passed.coderabbit review --agent: completed with zero actionable findings.References