Skip to content

Expand top-level action flow control (3.14.2)#309

Merged
leynos merged 12 commits into
mainfrom
3-14-2-top-level-flow-control-expansion
May 22, 2026
Merged

Expand top-level action flow control (3.14.2)#309
leynos merged 12 commits into
mainfrom
3-14-2-top-level-flow-control-expansion

Conversation

@leynos
Copy link
Copy Markdown
Owner

@leynos leynos commented May 18, 2026

Summary

Implements roadmap item (3.14.2) for top-level action flow-control expansion.

This change registers command_available beside the existing which helper so manifest-time when clauses can select complementary top-level actions, including branches such as command_available(...) and not command_available(...). The helper reuses the which resolver and options, returns false only for command absence, and preserves hard errors for invalid arguments.

Execplan: docs/execplans/3-14-2-top-level-flow-control-expansion.md

Changes

  • Add command_available to the stdlib executable-discovery boundary.
  • Add stdlib, manifest expansion, and BDD coverage for available, unavailable, complementary, and invalid-option paths.
  • Document the user-facing helper in docs/users-guide.md and the internal resolver-boundary convention in docs/developers-guide.md.
  • Mark roadmap item 3.14.2 done while leaving broader executable-probe item 3.14.4 open.

Validation

  • cargo test --all-targets --all-features command_available: passed
  • cargo test --all-targets --all-features manifest::expand: passed
  • Focused rstest-bdd manifest and manifest-subcommand scenarios: passed
  • make markdownlint: passed
  • make nixie: passed
  • make check-fmt: passed
  • make lint: passed
  • make test: passed
  • coderabbit review --agent: milestone reviews passed after fixes; final post-validation attempts were rate-limited before producing findings

Notes

  • make fmt was attempted and still fails in the older markdownlint --fix path on pre-existing repository-wide Markdown line-length issues. Formatter churn from that attempt was restored; task-scoped Markdown linting and make markdownlint pass.

References

Add the pre-implementation ExecPlan for roadmap item `3.14.2`.
The plan records the existing action expansion path, the remaining
`command_available` branch-selection work, validation expectations,
CodeRabbit review cadence, and the approval gate before implementation.
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 18, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: f47c9cfd-39a4-449f-a947-9bf4b1126d46

📥 Commits

Reviewing files that changed from the base of the PR and between a0727ee and 3d57aef.

📒 Files selected for processing (1)
  • docs/roadmap.md

Summary

Implements roadmap item 3.14.2 by registering a new manifest-time helper, command_available, alongside the existing which helper. This enables manifest when clauses to select complementary top-level actions using predicates such as command_available(...) and not command_available(...).

Execplan

The implementation is guided by the new execplan document: docs/execplans/3-14-2-top-level-flow-control-expansion.md. This document defines the work scope, constraints, tolerances, and risks; provides a milestone-based implementation workflow; and logs completed progress, discoveries, and design decisions (including the semantics of command_available returning false only when a command is absent whilst preserving hard errors for invalid arguments).

Changes

Stdlib and Core Logic

  • src/stdlib/which/error.rs: Introduced NOT_FOUND_CODE marker and is_not_found_error() function to classify "not found" errors, enabling distinction between missing commands and other errors.
  • src/stdlib/which/mod.rs: Registered new command_available function that reuses WhichResolver and WhichOptions, returning false only for missing commands whilst propagating other errors as hard manifest failures.

Documentation

  • docs/developers-guide.md: Extended expand_foreach helper description with details on command_available registration and behaviour.
  • docs/users-guide.md: Added documentation for the new command_available manifest-time helper alongside which concepts; included example demonstrating conditional top-level actions selection using complementary when predicates.
  • docs/roadmap.md: Marked task 3.14.2 as completed; kept task 3.14.4 open.

