Skip to content

fix(codex): compensate native restore when history migrates during writes - #4380

Merged
lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:agent/history-restore-migration-20260912
Sep 12, 2026
Merged

fix(codex): compensate native restore when history migrates during writes#4380
lidge-jun merged 3 commits into
lidge-jun:devfrom
luvs01:agent/history-restore-migration-20260912

Conversation

@luvs01

@luvs01 luvs01 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Recheck history compatibility after successful journal restoration or fallback config removal, before accepting a native restore.
  • A detected migration enters the existing config/profile/journal compensation path, rolls back the coordinated remove transition, and skips catalog/history restoration.
  • Add deterministic regression coverage for migration inside the write interval and successful legacy controls. Align existing fixtures with canonical paths, pre-import file spies, and the current skipped/compensation contract.

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)

  • Head 3f75991af98cd79774382bfb1041bdda2b2d1ee0, based on 81f6cd5915ca59f784a584d8cd739adff55c9bd0.
  • Six new cases failed on the unchanged dev implementation after reaching real restore writes.
  • Eight migration-refusal cases and two successful legacy controls passed after the fix.
  • The focused injection/write-lock/journal run initially passed 119 tests and failed five existing fixture/expectation cases. All five were corrected and passed targeted rechecks. A final three-case run passed with 28 assertions. No all-suite pass is claimed.
  • Typecheck, privacy, structure and diff checks passed; documentation built 425 pages.
  • The first commit isolates fixture corrections from the six-line runtime fix. Independent source review found no blocking issue.
  • Synthetic directories and database fixtures only; no installed service or live conversation was changed.
  • Current-head hosted full CI remains required; this PR is draft.

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.

Current rebase and readiness evidence (2026-09-12)

Current head: b06b520ce6b309be6c657f85294fb393dce9ad27, rebased onto dev@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.
16 pass
 0 fail
Ran 16 tests across 2 files. [54.54s]

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

    • Improved restore safety by detecting history migrations during configuration, profile, or journal restoration.
    • Automatically preserves prior configuration and journal state when a migration race is detected.
    • Prevents partial restores and rolls back coordinated removal changes when restoration cannot safely continue.
    • Successful restores continue to remove managed profile and journal entries and synchronize history as expected.
  • Documentation

    • Updated guidance describing restore verification, migration compensation, and rollback behavior.
  • Tests

    • Added coverage for journal and fallback restore scenarios across supported migration states.

@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview 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: Advanced

Run ID: bc37c3ee-2fec-435f-83cf-b5e35a725d7a

📥 Commits

Reviewing files that changed from the base of the PR and between 92e21bc and 7e8780b.

📒 Files selected for processing (11)
  • docs-site/src/content/docs/guides/codex-integration.md
  • src/codex/inject.ts
  • structure/catalog.md
  • structure/codex-home.md
  • structure/config.md
  • structure/gui-and-management-api.md
  • structure/ops/docs-and-release.md
  • structure/providers/openai-tiers.md
  • structure/runtime.md
  • structure/subagents.md
  • tests/codex-integration/codex-inject-integration.test.ts

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


📝 Walkthrough

Walkthrough

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

Changes

Native restore safety

Layer / File(s) Summary
Post-write restore validation
src/codex/inject.ts:1955-1960
restoreCodexConfigInlineImpl runs preflightCodexHistoryInjection(false, false) after a successful restore. If migration is detected, it returns history_paginated_requires_native_writer and reports that configuration and journal state were preserved.
Restore race integration coverage
tests/codex-integration/codex-inject-integration.test.ts:247-361
Parameterized tests cover journal and fallback restores across sync, legacy-uncoordinated, and coordinated transitions. Migration cases verify byte-for-byte restoration and unchanged transition state. No-migration cases verify successful convergence.
Restore safety contract documentation
docs-site/src/content/docs/guides/codex-integration.md:870-871, structure/*.md
Documentation describes post-write history checks, preserved configuration/profile/journal preimages, migration compensation, and coordinated remove rollback.

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
Loading

Merge Risk: ⚪ Minimal · up to 7e878

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)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning 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 … Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
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: compensating native Codex restore when history migration occurs during writes.
Full details: Docstring Coverage

Explanation

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

  • Fix all pre-merge checks with AI
✨ 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

Comment @coderabbitai help to get the list of available commands.

@lidge-jun

Copy link
Copy Markdown
Owner

리뷰 · 우선순위 71 / 80

설명

이 PR은 #4342가 막은 paginated history 외부 write 경계 위에서, 네이티브 restore가 journal/fallback 설정을 성공으로 되돌린 직후·최종 수락 전에 히스토리 형식이 바뀌는 창을 다시 한 번 검사합니다. 관련 이슈 #4311(페이지 분할 히스토리 프로젝션 정지)는 열어 두라고 본문이 말합니다. 네이티브 writer 락이나 “최종 검사 이후 이주”까지는 이 PR이 주장하지 않습니다.

지금 dev HEAD d42a1363dsrc/codex/inject.ts restoreCodexConfigInlineImpl는 journal restore 또는 fallback remove 성공 후 바로 ok를 반환합니다. 그 짧은 구간에 네이티브 쪽이 schema/ordinal migration을 넣으면, 설정은 되돌아갔는데 history는 이미 이주된 채 catalog/history 후속 단계로 넘어갈 수 있습니다. 패치는 성공 직후 preflightCodexHistoryInjection(false, false)를 다시 호출하고, 문제가 있으면 preimage 보상·remove 트랜잭션이 아직 가능한 상태에서 failed로 돌려 설정/저널을 보존합니다. 런타임 변경은 약 여섯 줄이고, 나머지는 fixture를 canonical path(realpathSync.native)·import 전 spy·성공 restore 중 migration 주입에 맞춘 것입니다. structure 여러 파일의 paginated history 문장도 같은 계약을 반영합니다.

#4342 후속이자 #4311의 restore-write 간격 조각입니다. #4368 라우팅이나 hub status 레인과는 파일이 안 겹칩니다. draft, 본문도 hosted full CI 미완을 인정합니다. types.ts/config.ts 분리와 무관하고, close-don't-rebase 대상도 아닙니다.

라인 - 이게 무슨 문제다

src/codex/inject.ts restoreCodexConfigInlineImpl 성공 직후 preflight - 방향이 맞습니다. 실패 메시지가 “configuration and journal preserved”인데, 호출부가 이 failed를 보상 경로로 항상 연결하는지만 확인하세요. preimage/remove rollback이 이 return에서 빠지면 문구와 실제가 어긋납니다.

테스트 migration 주입(setBeforeRestoreConfigForTests + unlink 훅) - schema/ordinal/none × sync/legacy/coordinated 조합이 좋습니다. Windows TEMP 8.3 alias를 realpathSync.native로 맞춘 것도 #4311/paths.ts 교훈과 일치합니다. 다만 hook 타이밍이 플랫폼마다 다르면 flake 후보입니다.

structure 다수 파일의 동일 문단 반복 - catalog/codex-home/config/gui/ops/openai-tiers/runtime/subagents에 같은 문장을 복제합니다. 머지 conflict 면적이 큽니다. 내용은 맞지만, 랜딩 직후 다른 history PR과 부딪히면 정리 비용이 납니다.

본문이 인정한 한계 - native-writer lock 없음, 최종 검사 이후 migration 미차단. #4311을 이 PR로 닫으면 안 됩니다.

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

  • 이 창만 막고 #4311은 계속 열어 둘지(본문·기존 리뷰와 동일 권고)
  • structure 복제 문단을 이 PR에서 모두 갱신할지, codex-home 한곳으로 줄일지
  • draft에서 full suite 필수인지, focused inject/journal + hosted CI로 충분한지

너의 추천
#4311은 열어 두고, 이 보상 검사만 CI 초록 후 머지하세요. native lock이나 외부 N+1 append PR은 계속 거절. structure conflict가 싫으면 문단 갱신을 codex-home+config 정도로 줄인 follow-up도 가능합니다. 우선순위는 history 가시성/복원 안전이라 hub 문서 수리보다 위에 둬도 됩니다.

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

@github-actions

Copy link
Copy Markdown
Contributor

Deterministic PR hygiene checks passed.

@luvs01

luvs01 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

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.

@luvs01

luvs01 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor Author

Completeness note for my previous status comment. Full enumeration of the failing jobs adds devin cli discovery > known install paths are preferred over a shadowed PATH entry, and absence is undefined (windows 6/6) to the Devin cluster already attributed to #4384, so that cluster is three cases rather than two. The conclusion is unchanged: injectCodexConfig integration (Design B) and codex-journal do not fail anywhere in this run on any platform, and every remaining failure belongs to open baseline work in #4386, #4384, or #4379.

@github-actions

github-actions Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

⏳ DRAFT

  • review readiness checklist open (2/4 boxes ticked).

What to do

  • Tick all four boxes in the PR description once you're done (currently 2/4).

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.

2/4 boxes ticked.

Automatic draft conversion failed. Please convert this pull request to a draft manually until every box above is ticked.

@luvs01
luvs01 force-pushed the agent/history-restore-migration-20260912 branch from 3f75991 to b06b520 Compare September 12, 2026 09:48

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

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.
@lidge-jun
lidge-jun marked this pull request as ready for review September 12, 2026 16:00
@lidge-jun
lidge-jun self-requested a review as a code owner September 12, 2026 16:00
@github-actions
github-actions Bot marked this pull request as draft September 12, 2026 16:00
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 12, 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-12T16:35:52.378998Z 7e8780b 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 marked this pull request as ready for review September 12, 2026 16:02
@github-actions
github-actions Bot marked this pull request as draft September 12, 2026 16:03
@lidge-jun

Copy link
Copy Markdown
Owner

Maintainer integration record (MAINTAINERS.md — review and merge policy).

Integrating into dev without a second maintainer approval under the maintainer-integration exception. Actor: @lidge-jun (project owner, GitHub admin access).

  • Exact head at integration: 7e8780b6f0be30a359157f297afb2cefe3feebce.
  • Exact-head verification: all PR checks pass on this head — Cross-platform CI run https://github.com/lidge-jun/opencodex/actions/runs/34703881234 (Linux test shards 1–4, macOS 1/2 + 2/2, gates, keyring, docker smoke, npm-global smoke, storage policy, api usage, aggregate ci) conclusion success; enforce-target / hygiene / label / CodeRabbit pass. (windows shards and macos control are designed skips for PRs.)
  • Base context: the branch merged origin/dev@92e21bc8ef; the current dev tip is 01e7d746d22e5a12aa35950c4035a3c5dee0bae6. The 20 newer dev commits touch none of this PR's runtime or test paths (structure/runtime.md overlaps and auto-merges cleanly).
  • Review state: no outstanding maintainer change requests — @Ingwannu's review is COMMENTED with no open review threads. The failures on the previous head b06b520c were stale-base issues (export-client/cline registries, i18n catalogs) and are green on the integrated head.
  • Security review: not required for this diff — no authentication, credential, OAuth, workflow, release-automation, or dependency-installation surface (6-line restore-time history recheck in src/codex/inject.ts plus tests and structure docs).

Squash-merging with --match-head-commit pinned to the SHA above.

@lidge-jun
lidge-jun marked this pull request as ready for review September 12, 2026 16:33
@lidge-jun
lidge-jun merged commit 2206f96 into lidge-jun:dev Sep 12, 2026
34 of 35 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants