Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion src/ai_company/config/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
.. autosummary::
load_config
load_config_from_string
discover_config
default_config_dict
RootConfig
AgentConfig
Expand All @@ -27,7 +28,11 @@
ConfigParseError,
ConfigValidationError,
)
from ai_company.config.loader import load_config, load_config_from_string
from ai_company.config.loader import (
discover_config,
load_config,
load_config_from_string,
)
from ai_company.config.schema import (
AgentConfig,
ProviderConfig,
Expand All @@ -50,6 +55,7 @@
"RoutingConfig",
"RoutingRuleConfig",
"default_config_dict",
"discover_config",
"load_config",
"load_config_from_string",
]
117 changes: 113 additions & 4 deletions src/ai_company/config/loader.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

import copy
import logging
import os
import re
from pathlib import Path
from typing import Any

Expand All @@ -19,6 +21,15 @@

logger = logging.getLogger(__name__)

_ENV_VAR_PATTERN = re.compile(r"\$\{([^}:]+?)(?::-([^}]*))?\}")

_CWD_CONFIG_LOCATIONS: tuple[Path, ...] = (
Path("ai-company.yaml"),
Path("config/ai-company.yaml"),
)

_HOME_CONFIG_RELATIVE = Path(".ai-company") / "config.yaml"

# ---------------------------------------------------------------------------
# Private helpers
# ---------------------------------------------------------------------------
Expand Down Expand Up @@ -263,13 +274,97 @@ def _validate_config_dict(
) from exc


def _substitute_env_vars(
data: dict[str, Any],
*,
source_file: str | None = None,
) -> dict[str, Any]:
"""Substitute ``${VAR}`` and ``${VAR:-default}`` in string values.

Walks the dict recursively, replacing environment variable
placeholders in string values. Non-string values (int, float,
bool, None) are passed through unchanged. Returns a new dict;
the input is never mutated.

Args:
data: Configuration dict to process.
source_file: File path label for error messages.

Returns:
A new dict with all env var placeholders resolved.

Raises:
ConfigValidationError: If a referenced env var is not set
and no default is provided.
"""

def _resolve_match(match: re.Match[str]) -> str:
var_name = match.group(1)
default = match.group(2)
value = os.environ.get(var_name)
if value is not None:
return value
if default is not None:
return default
msg = (
f"Environment variable '{var_name}' is not set and no default was provided"
)
raise ConfigValidationError(
msg,
locations=(ConfigLocation(file_path=source_file),),
)

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-high high

The _substitute_env_vars function allows unrestricted access to environment variables, posing a significant security risk. If configuration is provided by an untrusted source (e.g., via an API endpoint), an attacker could leak sensitive environment variables (e.g., DB_PASSWORD, API_KEYS, SECRET_KEY) if the resolved configuration is subsequently exposed. To mitigate this, consider implementing an allow-list of permitted environment variable names or providing an option to disable environment variable substitution when processing configuration from untrusted sources. Additionally, the current error reporting for missing environment variables only includes the source_file. Improving debuggability by tracking and including the key path in the ConfigLocation of the error would be beneficial to pinpoint the exact configuration key causing the issue.


def _walk(node: Any) -> Any:
if isinstance(node, str):
return _ENV_VAR_PATTERN.sub(_resolve_match, node)
if isinstance(node, dict):
return {key: _walk(value) for key, value in node.items()}
if isinstance(node, list):
return [_walk(item) for item in node]
return node

result: dict[str, Any] = _walk(data)
return result
Comment thread
coderabbitai[bot] marked this conversation as resolved.


# ---------------------------------------------------------------------------
# Public API
# ---------------------------------------------------------------------------


def discover_config() -> Path:
"""Auto-discover a configuration file from well-known locations.

Search order:

1. ``./ai-company.yaml``
2. ``./config/ai-company.yaml``
3. ``~/.ai-company/config.yaml``

Returns:
Resolved absolute :class:`~pathlib.Path` to the first file found.

Raises:
ConfigFileNotFoundError: If no configuration file is found
at any searched location.
"""
candidates = [*_CWD_CONFIG_LOCATIONS, Path.home() / _HOME_CONFIG_RELATIVE]
for candidate in candidates:
if candidate.is_file():
return candidate.resolve()

searched = [str(c) for c in candidates]
msg = "No configuration file found. Searched:\n" + "\n".join(
f" - {p}" for p in searched
)
raise ConfigFileNotFoundError(
msg,
locations=tuple(ConfigLocation(file_path=p) for p in searched),
)


