Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
ndycode
force-pushed
the
fresh/18-import-adapter-opencode
branch
from
5bd459c to
3b1ba04
Compare
…' into work/pr103-remediate-20260318 # Conflicts: # docs/getting-started.md # lib/codex-manager.ts # lib/ui/copy.ts # test/codex-manager-cli.test.ts # test/storage.test.ts
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
lib/codex-manager.ts (2)
4645-4668:⚠️ Potential issue | 🟠 Major
confirm()can still abort this import flow.
lib/codex-manager.ts:4645-4650sits outside both import error handlers. ifconfirm()throws on a tty/readline failure, this path skips the new redacted handling and exitscodex auth logininstead of returning to the menu like the assessment and import stages.suggested change
- const confirmed = await confirm( - `Import OpenCode accounts from ${backupLabel}?`, - ); - if (!confirmed) { - continue; - } - try { + let importStage: "confirm" | "import" = "confirm"; + try { + const confirmed = await confirm( + `Import OpenCode accounts from ${backupLabel}?`, + ); + if (!confirmed) { + continue; + } + importStage = "import"; await runActionPanel( "Import OpenCode Accounts", `Importing from ${backupLabel}`, @@ - } catch (error) { + } catch (error) { const errorLabel = collapseWhitespace( formatRedactedFilesystemError(error), ); - console.error(`Import failed: ${errorLabel}`); + console.error( + `${importStage === "confirm" ? "Import confirmation failed" : "Import failed"}: ${errorLabel}`, + ); }please add the matching regression in
test/codex-manager-cli.test.ts:1by forcingconfirm()to throw and asserting the menu resumes. As per coding guidelines,lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4645 - 4668, The confirm() call before importing accounts can throw (e.g., tty/readline failure) and currently sits outside the import error handling so a thrown error will escape and exit the CLI; wrap the confirm() invocation in the same error-handling flow (or add a surrounding try/catch) so thrown errors are processed via formatRedactedFilesystemError()/collapseWhitespace and the command returns to the menu instead of exiting; update the block around confirm(), runActionPanel(), and importAccounts() (symbols: confirm, runActionPanel, importAccounts, formatRedactedFilesystemError, collapseWhitespace) to ensure confirm errors are caught and handled the same way as import errors, and add a vitest regression in test/codex-manager-cli.test.ts that forces confirm() to throw and asserts the menu resumes.
4371-4409:⚠️ Potential issue | 🟠 Majoradd the missing vitest for wizard-side opencode probe throws.
lib/storage.ts:2470-2495showsassessOpencodeAccountPool()can throw, andlib/codex-manager.ts:4406-4409is now the guard that keeps first-runcodex auth loginalive when that happens. the pr summary still calls this gap out, and i do not see the matching regression intest/codex-manager-cli.test.ts:1. please add a case that forces the assess step to throw, asserts the warning stays redacted, and verifies the wizard keeps looping instead of exiting. As per coding guidelines,lib/**: focus on auth rotation, windows filesystem IO, and concurrency. verify every change cites affected tests (vitest) and that new queues handle EBUSY/429 scenarios.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4371 - 4409, Add a vitest that simulates assessOpencodeAccountPool throwing and verifies codex-manager handles it without exiting: stub/mock assessOpencodeAccountPool to throw a filesystem error, stub getRedactedFilesystemErrorLabel to return a predictable redacted string, invoke the CLI path exercised by "import-opencode" (same flow exercised by test/codex-manager-cli.test.ts), assert console.warn was called with the redacted label returned by getRedactedFilesystemErrorLabel, and assert the wizard/CLI loop did not exit (e.g. the command returned to prompt or continued to the next loop iteration); reference functions assessOpencodeAccountPool, getRedactedFilesystemErrorLabel, runActionPanel, and importAccounts when locating code to stub and update the test to cover this regression.test/codex-manager-cli.test.ts (1)
1201-1209:⚠️ Potential issue | 🟠 Majorthese redaction assertions still only check
warnarg 0.
test/codex-manager-cli.test.ts:1201-1209andtest/codex-manager-cli.test.ts:1239-1240only inspectcall[0]. a raw windows path can still leak viacall[1+]and the tests stay green.proposed fix
- expect( - warnSpy.mock.calls.some(([message]) => - String(message).includes( - "Failed to refresh saved accounts after first-run action", - ), - ), - ).toBe(true); - expect( - warnSpy.mock.calls.every((call) => !String(call[0]).includes("alice")), - ).toBe(true); + const warningLines = warnSpy.mock.calls.map(flattenMockCallArgs); + expect( + warningLines.some((line) => + line.includes("Failed to refresh saved accounts after first-run action"), + ), + ).toBe(true); + expect(warningLines.every((line) => !line.includes("alice"))).toBe(true); @@ - expect( - warnSpy.mock.calls.every((call) => !String(call[0]).includes("alice")), - ).toBe(true); + const warningLines = warnSpy.mock.calls.map(flattenMockCallArgs); + expect(warningLines.every((line) => !line.includes("alice"))).toBe(true);as per coding guidelines,
test/**: 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.Also applies to: 1235-1240
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 1201 - 1209, The failing tests only inspect warnSpy.mock.calls[i][0], so sensitive data in later args can still leak; update the two assertions that currently check only call[0] (the expect(...includes("Failed to refresh saved accounts...")) and the expect(...every(call => !String(call[0]).includes("alice")))) to scan all arguments of each call (e.g., join or iterate over call.slice(0) / call) and assert that none of the arguments contain the sensitive substrings ("Failed to refresh saved accounts after first-run action", "alice") — apply the same change to the duplicate assertion block around the second instance (the other expect at 1235-1240) and ensure warnSpy.mock.calls is fully inspected for every call rather than only index 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/sync-history.ts`:
- Around line 290-308: The cached historyEntryCountEstimate is unsafe under
multi-writer conditions; after the initial load (loadHistoryEntriesFromDisk) and
after appending (fs.appendFile to paths.historyPath), revalidate the on-disk
state instead of trusting the in-memory estimate: check the file mtime/size or
re-read the file (parse lines via serializeEntry/deserialize) and compute the
current entry count, then call trimHistoryFileIfNeeded(paths) based on that
authoritative count; update historyEntryCountEstimate from the re-read value.
Also add a vitest that seeds the file, initializes historyEntryCountEstimate,
then simulates an out-of-band append and verifies a subsequent append triggers
revalidation and trimming. Ensure any new retry logic for trimming/appending
handles transient EBUSY/429 errors with exponential backoff and cite the
affected tests in the commit message.
In `@test/codex-manager-cli.test.ts`:
- Around line 1215-1220: The test currently stubs showFirstRunWizard which
prevents exercising the real wizard re-render path when
assessOpencodeAccountPool throws; add an additional deterministic Vitest case
that does NOT spy on auth-menu.showFirstRunWizard and instead drives the real UI
flow by using selectMock to simulate user choices for "import-opencode" followed
by "cancel", while keeping loadAccountsMock as before and making
assessOpencodeAccountPool throw to trigger the re-render; ensure the new test
unregisters any spies, uses the real showFirstRunWizard function, asserts the
re-render occurred (e.g., that showFirstRunWizard was invoked twice or that
expected UI prompts were emitted), and keeps the test deterministic with Vitest
utilities.
- Around line 68-74: flattenMockCallArgs currently uses String(arg) which turns
objects into "[object Object]" and can miss leaked file paths; update
flattenMockCallArgs to robustly serialize arguments by handling Error instances
as `${err.name}: ${err.message}`, serializing objects with a JSON.stringify
replacer that redacts Windows-style absolute paths (e.g. matching
/[A-Za-z]:\\[^"\s]*/ → "[REDACTED_PATH]") and guards against circular references
using a WeakSet, then join the serialized pieces with spaces so object fields
containing paths are detected and redacted during tests.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 4645-4668: The confirm() call before importing accounts can throw
(e.g., tty/readline failure) and currently sits outside the import error
handling so a thrown error will escape and exit the CLI; wrap the confirm()
invocation in the same error-handling flow (or add a surrounding try/catch) so
thrown errors are processed via
formatRedactedFilesystemError()/collapseWhitespace and the command returns to
the menu instead of exiting; update the block around confirm(),
runActionPanel(), and importAccounts() (symbols: confirm, runActionPanel,
importAccounts, formatRedactedFilesystemError, collapseWhitespace) to ensure
confirm errors are caught and handled the same way as import errors, and add a
vitest regression in test/codex-manager-cli.test.ts that forces confirm() to
throw and asserts the menu resumes.
- Around line 4371-4409: Add a vitest that simulates assessOpencodeAccountPool
throwing and verifies codex-manager handles it without exiting: stub/mock
assessOpencodeAccountPool to throw a filesystem error, stub
getRedactedFilesystemErrorLabel to return a predictable redacted string, invoke
the CLI path exercised by "import-opencode" (same flow exercised by
test/codex-manager-cli.test.ts), assert console.warn was called with the
redacted label returned by getRedactedFilesystemErrorLabel, and assert the
wizard/CLI loop did not exit (e.g. the command returned to prompt or continued
to the next loop iteration); reference functions assessOpencodeAccountPool,
getRedactedFilesystemErrorLabel, runActionPanel, and importAccounts when
locating code to stub and update the test to cover this regression.
In `@test/codex-manager-cli.test.ts`:
- Around line 1201-1209: The failing tests only inspect
warnSpy.mock.calls[i][0], so sensitive data in later args can still leak; update
the two assertions that currently check only call[0] (the
expect(...includes("Failed to refresh saved accounts...")) and the
expect(...every(call => !String(call[0]).includes("alice")))) to scan all
arguments of each call (e.g., join or iterate over call.slice(0) / call) and
assert that none of the arguments contain the sensitive substrings ("Failed to
refresh saved accounts after first-run action", "alice") — apply the same change
to the duplicate assertion block around the second instance (the other expect at
1235-1240) and ensure warnSpy.mock.calls is fully inspected for every call
rather than only index 0.
🪄 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: a04cfa8b-f5bc-4b9a-b891-12f06d6d6b28
📒 Files selected for processing (6)
docs/getting-started.mdlib/codex-manager.tslib/sync-history.tstest/codex-manager-cli.test.tstest/storage.test.tstest/sync-history.test.ts
💤 Files with no reviewable changes (1)
- test/storage.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 (3)
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/sync-history.tslib/codex-manager.ts
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/sync-history.test.tstest/codex-manager-cli.test.ts
docs/**
⚙️ CodeRabbit configuration file
keep README, SECURITY, and docs consistent with actual CLI flags and workflows. whenever behavior changes, require updated upgrade notes and mention new npm scripts.
Files:
docs/getting-started.md
| function flattenMockCallArgs(call: unknown[]): string { | ||
| return call | ||
| .map((arg) => | ||
| arg instanceof Error ? `${arg.name}: ${arg.message}` : String(arg), | ||
| ) | ||
| .join(" "); | ||
| } |
There was a problem hiding this comment.
redaction helper can hide leaked paths in object args.
in test/codex-manager-cli.test.ts:68-73, String(arg) turns objects into "[object Object]". if a logger call includes { error: "C:\\Users\\alice\\..." }, this helper will miss the leak.
proposed fix
+function serializeMockArg(arg: unknown): string {
+ if (arg instanceof Error) {
+ return `${arg.name}: ${arg.message}`;
+ }
+ if (typeof arg === "string") {
+ return arg;
+ }
+ try {
+ return JSON.stringify(arg);
+ } catch {
+ return String(arg);
+ }
+}
+
function flattenMockCallArgs(call: unknown[]): string {
return call
- .map((arg) =>
- arg instanceof Error ? `${arg.name}: ${arg.message}` : String(arg),
- )
+ .map((arg) => serializeMockArg(arg))
.join(" ");
}as per coding guidelines, test/**: 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.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function flattenMockCallArgs(call: unknown[]): string { | |
| return call | |
| .map((arg) => | |
| arg instanceof Error ? `${arg.name}: ${arg.message}` : String(arg), | |
| ) | |
| .join(" "); | |
| } | |
| function serializeMockArg(arg: unknown): string { | |
| if (arg instanceof Error) { | |
| return `${arg.name}: ${arg.message}`; | |
| } | |
| if (typeof arg === "string") { | |
| return arg; | |
| } | |
| try { | |
| return JSON.stringify(arg); | |
| } catch { | |
| return String(arg); | |
| } | |
| } | |
| function flattenMockCallArgs(call: unknown[]): string { | |
| return call | |
| .map((arg) => serializeMockArg(arg)) | |
| .join(" "); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-cli.test.ts` around lines 68 - 74, flattenMockCallArgs
currently uses String(arg) which turns objects into "[object Object]" and can
miss leaked file paths; update flattenMockCallArgs to robustly serialize
arguments by handling Error instances as `${err.name}: ${err.message}`,
serializing objects with a JSON.stringify replacer that redacts Windows-style
absolute paths (e.g. matching /[A-Za-z]:\\[^"\s]*/ → "[REDACTED_PATH]") and
guards against circular references using a WeakSet, then join the serialized
pieces with spaces so object fields containing paths are detected and redacted
during tests.
| const authMenu = await import("../lib/ui/auth-menu.js"); | ||
| const wizardSpy = vi | ||
| .spyOn(authMenu, "showFirstRunWizard") | ||
| .mockResolvedValueOnce({ type: "import-opencode" }) | ||
| .mockResolvedValueOnce({ type: "cancel" }); | ||
| loadAccountsMock.mockResolvedValue(null); |
There was a problem hiding this comment.
coverage gap: this test mocks away the real wizard-loop failure path.
in test/codex-manager-cli.test.ts:1215-1220, stubbing showFirstRunWizard means we do not exercise the actual first-run menu re-render when assessOpencodeAccountPool throws. add one deterministic case that uses the real wizard UI path (no spy on showFirstRunWizard) and drives import-opencode -> cancel via selectMock.
as per coding guidelines, test/**: 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.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/codex-manager-cli.test.ts` around lines 1215 - 1220, The test currently
stubs showFirstRunWizard which prevents exercising the real wizard re-render
path when assessOpencodeAccountPool throws; add an additional deterministic
Vitest case that does NOT spy on auth-menu.showFirstRunWizard and instead drives
the real UI flow by using selectMock to simulate user choices for
"import-opencode" followed by "cancel", while keeping loadAccountsMock as before
and making assessOpencodeAccountPool throw to trigger the re-render; ensure the
new test unregisters any spies, uses the real showFirstRunWizard function,
asserts the re-render occurred (e.g., that showFirstRunWizard was invoked twice
or that expected UI prompts were emitted), and keeps the test deterministic with
Vitest utilities.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
lib/sync-history.ts (1)
290-308:⚠️ Potential issue | 🟠 Majorcached entry count is still unsafe with out-of-band writers.
lib/sync-history.ts:290-308trustshistoryEntryCountEstimateafter first init and does not revalidate disk state before decidingshouldTrim. if another process appends/prunessync-history.ndjson, this process can skip trimming for too long and let history grow past policy. please revalidate on-disk state (mtime/size or bounded reread) before trim gating, and add a deterministic vitest that simulates out-of-band append after estimate warm-up.As per coding guidelines,
lib/**: 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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/sync-history.ts` around lines 290 - 308, The code uses historyEntryCountEstimate (warmed via loadHistoryEntriesFromDisk) and then decides shouldTrim without revalidating on-disk state; change the logic around the append+trim decision (the block that writes serializeEntry(entry) to paths.historyPath and computes shouldTrim) to revalidate the file before gating trim by either checking the file mtime/size or performing a bounded reread of the file tail to update historyEntryCountEstimate (or compute a safe upper bound) and only then call trimHistoryFileIfNeeded(paths); also ensure trimHistoryFileIfNeeded still works correctly when called concurrently and keep loadHistoryEntriesFromDisk and serializeEntry usage intact. Add a vitest that warms the estimate, performs an out-of-band append to paths.historyPath (simulating another process), then runs the code path that triggers trimming to confirm trimming executes deterministically; include the new test alongside existing sync-history tests and handle EBUSY-like file contention in the test setup without leaking tokens/emails in logs.lib/codex-manager.ts (2)
4850-4878: 🧹 Nitpick | 🔵 Trivialremove the unreachable first-run replay branch.
lib/codex-manager.ts:4477(Line 4477) setsfirstRunWizardShownInLoop = trueon the onlystartedFromMissingStoragepath, solib/codex-manager.ts:4850(Line 4850) guard cannot pass. this is dead control flow and makes startup logic harder to reason about.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/codex-manager.ts` around lines 4850 - 4878, The if-block guarded by startedFromMissingStorage && !firstRunWizardShownInLoop && existingCount === 0 && isInteractiveLoginMenuAvailable() (the block that calls loadDashboardDisplaySettings, runFirstRunWizard, and attempts refreshedAfterWizard) is unreachable because firstRunWizardShownInLoop is already set true on the only startedFromMissingStorage path; remove this entire unreachable branch and its inner logic, and also remove or simplify any now-unused references to firstRunWizardShownInLoop (or related cleanup code) so startup flow no longer contains dead control flow around runFirstRunWizard and refreshedAfterWizard handling.
4316-4341:⚠️ Potential issue | 🟠 Majoravoid double opencode probes and add transient fs retry in wizard path.
lib/codex-manager.ts:4336(Line 4336) probes withassessOpencodeAccountPool()during menu option build, thenlib/codex-manager.ts:4374(Line 4374) probes again whenimport-opencodeis selected. this doubles windows fs contention and still has no bounded retry/backoff for transientEBUSY/sharing-lock failures. it can also hide the import action when the pre-probe throws andhasOpencodeSourcestays false.proposed direction
+async function withTransientFsRetry<T>(op: () => Promise<T>, attempts = 3): Promise<T> { + let delayMs = 75; + for (let i = 0; i < attempts; i += 1) { + try { + return await op(); + } catch (error) { + const code = (error as { code?: string })?.code; + const transient = code === "EBUSY" || code === "EPERM" || code === "EMFILE"; + if (!transient || i === attempts - 1) throw error; + await new Promise((resolve) => setTimeout(resolve, delayMs)); + delayMs *= 2; + } + } + throw new Error("unreachable"); +}- hasOpencodeSource = (await assessOpencodeAccountPool()) !== null; + hasOpencodeSource = (await withTransientFsRetry(() => assessOpencodeAccountPool())) !== null;- assessment = await assessOpencodeAccountPool(); + assessment = await withTransientFsRetry(() => assessOpencodeAccountPool());As per coding guidelines, "
lib/**: 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."Also applies to: 4371-4418
test/codex-manager-cli.test.ts (3)
1241-1249:⚠️ Potential issue | 🟠 Majorassert redaction against the full logger payload, not only
call[0].at
test/codex-manager-cli.test.ts:1241,test/codex-manager-cli.test.ts:1279,test/codex-manager-cli.test.ts:1338, andtest/codex-manager-cli.test.ts:4246, checks still miss secondary args and object args. aconsole.warn("...", err)call can leak full windows paths while these tests pass.proposed fix
- expect( - warnSpy.mock.calls.every((call) => !String(call[0]).includes("alice")), - ).toBe(true); + expect( + warnSpy.mock.calls.every( + (call) => !flattenMockCallArgs(call).includes("alice"), + ), + ).toBe(true); - const warningOutput = warnSpy.mock.calls.flat().join("\n"); + const warningOutput = warnSpy.mock.calls + .map((call) => flattenMockCallArgs(call)) + .join("\n");as per coding guidelines,
test/**: 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.Also applies to: 1279-1280, 1338-1343, 4246-4255
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 1241 - 1249, The assertions currently inspect only the first argument of warnSpy.mock.calls (call[0]) which misses secondary args or object args that may leak sensitive data; update the checks (those using warnSpy.mock.calls.some(...) and .every(...)) to inspect the entire call payload (e.g., stringify or join all call elements such as using JSON.stringify(call) or call.map(String).join(" ")) and then assert the presence/absence of the target text (e.g., "Failed to refresh saved accounts after first-run action" and that "alice" is not present) against that full payload; apply the same change to the other occurrences referenced in this test file so each assertion covers all arguments in warnSpy.mock.calls.
68-74:⚠️ Potential issue | 🟠 Majorserialize mock args deeply for redaction checks.
String(arg)hides structured payloads as"[object Object]", so path leaks in object fields can bypass checks intest/codex-manager-cli.test.ts:68. this weakens windows redaction coverage.proposed fix
+function serializeMockArg(arg: unknown): string { + if (arg instanceof Error) return `${arg.name}: ${arg.message}`; + if (typeof arg === "string") return arg; + const seen = new WeakSet<object>(); + const redactWindowsPath = (value: string) => + value.replace(/[A-Za-z]:\\[^"'\s)]+/g, "[REDACTED_PATH]"); + try { + return JSON.stringify(arg, (_key, value) => { + if (typeof value === "string") return redactWindowsPath(value); + if (value && typeof value === "object") { + if (seen.has(value as object)) return "[Circular]"; + seen.add(value as object); + } + return value; + }); + } catch { + return String(arg); + } +} + function flattenMockCallArgs(call: unknown[]): string { return call - .map((arg) => - arg instanceof Error ? `${arg.name}: ${arg.message}` : String(arg), - ) + .map((arg) => serializeMockArg(arg)) .join(" "); }as per coding guidelines,
test/**: 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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 68 - 74, The helper flattenMockCallArgs currently uses String(arg) which collapses objects to "[object Object]" and hides fields; update flattenMockCallArgs to serialize arguments deeply so object fields are visible for redaction checks: in the flattenMockCallArgs function, detect Error instances (keep `${err.name}: ${err.message}`) and for other values use a deep serializer (e.g., JSON.stringify with a circular-safe replacer or Node's util.inspect) to produce full object payloads (falling back to String(arg) if serialization fails) so tests in test/codex-manager-cli.test.ts can assert on nested paths instead of being bypassed by "[object Object]".
1253-1260:⚠️ Potential issue | 🟡 Minordo not mock away the wizard re-render path in this probe-failure regression.
test/codex-manager-cli.test.ts:1253stubsshowFirstRunWizard, so it does not exercise the real first-run render/re-render flow when OpenCode probing fails. that leaves the windowsEBUSYwizard-loop behavior under-tested.as per coding guidelines,
test/**: 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.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/codex-manager-cli.test.ts` around lines 1253 - 1260, The test should not stub out the real first-run wizard flow: remove the mockResolvedValueOnce stubs on authMenu.showFirstRunWizard so the actual showFirstRunWizard implementation runs (or replace the stub with a call-through). Concretely, in the test remove the .mockResolvedValueOnce(...) calls on the spy for showFirstRunWizard (or replace them with a call-through wrapper that invokes the original function: capture const orig = authMenu.showFirstRunWizard and use vi.spyOn(authMenu, "showFirstRunWizard").mockImplementation((...a) => orig.apply(authMenu, a))). This ensures the real render/re-render path and EBUSY windows behavior are exercised while still letting you observe the call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/sync-history.ts`:
- Around line 293-309: The production write paths can fail on transient Windows
file-lock errors; wrap the append/rewrite/trim write operations in bounded
retry+exponential backoff (same behavior as __resetSyncHistoryForTests): up to 5
attempts with delay 25 * 2^attempt ms, and only retry for transient error codes
(EBUSY/EPERM). Apply this retry logic around the fs.appendFile call in
appendSyncHistoryEntry, around the file-writes inside trimHistoryFileIfNeeded,
and around rewriteLatestEntry so each physical write is retried before failing.
Add a Vitest that stubs/mock fs to throw an EBUSY/EPERM on the first N-1
attempts and succeed on the final attempt to verify recovery.
---
Duplicate comments:
In `@lib/codex-manager.ts`:
- Around line 4850-4878: The if-block guarded by startedFromMissingStorage &&
!firstRunWizardShownInLoop && existingCount === 0 &&
isInteractiveLoginMenuAvailable() (the block that calls
loadDashboardDisplaySettings, runFirstRunWizard, and attempts
refreshedAfterWizard) is unreachable because firstRunWizardShownInLoop is
already set true on the only startedFromMissingStorage path; remove this entire
unreachable branch and its inner logic, and also remove or simplify any
now-unused references to firstRunWizardShownInLoop (or related cleanup code) so
startup flow no longer contains dead control flow around runFirstRunWizard and
refreshedAfterWizard handling.
In `@lib/sync-history.ts`:
- Around line 290-308: The code uses historyEntryCountEstimate (warmed via
loadHistoryEntriesFromDisk) and then decides shouldTrim without revalidating
on-disk state; change the logic around the append+trim decision (the block that
writes serializeEntry(entry) to paths.historyPath and computes shouldTrim) to
revalidate the file before gating trim by either checking the file mtime/size or
performing a bounded reread of the file tail to update historyEntryCountEstimate
(or compute a safe upper bound) and only then call
trimHistoryFileIfNeeded(paths); also ensure trimHistoryFileIfNeeded still works
correctly when called concurrently and keep loadHistoryEntriesFromDisk and
serializeEntry usage intact. Add a vitest that warms the estimate, performs an
out-of-band append to paths.historyPath (simulating another process), then runs
the code path that triggers trimming to confirm trimming executes
deterministically; include the new test alongside existing sync-history tests
and handle EBUSY-like file contention in the test setup without leaking
tokens/emails in logs.
In `@test/codex-manager-cli.test.ts`:
- Around line 1241-1249: The assertions currently inspect only the first
argument of warnSpy.mock.calls (call[0]) which misses secondary args or object
args that may leak sensitive data; update the checks (those using
warnSpy.mock.calls.some(...) and .every(...)) to inspect the entire call payload
(e.g., stringify or join all call elements such as using JSON.stringify(call) or
call.map(String).join(" ")) and then assert the presence/absence of the target
text (e.g., "Failed to refresh saved accounts after first-run action" and that
"alice" is not present) against that full payload; apply the same change to the
other occurrences referenced in this test file so each assertion covers all
arguments in warnSpy.mock.calls.
- Around line 68-74: The helper flattenMockCallArgs currently uses String(arg)
which collapses objects to "[object Object]" and hides fields; update
flattenMockCallArgs to serialize arguments deeply so object fields are visible
for redaction checks: in the flattenMockCallArgs function, detect Error
instances (keep `${err.name}: ${err.message}`) and for other values use a deep
serializer (e.g., JSON.stringify with a circular-safe replacer or Node's
util.inspect) to produce full object payloads (falling back to String(arg) if
serialization fails) so tests in test/codex-manager-cli.test.ts can assert on
nested paths instead of being bypassed by "[object Object]".
- Around line 1253-1260: The test should not stub out the real first-run wizard
flow: remove the mockResolvedValueOnce stubs on authMenu.showFirstRunWizard so
the actual showFirstRunWizard implementation runs (or replace the stub with a
call-through). Concretely, in the test remove the .mockResolvedValueOnce(...)
calls on the spy for showFirstRunWizard (or replace them with a call-through
wrapper that invokes the original function: capture const orig =
authMenu.showFirstRunWizard and use vi.spyOn(authMenu,
"showFirstRunWizard").mockImplementation((...a) => orig.apply(authMenu, a))).
This ensures the real render/re-render path and EBUSY windows behavior are
exercised while still letting you observe the call.
🪄 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: f5845167-2d5c-4391-8861-15a986edc805
📒 Files selected for processing (4)
lib/codex-manager.tslib/sync-history.tstest/codex-manager-cli.test.tstest/sync-history.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/sync-history.test.tstest/codex-manager-cli.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.tslib/sync-history.ts
🔇 Additional comments (3)
test/sync-history.test.ts (1)
271-371: good deterministic coverage for estimate lifecycle paths.
test/sync-history.test.ts:271-371is solid vitest coverage for the new append-estimate behavior and reset semantics.lib/codex-manager.ts (2)
1286-1286: good redaction hardening on failure output.
lib/codex-manager.ts:1286(Line 1286) andlib/codex-manager.ts:4679(Line 4679) now normalize and redact filesystem error details before surfacing them. this is the right direction for safe cli diagnostics.Also applies to: 4679-4681
4466-4497: tests exist for first-run wizard gating and EBUSY transient fallback, but verify via behavior rather than direct variable assertions.tests at
test/codex-manager-cli.test.ts:930andtest/codex-manager-cli.test.ts:1253verify that the first-run wizard is shown once when starting from missing storage and remains open when OpenCode import probing hits EBUSY (wizard called twice, second showing after failure). test attest/codex-manager-cli.test.ts:1448verifies the recovery menu appears after first-run wizard creates accounts.the implementation at
lib/codex-manager.ts:4466-4497usesskipEmptyStorageRecoveryMenuandfirstRunWizardShownInLoopto control these flows, and existing tests validate the behavior end-to-end through mock sequencing (e.g.,wizardSpy.toHaveBeenCalledTimes(2)after EBUSY) rather than asserting on variable state directly. this approach covers the transient EBUSY scenarios referenced in the coding guidelines and verifies recovery paths remain unbroken.
| historyEntryCountEstimate = (await loadHistoryEntriesFromDisk(paths)).length; | ||
| } |
There was a problem hiding this comment.
the loadHistoryEntriesFromDisk call for count estimation is not wrapped in any retry logic. withRetryableHistoryWrite guards all the write paths (appendFile, writeFile, rm), but the preceding read that seeds historyEntryCountEstimate when it is null (e.g. right after configureSyncHistoryForTests) has no equivalent protection. on Windows, antivirus/backup software can return EBUSY or EPERM on a readFile call to a newly active file, which causes:
historyEntryCountEstimatestaysnull- the exception propagates out of the lock before
appendFileis ever reached - the calling entry is silently not written to disk
historyEntryCountEstimateremainsnullon the next call, which re-tries the read – the previously missed entry is gone for good
withRetryableHistoryWrite works for writes; a withRetryableHistoryRead equivalent (or at minimum a small inline retry loop with the same RETRYABLE_WRITE_CODES set) should wrap this call. there is no vitest case that mocks fs.readFile to throw EBUSY during the initial estimation phase and then asserts the entry is still appended.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/sync-history.ts
Line: 321-322
Comment:
**unretried count-estimation read drops entries on Windows EBUSY**
the `loadHistoryEntriesFromDisk` call for count estimation is not wrapped in any retry logic. `withRetryableHistoryWrite` guards all the write paths (appendFile, writeFile, rm), but the preceding read that seeds `historyEntryCountEstimate` when it is `null` (e.g. right after `configureSyncHistoryForTests`) has no equivalent protection. on Windows, antivirus/backup software can return `EBUSY` or `EPERM` on a `readFile` call to a newly active file, which causes:
1. `historyEntryCountEstimate` stays `null`
2. the exception propagates out of the lock before `appendFile` is ever reached
3. the calling entry is silently not written to disk
4. `historyEntryCountEstimate` remains `null` on the next call, which re-tries the read – the previously missed entry is gone for good
`withRetryableHistoryWrite` works for writes; a `withRetryableHistoryRead` equivalent (or at minimum a small inline retry loop with the same `RETRYABLE_WRITE_CODES` set) should wrap this call. there is no vitest case that mocks `fs.readFile` to throw `EBUSY` during the initial estimation phase and then asserts the entry is still appended.
How can I resolve this? If you propose a fix, please make it concise.
| export function pruneSyncHistoryEntries( | ||
| entries: SyncHistoryEntry[], | ||
| maxEntries: number = MAX_HISTORY_ENTRIES, | ||
| ): PrunedSyncHistory { | ||
| if (entries.length === 0) { | ||
| return { entries: [], removed: 0, latest: null }; | ||
| } | ||
|
|
||
| const boundedMaxEntries = Math.max(0, maxEntries); | ||
| const latestByKind = new Map<SyncHistoryKind, SyncHistoryEntry>(); | ||
| for (let index = entries.length - 1; index >= 0; index -= 1) { | ||
| const entry = entries[index]; | ||
| if (!entry || latestByKind.has(entry.kind)) { | ||
| continue; | ||
| } | ||
| latestByKind.set(entry.kind, entry); | ||
| } | ||
|
|
There was a problem hiding this comment.
seenEntries reference-equality check is effectively dead code
seenEntries is built with new Set<SyncHistoryEntry>() and used via seenEntries.has(entry). every entry in the entries array comes from parseHistoryContent → parseEntry(line) → cloneEntry(parsed), so each is a freshly allocated object with a unique reference. seenEntries.has(entry) therefore never returns true during the loop iteration, and seenEntries never prevents any entry from being re-added.
the practical consequence is that if a crash-retry path appended the same JSON line twice to the NDJSON file, pruneSyncHistoryEntries would not deduplicate them – both copies survive the prune. if content-based deduplication is the goal, the set should key on a serialized form (e.g. JSON.stringify(entry)). if the only intent is to guard against the same object reference appearing twice in the array (impossible with parsed results), the seenEntries machinery can be removed entirely to avoid misleading future readers.
Prompt To Fix With AI
This is a comment left during a code review.
Path: lib/sync-history.ts
Line: 224-241
Comment:
**`seenEntries` reference-equality check is effectively dead code**
`seenEntries` is built with `new Set<SyncHistoryEntry>()` and used via `seenEntries.has(entry)`. every entry in the `entries` array comes from `parseHistoryContent` → `parseEntry(line)` → `cloneEntry(parsed)`, so each is a freshly allocated object with a unique reference. `seenEntries.has(entry)` therefore never returns `true` during the loop iteration, and `seenEntries` never prevents any entry from being re-added.
the practical consequence is that if a crash-retry path appended the same JSON line twice to the NDJSON file, `pruneSyncHistoryEntries` would not deduplicate them – both copies survive the prune. if content-based deduplication is the goal, the set should key on a serialized form (e.g. `JSON.stringify(entry)`). if the only intent is to guard against the same *object reference* appearing twice in the array (impossible with parsed results), the `seenEntries` machinery can be removed entirely to avoid misleading future readers.
How can I resolve this? If you propose a fix, please make it concise.
|
Superseded by clean replacement #129. This branch was rebuilt from the fresh restack audit onto |
Summary
fresh/18-import-adapter-opencode#832e22874What Changed
0vsnullreset semantics used by the new append optimization.Validation
npm run lintnpm run typechecknpm run buildnpm testnpm test -- test/codex-manager-cli.test.ts -t "imports OpenCode accounts from the first-run wizard|returns to the dashboard when OpenCode import fails|returns to the dashboard when OpenCode import assessment fails|redacts first-run backup listing warnings|falls back to a safe health summary when restore or rollback state reads fail"npm test -- test/sync-history.test.ts -t "skips trim reads while append count stays within the default cap|reloads and trims once when append count crosses the default cap|re-reads seeded history after configureSyncHistoryForTests resets the estimate to null|preserves the latest entry per kind when appends exceed the default cap"Docs and Governance Checklist
docs/getting-started.mdupdated (if onboarding flow changed)docs/features.mdupdated (if capability surface changed)docs/reference/*pages updated (if commands/settings/paths changed)docs/upgrade.mdupdated (if migration behavior changed)SECURITY.mdandCONTRIBUTING.mdreviewed for alignmentRisk and Rollback
ac2be34,16ba251,64a8ead,f5b74dc, and2e22874Additional Notes
fresh/18-import-adapter-opencode, notmain.2e22874.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 lands the first-run setup wizard (restore, opencode import, settings, doctor entry points) on top of the
fresh/18-import-adapter-opencodestack, and introduces a newsync-history.tsmodule with an append-optimized NDJSON history store. the wizard logic incodex-manager.tsis well-structured: assessment and import arms are in separatetry/catchblocks, the doctor case is guarded,loadAccountsis wrapped after every wizard action, and path redaction is applied consistently throughout. thefirstRunWizardShownInLoopflag correctly prevents double-firing.sync-history.ts:loadHistoryEntriesFromDiskis called unretried to seedhistoryEntryCountEstimatewhen it isnull. all write paths usewithRetryableHistoryWrite, but this read has no equivalent protection. on Windows, antivirus can returnEBUSYonreadFile, causing the entry to be silently lost beforeappendFileis ever reached. no vitest case covers this failure path.pruneSyncHistoryEntries:seenEntriescompares object references, but every entry fromloadHistoryEntriesFromDiskis a freshly allocated parse result.seenEntries.has(entry)is alwaysfalse, so crash-duplicated JSON lines in the NDJSON file survive pruning unchanged.loadAccounts()inside wizard'sassessOpencodeAccountPoolcall: bothbuildFirstRunWizardOptionsand theimport-opencodearm callassessOpencodeAccountPool()without passingcurrentStorage: null. the function then falls back toawait loadAccounts()internally, which is a second unnecessary disk read in the first-run context (we already know storage isnull). on Windows, this extra read is an additionalEBUSYsurface.sync-history.tstest file is solid: trim-skip threshold, reload-on-cap-cross, externally-cleared-file reset, and write-retry paths are all covered. the one gap is theEBUSYfailure on the initial count-estimationreadFile.Confidence Score: 3/5
Important Files Changed
Sequence Diagram
sequenceDiagram participant CLI as runAuthLogin participant W as runFirstRunWizard participant B as buildFirstRunWizardOptions participant S as storage.ts participant H as sync-history.ts CLI->>S: loadAccounts() S-->>CLI: null (first-run) CLI->>W: runFirstRunWizard(displaySettings) loop wizard loop W->>B: buildFirstRunWizardOptions() B->>S: listNamedBackups() B->>S: listRotatingBackups() B->>S: assessOpencodeAccountPool()<br/>[calls loadAccounts() internally — redundant] B-->>W: {namedBackupCount, hasOpencodeSource, …} W->>W: showFirstRunWizard(options) alt action = import-opencode W->>S: assessOpencodeAccountPool()<br/>[calls loadAccounts() internally — redundant] W->>S: importAccounts(path) [in runActionPanel] else action = restore W->>S: runBackupBrowserManager [unguarded] else action = doctor W->>W: runActionPanel → runDoctor [guarded try/catch] else action = settings W->>W: configureUnifiedSettings [unguarded] else action = login / skip W-->>CLI: {outcome: continue} else action = cancel W-->>CLI: {outcome: cancelled} end W->>S: loadAccounts() [guarded try/catch] note over W,S: returns if accounts.length > 0 end CLI->>H: appendSyncHistoryEntry(entry) note over H: if historyEntryCountEstimate===null<br/>→ loadHistoryEntriesFromDisk()<br/>[NO retry — EBUSY risk on Windows] H->>H: withRetryableHistoryWrite → appendFile [retried] H->>H: trimHistoryFileIfNeeded [if estimate > 200]Prompt To Fix All With AI
Last reviewed commit: "fix(sync): retry syn..."