Skip to content

fix(db): guard ::uuid cast in provisioning trigger against malformed company_id - #66

Closed
BluegReeno wants to merge 1 commit into
mainfrom
archon/task-fix-issue-62
Closed

fix(db): guard ::uuid cast in provisioning trigger against malformed company_id#66
BluegReeno wants to merge 1 commit into
mainfrom
archon/task-fix-issue-62

Conversation

@BluegReeno

Copy link
Copy Markdown
Owner

Summary

provision_workspace_membership() (PR #60, supabase/migrations/20260719000000_provision_workspace_membership.sql) casts raw_app_meta_data->>'company_id' to uuid unconditionally in its DECLARE section. The trigger's WHEN clause only checked the value was non-empty text, not that it was a syntactically valid UUID — a malformed company_id raised Postgres 22P02 before BEGIN, aborting the whole auth.users write (breaks the invite/signup path).

This PR ships a fix-forward migration that adds a UUID-format regex check to the trigger's WHEN clause. A malformed value now fails WHEN, the function never runs, the unguarded ::uuid cast is never evaluated, and the auth.users write commits normally (provisioning silently skipped for that row — same "skip, never abort" contract already used elsewhere in this function).

Changes

  • supabase/migrations/20260722195616_guard_company_id_uuid_cast.sql (new): DROP TRIGGER / CREATE TRIGGER only — the trigger's WHEN clause now also regex-validates the UUID format (^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$, case-insensitive via ~*) before the existing non-empty check. The function body (public.provision_workspace_membership()) is untouched — no CREATE OR REPLACE FUNCTION.
  • docs/cross-repo-log.md: dated entry documenting the fix, "Impact on edifice: None" for this repo's own trigger, plus a flag for a newly discovered companion issue (see below).
  • .claude/STATUS.md: moved #62 from open follow-ups to Done.

WHEN only references NEW (never OLD), so this cannot regress the 42P17 guard already fixed once in this file (commit f3920ab) — that guard stays in the function body, gated on TG_OP = 'UPDATE'.

New finding during validation

The edifice-owned companion trigger trg_provision_edifice_org (provision_edifice_org()) has the identical unguarded ::uuid cast and fires whenever company_id is non-null and allowed_apps contains edifice. It was hit mid-validation (test user had allowed_apps: ["edifice"]) and is not fixed by this PR — it's edifice-owned and out of scope for #62. Flagged in docs/cross-repo-log.md and .claude/STATUS.md as a follow-up issue to file in the edifice repo.

Validation

  • Static review: migration parses as valid SQL, mirrors the existing trigger's structure except for the WHEN clause.
  • Applied to prod (zgkvbjqlvebttbnkklpo) via apply_migration (Supabase MCP) — success, no error. Confirmed post-apply via pg_get_triggerdef.
  • Rolled-back behavioral test against prod (isolated from the edifice trigger):
    • Malformed company_id ("not-a-uuid") → auth.users UPDATE now commits (22P02 no longer raised).
    • Malformed value → no new/changed workspace_members row (provisioning correctly skipped).
    • Valid UUID → trigger still fires and provisions correctly (happy path unaffected, 6→7 members).
    • ROLLBACK → zero residue, Yani's row unchanged.
  • Offline regression: make testdeno check clean on both Edge Functions, 157 pytest passed / 17 deselected.

Not in scope

  • No change to #61 (M1, UNIQUE on halcrm_workspaces.company_id), #63 (M5 checklist), #64 (L3 CLAUDE.md doc) — each a separate open issue.
  • No RAISE WARNING for the malformed-input skip case — WHEN clauses can't execute statements; the issue's proposed fix is exactly the regex guard shipped here.
  • No hal-mcp / Edge Function change, no redeploy — pure DB trigger change.

Fixes #62

@BluegReeno

Copy link
Copy Markdown
Owner Author

🔍 PR Review Synthesis

PR: #66
Reviewed by: 2 of 5 planned specialized agents (code-review, error-handling)
Date: 2026-07-22


⚠️ Coverage note

This run only produced artifacts for code-review and error-handling. The test-coverage, comment-quality, and docs-impact agents did not run (no findings files were generated for this PR). Given the PR's shape — no test files (SQL migrations aren't unit-tested in this repo), a two-line trigger diff, and a docs entry already reviewed favorably by both agents that did run — those three dimensions likely have little to add, but that's an inference, not a verified result. Flagging rather than silently presenting this as a complete 5-agent review.


Summary

Small, well-scoped, trigger-only fix (3 files, +37/-1) guarding the ::uuid cast in trg_provision_workspace_membership's WHEN clause against a malformed company_id. Both agents verified the change live against prod (trigger/function text match the migration exactly, no 42P17 regression, function body untouched) and both independently recommend APPROVE.

Verdict: APPROVE (based on the 2 agents that ran)

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

🟡 Medium Issue (Needs Decision)

Malformed company_id skip has no diagnostic signal

📍 supabase/migrations/20260722195616_guard_company_id_uuid_cast.sql:20-26

The function already logs (RAISE WARNING) when company_id is well-formed but matches no halcrm_workspaces row. This PR's new malformed-company_id skip is structurally the same outcome (no membership created) but leaves zero trace, because a WHEN clause can't execute RAISE. A user with a malformed company_id gets a working invite but silently zero workspace access, with no log breadcrumb explaining why.

