Skip to content

Commit e453111

Browse files
ndycodendycode
committed
fix: resolve post-merge integration issues across 34 audit PRs
- fix: restore ensureFirstRunSetup wiring in codex-manager.ts (lost during audit-29 extraction) - fix: update ui/copy.js -> ui/ui-copy.js imports in extracted login modules (audit-17 rename) - fix: convert ci.yml to LF line endings so extractJobBlock regex works in tests - fix: update documentation test to expect config.schema.json gitattributes entry (audit-16) - fix: update codex-manager-cli.test.ts quota cache mock to use mockImplementation - fix: add options param and waitFor to stale-refresh test assertions (audit-32) - fix: merge fs-retry.test.ts conflict: combine shouldRetryFileOperation suite from PR #516 with comprehensive withRetry/withRetrySync suite from audit-07
1 parent 84b3385 commit e453111

6 files changed

Lines changed: 278 additions & 5 deletions

File tree

‎.bugsweep-index.md‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,118 @@
1+
# Bug Sweep: index.ts / lib/cli.ts / lib/index.ts
2+
3+
Scope: HIGH-confidence correctness defects only. `lib/index.ts` is pure re-exports (clean).
4+
5+
---
6+
7+
## 1. `index.ts:3163-3214` — "Set as current account" is silently ignored in the plugin login flow
8+
9+
**Severity: HIGH | Confidence: HIGH**
10+
11+
The `promptLoginMode` menu in `lib/cli.ts` can return a manage action carrying `switchAccountIndex` (from the "set as current" / "set-current-account" menu choices):
12+
13+
```ts
14+
// lib/cli.ts:237-241 and 257-260
15+
if (accountAction === "set-current") {
16+
const index = resolveAccountSourceIndex(action.account);
17+
if (index >= 0) return { mode: "manage", switchAccountIndex: index };
18+
...
19+
}
20+
...
21+
case "set-current-account": {
22+
const index = resolveAccountSourceIndex(action.account);
23+
if (index >= 0) return { mode: "manage", switchAccountIndex: index };
24+
...
25+
}
26+
```
27+
28+
But the `mode === "manage"` handler in `index.ts` only inspects `deleteAccountIndex`, `toggleAccountIndex`, and `refreshAccountIndex`:
29+
30+
```ts
31+
// index.ts:3163-3214
32+
if (menuResult.mode === "manage") {
33+
if (typeof menuResult.deleteAccountIndex === "number") { ... continue; }
34+
if (typeof menuResult.toggleAccountIndex === "number") { ... continue; }
35+
if (typeof menuResult.refreshAccountIndex === "number") {
36+
refreshAccountIndex = menuResult.refreshAccountIndex;
37+
startFresh = false;
38+
break;
39+
}
40+
continue; // <-- switchAccountIndex falls through to here and is dropped
41+
}
42+
```
43+
44+
There is **no** `switchAccountIndex` branch. (The only consumer of `switchAccountIndex` is `lib/codex-manager.ts:2713`, a different, standalone CLI path — not this plugin login loop.)
45+
46+
**Triggering input -> actual vs expected:** User runs `codex-multi-auth login`, opens an account's detail view, and chooses "Set as current". Expected: that account becomes the active account (`storage.activeIndex` / `activeIndexByFamily` updated and persisted). Actual: the manage block matches none of the three handled indexes, hits the trailing `continue`, and the menu simply re-renders. The active account is never changed and nothing is persisted — the action is a no-op.
47+
48+
**Suggested fix:** Add a handler in the manage block, e.g.:
49+
```ts
50+
if (typeof menuResult.switchAccountIndex === "number") {
51+
const target = workingStorage.accounts[menuResult.switchAccountIndex];
52+
if (target) {
53+
const now = Date.now();
54+
target.lastUsed = now;
55+
target.lastSwitchReason = "rotation";
56+
workingStorage.activeIndex = menuResult.switchAccountIndex;
57+
workingStorage.activeIndexByFamily = workingStorage.activeIndexByFamily ?? {};
58+
for (const family of MODEL_FAMILIES) {
59+
workingStorage.activeIndexByFamily[family] = menuResult.switchAccountIndex;
60+
}
61+
await saveAccounts(workingStorage);
62+
invalidateRuntimeAccountManagerCache(accountManagerCacheInvalidationDeps);
63+
}
64+
continue;
65+
}
66+
```
67+
68+
---
69+
70+
## 2. `index.ts:1203-1419` — `attempted` set and `accountCount` go stale after mid-loop `removeAccount`, causing account-traversal desync
71+
72+
**Severity: MEDIUM | Confidence: MEDIUM**
73+
74+
The account-attempt loop captures the pool size and tracks attempts by numeric index, captured/created once per outer iteration:
75+
76+
```ts
77+
// index.ts:1204-1205
78+
const accountCount = accountManager.getAccountCount();
79+
const attempted = new Set<number>();
80+
...
81+
accountAttemptLoop: while (attempted.size < Math.max(1, accountCount)) {
82+
...
83+
attempted.add(account.index);
84+
```
85+
86+
Inside this loop, on repeated auth-refresh failure the account is removed, which **reindexes** every remaining account (`acc.index = index` after splice — see `lib/accounts.ts:1540-1590`):
87+
88+
```ts
89+
// index.ts:1392-1404
90+
if (authFailurePolicy.removeAccount) {
91+
const removedIndex = account.index;
92+
sessionAffinityStore?.forgetAccount(removedIndex);
93+
accountManager.removeAccount(account); // shifts indices of all higher accounts down by 1
94+
sessionAffinityStore?.reindexAfterRemoval(removedIndex);
95+
...
96+
continue; // back to accountAttemptLoop with stale `attempted` + `accountCount`
97+
}
98+
```
99+
100+
**Wrong behavior:** After a removal, `attempted` still holds pre-removal indices. Every account that was above the removed slot now occupies index N-1, so the indices stored in `attempted` no longer identify the same accounts. An account that was already tried can be selected again under its new (not-yet-in-`attempted`) index, and `accountCount` is now one larger than the real pool, so the `attempted.size < accountCount` guard can permit extra/duplicate attempts (or, conversely, mark a fresh account as "attempted"). The session-affinity reindex is handled, but the local `attempted`/`accountCount` traversal state is not. Net effect: the rotation can re-hit a known-bad account and/or mis-count remaining candidates after any in-loop account removal.
101+
102+
**Suggested fix:** After `removeAccount`, re-derive traversal state — clear/rebuild `attempted` (it is keyed by a now-invalid index space) and recompute `accountCount` from `accountManager.getAccountCount()` before continuing, or track attempts by a stable identity key (accountId/refreshToken) instead of numeric index.
103+
104+
---
105+
106+
## Notes / inspected-but-not-reported
107+
108+
- `index.ts:2807` `if (response.status >= 500)` inside the empty-response retry block is effectively dead (this branch is only reached after `response.ok` was true), but it is not a correctness defect with wrong output — omitted.
109+
- `index.ts:3438-3454` add-account loop uses local `accounts.length` (which also grows on duplicate logins) for slot/limit guards; behavior is benign (duplicates merge), so not reported as a high-confidence bug.
110+
- `lib/cli.ts` arg/index parsing (`resolveAccountSourceIndex`, `promptAccountSelection` bounds, `Number.parseInt` guards) reviewed — correct.
111+
- `index.ts` `codex-switch` / `codex-remove` index math and `activeIndex` reindex-after-removal (4405-4427) reviewed — correct.
112+
113+
---
114+
115+
## Count by severity
116+
- HIGH: 1
117+
- MEDIUM: 1
118+
- LOW: 0

