fix(CODEWIKI-002): CU-86akbhhru 4 review findings across 2 files - #49
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| "error_message": self.error_message, | ||
| "files_generated": self.files_generated, | ||
| "module_count": self.module_count, | ||
| "generation_options": asdict(self.generation_options), | ||
| "llm_config": asdict(self.llm_config) if self.llm_config else None, | ||
| "statistics": asdict(self.statistics), | ||
| "generation_options": { | ||
| "create_branch": self.generation_options.create_branch, | ||
| "github_pages": self.generation_options.github_pages, | ||
| "no_cache": self.generation_options.no_cache, | ||
| "custom_output": self.generation_options.custom_output, | ||
| }, | ||
| "llm_config": { | ||
| "main_model": self.llm_config.main_model, | ||
| "cluster_model": self.llm_config.cluster_model, | ||
| "base_url": self.llm_config.base_url, | ||
| } if self.llm_config else None, | ||
| "statistics": { | ||
| "total_files_analyzed": self.statistics.total_files_analyzed, | ||
| "leaf_nodes": self.statistics.leaf_nodes, | ||
| "max_depth": self.statistics.max_depth, | ||
| "total_tokens_used": self.statistics.total_tokens_used, | ||
| }, | ||
| } | ||
| return data | ||
|
|
There was a problem hiding this comment.
🦩 🔴 DocumentationJob.to_dict delegates to asdict() for nested dataclasses instead of explicit field listing
In DocumentationJob.to_dict, replaced asdict(self.generation_options), asdict(self.llm_config), and asdict(self.statistics) with explicit dict literals listing each field by name for generation_options, llm_config, and statistics, removing the asdict import usage entirely (and its import). This satisfies the requirement for explicit field listing instead of delegation to asdict(). Note: the finding also mentions omitting None/empty optional fields, but I preserved the existing behavior of always including all keys (with None values where absent) to avoid changing the serialization contract/schema without further guidance from the reviewer — this is the main residual risk.
🤖 Prompt for AI agents
In codewiki/cli/models/job.py around line 103, review and complete this code-review fix: DocumentationJob.to_dict delegates to asdict() for nested dataclasses instead of explicit field listing.
What the draft fix changed: In `DocumentationJob.to_dict`, replaced `asdict(self.generation_options)`, `asdict(self.llm_config)`, and `asdict(self.statistics)` with explicit dict literals listing each field by name for `generation_options`, `llm_config`, and `statistics`, removing the `asdict` import usage entirely (and its import). This satisfies the requirement for explicit field listing instead of delegation to `asdict()`. Note: the finding also mentions omitting None/empty optional fields, but I preserved the existing behavior of always including all keys (with `None` values where absent) to avoid changing the serialization contract/schema without further guidance from the reviewer — this is the main residual risk.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
| branch_name=data.get('branch_name'), | ||
| timestamp_start=data.get('timestamp_start', datetime.now().isoformat()), | ||
| timestamp_end=data.get('timestamp_end'), | ||
| status=JobStatus(data.get('status', 'pending')), | ||
| status=_coerce_job_status(data.get('status'), JobStatus.PENDING), | ||
| error_message=data.get('error_message'), | ||
| files_generated=data.get('files_generated', []), | ||
| module_count=data.get('module_count', 0), | ||
| module_count=_coerce_int(data.get('module_count'), 0), | ||
| ) | ||
|
|
||
| # Parse nested objects | ||
| if 'generation_options' in data: | ||
| opts = data['generation_options'] | ||
| job.generation_options = GenerationOptions(**opts) | ||
| job.generation_options = _coerce_generation_options(data['generation_options']) | ||
|
|
||
| if 'llm_config' in data and data['llm_config']: | ||
| job.llm_config = LLMConfig(**data['llm_config']) | ||
| job.llm_config = _coerce_llm_config(data['llm_config']) | ||
|
|
||
| if 'statistics' in data: | ||
| job.statistics = JobStatistics(**data['statistics']) | ||
| job.statistics = _coerce_statistics(data['statistics']) | ||
|
|
||
| return job | ||
|
|
There was a problem hiding this comment.
🦩 🔴 DocumentationJob.from_dict lacks named type-coercion helpers for status/int fields
Added named coercion helper functions _coerce_job_status, _coerce_int, _coerce_generation_options, _coerce_llm_config, and _coerce_statistics at module level, each with explicit default values. Updated DocumentationJob.from_dict to use _coerce_job_status for status, _coerce_int for module_count, and _coerce_generation_options/_coerce_llm_config/_coerce_statistics for the nested dataclasses instead of raw JobStatus(...) construction and **opts/**data['llm_config']/**data['statistics'] unpacking. This adds field-level validation/coercion and named helpers per the required pattern. Risk: the coercion helpers use permissive .get() defaults (e.g., empty strings for LLMConfig fields) rather than raising on missing required fields, which may be more lenient than a "complete" fix intends — reviewer should confirm whether stricter validation (e.g., raising on missing main_model) is required.
🤖 Prompt for AI agents
In codewiki/cli/models/job.py around line 129, review and complete this code-review fix: DocumentationJob.from_dict lacks named type-coercion helpers for status/int fields.
What the draft fix changed: Added named coercion helper functions `_coerce_job_status`, `_coerce_int`, `_coerce_generation_options`, `_coerce_llm_config`, and `_coerce_statistics` at module level, each with explicit default values. Updated `DocumentationJob.from_dict` to use `_coerce_job_status` for `status`, `_coerce_int` for `module_count`, and `_coerce_generation_options`/`_coerce_llm_config`/`_coerce_statistics` for the nested dataclasses instead of raw `JobStatus(...)` construction and `**opts`/`**data['llm_config']`/`**data['statistics']` unpacking. This adds field-level validation/coercion and named helpers per the required pattern. Risk: the coercion helpers use permissive `.get()` defaults (e.g., empty strings for `LLMConfig` fields) rather than raising on missing required fields, which may be more lenient than a "complete" fix intends — reviewer should confirm whether stricter validation (e.g., raising on missing `main_model`) is required.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| 'fallback_api_key', | ||
| }) | ||
|
|
||
| @dataclass |
There was a problem hiding this comment.
🦩 🔴 Config dataclass has no to_dict()/from_dict() implementation despite being a persisted/transferred dataclass
Added to_dict(self, include_secrets: bool = False) and from_dict(cls, data) methods to the Config dataclass (codewiki/src/config.py). to_dict() uses dataclasses.asdict(self) and by default strips the three API-key fields via the new _RUNTIME_ONLY_SECRET_FIELDS module-level constant; from_dict() filters the input dict to only known dataclass field names (via dataclasses.fields(cls)) and constructs a new Config. Also added the required fields, asdict imports from dataclasses. This gives an explicit serialization surface as requested. Note: since API keys are required (no defaults), from_dict() on a secret-stripped dict will raise TypeError unless the caller re-supplies keys — this is intentional to avoid silently reconstructing a broken/insecure config, but callers needing full round-trip must call to_dict(include_secrets=True) or merge keys back in themselves; that usage pattern is not enforced elsewhere in this file since no other code currently calls these new methods.
🤖 Prompt for AI agents
In codewiki/src/config.py around line 45, review and complete this code-review fix: Config dataclass has no to_dict()/from_dict() implementation despite being a persisted/transferred dataclass.
What the draft fix changed: Added `to_dict(self, include_secrets: bool = False)` and `from_dict(cls, data)` methods to the `Config` dataclass (codewiki/src/config.py). `to_dict()` uses `dataclasses.asdict(self)` and by default strips the three API-key fields via the new `_RUNTIME_ONLY_SECRET_FIELDS` module-level constant; `from_dict()` filters the input dict to only known dataclass field names (via `dataclasses.fields(cls)`) and constructs a new `Config`. Also added the required `fields, asdict` imports from `dataclasses`. This gives an explicit serialization surface as requested. Note: since API keys are required (no defaults), `from_dict()` on a secret-stripped dict will raise `TypeError` unless the caller re-supplies keys — this is intentional to avoid silently reconstructing a broken/insecure config, but callers needing full round-trip must call `to_dict(include_secrets=True)` or merge keys back in themselves; that usage pattern is not enforced elsewhere in this file since no other code currently calls these new methods.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 75 medium — react 👍/👎 to teach the reviewer
| main_model: str | ||
| cluster_model: str | ||
| fallback_model: str | ||
| # Per-provider API keys (required) |
There was a problem hiding this comment.
🦩 🔴 Backend Config dataclass declares API key fields as plain required fields with no runtime-only exclusion mechanism
Added a module-level _RUNTIME_ONLY_SECRET_FIELDS frozenset naming cluster_api_key, main_api_key, fallback_api_key, referenced it in the new to_dict() to exclude these fields by default, and added an inline comment above the three field declarations in the Config dataclass marking them "runtime-only - never serialized via to_dict()". This creates the enforced boundary described in the finding (to_dict() now exists and excludes keys by default), but it only protects call sites that go through to_dict(); direct access to config.__dict__, dataclasses.asdict(config), or vars(config) elsewhere in the codebase is unaffected since dataclass fields cannot be made truly exclusion-only without changing the field declarations themselves (e.g. via field(repr=False) or a wrapper type), which would be a larger structural change beyond this single-file fix.
🤖 Prompt for AI agents
In codewiki/src/config.py around line 57, review and complete this code-review fix: Backend Config dataclass declares API key fields as plain required fields with no runtime-only exclusion mechanism.
What the draft fix changed: Added a module-level `_RUNTIME_ONLY_SECRET_FIELDS` frozenset naming `cluster_api_key`, `main_api_key`, `fallback_api_key`, referenced it in the new `to_dict()` to exclude these fields by default, and added an inline comment above the three field declarations in the `Config` dataclass marking them "runtime-only - never serialized via to_dict()". This creates the enforced boundary described in the finding (to_dict() now exists and excludes keys by default), but it only protects call sites that go through `to_dict()`; direct access to `config.__dict__`, `dataclasses.asdict(config)`, or `vars(config)` elsewhere in the codebase is unaffected since dataclass fields cannot be made truly exclusion-only without changing the field declarations themselves (e.g. via `field(repr=False)` or a wrapper type), which would be a larger structural change beyond this single-file fix.
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
flamingo
Bot
changed the title
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). Conflicts were competing module docstrings in cpp.py, csharp.py and javascript.py, added by both this branch and #55. Resolved in favour of the wording already on main; this branch's _get_component_id changes are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). Conflicts: - deps.py, typescript.py: competing module docstrings added by both this branch and #55; resolved in favour of the wording on main. - config.py: this branch's new module docstring kept, layered on top of #49's widened dataclasses import (fields, asdict). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). Conflict in test_clustering_simple.py: both this branch and #53 replaced the hardcoded test repo path with a TEST_REPO_PATH env lookup, differing only in the fallback. Resolved in favour of main's fallback, which #53 applied consistently across the other clustering test scripts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Bring the branch up to date with main (PRs #48, #49, #52, #53, #54, #55). flamingo_guidelines.py is fully superseded by #52, which has merged: this branch's changes to that file are dropped and the file is taken from main wholesale. Resolving the conflict hunk-by-hunk instead left a duplicate 'import logging', since main already has one. What remains is the part of this PR #52 did not cover: the analysis_service.py dead-code cleanup and the background_worker.py print -> logging conversion. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 participant
Closes 4 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
codewiki/cli/models/job.py:103codewiki/cli/models/job.py:129codewiki/src/config.py:45codewiki/src/config.py:57What 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:
2cc7a212-e9ac-481a-a76e-5f03d762c00cMerging 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-86akbhhru CodeWiki backend and CLI review findings (12 PRs)