fix(codex): compensate native restore when history migrates during writes - #4380
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 (11)
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughNative restore now performs a final history preflight after successful journal or fallback restoration. Migration detection returns a failure while preserving configuration and journal state. Integration tests cover migration races and successful restores. Documentation describes the updated safety contract. ChangesNative restore safety
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~25 minutes Change: Bug fix Sequence Diagram(s)sequenceDiagram
participant restoreCodexConfigInlineImpl
participant preflightCodexHistoryInjection
participant history_mode
restoreCodexConfigInlineImpl->>preflightCodexHistoryInjection: Run final preflight after restore
preflightCodexHistoryInjection->>history_mode: Check affected history
history_mode-->>preflightCodexHistoryInjection: Return migration status
preflightCodexHistoryInjection-->>restoreCodexConfigInlineImpl: Return failure when migration is detected
Merge Risk: ⚪ Minimal · up to The native restore safety change preserves configuration, journal, and coordinated removal state when migration is detected during the covered write interval. No unresolved merge-blocking risk remains. 🚥 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. (9 skipped: 9 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 |
리뷰 · 우선순위 71 / 80설명 이 PR은 지금
라인 - 이게 무슨 문제다
테스트 migration 주입( structure 다수 파일의 동일 문단 반복 - catalog/codex-home/config/gui/ops/openai-tiers/runtime/subagents에 같은 문장을 복제합니다. 머지 conflict 면적이 큽니다. 내용은 맞지만, 랜딩 직후 다른 history PR과 부딪히면 정리 비용이 납니다. 본문이 인정한 한계 - native-writer lock 없음, 최종 검사 이후 migration 미차단. 메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
✅ Deterministic PR hygiene checks passed. |
|
Exact-head CI for 3f75991 completed with failure: https://github.com/luvs01/opencodex/actions/runs/34678721625 The five recurring injection/journal assertion failures repaired here were absent from the inspected Linux, macOS and Windows failure logs. Remaining recorded assertion groups concern Cline inventories (#4386), three Devin path cases (#4384), and pnpm shims (#4379). This complements the focused regression evidence; it does not turn a red matrix into a pass or prove the cause of every job failure. Draft remains pending integration and validation. |
|
Completeness note for my previous status comment. Full enumeration of the failing jobs adds |
⏳ DRAFT
What to do
Review readiness checklist
2/4 boxes ticked. Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked. |
3f75991 to
b06b520
Compare
Ingwannu
left a comment
There was a problem hiding this comment.
Source review at b06b520 against c27a483.
The six-line runtime change is at the right compensation boundary: a successful journal/fallback write is rechecked before restoration is accepted. A failure returns through restoreCodexConfigInline's preimage restoration; the coordinated caller throws inside its transaction and the sync/uncoordinated callers stop before catalog/history work. The tests reach a successful write before injecting schema/ordinal migration, compare artifact bytes and coordinator state, and retain successful legacy controls. The fixture edits align canonical paths and install the file spy before import; they do not relax the production failure contract.
I approved this head's pending hosted Cross-platform CI 34686818215 and React Doctor 34686818210 after inspecting all 12 changed files and confirming no workflow/dependency/install changes. These are hosted pull_request runs, not self-hosted dispatches. This is execution approval only; prior-head failures and targeted passes are not full validation of this head.
Keep #4311 open. Compensation after a detected migration does not exclude a native write after the last check and is not native-writer coordination. No real Codex home, service or rollout was changed in this review.
Integrate origin/dev (1e28e62) so the restore-migration compensation can merge. Conflict resolutions: - structure/providers/openai-tiers.md: keep the PR invariant sentence (post-write history recheck, remove-transition rollback) plus dev's new Context relay ownership section. - tests/codex-integration/codex-inject-integration.test.ts: take dev's realpath/chmod fixture work wholesale (the spyOn-based denial never reached the ESM-bound readFileSync); retarget the new restore-migration cases to dev's failedConfigRestoreEnvelope contract (config failed, catalog/history skipped). - tests/codex-integration/codex-journal.test.ts: take dev's journal recovery envelope and damaged-cleanup snapshot coverage wholesale. Verified: bun test on both touched test files (109 pass, 0 fail), typecheck, structure:check, privacy:scan.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Maintainer integration record (MAINTAINERS.md — review and merge policy). Integrating into
Squash-merging with |
Summary
Addresses the remaining restore-write interval in #4311 after #4342. This does not establish a native-writer lock or exclude migration after the final check, so #4311 stays open.
Earlier focused verification (before this rebase)
3f75991af98cd79774382bfb1041bdda2b2d1ee0, based on81f6cd5915ca59f784a584d8cd739adff55c9bd0.Checklist
Review readiness checklist
Current rebase and readiness evidence (2026-09-12)
Current head:
b06b520ce6b309be6c657f85294fb393dce9ad27, rebased ontodev@c27a4831a9d1629005ffce626d54c7b60a00c1de. Range-diff shows only surrounding documentation whitespace/context changes. No unresolved review threads were found before publication. The latest-dev and resolved-findings items are checked on this evidence.Fresh verification on this head:
bun run typecheck— passed.bun run structure:check— passed.bun run privacy:scan— passed.bun test --isolate --timeout 60000 ./tests/codex-integration/codex-inject-integration.test.ts ./tests/codex-integration/codex-journal.test.ts -t migration|compensat|manifest-owned|preimage|hashless|damaged— passed.These are targeted checks, not a full CI pass. Previous hosted failures belong to the superseded head
3f75991af98cd79774382bfb1041bdda2b2d1ee0. Shared CI blockers (Cline inventories, native restore fixtures, Devin/pnpm fixtures) still require integration; known single-run failures also remain unproven resolved. The CI and Ready items stay unchecked, and the PR stays Draft. This rebase does not incorporate sibling PRs or claim their fixes. The upstream synchronize event supplies the new-head CI/approval entry; no duplicate fork matrix was dispatched merely to repeat known shared failures.Summary by CodeRabbit
Bug Fixes
Documentation
Tests