‎.bugsweep-transform.md‎

Lines changed: 129 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,129 @@
1+
# Bug Sweep: request transform / response handler / adapters
2+
3+
Scope reviewed:
4+
- lib/request/request-transformer.ts
5+
- lib/request/response-handler.ts
6+
- lib/request/fetch-helpers.ts
7+
- lib/request/helpers/{tool-utils,input-utils,model-map}.ts
8+
- lib/oc-chatgpt-import-adapter.ts
9+
- lib/oc-chatgpt-orchestrator.ts
10+
- lib/oc-chatgpt-target-detection.ts
11+
- lib/prompts/codex.ts
12+
13+
---
14+
15+
## Finding 1 — `trimInputForFastSession` discards the leading developer/system context it deliberately preserved
16+
17+
**File:** lib/request/request-transformer.ts:646-650
18+
19+
**Buggy code:**
20+
```ts
21+
const trimmed = input.filter((_item, index) => keepIndexes.has(index));
22+
if (trimmed.length === 0) return input;
23+
if (input.length <= maxItems && excludedHeadIndexes.size === 0) return input;
24+
if (trimmed.length <= safeMax) return trimmed;
25+
return trimmed.slice(trimmed.length - safeMax);
26+
```
27+
28+
**Why it's wrong:**
29+
Earlier in the function the head loop (lines 623-639) deliberately adds the first one or two
30+
short `developer`/`system` items to `keepIndexes`, and the tail loop (lines 641-644) adds the
31+
last `safeMax` items (`safeMax = Math.max(8, Math.floor(maxItems))`). Because `trimmed` is built
32+
via `input.filter(...has(index))`, the preserved head items appear FIRST in `trimmed`.
33+
34+
When the conversation is longer than `safeMax`, the head indices (0/1) are distinct from the tail
35+
range (`input.length - safeMax .. end`), so `trimmed.length` becomes `safeMax + 1` or `safeMax + 2`.
36+
The final `trimmed.slice(trimmed.length - safeMax)` then keeps only the last `safeMax` entries —
37+
which are exactly the tail items — and slices the leading head items off the front.
38+
39+
The function's own docstring states: "Keeps a small leading developer/system context plus the most
40+
recent items." The slice undoes that for any history longer than `safeMax`.
41+
42+
**Triggering input -> actual vs expected:**
43+
- Input: 50 items, `maxItems = 30` (`safeMax = 30`), item 0 = short `developer` instruction,
44+
`preferLatestUserOnly = false` (non-trivial turn — the common multi-turn fast-session case).
45+
- keepIndexes = {0, 1, 20..49} -> trimmed.length = 32 -> `trimmed.slice(2)` = items[20..49].
46+
- Actual: the leading developer/system instruction (items 0/1) is dropped.
47+
- Expected: leading developer/system context retained alongside the most recent items.
48+
49+
Reachable in production: `resolveFastSessionInputTrimPlan` only sets `preferLatestUserOnly=true`
50+
for trivial single-line turns (which return early before this slice). Non-trivial fast-session
51+
turns hit this path with `preferLatestUserOnly=false`.
52+
53+
**Severity:** MEDIUM (degraded prompt context in fast-session mode; non-host project/developer
54+
instructions are stripped from the request).
55+
**Confidence:** HIGH (deterministic; the code adds the head indices then unconditionally removes them).
56+
57+
**Suggested fix:** Reserve room for the head when slicing, e.g. partition `trimmed` into head vs
58+
tail and cap only the tail, or compute the slice as
59+
`[...headItems, ...tailItems.slice(tailItems.length - (safeMax - headItems.length))]`, so the
60+
preserved leading context is never sliced away.
61+
62+
---
63+
64+
## Finding 2 — `response.output_text.done` (and reasoning-summary `.done`) read text via `getStringField`, which can wipe accumulated deltas on an empty/whitespace final payload
65+
66+
**File:** lib/request/response-handler.ts:630-639 (and 670-677 for reasoning summary)
67+
68+
**Buggy code:**
69+
```ts
70+
if (data.type === "response.output_text.done") {
71+
setOutputTextValue(
72+
state,
73+
outputIndex,
74+
getNumberField(eventRecord, "content_index"),
75+
getStringField(eventRecord, "text"), // <-- trimmed/non-empty gate
76+
eventRecord.phase,
77+
);
78+
return;
79+
}
80+
```
81+
`getStringField` returns `null` when the value is empty or whitespace-only
82+
(`value.trim().length > 0 ? value : null`). The file's own doc comment (lines 54-59) explicitly
83+
warns: "For textual payloads where whitespace is meaningful, use a field-specific accessor such as
84+
`getDeltaField` instead of reusing this helper." The `.delta` handlers correctly use `getDeltaField`,
85+
but the `.done` handlers use `getStringField`.
86+
87+
**Why it's wrong / wrong behavior:**
88+
`setOutputTextValue(..., null, ...)` deletes the accumulated key:
89+
```ts
90+
if (!text) {
91+
state.outputText.delete(key); // line 262-265
92+
setPhaseTextSegment(state, phase, key, null);
93+
return;
94+
}
95+
```
96+
So if deltas accumulated content (e.g. "Hello") and a terminal `response.output_text.done` arrives
97+
with an empty or whitespace-only `text`, the accumulated text for that `output:content` key is
98+
deleted instead of finalized, dropping it from the synthesized final response.
99+
100+
**Triggering input -> actual vs expected:**
101+
- Stream: `output_text.delta` "Hello world", then `output_text.done` with `text: ""` (or `" "`).
102+
- Actual: accumulated "Hello world" is deleted; final JSON loses that content part's text.
103+
- Expected: the accumulated delta text is preserved as the final value.
104+
105+
**Severity:** LOW (the OpenAI Responses API normally carries the full text on `.done`; an empty/
106+
whitespace `.done` is the edge that triggers loss).
107+
**Confidence:** MEDIUM (depends on upstream emitting an empty terminal text event).
108+
109+
**Suggested fix:** Use `getDeltaField` (length>0 only, no trim) for the `.done` text fields, and/or
110+
guard `setOutputTextValue` so an empty `.done` does not delete already-accumulated delta text.
111+
112+
---
113+
114+
## Notes / examined and considered correct
115+
116+
- `getModelConfig` variant parsing, `coerceReasoningEffort` fallback tables, and `resolveInclude`
117+
(always re-adds `reasoning.encrypted_content`) behave as documented.
118+
- Model-family mapping in model-map.ts (codex aliases -> gpt-5.3-codex; gpt-5.4/5.5 -> gpt-5.2
119+
prompt family) is internally consistent with `MODEL_PROFILES`.
120+
- `mergeRecord` / `applyAccumulatedOutputText` / `appendPhaseTextSegment` ordering and the
121+
delta-append fast path are correct.
122+
- `filterInput` stripIds gating (stripped only when not background mode) is correct.
123+
- Import-adapter dedup precedence, `remapActiveIndex`, and `matchDestination` index handling are
124+
correct; `previewOcChatgptImportMerge` index alignment between `merged.accounts` and
125+
`destinationAccounts` is sound.
126+
- Orchestrator atomic write (temp+rename, 0o600/0o700, retry) and target-detection scope/ambiguity
127+
logic are correct.
128+
- `convertSseToJson` pre-append size cap, malformed-chunk handling, and `readWithTimeout` cleanup
129+
are correct.

