Skip to content

Add scripted verification checklist for provisioning trigger - #71

Merged
BluegReeno merged 2 commits into
mainfrom
archon/task-archon-fix-github-issue-1784843602175
Jul 24, 2026
Merged

Add scripted verification checklist for provisioning trigger#71
BluegReeno merged 2 commits into
mainfrom
archon/task-archon-fix-github-issue-1784843602175

Conversation

@BluegReeno

Copy link
Copy Markdown
Owner

Summary

provision_workspace_membership() — the SECURITY DEFINER trigger on auth.users that
auto-provisions workspace_members rows on invite — has only ever been checked by ad hoc,
undocumented rolled-back transactions run one-off through Supabase MCP execute_sql each time
a related migration landed (PR #60, #61/#68, #62/#70). PR #60's review flagged this as finding
M5: there was no reusable, complete verification asset.

This PR adds one: a single self-asserting SQL script, scripts/verify_provisioning_trigger.sql,
covering the 6 scenarios named in the issue. Each scenario is an independent BEGIN … ROLLBACK
block using synthetic fixture users (never real prod data), ending with a PASS/FAIL
RAISE self-check instead of requiring the operator to eyeball table contents.

Changes

  • scripts/verify_provisioning_trigger.sql (new, +283 lines) — 6 numbered, independent,
    self-asserting scenarios, each wrapped in its own BEGIN … ROLLBACK (zero residue on prod
    regardless of outcome):
    1. Fresh INSERT with company_id present (happy path)
    2. company_id set only on a later UPDATE (the real auth-gateway 2-step path)
    3. is_default guard — user already holds a default membership elsewhere
    4. company_id resolves to no halcrm_workspaces row (RAISE WARNING, no abort, no row)
    5. Idempotent re-run — membership already exists, no error, no duplicate
    6. Routine last_sign_in_at update does not re-invoke the trigger
  • docs/operations.md (+8 lines) — new bullet under §1 "Migrations" pointing to the script
    and stating when to run it (after any change to provision_workspace_membership(),
    trg_provision_workspace_membership, workspace_members, or halcrm_workspaces.company_id).

No change to the trigger function, trigger definition, or any table schema — this is
verification-only.

Deviation from plan

Scenario 3 uses INSERT (empty meta) → seed the ic-ingenieurs-conseils default membership →
UPDATE company_id, rather than a single INSERT with company_id already set. A single
INSERT would fire the trigger before the pre-existing default membership is seeded (FK on
workspace_members.user_id requires the user to already exist), defeating the exact guard
scenario 3 targets. The INSERT-empty → seed → UPDATE sequence is the only FK-valid ordering that
exercises the invariant (new row must land is_default=false when a default already exists);
behavior under test is unchanged.

Validation

Check Result
grep -c 'ROLLBACK;' scripts/verify_provisioning_trigger.sql (expect 6) ✅ 6
grep -c 'COMMIT;' scripts/verify_provisioning_trigger.sql (expect 0) ✅ 0
grep -c 'verify_provisioning_trigger' docs/operations.md (expect 1) ✅ 1
make test (deno check ×2 + offline pytest) ✅ 157 passed, 17 deselected; both edge functions type-check clean
Live 6/6 PASS run against prod via Supabase MCP ⏸ Deferred — Supabase MCP is unauthenticated in this non-interactive session

The live prod run (Task 3 of the plan) is a post-merge step for Renaud, matching PR #60's
"committed only, verified separately" precedent — it requires interactive Supabase MCP access
to zgkvbjqlvebttbnkklpo and will be logged as a dated entry in docs/cross-repo-log.md once
run.

Scope limits

  • No pytest/Python test file — no psycopg2/asyncpg dependency exists in this repo, and adding
    one solely for a manual, ~monthly verification checklist would violate KISS/YAGNI. The
    existing Supabase MCP execute_sql path is the tool every prior verification already used.
  • No CI/automated nightly run of this script — that's the separate, not-yet-implemented issue
    CI — migration smoke-test against an ephemeral DB with the prod schema (catch 42P17-class errors before apply) #65 (ephemeral-DB migration smoke test).
  • No mirror in edifice for provision_edifice_org — per the 2026-07-23 cross-repo-log decision,
    not propagating trigger-adjacent tooling across repos without a concrete need.

Fixes #63

…ger (#63)

provision_workspace_membership() (the SECURITY DEFINER trigger auto-provisioning
workspace_members on invite) had only ever been checked by ad hoc, undocumented
rolled-back transactions (PR #60 review finding M5). Add one reusable, self-asserting asset.

Changes:
- Add scripts/verify_provisioning_trigger.sql: 6 numbered, independent BEGIN...ROLLBACK
  scenarios (happy path, UPDATE-after-INSERT, is_default guard, orphan company_id,
  idempotent re-run, routine last_sign_in_at no-op), each self-reporting PASS/FAIL via
  RAISE NOTICE/EXCEPTION on synthetic fixtures — zero residue on prod
- Link the script from docs/operations.md §1 with guidance on when to run it

Task 3 (live 6/6 PASS run against prod + cross-repo-log entry) is a post-merge step
requiring Supabase MCP access, matching PR #60's precedent.

Fixes #63

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@BluegReeno

Copy link
Copy Markdown
Owner Author

🔍 Comprehensive PR Review

PR: #71
Reviewed by: 3 of 5 planned agents (code-review, comment-quality, docs-impact) — error-handling and test-coverage agents did not produce artifacts and were not run; treat those two dimensions as unreviewed, not as passing.
Date: 2026-07-24


Summary

This PR adds a 6-scenario, self-asserting SQL verification checklist for the auth.users provisioning trigger, plus a one-bullet pointer in docs/operations.md. Code review independently traced all 6 scenarios against the live trigger logic and confirmed every scenario correctly tests the invariant it claims to, with clean BEGIN…ROLLBACK residue safety and no silent-pass paths. The only substantive issue is in the file's header comment, not its logic: it overclaims coverage of the #62 malformed-company_id cast guard (no scenario tests it) and mischaracterizes the is_default guard as a reproduced prod incident when issue #61 records it as latent/never reproduced. Both are resolved by a single comment rewrite — no behavior change needed.

Verdict: REQUEST_CHANGES (auto-fixable, comment-only)

Severity Count
🔴 CRITICAL 0
🟠 HIGH 1
🟡 MEDIUM 1
🟢 LOW 1

🟠 High Issue

Header comment claims coverage of the #62 cast-guard regression that no scenario tests

📍 scripts/verify_provisioning_trigger.sql:10-13

No scenario inserts a malformed (non-UUID) company_id. Scenario 4 uses gen_random_uuid(), which always passes the #62 WHEN-clause regex — it tests a different path (orphan-but-well-formed UUID, finding M3). A future engineer reading this header could believe the #62 cast guard is already regression-tested and skip writing the real check.

View fix (also resolves the MEDIUM issue below)
-- Covers one failure mode this trigger has already hit in prod — the 42P17 "WHEN cannot
-- reference OLD" class (guard now lives in the function body, fixed in commit f3920ab) —
-- plus the is_default partial-unique-index collision (latent per issue #61, never
-- reproduced; hardened via the #61 UNIQUE constraint) and the three non-firing /
-- idempotency paths. Does NOT cover #62 (the malformed-company_id cast guard / WHEN-clause
-- regex) — no scenario here exercises a non-UUID company_id string.

🟡 Medium Issue

"already hit" mischaracterizes the is_default collision as a reproduced incident

📍 scripts/verify_provisioning_trigger.sql:10

Issue #61 states "Priority: P2. Latent, not currently reproducible" — the guard was designed in from the trigger's first commit and hardened after a code-review finding, not an observed failure. The header's "already hit once each" wording blurs that distinction.

Options: Fix now (same edit as the HIGH issue above, single comment block) | Skip (leaves the mischaracterization)

Recommendation: Fix now — zero behavior change, one edit resolves both HIGH and MEDIUM findings.


🟢 Low Issue

View 1 low-priority suggestion
Issue Location Suggestion
README.md's scripts/ tree omits the new script README.md (~line 250) No action this PR — the tree already omits 3 pre-existing scripts; it's a curated list, not a maintained inventory.

✅ What's Good

  • All 6 scenarios independently verified against the live trigger/function SQL — no scenario asserts an outcome the real code wouldn't produce.
  • 6/6 BEGIN/ROLLBACK pairs, zero COMMIT, no DDL — respects the "execute_sql read/debug only" rule.
  • Fixture UUIDs are genuine halcrm_workspaces company IDs, not placeholders.
  • 1:1 mapping confirmed against issue P3 — Scripted verification checklist for the auth.users provisioning trigger #63's 6 named cases — no more, no less.
  • No pytest/psycopg2 dependency added — scope discipline matches KISS/YAGNI.

📋 Suggested Follow-up Issues

Issue Title Priority Related Finding
"Add a #62 malformed-company_id regression scenario to verify_provisioning_trigger.sql" P3 HIGH issue — the coverage gap itself (distinct from the comment fix) is optional scope creep beyond issue #63

Next Steps

  1. Apply the single header-comment rewrite above (resolves HIGH + MEDIUM in one edit).
  2. Decide on the optional P3 follow-up for an actual #62 regression scenario, or accept the corrected comment's disclaimer.
  3. Error handling and test coverage were not reviewed this pass — re-run those two agents before merge if sign-off against those dimensions is required.
  4. README.md LOW issue: no action needed this PR.

Reviewed by Archon comprehensive-pr-review workflow
Artifacts: /Users/renaud/.archon/workspaces/BluegReeno/hal/artifacts/runs/c8d0c92cee105a4242de9bfad97816a0/review/

Fixed:
- scripts/verify_provisioning_trigger.sql header overclaimed coverage
  of the #62 malformed-company_id cast guard (no scenario tests it)
  and mischaracterized the is_default collision as a reproduced prod
  incident (issue #61 records it as latent, never reproduced).

Skipped:
- README.md scripts/ tree omission of the new script — docs-impact
  agent explicitly recommends no action (tree already omits 3
  pre-existing scripts; piecemeal fix would just extend the gap).
@BluegReeno

Copy link
Copy Markdown
Owner Author

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-archon-fix-github-issue-1784843602175 (commit 97b64c6)
Philosophy: Fix everything unless clearly a new concern


Fixes Applied (2 total)

Severity Count
🔴 CRITICAL 0
🟠 HIGH 1
🟡 MEDIUM 1
🟢 LOW 0
View all fixes

Tests Added

(none — no test-coverage finding; this PR is itself the test-coverage deliverable for #63)


Skipped (1)

Finding Reason
README.md scripts/ tree omits the new script New concern: docs-impact agent itself recommended no action — tree already omits 3 pre-existing scripts, curated-not-exhaustive list; fixing piecemeal would extend the gap pattern rather than close it.

Suggested Follow-up Issues

  1. Add a P2 — Guard ::uuid cast in provisioning trigger against malformed company_id #62 malformed-company_id regression scenario to verify_provisioning_trigger.sql (P3) — closes the coverage gap the comment now correctly discloses instead of overclaiming.
  2. Sweep README.md scripts/ tree for all 4 undocumented scripts (P3) — one pass covering the new script plus 3 pre-existing gaps.

Validation

✅ Offline pytest (157 passed, 17 deselected) | ⚠️ Deno type-check not run (binary unavailable in this environment; no .ts file touched by this change) | ✅ Diff scope confirmed comment-only, no DDL/logic change


Self-fix by Archon · aggressive mode · fixes pushed to archon/task-archon-fix-github-issue-1784843602175

@BluegReeno
BluegReeno marked this pull request as ready for review July 24, 2026 05:26
@BluegReeno
BluegReeno merged commit 750a839 into main Jul 24, 2026
2 checks passed
BluegReeno added a commit that referenced this pull request Jul 24, 2026
…prod

Ran scripts/verify_provisioning_trigger.sql (PR #71, issue #63) via Supabase MCP
execute_sql against prod zgkvbjqlvebttbnkklpo, 6 blocks one at a time. All PASS,
0 residue after (every block rolls back). Closes the deferred live-verification
item from #71.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.

P3 — Scripted verification checklist for the auth.users provisioning trigger

1 participant