diff --git a/openspec/changes/enforce-message-cap-at-data-layer/.openspec.yaml b/openspec/changes/enforce-message-cap-at-data-layer/.openspec.yaml new file mode 100644 index 00000000..44f55ffe --- /dev/null +++ b/openspec/changes/enforce-message-cap-at-data-layer/.openspec.yaml @@ -0,0 +1,2 @@ +schema: spec-driven +created: 2026-08-23 diff --git a/openspec/changes/enforce-message-cap-at-data-layer/design.md b/openspec/changes/enforce-message-cap-at-data-layer/design.md new file mode 100644 index 00000000..0e339a29 --- /dev/null +++ b/openspec/changes/enforce-message-cap-at-data-layer/design.md @@ -0,0 +1,67 @@ +## Context + +The `MessageList` component in `src/tui/messageList.js` manages conversation messages through four ref-based data structures. Currently, messages are added without any cap at the data layer — the 100-message limit is enforced only at render time via `idsRef.current.slice(-MAX_RENDER_MESSAGES)`. This means the internal state grows unbounded while the UI only displays the last 100 messages. + +## Goals / Non-Goals + +**Goals:** +- Enforce the 100-message cap at the data layer in `addMessage()` and `setMessages()`. +- Clean up orphaned data (dataRef, contentRef, pub/sub topics) when messages are shifted off. +- Maintain backward compatibility with the existing imperative API surface. +- Add regression tests for the cap enforcement. + +**Non-Goals:** +- Making the cap value configurable or parameterized. +- Adding pagination or "load older messages" functionality. +- Modifying session persistence/restore logic outside of messageList.js. +- Changing the render-layer slice (kept as defensive measure). + +## Decisions + +### Decision 1: Enforce cap in addMessage() after push +After pushing the new message ID to `idsRef.current`, check if length exceeds `MAX_RENDER_MESSAGES`. If so, shift off the oldest ID from index 0 and clean up all associated data structures. + +**Rationale:** This is the simplest and most efficient approach. A single check after push handles the common case of one message at a time. The `shift()` operation is O(n) but n is bounded at 100, so the cost is negligible. + +**Alternatives considered:** +- Using a circular buffer: More complex, unnecessary for a fixed cap of 100. +- Using `splice(0, 1)` instead of `shift()`: Functionally equivalent, `shift()` is more idiomatic. + +### Decision 2: Truncate in setMessages() after building full list +After iterating through all input messages and building the internal state, truncate `idsRef.current` to the last `MAX_RENDER_MESSAGES` entries and rebuild `idToIdxRef`. + +**Rationale:** Session restore may pass more than 100 messages. Truncating at the end ensures we keep the most recent 100, consistent with the render-layer behavior. + +**Alternatives considered:** +- Truncating during the build loop: Would require tracking indices dynamically, more complex. +- Accepting only 100 messages from the caller: Shifts responsibility to callers, breaks encapsulation. + +### Decision 3: Clean up orphaned data on shift +When shifting off a message, delete its entries from `dataRef`, `contentRef`, and its pub/sub topic from `topicsRef`. + +**Rationale:** Prevents memory leaks from orphaned message data. The pub/sub topic cleanup is critical — stale callbacks firing on orphaned message IDs could cause unexpected behavior. + +## Risks / Trade-offs + +### Risk: Streaming message updates on shifted messages +If a streaming assistant message is shifted off before completion, its pub/sub topic is deleted and the streaming updates will be lost. + +**Mitigation:** This is acceptable — the message was shifted because it's no longer visible in the UI. Streaming updates for shifted messages are a corner case that the user won't observe. The `updateMessage()` API will silently no-op for shifted messages since `idToIdxRef` won't contain them. + +### Risk: Index invalidation after shift +After shifting, all remaining message indices change by -1. `idToIdxRef` must be rebuilt to reflect new indices. + +**Mitigation:** Rebuild `idToIdxRef` from scratch after shifting by iterating `idsRef.current` and setting each index. This is O(n) with n ≤ 100, negligible cost. + +### Risk: Session restore truncation +If a session has more than 100 messages, only the last 100 will be retained in memory. + +**Mitigation:** This is consistent with existing render-layer behavior — the user never saw more than 100 messages anyway. The conversation history is preserved in the session checkpoint; only the in-memory TUI state is truncated. + +## Migration Plan + +No migration needed. This is a pure code change with no data migration. The behavioral change (messages now actually removed from memory) is consistent with existing render-layer behavior and is invisible to end users. + +## Open Questions + +None. The implementation approach is clear and well-defined by the audit findings and fix steps. \ No newline at end of file diff --git a/openspec/changes/enforce-message-cap-at-data-layer/proposal.md b/openspec/changes/enforce-message-cap-at-data-layer/proposal.md new file mode 100644 index 00000000..983a6e13 --- /dev/null +++ b/openspec/changes/enforce-message-cap-at-data-layer/proposal.md @@ -0,0 +1,32 @@ +## Why + +The conversation panel in `src/tui/messageList.js` enforces a 100-message limit only at the render layer via `idsRef.current.slice(-MAX_RENDER_MESSAGES)`. Messages are never removed from the underlying data structures (`idsRef`, `idToIdxRef`, `dataRef`, `contentRef`), causing unbounded memory growth as conversations accumulate messages. Long-running sessions can accumulate thousands of messages in memory, even though only 100 are visible. + +## What Changes + +- Enforce the 100-message cap at the data layer in `addMessage()` — shift off the oldest message when the array exceeds `MAX_RENDER_MESSAGES`. +- Enforce the 100-message cap at the data layer in `setMessages()` — truncate the input to the last 100 messages before building internal state. +- Clean up orphaned data (`dataRef`, `contentRef`, pub/sub topics) when messages are shifted off. +- Add regression tests verifying the cap at the data layer. + +## Capabilities + +### New Capabilities +- `message-cap`: Defines the requirement that the MessageList data layer enforces a maximum message count, ensuring memory is bounded regardless of conversation length. + +### Modified Capabilities +- None + +## Impact + +- **Affected code:** `src/tui/messageList.js` — `addMessage()` and `setMessages()` imperative APIs. +- **Affected tests:** `tests/unit/messageListApi.test.js` — new test cases for cap enforcement. +- **Behavioral change:** Sessions with more than 100 messages will now only retain the last 100 in memory, consistent with existing render-layer behavior. +- **Non-breaking:** The public imperative API surface (`addMessage`, `setMessages`, `getMessageCount`) remains unchanged. Only internal state management differs. + +## Non-goals + +- Changing the value of `MAX_RENDER_MESSAGES` — that remains a configurable constant. +- Adding a configurable cap limit — the cap value is not parameterized in this change. +- Modifying session persistence/restore logic outside of `messageList.js`. +- Adding pagination or "load older messages" functionality. \ No newline at end of file diff --git a/openspec/changes/enforce-message-cap-at-data-layer/specs/message-cap/spec.md b/openspec/changes/enforce-message-cap-at-data-layer/specs/message-cap/spec.md new file mode 100644 index 00000000..dd139ed2 --- /dev/null +++ b/openspec/changes/enforce-message-cap-at-data-layer/specs/message-cap/spec.md @@ -0,0 +1,57 @@ +## ADDED Requirements + +### Requirement: MessageList enforces data-layer message cap on addMessage +The MessageList imperative API SHALL enforce a maximum message count at the data layer when adding a new message via `addMessage()`. When the internal message array exceeds `MAX_RENDER_MESSAGES` (100), the oldest message ID SHALL be removed from the beginning of the array, and all associated data structures SHALL be cleaned up. + +#### Scenario: Add message when under cap +- **WHEN** the current message count is less than `MAX_RENDER_MESSAGES` (100) +- **THEN** the new message is appended to the end of the array and `getMessageCount()` returns the incremented count + +#### Scenario: Add message when at cap +- **WHEN** the current message count equals `MAX_RENDER_MESSAGES` (100) +- **THEN** the oldest message ID is shifted off the beginning, the new message is appended, and `getMessageCount()` returns `MAX_RENDER_MESSAGES` (100) + +#### Scenario: Add message when over cap +- **WHEN** the current message count exceeds `MAX_RENDER_MESSAGES` (100) — e.g., after a session restore with >100 messages +- **THEN** the oldest message ID is shifted off, the new message is appended, and `getMessageCount()` returns `MAX_RENDER_MESSAGES` (100) + +#### Scenario: Oldest message data is cleaned up on shift +- **WHEN** a message ID is shifted off the beginning of the array +- **THEN** the entry is removed from `dataRef`, `contentRef`, `idToIdxRef`, and its pub/sub topic from `topicsRef` + +### Requirement: MessageList enforces data-layer message cap on setMessages +The MessageList imperative API SHALL enforce a maximum message count at the data layer when initializing from a messages array via `setMessages()`. If the input array exceeds `MAX_RENDER_MESSAGES`, only the last `MAX_RENDER_MESSAGES` entries SHALL be retained. + +#### Scenario: Set messages when under cap +- **WHEN** the input message array has fewer than `MAX_RENDER_MESSAGES` (100) entries +- **THEN** all messages are retained and `getMessageCount()` returns the input array length + +#### Scenario: Set messages when over cap +- **WHEN** the input message array exceeds `MAX_RENDER_MESSAGES` (100) entries +- **THEN** only the last 100 messages are retained, `getMessageCount()` returns 100, and the oldest messages are excluded from all data structures + +#### Scenario: Set messages preserves message order +- **WHEN** the input message array is truncated to the last 100 entries +- **THEN** the relative order of the retained messages is preserved (first retained message is at index 0, last at index 99) + +### Requirement: updateMessage() is a no-op for shifted messages +The MessageList imperative API SHALL silently ignore `updateMessage()` calls for message IDs that have been shifted off the data layer. The function SHALL check `idToIdxRef` for the ID and return early if not found. + +#### Scenario: Update shifted message is no-op +- **WHEN** a message has been shifted off the data layer (e.g., during streaming) +- **THEN** calling `updateMessage()` with that ID does nothing — no error, no crash, no state change + +#### Scenario: Update retained message works normally +- **WHEN** a message is still within the data layer (not shifted off) +- **THEN** `updateMessage()` updates the message data and triggers a pub/sub notification as before + +### Requirement: Render layer cap remains as defensive measure +The render layer SHALL continue to use `idsRef.current.slice(-MAX_RENDER_MESSAGES)` to determine which messages to render, providing a defensive layer independent of the data-layer cap. + +#### Scenario: Render slice matches data layer +- **WHEN** the data layer has enforced the cap (count ≤ 100) +- **THEN** `idsRef.current.slice(-MAX_RENDER_MESSAGES)` returns all messages (no-op truncation) + +#### Scenario: Render slice protects against data layer bypass +- **WHEN** the data layer somehow exceeds the cap (e.g., external mutation) +- **THEN** the render layer still only renders the last 100 messages \ No newline at end of file diff --git a/openspec/changes/enforce-message-cap-at-data-layer/tasks.md b/openspec/changes/enforce-message-cap-at-data-layer/tasks.md new file mode 100644 index 00000000..6e839ee9 --- /dev/null +++ b/openspec/changes/enforce-message-cap-at-data-layer/tasks.md @@ -0,0 +1,31 @@ +## 1. Implement data-layer cap in addMessage() + +- [ ] 1.1 After pushing the new message ID to `idsRef.current`, add a check: if length exceeds `MAX_RENDER_MESSAGES`, shift off the oldest ID from index 0. +- [ ] 1.2 When shifting off an ID, clean up `dataRef`, `contentRef`, `idToIdxRef`, and the pub/sub topic from `topicsRef`. +- [ ] 1.3 Rebuild `idToIdxRef` indices after shifting (all remaining indices decrease by 1). + +## 2. Make updateMessage() a no-op for shifted messages + +- [ ] 2.1 In `updateMessage()`, verify the message ID exists in `idToIdxRef` before proceeding (early return if not found). +- [ ] 2.2 Add a test case verifying that updating a shifted message is a no-op. + +## 2. Implement data-layer cap in setMessages() + +- [ ] 2.1 After building the full internal state from the input array, check if `idsRef.current.length` exceeds `MAX_RENDER_MESSAGES`. +- [ ] 2.2 If over cap, truncate `idsRef.current` to the last 100 entries, rebuild `idToIdxRef`, and prune `dataRef`, `contentRef`, and pub/sub topics for removed messages. + +## 3. Add regression tests + +- [ ] 3.1 Test: addMessage when under cap — verify count increments correctly. +- [ ] 3.2 Test: addMessage when at cap — verify oldest message is shifted, count stays at 100. +- [ ] 3.3 Test: addMessage when over cap — verify oldest message is shifted, count stays at 100. +- [ ] 3.4 Test: setMessages when over cap — verify only last 100 retained, count is 100. +- [ ] 3.5 Test: setMessages preserves order — verify retained messages maintain relative order. +- [ ] 3.6 Test: orphaned data cleanup — verify dataRef, contentRef, idToIdxRef, and topicsRef are cleaned up on shift. + +## 4. Verify and commit + +- [ ] 4.1 Run `npm run test` — all tests pass. +- [ ] 4.2 Run `npm run lint` — no lint errors. +- [ ] 4.3 Run `npm run coverage` — coverage maintained. +- [ ] 4.4 Commit and push implementation code to the PR. \ No newline at end of file