Skip to content

docs: agent & strategy persistence design + plan#113

Merged
FlyM1ss merged 4 commits into
mainfrom
docs/agent-db-persistence
Jul 15, 2026
Merged

docs: agent & strategy persistence design + plan#113
FlyM1ss merged 4 commits into
mainfrom
docs/agent-db-persistence

Conversation

@FlyM1ss

@FlyM1ss FlyM1ss commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Approved design spec + task-by-task implementation plan for making user-created content survive Render redeploys: external_agents, agent_versions, and strategies get optional Postgres backends behind one new env var CONTENT_DATABASE_URL, cloning the shipped USERS_DATABASE_URL users fix (hand-written twin class per store, per-module factory, singletons unchanged). Deliberately not named DATABASE_URL — that is the Heroku-convention name add-ons inject and unrelated projects export, and an ambient value would silently bind the app to the wrong database.

Docs only — no code changes; safe to merge on its own. Implementation lands in a follow-up PR off the plan (docs/superpowers/plans/2026-07-15-agent-strategy-persistence.md), which opens as a draft and carries its own gate: CONTENT_DATABASE_URL must be set in the Render dashboard before that PR merges (unset silently selects ephemeral SQLite).

🤖 Generated with Claude Code

https://claude.ai/code/session_01VPTbEcj871EPmEyXwJb2Fi

FlyM1ss and others added 2 commits July 15, 2026 10:44
Task-by-task TDD plan for the DATABASE_URL Postgres backends spec
(docs/superpowers/specs/2026-07-15-agent-strategy-persistence-design.md).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VPTbEcj871EPmEyXwJb2Fi
@vercel

vercel Bot commented Jul 15, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
agentic-trading-lab Ready Ready Preview, Comment Jul 15, 2026 5:58am

Review of #113 raised 5 findings; adversarial verification of the fixes
surfaced a 6th that defeated one of them. All are folded in here.

- CI had no Postgres service, so the @pg_only tier skipped on every PR and
  ~450 lines of new SQL would reach prod unexecuted -- where the first run is
  _init_schema() at import time under fail-loud semantics, on a free tier with
  no zero-downtime deploys. New Task 2 adds a postgres:16-alpine service +
  TEST_POSTGRES_URL, plus a guard test so dropping it fails loudly instead of
  silently reverting to all-skip.
- "backend: postgres" could not distinguish the intended Neon DB from a stray
  DATABASE_URL. New Task 3 adds db_url.py::describe_database_url() -> host/db,
  defined once because it is credential-scrubbing code.
- print(), not logger.info(): uvicorn's LOGGING_CONFIG has no 'root' key, so
  dashboard.backend.* loggers sit at WARNING and info() emits nothing in prod.
  caplog.at_level() hides this -- green suite, silent prod, which is the exact
  failure this feature exists to kill. Verified by reproduction.
- "Fail loud" was a hard constraint with no tests; added one per twin (needs no
  live Postgres, and is the only tier that runs a twin's real __init__).
- test_strategy_store.py ALREADY EXISTS at tests/domain/strategies/; the plan
  said "Create" at tests/. Now Modify, reusing its _store() helper.
- Reconciled the spec/plan contradiction on ALTER TABLE migrations (none are
  needed; the tables are born with every column).
- Merge discipline: the PR opens as a draft with the DO NOT MERGE gate in its
  first line, and marking it ready stays a human decision after the Render env
  var is set. A gate that lives only in a plan file is not a gate.
- Dropped a miscited test (test_agent_repository_compatibility.py builds stores
  directly and is indifferent to DATABASE_URL) that would have shipped into a
  committed docstring.

Verified rather than asserted: describe_database_url and its tests run green;
psycopg raises OperationalError on a refused port in 0.00s; the collision tests
pass against the real StrategyStore (calls == 21); the CI yaml parses and
deploy-prod cannot fire from a draft PR; print() survives uvicorn's real config
where logger.info() does not.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VPTbEcj871EPmEyXwJb2Fi
@FlyM1ss

FlyM1ss commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Reviewed this branch and pushed the fixes as 515dc91. Six findings; five from the review, one found while adversarially verifying the fixes — that one defeated another fix, so it's the important one.

The Postgres half would have shipped to prod unexecuted. CI has no services: block, so TEST_POSTGRES_URL is never set and every @pg_only test skips on every PR — and the plan called running them locally "optional". Prod's first execution is _init_schema() at import time under fail-loud semantics, on a free tier with no zero-downtime deploys, so a DDL typo is an outage rather than a bug. New Task 2 adds a postgres:16-alpine service + a guard test, so dropping the block later fails loudly instead of silently reverting to all-skip.

logger.info() is dead code in this app. uvicorn's LOGGING_CONFIG has no root key, nothing under dashboard/backend/ configures logging, and no launch path passes --log-level — so dashboard.backend.* loggers sit at WARNING and isEnabledFor(INFO) is False. Every dispatch test would have been green anyway, because caplog.at_level() force-sets the level for the test. Green suite, silent prod — the exact failure this feature exists to kill, reintroduced by the fix meant to prevent it. All four factories now print() (the convention in ~25 modules here, including app.py's startup block), and tests assert on capsys.

