refactor: simplify cliproxyapi scripts - #499
Conversation
|
Warning Rate limit exceeded@shunkakinoki has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 15 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (8)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughReplaces legacy rsync-based multi-source auth syncing with S3-compatible hydration and backup scripts, adds a CLI wrapper, and updates Home Manager Nix service units (systemd/launchd), PATH construction, and activation wiring to run hydrate/backup flows and expose the wrapper on Darwin. Changes
Sequence Diagram(s)sequenceDiagram
participant Launcher as Launcher (systemd / launchd / user)
participant Start as start.sh
participant Hydrate as hydrate.sh
participant Backup as backup.sh
participant Store as ObjectStore (S3‑compatible)
participant CCS as CCS auth dir
Note over Launcher,Start: Activation / startup
Launcher->>Start: Exec startScript
Start-->>Hydrate: Invoke hydrateScript (post-activation / conditional)
Hydrate->>Store: aws s3 sync s3://cliproxyapi/auths/ -> AUTH_DIR
Hydrate->>Store: aws s3 sync s3://cliproxyapi/backup/auths/ -> AUTH_DIR
Hydrate->>CCS: cp *.json -> CCS auth dir
alt Backup triggered (oneshot/agent/WatchPath)
Launcher->>Backup: Exec backupScript
Backup->>Store: aws s3 sync AUTH_DIR -> s3://cliproxyapi/auths/
Backup->>Store: aws s3 sync AUTH_DIR -> s3://cliproxyapi/backup/auths/
Backup->>CCS: cp *.json -> CCS auth dir
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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 refactors the Highlights
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;DRRefactored the What changed?
Description generated by Mesa. Update settings |
6084f3b to
0e49241
Compare
![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 significantly simplifies the cliproxyapi backup and sync architecture by introducing dedicated hydrate and backup scripts, making S3 the definitive source of truth for authentication files. This is a great refactoring that reduces complexity and improves the system's robustness. My review includes suggestions to enhance maintainability by reducing code duplication in the new shell scripts and highlights a potential regression concerning configuration backups. Overall, the changes are a major improvement.
I am having trouble creating individual review comments. Click here to see my feedback.
home-manager/services/cliproxyapi/scripts/start.sh (160-177)
The logic for uploading the generated config.yaml to S3 has been removed. While the main goal of this PR is to simplify auth syncing, this appears to be a potential feature regression. The README.md still indicates s3://cliproxyapi/config/ as a config backup location.
Was the removal of the config backup intentional? If so, the README.md should be updated to avoid confusion. If not, this logic should probably be restored. Backing up the generated config can be useful for debugging and ensuring consistency across environments.
home-manager/services/cliproxyapi/scripts/start.sh (62-63)
The error message for when the cliproxyapi binary is not found has become less helpful. The previous version provided platform-specific guidance on how to resolve the issue.
Consider restoring the more detailed error message to improve the user experience, especially during initial setup.
echo "cliproxyapi not found" >&2
echo "Linux: Docker should be available" >&2
echo "macOS: brew install cliproxyapi" >&2
exit 1
home-manager/services/cliproxyapi/scripts/hydrate.sh (26-40)
The aws s3 sync command blocks for pulling from the primary and backup S3 locations are duplicated. This makes the script harder to maintain.
You can refactor this into a function to improve readability and reduce code duplication.
hydrate_from_s3() {
local s3_path="$1"
local s3_alias="$2"
AWS_ACCESS_KEY_ID="$ACCESS_KEY" \
AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \
@aws@ s3 sync \
--endpoint-url="$ENDPOINT" \
--no-progress \
"$s3_path" \
"$AUTH_DIR/" && echo "✅ Hydrated from R2 $s3_alias" >&2
}
hydrate_from_s3 "s3://cliproxyapi/auths/" "auths/"
hydrate_from_s3 "s3://cliproxyapi/backup/auths/" "backup/auths/"
home-manager/services/cliproxyapi/scripts/backup.sh (4-17)
The logic for sourcing the .env file and the strip_quotes function are duplicated across backup.sh, hydrate.sh, start.sh, and wrapper.sh. This code duplication makes maintenance more difficult, as any changes would need to be applied in multiple places.
Consider extracting this common logic into a shared utility script that can be sourced by all scripts that need it. This would centralize the environment setup and improve maintainability.
home-manager/services/cliproxyapi/scripts/backup.sh (31-45)
The aws s3 sync command blocks for the primary and backup locations are nearly identical. This duplication can make the script harder to maintain.
To improve readability and reduce redundancy, consider refactoring this logic into a function.
sync_to_s3() {
local s3_path="$1"
local s3_alias="$2"
AWS_ACCESS_KEY_ID="$ACCESS_KEY" \
AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \
@aws@ s3 sync \
--endpoint-url="$ENDPOINT" \
--no-progress \
"$AUTH_DIR/" \
"$s3_path" && echo "✅ Backed up to R2 $s3_alias" >&2
}
sync_to_s3 "s3://cliproxyapi/auths/" "auths/"
sync_to_s3 "s3://cliproxyapi/backup/auths/" "backup/auths/"
![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.
Reviewed commit: 6084f3b227
ℹ️ 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".
| export OBJECTSTORE_ACCESS_KEY="$(strip_quotes "${OBJECTSTORE_ACCESS_KEY:-}")" | ||
| export OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")" | ||
|
|
||
| exec /opt/homebrew/bin/cliproxyapi "$@" |
There was a problem hiding this comment.
The new wrapper always execs /opt/homebrew/bin/cliproxyapi, so on Intel macOS (or any setup where Homebrew is installed under /usr/local), invoking cliproxyapi from PATH will fail with “No such file or directory” even though a working binary exists in /usr/local/bin. Because this wrapper is added to home.packages, it will shadow the real binary in PATH, breaking CLI usage like OAuth login on those machines. Consider adding a /usr/local/bin/cliproxyapi fallback (or deferring to command -v cliproxyapi).
Useful? React with 👍 / 👎.
0e49241 to
2496cda
Compare
![mesa-dot-dev[bot]](https://avatars.githubusercontent.com/in/1050077?s=60&v=4){kind=small}
There was a problem hiding this comment.
Performed full review of d0a043f...6084f3b
Analysis
-
Blocking dependency on upstream fix - The PR relies on an upstream fix for a race condition where objectstore can wipe local auth files on startup. Without this fix merged first, auth files could be permanently lost as fallbacks have been removed.
-
Eliminated safety nets - The new architecture removes multiple fallback sources (dotfiles backup, CCS auth dir) that previously provided recovery options if S3/R2 failed or became corrupted.
-
Potential data loss window - Between service start and WatchPaths activation, local changes could be lost as the immediate startup sync safety measure has been removed.
-
Unexplained redundant syncing - Both scripts sync to
auths/andbackup/auths/paths without clear documentation explaining the necessity of this duplication. -
Missing validation mechanisms - The new implementation lacks error handling or validation to ensure syncs succeeded or to detect corrupted file transfers.
Tip
Help
Slash Commands:
/review- Request a full code review/review latest- Review only changes since the last review/describe- Generate PR description. This will update the PR body or issue comment depending on your configuration/help- Get help with Mesa commands and configuration options
0 files reviewed | 5 comments | Edit Agent Settings • Read Docs
| ENDPOINT="$(strip_quotes "${OBJECTSTORE_ENDPOINT:-}")" | ||
| ACCESS_KEY="$(strip_quotes "${OBJECTSTORE_ACCESS_KEY:-}")" | ||
| SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")" | ||
|
|
There was a problem hiding this comment.
If R2 credentials are missing, the script silently exits without error. During home-manager activation, this could leave the service in an uninitialized state. Consider logging this more prominently or failing the activation if credentials are required for the service to function.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#499
File: home-manager/services/cliproxyapi/scripts/hydrate.sh#L19
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
If R2 credentials are missing, the script silently exits without error. During home-manager activation, this could leave the service in an uninitialized state. Consider logging this more prominently or failing the activation if credentials are required for the service to function.
| echo "[$(date)] Backing up auth files..." >&2 | ||
|
|
||
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ |
There was a problem hiding this comment.
Both backup locations (auths/ and backup/auths/) are synced sequentially. If the first sync succeeds but the second fails (network interruption, quota exceeded), you'll have inconsistent state between the two locations. Consider checking if both syncs succeed and alerting on partial failures, or document why eventual consistency between these paths is acceptable.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#499
File: home-manager/services/cliproxyapi/scripts/backup.sh#L33
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
Both backup locations (`auths/` and `backup/auths/`) are synced sequentially. If the first sync succeeds but the second fails (network interruption, quota exceeded), you'll have inconsistent state between the two locations. Consider checking if both syncs succeed and alerting on partial failures, or document why eventual consistency between these paths is acceptable.
| export OBJECTSTORE_ACCESS_KEY="$(strip_quotes "${OBJECTSTORE_ACCESS_KEY:-}")" | ||
| export OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")" | ||
|
|
||
| exec /opt/homebrew/bin/cliproxyapi "$@" |
There was a problem hiding this comment.
The wrapper hardcodes /opt/homebrew/bin/cliproxyapi but doesn't check if the binary exists. If the binary is missing or in a different location (e.g., /usr/local/bin on Intel Macs), the wrapper will fail with a cryptic exec error. Consider adding a fallback path or existence check similar to the logic in start.sh lines 58-62.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#499
File: home-manager/services/cliproxyapi/scripts/wrapper.sh#L17
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The wrapper hardcodes `/opt/homebrew/bin/cliproxyapi` but doesn't check if the binary exists. If the binary is missing or in a different location (e.g., `/usr/local/bin` on Intel Macs), the wrapper will fail with a cryptic exec error. Consider adding a fallback path or existence check similar to the logic in `start.sh` lines 58-62.
| home.activation = lib.optionalAttrs (lib ? hm && lib.hm ? dag) { | ||
| hydrateCliproxyAuths = lib.hm.dag.entryAfter [ "writeBoundary" ] '' | ||
| ${pkgs.bash}/bin/bash ${backupAuthScript} || true | ||
| ${pkgs.bash}/bin/bash ${hydrateScript} || true |
There was a problem hiding this comment.
The activation hook runs hydrate with || true, suppressing all failures. If hydration fails due to network issues or credential problems, the service will start with stale or missing auth files, leading to authentication errors. Consider logging the failure more prominently or using a failure mode that alerts the user.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#499
File: home-manager/services/cliproxyapi/default.nix#L28
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
The activation hook runs hydrate with `|| true`, suppressing all failures. If hydration fails due to network issues or credential problems, the service will start with stale or missing auth files, leading to authentication errors. Consider logging the failure more prominently or using a failure mode that alerts the user.
| }; | ||
| # Watch auth directories for changes - triggers sync immediately | ||
| # NOTE: dotfiles is excluded to prevent circular sync loops | ||
| WatchPaths = [ |
There was a problem hiding this comment.
WatchPaths triggers on any change to the auth directory, including when the upstream service wipes files on startup (the race condition mentioned in the PR description). Until the upstream fix in PR #859 is deployed, this watcher will backup empty directories after file deletions, potentially losing auth data in S3. Consider adding a guard in backup.sh to prevent syncing if the directory suddenly became empty (e.g., check file count against previous state), or ensure this config is only deployed after the upstream fix is merged and deployed.
Agent: 🏛 Architecture •
Prompt for Agent
Task: Address review feedback left on GitHub.
Repository: shunkakinoki/dotfiles#499
File: home-manager/services/cliproxyapi/default.nix#L59
Action: Open this file location in your editor, inspect the highlighted code, and resolve the issue described below.
Feedback:
WatchPaths triggers on any change to the auth directory, including when the upstream service wipes files on startup (the race condition mentioned in the PR description). Until the upstream fix in PR #859 is deployed, this watcher will backup empty directories after file deletions, potentially losing auth data in S3. Consider adding a guard in backup.sh to prevent syncing if the directory suddenly became empty (e.g., check file count against previous state), or ensure this config is only deployed after the upstream fix is merged and deployed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
11-11: Consider extractingstrip_quotesto a shared helper.This function is duplicated verbatim across
wrapper.sh,hydrate.sh,backup.sh, andstart.sh. While acceptable for standalone scripts, consolidating it into a single sourced file would reduce maintenance burden and ensure consistency.home-manager/services/cliproxyapi/scripts/start.sh (1)
39-39: Consider adding existence check beforecd.If
$CONFIG_DIRdoesn't exist,cdwill fail withset -eenabled. While this is likely created by activation scripts, a defensive check could improve error messages.🔎 Optional defensive check
+if [ ! -d "$CONFIG_DIR" ]; then + echo "Config directory not found: $CONFIG_DIR" >&2 + exit 1 +fi cd "$CONFIG_DIR"home-manager/services/cliproxyapi/default.nix (1)
50-64: Consider addingThrottleIntervalto prevent rapid backup triggers.The
WatchPathsmechanism will trigger on every file change. Rapid successive changes (e.g., during auth refresh) could cause excessive S3 sync operations. Adding aThrottleInterval(e.g., 30 seconds) can debounce these triggers.🔎 Proposed change
WatchPaths = [ "${homeDir}/.cli-proxy-api/objectstore/auths" ]; + ThrottleInterval = 30; RunAtLoad = true;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
home-manager/services/cliproxyapi/default.nixhome-manager/services/cliproxyapi/scripts/backup-and-recover.shhome-manager/services/cliproxyapi/scripts/backup-auth.shhome-manager/services/cliproxyapi/scripts/backup.shhome-manager/services/cliproxyapi/scripts/hydrate.shhome-manager/services/cliproxyapi/scripts/start.shhome-manager/services/cliproxyapi/scripts/wrapper.sh
💤 Files with no reviewable changes (2)
- home-manager/services/cliproxyapi/scripts/backup-auth.sh
- home-manager/services/cliproxyapi/scripts/backup-and-recover.sh
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
Use shfmt with 2-space indentation for shell scripts
**/*.{sh,bash}: Use 2 spaces for indentation in shell scripts
Add proper shebang lines to shell scripts
Follow shellcheck recommendations in shell scripts
Document complex commands in shell scripts
Use consistent variable naming in shell scripts
Files:
home-manager/services/cliproxyapi/scripts/hydrate.shhome-manager/services/cliproxyapi/scripts/backup.shhome-manager/services/cliproxyapi/scripts/wrapper.shhome-manager/services/cliproxyapi/scripts/start.sh
**/*.nix
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.nix: Use nixfmt for formatting all Nix files
Document complex configurations with comments in Nix files
**/*.nix: Use 2 spaces for indentation in Nix files
Keep line length under 100 characters in Nix files
Sort attribute sets alphabetically in Nix files
Use consistent spacing around operators in Nix files
Format lists and sets consistently in Nix filesUse treefmt.toml for formatting Nix files
**/*.nix: UsemkOptionfor configurable options in Nix modules
Implement proper typing for all options in Nix modules
Follow the Nix expression language style guide
Files:
home-manager/services/cliproxyapi/default.nix
**/default.nix
📄 CodeRabbit inference engine (CLAUDE.md)
Use
default.nixfiles for module exports
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/*/default.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
Service configurations should be located in
home-manager/services/<name>/with proper service definitions and correct dependency handling
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/**/default.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
Service configurations must include proper service definitions, handle dependencies correctly, and document service parameters
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
home-manager/**/*.nix: Use typed options whenever possible in Nix configurations
Document all configuration options in Nix modules and programs
Follow home-manager's module structure and keep configurations modular
Use proper indentation and formatting in Nix configuration files
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/nix.mdc)
Service configurations in
home-manager/services/should follow systemd service conventions, include proper service dependencies, and have clear documentation for service parameters
Files:
home-manager/services/cliproxyapi/default.nix
🧠 Learnings (8)
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Document all major changes in Nix configurations
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Applies to home-manager/services/**/*.nix : Service configurations in `home-manager/services/` should follow systemd service conventions, include proper service dependencies, and have clear documentation for service parameters
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/services/**/default.nix : Service configurations must include proper service definitions, handle dependencies correctly, and document service parameters
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/programs/**/*.nix : Follow program-specific best practices in program configuration files
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Applies to home-manager/programs/**/*.nix : Program configurations in `home-manager/programs/` should be organized by program name, include all necessary dependencies, use `home.packages` for package installations, and use `programs.<name>` when available in home-manager
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Keep configurations modular across home-manager, hosts, and nix-darwin directories
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:23.224Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:34:23.224Z
Learning: Applies to nix-darwin/**/*.nix : Use Homebrew only for macOS-specific applications
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/services/*/default.nix : Service configurations should be located in `home-manager/services/<name>/` with proper service definitions and correct dependency handling
Applied to files:
home-manager/services/cliproxyapi/default.nix
🧬 Code graph analysis (3)
home-manager/services/cliproxyapi/scripts/hydrate.sh (3)
home-manager/services/cliproxyapi/scripts/backup.sh (1)
strip_quotes(14-14)home-manager/services/cliproxyapi/scripts/start.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
strip_quotes(11-11)
home-manager/services/cliproxyapi/scripts/backup.sh (3)
home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
strip_quotes(14-14)home-manager/services/cliproxyapi/scripts/start.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
strip_quotes(11-11)
home-manager/services/cliproxyapi/scripts/wrapper.sh (3)
home-manager/services/cliproxyapi/scripts/backup.sh (1)
strip_quotes(14-14)home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
strip_quotes(14-14)home-manager/services/cliproxyapi/scripts/start.sh (1)
strip_quotes(15-15)
⏰ 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). (16)
- GitHub Check: Agent
- GitHub Check: cubic · AI code reviewer
- GitHub Check: Mesa Review
- GitHub Check: nix-nixos
- GitHub Check: nix-darwin
- GitHub Check: nix-linux
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim-test
- GitHub Check: lua-neovim
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: shell-test
- GitHub Check: shell-lint
🔇 Additional comments (6)
home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
1-40: LGTM! Clean hydration script.The script properly validates credentials, creates the target directory, and gracefully exits when credentials are missing. The sync order (auths/ then backup/auths/) appears intentional—the backup path provides fallback data that can override the primary path.
home-manager/services/cliproxyapi/scripts/start.sh (1)
22-37: Config generation looks correct.The template-based generation with sed substitutions is clean. The conditional API key insertion for Linux is appropriately scoped.
home-manager/services/cliproxyapi/scripts/backup.sh (1)
1-45: LGTM! Solid backup implementation with proper guards.The script correctly validates credentials and ensures AUTH_DIR has content before syncing. Backing up to both
auths/andbackup/auths/provides good redundancy.Per the PR description, this flow depends on an upstream fix (CLIProxyAPI#859) to prevent a race condition where the objectstore could wipe local auth files on startup. Verify this upstream fix is merged before deploying this change.
home-manager/services/cliproxyapi/default.nix (3)
25-30: Activation hydration correctly uses|| trueto prevent failures.The
|| trueensures activation continues even if hydration fails (e.g., missing credentials or network issues). This is the right approach for a non-critical sync step.
66-81: Linux systemd service configuration looks correct.The service properly depends on
docker.service, includes docker in PATH, and uses the wrapper script forsg dockergroup execution. The restart policy is sensible.
83-99: Systemd path-based backup trigger is well-structured.The path unit watches the auth directory and triggers the oneshot backup service. This mirrors the Darwin WatchPaths behavior appropriately.
As per coding guidelines, consider adding brief documentation for the service parameters. Based on learnings, service configurations should have clear documentation.
| let | ||
| inherit (pkgs) lib; | ||
| # Use build-time HOME for paths that need it at plist generation | ||
| homeDir = builtins.getEnv "HOME"; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if this is a flakes-based project
cd /tmp && find . -maxdepth 2 -name "flake.nix" 2>/dev/null | head -5Repository: shunkakinoki/dotfiles
Length of output: 47
🏁 Script executed:
# Search for flake.nix configuration
fd "flake\.nix" -x cat {} \; 2>/dev/null | head -100Repository: shunkakinoki/dotfiles
Length of output: 2652
🏁 Script executed:
# Check the specific file and how homeDir is used
cat -n home-manager/services/cliproxyapi/default.nix | head -120Repository: shunkakinoki/dotfiles
Length of output: 4184
🏁 Script executed:
# Search for pure evaluation settings or --impure references
rg "impure|getEnv|builtins\." --type=nix -C2Repository: shunkakinoki/dotfiles
Length of output: 4998
🏁 Script executed:
# Check if home-manager modules are evaluated as part of flake outputs
rg "home-manager" flake.nix -A 3 | head -50Repository: shunkakinoki/dotfiles
Length of output: 347
🏁 Script executed:
# Check if there are any tests or checks that would catch this
fd "check\|test" --type f --extension nix | head -20Repository: shunkakinoki/dotfiles
Length of output: 47
🏁 Script executed:
# Look for how home-manager modules are integrated
rg "home.activation|home-manager.users" --type=nix | grep -v "Binary" | head -10Repository: shunkakinoki/dotfiles
Length of output: 1157
🏁 Script executed:
# Check if config.home.homeDirectory is available in home-manager modules
rg "config\.home\.homeDirectory" --type=nix | head -20Repository: shunkakinoki/dotfiles
Length of output: 2475
🏁 Script executed:
# Check what the home-manager standard practice is for getting home directory
rg "homeDirectory" --type=nix | head -15Repository: shunkakinoki/dotfiles
Length of output: 1974
🏁 Script executed:
# Verify the actual execution context - check if home-manager modules need --impure
grep -r "impure\|--pure" . --type=nix 2>/dev/null | head -10Repository: shunkakinoki/dotfiles
Length of output: 47
Use config.home.homeDirectory instead of builtins.getEnv "HOME" for launchd path configuration.
builtins.getEnv "HOME" returns empty string in pure evaluation mode (nix flake check), causing invalid paths in WatchPaths (lines 60–61). Home-manager modules have access to config.home.homeDirectory, which is the standard and reliable way to reference the home directory. Replace homeDir = builtins.getEnv "HOME" with homeDir = config.home.homeDirectory to fix both pure evaluation and align with home-manager conventions used elsewhere in the codebase.
🤖 Prompt for AI Agents
In home-manager/services/cliproxyapi/default.nix around line 4, replace the HOME
environment lookup with the module-provided home directory: change the binding
from using builtins.getEnv "HOME" to using config.home.homeDirectory (this will
fix pure evaluation failures and ensure WatchPaths at lines ~60–61 use valid
paths); update any uses of the homeDir variable accordingly so they reference
the new value.
| export OBJECTSTORE_ACCESS_KEY="$(strip_quotes "${OBJECTSTORE_ACCESS_KEY:-}")" | ||
| export OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")" | ||
|
|
||
| exec /opt/homebrew/bin/cliproxyapi "$@" |
There was a problem hiding this comment.
Missing fallback for Intel Mac path.
The wrapper hard-codes /opt/homebrew/bin/cliproxyapi (Apple Silicon), but start.sh (lines 57-60) includes a fallback to /usr/local/bin/cliproxyapi for Intel Macs. Consider adding the same fallback here for consistency.
🔎 Proposed fix
-exec /opt/homebrew/bin/cliproxyapi "$@"
+if [ -x /opt/homebrew/bin/cliproxyapi ]; then
+ exec /opt/homebrew/bin/cliproxyapi "$@"
+elif [ -x /usr/local/bin/cliproxyapi ]; then
+ exec /usr/local/bin/cliproxyapi "$@"
+else
+ echo "cliproxyapi not found" >&2
+ exit 1
+fi📝 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.
| exec /opt/homebrew/bin/cliproxyapi "$@" | |
| if [ -x /opt/homebrew/bin/cliproxyapi ]; then | |
| exec /opt/homebrew/bin/cliproxyapi "$@" | |
| elif [ -x /usr/local/bin/cliproxyapi ]; then | |
| exec /usr/local/bin/cliproxyapi "$@" | |
| else | |
| echo "cliproxyapi not found" >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In home-manager/services/cliproxyapi/scripts/wrapper.sh around line 17, the
wrapper currently hard-codes exec /opt/homebrew/bin/cliproxyapi "$@" which only
works on Apple Silicon; add the same Intel Mac fallback as start.sh by checking
for /opt/homebrew/bin/cliproxyapi first and if it doesn't exist or isn't
executable, fall back to /usr/local/bin/cliproxyapi, preserving the "$@"
arguments and exiting with a non-zero status if neither binary is found.
There was a problem hiding this comment.
Pull request overview
This PR simplifies the cliproxyapi backup/sync architecture by establishing S3 as the single source of truth and separating concerns into distinct scripts.
- Removes complex bidirectional sync logic with fallback chains to CCS auth dir and dotfiles backup
- Introduces separate hydrate (S3→local) and backup (local→S3) scripts with clear responsibilities
- Simplifies service startup by removing atomic swap logic and temp directory management
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| scripts/wrapper.sh | New CLI wrapper that loads environment variables and executes the binary |
| scripts/hydrate.sh | New script to pull auth files from S3 to local cache on activation |
| scripts/backup.sh | New script to push auth files from local cache to S3 when files change |
| scripts/start.sh | Simplified startup script removes complex sync logic, atomic swaps, and fallback chains |
| scripts/backup-auth.sh | Removed - old bidirectional sync script with multiple fallback sources |
| scripts/backup-and-recover.sh | Removed - old wrapper script for backup operations |
| default.nix | Updated to use new hydrate/backup scripts, adds CLI wrapper package, simplifies service configuration |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$ENDPOINT" \ | ||
| --no-progress \ | ||
| "$AUTH_DIR/" \ | ||
| "s3://cliproxyapi/auths/" && echo "✅ Backed up to S3 auths/" >&2 | ||
|
|
||
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$ENDPOINT" \ | ||
| --no-progress \ | ||
| "$AUTH_DIR/" \ | ||
| "s3://cliproxyapi/backup/auths/" && echo "✅ Backed up to S3 backup/auths/" >&2 | ||
|
|
There was a problem hiding this comment.
Both sync operations use "&&" followed by echo for success messages. If either sync fails, the script will exit due to "set -euo pipefail" before attempting the second sync to the backup location. This means the backup won't occur if the primary sync fails. Consider using "|| echo" for error messages or handling failures more gracefully to ensure both locations receive backups.
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | |
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="$ENDPOINT" \ | |
| --no-progress \ | |
| "$AUTH_DIR/" \ | |
| "s3://cliproxyapi/auths/" && echo "✅ Backed up to S3 auths/" >&2 | |
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | |
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="$ENDPOINT" \ | |
| --no-progress \ | |
| "$AUTH_DIR/" \ | |
| "s3://cliproxyapi/backup/auths/" && echo "✅ Backed up to S3 backup/auths/" >&2 | |
| primary_sync_failed=0 | |
| if ! AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | |
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="$ENDPOINT" \ | |
| --no-progress \ | |
| "$AUTH_DIR/" \ | |
| "s3://cliproxyapi/auths/"; then | |
| echo "❌ Failed to back up to S3 auths/" >&2 | |
| primary_sync_failed=1 | |
| else | |
| echo "✅ Backed up to S3 auths/" >&2 | |
| fi | |
| backup_sync_failed=0 | |
| if ! AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | |
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="$ENDPOINT" \ | |
| --no-progress \ | |
| "$AUTH_DIR/" \ | |
| "s3://cliproxyapi/backup/auths/"; then | |
| echo "❌ Failed to back up to S3 backup/auths/" >&2 | |
| backup_sync_failed=1 | |
| else | |
| echo "✅ Backed up to S3 backup/auths/" >&2 | |
| fi | |
| if [ "$primary_sync_failed" -ne 0 ] || [ "$backup_sync_failed" -ne 0 ]; then | |
| exit 1 | |
| fi |
| strip_quotes() { local v="$1"; v="${v%\"}"; v="${v#\"}"; printf '%s' "$v"; } | ||
| export OBJECTSTORE_ENDPOINT="$(strip_quotes "${OBJECTSTORE_ENDPOINT:-}")" | ||
| export OBJECTSTORE_BUCKET="$(strip_quotes "${OBJECTSTORE_BUCKET:-cliproxyapi}")" | ||
| export OBJECTSTORE_ACCESS_KEY="$(strip_quotes "${OBJECTSTORE_ACCESS_KEY:-}")" | ||
| export OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")" |
There was a problem hiding this comment.
The strip_quotes function is defined identically in wrapper.sh, start.sh, hydrate.sh, and backup.sh. This code duplication makes maintenance harder. Consider extracting this to a shared utility script that can be sourced by all scripts.
| export OBJECTSTORE_ACCESS_KEY="$(strip_quotes "${OBJECTSTORE_ACCESS_KEY:-}")" | ||
| export OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")" | ||
|
|
||
| exec /opt/homebrew/bin/cliproxyapi "$@" |
There was a problem hiding this comment.
The hardcoded path "/opt/homebrew/bin/cliproxyapi" assumes a specific Homebrew installation location. On Intel Macs, Homebrew is typically installed in "/usr/local", which is handled later in start.sh. However, for CLI usage via wrapper.sh, Intel Mac users would need to manually adjust this path or the wrapper won't work. Consider making this path configurable or checking both locations like start.sh does.
| exec /opt/homebrew/bin/cliproxyapi "$@" | |
| # Determine cliproxyapi binary location | |
| if [ -n "${CLIPROXYAPI_BIN-}" ] && [ -x "${CLIPROXYAPI_BIN}" ]; then | |
| : # use CLIPROXYAPI_BIN as provided | |
| elif [ -x /opt/homebrew/bin/cliproxyapi ]; then | |
| CLIPROXYAPI_BIN=/opt/homebrew/bin/cliproxyapi | |
| elif [ -x /usr/local/bin/cliproxyapi ]; then | |
| CLIPROXYAPI_BIN=/usr/local/bin/cliproxyapi | |
| else | |
| if command -v cliproxyapi >/dev/null 2>&1; then | |
| CLIPROXYAPI_BIN="$(command -v cliproxyapi)" | |
| else | |
| echo "Error: cliproxyapi binary not found. Set CLIPROXYAPI_BIN or install cliproxyapi in /opt/homebrew/bin, /usr/local/bin, or your PATH." >&2 | |
| exit 1 | |
| fi | |
| fi | |
| exec "${CLIPROXYAPI_BIN}" "$@" |
| echo 'cliproxyapi not found' >&2 | ||
| echo 'Linux: Docker should be available' >&2 | ||
| echo 'macOS: brew install cliproxyapi' >&2 | ||
| echo "cliproxyapi not found" >&2 |
There was a problem hiding this comment.
The error message has been simplified to only show "cliproxyapi not found" without the helpful installation instructions that were present before. The removed messages ("Linux: Docker should be available" and "macOS: brew install cliproxyapi") provided useful troubleshooting guidance. Consider keeping or improving these error messages to help users understand how to resolve the issue.
| echo "cliproxyapi not found" >&2 | |
| echo "cliproxyapi not found." >&2 | |
| case "$(uname)" in | |
| Darwin) | |
| echo "macOS: Install via Homebrew, for example: brew install cliproxyapi" >&2 | |
| ;; | |
| Linux) | |
| echo "Linux: cliproxyapi is expected to run via Docker. Ensure Docker is installed and available in your PATH." >&2 | |
| ;; | |
| *) | |
| echo "Please install cliproxyapi and ensure it is available on your PATH." >&2 | |
| ;; | |
| esac |
| }; | ||
| # Watch auth directories for changes - triggers sync immediately | ||
| # NOTE: dotfiles is excluded to prevent circular sync loops | ||
| WatchPaths = [ |
There was a problem hiding this comment.
The WatchPaths configuration watches the auth directory, which will trigger the backup script on any file change. However, according to the PR description, there's a race condition where "the objectstore wipes local auth files on startup, triggering file watcher events that delete files from S3." While the PR mentions this depends on an upstream fix, the current implementation still has this vulnerability. If files are deleted locally and the watcher triggers, the backup will sync the deletion to S3. Consider adding logic to detect mass deletions or implement a grace period after service start before enabling backups.
|
|
||
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$ENDPOINT" \ | ||
| --no-progress \ | ||
| "s3://cliproxyapi/auths/" \ | ||
| "$AUTH_DIR/" && echo "✅ Hydrated from S3 auths/" >&2 | ||
|
|
||
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$ENDPOINT" \ | ||
| --no-progress \ | ||
| "s3://cliproxyapi/backup/auths/" \ |
There was a problem hiding this comment.
Both sync operations use "&&" followed by echo, which means if the first sync fails, the script will exit due to "set -euo pipefail" before attempting the second sync. This prevents the backup location from being used as a fallback. Consider using "|| true" or handling failures more gracefully to ensure both locations are attempted.
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$ENDPOINT" \ | ||
| --no-progress \ | ||
| "s3://cliproxyapi/auths/" \ | ||
| "$AUTH_DIR/" && echo "✅ Hydrated from S3 auths/" >&2 | ||
|
|
||
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$ENDPOINT" \ | ||
| --no-progress \ | ||
| "s3://cliproxyapi/backup/auths/" \ |
There was a problem hiding this comment.
The bucket name "cliproxyapi" is hardcoded in multiple sync commands. This is inconsistent with the parameterized approach used in wrapper.sh and start.sh where OBJECTSTORE_BUCKET can be configured. Consider using a BUCKET variable derived from the environment variable for consistency and configurability.
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$ENDPOINT" \ | ||
| --no-progress \ | ||
| "$AUTH_DIR/" \ | ||
| "s3://cliproxyapi/auths/" && echo "✅ Backed up to S3 auths/" >&2 | ||
|
|
||
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$ENDPOINT" \ | ||
| --no-progress \ | ||
| "$AUTH_DIR/" \ |
There was a problem hiding this comment.
The bucket name "cliproxyapi" is hardcoded in multiple sync commands. This is inconsistent with the parameterized approach used in wrapper.sh and start.sh where OBJECTSTORE_BUCKET can be configured. Consider using a BUCKET variable derived from the environment variable for consistency and configurability.
| export OBJECTSTORE_BUCKET="$(strip_quotes "${OBJECTSTORE_BUCKET:-cliproxyapi}")" | ||
| export OBJECTSTORE_ACCESS_KEY="$(strip_quotes "${OBJECTSTORE_ACCESS_KEY:-}")" | ||
| export OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")" | ||
|
|
There was a problem hiding this comment.
The bucket name is hardcoded as "cliproxyapi" in the default value, but this bucket name is also hardcoded in multiple places in hydrate.sh and backup.sh. If the bucket name needs to change, it would require updates in multiple locations. Consider extracting this to a variable or ensuring consistency through environment variable usage only.
| export OBJECTSTORE_BUCKET="$(strip_quotes "${OBJECTSTORE_BUCKET:-cliproxyapi}")" | |
| export OBJECTSTORE_ACCESS_KEY="$(strip_quotes "${OBJECTSTORE_ACCESS_KEY:-}")" | |
| export OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")" | |
| export OBJECTSTORE_BUCKET="$(strip_quotes "${OBJECTSTORE_BUCKET:-}")" | |
| export OBJECTSTORE_ACCESS_KEY="$(strip_quotes "${OBJECTSTORE_ACCESS_KEY:-}")" | |
| export OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")" | |
| if [ -z "$OBJECTSTORE_BUCKET" ]; then | |
| echo "ERROR: OBJECTSTORE_BUCKET environment variable must be set" >&2 | |
| exit 1 | |
| fi |
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$ENDPOINT" \ | ||
| --no-progress \ | ||
| "s3://cliproxyapi/auths/" \ | ||
| "$AUTH_DIR/" && echo "✅ Hydrated from S3 auths/" >&2 | ||
|
|
||
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="$ENDPOINT" \ | ||
| --no-progress \ | ||
| "s3://cliproxyapi/backup/auths/" \ | ||
| "$AUTH_DIR/" && echo "✅ Hydrated from S3 backup/auths/" >&2 |
There was a problem hiding this comment.
The sync operations pull from both "s3://cliproxyapi/auths/" and "s3://cliproxyapi/backup/auths/" into the same directory. If files exist in both locations with the same name but different content, the second sync will overwrite files from the first. This could lead to unpredictable behavior. Consider documenting which location takes precedence or adding logic to handle conflicts.
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | |
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="$ENDPOINT" \ | |
| --no-progress \ | |
| "s3://cliproxyapi/auths/" \ | |
| "$AUTH_DIR/" && echo "✅ Hydrated from S3 auths/" >&2 | |
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | |
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="$ENDPOINT" \ | |
| --no-progress \ | |
| "s3://cliproxyapi/backup/auths/" \ | |
| "$AUTH_DIR/" && echo "✅ Hydrated from S3 backup/auths/" >&2 | |
| # First, hydrate from backup so we have any missing auths restored. | |
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | |
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="$ENDPOINT" \ | |
| --no-progress \ | |
| "s3://cliproxyapi/backup/auths/" \ | |
| "$AUTH_DIR/" && echo "✅ Hydrated from S3 backup/auths/" >&2 | |
| # Then hydrate from primary; primary auths take precedence over backup on conflicts. | |
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | |
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="$ENDPOINT" \ | |
| --no-progress \ | |
| "s3://cliproxyapi/auths/" \ | |
| "$AUTH_DIR/" && echo "✅ Hydrated from S3 auths/" >&2 |
2496cda to
766a796
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
home-manager/services/cliproxyapi/scripts/backup.sh (1)
32-46: Sequential syncs may leave inconsistent state on partial failure.This concern was raised in a previous review and remains unaddressed. If the first sync succeeds but the second fails, the two S3 locations will be inconsistent. Consider tracking exit codes and logging partial failures.
home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
17-17: Missing fallback for Intel Mac path.This issue has been flagged in previous reviews. The wrapper hard-codes
/opt/homebrew/bin/cliproxyapibutstart.sh(lines 57-60) includes a fallback to/usr/local/bin/cliproxyapifor Intel Macs.home-manager/services/cliproxyapi/default.nix (3)
4-4: Useconfig.home.homeDirectoryinstead ofbuiltins.getEnv "HOME".This issue was flagged in a previous review.
builtins.getEnv "HOME"returns an empty string in pure evaluation mode, which will cause invalid paths inWatchPaths(lines 60-61).
27-29: Hydration failures are silently suppressed.This concern was raised in a previous review and remains unaddressed. If hydration fails, the service starts with stale or missing auth files.
59-62: WatchPaths race condition with upstream service.This concern was raised in a previous review. Until the upstream fix (CLIProxyAPI#859) is deployed, the watcher may backup empty directories after the service wipes files on startup.
🧹 Nitpick comments (3)
home-manager/services/cliproxyapi/scripts/backup.sh (2)
15-15: Consider extractingstrip_quotesto a shared utility.This function is duplicated verbatim in
hydrate.sh,start.sh, andwrapper.sh. Consider sourcing it from a common file to reduce duplication and ensure consistency.
48-50: Silent failure on CCS copy may hide issues.The
|| truesuppresses all errors from thecpcommand. If JSON files exist but copying fails for other reasons (permissions, disk full), this will silently proceed. Consider logging a warning on failure.🔎 Proposed improvement
# Also sync back to CCS auth dir so ccs can find the tokens mkdir -p "$CCS_AUTH_DIR" -cp -u "$AUTH_DIR"/*.json "$CCS_AUTH_DIR/" 2>/dev/null || true +if ! cp -u "$AUTH_DIR"/*.json "$CCS_AUTH_DIR/" 2>/dev/null; then + echo "⚠️ No JSON files to copy to CCS or copy failed" >&2 +fihome-manager/services/cliproxyapi/default.nix (1)
98-104: Consider addingAfterdependency on main service.The backup service could run before the main cliproxyapi service is ready, potentially backing up incomplete state. Consider adding
After = [ "cliproxyapi.service" ];to ensure proper ordering.🔎 Proposed fix
systemd.user.services.cliproxyapi-backup = lib.mkIf pkgs.stdenv.isLinux { - Unit.Description = "CLIProxyAPI auth backup"; + Unit = { + Description = "CLIProxyAPI auth backup"; + After = [ "cliproxyapi.service" ]; + }; Service = { Type = "oneshot"; ExecStart = "${pkgs.bash}/bin/bash ${backupScript}"; Environment = "PATH=${lib.makeBinPath [ pkgs.bash pkgs.awscli2 pkgs.coreutils ]}"; }; };
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
home-manager/services/cliproxyapi/default.nixhome-manager/services/cliproxyapi/scripts/backup-and-recover.shhome-manager/services/cliproxyapi/scripts/backup-auth.shhome-manager/services/cliproxyapi/scripts/backup.shhome-manager/services/cliproxyapi/scripts/hydrate.shhome-manager/services/cliproxyapi/scripts/start.shhome-manager/services/cliproxyapi/scripts/wrapper.sh
💤 Files with no reviewable changes (2)
- home-manager/services/cliproxyapi/scripts/backup-and-recover.sh
- home-manager/services/cliproxyapi/scripts/backup-auth.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- home-manager/services/cliproxyapi/scripts/hydrate.sh
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
Use shfmt with 2-space indentation for shell scripts
**/*.{sh,bash}: Use 2 spaces for indentation in shell scripts
Add proper shebang lines to shell scripts
Follow shellcheck recommendations in shell scripts
Document complex commands in shell scripts
Use consistent variable naming in shell scripts
Files:
home-manager/services/cliproxyapi/scripts/start.shhome-manager/services/cliproxyapi/scripts/wrapper.shhome-manager/services/cliproxyapi/scripts/backup.sh
**/*.nix
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.nix: Use nixfmt for formatting all Nix files
Document complex configurations with comments in Nix files
**/*.nix: Use 2 spaces for indentation in Nix files
Keep line length under 100 characters in Nix files
Sort attribute sets alphabetically in Nix files
Use consistent spacing around operators in Nix files
Format lists and sets consistently in Nix filesUse treefmt.toml for formatting Nix files
**/*.nix: UsemkOptionfor configurable options in Nix modules
Implement proper typing for all options in Nix modules
Follow the Nix expression language style guide
Files:
home-manager/services/cliproxyapi/default.nix
**/default.nix
📄 CodeRabbit inference engine (CLAUDE.md)
Use
default.nixfiles for module exports
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/*/default.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
Service configurations should be located in
home-manager/services/<name>/with proper service definitions and correct dependency handling
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/**/default.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
Service configurations must include proper service definitions, handle dependencies correctly, and document service parameters
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
home-manager/**/*.nix: Use typed options whenever possible in Nix configurations
Document all configuration options in Nix modules and programs
Follow home-manager's module structure and keep configurations modular
Use proper indentation and formatting in Nix configuration files
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/nix.mdc)
Service configurations in
home-manager/services/should follow systemd service conventions, include proper service dependencies, and have clear documentation for service parameters
Files:
home-manager/services/cliproxyapi/default.nix
🧠 Learnings (14)
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Document all major changes in Nix configurations
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Applies to home-manager/services/**/*.nix : Service configurations in `home-manager/services/` should follow systemd service conventions, include proper service dependencies, and have clear documentation for service parameters
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/services/**/default.nix : Service configurations must include proper service definitions, handle dependencies correctly, and document service parameters
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/programs/**/*.nix : Follow program-specific best practices in program configuration files
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Applies to home-manager/programs/**/*.nix : Program configurations in `home-manager/programs/` should be organized by program name, include all necessary dependencies, use `home.packages` for package installations, and use `programs.<name>` when available in home-manager
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/programs/*/default.nix : Program configurations should be located in `home-manager/programs/<name>/` and use home-manager's built-in modules when available
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Test configurations before committing using `nix flake check` and `home-manager build --show-trace`
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Follow home-manager's module structure and keep configurations modular
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/services/*/default.nix : Service configurations should be located in `home-manager/services/<name>/` with proper service definitions and correct dependency handling
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Use proper indentation and formatting in Nix configuration files
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Use typed options whenever possible in Nix configurations
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Keep configurations modular across home-manager, hosts, and nix-darwin directories
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Document all configuration options in Nix modules and programs
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:23.224Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:34:23.224Z
Learning: Applies to nix-darwin/**/*.nix : Use Homebrew only for macOS-specific applications
Applied to files:
home-manager/services/cliproxyapi/default.nix
🧬 Code graph analysis (3)
home-manager/services/cliproxyapi/scripts/start.sh (3)
home-manager/services/cliproxyapi/scripts/backup.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
strip_quotes(11-11)
home-manager/services/cliproxyapi/scripts/wrapper.sh (3)
home-manager/services/cliproxyapi/scripts/backup.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/start.sh (1)
strip_quotes(15-15)
home-manager/services/cliproxyapi/scripts/backup.sh (3)
home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/start.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
strip_quotes(11-11)
⏰ 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). (13)
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim
- GitHub Check: lua-neovim-test
- GitHub Check: nix-linux
- GitHub Check: nix-darwin
- GitHub Check: nix-nixos
- GitHub Check: shell-lint
- GitHub Check: shell-test
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
🔇 Additional comments (7)
home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
1-9: LGTM!Strict mode and environment loading are properly implemented.
home-manager/services/cliproxyapi/scripts/start.sh (3)
4-19: LGTM!The simplified environment variable handling with
strip_quotesis clean and consistent with the other scripts.
22-37: LGTM!Config generation logic is straightforward. The
@sed@placeholder will be substituted by Nix.
56-64: Good fallback handling for macOS binary paths.This correctly handles both Apple Silicon (
/opt/homebrew/bin) and Intel Mac (/usr/local/bin) paths, which the wrapper script should also implement.home-manager/services/cliproxyapi/default.nix (3)
6-12: LGTM!The
pkgs.replaceVarspattern correctly substitutes the@aws@placeholder with the full path to awscli2.
22-22: Verify wrapper.sh has no substitution placeholders.Using
builtins.readFileinstead ofpkgs.replaceVarsis correct here sincewrapper.shdoesn't contain any@placeholder@patterns. However, the wrapper directly calls/opt/homebrew/bin/cliproxyapiwithout the Intel Mac fallback noted in previous reviews.
69-84: LGTM!The systemd service configuration follows conventions with proper
After/Wantsdependencies for docker.service and useslib.makeBinPathfor PATH construction. Based on learnings, service configurations should follow systemd conventions and include proper dependencies.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="home-manager/services/cliproxyapi/scripts/hydrate.sh">
<violation number="1" location="home-manager/services/cliproxyapi/scripts/hydrate.sh:28">
P1: If the first `aws s3 sync` (lines 25-31) fails, `set -e` causes the script to exit before attempting the second sync from `backup/auths/`. This defeats the purpose of having redundant R2 paths. Consider allowing the first sync to fail gracefully so the backup path can still be attempted.</violation>
</file>
<file name="home-manager/services/cliproxyapi/default.nix">
<violation number="1" location="home-manager/services/cliproxyapi/default.nix:22">
P2: The `wrapper.sh` script hardcodes `/opt/homebrew/bin/cliproxyapi` which only works on Apple Silicon Macs. Intel Macs install Homebrew to `/usr/local/bin`. Consider adding a fallback like `start.sh` does, or use `pkgs.replaceVars` to substitute the path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ | ||
| AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ | ||
| @aws@ s3 sync \ |
There was a problem hiding this comment.
P1: If the first aws s3 sync (lines 25-31) fails, set -e causes the script to exit before attempting the second sync from backup/auths/. This defeats the purpose of having redundant R2 paths. Consider allowing the first sync to fail gracefully so the backup path can still be attempted.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/cliproxyapi/scripts/hydrate.sh, line 28:
<comment>If the first `aws s3 sync` (lines 25-31) fails, `set -e` causes the script to exit before attempting the second sync from `backup/auths/`. This defeats the purpose of having redundant R2 paths. Consider allowing the first sync to fail gracefully so the backup path can still be attempted.</comment>
<file context>
@@ -0,0 +1,40 @@
+
+AWS_ACCESS_KEY_ID="$ACCESS_KEY" \
+AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \
+@aws@ s3 sync \
+ --endpoint-url="$ENDPOINT" \
+ --no-progress \
</file context>
✅ Addressed in d43ee4c
| bash = "${pkgs.bash}/bin/bash"; | ||
| backupAuthScript = backupAuthScript; | ||
| }; | ||
| cliWrapper = pkgs.writeShellScriptBin "cliproxyapi" (builtins.readFile ./scripts/wrapper.sh); |
There was a problem hiding this comment.
P2: The wrapper.sh script hardcodes /opt/homebrew/bin/cliproxyapi which only works on Apple Silicon Macs. Intel Macs install Homebrew to /usr/local/bin. Consider adding a fallback like start.sh does, or use pkgs.replaceVars to substitute the path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At home-manager/services/cliproxyapi/default.nix, line 22:
<comment>The `wrapper.sh` script hardcodes `/opt/homebrew/bin/cliproxyapi` which only works on Apple Silicon Macs. Intel Macs install Homebrew to `/usr/local/bin`. Consider adding a fallback like `start.sh` does, or use `pkgs.replaceVars` to substitute the path.</comment>
<file context>
@@ -1,58 +1,44 @@
- bash = "${pkgs.bash}/bin/bash";
- backupAuthScript = backupAuthScript;
- };
+ cliWrapper = pkgs.writeShellScriptBin "cliproxyapi" (builtins.readFile ./scripts/wrapper.sh);
in
{
</file context>
766a796 to
d43ee4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (11)
home-manager/services/cliproxyapi/scripts/wrapper.sh (2)
11-11: Consider extracting duplicatedstrip_quotesto a shared utility.The
strip_quotesfunction is duplicated identically across wrapper.sh, hydrate.sh, backup.sh, and start.sh. Extracting this to a shared utility script would improve maintainability.
17-17: Add fallback for Intel Mac path to prevent CLI breakage.The hardcoded
/opt/homebrew/bin/cliproxyapipath only works on Apple Silicon. On Intel Macs where Homebrew installs to/usr/local, this wrapper will fail with "No such file or directory". Since this wrapper is added tohome.packages, it shadows the real binary in PATH and breaks CLI usage (e.g., OAuth login) on Intel machines.🔎 Proposed fix matching start.sh pattern
-exec /opt/homebrew/bin/cliproxyapi "$@" +if [ -x /opt/homebrew/bin/cliproxyapi ]; then + exec /opt/homebrew/bin/cliproxyapi "$@" +elif [ -x /usr/local/bin/cliproxyapi ]; then + exec /usr/local/bin/cliproxyapi "$@" +else + echo "cliproxyapi not found" >&2 + exit 1 +fihome-manager/services/cliproxyapi/scripts/hydrate.sh (3)
20-23: Document the intentional silent exit on missing credentials.The script exits silently (exit 0) when S3 credentials are missing, which allows home-manager activation to succeed during initial setup. Consider adding a comment explaining this is intentional to prevent confusion during troubleshooting.
27-33: Parameterize hardcoded bucket name for consistency.The bucket name "cliproxyapi" is hardcoded here, while wrapper.sh and start.sh use
OBJECTSTORE_BUCKETwith a default value. For consistency and configurability, consider using the environment variable.🔎 Proposed fix
+BUCKET="$(strip_quotes "${OBJECTSTORE_BUCKET:-cliproxyapi}")" + AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ @aws@ s3 sync \ --endpoint-url="$ENDPOINT" \ --no-progress \ - "s3://cliproxyapi/auths/" \ + "s3://$BUCKET/auths/" \ "$AUTH_DIR/" && echo "✅ Hydrated from S3 auths/" >&2
27-41: Clarify precedence when syncing from both S3 locations.Both
s3://cliproxyapi/auths/ands3://cliproxyapi/backup/auths/are synced to the same local directory. If files exist in both locations with the same name, the backup sync (lines 35-41) will overwrite files from the primary sync (lines 27-33). Consider documenting which location should take precedence or reversing the order if primary should win.home-manager/services/cliproxyapi/scripts/backup.sh (2)
32-38: Parameterize hardcoded bucket name for consistency.The bucket name "cliproxyapi" is hardcoded, while wrapper.sh and start.sh use
OBJECTSTORE_BUCKET. Consider using the environment variable as suggested for hydrate.sh.
32-46: Ensure both backup locations are attempted despite failures.The sequential syncs use
&&which, combined withset -euo pipefail, means if the first sync fails, the script exits before attempting the second backup location. This defeats the redundancy purpose. Consider capturing exit codes and attempting both syncs, failing only if both fail.🔎 Proposed fix
-AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ -AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ -@aws@ s3 sync \ - --endpoint-url="$ENDPOINT" \ - --no-progress \ - "$AUTH_DIR/" \ - "s3://cliproxyapi/auths/" && echo "✅ Backed up to S3 auths/" >&2 - -AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ -AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ -@aws@ s3 sync \ - --endpoint-url="$ENDPOINT" \ - --no-progress \ - "$AUTH_DIR/" \ - "s3://cliproxyapi/backup/auths/" && echo "✅ Backed up to S3 backup/auths/" >&2 +primary_failed=0 +if AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ + AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ + @aws@ s3 sync \ + --endpoint-url="$ENDPOINT" \ + --no-progress \ + "$AUTH_DIR/" \ + "s3://cliproxyapi/auths/"; then + echo "✅ Backed up to S3 auths/" >&2 +else + echo "❌ Failed to back up to S3 auths/" >&2 + primary_failed=1 +fi + +backup_failed=0 +if AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ + AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ + @aws@ s3 sync \ + --endpoint-url="$ENDPOINT" \ + --no-progress \ + "$AUTH_DIR/" \ + "s3://cliproxyapi/backup/auths/"; then + echo "✅ Backed up to S3 backup/auths/" >&2 +else + echo "❌ Failed to back up to S3 backup/auths/" >&2 + backup_failed=1 +fi + +if [ "$primary_failed" -ne 0 ] && [ "$backup_failed" -ne 0 ]; then + echo "❌ Both backup locations failed" >&2 + exit 1 +fihome-manager/services/cliproxyapi/scripts/start.sh (1)
56-64: Consider more helpful error message for missing binary.The fallback logic correctly handles both Apple Silicon and Intel Mac paths. However, the error message at line 62 was simplified from previous versions that provided installation instructions. Consider restoring platform-specific guidance to help users resolve the issue.
🔎 Proposed enhancement
else - echo "cliproxyapi not found" >&2 + echo "cliproxyapi not found." >&2 + if [ "$(uname)" = "Darwin" ]; then + echo "Install via Homebrew: brew install cliproxyapi" >&2 + fi exit 1 fihome-manager/services/cliproxyapi/default.nix (3)
4-4: Useconfig.home.homeDirectoryinstead ofbuiltins.getEnv "HOME".This issue has been comprehensively covered in previous reviews. The
builtins.getEnv "HOME"call returns an empty string in pure evaluation mode, causing invalid paths inWatchPaths(lines 60-61) andEnvironment.HOME(lines 40, 56). Replace withconfig.home.homeDirectoryto align with home-manager conventions used throughout the codebase.
25-30: Activation hook suppresses hydration failures.This concern has been comprehensively covered in previous reviews. The
|| trueat line 28 suppresses all hydration failures, potentially causing the service to start with stale or missing auth files.
50-67: WatchPaths race condition risk.This concern has been thoroughly documented by multiple previous reviewers. The WatchPaths configuration can trigger backups when the upstream service wipes auth files on startup, potentially causing data loss in S3 until the upstream fix (PR #859) is deployed.
🧹 Nitpick comments (3)
home-manager/services/cliproxyapi/README.md (2)
23-23: Add language specifiers to fenced code blocks.The fenced code blocks at lines 23 (directory tree) and 39 (data flow diagram) are missing language specifiers. Consider adding
```textto satisfy markdown linting and improve rendering consistency.Also applies to: 39-39
108-108: Wrap bare URL in angle brackets or markdown link.The bare URL at line 108 should be wrapped in angle brackets
<...>or formatted as a markdown link to comply with markdown best practices.home-manager/services/cliproxyapi/default.nix (1)
32-32: Consider CLI wrapper availability on Linux.The
cliWrapperis only added tohome.packageson Darwin. If users need CLI access on Linux systems (outside the Docker service), consider making this available cross-platform. However, if the architecture intentionally restricts Linux to Docker-only usage, this is fine.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (8)
home-manager/services/cliproxyapi/README.mdhome-manager/services/cliproxyapi/default.nixhome-manager/services/cliproxyapi/scripts/backup-and-recover.shhome-manager/services/cliproxyapi/scripts/backup-auth.shhome-manager/services/cliproxyapi/scripts/backup.shhome-manager/services/cliproxyapi/scripts/hydrate.shhome-manager/services/cliproxyapi/scripts/start.shhome-manager/services/cliproxyapi/scripts/wrapper.sh
💤 Files with no reviewable changes (2)
- home-manager/services/cliproxyapi/scripts/backup-auth.sh
- home-manager/services/cliproxyapi/scripts/backup-and-recover.sh
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
Use shfmt with 2-space indentation for shell scripts
**/*.{sh,bash}: Use 2 spaces for indentation in shell scripts
Add proper shebang lines to shell scripts
Follow shellcheck recommendations in shell scripts
Document complex commands in shell scripts
Use consistent variable naming in shell scripts
Files:
home-manager/services/cliproxyapi/scripts/hydrate.shhome-manager/services/cliproxyapi/scripts/backup.shhome-manager/services/cliproxyapi/scripts/wrapper.shhome-manager/services/cliproxyapi/scripts/start.sh
**/*.{js,jsx,ts,tsx,json,jsonc,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use Biome for code formatting as configured in biome.json
Files:
home-manager/services/cliproxyapi/README.md
**/*.nix
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.nix: Use nixfmt for formatting all Nix files
Document complex configurations with comments in Nix files
**/*.nix: Use 2 spaces for indentation in Nix files
Keep line length under 100 characters in Nix files
Sort attribute sets alphabetically in Nix files
Use consistent spacing around operators in Nix files
Format lists and sets consistently in Nix filesUse treefmt.toml for formatting Nix files
**/*.nix: UsemkOptionfor configurable options in Nix modules
Implement proper typing for all options in Nix modules
Follow the Nix expression language style guide
Files:
home-manager/services/cliproxyapi/default.nix
**/default.nix
📄 CodeRabbit inference engine (CLAUDE.md)
Use
default.nixfiles for module exports
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/*/default.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
Service configurations should be located in
home-manager/services/<name>/with proper service definitions and correct dependency handling
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/**/default.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
Service configurations must include proper service definitions, handle dependencies correctly, and document service parameters
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
home-manager/**/*.nix: Use typed options whenever possible in Nix configurations
Document all configuration options in Nix modules and programs
Follow home-manager's module structure and keep configurations modular
Use proper indentation and formatting in Nix configuration files
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/nix.mdc)
Service configurations in
home-manager/services/should follow systemd service conventions, include proper service dependencies, and have clear documentation for service parameters
Files:
home-manager/services/cliproxyapi/default.nix
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Keep configurations modular across home-manager, hosts, and nix-darwin directories
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Follow home-manager's module structure and keep configurations modular
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Document all major changes in Nix configurations
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Applies to home-manager/services/**/*.nix : Service configurations in `home-manager/services/` should follow systemd service conventions, include proper service dependencies, and have clear documentation for service parameters
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/services/**/default.nix : Service configurations must include proper service definitions, handle dependencies correctly, and document service parameters
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/programs/**/*.nix : Follow program-specific best practices in program configuration files
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Applies to home-manager/programs/**/*.nix : Program configurations in `home-manager/programs/` should be organized by program name, include all necessary dependencies, use `home.packages` for package installations, and use `programs.<name>` when available in home-manager
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/programs/*/default.nix : Program configurations should be located in `home-manager/programs/<name>/` and use home-manager's built-in modules when available
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Test configurations before committing using `nix flake check` and `home-manager build --show-trace`
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Follow home-manager's module structure and keep configurations modular
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/services/*/default.nix : Service configurations should be located in `home-manager/services/<name>/` with proper service definitions and correct dependency handling
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Use proper indentation and formatting in Nix configuration files
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Use typed options whenever possible in Nix configurations
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Keep configurations modular across home-manager, hosts, and nix-darwin directories
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Document all configuration options in Nix modules and programs
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:23.224Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:34:23.224Z
Learning: Applies to nix-darwin/**/*.nix : Use Homebrew only for macOS-specific applications
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:32.423Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/formatting.mdc:0-0
Timestamp: 2025-11-25T09:34:32.423Z
Learning: Applies to **/*.{sh,bash} : Use consistent variable naming in shell scripts
Applied to files:
home-manager/services/cliproxyapi/scripts/wrapper.sh
🧬 Code graph analysis (4)
home-manager/services/cliproxyapi/scripts/hydrate.sh (3)
home-manager/services/cliproxyapi/scripts/backup.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/start.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
strip_quotes(11-11)
home-manager/services/cliproxyapi/scripts/backup.sh (3)
home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/start.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
strip_quotes(11-11)
home-manager/services/cliproxyapi/scripts/wrapper.sh (3)
home-manager/services/cliproxyapi/scripts/backup.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/start.sh (1)
strip_quotes(15-15)
home-manager/services/cliproxyapi/scripts/start.sh (3)
home-manager/services/cliproxyapi/scripts/backup.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
strip_quotes(15-15)home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
strip_quotes(11-11)
🪛 markdownlint-cli2 (0.18.1)
home-manager/services/cliproxyapi/README.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
108-108: Bare URL used
(MD034, no-bare-urls)
⏰ 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). (13)
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: lua-neovim-test
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: nix-darwin
- GitHub Check: nix-linux
- GitHub Check: nix-nixos
- GitHub Check: shell-test
- GitHub Check: shell-lint
🔇 Additional comments (14)
home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
1-9: LGTM: Clean environment loading.The strict shell mode and conditional environment file sourcing follow best practices.
home-manager/services/cliproxyapi/scripts/hydrate.sh (2)
1-13: LGTM: Proper setup and environment loading.Directory declarations and conditional environment file sourcing are correctly implemented.
43-45: LGTM: Defensive CCS synchronization.The conditional copy with error suppression gracefully handles the case where no JSON files exist yet.
home-manager/services/cliproxyapi/scripts/backup.sh (2)
25-28: LGTM: Prudent validation before backup.The check ensures the auth directory exists and contains files before attempting backup operations, preventing unnecessary S3 operations.
48-50: LGTM: Defensive CCS synchronization.Consistent with hydrate.sh, the conditional copy gracefully handles missing files.
home-manager/services/cliproxyapi/README.md (1)
1-115: LGTM: Comprehensive documentation of refactored architecture.The README clearly documents the new S3-backed auth management system, script purposes, data flows, and dependencies. The documentation aligns well with the code changes in this PR.
home-manager/services/cliproxyapi/scripts/start.sh (3)
4-19: LGTM: Streamlined environment loading.The simplified ENV_FILE path and consistent use of
strip_quotesfor environment variables align well with the refactored architecture.
22-37: LGTM: Config generation preserved.The template-based config generation with API key injection is correctly maintained.
41-54: LGTM: Docker execution maintained.The Linux Docker-based startup flow with appropriate volume mappings is correctly preserved.
home-manager/services/cliproxyapi/default.nix (5)
6-12: LGTM: Clean script path injection.The use of
pkgs.replaceVarsto inject AWS CLI paths into the hydrate and backup scripts follows Nix best practices for parameterizing shell scripts.
22-22: LGTM: Wrapper binary creation.The CLI wrapper is correctly defined using
writeShellScriptBinwith content from the wrapper script.
38-42: LGTM: Darwin service configuration with good PATH construction.The use of
lib.makeBinPathfor PATH construction is a best practice, and the inclusion of Homebrew paths is appropriate for Darwin. Note that theHOMEenvironment variable issue at line 40 is a derivative of thehomeDirdefinition issue at line 4.
70-84: LGTM: Well-structured systemd service.The Linux systemd service configuration follows proper conventions with appropriate service dependencies (
After/Wantsdocker.service), clean PATH construction usinglib.makeBinPath, and sensible restart policies. Note that line 79 depends ondockerStartScriptwhich has the hardcoded path issue flagged above.Based on learnings from
.cursor/rules/nix.mdcregarding systemd service conventions.
86-105: LGTM: Proper systemd path watcher and backup service.The systemd path unit correctly uses
%hfor home directory expansion (lines 89-91) and properly references the backup service. The backup service usesType=oneshotappropriately for a triggered task, with a minimal PATH that excludes unnecessary dependencies like Docker.Based on learnings from
.cursor/rules/nix.mdcregarding systemd service conventions.
| dockerStartScript = pkgs.writeShellScript "cliproxyapi-docker-start" '' | ||
| exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${startScript}" | ||
| ''; |
There was a problem hiding this comment.
Hardcoded /usr/bin/sg breaks on NixOS.
The hardcoded path /usr/bin/sg at line 19 will fail on NixOS systems where /usr/bin typically doesn't exist. This breaks the Linux systemd service configuration.
🔎 Proposed fix using Nix package
dockerStartScript = pkgs.writeShellScript "cliproxyapi-docker-start" ''
- exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${startScript}"
+ exec ${pkgs.shadow}/bin/sg docker -c "${pkgs.bash}/bin/bash ${startScript}"
'';📝 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.
| dockerStartScript = pkgs.writeShellScript "cliproxyapi-docker-start" '' | |
| exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${startScript}" | |
| ''; | |
| dockerStartScript = pkgs.writeShellScript "cliproxyapi-docker-start" '' | |
| exec ${pkgs.shadow}/bin/sg docker -c "${pkgs.bash}/bin/bash ${startScript}" | |
| ''; |
🤖 Prompt for AI Agents
In home-manager/services/cliproxyapi/default.nix around lines 18-20 the script
hardcodes /usr/bin/sg which breaks on NixOS; replace the literal path with the
sg from the Nix package set (e.g. use ${pkgs.util-linux}/bin/sg in the shell
script string) and ensure pkgs.util-linux is available in the scope or added to
the package inputs so the generated script points to a valid, pure Nix path
instead of /usr/bin/sg.
Simplify the cliproxyapi backup/sync architecture: - hydrate.sh: pull from S3 to local (runs at activation) - backup.sh: push from local to S3 (triggered by WatchPaths) - start.sh: just load .env and start the binary - wrapper.sh: load .env and exec binary for CLI usage Remove complex fallback chains (CCS, dotfiles backup) in favor of S3 as the single source of truth. Depends on: router-for-me/CLIProxyAPI#859 Co-authored-by: Amp <amp@ampcode.com> Amp-Thread-ID: https://ampcode.com/threads/T-019b79f0-bfa1-7107-bacc-4f98ddc99f99
cd07bf9 to
1bc308c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
spec/coverage_spec.sh (1)
36-42: Update test cases to reference new scripts instead of removed ones.Lines 36-42 still reference
backup-and-recover.shandbackup-auth.shwhich have been replaced by the new scripts. These test cases should be updated to referencebackup.sh,hydrate.sh, andwrapper.shto match the updatedcovered_scriptslist at lines 91-94.🔎 Proposed fix
-It 'has spec file for home-manager/services/cliproxyapi/scripts/backup-and-recover.sh' -The path "spec/cliproxyapi_backup_spec.sh" should be exist -End - -It 'has spec file for home-manager/services/cliproxyapi/scripts/backup-auth.sh' -The path "spec/cliproxyapi_backup_spec.sh" should be exist -End +It 'has spec file for home-manager/services/cliproxyapi/scripts/backup.sh' +The path "spec/cliproxyapi_backup_spec.sh" should be exist +End + +It 'has spec file for home-manager/services/cliproxyapi/scripts/hydrate.sh' +The path "spec/cliproxyapi_backup_spec.sh" should be exist +End + +It 'has spec file for home-manager/services/cliproxyapi/scripts/wrapper.sh' +The path "spec/cliproxyapi_backup_spec.sh" should be exist +End
♻️ Duplicate comments (9)
home-manager/services/cliproxyapi/scripts/start.sh (1)
67-68: Simplified error message removes helpful installation guidance.The error message was simplified from providing platform-specific installation instructions to just "cliproxyapi not found". Consider restoring the helpful guidance for users.
home-manager/services/cliproxyapi/scripts/backup.sh (2)
37-52: Sequential sync failure handling already flagged.The
set -euo pipefailcombined with&& echopattern means if the first sync fails, the script exits before attempting the second sync. This was previously flagged by Copilot with a suggested fix using explicit error handling.
43-43: Hardcoded bucket name inconsistent with parameterized approach.The bucket name
cliproxyapiis hardcoded here, butstart.shandwrapper.shuseOBJECTSTORE_BUCKETenvironment variable. This was previously flagged.Also applies to: 51-51
home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
32-46: First sync failure still exits before second sync is attempted.Despite being marked as addressed in a previous commit, the script still uses
&& echowithset -euo pipefail, meaning the first sync failure will exit before attempting thebackup/auths/fallback. If redundancy is intended, consider allowing the first sync to fail gracefully:🔎 Proposed fix for graceful fallback
-AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ - AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ - @aws@ s3 sync \ - --endpoint-url="$ENDPOINT" \ - --no-progress \ - "s3://cliproxyapi/auths/" \ - "$AUTH_DIR/" && echo "✅ Hydrated from S3 auths/" >&2 - -AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ - AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ - @aws@ s3 sync \ - --endpoint-url="$ENDPOINT" \ - --no-progress \ - "s3://cliproxyapi/backup/auths/" \ - "$AUTH_DIR/" && echo "✅ Hydrated from S3 backup/auths/" >&2 +if AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ + AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ + @aws@ s3 sync \ + --endpoint-url="$ENDPOINT" \ + --no-progress \ + "s3://cliproxyapi/auths/" \ + "$AUTH_DIR/"; then + echo "✅ Hydrated from S3 auths/" >&2 +else + echo "⚠️ Failed to hydrate from S3 auths/, trying backup..." >&2 +fi + +AWS_ACCESS_KEY_ID="$ACCESS_KEY" \ + AWS_SECRET_ACCESS_KEY="$SECRET_KEY" \ + @aws@ s3 sync \ + --endpoint-url="$ENDPOINT" \ + --no-progress \ + "s3://cliproxyapi/backup/auths/" \ + "$AUTH_DIR/" && echo "✅ Hydrated from S3 backup/auths/" >&2home-manager/services/cliproxyapi/default.nix (5)
77-80: Race condition risk: WatchPaths can trigger data loss until upstream fix deploys.WatchPaths monitors directories that the upstream service may wipe on startup (per PR description). Without the fix from router-for-me/CLIProxyAPI#859, file deletions will sync to S3, potentially losing auth data.
Verify the upstream fix status before deploying this configuration:
Has pull request #859 in router-for-me/CLIProxyAPI been merged and deployed?
1-4: Missingconfigargument prevents usingconfig.home.homeDirectory.The module function signature on line 1 only includes
pkgs, but line 4's use ofbuiltins.getEnv "HOME"should be replaced withconfig.home.homeDirectory(as flagged in previous comments). To enable this fix, the function signature must be updated to{ config, pkgs, ... }:.🔎 Complete fix including function signature
-{ pkgs, ... }: +{ config, pkgs, ... }: let inherit (pkgs) lib; - homeDir = builtins.getEnv "HOME"; + homeDir = config.home.homeDirectory;
18-20: Hardcoded/usr/bin/sgbreaks on NixOS.This path won't exist on NixOS systems. Use
${pkgs.shadow}/bin/sginstead (shadow package provides thesgcommand for group execution).🔎 Proposed fix
dockerStartScript = pkgs.writeShellScript "cliproxyapi-docker-start" '' - exec /usr/bin/sg docker -c "${pkgs.bash}/bin/bash ${startScript}" + exec ${pkgs.shadow}/bin/sg docker -c "${pkgs.bash}/bin/bash ${startScript}" '';
22-22: Hardcoded Homebrew path in wrapper breaks Intel Mac compatibility.The wrapper.sh hardcodes
/opt/homebrew/bin/cliproxyapi(Apple Silicon path). Usepkgs.replaceVarsto substitute the binary path dynamically, or add Intel Mac fallback (/usr/local/bin).🔎 Recommended fix using pkgs.replaceVars
Update wrapper.sh to use a placeholder like
@cliproxyapi@, then:- cliWrapper = pkgs.writeShellScriptBin "cliproxyapi" (builtins.readFile ./scripts/wrapper.sh); + cliWrapper = pkgs.writeShellScriptBin "cliproxyapi" (pkgs.replaceVars ./scripts/wrapper.sh { + cliproxyapi = "/opt/homebrew/bin/cliproxyapi"; # or detect at build time + });Alternatively, add runtime detection in wrapper.sh similar to start.sh's fallback logic.
25-30: Suppressed hydration failures could cause auth issues.Line 28's
|| truesilently ignores all hydration failures (network issues, credentials, S3 unavailability). The service will start with potentially stale or missing auth files.Consider logging failures prominently or adding a notification mechanism:
🔎 Example with logging
home.activation = lib.optionalAttrs (lib ? hm && lib.hm ? dag) { hydrateCliproxyAuths = lib.hm.dag.entryAfter [ "writeBoundary" ] '' - ${pkgs.bash}/bin/bash ${hydrateScript} || true + if ! ${pkgs.bash}/bin/bash ${hydrateScript}; then + echo "WARNING: CLIProxyAPI hydration failed - auth files may be stale" >&2 + fi ''; };
🧹 Nitpick comments (3)
home-manager/services/cliproxyapi/scripts/start.sh (1)
15-20: Consider extractingstrip_quotesto a shared library.The
strip_quotesfunction is duplicated identically across four scripts:backup.sh,hydrate.sh,start.sh, andwrapper.sh. While duplication is acceptable for standalone scripts, extracting this to a shared helper (e.g., sourced from a common file) would reduce maintenance burden.home-manager/services/cliproxyapi/README.md (1)
129-131: Use markdown link syntax for the URL.Per markdownlint (MD034), bare URLs should be wrapped in angle brackets or use proper markdown link syntax.
🔎 Proposed fix
-https://github.com/router-for-me/CLIProxyAPI/pull/859 +<https://github.com/router-for-me/CLIProxyAPI/pull/859>Or use a named link:
-https://github.com/router-for-me/CLIProxyAPI/pull/859 +[CLIProxyAPI#859](https://github.com/router-for-me/CLIProxyAPI/pull/859)home-manager/services/cliproxyapi/default.nix (1)
24-141: Consider adding documentation comments for service architecture.Per coding guidelines for Nix files, "Document complex configurations with comments." The multi-service architecture (hydrate → main service → backup watcher) and the S3-as-source-of-truth pattern would benefit from brief inline comments explaining the data flow, especially given the race condition dependency mentioned in the PR.
Based on coding guidelines requirement to document complex Nix configurations.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
home-manager/services/cliproxyapi/README.mdhome-manager/services/cliproxyapi/default.nixhome-manager/services/cliproxyapi/scripts/backup-and-recover.shhome-manager/services/cliproxyapi/scripts/backup-auth.shhome-manager/services/cliproxyapi/scripts/backup.shhome-manager/services/cliproxyapi/scripts/hydrate.shhome-manager/services/cliproxyapi/scripts/start.shhome-manager/services/cliproxyapi/scripts/wrapper.shspec/cliproxyapi_backup_spec.shspec/cliproxyapi_spec.shspec/coverage_spec.sh
💤 Files with no reviewable changes (2)
- home-manager/services/cliproxyapi/scripts/backup-and-recover.sh
- home-manager/services/cliproxyapi/scripts/backup-auth.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- home-manager/services/cliproxyapi/scripts/wrapper.sh
🧰 Additional context used
📓 Path-based instructions (8)
**/*.{sh,bash}
📄 CodeRabbit inference engine (CLAUDE.md)
Use shfmt with 2-space indentation for shell scripts
**/*.{sh,bash}: Use 2 spaces for indentation in shell scripts
Add proper shebang lines to shell scripts
Follow shellcheck recommendations in shell scripts
Document complex commands in shell scripts
Use consistent variable naming in shell scripts
Files:
spec/cliproxyapi_backup_spec.shspec/cliproxyapi_spec.shhome-manager/services/cliproxyapi/scripts/backup.shspec/coverage_spec.shhome-manager/services/cliproxyapi/scripts/start.shhome-manager/services/cliproxyapi/scripts/hydrate.sh
**/*.{js,jsx,ts,tsx,json,jsonc,md}
📄 CodeRabbit inference engine (.cursor/rules/general.mdc)
Use Biome for code formatting as configured in biome.json
Files:
home-manager/services/cliproxyapi/README.md
**/*.nix
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.nix: Use nixfmt for formatting all Nix files
Document complex configurations with comments in Nix files
**/*.nix: Use 2 spaces for indentation in Nix files
Keep line length under 100 characters in Nix files
Sort attribute sets alphabetically in Nix files
Use consistent spacing around operators in Nix files
Format lists and sets consistently in Nix filesUse treefmt.toml for formatting Nix files
**/*.nix: UsemkOptionfor configurable options in Nix modules
Implement proper typing for all options in Nix modules
Follow the Nix expression language style guide
Files:
home-manager/services/cliproxyapi/default.nix
**/default.nix
📄 CodeRabbit inference engine (CLAUDE.md)
Use
default.nixfiles for module exports
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/*/default.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
Service configurations should be located in
home-manager/services/<name>/with proper service definitions and correct dependency handling
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/**/default.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
Service configurations must include proper service definitions, handle dependencies correctly, and document service parameters
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/home-manager.mdc)
home-manager/**/*.nix: Use typed options whenever possible in Nix configurations
Document all configuration options in Nix modules and programs
Follow home-manager's module structure and keep configurations modular
Use proper indentation and formatting in Nix configuration files
Files:
home-manager/services/cliproxyapi/default.nix
home-manager/services/**/*.nix
📄 CodeRabbit inference engine (.cursor/rules/nix.mdc)
Service configurations in
home-manager/services/should follow systemd service conventions, include proper service dependencies, and have clear documentation for service parameters
Files:
home-manager/services/cliproxyapi/default.nix
🧠 Learnings (16)
📓 Common learnings
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Keep configurations modular across home-manager, hosts, and nix-darwin directories
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Follow home-manager's module structure and keep configurations modular
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Applies to home-manager/programs/**/*.nix : Program configurations in `home-manager/programs/` should be organized by program name, include all necessary dependencies, use `home.packages` for package installations, and use `programs.<name>` when available in home-manager
Applied to files:
spec/coverage_spec.shhome-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/programs/**/*.nix : Follow program-specific best practices in program configuration files
Applied to files:
spec/coverage_spec.shhome-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Test Nix and home-manager configurations locally before pushing using `make test`
Applied to files:
spec/coverage_spec.sh
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Document all major changes in Nix configurations
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Applies to home-manager/services/**/*.nix : Service configurations in `home-manager/services/` should follow systemd service conventions, include proper service dependencies, and have clear documentation for service parameters
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/programs/*/default.nix : Program configurations should be located in `home-manager/programs/<name>/` and use home-manager's built-in modules when available
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:35:01.066Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/nix.mdc:0-0
Timestamp: 2025-11-25T09:35:01.066Z
Learning: Test configurations before committing using `nix flake check` and `home-manager build --show-trace`
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Follow home-manager's module structure and keep configurations modular
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/services/*/default.nix : Service configurations should be located in `home-manager/services/<name>/` with proper service definitions and correct dependency handling
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Use proper indentation and formatting in Nix configuration files
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Use typed options whenever possible in Nix configurations
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:40.062Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/general.mdc:0-0
Timestamp: 2025-11-25T09:34:40.062Z
Learning: Keep configurations modular across home-manager, hosts, and nix-darwin directories
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/**/*.nix : Document all configuration options in Nix modules and programs
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:23.224Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: CLAUDE.md:0-0
Timestamp: 2025-11-25T09:34:23.224Z
Learning: Applies to nix-darwin/**/*.nix : Use Homebrew only for macOS-specific applications
Applied to files:
home-manager/services/cliproxyapi/default.nix
📚 Learning: 2025-11-25T09:34:55.014Z
Learnt from: CR
Repo: shunkakinoki/dotfiles PR: 0
File: .cursor/rules/home-manager.mdc:0-0
Timestamp: 2025-11-25T09:34:55.014Z
Learning: Applies to home-manager/services/**/default.nix : Service configurations must include proper service definitions, handle dependencies correctly, and document service parameters
Applied to files:
home-manager/services/cliproxyapi/default.nix
🧬 Code graph analysis (4)
spec/cliproxyapi_backup_spec.sh (1)
spec/support/custom_matcher.sh (1)
mock_bin_setup(5-25)
home-manager/services/cliproxyapi/scripts/backup.sh (3)
home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
strip_quotes(15-20)home-manager/services/cliproxyapi/scripts/start.sh (1)
strip_quotes(15-20)home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
strip_quotes(11-16)
home-manager/services/cliproxyapi/scripts/start.sh (3)
home-manager/services/cliproxyapi/scripts/backup.sh (1)
strip_quotes(15-20)home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
strip_quotes(15-20)home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
strip_quotes(11-16)
home-manager/services/cliproxyapi/scripts/hydrate.sh (3)
home-manager/services/cliproxyapi/scripts/backup.sh (1)
strip_quotes(15-20)home-manager/services/cliproxyapi/scripts/start.sh (1)
strip_quotes(15-20)home-manager/services/cliproxyapi/scripts/wrapper.sh (1)
strip_quotes(11-16)
🪛 markdownlint-cli2 (0.18.1)
home-manager/services/cliproxyapi/README.md
23-23: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
130-130: Bare URL used
(MD034, no-bare-urls)
⏰ 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). (13)
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: nix-linux
- GitHub Check: nix-darwin
- GitHub Check: nix-nixos
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim-test
- GitHub Check: lua-neovim
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: shell-lint
- GitHub Check: shell-test
🔇 Additional comments (15)
home-manager/services/cliproxyapi/scripts/start.sh (1)
1-69: LGTM!The streamlined environment loading,
strip_quoteshelper for credential normalization, and config generation logic are well-structured. The script correctly handles both Linux (Docker) and macOS (Homebrew binary) runtime paths.spec/coverage_spec.sh (1)
91-94: LGTM!The
covered_scriptslist correctly includes the newbackup.sh,hydrate.sh, andwrapper.shscripts.spec/cliproxyapi_spec.sh (2)
156-162: LGTM!The grep pattern correctly matches the updated conditional structure
if [ "$(uname)" = "Linux" ] && [ -n "${CLIPROXY_API_KEY:-}" ]in the script.
212-215: LGTM!The test correctly validates that the script exits with an error when the binary is not found.
home-manager/services/cliproxyapi/scripts/backup.sh (2)
53-55: LGTM!The CCS sync logic correctly creates the directory and uses
cp -ufor incremental updates with proper error suppression for the case when no JSON files exist.
25-33: LGTM!Good defensive checks: graceful exit when credentials are missing (line 25-28) and when auth directory is empty or non-existent (line 30-33). The
ls -Apattern correctly detects empty directories.home-manager/services/cliproxyapi/README.md (1)
1-137: LGTM!The README clearly documents the new S3-backed architecture, including the data flow diagram, script purposes, WatchPaths behavior on both macOS and Linux, and the dependency on the upstream race condition fix. Well-structured and informative.
home-manager/services/cliproxyapi/scripts/hydrate.sh (1)
48-50: LGTM!The CCS sync logic correctly creates the directory and uses
cp -ufor incremental updates with proper error suppression.spec/cliproxyapi_backup_spec.sh (3)
7-22: LGTM!Good approach: preprocessing scripts at describe-time to replace the
@aws@placeholder with the actualawscommand, enabling tests to run with the mock.
24-66: LGTM!The hydrate tests properly cover the key scenarios: successful S3 sync to both paths and graceful skip when credentials are missing.
68-118: LGTM!The backup tests cover essential scenarios: skipping on empty auth directory, pushing to S3 when files exist, and skipping when credentials are missing. Good test isolation with
mock_bin_setupandTEMP_HOME.home-manager/services/cliproxyapi/default.nix (4)
35-57: Darwin launchd service configuration follows best practices.The service configuration correctly uses
lib.makeBinPathfor PATH construction and appropriate daemon settings (KeepAlive, RunAtLoad). Only issue is thehomeDirdependency on line 43 (already flagged separately).
88-113: Linux systemd service follows proper conventions.The service configuration correctly declares docker dependencies, uses
lib.makeBinPathfor PATH, and sets appropriate restart policies. Only issue is thedockerStartScript/usr/bin/sg path (flagged separately at lines 18-20).
115-125: Systemd path unit correctly uses specifiers.The path unit properly uses
%hfor home directory (systemd specifier) and monitors the correct auth directories. Configuration follows systemd conventions.
127-140: Backup service configuration is correct.The oneshot service type is appropriate for path-triggered backups. PATH includes all necessary tools for the backup script.
1bc308c to
7852372
Compare
Summary
Simplify the cliproxyapi backup/sync architecture with S3 as the single source of truth.
Scripts
hydrate.shbackup.shstart.shwrapper.shData Flow
WatchPaths
~/.cli-proxy-api/objectstore/auths~/.ccs/cliproxy/authRemoved
Dependencies
The upstream PR fixes a race condition where the objectstore wipes local auth files on startup, triggering file watcher events that delete files from S3. Without this fix, auth files may be lost on service restart.