fix(feedback): address backend review gaps

packages/feedback/message-feedback/README.i18n.yaml

@@ -2,5 +2,5 @@
 # side as of the last confirmed-consistent state. Both languages carry equal authority;
 # after editing either side, bring the other along and re-record with:
 #   pnpm run verify-translation-pairing --write packages/feedback/message-feedback/README.md
-README.md: 948cdcd2ef0fa2d1bcea68fdc40f16be08e963a8
-README.zh.md: 14e62eea0a182bc60be2c370653d1cb031d6b8a8
+README.md: a9ebad25907e32435c8d2a65eb4b2cd9eaa89eeb
+README.zh.md: 29cbee0c1ec2ee810948d895c762d2e6320e9b66

packages/feedback/message-feedback/README.md

@@ -12,7 +12,7 @@ Public request, value, version, and failure types are exported from the package
 |---|---|
 | `maxNoteBytes` | Required positive safe integer: maximum UTF-8 byte length of one optional note. |
 
-Notes must contain at least one non-whitespace character, but accepted text is stored verbatim rather than trimmed. Omitting `note` means the desired value has no note, so an authorized material `put` clears an existing note.
+Notes must contain at least one non-whitespace character, but accepted text is stored verbatim rather than trimmed. Omitting `note` means the desired value has no note, so a version-matched material `put` clears an existing note. Note validation precedes Session lookup and can therefore return `note-blank` or `note-too-large` for a missing Session without touching persistence.
 
 ```yaml
 - id: message-feedback
@@ -55,6 +55,8 @@ A matching-version no-op returns the already stored item with unchanged version
 
 A per-Session promise queue encloses inspection, durability validation, sidecar read, comparison, and whole-row write. These semantics serialize concurrent mutations through one service instance; storage-domain itself has no cross-process conditional write.
 
+Plugin disposal closes mutation admission, drains every operation already accepted into the per-Session queues, and only then closes the storage domain. A mutation submitted after disposal begins rejects as a lifecycle failure instead of entering a closing domain.
+
 ## Model Experience
 
 ### Local message-feedback state
@@ -79,3 +81,4 @@ Independent. Listing or mutating message feedback does not touch a model request
 - **Detach/catalog retirement window** — a request in the narrow interval after live detach but before the persistence catalog materializes the header can receive `session-not-found`; callers retry after retirement materialization.
 - **Header identity is not a content fingerprint** — `{createdAt, cwd}` detects reuse only when those fields differ; a cloned log retaining the same header identity is indistinguishable.
 - **Trusted caller boundary** — `list`/`put`/`delete` carry no authenticated actor or audit identity. A deployment must expose the Host gateway only through its trusted or separately authenticated boundary until authorization and attribution are added.
+- **Catalog and row bounds** — a cold request scans the complete Session snapshot catalog because persistence has no lookup-by-id metadata operation. `maxNoteBytes` bounds one note, but the item count and aggregate retained bytes of one Session row are not capped; an indexed metadata read and deployment-owned row bound remain deferred until a concrete consumer defines their policy.

packages/feedback/message-feedback/README.zh.md

@@ -12,7 +12,7 @@
 |---|---|
 | `maxNoteBytes` | 必填正 safe integer：一条可选备注的最大 UTF-8 字节长度。 |
 
-备注必须包含至少一个非空白字符，但通过校验的文本按原样存储，不会 trim。省略 `note` 表示目标值不含备注，因此通过授权的实质 `put` 会清除已有备注。
+备注必须包含至少一个非空白字符，但通过校验的文本按原样存储，不会 trim。省略 `note` 表示目标值不含备注，因此 version 匹配的实质 `put` 会清除已有备注。备注校验早于 Session 查找，因此即使 Session 不存在，也可能在不访问持久化的情况下返回 `note-blank` 或 `note-too-large`。
 
 ```yaml
 - id: message-feedback
@@ -55,6 +55,8 @@ message feedback 不是 Session 日志内容或 Session 投影。它不发出 `f
 
 按 Session 划分的 promise 队列覆盖检查、持久性校验、伴随记录读取、比较与整行写入。这些语义会串行化经由同一服务实例的并发变更；storage-domain 自身没有跨进程条件写。
 
+Plugin disposal 会先关闭变更接纳，排空已进入各个 Session 队列的所有操作，然后才关闭 storage domain。disposal 开始后提交的变更会以生命周期故障拒绝，不会进入正在关闭的 domain。
+
 ## 模型体验
 
 ### 本地消息反馈状态
@@ -79,3 +81,4 @@ message feedback 不是 Session 日志内容或 Session 投影。它不发出 `f
 - **Detach/catalog retirement 窗口**——请求若恰好落在 live detach 之后、persistence catalog 物化 header 之前的极短窗口，可能收到 `session-not-found`；调用方应在 retirement materialization 后重试。
 - **Header 身份不是内容指纹**——只有 `{createdAt, cwd}` 不同时才能识别复用；本契约无法区分保留相同 header 身份的克隆日志。
 - **调用方边界受信任**——`list`/`put`/`delete` 不携带已认证的 actor 或审计身份。在加入授权与归属信息前，部署方必须只通过受信任或另行认证的边界暴露 Host gateway。
