Skip to content

fix(notifications): enforce the inbox's size bounds in NotificationCenter.post (#1666) - #1670

Merged
fluck-boss merged 2 commits into
risa-labs-inc:devfrom
arjun28115:fix/notification-store-bounds
Sep 24, 2026
Merged

fluck-boss merged 2 commits into
risa-labs-inc:devfrom
arjun28115:fix/notification-store-bounds

Conversation

@arjun28115

@arjun28115 arjun28115 commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Closes #1666.

#1644 bounded notification_post at the MCP provider, and #1660 put the analogous snippet caps in the store. So two adjacent fixes landed on opposite conventions. This one also left a real hole: NotificationCenter.post is documented as serving the host too, and a HOST-origin post met none of the three bounds. The first host publisher would have reopened what #1644 closed, through a call that reviews as innocuous.

The change

  • MAX_TITLE_CHARS (200) and MAX_MESSAGE_CHARS (4,000) move to NotificationCenter, so nothing stored exceeds them whoever posts. The inbox is rewritten in full on every post and listed without approval. Whether an over-long field is refused or cut depends on who wrote it and how:

    Field HOST AGENT
    title refused (a literal, so an over-long one is a bug) refused (the MCP tool checks first and names the limit)
    message cut to the cap, ending in an ellipsis, never splitting a surrogate pair. A host builds it from dynamic text, and refusing would turn a notice about one problem into a second problem on an error path refused (the tool checks first)
    source label flattened to one line, trimmed, then refused over 80 flattened, prefixed, then cut (fix(notifications): stamp notification source provenance at the service boundary #1619)

    The label cap is measured on the stamped label, which is exactly what is stored. post's KDoc states this per field, with @throws for the three refusals, and MAX_ENTRIES states the aggregate bound: about 860,000 characters, more once JSON-escaped.

  • NotificationMcpToolProvider.tooLong stays as the layer that runs first, so an agent's refusal still names the field and its limit instead of surfacing the store's exception. The provider imports the store's constants, so the two can't drift.

Today's only production caller of post is the MCP tool, an AGENT-origin post, and every value it can pass is already within these bounds. So nothing that works now changes.

Tests

In NotificationCenterTest:

  • an over-long title from either origin, and an over-long agent message, are refused, and nothing is stored
  • an over-long host message is cut to the cap and ends in an ellipsis, and a cut landing on a surrogate pair drops the whole pair
  • all three fields exactly at their limits are stored intact, not merely accepted
  • an over-long label is refused from the host and truncated from an agent. A host label padded with whitespace at the cap is accepted and stored trimmed, and a host label with a newline is flattened

In NotificationMcpToolProviderTest: MAX_LIST_LIMIT < NotificationCenter.MAX_ENTRIES is pinned directly (the review's nit). It is the invariant the list clamp depends on, and it now fails with a sentence rather than a count mismatch.

Mutation-checked: removing each require, storing the host message uncut, dropping the surrogate guard, not flattening the host label, and measuring it untrimmed each fail a named test. 420 tests across ai.rever.boss.notifications.* and ai.rever.boss.mcp.* pass. ktlintCheck and detekt are clean.

🤖 Generated with Claude Code

@fluck-boss

Copy link
Copy Markdown
Collaborator

@claude review

@claude

claude Bot commented Sep 24, 2026 •

Copy link
Copy Markdown

Claude finished @fluck-boss's task in 3m 30s —— View job


Review: enforce the inbox's size bounds in NotificationCenter.post


Verdict: no blocking issues. The change does what it says, and it lands on the right side of the convention split it names. I confirmed the parity claim directly: SnippetLibraryManager uses the same shape - public const val caps on the store, require(x.length <= MAX_…) { "… is over N characters" } before the mutex, same message wording - so after this the two adjacent stores read identically. I also confirmed NotificationMcpToolProvider.handlePost is the only production caller of post today (composeApp/src/commonMain/kotlin/ai/rever/boss/mcp/NotificationMcpToolProvider.kt:176), and every value it passes is pre-checked by tooLong, so the behaviour claim "nothing that works now changes" holds.

Things done right that are easy to get wrong: the three requires sit outside mutex.withLock (NotificationCenter.kt:131-137), so nothing throws with the lock held; the HOST label check uses source.trim().length, which is exactly what stampedSource stores, so the predicate cannot refuse a label that would have fitted; and keeping tooLong first is genuinely load-bearing for message quality - McpToolRegistryImpl.executeUncapped does catch handler throwables, so a divergence would surface as a generic caught-exception error result rather than the named field and limit.

Five notes, in order of how much I'd want them addressed.


1. message is the one field where a hard refusal is likely to bite a host caller (design, worth a decision)

NotificationCenter.kt:133. The asymmetry argument in the KDoc - "an agent is not ours to fix, while a host label that long is a bug to catch at the call site" - is sound for title and source. Both are, in practice, literals the call site authors, so an over-long one really is a typo you want to trip over in testing.

message is different: it is the field a host call site will build from dynamic content - an exception message, a command's tail, release notes, a diff summary. The first such caller gets an IllegalArgumentException out of a suspend call on what is usually an error or completion path, i.e. the notification about the problem is replaced by a second problem, and if it is inside a bare launch it is a silent coroutine failure rather than a visible one.

Two ways out, either fine:

  • Truncate message for HOST (message.take(MAX_MESSAGE_CHARS)) and keep the hard refusal for title and source; or
  • Keep the refusal and say so in the KDoc, explicitly: a host caller building a message from dynamic text must cap it itself.

Either way, post now has four preconditions and no @throws tag. Adding @throws IllegalArgumentException to the KDoc is cheap and is what a future host caller will read.

Fix this →

2. MAX_ENTRIES should now state the aggregate bound, the way the sibling does (consistency)

NotificationCenter.kt:36-37 still reads only "Newest entries kept; older ones are dropped on the next post." The store this PR is aligning with spells out the number to actually agree to (SnippetLibraryManager.kt:43-46): "500 x (200 + 20,000 + 500) characters, about 10 MB … rewritten in full on every save and parsed synchronously by loadSync at startup. MAX_BODY_CHARS is the lever if that is too much."

That sentence is only now available here, because the caps only now live here: 200 x (200 + 4,000 + 80) ≈ 860 KB, rewritten in full on every post and parsed synchronously at startup. Since the PR's whole thesis is that these two stores should read the same way, this is the paragraph that completes it.

One caveat to get right if you write it: unlike the agent label, message is never flattened, so JSON escaping of control characters (6 chars per \uXXXX) can multiply the worst case several times over the nominal 860 KB. Still bounded, just not 860 KB - better to say "about 860 KB of text, more on disk once escaped" than to quote a figure that a pathological message beats.

3. A HOST label is now length-checked but still not flattened (low, pre-existing, more conspicuous after this)

stampedSource applies unsafeLabelChars to AGENT only (NotificationCenter.kt:196-212), so a HOST label containing \n still renders as multiple lines wherever the inbox prints it - the exact thing the flatten defends against for agents. Lower risk, since host text is review-visible rather than attacker-controlled, and it predates this PR.

But the new HOST-only require sharpens the question: if the argument for refusing an 81-character host label is "that is a bug, catch it at the call site that writes it", the same argument covers a host label with a newline in it, and that one is currently silent. Either flatten for HOST too, or add a clause to the KDoc recording the gap as deliberate.

4. fields exactly at their limits are accepted asserts only one of the three (test nit)

NotificationCenterTest.kt:368-380 ends at assertEquals(MAX_MESSAGE_CHARS, posted.message.length). The title and the source at exactly their limits are covered only by the absence of a throw, so nothing pins that an accepted value is stored intact rather than quietly truncated somewhere downstream. Two more assertEquals on posted.title.length and posted.source.length finish the test its name promises.

The gap I'd actually add a case for is the trim interaction on the host label: source = " " + "s".repeat(80) + " " must be accepted, because stampedSource stores the trimmed form. That is the one place the boundary predicate can be got wrong later - dropping the .trim() from the require refuses a label that stores within bounds, and no current test notices.

Fix this →

5. The MAX_LIST_LIMIT < MAX_ENTRIES pin is the right shape (informational)

NotificationMcpToolProviderTest.kt:218-225. Both sides are const val, so the comparison is inlined into the test class at compile time - which is fine, because Gradle recompiles dependents of a changed const, so editing MAX_ENTRIES does re-evaluate it. Worth knowing rather than worth changing.


Checks I did not run. I read the code and the configuration but did not execute Gradle in this environment, so the PR's own claims (416 tests, ktlintCheck and detekt clean) are unverified by me. Two things I did verify statically: max_line_length = 140 in .editorconfig, and the longest new/edited line is about 113 characters, so neither the widened [NotificationCenter.post] KDoc reference nor the new constant docs can trip the limit; and composeApp/detekt-baseline.xml holds no entry for NotificationCenter, so the KDoc edit inside post cannot invalidate a baselined signature.

No changes were pushed - this is review feedback only.
· branch fix/notification-store-bounds

arjun28115 and others added 2 commits September 24, 2026 21:48
…nter.post (risa-labs-inc#1666)

risa-labs-inc#1644 bounded notification_post at the MCP provider, and risa-labs-inc#1660 put the snippet caps in the
store. Two adjacent fixes on opposite conventions, and this one left a hole: post is
documented as serving the host too, and a HOST-origin post met none of the three bounds.

- MAX_TITLE_CHARS and MAX_MESSAGE_CHARS move to NotificationCenter and are required in
  post beside the existing blank-title check, for every origin.
- A HOST source label longer than MAX_SOURCE_LABEL_CHARS is refused. An agent's label
  keeps risa-labs-inc#1619's truncation: an agent is not ours to fix, while a host label that long is
  a bug to catch where it is written.
- The MCP tool keeps its check first so its refusal names the field and the limit.
- Tests: over-long title and message refused from both origins with nothing stored; all
  three exactly at their limits accepted; host label refused while an agent's is cut.
  The provider test pins MAX_LIST_LIMIT < MAX_ENTRIES, which the clamp depends on.

Mutation-checked: removing each of the three requires fails a named test. 416 tests
across ai.rever.boss.notifications.* and ai.rever.boss.mcp.* pass; ktlint and detekt clean.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…latten host labels

Review follow-up on risa-labs-inc#1670.

- A host message over MAX_MESSAGE_CHARS is cut, ending in an ellipsis, never splitting a
  surrogate pair. A host builds its message from dynamic text (an exception message, a
  command's output), and refusing would turn a notice about one problem into a second
  problem on an error path. An agent's message is still refused, because the MCP tool
  checks first and names the limit. Titles are still refused from either origin.
- A host label is flattened to one line like an agent's, and the cap is measured on the
  stamped label, which is exactly what is stored. So trimmed whitespace cannot tip a
  label over.
- post's KDoc states the rule per field and origin, with @throws for the three refusals.
  MAX_ENTRIES states the aggregate bound, about 860,000 characters and more once escaped,
  as SnippetLibraryManager's does.
- Tests: the at-limit test asserts every field is stored intact. New cases cover a host
  label padded with whitespace at the cap, a host label with a newline, a host message
  cut with its ellipsis, and a cut that lands on a surrogate pair.

Mutation-checked: storing the host message uncut, dropping the surrogate guard, not
flattening the host label, measuring it untrimmed, and not refusing the agent message
each fail a named test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@arjun28115
arjun28115 force-pushed the fix/notification-store-bounds branch from 38308ac to 7a369be Compare September 24, 2026 16:24
@arjun28115

Copy link
Copy Markdown
Contributor Author

Thanks. All four asks are in, in 7a369be, rebased onto current dev.

1. A host message is cut, not refused. Your framing is right: a notice about a problem shouldn't fail into a second one. A HOST message over 4,000 characters is cut to the cap and ends in an ellipsis. The cut never splits a surrogate pair, since half an emoji would be stored as a lone surrogate. An AGENT message is still refused, because the MCP tool checks first and names the limit. Titles are still refused from both origins, since they are literals. post's KDoc states the rule per field and origin, and @throws IllegalArgumentException names the three refusals.

2. MAX_ENTRIES states the aggregate: 200 x (200 + 4,000 + 87) characters, about 860,000. I took your caveat: it says "more on disk once JSON-escaped", since a message isn't flattened and a control character escapes to six. As in SnippetLibraryManager, it names MAX_MESSAGE_CHARS as the lever.

3. Host labels are flattened like an agent's, rather than recording the gap. A newline in a host label now renders as one line. The cap is measured on the stamped label, which is exactly what is stored, so that one value is both checked and stored.

4. Tests:

  • The at-limit test now asserts title, message and label are stored intact.
  • The trim edge you named: a host label of exactly 80 characters padded with whitespace is accepted and stored trimmed.
  • New cases for the cut message, the surrogate boundary and the flattened host label.

Mutation check: each of these fails a named test:

  • storing the host message uncut
  • dropping the surrogate guard
  • not flattening the host label
  • measuring the label untrimmed
  • not refusing the agent message

All 26 of NotificationCenterTest's @Test methods ran. After #1667 I count them against the results rather than trusting a green build. 420 tests across ai.rever.boss.notifications.* and ai.rever.boss.mcp.* pass, and ktlint and detekt are clean.

@fluck-boss

Copy link
Copy Markdown
Collaborator

Merging. Gate complete: CI green on the head, Claude review has nothing blocking, my own read clean.

@fluck-boss
fluck-boss merged commit 0685139 into risa-labs-inc:dev Sep 24, 2026
8 checks passed
kshivang pushed a commit that referenced this pull request Sep 24, 2026
…nter.post (#1666) (#1670)

* fix(notifications): enforce the inbox's size bounds in NotificationCenter.post (#1666)

#1644 bounded notification_post at the MCP provider, and #1660 put the snippet caps in the
store. Two adjacent fixes on opposite conventions, and this one left a hole: post is
documented as serving the host too, and a HOST-origin post met none of the three bounds.

- MAX_TITLE_CHARS and MAX_MESSAGE_CHARS move to NotificationCenter and are required in
  post beside the existing blank-title check, for every origin.
- A HOST source label longer than MAX_SOURCE_LABEL_CHARS is refused. An agent's label
  keeps #1619's truncation: an agent is not ours to fix, while a host label that long is
  a bug to catch where it is written.
- The MCP tool keeps its check first so its refusal names the field and the limit.
- Tests: over-long title and message refused from both origins with nothing stored; all
  three exactly at their limits accepted; host label refused while an agent's is cut.
  The provider test pins MAX_LIST_LIMIT < MAX_ENTRIES, which the clamp depends on.

Mutation-checked: removing each of the three requires fails a named test. 416 tests
across ai.rever.boss.notifications.* and ai.rever.boss.mcp.* pass; ktlint and detekt clean.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* fix(notifications): cut a long host message instead of refusing it; flatten host labels

Review follow-up on #1670.

- A host message over MAX_MESSAGE_CHARS is cut, ending in an ellipsis, never splitting a
  surrogate pair. A host builds its message from dynamic text (an exception message, a
  command's output), and refusing would turn a notice about one problem into a second
  problem on an error path. An agent's message is still refused, because the MCP tool
  checks first and names the limit. Titles are still refused from either origin.
- A host label is flattened to one line like an agent's, and the cap is measured on the
  stamped label, which is exactly what is stored. So trimmed whitespace cannot tip a
  label over.
- post's KDoc states the rule per field and origin, with @throws for the three refusals.
  MAX_ENTRIES states the aggregate bound, about 860,000 characters and more once escaped,
  as SnippetLibraryManager's does.
- Tests: the at-limit test asserts every field is stored intact. New cases cover a host
  label padded with whitespace at the cap, a host label with a newline, a host message
  cut with its ellipsis, and a cut that lands on a surrogate pair.

Mutation-checked: storing the host message uncut, dropping the surrogate guard, not
flattening the host label, measuring it untrimmed, and not refusing the agent message
each fail a named test.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
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