cliproxyapi fix - #1080
Conversation
Entire-Checkpoint: 5581fcfce1fe
…nfig management Entire-Checkpoint: e251233dc7c5
Entire-Checkpoint: f655e56f1f53
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbitRelease Notes
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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, 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 refines the Highlights
Changelog
Activity
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;DRUnifies S3/objectstore handling for 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 introduces several enhancements for cliproxyapi. It refactors S3 operations into a common shell script, simplifying the hydrate, backup, start, and wrapper scripts and removing duplicated code. It also removes the redundant S3 backup path, streamlining the data flow. Additionally, it enhances the openclaw/hydrate.sh script to read the CLIPROXY_API_KEY from the cliproxyapi configuration file, with appropriate fallbacks. The changes are well-tested, with updates to existing specs and new tests for the added functionality. My review includes a couple of suggestions to improve code conciseness and error reporting in the shell scripts.
| cliproxy_api_key_from_env="${CLIPROXY_API_KEY:-}" | ||
| CLIPROXY_API_KEY="${OPENCLAW_CLIPROXY_API_KEY:-}" | ||
| if [ -z "$CLIPROXY_API_KEY" ]; then | ||
| CLIPROXY_API_KEY="$(read_cliproxy_api_key_from_config "$CLIPROXY_CONFIG")" | ||
| fi | ||
| if [ -z "$CLIPROXY_API_KEY" ]; then | ||
| CLIPROXY_API_KEY="$cliproxy_api_key_from_env" | ||
| fi | ||
| if [ -z "$CLIPROXY_API_KEY" ]; then | ||
| CLIPROXY_API_KEY="$(read_secret "${SECRETS_DIR}/cliproxy-key")" | ||
| fi |
There was a problem hiding this comment.
The chain of if statements to determine CLIPROXY_API_KEY is functionally correct, but a bit verbose. You can make this more concise by using && short-circuiting, which improves readability without changing the logic of conditional execution.
| cliproxy_api_key_from_env="${CLIPROXY_API_KEY:-}" | |
| CLIPROXY_API_KEY="${OPENCLAW_CLIPROXY_API_KEY:-}" | |
| if [ -z "$CLIPROXY_API_KEY" ]; then | |
| CLIPROXY_API_KEY="$(read_cliproxy_api_key_from_config "$CLIPROXY_CONFIG")" | |
| fi | |
| if [ -z "$CLIPROXY_API_KEY" ]; then | |
| CLIPROXY_API_KEY="$cliproxy_api_key_from_env" | |
| fi | |
| if [ -z "$CLIPROXY_API_KEY" ]; then | |
| CLIPROXY_API_KEY="$(read_secret "${SECRETS_DIR}/cliproxy-key")" | |
| fi | |
| cliproxy_api_key_from_env="${CLIPROXY_API_KEY:-}" | |
| CLIPROXY_API_KEY="${OPENCLAW_CLIPROXY_API_KEY:-}" | |
| [ -z "$CLIPROXY_API_KEY" ] && CLIPROXY_API_KEY="$(read_cliproxy_api_key_from_config "$CLIPROXY_CONFIG")" | |
| [ -z "$CLIPROXY_API_KEY" ] && CLIPROXY_API_KEY="$cliproxy_api_key_from_env" | |
| [ -z "$CLIPROXY_API_KEY" ] && CLIPROXY_API_KEY="$(read_secret "${SECRETS_DIR}/cliproxy-key")" |
| --endpoint-url="${OBJECTSTORE_ENDPOINT:?OBJECTSTORE_ENDPOINT is required}" \ | ||
| --no-progress \ | ||
| "$source_path" \ | ||
| "$destination_path" || true |
There was a problem hiding this comment.
The || true at the end of the aws s3 sync command will cause any sync failures to be silently ignored. While this prevents the script from exiting due to set -e, it also hides potentially important errors. It would be better to log a warning message to stderr if the sync fails. This provides visibility into failures without halting the script.
| "$destination_path" || true | |
| "$destination_path" || echo "⚠️ S3 sync failed. Source: '$source_path', Destination: '$destination_path'" >&2 |
Entire-Checkpoint: a682df10a476
There was a problem hiding this comment.
Pull request overview
This PR refactors the cliproxyapi service scripts to centralize S3/objectstore behavior into a shared common.sh, adjusts the docs/tests accordingly, and updates OpenClaw hydration to optionally resolve the Cliproxy API key from the cliproxyapi config.
Changes:
- Introduces
home-manager/services/cliproxyapi/scripts/common.shand updates cliproxyapi scripts to source it for env loading and auth-cache S3 sync. - Updates tests/coverage specs and cliproxyapi README to reflect the new S3/auth flow and bucket override behavior.
- Enhances
config/openclaw/hydrate.shto prefer the firstapi-keysentry from~/.cli-proxy-api/config.yaml(gateway mode), with new ShellSpec coverage.
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| spec/openclaw_hydrate_spec.sh | Adds assertions + an execution test for resolving Cliproxy API key from cliproxyapi config. |
| spec/coverage_spec.sh | Adds common.sh to the required coverage list. |
| spec/cliproxyapi_backup_spec.sh | Updates tests to preprocess/source common.sh and validates OBJECTSTORE_BUCKET usage. |
| home-manager/services/cliproxyapi/scripts/wrapper.sh | Sources common.sh and delegates auth sync to shared helpers. |
| home-manager/services/cliproxyapi/scripts/start.sh | Sources common.sh and delegates auth sync; still contains direct @aws@ syncs for config mirroring. |
| home-manager/services/cliproxyapi/scripts/hydrate.sh | Sources common.sh and uses shared hydrate helper. |
| home-manager/services/cliproxyapi/scripts/common.sh | New shared functions for env loading, credential checks, and S3 auth sync. |
| home-manager/services/cliproxyapi/scripts/backup.sh | Sources common.sh and uses shared backup helper. |
| home-manager/services/cliproxyapi/default.nix | Wires common.sh into other scripts via replaceVars. |
| home-manager/services/cliproxyapi/README.md | Updates documentation to remove the redundant backup/auths/ path. |
| config/pi/settings.json | Adds an additional package repository URL. |
| config/openclaw/hydrate.sh | Adds config-based Cliproxy API key resolution in gateway mode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| startScript = pkgs.replaceVars ./scripts/start.sh { | ||
| sed = "${pkgs.gnused}/bin/sed"; | ||
| aws = "${pkgs.awscli2}/bin/aws"; | ||
| common = commonScript; |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
config/openclaw/hydrate.sh (1)
34-49: The YAML parser makes assumptions about the config format.The awk-based parser assumes api-keys are formatted as
- "value"with double quotes. This works for the expected config format, but consider adding a fallback for unquoted values or values with single quotes if the config format may vary.💡 Optional: Handle unquoted and single-quoted values
read_cliproxy_api_key_from_config() { local config_file="$1" [ -f "$config_file" ] || return 0 awk ' /^api-keys:/ { in_api_keys = 1; next } in_api_keys && /^ - / { value = $0 - sub(/^ - "/, "", value) - sub(/"$/, "", value) + sub(/^ - ["'\'']?/, "", value) + sub(/["'\'']?$/, "", value) print value exit } in_api_keys && /^[^[:space:]]/ { exit } ' "$config_file" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/openclaw/hydrate.sh` around lines 34 - 49, The awk-based parser in read_cliproxy_api_key_from_config assumes api-keys are indented as ` - "value"` and only strips double quotes; update the function to accept unquoted values and single-quoted values by loosening the pattern and trimming optional leading `- ` and optional surrounding single or double quotes before printing; specifically change the awk block in read_cliproxy_api_key_from_config to capture the list item regardless of quoting (or fallback to a simple strip of leading ` - ` then remove leading/trailing quotes if present) so it returns values like value, 'value', and "value".home-manager/services/cliproxyapi/README.md (1)
38-40: Add a language specifier to the fenced code block.The code block is missing a language identifier. Since this is a simple text flow diagram, use
textorplaintextas the language specifier to satisfy markdown linting.📝 Proposed fix
-``` +```text S3 auths/ -> ~/.cli-proxy-api/objectstore/auths -> ~/.ccs/cliproxy/auth</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@home-manager/services/cliproxyapi/README.mdaround lines 38 - 40, The fenced
code block in README.md containing "S3 auths/ ->
~/.cli-proxy-api/objectstore/auths -> ~/.ccs/cliproxy/auth" lacks a language
specifier; update that triple-backtick block to include a language token such as
text or plaintext (e.g., ```text) so the markdown linter accepts it and the
snippet renders correctly.</details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@home-manager/services/cliproxyapi/scripts/common.sh:
- Around line 38-48: The cliproxy_s3_sync helper currently swallows errors by
appending "|| true"; remove that trailing "|| true" from the cliproxy_s3_sync
function so the s3 sync command returns its real exit code, allowing failures to
propagate; then update callers (e.g., start.sh and wrapper.sh) to rely on set
-euo pipefail or to explicitly check the return code of cliproxy_s3_sync and
implement retry/failure handling as needed.- Around line 12-17: The function cliproxy_strip_quotes currently only removes
double quotes; update it to handle both single and double-quoted .env values by
detecting and stripping matching leading and trailing quote characters (either '
or ") from the input value (cliproxy_strip_quotes) before returning; ensure you
only remove a pair of matching quotes (not internal or mismatched quotes) so
cliproxy_init_objectstore_env() receives the unquoted credential/endpoint
strings.In
@spec/openclaw_hydrate_spec.sh:
- Line 130: The test command currently chains the hydrate invocation and cat
with a semicolon so failures from the hydrate script are masked; update the
shell invocation in spec/openclaw_hydrate_spec.sh (the line running bash -c
'HOME="$TEMP_HOME" OPENCLAW_CONFIG_PATH="$TEMP_HOME/generated-openclaw.json"
bash "$PREPROCESSED_SCRIPT" >/dev/null 2>&1; cat
"$TEMP_HOME/generated-openclaw.json"') to use && between the hydrate invocation
and the cat so the spec fails if the hydrate step (the bash
"$PREPROCESSED_SCRIPT" call) exits non‑zero.
Nitpick comments:
In@config/openclaw/hydrate.sh:
- Around line 34-49: The awk-based parser in read_cliproxy_api_key_from_config
assumes api-keys are indented as- "value"and only strips double quotes;
update the function to accept unquoted values and single-quoted values by
loosening the pattern and trimming optional leading-and optional
surrounding single or double quotes before printing; specifically change the awk
block in read_cliproxy_api_key_from_config to capture the list item regardless
of quoting (or fallback to a simple strip of leading-then remove
leading/trailing quotes if present) so it returns values like value, 'value',
and "value".In
@home-manager/services/cliproxyapi/README.md:
- Around line 38-40: The fenced code block in README.md containing "S3 auths/ ->
~/.cli-proxy-api/objectstore/auths -> ~/.ccs/cliproxy/auth" lacks a language
specifier; update that triple-backtick block to include a language token such as
text or plaintext (e.g., ```text) so the markdown linter accepts it and the
snippet renders correctly.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: Organization UI **Review profile**: CHILL **Plan**: Pro **Run ID**: `6a429a28-2fc8-44ea-b0a9-1c887d58b5b9` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 7241abbce182197257c967709a3406e78bf6f57f and d9d6a96f0cdc4b6bae495ccfb28b3c18ab5affb4. </details> <details> <summary>📒 Files selected for processing (12)</summary> * `config/openclaw/hydrate.sh` * `config/pi/settings.json` * `home-manager/services/cliproxyapi/README.md` * `home-manager/services/cliproxyapi/default.nix` * `home-manager/services/cliproxyapi/scripts/backup.sh` * `home-manager/services/cliproxyapi/scripts/common.sh` * `home-manager/services/cliproxyapi/scripts/hydrate.sh` * `home-manager/services/cliproxyapi/scripts/start.sh` * `home-manager/services/cliproxyapi/scripts/wrapper.sh` * `spec/cliproxyapi_backup_spec.sh` * `spec/coverage_spec.sh` * `spec/openclaw_hydrate_spec.sh` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
| cliproxy_strip_quotes() { | ||
| local value="$1" | ||
| value="${value%\"}" | ||
| value="${value#\"}" | ||
| printf '%s' "$value" | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verifies current trimming behavior for double vs single quotes
v='"double-quoted"'; v="${v%\"}"; v="${v#\"}"; printf 'double => <%s>\n' "$v"
v="'single-quoted'"; v="${v%\"}"; v="${v#\"}"; printf 'single => <%s>\n' "$v"Repository: shunkakinoki/dotfiles
Length of output: 118
🏁 Script executed:
cat -n home-manager/services/cliproxyapi/scripts/common.sh | sed -n '1,30p'Repository: shunkakinoki/dotfiles
Length of output: 1155
🏁 Script executed:
rg "cliproxy_strip_quotes" -B 2 -A 10 home-manager/services/cliproxyapi/scripts/Repository: shunkakinoki/dotfiles
Length of output: 2155
🏁 Script executed:
# Check for .env parsing context and how the function is called
rg "\.env|strip_quotes" home-manager/services/cliproxyapi/ -B 2 -A 2Repository: shunkakinoki/dotfiles
Length of output: 2534
🏁 Script executed:
find home-manager/services/cliproxyapi -name "*.env*" -o -name "README*" | head -20Repository: shunkakinoki/dotfiles
Length of output: 108
🏁 Script executed:
rg "\.env|quote" home-manager/services/cliproxyapi/README.md -B 1 -A 3Repository: shunkakinoki/dotfiles
Length of output: 397
🏁 Script executed:
cat home-manager/services/cliproxyapi/README.md | grep -A 20 "Required in"Repository: shunkakinoki/dotfiles
Length of output: 469
cliproxy_strip_quotes must handle both single and double-quoted .env values.
Lines 12-17 only strip double quotes ("). If .env contains single-quoted values like KEY='value', the quotes persist in the variable and break credential/endpoint usage in cliproxy_init_objectstore_env().
🔧 Suggested fix
cliproxy_strip_quotes() {
local value="$1"
- value="${value%\"}"
- value="${value#\"}"
+ case "$value" in
+ \"*\")
+ value="${value#\"}"
+ value="${value%\"}"
+ ;;
+ \'*\')
+ value="${value#\'}"
+ value="${value%\'}"
+ ;;
+ esac
printf '%s' "$value"
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/services/cliproxyapi/scripts/common.sh` around lines 12 - 17,
The function cliproxy_strip_quotes currently only removes double quotes; update
it to handle both single and double-quoted .env values by detecting and
stripping matching leading and trailing quote characters (either ' or ") from
the input value (cliproxy_strip_quotes) before returning; ensure you only remove
a pair of matching quotes (not internal or mismatched quotes) so
cliproxy_init_objectstore_env() receives the unquoted credential/endpoint
strings.
| cliproxy_s3_sync() { | ||
| local source_path="$1" | ||
| local destination_path="$2" | ||
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY:?OBJECTSTORE_ACCESS_KEY is required}" \ | ||
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY:?OBJECTSTORE_SECRET_KEY is required}" \ | ||
| @aws@ s3 sync \ | ||
| --endpoint-url="${OBJECTSTORE_ENDPOINT:?OBJECTSTORE_ENDPOINT is required}" \ | ||
| --no-progress \ | ||
| "$source_path" \ | ||
| "$destination_path" || true | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect where helper return semantics affect flow
rg -nP '\bcliproxy_s3_sync\b|\bcliproxy_sync_auth_(from|to)_s3\b' \
home-manager/services/cliproxyapi/scripts/common.sh \
home-manager/services/cliproxyapi/scripts/start.sh \
home-manager/services/cliproxyapi/scripts/wrapper.shRepository: shunkakinoki/dotfiles
Length of output: 924
🏁 Script executed:
# Read the common.sh file to see the full context of the helper functions
head -70 home-manager/services/cliproxyapi/scripts/common.sh | tail -35Repository: shunkakinoki/dotfiles
Length of output: 1120
🏁 Script executed:
# Read start.sh to see how these functions are called
cat home-manager/services/cliproxyapi/scripts/start.shRepository: shunkakinoki/dotfiles
Length of output: 5973
🏁 Script executed:
# Read wrapper.sh to see how these functions are called
cat home-manager/services/cliproxyapi/scripts/wrapper.shRepository: shunkakinoki/dotfiles
Length of output: 689
Remove || true from the helper function to propagate S3 sync failures to callers.
The || true on line 47 masks sync failures silently, preventing callers from detecting auth/config drift. Even though start.sh and wrapper.sh use set -euo pipefail, this cannot catch failures when the helper always returns 0. Neither caller checks the return value anyway, so failures are completely undetected. Move error handling to the caller level where decisions about retry or failure modes belong.
🔧 Suggested change
cliproxy_s3_sync() {
local source_path="$1"
local destination_path="$2"
AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY:?OBJECTSTORE_ACCESS_KEY is required}" \
AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY:?OBJECTSTORE_SECRET_KEY is required}" \
`@aws`@ s3 sync \
--endpoint-url="${OBJECTSTORE_ENDPOINT:?OBJECTSTORE_ENDPOINT is required}" \
--no-progress \
"$source_path" \
- "$destination_path" || true
+ "$destination_path"
}📝 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.
| cliproxy_s3_sync() { | |
| local source_path="$1" | |
| local destination_path="$2" | |
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY:?OBJECTSTORE_ACCESS_KEY is required}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY:?OBJECTSTORE_SECRET_KEY is required}" \ | |
| @aws@ s3 sync \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT:?OBJECTSTORE_ENDPOINT is required}" \ | |
| --no-progress \ | |
| "$source_path" \ | |
| "$destination_path" || true | |
| } | |
| cliproxy_s3_sync() { | |
| local source_path="$1" | |
| local destination_path="$2" | |
| AWS_ACCESS_KEY_ID="${OBJECTSTORE_ACCESS_KEY:?OBJECTSTORE_ACCESS_KEY is required}" \ | |
| AWS_SECRET_ACCESS_KEY="${OBJECTSTORE_SECRET_KEY:?OBJECTSTORE_SECRET_KEY is required}" \ | |
| `@aws`@ s3 sync \ | |
| --endpoint-url="${OBJECTSTORE_ENDPOINT:?OBJECTSTORE_ENDPOINT is required}" \ | |
| --no-progress \ | |
| "$source_path" \ | |
| "$destination_path" | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@home-manager/services/cliproxyapi/scripts/common.sh` around lines 38 - 48,
The cliproxy_s3_sync helper currently swallows errors by appending "|| true";
remove that trailing "|| true" from the cliproxy_s3_sync function so the s3 sync
command returns its real exit code, allowing failures to propagate; then update
callers (e.g., start.sh and wrapper.sh) to rely on set -euo pipefail or to
explicitly check the return code of cliproxy_s3_sync and implement retry/failure
handling as needed.
| After 'cleanup_gateway' | ||
|
|
||
| It 'prefers the root api-keys entry from cliproxyapi config over the secret file' | ||
| When run bash -c 'HOME="'"$TEMP_HOME"'" OPENCLAW_CONFIG_PATH="'"$TEMP_HOME"'/generated-openclaw.json" bash "'"$PREPROCESSED_SCRIPT"'" >/dev/null 2>&1; cat "'"$TEMP_HOME"'/generated-openclaw.json"' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Demonstrates status masking with ';' vs correct propagation with '&&'
bash -c 'false; true' >/dev/null 2>&1; echo "semicolon_chain_exit=$?"
bash -c 'false && true' >/dev/null 2>&1; echo "and_chain_exit=$?"Repository: shunkakinoki/dotfiles
Length of output: 104
🏁 Script executed:
#!/bin/bash
# Check if the file exists and view context around line 130
if [ -f "spec/openclaw_hydrate_spec.sh" ]; then
echo "File found. Viewing lines 125-135:"
sed -n '125,135p' spec/openclaw_hydrate_spec.sh
else
echo "File not found at spec/openclaw_hydrate_spec.sh"
# Try to find it
find . -name "*openclaw_hydrate*" -type f 2>/dev/null
fiRepository: shunkakinoki/dotfiles
Length of output: 562
Use && instead of ; to ensure hydrate failures fail the spec.
At Line 130, the semicolon operator causes the exit status to be determined by cat regardless of whether the hydrate command failed. Use && to propagate failures from the bash script and properly fail the test when hydrate does not succeed.
Suggested change
-When run bash -c 'HOME="'"$TEMP_HOME"'" OPENCLAW_CONFIG_PATH="'"$TEMP_HOME"'/generated-openclaw.json" bash "'"$PREPROCESSED_SCRIPT"'" >/dev/null 2>&1; cat "'"$TEMP_HOME"'/generated-openclaw.json"'
+When run bash -c 'HOME="'"$TEMP_HOME"'" OPENCLAW_CONFIG_PATH="'"$TEMP_HOME"'/generated-openclaw.json" bash "'"$PREPROCESSED_SCRIPT"'" >/dev/null 2>&1 && cat "'"$TEMP_HOME"'/generated-openclaw.json"'📝 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.
| When run bash -c 'HOME="'"$TEMP_HOME"'" OPENCLAW_CONFIG_PATH="'"$TEMP_HOME"'/generated-openclaw.json" bash "'"$PREPROCESSED_SCRIPT"'" >/dev/null 2>&1; cat "'"$TEMP_HOME"'/generated-openclaw.json"' | |
| When run bash -c 'HOME="'"$TEMP_HOME"'" OPENCLAW_CONFIG_PATH="'"$TEMP_HOME"'/generated-openclaw.json" bash "'"$PREPROCESSED_SCRIPT"'" >/dev/null 2>&1 && cat "'"$TEMP_HOME"'/generated-openclaw.json"' |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@spec/openclaw_hydrate_spec.sh` at line 130, The test command currently chains
the hydrate invocation and cat with a semicolon so failures from the hydrate
script are masked; update the shell invocation in spec/openclaw_hydrate_spec.sh
(the line running bash -c 'HOME="$TEMP_HOME"
OPENCLAW_CONFIG_PATH="$TEMP_HOME/generated-openclaw.json" bash
"$PREPROCESSED_SCRIPT" >/dev/null 2>&1; cat
"$TEMP_HOME/generated-openclaw.json"') to use && between the hydrate invocation
and the cat so the spec fails if the hydrate step (the bash
"$PREPROCESSED_SCRIPT" call) exits non‑zero.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
9 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="spec/openclaw_hydrate_spec.sh">
<violation number="1" location="spec/openclaw_hydrate_spec.sh:130">
P2: Chain the hydrate step with `&&` so this spec actually fails when the gateway script exits non-zero.</violation>
</file>
<file name="home-manager/services/cliproxyapi/default.nix">
<violation number="1" location="home-manager/services/cliproxyapi/default.nix:17">
P1: `start.sh` still uses `@aws@`, but this replacement set no longer provides `aws`, so the generated startup script will break.</violation>
</file>
<file name="home-manager/services/cliproxyapi/scripts/common.sh">
<violation number="1" location="home-manager/services/cliproxyapi/scripts/common.sh:29">
P2: Include `OBJECTSTORE_BUCKET` in the credentials check; otherwise an empty bucket passes the guard and crashes later during sync.</violation>
<violation number="2" location="home-manager/services/cliproxyapi/scripts/common.sh:47">
P1: Don't swallow S3 sync failures here; the backup/hydrate scripts currently treat failed transfers as success.</violation>
</file>
<file name="spec/cliproxyapi_backup_spec.sh">
<violation number="1" location="spec/cliproxyapi_backup_spec.sh:60">
P2: Clear `OBJECTSTORE_BUCKET` in test setup. Otherwise these new default-bucket assertions become host-dependent when the environment already exports a bucket name.</violation>
</file>
<file name="config/openclaw/hydrate.sh">
<violation number="1" location="config/openclaw/hydrate.sh:40">
P2: This parser only handles `api-keys` entries written exactly as ` - "..."`, so other valid YAML styles can produce a broken `apiKey` value.</violation>
</file>
<file name="home-manager/services/cliproxyapi/scripts/wrapper.sh">
<violation number="1" location="home-manager/services/cliproxyapi/scripts/wrapper.sh:16">
P2: This refactor stops writing auth files to the `backup/auths/` S3 path, so the secondary auth backup is no longer maintained.</violation>
<violation number="2" location="home-manager/services/cliproxyapi/scripts/wrapper.sh:18">
P1: This restore path no longer falls back to `backup/auths/`, so an empty or damaged primary `auths/` prefix leaves the CLI cache empty even when the backup copy still exists.</violation>
</file>
<file name="home-manager/services/cliproxyapi/scripts/start.sh">
<violation number="1" location="home-manager/services/cliproxyapi/scripts/start.sh:13">
P2: Load the env-backed management URL after sourcing `.env`, otherwise `CLIPROXY_MANAGEMENT_URL` from `~/dotfiles/.env` is ignored.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| }; | ||
|
|
||
| hydrateScript = pkgs.replaceVars ./scripts/hydrate.sh { | ||
| common = commonScript; |
There was a problem hiding this comment.
P1: start.sh still uses @aws@, but this replacement set no longer provides aws, so the generated startup script will break.
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 17:
<comment>`start.sh` still uses `@aws@`, but this replacement set no longer provides `aws`, so the generated startup script will break.</comment>
<file context>
@@ -9,17 +9,21 @@ let
};
+ hydrateScript = pkgs.replaceVars ./scripts/hydrate.sh {
+ common = commonScript;
+ };
+
</file context>
| common = commonScript; | |
| aws = "${pkgs.awscli2}/bin/aws"; | |
| common = commonScript; |
| --endpoint-url="${OBJECTSTORE_ENDPOINT:?OBJECTSTORE_ENDPOINT is required}" \ | ||
| --no-progress \ | ||
| "$source_path" \ | ||
| "$destination_path" || true |
There was a problem hiding this comment.
P1: Don't swallow S3 sync failures here; the backup/hydrate scripts currently treat failed transfers as success.
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/common.sh, line 47:
<comment>Don't swallow S3 sync failures here; the backup/hydrate scripts currently treat failed transfers as success.</comment>
<file context>
@@ -0,0 +1,59 @@
+ --endpoint-url="${OBJECTSTORE_ENDPOINT:?OBJECTSTORE_ENDPOINT is required}" \
+ --no-progress \
+ "$source_path" \
+ "$destination_path" || true
+}
+
</file context>
| --no-progress \ | ||
| "s3://${OBJECTSTORE_BUCKET}/backup/auths/" \ | ||
| "$AUTH_DIR/" || true | ||
| cliproxy_sync_auth_from_s3 "$AUTH_DIR" |
There was a problem hiding this comment.
P1: This restore path no longer falls back to backup/auths/, so an empty or damaged primary auths/ prefix leaves the CLI cache empty even when the backup copy still exists.
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/wrapper.sh, line 18:
<comment>This restore path no longer falls back to `backup/auths/`, so an empty or damaged primary `auths/` prefix leaves the CLI cache empty even when the backup copy still exists.</comment>
<file context>
@@ -1,65 +1,21 @@
- --no-progress \
- "s3://${OBJECTSTORE_BUCKET}/backup/auths/" \
- "$AUTH_DIR/" || true
+ cliproxy_sync_auth_from_s3 "$AUTH_DIR"
fi
fi
</file context>
| After 'cleanup_gateway' | ||
|
|
||
| It 'prefers the root api-keys entry from cliproxyapi config over the secret file' | ||
| When run bash -c 'HOME="'"$TEMP_HOME"'" OPENCLAW_CONFIG_PATH="'"$TEMP_HOME"'/generated-openclaw.json" bash "'"$PREPROCESSED_SCRIPT"'" >/dev/null 2>&1; cat "'"$TEMP_HOME"'/generated-openclaw.json"' |
There was a problem hiding this comment.
P2: Chain the hydrate step with && so this spec actually fails when the gateway script exits non-zero.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/openclaw_hydrate_spec.sh, line 130:
<comment>Chain the hydrate step with `&&` so this spec actually fails when the gateway script exits non-zero.</comment>
<file context>
@@ -60,6 +70,70 @@ The output should include 'whatsapp-allow-from'
+After 'cleanup_gateway'
+
+It 'prefers the root api-keys entry from cliproxyapi config over the secret file'
+When run bash -c 'HOME="'"$TEMP_HOME"'" OPENCLAW_CONFIG_PATH="'"$TEMP_HOME"'/generated-openclaw.json" bash "'"$PREPROCESSED_SCRIPT"'" >/dev/null 2>&1; cat "'"$TEMP_HOME"'/generated-openclaw.json"'
+The status should be success
+The output should include 'from-cliproxy-config'
</file context>
| When run bash -c 'HOME="'"$TEMP_HOME"'" OPENCLAW_CONFIG_PATH="'"$TEMP_HOME"'/generated-openclaw.json" bash "'"$PREPROCESSED_SCRIPT"'" >/dev/null 2>&1; cat "'"$TEMP_HOME"'/generated-openclaw.json"' | |
| When run bash -c 'HOME="'"$TEMP_HOME"'" OPENCLAW_CONFIG_PATH="'"$TEMP_HOME"'/generated-openclaw.json" bash "'"$PREPROCESSED_SCRIPT"'" >/dev/null 2>&1 && cat "'"$TEMP_HOME"'/generated-openclaw.json"' |
| [ -n "${OBJECTSTORE_ENDPOINT:-}" ] && | ||
| [ -n "${OBJECTSTORE_ACCESS_KEY:-}" ] && | ||
| [ -n "${OBJECTSTORE_SECRET_KEY:-}" ] |
There was a problem hiding this comment.
P2: Include OBJECTSTORE_BUCKET in the credentials check; otherwise an empty bucket passes the guard and crashes later during sync.
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/common.sh, line 29:
<comment>Include `OBJECTSTORE_BUCKET` in the credentials check; otherwise an empty bucket passes the guard and crashes later during sync.</comment>
<file context>
@@ -0,0 +1,59 @@
+}
+
+cliproxy_has_objectstore_credentials() {
+ [ -n "${OBJECTSTORE_ENDPOINT:-}" ] &&
+ [ -n "${OBJECTSTORE_ACCESS_KEY:-}" ] &&
+ [ -n "${OBJECTSTORE_SECRET_KEY:-}" ]
</file context>
| [ -n "${OBJECTSTORE_ENDPOINT:-}" ] && | |
| [ -n "${OBJECTSTORE_ACCESS_KEY:-}" ] && | |
| [ -n "${OBJECTSTORE_SECRET_KEY:-}" ] | |
| [ -n "${OBJECTSTORE_ENDPOINT:-}" ] && | |
| [ -n "${OBJECTSTORE_BUCKET:-}" ] && | |
| [ -n "${OBJECTSTORE_ACCESS_KEY:-}" ] && | |
| [ -n "${OBJECTSTORE_SECRET_KEY:-}" ] |
| @@ -6,18 +6,25 @@ SCRIPTS_DIR="$PWD/home-manager/services/cliproxyapi/scripts" | |||
|
|
|||
There was a problem hiding this comment.
P2: Clear OBJECTSTORE_BUCKET in test setup. Otherwise these new default-bucket assertions become host-dependent when the environment already exports a bucket name.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/cliproxyapi_backup_spec.sh, line 60:
<comment>Clear `OBJECTSTORE_BUCKET` in test setup. Otherwise these new default-bucket assertions become host-dependent when the environment already exports a bucket name.</comment>
<file context>
@@ -50,11 +57,22 @@ cleanup() {
After 'cleanup'
-It 'pulls from S3 auths and backup/auths'
+It 'pulls from the configured S3 auth path'
When run bash -c 'HOME="'"$TEMP_HOME"'" bash "'"$__HYDRATE_SCRIPT"'" 2>&1; cat "$MOCK_LOG" 2>/dev/null || true'
The status should be success
</file context>
|
|
||
| awk ' | ||
| /^api-keys:/ { in_api_keys = 1; next } | ||
| in_api_keys && /^ - / { |
There was a problem hiding this comment.
P2: This parser only handles api-keys entries written exactly as - "...", so other valid YAML styles can produce a broken apiKey value.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/openclaw/hydrate.sh, line 40:
<comment>This parser only handles `api-keys` entries written exactly as ` - "..."`, so other valid YAML styles can produce a broken `apiKey` value.</comment>
<file context>
@@ -30,6 +31,23 @@ read_secret() {
+
+ awk '
+ /^api-keys:/ { in_api_keys = 1; next }
+ in_api_keys && /^ - / {
+ value = $0
+ sub(/^ - "/, "", value)
</file context>
| --no-progress \ | ||
| "$AUTH_DIR/" \ | ||
| "s3://${OBJECTSTORE_BUCKET}/backup/auths/" || true | ||
| cliproxy_sync_auth_to_s3 "$AUTH_DIR" |
There was a problem hiding this comment.
P2: This refactor stops writing auth files to the backup/auths/ S3 path, so the secondary auth backup is no longer maintained.
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/wrapper.sh, line 16:
<comment>This refactor stops writing auth files to the `backup/auths/` S3 path, so the secondary auth backup is no longer maintained.</comment>
<file context>
@@ -1,65 +1,21 @@
- --no-progress \
- "$AUTH_DIR/" \
- "s3://${OBJECTSTORE_BUCKET}/backup/auths/" || true
+ cliproxy_sync_auth_to_s3 "$AUTH_DIR"
else
- AWS_ACCESS_KEY_ID="$OBJECTSTORE_ACCESS_KEY" \
</file context>
| cliproxy_sync_auth_to_s3 "$AUTH_DIR" | |
| cliproxy_sync_auth_to_s3 "$AUTH_DIR" | |
| cliproxy_s3_sync "$AUTH_DIR/" "s3://${OBJECTSTORE_BUCKET}/backup/auths/" |
| OBJECTSTORE_BUCKET="$(strip_quotes "${OBJECTSTORE_BUCKET:-cliproxyapi}")" | ||
| OBJECTSTORE_ACCESS_KEY="$(strip_quotes "${OBJECTSTORE_ACCESS_KEY:-}")" | ||
| OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")" | ||
| cliproxy_init_objectstore_env |
There was a problem hiding this comment.
P2: Load the env-backed management URL after sourcing .env, otherwise CLIPROXY_MANAGEMENT_URL from ~/dotfiles/.env is ignored.
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/start.sh, line 13:
<comment>Load the env-backed management URL after sourcing `.env`, otherwise `CLIPROXY_MANAGEMENT_URL` from `~/dotfiles/.env` is ignored.</comment>
<file context>
@@ -1,74 +1,31 @@
-OBJECTSTORE_BUCKET="$(strip_quotes "${OBJECTSTORE_BUCKET:-cliproxyapi}")"
-OBJECTSTORE_ACCESS_KEY="$(strip_quotes "${OBJECTSTORE_ACCESS_KEY:-}")"
-OBJECTSTORE_SECRET_KEY="$(strip_quotes "${OBJECTSTORE_SECRET_KEY:-}")"
+cliproxy_init_objectstore_env
OBJECTSTORE_LOCAL_PATH="$CONFIG_DIR"
MANAGEMENT_PASSWORD="${CLIPROXY_MANAGEMENT_PASSWORD:-}"
</file context>
| cliproxy_init_objectstore_env | |
| cliproxy_init_objectstore_env | |
| MANAGEMENT_URL="${CLIPROXY_MANAGEMENT_URL:-http://127.0.0.1:8317/v0/management}" |
…cript Entire-Checkpoint: 184c969afd18
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="config/claude/settings.json">
<violation number="1" location="config/claude/settings.json:333">
P1: These deny rules are bypassable with Docker's long-form flags (`--all`/`--force`), so equivalent destructive prune commands remain allowed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| "Bash(docker system prune -a:*)", | ||
| "Bash(docker system prune -f:*)", |
There was a problem hiding this comment.
P1: These deny rules are bypassable with Docker's long-form flags (--all/--force), so equivalent destructive prune commands remain allowed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/claude/settings.json, line 333:
<comment>These deny rules are bypassable with Docker's long-form flags (`--all`/`--force`), so equivalent destructive prune commands remain allowed.</comment>
<file context>
@@ -330,6 +330,8 @@
"Bash(rm -rf /*:*)",
"Bash(rm -rf ~/*:*)",
"Bash(chmod -R 777:*)",
+ "Bash(docker system prune -a:*)",
+ "Bash(docker system prune -f:*)",
"Bash(mkfs:*)",
</file context>
| "Bash(docker system prune -a:*)", | |
| "Bash(docker system prune -f:*)", | |
| "Bash(docker system prune -a:*)", | |
| "Bash(docker system prune --all:*)", | |
| "Bash(docker system prune -f:*)", | |
| "Bash(docker system prune --force:*)", |
- Use docker stop before rm to fully free container name before reuse
- Guard usage_export trap against unbound variables with ${VAR:-}
- Remove redundant systemctl restart (home-manager handles it)
Summary by cubic
Unifies S3/objectstore handling for
cliproxyapiwith a sharedcommon.sh, standardizes on a singleauths/S3 path with a configurable bucket, and simplifies service scripts. Also improves OpenClaw hydration (uses~/.cli-proxy-api/config.yaml), addscagdotin/agentsto Pi, blocks unsafe Docker prune commands, and fixes container lifecycle to prevent 502s during switches.New Features
scripts/common.sh(env loading, creds checks, S3 sync); injected intohydrate.sh,backup.sh,start.sh, andwrapper.shvia Nix.s3://$OBJECTSTORE_BUCKET/auths/(defaultcliproxyapi); removedbackup/auths; README/specs updated; supports bucket overrides.~/.cli-proxy-api/config.yaml, then env, then secret file.https://github.com/cagdotin/agentsto Pipackages; expanded blocklist to includedocker system prune -a/-f.Bug Fixes
${VAR:-}).Written for commit 55d711f. Summary will update on new commits.