Test Coverage

  • src/manifest/expand_test_cases/condition_cases.rs: Added rstest verifying static action expansion correctly selects between complementary when branches based on command_available.
  • tests/stdlib_which_tests.rs: New integration test file covering command_available and which filter, including validation of successful resolution, missing-command handling, and argument validation failures. Includes refactored helper assert_render_error_contains() to reduce DRY violations in error-assertion tests.
  • tests/bdd/steps/conditional_manifest.rs: Added BDD helpers for conditional-manifest testing, including OS-appropriate executable script creation, PATH manipulation, and command state management.
  • tests/features/manifest.feature and tests/features/manifest_subcommand.feature: New Gherkin scenarios verifying manifest parsing and generation with command_available predicates.
  • tests/data/actions_command_available_absent.yml and tests/data/actions_command_available_invalid.yml: Test fixtures covering available/unavailable command scenarios and invalid-option validation.

Notes

A CodeScene diagnostic flagged test duplication in tests/stdlib_which_tests.rs; this has already been resolved through extraction of the assert_render_error_contains() helper function, which the three flagged tests now delegate to. Automated review confirms the refactor is complete with no further changes required.

Walkthrough

Implement command_available(...) as a MiniJinja predicate beside which, map resolver-not-found to boolean false, add unit/integration and BDD tests (success, absence, invalid-args), update user/developer docs, add an ExecPlan, and mark roadmap tasks complete for 3.14.2.

Changes

command_available helper and action branching

Layer / File(s) Summary
Stdlib predicate and error handling
src/stdlib/which/error.rs, src/stdlib/which/mod.rs
Introduce NOT_FOUND_CODE and is_not_found_error; register command_available MiniJinja function that trims and validates the command, parses WhichOptions, resolves with WhichResolver, returns boolean, maps resolver not-found errors to false, and propagates other errors.
Stdlib integration tests
tests/stdlib_which_tests.rs
Add integration tests: command_available returns "true" when fixture tool is on an overridden PATH, returns "false" when absent with cwd_mode='never', validates empty-command and unknown-kwarg errors, and add a which filter not-found failure test.
Manifest action expansion
src/manifest/expand_test_cases/condition_cases.rs
Add parameterised test registering command_available in Minijinja; expand mutually-exclusive when: command_available(...) branches and assert only the selected branch remains and when is removed after expansion.
BDD fixtures and scenarios
tests/bdd/steps/conditional_manifest.rs, tests/data/actions_command_available_*.yml, tests/features/manifest*.feature
Add manifest fixture and helpers (write executable, set perms, prepend PATH), YAML fixtures for absent/invalid cases, and Gherkin scenarios validating preferred/fallback selection and invalid-argument failure.
User & developer documentation
docs/users-guide.md, docs/developers-guide.md
Document command_available alongside which, state boolean-return and absence-not-error semantics, and add example manifests showing conditional top-level actions.
ExecPlan, roadmap and retrospective
docs/execplans/3-14-2-top-level-flow-control-expansion.md, docs/roadmap.md
Add ExecPlan with constraints, milestones, validation checklist, decision log, completed progress, and update roadmap to mark task 3.14.2 completed for top-level action expansion and command_available branching.

Sequence Diagram

sequenceDiagram
  participant Template
  participant command_available
  participant WhichResolver
  participant is_not_found_error
  Template->>command_available: invoke(name, kwargs)
  command_available->>WhichResolver: resolve(name, WhichOptions)
  WhichResolver-->>command_available: matches / Error
  command_available->>is_not_found_error: check(Error)
  is_not_found_error-->>command_available: true / false
  command_available-->>Template: return boolean or propagate error
Loading
  • Possibly related PRs
    • leynos/netsuke#298: Builds on prior manifest-time foreach/when expansion work and extends tests and semantics for command_available(...).

Poem

Probe the PATH and sound the bell,
Preferred or fallback, choose which will swell,
When presence guides the manifest plan,
Actions shrink to the chosen span,
command_available rings the build's knell. 🎯

🚥 Pre-merge checks | ✅ 18 | ❌ 2

