Fix create_block and update_block content parameter type mismatch #26

New issue

Closed

opened 2026-03-09 03:29:33 +00:00 by forgejo_admin · 1 comment

forgejo_admin commented

2026-03-09 03:29:33 +00:00

Owner

Lineage

todo-mcp-block-content-type — discovered during Epilogue item 9 work

Repo

forgejo_admin/pal-e-docs-mcp

User Story

As an agent using block tools
I want create_block and update_block to accept structured content
So that I can create and edit blocks without falling back to curl

Context

The content parameter on create_block and update_block MCP tools is typed as str in the Pydantic schema. The pal-e-docs REST API expects content as a JSON dict:

Headings: {"level": 4, "text": "..."}
Paragraphs: {"html": "..."}
Lists: {"ordered": false, "items": [...]}
Tables: {"headers": [...], "rows": [...]}

This creates a catch-22 — dict input is rejected by the tool ("Input should be a valid string"), string input is rejected by the API ("Input should be a valid dictionary"). Agents must use curl as a workaround.

File Targets

Files the agent should modify:

The MCP tool definitions for create_block and update_block — change content param type from str to dict
Any Pydantic model/schema that defines the tool input

Files the agent should NOT touch:

pal-e-docs app code — the API is correct, this is MCP-only

Acceptance Criteria

create_block accepts a dict for content and forwards it correctly to the API
update_block accepts a dict for content and forwards it correctly to the API
Creating a heading block with {"level": 4, "text": "Test"} works via MCP
Creating a paragraph block with {"html": "test"} works via MCP
Existing string-based content (if any callers depend on it) still works or is migrated

Test Expectations

Manual test: call create_block via MCP with dict content, verify block created
Manual test: call update_block via MCP with dict content, verify block updated
Run command: pytest tests/ -v (if tests exist)

Constraints

Match the pal-e-docs API contract exactly — don't invent a new schema
The MCP tool parameter schema must be valid JSON Schema (Claude reads it to know how to call the tool)

Checklist

PR opened
Tests pass
No unrelated changes

todo-mcp-block-content-type — tracking TODO
PR #120 on pal-e-docs — the anchor_id fix that exposed this bug

### Lineage `todo-mcp-block-content-type` — discovered during Epilogue item 9 work ### Repo `forgejo_admin/pal-e-docs-mcp` ### User Story As an agent using block tools I want `create_block` and `update_block` to accept structured content So that I can create and edit blocks without falling back to curl ### Context The `content` parameter on `create_block` and `update_block` MCP tools is typed as `str` in the Pydantic schema. The pal-e-docs REST API expects `content` as a JSON dict: - Headings: `{"level": 4, "text": "..."}` - Paragraphs: `{"html": "..."}` - Lists: `{"ordered": false, "items": [...]}` - Tables: `{"headers": [...], "rows": [...]}` This creates a catch-22 — dict input is rejected by the tool ("Input should be a valid string"), string input is rejected by the API ("Input should be a valid dictionary"). Agents must use curl as a workaround. ### File Targets Files the agent should modify: - The MCP tool definitions for `create_block` and `update_block` — change `content` param type from `str` to `dict` - Any Pydantic model/schema that defines the tool input Files the agent should NOT touch: - pal-e-docs app code — the API is correct, this is MCP-only ### Acceptance Criteria - [ ] `create_block` accepts a dict for `content` and forwards it correctly to the API - [ ] `update_block` accepts a dict for `content` and forwards it correctly to the API - [ ] Creating a heading block with `{"level": 4, "text": "Test"}` works via MCP - [ ] Creating a paragraph block with `{"html": "test"}` works via MCP - [ ] Existing string-based content (if any callers depend on it) still works or is migrated ### Test Expectations - [ ] Manual test: call `create_block` via MCP with dict content, verify block created - [ ] Manual test: call `update_block` via MCP with dict content, verify block updated - Run command: `pytest tests/ -v` (if tests exist) ### Constraints - Match the pal-e-docs API contract exactly — don't invent a new schema - The MCP tool parameter schema must be valid JSON Schema (Claude reads it to know how to call the tool) ### Checklist - [ ] PR opened - [ ] Tests pass - [ ] No unrelated changes ### Related - `todo-mcp-block-content-type` — tracking TODO - PR #120 on pal-e-docs — the anchor_id fix that exposed this bug

forgejo_admin referenced this issue from a commit

2026-03-09 03:35:15 +00:00

Fix block content param type: str -> dict|str with JSON parsing

forgejo_admin referenced this issue

2026-03-09 03:35:48 +00:00

Fix block content param type: str -> dict|str with JSON parsing #27

forgejo_admin commented

2026-03-09 03:37:47 +00:00

Author

Owner

PR #27 Review

BLOCKERS

None.

NITS

pytest imported inside test methods -- TestParseContent.test_invalid_json_raises_value_error, test_json_array_raises_value_error, and test_json_scalar_raises_value_error each do import pytest inside the method body. The module already has from __future__ import annotations at the top; adding import pytest alongside the other top-level imports would be cleaner and consistent with the rest of the test file (which uses mock_sdk fixture without local imports). Non-blocking since it works fine either way.
Truncated branch name -- Branch is 26-fix-create-block-and-update-block-conten (missing trailing 't'). Likely auto-generated by Forgejo from the issue title. No action needed, just noting it.

