Skip to content

fix: exit the goose and show the error message when provider environment variable is not set#103

Merged
lifeizhou-ap merged 13 commits intomainfrom
lifei/show-readable-error-message-when-api-key-not-specified
Oct 4, 2024
Merged

fix: exit the goose and show the error message when provider environment variable is not set#103
lifeizhou-ap merged 13 commits intomainfrom
lifei/show-readable-error-message-when-api-key-not-specified

Conversation

@lifeizhou-ap
Copy link
Collaborator

@lifeizhou-ap lifeizhou-ap commented Oct 1, 2024

Why

Currently it throws a runtime error and print the stack trace when

  • when the environment variable is not set for LLM apis,
  • when unknown provider or moderator is trying to be loaded

This make it hard for users to understand what is going.

What

Exchange

  • Raised a MissingProviderEnvVariableError with attributes of provider, env_variable. When providers fetch the environment variable for api key, it will raise this error if the environment variable is not available
  • Raised a LoadExchangeAttributeError for unknown provider or moderator

With the above custom erross we can construct user friendly messages in goose

goose

  • Raised a LoadExchangeAttributeError for unknown provider or toolkit
  • Catch the custom errors Show the error message without stacktrace and exit the program.

screenshots
Screenshot 2024-10-03 at 11 35 56 AM

Screenshot 2024-10-02 at 11 49 46 AM

Please give feedback including the content and presentation of these error message. Thank you!

break
except Exception:
pass
else:
Copy link
Collaborator Author

@lifeizhou-ap lifeizhou-ap Oct 1, 2024

Choose a reason for hiding this comment

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

we don't need this message. The error message (env var is not set) will be thrown when the provider is loaded while creating the session.

Copy link
Collaborator

Choose a reason for hiding this comment

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

so in this case, the provider value that is returned below will be whatever is last in the list of providers, and then be used to set the default profile. It will later error like you say.

Should we do

else:
    provider = RECOMMENDED_DEFAULT

so that we don't set it to something niche?

@lifeizhou-ap lifeizhou-ap changed the title feat: exit the goose and show the error message when provider environment is not set fix: exit the goose and show the error message when provider environment is not set Oct 1, 2024
@lifeizhou-ap lifeizhou-ap changed the title fix: exit the goose and show the error message when provider environment is not set fix: exit the goose and show the error message when provider environment variable is not set Oct 1, 2024
def test_create_exchange_exit_when_env_var_does_not_exist(create_session_with_mock_configs, mock_sessions_path):
session = create_session_with_mock_configs()
expected_error = MissingProviderEnvVariableError(env_variable="OPENAI_API_KEY", provider="openai")
with patch("goose.cli.session.build_exchange", side_effect=expected_error), patch(
Copy link
Contributor

Choose a reason for hiding this comment

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

:nit: we can make this easier to read by forcing ruff format to newline with trailing command and parenthesis

with (
  patch("goose.cli.session.build_exchange", side_effect=expected_error), 
  patch("goose.cli.session.print") as mock_print, 
  patch("sys.exit") as mock_exit,
):

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, I prefer the separated lines too. However, I had a look, it seems that ruff format is unable to do it as it only supports some light formatting rules.

Copy link
Contributor

@lamchau lamchau Oct 3, 2024

Choose a reason for hiding this comment

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

what do you mean? i refactored one of our tests and it worked for me - how did ruff format not work/fail?

image

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh, sorry I missed the extra brackets. I've updated and it works now!

@lifeizhou-ap lifeizhou-ap marked this pull request as ready for review October 3, 2024 01:42
Copy link
Collaborator

@baxen baxen left a comment

Choose a reason for hiding this comment

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

This is great! Much clearer

from typing import List


class LoadExchangeAttributeError(Exception):
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe InvalidChoice

return cls(client)

@classmethod
def _get_env_variable(cls: Type["AzureProvider"], key: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: i'd just call the helper directly instead of adding the new method?

return tool_config

@classmethod
def _get_env_variable(cls: Type["BedrockProvider"], key: str) -> str:
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: same comment as above

break
except Exception:
pass
else:
Copy link
Collaborator

Choose a reason for hiding this comment

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

so in this case, the provider value that is returned below will be whatever is last in the list of providers, and then be used to set the default profile. It will later error like you say.

Should we do

else:
    provider = RECOMMENDED_DEFAULT

so that we don't set it to something niche?

@lifeizhou-ap lifeizhou-ap merged commit 908af7f into main Oct 4, 2024
@lifeizhou-ap lifeizhou-ap deleted the lifei/show-readable-error-message-when-api-key-not-specified branch October 4, 2024 01:18
lifeizhou-ap added a commit that referenced this pull request Oct 4, 2024
* main:
  fix: exit the goose and show the error message when provider environment variable is not set (#103)
  fix: Update OpenAI pricing per https://openai.com/api/pricing/ (#110)
  fix: update developer tool prompts to use plan task status to match allowable statuses update_plan tool call (#107)
  fix: removed the panel in the output so that the user won't have unnecessary pane borders in the copied content (#109)
  docs: update links to exchange to the new location (#108)
  chore: setup workspace for exchange (#105)
lily-de pushed a commit that referenced this pull request Oct 7, 2024
lukealvoeiro added a commit that referenced this pull request Oct 9, 2024
* main: (41 commits)
  chore: Add goose providers list command (#116)
  docs: working ollama for desktop (#125)
  docs: format and clean up warnings/errors (#120)
  docs: update deploy workflow (#124)
  feat: Implement a goose run command (#121)
  feat: saved api_key to keychain for user (#104)
  docs: add callout plugin (#119)
  chore: add a page to docs for Goose application examples (#117)
  fix: exit the goose and show the error message when provider environment variable is not set (#103)
  fix: Update OpenAI pricing per https://openai.com/api/pricing/ (#110)
  fix: update developer tool prompts to use plan task status to match allowable statuses update_plan tool call (#107)
  fix: removed the panel in the output so that the user won't have unnecessary pane borders in the copied content (#109)
  docs: update links to exchange to the new location (#108)
  chore: setup workspace for exchange (#105)
  fix: resolve uvx when using a git client or IDE (#98)
  ci: add include-markdown for mkdocs (#100)
  chore: fix broken badge on readme (#102)
  feat: add global optional user goosehints file (#73)
  docs: update docs (#99)
  chore(release): release 0.9.3 (#97)
  ...
ahau-square pushed a commit that referenced this pull request May 2, 2025
cbruyndoncx pushed a commit to cbruyndoncx/goose that referenced this pull request Jul 20, 2025
dianed-square added a commit that referenced this pull request Dec 4, 2025
- Use environment variables instead of direct GitHub context interpolation
  in shell scripts to prevent potential shell injection attacks
- Pin peter-evans/create-pull-request to commit SHA (v6)

Resolves code scanning alerts #103, #104, #105
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.

4 participants