❌ Failed checks (2 warnings)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.29% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Testing (Compile-Time / Ui) ⚠️ Warning Missing snapshot test for Ninja output generated with command_available feature. PR adds manifest expansion with command_available branching but lacks insta snapshot test for structured output. Add snapshot test in tests/ninja_snapshot_tests.rs for command_available manifest expansion, capturing generated Ninja output via insta::assert_snapshot! alongside existing conditional_manifest_ninja_snapshot test.
✅ Passed checks (18 passed)
Check name Status Explanation
Title check ✅ Passed The PR title 'Expand top-level action flow control (3.14.2)' clearly summarises the main change—implementing roadmap item 3.14.2 for action flow-control expansion—and includes the required roadmap reference.
Description check ✅ Passed The PR description comprehensively documents the change: it explains the implementation of 3.14.2, lists changes made, provides validation results, and includes relevant references and notes.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Testing (Overall) ✅ Passed Thorough testing: stdlib unit tests validate core behaviour; manifest expansion tests verify filtering; BDD tests verify end-to-end. All assertions non-vacuous and would catch plausible bugs.
User-Facing Documentation ✅ Passed command_available is documented in docs/users-guide.md with clear explanation, use case, and YAML example. Non-breaking optional feature requires no migration docs.
Developer Documentation ✅ Passed New command_available API documented in developers-guide.md; roadmap item 3.14.2 marked complete; execplan created with COMPLETE status; no localized documentation sync needed.
Module-Level Documentation ✅ Passed All 5 Rust modules in the PR have compliant module-level documentation using //! syntax, each clearly explaining purpose, utility, and component relationships as required.
Testing (Unit And Behavioural) ✅ Passed Tests cover unit tests (positive/negative/errors), manifest expansion (complementary branches), and end-to-end BDD scenarios with actual command invocation through netsuke manifest subcommand.
Testing (Property / Proof) ✅ Passed Reuses resolver infrastructure with deterministic predicates; unit and BDD tests adequately cover error classification and branch selection without need for property testing.
Unit Architecture ✅ Passed Pure read-only query function, explicit Result fallibility, injected dependencies, no global state, tests verify boundaries through controlled fixture environments.
Domain Architecture ✅ Passed Stdlib adapter layer properly isolated. Manifest expansion treats Jinja environment as blackbox without accessing stdlib internals. Error classification constrained to adapter boundary.
Observability ✅ Passed command_available is manifest-time branching evaluated during TemplateExpansion with existing logging from expand_foreach. Hard errors for invalid arguments preserved; only 'not found' returns false.
Security And Privacy ✅ Passed Command-name inputs are validated (trimmed, non-empty), unknown kwargs rejected, results used only for manifest when conditionals, no credentials present, no injection risks.
Performance And Resource Use ✅ Passed Resource allocation is bounded: command_available runs at manifest-time only, LRU cache bounded by NonZeroUsize, Vec allocations bounded by system PATH, no algorithmic regressions.
Concurrency And State ✅ Passed command_available uses existing WhichResolver with Mutex cache. Locks only in cache access; I/O outside locks. No awaits holding locks. Single-threaded manifest expansion. Poisoning handled.
Architectural Complexity And Maintainability ✅ Passed Proportional change reusing WhichResolver, maintaining correct layering, extracting test helpers, no new dependencies or abstractions, comprehensive test coverage.
Rust Compiler Lint Integrity ✅ Passed No lint suppressions found; all imports and helpers have genuine callers; module boundaries properly scoped; no artificial references or lint appeasement patterns.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch 3-14-2-top-level-flow-control-expansion

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @leynos, you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

codescene-delta-analysis[bot]

This comment was marked as outdated.

Register `command_available` beside `which` so manifest-time `when`
clauses can choose between complementary top-level actions without turning
ordinary absence into a hard error.

Add direct stdlib coverage, manifest expansion regression tests, and BDD
scenarios for happy and unhappy paths. Document the user-visible helper,
record the internal resolver-boundary convention, and mark roadmap item
`3.14.2` complete.
codescene-delta-analysis[bot]

This comment was marked as outdated.

Update the execplan status after the implementation commit, branch push, and
draft pull request refresh so the living plan reflects the completed work.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos leynos marked this pull request as ready for review May 20, 2026 09:41
Copy link
Copy Markdown
Contributor

@sourcery-ai sourcery-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @leynos, you have reached your weekly rate limit of 2500000 diff characters.

