test(oauth): await guardian fixture ACL hardening before cleanup

[luvs01]

Summary

The token-guardian fixture deletes its temporary home immediately after restoring the environment. Credential reads and writes can start optional asynchronous Windows ACL hardening that outlives saveCredential(); its icacls.exe child can still hold the directory when teardown removes it. The existing synchronous removal retries do not await that work.

Make the fixture's afterEach asynchronous and await flushConfigDirHardening(join(tmp, "ocx")) before restoring the environment or deleting the home. This settles only the fixture's own registered hardening flight and uses the existing cleanup API. Production token-guardian behavior, test assertions, and retry/time budgets are unchanged.

Verification

• Current head: e16fa26224aa886c0600be8ba5832a103d4f0140, based on dev 386b6a0d9a8acef818b9c40ebd472e4974750199.
• The unchanged authored patch still awaits the fixture-owned ACL-hardening flight before teardown. Earlier focused Windows validation passed 17 tests / 56 assertions. No release guard or package version was changed.
• Current-head author cross-platform CI run 34439043900: 26/26 jobs passed, bound to e16fa26224aa886c0600be8ba5832a103d4f0140. The checklist CI attestation refers to this completed matrix; local focused results are listed separately.
• Attempt 1 timed out in the unchanged history fixture the synchronous restore body is gated on skipHistory after 30 seconds (child status null). That exact case passed in isolation on this head with Bun 1.4.2: 1 test / 6 assertions, 7.28 seconds. The hosted timeout cause remains unproven. No product code or deadline changed, and previously successful jobs are retained.
• Historical large local runs, where mentioned previously, remain incomplete diagnostic evidence and are not reported as green.

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.

Review readiness checklist

• All CI tests are green on my local testing.
• I pushed my PR to the latest dev commit.
• I resolved all correct Codex and CodeRabbit findings.
• My PR is ready for review.

Readiness base check: 7 commits behind current dev; within the repository allowance of ten.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: c1e7a913-f4b8-4e29-aada-46baada6b0fb

📥 Commits

Reviewing files that changed from the base of the PR and between c15a98c and f7f275f.

📒 Files selected for processing (1)

• tests/codex-integration/token-guardian.test.ts

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

---

📝 Walkthrough

Walkthrough

The token guardian integration test now flushes pending configuration directory hardening before removing its temporary directory. The cleanup hook is asynchronous and awaits this operation.

Changes

Token guardian test cleanup

Layer / File(s)	Summary
Flush hardening before cleanup tests/codex-integration/token-guardian.test.ts	The test imports flushConfigDirHardening at line 7. The asynchronous afterEach hook awaits flushConfigDirHardening(join(tmp, "ocx")) at lines 50–52 before the existing cleanup steps.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Merge Risk: ⚪ Minimal · up to f7f27

The token-guardian fixture now waits for pending Windows ACL hardening before deleting its temporary configuration directory, preventing cleanup races without changing production behavior or test assertions. No current merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
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.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and concisely describes the main change: the OAuth token-guardian test now awaits ACL hardening cleanup before fixture removal. It matches the changes in tests/codex-integration/toke…

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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

[github-actions]

✅ Deterministic PR hygiene checks passed.

[github-actions]

✅ READY

• all PR quality gates passed; the review readiness checklist is complete.

Review readiness checklist

• ✅ All CI tests are green on my local testing.
• ✅ I pushed my PR to the latest dev commit.
• ✅ I resolved all correct Codex and CodeRabbit findings.
• ✅ My PR is ready for review.

✅ 4/4 boxes ticked.

This pull request is already Ready for Review.
The review-ready label marks this PR as ready; review automation runs independently.
Maintainers: @lidge-jun @Ingwannu

[luvs01]

@coderabbitai review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[lidge-jun]

리뷰 · 우선순위 64 / 80

설명

