fix(adapters): classify failed exec wrappers by forward scan, not backtracking

[lidge-jun]

Summary

Replaces the backtracking failed-wrapper classifier in src/adapters/exec-tool-result-normalize.ts with a forward scan. The diagnosis and the linear-scan approach are @luvs01's from #2938 — this lands the same idea with the behavioural divergences fixed. #2938 can close in favour of this, or they can push these two boundaries onto their branch and I will merge theirs instead.

Why #2938 could not land as written. Its scan diverges from the previous classifier on six inputs, in opposite directions, so "is it stricter or looser" does not surface them. Three previously-classifying shapes stopped classifying, and three previously-rejected ones started. The second group is the damaging one: classifying a duplicate-marker wrapper erases a real payload, replacing it with the failed-wrapper guidance. A normalization becoming data loss is worse than the defect it was fixing. Exact inputs are on #2938.

This implementation keeps both boundaries explicit. Whitespace after Output: may precede the marker, so an indented <empty> still classifies; only whitespace may follow it, so a duplicate <empty> still does not.

Linearity is structural, not asserted: every loop advances an index monotonically, the newline searches cover disjoint forward spans, every token check is fixed-length. No regex, no recursion, no path that retries a prefix.

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 in fact failed.

Verification

bun test tests/cursor-exec-empty-result.test.ts tests/cursor-tool-result-invocation.test.ts tests/kiro-adapter.test.ts → 111 pass / 0 fail / 467 expectations. bun x tsc --noEmit exit 0. bun run privacy:scan passed.

Equivalence was checked twice, independently. A 63-shape differential corpus of mine, and an independent 662-shape pairwise corpus generated by a reviewer covering marker variants, first-line text, Wall-time lookalikes such as Wall timeout, Output: variants, marker placement, LF/CRLF/bare-CR/mixed/U+2028 separators, and ASCII/NBSP/U+2028 outer whitespace. Neither found a disagreement outside the CRLF blank-separator class. Bare \r, U+2028, and trailing-whitespace-only inputs all agree.

Four mutations, each red at the intended test:

Mutation	Result
restrict whitespace skipping to CR/LF	indented-marker test red
accept a second <empty> marker	duplicate-marker test red
remove CRLF separator handling	CRLF test red
revert to the previous classifier	bounded-work test red at 1230 ms CPU against a 250 ms bound, and the CRLF test red

Changes since first push

Three review findings, all correct:

A tracked planning note under devlog/_plan/ has been removed — that was a policy violation on my part. AGENTS.md requires reproduction detail for an unshipped defect to stay in scratch space, and states plainly that it binds maintainers exactly as it binds contributors. I wrote the note anyway, in a public directory, before the fix had shipped. The notes are now in gitignored .tmp/. The reviewer's point that removing the tip does not undo the disclosure already made by this PR is correct; what it does prevent is the reproduction persisting on dev. The source comment that ships with the fix stays, since the diff itself reveals the weakness — that is the test AGENTS.md sets.

The timing assertion now measures CPU time. performance.now() counts OS descheduling, VM pauses and GC, so a loaded runner could fail it while the code under test did nothing wrong. process.cpuUsage() counts only work this process performed. The bound is a tripwire three orders of magnitude above the scan's real cost, not a performance target — wide enough that only a return to super-linear work crosses it. Reverting the classifier spends 1230 ms CPU against 250 ms, so there is ~5x separation without depending on wall-clock.

A stale comment claiming Cursor keeps its own regex was corrected; Cursor now calls the shared predicate.

On the removed export: repository-wide search found no remaining reference to the old regex constant, and it was never part of the package API — package.json exports only ., and src/index.ts never re-exported it. Keeping a vulnerable primitive alive for unsupported deep imports would defeat the fix.

Not verified: the CPU bound is a threshold, not a proof of asymptotic complexity; the linearity argument is structural. I also have no evidence of exploitation in practice — the wrapper text is produced by our own exec path, so reachability depends on a tool result a model can influence.