Please try again later or upgrade to continue using Sourcery

@leynos
Copy link
Copy Markdown
Owner Author

leynos commented May 20, 2026

@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.

Code Duplication

tests/stdlib_which_tests.rs:

What lead to degradation?

The module contains 3 functions with similar structure: command_available_rejects_empty_command,command_available_rejects_unknown_keyword,which_filter_reports_missing_command

Why does this problem occur?

Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health.

How to fix it?

A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More
Once you have identified the similarities across functions, look to extract and encapsulate the concept that varies into its own function(s). These shared abstractions can then be re-used, which minimizes the amount of duplication and simplifies change.

@coderabbitai

This comment was marked as resolved.

@coderabbitai coderabbitai Bot added the Roadmap label May 20, 2026
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 306290ad19

ℹ️ 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 thread tests/stdlib_which_tests.rs
Comment thread tests/bdd/steps/conditional_manifest.rs Outdated
coderabbitai[bot]

This comment was marked as resolved.

Write `.cmd` fixture executables on Windows while keeping bare command names
in the templates and manifests. This keeps `command_available` positive-path
coverage aligned with the resolver's Windows `PATHEXT` probing behaviour.
codescene-delta-analysis[bot]

This comment was marked as outdated.

leynos added 2 commits May 20, 2026 13:21
Normalize the execplan reference definitions so each entry is a single,
consistent line, and tighten the `command_available` bullet in the user
guide to remove the repeated wording while keeping the `which` and
`command_available` references intact.
Prefix `which` not-found errors with the stable diagnostic sentinel before
classifying them for `command_available`. This keeps absence detection tied to
the intended code marker instead of localized rendered text.

Clean up two unrelated Markdown formatter artefacts left by the documentation
review pass so the user-guide diff stays focused.
codescene-delta-analysis[bot]

This comment was marked as outdated.

Extract a shared render-error assertion helper for the `which` and
`command_available` negative-path tests so the expected template, context, and
message fragment are the only per-case differences.
codescene-delta-analysis[bot]

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
docs/users-guide.md (2)

501-502: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Wrap paragraph at 80 columns.

Line 501 exceeds the 80-column limit (~85 characters).

Triage: [type:docstyle]

📝 Proposed fix
-Use `command_available` in manifest-time `when` clauses when optional tooling
-selects between actions:
+Use `command_available` in manifest-time `when` clauses when optional
+tooling selects between actions:

As per coding guidelines, Markdown paragraphs must be wrapped at 80 columns.

🤖 Prompt for AI Agents
In `@docs/users-guide.md` at lines 501-502, rewrap the introductory sentence
so line 501 is at most 80 columns. Break after "when optional" and continue
"tooling selects between actions:" on line 502.
🤖 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/users-guide.md` around lines 501 - 502, Reflow the Markdown paragraph in
docs/users-guide.md so no line exceeds 80 columns: split the sentence "Use
`command_available` in manifest-time `when` clauses when optional tooling
selects between actions:" into two lines by breaking after "when optional" and
placing "tooling selects between actions:" on the next line, ensuring proper
Markdown paragraph formatting and preserving backticks around
`command_available` and `when`.

514-514: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Wrap line at 80 columns.

Line 514 slightly exceeds the 80-column limit (~81 characters).

Triage: [type:docstyle]

📝 Proposed fix
-Only the selected action reaches the typed manifest and generated Ninja file.
+Only the selected action reaches the typed manifest and generated Ninja
+file.

As per coding guidelines, Markdown paragraphs must be wrapped at 80 columns.

🤖 Prompt for AI Agents
In `@docs/users-guide.md` at line 514, rewrap the sentence to at most 80
columns per line. Break after "manifest and generated Ninja" and continue
"file." on the next line.
🤖 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/users-guide.md` at line 514, The sentence "Only the selected action
reaches the typed manifest and generated Ninja file." exceeds 80 columns; rewrap
it so no line is longer than 80 columns by breaking after "manifest and
generated Ninja" and placing "file." on the next line (i.e., two lines: "Only
the selected action reaches the typed manifest and generated Ninja" then
"file.").
♻️ Duplicate comments (1)
docs/users-guide.md (1)

