Phase 8f-3 + 8f-4: Param alignment audit + QA nits (#24) #25

Merged

forgejo_admin merged 1 commit from 24-param-alignment-qa-nits into main

2026-03-08 00:37:19 +00:00

forgejo_admin commented

2026-03-08 00:31:05 +00:00

Owner

Summary

Audit and harden all MCP-to-SDK parameter translations introduced in PR #23 (8f-2 rewrite). Fix edge case #12 (create_note tags=None), harden CSV splitting against trailing commas and whitespace, resolve E741 lint warnings, and add .claude/ to .gitignore.

Changes

src/pal_e_docs_mcp/tools/notes.py — create_note now passes tags=[] explicitly when tags param is None (matching old MCP behavior); added if t.strip() filter to all CSV splits to handle trailing commas
src/pal_e_docs_mcp/tools/sprints.py — renamed l to lbl in 2 CSV comprehensions (fixes E741), removed # noqa: E741 comments, added if lbl.strip() filter, added docstring on move_sprint_item documenting SDK update_sprint_item name mapping
src/pal_e_docs_mcp/tools/links.py — added if s.strip() filter to update_note_links CSV split
.gitignore — added .claude/ to prevent worktree artifacts from being staged
tests/conftest.py — new shared mock_sdk fixture for patching get_sdk across tool modules
tests/test_param_alignment.py — 25 unit tests covering all 8 documented param translations, CSV edge cases, sentinel handling, field renames, and defaults

Test Plan

pytest tests/ -v — 25/25 passed
ruff check — no errors
ruff format --check — all files formatted
Edge case #12 verified: create_note(tags=None) sends [] to SDK
Edge case #13 verified: add_sprint_item(labels="") treated as no labels
CSV trailing comma: "sop,active," produces ["sop", "active"]
CSV whitespace: " sop , active " produces ["sop", "active"]

Review Checklist

No unrelated changes
Backward compatible — no MCP tool param names or required/optional changes
Lint clean (ruff check + ruff format --check)
All 25 tests pass

Plan: plan-2026-02-26-tf-modularize-postgres
Forgejo issue: #24
Parent: PR #23 (8f-2 rewrite)

## Summary Audit and harden all MCP-to-SDK parameter translations introduced in PR #23 (8f-2 rewrite). Fix edge case #12 (create_note tags=None), harden CSV splitting against trailing commas and whitespace, resolve E741 lint warnings, and add .claude/ to .gitignore. ## Changes - **`src/pal_e_docs_mcp/tools/notes.py`** — create_note now passes `tags=[]` explicitly when tags param is None (matching old MCP behavior); added `if t.strip()` filter to all CSV splits to handle trailing commas - **`src/pal_e_docs_mcp/tools/sprints.py`** — renamed `l` to `lbl` in 2 CSV comprehensions (fixes E741), removed `# noqa: E741` comments, added `if lbl.strip()` filter, added docstring on `move_sprint_item` documenting SDK `update_sprint_item` name mapping - **`src/pal_e_docs_mcp/tools/links.py`** — added `if s.strip()` filter to update_note_links CSV split - **`.gitignore`** — added `.claude/` to prevent worktree artifacts from being staged - **`tests/conftest.py`** — new shared `mock_sdk` fixture for patching get_sdk across tool modules - **`tests/test_param_alignment.py`** — 25 unit tests covering all 8 documented param translations, CSV edge cases, sentinel handling, field renames, and defaults ## Test Plan - [x] `pytest tests/ -v` — 25/25 passed - [x] `ruff check` — no errors - [x] `ruff format --check` — all files formatted - [x] Edge case #12 verified: create_note(tags=None) sends `[]` to SDK - [x] Edge case #13 verified: add_sprint_item(labels="") treated as no labels - [x] CSV trailing comma: `"sop,active,"` produces `["sop", "active"]` - [x] CSV whitespace: `" sop , active "` produces `["sop", "active"]` ## Review Checklist - [x] No unrelated changes - [x] Backward compatible — no MCP tool param names or required/optional changes - [x] Lint clean (`ruff check` + `ruff format --check`) - [x] All 25 tests pass ## Related - Plan: `plan-2026-02-26-tf-modularize-postgres` - Forgejo issue: #24 - Parent: PR #23 (8f-2 rewrite)

forgejo_admin added 1 commit

2026-03-08 00:31:05 +00:00

Phase 8f-3 + 8f-4: Param alignment audit + QA nits cleanup

ci/woodpecker/push/woodpecker Pipeline was successful

Details

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

007b8edd04

Fix edge case #12: create_note with tags=None now explicitly passes []
to the SDK (matching old MCP behavior) instead of relying on SDK's
internal `tags or []` fallback. Harden all CSV splitting to filter
empty strings from trailing commas and strip whitespace consistently
across notes, sprints, and links tools.

8f-4 nits: rename `l` -> `lbl` loop variable in sprints.py to fix E741
ambiguous variable name lint, remove noqa comments, add .claude/ to
.gitignore, add docstring documenting move_sprint_item -> SDK
update_sprint_item name mapping.

25 unit tests covering all 8 documented param translations, CSV edge
cases (trailing comma, whitespace, single value), sentinel handling,
field renames, and default values.

Closes #24

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

forgejo_admin commented

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

Author

Owner

PR #25 Review

BLOCKERS

None.

NITS

conftest.py does not patch blocks, projects, repos, or tags modules. The mock_sdk fixture only patches notes, sprints, and links -- which is correct for the current test file. However, if future test files use mock_sdk expecting it to cover all tool modules, they will hit real get_sdk() calls. Consider a comment in conftest noting which modules are patched and why, or patching all tool modules preemptively.
list_notes and search_notes pass tags as a raw CSV string to the SDK (no split). This is correct per the SDK signatures (tags: str | None), but it means trailing-comma and whitespace hardening does NOT apply to those two tools. If a user calls list_notes(tags="sop,active,"), the trailing comma goes straight to the API. This is outside the scope of this PR (it was not in the 8f-3 audit list, and the API likely handles it), but worth noting for a future hardening pass.
Minor: move_sprint_item labels with empty string. When labels="" (not None), the code enters the split path and produces [] (all elements filtered). The _UNSET sentinel is only used when labels is None. Passing labels=[] to update_sprint_item would clear all labels rather than leaving them unchanged. This edge case is not tested. If the intent is that labels="" should clear labels, that is fine; if it should be a no-op, it needs different handling. Low risk since MCP callers typically omit the param rather than passing empty string.

SOP COMPLIANCE

Branch named after issue -- 24-param-alignment-qa-nits references issue #24
PR body follows template -- Summary, Changes, Test Plan, Review Checklist, Related all present
Related references plan slug -- plan-2026-02-26-tf-modularize-postgres referenced
No secrets committed -- .gitignore adds .claude/, no .env or credentials in diff
No scope creep -- 6 files changed, all directly related to the 8f-3/8f-4 acceptance criteria
Tests exist -- 25 tests in test_param_alignment.py covering all 8 documented param translations plus CSV edge cases

CODE QUALITY

Edge case #12 (tags=None): Correctly changed from None to [] in create_note. The if tags else [] pattern handles both None and "". Correct.
CSV hardening: if t.strip() / if s.strip() / if lbl.strip() applied consistently across all 5 CSV split sites in notes.py, sprints.py, and links.py. No other tool modules have CSV splits. Complete coverage.
E741 fix: l renamed to lbl in both add_sprint_item and move_sprint_item. # noqa: E741 comments removed. Clean.
Sentinel handling: update_sprint and move_sprint_item correctly use _UNSET from SDK for optional fields. Tests verify this.
Name mapping docstring: move_sprint_item now documents the SDK update_sprint_item mapping. Good.
conftest.py mock_sdk fixture: Clean shared fixture using monkeypatch.setattr. Correctly patches the module-level get_sdk reference in each tool module.
Backward compatibility: No MCP tool param names changed. No required/optional changes. Error format unchanged.

VERDICT: APPROVED

## PR #25 Review ### BLOCKERS None. ### NITS 1. **`conftest.py` does not patch blocks, projects, repos, or tags modules.** The `mock_sdk` fixture only patches `notes`, `sprints`, and `links` -- which is correct for the current test file. However, if future test files use `mock_sdk` expecting it to cover all tool modules, they will hit real `get_sdk()` calls. Consider a comment in conftest noting which modules are patched and why, or patching all tool modules preemptively. 2. **`list_notes` and `search_notes` pass `tags` as a raw CSV string to the SDK** (no split). This is correct per the SDK signatures (`tags: str | None`), but it means trailing-comma and whitespace hardening does NOT apply to those two tools. If a user calls `list_notes(tags="sop,active,")`, the trailing comma goes straight to the API. This is outside the scope of this PR (it was not in the 8f-3 audit list, and the API likely handles it), but worth noting for a future hardening pass. 3. **Minor: `move_sprint_item` labels with empty string.** When `labels=""` (not None), the code enters the split path and produces `[]` (all elements filtered). The `_UNSET` sentinel is only used when `labels is None`. Passing `labels=[]` to `update_sprint_item` would clear all labels rather than leaving them unchanged. This edge case is not tested. If the intent is that `labels=""` should clear labels, that is fine; if it should be a no-op, it needs different handling. Low risk since MCP callers typically omit the param rather than passing empty string. ### SOP COMPLIANCE - [x] **Branch named after issue** -- `24-param-alignment-qa-nits` references issue #24 - [x] **PR body follows template** -- Summary, Changes, Test Plan, Review Checklist, Related all present - [x] **Related references plan slug** -- `plan-2026-02-26-tf-modularize-postgres` referenced - [x] **No secrets committed** -- `.gitignore` adds `.claude/`, no `.env` or credentials in diff - [x] **No scope creep** -- 6 files changed, all directly related to the 8f-3/8f-4 acceptance criteria - [x] **Tests exist** -- 25 tests in `test_param_alignment.py` covering all 8 documented param translations plus CSV edge cases ### CODE QUALITY - **Edge case #12 (tags=None):** Correctly changed from `None` to `[]` in `create_note`. The `if tags else []` pattern handles both `None` and `""`. Correct. - **CSV hardening:** `if t.strip()` / `if s.strip()` / `if lbl.strip()` applied consistently across all 5 CSV split sites in notes.py, sprints.py, and links.py. No other tool modules have CSV splits. Complete coverage. - **E741 fix:** `l` renamed to `lbl` in both `add_sprint_item` and `move_sprint_item`. `# noqa: E741` comments removed. Clean. - **Sentinel handling:** `update_sprint` and `move_sprint_item` correctly use `_UNSET` from SDK for optional fields. Tests verify this. - **Name mapping docstring:** `move_sprint_item` now documents the SDK `update_sprint_item` mapping. Good. - **`conftest.py` mock_sdk fixture:** Clean shared fixture using `monkeypatch.setattr`. Correctly patches the module-level `get_sdk` reference in each tool module. - **Backward compatibility:** No MCP tool param names changed. No required/optional changes. Error format unchanged. ### VERDICT: APPROVED