fix: use k8s postgres for paperclip, remove docker-postgres dep - #1347
Conversation
…dd authenticated mode
|
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
WalkthroughThe changes migrate Paperclip database configuration from a Docker-based approach to environment-variable-based configuration. The system now reads Changes
Sequence DiagramsequenceDiagram
participant Script as hydrate.sh Script
participant EnvFile as dotfiles/.env
participant Config as config.json
participant DB as psql / Postgres
Script->>EnvFile: Source environment file (if exists)
EnvFile-->>Script: DATABASE_URL
Script->>Script: Derive DB_CONNECTION from DATABASE_URL<br/>(or use Nix default)
Script->>Config: Replace __DATABASE_CONNECTION_STRING__<br/>with DB_CONNECTION
Script->>DB: Check if psql client available
Script->>DB: Derive base connection string<br/>(replace path/user with /postgres)
Script->>DB: Query pg_database for<br/>existing 'paperclip' database
DB-->>Script: Database exists? (yes/no)
alt Database does not exist
Script->>DB: Execute CREATE DATABASE paperclip
DB-->>Script: Success
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
Mesa DescriptionTL;DRRead What changed?
Description generated by Mesa. Update settings |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 issues found across 5 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/paperclip_hydrate_spec.sh">
<violation number="1" location="spec/paperclip_hydrate_spec.sh:60">
P2: This assertion doesn't actually test the "if missing" condition; it only checks that a CREATE DATABASE statement exists. The spec can pass even if database creation is unconditional.</violation>
</file>
<file name="config/paperclip/hydrate.sh">
<violation number="1" location="config/paperclip/hydrate.sh:24">
P2: Escape `DB_CONNECTION` before using it in the sed replacement; unescaped `&`/`|` can corrupt the rendered database URL in `config.json`.</violation>
<violation number="2" location="config/paperclip/hydrate.sh:36">
P1: Preserve URL query parameters when rewriting the DB name to `/postgres`; currently required options can be dropped from `DB_BASE`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| echo "Created paperclip database" >&2 | ||
| if [ "@is_kyber@" = "true" ] && [ -n "$DB_CONNECTION" ]; then | ||
| if command -v psql >/dev/null 2>&1; then | ||
| DB_BASE="${DB_CONNECTION%/*}/postgres" |
There was a problem hiding this comment.
P1: Preserve URL query parameters when rewriting the DB name to /postgres; currently required options can be dropped from DB_BASE.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/paperclip/hydrate.sh, line 36:
<comment>Preserve URL query parameters when rewriting the DB name to `/postgres`; currently required options can be dropped from `DB_BASE`.</comment>
<file context>
@@ -4,26 +4,38 @@ set -euo pipefail
- echo "Created paperclip database" >&2
+if [ "@is_kyber@" = "true" ] && [ -n "$DB_CONNECTION" ]; then
+ if command -v psql >/dev/null 2>&1; then
+ DB_BASE="${DB_CONNECTION%/*}/postgres"
+ if ! psql "$DB_BASE" -tAc "SELECT 1 FROM pg_database WHERE datname='paperclip'" 2>/dev/null | grep -q 1; then
+ psql "$DB_BASE" -c "CREATE DATABASE paperclip" 2>/dev/null && echo "Created paperclip database" >&2
</file context>
| DB_BASE="${DB_CONNECTION%/*}/postgres" | |
| DB_NO_QUERY="${DB_CONNECTION%%\?*}" | |
| DB_QUERY="${DB_CONNECTION#"$DB_NO_QUERY"}" | |
| DB_BASE="${DB_NO_QUERY%/*}/postgres${DB_QUERY}" |
| It 'checks if database already exists before creating' | ||
| When run bash -c "grep 'grep -qw paperclip' '$SCRIPT'" | ||
| It 'creates paperclip database if missing' | ||
| When run bash -c "grep 'CREATE DATABASE' '$SCRIPT'" |
There was a problem hiding this comment.
P2: This assertion doesn't actually test the "if missing" condition; it only checks that a CREATE DATABASE statement exists. The spec can pass even if database creation is unconditional.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At spec/paperclip_hydrate_spec.sh, line 60:
<comment>This assertion doesn't actually test the "if missing" condition; it only checks that a CREATE DATABASE statement exists. The spec can pass even if database creation is unconditional.</comment>
<file context>
@@ -44,13 +56,8 @@ End
-It 'checks if database already exists before creating'
-When run bash -c "grep 'grep -qw paperclip' '$SCRIPT'"
+It 'creates paperclip database if missing'
+When run bash -c "grep 'CREATE DATABASE' '$SCRIPT'"
The output should include 'paperclip'
End
</file context>
| @sed@ \ | ||
| -e "s|__DATABASE_MODE__|@database_mode@|g" \ | ||
| -e "s|__DATABASE_CONNECTION_STRING__|@database_connection_string@|g" \ | ||
| -e "s|__DATABASE_CONNECTION_STRING__|${DB_CONNECTION}|g" \ |
There was a problem hiding this comment.
P2: Escape DB_CONNECTION before using it in the sed replacement; unescaped &/| can corrupt the rendered database URL in config.json.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At config/paperclip/hydrate.sh, line 24:
<comment>Escape `DB_CONNECTION` before using it in the sed replacement; unescaped `&`/`|` can corrupt the rendered database URL in `config.json`.</comment>
<file context>
@@ -4,26 +4,38 @@ set -euo pipefail
@sed@ \
-e "s|__DATABASE_MODE__|@database_mode@|g" \
- -e "s|__DATABASE_CONNECTION_STRING__|@database_connection_string@|g" \
+ -e "s|__DATABASE_CONNECTION_STRING__|${DB_CONNECTION}|g" \
-e "s|__DEPLOYMENT_MODE__|@deployment_mode@|g" \
-e "s|__HOST__|@host@|g" \
</file context>
| -e "s|__DATABASE_CONNECTION_STRING__|${DB_CONNECTION}|g" \ | |
| -e "s|__DATABASE_CONNECTION_STRING__|$(printf '%s' "$DB_CONNECTION" | sed 's/[&|]/\\&/g')|g" \ |
![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 refactors the Paperclip database configuration to support external PostgreSQL instances via a DATABASE_URL environment variable, moving away from a hardcoded dependency on a local Docker-based PostgreSQL service. Key changes include updating the hydration script to source environment variables from a .env file, removing the docker-postgres.service dependency from the systemd unit, and updating the test suite to reflect the new database provisioning logic. A review comment pointed out that the manual construction of the base database URL is fragile and suggested using the psql -d flag to more robustly override the database name while preserving connection parameters.
| DB_BASE="${DB_CONNECTION%/*}/postgres" | ||
| if ! psql "$DB_BASE" -tAc "SELECT 1 FROM pg_database WHERE datname='paperclip'" 2>/dev/null | grep -q 1; then | ||
| psql "$DB_BASE" -c "CREATE DATABASE paperclip" 2>/dev/null && echo "Created paperclip database" >&2 |
There was a problem hiding this comment.
The logic for constructing DB_BASE by stripping the last component of the URL is fragile. If the DATABASE_URL contains query parameters (e.g., ?sslmode=require), they will be lost when appending /postgres, which might cause connection failures if the server requires those parameters to connect.
A more robust approach is to use the full connection string and override the database name using the -d flag in psql. According to the psql documentation, the -d flag overrides the database name specified in a connection URI, preserving all other parameters like host, port, and options.
| DB_BASE="${DB_CONNECTION%/*}/postgres" | |
| if ! psql "$DB_BASE" -tAc "SELECT 1 FROM pg_database WHERE datname='paperclip'" 2>/dev/null | grep -q 1; then | |
| psql "$DB_BASE" -c "CREATE DATABASE paperclip" 2>/dev/null && echo "Created paperclip database" >&2 | |
| if ! psql "$DB_CONNECTION" -d postgres -tAc "SELECT 1 FROM pg_database WHERE datname='paperclip'" 2>/dev/null | grep -q 1; then | |
| psql "$DB_CONNECTION" -d postgres -c "CREATE DATABASE paperclip" 2>/dev/null && echo "Created paperclip database" >&2 | |
| fi |
There was a problem hiding this comment.
Pull request overview
Updates Paperclip’s Kyber deployment to use an external (k8s) Postgres connection configured via ~/dotfiles/.env, removing the previous dependency on a locally managed Docker Postgres service.
Changes:
- Load
DATABASE_URLfrom~/dotfiles/.envduring Paperclip config hydration and inject it into the generated config. - Remove
docker-postgres.serviceordering/requirements from the Paperclip systemd user service. - Provision the
paperclipdatabase viapsql(CREATE DATABASE) when missing, and documentDATABASE_URLin.env.example.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
config/paperclip/hydrate.sh |
Sources ~/dotfiles/.env, substitutes DB connection into config template, and attempts DB auto-provisioning via psql. |
config/paperclip/default.nix |
Removes the Kyber default DB connection string (now relies on DATABASE_URL). |
home-manager/modules/paperclip/default.nix |
Drops systemd dependency on docker-postgres.service. |
spec/paperclip_hydrate_spec.sh |
Updates ShellSpec checks to match the new env-loading and CREATE DATABASE provisioning behavior. |
.env.example |
Documents DATABASE_URL for Paperclip. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @sed@ \ | ||
| -e "s|__DATABASE_MODE__|@database_mode@|g" \ | ||
| -e "s|__DATABASE_CONNECTION_STRING__|@database_connection_string@|g" \ | ||
| -e "s|__DATABASE_CONNECTION_STRING__|${DB_CONNECTION}|g" \ | ||
| -e "s|__DEPLOYMENT_MODE__|@deployment_mode@|g" \ |
There was a problem hiding this comment.
DB_CONNECTION is interpolated directly into the sed replacement. If DATABASE_URL contains &, backslashes, or the delimiter, sed will treat those specially and can corrupt the generated JSON (e.g., query params like ...&sslmode=...). Escape the replacement value (or switch to a templating approach that safely JSON-encodes the value) before substituting into the template.
| echo "Created paperclip database" >&2 | ||
| if [ "@is_kyber@" = "true" ] && [ -n "$DB_CONNECTION" ]; then | ||
| if command -v psql >/dev/null 2>&1; then | ||
| DB_BASE="${DB_CONNECTION%/*}/postgres" |
There was a problem hiding this comment.
DB_BASE="${DB_CONNECTION%/*}/postgres" drops any query string/options from DATABASE_URL (e.g., ?sslmode=require), which can make the subsequent psql connection fail even though the main connection string works. Consider preserving connection options when deriving the base DB connection (or parse the URL more robustly).
| DB_BASE="${DB_CONNECTION%/*}/postgres" | |
| DB_QUERY="" | |
| DB_WITHOUT_QUERY="$DB_CONNECTION" | |
| if [ "${DB_CONNECTION#*\?}" != "$DB_CONNECTION" ]; then | |
| DB_QUERY="?${DB_CONNECTION#*\?}" | |
| DB_WITHOUT_QUERY="${DB_CONNECTION%%\?*}" | |
| fi | |
| DB_BASE="${DB_WITHOUT_QUERY%/*}/postgres${DB_QUERY}" |
|
|
||
| # Use DATABASE_URL from env if set, otherwise use the Nix default | ||
| DB_CONNECTION="${DATABASE_URL:-@database_connection_string@}" | ||
|
|
There was a problem hiding this comment.
On Kyber, database_mode is set to postgres but if DATABASE_URL is missing (and the Nix default is empty), the script will still render a config with an empty connectionString, likely causing Paperclip to fail later with a hard-to-debug error. Recommend explicitly checking for a non-empty DATABASE_URL on Kyber and exiting early with a clear warning (similar to other hydrate scripts) instead of producing an invalid config.
| # On Kyber, postgres mode requires a non-empty connection string. | |
| # Fail early with a clear error instead of rendering an invalid config. | |
| # shellcheck disable=SC2050 | |
| if [ "@is_kyber@" = "true" ] && [ -z "$DB_CONNECTION" ]; then | |
| echo "error: DATABASE_URL is required on Kyber when using postgres, but no non-empty value was provided." >&2 | |
| echo "Set DATABASE_URL in ${ENV_FILE} or provide a non-empty Nix database connection string." >&2 | |
| exit 1 | |
| fi |
| DB_BASE="${DB_CONNECTION%/*}/postgres" | ||
| if ! psql "$DB_BASE" -tAc "SELECT 1 FROM pg_database WHERE datname='paperclip'" 2>/dev/null | grep -q 1; then | ||
| psql "$DB_BASE" -c "CREATE DATABASE paperclip" 2>/dev/null && echo "Created paperclip database" >&2 | ||
| fi |
There was a problem hiding this comment.
If psql is not installed, database auto-provisioning is silently skipped. Since this PR relies on provisioning against the k8s Postgres, consider emitting a warning when @is_kyber@ is true but psql is missing (or ensure psql is available via the Nix environment) so failures are discoverable.
| fi | |
| fi | |
| else | |
| echo "Warning: psql is not installed; skipping automatic paperclip database provisioning for kyber deployment" >&2 |
| database_mode = if host.isKyber then "postgres" else "embedded-postgres"; | ||
| database_connection_string = | ||
| if host.isKyber then "postgres://postgres:postgres@localhost:5432/paperclip" else ""; | ||
| database_connection_string = ""; | ||
| deployment_mode = if host.isKyber then "authenticated" else "local_trusted"; |
There was a problem hiding this comment.
database_mode is postgres on Kyber but database_connection_string is now always an empty string. Unless DATABASE_URL is guaranteed to be present at hydrate time, this produces an invalid config. Either provide a sane Kyber default here or enforce/validate that DATABASE_URL is set (and fail/warn clearly) during hydration.
* feat: add paperclip service (#1342) * 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 * test: add auto-switch hook tests and update coverage spec * fix: remove EnvironmentFile from paperclip service (#1344) * fix: paperclip authenticated mode and required config fields (#1345) * 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 * fix: use trust auth for docker-postgres (#1346) 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. * fix: use k8s postgres via DATABASE_URL, remove docker-postgres dep, add authenticated mode (#1347) * fix: use bun runtime for paperclip (pino-http node crash), k8s postgres via DATABASE_URL (#1348) * fix: use nix-profile bun path for paperclip service (#1350) * fix: add caret prefix to paperclipai dependency (#1349) * fix: use extra-substituters to avoid untrusted user warnings (#1351) * fix: run paperclip from cloned repo via pnpm dev:once (#1352) * 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 paperclipai with pino override (#1353) * 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: resolve GitHub Actions failures and code review issues - 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 --------- Co-authored-by: Claude <noreply@anthropic.com>
Summary
DATABASE_URLfrom~/dotfiles/.envat hydrate time (like openclaw reads secrets)docker-postgres.servicedependency from systemd unitauthenticatedmode on kyber (required for0.0.0.0binding)allowedHostnamesforpaperclip.shunkakinoki.compsqlif missingRequired
Add to
~/dotfiles/.env:Summary by cubic
Switch Paperclip to k8s Postgres via
DATABASE_URLand remove thedocker-postgres.servicedependency. Hydration reads from~/dotfiles/.env, runs Kyber in authenticated mode, and auto-creates thepaperclipdatabase if missing.Refactors
DATABASE_URLfrom~/dotfiles/.envat hydrate time.docker-postgres.serviceand Docker-based DB provisioning.authenticatedmode on Kyber and bind to0.0.0.0; allow paperclip.shunkakinoki.com.paperclipDB viapsqlif missing..env.exampleand tests.Migration
DATABASE_URL=postgres://postgres:shunkakinoki@10.43.199.36:5432/paperclipto~/dotfiles/.envon Kyber.Written for commit b2b3afa. Summary will update on new commits.