Expand top-level action flow control (3.14.2)#309
Conversation
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.
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
WalkthroughImplement the Changescommand_available helper and action branching
Possibly related PRs
Suggested labelsRoadmap Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 3 warnings)
✅ Passed checks (12 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Update the execplan status after the implementation commit, branch push, and draft pull request refresh so the living plan reflects the completed work.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response. Ensure that this is validated against the current version of the codegraph. If further refinement to address this finding would be deleterious, please supply a clear explanatory one to two paragraph markdown message I can paste into the CodeScene web ui's diagnostic suppression function so this diagnostic can be silenced. Code Duplicationtests/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 |
This comment was marked as resolved.
This comment was marked as resolved.
![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: 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".
| let tool = bin.join("preferred-tool"); | ||
| write_executable(&tool)?; |
There was a problem hiding this comment.
This BDD setup creates preferred-tool without a Windows executable suffix, but command discovery for non-path commands on Windows resolves via PATHEXT extensions. As a result, the “preferred command available” scenario can incorrectly select the fallback branch on Windows hosts, making the behavioural assertion platform-dependent.
Useful? React with 👍 / 👎.
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.
There was a problem hiding this comment.
🧩 Analysis chain
Yes — this has been fully resolved in the latest commit. The full file content confirms all three necessary fixes are in place:
- Platform-specific executable path —
fixture_command_path(&bin, "preferred-tool")now appends.cmdon Windows via the new helper. - Platform-specific script content —
executable_script()returns@echo off\r\non Windows and a POSIX shebang script elsewhere. - No-op
mark_executableon non-Unix — the#[cfg(not(unix))]variant returnsOk(())cleanly.
Resolving the comment now.
🏁 Script executed:
#!/bin/bash
# Resolve the review comment via GitHub API
gh api \
--method PUT \
-H "Accept: application/vnd.github+json" \
repos/leynos/netsuke/pulls/309/reviews \
2>/dev/null || true
# List review threads to find the comment ID
gh api \
-H "Accept: application/vnd.github+json" \
"repos/leynos/netsuke/pulls/309/comments" \
--jq '.[] | select(.user.login == "coderabbitai[bot]" or .user.login == "chatgpt-codex-connector[bot]") | {id: .id, body: .body[0:80]}'
Length of output: 76
@leynos — The fix is confirmed resolved from the full file content. The comment was originally posted by chatgpt-codex-connector[bot] rather than by me, so I am unable to resolve it via the API on my own behalf. Resolve the thread manually using the Resolve conversation button on GitHub.
To summarise what the latest commit addresses:
| Concern | Resolution |
|---|---|
Missing .cmd suffix on Windows |
fixture_command_path() appends .cmd via cfg!(windows) |
| Non-executable script content on Windows | executable_script() returns @echo off\r\n on Windows |
chmod call on non-Unix |
mark_executable() is a no-op under #[cfg(not(unix))] |
🧠 Learnings used
Learnt from: leynos
Repo: leynos/netsuke PR: 251
File: tests/bdd/types.rs:309-318
Timestamp: 2025-12-29T19:00:20.890Z
Learning: In the netsuke repository, rustfmt formatting rules take precedence over manual formatting preferences. Ensure that functions like HelperName::as_str() and From trait implementations remain in multi-line format as enforced by cargo fmt, even if internal coding guidelines suggest single-line forms, to avoid breaking make check-fmt.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: In Rust projects, when a type is #[must_use] and you would otherwise discard its value, prefer using drop(...) instead of lines like 'let _ = ...' to satisfy must_use checks. This aligns with an enforcement in Cargo.toml: let_underscore_must_use = "deny". Apply this guideline across Rust source files (e.g., in src/status.rs and other Rust files) to ensure lint compliance and avoid accidental ignored results.
Learnt from: leynos
Repo: leynos/netsuke PR: 266
File: src/status.rs:92-106
Timestamp: 2026-02-15T18:36:42.457Z
Learning: Enforce the deny(lint) setting for must_use results in Cargo.toml. When discarding must_use values (e.g., Result) after operations like writeln!(io::stderr(), ...), prefer the drop(...) idiom over using let _ = to satisfy the lint. Apply this pattern across all Rust source files (src/ and beyond) to ensure consistent lint compliance.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
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 winWrap 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 winWrap 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 winWrap 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
📒 Files selected for processing (4)
docs/execplans/3-14-2-top-level-flow-control-expansion.mddocs/users-guide.mdsrc/stdlib/which/error.rstests/stdlib_which_tests.rs
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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
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.
There was a problem hiding this comment.
Gates Failed
(1 file with Code Duplication)
Gates Passed
See analysis details in CodeScene
Reason for failure
| Enforce advisory code health rules | Violations | Code Health Impact | |
|---|---|---|---|
| stdlib_which_tests.rs | 1 advisory rule | 9.39 | Suppress |
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
1 participant
Summary
Implements roadmap item
(3.14.2)for top-level action flow-control expansion.This change registers
command_availablebeside the existingwhichhelper so manifest-timewhenclauses can select complementary top-levelactions, including branches such ascommand_available(...)andnot command_available(...). The helper reuses thewhichresolver and options, returnsfalseonly for command absence, and preserves hard errors for invalid arguments.Execplan: docs/execplans/3-14-2-top-level-flow-control-expansion.md
Changes
command_availableto the stdlib executable-discovery boundary.docs/users-guide.mdand the internal resolver-boundary convention indocs/developers-guide.md.3.14.2done while leaving broader executable-probe item3.14.4open.Validation
cargo test --all-targets --all-features command_available: passedcargo test --all-targets --all-features manifest::expand: passedrstest-bddmanifest and manifest-subcommand scenarios: passedmake markdownlint: passedmake nixie: passedmake check-fmt: passedmake lint: passedmake test: passedcoderabbit review --agent: milestone reviews passed after fixes; final post-validation attempts were rate-limited before producing findingsNotes
make fmtwas attempted and still fails in the oldermarkdownlint --fixpath on pre-existing repository-wide Markdown line-length issues. Formatter churn from that attempt was restored; task-scoped Markdown linting andmake markdownlintpass.References