refactor(agent): share model endpoint resolution - #7942
shellz-n-stuff wants to merge 1 commit into
Conversation
🔐 Codex Security Review
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1bb547c2d7
ℹ️ About Codex in GitHub
Your team has set up Codex to 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 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| //! Endpoint resolution shared by agent startup and desktop security suggestions. | ||
| use super::Provider; | ||
|
|
||
| pub fn provider_base_url( |
There was a problem hiding this comment.
provider_base_url is newly exposed through the public config::endpoint module but has no Rustdoc describing its lookup contract, provider defaults, or the required Databricks host. Add a doc comment, or restrict its visibility if it is not intended for external launchers, to satisfy the repository requirement that every new public API be documented.
AGENTS.md reference: AGENTS.md:L193-L196
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This seems overkill for a lookup?
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
1bb547c to
68c0058
Compare
wpfleger96
left a comment
There was a problem hiding this comment.
🤖 hey @shellz-n-stuff, thanks for splitting the stack up like this, it made it easy to review.
The from_env refactor looks right to me. As far as I can tell the defaults, the DATABRICKS_HOST required error, and the order errors come out in are all unchanged, and the existing integration tests already push every base-URL env var through from_env. So I don't have concerns about how the agent behaves at startup. My comments are all about configured_endpoint, what it promises, and its test.
One small thing on the description: I'd scope "ensures you can't accidentally prevent your agent from talking to a required provider endpoint" down to buzz-agent model traffic. goose, Claude Code, and Codex each work out their own endpoints, and MCP servers, the relay, and the databricks CLI auth path aren't covered either. So a launcher that only allows this URL would still block things the agent needs.
The red Desktop Smoke E2E (4) shard looks unrelated. This PR only touches crates/buzz-agent, and shard 4 has also been red on main today on other specs (video-attachment, workflows), so I think a rerun should clear it.
| /// Resolve without fetching credentials, signing in, or making network requests. | ||
| pub fn configured_endpoint(lookup: impl Fn(&str) -> Option<String>) -> Option<String> { |
There was a problem hiding this comment.
🤖 I think this one needs a sentence or two of contract. My guess is that's why the bot keeps flagging it, though I agree full Rustdoc on provider_base_url would be overkill. Nothing in the repo calls configured_endpoint, and nothing in #7943 or #7944 does either. The module comment says desktop uses it, but I couldn't find a caller there. If the plan is for Sandpit to depend on buzz-agent as a crate, I'd say that here: it's for launchers, it returns the configured base URL (not a validated host or a full allowlist), overrides are returned as-is, and None means the provider is missing or unsupported, or it's Databricks with no host. The caller also has to pass the child's actual effective env.
Related: for an allowlist use case, OPENAI_COMPAT_BASE_URL="" coming back as Some("") is probably not what a launcher wants. I'd treat empty or whitespace-only values as unset here and leave from_env alone, since it already behaves that way on main.
| let provider = super::resolve_provider( | ||
| lookup("BUZZ_AGENT_PROVIDER").as_deref(), | ||
| Some("configured"), | ||
| Some("configured"), | ||
| Some("configured"), | ||
| ) |
There was a problem hiding this comment.
🤖 The "configured" placeholders work because they happen to satisfy the three key-presence checks in resolve_provider, but that ties this function to resolve_provider's current parameters. If a provider later requires a different key, this would silently lie about it. I think it'd be cleaner to pull the name → Provider match out of resolve_provider into a small pure fn and have both call it.
| assert_eq!( | ||
| provider_base_url(&Provider::OpenAi, |_| Some( | ||
| "http://localhost:1234/v1".into() | ||
| )) | ||
| .unwrap(), | ||
| "http://localhost:1234/v1" | ||
| ); |
There was a problem hiding this comment.
🤖 This lookup returns the custom URL for any key, so if OpenAI's override key accidentally became ANTHROPIC_BASE_URL this would still pass. Could this use a key-sensitive lookup, ideally with a different override set for each provider at once, and assert that only the selected provider's value wins? A table-driven test would also make it easy to add a couple of cases that aren't checked today: Databricks with no DATABRICKS_HOST (should be None), and a blank or unsupported BUZZ_AGENT_PROVIDER.
## Summary Replacing `BUZZ_ACP_AGENT_COMMAND` with a sandbox launcher hides the real adapter from Buzz: for example, Goose loses its default `acp` argument. Add optional `BUZZ_ACP_LAUNCH_PREFIX`, a JSON argument array applied at the shared subprocess boundary, after normal worker configuration. On Unix, every spawn runs `prefix... worker args...` without shell interpolation. Worker identity, environment setup, stdio and existing process cleanup are preserved. Invalid or unavailable prefixes fail launch without falling back to the worker; unset keeps direct launch on every platform. Configured prefixes fail closed on non-Unix because per-worker process-tree cleanup is unavailable. Desktop reserves the key against saved user-environment overrides, and spawn failures name the executable. ### Related issue Prerequisite for [Buzz-App #415](block/buzz-app#415). One commit directly on `main`, independent of #7942–#7944. No duplicate launch-prefix PR found. Policy enforcement, verified launcher staging, protected paths and the supporting runtime pin remain Buzz-App/plugin work. ### Testing - Original head `9ae9d82f8`: complete ACP suite (990 passed, three existing ignored), shipped local-task test, real-Goose probe, and full repository-wide `just ci` passed. - Review fixes: five launch tests and 44 Desktop environment-filter tests pass locally, covering the real production platform gate, save rejection, both merge layers, case variants, and executable error context. - Subprocess tests cover direct/wrapped argument defaults, Pi skills, Hermes/Codex environment, adapter identity, repeated spawns and invalid/missing prefix refusal. - Shipped `buzz-acp run` entrypoint completes a task through the wrapper and deterministic ACP peer. - Real Goose 1.52.0: direct and wrapped `auth-methods` return identical results; the wrapper receives `goose acp` with initially empty worker args. - Independent agent review: no blocking findings. - Required push checks passed on the updated head. The existing native Windows CI job now explicitly runs the platform-contract tests. Its result is pending; the non-Unix refusal branch was not exercised on this macOS host. Repeated-spawn coverage is not a full lazy-pool wake/crash test. Protected Goose in the Buzz-App UI and human acceptance remain pending. To try the hook after building `buzz-acp`, use an installed Goose path: ```sh BUZZ_ACP_LAUNCH_PREFIX='["/usr/bin/env"]' target/debug/buzz-acp auth-methods \ --agent-command /absolute/path/to/goose --agent-args '' --json ``` Expect the same authentication methods as without the prefix. Changing the prefix to `["/missing-launcher"]` must fail. Draft pending human testing. Signed-off-by: Alex Rosenzweig <arosenzweig@squareup.com>
Description
Centralize model endpoint selection so external launchers can resolve the same destination as the agent without reading credentials or making network requests. Existing provider defaults and overrides are unchanged.
This has an explicit use-case for network sandboxing as it ensures you can't accidentally prevent your agent from talking to a required provider endpoint.
Stack 1/3: base
main; next: TLS trust snapshots.Validation: endpoint regression test passes; commit hooks pass. No plugin dependency.
Review / merge order: #7942 → #7943 → #7944. Related: #5286. Replaces #7940.
Required push checks passed for this branch. Hosted CI is tracked on the PR.