483-485: ⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Wrap lines at 80 columns.

Lines 483-484 exceed the 80-column paragraph wrapping limit (line 483: ~88 chars, line 484: ~85 chars).

Triage: [type:docstyle]

📝 Proposed fix
-- `command_available` function: Uses the same resolver and options as `which`,
-  returning `true` when a matching executable is found and `false` when it is
-  absent. Absence is not an error.
+- `command_available` function: Uses the same resolver and options as
+  `which`, returning `true` when a matching executable is found and
+  `false` when it is absent. Absence is not an error.

As per coding guidelines, Markdown paragraphs must be wrapped at 80 columns.

🤖 Prompt for AI Agents
In `@docs/users-guide.md` at lines 483-485, rewrap the `command_available`
bullet description so each line is at most 80 columns. Break after "`which`,"
on line 483 and after "and" on line 484. Preserve all backtick delimiters and
wording.
🤖 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/users-guide.md` around lines 483 - 485, The `command_available` bullet
in docs/users-guide.md exceeds the 80-column wrap limit; reflow that paragraph
so no line exceeds 80 columns while preserving wording and backticks: break the
sentence after "`which`," on the first line and after "and" on the next line so
the description reads the same but is wrapped to <=80 columns (keep
`command_available` and `which` backticks intact).
🤖 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 `@src/stdlib/which/error.rs`:
- Around line 18-21: The helper function with_not_found_code currently does an
unnecessary allocation by calling into_boxed_str(); remove that call and pass
the original message directly into the format! macro (e.g.
format!("{NOT_FOUND_CODE}: {message}") or format!("{}: {}", NOT_FOUND_CODE,
message)) so with_not_found_code(message: String) uses the String without
creating a Box<str>; update the function body in with_not_found_code
accordingly.

In `@tests/stdlib_which_tests.rs`:
- Line 132: Replace the redundant .to_string() call inside the with_context
closure by using .to_owned() on the &str; specifically update the closure
expression that currently reads .with_context(|| context_msg.to_string())? to
use .with_context(|| context_msg.to_owned())? where context_msg is the &str
being converted so Clippy no longer flags the redundant .to_string().

---

Outside diff comments:
In `@docs/users-guide.md`:
- Around line 501-502: Reflow the Markdown paragraph in docs/users-guide.md so
no line exceeds 80 columns: split the sentence "Use `command_available` in
manifest-time `when` clauses when optional tooling selects between actions:"
into two lines by breaking after "when optional" and placing "tooling selects
between actions:" on the next line, ensuring proper Markdown paragraph
formatting and preserving backticks around `command_available` and `when`.
- Line 514: The sentence "Only the selected action reaches the typed manifest
and generated Ninja file." exceeds 80 columns; rewrap it so no line is longer
than 80 columns by breaking after "manifest and generated Ninja" and placing
"file." on the next line (i.e., two lines: "Only the selected action reaches the
typed manifest and generated Ninja" then "file.").

---

Duplicate comments:
In `@docs/users-guide.md`:
- Around line 483-485: The `command_available` bullet in docs/users-guide.md
exceeds the 80-column wrap limit; reflow that paragraph so no line exceeds 80
columns while preserving wording and backticks: break the sentence after
"`which`," on the first line and after "and" on the next line so the description
reads the same but is wrapped to <=80 columns (keep `command_available` and
`which` backticks intact).
🪄 Autofix (Beta)

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: a3d51ac1-2c82-451b-a2f2-8405ed2968b2

📥 Commits

Reviewing files that changed from the base of the PR and between 2e94f38 and cdcb20a.

📒 Files selected for processing (4)
  • docs/execplans/3-14-2-top-level-flow-control-expansion.md
  • docs/users-guide.md
  • src/stdlib/which/error.rs
  • tests/stdlib_which_tests.rs

Comment thread src/stdlib/which/error.rs Outdated
Comment thread tests/stdlib_which_tests.rs Outdated
leynos added 2 commits May 22, 2026 17:43
Tighten line wrapping in `docs/users-guide.md` so the affected examples
and explanatory text stay within the Markdown line-length limit.
Remove the intermediate boxed-string conversion from the not-found sentinel
helper while still consuming the owned message under the repository lint
profile.

Use `to_owned` for the stdlib render-error context string to satisfy the
string-conversion lint in the shared assertion helper.
codescene-delta-analysis[bot]

This comment was marked as outdated.

leynos added 2 commits May 22, 2026 17:48
Expand the `tests/bdd/steps/conditional_manifest.rs` module docs to
explain the BDD scenarios, the conditional manifest behaviour they
verify, and how `TestWorld` carries workspace and environment state.

Mark roadmap item `3.14.4` complete in `docs/roadmap.md` after
confirming the `command_available(...)` implementation already reuses
the `which` resolver, returns `false` for missing commands, and
preserves argument validation diagnostics.
Accept the not-found message by reference in `with_not_found_code` so the
helper can format the stable sentinel without converting the owned message
through an intermediate wrapper.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos
Copy link
Copy Markdown
Owner Author

leynos commented May 22, 2026

@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.

Code Duplication

tests/stdlib_which_tests.rs:

What lead to degradation?

The module contains 3 functions with similar structure: command_available_rejects_empty_command,command_available_rejects_unknown_keyword,which_filter_reports_missing_command

Why does this problem occur?

Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health.

How to fix it?

A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More
Once you have identified the similarities across functions, look to extract and encapsulate the concept that varies into its own function(s). These shared abstractions can then be re-used, which minimizes the amount of duplication and simplifies change.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 22, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

Let me verify the current state of the file before finalising the response.


🏁 Script executed:

#!/bin/bash
cat tests/stdlib_which_tests.rs

Length of output: 5355


@leynos The fix is already present in the current codegraph. assert_render_error_contains is defined at line 117 of tests/stdlib_which_tests.rs, and all three flagged functions — command_available_rejects_empty_command, command_available_rejects_unknown_keyword, and which_filter_reports_missing_command — already delegate to it. No further code change is required.


If CodeScene's analysis has not yet caught up with the committed state and you still need to suppress the diagnostic, paste the following into the CodeScene suppression UI:

The three functions flagged by this diagnostic — command_available_rejects_empty_command, command_available_rejects_unknown_keyword, and which_filter_reports_missing_command — have been refactored in PR #309. A private helper, assert_render_error_contains, was extracted to encapsulate the shared render-and-assert pattern; each function now delegates to it and differs only in the three scalar arguments it passes. The duplicated structure no longer exists in the codebase.

This suppression covers the period between the CodeScene snapshot that triggered the diagnostic and the next analysis run that will reflect the refactored state.

codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos
Copy link
Copy Markdown
Owner Author

leynos commented May 22, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 22, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@docs/roadmap.md`:
- Around line 90-91: Revert the marked "completed" state for task identifier
3.14.4 in the roadmap document: change the historical traceability note and the
active checklist entries that mark `3.14.4` as completed back to an
open/in-progress state (ensure any checkboxes or "completed" wording are undone)
and add a short clarifying note that 3.14.4 remains open per the PR objective;
apply the same revert to the corresponding duplicate entries later in the file
(the other occurrence of `3.14.4`) so both the history and active checklist stay
consistent.
🪄 Autofix (Beta)

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: f28fc9d9-0c3f-4e54-89a2-7f9a493a8002

