fix(channels): persist a durable row id for channel-born transcript rows

[chenmingwei23]

What is the problem?

Viewing a channel-born session (Weixin/iLink) in the dashboard renders the assistant's reply several times - reported as 5x on one turn - while the on-disk transcript holds exactly one copy of the message.

A channel dispatcher persists its rows through ConversationLog.append without a mid, so the JSONL row gets no meta at all. _ChatSlot.append mints a meta.mid for any row that lacks one, and channel_slots._rebuild_window / refresh_channel_window both materialize the disk tail through that append - so the same logical row gets a fresh identity on every materialization, and the identity the dashboard was handed a moment ago is not the identity it is handed next time.

That matters because the dashboard's merge keys on meta.mid and nothing else, and each consumer is deliberately written to decline rather than guess when the id is absent or has changed:

• isRedeliveredMessage (chatSlice.ts:90) returns false for a missing mid, so a redelivered channel row is never recognised as one.
• olderHeadAbovePage (chatSlice.ts:1374) locates the retained scrollback head by page[0].meta.mid; a changed mid makes findIndex return -1.
• rowIdentities (chatSlice.ts:1265) derives identity from meta.mid/meta.sendId only, so a row with neither yields an empty identity array - and tailNotInPage (:1280) therefore treats it as "not present in the page" and always retains it.

With cutIdx < 0 permanent for a channel session, the warm-cache merge runs in its degraded branch, where priorEndsBeforePage unions [...prior, ...tailNotInPage(warmed, prior)] - appending a copy the pane already holds, once per pass.

Why this issue matters to the user

The dashboard's view of a Weixin/Telegram/Discord conversation is visibly wrong: a reply is repeated several times, so the history reads as though the agent answered over and over. Because the duplication is display-side, nothing in the transcript looks broken and there is no obvious recovery - and since the identity is re-minted on every window rebuild, re-opening the tab can produce a different wrong answer rather than a fixed one. The same root cause also degrades scrollback retention in the other direction: a declined cut can drop history the pane had already paged in.

How our fix solves it

The identity has to come from the writer, because only the writer sees the row once.

A channel turn runs on the dispatcher's own session, so unlike the dashboard dual-writers (cron_inject, workflow_inject, crew_chat, which read the id back off slot.append and pass it to append_if_absent) there is normally no slot to mint from. The dispatcher is the first and only place the row exists, so it now mints its own id and persists it.

• history.py gains mint_row_mid(), the single definition of the meta.mid format. _ChatSlot.append now calls it instead of spelling f"m-{uuid4().hex[:16]}" inline, so the format cannot drift between the two mint sites. ConversationLog.append already accepted mid and already wrote it as meta.mid; no storage change was needed, and its id-less contract (TestAppendMid) is untouched.
• Nine dispatchers that share the two-append _persist_turn shape now pass mid=mint_row_mid() on each row: weixin, telegram, discord, feishu, imessage, teams, webex, wecom, whatsapp.
• Discord's mirrored branch is the one exception, because it DOES have a live slot. _mirror_turn_to_live_slot now returns the (user_mid, assistant_mid) the slot minted and _persist_turn threads them into append_if_absent, so both copies of one row share ONE identity. Minting a second id there would defeat that call's same-mid skip - append_if_absent only skips a body-equal row carrying the same mid - and persist the turn twice on disk. An earlier revision of this PR had exactly that bug; it is fixed and pinned by four tests.

Chaining it back to the symptom: the row now reaches disk with meta.mid; _rebuild_window/refresh_channel_window pass that meta straight into slot.append, which preserves a caller-supplied mid rather than minting; so every materialization resolves to the same identity, isRedeliveredMessage can recognise a redelivery, olderHeadAbovePage can find its cut, and the union branch that was appending a duplicate is no longer reached.

Two deliberate exclusions. Slack stays out: its persist is a different topology (three write branches, off-loop to_thread stamping, an options-turn path) and folding it in would mean reworking that path rather than adding a kwarg. And the other half of the producer analysis on #5981 - removing manual broadcast_ws("chat_message", ...) frames that follow a slot.append - is not here: I traced each candidate site (chat_orchestrator.py 238/809/847/1022/1068, chat_handlers.py 617/778, workflow_inject.py:173, crew_chat.py:1164, slack/handler.py:2538, slack/gateway.py 5902/6821, handlers/files.py:326) and none is reachable from a channel-born turn - a weixin turn never enters the dashboard orchestrator, and the weixin package contains no broadcast_ws/slot.append call site at all.

What tests we did

New test/test_channel_row_identity.py - 36 tests, 33 red on pristine base (origin/main 1ee69f225), verified in a detached base worktree:

• test_persist_turn_stamps_a_durable_mid_on_both_rows - parametrized over all 9 channels, drives the real unbound _persist_turn against a real ConversationLog and asserts meta.mid on the raw JSONL (read off the file, not through a reader that could re-derive an id).
• test_persist_turn_ids_are_unique_across_turns - 4 rows over 2 turns, 4 distinct ids. A reused id makes a client drop a real row, the opposite failure to a duplicate.
• test_persist_turn_still_writes_an_empty_reply - the empty-reply guard is unchanged.
• TestDiscordMirroredBranchStaysIdempotent - 4 tests on the exception path, including the real race (slot save lands the row, then the mirror retries it) which was red under the previous head.
• TestMintRowMid - format and distinctness over 512 mints. Imported inside each test on purpose, so the rest of the module still collects on a tree without the helper and the behavioural tests fail on their assertions rather than the file erroring out on an ImportError.
• TestIdentitySurvivesMaterialization - 3 characterization tests that pass on base: channel_slots already preserves a persisted mid and already re-mints an absent one. They are what make the fix load-bearing rather than incidental, and they pin the re-mint behaviour that must stay for legacy rows.

Structured after test_channel_persist_agent_metadata.py (#2890), which locks the same "every channel omitted a kwarg on its persist writes" shape.

Eight existing conv_log test doubles were widened to accept mid, matching the real signature (weixin, whatsapp, discord_sessions, webex, wecom, feishu, teams dispatch tests). Enumerated in one pass rather than discovered per CI round.

2893 passed across every affected channel suite plus channel_slots, history (TestAppendMid intact), cron_history, chat_row_identity and the discord suite. flake8, isort, black clean on all 19 changed files. Inherited, reproduces on pristine base: test_weixin_qr.py fails 2 with ModuleNotFoundError: No module named 'qrcode' (missing optional dep in my environment, unrelated to this diff).

Pattern harvest

Rule candidate: semgrep (or a review checklist item)

Pattern: a dual-writer that mints a fresh identity for its durable copy instead of reusing the one its in-memory copy already carries

The generalizable defect is not "channels forgot a kwarg" - it is that a row's identity must be minted once per row, not once per write. Two shapes are worth catching mechanically:

1. A call to ConversationLog.append_if_absent(..., mid=<freshly minted>). That call's skip is defined as "body-equal AND same mid", so a fresh id makes it structurally unable to skip and converts an idempotent write into an unconditional one. Any mid= argument there that is not derived from a prior slot.append return (row_mid(...)) or a stored value is a bug. This is exactly what GPT caught on head d08ede5fd, and it is the second time this class has appeared: chat_runner.py:9287's comment already warns about the sibling case (append plus a manual re-broadcast).
2. A persist site that writes a row a live slot also holds, without threading the slot's meta.mid. The correct shape is documented three times in-tree (cron_inject.py:136-167, workflow_inject.py:167-198, crew_chat.py:1163-1184) - window_mid = row_mid(slot.append(...)) then append_if_absent(..., mid=window_mid) - which is a strong signal it should be a lint rule or a small shared helper rather than a pattern each writer re-derives.

Not generalizable: the specific list of nine dispatchers is not a reusable rule. That breadth is a consequence of the same _persist_turn shape having been copied per channel, which #2890 already hit for the agent kwarg - the durable lesson there is that a per-channel copy of one persist routine will keep needing the same fix N times, and is a refactor candidate in its own right.

Any other suggestions on the work?

• This is the backend half of fix(dashboard): channel-born session renders duplicate assistant messages on dashboard #5981. PR #5982 (@isotope14) is the frontend deduplicateByMid sweep, currently held by a First Principles BLOCK whose confirmed half is that the sweep cannot reach the producer. The two are complementary: with ids now stable, a future duplicate of a channel row becomes a same-mid duplicate, which is exactly what that sweep collapses. Nothing here conflicts with it, and the diffs share no file.
• Defaulting the mint inside ConversationLog.append is a real improvement, deliberately not taken here. It would need no call-site edits and would cover Slack, WhatsApp and eval at once. I implemented it in full before reverting: it breaks test_history.py::TestAppendMid, which explicitly pins that an id-less append must not grow a meta field. Forking a documented identity contract belongs in its own change where that can be argued on its merits, not folded into a bug fix. Recorded as a follow-up.
• Existing transcripts are not backfilled. Rows already on disk stay id-less and keep being re-minted per materialization, so the duplication is still observable on conversations that predate this fix. That is intentional (no retro-migration, per the same TestAppendMid contract) and worth an explicit decision rather than a silent migration.
• Slack's _persist_turn still has the gap and deserves its own change, sized for its three-branch write path.
• The evidence gap named on fix(dashboard): channel-born session renders duplicate assistant messages on dashboard #5981 is now closed by construction rather than by observation. Nobody ever captured whether the five rendered rows carried mids. If a duplicate is seen again after this lands, the discriminator is whether the copies now share a mid; if they do, the producer is a second delivery rather than an identity churn, and fix(dashboard): deduplicate channel-born session messages by mid #5982's sweep is the right place for it.

Closes #5981

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] a51e898

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

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of a51e8986d78856a5ab25e429540510e851f7e962 — 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.

