fix: send board item labels as string, not array

forgejo_admin commented

2026-03-16 03:04:38 +00:00

Owner

Summary

The pal-e-docs API expects labels as str | None (comma-separated string), but create_board_item and update_board_item were splitting the input string into a Python list before sending it in the JSON body. This caused the API to receive a JSON array ["type:bug", "scope:unplanned"] instead of the expected string "type:bug,scope:unplanned".

Changes

src/pal_e_docs_mcp/tools/boards.py -- Changed create_board_item and update_board_item to rejoin labels into a comma-separated string after stripping whitespace, instead of sending a list. The whitespace normalization and trailing-comma handling are preserved.
tests/test_param_alignment.py -- Updated existing assertions to expect strings instead of lists. Added new tests for whitespace stripping and trailing comma handling on both create_board_item and update_board_item.

Test Plan

All 55 unit tests pass (pytest tests/ -v)
ruff format and ruff check clean
New test cases cover: basic CSV string, whitespace stripping, trailing comma, empty string omission

Review Checklist

Code changes match the issue description
Tests updated to reflect new behavior (strings, not lists)
ruff format and ruff check pass
No unrelated changes included

Closes #41
Note: the pal-e-docs-sdk add_board_item method also types labels as list[str] | None which is inconsistent with the API schema -- that is a separate fix for pal-e-docs-sdk.

## Summary The pal-e-docs API expects `labels` as `str | None` (comma-separated string), but `create_board_item` and `update_board_item` were splitting the input string into a Python list before sending it in the JSON body. This caused the API to receive a JSON array `["type:bug", "scope:unplanned"]` instead of the expected string `"type:bug,scope:unplanned"`. ## Changes - `src/pal_e_docs_mcp/tools/boards.py` -- Changed `create_board_item` and `update_board_item` to rejoin labels into a comma-separated string after stripping whitespace, instead of sending a list. The whitespace normalization and trailing-comma handling are preserved. - `tests/test_param_alignment.py` -- Updated existing assertions to expect strings instead of lists. Added new tests for whitespace stripping and trailing comma handling on both `create_board_item` and `update_board_item`. ## Test Plan - All 55 unit tests pass (`pytest tests/ -v`) - `ruff format` and `ruff check` clean - New test cases cover: basic CSV string, whitespace stripping, trailing comma, empty string omission ## Review Checklist - [x] Code changes match the issue description - [x] Tests updated to reflect new behavior (strings, not lists) - [x] ruff format and ruff check pass - [x] No unrelated changes included ## Related - Closes #41 - Note: the pal-e-docs-sdk `add_board_item` method also types `labels` as `list[str] | None` which is inconsistent with the API schema -- that is a separate fix for pal-e-docs-sdk.

forgejo_admin added 1 commit

2026-03-16 03:04:38 +00:00

fix: send board item labels as comma-separated string instead of array

ci/woodpecker/push/woodpecker Pipeline was successful

Details

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

10b2c8eccc

The pal-e-docs API expects labels as `str | None` (comma-separated),
but create_board_item and update_board_item were splitting the string
into a Python list before sending it in the JSON body, causing the API
to receive a JSON array instead of a string.

Now both tools normalize whitespace and trailing commas but keep
labels as a single comma-separated string matching the API schema.

Closes #41

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

forgejo_admin commented

2026-03-16 03:05:14 +00:00

Author

Owner

Self-Review

Reviewed the diff. No issues found.

Fix is minimal and targeted: list -> ",".join(...) in both create_board_item and update_board_item
All 55 tests pass, including 4 new edge case tests (whitespace stripping, trailing comma for both tools)
ruff format and ruff check clean
Edge case noted: update_board_item with labels="," would send "" (empty string) rather than omitting the field. This matches pre-existing behavior for create_board_item (which also sends falsy values through). Not a regression and not in scope for this issue.
Discovered scope: pal-e-docs-sdk add_board_item also types labels as list[str] | None instead of str | None -- separate fix needed there.

## Self-Review Reviewed the diff. No issues found. - Fix is minimal and targeted: `list` -> `",".join(...)` in both `create_board_item` and `update_board_item` - All 55 tests pass, including 4 new edge case tests (whitespace stripping, trailing comma for both tools) - ruff format and ruff check clean - Edge case noted: `update_board_item` with `labels=","` would send `""` (empty string) rather than omitting the field. This matches pre-existing behavior for `create_board_item` (which also sends falsy values through). Not a regression and not in scope for this issue. - Discovered scope: pal-e-docs-sdk `add_board_item` also types `labels` as `list[str] | None` instead of `str | None` -- separate fix needed there.

forgejo_admin commented

2026-03-16 03:07:20 +00:00

Author

Owner

PR #42 Review

DOMAIN REVIEW

Tech stack: Python / MCP server wrapping pal-e-docs REST API via SDK HTTP helpers. Test framework: pytest with monkeypatched mock SDK.

Bug analysis: Verified the root cause. The pal-e-docs API defines labels as str | None at every layer:

SQLAlchemy model: labels: Mapped[str | None] = mapped_column(Text, nullable=True) (/home/ldraney/pal-e-docs/src/pal_e_docs/models.py:272)
Pydantic schemas: labels: str | None = None (/home/ldraney/pal-e-docs/src/pal_e_docs/schemas.py:252,288,296)

The old code was calling .split(",") to produce a Python list, then sending that list as JSON (an array). The fix correctly changes to .join() after stripping, preserving the whitespace normalization and trailing-comma handling while sending the API-expected string format.

Code correctness:

create_board_item (line 186): labels_str = ",".join(lbl.strip() for lbl in labels.split(",") if lbl.strip()) if labels else None -- correct. The if labels guard handles both None and "" (empty string is falsy), producing None which is then correctly excluded from the body by the if labels_str is not None check. This matches the existing test_labels_empty_string_is_no_labels test.
update_board_item (line 242): Same pattern inside if labels is not None: guard -- correct. The if lbl.strip() filter handles trailing commas.

Both functions use the same normalization logic (DRY observation): The expression ",".join(lbl.strip() for lbl in labels.split(",") if lbl.strip()) appears in both create_board_item and update_board_item. This is a minor duplication -- a small _normalize_labels(labels: str) -> str helper would reduce it. However, this is a 1-liner repeated twice in the same file, and extracting it would be scope creep for a bugfix PR. Flagging as a nit only.

Test coverage:

Existing tests updated: test_labels_csv_basic and test_labels_csv_as_string now assert string output instead of list output.
New tests added (4 total):
- TestCreateBoardItem.test_labels_whitespace_stripped -- whitespace normalization
- TestUpdateBoardItem.test_labels_whitespace_stripped -- whitespace normalization
- TestUpdateBoardItem.test_labels_trailing_comma -- trailing comma handling
- (Existing test_labels_trailing_comma on create was already present and updated)
Edge cases covered: empty string, trailing comma, whitespace, basic CSV, None (via omission).
All 55 tests reported passing per PR body.

PEP compliance: Type hints present. Docstrings present on tool functions. Generator expression in .join() is idiomatic Python. Variable renamed from labels_list to labels_str to accurately reflect the type -- good.

SDK inconsistency noted: The PR body correctly identifies that pal-e-docs-sdk types labels as list[str] | None in add_board_item (/home/ldraney/pal-e-docs-sdk/src/pal_e_docs_sdk/boards.py:105), which is inconsistent with the API schema. This is correctly scoped out as a separate fix. The MCP tools bypass the SDK board methods (using raw .post() / .patch()), so this SDK inconsistency does not affect this PR.

BLOCKERS

None.

New functionality has test coverage (4 new test cases + 2 updated assertions).
No unvalidated user input risk (labels come from MCP tool parameters, not raw HTTP).
No secrets or credentials in code.
No DRY violation in auth/security paths.

NITS

Minor DRY: The labels normalization expression ",".join(lbl.strip() for lbl in labels.split(",") if lbl.strip()) is duplicated across create_board_item (line 186) and update_board_item (line 242). A small _normalize_labels(labels: str) -> str helper would reduce this to a single definition. Non-blocking -- the duplication is in a data formatting path, not an auth/security path.

SOP COMPLIANCE

