GPT Review — 23-P3C1 Safe Functions Prompt rev2

Date: 2026-05-07
Reviewer: GPT-5.5 Thinking / Incomex Hội đồng AI
Reviewed: knowledge/dev/laws/dieu44-trien-khai/prompts/23-p3c1-iu-edit-draft-safe-functions-prompt.md rev2

Verdict

Rev2 is a major improvement but NOT ready to dispatch. Rev3 required.

Opus did the right shift from prose to near-complete SQL bodies. The P3C1 split, safe comment behavior, exact signatures, and UX philosophy are accepted. However, rev2 still contains several concrete SQL/runtime issues that would likely fail at execution.

Accepted rev2 improvements

Dedicated P3C1 path is used.
Near-complete SQL bodies are provided.
Exact function signatures are specified.
fn_iu_comment natural UX is safe: no silent latest when multiple drafts.
Status vocabulary is clearer.
STABLE/VOLATILE and SECURITY DEFINER are specified.
REVOKE/GRANT uses exact signatures.
Tests include no-silent-wrong-comment and address mismatch.
P3C1 excludes IU/UV writes by design.

Required rev3 fixes

P1 — BLOCKER: GRANT DO block uses psql variable inside dollar-quoted PL/pgSQL

Rev2 contains:

DO $$
DECLARE v_role text := current_setting('app.p3b_owner', true);
BEGIN
  IF v_role IS NULL OR v_role='' THEN v_role := :'fn_owner'; END IF;
  ...
END;
$$;

:'fn_owner' is not substituted inside a dollar-quoted string. This will fail.

Patch by setting a GUC before the DO block in the same transaction:

SELECT set_config('app.p3c1_fn_owner', :'fn_owner', true);
DO $$
DECLARE v_role text := current_setting('app.p3c1_fn_owner');
BEGIN
  EXECUTE format('GRANT EXECUTE ON FUNCTION ... TO %I', v_role);
END;
$$;

Or avoid DO and issue static GRANT only if role is safe. Preferred: GUC + DO with format('%I').

P2 — BLOCKER: `RETURNING id INTO v_result` where `v_result` is jsonb

In fn_iu_comment, rev2 declares:

v_result jsonb;
...
INSERT ... RETURNING id INTO v_result;

This is wrong type. Use v_comment_id uuid.

Patch:

DECLARE ... v_comment_id uuid;
...
INSERT ... RETURNING id INTO v_comment_id;
RETURN jsonb_build_object('comment_id', v_comment_id, ...);

Do not re-query latest comment to infer the ID.

P3 — BLOCKER: `v_ctx_draft_id::uuid` can throw before returning JSON

fn_iu_comment casts:

WHERE id=v_ctx_draft_id::uuid

If context contains a bad UUID string, the function raises an exception instead of returning self-guiding JSON.

Patch with validation:

BEGIN
  v_target_draft_id := v_ctx_draft_id::uuid;
EXCEPTION WHEN invalid_text_representation THEN
  RETURN jsonb_build_object('status','invalid_input','field','context.draft_id',...);
END;

Then query by v_target_draft_id.

P4 — `gen_random_uuid()` may require extension visibility; preflight should verify

P3B tables use gen_random_uuid() default and tests use gen_random_uuid(). Add preflight:

SELECT to_regprocedure('public.gen_random_uuid()') OR to_regprocedure('pg_catalog.gen_random_uuid()') ...

Or avoid random UUID in tests by selecting a hardcoded impossible UUID literal.

Simpler: replace T8 random call with fixed UUID:

'00000000-0000-0000-0000-000000000001'::uuid

P5 — Function source safety test should account for comments / false positives

T21 checks source does not contain UPDATE information_unit / INSERT INTO unit_version. Good. Also check public.information_unit SET and case-insensitive patterns.

But do not fail because source contains the phrase in a comment or in report text. Restrict to pg_proc.prosrc of the four new functions and use case-insensitive regex.

Example:

SELECT proname
FROM pg_proc p JOIN pg_namespace n ON ...
WHERE proname IN (...)
  AND p.prosrc ~* '(app\.canonical_writer|insert\s+into\s+(public\.)?unit_version|update\s+(public\.)?information_unit)';

Expected 0 rows.

P6 — Test transaction model is unclear: create functions commits before tests

Rev2 creates functions in a transaction and commits, then tests. If tests fail, it drops functions but retains test rows. This is acceptable if explicitly designed, but it is not “all-or-clean.”

Choose one and state clearly:

Option A: single transaction create+tests+commit. Harder because tests need shell-captured IDs.
Option B: create functions commit, tests run, if tests fail drop functions but retain test rows for debug. This is what rev2 does.

GPT accepts Option B for P3C1, but rev3 must report it honestly:

cleanup_on_test_fail=drop_functions_only
test_rows_retained_on_fail=true

Also if cleanup function drop fails, report phase_status=CRITICAL.

P7 — Cleanup DROP FUNCTION lacks exact signatures

Current cleanup:

DROP FUNCTION IF EXISTS public.$FN CASCADE;

This is unsafe and may fail/affect overloads. Use exact signatures and avoid CASCADE unless necessary.

