feat: enhance cliproxyapi with AMP API keys, GLM-4.7 support, and S3 config backup - #447
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. 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. 📝 WalkthroughSummary by CodeRabbitRelease Notes
✏️ Tip: You can customize this high-level summary in your review settings. WalkthroughReplaces sample API key lines with a single placeholder, adds AMP upstream-api-key and disables localhost-only management, adds GLM-4.7 model entries across providers, generates a build-time substituted startScript (PATH and aws/coreutils adjustments), extends env injection (AMP_UPSTREAM_API_KEY), and adds conditional S3 backup of generated config in the startup script. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Init as Service Manager
participant Script as startScript
participant FS as File System
participant ObjStore as ObjectStore (S3-compatible)
participant AMP as AMP Upstream
rect rgb(230,245,255)
Init->>Script: launch startScript (with substituted paths)
end
Script->>FS: read ENV_FILE (fallback-friendly path)
Script->>Script: inject env vars (incl. AMP_UPSTREAM_API_KEY) into templates
Script->>FS: write generated config.yaml
Note over Script,FS: local config created
alt OBJECTSTORE_* present
Script->>ObjStore: upload config to S3 endpoint (using OBJECTSTORE_ACCESS_KEY/SECRET)
alt upload success
ObjStore-->>Script: 200 OK
Script->>Init: log success and continue
else upload fail
ObjStore-->>Script: error
Script->>Init: log failure and continue
end
else no credentials
Script->>Init: log missing credentials and continue
end
Script->>AMP: (if configured) use upstream key for AMP interactions
Script->>Init: exit -> service proceeds to start with generated config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
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 significantly enhances the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
Mesa DescriptionTL;DREnhanced What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request enhances the cliproxyapi service by adding support for AMP API keys, the GLM-4.7 model, and S3-based configuration backup for improved reliability. The changes look good overall, introducing valuable features. I've identified a few areas for improvement:
- A potential security concern with allowing remote management access.
- A hardcoded user path in a shell script that affects portability.
- An opportunity to make the S3 backup process more robust by adding a timeout.
Please see my detailed comments below.
| restrict-management-to-localhost: true | ||
| # amp-upstream-api-key: "" # Optional - use AMP_API_KEY env var or ~/.local/share/amp/secrets.json | ||
| upstream-api-key: "__AMP_UPSTREAM_API_KEY__" | ||
| restrict-management-to-localhost: false |
There was a problem hiding this comment.
Changing restrict-management-to-localhost to false allows management access from any remote machine, which significantly increases the attack surface of the service. While access is still protected by a secret key, it's recommended to keep management endpoints restricted to localhost unless remote management is a strict requirement. If remote access is needed, consider firewalling the management port to a trusted set of IP addresses.
| CONFIG="$CONFIG_DIR/config.yaml" | ||
| ENV_FILE="$HOME/dotfiles/.env" | ||
| # Use explicit path since $HOME may not be set correctly in launchd context | ||
| ENV_FILE="${HOME:-/Users/shunkakinoki}/dotfiles/.env" |
There was a problem hiding this comment.
The hardcoded fallback path /Users/shunkakinoki makes the script non-portable and specific to a single user's machine setup. This harms reusability and maintainability. A better approach would be to pass the home directory path from the Nix configuration (e.g., using config.home.homeDirectory in default.nix) and substitute it into this script, avoiding any hardcoded user-specific paths.
| # This prevents corrupted configs from persisting across restarts | ||
| if [ -n "${OBJECTSTORE_ENDPOINT:-}" ] && [ -n "${OBJECTSTORE_ACCESS_KEY:-}" ]; then | ||
| echo "Uploading config to S3 backup..." >&2 | ||
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ |
There was a problem hiding this comment.
The aws s3 cp command could potentially hang, blocking the service from starting. To improve reliability, consider wrapping the command with timeout to prevent it from running indefinitely. The timeout utility is available since pkgs.coreutils is in the PATH.
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | |
| if timeout 30s AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 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="config/cliproxyapi/config.yaml">
<violation number="1" location="config/cliproxyapi/config.yaml:40">
P2: Changing `restrict-management-to-localhost` from `true` to `false` relaxes a security restriction, allowing remote access to AMP management endpoints. Ensure this is intentional and that proper authentication/authorization is in place to protect these endpoints from unauthorized remote access.</violation>
</file>
Reply to cubic to teach it or ask questions. Re-run a review with @cubic-dev-ai review this PR
| restrict-management-to-localhost: true | ||
| # amp-upstream-api-key: "" # Optional - use AMP_API_KEY env var or ~/.local/share/amp/secrets.json | ||
| upstream-api-key: "__AMP_UPSTREAM_API_KEY__" | ||
| restrict-management-to-localhost: false |
There was a problem hiding this comment.
P2: Changing restrict-management-to-localhost from true to false relaxes a security restriction, allowing remote access to AMP management endpoints. Ensure this is intentional and that proper authentication/authorization is in place to protect these endpoints from unauthorized remote access.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/cliproxyapi/config.yaml, line 40:
<comment>Changing `restrict-management-to-localhost` from `true` to `false` relaxes a security restriction, allowing remote access to AMP management endpoints. Ensure this is intentional and that proper authentication/authorization is in place to protect these endpoints from unauthorized remote access.</comment>
<file context>
@@ -34,11 +33,11 @@ quota-exceeded:
- restrict-management-to-localhost: true
-# amp-upstream-api-key: "" # Optional - use AMP_API_KEY env var or ~/.local/share/amp/secrets.json
+ upstream-api-key: "__AMP_UPSTREAM_API_KEY__"
+ restrict-management-to-localhost: false
# Gemini API keys (preferred)
</file context>
There was a problem hiding this comment.
Pull request overview
This PR enhances the cliproxyapi service with improved reliability and expanded AI model support. It addresses configuration corruption issues across service restarts by implementing S3-based config backups, adds authentication support for AMP API clients, and enables the GLM-4.7 model for improved AI capabilities.
- Implements S3 config backup on service start to prevent configuration corruption
- Adds AMP_API_KEY and AMP_UPSTREAM_API_KEY authentication support
- Enables GLM-4.7 model support in both OpenRouter and Z-AI providers
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| home-manager/services/cliproxyapi/scripts/start.sh | Adds S3 config upload functionality, improves launchd path handling with HOME fallback, and injects new AMP API key placeholders |
| home-manager/services/cliproxyapi/default.nix | Implements path substitution for sed and aws commands using replaceVars, adds awscli2 to PATH for both launchd and systemd services |
| config/opencode/opencode.jsonc | Adds GLM-4.7 Coding Plan model configuration for Z-AI provider |
| config/cliproxyapi/config.yaml | Configures AMP API key authentication, adds GLM-4.7 model to both OpenRouter and Z-AI providers, and enables remote AMP management access |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| CONFIG="$CONFIG_DIR/config.yaml" | ||
| ENV_FILE="$HOME/dotfiles/.env" | ||
| # Use explicit path since $HOME may not be set correctly in launchd context | ||
| ENV_FILE="${HOME:-/Users/shunkakinoki}/dotfiles/.env" |
There was a problem hiding this comment.
The HOME variable fallback uses a hardcoded username. While this may work for the specific user environment, consider making this more maintainable by using a configurable value passed from the Nix configuration where HOME is already set in the launchd Environment (line 21 of default.nix). The script could validate that HOME is set before using it, and fail early if it's not rather than falling back to a hardcoded path.
|
|
||
| # Upload config to S3 to ensure backup is always correct | ||
| # This prevents corrupted configs from persisting across restarts | ||
| if [ -n "${OBJECTSTORE_ENDPOINT:-}" ] && [ -n "${OBJECTSTORE_ACCESS_KEY:-}" ]; then |
There was a problem hiding this comment.
The S3 backup check only validates OBJECTSTORE_ENDPOINT and OBJECTSTORE_ACCESS_KEY, but the AWS CLI also requires OBJECTSTORE_SECRET_KEY to function properly. If the secret key is missing, the aws s3 cp command will fail. Add a check for OBJECTSTORE_SECRET_KEY in the condition to prevent attempting uploads that are guaranteed to fail.
| if [ -n "${OBJECTSTORE_ENDPOINT:-}" ] && [ -n "${OBJECTSTORE_ACCESS_KEY:-}" ]; then | |
| if [ -n "${OBJECTSTORE_ENDPOINT:-}" ] && [ -n "${OBJECTSTORE_ACCESS_KEY:-}" ] && [ -n "${OBJECTSTORE_SECRET_KEY:-}" ]; then |
| @@ -197,6 +197,9 @@ | |||
| "models": { | |||
| "glm-4.6": { | |||
| "name": "GLM-4.6 (via Z-AI)", | |||
There was a problem hiding this comment.
There is a trailing comma after the "name" field which violates JSON syntax. While JSONC (JSON with Comments) may be more lenient in some parsers, trailing commas after the last property in an object are not universally supported and could cause parsing errors.
| restrict-management-to-localhost: true | ||
| # amp-upstream-api-key: "" # Optional - use AMP_API_KEY env var or ~/.local/share/amp/secrets.json | ||
| upstream-api-key: "__AMP_UPSTREAM_API_KEY__" | ||
| restrict-management-to-localhost: false |
There was a problem hiding this comment.
The restrict-management-to-localhost setting is being changed from true to false. This opens up management endpoints to non-localhost access which could be a security risk if the API is exposed to untrusted networks. Ensure this change is intentional and that the management secret key is strong enough to prevent unauthorized access from remote locations.
| restrict-management-to-localhost: false | |
| restrict-management-to-localhost: true |
| # api-keys: | ||
| # - "your-api-key-1" | ||
| # - "your-api-key-2" | ||
| # API keys for client authentication (e.g., Amp CLI) |
There was a problem hiding this comment.
The comment says "API keys for client authentication (e.g., Amp CLI)" but only a single placeholder is configured. If multiple API keys are expected for different clients, the comment should clarify that additional keys can be added. Alternatively, if only one key is intended, consider updating the comment to match this single-key pattern or add a comment explaining how to add more keys.
| # API keys for client authentication (e.g., Amp CLI) | |
| # API keys for client authentication (e.g., Amp CLI). Add one or more keys as list items below. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # API keys for client authentication (e.g., Amp CLI) | ||
| api-keys: | ||
| - "__AMP_API_KEY__" |
There was a problem hiding this comment.
The template now always emits api-keys with a placeholder, and the start script replaces missing AMP_API_KEY with an empty string. If AMP_API_KEY is unset (common when not using Amp), the generated config will contain a blank key (""), which flips auth behavior compared to the previous config (no api-keys). Depending on cliproxyapi’s handling, this either blocks all clients (no one can authenticate) or allows unauthenticated access via an empty key. Consider omitting api-keys entirely when the env var is missing or rendering an empty list instead.
Useful? React with 👍 / 👎.
![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
📜 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 (4)
config/cliproxyapi/config.yamlconfig/opencode/opencode.jsonchome-manager/services/cliproxyapi/default.nixhome-manager/services/cliproxyapi/scripts/start.sh
🧰 Additional context used
📓 Path-based instructions (10)
**/*.{json,yaml,yml,toml}
📄 CodeRabbit inference engine (.cursor/rules/formatting.mdc)
**/*.{json,yaml,yml,toml}: Use consistent indentation (2 spaces) in configuration files
Sort keys alphabetically when possible in configuration files
Use clear, descriptive names in configuration files
Files:
config/cliproxyapi/config.yaml
**/*.{yaml,yml,toml}
📄 CodeRabbit inference engine (.cursor/rules/formatting.mdc)
Add comments for complex configurations
Files:
config/cliproxyapi/config.yaml
**/*.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
**/*.{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:
config/opencode/opencode.jsonc
**/*.{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.sh
🧠 Learnings (4)
📓 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: Document all major changes in Nix configurations
📚 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: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: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
⏰ 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). (15)
- GitHub Check: Agent
- GitHub Check: cubic · AI code reviewer
- GitHub Check: lua-hammerspoon
- GitHub Check: lua-neovim
- GitHub Check: lua-neovim-test
- GitHub Check: nix-darwin
- GitHub Check: shellcheck
- GitHub Check: e2e-run (MacOS, macos-latest)
- GitHub Check: shellspec
- GitHub Check: e2e-run (NixOS, ubuntu-latest)
- GitHub Check: e2e-run (Ubuntu, ubuntu-latest)
- GitHub Check: nix-nixos
- GitHub Check: nix-linux
- GitHub Check: docker-build-push (linux/arm64, arm64, ubuntu-24.04-arm)
- GitHub Check: docker-build-push (linux/amd64, amd64, ubuntu-latest)
🔇 Additional comments (8)
config/opencode/opencode.jsonc (1)
200-203: LGTM!The new GLM-4.7 model entry is properly formatted and consistent with the existing model configuration structure.
home-manager/services/cliproxyapi/default.nix (3)
4-9: Good use of build-time variable substitution.The
replaceVarspattern ensures consistent binary paths and follows Nix best practices for script generation.
13-35: LGTM!Darwin launchd configuration correctly references the new startScript and includes awscli2 in the PATH for S3 backup operations.
37-59: LGTM!Linux systemd configuration mirrors the Darwin changes appropriately, with correct PATH management and startScript reference.
home-manager/services/cliproxyapi/scripts/start.sh (1)
30-36: LGTM!Variable substitution pattern correctly uses the
@sed@placeholder and properly adds AMP API key handling.config/cliproxyapi/config.yaml (3)
16-18: LGTM!API keys configuration properly uses template placeholder that matches the substitution in start.sh.
72-87: LGTM!GLM-4.7 model entries are properly configured for both openrouter and z-ai providers, consistent with the corresponding changes in opencode.jsonc.
36-40: Verify security implication of AMP integration management setting.The configuration shows
ampcode.restrict-management-to-localhost: falsein the AMP integration section. This is distinct from the main management API setting (remote-management.allow-remote: true), which is clearly documented with security notes.Before flagging this as a critical security issue, clarify:
- What management endpoints does the
ampcode.restrict-management-to-localhostsetting control?- Is this a secondary management API for the AMP integration specifically?
- What are the security implications of exposing AMP's management endpoints to remote access?
The setting name is self-documenting, but its security risk depends on what it actually controls in the application.
| # Use explicit path since $HOME may not be set correctly in launchd context | ||
| ENV_FILE="${HOME:-/Users/shunkakinoki}/dotfiles/.env" |
There was a problem hiding this comment.
Hardcoded user path breaks portability.
The fallback path /Users/shunkakinoki will fail for other users or on Linux systems. Consider using a more portable approach or failing explicitly when HOME is not set.
🔎 Suggested refactor for portable path handling
-# Use explicit path since $HOME may not be set correctly in launchd context
-ENV_FILE="${HOME:-/Users/shunkakinoki}/dotfiles/.env"
+# Use explicit path since $HOME may not be set correctly in launchd context
+# Fail explicitly if HOME is not set rather than using a hardcoded path
+if [ -z "${HOME:-}" ]; then
+ echo "ERROR: HOME environment variable not set" >&2
+ exit 1
+fi
+ENV_FILE="$HOME/dotfiles/.env"📝 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.
| # Use explicit path since $HOME may not be set correctly in launchd context | |
| ENV_FILE="${HOME:-/Users/shunkakinoki}/dotfiles/.env" | |
| # Use explicit path since $HOME may not be set correctly in launchd context | |
| # Fail explicitly if HOME is not set rather than using a hardcoded path | |
| if [ -z "${HOME:-}" ]; then | |
| echo "ERROR: HOME environment variable not set" >&2 | |
| exit 1 | |
| fi | |
| ENV_FILE="$HOME/dotfiles/.env" |
🤖 Prompt for AI Agents
In home-manager/services/cliproxyapi/scripts/start.sh around lines 8-9, remove
the hardcoded fallback /Users/shunkakinoki and instead require a valid HOME:
check if $HOME is set and non-empty and exit with a clear error if it isn't,
then construct ENV_FILE using "$HOME/dotfiles/.env" (no hardcoded user path) so
the script is portable across users and platforms.
| # Upload config to S3 to ensure backup is always correct | ||
| # This prevents corrupted configs from persisting across restarts | ||
| if [ -n "${OBJECTSTORE_ENDPOINT:-}" ] && [ -n "${OBJECTSTORE_ACCESS_KEY:-}" ]; then | ||
| echo "Uploading config to S3 backup..." >&2 | ||
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | ||
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | ||
| @aws@ s3 cp \ | ||
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | ||
| --no-progress \ | ||
| "$CONFIG" \ | ||
| "s3://cliproxyapi/config/config.yaml" 2>&1; then | ||
| echo "✅ Config backup uploaded" >&2 | ||
| else | ||
| echo "⚠️ Config backup failed (continuing anyway)" >&2 | ||
| fi | ||
| else | ||
| echo "⚠️ S3 config backup skipped: missing credentials" >&2 | ||
| fi |
There was a problem hiding this comment.
S3 backup logic has performance and configurability concerns.
Two issues to consider:
-
Synchronous backup delays startup: The S3 upload happens synchronously during service initialization, which can significantly delay startup if there are network issues.
-
Hardcoded bucket name: Line 51 uses a hardcoded bucket name
"cliproxyapi"instead of using the${OBJECTSTORE_BUCKET}variable that's already exported on line 24.
🔎 Suggested improvements
For the hardcoded bucket name, use the environment variable:
- "s3://cliproxyapi/config/config.yaml" 2>&1; then
+ "s3://${OBJECTSTORE_BUCKET}/config/config.yaml" 2>&1; thenFor the startup delay concern, consider:
- Making the S3 backup asynchronous (backgrounded)
- Or moving it to the separate backup service that already runs periodically
- Or adding a timeout to the AWS CLI command
📝 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.
| # Upload config to S3 to ensure backup is always correct | |
| # This prevents corrupted configs from persisting across restarts | |
| if [ -n "${OBJECTSTORE_ENDPOINT:-}" ] && [ -n "${OBJECTSTORE_ACCESS_KEY:-}" ]; then | |
| echo "Uploading config to S3 backup..." >&2 | |
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | |
| @aws@ s3 cp \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | |
| --no-progress \ | |
| "$CONFIG" \ | |
| "s3://cliproxyapi/config/config.yaml" 2>&1; then | |
| echo "✅ Config backup uploaded" >&2 | |
| else | |
| echo "⚠️ Config backup failed (continuing anyway)" >&2 | |
| fi | |
| else | |
| echo "⚠️ S3 config backup skipped: missing credentials" >&2 | |
| fi | |
| # Upload config to S3 to ensure backup is always correct | |
| # This prevents corrupted configs from persisting across restarts | |
| if [ -n "${OBJECTSTORE_ENDPOINT:-}" ] && [ -n "${OBJECTSTORE_ACCESS_KEY:-}" ]; then | |
| echo "Uploading config to S3 backup..." >&2 | |
| if AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY}" \ | |
| @aws@ s3 cp \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT}" \ | |
| --no-progress \ | |
| "$CONFIG" \ | |
| "s3://${OBJECTSTORE_BUCKET}/config/config.yaml" 2>&1; then | |
| echo "✅ Config backup uploaded" >&2 | |
| else | |
| echo "⚠️ Config backup failed (continuing anyway)" >&2 | |
| fi | |
| else | |
| echo "⚠️ S3 config backup skipped: missing credentials" >&2 | |
| fi |
Summary
Summary by cubic
Adds AMP API key auth, enables GLM-4.7 models, and hardens cliproxyapi with S3-backed config and launchd-safe startup. Improves reliability across restarts and expands model options.
New Features
Bug Fixes
Written for commit 6fc5a70. Summary will update automatically on new commits.