Skip to content

fix(exec): rewrite empty-output wrapper regex without repartitionable quantifiers - #5774

Merged
lidge-jun merged 2 commits into
lidge-jun:devfrom
luvs01:fix/exec-empty-output-regex
Sep 25, 2026
Merged

lidge-jun merged 2 commits into
lidge-jun:devfrom
luvs01:fix/exec-empty-output-regex

Conversation

@luvs01

@luvs01 luvs01 commented Sep 24, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

EMPTY_EXEC_OUTPUT_REGEX in src/adapters/exec-tool-result-normalize.ts wrapped every optional section in plain quantifiers ([^\n]*\n+, \s*). Adjacent wildcard runs can repartition a newline/whitespace block combinatorially — the ReDoS shape the pattern had before.

Each wildcard run is now pinned to its maximal match with (?=(X))\1 lookahead-and-backreference, so the engine cannot redistribute characters between neighbouring quantifiers. The accepted language is unchanged.

Verification

  • bun test tests/adapters/exec-tool-result-normalize.test.ts — 25 pass, including the linear-time pathological-whitespace case.

Checklist

  • Base is dev
  • Tests updated for the new behavior

Summary by CodeRabbit

  • Bug Fixes
    • Improved recognition of empty command results across supported completion headers, spacing, and output formats.
    • Preserved the distinction between empty results, command failures, and actual output, including when trailing text follows an empty marker.
    • Improved handling of unusually long whitespace-only results to avoid slowdowns.
    • Clarified how trailing newlines are handled when determining whether a command result is empty.

… quantifiers

Adjacent \n+/\s* quantifiers in EMPTY_EXEC_OUTPUT_REGEX could repartition
a newline block combinatorially — 'Script completed\n' + 200k newlines +
'!' takes ~34s to reject on the old pattern. Each wildcard run is now
pinned to its maximal match via the lookahead-capture idiom; acceptance
is unchanged (25 tests incl. section combinations and a pathological
whitespace case pass, mutation-checked against the old pattern).

Co-Authored-By: Epinephrine <luvs01@hanmail.net>
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@github-actions

Copy link
Copy Markdown
Contributor

✅ Deterministic PR hygiene checks passed.

@coderabbitai

coderabbitai Bot commented Sep 24, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Repository: lidge-jun/opencodex/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 3c498c33-95d6-4da7-9b06-794dcdca75ec

📥 Commits

Reviewing files that changed from the base of the PR and between 7131af2 and 4a2f450.

📒 Files selected for processing (1)
  • tests/adapters/exec-tool-result-normalize.test.ts

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


📝 Walkthrough

Walkthrough

EMPTY_EXEC_OUTPUT_REGEX now pins wildcard runs to maximal matches. Tests cover accepted and rejected empty-output wrappers, matching with 200,000 newlines, and the behavior of normalization and isEmptyExecToolResult.

Changes

Exec output wrapper detection

Layer / File(s) Summary
Regex matching and validation
src/adapters/exec-tool-result-normalize.ts, tests/adapters/exec-tool-result-normalize.test.ts
The regex uses lookahead captures and backreferences to pin wildcard runs to maximal matches. Tests cover accepted wrapper forms, rejected failure headers and output, long newline inputs, and normalization behavior.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 4a2f4

No actionable merge-blocking risk is established by the reviewed changes.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: rewriting the empty-output wrapper regex to prevent repartitionable quantifiers. It matches the changes in src/adapters/exec-tool-result-norm…
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.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR

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

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

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 58 / 80

이 풀리퀘스트는 바탕이 dev예요. 명령 결과가 비었는지 가리는 식을 고쳐요.

빈 결과는 Script completed, Command finished, Execution finished, Wall time, Output:, <empty>가 섞여 있어도 본문이 없으면 빈 출력으로 봐요. 예전 식은 줄바꿈을 삼키는 자리가 여러 개 붙어 있었어요. 거절해야 하는 글자 앞에서, 그 자리들이 줄바꿈을 서로 나눠 가지며 경우의 수가 폭발해요. 작성자는 Script completed 뒤에 줄바꿈 20만 개와 !가 오면 예전 식이 약 34초 걸렸다고 적었어요.

새 식은 각 자리를 가장 길게 한 번만 먹게 고정해요. 헤더, Wall time, Output:, <empty>, 꼬리를 섞은 짧은 조합에서는 옛 식과 맞고 거절이 같아요. Script failed로 시작하는 실패 문구는 이미 앞에서부터 한 번만 읽는 함수예요. 이 글은 성공으로 보이는 빈 결과 식만 고쳐요. types.ts와 config.ts는 안 건드려요. 같은 식을 고치는 다른 열린 글은 없어요.

라인 - tests/adapters/exec-tool-result-normalize.test.ts 123행 — "Wall time 5s\n"은 식만 보면 통과해요. isEmptyExecToolResult와 normalizeEmptyExecToolResultText는 식을 돌리기 전에 trim()으로 앞뒤 공백과 줄바꿈을 잘라요. 잘리면 "Wall time 5s"가 되고, Wall time 뒤에 필요한 줄바꿈이 없어서 빈 출력으로 안 봐요. "Script completed\n"만 있는 줄도 같아요. 130행은 식에 바로 test를 해서, 실제 함수가 거절하는 이 줄을 통과로 적어요.

라인 - 같은 파일 148–152행 — 20만 줄바꿈이 끝나기만 하면 통과예요. 걸린 시간은 안 재요. 거절 두 줄(!, trailing)은 trim() 뒤에도 줄바꿈이 남아서, 실제 함수가 식에 넣는 입력이에요. Output: 뒤에 줄바꿈만 20만 개인 줄은 trim()이 먼저 "Output:"으로 줄여요. 함수 경로에서는 식이 그 긴 줄을 안 봐요.

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

성공 쪽도 실패 쪽 isFailedEmptyExecWrapper처럼 앞에서부터 한 번만 읽는 함수로 바꿀지 정해 주세요. 지금 식은 받는 문장이 예전과 같아요. 읽고 고치기는 어려워요. 실패 쪽 주석은 예전에 식을 다시 쓰다 받는 문장이 어긋난 적이 있다고 적혀 있어요.

끝 줄바꿈만 있는 "Wall time 5s\n"을 trim() 뒤에도 빈 출력으로 볼지는 이번 고침 밖이에요. 예전 식도 잘린 뒤에는 거절해요.

너의 추천

방향은 맞아요. 바탕은 dev예요. 닫을 중복 글은 없어요. 받는 문장은 그대로 두고, 줄바꿈을 나눠 먹어서 느려지던 거절을 없앤 고침이에요. 이대로 넣어도 돼요.

테스트를 보태면 더 좋아요. 123행 예시는 normalizeEmptyExecToolResultText에 넣어서, trim() 뒤에 빈 안내 문구가 나오는지 따로 적기. 148행에는 걸린 시간이 1초보다 짧은지 한 줄 넣기.

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

@luvs01

luvs01 commented Sep 24, 2026

Copy link
Copy Markdown
Collaborator Author

유지관리자 권고(선택 사항)를 둘 다 반영했습니다 (head: 4a2f450).

  • 실제 경로 테스트: "Wall time 5s\n"와 "Script completed\n"는 베어 정규식은 통과하지만 실제 함수 경로(trim 선행)에서는 빈 출력으로 보지 않음을 문서화하는 테스트를 추가했습니다 — normalizeEmptyExecToolResultText/isEmptyExecToolResult가 undefined를 반환함을 단언하고, 양성 대조군(Script completed\nWall time 0.1 seconds\nOutput:\n → EMPTY_EXEC_OUTPUT_MESSAGE)도 함께 고정했습니다.
  • 타이밍 단언: 20만 줄바꿈 거절 입력에 performance.now() 측정으로 1초 미만을 단언합니다(실측 ~1.25ms).

검증: exec-tool-result-normalize 26/26 통과, tsc --noEmit 클린.

@Ingwannu Ingwannu left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed exact head 4a2f450e9132f159ed265b1b0e87f7dbaf9b65f7. Atomic lookahead/backreference runs preserve the accepted wrapper language while preventing adjacent whitespace quantifiers from repartitioning pathological newline input. The callable trim-before-match divergence is explicitly pinned. Focused bounded suite passed 26/26; the 200,000-newline adversarial cases completed in ~1.6 ms under CPUQuota=200%, MemoryMax=4G, swap disabled.

@lidge-jun
lidge-jun merged commit 9d471d9 into lidge-jun:dev Sep 25, 2026
35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants