[devopam]

Summary

MCP tools for persistent GUC changes via ALTER SYSTEM (not an extension of run_ddl).

Tool	Gate	Behaviour
inspect_guc	READ	Live pg_settings metadata
alter_system	unrestricted + MCPG_ALLOW_ALTER_SYSTEM	SET/RESET one GUC on autocommit; requires confirm=true after a structured warning
alter_system_reset_all	same	ALTER SYSTEM RESET ALL
reload_config	same	pg_reload_conf()

Design

• Autocommit via Database.run_unmanaged (transactional run_ddl cannot run ALTER SYSTEM)
• First call without confirm=true → status=confirmation_required + warning for the agent to show the user
• Built-in denylist for connectivity/preload foot-guns; optional allow/deny lists
• DSN user must be superuser (or allowed to ALTER SYSTEM); no separate superuser env var

On this branch

• src/mcpg/guc.py — full module
• src/mcpg/guc_tools.py — MCP registration helpers
• tests/unit/test_guc.py
• docs/alter-system.md

Still required before ready-for-review

1. config.py: add allow_alter_system, alter_system_allowlist, alter_system_denylist + env MCPG_ALLOW_ALTER_SYSTEM / MCPG_ALTER_SYSTEM_ALLOWLIST / MCPG_ALTER_SYSTEM_DENYLIST
2. tools.py (3 sites):
   • from mcpg.config import AccessMode, Settings and from mcpg import guc_tools
   • after _register_advisors(server): guc_tools.register_guc_reads(server)
   • before DDL gate: if settings.access_mode == AccessMode.UNRESTRICTED and settings.allow_alter_system: guc_tools.register_guc_writes(server)
3. README env table + CHANGELOG Unreleased entry

Test plan

• pytest tests/unit/test_guc.py
• unrestricted + MCPG_ALLOW_ALTER_SYSTEM: dry_run → confirm → SET work_mem
• denylist rejects listen_addresses
• without confirm, no write occurs

Summary by Sourcery

Add guarded MCP tools for inspecting and persistently managing PostgreSQL configuration through ALTER SYSTEM.

New Features:

• Add read-only GUC inspection and guarded tools for persistent ALTER SYSTEM updates, resetting all overrides, and reloading configuration.

Enhancements:

• Require explicit confirmation tokens for configuration writes, validate values against live PostgreSQL metadata, and report reload or restart requirements.
• Protect connectivity, preload, command-execution, and credential-related settings with built-in and configurable allow/deny policies.
• Run ALTER SYSTEM operations through autocommit connections and block them when tenant-role configuration could bypass privilege separation.

Documentation:

• Document the ALTER SYSTEM tools, confirmation workflow, privilege requirements, environment variables, and safety restrictions.
• Update the README, architecture module map, tool index, and changelog for the new GUC capabilities.

Tests:

• Add unit coverage for GUC validation, redaction, policy enforcement, confirmation flows, reset-all behavior, reload handling, timeouts, and tool registration.

Chores:

• Register GUC tools in the capability and tool surfaces and add their Docker registry and contract metadata.

[sourcery-ai]

Hey - I've found 15 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="src/mcpg/guc.py" line_range="141" />
<code_context>
+
+
+def _quote_literal(value: str) -> str:
+    return "'" + value.replace("'", "''") + "'"
+
+
</code_context>
<issue_to_address>
**Injected SQL executes**

When the connection uses `standard_conforming_strings=off` and a value contains a backslash immediately before an apostrophe, `_quote_literal()` leaves the backslash unchanged, so PostgreSQL treats the apostrophe as escaped and the generated statement can execute injected SQL through `Database.run_unmanaged`; callers can cause arbitrary SQL effects under the database role.

Quote values so they remain safe when `standard_conforming_strings` is off, or use parameterized execution.

Also at `src/mcpg/guc.py:306`.
</issue_to_address>

### Comment 2
<location path="src/mcpg/guc.py" line_range="24" />
<code_context>
+from mcpg.sql import SqlDriver
+
+# Foot-guns: changing these via agents can brick connectivity or preload.
+_DEFAULT_DENYLIST: frozenset[str] = frozenset(
+    {
+        "listen_addresses",
</code_context>
<issue_to_address>
**Callers can execute OS commands**

When ALTER SYSTEM tools are enabled, a caller supplies `archive_command`, and PostgreSQL later archives WAL, `_check_policy()` allows `archive_command` unless an operator separately denies it or configures an allowlist, so PostgreSQL runs the supplied shell command as the server OS account.

Add `archive_command` to the built-in denylist.

Also at `src/mcpg/guc.py:45`.
</issue_to_address>

### Comment 3
<location path="src/mcpg/guc.py" line_range="345" />
<code_context>
+        )
+
+    try:
+        await database.run_unmanaged(planned)
+    except Exception as exc:
+        raise GucError(f"ALTER SYSTEM failed: {exc}") from exc
</code_context>
<issue_to_address>
**Restricted roles gain privileges**

When the pooled DSN login has more privileges than the configured default or request role, `alter_system` executes through `Database.run_unmanaged()` on the raw pool connection, bypassing tenant-aware role setup and running as the DSN login; restricted callers can perform configuration changes with that login's broader privileges.

Execute these operations through a path that applies the configured tenant or request role.

Also at `src/mcpg/guc.py:422`, `src/mcpg/guc.py:469`.
</issue_to_address>

### Comment 4
<location path="src/mcpg/guc.py" line_range="380-383" />
<code_context>
+    confirm: bool = False,
+) -> AlterSystemResult:
+    """``ALTER SYSTEM RESET ALL`` — clears every prior ALTER SYSTEM override."""
+    if reload not in ("none", "if_needed", "always"):
+        raise GucError(f"reload must be one of none, if_needed, always; got {reload!r}")
+
+    planned = "ALTER SYSTEM RESET ALL"
+    warning = _warning_text(action=planned, name=None, activation="reload")
+
</code_context>
<issue_to_address>
**Reset all bypasses policy**

When an allowlist or denylist is configured and a caller confirms RESET ALL, `alter_system_reset_all()` does not apply the configured policies, including the built-in denylist, so `ALTER SYSTEM RESET ALL` removes overrides outside the permitted names and callers can change protected settings.

Enforce the configured allowlist and denylist before allowing RESET ALL.

Also at `src/mcpg/guc.py:371-379`, `src/mcpg/guc.py:422`, `src/mcpg/guc_tools.py:121-129`.
</issue_to_address>

### Comment 5
<location path="src/mcpg/guc.py" line_range="328" />
<code_context>
+            detail="No changes applied (dry_run=true). Call again with dry_run=false and confirm=true to apply.",
+        )
+
+    if not confirm:
+        return AlterSystemResult(
+            status="confirmation_required",
</code_context>
<issue_to_address>
**Changes bypass warning approval**

When a caller submits `confirm=true` without a matching warning, or changes the action between the warning and confirmation calls, `alter_system` and `alter_system_reset_all` treat `confirm` as an unbound boolean, so the action executes without its warning having been returned for that action and approved.

Bind confirmation to the specific warned action and require that approval before executing it.

Also at `src/mcpg/guc.py:345`, `src/mcpg/guc.py:402`, `src/mcpg/guc.py:422`.
</issue_to_address>

### Comment 6
<location path="src/mcpg/guc.py" line_range="179" />
<code_context>
+
+async def _fetch_guc_row(driver: SqlDriver, name: str) -> dict[str, Any] | None:
+    rows = await driver.execute_query(
+        "SELECT name, setting, unit, category, short_desc, context, vartype, "
+        "source, pending_restart, boot_val, reset_val, min_val, max_val, enumvals "
+        "FROM pg_settings WHERE name = %s",
</code_context>
<issue_to_address>
**Inspection exposes secret settings**

When a secret is embedded in a GUC value and a read-only client can call `inspect_guc` for that setting, the `pg_settings` query returns the raw `setting` value and `inspect_guc` exposes it through the always-available READ tool. When a secret is embedded in a setting such as `archive_command` or `primary_conninfo`, any client permitted to call read tools can retrieve it.

Redact known secret-bearing GUC values or expose only safe metadata for them, and restrict raw-value inspection to an appropriately privileged surface.

Also at `src/mcpg/guc_tools.py:42`.
</issue_to_address>

### Comment 7
<location path="src/mcpg/guc.py" line_range="183" />
<code_context>
+        "source, pending_restart, boot_val, reset_val, min_val, max_val, enumvals "
+        "FROM pg_settings WHERE name = %s",
+        [name],
+        force_readonly=True,
+    )
+    if not rows:
</code_context>
<issue_to_address>
**Inspection reports replica state**

When replica routing is configured and a replica's GUC state differs from the primary, `_fetch_guc_row()` and pending-restart queries use `force_readonly=True`, so they can read a replica while `ALTER SYSTEM` and reload operate on the primary; inspection and post-write results report the replica's values and restart state as if they described the changed instance.

Read GUC metadata and pending-restart state from the primary.

Also at `src/mcpg/guc.py:184`, `src/mcpg/guc.py:351`, `src/mcpg/guc.py:443`, `src/mcpg/guc.py:477-478`.
</issue_to_address>

### Comment 8
<location path="src/mcpg/guc.py" line_range="265" />
<code_context>
+        return False, None, "immediate"
+
+    try:
+        await database.run_unmanaged("SELECT pg_reload_conf()")
+    except Exception:
+        return True, False, "reload_failed"
</code_context>
<issue_to_address>
**Failed reload reported successful**

When `pg_reload_conf()` returns false without raising an SQL exception, `Database.run_unmanaged()` discards the result of `SELECT pg_reload_conf()`, so `_maybe_reload()` treats any non-exceptional query as success and callers see `reload_succeeded=true` or `reloaded=true` although PostgreSQL did not signal the reload.

Read and check the boolean returned by `pg_reload_conf()` before reporting reload success.

Also at `src/mcpg/guc.py:432-437`, `src/mcpg/guc.py:469-471`.
</issue_to_address>

### Comment 9
<location path="src/mcpg/guc.py" line_range="256" />
<code_context>
+    needs_restart = _activation_for_context(context) == "restart_required"
+
+    if needs_restart:
+        return False, None, "restart_required"
+
+    if reload == "none":
</code_context>
<issue_to_address>
**Requested reload is skipped**

When a confirmed change targets a postmaster-context GUC with `reload="always"`, `_maybe_reload()` returns for restart-required contexts before checking the requested mode, so it does not attempt the unconditional reload and the result reports no reload attempt.

Honor `reload="always"` before returning for restart-required contexts.

Also at `src/mcpg/guc.py:351`.
</issue_to_address>

### Comment 10
<location path="src/mcpg/guc.py" line_range="345" />
<code_context>
+        )
+
+    try:
+        await database.run_unmanaged(planned)
+    except Exception as exc:
+        raise GucError(f"ALTER SYSTEM failed: {exc}") from exc
</code_context>
<issue_to_address>
**Unmanaged operations lack query timeouts**

When an ALTER SYSTEM or reload operation stalls, for example while the server or its configuration filesystem is unresponsive, these calls use `Database.run_unmanaged`, which executes directly on a pooled connection without applying the configured statement or lock timeouts. A stalled ALTER SYSTEM or reload can therefore leave the tool call waiting beyond the limits used by ordinary driver queries.

Apply the configured timeouts to unmanaged operations or add a bounded timeout to these GUC calls.

Also at `src/mcpg/guc.py:265`, `src/mcpg/guc.py:469`.
</issue_to_address>

