Skip to content

docs: identifier policy (match the sink) - #333

Closed
devopam wants to merge 4 commits into
mainfrom
docs/identifier-policy-v2
Closed

docs: identifier policy (match the sink)#333
devopam wants to merge 4 commits into
mainfrom
docs/identifier-policy-v2

Conversation

@devopam

@devopam devopam commented Sep 7, 2026

Copy link
Copy Markdown
Owner

Superseded by #334 (clean docs-only branch docs/identifier-policy-v3). This PR’s head branch had accidental placeholder/truncated file pushes — do not merge.

Document naming rules beyond hyphen (spaces, case, leading digits, dollar,
unicode, reserved words, embedded quotes). Distinguish SQL/pg_dump sinks
(quote_identifier) from plain-identifier-only sinks (ORM, AGE labels, shell,
regconfig, config keys). Include suggested tool-description and error text
so agents can recover.

Also notes the outdated README bullet claiming a global plain-identifier
regex still gates all SQL paths (fixed in 0.8.2 for SQL sinks).

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 3 issues

Prompt for AI Agents
Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="docs/identifier-policy.md" line_range="29-31" />
<code_context>
+| `order` / `user` | Reserved words — must be quoted as identifiers |
+| `weird"name` | Embedded `"` is doubled to `""` inside quotes |
+
+**Always rejected** (not valid addressable identifiers): empty string, embedded
+NUL (`\x00`), names longer than **63 bytes** (PostgreSQL's `NAMEDATALEN - 1`;
+the server would otherwise **silently truncate**).
+
+Callers pass the **bare** name (e.g. `adm-pgbench`). Do not add SQL quotes
</code_context>
<issue_to_address>
**issue:** The policy says names longer than 63 bytes are always rejected on SQL and `pg_dump` paths, but `pg_dump` pattern encoding accepts overlong names and passes them to `pg_dump`; callers therefore receive contradictory guidance about whether such names are valid for dump operations.

**Triggers:** When a caller uses an over-63-byte schema or table name with a dump tool.

**Suggested fix:** Either enforce the 63-byte limit in `_encode_schema_pattern` or document the dump-specific behavior separately from SQL identifier validation.

```suggestion
**Always rejected for SQL identifier paths** (not valid addressable identifiers):
empty string, embedded NUL (`\x00`), names longer than **63 bytes**
(PostgreSQL's `NAMEDATALEN - 1`; the server would otherwise **silently
truncate**). `pg_dump` pattern paths may accept overlong names and pass them
through to `pg_dump`.
```
</issue_to_address>

### Comment 2
<location path="docs/identifier-policy.md" line_range="12-16" />
<code_context>
+
+## SQL and `pg_dump` / `pg_restore` paths
+
+Tools that address real PostgreSQL objects (query, export, import, dump,
+replication, roles used in `SET ROLE`, and similar) accept any **addressable**
+PostgreSQL identifier. Values are encoded with `quote_identifier` (or dump
+pattern encoding for `pg_dump`), so injection is prevented by **escaping**,
+not by forbidding hyphens.
+
+| Example | Notes |
</code_context>
<issue_to_address>
**issue:** The document claims SQL-oriented tools accept any addressable PostgreSQL identifier and tells agents to retry SQL tools with the same name, but NL→SQL deliberately rejects names requiring delimited quoting and the documented `run_select` example is not an available MCPg tool. An agent following this policy either gets another rejection or attempts a nonexistent tool.

**Triggers:** When an agent uses NL→SQL or follows the documented retry list after a plain-identifier rejection.

**Suggested fix:** List NL→SQL among the deliberate strict exceptions with its prompt-injection rationale, and replace `run_select` with an actual registered SQL tool name.
</issue_to_address>

### Comment 3
<location path="docs/identifier-policy.md" line_range="48-50" />
<code_context>
+
+| Sink | Examples | Why |
+|------|----------|-----|
+| Generated source | Prisma, Drizzle, Diesel, Ecto, Ent, jOOQ, sqlc, SQLAlchemy export | Name becomes an identifier in TypeScript / Go / Rust / Elixir / Python |
+| Graph labels | Apache AGE / graph projection labels | Extension label rules, not PG delimited identifiers |
+| Shell / host command | `schedule_logical_backup` paths, some `COPY TO PROGRAM` args | Shell metacharacters are a real injection surface |
+| Some SQL *literals* | `regconfig` in text-search helpers | Embedded as a string literal, not as a delimited identifier |
+| Config keys | `MCPG_SECONDARY_DATABASE_URLS` names | MCPg's own ids (`[a-z0-9_]+`), not PG object names |
</code_context>
<issue_to_address>
**nitpick:** The shell row describes `schedule_logical_backup` paths as plain-identifier-only inputs, but those inputs are path values with a different allowlist: destinations require absolute POSIX paths and `pg_dump_path` permits path separators, while the database argument accepts hyphens. Treating these as `[A-Za-z_][A-Za-z0-9_]*` names gives agents the wrong validation and recovery instructions.

**Triggers:** When an agent chooses or validates arguments for `schedule_logical_backup`.

**Suggested fix:** Describe shell/path arguments using their actual path-specific allowlists, and distinguish them from identifier validation for the database argument.
</issue_to_address>

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

Comment thread docs/identifier-policy.md
Comment on lines +29 to +31
**Always rejected** (not valid addressable identifiers): empty string, embedded
NUL (`\x00`), names longer than **63 bytes** (PostgreSQL's `NAMEDATALEN - 1`;
the server would otherwise **silently truncate**).

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The policy says names longer than 63 bytes are always rejected on SQL and pg_dump paths, but pg_dump pattern encoding accepts overlong names and passes them to pg_dump; callers therefore receive contradictory guidance about whether such names are valid for dump operations.

Triggers: When a caller uses an over-63-byte schema or table name with a dump tool.

Suggested fix: Either enforce the 63-byte limit in _encode_schema_pattern or document the dump-specific behavior separately from SQL identifier validation.

Suggested change
**Always rejected** (not valid addressable identifiers): empty string, embedded
NUL (`\x00`), names longer than **63 bytes** (PostgreSQL's `NAMEDATALEN - 1`;
the server would otherwise **silently truncate**).
**Always rejected for SQL identifier paths** (not valid addressable identifiers):
empty string, embedded NUL (`\x00`), names longer than **63 bytes**
(PostgreSQL's `NAMEDATALEN - 1`; the server would otherwise **silently
truncate**). `pg_dump` pattern paths may accept overlong names and pass them
through to `pg_dump`.

Comment thread docs/identifier-policy.md
Comment on lines +12 to +16
Tools that address real PostgreSQL objects (query, export, import, dump,
replication, roles used in `SET ROLE`, and similar) accept any **addressable**
PostgreSQL identifier. Values are encoded with `quote_identifier` (or dump
pattern encoding for `pg_dump`), so injection is prevented by **escaping**,
not by forbidding hyphens.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

issue: The document claims SQL-oriented tools accept any addressable PostgreSQL identifier and tells agents to retry SQL tools with the same name, but NL→SQL deliberately rejects names requiring delimited quoting and the documented run_select example is not an available MCPg tool. An agent following this policy either gets another rejection or attempts a nonexistent tool.

Triggers: When an agent uses NL→SQL or follows the documented retry list after a plain-identifier rejection.

Suggested fix: List NL→SQL among the deliberate strict exceptions with its prompt-injection rationale, and replace run_select with an actual registered SQL tool name.

Comment thread docs/identifier-policy.md
Comment on lines +48 to +50
| Generated source | Prisma, Drizzle, Diesel, Ecto, Ent, jOOQ, sqlc, SQLAlchemy export | Name becomes an identifier in TypeScript / Go / Rust / Elixir / Python |
| Graph labels | Apache AGE / graph projection labels | Extension label rules, not PG delimited identifiers |
| Shell / host command | `schedule_logical_backup` paths, some `COPY TO PROGRAM` args | Shell metacharacters are a real injection surface |

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nitpick: The shell row describes schedule_logical_backup paths as plain-identifier-only inputs, but those inputs are path values with a different allowlist: destinations require absolute POSIX paths and pg_dump_path permits path separators, while the database argument accepts hyphens. Treating these as [A-Za-z_][A-Za-z0-9_]* names gives agents the wrong validation and recovery instructions.

Triggers: When an agent chooses or validates arguments for schedule_logical_backup.

Suggested fix: Describe shell/path arguments using their actual path-specific allowlists, and distinguish them from identifier validation for the database argument.

Rejection messages now name the sink, the plain-identifier rule, and
point agents at SQL tools / docs/identifier-policy.md instead of a
vague "invalid name".
@devopam devopam closed this Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant