feat(sdk): add Rust code generation#669
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughAdds Rust as a first-class SDK generation target: CLI/options/docs updated, Rust types/client/runtime generators implemented, Python/TypeScript exec generation refactored for consistent arg/flag ordering, and tests added to compile the generated Rust SDK. ChangesRust SDK Generation
Sequence Diagram(s)sequenceDiagram
participant CLI as CLI (--language)
participant Dispatcher as generate(dispatch)
participant RustGen as rust::generate
participant Types as types::render
participant Client as client::render
participant Runtime as RUNTIME_RS
participant Output as SdkOutput
CLI->>Dispatcher: SdkLanguage::Rust + SdkOptions
Dispatcher->>RustGen: call rust::generate(spec, opts)
RustGen->>Types: render(spec, package_name)
Types->>Output: types.rs
RustGen->>Client: render(spec, package_name)
Client->>Output: client.rs
RustGen->>Runtime: include RUNTIME_RS
Runtime->>Output: runtime.rs
RustGen->>Output: render lib.rs and finalize SdkOutput
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #669 +/- ##
==========================================
+ Coverage 79.31% 81.16% +1.85%
==========================================
Files 56 59 +3
Lines 10487 11779 +1292
Branches 10487 11779 +1292
==========================================
+ Hits 8318 9561 +1243
- Misses 1409 1445 +36
- Partials 760 773 +13 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Greptile SummaryThis PR adds Rust as a supported SDK generation target, producing
Confidence Score: 4/5Safe to merge after fixing the exec doc comment placement; the generated Rust code will otherwise emit unused_doc_comments warnings on every spec that carries usage strings or examples. The generator emits /// doc comments for the exec method followed immediately by a blank line before the function signature. Because Rust requires doc comments to sit flush against the item they document, this blank line detaches the comment and produces an unused_doc_comments compiler warning in every generated SDK whose spec defines usage or examples. All other previously-flagged issues have been resolved in this iteration. lib/src/sdk/rust/client.rs — the w.line("") call after the exec doc comment block needs to move before the comment rather than after it. Important Files Changed
Reviews (11): Last reviewed commit: "fix(sdk/rust): address PR review feedbac..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…type name mismatch, borrow-after-move
…structor borrow-after-move Flags are now inserted before the '--' separator instead of after it, ensuring CLI parsers correctly recognize them as flags rather than positional arguments. This fix applies to all three SDK generators (Rust, Python, TypeScript). Also fixes the non-root constructor to emit 'runner,' after 'runner.clone()' calls, preventing borrow-after-move compile errors for nested subcommands.
- Use let cmd_args (no mut) when command has no args and no flags
- Only generate {Name}Flags struct when command has its own flags;
commands that only inherit global flags use GlobalFlags directly
…rved keywords - Filter out choice enum names from client.rs imports since they are only referenced in types.rs, not in client.rs - Add missing Rust reserved keywords to sanitize_rs_ident: priv, do, try, become, final, typeof, unsized, virtual - Remove 'default' which is not a reserved keyword in Rust
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lib/tests/sdk_compile.rs (1)
144-188: ⚡ Quick winAdd cargo availability check for consistency with Python test.
The Rust compilation test should verify
cargois available before running, matching the pattern used in the Python test (Line 95). Without this check, the test will panic with a confusing error ifcargois not installed.🔧 Proposed fix to add cargo availability check
#[test] fn test_rust_sdk_compiles() { + if !tool_exists("cargo") { + eprintln!("Skipping Rust SDK compilation test - cargo not found"); + return; + } + let spec = full_spec();🤖 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 `@lib/tests/sdk_compile.rs` around lines 144 - 188, The test_rust_sdk_compiles test should check that the "cargo" binary is available before invoking Command::new("cargo") to avoid a confusing panic when cargo is missing; add an availability check (e.g. call Command::new("cargo").arg("--version").output() or .status() and test for Err or non-success) immediately before the existing Command::new("cargo") invocation and if cargo is not found, print/elog a short message and return/skip the test instead of continuing to run cargo check; update the test_rust_sdk_compiles function (which uses usage::sdk::generate and later builds the Command) to perform this pre-check.
🤖 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 `@lib/src/sdk/rust/types.rs`:
- Around line 461-463: escape_rs_string currently only handles backslash and
double-quote and fails to escape control/non-printable characters; change its
implementation to build a fully escaped string by iterating chars and using each
char's escape_default (e.g. s.chars().flat_map(|c|
c.escape_default()).collect::<String>()) and then ensure double quotes are
escaped (e.g. replace '"' with r#"\""#) so that control characters like \n, \r,
\t, \0 and other non-printables are emitted as valid Rust escapes when used by
the generator (function escape_rs_string, which is used to embed spec metadata
such as spec.about).
---
Nitpick comments:
In `@lib/tests/sdk_compile.rs`:
- Around line 144-188: The test_rust_sdk_compiles test should check that the
"cargo" binary is available before invoking Command::new("cargo") to avoid a
confusing panic when cargo is missing; add an availability check (e.g. call
Command::new("cargo").arg("--version").output() or .status() and test for Err or
non-success) immediately before the existing Command::new("cargo") invocation
and if cargo is not found, print/elog a short message and return/skip the test
instead of continuing to run cargo check; update the test_rust_sdk_compiles
function (which uses usage::sdk::generate and later builds the Command) to
perform this pre-check.
🪄 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: 97b0195e-3daf-425e-8d7c-c48d4598f82b
⛔ Files ignored due to path filters (56)
lib/src/sdk/python/snapshots/usage__sdk__python__tests__python_client.snapis excluded by!**/*.snaplib/src/sdk/python/snapshots/usage__sdk__python__tests__python_client_edge_cases.snapis excluded by!**/*.snaplib/src/sdk/python/snapshots/usage__sdk__python__tests__python_count_flag_build.snapis excluded by!**/*.snaplib/src/sdk/python/snapshots/usage__sdk__python__tests__python_deep_nesting.snapis excluded by!**/*.snaplib/src/sdk/python/snapshots/usage__sdk__python__tests__python_double_dash_automatic.snapis excluded by!**/*.snaplib/src/sdk/python/snapshots/usage__sdk__python__tests__python_exec_edge_cases.snapis excluded by!**/*.snaplib/src/sdk/python/snapshots/usage__sdk__python__tests__python_flag_edge_cases-2.snapis excluded by!**/*.snaplib/src/sdk/python/snapshots/usage__sdk__python__tests__python_flags_only_subcommand.snapis excluded by!**/*.snaplib/src/sdk/python/snapshots/usage__sdk__python__tests__python_full_feature_client.snapis excluded by!**/*.snaplib/src/sdk/python/snapshots/usage__sdk__python__tests__python_global_flags_flags_only.snapis excluded by!**/*.snaplib/src/sdk/python/snapshots/usage__sdk__python__tests__python_multiple_aliases.snapis excluded by!**/*.snaplib/src/sdk/python/snapshots/usage__sdk__python__tests__python_negate_flag_build.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_arg_defaults.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_boolean_flag_default_false.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_choice_collision.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_client.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_config_all_optional.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_config_boolean_default_false.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_config_props.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_config_string_with_default.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_count_flag_build.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_deep_nesting.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_double_dash_automatic.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_example_without_lang.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_flag_edge_cases-2.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_flag_edge_cases.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_flag_with_choices.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_flag_with_env.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_flags_only_subcommand.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_full_feature_client.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_full_feature_types.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_global_flags_flags_only.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_global_repeatable_flags.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_hyphenated_subcommands.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_lib.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_minimal.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_multiple_aliases.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_negate_flag_build.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_optional_arg_empty_flags.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_optional_variadic_arg.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_package_name_override.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_required_flag_type.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_runtime.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_types.snapis excluded by!**/*.snaplib/src/sdk/rust/snapshots/usage__sdk__rust__tests__rust_var_value_flag_with_default.snapis excluded by!**/*.snaplib/src/sdk/typescript/snapshots/usage__sdk__typescript__types__tests__deep_nesting.snapis excluded by!**/*.snaplib/src/sdk/typescript/snapshots/usage__sdk__typescript__types__tests__flags_only_subcommand.snapis excluded by!**/*.snaplib/src/sdk/typescript/snapshots/usage__sdk__typescript__types__tests__full_feature_client.snapis excluded by!**/*.snaplib/src/sdk/typescript/snapshots/usage__sdk__typescript__types__tests__typescript_client.snapis excluded by!**/*.snaplib/src/sdk/typescript/snapshots/usage__sdk__typescript__types__tests__typescript_client_edge_cases.snapis excluded by!**/*.snaplib/src/sdk/typescript/snapshots/usage__sdk__typescript__types__tests__typescript_count_flag_build.snapis excluded by!**/*.snaplib/src/sdk/typescript/snapshots/usage__sdk__typescript__types__tests__typescript_double_dash_automatic.snapis excluded by!**/*.snaplib/src/sdk/typescript/snapshots/usage__sdk__typescript__types__tests__typescript_flag_edge_cases-2.snapis excluded by!**/*.snaplib/src/sdk/typescript/snapshots/usage__sdk__typescript__types__tests__typescript_global_flags_flags_only.snapis excluded by!**/*.snaplib/src/sdk/typescript/snapshots/usage__sdk__typescript__types__tests__typescript_multiple_aliases.snapis excluded by!**/*.snaplib/src/sdk/typescript/snapshots/usage__sdk__typescript__types__tests__typescript_negate_flag_build.snapis excluded by!**/*.snap
📒 Files selected for processing (14)
cli/assets/fig.tscli/src/cli/generate/sdk.rscli/usage.usage.kdldocs/cli/reference/commands.jsondocs/cli/reference/generate/sdk.mddocs/cli/sdk.mdlib/src/sdk/mod.rslib/src/sdk/python/mod.rslib/src/sdk/rust/client.rslib/src/sdk/rust/mod.rslib/src/sdk/rust/runtime.rslib/src/sdk/rust/types.rslib/src/sdk/typescript/wrappers.rslib/tests/sdk_compile.rs
|
@jdx ready for review! remaining comments are for add cargo cli existence check in ci and I don't think it's needed lol. and coverage workflow fails strangely and may need a check |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 2 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 3 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
@jdx sorry to bother you but could you check this PR? it fails in coverage test and it'll be closed if no actions are performed. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 4 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 5 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
lib/src/sdk/mod.rs (1)
15-21:⚠️ Potential issue | 🟠 Major
#[non_exhaustive]onSdkLanguageis a semver-breaking public API change.
SdkLanguageis apub enum; adding#[non_exhaustive](line 16) forces downstream crates to handle future variants via a wildcard arm for exhaustivematches, making existing exhaustive matches fail to compile. If this isn’t an intentional breaking release, keep the enum exhaustive.Suggested change
#[derive(Debug, Clone)] -#[non_exhaustive] pub enum SdkLanguage { TypeScript, Python, Rust, }🤖 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 `@lib/src/sdk/mod.rs` around lines 15 - 21, The addition of #[non_exhaustive] to the public enum SdkLanguage makes this a semver-breaking change; revert that by removing the #[non_exhaustive] attribute from the SdkLanguage declaration (the enum with variants TypeScript, Python, Rust) so downstream crates with exhaustive match statements continue to compile, or if you intended to permit extension, keep the attribute but document the breaking change and update callers to use wildcard match arms for SdkLanguage.
🤖 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.
Outside diff comments:
In `@lib/src/sdk/mod.rs`:
- Around line 15-21: The addition of #[non_exhaustive] to the public enum
SdkLanguage makes this a semver-breaking change; revert that by removing the
#[non_exhaustive] attribute from the SdkLanguage declaration (the enum with
variants TypeScript, Python, Rust) so downstream crates with exhaustive match
statements continue to compile, or if you intended to permit extension, keep the
attribute but document the breaking change and update callers to use wildcard
match arms for SdkLanguage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 692105cb-97e3-4174-abe9-e4735bb1eb4c
📒 Files selected for processing (1)
lib/src/sdk/mod.rs
- Add cargo availability check in test_rust_sdk_compiles (matches Python test pattern) - Emit #[allow(deprecated)] on build_flag_args when deprecated flags exist - Use char::escape_default() in escape_rs_string for complete control char handling
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 2 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 3 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 4 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
2 participants
Summary by CodeRabbit
New Features
CLI
Documentation
Behavior
Tests