Skip to content

fix: compare extension configs before skipping add_extension#7650

Merged
wpfleger96 merged 2 commits intomainfrom
wpfleger/extension-config
Mar 5, 2026
Merged

fix: compare extension configs before skipping add_extension#7650
wpfleger96 merged 2 commits intomainfrom
wpfleger/extension-config

Conversation

@wpfleger96
Copy link
Copy Markdown
Collaborator

@wpfleger96 wpfleger96 commented Mar 4, 2026 •
edited
Loading

Summary

  • add_extension silently dropped config updates when an extension with the same name was already loaded, returning Ok(()) regardless of whether the config differed
  • Now compares incoming ExtensionConfig against the existing one using derived PartialEq — identical configs still no-op, differing configs remove the stale extension and re-add with the new config
  • Adds debug-level logging when a config change triggers an extension restart

Motivation

In the internal Block slackbot, per-session env vars (SLACK_ORIGIN_CHANNEL) are injected into the Slack MCP extension config via add_extension. But load_extensions_from_session typically wins the race and loads the extension from stored session config (without the env var) before the caller's add_extension arrives. The caller's config update was silently dropped.

The workaround (remove_extension + add_extension) restarts the subprocess on every session setup, adding latency and unnecessary churn.

Closes #7647

Test plan

  • test_add_extension_noop_on_identical_config — verifies identical config returns Ok(()) without removing the extension
  • test_add_extension_replaces_extension_on_config_change — verifies differing config removes the stale extension before attempting re-add
  • CI passes (local build environment currently has an unrelated Xcode/v8 linker issue)

@wpfleger96
fix: compare extension configs before skipping add_extension
f762247 
add_extension silently dropped config updates when an extension with the
same name was already loaded. This prevented callers from updating envs,
args, or other config without first calling remove_extension (which
restarts the subprocess unnecessarily).

Now compares incoming config against the existing one using derived
PartialEq. Identical configs still no-op; differing configs remove the
stale extension and proceed with the new config.

Closes #7647
@wpfleger96 wpfleger96 marked this pull request as ready for review March 4, 2026 18:44
chatgpt-codex-connector[bot]
chatgpt-codex-connector bot reviewed Mar 4, 2026
View reviewed changes
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: f762247b56

ℹ️ 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".

@wpfleger96
fix: preserve old extension if replacement client creation fails
479882e 
The previous approach removed the old extension before creating the new
client. If client creation failed (bad command, transient network error),
the session was left with no extension at all — a regression from the
original behavior of keeping the loaded extension on conflict.

Instead, let HashMap::insert at the end of add_extension overwrite the
old entry only after the new client is successfully created. On failure,
the old extension stays intact as a safe fallback.
@wpfleger96 wpfleger96 enabled auto-merge March 4, 2026 20:19
@wpfleger96 wpfleger96 added this pull request to the merge queue Mar 5, 2026
Merged via the queue into main with commit 5f48087 Mar 5, 2026
20 of 21 checks passed
@wpfleger96 wpfleger96 deleted the wpfleger/extension-config branch March 5, 2026 20:37
wpfleger96 added a commit that referenced this pull request Mar 6, 2026
@wpfleger96
Merge remote-tracking branch 'origin/main' into wpfleger/tool-respons…
d96cdff 
…e-issue

* origin/main:
  Fix max turns configuration (#7612)
  feat: add base path field to custom provider configuration (#7614)
  fix: compare extension configs before skipping add_extension (#7650)
  chore(release): release version 1.27.0 (minor) (#7611)
  feat: better private channel detection, bot version debugging (#7680)
  chore(deps): bump svgo from 3.3.2 to 3.3.3 in /documentation (#7667)
  fix: only add viewable channels to bot context (#7678)
  chore: added a recipe to help identify high risk change prs for testing (#7651)
  fix: make sure platform binary exists (#7676)
wpfleger96 added a commit that referenced this pull request Mar 6, 2026
@wpfleger96
Merge remote-tracking branch 'origin/main' into wpfleger/socket-support
5716182 
* origin/main: (29 commits)
  Update to rmcp 1.1.0 (#7619)
  Fix max turns configuration (#7612)
  feat: add base path field to custom provider configuration (#7614)
  fix: compare extension configs before skipping add_extension (#7650)
  chore(release): release version 1.27.0 (minor) (#7611)
  feat: better private channel detection, bot version debugging (#7680)
  chore(deps): bump svgo from 3.3.2 to 3.3.3 in /documentation (#7667)
  fix: only add viewable channels to bot context (#7678)
  chore: added a recipe to help identify high risk change prs for testing (#7651)
  fix: make sure platform binary exists (#7676)
  fix(shell): replace global static output buffer with per-instance TempDir (#7632)
  opt: remove timestamped config file backup (#7618)
  chore(deps): bump @hono/node-server from 1.19.9 to 1.19.10 in /ui/desktop (#7662)
  chore(deps): bump hono from 4.12.3 to 4.12.5 in /evals/open-model-gym/mcp-harness (#7661)
  chore(deps): bump hono from 4.12.2 to 4.12.5 in /ui/desktop (#7660)
  fix: resolve parameters in initial message with autosubmit (#7659)
  fix: this should not be blocked (#7656)
  Relax the assertion for the model list ACP test (#7653)
  fix: add analyzer extension in recipe to maintain backwards compatibility  (#7652)
  docs: add GOOSE_INPUT_LIMIT environment variable documentation (#7299)
  ...
michaelneale added a commit that referenced this pull request Mar 6, 2026
@michaelneale
Merge remote-tracking branch 'origin/main' into micn/claude-sub
10c783f 
* origin/main: (40 commits)
  fix: make TLS configurable in goosed agent via GOOSE_TLS env var (#7686)
  Update to rmcp 1.1.0 (#7619)
  Fix max turns configuration (#7612)
  feat: add base path field to custom provider configuration (#7614)
  fix: compare extension configs before skipping add_extension (#7650)
  chore(release): release version 1.27.0 (minor) (#7611)
  feat: better private channel detection, bot version debugging (#7680)
  chore(deps): bump svgo from 3.3.2 to 3.3.3 in /documentation (#7667)
  fix: only add viewable channels to bot context (#7678)
  chore: added a recipe to help identify high risk change prs for testing (#7651)
  fix: make sure platform binary exists (#7676)
  fix(shell): replace global static output buffer with per-instance TempDir (#7632)
  opt: remove timestamped config file backup (#7618)
  chore(deps): bump @hono/node-server from 1.19.9 to 1.19.10 in /ui/desktop (#7662)
  chore(deps): bump hono from 4.12.3 to 4.12.5 in /evals/open-model-gym/mcp-harness (#7661)
  chore(deps): bump hono from 4.12.2 to 4.12.5 in /ui/desktop (#7660)
  fix: resolve parameters in initial message with autosubmit (#7659)
  fix: this should not be blocked (#7656)
  Relax the assertion for the model list ACP test (#7653)
  fix: add analyzer extension in recipe to maintain backwards compatibility  (#7652)
  ...
Abhijay007 pushed a commit to Abhijay007/goose that referenced this pull request Mar 6, 2026
@wpfleger96 @Abhijay007
fix: compare extension configs before skipping add_extension (block#7650
d80cf13 
)
wpfleger96 added a commit that referenced this pull request Mar 6, 2026
@wpfleger96
Merge remote-tracking branch 'origin/main' into wpfleger/summon
ec87472 
* origin/main: (59 commits)
  fix: restore smart-approve mode (#7690)
  fix: make TLS configurable in goosed agent via GOOSE_TLS env var (#7686)
  Update to rmcp 1.1.0 (#7619)
  Fix max turns configuration (#7612)
  feat: add base path field to custom provider configuration (#7614)
  fix: compare extension configs before skipping add_extension (#7650)
  chore(release): release version 1.27.0 (minor) (#7611)
  feat: better private channel detection, bot version debugging (#7680)
  chore(deps): bump svgo from 3.3.2 to 3.3.3 in /documentation (#7667)
  fix: only add viewable channels to bot context (#7678)
  chore: added a recipe to help identify high risk change prs for testing (#7651)
  fix: make sure platform binary exists (#7676)
  fix(shell): replace global static output buffer with per-instance TempDir (#7632)
  opt: remove timestamped config file backup (#7618)
  chore(deps): bump @hono/node-server from 1.19.9 to 1.19.10 in /ui/desktop (#7662)
  chore(deps): bump hono from 4.12.3 to 4.12.5 in /evals/open-model-gym/mcp-harness (#7661)
  chore(deps): bump hono from 4.12.2 to 4.12.5 in /ui/desktop (#7660)
  fix: resolve parameters in initial message with autosubmit (#7659)
  fix: this should not be blocked (#7656)
  Relax the assertion for the model list ACP test (#7653)
  ...
@github-actions github-actions bot mentioned this pull request Mar 10, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chatgpt-codex-connector chatgpt-codex-connector[bot] chatgpt-codex-connector[bot] left review comments

@jamadeo jamadeo jamadeo approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

add_extension silently drops config updates when extension name already loaded

2 participants

@wpfleger96 @jamadeo