Open
Conversation
- Introduced new patch operations: draw_grid_border, set_bold, set_fill_color, set_dimensions, and restore_design_snapshot. - Enhanced test coverage for the new operations, including validation checks for input parameters. - Updated server and tool models to accept and process the new design operations.
…tings - Introduced `merge_cells`, `unmerge_cells`, and `set_alignment` operations in the MCP. - Updated documentation to include new operations and their descriptions. - Enhanced `PatchOp` model to support new operations and validation. - Implemented validation for merging cells to ensure multi-cell ranges and prevent overlaps. - Added tests for merging, unmerging, and setting alignment, including inverse restoration scenarios. - Updated version to 0.4.5 to reflect new features.
- Added support for backend selection in patch requests, allowing users to specify "auto", "com", or "openpyxl". - Updated documentation to reflect new backend options and their behaviors. - Implemented validation to ensure compatibility of backend options with various operations (e.g., dry_run, return_inverse_ops). - Enhanced error handling for unsupported operations based on selected backend. - Introduced tests to verify backend selection logic and its impact on patch operations.
- Introduced `set_font_color` operation to allow users to set font colors in Excel sheets. - Updated documentation to include `set_font_color` and its usage examples. - Enhanced path policy to resolve relative output paths from the root directory. - Normalized hex color inputs for both `set_font_color` and `set_fill_color` operations. - Implemented validation to ensure `set_font_color` does not accept fill color and vice versa. - Added tests for new functionality, including validation and normalization of color inputs.
…ality - Implemented `apply_table_style` operation in the patch runner, allowing users to create Excel tables with specified styles. - Enhanced `PatchOp` model to include `style` and `table_name` fields for the new operation. - Added validation for `apply_table_style` to ensure correct usage and prevent conflicts. - Introduced artifact mirroring feature that saves output files to a specified directory when `mirror_artifact` is enabled. - Updated server configuration to accept an optional `--artifact-bridge-dir` argument for mirroring artifacts. - Added tests for the new operation and artifact mirroring, ensuring correct behavior and error handling.
- Introduced the `auto_fit_columns` operation to automatically adjust column widths based on content, with optional minimum and maximum width constraints. - Updated documentation to include usage examples and detailed descriptions of the new operation. - Enhanced path policy error messages to recommend using `exstruct_get_runtime_info` for paths outside the defined root. - Implemented validation for `min_width` and `max_width` parameters to ensure they are positive and that `min_width` does not exceed `max_width`. - Added tests to verify the functionality of the new operation, including edge cases and validation checks. - Updated existing tests to ensure compatibility with the new operation and its constraints.
…e column width estimation in auto_fit_columns
…alization functions - Moved top-level sheet resolution and missing sheet message building to the new `normalize` module. - Introduced `coerce_patch_ops` to normalize patch operations and handle aliases. - Added new `PatchOpSpec` and `PATCH_OP_SPECS` for better operation specification management. - Created protocols for `OpenpyxlPatchEngine` and `XlwingsPatchEngine` to standardize engine implementations. - Implemented `apply_openpyxl_engine` and `apply_xlwings_engine` for applying patch operations. - Added shared utilities for A1 notation parsing and output path resolution. - Enhanced error handling and validation for patch operations. - Added unit tests for normalization functions, service layer, and shared utilities.
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Contributor
📝 WalkthroughWalkthroughAdds a v0.5.0 MCP feature set: new make/patch tooling, operation-schema introspection, runtime diagnostics, artifact mirroring, and a refactored patch subsystem (models, normalization, engines, service). Updates docs, schemas, CI, shared utilities, server tooling, and extensive tests to support these features. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Server
participant PatchService
participant Engine
participant Filesystem
participant ArtifactBridge
Client->>Server: exstruct_patch / exstruct_make (ops, sheet, backend, mirror_artifact)
Server->>PatchService: normalize payload, resolve top-level sheet
PatchService->>Filesystem: resolve input/output path
PatchService->>PatchService: select_patch_engine (com/openpyxl/auto)
PatchService->>Engine: apply(request, input_path, output_path)
Engine-->>PatchService: diff, inverse_ops, formula_issues, warnings, engine_name
PatchService->>Filesystem: write output_path
alt mirror_artifact true
PatchService->>ArtifactBridge: copy output to bridge dir
ArtifactBridge-->>PatchService: mirrored_out_path
end
PatchService-->>Server: PatchResult (out_path, patch_diff, inverse_ops, formula_issues, warnings, engine, mirrored_out_path?)
Server-->>Client: Tool output (PatchToolOutput / MakeToolOutput)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
This pull request implements comprehensive design editing features for the MCP server (Issue #61), including table borders, bold text, background colors, and row/column width adjustments. It also includes a major architecture refactoring of the patch system and adds several new MCP tools.
Changes:
- Added
exstruct_maketool for creating new workbooks with initial operations - Expanded
exstruct_patchwith 12+ design editing operations (borders, fonts, colors, dimensions, alignment, merging, table styles) - Added MCP schema discovery tools (
exstruct_list_ops,exstruct_describe_op) and runtime diagnostics (exstruct_get_runtime_info) - Refactored patch architecture into layered modules for better maintainability while preserving backward compatibility
- Added artifact mirroring support and backend selection controls
Reviewed changes
Copilot reviewed 62 out of 65 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Version bump to 0.5.0, added coverage threshold enforcement |
| uv.lock | Dependency updates (exstruct 0.5.0, virtualenv 20.39.0) |
| src/exstruct/mcp/tools.py | Added new tool models and handlers (make, list_ops, describe_op, runtime_info, artifact mirroring) |
| src/exstruct/mcp/server.py | Integrated new tools, added backend selection, alias normalization, and top-level sheet resolution |
| src/exstruct/mcp/io.py | Enhanced PathPolicy to resolve relative paths from root with better error messages |
| src/exstruct/mcp/shared/* | New shared utilities for A1 notation parsing and output path handling |
| src/exstruct/mcp/patch/* | Major refactoring into service/engine/ops/runtime layers with specs and normalization |
| src/exstruct/mcp/op_schema.py | New operation schema metadata system for discovery tools |
| tests/mcp/* | Comprehensive test suite additions covering all new features and edge cases |
| docs/* | Updated release notes, MCP documentation, architecture docs, and READMEs |
| schemas/* | Added merged_ranges field to workbook/sheet schemas |
| codecov.yml | Set coverage targets to 80% for project and patch |
| .github/workflows/pytest.yml | Added --cov-fail-under=80 to CI |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4181e7ee9b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
tests/core/test_mode_output.py (1)
84-99:⚠️ Potential issue | 🟡 MinorAdd Google‑style docstrings to the two marked tests.
Both functions are missing docstrings, which violates the project guideline for Python files.
📎 Suggested docstrings
@@ def test_standardモードはテキストなし図形を除外する(tmp_path: Path) -> None: + """Verify standard mode excludes shapes without text, except arrows.""" _ensure_excel() @@ def test_verboseモードでは全図形と幅高さが出力される(tmp_path: Path) -> None: + """Verify verbose mode outputs all shapes with width/height populated.""" _ensure_excel()As per coding guidelines, "Add Google-style docstrings to all functions and classes".
Also applies to: 102-113
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/test_mode_output.py` around lines 84 - 99, The two test functions (e.g., test_standardモードはテキストなし図形を除外する and the test at lines 102–113) are missing Google-style docstrings; add a concise Google-style docstring to each test describing what the test verifies (e.g., inputs: tmp_path, actions: call _make_shapes_book and extract(path, mode="standard"), expected behavior: textless shapes are only Arrow instances with direction or begin_arrow_style set), placed immediately under the def line; ensure the docstrings follow the project's Google-style format (short summary, blank line, and any brief Args/Returns if desired) and reference relevant symbols like extract, _make_shapes_book, and Arrow in the description.tests/mcp/test_tools_handlers.py (1)
170-322:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to the new/updated tests.
The newly added tests lack docstrings. Please add brief Google-style docstrings for each test function.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_tools_handlers.py` around lines 170 - 322, Add a short Google-style docstring under each test function in this diff (e.g., test_run_patch_tool_builds_request, test_run_patch_tool_mirrors_artifact_when_enabled, test_run_make_tool_warns_when_bridge_is_not_configured, test_run_patch_tool_warns_when_mirror_copy_fails, test_run_make_tool_builds_request, test_run_list_ops_tool_returns_known_ops, test_run_describe_op_tool_returns_schema_details, test_run_describe_op_tool_rejects_unknown_op) describing the test purpose; place the docstring as the first statement in the function, use triple quotes, include a one-line summary and optional short Args/Returns section if relevant to adhere to Google-style, and ensure formatting/indentation matches existing tests.tests/mcp/test_server.py (5)
1161-1189:⚠️ Potential issue | 🟡 MinorAdd docstrings to the nested run-server stubs.
The nested
_Appandfake_create_apphelpers should carry Google-style docstrings.As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
class _App: + """Minimal app stub for run_server tests.""" + def run(self) -> None: created["ran"] = True @@ def fake_create_app( policy: PathPolicy, *, on_conflict: OnConflictPolicy, artifact_bridge_dir: Path | None = None, ) -> _App: + """Create a stub app for run_server. + + Args: + policy: Path policy for run_server. + on_conflict: Conflict policy. + artifact_bridge_dir: Optional artifact bridge path. + + Returns: + Stub app instance. + """ created["policy"] = policy🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_server.py` around lines 1161 - 1189, The nested test helpers _App and fake_create_app lack Google-style docstrings; add concise docstrings to class _App (one-line summary and optional brief description of its run method behavior) and to function fake_create_app (one-line summary, Args for policy/on_conflict/artifact_bridge_dir, and Returns describing the _App instance) so they follow the project's docstring convention and clarify their purpose in test_run_server_sets_env.
41-57:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings for the new test helpers.
DummyAppand_call_asyncare missing docstrings; add Google-style docstrings (summary + Args/Returns) to satisfy the project standard.As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
class DummyApp: + """Test double for MCP tool registration.""" + def __init__(self) -> None: self.tools: dict[str, ToolFunc] = {} @@ async def _call_async( func: Callable[..., Awaitable[object]], kwargs: dict[str, object], ) -> object: + """Invoke an async tool function with keyword arguments. + + Args: + func: Async tool function. + kwargs: Keyword arguments for the call. + + Returns: + Tool call result. + """ return await func(**kwargs)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_server.py` around lines 41 - 57, Add Google-style docstrings for the new test helpers: document the DummyApp class (including its purpose), its __init__ method (Args/Attributes) and the tool decorator method (Args/Returns) using the Google style, and add a docstring to the _call_async function summarizing its behavior and listing Args and Returns. Ensure each docstring has a one-line summary followed by an Args section for parameters (e.g., name for DummyApp.tool, func/kwargs for _call_async) and a Returns section describing the returned value/type.
60-99:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings for the CLI arg parsing tests.
test_parse_args_defaultsandtest_parse_args_with_optionsneed Google-style docstrings.As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
def test_parse_args_defaults(tmp_path: Path) -> None: + """Validate default CLI argument parsing. + + Args: + tmp_path: Temporary directory fixture. + """ config = server._parse_args(["--root", str(tmp_path)])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_server.py` around lines 60 - 99, Add Google-style docstrings to the two test functions test_parse_args_defaults and test_parse_args_with_options: for each function include a short summary line describing what the test verifies, an "Args" section describing the tmp_path fixture parameter, and a "Returns" section (None) per the Google docstring format; place the docstrings immediately under each def statement so they document the test behavior (parsing defaults and parsing with provided options) and expected outcomes.
549-967:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to the JSON-string ops tests.
The JSON-string ops tests (patch/make/fallback/warnings) are missing docstrings.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
def test_register_tools_accepts_patch_ops_json_strings( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: + """Ensure patch tool accepts JSON-string ops payloads. + + Args: + monkeypatch: Pytest monkeypatch fixture. + tmp_path: Temporary directory fixture. + """ app = DummyApp()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_server.py` around lines 549 - 967, Several test functions (e.g., test_register_tools_accepts_patch_ops_json_strings, test_register_tools_applies_top_level_sheet_fallback, test_register_tools_accepts_make_ops_json_strings, test_register_tools_accepts_merge_and_alignment_json_strings, test_register_tools_accepts_set_font_size_json_string, test_register_tools_returns_patch_warnings) are missing Google-style docstrings; add a concise Google-style docstring to each test that describes the test purpose, lists parameters (monkeypatch: pytest.MonkeyPatch, tmp_path: Path) under Args, and notes the return type (None) under Returns so they follow the project's docstring guideline and remain adjacent to the existing test bodies that exercise server._register_tools and the related fake_run_* helpers.
246-353:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to the register-tools tests and helpers.
The expanded register-tools test (and its nested helpers) should include Google-style docstrings.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
def test_register_tools_uses_default_on_conflict( monkeypatch: pytest.MonkeyPatch, tmp_path: Path ) -> None: + """Ensure tool registration applies the default conflict policy. + + Args: + monkeypatch: Pytest monkeypatch fixture. + tmp_path: Temporary directory fixture. + """ app = DummyApp()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_server.py` around lines 246 - 353, The test and its nested helper functions are missing Google-style docstrings; add concise Google-style docstrings to test_register_tools_uses_default_on_conflict and each helper function (fake_run_extract_tool, fake_run_read_json_chunk_tool, fake_run_validate_input_tool, fake_run_patch_tool, fake_run_make_tool, and fake_run_sync) describing purpose, args, returns, and any side effects; follow the project's docstring template and include parameter types/meanings and return values for the ExtractToolInput/Output, ReadJsonChunkToolInput/Output, ValidateInputToolInput/Output, PatchToolInput/Output, MakeToolInput/Output, and the async fake_run_sync behavior so linters and docs are satisfied.src/exstruct/mcp/extract_runner.py (1)
255-267: 🛠️ Refactor suggestion | 🟠 MajorReturn a Pydantic model from
_apply_conflict_policyand update the shared utility.This function returns
tuple[Path, str | None, bool], which violates the coding guideline requiring all structured data to be represented as Pydantic models. Create aConflictPolicyResultmodel with fields for the resolved path, warning message, and skipped flag, then updateapply_conflict_policyinsrc/exstruct/mcp/shared/output_path.pyand the wrapper functions inextract_runner.py,patch/runtime.py, andpatch/internal.py. Update the call sites atextract_runner.py:97andpatch/service.py:91to access model fields instead of tuple unpacking.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/extract_runner.py` around lines 255 - 267, The helper currently returns a raw tuple from _shared_apply_conflict_policy and _apply_conflict_policy; create a Pydantic model ConflictPolicyResult with fields resolved_path: Path, warning: str | None, skipped: bool, update the shared function apply_conflict_policy/_shared_apply_conflict_policy to return ConflictPolicyResult instead of tuple, adjust the wrapper _apply_conflict_policy to return that model, and update all callers (the wrapper functions in extract_runner.py, patch/runtime.py, patch/internal.py and the sites that unpack the tuple in extract_runner (call at line where _apply_conflict_policy is used) and patch/service (call at line where result was unpacked)) to access result.resolved_path, result.warning, and result.skipped instead of tuple unpacking.src/exstruct/mcp/server.py (1)
219-243:⚠️ Potential issue | 🟡 MinorDocstring for
_create_appis stale after signature change.The docstring only documents the
policyargument, but the function now also acceptson_conflictandartifact_bridge_dirkeyword arguments. Update the Args section accordingly.📝 Proposed docstring update
"""Create the MCP FastMCP application. Args: policy: Path policy for filesystem access. + on_conflict: Output conflict policy. + artifact_bridge_dir: Optional bridge directory for mirrored artifacts. Returns: FastMCP application instance. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/server.py` around lines 219 - 243, The _create_app docstring is stale: update the Args section to document all parameters (policy: PathPolicy, on_conflict: OnConflictPolicy, artifact_bridge_dir: Path | None) and their purpose/semantics; mention that on_conflict controls default_on_conflict passed to _register_tools and artifact_bridge_dir is forwarded to _register_tools as the optional artifact bridge directory, and keep the Returns and overall description unchanged.
🟡 Minor comments (25)
src/exstruct/mcp/io.py-49-53 (1)
49-53:⚠️ Potential issue | 🟡 MinorAdd a Google-style docstring for
_resolve_from_root.The new helper lacks
Args/Returns, which violates the docstring guideline. As per coding guidelines, "Add Google-style docstrings to all functions and classes".📝 Proposed docstring update
def _resolve_from_root(self, path: Path, root: Path) -> Path: - """Resolve path using root as the base for relative inputs.""" + """Resolve path using root as the base for relative inputs. + + Args: + path: Candidate path to resolve. + root: Resolved root path used as the base for relative inputs. + + Returns: + Resolved candidate path. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/io.py` around lines 49 - 53, The function _resolve_from_root is missing a Google-style docstring; add a triple-quoted Google-style docstring above _resolve_from_root that includes a short summary and an Args section documenting path (Path): the input path which may be absolute or relative, and root (Path): the base used when path is relative, plus a Returns section describing that the function returns a resolved Path object; keep wording concise and consistent with other docstrings in the module.schemas/sheet.json-754-761 (1)
754-761:⚠️ Potential issue | 🟡 MinorDocument the new
merged_rangesfield.Since
merged_rangesis now part of the public SheetData schema, please update docs/agents/DATA_MODEL.md (Section 9) and the changelog entry to keep documentation aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/sheet.json` around lines 754 - 761, Add documentation for the new merged_ranges field on the public SheetData schema: update docs/agents/DATA_MODEL.md (Section 9) to describe merged_ranges (type: array of strings, A1 notation examples like "A1:C3", semantics and when it's populated, and its relation to alpha_col-oriented output) and add a changelog entry noting that merged_ranges was added to the SheetData schema and any migration/compatibility notes; reference the schema field name merged_ranges and the SheetData type so readers can locate the change.docs/agents/FEATURE_SPEC.md-165-185 (1)
165-185:⚠️ Potential issue | 🟡 MinorDeduplicate repeated headings to satisfy MD024.
The second phase repeats headings like "Feature Name/背景/目的", which triggers markdownlint's duplicate-heading rule. Consider suffixing the phase name or using lower-level headings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/agents/FEATURE_SPEC.md` around lines 165 - 185, The markdown repeats top-level headings ("Feature Name", "背景", "目的") across phases causing MD024; update the second-phase headings in this file by making them unique (e.g., append a phase suffix like "— Phase 2" or change them to lower-level headings such as "## Feature Name — Phase 2" or "### 背景 (Phase 2)"), ensuring each heading string is distinct so markdownlint MD024 no longer triggers and the structure remains clear.docs/agents/FEATURE_SPEC.md-89-92 (1)
89-92:⚠️ Potential issue | 🟡 MinorFix inconsistent list indentation (MD005).
markdownlint reports inconsistent indentation at Line 92; align list items to the same indentation level to clear the warning.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/agents/FEATURE_SPEC.md` around lines 89 - 92, Fix the inconsistent Markdown list indentation (MD005) around the `shared/a1.py` and `shared/output_path.py` entries: make the nested numbered sub-items under `shared/a1.py` and `shared/output_path.py` use the same indentation level as each other (e.g., align both subitem lines to the same number of leading spaces or tabs) so the list is consistent and markdownlint MD005 is resolved; update the two lines currently indented differently (the lines beginning "1. A1 解析、列変換、範囲展開の共通関数" and "1. `on_conflict`、`rename`、出力先決定の共通関数") to match the chosen indentation style.src/exstruct/mcp/patch/models.py-58-124 (1)
58-124:⚠️ Potential issue | 🟡 MinorConvert model docstrings to Google-style.
Docstrings on the snapshot models here are brief and not Google-style; please add Args/Attributes sections where applicable. As per coding guidelines Add Google-style docstrings to all functions and classes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/models.py` around lines 58 - 124, Update the brief docstrings on the snapshot Pydantic models (BorderSideSnapshot, BorderSnapshot, FontSnapshot, FillSnapshot, AlignmentSnapshot, MergeStateSnapshot, RowDimensionSnapshot, ColumnDimensionSnapshot, DesignSnapshot) to Google-style class docstrings: include a short summary line and an "Attributes:" (or "Args:" where constructor args apply) section listing each field name, its type and a one-line description; keep existing semantics and types, use consistent phrasing for nullable fields (e.g., "Optional[str]") and default descriptions, and ensure each class-level docstring follows the project's Google-style pattern.src/exstruct/mcp/patch_runner.py-31-47 (1)
31-47:⚠️ Potential issue | 🟡 MinorUse Google-style docstrings for the wrappers.
Docstrings for
_sync_legacy_overrides,run_make, andrun_patcharen't in Google format (Args/Returns). Please update for guideline compliance. As per coding guidelines Add Google-style docstrings to all functions and classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch_runner.py` around lines 31 - 47, Update the docstrings for _sync_legacy_overrides, run_make, and run_patch to Google-style: for _sync_legacy_overrides add a one-line summary and an Args: (none) or omit Args and a Returns: None with a short "Propagates supported monkeypatch overrides to internal module." description; for run_make and run_patch add a short summary indicating they are compatibility wrappers, an Args: section documenting request (MakeRequest or PatchRequest) and policy (PathPolicy | None, optional), and a Returns: section documenting PatchResult; keep descriptions concise and factual and place them directly above each function (_sync_legacy_overrides, run_make, run_patch).src/exstruct/mcp/patch/models.py-10-10 (1)
10-10:⚠️ Potential issue | 🟡 MinorRemove unused
get_com_availabilityimport.Line 10 is unused (F401) and will fail Ruff. Consider removing it.
As per coding guidelines Ensure generated code passes Ruff checks (E, W, F, I, B, UP, N, C90) with zero errors.🧹 Suggested fix
-from exstruct.cli.availability import get_com_availability as get_com_availability🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/models.py` at line 10, The file imports get_com_availability which is unused and triggers Ruff F401; remove the unused import statement "get_com_availability" from the import line in models.py (or delete the entire import if nothing else is imported from exstruct.cli.availability) so that the module no longer contains the unused symbol; ensure any remaining imports are alphabetized/organized per project import style.src/exstruct/mcp/patch/engine/openpyxl_engine.py-9-14 (1)
9-14:⚠️ Potential issue | 🟡 MinorUse a Google-style docstring for apply_openpyxl_engine.
Please expand the docstring to include
Args:andReturns:in Google format.As per coding guidelines, "Add Google-style docstrings to all functions and classes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/engine/openpyxl_engine.py` around lines 9 - 14, Update the apply_openpyxl_engine function docstring to a Google-style docstring: start with a one-line summary, then an Args: section listing request (PatchRequest) with its purpose, input_path (Path) and output_path (Path) with their roles, and a Returns: section describing the returned tuple of four elements (three lists of objects and one list of str) and what each list represents; keep the types consistent with the signature (tuple[list[object], list[object], list[object], list[str]]) and concise descriptions for each field.tests/mcp/test_path_policy.py-17-22 (1)
17-22:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to the updated tests.
These test functions were modified/added but still lack docstrings. Please add brief Google-style docstrings (even for tests) to stay consistent with repo standards.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".
Also applies to: 34-37
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_path_policy.py` around lines 17 - 22, Add a brief Google-style docstring to the test function test_path_policy_denies_outside_root describing its purpose (that it verifies PathPolicy(root=...) raises ValueError for paths outside the root) and mentioning expected behavior; update other modified/added tests (lines ~34-37) similarly. Locate the test functions (e.g., test_path_policy_denies_outside_root) and add a one- or two-sentence docstring immediately under the def line following Google style, referencing PathPolicy and its ensure_allowed method where relevant.tests/mcp/shared/test_a1.py-14-38 (1)
14-38:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to these new tests.
All new test functions are missing docstrings. Please add short Google-style docstrings to comply with repo standards.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/shared/test_a1.py` around lines 14 - 38, Each test function (test_column_roundtrip, test_split_a1, test_range_cell_count, test_parse_range_geometry, test_split_a1_rejects_invalid) is missing a Google-style docstring; add a one- to two-line docstring above each def summarizing the test purpose and expected behavior in Google style (short description line, optional blank line, and Args/Returns only if needed), e.g., describe what is being validated (e.g., "Verify conversion between column labels and indices."), keeping them concise and consistent with repo docstring conventions.src/exstruct/mcp/patch/ops/openpyxl_ops.py-4-18 (1)
4-18:⚠️ Potential issue | 🟡 MinorRemove the unnecessary cast and add a Google-style docstring.
_apply_ops_openpyxlalready acceptsPatchRequest, socast(Any, request)violates the guideline to useAnyonly at external library boundaries. Remove bothAnyandcastfrom the imports (sincecastis only used in this now-removed line). Also expand the docstring to Google style withArgsandReturnssections:def apply_openpyxl_ops( request: PatchRequest, input_path: Path, output_path: Path, ) -> tuple[list[object], list[object], list[object], list[str]]: """Apply patch operations using the openpyxl implementation. Args: request: The patch request containing operations to apply. input_path: Path to the input file. output_path: Path where the output file should be written. Returns: A tuple of (diff, inverse_ops, formula_issues, op_warnings) as lists. """ diff, inverse_ops, formula_issues, op_warnings = _internal._apply_ops_openpyxl( request, input_path, output_path, )Remove
Anyandcastfrom line 4 imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/ops/openpyxl_ops.py` around lines 4 - 18, Remove the unnecessary typing.cast usage and Any import: update apply_openpyxl_ops to pass request directly to _internal._apply_ops_openpyxl (call _apply_ops_openpyxl(request, input_path, output_path)), and delete Any and cast from the top-level imports; also replace the short docstring for apply_openpyxl_ops with a Google-style docstring including Args (request, input_path, output_path) and Returns describing the tuple (diff, inverse_ops, formula_issues, op_warnings).tests/mcp/shared/test_output_path.py-12-49 (1)
12-49:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to the test functions.
These test functions are missing docstrings required by the guidelines.
As per coding guidelines, "Add Google-style docstrings to all functions and classes."📝 Example pattern (apply to all tests in this module)
def test_normalize_output_name_same_stem() -> None: + """Verify default suffix handling with same-stem naming.""" input_path = Path("report.xlsx")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/shared/test_output_path.py` around lines 12 - 49, The test functions test_normalize_output_name_same_stem, test_normalize_output_name_patched, test_apply_conflict_policy_rename, and test_next_available_path_no_conflict lack Google-style docstrings; add a concise Google-style docstring to each function describing what the test verifies, any parameters (e.g., tmp_path for those that use it), and the return value (None) following the pattern: short summary, Args (if applicable), and Returns: None, so linters and documentation rules are satisfied.tests/mcp/patch/test_ops.py-12-60 (1)
12-60:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to test and helper functions.
These test functions (and the local fake implementations) are missing docstrings, which the guidelines require.
As per coding guidelines, "Add Google-style docstrings to all functions and classes."📝 Example pattern (apply to all functions in this file)
def test_apply_openpyxl_ops_delegates_to_legacy( monkeypatch: pytest.MonkeyPatch, ) -> None: + """Verify openpyxl ops delegate to the legacy implementation.""" import exstruct.mcp.patch.internal as legacy_runner @@ def _fake_apply_ops_openpyxl( request: PatchRequest, input_path: Path, output_path: Path, ) -> tuple[tuple[str, ...], tuple[str, ...], tuple[str, ...], tuple[str, ...]]: + """Fake legacy openpyxl implementation for delegation testing.""" return expected🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/patch/test_ops.py` around lines 12 - 60, Add Google-style docstrings to each test and to the local fake helper functions: test_apply_openpyxl_ops_delegates_to_legacy, _fake_apply_ops_openpyxl, test_apply_xlwings_ops_delegates_to_legacy, and _fake_apply_ops_xlwings; for each docstring include a one-line summary, an Args section documenting parameters (e.g., monkeypatch, request, input_path, output_path, ops, auto_formula) and a Returns section describing the return value (expected tuple/list of strings) so the tests comply with the project's documentation guidelines.tests/mcp/patch/test_normalize.py-11-77 (1)
11-77:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to the test functions.
These test functions lack docstrings required by the guidelines.
As per coding guidelines, "Add Google-style docstrings to all functions and classes."📝 Example pattern (apply to all tests in this module)
def test_coerce_patch_ops_normalizes_aliases() -> None: + """Verify alias normalization for patch operations.""" result = coerce_patch_ops(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/patch/test_normalize.py` around lines 11 - 77, Add Google-style docstrings to each test function: test_coerce_patch_ops_normalizes_aliases, test_coerce_patch_ops_normalizes_draw_grid_border_range, test_resolve_top_level_sheet_for_payload, and test_resolve_top_level_sheet_for_payload_rejects_missing_sheet. For each, add a short one-line summary describing what the test verifies (e.g., normalization of aliases, draw_grid_border range parsing, top-level sheet resolution), and where appropriate include a "Raises" section for the last test indicating it expects a ValueError; keep the docstrings concise and formatted in Google style.src/exstruct/mcp/patch/specs.py-10-15 (1)
10-15:⚠️ Potential issue | 🟡 MinorUpgrade
PatchOpSpecto a Google-style docstring.The current class docstring isn’t in Google style.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
class PatchOpSpec(BaseModel): - """Specification metadata used by patch-op normalization.""" + """Specification metadata used by patch-op normalization. + + Attributes: + op: Operation name. + aliases: Mapping of alias keys to canonical fields. + """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/specs.py` around lines 10 - 15, The PatchOpSpec class currently uses a short one-line docstring; replace it with a Google-style class docstring for PatchOpSpec that includes a brief summary and sections for Attributes documenting the op (type: PatchOpType) and aliases (dict[str, str], default empty dict) and any purpose/usage notes; update the docstring directly above the class definition so tools and linters recognize the Google-style format.tests/mcp/patch/test_models_internal_coverage.py-135-179 (1)
135-179:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to backend-constraint tests.
test_backend_com_rejects_dry_run_and_restore_design_snapshotis missing a Google-style docstring.As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
def test_backend_com_rejects_dry_run_and_restore_design_snapshot( op_factory: PatchOpFactory, request_factory: Callable[..., object], make_factory: Callable[..., object], ) -> None: + """Verify COM backend rejects unsupported features. + + Args: + op_factory: PatchOp constructor under test. + request_factory: PatchRequest constructor. + make_factory: MakeRequest constructor. + """ with pytest.raises(ValidationError, match="backend='com' does not support"):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/patch/test_models_internal_coverage.py` around lines 135 - 179, Add a Google-style docstring to the test function test_backend_com_rejects_dry_run_and_restore_design_snapshot: insert a short one-line summary describing the test behavior (that backend="com" should reject dry_run and restore_design_snapshot ops), and include an Args section listing the fixtures/parameters (op_factory, request_factory, make_factory) and what they represent; keep it concise and follow Google docstring formatting.tests/mcp/test_server.py-355-427 (1)
355-427:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to the runtime/op-schema tests.
The new runtime info and op schema tests need Google-style docstrings.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
def test_register_tools_returns_runtime_info(tmp_path: Path) -> None: + """Validate runtime info tool output fields. + + Args: + tmp_path: Temporary directory fixture. + """ app = DummyApp()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_server.py` around lines 355 - 427, Add a short Google-style docstring to each test function (test_register_tools_returns_runtime_info, test_register_tools_returns_ops_schema_tools, test_register_tools_describe_op_rejects_unknown_op, test_patch_tool_doc_includes_op_mini_schema) describing what the test verifies, the tmp_path fixture param (tmp_path: Path), expected behavior, and that it returns None; keep the docstrings brief (one to three lines) and follow Google style (Args:, Returns: if needed).tests/mcp/patch/test_models_internal_coverage.py-112-125 (1)
112-125:⚠️ Potential issue | 🟡 MinorAdd a docstring and narrow the payload typing.
payloadusesAny, and the test lacks a Google-style docstring. Usedict[str, object](or a narrow union) and add a docstring.As per coding guidelines, "Add Google-style docstrings to all functions and classes" and "Use `Any` type only at external library boundaries (xlwings, pandas, etc.), not in general code".✍️ Docstring + typing update
def test_patch_op_validation_errors( op_factory: PatchOpFactory, request_factory: Callable[..., object], make_factory: Callable[..., object], - payload: dict[str, Any], + payload: dict[str, object], message: str, ) -> None: + """Validate PatchOp validation errors for invalid payloads. + + Args: + op_factory: PatchOp constructor under test. + request_factory: PatchRequest constructor. + make_factory: MakeRequest constructor. + payload: Invalid patch payload. + message: Expected error message fragment. + """ with pytest.raises(ValidationError, match=message):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/patch/test_models_internal_coverage.py` around lines 112 - 125, The test function test_patch_op_validation_errors should be given a Google-style docstring and its payload parameter narrowed from dict[str, Any] to dict[str, object]; update the signature to payload: dict[str, object] and add a brief Google-style docstring describing the purpose, parameters (op_factory, request_factory, make_factory, payload, message) and expected behavior (raises ValidationError), keeping the existing fixture-referencing asserts intact so parametrization alignment is preserved.tests/mcp/test_server.py-143-244 (1)
143-244:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to the patch-op normalization tests.
The new alias/normalization tests in this block need Google-style docstrings.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
def test_coerce_patch_ops_normalizes_aliases() -> None: + """Normalize alias fields in patch op payloads.""" result = server._coerce_patch_ops(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_server.py` around lines 143 - 244, Add a Google-style docstring to each test function (test_coerce_patch_ops_normalizes_aliases, test_coerce_patch_ops_rejects_conflicting_aliases, test_coerce_patch_ops_rejects_conflicting_alignment_aliases, test_coerce_patch_ops_normalizes_draw_grid_border_range, test_coerce_patch_ops_rejects_mixed_draw_grid_border_inputs) as the first statement in the function: a one-line summary of what the test asserts followed by an "Args:" section (None) and "Returns:" section (None) per Google style, using triple-quoted strings immediately under each def.tests/mcp/test_server.py-1221-1268 (1)
1221-1268:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to the helper normalization tests.
The new helper-level tests at the end of the file need docstrings.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
def test_normalize_patch_op_aliases_helper_covers_all_alias_paths() -> None: + """Ensure alias normalization covers all alias paths.""" op = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_server.py` around lines 1221 - 1268, Add Google-style docstrings to each helper test function: test_normalize_patch_op_aliases_helper_covers_all_alias_paths, test_normalize_draw_grid_border_range_rejects_non_string, test_parse_a1_range_geometry_normalizes_reverse_range, test_parse_a1_range_geometry_rejects_invalid_shape, test_split_a1_cell_rejects_invalid_reference, and test_parse_patch_op_json_and_error_message_helpers. For each, insert a concise Google-style docstring at the top of the function describing the test purpose, noting there are no args and that it returns None (e.g., "Tests that X does Y." followed by "Args: None" and "Returns: None"). Ensure formatting matches existing project docstring style and keep content focused on what the test validates.tests/mcp/patch/test_legacy_runner_ops.py-14-161 (1)
14-161:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to helper/test functions.
The helper functions and tests in this file need Google-style docstrings.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
def _create_workbook(path: Path) -> None: + """Create a minimal workbook for legacy patch tests. + + Args: + path: Output workbook path. + """ workbook = Workbook()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/patch/test_legacy_runner_ops.py` around lines 14 - 161, The tests and helper functions lack Google-style docstrings; add concise Google-style docstrings to each top-level helper function and test (e.g., _create_workbook, _disable_com, test_run_patch_auto_fit_columns_openpyxl_uses_single_pass_collector, test_apply_xlwings_set_font_size, test_run_patch_error_includes_hint_for_known_set_fill_color_mistake) and to any small helper classes defined inside tests (e.g., _FakeFontApi, _FakeRangeApi, _FakeRange, _FakeSheet) describing purpose, Args (with types), Returns (if any) and Raises (if applicable); keep descriptions short and place the docstring immediately under the def/class line following Google style conventions.tests/mcp/patch/test_models_internal_coverage.py-181-328 (1)
181-328:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to the xlwings-helper tests and fakes.
The helper test and its nested fake classes should carry docstrings to align with the standard.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
def test_internal_xlwings_helpers_error_and_success_paths() -> None: + """Exercise xlwings helper error/success paths.""" class _FakeFont: + """Minimal fake xlwings font API.""" Bold: bool = False Size: float = 0.0 Color: int = 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/patch/test_models_internal_coverage.py` around lines 181 - 328, Add Google-style docstrings to the test functions and all nested fake classes so they meet the project's docstring guidelines: update test_internal_xlwings_helpers_error_and_success_paths and test_internal_auto_fit_column_resolution_defaults with a short description, descriptions of what each test asserts, and any important params/returns; add Google-style docstrings to the fake classes _FakeFont, _FakeInterior, _FakeRangeApi, _FakeRange, _FakeSheet, _FakeSheets, _FakeWorkbook, _SheetWithoutUsedRange, _LastCell, _UsedRange, and _SheetWithUsedRange that briefly explain their role in the tests and any key attributes or methods. Ensure the docstrings follow the Google format (one-line summary, optional extended description, and parameter/attribute sections where applicable) and keep them concise.tests/mcp/patch/test_service.py-13-220 (1)
13-220:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to helper/test functions.
The helper and test functions should include Google-style docstrings for consistency.
As per coding guidelines, "Add Google-style docstrings to all functions and classes".✍️ Docstring template
def _create_workbook(path: Path) -> None: + """Create a minimal workbook for patch service tests. + + Args: + path: Output workbook path. + """ workbook = Workbook()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/patch/test_service.py` around lines 13 - 220, The tests and helper lack Google-style docstrings; add concise Google-style docstrings to _create_workbook and each test function (e.g., test_patch_runner_run_patch_delegates_to_service, test_patch_runner_run_make_delegates_to_service, test_service_run_patch_backend_auto_prefers_com, test_service_run_patch_backend_auto_fallbacks_to_openpyxl_on_com_error, test_service_run_patch_backend_com_does_not_fallback_on_com_error, test_service_run_patch_backend_com_fallbacks_for_apply_table_style) describing purpose, args (if any) and return (None) where applicable; place the docstring immediately under each def and follow the Google style short summary, optional Args/Returns sections for clarity.src/exstruct/mcp/server.py-245-251 (1)
245-251:⚠️ Potential issue | 🟡 MinorDocstring for
_register_toolsis incomplete after signature change.The
default_on_conflictandartifact_bridge_dirparameters are not documented.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/server.py` around lines 245 - 251, The docstring for _register_tools is missing documentation for the new parameters; update the function's docstring (the _register_tools function) to document default_on_conflict (type OnConflictPolicy, describe what the policy does and its default behavior) and artifact_bridge_dir (type Path | None, explain what passing None means and how the directory is used), and ensure the signature types FastMCP and PathPolicy remain described; keep the existing description of the function's purpose and include brief examples or allowed values for OnConflictPolicy if applicable.docs/mcp.md-458-468 (1)
458-468:⚠️ Potential issue | 🟡 MinorMistake catalog contradicts the alias documentation.
The "Mistake catalog" section marks
colorforset_fill_colorandhorizontal/verticalforset_alignmentas "Wrong," but the "Alias and shorthand inputs" section (lines 411–427) and the op schema (with explicit alias definitions) document these exact fields as accepted aliases that are normalized to canonical form. This will confuse users who see the alias documentation and then see the same patterns flagged as mistakes.Either remove these entries from the mistake catalog or clarify that while aliases work, canonical field names are preferred for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/mcp.md` around lines 458 - 468, The Mistake catalog incorrectly flags alias fields as wrong for operations set_fill_color and set_alignment; update the Mistake catalog entries so they no longer contradict the "Alias and shorthand inputs" and the op schema: either remove the entries that mark `color`, `horizontal`, and `vertical` as wrong, or replace them with a short clarification note that these are accepted aliases (normalized to `fill_color`, `horizontal_align`, `vertical_align`) but canonical field names are preferred for clarity; ensure references to set_fill_color and set_alignment remain consistent with the alias documentation and op schema.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (64)
.github/pull_request_template.md.github/workflows/pytest.yml.gitignoreCHANGELOG.mdREADME.ja.mdREADME.mdcodecov.ymldocs/README.en.mddocs/README.ja.mddocs/agents/ARCHITECTURE.mddocs/agents/DATA_MODEL.mddocs/agents/FEATURE_SPEC.mddocs/agents/LEGACY_DEPENDENCY_INVENTORY.mddocs/agents/MODEL_MIGRATION_NOTES.mddocs/agents/ROADMAP.mddocs/agents/TASKS.mddocs/mcp.mddocs/release-notes/v0.5.0.mdmkdocs.ymlpyproject.tomlschemas/sheet.jsonschemas/workbook.jsonsrc/exstruct/mcp/__init__.pysrc/exstruct/mcp/extract_runner.pysrc/exstruct/mcp/io.pysrc/exstruct/mcp/op_schema.pysrc/exstruct/mcp/patch/__init__.pysrc/exstruct/mcp/patch/engine/__init__.pysrc/exstruct/mcp/patch/engine/base.pysrc/exstruct/mcp/patch/engine/openpyxl_engine.pysrc/exstruct/mcp/patch/engine/xlwings_engine.pysrc/exstruct/mcp/patch/internal.pysrc/exstruct/mcp/patch/models.pysrc/exstruct/mcp/patch/normalize.pysrc/exstruct/mcp/patch/ops/__init__.pysrc/exstruct/mcp/patch/ops/common.pysrc/exstruct/mcp/patch/ops/openpyxl_ops.pysrc/exstruct/mcp/patch/ops/xlwings_ops.pysrc/exstruct/mcp/patch/runtime.pysrc/exstruct/mcp/patch/service.pysrc/exstruct/mcp/patch/specs.pysrc/exstruct/mcp/patch/types.pysrc/exstruct/mcp/patch_runner.pysrc/exstruct/mcp/server.pysrc/exstruct/mcp/shared/__init__.pysrc/exstruct/mcp/shared/a1.pysrc/exstruct/mcp/shared/output_path.pysrc/exstruct/mcp/tools.pytests/core/test_mode_output.pytests/mcp/patch/test_legacy_runner_ops.pytests/mcp/patch/test_models_internal_coverage.pytests/mcp/patch/test_normalize.pytests/mcp/patch/test_ops.pytests/mcp/patch/test_service.pytests/mcp/shared/test_a1.pytests/mcp/shared/test_output_path.pytests/mcp/test_chunk_reader.pytests/mcp/test_make_runner.pytests/mcp/test_patch_runner.pytests/mcp/test_path_policy.pytests/mcp/test_server.pytests/mcp/test_sheet_reader.pytests/mcp/test_tool_models.pytests/mcp/test_tools_handlers.py
Owner
Author
|
PR #65 の未解決レビュー指摘について対応方針を反映しました。
この方針に基づき、該当スレッドは resolve します。 |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/exstruct/mcp/server.py (1)
214-227:⚠️ Potential issue | 🟡 MinorDocument the new parameters in
_create_app’s docstring.
on_conflictandartifact_bridge_dirare now part of the signature but aren’t documented in the Args section.
As per coding guidelines: Add Google-style docstrings to all functions and classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/server.py` around lines 214 - 227, Update the `_create_app` Google-style docstring to document the new parameters: add `on_conflict` (describe type OnConflictPolicy and what it controls, e.g., how artifact/name conflicts are handled) and `artifact_bridge_dir` (describe it as Path | None and what passing a path vs None does, e.g., directory used for artifact bridging or disabled when None); keep the existing `policy` entry and the `Returns` section unchanged and ensure parameter descriptions are concise and aligned with the existing Args formatting.tests/mcp/test_server.py (1)
42-1267:⚠️ Potential issue | 🟡 MinorAdd Google-style docstrings to DummyApp and the new test helpers/cases.
The added class and test/helper functions are missing docstrings, which violates the project’s docstring requirement.
As per coding guidelines: Add Google-style docstrings to all functions and classes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/test_server.py` around lines 42 - 1267, The DummyApp class and newly added test helpers (e.g., DummyApp, _call_async) and test functions in tests/mcp/test_server.py lack Google-style docstrings; add concise Google-style docstrings to the DummyApp class and its tool decorator, the _call_async helper, and any newly added test functions/helpers (mentioning purpose, Args, Returns where applicable) so they meet the project's docstring requirement and describe behavior and parameters for each symbol (DummyApp, DummyApp.tool, _call_async, and the new/modified test_* functions).
♻️ Duplicate comments (2)
src/exstruct/mcp/patch/models.py (2)
727-748:⚠️ Potential issue | 🟠 MajorValidate
set_range_valuesshape against the A1 range dimensions.Rectangularity is checked, but range size mismatch can still slip through and fail later. Please add a range-dimension check.
✅ Suggested validation update
def _validate_set_range_values(op: PatchOp) -> None: @@ expected_width = len(op.values[0]) if any(len(row) != expected_width for row in op.values): raise ValueError("set_range_values requires rectangular values.") + expected_rows, expected_cols = _range_dimensions(op.range) + if len(op.values) != expected_rows or expected_width != expected_cols: + raise ValueError( + "set_range_values requires values shape matching the range dimensions." + ) @@ def _range_cell_count(range_ref: str | None) -> int: @@ return _shared_range_cell_count(range_ref) + + +def _range_dimensions(range_ref: str) -> tuple[int, int]: + """Return (row_count, col_count) for an A1 range.""" + start, end = range_ref.split(":", maxsplit=1) + start_col, start_row = _split_a1(start) + end_col, end_row = _split_a1(end) + start_col_idx = _column_label_to_index(start_col) + end_col_idx = _column_label_to_index(end_col) + return ( + abs(end_row - start_row) + 1, + abs(end_col_idx - start_col_idx) + 1, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/models.py` around lines 727 - 748, The current _validate_set_range_values validates rectangularity of op.values but doesn't ensure the values' dimensions match the A1 range in op.range; update _validate_set_range_values to compute the range's height and width from op.range and compare them to len(op.values) and len(op.values[0]) respectively, and raise a ValueError (e.g. "set_range_values requires values matching range dimensions") when they differ; keep existing checks (non-empty, rectangular) and reference op.range and op.values in the validation and error message.
133-379:⚠️ Potential issue | 🟠 MajorAvoid exposing openpyxl/xlwings Protocols; normalize at the boundary.
These Protocols hard-code external library internals and leak them across the API surface. The guideline calls for accepting external objects as
Anyat boundaries and normalizing into Pydantic models immediately. Consider replacing these Protocols with adapter/normalizer functions that produce the existing snapshot BaseModels (or new ones) and update entry points to acceptAny.
Based on learnings: Do not depend on internal structures of external libraries (xlwings, pandas, numpy); accept entities as Any at boundaries and normalize them to Pydantic models internally.#!/bin/bash # Locate all Protocol usages to scope refactor rg -n "Openpyxl.*Protocol|Xlwings.*Protocol" src/exstruct/mcp/patch --type py🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/models.py` around lines 133 - 379, The Protocol classes (e.g., OpenpyxlCellProtocol, OpenpyxlWorksheetProtocol, OpenpyxlWorkbookProtocol, XlwingsRangeProtocol, XlwingsSheetProtocol and all other Openpyxl*/Xlwings*Protocol types) leak external-library internals; replace their usage by accepting objects as Any at the public boundary and normalize them immediately into your Pydantic snapshot models via small adapter functions. Concretely: remove or stop exporting the Openpyxl*/Xlwings*Protocol types from the API surface, change public entry-point signatures that currently reference those protocols (search for usages of OpenpyxlWorksheetProtocol, OpenpyxlWorkbookProtocol, XlwingsSheetProtocol, XlwingsWorkbookProtocol, etc.) to accept Any, and implement/inline normalizer functions (e.g., normalize_openpyxl_workbook, normalize_openpyxl_cell, normalize_xlwings_sheet) that inspect the runtime object and construct the existing snapshot/BaseModel types; update callers to use these normalizers immediately at the boundary so internal code works only with Pydantic models. Ensure tests and type hints internal to the module keep concrete Pydantic types, not the external Protocols.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/exstruct/mcp/patch/service.py`:
- Around line 179-183: Replace the tuple return with a Pydantic BaseModel:
define an OpenpyxlEngineResult BaseModel with fields diff: list[object],
inverse_ops: list[object], formula_issues: list[object], op_warnings: list[str];
update both apply_openpyxl_ops and apply_openpyxl_engine to return
OpenpyxlEngineResult instead of tuple, and update the call site in service.py
(the call to apply_openpyxl_engine) to receive a single variable (e.g.,
engine_result) and consume engine_result.diff, engine_result.inverse_ops,
engine_result.formula_issues, and engine_result.op_warnings accordingly.
In `@tests/mcp/patch/test_service.py`:
- Around line 13-220: Add Google-style docstrings to each newly added function:
the helper _create_workbook and the test functions
test_patch_runner_run_patch_delegates_to_service,
test_patch_runner_run_make_delegates_to_service,
test_service_run_patch_backend_auto_prefers_com,
test_service_run_patch_backend_auto_fallbacks_to_openpyxl_on_com_error,
test_service_run_patch_backend_com_does_not_fallback_on_com_error, and
test_service_run_patch_backend_com_fallbacks_for_apply_table_style; for each
function include a one-line summary, a short description of parameters (e.g.,
tmp_path, monkeypatch, input_path, request) and return behavior (if any)
following Google-style docstring format so the repo linting/guidelines are
satisfied.
---
Outside diff comments:
In `@src/exstruct/mcp/server.py`:
- Around line 214-227: Update the `_create_app` Google-style docstring to
document the new parameters: add `on_conflict` (describe type OnConflictPolicy
and what it controls, e.g., how artifact/name conflicts are handled) and
`artifact_bridge_dir` (describe it as Path | None and what passing a path vs
None does, e.g., directory used for artifact bridging or disabled when None);
keep the existing `policy` entry and the `Returns` section unchanged and ensure
parameter descriptions are concise and aligned with the existing Args
formatting.
In `@tests/mcp/test_server.py`:
- Around line 42-1267: The DummyApp class and newly added test helpers (e.g.,
DummyApp, _call_async) and test functions in tests/mcp/test_server.py lack
Google-style docstrings; add concise Google-style docstrings to the DummyApp
class and its tool decorator, the _call_async helper, and any newly added test
functions/helpers (mentioning purpose, Args, Returns where applicable) so they
meet the project's docstring requirement and describe behavior and parameters
for each symbol (DummyApp, DummyApp.tool, _call_async, and the new/modified
test_* functions).
---
Duplicate comments:
In `@src/exstruct/mcp/patch/models.py`:
- Around line 727-748: The current _validate_set_range_values validates
rectangularity of op.values but doesn't ensure the values' dimensions match the
A1 range in op.range; update _validate_set_range_values to compute the range's
height and width from op.range and compare them to len(op.values) and
len(op.values[0]) respectively, and raise a ValueError (e.g. "set_range_values
requires values matching range dimensions") when they differ; keep existing
checks (non-empty, rectangular) and reference op.range and op.values in the
validation and error message.
- Around line 133-379: The Protocol classes (e.g., OpenpyxlCellProtocol,
OpenpyxlWorksheetProtocol, OpenpyxlWorkbookProtocol, XlwingsRangeProtocol,
XlwingsSheetProtocol and all other Openpyxl*/Xlwings*Protocol types) leak
external-library internals; replace their usage by accepting objects as Any at
the public boundary and normalize them immediately into your Pydantic snapshot
models via small adapter functions. Concretely: remove or stop exporting the
Openpyxl*/Xlwings*Protocol types from the API surface, change public entry-point
signatures that currently reference those protocols (search for usages of
OpenpyxlWorksheetProtocol, OpenpyxlWorkbookProtocol, XlwingsSheetProtocol,
XlwingsWorkbookProtocol, etc.) to accept Any, and implement/inline normalizer
functions (e.g., normalize_openpyxl_workbook, normalize_openpyxl_cell,
normalize_xlwings_sheet) that inspect the runtime object and construct the
existing snapshot/BaseModel types; update callers to use these normalizers
immediately at the boundary so internal code works only with Pydantic models.
Ensure tests and type hints internal to the module keep concrete Pydantic types,
not the external Protocols.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
docs/agents/FEATURE_SPEC.mddocs/agents/LEGACY_DEPENDENCY_INVENTORY.mddocs/agents/TASKS.mddocs/mcp.mdsrc/exstruct/mcp/patch/internal.pysrc/exstruct/mcp/patch/models.pysrc/exstruct/mcp/patch/service.pysrc/exstruct/mcp/server.pytests/mcp/patch/test_service.pytests/mcp/test_server.py
✅ Files skipped from review due to trivial changes (1)
- docs/agents/LEGACY_DEPENDENCY_INVENTORY.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
src/exstruct/mcp/patch/models.py (2)
727-749:_validate_set_range_valuesstill does not verify thatvaluesshape matches the A1 range dimensions (flagged in previous review).Only rectangular shape is enforced; a mismatch between the declared range (e.g.,
A1:C3) and the actual values matrix passes validation here and fails later at engine execution time.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/models.py` around lines 727 - 749, _validator _validate_set_range_values currently only enforces rectangular op.values but not that its shape matches the declared A1 op.range; update _validate_set_range_values to parse op.range (the A1 range string) to compute expected_row_count and expected_col_count and then check len(op.values) == expected_row_count and expected_width (computed as len(op.values[0])) == expected_col_count, raising a ValueError with a clear message (e.g. "set_range_values values shape does not match range") if they differ; keep existing checks (no cell/base_cell/expected/formula and non-empty rows) and reuse variables op.range, op.values, expected_width to locate where to add the new validations.
133-379: Protocol types encoding library internals still present in the model layer (deferred from previous review).
Openpyxl*andXlwings*Protocols expose internal library attributes (e.g.,font.bold,interior.Color,lineStyle) and COM API methods directly in the models module. Per the coding guidelines, external library objects should be accepted asAnyat entry points and immediately normalized to Pydantic domain models — not propagated as Protocols throughout the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/models.py` around lines 133 - 379, The model layer currently exposes external-library internals via Protocols (e.g., OpenpyxlCellProtocol, OpenpyxlFontProtocol, OpenpyxlFillProtocol, OpenpyxlBorderProtocol, XlwingsRangeApiProtocol, XlwingsFontApiProtocol, XlwingsInteriorApiProtocol, etc.) which leaks library-specific attrs (font.bold, interior.Color, Borders, LineStyle, api) into domain types; change these Protocol definitions so any external library object is typed as Any at entry points (e.g., replace detailed attribute types like font: OpenpyxlFontProtocol or Interior: XlwingsInteriorApiProtocol with font: Any, interior: Any, api: Any), remove/avoid exposing COM method signatures in model protocols, and ensure callers convert/normalize received Any objects to Pydantic domain models (create or use existing normalizer functions in the patch runner) before using attributes like bold, Color, MergeCells. Refer to the Openpyxl* and Xlwings* Protocol class names above when making these replacements and add tests or type assertions to ensure normalization happens at workbook/sheet entry points.
🧹 Nitpick comments (6)
docs/agents/TASKS.md (1)
136-183: Inconsistent完了条件:formatting within Epic 2.Epic 1 uses a two-level structure — tasks as list items, then
完了条件:as a standalone label followed by its own checklist. Epic 2 inlines completion conditions directly as checklist items (- [ ] 完了条件: ...), mixing acceptance criteria with tasks in the same list, which reduces readability.♻️ Example alignment to Epic 1 style (illustrative for section 0)
## 0. 現状固定と差分計測 - [x] `coverage.xml` を基準値として保存（78.24% / miss 1,654） - [x] 低下主因3ファイルの未実行行を記録（internal/models/server） - [ ] 改善後比較用コマンドを固定化 -- [ ] 完了条件: before/after の比較表が作成されている + +完了条件: +- [ ] before/after の比較表が作成されている🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/agents/TASKS.md` around lines 136 - 183, The "## 2. `patch/internal.py` 分岐網羅" section mixes the acceptance criteria into task checklist items (e.g. "- [ ] 完了条件: ..."), which is inconsistent with Epic 1's two-level layout; update that section so "完了条件:" is a standalone heading/label followed by its own checklist (separate lines) instead of inline checklist items—adjust the block containing the openpyxl/conflict policy/dry_run items under "## 2. `patch/internal.py` 分岐網羅" to match the pattern used in "## 0. 現状固定と差分計測" and other epics.tests/mcp/patch/test_service.py (2)
88-116: Uninitializedcallskey yieldsKeyError(test ERROR) instead of a clearAssertionError(test FAILURE).If
_fake_apply_xlwings_engineis never invoked,calls["com"]raisesKeyError, causing pytest to report an unexpected ERROR rather than a descriptive assertion failure. Initialise the sentinel toFalseso the assertion message is unambiguous.🔧 Proposed fix
- calls: dict[str, bool] = {} + calls: dict[str, bool] = {"com": False}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/patch/test_service.py` around lines 88 - 116, The test uses a sentinel dict "calls" and later asserts calls["com"] but never initializes that key, causing a KeyError; initialize the sentinel key before the monkeypatch (e.g., set calls["com"] = False) or use a default-access pattern so that _fake_apply_xlwings_engine can flip it to True; update the test around the _fake_apply_xlwings_engine and the assertion that follows (the calls dict, the fake function, and the final assert result.engine == "com" / assert calls["com"]) to ensure calls["com"] exists with a False default prior to invoking service.run_patch.
205-317: Parametrize the two near-identicalapply_table_stylefallback tests.
test_service_run_patch_backend_com_fallbacks_for_apply_table_styleandtest_service_run_patch_backend_auto_fallbacks_for_apply_table_styleare identical except for thebackend=value. Collapsing them into a single parametrized test eliminates duplication and makes future backend additions a one-liner.♻️ Proposed refactor
-def test_service_run_patch_backend_com_fallbacks_for_apply_table_style( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """Verify backend=com reroutes apply_table_style to openpyxl. - - Args: - tmp_path: Temporary directory fixture. - monkeypatch: Pytest monkeypatch fixture. - """ - ... # lines 214-260 - - -def test_service_run_patch_backend_auto_fallbacks_for_apply_table_style( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """Verify backend=auto reroutes apply_table_style to openpyxl. - - Args: - tmp_path: Temporary directory fixture. - monkeypatch: Pytest monkeypatch fixture. - """ - ... # lines 272-317 +@pytest.mark.parametrize("backend", ["com", "auto"]) +def test_service_run_patch_backend_fallbacks_for_apply_table_style( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch, backend: str +) -> None: + """Verify both COM and auto backends reroute apply_table_style to openpyxl. + + Args: + tmp_path: Temporary directory fixture. + monkeypatch: Pytest monkeypatch fixture. + backend: Backend value under test ("com" or "auto"). + """ + input_path = tmp_path / "book.xlsx" + _create_workbook(input_path) + + monkeypatch.setattr( + patch_runtime, + "get_com_availability", + lambda: ComAvailability(available=True, reason=None), + ) + + def _fail_if_called( + input_path: Path, + output_path: Path, + ops: list[PatchOp], + auto_formula: bool, + ) -> list[object]: + raise AssertionError("COM backend should not be called for apply_table_style") + + def _fake_apply_openpyxl_engine( + request: PatchRequest, + input_path: Path, + output_path: Path, + ) -> OpenpyxlEngineResult: + return OpenpyxlEngineResult() + + monkeypatch.setattr(service, "apply_xlwings_engine", _fail_if_called) + monkeypatch.setattr(service, "apply_openpyxl_engine", _fake_apply_openpyxl_engine) + result = service.run_patch( + PatchRequest( + xlsx_path=input_path, + ops=[ + PatchOp( + op="apply_table_style", + sheet="Sheet1", + range="A1:B3", + style="TableStyleMedium2", + table_name="SalesTable", + ) + ], + on_conflict="rename", + backend=backend, + ) + ) + assert result.error is None + assert result.engine == "openpyxl" + assert any( + "does not support apply_table_style" in warning for warning in result.warnings + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/mcp/patch/test_service.py` around lines 205 - 317, Two tests test_service_run_patch_backend_com_fallbacks_for_apply_table_style and test_service_run_patch_backend_auto_fallbacks_for_apply_table_style are duplicate except for backend value; refactor by parameterizing a single test over backend ("com" and "auto"). Update the test to accept a backend parameter and replace the two functions with one pytest.mark.parametrize-decorated function that constructs the same PatchRequest using the parametrized backend, keeps the monkeypatching of patch_runtime.get_com_availability and service.apply_xlwings_engine/service.apply_openpyxl_engine, calls service.run_patch, and asserts result.error/result.engine/result.warnings as before so coverage is unchanged (refer to test names, PatchRequest, PatchOp, service.run_patch, apply_xlwings_engine, apply_openpyxl_engine).src/exstruct/mcp/patch/models.py (3)
1322-1364:_validate_backend_constraintsis copy-pasted verbatim betweenPatchRequestandMakeRequest.Both model validators have identical bodies. Extract a shared module-level helper to keep the constraint logic in a single place.
♻️ Proposed refactor
+def _validate_com_backend_constraints( + backend: PatchBackend, + dry_run: bool, + return_inverse_ops: bool, + preflight_formula_check: bool, + ops: list[PatchOp], +) -> None: + """Raise ValueError if com backend is used with unsupported options.""" + if backend != "com": + return + if dry_run or return_inverse_ops or preflight_formula_check: + raise ValueError( + "backend='com' does not support dry_run, return_inverse_ops, " + "or preflight_formula_check." + ) + if any(op.op == "restore_design_snapshot" for op in ops): + raise ValueError( + "backend='com' does not support restore_design_snapshot operation." + ) + class PatchRequest(BaseModel): ... `@model_validator`(mode="after") def _validate_backend_constraints(self) -> PatchRequest: - if self.backend != "com": - return self - if self.dry_run or self.return_inverse_ops or self.preflight_formula_check: - raise ValueError( - "backend='com' does not support dry_run, return_inverse_ops, " - "or preflight_formula_check." - ) - if any(op.op == "restore_design_snapshot" for op in self.ops): - raise ValueError( - "backend='com' does not support restore_design_snapshot operation." - ) - return self + _validate_com_backend_constraints( + self.backend, self.dry_run, self.return_inverse_ops, + self.preflight_formula_check, self.ops + ) + return self class MakeRequest(BaseModel): ... `@model_validator`(mode="after") def _validate_backend_constraints(self) -> MakeRequest: - # (same diff as above) + _validate_com_backend_constraints( + self.backend, self.dry_run, self.return_inverse_ops, + self.preflight_formula_check, self.ops + ) + return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/models.py` around lines 1322 - 1364, The two identical `@model_validator` methods named _validate_backend_constraints on PatchRequest and MakeRequest should be replaced by a single module-level helper (e.g., validate_backend_constraints or _check_backend_constraints) that encapsulates the logic (inspect backend, dry_run, return_inverse_ops, preflight_formula_check, and ops for restore_design_snapshot) and raises the same ValueErrors; then change both PatchRequest._validate_backend_constraints and MakeRequest._validate_backend_constraints to simply call that helper with self (or the necessary attributes) and return self so behavior remains identical but the constraint logic lives in one place.
844-849: Field mutation inside_validate_set_boldis fragile ifvalidate_assignment=Trueis ever added.
op.bold = Truedirectly mutates the model instance within the model validator chain. Ifvalidate_assignment=Trueis set onPatchOp'smodel_config, the same field assignment triggers re-validation, which re-runs_validate_op, resulting in aRecursionError. Consider expressing the default viaobject.__setattr__(self, "bold", True)(bypasses re-validation) or by moving the default-fill logic into a dedicated@model_validator(mode="after")that usesmodel_copy(update=...).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/models.py` around lines 844 - 849, The validator _validate_set_bold mutates op.bold directly which will cause re-validation/recursion if PatchOp later enables validate_assignment; change the logic so you do not assign via the public setter inside the validator: either set the default using object.__setattr__(op, "bold", True) to bypass re-validation, or move the default-fill into a dedicated `@model_validator`(mode="after") for PatchOp and return model_copy(update={"bold": True}) when bold is None; keep the existing calls to _validate_no_alignment_fields, _validate_exactly_one_cell_or_range, and _validate_style_target_size but remove direct op.bold = True in _validate_set_bold.
622-630:_validate_opsilently skips validation for unrecognised op types.
_validator_for_opreturnsNonefor any op not in its dispatch dict (line 665), and_validate_opimmediately returnsselfin that case (line 625–626). Any newPatchOpTypeadded to.typeswithout a matching entry will pass model validation with zero checks applied.🛡️ Proposed guard
def _validate_op(self) -> PatchOp: validator = _validator_for_op(self.op) if validator is None: - return self + raise ValueError(f"No validator registered for op type: {self.op!r}") if self.op in _CELL_REQUIRED_OPS:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/exstruct/mcp/patch/models.py` around lines 622 - 630, The _validate_op validator currently returns self when _validator_for_op(self.op) is None, which silently skips validation for unrecognized PatchOpType values; change _validate_op so that if validator is None it raises a clear validation error (e.g., ValueError or TypeError) referencing the offending op (self.op) instead of returning self, while still calling _validate_cell_required when self.op is in _CELL_REQUIRED_OPS; update the method _validate_op (and any tests) to raise on unknown ops so new PatchOpType entries without a matching _validator_for_op dispatch won't bypass validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/agents/FEATURE_SPEC.md`:
- Around line 92-93: The nested sub-item under the numbered item starting with
"10." (the section containing "`on_conflict`、`rename`、出力先決定の共通関数") is indented 3
spaces and triggers MD005; change that nested list's indentation to 4 spaces so
it aligns with other sub-items under 1.–9. and satisfies the markdown linter
(precommit). Locate the list under the "10." header and increase its child
line(s) indentation from 3 to 4 spaces to fix the MD005 error.
- Around line 165-314: Multiple spec sections reuse identical level-2 headings
(e.g., "## Feature Name", "## 背景", "## 目的", "## スコープ", "## 受け入れ基準", "##
リスクと対策"), causing MD024 failures; collapse each spec's title line into a single
unique level-2 heading (combine "## Feature Name" and its following name into
one distinct "## <Spec Title>") and demote the repeated sub-headings ("## 背景",
"## 目的", etc.) to level-3 ("###") for each spec (apply same change to all three
specs including "PR `#65` Review Follow-up (Stabilization)"); this will eliminate
duplicate-heading violations and allow uv run task precommit-run to pass.
In `@docs/agents/TASKS.md`:
- Around line 173-183: Enable the coverage gate by marking the Codecov "project"
and "patch" statuses as required in your branch protection settings and then run
the verification pytest command locally or in CI using the exact task: uv run
pytest -m "not com and not render" --cov=exstruct --cov-report=xml
--cov-fail-under=80 to ensure the branch actually fails below 80%; confirm
codecov.yml still has patch: 80 and re-run uv run coverage report -m to verify
overall >=80% and that low-coverage files (e.g., patch/models.py) have been
addressed or open a follow-up issue if you intend to defer fixes.
In `@src/exstruct/mcp/patch/models.py`:
- Around line 1367-1380: OpenpyxlEngineResult currently types patch_diff,
inverse_ops, and formula_issues as list[object], which bypasses Pydantic
validation; update those annotations to the concrete Pydantic models declared
earlier in the file (use list[PatchDiffItem] for patch_diff, list[PatchOp] for
inverse_ops, and list[FormulaIssue] for formula_issues) so Pydantic will
validate and serialize correctly while leaving op_warnings as list[str]; ensure
the referenced model names (PatchDiffItem, PatchOp, FormulaIssue) are available
in the same module scope.
- Around line 29-35: Remove the duplicate unused constants from models.py:
delete _ALLOWED_EXTENSIONS and _SOFT_MAX_OPS_WARNING_THRESHOLD (they are defined
and used in validate_input.py/internal.py), and either remove or relocate
_XLWINGS_HORIZONTAL_ALIGN_MAP and _XLWINGS_VERTICAL_ALIGN_MAP from models.py to
the module that actually consumes them (or import them from that module) so
models.py no longer declares unused alignment maps; update any imports/usages
accordingly and run tests/static checks to ensure no references break.
- Around line 37-53: Remove the unused xlwings COM constant maps
_XLWINGS_HORIZONTAL_ALIGN_MAP and _XLWINGS_VERTICAL_ALIGN_MAP from models.py;
these definitions are duplicates of the maps in internal.py and leaking
adapter-specific details into the data model layer. Edit
src/exstruct/mcp/patch/models.py to delete the two dict definitions
(_XLWINGS_HORIZONTAL_ALIGN_MAP and _XLWINGS_VERTICAL_ALIGN_MAP), ensure no
references to those symbols remain in that file, and rely on the definitions in
src/exstruct/mcp/patch/internal.py where the maps are actually used (e.g., usage
around the symbols referenced at lines near 3612/3616/3649/3653 in internal.py);
run tests/linting to confirm nothing else imports or depends on those names from
models.py.
In `@tests/mcp/patch/test_service.py`:
- Around line 21-23: workbook.active is typed Optional[Worksheet], so add a
None-safe guard before using sheet.title and sheet["A1"]; e.g., retrieve the
active sheet into a variable (sheet = workbook.active), assert or raise if sheet
is None (or fall back to workbook.worksheets[0]), then proceed to set
sheet.title and sheet["A1"]; update references to the symbols workbook.active,
sheet.title, and sheet["A1"] so mypy --strict no longer reports a possible None
access.
---
Duplicate comments:
In `@src/exstruct/mcp/patch/models.py`:
- Around line 727-749: _validator _validate_set_range_values currently only
enforces rectangular op.values but not that its shape matches the declared A1
op.range; update _validate_set_range_values to parse op.range (the A1 range
string) to compute expected_row_count and expected_col_count and then check
len(op.values) == expected_row_count and expected_width (computed as
len(op.values[0])) == expected_col_count, raising a ValueError with a clear
message (e.g. "set_range_values values shape does not match range") if they
differ; keep existing checks (no cell/base_cell/expected/formula and non-empty
rows) and reuse variables op.range, op.values, expected_width to locate where to
add the new validations.
- Around line 133-379: The model layer currently exposes external-library
internals via Protocols (e.g., OpenpyxlCellProtocol, OpenpyxlFontProtocol,
OpenpyxlFillProtocol, OpenpyxlBorderProtocol, XlwingsRangeApiProtocol,
XlwingsFontApiProtocol, XlwingsInteriorApiProtocol, etc.) which leaks
library-specific attrs (font.bold, interior.Color, Borders, LineStyle, api) into
domain types; change these Protocol definitions so any external library object
is typed as Any at entry points (e.g., replace detailed attribute types like
font: OpenpyxlFontProtocol or Interior: XlwingsInteriorApiProtocol with font:
Any, interior: Any, api: Any), remove/avoid exposing COM method signatures in
model protocols, and ensure callers convert/normalize received Any objects to
Pydantic domain models (create or use existing normalizer functions in the patch
runner) before using attributes like bold, Color, MergeCells. Refer to the
Openpyxl* and Xlwings* Protocol class names above when making these replacements
and add tests or type assertions to ensure normalization happens at
workbook/sheet entry points.
---
Nitpick comments:
In `@docs/agents/TASKS.md`:
- Around line 136-183: The "## 2. `patch/internal.py` 分岐網羅" section mixes the
acceptance criteria into task checklist items (e.g. "- [ ] 完了条件: ..."), which is
inconsistent with Epic 1's two-level layout; update that section so "完了条件:" is a
standalone heading/label followed by its own checklist (separate lines) instead
of inline checklist items—adjust the block containing the openpyxl/conflict
policy/dry_run items under "## 2. `patch/internal.py` 分岐網羅" to match the pattern
used in "## 0. 現状固定と差分計測" and other epics.
In `@src/exstruct/mcp/patch/models.py`:
- Around line 1322-1364: The two identical `@model_validator` methods named
_validate_backend_constraints on PatchRequest and MakeRequest should be replaced
by a single module-level helper (e.g., validate_backend_constraints or
_check_backend_constraints) that encapsulates the logic (inspect backend,
dry_run, return_inverse_ops, preflight_formula_check, and ops for
restore_design_snapshot) and raises the same ValueErrors; then change both
PatchRequest._validate_backend_constraints and
MakeRequest._validate_backend_constraints to simply call that helper with self
(or the necessary attributes) and return self so behavior remains identical but
the constraint logic lives in one place.
- Around line 844-849: The validator _validate_set_bold mutates op.bold directly
which will cause re-validation/recursion if PatchOp later enables
validate_assignment; change the logic so you do not assign via the public setter
inside the validator: either set the default using object.__setattr__(op,
"bold", True) to bypass re-validation, or move the default-fill into a dedicated
`@model_validator`(mode="after") for PatchOp and return model_copy(update={"bold":
True}) when bold is None; keep the existing calls to
_validate_no_alignment_fields, _validate_exactly_one_cell_or_range, and
_validate_style_target_size but remove direct op.bold = True in
_validate_set_bold.
- Around line 622-630: The _validate_op validator currently returns self when
_validator_for_op(self.op) is None, which silently skips validation for
unrecognized PatchOpType values; change _validate_op so that if validator is
None it raises a clear validation error (e.g., ValueError or TypeError)
referencing the offending op (self.op) instead of returning self, while still
calling _validate_cell_required when self.op is in _CELL_REQUIRED_OPS; update
the method _validate_op (and any tests) to raise on unknown ops so new
PatchOpType entries without a matching _validator_for_op dispatch won't bypass
validation.
In `@tests/mcp/patch/test_service.py`:
- Around line 88-116: The test uses a sentinel dict "calls" and later asserts
calls["com"] but never initializes that key, causing a KeyError; initialize the
sentinel key before the monkeypatch (e.g., set calls["com"] = False) or use a
default-access pattern so that _fake_apply_xlwings_engine can flip it to True;
update the test around the _fake_apply_xlwings_engine and the assertion that
follows (the calls dict, the fake function, and the final assert result.engine
== "com" / assert calls["com"]) to ensure calls["com"] exists with a False
default prior to invoking service.run_patch.
- Around line 205-317: Two tests
test_service_run_patch_backend_com_fallbacks_for_apply_table_style and
test_service_run_patch_backend_auto_fallbacks_for_apply_table_style are
duplicate except for backend value; refactor by parameterizing a single test
over backend ("com" and "auto"). Update the test to accept a backend parameter
and replace the two functions with one pytest.mark.parametrize-decorated
function that constructs the same PatchRequest using the parametrized backend,
keeps the monkeypatching of patch_runtime.get_com_availability and
service.apply_xlwings_engine/service.apply_openpyxl_engine, calls
service.run_patch, and asserts result.error/result.engine/result.warnings as
before so coverage is unchanged (refer to test names, PatchRequest, PatchOp,
service.run_patch, apply_xlwings_engine, apply_openpyxl_engine).
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
docs/agents/FEATURE_SPEC.mddocs/agents/TASKS.mdsrc/exstruct/mcp/patch/engine/openpyxl_engine.pysrc/exstruct/mcp/patch/models.pysrc/exstruct/mcp/patch/ops/openpyxl_ops.pysrc/exstruct/mcp/patch/service.pytests/mcp/patch/test_ops.pytests/mcp/patch/test_service.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/exstruct/mcp/patch/ops/openpyxl_ops.py
- tests/mcp/patch/test_ops.py
- src/exstruct/mcp/patch/engine/openpyxl_engine.py
- src/exstruct/mcp/patch/service.py
Comment on lines
+29
to
+35
| _ALLOWED_EXTENSIONS = {".xlsx", ".xlsm", ".xls"} | ||
| _A1_PATTERN = re.compile(r"^[A-Za-z]{1,3}[1-9][0-9]*$") | ||
| _A1_RANGE_PATTERN = re.compile(r"^[A-Za-z]{1,3}[1-9][0-9]*:[A-Za-z]{1,3}[1-9][0-9]*$") | ||
| _HEX_COLOR_PATTERN = re.compile(r"^#?(?:[0-9A-Fa-f]{6}|[0-9A-Fa-f]{8})$") | ||
| _COLUMN_LABEL_PATTERN = re.compile(r"^[A-Za-z]{1,3}$") | ||
| _MAX_STYLE_TARGET_CELLS = 10_000 | ||
| _SOFT_MAX_OPS_WARNING_THRESHOLD = 200 |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for usages of the constants across all Python files
echo "=== Searching for _ALLOWED_EXTENSIONS usage ==="
rg -n "_ALLOWED_EXTENSIONS" --type py
echo -e "\n=== Searching for _SOFT_MAX_OPS_WARNING_THRESHOLD usage ==="
rg -n "_SOFT_MAX_OPS_WARNING_THRESHOLD" --type pyRepository: harumiWeb/exstruct
Length of output: 982
🏁 Script executed:
#!/bin/bash
# Check the PatchRequest and MakeRequest models for xpath and out_path validators
echo "=== PatchRequest definition around xlsx_path ==="
sed -n '1307,1340p' src/exstruct/mcp/patch/models.py
echo -e "\n=== MakeRequest definition around out_path ==="
sed -n '1341,1365p' src/exstruct/mcp/patch/models.pyRepository: harumiWeb/exstruct
Length of output: 2256
🏁 Script executed:
#!/bin/bash
# Check imports and overall structure to understand if these constants are truly unused
echo "=== Check all references in models.py ==="
rg -n "ALLOWED_EXTENSIONS|SOFT_MAX_OPS_WARNING_THRESHOLD|XLWINGS_HORIZONTAL|XLWINGS_VERTICAL" src/exstruct/mcp/patch/models.pyRepository: harumiWeb/exstruct
Length of output: 330
_ALLOWED_EXTENSIONS and _SOFT_MAX_OPS_WARNING_THRESHOLD are unused in this file and appear to be redundant duplicates.
These constants (lines 29 and 35) are defined here but never referenced in models.py. They are defined and actively used in validate_input.py and internal.py instead. This duplication suggests either an incomplete refactoring or copy-paste artifact. Remove these constants from models.py to avoid confusion and maintenance overhead.
Additionally, _XLWINGS_HORIZONTAL_ALIGN_MAP and _XLWINGS_VERTICAL_ALIGN_MAP (lines 37–53) are also defined but never used in this file. If they are not used elsewhere either, consider relocating them to the module that actually consumes them.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/exstruct/mcp/patch/models.py` around lines 29 - 35, Remove the duplicate
unused constants from models.py: delete _ALLOWED_EXTENSIONS and
_SOFT_MAX_OPS_WARNING_THRESHOLD (they are defined and used in
validate_input.py/internal.py), and either remove or relocate
_XLWINGS_HORIZONTAL_ALIGN_MAP and _XLWINGS_VERTICAL_ALIGN_MAP from models.py to
the module that actually consumes them (or import them from that module) so
models.py no longer declares unused alignment maps; update any imports/usages
accordingly and run tests/static checks to ensure no references break.
Comment on lines
+37
to
+53
| _XLWINGS_HORIZONTAL_ALIGN_MAP: dict[HorizontalAlignType, int] = { | ||
| "general": -4105, | ||
| "left": -4131, | ||
| "center": -4108, | ||
| "right": -4152, | ||
| "fill": 5, | ||
| "justify": -4130, | ||
| "centerContinuous": 7, | ||
| "distributed": -4117, | ||
| } | ||
| _XLWINGS_VERTICAL_ALIGN_MAP: dict[VerticalAlignType, int] = { | ||
| "top": -4160, | ||
| "center": -4108, | ||
| "bottom": -4107, | ||
| "justify": -4130, | ||
| "distributed": -4117, | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "_XLWINGS_HORIZONTAL_ALIGN_MAP|_XLWINGS_VERTICAL_ALIGN_MAP" --type pyRepository: harumiWeb/exstruct
Length of output: 941
🏁 Script executed:
# Check the models.py file around the map definitions
sed -n '37,53p' src/exstruct/mcp/patch/models.py
# Check the internal.py file around the map definitions
sed -n '49,69p' src/exstruct/mcp/patch/internal.py
# Verify imports/dependencies to understand the layering
head -20 src/exstruct/mcp/patch/models.py
head -20 src/exstruct/mcp/patch/internal.pyRepository: harumiWeb/exstruct
Length of output: 2256
Remove unused xlwings COM constant maps from models.py—these belong in the adapter layer.
_XLWINGS_HORIZONTAL_ALIGN_MAP and _XLWINGS_VERTICAL_ALIGN_MAP are dead code in this file. The maps are defined identically in src/exstruct/mcp/patch/internal.py (lines 49, 59) where they are actually used (lines 3612, 3616, 3649, 3653). Xlwings COM internals should not leak into the data model layer; remove these unused definitions from models.py.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/exstruct/mcp/patch/models.py` around lines 37 - 53, Remove the unused
xlwings COM constant maps _XLWINGS_HORIZONTAL_ALIGN_MAP and
_XLWINGS_VERTICAL_ALIGN_MAP from models.py; these definitions are duplicates of
the maps in internal.py and leaking adapter-specific details into the data model
layer. Edit src/exstruct/mcp/patch/models.py to delete the two dict definitions
(_XLWINGS_HORIZONTAL_ALIGN_MAP and _XLWINGS_VERTICAL_ALIGN_MAP), ensure no
references to those symbols remain in that file, and rely on the definitions in
src/exstruct/mcp/patch/internal.py where the maps are actually used (e.g., usage
around the symbols referenced at lines near 3612/3616/3649/3653 in internal.py);
run tests/linting to confirm nothing else imports or depends on those names from
models.py.
Comment on lines
+1367
to
+1380
| class OpenpyxlEngineResult(BaseModel): | ||
| """Structured result returned by the openpyxl engine boundary. | ||
|
|
||
| Attributes: | ||
| patch_diff: Backend patch diff payload items. | ||
| inverse_ops: Backend inverse operation payload items. | ||
| formula_issues: Backend formula issue payload items. | ||
| op_warnings: Backend warning messages emitted during apply. | ||
| """ | ||
|
|
||
| patch_diff: list[object] = Field(default_factory=list) | ||
| inverse_ops: list[object] = Field(default_factory=list) | ||
| formula_issues: list[object] = Field(default_factory=list) | ||
| op_warnings: list[str] = Field(default_factory=list) |
Contributor
There was a problem hiding this comment.
OpenpyxlEngineResult uses list[object] for structured Pydantic types available in the same file.
patch_diff, inverse_ops, and formula_issues are documented as PatchDiffItem, PatchOp, and FormulaIssue items respectively. Annotating them as list[object] silently discards Pydantic validation and type narrowing, which contradicts both the stated PR goal ("unify openpyxl engine return type to BaseModel") and the guideline requiring structured data to be represented as Pydantic models. There is no circular-import barrier since all three types are declared above in the same file.
🔧 Proposed fix
class OpenpyxlEngineResult(BaseModel):
"""Structured result returned by the openpyxl engine boundary.
Attributes:
patch_diff: Backend patch diff payload items.
inverse_ops: Backend inverse operation payload items.
formula_issues: Backend formula issue payload items.
op_warnings: Backend warning messages emitted during apply.
"""
- patch_diff: list[object] = Field(default_factory=list)
- inverse_ops: list[object] = Field(default_factory=list)
- formula_issues: list[object] = Field(default_factory=list)
+ patch_diff: list[PatchDiffItem] = Field(default_factory=list)
+ inverse_ops: list[PatchOp] = Field(default_factory=list)
+ formula_issues: list[FormulaIssue] = Field(default_factory=list)
op_warnings: list[str] = Field(default_factory=list)As per coding guidelines: "Return Pydantic BaseModel objects instead of dictionaries or tuples; all structured data must be represented using Pydantic models."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/exstruct/mcp/patch/models.py` around lines 1367 - 1380,
OpenpyxlEngineResult currently types patch_diff, inverse_ops, and formula_issues
as list[object], which bypasses Pydantic validation; update those annotations to
the concrete Pydantic models declared earlier in the file (use
list[PatchDiffItem] for patch_diff, list[PatchOp] for inverse_ops, and
list[FormulaIssue] for formula_issues) so Pydantic will validate and serialize
correctly while leaving op_warnings as list[str]; ensure the referenced model
names (PatchDiffItem, PatchOp, FormulaIssue) are available in the same module
scope.
Comment on lines
+21
to
+23
| sheet = workbook.active | ||
| sheet.title = "Sheet1" | ||
| sheet["A1"] = "old" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
openpyxl Workbook.active return type Optional[Worksheet] mypy
💡 Result:
In the current typeshed stubs for openpyxl, Workbook.active is typed as possibly missing:
@property def active(self) -> _WorksheetOrChartsheetLike | None: ...(i.e.Optional[...]) [1]
So with mypy, you need to narrow away None (and often also narrow away Chartsheet) before using worksheet-only attributes:
from openpyxl import Workbook
from openpyxl.worksheet.worksheet import Worksheet
from typing import cast
wb = Workbook()
active = wb.active
assert active is not None # fixes Optional
ws = cast(Worksheet, active) # if you need Worksheet specificallyIf you just want a guaranteed non-optional first worksheet, openpyxl documents that a new Workbook() “is always created with at least one worksheet” [2], so this is also a common pattern:
ws = wb.worksheets[0]Sources: [1] typeshed openpyxl stub for Workbook.active; [2] openpyxl tutorial stating a workbook is created with at least one worksheet.
🏁 Script executed:
# Check the actual file and lines 21-23
cat -n tests/mcp/patch/test_service.py | sed -n '14,25p'Repository: harumiWeb/exstruct
Length of output: 450
🏁 Script executed:
# Check if there's a pyproject.toml or requirements to see openpyxl version
find . -maxdepth 2 -type f \( -name "*.toml" -o -name "*.txt" -o -name "*.in" \) | head -20Repository: harumiWeb/exstruct
Length of output: 105
workbook.active returns Optional[Worksheet] — add None guard for mypy strict compliance.
In openpyxl, Workbook.active is typed as Optional[Worksheet] in the typeshed stubs. Lines 22–23 access .title and ["A1"] on a potentially-None reference, which is a type error under mypy --strict. While a new Workbook() is guaranteed to have at least one active sheet at runtime, the coding guidelines require mypy strict compatibility with zero errors.
Proposed fix
sheet = workbook.active
+ assert sheet is not None, "Workbook must have an active sheet"
sheet.title = "Sheet1"
sheet["A1"] = "old"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/mcp/patch/test_service.py` around lines 21 - 23, workbook.active is
typed Optional[Worksheet], so add a None-safe guard before using sheet.title and
sheet["A1"]; e.g., retrieve the active sheet into a variable (sheet =
workbook.active), assert or raise if sheet is None (or fall back to
workbook.worksheets[0]), then proceed to set sheet.title and sheet["A1"]; update
references to the symbols workbook.active, sheet.title, and sheet["A1"] so mypy
--strict no longer reports a possible None access.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#61
Summary by CodeRabbit
New Features
Enhancements
Documentation
Tests
Chores