Skip to content

feat(folders): folders carry tags; new chats in a tagged folder inherit them - #5432

Merged
kyleseaman merged 1 commit into
kirodotdev:mainfrom
jeeshofone:feat/folder-tags
Aug 30, 2026
Merged

feat(folders): folders carry tags; new chats in a tagged folder inherit them#5432
kyleseaman merged 1 commit into
kirodotdev:mainfrom
jeeshofone:feat/folder-tags

Conversation

@jeeshofone

@jeeshofone jeeshofone commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Folders and tags are the sidebar's two organizational dimensions, but they don't compose: a folder that represents a workstream gives its sessions no tags, so every new chat launched inside it starts untagged and is invisible to tag-filtered board columns until someone tags it by hand — repeating a manual step for information the folder placement already expresses. Issue #5419 (design + use cases discussed there).

Why it matters

A session lives in exactly one folder (a tree) but can carry many tags (sets). Folder tags let the tree project into overlapping tag sets automatically, which turns tag-filtered board columns into views that populate by provenance: cross-folder rollups, attention/risk lanes, non-kanban context columns, and durable machine-readable selectors for automation — all with zero per-session bookkeeping. Without this, tag columns only work for users willing to hand-tag every session.

What changed (motivation → approach → change)

Goal: folders carry tags; new chats born in a tagged folder inherit them. Approach: extend the folder record's existing optional-metadata pattern (color / project_dir / default_agent are already absent-means-default keys in folders.json, so tags needs no migration), and mirror the tag-inheritance mechanism #4419 established for forks (copy-by-value at the moment of association). Alternatives considered and rejected on the issue: LLM-derived tags (heavier, non-deterministic, and gated by the _bg-session constraint from #2253) and coupling tags to runtime scope (#2164 keeps folders/tags organizational).

