Skip to content

fix(CODEWIKI-002): 2 review findings in job.py #38

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): 2 review findings in job.py #38

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
88 changes: 81 additions & 7 deletions codewiki/cli/models/job.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,40 @@ class LLMConfig:
base_url: str


def _coerce_int(value: Any, default: int = 0) -> int:
"""Coerce a value to int, falling back to default on failure."""
if value is None:
return default
try:
return int(value)
except (TypeError, ValueError):
return default


def _coerce_bool(value: Any, default: bool = False) -> bool:
"""Coerce a value to bool, falling back to default on failure."""
if value is None:
return default
if isinstance(value, bool):
return value
if isinstance(value, str):
return value.strip().lower() in ("1", "true", "yes", "on")
try:
return bool(value)
except (TypeError, ValueError):
return default


def _coerce_str(value: Any, default: Optional[str] = None) -> Optional[str]:
"""Coerce a value to str, falling back to default on failure."""
if value is None:
return default
try:
return str(value)
except (TypeError, ValueError):
return default


@dataclass
class DocumentationJob:
"""
Expand Down Expand Up @@ -100,6 +134,30 @@ def fail(self, error_message: str):

@flamingo flamingo Bot Aug 24, 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.

🦩 🔴 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. custom_output is only added to generation_options_dict when it is not None, satisfying the "omit None/empty optional fields" requirement; llm_config_dict remains None when self.llm_config is falsy.

🤖 Prompt for AI agents 
In codewiki/cli/models/job.py around line 100, 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. `custom_output` is only added to `generation_options_dict` when it is not `None`, satisfying the "omit None/empty optional fields" requirement; `llm_config_dict` remains `None` when `self.llm_config` is falsy.
Verify the change is correct and complete; do not refactor unrelated code.

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

def to_dict(self) -> Dict[str, Any]:
"""Convert to dictionary for JSON serialization."""
generation_options_dict = {
"create_branch": self.generation_options.create_branch,
"github_pages": self.generation_options.github_pages,
"no_cache": self.generation_options.no_cache,
}
if self.generation_options.custom_output is not None:
generation_options_dict["custom_output"] = self.generation_options.custom_output

if self.llm_config:
llm_config_dict = {
"main_model": self.llm_config.main_model,
"cluster_model": self.llm_config.cluster_model,
"base_url": self.llm_config.base_url,
}
else:
llm_config_dict = None

statistics_dict = {
"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,
}

data = {
"job_id": self.job_id,
"repository_path": self.repository_path,
Expand All @@ -113,9 +171,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": asdict(self.generation_options),
"llm_config": asdict(self.llm_config) if self.llm_config else None,
"statistics": asdict(self.statistics),
"generation_options": generation_options_dict,
"llm_config": llm_config_dict,
"statistics": statistics_dict,
}
return data

Expand All @@ -138,19 +196,35 @@ def from_dict(cls, data: Dict[str, Any]) -> 'DocumentationJob':
status=JobStatus(data.get('status', '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 = GenerationOptions(
create_branch=_coerce_bool(opts.get('create_branch', False)),
github_pages=_coerce_bool(opts.get('github_pages', False)),
no_cache=_coerce_bool(opts.get('no_cache', False)),
custom_output=_coerce_str(opts.get('custom_output')),
)

if 'llm_config' in data and data['llm_config']:
job.llm_config = LLMConfig(**data['llm_config'])
llm_cfg = data['llm_config']
job.llm_config = LLMConfig(
main_model=_coerce_str(llm_cfg.get('main_model'), ''),
cluster_model=_coerce_str(llm_cfg.get('cluster_model'), ''),
base_url=_coerce_str(llm_cfg.get('base_url'), ''),
)

if 'statistics' in data:
job.statistics = JobStatistics(**data['statistics'])
stats = data['statistics']
job.statistics = JobStatistics(
total_files_analyzed=_coerce_int(stats.get('total_files_analyzed', 0)),
leaf_nodes=_coerce_int(stats.get('leaf_nodes', 0)),
max_depth=_coerce_int(stats.get('max_depth', 0)),
total_tokens_used=_coerce_int(stats.get('total_tokens_used', 0)),
)

return job

Comment on lines 196 to 230

@flamingo flamingo Bot Aug 24, 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.

🦩 🔴 DocumentationJob.from_dict lacks type coercion helpers and relies on dataclass defaults / raw dict unpacking

Added module-level coercion helpers _coerce_int, _coerce_bool, and _coerce_str, and updated DocumentationJob.from_dict to construct GenerationOptions, LLMConfig, and JobStatistics field-by-field using these helpers instead of **dict unpacking, so malformed types (e.g. a string count) are normalized rather than passed through raw. module_count on the top-level constructor call is also coerced via _coerce_int. Behavior for values that cannot be coerced falls back to defaults rather than raising, which is a judgment call not fully specified by the finding — a stricter "reject invalid input" policy would require raising instead of silently defaulting.

🤖 Prompt for AI agents 
In codewiki/cli/models/job.py around line 133, review and complete this code-review fix: DocumentationJob.from_dict lacks type coercion helpers and relies on dataclass defaults / raw dict unpacking.
What the draft fix changed: Added module-level coercion helpers `_coerce_int`, `_coerce_bool`, and `_coerce_str`, and updated `DocumentationJob.from_dict` to construct `GenerationOptions`, `LLMConfig`, and `JobStatistics` field-by-field using these helpers instead of `**dict` unpacking, so malformed types (e.g. a string count) are normalized rather than passed through raw. `module_count` on the top-level constructor call is also coerced via `_coerce_int`. Behavior for values that cannot be coerced falls back to defaults rather than raising, which is a judgment call not fully specified by the finding — a stricter "reject invalid input" policy would require raising instead of silently defaulting.
Verify the change is correct and complete; do not refactor unrelated code.

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