fix(codex): retain overlapping multiline TOML terminators - #4039
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe parser now handles overlapping escaped quotes and multiline TOML terminators correctly. Integration tests cover routing diagnostics and escaped content. English and Korean CLI documentation describe the parsing behavior. ChangesMultiline TOML diagnostics
Priority: ⬇️ Low — Defer this TOML scanner fix because it narrowly restores project-config diagnostics for overlapping multiline delimiters, with documentation and regression-test updates. Estimated code review effort: 2 (Simple) | ~10 minutes Merge Risk: ⚪ Minimal · up to This fixes project configuration diagnostics after multiline TOML strings containing an escaped quote before the closing delimiter, while preserving examples inside strings as ignored content. The covered parsing and diagnostic behavior is ready to merge. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation 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. (2 skipped: 2 unsupported.)
✨ 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 |
|
✅ Deterministic PR hygiene checks passed. |
|
@coderabbitai review Please review head The runtime change advances only one character after an escaped delimiter candidate, preserving an overlapping real terminator. Four regressions failed before the fix and now pass; Bun's TOML parser independently validates the fixtures. The negative control ensures escaped three quotes do not expose fake routing in the string body. Existing diagnostics and legacy-key parser consumers pass 38 tests / 102 assertions in total. Typecheck, privacy scan, diff check, independent read-only review, and the documentation build passed. Full cross-platform CI remains separate and pending. |
✅ READY
Review readiness checklist
✅ 4/4 boxes ticked. This pull request has been marked Ready for Review. Hygiene✅ Deterministic PR hygiene checks passed. |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base="7dc7dc99e65268bc8764e19840952256b030bce9"
head="7ce4dac80b5cc81e9f1eb1a9dbb4751f8dbe544c"
printf '%s\n' '=== Changed files ==='
git diff --name-status "$base" "$head"
printf '%s\n' '=== Diff (with function context) ==='
git diff --find-renames --find-copies --unified=80 "$base" "$head"
printf '%s\n' '=== Outlines of changed code files ==='
git diff --name-only "$base" "$head" \
| grep -E '\.(ts|tsx|js|jsx|mjs|cjs)$' \
| while IFS= read -r file; do
echo "--- $file"
ast-grep outline "$file" --items all || true
doneLength of output: 35338 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '=== Project-config parser and diagnostic flow ==='
nl -ba src/codex/project-config-warnings.ts | sed -n '45,325p'
printf '%s\n' '=== Full affected test section ==='
nl -ba tests/codex-integration/project-config-warnings.test.ts | sed -n '1,285p'
printf '%s\n' '=== Direct parser and scanner references ==='
rg -n -C 3 --glob '*.ts' \
'\b(parseTomlDocument|resolveEffectiveProjectModelProvider|analyzeProjectCodexConfig|multilineCloseIndex|multilineStateAfterLine)\s*\(' \
src tests
printf '%s\n' '=== Static delimiter-candidate model ==='
python3 - <<'PY'
def close_index(line, start=0):
delimiter = '"""'
index = line.find(delimiter, start)
while index >= 0:
backslashes = 0
cursor = index - 1
while cursor >= 0 and line[cursor] == '\\':
backslashes += 1
cursor -= 1
if backslashes % 2 == 0:
return index
index = line.find(delimiter, index + 1)
return -1
for slash_count in range(0, 5):
for quote_count in range(3, 8):
line = '\\' * slash_count + '"' * quote_count
print(f"slashes={slash_count}; quotes={quote_count}; close={close_index(line)}; repr={line!r}")
PYLength of output: 1626 🏁 Script executed: #!/bin/bash
set -euo pipefail
numbered_range() {
local file="$1"
local start="$2"
local end="$3"
awk -v start="$start" -v end="$end" 'NR >= start && NR <= end { printf "%6d\t%s\n", NR, $0 }' "$file"
}
printf '%s\n' '=== Project-config parser and diagnostic flow ==='
numbered_range src/codex/project-config-warnings.ts 45 325
printf '%s\n' '=== Full affected test section ==='
numbered_range tests/codex-integration/project-config-warnings.test.ts 1 285
printf '%s\n' '=== Direct parser and scanner references ==='
rg -n -C 3 --glob '*.ts' \
'\b(parseTomlDocument|resolveEffectiveProjectModelProvider|analyzeProjectCodexConfig|multilineCloseIndex|multilineStateAfterLine)\s*\(' \
src tests || true
printf '%s\n' '=== Static delimiter-candidate model ==='
python3 - <<'PY'
def close_index(line, start=0):
delimiter = '"""'
index = line.find(delimiter, start)
while index >= 0:
backslashes = 0
cursor = index - 1
while cursor >= 0 and line[cursor] == '\\':
backslashes += 1
cursor -= 1
if backslashes % 2 == 0:
return index
index = line.find(delimiter, index + 1)
return -1
for slash_count in range(0, 5):
for quote_count in range(3, 8):
line = '\\' * slash_count + '"' * quote_count
print(f"slashes={slash_count}; quotes={quote_count}; close={close_index(line)}; repr={line!r}")
PYLength of output: 42201
I found no blocking findings.
The scanner state flows correctly into The added tests in I also checked direct parser consumers in I could not execute repository tests in this sandbox. This review is based on static inspection and the verification results in the PR description. ✅ Action performedReview finished.
|
리뷰 · 우선순위 70 / 80이 PR은 Codex 프로젝트 설정 진단용 간이 TOML 스캐너( 왜 지금 테스트는 라인 72 - HEAD의 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
* test(lib): make idle deadline reset timing deterministic Carried from #4041 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test: stabilize Windows fixture waits and case cleanup Carried from #4015 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(codex): verify timeout termination without racing child timers Carried from #4012 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(codex): hold prompt-probe admission through document edits Carried from #4014 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(clients): bound transaction fixture child completion Carried from #4004 (author luvs01) for maintainer-side CI and sequential integration. Closes #4003 Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(codex): retain overlapping multiline TOML terminators Carried from #4039 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(cli): reject unsupported caps and report ignored legacy values Carried from #4043 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(codex): share trigger-only delegation guidance with v1 Carried from #4034 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(codex): preserve settings when journal injection hashes are missing Carried from #4006 (author luvs01) for maintainer-side CI and sequential integration. Closes #4005 Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * docs(cli): note that --injection still accepts none and minimal CodeRabbit review on #4049: the effort reference documents caps only, so the new rejection of none/minimal reads as a global removal. --injection keeps both values because it sets the injection effort rather than a cap. --------- Co-authored-by: t <a@b.com> Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
|
Landed on Verification at the exact merged head Closing this PR because the change is already on |
lidge-jun
added
the
landed-via-maintainer
2 participants
Summary
A valid TOML multiline basic string ending with an escaped quote immediately followed by its closing delimiter can hide the real provider/profile settings on subsequent lines from project-config diagnostics. For example, a backslash followed by four quotes represents one escaped quote and the three-quote terminator. The scanner rejects the first overlapping candidate but skips all three characters, missing the actual terminator one character later. Subsequent configuration is then incorrectly treated as string content.
Search the next candidate one character later after rejecting an escaped delimiter. Keep provider examples inside an unfinished string ignored, while restoring the existing warnings and effective-provider resolution for real settings after the string. The focused regressions cover root overrides, a same-line multiline string, a selected profile, and a selected provider table; a negative control keeps fake routing inside an escaped three-quote string body. English/Korean lifecycle documentation describes this diagnostic behavior. The change adds no settings or new warning types.
Verification
7ce4dac80b5cc81e9f1eb1a9dbb4751f8dbe544c, based ondev7dc7dc99e65268bc8764e19840952256b030bce9; Bun 1.4.0 on Windows.Bun.TOML.parse, including the actual escaped-quote string value. This follows the TOML 1.0 multiline basic-string syntax.bun run test -- --timeout 60000 --parallel=1 tests/codex-integration/project-config-warnings.test.ts: 26 tests / 59 assertions passed. Assertions cover effective provider, warning code/detail/profile, and provider-table contents, rather than only the internal string boundary.tests/codex-integration/codex-legacy-config-keys.test.ts, another direct parser consumer: 12 tests / 43 assertions passed.bun run typecheck,bun run privacy:scan, andgit diff --checkpassed. An independent read-only review found no required corrections.docs-sitebuild passed: 425 pages, 21.38 seconds. Both new paragraphs were verified in generated HTML without opening a preview. Existing dependency assets were reused only after matching their lockfile hash and declared dependencies.7ce4dac80and found no blocking findings after checking the parser state, diagnostic consumers, and positive/negative regressions. No inline findings are outstanding at this update. Full contributor CI completed successfully: 26/26 jobs, including all six Windows shards and the macOS control, on this exact head. Currentdevremains7dc7dc99e.Checklist
Review readiness checklist