Branch named after issue (41-fix-labels-array-to-string references #41)
PR body has: Summary, Changes, Test Plan, Related
Related section references Closes #41
Related section notes the SDK inconsistency as a separate fix (good scoping)
Tests exist and cover happy path + edge cases
No secrets, .env files, or credentials committed
No unnecessary file changes (2 files changed, both directly relevant)
Commit message is descriptive (fix: send board item labels as string, not array)
Related section does not reference a plan slug (should reference plan-pal-e-docs per SOP)

PROCESS OBSERVATIONS

Change failure risk: LOW. This is a 2-file, 30-addition/9-deletion bugfix with a clear root cause and verification against the upstream API schema. The fix aligns the MCP layer with the API contract.
Deployment frequency: POSITIVE. Small, focused bugfix PR -- exactly the kind of change that supports high deployment frequency.
Downstream impact: The SDK (pal-e-docs-sdk) has the same type mismatch (list[str] | None vs API's str | None). The PR correctly scopes this out but it should be tracked as a follow-up issue on pal-e-docs-sdk.
Missing plan slug in Related section is a minor SOP gap, not a blocker.

VERDICT: APPROVED

## PR #42 Review ### DOMAIN REVIEW **Tech stack:** Python / MCP server wrapping pal-e-docs REST API via SDK HTTP helpers. Test framework: pytest with monkeypatched mock SDK. **Bug analysis:** Verified the root cause. The pal-e-docs API defines `labels` as `str | None` at every layer: - SQLAlchemy model: `labels: Mapped[str | None] = mapped_column(Text, nullable=True)` (`/home/ldraney/pal-e-docs/src/pal_e_docs/models.py:272`) - Pydantic schemas: `labels: str | None = None` (`/home/ldraney/pal-e-docs/src/pal_e_docs/schemas.py:252,288,296`) The old code was calling `.split(",")` to produce a Python list, then sending that list as JSON (an array). The fix correctly changes to `.join()` after stripping, preserving the whitespace normalization and trailing-comma handling while sending the API-expected string format. **Code correctness:** - `create_board_item` (line 186): `labels_str = ",".join(lbl.strip() for lbl in labels.split(",") if lbl.strip()) if labels else None` -- correct. The `if labels` guard handles both `None` and `""` (empty string is falsy), producing `None` which is then correctly excluded from the body by the `if labels_str is not None` check. This matches the existing `test_labels_empty_string_is_no_labels` test. - `update_board_item` (line 242): Same pattern inside `if labels is not None:` guard -- correct. The `if lbl.strip()` filter handles trailing commas. **Both functions use the same normalization logic (DRY observation):** The expression `",".join(lbl.strip() for lbl in labels.split(",") if lbl.strip())` appears in both `create_board_item` and `update_board_item`. This is a minor duplication -- a small `_normalize_labels(labels: str) -> str` helper would reduce it. However, this is a 1-liner repeated twice in the same file, and extracting it would be scope creep for a bugfix PR. Flagging as a nit only. **Test coverage:** - Existing tests updated: `test_labels_csv_basic` and `test_labels_csv_as_string` now assert string output instead of list output. - New tests added (4 total): - `TestCreateBoardItem.test_labels_whitespace_stripped` -- whitespace normalization - `TestUpdateBoardItem.test_labels_whitespace_stripped` -- whitespace normalization - `TestUpdateBoardItem.test_labels_trailing_comma` -- trailing comma handling - (Existing `test_labels_trailing_comma` on create was already present and updated) - Edge cases covered: empty string, trailing comma, whitespace, basic CSV, `None` (via omission). - All 55 tests reported passing per PR body. **PEP compliance:** Type hints present. Docstrings present on tool functions. Generator expression in `.join()` is idiomatic Python. Variable renamed from `labels_list` to `labels_str` to accurately reflect the type -- good. **SDK inconsistency noted:** The PR body correctly identifies that `pal-e-docs-sdk` types `labels` as `list[str] | None` in `add_board_item` (`/home/ldraney/pal-e-docs-sdk/src/pal_e_docs_sdk/boards.py:105`), which is inconsistent with the API schema. This is correctly scoped out as a separate fix. The MCP tools bypass the SDK board methods (using raw `.post()` / `.patch()`), so this SDK inconsistency does not affect this PR. ### BLOCKERS None. - New functionality has test coverage (4 new test cases + 2 updated assertions). - No unvalidated user input risk (labels come from MCP tool parameters, not raw HTTP). - No secrets or credentials in code. - No DRY violation in auth/security paths. ### NITS 1. **Minor DRY:** The labels normalization expression `",".join(lbl.strip() for lbl in labels.split(",") if lbl.strip())` is duplicated across `create_board_item` (line 186) and `update_board_item` (line 242). A small `_normalize_labels(labels: str) -> str` helper would reduce this to a single definition. Non-blocking -- the duplication is in a data formatting path, not an auth/security path. ### SOP COMPLIANCE - [x] Branch named after issue (`41-fix-labels-array-to-string` references #41) - [x] PR body has: Summary, Changes, Test Plan, Related - [x] Related section references `Closes #41` - [x] Related section notes the SDK inconsistency as a separate fix (good scoping) - [x] Tests exist and cover happy path + edge cases - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes (2 files changed, both directly relevant) - [x] Commit message is descriptive (`fix: send board item labels as string, not array`) - [ ] Related section does not reference a plan slug (should reference `plan-pal-e-docs` per SOP) ### PROCESS OBSERVATIONS - **Change failure risk: LOW.** This is a 2-file, 30-addition/9-deletion bugfix with a clear root cause and verification against the upstream API schema. The fix aligns the MCP layer with the API contract. - **Deployment frequency: POSITIVE.** Small, focused bugfix PR -- exactly the kind of change that supports high deployment frequency. - **Downstream impact:** The SDK (`pal-e-docs-sdk`) has the same type mismatch (`list[str] | None` vs API's `str | None`). The PR correctly scopes this out but it should be tracked as a follow-up issue on `pal-e-docs-sdk`. - **Missing plan slug in Related section** is a minor SOP gap, not a blocker. ### VERDICT: APPROVED