feat: add include_cold param to list_notes tool #44

Merged

forgejo_admin merged 1 commit from 43-add-include-cold-param-to-list-notes into main

2026-03-17 03:37:13 +00:00

forgejo_admin commented

2026-03-17 03:32:08 +00:00

Owner

Summary

Add include_cold boolean parameter to the list_notes MCP tool so agents can retrieve completed/archived/deprecated notes when researching historical decisions or provenance. Defaults to false to keep token cost low by excluding cold notes.

Changes

src/pal_e_docs_mcp/tools/notes.py — Added include_cold: Annotated[bool, Field(...)] parameter after parent_slug, forwarded to get_sdk().list_notes(include_cold=...). Updated docstring to mention cold exclusion behavior.
tests/test_param_alignment.py — Added TestListNotesIncludeCold class with 4 tests: default false, true forwarded, false forwarded, and coexistence with other params.

Test Plan

pytest tests/ -x -q — 59 tests pass (4 new)
ruff format and ruff check clean

Review Checklist

Parameter follows existing Annotated[..., Field(...)] pattern
Description matches issue spec verbatim
Default is False — no behavior change for existing callers
Docstring updated to document cold exclusion
SDK dependency has include_cold on main (f56a903); published 0.4.0 will pass it as query param at runtime

Plan: plan-2026-03-16-knowledge-architecture
Closes #43

## Summary Add `include_cold` boolean parameter to the `list_notes` MCP tool so agents can retrieve completed/archived/deprecated notes when researching historical decisions or provenance. Defaults to `false` to keep token cost low by excluding cold notes. ## Changes - `src/pal_e_docs_mcp/tools/notes.py` — Added `include_cold: Annotated[bool, Field(...)]` parameter after `parent_slug`, forwarded to `get_sdk().list_notes(include_cold=...)`. Updated docstring to mention cold exclusion behavior. - `tests/test_param_alignment.py` — Added `TestListNotesIncludeCold` class with 4 tests: default false, true forwarded, false forwarded, and coexistence with other params. ## Test Plan - `pytest tests/ -x -q` — 59 tests pass (4 new) - `ruff format` and `ruff check` clean ## Review Checklist - [x] Parameter follows existing `Annotated[..., Field(...)]` pattern - [x] Description matches issue spec verbatim - [x] Default is `False` — no behavior change for existing callers - [x] Docstring updated to document cold exclusion - [x] SDK dependency has `include_cold` on main (`f56a903`); published 0.4.0 will pass it as query param at runtime ## Related - Plan: `plan-2026-03-16-knowledge-architecture` - Closes #43

forgejo_admin added 1 commit

2026-03-17 03:32:08 +00:00

feat: add include_cold param to list_notes MCP tool

ci/woodpecker/push/woodpecker Pipeline was successful

Details

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

563ae554ec

Expose include_cold boolean (default false) so agents can retrieve
completed/archived notes when researching historical decisions.
Parameter forwards directly to SDK list_notes(include_cold=...).

Closes #43

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

forgejo_admin commented

2026-03-17 03:32:31 +00:00

Author

Owner

Self-Review: LGTM

Correctness

Parameter type Annotated[bool, Field(...)] matches existing patterns (e.g., is_public in create_note)
Positioned after parent_slug per issue spec
Description text matches issue spec verbatim
Default False -- no behavior change for existing callers
Forwarded to SDK as include_cold=include_cold -- correct

Tests

4 tests in TestListNotesIncludeCold: default false, explicit true, explicit false, coexistence with other params
All use mock_sdk fixture correctly
Import added alphabetically

Lint/Test

ruff format + ruff check clean
59/59 tests pass (4 new)

No issues found. Ready for human review.

## Self-Review: LGTM **Correctness** - Parameter type `Annotated[bool, Field(...)]` matches existing patterns (e.g., `is_public` in `create_note`) - Positioned after `parent_slug` per issue spec - Description text matches issue spec verbatim - Default `False` -- no behavior change for existing callers - Forwarded to SDK as `include_cold=include_cold` -- correct **Tests** - 4 tests in `TestListNotesIncludeCold`: default false, explicit true, explicit false, coexistence with other params - All use `mock_sdk` fixture correctly - Import added alphabetically **Lint/Test** - `ruff format` + `ruff check` clean - 59/59 tests pass (4 new) No issues found. Ready for human review.

forgejo_admin commented

2026-03-17 03:33:44 +00:00

Author

Owner

PR #44 Review

DOMAIN REVIEW

Tech stack: Python / MCP tool layer (pydantic Field + Annotated pattern) bridging to pal-e-docs-sdk.

Parameter design: The include_cold parameter follows the established Annotated[type, Field(description=...)] pattern used by every other parameter in list_notes. Placement after parent_slug maintains the existing parameter ordering convention (required params first, optional filters after).

