fix: stripEnvelopeMetadata multiline wrapper regression#531
fix: stripEnvelopeMetadata multiline wrapper regression#531jlin53882 wants to merge 2 commits intoCortexReach:masterfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
b5cac64 to
4c2b801
Compare
AliceLJY
left a comment
There was a problem hiding this comment.
Addresses rwmjhb's feedback from #510 — "Must Fix #1" (standalone boilerplate line) and "Nice to Have #2" (multiline wrapper with You are running as a subagent... on separate line).
"wrapper zone" 状态机逻辑合理:Step 1 整行移除 [Subagent Context/Task] prefix,Step 2 在 wrapper zone 内继续清理 boilerplate continuation lines,遇到真实内容就退出 zone。
关键:false-positive 保护测试到位 — 如果用户消息本身包含 boilerplate 短语(如 "Do not use any memory tools."),只要不在 wrapper zone 内就不会被误删。
LGTM ✅
|
Hi @AliceLJY — a new commit (703d5b2) was pushed after your review with an important fix: Root cause: The \hadWrapperPrefix\ array was computed AFTER Step 1 stripped wrapper lines (turning them into empty strings), so the state machine never detected wrapper zones. Fix: Compute \hadWrapperPrefix\ from the original input text before Step 1 runs. All 17 unit tests + 9 bug-scenario tests pass. Could you take another look when you have a moment? Thanks! |
Review SummaryAutomated multi-round review (7 rounds, Claude + Codex adversarial). The problem (subagent instruction fragments leaking into extracted memories) is real and worth solving. Must Fix
Nice to Have
Questions
Please address the must-fix items. The direction is good — once these are resolved, this is ready to merge. |
|
@rwmjhb Thanks for the detailed review. New commit Must Fix #1: Strip condition invertedThe Must Fix #2: Case A not implementedCommit Must Fix #3: Global regex scope wideningRoot cause: old Step 1 used global regex matching wrapper prefixes anywhere in the message. Fix: single-pass state machine that only strips in the leading zone. Once Must Fix #4: Build failureCI environment issue (EADDRINUSE on port 11434), not a code issue. Must Fix #5: Test infrastructureSeparate CI infrastructure issue, unrelated to stripEnvelopeMetadata. All 17 unit tests pass. Could you re-review when you have a moment? |
rwmjhb
left a comment
There was a problem hiding this comment.
Review
Thanks for tackling this — the multiline wrapper regression is a real problem worth fixing.
However, the current implementation has fundamental issues that would need a significant rework:
- Guard logic is inverted:
!isFollowedByUserContent(i)means boilerplate is never stripped when followed by user content (the main use case). The existing test "strips multiline wrapper continuation text but preserves following conversation" would regress. - Case A is described but not implemented: No code handles
"You are running as a subagent..."on its own line — the Step 1 regex only catches[Subagent Context|Task]lines. - Build and tests: TypeScript compilation failed in our run, and the test suite crashed before the 3 new tests could execute.
Suggest rebasing onto main and considering reusing the existing stripLeadingRuntimeWrappers (which already handles the subagent phrase) with corrected logic, rather than inlining a replacement.
Adversarial review fixes pushedAfter conducting a full adversarial review (claw-self-review + Claude Code + opencode), we identified and fixed 3 MAJOR bugs in commit MAJOR #1 — Guard decoupled from state machine
MAJOR #2 — Missing
|
Regarding
|
Follow-up review on
|
| # | Finding | Author response | Status |
|---|---|---|---|
| F2 | Guard logic inverted (isFollowedByUserContent) |
Fixed in 0194402 — now strips boilerplate within wrapper zone |
✅ Claimed resolved |
| F3 | Case A not implemented | prevWasWrapper tracking added in 81a6205 |
✅ Claimed resolved |
| MR1 | Global regex scope widening | Single-pass state machine with stillInLeadingZone in 9b93534 |
✅ Claimed resolved |
| EF1 | Build failure | Author attributes to environment (EADDRINUSE) | |
| EF2/EF3 | Test suite crash (port 11434) | Pre-existing infra defect, unrelated to this PR |
Remaining asks
-
Rebase onto main —
stale_base=true. There have been commits on main since this branch diverged; please rebase to confirm no conflicts and that the state machine still works against the current codebase. -
Confirm test results post-rebase — The 20/20 claim is from the author's local run. After rebase, please paste the full test output (at minimum the
strip-envelope-metadatatests) so we can close the loop on EF1-EF3. -
Dead code cleanup (non-blocking) —
RUNTIME_WRAPPER_LINE_RE,RUNTIME_WRAPPER_PREFIX_RE,RUNTIME_WRAPPER_BOILERPLATE_RE,stripRuntimeWrapperBoilerplate, andstripLeadingRuntimeWrappersappear to be unreachable after this refactor. If they're still composed in the pipeline as you described, a brief comment clarifying the call chain would help future maintainers.
Verdict
The direction is solid and the author has been responsive. Pending rebase + green test confirmation, this should be good to merge.
9b93534 to
93f8e2e
Compare
Rebase onto main + test resultsBranch Post-rebase test outputAll 20 tests pass on latest main. ✅ EF1-EF3 (build/test infrastructure concerns) confirmed resolved — this is a clean run on the latest codebase. |
Dead code cleanup — resolvedAfter adversarial review (opencode + claude), confirmed the following were dead code:
Confirmed: Removed: 55 lines from Commit: |
|
@rwmjhb All three asks have been addressed:
Could you take another look when you have a moment? The branch is ready for merge. |
Review:
|
All issues addressed — please re-reviewWe have gone through your review point by point. Here is the complete resolution: Must Fix 1 —
|
Re-review after author fixes (commit
|
All issues addressed (commit
|
Re-review on
|
| Item | Status |
|---|---|
N2: isWrapper guard on stillInLeadingZone |
✅ Fixed |
N4: SUBAGENT_RUNNING_RE anchored with ^ |
✅ Fixed |
N3: hasLeadingWrapper pre-scan scope |
Retained — see Must Fix 2 |
68717b7 to
4e3af31
Compare
|
Hi @rwmjhb — thanks for the feedback on Fix 1. Regarding the On the latest HEAD (commit
The |
|
Additionally, here is the full test output from the latest HEAD ( 20/20 pass ✅ |
Fix 2 (hasLeadingWrapper pre-scan scope) — FixedCommit Before (buggy): const hasLeadingWrapper = originalLines.some((rawLine) =>
WRAPPER_LINE_RE.test(rawLine.trim())
);
// Scanned ALL lines — a wrapper appearing in trailing zone caused false-positive strippingAfter (fixed): let foundLeadingWrapper = false;
for (let i = 0; i < originalLines.length; i++) {
const trimmed = originalLines[i].trim();
if (trimmed === "") continue;
if (WRAPPER_LINE_RE.test(trimmed)) { foundLeadingWrapper = true; continue; }
if (BOILERPLATE_RE.test(trimmed)) continue;
break; // first real user content — stop scanning
}
const hasLeadingWrapper = foundLeadingWrapper;Full test output (20/20 pass): Must Fix 1 (Build verification) — AddressedConfirmed: this repo uses |
修復內容Claim 1 (MAJOR): Boilerplate 在 wrapper 之前被誤刪問題: 範例: 根因:pre-scan 用 修復:
Claim 2 (MINOR): Wrapper 行 inline 內容全被丟棄問題:新實作把 wrapper 行整行 push 空字串,沒有保留 inline payload。 範例: 根因:
修復:
新增測試
測試結果本地測試:✅ 23/23 pass |
rwmjhb
left a comment
There was a problem hiding this comment.
感谢这个 PR,subagent 指令片段污染 memory store 是真实问题,方向正确。
必须修复(2 项)
MR1:空行会提前终止 leading wrapper zone,后续 boilerplate 泄漏
当前实现遇到空行就把 stillInLeadingZone 置为 false。但 subagent wrapper 里经常有空行分隔各段(例如 [Subagent Context] 块和 [Instructions] 块之间),空行出现后的 boilerplate 就不会被 strip。建议只在遇到明确的非 wrapper 内容行时才结束 leading zone,而不是空行。
EF1:TypeScript build 失败
验证器报告 BUILD_FAILURE。如果这是 main 上的预存问题,请 rebase 后重新确认;如果是本 PR 引入的,需要修复后再合并。
建议修复(不阻塞合并)
- F2:
isWrapper检查是全局的,没有限制在 leading zone 内——leading zone 结束后的 wrapper 行也会被 strip,可能误删用户引用的文本 - F3:
hasLeadingWrapper预扫描所有行,如果用户消息里含有[Subagent Context]作为引用/粘贴文本,会误判为有 leading wrapper 并触发 boilerplate stripping - MR2:continuation 匹配是 substring-based,用户粘贴的包含 wrapper 关键词的文本可能被整行删除
一个问题
hasLeadingWrapper 预扫描所有行——如果用户消息里含有 [Subagent Context] 作为引用/粘贴文本,会误判为有 leading wrapper 并触发 boilerplate stripping。在实际使用场景中这种情况是否需要考虑?
Re-review on
|
Code Review 完成 (commit
|
| 測試 | 說明 |
|---|---|
preserves inline wrapper payload that only mentions boilerplate later in the sentence |
MR2 驗證 |
strips leading inline boilerplate but preserves payload that follows it |
MR2 邊界 |
strips multiple leading boilerplate phrases before preserving inline payload |
MR2 composite |
strips standalone JSON blocks when sender_id appears before message_id |
Hidden bug 驗證 |
測試結果
✅ 27/27 tests pass
總結
- MR2: 現在正確區分「wrapper boilerplate」vs「使用者引用 boilerplate」
- Hidden bug: JSON stripper 不再受 key 順序影響
- 測試覆蓋率提升至 27 cases
9e1a685 to
e01e824
Compare
- FIX 1 (MAJOR): encounteredWrapperYet flag replaces global hasLeadingWrapper Boilerplate is now only stripped when a wrapper has ALREADY appeared on a previous line, not just because a wrapper exists somewhere in the leading zone. This fixes the false positive where boilerplate text like 'Do not use any memory tools.' appearing BEFORE a wrapper in the input would incorrectly be deleted. - FIX 2 (MINOR): preserve inline content after wrapper prefix Old implementation stripped only the wrapper prefix, preserving inline payload. New implementation initially dropped the entire line (regression). This fix restores inline content preservation by: 1. Stripping wrapper prefix 2. Removing boilerplate phrases from remainder using INLINE_BOILERPLATE_RE 3. Keeping remainder if non-empty (non-boilerplate content preserved) - Add 3 new test cases covering both fixes Closes: (PR 531 review fixes)
…er bug - MR2: INLINE_BOILERPLATE_RE now anchored with ^ to avoid stripping quoted boilerplate later in payload - Hidden bug: JSON stripper regex only worked when message_id came before sender_id; now uses order-independent lookahead - Added 4 new test cases covering the fixes
e01e824 to
c4bca34
Compare
Summary
Fixes the multiline wrapper regression identified by rwmjhb in PR #510 review, and resolves the original subagent instruction fragment bug.
Changes
Step 1 — Strip entire wrapper lines (prefix + all inline content)
Uses
.*so that subagent instruction fragments likeYou are running as a subagent (depth 1/1).are not left behind.Step 2 — Strip multiline continuation lines
Case A — "You are running as a subagent..." on separate line
If a line matches the subagent phrase AND the previous line is a wrapper prefix, strip it. (rwmjhb Nice to Have #2)
Case B — Known boilerplate phrases (only when not followed by user content)
Strips
Results auto-announce...,do not busy-poll...,Reply with a brief acknowledgment only.,Do not use any memory tools.only when NOT followed by user content. (rwmjhb Must Fix #1)Bug fixed
You are running as a subagent (depth 1/1).left behindReply with a brief acknowledgment only.on its own line not strippedYou are running as a subagent...on separate line not strippedTest coverage
14 original tests + 3 new tests covering Must Fix #1, Nice to Have #2, and do-not-false-positive guard.
Related