Conversation
samxu01
left a comment
samxu01
left a comment
There was a problem hiding this comment.
Nice quality-of-life improvement for local bootstrap — .env auto-creation, optional-bot mode, and the docs split between "local defaults work" vs. "external PG needs CA" are all good moves.
Three concise inline notes:
- Postgres volume comment is factually wrong for the official
postgresimage; the mount works but the justification doesn't. get_env_valuedoes naive RHS extraction — fine for the three presence-checks today, fragile if reused for printing/comparing.- Bot's polling-loop "no accounts" branch no longer exits in non-optional mode, so a mid-run config loss turns into a 5s-cadence error log.
None are blockers. LGTM once the volume comment is corrected or the mount is narrowed back.
Generated by Claude Code
| # Local dev uses postgres:latest; mounting the parent dir keeps pace with | ||
| # versioned PGDATA paths like /var/lib/postgresql/<major>/docker. | ||
| - postgres-data-dev:/var/lib/postgresql |
There was a problem hiding this comment.
The comment's premise is incorrect: the official postgres Docker image hard-codes PGDATA=/var/lib/postgresql/data regardless of major version (the /<major>/docker layout is Debian's system package, not this image). Widening to /var/lib/postgresql still works but also captures the postgres user's home dir in the volume, and masks whatever actual bug motivated the change.
If there's a real issue (permissions on a specific host? a transient initdb failure?), document that; otherwise revert to /var/lib/postgresql/data — it's narrower and correct for postgres:latest.
Generated by Claude Code
| fi | ||
|
|
||
| if [ -f "$ENV_FILE" ]; then | ||
| local line | ||
| line=$(grep -E "^${key}=" "$ENV_FILE" | tail -n 1 || true) | ||
| if [ -n "$line" ]; then |
There was a problem hiding this comment.
${line#*=} returns the raw RHS including surrounding quotes and trailing comments. So FOO="bar" yields "bar" (literal quotes) and FOO=bar # note yields bar # note.
For the three keys checked here, -n still evaluates true so has_nonempty_env_value is correct in practice — but the moment this helper is used to print a value (or to compare against a literal), it bites. Either narrow it to a presence-check (grep -qE "^${key}=.\+") or strip quotes/comments.
Generated by Claude Code
| } | ||
| console.error('No agent tokens configured. Set COMMONLY_AGENT_TOKEN or COMMONLY_AGENT_CONFIG_PATH.'); | ||
| isPolling = false; | ||
| return; |
There was a problem hiding this comment.
Subtle behavior change: previously, !optionalRuntime + zero accounts was impossible inside the polling loop because startup already called process.exit(1). Now, if the runtime config file disappears mid-run (e.g. user deletes runtime.json), this branch fires every tick and logs "No agent tokens configured" indefinitely instead of failing loudly.
Low-probability in practice, but either exit here too (match startup semantics) or rate-limit the log like logWaitingForAccounts does.
Generated by Claude Code
|
Feel free to ignore nit comments and merge |
2 participants
Summary
./dev.shandcommonly-botdocker-compose.dev.ymlso the base compose remains production-stylecommonly-botprovisioningVerification
bash -n dev.shnode -c external/commonly-agent-services/commonly-bot/index.js./dev.sh upcommonly-botcan stay up in local optional mode instead of crash-looping before provisioningNotes
origin/main, so this PR branch was created from a clean clone oforigin/main