feat(schema): provision schema externally; drop runtime DDL and defaults seed#27
Merged
jeffersonrodrigues92 merged 2 commits intoMay 27, 2026
Merged
Conversation
Contributor
🔍 PR Validation Summary🚫 PR Blocked — 1 blocking failure
|
WalkthroughThis PR removes all runtime schema provisioning and default seeding from the library. The Manager and Postgres Store no longer create tables, indexes, or NOTIFY triggers at runtime; consumers must provision these externally. Warm-load tolerates missing tables, and database role permissions are restricted to DML + LISTEN. ChangesRemove Runtime Schema Provisioning and Default Seeding
Comment |
Contributor
🔒 Security Scan Results —
|
| Stage | Status | Blocking? |
|---|---|---|
| Filesystem Scan | ✅ Clean | — |
| Docker Image Scan | ➖ Skipped | — |
| Docker Hub Health Score | ➖ Skipped | — |
| Pre-release Version Check | ✅ Clean | — |
Trivy
Filesystem Scan
✅ No vulnerabilities or secrets found.
Pre-release Version Check
✅ No unstable version pins found.
Contributor
📊 Unit Test Coverage Report:
|
| Metric | Value |
|---|---|
| Overall Coverage | 80.4% ✅ PASS |
| Threshold | 80% |
Coverage by Package
| Package | Coverage |
|---|---|
github.com/LerianStudio/lib-systemplane/admin |
88.4% |
github.com/LerianStudio/lib-systemplane/internal/client |
80.7% |
github.com/LerianStudio/lib-systemplane/internal/debounce |
73.1% |
github.com/LerianStudio/lib-systemplane/internal/manager |
87.9% |
github.com/LerianStudio/lib-systemplane/internal/mongodb |
97.5% |
github.com/LerianStudio/lib-systemplane/internal/postgres |
100.0% |
github.com/LerianStudio/lib-systemplane |
94.8% |
Generated by Go PR Analysis workflow
…lts seed lib-systemplane no longer creates its schema or seeds defaults at runtime. The Postgres store and the multi-tenant Manager previously ran CREATE TABLE / CREATE FUNCTION / CREATE TRIGGER plus an INSERT ... ON CONFLICT DO NOTHING defaults seed on first use (Store.Start / OnTenantActivated). Those runtime DDL/seed paths are removed. Consumers now provision systemplane_entries (plus systemplane_notify_v3() and the NOTIFY triggers) and any defaults externally using the published SchemaSQL()/DefaultSeedSQL() (e.g. via their migration pipeline). The runtime database role needs only DML + LISTEN — no CREATE on the schema — which aligns with the least-privilege per-tenant roles the tenant-manager hands back (runtime DDL otherwise fails with permission denied for schema ... (42501)). No current consumers depend on the removed runtime bootstrap, so this is a behavior change with no expected real-world breakage — shipped as a non-breaking minor. Postgres store: - Remove runSchema and its CREATE TABLE/FUNCTION/TRIGGER builders (postgres_schema.go deleted). - Remove ensureSchema / schemaOnce / schemaErr lazy-bootstrap machinery and the Start()/resolveDB() calls into them. Start now only opens LISTEN; resolveDB only resolves the tenant handle. Manager: - OnTenantActivated no longer runs schema creation or the runtime seed; it warm-loads + opens LISTEN only. - Remove runSchemaAndSeed, runSchema, and seedDefaults. - warmLoad tolerates a not-yet-provisioned table (SQLSTATE 42P01): it logs at WARN and proceeds with an empty, non-stale cache instead of failing activation; LISTEN/poll refreshes once the table exists. SchemaSQL()/DefaultSeedSQL() are intact and are now the ONLY way the schema/defaults are expressed. Tests: - Integration tests provision the table via the published DDL in setup instead of relying on runtime auto-create. - Add a least-privilege Postgres integration test (DML-only role, no CREATE) proving Get/Set/Delete succeed with no runtime DDL. - Add a Manager integration test proving activation tolerates a missing table, and a unit test for the 42P01 detection helper. Recommended next version: v1.7.0 (next beta v1.7.0-beta.1). Not tagged here.
jeffersonrodrigues92
force-pushed
the
feat/remove-runtime-schema-bootstrap
branch
from
86b8868 to
5dbe77b
Compare
jeffersonrodrigues92
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/manager/lifecycle.go`:
- Around line 17-23: Update the exported documentation for OnTenantActivated in
manager_methods.go to reflect the new activation flow: remove any wording that
says activation creates schema or seeds defaults and instead document that
OnTenantActivated performs a warm-load of registered keys into the per-tenant
cache and opens the LISTEN goroutine, tolerating a missing table (logs and
proceeds with an empty cache) while external migrations handle DDL/seed; also
add or update a package-level example that uses OnTenantActivated and shows the
expected behavior with warm-load + LISTEN so the public docs compile and match
the current API names.
In `@internal/manager/manager_integration_test.go`:
- Around line 271-295: Test currently only checks initial miss and never
exercises the post-42P01 LISTEN path; after calling m.OnTenantActivated(...) add
steps to provision the missing schema/table in the fake tenant (use fc or
resolver helper used earlier), then perform an insert/update that would trigger
the LISTEN/NOTIFY path and finally call m.Lookup(ctx, "tenant-miss", "ns", "k")
asserting hit==true and value==expected; in other words, after activation: 1)
create the missing table/schema for tenant-miss, 2) perform the change that
would notify the manager, and 3) assert Lookup now returns a cache hit so you
verify the manager kept listening.
In `@internal/manager/warmload.go`:
- Around line 55-66: When isUndefinedTable(err) is hit in warmload.go, the code
currently flips ts.stale to false but leaves ts.entries populated, which can
expose stale values; update the handler so you acquire ts.mu, clear or
reinitialize ts.entries (e.g., set to nil or an empty map/slice as appropriate
for the type), then set ts.stale = false and release the lock — ensure the
clearing and stale flip happen under the same mutex to remain concurrency-safe
(apply this change in the same block where isUndefinedTable(err) is checked and
m.logWarn is called, referencing ts, ts.mu and ts.entries).
In `@internal/postgres/postgres_integration_test.go`:
- Around line 323-342: The test revokes CREATE from the new role but doesn't
prevent CREATE being granted via PUBLIC or another group; add a hardening check
or revoke to ensure the role cannot create in public: after creating roleName
(and before starting the store) execute a query using owner.Exec/owner.Query to
check SELECT has_schema_privilege('<roleName>', 'public', 'CREATE') and fail the
test if it returns true, or explicitly run owner.Exec to REVOKE CREATE ON SCHEMA
public FROM PUBLIC (and verify the revoke succeeded) so the created role truly
lacks CREATE on schema public; update the stmts/validation accordingly to
reference roleName and owner.Exec.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9e93797e-22f7-4671-89b2-43d2c3d125b0
📒 Files selected for processing (17)
CHANGELOG.mdCLAUDE.mdREADME.mdexamples/manager/main.gointernal/manager/connector.gointernal/manager/lifecycle.gointernal/manager/listen_integration_test.gointernal/manager/manager_integration_test.gointernal/manager/schema.gointernal/manager/schema_integration_test.gointernal/manager/schema_test.gointernal/manager/testhelpers_integration_test.gointernal/manager/warmload.gointernal/manager/warmload_test.gointernal/postgres/postgres.gointernal/postgres/postgres_integration_test.gointernal/postgres/postgres_schema.go
💤 Files with no reviewable changes (3)
- internal/postgres/postgres_schema.go
- internal/manager/schema_integration_test.go
- internal/manager/schema_test.go
Comment on lines
+55
to
+66
| if isUndefinedTable(err) { | ||
| m.logWarn(ctx, "manager warm-load: systemplane table not provisioned yet, proceeding with empty cache", | ||
| log.String("table", defaultTable), | ||
| log.Err(err), | ||
| ) | ||
|
|
||
| ts.mu.Lock() | ||
| ts.stale = false | ||
| ts.mu.Unlock() | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Clear cached entries on 42P01 before marking the tenant cache fresh.
At Line 62, ts.stale is flipped to false, but existing ts.entries are left intact. On re-activation paths this can expose stale values while the table is missing, despite the intended “empty cache” fallback.
Suggested fix
if isUndefinedTable(err) {
m.logWarn(ctx, "manager warm-load: systemplane table not provisioned yet, proceeding with empty cache",
log.String("table", defaultTable),
log.Err(err),
)
ts.mu.Lock()
+ clear(ts.entries)
ts.stale = false
ts.mu.Unlock()
return nil
}Based on learnings: "Keep behavior nil-safe and concurrency-safe by default in all Client operations and public API methods."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if isUndefinedTable(err) { | |
| m.logWarn(ctx, "manager warm-load: systemplane table not provisioned yet, proceeding with empty cache", | |
| log.String("table", defaultTable), | |
| log.Err(err), | |
| ) | |
| ts.mu.Lock() | |
| ts.stale = false | |
| ts.mu.Unlock() | |
| return nil | |
| } | |
| if isUndefinedTable(err) { | |
| m.logWarn(ctx, "manager warm-load: systemplane table not provisioned yet, proceeding with empty cache", | |
| log.String("table", defaultTable), | |
| log.Err(err), | |
| ) | |
| ts.mu.Lock() | |
| clear(ts.entries) | |
| ts.stale = false | |
| ts.mu.Unlock() | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/manager/warmload.go` around lines 55 - 66, When
isUndefinedTable(err) is hit in warmload.go, the code currently flips ts.stale
to false but leaves ts.entries populated, which can expose stale values; update
the handler so you acquire ts.mu, clear or reinitialize ts.entries (e.g., set to
nil or an empty map/slice as appropriate for the type), then set ts.stale =
false and release the lock — ensure the clearing and stale flip happen under the
same mutex to remain concurrency-safe (apply this change in the same block where
isUndefinedTable(err) is checked and m.logWarn is called, referencing ts, ts.mu
and ts.entries).
Comment on lines
+323
to
+342
| // Create a least-privilege role with DML only — explicitly NO CREATE on | ||
| // the public schema. This mirrors the role the tenant-manager hands the | ||
| // runtime. | ||
| roleName := fmt.Sprintf("sp_dml_%d", time.Now().UnixNano()) | ||
| rolePass := "dmlpass" | ||
|
|
||
| if _, err := owner.Exec(fmt.Sprintf(`CREATE ROLE %s LOGIN PASSWORD '%s'`, roleName, rolePass)); err != nil { | ||
| t.Fatalf("create role: %v", err) | ||
| } | ||
|
|
||
| stmts := []string{ | ||
| fmt.Sprintf(`REVOKE CREATE ON SCHEMA public FROM %s`, roleName), | ||
| fmt.Sprintf(`GRANT USAGE ON SCHEMA public TO %s`, roleName), | ||
| fmt.Sprintf(`GRANT SELECT, INSERT, UPDATE, DELETE ON systemplane_entries TO %s`, roleName), | ||
| } | ||
| for _, stmt := range stmts { | ||
| if _, err := owner.Exec(stmt); err != nil { | ||
| t.Fatalf("grant stmt %q: %v", stmt, err) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PostgreSQL, does REVOKE CREATE ON SCHEMA public FROM some_rolestill leave that role able to create objects via the defaultPUBLICgrant on schemapublic? What SQL guarantees has_schema_privilege(some_role, 'public', 'CREATE') is false in a fresh database?
💡 Result:
In PostgreSQL, if you execute REVOKE CREATE ON SCHEMA public FROM some_role, that role will no longer be able to create objects in the public schema, provided it does not hold the CREATE privilege through another source [1][2]. Importantly, privileges in PostgreSQL are additive [1][3]. If the role also has the CREATE privilege granted via the PUBLIC pseudo-role—which is the default in many PostgreSQL versions—you must explicitly revoke it from PUBLIC to effectively restrict that role (and all others) [1][3][4]. If you only revoke CREATE from a specific role but leave the PUBLIC grant intact, the role will still possess the CREATE privilege because it is a member of the implicit PUBLIC group [1][5]. To ensure that has_schema_privilege(some_role, 'public', 'CREATE') returns false in a fresh database for a specific role, you must perform the following actions: 1. Revoke the CREATE privilege from the PUBLIC pseudo-role to remove the default access for all users [3][2][4]: REVOKE CREATE ON SCHEMA public FROM PUBLIC; 2. Ensure no other roles or group memberships grant the CREATE privilege to some_role, as privileges are the sum of all direct and indirect grants [1][3]. You can verify the effective state for a specific role using the following SQL query [6][7]: SELECT has_schema_privilege('some_role', 'public', 'CREATE') AS has_create_privilege; If this query returns false, the role does not have the ability to create objects in the public schema [8][6]. Note that PostgreSQL 15 and later versions have changed default security behaviors, where the CREATE privilege on the public schema is no longer granted to PUBLIC by default [9].
Citations:
- 1: https://www.postgresql.org/docs/18/sql-revoke.html
- 2: https://severalnines.com/blog/postgresql-privileges-and-security-locking-down-public-schema/
- 3: https://dba.stackexchange.com/questions/261542/postgres-revoke-access-to-public-schema-for-a-user
- 4: https://medium.com/@jramcloud1/understanding-the-public-schema-and-search-path-in-postgresql-a-practical-guide-b8b550fab9cc
- 5: https://www.postgresql.org/docs/current/sql-revoke.html
- 6: https://docs.vultr.com/support/products/managed-databases/how-to-fix-permission-denied-for-schema-public-error-in-postgresql
- 7: https://stackoverflow.com/questions/22715053/postgresql-view-schema-privileges
- 8: https://docs.oxla.com/sql-reference/sql-functions/other-functions/has-schema-privillege
- 9: https://dba.stackexchange.com/questions/338328/grant-permission-for-a-user-to-create-objects-in-a-postgresql-schema-is-not-work
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/postgres/postgres_integration_test.go"
echo "== locate test function =="
rg -n "LeastPrivilege|NoRuntimeDDL|TestIntegration_PostgresLeastPrivilegeRole" "$FILE" || true
echo
echo "== show lines 280-380 =="
nl -ba "$FILE" | sed -n '280,380p'Repository: LerianStudio/lib-systemplane
Length of output: 344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/postgres/postgres_integration_test.go"
echo "== locate test function =="
rg -n "LeastPrivilege|NoRuntimeDDL|TestIntegration_PostgresLeastPrivilegeRole" "$FILE" || true
echo
echo "== show lines 280-380 =="
nl -ba "$FILE" | sed -n '280,380p'Repository: LerianStudio/lib-systemplane
Length of output: 344
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/postgres/postgres_integration_test.go"
echo "== show lines 260-380 =="
awk 'NR>=260 && NR<=380 {printf "%d:%s\n", NR, $0}' "$FILE"
echo "== show any schema/role setup near this test =="
awk 'NR>=250 && NR<=430 {print}' "$FILE" | rg -n "roleName|rolePass|CREATE ROLE|REVOKE|GRANT|systemplane_entries|SchemaSQL|Start|New|Subscribe|Runtime|DDL" || trueRepository: LerianStudio/lib-systemplane
Length of output: 5644
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="internal/postgres/postgres_integration_test.go"
rg -n "func startContainer|startContainer\\(" -S "$FILE"
rg -n "func freshDB|freshDB\\(" -S "$FILE"
rg -n "func provisionSchema|provisionSchema\\(" -S "$FILE"
# Print the helper definitions (small sections around each match)
for line in $(rg -n "func startContainer|func freshDB|func provisionSchema" "$FILE" | cut -d: -f1); do
start=$((line-20))
end=$((line+120))
echo "== section around line $line =="
awk -v s=$start -v e=$end 'NR>=s && NR<=e {print}' "$FILE"
doneRepository: LerianStudio/lib-systemplane
Length of output: 11528
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate SchemaSQL implementation and inspect its SQL output
rg -n "func \\w*SchemaSQL\\(|SchemaSQL\\(" -S systemplane internal || true
# Find definition (best effort)
defs=$(rg -n "SchemaSQL\\(" -S --glob='*.go' . | cut -d: -f1 | head -n 20)
echo "$defs"
# Print around the first few matches to locate the actual function
i=0
for f in $defs; do
i=$((i+1))
echo "== $i: $f =="
awk 'NR>=1 && NR<=200 {print}' "$f" | head -n 40
# show only function body area if present
awk 'BEGIN{p=0} /func .*SchemaSQL/ {p=1} p && NR<200{print} NR>400{exit}' "$f" || true
if [ $i -ge 3 ]; then break; fi
done
# Also search for explicit GRANT/REVOKE/ALTER DEFAULT PRIVILEGES in SchemaSQL SQL string literals
schema_files=$(rg -n "SchemaSQL\\(" -S --glob='*.go' . | cut -d: -f1 | sort -u)
echo "== possible files with SchemaSQL matches =="
echo "$schema_files"
for f in $(echo "$schema_files" | head -n 10); do
echo "== scanning $f for GRANT/REVOKE on schema public =="
rg -n "GRANT|REVOKE|ALTER DEFAULT PRIVILEGES|CREATE ON SCHEMA|USAGE ON SCHEMA" "$f" || true
doneRepository: LerianStudio/lib-systemplane
Length of output: 2588
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Find SchemaSQL() definition =="
rg -n "func \\w*SchemaSQL\\(" . --glob='*.go'
echo
echo "== Show likely implementation files (top candidates) =="
for f in ddl.go internal/postgres/postgres.go internal/manager/schema.go internal/manager/testhelpers_integration_test.go; do
if [ -f "$f" ]; then
echo "---- $f (first 200 lines) ----"
sed -n '1,220p' "$f"
fi
done
echo
echo "== Show around SchemaSQL() in ddl.go if present =="
if [ -f ddl.go ]; then
rg -n "SchemaSQL\\(" ddl.go
# print region around the match(s)
for line in $(rg -n "SchemaSQL\\(" ddl.go | head -n 5 | cut -d: -f1); do
start=$((line-60)); end=$((line+160))
echo "---- ddl.go:$line ----"
sed -n "${start},${end}p" ddl.go
done
fi
echo
echo "== Grep SQL literals for CREATE/GRANT/REVOKE on schema public anywhere in Go code =="
rg -n "(GRANT|REVOKE|ALTER DEFAULT PRIVILEGES|CREATE ON SCHEMA|USAGE ON SCHEMA|CREATE SCHEMA)" --glob='*.go' .Repository: LerianStudio/lib-systemplane
Length of output: 16237
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect ddl/schema.sql (first ~200 lines) =="
sed -n '1,200p' ddl/schema.sql
echo
echo "== Search for GRANT/REVOKE/ALTER DEFAULT PRIVILEGES/CREATE ON SCHEMA in ddl/schema.sql =="
rg -n "(GRANT|REVOKE|ALTER DEFAULT PRIVILEGES|CREATE ON SCHEMA|USAGE ON SCHEMA|CREATE SCHEMA)" ddl/schema.sql || true
echo
echo "== Inspect ddl.go (SchemaSQL helper only) =="
sed -n '1,120p' ddl.goRepository: LerianStudio/lib-systemplane
Length of output: 3840
Harden the least-privilege assertion for public.CREATE
The test only revokes CREATE from the new role; if public.CREATE is granted to PUBLIC (or another group) in that Postgres environment, the role can still effectively have CREATE, weakening the regression signal. Add an explicit has_schema_privilege check (or revoke from PUBLIC) before starting the store.
Suggested hardening
stmts := []string{
- fmt.Sprintf(`REVOKE CREATE ON SCHEMA public FROM %s`, roleName),
+ `REVOKE CREATE ON SCHEMA public FROM PUBLIC`,
fmt.Sprintf(`GRANT USAGE ON SCHEMA public TO %s`, roleName),
fmt.Sprintf(`GRANT SELECT, INSERT, UPDATE, DELETE ON systemplane_entries TO %s`, roleName),
}
for _, stmt := range stmts {
if _, err := owner.Exec(stmt); err != nil {
t.Fatalf("grant stmt %q: %v", stmt, err)
}
}
+
+ var hasCreate bool
+ if err := owner.QueryRow(
+ `SELECT has_schema_privilege($1, 'public', 'CREATE')`,
+ roleName,
+ ).Scan(&hasCreate); err != nil {
+ t.Fatalf("check schema privilege: %v", err)
+ }
+ if hasCreate {
+ t.Fatal("least-privilege role still has CREATE on schema public")
+ }🧰 Tools
🪛 OpenGrep (1.22.0)
[ERROR] 329-329: SQL query built via fmt.Sprintf or string concatenation passed to a database method. Use parameterized queries with placeholder arguments.
(coderabbit.sql-injection.go-query-format)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/postgres/postgres_integration_test.go` around lines 323 - 342, The
test revokes CREATE from the new role but doesn't prevent CREATE being granted
via PUBLIC or another group; add a hardening check or revoke to ensure the role
cannot create in public: after creating roleName (and before starting the store)
execute a query using owner.Exec/owner.Query to check SELECT
has_schema_privilege('<roleName>', 'public', 'CREATE') and fail the test if it
returns true, or explicitly run owner.Exec to REVOKE CREATE ON SCHEMA public
FROM PUBLIC (and verify the revoke succeeded) so the created role truly lacks
CREATE on schema public; update the stmts/validation accordingly to reference
roleName and owner.Exec.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CHANGELOG.md`:
- Around line 41-43: Update the recommended version string from "v1.7.0" /
"v1.7.0-beta.1" to a major bump "v2.0.0" (and corresponding next beta like
"v2.0.0-beta.1") in the CHANGELOG entry that currently references `v1.7.0` so
the release line reflects a breaking runtime change and communicates a major
version upgrade.
- Around line 20-21: Edit the CHANGELOG entry text that reads "current consumers
depend on the removed runtime bootstrap, so this is a behavior change with no
expected real-world breakage." and replace the speculative assertion with a
factual, evidence-backed statement (e.g., describe what changed and any
confirmed compatibility notes) — keep only verifiable details about the change
and remove any language about adoption impact or expectations; ensure the
revised line stays in the same changelog section and preserves tense/formatting
consistent with surrounding entries.
In `@README.md`:
- Line 29: Update the README sentence to scope the multi-tenant read-path
statement to non-Manager deployments: change the absolute claim that "In
multi-tenant mode the library does NOT hold an in-process cache. Every `Get`
reads through the resolved tenant database." to explicitly state this applies
when there is no bound Manager (e.g., "without a bound Manager"). Reference the
existing middleware and helper symbols (`middleware.TenantMiddleware`, `WithPG`,
`WithMB`, `WithModule`, `tmcore.GetPGContext`, `tmcore.GetMBContext`) and ensure
the wording clarifies that Manager-backed MT deployments may behave differently
(i.e., may provide caching or a different read-path).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 103ab52e-d415-4b96-b130-f8330d1fbad9
📒 Files selected for processing (17)
CHANGELOG.mdCLAUDE.mdREADME.mdexamples/manager/main.gointernal/manager/connector.gointernal/manager/lifecycle.gointernal/manager/listen_integration_test.gointernal/manager/manager_integration_test.gointernal/manager/schema.gointernal/manager/schema_integration_test.gointernal/manager/schema_test.gointernal/manager/testhelpers_integration_test.gointernal/manager/warmload.gointernal/manager/warmload_test.gointernal/postgres/postgres.gointernal/postgres/postgres_integration_test.gointernal/postgres/postgres_schema.go
💤 Files with no reviewable changes (3)
- internal/manager/schema_integration_test.go
- internal/postgres/postgres_schema.go
- internal/manager/schema_test.go
Comment on lines
+20
to
+21
| current consumers depend on the removed runtime bootstrap, so this is a | ||
| behavior change with no expected real-world breakage. |
There was a problem hiding this comment.
Remove speculative consumer-impact wording.
Lines 20-21 assert adoption impact that is not verifiable from code/changelog evidence. Prefer factual release-note language only.
Based on learnings: "Keep docs factual and code-backed; avoid speculative roadmap text."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 20 - 21, Edit the CHANGELOG entry text that reads
"current consumers depend on the removed runtime bootstrap, so this is a
behavior change with no expected real-world breakage." and replace the
speculative assertion with a factual, evidence-backed statement (e.g., describe
what changed and any confirmed compatibility notes) — keep only verifiable
details about the change and remove any language about adoption impact or
expectations; ensure the revised line stays in the same changelog section and
preserves tense/formatting consistent with surrounding entries.
Comment on lines
+41
to
+43
| > Recommended version: **v1.7.0** (next beta `v1.7.0-beta.1`) — minor bump | ||
| > continuing the v1.6.x line that introduced `SchemaSQL()` / `DefaultSeedSQL()`. | ||
| > Tag owned by the maintainer; not tagged here. |
There was a problem hiding this comment.
Update the recommended version to a major bump.
Line 41 recommends a minor release (v1.7.0), but this section documents a breaking runtime behavior change. This should be aligned to v2.0.0 to avoid signaling a non-breaking upgrade path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CHANGELOG.md` around lines 41 - 43, Update the recommended version string
from "v1.7.0" / "v1.7.0-beta.1" to a major bump "v2.0.0" (and corresponding next
beta like "v2.0.0-beta.1") in the CHANGELOG entry that currently references
`v1.7.0` so the release line reflects a breaking runtime change and communicates
a major version upgrade.
| | Multi-tenant | May be nil | Resolved per-call via tenant-manager ctx | Same | Disabled — `OnChange` returns `ErrNotSupportedInMultiTenant` | | ||
|
|
||
| In multi-tenant mode the library does NOT hold an in-process cache. Every `Get` reads through the resolved tenant database. The lib expects the caller to wire `lib-commons/v5/commons/tenant-manager/middleware.TenantMiddleware` with `WithPG(pgManager, "<module>")` (Postgres) or `WithMB(mongoManager, "<module>")` (MongoDB) where `<module>` matches the lib's `WithModule(...)` option (default `"systemplane"`). The middleware populates the request context; the lib calls `tmcore.GetPGContext` / `tmcore.GetMBContext` to resolve the tenant database, lazily ensures the schema on first use per database, and runs the read/write against that handle. | ||
| In multi-tenant mode the library does NOT hold an in-process cache. Every `Get` reads through the resolved tenant database. The lib expects the caller to wire `lib-commons/v5/commons/tenant-manager/middleware.TenantMiddleware` with `WithPG(pgManager, "<module>")` (Postgres) or `WithMB(mongoManager, "<module>")` (MongoDB) where `<module>` matches the lib's `WithModule(...)` option (default `"systemplane"`). The middleware populates the request context; the lib calls `tmcore.GetPGContext` / `tmcore.GetMBContext` to resolve the tenant database and runs the read/write against that handle. |
There was a problem hiding this comment.
Scope the MT read-path statement to the non-Manager path.
Line 29 currently reads as absolute behavior for all MT usage. Please qualify it (e.g., “without a bound Manager”) so docs stay accurate for Manager-backed MT deployments.
Based on learnings: "Keep docs factual and code-backed; avoid speculative roadmap text."
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@README.md` at line 29, Update the README sentence to scope the multi-tenant
read-path statement to non-Manager deployments: change the absolute claim that
"In multi-tenant mode the library does NOT hold an in-process cache. Every `Get`
reads through the resolved tenant database." to explicitly state this applies
when there is no bound Manager (e.g., "without a bound Manager"). Reference the
existing middleware and helper symbols (`middleware.TenantMiddleware`, `WithPG`,
`WithMB`, `WithModule`, `tmcore.GetPGContext`, `tmcore.GetMBContext`) and ensure
the wording clarifies that Manager-backed MT deployments may behave differently
(i.e., may provide caching or a different read-path).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
lib-systemplaneno longer creates its schema or seeds defaults at runtime. The Postgres store and the multi-tenantManagerpreviously ranCREATE TABLE/CREATE FUNCTION/CREATE TRIGGERplus anINSERT ... ON CONFLICT DO NOTHINGdefaults seed on first use (Store.Start/OnTenantActivated). Those runtime DDL/seed paths are removed.Consumers now provision the schema externally (e.g. via their migration pipeline) using the already-published
SchemaSQL()/DefaultSeedSQL()(PR #26). The runtime database role needs only DML + LISTEN — noCREATEon the schema — which aligns with the least-privilege per-tenant roles the tenant-manager hands back. Runtime DDL previously failed against those roles withpermission denied for schema ... (42501).Non-breaking minor. No current consumers depend on the removed runtime auto-create, so this is a behavior change with no expected real-world breakage. Recommended next version: v1.7.0 (next beta
v1.7.0-beta.1), continuing the v1.6.x line that introducedSchemaSQL()/DefaultSeedSQL(). Not tagged here.What was removed
Postgres store (
internal/postgres/)runSchemaand theCREATE TABLE/FUNCTION/TRIGGERSQL builders —postgres_schema.godeleted.ensureSchema/schemaOnce/schemaErrlazy-bootstrap machinery and theStart()/resolveDB()calls into them.Startnow only opens LISTEN;resolveDBonly resolves the tenant handle.Manager (
internal/manager/)OnTenantActivatedno longer runs schema creation or the runtime seed — it warm-loads + opens LISTEN only.runSchemaAndSeed,runSchema, andseedDefaultsremoved.seed.go→warmload.go(warm-load only).defaultActorconstant removed (was seed-only).Graceful missing-table tolerance
warmLoad(run byOnTenantActivated) now detects the Postgres "undefined table" error (SQLSTATE42P01, possibly wrapped) viaerrors.As(*pgconn.PgError). On that error it logs at WARN and returnsnilwith an empty, non-stale cache instead of failing activation. The consumer's migration is expected to create the table; LISTEN/poll repopulates the cache once it exists. Every other error still propagates. Reads return not-found / zero-value as before.SchemaSQL()/DefaultSeedSQL()intactConfirmed unchanged and still the single source of truth (root
ddl_test.gopasses). They are now the ONLY way the schema/defaults are expressed.Scope note
This change targets the Postgres path explicitly named by the goal and the published Postgres DDL artifact. MongoDB collection/index bootstrap (
internal/mongodb) is intentionally left untouched — it is a separate concern not covered bySchemaSQL()and not implicated by the42501permission failure.Tests
SchemaSQL()in setup instead of relying on runtime auto-create.TestIntegration_PostgresLeastPrivilegeRole_NoRuntimeDDL: provisions schema as a privileged role, then runs the store under a DML-only role withCREATErevoked —Start/Set/Get/Deletesucceed, proving no runtime DDL is attempted.TestIntegration_Manager_OnTenantActivated_MissingTableTolerated: activation against an unprovisioned DB succeeds with an empty cache (no panic/error).TestIsUndefinedTable_DetectsSQLState42P01unit test for the 42P01 detection helper.go build ./...,go vet ./...,make lint,go test -tags unit ./...(209 pass), andgo test -tags integration ./internal/postgres ./internal/manager(39 pass) all green.Versioning
Recommended next version: v1.7.0 (next beta
v1.7.0-beta.1) — minor bump, non-breaking. Not tagged (owner tags the beta). The commit is a plainfeat(schema):with no!and noBREAKING CHANGE:footer, so semantic-release classifies it as a minor. CHANGELOG updated with a non-breakingChangedentry.🤖 Generated with Claude Code