[c1-squire-dev]

Fixes the privilege names baton-mysql sends to GRANT and REVOKE, so grants and revokes that MySQL rejects today (errors 1149/1064/3619, and revokes that silently do nothing) work as the entitlements promise.

What was asked

CXH-2679: baton-mysql builds the SQL privilege name from its lower-case catalog keys. Multi-word privileges reached MySQL as SHOW_DATABASES instead of SHOW DATABASES — grant failed with error 1149, revoke "succeeded" with only warning 3929 while the privilege stayed on the account. The _with_grant suffix was never stripped (a case-mismatched replace), so every *_with_grant server entitlement failed with 1064 on grant and revoke. The table path passed the key through unchanged, so create_view/show_view failed with 3619. Sync already reads all of this correctly; only the provisioning path is wrong.

This is a fresh implementation of #33 — same bug, reworked after that PR's review feedback.

What changed

The privilege mapper also corrects the transposed NDB stored-user spelling — both the frozen catalog slug nbd_stored_user and the DB-derived ndb_stored_user spelling sync emits on revoke — to MySQL's NDB_STORED_USER.

One privilege-name mapper, five call sites:

pkg/connector/privileges.go (new)

What changed — sqlPrivilegeName maps the 13 multi-word static privileges to their space-separated SQL spellings and maps grant to GRANT OPTION. Single-word statics and the enumerated dynamic privileges (BACKUP_ADMIN, SYSTEM_VARIABLES_ADMIN, …) pass through unchanged, so the underscores MySQL requires stay in place. trimWithGrantSuffix strips the _with_grant suffix case-insensitively. resolveServerPrivilege rejects proxy at server scope with a gRPC Unimplemented error before any SQL is built, because GRANT PROXY ON *.* is not valid MySQL grammar.

Why — multi-word static privileges are keywords MySQL only accepts space-separated, while dynamic privileges are names registered with underscores. A blanket underscore→space replace (the database path's old shortcut) trades one error class for another, so the map is closed and anything unclassified with underscores fails with a clear error instead of guessing.

pkg/connector/server.go

Grant passes the resolved SQL name plus a withGrant flag to the client, which appends WITH GRANT OPTION after the quoted user — MySQL grammar requires the clause to trail the TO clause (same shape as the existing proxy grant in pkg/client/roles.go), not to sit in the privilege slot. Revoke sends the base privilege only — revoking drops the grant option along with the privilege, matching how sync models the entitlement. The broken extractServerPrivilege is deleted.

pkg/connector/table.go, database.go, routine.go, column.go

All four route the entitlement key through the mapper. Table gets CREATE VIEW/SHOW VIEW. Database loses its blanket replace — byte-identical SQL for every existing database-scope privilege, and GRANT OPTION for grant. Routine gains case "grant" (GRANT OPTION ON PROCEDURE is valid MySQL). Column behavior is unchanged: the client expands grant into SELECT, INSERT, UPDATE, REFERENCES (col), which is valid SQL; the syncer now also guards len(parts) != 4 on Grant, matching its Revoke.

Untouched on purpose

pkg/client/* keeps doing the SQL interpolation (its ToUpper is space-preserving), and pkg/connector/entitlements.go — the sync catalog: slugs, display names, entitlement IDs and counts are byte-identical, pinned by a test.

How it was tested

• Table-driven unit tests in pkg/connector: the mapper for every class (13 multi-word statics, dynamics, _with_grant case variants, grant, fail-closed rejections); a catalog-exhaustiveness test that fails the build when a future entitlement is added without a classification; a sync-invariance test pinning the sync surface; syncer-level tests proving rejected entitlements never reach a client call; charset validation tests pinning fail-closed behavior for underscore-free garbage slugs.
• DSN-gated live tests (provisioning_mysql_test.go) grant and revoke through the real syncers against a MySQL server and assert presence/absence via SHOW GRANTS. They are wired into the existing ci.yaml test job — which already starts mysql:8 and exports BATON_CONNECTION_STRING — as a new go test ./pkg/connector/... ./pkg/client/... step, so they run on every PR. Unit tests run without a DSN.
• Smoke in this environment: SMOKE PASS (partial): cmd/baton-mysql builds and starts. NOTE: this repo declares no mock, so no sync was exercised — the change is unproven against a live upstream here; the wired CI step above is where the live sync runs.
• connector-eval (advisory): not-applicable — no recording (eval-prepare: mock-needed: no credentials and no mock); static analysis only: 0 new findings at any severity.
• CI and the repository's review bot have ruled: all checks green at head c9a4881a (including the wired test job running the go test suite against mysql:8), and the review bot accepted after 5 gate passes — its objections across passes (a real WITH GRANT OPTION grammar bug its first CI run caught, the column-grant expansion restoration, guards, prefixes, and the NDB spelling) were each fixed or answered, and every review thread is resolved.
• An independent reviewer that had seen none of this batch read the PR cold in a separate environment across two rounds: round 1 raised three things (the transposed NDB_STORED_USER spelling — both the catalog slug and the DB-derived spelling now map correctly; missing baton-mysql: error prefixes; an exhaustiveness-test default), round 2 raised two (a revoke-only regression for dynamic privileges the frozen catalog does not list — revokes now keep the spelling main sent while grants stay fail-closed; one error prefix). All five were fixed; every review thread is answered and resolved.
• A validation pass drove the real syncers against a fake MySQL wire-protocol server and captured the actual SQL bytes before/after for every classified case (all 13 multi-word statics now spelled with spaces, BACKUP_ADMIN unchanged, trailing WITH GRANT OPTION, GRANT OPTION mapping, table CREATE VIEW/SHOW VIEW, proxy rejected before any client call, sync surface byte-identical); the end-to-end grant/revoke round-trip against a real server is exercised by the wired CI step rather than locally (no MySQL server exists in the validation environment) — 13 of 15 claims verified working, 2 honestly untestable locally, 0 broken.

What to look at hardest

• pkg/connector/privileges.go — the static/dynamic classification is the whole fix. A misclassification fails loudly (clear error), but a wrong multi-word mapping would still reach MySQL.
• The deliberate behavior changes: proxy at server scope now returns a clear error instead of sending SQL MySQL rejects; REVOKE of a *_with_grant entitlement removes the grant option together with the privilege. Column-scope grant is unchanged behavior — the client expands it into the four column privileges. The server syncer's revoke path deliberately falls back to the uppercased slug for dynamic privileges the frozen catalog does not list — those revokes keep the spelling main sent (sync can surface documented dynamics the catalog cannot grow without breaking the stable-slug API) — while grants stay fail-closed.

Release Notes

baton-mysql grants and revokes now spell privileges the way MySQL expects: multi-word privileges (SHOW DATABASES, CREATE USER, REPLICATION CLIENT, …) work at server, database, table and routine scope, *_with_grant entitlements grant and revoke with WITH GRANT OPTION, and grant maps to GRANT OPTION everywhere it is valid. Granting proxy at server scope now returns a clear "not supported at this scope" error instead of sending SQL that MySQL rejects. *_with_grant grants use MySQL's trailing WITH GRANT OPTION grammar. Column-scope grant keeps its pre-existing expansion into the four column privileges. Sync output is unchanged.

Refs #33

[linear-code]

CXH-2679

[github-actions]

Superseded — see the current review report for commit 4e6f93783a8c

Connector PR Review: CXH-2679: fix server and table privilege names in baton-mysql

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 50261bda21d7.
Review mode: full
View review run

Review Summary

This PR adds one mapper, pkg/connector/privileges.go, that turns entitlement slugs into the privilege names MySQL expects:

• The 13 multi-word static privileges get spaces (for example SHOW DATABASES), and grant becomes GRANT OPTION.
• The _with_grant suffix is now stripped regardless of case.
• Dynamic privileges keep their underscores, and unknown names containing an underscore fail closed.
• Server proxy returns Unimplemented.

Server, database and table Grant/Revoke use the mapper. Routine adds a grant case and column rejects grant. Server Revoke sends only the base privilege. That matches sync: pkg/connector/grants.go:134 emits only X_with_grant when WITH_GRANT_OPTION=Y, so revoking the base privilege removes exactly the access that entitlement represents. go.mod only moves google.golang.org/grpc from indirect to direct, which matches the new codes/status imports. The CI step runs the DSN-gated live tests against the mysql:8 service that is already in that job.

I scanned the full diff for security and correctness. I applied the repo-local criteria as follows:

• Provisioning (PR1–PR3, P1–P6): I read all of server.go, database.go, table.go, routine.go and column.go. Principal and entitlement sources are unchanged, and the argument order to the client calls is correct.
• E1–E3: the new rejections carry gRPC codes.
• Breaking changes (B/BP): none. The sync catalog in entitlements.go is untouched.
• Section S and logging (A): not relevant, because no sync method changed.

Coverage gap: I read the new test files only for setup and cleanup (the live test creates and drops baton_prov_test). I did not check them assertion by assertion.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (medium confidence), pkg/connector/column.go:80-82,107-109: column grant used to work. pkg/client/columns.go:94/:128 turned it into SELECT, INSERT, UPDATE, REFERENCES (col), which is valid SQL, and it never sent GRANT OPTION. It is now rejected with InvalidArgument. The PR description says it replaces "SQL MySQL rejects", which is wrong, and the client's expansion branch is now dead code.
• New (low confidence), pkg/connector/privileges.go:101: any slug without an underscore is upper-cased and interpolated into GRANT %s ON ... with no allowlist check. This was already true before the PR, so it is not a regression. Validating against the entitlement catalog would make the fail-closed claim hold for every input.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/column.go`:
- Around lines 80-82 and 107-109: Column-scope `grant` is now rejected, but before this PR the client (`pkg/client/columns.go` lines ~94 and ~128) expanded it to SELECT, INSERT, UPDATE, REFERENCES on the column, which is valid SQL. Either keep the rejection, delete the now-unreachable `grant` expansion branch in GrantColumnPrivilege/RevokeColumnPrivilege, and correct the PR description/release notes to say this removes the old four-privilege expansion; or drop the rejection to keep the previous behavior.

In `pkg/connector/privileges.go`:
- Around line 101: sqlPrivilegeName returns strings.ToUpper(base) for any slug without an underscore, with no allowlist check, and the result is interpolated into GRANT/REVOKE SQL. Check `base` against the known entitlement IDs (for example, build a set from entitlementsByResourceType) or at least require ^[a-z]+$, and return codes.InvalidArgument otherwise.

[github-actions]

Superseded — see the current review report for commit b97dcd12d768

Connector PR Review: CXH-2679: fix server and table privilege names in baton-mysql

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 50261bda21d7.
Review mode: full
View review run

Review Summary

This PR adds one mapper, pkg/connector/privileges.go, that turns entitlement slugs into the privilege names MySQL expects:

• The 13 multi-word static privileges get spaces (for example SHOW DATABASES), and grant becomes GRANT OPTION.
• The _with_grant suffix is now stripped regardless of case.
• Dynamic privileges keep their underscores, and unknown names containing an underscore fail closed.
• Server proxy returns Unimplemented.

Server, database and table Grant/Revoke use the mapper. Routine adds a grant case and column rejects grant. Server Revoke sends only the base privilege. That matches sync: pkg/connector/grants.go:134 emits only X_with_grant when WITH_GRANT_OPTION=Y, so revoking the base privilege removes exactly the access that entitlement represents. go.mod only moves google.golang.org/grpc from indirect to direct, which matches the new codes/status imports. The CI step runs the DSN-gated live tests against the mysql:8 service that is already in that job.

I scanned the full diff for security and correctness. I applied the repo-local criteria as follows:

• Provisioning (PR1–PR3, P1–P6): I read all of server.go, database.go, table.go, routine.go and column.go. Principal and entitlement sources are unchanged, and the argument order to the client calls is correct.
• E1–E3: the new rejections carry gRPC codes.
• Breaking changes (B/BP): none. The sync catalog in entitlements.go is untouched.
• Section S and logging (A): not relevant, because no sync method changed.

Coverage gap: I read the new test files only for setup and cleanup (the live test creates and drops baton_prov_test). I did not check them assertion by assertion.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (medium confidence), pkg/connector/column.go:80-82,107-109: column grant used to work. pkg/client/columns.go:94/:128 turned it into SELECT, INSERT, UPDATE, REFERENCES (col), which is valid SQL, and it never sent GRANT OPTION. It is now rejected with InvalidArgument. The PR description says it replaces "SQL MySQL rejects", which is wrong, and the client's expansion branch is now dead code.
• New (low confidence), pkg/connector/privileges.go:101: any slug without an underscore is upper-cased and interpolated into GRANT %s ON ... with no allowlist check. This was already true before the PR, so it is not a regression. Validating against the entitlement catalog would make the fail-closed claim hold for every input.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/column.go`:
- Around lines 80-82 and 107-109: Column-scope `grant` is now rejected, but before this PR the client (`pkg/client/columns.go` lines ~94 and ~128) expanded it to SELECT, INSERT, UPDATE, REFERENCES on the column, which is valid SQL. Either keep the rejection, delete the now-unreachable `grant` expansion branch in GrantColumnPrivilege/RevokeColumnPrivilege, and correct the PR description/release notes to say this removes the old four-privilege expansion; or drop the rejection to keep the previous behavior.

In `pkg/connector/privileges.go`:
- Around line 101: sqlPrivilegeName returns strings.ToUpper(base) for any slug without an underscore, with no allowlist check, and the result is interpolated into GRANT/REVOKE SQL. Check `base` against the known entitlement IDs (for example, build a set from entitlementsByResourceType) or at least require ^[a-z]+$, and return codes.InvalidArgument otherwise.

---

Reviewed commit: 4e6f93783a8c

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit b97dcd12d768

Connector PR Review: CXH-2679: fix server and table privilege names in baton-mysql

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 50261bda21d7.
Review mode: incremental since 4e6f937
View review run

Review Summary

The new commit changes three things:

• GrantServerPrivilege now takes a withGrant flag and adds WITH GRANT OPTION after the TO clause. The live test now checks that the clause comes last.
• The column-scope InvalidArgument rejection is removed, so the client's existing grant → SELECT, INSERT, UPDATE, REFERENCES (col) expansion runs again.
• sqlPrivilegeName now rejects any base slug that doesn't match ^[a-z_]+$ before building SQL.

I scanned the full PR diff for security and correctness: the mapper, the server/database/table/routine/column syncers, the client SQL builders, ci.yaml, and go.mod (grpc moved from indirect to direct because of the new status/codes imports, which is justified). The repo-local criteria applied:

• Provisioning (PR1–PR3): I read the full Grant/Revoke flows. The principal comes from principal.Id, there is no entitlement.Resource.ParentResourceId use, and the client argument order is correct.
• Breaking changes (B/BP): none. The sync catalog in entitlements.go is unchanged.
• Error codes (E2/E3): rejections use InvalidArgument/Unimplemented.
• Sync rules (S/L): not applicable. Grants()/List were not touched.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (high confidence, low impact), pkg/connector/privileges.go:86-95: grantPrivilegeString still puts WITH GRANT OPTION in the privilege slot for database scope (database.go:97) and table scope (table.go:99). That is the same invalid grammar this commit fixed at server scope. Catalog *_with_grant entitlements are server-only (includeWithGrant + globalScope), so only crafted IDs reach it, and they fail with MySQL 1064.
• New (high confidence, low impact, existed before this PR), pkg/connector/column.go:79-81: Grant indexes columnParts[1]/[2] without a length check, so entitlement:select:column:db.tbl panics. Revoke already guards len(idParts) != 3.

Resolved prior findings

• Column grant rejection, flagged in the previous summary: fixed. The rejection is removed in pkg/connector/column.go:73-77,97-101, so the four-privilege expansion in pkg/client/columns.go:94,128 runs again, which matches the PR description.
• Underscore-free slugs reached SQL without validation, flagged in the previous summary: fixed. baseSlugRe (^[a-z_]+$) is checked in pkg/connector/privileges.go:97-105, and TestSqlPrivilegeNameCharset covers it. The column path still passes parts[1] to the client unvalidated, but that is unchanged from base and only happens with crafted IDs.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/privileges.go`:
- Around line 86-95: grantPrivilegeString appends " WITH GRANT OPTION" to the privilege name. database.go:97 and table.go:99 then interpolate that into "GRANT <priv> ON <obj> TO <user>", which is invalid MySQL grammar (the clause must come after TO). Either return codes.InvalidArgument when withGrant is true at database/table scope (no catalog entitlement produces it there), or add a withGrant bool parameter to GrantDatabasePrivilege/GrantTablePrivilege that appends the clause after the TO clause, like GrantServerPrivilege does. Then delete grantPrivilegeString and fix its doc comment.

In `pkg/connector/column.go`:
- Around line 79-81: columnSyncer.Grant splits parts[3] on "." and indexes [1] and [2] without checking the length, so it panics on a malformed column ID. Add `if len(columnParts) != 3 { return nil, fmt.Errorf("invalid column ID format: %s", parts[3]) }`, matching Revoke, and add a Grant case to TestColumnSyncerGuards.

[github-actions]

Superseded — see the current review report for commit 56d93cae577c

Connector PR Review: CXH-2679: fix server and table privilege names in baton-mysql

Blocking Issues: 0 | Suggestions: 2 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 50261bda21d7.
Review mode: incremental since 4e6f937
View review run

Review Summary

The new commit changes three things:

• GrantServerPrivilege now takes a withGrant flag and adds WITH GRANT OPTION after the TO clause. The live test now checks that the clause comes last.
• The column-scope InvalidArgument rejection is removed, so the client's existing grant → SELECT, INSERT, UPDATE, REFERENCES (col) expansion runs again.
• sqlPrivilegeName now rejects any base slug that doesn't match ^[a-z_]+$ before building SQL.

I scanned the full PR diff for security and correctness: the mapper, the server/database/table/routine/column syncers, the client SQL builders, ci.yaml, and go.mod (grpc moved from indirect to direct because of the new status/codes imports, which is justified). The repo-local criteria applied:

• Provisioning (PR1–PR3): I read the full Grant/Revoke flows. The principal comes from principal.Id, there is no entitlement.Resource.ParentResourceId use, and the client argument order is correct.
• Breaking changes (B/BP): none. The sync catalog in entitlements.go is unchanged.
• Error codes (E2/E3): rejections use InvalidArgument/Unimplemented.
• Sync rules (S/L): not applicable. Grants()/List were not touched.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

• New (high confidence, low impact), pkg/connector/privileges.go:86-95: grantPrivilegeString still puts WITH GRANT OPTION in the privilege slot for database scope (database.go:97) and table scope (table.go:99). That is the same invalid grammar this commit fixed at server scope. Catalog *_with_grant entitlements are server-only (includeWithGrant + globalScope), so only crafted IDs reach it, and they fail with MySQL 1064.
• New (high confidence, low impact, existed before this PR), pkg/connector/column.go:79-81: Grant indexes columnParts[1]/[2] without a length check, so entitlement:select:column:db.tbl panics. Revoke already guards len(idParts) != 3.

Resolved prior findings

• Column grant rejection, flagged in the previous summary: fixed. The rejection is removed in pkg/connector/column.go:73-77,97-101, so the four-privilege expansion in pkg/client/columns.go:94,128 runs again, which matches the PR description.
• Underscore-free slugs reached SQL without validation, flagged in the previous summary: fixed. baseSlugRe (^[a-z_]+$) is checked in pkg/connector/privileges.go:97-105, and TestSqlPrivilegeNameCharset covers it. The column path still passes parts[1] to the client unvalidated, but that is unchanged from base and only happens with crafted IDs.

Prompt for AI agents

Verify each finding against the current code and only fix it if needed.

## Suggestions

In `pkg/connector/privileges.go`:
- Around line 86-95: grantPrivilegeString appends " WITH GRANT OPTION" to the privilege name. database.go:97 and table.go:99 then interpolate that into "GRANT <priv> ON <obj> TO <user>", which is invalid MySQL grammar (the clause must come after TO). Either return codes.InvalidArgument when withGrant is true at database/table scope (no catalog entitlement produces it there), or add a withGrant bool parameter to GrantDatabasePrivilege/GrantTablePrivilege that appends the clause after the TO clause, like GrantServerPrivilege does. Then delete grantPrivilegeString and fix its doc comment.

In `pkg/connector/column.go`:
- Around line 79-81: columnSyncer.Grant splits parts[3] on "." and indexes [1] and [2] without checking the length, so it panics on a malformed column ID. Add `if len(columnParts) != 3 { return nil, fmt.Errorf("invalid column ID format: %s", parts[3]) }`, matching Revoke, and add a Grant case to TestColumnSyncerGuards.

---

Reviewed commit: b97dcd12d768

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 56d93cae577c

Connector PR Review: CXH-2679: fix server and table privilege names in baton-mysql

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 50261bda21d7.
Review mode: incremental since b97dcd1
View review run

Review Summary

This PR adds one privilege-name mapper (pkg/connector/privileges.go) and uses it in the server, database, table, routine and column Grant/Revoke paths. Multi-word static privileges now go to MySQL with spaces (SHOW DATABASES, CREATE VIEW, …), dynamic privileges keep their underscores, grant becomes GRANT OPTION, and server scope puts WITH GRANT OPTION after the TO clause.

The new commit does two things:

• grantPrivilegeString now returns InvalidArgument instead of putting WITH GRANT OPTION in the privilege slot at database and table scope (privileges.go:91-96, database.go:97-101, table.go:99-103).
• columnSyncer.Grant now checks len(columnParts) != 3 (column.go:80-82).

Both changes have tests (TestGrantPrivilegeStringRejectsWithGrant, TestDatabaseTableSyncerWithGrantRejected, and a new case in TestColumnSyncerGuards).

I scanned the full PR diff for security and correctness, including go.mod and ci.yaml:

• go.mod: google.golang.org/grpc moves from indirect to direct, which matches the new status/codes imports. The version is unchanged.
• ci.yaml: the new go test step goes into the existing job, which already has MySQL. Nothing in the incremental artifact was dropped or truncated.

How the repo-local criteria applied:

• Provisioning (PR1–PR3): I read the full Grant/Revoke flows. The principal comes from principal.Id. There is no entitlement.Resource.ParentResourceId use. Argument order to the client is correct.
• Breaking changes (B/BP): none. entitlements.go is unchanged, so slugs and IDs are the same.
• Error codes (E2/E3): rejections use InvalidArgument/Unimplemented.
• Sync, logging, spans, JSON (S/L/T/J): not applicable. List/Entitlements/Grants and the response structs were not touched.

On revoke behavior: revoking a server-scope *_with_grant sends the base privilege. The only catalog *_with_grant entitlements at server scope are dynamic privileges and proxy, which is rejected. For a dynamic privilege, REVOKE deletes the whole mysql.global_grants row, so the grant option goes with it. The PR description already calls this out as intended.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Database/table WITH GRANT OPTION in the privilege slot (from the previous summary): fixed. grantPrivilegeString now fails closed with InvalidArgument (pkg/connector/privileges.go:91-96), and both callers check the error (database.go:97-101, table.go:99-103). TestDatabaseTableSyncerWithGrantRejected covers it.
• columnSyncer.Grant indexing without a length check (from the previous summary): fixed. The guard is at pkg/connector/column.go:80-82, and the grant malformed column ID case in syncer_test.go covers it.

[github-actions]

Superseded — see the current review report for commit 58af28841b30

Connector PR Review: CXH-2679: fix server and table privilege names in baton-mysql

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 50261bda21d7.
Review mode: incremental since b97dcd1
View review run

Review Summary

This PR adds one privilege-name mapper (pkg/connector/privileges.go) and uses it in the server, database, table, routine and column Grant/Revoke paths. Multi-word static privileges now go to MySQL with spaces (SHOW DATABASES, CREATE VIEW, …), dynamic privileges keep their underscores, grant becomes GRANT OPTION, and server scope puts WITH GRANT OPTION after the TO clause.

The new commit does two things:

• grantPrivilegeString now returns InvalidArgument instead of putting WITH GRANT OPTION in the privilege slot at database and table scope (privileges.go:91-96, database.go:97-101, table.go:99-103).
• columnSyncer.Grant now checks len(columnParts) != 3 (column.go:80-82).

Both changes have tests (TestGrantPrivilegeStringRejectsWithGrant, TestDatabaseTableSyncerWithGrantRejected, and a new case in TestColumnSyncerGuards).

I scanned the full PR diff for security and correctness, including go.mod and ci.yaml:

• go.mod: google.golang.org/grpc moves from indirect to direct, which matches the new status/codes imports. The version is unchanged.
• ci.yaml: the new go test step goes into the existing job, which already has MySQL. Nothing in the incremental artifact was dropped or truncated.

How the repo-local criteria applied:

• Provisioning (PR1–PR3): I read the full Grant/Revoke flows. The principal comes from principal.Id. There is no entitlement.Resource.ParentResourceId use. Argument order to the client is correct.
• Breaking changes (B/BP): none. entitlements.go is unchanged, so slugs and IDs are the same.
• Error codes (E2/E3): rejections use InvalidArgument/Unimplemented.
• Sync, logging, spans, JSON (S/L/T/J): not applicable. List/Entitlements/Grants and the response structs were not touched.

On revoke behavior: revoking a server-scope *_with_grant sends the base privilege. The only catalog *_with_grant entitlements at server scope are dynamic privileges and proxy, which is rejected. For a dynamic privilege, REVOKE deletes the whole mysql.global_grants row, so the grant option goes with it. The PR description already calls this out as intended.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Resolved prior findings

• Database/table WITH GRANT OPTION in the privilege slot (from the previous summary): fixed. grantPrivilegeString now fails closed with InvalidArgument (pkg/connector/privileges.go:91-96), and both callers check the error (database.go:97-101, table.go:99-103). TestDatabaseTableSyncerWithGrantRejected covers it.
• columnSyncer.Grant indexing without a length check (from the previous summary): fixed. The guard is at pkg/connector/column.go:80-82, and the grant malformed column ID case in syncer_test.go covers it.

---

Reviewed commit: 56d93cae577c

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit 58af28841b30

Connector PR Review: CXH-2679: fix server and table privilege names in baton-mysql

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 50261bda21d7.
Review mode: incremental since 56d93ca
View review run

Review Summary

The new commit adds privilegeSpellingOverrides in pkg/connector/privileges.go:107-110, which runs before the static and dynamic lookups. It maps both the transposed catalog slug nbd_stored_user and the DB spelling ndb_stored_user, with or without _with_grant, to MySQL's NDB_STORED_USER.

It also adds the baton-mysql: prefix to the error messages in column.go and database.go, and adds a default: arm to TestCatalogExhaustiveness so an unclassified resource type fails the test. The new tests cover both spellings, the _with_grant variant, and the server-scope resolve path.

Coverage:

• I scanned the full PR diff for security and correctness. That covers server.go, table.go, database.go, routine.go, column.go, pkg/client/servers.go, go.mod and ci.yaml.
• Every privilege name that reaches SQL comes from a closed map or matches ^[a-z_]+$ before being uppercased, so there is no injection path.
• go.mod: google.golang.org/grpc moves from indirect to direct, which matches the new status/codes imports. The version is unchanged.
• The incremental artifact was not partial.

Criteria applied:

• Provisioning (PR1–PR3): I re-read the Grant/Revoke flows. The principal comes from principal.Id/grant.Principal.Id, there is no entitlement.Resource.ParentResourceId use, and the argument order is correct.
• Breaking changes (B/BP): none. entitlements.go is untouched, so slugs and IDs are unchanged.
• Error codes (E2/E3): the rejections use InvalidArgument/Unimplemented.
• Sync, logging, spans, JSON (S/L/T/J): not applicable, because no List/Entitlements/Grants path or response struct changed.

There are no prior inline findings to audit, and this run found nothing new.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

[github-actions]

Superseded — see the current review report for commit c9a4881aa19f

Connector PR Review: CXH-2679: fix server and table privilege names in baton-mysql

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 50261bda21d7.
Review mode: incremental since 56d93ca
View review run

Review Summary

The new commit adds privilegeSpellingOverrides in pkg/connector/privileges.go:107-110, which runs before the static and dynamic lookups. It maps both the transposed catalog slug nbd_stored_user and the DB spelling ndb_stored_user, with or without _with_grant, to MySQL's NDB_STORED_USER.

It also adds the baton-mysql: prefix to the error messages in column.go and database.go, and adds a default: arm to TestCatalogExhaustiveness so an unclassified resource type fails the test. The new tests cover both spellings, the _with_grant variant, and the server-scope resolve path.

Coverage:

• I scanned the full PR diff for security and correctness. That covers server.go, table.go, database.go, routine.go, column.go, pkg/client/servers.go, go.mod and ci.yaml.
• Every privilege name that reaches SQL comes from a closed map or matches ^[a-z_]+$ before being uppercased, so there is no injection path.
• go.mod: google.golang.org/grpc moves from indirect to direct, which matches the new status/codes imports. The version is unchanged.
• The incremental artifact was not partial.

Criteria applied:

• Provisioning (PR1–PR3): I re-read the Grant/Revoke flows. The principal comes from principal.Id/grant.Principal.Id, there is no entitlement.Resource.ParentResourceId use, and the argument order is correct.
• Breaking changes (B/BP): none. entitlements.go is untouched, so slugs and IDs are unchanged.
• Error codes (E2/E3): the rejections use InvalidArgument/Unimplemented.
• Sync, logging, spans, JSON (S/L/T/J): not applicable, because no List/Entitlements/Grants path or response struct changed.

There are no prior inline findings to audit, and this run found nothing new.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

---

Reviewed commit: 58af28841b30

[github-actions]

No blocking issues found — see the full review report

[github-actions]

Superseded — see the current review report for commit c9a4881aa19f

Connector PR Review: CXH-2679: fix server and table privilege names in baton-mysql

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 50261bda21d7.
Review mode: incremental since 58af288
View review run

Review Summary

The PR adds one mapper, sqlPrivilegeName, that turns catalog slugs into the privilege names MySQL accepts. Server, table, database, routine and column Grant/Revoke all go through it. Server *_with_grant grants now put WITH GRANT OPTION after the TO clause, as MySQL requires.

This push adds resolveServerPrivilege(id, forRevoke) and revokeServerPrivilegeName (pkg/connector/privileges.go:138-174). On the server Revoke path only, a slug that the frozen catalog does not list still gets uppercased and sent, but only if it matches ^[a-z_]+$ and contains an underscore. Grant still refuses unknown slugs. proxy and role_assignment are still rejected on both paths. The push also adds the baton-mysql: prefix to the malformed-ID error, and updates the tests: a syncer-level revoke check was replaced by pass-through cases in TestResolveServerPrivilege.

The fallback is safe from SQL injection: everything reaching REVOKE %s ON *.* (pkg/client/servers.go:63) is limited to [a-z_] before being uppercased. This matches what main sent, so dynamic privileges that sync finds but the catalog lacks can still be revoked.

I scanned the full PR diff for security and correctness: ci.yaml, go.mod, pkg/client/servers.go, the five syncers, privileges.go and the tests.

• go.mod: google.golang.org/grpc moves from indirect to direct with no version change. That matches the new status/codes imports.

How the trusted criteria applied:

• Provisioning (PR1–PR3): I re-read the full Grant/Revoke flows. The principal comes from principal.Id/grant.Principal.Id, there is no entitlement.Resource.ParentResourceId use, and the argument order is correct.
• Breaking changes (B/BP): none. entitlements.go is unchanged.
• Error codes (E2/E3): rejections use InvalidArgument/Unimplemented.
• Sync, logging, spans, JSON (S/L/T/J): not applicable, because no List/Entitlements/Grants path, log line, span or response struct changed.

There were no prior inline findings to audit, and the incremental diff was not partial.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

[github-actions]

Connector PR Review: CXH-2679: fix server and table privilege names in baton-mysql

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base 50261bda21d7.
Review mode: incremental since 58af288
View review run

Review Summary

The PR adds one mapper, sqlPrivilegeName, that turns catalog slugs into the privilege names MySQL accepts. Server, table, database, routine and column Grant/Revoke all go through it. Server *_with_grant grants now put WITH GRANT OPTION after the TO clause, as MySQL requires.

This push adds resolveServerPrivilege(id, forRevoke) and revokeServerPrivilegeName (pkg/connector/privileges.go:138-174). On the server Revoke path only, a slug that the frozen catalog does not list still gets uppercased and sent, but only if it matches ^[a-z_]+$ and contains an underscore. Grant still refuses unknown slugs. proxy and role_assignment are still rejected on both paths. The push also adds the baton-mysql: prefix to the malformed-ID error, and updates the tests: a syncer-level revoke check was replaced by pass-through cases in TestResolveServerPrivilege.

The fallback is safe from SQL injection: everything reaching REVOKE %s ON *.* (pkg/client/servers.go:63) is limited to [a-z_] before being uppercased. This matches what main sent, so dynamic privileges that sync finds but the catalog lacks can still be revoked.

I scanned the full PR diff for security and correctness: ci.yaml, go.mod, pkg/client/servers.go, the five syncers, privileges.go and the tests.

• go.mod: google.golang.org/grpc moves from indirect to direct with no version change. That matches the new status/codes imports.

How the trusted criteria applied:

• Provisioning (PR1–PR3): I re-read the full Grant/Revoke flows. The principal comes from principal.Id/grant.Principal.Id, there is no entitlement.Resource.ParentResourceId use, and the argument order is correct.
• Breaking changes (B/BP): none. entitlements.go is unchanged.
• Error codes (E2/E3): rejections use InvalidArgument/Unimplemented.
• Sync, logging, spans, JSON (S/L/T/J): not applicable, because no List/Entitlements/Grants path, log line, span or response struct changed.

There were no prior inline findings to audit, and the incremental diff was not partial.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

---

Reviewed commit: c9a4881aa19f

[github-actions]

No blocking issues found — see the full review report