📥 Commits

Reviewing files that changed from the base of the PR and between cdcb20a and a0727ee.

📒 Files selected for processing (5)
  • docs/roadmap.md
  • docs/users-guide.md
  • src/stdlib/which/error.rs
  • tests/bdd/steps/conditional_manifest.rs
  • tests/stdlib_which_tests.rs

Comment thread docs/roadmap.md Outdated
Revert the roadmap completion state for `3.14.4` because this pull request is
scoped to `3.14.2`. Note that the branch introduces supporting
`command_available` behaviour without closing the broader roadmap item.
@leynos
Copy link
Copy Markdown
Owner Author

leynos commented May 22, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 22, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@leynos
Copy link
Copy Markdown
Owner Author

leynos commented May 22, 2026

@coderabbitai please create a new GitHub issue for the following:

Add snapshot test in tests/ninja_snapshot_tests.rs for command_available manifest expansion, capturing generated Ninja output via insta::assert_snapshot! alongside existing conditional_manifest_ninja_snapshot test.

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 22, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
cat tests/ninja_snapshot_tests.rs

Length of output: 4310


🏁 Script executed:

#!/bin/bash
rg -n "conditional_manifest_ninja_snapshot\|command_available\|insta\|assert_snapshot" tests/ninja_snapshot_tests.rs | head -40

