Sanitize subagent result tags - #6027
Merged
Merged
Conversation
Contributor
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
yiliang114
force-pushed
the
codex/subagent-result-tags
branch
from
June 29, 2026 16:48
9a17afd to
a0adf15
Compare
Collaborator
Author
|
Manual tmux verification result: I ran a local tmux-driven probe against the dev CLI with a fake OpenAI-compatible server. The subagent final answer deliberately included Observed result:
I kept this as manual PR evidence instead of committing the local tmux probe script; the committed regression coverage is the focused core test coverage for the agent tool path and background resume path. |
yiliang114
changed the title
yiliang114
force-pushed
the
codex/subagent-result-tags
branch
from
June 29, 2026 16:55
a0adf15 to
501ae9b
Compare
yiliang114
marked this pull request as ready for review
June 29, 2026 16:59
Collaborator
|
Thanks for the PR, @yiliang114! Template looks good — all required sections present, bilingual, linked to #6023. ✓ On direction: this is a straightforward bug fix. #6023 documents real evidence of On approach: the scope feels right — one pure function ( Moving on to code review. 🔍 中文说明感谢 PR，@yiliang114！ 模板完整——所有必需章节齐全，双语，关联了 #6023。✓ 方向：这是一个清晰的 bug 修复。#6023 记录了 方案：范围合理——一个纯函数（ 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal (before reading diff): I'd extract a single pure function that strips The implementation is clean:
No correctness bugs, no security issues, no AGENTS.md violations. The diff is focused — every line serves the stated goal. Test ResultsUnit tests (worktree, PR branch): Both new regression tests ( Direct function tests: Before (installed build)Installed v0.19.3 does not include subagent result sanitization. The bug from #6023 (raw After (this PR)Dev build starts cleanly. Sanitization function verified with 4 targeted inputs: Note on end-to-end tmux testing: The bug requires a subagent model to produce 中文说明代码审查独立方案（看 diff 之前）：我会提取一个纯函数来剥离 实现干净：
无正确性 bug、无安全问题、无 AGENTS.md 违规。diff 聚焦——每一行都服务于既定目标。 测试结果单元测试（worktree，PR 分支）： 两个新回归测试均通过。 直接函数测试： Before（已安装版本）已安装 v0.19.3 不包含子 agent 结果清理。#6023 中的 bug（原始 After（此 PR）Dev 构建正常启动。清理函数经 4 个针对性输入验证通过。 关于端到端 tmux 测试说明： 该 bug 需要子 agent 模型在输出中产生 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a clean, focused bug fix that does exactly what it says. The motivation is well-documented in #6023, the implementation is a minimal pure function applied at the right boundary, and the test coverage is thorough — unit tests for both the normal agent path and the resume path, plus the author's manual tmux probe showing zero tag leakage. Going back to my independent proposal: I would have done the same thing. One function, regex-based stripping, conditional summary unwrap, wired into all call sites. The PR's approach matches — no simpler path was missed. The risk tradeoff (literal No concerns. Approving. 中文说明这是一个干净、聚焦的 bug 修复，完全如其所述。动机在 #6023 中有充分记录，实现是一个最小化的纯函数在正确的边界应用，测试覆盖全面——普通 agent 路径和恢复路径的单元测试，加上作者手动 tmux 探测显示零标签泄漏。 回到我的独立方案：我会做同样的事情。一个函数，基于正则的剥离，条件性 summary 展开，接入所有调用点。PR 的方案完全匹配——没有遗漏更简单的路径。 风险取舍（子 agent 输出中的字面量 无顾虑。批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 29, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
added
category/core
qwen-code-ci-bot
requested changes
Jun 29, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Two Critical issues block merge:
-
Unclosed
<analysis>tag = silent data loss. The$fallback in the analysis regex causes an unclosed<analysis>(common when subagent output is truncated by token limits) to consume all subsequent content — including<summary>blocks. The function returns"". Fix: drop|$from the alternation. -
ReDoS in summary regex. The summary regex has quadratic backtracking on whitespace-heavy input without a closing
</summary>. Empirically: 5,000 chars = 16s. Fix: replace with O(n) string-based tag detection.
Additional suggestions and missed call sites (workflow-orchestrator.ts:469 and :836, forkedAgent.ts:535) are detailed in the inline comments below.
— qwen3.7-max via Qwen Code /review
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
Automated code review — critical findings in this PR were already documented by the existing review. No new findings to add at this commit.
Generated by Claude Code
DragonnZhang
left a comment
DragonnZhang
left a comment
Collaborator
There was a problem hiding this comment.
No new blocking issues found at this head — the two existing unresolved threads on subagent-result.ts (unclosed-<analysis>-consumes-all and summary-regex ReDoS) already cover the highest-severity findings in this diff.
Additional observations from this pass (not duplicating existing threads):
-
The new test mock in
background-agent-resume.test.ts(the sanitization regression test) returns{ totalTokens: 0, totalDurationMs: 0 }fromgetExecutionSummary, omittingoutputTokens,inputTokens, and otherAgentStatsSummaryfields.getCompletionStatsaccessessummary.outputTokens, so the mock producesundefinedthere. The test still passes because it only asserts onregistry.get(agentId)?.result, not on the stored stats. This is a minor test-mock incompleteness rather than a production bug, but a strict-mode typecheck on the test file would flag it. -
forkedAgent.ts:535andworkflow-orchestrator.ts:469,836are not touched by this PR and still pass rawgetFinalText()output toward callers. The existing thread at line 8 already flags this; confirming the observation is accurate.
— claude-sonnet-4-6 via Qwen Code /review
Generated by Claude Code
qwen-code-ci-bot
requested changes
Jun 30, 2026
Collaborator
Author
|
@qwen-code /triage |
…-tags # Conflicts: # packages/core/src/tools/agent/agent.ts
Collaborator
|
Thanks for the PR, @yiliang114! Template looks good — all required sections present, bilingual, linked to #6023. ✓ On direction: this is a clean bug fix. #6023 documents real evidence — session JSONL shows On approach: minimal and focused — one pure function ( Moving on to code review. 🔍 中文说明感谢 PR，@yiliang114！ 模板完整——所有章节齐全，双语，关联了 #6023。✓ 方向：清晰的 bug 修复。#6023 记录了真实证据——会话 JSONL 显示子 agent 最终文本中的 方案：最小化且聚焦——一个纯函数 ( 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
|
Code review: I independently proposed what I'd do to fix this bug — one pure sanitizer function at the subagent→parent boundary that strips The implementation is clean:
Test results (289 total, all passing):
Core package typecheck ( Smoke test (tmux, dev build from PR branch): CLI runs cleanly, no regressions. The bug fix targets subagent return-text processing which requires a subagent model to produce No blocking issues. ✅ 中文说明代码审查： 我独立提出了修复此 bug 的方案——在子 agent→父 agent 边界添加一个纯清理函数，剥离 实现干净：
测试结果（共 289 个，全部通过）： 见上表 Core 包类型检查：clean ✓ 冒烟测试（tmux，PR 分支 dev 构建）： CLI 正常运行，回答 "4"，无回归。bug 修复针对子 agent 返回文本处理，需要子 agent 模型产生 无阻塞问题。✅ — Qwen Code · qwen3.7-max |
Collaborator
|
Clean, focused bug fix. The motivation is well-documented in #6023 with real session JSONL evidence, the implementation is a minimal pure function applied at exactly the right boundary, and all 289 tests pass with a clean typecheck. Going back to my independent proposal: I'd do the same thing — one function with depth-tracking for nested analysis blocks, conditional on successful termination, wired into all return-to-parent call sites. The PR's approach matches. The implementation handles the tricky edge cases well: nested The risk tradeoff (literal No concerns. Approving. 中文说明干净、聚焦的 bug 修复。动机在 #6023 中有真实会话 JSONL 证据支持，实现是一个最小化的纯函数应用在正确的边界，全部 289 个测试通过且类型检查干净。 回到我的独立方案：我会做同样的事情——一个带深度跟踪的函数处理嵌套 analysis 块，仅在成功终止时生效，接入所有返回父级的调用点。PR 的方案完全匹配。实现很好地处理了棘手的边界情况：嵌套 风险取舍（子 agent 输出中的字面量 无顾虑。批准。 — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jun 30, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
requested changes
Jun 30, 2026
chiga0
previously approved these changes
Jun 30, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Overview — Independent Review (Phase 1 blind → Phase 2 cross-validation)
Verdict: LGTM. Well-structured sanitization fix that evolved significantly across 8 commits in response to review feedback. The implementation moved from a fragile regex-based approach to a proper state machine parser. All 12 existing inline findings (5 Critical, 5 Suggestion) are stale against HEAD 54f5f7d9.
Round 0 — Design & Intent
The PR fixes a real bug (#6023): subagent final results leak <analysis> and <summary> internal tags into the parent context, appearing as implementation markers in the model and daemon UI. The fix applies a pure sanitization function at the subagent-to-parent boundary across all 5 call sites (standard agent, worktree agent, background resume, forked agent, workflow orchestrator). Raw transcripts are preserved for debugging — only the parent-facing result is sanitized.
Round 1 — Architecture
toModelVisibleSubagentResult is a well-scoped pure function in its own module (subagent-result.ts):
- Terminate-mode guard: Non-GOAL terminations (ERROR, TIMEOUT, CANCELLED) return raw text unchanged — preserves crash/timeout diagnostics
stripAnalysisOutsideSummary: State machine parser that correctly handles nested tags, unclosed analysis blocks, and multiple summary blocks. TrackssummaryDepthto know when inside<summary>, strips<analysis>blocks outside summary, preserves everything inside summarystripVisibleTags: Regex cleanup that removes residual<analysis>blocks (including nested inside summary), strips<summary>tag markers, and collapses whitespace- Trim: Final whitespace cleanup
The two-pass design is correct: the state machine handles the complex nesting semantics, the regex pass cleans up residual markers and whitespace.
Round 2 — Robustness
Traced the state machine through all critical edge cases:
- Unclosed
<analysis>: State machine finds<summary>in remaining text and consumes only up to<summary>start — does NOT consume all content (test:'<analysis>scratch\n<summary>visible</summary>' → 'visible') - Analysis inside summary: State machine preserves everything inside
<summary>(including nested<analysis>). Second pass strips the nested analysis — correct (test:'<summary>visible <analysis>hidden</analysis></summary>' → 'visible') - Multiple analysis blocks: Each stripped independently (test:
'a<analysis>one</analysis>b<analysis>two</analysis>' → 'ab') - Multiple summary blocks: Each handled independently (test:
'<summary>part1</summary> middle <summary>part2</summary>' → 'part1 middle part2') - Summary mentioning
<analysis>as literal text: State machine skips analysis inside summary depth, second pass strips it — preserves the summary content (test verified in parametrized cases) - Stray
</analysis>closing tag: Regex lazy match doesn't backtrack, passes through as literal text (test:'literal </analysis> marker' → 'literal </analysis> marker') - All-analysis output (no summary): Returns empty string after sanitization, agent.ts handles with
'(subagent produced no model-visible output)'fallback
No ReDoS risk: state machine is O(n), regex patterns use [^>]* (no .*), and [\s\S]*? is bounded by <\/analysis>.
Round 4 — Integration Points
Verified toModelVisibleSubagentResult applied at all 5 call sites:
agent.ts:1645— standard agent pathagent.ts:2585— worktree-isolated agent pathagent.ts:3016— worktree agent second sitebackground-agent-resume.ts:939— background resume pathforkedAgent.ts:535— forked agent pathworkflow-orchestrator.ts:470,837— workflow dispatch (both fast and override paths)
Round 5 — Test Coverage
Comprehensive across unit + integration:
subagent-result.test.ts: 15 parametrized cases + non-GOAL preservation testagent.test.ts: 3 new tests (strip tags, preserve diagnostics on ERROR, empty-result fallback)background-agent-resume.test.ts: 1 new test (resume path sanitization)forkedAgent.agent.test.ts: 1 new test (forked agent sanitization)workflow-orchestrator.test.ts: 2 new tests (fast-path + override-path sanitization)
Cross-Validation
| Finding | Reviewer | My Assessment |
|---|---|---|
C1: Unclosed <analysis> consumes all content |
qwen-code-ci-bot | Stale — HEAD uses state machine, consumes only to <summary> start |
| C2: ReDoS in summary regex | qwen-code-ci-bot | Stale — HEAD uses state machine + safe regex patterns |
| C3: Tag strip collapses adjacent words | qwen-code-ci-bot | Stale — HEAD replaces tags with space, then collapses whitespace |
| C4: Envelope unwrap mishandles multiple summaries | qwen-code-ci-bot | Stale — HEAD uses depth-tracking state machine |
| C5: Sanitizer runs unconditionally on all modes | qwen-code-ci-bot | Stale — HEAD has terminateMode !== GOAL guard |
| C6: Analysis-stripping regex destroys summary content | qwen-code-ci-bot | Stale — HEAD state machine preserves content inside summary |
| S1: Missing workflow-orchestrator call sites | qwen-code-ci-bot | Stale — both sites wired |
| S2: No dedicated unit test file | qwen-code-ci-bot | Stale — subagent-result.test.ts with 15+ cases |
S3: Missing happy-path <summary> test |
qwen-code-ci-bot | Stale — covered in parametrized tests |
| S4: No workflow-orchestrator integration test | qwen-code-ci-bot | Stale — 2 new tests added |
| S5: No empty-string fallback | qwen-code-ci-bot | Stale — '(subagent produced no model-visible output)' at agent.ts:3049 |
| S6: No forkedAgent test | qwen-code-ci-bot | Stale — forkedAgent.agent.test.ts test added |
Additional Audit Coverage
- State machine correctness: traced all 15 parametrized test cases through both passes manually, verified each produces the expected output
- ReDoS safety: verified no
.*or unbounded quantifiers in any regex, state machine is strictly O(n) with single forward pass stripVisibleTagsresidual analysis regex: uses[\s\S]*?bounded by<\/analysis>— safe against backtracking, catches analysis blocks nested inside summary that the state machine intentionally preserves- Tag attribute handling:
\b[^>]*pattern correctly matches tags with attributes (spaces, quotes, multiple attributes) - Whitespace normalization:
replace(/ {2,}/g, ' ')collapses multi-space to single — appropriate for model-visible text
This review was generated by QoderWork AI
qwen-code-ci-bot
requested changes
Jun 30, 2026
doudouOUC
reviewed
Jun 30, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Review Summary
Overall this is a well-designed and well-tested change. The two-step sanitization approach (state-machine pass + regex cleanup) is sound, and the call-site coverage is thorough — all parent-facing paths are wrapped while hook/telemetry/gate paths correctly retain raw text.
Verdict: Approve once CI is green. A few inline observations below (none blocking).
Collaborator
E2E Tmux Test Report -- PR #6027Tested on: Linux (Ubuntu x86_64, server) | Branch: Test Results
Total: 285 tests passed, 0 failed. DetailsTest 1: subagent-result.test.ts (16/16 passed)Test 2: agent.test.ts (115/115 passed)Test 3: background-agent-resume.test.ts (29/29 passed)Test 4: workflow-orchestrator.test.ts (117/117 passed)Test 5: forkedAgent.agent.test.ts (8/8 passed)Test 6: Typecheck (passed)No errors emitted; clean exit. VerdictPASS -- All 285 tests across 5 test files pass. TypeScript typecheck is clean. The subagent result sanitization logic, agent tool integration, background agent resume, workflow orchestrator, and forked agent utilities all behave correctly. 中文测试报告E2E Tmux 测试报告 -- PR #6027测试环境： Linux (Ubuntu x86_64, 服务器) | 分支： 测试结果
总计：285 个测试全部通过，0 个失败。 结论通过 -- 所有 5 个测试文件中的 285 个测试均通过。TypeScript 类型检查无误。子代理结果清理逻辑、Agent 工具集成、后台代理恢复、工作流编排器及分叉代理工具均表现正常。 |
qwen-code-ci-bot
requested changes
Jun 30, 2026
chiga0
previously approved these changes
Jun 30, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Re-Review at HEAD d4dd28a3
HEAD advanced from 54f5f7d9 (my prior APPROVE) with two new commits:
2482271— fix(core): address remaining subagent result feedbackd4dd28a— Merge branch 'main' into codex/subagent-result-tags
New Changes Verified
The new commit adds:
- Self-closing
<analysis/>handling — New regex/<analysis\b[^>]*\/>/giinstripVisibleTagscorrectly strips self-closing analysis tags that could leak inside summary blocks. ✓ - Self-closing analysis test case —
['<summary>Done<analysis/>leaked</summary>', 'Done leaked']correctly validates the fix. ✓ - Background registry sanitization test — "stores sanitized background results in the registry" verifies the sanitization is applied at the
registry.completecall site. ✓
Cross-Validation of All 21 Existing Inline Findings
| # | Finding | Reviewer | Status at d4dd28a |
|---|---|---|---|
| 1 | Unclosed <analysis> consumes to end-of-string |
qwen-code-ci-bot | ✓ Stale — state machine handles unclosed blocks |
| 2 | ReDoS quadratic backtracking | qwen-code-ci-bot | ✓ Stale — anchored lazy match, no nested quantifiers |
| 3 | Missed workflow-orchestrator call sites | qwen-code-ci-bot | ✓ Stale — addressed in prior commit |
| 4 | No dedicated unit test file | qwen-code-ci-bot | ✓ Stale — subagent-result.test.ts with 15+ parametrized cases |
| 5 | Fallback tag-strip collapses adjacent words | qwen-code-ci-bot | ✓ Stale — uses space replacement ' ' |
| 6 | Multiple summary blocks leak stray tags | qwen-code-ci-bot | ✓ Stale — state machine with depth tracking |
| 7 | Sanitizer runs unconditionally on all termination modes | qwen-code-ci-bot | ✓ Stale — guarded by terminateMode !== GOAL |
| 8 | Missing test for single summary block | qwen-code-ci-bot | ✓ Stale — '<summary>just summary</summary>' case exists |
| 9 | Analysis-stripping regex destroys summary content | qwen-code-ci-bot | ✓ Stale — state machine preserves analysis inside summary |
| 10 | No integration test in workflow-orchestrator | qwen-code-ci-bot | ✓ Stale — tests added in prior commit |
| 11 | No empty-string fallback on success path | qwen-code-ci-bot | Author deferral — foreground already adds placeholder |
| 12 | No forkedAgent sanitization test | qwen-code-ci-bot | ✓ Stale — test added in prior commit |
| 13 | Empty-output fallback at only 1 of 6 call sites | qwen-code-ci-bot | Author deferral — different callers have different contracts |
| 14 | Self-closing <analysis/> bypasses sanitizer |
qwen-code-ci-bot | ✓ Fixed in 2482271 — new regex added |
| 15 | No test for background agent sanitized registry | qwen-code-ci-bot | ✓ Fixed in 2482271 — new test added |
| 16 | Nested <analysis> bypasses sanitizer |
qwen-code-ci-bot | ✓ Stale — state machine tracks summaryDepth |
| 17 | Empty-result fallback missing on background agent path | qwen-code-ci-bot | Author deferral — same as #13 |
| 18 | Lazy match vs nested close tags | doudouOUC | Author deferral — known limitation |
| 19 | Whitespace normalization | doudouOUC | Author deferral — only normalizes tag-introduced spacing |
| 20 | Inconsistent empty-result fallback | doudouOUC | Author deferral — same as #13 |
| 21 | Additional edge-case test | doudouOUC | Author deferral — literal analysis in code-fenced text |
Additional Audit Coverage
- Self-closing regex correctness: Verified
<analysis/>is matched by/<analysis\b[^>]*\/>/giand replaced with space. The\bprevents false matches on<analysisfoo/>. ✓ - Test case validation: Traced
'<summary>Done<analysis/>leaked</summary>'through the state machine (preserves all content inside summary) →stripVisibleTags(strips<analysis/>and<summary>tags) →'Done leaked'. ✓ - Background registry test: Verified the new test mocks
getFinalTextto return tagged content and assertsregistry.completereceives the sanitized text. ✓
Final Verdict
APPROVE — The new commit correctly addresses the self-closing analysis tag bypass and adds background registry sanitization coverage. All 21 existing findings verified: 16 stale, 2 fixed in the new commit, 5 author-deferred with reasonable justifications. No blocking issues.
This review was generated by QoderWork AI
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jun 30, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
disabled auto-merge
June 30, 2026 11:51
yiliang114
enabled auto-merge
June 30, 2026 11:51
doudouOUC
approved these changes
Jun 30, 2026
doudouOUC
left a comment
doudouOUC
left a comment
Collaborator
There was a problem hiding this comment.
Updated Review (v2)
The latest revision addresses all observations from the first review round:
- Nested
<analysis>handling —findAnalysisCloseEndnow uses proper depth tracking, eliminating the lazy*?regex truncation concern. - Self-closing
<analysis/>— handled explicitly viaisSelfClosingTag()in bothstripClosedAnalysisBlocksandstripAnalysisOutsideSummary. - Consistent empty-result fallback —
'(subagent produced no model-visible output)'is now applied uniformly across foreground, background, and resume paths. - Additional edge-case tests — nested analysis (
<analysis>outer<analysis>inner</analysis>LEAKED</analysis>) and self-closing (<analysis/>) are both covered.
The sanitization logic is now demonstrably more robust: depth-aware for nested blocks, handles self-closing tags, and has consistent fallback behavior across all execution paths.
CI status: Test (ubuntu-latest, Node 22.x) passed.
Approving — no blocking issues found.
| } | ||
|
|
||
| /** Keeps successful subagent scratchpad tags out of the parent model context. */ | ||
| export function toModelVisibleSubagentResult( |
Collaborator
There was a problem hiding this comment.
[Nit, non-blocking] Double sanitization in foreground path
In the foreground execution path, toModelVisibleSubagentResult is called twice on the same subagent.getFinalText() result:
- Once inside
runSubagentWithHooks(line ~1648 of agent.ts) for the display/registryresultfield - Once in the outer
tryblock (line ~3025 of agent.ts) for thellmContenttool return
Since this is a pure function with no side effects, it produces the same result both times — so this is purely a minor redundancy, not a bug. Could be deduplicated by having runSubagentWithHooks return the sanitized text alongside stopHookWarning, but that's a larger refactor not worth doing in this PR.
| continue; | ||
| } | ||
|
|
||
| result += text.slice(index, start) + ' '; |
Collaborator
There was a problem hiding this comment.
[Nit, non-blocking] Whitespace after multi-line analysis removal
The space replacement for removed analysis blocks (result += text.slice(index, start) + ' ') is good for inline removals, but when a multi-line <analysis> block sits on its own lines:
text before\n\n<analysis>\nscratch\n</analysis>\n\ntext after
→ produces text before\n\n \n\ntext after (extra space + blank lines). The final trim() handles leading/trailing, but internal blank runs remain. Not a problem in practice since models typically emit <analysis> adjacent to <summary>, and the compressed output is still readable.
qwen-code-ci-bot
requested changes
Jun 30, 2026
| function findAnalysisCloseEnd(text: string, from: number): number { | ||
| const tagPattern = /<\/?analysis\b[^>]*>/gi; | ||
| let depth = 1; | ||
| tagPattern.lastIndex = from; |
Collaborator
There was a problem hiding this comment.
[Critical] Quoted-attribute bypass in tag-matching regex
All tag-matching regexes use [^>]* to consume attributes, which stops at the first > regardless of whether it's inside a quoted attribute value. A crafted tag like <analysis note=">"> causes the regex to misparse the tag boundary: <analysis note="> is matched as the opening tag, and findAnalysisCloseEnd pairs it with the first </analysis>, leaving subsequent content to leak through.
Confirmed at runtime:
- Input:
<analysis note=">"></analysis>INJECTION</analysis> - Output:
INJECTION</analysis>(injection content survives)
Impact: A malicious or hallucinating subagent can inject arbitrary text into the parent model's context, defeating the sanitizer's stated purpose.
Suggested change
| tagPattern.lastIndex = from; | |
| function findAnalysisCloseEnd(text: string, from: number): number { | |
| const tagPattern = /<\/?analysis\b(?:[^>"']|"[^"]*"|'[^']*')*>/gi; |
Apply the same fix to the regex patterns in stripClosedAnalysisBlocks (line 49) and stripAnalysisOutsideSummary (line 93).
— qwen3.7-max via Qwen Code /review
| continue; | ||
| } | ||
|
|
||
| result += text.slice(index, start) + ' '; |
Collaborator
There was a problem hiding this comment.
[Critical] Unclosed <analysis> inside <summary> leaks content to parent model
When a subagent produces <analysis> opened but never closed inside a <summary> block (common when output is truncated by token limits), Pass 1 (stripAnalysisOutsideSummary) preserves it verbatim because inside-summary content is passed through. Pass 2 (stripClosedAnalysisBlocks) then hits closeEnd === -1 and executes result += text.slice(index, end), preserving the <analysis> tag plus all subsequent content.
Confirmed at runtime:
- Input:
<summary>Task done<analysis>INJECTION</summary> - Output:
Task done<analysis>INJECTION(tag and injection survive)
Impact: Truncated subagent output leaks internal analysis content into the parent model context.
Suggested change
| result += text.slice(index, start) + ' '; | |
| const closeEnd = findAnalysisCloseEnd(text, end); | |
| if (closeEnd === -1) { | |
| result += text.slice(index, start) + ' '; | |
| index = end; | |
| continue; | |
| } |
— qwen3.7-max via Qwen Code /review
This was referenced Jul 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
This PR sanitizes subagent final results before they are fed back into the parent agent context. Raw subagent transcripts still preserve the model output, but the parent receives only the model-visible result: internal-looking
<analysis>blocks are removed, and<summary>tags are stripped while keeping their contents visible.It applies the same boundary to foreground agents, background agents, resumed background agents, and worktree-isolated agent runs, with focused regression coverage for the normal agent tool path and the resume path.
Why it's needed
A subagent can sometimes return internal-looking tags as ordinary assistant text. If Qwen Code forwards that raw final text into the parent context, the parent model and daemon UI can both see those implementation markers, making the response look like leaked analysis or malformed markdown. The correct boundary is to keep raw output in the child transcript/debug trail while returning only the model-visible summary to the parent.
Reviewer Test Plan
How to verify
Run the targeted core tests and inspect that the parent-facing result contains only the summary text when a subagent returns
<analysis>...<summary>.... Manual tmux verification results are posted in a PR comment.Evidence (Before & After)
Before: the logged failing session showed a subagent final answer with
<analysis>and<summary>tags visible in the parent/daemon UI path. After: the targeted core tests pass, and manual tmux verification recorded no raw tag leakage in parent follow-up requests.Local checks:
npx vitest run src/agents/subagent-result.test.ts src/tools/agent/agent.test.ts src/agents/background-agent-resume.test.ts src/agents/runtime/workflow-orchestrator.test.tspassed with 275 tests;git diff --checkpassed.npm run typecheckwas also attempted, but the full workspace check failed before validating this change because of existing unrelated errors inpackages/cliaroundfindExistingProviderModelsandstring | string[]handling, then a later workspace exited with a Node heap OOM.Tested on
Environment (optional)
macOS local checkout, Node.js 22.22.0. Targeted tests were run from a clean PR worktree using the local dependency install.
Risk & Scope
<analysis>as internal-only when it appears in subagent final text, so a subagent that truly wants to return a literal analysis XML-like block to the parent would have that block removed.Linked Issues
Fixes #6023
中文说明
这个 PR 做了什么
这个 PR 在 subagent 最终结果回传给父 agent context 之前做清理。子 agent 的原始 transcript 仍然保留模型原始输出，但父 agent 只会收到模型可见结果：移除内部样式的
<analysis>块，并剥离<summary>标签但保留其中的可见内容。它同时覆盖了前台 agent、后台 agent、恢复后台 agent、以及 worktree 隔离 agent 的返回路径，并为普通 agent tool 路径和 resume 路径补了聚焦的回归测试。
为什么需要
subagent 有时会把内部样式标签作为普通 assistant 文本返回。如果 Qwen Code 把这段原始最终文本直接放进父 context，父模型和 daemon UI 都可能看到这些实现标记，表现为 analysis 泄漏或 markdown/html 渲染异常。正确边界是：原始输出保留在 child transcript/debug 里，父 agent 只收到模型可见 summary。
Reviewer Test Plan
如何验证
运行目标 core 测试，确认当 subagent 返回
<analysis>...<summary>...时，父侧结果只包含 summary 文本。手动 tmux 验证结果已作为 PR 评论发布。证据（Before & After）
Before：失败 session 日志显示 subagent 最终答案中的
<analysis>和<summary>标签进入了 parent/daemon UI 路径。After：目标 core 测试通过，手动 tmux 验证记录到父 follow-up 请求中没有原始标签泄漏。本地检查：
npx vitest run src/agents/subagent-result.test.ts src/tools/agent/agent.test.ts src/agents/background-agent-resume.test.ts src/agents/runtime/workflow-orchestrator.test.ts通过，共 275 个测试；git diff --check通过。也尝试了
npm run typecheck，但全 workspace 检查在验证本次改动前就因为packages/cli中已有的findExistingProviderModels导出问题和string | string[]类型问题失败，随后另一个 workspace 又因 Node heap OOM 退出。Tested on
Environment (optional)
macOS 本地 checkout，Node.js 22.22.0。目标测试在 clean PR worktree 中复用本地依赖安装运行。
风险和范围
<analysis>当成内部内容处理，所以如果 subagent 真的想把这种 XML-like analysis block 返回给父 agent，这个块会被移除。Linked Issues
Fixes #6023