+- **目录与行边界**——由于 persistence 没有按 id 读取元数据的操作，cold 请求会扫描完整的 Session snapshot 目录。`maxNoteBytes` 只限制单条备注，单个 Session 行的条目数和聚合保留字节尚无上限；按索引读取元数据和由部署决定的行边界，延后到具体消费方明确策略时处理。

packages/feedback/message-feedback/src/index.ts

@@ -158,6 +158,7 @@ export class MessageFeedbackService extends GatewayService {
   private readonly maxNoteBytes: number
   private table?: KvTable<SessionId, MessageFeedbackRow>
   private readonly operationTails = new Map<SessionId, Promise<void>>()
+  private mutationAdmissionOpen = true
 
   /**
    * @param ctx - Host context carrying persistence and the storage-domain form.
@@ -171,7 +172,11 @@ export class MessageFeedbackService extends GatewayService {
   /** Open and own the one message-feedback sidecar domain. */
   protected async [Service.init](): Promise<void> {
     const domain = await this.ctx.storageDomain.open(messageFeedbackDomainSpec)
-    this.ctx.effect(() => () => domain.close(), 'message-feedback.domainClose')
+    this.ctx.effect(() => async () => {
+      this.mutationAdmissionOpen = false
+      await Promise.all(this.operationTails.values())
+      await domain.close()
+    }, 'message-feedback.domainClose')
     this.table = domain.table('sessions')
   }
 
@@ -354,6 +359,9 @@ export class MessageFeedbackService extends GatewayService {
 
   /** Queue a complete read/compare/write mutation behind this Session's prior mutation. */
   private enqueue<T>(sessionId: SessionId, operation: () => Promise<T>): Promise<T> {
+    if (!this.mutationAdmissionOpen) {
+      return Promise.reject(new Error('message-feedback: service is disposing'))
+    }
     const previous = this.operationTails.get(sessionId) ?? Promise.resolve()
     const result = previous.then(operation)
     const tail = result.then(() => undefined, () => undefined)

packages/feedback/message-feedback/src/spec.ts

@@ -22,6 +22,8 @@ export const messageFeedbackVersionSchema = z.uuid()
   .transform(value => value as MessageFeedbackVersion)
 
 /** Runtime schema for one current feedback item. */
+// Zod infers transformed branded fields structurally, so it cannot name the
+// public interface even though every branded output is created below.
 export const messageFeedbackItemSchema = z.object({
   messageId: z.string().min(1).transform(value => value as MessageId),
   rating: messageFeedbackRatingSchema,

packages/feedback/message-feedback/tests/helpers.ts

@@ -116,7 +116,7 @@ class TestPersistence extends SessionPersistence {
   inspectFailure: Error | undefined
   inspectCalls = 0
   readFromCalls = 0
-  onReadFrom: (() => void) | undefined
+  onReadFrom: (() => void | Promise<void>) | undefined
   onListSnapshots: (() => void | Promise<void>) | undefined
 
   locate(_meta: SessionHeader): SessionLocation | undefined { return undefined }
@@ -140,16 +140,16 @@ class TestPersistence extends SessionPersistence {
       : Promise.resolve(stored)
   }
 
-  readFrom(
+  async readFrom(
     id: SessionId,
     fromSeq: number,
   ): Promise<{ meta: SessionHeader; events: SessionEvent[] }> {
     this.readFromCalls += 1
-    this.onReadFrom?.()
+    await this.onReadFrom?.()
     const stored = this.durable.get(id)
     return stored === undefined
       ? Promise.reject(new Error(`test persistence: session '${id}' not found`))
-      : Promise.resolve({ meta: stored.meta, events: stored.events.filter(event => event.seq >= fromSeq) })
+      : { meta: stored.meta, events: stored.events.filter(event => event.seq >= fromSeq) }
   }
 
   list(): Promise<SessionHeader[]> {
@@ -177,6 +177,7 @@ export interface TestHarness {
   readonly ctx: Context
   readonly persistence: TestPersistence
   readonly root: string
+  disposeFeedback(): Promise<void>
   dispose(): Promise<void>
 }
 
@@ -184,22 +185,26 @@ export interface TestHarness {
 export async function setupHarness(maxNoteBytes = 64): Promise<TestHarness> {
   const root = await mkdtemp(join(tmpdir(), 'dsh-message-feedback-test-'))
   const ctx = new Context()
+  let disposeFeedback: (() => Promise<void>) | undefined
   try {
     await ctx.plugin(SessionStore)
     await ctx.plugin(TestPersistence)
     await ctx.plugin(Storage)
     await ctx.plugin(StorageJson, { root })
     await ctx.plugin(StorageDomain, { backend: 'json' })
-    await ctx.plugin(MessageFeedbackService, { maxNoteBytes })
+    const feedbackFiber = await ctx.plugin(MessageFeedbackService, { maxNoteBytes })
+    disposeFeedback = feedbackFiber.dispose
   } catch (error) {
     await ctx.fiber.dispose()
     await rm(root, { recursive: true, force: true })
     throw error
   }
+  if (disposeFeedback === undefined) throw new Error('message feedback test plugin did not load')
   return {
     ctx,
     persistence: ctx.sessionPersistence as unknown as TestPersistence,
     root,
+    disposeFeedback,
     async dispose() {
       await ctx.fiber.dispose()
       await rm(root, { recursive: true, force: true })

packages/feedback/message-feedback/tests/invariant.spec.ts

@@ -0,0 +1,23 @@
+import { describe, expect, it } from 'vitest'
+import InvariantService from '@deepseek-ai/dsh-invariants'
+import * as MessageFeedbackInvariant from '../src/invariant.ts'
+import { setupHarness } from './helpers.ts'
+
+describe('message-feedback invariant companion', () => {
+  it('removes its registry contribution when its fiber is disposed (HMR safety)', async () => {
+    const harness = await setupHarness()
+    try {
+      await harness.ctx.plugin(InvariantService)
+      const fiber = await harness.ctx.plugin(MessageFeedbackInvariant)
+
+      expect(() => {
+        harness.ctx.invariants.register('@deepseek-ai/dsh-message-feedback', () => {})
+      }).toThrow(/already registered/u)
+
+      await fiber.dispose()
+      await expect(harness.ctx.plugin(MessageFeedbackInvariant).await()).resolves.toBeDefined()
+    } finally {
+      await harness.dispose()
+    }
+  })
+})

packages/feedback/message-feedback/tests/message-feedback.spec.ts

@@ -459,6 +459,57 @@ describe('MessageFeedbackService item concurrency', () => {
     }))
     expect(newItem.version).not.toBe(oldItem.version)
   })
+
+  it('drains admitted mutations before domain close and rejects later admission', async () => {
+    const current = await harness()
+    const { ctx, persistence } = current
+    const fixture = messageFixture('dispose-quiescence')
+    persistence.persist(fixture.session)
+    const service = ctx.messageFeedback
+    const lifecycle = service as unknown as { readonly mutationAdmissionOpen: boolean }
+    const started = Promise.withResolvers<undefined>()
+    const release = Promise.withResolvers<undefined>()
+    let physicalReads = 0
+    let committed = 0
+    persistence.onReadFrom = async () => {
+      physicalReads += 1
+      if (physicalReads !== 1) return
+      started.resolve(undefined)
+      await release.promise
+    }
+    ctx.on('domain/changed', (change) => {
+      if (change.domain === 'message_feedback') committed += 1
+    })
+
+    const first = service.put({
+      sessionId: fixture.session.id,
+      messageId: fixture.assistantMessageIds[0],
+      rating: 'positive',
+      ifVersion: null,
+    })
+    await started.promise
+    const second = service.put({
+      sessionId: fixture.session.id,
+      messageId: fixture.assistantMessageIds[1],
+      rating: 'negative',
+      ifVersion: null,
+    })
+    const disposal = current.disposeFeedback()
+    await vi.waitFor(() => { expect(lifecycle.mutationAdmissionOpen).toBe(false) })
+
+    await expect(service.delete({
+      sessionId: fixture.session.id,
+      messageId: fixture.assistantMessageIds[0],
+      ifVersion: staleVersion(),
+    })).rejects.toThrow('message-feedback: service is disposing')
+    release.resolve(undefined)
+
+    expectItem(await first)
+    expectItem(await second)
+    await disposal
+    expect(physicalReads).toBe(2)
+    expect(committed).toBe(2)
+  })
 })
 
 describe('MessageFeedbackService durability ordering', () => {