Skip to content

feat: block First Tree chat tools in Feishu-bridged chats - #2344

Open
liuchao-001 wants to merge 4 commits into
mainfrom
feat/feishu-chat-agent-guard
Open

feat: block First Tree chat tools in Feishu-bridged chats#2344
liuchao-001 wants to merge 4 commits into
mainfrom
feat/feishu-chat-agent-guard

Conversation

@liuchao-001

@liuchao-001 liuchao-001 commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

What changed

A First Tree chat bridged to a Feishu conversation lives in Feishu — the humans in it read the Feishu group, not the web app. An agent could still answer with chat send / chat ask / chat invite, and those writes landed where nobody on the other side could see them. The reply was silently lost.

This adds the agent-scope counterpart of assertWebMutableChat, which has covered the Web/user scope for a while. Both scopes now share one active-only predicate, and neither calls a bridged chat "read-only" any more: personal view state (read, pin, archive) deliberately keeps working, so the refusals name the blocked class instead.

Server (blocking, 403 + code: "FEISHU_CHAT_AGENT_WRITE_FORBIDDEN")

  • POST /api/v1/agent/chats/:chatId/messages — covers chat send and chat ask (same route; chat ask is just format: "request").
  • PATCH /api/v1/agent/chats/:chatId/messages/:messageId — message edit; rewriting history the Feishu humans cannot see is the same class of write.
  • POST /api/v1/agent/chats/:chatId/participants — covers chat invite.
  • DELETE /api/v1/agent/chats/:chatId/participants/:agentId — membership removal, the other half of "membership changes".

Every one of them applies the boundary after authorizing the caller's membership, so a non-member cannot tell a bridged chat from an ordinary one by the difference in error.

The shared helper is packages/server/src/api/agent/feishu-chat-guard.ts. Authority is im_chat_bindings filtered to status = 'active', not chats.metadata.source — that label stays "feishu" after a binding detaches. The Web helper predates the status column and does not filter; this one does, so a detached chat becomes an ordinary First Tree chat again.

The error message names the alternative rather than only refusing: record the delivery with feishu intent, then send with the official lark-cli --as bot.

Client-side preconditions (advisory, exit code 2, FEISHU_CHAT_CONTEXT)

chat create and chat open are refused from inside a bridged session, in apps/cli/src/core/feishu-chat-context.ts.

The bridge-collision hazard

This is the part worth reviewing closely. The Feishu bridge's own outbound delivery (POST /api/v1/agent/feishu/intents) calls the same messageService.sendMessage that chat send calls, with the identical source: MESSAGE_SOURCES.CLI and the agent's own senderId. A guard placed inside sendMessage would have broken the bot's own replies — the exact behaviour this PR exists to protect.

The two are distinguishable only by their route and by trusted in-process options (allowFeishuMetadata / allowRecipientlessSend) that never cross HTTP. So the guard lives in the route/adapter layer and is applied per-route, and the module header says so.

feishu-cli-preflight.test.ts gains the regression that pins this: on one bridged chat, the intent route still delivers (message stored, silent notify=false fan-out intact) while the agent chat route on that same chat returns 403.

The runtime-notice exemption

runtime/runtime-notice.ts::postProviderFailureRuntimeNotice posts through the same agent message route when a provider terminally fails. That row is the only in-product signal an operator gets that the agent could not run at all; suppressing it on a Feishu chat would make the chat look merely idle. It is exempt.

The exemption is a property of which route is called, not of anything in a request body: notices go to a dedicated POST /api/v1/agent/chats/:chatId/runtime-notices, whose body carries only the text. The server authors source, format, the silent recipientless delivery profile and the stored runtimeNotice marker, and strips any inbound copy of that marker. An ordinary chat send decorated to look like a notice is refused.

Be precise about what that endpoint is: a misuse-prevention rail carrying a notice the client runtime reports about itself — not a security or authorization boundary, and the exemption is not unforgeable. The route is gated on chat membership and nothing else, exactly like POST /messages, so any credential that can reach one can reach the other, and nothing verifies that a provider actually failed. What the separate route does buy is still worth having: the ordinary send path stays uniformly guarded with no shape of body that opens it, and the server authors the whole stored row so a notice cannot quietly become an addressed message. Whether the capability should be narrowed further — to the daemon, or scoped to a chat/turn — is an open posture question this PR deliberately does not settle.

Rolling deploy. The endpoint degrades in both directions rather than dropping notices, which matter most mid-deploy. A newer client falls back to the legacy send shape when the server answers 404; a newer server recognises that same exact legacy shape from a client that predates the endpoint. Both halves share one definition in shared so they cannot drift. That compatibility path is body-shaped by necessity and is scoped to an exact match on the five fields older clients emitted; it grants nothing, since the same caller could call the endpoint directly. Delete it once no supported client predates the endpoint.

Why chat create is CLI-side only

The server never learns which chat the caller is sitting in: there is no field for it in createTaskChatSchema, no header carries it, and chat/create.ts does not read FIRST_TREE_CHAT_ID. There is nothing to gate on server-side. chat open is worse — it runs on the user scope and starts an interactive REPL, so the server cannot tell an operator terminal from an agent session.

Rather than sniff the stale metadata.source label, both read a new ChatDetail.externalChannel field, populated by the agent chat-detail route from the same live im_chat_bindings state the write boundary enforces. That keeps the advisory rail and the real boundary from disagreeing — with metadata.source, a detached chat would have been refused locally while the server happily accepted it.

Both checks fail closed, because there is no server-side boundary behind them to catch a wrong guess. Only an explicit null means "not bridged". An absent field (a server older than it omits the field, and the SDK does not re-parse the response through Zod), an unrecognised value, a failed lookup, or a session naming a chat with no FIRST_TREE_AGENT_ID to read it as are all unknown and refuse with FEISHU_CHAT_CONTEXT_UNKNOWN, naming what to fix.

The one allowed silence is no chat context at all. Absent FIRST_TREE_CHAT_ID means the command is not running inside a chat: it proceeds without a lookup and never constructs an SDK, so chat open from a human operator's terminal with no agent configured keeps working. "No chat context" and "chat context we cannot resolve" are deliberately different answers.

Deliberately left allowed

  • chat update / set-topic — the agent briefing requires it to keep topic/description current, and neither is a message to a human.
  • chat list, chat history, participant reads, feishu intent, feishu credential-env, cron, github/gitlab follow, doc commands.
  • POST /agent/chats/:chatId/archive — this is a deviation from the original scope, called out for review. The route does exist on the agent scope, but it writes the calling human's private engagement row, i.e. personal view state. That is the same class the Web boundary deliberately keeps working on Feishu chats (its /read, /unread, /pin routes are all unguarded — note these are Web routes and personal state, not CLI commands), and packages/qa/cases/cross-surface/feishu-agent-channel.md already pins "personal read, pin and archive state must continue to work." Blocking it would have made the agent scope stricter than Web for no delivery-visibility reason. Happy to add it if reviewers disagree.

Tests

  • packages/server/src/__tests__/feishu-agent-readonly.test.ts — every blocked route (send, ask, invite, message edit, membership removal) returns 403 with the full expected body, not just status + code; the non-member probe asserts whole-body equality between a bridged and an ordinary chat; reads, chat update and the externalChannel signal still work; a genuine runtime notice lands through the dedicated route while decorated ordinary sends do not, and the legacy wire shape still lands for an older client; the boundary releases on detach; an unbridged chat is untouched.
  • packages/server/src/__tests__/feishu-cli-preflight.test.ts — the bridge-still-delivers regression described above.
  • apps/cli/src/__tests__/chat-feishu-context-guard.test.ts — both preconditions and every fail-closed path (field absent, unrecognised value, lookup failure, chat id with no agent id, unconstructable reader), plus the operator case that must keep working.
  • packages/client/src/__tests__/runtime-notice.test.ts — the 404 fallback to the legacy shape, and that a non-404 refusal is never reshaped into an ordinary send.
  • packages/qa/cases/cross-surface/feishu-agent-channel.md — new operate/observe steps for the agent-side boundary, the runtime notice under a forced provider failure, and post-detach release; FAIL criteria extended.

Four web DOM test fixtures and one client fixture gain externalChannel: null, following the existing descriptionUpdatedAt / lastReadAt .default(null) precedent on chatDetailSchema.

Checks run

  • pnpm check — pass (0 errors; the remaining warnings are pre-existing).
  • pnpm typecheck — pass, 9/9 packages.
  • pnpm test — pass, 10/10 packages, full monorepo. Docker was available, so the server Postgres testcontainer suites ran for real; nothing was skipped.

No database change: no schema, migration, constraint, index, default, or backfill. The guard reads existing im_chat_bindings rows.

🤖 Generated with Claude Code

An agent sitting in a chat bridged to a Feishu conversation could answer
with `chat send` / `chat ask` / `chat invite`. Those writes land in First
Tree, which nobody in the Feishu group ever reads, so the reply was
silently lost.

Add the agent-scope counterpart of `assertWebMutableChat`: the agent
message and participant routes now refuse a chat with an active
`im_chat_bindings` row, naming the path that actually delivers. The guard
lives in the route layer, never in `messageService.sendMessage` — the
Feishu bridge's own outbound delivery reuses that exact service call with
the same source and sender, so a service-layer guard would silence the bot
itself.

Provider-failure runtime notices stay exempt: an agent that cannot run at
all must not also go silent on its operators.

`chat create` and `chat open` cannot be gated server-side (neither
transmits the originating chat), so both get an advisory CLI precondition
reading the same live binding state through a new `ChatDetail.externalChannel`.

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

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation: request changes

  • Rationale: The active-binding guard is directionally correct, but detached chats get contradictory cross-surface behavior and chat create --agent can bypass the only available precondition.

Risk level: A

  • Path baseline: includes apps/cli/** and packages/client/** -> A
  • Semantic lift: touches agent message routing and live chat-binding state; no further grade above A

PR summary

  • Author / repo: liuchao-001 / agent-team-foundation/first-tree
  • Problem: Agents working from Feishu-bridged chats can currently use First Tree chat commands whose output is invisible to the humans following the conversation in Feishu.
  • Approach: Reject agent message/invite writes against active Feishu bindings, expose live binding state for CLI-only preconditions, and preserve the trusted Feishu delivery and provider-failure-notice paths.
  • Impacted modules: CLI chat commands, shared chat DTOs, agent chat/message routes, Feishu integration regressions, and cross-surface QA guidance

Review findings
❌ 1. A detached binding is treated as an ordinary chat only on the agent surface. isFeishuBridgedChat filters status = 'active', so this PR allows agent sends after detach, but assertWebMutableChat still rejects any historical im_chat_bindings row. The result is a chat where agents can resume writing while the managing human still cannot send, rename, or manage membership in Web. This also makes the QA sequence internally contradictory: it detaches and calls the chat ordinary, then immediately requires Web structural writes to remain blocked. Please make both surfaces share the active-binding predicate (or explicitly choose and document a different post-detach contract) and pin the same post-detach behavior from agent and Web. [R1/R5 / packages/server/src/api/agent/feishu-chat-guard.ts:48, packages/server/src/api/chats.ts:80, packages/qa/cases/cross-surface/feishu-agent-channel.md:91]
❌ 2. chat create checks the current chat through the outbound --agent override rather than the session identity. resolveSenderName gives the override precedence over FIRST_TREE_AGENT_ID; if that other local agent is not a participant in the bridged chat, getChatDetail returns 403, the advisory lookup deliberately fails open, and createTaskChat then succeeds with the overridden SDK. Because the create route never receives the origin chat, there is no server guard to catch this bypass. Resolve the bridge signal with the session agent independently from the selected sender, and add a command-level regression proving --agent <other> cannot create a chat from a bridged session. [R4/R5 / apps/cli/src/commands/chat/create.ts:143]
✅ 3. Keeping the hard guard at the agent route boundary preserves the trusted Feishu intent route's reuse of sendMessage; the same-chat regression is the right blast-radius check.

Action taken

  • Submitted request changes.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change correctly moves the Feishu boundary to the agent route layer, preserving the trusted feishu intent delivery path, and adds live binding state to ChatDetail for the two CLI-only preconditions. The runtime-notice and private archive exceptions match the existing chat semantics.

Two changes are required before approval:

  1. Blocker — chat create --agent bypasses the new precondition (apps/cli/src/commands/chat/create.ts:142-147). The same SDK selected by options.agent is used both for the new task write and for reading the current session chat. --agent takes precedence over FIRST_TREE_AGENT_ID; if the selected agent is not a participant in the current Feishu-bridged chat, getChatDetail() fails, isFeishuBridgedChatContext() deliberately returns false, and the command proceeds to create the task chat. Please perform the context lookup with the current session agent (FIRST_TREE_AGENT_ID) independently of the optional sender override, and add a command-level regression covering a bridged session plus --agent <different-local-agent>.

  2. Required documentation update — this changes user-visible behavior for chat send, chat ask, chat invite, chat create, and chat open, but docs/cli-reference.md still describes all five as generally available and says FIRST_TREE_CHAT_ID is used only by send/invite (plus cron). Please document the active-Feishu refusal, the allowed commands/exceptions, the alternative feishu intent + lark-cli --as bot path, and the new create/open use of session context.

Human contract check: the shared ChatDetail shape gains externalChannel; there is no database schema or migration change.

Address four review findings on the agent-scope Feishu guard.

`chat create --agent <other>` bypass: the origin-chat lookup ran as the
overridden agent, and an agent that is not a member of the origin chat gets a
403 that the fail-open path read as "not a Feishu chat". The lookup now runs
under the session identity, so it is performed by an agent that can actually
see the chat, and an inconclusive answer refuses instead of proceeding.
`--agent` still selects who creates the new chat and never becomes a
membership requirement for an ordinary create.

Forgeable runtime-notice exemption: the exemption trusted `purpose` and
`metadata.runtimeNotice` from the request body, which any agent credential can
set. Runtime notices now post to a dedicated route that authors the delivery
profile and the marker server-side, and the guard exempts by route rather than
by request content — the same property that makes the Feishu bridge safe. The
marker joins the server-owned metadata keys, so an inbound copy is stripped on
every ordinary write path. The client runtime posts through a new SDK method.

Detach semantics: the agent and Web scopes disagreed on what "bridged" means,
leaving a detached chat agent-writable but Web-read-only. Both now share one
active-only binding predicate.

BEHAVIOR CHANGE: the Web guard previously matched any binding row, including
detached ones. Web structural writes are now accepted again once a binding
detaches.

Probe leak: the invite guard ran before membership authorization, so the error
difference revealed which chats are Feishu-bound. Membership is now enforced
first. The message route already authorized before the guard.

Also correct the product wording — the boundary blocks messages and membership
changes, not all writes; `chat update` and personal state keep working — and
document the restrictions in the CLI reference.
@liuchao-001

Copy link
Copy Markdown
Contributor Author

Review findings addressed — new head 7a3756e

All four findings were re-verified against the code first. Three were real as described; one was half right (details below).


⚠️ BEHAVIOR CHANGE NEEDING HUMAN SIGN-OFF (from HIGH 3)

The Web app now allows structural writes on a chat whose Feishu binding has been detached.

Previously the Web guard matched any im_chat_bindings row, detached ones included, so a chat that had ever been bridged stayed Web-read-only forever. It now shares the agent scope's active-only predicate: once the binding detaches, Web rename / send / membership / entity-follow all work again.

This is a deliberate, user-visible change to Web behavior — not a refactor side effect. Rationale: a detached binding means the chat is no longer mirrored into any Feishu conversation, so a Web write reaches exactly the people it always did and the boundary has nothing left to protect. The old behavior also had no way back — a detach left the chat permanently frozen in Web.

Covered by a new case in feishu-web-readonly.test.ts. Please confirm this is the intent before merge.


BLOCKER 1 — chat create --agent <other> bypass — fixed

Confirmed. Two independent defects, both fixed:

  1. Wrong identity. The origin-chat lookup ran on createSdk(options.agent). It now runs on createSdk(), which resolves from FIRST_TREE_AGENT_ID — the session agent, which by construction can see its own chat. --agent still chooses who creates the new chat; it no longer decides who may answer "is the chat I'm sitting in bridged?". An unrelated agent's membership is never a precondition for an ordinary create — pinned by a test asserting the overridden agent's getChatDetail is not consulted.
  2. Fail-open. isFeishuBridgedChatContext collapsed every error into false. Replaced with a tri-state (bridged / unbridged / unknown); unknown refuses under a distinct FEISHU_CHAT_CONTEXT_UNKNOWN code with the underlying reason in the message.

Why fail-closed is safe here: the lookup and the create hit the same server with the same credentials, so a failed lookup means the create was going to fail anyway. The refusal replaces a confusing downstream error with a precise one — and the one case where the lookup fails but the create would have succeeded is exactly the case that produced this bug.

Gated on both FIRST_TREE_CHAT_ID and FIRST_TREE_AGENT_ID being set, matching the existing chat open precedent, so an operator terminal with no agent configured is unaffected.

BLOCKER 2 — forgeable runtime-notice exemption — fixed

Confirmed: purpose and metadata.runtimeNotice are both request fields, so the exemption was a hole straight through the 403.

Took the recommended direction — a dedicated route, POST /api/v1/agent/chats/:chatId/runtime-notices:

  • The server authors source, format, the recipientless silent delivery profile, and the runtimeNotice marker. The request carries only content, and the schema is .strict(), so an attempt to smuggle purpose / metadata is a 400 rather than a silent drop.
  • The guard exempts by route. Nothing a caller can write in a chat send body opens the boundary any more.
  • RUNTIME_NOTICE_METADATA_KEY joins the server-owned keys in stripUntrustedMetadataKeys, alongside agentFinalText and the ask-agent / GitHub-task markers. An inbound copy is now stripped on every write path, so the forgery can't just move to an unbridged chat and be laundered back.
  • The marker is set through a trusted SendMessageOptions.runtimeNotice flag, matching the existing askAgentRequestId / allowFirstChatOrientation pattern.
  • The route still requires chat membership — a narrower capability, not an open door.

Client runtime updated: new sdk.postRuntimeNotice(chatId, content), used by postProviderFailureRuntimeNotice and by both codex usage-limit sites. Provider-failure notices still deliver into bridged chats.

One honest limitation. The new route accepts client-supplied notice text. That matches the trust model of the Feishu bridge route itself, which is the architecture this reuses. Making the text fully server-authored would mean relocating formatProviderFailureRuntimeNotice and redactErrorPreview into @first-tree/shared, and redactErrorPreview's module path is pinned by provider-boundary-guard.test.ts and provider-support-export-allowlists.ts — a much larger, riskier change than this review calls for. What is closed is the part that mattered: the exemption is no longer expressible in a request body, and the marker can no longer be minted by an agent credential.

MEDIUM 4 — probe leak + stale docs — fixed (message route: disputed)

  • Invite route: confirmed and fixed. POST /:chatId/participants ran assertAgentMutableChat before any membership check (authz happened later, inside addParticipantinviteParticipantsToChat). A non-member with a guessed chat UUID could distinguish bridged chats by the error. Now assertParticipant runs first. Same speaker-level check the invite service applies, so no legitimate invite is affected.
  • Message route: disputed — already correct. POST /:chatId/messages calls chatService.assertParticipant at the top of the handler, before the guard. No ordering change needed. Pinned by a new test that walks a non-member through both routes against a bridged and an ordinary chat and asserts the errors are indistinguishable.
  • Docs: new "Chats bridged to a Feishu conversation" section in docs/cli-reference.md with a per-command table.
  • Wording corrected in the 403 message, the docs, and the QA case: the boundary blocks messages and membership changes, not all writes. chat update, chat archive, personal read/pin state and all reads keep working. "Read-only" would send an agent hunting for a workaround it doesn't need.

Tests

One regression per finding:

Finding Test
1 chat-feishu-context-guard.test.ts--agent <other> refuses and createTaskChat is never called; inconclusive lookup refuses; ordinary --agent create still works without consulting the overridden agent
2 feishu-agent-readonly.test.ts — forged runtimeNotice from an ordinary agent credential → 403; genuine notice via the dedicated route → 201 with the server-stamped marker; smuggled marker stripped even in an unbridged chat; route is membership-gated and rejects a non-strict body. Plus agent-final-text-purpose.test.ts for the strip
3 feishu-web-readonly.test.ts detach case + the existing agent-scope detach case — identical release point in both scopes
4 feishu-agent-readonly.test.ts — non-member gets an indistinguishable error on bridged vs ordinary chats, for both invite and message routes

The bridge regression in feishu-cli-preflight.test.ts still passeschat send refused and feishu intent delivering on the same chat.

Verification

pnpm check ✅ · pnpm typecheck ✅ (9/9 packages)

Docker was available and the server testcontainer suites really ran:

Suite Result
server 294 files, 3450 passed
client 199 files, 2558 passed, 7 skipped
web 252 files, 2365 passed
shared 77 files, 926 passed
cli 117 passed, 3 failed

The 3 CLI failures are in daemon-refresh-unit.test.ts and are environmental and pre-existing — the machine's ~/.local/bin/first-tree-dev shim is a symlink into a different, deleted worktree. That file is not in this diff and the failure is unrelated to this change.

chat-attention-commands-extra.test.ts needed a fixture update: its stub SDK had no getChatDetail, which the old fail-open silently swallowed. Adding it makes those cases deterministic regardless of ambient FIRST_TREE_AGENT_ID.

Also updated packages/qa/cases/cross-surface/feishu-agent-channel.md with the probe-oracle, --agent, forged-exemption and Web-detach branches.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new head resolves the previous --agent identity bug, shares the active-binding predicate across Web and agent scopes, fixes invite authorization ordering, and adds the missing CLI documentation. The route-layer placement still correctly preserves the trusted Feishu intent path.

Two blockers remain:

  1. Blocker — the dedicated runtime-notice route is still an agent-callable message bypass (packages/server/src/api/agent/messages.ts:112-133). The same ordinary agent credential blocked by POST /messages can call POST /runtime-notices with any 1–4000 character content, and the server then stores that arbitrary text in the active bridged chat. Moving the exemption from body markers to a public agent route makes the marker server-authored, but it does not make the caller trusted; membership is exactly the authority an ordinary chat send caller already has. A request such as { "content": "ordinary reply" } therefore still crosses the 403 boundary. Please require authority unavailable to the agent tool/API surface (or make the notice semantics/content genuinely server-authored from a constrained event), and add a regression proving an ordinary agent credential cannot use this endpoint to publish arbitrary chat text.

  2. Blocker — the advertised fail-closed CLI check still fails open on an omitted field (apps/cli/src/core/feishu-chat-context.ts:104-111). ChatDetailReader intentionally allows externalChannel to be absent, but the resolver maps every value except "feishu" — including undefined from an older/malformed server — to unbridged. The test explicitly pins this. That lets chat create/chat open proceed even though the bridge state is unknown, contradicting the new tri-state contract and the statement that inconclusive answers refuse. Only explicit null should mean unbridged; an absent/unknown value should return unknown and use FEISHU_CHAT_CONTEXT_UNKNOWN.

Documentation follow-up: the new CLI table lists chat detail, read/unread, and pin as commands, but those commands are not registered in apps/cli/src/commands/chat/index.ts. Please label the table as operations/surfaces or list only actual CLI commands. The Web 403 still says the chat is wholly “read-only” even though this change deliberately preserves personal-state and metadata writes; align that wording with the corrected contract.

Human sign-off remains required for the deliberate behavior change that restores Web structural writes after a binding detaches. The active-only rule is internally consistent and avoids permanently freezing the chat, but it should not be treated as approved until yuezengwu explicitly confirms it.

Core contract note: this head adds the dedicated runtime-notice request/API shape in addition to ChatDetail.externalChannel; there is still no database schema or migration change.

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation: request changes

  • Rationale: The original detach and --agent blockers are fixed, but the new runtime-notice endpoint remains an agent-callable message bypass and the advertised fail-closed CLI check still treats an omitted signal as unbridged.

Risk level: A

  • Path baseline: includes apps/cli/** and packages/client/** -> A
  • Semantic lift: the follow-up adds a new agent write capability and changes the runtime/server protocol; no further grade above A

PR summary

  • Author / repo: liuchao-001 / agent-team-foundation/first-tree
  • Problem: Prevent agents in Feishu-bridged conversations from writing First Tree-only messages or membership changes that Feishu participants cannot observe.
  • Approach: Share one active-binding predicate across Web and agent routes, resolve CLI origin context with the session identity, guard membership before bridge state, and move exceptional provider notices to a dedicated endpoint.
  • Impacted modules: agent chat/message routes, Feishu binding service, CLI chat preconditions, Client runtime/provider handlers, shared message contracts, Web boundary tests, docs, and QA guidance

Review findings
❌ 1. The dedicated route does not make the runtime-notice exemption unforgeable. It uses the same agent authentication as the ordinary message route, requires only chat participation, and accepts up to 4,000 characters of caller-chosen text; postRuntimeNotice is also a public SDK method. Any speaking agent that could previously forge { purpose, metadata.runtimeNotice } can now call /runtime-notices and persist the same arbitrary First Tree-only message in a bridged chat. A route name and strict body shape do not add caller authority. This differs from feishu intent, which additionally proves Bot ownership and the exact bound conversation/target. Please gate this path with authority unavailable to the agent-authored call surface (or make the server derive a tightly closed notice from a trusted runtime event); otherwise the claimed blocker remains open. [R4 / packages/server/src/api/agent/messages.ts:112, packages/client/src/cloud/sdk.ts:447, packages/server/src/api/agent/feishu.ts:123]
❌ 2. The tri-state CLI resolver still maps an omitted externalChannel to unbridged. ChatDetailReader deliberately makes the field optional for older Servers, but line 111 treats every value except "feishu" — including undefined or a malformed value — as an affirmative unbridged answer. That lets chat create and chat open proceed precisely when bridge state is unknown, contradicting the new fail-closed contract. Only explicit null should resolve to unbridged; absence should return unknown and use FEISHU_CHAT_CONTEXT_UNKNOWN, with the existing older-server test inverted accordingly. [R4/R5 / apps/cli/src/core/feishu-chat-context.ts:104]
❌ 3. Moving every notice producer to a new endpoint also breaks the exceptional signal across independently deployed Client/Server versions. An older Client talking to this Server still posts the old decorated /messages request and is rejected by the unconditional Feishu guard; a new Client talking to an older Server posts /runtime-notices and gets 404. In both rollout directions, the provider-failure row this exception exists to preserve disappears. Please provide a staged compatibility/fencing contract and regression coverage for both version directions rather than assuming lockstep deployment. [R5 / packages/client/src/runtime/runtime-notice.ts:42, packages/server/src/api/agent/messages.ts:59]
✅ 4. The previous findings are otherwise addressed: Web and agent scopes now share the active-only binding predicate; chat create --agent resolves the origin with the session identity; invite authorization precedes bridge disclosure; and the CLI reference/QA contract now describe the intended command boundary.

Action taken

  • Submitted request changes on head 7a3756e.

@yuezengwu

Copy link
Copy Markdown
Contributor

Human sign-off confirmed: use the active-binding-only contract. While an im_chat_bindings row is active, Web and agent message/membership writes remain restricted; once it is detached, both surfaces restore ordinary First Tree writes.

This resolves only the requested product decision. The three technical blockers in the current reviews remain open: the agent-callable runtime-notice bypass, omitted externalChannel still resolving as unbridged, and the two-direction Client/Server rollout compatibility gap.

@liuchao-001

Copy link
Copy Markdown
Contributor Author

Thanks for the second round — the convergence between reviewers was useful. Three of you independently flagged the externalChannel fail-open, which settled it. Fixes for that, the unguarded DELETE /participants/:agentId and message-edit routes, the rolling-deploy incompatibility on the runtime-notice endpoint, and the documentation problems (including the table listing commands that do not exist) are in progress and will arrive as a separate push with a per-finding summary. The branch will be updated from main in the same push.

Two items I would rather resolve by agreement than decide unilaterally.

1. Posture of POST /runtime-notices

I agree with the substance: this route is not a provenance boundary. Any agent credential with chat membership can call it, and the server then stamps a trusted marker. Calling that "unforgeable" was wrong and I am removing every such claim from the code, comments, docs, and PR description.

What I would like to agree on is the scope of the fix in this PR. Two exits were offered: build a chat/turn-scoped daemon capability, or accept it as a misuse-prevention rail and describe it accurately.

My proposal is the second, with a follow-up issue for the first. The reasoning:

  • This PR's contract is the agent-facing write boundary in bridged chats. A daemon-exclusive capability is a new authentication primitive whose blast radius is the whole runtime, not just Feishu chats. Landing it here makes both changes harder to review and harder to revert independently.
  • The pre-existing behaviour it replaces was strictly worse: before this PR the same content could be smuggled through metadata on the ordinary message route, in any chat. That path is now closed and the marker is stripped on every write path. So this PR is a net reduction in reachable surface even without the capability.
  • Naming it honestly removes the actual hazard, which is that a future reader trusts the marker as provenance.

If either of you considers the capability a merge blocker rather than a follow-up, say so and I will implement it in this PR instead. I am not attached to the smaller scope — I would just rather not expand an auth primitive quietly inside a Feishu PR.

2. Real-tenant Feishu loopback — the remaining merge gate

I do not consider this mergeable until an end-to-end ingress → agent → intent → bot reply has been proven against a real tenant on this exact build. Local suites cannot substitute for it.

Investigation concluded that the existing group cannot provide a zero-disturbance verification: valid ingress has to produce a visible message and wake the agent, the bot's reply is likewise visible and written to immutable canonical history, and running the exact PR build would require taking the live Bot's lease, which affects the production Bot.

The workable option is a separate QA App/Bot with its own agent/org and a disposable group, deployed at the exact commit with a single lease owner, exercising one uniquely-marked round trip.

Who can provision that? If a QA App/Bot is not available, the fallback is an explicit maintenance window on the existing group using a QA identity distinct from the live Bot — which is low-disturbance, not zero, and needs someone to accept that tradeoff.

Happy to be argued out of either position.

@yuezengwu

Copy link
Copy Markdown
Contributor

On item 1, I still consider the current POST /runtime-notices shape a merge blocker.

Correcting the provenance language is necessary, but it does not change the enforced boundary: the same ordinary agent credential rejected by POST /messages can persist arbitrary caller-chosen text through POST /runtime-notices. That remains a message-write bypass, not only a misleading marker.

I do not think this forces the full daemon-capability design into this PR. A narrower in-scope fix is possible if the server accepts a closed notice kind and authors the complete user-visible text from a fixed template. If detailed caller-supplied text or error previews must remain, then the route needs authority unavailable to the ordinary agent API/tool surface. Deferring either enforcement change to a follow-up would leave this PR's advertised agent write boundary incomplete.

On item 2, I agree that the separate QA App/Bot plus disposable group is the safer exact-build loopback. I am routing the provisioning decision to yuezengwu; I cannot commit the shared QA identity or a production maintenance window on the team's behalf.

The origin check that gates `chat create` / `chat open` treated an
unreadable answer as "not bridged". A server older than
`ChatDetail.externalChannel` omits the field, and a session that names a
chat with no agent id could not read it at all — both were silently
allowed, so a rolling deploy turned the guard off. Only an explicit
`null` means unbridged now; absent, unrecognised, unreachable, or
unreadable all refuse with an actionable reason. The operator case is
unchanged and pinned by tests: no chat context at all still allows
without a lookup, and never builds an SDK on a machine with no agent.

Membership removal and message edit were unguarded, so an agent could
still mutate shared membership of a bridged chat or rewrite history the
Feishu humans cannot see. Both now apply the boundary, after the
membership check so neither can be used to probe binding state.

Runtime notices lost their rolling-deploy story when they moved to a
dedicated endpoint: a newer client met servers without the route, and an
older client's notices were refused. The SDK now falls back to the legacy
send shape on 404, and the server recognises that exact shape, sharing
one definition so the halves cannot drift. Both directions are covered.

Corrects the security framing throughout. The notice endpoint is
membership-gated exactly like an ordinary send, so it is a
misuse-prevention rail carrying a client-runtime-reported notice, not an
unforgeable authorization boundary — the code, comments and docs now say
so. The Web 403 no longer claims "read-only" while personal state keeps
working, the CLI reference no longer documents commands that do not
exist (`chat detail`, `read/unread`, `pin`), and the live-QA case no
longer invites an agent into a chat where invites are refused.
Three test conflicts, all where main and this branch touched the same
provider-test SDK stubs:

- opencode handler, codex app-server extra-coverage: main added
  `serverUrl` / `getAgentContextTreeConfig` to the stub SDK while this
  branch added `postRuntimeNotice`. Both kept.
- pi session-custody: main REWROTE the "active inject before prompt
  write" case into "active inject on a healthy RPC bypasses config
  refresh". Main's rewritten case is kept whole; its "no operator-facing
  write happened" assertion now watches `postRuntimeNotice`, which is
  where a runtime notice goes on this branch.

No product code conflicted, and no guard semantics were touched.
@liuchao-001

Copy link
Copy Markdown
Contributor Author

Round 2 — per-finding disposition

Head is now ecaf3264c (fix commit 464bdecf3 + a merge from main). pnpm check, pnpm typecheck and a full pnpm test are green after the merge, with Docker up so the server testcontainer suites really ran.

A — create / open origin check was fail-open at the edges — fixed

Confirmed both holes. resolveFeishuChatContext collapsed everything that was not "feishu" into unbridged, so a server older than ChatDetail.externalChannel (it omits the field, and the SDK does not re-parse the response through Zod) read as "ordinary chat" — the guard switched itself off for a whole rollout. Separately, the call sites gated on chatId && agentId, so a session naming a chat with no agent id skipped the check entirely.

Now only an explicit null means unbridged. Absent field, unrecognised value, failed lookup, and "chat id present but no session agent to read it as" are all unknown and refuse with FEISHU_CHAT_CONTEXT_UNKNOWN plus what to fix.

The operator case is preserved and pinned. The distinction encoded is no chat context at all vs chat context present but unresolvable: absent FIRST_TREE_CHAT_ID allows without any lookup and never constructs an SDK (checkFeishuChatContext takes a reader factory), so chat open from a human terminal with no agent configured keeps working. Tests cover: no chat id (three shapes), chat id without agent id, field absent, unrecognised value, and a factory that throws.

B — membership removal and message edit unguarded — fixed

Both confirmed. DELETE /participants/:agentId mutated shared membership of a bridged chat, and the edit route let ordinary agent messages and runtime notices be rewritten (only bridge-authored rows were protected, by editMessage's own rule). Both now call assertAgentMutableChat after the membership check, keeping the established ordering so neither can be used as an oracle.

Per the reviewer's suggestion the new tests compare the full response body, not status + code — the actionable guidance is half of what this boundary is for. The outsider probe now covers all four routes and asserts whole-body equality between the bridged and the ordinary chat. One coverage note: while a binding is active the chat-level guard now answers first on the edit route, so feishu-cli-preflight gained an explicit post-detach case proving a delivered provider row stays immutable regardless of binding state.

C — documentation gaps and self-contradiction — fixed

The env-var table now documents that FIRST_TREE_CHAT_ID / FIRST_TREE_AGENT_ID feed the origin check, with a table of exactly which session shapes allow, refuse, or refuse-as-unknown. The live-QA case no longer says "first invite Agent B normally" in a chat where invites are refused: B's membership must be established while no active binding covers the chat, and the refusal itself is the thing checked later.

D — runtime-notice rolling-deploy incompatibility (baixiaohang) — fixed, both directions

Confirmed in both directions and worth stressing that "old client → new server" is not a brief window: clients upgrade on their own schedule.

  • New client → old server: postRuntimeNotice falls back to the legacy send shape on 404 (only 404 — a 403/5xx still surfaces). It lives in the SDK, so all three call sites (provider failure + both Codex usage-limit notices) get it.
  • Old client → new server: the send route recognises the exact legacy wire shape and handles it as the notice it is, with the marker still server-stamped.

Both halves share legacyRuntimeNoticeSendBody / isLegacyRuntimeNoticeSend in shared so they cannot drift.

Please look at this one closely. The compatibility path is body-shaped, which round 1 removed. I kept it narrow — exact match on purpose, format: "text", source: "api", and runtimeNotice as the sole metadata key, which is precisely what the three pre-endpoint call sites emitted — and the round-1 regression test still passes untouched, because a decorated ordinary send is not that shape. The reason I judged this acceptable is in finding F's territory: the runtime-notice endpoint is gated on chat membership and nothing else, exactly like POST /messages, so this body buys a caller nothing it could not get by calling the endpoint directly. If you would rather accept notice loss for older clients in bridged chats than reintroduce any body-shaped path, say so and I will drop it — it is one predicate and one branch.

E — CLI reference documented commands that do not exist — fixed

Confirmed: registered chat commands are send, ask, create, invite, list, history, archive, update, open, and the hidden deprecated set-topic. There is no chat detail, chat read/unread, or chat pin — read/pin are Web personal state, not CLI commands. The table now lists only real commands (adding feishu credential-env, and the two newly guarded operations), and mentions Web personal state as prose instead of inventing commands for it.

F — Web 403 still said "read-only" — fixed

Also inaccurate for a second reason worth naming: the Web scope blocks more than the agent scope, not less — rename and entity follows are structural writes there, while an agent is required to keep topic/description current. The message now names the blocked class ("structural changes… messages, membership, rename and entity follows") and says read/pin/archive still work. Stale copies swept: the QA case's "read-only Web task" framing and the web-boundary test's describe block.

Wording (in scope, per the round-2 note)

The runtime-notice route is no longer described as "unforgeable" or as an authorization boundary anywhere in code, comments, tests or docs. It is a misuse-prevention rail carrying a client-runtime-reported notice: membership-gated exactly like an ordinary send, so any credential that can reach one can reach the other, and nothing verifies a provider actually failed. What the separate route does buy is still stated plainly — the ordinary send path stays uniformly guarded, and the server authors the whole stored row. The runtimeNotice marker is documented as a classification label, not a capability. The route itself is unchanged; narrowing it is left as the open posture question.

Merge from main

Three conflicts, all in provider test SDK stubs. main added serverUrl / getAgentContextTreeConfig to stubs this branch had added postRuntimeNotice to — both kept. main also rewrote pi's "active inject before prompt write" case into "active inject on a healthy RPC bypasses config refresh"; that rewritten case is kept whole, with its "no operator-facing write happened" assertion now watching postRuntimeNotice. No product code conflicted and no guard semantics were touched.

Not done, deliberately

The runtime-notice route is untouched — not daemon-scoped, no chat/turn capability. Out of scope this round.

@yuezengwu yuezengwu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new head fixes the fail-closed CLI edges, covers participant removal and message edits with the correct authorization ordering, repairs the CLI/Web documentation, and implements both rolling-deploy directions. The trusted Feishu intent path remains isolated correctly.

Two enforcement blockers remain:

  1. Blocker — POST /runtime-notices still defeats the advertised agent write boundary (packages/server/src/api/agent/messages.ts:148-169). The route accepts any 1–4000 character caller-authored text under the same ordinary agent membership authority as POST /messages, then stores it in the bridged chat. Renaming this a misuse-prevention rail makes the limitation honest, but does not change the behavior: the credential refused by the guarded route can still persist an arbitrary First Tree-only reply. Please either accept a closed notice kind whose complete visible text is server-authored, or require authority unavailable to the ordinary agent HTTP/tool surface. A follow-up cannot close a boundary this PR claims to enforce.

  2. Blocker — the legacy-body compatibility path grants authority that some callers do not have on the dedicated endpoint (packages/server/src/api/agent/messages.ts:55-90, packages/server/src/middleware/user-auth.ts:95-105, packages/shared/src/schemas/message.ts:497-509). agent_outbox JWTs are deliberately accepted only for POST /agent/chats/:chatId/messages; they cannot call /runtime-notices. Yet an outbox token can submit the legacy body, skip assertAgentMutableChat, and receive the trusted runtime-notice handling. That directly disproves the comments and PR description saying every such caller could call the endpoint and the body grants nothing. The matcher is also not exact: it does not reject receiverNames or inReplyTo, both of which survive sendMessageSchema.parse; nor does it apply the endpoint's string/length constraint. Add an outbox-token regression on a bridged chat and close this privilege expansion. If legacy compatibility remains, it must be limited to callers that already possess the notice capability and to the actual legacy body shape.

Related comments remain internally contradictory and should be corrected with the enforcement fix: messages.ts says no request-body shape opens the ordinary path even though the legacy branch does exactly that, while services/chat/message.ts:449-456 says runtimeNotice is set only by the dedicated route and is a capability although the legacy message route sets it and the shared contract calls it a classification label.

The real-tenant exact-build Feishu loopback is still an external merge gate; the QA App/Bot provisioning decision is pending with yuezengwu.

Core contract note: this head still adds ChatDetail.externalChannel, the runtime-notice request/API shape, and now a shared legacy wire-shape contract. There is no database schema or migration change.

@baixiaohang baixiaohang left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Recommendation: request changes

  • Rationale: The CLI, route-coverage, authorization-ordering, and documentation gaps are fixed, but the retained runtime-notice surface still lets an ordinary agent write into an active bridged chat, and the new compatibility branch expands that bypass to a narrower token that cannot call the dedicated endpoint.

Risk level: A

  • Path baseline: includes apps/cli/** and packages/client/** -> A
  • Semantic lift: changes agent message authorization and the independently deployed Client/Server protocol; no further grade above A

PR summary

  • Author / repo: liuchao-001 / agent-team-foundation/first-tree
  • Problem: Agents working from a Feishu-bridged conversation can otherwise write First Tree-only messages or membership changes that the humans following the conversation in Feishu never see.
  • Approach: Guard every agent message/membership route against active Feishu bindings, share the live-binding predicate with Web, fail closed for CLI-only origin checks, and preserve exceptional runtime notices across mixed Client/Server versions.
  • Impacted modules: CLI chat preconditions, shared chat/message contracts, Client SDK/runtime/provider handlers, agent and Web chat routes, Feishu binding services, docs, tests, and cross-surface QA guidance

Review findings
❌ 1. The original runtime-notice blocker is deliberately unchanged. POST /runtime-notices accepts arbitrary caller-authored text under the same ordinary agent membership authority as POST /messages, then stores it in the active bridged chat. Calling it a misuse-prevention rail accurately documents the limitation but does not change the user-visible effect: the credential refused by the advertised message boundary can still persist an arbitrary First Tree-only reply. A follow-up cannot complete a boundary this PR claims to enforce. Require authority unavailable to the ordinary agent HTTP/tool surface, accept a closed notice kind whose complete visible text is server-authored, or remove the bridged-chat exception until that capability exists. [R4/R5 / packages/server/src/api/agent/messages.ts:119]
❌ 2. The legacy compatibility branch is a privilege expansion, not merely another spelling of the dedicated endpoint. An agent_outbox JWT is intentionally accepted only for POST /agent/chats/:chatId/messages, so it cannot call /runtime-notices; nevertheless, it can submit the legacy body, skip assertAgentMutableChat, and receive trusted runtime-notice handling. The matcher is also not the claimed exact five-field shape: it neither constrains content to the endpoint's string/length contract nor excludes receiverNames and inReplyTo, both of which survive sendMessageSchema.parse and shape the stored row. Add the scoped-outbox regression and redesign compatibility so it cannot grant notice authority to a caller that lacks it. Securing finding 1 while retaining this unauthenticated body branch would simply reopen the same boundary. [R4/R5 / packages/server/src/api/agent/messages.ts:55, packages/server/src/middleware/user-auth.ts:95, packages/shared/src/schemas/message.ts:497]
⚠️ 3. The source comments still describe mutually incompatible contracts: the route says no body shape opens the ordinary send path, while the legacy branch does; SendMessageOptions.runtimeNotice says it is set only by the dedicated route and is a capability, while the message route now sets it and the shared schema calls the marker a classification label. Align these statements with the enforcement outcome so future callers do not treat the service option as a provenance or authorization guarantee. [R1/R5 / packages/server/src/api/agent/messages.ts:133, packages/server/src/services/chat/message.ts:98, packages/server/src/services/chat/message.ts:449]
✅ 4. The previous non-notice findings are addressed: only explicit externalChannel: null is unbridged; half-configured session context refuses; edit/removal routes authorize membership before the bridge guard; the Web wording and CLI command table are corrected; and active-only post-detach behavior has human sign-off.

Action taken

  • Submitted request changes on head ecaf3264c.

@yuezengwu

Copy link
Copy Markdown
Contributor

There is one more product-completeness blocker in the outbound path.

Workspace currently does not show evidence that the Agent actually sent the message through official lark-cli. POST /feishu/intents writes the canonical Agent message before the external CLI call, and chat-view.tsx renders that preflight row with → Feishu. This PR's new regression likewise stops after proving that the intent row was stored; it never observes the lark-cli execution or a provider-side message.

First Tree already has two possible observation inputs, but neither is connected to durable outbound chat history here:

  • the runtime captures shell/tool calls as session_events, including the lark-cli invocation and result preview, but those are transient work traces and completed-turn tool rows are intentionally removed from the Workspace timeline;
  • the Feishu ingress path receives provider messages, but ingestFeishuMessage deliberately drops messages whose sender is the bound Bot as bot_echo, so that path cannot confirm/materialize the Bot's outbound row either.

The consequence is that an intent followed by a failed or omitted CLI call is indistinguishable in Workspace from a successfully delivered Feishu message. Conversely, a direct observed lark-cli send without the preflight row is not projected as durable chat history.

Please close the loop by correlating actual outbound execution/provider observation with the canonical intent (the canonical message id is already the lark-cli idempotency key), with dedupe/retry semantics and a visible distinction between intent-only, delivered, and failed/unknown. Add a regression that starts from an observed lark-cli send or provider-side Bot message and proves the corresponding Workspace chat row/state. Do not create a second duplicate message for the same intent.

This is separate from the existing runtime-notice authorization blockers. No database migration is present in the current head; if the delivery-state solution adds a durable field/table, that schema contract needs explicit human review.

@baixiaohang

Copy link
Copy Markdown
Collaborator

I agree that the missing provider-delivery observation is real, but it predates this PR and should not expand this guard change into a new durable delivery-state subsystem.

For this PR, I would require the narrower truthful-state fix: the canonical row created by feishu intent must be presented as intent recorded / delivery unconfirmed, not as evidence that the message was sent. Please replace the bare → Feishu treatment and any “record the delivery” guidance with wording that says to record the intent and then send through official lark-cli --as bot.

Confirmed delivery, failed/unknown state, correlation with tool/provider observation, and retry/dedupe semantics should be a separately designed follow-up with an explicit owner. If that design adds a durable field or table, it should receive its own human schema review.

This scope decision does not change the two current enforcement blockers: the ordinary-agent runtime-notice write bypass and the legacy-body/outbox privilege expansion remain merge blockers for this PR.

@yuezengwu

Copy link
Copy Markdown
Contributor

Human scope decision confirmed: accept the split proposed above.

For this PR, the outbound-history requirement is now:

  1. Present the canonical row truthfully as intent recorded / provider delivery unconfirmed. Replace the bare → Feishu treatment and sweep all guidance that says “record the delivery” so it instead says to record the intent and then send with official lark-cli --as bot.
  2. Create a separately owned follow-up for durable provider-delivery observation: correlate runtime/tool or provider evidence with the canonical intent, define delivered/failed/unknown states and retry/dedupe semantics, and use an independent projection rather than mutating the canonical message row. Link that follow-up here before merge.

The full delivery-observation subsystem is therefore no longer a blocker for #2344 once those two scoped requirements are satisfied. The existing runtime-notice enforcement blockers remain unchanged: ordinary-agent arbitrary-text access and the legacy-body/agent_outbox privilege expansion still need to be closed in this PR.

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.

3 participants