fix: reject unknown tag ids on board columns instead of silent drop (#1897) - #2568
Conversation
…1897) A column PATCH/POST naming a tag id missing from the vocabulary was silently stripped while still returning 200 — the column landed back on tag_ids [] (the deliberate match-all state), so the board filter appeared to do nothing: every column kept showing the full session list. - _normalize_column now rejects a payload with an unknown or non-string tag id (400 invalid_column_payload, same shape as the existing branch). The empty list stays valid: it is the documented clear-filter/match-all state the board UI depends on. - load_tags() now prunes dangling tag ids from persisted columns (a crash mid-tag-delete can leave them), mirroring the slot-restore prune including its fail-open rule for an unknown vocabulary — without this, a dangling id would make that column's filter permanently un-editable under the stricter validation. - The board's updateColumnMutation re-syncs the chat-tags and tag-columns caches on error, so a popover holding a stale tag list redraws from server truth instead of showing a selection that was never applied. - Backend tests: accepted-valid / rejected-unknown (update + create) / empty-list-still-allowed / dangling-id prune (+ fail-open). - Frontend tests: a filtered column renders only matching sessions; the PATCH-rejection path invalidates both caches. - Asserting screenshot harness + evidence under temp-screenshots/board-column-tag-filter/. Closes #1897
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsThe PR is small and well-contained. Let me verify one remaining concern: whether the load-time prune mutating in-memory only (not persisted) causes any inconsistency, and whether the reject-path breaks the update flow when Both are handled correctly:
The reject-instead-of-drop change is the correct fix, the empty-list match-all state stays valid, and the frontend No findings. [OPUS-REVIEWED] 61000ea Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
UX Review (Fable 5) — ✅ PASSAdvisory UX-level review of UX-Verdict: PASS The fix turns an invisible silent failure into a self-healing one; the rendered board matches the claims, and the residual rejection path is rare and self-correcting. Suggestions
[UX-REVIEWED] 61000ea |
Design Review (Fable 5) — ✅ PASSAdvisory design-level review of Design-Verdict: PASS Root cause fixed at the right layers: API rejects, load-time prune self-heals persisted dangling ids, client re-syncs — with fail-open guards preserved. Suggestions
[DESIGN-REVIEWED] 61000ea |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
…irodotdev#1897) (kirodotdev#2568) A column PATCH/POST naming a tag id missing from the vocabulary was silently stripped while still returning 200 — the column landed back on tag_ids [] (the deliberate match-all state), so the board filter appeared to do nothing: every column kept showing the full session list. - _normalize_column now rejects a payload with an unknown or non-string tag id (400 invalid_column_payload, same shape as the existing branch). The empty list stays valid: it is the documented clear-filter/match-all state the board UI depends on. - load_tags() now prunes dangling tag ids from persisted columns (a crash mid-tag-delete can leave them), mirroring the slot-restore prune including its fail-open rule for an unknown vocabulary — without this, a dangling id would make that column's filter permanently un-editable under the stricter validation. - The board's updateColumnMutation re-syncs the chat-tags and tag-columns caches on error, so a popover holding a stale tag list redraws from server truth instead of showing a selection that was never applied. - Backend tests: accepted-valid / rejected-unknown (update + create) / empty-list-still-allowed / dangling-id prune (+ fail-open). - Frontend tests: a filtered column renders only matching sessions; the PATCH-rejection path invalidates both caches. - Asserting screenshot harness + evidence under temp-screenshots/board-column-tag-filter/. Closes kirodotdev#1897
Summary
Fixes the board column tag filter appearing to do nothing (#1897). A column PATCH/POST whose
tag_idsnamed a tag id missing from the vocabulary was silently stripped while still returning 200 — the column landed back ontag_ids: [], the deliberate match-all state, so the lane kept showing every session ("same count as ALL SESSIONS").What changed
_normalize_columnrejects unknown/non-string tag ids (400 +invalid_column_payload, the handler's existing error shape) on bothPATCH /api/chat/tag-columns/{id}andPOST /api/chat/tag-columns. The empty list stays valid — it is the documented clear-filter/match-all state the board UI depends on (clear-filter button, both add-column flows).load_tags()prunes dangling tag ids from persisted columns. A crash mid-tag-delete can leave a deleted id on a column (the vocab write commits first, board strip is best-effort); under the stricter validation a dangling id would make that column's filter permanently un-editable (the popover echoes the full list back). Mirrors the slot-restore prune, including its fail-open rule when the vocabulary is unknown (corrupttags.json).updateColumnMutationgainsonErrorinvalidating['chat-tags']+['tag-columns']: a popover holding a stale tag list (tag deleted from another window) re-syncs to server truth instead of showing a selection that was never applied.columnMatchessemantics (any/all/none, include_untagged) are untouched — they were verified correct.Root-cause verification
The reported happy path (valid id → PATCH → persist → render) was reproduced green: the popover,
_normalize_column'svalid_ids, and slot tags all share onestate._tagsstore. The only reachable way to land on the match-all symptom is an id the vocabulary doesn't hold at PATCH time (stale popover cache after a cross-window tag delete, or a dangling id from a crash mid-delete) — precisely the silent-drop path this PR makes loud, self-healing, and self-repairing at load.Designed residual (noted for reviewers)
A PATCH carrying one stale + one valid id rejects the whole payload — the valid toggle is lost until the onError re-sync redraws the popover (one extra click). Reject-don't-guess is deliberate: partially applying a filter write would reintroduce a quiet divergence between what the user selected and what persisted.
Tests
test/test_chat_tags.py): accepted-valid persists end-to-end · unknown id → 400 + code, prior filter untouched (update AND create) · empty list still accepted on both · non-string entries rejected · load-time dangling-id prune · fail-open when vocabulary unknown. All 8 new tests verified failing on unfixed code.ChatSidebar.boardColumnTagFilter.test.tsx): a filtered column renders only matching sessions (the issue's expected behavior) · PATCH rejection invalidates both caches. Verified failing without the fix.npx tsc -bclean · full vitest 12164 passed.Screenshots (asserting harness:
website/scripts/capture-board-column-tag-filter.mjs)Closes #1897