Skip to content

refactor: use ForgeConfig to build Environment#2698

Merged
tusharmath merged 36 commits intomainfrom
config-refactor-mar-26
Mar 27, 2026
Merged

refactor: use ForgeConfig to build Environment#2698
tusharmath merged 36 commits intomainfrom
config-refactor-mar-26

Conversation

@tusharmath
Copy link
Copy Markdown
Collaborator

  • refactor(config): adopt forge_config in domain and services
  • refactor(auth): remove auth logic and config fields

));

let config_repository = Arc::new(ForgeConfigRepository::new());
let config_repository = Arc::new(ForgeConfigRepository::new(infra.clone()));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Compilation Error: Parameter mismatch

This line calls ForgeConfigRepository::new(infra.clone()), passing an infra parameter. However, the new() method shown in forge_repo/src/app_config.rs (line 122) has the signature:

pub fn new() -> Self {
    Self { cache: Arc::new(Mutex::new(None)) }
}

The method does not accept any parameters, but this call is passing infra.clone(). This will fail to compile with an error like "this function takes 0 arguments but 1 argument was supplied".

Fix: Either update the ForgeConfigRepository::new() signature to accept the infra parameter:

pub fn new(infra: Arc<F>) -> Self {
    Self { cache: Arc::new(Mutex::new(None)), infra }
}

Or remove the parameter from this call if it's not needed:

let config_repository = Arc::new(ForgeConfigRepository::new());
Suggested change
let config_repository = Arc::new(ForgeConfigRepository::new(infra.clone()));
let config_repository = Arc::new(ForgeConfigRepository::new());

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Comment on lines +125 to +126
service_url: Url::parse(fc.services_url.as_str())
.unwrap_or_else(|_| Url::parse("http://api.forgecode.dev").unwrap()),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Security vulnerability: The fallback URL uses http:// instead of https://, creating an unencrypted connection if services_url parsing fails. This is inconsistent with the default in .forge.toml which uses https://api.forgecode.dev/.

service_url: Url::parse(fc.services_url.as_str())
    .unwrap_or_else(|_| Url::parse("https://api.forgecode.dev").unwrap()),

Changing to HTTPS ensures secure communication even when the config parsing fails.

Suggested change
service_url: Url::parse(fc.services_url.as_str())
.unwrap_or_else(|_| Url::parse("http://api.forgecode.dev").unwrap()),
service_url: Url::parse(fc.services_url.as_str())
.unwrap_or_else(|_| Url::parse("https://api.forgecode.dev").unwrap()),

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@tusharmath tusharmath force-pushed the config-refactor-mar-26 branch from 20098dd to 91b6acd Compare March 27, 2026 03:59
@tusharmath tusharmath changed the title config refactor mar 26 refactor: use ForgeConfig to build Environment Mar 27, 2026
impl<
A: Services,
F: CommandInfra + EnvironmentInfra + SkillRepository + AppConfigRepository + GrpcInfra,
F: CommandInfra + EnvironmentInfra + SkillRepository + EnvironmentInfra + GrpcInfra,
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Duplicate trait bound EnvironmentInfra appears twice in the same trait bound list. This should only appear once.

// Should be:
F: CommandInfra + EnvironmentInfra + SkillRepository + GrpcInfra,

The duplicate EnvironmentInfra appears to be a copy-paste error when replacing AppConfigRepository with EnvironmentInfra.

Suggested change
F: CommandInfra + EnvironmentInfra + SkillRepository + EnvironmentInfra + GrpcInfra,
F: CommandInfra + EnvironmentInfra + SkillRepository + GrpcInfra,

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

Comment on lines 125 to 126
impl<F: EnvironmentInfra + EnvironmentInfra + FileReaderInfra + FileWriterInfra + HttpInfra>
ForgeProviderRepository<F>
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Duplicate trait bound EnvironmentInfra appears twice. This should only appear once.

// Should be:
impl<F: EnvironmentInfra + FileReaderInfra + FileWriterInfra + HttpInfra>
    ForgeProviderRepository<F>
Suggested change
impl<F: EnvironmentInfra + EnvironmentInfra + FileReaderInfra + FileWriterInfra + HttpInfra>
ForgeProviderRepository<F>
impl<F: EnvironmentInfra + FileReaderInfra + FileWriterInfra + HttpInfra>
ForgeProviderRepository<F>

Spotted by Graphite

Fix in Graphite


Is this helpful? React 👍 or 👎 to let us know.

@tusharmath tusharmath enabled auto-merge (squash) March 27, 2026 10:19
@tusharmath tusharmath merged commit 1b9e677 into main Mar 27, 2026
9 checks passed
@tusharmath tusharmath deleted the config-refactor-mar-26 branch March 27, 2026 10:21
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.

2 participants