backend: postgres couldn't say which postgres. DATABASE_URL is the most collision-prone name in the ecosystem, and the line read identically for the intended Neon DB and a stray one. New Task 3 adds db_url.py::describe_database_url()host[:port]/dbname, defined once because it's credential-scrubbing code. (Its first draft leaked: urlsplit dumps a psycopg keyword/DSN string — password and all — into .path, so the "safe" fallback would have logged the credential verbatim. It now keys off hostname and echoes nothing.) The env var name stays DATABASE_URL — that was a settled decision — with the residual risk documented and CONTENT_DATABASE_URL recorded as the escape hatch.

Also: "fail loud" was a hard constraint with zero tests (added one per twin — no live Postgres needed, and it's the only tier that runs a twin's real __init__); test_strategy_store.py already exists at tests/domain/strategies/ and the plan said "Create" it at tests/, so that's now Modify; the spec/plan ALTER-migration contradiction is reconciled; the PR now opens as a draft with the DO NOT MERGE gate in its first line, since a gate living only in a plan file is not a gate; and a miscited test was dropped before it shipped into a committed docstring.

Verified rather than asserted: describe_database_url + its tests run green; psycopg raises OperationalError on a refused port in 0.00s; the collision tests pass against the real StrategyStore (calls == 21); the CI yaml parses and deploy-prod cannot fire from a draft PR; and print() survives uvicorn's real config where logger.info() doesn't.

Still docs-only and safe to merge. Task count went 7 → 9.

Reverses Decision 2's naming call. DATABASE_URL is the Heroku de-facto
standard: auto-injected by managed-Postgres add-ons and plausibly already
exported in a dev's shell, so an ambient value could silently bind the app
to the wrong database. Nothing can protect a local uvicorn run from that --
reading the env var is the feature -- so the collision is designed out of
the name instead of merely logged.

Drops the users-store fallback as a consequence: a var named for content
has no business selecting the account database. USERS_DATABASE_URL and
CONTENT_DATABASE_URL are now scoped per store and never resolve against
each other; a fully durable deployment sets both at the same Neon DB. Prod
already sets USERS_DATABASE_URL, so its behavior is unchanged.

Not a find/replace -- the prose that argued for the old name was rewritten:
- Decision 2 becomes a settled choice; the log-line rationale is re-anchored
  to typo'd/staging URLs and the unset->SQLite branch, which the rename does
  NOT address (so a later reader can't read it as redundant).
- Task 4 shrinks to "log line only"; its two fallback tests are replaced by
  one inverted test pinning that users IGNORES CONTENT_DATABASE_URL, so
  re-adding the fallback can't pass green.
- Task 1's urgency recalibrated: a scoped name narrows the ambient trigger.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VPTbEcj871EPmEyXwJb2Fi
@FlyM1ss

FlyM1ss commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

f911884 — renamed the content-store env var DATABASE_URLCONTENT_DATABASE_URL, which reverses Decision 2's naming call and takes the escape hatch the spec itself had recorded.

Two settled changes:

1. Scoped name. DATABASE_URL is the Heroku de-facto standard — auto-injected by managed-Postgres add-ons, plausibly already exported in a dev's shell for another project. Nothing can protect a local uvicorn run from an ambient value (reading the env var is the feature), so the collision is designed out of the name rather than merely logged.

2. Users fallback dropped (the "cost of the one-var goal" the old Decision 2 predicted). USERS_DATABASE_URL and CONTENT_DATABASE_URL are scoped per store and never resolve against each other — a var named for content shouldn't select the account database. A fully durable deployment sets both, pointed at the same Neon DB. Prod already sets USERS_DATABASE_URL, so its behavior is unchanged; rollout still only needs the one new var added in the Render dashboard.

Not a find/replace — the prose that argued for the old name was rewritten:

  • Decision 2 now reads as a settled choice. Its log-line rationale is re-anchored to typo'd/staging URLs and the unset→SQLite branch — the risks the rename does not address — so a later reader can't conclude the rename made the host/dbname log line redundant.
  • Task 4 shrinks from "build the fallback + log line" to "log line only" (users.py:313 already reads USERS_DATABASE_URL alone, so selection genuinely doesn't change). Its two fallback tests are replaced by one inverted test pinning that the users factory ignores CONTENT_DATABASE_URL — otherwise re-adding the fallback would pass green.
  • Task 1's urgency recalibrated honestly: a scoped name narrows the ambient trigger, though the strip is still worth its two lines and its .db_path job is unaffected.

Verified with a 4-lens review + adversarial refutation, which caught two mechanical-rename leftovers I'd missed (Task 3's intro and the File Structure table both still called the new name "the most collision-prone name in the ecosystem" / "Precedence"); a follow-up grep caught a third in describe_database_url()'s docstring. All three fixed. No code changes — docs only.

@FlyM1ss FlyM1ss merged commit 4eea117 into main Jul 15, 2026
5 checks passed
@FlyM1ss FlyM1ss deleted the docs/agent-db-persistence branch July 15, 2026 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant