fix: validate provider urls before use by codefromthecrypt · Pull Request #147 · aaif-goose/goose

codefromthecrypt · 2024-10-13T10:36:15Z

This fixes validation, like #133, except for all providers that accept a hostname/base URL variable.

codefromthecrypt · 2024-10-13T20:58:08Z


        client = httpx.Client(
-            base_url=url + "v1/",
+            base_url=url.join("v1/"),


fyi using httpx.URL as it isn't so sensitive about trailing slash etc when joining

interesting, i always remembered // would resolve? just curious what bug you can into since i haven't seen this before!

I think it was more about not having a slash at all. e.g if you set a base URL ending in the port

michaelneale · 2024-10-13T22:48:27Z

@lamchau can you cast eagle eyes over this?

lamchau · 2024-10-15T05:35:26Z

+
+    val = os.environ.get(key, default)
+    if val == "":
+        raise ValueError(f"{key} was empty")


:nit: should use KeyError which is consistent with the non-default value getter os.environ[key].

[nav] In [1]: import os ...: ...: os.environ["SHELL"] ...: os.environ["SHELL_MISSING"] ...: --------------------------------------------------------------------------- KeyError Traceback (most recent call last) Cell In[1], line 4 1 import os 3 os.environ["SHELL"] ----> 4 os.environ["SHELL_MISSING"] File <frozen os>:714, in __getitem__(self, key) KeyError: 'SHELL_MISSING'

forgot what package i was in – it just occurred to me that we recently wrote up a way to do this on the base provider. could we add the check in there?

https://github.com/block-open-source/goose/blob/06c12b627c4b2da94c05b021a6b2e31539557798/packages/exchange/src/exchange/providers/base.py#L26-L29

so you mean add a parameter to check_env_vars() like base_url_key?

exactly! imo we might want to tweak it slightly so it'll dump all the env vars in a single pass rather than one at a time (especially with how much setup is needed for AZURE_*).

not sure if @elenazherdeva is working on that atm, but it was from this #116 (comment)

@lamchau thank you for pinging! I’m not working on it at the moment, but I should fix it. Let me create a follow-up PR based on your comments!

codefromthecrypt · 2024-10-15T22:49:45Z

thanks for the advice @lamchau will revise impl after clarification on #147 (comment)

codefromthecrypt · 2024-10-28T03:33:28Z

sorry was awol on medical leave. sporadic this week, but will adjust this and others soon

lamchau · 2024-10-28T22:06:37Z

@codefromthecrypt no worries! hopefully you're doing well/on the mend!

Signed-off-by: Adrian Cole <adrian.cole@elastic.co>

codefromthecrypt · 2024-11-04T07:29:44Z

@lamchau PTAL I have consolidated what I could into the same check, and refactored things around it.

codefromthecrypt · 2024-11-04T07:30:46Z

    @retry_procedure
    def _post(self, payload: dict) -> httpx.Response:
-        response = self.client.post(ANTHROPIC_HOST, json=payload)
+        response = self.client.post(self.BASE_URL_DEFAULT, json=payload)


I think this is incorrect as it defers to the default value without considering an override. OTOH, if I make this empty, it fails tests...

codefromthecrypt · 2024-11-04T07:31:20Z

    PROVIDER_NAME = "azure"
+    BASE_URL_ENV_VAR = "AZURE_CHAT_COMPLETIONS_HOST_NAME"
    REQUIRED_ENV_VARS = [
-        "AZURE_CHAT_COMPLETIONS_HOST_NAME",


I separated out the base URL enforcement from the other ENV vars as there is special handling

michaelneale · 2024-11-04T08:07:21Z


    PROVIDER_NAME = "google"
+    BASE_URL_ENV_VAR = "GOOGLE_HOST"
+    BASE_URL_DEFAULT = "https://generativelanguage.googleapis.com/v1beta"


does it make sense to have the constant at the top of the file just as people may want to quickly scan/change?

So, these constants are accessed as class variables, so if we move to the top they'd no longer be that. I'm not sure a hack to work around this..

baxen · 2025-02-17T20:33:56Z

Closing up PRs that point to pre-v1.0 but this is still a good idea, happy to open this up again against v1.0

This removes the goose/temp/ directory which contained old test projects with vulnerable dependencies that were incorrectly tracked in git. Fixes Dependabot alerts: - #147-162: Python dependencies in watchflow-main/uv.lock (langchain, aiohttp, urllib3, etc.) - #138-139: esbuild vulnerabilities in vibes-cli temp files The @modelcontextprotocol/sdk in ui/desktop is already updated to 1.26.0 which fixes alert #167. Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>

* feat: file @-mention with fuzzy search in chat input Extend the mention system to show session files alongside personas: - MentionAutocomplete now renders two sections: Personas and Files - FileMentionItem type for file suggestions (resolvedPath, displayPath, filename, kind) - MentionItem union type (persona | file) for unified selection handling - useMentionDetection accepts files param, filters both lists on query - confirmMention returns MentionItem instead of Persona - Extract mention handlers into useMentionHandlers hook (keeps ChatInput under 500 lines) - File data sourced from ArtifactPolicyContext.getAllSessionArtifacts() - Selecting a file inserts its resolved path into the message text - Selecting a persona still switches the active persona (unchanged behavior) - Add i18n key mention.filesTitle for the files section header * fix: use Radix Popover for @-mention autocomplete positioning Replace manual absolute positioning with shared/ui Popover primitives so the dropdown gets viewport collision detection and portal rendering. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat(system): add file mention scan command Add a Tauri command that scans project roots for files usable in @ mention completion.\n\nThe scan deduplicates roots and file paths, skips common generated directories,\nlimits traversal depth and result count, and runs in a blocking task to avoid\nstalling the async runtime.\n\nExpose the command through the frontend system API so chat mention handlers can\nrequest project files. * feat(chat): include project files in @mentions Load project file paths from configured working directories and merge them\nwith session artifacts in mention autocomplete results.\n\nWhen selecting a project, mention suggestions now include matching files even\nbefore they have been attached in the current session, and retain deduped\nabsolute paths for insertion.\n\nAdd a chat input test for file mention selection and include the Spanish\ntranslation for the files section heading. * fix: scroll selected mention item into view on arrow navigation Add scrollIntoView({ block: "nearest" }) via ref map so the active item stays visible when arrowing through the popover list. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * feat: use fuzzy subsequence matching for @-mention filtering Replace includes() with an fzf-style subsequence matcher so queries like "crates/sprout-acp/.rs" match files whose path contains those characters in order (e.g. acp.rs, config.rs in that directory). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: project files not appearing in @-mention popover The loadedRootsKeyRef dedup guard prevented the second mount in React StrictMode from fetching files, while the first mount's response was cancelled by cleanup. Remove the ref guard — the sameStringArray check in setProjectFilePaths already prevents unnecessary state updates. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: fix biome formatting in ChatInput Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: replace requestAnimationFrame with useEffect for cursor placement Use a pendingCursorRef + useEffect triggered by text changes instead of rAF to set focus and cursor position after mention selection. This is React-idiomatic — the effect runs after React flushes the new text into the DOM, avoiding timing hacks. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * chore: remove stale comment in MentionAutocomplete Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: biome format and lint issues - Expand biome.json includes array to multi-line format - Exclude .worktrees and .claude/worktrees from biome checks - Add biome-ignore for intentional text dependency in cursor effect Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: resolve TypeScript error in ChatInput test mock Type the listFilesForMentions mock with explicit parameter types instead of rest spread to satisfy tsc --noEmit. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: fix biome formatting in ChatInput test Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: add list_files_for_mentions to E2E Tauri mock The mock IPC fallback returns null for unknown commands, which crashes the mention handler expecting a string array. Add the mock so E2E draft tests can render the chat view. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * refactor: use ignore crate for gitignore-aware file scanning Replace the hardcoded should_skip_dir list with the `ignore` crate (from ripgrep) which respects .gitignore, .git/info/exclude, and global gitignore. This automatically handles project-specific ignores instead of maintaining a static list of directory names. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * style: cargo fmt Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: harden file scanner and deduplicate mention filtering Two fixes: 1. File scanner: add follow_links(false) explicitly and canonicalize all paths to reject any that escape the project roots. Prevents symlink loops and path leakage. 2. Mention filtering: useMentionDetection hook now owns all filtering and exposes filteredPersonas/filteredFiles. MentionAutocomplete renders pre-filtered results instead of re-running the same fuzzy match on every keystroke. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: remove unused mentionQuery destructure Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: simplify persona header logic and stop collapsing user spaces - Collapse redundant persona header branches into a single condition - Remove replace(/\s{2,}/g, " ") from mention insertion which silently ate intentional double spaces (markdown, code blocks) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> * fix: clear file suggestions immediately when project roots change Prevents stale files from a previous project appearing in mention results while the new scan is in flight. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

codefromthecrypt requested a review from michaelneale October 13, 2024 10:36

codefromthecrypt commented Oct 13, 2024

View reviewed changes

codefromthecrypt mentioned this pull request Oct 13, 2024

OLLAMA_HOST fails silently if missing https #133

Closed

michaelneale requested review from baxen and lamchau October 13, 2024 22:42

lamchau reviewed Oct 15, 2024

View reviewed changes

Comment thread packages/exchange/src/exchange/providers/utils.py Outdated

lamchau reviewed Oct 15, 2024

View reviewed changes

fix: validate provider urls before use

f53c503

Signed-off-by: Adrian Cole <adrian.cole@elastic.co>

codefromthecrypt force-pushed the validate_env branch from 4dc27d9 to 224d729 Compare November 4, 2024 07:11

feedback

e8049a4

Signed-off-by: Adrian Cole <adrian.cole@elastic.co>

codefromthecrypt force-pushed the validate_env branch from 224d729 to e8049a4 Compare November 4, 2024 07:29

codefromthecrypt commented Nov 4, 2024

View reviewed changes

michaelneale reviewed Nov 4, 2024

View reviewed changes

michaelneale approved these changes Nov 4, 2024

View reviewed changes

baxen closed this Feb 17, 2025

Conversation

codefromthecrypt commented Oct 13, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

michaelneale commented Oct 13, 2024

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codefromthecrypt commented Oct 15, 2024

Uh oh!

codefromthecrypt commented Oct 28, 2024

Uh oh!

lamchau commented Oct 28, 2024

Uh oh!

codefromthecrypt commented Nov 4, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

baxen commented Feb 17, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants