Skip to content

make agent manager singleton #4880

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
yingjiehe-xyz merged 5 commits into from
Sep 29, 2025
Merged

make agent manager singleton #4880

yingjiehe-xyz merged 5 commits into from
Sep 29, 2025

Conversation

@yingjiehe-xyz
Copy link
Contributor

@yingjiehe-xyz yingjiehe-xyz commented Sep 29, 2025

Agent manager should be created every time and can be shared in different classes

DOsinga
DOsinga reviewed Sep 29, 2025
View reviewed changes
DOsinga
DOsinga approved these changes Sep 29, 2025
View reviewed changes
crates/goose/src/execution/manager.rs
impl AgentManager {
pub async fn new(max_sessions: Option<usize>) -> Result<Self> {
/// Reset the global singleton - ONLY for testing
pub fn reset_for_test() {
Copy link
Collaborator

@DOsinga DOsinga Sep 29, 2025

Choose a reason for hiding this comment

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

you do want to annotate this with test only though

Copy link
Contributor Author

@yingjiehe-xyz yingjiehe-xyz Sep 29, 2025

Choose a reason for hiding this comment

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

I don't think it work here, we can use #[cfg(test)] if the tests are agent class are in the same package, otherwise, tests cannot find the reset_for_test

see error:

no function or associated item named `reset_for_test` found for struct `AgentManager` in the current scope

crates/goose/tests/execution_tests.rs
#[serial]
async fn test_session_isolation() {
let manager = AgentManager::new(None).await.unwrap();
AgentManager::reset_for_test();
Copy link
Collaborator

@DOsinga DOsinga Sep 29, 2025

Choose a reason for hiding this comment

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

why do we need to reset these? I'd imagine you only want to reset for testing for situations where you actually want to test that a new singleton does somethign different

Copy link
Contributor Author

@yingjiehe-xyz yingjiehe-xyz Sep 29, 2025

Choose a reason for hiding this comment

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

I think it is better to reset every time to keep the tests isolated from each other

Copy link
Collaborator

@DOsinga DOsinga Sep 30, 2025

Choose a reason for hiding this comment

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

yeah, no. you should isolate these tests from the rest of the system, but if you write tests for a singleton and then reset the singleton all the time, what are you testing?

Copy link
Contributor Author

@yingjiehe-xyz yingjiehe-xyz Sep 30, 2025

Choose a reason for hiding this comment

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

Do you mean that we want to cover the singleton access crossing multiple test cases? Unit tests should only cover each simple functionalities of the methods, we have a test case to test the multiple access as the same time, and others for the main logic, to make all these tests isolated, we need such reset.

@yingjiehe-xyz yingjiehe-xyz merged commit 1c42430 into main Sep 29, 2025
11 checks passed
@yingjiehe-xyz yingjiehe-xyz deleted the yingjiehe/agent branch September 29, 2025 20:09
zanesq added a commit that referenced this pull request Sep 30, 2025
@zanesq
Merge branch 'main' of github.com:block/goose into zane/create-recipe…
20227e3 
…-unification

* 'main' of github.com:block/goose: (24 commits)
  feat(cli): add `path` & `limit` to `session list` command (#4878)
  Allow better concurrent access (#4896)
  fix: Windows prompt cursor positioning issue with ANSI escape sequences (#4464)
  Fix: LiteLLM API key field not showing in UI configuration (#4105)
  fix: path is duplicated on tool calls causing them to fail (#4658) (#4859)
  add new prompt to get all available tutorials (#4802)
  Add filtering for agentVisible: false messages on streaming providers (#4847)
  alexhancock/mcp-crate-cleanup (#4885)
  docs: rename sub-recipe to subrecipe (#4886)
  docs: new multi-model section with autopilot topic (#4864)
  make agent manager singleton (#4880)
  Cli web auth token (#4456)
  fix(token_counter): fix panic with GitHub Copilot (#4632)
  Revert "Internal MCP Crate Cleanup (#4800)" (#4883)
  remove 2 redundant comments and one that lies (#4866)
  Internal MCP Crate Cleanup (#4800)
  Fix #4612: Return non-zero exit code when CLI session ends with error (#4621)
  Dead code cleanup (#4873)
  fix: restoring test data and correcting name (#4875)
  Add .goosehints file to enforce lowercase branding in documentation (#4870)
  ...
matt-wirth added a commit to LiquidMetal-AI/goose that referenced this pull request Sep 30, 2025
@matt-wirth @angelahning
@emma-squared @katzdave @DOsinga @EbonyLouis @taniandjerry @angiejones @jamadeo @michaelneale @iandouglas @alexhancock @jsibbison-square @blackgirlbytes @jalateras @sings-to-bees-on-wednesdays @Evanfeenstra @yingjiehe-xyz @dianed-square @AdemolaAri @demetrio108 @claude @thebristolsound @exitcode0 @GuiSim
MCP workaround (#1)
e6b522f 
* remove only-pr-labels (block#4842)

Signed-off-by: Angela Ning <[email protected]>

* Docs: Add link to Plug & Play video for Reddit MCP (block#4852)

* Fix: Token count UI doesn't re-render if it's open. (block#4822)

* Update databricks flash model (block#4836)

* Session manager (block#4648)

Co-authored-by: Douwe Osinga <[email protected]>

* Add Hacktoberfest Guides (block#4830)

Co-authored-by: taniashiba <[email protected]>

* docs: goose x Hacktoberfest 2025 Blog (block#4855)

Co-authored-by: Tania Chakraborty <[email protected]>
Co-authored-by: Angie Jones <[email protected]>

* fix: delete some flaky and not-useful tests (block#4861)

* can tell the system what shell it is using (block#4807)

* new subrecipe blog post banner (block#4862)

* docs: remove recipe generator link from next to extension search (block#4858)

* lowercase g in goose (block#4832)

* doc: file parameter recipe update (block#4594)

* Fiie input recipe ref doc (block#4869)

* Add .goosehints file to enforce lowercase branding in documentation (block#4870)

Co-authored-by: Angie Jones <[email protected]>

* fix: restoring test data and correcting name (block#4875)

* Dead code cleanup (block#4873)

Co-authored-by: Douwe Osinga <[email protected]>
Co-authored-by: Michael Neale <[email protected]>

* Fix block#4612: Return non-zero exit code when CLI session ends with error (block#4621)

Signed-off-by: jalateras <[email protected]>

* Internal MCP Crate Cleanup (block#4800)

* remove 2 redundant comments and one that lies (block#4866)

Co-authored-by: Douwe Osinga <[email protected]>

* Revert "Internal MCP Crate Cleanup (block#4800)" (block#4883)

* fix(token_counter): fix panic with GitHub Copilot (block#4632)

Signed-off-by: sings-to-bees-on-wednesdays <[email protected]>

main was broken. this seems important

* Cli web auth token (block#4456)

Signed-off-by: Evanfeenstra <[email protected]>

* make agent manager singleton (block#4880)

* docs: new multi-model section with autopilot topic (block#4864)

* docs: rename sub-recipe to subrecipe (block#4886)

* alexhancock/mcp-crate-cleanup (block#4885)

* Temporary workaround for mcp server

* Add filtering for agentVisible: false messages on streaming providers (block#4847)

* add new prompt to get all available tutorials (block#4802)

Signed-off-by: AdemolaAri <[email protected]>

* Fix for auth in extension, fix for stale keychain

* fix: path is duplicated on tool calls causing them to fail (block#4658) (block#4859)

Signed-off-by: demetrio108 <[email protected]>

* Fix: LiteLLM API key field not showing in UI configuration (block#4105)

Signed-off-by: jalateras <[email protected]>
Co-authored-by: Ebony Louis <[email protected]>
Co-authored-by: Claude <[email protected]>
Co-authored-by: Jack Amadeo <[email protected]>

* fix: Windows prompt cursor positioning issue with ANSI escape sequences (block#4464)

Signed-off-by: Matt Donovan <[email protected]>
Co-authored-by: Matt Donovan <[email protected]>

* Allow better concurrent access (block#4896)

Co-authored-by: Douwe Osinga <[email protected]>

* feat(cli): add `path` & `limit` to `session list` command (block#4878)

* Added CMD+T keyboard shortcut that takes you to the Home tab (block#4541)

Signed-off-by: Guillaume Simard <[email protected]>

---------

Signed-off-by: Angela Ning <[email protected]>
Signed-off-by: jalateras <[email protected]>
Signed-off-by: Evanfeenstra <[email protected]>
Signed-off-by: AdemolaAri <[email protected]>
Signed-off-by: demetrio108 <[email protected]>
Signed-off-by: Matt Donovan <[email protected]>
Signed-off-by: Guillaume Simard <[email protected]>
Co-authored-by: Angela Ning <[email protected]>
Co-authored-by: Emma Youndtsmith <[email protected]>
Co-authored-by: David Katz <[email protected]>
Co-authored-by: Douwe Osinga <[email protected]>
Co-authored-by: Douwe Osinga <[email protected]>
Co-authored-by: Ebony Louis <[email protected]>
Co-authored-by: taniashiba <[email protected]>
Co-authored-by: taniandjerry <[email protected]>
Co-authored-by: Tania Chakraborty <[email protected]>
Co-authored-by: Angie Jones <[email protected]>
Co-authored-by: Jack Amadeo <[email protected]>
Co-authored-by: Michael Neale <[email protected]>
Co-authored-by: w. ian douglas <[email protected]>
Co-authored-by: Alex Hancock <[email protected]>
Co-authored-by: Jarrod Sibbison <[email protected]>
Co-authored-by: Rizel Scarlett <[email protected]>
Co-authored-by: Jim Alateras <[email protected]>
Co-authored-by: sings-to-bees-on-wednesdays <[email protected]>
Co-authored-by: Evan Feenstra <[email protected]>
Co-authored-by: Yingjie He <[email protected]>
Co-authored-by: dianed-square <[email protected]>
Co-authored-by: Ademola Arigbabuwo <[email protected]>
Co-authored-by: Demetrio ⚡️ <[email protected]>
Co-authored-by: Claude <[email protected]>
Co-authored-by: Jack Amadeo <[email protected]>
Co-authored-by: Matt Donovan <[email protected]>
Co-authored-by: Matt Donovan <[email protected]>
Co-authored-by: Robert Mcgregor <[email protected]>
Co-authored-by: Guillaume Simard <[email protected]>
wpfleger96 added a commit to wpfleger96/goose that referenced this pull request Oct 1, 2025
@wpfleger96
Merge branch 'main' into wpfleger/restore-extensions-on-resume
9bacd60 
* main: (206 commits)
  Tiny: fix github casing  (block#4903)
  remove anyOf from create_task tool (block#4897)
  chore(deps): bump tracing-subscriber from 0.3.19 to 0.3.20 (block#4442)
  fix optional recipe schema zod validation (block#4900)
  Added CMD+T keyboard shortcut that takes you to the Home tab (block#4541)
  feat(cli): add `path` & `limit` to `session list` command (block#4878)
  Allow better concurrent access (block#4896)
  fix: Windows prompt cursor positioning issue with ANSI escape sequences (block#4464)
  Fix: LiteLLM API key field not showing in UI configuration (block#4105)
  fix: path is duplicated on tool calls causing them to fail (block#4658) (block#4859)
  add new prompt to get all available tutorials (block#4802)
  Add filtering for agentVisible: false messages on streaming providers (block#4847)
  alexhancock/mcp-crate-cleanup (block#4885)
  docs: rename sub-recipe to subrecipe (block#4886)
  docs: new multi-model section with autopilot topic (block#4864)
  make agent manager singleton (block#4880)
  Cli web auth token (block#4456)
  fix(token_counter): fix panic with GitHub Copilot (block#4632)
  Revert "Internal MCP Crate Cleanup (block#4800)" (block#4883)
  remove 2 redundant comments and one that lies (block#4866)
  ...
HikaruEgashira pushed a commit to HikaruEgashira/goose that referenced this pull request Oct 3, 2025
@yingjiehe-xyz @HikaruEgashira
make agent manager singleton (block#4880)
2cd2193 
Signed-off-by: HikaruEgashira <[email protected]>
This was referenced Oct 8, 2025
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@DOsinga DOsinga DOsinga approved these changes

@tlongwell-block tlongwell-block tlongwell-block 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.

4 participants

@yingjiehe-xyz @DOsinga @tlongwell-block