What was built:

  • Folder model (src/kiro_crew/dashboard/chat_folders.py): optional tags: list[str] of existing tag-vocabulary ids on create and update, validated via the shared validate_folder_tag_ids chokepoint (only a non-array payload 400s; unknown and non-string ids are silently filtered, deduped preserving order — no count cap — and the vocabulary intersection is skipped when tags.json was unreadable at boot, failing open so a save cannot wipe stored tags). Malformed persisted entries (a hand-edited non-string in folders.json) are skipped at inheritance time rather than crashing slot creation.
  • Inheritance (src/kiro_crew/dashboard/chat_handlers.py, api_chat_slot_create): when a genuinely NEW slot is created with a folder_id, the folder's tags are copied by value onto the slot after the folder's existence is confirmed. Creation-only: addressing an existing slot by name never re-tags, and moving a session into a folder never retro-tags.
  • Channel parity (src/kiro_crew/dashboard/channel_slots.py): the per-channel default-filing path applies the same copy-by-value inheritance on its first-filing branch, so a channel chat born into a tagged folder inherits exactly like a dashboard chat (its restore branch — a persisted folder_id — never re-tags). The folder's raw tag ids are read fresh inside tags_write_lock, immediately before validation and the filing write (lock ordering tags_write_lock → folder-store lock, matching the folder create/PATCH paths), so a concurrent folder PATCH or tag deletion can never stamp an obsolete tag set onto a freshly filed chat. The inherited ids are persisted in the channel conversation's metadata (a tags key written atomically with the channel_folder_filed marker) and re-applied on every surface through an authority-aware validator that fails open when tags.json is unreadable — this is permanent on-disk state, required because the sticky filing marker blocks re-inheritance, so a crash between the filing write and the slot's first save must be recoverable. The zero option (accept the crash-window loss) was weighed and rejected: with the marker persisted alone, the loss is permanent and silent (no pass ever re-offers the tags); dropping the marker instead would re-run inheritance onto chats whose user deliberately removed the tags. Carrying the tags in the already-required atomic marker write is the minimal crash-consistent shape.
  • Unresolved tag vocabulary placeholder (website/src/components/FolderConfigModal.tsx): while the chat-tags query is loading or has failed, the Tags section renders its heading with a muted "Loading tags…" placeholder (localized in all 11 catalogs) instead of nothing, so a failed query no longer silently erases the feature and the section resolving after open does not shift the layout.
  • Declared lint riders from the warning-ratchet round: the repo's shrink-only eslint warning ceiling dropped under this branch's count after a rebase; the three warnings in the new FolderConfigModal.tsx were fixed with real a11y wiring (id/htmlFor/aria-label on the tag chips), plus one justified jsx-a11y/label-has-for disable on the folder-name label — the deprecated rule cannot see the htmlForid link to the custom Input control (same precedent as RegistryManager.tsx). channel_slots.py was black-formatted because the gate flagged it as a new offender (it is not baselined).
  • Endpoint semantics: folder tags writes silently filter unknown ids, mirroring api_chat_slot_tags' filter-don't-400 shape — a dangling id left by a failed best-effort strip is shed on the next save rather than 400ing the folder. The two validators differ deliberately on one axis: the folder/inheritance paths carry an _tags_authoritative fail-open gate (an unreadable tags.json at boot must not wipe a folder's stored tags on save), while api_chat_slot_tags validates user-submitted ids strictly against the live vocabulary with no such gate (a vocabulary outage should not silently accept arbitrary ids from a PUT body). They are one filtering shape with two intentionally different failure postures, not two spellings of one rule.
  • Vocabulary integrity (src/kiro_crew/dashboard/chat_tags.py, api_chat_tag_delete): deleting a tag now strips its id from every folder's tags list, exactly as it already strips it from every slot.
  • UI (website/src/components/FolderConfigModal.tsx + ChatSidebar.tsx, types/index.ts, api/client.ts): a Tags section in the folder settings modal renders the vocabulary as color chips. Each chip is a label wrapping a visually-hidden checkbox (a multi-select choice control, not an action row — keeps the picker inside the max-two-buttons-per-row design rule), wired into the modal's touched-based PATCH. The draft seed filters persisted ids against the live vocabulary, so a dangling reference (left by a failed best-effort strip) can never make the folder uneditable via a 400. With an empty vocabulary the section shows a one-line onboarding hint instead of vanishing. Keyboard focus (ring-2) is visually distinct from selection (ring-1 + tint). New strings added to all i18n catalogs, en-XA regenerated.
  • Spec (docs/system-specs/modules/learn-cron-dashboard.md): the folder POST/PATCH contract and the slot-create inheritance are documented in the same commit, per AGENTS.md.

Tests

Backend (test/test_dashboard_chat.py, test/test_chat_slot_create_folder.py — including the new TestFolderTagInheritance, plus test/test_channel_folders.py for the channel first-filing branch):

  • folder create/update with valid tags persists; an unknown tag id is silently filtered (only a non-array payload 400s); an unreadable vocabulary preserves stored tags instead of wiping them
  • a new slot created with folder_id inherits the folder's tags
  • inheritance does NOT fire when addressing an existing slot
  • moving an existing slot into a tagged folder does NOT retro-tag
  • deleting a tag strips it from folders

Frontend (website/src/test/FolderConfigModal.test.tsx — 46 pass, component statement coverage 92%):

  • onboarding hint replaces the picker without a vocabulary; renders a chip per tag
  • a dangling persisted tag id is filtered from the seed and never submitted
  • edit mode pre-selects the folder's tags (checkbox state)
  • toggling updates the draft and submits the selected ids; touched only reports tags when the selection actually changed
  • a tag-only change arms the dismiss guard; deselecting back to the original set is not a change

Manual verification

Verified on a seeded dev-harness gateway (built dist, port-isolated home): created a tag vocabulary and a tagged folder via the API, opened the folder settings modal in a headless browser, exercised the chip toggles, and captured the screenshots below from that live session.

Screenshots / video

Folder settings modal with the new Tags section — chips carry the tag's color; the folder's tags are pre-selected in edit mode:

Folder settings modal showing the tag picker with two tags selected

Toggling a third chip on (selection ring + tinted fill):

Folder settings modal with a third tag toggled on

Related Issues

Fixes #5419

Checklist

  • Single commit with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@jeeshofone
jeeshofone requested a review from a team August 24, 2026 01:10
@jeeshofone
jeeshofone requested a review from a team as a code owner August 24, 2026 01:10
@jeeshofone
jeeshofone requested a review from dwu96 August 24, 2026 01:10
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Aug 24, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 24, 2026
@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed aafa878148f2db8eee3611de0a7d8d33534a459a via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] aafa878

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — ✅ PASS

UX-level review of aafa878148f2db8eee3611de0a7d8d33534a459a via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

UX-Verdict: PASS

Complete state coverage (loading/error+retry/empty/populated), accurate creation-only copy, and keyboard-operable chips — the only rough edge is the cold-start path to a first tag.

Suggestions

  • tags_empty_hint says "create them from any chat's tag menu," but the actual menu item is labeled "Tags…" (sessionActionsMenu.tags) — quote the real label so a first-time user can find the affordance it points at.
  • The empty state sends a user out of a possibly-dirty modal to create their first tag; SlotTagPopover already ships an inline "New tag…" input for exactly this — reuse it in the empty-state branch so folder tagging starts without the round trip.

[UX-REVIEWED] aafa878

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

Design-level review of aafa878148f2db8eee3611de0a7d8d33534a459a via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

temp-screenshots/ turns out to be an established repo convention (2,781 files, gitignore even calls it a deliverable) — that kills my last candidate finding. I also verified the deeper design question: tags is in SLOT_OWNED_META_KEYS (history.py:183), so a slot save is authoritative for the same metadata key the channel filing write seeds, meaning user tag removals persist and the every-surface restore cannot resurrect them — the "recovery, not re-inheritance" claim holds. The remaining candidates (fail-open/fail-closed validator split, direct-folder-only inheritance) are deliberate, documented decisions with named rationale.

Design-Verdict: PASS

Additive optional-key model, creation-only inheritance validated at the point of application, and crash-consistent filing — the right shape, verified coherent with the slot-save metadata contract.

[DESIGN-REVIEWED] aafa878

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of aafa878148f2db8eee3611de0a7d8d33534a459a via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified against the base checkout. The fork copy-by-value precedent is real (chat_fork.py:698), the helper docstring's sibling counts are accurate (3 restore-path prunes + the strict api_chat_slot_tags filter at chat_tags.py:418), temp-screenshots/ is an established committed-deliverable convention named in .gitignore, and the two failure postures (fail-open vs strict) genuinely pre-exist in base code. The one thing the change leaves uneven: the new inline Retry recovers a dead tag vocabulary only inside the folder modal, while the two other chat-tags consumers keep failing silently.

First-Principles-Verdict: CONCERNS

The feature earns its place end-to-end and is unusually fully declared; the modal-only vocabulary Retry and a fifth tag-validator spelling are point patches with counted, deferred siblings.

What this change ships

