Skip to content

fix/feat: P0 bug fixes, type annotations, docstrings, and more tests - #7

Merged
MisterBrookT merged 2 commits into
mainfrom
claude/go-execution-ZgMFl
Jun 4, 2026
Merged

fix/feat: P0 bug fixes, type annotations, docstrings, and more tests#7
MisterBrookT merged 2 commits into
mainfrom
claude/go-execution-ZgMFl

Conversation

@MisterBrookT

Copy link
Copy Markdown
Owner

Summary

  • P0 — 致命 bug 修复

    • prompts/gen_text2image.py: 修复错误的 nanochart 导入 → igenbench
    • eval_engine.py: call_image_understanding 返回值可能是 str,增加 isinstance(dict) 判断,避免 AttributeError
    • vis_item.py: generation 字段类型由错误的 Optional[dict] 改为 Dict[str, Any]
    • utils/io.py: 函数名拼写错误 senmanticsemantic,并添加 separator 不存在时的 ValueError
  • P1 — 元数据与类型

    • pyproject.toml 补全 authorskeywordsclassifiers[project.urls]
    • 所有 CLI 命令函数添加 -> None 返回类型;GenEngine.text2image 添加 -> PILImage
    • caller_registry.py / llm_caller.py / client.py 补全 docstring 和返回类型注解
    • CI 添加 pytest --cov 和 coverage artifact 上传
  • 新增测试(41 个,较之前 17 个增加 141%)

    • test_io.py:9 个测试覆盖 extract_from_markdownsplit_semantic_and_data_in_t2i_prompt
    • test_eval_engine.py:6 个测试覆盖 EvalEngine(mock LLMClient,无需 API key)
    • test_caller_registry.py:4 个测试覆盖 @register_caller 装饰器
    • test_cli_score.py:5 个测试覆盖 igenbench score CLI 命令
  • P2 — 文档

    • AGENTS.md:补全废弃 QuestionEvalEntry 的字段迁移说明

Test plan

  • uv run pytest tests/ -v — 41/41 passed 本地验证
  • uv run ruff check . — All checks passed
  • uv run ruff format --check . — All files formatted
  • CI 三个 Python 版本全绿

Generated by Claude Code

…overage

P0 fixes:
- prompts/gen_text2image.py: fix nanochart -> igenbench import
- eval_engine.py: guard response with isinstance(dict) before .get()
- vis_item.py: correct generation field type Optional[dict] -> Dict[str,Any]
- utils/io.py: rename senmantic->semantic, add boundary check with ValueError

P1 improvements:
- pyproject.toml: add authors, keywords, classifiers, project URLs
- pyproject.toml: add pytest-cov to dev deps
- CLI functions: add -> None return type annotations
- gen_engine.py: add PILImage return type to text2image()
- caller_registry.py: add docstring + correct return type annotation
- llm_caller.py: add docstrings to all three LLMCaller abstract methods
- client.py: add docstrings to call_text/image_generation/understanding
- ci.yml: add --cov flag and upload coverage artifact

New tests (41 total, up from 17):
- tests/test_io.py: 9 tests for extract_from_markdown + split_semantic
- tests/test_eval_engine.py: 6 tests for EvalEngine (mocked LLMClient)
- tests/test_caller_registry.py: 4 tests for @register_caller decorator
- tests/test_cli_score.py: 5 tests for igenbench score command

P2:
- AGENTS.md: expand deprecated Question -> EvalEntry migration guide

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request adds comprehensive type annotations, docstrings, metadata, and unit tests across the repository, alongside minor bug fixes such as correcting an import path and handling non-dict LLM responses. Reviewer feedback highlights several opportunities to improve robustness through defensive programming, specifically by handling potential None or empty values in LLM responses and prompt inputs to prevent runtime errors like AttributeError and TypeError.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines 29 to 30
text_response = self._caller.generate_text(model, prompt, **kwargs)
return extract_from_markdown(text_response)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

If the LLM provider returns an empty response or None (for example, due to safety filters or generation limits), passing it directly to extract_from_markdown will raise an AttributeError when calling .strip(). Adding a check to return an empty string when text_response is falsy prevents runtime crashes.

        text_response = self._caller.generate_text(model, prompt, **kwargs)
        if not text_response:
            return ""
        return extract_from_markdown(text_response)

Comment on lines 40 to 43
text_response = self._caller.understand_image(
model, prompt, image_path, **kwargs
)
return extract_from_markdown(text_response)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

Similar to call_text_generation, if understand_image returns None or an empty response, passing it directly to extract_from_markdown will cause an AttributeError. Adding a defensive check here ensures the engine handles empty or blocked responses gracefully.

Suggested change
text_response = self._caller.understand_image(
model, prompt, image_path, **kwargs
)
return extract_from_markdown(text_response)
text_response = self._caller.understand_image(
model, prompt, image_path, **kwargs
)
if not text_response:
return ""
return extract_from_markdown(text_response)

Comment on lines +52 to +57
if isinstance(response, dict):
analysis = response.get("analysis", "")
answer = response.get("answer", "")
else:
analysis = str(response)
answer = ""

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If response is None (e.g., if the API call failed or returned an empty response), calling str(response) will set analysis to the literal string "None". It is better to default to an empty string "" to avoid saving "None" as the analysis result.

Suggested change
if isinstance(response, dict):
analysis = response.get("analysis", "")
answer = response.get("answer", "")
else:
analysis = str(response)
answer = ""
if isinstance(response, dict):
analysis = response.get("analysis", "")
answer = response.get("answer", "")
else:
analysis = str(response) if response is not None else ""
answer = ""

Comment thread igenbench/utils/io.py
Comment on lines +112 to +113
separator = "The given data is:"
if separator not in t2i_prompt:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If t2i_prompt is None or empty, checking separator not in t2i_prompt will raise a TypeError. Adding a defensive check at the beginning of the function ensures we raise a clear, descriptive ValueError instead of a generic runtime error.

    if not t2i_prompt:
        raise ValueError("Prompt cannot be empty or None.")
    separator = "The given data is:"
    if separator not in t2i_prompt:

…onses

- client.py: return '' early when provider returns None/empty in
  call_text_generation and call_image_understanding
- io.py: raise ValueError early when prompt is None or empty in
  split_semantic_and_data_in_t2i_prompt
- eval_engine.py: avoid literal 'None' string by checking response is
  not None before str() conversion
- test_io.py: add tests for empty string and None prompt inputs
@MisterBrookT
MisterBrookT marked this pull request as ready for review June 4, 2026 15:52
@MisterBrookT
MisterBrookT merged commit 2bd1c7c into main Jun 4, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants