feat: add include_cold param to list_notes for knowledge tiering

forgejo_admin commented

2026-03-17 03:14:46 +00:00

Author

Owner

PR #191 Review

DOMAIN REVIEW

Tech stack: Python / FastAPI / SQLAlchemy (ORM queries) / pytest with TestClient

Route logic (src/pal_e_docs/routes/notes.py)

COLD_STATUSES is declared as a module-level frozenset[str] -- immutable, type-annotated, well-documented. Good.
The include_cold Query parameter defaults to False with a clear description string. Placement before limit/offset in the parameter list is consistent with filter params above.
The filter logic at line 412-413 is correct and clean:
```
if not include_cold and status is None:
    q = q.filter(or_(Note.status.notin_(COLD_STATUSES), Note.status.is_(None)))
```
- When status is explicitly provided, the cold filter is skipped entirely -- this is the right approach because the caller is explicitly asking for that status.
- Note.status.is_(None) correctly handles NULL status values so notes without a status are never accidentally excluded.
- The or_ import was added to the existing sqlalchemy import line. Clean diff.
The filter is placed AFTER the parent_slug filter and BEFORE the ordering logic. This is the correct position -- it does not interfere with parent resolution or ordering.
Search endpoints (/notes/search, /notes/semantic-search) are correctly NOT affected. They use raw SQL via _build_filter_clauses / text() and have their own status parameter. The cold exclusion is scoped only to the ORM-based list_notes.

COLD_STATUSES consistency check against VALID_STATUSES:

completed: appears in plan, phase, sprint, milestone -- correct to be cold.
done: appears in todo -- correct to be cold.
deprecated: appears in sop, convention, template, skill, agent -- correct to be cold.
deferred: appears in plan, phase -- correct to be cold.
archived: appears in project-page, doc, sprint, post -- correct to be cold.
resolved (incident): arguably cold, but incidents are a different lifecycle. Reasonable to leave out for now.
published (journal, post): active content, NOT cold. Correct exclusion from the set.

All five cold statuses are terminal/inactive states. The set is well-chosen.

Tests (tests/test_include_cold.py)

6 tests covering:

Default exclusion of cold notes -- verifies all 5 hot + NULL-status notes appear, all 5 cold notes excluded.
include_cold=true returns all 10 notes.
status=completed overrides cold exclusion -- returns only completed notes.
status=done overrides cold exclusion -- returns only done notes.
project=agency combined with cold exclusion -- verifies project filter and cold filter compose correctly.
project=agency&include_cold=true -- verifies project filter with cold inclusion returns all 9 project notes.

Tests use the shared _seed_notes helper which creates a realistic mix of hot/cold/NULL-status notes across a project. The helper asserts creation success, which is good defensive testing.

The client fixture uses autouse=True setup_db which drops/recreates all tables between tests, so test isolation is solid.

PEP compliance:

Type hints present (frozenset[str], bool, str | None).
Docstrings on the helper function.
Module docstring on the test file.
Ruff format/check reported clean in PR body.

BLOCKERS

None.

Test coverage is present and thorough (6 tests, happy path + edge cases + composition).
No unvalidated user input -- include_cold is a bool Query param, FastAPI validates it automatically.
No secrets or credentials in code.
No DRY violations in auth/security paths.

NITS

Missing note_type + cold exclusion test: When note_type=phase is specified but status is NOT, the cold filter still applies (because status is None in the condition). This means GET /notes?note_type=phase would return only non-cold phases. This is correct behavior, but there is no test covering note_type filter combined with cold exclusion. Consider adding one in a follow-up.
_seed_notes docstring says "Returns a dict" but the function returns None. The docstring is inaccurate -- it is a void helper. Minor copy-paste artifact.
resolved status consideration: Incident resolved status could arguably be cold. Worth a brief comment in the COLD_STATUSES definition noting the intentional exclusion of resolved (since incidents have a different lifecycle). Not blocking -- the current set is defensible.

SOP COMPLIANCE

Branch named after issue: 190-include-cold-param-list-notes references issue #190
PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present
Related references plan slug: plan-2026-03-16-knowledge-architecture referenced, Closes #190 present
Tests exist and pass: 6 new tests, 638 total reported passing
No secrets committed
No unnecessary file changes: exactly 2 files changed, both directly related to the feature
Commit messages are descriptive: PR title follows conventional commit format

PROCESS OBSERVATIONS