Intent: let a folder auto-tag every new chat started inside it, so tag-filtered views populate without hand-tagging. ADDITION (issue #5419).

  1. Folders can carry tags, set on create and edit — justified
  2. A new chat started in a tagged folder starts with those tags — justified (the feature)
  3. A channel chat first filed into a tagged folder inherits identically — justified parity
  4. Channel conversations persist inherited tags on disk with the filing marker — declared; derived crash-consistency (sticky marker blocks re-inheritance)
  5. Deleting a tag strips it from every folder — justified, mirrors the existing slot strip
  6. Folder settings modal gains a tag-chip picker with empty-vocabulary hint — justified
  7. Loading placeholder + error line + inline Retry in the modal's Tags section — declared; retry is a point patch, 2 unfixed siblings
  8. Shared validate_folder_tag_ids, a 5th spelling of vocabulary filtering — declared accepted-and-deferred consolidation
  9. New strings in every locale catalog — mandated (i18n invariant)
  10. a11y wiring + one eslint disable on the folder-name label — declared rider

Watch

  • The inline Retry (onRetryTags, availableTagsFailed, 3 strings × 13 catalogs) recovers a failed chat-tags query only in the folder modal. Grep ['chat-tags'] query sites: SlotTagPopover.tsx:25 and TagManagerList.tsx:43 fall back to [] and render "no tags" with no recovery — 2 unfixed siblings of the same cause (the description's own comment: "a FAILED chat-tags query never self-heals"). A query-level recovery would retire the modal-local props.
  • validate_folder_tag_ids is the 5th filter-tag-ids spelling (counted: chat_persistence.py:962,1430, chat_handlers.py:5850, chat_tags.py:418). The strict/fail-open divergence is real and argued, but the docstring itself defers the strict-mode-flag consolidation — hold the author to that follow-up or the five will drift.

[FIRST-PRINCIPLES-REVIEWED] aafa878

@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed aafa878148f2db8eee3611de0a7d8d33534a459a via the fork AI-review pipeline; updated in place on each push.

Review details

Both candidates confirmed low-value and below the bar:

  • Candidate 1 (unbounded folder-tags list on the fail-open branch): requires an owner-authenticated request during a narrow non-authoritative boot window sending thousands of ids. No boundary from the threat model is crossed — the owner is the trusted party, so this is self-inflicted resource use, not data loss/corruption/security. Discovery itself rated it "low" and left "(a)" contingent on an unconfirmed upstream body cap. Fails the 80+ bar.
  • Candidate 2 (fail-open inheritance stamps dangling ids onto a new slot): this is the deliberately documented fail-open tradeoff (validate_folder_tag_ids docstring; sibling restore paths do the same). Outcome is transient (stale tag chips, self-heals on the next authoritative boot) — cosmetic, no crash/corruption/boundary. Fails the bar.

I also verified load_folders filters self._folders to rows with a non-empty string id (state.py:7435-7439), so the unguarded x["id"] in chat_handlers._read_folder_tags cannot KeyError — no Step-2 finding there.

No findings.

[OPUS-REVIEWED] aafa878

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 24, 2026
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Dispositions for the GPT 5.6 Review 🔴 on 517b24797 — both blockers fixed in 72a3b30c0.

  • Malformed persisted folder tags crash slot creation (chat_handlers.py)fixed.

    folders.json with tags: [{}] → slot creation inheritance → unhashable membership test raises 500 after inserting the slot.

    Verified real: folders.json is hand-editable and a dict entry is unhashable, so tid in valid_ids itself raises after the slot is already inserted. The inheritance loop now requires isinstance(tid, str) before any membership test, skipping malformed entries while still copying valid sibling ids. Regression test test_malformed_folder_tag_entry_does_not_crash_slot_creation seeds [{}, None, 42, "t1"] and asserts a 200 with tags == ["t1"].

  • Failed cleanup leaves folder tags permanently uneditable (chat_tags.py → modal PATCH loop)fixed via the prescribed draft filter.

    Folder-store write failure during tag deletion → swallowed rollback retains the deleted ID → the modal cannot display or remove it, and every later tag edit is rejected.

    Verified real: the swallowed best-effort strip can leave a dangling id, the modal seeded it verbatim into the draft, and _validate_folder_tags 400s the whole PATCH — the folder becomes permanently uneditable over a reference the picker cannot even render. Fixed exactly as prescribed: the modal's draft seed now filters persisted ids against the live vocabulary (FolderConfigModal.tsx, via a ref so the vocabulary is not an effect dependency that would re-seed an open draft). A dangling id alone no longer marks tags as touched (rename-only edits succeed and never send the stale reference), and any tag edit submits the filtered list, pruning the dangler. Tests: a dangling persisted tag id is filtered out of the seed, a dangling id alone does not mark tags as touched.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Dispositions for the UX Review 🟡 CONCERNS on 517b24797 — all three items resolved in 72a3b30c0.

  • Ambiguous ring on tag chips (selected vs focus pixel-identical)fixed.

    clicking a label focuses its checkbox, so deselecting a chip leaves the accent ring in place (looks selected, only the 30% tint dropped), and keyboard users tabbing onto an already-selected chip see zero focus movement.

    The focus-within ring is now ring-2 while selection stays ring-1 + tint: a just-deselected chip shows a visibly different (thicker, untinted) ring that reads as focus, and tabbing onto a selected chip produces visible focus movement. Selection rendering is unchanged, so the PR's screenshots remain accurate.

  • Surface the 10-tag cap before rejectionfixed by subtraction rather than by disclosure: the cap is gone entirely. First Principles independently established no sibling validator caps a validated id list (api_chat_slot_tags, column tag_ids), so _FOLDER_TAGS_MAX and its 400 branch were removed — there is no longer a post-submit rejection to discover.

  • Tags section vanishes with no vocabularyfixed as suggested: the section now renders its heading plus a one-line muted onboarding hint ("Create tags to auto-tag new chats created in this folder.", localized across all catalogs) instead of disappearing. Tests updated to lock the hint in.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Dispositions for the Design Review 🟡 CONCERNS on 517b24797.

  • Channel sessions bypass inheritancefixed in 72a3b30c0.

    a chat born into a tagged channel folder gets no tags → tag-filtered board columns silently miss exactly the provenance-driven sessions the description promises.

    The per-channel default-filing path now applies the same copy-by-value inheritance on its first-filing branch: the reconcile pass resolves and validates the target folders' tags in one store read, and surface_channel_session copies them only where it applies folder_id for the first time. The restore branch (a persisted folder_id) deliberately does not — that conversation was filed long ago, and re-stamping it would be the retro-tagging the creation-only rule forbids. Tests: test_first_filing_inherits_the_folders_tags, test_restoring_a_filed_session_never_re_tags.

  • Tagging an existing folder does nothing visibleneeds-a-decision, and the question is already in front of you: this is decision 2 in Folders can carry tags; new chats created in a tagged folder inherit them #5419 ("inherit at creation only; moving a session into a folder does not retro-tag"), which my follow-up comment there explicitly re-opened with the same argument you make (lens-column truth vs. move semantics). Whichever way you rule on Folders can carry tags; new chats created in a tagged folder inherit them #5419, this PR implements the conservative creation-only baseline, and the modal's hint string states it ("Applied to every new chat created in this folder"). A one-time "apply to existing sessions?" affordance is a clean follow-up once ruled.

  • Spec not updated in the same commitfixed in 72a3b30c0: docs/system-specs/modules/learn-cron-dashboard.md now documents tags on the folder POST/PATCH contract (validation, no-cap rationale, absent-means-no-tags) and the creation-only inheritance on both the slot-create and channel filing paths.

  • Exclude/cap status: true tags in the folder picker (suggestion)needs-a-decision. The observation is real: the seed vocabulary is workflow states, and inheriting a kanban state stamps every new chat with a status the board's drag semantics never produced. But whether folder tags should be restricted to non-status tags (or the seed vocabulary is simply not the intended folder-tag vocabulary) is a product ruling about what tags are — restricting the picker unilaterally would also make it inconsistent with the session tag picker, which offers status tags freely. Flagging it to the maintainer here rather than choosing.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Dispositions for the First Principles Review 🟡 CONCERNS on 517b24797 — both Watch items and the Subtraction taken in 72a3b30c0.

  • Unfixed sibling: channel default filing (channel_slots.py)fixed. Your grep was right and the hint string's promise was false for exactly that path. The first-filing branch now applies the same copy-by-value inheritance (tags resolved and validated by the reconcile caller in one store read); the restore branch stays untouched, keeping creation-only semantics consistent across both mint paths. Locked by two tests, one per branch.

  • Cap justification is analogy + Subtraction: drop _FOLDER_TAGS_MAXfixed by taking the subtraction. Your count was correct (sibling caps: 0 — neither api_chat_slot_tags nor column tag_ids caps a validated id list) and the "chip row bound" rationale was indeed backwards, since the row renders the whole vocabulary regardless. The constant and its 400 branch are gone; the PR body's "validates exactly like the slot-tags endpoint" claim is now actually true. This also dissolved UX's cap-discoverability concern.

  • Ride-along (api_chat_slot_stop line rejoin) and screenshot convention — noted; unchanged and declared.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for the Opus 4.8 Review ✅ advisory on 517b24797.

  • temp-screenshots/ PNGs land in permanent historyrebutted (deliberate, repo convention). temp-screenshots/ is this repository's documented Screenshot Evidence mechanism: images are committed into the PR branch under a top-level dir that is outside every packaged path (wheel/sdist/DMG), embedded via commit-SHA-pinned URLs so they survive branch deletion, and the directory is pruned from main periodically — the ~560 sibling dirs are the precedent. The Screenshot Evidence check requires exactly this for a user-visible UI change, so dropping them would trade a passing gate for a red one.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 24, 2026
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Round-2 CI reds on 72a3b30c0 (Backend Tests shard 3 × 3 matrices + the fail-closed Coverage Gate cascade) are inherited main breakage, not this PR's diff: test_messaging_pre_turn.py's roster ratchet fails on whatsapp/feishu after #5114/#4282 crossed with #5379. This diff touches no messaging code. Fix opened as #5469 (test-only exempt addition per the ratchet's own docstring); I'll rebase this PR past it once it lands, which should also let the skipped fork review lanes run round 2.

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Aug 28, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Aug 28, 2026
@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for GPT round (BLOCK + one finding), fixed in 000934271:

BLOCKING — stale folder-tags snapshot before filing: ACCEPTED and fixed with the suggested mechanism. The pass-top bulk read is deleted; the folder's raw tag ids are now read fresh via state.read_folders inside tags_write_lock, immediately before validate_folder_tag_ids and the filing write, so a folder PATCH or tag deletion committing mid-pass is fully visible before anything is stamped onto the filed chat. Lock ordering is tags_write_lock → folder-store lock, matching the folder create/PATCH paths (no inversion anywhere), so the nesting introduces no deadlock. Covered by the existing filing/inheritance suites (87 + 65 tests green on the new head).

FINDING — function-local imports: ACCEPTED for chat_folders.py, DECLINED for channel_slots.py with evidence. chat_folders.py's three local chat_tags imports are now one top-level import (nothing in chat_tags' import chain reaches back to chat_folders). The channel_slots.py local import cannot move: chat_tags imports chat_persistence (chat_tags.py:24), and chat_persistence imports channel_slots (chat_persistence.py:23 — slot_closed_since), so a top-level chat_tags import in channel_slots is a real circular import, not a style choice. The local import site carries a comment naming the cycle.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for UX round (CONCERNS), addressed in 000934271:

Unresolved vocabulary renders nothing: ACCEPTED and fixed. The unresolved state (availableTags === undefined, covering both loading and a failed chat-tags query) now renders the section heading with a muted "Loading tags…" placeholder instead of nothing — the feature never silently vanishes, and the section resolving after open no longer shifts the layout. Localized in all 11 catalogs; the prior test pinning the bare-nothing branch is updated to pin the placeholder.

No inline tag creation: DEFERRED as a follow-up, not folded into this round. The point is fair — SlotTagPopover's inline "New tag" input is the sibling pattern. Deliberately not adding it here: it would give the folder modal a tag-vocabulary write path (today the modal only selects from the vocabulary; all writes remain folder writes), which is exactly the scope expansion the First Principles lane on this same head is asking to shrink. Happy to bring the inline input over in a follow-up PR where its write path, rate limiting, and tests get their own review.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for First Principles round (CONCERNS):

Item 10 — undeclared eslint suppression: ACCEPTED, now declared in the PR body. The jsx-a11y/label-has-for disable on the folder-name label landed in the previous push's lint round: the rebase brought the repo's shrink-only warning ceiling (659) under the branch's count, and the deprecated rule cannot see the htmlForid link to the custom Input control — the disable follows the repo's existing justified precedent (RegistryManager.tsx). The body now says so.

Item 6 subtraction (drop the crash-durability half): DECLINED, with the zero option now weighed in the body. Dropping the tags key from the filing write while keeping the channel_folder_filed marker does not degrade to "one untagged chat a user can tag by hand" — it makes the loss permanent and silent, because the sticky marker is precisely what tells every later pass that inheritance already ran, so no pass will ever re-offer the tags. The alternative — dropping the marker so a later pass retries — re-runs inheritance against a user who deliberately removed the tags, which is a worse bug (resurrecting removed tags) that an earlier round's design explicitly closed. Given the marker must be persistent, carrying the tags in the same atomic write is the minimal crash-consistent shape: the "three mechanisms" reduce to one metadata key riding an already-required write, plus the restore read. The zero option was considered and is now recorded in the body rather than only in code comments.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for UX round (CONCERNS), addressed in aa5e15ca4:

Failed fetch renders a permanent "Loading tags…": ACCEPTED and fixed with the suggested mechanism. ChatSidebar now passes the query's isError as availableTagsFailed; the modal renders a distinct "Tags couldn't load — close and reopen this dialog to retry" error line (localized in all 11 catalogs) instead of the loading hint, so a dead query no longer asserts an in-progress state. Both branches are pinned by tests (loading shows no error line; failed shows no loading hint).

tags_hint reword suggestion: DEFERRED. The proposed copy is better at answering "why did my existing chats stay untagged," but the current string is accurate (states the new-chats-only rule and the subfolder boundary), and rewording re-translates 11 catalogs mid-review through the per-language style suites for a phrasing preference. Will fold the reword into the inline-tag-creation follow-up already deferred from the previous round, where that modal copy gets touched anyway.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for First Principles round (CONCERNS):

Fold the three inline slot-tag prunes onto validate_folder_tag_ids: AGREED in principle, DECLINED in this PR — the three sites are not this diff's code. All three named spans (chat_persistence.py:950-964, chat_persistence.py:1418-1432, chat_handlers.py:5830-5840) are pre-existing main code: chat_persistence.py is not touched by this PR at all, and the chat_handlers.py hunks here are elsewhere. Folding them in would (a) grow this diff with refactors of code it doesn't otherwise change — the inverse of this lane's own smallest-honest-version bar — and (b) pull chat_persistence.py into this PR's blast radius while maintainer bolichen97 has an open coordination constraint sequencing changes to that file behind #2783/#3139 (see his audit on #4904). The docstring's "single definition" claim is softened to name the helper as the definition for the folder/inheritance paths. The four-spellings-diverge risk is real; the consolidation is queued as a dedicated follow-up (with the rename) once the chat_persistence sequencing clears.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for GPT round (BLOCK), fixed in ebbe99f6b:

BLOCKING — stale folder read before tags_write_lock at the dashboard slot-create site: ACCEPTED and fixed with the suggested mechanism. Same defect class as the channel-filing site fixed two rounds ago — the state.read_folders call now sits inside the tags_write_lock critical section, immediately before validate_folder_tag_ids and the apply, matching the channel-filing path and the folder create/PATCH lock ordering (tags_write_lock → folder-store lock, no inversion). The inheritance comment now documents the read-inside-lock invariant. Covered by the slot-create folder suite (18 tests) plus tsc/lint floor on the new head.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for UX round (CONCERNS), addressed in ebbe99f6b:

tags_error_hint promises an inert action: ACCEPTED and fixed with the first suggested mechanism. ChatSidebar now refetches the chat-tags query whenever the folder modal opens while the query is errored, so "close and reopen this dialog to retry" is a true instruction — the reopen genuinely retries. No copy or catalog change needed; the fix makes the existing localized string accurate.

Empty state sends the user out of the modal (inline "New tag" input): position unchanged from the prior two rounds. The draft-loss framing is noted and real, but the calculus hasn't moved: an inline create input gives this modal a tag-vocabulary write path (today it only selects), which is the scope expansion the First Principles lane is simultaneously pressing to shrink on this same head. The inline-create follow-up (bundling this, the tags_hint reword, and the SlotTagPopover input reuse) remains committed; third statement of the same ask without a new mechanism is recorded as advisory.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for First Principles round (CONCERNS):

Fold api_chat_slot_tags' inline loop onto validate_folder_tag_ids: DECLINED — the two spellings differ deliberately, and unifying them decides a behavior question this PR shouldn't decide. The endpoint's loop validates user-submitted ids from a PUT body against the live vocabulary with no _tags_authoritative fail-open gate — correct for direct user input, where an unreadable vocabulary should not silently accept arbitrary ids onto a slot. The helper's gate exists for stored ids (a folder's persisted tags), where failing closed would wipe data the user already saved. Folding them means either adding fail-open to a user-input endpoint (accepts unvalidated ids during a vocabulary outage) or removing it from the inheritance paths (re-opens the wipe-on-unreadable-vocabulary loss this PR's design defends against). That is a real semantic fork, not two spellings of one rule — deciding it belongs to the already-queued prune-consolidation follow-up where all five sites get looked at together, not to a rider on this diff. The PR body's "exactly match" claim is the real defect here and is corrected to name the intentional difference.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for GPT round (BLOCK), fixed in 778333269:

BLOCKING — malformed vocabulary id crashes the validator's set build: ACCEPTED and fixed. The vocabulary set now admits only string ids ({i for t in state._tags if isinstance(i := t.get("id"), str)}), so a hand-edited tags.json carrying a list/dict id degrades to "unknown id" instead of raising TypeError: unhashable and 500-ing every folder-tag operation — consistent with the helper's existing fail-open posture toward an unreadable vocabulary. Pinned by a new test (test_malformed_vocabulary_id_does_not_crash_validation: unhashable id + missing id alongside a valid one; the valid id survives, the request succeeds).

BLOCKING — quadratic dedupe (tid in out against a list): ACCEPTED and fixed with the suggested mechanism. Membership is now tracked in a seen: set[str] alongside the ordered output list — O(n) for pathological large-array PATCH bodies, identical output for every well-formed input (order-preserving first-occurrence dedupe, pinned by the existing test_create_dedupes_tags).

Both fixes are in the shared helper this PR introduces (validate_folder_tag_ids); the deliberately separate strict inline loop at api_chat_slot_tags predates this PR on the slot path and keeps its documented fail-strict semantics — its consolidation is queued in the declared follow-up.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for UX round (CONCERNS), fixed in 778333269:

"Close and reopen to retry" tells users to destroy their draft: ACCEPTED and fixed with the suggested mechanism. The error line now carries an inline Retry action wired to the sidebar's existing refetchTags (new onRetryTags prop on FolderConfigModal), and the copy drops the close-and-reopen instruction across all 11 catalogs + pseudo ("Tags couldn't load." + a localized "Retry"). Recovery never leaves the modal, so a mid-draft name/color/agent survives the retry. Pinned by two new tests: clicking Retry fires the callback with the form still mounted, and the action is omitted when no callback is wired (the error line stays honest in render sites that cannot retry).

The once-per-open background refetch from the prior round stays as a belt-and-braces first attempt; the inline Retry is the user-visible path when that attempt also fails.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for First Principles round (CONCERNS), addressed in aafa87814:

Subtraction 1 — delete the once-per-open auto-retry: ACCEPTED. The tagsRetriedForOpenRef guard + its useEffect are gone from ChatSidebar.tsx. The finding is exactly right: the effect's justifying comment argued against "reopen this dialog to retry" copy that the inline Retry button replaced in the same push, and a mechanism that only exists safely by carrying its own infinite-loop guard is a cost with no remaining benefit. The replacement comment now documents the single recovery path (user-driven inline Retry via onRetryTagsrefetchTags).

Subtraction 2 — drop onRetryTags optionality: ACCEPTED. The prop is now required and the {onRetryTags && …} branch is gone; the sole production consumer always passed it, so the button-less branch guarded a caller that didn't exist. The test asserting that branch was removed with it; the render-helper supplies the now-required prop.

Watch — the deferred consolidation should count 4 sites, not 3: ACCEPTED. The helper's docstring now names api_chat_slot_tags's strict inline filter as the fourth spelling (differing only on the fail-open axis) and records the strict-mode-flag consolidation shape for the follow-up.

Verified: unpiped tsc rc=0, eslint ratchet clean, 53→52 modal tests + 517 sidebar + integration suite green, black/flake8 clean.

@jeeshofone

Copy link
Copy Markdown
Contributor Author

Disposition for First Principles round (CONCERNS, Watch items only) — holding position, no code change:

Watch — extend the vocabulary Retry to the two other chat-tags consumers: DECLINED in-PR, folded into the declared follow-up. Both siblings are pre-existing main code this diff never touches (SlotTagPopover.tsx, TagManagerList.tsx — zero lines changed vs base 93ddbc849, and their chat-tags fallback-to-[] behaviour exists on base). The modal got the Retry because this PR created the modal's tags section — recovering a surface it ships is in scope; retrofitting recovery onto two surfaces it doesn't touch is the same scope boundary this lane accepted for the validator prunes two rounds ago. The suggested query-level recovery would also retire the onRetryTags shape this lane asked for and accepted in the two prior rounds — redesigning a just-shipped, just-reviewed mechanism inside the same PR is churn, not convergence. The failed-vocabulary recovery follow-up will consider the query-level shape across all three consumers together, where it can actually be evaluated as a replacement rather than a parallel mechanism.

Watch — hold the author to the validator-consolidation follow-up: ACKNOWLEDGED. The commitment stands as declared in the helper's docstring (all five spellings, strict-mode-flag shape). Follow-up issues are queued for filing.

With CI green and GPT/UX/Design/Opus all clean on this head, these advisory Watch items are the only open lane feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Folders can carry tags; new chats created in a tagged folder inherit them

2 participants