fix(CODEWIKI-007): CU-86akn96pk 2 review findings in job.py - #97
flamingo[bot] wants to merge 1 commit into
Conversation
![flamingo[bot]](https://avatars.githubusercontent.com/in/3426709?s=60&v=4){kind=small}
| ) | ||
|
|
||
|
|
||
| @dataclass |
There was a problem hiding this comment.
🦩 🔴 LLMConfig dataclass in cli/models/job.py collapses the three-role LLM pattern into a single main/cluster pair
Replaced the flat LLMConfig (main_model/cluster_model/base_url) in codewiki/cli/models/job.py with a three-role structure: new LLMRoleConfig dataclass (model, api_key, base_url, api_version, max_tokens, temperature, temperature_supported, max_token_field) and LLMConfig now holding cluster, main, and fallback fields of type LLMRoleConfig. This mirrors the three-role pattern described for codewiki/src/config.py. Risk: I could not see the actual Config dataclass in codewiki/src/config.py, so exact field names/types on LLMRoleConfig are a best-effort reconstruction from the finding text; any consumer of the old llm_config.main_model/llm_config.cluster_model attributes elsewhere in the codebase will break and needs updating (out of scope for this single-file fix).
🤖 Prompt for AI agents
In codewiki/cli/models/job.py around line 39, review and complete this code-review fix: LLMConfig dataclass in cli/models/job.py collapses the three-role LLM pattern into a single main/cluster pair.
What the draft fix changed: Replaced the flat `LLMConfig` (main_model/cluster_model/base_url) in codewiki/cli/models/job.py with a three-role structure: new `LLMRoleConfig` dataclass (model, api_key, base_url, api_version, max_tokens, temperature, temperature_supported, max_token_field) and `LLMConfig` now holding `cluster`, `main`, and `fallback` fields of type `LLMRoleConfig`. This mirrors the three-role pattern described for codewiki/src/config.py. Risk: I could not see the actual `Config` dataclass in codewiki/src/config.py, so exact field names/types on `LLMRoleConfig` are a best-effort reconstruction from the finding text; any consumer of the old `llm_config.main_model`/`llm_config.cluster_model` attributes elsewhere in the codebase will break and needs updating (out of scope for this single-file fix).
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| no_cache: bool = False | ||
| custom_output: Optional[str] = None | ||
|
|
||
| def to_dict(self) -> Dict[str, Any]: | ||
| """Convert to dictionary for JSON serialization.""" | ||
| return { | ||
| "create_branch": self.create_branch, | ||
| "github_pages": self.github_pages, | ||
| "no_cache": self.no_cache, | ||
| "custom_output": self.custom_output, | ||
| } | ||
|
|
||
| @classmethod | ||
| def from_dict(cls, data: Any) -> 'GenerationOptions': | ||
| """Create from dictionary.""" | ||
| if isinstance(data, cls): | ||
| return data | ||
| if not data: | ||
| return cls() | ||
| return cls( | ||
| create_branch=bool(data.get('create_branch', False)), | ||
| github_pages=bool(data.get('github_pages', False)), | ||
| no_cache=bool(data.get('no_cache', False)), | ||
| custom_output=data.get('custom_output'), | ||
| ) | ||
|
|
||
|
|
||
| @dataclass | ||
| class JobStatistics: |
There was a problem hiding this comment.
🦩 🔴 GenerationOptions, JobStatistics, and LLMConfig dataclasses lack their own to_dict/from_dict methods
Added to_dict/from_dict methods directly on GenerationOptions, JobStatistics, LLMRoleConfig, and LLMConfig in codewiki/cli/models/job.py. Removed the free-function coercers _coerce_generation_options, _coerce_llm_config, _coerce_statistics and rewired DocumentationJob.to_dict()/from_dict() to call the new classmethods/instance methods instead, eliminating the inline ad-hoc dict construction and centralizing conversion logic on each dataclass. Kept _coerce_job_status and _coerce_int as shared small helpers since they are primitive coercions, not dataclass (de)serialization.
🤖 Prompt for AI agents
In codewiki/cli/models/job.py around line 21, review and complete this code-review fix: GenerationOptions, JobStatistics, and LLMConfig dataclasses lack their own to_dict/from_dict methods.
What the draft fix changed: Added `to_dict`/`from_dict` methods directly on `GenerationOptions`, `JobStatistics`, `LLMRoleConfig`, and `LLMConfig` in codewiki/cli/models/job.py. Removed the free-function coercers `_coerce_generation_options`, `_coerce_llm_config`, `_coerce_statistics` and rewired `DocumentationJob.to_dict()`/`from_dict()` to call the new classmethods/instance methods instead, eliminating the inline ad-hoc dict construction and centralizing conversion logic on each dataclass. Kept `_coerce_job_status` and `_coerce_int` as shared small helpers since they are primitive coercions, not dataclass (de)serialization.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 85 medium — react 👍/👎 to teach the reviewer
flamingo
Bot
changed the title
Closes 2 review findings in
codewiki/cli/models/job.py.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:39codewiki/cli/models/job.py:21What 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:
9aa9712c-4ca2-4571-94bc-95e5cff4b3cdMerging 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-86akn96pk CodeWiki review findings sweep (9 PRs)