fix: cliproxyapi services - #446
Conversation
…h scripts from start.sh
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. 📝 WalkthroughSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReorganizes cliproxyapi service startup by relocating start script to a scripts subdirectory, updating environment configurations for Darwin and Linux, removing backup/recovery logic, and introducing objectstore directory and config file handling. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (3)
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. Comment |
Summary of ChangesHello @shunkakinoki, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request implements several fixes for the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DRFixes issues with What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix the cliproxyapi services by updating script paths, environment variables, and configuration handling. The changes are generally moving in the right direction, but I've identified a critical portability issue with a hardcoded home directory path in the Nix configuration for launchd. Additionally, the systemd service configuration is missing a necessary dependency (coreutils), which will likely cause failures on Linux. I've also left a comment regarding a potentially ineffective .gitignore entry for clarity.
| ]; | ||
| Environment = { | ||
| PATH = "${lib.makeBinPath [ pkgs.gnused ]}:/opt/homebrew/bin:/usr/local/bin"; | ||
| HOME = "/Users/shunkakinoki"; |
There was a problem hiding this comment.
Hardcoding the HOME environment variable to a specific user's path (/Users/shunkakinoki) makes this configuration non-portable and will cause it to fail for any other user or on a different machine. You should use a variable to dynamically set the correct home directory. In a home-manager module, config.home.homeDirectory is the standard way to reference the user's home directory.
HOME = config.home.homeDirectory;
| ] | ||
| }"; | ||
| ExecStart = "${pkgs.bash}/bin/bash ${./start.sh}"; | ||
| ExecStart = "${pkgs.bash}/bin/bash ${./scripts/start.sh}"; |
There was a problem hiding this comment.
The start.sh script uses mkdir and cp, which are provided by coreutils. The PATH for this systemd service doesn't include coreutils, which will likely cause the script to fail on Linux systems. For consistency with the launchd configuration and to ensure the script runs correctly, you should add pkgs.coreutils to the lib.makeBinPath list in the Environment setting for this service.
| # AI | ||
| .claude | ||
| .devenv.nix | ||
| objectstore |
There was a problem hiding this comment.
The objectstore directory appears to be created within $HOME/.cli-proxy-api, which is typically outside the scope of this Git repository. Consequently, this .gitignore entry may not have any effect. If this entry is intended for a local development workflow where an objectstore directory is created at the repository root, consider adding a comment to clarify its purpose. Otherwise, it could be removed to avoid confusion.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # Also copy to objectstore config location (cliproxyapi uses this for persistence) | ||
| mkdir -p "$CONFIG_DIR/objectstore/config" | ||
| cp "$CONFIG" "$CONFIG_DIR/objectstore/config/config.yaml" |
There was a problem hiding this comment.
On Linux, systemd.user.services.cliproxyapi sets PATH to only gnused and bash in home-manager/services/cliproxyapi/default.nix, so the new mkdir/cp calls added here will fail with command not found (and set -e will abort startup) unless the template is missing. This regression only affects the systemd service because launchd’s PATH was updated to include coreutils, but the Linux PATH was not, so the service can’t create/copy the objectstore config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Pull request overview
This PR fixes several configuration and path issues in the cliproxyapi service. The changes improve the service's config persistence mechanism by replacing backup/recover auth scripts with direct config copying to an objectstore location, while also updating file paths and environment variables.
- Updates script paths from
./start.shto./scripts/start.shin service definitions - Adds
objectstoredirectory to.gitignorefor local development - Replaces backup/recover auth script calls with direct config persistence to objectstore
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
.gitignore |
Adds objectstore directory to prevent tracking local objectstore data |
home-manager/services/cliproxyapi/default.nix |
Updates script paths, adds HOME environment variable, expands PATH with coreutils and /usr/bin for Darwin service |
home-manager/services/cliproxyapi/scripts/start.sh |
Removes backup/recover auth script calls and adds direct config copying to objectstore location for persistence |
Comments suppressed due to low confidence (1)
home-manager/services/cliproxyapi/scripts/start.sh:36
- The config is copied to the objectstore location without checking if the source config file was successfully created or if the copy operation succeeds. If the sed command fails or produces an empty/invalid config, this could silently copy a corrupted config to the persistence location. Consider adding error checking or validation after the sed command and the cp operation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ]; | ||
| Environment = { | ||
| PATH = "${lib.makeBinPath [ pkgs.gnused ]}:/opt/homebrew/bin:/usr/local/bin"; | ||
| HOME = "/Users/shunkakinoki"; |
There was a problem hiding this comment.
The HOME environment variable is hardcoded to a specific user path '/Users/shunkakinoki'. This makes the service configuration non-portable and will fail for any other user or system. Consider using a variable or configuration option that can be set per-user or per-deployment, or remove this if it's not strictly necessary.
| HOME = "/Users/shunkakinoki"; |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Summary by cubic
Fix cliproxyapi service startup and config persistence by correcting script paths, environment variables, and copying config to objectstore. Prevents launch failures and ensures config survives restarts.
Written for commit 9811f22. Summary will update automatically on new commits.