fix: use target database user for embedded postgres in plan command (#303)

[tianzhou]

Summary

• The embedded postgres in the plan command used a hardcoded Username: "pgschema", which caused ALTER DEFAULT PRIVILEGES FOR ROLE postgres to fail because the "postgres" role didn't exist in the embedded instance
• Without explicit FOR ROLE, default privileges were attributed to the "pgschema" user instead of the target database user (e.g., "postgres"), causing a role mismatch that generated incorrect migration plans (revoke from one role, grant to another)
• Fixed by using the target database user (config.User) as the embedded postgres username, ensuring role references match between desired and current state

Fixes #303

Test plan

• Added testdata/diff/default_privilege/issue_303_for_role/ test case with explicit FOR ROLE in ALTER DEFAULT PRIVILEGES
• All default_privilege diff tests pass: PGSCHEMA_TEST_FILTER="default_privilege/" go test -v ./internal/diff -run TestDiffFromFiles
• All default_privilege integration tests pass: PGSCHEMA_TEST_FILTER="default_privilege/" go test -v ./cmd -run TestPlanAndApply
• All privilege integration tests pass: PGSCHEMA_TEST_FILTER="privilege/" go test -v ./cmd -run TestPlanAndApply

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

This PR fixes a role-mismatch bug (#303) in the plan command where the embedded PostgreSQL instance used for desired-state validation was started with a hardcoded username "pgschema" instead of the target database user. This caused two classes of failures: ALTER DEFAULT PRIVILEGES FOR ROLE <target-user> would error because that role didn't exist in the embedded instance, and even without an explicit FOR ROLE, default privileges were attributed to "pgschema" rather than the target user, generating spurious REVOKE/GRANT DDL in the migration plan.

Changes:

• cmd/plan/plan.go — CreateEmbeddedPostgresForPlan now sets Username: config.User instead of "pgschema", ensuring the embedded postgres superuser matches the target database user.
• testdata/diff/default_privilege/issue_303_for_role/ — New diff test case with ALTER DEFAULT PRIVILEGES FOR ROLE testuser to validate the IR-level diff correctly generates the expected migration SQL.
• .claude/skills/*.md — Documentation-only simplifications of the internal workflow skill files (no code impact).

How the fix fits the codebase: The TestPlanAndApply integration test infrastructure always sets up its "target" embedded postgres with Username: "testuser" (via testutil.SetupPostgres), so config.User is always "testuser" during integration tests — confirming the new test case will correctly exercise the scenario end-to-end.

Confidence Score: 4/5

• This PR is safe to merge — the fix is minimal, well-targeted, and covered by both diff and integration tests.
• The one-line change in CreateEmbeddedPostgresForPlan is clearly correct, the comment explains the rationale well, and the test case directly exercises the reported bug scenario. A minor concern is that the exported function has no guard against config.User being empty in programmatic callers outside the CLI path, which could silently break the role-matching invariant introduced by this fix.
• cmd/plan/plan.go — the CreateEmbeddedPostgresForPlan function would benefit from a defensive empty-username check.

Important Files Changed

Filename	Overview
cmd/plan/plan.go	Core fix: changes embedded postgres Username from hardcoded "pgschema" to config.User, ensuring role names match between desired and current state; no guard against empty config.User in programmatic use.
testdata/diff/default_privilege/issue_303_for_role/new.sql	Adds desired state SQL with ALTER DEFAULT PRIVILEGES FOR ROLE testuser; correctly relies on testuser being the embedded postgres superuser (now guaranteed by the fix), but doesn't explicitly create that role.
testdata/diff/default_privilege/issue_303_for_role/diff.sql	Expected diff output correctly shows the ALTER DEFAULT PRIVILEGES statement for testuser granting all privileges on tables to demouser.
testdata/diff/default_privilege/issue_303_for_role/plan.json	Expected JSON plan output with correct type=default_privilege, operation=create, and matching SQL for the ALTER DEFAULT PRIVILEGES step.
.claude/skills/fix_bug/SKILL.md	Documentation-only simplification: condenses the fix_bug skill from verbose step-by-step instructions into a more concise reference format; no code changes.

Sequence Diagram

sequenceDiagram
    participant User
    participant PlanCmd as plan command
    participant TargetDB as Target Database
    participant EmbeddedPG as Embedded PostgreSQL

    User->>PlanCmd: pgschema plan --user postgres --db mydb
    PlanCmd->>TargetDB: Detect PostgreSQL version
    TargetDB-->>PlanCmd: version (e.g. 17.x)
    Note over PlanCmd,EmbeddedPG: BEFORE fix: Username hardcoded to "pgschema"<br/>AFTER fix: Username = config.User (e.g. "postgres")
    PlanCmd->>EmbeddedPG: Start (Username=config.User)
    PlanCmd->>EmbeddedPG: Apply new.sql (desired state)
    Note over EmbeddedPG: ALTER DEFAULT PRIVILEGES FOR ROLE postgres<br/>Now works — "postgres" role exists as superuser
    PlanCmd->>EmbeddedPG: Inspect desired state IR
    EmbeddedPG-->>PlanCmd: desiredStateIR (roles match target DB)
    PlanCmd->>TargetDB: Inspect current state IR
    TargetDB-->>PlanCmd: currentStateIR
    PlanCmd->>PlanCmd: diff(currentStateIR, desiredStateIR)
    Note over PlanCmd: Role names match — no spurious REVOKE/GRANT generated
    PlanCmd-->>User: Correct migration plan

Loading

_{Last reviewed commit: 9c7288e}

[greptile-apps]

No guard against empty config.User in programmatic use

CreateEmbeddedPostgresForPlan is an exported function, and config.User is passed directly as the embedded postgres Username. In the CLI path this is safe because PreRunE validates the --user flag. However, if this function is ever called programmatically with an empty config.User, the embedded-postgres library will either fail to start or silently fall back to a default username — breaking the role-matching invariant this fix establishes.

Consider adding a defensive guard before constructing embeddedConfig:

if config.User == "" {
    return nil, fmt.Errorf("target database user must not be empty when creating embedded postgres")
}

[tianzhou]

Added in 9937ab6.

[Copilot]

Pull request overview

Updates the plan command’s embedded PostgreSQL setup to use the target database user so role ownership/default-privilege semantics match between desired/current state, addressing issue #303 around ALTER DEFAULT PRIVILEGES ... FOR ROLE ... when planning with embedded Postgres.

Changes:

• Use config.User (target DB user) as the embedded Postgres superuser for plan.
• Add a new default_privilege/issue_303_for_role fixture exercising explicit FOR ROLE.
• Substantially rewrite/remove several .claude/skills/* docs (unrelated to the bug fix).

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
cmd/plan/plan.go	Uses target DB user for embedded Postgres in plan to align role references/ownership.
testdata/diff/default_privilege/issue_303_for_role/old.sql	Test fixture: initial state (no default privileges) and ensures demouser exists.
testdata/diff/default_privilege/issue_303_for_role/new.sql	Test fixture: desired state includes ALTER DEFAULT PRIVILEGES ... FOR ROLE testuser ....
testdata/diff/default_privilege/issue_303_for_role/diff.sql	Expected diff SQL for the new fixture.
testdata/diff/default_privilege/issue_303_for_role/plan.sql	Expected plan SQL output for the new fixture.
testdata/diff/default_privilege/issue_303_for_role/plan.json	Expected plan JSON output for the new fixture.
testdata/diff/default_privilege/issue_303_for_role/plan.txt	Expected human-readable plan output for the new fixture.
.claude/skills/validate_db/SKILL.md	Large rewrite of skill documentation.
.claude/skills/run_tests/SKILL.md	Large rewrite of skill documentation.
.claude/skills/postgres_syntax/SKILL.md	Large rewrite of skill documentation.
.claude/skills/pg_dump/SKILL.md	Large rewrite of skill documentation.
.claude/skills/fix_bug/SKILL.md	Large rewrite of skill documentation.
.claude/skills/refactor-pass/SKILL.md	Deletes the refactor-pass skill doc.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

The behavior change (embedded DB username now derived from config.User) isn’t directly covered by a regression test that would have failed before this fix. Consider adding an integration test for plan that uses a desired-state file containing ALTER DEFAULT PRIVILEGES FOR ROLE <target user> ... (and creates any required roles), and asserts the plan command succeeds; this would fail when the embedded username was hardcoded to pgschema and pass with this change.

Suggested change

	Password: "pgschema",
	Password: config.Password,

[tianzhou]

The password suggestion is incorrect. The embedded postgres is a fresh local instance — its password is independent of the target database. The username matters because PostgreSQL uses it for role ownership attribution (pg_get_userbyid(defaclrole)). The password has no semantic meaning here; it is just auth to a throwaway instance.

Regarding test coverage: valid observation. The integration test uses sharedEmbeddedPG as the provider (bypassing CreateEmbeddedPostgresForPlan), so it would not have caught the hardcoded username directly. However, a true end-to-end test spinning up a separate embedded PG with a mismatched user would be complex and slow. The diff test verifies FOR ROLE works correctly at the IR level.

[Copilot]

This PR includes a large, unrelated rewrite of the .claude/skills/* documentation (including deleting a skill file). Since the PR description is focused on the embedded-postgres username fix and a regression test for issue #303, consider splitting these doc/tooling changes into a separate PR to keep the change set minimal and easier to review/revert if needed.

[tianzhou]

The skill documentation changes are from a prior commit on main (04a882d7) already merged. They are not part of this branch — likely a git diff artifact.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This test uses GRANT ALL ON TABLES, which expands to a version-dependent privilege list (e.g., MAINTAIN is present in newer PostgreSQL versions but not older ones). Since CI runs the test suite across PG 14–18, the expected outputs in this fixture will likely fail on older versions. Prefer granting an explicit, version-stable subset of privileges (e.g., SELECT), and update the expected diff.sql / plan.* files accordingly (or add version-specific expectations/skips).

Suggested change

	ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT ALL ON TABLES TO demouser;
	ALTER DEFAULT PRIVILEGES FOR ROLE testuser IN SCHEMA public GRANT SELECT ON TABLES TO demouser;

[tianzhou]

Fixed in 691d56d. Changed to GRANT SELECT for cross-version stability.

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated no new comments.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.