Skip to content

fix(ui): preserve pending env vars in Add Extension form#9285

Open
williams145 wants to merge 3 commits into
aaif-goose:mainfrom
williams145:fix/8969-env-vars-not-saved
Open

fix(ui): preserve pending env vars in Add Extension form#9285
williams145 wants to merge 3 commits into
aaif-goose:mainfrom
williams145:fix/8969-env-vars-not-saved

Conversation

@williams145
Copy link
Copy Markdown
Contributor

Summary

When adding an MCP extension via the desktop UI, env vars typed into the empty input row at the bottom of the Environment Variables section are silently dropped if the user clicks Save without first clicking "+ Add". MCPs that need secrets (most of them — API tokens, JWTs, etc.) then fail at activation with:

Failed to add extension: invalid config: Failed to fetch secret 'KEY_NAME' from config: Configuration value not found: KEY_NAME

This is an inconsistency with the sibling HeadersSection, which already handles this case correctly via pendingHeader + getFinalHeaders(). This PR mirrors the same pattern for env vars.

Change

EnvVarsSection.tsx now passes the pending { key, value } object alongside the boolean, the same way HeadersSection.tsx already does:

 React.useEffect(() => {
   const hasPendingInput = newKey.trim() !== '' || newValue.trim() !== '';
+  const pendingEnvVar =
+    newKey.trim() && newValue.trim() ? { key: newKey, value: newValue } : null;
-  onPendingInputChange?.(hasPendingInput);
+  onPendingInputChange?.(hasPendingInput, pendingEnvVar);
 }, [newKey, newValue, onPendingInputChange]);

ExtensionModal.tsx gets a matching pendingEnvVar state, a handlePendingEnvVarChange callback, and a getFinalEnvVars() helper (modeled on the existing getFinalHeaders()). handleSubmit now uses getFinalEnvVars() so the pending entry is included in both the storeSecret calls and the data passed to onSubmit.

Fixes #8969

Test plan

  • Added regression test in ExtensionModal.test.tsx: typing JWT_TOKEN + my_very_long_token into the env-var row and clicking Submit without "+ Add" — verifies upsertConfig({ is_secret: true, key, value }) is called and onSubmit receives the entry with isEdited: true. This test fails on main and passes with this change.
  • Added negative test: typing only a key (no value) does not trigger any upsertConfig call and does not appear in submitted data.
  • Manual verification (recommended for reviewer):
    1. Run just run-ui
    2. Settings → Extensions → Add Extension
    3. Type a name, command, and a JWT_TOKEN + value in the env vars row — do NOT click "+ Add"
    4. Click Save
    5. Expected (after fix): extension activates successfully without "Configuration value not found"

Follow-up (separate PR)

While investigating I noticed EnvVarsSection.tsx:handleEdit (lines 112-125) builds a local newEnvVars array but never sends it back to the parent — so clicking the edit (pencil) icon on an existing •••••••• secret silently fails to mark it as edited. That's a related but distinct bug; I'll file a follow-up PR for it after this one lands.

…8969)

When adding an MCP extension via the desktop UI, env vars typed into
the empty input row at the bottom of the Environment Variables section
were silently dropped if the user clicked Save without first clicking
"+ Add". This caused MCPs that need secrets (most of them) to fail at
activation with `Configuration value not found: KEY_NAME`.

The bug was inconsistent handling between EnvVarsSection and the
sibling HeadersSection: HeadersSection already passes `pendingHeader`
upward via `onPendingInputChange` and the parent uses
`getFinalHeaders()` to include it on submit. EnvVarsSection only passed
a boolean `hasPendingInput`, so the parent had no way to recover the
typed value.

Mirror the headers pattern:

1. Extend `EnvVarsSection.onPendingInputChange` to pass the pending
   `{ key, value }` object alongside the boolean.
2. Add `pendingEnvVar` state and `getFinalEnvVars()` in
   `ExtensionModal` (mirrors `pendingHeader` + `getFinalHeaders()`).
3. Use `getFinalEnvVars()` in `handleSubmit` so the pending entry is
   included in both `storeSecret` calls and the data passed to
   `onSubmit`.

Adds two regression tests in `ExtensionModal.test.tsx`:
- pending env var typed without "+ Add" is captured on submit (the
  exact aaif-goose#8969 repro — fails before this fix, passes after)
- pending env var with only a key (no value) is ignored

Fixes aaif-goose#8969
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 38ab7179c5

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment on lines +303 to +304
if (pendingEnvVar && pendingEnvVar.key.trim() !== '' && pendingEnvVar.value.trim() !== '') {
finalEnvVars.push({ ...pendingEnvVar, isEdited: true });
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Validate pending env var keys before auto-submitting

When a user clicks Save without pressing + Add, getFinalEnvVars() now appends pendingEnvVar based only on non-empty checks, so it bypasses the handleAdd rule that rejects keys containing spaces in EnvVarsSection. In that flow, values like "API KEY" are submitted to storeSecret/onSubmit even though the UI validation message says spaces are invalid, which can lead to invalid extension env configuration being persisted.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed in ff284eegetFinalEnvVars() now checks !pendingEnvVar.key.includes(' ') to match the handleAdd validation.

Douwe Osinga added 2 commits May 18, 2026 10:34
…ngInputChange required

Address codex review: getFinalEnvVars now applies the same space-in-key
validation that handleAdd enforces, preventing bypass when a pending env
var is auto-captured at submit time.

Also make onPendingInputChange non-optional in EnvVarsSectionProps since
it is always provided (matches HeadersSection pattern).

Signed-off-by: Douwe Osinga <douwe@squareup.com>
Copy link
Copy Markdown
Collaborator

@DOsinga DOsinga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Good fix — correctly mirrors the existing pendingHeader/getFinalHeaders() pattern for env vars.

I pushed one commit (ff284ee) to address the codex review comment: getFinalEnvVars() now also rejects keys with spaces, matching the handleAdd validation. Also made onPendingInputChange non-optional in EnvVarsSectionProps since it's always provided.

All 349 desktop tests pass locally. The CI Electron test failure on 38ab717 looks unrelated (annotation points at .github line 21, not any test file).

Note: the original commit is missing Signed-off-by — please add it for DCO compliance (git commit --amend -s + force push, or use the GitHub DCO bot's instructions if available).

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.

Win11 Goose desktop Extentsion(MCP): values of Environment Variables cannot be retrieved by system?

2 participants