Length of output: 0


🏁 Script executed:

#!/bin/bash
gh issue create \
  --repo leynos/netsuke \
  --assignee leynos \
  --title "Add snapshot test for command_available manifest expansion in ninja_snapshot_tests.rs" \
  --body "## Summary

Add a snapshot test to \`tests/ninja_snapshot_tests.rs\` that captures the generated Ninja output for a manifest that uses \`command_available\` branching in its \`when\` clauses, validated with \`insta::assert_snapshot!\` alongside the existing \`conditional_manifest_ninja_snapshot\` test.

## Background

PR `#309` introduced the \`command_available\` stdlib predicate and exercised it through BDD and unit tests, but no Ninja-output snapshot test exists for the code path where a \`when: command_available(...)\` / \`when: not command_available(...)\` pair filters top-level actions during manifest expansion and the result is handed to \`ninja_gen::generate\`.

## Proposed work

Add a new \`#[test]\` function — suggested name: \`command_available_manifest_ninja_snapshot\` — in \`tests/ninja_snapshot_tests.rs\`, following the same structure as \`conditional_manifest_ninja_snapshot\`:

1. Define an inline manifest YAML that declares two complementary actions:
   - one guarded by \`when: command_available(\"netsuke-command-that-should-not-exist\", cwd_mode=\"never\")\`
   - one guarded by \`when: not command_available(\"netsuke-command-that-should-not-exist\", cwd_mode=\"never\")\`
   - plus a simple \`targets\` section so the Ninja file is non-trivial
2. Call \`manifest::from_str\` → \`BuildGraph::from_manifest\` → \`ninja_gen::generate\`.
3. Use \`ensure!\` assertions to confirm the fallback action is present and the preferred action is absent from \`ninja_content\`.
4. Bind \`insta::Settings\` with \`snapshot_path\` pointing at \`tests/snapshots/ninja\` (matching the convention used by the existing snapshot tests) and call \`assert_snapshot!(\"command_available_manifest_ninja\", ninja_content)\`.

The test must be deterministic without any real binary on \`PATH\` (hence \`cwd_mode=\"never\"\` and a command name that is guaranteed absent).

## Acceptance criteria

- New test compiles and passes with \`cargo test --test ninja_snapshot_tests\`.
- A committed \`insta\` snapshot file appears at \`tests/snapshots/ninja/command_available_manifest_ninja.snap\`.
- The snapshot file is reviewed and approved before merging.

## References

- PR `#309` — introduces \`command_available\`
- \`tests/ninja_snapshot_tests.rs\` — \`conditional_manifest_ninja_snapshot\` as the model
- \`tests/data/actions_command_available_absent.yml\` — existing fixture for the absent-command path
"

Length of output: 45


@leynos GitHub issue #310 has been created and assigned to you: #310

The issue specifies:

  • Test name: command_available_manifest_ninja_snapshot
  • Pattern: mirrors conditional_manifest_ninja_snapshot — inline YAML, manifest::from_strBuildGraph::from_manifestninja_gen::generate, ensure! content assertions, then insta::assert_snapshot! bound to tests/snapshots/ninja
  • Determinism constraint: use cwd_mode="never" and a command name guaranteed absent, so no real binary is required on PATH
  • Acceptance criteria: test passes, snapshot file committed at tests/snapshots/ninja/command_available_manifest_ninja.snap

@leynos leynos merged commit 05c2994 into main May 22, 2026
16 checks passed
@leynos leynos deleted the 3-14-2-top-level-flow-control-expansion branch May 22, 2026 19:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant