Cli dx improvements#932
Merged
Merged
Conversation
This commit updates the helix-db package to version 2.0.4 in the Cargo.lock file. Additionally, it refines the CLI documentation, replacing references to the `helix run` command with `helix start` for consistency. The README and contributors' guide have been updated to reflect these changes, improving clarity on building and running local instances. The dashboard command has been removed to streamline the CLI functionality.
This commit introduces a new `skills` command group to the Helix CLI, allowing users to install, update, and list Helix agent skills. The `SkillsAction` enum has been added to define the available actions, and corresponding functionality has been implemented in the `skills` module. Additionally, the CLI now checks for updates to installed skills and provides notifications when updates are available. Tests have been added to ensure proper parsing and functionality of the new commands.
This commit introduces a new error type for handling HTTP 409 conflicts in the Go SDK, along with a function to check for conflict errors. The `HelixError` struct has been updated to include a `StatusCode` field, and the `Exec` method now populates this field when a conflict occurs. Additionally, a new test has been added to validate the conflict error handling. The `Returning` method in the DSL has been refactored to improve the serialization of empty return values. The README has been updated to document these changes, including guidance on handling conflicts in client requests.
Comment on lines
+124
to
+133
| func ExecWithConflictRetry(ctx context.Context, client *helix.Client, build func() helix.Request, out any) error { | ||
| for attempt := 0; attempt < 3; attempt++ { | ||
| err := client.Exec(ctx, build(), out) | ||
| if err == nil || !helix.IsConflict(err) || attempt == 2 { | ||
| return err | ||
| } | ||
| time.Sleep(time.Duration(attempt+1) * 50 * time.Millisecond) | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
Unreachable
return nil after the retry loop
On the third iteration (attempt == 2) the condition attempt == 2 is always true, so return err is always hit from inside the loop. The return nil after the loop is dead code and can mislead readers into thinking a third failed conflict can silently succeed — if the last call still errors, that error should be returned.
Suggested change
| func ExecWithConflictRetry(ctx context.Context, client *helix.Client, build func() helix.Request, out any) error { | |
| for attempt := 0; attempt < 3; attempt++ { | |
| err := client.Exec(ctx, build(), out) | |
| if err == nil || !helix.IsConflict(err) || attempt == 2 { | |
| return err | |
| } | |
| time.Sleep(time.Duration(attempt+1) * 50 * time.Millisecond) | |
| } | |
| return nil | |
| } | |
| func ExecWithConflictRetry(ctx context.Context, client *helix.Client, build func() helix.Request, out any) error { | |
| const maxAttempts = 3 | |
| var err error | |
| for attempt := 0; attempt < maxAttempts; attempt++ { | |
| err = client.Exec(ctx, build(), out) | |
| if err == nil || !helix.IsConflict(err) { | |
| return err | |
| } | |
| if attempt < maxAttempts-1 { | |
| time.Sleep(time.Duration(attempt+1) * 50 * time.Millisecond) | |
| } | |
| } | |
| return err | |
| } |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Greptile Summary
This PR delivers a set of CLI developer-experience improvements: renames
helix runtohelix start(keepingrunas a backward-compatible alias), removes the Dashboard command, adds a newhelix skillssubcommand for managing agent skills, makeshelix chefwork without Cloud auth in non-interactive / headless environments, and adds branded help styling and a 24 h skills-update-check alongside the existing binary update check.helix run→helix start: All internal call sites updated;#[command(alias = "run")]preserves backward compatibility with scripts and agent muscle memory.helix skillscommand: New install / update / list subcommands wrappingnpx skills;helix updateandhelix init/chefautomatically refresh skills after install, with a 24 h GitHub-SHA–based staleness check surfaced on the welcome screen.ErrConflictsentinel,StatusCodeonHelixError, andIsConflicthelper added; emptyReturning()now serializes as[]instead ofnull(bug fix inreturningVars).Important Files Changed
runtostart(with backward alias), removes Dashboard, adds Skills command, adds branded help styling, adds hidden removed-command stubs with friendly errors, and parallelizes update checks sequentially (minor latency concern).HELIX_NO_UPDATE_CHECKopt-out,skills_installed()lockfile probe, andrecord_skills_refreshed()cache reset — all mirroring the existing binary update-check pattern.HELIX_SKIP_CLOUD_AUTHenvironments; snapshot upload is skipped gracefully when credentials are absent, unblocking headless agent runs.helix skillssubcommand (install / update / list) delegating tonpx skills; checks fornpxpresence and returns a helpful error when Node.js is absent.ErrConflictsentinel,StatusCodefield onHelixError, andIsConflicthelper; operator precedence inIsConflictis correct but explicit parentheses would improve readability.returningVarshelper to ensure emptyReturning()calls serialize as[]instead ofnull— bug fix confirmed by new test.ExecWithConflictRetryexample has a deadreturn nilafter the loop that is unreachable and potentially misleading.SkillsActionenum and--skills/--no-skillsflags to bothInitTargetvariants, withskills_override()to resolve subcommand-level flags over parent-level flags.list_skills/skills_list_argsand updates warning messages to referencehelix start; well-tested with new unit tests.Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[helix CLI invoked] --> B{top-level help?} B -- yes --> C[print_help / exit] B -- no --> D[check_for_updates] D --> E[check_skills_update] E --> F[Cli::parse] F --> G{command?} G -- none --> H[display_welcome\nupdate + skills notices] G -- start / run alias --> I[commands::start::run] G -- skills install/update/list --> J[commands::skills::run\nnpx skills ...] G -- init local/cloud --> K[commands::init::run\nmaybe_install_tooling] G -- chef --> L[commands::chef::run\nskip auth if non-interactive] G -- update --> M[commands::update::run\nrefresh_skills_if_installed] G -- compile/check/deploy --> N[friendly removed-command error] J --> O[crate::setup::install_skills\nor list_skills] O --> P[record_skills_refreshed\ndelete skills_cache.toml]Comments Outside Diff (1)
helix-cli/src/main.rs, line 659-661 (link)check_for_updatesandcheck_skills_updateare awaited sequentially beforeCli::parse(). On a cache-miss day each can block up to 10 s (thereqwesttimeout), meaning any command — not just the welcome screen — could stall for up to 20 s once every 24 h. These two independent futures could be parallelized withtokio::join!to cap the overhead at one network round-trip instead of two.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (1): Last reviewed commit: "Enhance Go SDK error handling and query ..." | Re-trigger Greptile