### Comment 11
<location path="src/mcpg/guc.py" line_range="351" />
<code_context>
+
+    reload_attempted, reload_succeeded, activation = await _maybe_reload(database, reload=reload, context=info.context)
+
+    after = await inspect_guc(driver, name)
+    return AlterSystemResult(
+        status="applied",
</code_context>
<issue_to_address>
**Post-write read errors hide success**

When the write or reload succeeds but its follow-up inspection query fails, each path performs its side effect before a fallible status query. If that query fails, the tool raises instead of returning that the ALTER SYSTEM write or reload already happened, so callers see an error for a completed nontransactional operation.

Preserve the completed-operation status and return a partial-result diagnostic when follow-up inspection fails, rather than reporting the whole operation as failed.

Also at `src/mcpg/guc.py:442-443`, `src/mcpg/guc.py:476-478`.
</issue_to_address>

### Comment 12
<location path="src/mcpg/guc.py" line_range="422" />
<code_context>
+        )
+
+    try:
+        await database.run_unmanaged(planned)
+    except Exception as exc:
+        raise GucError(f"ALTER SYSTEM failed: {exc}") from exc
</code_context>
<issue_to_address>
**Retry erases newer override**

When A confirmed reset-all request is retried after its first execution committed but before the caller received a result, and another actor set a GUC before the retry, the first `RESET ALL` can succeed while its response is lost or a later status query fails; if the caller retries, the second execution also clears any `ALTER SYSTEM SET` committed by another actor in between, losing that newer configuration.

Make reset-all retries identifiable and avoid re-executing a request whose first execution already committed.
</issue_to_address>

### Comment 13
<location path="src/mcpg/guc_tools.py" line_range="26" />
<code_context>
+        return "none"
+    if reload == "always":
+        return "always"
+    return "if_needed"
+
+
</code_context>
<issue_to_address>
**Invalid reload mode is accepted**

When a caller supplies an unsupported or misspelled reload value, `_reload_mode()` maps unrecognized values to `if_needed`, so the tool proceeds with a different reload behavior instead of rejecting the invalid input.

Reject values other than `none`, `if_needed`, and `always` in `_reload_mode()`.

Also at `src/mcpg/guc_tools.py:79`.
</issue_to_address>

### Comment 14
<location path="src/mcpg/guc.py" line_range="312" />
<code_context>
+    activation_hint = "restart_required" if info.activation == "restart_required" else info.activation
+    warning = _warning_text(action=planned, name=name, activation=activation_hint)
+
+    if dry_run:
+        return AlterSystemResult(
+            status="dry_run",
</code_context>
<issue_to_address>
**Dry run accepts invalid values**

When the requested value is invalid for the selected GUC, the dry-run branch returns without asking PostgreSQL to validate the value, so callers receive a successful plan and discover the error only when they confirm the write.

Validate the requested value with PostgreSQL before returning a successful dry-run plan.

Also at `src/mcpg/guc_tools.py:65`, `src/mcpg/guc.py:313`.
</issue_to_address>

### Comment 15
<location path="src/mcpg/guc.py" line_range="183-184" />
<code_context>
+        "source, pending_restart, boot_val, reset_val, min_val, max_val, enumvals "
+        "FROM pg_settings WHERE name = %s",
+        [name],
+        force_readonly=True,
+    )
+    if not rows:
+        return None
</code_context>
<issue_to_address>
**Mixed-case GUC names appear unknown**

When a caller supplies a valid GUC name using uppercase or mixed case, `_fetch_guc_row()` compares the supplied name exactly with lowercase `pg_settings.name`, so the lookup returns no row and `inspect_guc()` and `alter_system()` report the setting as unknown.

Make the lookup case-insensitive or normalize the supplied GUC name before querying.

Also at `src/mcpg/guc.py:179`, `src/mcpg/guc.py:181-182`.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook