[erikdarlingdata]

Claude posting for Erik Darling.

Dev's store script for bring-your-own stores stops at step 8. This PR fixes the list that step reads.

What broke

Step 8 of Darling/tools/provision-roles.sql came in with #6613. It hands each definer-rights function to the owner of config.config_command. It names each function by its signature.

#6593 added the actor to config.update_notification_route and config.set_notification_route_destinations, and dropped their old signatures. Each PR passed CI on its own. They merged in that order, and no CI run had both. Now step 8 asks for the old signature, and the script fails with 42883: function config.update_notification_route(integer, text[], text, boolean, integer, integer, boolean, integer) does not exist.

Every PR that runs the Darling PostgreSQL tests now fails the bring-your-own-script tests: ExternalAlertGrantLiveTests.TheByoScript_*, ComposeStoreRolesLiveTests, PhaseCStoreGrantLiveTests, InvestigationPracticeGrantLiveTests, AgentLevel2StoreGrantLiveTests and QueryTagStoreGrantLiveTests(managed: False). The managed-role path does not use this step, so its tests pass.

The change

• The list names the 9-argument update_notification_route signature.
• The list names the 8-argument set_notification_route_destinations signature. Darling MCP: an operator token sets route agent fields, part 2 of #6542 #6593 added the actor to it too. The first CI run on this PR found it.
• The list also names config.set_notification_route_agent, the definer-rights function Darling MCP: an operator token sets route agent fields, part 2 of #6542 #6593 added. Step 8's comment says it hands each one to the table owner, so the new function belongs on the list.

Every entry in the list was then checked against the script's own definitions. Each one names a function that the script creates at the top level and does not drop before step 8. So to_regprocedure finds each one on every store.

Tests

No new test. The failing tests above are the proof: they fail on dev now, and they should pass on this PR.

Considered, not added

config.investigation_request_alerts_owner_rows and config.investigation_journal_owner_rows are not on the list. They run as the caller, not the owner, because their check uses current_user. So their owner does not change what they can do, and step 8 has nothing to hand over.

CHANGELOG

No entry. Step 8 has not shipped in a release.

[claude]

The next entry is stale in exactly the same way, so step 8 still fails. set_notification_route_destinations also took an actor argument in #6542 part 2: this script drops the 7-argument form at line 1736, creates the 8-argument (integer, text, text[], text, text, text, text, text) at 1737, and REVOKEs/GRANTs that 8-argument form at 1823 and 1826. The 7-argument signature named here therefore does not exist at the moment step 8 runs — on a fresh store it never existed, and on an upgraded store line 1736 has just dropped it.

What that costs is the same failure the PR body describes, one array element later, and it is worse than "the last three functions keep their owner": to_regprocedure answers NULL, the subselect over pg_proc yields NULL, NULL IS DISTINCT FROM <relowner> is TRUE, so line 3021 executes ALTER FUNCTION config.set_notification_route_destinations(integer, text[], text, text, text, text, text) OWNER TO … and raises 42883. The DO block is one statement, so the error rolls back every owner change the loop already made — the fixup becomes a no-op and all fifteen definer-rights functions stay owned by whoever ran the script, which on the stores this step exists for is a superuser. ComposeStoreRolesLiveTests.TheBringYourOwnScript_GivesTheSettingsLoginsExactlyTheManagedGrants_Gated and the other tests that run the shipped script through one NpgsqlCommand will still throw.

Suggested change

	'config.set_notification_route_destinations(integer, text[], text, text, text, text, text)',
	'config.set_notification_route_destinations(integer, text, text[], text, text, text, text, text)',

Worth considering alongside the fix: line 3019's guard treats "the function is absent" as "the owner differs", which is what turns any future signature drift in this list into a hard script failure instead of a skipped entry. Adding pg_catalog.to_regprocedure(v_sig) IS NOT NULL AND to that condition would make the list self-healing for a legitimately absent overload — the three edit_monitored_server arities are the shape most likely to lose one.

[claude]

Verdict: CHANGES REQUESTED