DROP FUNCTION IF EXISTS public.fn_iu_comment(text,text,text,text,text,jsonb);
DROP FUNCTION IF EXISTS public.fn_iu_comment_edit_draft(uuid,text,text,text,text);
DROP FUNCTION IF EXISTS public.fn_iu_create_edit_draft(text,text,text,text,text);
DROP FUNCTION IF EXISTS public.fn_iu_edit_plan(text,text,text);

No CASCADE unless preflight proves no dependents and it is required. Prefer no CASCADE.

P8 — `fn_iu_comment` candidate list should include hash preview as required

Rev2 candidate list includes reason/title but not draft_content_hash preview. Add:

'hash_preview', left(d.draft_content_hash, 16)

Still no full body leak.

P9 — `TEST_ADDR_A` selection uses `created_at`, but IU may not have `created_at`

Earlier reviews removed reliance on created_at. P3B report detail did not prove information_unit.created_at exists.

Patch test address selection:

SELECT canonical_address FROM information_unit ORDER BY canonical_address LIMIT 1;

and B similarly.

P10 — Shell substitution of body text into SQL is unsafe for multiline/quotes

Tests embed $SAME_BODY directly into SQL literals. If body contains quotes/newlines, SQL breaks.

Patch tests to avoid embedding current body via shell literal:

For same-body tests, query body inside SQL by address:

WITH cur AS (... SELECT uv.body ...),
r AS (SELECT public.fn_iu_edit_plan(:'addr', cur.body, 'tester') AS j FROM cur)
SELECT j->>'status' FROM r;

Use psql variables with :'addr' for address, not raw shell interpolation.

P11 — Tests are still a matrix, not executable enough

Rev2 lists test SQL examples but does not provide a runnable test harness that sets TEST_FAIL, captures draft IDs, and writes UX samples.

Patch rev3 to add either:

a concrete bash test harness for T1–T21; or
explicit instruction that Agent must implement the harness and exact required assertions.

Given our effort to avoid improvisation, prefer concrete harness for the key tests at least T1–T15.

P12 — `fn_iu_edit_plan` leaks full content hash; acceptable, but preview policy should be consistent

Earlier plan functions used hash preview for privacy. Here full base_content_hash and draft_content_hash are returned. This may be acceptable for internal Agent tooling, but state policy.

GPT recommendation: return full hash only if needed for draft creation? P3C1 can return full hash because draft table stores it and agents are internal, but candidate lists should use preview only. Add comment to design/report.

P13 — `fn_iu_create_edit_draft` no-change with title change logic is too weak

Current logic:

IF v_hash = v_uv.content_hash AND p_title IS NULL THEN no_change;

If p_title is non-null but equal to current title, it still creates a draft with no actual change. Patch:

compare p_title to current identity_profile->>'title' when p_title is provided;
no_change if body hash same AND title is NULL or same as current title.

P14 — `fn_iu_edit_plan` does not account for title change

P3C1 exact signature for plan currently lacks title/reason. If edit draft supports title, plan should either:

accept optional p_title text DEFAULT NULL; or
explicitly state P3C1 plan is body-only and title planning is deferred.

Given create draft supports title, revise signature to include optional title:

fn_iu_edit_plan(p_address text, p_body text, p_actor text, p_title text DEFAULT NULL) RETURNS jsonb

Update REVOKE/GRANT/tests accordingly.

P15 — Function owner grants should follow execute role, not necessarily owner

Rev2 grants to FN_OWNER. Pack 22 reports often say owner directus and directus executes; that is okay. But if an explicit grantee exists in Pack 22 routine privileges, prefer matching the non-PUBLIC grantee(s), not only owner.

Patch preflight:

capture non-PUBLIC EXECUTE grantees for fn_iu_create;
if none, use owner;
grant new functions to the same grantee set.

If implementing multiple grants is too much for rev3, at least log and justify owner-only.

P16 — Low-level comment should include guidance or next_action?

comment_added success does not need guidance, but draft_not_found and invalid statuses do. Confirm all non-success branches in all four functions include both guidance and next_action.

P17 — P3C2 function existence gate may block rerun after P3C2 later

Rev2 P3C1 preflight fails if fn_iu_apply_edit_draft or fn_iu_edit exists. That is fine for first run, but it makes re-running P3C1 after P3C2 impossible.

For this prompt, fresh-create is acceptable. Add wording:

P3C1 is a one-time create pack; if P3C2 already exists, STOP to avoid out-of-order repair. This is intentional.

Directive to Opus

Patch P3C1 prompt to rev3 with P1–P17.

Path:

knowledge/dev/laws/dieu44-trien-khai/prompts/23-p3c1-iu-edit-draft-safe-functions-prompt.md

Do not dispatch after patch. Return for GPT/User review.

Hard boundaries remain

No dispatch.
No table DDL.
No schema changes.
No trigger changes.
No gateway changes.
No IU/UV writes.
No vector mutation.
No cleanup.
No Pack 2C.

Summary

Rev2 is the right shape but contains concrete SQL blockers: psql variable inside dollar-quoted DO, uuid cast exceptions, wrong variable type on RETURNING, unsafe cleanup signatures, and fragile test SQL interpolation. Rev3 should fix these and make the test harness more executable.