keychainsy - #1280
Conversation
shunkakinoki
commented
Mar 26, 2026
shunkakinoki
commented
- feat: added cliproxyapi sync and bd dependency
- feat(cliproxyapi): added overrideable security path
- Wired keychain sync into cliproxyapi launchd and systemd services. - Added the @beads/bd package to the dependency manifest. Entire-Checkpoint: 1d8625226079
- Made the keychain lookup honor a SECURITY override and default to /usr/bin/security. - Added a dedicated shell spec for keychain sync behavior. - Extended coverage checks to require the new script and spec. Entire-Checkpoint: 5e4d60d743b6
|
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 ignored due to path filters (1)
📒 Files selected for processing (5)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughAdded platform-specific background services to automatically synchronize OAuth token credentials from system keychain and local auth files into a centralized auth directory. Includes a new keychain-sync script for macOS Keychain integration, Darwin launchd agents, Linux systemd timer and service units, comprehensive test coverage, and a new npm dependency. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 enhances 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. Footnotes
|
Mesa DescriptionTL;DRIntroduced 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 a new keychain-sync.sh script and integrates it into the cliproxyapi service for both macOS (via launchd) and Linux (via systemd). The script is designed to synchronize authentication tokens from Claude Code (macOS Keychain) and Codex CLI (~/.codex/auth.json) into a common authentication directory. Additionally, it sets up a periodic sync for S3 auth files on both operating systems and adds the @beads/bd dependency. Review feedback suggests improving error handling in the keychain-sync.sh script, making the email and keychain_account configurable, using a persistent logging location instead of /tmp/, and implementing atomic write operations for authentication tokens to prevent data corruption.
| # | ||
| # On first run, macOS will prompt to allow keychain access — click "Always Allow". | ||
| # shellcheck source=/dev/null | ||
| set -euo pipefail |
There was a problem hiding this comment.
The script uses set -euo pipefail. According to the general rules, "Do not use set -e in PAM execution scripts. These scripts must handle failures gracefully (e.g., during credential decryption or in retry loops) to avoid blocking the user's login." While this script isn't strictly a PAM script, its role in syncing authentication tokens suggests that failures should be handled gracefully rather than causing an immediate exit, which could impact user experience or automated processes. Consider replacing set -e with explicit error handling for critical commands.
References
- Do not use
set -ein PAM execution scripts. These scripts must handle failures gracefully (e.g., during credential decryption or in retry loops) to avoid blocking the user's login.
| ); | ||
|
|
||
| keychainSyncScript = pkgs.replaceVars ./scripts/keychain-sync.sh { | ||
| email = "shunkakinoki@gmail.com"; |
There was a problem hiding this comment.
The email for keychainSyncScript is hardcoded. It would be more flexible and maintainable to make this configurable, perhaps by deriving it from config.home.username or allowing it to be passed as an argument, especially if this configuration is intended for multiple users or environments.
|
|
||
| keychainSyncScript = pkgs.replaceVars ./scripts/keychain-sync.sh { | ||
| email = "shunkakinoki@gmail.com"; | ||
| keychain_account = "shunkakinoki"; |
There was a problem hiding this comment.
The keychain_account is hardcoded. Similar to the email, this should ideally be configurable to support different user accounts or to avoid exposing sensitive details directly in the configuration.
| StandardOutPath = "/tmp/cliproxyapi-keychain-sync.log"; | ||
| StandardErrorPath = "/tmp/cliproxyapi-keychain-sync.error.log"; |
There was a problem hiding this comment.
Logging to /tmp/ is generally not persistent across reboots and can be cleared by the system. For important service logs, consider using a more permanent location, such as a subdirectory within the user's home directory (e.g., ~/.local/state/cliproxyapi/logs) or a system-wide log directory if appropriate permissions are managed.
| StandardOutPath = "/tmp/cliproxyapi-sync.log"; | ||
| StandardErrorPath = "/tmp/cliproxyapi-sync.error.log"; |
There was a problem hiding this comment.
Similar to the cliproxyapi-keychain-sync agent, logging to /tmp/ for cliproxyapi-sync is not ideal for persistence. Please consider a more permanent logging location.
| existing_at=$($JQ -r '.access_token // empty' "$dest" 2>/dev/null) || true | ||
| fi | ||
|
|
||
| if [ "$access_token" != "$existing_at" ]; then |
There was a problem hiding this comment.
Writing directly to $dest using printf '%s' "$new_json" >"$dest" is not atomic. If the script is interrupted during the write operation, the destination file could be corrupted or left in an incomplete state. For sensitive data like authentication tokens, it's safer to write to a temporary file first and then atomically move it to the final destination.
| if [ "$access_token" != "$existing_at" ]; then | |
| printf '%s' "$new_json" >"${dest}.tmp" && mv "${dest}.tmp" "$dest" |
| existing_at=$($JQ -r '.access_token // empty' "$dest" 2>/dev/null) || true | ||
| fi | ||
|
|
||
| if [ "$access_token" != "$existing_at" ]; then |
There was a problem hiding this comment.
Similar to the Claude sync function, this write operation is not atomic. Consider using a temporary file and then atomically moving it to prevent data corruption in case of interruption.
| if [ "$access_token" != "$existing_at" ]; then | |
| printf '%s' "$new_json" >"${dest}.tmp" && mv "${dest}.tmp" "$dest" |
There was a problem hiding this comment.
Pull request overview
Adds a new cliproxyapi token sync path that pulls credentials from local stores (macOS Keychain for Claude Code; ~/.codex/auth.json for Codex) into the cliproxyapi auth cache, along with wiring for periodic sync and a new Node dependency.
Changes:
- Add
home-manager/services/cliproxyapi/scripts/keychain-sync.shto generate cliproxyapi auth JSON files from local credential sources. - Wire up periodic sync: a new Launchd agent on Darwin for keychain sync, plus a systemd timer/service on Linux for periodic S3 hydration.
- Add
@beads/bddependency (and lockfile updates) and add ShellSpec coverage for the new script.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/coverage_spec.sh | Adds coverage expectations for the new keychain-sync.sh script/spec. |
| spec/cliproxyapi_keychain_sync_spec.sh | New ShellSpec suite covering keychain and codex auth sync behaviors. |
| package.json | Adds @beads/bd dependency and marks it trusted. |
| bun.lock | Lockfile updates for @beads/bd. |
| home-manager/services/cliproxyapi/scripts/keychain-sync.sh | New token sync script producing cliproxyapi auth JSON files. |
| home-manager/services/cliproxyapi/default.nix | Adds Launchd agent for keychain sync (Darwin) and periodic hydrate timer/service (Linux). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| fi | ||
|
|
||
| if [ "$access_token" != "$existing_at" ]; then | ||
| printf '%s' "$new_json" >"$dest" |
There was a problem hiding this comment.
Same as above for the Codex auth file: writing secrets via direct redirection can create overly-permissive permissions and is not atomic, which can race with the backup watcher reading/syncing. Prefer restrictive permissions (0600) and an atomic write pattern (temp file + rename).
| printf '%s' "$new_json" >"$dest" | |
| umask 077 | |
| local tmp | |
| tmp="$(mktemp "${dest}.tmp.XXXXXX")" | |
| printf '%s' "$new_json" >"$tmp" | |
| chmod 600 "$tmp" 2>/dev/null || true | |
| mv "$tmp" "$dest" |
| rm -f "$TEMP_HOME/.mock-keychain-data" 2>/dev/null || true | ||
| rmdir "$TEMP_HOME/.cli-proxy-api/objectstore/auths" "$TEMP_HOME/.cli-proxy-api/objectstore" "$TEMP_HOME/.cli-proxy-api" "$TEMP_HOME/.codex" "$TEMP_HOME" 2>/dev/null || true |
There was a problem hiding this comment.
The cleanup() here uses rmdir on $TEMP_HOME, but the test creates auth JSON files under $TEMP_HOME/.cli-proxy-api/objectstore/auths, so the directories won’t be empty and the temp tree will be left behind. For consistency with other specs (e.g. cliproxyapi_backup_spec.sh), prefer rm -rf "$TEMP_HOME" (and any other temp artifacts) so tests don’t leak temp dirs/files.
| rm -f "$TEMP_HOME/.mock-keychain-data" 2>/dev/null || true | |
| rmdir "$TEMP_HOME/.cli-proxy-api/objectstore/auths" "$TEMP_HOME/.cli-proxy-api/objectstore" "$TEMP_HOME/.cli-proxy-api" "$TEMP_HOME/.codex" "$TEMP_HOME" 2>/dev/null || true | |
| rm -rf "$TEMP_HOME" 2>/dev/null || true |
|
|
||
| cleanup() { | ||
| rm -f "$TEMP_HOME/.codex/auth.json" 2>/dev/null || true | ||
| rmdir "$TEMP_HOME/.cli-proxy-api/objectstore/auths" "$TEMP_HOME/.cli-proxy-api/objectstore" "$TEMP_HOME/.cli-proxy-api" "$TEMP_HOME/.codex" "$TEMP_HOME" 2>/dev/null || true |
There was a problem hiding this comment.
Same temp cleanup issue in this cleanup(): since the script writes auth JSON files, rmdir won’t remove non-empty directories and $TEMP_HOME will remain on disk. Prefer removing the entire temp directory tree with rm -rf to avoid leaking files across test runs.
| rmdir "$TEMP_HOME/.cli-proxy-api/objectstore/auths" "$TEMP_HOME/.cli-proxy-api/objectstore" "$TEMP_HOME/.cli-proxy-api" "$TEMP_HOME/.codex" "$TEMP_HOME" 2>/dev/null || true | |
| rm -rf "$TEMP_HOME" 2>/dev/null || true |
|
|
||
| # Convert epoch ms to ISO 8601 | ||
| if [ -n "$expires_at" ] && [ "$expires_at" != "null" ]; then | ||
| expires_at=$(date -u -r "$((expires_at / 1000))" +%Y-%m-%dT%H:%M:%S+00:00 2>/dev/null) || expires_at="" |
There was a problem hiding this comment.
expiresAt conversion uses date -u -r <epoch> which is BSD-date syntax, but this agent’s PATH includes pkgs.coreutils (GNU date), where -r means “reference file” and the conversion will fail (leaving expired empty). Consider either using a GNU-compatible conversion (date -d "@...") or explicitly calling the system /bin/date (and avoiding GNU date shadowing) so the field is populated reliably.
| expires_at=$(date -u -r "$((expires_at / 1000))" +%Y-%m-%dT%H:%M:%S+00:00 2>/dev/null) || expires_at="" | |
| expires_at=$(/bin/date -u -r "$((expires_at / 1000))" +%Y-%m-%dT%H:%M:%S+00:00 2>/dev/null) || expires_at="" |
| local access_token refresh_token account_id | ||
| access_token=$($JQ -r '.tokens.access_token // .OPENAI_API_KEY // empty' "$auth_file" 2>/dev/null) || true | ||
| refresh_token=$($JQ -r '.tokens.refresh_token // empty' "$auth_file" 2>/dev/null) || true | ||
| account_id=$($JQ -r '.account_id // empty' "$auth_file" 2>/dev/null) || true |
There was a problem hiding this comment.
Codex account_id is extracted from .account_id, but the Codex auth format used elsewhere (and in this repo’s spec fixtures) nests it under .tokens.account_id. This will always write an empty account_id in the generated cliproxyapi auth JSON. Update the jq path to read the correct field (optionally supporting both layouts for compatibility).
| account_id=$($JQ -r '.account_id // empty' "$auth_file" 2>/dev/null) || true | |
| account_id=$($JQ -r '.tokens.account_id // .account_id // empty' "$auth_file" 2>/dev/null) || true |
| fi | ||
|
|
||
| if [ "$access_token" != "$existing_at" ]; then | ||
| printf '%s' "$new_json" >"$dest" |
There was a problem hiding this comment.
Auth JSON files containing access tokens are written with the process umask and via direct redirection. On many systems this can create world-readable files (e.g. mode 0644) and it’s also non-atomic (a watcher could read a partially-written file). Consider setting a restrictive umask / chmod to 0600 and writing via a temp file + atomic rename (mv) to avoid token exposure and partial reads.
| printf '%s' "$new_json" >"$dest" | |
| # Write via a temp file with restrictive permissions, then atomically rename | |
| local tmp_dest | |
| tmp_dest=$(mktemp "${dest}.tmp.XXXXXX") | |
| printf '%s' "$new_json" >"$tmp_dest" | |
| chmod 600 "$tmp_dest" | |
| mv -f "$tmp_dest" "$dest" |
![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 6 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/cliproxyapi_keychain_sync_spec.sh">
<violation number="1" location="spec/cliproxyapi_keychain_sync_spec.sh:39">
P2: Cleanup uses `rmdir` on directories that can contain generated auth files, so temp directories are not reliably removed.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| cleanup() { | ||
| rm -f "$TEMP_HOME/.mock-keychain-data" 2>/dev/null || true | ||
| rmdir "$TEMP_HOME/.cli-proxy-api/objectstore/auths" "$TEMP_HOME/.cli-proxy-api/objectstore" "$TEMP_HOME/.cli-proxy-api" "$TEMP_HOME/.codex" "$TEMP_HOME" 2>/dev/null || true |
There was a problem hiding this comment.
P2: Cleanup uses rmdir on directories that can contain generated auth files, so temp directories are not reliably removed.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/cliproxyapi_keychain_sync_spec.sh, line 39:
<comment>Cleanup uses `rmdir` on directories that can contain generated auth files, so temp directories are not reliably removed.</comment>
<file context>
@@ -0,0 +1,166 @@
+
+cleanup() {
+ rm -f "$TEMP_HOME/.mock-keychain-data" 2>/dev/null || true
+ rmdir "$TEMP_HOME/.cli-proxy-api/objectstore/auths" "$TEMP_HOME/.cli-proxy-api/objectstore" "$TEMP_HOME/.cli-proxy-api" "$TEMP_HOME/.codex" "$TEMP_HOME" 2>/dev/null || true
+}
+
</file context>
