Conversation
📝 WalkthroughWalkthroughThe changes expand the system to support PostgREST as a new service type alongside MCP. Updates include design documentation, API enum expansion, configuration validation rules, and orchestrator logic for service provisioning including credential handling, database environment variables, and role management. Changes
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/development/Postgrest-design-docs/postgrest-design-doc.md (1)
92-92: Clarify version references with upstream links to maintain accuracy as releases update.The v14.5 claim is accurate (released Feb 2026), but rephrasing as "As of February 2026, v14.5 is the latest stable release" plus a link to the PostgREST releases page ensures readers can re-verify and the doc won't mislead future maintainers if versions drift. For Postgres constraints, note that PostgREST v14 minimum is ≥13, but PostgreSQL 13 itself is now EOL (officially supported: 14–18 as of March 2026).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/development/Postgrest-design-docs/postgrest-design-doc.md` at line 92, Update the "PostgREST version — v14.5" sentence to read "As of February 2026, v14.5 is the latest stable release" and add a link to the PostgREST releases page (https://github.com/PostgREST/postgrest/releases) for verification; in the same table entry referencing "PostgREST version — v14.5" and the Postgres constraint text, explicitly state that PostgREST v14's minimum supported Postgres is >=13 but note that PostgreSQL 13 is EOL and recommend enforcing the project's chosen constraint (either >=13 to match PostgREST minimum or >=15 to match pgEdge preference), then register v14.5 with the agreed constraint and add a note to update the version as new releases ship.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/development/Postgrest-design-docs/postgrest-control-plane-changes.md`:
- Around line 3-5: The doc currently uses the phrase "Code to write" without a
stale-snippet guardrail; add a short banner immediately under the top intro that
states something like "Illustrative/pseudo-code — verify against current file
signatures/types before applying" and explicitly warns readers not to copy
snippets verbatim, and apply the same banner/warning to the sections referenced
as 13-16; ensure the banner is prominent (bold or boxed) and placed near the
existing "Code to write" wording so readers see the caution before copying
examples.
In `@docs/development/Postgrest-design-docs/postgrest-service-qa.md`:
- Line 13: Change the absolute statement "**Zero network hops to Postgres.**
PostgREST connects over the Docker overlay network to the Postgres instance on
the same host. Latency is sub-millisecond. Deploying it remotely adds a network
round-trip to every request." to a conditional phrasing that reflects typical
behavior (e.g., "Typically zero network hops when co-located" or "When
co-located, PostgREST connects..."), and add a note that service placement may
fall back to a different Postgres host so remote deployments incur an extra
network round-trip; update the sentence and following lines to use
"typically"/"when co-located" instead of guaranteed language.
---
Nitpick comments:
In `@docs/development/Postgrest-design-docs/postgrest-design-doc.md`:
- Line 92: Update the "PostgREST version — v14.5" sentence to read "As of
February 2026, v14.5 is the latest stable release" and add a link to the
PostgREST releases page (https://github.com/PostgREST/postgrest/releases) for
verification; in the same table entry referencing "PostgREST version — v14.5"
and the Postgres constraint text, explicitly state that PostgREST v14's minimum
supported Postgres is >=13 but note that PostgreSQL 13 is EOL and recommend
enforcing the project's chosen constraint (either >=13 to match PostgREST
minimum or >=15 to match pgEdge preference), then register v14.5 with the agreed
constraint and add a note to update the version as new releases ship.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6ea0f91f-f7d0-468d-8a77-b74c13119371
📒 Files selected for processing (4)
docs/development/Postgrest-design-docs/Detailed-design.mddocs/development/Postgrest-design-docs/postgrest-control-plane-changes.mddocs/development/Postgrest-design-docs/postgrest-design-doc.mddocs/development/Postgrest-design-docs/postgrest-service-qa.md
| Every file that needs changing, what to change, and the code to write. This targets | ||
| the **post-PR-#280 codebase** (MCP YAML config merged). Pre-PR-280 alternatives are | ||
| noted where they differ. |
There was a problem hiding this comment.
“Code to write” wording is risky without an explicit stale-snippet guardrail.
Because this doc mixes planned post-merge state with current-main caveats, readers may still copy snippets that don’t match current signatures/types. Add a short banner like “illustrative/pseudo-code; verify against current file signatures before applying” near the top to prevent implementation mistakes.
Also applies to: 13-16
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/development/Postgrest-design-docs/postgrest-control-plane-changes.md`
around lines 3 - 5, The doc currently uses the phrase "Code to write" without a
stale-snippet guardrail; add a short banner immediately under the top intro that
states something like "Illustrative/pseudo-code — verify against current file
signatures/types before applying" and explicitly warns readers not to copy
snippets verbatim, and apply the same banner/warning to the sections referenced
as 13-16; ensure the banner is prominent (bold or boxed) and placed near the
existing "Code to write" wording so readers see the caution before copying
examples.
|
|
||
| **Why it lives next to the database:** | ||
|
|
||
| - **Zero network hops to Postgres.** PostgREST connects over the Docker overlay network to the Postgres instance on the same host. Latency is sub-millisecond. Deploying it remotely adds a network round-trip to every request. |
There was a problem hiding this comment.
“Zero network hops” is too strong and can be inaccurate.
Service placement prefers co-location but can fall back to another Postgres host, so this should be phrased as “typically”/“when co-located” rather than guaranteed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/development/Postgrest-design-docs/postgrest-service-qa.md` at line 13,
Change the absolute statement "**Zero network hops to Postgres.** PostgREST
connects over the Docker overlay network to the Postgres instance on the same
host. Latency is sub-millisecond. Deploying it remotely adds a network
round-trip to every request." to a conditional phrasing that reflects typical
behavior (e.g., "Typically zero network hops when co-located" or "When
co-located, PostgREST connects..."), and add a note that service placement may
fall back to a different Postgres host so remote deployments incur an extra
network round-trip; update the sentence and following lines to use
"typically"/"when co-located" instead of guaranteed language.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
docs/development/Postgrest-design-docs/postgrest-service-qa.md (1)
13-13:⚠️ Potential issue | 🟡 MinorUse conditional phrasing for network-hop claim.
Line 13 still states this as guaranteed behavior; placement fallback can put PostgREST on a different host, so this should be “typically/when co-located,” not absolute.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/development/Postgrest-design-docs/postgrest-service-qa.md` at line 13, The sentence stating "Zero network hops to Postgres" is too absolute; update the phrasing around that bullet (the PostgREST network hops claim) to be conditional — e.g. "Typically zero network hops when co-located with Postgres" — and add a short clause noting that remote placement or different-host deployment will introduce a network round-trip and higher latency; keep the rest of the bullet about Docker overlay and latency but make the co-location caveat explicit.docs/development/Postgrest-design-docs/postgrest-control-plane-changes.md (1)
3-5:⚠️ Potential issue | 🟡 MinorAdd a clear “illustrative snippet” warning near the top.
Please add an explicit banner saying snippets are pseudo-code and must be validated against current signatures/types before applying, to reduce copy/paste implementation errors.
Also applies to: 13-16
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/development/Postgrest-design-docs/postgrest-control-plane-changes.md` around lines 3 - 5, Add a prominent “Illustrative snippet” warning banner at the top of the document (near the existing opening paragraph that begins "Every file that needs changing...") stating that code snippets are pseudo-code, must be validated against current signatures and types, and should not be copy/pasted as-is; place the banner before any example snippets and replicate the same banner around sections 13–16 to cover those examples as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/development/Postgrest-design-docs/postgrest-design-doc.md`:
- Line 61: Update the documentation path to reference the actual implementation
location: change `api/apiv1/validate.go` to
`server/internal/api/apiv1/validate.go` wherever it appears (including the
allowlist note and the line describing `validatePostgRESTServiceConfig()` and
its wiring via the `switch` dispatch) so the doc correctly points to the file
that defines `validatePostgRESTServiceConfig()` and the switch-based dispatch
logic.
- Around line 99-104: Update the health-check examples to call the PostgREST
admin server (configured via admin-server-port / PGRST_ADMIN_SERVER_PORT) rather
than the API port; replace the example curl to use the admin port (not 3100
which is the API port used by the other examples) and use the admin endpoints
/live or /ready (e.g., curl http://localhost:<admin-port>/live and curl
http://localhost:<admin-port>/ready), and add a short note that the admin server
must run on a different port than the main API server.
In `@docs/development/Postgrest-design-docs/postgrest-service-qa.md`:
- Around line 114-116: The table currently lists a non-existent `/health`
endpoint and mislabels admin endpoints; remove `/health` from the API endpoints
table and update the admin-server descriptions (those referencing
PGRST_ADMIN_SERVER_PORT) to reflect PostgREST v14's actual admin endpoints:
replace any `/health` entry with `/live` and update the `/live` description to
state it is the liveness check (200 if PostgREST is alive), and ensure `/ready`
is documented as 200 when alive and schema cache is loaded; ensure all mentions
reference the admin port (PGRST_ADMIN_SERVER_PORT) and remove any suggestion
that `/live` is an alias for `/health`.
---
Duplicate comments:
In `@docs/development/Postgrest-design-docs/postgrest-control-plane-changes.md`:
- Around line 3-5: Add a prominent “Illustrative snippet” warning banner at the
top of the document (near the existing opening paragraph that begins "Every file
that needs changing...") stating that code snippets are pseudo-code, must be
validated against current signatures and types, and should not be copy/pasted
as-is; place the banner before any example snippets and replicate the same
banner around sections 13–16 to cover those examples as well.
In `@docs/development/Postgrest-design-docs/postgrest-service-qa.md`:
- Line 13: The sentence stating "Zero network hops to Postgres" is too absolute;
update the phrasing around that bullet (the PostgREST network hops claim) to be
conditional — e.g. "Typically zero network hops when co-located with Postgres" —
and add a short clause noting that remote placement or different-host deployment
will introduce a network round-trip and higher latency; keep the rest of the
bullet about Docker overlay and latency but make the co-location caveat
explicit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 45944d26-0356-495f-8856-94a3f2591278
📒 Files selected for processing (3)
docs/development/Postgrest-design-docs/postgrest-control-plane-changes.mddocs/development/Postgrest-design-docs/postgrest-design-doc.mddocs/development/Postgrest-design-docs/postgrest-service-qa.md
| | File | Change | | ||
| |------|--------| | ||
| | `api/apiv1/design/database.go` | Add `"postgrest"` to `service_type` enum. Run `make -C api generate`. | | ||
| | `api/apiv1/validate.go` | Add to allowlist. Add `validatePostgRESTServiceConfig()` (required field + range checks). Wire dispatch via `switch`. | |
There was a problem hiding this comment.
Fix the file path for validation code.
Line 61 points to api/apiv1/validate.go, but the implementation path described elsewhere is server/internal/api/apiv1/validate.go.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/development/Postgrest-design-docs/postgrest-design-doc.md` at line 61,
Update the documentation path to reference the actual implementation location:
change `api/apiv1/validate.go` to `server/internal/api/apiv1/validate.go`
wherever it appears (including the allowlist note and the line describing
`validatePostgRESTServiceConfig()` and its wiring via the `switch` dispatch) so
the doc correctly points to the file that defines
`validatePostgRESTServiceConfig()` and the switch-based dispatch logic.
| docker service ls --filter name=postgrest | ||
| curl http://localhost:3100/health # 200 | ||
| curl http://localhost:3100/ -H "Accept-Profile: api" # OpenAPI spec | ||
| curl "http://localhost:3100/gold_summaries?limit=1" -H "Accept-Profile: api" # JSON rows | ||
| curl -X DELETE "http://localhost:3100/gold_summaries?id=eq.1" -H "Content-Profile: api" # 405 (blocked) | ||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PostgREST health endpoint port configuration API admin server
💡 Result:
PostgREST exposes its health endpoints on the “admin server”, not on the main API port.
- Enable the admin server by setting
admin-server-port(config file) orPGRST_ADMIN_SERVER_PORT(env var). (docs.postgrest.org) - The admin server can also bind a different host via
admin-server-host/PGRST_ADMIN_SERVER_HOST. (postgrest.org) - The admin server port cannot be the same as the main API
server-port. (postgrest.org)
Once enabled, health checks are:
GET http://<admin-host>:<admin-port>/live(process is up) (docs.postgrest.org)GET http://<admin-host>:<admin-port>/ready(also checks DB connection pool + schema cache; returns503if not ready) (docs.postgrest.org)
Example (admin server on 3001):
curl -I http://localhost:3001/live
curl -I http://localhost:3001/readyCitations:
- 1: https://docs.postgrest.org/en/v12/references/admin_server.html
- 2: https://postgrest.org/en/stable/configuration.html
- 3: https://postgrest.org/en/stable/configuration.html
- 4: https://docs.postgrest.org/en/v12/references/admin_server.html
- 5: https://docs.postgrest.org/en/v12/references/admin_server.html
- 6: https://docs.postgrest.org/en/v12/references/admin_server.html
🏁 Script executed:
# Check the documentation file to understand the context
cat -n docs/development/Postgrest-design-docs/postgrest-design-doc.md | sed -n '90,110p'Repository: pgEdge/control-plane
Length of output: 1562
🏁 Script executed:
# Search for PostgREST version references in the documentation
grep -i "version\|postgrest" docs/development/Postgrest-design-docs/postgrest-design-doc.md | head -20Repository: pgEdge/control-plane
Length of output: 2467
Correct health endpoint location: use admin server port, not API port.
PostgREST health endpoints are served on the admin server (separate port), not the API port. Line 100 shows curl http://localhost:3100/health, but:
- Port 3100 is the main API port (see subsequent curl commands on lines 101–103)
- Health checks require the admin server port (configured via
admin-server-port/PGRST_ADMIN_SERVER_PORT) - The endpoint path is
/liveor/ready, not/health
Correct examples:
curl http://localhost:3001/live # admin server on 3001
curl http://localhost:3001/ready # includes DB connection + schema cache checkAdmin server must be configured with a different port than the main API server.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/development/Postgrest-design-docs/postgrest-design-doc.md` around lines
99 - 104, Update the health-check examples to call the PostgREST admin server
(configured via admin-server-port / PGRST_ADMIN_SERVER_PORT) rather than the API
port; replace the example curl to use the admin port (not 3100 which is the API
port used by the other examples) and use the admin endpoints /live or /ready
(e.g., curl http://localhost:<admin-port>/live and curl
http://localhost:<admin-port>/ready), and add a short note that the admin server
must run on a different port than the main API server.
| | Health check | `GET /health` | Returns 200 if connected to Postgres | | ||
| | Readiness | `GET /ready` | Returns 200 if schema cache is loaded | | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n docs/development/Postgrest-design-docs/postgrest-service-qa.md | sed -n '100,190p'Repository: pgEdge/control-plane
Length of output: 5267
🌐 Web query:
PostgREST v14 /health /ready /live endpoints admin port PGRST_ADMIN_SERVER_PORT
💡 Result:
In PostgREST v14, the built-in health checks live on the Admin Server, which you enable by setting:
admin-server-port/PGRST_ADMIN_SERVER_PORT(must be different fromserver-port) [2]- optionally
admin-server-host/PGRST_ADMIN_SERVER_HOST[2]
Once enabled, the Admin Server exposes these health endpoints (empty body, status code only):
GET /live→ 200 if the PostgREST process is alive, 500 otherwise [1]GET /ready→ 200 if alive and connection pool + schema cache are OK, 503 otherwise [1]
There is no built-in /health endpoint in PostgREST v14 docs; if you need /health, typically you map it to /live or /ready in your reverse proxy / ingress. [1]
Also: postgrest --ready performs a request to the Admin Server’s /ready endpoint and exits 0/1 accordingly. [3]
Sources: Admin Server (v14) [1], Configuration/env vars (stable) [2], CLI --ready (v14) [3].
Health and readiness endpoint descriptions are inconsistent and contain incorrect endpoint names.
Lines 114–116 list /health and /ready in a general API endpoints table without mentioning the admin port. Lines 174–179 correctly state these are admin endpoints on port 3001 (PGRST_ADMIN_SERVER_PORT), but the documentation is factually incorrect: PostgREST v14 does not expose a /health endpoint. Only /live and /ready are available on the admin server.
/live— 200 if PostgREST is alive/ready— 200 if alive and schema cache is loaded
Remove /health from lines 114–116 (or add a note that it is not a real PostgREST endpoint). Update lines 174–179 to remove the /health description and correct the /live description; /live is not an alias for /health, it is the primary liveness check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/development/Postgrest-design-docs/postgrest-service-qa.md` around lines
114 - 116, The table currently lists a non-existent `/health` endpoint and
mislabels admin endpoints; remove `/health` from the API endpoints table and
update the admin-server descriptions (those referencing PGRST_ADMIN_SERVER_PORT)
to reflect PostgREST v14's actual admin endpoints: replace any `/health` entry
with `/live` and update the `/live` description to state it is the liveness
check (200 if PostgREST is alive), and ensure `/ready` is documented as 200 when
alive and schema cache is loaded; ensure all mentions reference the admin port
(PGRST_ADMIN_SERVER_PORT) and remove any suggestion that `/live` is an alias for
`/health`.
moizpgedge
force-pushed
the
Docs/PLAT-458/PostgRest-Design-docs
branch
2 times, most recently
from
79e9d88 to
7260a26
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/internal/orchestrator/swarm/service_user_role.go (1)
63-69:⚠️ Potential issue | 🟠 Major
anon_roledrift is masked, so role-grant updates won’t reconcile.Line 68 ignores
/anon_role, andUpdateis a no-op; ifdb_anon_rolechanges, the new grant is never applied. This creates credential/config drift for PostgREST service updates.🔧 Minimal correction path
func (r *ServiceUserRole) DiffIgnore() []string { return []string{ "/postgres_host_id", "/username", "/password", - "/anon_role", } }Then reconcile
GRANT <anon_role> TO <service_user>idempotently inUpdate(or enforce immutability during validation).Based on learnings: implement idempotent provisioning semantics so retries/updates are safe and non-destructive.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/orchestrator/swarm/service_user_role.go` around lines 63 - 69, The DiffIgnore currently hides "/anon_role" in ServiceUserRole which masks changes to db_anon_role so Update becomes a no-op; remove "/anon_role" from DiffIgnore or stop ignoring anon_role and implement idempotent reconcile logic in ServiceUserRole.Update to apply "GRANT <anon_role> TO <service_user>" when db_anon_role differs (or validate immutability). Specifically, in the ServiceUserRole.Update method detect the desired anon_role vs current grants and run an idempotent GRANT (and optional REVOKE if required) so that retries/updates safely reconcile the anon_role change for the service user.
🧹 Nitpick comments (2)
server/internal/api/apiv1/validate.go (2)
345-346: Minor: Inconsistent error message format for required fields.The error format here (
"%s is required") differs from the MCP validator's format ("missing required field '%s'"at line 295). Consider aligning for consistency across service types.♻️ Suggested fix for consistency
- err := fmt.Errorf("%s is required", field) + err := fmt.Errorf("missing required field '%s'", field)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/api/apiv1/validate.go` around lines 345 - 346, Change the inconsistent required-field error message created where err := fmt.Errorf("%s is required") and appended via newValidationError(err, path) to match the MCP validator style; update the fmt.Errorf format to "missing required field '%s'" (using the same field variable) so newValidationError receives the consistent message across validators.
362-372: Consider validating Postgres identifier format fordb_anon_role.The current validation only checks for non-empty strings, but Postgres role names have specific restrictions (must start with a letter or underscore, limited character set). Invalid role names will fail at PostgREST runtime. If earlier feedback is desired, consider adding a regex check for valid Postgres identifiers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@server/internal/api/apiv1/validate.go` around lines 362 - 372, The current check in validate.go for config["db_anon_role"] only ensures a non-empty string but should enforce Postgres identifier rules; update the validation inside the db_anon_role block (the code that currently assigns s and appends newValidationError) to also test s against a Postgres identifier regex (e.g. start with a letter or underscore, then letters, digits, or underscores, and reasonable length) and, on failure, append a newValidationError with a clear message and the same path (appendPath(path, mapKeyPath("db_anon_role"))); keep the existing non-empty check but perform the regex check immediately after trimming and before accepting the value so invalid role names are reported early.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/development/Postgrest-design-docs/postgrest-service-qa.md`:
- Line 66: Add language identifiers to the Markdown fenced code blocks that
currently use only ``` (the triple-backtick fences) so they satisfy markdownlint
MD040; update the three affected code blocks referenced in the review by
changing the opening fences from ``` to e.g. ```text, ```http, or ```sql as
appropriate for the block contents (ensure all three affected fenced blocks are
updated).
In `@server/internal/orchestrator/swarm/service_images.go`:
- Around line 67-70: Replace the hardcoded image string in the PostgREST service
entry with the repository-rewriting helper: in the versions.addServiceImage call
that defines the ServiceImage for "postgrest" use Tag:
serviceImageTag("postgrest", "latest") instead of "postgrest/postgrest:latest"
so cfg.DockerSwarm.ImageRepositoryHost is honored; ensure the call uses the
exact service name and tag parameters expected by serviceImageTag.
---
Outside diff comments:
In `@server/internal/orchestrator/swarm/service_user_role.go`:
- Around line 63-69: The DiffIgnore currently hides "/anon_role" in
ServiceUserRole which masks changes to db_anon_role so Update becomes a no-op;
remove "/anon_role" from DiffIgnore or stop ignoring anon_role and implement
idempotent reconcile logic in ServiceUserRole.Update to apply "GRANT <anon_role>
TO <service_user>" when db_anon_role differs (or validate immutability).
Specifically, in the ServiceUserRole.Update method detect the desired anon_role
vs current grants and run an idempotent GRANT (and optional REVOKE if required)
so that retries/updates safely reconcile the anon_role change for the service
user.
---
Nitpick comments:
In `@server/internal/api/apiv1/validate.go`:
- Around line 345-346: Change the inconsistent required-field error message
created where err := fmt.Errorf("%s is required") and appended via
newValidationError(err, path) to match the MCP validator style; update the
fmt.Errorf format to "missing required field '%s'" (using the same field
variable) so newValidationError receives the consistent message across
validators.
- Around line 362-372: The current check in validate.go for
config["db_anon_role"] only ensures a non-empty string but should enforce
Postgres identifier rules; update the validation inside the db_anon_role block
(the code that currently assigns s and appends newValidationError) to also test
s against a Postgres identifier regex (e.g. start with a letter or underscore,
then letters, digits, or underscores, and reasonable length) and, on failure,
append a newValidationError with a clear message and the same path
(appendPath(path, mapKeyPath("db_anon_role"))); keep the existing non-empty
check but perform the regex check immediately after trimming and before
accepting the value so invalid role names are reported early.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2ccc0671-a47c-4010-99bb-92f7fad7c086
⛔ Files ignored due to path filters (6)
api/apiv1/gen/http/control_plane/client/types.gois excluded by!**/gen/**api/apiv1/gen/http/control_plane/server/types.gois excluded by!**/gen/**api/apiv1/gen/http/openapi.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi.yamlis excluded by!**/gen/**api/apiv1/gen/http/openapi3.jsonis excluded by!**/gen/**api/apiv1/gen/http/openapi3.yamlis excluded by!**/gen/**
📒 Files selected for processing (12)
api/apiv1/design/database.goapi/goa3441651573/goaapi/goa3441651573/main.godocs/development/Postgrest-design-docs/postgrest-service-qa.mdprimaryreplicaserver/internal/api/apiv1/validate.goserver/internal/orchestrator/swarm/orchestrator.goserver/internal/orchestrator/swarm/service_images.goserver/internal/orchestrator/swarm/service_instance_spec.goserver/internal/orchestrator/swarm/service_spec.goserver/internal/orchestrator/swarm/service_user_role.go
|
|
||
| ### Validation Rules | ||
|
|
||
| ``` |
There was a problem hiding this comment.
Add fenced-code language identifiers (markdownlint MD040).
Lines 66, 129, and 135 should specify a language (for example text, http, or sql where appropriate) to satisfy linting.
Also applies to: 129-129, 135-135
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 66-66: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/development/Postgrest-design-docs/postgrest-service-qa.md` at line 66,
Add language identifiers to the Markdown fenced code blocks that currently use
only ``` (the triple-backtick fences) so they satisfy markdownlint MD040; update
the three affected code blocks referenced in the review by changing the opening
fences from ``` to e.g. ```text, ```http, or ```sql as appropriate for the block
contents (ensure all three affected fenced blocks are updated).
| { | ||
| "service_id": "postgrest", | ||
| "service_type": "postgrest", | ||
| "version": "14.5", |
There was a problem hiding this comment.
Version examples conflict with current implementation behavior.
Line 85 ("version": "14.5") and the Line 163-Line 169 table imply versioned PostgREST support, but this PR’s implementation currently registers postgrest with latest only. These examples will mislead users into invalid specs.
📝 Suggested doc correction
- "version": "14.5",
+ "version": "latest",-| `14.5` | `postgrest/postgrest:v14.5` | Postgres >= 15 (as defined in the implementation guide) |
-| `latest` | `postgrest/postgrest:latest` | None |
+| `latest` | `postgrest/postgrest:latest` | None |Also applies to: 163-169
| // PostgREST service versions | ||
| versions.addServiceImage("postgrest", "latest", &ServiceImage{ | ||
| Tag: "postgrest/postgrest:latest", | ||
| }) |
There was a problem hiding this comment.
Honor repository-host rewriting for PostgREST images.
Line 69 hardcodes postgrest/postgrest:latest, which bypasses serviceImageTag(...). This skips cfg.DockerSwarm.ImageRepositoryHost and can fail in private-registry environments.
🔧 Proposed fix
// PostgREST service versions
versions.addServiceImage("postgrest", "latest", &ServiceImage{
- Tag: "postgrest/postgrest:latest",
+ Tag: serviceImageTag(cfg, "postgrest/postgrest:latest"),
})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@server/internal/orchestrator/swarm/service_images.go` around lines 67 - 70,
Replace the hardcoded image string in the PostgREST service entry with the
repository-rewriting helper: in the versions.addServiceImage call that defines
the ServiceImage for "postgrest" use Tag: serviceImageTag("postgrest", "latest")
instead of "postgrest/postgrest:latest" so cfg.DockerSwarm.ImageRepositoryHost
is honored; ensure the call uses the exact service name and tag parameters
expected by serviceImageTag.
1 participant
Design documentation for integrating PostgREST as the second supported service type in the control plane. This PR contains design docs only , no code changes. It covers the complete implementation plan, gap analysis, and code change specifications needed to add PostgREST alongside MCP.