Skip to content

test(oauth): drain ACL flights before fixture teardown - #3818

Merged
lidge-jun merged 2 commits into
devfrom
codex/platform-lane4-oauth
Sep 7, 2026
Merged

test(oauth): drain ACL flights before fixture teardown#3818
lidge-jun merged 2 commits into
devfrom
codex/platform-lane4-oauth

Conversation

@lidge-jun

@lidge-jun lidge-jun commented Sep 6, 2026

Copy link
Copy Markdown
Owner

Summary

OAuth store fixtures now await tracked asynchronous directory ACL work before restoring the environment or deleting their directory. Both ICACLS runners are stubbed, and a held-flight regression exercises the actual cleanup ordering. Production authentication and ACL policy are unchanged.

Carries the teardown residual from #3383 commit 51726d2c7; the numeric-open-flags report remains unverified. Layer 1 of the manual platform chain.

Verification

  • Final candidate: 6f2ad1ef32c113ff5827c58959c85cefa923b15c. Cross-platform CI, lane=all passed (26 successful jobs). The verification ref points to the same commit as the final PR.
  • Independent Astra high implementation review completed; container lifecycle and workflow changes received explicit independent security/contract reviews.
  • Documentation build passed on isolated Linux at this exact source snapshot: 425 pages.
  • Local test suites, typecheck, and Docker execution were not run, as instructed by the maintainer. Pushes use --no-verify.
  • Lower-layer expensive CI is deliberately omitted; only failed final checks triggered narrowed diagnosis. No lower-layer green is fabricated.
  • Earlier combined runs passed all six Windows shards, four Linux shards, and two macOS shards. They exposed Docker harness assumptions, a TypeScript 7 CLI requirement, and then a real container shutdown/catalog persistence defect. These were corrected. One earlier macOS full-control run failed an unchanged Cursor watchdog test; its cause remains unproven, with no timeout/skip workaround. Superseded runs are not final-head success evidence.

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.

Co-authored-by: x3M3x amroeid1999@gmail.com

Maintainer integration

The repository owner explicitly authorized admin integration of this manual chain into dev. The final combined head 6f2ad1ef32c113ff5827c58959c85cefa923b15c passed all 26 CI jobs. One cancelled macOS shard was rerun with unchanged source, commands and limits; its original stall remains recorded rather than claimed fixed.

Independent integration review found no conflict with current dev a5f9c3497. A clean merge produced tree 2c51d5e8042011fe8362e267b224016c6061d684; the relevant OAuth/body/container tests passed there on isolated Linux (129 passed, zero failures). This focused result is distinct from the full CI proof on the combined PR head. The branch commits preserve original contributors through Co-authored-by trailers.

Summary by CodeRabbit

  • Documentation

    • Added a comprehensive cross-platform validation plan covering container support, OAuth cleanup, diagnostics, Windows behavior, spill handling, CI verification, and completion criteria.
    • Documented container smoke-test expectations, including health checks, authentication, persistence, security constraints, and controlled failure handling.
    • Defined clearer diagnostic requirements for compressed request bodies and residual platform issues.
  • Tests

    • Improved OAuth test isolation and cleanup reliability, including validation that asynchronous platform cleanup completes before temporary resources are removed.

t and others added 2 commits September 7, 2026 07:35
Record scoped roadmap and independent plan audit. Local suites are excluded by owner instruction; final remote CI will verify the combined candidate.
Carry the remaining teardown fix from #3383 (51726d2) and verify cleanup ordering with a held asynchronous runner. Final combined CI will validate the stack; no local suite was run.

Co-authored-by: x3M3x <amroeid1999@gmail.com>
@coderabbitai

coderabbitai Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Caution

Review failed

The pull request is closed.

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Team

Run ID: 1d759794-16be-4083-bdfc-9de8e5c27dcc

📥 Commits

Reviewing files that changed from the base of the PR and between 137d6a7 and 6a51f04.

📒 Files selected for processing (8)
  • devlog/_plan/260907_platform_validation/000_plan.md
  • devlog/_plan/260907_platform_validation/001_plan_audit.md
  • devlog/_plan/260907_platform_validation/010_oauth_teardown.md
  • devlog/_plan/260907_platform_validation/020_container_smoke.md
  • devlog/_plan/260907_platform_validation/030_container_ci.md
  • devlog/_plan/260907_platform_validation/035_body_diagnostics.md
  • devlog/_plan/260907_platform_validation/040_residual_evidence.md
  • tests/oauth/oauth-store-multi.test.ts

📝 Walkthrough

Walkthrough

The change adds platform-validation planning documents for OAuth teardown, Docker smoke coverage, CI integration, body diagnostics, and residual evidence. OAuth fixture cleanup now waits for asynchronous Windows ACL hardening and includes a regression test for an in-flight hardening operation.

Changes

Platform validation

Layer / File(s) Summary
Validation scope and residual evidence
devlog/_plan/260907_platform_validation/000_plan.md:1-31, devlog/_plan/260907_platform_validation/001_plan_audit.md:1-5, devlog/_plan/260907_platform_validation/040_residual_evidence.md:1-15
The plans define validation scope, audit disposition, read-only investigations, source-grounded residual requirements, evidence rules, and completion statuses.
Container and body-diagnostic plans
devlog/_plan/260907_platform_validation/020_container_smoke.md:1-25, devlog/_plan/260907_platform_validation/030_container_ci.md:1-21, devlog/_plan/260907_platform_validation/035_body_diagnostics.md:1-17
The plans specify Docker build, health, admission, persistence, and cleanup checks; CI workflow integration; and bounded declared, observed, and decoded body diagnostics.
OAuth fixture teardown synchronization
devlog/_plan/260907_platform_validation/010_oauth_teardown.md:1-14, tests/oauth/oauth-store-multi.test.ts:7-14, tests/oauth/oauth-store-multi.test.ts:42-52, tests/oauth/oauth-store-multi.test.ts:79-83, tests/oauth/oauth-store-multi.test.ts:85-138
The OAuth tests configure synchronous and asynchronous ICACLS runners. Cleanup flushes hardening before resetting state, restoring OPENCODEX_HOME, or removing the fixture. A regression test holds an ACL operation and verifies that cleanup waits for its release.

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

✨ 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/platform-lane4-oauth

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.

@github-actions

github-actions Bot commented Sep 6, 2026

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 Sep 6, 2026
@lidge-jun

Copy link
Copy Markdown
Owner Author

리뷰 · 우선순위 54 / 80

이 PR은 지금 dev HEAD 137d6a7270e7ecfb1c791993800a17c0e30022d9 위에 올라가는 플랫폼 검증 체인 1층입니다. 하는 일은 단순합니다. OAuth 멀티 계정 스토어 테스트 픽스처가 끝날 때, 설정 디렉터리에 걸어 둔 비동기 ACL(접근 권한) 작업이 아직 돌아가는 중이면 그 작업을 먼저 기다린 다음에야 환경 변수를 되돌리고 임시 폴더를 지웁니다.

왜 이게 중요하냐면, Windows에서 디렉터리 권한을 고치는 ICACLS 작업이 백그라운드로 남을 수 있기 때문입니다. 예전에 #3383 쪽에서 남은 teardown(정리) 잔여가 바로 이 부분입니다. 생산(프로덕션) 인증 코드나 ACL 정책 자체는 손대지 않습니다. 테스트만 고칩니다. 파일은 tests/oauth/oauth-store-multi.test.ts 와 계획 문서 devlog/_plan/260907_platform_validation/ 아래입니다.

