[codex] Add explicit channel memory for messaging channels - #6051
Conversation
…e-pr-6051-conflicts # Conflicts: # packages/channels/base/src/ChannelBase.ts # packages/cli/src/commands/channel/start.test.ts # packages/cli/src/commands/channel/start.ts
|
Thanks for the PR! Template looks good ✓ On direction: Per-chat channel memory for messaging channels fills a real gap — teams using channels need persistent context (env defaults, release norms, ownership notes) per-chat without repeating it every session. This sits squarely within qwen-code's channel system and complements the existing session/instructions infrastructure. No direct Claude Code analogue, but the area is clearly relevant to qwen-code's channel feature set. On approach: The scope is well-scoped — three commands ( Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 方向：按聊天保存频道记忆是消息频道功能的实际需求——团队需要在每个聊天中持久化环境默认值、发布规范或负责人信息，而不必每次重复。这在 qwen-code 的频道系统范围内，与现有的 session/instructions 基础设施互补。 方案：范围合理——三个命令（ 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Code ReviewNo blocking issues found. Independently, before reading the diff: I'd have built a file-backed per-channel+chat memory store in What the implementation gets right:
One actionable note (pre-existing, not blocking this PR): Test Results
tmux real-scenario testing: N/A. Channel memory is backend logic exercised through messaging adapters (Telegram, Feishu, QQ). There is no CLI TUI surface to drive — the feature's behavior is fully covered by the unit test suites above (command handlers, authorization, group guards, prompt injection, sanitization, concurrency). Prior maintainer verification reports on this PR include real-binary E2E harness tests (50/50 assertions), mutation testing (all guards proven load-bearing), and 30-way concurrent append probes (zero cap violations). These are independent confirmations that go beyond what unit tests alone can show. 中文说明代码审查未发现阻塞性问题。 独立方案（读 diff 前）： 我会在 实现优点：
一个可操作的建议（既有问题，不阻塞本 PR）： 测试结果
tmux 真实场景测试：不适用。 频道记忆是通过消息 adapter（Telegram、Feishu、QQ）使用的后端逻辑，没有 CLI TUI 界面可驱动——功能行为已被上述单测套件完整覆盖。 此前维护者验证报告包括真实编译产物的端到端测试（50/50 断言通过）、变异测试（所有守卫均承重）、30 路并发 append 探针（零超限）。这些是超越单测的独立确认。 — Qwen Code · qwen3.7-max |
|
This PR is well-designed and well-tested. The implementation does exactly what the description says: per-chat channel memory with strict authorization, group-chat guards, prompt-injection sanitization, and correct concurrency handling. The approach matches what I'd have done independently, and I don't see a simpler path that covers the same ground. Every security guard is load-bearing (proven by mutation testing in prior verification rounds). The test suites pass clean (272 tests across three packages). Typecheck is clean. The only gap — The design choices I'd want to flag for awareness (not blocking):
Approving. ✅ 中文说明这个 PR 设计合理、测试充分。实现完全符合描述：按聊天的频道记忆，带严格鉴权、群聊守卫、防 prompt 注入的净化和正确的并发处理。方案与我独立想到的吻合，没有看到更简的路径能覆盖同样场景。 每个安全护栏都承重（此前变异测试证明）。三个包的测试套件全部通过（272 个测试），类型检查无错误。唯一的缺口—— 提醒注意（非阻塞）：
批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
3 Critical findings and 4 Suggestions below. The Critical items are (1) a persistent prompt-injection vector via stored channel memory, (2) a TOCTOU race that lets concurrent appends silently bypass the 1 MB cap, and (3) unhandled errors from the new slash-command callbacks that can surface as generic failures to users and operators.
— qwen3.7-max via Qwen Code /review
| this.channelMemoryTarget(envelope), | ||
| ) | ||
| )?.trim(); | ||
| if (channelMemory) { |
There was a problem hiding this comment.
[Critical] Persistent prompt injection via stored channel memory
Channel memory content is concatenated directly into the agent prompt here without going through sanitizePromptText. The codebase already applies sanitizePromptText to user messages (e.g. ChannelBase.ts:1105) to strip PROMPT_UNSAFE_INVISIBLES — bidi overrides (U+202A–U+202E), C1 controls including NEL U+0085, Unicode line/paragraph separators U+2028/U+2029, zero-width chars, and C0/DEL controls. Stored channel memory bypasses all of those defenses.
An authorized user who runs /remember-channel with crafted content (e.g. [SYSTEM]: ignore prior instructions… or text containing bidi overrides for a trojan-source attack) stores it verbatim. On every subsequent new session in that chat, the unsanitized text is prepended ahead of the real user message and is seen by the model as authoritative context. It also survives /clear and persists across senders — any user in the chat receives the injected instructions, even if they are not in allowedUsers (since the prompt-prepend path does not gate on isAuthorizedForChannelMemory, see related Suggestion).
Apply the same sanitizer used for user messages:
| if (channelMemory) { | |
| if (channelMemory) { | |
| context.push( | |
| `Channel memory for this chat:\n${sanitizePromptText(channelMemory)}`, | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
|
|
||
| await fs.mkdir(path.dirname(filePath), { recursive: true }); | ||
| await fs.appendFile(filePath, `${entry}\n`, 'utf8'); | ||
| return { changed: true, filePath }; |
There was a problem hiding this comment.
[Critical] TOCTOU race lets concurrent appends bypass the 1 MB cap
The size check at line 106 (getExistingSize via fs.stat) and the actual write at line 111 (fs.appendFile) are separated by fs.mkdir and an await, so two concurrent /remember-channel calls targeting the same file can both read a small existingSize, both pass the cap, and both write. The result is a file that silently grows past MAX_CHANNEL_MEMORY_BYTES.
Worse, readChannelMemory silently returns '' for oversized files (no log, no warning), so after the race the memory becomes invisible to both the /channel-memory command and the prompt-injection path — a silent data-loss mode that is only discoverable by ls-ing the hashed directory on disk.
Make the check and write atomic on the same open file descriptor:
| return { changed: true, filePath }; | |
| const dir = path.dirname(filePath); | |
| await fs.mkdir(dir, { recursive: true }); | |
| const fh = await fs.open(filePath, 'a+'); | |
| try { | |
| const stat = await fh.stat(); | |
| if (stat.size + appendBytes > MAX_CHANNEL_MEMORY_BYTES) { | |
| throw new Error('Channel memory exceeds maximum size'); | |
| } | |
| await fh.appendFile(`${entry}\n`, 'utf8'); | |
| } finally { | |
| await fh.close(); | |
| } | |
| return { changed: true, filePath }; |
— qwen3.7-max via Qwen Code /review
| return true; | ||
| } | ||
| await channelMemory.appendChannelMemory( | ||
| this.channelMemoryTarget(envelope), |
There was a problem hiding this comment.
[Critical] Unhandled errors from memory callbacks propagate out of command handlers
/remember-channel, /channel-memory, and /forget-channel call appendChannelMemory, readChannelMemory, and clearChannelMemory with no try/catch. When a callback throws (e.g. Error('Channel memory exceeds maximum size') from appendChannelMemory, a disk-full EACCES, or a transient I/O error), the exception propagates out of handleInbound. Adapters then surface a generic "something went wrong" reply to the user, and the operator-side log lacks the chat ID / thread ID / sender ID needed to triage. On the /remember-channel path, the throw also skips invalidateSessionContext, leaving the in-process session context stale relative to whatever partial state landed on disk.
Other command handlers in this file guard their logic and always send a user-facing message before returning true. These three should follow the same pattern:
| this.channelMemoryTarget(envelope), | |
| const channelMemory = await this.getChannelMemory(envelope); | |
| if (!channelMemory) { | |
| return true; | |
| } | |
| try { | |
| await channelMemory.appendChannelMemory( | |
| this.channelMemoryTarget(envelope), | |
| args.trim(), | |
| ); | |
| } catch (error) { | |
| const msg = error instanceof Error ? error.message : String(error); | |
| await this.sendMessage( | |
| envelope.chatId, | |
| `Failed to save channel memory: ${msg}`, | |
| ); | |
| return true; | |
| } | |
| this.invalidateSessionContext(envelope); | |
| await this.sendMessage(envelope.chatId, 'Channel memory updated.'); | |
| return true; |
Apply the same try/catch + user-visible message pattern to the /channel-memory handler (around readChannelMemory) and the /forget-channel confirm handler (around clearChannelMemory).
— qwen3.7-max via Qwen Code /review
| if (context.length > 0) { | ||
| promptText = `${context.join('\n\n')}\n\n${promptText}`; | ||
| } | ||
| } catch (error) { |
There was a problem hiding this comment.
[Suggestion] Prompt-injection path reads memory regardless of isAuthorizedForChannelMemory
The three slash commands are gated by ensureChannelMemoryAuthorized, which requires allowedUsers.length > 0 && allowedUsers.includes(senderId). This prompt-prepend path at lines 1300–1316 is unconditional: any sender who reaches shouldPrependSessionContext causes a memory read that is concatenated into the agent's context.
Two consequences:
- In an open channel with a non-empty
allowedUserslist, an unauthorized user can send a message and then ask the agent to repeat or summarize the injected memory content — bypassing the command-level gate. - If
allowedUsersis later cleared (or a user is removed), previously written memory still influences every new session. The documentation says "IfallowedUsersis empty, channel memory commands are disabled for everyone", which operators will read as "fully inactive" — but the read path stays active.
Either gate the read the same way the commands are gated (call isAuthorizedForChannelMemory(envelope) before reading), or document explicitly that channel memory is visible to all chat participants through the agent and remove the authorization gate from /channel-memory for consistency.
— qwen3.7-max via Qwen Code /review
| return fs.readFile(filePath, 'utf8'); | ||
| } | ||
|
|
||
| export async function appendChannelMemory( |
There was a problem hiding this comment.
[Suggestion] Oversized-file branch is silent and indistinguishable from "no memory"
When size > MAX_CHANNEL_MEMORY_BYTES, readChannelMemory returns '' with no log, no warning, and no error. The /channel-memory command then reports "No channel memory saved." even though a file with data exists on disk. Combined with the TOCTOU race in appendChannelMemory, this is the likely end state after a concurrent overshoot — and it is undiagnosable from any observable surface (command response, prompt injection, log output).
At minimum, emit a stderr warning so an operator can tell the file exists and is oversized:
| export async function appendChannelMemory( | |
| if (size > MAX_CHANNEL_MEMORY_BYTES) { | |
| process.stderr.write( | |
| `[channel-memory] WARNING: ${filePath} is ${size} bytes (max ${MAX_CHANNEL_MEMORY_BYTES}); treating as empty\n`, | |
| ); | |
| return ''; | |
| } |
Ideally surface this to the /channel-memory command response as well (e.g. return a sentinel or throw, so the handler can say "Channel memory file is too large to read" instead of "No channel memory saved.").
— qwen3.7-max via Qwen Code /review
| } | ||
|
|
||
| export interface ChannelMemoryWriteResult { | ||
| changed: boolean; |
There was a problem hiding this comment.
[Suggestion] Duplicated callback contract with divergent filePath optionality
ChannelMemoryTarget and ChannelMemoryWriteResult are defined in two places: packages/core/src/memory/channel-memory.ts:18-27 and packages/channels/base/src/types.ts:105-120. TypeScript structural typing masks the duplication today, but the filePath field has different optionality: required (filePath: string) in the core implementation, optional (filePath?: string) in the channel-base callback contract. A consumer of ChannelMemoryCallbacks therefore sees filePath as potentially undefined, while the implementation always provides it; any future branch on result.filePath === undefined is dead code.
Worse, a maintainer who adds a field to one definition (e.g. createdAt) gets no compile error — the callback boundary is satisfied structurally, and the divergence is only discovered at runtime.
Either import the shared types from @qwen-code/qwen-code-core (or a shared types package), or at minimum make the optionality consistent and add a // SYNC: keep in lockstep with packages/core/src/memory/channel-memory.ts comment on both definitions.
— qwen3.7-max via Qwen Code /review
| expect(ch.sent[0]!.text).not.toContain('/global-only'); | ||
| }); | ||
|
|
||
| it('/remember-channel appends memory for an allowed user', async () => { |
There was a problem hiding this comment.
[Suggestion] Test coverage gaps around the new channel-memory surface
The new command handlers and memory callbacks have several untested branches that could regress silently:
/remember-channelwith empty or whitespace-only args (theUsage: /remember-channel <text>branch atChannelBase.ts:516-520)./forget-channel confirmwhenclearChannelMemoryreturns{ changed: false }(theNo channel memory saved.branch atChannelBase.ts:576).- Authorization denial with a non-empty
allowedUsersthat does not include the sender (onlyallowedUsers: []is tested for denial; theincludeshalf ofisAuthorizedForChannelMemoryis unverified). - Non-ENOENT errors from
fs.stat/fs.unlinkinreadChannelMemoryandclearChannelMemory(thethrow errorbranches atchannel-memory.ts:86andchannel-memory.ts:126). - Callback-throws behavior from command handlers (the scenario that makes the Critical finding above user-visible).
Add focused tests for each so the authorization, error-propagation, and branch-coverage contracts are pinned.
— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Automated Review — PR #6051
Reviewed the channel memory implementation for persistence correctness, data isolation, memory size limits, and cleanup. Two high-confidence findings below.
Note: This PR contains substantial changes beyond the channel memory feature (see inline comment on scope).
| this.instructedSessions.add(sessionId); | ||
| } | ||
| if (shouldPrependSessionContext) { | ||
| try { |
There was a problem hiding this comment.
[HIGH] Prompt injection reads channel memory without allowedUsers authorization
The prompt injection path calls this.channelMemory?.readChannelMemory(this.channelMemoryTarget(envelope)) directly, without the ensureChannelMemoryAuthorized(envelope) check that gates the three channel memory commands (/remember-channel, /channel-memory, /forget-channel).
This means saved channel memory is injected into the prompt of every sender who starts a new session in this chat — including senders NOT listed in allowedUsers. The PR description states: "Only users listed in allowedUsers can read, write, or clear channel memory."
The implementation diverges from this contract on the read side: unauthorized users cannot use the /channel-memory command, but their prompts still receive the saved memory.
This matters for sessionScope: 'user' (per-sender sessions) and for channels with restricted allowedUsers where some members should not see team-shared context.
Suggested fix: Either (a) gate the injection with the same isAuthorizedForChannelMemory(envelope) check so only authorized senders receive the memory in their prompt, or (b) update the PR description and docs to clarify that channel memory is write-restricted but read-shared with all session participants.
| } | ||
|
|
||
| const appendBytes = Buffer.byteLength(`${entry}\n`, 'utf8'); | ||
| const existingSize = await getExistingSize(filePath); |
There was a problem hiding this comment.
[HIGH] TOCTOU race in appendChannelMemory size check
The size enforcement has a time-of-check-time-of-use gap: getExistingSize(filePath) reads the current file size, then fs.appendFile writes the entry (line 105). Between these two async operations, another concurrent call can read the same pre-write size, also pass the check, and both writes land — pushing the file past MAX_CHANNEL_MEMORY_BYTES.
Once the file exceeds the limit, readChannelMemory (line 82) silently returns '' for all future reads, making the memory invisible to both prompt injection and the /channel-memory command — while the file continues to exist on disk. The user has no recovery path short of manually deleting the file.
The /remember-channel command handler serializes through sessionQueues for a given session, which mitigates this for the common case. But different sessions targeting the same file (e.g., sessionScope: 'user' with multiple authorized senders in the same chat) can still race.
Suggested fix: Use fs.open with 'a+' mode, fstat the fd for current size, check the limit, then write — all on the same file descriptor. This makes the check-and-append atomic from the process's perspective.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Code Review Summary
Found 4 findings (1 Critical, 2 Suggestions, 1 Needs Human Review). The overall design is solid — file-based per-channel memory with serialization is a good approach. The critical item is a race condition in clearChannelMemory that could silently defeat /forget-channel when used concurrently with /remember-channel.
| ): Promise<ChannelMemoryWriteResult> { | ||
| const filePath = getChannelMemoryFilePath(target); | ||
| try { | ||
| await fs.unlink(filePath); |
There was a problem hiding this comment.
🔴 Critical: clearChannelMemory bypasses serializeAppend queue
clearChannelMemory calls fs.unlink directly without going through the serializeAppend queue. This creates a race condition:
- User runs
/remember-channel something→appendChannelMemoryentersserializeAppend, acquires lock, opens file with'a+' - User runs
/forget-channel confirmconcurrently →clearChannelMemorycallsfs.unlink, deleting the file - The pending append's
fs.open('a+')(already holding the old handle or opening after unlink) recreates the file - Result: the clear is silently defeated, memory reappears
Fix: Wrap the unlink in serializeAppend to ensure mutual exclusion with appends:
export async function clearChannelMemory(
target: ChannelMemoryTarget,
): Promise<ChannelMemoryWriteResult> {
const filePath = getChannelMemoryFilePath(target);
return serializeAppend(filePath, async () => {
try {
await fs.unlink(filePath);
return { changed: true, filePath };
} catch (error) {
if (isMissingFile(error)) {
return { changed: false, filePath };
}
throw error;
}
});
}| promptText = `${context.join('\n\n')}\n\n${promptText}`; | ||
| } | ||
| } catch (error) { | ||
| this.instructedSessions.delete(sessionId); |
There was a problem hiding this comment.
🟡 Suggestion: Missing structured logging on prompt injection error path
All three slash command handlers (/remember-channel, /channel-memory, /forget-channel) use this.logChannelMemoryError(...) for structured error logging. However, this catch block in the session context injection path only deletes the session and re-throws — without calling logChannelMemoryError.
This means channel memory read failures during session setup are silently swallowed from the structured log, making production debugging harder.
Fix: Add structured logging before the throw:
} catch (error) {
this.logChannelMemoryError('read', envelope, this.channelMemoryErrorMessage(error));
this.instructedSessions.delete(sessionId);
throw error;
}| } | ||
|
|
||
| private channelMemoryErrorMessage(error: unknown): string { | ||
| return error instanceof Error ? error.message : String(error); |
There was a problem hiding this comment.
🟡 Suggestion: Error messages may disclose absolute filesystem paths
channelMemoryErrorMessage returns error.message verbatim from fs operations. Node.js filesystem errors include the full absolute path (e.g., ENOENT: no such file or directory, open '/home/user/.qwen/channels/memory/...'). These messages are sent back to chat users via the slash command handlers.
Consider returning a generic message to avoid leaking server filesystem layout:
private channelMemoryErrorMessage(error: unknown): string {
return 'An error occurred while accessing channel memory.';
}Keep the detailed error for logChannelMemoryError (which already sanitizes via sanitizeLogText), but don't expose it to end users.
| if (size > MAX_CHANNEL_MEMORY_BYTES) { | ||
| return ''; | ||
| } | ||
| return fs.readFile(filePath, 'utf8'); |
There was a problem hiding this comment.
🔵 Needs Human Review: stat-then-read TOCTOU with concurrent clearChannelMemory
readChannelMemory calls fs.stat then fs.readFile. If a concurrent clearChannelMemory (which bypasses serializeAppend — see finding on line 133) deletes the file between these two calls, fs.readFile will throw ENOENT, which is not caught here — only the fs.stat ENOENT is handled.
This is a narrow race on local filesystem, but becomes relevant given the clearChannelMemory serialization bug. If clearChannelMemory is fixed to use serializeAppend, the risk here is also reduced since reads would be the only non-serialized operation remaining.
Consider adding an ENOENT catch around fs.readFile as well, or wrapping the read in serializeAppend too.
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Incremental Review — Commits 26f050f..6de49e0
Both HIGH findings from the previous review are resolved:
-
Prompt injection auth bypass (FIXED): The prompt injection path now gates
readChannelMemorybehindisAuthorizedForChannelMemory(envelope)and appliessanitizePromptText()to the memory content before injection. Tests confirm unauthorized senders (e.g.bob) do not trigger a memory read, and Unicode bidi overrides are stripped. -
TOCTOU race in
appendChannelMemory(FIXED): The size check and write are now atomic within aserializeAppendin-process queue plus aproper-lockfilecross-process lock. The check reads size from the open file handle (handle.stat()) rather than a separatefs.stat, eliminating the race window. A new concurrent-append test verifies the cap holds under contention.
The error handling additions (try/catch on all three command handlers) are correct and well-tested. The serializeAppend queue implementation is sound — tasks chain on predecessor completion without propagating failures to successors, and the queue map is cleaned up properly.
No new high-confidence findings in this increment.
| } | ||
|
|
||
| let shouldPrependSessionContext = !this.instructedSessions.has(sessionId); | ||
| if (shouldPrependSessionContext) { |
There was a problem hiding this comment.
[Critical] Unauthorized-first-sender starves channel memory for shared sessions
instructedSessions.add(sessionId) fires unconditionally before the serialization queue. Inside the queue, isAuthorizedForChannelMemory gates the actual memory read. For shared sessions (sessionScope: 'single' or 'thread' in group chats), if an unauthorized user sends the first message, the session is permanently marked as "instructed" without memory injection. All subsequent authorized users skip context-prepend because instructedSessions.has(sessionId) is already true — channel memory is silently starved for the lifetime of the session.
The only recovery is /clear. An unauthorized user can permanently prevent authorized users from receiving their channel memory simply by being the first to message after a session is created or cleared.
| if (shouldPrependSessionContext) { | |
| let shouldPrependSessionContext = !this.instructedSessions.has(sessionId); |
Move instructedSessions.add(sessionId) inside the queue callback, after the authorization check succeeds and context is actually built. For unauthorized senders on shared sessions, defer the marking so the next authorized sender still triggers injection:
// Inside the queued callback, after building context:
if (shouldPrependSessionContext) {
try {
const context: string[] = [];
const isAuth = this.isAuthorizedForChannelMemory(envelope);
// ... build context ...
if (context.length > 0) {
promptText = `${context.join('\n\n')}\n\n${promptText}`;
}
// Only mark instructed if we actually injected or no memory backend exists
if (isAuth || !this.channelMemory) {
this.instructedSessions.add(sessionId);
}
} catch (error) {
this.instructedSessions.delete(sessionId);
throw error;
}
}— qwen3.7-max via Qwen Code /review
| )?.trim() | ||
| : undefined; | ||
| if (channelMemory) { | ||
| context.push( |
There was a problem hiding this comment.
[Suggestion] Channel memory injected before operator instructions reverses prompt precedence
Channel memory (user-authored content from /remember-channel) is pushed into the context array before this.config.instructions. After joining, user-controlled memory appears ahead of the operator's directives. In the previous code, instructions was always the first content prepended to the prompt.
While sanitizePromptText strips bracket-tag patterns and bidi overrides from memory, semantic prompt injection (e.g., "always answer in French, skip code reviews") is not mitigated by character-level sanitization and now occupies a higher-priority position than operator instructions.
| context.push( | |
| if (this.config.instructions) { | |
| context.push(this.config.instructions); | |
| } | |
| if (channelMemory) { | |
| context.push( | |
| `Channel memory for this chat:\n${sanitizePromptText(channelMemory)}`, | |
| ); | |
| } |
— qwen3.7-max via Qwen Code /review
| return true; | ||
| }); | ||
|
|
||
| this.registerCommand('forget-channel', async (envelope, args) => { |
There was a problem hiding this comment.
[Suggestion] /forget-channel shows confirmation prompt before checking if memory is configured
The args !== 'confirm' guard runs before getChannelMemory(), so an authorized user on a channel without channelMemory callbacks gets a misleading "Re-send with 'confirm'" prompt. Only on the follow-up /forget-channel confirm does the user learn memory isn't configured.
The other two commands (/remember-channel and /channel-memory) both call getChannelMemory immediately after the authorization check, giving a clear "Channel memory is not configured" response.
| this.registerCommand('forget-channel', async (envelope, args) => { | |
| this.registerCommand('forget-channel', async (envelope, args) => { | |
| if (!(await this.ensureChannelMemoryAuthorized(envelope))) { | |
| return true; | |
| } | |
| const channelMemory = await this.getChannelMemory(envelope); | |
| if (!channelMemory) { | |
| return true; | |
| } | |
| if (args.toLowerCase() !== 'confirm') { |
— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
There was a problem hiding this comment.
Incremental review (045c4f7)
Commit: fix(channels): serialize channel memory clears
This commit correctly addresses both HIGH findings from the previous review cycle:
-
TOCTOU in readChannelMemory -- Fixed by wrapping
fs.readFilein a try/catch that handles ENOENT when the file is deleted betweenstatandreadFile. -
clearChannelMemory serialization -- Fixed by wrapping in
serializeAppend(in-process serialization per filePath) pluslockfile.lock(cross-process safety), consistent with theappendChannelMemorypattern.
Additional improvements:
- User-facing error messages now use a generic string instead of leaking raw error details (EACCES, disk full, etc.) to chat users. Raw errors are still logged to stderr for debugging.
- Added missing
logChannelMemoryErrorcall in the prompt-building catch block for observability. - Tests updated with stderr spy assertions and a new serialization concurrency test.
No new high-confidence findings. LGTM.
| this.channelMemoryErrorMessage(error), | ||
| ); | ||
| this.instructedSessions.delete(sessionId); | ||
| throw error; |
There was a problem hiding this comment.
[Critical] Memory read error in prompt injection path re-throws, silently dropping user message
When readChannelMemory throws during first-session context injection, this throw error propagates through the session queue and causes handleInbound to reject — the user's message is never sent to the agent. No response reaches the chat.
The three slash command handlers (/remember-channel, /channel-memory, /forget-channel) handle the same error class gracefully: they log the detail, send a user-facing error message, and return true. But the prompt injection path does not degrade — a non-critical feature (channel memory read) kills the entire message processing path.
Any transient filesystem issue (NFS timeout, permission change, disk pressure) on the first message of a session silently drops the user's turn.
| throw error; | |
| // Degrade gracefully — don't drop the user's message over a memory read failure |
— qwen3.7-max via Qwen Code /review
| } | ||
| } | ||
|
|
||
| export async function readChannelMemory( |
There was a problem hiding this comment.
[Suggestion] readChannelMemory not serialized with writes
Both appendChannelMemory and clearChannelMemory go through serializeAppend (in-process serialization) + proper-lockfile (cross-process locking). readChannelMemory uses neither — it does a bare fs.stat then fs.readFile with no lock and no queue.
A concurrent read during a write could yield partially-written content. The practical impact is low (writes are fast, the race window is tiny), but the asymmetry creates a structural gap.
| export async function readChannelMemory( | |
| export async function readChannelMemory( | |
| target: ChannelMemoryTarget, | |
| ): Promise<string> { | |
| const filePath = getChannelMemoryFilePath(target); | |
| return serializeAppend(filePath, async () => { | |
| // ... existing stat + readFile logic ... | |
| }); | |
| } |
— qwen3.7-max via Qwen Code /review
| : undefined; | ||
| if (channelMemory) { | ||
| context.push( | ||
| `Channel memory for this chat:\n${sanitizePromptText(channelMemory)}`, |
There was a problem hiding this comment.
[Suggestion] sanitizePromptText strips \n from channel memory, collapsing multi-entry memory into a single line
sanitizePromptText applies .replace(/[\u0000-\u001f\u007f]/g, ' ') which strips \n (U+000A). When multiple /remember-channel entries are stored (each on its own line), they merge into one space-separated line in the prompt, making it harder for the model to distinguish discrete instructions. No test covers this multi-line scenario.
Consider sanitizing each line individually and rejoining with \n, or using a channel-memory-specific sanitizer that preserves structural newlines while still stripping bracket patterns and invisible characters.
— qwen3.7-max via Qwen Code /review
| return '_'; | ||
| } | ||
| return safeName || '_'; | ||
| } |
There was a problem hiding this comment.
[Suggestion] safeChannelName is a many-to-one mapping — channel names like "ops/alerts", "ops-alerts", and "ops alerts" all sanitize to "ops_alerts". Since the channel name is the first path component isolating one channel's memory from another, a collision means two independently configured channels share the same CHANNEL.md file. A user authorized on one channel can read or poison the memory of the colliding channel, which may have a different allowedUsers list.
Consider using a hash-based directory name (optionally prefixed with a truncated slug for debuggability) to make the mapping injective:
| } | |
| function safeChannelName(channelName: string): string { | |
| const slug = channelName.replace(/[^a-zA-Z0-9._-]/g, '_').slice(0, 20); | |
| const hash = createHash('sha256').update(channelName).digest('hex').slice(0, 16); | |
| return `${slug}-${hash}`; | |
| } |
— qwen3.7-max via Qwen Code /review
| }; | ||
| } | ||
|
|
||
| private invalidateSessionContext(envelope: Envelope): void { |
There was a problem hiding this comment.
[Suggestion] invalidateSessionContext only removes the calling sender's session from instructedSessions (resolved via router.getSession with the sender's ID). In user-scoped group chats, each user has a distinct session, so when user A runs /remember-channel or /forget-channel, only A's session is marked for re-injection. Other authorized users' ongoing sessions keep the previously-injected (now stale) channel memory until they individually run /clear.
Consider either:
- Adding a
getSessionsForChat(channelName, chatId, threadId)method toSessionRouterto enumerate all sessions for the chat and invalidate them all. - Documenting this as a known limitation in the channel memory docs, noting that other participants need
/clearto pick up memory changes.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary (Incremental)
This review adds 2 new findings after deduplicating against 51 existing comments. Most previously raised concerns (file permissions, duplicate types, test gaps, senderPolicy design) are already well-covered by prior review rounds.
Additional concern not on a diff line
Loop job path skips channel memory injection (ChannelBase.ts:292): The loop job path calls this.instructedSessions.add(sessionId) and prepends only this.config.instructions, but does not inject channel memory. Since the main message path (lines 1938-1972) now reads and injects channel memory inside the queue callback, loop-triggered messages in the same session will have the session marked as "instructed" without ever receiving channel memory context. This is a behavioral regression — loop jobs get instructions but not the channel memory that a normal first message would receive.
CI Status
6 of 30 checks are failing (Test on all 3 platforms, Integration Tests, Post Coverage Comment, review-pr). The CI failures themselves may warrant investigation before merge.
| context.push(this.config.instructions); | ||
| } | ||
| if (context.length > 0) { | ||
| promptText = `${context.join('\n\n')}\n\n${promptText}`; |
There was a problem hiding this comment.
[Suggestion] No prompt-size budget for channel memory injection
Up to 1 MB of channel memory (the MAX_CHANNEL_MEMORY_BYTES cap) can be prepended to promptText here with no truncation or token budget. On models with smaller context windows, this could crowd out the user's actual message or cause truncation elsewhere.
Consider adding a token/character budget that accounts for the model's context window and the user's prompt length, truncating channel memory with a [...truncated] marker when it exceeds the budget.
| } | ||
| if (shouldPrependSessionContext) { | ||
| const context: string[] = []; | ||
| let channelMemory: string | undefined; |
There was a problem hiding this comment.
[Suggestion] Local channelMemory (string) shadows this.channelMemory (callbacks object)
This local variable has the same name as the instance field this.channelMemory: ChannelMemoryCallbacks declared at line 146. Inside this block, this.channelMemory?.readChannelMemory(...) refers to the callbacks while channelMemory refers to the string result — easy to confuse during maintenance.
Consider renaming to memoryText or channelMemoryContent.
| await this.channelMemory?.readChannelMemory( | ||
| this.channelMemoryTarget(envelope), | ||
| ) | ||
| )?.trim(); |
There was a problem hiding this comment.
[HIGH] Prompt injection path crashes with TypeError when channelMemory is not wired but allowedUsers is non-empty
this.channelMemory is typed ChannelMemoryCallbacks | undefined (optional). The authorization guard isAuthorizedForChannelMemory only checks allowedUsers — it does not verify that this.channelMemory is defined.
When this.channelMemory is undefined and the sender passes the allowedUsers check, the optional chain this.channelMemory?.readChannelMemory(...) resolves to undefined. The subsequent .trim() is then called on undefined, throwing a TypeError.
The catch block handles the error, but it also calls this.instructedSessions.delete(sessionId), which causes the next message in the same session to retry the same failing injection — creating a loop of crash-log-retry on every message.
The CLI (start.ts) always provides channelMemory callbacks, so this is not hit in the standard path today. But ChannelBase accepts channelMemory as an optional option while allowedUsers is a standard config field, so external adapters, integration tests, or future refactors can trigger it.
// Fix: guard on channelMemory existence before the read
if (
this.channelMemory &&
this.isAuthorizedForChannelMemory(envelope) &&
(!this.isSharedSession(envelope) ||
this.config.senderPolicy === 'allowlist')
) {
|
@qwen-code /triage |
|
@qwen-code /resolve |
|
Qwen Code resolved the merge conflicts and pushed the branch update. Merge Conflict Resolution Summary — PR #6051Base branch
Conflicted file
ConflictsConflict 1 — Field declaration (line ~155)
Conflict 2 — Constructor initialization (line ~202)
Commit
Notes
|
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
| ); | ||
| } | ||
| } | ||
| if (this.config.instructions) { |
There was a problem hiding this comment.
[Critical] Loop path does not clean up instructedSessions on memory read failure
When readChannelMemory throws here, the catch block logs the error but does not call this.instructedSessions.delete(sessionId). Line 344 then unconditionally adds to instructedSessions. Compare the main dispatch path (~line 1999) which correctly does this.instructedSessions.delete(sessionId) in its catch block.
A transient I/O error (disk hiccup, lock timeout, NFS stall) during a loop iteration permanently marks the session as "instructed," suppressing all future memory injection for that loop session — even after the transient condition resolves. Since loops run repeatedly, a single momentary failure silently degrades channel memory for the lifetime of the process.
| if (this.config.instructions) { | |
| } catch (error) { | |
| process.stderr.write( | |
| `[${this.name}] channel memory read failed for loop ${job.id} chat ${sanitizeLogText(job.target.chatId, 64)}: ${sanitizeLogText(this.channelMemoryErrorMessage(error), 200)}\n`, | |
| ); | |
| this.instructedSessions.delete(sessionId); | |
| } |
— qwen3.7-max via Qwen Code /review
| @@ -321,6 +305,41 @@ export abstract class ChannelBase { | |||
| 'loop dropped because it is no longer enabled', | |||
| ); | |||
| } | |||
There was a problem hiding this comment.
[Suggestion] Duplicated context-injection logic between loop and main dispatch paths
~30 lines of near-identical logic (authorization check, memory read, instructions prepend, error recovery) appear in both this loop path (lines 307–344) and the main dispatch path (lines ~1978–2015). The two copies have already diverged in error-handling behavior — the loop path is missing instructedSessions.delete on failure, unsanitized error logging, and the second staleness check — which is exactly the risk duplication creates.
Consider extracting a shared private method (e.g., buildSessionContext(promptText, senderId, target, sessionId)) that both dispatch paths call.
— qwen3.7-max via Qwen Code /review
| @@ -5562,6 +5802,55 @@ describe('ChannelBase', () => { | |||
| ]); | |||
There was a problem hiding this comment.
[Suggestion] Missing tests for loop-path memory read failure and staleness check
The main dispatch path has explicit tests for memory read failure ('continues the user prompt when channel memory read fails', 'cleans up first-session context claim when memory read fails') and for /clear races during slow memory reads ('drops a queued turn cleared during a slow memory read'). None of these scenarios are tested for the loop dispatch path (runLoopPrompt).
This gap allowed the missing instructedSessions.delete and the missing second staleness check to go undetected. Suggested tests:
- Loop path retries memory injection after a transient read failure
- Loop path drops the prompt when
/cleararrives during the async memory read
— qwen3.7-max via Qwen Code /review
|
Qwen Code review did not complete successfully. Qwen review aborted with an API error before posting comments. See workflow logs. |
|
@qwen-code /triage |
Addressed by b97d9e6: loop channel-memory read failures no longer mark the session as instructed, and loop prompts now re-check session generation after async memory reads. Added focused regression coverage.
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Review Summary: After thorough analysis across correctness, security, code quality, performance, test coverage, and three undirected audit personas, only 1 genuinely new finding was identified beyond the 51+ existing inline comments. Build passes and all 272 tests pass. ESLint and tsc are clean on all changed files.
— qwen3.7-max via Qwen Code /review
| const context: string[] = []; | ||
| let sessionContextReady = true; | ||
| if ( | ||
| this.channelMemory && |
There was a problem hiding this comment.
[Suggestion] Missing test for loop path unauthorized sender memory skip
The loop path checks isSenderAuthorizedForChannelMemory(job.target.senderId) here to gate memory injection, but no test verifies that an unauthorized sender's loop prompt correctly skips the memory read. The main dispatch path has this test at line ~3327 ("does not read channel memory for unauthorized senders"), but the loop path equivalent is missing.
A sender could pass isStoredLoopTargetAuthorized (which checks groupGate and shared-session authorization) while failing isSenderAuthorizedForChannelMemory (which checks allowedUsers.includes(senderId)). If this guard is accidentally removed, unauthorized loop prompts would silently inject channel memory.
— qwen3.7-max via Qwen Code /review
6 participants
What this PR does
Adds explicit per-chat/thread channel memory for Qwen Code messaging channels. Authorized channel members can save, view, and clear stable context with channel commands, and Qwen Code injects that memory when a fresh channel session starts.
Why it's needed
Messaging channels are often used by teams that need chat-specific context such as environment defaults, release norms, ownership notes, or workflow constraints. Repeating that context manually is noisy, while putting it in channel-wide instructions is too broad. Channel memory gives each chat or thread a small explicit memory surface with strict
allowedUserscontrol.Reviewer Test Plan
How to verify
Configure a channel with
allowedUserscontaining the sender. Run/remember-channel Prefer concise release triage., then/channel-memoryand confirm the saved memory is shown. Run/clear, send a normal prompt, and confirm the new session receives the saved channel memory. Run/forget-channel confirm, then/channel-memory, and confirm it reports no saved memory. Also confirm a sender not listed inallowedUsers, or a channel with emptyallowedUsers, cannot read, write, or clear channel memory.Evidence (Before & After)
N/A for local automated verification. This change is covered by focused unit tests for the core memory store, channel command behavior, prompt injection, concurrency edge cases, and CLI channel startup wiring.
Tested on
Environment (optional)
Node.js workspace tests and build on local macOS.
Risk & Scope
/clearraces, and read failure cleanup.Linked Issues
Closes #6050
中文说明
What this PR does
为 Qwen Code 的消息频道增加显式的按 chat/thread 隔离的频道记忆。授权频道成员可以通过频道命令保存、查看、清除稳定上下文，Qwen Code 会在新的频道会话开始时注入这段记忆。
Why it's needed
团队在消息频道中使用 agent 时，经常需要按聊天保存环境默认值、发布规范、负责人信息或工作流约束。每次手动重复这些上下文很吵，而写入整个频道的 instructions 又过于宽泛。频道记忆提供了一个小而显式的记忆面，并严格由
allowedUsers控制。Reviewer Test Plan
How to verify
配置一个频道，并让
allowedUsers包含发送者。执行/remember-channel Prefer concise release triage.，再执行/channel-memory，确认能看到保存的记忆。执行/clear后发送普通 prompt，确认新会话会收到保存的频道记忆。执行/forget-channel confirm，再执行/channel-memory，确认返回没有保存的记忆。还要确认未列入allowedUsers的发送者，或allowedUsers为空的频道，不能读取、写入或清除频道记忆。Evidence (Before & After)
本地自动化验证无需截图。该改动由 focused unit tests 覆盖，包括 core 记忆存储、频道命令行为、prompt 注入、并发边界，以及 CLI channel startup wiring。
Tested on
Environment (optional)
本地 macOS Node.js workspace 测试和构建。
Risk & Scope
/clearrace 和 read failure cleanup 的回归测试。Linked Issues
Closes #6050