Deployment frequency: Small, focused change (180 additions, 1 deletion across 2 files). Low merge risk. This is a good example of a single-concern PR.
Change failure risk: Low. The change is additive (new query param with backward-compatible default). Existing callers that do not pass include_cold get the new filtering behavior, which is the intended optimization. The explicit status= override ensures no breaking change for callers that filter by specific statuses.
DORA impact: This directly reduces token cost during session injection (~131 to ~60 notes), improving agent response time and cost efficiency. Good operational win.

VERDICT: APPROVED

## PR #191 Review ### DOMAIN REVIEW **Tech stack:** Python / FastAPI / SQLAlchemy (ORM queries) / pytest with TestClient **Route logic (`src/pal_e_docs/routes/notes.py`)** 1. `COLD_STATUSES` is declared as a module-level `frozenset[str]` -- immutable, type-annotated, well-documented. Good. 2. The `include_cold` Query parameter defaults to `False` with a clear description string. Placement before `limit`/`offset` in the parameter list is consistent with filter params above. 3. The filter logic at line 412-413 is correct and clean: ```python if not include_cold and status is None: q = q.filter(or_(Note.status.notin_(COLD_STATUSES), Note.status.is_(None))) ``` - When `status` is explicitly provided, the cold filter is skipped entirely -- this is the right approach because the caller is explicitly asking for that status. - `Note.status.is_(None)` correctly handles NULL status values so notes without a status are never accidentally excluded. - The `or_` import was added to the existing `sqlalchemy` import line. Clean diff. 4. The filter is placed AFTER the `parent_slug` filter and BEFORE the ordering logic. This is the correct position -- it does not interfere with parent resolution or ordering. 5. Search endpoints (`/notes/search`, `/notes/semantic-search`) are correctly NOT affected. They use raw SQL via `_build_filter_clauses` / `text()` and have their own `status` parameter. The cold exclusion is scoped only to the ORM-based `list_notes`. **COLD_STATUSES consistency check against VALID_STATUSES:** - `completed`: appears in plan, phase, sprint, milestone -- correct to be cold. - `done`: appears in todo -- correct to be cold. - `deprecated`: appears in sop, convention, template, skill, agent -- correct to be cold. - `deferred`: appears in plan, phase -- correct to be cold. - `archived`: appears in project-page, doc, sprint, post -- correct to be cold. - `resolved` (incident): arguably cold, but incidents are a different lifecycle. Reasonable to leave out for now. - `published` (journal, post): active content, NOT cold. Correct exclusion from the set. All five cold statuses are terminal/inactive states. The set is well-chosen. **Tests (`tests/test_include_cold.py`)** 6 tests covering: 1. Default exclusion of cold notes -- verifies all 5 hot + NULL-status notes appear, all 5 cold notes excluded. 2. `include_cold=true` returns all 10 notes. 3. `status=completed` overrides cold exclusion -- returns only completed notes. 4. `status=done` overrides cold exclusion -- returns only done notes. 5. `project=agency` combined with cold exclusion -- verifies project filter and cold filter compose correctly. 6. `project=agency&include_cold=true` -- verifies project filter with cold inclusion returns all 9 project notes. Tests use the shared `_seed_notes` helper which creates a realistic mix of hot/cold/NULL-status notes across a project. The helper asserts creation success, which is good defensive testing. The `client` fixture uses `autouse=True` `setup_db` which drops/recreates all tables between tests, so test isolation is solid. **PEP compliance:** - Type hints present (`frozenset[str]`, `bool`, `str | None`). - Docstrings on the helper function. - Module docstring on the test file. - Ruff format/check reported clean in PR body. ### BLOCKERS None. - Test coverage is present and thorough (6 tests, happy path + edge cases + composition). - No unvalidated user input -- `include_cold` is a `bool` Query param, FastAPI validates it automatically. - No secrets or credentials in code. - No DRY violations in auth/security paths. ### NITS 1. **Missing `note_type` + cold exclusion test**: When `note_type=phase` is specified but `status` is NOT, the cold filter still applies (because `status is None` in the condition). This means `GET /notes?note_type=phase` would return only non-cold phases. This is correct behavior, but there is no test covering `note_type` filter combined with cold exclusion. Consider adding one in a follow-up. 2. **`_seed_notes` docstring says "Returns a dict"** but the function returns `None`. The docstring is inaccurate -- it is a void helper. Minor copy-paste artifact. 3. **`resolved` status consideration**: Incident `resolved` status could arguably be cold. Worth a brief comment in the `COLD_STATUSES` definition noting the intentional exclusion of `resolved` (since incidents have a different lifecycle). Not blocking -- the current set is defensible. ### SOP COMPLIANCE - [x] Branch named after issue: `190-include-cold-param-list-notes` references issue #190 - [x] PR body follows template: Summary, Changes, Test Plan, Review Checklist, Related sections all present - [x] Related references plan slug: `plan-2026-03-16-knowledge-architecture` referenced, `Closes #190` present - [x] Tests exist and pass: 6 new tests, 638 total reported passing - [x] No secrets committed - [x] No unnecessary file changes: exactly 2 files changed, both directly related to the feature - [x] Commit messages are descriptive: PR title follows conventional commit format ### PROCESS OBSERVATIONS - **Deployment frequency**: Small, focused change (180 additions, 1 deletion across 2 files). Low merge risk. This is a good example of a single-concern PR. - **Change failure risk**: Low. The change is additive (new query param with backward-compatible default). Existing callers that do not pass `include_cold` get the new filtering behavior, which is the intended optimization. The explicit `status=` override ensures no breaking change for callers that filter by specific statuses. - **DORA impact**: This directly reduces token cost during session injection (~131 to ~60 notes), improving agent response time and cost efficiency. Good operational win. ### VERDICT: APPROVED

2026-03-17 03:12:21 +00:00

Summary

Adds knowledge tiering to list_notes via an include_cold query parameter. Cold notes (status in completed/done/deprecated/deferred/archived) are excluded by default, reducing session injection from ~131 notes to ~60 for pal-e-agency. Explicit status= param overrides the exclusion, and include_cold=true restores full backward-compatible behavior.

Changes

src/pal_e_docs/routes/notes.py -- Added COLD_STATUSES frozenset constant, include_cold Query param on list_notes, and filter logic that skips cold exclusion when status is explicitly provided. Handles NULL status correctly via or_ clause.
tests/test_include_cold.py -- 6 new tests covering default exclusion, include_cold=true, explicit status override, done status, project filter + cold exclusion, and project filter + include_cold.

Test Plan

pytest tests/ -x -q -- all 638 tests pass (632 existing + 6 new)
ruff format and ruff check clean
Key scenarios tested:
- Default excludes cold notes
- include_cold=true returns all notes
- status=completed returns completed notes even with default include_cold=false
- status=done returns done notes
- Project filter combined with cold exclusion
- Notes with NULL status are not excluded

Review Checklist

Code follows existing patterns in the codebase
Tests added and passing (6 new, 638 total)
Linting clean (ruff format + ruff check)
No unrelated changes
Search endpoints (semantic_search, search_notes) NOT affected

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

## Summary Adds knowledge tiering to `list_notes` via an `include_cold` query parameter. Cold notes (status in completed/done/deprecated/deferred/archived) are excluded by default, reducing session injection from ~131 notes to ~60 for pal-e-agency. Explicit `status=` param overrides the exclusion, and `include_cold=true` restores full backward-compatible behavior. ## Changes - `src/pal_e_docs/routes/notes.py` -- Added `COLD_STATUSES` frozenset constant, `include_cold` Query param on `list_notes`, and filter logic that skips cold exclusion when `status` is explicitly provided. Handles NULL status correctly via `or_` clause. - `tests/test_include_cold.py` -- 6 new tests covering default exclusion, include_cold=true, explicit status override, done status, project filter + cold exclusion, and project filter + include_cold. ## Test Plan - `pytest tests/ -x -q` -- all 638 tests pass (632 existing + 6 new) - `ruff format` and `ruff check` clean - Key scenarios tested: - Default excludes cold notes - `include_cold=true` returns all notes - `status=completed` returns completed notes even with default `include_cold=false` - `status=done` returns done notes - Project filter combined with cold exclusion - Notes with NULL status are not excluded ## Review Checklist - [x] Code follows existing patterns in the codebase - [x] Tests added and passing (6 new, 638 total) - [x] Linting clean (ruff format + ruff check) - [x] No unrelated changes - [x] Search endpoints (semantic_search, search_notes) NOT affected ## Related - Plan: `plan-2026-03-16-knowledge-architecture` - Closes #190

forgejo_admin added 1 commit

feat: add include_cold param to list_notes for knowledge tiering (#190 )

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

0614fe193b

Exclude cold notes (completed/done/deprecated/deferred/archived) by
default from list_notes to reduce token cost during session injection.
Explicit status= param overrides the exclusion. include_cold=true
restores full backward-compatible behavior.

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