Skip to content

fix(CODEWIKI-007): CU-86akn96pk 2 review findings in job.py #97

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
+122 −65
Draft

fix(CODEWIKI-007): CU-86akn96pk 2 review findings in job.py #97

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
187 changes: 122 additions & 65 deletions codewiki/cli/models/job.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,29 @@ class GenerationOptions:
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:
Comment on lines 26 to 54

@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.

🦩 🔴 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

Expand All @@ -35,13 +58,101 @@ class JobStatistics:
max_depth: int = 0
total_tokens_used: int = 0

def to_dict(self) -> Dict[str, Any]:
"""Convert to dictionary for JSON serialization."""
return {
"total_files_analyzed": self.total_files_analyzed,
"leaf_nodes": self.leaf_nodes,
"max_depth": self.max_depth,
"total_tokens_used": self.total_tokens_used,
}

@classmethod
def from_dict(cls, data: Any) -> 'JobStatistics':
"""Create from dictionary."""
if isinstance(data, cls):
return data
if not data:
return cls()
return cls(
total_files_analyzed=_coerce_int(data.get('total_files_analyzed'), 0),
leaf_nodes=_coerce_int(data.get('leaf_nodes'), 0),
max_depth=_coerce_int(data.get('max_depth'), 0),
total_tokens_used=_coerce_int(data.get('total_tokens_used'), 0),
)


@dataclass
class LLMRoleConfig:
"""Configuration for a single LLM role (cluster, main, or fallback)."""
model: str = ""
api_key: str = ""
base_url: str = ""
api_version: str = ""
max_tokens: int = 0
temperature: float = 0.0
temperature_supported: bool = True
max_token_field: str = "max_tokens"

def to_dict(self) -> Dict[str, Any]:
"""Convert to dictionary for JSON serialization."""
return {
"model": self.model,
"api_key": self.api_key,
"base_url": self.base_url,
"api_version": self.api_version,
"max_tokens": self.max_tokens,
"temperature": self.temperature,
"temperature_supported": self.temperature_supported,
"max_token_field": self.max_token_field,
}

@classmethod
def from_dict(cls, data: Any) -> 'LLMRoleConfig':
"""Create from dictionary."""
if isinstance(data, cls):
return data
if not data:
return cls()
return cls(
model=data.get('model', ''),
api_key=data.get('api_key', ''),
base_url=data.get('base_url', ''),
api_version=data.get('api_version', ''),
max_tokens=_coerce_int(data.get('max_tokens'), 0),
temperature=float(data.get('temperature', 0.0) or 0.0),
temperature_supported=bool(data.get('temperature_supported', True)),
max_token_field=data.get('max_token_field', 'max_tokens'),
)


@dataclass

@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.

🦩 🔴 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

class LLMConfig:
"""LLM configuration for a job."""
main_model: str
cluster_model: str
base_url: str
"""LLM configuration for a job, mirroring the three-role (cluster, main, fallback) pattern."""
cluster: LLMRoleConfig = field(default_factory=LLMRoleConfig)
main: LLMRoleConfig = field(default_factory=LLMRoleConfig)
fallback: LLMRoleConfig = field(default_factory=LLMRoleConfig)

def to_dict(self) -> Dict[str, Any]:
"""Convert to dictionary for JSON serialization."""
return {
"cluster": self.cluster.to_dict(),
"main": self.main.to_dict(),
"fallback": self.fallback.to_dict(),
}

@classmethod
def from_dict(cls, data: Any) -> Optional['LLMConfig']:
"""Create from dictionary, or None if data is empty."""
if isinstance(data, cls):
return data
if not data:
return None
return cls(
cluster=LLMRoleConfig.from_dict(data.get('cluster')),
main=LLMRoleConfig.from_dict(data.get('main')),
fallback=LLMRoleConfig.from_dict(data.get('fallback')),
)


def _coerce_job_status(value: Any, default: JobStatus = JobStatus.PENDING) -> JobStatus:
Expand All @@ -66,47 +177,6 @@ def _coerce_int(value: Any, default: int = 0) -> int:
return default


def _coerce_generation_options(value: Any) -> GenerationOptions:
"""Coerce a raw dict into a GenerationOptions instance."""
if isinstance(value, GenerationOptions):
return value
if not value:
return GenerationOptions()
return GenerationOptions(
create_branch=bool(value.get('create_branch', False)),
github_pages=bool(value.get('github_pages', False)),
no_cache=bool(value.get('no_cache', False)),
custom_output=value.get('custom_output'),
)


def _coerce_llm_config(value: Any) -> Optional[LLMConfig]:
"""Coerce a raw dict into an LLMConfig instance, or None."""
if isinstance(value, LLMConfig):
return value
if not value:
return None
return LLMConfig(
main_model=value.get('main_model', ''),
cluster_model=value.get('cluster_model', ''),
base_url=value.get('base_url', ''),
)


def _coerce_statistics(value: Any) -> JobStatistics:
"""Coerce a raw dict into a JobStatistics instance."""
if isinstance(value, JobStatistics):
return value
if not value:
return JobStatistics()
return JobStatistics(
total_files_analyzed=_coerce_int(value.get('total_files_analyzed'), 0),
leaf_nodes=_coerce_int(value.get('leaf_nodes'), 0),
max_depth=_coerce_int(value.get('max_depth'), 0),
total_tokens_used=_coerce_int(value.get('total_tokens_used'), 0),
)


@dataclass
class DocumentationJob:
"""
Expand Down Expand Up @@ -176,23 +246,9 @@ def to_dict(self) -> Dict[str, Any]:
"error_message": self.error_message,
"files_generated": self.files_generated,
"module_count": self.module_count,
"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,
},
"generation_options": self.generation_options.to_dict(),
"llm_config": self.llm_config.to_dict() if self.llm_config else None,
"statistics": self.statistics.to_dict(),
}
return data

Expand Down Expand Up @@ -220,13 +276,14 @@ def from_dict(cls, data: Dict[str, Any]) -> 'DocumentationJob':

# Parse nested objects
if 'generation_options' in data:
job.generation_options = _coerce_generation_options(data['generation_options'])
job.generation_options = GenerationOptions.from_dict(data['generation_options'])

if 'llm_config' in data and data['llm_config']:
job.llm_config = _coerce_llm_config(data['llm_config'])
job.llm_config = LLMConfig.from_dict(data['llm_config'])

if 'statistics' in data:
job.statistics = _coerce_statistics(data['statistics'])
job.statistics = JobStatistics.from_dict(data['statistics'])

return job