Conversation
|
Heads-up on the It's my fix working against a real server, not a regression. The failing test is the pre-existing Fixture update ( For transparency: this is a minimal hand-edit, not a Re-recording against the currently-published Happy to adjust anything here. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 200dd04c14
ℹ️ 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".
| STDIN: {"jsonrpc":"2.0","id":5,"method":"tools/call","params":{"_meta":{"agent-session-id":"test-session-id","agent-tool-call-request-id":"test-id","progressToken":4},"name":"get-structured-content","arguments":{"location":"New York"}}} | ||
| STDOUT: {"result":{"content":[{"type":"text","text":"{\"temperature\":33,\"conditions\":\"Cloudy\",\"humidity\":82}"}],"structuredContent":{"temperature":33,"conditions":"Cloudy","humidity":82}},"jsonrpc":"2.0","id":5} | ||
| STDIN: {"jsonrpc":"2.0","id":6,"method":"tools/call","params":{"_meta":{"agent-session-id":"test-session-id","agent-tool-call-request-id":"test-id","progressToken":5},"name":"trigger-sampling-request","arguments":{"maxTokens":100,"prompt":"Please provide a quote from The Great Gatsby"}}} | ||
| STDIN: {"jsonrpc":"2.0","id":3,"method":"tools/list","params":{"_meta":{"agent-session-id":"test-session-id","progressToken":2}}} |
There was a problem hiding this comment.
This replay now assumes the startup notifications/tools/list_changed messages on lines 5-6 are handled after the first tool list has been cached, so it expects this extra tools/list before get-sum. Those notifications are emitted before the first tools/list, though; if Tokio schedules the rmcp notification handler before get_all_tools_cached records/caches the initial list, the second dispatch reuses the cache and sends tools/call with id 3 instead. Since playback waits for an exact next STDIN match (crates/goose-test/src/mcp/stdio/playback.rs:72-80), this makes the replay timing-dependent.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Indeed, this is timing-dependent. But goose ends up with the correct tool set under either schedule; the two only differ in the wire sequence (whether the post-list_changed re-fetch lands before get-sum or is folded into the initial fetch). So the fix is correct either way — it's the exact-match replay that's sensitive, and it would be for any server that emits tools/list_changed at startup.
Rather than rely on the observed ordering (consistent across CI-Linux and ~20 local runs, but not guaranteed), we could make the replay itself order-tolerant: treat a tools/list as a "floating" request in playback.rs (replay its recorded response wherever goose issues it, and don't fail if it's absent). That would remove the timing-dependency generically.
I could work on this here in this PR, but I would have a (slight?) concern with scope creep. This is my first PR and I'm aiming to keep it very focused.
Could someone provide guidance on whether I should try to implement this in this PR? I'm open to doing it here if the team thinks it makes sense.
|
I had my agent investigate why CI is still failing, and it seems it's due to a change committed to main earlier today. Here's my agents' summary: The two Build and Test TLS Backend failures are the pre-existing breakage from #10148 — main is red on the same job; unrelated to this PR. I guess I can wait until #10148 is fixed EDIT - this was fixed in main, CI pipeline now passes; just awaiting review now. |
200dd04 to
d88d8ea
Compare
|
Hope it's OK to tag @DOsinga - I'm a first time contributor just hoping this PR doesn't get lost in the sea of PRs coming your way. I'm tagging you because it seems you do a lot of the MCP-related merging. Would appreciate any feedback on this first PR. Thanks in advance! |
|
Tested commit The PR's end-to-end test passed 1/1, the cache-related unit tests passed 4/4, and a separate 7-case rig passed:
I also checked the counterfactual. Restoring One behavior worth recording: invalidation is manager-wide, so the next lookup re-lists every extension. The handler also honors the notification when the server did not advertise I found no blocker in the paths I exercised. Scope: I did not run the full Goose suite, and I did not exercise the I can share the fixture and 7-case rig if useful. |
Goose fetches each MCP extension's tools/list once at init and caches it for the session. rmcp's ClientHandler::on_tool_list_changed defaults to a no-op and GooseClient never overrode it, so notifications/tools/list_changed was silently dropped, and the ExtensionManager tool cache (only invalidated on add/remove-extension) went stale. Tools added or removed mid-session never appeared until the app was restarted. Real-world trigger: an stdio proxy that serves an "offline" tool set while its upstream is unreachable, then sends tools/list_changed on recovery -- goose keeps showing the offline tools and the agent wrongly concludes the backend is down. Add a small ToolCacheInvalidator trait in mcp_client.rs. GooseClient holds an Option<Weak<dyn ToolCacheInvalidator>> and overrides on_tool_list_changed to upgrade the Weak and drop the cache via the existing invalidate_tools_cache_and_bump_version(); the next agent turn re-fetches tools/list. The back-reference is Weak, not Arc, so a client never keeps its ExtensionManager (and thus the child MCP process) alive past the session -- this resolves the retain-cycle P1 that the review of the earlier attempt (aaif-goose#8276) flagged. The invalidator is threaded through every client construction path (stdio/child-process, streamable-http, OAuth, unix-socket, builtin in-process). The in-process platform-extension factory does not take it: it is not an MCP transport and cannot emit tools/list_changed. Revives and hardens aaif-goose#8276 (auto-closed as stale, never human-reviewed). Tests: - crates/goose/tests/tool_list_changed_test.rs: end-to-end reproducer that drives a real MCP stdio server (tests/dynamic_tools_server.py) through the extension machinery and rmcp transport, observing the public tool list. Fails on unpatched main (notification dropped, times out waiting for the new tool), passes with the fix. - unit tests for the Weak drop no-op (the P1) and cache hiding/invalidation. Refs aaif-goose#8276 Co-authored-by: makanilani <43491852+makanilani@users.noreply.github.com> Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The mcp_integration_test replay for @modelcontextprotocol/server-everything records that server sending notifications/tools/list_changed at startup. With the tool-cache-refresh fix, goose now honors that notification and re-fetches tools/list, so the recorded request sequence gains one tools/list call before the next tool call. Update the fixture to expect it: insert the re-fetch and shift the following request ids / progressTokens by one. This is a minimal hand-edit rather than a GOOSE_RECORD_MCP re-record on purpose: re-recording against the currently-published server pulls in unrelated drift (a larger tool set, a new roots/list exchange, npm notices). The edit is semantically identical to what a clean re-record against the pinned server would produce, and playback is deterministic across repeated runs. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
d88d8ea to
05cec9e
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 05cec9e06e
ℹ️ 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".
|
Thank you @aeoess for your review! I appreciate you exercising paths well beyond the PR's own test, plus confirming the counterfactual. On the two behaviors you flagged:
Neither is a correctness issue (as you found), but good to have on record — thanks for calling them out. Note: I've since rebased onto current Thanks again for the careful review. |
Invalidating the ExtensionManager cache makes the *next* reply see updated tools, but a reply already in flight keeps passing its up-front `tools` snapshot to the provider on subsequent iterations. So a tool call that unlocks new tools mid-reply wasn't offered to the model until a later reply. Poll the (existing) `tools_cache_version` at each reply-loop iteration and, when it changes, re-run `prepare_tools_and_prompt` via the existing `tools_updated` refresh path, so the updated tool set reaches the model within the same reply. - extension_manager: expose `tools_cache_version()` (already bumped on every cache invalidation). - agent reply loop: snapshot the version next to `tools`; flag a refresh when it moves. - test: `tool_list_changed_reply_loop_test` drives a real reply loop with a stdio server that emits `tools/list_changed` mid-reply and asserts the new tool is offered within the same reply. Fails without the loop change, passes with it. Addresses review feedback on the tools/list_changed PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cc6df5ca77
ℹ️ 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".
`prepare_tools_and_prompt` rebuilds `system_prompt` without the session's project addendum, so every reply-loop refresh path — runtime extension enable, directory hints, and the tools/list_changed version check added earlier in this PR — dropped project instructions for the rest of the reply. The initial prompt appended the addendum, but the next provider turn after a refresh ran without it. Re-append the project addendum after each refresh via a shared `with_project_addendum` helper (also used for the initial prompt). Addresses review feedback on the tools/list_changed PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Reviewed cc6df5c. The change looks good. It uses the existing The new test covers the case we were concerned about: I reviewed the diff against the earlier issue but did not rerun the suite. I don't see any blockers. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3d9d9f7f9f
ℹ️ 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".
The tools/list_changed version poll ran at the end of each reply-loop iteration, so a notification whose handler landed after that poll but before the next provider call was only noticed after a stale model turn — if that turn returned a final answer, a newly published tool was never offered within the reply. Move the version recheck to immediately before the provider call (top of the loop), which also covers iterations reached via an earlier `continue` that bypass the end-of-loop refresh block. This is best-effort by nature: rmcp dispatches the notification handler on a detached task with no completion signal, so a change whose handler has not run yet is caught on a later iteration. The cache invalidation guarantees the notification is never lost — only, in a narrow race, deferred by a turn. Addresses review feedback on the tools/list_changed PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ffa95f9810
ℹ️ 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".
`invalidate_tools_cache_and_bump_version` bumped `tools_cache_version` before clearing `tools_cache`, while `get_all_tools_cached` early-returns on a cached `Some` without a version check. On a multi-threaded runtime the reply loop's version poll could observe the new version, lock the cache before the clear, take the stale early-return, advance its local version snapshot, and then never re-list for the rest of the reply. Clear the cache first, then bump the version. Via the cache-mutex unlock (release) sequenced-before the SeqCst bump, which synchronizes-with the poller's SeqCst version load, an observer of the new version is guaranteed to see the cleared cache and fall through to a refetch. The `version_after == version_before && cache.is_none()` write-back guard is order-independent and unchanged. Addresses review feedback on the tools/list_changed PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aacd89c09
ℹ️ 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".
The reply loop kept a tool-cache version baseline separately from its `tools` vector and read the two at different points, so they could drift: a `notifications/tools/list_changed` handled while `prepare_reply_context` was still building `tools` was recorded as "already consumed" by a later baseline read, and the top-of-loop check then never re-listed for the rest of the reply. Make the version travel with the tools instead of being read separately: - `tools_cache` now stores `(version, tools)`, and `get_all_tools_cached` returns the version the returned tools were built from (the stored version on a cache hit, or `version_before` on a refetch). - surface it via `get_prefixed_tools_with_version` / `list_tools_with_version` (existing `get_prefixed_tools` / `list_tools` stay as version-dropping wrappers, so other callers are untouched), and add it to the tuple returned by `prepare_tools_and_prompt` and to `ReplyContext`. - the reply loop takes its baseline from the version returned alongside the tools at every build/rebuild site, and no longer reads the counter separately. Now baseline == version-of-tools by construction, so the baseline can never drift ahead of the tools it is paired with. The only residual is the inherent async-delivery race (a notification whose handler has not run yet), which is caught on a later turn and never loses a tool. Addresses review feedback on the tools/list_changed PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 84da77bc51
ℹ️ 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".
The previous invalidation cleared the cache and released the lock, then bumped the version in a separate step. A get_all_tools_cached write-back finishing a fetch could acquire the cache lock in that gap, observe the not-yet-bumped version (so its `version_after == version_before` guard passed) and an empty cache, and repopulate `Some((old_version, stale_tools))`. The bump then left a stale entry tagged with the old version that the version poll can't evict, so refreshes kept returning the stale tools until the next invalidation. Hold the cache lock across both the version bump and the clear, so a write-back runs either fully before invalidation (its entry is then cleared) or fully after (its guard fails and it doesn't cache). This preserves the property that an observer of the new version sees the cleared cache. Addresses review feedback on the tools/list_changed PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@alexhancock @DOsinga @jamadeo — small first contribution (fixes #10433): wires MCP tools/list_changed into the ExtensionManager tool cache you all maintain. 2 source + 2 test files, one concern. All codex review comments are resolved, and at least one user has reviewed & tested (see comments from @aeoess). Would any of you have a few minutes for a human look? Happy to adjust anything. |
|
Thank you for the contribution and for continuing to work through the review findings. The evolution of this PR has been useful in showing that the problem is broader than simply wiring Given everything uncovered so far, we don't think we have a sufficiently clear design boundary yet. We have moved #10433 to Accepted / design so we can first agree on the intended semantics and a design that handles these interactions together, rather than continuing to add point fixes here. In particular, we need to decide whether a changed tool must be available during the next provider iteration of the same reply or only on the next user turn; if same-reply visibility is required, we need an explicit update/coordination mechanism rather than relying on loosely timed version polling around rmcp's detached notification handler. We are not closing this PR yet. Please join the design discussion on #10433; your findings and experience implementing this version are valuable input. We'll pause and revisit this PR in four days to give everyone time to respond. |
|
Thank you, Alexis. Your careful review, independent tracing of the next-turn behavior, and the strict replay-fixture update in #10928 were key to getting this over the line. We merged that fixture into #10925, the full CI suite passed, and the fix has now landed. This was very much something we landed together. I’m closing this PR as superseded by #10925, with real appreciation for the work and collaboration here. |
3 participants
Summary
My first contribution to goose — deliberately kept small (2 source + 2 test files,
one concern) and linked to #10433, per CONTRIBUTING's "start small" guidance. I'm very open to feedback & guidance!
Goose fetches each MCP extension's
tools/listonce at init and caches it for thesession. rmcp's
ClientHandler::on_tool_list_changeddefaults to a no-op andGooseClientnever overrode it, sonotifications/tools/list_changedwas silentlydropped, and the
ExtensionManagertool cache (only invalidated onadd/remove-extension) went stale. Tools added or removed mid-session never appeared
until the app was restarted.
This wires the dropped notification into the cache-invalidation path that
ExtensionManageralready has:ToolCacheInvalidatortrait inmcp_client.rs(
async fn invalidate_tools).GooseClientholds anOption<Weak<dyn ToolCacheInvalidator>>and overrideson_tool_list_changedto upgrade theWeakand call it. Weak, not Arc — aclient must never keep its
ExtensionManager(and thus the child MCP process)alive past the session. This is the retain-cycle P1 that the automated review of
the earlier attempt (feat: implement dynamic tool discovery via MCP notifications/tools/li… #8276) flagged.
impl ToolCacheInvalidator for ExtensionManagerdelegates to the existinginvalidate_tools_cache_and_bump_version(); the next agent turn re-fetches viaget_prefixed_tools().(stdio/child-process, streamable-http, OAuth
connect_with_auth, unix-socket,builtin in-process). The in-process platform-extension factory does not take
it — it is not an MCP transport and cannot emit
tools/list_changed; it alreadygets a
Weakextension-manager back-reference viacontext.Scope: 2 source files + 2 new test files, one concern. No new dependencies, no
Cargo.lockchange. Revives and hardens #8276 (auto-closed as stale, neverhuman-reviewed); credit to the original author below.
Testing
New end-to-end reproducer that drives a real MCP stdio server through goose's
own extension machinery and rmcp transport (no mocks, no internal-API pokes) and
observes the public tool list (
get_prefixed_tools, i.e. what the agent sees):crates/goose/tests/tool_list_changed_test.rs+crates/goose/tests/dynamic_tools_server.py(stdlib-only; advertisestools.listChanged=true, servesalpha, and on the firstalphacall emitsnotifications/tools/list_changedthen starts servingbeta).With the fix (this branch) it passes:
cargo test -p goose --test tool_list_changed_testTo see it fail without the fix (i.e. confirm the test actually gates on the
change): the two test files exist only in this commit, so don't check out
main—instead restore just the two source files to
origin/main, keeping the test,then run and restore:
git checkout origin/main -- crates/goose/src/agents/mcp_client.rs \ crates/goose/src/agents/extension_manager.rs cargo test -p goose --test tool_list_changed_test # FAILS (see below) git checkout HEAD -- crates/goose/src/agents/mcp_client.rs \ crates/goose/src/agents/extension_manager.rs # restore the fix(
git revert HEADwould remove the test too — the targeted source-file checkout iswhat isolates "test present, fix absent".)
Unit tests added alongside:
agents::mcp_client::tests::on_tool_list_changed_invalidates_then_noops_after_drop— a live invalidator triggers one invalidation; after the owning manager is
dropped, the
Weakfails to upgrade and the handler is a safe no-op (guards theP1).
agents::extension_manager::tests::test_tools_cache_invalidated_on_tool_list_changed— a client whose tool set grows; the cache hides the new tool until
invalidate_toolsis called, then surfaces it.Manual server-behavior check (no goose needed) —
tools/listgoes[alpha]→[alpha, beta]with anotifications/tools/list_changedbetween:Full local verification (hermit toolchain, rustc 1.96.1):
cargo check -p goose --tests— cleancargo test -p goose --lib tools_cache/... on_tool_list_changed— passcargo clippy -p goose --all-targets -- -D warnings— cleancargo fmt— no changes outside the touched filesRelated Issues
Fixes #10433
Prior art / supersedes: #8276 (auto-closed as stale, never human-reviewed)
Screenshots/Demos (for UX changes)
N/A — behavior fix, no UI change. See the before/after test output above.
Co-authored-by: makanilani 43491852+makanilani@users.noreply.github.com