Skip to content

Tighten Integrations to strict-UUID + V107 endpoint UNIQUE index + V106 follow-up + provider registry expansion#511

Merged
ddon merged 9 commits into
BeamLabEU:devfrom
mdon:dev
May 1, 2026
Merged

Tighten Integrations to strict-UUID + V107 endpoint UNIQUE index + V106 follow-up + provider registry expansion#511
ddon merged 9 commits into
BeamLabEU:devfrom
mdon:dev

Conversation

@mdon
Copy link
Copy Markdown
Contributor

@mdon mdon commented May 1, 2026

Summary

Nine commits since #510. Major themes:

  • Strict-UUID Integrations API. Public mutating / OAuth / HTTP / validation API now takes the storage row's uuid. Provider-string + name strings are sourced only from the row's key column; corrupted JSONB cannot leak into a new storage key. Read shims (get_integration/1, get_credentials/1, connected?/1) accept either uuid OR provider:name for legacy data walks. New Integrations.resolve_to_uuid/1 is a dual-input lookup primitive used by feature modules during their migrate_legacy/0 runs.

  • V107. Adds integration_uuid to phoenix_kit_ai_endpoints with backfill from the legacy provider string column, plus a UNIQUE index on lower(name) for AI endpoint names (folded in for atomicity rather than spread across two migrations).

  • migrate_legacy/0 module callback. Each module that has legacy data implements its own migrator; PhoenixKit.ModuleRegistry.run_all_legacy_migrations/0 walks every registered module. Errors are caught + logged so a misbehaving module never crashes boot. The pre-uuid Integrations.run_legacy_migrations/0 now delegates to the orchestrator (deprecated shim).

  • Picker policy: never auto-pick. IntegrationPicker no longer auto-selects when only one connection exists — the picker reflects the consuming module's stored state, not the available connections. The operator must explicitly select / deselect.

  • Provider registry expansion. Built-in providers gain mistral, deepseek, and microsoft (OAuth2). Registry remains extensible via modules' integration_providers/0 callbacks.

  • PR Add V106 migration: split phoenix_kit_projects name uniqueness for templates vs projects #510 follow-up (last commit). V106's down/1 gains a cross-mode duplicate pre-check (raises a named error before dropping the partial indexes, so the table never ends up with no name uniqueness during a half-applied rollback). Moduledoc rewritten to a dedicated "Schema-side change only — changeset half lives downstream" subsection. 12 new tests in test/phoenix_kit/migrations/v106_test.exs. Skipped items (prefix_str/1 hoist, generic incremental-version-comment integration test) surfaced for separate consideration. See dev_docs/pull_requests/2026/510-v106-projects-name-uniqueness-split/FOLLOW_UP.md.

Test plan

  • mix compile --warnings-as-errors clean
  • mix test test/phoenix_kit/migrations/ — 18 tests, 0 failures
  • mix test — 1040 tests + 11 doctests, 1 pre-existing intermittent flake unrelated to this PR
  • mix format --check-formatted clean

mdon added 9 commits May 1, 2026 19:09
The integrations admin treated `"default"` as a privileged connection
name — locked from rename/remove, hidden behind UI gates, and used as
the implicit fallback in `get_credentials/1`. Names are now pure
user-chosen labels with no system semantics, and consumers reference
integration rows by uuid.

UI / form
- /new always asks for a connection name (no silent "default")
- Edit page rename input is always editable on every connection
- List page shows the name verbatim, no "—" rendering for default
- Edit URL is `/admin/settings/integrations/:uuid` (was
  `/:provider/:name`); renames don't change the route
- Remove button no longer gated on `name != "default"`
- Test Connection action available on `connected`, `configured`, and
  `error` rows (was only on `connected`)
- Drop "Used by" column — was a static module declaration map, not
  actual usage; misleading on freshly-created rows
- Empty-state subtitle lists provider names dynamically with a
  gettext-extractable "and" conjunction

Status semantics
- Saving credentials no longer flips status to "connected" optimistically;
  goes to "configured" instead. Successful validation is what flips
  status to "connected" and stamps `connected_at` (one-shot — never
  overwritten on subsequent successful checks).
- Form auto-triggers a validation immediately after save when there's
  something testable, so the transition to "connected" typically
  happens within milliseconds of the save click.

Context API
- Add `get_integration_by_uuid/1` returning `{:ok, %{uuid, provider, name, data}}`
- Add `rename_connection/4` with copy-row → delete-old → broadcast
- Add `Events.broadcast_connection_renamed/3` and the matching
  `:integration_connection_renamed` topic event
- Drop `cannot_rename_default` and `cannot_remove_default` guards
- Drop `find_first_connected/1` and the bare-provider fallback chain
  in `get_credentials/1` — both consumers (AI, document_creator) now
  resolve by uuid, so the system never has to guess

V107 migration
- New `phoenix_kit_ai_endpoints.integration_uuid uuid` column + index
- Backfill from existing `provider` strings: exact match for
  `provider:name` shape, most-recently-validated row for bare
  providers, `uuid ASC` tiebreaker, NULL when unresolvable

Test infrastructure (incidental, was already staged on this branch)
- Endpoint started once at suite level instead of per-test
  `start_supervised` — fixes ETS ArgumentErrors when async tests'
  teardowns race against concurrent requests
- `render_errors` formats + `live_view` signing salt in test config so
  500-class crashes surface the real error instead of "no 500 template"
- `lazy_html` test-only dep
- Fixture corrections for V95 NOT NULL `file_checksum` /
  `user_file_checksum`; `list_files_in_scope/2` direct-children-vs-
  subtree split; `cancel_invitation/1` returns `:not_pending` atom not
  changeset; `list_folder_tree/1` returns nested `%{folder, children}`;
  unauth LV redirect needs `fetch_flash` primed; UUIDv7-typed fields
  reject non-uuid string inputs
- `assigns[:key]` instead of `@key` in root layout for assigns that
  may be absent on error pages; flash_group gets explicit `id`
Coverage for the uuid-everywhere integrations migration that landed in
the previous commit. Also surfaces and fixes two bugs that the new
tests caught.

Tests
- `rename_connection/4` — success, exact-name no-op, :empty_name,
  :invalid_name, :already_exists, :not_found, name-pattern violations,
  `default` is renameable (no privileged name), broadcasts
  `:integration_connection_renamed` on success only
- `get_integration_by_uuid/1` — normalized %{uuid, provider, name, data}
  shape, :invalid_uuid for non-uuid input, :not_configured for
  legacy rows missing the `provider` field
- `get_credentials/1` — uuid input that misses → :deleted; bare /
  provider:name input that misses → :not_configured (no more
  silent find_first_connected fallback)
- `Events.broadcast_connection_renamed/3` — subscribe + assert_receive
- LV smoke (`integrations_test.exs` under `live/settings/`):
  list page name verbatim (no `—` for default), uuid-based Configure
  links, /new always asks for a connection name, edit page rename
  input always editable, rename persists in place, default-named
  rows can be renamed, already-exists surfaces an error flash,
  unknown uuid → flash + redirect to list
- `v107_test.exs` — schema additions (column + index, version comment),
  exact `provider:name` match, most-recently-validated wins on bare
  provider, `uuid ASC` tiebreaker when `last_validated_at` ties,
  unresolvable rows stay NULL. Runs the backfill SQL directly because
  `Ecto.Migration.execute/1` requires a runner process the regular
  test framework can't provide.

Bug fixes surfaced by the new tests
- `save_setup/3` was overwriting `data["provider"]` with the full
  storage key (`"openrouter:primary"`). Now extracts the base
  provider and stores `name` separately so `get_integration_by_uuid/1`
  returns sane values.
- `rename_connection/4` originally did copy-then-delete, which
  generated a NEW uuid for the renamed row — defeating the entire
  point of uuid-stable references. Reworked to UPDATE the row's
  `key` column in place via Ecto changeset so consumers'
  `integration_uuid` references survive renames.

Documentation
- Integrations moduledoc: uuid-first usage examples; document
  rename/remove freedom (no privileged names)
- AGENTS.md "Integrations System" section: updated arities (3 for
  add/remove, 4 for rename), dropped the cannot-remove-default note,
  added consumer pattern + the renamed broadcast event
- dev_docs/plans/integrations-system.md: addendum at the top
  documenting the uuid-everywhere migration with cross-references
  to the test files
…ator-specific bits

The integrations system used to migrate legacy data via two
mechanisms: hardcoded `@legacy_keys` knowledge in
`Integrations.run_legacy_migrations/0` (covered the
`document_creator_google_oauth` → `integration:google:default` case),
and an on-read fallback in `get_integration/1` that fired the same
migration lazily. Both bound core to a specific module's data shape,
which was the wrong layer.

This commit moves the orchestration into a behaviour-based pattern:

- New optional `migrate_legacy/0` callback on `PhoenixKit.Module`.
  Default impl returns `:ok`. Modules with legacy data override to
  do their own combined credentials + reference migration.
- New `PhoenixKit.ModuleRegistry.run_all_legacy_migrations/0` —
  walks every registered module, calls `migrate_legacy/0` on each,
  catches per-module errors so the host-app boot can't be taken
  down by a flaky module. Returns a result map keyed by module atom.
- New `Integrations.find_uuid_by_provider_name/1` primitive —
  resolves a `provider:name` string (or tuple) to the matching
  integration row's uuid. Modules use this in their migrators to
  rewrite legacy name-string references.

Removed from core:

- The `@legacy_keys` map (was hardcoded knowledge of doc_creator's
  old key shape)
- `do_migrate_legacy("google", _)` and `migrate_legacy_folders/1`
  (doc_creator-specific OAuth-shape conversion)
- `maybe_migrate_legacy/1` and the on-read fallback branch in
  `get_integration/1` (lazy-on-read pattern moved to module side)
- `has_legacy_data?/1` helper (no longer used)

`Integrations.run_legacy_migrations/0` is now a `@deprecated` thin
shim that delegates to the orchestrator — existing host-app callers
get the union of all module migrations now, not just doc_creator's
google-OAuth case (probably what they want anyway).

Tests:

- 5 new `find_uuid_by_provider_name/1` cases (exact match, tuple
  form, bare-provider treated as `:default`, `:not_found`,
  `:invalid`)
- 2 new `get_integration/1` post-extraction semantics (non-uuid
  miss returns `:not_configured` cleanly, even when legacy keys
  exist on the side)
- 5 new orchestrator tests on `module_registry_test.exs`
  (returns map keyed by module, every registered module appears,
  default-impl modules return `:ok`, swallows per-module raises)
- Removed 2 stale on-read legacy-migration tests in `integrations_test.exs`
  that asserted behavior I deliberately removed (replaced with
  comment pointing at the new home: doc_creator's `migrate_legacy/0`)
`Endpoint.changeset/2` (in phoenix_kit_ai) has registered
`unique_constraint(:name)` since the schema was extracted, but
core's V34 migration that creates `phoenix_kit_ai_endpoints`
never created the matching index. Duplicate names were silently
accepted in production for years; the AI module's hand-rolled
test migration was secretly creating the index, masking the
discrepancy. Surfaced 2026-04-29 when the AI test migration
was removed in favor of delegating to core.

V107 hasn't been published yet — fold the missing index in
rather than spinning a separate V108. UNIQUE on `lower(name)`
so case-only differences don't sneak past the constraint
either. Down step drops the index alongside the integration_uuid
column.

Idempotent (`CREATE UNIQUE INDEX IF NOT EXISTS`); safe to apply
to a dev DB that already ran the original V107 by running the
SQL directly.
Convert the Integrations module from string-keyed (`provider:name`)
to strict-UUID for every operation past row creation. Storage-key
construction (`integration:{provider}:{name}`) is now confined to
`add_connection/3` (row birth) and module-side `migrate_legacy/0`
migrators — every other public API takes the row's uuid. Closes the
bug class where corrupted JSONB `provider`/`name` fields could leak
into new storage keys (the historical
`integration:openrouter:default:default` double-concat).

Functions flipped to uuid-strict:
- save_setup, disconnect, remove_connection, rename_connection
- record_validation, validate_connection
- authorization_url, exchange_code, refresh_access_token
- authenticated_request

Read shims stay dual-input for `migrate_legacy/0` walks of legacy
data: get_integration/1, get_credentials/1, connected?/1,
find_uuid_by_provider_name/1.

Form / UX changes alongside:
- Test Connection on /new dry-runs against the provider's API
  without persisting (no half-baked rows on a failed test)
- Test Connection always uses the inputted values, not a stale
  saved row — so re-typed keys actually get verified
- Test bypasses HTML5 required-field validation (formnovalidate)
  so operators can verify a key without inventing a name first
- Typed values survive a dry-run re-render (form_values assign)
- Auto-test fires for status="error" too — re-test after fixing a
  broken connection now actually re-tests
- record_validation always advances last_validated_at (was guarded
  against churn, broke the "Last tested N ago" UI)
- connected_at now tracks last successful connection (was one-shot,
  inconsistent with OAuth exchange_code; matches user expectation
  that "Connected N ago" updates on re-connect)
- add_connection/3 returns {:ok, %{uuid, data}} — callers no longer
  need a list_connections lookup to recover the row uuid
- Recovery card on stuck-migration endpoints (api_key set,
  integration_uuid nil) — read-only field with copy button
- Password-manager friction removed: type="text" + autocomplete=off
  instead of type="password" stack, so Bitwarden/Firefox/etc. don't
  offer to vault API keys as logins
- record_validation broadcast gated on actual state change (no
  reload storms from high-frequency automatic paths)

Storage-key invariant pinned by a regression suite — fuzzes
provider/name edge cases (colon-bearing names, JSONB body drift)
through create→rename→read and asserts the storage key never grows
extra colon segments.

mix precommit clean: 1022 tests, 11 doctests, 0 failures.
The picker should mirror the consumer's actual stored selection, not
make convenience guesses. Two behavior changes:

- `auto_select/2` removed. Single-connection auto-selection visually
  highlighted a card the consumer LV had not chosen, contradicting
  the picker's role as a state display. The single visible
  auto-select also never propagated to the parent, so it was a
  display-only lie that diverged from what would be saved.

- `select_action(false, uuid, selected_set)` now returns "deselect"
  when clicking the currently-selected card, "select" otherwise.
  Single-mode consumers can match on the action and clear their
  pinning. Multi-mode behaviour (add/remove) is unchanged.

Moduledoc updated to document the new single-mode event payload
shape (`%{"uuid" => uuid, "action" => "select" | "deselect"}`).
Three new entries on `Providers.builtin_providers/0` so consumers
can pick them in `Settings → Integrations → Add`. The integration
form (`integration_form.html.heex`) is fully driven by the provider
definition's `auth_type`, `setup_fields`, `validation`, and
`instructions` — no consumer-side wiring needed for the form itself.

Mistral (`auth_type: :api_key`):
- Validation hits `https://api.mistral.ai/v1/models` with
  `Authorization: Bearer <key>` (the lightest endpoint that 200s
  for valid keys, 401s for invalid)
- Setup steps cover the billing-required gotcha (Mistral refuses to
  create keys without a payment method on file even for free models)

DeepSeek (`auth_type: :api_key`):
- Validation hits `https://api.deepseek.com/models` (same shape as
  OpenAI's /models endpoint)
- Setup steps cover the credit-required gotcha (calls 402 if the
  workspace has zero balance)

Microsoft 365 (`auth_type: :oauth2`):
- Authority defaults to `common` so multi-tenant + personal-account
  apps work out of the box; instructions panel calls out the
  single-tenant case where operators replace `common` with their
  tenant ID
- `default_scopes` requests `openid email profile offline_access User.Read`
  — `offline_access` is required for refresh tokens (Microsoft drops
  refresh_token without it), `User.Read` is the minimum for the
  /me userinfo probe to return a profile during validation
- `auth_params: %{"prompt" => "consent"}` forces the consent screen
  on every connect so refresh_token is always issued (Microsoft
  silently omits it on re-auth otherwise)
- Setup steps walk through Azure App registration, the client
  secret Value-vs-Secret-ID gotcha, and the API permissions /
  admin consent flow

All three follow the same conventions as the existing `google` and
`openrouter` definitions: `gettext` on every label/help/instruction
string, `setup_fields` with explicit `required` + `placeholder` +
`help`, and `instructions` rendered by the form's collapsible
"Setup Instructions" panel.
Centralises the "given a binary that may be EITHER a settings-row uuid
OR a `provider:name` string, resolve to the row's uuid" lookup that
consumer modules' lazy-promotion paths and migration sweeps converge
on.

The shape was duplicated across at least two consumer call sites:
- `phoenix_kit_ai/lib/phoenix_kit_ai/openrouter_client.ex`
  (`lookup_uuid_for_provider/1` — lazy on-read promotion)
- `phoenix_kit_ai/lib/phoenix_kit_ai.ex`
  (`resolve_provider_to_uuid/1` — V107 migration sweep)

Each carried the same regex + dispatch + provider:name split. Without
this primitive the next consumer module that needs the same lookup
would tempt a third copy-paste; centralising removes that risk.

`find_uuid_by_provider_name/1` already existed for the provider:name
half of the lookup, but it doesn't handle the "input is already a
uuid" case — given `"019b669c-..."` it parses as a provider name and
searches `integration:019b669c-...:default`, which always misses.
`resolve_to_uuid/1` gates on `uuid?/1` first, verifies the row via
`get_integration_by_uuid/1`, then delegates to
`find_uuid_by_provider_name/1` for everything else.

Returns `{:ok, uuid}` when the input resolves to a current row,
`{:error, :not_found}` when it parses cleanly but no row exists,
`{:error, :invalid}` for malformed input (empty / nil / non-binary).

6 new tests cover the matrix:
- valid uuid passthrough (uuid → verify-row → return)
- :not_found for uuid-shaped strings with no matching row
- delegation to find_uuid_by_provider_name for provider:name input
- bare provider treated as provider:default
- :not_found for provider:name that doesn't resolve
- :invalid for empty / nil / non-binary input

mix test: 1028 tests, 0 failures (was 1022, +6 new).
Closes the IMPROVEMENT-MEDIUM and one of the NITPICKs from PR BeamLabEU#510's
review (`CLAUDE_REVIEW.md`). The HIGH-severity finding (off-by-one
version comments in V106's `up`/`down`) was already fixed by Pincer
in `86f8ac0d` on review-day; the remaining hoist + integration-test
nitpicks are surfaced in FOLLOW_UP.md as candidates for separate
follow-up PRs.

down/1 cross-mode duplicate pre-check
-------------------------------------

V101's global `phoenix_kit_projects_name_index` was lossy on rollback:
when the post-V106 state legitimately has a template and a real
project sharing a name (the whole point of the V106 split), recreating
the global UNIQUE index during `down/1` would surface a generic
Postgres `duplicate key value violates unique constraint` error —
AFTER the partial indexes had already been dropped, leaving the
table with no name uniqueness at all until the rollback could be
either completed or reversed.

`down/1` now runs a `SELECT ... GROUP BY ... HAVING count > 1
LIMIT 1` ahead of the index drops. If duplicates exist, it raises an
actionable message naming the offending lower-cased name so the
operator has something to grep for. `LIMIT 1` because operators
resolve duplicates one at a time and re-run; surfacing all
duplicates eagerly would be expensive on a large projects table.

The moduledoc on `down/1` was updated to call out the pre-check
ordering explicitly so the next reader doesn't wonder why the
SELECT runs before the DROPs.

Moduledoc — schema-side-only is more explicit
---------------------------------------------

The original V106 moduledoc carried a one-sentence note that the
matching changeset's `unique_constraint(...)` swap is in
`phoenix_kit_projects` "in the same release." That left a future
maintainer free to grep `lib/` for the constraint reference here and
conclude something was missing. The note is now a dedicated
subsection ("Schema-side change only — changeset half lives
downstream") that:

- Calls out explicitly that no `Project` schema lives in this repo
  (`rg phoenix_kit_projects_name_template_index lib/` returns only
  V106 itself).
- Documents the downstream changeset's `is_template`-driven
  constraint-name selection.
- Spells out the consequence of forgetting to ship the changeset
  half — V106's split would be invisible to end users.

Tests
-----

New `test/phoenix_kit/migrations/v106_test.exs` — 12 tests. Same
pattern as V107Test (the migration helpers can't be invoked outside
an `Ecto.Migrator` runner, so the tests pin schema state via
`pg_indexes` queries and replicate the down step's pre-check SELECT
verbatim).

Schema state (6 tests):
- both partial unique indexes exist
- V101's global index has been replaced (does NOT exist) — catches a
  regression that re-introduces it
- templates and real projects can share a name (the V106 goal)
- two templates with the same name are still rejected
- two real projects with the same name are still rejected
- name uniqueness is case-insensitive within each mode

Down pre-check (5 tests):
- empty state → query returns empty rows
- single template + single real project sharing a name → duplicate
  detected (the canonical scenario the pre-check exists for)
- case-only difference still counts as a duplicate
- multiple duplicates → query returns one (LIMIT 1 semantics)
- single rows at distinct names → no duplicate (sanity)

Skipped from this follow-up
---------------------------

- `prefix_str/1` hoist to `Postgres.Helpers` — workspace-wide
  refactor across 30+ migrations, scope-mismatched with this PR's
  one-file change. Reviewer's own framing ("chore PR, not part of
  V106") agrees.
- Integration test for the incremental V105 → V106 upgrade path
  — preventive coverage for off-by-one bugs in OTHER migrations,
  not V106-specific. The actual bug is already fixed.

Both surfaced in FOLLOW_UP.md as candidates for separate PRs if Max
wants them — they're cumulative-value-positive, just not part of
this follow-up's scope.

Verification
------------

mix compile --warnings-as-errors clean. Migrations test suite
(`test/phoenix_kit/migrations/`) at 18/18 stable (12 new + 6
pre-existing V107). Full mix test 1040/0 with one intermittent flake
in `test/integration/media_browser_scope_test.exs` (pre-existing,
converged 1/0/0 across 3 stability runs).
@ddon ddon merged commit 2282422 into BeamLabEU:dev May 1, 2026
ddon pushed a commit that referenced this pull request May 2, 2026
Review of strict-UUID Integrations API + V107 + V106 follow-up + provider
registry expansion. Two MEDIUM findings (connected_at doc drift, hardcoded
Microsoft tenant), two LOW findings, and several nitpicks. Approve on merit;
shipped state is safe.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
ddon pushed a commit that referenced this pull request May 2, 2026
- AGENTS.md: connected_at description was stale (still said "one-shot").
  Code now bumps connected_at on every successful re-test, matching the
  OAuth exchange_code/4 path. Update the integrations section to say so.
- v107.ex moduledoc: said "tiebreaker on inserted_at ASC" but the actual
  SQL uses "uuid ASC" with a comment explaining UUIDv7 time-ordering.
  Fix the moduledoc to match.
- module.ex: migrate_legacy/0 callback doc referenced
  PhoenixKit.Modules.run_all_legacy_migrations/0 — should be
  PhoenixKit.ModuleRegistry.run_all_legacy_migrations/0 (typo).
- v107_test.exs: V107 added a UNIQUE (lower(name)) index on
  phoenix_kit_ai_endpoints — the moduledoc calls out that
  Endpoint.changeset/2 has registered unique_constraint(:name) since
  V34 but the index never existed, silently letting duplicates
  through. Add three tests pinning the new contract: index exists,
  duplicate names rejected, case-only differences collide.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
mdon added a commit to mdon/phoenix_kit that referenced this pull request May 5, 2026
…LOW_UP.md

Closes the one remaining MEDIUM finding from CLAUDE_REVIEW.md (BeamLabEU#2 —
hardcoded `/common/` tenant in M365 OAuth URLs that broke single-
tenant operators with AADSTS50194). Two-part fix:

1. New `interpolate_url/3` private helper in `OAuth` substitutes
   `{key}` placeholders with values from the integration's data
   (string-keyed JSONB), falling back to `oauth_config[:url_defaults]`.
   Wired into `authorization_url/5`, `exchange_code/4`, and
   `refresh_access_token/2`. URLs without `{` pass through unchanged
   (zero impact on Google / OpenRouter / Mistral / DeepSeek).

2. Microsoft provider's `auth_url` / `token_url` now use `{tenant_id}`;
   new `url_defaults: %{"tenant_id" => "common"}` preserves multi-tenant
   default behavior; new optional `tenant_id` setup field with help
   text covering the GUID / `consumers` / `organizations` options.
   Instructions panel note rewritten — operators fill in the form
   field rather than manually editing URLs.

Three pinning tests added to OAuth test suite (interpolation with a
GUID; fallback to url_defaults; backwards-compat pass-through for
URLs without placeholders). All 21 OAuth tests pass.

The other nine reviewer findings (1 MEDIUM doc, 2 LOW doc/typo,
6 NITPICKs) were addressed pre-existing or reviewer-acknowledged as
non-blocking — see FOLLOW_UP.md.
mdon added a commit to mdon/phoenix_kit that referenced this pull request May 5, 2026
…follow-up)

Closes NITPICK BeamLabEU#10 from CLAUDE_REVIEW.md on PR BeamLabEU#511. The lenient `:ok`
return on the missing-state branch of verify_oauth_state/2 dated from
an older flow that didn't always save state before redirect; that flow
is gone (every connect_oauth event now calls save_oauth_state/2 first
at integration_form.ex:227). A missing state at callback time now
implies either someone bypassed connect_oauth or the row was mutated
between authorize and callback — both are CSRF-relevant. Returns
{:error, :state_mismatch} on that branch with an updated comment
explaining why.

No tests exercised the lenient branch (verified via grep). The
{:error, :state_mismatch} return shape is already handled by the
caller's existing mismatch case.

FOLLOW_UP.md updated to reflect the closure (moves from Skipped to
Fixed Batch 1; NITPICK BeamLabEU#6 stays Open pending Max's decision).
mdon added a commit to mdon/phoenix_kit that referenced this pull request May 5, 2026
… follow-up)

Closes NITPICK BeamLabEU#6 from CLAUDE_REVIEW.md on PR BeamLabEU#511, but as a code fix
rather than the doc-note the reviewer suggested. The picker was
rendering provider name in place of `conn.name` whenever
`conn.name == "default"` (a pre-BeamLabEU#511 carryover from when `default` was
system-privileged). PR BeamLabEU#511's own moduledoc explicitly says "Names are
pure user-chosen labels with no system semantics" — the picker
contradicting that is the actual bug.

Now always renders `conn.name` verbatim. Provider badge is also
unconditional (drop the `conn.name != "default"` guard) so users
always see which provider they're picking regardless of how the
connection is named.

Only call site of the substitution; no other surface depended on it.
Two test references to `name == "default"` are persistence assertions,
not picker rendering — unaffected.

FOLLOW_UP.md updated: NITPICK BeamLabEU#6 moves to Fixed (Batch 1); Open is now
None.
ddon added a commit that referenced this pull request May 6, 2026
V111 PDF library tables + #511/#512/#515 follow-ups
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.

2 participants