feat(feishu): answer an unreadable attachment instead of dropping it silently

[JiaDe-Wu]

Fixes #7848.

Problem / Motivation

Send a photo to the Feishu bot and nothing happens. Not an error, not a hint, not a reaction — silence.

feishu/client.py dropped any non-text message at the parse step. #7551 added a log line there, and its reasoning was right for the audience it chose:

Non-text inbound messages (and six other early-return cases) were dropped with no log line anywhere, making a delivered-and-ignored message indistinguishable from a dead WebSocket, wrong App ID, missing scope, or allowlist rejection.

That sentence is just as true for the sender, and for them nothing changed. A line in gateway.log resolves the ambiguity for whoever runs the gateway; the person who dropped a screenshot into the chat still cannot tell whether the bot is broken, whether their message arrived, or whether images are simply not a thing here. The rational next move is to send it again — or to send "did you get that?", which does get answered, making the first look like a bug.

Why it matters

messaging/attachments.py already exists to prevent precisely this, and says so:

Why rejections are returned rather than swallowed. Silently dropping an attachment is the original defect this module exists to fix: a user posts a file and the reply simply ignores it, with no explanation. Callers are expected to surface IngestResult.rejections to the user.

But a transport declaring files_inbound=False never reaches ingest, so it has no IngestResult to carry a reason and that mechanism never runs for it. The guarantee exists; two channels sit outside it.

What changed (motivation → approach → change)

The reply reuses the existing mechanism's shape rather than inventing a parallel one. attachments.channel_reads_no_attachments() builds the same single bracketed, em-dash-separated segment every ingest rejection already produces ([Attachment {name} — download failed]). It lives in that module because that module owns rejection wording, so a channel that later gains real ingestion changes which function builds the string, not how it reads.

Three gates decide where the reply happens, and each one is a security property:

Position	Why
after authorize	An unauthorised sender still learns nothing. Telling a stranger the bot is alive is a disclosure this channel deliberately avoids — authorize is deny-by-default and owner-only.
after the group gate	A Feishu bot added to a group receives every message in it. Answering a photo in a group nobody allow-listed would announce the bot to that whole room.
after the dedup window	lark's WS redelivers. Two answers to one photo is the failure this sits behind the window to avoid.

The parse step had to change to make that possible. _handle_receive_v1 dropped the message before extracting open_id, and the transport is the only layer that knows whether a sender is authorised — so it cannot answer a message it was never given. The type is now decided after the sender, and an unreadable message is carried on with text="" and unsupported_type set. Empty text plus no unsupported_type still stops in receive exactly as before, so a frame whose body resolved to nothing is unchanged. The existing log line is untouched.

files_inbound moves ASPIRATIONAL → ENFORCED in test_capability_ledger.py, with its read site cited inline, per the migration contract that module documents ("A field moving from ASPIRATIONAL to ENFORCED must move sets here in the same change"). A channel declaring True is untouched: its attachments go through ingest as before, and the test pins that by flipping the flag.

The echoed message_type is treated as untrusted. It is platform-supplied and goes back to the sender, so it is stripped to [a-zA-Z0-9_] and length-capped: brackets and dashes are what a rejection line is made of, and leaving them in would let a crafted type forge a second bracketed segment inside the reply.

Scope: Feishu only, deliberately

iMessage also declares files_inbound=False and still drops silently. Its receive checks text ahead of is_own_echo, whose position carries this comment:

LAST gate, and its position is load-bearing in both directions … consuming it there would restore the loop while looking fixed.

Reordering a gate whose own comment warns it can silently restore a message loop belongs in its own change, with its own reasoning and its own tests. The doc note records that iMessage is the remaining reader.

Tests

24 new tests. TestUnreadableInboundIsAnswered (10) pins the reply and, one test per gate, the three deny paths that must stay silent — unauthorised sender, empty allow-list, unlisted group, unknown chat type — plus answered-once on redelivery, never-drives-a-turn, an allow-listed group is answered, and a files_inbound=True transport answering nothing. TestChannelReadsNoAttachments (6) pins the string's shape rather than its prose, so a reworded reason stays one segment. Two client tests replace the old "ignored" assertion with the new carry-through contract, including that a message with no sender is still dropped.

Mutation-checked, so each test fails for the reason it claims:

mutation	reds
answer before authorize (leaks the bot to a stranger)	3
ignore the files_inbound gate	1
answer before the dedup window (double reply)	1
stop sanitising the platform-supplied kind	3
drop unreadable messages again (the original bug)	5

Manual verification

N/A — unit coverage is sufficient and the alternative is not available to me: reaching the real drop path needs a Feishu app plus lark-oapi, and every branch this change adds is reachable with the FakeClient the suite already uses. The tests drive transport.receive and client._handle_receive_v1 directly, which is where all five decisions live.

Gates run on Linux (parity with CI):

black --check (touched files)          7 files unchanged
scripts/check_black_formatting.py      passed
isort --check-only src/kiro_crew test  clean
flake8 src/kiro_crew test              clean
mypy --platform linux src/kiro_crew    no issues in 1280 source files
scripts/docs-lint.sh                   259 files, all checks passed
scripts/check_brand_name.py            passed
pytest (the four touched suites)       166 passed
pytest -k "messaging or feishu or capability or channel"   3640 passed, 2 skipped

Related Issues

Fixes #7848.

On the one question that issue held for — where a backend-owned user-facing string lives — the answer turned out to be in the tree rather than a decision still owed, and it is the reason this is implementable: messaging/commands.py already returns hardcoded English reply text to users ("No subagents running.", "No cron jobs scheduled.", "No task running.", and three more), the backend has no catalog, no .po, and no gettext anywhere, and website/src/i18n/ is frontend-only. So "backend-owned strings have no catalog path yet" is literal. This string is English for consistency with those six, and when a backend catalog arrives it migrates with them.

That remains the reviewable choice here, and it is a product call as much as an engineering one: on Feishu the likely reader is Chinese-speaking, and so are the six precedents. If you would rather this class of string wait for a catalog, say so and I will drop the reply and keep only the carry-through plumbing.

[JiaDe-Wu]

The one red check is Coverage Gate, and it is not a coverage shortfall — it fails closed on a cancelled dependency:

coverage-combine=skipped  frontend-test=success  frontend-coverage-merge=skipped
backend-test=cancelled  only_backend=true  only_frontend=false
##[error]backend-test=cancelled -- failing closed.

One shard of eight, Backend Tests (3.12, 4), was cancelled. The other three 3.12 shards, all four Windows shards, and the namespace-sandbox job all passed. Its step log ends like this:

19:16:12  ........................................ [ 99%]
19:28:32  ##[error]The operation was canceled.
          Terminate orphan process: pid (2405) (pytest)
          Terminate orphan process: pid (2406) (python)
          ... three more orphaned python processes

Twelve minutes of zero output at 99%, then cancellation, with orphaned workers. I do not think this is my diff, and here is the reasoning rather than the assertion:

setup.cfg runs with --timeout=120, so a Python-level hang in a test is killed at two minutes and reported as a failure. Twelve minutes of silence is six times that ceiling, so whatever stalled was not something pytest-timeout could interrupt. The signature also matches the class setup.cfg's own comment describes, two lines above that timeout:

When workers die from memory pressure xdist silently clones replacements, up to numprocesses*4 of them — a 10-worker run will quietly restart 40 times, which on a host that has started swapping means ~20 minutes of zero progress and an empty log.

Zero progress, empty log, orphaned python processes. --max-worker-restart=2 exists to surface exactly that loudly, and the job was cancelled before it got to.

And this diff has nothing that can block outside Python: no subprocess, no C call, no network, no sleep. The reply path awaits a FakeClient in every test.

What I checked rather than assumed:

• It is not a general flake right now. The eight most recently merged PRs (test(autonudge): gate the prompt-loop double that carries probe state #8317, fix(members): give the detail drawer the chat SidePanel's frame #8243, feat(connections): register Superhuman Mail as a launch-gated provider #8239, fix(chat): flow a collapsed paste chip on a blockquote-prefixed line #8233, refactor(events): dedupe the RawEvent fallback and the tombstone stat #8225, fix(tests): anchor bare-interpreter assertion on its code-span backtick #8224, fix(ci): render the review lanes' fork check name instead of its expression source #8216, fix(dashboard): fold a tool-call diff card to its chip by default #8214) all show 1=success 2=success 3=success 4=success and Coverage Gate=success. So I am not claiming "shard 4 always does this".
• I cannot re-run it. gh run rerun --failed answers Must have admin rights to Repository, which is correct for a fork PR.
• I am reproducing CI's shard 4 locally with pytest --splits 4 --group 4 on a Linux host, on this branch, and will report the result here either way — including if it does hang, in which case this is mine and I will fix it.

Gates I can run all pass on Linux (parity with CI): flake8 clean, mypy --platform linux no issues in 1280 source files, docs-lint 259 files, brand gate, the four touched suites 166 passed, and -k "messaging or feishu or capability or channel" 3640 passed / 2 skipped. Five mutations of the new logic each produce reds.

If a maintainer can re-run that one shard, that would settle it faster than I can from outside. Flagging rather than force-pushing a no-op commit to trigger a fresh run, since a rebase would also reset the three AI-review lanes that have already reported.

[JiaDe-Wu]

Reproduced CI's shard 4 on a Linux host, both with and without this diff. It does not hang, and the failure count is identical to the unpatched base.

this branch (b4030c1b3):   59 failed, 20823 passed, 122 skipped   in 330.36s
upstream/main (74eab0ca9): 59 failed, 20821 passed, 122 skipped   in 325.25s

Same command both times — pytest --splits 4 --group 4 --no-cov --timeout=120 -p no:randomly, the same pytest-split slice CI runs. The two-test delta is this diff's own new tests landing in this group; the 59 failures are byte-for-byte the same set, all in src/kiro_crew/apps/builtins/{auto_improvement,ops_mission_control}/tests/ — git-sync round-trips, profile capture, provider pollers, and a @sandbox_probe case. They are host-environment failures on my box (no sandbox backend, git behaviour) and they occur without this branch checked out.

Not one of them is in a file this PR touches, and shard 4 finished in 5 minutes 30 seconds where CI sat silent for twelve and was then cancelled at forty.

So the cancelled shard was not this diff. Combined with what I noted above — a Python-level hang would have been killed by setup.cfg's --timeout=120 six times over, the twelve-minutes-of-silence-plus-orphaned-workers signature is the memory-pressure class setup.cfg's own comment describes, and this change contains no subprocess, network call, or C-level block — I think the run needs re-running rather than the branch needing changing.

I cannot trigger that myself (gh run rerun --failed → Must have admin rights to Repository, correct for a fork PR). If it is easier to get a fresh run than to re-run one job, say so and I will push a rebase onto current main — I held off only because it also resets the three AI-review lanes that have already reported here.

For completeness, the rest of the gates on the same Linux host: flake8 clean, mypy --platform linux no issues across 1280 source files, docs-lint 259 files pass, brand gate pass, the four touched suites 166 passed, -k "messaging or feishu or capability or channel" 3640 passed / 2 skipped, and five mutations of the new logic each produce reds (answer before authorize → 3, ignore the files_inbound gate → 1, answer before the dedup window → 1, stop sanitising the echoed kind → 3, restore the original silent drop → 5).

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #7823 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7823: REBASE. Complementary Feishu improvements in opposite directions with no textual or semantic conflict; neither needs to wait on the other. Files: src/kiro_crew/feishu/client.py, src/kiro_crew/feishu/transport.py.
• This PR is OVERLAPPING with PR #3754. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #8228: KEEP. Complementary halves of the same module's guarantee; 3754 gives no rejection path to a files_inbound=False channel, so it cannot cover 8228. Files: src/kiro_crew/messaging/attachments.py, test/test_messaging_attachments.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

[JiaDe-Wu]

Correcting one link in my own reasoning above, now that I have read the full job log rather than its tail.

What I got right and what I got wrong. I wrote that "the twelve-minutes-of-silence-plus-orphaned-workers signature is the memory-pressure class setup.cfg's own comment describes", and quoted that comment's "~20 minutes of zero progress and an empty log". That passage describes xdist cloning replacements repeatedly on a swapping host. That is not what happened. The log records exactly one replacement:

19:12:56.6287Z  ...........................................[gw2] node down: Not properly terminated
19:12:56.6738Z  F
19:12:56.6738Z  replacing crashed worker gw2
19:16:12.477Z   <last progress output>
19:28:32.868Z   ##[error]The operation was canceled.

So the mechanism is the single-replacement one: setup.cfg's --max-worker-restart=2 permitted that replacement, and xdist's node-replacement path then wedged the session — the defect #2803 documents (a node left in assigned_work but absent from registered_collections) and #4227 observed burning the full cap. --max-worker-restart=2 does not "surface it loudly", as I said it would; permitting the replacement is what enters the bad path. Anyone reading my comment above would go looking for a memory problem, and the log gives no support for one — no OOM, MemoryError or kill message appears anywhere in it, so why gw2 died is still unidentified.

The part of my reasoning that holds is the timeout argument, and the log confirms its premise: the session header reports timeout method: signal, so a Python-level hang in a test would have been failed at 120s. It did not fire because by then no test was running — the session was wedged, which is outside what pytest-timeout can reach.

Nothing changes about the exoneration. Progress at the wedge was 99.8% of 21077 collected items, and the crashed item is the run's only F — unnamed, because the session never printed a summary. The three sibling shards passed in 28m44s, 30m49s and 31m56s on this same commit. The controlled comparison still stands on its own: 59 failures on this branch, the same 59 on unpatched base, shard 4 complete in 5m30s locally.

I have filed the general problem rather than leaving it as a footnote on my own PR: evidence on #4227 (comment) and a fix in #8691 — --max-worker-restart=0 was applied to backend-test-windows only, and the POSIX job still inherits =2. This run is the recurrence that issue's closing comment asked to be told about.

The re-run request above is unchanged, and if #8691 lands first this shard's failure mode would at least come back named.

[bolichen97]

Rebased onto main 6ae74179 by a maintainer as part of the 2026-09-08 open-PR audit. This branch was 565 commits behind and mergeable_state=dirty.

One conflict:

• src/kiro_crew/docs/messaging-transport.md (modify/delete): merged docs: refresh prompts, skills and docs against the shipped code #8905 deleted this file outright, so main's deletion was taken and your ASPIRATIONAL -> ENFORCED paragraph for files_inbound was dropped with it. Nothing stale is left behind, because no ENFORCED/ASPIRATIONAL narrative survives anywhere under docs/ or src/kiro_crew/docs/ on main, and the classification itself still moves in test/test_capability_ledger.py, which is the enforcing location. If you want that prose back, docs/system-specs/modules/messaging.md is where files_inbound is described now. That placement is your call, so I did not relocate it.

src/kiro_crew/docs/feishu-integration.md auto-merged into #8905's rewritten section. No code file conflicted.

Gates run locally on the changed files only: black, isort, flake8 clean; pytest test/test_capability_ledger.py test/test_feishu_client.py test/test_feishu_transport.py test/test_messaging_attachments.py -> 170 passed.

Please review the resolution. A maintainer push makes the maintainer the last pusher, so under this repo's last-push rule a second approver is needed. Reply if anything looks wrong.

[github-actions]

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 0d0eff35f27401a289d670028737ea3ed0391cce via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: CONCERNS

Sound placement and reuse; the one gap is that the new reply's only failure signal is a raise nothing observes.

Watch

The rejection reply's failure path is invisible by construction. send_message documents that a raise is "the ONLY delivery signal there is", but the new call site (await self.send_message(inbound.message_id, ...) in receive) runs inside a coroutine the client fires via asyncio.run_coroutine_threadsafe(handler(inbound), loop) with the future discarded — so a refused or failed REST reply raises into a dropped concurrent.futures.Future: no log, no retry, and the sender gets exactly the silence this PR exists to eliminate, now with no operator trace that a reply was attempted and lost. The dedup entry is already recorded, so a redelivered frame won't retry either.
Clears when: the reply call is wrapped so a send_message failure is caught and logged (matching the existing Feishu inbound dropped operator record), with a test pinning that a failing send_reply does not propagate out of receive.

Suggestions

• File the iMessage half of the files_inbound=False gap as a tracked issue rather than only a doc sentence, so the "guarantee exists; two channels sit outside it" defect the PR names doesn't quietly become one channel forever.

[DESIGN-REVIEWED] 0d0eff3

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 0d0eff35f27401a289d670028737ea3ed0391cce via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 0d0eff3

[github-actions]

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 0d0eff35f27401a289d670028737ea3ed0391cce via the fork AI-review pipeline — 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 against the base. The final review follows.

First-Principles-Verdict: CONCERNS

A shared helper generalized for a second channel this PR explicitly defers, and the operator log line the description calls "untouched" moved and now mislabels answered messages.

Not justified as shipped

• 5 — one consumer, generalized: channel_reads_no_attachments has exactly 1 consumer (feishu/transport.py); its second (iMessage) is deferred to its own change, and the tree's two existing unsupported-attachment lines are channel-local anyway (teams/attachments.py:293, weixin/transport_dispatch.py:244 — grepped \[Attachment, 2 hand-rolled variants).
• 7 — undeclared: "The existing log line is untouched" is contradicted by the hunk — the line moved below the open_id gate (a senderless non-text frame now logs no open_id instead), and "dropped" now describes a message that gets answered.

What this change ships

Intent: a person who sends a photo to the Feishu bot learns it can't be read instead of getting silence — a FIX (#7848; the base doc already listed the silence under "Known gaps").

1. An allow-listed Feishu sender's image/file/audio now gets a one-line "text only" reply — justified
2. Strangers, non-allow-listed groups, and redelivered frames still get silence — justified
3. The reply names the attachment kind, stripped to word characters and length-capped — justified
4. Non-text frames now travel client→transport with empty text plus unsupported_type (1 consumer) — justified
5. New shared helper channel_reads_no_attachments in messaging/attachments.py — one consumer, generalized
6. files_inbound reclassified ASPIRATIONAL→ENFORCED — justified
7. Operator log for non-text frames fires later and still says "dropped" — undeclared
8. Feishu user doc gains the reply note — justified
9. _deliver extraction in client.py — rides along
10. New backend English string with no catalog path — justified

Watch

• The description says "The doc note records that iMessage is the remaining reader," but no hunk mentions iMessage — the one counted sibling (imessage/transport.py:72, files_inbound=False) is deferred with sound reasoning (its text check at line 195 precedes the position-load-bearing is_own_echo gate, verified on base) yet recorded nowhere. Clears when: the note exists in the diff, or the sentence is dropped and an issue links the deferral.
• Item 7: the log's firing condition and truthfulness changed. Clears when: the description stops claiming "untouched", or the line's wording matches delivery.

Subtractions

• Inline the rejection string at its one consumer (feishu/transport.py) and drop channel_reads_no_attachments plus _REJECTION_KIND_RE/_REJECTION_KIND_MAX from messaging/attachments.py; hoist into the shared module when iMessage actually calls it.

[FIRST-PRINCIPLES-REVIEWED] 0d0eff3

[github-actions]

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed 0d0eff35f27401a289d670028737ea3ed0391cce via the fork AI-review pipeline; updated in place on each push.

2 of 2 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands.

BLOCKING -- src/kiro_crew/feishu/transport.py:281 -- unreadable-message reply bypasses channel governance
if inbound.unsupported_type: ... await self.send_message(...)
Runtime Feishu deny -> authorized attachment delivery -> reply sent without inbound_permitted -> administrator-disabled channel still responds.
Anchor: residual/security
Fix: run inbound_permitted("feishu") before sending the rejection.

BLOCKING -- src/kiro_crew/feishu/transport.py:283 -- failed reply remains marked as delivered
await self.send_message(...)
Transient REST failure -> message ID remains in _seen -> WebSocket redelivery is discarded -> the sender receives no reply.
Anchor: residual/crash-data-loss-corruption
Fix: remove the message ID from _seen when the send raises, then re-raise.

[BLOCK-MERGE] 0d0eff3
[GPT-REVIEWED] 0d0eff3

Adjudication (Opus 4.8) — is blocking on each finding proportionate?

Both findings are FENCED (annotate-only); the adjudicable block is empty. I verified the code paths.

F1 — The dispatcher's per-message governance ceiling is inbound_permitted("feishu") at feishu/transport_dispatch.py:107, which resolves the administrator channels deny (messaging/identity.py:56-130, fail-closed). The new unreadable-reply block in receive sends the rejection and returns before ever calling self._dispatch, so it never reaches handle_message and its inbound_permitted gate. An administratively-disabled feishu channel therefore still emits a reply to an authorized owner. Conditions required (admin denies feishu + owner sends an attachment) are plausible, not extreme, and the harm is a governance-ceiling bypass — unbounded security class. No clearance record can be completed.

F2 — _seen[msg_id] is inserted at transport.py:262-264 (base) before the reply send at the bottom; a transient send_message raise leaves the id recorded, so a lark WS redelivery is discarded at transport.py:262-263 and no notice is sent. But the "lost" item is a static, regenerable canned notice, not user data; the degradation is exactly the pre-PR silence and self-corrects on the sender's next message (new message_id, not in _seen). The record-before-act ordering is also the module's established pattern for the normal dispatch path. Conditions: reply-path reached (new block) + transient REST failure + WS redelivery — a rare coincidence whose residual (a missed one-line notice, = prior behavior) a human would plausibly accept.

[ADJUDICATION] 0d0eff35f27401a289d670028737ea3ed0391cce total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 0d0eff35f27401a289d670028737ea3ed0391cce

[ADJUDICATION-FENCED] 0d0eff35f27401a289d670028737ea3ed0391cce fenced=2 flagged=1
UPHOLD-FENCED F1 src/kiro_crew/feishu/transport.py:281 -- reply is sent before the dispatcher's inbound_permitted("feishu") ceiling, so a governance-disabled channel still responds; conditions are plausible, not extreme.
FLAG F2 src/kiro_crew/feishu/transport.py:283 -- on a transient send failure with WS redelivery the only loss is a regenerable canned notice, degrading to pre-PR silence and self-correcting on the sender's next message; no user data is lost.
[GPT-ADJUDICATED-FENCED] 0d0eff35f27401a289d670028737ea3ed0391cce

🏷️ Fenced finding(s) machine-flagged as likely edge case

The security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)

• F2 src/kiro_crew/feishu/transport.py:283 — on a transient send failure with WS redelivery the only loss is a regenerable canned notice, degrading to pre-PR silence and self-correcting on the sender's next message; no user data is lost.