The evidence checks out. I ran the counts the contract requires: mint_row_mid has 10 real consumers (state.py:3900 plus 9 dispatchers); the only remaining id-less channel persist is Slack (2 sites, slack/transport_dispatch.py:511,772), declared and deferred in the description; eval/runner.py:334-335 writes id-less rows but into eval_* memory-consolidation keys that never reach the dashboard merge, so they are not siblings of this defect; the id-less ConversationLog.append contract the author left intact is a pre-existing recorded decision (test_history.py:4834, test_id_less_legacy_append_still_round_trips_without_meta), so the "mint by default in append" alternative is already settled by the repo.

First-Principles-Verdict: PASS

A reported display defect, fixed at the writer that owns the row's first existence, with the one duplicate spelling of the id format deleted rather than added to.

What this change ships

Intent: stop the dashboard rendering one channel reply several times — a FIX.

1. Channel rows now reach disk with a stable identity, ending duplicate replies (9 dispatchers) — justified, the fix.
2. The meta.mid format now has one definition (mint_row_mid); _ChatSlot.append's inline spelling replaced — justified; 10 consumers counted, removes a second spelling.
3. Discord's mirrored turn reuses the slot's ids; the mirrored bool is deleted, not kept alongside — justified, prevents a durable double-write.
4. Slack keeps the old id-less behavior — declared, deferred with a stated reason (2 sites counted).
5. Pre-fix transcripts still get a per-view id; nothing migrates — declared, pinned by a pre-existing recorded contract.
6. 36 new tests, 33 red on base; 8 fakes widened to the real signature — not consumers.

No item's zero option is free (the defect is user-reported), no existing mechanism does the job (the dual-writer row_mid(slot.append(...)) shape needs a slot these dispatchers don't have), and the fix sits at the mechanism the author's own cause analysis names, with the one deferred sibling counted and disclosed.

[FIRST-PRINCIPLES-REVIEWED] a51e898

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Root-cause fix at the correct layer — identity minted once by the writer, single format owner, the one dual-writer exception threaded correctly — with residual gaps (Slack, legacy rows) explicitly scoped and disclosed.

[DESIGN-REVIEWED] a51e898

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] a51e898

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

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

[chenmingwei23]

Disposition for the three findings on head d08ede5fd. New head is dc927d9ab.

GPT BLOCKING -- discord/transport_dispatch.py:1588, mirrored rows persisted under a second identity -- FIXED, and the finding is correct.

Verified against append_if_absent's own contract rather than accepting it on faith: "WITH mid, only a body-equal row carrying the SAME meta.mid" counts as already persisted. A fresh mint_row_mid() per call can therefore never match the copy the slot save landed, so the branch's idempotence was disabled and the turn would persist twice under two unrelated ids -- a durable duplicate, strictly worse than the display duplication this PR fixes. Self-introduced by the previous head.

Fixed with the dual-writer shape Design Review named, not by dropping the id: _mirror_turn_to_live_slot now returns the (user_mid, assistant_mid) the slot minted, and _persist_turn threads them into append_if_absent -- the same row_mid(slot.append(...)) then append_if_absent(..., mid=...) pattern as cron_inject.py:136-167. Both copies of one row now share ONE identity, so the skip works. A caller that cannot supply ids falls back to the id-less form (body-equality check, weaker but never duplicating).

Four new tests pin it, including the mirrored-branch coverage Design Review asked for and whose absence it correctly flagged: test_mirrored_write_reuses_the_slot_minted_ids, test_mirrored_write_is_a_no_op_when_the_slot_save_landed_it (the actual race -- red under the previous head), test_mirrored_write_without_ids_falls_back_to_body_equality, test_unmirrored_branch_still_stamps_a_fresh_id.

First Principles -- WhatsApp is an undeclared unfixed sibling -- FIXED, the count was wrong.

