From 5d695f8546b1a0f5dfc7fc9a2f5ac5a4054b59c8 Mon Sep 17 00:00:00 2001 From: Guilhem Lemouel Date: Tue, 25 Aug 2026 18:02:50 +0200 Subject: [PATCH] fix: fail closed everywhere the import cannot tell MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Codex was right that the occupant-type guard failed open: a getResource that threw became `undefined`, which passed the mismatch test and left filling enabled — so a transient read failure still overwrote the resource the guard exists to protect. Only a read that succeeds and answers with exactly this type now permits the write; a failed read, a missing type and any other type all refuse. That was the same "cannot tell, so proceed" this branch already fixed once in settle(), so the rest of the wizard was swept for it. Two more: findBlankResources dropped a row whenever getResource threw, on the assumption that meant absent. Only a 404 means absent — and that failure the import already reported. Any other error is a read that did not complete, which says nothing about whether the credential needs filling; dropping the row reports "all set" over one nobody filled. The row now stays and offers no action, since none of them can be safe about a path this cannot read. A resource type whose schema would not load left `required` empty, which reads as "nothing missing" — so a half-filled resource passed as done. It stays on the checklist; it just cannot name which fields are short. The other four catches were checked and are already closed in the right direction: probeWorkspace reports absent so the caller creates rather than adopts, probeMigrationsApplied answers undefined which settles to a non-actionable row, and afterWizard keeps whatever the run last said. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01Xg8vXUuHCH3aRkf91sfxjx --- .../src/lib/components/AppConnectInner.svelte | 31 +++++++---- .../src/lib/components/ImportSetupStep.svelte | 54 +++++++++++++++---- 2 files changed, 64 insertions(+), 21 deletions(-) diff --git a/frontend/src/lib/components/AppConnectInner.svelte b/frontend/src/lib/components/AppConnectInner.svelte index a461a6cb64..a9a0837d1f 100644 --- a/frontend/src/lib/components/AppConnectInner.svelte +++ b/frontend/src/lib/components/AppConnectInner.svelte @@ -760,18 +760,29 @@ // of another type sitting where the project wanted one of ours would otherwise have // its value replaced with credentials for a different provider, while keeping its // own type — destroying a working resource that has nothing to do with the import. - let filling = exists && !!fillPath && path === fillPath + const filling = exists && !!fillPath && path === fillPath if (filling) { - const occupantType = await ResourceService.getResource({ - workspace: effectiveWorkspace, - path - }) - .then((r) => r?.resource_type) - .catch(() => undefined) - if (occupantType && occupantType !== resourceType) { + // Fails closed. Only a read that succeeds and answers with exactly this type + // permits the write — a failed read, a missing type, or any other type all + // refuse. Letting "could not tell" through is how the overwrite this guard + // exists to stop would happen anyway, on the one occasion the check was needed + // and could not run. + let occupantType: string | undefined + try { + occupantType = ( + await ResourceService.getResource({ workspace: effectiveWorkspace, path }) + )?.resource_type + } catch (e: any) { throw Error( - `Resource at path ${path} is a ${occupantType} resource, not ${resourceType}. ` + - `Move or rename it, then import again.` + `Could not read what is already at ${path} (${e?.body ?? e?.message ?? e}), ` + + `so it will not be written over. Try again.` + ) + } + if (occupantType !== resourceType) { + throw Error( + `Resource at path ${path} is ${ + occupantType ? `a ${occupantType} resource` : 'of an unknown type' + }, not ${resourceType}. Move or rename it, then import again.` ) } } diff --git a/frontend/src/lib/components/ImportSetupStep.svelte b/frontend/src/lib/components/ImportSetupStep.svelte index 16c704150b..ae22622bee 100644 --- a/frontend/src/lib/components/ImportSetupStep.svelte +++ b/frontend/src/lib/components/ImportSetupStep.svelte @@ -76,6 +76,12 @@ * shipped — and to make sure nothing offers to write over what is there. */ occupiedBy?: string + /** + * The resource could not be read, so nothing here knows whether it needs filling. Kept + * on the checklist rather than dropped: a read that fails is not evidence the resource + * is absent, and removing the row reports "all set" over a credential nobody filled. + */ + unreadable?: boolean } let loading = $state(true) @@ -268,8 +274,20 @@ if (found?.resource_type && found.resource_type !== r.resource_type) { occupiedBy = found.resource_type } - } catch { - continue // Not there — the import reported that failure already. + } catch (e: any) { + // A 404 is the import having failed to create it, which it reported itself. + // Any other failure is a read this could not complete, which says nothing about + // whether the resource is there or needs filling — so the row stays. + if (e?.status === 404) continue + out.push({ + path: r.path, + resourceType: r.resource_type, + missing: [], + done: false, + justSaved: false, + unreadable: true + }) + continue } const filled = new Set( value && typeof value === 'object' @@ -279,15 +297,21 @@ : [] ) let required: string[] = [] + // A type whose schema will not load leaves `required` empty, which reads as "nothing + // missing" — and a half-filled resource would drop off the checklist as done. The + // row is kept instead; it just cannot name which fields are short. + let requirementsUnknown = false try { const schema = (await ResourceService.getResourceType({ workspace, path: r.resource_type })) ?.schema as { required?: string[] } | undefined required = schema?.required ?? [] - } catch {} + } catch { + requirementsUnknown = true + } const missing = required.filter((k) => !filled.has(k)) // A conflicting occupant is always listed, however full its value looks: the row is // what tells the user the project is missing a resource it shipped. - if (occupiedBy || missing.length > 0 || filled.size === 0) { + if (occupiedBy || requirementsUnknown || missing.length > 0 || filled.size === 0) { out.push({ path: r.path, resourceType: r.resource_type, @@ -604,7 +628,8 @@ >