Issue #99 Phase 1: add exstruct.edit public edit API#102
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds a first-class public workbook editing API under Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant EditAPI as exstruct.edit.service
participant MCP as exstruct.mcp.patch_runner
participant Backend as PatchEngine (com/openpyxl)
Client->>EditAPI: call patch_workbook(PatchRequest)
EditAPI->>MCP: delegate to run_patch(request, policy=None)
MCP->>Backend: execute patch ops (apply ops, engine selection)
Backend-->>MCP: PatchResult (diffs, errors)
MCP-->>EditAPI: PatchResult
EditAPI-->>Client: PatchResult
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related issues
Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/mcp/test_server.py (1)
1033-1073: Consider extracting common fake functions into pytest fixtures.The fake function setup (fake_run_extract_tool, fake_run_read_json_chunk_tool, etc.) is repeated across many tests in this file. A shared fixture could reduce duplication and improve maintainability.
💡 Example fixture approach
`@pytest.fixture` def mocked_server_tools(monkeypatch: pytest.MonkeyPatch, tmp_path: Path): """Fixture providing mocked server tools with call tracking.""" calls: dict[str, tuple[object, ...]] = {} def fake_run_patch_tool( payload: PatchToolInput, *, policy: PathPolicy, on_conflict: OnConflictPolicy, ) -> PatchToolOutput: calls["patch"] = (payload, policy, on_conflict) return PatchToolOutput(out_path="out.xlsx", patch_diff=[], engine="openpyxl") # ... additional fake functions ... monkeypatch.setattr(server, "run_patch_tool", fake_run_patch_tool) # ... additional monkeypatches ... return calls🤖 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 1033 - 1073, The repeated fake tool functions (fake_run_extract_tool, fake_run_read_json_chunk_tool, fake_run_validate_input_tool, fake_run_patch_tool) and monkeypatch.setattr calls should be consolidated into a pytest fixture (e.g., mocked_server_tools) that registers those fakes and monkeypatches server.run_* and anyio.to_thread.run_sync once, returns a calls dict for assertions, and is used by tests instead of duplicating the setup; update tests to accept the fixture and remove the inline fake_* functions and monkeypatch.setattr calls so all mocks (including fake_run_patch_tool and run_sync replacement) are centralized and reusable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/mcp/test_server.py`:
- Around line 1033-1073: The repeated fake tool functions
(fake_run_extract_tool, fake_run_read_json_chunk_tool,
fake_run_validate_input_tool, fake_run_patch_tool) and monkeypatch.setattr calls
should be consolidated into a pytest fixture (e.g., mocked_server_tools) that
registers those fakes and monkeypatches server.run_* and
anyio.to_thread.run_sync once, returns a calls dict for assertions, and is used
by tests instead of duplicating the setup; update tests to accept the fixture
and remove the inline fake_* functions and monkeypatch.setattr calls so all
mocks (including fake_run_patch_tool and run_sync replacement) are centralized
and reusable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea8d930b-7a3d-4ac3-82c2-b2018849e8cb
📒 Files selected for processing (15)
dev-docs/adr/README.mddev-docs/adr/decision-map.mddev-docs/adr/index.yamldev-docs/architecture/overview.mddev-docs/specs/data-model.mdsrc/exstruct/edit/chart_types.pysrc/exstruct/edit/normalize.pysrc/exstruct/edit/op_schema.pysrc/exstruct/edit/specs.pysrc/exstruct/edit/types.pytasks/feature_spec.mdtasks/todo.mdtests/mcp/patch/test_normalize.pytests/mcp/test_server.pytests/mcp/test_tool_models.py
🚧 Files skipped from review as they are similar to previous changes (2)
- dev-docs/adr/README.md
- src/exstruct/edit/types.py
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
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.
Summary
exstruct.editas the Phase 1 public Python editing API for issue Promote Excel editing to first-class Python API and CLI, and reposition MCP as an integration layer #99.sheetfallback, and ADR-0006 status consistency.Acceptance Criteria
ValueErrorinstead of leakingAttributeError.sheetfallback applies to JSON-string ops as well as dict ops.PatchToolInput/MakeToolInputand an MCP server caller path are covered by regression tests for the JSON-string + top-levelsheetflow.exstruct.editmodules include the missing module docstrings added in this PR.Validation
uv run pytest tests/mcp/patch/test_normalize.py tests/mcp/test_tool_models.py tests/mcp/test_server.py -quv run task precommit-runRefs #99
Summary by CodeRabbit
Release Notes
New Features
exstruct.editwithpatch_workbook()andmake_workbook()functionsDocumentation