def load_config(
config_path: Path | str,
config_path: Path | str | None = None,
*,
override_paths: tuple[Path | str, ...] = (),
) -> RootConfig:
Expand All @@ -280,6 +375,11 @@ def load_config(
1. Built-in defaults (from :func:`default_config_dict`).
2. Primary config file at *config_path*.
3. Override files in order.
4. Environment variable substitution (``${VAR}`` /
``${VAR:-default}``).

When *config_path* is ``None``, :func:`discover_config` is called
to auto-discover the configuration file.

.. note::

Expand All @@ -289,18 +389,23 @@ def load_config(
file's line numbers or lack location information entirely.

Args:
config_path: Path to the primary config file.
config_path: Path to the primary config file, or ``None``
to auto-discover.
override_paths: Additional config files layered on top.

Returns:
Validated, frozen :class:`RootConfig`.

Raises:
ConfigFileNotFoundError: If any config file does not exist.
ConfigFileNotFoundError: If any config file does not exist
(or discovery finds nothing).
ConfigParseError: If any file contains invalid YAML or cannot
be read.
ConfigValidationError: If the merged config fails validation.
ConfigValidationError: If the merged config fails validation
or an env var is missing.
"""
if config_path is None:
config_path = discover_config()
config_path = Path(config_path)

# 1. Start with built-in defaults
Expand All @@ -317,6 +422,9 @@ def load_config(
override = _parse_yaml_file(Path(override_path))
merged = _deep_merge(merged, override)

# 4. Substitute environment variables
merged = _substitute_env_vars(merged, source_file=str(config_path))

Copilot AI Feb 28, 2026

Copy link

Choose a reason for hiding this comment

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

In load_config, env var substitution errors are always attributed to the primary config_path via source_file=str(config_path). If a placeholder comes from an override file, the resulting ConfigValidationError will misleadingly point at the primary config. Consider passing a more accurate label (e.g., include override paths or use a generic "" source) so missing-var errors don’t misattribute the source file.

Suggested change
# 4. Substitute environment variables
merged = _substitute_env_vars(merged, source_file=str(config_path))
# 4. Substitute environment variables on the fully merged config.
# Use a neutral label so env-var errors aren't misattributed solely
# to the primary config file when they may originate from overrides.
merged = _substitute_env_vars(merged, source_file="<merged config>")

Copilot uses AI. Check for mistakes.

# Build line map from primary file for error enrichment
line_map = _build_line_map(yaml_text)

Expand Down Expand Up @@ -351,6 +459,7 @@ def load_config_from_string(
"""
data = _parse_yaml_string(yaml_string, source_name)
merged = _deep_merge(default_config_dict(), data)
merged = _substitute_env_vars(merged, source_file=source_name)
line_map = _build_line_map(yaml_string)
return _validate_config_dict(
merged,
Expand Down
25 changes: 25 additions & 0 deletions tests/unit/config/conftest.py
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,31 @@ class RootConfigFactory(ModelFactory):
total_monthly: -100.0
"""

ENV_VAR_SIMPLE_YAML = """\
company_name: ${COMPANY_NAME}
"""

ENV_VAR_DEFAULT_YAML = """\
company_name: ${COMPANY_NAME:-Fallback Corp}
"""

Copilot AI Feb 28, 2026

Copy link

Choose a reason for hiding this comment

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

ENV_VAR_DEFAULT_YAML is added here but isn’t referenced anywhere in the test suite. If it’s not needed, removing it would reduce noise; if it is intended, add a test that uses it so the fixture stays meaningful.

Suggested change
ENV_VAR_DEFAULT_YAML = """\
company_name: ${COMPANY_NAME:-Fallback Corp}
"""

Copilot uses AI. Check for mistakes.
ENV_VAR_NESTED_YAML = """\
company_name: ${COMPANY_NAME}
budget:
total_monthly: 500.0
alerts:
warn_at: 75
critical_at: 90
hard_stop_at: 100
providers:
anthropic:
base_url: ${ANTHROPIC_BASE_URL:-https://api.anthropic.com}
"""

ENV_VAR_MISSING_YAML = """\
company_name: ${UNDEFINED_VAR}
"""


# ── Fixtures ──────────────────────────────────────────────────────

Expand Down
Loading
Loading