From 22cbf4a7d95c5e0c856d675a9a20d063420c3ce6 Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Fri, 7 Aug 2026 10:34:47 -0400 Subject: [PATCH 1/2] feat(cli): resolve PostgreSQL settings from app.yaml, not just env create_server_from_config built an explicit DatabaseConfig with mongodb's uri/name and dynamodb's table/region, but nothing for postgres. The DSN reached the driver only through jvspatial's own env read, so an app.yaml database.uri was silently ignored for postgres while working for every other backend -- the one case where a documented config path did nothing. Thread database.uri / pooler_mode / min_pool_size / max_pool_size through for postgres and postgresql. database.uri is shared with mongodb; database.type decides how it is read. Unset values stay None so PostgresDB's own defaults still apply, and a non-integer pool size is logged and ignored rather than failing startup -- a typo'd pool size should not stop a server from booting. Requires jvspatial 0.0.17. Until then DatabaseConfig discarded anything passed by field name (its aliased fields lacked populate_by_name), so this passthrough would have been written correctly and done nothing -- which is how that upstream bug was found. Pin bumped. Verified against the released 0.0.17 wheel, not a local checkout: full suite green, and scripts/smoke_postgres.sh 15/15 including recall across a server restart. Docs drop the "DSN is env-only" caveat and gain a worked app.yaml example. Also refreshes two stale pin references in jvspatial-integration.md sections 1 and 5. --- .env.example | 3 +- .planning/reference/jvspatial-integration.md | 4 +- CHANGELOG.md | 14 ++ docs/environment-keys-reference.md | 2 +- docs/postgres.md | 14 +- jvagent/cli/server_config.py | 69 +++++++++ pyproject.toml | 2 +- tests/cli/test_server_config_postgres.py | 146 +++++++++++++++++++ 8 files changed, 248 insertions(+), 6 deletions(-) create mode 100644 tests/cli/test_server_config_postgres.py diff --git a/.env.example b/.env.example index 3f6156aa..30dc2525 100644 --- a/.env.example +++ b/.env.example @@ -83,7 +83,8 @@ TYPESENSE_API_KEY=your-typesense-api-key-here # PostgreSQL — requires jvspatial >= 0.0.16 and the asyncpg driver # (pip install asyncpg). On 0.0.15 and earlier this fails at startup with # "ValueError: Unsupported database type: postgres". See docs/postgres.md. -# The DSN is read from the environment only — app.yaml database.uri is not wired. +# These also resolve from app.yaml config.database (uri / pooler_mode / +# min_pool_size / max_pool_size); env wins. See docs/postgres.md. # JVSPATIAL_DB_TYPE=postgres # JVSPATIAL_POSTGRES_DSN=postgresql://user:password@localhost:5432/jvagent_db # JVSPATIAL_POSTGRES_MIN_POOL_SIZE=2 diff --git a/.planning/reference/jvspatial-integration.md b/.planning/reference/jvspatial-integration.md index eed03247..8db76730 100644 --- a/.planning/reference/jvspatial-integration.md +++ b/.planning/reference/jvspatial-integration.md @@ -7,7 +7,7 @@ ## 1. Where jvspatial lives - **Source**: `/Users/eldonmarks/Briefcase/dev/jv/jvspatial` (sibling directory in this workspace). -- **Pip install**: declared in [`pyproject.toml`](../../pyproject.toml) as `jvspatial==0.0.16`. +- **Pip install**: declared in [`pyproject.toml`](../../pyproject.toml) as `jvspatial==0.0.17`. - **Own docs**: jvspatial has its own [`README.md`](../../../jvspatial/README.md) and [`SPEC.md`](../../../jvspatial/SPEC.md). Treat those as authoritative for anything below. --- @@ -171,7 +171,7 @@ Things jvagent **owns**: ## 5. Version policy -- Minimum required jvspatial: pinned in [`pyproject.toml`](../../pyproject.toml) as `jvspatial==X.Y.Z`. Current: `==0.0.16`. +- Minimum required jvspatial: pinned in [`pyproject.toml`](../../pyproject.toml) as `jvspatial==X.Y.Z`. Current: `==0.0.17`. - When jvspatial introduces breaking changes (e.g., walker API rename, persistence shape change), bump the pin and update this section. - When adding a new dependency on a jvspatial feature, document the symbol + version it was introduced in. Helps downstream consumers know the floor. - Rationale: [`adr/0006-jvspatial-dependency.md`](../adr/0006-jvspatial-dependency.md). diff --git a/CHANGELOG.md b/CHANGELOG.md index ed05a822..4a601049 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,20 @@ and this project adheres to [PEP 440](https://peps.python.org/pep-0440/) / ## [Unreleased] +### Added + +- **PostgreSQL settings resolve from `app.yaml`, not just env** (`jvagent/cli/server_config.py`). + `create_server_from_config` now threads `database.uri` / `pooler_mode` / + `min_pool_size` / `max_pool_size` into jvspatial's `DatabaseConfig` when + `database.type` is `postgres`/`postgresql`, alongside the mongodb and dynamodb + settings that were already wired. Previously the DSN reached the driver only + through jvspatial's own env read, so an `app.yaml` `database.uri` was silently + ignored for Postgres. Unset values stay `None` so `PostgresDB`'s defaults still + apply, and a non-integer pool size is logged and ignored rather than failing + startup. Requires `jvspatial >= 0.0.17` (pin bumped), which is where + `DatabaseConfig` began accepting values by field name. Coverage: + `tests/cli/test_server_config_postgres.py`. + ### Fixed - **No WARNING when ambient core parameters are re-unioned.** diff --git a/docs/environment-keys-reference.md b/docs/environment-keys-reference.md index 2a47566c..56367132 100644 --- a/docs/environment-keys-reference.md +++ b/docs/environment-keys-reference.md @@ -95,7 +95,7 @@ These are commonly used by `jvagent` and should be configured in `jvagent` deplo - `JVSPATIAL_DYNAMODB_TABLE_NAME` - `JVSPATIAL_DYNAMODB_REGION` - `JVSPATIAL_DYNAMODB_ENDPOINT_URL` -- `JVSPATIAL_POSTGRES_DSN` - PostgreSQL DSN. Env-only (no `app.yaml` path). Requires `jvspatial >= 0.0.16` — see [postgres.md](postgres.md). +- `JVSPATIAL_POSTGRES_DSN` - PostgreSQL DSN. Also settable as `config.database.uri`. Requires `jvspatial >= 0.0.16` — see [postgres.md](postgres.md). - `JVSPATIAL_POSTGRES_MIN_POOL_SIZE` - `JVSPATIAL_POSTGRES_MAX_POOL_SIZE` - `JVSPATIAL_POSTGRES_POOLER_MODE` - `session` (default) or `transaction` for PgBouncer / RDS Proxy. diff --git a/docs/postgres.md b/docs/postgres.md index aa00907e..35872087 100644 --- a/docs/postgres.md +++ b/docs/postgres.md @@ -22,7 +22,19 @@ Notes: - **The driver is an extra.** `pip install asyncpg` (or `jvspatial[postgres]`); it is not pulled in by jvagent's base dependencies. - **`JVSPATIAL_DB_PATH` does not apply.** [`server_config.py:161`](../jvagent/cli/server_config.py) only exports a path for `json` / `sqlite`. -- **The DSN reaches the driver via env only.** [`server_config.py:126-164`](../jvagent/cli/server_config.py) threads a connection string into jvspatial's `DatabaseConfig` for `mongodb` and table/region for `dynamodb`; there is no Postgres field, so `JVSPATIAL_POSTGRES_DSN` must be set in the environment — an `app.yaml` `database.uri` will **not** be picked up. +- **`app.yaml` works too.** Every key above also resolves from the `config.database` stanza, with the usual precedence (env wins): + + ```yaml + config: + database: + type: postgres + uri: ${JVSPATIAL_POSTGRES_DSN} # or a literal DSN, but keep credentials in env + pooler_mode: transaction + min_pool_size: 2 + max_pool_size: 10 + ``` + + `database.uri` is shared with mongodb — `database.type` decides how it is read. A non-integer pool size is logged and ignored rather than taking the server down at startup. - **Logging DB has no Postgres branch.** jvspatial's `logging/config.py` falls through to a `json` file log for any unrecognized type — silently. Set `JVSPATIAL_LOG_DB_TYPE=json` (or `mongodb`) explicitly so the fallback is a decision rather than a surprise. See [logging.md](logging.md). - **PageIndex is a separate store** with its own `JVAGENT_PAGEINDEX_DB_TYPE` (`json` by default) and is unaffected by the main graph backend. diff --git a/jvagent/cli/server_config.py b/jvagent/cli/server_config.py index 27f4aaeb..2dce27f5 100644 --- a/jvagent/cli/server_config.py +++ b/jvagent/cli/server_config.py @@ -60,6 +60,21 @@ def _ensure_session_token_header(cors_headers: Optional[list]) -> list: return base +def _optional_int(value: Any, key: str) -> Optional[int]: + """Coerce a config/env value to ``int``, or ``None`` if unusable. + + A typo'd pool size should not take the server down before it starts — + warn and fall through to the driver's own default instead. + """ + if normalize_empty(value) is None: + return None + try: + return int(value) + except (TypeError, ValueError): + logger.warning("%s is not an integer (%r); ignoring it.", key, value) + return None + + def _set_db_env_from_config(app_root: str) -> None: """Set database environment variables from app config. @@ -133,6 +148,55 @@ def create_server_from_config(debug: bool = False, app_root: str = None) -> Serv if normalize_empty(mongodb_uri) is None: mongodb_uri = env("JVSPATIAL_MONGODB_URI", default="mongodb://localhost:27017") + # PostgreSQL configuration (jvspatial >= 0.0.16). ``database.uri`` is shared + # with mongodb — ``database.type`` decides how it is read. + is_postgres = db_type in ("postgres", "postgresql") + postgres_dsn = ( + normalize_empty( + get_config_value(app_config, "database.uri", "JVSPATIAL_POSTGRES_DSN", None) + ) + if is_postgres + else None + ) + postgres_pooler_mode = ( + normalize_empty( + get_config_value( + app_config, + "database.pooler_mode", + "JVSPATIAL_POSTGRES_POOLER_MODE", + None, + ) + ) + if is_postgres + else None + ) + postgres_min_pool_size = ( + _optional_int( + get_config_value( + app_config, + "database.min_pool_size", + "JVSPATIAL_POSTGRES_MIN_POOL_SIZE", + None, + ), + "JVSPATIAL_POSTGRES_MIN_POOL_SIZE", + ) + if is_postgres + else None + ) + postgres_max_pool_size = ( + _optional_int( + get_config_value( + app_config, + "database.max_pool_size", + "JVSPATIAL_POSTGRES_MAX_POOL_SIZE", + None, + ), + "JVSPATIAL_POSTGRES_MAX_POOL_SIZE", + ) + if is_postgres + else None + ) + # DynamoDB configuration dynamodb_table_name = get_config_value( app_config, "database.table_name", "JVSPATIAL_DYNAMODB_TABLE_NAME", None @@ -325,6 +389,11 @@ def create_server_from_config(debug: bool = False, app_root: str = None) -> Serv dynamodb_secret_access_key=( dynamodb_secret_access_key if db_type == "dynamodb" else None ), + # Unset values stay None so PostgresDB's own defaults still apply. + postgres_dsn=postgres_dsn, + postgres_pooler_mode=postgres_pooler_mode, + postgres_min_pool_size=postgres_min_pool_size, + postgres_max_pool_size=postgres_max_pool_size, ) # Auth configuration - merge default exempt paths with app-specific (auth.exempt_paths) diff --git a/pyproject.toml b/pyproject.toml index efbe7dbf..712b5d3a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -31,7 +31,7 @@ classifiers = [ dependencies = [ "aiohttp>=3.9.0", # CI records the resolved jvspatial version after install (see .github/workflows/test-jvagent.yaml). - "jvspatial==0.0.16", + "jvspatial==0.0.17", "python-dotenv>=1.0.0", "pyyaml>=6.0.0", "httpx>=0.27.0", diff --git a/tests/cli/test_server_config_postgres.py b/tests/cli/test_server_config_postgres.py new file mode 100644 index 00000000..a6049088 --- /dev/null +++ b/tests/cli/test_server_config_postgres.py @@ -0,0 +1,146 @@ +"""create_server_from_config: PostgreSQL settings reach jvspatial's DatabaseConfig. + +Every other backend has its connection settings threaded into the config +object (mongodb its uri/name, dynamodb its table/region). Postgres did not, +so an ``app.yaml`` ``database.uri`` was silently ignored and only the +driver's own env read made a connection possible. +""" + +import pytest + +POSTGRES_APP_YAML = """ +app: pg_config_test +context: + name: test + description: test +config: + database: + type: postgres + logging: + enabled: false + server: + host: 127.0.0.1 + port: 8765 +agents: [] +""" + +YAML_DSN_APP_YAML = """ +app: pg_config_test +context: + name: test + description: test +config: + database: + type: postgres + uri: postgresql://yaml:pw@yamlhost:5432/yamldb + pooler_mode: transaction + min_pool_size: 3 + max_pool_size: 9 + logging: + enabled: false + server: + host: 127.0.0.1 + port: 8765 +agents: [] +""" + +JSON_APP_YAML = """ +app: pg_config_test +context: + name: test + description: test +config: + database: + type: json + path: ./test_jvdb + logging: + enabled: false +agents: [] +""" + +_PG_ENV = ( + "JVSPATIAL_POSTGRES_DSN", + "JVSPATIAL_POSTGRES_POOLER_MODE", + "JVSPATIAL_POSTGRES_MIN_POOL_SIZE", + "JVSPATIAL_POSTGRES_MAX_POOL_SIZE", +) + + +@pytest.fixture +def build_server(tmp_path, monkeypatch): + """Build a Server from an app.yaml body, with a clean Postgres env.""" + from jvagent.cli.server_config import create_server_from_config + from jvagent.core.app_context import clear_app_root, set_app_root + + monkeypatch.setenv("JVSPATIAL_JWT_SECRET_KEY", "test-secret-for-pg-config-tests") + monkeypatch.setenv("JVAGENT_ADMIN_PASSWORD", "x") + for key in _PG_ENV: + monkeypatch.delenv(key, raising=False) + + def _build(app_yaml: str): + app_root = str(tmp_path) + (tmp_path / "app.yaml").write_text(app_yaml.strip(), encoding="utf-8") + set_app_root(app_root) + try: + return create_server_from_config(debug=False, app_root=app_root) + finally: + clear_app_root() + + yield _build + + +def test_dsn_from_env_reaches_database_config(build_server, monkeypatch): + monkeypatch.setenv("JVSPATIAL_POSTGRES_DSN", "postgresql://u:pw@envhost:5432/envdb") + db = build_server(POSTGRES_APP_YAML).config.database + assert db.db_type == "postgres" + assert db.postgres_dsn == "postgresql://u:pw@envhost:5432/envdb" + + +def test_settings_from_app_yaml_reach_database_config(build_server): + db = build_server(YAML_DSN_APP_YAML).config.database + assert db.postgres_dsn == "postgresql://yaml:pw@yamlhost:5432/yamldb" + assert db.postgres_pooler_mode == "transaction" + assert db.postgres_min_pool_size == 3 + assert db.postgres_max_pool_size == 9 + + +def test_env_overrides_app_yaml(build_server, monkeypatch): + """Documented precedence: env var > app.yaml > default.""" + monkeypatch.setenv("JVSPATIAL_POSTGRES_DSN", "postgresql://u:pw@envhost:5432/envdb") + monkeypatch.setenv("JVSPATIAL_POSTGRES_MAX_POOL_SIZE", "25") + db = build_server(YAML_DSN_APP_YAML).config.database + assert db.postgres_dsn == "postgresql://u:pw@envhost:5432/envdb" + assert db.postgres_max_pool_size == 25 + # Untouched by env, so the YAML value still applies. + assert db.postgres_pooler_mode == "transaction" + + +def test_unset_settings_stay_none_for_driver_defaults(build_server): + """Omitted settings must not be pinned here — PostgresDB has its own defaults.""" + db = build_server(POSTGRES_APP_YAML).config.database + assert db.postgres_dsn is None + assert db.postgres_pooler_mode is None + assert db.postgres_min_pool_size is None + assert db.postgres_max_pool_size is None + + +def test_postgres_settings_ignored_for_other_backends(build_server, monkeypatch): + """A stray Postgres DSN must not ride along on a json deployment.""" + monkeypatch.setenv("JVSPATIAL_POSTGRES_DSN", "postgresql://u:pw@envhost:5432/envdb") + db = build_server(JSON_APP_YAML).config.database + assert db.db_type == "json" + assert db.postgres_dsn is None + + +def test_postgresql_alias_is_honored(build_server, monkeypatch): + monkeypatch.setenv("JVSPATIAL_POSTGRES_DSN", "postgresql://u:pw@envhost:5432/envdb") + db = build_server(POSTGRES_APP_YAML.replace("type: postgres", "type: postgresql")) + assert db.config.database.postgres_dsn == "postgresql://u:pw@envhost:5432/envdb" + + +def test_non_integer_pool_size_is_ignored(build_server, monkeypatch): + """A typo'd pool size must not crash startup; the driver default applies.""" + monkeypatch.setenv("JVSPATIAL_POSTGRES_DSN", "postgresql://u:pw@envhost:5432/envdb") + monkeypatch.setenv("JVSPATIAL_POSTGRES_MAX_POOL_SIZE", "not-a-number") + db = build_server(POSTGRES_APP_YAML).config.database + assert db.postgres_max_pool_size is None From 5770f194330a7d3df8eef3cdc84e9c8ba4060cad Mon Sep 17 00:00:00 2001 From: Eldon Marks Date: Fri, 7 Aug 2026 10:41:17 -0400 Subject: [PATCH 2/2] fix(test): declare asyncpg so the postgres config tests run in CI The new tests build a real Server with db_type=postgres, which now instantiates PostgresDB and imports asyncpg. asyncpg is an optional extra, so CI did not have it and the job failed with ImportError -- while passing locally purely because this machine had the driver installed from earlier Postgres work. A test that depends on an undeclared extra is a test that passes for the wrong reason. Add asyncpg to the [test] extra so CI exercises the feature, and guard the module with pytest.importorskip so anyone running the suite without the extra skips instead of erroring (matching jvspatial's own postgres tests). Verified both ways: 7 passed with the driver, cleanly skipped without it. Also syncs requirements.txt and requirements-all.txt, which still pinned jvspatial==0.0.15 -- stale since the 0.0.16 bump and now two releases behind pyproject. --- pyproject.toml | 4 ++++ requirements-all.txt | 2 +- requirements.txt | 2 +- tests/cli/test_server_config_postgres.py | 5 +++++ 4 files changed, 11 insertions(+), 2 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 712b5d3a..e383fbe4 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -77,6 +77,10 @@ test = [ # web_fetch tests (same stack as jvagent/action/web_fetch/info.yaml) "beautifulsoup4", "markdownify", + # PostgreSQL config tests build a real Server with db_type=postgres, which + # instantiates PostgresDB and therefore needs the driver. Without it the + # suite passes only on machines that happen to have asyncpg installed. + "asyncpg>=0.29.0", ] # Optional: cluster-wide conversation locks (Redis SET NX or DynamoDB). diff --git a/requirements-all.txt b/requirements-all.txt index 83c7a972..ac3a5f0d 100644 --- a/requirements-all.txt +++ b/requirements-all.txt @@ -3,7 +3,7 @@ # Install with: pip install -r requirements-all.txt # Core jvagent dependencies -jvspatial==0.0.15 +jvspatial==0.0.17 python-dotenv>=1.0.0 pyyaml>=6.0.0 httpx>=0.27.0 diff --git a/requirements.txt b/requirements.txt index 53550d56..c0fab01a 100644 --- a/requirements.txt +++ b/requirements.txt @@ -1,6 +1,6 @@ # Core runtime. Test-only deps (incl. Docling for PageIndex): pyproject.toml # [project.optional-dependencies] test — install with: pip install -e ".[test]" -jvspatial==0.0.15 +jvspatial==0.0.17 python-dotenv>=1.0.0 pyyaml>=6.0.0 httpx>=0.27.0 diff --git a/tests/cli/test_server_config_postgres.py b/tests/cli/test_server_config_postgres.py index a6049088..c44af86c 100644 --- a/tests/cli/test_server_config_postgres.py +++ b/tests/cli/test_server_config_postgres.py @@ -8,6 +8,11 @@ import pytest +# Building a Server with db_type=postgres instantiates PostgresDB, which imports +# asyncpg. It ships in the [test] extra; skip rather than error for anyone +# running the suite without it. +pytest.importorskip("asyncpg") + POSTGRES_APP_YAML = """ app: pg_config_test context: