molecule-ai/molecule-core
35
0
Fork 2
Code Issues 16 Pull Requests 2 Actions 49 Packages Projects Releases Wiki Activity

review: drop destructive Override + single-fetch configuredKeys

Browse Source 
Self-review of #2460 found two issues:

1. Critical: Override button in ProviderPickerModal called
   /settings/secrets when no workspaceId, overwriting the GLOBAL
   secret used by every workspace. The only consumers of this
   modal today (TemplatePalette, EmptyState via useTemplateDeploy)
   never pass workspaceId, so Override was always destructive.
   Removed entirely — the picker still solves the user-reported
   bug (always-ask + reuse saved keys); per-workspace key override
   can be a separate PR that plumbs secrets through POST /workspaces.

2. Optional: /settings/secrets was being fetched twice — once
   inside checkDeploySecrets (silently) and again in the hook to
   populate configuredKeys. Surfaced configuredKeys on
   PreflightResult so the hook re-uses the existing fetch.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Hongming Wang 2026-05-01 13:40:58 -07:00
parent 0608e15ab3
commit 3ba924d174
5 changed files with 72 additions and 52 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

28
canvas/src/components/MissingKeysModal.tsx
View File

@ -400,28 +400,12 @@ function ProviderPickerModal({
<div className="text-[9px] font-mono text-zinc-500">{entry.key}</div>
</div>
{entry.saved && (
<div className="flex items-center gap-1.5">
<span className="text-[9px] text-emerald-400 bg-emerald-900/30 px-1.5 py-0.5 rounded flex items-center gap-1">
<svg width="8" height="8" viewBox="0 0 8 8" fill="none" aria-hidden="true">
<path d="M1.5 4L3.5 6L6.5 2" stroke="currentColor" strokeWidth="1.2" strokeLinecap="round" strokeLinejoin="round" />
</svg>
Saved
</span>
{/* Allow override when the saved state came from a
pre-configured global secret — the user may want
to use a different key for this workspace. */}
{configuredKeys?.has(entry.key) && (
<button
type="button"
onClick={() =>
updateEntry(index, { saved: false, value: "" })
}
className="text-[9px] text-zinc-400 hover:text-zinc-200 underline"
>
Override
</button>
)}
</div>
<span className="text-[9px] text-emerald-400 bg-emerald-900/30 px-1.5 py-0.5 rounded flex items-center gap-1">
<svg width="8" height="8" viewBox="0 0 8 8" fill="none" aria-hidden="true">
<path d="M1.5 4L3.5 6L6.5 2" stroke="currentColor" strokeWidth="1.2" strokeLinecap="round" strokeLinejoin="round" />
</svg>
Saved
</span>
)}
</div>

19
canvas/src/hooks/__tests__/useTemplateDeploy.test.tsx
View File

@ -129,6 +129,7 @@ beforeEach(() => {
missingKeys: [],
providers: [],
runtime: "claude-code",
configuredKeys: new Set(),
});
mockApiPost.mockResolvedValue({ id: "ws-new" });
// Default: secrets endpoint returns nothing so the picker
@ -232,6 +233,7 @@ describe("useTemplateDeploy — preflight failure modes", () => {
missingKeys: ["ANTHROPIC_API_KEY"],
providers: [],
runtime: "claude-code",
configuredKeys: new Set(),
});
const onDeployed = vi.fn();
@ -259,6 +261,7 @@ describe("useTemplateDeploy — modal lifecycle", () => {
missingKeys: ["ANTHROPIC_API_KEY"],
providers: [],
runtime: "claude-code",
configuredKeys: new Set(),
});
const onDeployed = vi.fn();
const { result, rerender } = renderHook(() =>
@ -293,6 +296,7 @@ describe("useTemplateDeploy — modal lifecycle", () => {
missingKeys: ["ANTHROPIC_API_KEY"],
providers: [],
runtime: "claude-code",
configuredKeys: new Set(),
});
const { result, rerender } = renderHook(() => useTemplateDeploy());
@ -345,11 +349,8 @@ describe("useTemplateDeploy — multi-provider always-ask flow", () => {
{ id: "ANTHROPIC_API_KEY", label: "Anthropic", envVars: ["ANTHROPIC_API_KEY"] },
],
runtime: "hermes",
configuredKeys: new Set(["MINIMAX_API_KEY", "ANTHROPIC_API_KEY"]),
});
mockApiGet.mockResolvedValueOnce([
{ key: "MINIMAX_API_KEY", has_value: true, created_at: "", updated_at: "" },
{ key: "ANTHROPIC_API_KEY", has_value: true, created_at: "", updated_at: "" },
]);
const { result, rerender } = renderHook(() => useTemplateDeploy());
await act(async () => {
@ -381,6 +382,7 @@ describe("useTemplateDeploy — multi-provider always-ask flow", () => {
{ id: "ANTHROPIC_API_KEY", label: "Anthropic", envVars: ["ANTHROPIC_API_KEY"] },
],
runtime: "hermes",
configuredKeys: new Set(),
});
const { result, rerender } = renderHook(() => useTemplateDeploy());
@ -408,6 +410,7 @@ describe("useTemplateDeploy — multi-provider always-ask flow", () => {
{ id: "ANTHROPIC_API_KEY", label: "Anthropic", envVars: ["ANTHROPIC_API_KEY"] },
],
runtime: "hermes",
configuredKeys: new Set(),
});
const { result, rerender } = renderHook(() => useTemplateDeploy());
@ -449,7 +452,11 @@ describe("useTemplateDeploy — multi-provider always-ask flow", () => {
expect(onDeployed).toHaveBeenCalledWith("ws-new");
});
it("secrets fetch failure still opens picker (empty configuredKeys)", async () => {
it("empty configuredKeys (preflight defensive fallback) still opens picker", async () => {
// checkDeploySecrets falls back to an empty Set when the
// /settings/secrets endpoint errors — the modal must still
// open so the user isn't blocked, just with every entry
// rendered as input rather than Saved.
mockCheckDeploySecrets.mockResolvedValueOnce({
ok: true,
missingKeys: [],
@ -458,8 +465,8 @@ describe("useTemplateDeploy — multi-provider always-ask flow", () => {
{ id: "ANTHROPIC_API_KEY", label: "Anthropic", envVars: ["ANTHROPIC_API_KEY"] },
],
runtime: "hermes",
configuredKeys: new Set(),
});
mockApiGet.mockRejectedValueOnce(new Error("secrets fetch down"));
const { result, rerender } = renderHook(() => useTemplateDeploy());
await act(async () => {

26
canvas/src/hooks/useTemplateDeploy.tsx
View File

@ -6,7 +6,6 @@ import {
checkDeploySecrets,
resolveRuntime,
type PreflightResult,
type SecretEntry,
type Template,
} from "@/lib/deploy-preflight";
import { MissingKeysModal } from "@/components/MissingKeysModal";
@ -45,15 +44,14 @@ export interface UseTemplateDeployOptions {
/** Paired template + preflight result carried through the "user
* clicked deploy → modal opens → keys saved → retry" loop. Named
* so the `useState` generic and any future signature change have
* a single place to track. `configuredKeys` lets the modal mark
* pre-saved entries (global secrets) without re-prompting — the
* a single place to track. `preflight.configuredKeys` lets the
* modal mark pre-saved entries without re-prompting — the
* template-deploy "always ask" flow surfaces the picker even when
* preflight.ok is true so the user can pick a different provider
* per workspace. */
interface MissingKeysInfo {
template: Template;
preflight: PreflightResult;
configuredKeys: Set<string>;
}
export interface UseTemplateDeployResult {
@ -157,21 +155,7 @@ export function useTemplateDeploy(
const shouldShowPicker =
!preflight.ok || preflight.providers.length >= 2;
if (shouldShowPicker) {
// Read the secret set the modal needs to mark pre-saved
// entries. We did this inside checkDeploySecrets too but
// didn't surface it; pull it again so a slow secrets
// endpoint failing here doesn't block the modal — empty
// set just means everything renders as input.
let configuredKeys = new Set<string>();
try {
const secrets = await api.get<SecretEntry[]>("/settings/secrets");
configuredKeys = new Set(
secrets.filter((s) => s.has_value).map((s) => s.key),
);
} catch {
// Empty set — modal will render every entry as input.
}
setMissingKeysInfo({ template, preflight, configuredKeys });
setMissingKeysInfo({ template, preflight });
setDeploying(null);
return;
}
@ -201,7 +185,7 @@ export function useTemplateDeploy(
? "Configure Workspace"
: undefined;
const modalDescription = allConfigured
? "Pick the provider and model for this workspace. Saved API keys are reused — click Override to use a different one."
? "Pick the provider and model for this workspace. Saved API keys are reused automatically."
: undefined;
const modal: ReactNode = (
<MissingKeysModal
@ -209,7 +193,7 @@ export function useTemplateDeploy(
missingKeys={missingKeysInfo?.preflight.missingKeys ?? []}
providers={missingKeysInfo?.preflight.providers ?? []}
runtime={missingKeysInfo?.preflight.runtime ?? ""}
configuredKeys={missingKeysInfo?.configuredKeys}
configuredKeys={missingKeysInfo?.preflight.configuredKeys}
modelSuggestions={isMultiProvider ? modelSuggestions : undefined}
initialModel={isMultiProvider ? initialModel : undefined}
title={modalTitle}

21
canvas/src/lib/__tests__/deploy-preflight.test.ts
View File

@ -244,5 +244,26 @@ describe("checkDeploySecrets", () => {
const result = await checkDeploySecrets(LANGGRAPH);
expect(result.ok).toBe(false);
expect(result.missingKeys).toEqual(["OPENAI_API_KEY"]);
// Empty Set on fetch failure — useTemplateDeploy relies on this
// so the picker still opens with every entry rendered as input.
expect(result.configuredKeys).toEqual(new Set());
});
it("surfaces configuredKeys (has_value=true entries only) so callers skip a second fetch", async () => {
(global.fetch as ReturnType<typeof vi.fn>).mockResolvedValueOnce({
ok: true,
json: () =>
Promise.resolve([
{ key: "ANTHROPIC_API_KEY", has_value: true, created_at: "", updated_at: "" },
{ key: "OPENROUTER_API_KEY", has_value: false, created_at: "", updated_at: "" },
{ key: "RANDOM_OTHER_KEY", has_value: true, created_at: "", updated_at: "" },
]),
} as Response);
const result = await checkDeploySecrets(HERMES);
// Only has_value=true entries belong in the set.
expect(result.configuredKeys).toEqual(
new Set(["ANTHROPIC_API_KEY", "RANDOM_OTHER_KEY"]),
);
});
});

30
canvas/src/lib/deploy-preflight.ts
View File

@ -91,6 +91,12 @@ export interface PreflightResult {
* required (AllKeysModal renders the N envVars inline). */
providers: ProviderChoice[];
runtime: string;
/** Set of env var names already configured (i.e. `has_value: true`) at
* the relevant scope (workspace if `workspaceId` was passed, otherwise
* global). Surfaced so callers can mark pre-saved entries in the
* picker without making a second `/settings/secrets` round trip.
* Empty Set on secrets-endpoint failure (treated as "nothing set"). */
configuredKeys: Set<string>;
}
/* ---------- Provider options ---------- */
@ -235,7 +241,13 @@ export async function checkDeploySecrets(
if (providers.length === 0) {
// Template declares no env requirements — nothing to preflight.
return { ok: true, missingKeys: [], providers: [], runtime };
return {
ok: true,
missingKeys: [],
providers: [],
runtime,
configuredKeys: new Set(),
};
}
let configured: Set<string>;
@ -254,7 +266,13 @@ export async function checkDeploySecrets(
}
if (findSatisfiedProvider(providers, configured)) {
return { ok: true, missingKeys: [], providers, runtime };
return {
ok: true,
missingKeys: [],
providers,
runtime,
configuredKeys: configured,
};
}
// Nothing configured — surface every candidate env var so the modal
@ -262,5 +280,11 @@ export async function checkDeploySecrets(
const missingKeys = Array.from(
new Set(providers.flatMap((p) => p.envVars)),
);
return { ok: false, missingKeys, providers, runtime };
return {
ok: false,
missingKeys,
providers,
runtime,
configuredKeys: configured,
};
}