Skip to content

fix(members): word the memory disclosure for the member's store - #7700

Merged
bolichen97 merged 1 commit into
mainfrom
fix/members-memory-disclosure
Sep 6, 2026
Merged

fix(members): word the memory disclosure for the member's store#7700
bolichen97 merged 1 commit into
mainfrom
fix/members-memory-disclosure

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Sep 1, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

The Members drawer's disclosure — "Members currently share one memory. What
you tell one member is known to all of them." — renders the same sentence for
a member configured with its own memory_store. For that member it is only
half the story: the markdown layer (preferences, project notes) is read from
the named store, while conversation memory and lessons stay in the one global
vector store every member reads. The user who took the trouble to point a
member at its own store cannot tell from the drawer which half moved.

An earlier revision of this PR hid the note on a dedicated store. The Design,
First Principles and Opus lanes rejected that, and they were right: the
sharing claim is still true at the vector layer (context.py
get_memory_for hands every named store the default's VectorMemoryStore;
history_consolidation.py routes by workspace, never by memory_store), so
silence implied an isolation that does not exist.

Change

Store-aware copy instead of hiding. The note renders on every member; its
wording follows the member's store:

  • Default store — the existing sentence, unchanged.
  • Named store — new key pages.membersPage.memory_dedicated_note:
    "Preferences and project notes come from the {{store}} store rather than
    the shared default. Conversation memory and lessons are not separated yet:
    what you tell this member is still known to every member."

The wording keys on the store itself (a config fact), not on a roster
heuristic of who else uses the store — the previous memoryShared
computation is deleted. Whether a store is shared is a backend property of
which layers it scopes, not something the client should infer from
membership.

Lessons are named as shared, not separate: context.py reads them from
memory.vector_store (global) or the default LessonStore, and the crew
editor's own preview banner already says "global lessons reach every agent".

i18n: the new key is added to all twelve hand-maintained catalogs
(bn/de/es/fr/hi/it/ja/ko/pt/ru/zh-CN/en) and en-XA is regenerated via
gen-pseudolocale.mjs.

Screenshots

Dedicated-store member (scout on scout-own) — store-aware wording below
Configuration:

Dedicated store member with the store-aware disclosure

Default-store member (radar) — existing wording, unchanged (frame 02 on main)

Default store member keeps the shared-memory disclosure

Testing

  • Pinning tests: default store → existing sentence; dedicated store →
    store-aware sentence naming the store, "not separated yet", "still known to
    every member", and NOT the default sentence; two members on the same named
    store → store-aware sentence (pins that the roster heuristic is gone).
  • Capture frame 07 asserts the store-aware wording for the dedicated-store
    fixture member; the earlier "note absent" frame is removed. Frame 02 on main
    remains the default-store counterpart; no other frame is re-captured.
  • Local gates: tsc -b clean, eslint --max-warnings 0 clean on touched
    files, npm run i18n:check passes (diff-scoped gates green). Test suites
    are left to CI.

Pattern harvest

Rule candidate: review-prompt
Pattern: a UI disclosure gated on a client-side inference about backend
state ("is this member's memory shared?") — reviewers should ask whether the
condition is a config fact the client owns or a backend property it is
guessing at; when the answer is "guessing", prefer wording the disclosure for
the fact the client does know (the store name) over hiding it.

no linked issue: PM feedback from the Members-page review thread, no tracked issue exists.

@CrysisDeu
CrysisDeu requested a review from a team September 1, 2026 18:42
@CrysisDeu
CrysisDeu requested a review from a team as a code owner September 1, 2026 18:42
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 66f9708d277ac9c74ab57edfa758cf033ffb9af3 — this comment is updated in place on each push.

Review details

No findings.

The sole candidate requires a member with memory_store: "" (explicitly empty) and a non-default workspace to reach the backend fallback mem_key = memory_store or workspace. But the config loader defaults memory_store to "default" (config/loader.py:2547), the create handler defaults it to "default" (agents.py:3113), and no shipped UI path sets it to empty string — reaching the empty-string case demands a hand-edited config, which is "if a caller were to," not a condition occurring in practice. The candidate's own analysis concedes this and rates it low. Even in that speculative case the harm is benign under-disclosure (says "shared" when the markdown layer is in fact separated), never the dangerous direction of claiming isolation that does not exist. Fails (a).

[OPUS-REVIEWED] 66f9708

Verdict parsed from the review's SHA-scoped output markers for commit 66f9708d277ac9c74ab57edfa758cf033ffb9af3.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 66f9708d277ac9c74ab57edfa758cf033ffb9af3: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 66f9708d277ac9c74ab57edfa758cf033ffb9af3 — 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. The backend semantics match the copy (context.py:1986-1990 shares the default VectorMemoryStore with every named store; history_consolidation.py has zero memory_store references), "default" is the real backend sentinel (config/sections.py:2845, loader.py:2547), the crew editor banner cited exists (en.json:6570), and temp-screenshots/ is a tracked repo convention. The new key has exactly one consumer by design (i18n catalog). I checked the roster-inference "sibling" in KiroCrewAgentsPage.tsx/CrewOverviewPane.tsx: its memoryShared tags claim two crews name the same store, a config-fact comparison, not the backend-property guess this PR removes, and the editor's bindings_preview_notice already discloses the vector-layer sharing there — so it is not the same defect and I dropped it.

First-Principles-Verdict: PASS

A half-true disclosure becomes layer-accurate for the one config where it misled, and every item is the fix or its mandated i18n/test scaffolding.

What this change ships

Intent: tell a user who gave a member its own memory store which half of memory actually moved — a FIX.

  1. Dedicated-store member's drawer note now names the store and says conversation memory stays shared — justified
  2. Default-store members keep the existing sentence, note still always shown — justified
  3. New catalog key memory_dedicated_note in all 12 locales + en-XA — justified (i18n invariant, CI-gated)
  4. Two pinning tests, including two-members-one-store pinning the roster heuristic out — justified
  5. Capture fixture scout moved to store scout-own; new asserted frame 07 — justified, declared
  6. Committed 07-dedicated-store-dark.png — justified (tracked temp-screenshots/ convention)

The fix sits at mechanism level for the drawer's wording; the deeper cause (no per-store separation of conversation memory and lessons) is a backend product gap genuinely out of scope, and the shipped copy states exactly that boundary ("not separated yet"). Backend claims verified: context.py:1986-1990 hands every named store the default's shared VectorMemoryStore; history_consolidation.py contains zero memory_store references (grep: memory_store, 0 hits). The condition keys on the backend's real "default" sentinel (config/sections.py:2845). The one-consumer key is normal catalog shape, not generalization. Nothing rides along; nothing is undeclared.

[FIRST-PRINCIPLES-REVIEWED] 66f9708

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 66f9708d277ac9c74ab57edfa758cf033ffb9af3 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All backend claims in the description verify: get_memory_for (context.py:1972) shares the default VectorMemoryStore with every named store, mem_key = memory_store or workspace (context.py:2878), and memory_store_name resolves to the literal store name with "default" as the field default. The copy is accurate, the roster heuristic the description mentions was in an earlier revision of this PR (not base), and tests pin both wordings. Two residuals worth flagging as follow-ups: the Agents page (KiroCrewAgentsPage.tsx:546,636) and CrewOverviewPane still key their "shared" badges on the roster inference this PR's own rationale rejects, and the client compares against the literal 'default' while the backend's real fact is config.default_memory_store.

Design-Verdict: PASS

Accurate two-layer disclosure keyed on a config fact, verified against the backend split; right scope, alternatives weighed, wording pinned by tests.

Suggestions

  • KiroCrewAgentsPage.tsx:546/636 and CrewOverviewPane still derive memoryShared from roster membership, the exact inference this PR's rationale rejects; a dedicated-store agent shows no shared badge there while the drawer says "still known to every member" — align those surfaces in a follow-up.
  • The client tests memory_store === 'default' literally, but the backend's shared store is config.default_memory_store; have the members endpoint send a resolved is-default flag so a renamed default store doesn't get the "rather than the shared default" wording.

[DESIGN-REVIEWED] 66f9708

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

UX-level review of 66f9708d277ac9c74ab57edfa758cf033ffb9af3 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The diff is small and focused: one new i18n string (memory_dedicated_note), a store-keyed conditional replacing the single shared-memory sentence in the Members drawer, a new capture frame, and tests. Reconciling against the blind read: the only user-visible change is the dedicated-store disclosure note, it appears in the committed screenshot (shot-01 = 07-dedicated-store-dark.png), and the blind reader parsed it correctly and confidently — "scout keeps some of its own notes, but anything I say to scout is still shared with all the other helpers … sure it's a warning, and I find it important." That matches exactly what the diff implements and what the PR claims. It's a static informational note, not a control; no state-transition of a persistent element is involved (the wording varies per member, not in place). The reader's remaining confusions ($skill hint, "auto" slider, count boxes, "Crew" polysemy) are all pre-existing surface outside this PR. No misleading copy, no hedging (the note asserts "not separated yet" as fact), no evidence gaps.

UX-Verdict: PASS

The store-aware disclosure reads exactly as intended: a cold reader correctly understood both halves — own notes separate, conversation still shared — and called it important.

[UX-REVIEWED] 66f9708

@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 Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 66f9708d277ac9c74ab57edfa758cf033ffb9af3 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 66f9708

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 66f9708d277ac9c74ab57edfa758cf033ffb9af3: <one-sentence reason>

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

@CrysisDeu — triage note on the current red board, so the next push can target the right thing.

What I verified on origin/main

The Design Review and First Principles lanes both BLOCK on the same premise, and the code agrees with them:

  • src/kiro_crew/context.py:1853-1857 — every named memory_store is handed the default store's VectorMemoryStore ("Share the global VectorMemoryStore so all agents get semantic/episodic reads"). Semantic and episodic memory are therefore global across all stores.
  • src/kiro_crew/history_consolidation.py — zero occurrences of memory_store; consolidation routes what a user says to any member into that shared store by workspace, not by member store.
  • docs/request-for-change/rfc-workspace-config-evolution.md:18 — records that per-store memory.db/memory.faiss isolation was affirmatively reversed by commit 7d1ff74e.

So memory_store isolates only the read side of the markdown layer (preferences / projects / lessons). "What you tell one member is known to all of them" is still true for a member on a dedicated store. Gating the note on memoryShared replaces a true warning with an implied isolation guarantee that does not exist — which is what both blocking lanes object to. The Opus lane filed the same point as advisory, and the UX lane flags that the copy overclaims ("all of them") even where it does render.

Paths forward (author's call)

  1. Store-aware copy instead of hiding — keep a note on dedicated-store members, reworded to say what is actually isolated (markdown prefs/lessons) and what still flows into shared memory (conversational learnings). Satisfies all four lanes and preserves the PR's motivation; costs i18n changes across all locales.
  2. Close this PR — keep the unconditional note (the First Principles "Subtraction"). Smallest honest version; the original request goes unaddressed.
  3. Wait for real isolation — land RFC Phase 3 (per-store vector memory) first, then revisit this gate driven by a backend-reported isolation flag rather than a client-side roster heuristic.

Mechanical reds, independent of the above (worth fixing in the same push whichever path is chosen):

  • Frontend Lint: main tightened eslint to --max-warnings 0; a rebase is needed and touched files must be warning-free.
  • PR Hygiene: fix PRs now require a ## Pattern harvest section in the body.
  • Dependency Audit: removed from PR CI on main (ci: run the npm audit gate on releases only; retry transient faults #8362); the old red clears on rebase.
  • temp-screenshots/crew-members/05-thread-wide-1920.png is flagged as an undeclared rider — drop or declare it.
  • GPT 5.6 Review: harness produced no verdict; a re-run is enough.
  • Backend Tests (Windows) (3): test_session_control.py rate-limit failure, unrelated to this frontend-only diff.

Not rebasing or pushing on your branch until you pick a direction.

The drawer's disclosure ('Members currently share one memory. What you
tell one member is known to all of them.') rendered the same sentence
for a member configured with its own memory_store, where it is only
half the story: the markdown layer (preferences, project notes) is read
from that store, while conversation memory and lessons stay in the one
global vector store every member reads (context.py get_memory_for hands
every named store the default's VectorMemoryStore).

An earlier revision hid the note on a dedicated store. Review lanes
rejected that: the sharing claim is still true at the vector layer, so
silence implied an isolation that does not exist. This revision keeps
the note on every member and words it for the store instead:

- default store: the existing sentence, unchanged.
- named store: 'Preferences and project notes come from the {store}
  store rather than the shared default. Conversation memory and lessons
  are not separated yet: what you tell this member is still known to
  every member.'

The wording keys on the store itself (a config fact), not on a roster
heuristic of who else uses the store — whether a store is shared is a
backend property of which layers it scopes.

New key pages.membersPage.memory_dedicated_note in all twelve
hand-maintained catalogs plus regenerated en-XA. Capture: scout moves to
memory_store 'scout-own' and frame 07 asserts the store-aware wording
there; frame 02 (radar, default store) remains the counterpart.
@CrysisDeu CrysisDeu changed the title fix(members): render the shared-memory disclosure only where it is true fix(members): word the memory disclosure for the member's store Sep 6, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/members-memory-disclosure branch from b8db681 to 66f9708 Compare September 6, 2026 00:55
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 6, 2026

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tech Lead review — approved.

Privacy-adjacent correctness (the thing that matters here): the copy is accurate. I verified the backend independently rather than trusting the description: context.py get_memory_for hands every named store the default store's VectorMemoryStore ("Share the global VectorMemoryStore so all agents get semantic/episodic reads"), and get_lessons_for keys on workspace, never on memory_store. So a named memory_store scopes only the markdown read side (preferences, project notes) while conversation memory and lessons remain global. The new memory_dedicated_note says exactly that, and — critically — it does not stop saying the shared part: "what you tell this member is still known to every member." No implied isolation guarantee is created, which is what the earlier hide-the-note revision got wrong and what the Design / First Principles lanes correctly blocked on.

Store identity is a config fact, not a roster inference. Deleting memoryShared is the right call: whether a store isolates a layer is a backend property, and the client had no business guessing it from who else is on the store. The second test pins that (two members on triage still get the store-scoped wording).

Edge case checked and dismissed: String(active.memory_store || 'default') === 'default' treats an explicitly-empty memory_store as default, which mirrors the backend's own memory_store or workspace fallback closely enough; the config loader and the create handler both default the field to "default", so an empty value is not a shipped state.

Scope and hygiene: 17 files but only 3 are substantive — MembersPage.tsx (+11/-2), its test (+32), and the capture script; the other 13 are the single new key across all twelve hand-maintained catalogs plus a regenerated en-XA, per the i18n rule. One commit, ## Pattern harvest present, all 57 checks green, all four AI lanes PASS on 66f9708, zero prior approvals. The 2026-09-04 triage note describes the superseded red board and its path 1 is what shipped here.

Approving as a kirocrew-ux-reviewers member for the website/src/** UX gate.

@bolichen97
bolichen97 merged commit 6c7578e into main Sep 6, 2026
65 checks passed
@bolichen97
bolichen97 deleted the fix/members-memory-disclosure branch September 6, 2026 01:54
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants