fix(adapters): classify failed exec wrappers by line scan, not backtracking - #2938
fix(adapters): classify failed exec wrappers by line scan, not backtracking#2938luvs01 wants to merge 1 commit into
Conversation
…acking FAILED_EXEC_OUTPUT_REGEX chained `[^\n]*`, `\n*` and `\s*` groups that can all match the same whitespace. An input that starts with the literal prefix but never completes the match makes the engine try every split between them, so cost grows quadratically with the padding length. Measured on Bun 1.4, a single `Script failed` line padded with 30k spaces took ~820ms and 60k took ~3.1s. The text comes from a tool result, so its shape is not fully under our control. Replace the pattern with a line scan that classifies each line exactly once. The same 60k input costs ~0.02ms. The rewrite also closes a latent bug it made visible: the old `\n*` accepted an LF blank line but not a CRLF one, so a Windows wrapper such as `Script failed\r\n\r\nOutput:` fell through to the empty-SUCCESS guidance and erased the only signal that the cell had failed. CRLF blank lines are now separators, as they always should have been. Every other accepted and rejected shape is unchanged; a differential run over 200k generated wrappers found no behavior difference outside the CRLF case.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
✅ Deterministic PR hygiene checks passed. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request is already Ready for Review. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe change replaces regex-based failed empty-output detection with a shared line-scanning helper. Cursor normalization uses the helper. Tests cover long malformed wrappers, CRLF separators, and previously accepted wrapper shapes. ChangesExec wrapper normalization
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR replaces vulnerable backtracking classification with a linear line scan and correctly handles failed wrappers using CRLF separators; no actionable merge-blocking risk remains after normal checks and review. Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
리뷰 · 우선순위 75 / 80이 PR은 지금 쉽게 말하면 이렇다. 코드 모드 exec 칸이 실패하고 출력이 비면, 도구 결과는 고치는 방법은 그 CRLF 한 가지가 이 PR의 두 번째 이유다. 예전 패턴은 LF 빈 줄은 먹고 캐리지 리턴은 못 먹는다. 그래서 호출 쪽도 같이 바뀐다. Cursor 쪽 테스트는 세 갈래다. 6만 공백 잘못된 감싸기가 500ms 안에 분류되고 통과로 남는 시간 회귀, CRLF 가 실패 안내를 남기고 빈 성공 문구를 안 남기는 회귀, 예전 패턴이 받던 모양과 거절하던 모양을 하나씩 잠근 동등성이다. 소스를 되돌리면 시간 회귀와 CRLF 회귀가 빨개진다고 했다. 로컬에서 라인 - 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
Ingwannu
left a comment
Ingwannu
left a comment
There was a problem hiding this comment.
Reviewed exact head 00a30c3 against dev@d882caed5eb212bf5737d3cb0022dace2dab418e.
The linear-time direction is justified: on the exact old pattern, the PR's 60,000-space malformed input took 5,385.6 ms on this host while isFailedEmptyExecWrapper took 0.083 ms. The focused Cursor/Kiro normalization set passes 102/102 under isolated HOME, OPENCODEX_HOME, and CODEX_HOME. I am requesting changes because the claimed old-shape equivalence is not yet true.
In src/adapters/exec-tool-result-normalize.ts, the Output branch both rejects a previously accepted marker and accepts a previously rejected duplicate marker:
- "Script failed\nOutput:\n ": old=true, new=false. The old Output:\s* arm accepts indentation before the optional marker; the line scan requires the later line to equal "" byte-for-byte.
- "Script failed\nOutput: \n": old=false, new=true. Once the inline marker has already been consumed, the unconditional later "" branch accepts a second marker that the old pattern treated as payload/malformed content.
The same results hold with two leading spaces and with a blank line before the duplicate marker. CRLF is the one intended false->true change and remains correct.
Please track whether the Output arm already consumed the marker, accept a later marker with surrounding horizontal whitespace only when the Output arm was bare, and do not consume a second marker. Add both directions to the equivalence regression. This keeps the performance/CRLF fix while preventing the classifier from silently changing which malformed or indented wrappers are erased.
The local typecheck currently reports only the three existing fetch(..., { timeout }) errors in claude-messages.ts and responses/fetch-helpers.ts; this diff does not touch those files. No repository-wide security scan was run.
|
I ran an independent differential check on your exact head Four of them are your intended CRLF correction and are improvements: The other six are the behavioural divergence, and they flip in opposite directions — which is why a single "is it stricter or looser" framing does not catch them: Both directions matter, and the second group is the one I would fix first:
The relevant code is the On the rest: the linear-time approach is right and the ReDoS concern is real, so I want this to land. Two things would get it there.
Also worth rebasing: you are 2 commits behind No changes pushed to your branch — the corpus is yours to use. |
|
I opened #2945 with your approach and the six divergences fixed, so the ReDoS gets closed either way. Your diagnosis and the linear-scan design are the substance of it and the PR says so. To be explicit about the options, because I do not want to have talked you out of your own patch:
Your call. If I hear nothing I will let #2945 go through, but I would rather merge yours. One measurement you may want for your own notes: the old regex takes 1224 ms on The CRLF half of your patch was a real find that I would not have looked for. A Windows-produced failed wrapper was falling through to the empty-success message, so the model was being told nothing had gone wrong when the cell had failed. That is carried over intact. |
…ktracking The previous classifier placed two adjacent unbounded whitespace runs over the same span, so a long whitespace run followed by one non-matching character forced the engine to retry prefixes. Replaced with a single forward scan. Every loop advances an index monotonically, the newline searches cover disjoint forward spans, and the token checks are fixed-length, so no path retries a prefix. Two boundaries are load-bearing and both were divergences in an earlier attempt at this rewrite: whitespace after Output: may precede the marker, so an indented <empty> still classifies; and only whitespace may follow it, so a duplicate <empty> still does not. That second one matters most -- classifying it would replace a real payload with the failed-wrapper guidance, turning a normalization into data loss. One intended behaviour change: CRLF blank separators now count. The old pattern matched only \n, so a Windows-produced failed wrapper never classified and fell through to the empty-SUCCESS message, telling the model nothing had gone wrong when the cell had failed. Differential comparison over 63 shapes locally and an independent 662-shape pairwise corpus found no disagreement outside that CRLF blank-separator class. Diagnosis and the linear-scan approach are @luvs01's from #2938; that PR could not land as written because of the six divergences, which I posted there with the exact inputs. The bounded-work test measures process.cpuUsage() rather than elapsed wall time: performance.now() counts OS descheduling, VM pauses and GC, so a loaded CI runner can blow a wall-clock budget while the code under test did nothing wrong. Reverting to the previous classifier spends 1230ms CPU against a 250ms bound. Four mutations proven red at the intended test each: whitespace restricted to CR/LF, accepting a second marker, removing CRLF handling, and reverting the classifier.
…ktracking (#2945) The previous classifier placed two adjacent unbounded whitespace runs over the same span, so a long whitespace run followed by one non-matching character forced the engine to retry prefixes. Replaced with a single forward scan. Every loop advances an index monotonically, the newline searches cover disjoint forward spans, and the token checks are fixed-length, so no path retries a prefix. Two boundaries are load-bearing and both were divergences in an earlier attempt at this rewrite: whitespace after Output: may precede the marker, so an indented <empty> still classifies; and only whitespace may follow it, so a duplicate <empty> still does not. That second one matters most -- classifying it would replace a real payload with the failed-wrapper guidance, turning a normalization into data loss. One intended behaviour change: CRLF blank separators now count. The old pattern matched only \n, so a Windows-produced failed wrapper never classified and fell through to the empty-SUCCESS message, telling the model nothing had gone wrong when the cell had failed. Differential comparison over 63 shapes locally and an independent 662-shape pairwise corpus found no disagreement outside that CRLF blank-separator class. Diagnosis and the linear-scan approach are @luvs01's from #2938; that PR could not land as written because of the six divergences, which I posted there with the exact inputs. The bounded-work test measures process.cpuUsage() rather than elapsed wall time: performance.now() counts OS descheduling, VM pauses and GC, so a loaded CI runner can blow a wall-clock budget while the code under test did nothing wrong. Reverting to the previous classifier spends 1230ms CPU against a 250ms bound. Four mutations proven red at the intended test each: whitespace restricted to CR/LF, accepting a second marker, removing CRLF handling, and reverting the classifier.
|
The ReDoS is fixed on Closing this as superseded rather than rejected. What differs from your branch is two boundary conditions, nothing about the strategy:
The second is the one I would flag for any future rewrite in this area: accepting it means an exec payload gets replaced by the failed-wrapper guidance, so a normalization turns into data loss. That is a worse outcome than the ReDoS, which is why the six divergences blocked an otherwise correct patch. Your CRLF fix carried over intact and was a genuinely good catch that I would not have gone looking for — a Windows-produced failed wrapper was falling through to the empty-success message, so the model was told nothing had gone wrong when the cell had failed. Two things from the review that are worth having if you work on this kind of change again. A differential harness against the old implementation is what makes an equivalence claim checkable — an independently generated 662-shape corpus confirmed no disagreement outside the CRLF class, which is a much stronger statement than a passing test suite. And a timing test measured in wall-clock can pass for the wrong reason on a fast machine and fail for the wrong reason on a loaded one; the merged version measures If you would still rather have your own commit in the history, say so — I am happy to revert Please do keep sending these. |
3 participants
Summary
FAILED_EXEC_OUTPUT_REGEXwithisFailedEmptyExecWrapper, a line scan that classifies each line exactly once.The backtracking
The pattern chained
[^\n]*,\n*and\s*groups that can all match the same whitespace. An input beginning with the literal prefix but never completing the match forces the engine to try every split between those groups, so cost grows quadratically with the padding length.Measured on Bun
1.4.0+34cbb9a40, a singleScript failedline followed by padding:Doubling the input roughly quadrupled the time, which is the quadratic signature. This text arrives inside a tool result, so its shape is not fully under our control.
I first tried to keep a regex and only restructure the quantifiers. Every variant that preserved the accepted shapes still measured in the hundreds of milliseconds to several seconds, because the prefix scan and the trailing whitespace group inevitably overlap. The scan removes the class of problem rather than moving it.
The CRLF bug this exposes
While proving equivalence I found the old pattern accepted
Script failed\n\nOutput:but rejectedScript failed\r\n\r\nOutput:, because\n*cannot consume the\r. A CRLF wrapper therefore fell through to the empty-success guidance ("NOT lost context", "Do not re-run") and erased the only signal that the cell had failed — precisely the outcome the failure branch exists to prevent.CRLF blank lines are now separators. That is a deliberate behavior change and the only one in this PR.
Equivalence
Blank-separator semantics are preserved exactly: a line of spaces is not a separator, only a genuinely empty one is, matching what
\n*could consume. A differential run over 200,000 generated wrappers (varied fragments, LF and CRLF) found no behavior difference outside the CRLF case above.Verification
dev@d882caed5eb2.1.4.0+34cbb9a40,tests/cursor-exec-empty-result.test.ts:11passed,0failed.3320 msand the CRLF regression fails on classification.1.4.0+34cbb9a40, withcursor-toolresult-normalize,kiro-adapter, andtool-catalog-nudge:122passed,0failed (507expectations).bun run typecheck: passed.bun run privacy:scan: passed.Checklist
Review readiness checklist
Summary by CodeRabbit