SOP COMPLIANCE

Branch named after issue number (26-...)
PR body has: Summary, Changes, Test Plan, Related
Related section references the plan slug (todo-mcp-block-content-type) and issue #26
Tests exist -- 14 new tests covering all paths (dict, JSON string, None, invalid JSON, array, scalar)
No secrets, .env files, or credentials committed
No unnecessary file changes -- all 3 files directly related to the fix
Commit messages are descriptive (PR title is clear)

CODE REVIEW

_parse_content helper -- Correct implementation. Handles all three input types cleanly:

None and dict pass through immediately (line 39)
String inputs go through json.loads with proper error handling for both JSONDecodeError/TypeError (invalid JSON) and non-dict parsed results (arrays, scalars)
Both error paths produce descriptive ValueError messages with the specific type or parse error

Integration into create_block and update_block -- Both tools call _parse_content(content) inside their try block, before the SDK call. The ValueError raised by _parse_content is caught by the broad except Exception handler and formatted via _error_response, which correctly produces {"error": true, "detail": "..."} for non-PalEDocsError exceptions. This means invalid content returns a user-friendly error without crashing.

_CONTENT_DESC field description -- Documents all 5 content shapes (heading, paragraph, list, table, code) with example JSON structures. Uses JSON-style false in the description, which is correct since the description shows JSON object shapes to LLM consumers.

conftest.py change -- Correctly patches get_sdk in the blocks module namespace, matching the existing pattern for notes, sprints, and links modules.

Test coverage -- Thorough:

6 unit tests for _parse_content (None, dict identity, valid JSON string, invalid JSON, JSON array, JSON scalar)
4 integration tests each for create_block and update_block (dict forwarded, JSON string parsed, None forwarded, invalid JSON returns error)
Error path tests verify both the ValueError raise (unit) and the _error_response formatting (integration)

Type annotation -- The # type: ignore[return-value] on line 40 is justified; it suppresses a mypy narrowing limitation where isinstance(content, dict) narrows to dict but not dict[str, Any].

No security concerns. No scope creep. Clean fix.

VERDICT: APPROVED

## PR #27 Review ### BLOCKERS None. ### NITS 1. **`pytest` imported inside test methods** -- `TestParseContent.test_invalid_json_raises_value_error`, `test_json_array_raises_value_error`, and `test_json_scalar_raises_value_error` each do `import pytest` inside the method body. The module already has `from __future__ import annotations` at the top; adding `import pytest` alongside the other top-level imports would be cleaner and consistent with the rest of the test file (which uses `mock_sdk` fixture without local imports). Non-blocking since it works fine either way. 2. **Truncated branch name** -- Branch is `26-fix-create-block-and-update-block-conten` (missing trailing 't'). Likely auto-generated by Forgejo from the issue title. No action needed, just noting it. ### SOP COMPLIANCE - [x] Branch named after issue number (`26-...`) - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Related section references the plan slug (`todo-mcp-block-content-type`) and issue #26 - [x] Tests exist -- 14 new tests covering all paths (dict, JSON string, None, invalid JSON, array, scalar) - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes -- all 3 files directly related to the fix - [x] Commit messages are descriptive (PR title is clear) ### CODE REVIEW **`_parse_content` helper** -- Correct implementation. Handles all three input types cleanly: - `None` and `dict` pass through immediately (line 39) - String inputs go through `json.loads` with proper error handling for both `JSONDecodeError`/`TypeError` (invalid JSON) and non-dict parsed results (arrays, scalars) - Both error paths produce descriptive `ValueError` messages with the specific type or parse error **Integration into `create_block` and `update_block`** -- Both tools call `_parse_content(content)` inside their `try` block, before the SDK call. The `ValueError` raised by `_parse_content` is caught by the broad `except Exception` handler and formatted via `_error_response`, which correctly produces `{"error": true, "detail": "..."}` for non-`PalEDocsError` exceptions. This means invalid content returns a user-friendly error without crashing. **`_CONTENT_DESC` field description** -- Documents all 5 content shapes (heading, paragraph, list, table, code) with example JSON structures. Uses JSON-style `false` in the description, which is correct since the description shows JSON object shapes to LLM consumers. **`conftest.py` change** -- Correctly patches `get_sdk` in the blocks module namespace, matching the existing pattern for notes, sprints, and links modules. **Test coverage** -- Thorough: - 6 unit tests for `_parse_content` (None, dict identity, valid JSON string, invalid JSON, JSON array, JSON scalar) - 4 integration tests each for `create_block` and `update_block` (dict forwarded, JSON string parsed, None forwarded, invalid JSON returns error) - Error path tests verify both the `ValueError` raise (unit) and the `_error_response` formatting (integration) **Type annotation** -- The `# type: ignore[return-value]` on line 40 is justified; it suppresses a mypy narrowing limitation where `isinstance(content, dict)` narrows to `dict` but not `dict[str, Any]`. No security concerns. No scope creep. Clean fix. ### VERDICT: APPROVED

forgejo_admin closed this issue

2026-03-09 03:40:01 +00:00