refactor: improve API ergonomics with builder pattern and method chaining #165
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughRestructures gh-workflow into focused modules, converts many builder APIs to in-place mutation, adds typed workflow primitives (artifacts, concurrency, container, env, etc.), reworks Tailcall CI generation (incremental assembly, revamped lint/test/release jobs, StandardWorkflow::add_setup), and removes dependency-organizing helpers. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller
participant Std as StandardWorkflow
participant Builder as Workflow (mutable)
participant Jobs
participant Lint
participant Test
participant Release
Caller->>Std: to_ci_workflow(opts)
Std->>Builder: create base workflow (mutable)
Builder->>Jobs: add lint job
Jobs->>Lint: assemble nightly toolchain + fmt/clippy steps
alt auto_fix
Lint->>Lint: fmt (nightly), clippy --fix --allow-dirty, add autofix-ci step
else
Lint->>Lint: fmt --check, clippy -D warnings
end
Builder->>Jobs: add test job
Jobs->>Test: add rust-cache, optional nextest install, run tests (cargo/nextest), optional bench
alt auto_release
Builder->>Jobs: add release & release-pr jobs (needs: "build","lint")
end
Std-->>Caller: return assembled Workflow
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (4)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/gh-workflow/src/workflow.rs (1)
409-414: StepValue setters still consume self — add borrow_self for consistency.All other builders now return &mut Self; StepValue remains by-value. This makes delegated setters on Step/Step consume the step, breaking uniform chaining across the API.
Apply:
#[setters( strip_option, into, + borrow_self, generate_delegates(ty = "Step<Run>", field = "value"), generate_delegates(ty = "Step<Use>", field = "value") )]crates/gh-workflow/src/event.rs (1)
103-109: Return type inconsistency: methods mutate self but return &Self, breaking chaining with &mut setters.These functions accept &mut self and push into Vecs but return &Self. That prevents chaining with newly generated setters that require &mut Self.
Apply this mechanical change pattern (example shown; repeat for all listed methods):
-pub fn add_type(&mut self, type_: BranchProtectionRuleType) -> &Self { +pub fn add_type(&mut self, type_: BranchProtectionRuleType) -> &mut Self { self.types.push(type_); self }Similarly for:
- CheckRun::add_type
- CheckSuite::add_type
- Create::{add_branch, add_tag}
- Delete::{add_branch, add_tag}
- Deployment::add_branch
- DeploymentStatus::add_state
- Discussion::add_type
- DiscussionComment::add_type
- IssueComment::add_type
- Issues::add_type
- Label::add_type
- MergeGroup::add_type
- Milestone::add_type
- PullRequestTarget::{add_type, add_branch}
- Push::{add_branch, add_path, add_tag} – already returning &mut Self (keep as-is)
- RepositoryDispatch::add_type
- Schedule::add_cron
- Watch::add_type
- WorkflowRun::{add_type, add_workflow, add_branch}
Also applies to: 135-139, 161-165, 182-186, 188-192, 209-213, 215-219, 233-238, 253-257, 303-307, 333-337, 362-366, 419-423, 450-454, 470-474, 494-498, 531-535, 536-540, 541-545, 572-576, 602-606, 623-627, 629-633, 653-657, 659-663, 665-669, 693-698, 733-737, 747-751, 761-765, 775-779, 898-902, 904-908, 910-914 </blockquote></details> </blockquote></details>🧹 Nitpick comments (8)
crates/gh-workflow/src/cargo.rs (3)
46-49: args() setter overwrites to a single token — consider clarifying or adding a multi-arg API.Current doc says “overwritten”; implementation stores a single element. Recommend adding explicit single-token and multi-token helpers to avoid ambiguity.
Apply within-file diff to keep behavior but delegate to token helpers:
pub fn args<T: ToString>(&mut self, args: T) -> &mut Self { - self.args = vec![args.to_string()]; + self.args = vec![args.to_string()]; self }Additions outside the shown range (new helpers):
impl Cargo { pub fn add_arg<S: ToString>(&mut self, arg: S) -> &mut Self { self.args.push(arg.to_string()); self } pub fn add_args_tokens<I, S>(&mut self, args: I) -> &mut Self where I: IntoIterator<Item = S>, S: ToString, { self.args.extend(args.into_iter().map(|s| s.to_string())); self } }
52-60: Avoid split_whitespace tokenization; accept an iterator of args instead.Whitespace-splitting breaks quoted values and flags with spaces. Keep this method for back-compat, but add an iterator-based variant and prefer it in new code.
Apply this focused change to delegate split_whitespace to a token-aware helper:
pub fn add_args<T: ToString>(&mut self, args: T) -> &mut Self { - self.args.extend( - args.to_string() - .split_whitespace() - .map(|s| s.to_string()) - .collect::<Vec<_>>(), - ); + // Back-compat: keep simple whitespace split, but route through token API. + let tokens = args.to_string().split_whitespace().map(|s| s.to_string()); + self.add_args_tokens(tokens); self }
63-81: Optional: quote/escape args when joining into a shell string.Join with proper shell escaping to avoid breakage with spaces/special chars. Consider shell-escape for POSIX shells; on Windows, respect configured shell.
crates/gh-workflow/src/workflow.rs (1)
525-528: Optional security hardening: pin checkout to a commit SHA.actions/checkout v5 exists as of August 2025, but pinning to the v5.0.0 commit (08c6903cd8c0fde910a37f88322edcfb5dd907a8) mitigates tag-retargeting risks. Consider:
- keep API taking a “version” string,
- change checkout() to pass the SHA instead of the tag.
Example update outside the shown range:
pub fn checkout() -> Step<Use> { - Step::uses("actions", "checkout", "v5").name("Checkout Code") + Step::uses("actions", "checkout", "08c6903cd8c0fde910a37f88322edcfb5dd907a8") + .name("Checkout Code") }References: actions/checkout v5.0.0 exists and the SHA above corresponds to that release; pinning is widely recommended for supply-chain safety. (forge.tourmentine.com, emmer.dev)
crates/gh-workflow-tailcall/tests/ci.rs (1)
8-8: Avoid cloning in the test chain; use a local mutable instead.
to_owned()clones the workflow just to callgenerate(self). Slightly cleaner (and avoids the clone):fn generate() { - StandardWorkflow::default() - .auto_release(true) - .auto_fix(true) - .to_owned() - .generate() - .unwrap(); + let mut wf = StandardWorkflow::default(); + wf.auto_release(true).auto_fix(true); + wf.generate().unwrap(); }crates/gh-workflow-tailcall/src/standard.rs (3)
174-189: Prefer consistent args API usage for readability.You mix
args(...)andadd_args(...). Consider using one style (e.g., repeatedadd_argswith single tokens) for clarity and safer quoting.Example:
- clippy_step.add_args("--all-features --workspace -- -D warnings"); + clippy_step + .add_args("--all-features") + .add_args("--workspace") + .add_args("--") + .add_args("-D") + .add_args("warnings");
206-212: Pin looks good; consider adding a brief comment for the pinned SHA.Optional: add a note with the action version/tag mapped to this SHA for future upgrades.
job.add_step(Step::uses( "autofix-ci", "action", "551dded8c6cc8a1054039c8bc0b8b48c51dfc6ef", )); + // Pinned to action vX.Y.Z (update SHA alongside version bump)
114-114: Typo in comment.“Nneeds to by” → “needs to be”.
- // NOTE: The workflow name needs to by `autofix.ci` + // NOTE: The workflow name needs to be `autofix.ci`📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
crates/gh-workflow-tailcall/src/standard.rs(5 hunks)crates/gh-workflow-tailcall/tests/ci.rs(1 hunks)crates/gh-workflow/src/cargo.rs(2 hunks)crates/gh-workflow/src/event.rs(43 hunks)crates/gh-workflow/src/generate.rs(3 hunks)crates/gh-workflow/src/workflow.rs(22 hunks)🚧 Files skipped from review as they are similar to previous changes (1)
- crates/gh-workflow/src/generate.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tusharmath PR: tailcallhq/gh-workflow#97 File: crates/gh-workflow-tailcall/src/workflow.rs:121-121 Timestamp: 2024-12-10T06:33:21.046Z Learning: In `crates/gh-workflow-tailcall/src/workflow.rs`, prefer converting standalone functions into methods of the `Workflow` struct when appropriate.📚 Learning: 2024-12-10T06:33:21.046Z
Learnt from: tusharmath PR: tailcallhq/gh-workflow#97 File: crates/gh-workflow-tailcall/src/workflow.rs:121-121 Timestamp: 2024-12-10T06:33:21.046Z Learning: In `crates/gh-workflow-tailcall/src/workflow.rs`, prefer converting standalone functions into methods of the `Workflow` struct when appropriate.Applied to files:
crates/gh-workflow-tailcall/tests/ci.rscrates/gh-workflow-tailcall/src/standard.rscrates/gh-workflow/src/workflow.rs🧬 Code graph analysis (1)
crates/gh-workflow-tailcall/src/standard.rs (2)
crates/gh-workflow/src/cargo.rs (1)
new(28-36)crates/gh-workflow/src/workflow.rs (10)
new(108-110)new(233-239)new(362-364)new(645-655)new(676-679)new(816-818)checkout(526-528)github(352-359)uses(474-488)uses(514-523)⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (17)
crates/gh-workflow/src/cargo.rs (3)
7-7: Setter ergonomics: borrow_self adoption looks good.Consistent with the PR’s shift to in-place mutation and fluent chaining.
39-42: nightly() signature change is correct and chain-friendly.No functional concerns.
26-36: add_args_when removal verified across repo — no occurrences found.crates/gh-workflow/src/workflow.rs (11)
46-47: borrow_self on Workflow setters — good move.Improves ergonomics without changing serialization.
117-126: add_job uses take() correctly to avoid moves.Solid pattern for in-place mutation.
139-147: add_event merge logic is sound.Merges when present; initializes otherwise.
150-156: add_env uses take+extend — good.Preserves existing vars and avoids Option juggling.
242-249: add_step API: nice use of StepType and take().No issues.
252-258: Job::add_env mirrors Workflow::add_env — consistent.Looks good.
265-272: add_steps loops via add_step — fine, preserves validation.No concerns.
274-281: add_needs accumulates tmp_needs — consistent with builder staging.Assuming later resolution to string IDs remains unchanged.
495-500: Step::add_env switch to take()+extend is correct.Matches the add_env pattern elsewhere.
531-541: Step::add_with uses take()+merge and emptiness pruning — good.Keeps YAML clean by omitting empty maps.
579-606: Bulk borrow_self migration across Container/Credentials/Volume/... — LGTM.Consistent, no semantic changes.
Also applies to: 612-619, 635-642, 662-673, 686-727, 729-736, 742-754, 760-767, 773-785, 791-799, 825-832, 847-855, 861-871
crates/gh-workflow/src/event.rs (1)
14-15: borrow_self derivations across events — good consistency.Matches the project-wide API direction.
Also applies to: 97-98, 127-128, 153-154, 170-171, 197-198, 225-226, 244-245, 441-442, 463-464, 487-488, 520-521, 562-564, 593-594, 611-612, 638-639, 685-686, 724-725, 739-741, 754-755, 768-769, 784-785, 799-800, 817-818, 829-830, 843-844, 852-853, 883-884
crates/gh-workflow-tailcall/src/standard.rs (2)
29-30: Setter borrow_self change: confirm downstream call sites are updated.Switching setters to borrow
&mut selfchanges chaining semantics. CI passed here, but please ensure dependent crates don’t rely on by-value setters.
219-233: Nice addition: rust-cache to speed up CI.Good call on caching; aligns with typical Rust CI best practices.
| /// Converts the workflow into a Github workflow. | ||
| fn to_autofix_workflow(&self) -> GHWorkflow { | ||
| // NOTE: The workflow name needs to by `autofix.ci` | ||
| GHWorkflow::new("autofix.ci") | ||
| let mut workflow = GHWorkflow::new("autofix.ci"); | ||
| workflow | ||
| .add_env(self.workflow_flags()) | ||
| .on(self.workflow_event()) | ||
| .add_job("lint", self.lint_job(true)) | ||
| .add_job("lint", self.lint_job(true)); | ||
|
|
||
| workflow | ||
| } | ||
|
|
There was a problem hiding this comment.
Autofix workflow triggers on pushes; restrict to PRs only.
Objective requires auto-commit to run only on PRs. to_autofix_workflow() currently uses the same event as CI (includes pushes), which can push to main.
Restrict events here (leaving CI events unchanged):
- workflow
- .add_env(self.workflow_flags())
- .on(self.workflow_event())
- .add_job("lint", self.lint_job(true));
+ workflow
+ .add_env(self.workflow_flags())
+ .on(
+ Event::default()
+ .pull_request(
+ PullRequest::default()
+ .add_type(PullRequestType::Opened)
+ .add_type(PullRequestType::Synchronize)
+ .add_type(PullRequestType::Reopened)
+ .add_branch("main")
+ .to_owned(),
+ )
+ .to_owned(),
+ )
+ .add_job("lint", self.lint_job(true));📝 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.
| /// Converts the workflow into a Github workflow. | |
| fn to_autofix_workflow(&self) -> GHWorkflow { | |
| // NOTE: The workflow name needs to by `autofix.ci` | |
| GHWorkflow::new("autofix.ci") | |
| let mut workflow = GHWorkflow::new("autofix.ci"); | |
| workflow | |
| .add_env(self.workflow_flags()) | |
| .on(self.workflow_event()) | |
| .add_job("lint", self.lint_job(true)) | |
| .add_job("lint", self.lint_job(true)); | |
| workflow | |
| } | |
| /// Converts the workflow into a Github workflow. | |
| fn to_autofix_workflow(&self) -> GHWorkflow { | |
| // NOTE: The workflow name needs to by `autofix.ci` | |
| let mut workflow = GHWorkflow::new("autofix.ci"); | |
| workflow | |
| .add_env(self.workflow_flags()) | |
| .on( | |
| Event::default() | |
| .pull_request( | |
| PullRequest::default() | |
| .add_type(PullRequestType::Opened) | |
| .add_type(PullRequestType::Synchronize) | |
| .add_type(PullRequestType::Reopened) | |
| .add_branch("main") | |
| .to_owned(), | |
| ) | |
| .to_owned(), | |
| ) | |
| .add_job("lint", self.lint_job(true)); | |
| workflow | |
| } |
🤖 Prompt for AI Agents
In crates/gh-workflow-tailcall/src/standard.rs around lines 112 to 123, the
autofix workflow currently uses self.workflow_event() (which includes pushes)
and must be restricted to PRs only; change the workflow trigger to a
pull_request-only event (e.g., set the workflow to trigger on pull_request with
types like opened, reopened, synchronize) instead of reusing
self.workflow_event(), leaving the CI workflow code unchanged.
| fn lint_job(&self, auto_fix: bool) -> Job { | ||
| let job = self.init_job(if auto_fix { "Lint Fix" } else { "Lint" }); | ||
| let mut job = self.init_job(if auto_fix { "Lint Fix" } else { "Lint" }); | ||
|
|
||
| let job = if auto_fix { | ||
| if auto_fix { | ||
| job.concurrency( | ||
| Concurrency::new(Expression::new("autofix-${{github.ref}}")) | ||
| .cancel_in_progress(false), | ||
| ) | ||
| } else { | ||
| job | ||
| }; | ||
| .cancel_in_progress(false) | ||
| .to_owned(), | ||
| ); | ||
| } | ||
|
|
There was a problem hiding this comment.
Autofix job lacks write permissions to push commits.
init_job() sets contents: read. The autofix job needs contents: write (and typically pull_requests: write) to push fixes.
Grant write only when auto_fix is true:
- if auto_fix {
+ if auto_fix {
+ job.permissions(self.write_permissions());
job.concurrency(
Concurrency::new(Expression::new("autofix-${{github.ref}}"))
.cancel_in_progress(false)
.to_owned(),
);
}📝 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.
| fn lint_job(&self, auto_fix: bool) -> Job { | |
| let job = self.init_job(if auto_fix { "Lint Fix" } else { "Lint" }); | |
| let mut job = self.init_job(if auto_fix { "Lint Fix" } else { "Lint" }); | |
| let job = if auto_fix { | |
| if auto_fix { | |
| job.concurrency( | |
| Concurrency::new(Expression::new("autofix-${{github.ref}}")) | |
| .cancel_in_progress(false), | |
| ) | |
| } else { | |
| job | |
| }; | |
| .cancel_in_progress(false) | |
| .to_owned(), | |
| ); | |
| } | |
| fn lint_job(&self, auto_fix: bool) -> Job { | |
| let mut job = self.init_job(if auto_fix { "Lint Fix" } else { "Lint" }); | |
| if auto_fix { | |
| job.permissions(self.write_permissions()); | |
| job.concurrency( | |
| Concurrency::new(Expression::new("autofix-${{github.ref}}")) | |
| .cancel_in_progress(false) | |
| .to_owned(), | |
| ); | |
| } |
🤖 Prompt for AI Agents
In crates/gh-workflow-tailcall/src/standard.rs around lines 163 to 173, the
autofix branch uses a job initialized with read-only permissions (init_job sets
contents: read) but needs write permissions to push commits; when auto_fix is
true update the job permissions to set contents: write (and typically
pull_requests: write) only in that branch — e.g. after creating job for autofix
call the job permissions setter to change contents to write and add
pull_requests write, leaving the non-autofix path with the original read-only
permissions.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
crates/gh-workflow-tailcall/src/standard.rs (3)
77-83: Setup steps run in reverse order; use FIFO.Users expect insertion order. Also fix the misleading comment.
Apply:
- // Add setup steps in reverse order to maintain the correct sequence - for step in self.setup.iter().rev() { + // Add setup steps in insertion order + for step in &self.setup { job.add_step(step.clone()); }
115-121: Autofix workflow must trigger on PRs only (not pushes).Per objective, auto-commit must never run on main. Restrict triggers here (leave CI workflow unchanged).
Apply:
- workflow - .add_env(self.workflow_flags()) - .on(self.workflow_event()) - .add_job("lint", self.lint_job(true)); + workflow + .add_env(self.workflow_flags()) + .on( + Event::default() + .pull_request( + PullRequest::default() + .add_type(PullRequestType::Opened) + .add_type(PullRequestType::Synchronize) + .add_type(PullRequestType::Reopened) + .add_branch("main") + .to_owned(), + ) + .to_owned(), + ) + .add_job("lint", self.lint_job(true));
163-173: Autofix job needs write permissions to push.init_job sets contents: read. Grant write only when auto_fix is true.
Apply:
- if auto_fix { - job.concurrency( + if auto_fix { + job + .permissions(self.write_permissions()) + .concurrency( Concurrency::new(Expression::new("autofix-${{github.ref}}")) .cancel_in_progress(false) .to_owned(), - ); + ); }
🧹 Nitpick comments (1)
crates/gh-workflow/src/generate.rs (1)
39-51: Prefer GITHUB_WORKSPACE; fall back to git rev-parse.Avoids spawning git when the env is present (faster, works in CI/worktrees).
Apply:
- let root_dir = String::from_utf8( - Command::new("git") - .args(["rev-parse", "--show-toplevel"]) - .output()? - .stdout, - )?; + let root_dir = if let Ok(ws) = std::env::var("GITHUB_WORKSPACE") { + ws + } else { + String::from_utf8( + Command::new("git") + .args(["rev-parse", "--show-toplevel"]) + .output()? + .stdout, + )? + };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
crates/gh-workflow-tailcall/src/standard.rs(5 hunks)crates/gh-workflow/src/generate.rs(1 hunks)crates/gh-workflow/src/workflow.rs(23 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tusharmath
PR: tailcallhq/gh-workflow#97
File: crates/gh-workflow-tailcall/src/workflow.rs:121-121
Timestamp: 2024-12-10T06:33:21.046Z
Learning: In `crates/gh-workflow-tailcall/src/workflow.rs`, prefer converting standalone functions into methods of the `Workflow` struct when appropriate.
📚 Learning: 2024-12-10T06:33:21.046Z
Learnt from: tusharmath
PR: tailcallhq/gh-workflow#97
File: crates/gh-workflow-tailcall/src/workflow.rs:121-121
Timestamp: 2024-12-10T06:33:21.046Z
Learning: In `crates/gh-workflow-tailcall/src/workflow.rs`, prefer converting standalone functions into methods of the `Workflow` struct when appropriate.
Applied to files:
crates/gh-workflow-tailcall/src/standard.rscrates/gh-workflow/src/workflow.rs
🧬 Code graph analysis (1)
crates/gh-workflow-tailcall/src/standard.rs (2)
crates/gh-workflow/src/workflow.rs (10)
new(108-110)new(230-236)new(360-362)new(643-653)new(674-677)new(815-817)checkout(524-526)github(350-357)uses(472-486)uses(512-521)crates/gh-workflow/src/cargo.rs (1)
new(28-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (9)
crates/gh-workflow-tailcall/src/standard.rs (6)
29-29: Setter mutability on StandardWorkflow — LGTM.Adopting borrow_self improves ergonomics and matches the project-wide builder pattern.
142-161: Release job dependencies by ID — LGTM.Using .add_needs("build") and .add_needs("lint") resolves the earlier mismatch risk.
174-189: Lint steps composition — LGTM.Nightly fmt/clippy setup and --check gating look correct; warnings are denied.
Also applies to: 200-205
206-213: Action pinned by commit — good supply-chain hygiene.
218-255: Build/Test job — LGTM.Nextest install is gated; rust-cache v2 with cache-all-crates enabled looks good.
257-263: Permissions and events helpers — LGTM.write_permissions returns an owned value; workflow_event covers push main + tags and PRs targeting main.
Also applies to: 272-284
crates/gh-workflow/src/generate.rs (1)
13-14: Setter mutability on Generate — LGTM.crates/gh-workflow/src/workflow.rs (2)
46-47: Borrow-self migration and in-place mutation — LGTM across these sections.Consistent &mut chaining, Option::take patterns, and transparent Expression/Copy Level are well applied.
Also applies to: 118-126, 139-147, 150-156, 186-190, 239-246, 249-256, 261-269, 271-278, 492-498, 529-540, 659-660, 683-684, 739-740, 757-758, 771-771, 808-811, 727-734
106-116: No directWorkflow::new(...).<builder>chains found. Also verify no.default().<builder>chains on temporaries.
tusharmath
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
crates/gh-workflow-tailcall/src/standard.rs (3)
75-78: Setup steps are added in reverse order, contradicting expected FIFO behavior.The comment says "in reverse order to maintain the correct sequence" but this is counterintuitive. Users expect setup steps to execute in the order they were added (FIFO). The current implementation reverses this, which could lead to unexpected behavior.
Apply this diff to fix the order:
- // Add setup steps in reverse order to maintain the correct sequence - for step in self.setup.iter().rev() { + // Add setup steps in insertion order + for step in &self.setup { job = job.add_step(step.clone()); }
111-114: Autofix workflow triggers on pushes; restrict to PRs only.The autofix workflow uses the same event configuration as CI (line 113:
self.workflow_event()), which includes pushes to main. Per the PR objectives, auto-commit should only run on pull requests to prevent pushing to main.Apply this diff to restrict autofix to pull requests only:
GHWorkflow::new("autofix.ci") .add_env(self.workflow_flags()) - .on(self.workflow_event()) + .on( + Event::default() + .pull_request( + PullRequest::default() + .add_type(PullRequestType::Opened) + .add_type(PullRequestType::Synchronize) + .add_type(PullRequestType::Reopened) + .add_branch("main"), + ), + ) .add_job("lint", self.lint_job(true))
155-160: Autofix job lacks write permissions to push commits.The autofix job is initialized with read-only permissions from
init_job()(line 73 setscontents: read). However, to push lint fixes back to the PR branch, it needs write permissions.Add write permissions when
auto_fixis true:if auto_fix { + job = job.permissions(self.write_permissions()); job = job.concurrency( Concurrency::new(Expression::new("autofix-${{github.ref}}")) .cancel_in_progress(false), ); }
🧹 Nitpick comments (1)
crates/gh-workflow/src/workflow.rs (1)
119-125: Consider using mutable reference for consistency.While the current implementation works, most builder methods in this file now use
&mut self. Consider changingadd_jobto use&mut selffor consistency with the new API pattern.- pub fn add_job<T: ToString, J: Into<Job>>(mut self, id: T, job: J) -> Self { + pub fn add_job<T: ToString, J: Into<Job>>(&mut self, id: T, job: J) -> &mut Self { let key = id.to_string(); let jobs = self.jobs.take().unwrap_or_default().add(key, job.into()); self.jobs = Some(jobs); self }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (4)
crates/gh-workflow-tailcall/src/standard.rs(5 hunks)crates/gh-workflow/src/cargo.rs(0 hunks)crates/gh-workflow/src/generate.rs(1 hunks)crates/gh-workflow/src/workflow.rs(10 hunks)
💤 Files with no reviewable changes (1)
- crates/gh-workflow/src/cargo.rs
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tusharmath
PR: tailcallhq/gh-workflow#97
File: crates/gh-workflow-tailcall/src/workflow.rs:121-121
Timestamp: 2024-12-10T06:33:21.046Z
Learning: In `crates/gh-workflow-tailcall/src/workflow.rs`, prefer converting standalone functions into methods of the `Workflow` struct when appropriate.
📚 Learning: 2024-12-10T06:33:21.046Z
Learnt from: tusharmath
PR: tailcallhq/gh-workflow#97
File: crates/gh-workflow-tailcall/src/workflow.rs:121-121
Timestamp: 2024-12-10T06:33:21.046Z
Learning: In `crates/gh-workflow-tailcall/src/workflow.rs`, prefer converting standalone functions into methods of the `Workflow` struct when appropriate.
Applied to files:
crates/gh-workflow-tailcall/src/standard.rscrates/gh-workflow/src/workflow.rs
🧬 Code graph analysis (1)
crates/gh-workflow-tailcall/src/standard.rs (2)
crates/gh-workflow/src/workflow.rs (8)
new(109-111)new(229-235)new(345-347)new(628-638)new(659-662)new(800-802)uses(457-471)uses(497-506)crates/gh-workflow/src/cargo.rs (1)
new(28-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (15)
crates/gh-workflow-tailcall/src/standard.rs (6)
29-29: Great addition of setters attributes!The addition of
#[setters(strip_option, into)]improves the builder API ergonomics by allowing users to pass values directly without wrapping them inOption.
92-95: Well-designed public API for adding setup steps.The method signature is flexible with
Into<Step<Run>>and follows builder pattern conventions by returningSelf.
119-131: Improved incremental workflow building approach.The refactor to build the workflow incrementally using a mutable variable is cleaner and more maintainable than chaining multiple conditional calls.
141-142: Good fix for release job dependencies.Using explicit job IDs ("build", "lint") instead of reconstructed Job objects correctly matches the actual job IDs used in
to_ci_workflow().
162-202: Clean refactor of lint job with proper conditional logic.The lint job is well-structured with clear separation between fmt and clippy steps, and proper conditional handling for autofix mode. The use of mutable builders for each step improves readability.
207-237: Well-structured test job with proper nextest and caching support.The test job refactor properly handles the conditional installation of nextest and adds appropriate caching with
rust-cache. The conditional benchmark step is cleanly integrated.crates/gh-workflow/src/workflow.rs (7)
20-23: Clean API change for Jobs collection.The change from
inserttoaddwith owned self and return of Self aligns well with the builder pattern used throughout the codebase.
138-155: Consistent API improvement with mutable references.The changes to
add_eventandadd_envproperly implement the mutable reference pattern, making the API more consistent and efficient.
188-188: Good change to make Job.needs publicly accessible.Making
needspublic aligns with the removal oftmp_needsand simplifies the API by allowing direct access to job dependencies.
256-263: Well-implemented add_needs method.The method properly handles both initial and subsequent additions to the needs vector, maintaining clean builder semantics.
477-483: Proper use of take() for interior mutation.The consistent use of
take()on Option fields for in-place mutation is a good pattern that avoids unnecessary clones while maintaining ownership semantics.Also applies to: 514-524
711-711: Addition of Copy trait improves ergonomics.Adding
Copyto theLevelenum is appropriate since it's a simple enum without heap-allocated data, making it more convenient to use.
795-795: Good addition of serde(transparent) for cleaner serialization.The
#[serde(transparent)]attribute ensuresExpressionserializes directly as a string without wrapper structure, improving YAML output.crates/gh-workflow/src/generate.rs (2)
13-13: Consistent setter attributes across the codebase.The addition of
strip_optionto the setters aligns with the changes in other modules, providing a consistent API experience.
10-10: Simplified constructor after removing dependency organization.The removal of
organize_job_dependencycall simplifies the constructor and respects the workflow structure as provided by the caller, which is a cleaner approach.Also applies to: 21-22
… artifacts, environment, defaults, and strategies
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (13)
crates/gh-workflow/src/container.rs (2)
64-89: Validate volume parsing and align serialization with GitHub format
- String format for volumes in workflows is typically
"/host:/container"; modeling as a map may not match expected YAML. Confirm how this struct is serialized; you might need custom (de)serialize to emit a single string.- Parsing with
split(':')is fragile (Windows paths, extra segments like:ro). Prefersplitn(2, ':')and basic non-empty checks.- pub fn new(volume_str: &str) -> Option<Self> { - let parts: Vec<&str> = volume_str.split(':').collect(); - if parts.len() == 2 { - Some(Volume { - source: parts[0].to_string(), - destination: parts[1].to_string(), - }) - } else { - None - } - } + pub fn new(volume_str: &str) -> Option<Self> { + let mut it = volume_str.splitn(2, ':'); + let source = it.next().unwrap_or_default().trim(); + let destination = it.next().unwrap_or_default().trim(); + if source.is_empty() || destination.is_empty() { return None; } + Some(Volume { source: source.to_string(), destination: destination.to_string() }) + }If you confirm GitHub expects strings, consider:
// Emit "source:destination" as a single string in YAML. impl serde::Serialize for Volume { /* format as "{source}:{destination}" */ } impl<'de> serde::Deserialize<'de> for Volume { /* parse from string */ }
9-15: Provide a constructor to ensure requiredimageis always set
Container::default()yieldsimage = "". OfferContainer::new(image)to prevent invalid instances while keeping builder ergonomics.impl Container { pub fn new(image: impl Into<String>) -> Self { Self { image: image.into(), ..Default::default() } } }crates/gh-workflow/src/strategy.rs (1)
13-24: Optional: document matrix shape and add light helpersAdd a short doc/example for expected matrix JSON and consider a
with_matrix(Value)helper to avoid accidental wrong shapes.crates/gh-workflow/src/artifacts.rs (1)
21-35: Differentiate upload vs download fields for type-safety
retention_daysapplies to uploads, not downloads. Consider separate types (UploadArtifactwithretention_days,DownloadArtifactwithout) to prevent invalid combinations and improve codegen.If you keep a single
Artifact, ensure step generation omitsretention-daysfor downloads.crates/gh-workflow/src/env.rs (2)
22-31: Confirm GITHUB_TOKEN expression is emitted unquoted in YAML.Storing ${{ secrets.GITHUB_TOKEN }} as a JSON string can get quoted by YAML emitters, breaking the expression. If you have an Expression newtype in this crate, prefer using it here; otherwise, please verify the generated YAML keeps the token unquoted.
38-43: Minor generics polish.Rename T1/T2 to K/V for readability; no behavioral change.
- pub fn add<T1: ToString, T2: Into<Value>>(mut self, key: T1, value: T2) -> Self { + pub fn add<K: ToString, V: Into<Value>>(mut self, key: K, value: V) -> Self {crates/gh-workflow/src/environment.rs (1)
12-18: Consider supporting expressions for name/url.GitHub allows expressions in environment name/url. If your serializer quotes strings, expressions may break. Optionally switch these fields to an Expression or serde_json::Value to preserve raw expressions.
crates/gh-workflow/src/lib.rs (1)
1-1: Drop redundant extern crate
extern crate proc_macro;is unnecessary for non-proc-macro crates on Rust 2018+ editions. Safe to remove.-extern crate proc_macro;crates/gh-workflow/src/job.rs (2)
95-101: Env merge is fine; minor ergonomicsUsing
Option::take()is good. If Env’s inner map remains public, keep as-is; otherwise consider anEnv::mergeto avoid touchingenv.0.
52-56: Add job-level with for reusable workflowsJobs that
uses:a reusable workflow often requirewith:inputs. Consider addingwith: Option<IndexMap<String, Value>>to Job for parity with YAML.#[serde(skip_serializing_if = "Option::is_none")] pub uses: Option<String>, + #[serde(skip_serializing_if = "Option::is_none")] + pub with: Option<IndexMap<String, Value>>,Docs reference
jobs.<job_id>.uses/with. (docs.github.com)crates/gh-workflow/src/workflow.rs (3)
24-27: Generalize Jobs::add signature to improve ergonomics and consistencyAlign with Env::add/Step::add and avoid forcing callers to allocate a String or construct Job eagerly.
Apply:
- pub fn add(mut self, key: String, value: Job) -> Self { - self.0.insert(key, value); + pub fn add<K: ToString, J: Into<Job>>(mut self, key: K, value: J) -> Self { + self.0.insert(key.to_string(), value.into()); self }
123-129: Clarify/guard duplicate job IDsRepeated add_job with the same id silently overwrites. If overwrite is intended, document it; otherwise consider a checked variant (e.g., add_job_unique) that returns Result or asserts in debug.
153-159: Avoid reaching into Env internals; add an extend/merge helper on EnvHide tuple-field access and make precedence explicit. Also clarifies that new_env values overwrite existing keys.
Apply here:
- let mut env = self.env.take().unwrap_or_default(); - - env.0.extend(new_env.into().0); - self.env = Some(env); + let env = self.env + .take() + .unwrap_or_default() + .extend(new_env.into()); + self.env = Some(env);And add in crates/gh-workflow/src/env.rs:
impl Env { pub fn extend(mut self, other: Env) -> Self { self.0.extend(other.0); self } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (14)
crates/gh-workflow/src/artifacts.rs(1 hunks)crates/gh-workflow/src/concurrency.rs(1 hunks)crates/gh-workflow/src/container.rs(1 hunks)crates/gh-workflow/src/defaults.rs(1 hunks)crates/gh-workflow/src/env.rs(1 hunks)crates/gh-workflow/src/environment.rs(1 hunks)crates/gh-workflow/src/expression.rs(1 hunks)crates/gh-workflow/src/job.rs(1 hunks)crates/gh-workflow/src/lib.rs(2 hunks)crates/gh-workflow/src/permissions.rs(1 hunks)crates/gh-workflow/src/secret.rs(1 hunks)crates/gh-workflow/src/step.rs(1 hunks)crates/gh-workflow/src/strategy.rs(1 hunks)crates/gh-workflow/src/workflow.rs(3 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: tusharmath
PR: tailcallhq/gh-workflow#97
File: crates/gh-workflow-tailcall/src/workflow.rs:121-121
Timestamp: 2024-12-10T06:33:21.046Z
Learning: In `crates/gh-workflow-tailcall/src/workflow.rs`, prefer converting standalone functions into methods of the `Workflow` struct when appropriate.
📚 Learning: 2024-12-10T06:33:21.046Z
Learnt from: tusharmath
PR: tailcallhq/gh-workflow#97
File: crates/gh-workflow-tailcall/src/workflow.rs:121-121
Timestamp: 2024-12-10T06:33:21.046Z
Learning: In `crates/gh-workflow-tailcall/src/workflow.rs`, prefer converting standalone functions into methods of the `Workflow` struct when appropriate.
Applied to files:
crates/gh-workflow/src/workflow.rs
🧬 Code graph analysis (7)
crates/gh-workflow/src/concurrency.rs (1)
crates/gh-workflow/src/expression.rs (1)
new(13-15)
crates/gh-workflow/src/container.rs (1)
crates/gh-workflow/src/env.rs (1)
new(34-36)
crates/gh-workflow/src/expression.rs (1)
crates/gh-workflow/src/concurrency.rs (1)
new(24-27)
crates/gh-workflow/src/step.rs (3)
crates/gh-workflow/src/env.rs (4)
from(17-19)from(48-52)add(39-42)new(34-36)crates/gh-workflow/src/job.rs (3)
from(25-27)add_env(96-102)new(77-83)crates/gh-workflow/src/expression.rs (1)
new(13-15)
crates/gh-workflow/src/env.rs (1)
crates/gh-workflow/src/step.rs (5)
from(24-26)from(31-33)from(74-76)from(241-245)add(88-91)
crates/gh-workflow/src/job.rs (2)
crates/gh-workflow/src/env.rs (3)
from(17-19)from(48-52)new(34-36)crates/gh-workflow/src/step.rs (8)
from(24-26)from(31-33)from(74-76)from(241-245)to_value(47-47)to_value(55-57)to_value(62-64)add_env(188-194)
crates/gh-workflow/src/workflow.rs (2)
crates/gh-workflow/src/env.rs (1)
add(39-42)crates/gh-workflow/src/step.rs (1)
add(88-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (15)
crates/gh-workflow/src/secret.rs (1)
8-11: Confirm setter receiver style in generated setters
The derive_setters macro attributes are present, but we couldn’t verify from source if setters consumeself. Please inspect the expanded macro output (e.g., viacargo expand) forSecret(and other types) to ensure setters usefn field(mut self) -> Selfrather than&mut self.crates/gh-workflow/src/artifacts.rs (1)
11-19: LGTM on overall structureFields, serde settings, and setters align with the crate’s builder pattern and kebab-case convention.
crates/gh-workflow/src/permissions.rs (2)
52-60: LGTM on the model and serde shape.Level defaults to none and fields are skipped when None; kebab-case matches Actions schema (e.g., pull-requests, id-token).
7-10: No action needed: derive_setters already generates consuming setters
The#[setters(strip_option, into)]attribute withoutborrow_selfproducesfn field(self, …) -> Selfmethods by default, matching the consuming‐builder pattern. No manualwith_*methods are required.Likely an incorrect or invalid review comment.
crates/gh-workflow/src/environment.rs (1)
8-11: derive_setters emits consuming setters by default. No manualwith_*methods required. (lib.rs)crates/gh-workflow/src/lib.rs (1)
26-41: Re-exports and module split look goodThe crate-level re-exports nicely consolidate the public API and align with the builder ergonomics.
crates/gh-workflow/src/concurrency.rs (1)
24-27: Drop visibility-based concern Expression’s inner String is already public (pub struct Expression(pub String)), so using expr.0 is valid across modules.Likely an incorrect or invalid review comment.
crates/gh-workflow/src/job.rs (1)
75-83: Sane defaults; builder style LGTM
Job::newdefaulting to ubuntu-latest and chaining via Self is consistent with the PR’s ergonomics.crates/gh-workflow/src/defaults.rs (2)
22-25: Concurrency in defaults: OK (once Concurrency is valid)This is fine provided Concurrency only serializes supported keys (drop limit per earlier comment).
50-57: Retry types are internal? Ensure not serialized to invalid YAMLIf
RetryStrategymaps to native Actions keys, great. If not, keep it internal (e.g., consumed by generate.rs) and avoid serializing rawretry:at job/step level (not an official key).Docs list allowed keys and do not include retry. (docs.github.com)
crates/gh-workflow/src/step.rs (3)
219-222: Checkout default version v5: verify runner compatibilityactions/checkout v5 requires runner ≥ v2.327.1. Fine for GitHub-hosted; self-hosted users must update.
Reference: actions/checkout README shows v5 and runner requirement. (github.com)
224-235: with merging is neatUsing Merge for Input and stripping empty maps to None keeps YAML clean. Nice.
99-109: Delegated setters: good for chainingThe
generate_delegatesto Step/Step provides fluent APIs without duplication.crates/gh-workflow/src/workflow.rs (2)
9-19: Imports modularization looks goodImports align with the new module split and keep the surface tidy.
142-150: Event merge logic: LGTMUsing take()+merge keeps ownership clean and composes multiple add_event calls predictably.
| /// Whether to cancel in-progress jobs. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub cancel_in_progress: Option<bool>, | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Allow expressions for cancel-in-progress
Docs allow cancel-in-progress to be a boolean OR an expression. Consider modeling it with an untagged enum so YAML stays valid for both forms.
- /// Whether to cancel in-progress jobs.
- #[serde(skip_serializing_if = "Option::is_none")]
- pub cancel_in_progress: Option<bool>,
+ /// Whether to cancel in-progress jobs; supports bool or expression.
+ #[serde(skip_serializing_if = "Option::is_none")]
+ pub cancel_in_progress: Option<CancelInProgress>,Add alongside the struct:
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
#[serde(untagged)]
pub enum CancelInProgress {
Bool(bool),
Expr(Expression),
}Refs. (docs.github.com)
🤖 Prompt for AI Agents
In crates/gh-workflow/src/concurrency.rs around lines 14 to 17, the
cancel_in_progress field is typed as Option<bool> but GitHub allows a boolean or
an expression; change this by adding an untagged enum CancelInProgress with
variants Bool(bool) and Expr(Expression) (derive Debug, Serialize, Deserialize,
Clone, PartialEq, Eq) and then change the struct field to pub
cancel_in_progress: Option<CancelInProgress> (keep #[serde(skip_serializing_if =
"Option::is_none")] on the field); ensure Expression is in scope/imported and
adjust any usages/serialization expectations accordingly.
| /// The limit on concurrent jobs. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub limit: Option<u32>, |
There was a problem hiding this comment.
Remove unsupported field: concurrency.limit
GitHub Actions concurrency supports only group and cancel-in-progress; limit is not a valid key and would generate invalid YAML if emitted. Drop the field.
- /// The limit on concurrent jobs.
- #[serde(skip_serializing_if = "Option::is_none")]
- pub limit: Option<u32>,References: official docs show only group and cancel-in-progress. (docs.github.com)
📝 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.
| /// The limit on concurrent jobs. | |
| #[serde(skip_serializing_if = "Option::is_none")] | |
| pub limit: Option<u32>, |
🤖 Prompt for AI Agents
In crates/gh-workflow/src/concurrency.rs around lines 18-20, remove the
unsupported pub limit: Option<u32> field and any serde attributes referencing
it; update the struct to only include fields valid for GitHub Actions
concurrency (e.g., group and cancel_in_progress) and remove usages/tests that
serialize or construct the limit field so the generated YAML no longer emits an
invalid concurrency.limit key.
| /// Represents a network port. | ||
| #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] | ||
| #[serde(rename_all = "kebab-case")] | ||
| pub enum Port { | ||
| /// A port specified by its number. | ||
| Number(u16), | ||
|
|
||
| /// A port specified by its name. | ||
| Name(String), | ||
| } |
There was a problem hiding this comment.
Ports serialize incorrectly; use untagged enum
Without #[serde(untagged)], YAML becomes {number: 80}/{name: "http"} instead of bare 80/"http". This breaks GitHub schema expectations.
/// Represents a network port.
#[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
-#[serde(rename_all = "kebab-case")]
+#[serde(untagged)]
pub enum Port {
/// A port specified by its number.
Number(u16),
/// A port specified by its name.
Name(String),
}📝 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.
| /// Represents a network port. | |
| #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] | |
| #[serde(rename_all = "kebab-case")] | |
| pub enum Port { | |
| /// A port specified by its number. | |
| Number(u16), | |
| /// A port specified by its name. | |
| Name(String), | |
| } | |
| /// Represents a network port. | |
| #[derive(Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] | |
| #[serde(untagged)] | |
| pub enum Port { | |
| /// A port specified by its number. | |
| Number(u16), | |
| /// A port specified by its name. | |
| Name(String), | |
| } |
🤖 Prompt for AI Agents
In crates/gh-workflow/src/container.rs around lines 53 to 62, the Port enum
currently serializes as tagged maps (e.g. {number: 80}) but GitHub expects bare
values (80 or "http"); add the serde untagged attribute to the enum by
annotating it with #[serde(untagged)] so the Number(u16) and Name(String)
variants serialize as plain scalar values, leaving the existing rename_all
attribute alone.
| #[derive(Default, Debug, Serialize, Deserialize, Clone, PartialEq, Eq)] | ||
| #[serde(transparent)] | ||
| pub struct Env(pub IndexMap<String, Value>); |
There was a problem hiding this comment.
Deriving Eq here won’t compile (serde_json::Value does not implement Eq).
Env wraps IndexMap<String, Value>; serde_json::Value only implements PartialEq. Drop Eq from the derive list.
-#[derive(Default, Debug, Serialize, Deserialize, Clone, PartialEq, Eq)]
+#[derive(Default, Debug, Serialize, Deserialize, Clone, PartialEq)]🤖 Prompt for AI Agents
In crates/gh-workflow/src/env.rs around lines 11 to 13, the struct derives Eq
which fails to compile because serde_json::Value does not implement Eq; remove
Eq from the derive list (leave Default, Debug, Serialize, Deserialize, Clone,
PartialEq) so Env derives only traits supported by its inner types and recompile
to confirm the error is resolved.
| /// Represents environment variables as key-value pairs. | ||
| impl<S1: Display, S2: Display> From<(S1, S2)> for Env { | ||
| /// Converts a tuple into an `Env`. | ||
| fn from(value: (S1, S2)) -> Self { | ||
| let mut index_map: IndexMap<String, Value> = IndexMap::new(); | ||
| index_map.insert(value.0.to_string(), Value::String(value.1.to_string())); | ||
| Env(index_map) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Broaden tuple conversion to accept non-string values.
Current impl forces the value to String. Accept any Into so booleans/numbers/expressions don’t get stringified.
-impl<S1: Display, S2: Display> From<(S1, S2)> for Env {
+impl<K: ToString, V: Into<Value>> From<(K, V)> for Env {
/// Converts a tuple into an `Env`.
- fn from(value: (S1, S2)) -> Self {
+ fn from(value: (K, V)) -> Self {
let mut index_map: IndexMap<String, Value> = IndexMap::new();
- index_map.insert(value.0.to_string(), Value::String(value.1.to_string()));
+ index_map.insert(value.0.to_string(), value.1.into());
Env(index_map)
}
}📝 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.
| /// Represents environment variables as key-value pairs. | |
| impl<S1: Display, S2: Display> From<(S1, S2)> for Env { | |
| /// Converts a tuple into an `Env`. | |
| fn from(value: (S1, S2)) -> Self { | |
| let mut index_map: IndexMap<String, Value> = IndexMap::new(); | |
| index_map.insert(value.0.to_string(), Value::String(value.1.to_string())); | |
| Env(index_map) | |
| } | |
| } | |
| /// Represents environment variables as key-value pairs. | |
| impl<K: ToString, V: Into<Value>> From<(K, V)> for Env { | |
| /// Converts a tuple into an `Env`. | |
| fn from(value: (K, V)) -> Self { | |
| let mut index_map: IndexMap<String, Value> = IndexMap::new(); | |
| index_map.insert(value.0.to_string(), value.1.into()); | |
| Env(index_map) | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/gh-workflow/src/env.rs around lines 45 to 53, the From<(S1,S2)> impl
currently forces the value to a String which stringifies booleans/numbers;
change the impl to accept a key type convertible to String and a value type
convertible Into<Value> (e.g. impl<K: Into<String>, V: Into<Value>> From<(K, V)>
for Env), call key.into() for the map key and value.into() for the Value, and
ensure the Value type is in scope (add use if necessary) so non-string values
are preserved instead of being stringified.
| impl Expression { | ||
| /// Creates a new `Expression` from a string. | ||
| pub fn new<T: ToString>(expr: T) -> Self { | ||
| Self(expr.to_string()) | ||
| } | ||
| } |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Into<Expression> usage will fail without From impls
Call sites like Concurrency::new("group") require From<&str>/From<String> for Expression. Add them to make impl Into<Expression> ergonomic.
impl Expression {
/// Creates a new `Expression` from a string.
pub fn new<T: ToString>(expr: T) -> Self {
Self(expr.to_string())
}
}
+
+impl From<&str> for Expression {
+ fn from(s: &str) -> Self { Self(s.to_string()) }
+}
+
+impl From<String> for Expression {
+ fn from(s: String) -> Self { Self(s) }
+}🏁 Script executed:
#!/bin/bash
# Verify call sites expecting Into<Expression> compile ergonomically.
rg -n 'Into<Expression>' -C3
rg -nP 'new\([^)]*"(?:[^"]*)"\)' crates/gh-workflow/src | sed -n '1,200p'Length of output: 642
Implement From<&str> and From for Expression
Add these impls in crates/gh-workflow/src/expression.rs (below the existing impl Expression) so that &str and String automatically convert into Expression, enabling ergonomic calls like Concurrency::new("group"):
impl Expression {
/// Creates a new `Expression` from a string.
pub fn new<T: ToString>(expr: T) -> Self {
Self(expr.to_string())
}
}
+impl From<&str> for Expression {
+ fn from(s: &str) -> Self { Self(s.to_string()) }
+}
+
+impl From<String> for Expression {
+ fn from(s: String) -> Self { Self(s) }
+}📝 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.
| impl Expression { | |
| /// Creates a new `Expression` from a string. | |
| pub fn new<T: ToString>(expr: T) -> Self { | |
| Self(expr.to_string()) | |
| } | |
| } | |
| impl Expression { | |
| /// Creates a new `Expression` from a string. | |
| pub fn new<T: ToString>(expr: T) -> Self { | |
| Self(expr.to_string()) | |
| } | |
| } | |
| impl From<&str> for Expression { | |
| fn from(s: &str) -> Self { | |
| Self(s.to_string()) | |
| } | |
| } | |
| impl From<String> for Expression { | |
| fn from(s: String) -> Self { | |
| Self(s) | |
| } | |
| } |
🤖 Prompt for AI Agents
In crates/gh-workflow/src/expression.rs around lines 11 to 16, add
implementations of the From trait for &str and String for Expression (placed
below the existing impl Expression block); implement From<&str> by converting
the &str to String and calling Expression::new (or Expression(String)), and
implement From<String> by wrapping the String into an Expression directly, so
users can pass &str and String where an Expression is expected.
| /// Indicates if the secret is required. | ||
| pub required: bool, | ||
|
|
||
| /// A description of the secret. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub description: Option<String>, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make required backward-compatible and omit falsey defaults in YAML
Without a default, deserialization fails when required is omitted; and always emitting required: false is noisy. Add a default and skip serializing false.
- /// Indicates if the secret is required.
- pub required: bool,
+ /// Indicates if the secret is required.
+ #[serde(default, skip_serializing_if = "is_false")]
+ pub required: bool,Add this helper in the module:
fn is_false(b: &bool) -> bool { !*b }🤖 Prompt for AI Agents
In crates/gh-workflow/src/secret.rs around lines 12 to 17, the pub required:
bool field needs a serde default and should skip serializing when false to be
backward-compatible and avoid noisy YAML; add a module helper fn is_false(b:
&bool) -> bool { !*b } and annotate the field with #[serde(default,
skip_serializing_if = "is_false")] so missing values deserialize as false and
false values are omitted from serialized YAML.
| /// The retry strategy for the step. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub retry: Option<RetryStrategy>, | ||
|
|
||
| /// Artifacts produced by the step. | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub artifacts: Option<Artifacts>, | ||
| } |
There was a problem hiding this comment.
Do not emit non-standard retry in steps
retry is not an official step key. If you intend to expand it in generate.rs to a pattern (e.g., wrapper action), mark it #[serde(skip)] to avoid invalid YAML.
- /// The retry strategy for the step.
- #[serde(skip_serializing_if = "Option::is_none")]
- pub retry: Option<RetryStrategy>,
+ /// The retry strategy for the step (internal; expanded by generator).
+ #[serde(skip)]
+ pub retry: Option<RetryStrategy>,Official step keys: id, name, if, uses, with, run, shell, env, timeout-minutes, continue-on-error, working-directory. (docs.github.com)
📝 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.
| /// The retry strategy for the step. | |
| #[serde(skip_serializing_if = "Option::is_none")] | |
| pub retry: Option<RetryStrategy>, | |
| /// Artifacts produced by the step. | |
| #[serde(skip_serializing_if = "Option::is_none")] | |
| pub artifacts: Option<Artifacts>, | |
| } | |
| /// The retry strategy for the step (internal; expanded by generator). | |
| #[serde(skip)] | |
| pub retry: Option<RetryStrategy>, | |
| /// Artifacts produced by the step. | |
| #[serde(skip_serializing_if = "Option::is_none")] | |
| pub artifacts: Option<Artifacts>, | |
| } |
🤖 Prompt for AI Agents
crates/gh-workflow/src/step.rs around lines 152 to 159: the Step struct
currently serializes a non-standard `retry` field which will emit invalid YAML
for GitHub Actions steps; change its serde attribute to skip serialization
(e.g., #[serde(skip)]) so the field is available in Rust for internal use or
transformation but will not be emitted into the generated workflow YAML, or
remove the field entirely if unused.
| /// Represents the strategy for running jobs. | ||
| #[derive(Debug, Setters, Serialize, Deserialize, Clone, Default, PartialEq, Eq)] | ||
| #[serde(rename_all = "kebab-case")] | ||
| #[setters(strip_option, into)] | ||
| pub struct Strategy { |
There was a problem hiding this comment.
Eq derive conflicts with serde_json::Value
serde_json::Value does not implement Eq, so #[derive(Eq)] on Strategy will not compile.
-#[derive(Debug, Setters, Serialize, Deserialize, Clone, Default, PartialEq, Eq)]
+#[derive(Debug, Setters, Serialize, Deserialize, Clone, Default, PartialEq)]
pub struct Strategy {📝 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.
| /// Represents the strategy for running jobs. | |
| #[derive(Debug, Setters, Serialize, Deserialize, Clone, Default, PartialEq, Eq)] | |
| #[serde(rename_all = "kebab-case")] | |
| #[setters(strip_option, into)] | |
| pub struct Strategy { | |
| /// Represents the strategy for running jobs. | |
| #[derive(Debug, Setters, Serialize, Deserialize, Clone, Default, PartialEq)] | |
| #[serde(rename_all = "kebab-case")] | |
| #[setters(strip_option, into)] | |
| pub struct Strategy { |
🤖 Prompt for AI Agents
In crates/gh-workflow/src/strategy.rs around lines 8 to 12, the #[derive(...,
Eq)] on Strategy fails because serde_json::Value does not implement Eq; remove
Eq from the derive list (leave PartialEq) so the struct derives PartialEq but
not Eq, or alternatively replace Eq with a manual implementation only if all
fields support Eq; update the attribute to #[derive(Debug, Setters, Serialize,
Deserialize, Clone, Default, PartialEq)].
…ter clarity and consistency
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)
crates/gh-workflow/src/toolchain.rs (2)
223-229: ABI is concatenated without a hyphen (generates invalid targets like linuxmusl)Format becomes x86_64-unknown-linuxmusl instead of x86_64-unknown-linux-musl.
Apply:
- let target = format!( - "{}-{}-{}{}", - target.arch, - target.vendor, - target.system, - target.abi.map(|v| v.to_string()).unwrap_or_default(), - ); + let abi = target.abi.map(|v| format!("-{}", v)).unwrap_or_default(); + let target = format!( + "{}-{}-{}{}", + target.arch, + target.vendor, + target.system, + abi, + );
255-259: Leading newline in cache-workspacesFold introduces a leading newline. Use join like cache_directories.
- let cache_workspaces = value - .cache_workspaces - .iter() - .fold("".to_string(), |acc, a| format!("{acc}\n{a}")); + let cache_workspaces = value.cache_workspaces.join("\n");
♻️ Duplicate comments (1)
crates/gh-workflow/src/step.rs (1)
100-159: Do not serialize non-standard step fields (retry,artifacts)These are not valid GitHub Actions step keys and will emit invalid YAML unless expanded elsewhere. Mark them to skip serialization.
/// The retry strategy for the step. - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(skip)] pub retry: Option<RetryStrategy>, /// Artifacts produced by the step. - #[serde(skip_serializing_if = "Option::is_none")] + #[serde(skip)] pub artifacts: Option<Artifacts>,If a generator expands them, add docs/comments noting they’re internal-only.
🧹 Nitpick comments (1)
crates/gh-workflow/src/toolchain.rs (1)
204-214: Prefer join over reduce+format for comma-separated listsSimpler and avoids intermediate allocations.
- let toolchain = value - .toolchain - .iter() - .map(|t| match t { - Version::Stable => "stable".to_string(), - Version::Nightly => "nightly".to_string(), - Version::Custom((major, minor, patch)) => { - format!("{major}.{minor}.{patch}") - } - }) - .reduce(|acc, a| format!("{acc}, {a}")); + let toolchain = { + let v: Vec<String> = value.toolchain.iter().map(|t| match t { + Version::Stable => "stable".to_string(), + Version::Nightly => "nightly".to_string(), + Version::Custom((major, minor, patch)) => format!("{major}.{minor}.{patch}"), + }).collect(); + (!v.is_empty()).then(|| v.join(", ")) + };- let components = value - .components - .iter() - .map(|c| c.to_string()) - .reduce(|acc, a| format!("{acc}, {a}")) - .unwrap_or_default(); + let components = value.components.iter().map(|c| c.to_string()).collect::<Vec<_>>().join(", ");Also applies to: 235-241
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
.github/workflows/autofix.yml(1 hunks)crates/gh-workflow-tailcall/src/standard.rs(5 hunks)crates/gh-workflow/src/cargo.rs(2 hunks)crates/gh-workflow/src/release_plz.rs(1 hunks)crates/gh-workflow/src/step.rs(1 hunks)crates/gh-workflow/src/toolchain.rs(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/autofix.yml
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/gh-workflow/src/cargo.rs
- crates/gh-workflow-tailcall/src/standard.rs
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: tusharmath
PR: tailcallhq/gh-workflow#97
File: crates/gh-workflow-tailcall/src/workflow.rs:121-121
Timestamp: 2024-12-10T06:33:21.046Z
Learning: In `crates/gh-workflow-tailcall/src/workflow.rs`, prefer converting standalone functions into methods of the `Workflow` struct when appropriate.
🧬 Code graph analysis (3)
crates/gh-workflow/src/toolchain.rs (1)
crates/gh-workflow/src/step.rs (1)
new(198-203)
crates/gh-workflow/src/release_plz.rs (1)
crates/gh-workflow/src/step.rs (1)
new(198-203)
crates/gh-workflow/src/step.rs (5)
crates/gh-workflow/src/cargo.rs (2)
from(64-93)new(28-36)crates/gh-workflow/src/release_plz.rs (1)
from(75-107)crates/gh-workflow/src/toolchain.rs (2)
from(197-287)new(28-30)crates/gh-workflow/src/env.rs (4)
from(17-19)from(48-52)add(39-42)new(34-36)crates/gh-workflow/src/job.rs (3)
from(25-27)add_env(96-102)new(77-83)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Build and Test
🔇 Additional comments (8)
crates/gh-workflow/src/toolchain.rs (1)
198-202: Constructor switch to Step::new(...).uses(...) looks goodKeeps behavior while aligning with the new builder surface.
crates/gh-workflow/src/release_plz.rs (2)
76-76: Builder-style step construction LGTMMatches the new Step API and keeps semantics.
87-88: No change required for input key casing
The release-plz/[email protected] officially definesmanifest_path(with an underscore) as the input key, so the existing code is correct.crates/gh-workflow/src/step.rs (5)
12-20: serde-transparent Step wrapper is correctLayout and markers are serialization-safe.
185-195: Env merge via Option::take is cleanNice usage to avoid clones.
197-219: Typed Step constructors read wellThe Step::new -> uses/run flow enables fluent chaining and enforces types.
44-65:StepTypeis used incrates/gh-workflow/src/job.rs(in theadd_stepsignature), so it isn’t dead code.Likely an incorrect or invalid review comment.
221-227: actions/checkout@v5 validated v5.0.0 (released Aug 11, 2025) is the latest stable major version; update GitHub Actions runners to v2.327.1+ with Node 24+ to ensure compatibility. [1][3]
| @@ -73,7 +73,7 @@ impl std::fmt::Display for Backend { | |||
|
|
|||
| impl From<Release> for Step<Use> { | |||
| fn from(value: Release) -> Self { | |||
| let mut step = Step::uses("release-plz", "action", "v0.5").name("Release Plz"); | |||
| let mut step = Step::new("Release Plz").uses("release-plz", "action", "v0.5"); | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pin version check
Ensure v0.5 is still current or intentionally pinned; consider semver range or digest if supply-chain constraints apply.
🌐 Web query:
What is the latest recommended tag for release-plz/action as of September 2025, and are there security/bugfix notes since v0.5?
💡 Result:
-
Recommended tag (Sept 2025): use the v0.5 series — e.g. the repository’s latest v0.5 release is v0.5.113 (Sep 1, 2025). [1]
-
Security / bugfix notes since v0.5:
- Documentation recommends pinning the action (either a specific v0.5.x tag or a commit SHA) to mitigate risk of malicious updates; it explains the risk and shows how to pin to a commit SHA. [2]
- The v0.5.* release stream contains many small bugfix/maintenance updates (each v0.5.N updates the bundled release-plz version and dependency bumps); see the releases/changelog for per-release details. [1][3]
Sources:
- release-plz/action releases (latest v0.5.113).
- release-plz GitHub Action security docs (pinning / risks).
- release-plz troubleshooting / changelog notes.
Pin to specific patch or digest
In crates/gh-workflow/src/release_plz.rs:76, change
-let mut step = Step::new("Release Plz").uses("release-plz", "action", "v0.5");
+let mut step = Step::new("Release Plz").uses("release-plz", "action", "v0.5.113");—or alternatively pin to a commit SHA in the v0.5 series to prevent unintended updates and mitigate supply-chain risks.
📝 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.
| let mut step = Step::new("Release Plz").uses("release-plz", "action", "v0.5"); | |
| let mut step = Step::new("Release Plz").uses("release-plz", "action", "v0.5.113"); |
🤖 Prompt for AI Agents
In crates/gh-workflow/src/release_plz.rs around line 76, the action is currently
referenced as the floating tag "v0.5"; replace that with a pinned patch tag or
commit SHA from the v0.5 series (e.g. "v0.5.3" or the full commit SHA) or a
verified digest to avoid implicit updates — update the third argument of
.uses(...) accordingly and commit the exact tag or SHA.
🔄 Refactor: Improve API Ergonomics with Method Chaining
📝 Summary
This PR refactors the method signatures across the gh-workflow crate to improve the developer experience by enabling fluent method chaining. The changes convert setter methods from taking
&mut selfto takingmut selfand returningSelf, allowing for more idiomatic and readable code.🎯 Motivation
The previous API design required multiple mutable variable bindings when building workflows, jobs, and other configurations. This made the code verbose and less ergonomic. By adopting the builder pattern with method chaining, users can now write more concise and expressive code.
🔧 Key Changes
Method Signature Updates
&mut self -> ()tomut self -> Selfacross multiple structs:Workflowstruct methods (add_job,add_event,add_env)Jobstruct methods (add_step,add_env,add_needs)Jobscollection method (insertrenamed toadd)Cargoconfiguration methodsAPI Simplifications
*_whenvariants (add_job_when,add_step_when,add_env_when) as they can now be handled with standard Rust control flowadd_stepsmethod fromJobstruct (users can chain multipleadd_stepcalls)get_idreverse lookup method fromWorkflowtmp_needsfield and complex dependency resolution logicImplementation Improvements
take()for cleaner Option handlinggenerate.rsmodule by removing ~110 lines of complex dependency management code📊 Impact
Before (verbose, multiple mutable bindings):
After (fluent, chainable):
✅ Benefits
🧪 Testing
📦 Affected Crates
gh-workflow: Core library refactoringgh-workflow-tailcall: Updated to use new API patternsThis is a breaking change as it modifies public API signatures. Users will need to update their code to use the new chaining pattern. However, the migration is straightforward and improves code quality.
🔍 Files Changed
crates/gh-workflow/src/workflow.rs- Core workflow building APIscrates/gh-workflow/src/cargo.rs- Cargo configuration methodscrates/gh-workflow/src/event.rs- Event configuration methodscrates/gh-workflow/src/generate.rs- Simplified generation logiccrates/gh-workflow-tailcall/src/standard.rs- Updated integrationcrates/gh-workflow-tailcall/tests/ci.rs- Test updates📚 Migration Guide
Users should update their code to chain methods instead of using mutable references:
mutfrom workflow/job variable declarations*_whenmethodsadd_stepsto chain multipleadd_stepcalls🚀 Next Steps
Summary by CodeRabbit