Skip to content

test(windows): create the external prompt fixture parent directory - #2972

Merged
lidge-jun merged 1 commit into
devfrom
codex/windows-prompt-path-260830
Aug 30, 2026
Merged

test(windows): create the external prompt fixture parent directory#2972
lidge-jun merged 1 commit into
devfrom
codex/windows-prompt-path-260830

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Aug 30, 2026

Copy link
Copy Markdown
Owner

Summary

The last Windows-only failure from the dispatched leg. #2968 fixed the first layer of this case and the
run at 1494acd98 showed it still failing — 4ms instead of 243ms, so it was failing earlier, which
is what identified the remaining defect.

The production path was never wrong. It decodes escaped paths, hashes the external path together with
the complete file bytes, and returns busy when the fingerprint differs.

What each layer was

Two distinct test defects stacked on the same case:

  • Originally the test wrote model_instructions_file = "C:\Users\..." as raw TOML. decodeBasicString
    accepts only \\, \" and \n, so \U was rejected, the base was classified default, the
    external file's bytes dropped out of the probe fingerprint, and the second request joined the stale
    flight — exactly what the case exists to forbid.
  • fix(windows): repair the two dispatch-only Windows test failures #2968's fix treated the backslash as a literal filename character. That holds on POSIX and is false on
    Windows, where external\base.md is a nested path whose parent directory the test never created,
    so it failed with ENOENT before reaching the assertion.

The fixture now creates the parent directory, encodes the TOML value with an oracle independent of the
production encoder, and asserts readPromptLayers(...).baseSelection before the probe starts rather
than assuming it. The backslash stays in the fixture so the TOML-escaping boundary is still exercised on
POSIX while being a real separator on Windows.

Verification

Based on dev@d2a802275.

  • bun test tests/codex-prompt-route.test.ts75 pass, 0 fail
  • Mutation-proven: restoring the raw TOML path reproduces baseSelection: default and one failure.
  • bun x tsc --noEmit → clean

A dispatched Windows run on this branch is the gate I will check before merging, since platform-windows
does not run on push.

Checklist

  • Scope stays focused and avoids unrelated cleanup.
  • Docs or release notes were updated when needed.
  • Security-sensitive changes were reviewed for secrets, auth, and unsafe defaults.

Summary by CodeRabbit

  • Tests
    • Updated coverage for external prompt base configuration.
    • Improved test setup to validate configuration state independently of the selection API.
    • Added verification for external file paths and directory creation.

@lidge-jun
lidge-jun requested a review from Ingwannu as a code owner August 30, 2026 02:58
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Aug 30, 2026

Copy link
Copy Markdown

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-30T03:00:57.943304Z 97a6972 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.

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@github-actions github-actions Bot added the chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature). label Aug 30, 2026
@coderabbitai

coderabbitai Bot commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

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: a2a1f583-dc38-4b48-82ee-dbaeb8de29c3

📥 Commits

Reviewing files that changed from the base of the PR and between d2a8022 and 97a6972.

📒 Files selected for processing (1)
  • tests/codex-prompt-route.test.ts

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


📝 Walkthrough

Walkthrough

Test 34 now creates the external base file directory, writes its configuration with JSON.stringify, and verifies baseSelection through readPromptLayers. The test no longer imports encodeBasicString or calls the base-selection route.

Changes

Codex prompt route tests

Layer / File(s) Summary
External base configuration test setup
tests/codex-prompt-route.test.ts
Test 34 removes the unused encodeBasicString import. It creates the external base directory, writes model_instructions_file with JSON.stringify, and verifies the resulting baseSelection state through readPromptLayers instead of calling PUT /api/codex-prompt/base/select.

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

Merge Risk: ⚪ Minimal · up to 97a69

This change prepares the Windows test fixture correctly and strengthens its path-selection assertion without changing production behavior. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: ingwannu

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: the Windows test fixture now creates the external prompt fixture parent directory before writing the file. It is specific, concise, and consistent with …
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1…
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.
Full details: Title check

Explanation

The title accurately describes the main change: the Windows test fixture now creates the external prompt fixture parent directory before writing the file. It is specific, concise, and consistent with the changes in tests/codex-prompt-route.test.ts.

Full details: Docstring Coverage

Explanation

No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 1 files.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch codex/windows-prompt-path-260830

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 Author

리뷰 · 우선순위 66 / 80

설명

