You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
The PR standardizes Rust subprocess error handling with command-error, adds AST-grep and documentation guidance, introduces a shared empty-PATH test helper, reorganizes et-int-gen errors, and updates Zig generation normalization and unavailable-tool behavior.
Changes
Command error rollout
Layer / File(s)
Summary
Repository rules and workspace configuration .deepsource.toml, CLAUDE.md, Cargo.toml, config/ast-grep/rules/*
Adds the shared command-error dependency, coverage exclusions, subprocess-import enforcement, doc-summary enforcement, and related contributor guidance.
Shared test environment helpers libs/test-helpers/*, libs/ws-runner-common/*, services/ws/*
Adds with_empty_path, re-exports temp_env, and updates environment-driven tests and dependencies to use the shared helpers.
Edge toolkit command checks libs/edge-toolkit/*
Uses checked mise command output and updates the unavailable-mise test to temporarily clear PATH.
Replaces manual status and spawn checks with status_checked, output_checked, or spawn_checked.
Int-gen errors and Zig generation utilities/int-gen/*
Moves the public error type into a module, adds checked openapi2zig execution, strips volatile timestamps, updates inline-fetch matching, and tests behavior when the tool is unavailable.
We reviewed changes in 15af375...c08720c on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
While the code is technically up to standards according to Codacy, this PR suffers from significant scope creep. The title suggests a simple dependency upgrade to openapi2zig v0.4, but the changes include a workspace-wide migration to the command-error crate and the introduction of new ast-grep linting infrastructure.
Crucially, the PR description is empty, providing no context for these secondary changes. Furthermore, the test plan is incomplete; while some environment-handling tests were found, there are no tests verifying the 0.4-specific regex replacement logic or the enforcement of the new command-requires-command-error linting rule. These gaps should be addressed to ensure the new infrastructure functions as intended.
About this PR
The PR description is empty, and the scope significantly exceeds the title 'Upgrade to openapi2zig v0.4'. It includes workspace-wide error handling migrations and new linting infrastructure which should be documented or split into separate PRs.
Test suggestions
Verify generate_zig and is_available correctly handle the absence of the openapi2zig binary via PATH manipulation.
Verify mise_is_available returns false when mise is not present on PATH.
Verify that rewrite correctly identifies and replaces the 0.4-specific inlined binary/text request pattern.
Verify that the command-requires-command-error ast-grep rule correctly flags std::process::Command imports lacking the extension trait.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify that `rewrite` correctly identifies and replaces the 0.4-specific inlined binary/text request pattern.
2. Verify that the `command-requires-command-error` ast-grep rule correctly flags `std::process::Command` imports lacking the extension trait.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: The regex INLINE_FETCH_BLOCK_PATTERN is sensitive to exact whitespace and structural formatting. Consider making the whitespace matches more flexible to avoid breakage if the generator's output style changes slightly.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
🧹 Nitpick comments (2)
Cargo.toml (1)
439-460: 🔒 Security & Privacy | 🔵 Trivial
These entries are package-name only. cargo-unmaintained does not support version-scoped ignores, so this suppresses every future windows-* occurrence too; keep it only if that broad suppression is intentional.
🤖 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 `@Cargo.toml` around lines 439 - 460, Review the cargo-unmaintained ignore list
in Cargo.toml and confirm whether suppressing all current and future versions of
these windows-* package names is intentional. If the suppression should not be
broad, remove these package-name entries and use a supported narrower mechanism;
otherwise retain them and document the deliberate scope.
Make the intentional raw-outcome exception explicit.
The new command-requires-command-error rule reserves fully qualified std::process::Command for tests that must inspect a non-zero result. This file imports Command, so the raw Command::new(...).output() call is indistinguishable from an unchecked subprocess call; qualify only this call as std::process::Command::new(...). (raw.githubusercontent.com)
Suggested change
- let output = Command::new(env!("CARGO_BIN_EXE_et-ws-pyo3-runner"))+ let output = std::process::Command::new(env!("CARGO_BIN_EXE_et-ws-pyo3-runner"))
🤖 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 `@services/ws-pyo3-runner/tests/modules.rs` around lines 139 - 140, In the test
case around the intentional raw subprocess outcome, qualify only the
`Command::new(...).output()` call as `std::process::Command::new(...)` instead
of using the imported `Command`. Preserve the existing raw output inspection and
leave other command invocations unchanged.
🤖 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 `@CLAUDE.md`:
- Around line 892-900: Reduce the “Expand doc-summary-ends-with-period as you
touch Rust files” section in CLAUDE.md to only the actionable imperative: when
modifying a Rust file, add it to the rule’s sorted files list and fix its
first-line summary violations. Remove the duplicated rollout, backlog, scoping,
and future-coverage explanation while preserving the required behavior.
In `@config/ast-grep/rules/command-requires-command-error.yaml`:
- Around line 18-20: Update the use_declaration matcher in
command-requires-command-error to require the CommandExt import specifically,
including the command_error::CommandExt as _ form, rather than matching any
import containing command_error. Ensure imports such as
command_error::CommandError do not satisfy the rule.
- Around line 5-10: The rule currently misses fully qualified
std::process::Command usages, allowing unchecked command execution to bypass
validation. Update command-requires-command-error.yaml to detect fully qualified
command construction and require command_error::CommandExt with checked methods,
while explicitly allowing only the narrow raw-command cases described in the
existing rule.
In `@config/ast-grep/rules/doc-summary-ends-with-period.yaml`:
- Around line 20-25: Update the follows guard in the doc-summary rule so only
preceding /// lines containing visible content are treated as doc lines; exclude
an empty /// line while preserving the existing handling for non-empty doc
comments.
In `@utilities/int-gen/tests/generate_zig.rs`:
- Around line 3-4: Update the module-level documentation describing the
absent-Zig behavior near generate_zig so it no longer calls the skip quiet. Keep
the description consistent with generate_zig’s eprintln! notice and the existing
wording around line 17.
---
Nitpick comments:
In `@Cargo.toml`:
- Around line 439-460: Review the cargo-unmaintained ignore list in Cargo.toml
and confirm whether suppressing all current and future versions of these
windows-* package names is intentional. If the suppression should not be broad,
remove these package-name entries and use a supported narrower mechanism;
otherwise retain them and document the deliberate scope.
In `@services/ws-pyo3-runner/tests/modules.rs`:
- Around line 139-140: In the test case around the intentional raw subprocess
outcome, qualify only the `Command::new(...).output()` call as
`std::process::Command::new(...)` instead of using the imported `Command`.
Preserve the existing raw output inspection and leave other command invocations
unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c8cc42be-fa10-402b-80a4-74d4b97877b2
📥 Commits
Reviewing files that changed from the base of the PR and between 15af375 and c08720c.
⛔ Files ignored due to path filters (2)
Cargo.lock is excluded by !**/*.lock
generated/zig-rest/src/et_rest_client.zig is excluded by !**/generated/**
This repeats the incremental allowlist and backlog guidance already documented in config/ast-grep/rules/doc-summary-ends-with-period.yaml Lines 1-4. Keep one canonical explanation and reduce the other location to only the necessary imperative.
As per coding guidelines, “Document each thing exactly once in its most relevant location; do not duplicate explanations or add documentation cross-references.”
🤖 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 `@CLAUDE.md` around lines 892 - 900, Reduce the “Expand
doc-summary-ends-with-period as you touch Rust files” section in CLAUDE.md to
only the actionable imperative: when modifying a Rust file, add it to the rule’s
sorted files list and fix its first-line summary violations. Remove the
duplicated rollout, backlog, scoping, and future-coverage explanation while
preserving the required behavior.
Close the fully-qualified Command bypass. This rule only matches use std::process::Command, so fully-qualified std::process::Command calls skip the check entirely. Add a companion rule for fully-qualified command use, or make the raw-command exception explicit and narrow.
🤖 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 `@config/ast-grep/rules/command-requires-command-error.yaml` around lines 5 -
10, The rule currently misses fully qualified std::process::Command usages,
allowing unchecked command execution to bypass validation. Update
command-requires-command-error.yaml to detect fully qualified command
construction and require command_error::CommandExt with checked methods, while
explicitly allowing only the narrow raw-command cases described in the existing
rule.
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -euo pipefail
printf'--- file context ---\n'
cat -n config/ast-grep/rules/command-requires-command-error.yaml
printf'\n--- related occurrences ---\n'
rg -n "command_error|CommandExt|CommandError" config/ast-grep/rules -S ||trueprintf'\n--- any tests or docs for this rule ---\n'
rg -n "command-requires-command-error|command_error". -S ||true
Repository: edge-toolkit/core
Length of output: 246
🏁 Script executed:
#!/bin/bashset -euo pipefail
echo'--- file context ---'
cat -n config/ast-grep/rules/command-requires-command-error.yaml
echoecho'--- related occurrences ---'
rg -n "command_error|CommandExt|CommandError" config/ast-grep/rules -S ||trueechoecho'--- any tests or docs for this rule ---'
rg -n "command-requires-command-error|command_error". -S ||true
Repository: edge-toolkit/core
Length of output: 3153
Match CommandExt, not just any command_error import. A file like use command_error::CommandError; should not satisfy this rule; only use command_error::CommandExt as _; actually puts the checked methods in scope.
🤖 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 `@config/ast-grep/rules/command-requires-command-error.yaml` around lines 18 -
20, Update the use_declaration matcher in command-requires-command-error to
require the CommandExt import specifically, including the
command_error::CommandExt as _ form, rather than matching any import containing
command_error. Ensure imports such as command_error::CommandError do not satisfy
the rule.
The reason will be displayed to describe this comment to others. Learn more.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' config/ast-grep/rules/doc-summary-ends-with-period.yaml
Repository: edge-toolkit/core
Length of output: 2064
Empty /// lines still bypass the summary check. follows treats a blank /// as a preceding doc line, so /// + ///Summary without punctuation skips the first-line punctuation rule. Narrow the guard to visible doc lines only.
🤖 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 `@config/ast-grep/rules/doc-summary-ends-with-period.yaml` around lines 20 -
25, Update the follows guard in the doc-summary rule so only preceding /// lines
containing visible content are treated as doc lines; exclude an empty /// line
while preserving the existing handling for non-empty doc comments.
generate_zig() emits an eprintln! notice on this branch, so it does not skip "quietly"; Line 17 already describes this correctly.
Proposed fix
-//! generation when it is absent (upstream ships no linux/arm64 release, so that lane always takes this path). We+//! generation when it is absent after printing a notice (upstream ships no linux/arm64 release, so that lane+//! always takes this path). We
📝 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
//! `generate_zig` probes for the `openapi2zig` binary via [`zig::is_available`] and quietly skips Zig REST-client
//! generation when it is absent (upstream ships no linux/arm64 release, so that lane always takes this path). We
//! `generate_zig` probes for the `openapi2zig` binary via [`zig::is_available`] and quietly skips Zig REST-client
//! generation when it is absent after printing a notice (upstream ships no linux/arm64 release, so that lane
//! always takes this path). We
🤖 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 `@utilities/int-gen/tests/generate_zig.rs` around lines 3 - 4, Update the
module-level documentation describing the absent-Zig behavior near generate_zig
so it no longer calls the skip quiet. Keep the description consistent with
generate_zig’s eprintln! notice and the existing wording around line 17.
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
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.
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary by CodeRabbit
Bug Fixes
Developer Experience