Skip to content

fix(CODEWIKI-003): CU-86akhf8u6 4 review findings in config.py #74

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
flamingo wants to merge 1 commit into
base: main
Choose a base branch
from
+132 −30
Draft

fix(CODEWIKI-003): CU-86akhf8u6 4 review findings in config.py #74

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
162 changes: 132 additions & 30 deletions codewiki/src/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,12 +17,15 @@
from dataclasses import dataclass, field, fields, asdict
from typing import Optional, List, Dict, Any
import argparse
import logging
import os
import sys
from dotenv import load_dotenv
from codewiki.src.be.flamingo_guidelines import escape_format_braces
load_dotenv()

logger = logging.getLogger(__name__)

# Constants
OUTPUT_BASE_DIR = 'output'
DEPENDENCY_GRAPHS_DIR = 'dependency_graphs'
Expand Down Expand Up @@ -66,6 +69,47 @@ def is_cli_context() -> bool:
'fallback_api_key',
})


def _to_int(value: Any, default: int) -> int:
"""Coerce a value to int, falling back to `default` on failure."""
if value is None:
return default
try:
return int(value)
except (ValueError, TypeError):
return default


def _to_float(value: Any, default: float) -> float:
"""Coerce a value to float, falling back to `default` on failure."""
if value is None:
return default
try:
return float(value)
except (ValueError, TypeError):
return default


def _to_bool(value: Any, default: bool) -> bool:
"""Coerce a value to bool, falling back to `default` on failure."""
if value is None:
return default
if isinstance(value, bool):
return value
if isinstance(value, str):
return value.strip().lower() in ('1', 'true', 'yes', 'on')
try:
return bool(value)
except (ValueError, TypeError):
return default


def _to_optional_str(value: Any) -> Optional[str]:
"""Coerce a value to str, preserving None."""
if value is None:
return None
return str(value)

@dataclass
class Config:
"""Configuration class for CodeWiki."""
Expand Down Expand Up @@ -118,35 +162,104 @@ class Config:
# When set, all paths are analyzed and merged into unified documentation
additional_source_paths: Optional[List[str]] = None

def to_dict(self, include_secrets: bool = False) -> Dict[str, Any]:

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 Config.to_dict() can include API key secrets when include_secrets=True is passed, contradicting CODEWIKI-003's absolute exclusion requirement

Removed the include_secrets parameter from Config.to_dict() entirely. The method now unconditionally excludes cluster_api_key/main_api_key/fallback_api_key from its output, with no code path able to include them, satisfying the "no exception" requirement in CODEWIKI-003.

🤖 Prompt for AI agents 
In codewiki/src/config.py around line 121, review and complete this code-review fix: Config.to_dict() can include API key secrets when include_secrets=True is passed, contradicting CODEWIKI-003's absolute exclusion requirement.
What the draft fix changed: Removed the `include_secrets` parameter from `Config.to_dict()` entirely. The method now unconditionally excludes cluster_api_key/main_api_key/fallback_api_key from its output, with no code path able to include them, satisfying the "no exception" requirement in CODEWIKI-003.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 Config.to_dict() delegates to asdict() instead of explicitly listing fields, violating explicit-field-handling requirement

Replaced asdict(self) + secret-field-popping in Config.to_dict() with an explicit dict literal listing every non-secret field by name. New fields added to Config in the future will no longer be silently serialized; a developer must explicitly add them to this literal.

🤖 Prompt for AI agents 
In codewiki/src/config.py around line 121, review and complete this code-review fix: Config.to_dict() delegates to asdict() instead of explicitly listing fields, violating explicit-field-handling requirement.
What the draft fix changed: Replaced `asdict(self)` + secret-field-popping in `Config.to_dict()` with an explicit dict literal listing every non-secret field by name. New fields added to `Config` in the future will no longer be silently serialized; a developer must explicitly add them to this literal.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 80 medium — react 👍/👎 to teach the reviewer

def to_dict(self) -> Dict[str, Any]:
"""
Serialize this Config to a plain dict.

By default, runtime-only secret fields (cluster_api_key, main_api_key,
fallback_api_key) are excluded from the result to prevent accidental
persistence, caching, or logging of API keys. Pass include_secrets=True
only when the caller explicitly needs to reconstruct a fully-functional
Config via from_dict() (e.g. in-process transfer within the same trust
boundary).
Runtime-only secret fields (cluster_api_key, main_api_key,
fallback_api_key) are never included in the result, with no exception,
to prevent accidental persistence, caching, or logging of API keys.

Each field is listed explicitly rather than delegating to
dataclasses.asdict(), so that adding a new field to Config requires an
explicit decision about whether it belongs in the serialized output.
"""
data = asdict(self)
if not include_secrets:
for secret_field in _RUNTIME_ONLY_SECRET_FIELDS:
data.pop(secret_field, None)
return data
return {
'repo_path': self.repo_path,
'output_dir': self.output_dir,
'dependency_graph_dir': self.dependency_graph_dir,
'docs_dir': self.docs_dir,
'max_depth': self.max_depth,
'main_model': self.main_model,
'cluster_model': self.cluster_model,
'fallback_model': self.fallback_model,
'cluster_base_url': self.cluster_base_url,
'main_base_url': self.main_base_url,
'fallback_base_url': self.fallback_base_url,
'cluster_api_version': self.cluster_api_version,
'main_api_version': self.main_api_version,
'fallback_api_version': self.fallback_api_version,
'cluster_max_tokens': self.cluster_max_tokens,
'main_max_tokens': self.main_max_tokens,
'fallback_max_tokens': self.fallback_max_tokens,
'max_token_per_module': self.max_token_per_module,
'max_token_per_leaf_module': self.max_token_per_leaf_module,
'cluster_temperature': self.cluster_temperature,
'main_temperature': self.main_temperature,
'fallback_temperature': self.fallback_temperature,
'cluster_temperature_supported': self.cluster_temperature_supported,
'main_temperature_supported': self.main_temperature_supported,
'fallback_temperature_supported': self.fallback_temperature_supported,
'cluster_max_token_field': self.cluster_max_token_field,
'main_max_token_field': self.main_max_token_field,
'fallback_max_token_field': self.fallback_max_token_field,
'agent_instructions': self.agent_instructions,
'diagrams_dir': self.diagrams_dir,
'additional_source_paths': self.additional_source_paths,
}

@classmethod

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🔴 Config.from_dict() has no explicit type coercion (int/float/bool) with named helper functions, unlike the CLI Configuration.from_dict()

Rewrote Config.from_dict() to construct the Config via explicit keyword arguments with named coercion helpers (_to_int, _to_float, _to_bool, _to_optional_str) mirroring the CLI's Configuration.from_dict() pattern, instead of filtering fields and passing them straight through. Required fields (repo_path, output_dir, dependency_graph_dir, docs_dir, model names, and the three api_key fields) are accessed via data[...] and will raise KeyError if missing, matching the documented contract that secrets must be supplied separately. Risk: this is a stricter/different error type (KeyError vs. the previous TypeError from cls(**filtered)) for missing required fields, and unknown extra keys in data are now silently ignored rather than filtered — both are acceptable given the finding's intent but change from_dict()'s exact failure mode, so callers relying on TypeError specifically should be checked.

🤖 Prompt for AI agents 
In codewiki/src/config.py around line 138, review and complete this code-review fix: Config.from_dict() has no explicit type coercion (int/float/bool) with named helper functions, unlike the CLI Configuration.from_dict().
What the draft fix changed: Rewrote `Config.from_dict()` to construct the `Config` via explicit keyword arguments with named coercion helpers (`_to_int`, `_to_float`, `_to_bool`, `_to_optional_str`) mirroring the CLI's `Configuration.from_dict()` pattern, instead of filtering fields and passing them straight through. Required fields (repo_path, output_dir, dependency_graph_dir, docs_dir, model names, and the three api_key fields) are accessed via `data[...]` and will raise KeyError if missing, matching the documented contract that secrets must be supplied separately. Risk: this is a stricter/different error type (KeyError vs. the previous TypeError from `cls(**filtered)`) for missing required fields, and unknown extra keys in `data` are now silently ignored rather than filtered — both are acceptable given the finding's intent but change from_dict()'s exact failure mode, so callers relying on TypeError specifically should be checked.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟡 65 medium — react 👍/👎 to teach the reviewer

def from_dict(cls, data: Dict[str, Any]) -> 'Config':
"""
Construct a Config from a dict previously produced by to_dict().

If secret fields (cluster_api_key, main_api_key, fallback_api_key) were
excluded (the default for to_dict()), they must be supplied separately
in `data` or this will raise a TypeError due to missing required fields.
Secret fields (cluster_api_key, main_api_key, fallback_api_key) are
never present in to_dict() output, so they must be supplied separately
in `data` or this will raise a TypeError due to missing required
fields.

Numeric and boolean fields are explicitly coerced via named helper
functions (_to_int, _to_float, _to_bool) so that values originating
from JSON (e.g. string '128000' for a token limit) are stored with
the correct type, matching the pattern used by the CLI's
Configuration.from_dict().
"""
known_fields = {f.name for f in fields(cls)}
filtered = {k: v for k, v in data.items() if k in known_fields}
return cls(**filtered)
return cls(
repo_path=data['repo_path'],
output_dir=data['output_dir'],
dependency_graph_dir=data['dependency_graph_dir'],
docs_dir=data['docs_dir'],
max_depth=_to_int(data.get('max_depth'), MAX_DEPTH),
main_model=data['main_model'],
cluster_model=data['cluster_model'],
fallback_model=data['fallback_model'],
cluster_api_key=data['cluster_api_key'],
main_api_key=data['main_api_key'],
fallback_api_key=data['fallback_api_key'],
cluster_base_url=_to_optional_str(data.get('cluster_base_url')),
main_base_url=_to_optional_str(data.get('main_base_url')),
fallback_base_url=_to_optional_str(data.get('fallback_base_url')),
cluster_api_version=_to_optional_str(data.get('cluster_api_version')),
main_api_version=_to_optional_str(data.get('main_api_version')),
fallback_api_version=_to_optional_str(data.get('fallback_api_version')),
cluster_max_tokens=_to_int(data.get('cluster_max_tokens'), DEFAULT_MAX_TOKENS),
main_max_tokens=_to_int(data.get('main_max_tokens'), DEFAULT_MAX_TOKENS),
fallback_max_tokens=_to_int(data.get('fallback_max_tokens'), DEFAULT_MAX_TOKENS),
max_token_per_module=_to_int(data.get('max_token_per_module'), DEFAULT_MAX_TOKEN_PER_MODULE),
max_token_per_leaf_module=_to_int(data.get('max_token_per_leaf_module'), DEFAULT_MAX_TOKEN_PER_LEAF_MODULE),
cluster_temperature=_to_float(data.get('cluster_temperature'), 0.0),
main_temperature=_to_float(data.get('main_temperature'), 0.0),
fallback_temperature=_to_float(data.get('fallback_temperature'), 0.0),
cluster_temperature_supported=_to_bool(data.get('cluster_temperature_supported'), True),
main_temperature_supported=_to_bool(data.get('main_temperature_supported'), True),
fallback_temperature_supported=_to_bool(data.get('fallback_temperature_supported'), True),
cluster_max_token_field=data.get('cluster_max_token_field', 'max_tokens'),
main_max_token_field=data.get('main_max_token_field', 'max_tokens'),
fallback_max_token_field=data.get('fallback_max_token_field', 'max_tokens'),
agent_instructions=data.get('agent_instructions'),
diagrams_dir=data.get('diagrams_dir'),
additional_source_paths=data.get('additional_source_paths'),
)

@property
def include_patterns(self) -> Optional[List[str]]:
Expand Down Expand Up @@ -199,9 +312,6 @@ def all_source_paths(self) -> List[str]:
List of absolute paths. Always includes repo_path as first element.
If additional_source_paths is None, returns single-element list.
"""
import logging

@flamingo flamingo Bot Sep 14, 2026

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

🦩 🟠 Loggers obtained inside methods via logging.getLogger(name) instead of once at module level

Moved import logging and logging.getLogger(__name__) to module level (once, right after the existing imports) and removed the per-method import logging; logger = logging.getLogger(__name__) lines from all_source_paths, validate_source_paths, is_multi_path_mode, and get_prompt_addition. All methods now reference the single module-level logger.

🤖 Prompt for AI agents 
In codewiki/src/config.py around line 202, review and complete this code-review fix: Loggers obtained inside methods via logging.getLogger(__name__) instead of once at module level.
What the draft fix changed: Moved `import logging` and `logging.getLogger(__name__)` to module level (once, right after the existing imports) and removed the per-method `import logging; logger = logging.getLogger(__name__)` lines from `all_source_paths`, `validate_source_paths`, `is_multi_path_mode`, and `get_prompt_addition`. All methods now reference the single module-level `logger`.
Verify the change is correct and complete; do not refactor unrelated code.

fix confidence: 🟢 90 high — react 👍/👎 to teach the reviewer

logger = logging.getLogger(__name__)

paths = [os.path.abspath(self.repo_path)]
if self.additional_source_paths:
paths.extend([os.path.abspath(p) for p in self.additional_source_paths])
Expand All @@ -221,9 +331,6 @@ def validate_source_paths(self) -> None:
ValueError: If any path does not exist or is not a directory
OSError: If any path cannot be accessed
"""
import logging
logger = logging.getLogger(__name__)

logger.info("🔍 Validating source paths...")

# Validate primary repo_path
Expand Down Expand Up @@ -259,9 +366,6 @@ def is_multi_path_mode(self) -> bool:
Returns:
True if additional_source_paths is set and non-empty, False otherwise
"""
import logging
logger = logging.getLogger(__name__)

result = bool(self.additional_source_paths)
if result:
logger.debug(f"🔍 Multi-path mode: ENABLED ({len(self.additional_source_paths)} additional paths)")
Expand All @@ -271,9 +375,6 @@ def is_multi_path_mode(self) -> bool:

def get_prompt_addition(self) -> str:
"""Generate prompt additions based on agent instructions."""
import logging
logger = logging.getLogger(__name__)

if not self.agent_instructions:
logger.debug("📋 get_prompt_addition: No agent_instructions configured")
return ""
Expand Down Expand Up @@ -760,3 +861,4 @@ def from_config_manager(
diagrams_dir=None,
additional_source_paths=additional_paths
)