feat(provider): add AdaL (SylphAI) as a built-in provider#1
Closed
chindris-mihai-alexandru wants to merge 21 commits intomainfrom
Closed
feat(provider): add AdaL (SylphAI) as a built-in provider#1chindris-mihai-alexandru wants to merge 21 commits intomainfrom
chindris-mihai-alexandru wants to merge 21 commits intomainfrom
Conversation
…2814) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…#2813) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
tailcallhq#2762) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Amit Singh <amitksingh1490@gmail.com>
…ailcallhq#2790) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Amit Singh <amitksingh1490@gmail.com> Co-authored-by: ForgeCode <noreply@forgecode.dev>
…ailcallhq#2783) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: Amit Singh <amitksingh1490@gmail.com>
…lcallhq#2837) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…#2849) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
tailcallhq#2861) Co-authored-by: ForgeCode <noreply@forgecode.dev> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
…dling (tailcallhq#2803) Co-authored-by: Aiswarya Prakasan <aiswaryaprakasan@Aiswaryas-MacBook-Pro.local> Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com> Co-authored-by: Tushar Mathur <tusharmath@gmail.com> Co-authored-by: Amit Singh <amitksingh1490@gmail.com>
…ugh the stack (tailcallhq#2850) Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Review Summary by QodoEager config initialization, provider ecosystem expansion, and OAuth localhost callback support
WalkthroughsDescription **Major refactoring for eager configuration initialization and provider ecosystem expansion:**
• **Configuration threading**: Refactored config initialization from lazy (with silent errors) to
eager at application startup, reading ForgeConfig once in main.rs and threading it through all
layers (API, services, repositories, apps) to eliminate repeated disk reads and surface errors early
• **Provider ecosystem expansion**: Added GoogleAIStudio and AdaL (SylphAI) as built-in
providers; refreshed Bedrock model catalog with 100+ new models across Google, Qwen, Mistral, Meta,
DeepSeek, Anthropic, NVIDIA, MiniMax, OpenAI, Writer, and Zai; added inline provider definitions
support via forge.toml
• **OAuth improvements**: Implemented LocalhostOAuthCallbackServer for capturing OAuth browser
redirects on localhost with automatic redirect capture; added OpenID Connect support via id_token
field; enhanced credential enrichment for Codex OAuth strategies
• **Tool call argument normalization**: Added NormalizeToolCallArguments transformer to detect and
parse stringified JSON arguments (e.g., from kimi-k2p5-turbo API), converting to structured JSON
with fallback to raw content for malformed data
• **Streaming usage fix**: Changed Anthropic streaming usage accumulation from sum to max-based
merge strategy to fix double-counting where message_start and message_delta both report token
counts
• **Bedrock improvements**: Added SanitizeToolIds transformer to replace invalid characters in
tool call IDs with underscores (Bedrock requires ^[a-zA-Z0-9_-]+$); fixed prompt token calculation
to use input_tokens instead of total_tokens
• **Conversation tracking**: Added initiator field to Context to track conversation origin
("user" or "agent"); persisted in conversation records for audit trails
• **File system improvements**: Added symlink exclusion from file discovery pipeline in both walker
and fd modules; added is_symlink() helper function
• **Shell plugin refactoring**: Removed config-provider command; renamed model-reset to
config-reload with expanded functionality to clear reasoning effort; updated variable declarations
for better scoping
• **Dependency updates**: Updated rustyline (17→18), sha2 (0.10→0.11), similar (2.4→3.0),
toml_edit (0.22→0.25), posthog-rs (0.4.7→0.5.0); added hex, tiny_http, and regex crates
• **Comprehensive test coverage**: Added tests for stringified tool call arguments, symlink
exclusion, Anthropic usage merging, Bedrock token counting, OAuth callback validation, and provider
configuration
Diagramflowchart LR
A["main.rs<br/>Read ForgeConfig"] -->|"pass config"| B["ForgeAPI"]
B -->|"pass config"| C["ForgeInfra"]
B -->|"pass config"| D["ForgeServices"]
D -->|"pass config"| E["ForgeApp"]
D -->|"pass config"| F["ToolRegistry"]
E -->|"pass config"| G["Orchestrator"]
F -->|"pass config"| H["ToolExecutor"]
B -->|"pass config"| I["ForgeRepo"]
I -->|"inline providers"| J["ProviderRepository"]
K["OAuth Callback<br/>LocalhostServer"] -->|"capture redirect"| L["Browser"]
M["Tool Arguments<br/>Stringified JSON"] -->|"normalize"| N["NormalizeToolCallArguments"]
O["Streaming Usage<br/>Anthropic"] -->|"merge max"| P["Usage Accumulation"]
Q["Bedrock Tool IDs"] -->|"sanitize"| R["SanitizeToolIds"]
File Changes1. crates/forge_main/src/ui.rs
|
Code Review by Qodo
1. ADAL const missing rustdoc
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors configuration management to read settings at startup and propagate them through the system, eliminating lazy disk reads and silent failures. It introduces a localhost OAuth callback server for streamlined authentication and adds support for Google AI Studio and AdaL providers. Key improvements include normalizing stringified tool call arguments and sanitizing tool call IDs for Bedrock compatibility. Feedback highlighted that the JWT claim extraction for ChatGPT account IDs lacks signature verification and should be documented as an insecure method.
Comment on lines
+65
to
+93
| fn extract_chatgpt_account_id(token: &str) -> Option<String> { | ||
| let parts: Vec<&str> = token.split('.').collect(); | ||
| if parts.len() != 3 { | ||
| return None; | ||
| } | ||
| use base64::Engine; | ||
| let payload = base64::engine::general_purpose::URL_SAFE_NO_PAD | ||
| .decode(parts[1]) | ||
| .ok()?; | ||
| let claims: serde_json::Value = serde_json::from_slice(&payload).ok()?; | ||
|
|
||
| // Try chatgpt_account_id first | ||
| if let Some(id) = claims["chatgpt_account_id"].as_str() { | ||
| return Some(id.to_string()); | ||
| } | ||
| // Try nested auth claim | ||
| if let Some(id) = claims["https://api.openai.com/auth"]["chatgpt_account_id"].as_str() { | ||
| return Some(id.to_string()); | ||
| } | ||
| // Fall back to organizations[0].id | ||
| if let Some(id) = claims["organizations"] | ||
| .as_array() | ||
| .and_then(|orgs| orgs.first()) | ||
| .and_then(|org| org["id"].as_str()) | ||
| { | ||
| return Some(id.to_string()); | ||
| } | ||
| None | ||
| } |
There was a problem hiding this comment.
The extract_chatgpt_account_id function performs base64 decoding and JSON parsing on a token without verifying the token's signature. While this is acceptable for extracting claims for non-security purposes, it should be clearly documented that this is an insecure extraction method.
There was a problem hiding this comment.
Pull request overview
This PR introduces AdaL (SylphAI) as a built-in provider and expands Forge’s provider/config system, alongside a broader refactor to read config eagerly at startup (and thread config through services) plus several related UX, provider, and tooling improvements.
Changes:
- Added built-in provider IDs (incl.
adal) and expanded provider configuration support (inlineprovidersinforge.toml, schema updates). - Refactored config access to avoid
EnvironmentInfra::get_config()and instead pass startup config into repos/services/tools; added config reload behavior in the shell plugin/UI. - Added OAuth localhost callback helper, symlink exclusion in discovery/walker paths, and several provider/request/usage normalization fixes (tool args, streaming usage merge, Bedrock tool id sanitization).
Reviewed changes
Copilot reviewed 83 out of 84 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| shell-plugin/lib/highlight.zsh | Adjusts zsh-syntax-highlighting pattern setup |
| shell-plugin/lib/dispatcher.zsh | Updates command dispatch aliases (config reload/provider login) |
| shell-plugin/lib/config.zsh | Changes plugin variable scoping/visibility and adds session reasoning effort var |
| shell-plugin/lib/actions/config.zsh | Removes provider picker action; adds config reload action; uses interactive exec for workspace ops |
| shell-plugin/forge.theme.zsh | Simplifies prompt subst; suppresses prompt command stderr |
| plans/2026-04-05-config-init-at-startup-v1.md | Design plan for eager config read / trait changes |
| forge.schema.json | Adds schema for max_commit_count and inline providers |
| crates/forge_walker/src/walker.rs | Skips symlinks during walk + adds tests |
| crates/forge_tracker/Cargo.toml | Bumps posthog-rs dependency |
| crates/forge_services/src/tool_services/shell.rs | Removes test mock get_config implementation |
| crates/forge_services/src/tool_services/image_read.rs | Injects max image size via constructor instead of config lookup |
| crates/forge_services/src/tool_services/fs_read.rs | Injects file/image/line limits via constructor instead of config lookup |
| crates/forge_services/src/forge_services.rs | Threads config into services; removes EnvironmentInfra bound; config-driven construction |
| crates/forge_services/src/fd.rs | Excludes symlinks from discovered files + adds tests |
| crates/forge_services/src/discovery.rs | Removes test mock get_config implementation |
| crates/forge_services/src/context_engine.rs | Injects batch size instead of reading config via infra |
| crates/forge_services/src/auth.rs | Injects services_url instead of reading config via infra |
| crates/forge_services/src/attachment.rs | Injects max_read_lines instead of reading config via infra |
| crates/forge_services/src/app_config.rs | Stores/updates config in-memory mutex; adds atomic provider+model setter |
| crates/forge_services/src/agent_registry.rs | Uses injected startup config for resolving defaults when loading agents |
| crates/forge_repo/src/skill.rs | Updates tests to new ForgeInfra constructor |
| crates/forge_repo/src/provider/provider_repo.rs | Merges inline providers; adds conversions from config provider entries |
| crates/forge_repo/src/provider/openai.rs | Adds GitHub Copilot optimization headers + tests |
| crates/forge_repo/src/provider/openai_responses/response.rs | Adds doc comment re: usage conversion |
| crates/forge_repo/src/provider/mod.rs | Adds bedrock tool-id sanitization module |
| crates/forge_repo/src/provider/chat.rs | Injects retry config + model cache TTL instead of reading config |
| crates/forge_repo/src/provider/bedrock.rs | Adds tool-id sanitization; fixes usage mapping; updates tests for initiator field |
| crates/forge_repo/src/provider/bedrock_sanitize_ids.rs | New transformer to sanitize Bedrock tool call IDs + tests |
| crates/forge_repo/src/provider/bedrock_cache.rs | Updates tests for initiator field |
| crates/forge_repo/src/forge_repo.rs | Threads config into repo; removes get_config delegation |
| crates/forge_repo/src/conversation/conversation_record.rs | Persists initiator in context records |
| crates/forge_repo/Cargo.toml | Adds regex dependency |
| crates/forge_main/src/zsh/plugin.rs | Removes typeset -g when setting loaded markers |
| crates/forge_main/src/ui.rs | Threads config into UI factory; refreshes config on /new; OAuth callback support; provider/model selection tweaks |
| crates/forge_main/src/oauth_callback.rs | New localhost OAuth callback server helper |
| crates/forge_main/src/main.rs | Eager config read + services_url validation; improved error printing |
| crates/forge_main/src/lib.rs | Exposes oauth_callback module |
| crates/forge_main/src/built_in_commands.json | Removes config-provider; replaces model-reset with config-reload; provider alias updates |
| crates/forge_main/Cargo.toml | Adds tiny_http dependency |
| crates/forge_infra/src/forge_infra.rs | New ForgeInfra constructor taking config + services_url; removes trait get_config |
| crates/forge_infra/src/env.rs | Seeds config cache at construction; makes cached_config fallible; removes silent defaulting |
| crates/forge_infra/src/auth/util.rs | Adds id_token field to token responses |
| crates/forge_infra/src/auth/strategy.rs | Adds id_token support + Codex credential enrichment refactor |
| crates/forge_fs/src/lib.rs | Uses hex::encode for hashing |
| crates/forge_fs/Cargo.toml | Adds hex dependency |
| crates/forge_domain/tests/test_stringified_tool_calls.rs | New integration tests for stringified tool args roundtrip |
| crates/forge_domain/src/transformer/normalize_tool_args.rs | New transformer to normalize tool call args in context |
| crates/forge_domain/src/transformer/mod.rs | Exports NormalizeToolCallArguments |
| crates/forge_domain/src/tools/call/args.rs | Repairs stringified JSON tool args; adds normalize() + tests |
| crates/forge_domain/src/result_stream_ext.rs | Changes streaming usage aggregation strategy (merge vs accumulate) |
| crates/forge_domain/src/provider.rs | Adds ProviderId constants (incl. ADAL, Google AI Studio) and mappings/tests |
| crates/forge_domain/src/message.rs | Adds Usage::merge and related tests |
| crates/forge_domain/src/context.rs | Adds initiator to Context; adds TokenCount::max helper |
| crates/forge_domain/src/auth/auth_token_response.rs | Adds id_token field to OAuthTokenResponse |
| crates/forge_config/src/config.rs | Adds provider entry types; adds max_commit_count + inline providers |
| crates/forge_config/.forge.toml | Sets max_commit_count default in embedded defaults |
| crates/forge_app/src/utils.rs | Uses hex::encode for hashing |
| crates/forge_app/src/tool_registry.rs | Threads config into tool registry/executors; removes config lookups from services |
| crates/forge_app/src/tool_executor.rs | Uses injected config instead of services.get_config() |
| crates/forge_app/src/services.rs | Adds AppConfigService atomic provider+model setter |
| crates/forge_app/src/orch.rs | Threads config into orchestrator; adds NormalizeToolCallArguments to pipeline |
| crates/forge_app/src/orch_spec/orch_runner.rs | Updates orchestrator construction and AgentService signature |
| crates/forge_app/src/infra.rs | Removes get_config() from EnvironmentInfra trait |
| crates/forge_app/src/hooks/title_generation.rs | Updates AgentService signature in tests |
| crates/forge_app/src/git_app.rs | Threads config into GitApp; uses max_commit_count |
| crates/forge_app/src/dto/openai/transformers/set_cache.rs | Updates tests for initiator field |
| crates/forge_app/src/dto/openai/transformers/drop_tool_call.rs | Updates tests for initiator field |
| crates/forge_app/src/dto/openai/request.rs | Adds non-serialized initiator field to OpenAI request DTO |
| crates/forge_app/src/dto/anthropic/transforms/set_cache.rs | Updates tests for initiator field |
| crates/forge_app/src/dto/anthropic/transforms/auth_system_message.rs | Updates tests for initiator field |
| crates/forge_app/src/dto/anthropic/response.rs | Switches test expectations to merge semantics for usage |
| crates/forge_app/src/command_generator.rs | Removes test mock get_config; updates AppConfigService mock |
| crates/forge_app/src/changed_files.rs | Passes parallel read limit explicitly rather than reading config |
| crates/forge_app/src/app.rs | Threads config into ForgeApp; improves conversation-not-found handling |
| crates/forge_app/src/agent.rs | Threads config through AgentService tool call execution path |
| crates/forge_app/src/agent_executor.rs | Sets context initiator=agent for spawned agent conversations; threads config into ForgeApp |
| crates/forge_app/Cargo.toml | Adds hex dependency |
| crates/forge_api/src/forge_api.rs | Threads config through ForgeAPI init; removes get_config; adds atomic provider+model setter |
| crates/forge_api/src/api.rs | Removes get_config from API trait; adds atomic provider+model setter |
| Cargo.toml | Dependency upgrades/additions (rustyline/sha2/similar/toml_edit/tiny_http/hex) |
| Cargo.lock | Lockfile updates for dependency changes |
| .github/ISSUE_TEMPLATE/bug_report.yml | Simplifies OS field; removes logs textarea and OS dropdown |
| .gitattributes | Forces LF for *.zsh files |
Comments suppressed due to low confidence (2)
crates/forge_services/src/agent_registry.rs:95
ForgeAgentRegistryServicestores a startupForgeConfigsnapshot and uses it inload_agents(). After a user changes the default provider/model (e.g. viaset_default_provider_and_model),reload_agents()will still reload using the stale snapshot, so agents without explicit provider/model won’t inherit the updated session defaults. Consider storing shared, updatable config (e.g.Arc<Mutex<ForgeConfig>>shared withForgeAppConfigService), or re-reading config from disk insideload_agents()/reload_agents()before callingrepository.get_agents(...).
shell-plugin/lib/highlight.zsh:17- Without the global
typeset -gA ZSH_HIGHLIGHT_PATTERNS/typeset -ga ZSH_HIGHLIGHT_HIGHLIGHTERSdeclarations, sourcing this file before zsh-syntax-highlighting initializes its globals can create the variables with the wrong type (e.g. indexed array instead of associative), breaking highlighting. Consider restoring the global type declarations (ideally guarded so they don’t override an existing correctly-typed var).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+112
to
119
| async fn set_default_provider_and_model( | ||
| &self, | ||
| provider_id: ProviderId, | ||
| model: ModelId, | ||
| ) -> anyhow::Result<()> { | ||
| self.update(ConfigOperation::SetModel(provider_id, model)) | ||
| .await | ||
| } |
There was a problem hiding this comment.
set_default_provider_and_model persists the change via update_environment, but it never updates the in-memory self.config cache. After calling this method, get_default_provider() / get_provider_model() will continue returning the previous values until the process is restarted or /new reloads config. Update the cached ForgeConfig (session.provider_id + session.model_id) after the operation succeeds, similar to set_default_provider and set_default_model.
Comment on lines
159
to
163
| async fn on_new(&mut self) -> Result<()> { | ||
| self.api = Arc::new((self.new_api)()); | ||
| let config = forge_config::ForgeConfig::read().unwrap_or_default(); | ||
| self.config = config.clone(); | ||
| self.api = Arc::new((self.new_api)(config)); | ||
| self.init_state(false).await?; |
There was a problem hiding this comment.
on_new re-reads config using ForgeConfig::read().unwrap_or_default(), which silently falls back to defaults on parse/deserialization errors. This undermines the new “surface config errors” behavior added in main.rs. Prefer propagating the error to the UI (and keeping the existing config on failure) rather than defaulting silently.
Comment on lines
+66
to
+70
| pub fn merge(mut self, other: &Usage) -> Self { | ||
| self.prompt_tokens = self.prompt_tokens.max(other.prompt_tokens); | ||
| self.completion_tokens = self.completion_tokens.max(other.completion_tokens); | ||
| self.total_tokens = self.total_tokens.max(other.total_tokens); | ||
| self.cached_tokens = self.cached_tokens.max(other.cached_tokens); |
There was a problem hiding this comment.
Usage::merge currently takes max() for total_tokens, which can break the invariant that total_tokens == prompt_tokens + completion_tokens (see Anthropic conversion where total is computed as prompt+completion). With partial/cumulative streaming events, max(total) will undercount (e.g. prompt=1000, completion=50 => total should be 1050). Consider recomputing total_tokens from the merged prompt/completion (or otherwise ensuring consistency) instead of maxing the precomputed totals.
Comment on lines
+4
to
14
| # Using typeset to keep variables local to plugin scope and prevent public exposure | ||
|
|
||
| typeset -gh _FORGE_BIN="${FORGE_BIN:-forge}" | ||
| typeset -gh _FORGE_CONVERSATION_PATTERN=":" | ||
| typeset -gh _FORGE_MAX_COMMIT_DIFF="${FORGE_MAX_COMMIT_DIFF:-100000}" | ||
| typeset -gh _FORGE_DELIMITER='\s\s+' | ||
| typeset -gh _FORGE_PREVIEW_WINDOW="--preview-window=bottom:75%:wrap:border-sharp" | ||
| typeset -h _FORGE_BIN="${FORGE_BIN:-forge}" | ||
| typeset -h _FORGE_CONVERSATION_PATTERN=":" | ||
| typeset -h _FORGE_MAX_COMMIT_DIFF="${FORGE_MAX_COMMIT_DIFF:-100000}" | ||
| typeset -h _FORGE_DELIMITER='\s\s+' | ||
| typeset -h _FORGE_PREVIEW_WINDOW="--preview-window=bottom:75%:wrap:border-sharp" | ||
|
|
||
| # Detect fd command - Ubuntu/Debian use 'fdfind', others use 'fd' | ||
| typeset -gh _FORGE_FD_CMD="$(command -v fdfind 2>/dev/null || command -v fd 2>/dev/null || echo 'fd')" | ||
| typeset -h _FORGE_FD_CMD="$(command -v fdfind 2>/dev/null || command -v fd 2>/dev/null || echo 'fd')" | ||
|
|
There was a problem hiding this comment.
These plugin variables are declared with typeset -h inside a sourced script. If the plugin is sourced from within a function (common with lazy-loading plugin managers), typeset creates local variables unless -g is used, so _FORGE_* variables may disappear when the sourcing function returns. Restore global declarations (e.g. typeset -gh ...) to ensure the plugin works reliably under lazy loaders while still keeping variables hidden.
Comment on lines
+141
to
+176
| #[test] | ||
| fn test_filter_and_resolve_excludes_symlinks() { | ||
| let dir = tempdir().unwrap(); | ||
| let base = dir.path(); | ||
|
|
||
| // Real file with an allowed extension. | ||
| let real_path = base.join("main.rs"); | ||
| File::create(&real_path) | ||
| .unwrap() | ||
| .write_all(b"fn main() {}") | ||
| .unwrap(); | ||
|
|
||
| // Symlink pointing to the real file (also carries an allowed extension). | ||
| let link_path = base.join("link.rs"); | ||
| std::os::unix::fs::symlink(&real_path, &link_path).unwrap(); | ||
|
|
||
| let paths = vec!["main.rs".to_string(), "link.rs".to_string()]; | ||
| let actual = filter_and_resolve(base, paths).unwrap(); | ||
|
|
||
| let expected = vec![base.join("main.rs")]; | ||
| assert_eq!(actual, expected); | ||
| } | ||
|
|
||
| #[test] | ||
| fn test_filter_and_resolve_excludes_dangling_symlinks() { | ||
| let dir = tempdir().unwrap(); | ||
| let base = dir.path(); | ||
|
|
||
| // Real file with an allowed extension (keeps the result non-empty). | ||
| let real_path = base.join("lib.rs"); | ||
| File::create(&real_path).unwrap().write_all(b"").unwrap(); | ||
|
|
||
| // Dangling symlink — target does not exist. | ||
| let dangling = base.join("missing.rs"); | ||
| std::os::unix::fs::symlink(base.join("nonexistent.rs"), &dangling).unwrap(); | ||
|
|
There was a problem hiding this comment.
These tests use std::os::unix::fs::symlink, which will not compile/run on Windows targets. If the project CI includes Windows, gate the symlink-based tests with #[cfg(unix)] (or provide a Windows implementation using std::os::windows::fs::symlink_file/symlink_dir).
Comment on lines
+625
to
+635
| #[tokio::test] | ||
| async fn test_walker_excludes_symlinks() { | ||
| let fixture = fixtures::Fixture::default(); | ||
|
|
||
| // Real file that should appear in results. | ||
| fixture.add_file("real.txt", "content").unwrap(); | ||
|
|
||
| // Symlink pointing to the real file — must be excluded. | ||
| let link_path = fixture.as_path().join("link.txt"); | ||
| std::os::unix::fs::symlink(fixture.as_path().join("real.txt"), &link_path).unwrap(); | ||
|
|
There was a problem hiding this comment.
This test uses std::os::unix::fs::symlink, which will not compile/run on Windows targets. Gate with #[cfg(unix)] (or add a Windows symlink implementation) so the test suite remains portable.
Comment on lines
72
to
+110
| @@ -104,6 +106,8 @@ impl ProviderId { | |||
| ProviderId::OPENCODE_ZEN, | |||
| ProviderId::FIREWORKS_AI, | |||
| ProviderId::NOVITA, | |||
| ProviderId::GOOGLE_AI_STUDIO, | |||
| ProviderId::ADAL, | |||
There was a problem hiding this comment.
The PR title/description focus on adding the AdaL provider, but this PR also includes a large config-loading refactor (removing get_config from EnvironmentInfra), OAuth callback server, symlink filtering changes, zsh plugin behavior changes, and multiple dependency upgrades. Consider updating the PR description to reflect the full scope, or splitting into smaller PRs to reduce review/rollback risk.
Owner
Author
There was a problem hiding this comment.
Good catch — the fork PR includes upstream merge commits which inflate the diff. The upstream PR (antinomyhq/forgecode#2868) shows only the 2-file, 34-line AdaL change.
Regarding the AdaL provider specifically: the updated commit now includes 3 unit tests (test_adal_from_str, test_adal_display_name, test_adal_in_built_in_providers) and also fixes 3 pre-existing missing FromStr arms for vertex_ai_anthropic, bedrock, and opencode_zen.
Comment on lines
297
to
+306
| fn fixture_skill_repo() -> (ForgeSkillRepository<ForgeInfra>, std::path::PathBuf) { | ||
| let skill_dir = std::path::Path::new(env!("CARGO_MANIFEST_DIR")) | ||
| .join("src/fixtures/skills_with_resources"); | ||
| let infra = Arc::new(ForgeInfra::new(std::env::current_dir().unwrap())); | ||
| let config = ForgeConfig::read().unwrap_or_default(); | ||
| let services_url = config.services_url.parse().unwrap(); | ||
| let infra = Arc::new(ForgeInfra::new( | ||
| std::env::current_dir().unwrap(), | ||
| config, | ||
| services_url, | ||
| )); |
There was a problem hiding this comment.
This test helper reads the user’s real ~/.forge.toml via ForgeConfig::read().unwrap_or_default(), making the unit test environment-dependent and potentially flaky (and it also silently swallows parse errors). Prefer using ForgeConfig::default() (or a minimal synthetic config with a known-valid services_url) so the test is deterministic and doesn’t depend on developer machine state.
Comment on lines
+70
to
+76
| "max_commit_count": { | ||
| "description": "Maximum number of recent commits included as context for commit message\ngeneration.", | ||
| "type": "integer", | ||
| "format": "uint", | ||
| "default": 0, | ||
| "minimum": 0 | ||
| }, |
There was a problem hiding this comment.
The schema lists max_commit_count.default as 0, but the embedded defaults file (crates/forge_config/.forge.toml) sets max_commit_count = 20, and runtime behavior (e.g. GitApp::fetch_git_context) depends on this being non-zero. Consider updating the schema default to match the actual default so validation/UI tooling doesn’t suggest an unusable value.
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+75
to
+76
| pub const GOOGLE_AI_STUDIO: ProviderId = ProviderId(Cow::Borrowed("google_ai_studio")); | ||
| pub const ADAL: ProviderId = ProviderId(Cow::Borrowed("adal")); |
There was a problem hiding this comment.
1. adal const missing rustdoc 📘 Rule violation ⚙ Maintainability
New public constants ProviderId::GOOGLE_AI_STUDIO and ProviderId::ADAL were added without /// Rustdoc comments. This violates the requirement that all public Rust items must be documented.
Agent Prompt
## Issue description
New public constants were added without required Rustdoc comments.
## Issue Context
Compliance requires `///` docs for all public Rust items (no code examples).
## Fix Focus Areas
- crates/forge_domain/src/provider.rs[72-76]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Owner
Author
There was a problem hiding this comment.
Fixed in the updated commit — added ProviderId::ADAL with full wiring:
built_in_providers()inclusiondisplay_name()mapping →"AdaL"FromStrparsing for"adal"- Also fixed 3 pre-existing missing
FromStrarms (vertex_ai_anthropic,bedrock,opencode_zen) - 3 new unit tests covering all of the above
All 20 provider tests pass ✅
Comment on lines
+1
to
+21
| //! Integration test for stringified tool call arguments fix | ||
| //! | ||
| //! This test verifies that when the API sends tool call arguments as a string | ||
| //! containing JSON (like kimi-k2p5-turbo does), we properly parse it and | ||
| //! serialize it back as a JSON object when sending to the API. | ||
|
|
||
| use forge_domain::{Context, ContextMessage, Role}; | ||
|
|
||
| /// Test that stringified tool call arguments from API are properly handled | ||
| /// | ||
| /// This simulates the exact scenario: API sends arguments as a string | ||
| /// containing JSON, and when we serialize the conversation back to send to API, | ||
| /// it should be a proper object. | ||
| #[test] | ||
| fn test_stringified_tool_call_arguments_roundtrip() { | ||
| // Simulate what kimi-k2p5-turbo sends: arguments as a string containing JSON | ||
| // Note: This is what the API sends us - a JSON string value containing JSON | ||
| // object | ||
| let conversation_json = r#"{ | ||
| "messages": [ | ||
| { |
There was a problem hiding this comment.
2. Integration tests not co-located 📘 Rule violation ⚙ Maintainability
New tests were added under crates/forge_domain/tests/ instead of being co-located with the source module via #[cfg(test)] mod tests. This violates the requirement to keep tests in the same file as the code under test.
Agent Prompt
## Issue description
Tests were added as a separate file under `crates/forge_domain/tests/`, but compliance requires unit tests to be co-located with the implementation.
## Issue Context
Refactor these tests into the relevant module file using `#[cfg(test)] mod tests { ... }`.
## Fix Focus Areas
- crates/forge_domain/tests/test_stringified_tool_calls.rs[1-313]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+160
to
+162
| let config = forge_config::ForgeConfig::read().unwrap_or_default(); | ||
| self.config = config.clone(); | ||
| self.api = Arc::new((self.new_api)(config)); |
There was a problem hiding this comment.
4. Config errors masked on /new 🐞 Bug ☼ Reliability
UI::on_new uses ForgeConfig::read().unwrap_or_default(), so a malformed/invalid config silently falls back to defaults after startup, undermining the PR’s “fail fast” config validation and causing unexpected runtime behavior. This also makes debugging configuration problems much harder because the error is discarded.
Agent Prompt
### Issue description
`UI::on_new()` calls `ForgeConfig::read().unwrap_or_default()`, swallowing config read/parse errors and replacing config with defaults.
### Issue Context
The PR explicitly adds startup-time validation to avoid silent fallback; `/new` should not reintroduce silent fallback.
### Fix Focus Areas
- crates/forge_main/src/ui.rs[159-163]
### Expected fix
- Replace `unwrap_or_default()` with error propagation (e.g., `ForgeConfig::read()?` plus a helpful `context(...)`).
- Ensure the UI prints a clear error and keeps the existing API/config if the reload fails (instead of resetting to defaults).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+111
to
118
| let services_url: Url = config | ||
| .services_url | ||
| .parse() | ||
| .context("services_url in configuration must be a valid URL")?; | ||
|
|
||
| // Handle worktree creation if specified | ||
| let cwd: PathBuf = match (&cli.sandbox, &cli.directory) { | ||
| (Some(sandbox), Some(cli)) => { |
There was a problem hiding this comment.
5. Services_url never refreshes 🐞 Bug ≡ Correctness
main.rs parses services_url once from the initial config and captures it in the UI API-factory closure, so later config changes to services_url (followed by /new) cannot take effect because new API instances still use the originally captured URL. This can route requests to the wrong workspace server despite the user updating configuration.
Agent Prompt
### Issue description
The API factory closure captures `services_url` computed from the initial config, so subsequent `ForgeConfig` reloads (e.g. `/new`) cannot change which gRPC workspace server is used.
### Issue Context
`UI::init`’s factory closure receives a `ForgeConfig` argument explicitly to reflect config changes in new conversations.
### Fix Focus Areas
- crates/forge_main/src/main.rs[104-133]
### Expected fix
- Compute/validate `services_url` *inside* the factory closure from the passed-in `config.services_url` (and return an error instead of capturing a stale value).
- If the closure cannot return `Result`, consider changing `ForgeAPI::init` to accept only `ForgeConfig` and perform parsing/validation internally, or change the UI factory signature to return `Result<ForgeAPI>` so invalid URLs are surfaced without panics/silent fallback.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
|
||
| async fn update_environment(&self, ops: Vec<ConfigOperation>) -> anyhow::Result<()> { | ||
| // Load the global config (with defaults applied) for the update round-trip | ||
| let mut fc = ConfigReader::default() |
There was a problem hiding this comment.
3. Legacy config lost on update 🐞 Bug ≡ Correctness
ForgeEnvironmentInfra::update_environment rebuilds the writable config from defaults+global only and writes it back, so any settings that exist only in the legacy ~/.forge/.config.json source will be silently dropped when a config operation is applied. After cache invalidation, cached_config also re-reads without legacy, making the session’s resolved config inconsistent with ForgeConfig::read().
Agent Prompt
### Issue description
`ForgeEnvironmentInfra::cached_config()` and `ForgeEnvironmentInfra::update_environment()` re-read config without `read_legacy()`, even though `ForgeConfig::read()` includes it. This can drop legacy-only settings (from `~/.forge/.config.json`) when any config update is persisted.
### Issue Context
- `ForgeConfig::read()` uses `ConfigReader::read_legacy()`.
- `update_environment()` writes a new global config derived from defaults+global only.
### Fix Focus Areas
- crates/forge_infra/src/env.rs[118-130]
- crates/forge_infra/src/env.rs[149-166]
### Expected fix
- Prefer calling `ForgeConfig::read()` for the base config used by `cached_config()` when cache is empty.
- In `update_environment()`, build the base config using the same merged sources as `ForgeConfig::read()` (at least `read_legacy()+read_defaults()+read_global()`; include `read_env()` if you intend env overrides to participate in the computed writeback), so applying an operation does not discard legacy-only fields.
- Add/adjust a regression test that starts with a config coming only from legacy source, runs `update_environment`, and asserts unrelated legacy-derived fields are preserved.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Add AdaL CLI by SylphAI as a new provider option in ForgeCode. AdaL is
registered as an OpenAI-compatible provider with api.sylph.ai endpoints,
enabling ForgeCode users to access AdaL's models via API key auth.
Changes:
- Add 'adal' entry to provider.json with OpenAI response type
- Add ProviderId::ADAL constant to forge_domain
- Wire up display_name ("AdaL"), FromStr, and built_in_providers()
- Add missing FromStr arms for vertex_ai_anthropic, bedrock, opencode_zen
- Add unit tests for ADAL display name, from_str, and built_in_providers
Co-Authored-By: ForgeCode <noreply@forgecode.dev>
chindris-mihai-alexandru
force-pushed
the
feat/adal-provider
branch
from
c933d80 to
57ac2a0
Compare
Owner
Author
|
Closing — the upstream PR (tailcallhq#2868) is the canonical one. This fork PR was only needed to push the branch. Once upstream merges, changes will be pulled back via |
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
Add AdaL (SylphAI) as a built-in provider in ForgeCode, enabling users to access AdaL's AI models directly from the ForgeCode terminal.
Motivation
AdaL CLI by SylphAI is a self-evolving coding agent that provides access to models from OpenAI, Anthropic, Google, and more. Adding AdaL as a provider in ForgeCode allows:
forge provider login adalADAL_API_KEYChanges
crates/forge_repo/src/provider/provider.jsonadalprovider entry with OpenAI-compatible response typehttps://api.sylph.ai/v1/chat/completionshttps://api.sylph.ai/v1/models(dynamic model fetching)ADAL_API_KEY)crates/forge_domain/src/provider.rsProviderId::ADALconstant"adal"tobuilt_in_providers()list"AdaL"display name mapping"adal"toFromStrimplementationTesting
cargo check -p forge_domainpassescargo test -p forge_domain -- provider)Related
🌸 Generated with AdaL