Confirmed: whatsapp/transport_dispatch.py:497-499 is byte-identical in shape, whatsapp surfaces in the dashboard (channel_slots._CHANNEL_LABELS), and my "eight dispatchers" claim was a description/code mismatch. It is now fixed and the body says nine. Its _persist_turn takes no agent (it predates #2890), so the test helper covers both signatures.

First Principles -- replace the per-call-site edits with one default in ConversationLog.append -- TRIED, then REVERTED. Rebutted with evidence, and I want to flag the trade-off rather than bury it.

I implemented this subtraction in full before rejecting it, because the reasoning is good: it needs no call-site edits, and it would cover Slack, WhatsApp and eval for free. Two things came back:

1. It forks a documented, tested contract. test_history.py::TestAppendMid explicitly pins test_id_less_legacy_append_still_round_trips_without_meta -- "an id-less append must not grow a meta field" -- with the stated rationale that readers keep an id-less fallback and nothing migrates old sessions. Defaulting the mint breaks that test and test_append_if_absent_treats_an_id_less_twin_as_another_occurrence. Forking a documented identity contract inside a bug fix is the same objection this lane raised against fix(dashboard): deduplicate channel-born session messages by mid #5982, so I did not want to do it silently.
2. The cost it correctly predicted is real but bounded. The call-site form requires every conv_log test double in the suite to accept the kwarg. I enumerated them rather than discovering them one CI round at a time: eight doubles across eight files (weixin, whatsapp, discord_sessions, webex, wecom, feishu, teams dispatch tests). All widened to match the real signature. That is genuine diff noise and the honest count.

So the subtraction is a real improvement that belongs in its own change, where breaking TestAppendMid can be argued on its merits and Slack/eval come along with it -- not folded into the fix for #5981. Recorded as a follow-up. If a maintainer prefers it here instead, the branch is one commit and I will reshape it.

Verification on dc927d9ab: test_channel_row_identity.py is 36 tests, 33 red on pristine base 1ee69f225 (run in a detached base worktree, not a stash -- an earlier stash check was reverting to HEAD rather than to main and undercounted). 2893 passed across every affected channel, channel_slots, history, cron_history, chat_row_identity and discord suite; TestAppendMid intact. flake8 + isort + black clean on all 19 files. Pre-existing and inherited: test_weixin_qr.py fails 2 with ModuleNotFoundError: No module named 'qrcode' on pristine base too (missing optional dep in my environment, not this diff).

[chenmingwei23]

Disposition for First Principles on dc927d9ab. New head is a51e8986d.

Subtraction -- drop mirrored, derive it from mirror_mids, delete the unreachable fallback -- TAKEN. The finding is correct.

Verified the sole-caller claim rather than taking it on trust: grep -rn "_persist_turn" src/kiro_crew/discord/ returns exactly one invocation (transport_dispatch.py:757, via to_thread) plus the definition and one unrelated comment. That caller was passing mirror_mids is not None to fill mirrored, so the two parameters encoded one fact and could only ever disagree.

mirrored is gone. mirror_mids is not None now IS the branch condition -- the presence of the tuple carries both facts. That deleted the mid=user_mid or None fallback, which no src path could reach, and with it the ""-sentinel handling that only existed to service that branch. mirror_mids is now unpacked directly, so a malformed tuple fails loudly instead of silently degrading to an id-less write.

I also deleted test_mirrored_write_without_ids_falls_back_to_body_equality. It was the only construction of the mirrored-with-no-ids combination, and the finding is right that a test is not a consumer -- keeping it would have been a test pinning dead code into existence. test_unmirrored_branch_still_stamps_a_fresh_id is renamed test_no_live_slot_means_a_plain_append_under_a_fresh_id to describe the condition that actually selects the branch now. Three mirrored tests remain, covering both live branches and the real race.

Slack -- accepted-and-deferred, restated with the count you grepped. slack/transport_dispatch.py:511 and :772 are the two id-less appends, and a Slack-born session keeps today's duplication until they land. The deferral reason stands (three write branches plus off-loop to_thread stamping and an options-turn stamp path -- a rework, not a kwarg), and it is named in the PR body rather than left implicit.

Verification on a51e8986d: 690 passed across test_channel_row_identity.py, test_discord_sessions.py, test_channel_persist_agent_metadata.py, test_history.py and test_discord.py; 466 passed across the other eight channel dispatch suites plus channel_slots, chat_row_identity and cron_history. flake8, isort, black clean.

Also fixed the PR Hygiene red, which was mine twice over: the first head had no ## Pattern harvest section, and the second had one whose Rule candidate: line was wrapped in markdown bold, so it failed the gate's ^ *(Rule candidate|Not generalizable) *: match. Both lines are now bare, checked against that exact regex before pushing.

Still outstanding: GPT reported "review incomplete" for dc927d9ab (no verdict produced -- not a finding), so it needs a clean roll on this head.

[bolichen97]

Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.