fix(CODEWIKI-003): CU-86akhf8u6 4 review findings in config.py - #74
flamingo[bot] wants to merge 1 commit into
Conversation
| # 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]: |
There was a problem hiding this comment.
🦩 🔴 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
| # 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]: |
There was a problem hiding this comment.
🦩 🔴 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
| 'additional_source_paths': self.additional_source_paths, | ||
| } | ||
|
|
||
| @classmethod |
There was a problem hiding this comment.
🦩 🔴 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
| List of absolute paths. Always includes repo_path as first element. | ||
| If additional_source_paths is None, returns single-element list. | ||
| """ | ||
| import logging |
There was a problem hiding this comment.
🦩 🟠 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
Closes 4 review findings in
codewiki/src/config.py.Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
codewiki/src/config.py:121codewiki/src/config.py:121codewiki/src/config.py:138codewiki/src/config.py:202What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
4b0306d9-ca7f-413c-857e-fc323d1e9f21Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akhf8u6 CodeWiki review findings sweep (9 PRs)