The mechanism the body describes is real and the two lines it changes are correct, but the list is one entry short: #6542 part 2 gave an actor argument to three route functions in this script, and set_notification_route_destinations at line 3014 still names the 7-argument form that line 1736 of the same file drops. Step 8 still raises 42883, one array element later — and because the fixup is a single DO block, that error rolls back every owner change the loop already made, so all fifteen definer-rights functions keep the running role's ownership (a superuser on the stores this step exists for). The tests the body offers as proof will still throw. The one-line fix is in the inline comment.

Claims

• VERIFIED — step 8 hands each definer-rights function to the owner of config.config_command, naming each by signature — provision-roles.sql:2988-3024; the roster is complete at fifteen, matching the fifteen SECURITY DEFINER functions the script creates (949, 1195, 1229, 1257, 1288, 1310, 1336, 1402, 1470, 1525, 1635, 1748, 1839, 1935, 2155).
• VERIFIED — #6593 dropped the 8-argument update_notification_route and created a 9-argument one taking the actor — dropped at :1512, created at :1513, revoked at :1614.
• VERIFIED — the old signature makes the script fail 42883 at step 8 — :3019 reads proowner via to_regprocedure(v_sig), which answers NULL for an absent function, so NULL IS DISTINCT FROM <relowner> is TRUE and :3021 runs ALTER FUNCTION <absent signature> OWNER TO ....
• VERIFIED — the six named test classes run the bring-your-own script — ComposeStoreRolesLiveTests.cs:543, ExternalAlertGrantLiveTests.cs:856, PhaseCStoreGrantLiveTests (PhaseCStoreTests.cs:266, exec :429), InvestigationPracticeGrantLiveTests (InvestigationPracticeTests.cs:569, exec :688), AgentLevel2StoreGrantLiveTests (AgentLevel2StoreTests.cs:331, exec :462), QueryTagStoreGrantLiveTests (QueryTagStoreLiveTests.cs:359, exec :426) — each reads Fixtures/provision-roles.sql (copied by Darling.Tests.csproj:113) and sends the whole text through one NpgsqlCommand, which throws on the step-8 error.
• VERIFIED — the managed path does not use this step — OWNER TO on a function appears nowhere but provision-roles.sql:3021; DarlingManagedRoles.BuildNotificationRouteFunctionsSql creates the functions as the owner already.
• VERIFIED — the list now names the 9-argument update_notification_route — :3012 matches the revoke at :1614 and the grant at :1825 character for character.
• VERIFIED — the list now names set_notification_route_agent, a definer-rights function #6593 added — :3013 matches :1733 and :1827; SECURITY DEFINER at :1635.
• VERIFIED — both functions are created at the top level, so to_regprocedure finds them — :1513 and :1621 are top-level CREATE OR REPLACE, not inside a DO or an EXECUTE.
• VERIFIED — the two investigation_*_owner_rows functions run as the caller, so step 8 has nothing to hand over — :2702 and :2749 declare no SECURITY DEFINER and gate on pg_has_role(current_user, ...).
• UNVERIFIED — step 8 arrived with #6613, #6593 merged first, and no CI run had both — this checkout is one squashed merge commit, so neither merge order nor CI history is readable here.
• UNVERIFIED — the named tests "fail on dev now" — all six are gated on a live PostgreSQL rig (RequireLivePostgres / RequireRuntime, plus Assert.SkipUnless on a cluster-wide darling role at ExternalAlertGrantLiveTests.cs:846), which a read-only tree cannot stand up.
• UNVERIFIED — no CHANGELOG entry because step 8 has not shipped in a release — needs the release tags; CHANGELOG.md and docs/changelog/*.md name neither #6613 nor #6593, which is consistent with the claim but does not establish it.
• REFUTED — "they fail on dev now, and they should pass on this PR" — they will not pass. :3014 still names set_notification_route_destinations(integer, text[], text, text, text, text, text), the exact signature :1736 drops; the live function is the 8-argument form created at :1737 and revoked/granted at :1823/:1826. The loop reaches :3014 right after the two entries this PR fixed and raises the same 42883.

Riskiest lines

1. Darling/tools/provision-roles.sql:3014 — the remaining stale signature; it aborts the one DO block, rolling back the owner change for all fifteen functions, so definer-rights functions granted to mcp keep superuser ownership — covered by ComposeStoreRolesLiveTests.TheBringYourOwnScript_GivesTheSettingsLoginsExactlyTheManagedGrants_Gated (sends the shipped script through one NpgsqlCommand; throws on the 42883, passes with the inline fix), though only where the live rig runs.
2. Darling/tools/provision-roles.sql:3012 — must match :1614 in all nine type names and their order; one wrong type re-raises the same 42883 with no other symptom — covered by ComposeStoreRolesLiveTests.TheBringYourOwnScript_GivesTheSettingsLoginsExactlyTheManagedGrants_Gated.
3. Darling/tools/provision-roles.sql:3013 — the added eleven-argument entry matches :1733, but it also makes the block depend on that function existing, and :3019's guard cannot tell "absent" from "owned by someone else" — covered by ComposeStoreRolesLiveTests.TheBringYourOwnScript_GivesTheSettingsLoginsExactlyTheManagedGrants_Gated.

Traps near this diff

• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies — the step-8 list hand-copies fifteen signatures already spelled at :1169, :1205, :1239, :1274, :1306, :1321, :1391, :1450, :1510, :1614, :1733, :1823, :1915, :1944, :2229 of the same file; this PR is that drift arriving and :3014 is the copy that drifted and stayed. A list built from pg_proc (every SECURITY DEFINER function in config whose owner differs) would have no drift to correct.
• "Lite and Darling ship in parity ... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding" — does not apply — this script provisions the PostgreSQL store and Lite has no PostgreSQL store (PgMigrations.cs:2686); the managed counterpart already carries the current signatures at DarlingManagedRoles.cs:2850, :2969, :3059, and has no ownership step to mirror.
• "Our wrong test is a bug we introduced, never a flake ... a line in .github/ci-quarantine.txt only while that fix lands" — applies, and is honored — the PR fixes the cause and adds no quarantine line, retry or skip; the only gap is that the fix stops one entry short.
• "Ruling (#5833): tests are load-bearing ... never answer a trap with a test that reads source text" — applies to the grading above — the static pins over this file (NotificationRoutesRungTests.cs:204, PagerDutyAutoResolveRungTests.cs:180, PasswordKeyGrantParityTests.cs:28, ProvisionRolesAclDriftTests.cs:22) are not counted as coverage; the script-executing live tests are, and the body is right to lean on them.
• ".gitattributes says text=auto eol=crlf ... a diff that touches every line of a file is a line-ending conversion until proven otherwise" — does not apply — the diff is one deletion and two insertions in one hunk (+2 -1).

Divergence (blind read vs PR body)

The title and "The change" scope #6593's damage to "the two route functions". #6593 (as #6542 part 2) changed three signatures in this script: update_notification_route (:1512), set_notification_route_agent (:1620) and set_notification_route_destinations (:1736) — the script's own comment at :1734-1735 says so ("the actor is an argument too, so the old signature is dropped and stays uncallable"). The list carries one entry per changed signature, so a two-function fix cannot clear a three-function drift, and the body's closing claim that the named tests should now pass follows from that undercount rather than from the tree. Everything else matches what I read, including the reasoning for leaving the two investigation_*_owner_rows trigger functions off the list.

Reviewer checklist

• On a live rig, run the shipped script end-to-end on a scratch store as the store owner with ON_ERROR_STOP on, confirm step 8 raises nothing, then assert pg_get_userbyid(proowner) equals config.config_command's owner for all fifteen signatures — the outcome this step claims and that the current failure silently rolls back.
• Confirm the six gated suites execute rather than skip in the Darling PostgreSQL CI job (they need a live cluster and a cluster-wide darling role): if they skip, this class of drift reaches dev again with CI green, which is how the present failure arrived.

[claude]

Follow-up pass on cf485d477..01c042845 (one commit, one file, +2/-1).

The commit fixes exactly the gap the prior full review flagged: provision-roles.sql:3014 named config.set_notification_route_destinations by its old 7-argument signature (integer, text[], text, text, text, text, text), while the live function (dropped/recreated at :1736-1737 in the same script, and at DarlingManagedRoles.cs:2972-2981 in the managed path) takes 8 arguments with p_actor text as the second parameter. The new string now reads (integer, text, text[], text, text, text, text, text), which matches the CREATE OR REPLACE FUNCTION at :1737, the REVOKE/GRANT pair at :1823/:1826, and DarlingManagedRoles.cs:2973-2981 character for character.

I re-checked the PR body's claim that "every entry in the list was then checked against the script's own definitions" by walking all 15 step-8 signatures against their CREATE OR REPLACE FUNCTION statements in the file (928, 1175, 1211, 1246, 1276, 1307, 1330, 1397, 1461, 1513, 1621, 1737, 1832, 1922, 2145) — each name, argument count and order lines up, including the two edit_monitored_server overloads. No stale signature remains in the list.

No Lite counterpart needed: Lite has no PostgreSQL store to provision, so the Lite/Darling parity rule doesn't apply to this script.

[claude]

Verdict: LGTM

Claims

• VERIFIED — "The list names the 8-argument set_notification_route_destinations signature. #6593 added the actor to it too." — provision-roles.sql:3014 now reads (integer, text, text[], text, text, text, text, text), matching the CREATE OR REPLACE FUNCTION at :1737, the DROP FUNCTION IF EXISTS of the old signature at :1736, the REVOKE/GRANT pair at :1823/:1826, and DarlingManagedRoles.cs:2972-2981.
• VERIFIED — "Every entry in the list was then checked against the script's own definitions. Each one names a function that the script creates at the top level and does not drop before step 8." — walked all 15 signatures in the step-8 array against their CREATE OR REPLACE FUNCTION statements at :928, 1175, 1211, 1246, 1276, 1307, 1330, 1397, 1461, 1513, 1621, 1737, 1832, 1922, 2145; each name, argument count and order matches, including both edit_monitored_server overloads (:1175, :1211) and set_notification_route_agent (:1621).
• UNVERIFIED — "they fail on dev now, and they should pass on this PR" — the cited tests (ComposeStoreRolesLiveTests, ExternalAlertGrantLiveTests, etc.) are gated on a live PostgreSQL rig; a read-only tree cannot execute them to confirm the pass.

Riskiest lines

1. Darling/tools/provision-roles.sql:3014 — the corrected signature; to_regprocedure(v_sig) at :3019 returns NULL on any mismatch (missing arg, wrong type, wrong order), which raises 42883 and rolls back the whole owner-handoff DO block for all 15 functions — covered by ComposeStoreRolesLiveTests.TheBringYourOwnScript_GivesTheSettingsLoginsExactlyTheManagedGrants_Gated, gated on a live rig.

Traps near this diff

• "Two copies of one definition pinned EQUAL to each other fail only after BOTH have drifted (#3684). Alias, do not copy." — applies — the step-8 list hand-copies signatures already spelled earlier in the same file; this commit closes the one remaining drifted copy (set_notification_route_destinations) that the prior full review found at :3014, leaving no outstanding instance.
• "Lite and Darling ship in parity ... a fix to one SKU's reader, tool, alert or description without the counterpart's is a finding" — does not apply — this script provisions the PostgreSQL store; Lite has no PostgreSQL store to provision, and the managed path (DarlingManagedRoles.cs) already carries the correct 8-argument signature with no ownership step to mirror.

Divergence (blind read vs PR body)

None — the body's account of this commit (fixing the one remaining stale signature, found by CI on this PR) matches the diff and the verification above.

Reviewer checklist

• On a live rig, run the shipped script end-to-end as the store owner and confirm step 8 raises nothing, then confirm the six gated bring-your-own-script test suites pass rather than skip.

Claude posting for Erik Darling. Stale review at cf485d4. Its one finding (set_notification_route_destinations still named by its 7-argument signature) is fixed in 01c0428. The re-review 5630680658 at 01c0428 is LGTM, and CI is green there.