SDK alignment verified: The SDK's list_notes method at /home/ldraney/pal-e-docs-sdk/src/pal_e_docs_sdk/notes.py line 19 accepts include_cold: bool = False with identical default and semantics. The SDK only sends the query param when True (line 34-35), so the default False is a true no-op for existing callers.

Type correctness: bool with default False is correct. No CSV splitting or sentinel handling needed (unlike tags/labels params). The value is forwarded directly without transformation, which is the right approach.

Docstring: Updated to document cold exclusion behavior. The added line integrates cleanly with the existing docstring structure.

Test quality: 4 tests in TestListNotesIncludeCold covering:

Default value forwarding (False)
Explicit True forwarding
Explicit False forwarding
Coexistence with other parameters (tags + project + include_cold together)

Tests follow the exact same mock_sdk fixture pattern and call_args assertion style used throughout the test file. The coexistence test is particularly good -- it verifies include_cold does not interfere with other parameter forwarding.

BLOCKERS

None.

New functionality has test coverage (4 tests covering default, true, false, and param coexistence).
No user input validation risk (bool param with default; no injection surface).
No secrets or credentials.
No DRY violations (single forwarding path, no duplicated logic).

NITS

None. This is a minimal, well-scoped change. The description text, default value, parameter placement, and test coverage all follow established patterns precisely.

SOP COMPLIANCE

Branch named after issue: 43-add-include-cold-param-to-list-notes (references #43)
PR body has: Summary, Changes, Test Plan, Related sections
Related references plan slug: plan-2026-03-16-knowledge-architecture
Tests exist: 4 new tests, 59 total passing
No secrets committed
No unnecessary file changes (exactly 2 files, both directly relevant)
Commit message is descriptive (feat: add include_cold param to list_notes tool)

PROCESS OBSERVATIONS

Clean single-purpose feature addition. The SDK dependency is confirmed present (include_cold on SDK main at f56a903). The PR body correctly notes that SDK 0.4.0 will pass it as a query param at runtime -- this is good provenance documentation. Zero deployment risk since the default is False (backward compatible). Change failure risk: negligible.

VERDICT: APPROVED

## PR #44 Review ### DOMAIN REVIEW **Tech stack**: Python / MCP tool layer (pydantic `Field` + `Annotated` pattern) bridging to pal-e-docs-sdk. **Parameter design**: The `include_cold` parameter follows the established `Annotated[type, Field(description=...)]` pattern used by every other parameter in `list_notes`. Placement after `parent_slug` maintains the existing parameter ordering convention (required params first, optional filters after). **SDK alignment verified**: The SDK's `list_notes` method at `/home/ldraney/pal-e-docs-sdk/src/pal_e_docs_sdk/notes.py` line 19 accepts `include_cold: bool = False` with identical default and semantics. The SDK only sends the query param when `True` (line 34-35), so the default `False` is a true no-op for existing callers. **Type correctness**: `bool` with default `False` is correct. No CSV splitting or sentinel handling needed (unlike tags/labels params). The value is forwarded directly without transformation, which is the right approach. **Docstring**: Updated to document cold exclusion behavior. The added line integrates cleanly with the existing docstring structure. **Test quality**: 4 tests in `TestListNotesIncludeCold` covering: - Default value forwarding (False) - Explicit True forwarding - Explicit False forwarding - Coexistence with other parameters (tags + project + include_cold together) Tests follow the exact same `mock_sdk` fixture pattern and `call_args` assertion style used throughout the test file. The coexistence test is particularly good -- it verifies `include_cold` does not interfere with other parameter forwarding. ### BLOCKERS None. - New functionality has test coverage (4 tests covering default, true, false, and param coexistence). - No user input validation risk (bool param with default; no injection surface). - No secrets or credentials. - No DRY violations (single forwarding path, no duplicated logic). ### NITS None. This is a minimal, well-scoped change. The description text, default value, parameter placement, and test coverage all follow established patterns precisely. ### SOP COMPLIANCE - [x] Branch named after issue: `43-add-include-cold-param-to-list-notes` (references #43) - [x] PR body has: Summary, Changes, Test Plan, Related sections - [x] Related references plan slug: `plan-2026-03-16-knowledge-architecture` - [x] Tests exist: 4 new tests, 59 total passing - [x] No secrets committed - [x] No unnecessary file changes (exactly 2 files, both directly relevant) - [x] Commit message is descriptive (`feat: add include_cold param to list_notes tool`) ### PROCESS OBSERVATIONS Clean single-purpose feature addition. The SDK dependency is confirmed present (`include_cold` on SDK main at `f56a903`). The PR body correctly notes that SDK 0.4.0 will pass it as a query param at runtime -- this is good provenance documentation. Zero deployment risk since the default is `False` (backward compatible). Change failure risk: negligible. ### VERDICT: APPROVED