‎.github/workflows/ci.yml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
name: CI
1+
name: CI
22

33
on:
44
push:

‎lib/codex-manager.ts‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@ import {
2828
} from "./codex-manager/quota-cache-helpers.js";
2929
import { runAccountCommand } from "./codex-manager/commands/account.js";
3030
import { ACCOUNT_MANAGER_COMMANDS } from "./codex-manager/account-manager-commands.js";
31+
import { ensureFirstRunSetup } from "./runtime/first-run.js";
3132
import { runBudgetCommand } from "./codex-manager/commands/budget.js";
3233
import { runBridgeCommand } from "./codex-manager/commands/bridge.js";
3334
import { runCheckCommand } from "./codex-manager/commands/check.js";
@@ -651,6 +652,13 @@ const CLI_COMMAND_HANDLERS: ReadonlyMap<string, CliCommandHandler> = new Map<
651652
]);
652653

653654
export async function runCodexMultiAuthCli(rawArgs: string[]): Promise<number> {
655+
// Lazy install setup (audit roadmap §4.5.4): app detection, Codex app bind,
656+
// and launcher routing moved out of npm postinstall to the first CLI run.
657+
// ensureFirstRunSetup never throws; the catch is belt-and-braces so no
658+
// command can ever fail because of first-run housekeeping.
659+
await ensureFirstRunSetup({
660+
notify: (message) => console.error(`codex-multi-auth: ${message}`),
661+
}).catch(() => undefined);
654662
const startupDisplaySettings = await loadDashboardDisplaySettings();
655663
applyUiThemeFromDashboardSettings(startupDisplaySettings);
656664

‎test/codex-manager-cli.test.ts‎

Lines changed: 21 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -740,10 +740,14 @@ describe("codex manager cli commands", () => {
740740
primary: {},
741741
secondary: {},
742742
});
743-
quotaCacheMocks.loadQuotaCache.mockResolvedValue({
743+
// Fresh object per call, like the real loadQuotaCache (a fresh disk read
744+
// each time): refreshQuotaCacheForMenu rebases onto and mutates the
745+
// loaded cache before saving, and a shared singleton would leak that
746+
// mutation into every later load in the same test.
747+
quotaCacheMocks.loadQuotaCache.mockImplementation(async () => ({
744748
byAccountId: {},
745749
byEmail: {},
746-
});
750+
}));
747751
storageMocks.loadFlaggedAccounts.mockResolvedValue({
748752
version: 1,
749753
accounts: [],
@@ -9063,7 +9067,7 @@ describe("codex manager cli commands", () => {
90639067

90649068
let promptCallCount = 0;
90659069
promptLoginModeMock
9066-
.mockImplementationOnce(async (accounts) => {
9070+
.mockImplementationOnce(async (accounts, options) => {
90679071
promptCallCount += 1;
90689072
expect(promptCallCount).toBe(1);
90699073
expect(
@@ -9076,6 +9080,14 @@ describe("codex manager cli commands", () => {
90769080
queueMicrotask(() => {
90779081
releaseFirstRefresh.resolve();
90789082
});
9083+
// Wait for the first refresh to fully settle (statusMessage clears in
9084+
// the same .finally that releases the pending slot) so the second
9085+
// menu pass deterministically starts its own refresh; the refresh
9086+
// chain gained an await (the save-time cache rebase), so returning
9087+
// immediately could reach the pass-2 guard while pass 1 is pending.
9088+
await vi.waitFor(() => {
9089+
expect(options?.statusMessage?.()).toBeUndefined();
9090+
});
90799091
return { mode: "manage", deleteAccountIndex: 99 };
90809092
})
90819093
.mockImplementationOnce(async (accounts, options) => {
@@ -9248,13 +9260,18 @@ describe("codex manager cli commands", () => {
92489260

92499261
let promptCallCount = 0;
92509262
promptLoginModeMock
9251-
.mockImplementationOnce(async () => {
9263+
.mockImplementationOnce(async (_accounts, options) => {
92529264
promptCallCount += 1;
92539265
expect(promptCallCount).toBe(1);
92549266

92559267
queueMicrotask(() => {
92569268
releaseFirstRefresh.resolve();
92579269
});
9270+
// See the stale-refresh test above: settle pass 1's refresh before
9271+
// returning so pass 2's auto-fetch guard sees a free slot.
9272+
await vi.waitFor(() => {
9273+
expect(options?.statusMessage?.()).toBeUndefined();
9274+
});
92589275
return { mode: "manage", deleteAccountIndex: 99 };
92599276
})
92609277
.mockImplementationOnce(async (_accounts, options) => {

‎test/documentation.test.ts‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -697,6 +697,7 @@ describe("Documentation Integrity", () => {
697697
"*.mjs -linguist-detectable",
698698
"*.sh -linguist-detectable",
699699
"*.html -linguist-detectable",
700+
"config/schema/config.schema.json text eol=lf",
700701
]);
701702
});
702703

0 commit comments

Comments
 (0)