Sandboxes: handle credentials on merge#4835
Open
josephjclark wants to merge 11 commits into
Open
Conversation
josephjclark
changed the title
Collaborator
Author
|
I've pushed a clauded fix. Haven't look at details yet, but it works in manual testing. Two limitations suggested by Claude (1. is on my list to test anyway):
I think we can leave 2 to later |
Collaborator
Author
|
I suppose we should also be thinking about: when this fails, can we show a better error? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4835 +/- ##
=====================================
Coverage 90.3% 90.4%
=====================================
Files 444 444
Lines 22607 22689 +82
=====================================
+ Hits 20421 20503 +82
Misses 2186 2186 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
josephjclark
force-pushed
the
fix-merge-new-workflows
branch
from
089a8c0 to
0e0edd0
Compare
Security Review |
7 tasks
elias-ba
approved these changes
Jun 9, 2026
elias-ba
left a comment
elias-ba
left a comment
Contributor
There was a problem hiding this comment.
Hey @josephjclark nice work man, thanks a lot for taking this one. The credential remap reads cleanly, and matching on the shared credential and rewriting it in the merged document is a sound approach.
I've opened #4842 on top to cover a couple of cases this doesn't reach yet: freeing a deleted workflow's name on merge, which is the other cause behind the "validation error", and extending the new-credential pre-fill to the full ancestor chain so a credential also survives a merge from a nested sandbox. Together I think the two PRs close #4831 and #4834.
* fix: free up workflow name on merge/provisioner soft-delete
A workflow deleted as a side effect of a merge kept its original name while
hidden, so the (name, project_id) unique index still treated the name as taken.
A later merge that recreated a workflow with that name was rejected, surfacing
as a generic merge validation error.
Apply the same name-freeing the UI delete path already uses (rename to
name_del) at the provisioner soft-delete step, so every delete path releases
the name consistently.
* fix: surface nested validation errors on sandbox merge failure
Merge failures whose error was nested in an association (a workflow name, a job
credential) had no top-level error and collapsed into an opaque "validation
error" message. Walk the changeset's nested errors so the real reason is shown
instead.
* fix: add sandbox credentials to the full ancestor chain on creation
A credential created in a sandbox was added only to the sandbox and the root
project, but a merge targets the immediate parent and can target any ancestor.
For a sandbox nested two or more levels deep the credential never reached the
parent and was dropped on merge. Pre-select the whole ancestor chain so it
survives a merge into any ancestor.
* refactor: define the workflow soft-delete transition once
Both the workflows list (mark_for_deletion) and the provisioner (YAML, CLI,
GitHub sync, sandbox merge) soft-delete workflows, but each spelled out the
transition by hand: set deleted_at, then free the name. The name-freeing had
drifted out of the provisioner path, which is what let a merge leave a hidden
workflow holding its name.
Extract a single soft_delete_changeset/1 in the Workflows context that sets
deleted_at and frees the name together, and route both paths through it, so the
transition has one definition and cannot drift again. Replaces the
per-path rename helper. Adds a unit test on the primitive alongside the existing
both-path coverage.
* chore: trim comments and changelog to essentials
* feat: classify sandbox merge failures into user-facing reasons
The merge screen used to render a generic 'validation error', and an attempt to
surface the raw changeset risked leaking schema field paths to users.
Classify merge failures in the domain (Sandboxes.merge) into typed reasons: a
named workflow-name conflict, or a generic validation failure. The view maps
each to curated copy and no longer inspects changesets. The full validation
detail is logged at warning level for diagnosis, so users get clean copy while
engineers keep the specifics.
* chore: update changelog wording for merge error message
* feat: send unclassified merge failures to Sentry
Split merge-failure logging by recognisability: a workflow name conflict is
user-actionable, so it logs at warning. An unclassified validation failure is a
mode we don't yet handle, so it logs at error, which reaches Sentry, giving
proactive signal to recognise and name it. Detail stays in the log message
(not metadata) so it remains visible to logs-only operators.
* fix: tidy merge failure copy and stop leaking raw reasons
Drop the 'contact support if it persists' wording (not a flash style used in
this app) for the short, direct house style. Stop showing inspect(reason) to
the user for an unexpected failure: log it at error (Sentry) and show a generic
message instead. Keep passing through human-readable string errors such as the
collection-sync message.
* fix: satisfy credo and dialyzer
Move require Logger after the alias block (credo ordering), and widen the
merge_error type to include the string-error case the merge can actually
return, so the spec matches the success typing.
* test: assert nested credential pre-fill at the LiveView level
The settings-page test still asserted the old root-only pre-fill; update it to
expect the full ancestor chain, matching the ancestor-chain change.
* refactor: drop unreachable binary merge-error handling
A merge can never fail with a bare string: sync_collections raises rather than
returning an error, and every import_document error path returns a changeset,
a usage-limit message, or an atom. So the string-passthrough clauses (and the
String.t() in merge_error) were dead. Remove them, and point the merge-failure
view test at a real merge_error reason instead of a fabricated binary.
* refactor: log raw merge-error messages instead of interpolating
The placeholder interpolation only ran for messages carrying %{...} opts, so it
was untested, and String.to_existing_atom/1 could raise on an unexpected
placeholder key while formatting a failure for the log. Log the raw message
instead: simpler, can't crash, and covered by the existing classify tests.
* refactor: simplify merge-failure handling to a generic message
Per review, drop the per-cause merge-error classification and bespoke user
messages (the workflow-name-conflict case in particular was a collision with a
hidden deleted row, so its advice wasn't actionable). The user now sees one
generic message; usage-limit messages still pass through. Every merge failure
is logged at error so it surfaces in Sentry, since a failed merge is sensitive
and we want to be aware of it.
* fix: add retry hint to the generic merge-failure message
* feat: attach sandbox credentials to the target at merge time
Replace the credential pre-fill (which speculatively attached new credentials to
ancestor projects at creation) with merge-time attachment. The merge modal now
diffs the sandbox's credentials against the target and shows the ones that would
be dropped as a deselectable picklist, all selected by default. On merge, the
selected credentials are attached to the chosen target inside the transaction,
before the remap, so they carry over instead of being dropped; deselected ones
stay dropped. default_project_credentials reverts to the active project only.
* feat: select-all for the merge credential picklist; tighten heading
Rename the heading to 'Credentials to add' and give the credential picklist the
same tri-state select-all checkbox the workflows list has, with a matching
toggle-all-credentials handler and a selected-count.
* fix: keep credential selections across merge-modal form changes
The credential checkboxes share the merge form, so toggling one fires
select-merge-target, which was resetting the selection to all and snapping the
box back. Preserve the current selection across the change (keeping ones still
in the diff, defaulting newly-appeared credentials to selected), mirroring how
workflow selections are preserved.
Collaborator
Author
|
@elias-ba changes are merged in @theroinaochieng this ought to get released to prod as soon as possible, it fixes a big blocking user bug. It ought to have another review but it's quite isolated. Maybe @midigofrank can give it a real quick look |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
EDIT
This PR has been updated to include #4842
Instead of attaching credentials in the UI at create time, @elias-ba's work now attached credentials at merge time, and tells the user about it. Big improvement which removes The Ick.
Description
This PR ensures that when merging a sandbox, any credentials added to the sandbox get properly mapped to a matching credential on the parent.
When adding a new credential, the UI will auto-fill to also add the credential to the parent.
Closes #4831 #4834
Discussion
I'm sure I've tested this back in the early days of sandboxes. I can visualise doing it. Did we break it? Am I misremembering? Most likely I didn't test properly after the environments work.
The original fix is to map credentials from sandbox -> parent on merge, which fixes most of the problems. But that still leaves an issue when you create a whole new credential on the sandbox: after merging, the credential is not linked to the parent project. What happens then? Do we auto-create the credential? Probably but it feels a little bit icky.My solution is to handle this in the UI: when creating a new credential within a sandbox, automatically add it to the parent project. Now we ensure it'll survive on merge.Validation steps
Run the following tests on a new sandbox and merge
Also need to ensure existing production behaviour isn't broken
AI Usage
Please disclose whether you've used AI anywhere in this PR (it's cool, we just
want to know!):
You can read more details in our
Responsible AI Policy
Pre-submission checklist
/reviewwith Claude Code)
(e.g.,
:owner,:admin,:editor,:viewer)