Record trusted sandbox provider identity

[jhgaylor]

A provider name alone cannot identify which sandbox instance was observed. Add an optional provider-issued ID and an internal writer that locks and rechecks tenant/provider/name, binds once, and refuses retired rows or conflicting identities. General sandbox changesets cannot set it; rollback refuses to discard recorded IDs.

Extracted from #1754 onto #1796: six files, +466 lines (278 test lines plus ADR/index). This is recording infrastructure. It adds no lifecycle caller or provider-write authorization; those need a separate durable operation protocol.

Validation: 35 focused tests, including two observed PostgreSQL lock interleavings; full mix precommit --seed 828329 passes (4,900 tests +6 doctests). Disposable-database migration proof covers legacy null backfill, refusal of populated rollback, empty rollback and reapply. OKF validation/index and staged secret scan pass. CI passes (run).

Part of #1864

[jhgaylor]

Reviewed — no changes requested, two things worth a decision.

The write path is careful in the ways that matter. Taking the advisory lock and FOR UPDATE before rechecking {user_id, provider, sprite_name}, refusing failed/terminated rows, treating a re-bind of the same id as a no-op rather than an error, and rolling back on a different id — that's the full set. Audit.record sitting outside the transaction is right (0013: a rescued audit insert doesn't survive a transaction, it aborts the enclosing one), and the docstring saying so means the next person won't helpfully move it inside. actor: "system:sandbox_identity" is in the closed vocabulary.

I verified the claim that general changesets can't set it: provider_instance_id is absent from Sandbox.changeset/2's cast list, so the only writer is _unsafe_bind/2. Good — that's the property the whole module rests on.

Two things I'd like you to decide rather than change on my say-so:

1. The down migration raises rather than rolls back. Once any row has an identity, down is a hard stop and the only way past it is manual SQL. That's a deployment hazard for a column whose contents are re-derivable — the provider still knows the id, and a later capture re-reads it. Refusing rollback to protect data you can ask the provider for again feels like the wrong side of that trade. If the intent is "don't silently lose evidence during an incident", a log line plus the drop gets you most of it without wedging a rollback.

2. Nothing calls it. SandboxIdentity has zero callers outside its own tests — I grepped. I'm not treating that as a blocker, because landing storage ahead of its caller is exactly how you keep the follow-up small and that's the point of this stack. It's also honestly labelled: ADR 0049 is Proposed and says outright that worker integration is separate, which is the discipline the ADR rules ask for and that this kind of PR usually skips. Just worth being deliberate that the unique index on (provider, provider_instance_id) is a constraint we're committing to before anything exercises it.

Minor: _unsafe_capture/2 and _unsafe_bind/2 have @doc but no @spec, unlike most of the surrounding context modules.

[BinaryBourbon]

Approving — detailed review in the comment above.

[BinaryBourbon]

Approving. Storage plus one trusted writer, and the ADR is honest about what it is not — "this records an observation, it does not authorize a later provider mutation" is exactly the caveat the 2026-07 audit exists to force, and it is in the ADR body, the @moduledoc and the index row.

What I verified:

• The identity cannot arrive from tenant input. provider_instance_id is absent from Sandbox.changeset/2's cast/3 list, so an ordinary update drops it silently. The test drives that both ways — before binding and after — with string and atom keys.
• Provider I/O is outside every lock. _unsafe_capture/2 refuses an open transaction before calling Managoat.Sandbox.get/1, and the happy-path test asserts refute Repo.in_transaction?() from inside the stubbed provider call. _unsafe_bind/2 repeats the refusal so a direct caller cannot get around it.
• The write is genuinely fenced. Advisory lock, then FOR UPDATE, then a recheck of tenant/provider/name and status, then a write only on nil. ^id re-binding is idempotent and does not re-audit; a different id is :provider_identity_changed. The lock test is the part that earns this — it drives a real PostgreSQL row-lock wait, confirms two distinct backends, waits on pg_blocking_pids, and asserts the post-wait recheck rejects. That is the test most people skip and then ship the race.
• The partial unique index and the changeset constraint agree. create unique_index(:sandboxes, [:provider, :provider_instance_id], where: ...) produces sandboxes_provider_provider_instance_id_index, which is what unique_constraint([:provider, :provider_instance_id]) infers, and the retired-owner test proves the {:error, changeset} return survives the enclosing transaction rather than aborting it.
• The audit follows the commit and records provider only — no id, no name. Correct under ADR 0013's "never record values."

Three things to fix or answer, none of which I would hold the merge for:

1. 4316 is a bare literal here. It is @sandbox_lock_namespace in conversations.ex, whose own comment says the namespaces "must never be mistaken for one another." Taking the same namespace with the same phash2(sandbox.id) key is deliberate and correct — it is what serializes a binding against do_reset_sandbox/2 and turn admission — but that intent is now invisible at the third site. Please reference the named attribute, or add a comment saying which lock this is joining and why.

2. unboxed_run has no precedent in this repo. SandboxIdentityLockTest is the first place that writes real rows outside the SQL Sandbox into fountain_test, which is shared across worktrees and CI partitions. The after blocks clean up, but Repo.delete!(Repo.get!(Sandbox, observed.id)) raises if the row is already gone and would mask the real failure while leaking the users. Wrapping the teardown in delete_all by id would make it unconditional. Worth a sentence in the module doc explaining why unboxed is unavoidable here, so the next person does not copy the pattern for a test that does not need it.

3. The down migration is one-way in practice. It raises whenever any row carries an identity, and by design nothing ever clears one. So after the first bind in production, mix ecto.rollback past this migration fails permanently — including a rollback you want for an unrelated reason. Refusing to discard data is defensible; making the release irreversible is a separate decision. Is that intended? #1768 has the identical shape one PR over, which is why I am asking here rather than treating it as a nit.

decisions/index.md is refreshed in the same PR and okf validate is green, as the bundle requires. No caller yet is fine — the ADR says so explicitly and the tracker sequences integration separately.