refactor: extract experimental sync target wrapper

[ndycode]

Summary

• extract the experimental sync target loader wrapper out of settings-hub
• keep retrying file reads and target normalization behavior unchanged while slimming the settings facade
• add a focused test for the new experimental sync target entry helper

Validation

• npm run typecheck
• npm run lint -- lib/codex-manager/experimental-sync-target-entry.ts lib/codex-manager/settings-hub.ts test/experimental-sync-target-entry.test.ts
• npm run test -- test/experimental-sync-target-entry.test.ts

note: greptile review for oc-chatgpt-multi-auth. cite files like lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.

Greptile Summary

this pr extracts the windows-safe readJson wrapper (with EBUSY/EPERM/EAGAIN/ENOTEMPTY/EACCES retry) out of settings-hub.ts into a focused loadExperimentalSyncTargetEntry helper, then adds a targeted vitest suite for it. the refactor is behavior-preserving and the extraction boundary is clean.

• settings-hub.ts: straightforward swap — no logic change, just delegation to the new entry function
• experimental-sync-target-entry.ts: all deps injected; fully testable without side effects; windows retry codes are correctly preserved
• test coverage: the second test explicitly exercises the windows path (C:\state.json) and asserts the full retry options object — good
• gap: JSON.parse on a partial/empty file (possible on windows during concurrent writes) throws an unguarded SyntaxError; no test covers this path and no mapping to an { kind: "error" } result is present — this was pre-existing behavior but worth closing now that the logic lives in an isolated, testable unit
• minor: capturedReadJson in test 2 is captured and only checked toBeDefined(); the meaningful assertion is readFileWithRetry.toHaveBeenCalledWith, which already proves the wiring

Confidence Score: 5/5

• safe to merge — pure extraction refactoring with no behavior change and focused tests covering the windows-retry wiring
• the change is semantically identical to the pre-refactor inline logic in settings-hub; the only gap (unguarded JSON.parse) was pre-existing and is not a regression introduced here; test coverage for the new unit is solid for the happy path and the windows retry path
• experimental-sync-target-entry.ts — JSON.parse on a partial/empty file is unguarded; low urgency since this was pre-existing, but the isolated unit is now the right place to fix it

Important Files Changed

Filename	Overview
lib/codex-manager/experimental-sync-target-entry.ts	new extraction module wiring windows-safe retry into readJson; JSON.parse on partial file content is unguarded and untested
lib/codex-manager/settings-hub.ts	clean swap of inline logic to the new entry helper; behavior is semantically identical to before the refactor
test/experimental-sync-target-entry.test.ts	good windows-path and retry-options coverage; missing JSON.parse failure path and capturedReadJson assertion is redundant

Sequence Diagram

sequenceDiagram
    participant SH as settings-hub.ts
    participant ESTE as experimental-sync-target-entry.ts
    participant EST as experimental-sync-target.ts
    participant RFR as readFileWithRetry

    SH->>ESTE: loadExperimentalSyncTargetEntry({ loadExperimentalSyncTargetState, detectTarget, readFileWithRetry, normalizeAccountStorage, sleep })
    ESTE->>EST: loadExperimentalSyncTargetState({ detectTarget, readJson, normalizeAccountStorage })
    EST->>ESTE: calls readJson(path)
    ESTE->>RFR: readFileWithRetry(path, { retryableCodes: [EBUSY,EPERM,EAGAIN,ENOTEMPTY,EACCES], maxAttempts: 4, sleep })
    RFR-->>ESTE: raw string
    ESTE-->>EST: JSON.parse(raw)
    EST-->>SH: { kind, detection, destination? }

Loading

Prompt To Fix All With AI

This is a comment left during a code review.
Path: lib/codex-manager/experimental-sync-target-entry.ts
Line: 48-61

Comment:
**missing `JSON.parse` error coverage for partial windows writes**

the retry codes guard against locked-file errors (`EBUSY`, `EPERM`, etc.), but if `readFileWithRetry` succeeds and returns a partial/empty string — which can happen on windows when a file is being written concurrently — `JSON.parse` throws a `SyntaxError` that bubbles unhandled to the caller. the project's windows-safety focus makes this a worth noting gap.

consider catching and re-mapping:

```ts
readJson: async (path) => {
    const raw = await params.readFileWithRetry(path, {
        retryableCodes: new Set([
            "EBUSY",
            "EPERM",
            "EAGAIN",
            "ENOTEMPTY",
            "EACCES",
        ]),
        maxAttempts: 4,
        sleep: params.sleep,
    });
    try {
        return JSON.parse(raw);
    } catch {
        throw new Error(`failed to parse json at ${path}: file may have been partially written`);
    }
},
```

at minimum, a vitest case for this path would close the gap.

How can I resolve this? If you propose a fix, please make it concise.

---

This is a comment left during a code review.
Path: test/experimental-sync-target-entry.test.ts
Line: 53

Comment:
**redundant `capturedReadJson` assertion**

`capturedReadJson` is captured and then only asserted `toBeDefined()`, but the real meaningful assertion is `readFileWithRetry.toHaveBeenCalledWith(...)` on line 54, which already proves `readJson` was constructed and invoked. the `toBeDefined()` check and the capture variable add noise without extra coverage.

```suggestion
		expect(readFileWithRetry).toHaveBeenCalledWith("C:\\state.json", {
```

How can I resolve this? If you propose a fix, please make it concise.

_{Last reviewed commit: "Exercise sync target..."}

[chatgpt-codex-connector]

Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits.
Credits must be used to enable repository wide code reviews.

[coderabbitai]

📝 Walkthrough

Walkthrough

introduces a new experimental-sync-target-entry module that extracts file-reading and json-parsing logic for loading experimental sync targets, delegating calls from settings-hub to this dedicated entry point with injected dependencies.

Changes

Cohort / File(s)	Summary
Experimental Sync Target Entry lib/codex-manager/experimental-sync-target-entry.ts	new module exports loadExperimentalSyncTargetEntry; wraps readFileWithRetry with fixed retry codes (EBUSY, EPERM, EAGAIN, ENOTEMPTY, EACCES) at 4 max attempts, injects sleep for backoff, parses json, and delegates final state assembly to loadExperimentalSyncTargetState.
Settings Hub Refactoring lib/codex-manager/settings-hub.ts	removed inlined file reading and json parsing logic from loadExperimentalSyncTarget(); now calls loadExperimentalSyncTargetEntry passing readFileWithRetry, sleep, detectOcChatgptMultiAuthTarget, and normalizeAccountStorage as injected dependencies.
Test Coverage test/experimental-sync-target-entry.test.ts	new vitest test file validates loadExperimentalSyncTargetEntry invokes mocked dependencies correctly and returns expected target-state object shape with kind: "target".

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

---

notes for review

missing regression coverage: the existing integration tests for loadExperimentalSyncTarget() in settings-hub need to be re-run to confirm the delegation refactor preserves end-to-end behavior. test/experimental-sync-target-entry.test.ts:1 mocks the state function heavily—verify no gap in asserting actual retry loop behavior or json parse error handling.

concurrency risk: readFileWithRetry with maxAttempts: 4 and injected sleep could mask transient filesystem contention on windows. if multiple processes race to read the same config file, the fixed retry set might miss ERROR_FILE_NOT_FOUND or ERROR_SHARING_VIOLATION on windows. recommend adding integration tests that simulate concurrent file access.

windows edge case: verify sleep injection works correctly with timers on windows with dpc latency. no platform-specific handling for backoff timing visible in the diff.

🚥 Pre-merge checks | ✅ 2 | ❌ 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%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	title follows conventional commits format with lowercase imperative summary (50 chars) and valid type 'refactor', clearly describing the main change of extracting the experimental sync target wrapper.
Description check	✅ Passed	PR description follows template structure with summary, what changed, and validation sections. All critical info present; docs/governance checklist partially addressed via greptile analysis.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ 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 refactor/pr2-experimental-sync-target-wrapper

✨ Simplify code

• Create PR with simplified code
• Commit simplified code in branch refactor/pr2-experimental-sync-target-wrapper

---

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 and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@test/experimental-sync-target-entry.test.ts`:
- Around line 5-25: Update the test for loadExperimentalSyncTargetEntry to
deterministically exercise the readJson/retry wiring: mock readFileWithRetry
(the function passed into loadExperimentalSyncTargetEntry) to fail on the first
call with a windows-like transient error and succeed on the second call, and
assert that loadExperimentalSyncTargetState is still called and the final result
matches the expected shape; ensure you use vitest mocks (vi.fn) and
deterministic async behavior (returning Promises) so the retry branch in
loadExperimentalSyncTargetEntry (the logic that calls readJson /
readFileWithRetry) is executed and validated rather than only testing delegation
and final shape.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 21c6e111-45ce-41eb-ba40-2794c6552c5f

📥 Commits

Reviewing files that changed from the base of the PR and between 16acf57 and 60cabf8.

📒 Files selected for processing (3)

• lib/codex-manager/experimental-sync-target-entry.ts
• lib/codex-manager/settings-hub.ts
• test/experimental-sync-target-entry.test.ts

📜 Review details

⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)

• GitHub Check: Greptile Review

🧰 Additional context used

📓 Path-based instructions (2)

test/**

⚙️ CodeRabbit configuration file

tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.

Files:

• test/experimental-sync-target-entry.test.ts

lib/**

⚙️ CodeRabbit configuration file

focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios. check for logging that leaks tokens or emails.

Files:

• lib/codex-manager/experimental-sync-target-entry.ts
• lib/codex-manager/settings-hub.ts

🔇 Additional comments (2)

lib/codex-manager/settings-hub.ts (1)

773-779: good extraction wiring in settings hub.

the delegation in lib/codex-manager/settings-hub.ts:773 cleanly forwards the same io and normalization dependencies, which keeps loadExperimentalSyncTarget slim without changing call semantics.

lib/codex-manager/experimental-sync-target-entry.ts (1)

46-63: entry helper implementation looks solid.

lib/codex-manager/experimental-sync-target-entry.ts:46 through lib/codex-manager/experimental-sync-target-entry.ts:63 correctly centralizes retrying fs reads, json parsing, and normalization wiring in one place.

[ndycode]

Closing because this work is now included in main via #318 and #319.