fix(codex): preserve settings when journal injection hashes are missing - #4006
fix(codex): preserve settings when journal injection hashes are missing#4006luvs01 wants to merge 2 commits into
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 (14)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe change adds verified-baseline checks for Codex journal restore and reinjection. Uncertain hashless states now preserve files and journals, report incomplete recovery, and block routed reinjection. Tests and multilingual documentation cover the updated behavior. ChangesCodex recovery safeguards
Priority: ➖ Normal — Impact reflects medium issue severity. Estimated code review effort: 3 (Moderate) | ~25 minutes Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to Hashless journal recovery now preserves uncertain user configuration and reports incomplete restoration rather than overwriting it. No actionable merge risk remains. Sequence Diagram(s)sequenceDiagram
participant CodexCommand
participant injectCodexConfig
participant journal.ts
participant CodexFiles
CodexCommand->>injectCodexConfig: request injection or restore
injectCodexConfig->>journal.ts: check journal baseline and hashes
journal.ts->>CodexFiles: compare saved originals with current files
journal.ts-->>injectCodexConfig: return verified or unverified state
injectCodexConfig-->>CodexCommand: complete recovery or failure
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (11 skipped: 11 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 |
|
✅ Deterministic PR hygiene checks passed. |
⏳ DRAFT
What to do
Review readiness checklist
2/4 boxes ticked. This PR stays in draft until every box above is ticked. Hygiene✅ Deterministic PR hygiene checks passed. |
|
@coderabbitai review Please review head |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== HEAD / BASE ==='
git rev-parse HEAD
git merge-base HEAD 74f62f9c2914ead2fba474aa97734e322251bd46 || true
echo '=== CHANGED FILES ==='
git diff --name-status 74f62f9c2914ead2fba474aa97734e322251bd46...HEAD
echo '=== JOURNAL CANDIDATES ==='
fd -t f -i 'journal' .
echo '=== INJECTOR / RESTORE CANDIDATES ==='
fd -t f . | rg -i '(codex.*(inject|restore)|inject.*codex|restore.*codex)'
echo '=== CHANGED SYMBOL REFERENCES ==='
rg -n -C 3 --glob '!node_modules/**' \
'(unverified|Unverified|injected.*hash|injection.*hash|routed.*journal|restore.*result|RestoreResult|reconcil)' .Length of output: 200581 🏁 Script executed: #!/bin/bash
set -euo pipefail
base="74f62f9c2914ead2fba474aa97734e322251bd46"
echo '=== CHANGED FILES ==='
git diff --name-status "${base}...HEAD"
echo '=== RUNTIME DIFF ==='
git diff --unified=80 "${base}...HEAD" -- src/codex/journal.ts src/codex/inject.ts
echo '=== JOURNAL OUTLINE ==='
ast-grep outline src/codex/journal.ts --items all --view expanded
echo '=== INJECT OUTLINE ==='
ast-grep outline src/codex/inject.ts --items all --view expanded
echo '=== JOURNAL API REFERENCES IN SOURCE ==='
rg -n -C 5 --glob '*.ts' --glob '!**/node_modules/**' \
'\b(reconcileJournal|restoreNativeCodex|restoreCodex|readCodexJournal|writeCodexJournal|injectCodexConfig)\s*\(' \
src tests/codex-integration
echo '=== CHANGED JOURNAL TEST DIFF ==='
git diff --unified=50 "${base}...HEAD" -- tests/codex-integration/codex-journal.test.tsLength of output: 138726 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== INJECTION: INPUT SNAPSHOT THROUGH COORDINATED WRITE ==='
nl -ba src/codex/inject.ts | sed -n '884,1435p'
echo '=== NATIVE RESTORE: ASYNC AND SYNCHRONOUS PATHS ==='
nl -ba src/codex/inject.ts | sed -n '1740,2028p'
echo '=== LIFECYCLE CALLER FAILURE HANDLING ==='
nl -ba src/cli/index.ts | sed -n '390,440p'
nl -ba src/server/stop-teardown.ts | sed -n '35,100p'
nl -ba src/client/connect.ts | sed -n '535,575p;670,700p'
nl -ba src/codex/sync.ts | sed -n '140,250p'
echo '=== DOCUMENTATION DIFF ==='
git diff --unified=30 74f62f9c2914ead2fba474aa97734e322251bd46...HEAD -- \
docs-site/src/content/docs/guides/codex-integration.md \
docs-site/src/content/docs/reference/cli/lifecycle.md \
structure/02_config-and-codex-home.md
echo '=== ALL DIRECT RESTORE-JOURNAL-STATE CONSUMERS ==='
rg -n -C 4 --glob '*.ts' '\brestoreJournalState\s*\(' src testsLength of output: 1301 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== INJECTION: INPUT SNAPSHOT THROUGH COORDINATED WRITE ==='
sed -n '884,1435p' src/codex/inject.ts
echo '=== NATIVE RESTORE: ASYNC AND SYNCHRONOUS PATHS ==='
sed -n '1740,2028p' src/codex/inject.ts
echo '=== LIFECYCLE CALLER FAILURE HANDLING ==='
sed -n '390,440p' src/cli/index.ts
sed -n '35,100p' src/server/stop-teardown.ts
sed -n '535,575p;670,700p' src/client/connect.ts
sed -n '140,250p' src/codex/sync.ts
echo '=== DOCUMENTATION DIFF ==='
git diff --unified=30 74f62f9c2914ead2fba474aa97734e322251bd46...HEAD -- \
docs-site/src/content/docs/guides/codex-integration.md \
docs-site/src/content/docs/reference/cli/lifecycle.md \
structure/02_config-and-codex-home.md
echo '=== ALL DIRECT RESTORE-JOURNAL-STATE CONSUMERS ==='
rg -n -C 4 --glob '*.ts' '\brestoreJournalState\s*\(' src testsLength of output: 79638
The changed logic preserves a changed config or profile when its own injected-state hash is missing. The result propagation is correct. The routed reinjection guard is present in both required locations. The regression coverage in I performed static source and diff review. I did not execute the test suite in this environment. Full contributor CI is still the remaining verification step. ✅ Action performedReview finished.
|
리뷰 · 우선순위 66 / 80이 PR은 이슈 #4005를 고칩니다. 지금 이 변경은 그 판단을 레거시 동작이 바뀝니다. 해시 없는 저널은 더 이상 ‘다른 현재 내용 전체를 원본으로 되돌릴 권한’이 없습니다. 검증된 해시는 예전처럼 복원·이후 편집 보존을 합니다. 이미 원본인 파일은 다시 쓰지 않고 끝냅니다. 외부 프로바이더 opt-out은 그대로입니다. 런타임 핵심은 테스트 쪽이 이 PR의 설득력입니다. 라인 단위로 보면 계약은 대체로 맞습니다. 다만 메인테이너가 한 번 더 눌러볼 지점이 있습니다.
메인테이너의 판단이 필요한 지점
너의 추천 이 댓글은 grok-bot이 작성했습니다 |
|
@coderabbitai review Please include the follow-up commit |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
previous="3606919780007fe2d4ca04449cbdc0085ee31e93"
followup="572d0b217a5b0de7027906ffa970644244556773"
echo '=== CHECKOUT HEAD AND COMMIT RELATIONSHIP ==='
git rev-parse HEAD
git merge-base --is-ancestor "$previous" "$followup" && echo "previous_is_ancestor=yes"
git merge-base --is-ancestor "$followup" HEAD && echo "followup_is_in_head=yes"
git show --no-patch --format='followup=%H%nparents=%P%nsubject=%s' "$followup"
echo '=== INCREMENTAL CHANGED FILES ==='
git diff --name-status "$previous" "$followup"
echo '=== INCREMENTAL DIFF ==='
git diff --check "$previous" "$followup"
git diff --unified=80 "$previous" "$followup"
echo '=== TEST FILE STRUCTURE ==='
ast-grep outline tests/codex-integration/codex-journal.test.ts --items all --view expanded
echo '=== TARGETED FIXTURE AND HASH CONTEXT ==='
rg -n -C 8 --glob '*.ts' \
'(injectedConfigHash|injectedProfileHash|originalConfig|originalProfile|foreign.owner|foreign-owner|matching)' \
tests/codex-integration/codex-journal.test.ts
echo '=== CURRENT CI RUN ==='
gh run view 34199076801 --repo luvs01/opencodex --json status,conclusion,event,headSha,workflowName,displayTitle,url,jobsLength of output: 104709 🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '=== JOURNAL HASH CONTRACT ==='
ast-grep outline src/codex/journal.ts --items all --view expanded
rg -n -C 8 --glob '*.ts' \
'(injectedProfileHash|function markJournalInjectedState|function compareJournalState|function reconcileJournal)' \
src/codex/journal.ts
echo '=== FOLLOW-UP FIXTURES WITH CURRENT LINE NUMBERS ==='
nl -ba tests/cli/cli-start-journal-order.test.ts | sed -n '45,92p;118,174p'
nl -ba tests/clients/client-connect.test.ts | sed -n '480,570p'
nl -ba tests/clients/client-connect.test.ts | sed -n '890,940p'
nl -ba tests/codex-integration/codex-catalog-restore.test.ts | sed -n '35,95p'
echo '=== ASSERTIONS FOR THE AFFECTED CLIENT CONTROLS ==='
rg -n -A 28 -B 5 --glob '*.ts' \
'journal ownership conflict preserves|process-owned journal|matching committed api key|foreign' \
tests/clients/client-connect.test.ts tests/cli/cli-start-journal-order.test.ts tests/codex-integration/codex-catalog-restore.test.ts
echo '=== CURRENT CI RUN STATUS ==='
gh run view 34199076801 --repo luvs01/opencodex \
--json status,conclusion,headSha,url,jobs \
--jq '{status, conclusion, headSha, url, jobs: [.jobs[] | {name, status, conclusion}]}'Length of output: 9077
The commit changes only these normal-recovery fixtures:
Each fixture now hashes the exact routed This is consistent with the journal contract. In The matching-client and foreign-owner controls now test journal ownership after the unverified-baseline guard. The guard cannot mask a regression in the ownership checks. The existing assertions remain valid. I also confirmed that this commit is the current PR head and directly follows the previously reviewed commit. It does not change the runtime restoration or routed-reinjection logic. The workflow run targets this exact SHA. The completed gates and multiple Linux, macOS, and Windows test jobs passed when checked. Some Windows shards and the macOS control job were still in progress. I cannot confirm the final full-CI conclusion yet. You are interacting with an AI system. |
572d0b2 to
ffdd705
Compare
|
@coderabbitai review Please verify rebased head The previous head's full contributor CI passed 26/26 jobs on attempt 2. Current-head CI is https://github.com/luvs01/opencodex/actions/runs/34208728445. Typecheck, privacy scan and diff checks passed on the rebased head; earlier test results remain explicitly bound to their original heads. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
old_head="572d0b217a5b0de7027906ffa970644244556773"
new_head="ffdd705561330424b65ddd4cdee2f49ff27d6366"
new_base="7797586a8899c673eab48886a490e85b480c6d72"
repo="lidge-jun/opencodex"
run_id="34208728445"
echo '=== HEAD AND ANCESTRY ==='
printf 'checked_out_head='; git rev-parse HEAD
printf 'new_head='; git rev-parse "$new_head"
printf 'new_base='; git rev-parse "$new_base"
printf 'merge_base(new_base,new_head)='; git merge-base "$new_base" "$new_head"
git merge-base --is-ancestor "$new_base" "$new_head" && echo 'new_base_is_ancestor=yes'
git merge-base --is-ancestor "$old_head" "$new_head" && echo 'old_head_is_ancestor=yes' || echo 'old_head_is_ancestor=no (expected for a rebase)'
echo '=== DIFF INTEGRITY ==='
git diff --check "$new_base...$new_head"
echo '=== PATCH IDENTITY ==='
old_base=$(git merge-base "$old_head" "$new_base")
printf 'old_base='; git rev-parse "$old_base"
old_patch=$(git diff --binary "$old_base...$old_head" | git patch-id --stable | cut -d' ' -f1)
new_patch=$(git diff --binary "$new_base...$new_head" | git patch-id --stable | cut -d' ' -f1)
printf 'old_patch_id=%s\nnew_patch_id=%s\n' "$old_patch" "$new_patch"
[ "$old_patch" = "$new_patch" ] && echo 'patch_identity=equal' || echo 'patch_identity=DIFFERENT'
echo '=== PR FILES AND UPSTREAM FILES ==='
git diff --name-only "$new_base...$new_head" | sort > /tmp/pr-files
git diff --name-only "$old_base...$new_base" | sort > /tmp/upstream-files
printf 'pr_file_count='; wc -l < /tmp/pr-files
printf 'upstream_file_count='; wc -l < /tmp/upstream-files
echo '-- PR files --'; cat /tmp/pr-files
echo '-- overlap --'
comm -12 /tmp/pr-files /tmp/upstream-files || true
echo '=== CURRENT WORKFLOW ==='
gh run view "$run_id" --repo "$repo" \
--json status,conclusion,headSha,event,workflowName,displayTitle,url,jobs \
--jq '{status, conclusion, headSha, event, workflowName, displayTitle, url, jobs: [.jobs[] | {name, status, conclusion}]}'Length of output: 3928 🏁 Script executed: #!/bin/bash
set -euo pipefail
old_head="572d0b217a5b0de7027906ffa970644244556773"
old_base="74f62f9c2914ead2fba474aa97734e322251bd46"
new_head="ffdd705561330424b65ddd4cdee2f49ff27d6366"
new_base="7797586a8899c673eab48886a490e85b480c6d72"
repo="luvs01/opencodex"
run_id="34208728445"
echo '=== COMPLETE DIFF-STREAM IDENTITY ==='
git diff --binary --full-index "$old_base...$old_head" > /tmp/old.patch
git diff --binary --full-index "$new_base...$new_head" > /tmp/new.patch
printf 'old_diff_sha256='; sha256sum /tmp/old.patch | cut -d' ' -f1
printf 'new_diff_sha256='; sha256sum /tmp/new.patch | cut -d' ' -f1
cmp -s /tmp/old.patch /tmp/new.patch && echo 'diff_stream_identity=equal' || echo 'diff_stream_identity=DIFFERENT'
printf 'old_diff_bytes='; wc -c < /tmp/old.patch
printf 'new_diff_bytes='; wc -c < /tmp/new.patch
echo '=== CURRENT WORKFLOW ==='
gh run view "$run_id" --repo "$repo" \
--json status,conclusion,headSha,event,workflowName,displayTitle,url,jobs \
--jq '{status, conclusion, headSha, event, workflowName, displayTitle, url, jobs: [.jobs[] | {name, status, conclusion}]}'Length of output: 3483
The rebase is valid:
Therefore, the static review result for The current workflow is correctly bound to The earlier 26/26 contributor-CI result remains evidence for the identical patch, but it is not a completed result for the rebased commit. You are interacting with an AI system. |
* test(lib): make idle deadline reset timing deterministic Carried from #4041 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test: stabilize Windows fixture waits and case cleanup Carried from #4015 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(codex): verify timeout termination without racing child timers Carried from #4012 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(codex): hold prompt-probe admission through document edits Carried from #4014 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * test(clients): bound transaction fixture child completion Carried from #4004 (author luvs01) for maintainer-side CI and sequential integration. Closes #4003 Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(codex): retain overlapping multiline TOML terminators Carried from #4039 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(cli): reject unsupported caps and report ignored legacy values Carried from #4043 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(codex): share trigger-only delegation guidance with v1 Carried from #4034 (author luvs01) for maintainer-side CI and sequential integration. Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * fix(codex): preserve settings when journal injection hashes are missing Carried from #4006 (author luvs01) for maintainer-side CI and sequential integration. Closes #4005 Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com> * docs(cli): note that --injection still accepts none and minimal CodeRabbit review on #4049: the effort reference documents caps only, so the new rejection of none/minimal reads as a global removal. --injection keeps both values because it sets the injection effort rather than a cap. --------- Co-authored-by: t <a@b.com> Co-authored-by: luvs01 <27862058+luvs01@users.noreply.github.com>
|
Landed on Verification at the exact merged head Closing this PR because the change is already on |
lidge-jun
added
the
landed-via-maintainer
2 participants
Summary
Closes #4005.
A missing injected-state hash currently lets an older Codex journal overwrite later config bytes or delete a later profile. Classify an already-original artifact separately from a verified injected artifact. If a changed config or profile lacks its own injection hash, preserve both files and the journal and return an explicitly unverified restore result.
Carry that result through automatic reconciliation and native restore so it cannot become successful fallback cleanup. Check a retained routed journal during read-only injection preflight and again before writing, preventing a new injected hash from being attached to an unverified older original. Native configuration can still establish a fresh snapshot. Preserve empty original profiles separately from absence.
Legacy behavior change: hashless journals no longer authorize whole-file restoration of differing current content. Verified hashes keep normal restoration and later-edit cleanup behavior. Already-original files complete without rewriting. External-provider opt-out remains unchanged. This addresses the reproducible missing-hash path discussed alongside #2948, not the unverified cause of that historical incident or all journal liveness/age policies.
The runtime change is limited to
journal.tsand its existing injector/restore caller. The Codex guide is synchronized across eight locales; English/Korean CLI references and the configuration invariant describe the refusal and retained recovery evidence.Verification
ffdd705561330424b65ddd4cdee2f49ff27d6366is based ondevcommit7797586a8899c673eab48886a490e85b480c6d72. The rebase preserves all 17 PR files byte-for-byte from reviewed head572d0b217; none overlap the 23 files changed upstream since the old base. This brings the branch within the automated readiness gate's base-freshness limit.bun run test -- --timeout 60000 --parallel=1 tests/codex-integration/codex-journal.test.ts: 34 tests / 163 assertions passed on the preceding base. Runtime and test bytes remain unchanged. Normal hashed restoration, changed-config preservation, original-profile recovery, reinjection hash retention and another-process marking remain covered.bun run test -- --timeout 60000 --parallel=1 tests/codex-integration/codex-inject-integration.test.ts tests/codex-integration/codex-inject-write-lock.test.ts: 57 tests / 359 assertions passed. These cover normal and changed profiles, user edits, CRLF, managed defaults, external-provider opt-out, coordination/adoption and held-lock behavior.bun run typecheck,bun run privacy:scanandgit diff --checkpassed on current headffdd70556.ffdd70556.codex-retained-root-serialization.test.tstried to consume a child's stdout twice and raised a disturbed/locked stream error before the catalog assertion. A deterministic child-exit ordering reproduces that fixture race. The initial isolated fix passed all 7 file cases / 35 assertions both ondevand as a temporary overlay on this PR; the overlay was restored and is not part of this diff. The one job-only rerun passed those catalog cases but failed the xAI fixture: a timed-out case continued into the following case's fetch mock. Both fixture fixes are now in test: stabilize Windows fixture waits and case cleanup #4015, whose final two-file CI-mode run passed 13 tests / 101 assertions. This PR remains draft pending integration and successful current-head CI; no third unchanged retry has been started. The original failures are retained, and the earlier green run on572d0b217is not attested as a run on this head.Checklist
Review readiness checklist