fix: paperclip authenticated mode and required config fields - #1345
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 8 minutes and 18 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughConfiguration templating enhancements are introduced across Paperclip's setup: metadata and logging configuration added to the JSON template, deployment mode parameter introduced in Nix with conditional logic, template substitution extended in hydrate.sh, and the EnvironmentFile directive removed from the systemd service definition. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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;DRSwitches Paperclip on Kyber to authenticated mode, adds required config metadata/logging, and restricts access to the expected hostname. Fixes 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 updates the Paperclip configuration template by adding metadata, logging settings, and server deployment options. It also updates the Nix expressions and hydration script to support a new deployment_mode variable and removes an unused environment file from the systemd service. Feedback suggests using dynamic placeholders for the configuration timestamp to avoid hardcoded dates and warns of a potential conflict between the application's file-based logging and systemd's log redirection.
| { | ||
| "$meta": { | ||
| "version": 1, | ||
| "updatedAt": "2026-04-04T00:00:00.000Z", |
There was a problem hiding this comment.
The updatedAt field is hardcoded to a static date. It is better to use a placeholder and populate it dynamically during the hydration process to reflect the actual configuration generation time.
| "updatedAt": "2026-04-04T00:00:00.000Z", | |
| "updatedAt": "__UPDATED_AT__", |
| "connectionString": "__DATABASE_CONNECTION_STRING__" | ||
| }, | ||
| "logging": { | ||
| "mode": "file" |
There was a problem hiding this comment.
Setting logging.mode to file may conflict with the systemd service configuration in home-manager/modules/paperclip/default.nix, which redirects StandardOutput and StandardError to a log file. If the application logs to its own file, it might bypass the systemd log capture or cause duplication. Consider using a mode that logs to stdout (e.g., console) to allow systemd to manage logs centrally via the journal or the configured redirection.
| @sed@ \ | ||
| -e "s|__DATABASE_MODE__|@database_mode@|g" \ | ||
| -e "s|__DATABASE_CONNECTION_STRING__|@database_connection_string@|g" \ | ||
| -e "s|__DEPLOYMENT_MODE__|@deployment_mode@|g" \ |
There was a problem hiding this comment.
Add a substitution for the __UPDATED_AT__ placeholder to dynamically set the configuration's last updated timestamp using the current system time during deployment. This follows the established pattern for Nix-processed scripts in this repository.
| -e "s|__DEPLOYMENT_MODE__|@deployment_mode@|g" \ | |
| -e "s|__DEPLOYMENT_MODE__|@deployment_mode@|g" \ | |
| -e "s|__UPDATED_AT__|$(date -u +'%Y-%m-%dT%H:%M:%S.000Z')|g" \ |
References
- Maintain consistency with established patterns for writing scripts that are extracted from Nix expressions, such as using @variable@ placeholders for build-time or deployment-time substitutions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
config/paperclip/config.template.json (1)
2-5: Consider generating$meta.updatedAtdynamically instead of hardcoding.Using a fixed timestamp at Line 4 will age quickly and make metadata less trustworthy. Prefer templating this value during hydration (or generation) so it reflects actual config creation/update time.
♻️ Suggested direction
- "updatedAt": "2026-04-04T00:00:00.000Z", + "updatedAt": "__UPDATED_AT__",And in
config/paperclip/hydrate.sh, add a substitution using a generated UTC timestamp before writing the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@config/paperclip/config.template.json` around lines 2 - 5, Replace the hardcoded "$meta.updatedAt" value with a templated placeholder in config/paperclip/config.template.json and update config/paperclip/hydrate.sh to substitute that placeholder with a generated UTC timestamp at hydration time (e.g., using date -u +%Y-%m-%dT%H:%M:%SZ) before writing the final config; target the "$meta.updatedAt" field in the template and perform the replacement in hydrate.sh so the produced config contains the actual creation/update time.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@config/paperclip/config.template.json`:
- Around line 2-5: Replace the hardcoded "$meta.updatedAt" value with a
templated placeholder in config/paperclip/config.template.json and update
config/paperclip/hydrate.sh to substitute that placeholder with a generated UTC
timestamp at hydration time (e.g., using date -u +%Y-%m-%dT%H:%M:%SZ) before
writing the final config; target the "$meta.updatedAt" field in the template and
perform the replacement in hydrate.sh so the produced config contains the actual
creation/update time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bfdf79cd-bf44-46e4-8e04-58d540abc278
📒 Files selected for processing (4)
config/paperclip/config.template.jsonconfig/paperclip/default.nixconfig/paperclip/hydrate.shhome-manager/modules/paperclip/default.nix
💤 Files with no reviewable changes (1)
- home-manager/modules/paperclip/default.nix
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adjusts the Paperclip Home Manager integration/config generation to work on Kyber (k8s-reachable bind address) and to satisfy Paperclip’s config validation requirements.
Changes:
- Switch Kyber to
authenticateddeployment mode (keeplocal_trustedelsewhere) and wire it into config hydration. - Extend
config.template.jsonwith required$metafields and aloggingsection. - Remove the systemd
EnvironmentFilereference for the Paperclip user service.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| home-manager/modules/paperclip/default.nix | Drops EnvironmentFile from the Paperclip systemd user service. |
| config/paperclip/hydrate.sh | Adds __DEPLOYMENT_MODE__ substitution during config generation. |
| config/paperclip/default.nix | Introduces deployment_mode and sets Kyber to authenticated. |
| config/paperclip/config.template.json | Adds required $meta + logging, and wires server.deploymentMode. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { | ||
| "$meta": { | ||
| "version": 1, | ||
| "updatedAt": "2026-04-04T00:00:00.000Z", |
There was a problem hiding this comment.
$meta.updatedAt is hard-coded to a fixed timestamp in the template, which will quickly become stale/misleading for regenerated configs. Consider making it a placeholder and populating it at hydration/activation time (or otherwise deriving it automatically) so the metadata reflects when the config was actually produced/updated.
| "updatedAt": "2026-04-04T00:00:00.000Z", | |
| "updatedAt": "__UPDATED_AT__", |
| @sed@ \ | ||
| -e "s|__DATABASE_MODE__|@database_mode@|g" \ | ||
| -e "s|__DATABASE_CONNECTION_STRING__|@database_connection_string@|g" \ | ||
| -e "s|__DEPLOYMENT_MODE__|@deployment_mode@|g" \ | ||
| -e "s|__HOST__|@host@|g" \ | ||
| "$TEMPLATE" >"$CONFIG" |
There was a problem hiding this comment.
A new __DEPLOYMENT_MODE__ substitution was added, but the existing ShellSpec coverage for this hydrate script doesn't assert that placeholder/substitution is present. Please extend spec/paperclip_hydrate_spec.sh to cover the new deployment mode substitution so regressions get caught.
* 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
authenticatedmode on kyber (local_trustedrejects0.0.0.0binding)$meta(version, updatedAt, source) andloggingfields to templateProblem
local_trustedmode rejects0.0.0.0host binding — but we need it for k8s to reach paperclip$meta.updatedAt,$meta.source(enum), andloggingsectionSummary by cubic
Switches Paperclip on Kyber to authenticated mode, adds required config metadata/logging, and restricts access to the expected hostname. Fixes
0.0.0.0binding rejection and config validation errors.server.deploymentModetoauthenticatedon Kyber (keeplocal_trustedelsewhere); required for k8s0.0.0.0binding; external auth stays at Cloudflare tunnel/nginx ingress.$meta(version, updatedAt, source) andloggingtoconfig.template.json.server.allowedHostnamesand setpaperclip.shunkakinoki.comon Kyber.__DEPLOYMENT_MODE__and__ALLOWED_HOSTNAME__via Nix andhydrate.sh. RemoveEnvironmentFilefrom the Paperclip systemd service.config.template.jsonfor consistency.Written for commit 0ef7968. Summary will update on new commits.