이 PR은 #2968이 고친 같은 테스트가 Windows에서 한 겹 더 깨지던 자리를 메웁니다. 지금 dev HEAD는 7747bf74f (#2971)이고, #2968은 그 아래 41d7d4c3e입니다. #2968은 외부 프롬프트 경로를 TOML에 넣을 때 encodeBasicString을 쓰게 해서 \U 같은 잘못된 이스케이프가 default로 분류되던 문제를 없앴습니다. 그런데 Windows에서는 join(home, "external\base.md")가 파일 이름이 아니라 하위 경로가 됩니다. 부모 폴더를 안 만들어서 writeFileSyncENOENT로 먼저 죽고, 본문이 말한 디스패치 런 1494acd98에서는 243ms가 아니라 4ms에 실패했습니다. 더 앞에서 죽은 것입니다.

고친 내용은 테스트 한 파일입니다. 부모 디렉터리를 mkdirSync(..., { recursive: true })로 만들고, TOML 값은 생산 인코더 대신 JSON.stringify로 넣습니다. encodeBasicString은 백슬래시·따옴표·줄바꿈만 이스케이프하고, decodeBasicString도 그 세 개만 받습니다. join이 만든 경로에는 보통 백슬래시만 있어서 JSON 문자열이 그 세 개와 맞습니다. 프로브가 돌기 전에 readPromptLayers(...).baseSelection이 외부 경로인지 확인하니, 선택이 기본값으로 떨어지면 프로브 타이밍이 아니라 분류가 바로 빨갛게 됩니다. POSIX에서는 백슬래시가 파일 이름 글자라서 같은 TOML 경계를 그대로 재현합니다.

생산 경로(src/codex/prompt-layers.ts의 디코드·해시)는 안 바꿉니다. types.ts/config.ts 분할과도 무관합니다. 남은 게이트는 여전히 디스패치 Windows 레그입니다. push CI는 Windows를 안 돌립니다.

라인 - 문제

tests/codex-prompt-route.test.ts 1138줄 근처 - mkdirSync(dirname(externalPath))는 Windows에서만 새 폴더를 만듭니다. POSIX에서는 dirname이 이미 있는 decoyHome이라 해가 없습니다.

tests/codex-prompt-route.test.ts TOML 기록 - JSON.stringify가 제어 문자에 \uXXXX를 내면 decodeBasicString이 거절합니다. join 경로에서는 안 나오니 이 픽스처에는 맞습니다. 주석에 그 경계를 남겨 두면 나중에 다른 문자를 넣지 않습니다.

.github/workflows/ci.yml (이 PR 밖) - Windows가 여전히 dispatch 전용이라, 이번에도 진짜 확인은 디스패치 런입니다.

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

너의 추천

디스패치 Windows CI가 초록이면 dev에 병합하세요. #2968이 남긴 Windows ENOENT를 테스트만으로 고치고, POSIX에서도 같은 TOML 경계를 유지합니다.

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

@lidge-jun
lidge-jun merged commit aa16a71 into dev Aug 30, 2026
43 of 45 checks passed
@lidge-jun
lidge-jun deleted the codex/windows-prompt-path-260830 branch August 30, 2026 03:15

@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.

Approved exact head 97a69728cfe9cbf1d0081bfbc2ebb69a5c3aa412 against dev@d2a802275b6c16c3194e9a9cc55645c98e92089f.

The fixture change is scoped and correct: it creates the Windows parent directory before writing, uses an independent compatible string encoder, and proves readPromptLayers(...).baseSelection before the in-flight fingerprint assertion. Current dev is two commits ahead with no overlapping path.

Local isolated execution produced 74/75; the only failure is the unrelated Bun 1.3.14 TOML-overflow premise in case 39, and current clean dev@c2b64dbc3 fails the identical assertion. The changed case 34 passes locally.

More importantly, the exact-head workflow-dispatch run exercised tests\codex-prompt-route.test.ts on Windows and case 34 passed in windows 1/4. The normal exact-head Cross-platform CI run 33289202104 is fully green. The dispatch run still has an unrelated red windows 2/4 job (ocx init piped stdin, launchd retargeting, and two storage-coordinator timing assertions), so this approval is for the patch and its target regression; I am not merging while the author-defined dispatch gate remains red. No repository-wide security scan was run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

chore Maintenance, CI, tests, refactors, or build changes (not a user-facing bug or feature).

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants