Skip to content

fix(CODEWIKI-002): CU-86akn96pk 2 review findings in config.py #96

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
Draft

fix(CODEWIKI-002): CU-86akn96pk 2 review findings in config.py #96

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
61 changes: 55 additions & 6 deletions codewiki/src/config.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,11 +128,48 @@ def to_dict(self, include_secrets: bool = False) -> Dict[str, Any]:
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).

Each field is listed explicitly (rather than delegating to
dataclasses.asdict()) so that adding a new field to Config requires a
deliberate decision about whether/how it is serialized here.
"""
data = asdict(self)

@flamingo flamingo Bot Sep 23, 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

Rewrote Config.to_dict() (codewiki/src/config.py) to build the returned dict by explicitly listing every non-secret field by name (repo_path, output_dir, ... additional_source_paths), instead of calling asdict(self) and popping secret keys afterward. Secret fields (cluster_api_key, main_api_key, fallback_api_key) are now only added to the dict explicitly when include_secrets=True, rather than being present by default and removed. This satisfies CODEWIKI-002's requirement that to_dict() explicitly enumerate fields, and it removes the "new field silently serialized" risk since any new dataclass field must be added to this list by hand to be included.

🤖 Prompt for AI agents 
In codewiki/src/config.py around line 132, review and complete this code-review fix: Config.to_dict() delegates to asdict() instead of explicitly listing fields.
What the draft fix changed: Rewrote `Config.to_dict()` (codewiki/src/config.py) to build the returned dict by explicitly listing every non-secret field by name (repo_path, output_dir, ... additional_source_paths), instead of calling `asdict(self)` and popping secret keys afterward. Secret fields (cluster_api_key, main_api_key, fallback_api_key) are now only added to the dict explicitly when `include_secrets=True`, rather than being present by default and removed. This satisfies CODEWIKI-002's requirement that to_dict() explicitly enumerate fields, and it removes the "new field silently serialized" risk since any new dataclass field must be added to this list by hand to be included.
Verify the change is correct and complete; do not refactor unrelated code.

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

if not include_secrets:
for secret_field in _RUNTIME_ONLY_SECRET_FIELDS:
data.pop(secret_field, None)
data = {
'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,
}
if include_secrets:
data['cluster_api_key'] = self.cluster_api_key
data['main_api_key'] = self.main_api_key
data['fallback_api_key'] = self.fallback_api_key
return data

@classmethod
Expand All @@ -143,10 +180,21 @@ def from_dict(cls, data: Dict[str, Any]) -> 'Config':
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.

Unknown/unexpected keys in `data` (e.g. leftover fields from a previous
schema version) are rejected with a ValueError rather than silently
discarded, so that config drift between the CLI and backend Config is
surfaced instead of hidden.
"""
known_fields = {f.name for f in fields(cls)}

@flamingo flamingo Bot Sep 23, 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 silently drops unknown/extra fields including ones from a previous schema version

Changed Config.from_dict() (codewiki/src/config.py) to compute unknown = set(data.keys()) - known_fields and raise ValueError listing the unknown keys instead of silently filtering them out with a dict comprehension. This directly addresses the "silently drops unknown/extra fields" finding by turning schema drift into a visible error. Risk/incompleteness: this is a behavior change — any existing caller that relied on from_dict() tolerating extra/renamed keys (e.g. passing through a superset dict from a newer version) will now raise instead of degrading gracefully; a complete fix might instead want a warning-log path or an explicit migration/versioning mechanism, which is beyond a minimal in-file change and was not implemented here.

🤖 Prompt for AI agents 
In codewiki/src/config.py around line 147, review and complete this code-review fix: Config.from_dict silently drops unknown/extra fields including ones from a previous schema version.
What the draft fix changed: Changed `Config.from_dict()` (codewiki/src/config.py) to compute `unknown = set(data.keys()) - known_fields` and raise `ValueError` listing the unknown keys instead of silently filtering them out with a dict comprehension. This directly addresses the "silently drops unknown/extra fields" finding by turning schema drift into a visible error. Risk/incompleteness: this is a behavior change — any existing caller that relied on from_dict() tolerating extra/renamed keys (e.g. passing through a superset dict from a newer version) will now raise instead of degrading gracefully; a complete fix might instead want a warning-log path or an explicit migration/versioning mechanism, which is beyond a minimal in-file change and was not implemented here.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.

fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer

filtered = {k: v for k, v in data.items() if k in known_fields}
return cls(**filtered)
unknown = set(data.keys()) - known_fields
if unknown:
raise ValueError(
f"Unknown Config field(s) in data passed to from_dict(): {sorted(unknown)}. "
"This usually indicates config drift between schema versions; "
"remove or migrate these fields before constructing a Config."
)
return cls(**data)

@property
def include_patterns(self) -> Optional[List[str]]:
Expand Down Expand Up @@ -760,3 +808,4 @@ def from_config_manager(
diagrams_dir=None,
additional_source_paths=additional_paths
)