이 PR은 테스트 fixture teardown 순서만 고친다. 대상은 tests/codex-integration/token-guardian.test.ts 한 파일(+4/−1). base는 현재 dev 04808916c(#4098)와 같고, Draft다. 런타임·가디언 정책·assertion·시간 예산은 안 바뀐다.

문제 그림. Windows에서 credential 읽기/쓰기는 optional ACL hardening을 백그라운드로 띄울 수 있다. src/config/paths.ts의 flushConfigDirHardening 주석이 말하듯 icacls.exe 자식이 디렉터리를 붙잡고 있으면, fixture가 HOME/OPENCODEX_HOME을 되돌린 뒤 removeTreeWithRetry(tmp)를 호출해도 EPERM/EBUSY가 난다. 동기 재시도만으로는 “아직 안 끝난 hardening flight”를 기다리지 못한다. 실제 Windows CI에서 provider assertion 통과 뒤 cleanup 단계에서 깨진 로그가 본문에 링크돼 있다.

고치는 방법. afterEach를 async로 바꾸고, env 복구·트리 삭제 앞에 await flushConfigDirHardening(join(tmp, "ocx"))를 넣는다. 인자가 fixture가 만든 ocx 홈이라 getConfigDir()에 의존하지 않고, 다른 테스트의 flight를 전부 긁는 flushConfigDirHardeningForTests보다 범위가 좁다. 제품 코드가 이미 쓰는 cleanup API를 테스트가 따라가는 형태라 방향이 맞다.

지금 dev 기준 점수. tip 기능(#3719/#3379/#3774 등)이나 2.49.x 핫픽스 본체는 아니다. 그래도 Windows CI flake는 메인테이너 시간을 잡아먹고, token-guardian·credential 회귀를 거짓 실패로 물들인다. types/config 분할 무효화·중복 close 대상 아님. CodeRabbit·hygiene·enforce-target 등은 pass로 보이고, 본문은 full cross-platform CI 도는 중이라 Draft를 유지한다.

라인 - 이게 무슨 문제다

tests/codex-integration/token-guardian.test.ts afterEach 순서 - flush → lifecycle drain reset → env 복구 → removeTreeWithRetry 순서는 맞다. flush가 명시 경로 join(tmp, "ocx")를 쓰므로 env를 나중에 되돌려도 된다. 순서를 바꾸어 env를 먼저 지우고 getConfigDir() 기본 인자로 flush하면 잘못된 디렉터리를 기다릴 수 있으니, 지금처럼 경로를 고정한 채 두는 편이 안전하다.

flushConfigDirHardening vs flushConfigDirHardeningForTests - 전체 flight를 기다리는 테스트 전용 API도 있지만, 이 fixture는 자기 ocx만 등록했을 가능성이 커 좁은 API가 맞다. 다만 같은 파일/다른 스위트에서 CODEX_HOME 쪽 hardening이 따로 뜨면 ocx만 flush해서는 부족할 수 있다. 현재 실패 로그가 ocx 삭제 쪽이면 이 패치로 충분하고, 남는 EPERM이 있으면 inventory가 필요하다.

다른 fixture inventory - credential을 쓰는 integration 테스트 중 afterEach에서 동일하게 hardening을 await하지 않는 곳이 더 있을 수 있다. 이번 PR 범위 밖이지만, 같은 Windows flake가 다른 파일에서 재발하면 패턴 복제가 아니라 공통 helper로 묶는 편이 낫다.

Draft/CI - readiness 체크리스트가 아직 비어 있다. Windows에서 token-guardian 파일 10 tests 통과 주장은 설득력 있고, 현재 HEAD full CI green + Draft 해제가 머지 조건이다.

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

• CI green 직후 바로 dev에 머지할지(권장), 아니면 더 큰 auth 열차(fix(auth): keep credentials scoped to the final routed provider (carry of #4081) #4102)와 묶어 올릴지(이 PR은 독립·충돌 없음).
• 다른 integration fixture에 같은 flush를 일괄 적용하는 follow-up을 열지.
• Draft를 CI만 보고 빨리 해제할지.

너의 추천

CI green 확인 후 머지할 것. 한 줄짜리 올바른 대기이고, 문서화된 API를 쓰며 제품 동작을 안 건드린다. #4102/#4103과 무관하니 따로 랜딩해도 된다. 다음 스텝: (1) 현재 HEAD cross-platform CI green, (2) Draft 해제, (3) dev 머지. 같은 Windows cleanup flake가 다른 테스트에서 보이면 fixture inventory follow-up. types/config 분할 무효화 아님. CI 안정화·범위 작음이라 64.

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

[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-09-10T05:56:31.852957Z	e16fa26	Draft marked ready

ℹ️ 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.

[Ingwannu]

Reviewed 04b8187 against 0480891. Approving this one-file fixture cleanup change.

The explicit join(tmp, "ocx") matches the fixture's OPENCODEX_HOME. Waiting before environment restoration and directory deletion settles that directory's registered optional ACL flight, rather than relying on the ambient home or waiting on unrelated homes. src/config/paths.ts catches failures within the optional hardening flight and removes its entry in finally; the flush is a no-op when no flight exists. Production hardening and guardian behavior are unchanged, as are every existing assertion and retry budget.

I independently verified contributor CI run 34335603271: all 26 jobs succeeded at this exact head, including all Windows shards and macOS control. The motivating missing wait is supported by source; without an owning-handle capture, this does not prove every EPERM seen in other runs had that cause.

No local contributor code or live runtime/settings operation was executed. This approval covers test cleanup ordering, not a broader security audit, and does not merge the PR.

[luvs01]

@Ingwannu @lidge-jun This approved fixture-only change is the prerequisite for the requested #4090 rebase, which in turn precedes #4105. Current head remains 04b81874c346fbafc8dd3c6da6a9531adf82f726; the exact-head approval and 26/26 author CI are already complete.

The upstream Cross-platform CI and React Doctor still report action_required at that same head. Please approve the pending upstream runs and integrate this PR once your required gates are satisfied. I will then rebase #4090 onto the landed dev and run its final suite, following the existing review instruction. No unchanged author-CI rerun or new source change is proposed here.

[Ingwannu]

Approved the two pending upstream runs (Cross-platform CI 34335557657 and React Doctor 34338786605) for the unchanged reviewed head 04b81874c346fbafc8dd3c6da6a9531adf82f726. I rechecked the exact one-test-file delta and its workflow context: pull_request, read-only permissions, hosted runner selection, no workflow/install/dependency delta. This is execution approval, not a merge.

Current dev is now c15a98caa; the guardian runtime and fixture have changed since your reviewed base 04808916 (including terminal-verdict coverage). Keep that new coverage when carrying this cleanup wait forward. A green old-head run cannot establish compatibility with those newer callers. Please update against current dev before final integration; the narrow cleanup ordering review remains valid, but current-base acceptance is still required. #4090/#4105 stay sequenced after the actual landing.

[Ingwannu]

The approved CI has now completed: React Doctor passed, but Cross-platform CI run 34335557657 failed at the old head 04b8187.

Both failing test shards identify the release-version-line assertion: this branch still declares 2.49.0, which now equals the published v2.49.0 tag but is not that tag's commit. The Linux shard stopped at that failure; the macOS shard reported the same single failed test. This is not evidence that the ACL cleanup assertion regressed, but it is also not a green complete CI run.

Please carry the focused cleanup fix onto current dev (preserving the newer guardian tests) and rerun CI at the resulting exact head. Do not disable the release guard or make an arbitrary package-version bump just to pass this old branch. The related follow-ups remain sequenced after a verified landing.

[coderabbitai]

Note

GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer.

[luvs01]

Carried onto current dev; author CI green at the new head

@Ingwannu Done as requested. The cleanup fix now sits on dev c15a98caa9ab4b24256bd16a49b0947bea72d5c6 at head f7f275f123780d38c1c96e7514db563f934f58f8, 0 commits behind.

The newer guardian coverage is preserved. Rebasing brought in the guardian runtime and fixture changes that landed after my old base 04808916, including the terminal-verdict cases. tests/codex-integration/token-guardian.test.ts now carries 15 tests where that base had 10, and the awaited flushConfigDirHardening in afterEach applies on top of them without touching a single assertion. The diff against current dev is still one file, 4 insertions and 1 deletion. Locally on Windows with Bun 1.4.2 the whole file passes 17 tests / 56 assertions, so the new callers run through the corrected teardown ordering rather than around it.

The release-version failure is gone because of the rebase, not because of a guard change. As you noted, both old-head shards stopped at the release-version-line assertion for a branch still declaring 2.49.0. Moving to current dev removes that conflict on its own. I did not disable the release guard and did not bump the package version.

Full author CI is 26/26 green at this head, after one failed-only rerun of windows 2/6. I want to be precise about that rerun rather than present it as a clean first pass. The first attempt failed on exactly one case, Codex CLI updater zero-effect boundary > published Node launcher check neither executes the candidate launcher nor rewrites invalid state, which hit its 15-second bound at 15,010.90 ms while 3,015 other tests in the same shard passed. That case lives in tests/codex-integration/codex-cli-update-zero-effect.test.ts, a file this branch does not touch; its whole file passes locally on this head in 2.83 seconds, and the hosted rerun passed it. I read that as hosted-runner timing, not an assertion regression, and I raised no timeout and changed no code for it. If you would rather see a fully clean first attempt before integration, say so and I will dispatch a fresh full run.

The upstream Cross-platform CI (34431386203) and React Doctor (34431386142) runs for this head are again sitting at action_required and need your execution approval. The delta is the same one-test-file change under pull_request with read-only permissions and no workflow, install or dependency change.

The narrow cleanup-ordering review you already gave still applies unchanged, and #4090/#4105 remain sequenced after this actually lands.

[luvs01]

Current integration head is now e16fa26224aa886c0600be8ba5832a103d4f0140, rebased onto dev 386b6a0d9a8acef818b9c40ebd472e4974750199. Range-diff confirms the authored one-file cleanup patch is unchanged and the newer guardian coverage is retained.

The current-head upstream Cross-platform CI run 34439015408 and React Doctor run 34439015454 require maintainer execution approval.

Author CI run 34439043900 has now completed successfully: 26/26 jobs passed after one failed-job rerun. Its first attempt failed only the unchanged history fixture the synchronous restore body is gated on skipHistory at the 30-second deadline (child status null). The exact case passed locally on this head in 7.28 seconds, with 1 test / 6 assertions, and subsequently passed on the hosted rerun. The original hosted timeout cause remains unproven; no product code or deadline was changed and successful jobs were retained. #4090 and #4105 continue to wait for the actual landing.

[Ingwannu]

I inspected the current e16fa26 patch: it is still only the awaited fixture teardown on top of the newer guardian assertions. I approved the current-head hosted Cross-platform CI 34439015408 and React Doctor 34439015454 runs, not the older f7f275f runs linked in the earlier comment. Workflow/dependency files are unchanged, the pull_request runner selection remains GitHub-hosted, and privileges remain read-only. This is execution approval, not merge approval. The reported prior-head timed-test rerun is retained as historical context; the new head needs its own completed result.