fix: replace Provider HTTP module with beam_agent session-based model listing#35
Open
beardedeagle wants to merge 11 commits intomainfrom
Open
fix: replace Provider HTTP module with beam_agent session-based model listing#35beardedeagle wants to merge 11 commits intomainfrom
beardedeagle wants to merge 11 commits intomainfrom
Conversation
The VaultLive "Refresh Models" button was a no-op — it called
refresh_all() which iterates state.backends (empty by default),
and probes had no workspace_id or secret_name to resolve API keys.
Add ModelRegistry.refresh_for_workspace/1 which discovers vault
secrets for a workspace, maps providers to backends via a static
reverse mapping, probes each backend with the correct secret_name,
and auto-configures state.backends/backend_configs/workspace_id so
future tick probes work without manual reconfiguration.
- Refactor do_synchronous_probe to accept explicit {adapter, opts}
- Add discover_workspace_backends/1 (vault query + provider→backend)
- Add bootstrap_workspace_config/3 (auto-configure for tick probes)
- VaultLive passes workspace_id through the refresh chain
- 6 new integration tests covering all discovery paths
There was a problem hiding this comment.
Pull request overview
This PR fixes VaultLive’s “Refresh models” flow by making it workspace-aware: it discovers vault secrets for the active workspace, maps them to ModelRegistry backends, probes upstream providers using the correct {workspace_id, secret_name}, and bootstraps the registry state so future periodic ticks probe without manual configuration.
Changes:
- Add
ModelRegistry.refresh_for_workspace/1plus internal discovery/bootstrap helpers to probe using workspace vault secrets. - Refactor synchronous probing to accept an explicit
{adapter, opts}tuple to support workspace-derived probes. - Wire VaultLive’s refresh button to pass
workspace_idthrough to the new refresh path, and add workspace refresh integration tests.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
lib/monkey_claw/model_registry.ex |
Introduces refresh_for_workspace/1, backend discovery/bootstrap logic, and updates synchronous probe plumbing. |
lib/monkey_claw_web/live/vault_live.ex |
Updates model refresh event handling to call the workspace-aware registry refresh. |
test/monkey_claw/model_registry/workspace_refresh_test.exs |
Adds integration tests covering workspace backend discovery, error cases, and deduplication. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The tick handler was a no-op because state.backends defaulted to [] and nothing ever populated it. Now on each tick where backends is empty, maybe_auto_discover/1 queries the first workspace's vault secrets, maps providers to backends, and bootstraps the registry state — so the very next probe dispatch has targets to hit. Combined with the refresh_for_workspace/1 fix, the registry now has two paths to discover backends: automatic (first tick, 5s after boot) and manual (VaultLive refresh button).
…solution
fire_model_hook passed session_opts to backend.list_models(), but
Scope.session_opts/1 strips :workspace_id and :secret_name (they aren't
persona config keys). This caused Provider.resolve_via_vault to return
{:error, :missing_workspace_id} on every session start — silently
swallowed by the catch-all fallback, so PATH 3 (session hook) never
populated the ModelRegistry from real API calls.
Fix: enrich_model_hook_opts/2 injects :workspace_id from the session's
own id (which IS the workspace id) and discovers :secret_name by querying
the vault for secrets matching the backend's upstream provider.
beardedeagle
changed the title
…ge, test docs - Add github_copilot → copilot to @provider_to_backend (model_registry) and copilot → github_copilot to @backend_to_provider (session) to match BeamAgent.backend_to_provider/1 coverage - Change bootstrap_workspace_config from Map.put_new to Map.update so re-discovered secret_name always overwrites stale config entries - Update workspace_refresh_test moduledoc to accurately describe that tests hit real APIs with fake keys (fast 401s), not unreachable hosts
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
… listing Replace MonkeyClaw's direct HTTP Provider.fetch_models with beam_agent session-based model listing. BeamAgent.Auth.status/1 handles auth detection, and BeamAgent.Catalog.supported_models/1 queries model catalogs through temporary sessions. - Rewrite Backend.BeamAgent.list_models/1 to use auth check + temp session pattern (start → query catalog → stop) with try/after cleanup - Add auth-based auto-discovery in ModelRegistry replacing vault-based as primary path; vault remains secondary via refresh_for_workspace/1 - Remove Provider module (277 lines of direct HTTP client code) - Remove credential enrichment from Session (enrich_model_hook_opts, discover_secret_for_backend) — backend adapter owns auth now - Clean fire_model_hook to 4-param signature (dead workspace_id removed) - Add function_exported?/3 guards for unreleased BeamAgent.Auth API - Relocate sanitize_for_log/1 to ModelRegistry as private function - Rewrite tests as AuthDiscoveryTest using Backend.Test adapter - Update README and index.html for documentation parity
beardedeagle
changed the title
BeamAgent.Auth and BeamAgent.Catalog are compiled beam_agent_ex modules present on the code path. function_exported?/3 only checks already-loaded modules without triggering autoload, so the guards always returned false — silently disabling auth-based discovery and session-based model listing at runtime. Remove the function_exported? guards and the dead :not_supported branch. The modules are a hard compile-time dependency, not optional. Retain @dialyzer annotations for PLT compatibility.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
When vault returns no credentials for a workspace, ModelRegistry now falls back to beam_agent auth status detection to discover authenticated backends. This prevents the no_backends_discovered error on UI refresh when vault is empty but CLI tools are logged in. Also adds copilot and opencode to baseline config in runtime.exs — previously only claude, codex, and gemini were configured, so 2 of 5 backends never appeared in the UI.
…daction test, vault_live doc - beam_agent.ex: document that vault-derived opts (workspace_id, secret_name) are intentionally unused — BeamAgent authenticates via CLI auth status - model_registry.ex: validate_option(:backend_configs) now enforces string keys to match the rest of the module's backend identifier convention - model_registry.ex: add sanitized logging to synchronous probe crash path (was only present in the async handle_info path) - auth_discovery_test.exs: add redaction test that verifies sanitize_for_log redacts secret-shaped values in ModelRegistry crash logs - vault_live.ex: update moduledoc to reflect composite discovery (vault secrets or CLI auth status), not vault-only probing
start_session/1 returns {ok, pid} before the CLI init handshake
completes — supported_models/1 reads from init_response which is
only populated once the session state machine reaches :ready.
Add await_session_ready/2 that polls BeamAgent.health/1 (100ms
intervals, 15s timeout) to gate the catalog query. Handles three
outcomes: :ready proceeds, :connecting/:initializing retries,
terminal states (:error/:unknown) fail fast.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
beam_agent.ex: add explicit normalize_backend(nil) clause returning
{:error, :backend_required} instead of the opaque {:unknown_backend, nil}.
Makes the contract clear: callers must pass :backend in opts.
auth_discovery_test.exs: remove ignored [table_name: ...] opts from
EtsHeir start_supervised! — EtsHeir.start_link/1 hardcodes the table
name and ignores any opts passed to it.
The CachedModel @model_field_pattern regex was too restrictive for real-world model names from upstream providers. Gemini models include parentheses (e.g., "Flash (Thinking)"), brackets, plus signs, and other characters that were rejected by the old allowlist regex. Changed from explicit allowlist to rejecting only control characters and null bytes — all printable UTF-8 is valid for model display text. Also fixed three related issues: 1. validate_writes now surfaces embedded changeset errors via collect_all_errors/1 instead of logging opaque [] for top-level errors when the failure is in a nested model changeset. 2. list_models_via_session handles the case where supported_models returns a map with "availableModels" sub-key (Gemini init_response format) instead of a flat list, preventing CaseClauseError crashes. 3. Moved rescue from discover_authenticated_backends to the per-backend backend_authenticated?/1 so a crash in one backend's auth check doesn't discard discoveries from the other four backends.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+54
to
+56
| # brackets, plus signs, commas, at-signs, etc. Reject only control | ||
| # characters and null bytes — everything else is valid UTF-8 display text. | ||
| @model_field_pattern ~r/\A[^\x00-\x1f\x7f]+\z/u |
There was a problem hiding this comment.
@model_field_pattern claims to reject “control characters”, but the current regex only excludes C0 controls (U+0000–U+001F) and DEL (U+007F). It still allows C1 control characters (U+0080–U+009F) and other Unicode control/format characters, so the validation does not match the stated invariant. Consider switching to a Unicode category-based exclusion (e.g. \p{Cc} / \p{C}) or explicitly adding the C1 range if the intent is to block all control characters.
Suggested change
| # brackets, plus signs, commas, at-signs, etc. Reject only control | |
| # characters and null bytes — everything else is valid UTF-8 display text. | |
| @model_field_pattern ~r/\A[^\x00-\x1f\x7f]+\z/u | |
| # brackets, plus signs, commas, at-signs, etc. Reject only Unicode | |
| # control characters (including null bytes) — everything else is valid | |
| # UTF-8 display text. | |
| @model_field_pattern ~r/\A[^\p{Cc}]+\z/u |
Comment on lines
411
to
416
| def handle_info(:tick, %State{} = state) do | ||
| state = maybe_retry_sqlite_load(state) | ||
| state = maybe_auto_discover(state) | ||
| state = Enum.reduce(state.backends, state, &maybe_dispatch_probe/2) | ||
| state = schedule_next_tick(state) | ||
| {:noreply, state} |
There was a problem hiding this comment.
maybe_auto_discover/1 only runs on :tick, but when state.backends is empty, schedule_next_tick/1 computes the next tick delay as state.default_interval (24h by default). This means if no CLI auth is present at startup, newly authenticated backends won’t be auto-discovered until the next daily tick unless an explicit refresh is triggered. Consider scheduling a much shorter retry cadence while backends == [] (or a separate discovery timer) so “auto-discovery” reacts promptly to users logging in after boot.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
BeamAgent.Auth.status/1(auth detection) andBeamAgent.Catalog.supported_models/1(model discovery through temporary sessions)refresh_for_workspace/1enrich_model_hook_opts,discover_secret_for_backend, dead_workspace_idparameter; backend adapter now owns authsanitize_for_log/1to ModelRegistry as private functionAuthDiscoveryTestusingBackend.Testadapter — no mocks, deterministicNet: -105 lines (483 added, 588 removed). Single source of truth for model data is now beam_agent.
Test plan