Checklist

• Focused regressions beside the existing tests for this subsystem, each mutation-proven
• bun x tsc --noEmit clean
• bun run privacy:scan green
• No credentials, tokens, or account identifiers logged
• Targets dev
• Not a GUI change, so no screenshot applies

Summary by CodeRabbit

• Bug Fixes

  • Improved handling of failed command results containing empty output.
  • Added support for Windows-style line endings in failure messages.
  • Preserved correct behavior for indented and duplicate empty-output markers.
  • Prevented malformed, whitespace-heavy results from causing slow processing.

• Tests

  • Added regression coverage for empty-output failure scenarios and performance safeguards.

[github-actions]

✅ Deterministic PR hygiene checks passed.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: b3bfe989-76d6-41d4-8435-bbf7266c9f5d

📥 Commits

Reviewing files that changed from the base of the PR and between 9f68176 and 2b760c6.

📒 Files selected for processing (2)

• src/adapters/exec-tool-result-normalize.ts
• tests/cursor-exec-empty-result.test.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.

---

📝 Walkthrough

Walkthrough

The PR replaces regex-based failed exec wrapper detection with a forward-scanning predicate. Cursor integrations use the shared predicate. Tests cover indentation, duplicate markers, CRLF input, and long malformed whitespace.

Changes

Failed execution wrapper classification

Layer / File(s)	Summary
Linear wrapper scanner src/adapters/exec-tool-result-normalize.ts	Lines 20–91 replace FAILED_EXEC_OUTPUT_REGEX with isFailedEmptyExecWrapper. The scanner supports CRLF separators, optional metadata, indented <empty> markers, and rejects duplicate markers. Line 162 uses the new predicate during normalization.
Shared predicate integration src/adapters/cursor/tool-result-normalize.ts	Lines 16, 26, and 102 use isFailedEmptyExecWrapper for Cursor Computer Use, node_repl, and the Codex exec bridge.
Classification and performance coverage tests/cursor-exec-empty-result.test.ts	Lines 48–116 cover indented markers, duplicate markers, CRLF wrappers, and a 60,000-character malformed whitespace input.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 2b760

The change improves failed-wrapper recognition, preserves payload-protection boundaries, and adds CRLF handling without changing public interfaces or deployment behavior. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: ingwannu

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 7 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the primary change: replacing backtracking regex classification with a forward scan for failed exec wrappers. It is specific and concise.

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch lane-s/2938-redos-linear

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[chatgpt-codex-connector]

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review	Status	Commit	Review trigger
📝 Code Review	✅ Completed 2026-08-29T20:36:54.739910Z	9f68176	PR opened

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9f68176b41

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Remove the unshipped ReDoS write-up from the public devlog

This tracked planning note publishes the exact vulnerable expression, attack shape, and measured reproduction while the fix is still under review and therefore unshipped. Move this material to .tmp/ or another scratch directory and publish only the shipped outcome; repository policy explicitly prohibits storing unreleased security findings and reproduction steps for unfixed defects in tracked directories.

AGENTS.md reference: AGENTS.md:L103-L110

Useful? React with 👍 / 👎.

[lidge-jun]

리뷰 · 우선순위 69 / 80

이 PR은 지금 dev HEAD 370052648 (#2943, Copilot 카탈로그가 겹친 supports.vision 을 읽게 함) 바로 위에서, 실패한 exec 감싸기 글을 정규식 대신 앞으로만 가는 스캔으로 나눈다. 미리보기 배포는 계획에 없고, types.ts/config.ts 분할과도 안 겹친다. 손대는 파일은 src/adapters/exec-tool-result-normalize.ts, src/adapters/cursor/tool-result-normalize.ts, tests/cursor-exec-empty-result.test.ts, devlog/_plan/260830_lane_s_2938_redos_linear/000_units.md 네 곳이고, 더하기 176줄·빼기 6줄이다. 갈래는 dev 이고 라벨은 bug 다. 작성자는 메인테이너(lidge-jun)다.

지금 HEAD 가 하고 있는 일의 한가운데는 아니다. 최근 끝은 Copilot 비전, Grok 상속, Ollama 네이티브, Codex 401·권한·쿼터 쪽이다. 그래도 같은 Cursor tool-result 길을 #2900/#2903/#2910/#2936 이 이미 호출 예산과 call-id 와 순서로 잠가 두었다. 그 길이 받는 글을 나누는 곳이 바로 이 두 어댑터 파일이다. 여기를 잘못 나누면 실패한 칸이 빈 성공으로 보이거나, 반대로 실제 출력이 실패 안내문으로 바뀐다.

쉽게 말하면 이렇다. 코드 모드 exec 칸이 실패하고 출력이 비면, 도구 결과는 Script failed 로 시작하는 감싸기 글이 온다. 예전에는 FAILED_EXEC_OUTPUT_REGEX 가 이 글을 잡았다. 패턴 안에 \n* 와 \s* 가 같은 공백 구간을 나눠 먹을 수 있어서, 앞부분만 맞고 끝까지 안 맞는 입력이 오면 엔진이 앞부분을 반복해서 다시 맞춰 본다. 본문 측정은 "Script failed" + 스페이스 6만 개 + "\nY" 가 약 1224ms 다. 이 글은 도구 결과 안에 들어오므로 모양이 완전히 우리 손이 아니다.

고치는 방법은 isFailedEmptyExecWrapper 다. Script failed 로 시작하는지 본 뒤, 인덱스를 앞으로만 민다. Wall time, Output:, <empty> 순서를 예전과 같이 따른다. 줄바꿈 탐색은 겹치지 않는 앞쪽 구간만 보고, 토큰 검사는 고정 길이다. 정규식도 재귀도 접두사 재시도도 없다. Cursor 쪽 normalizeCursorToolResultText 와 공유 normalizeEmptyExecToolResultText 가 둘 다 이 함수를 쓴다. HEAD 에서 FAILED_EXEC_OUTPUT_REGEX 를 쓰는 곳은 이 두 파일뿐이다. 내보내기를 지워도 다른 모듈이 깨지지 않는다. Kiro 는 src/adapters/kiro.ts 에서 이미 공유 헬퍼만 쓰므로 같은 분류를 탄다. Computer Use 분기는 예전처럼 isEmptyOrFailedExecWrapper 로 먼저 가고, 그 안에서 실패 팔만 새 함수로 바꾼다. isError 정책은 예전과 같다.

왜 열린 #2938 을 그대로 못 넣었는가. 진단과 앞으로 가는 스캔 생각은 @luvs01 의 #2938 것이다. 이 PR 본문도 그렇게 적었다. 다만 그 스캔은 옛 정규식과 여섯 입력에서 어긋났고, 방향이 반대라 "더 엄격한가 느슨한가" 로는 안 보인다. 세 개는 Output: 다음 줄에 들여 쓴 <empty> 를 실패로 안 나눠서, 실패가 설명되지 않은 채 지나간다. 세 개는 마커가 두 번 있는 감싸기를 실패로 나눠서, 실제 출력을 실패 안내문으로 바꿔 버린다. 정규화가 글을 지우는 쪽이 더 나쁘다. 그래서 이 구현은 경계를 두 개 박았다. Output: 뒤 공백은 마커 앞에 올 수 있다. 들여 쓴 <empty> 도 실패로 나눈다. 마커 뒤에는 공백만 올 수 있다. 두 번째 <empty> 가 있으면 분류하지 않는다.

의도한 행동 변화는 하나다. 옛 정규식은 빈 줄 구분자를 \n 만 봐서, Windows 가 만든 Script failed\r\n\r\nOutput: 같은 실패 감싸기가 빈 성공 안내(NOT lost context, Do not re-run) 로 떨어졌다. 칸은 실패했는데 모델은 아무 일도 없었다는 말을 읽는다. skipFailedWrapperBlankSeparators 가 \r\n 빈 줄을 하나의 구분자로 먹는다. 63개 모양 비교에서 남은 불일치는 이 CRLF 네 개뿐이라고 한다. 그 네 개가 테스트에 잠겨 있다.

테스트는 tests/cursor-exec-empty-result.test.ts 에 네 개를 더 넣었다. 들여 쓴 마커, 중복 마커, CRLF, 6만 공백 시간 제한. 변이를 네 개 돌려서 각각 의도한 테스트가 빨개진다고 적혀 있다. 마지막 변이가 중요하다. 옛 정규식을 되돌리면 시간 테스트가 1224ms 로 500ms 한계를 넘고 CRLF 테스트도 빨개진다. 기계가 빨라서 통과하는 것이 아님을 보여 주려는 장치다. 로컬에서 그 두 파일 36개, tsc, privacy:scan 은 초록이라고 했다. 본문도 500ms 는 임계값이지 선형의 증명이 아니라고 적었다. 선형 주장은 코드 구조에 기대는 것이다.

라인 20-21 - src/adapters/exec-tool-result-normalize.ts 모듈 주석이 아직 "Cursor keeps its own broader regex" 라고 한다. 이 PR 이후 Cursor 는 공유 함수 isFailedEmptyExecWrapper 를 쓴다. Computer Use 의 isError 정책은 그대로지만, "더 넓은 정규식" 은 이제 사실이 아니다.
라인 23 - EMPTY_EXEC_OUTPUT_REGEX 는 그대로 정규식이다. 실패 쪽만 스캔으로 바꿨다. 성공 감싸기의 줄바꿈 묶음과 끝의 공백 묶음은 이번 ReDoS 와 같은 급은 아니지만, 짝이 되는 분류기가 정규식으로 남는다. Windows 빈 성공 감싸기의 CRLF 도 이번 범위 밖이다.
라인 77 - 첫 줄에 \n 이 없으면 Script failed 로 시작하기만 해도 true 다. 옛 정규식의 [^\n]* 와 같지만, 같은 줄에 나머지 글자가 있어도 실패 감싸기로 나눈다. 테스트에 이 모양이 없다.
라인 50-52 - skipFailedWrapperLine 은 Wall time 줄에서 \n 만 찾고, 없으면 나머지 전체를 삼킨다. 옛 Wall time[^\n]* 와 같다. 줄바꿈 없는 Wall time 줄은 테스트에 없다.
라인 26 / 라인 102 - src/adapters/cursor/tool-result-normalize.ts 가 같은 text.trim() 에 대해 isFailedEmptyExecWrapper 를 두 번 부른다. 한 번은 isEmptyOrFailedExecWrapper 안에서, 한 번은 실패 안내와 빈 성공 안내를 고를 때다. 결과는 맞다. 호출만 겹친다.
경로/심볼 - tests/cursor-exec-empty-result.test.ts - 새 테스트 네 개 모두 normalizeCursorToolResultText 만 친다. Kiro 가 직접 쓰는 normalizeEmptyExecToolResultText 를 치는 줄은 없다. 공유 함수를 바꾸는데 헬퍼·Kiro 직접 테스트가 없다.
라인 94-105 - 500ms 는 임계값이지 선형의 증명이 아니다. 본문도 그렇게 적었다. 선형 스캔의 실제 비용은 이보다 훨씬 작다. 바쁜 CI 에서 드물게 흔들릴 여지는 작다.
경로/심볼 - devlog/_plan/260830_lane_s_2938_redos_linear/000_units.md 제목이 "a backtracking classifier" 라서 이 PR 이 백트래킹을 넣는 것처럼 읽힌다. 옛 결함을 가리키는 말이다.
경로/심볼 - #2938 - 공로는 본문에 분명히 있다. 지금 그 PR 은 열린 채 review-ready 다. 이 패치가 들어가면 열린 PR 숫자에 형제로 남는다. 63개 비교 하네스는 저장소에 없고, 테스트 파일에 고정한 모양만 남는다.

메인테이너의 판단이 필요한 지점

• fix(adapters): classify failed exec wrappers by line scan, not backtracking #2938 을 이 PR 이 랜딩하면 superseded 로 닫을지, @luvs01 이 이 경계(들여 쓴 마커 / 중복 마커)를 자기 브랜치에 받아 fix(adapters): classify failed exec wrappers by line scan, not backtracking #2938 을 넣을지. 작성자는 둘 다 된다고 했다. 진단과 스캔 생각은 그쪽 것이다
• 적대 입력의 도달 가능성. 감싸기 글은 우리 exec 길이 만들지만, 도구 결과는 모델이 영향을 줄 수 있다. 실해 증거는 본문도 없다고 했다
• 500ms 한계를 게이트로 둘지, 구조만으로 충분한지. 본문은 임계값이지 증명이 아니라고 적었다
• 성공 쪽 EMPTY_EXEC_OUTPUT_REGEX 를 같은 스캔으로 이을지, 이번엔 실패 쪽만 넣을지. Windows 빈 성공 감싸기 CRLF 도 여기 묶인다
• 63개 비교 하네스를 저장소에 넣을지. 지금 테스트는 고정한 모양만 잠근다

너의 추천
이 PR 을 dev 로 넣는 쪽으로 가세요. 도구 결과에 실릴 수 있는 글의 이차 시간 비용을 없애고, Windows CRLF 실패 감싸기가 빈 성공으로 떨어지던 구멍도 막았고, #2938 이 빠뜨린 중복 마커 데이터 손실을 이 브랜치가 막고 있다. 동작 변경은 CRLF 구분자 하나뿐이고 그 네 모양이 테스트에 잠긴다. types/config 분할과 무관하니 close-don't-rebase 대상이 아니다. 주석 라인 20-21 의 "broader regex" 만 고치고, 성공 팔 정규식은 후속으로 남겨도 된다. 랜딩 직후 #2938 에 Landed via #2945 at <commit> 를 남기고 라벨 landed-via-maintainer 를 붙인 뒤 닫으세요. @luvs01 이 같은 경계를 #2938 에 먼저 밀어 넣으면 그쪽을 넣고 이 PR 을 닫아도 된다. 공로는 그쪽에 남겨 두세요.

이 댓글은 grok-bot이 작성했습니다

[lidge-jun]

Merging on the operator's standing admin authorization. Recording the reasoning rather than leaving it implicit, because this one touches a defect class where "green CI" was demonstrably not sufficient — the first head of this PR was fully green while carrying a policy violation.

What the evidence actually covers on 2b760c620a93252502daa1423eb354a2931347bc:

• All 28 hosted checks SUCCESS, including macOS and all four Linux shards.
• Equivalence verified twice independently: my 63-shape corpus and a reviewer's separately generated 662-shape pairwise corpus. Neither found a disagreement outside the intended CRLF blank-separator class. The reviewer's corpus specifically covered bare \r, U+2028, NBSP outer whitespace, and Wall timeout lookalikes — inputs I had not thought to probe.
• Complexity confirmed O(n) time and O(1) space by instrumented operation counts, not just timing, with the previous pattern measured at roughly 4x cost per input doubling.
• Four mutations red at the intended test each.

Both review findings that mattered are fixed: the pre-disclosure note is out of tracked devlog/ and in gitignored .tmp/, and the bounded-work assertion measures CPU time instead of wall time.

This is not a credential or auth path, so MAINTAINERS.md does not require a separate security reviewer — but it is a hardening change, so I want the record to show it received an adversarial review that found real problems and that they were addressed rather than argued away.

@luvs01's #2938 stays open for them to respond to; the offer to merge their branch instead still stands, and if they take it I will revert this in favour of theirs.