Claude/fix GitHub actions ax7ky - #1354
Conversation
* feat: add paperclip service - Systemd service on kyber: runs `paperclipai run` via bun, depends on docker-postgres - Config via builtins.toJSON: external postgres on kyber, embedded on macOS - Setup script creates paperclip database on docker-postgres - Makefile target: `make systemctl-paperclip` * fix: add shellcheck disable and shell test coverage for paperclip * fix: correct shellcheck disable directive syntax * refactor: use config.template.json pattern for paperclip * refactor: rename setup.sh to hydrate.sh for paperclip
* fix: remove EnvironmentFile from paperclip service * fix: use authenticated mode on kyber, add required meta/logging fields * fix: add allowedHostnames for paperclip.shunkakinoki.com * fix: format config template json
POSTGRES_HOST_AUTH_METHOD=trust removes password auth for all connections. The password kept going out of sync after crash recovery, causing paperclip and other services to fail with 'password authentication failed'. Safe since postgres is only reachable from the host.
…dd authenticated mode (#1347)
…es via DATABASE_URL (#1348)
* fix: run paperclip from cloned repo via pnpm dev:once The global bun install flattens pino@10 + pino-http@10.5 together, but pino-http needs pino@9. The repo lockfile resolves this correctly with nested dependencies. Running from the repo avoids the crash. * fix: use bun run server/src/index.ts instead of pnpm dev:once * fix: pin pino@9.14.0 override, run paperclipai from dotfiles node_modules The bun flat hoisting was resolving pino@10 which is incompatible with pino-http@10.5. Pinning pino to 9.14.0 via overrides matches the paperclip repo's lockfile resolution and fixes the crash.
* fix: use global ~/.bun/bin/paperclipai with pino override Propagate overrides from dotfiles package.json to ~/.bun/install/global/ so the global binary resolves pino@9.14.0 correctly. * test: add tests for npm-globals dependency overrides
- Fix non-portable \s regex to [[:space:]] in auto-switch.sh (shfmt compat) - Add jq dependency check alongside cswap - Use printf instead of echo for safer output - Fix claude-swap version from >=1.1.5 (non-existent) to >=0.7.1 - Add auto-switch.sh to Nix deployment config (default.nix) - Sort covered_scripts list alphabetically in coverage_spec.sh https://claude.ai/code/session_012GyQBesQGF1asTfKebWyLM
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 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 |
Mesa DescriptionTL;DRFixed GitHub Actions related to Claude. What changed?File summaries are undefined, so a detailed list of changes by file cannot be provided. 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 the Paperclip AI agent orchestration platform, including its configuration templates, hydration scripts, and a systemd user service. It also adds an auto-switching hook for Claude accounts to handle rate limits, implements dependency overrides for global Bun installations, and updates Nix configuration for substituters. Feedback highlights a security risk with unauthenticated PostgreSQL access and recommends using systemd's RuntimeDirectory for Paperclip's logging and temporary files to improve directory management and security.
| -p "${HOST_PORT}:${CONTAINER_PORT}" \ | ||
| -e POSTGRES_DB=trails_api \ | ||
| -e POSTGRES_PASSWORD=postgres \ | ||
| -e POSTGRES_HOST_AUTH_METHOD=trust \ |
There was a problem hiding this comment.
Enabling POSTGRES_HOST_AUTH_METHOD=trust is a security risk when the database port is exposed to the host (line 47). It allows any user on the system to connect to the database without a password, effectively ignoring the POSTGRES_PASSWORD requirement. It is safer to remove this line and rely on password authentication.
| lib.mkIf host.isKyber { | ||
| # Ensure Paperclip directories exist with correct permissions | ||
| home.activation.paperclipSetup = config.lib.dag.entryAfter [ "writeBoundary" ] '' | ||
| mkdir -p /tmp/paperclip |
There was a problem hiding this comment.
This manual directory creation in /tmp can be removed if you adopt the RuntimeDirectory approach suggested for the systemd service below. Using a fixed directory name in /tmp can lead to permission conflicts or collision issues on multi-user systems.
| Service = { | ||
| Type = "simple"; | ||
| ExecStart = "${homeDir}/.bun/bin/paperclipai run --no-repair"; | ||
| Restart = "always"; | ||
| RestartSec = "5s"; | ||
| Environment = [ | ||
| "HOME=${homeDir}" | ||
| "HOST=0.0.0.0" | ||
| "PAPERCLIP_DEPLOYMENT_MODE=authenticated" | ||
| "PAPERCLIP_ALLOWED_HOSTNAMES=paperclip.shunkakinoki.com,172.17.0.1" | ||
| "PATH=${homeDir}/.local/bin:${homeDir}/.bun/bin:${homeDir}/.nix-profile/bin:${homeDir}/.local/share/pnpm:${homeDir}/.local/share/fnm/current/bin:${homeDir}/.npm-global/bin:/usr/local/bin:/usr/bin:/bin" | ||
| ]; | ||
| EnvironmentFile = [ | ||
| "${homeDir}/dotfiles/.env" | ||
| "${homeDir}/.paperclip/instances/default/.env" | ||
| ]; | ||
| WorkingDirectory = "${homeDir}/.paperclip"; | ||
| StandardOutput = "append:/tmp/paperclip/paperclip.log"; | ||
| StandardError = "append:/tmp/paperclip/paperclip.log"; | ||
| }; |
There was a problem hiding this comment.
Instead of manually creating /tmp/paperclip and hardcoding log paths there, you can use systemd's RuntimeDirectory and the %t specifier (which resolves to $XDG_RUNTIME_DIR for user services). This is cleaner, avoids permission issues in shared /tmp environments, and ensures the directory is managed by systemd.
Service = {
Type = "simple";
ExecStart = "${homeDir}/.bun/bin/paperclipai run --no-repair";
Restart = "always";
RestartSec = "5s";
Environment = [
"HOME=${homeDir}"
"HOST=0.0.0.0"
"PAPERCLIP_DEPLOYMENT_MODE=authenticated"
"PAPERCLIP_ALLOWED_HOSTNAMES=paperclip.shunkakinoki.com,172.17.0.1"
"PATH=${homeDir}/.local/bin:${homeDir}/.bun/bin:${homeDir}/.nix-profile/bin:${homeDir}/.local/share/pnpm:${homeDir}/.local/share/fnm/current/bin:${homeDir}/.npm-global/bin:/usr/local/bin:/usr/bin:/bin"
];
EnvironmentFile = [
"${homeDir}/dotfiles/.env"
"${homeDir}/.paperclip/instances/default/.env"
];
WorkingDirectory = "${homeDir}/.paperclip";
RuntimeDirectory = "paperclip";
StandardOutput = "append:%t/paperclip/paperclip.log";
StandardError = "append:%t/paperclip/paperclip.log";
};
There was a problem hiding this comment.
Pull request overview
Adds Paperclip (paperclipai) provisioning/config + a Claude hook to auto-switch accounts on rate limits, along with dependency overrides and expanded shellspec coverage.
Changes:
- Introduce Paperclip configuration hydration + Home Manager systemd user service (kyber-only) and Makefile restart target.
- Add Claude
auto-switch.shhook + tests, and extend shellspec coverage tracking. - Add
paperclipaito global deps and pinpinoviaoverrides; adjust Nix substituter settings and Docker Postgres env.
Reviewed changes
Copilot reviewed 19 out of 20 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| spec/paperclip_hydrate_spec.sh | Adds shellspec assertions for the new Paperclip hydrate script. |
| spec/npm_globals_spec.sh | Adds coverage for new “overrides” handling in npm-globals installer. |
| spec/coverage_spec.sh | Registers new spec files + updates covered script list. |
| spec/auto_switch_spec.sh | Adds behavior tests for the new Claude auto-switch hook. |
| config/claude/hooks/auto-switch.sh | Updates hook to require jq, improve parsing/output, and switch on rate-limit signals. |
| config/claude/default.nix | Installs the new auto-switch hook into ~/.claude/hooks. |
| config/paperclip/hydrate.sh | New hydrate script that generates Paperclip config and optionally provisions DB on kyber. |
| config/paperclip/config.template.json | New Paperclip config template. |
| config/paperclip/default.nix | Home activation to run Paperclip hydration with substituted vars. |
| config/default.nix | Includes the new config/paperclip module. |
| home-manager/modules/paperclip/default.nix | Adds kyber-only Paperclip user service + setup activation. |
| home-manager/modules/default.nix | Enables the new Home Manager paperclip module. |
| home-manager/modules/npm-globals/install-npm-globals.sh | Applies package.json#overrides to Bun global install after installing globals. |
| package.json | Adds paperclipai + overrides pinning pino and updates trusted deps list. |
| bun.lock | Locks new dependency graph (paperclipai + overrides). |
| pyproject.toml | Adjusts claude-swap version constraint. |
| home-manager/nix/nix.nix | Switches to extra-substituters / extra-trusted-public-keys. |
| home-manager/services/docker-postgres/start-postgres.sh | Adds POSTGRES_HOST_AUTH_METHOD=trust env var. |
| Makefile | Adds systemctl-paperclip target and includes it in systemctl. |
| .env.example | Documents DATABASE_URL for Paperclip. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| -p "${HOST_PORT}:${CONTAINER_PORT}" \ | ||
| -e POSTGRES_DB=trails_api \ | ||
| -e POSTGRES_PASSWORD=postgres \ | ||
| -e POSTGRES_HOST_AUTH_METHOD=trust \ |
There was a problem hiding this comment.
Setting POSTGRES_HOST_AUTH_METHOD=trust disables password authentication and, combined with -p 5432:5432 (binds on all interfaces by default), exposes an unauthenticated Postgres instance to the network. Recommend removing POSTGRES_HOST_AUTH_METHOD=trust and relying on password auth, and/or binding the published port to localhost (e.g. 127.0.0.1:5432:5432) if this is only for local development.
| -p "${HOST_PORT}:${CONTAINER_PORT}" \ | |
| -e POSTGRES_DB=trails_api \ | |
| -e POSTGRES_PASSWORD=postgres \ | |
| -e POSTGRES_HOST_AUTH_METHOD=trust \ | |
| -p "127.0.0.1:${HOST_PORT}:${CONTAINER_PORT}" \ | |
| -e POSTGRES_DB=trails_api \ | |
| -e POSTGRES_PASSWORD=postgres \ |
|
|
||
| # Require at least 2 managed accounts | ||
| ACCOUNT_COUNT=$(cswap --list 2>/dev/null | grep -c '^\s*[0-9]' || echo 0) | ||
| ACCOUNT_COUNT=$(cswap --list 2>/dev/null | grep -c '^[[:space:]]*[0-9]' || printf '0') |
There was a problem hiding this comment.
grep -c prints 0 even when it exits with status 1 (no matches). With || printf '0', that can yield a value like 0\n0, which then breaks the numeric -lt test under set -e. Prefer ACCOUNT_COUNT=$(cswap --list 2>/dev/null | grep -c '^[[:space:]]*[0-9]' || true) and then default ACCOUNT_COUNT=${ACCOUNT_COUNT:-0} if needed.
| ACCOUNT_COUNT=$(cswap --list 2>/dev/null | grep -c '^[[:space:]]*[0-9]' || printf '0') | |
| ACCOUNT_COUNT=$(cswap --list 2>/dev/null | grep -c '^[[:space:]]*[0-9]' || true) | |
| ACCOUNT_COUNT=${ACCOUNT_COUNT:-0} |
| DB_CONNECTION="${DATABASE_URL:-@database_connection_string@}" | ||
|
|
||
| @sed@ \ | ||
| -e "s|__DATABASE_MODE__|@database_mode@|g" \ | ||
| -e "s|__DATABASE_CONNECTION_STRING__|${DB_CONNECTION}|g" \ | ||
| -e "s|__DEPLOYMENT_MODE__|@deployment_mode@|g" \ | ||
| -e "s|__HOST__|@host@|g" \ | ||
| -e "s|__ALLOWED_HOSTNAME__|@allowed_hostname@|g" \ | ||
| "$TEMPLATE" >"$CONFIG" |
There was a problem hiding this comment.
DB_CONNECTION is interpolated directly into a sed replacement. If the connection string contains &, |, or backslashes (or other sed-special chars), the generated JSON can be corrupted (e.g. & expands to the matched text). Consider escaping the replacement string before sed substitution, or generate config.json via jq (parse template and set fields) to avoid fragile string replacement.
| sed = "${pkgs.gnused}/bin/sed"; | ||
| database_mode = if host.isKyber then "postgres" else "embedded-postgres"; | ||
| database_connection_string = ""; | ||
| deployment_mode = if host.isKyber then "authenticated" else "local_trusted"; | ||
| host = if host.isKyber then "0.0.0.0" else "127.0.0.1"; | ||
| allowed_hostname = if host.isKyber then "paperclip.shunkakinoki.com" else ""; | ||
| is_kyber = if host.isKyber then "true" else "false"; |
There was a problem hiding this comment.
On kyber, database_mode is set to postgres but database_connection_string is substituted as an empty string. If DATABASE_URL is not present in ${HOME}/dotfiles/.env, this will hydrate an invalid config (and the DB provisioning block will be skipped). Either provide a sensible default connection string for kyber or fail fast when @is_kyber@ is true and DATABASE_URL is unset.
| ]; | ||
| EnvironmentFile = [ | ||
| "${homeDir}/dotfiles/.env" | ||
| "${homeDir}/.paperclip/instances/default/.env" |
There was a problem hiding this comment.
EnvironmentFile entries are required by systemd unless prefixed with -. ${homeDir}/.paperclip/instances/default/.env is not created by this module/config hydration, so the service can fail to start on first run. Consider prefixing it with - to make it optional, or creating an empty file during activation.
| "${homeDir}/.paperclip/instances/default/.env" | |
| "-${homeDir}/.paperclip/instances/default/.env" |
| # Ensure Paperclip directories exist with correct permissions | ||
| home.activation.paperclipSetup = config.lib.dag.entryAfter [ "writeBoundary" ] '' | ||
| mkdir -p /tmp/paperclip | ||
| mkdir -p ${homeDir}/.paperclip | ||
| chmod 700 ${homeDir}/.paperclip |
There was a problem hiding this comment.
Logs are written to /tmp/paperclip/paperclip.log, but /tmp/paperclip is created without restrictive permissions. That can allow other local users to read logs or mount symlink attacks on the log path. Consider using systemd RuntimeDirectory/StateDirectory or at least chmod 700 /tmp/paperclip and ensure the log file is created with mode 600 in a non-world-writable directory.
Summary by cubic
Adds a Paperclip service with Nix + systemd integration on kyber and a config hydrator that reads
DATABASE_URL. Also fixes CI by hardening the Claude auto-switch hook, adding tests, and applyingbunglobal overrides to avoidpinocrashes.New Features
paperclipai run) with logs and env setup.~/.paperclip/instances/default/config.jsonfrom a template and.env; creates the database if missing.make systemctl-papercliptarget to restart the service..env.examplenow includesDATABASE_URL.Bug Fixes
auto-switch.sh: addjqcheck, portable whitespace regex, useprintf; deployed via Nix and covered by tests.npm-globalsappliespackage.jsonoverridesto~/.bun/install/global, pinningpino@9.14.0to fixpino-http@10.5crashes; addedpaperclipaiand overrides inpackage.json.extra-substituters/extra-trusted-public-keysto silence warnings.POSTGRES_HOST_AUTH_METHOD=trust;claude-swapversion corrected to>=0.7.1.Written for commit 38b3b45. Summary will update on new commits.