코드를 보면 cleanupOAuthStoreFixture가 먼저 flushConfigDirHardeningForTests() 를 await 하고, 그다음 동기/비동기 ICACLS 러너를 null로 되돌리고, OPENCODEX_HOME 을 복구한 뒤 removeTreeWithRetry 로 임시 디렉터리를 지웁니다. beforeEach에서는 동기 러너뿐 아니라 비동기 러너도 성공 stub으로 같이 꽂습니다. 이게 핵심입니다. 예전에 동기만 stub하고 비동기는 실러너로 새면 Windows CI에서 정리가 꼬일 수 있었습니다.

새 회귀 테스트는 플랫폼을 win32로 강제하고 가짜 Windows SID를 넣은 뒤, 비동기 ICACLS를 Promise로 붙잡아 둡니다. 그 상태에서 정리 함수를 시작하고 setImmediate 한 틱을 기다린 다음, 아직 정리가 끝나지 않았고 홈/디렉터리가 그대로인지 확인합니다. 그다음에야 release를 풀어서 정리가 끝나는지 봅니다. 실러너로 비행이 새지 않게 finally에서도 다시 stub을 씌웁니다. 이 순서는 현재 devflushConfigDirHardeningForTests / setAsyncIcaclsRunnerForTests 시임과 맞습니다.

types.ts/config.ts 대형 분할 캠페인과는 무관합니다. 테스트 픽스처만이라 close-don't-rebase 대상이 아닙니다. 런타임 #3808 Claude 호환이나 track2 chat과도 겹치지 않습니다. 다만 이 층만 머지해도 Docker/본문 진단은 안 옵니다. 체인 아래층입니다.

라인 86 - afterEach가 이제 async cleanup을 직접 await하지 않고 함수 참조만 넘깁니다. Bun/테스트 러너가 반환 Promise를 기다려 주는 현재 관례에 기대는데, 회귀 테스트 본문에서는 명시 await를 쓰므로 그 경로는 안전합니다.
라인 48-50 - removeTreeWithRetry는 동기 호출입니다. 기존 패턴 유지라 새 버그는 아니지만, flush 대기 후에야 지우므로 비동기 ACL과의 경합은 줄었습니다.
경로 tests/oauth/oauth-store-multi.test.ts - 생산 src/oauth 나 ACL 정책 파일은 변경 없음. 범위가 맞습니다.
경로 devlog/_plan/260907_platform_validation/000_plan.md - 체인 전체 계획(010→035→020→030)이 이 PR에 같이 들어옵니다. 문서만으로 상위 층 구현을 주장하지는 않습니다.

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

  • #3383의 Windows numeric-open-flags 보고는 이번에도 안 들어왔습니다. 이 PR로 feat(models): add main picker ordering controls #3383 전체를 닫을지, teardown만 landed로 볼지.
  • 로컬 스위트/타입체크를 의도적으로 안 돌렸고, 최종 lane=all CI(헤드 1f5881d00) 결과에 기대기로 했습니다. 머지 전에 그 런이 초록인지 확인할지.
  • 계획 문서 묶음을 1층에 넣는 것이 리뷰 단위로 괜찮은지, 아니면 구현 PR과 문서 PR을 더 나눌지.

너의 추천
스택 순서상 #3818을 가장 먼저 머지하세요. 베이스가 현재 dev와 같고, 상위 #3819/#3822/#3823이 이 커밋 조상을 전제합니다. 최종 Cross-platform CI lane=all이 초록이면 merge commit으로 올리고, 원 기여자 x3M3x 트레일러는 유지하세요. #3383 이슈/원 PR은 numeric-open-flags 잔여가 남아 있으면 이번 teardown만 landed 주석 후 이슈는 열어 두세요.

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

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 7, 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-09-07T00:49:23.415147Z 6a51f04 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.

@lidge-jun
lidge-jun merged commit 6388ec7 into dev Sep 7, 2026
21 of 25 checks passed
@lidge-jun
lidge-jun deleted the codex/platform-lane4-oauth branch September 7, 2026 00:48
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.

1 participant