Both agents agree: not a blocker for this PR (moving the check into the function body would exceed this PR's intentionally trigger-only scope). Recommended: file a follow-up issue for a periodic ops check (e.g. scan auth.users for non-UUID company_id values with no matching workspace_members row) — mirroring how this same PR already flagged the sibling edifice trg_provision_edifice_org gap in docs/cross-repo-log.md.


🟢 Low Issues

View 2 low-priority notes
Issue Location Suggestion
Regex accepts only canonical dashed UUID form, narrower than Postgres's ::uuid parser 20260722195616_guard_company_id_uuid_cast.sql:24-25 No action needed — company_id is only ever written by auth-gateway in canonical form; widening would be speculative (YAGNI)
Same finding, from error-handling's independent pass same location Optional: one-line comment documenting the canonical-form assumption for future readers

✅ What's Good

  • Live-verified against prod: trigger/function text match the migration exactly, function body byte-identical to the prior migration, no 42P17 regression.
  • docs/cross-repo-log.md proactively documents the out-of-scope edifice companion-trigger gap rather than leaving it silent.
  • Validation was a rolled-back behavioral test against real prod data (both the malformed-input and valid-UUID paths), not just static review.
  • .claude/STATUS.md correctly moves issue P2 — Guard ::uuid cast in provisioning trigger against malformed company_id #62 to Done in the same commit.

📋 Suggested Follow-up

Issue Title Priority Related Finding
Add periodic ops check for malformed company_id values silently skipped by provisioning trigger P3 MEDIUM finding above

(The edifice-side companion-trigger fix is already tracked as a follow-up in docs/cross-repo-log.md / .claude/STATUS.md — not a gap in this repo's tracker.)


Next Steps

  1. No CRITICAL/HIGH issues block merge.
  2. Decide on the MEDIUM finding: file the periodic-check follow-up (recommended) or explicitly accept the gap.
  3. Optional: add the one-line regex-assumption comment.
  4. Confirm whether the missing test-coverage/comment-quality/docs-impact passes are needed before treating this review as complete.
  5. PR is currently a draft — mark ready when the above is resolved.

Synthesized from 2/5 planned review agents. Artifacts: artifacts/runs/87b9961cdc3f64c7e651dffe5b31eb1a/review/

@BluegReeno

Copy link
Copy Markdown
Owner Author

⚡ Self-Fix Report (Aggressive)

Status: COMPLETE
Pushed: ✅ Changes pushed to archon/task-fix-issue-62 (7797df3)
Philosophy: Fix everything unless clearly a new concern


Fixes Applied (1 total)

Severity Count
🔴 CRITICAL 0
🟠 HIGH 0
🟡 MEDIUM 0
🟢 LOW 1
View all fixes
  • Regex accepts only canonical dashed UUID form (supabase/migrations/20260722195616_guard_company_id_uuid_cast.sql:24) — added a 3-line comment documenting the canonical-form assumption and why (auth-gateway is the sole writer, always emits this form)

Tests Added

(none — this repo does not unit-test SQL migrations; validation stays manual review + rolled-back behavioral test, already done before review)


Docs Updated

  • docs/cross-repo-log.md — extended the 2026-07-22 entry to record the MEDIUM finding was tracked as an issue rather than fixed inline

Skipped (2)

Finding Reason
MEDIUM: malformed company_id skip has no diagnostic signal Architectural change (would require CREATE OR REPLACE FUNCTION), explicitly OUT OF SCOPE per scope.md; both reviewing agents recommended a follow-up issue over an in-PR fix. Filed as hal#67.
LOW: regex narrower than Postgres's ::uuid parser (code-review's take) Reviewing agent's own explicit recommendation is "no action" — widening it would be speculative configurability against YAGNI/KISS (CLAUDE.md); the sibling take on the same finding was addressed instead via a documentation comment.

Suggested Follow-up Issues

  1. Already filed: hal#67 — periodic ops check for the silent-skip observability gap (P3)

Validation

✅ Tests (157 passed, pytest) | ⚠️ Deno type-check not run (binary unavailable in this environment; no .ts files touched) | N/A Lint


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

…company_id (#62)

The WHEN clause of trg_provision_workspace_membership now regex-validates the
canonical UUID shape before the function's unconditional ::uuid cast runs, so a
malformed company_id fails WHEN, the function never executes, and the auth.users
write commits (provisioning silently skipped) instead of aborting on 22P02.
Trigger-only fix-forward — function body unchanged, WHEN references only NEW
(no 42P17). Already applied to prod zgkvbjqlvebttbnkklpo via apply_migration.

Flattened onto main (was a merge commit) to resolve the STATUS.md /
cross-repo-log.md overlap with #61 (#68) — both follow-ups now land in the same
backlog line and Done section.

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

Copy link
Copy Markdown
Owner Author

⚠️ This PR's GitHub head object is stuck on a pre-merge commit (7797df3) — the synchronize webhook never fired after the conflict resolution was pushed, so GitHub's merge engine keeps seeing a stale, conflicting tree even though the branch ref is clean and linear on main. Superseded by #70 (identical content, fresh branch fix/issue-62-guard-uuid-cast). Resume from the hal repo: merge #70, then close this. The migration is already live in prod.

@BluegReeno
BluegReeno force-pushed the archon/task-fix-issue-62 branch from 097d340 to 26425b3 Compare July 23, 2026 08:31
@BluegReeno

Copy link
Copy Markdown
Owner Author

Superseded by #70 (identical content on a fresh linear branch). #70 merged.

@BluegReeno BluegReeno closed this Jul 23, 2026
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.

P2 — Guard ::uuid cast in provisioning trigger against malformed company_id

1 participant